fix(desktop): switch to chat when resuming the open session from a full-page tab (#66875) - #66880
Conversation
|
Thanks for the focused regression fix. The premise is confirmed on current The proposed decision helper preserves the existing focus operation and restores a chat route only for an already-open session outside chat. Routing through the main session avoids promoting an open tile; Automated hermes-sweeper review. |
|
Win11 Desktop still hits this from Capabilities ( |
|
@apoapostolov thanks for the Win11 confirmation on Capabilities (
To retest on Win11 once you're on this branch:
Appreciate the extra platform coverage — let me know if anything still sticks on Windows. |
…ll-page tab Clicking the most recent session in the sidebar did nothing when a full-page route (Plugins/Artifacts/Messaging/Skills) was showing. That session is usually the main session, so onResumeSession's focusOpenSession call returned true (it fronts the session's pane inside the chat layout) and the handler then skipped navigation — but the chat layout is hidden behind the full page, so nothing visibly changed. Older sessions worked because they are neither the main session nor an open tile, so focusOpenSession returned false and the handler navigated to chat. Extract the navigation decision into a pure resumeSessionNavTarget helper: when the session is already open but the current route is not the chat view, route back to chat via the MAIN session's own route. That keeps an open tile a tile (focusOpenSession already fronted it) instead of promoting it to main, and useRouteResume treats the main session's own route as alreadyActive so no re-resume is triggered.
c39e727 to
fb1b942
Compare
erict16
left a comment
There was a problem hiding this comment.
Review (Win11 Desktop / Capabilities path)
Reproduced the same dead-click on current main from Capabilities (/skills):
- Open Chat with the active (main) session highlighted
- Open Capabilities (page rail → full-page
/skills) - Click that same / top session in the sidebar
- Before: main pane stays on Capabilities (
focusOpenSessionreturns true → nonavigate) - After this PR: should route back via
sessionRoute(main)so the chat layout is visible again
Code review
- Root cause match is correct:
wiring.tsxonResumeSessiononly navigates when!focusOpenSession(...), while the main session is almost always already open, so full-page routes (skills/artifacts/messaging/ extensions) hide the fronted pane. - Extracting
resumeSessionNavTargetis the right shape — pure helper, easy to test, keeps tile-vs-main semantics (route to MAIN when an open tile was fronted, don't promote the tile). useRouteResumetreating the main session's own route asalreadyActivemeans this is a view-only switch (no double-resume) — good.- Tests cover main-from-full-page (including
/skills), tile preservation, chat no-op, and no-main fallback.
Nit (non-blocking)
/cron is an overlay view (OVERLAY_VIEWS), not a full page like Capabilities. Routing back to chat from overlays is still reasonable UX when the user explicitly clicks a session; just noting the helper uses appViewForPath !== 'chat' for both full pages and overlays. No change required unless maintainers want overlay clicks to only dismiss the overlay without a route change.
LGTM from a Win11 daily-driver perspective — Capabilities is a frequent path. Would love this merged; happy to retest after land.
(Could not click GitHub “Approve” — no write access on the upstream repo; this is a substantive review comment instead.)
Simpler take (Win11 + Capabilities)What breaks: On Capabilities, clicking the latest session does nothing. You stay on Capabilities. Older sessions still work. Why: That session is already open behind the page. The app tries to “show” it without leaving Capabilities, so the click looks dead. What this PR does: If the session is already open and you’re not on Chat, it switches back to Chat so you can actually see it. Tests cover Capabilities ( Verdict: Looks good — please merge. Happy to retest on Win11 after it lands. (Earlier review comment was more technical; this is the plain-language version.) |
|
Closing this as superseded by upstream. The upstream fix routes the resume through a dedicated helper, export function focusedSessionNeedsRoute(focused: 'main' | 'tile' | null, workspaceIsPage: boolean): boolean {
return !focused || (focused === 'main' && workspaceIsPage)
}That covers everything this PR did:
So the branch is now redundant and conflicts with |
What does this PR do?
Fixes a desktop-app dead click: selecting the most recent session in the sidebar did nothing when a full-page route (Plugins / Artifacts / Messaging / Skills) was on screen. Selecting an older session worked, which is what made the bug so confusing.
Root cause.
onResumeSession(inapps/desktop/src/app/contrib/wiring.tsx) callsfocusOpenSession(sessionId)and only navigates when it returnsfalse:The most recent session is almost always the main session, so
focusOpenSessionreturnstrue— it fronts that session's pane inside the chat layout (revealTreePane('workspace')). But when a full-page route is showing, the chat layout is hidden behind that page, so fronting a pane in it is invisible and the top-level route never leaves/artifacts(etc.). Older sessions are neither the main session nor an open tile, sofocusOpenSessionreturnsfalseand the handler navigates to chat — hence they worked.Fix. Extract the navigation decision into a pure, tested
resumeSessionNavTargethelper. When the session is already open and the current route isn't the chat view, route back to chat via the main session's own route. Targeting the main session (not the clicked id) keeps an already-open tile a tile —focusOpenSessionhas already fronted it — instead of promoting it to main. Navigating to the main session's own route is a view-only switch:useRouteResumetreats it asalreadyActiveand does not re-resume, so there is no reload.Related Issue
Fixes #66875
Type of Change
Changes Made
apps/desktop/src/app/routes.ts— new pureresumeSessionNavTarget(sessionId, alreadyOpen, pathname, mainSessionId)helper deciding where a sidebar session click should route (ornullfor "stay put").apps/desktop/src/app/contrib/wiring.tsx—onResumeSessionnow uses the helper, so an already-open session clicked from a full-page route routes back to chat.apps/desktop/src/app/routes.resume-nav.test.ts— regression tests for the helper (not-open → own route; open on chat → no-op; open main from a page → back to chat; open tile from a page → main, not the tile; no main → fall back to clicked id).How to Test
Automated (desktop workspace):
Manual reproduction (from the issue):
Checklist
Code
fix(scope):)pytest tests/ -qand all tests pass — N/A, desktop-only TypeScript change; verified withvitest/tsc/eslint(see How to Test)Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A