fix(app): keep session timeline stable across worktree exit#436
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughSeparates timeline/session identity from directory, makes many directory inputs reactive (Accessors) to avoid remounts, preserves timeline messages across transient cache misses, introduces action-ready gating across prompt/followup/revert flows, and adds per-directory persisted stores, session-status readiness, and retainDirectory APIs. ChangesSession & Timeline / Reactive Directory / Action-Ready / Sync
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant DirLayout as DirectoryLayout
participant SDKProv as SDKProvider
participant GlobalSync as GlobalSync
participant Timeline as TimelineData
participant Snapshot as RevertSnapshot
participant Client as SessionClient
Browser->>DirLayout: route /base64(dir) change
DirLayout->>SDKProv: resolved() accessor (no keyed remount)
SDKProv->>GlobalSync: retainDirectory(directory) / child({ pin: false })
GlobalSync-->>SDKProv: { store, setStore, release }
SDKProv->>Timeline: readTimelineMessagesFromCache(raw?, lastGood)
Timeline-->>SDKProv: messages + lastGood (reused if raw missing)
SDKProv->>Snapshot: snapshot() => { client, store, setStore, promptScope }
Snapshot->>Client: client.session.revert/unrevert(payload)
Client-->>Snapshot: success / error
Snapshot->>GlobalSync: release()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Code Review
This pull request improves session state management by allowing the application to retain 'last good' messages and 'ready' states during transient cache misses, provided the session identity remains the same. It also simplifies session keys by removing directory prefixes and updates the DirectoryDataProvider to accept a directory accessor. A critical issue was identified in the directory layout where removing the 'keyed' property from a Show component while using its render argument causes stale data and a type mismatch, as the child components now expect an accessor rather than a raw string.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/app/src/pages/session/session-main-view.test.ts (1)
35-44: 💤 Low valueThe test duplicates an existing assertion.
This test case (lines 35-44) uses identical inputs to the second assertion in "does not replace the new-session home or ready timeline" (lines 25-32), both checking
routeReady: truewith matching session IDs and expectingfalse. While the descriptive name clarifies the transient cache miss intent, consider consolidating or adding a distinguishing comment to explain why both tests exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/session/session-main-view.test.ts` around lines 35 - 44, The test "does not replace an already-ready same-session timeline during a transient cache miss" duplicates the assertion already covered by "does not replace the new-session home or ready timeline" because both call shouldShowSessionOpeningState with identical inputs (activeSessionID/routeSessionID/routeReady/timelineSessionID all "ses_target" and routeReady: true); either remove this redundant test or modify its inputs to reflect the intended transient cache-miss scenario (e.g., change one of the IDs or routeReady flag) and/or add a brief clarifying comment above the test explaining why a separate case is necessary; update the test name or inputs accordingly and keep references to shouldShowSessionOpeningState and the two test names when making the change.packages/app/src/pages/session/use-session-timeline-data.test.ts (1)
5-11: 💤 Low valueMinor: The hardcoded
sessionIDin the helper could be parameterized for clarity.The
userMessagehelper hardcodessessionID: "ses_target", but test 2 uses it for"ses_source". While this doesn't affect test correctness (the function doesn't validate message sessionID), parameterizing it would improve readability.♻️ Optionally parameterize the helper
-const userMessage = (id: string): Message => +const userMessage = (id: string, sessionID = "ses_target"): Message => ({ id, role: "user", - sessionID: "ses_target", + sessionID, time: { created: 1 }, }) as Message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/session/use-session-timeline-data.test.ts` around lines 5 - 11, The helper userMessage currently hardcodes sessionID ("ses_target"); change it to accept an optional sessionID parameter (e.g., userMessage(id: string, sessionID = "ses_target"): Message) and use that value for the sessionID field, then update any test calls that expect "ses_source" to pass that value explicitly so intent is clear (leave callers that rely on the default unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/app/src/pages/session/session-main-view.test.ts`:
- Around line 35-44: The test "does not replace an already-ready same-session
timeline during a transient cache miss" duplicates the assertion already covered
by "does not replace the new-session home or ready timeline" because both call
shouldShowSessionOpeningState with identical inputs
(activeSessionID/routeSessionID/routeReady/timelineSessionID all "ses_target"
and routeReady: true); either remove this redundant test or modify its inputs to
reflect the intended transient cache-miss scenario (e.g., change one of the IDs
or routeReady flag) and/or add a brief clarifying comment above the test
explaining why a separate case is necessary; update the test name or inputs
accordingly and keep references to shouldShowSessionOpeningState and the two
test names when making the change.
In `@packages/app/src/pages/session/use-session-timeline-data.test.ts`:
- Around line 5-11: The helper userMessage currently hardcodes sessionID
("ses_target"); change it to accept an optional sessionID parameter (e.g.,
userMessage(id: string, sessionID = "ses_target"): Message) and use that value
for the sessionID field, then update any test calls that expect "ses_source" to
pass that value explicitly so intent is clear (leave callers that rely on the
default unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71ab5c67-a2ae-4500-8917-2fdbf868ca2e
📒 Files selected for processing (8)
packages/app/src/pages/directory-layout.tsxpackages/app/src/pages/session.tsxpackages/app/src/pages/session/session-main-view.test.tspackages/app/src/pages/session/session-main-view.tsxpackages/app/src/pages/session/session-view-controller.test.tspackages/app/src/pages/session/session-view-controller.tspackages/app/src/pages/session/use-session-timeline-data.test.tspackages/app/src/pages/session/use-session-timeline-data.ts
Owner
Author
|
Follow-up on the broader review checklist:
Local verification after the follow-up: bun --cwd packages/app test src/pages/session/session-view-controller.test.ts src/pages/session/use-session-timeline-data.test.ts src/pages/session/session-main-view.test.ts src/context/sync-current-child.test.ts src/context/renderer-diagnostics.test.ts
bun --cwd packages/app typecheck
git diff --checkAll passed locally. The remaining Stage 2 work should split |
Owner
Author
|
Follow-up for the latest review:
Local verification: bun --cwd packages/app test src/pages/session/use-session-followups.test.ts src/pages/session/session-view-controller.test.ts src/pages/session/use-session-timeline-data.test.ts src/pages/session/session-main-view.test.ts src/context/sync-current-child.test.ts src/context/renderer-diagnostics.test.ts
bun --cwd packages/app typecheck
git diff --checkAll passed locally. |
Owner
Author
|
Final follow-up for the repeated review checklist:
bun --cwd packages/app test src/pages/session/use-session-followups.test.ts src/pages/session/session-view-controller.test.ts src/pages/session/use-session-timeline-data.test.ts src/pages/session/session-main-view.test.ts src/context/sync-current-child.test.ts src/context/renderer-diagnostics.test.ts
bun --cwd packages/app typecheck
git diff --checkResult: 852 app tests passed locally, typecheck passed, diff check passed. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/pages/session/use-session-followups.ts (1)
77-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExisting queued follow-ups are silently dropped on upgrade — confirm this is intentional.
The old persistence key was constructed by
Persist.workspace(input.directory, …), which presumably produced a directory-namespaced key (e.g."<dir>:followup.v1"). The migration array["followup.v1"]is flat and non-namespaced, so it cannot locate those old keys; users upgrading with queued-but-unsent items will silently lose them.If this data loss is an accepted trade-off for this fix, a short inline comment (
// legacy workspace-scoped keys are intentionally not migrated) would prevent future confusion when the Stage 2 migration work resumes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/pages/session/use-session-followups.ts` around lines 77 - 90, The current persisted store uses Persist.global("session-followup.v1", ["followup.v1"]) which will not find legacy workspace-scoped keys previously created via Persist.workspace(...), causing queued follow-ups to be dropped; either migrate those legacy keys by adding the workspace-scoped legacy key(s) to the migration list (e.g. include the workspace-prefixed `"followup.v1"` variants or derive the workspace namespace used previously) so persisted(...) can pick them up, or if data loss is intentional add a clear inline comment next to the persisted/Persist.global call (referencing persisted, Persist.global, Persist.workspace and the "session-followup.v1"/"followup.v1" keys) stating that legacy workspace-scoped keys are intentionally not migrated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/app/src/pages/session/use-session-followups.ts`:
- Around line 77-90: The current persisted store uses
Persist.global("session-followup.v1", ["followup.v1"]) which will not find
legacy workspace-scoped keys previously created via Persist.workspace(...),
causing queued follow-ups to be dropped; either migrate those legacy keys by
adding the workspace-scoped legacy key(s) to the migration list (e.g. include
the workspace-prefixed `"followup.v1"` variants or derive the workspace
namespace used previously) so persisted(...) can pick them up, or if data loss
is intentional add a clear inline comment next to the persisted/Persist.global
call (referencing persisted, Persist.global, Persist.workspace and the
"session-followup.v1"/"followup.v1" keys) stating that legacy workspace-scoped
keys are intentionally not migrated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 025561ad-ef3f-4c33-97ff-de16aeb8e42a
📒 Files selected for processing (11)
packages/app/src/context/renderer-diagnostics.test.tspackages/app/src/context/sync-current-child.test.tspackages/app/src/pages/session.tsxpackages/app/src/pages/session/session-view-controller.test.tspackages/app/src/pages/session/session-view-controller.tspackages/app/src/pages/session/use-session-followups.test.tspackages/app/src/pages/session/use-session-followups.tspackages/app/src/pages/session/use-session-revert.tspackages/app/src/pages/session/use-session-review-state.tspackages/app/src/pages/session/use-session-timeline-data.test.tspackages/app/src/pages/session/use-session-timeline-data.ts
✅ Files skipped from review due to trivial changes (1)
- packages/app/src/pages/session/session-view-controller.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/app/src/pages/session/use-session-timeline-data.ts
- packages/app/src/pages/session.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/src/context/local.tsx (1)
123-147: ⚡ Quick winThe
"__unknown__"bucket pattern is architecturally guarded but represents defensive design weakness.LocalProvider is instantiated only within
<Show when={resolved()}>in directory-layout.tsx (line 73), whereresolved()returns""if the directory param is missing or invalid. This guard ensuressdk.directorywill be truthy whenever LocalProvider initializes. However, the fallback pattern at line 124 (const key = directory || "__unknown__") creates unnecessary risk and violates defensive design principles.Even with the current guard in place, if the assumption about route stability changes, or if the context is used in an unexpected way, unresolved state could persist and leak between workspaces. Better approach: assert that
sdk.directoryis always defined when needed, or explicitly reject unresolved directory state rather than silently persisting it to a shared bucket.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/context/local.tsx` around lines 123 - 147, The code uses a fallback bucket "__unknown__" in savedFor which can cause cross-workspace leakage; remove this fallback and enforce that a valid directory is provided by changing savedFor to assert/throw when directory is falsy instead of using "__unknown__", update any callers (the createMemo saved = createMemo(() => savedFor(sdk.directory).store) and setSavedSession which calls savedFor(sdk.directory).setStore) to ensure they only call savedFor with a defined sdk.directory, and keep existing cleanup/pruneLocalSavedStores logic intact (references: savedFor, createSavedEntry, savedStores, setSavedSession, pruneLocalSavedStores).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/app/src/context/local.tsx`:
- Around line 123-147: The code uses a fallback bucket "__unknown__" in savedFor
which can cause cross-workspace leakage; remove this fallback and enforce that a
valid directory is provided by changing savedFor to assert/throw when directory
is falsy instead of using "__unknown__", update any callers (the createMemo
saved = createMemo(() => savedFor(sdk.directory).store) and setSavedSession
which calls savedFor(sdk.directory).setStore) to ensure they only call savedFor
with a defined sdk.directory, and keep existing cleanup/pruneLocalSavedStores
logic intact (references: savedFor, createSavedEntry, savedStores,
setSavedSession, pruneLocalSavedStores).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3293ad3b-e530-41e6-b14e-74a1c99e93cf
📒 Files selected for processing (21)
packages/app/src/components/prompt-input.tsxpackages/app/src/components/prompt-input/submit.test.tspackages/app/src/components/prompt-input/submit.tspackages/app/src/context/global-sync/bootstrap.test.tspackages/app/src/context/global-sync/bootstrap.tspackages/app/src/context/global-sync/child-store.tspackages/app/src/context/global-sync/event-reducer.test.tspackages/app/src/context/global-sync/types.tspackages/app/src/context/local.test.tspackages/app/src/context/local.tsxpackages/app/src/context/sync.tsxpackages/app/src/pages/session.tsxpackages/app/src/pages/session/composer/session-composer-region.tsxpackages/app/src/pages/session/session-composer-region.tsxpackages/app/src/pages/session/use-session-followups.test.tspackages/app/src/pages/session/use-session-followups.tspackages/app/src/pages/session/use-session-revert.tspackages/app/src/pages/session/use-session-review-state.test.tspackages/app/src/pages/session/use-session-review-state.tspackages/app/src/pages/session/use-session-timeline-data.test.tspackages/app/src/pages/session/use-session-timeline-data.ts
✅ Files skipped from review due to trivial changes (3)
- packages/app/src/context/global-sync/event-reducer.test.ts
- packages/app/src/context/local.test.ts
- packages/app/src/context/global-sync/child-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app/src/pages/session/use-session-followups.ts
Owner
Author
|
CodeRabbit follow-up status:
Verification after the latest fixes: GitHub checks are green on head |
Owner
Author
|
Review follow-up for the latest P1/P2/P3 batch: Fixed in
Still not fixed in this PR, by scope:
Verification: |
Owner
Author
|
Latest review follow-up: Fixed in
Not fixed in this PR:
Why P1s keep appearing after many commits: This PR deliberately removed directory-driven remounts. That fixed the timeline flash root path, but it also exposed places that had been relying on remount as an implicit cleanup boundary: pending VCS promises, status hydration snapshots, LocalProvider persisted ready, revert prompt rollback, follow-up queue state, etc. The commits are not repeatedly fixing the same defect; each review is uncovering another hidden lifecycle assumption that became visible once the provider subtree stays mounted. Verification: |
Owner
Author
|
Latest review follow-up: Fixed in
Not changed in this commit:
|
Owner
Author
|
Addressed the latest P1 review in What changed:
Verification:
No P1 was rejected. The P2/P3 items around non-creating |
Owner
Author
|
Addressed the latest review in What changed:
Verification:
No P1 was rejected. The P2 items around home composer readiness, command-list scoping for queued followups, non-creating peek, and shared provider scope remain valid follow-up hardening unless you want this PR to expand beyond the Stage 1 timeline/action-readiness fix. |
Owner
Author
|
Addressed the latest P1 review in What changed:
Verification:
No P1 was rejected. P2 items around new-session/workspace readiness, unified provider scope, delayed persisted load dirty tracking, and follow-up command scoping remain valid follow-up hardening beyond this Stage 1 PR. |
Owner
Author
|
Handled the latest review in c8dcadc. Reclassification:
Verification:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/context/local.tsx (1)
234-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWait for the target local store before replaying the handoff.
Lines 241-247 treat
saved().session[session] === undefinedas authoritative even while the directory-localpersisted(...)store is still hydrating. On a cold directory switch, that can overwrite an existing saved session model with the handoff snapshot before the persisted value has been read.Suggested fix
createEffect(() => { const session = id() if (!session) return const key = handoffKey(sdk.directory, session) const next = handoff.get(key) if (!next) return - if (saved().session[session] !== undefined) { + const savedEntry = savedFor(sdk.directory) + if (!savedEntry?.readyForAction()) return + if (savedEntry.store.session[session] !== undefined) { handoff.delete(key) return } - setSavedSession(session, clone(next)) + savedEntry.setStore("session", session, clone(next)) handoff.delete(key) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/context/local.tsx` around lines 234 - 247, The replay logic in createEffect currently assumes saved().session[session] is authoritative and may overwrite a persisted session before the directory-local persisted store has finished hydrating; modify the handler so it waits for the directory-local persisted store to be ready before treating saved().session[session] as definitive — for example, query or check the persisted(...) store/readiness for sdk.directory (or a persisted.hasHydrated flag) and only run the saved().session[session] !== undefined guard after the persisted value is available; update the block around id(), handoffKey(...), handoff.get(...), setSavedSession(...) and handoff.delete(...) to first await or verify persisted(sdk.directory) hydration or existing persisted(session) value.
🧹 Nitpick comments (1)
packages/app/src/context/sync-current-child.test.ts (1)
90-99: 💤 Low valueOptional: cover
currentDirectory: undefinedinsyncChildOptionsForTargetcases.You already exercise same-dir, undefined-target, and different-target. The "current is undefined, target is set" branch (early bootstrap before
sdk.directoryresolves) flips to non-current behavior and is worth pinning down given how readiness/hydration races feature in this PR.🧪 Proposed addition
expect(syncChildOptionsForTarget({ currentDirectory: "/repo", targetDirectory: "/repo-worktree" })).toEqual({ bootstrap: false, pin: false, }) + expect(syncChildOptionsForTarget({ currentDirectory: undefined, targetDirectory: "/repo" })).toEqual({ + bootstrap: false, + pin: false, + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/context/sync-current-child.test.ts` around lines 90 - 99, Add a test case in the syncChildOptionsForTarget suite to cover the branch where currentDirectory is undefined and targetDirectory is set (to simulate early bootstrap before sdk.directory resolves); specifically, call syncChildOptionsForTarget({ currentDirectory: undefined, targetDirectory: "/repo-worktree" }) and assert it returns { bootstrap: false, pin: false } (matching the non-current-directory behavior already asserted for "/repo-worktree"); update the test block that contains syncChildOptionsForTarget to include this expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/context/local.tsx`:
- Around line 466-468: session.ready() currently returns
savedFor(sdk.directory)?.readyForAction() which can become true after a
timeout/rejected hydration while restore() still requires savedEntry?.ready(),
causing a mismatch; change ready() to mirror restore()’s check (use
savedEntry?.ready() or the same readiness predicate restore uses) so readiness
only becomes true when the session has actually been seeded, and apply the same
fix to the other readiness-related logic around the restore flow (the block
referencing savedEntry and restore at lines ~485-499) to keep both paths
consistent.
In `@packages/app/src/context/sync.tsx`:
- Around line 215-218: retainTarget may pass undefined into
globalSync.retainDirectory causing children.pin(undefined) errors; add a guard
in retainTarget (the function defined as const retainTarget) that checks whether
directory || sdk.directory resolves to a truthy string and if not returns a
no-op release handle (i.e., an object with a no-op release/unpin function)
instead of calling globalSync.retainDirectory; this prevents calling
globalSync.retainDirectory or children.pin with undefined during
bootstrap/teardown.
---
Outside diff comments:
In `@packages/app/src/context/local.tsx`:
- Around line 234-247: The replay logic in createEffect currently assumes
saved().session[session] is authoritative and may overwrite a persisted session
before the directory-local persisted store has finished hydrating; modify the
handler so it waits for the directory-local persisted store to be ready before
treating saved().session[session] as definitive — for example, query or check
the persisted(...) store/readiness for sdk.directory (or a persisted.hasHydrated
flag) and only run the saved().session[session] !== undefined guard after the
persisted value is available; update the block around id(), handoffKey(...),
handoff.get(...), setSavedSession(...) and handoff.delete(...) to first await or
verify persisted(sdk.directory) hydration or existing persisted(session) value.
---
Nitpick comments:
In `@packages/app/src/context/sync-current-child.test.ts`:
- Around line 90-99: Add a test case in the syncChildOptionsForTarget suite to
cover the branch where currentDirectory is undefined and targetDirectory is set
(to simulate early bootstrap before sdk.directory resolves); specifically, call
syncChildOptionsForTarget({ currentDirectory: undefined, targetDirectory:
"/repo-worktree" }) and assert it returns { bootstrap: false, pin: false }
(matching the non-current-directory behavior already asserted for
"/repo-worktree"); update the test block that contains syncChildOptionsForTarget
to include this expect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a5c3336-6738-4379-9a82-a61b85f9e338
📒 Files selected for processing (28)
packages/app/src/components/prompt-input.tsxpackages/app/src/components/prompt-input/readiness.test.tspackages/app/src/components/prompt-input/readiness.tspackages/app/src/components/prompt-input/submit.test.tspackages/app/src/components/prompt-input/submit.tspackages/app/src/context/global-sync.tsxpackages/app/src/context/global-sync/bootstrap.test.tspackages/app/src/context/global-sync/bootstrap.tspackages/app/src/context/global-sync/child-store.test.tspackages/app/src/context/global-sync/child-store.tspackages/app/src/context/global-sync/event-reducer.test.tspackages/app/src/context/global-sync/types.tspackages/app/src/context/local.test.tspackages/app/src/context/local.tsxpackages/app/src/context/sync-current-child.test.tspackages/app/src/context/sync.tsxpackages/app/src/pages/session.tsxpackages/app/src/pages/session/composer/session-composer-region.tsxpackages/app/src/pages/session/session-composer-region.tsxpackages/app/src/pages/session/session-model-helpers.test.tspackages/app/src/pages/session/session-model-helpers.tspackages/app/src/pages/session/use-session-followups.test.tspackages/app/src/pages/session/use-session-revert.test.tspackages/app/src/pages/session/use-session-revert.tspackages/app/src/pages/session/use-session-review-state.test.tspackages/app/src/pages/session/use-session-review-state.tspackages/app/src/pages/session/use-session-timeline-data.test.tspackages/app/src/pages/session/use-session-timeline-data.ts
✅ Files skipped from review due to trivial changes (6)
- packages/app/src/context/global-sync/event-reducer.test.ts
- packages/app/src/pages/session/session-model-helpers.ts
- packages/app/src/components/prompt-input/readiness.test.ts
- packages/app/src/pages/session/session-model-helpers.test.ts
- packages/app/src/pages/session/use-session-followups.test.ts
- packages/app/src/pages/session/use-session-timeline-data.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/app/src/context/global-sync/child-store.ts
- packages/app/src/pages/session/use-session-review-state.test.ts
- packages/app/src/context/local.test.ts
- packages/app/src/pages/session/session-composer-region.tsx
- packages/app/src/components/prompt-input/submit.ts
- packages/app/src/components/prompt-input/submit.test.ts
- packages/app/src/pages/session/use-session-review-state.ts
- packages/app/src/pages/session/use-session-timeline-data.ts
Owner
Author
|
Handled latest review in ee84e79. Reclassification:
Verification:
|
Owner
Author
|
Resolved the two remaining CodeRabbit conversations after fixing them in 3531bfa.
Verification:
|
This was referenced May 10, 2026
This was referenced May 21, 2026
1 task
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
1 participant
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.
Summary
Keep the current session timeline mounted when
exit-worktreechanges the active execution directory.directory + sessionID.provider_readystuck.Why
Fixes #432.
exit-worktreechanges where subsequent tools execute. It should not make the current session timeline look like a new timeline. The previous route/provider/timeline keys allowed directory changes and short cache misses to tear downMessageTimeline, resetting staged rendering and scroll state.Related Issue
Fixes #432
Follow-up architecture work: #441
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Please focus on the session identity boundary and the remaining directory-scoped surfaces:
activeDirectory/ route directory changes should not remount the session timeline.messagesReady=falseshould be treated as timeline refresh/render state, not action readiness.Risk Notes
Moderate UI lifecycle risk:
DirectoryLayoutno longer remounts the provider subtree on valid route directory changes. This is intentional for #432, but reviewers should check for assumptions that depended on directory-driven remounts.Follow-up queue persistence is now session-scoped/global instead of workspace-scoped so a mounted session subtree cannot keep writing queued followups into the initial directory's workspace storage. Legacy workspace-scoped follow-up queues are intentionally not migrated because their saved execution directory can be stale after worktree exit.
Local model selection remains directory-scoped in this Stage 1 patch. If a session has a saved selection in the current directory, restore from the latest user message does not overwrite it; this preserves directory-local user intent. Broader
{server, sessionID}identity, session-scoped cache, route canonicalization, and SyncProvider current-directory unpin/replace policy are tracked in #441.This PR intentionally does not migrate timeline identity, last-good cache, or follow-up persistence to
{serverKey, sessionID}. That needs a coherent SessionScope migration and is tracked in #441 rather than mixed into this Stage 1 stopgap.Manual browser reproduction of the original long-timeline
enter-worktree -> exit-worktree -> follow-uppath is still not complete. This PR should be merged only if that gap is accepted or after a reviewer runs that UI path and confirms no same-session timeline unmount, no rendered-count reset, and no scrollTop jump near 0.How To Verify
Note: one combined local test invocation that mixed the local/sync context tests with other test files hit existing Bun module-mock isolation pollution. The same test files passed when run in the isolated groups above.
Screenshots or Recordings
Not included. This PR changes lifecycle behavior for the session timeline; targeted unit coverage, diagnostics tests, typecheck, and GitHub CI were run. Manual browser reproduction of the original
exit-worktreescroll path was not run in this turn.Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Bug Fixes
Performance