Skip to content

fix(desktop): preserve profile for session windows - #48473

Open
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/desktop-session-window-profile-scope
Open

fix(desktop): preserve profile for session windows#48473
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/desktop-session-window-profile-scope

Conversation

@necoweb3

Copy link
Copy Markdown
Contributor

Summary

Pass the owning profile through desktop secondary session windows so opening a stored session in a new window resumes it against the correct profile context.

Previously, openSessionInNewWindow() only forwarded sessionId and watch, while the secondary-window URL and route-resume path had no profile hint. That meant a pop-out window could fall back to the active/default profile during cold resume, which is wrong for profile-scoped sessions.

Changes

  • Forward profile from session-row and session-actions-menu into the desktop bridge.
  • Include profile= in the secondary-window URL before the hash route.
  • Read the profile hint in the renderer and pass it into routed resume.
  • Prefer the hinted profile during session lookup and gateway swap.
  • Key the secondary-window registry by profile + session id so same-id sessions from different profiles cannot collide.
  • Add regressions for URL construction, bridge forwarding, and routed resume hinting.

Tests

node --test apps/desktop/electron/session-windows.test.cjs
npm run test:ui -- src/store/windows.test.ts src/app/session/hooks/use-route-resume.test.tsx
npm run typecheck

@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists labels Jun 18, 2026
@alt-glitch alt-glitch added comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state and removed comp/gateway Gateway runner, session dispatch, delivery comp/tui Terminal UI (ui-tui/ + tui_gateway/) labels Jun 26, 2026
@necoweb3
necoweb3 force-pushed the fix/desktop-session-window-profile-scope branch from a60a639 to 9b27313 Compare June 26, 2026 16:23
@necoweb3

Copy link
Copy Markdown
Contributor Author

Rebased this PR onto current origin/main and resolved the route-resume conflict with the newer stranded-session retry logic.

Kept both behaviors:

  • secondary session windows still pass the owning profile into routed resume
  • the newer bounded retry/exhausted-resume handling remains intact

Validation:

node --test apps\desktop\electron\session-windows.test.cjs
16 passed

npm run test:ui -- src/store/windows.test.ts src/app/session/hooks/use-route-resume.test.tsx
28 passed

@necoweb3 necoweb3 closed this Jun 28, 2026
@necoweb3
necoweb3 deleted the fix/desktop-session-window-profile-scope branch June 28, 2026 10:44
@necoweb3
necoweb3 restored the fix/desktop-session-window-profile-scope branch June 30, 2026 23:59
@necoweb3 necoweb3 reopened this Jul 1, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the profile identity through the pop-out flow. The underlying bug remains on current main: both pop-out entry points discard session.profile (apps/desktop/src/app/chat/sidebar/session-row.tsx:174, session-actions-menu.tsx:129), and Electron opens the window with only watch (apps/desktop/electron/main.ts:7424-7431).

Problems

  • The submitted route-resume change is too late for cold boot: route resume waits for gatewayState === 'open' (apps/desktop/src/app/session/hooks/use-route-resume.ts:111-150), while the PR does not make the initial gateway connection profile-aware. The boot path must consume the URL hint before connecting so a window does not depend on the default profile starting first.
  • The PR targets superseded CJS and pre-split hook paths. Current main uses apps/desktop/electron/main.ts, apps/desktop/electron/session-windows.ts, and apps/desktop/src/app/session/hooks/use-session-actions/.
  • Please remove the root-level hermes-desktop-session-window-profile-scope-pr.md; it duplicates the PR description.

Suggested changes

  • Port the flow to the current TypeScript paths, seed gateway boot from the validated URL profile, and test the first connection plus profile-qualified window reuse.

Automated hermes-sweeper review.

@@ -1047,6 +1048,7 @@ export function DesktopController() {
resumeSession,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

routeProfile reaches resume only after the gateway is open. Please also consume this secondary-window hint during gateway boot: otherwise a window must first connect to the default profile, and a default-profile boot failure prevents this resume path from running.

@@ -0,0 +1,23 @@
## Summary

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this repository-root PR-summary artifact. The PR body already documents the change, and this file is neither runtime documentation nor regression coverage.

@necoweb3
necoweb3 force-pushed the fix/desktop-session-window-profile-scope branch from 9b27313 to 6f47d9d Compare July 14, 2026 20:36
@necoweb3

Copy link
Copy Markdown
Contributor Author

Updated the branch onto current main and addressed the review feedback.

Changes:

  • Ported the session-window profile flow to the current TypeScript paths.
  • Removed the root-level PR body markdown file from the diff.
  • Seeded the initial desktop gateway boot from the validated secondary-window profile= URL hint, so a cold secondary window connects to the owning profile before route resume runs.
  • Kept route-resume profile propagation for the resumed session path.
  • Added behavior coverage proving the first backend connection uses the secondary-window profile hint.

Validation:

  • npm --prefix apps/desktop run test:desktop:platforms -- electron/session-windows.test.ts
  • npm --prefix apps/desktop run test:ui -- src/store/windows.test.ts src/app/session/hooks/use-route-resume.test.tsx src/app/gateway/hooks/use-gateway-boot.test.tsx

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1 teknium1 added area/sessions Session lifecycle, resume, persistence, history area/profiles Multi-profile isolation, HERMES_HOME scoping labels Jul 19, 2026
@DomGrieco

Copy link
Copy Markdown
Contributor

I reproduced the remaining spectator-window variant on current main: subagent rows in composer/status-stack and the delegate transcript card still call openSessionInNewWindow(..., { watch: true }) without the owning profile. The child can therefore boot against the default backend and be reset to a blank watch draft before profile discovery completes.

Current main now has the profile-aware auxiliary boot seam from #82325 (windowProfileOverride() -> getConnection(profile)), so the current-main salvage can be smaller than the older branch: carry the profile through the session-window IPC/URL and cover both spectator openers. I am preparing a test-first current-main salvage that will preserve and credit the profile-through-window design from this PR, unless you would prefer to refresh #48473 directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants