fix(desktop): keep new-session windows isolated from remembered routes - #66270
fix(desktop): keep new-session windows isolated from remembered routes#66270deniqlab wants to merge 2 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved (LGTM)
Overview
Desktop session window isolation fix. +52/0, minimal and focused.
Security
- No hardcoded secrets or credentials
Code Quality
- Clean fix for window routing isolation
Looks Good
- Minimal, targeted fix
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating both directions of the shared remembered-state path. The underlying issue is confirmed on current origin/main: apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts:64-99 persists and restores remembered state whenever the new-session window starts at NEW_CHAT_ROUTE, while apps/desktop/electron/session-windows.ts:46-47 deliberately gives those windows new=1#/.
Problems
apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts:155changes the deep-link quote replacement from one that emits an escaped quote to one that emits an unescaped quote. This unrelated edit can break/blueprintvalues containing whitespace and quotes; please retain the existing escape behavior.apps/desktop/src/app/contrib/hooks/use-desktop-integrations.test.ts:6-25only tests exported helpers, not their use by the hook. The regression is at the effect boundary, so this test would not catch removed or inverted hook guards.
Suggested changes
- Remove the unrelated escaping edit at
use-desktop-integrations.ts:155. - Add a hook-level regression test asserting a
new=1window neither reads/writes remembered state nor navigates; the related PR #67049 contains that test shape.
Automated hermes-sweeper review.
| const slots = Object.entries(payload.params || {}) | ||
| .map(([k, v]) => { | ||
| const sval = /\s/.test(v) ? `"${v.replace(/"/g, '\\"')}"` : v | ||
| const sval = /\s/.test(v) ? `"${v.replace(/"/g, '\"')}"` : v |
There was a problem hiding this comment.
This unrelated change removes the emitted backslash before an embedded quote: the previous replacement produced \", while this one produces ". Please retain the existing deep-link escaping behavior; quoted /blueprint parameter values containing whitespace and quotes otherwise become malformed.
| import { shouldPersistRememberedLocation, shouldRestoreRememberedLocation } from './use-desktop-integrations' | ||
|
|
||
| describe('shouldRestoreRememberedLocation', () => { | ||
| it('does not restore shared renderer history into a new-session window', () => { |
There was a problem hiding this comment.
This tests only a pure helper, not the hook effects that read/write shared remembered state. Please add or replace this with a renderHook regression that mocks isNewSessionWindow() and asserts no remembered-state access and no navigation.
SummarySixteen PRs address distinct Desktop persistence, restore, routing, queue, and stream-attribution facets in this issue complex. The diffs range from pin hydration and stale-route recovery to new-window/profile isolation, cross-session submit guards, queue-lineage protection, and per-stream event pins; several closed PRs remain useful as superseded or merged reference implementations. Related pull requests
Duplicates#66270 and #67049 implement the same compact new-session remembered-state guards, with #67049 adding the hook-boundary regression requested on #66270; #69815 overlaps that family for full peer windows. #70610 is superseded by merged #70986, #56444 by merged #66001, and the queue-migration portion of the complex is implemented by merged #74581. Suggested consolidationKeep #67049 open with its hook-level regression as the recorded best fix for the new-session-window isolation reports, and close #66270 as its duplicate despite the keep_open review: #67049 preserves the same two guards while avoiding #66270's unrelated quote-escaping change and addressing the review's missing effect-boundary test. Keep #69815 open only with a salvage path for its distinct win=instance URL marking after rebasing onto profile-scoped state and adding the requested hook test; separately, author action on #67823 is to rebase and split out the profile-readiness and ownership-validation pieces not already implemented by #74277, while #58332 and #77826 should remain open for their distinct polling and concurrent-stream fixes. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I65596(["issue #65596 (closed)"])
I65597(["issue #65597 (closed)"])
I65598(["issue #65598 (closed)"])
I65601(["issue #65601 (open)"])
subgraph Dup66270 ["PRs duplicating each other"]
P66270["PR #66270 (open)"]
P67049["PR #67049 (open)"]
P69815["PR #69815 (open)"]
end
P66270 -->|fixes| I65596
P66270 -->|fixes| I65597
P66270 -->|fixes| I65598
P66270 -->|fixes| I65601
class I65596 closed
class I65597 closed
class I65598 closed
class I65601 open
class P66270 open
class P67049 open
class P69815 open
class P67049 best
class P67049 best
class P67049 best
class P67049 best
class P66270 target
click I65596 "https://github.com/NousResearch/hermes-agent/issues/65596"
click I65597 "https://github.com/NousResearch/hermes-agent/issues/65597"
click I65598 "https://github.com/NousResearch/hermes-agent/issues/65598"
click I65601 "https://github.com/NousResearch/hermes-agent/issues/65601"
click P66270 "https://github.com/NousResearch/hermes-agent/pull/66270"
click P67049 "https://github.com/NousResearch/hermes-agent/pull/67049"
click P69815 "https://github.com/NousResearch/hermes-agent/pull/69815"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 16 pull requests and 21 issues in this complex. Each diff was read against this issue; Assessment working set: 207 kB of PR diffs, 89 kB of issue/PR text, 53 kB of discussion (62 comments), 66 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Summary
new=1new-session windows.Why
New-session windows were inheriting the primary renderer's remembered route/session and starting from stale conversation context instead of a clean draft.
Validation
npx vitest run --project ui src/app/contrib/hooks/use-desktop-integrations.test.tsnpx tsc --noEmit -p apps/desktop/tsconfig.jsonnpm run lintnpm run test:uinpm run buildgit diff --check