fix(chat): ensure agent persists correctly after page refresh#9832
Open
juliennonin wants to merge 3 commits intoonyx-dot-app:mainfrom
Open
fix(chat): ensure agent persists correctly after page refresh#9832juliennonin wants to merge 3 commits intoonyx-dot-app:mainfrom
juliennonin wants to merge 3 commits intoonyx-dot-app:mainfrom
Conversation
68a06d6 to
1126f74
Compare
1126f74 to
cafea39
Compare
Contributor
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
- There is a concrete regression risk in
web/src/hooks/useAgentController.ts:liveAgentcan remain memoized withoutimpliedAssistantin its dependency set, so persona/session changes may show a stale assistant state to users. web/tests/e2e/utils/chatActions.tsintroduces a reliability risk rather than a product bug: the 1s default timeout is likely too short after refresh and can cause intermittent E2E failures.- Given two medium-severity, high-confidence findings (including one user-facing behavior issue), this carries some merge risk and is better addressed before landing.
- Pay close attention to
web/src/hooks/useAgentController.tsandweb/tests/e2e/utils/chatActions.ts- stale memoized agent selection and overly aggressive timeout defaults can cause incorrect UI state and flaky tests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/src/hooks/useAgentController.ts">
<violation number="1" location="web/src/hooks/useAgentController.ts:62">
P2: `liveAgent` now returns `impliedAssistant` but the memo deps don’t include it (or its source IDs). When the session/URL persona changes, `impliedAssistant` updates while `liveAgent` stays memoized, leaving a stale agent selection.</violation>
</file>
<file name="web/tests/e2e/utils/chatActions.ts">
<violation number="1" location="web/tests/e2e/utils/chatActions.ts:21">
P2: New sidebar verification helper uses a 1s default timeout, which is too aggressive for post-refresh UI assertions and can cause flaky E2E failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Greptile SummaryThis PR fixes a race condition in Key changes:
The fix is minimal and well-scoped. The only minor nit is a leftover comment stub in the spec file (no assertion follows it) that may confuse future maintainers. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant React
participant SWR
participant useAgentController
Browser->>React: Hard page reload
React->>useAgentController: mount (availableAgents = [])
Note over useAgentController: selectedAgent = undefined (useState)<br/>impliedAssistant = undefined (availableAgents empty)<br/>liveAgent → unified/default fallback
React->>SWR: fetch /api/persona & /api/chat-session
SWR-->>React: availableAgents loaded, chatSession loaded
React->>useAgentController: re-render (availableAgents populated)
Note over useAgentController: impliedAssistant = availableAgents.find(chatSession.persona_id)<br/>liveAgent = impliedAssistant ✓ correct agent shown
Prompt To Fix All With AIThis is a comment left during a code review.
Path: web/tests/e2e/chat/live_agent.spec.ts
Line: 57
Comment:
**Dangling comment with no assertion**
The comment `// Verify the agent is still selected in the sidebar` is left without a corresponding assertion directly below it — the actual `verifyAgentIsChosenInSidebar` call for the pre-reload state was placed above at line 52, and the post-reload check is at line 63. This orphaned comment could mislead future readers into thinking a step was accidentally omitted.
```suggestion
// Hard refresh the page
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/agent_selec..." | Re-trigger Greptile |
cafea39 to
0620314
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #8087.
Fixes a race condition where the agent selection was initialized in
useStatebeforeavailableAgentsloaded, causing the wrong agent to be selected after page refresh.How Has This Been Tested?
Updated
live_agent.spec.tsto test after page refresh.Additional Options
Summary by cubic
Fixes a race condition that selected the wrong agent after a page refresh. The chosen agent now persists through reloads and after starting a chat, and stays in sync with the sidebar.
useMemoafteravailableAgentsload, using chatpersona_idordefaultAgentIdwhen present.data-testid="sidebar-section/{title}"and add a sidebar check in e2e to ensure the selected agent stays highlighted after sending a message and after a hard reload.Written for commit 6204e7f. Summary will update on new commits.