fix(desktop): navigate when resuming selected session from non-session page - #68698
Closed
re-ITRT wants to merge 1 commit into
Closed
fix(desktop): navigate when resuming selected session from non-session page#68698re-ITRT wants to merge 1 commit into
re-ITRT wants to merge 1 commit into
Conversation
…n page
When the user is viewing a non-session page (e.g. /messaging) and
clicks the already-selected session in the sidebar, focusOpenSession
short-circuits with revealTreePane('workspace') instead of navigating.
This leaves the URL at /messaging, so the sidebar highlight (which
reads currentView from the pathname) doesn't update and the user
appears to be stuck.
Fix: check routeSessionId(window.location.pathname) before returning
true. When the current page is NOT a session route, return false and
let the caller navigate — updating both the URL and the sidebar.
Collaborator
Related: #66880 addresses the same full-page session-resume symptom, but the live patches differ on open-tile semantics: #66880 preserves an open tile and returns to the main session, while this change falls through to navigate to the clicked session. Maintainer decision needed. |
Contributor
Author
|
This PR was created by an automated process error. Sorry for the noise. |
Contributor
Author
|
Thanks for the triage note. The same fix was resubmitted as #68738 with 7 regression tests and a detailed comparison with #66880.
Summary of the difference:
- #66880 patches the caller (onResumeSession in wiring.tsx) — it compensates for focusOpenSession's incorrect return downstream.
- #68738 fixes the decision function itself (focusOpenSession) by checking routeSessionId(window.location.pathname) before returning true. All callers benefit automatically.
Both approaches resolve the symptom, but the architectural root belongs in focusOpenSession — the bug is that it assumes matching $selectedStoredSessionId means the workspace pane is the current view, which is false when a route tile is focused.
The 7 tests cover: tile session, selected session on session route, selected session on /messaging, selected session on /skills, unmatched session, no selected session, and tile session on non-session route.
PR: #68738
|
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.
The Bug
When the user is viewing a non-session page (e.g.
/messaging) and clicks the already-selected session in the sidebar,focusOpenSessionshort-circuits withrevealTreePane("workspace")instead of navigating. This leaves the URL at/messaging, so the sidebar highlight (which readscurrentViewfrom the pathname) doesn't update and the user appears to be stuck — only the previous session is affected since$selectedStoredSessionIdstill points to it.But clicking any other session works fine because
$selectedStoredSessionIddoesn't match →focusOpenSessionreturns false →navigate(sessionRoute(B))executes normally.The Fix
In
focusOpenSession's second branch (the "already selected" shortcut), check if the current URL is actually a session route before returning true:Deeper Discussion — Is This the Real Root Cause?
This fix is correct but operates at the decision layer, not the data source layer. The deeper question: why is the sidebar highlight reading from the URL at all?
The URL and the pane layout tree are two separate state sources.
revealTreePane("workspace")changes the layout tree but not the URL. The sidebar readscurrentViewfromappViewForPath(pathname), which reads the URL — so even though the workspace pane is revealed, the URL hasn't changed and the highlight doesn't move.A true "fix the data source" approach would make the sidebar read from
$selectedStoredSessionIddirectly (or the pane tree) instead of derivingcurrentViewfrom the URL. That change is broader — it touchesSidebarSurface,appViewForPath, and thecurrentViewplumbing — and belongs in a separate cleanup. The fix here avoids that larger refactor while completely resolving the user-visible symptom: clicks that silently don't work.Root cause chain:
/messagingdoes not clear$selectedStoredSessionIdfocusOpenSessionassumes matching selectedId ⇒ workspace pane is the current view — wrong when a route tile is focusedThis fix addresses #2 at the right layer (the decision function that controls whether to navigate). #1 and #3 are deeper architectural topics for future work.