fix(site/src/pages/AgentsPage): support archived URL query#24742
fix(site/src/pages/AgentsPage): support archived URL query#24742
Conversation
|
/coder-agents-review |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Looks OK to me, although FE is not my expertise.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-scoped change. Replaces useState with the established useSearchParamsKey hook in 12 lines of production code. The toArchivedFilter normalizer handles garbage inputs. The test suite covers four URL-parse variants, replace semantics via back-button, and history restoration.
"I tried to build a case against this change and could not." (Pariston)
1 P1, 3 P3, 2 Nit. The P1 is a functional regression: sidebar chat links use absolute paths that strip ?archived, so clicking any chat while viewing the archived list silently resets the filter to active. Before the PR, useState survived child-route navigation because component state persists in a mounted layout. After the PR, the filter is URL-derived, and child-route URLs carry no ?archived param.
Nit [DEREM-7] Commit subject "sync archived filter URL" does not name the problem or the consequence. A body sentence explaining the prior useState-only state would help future git blame readers. (Leorio)
PS. The PR description says "container URL parsing." In this codebase, "container" means Docker/dev containers. "Page-level URL param parsing" or "archived filter URL parsing" would avoid the ambiguity.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Strong progress. 5 of 7 findings addressed in 7dc8f3c, including the P1 (sidebar links now preserve location.search) and the spy assertion issue (test file replaced with hook-level renderHook tests). The PR description is much improved.
DERM-6 (commit type fix vs feat) is contested by the maintainer; the panel will evaluate the defense in the next round.
One finding is blocking further review:
DEREM-7 (Nit, commit message): The commit subject "sync archived filter URL" does not name the problem or consequence. No response or change. The PR description body now explains the prior state well, which addresses part of the concern (the body will become the squash commit message body). If the subject is intentional, a brief acknowledgment unblocks the next round.
🤖 This review was automatically generated with Coder Agents.
johnstcn
changed the title
Updated PR title. |
|
/coder-agents-review |
There was a problem hiding this comment.
All five R1 fixes verified by the full panel. The hook extraction is clean, the NavLink search propagation works, the test strategy is solid. The PreservesArchivedFilterOnChatNavigation story is well-constructed: a probe component at the target route reads real useLocation() search, proving the param survived navigation rather than just checking the NavLink's rendered href.
"The code held up. Clean hook extraction, correct URL semantics, appropriate test strategy. Nothing left to fight." (Hisoka)
DEREM-6 (commit type fix vs feat) closed by panel vote (5/5 accept the author's defense). A filter that resets on navigation is plausibly broken behavior; the consequence of either label is changelog placement only.
1 new P3 below. Otherwise this is ready.
🤖 This review was automatically generated with Coder Agents.
| to={`/agents/${chat.id}`} | ||
| to={{ | ||
| pathname: `/agents/${chat.id}`, | ||
| search: location.search, |
There was a problem hiding this comment.
P3 [DEREM-17] Sibling navigation paths still drop ?archived, silently resetting the filter. (Kite)
"Every other navigation to
/agentsstill drops search params.ChatTopBar.tsx:90uses<Link to="/agents">(mobile back button).AgentsSidebar.tsx:1108storesstate={{ from: location.pathname }}without search. The return link at line 1359 restores the pathname but loses?archived=archived." (Kite)
The sidebar NavLink fix here is correct and complete. But the same root cause (bare /agents links dropping search params) exists in two siblings:
- ChatTopBar.tsx:90:
<Link to="/agents">(mobile back button,md:hidden). User views archived list, taps a chat, taps Back, filter resets. - AgentsSidebar.tsx:1108/1359: Settings link stores
location.pathnamewithout search; return link restores it without?archived.
Before this PR the filter was useState-only so these paths already lost it. Now that the filter is URL-derived, users have higher expectations that it will persist. The fix is the same pattern: pass { pathname, search: location.search } or store location.pathname + location.search in state.from.
Could we address these in this PR, or are they better as a follow-up?
🤖
There was a problem hiding this comment.
Will do as a follow-up.
2 participants
Persists the agents page archived filter in the URL via
?archived=archived,so deep-linking to archived agents and restoring the filter from history work
as expected. Unknown values fall back to active. Toggling back to active
removes the param from the URL so the default state has one canonical form.
Also fixes a regression that surfaced once the filter became URL-derived:
clicking a chat in the sidebar previously navigated to
/agents/:idwith nosearch params, silently resetting the filter. The sidebar's chat
NavLinknow preserves
location.search.Coverage:
useArchivedFilterParamis unit-tested withrenderHook, covering URLparsing variants and the
deleteValuesemantics for the default state.(
PreservesArchivedFilterOnChatNavigation) that renders the realNavLinkand asserts on a probe child route.
AgentsSidebar.test.tsxcoverwiring that exists on
main; they're kept here for completeness ratherthan as new feature gates.
Generated by Coder Agent.