Fix session timeline scroll containment - #1362
Merged
Merged
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
📝 WalkthroughWalkthroughThe session UI now constrains layout overflow and minimum dimensions. Timeline scroll adjustment runs during layout and reevaluates when processing starts. Tests cover panel styles, scrolling, user-position preservation, and processing-indicator height changes. ChangesSession UI behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![open-inspect[bot]](https://avatars.githubusercontent.com/in/2661131?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
Reviewed PR #1362, “Fix session timeline scroll containment” by @ColeMurray. The PR changes 5 files with +51/-8 and keeps the fix narrowly focused on scroll containment and timeline bottom-follow behavior.
Critical Issues
None found.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- The panel overflow containment is applied at the resizable panel boundary where the competing scroll container is introduced.
- Moving the bottom-follow adjustment to a layout effect and including
isProcessingcovers the indicator height-change case without mixing it into the history-prepend compensation effect. - The added regression tests are focused on the changed behavior.
Questions
None.
Verification
- Reviewed
gh pr diff 1362and the fetched PR head diff. git diff --check main...origin/pr-1362passed.- Focused web tests could not be run locally because dependencies were unavailable in the PR worktree (
vitestwas not installed).
Verdict
Approve.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/web/src/components/session-timeline.test.tsx`:
- Around line 605-613: Update the mocked timeline element in the test around
SessionTimeline to define a scrollTop setter that clamps assigned values to
scrollHeight minus clientHeight, matching browser behavior. Keep the existing
clientHeight and scrollHeight values, and change the assertion to expect the
clamped position of 800.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7625a167-8fbd-4f7e-a8e0-c7d6d532bdd1
📒 Files selected for processing (5)
packages/web/src/app/(app)/session/[id]/page.tsxpackages/web/src/components/session-desktop-layout.test.tsxpackages/web/src/components/session-desktop-layout.tsxpackages/web/src/components/session-timeline.test.tsxpackages/web/src/components/session-timeline.tsx
Contributor
There was a problem hiding this comment.
[deep review] Requesting changes because the panel fix still leaves hidden-overflow ancestors as scroll containers, while the regression tests mock away the relevant DOM and effect timing. The new timeline test also pushes an already-large test file from 999 to 1,015 lines; the focused scroll suite should be decomposed rather than growing this monolith.
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/components/session-timeline-scroll.test.tsx (1)
41-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the default helper argument.
Line 45 defines the default value inline. Declare the empty value once as a named constant and use it for
extra.Proposed change
+const EMPTY_TOOL_EVENT_EXTRA: Partial<Extract<SandboxEvent, { type: "tool_call" }>> = {}; + function toolEvent( tool: string, callId: string, timestamp: number, - extra: Partial<Extract<SandboxEvent, { type: "tool_call" }>> = {} + extra: Partial<Extract<SandboxEvent, { type: "tool_call" }>> = EMPTY_TOOL_EVENT_EXTRA ): Extract<SandboxEvent, { type: "tool_call" }> {As per coding guidelines, define each TypeScript default value exactly once as a named constant and import it wherever needed.
🤖 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/web/src/components/session-timeline-scroll.test.tsx` around lines 41 - 58, Update the toolEvent helper to define the empty extra-value object once as a named constant, then use that constant as the default for extra instead of the inline literal. Keep the helper’s returned event behavior unchanged.Source: Coding guidelines
🤖 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/web/src/components/session-timeline-scroll.test.tsx`:
- Around line 41-58: Update the toolEvent helper to define the empty extra-value
object once as a named constant, then use that constant as the default for extra
instead of the inline literal. Keep the helper’s returned event behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2f8e6a3-06d8-457a-beb6-24ceeeafb1d2
📒 Files selected for processing (5)
packages/web/src/app/(app)/session/[id]/page.tsxpackages/web/src/components/session-desktop-layout.test.tsxpackages/web/src/components/session-desktop-layout.tsxpackages/web/src/components/session-timeline-scroll.test.tsxpackages/web/src/components/session-timeline.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/web/src/components/session-desktop-layout.tsx
- packages/web/src/app/(app)/session/[id]/page.tsx
- packages/web/src/components/session-desktop-layout.test.tsx
Owner
Author
|
@open-inspect[bot] please re-review and approve |
Contributor
|
Re-reviewed PR #1362 and approved it. No blocking findings found. Verification: focused web tests passed locally (27 tests), and the PR status checks are green. |
Contributor
There was a problem hiding this comment.
Re-reviewed the latest revision. The prior concern is resolved: the scroll regression coverage is now in a focused suite, the layout test uses the real resizable panel components, and overflow/min-size containment remains targeted at the relevant panel boundaries. No blocking findings.
ColeMurray
added a commit
that referenced
this pull request
Aug 13, 2026
## Summary One-word layout fix: add `relative` to the session timeline scroll container so it is the **containing block** for absolutely-positioned descendants, plus a structural regression test. ## Symptom While sub-agent tasks are running, the page itself becomes scrollable: you can scroll past the application bounds and the entire layout (header, sidebar, everything) moves. The moment the tasks finish, the symptom disappears. Long task activity makes it dramatically worse. ## Root cause `TaskActivityItem` renders a screen-reader live status while a task is running: ```tsx <span role="status" className="sr-only">Task in progress</span> ``` Tailwind's `sr-only` is `position: absolute`. **No ancestor in the session layout chain is positioned**, so the span's containing block is the initial containing block — the document. That has two consequences: 1. It escapes every `overflow: clip` / `overflow: hidden` barrier between the timeline and `<body>` (overflow clipping does not apply to absolutely-positioned descendants whose containing block is outside the clipper). 2. It is laid out at its static position measured in the timeline's **content** flow, and does not move with the timeline's `scrollTop` (the scroller is not its containing block). With a long session, the running task's row sits thousands of pixels down the timeline content — so the span lands thousands of pixels down the **document**, giving `<html>` real scrollable overflow. Wheel input over the timeline chains into the document once the timeline hits bottom, scrolling the whole layout. When the task completes the span unmounts and the overflow collapses, which is why the bug is transient and only reproduces while tasks are thinking. This is why the previous containment passes (#1340, #1362, #1396) never caught it: they hardened the overflow/height chain, which absolute-to-ICB content bypasses entirely, and the jsdom tests cannot observe real layout. ## Evidence Measured live against the deployed app (CDP attached to Chrome) during a parallel sub-task run: - `html.scrollHeight` grew to **7004px** against a 1464px viewport while `scrollTop` was still 0 — real document overflow, growing +32px per streamed row (each new row pushes the running task's static position down). - 7004 ≈ the running task's y-position in the 7049px timeline content — independent of the timeline's scroll offset. - The only absolutely-positioned elements in timeline content are this span (running tasks only) and the screenshot-card play overlay (already contained by a `relative` parent). Reproduced locally in headless Chrome with a 150-row history before a running task: | | page scrollHeight / clientHeight | page scrollTop after wheeling past timeline bottom | |---|---|---| | before fix | 4895 / 813 | **4082** (layout scrolls away) | | after fix | 813 / 813 | 0 | After the fix, a 10,000px expanded task activity stays fully inside the timeline scroller across every configuration (base, terminal open, changes open, narrow viewport, streaming) — no nested scrollbar or activity height cap needed; expand/collapse behaves like a normal disclosure. ## Changes - `session-timeline.tsx`: `relative` on the timeline scroll container, with a comment explaining the invariant. - `session-timeline-scroll.test.tsx`: regression test asserting the scroller is the containing block for the running-task status span. ## Testing - `npm test -w @open-inspect/web -- src/components/session-timeline-scroll.test.tsx src/components/session-timeline.test.tsx src/components/task-activity-item.test.tsx src/components/session-desktop-layout.test.tsx` (37 passed) - `npm run typecheck -w @open-inspect/web` - ESLint on changed files - Headless-Chrome layout verification described above (real browser, real components, fabricated events) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed live-status elements in the session timeline so they remain properly contained within the scroll area during sub-task streaming. * **Tests** * Added regression coverage to verify correct timeline scrolling behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
min-h-0through the session flex hierarchy and useoverflow-clipat the session rootWhy PR #1340 was incomplete
PR #1340 correctly removed streaming-triggered
scrollIntoViewcalls and scoped auto-follow writes totimeline.scrollTop. That prevented direct page-level scrolling, but it did not eliminate the other scroll containers surrounding the timeline.react-resizable-panelsv4 wraps eachPanelchild in an implicit element whose defaultoverflow: autocan make it independently scrollable. When nested sub-task activity grows beneath an earlier Task row, browser scroll anchoring can adjust those panel wrappers even though the timeline code only writes to the intended timeline element. This is most visible while a sub-task is thinking because activity is inserted retroactively rather than only appended at the end.The timeline also performed bottom-following in a passive effect keyed only by
events. TogglingisProcessingadds or removes the Thinking indicator without necessarily changing the event array, so its height change was not corrected before paint. The follow effect now usesuseLayoutEffectand depends on botheventsandisProcessing.History prepend compensation remains a separate layout effect that runs first. This preserves the reader's viewport and marks that render so the following bottom-follow effect does not race the prepend adjustment when processing state changes at the same time.
Testing
npm test -w @open-inspect/web -- src/components/session-timeline.test.tsx src/components/session-desktop-layout.test.tsx(29 passed)npm run typecheck -w @open-inspect/webnpm run lint -w @open-inspect/webgit diff --checkThe unrelated pre-existing
package-lock.jsonmodification is not included.Created with Open-Inspect
Summary by CodeRabbit
Bug Fixes
Tests