fix(app): isolate home composer from session-only docks (#753) - #759
Merged
Conversation
The session route component serves both /:dir/session and /:dir/session/:id
and previously rendered the same SessionComposerRegion (with the shared
composer state) for both. On a session→home navigation the home variant
re-mounted while composer.dock() was still stale-true from the session
view; useSpring initialised at 1, so SessionTodoDock rendered immediately
at full height and then animated out — a phantom collapse on a page that
should never show the dock.
Split a dedicated HomeComposerRegion that has no SessionComposerState
prop and does not import any session-only dock components, so the home
path cannot mount a session dock by construction. The session variant of
SessionComposerRegion + the SessionPageComposerRegion wrapper narrow
their variant union to "session" only; renderHomeComposerRegion takes
just the props PromptInput actually needs in homeMode. Page-level
composer / timeline / followups / revert hook still run on the home
variant — they are needed for the moment a session id appears — but
nothing in the home composer subtree can read SessionComposerState.
Coverage:
- new e2e (session-composer-dock.spec.ts): seeds a todo dock, installs a
MutationObserver rooted at document.body before clicking the sidebar
"new session" button, asserts no addedNodes contain
[data-component="session-todo-dock"] during the transition. Verified
red against pre-fix code and green after.
- shell-frame-contract.test.ts: locks the new `renderHomeComposerRegion`
literal + JSX `variant="session"` attribute + `not.toContain('| "home"')`
on session.tsx; adds an import-only grep that asserts
home-composer-region.tsx does not import any of SessionTodoDock,
SessionQuestionDock, SessionPermissionContent, SessionRevertDock,
SessionFollowupDock, SessionComposerState, or their module paths.
Known residual (out of scope for this issue, follow-up will be filed):
a fast session_A → home → session_B sequence may still mount session_B's
region while composer.dock() is stale-true from session_A. Different
navigation pattern, different fix surface.
Local verify: app typecheck (tsgo -b), eslint, contract test (1179/1179),
session-composer-dock e2e (41/41 incl. new case), home/theme/a11y/
workspace-chip e2e suites (no regressions; one pre-existing failure on
`active workspace has a check icon` exists on origin/dev too, unrelated
to this change).
Fixes #753
Post-split, SessionComposerRegion + SessionPageComposerRegion + the
session.tsx call site all carried a `variant: "session"` literal that no
longer branches anything (the home path renders HomeComposerRegion).
Removing the dead prop makes the API self-explanatory and the contract
test more precise.
Contract test now asserts the actual boundary the split establishes:
- `renderHomeComposerRegion` is declared in session.tsx
- `composerHome` is wired to that function (catches a future refactor
that drops the wiring while keeping the function around)
Replaces the prior `toContain('variant="session"')` + broad
`not.toContain('| "home"')` pair, which would have tripped on any
unrelated `| "home"` union appearing in session.tsx later.
Also inlines the `composerHome={(ctx) => renderHomeComposerRegion(ctx)}`
identity wrapper.
github-actions
Bot
added
app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/pages/session.tsx, packages/app/src/pages/session/composer/home-composer-region.tsx, packages/app/src/pages/session/composer/index.ts, packages/app/src/pages/session/composer/session-composer-region.tsx, packages/app/src/pages/session/session-composer-region.tsx, packages/app/src/shell-frame-contract.test.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.
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 (7)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR fixes a visual bug where the session todo dock briefly flashes at full height when navigating from an active session back to the home view. The fix separates the shared variant-driven composer renderer into dedicated ChangesComposer Region Separation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the session composer by introducing a dedicated HomeComposerRegion component and simplifying SessionComposerRegion by removing the 'home' variant logic. This separation decouples the home view from session-specific dependencies, which is now verified by a new contract test. Additionally, an E2E test was added to ensure the todo dock does not flash when navigating from a session to the home screen. Feedback suggests extracting duplicated logic for onSubmit and previewPrompt into shared utilities to improve maintainability and removing a redundant createMemo wrapper in the new component to better align with SolidJS best practices.
Perf delta summaryComparator: pass
|
route.layoutRouteKey is already a reactive accessor from useSessionRouteKey; wrapping it in createMemo just calls through with no extra value. Aliasing the accessor directly is equivalent and avoids modeling a cargo-cult memo pattern that other composer-region files might copy.
5 tasks
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
Split a dedicated
HomeComposerRegionoff fromSessionComposerRegionso the home route can no longer mount session-only docks with stale composer state. Removes the visible 1-frame full-height todo-dock flash that appeared when navigating session → home.Why
session.tsxis a single route component serving both/:dir/sessionand/:dir/session/:id. It constructed onecomposer = createSessionComposerState({...})and passed it into the sameSessionComposerRegionfor both variants. On a session→home transition, the home variant re-mounted whilecomposer.dock()was still stale-true from the session view;useSpringinitialises from the current target on mount (no animate-from-0), soSessionTodoDockrendered immediately at full height before the todos effect re-ran and animated it out — a phantom dock collapse on a page that should never show the dock.The fix moves Home onto a region that never receives
SessionComposerStateand never importsSessionTodoDock/SessionQuestionDock/SessionPermissionContent/SessionRevertDock/SessionFollowupDock. The boundary is structural, so the next field added toSessionComposerStatecannot leak the same way.Plan iteration: 10 review rounds (3× Claude+Codex crosscheck + 5× GPT Pro external) before code was written; final plan at #753 (comment). Post-implementation crosscheck (Claude+Codex) returned 0 P0/P1; P2 findings folded into the cleanup commit.
Related Issue
Fixes #753
Human Review Status
Pending
Review Focus
home-composer-region.tsxhas zero imports from session-only docks and noSessionComposerStateprop (locked by an import-grep contract test).composerHomerewire insession.tsx: now points atrenderHomeComposerRegioninstead of branchingrenderComposerRegion("home", ...).SessionComposerRegionstill serves child-session "back to parent" + disabled-prompt path (usesuseNavigate,useSync,useSessionRouteKeyforroute.params.dir— preserved intentionally; only thehome()memo and home-only classList branches are removed).packages/app/e2e/session/session-composer-dock.spec.ts:1691installs aMutationObserverondocument.bodyBEFORE clicking the sidebar new-session button and asserts zero transient[data-component="session-todo-dock"]additions until[data-component="session-new-home"]is visible. This catches the one-frame flash that a steady-statetoHaveCount(0)assertion would miss.Risk Notes
session_A → home → session_Bsequence may still mount session_B's region whilecomposer.dock()is stale-true from session_A's snapshot. Different navigation pattern, separate flash surface. Will be tracked as a follow-up issue post-merge. The cheapest remedy when it surfaces is a per-call{ from: 0 }option onuseSpringapplied atsession-composer-region.tsx.session-composer-dock.spec.ts:1691is the precise dynamic contract — TDD-verified red against pre-fix code (the dock element was recorded inaddedNodesduring the transition) and green after the split.How To Verify
Screenshots or Recordings
Not applicable — the bug is a single-frame DOM mutation during route transition; static screenshots cannot capture it. The new MutationObserver-based e2e at
packages/app/e2e/session/session-composer-dock.spec.ts:1691records the dock element being added to the DOM during the transition and asserts it is empty post-fix. Verified red→green.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
Bug Fixes
Tests