fix(app): stabilize session composer dock scrolling#354
Open
fix(app): stabilize session composer dock scrolling#354
Conversation
Astro-Han
added
P2
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR extracts session-page controllers into dedicated composable hooks to fix a composer dock scrolling bug where height changes sometimes fail to compensate scroll position. It introduces six new hook modules managing scroll-dock layout, desktop context sync, review state, follow-ups queuing, history windowing, and revert/restore mutations, reducing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ScrollDock as ScrollDock Hook
participant DOM
participant Scroll as RAF/Timer
User->>User: Resize composer dock
User->>ScrollDock: setPromptDockRef() observes height change
ScrollDock->>ScrollDock: Calculate previousDockHeight
ScrollDock->>ScrollDock: shouldStickToBottomAfterDockResize()
alt User scrolled upward
ScrollDock->>DOM: scheduleScrollState() (no forced scroll)
else User at bottom
ScrollDock->>DOM: setCssHeight(nextDockHeight)
ScrollDock->>Scroll: forceScrollToBottom() via RAF
Scroll->>DOM: scrollTop = scrollHeight
ScrollDock->>DOM: Update --composer-dock-height CSS var
end
DOM->>Scroll: scheduleScrollState() on RAF
Scroll->>ScrollDock: calculateSessionScrollState() updates store
ScrollDock->>User: Timeline reflows, latest message visible above dock
sequenceDiagram
participant Page as Session Page
participant FollowupHook as Followups Hook
participant Sync as Sync/Storage
participant Client as SDK Client
participant Session as Server Session
Page->>FollowupHook: createSessionFollowups({ sessionID, busy, blocked, ...})
loop Auto-send gating (createEffect watches queue + conditions)
FollowupHook->>FollowupHook: Check shouldAutoSendFollowup predicate
alt Conditions met (session ready, item queued, not busy/blocked/failed/paused)
FollowupHook->>Client: useMutation(sendFollowupDraft)
Client->>Session: POST /followup
Session->>Session: Process draft
alt Success
Session-->>Client: OK
Client-->>FollowupHook: Remove from queue
FollowupHook->>Sync: Persist updated queue to workspace
else Failure
Session-->>Client: Error
Client-->>FollowupHook: Mark item failed
end
else Conditions not met
FollowupHook->>FollowupHook: Wait (busy, blocked, paused, or no item)
end
end
Page->>Page: UI binds followups state (queue, sending, failed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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: 9/10 reviews remaining, refill in 6 minutes. 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 page by extracting complex logic into modular hooks, including management for desktop context, followups, history windows, revert actions, review states, and scroll dock behavior. It also introduces a new E2E test for composer dock scrolling. The review feedback highlights several improvement opportunities: ensuring SolidJS ref callbacks handle null/undefined values to prevent runtime errors, clearing global CSS variables during component cleanup to avoid side effects, and replacing 'any' types with concrete TypeScript interfaces in the new hooks to restore type safety.
Comment on lines
+149
to
+153
| const setPromptDockRef = (el: HTMLDivElement) => { | ||
| promptDock = el | ||
| const next = Math.ceil(el.getBoundingClientRect().height) | ||
| if (next > 0) updateDockHeight(next) | ||
| } |
There was a problem hiding this comment.
The setPromptDockRef callback is missing a null check before accessing getBoundingClientRect(). SolidJS calls ref callbacks with undefined when the element is unmounted, which will cause a runtime error. Additionally, the parameter type should include undefined to correctly reflect SolidJS ref behavior.
Suggested change
| const setPromptDockRef = (el: HTMLDivElement) => { | |
| promptDock = el | |
| const next = Math.ceil(el.getBoundingClientRect().height) | |
| if (next > 0) updateDockHeight(next) | |
| } | |
| const setPromptDockRef = (el: HTMLDivElement | undefined) => { | |
| promptDock = el | |
| if (!el) return | |
| const next = Math.ceil(el.getBoundingClientRect().height) | |
| if (next > 0) updateDockHeight(next) | |
| } |
References
- In SolidJS, handle potential undefined values in refs and memos during component disposal or unmounting to prevent runtime errors.
Comment on lines
+130
to
+134
| const setContentRef = (el: HTMLDivElement) => { | ||
| content = el | ||
| autoScroll.contentRef(el) | ||
| if (scroller) scheduleScrollState(scroller) | ||
| } |
There was a problem hiding this comment.
The setContentRef callback should handle undefined values and update its parameter type accordingly to match SolidJS ref callback signatures. It is also safer to only schedule a scroll state update if the element is present.
Suggested change
| const setContentRef = (el: HTMLDivElement) => { | |
| content = el | |
| autoScroll.contentRef(el) | |
| if (scroller) scheduleScrollState(scroller) | |
| } | |
| const setContentRef = (el: HTMLDivElement | undefined) => { | |
| content = el | |
| autoScroll.contentRef(el) | |
| if (el && scroller) scheduleScrollState(scroller) | |
| } |
References
- In SolidJS, handle potential undefined values in refs and memos during component disposal or unmounting to prevent runtime errors.
Comment on lines
+189
to
+191
| onCleanup(() => { | ||
| if (scrollStateFrame !== undefined) cancelAnimationFrame(scrollStateFrame) | ||
| }) |
There was a problem hiding this comment.
The global CSS variable --composer-dock-height is set on document.documentElement but never cleared. This can lead to stale layout values if the user navigates to other pages that might inadvertently use this variable. It should be removed during cleanup to ensure side-effect isolation.
Suggested change
| onCleanup(() => { | |
| if (scrollStateFrame !== undefined) cancelAnimationFrame(scrollStateFrame) | |
| }) | |
| onCleanup(() => { | |
| if (scrollStateFrame !== undefined) cancelAnimationFrame(scrollStateFrame) | |
| document.documentElement.style.removeProperty("--composer-dock-height") | |
| }) |
References
- Any modification to global state must be restored in a cleanup hook to ensure isolation and prevent side-effects across the application lifecycle.
Comment on lines
+36
to
+44
| export function createSessionReviewState(input: { | ||
| directory: string | ||
| sessionKey: () => string | ||
| sessionID: () => string | undefined | ||
| sync: any | ||
| sdk: any | ||
| wantsReview: () => boolean | ||
| turnDiffs: () => any[] | ||
| }) { |
There was a problem hiding this comment.
The createSessionReviewState function uses any for several parameters (sync, sdk, turnDiffs), which bypasses TypeScript's type checking. This hook should use concrete types (e.g., ReturnType<typeof useSync>, ReturnType<typeof useSDK>, and VcsFileDiff[]) to maintain type safety and improve maintainability, following the pattern established in use-session-followups.ts.
Comment on lines
+18
to
+45
| export function createSessionRevert(input: { | ||
| sessionID: () => string | undefined | ||
| revertMessageID: () => string | undefined | ||
| timelineUserMessages: () => UserMessage[] | ||
| lineText: (id: string) => string | ||
| prompt: { | ||
| current: () => any[] | ||
| set: (value: any[]) => void | ||
| reset: () => void | ||
| } | ||
| sync: { | ||
| data: { message: Record<string, unknown> } | ||
| session: { | ||
| get: (sessionID: string) => { revert?: { messageID: string } } | undefined | ||
| } | ||
| } | ||
| client: { | ||
| session: { | ||
| revert: (request: { sessionID: string; messageID: string }) => Promise<{ data?: any }> | ||
| unrevert: (request: { sessionID: string }) => Promise<{ data?: any }> | ||
| } | ||
| } | ||
| halt: (sessionID: string) => Promise<unknown> | ||
| draft: (id: string) => any[] | ||
| fail: (err: unknown) => void | ||
| merge: (next: any) => void | ||
| roll: (sessionID: string, next: any) => void | ||
| }) { |
There was a problem hiding this comment.
The createSessionRevert function signature relies heavily on any types for its input configuration. This is a regression in type safety compared to the original implementation in session.tsx. It is recommended to use proper types for the prompt, sync, client, and callback functions to ensure the refactored code remains robust and easy to maintain.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/e2e/session/session-composer-dock.spec.ts`:
- Around line 969-988: The test reads `afterUserScroll` immediately after
`dock.open(...)`, which can race with dock resize/scroll; change the direct
`page.evaluate` call to poll until the viewport metrics settle using
Playwright's `expect.poll` (or `expect(...).poll`) to repeatedly evaluate the
same function that queries '[data-component="scroll-viewport"]' and returns `{
scrollTop, distanceFromBottom }`, and assert/poll for `scrollTop` to be <
`before` and `distanceFromBottom` to be >= `distanceBeforeExpansion - 40` (or
poll `distanceFromBottom` first then assert `scrollTop`) so the test waits for
the dock's resize/scroll compensation before making final assertions on
`afterUserScroll`.
In `@packages/app/src/pages/session/use-session-revert.test.ts`:
- Around line 10-18: The test and implementation rely on lexicographic
comparison of message IDs causing incorrect slicing; update the
rolledRevertItems implementation to find the index of the message whose id
equals revertMessageID and slice the messages array by index (i.e., include
items after that index) instead of using string comparison (remove any `id >=
revertMessageID` style logic in rolledRevertItems), and update the
use-session-revert.test.ts case to use non-lexical IDs (e.g., "x1","a2","b3" or
random GUID-like strings) to assert index-based behavior matches expected order.
In `@packages/app/src/pages/session/use-session-review-state.ts`:
- Around line 45-47: The state uses createStore for a single independent field
"changes"; replace the createStore/createStore-set pattern with a
createSignal<ReviewChangeMode>("turn") to simplify state: remove createStore and
setStore, introduce a signal (e.g., const [changes, setChanges] =
createSignal<ReviewChangeMode>("turn")) and update all usages of store.changes
and setStore to use changes() and setChanges(...) respectively; keep the
ReviewChangeMode type and behavior unchanged.
- Around line 179-191: The two createEffect blocks (watching
input.sessionID()/input.turnDiffs() and input.sync.data.session_diff[id]) can
both call refetchArtifactHistory() and cause duplicate fetches; consolidate them
into a single createEffect that reads input.sessionID(), input.turnDiffs(), and
input.sync.data.session_diff[id] together and only calls
refetchArtifactHistory() once when the combined guard conditions pass, or add a
short debounce/one-shot guard (e.g., a local lastRefetchSessionID or a timeout)
inside that unified effect to ensure back-to-back triggers for the same session
do not call refetchArtifactHistory() twice. Ensure you reference the existing
symbols createEffect, input.sessionID(), input.turnDiffs(),
input.sync.data.session_diff and refetchArtifactHistory when making the change.
In `@packages/app/src/pages/session/use-session-scroll-dock.test.ts`:
- Around line 109-137: The test "syncs composer height through one path and
scrolls once when sticky" modifies the global document.documentElement.style by
setting the "--composer-dock-height" CSS property, but does not clean up after
itself. Add a cleanup step after the test assertions to remove the
"--composer-dock-height" property using removeProperty method on
document.documentElement.style to prevent this mutation from affecting other
tests. Consider using an afterEach hook if this pattern is repeated across
multiple tests, or add cleanup directly at the end of this test function.
🪄 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: 8fe63b45-8370-43a1-992a-755e5a2a0ea0
📒 Files selected for processing (16)
packages/app/e2e/session/session-composer-dock.spec.tspackages/app/src/components/prompt-input/submit.test.tspackages/app/src/pages/session.tsxpackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-desktop-context.test.tspackages/app/src/pages/session/use-session-desktop-context.tspackages/app/src/pages/session/use-session-followups.test.tspackages/app/src/pages/session/use-session-followups.tspackages/app/src/pages/session/use-session-history-window.test.tspackages/app/src/pages/session/use-session-history-window.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-scroll-dock.test.tspackages/app/src/pages/session/use-session-scroll-dock.ts
Comment on lines
+969
to
+988
| await dock.open([ | ||
| { content: "first scroll dock task", status: "pending" }, | ||
| { content: "second scroll dock task", status: "pending" }, | ||
| { content: "third scroll dock task", status: "pending" }, | ||
| { content: "fourth scroll dock task expands height", status: "pending" }, | ||
| { content: "fifth scroll dock task expands height", status: "pending" }, | ||
| ]) | ||
|
|
||
| const afterUserScroll = await page.evaluate(() => { | ||
| const viewport = document.querySelector('[data-component="scroll-viewport"]') | ||
| if (!(viewport instanceof HTMLElement)) return null | ||
| return { | ||
| scrollTop: viewport.scrollTop, | ||
| distanceFromBottom: viewport.scrollHeight - viewport.clientHeight - viewport.scrollTop, | ||
| } | ||
| }) | ||
|
|
||
| expect(afterUserScroll).not.toBeNull() | ||
| expect(afterUserScroll!.scrollTop).toBeLessThan(before) | ||
| expect(afterUserScroll!.distanceFromBottom).toBeGreaterThanOrEqual(distanceBeforeExpansion - 40) |
Contributor
There was a problem hiding this comment.
Post-expansion scroll assertions should wait for observable state to settle.
afterUserScroll is captured immediately after dock.open(...), which can race dock resize + scroll compensation and cause flakiness. Poll the metric you assert on before reading final values.
💡 Proposed stabilization
- const afterUserScroll = await page.evaluate(() => {
- const viewport = document.querySelector('[data-component="scroll-viewport"]')
- if (!(viewport instanceof HTMLElement)) return null
- return {
- scrollTop: viewport.scrollTop,
- distanceFromBottom: viewport.scrollHeight - viewport.clientHeight - viewport.scrollTop,
- }
- })
-
- expect(afterUserScroll).not.toBeNull()
- expect(afterUserScroll!.scrollTop).toBeLessThan(before)
- expect(afterUserScroll!.distanceFromBottom).toBeGreaterThanOrEqual(distanceBeforeExpansion - 40)
+ let afterUserScroll: { scrollTop: number; distanceFromBottom: number } | null = null
+ await expect
+ .poll(async () => {
+ afterUserScroll = await page.evaluate(() => {
+ const viewport = document.querySelector('[data-component="scroll-viewport"]')
+ if (!(viewport instanceof HTMLElement)) return null
+ return {
+ scrollTop: viewport.scrollTop,
+ distanceFromBottom: viewport.scrollHeight - viewport.clientHeight - viewport.scrollTop,
+ }
+ })
+ return afterUserScroll?.distanceFromBottom ?? -1
+ })
+ .toBeGreaterThanOrEqual(distanceBeforeExpansion - 40)
+
+ expect(afterUserScroll).not.toBeNull()
+ expect(afterUserScroll!.scrollTop).toBeLessThan(before)As per coding guidelines: Wait on observable state with expect(...), expect.poll(...), or existing helpers instead of assuming work is finished after an action.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app/e2e/session/session-composer-dock.spec.ts` around lines 969 -
988, The test reads `afterUserScroll` immediately after `dock.open(...)`, which
can race with dock resize/scroll; change the direct `page.evaluate` call to poll
until the viewport metrics settle using Playwright's `expect.poll` (or
`expect(...).poll`) to repeatedly evaluate the same function that queries
'[data-component="scroll-viewport"]' and returns `{ scrollTop,
distanceFromBottom }`, and assert/poll for `scrollTop` to be < `before` and
`distanceFromBottom` to be >= `distanceBeforeExpansion - 40` (or poll
`distanceFromBottom` first then assert `scrollTop`) so the test waits for the
dock's resize/scroll compensation before making final assertions on
`afterUserScroll`.
Comment on lines
+10
to
+18
| rolledRevertItems({ | ||
| revertMessageID: "b", | ||
| messages: [message("a"), message("b"), message("c")], | ||
| lineText: (id) => `line:${id}`, | ||
| }), | ||
| ).toEqual([ | ||
| { id: "b", text: "line:b" }, | ||
| { id: "c", text: "line:c" }, | ||
| ]) |
Contributor
There was a problem hiding this comment.
Revert slicing is being validated with lexicographically ordered IDs only.
This test locks in behavior that depends on string comparison order (id >= revertMessageID) rather than timeline order. If message IDs are non-ordered strings, revert output can be wrong. Please switch rolledRevertItems to index-based slicing and update this test to use non-lexical IDs.
💡 Proposed fix
diff --git a/packages/app/src/pages/session/use-session-revert.ts b/packages/app/src/pages/session/use-session-revert.ts
@@
export function rolledRevertItems(input: {
revertMessageID: string | undefined
messages: UserMessage[]
lineText: (id: string) => string
}) {
const id = input.revertMessageID
if (!id) return []
- return input.messages
- .filter((item) => item.id >= id)
+ const start = input.messages.findIndex((item) => item.id === id)
+ if (start < 0) return []
+ return input.messages
+ .slice(start)
.map((item) => ({ id: item.id, text: input.lineText(item.id) }))
}diff --git a/packages/app/src/pages/session/use-session-revert.test.ts b/packages/app/src/pages/session/use-session-revert.test.ts
@@
rolledRevertItems({
- revertMessageID: "b",
- messages: [message("a"), message("b"), message("c")],
+ revertMessageID: "msg_2",
+ messages: [message("msg_10"), message("msg_2"), message("msg_30")],
lineText: (id) => `line:${id}`,
}),
).toEqual([
- { id: "b", text: "line:b" },
- { id: "c", text: "line:c" },
+ { id: "msg_2", text: "line:msg_2" },
+ { id: "msg_30", text: "line:msg_30" },
])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rolledRevertItems({ | |
| revertMessageID: "b", | |
| messages: [message("a"), message("b"), message("c")], | |
| lineText: (id) => `line:${id}`, | |
| }), | |
| ).toEqual([ | |
| { id: "b", text: "line:b" }, | |
| { id: "c", text: "line:c" }, | |
| ]) | |
| rolledRevertItems({ | |
| revertMessageID: "msg_2", | |
| messages: [message("msg_10"), message("msg_2"), message("msg_30")], | |
| lineText: (id) => `line:${id}`, | |
| }), | |
| ).toEqual([ | |
| { id: "msg_2", text: "line:msg_2" }, | |
| { id: "msg_30", text: "line:msg_30" }, | |
| ]) |
🤖 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-revert.test.ts` around lines 10 -
18, The test and implementation rely on lexicographic comparison of message IDs
causing incorrect slicing; update the rolledRevertItems implementation to find
the index of the message whose id equals revertMessageID and slice the messages
array by index (i.e., include items after that index) instead of using string
comparison (remove any `id >= revertMessageID` style logic in
rolledRevertItems), and update the use-session-revert.test.ts case to use
non-lexical IDs (e.g., "x1","a2","b3" or random GUID-like strings) to assert
index-based behavior matches expected order.
Comment on lines
+45
to
+47
| const [store, setStore] = createStore({ | ||
| changes: "turn" as ReviewChangeMode, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider: Single-field store could be createSignal.
The store for changes has only one independent field. Per coding guidelines, createStore is preferred for coupled state updated together. A createSignal<ReviewChangeMode>("turn") would be slightly simpler here. However, this is a minor style preference and the current implementation works correctly.
🤖 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-review-state.ts` around lines 45 -
47, The state uses createStore for a single independent field "changes"; replace
the createStore/createStore-set pattern with a
createSignal<ReviewChangeMode>("turn") to simplify state: remove createStore and
setStore, introduce a signal (e.g., const [changes, setChanges] =
createSignal<ReviewChangeMode>("turn")) and update all usages of store.changes
and setStore to use changes() and setChanges(...) respectively; keep the
ReviewChangeMode type and behavior unchanged.
Comment on lines
+179
to
+191
| createEffect(() => { | ||
| const id = input.sessionID() | ||
| if (!id) return | ||
| input.turnDiffs() | ||
| void refetchArtifactHistory() | ||
| }) | ||
|
|
||
| createEffect(() => { | ||
| const id = input.sessionID() | ||
| if (!id) return | ||
| if (input.sync.data.session_diff[id] === undefined) return | ||
| void refetchArtifactHistory() | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Potential redundant refetch on session change.
Two effects can trigger refetchArtifactHistory() on overlapping conditions:
- Lines 179-184: When
sessionIDorturnDiffschanges - Lines 186-191: When
session_diff[id]becomes defined
When navigating to a session, both effects may fire close together, causing duplicate fetches. Consider consolidating into one effect or adding a short debounce via a single tracking variable.
🤖 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-review-state.ts` around lines 179
- 191, The two createEffect blocks (watching input.sessionID()/input.turnDiffs()
and input.sync.data.session_diff[id]) can both call refetchArtifactHistory() and
cause duplicate fetches; consolidate them into a single createEffect that reads
input.sessionID(), input.turnDiffs(), and input.sync.data.session_diff[id]
together and only calls refetchArtifactHistory() once when the combined guard
conditions pass, or add a short debounce/one-shot guard (e.g., a local
lastRefetchSessionID or a timeout) inside that unified effect to ensure
back-to-back triggers for the same session do not call refetchArtifactHistory()
twice. Ensure you reference the existing symbols createEffect,
input.sessionID(), input.turnDiffs(), input.sync.data.session_diff and
refetchArtifactHistory when making the change.
Comment on lines
+109
to
+137
| test("syncs composer height through one path and scrolls once when sticky", () => { | ||
| const scroller = makeScroller({ | ||
| clientHeight: 400, | ||
| scrollHeight: 1000, | ||
| scrollTop: 600, | ||
| }) | ||
| const calls: number[] = [] | ||
|
|
||
| const next = syncComposerDockHeight({ | ||
| el: scroller.el, | ||
| previousDockHeight: 120, | ||
| nextDockHeight: 180, | ||
| userScrolled: false, | ||
| setCssHeight: (height) => { | ||
| document.documentElement.style.setProperty("--composer-dock-height", `${height}px`) | ||
| }, | ||
| forceScrollToBottom: () => { | ||
| calls.push(1) | ||
| scroller.el.scrollTop = scroller.el.scrollHeight | ||
| }, | ||
| scheduleScrollState: () => undefined, | ||
| fill: () => undefined, | ||
| }) | ||
|
|
||
| expect(next).toBe(180) | ||
| expect(document.documentElement.style.getPropertyValue("--composer-dock-height")).toBe("180px") | ||
| expect(calls).toHaveLength(1) | ||
| expect(scroller.top).toBe(1000) | ||
| }) |
Contributor
There was a problem hiding this comment.
Reset --composer-dock-height after the test to avoid cross-test bleed.
This test mutates global document.documentElement.style and leaves it set, which can affect subsequent cases.
💡 Proposed fix
test("syncs composer height through one path and scrolls once when sticky", () => {
+ const previousDockHeight = document.documentElement.style.getPropertyValue("--composer-dock-height")
const scroller = makeScroller({
clientHeight: 400,
scrollHeight: 1000,
scrollTop: 600,
})
const calls: number[] = []
-
- const next = syncComposerDockHeight({
- el: scroller.el,
- previousDockHeight: 120,
- nextDockHeight: 180,
- userScrolled: false,
- setCssHeight: (height) => {
- document.documentElement.style.setProperty("--composer-dock-height", `${height}px`)
- },
- forceScrollToBottom: () => {
- calls.push(1)
- scroller.el.scrollTop = scroller.el.scrollHeight
- },
- scheduleScrollState: () => undefined,
- fill: () => undefined,
- })
-
- expect(next).toBe(180)
- expect(document.documentElement.style.getPropertyValue("--composer-dock-height")).toBe("180px")
- expect(calls).toHaveLength(1)
- expect(scroller.top).toBe(1000)
+ try {
+ const next = syncComposerDockHeight({
+ el: scroller.el,
+ previousDockHeight: 120,
+ nextDockHeight: 180,
+ userScrolled: false,
+ setCssHeight: (height) => {
+ document.documentElement.style.setProperty("--composer-dock-height", `${height}px`)
+ },
+ forceScrollToBottom: () => {
+ calls.push(1)
+ scroller.el.scrollTop = scroller.el.scrollHeight
+ },
+ scheduleScrollState: () => undefined,
+ fill: () => undefined,
+ })
+
+ expect(next).toBe(180)
+ expect(document.documentElement.style.getPropertyValue("--composer-dock-height")).toBe("180px")
+ expect(calls).toHaveLength(1)
+ expect(scroller.top).toBe(1000)
+ } finally {
+ if (previousDockHeight) {
+ document.documentElement.style.setProperty("--composer-dock-height", previousDockHeight)
+ } else {
+ document.documentElement.style.removeProperty("--composer-dock-height")
+ }
+ }
})🤖 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-scroll-dock.test.ts` around lines
109 - 137, The test "syncs composer height through one path and scrolls once
when sticky" modifies the global document.documentElement.style by setting the
"--composer-dock-height" CSS property, but does not clean up after itself. Add a
cleanup step after the test assertions to remove the "--composer-dock-height"
property using removeProperty method on document.documentElement.style to
prevent this mutation from affecting other tests. Consider using an afterEach
hook if this pattern is repeated across multiple tests, or add cleanup directly
at the end of this test function.
Owner
Author
|
Updated with the follow-up session page split pass. What changed:
Verification rerun locally:
Note: |
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
packages/app/src/pages/session.tsxinto focused session controllers with unit coverage.Why
session.tsxmixed route composition, scroll dock layout, history windowing, desktop context sync, followup drafts, revert flow, and review state. The scroll bug came from duplicated dock measurement and bottom-padding paths, which could leave the latest message underneath the Composer after the dock height changed.Related Issue
Fixes #351
Human Review Status
Pending maintainer review.
Review Focus
use-session-scroll-dock.ts: sticky-bottom behavior, composer dock height sync, user-scroll preservation.session.tsx: integration wiring after controller extraction.use-session-followups.tsanduse-session-review-state.ts: extracted state ownership and unchanged behavior.session-composer-dock.spec.ts: regression coverage for the covered latest-turn case.Risk Notes
How To Verify
bun test --preload ./happydom.ts src/pages/session/use-session-scroll-dock.test.ts src/pages/session/session-auto-scroll.test.ts src/pages/session/use-session-history-window.test.ts src/pages/session/use-session-desktop-context.test.ts src/pages/session/use-session-followups.test.ts src/pages/session/use-session-revert.test.ts src/pages/session/use-session-review-state.test.ts src/pages/session/review-change-mode.test.ts src/pages/session/session-messages.test.ts src/components/prompt-input/submit.test.tsbun typecheckbun test:e2e:local -- --grep "composer dock keeps latest turn visible"git diff --checkScreenshots or Recordings
Checklist
Summary by CodeRabbit
Refactor
New Features
Tests
Style