fix: stabilize session opening state#969
Conversation
|
Warning Review limit reached
More reviews will be available in 24 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR fixes the blank half-loaded session state that occurs during cross-session switching by introducing a ChangesSession Opening Skeleton Overlay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/prompt-input.tsx, packages/app/src/components/prompt-input/attachments.test.ts, packages/app/src/components/prompt-input/attachments.ts, packages/app/src/components/prompt-input/readiness.test.ts, packages/app/src/components/prompt-input/readiness.ts, packages/app/src/pages/session/session-main-view.tsx, packages/app/src/pages/session/session-opening-skeleton.test.ts, packages/app/src/pages/session/session-opening-skeleton.tsx, packages/app/src/pages/session/use-session-timeline-data.test.ts, packages/app/src/pages/session/use-session-timeline-data.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request introduces a timeline-shaped SessionOpeningSkeleton component to replace the previous spinner and action buttons during session initialization, managing its transitions and visibility within SessionMainView. It also refactors PromptInput to allow local text input, navigation, and plain-text pasting while actions are not ready, while still restricting file/image attachments. The review feedback suggests tightening the paste handler in PromptInput to prevent bypassing file attachment blocks with native clipboard images, and optimizing the initial state of openingSkeletonMounted in SessionMainView to avoid an unnecessary re-render on mount.
Perf delta summaryComparator: pass
|
Replace the hand-drawn skeleton turns with `[data-component="user-message"]` and `[data-component="assistant-message"]` DOM so the placeholder bubble inherits real `user-message-text` geometry (inline-block, max-width 75%, fit-content, brand bubble fill). User-row placeholder lines use `ch` widths to let the real inline-block padding/radius drive the bubble shape; assistant rows use `%` widths against the 100%-wide assistant container. Adds a small `[data-slot="skeleton-line"]` style in `message-part.css` so the new placeholder shares palette and dark-mode handling with the surrounding message styles, and tightens the `assistant-message:has(skeleton-line)` gap to 6px so single-paragraph skeleton lines don't read as separate parts. Tests assert the real selectors and the body>text nesting that downstream CSS relies on.
Pass `timelineMessages` so the skeleton turn count and user/assistant order match the conversation that is about to render. Without this, every opening state looked identical regardless of which session was being opened, which defeats the point of mirroring the target layout.
Two readiness bypasses surfaced by code review on PR #969: 1. `handleGlobalDrop` returned early when `externalReady()` was false without calling `preventDefault()` first. Dropping a file during the opening state fell through to Electron's default handler, which navigates the window to the file URL and discards the in-flight draft. 2. `handlePaste` only checked readiness on the upstream `onPaste` handler when the clipboard surfaced a `file` item. Native screenshots arrive with no clipboard file item, so the paste fell through to the `readClipboardImage()` desktop fallback and silently attached the image (or fired an unsupported-image toast) while the composer's attachment controls were still disabled. Cancel the native drop before bailing, and gate the `readClipboardImage()` fallback with the same `externalReady` check. Regression tests cover both paths via the existing `createPromptAttachments` factory.
Initializing the mount signal to false forced an extra render on first mount whenever showSessionOpeningState() already resolved to true. Seed it with the current value so the overlay element exists in render 1.
The skeleton overlay does not render retry or new-session actions, so the onRetryOpenSession / onOpenNewSession props on SessionMainView and their providers in session.tsx were dead. Removed both ends along with the now-unused decode64 and useShellSurface imports they pulled in.
The "!" shortcut that flips the prompt into shell mode skipped the readiness check, so users could change modes while the session was still opening. The button and command paths already routed through a guarded setMode. Extract a shouldActivateShellModeFromBang helper that considers action readiness alongside cursor position and current mode, then use it from the keydown handler.
Both the native and browser file pickers open async dialogs that can outlive the readiness window the entry guard checked. Add an isReady gate inside pickAttachments and re-check actionReady at the browser file input onChange so opening sessions don't accept stale picks.
|
Addressed the review:
Pre-existing test failures on |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/app/e2e/snap/session-opening-skeleton.snap.ts (1)
27-27: 💤 Low valueConsider whether the 180-second timeout is necessary.
The test timeout is set to 3 minutes, which is very generous for a snapshot test that navigates, waits for theme boot, mounts a fixture, and captures two screenshots. Even accounting for CI variability and cold starts, this seems excessive—most Playwright tests complete in seconds. If the test consistently finishes much faster, consider reducing this to a more typical value (e.g., 60 seconds) to catch performance regressions earlier.
🤖 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/e2e/snap/session-opening-skeleton.snap.ts` at line 27, The test currently sets a very long 180_000ms timeout via test.setTimeout; reduce it to a more typical value (e.g., 60_000) to fail faster on slow tests while still allowing CI variability, or make it configurable if this specific snapshot occasionally needs more time; update the test.setTimeout call accordingly (replace 180_000 with 60_000 or a reference to a TIMEOUT constant) so the change is clear in the session-opening-skeleton snapshot test.
🤖 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/e2e/snap/session-opening-skeleton.snap.ts`:
- Line 27: The test currently sets a very long 180_000ms timeout via
test.setTimeout; reduce it to a more typical value (e.g., 60_000) to fail faster
on slow tests while still allowing CI variability, or make it configurable if
this specific snapshot occasionally needs more time; update the test.setTimeout
call accordingly (replace 180_000 with 60_000 or a reference to a TIMEOUT
constant) so the change is clear in the session-opening-skeleton snapshot test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cf928ae-5bd3-4011-a617-bf1eef332806
📒 Files selected for processing (18)
packages/app/e2e/snap/fixtures/session-opening-skeleton-fixture.tsxpackages/app/e2e/snap/session-opening-skeleton.snap.tspackages/app/src/components/prompt-input.tsxpackages/app/src/components/prompt-input/attachments.test.tspackages/app/src/components/prompt-input/attachments.tspackages/app/src/components/prompt-input/keydown.tspackages/app/src/components/prompt-input/pick-attachments.test.tspackages/app/src/components/prompt-input/pick-attachments.tspackages/app/src/components/prompt-input/readiness.test.tspackages/app/src/components/prompt-input/readiness.tspackages/app/src/pages/session.tsxpackages/app/src/pages/session/session-main-view.tsxpackages/app/src/pages/session/session-opening-skeleton.test.tspackages/app/src/pages/session/session-opening-skeleton.tsxpackages/app/src/pages/session/use-session-timeline-data.test.tspackages/app/src/pages/session/use-session-timeline-data.tspackages/app/src/shell-frame-contract.test.tspackages/ui/src/components/message-part.css
💤 Files with no reviewable changes (1)
- packages/app/src/pages/session.tsx
Extract shouldExitShellModeOnBackspace mirroring the bang shortcut helper so the shell-empty Backspace path honors the same readiness contract as other mode toggles.
Bump PawWork desktop release version to 2026.5.29. Changes since v2026.5.28: - feat(settings): move Connections to Integrations + global toast monitor (#975) - feat(ui): display cache hit rate with one decimal place (#967) - fix: stabilize session opening state (#969) - fix(session): repair stale paginated question blockers (#962) - fix(ui): refit read-file icon into 0-20 viewBox (#964) - fix: allow running tools to expand - fix: add run lifecycle diagnostics - fix: harden Electron repair fallback - refactor: remove legacy theme choices - ci: stabilize e2e Playwright install and PR triage paths Verification: diff scope matches prior release bump (#958) — only the version string in packages/desktop-electron/package.json and the workspace entry in bun.lock. All CI checks green (e2e-artifacts required a rerun due to Playwright browser install flake unrelated to this change).
Fixes #985. Root cause: - SessionContextUsage used Solid's callback-form non-keyed Show accessors for the active context label/status. - PR #969 kept the composer mounted during opening; when switching sessions, that exposed a stale Show accessor read after context metrics disappeared. Changes: - Derive the context label with a memo instead of reading a Show callback accessor later. - Render the tooltip through a small content component that accepts stable values. - Add a browser-condition regression that renders SessionContextUsage and clears context metrics, so reverting to the callback-form Show pattern fails. Verification: - bun test --preload ./happydom.ts ./src/components/session/session-context-metrics.test.ts ./src/components/session-context-usage.test.ts - bun run typecheck - bun run snap session-context-cache - GitHub PR checks passed except perf-probe-baseline, which was skipped because it is stuck in the Install Playwright browsers step (`bunx playwright install --with-deps chromium`). The e2e-artifacts workflow already avoids that Playwright browser download path and passed on this head, so this is treated as a CI workflow hang rather than a PR regression.
Summary
Fixes session switching so PawWork no longer renders a half-open blank session state. The target session now stays in an opening skeleton until both session info and its message cache are hydrated, and the composer remains editable while send/external actions stay blocked.
Why
Switching sessions could briefly expose raw target state, including a
ses_...header, blank timeline, and loading prompt placeholder. In some races it could stay there. The fix keeps route display readiness tied to the real target session cache and replaces the old spinner card with a timeline-shaped skeleton.Related Issue
Fixes #965
Human Review Status
Pending
Review Focus
Please check the route readiness gate and the composer readiness split. The intended contract is: target timeline only renders after target session info plus target message cache are present; local typing remains available; sending, file attachments, mode changes, and other external actions remain disabled until action readiness returns.
Risk Notes
UI behavior changes are limited to session opening and composer readiness. No platform, packaging, updater, signing, dependency, permission, migration, deletion, docs, release note, credential, or generated-content surfaces were changed.
How To Verify
Screenshots or Recordings
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests