fix: keep timeline visible while switching sessions#313
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🧰 Additional context used📓 Path-based instructions (1)packages/app/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (packages/app/AGENTS.md)
Files:
🧠 Learnings (13)📓 Common learnings📚 Learning: 2026-04-26T16:34:57.130ZApplied to files:
📚 Learning: 2026-04-23T15:26:07.250ZApplied to files:
📚 Learning: 2026-04-27T10:33:12.228ZApplied to files:
📚 Learning: 2026-04-28T13:10:01.345ZApplied to files:
📚 Learning: 2026-04-28T04:38:11.771ZApplied to files:
📚 Learning: 2026-04-28T05:36:22.128ZApplied to files:
📚 Learning: 2026-04-24T03:51:56.211ZApplied to files:
📚 Learning: 2026-04-28T12:01:16.559ZApplied to files:
📚 Learning: 2026-04-28T12:01:13.981ZApplied to files:
📚 Learning: 2026-04-28T18:53:46.234ZApplied to files:
📚 Learning: 2026-04-23T07:23:23.849ZApplied to files:
📚 Learning: 2026-04-23T15:10:21.635ZApplied to files:
🔇 Additional comments (7)
📝 WalkthroughWalkthroughIntroduces a timeline-specific rendered session identity and message set, passes timeline-scoped Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Nav as Navigation
participant Session as session.tsx
participant Timeline as MessageTimeline
participant Sync as sync.data.message
rect rgba(255, 200, 100, 0.5)
Note over Nav,Session: Old Behavior (Blank Frame)
Nav->>Session: params.id -> target
Session->>Session: messagesReady() = false
Session->>Timeline: <Show when={false}> → unmount
Timeline->>Timeline: BLANK FRAME
Sync->>Session: target messages arrive
Session->>Timeline: re-mount with target messages
end
rect rgba(100, 200, 100, 0.5)
Note over Nav,Sync: New Behavior (Timeline-stable)
Nav->>Session: params.id -> target
Session->>Session: timelineSessionID stays as current
Session->>Timeline: remain mounted with current props
Sync->>Session: target messages arrive
Session->>Session: compute nextTimelineSessionID() -> target
Session->>Timeline: update props -> switch to target smoothly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Review rate limit: 0/3 reviews remaining, refill in 56 minutes and 40 seconds. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to maintain the visibility of the current session's messages during a transition to a new session, preventing the UI from clearing until the new data is ready. It adds a nextTimelineSessionID helper and updates the MessageTimeline component to accept session data as props rather than deriving it from the route parameters. A critical piece of feedback identifies a state mismatch where, although the UI preserves the old session's messages, other reactive signals like history loading and scroll handling remain tied to the new route ID, potentially causing inconsistent behavior during the loading phase.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/src/pages/session.tsx`:
- Around line 517-568: Timeline UI state (timelineSessionID) can lag behind
params.id causing navigateMessageByOffset and useSessionHashScroll to act on the
wrong session; fix by reading navigation and scroll helpers from the
timeline-specific signals instead of route signals or by gating them until
timelineSessionID() === params.id. Concretely, update usages of
navigateMessageByOffset, useSessionHashScroll (and any reads of
visibleUserMessages, timelineHistoryMore, timelineHistoryLoading) to consume
timelineVisibleUserMessages(), timelineHistoryMore(), timelineHistoryLoading()
(or wrap calls in a guard if timelineSessionID() !== params.id) so keyboard
navigation and pending hash-scroll always operate on the same session rendered
by timelineSessionID().
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1910f31-b69d-44ba-abbb-b572e1a9ed75
📒 Files selected for processing (1)
packages/app/src/pages/session.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/app/AGENTS.md)
Always prefer
createStoreover multiplecreateSignalcalls in SolidJS
Files:
packages/app/src/pages/session.tsx
🧠 Learnings (9)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/test/session/subagent-lifecycle-integration.test.ts:41-47
Timestamp: 2026-04-28T11:24:35.312Z
Learning: In `packages/opencode/test/session/subagent-lifecycle-integration.test.ts` (Astro-Han/pawwork, PR `#287`), the test suite intentionally uses a manual `run` helper (`Effect.runPromise(program.pipe(Effect.provide(Layer.mergeAll(SubagentRun.defaultLayer, Session.defaultLayer)), Effect.orDie))`) and raw `bun:test` test cases rather than the `testEffect(...)` harness. All 27 lifecycle tests pass with this pattern. Migration to `testEffect` + `it.live(...)` is deferred to a dedicated test-harness sweep PR to avoid fixture drift before merge. Do NOT re-flag the manual `Effect.runPromise` runner in this file as needing harness migration until that sweep PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 247
File: packages/ui/src/components/message-part.tsx:1322-1324
Timestamp: 2026-04-26T16:34:57.130Z
Learning: In Astro-Han/pawwork (`packages/ui/src/components/message-part.tsx`), the `taskId` createMemo and `childSessionId` createMemo both intentionally read only from `partMetadata().sessionId` (populated post-execution), not from `input.task_id` / `input.subagent_session_id`. This has always been the case — the original code never read the input field either. Adding an `input.subagent_session_id` fallback would be a new capability, not a bug fix. Do NOT flag the absence of this fallback as a regression in PR `#247` or future PRs unless there is a concrete case where metadata is not populated.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/src/pages/layout/sidebar-items.tsx:102-107
Timestamp: 2026-04-23T15:26:07.250Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/layout/sidebar-items.tsx`), the `indicator()` function in `SessionRow` intentionally renders `props.leadingSlot` (the pin button) only as a fallback when no status indicator (running/permission/error/unseen) is active. When a higher-priority status wins the slot, the pin button is removed from the DOM — this is a deliberate design choice for the merged leading slot (`#150`). The keyboard unpin path is preserved via: (1) focusing the row anchor triggers `group-focus-within` which reveals the dots menu trigger, then Tab → Enter → "Unpin Session"; (2) the context menu (right-click / Shift+F10) exposes "Unpin Session". The "always render + CSS overlay" approach was considered but rejected due to z-index/pointer-events complexity; residual `...` slot behavior is tracked in `#192`. Do NOT flag the absence of the pin button from the DOM when a status is active as an accessibility regression.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/patch/index.ts:337-346
Timestamp: 2026-04-28T04:38:05.946Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/patch/index.ts`), the BOM-transition surfacing gap — where `Bom.split` strips BOM before building `unified_diff`/`new_content` but `Bom.join` later re-attaches BOM on disk write, meaning BOM changes are not reflected in the diff payload — is intentionally deferred. PR `#270` is an upstream-sync graft; fixing the issue here would mix refactor + bugfix intents and drift the diff from the upstream baseline needed for clean future grafts. A dedicated follow-up PR (or upstream report) will address this. Do NOT re-flag the missing BOM-change surfacing in `ApplyPatchFileUpdate`/`ApplyPatchFileChange` as a blocking issue in PR `#270` or in future sync PRs that carry the same upstream baseline.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/tool/agent.ts:89-102
Timestamp: 2026-04-28T13:10:01.345Z
Learning: In `packages/opencode/src/tool/agent.ts` (Astro-Han/pawwork, PR `#287`), `makeReadLastCompletedAssistantText` intentionally scans the full child session message history (no `limit`) to find the latest completed assistant text part for `partial_result` on cancellation. Cancellation is a rare path so the extra read cost is acceptable; tool-heavy children can push the last stable assistant text far back. Do NOT suggest re-adding a fixed small limit (e.g., `limit: 5`) to this function.
📚 Learning: 2026-04-26T16:34:57.130Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 247
File: packages/ui/src/components/message-part.tsx:1322-1324
Timestamp: 2026-04-26T16:34:57.130Z
Learning: In Astro-Han/pawwork (`packages/ui/src/components/message-part.tsx`), the `taskId` createMemo and `childSessionId` createMemo both intentionally read only from `partMetadata().sessionId` (populated post-execution), not from `input.task_id` / `input.subagent_session_id`. This has always been the case — the original code never read the input field either. Adding an `input.subagent_session_id` fallback would be a new capability, not a bug fix. Do NOT flag the absence of this fallback as a regression in PR `#247` or future PRs unless there is a concrete case where metadata is not populated.
Applied to files:
packages/app/src/pages/session.tsx
📚 Learning: 2026-04-27T10:33:12.228Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
Applied to files:
packages/app/src/pages/session.tsx
📚 Learning: 2026-04-28T13:10:01.345Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/tool/agent.ts:89-102
Timestamp: 2026-04-28T13:10:01.345Z
Learning: In `packages/opencode/src/tool/agent.ts` (Astro-Han/pawwork, PR `#287`), `makeReadLastCompletedAssistantText` intentionally scans the full child session message history (no `limit`) to find the latest completed assistant text part for `partial_result` on cancellation. Cancellation is a rare path so the extra read cost is acceptable; tool-heavy children can push the last stable assistant text far back. Do NOT suggest re-adding a fixed small limit (e.g., `limit: 5`) to this function.
Applied to files:
packages/app/src/pages/session.tsx
📚 Learning: 2026-04-23T15:26:07.250Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/src/pages/layout/sidebar-items.tsx:102-107
Timestamp: 2026-04-23T15:26:07.250Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/layout/sidebar-items.tsx`), the `indicator()` function in `SessionRow` intentionally renders `props.leadingSlot` (the pin button) only as a fallback when no status indicator (running/permission/error/unseen) is active. When a higher-priority status wins the slot, the pin button is removed from the DOM — this is a deliberate design choice for the merged leading slot (`#150`). The keyboard unpin path is preserved via: (1) focusing the row anchor triggers `group-focus-within` which reveals the dots menu trigger, then Tab → Enter → "Unpin Session"; (2) the context menu (right-click / Shift+F10) exposes "Unpin Session". The "always render + CSS overlay" approach was considered but rejected due to z-index/pointer-events complexity; residual `...` slot behavior is tracked in `#192`. Do NOT flag the absence of the pin button from the DOM when a status is active as an accessibility regression.
Applied to files:
packages/app/src/pages/session.tsx
📚 Learning: 2026-04-28T04:38:11.771Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/agent.ts:23-27
Timestamp: 2026-04-28T04:38:11.771Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/tool/agent.ts`), the `subagent_session_id` field in the `Parameters` schema accepts any `Schema.String` rather than a branded `SessionID`. This is inherited upstream behavior (adopted in PR `#270`, an upstream-sync graft of upstream PR `#23244`). The fix — validating `subagent_session_id` as a `SessionID` brand up front so malformed/typo'd IDs fail explicitly rather than silently forking a new subagent session — is intentionally deferred to a follow-up PR or upstream report. Do NOT re-flag this as a blocking issue in PR `#270` or in future upstream-sync PRs that carry the same schema; flag it only in a PawWork-authored PR that touches `agent.ts` parameter validation.
Applied to files:
packages/app/src/pages/session.tsx
📚 Learning: 2026-04-28T05:36:22.128Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/session/compaction.ts:169-191
Timestamp: 2026-04-28T05:36:22.128Z
Learning: In `packages/opencode/src/session/compaction.ts` (Astro-Han/pawwork), the internal helper `splitTurn` intentionally returns a raw `Effect.gen(...)` rather than being wrapped with `Effect.fnUntraced`. Converting `splitTurn` (and similar plain-Effect.gen internal helpers in this file, e.g. `turns`, `completedCompactions`, `buildPrompt`) to `Effect.fnUntraced` is a repo-wide convention sweep deferred to a dedicated follow-up PR. The same sweep covers `tool/tool.ts` and other helpers flagged in the same sync. Do NOT re-flag the absence of `Effect.fnUntraced` on `splitTurn` or other internal helpers in `compaction.ts` during upstream-sync PRs; flag it only in the dedicated convention-sweep PR.
Applied to files:
packages/app/src/pages/session.tsx
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.
Applied to files:
packages/app/src/pages/session.tsx
📚 Learning: 2026-04-23T15:10:21.635Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 191
File: packages/app/src/components/session/pawwork-skill-meta.ts:38-39
Timestamp: 2026-04-23T15:10:21.635Z
Learning: This repo configures Tailwind v4 with `--color-*: initial`, which effectively breaks standard Tailwind palette utilities (e.g., `text-violet-500` can resolve to no CSS variable and render as a no-op/black). For brand/accent colors that are not backed by semantic design tokens, use inline styles with the exact hex value (e.g., `style={{ color: '#8B5FBF' }}` / `homeIconStyle: { color: '#8B5FBF' }`) and add a short comment explaining that Tailwind palette utilities won’t work due to the `--color-*: initial` setup. Do not suggest replacing these inline hex colors with Tailwind palette classes anywhere in this repo.
Applied to files:
packages/app/src/pages/session.tsx
🔇 Additional comments (2)
packages/app/src/pages/session.tsx (2)
1641-1691: Good: history backfill/fill now follows the visible timeline session.This is the right boundary for the blank-frame fix; it keeps incremental reveal and load-more behavior attached to the mounted timeline instead of the route session.
2126-2162: Good mount boundary forMessageTimeline.Passing
sessionID,sessionKey, andsessionMessagesfrom the timeline state should keep the component identity aligned with the session that is actually being rendered.
7 tasks
This was referenced May 1, 2026
This was referenced May 10, 2026
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 previous rendered session timeline visible while the target session messages are still loading, then switch the timeline once the target message cache is ready. Add a small timeline-session state helper plus coverage for retaining the visible timeline session, and extend the session switching E2E probe to reject 0-message samples after navigation reaches the target session.
Why
Fixes #311. Session switching could briefly route to the target session before its message cache existed, which made the session page unmount
MessageTimelineand show an empty conversation frame.Related Issue
Fixes #311
How To Verify
Screenshots or Recordings
Not included. This fixes a transient session-switch frame, and the behavior is covered by the E2E timeline sampling probe.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Tests
Refactor
UX