fix(app): align jump-to-bottom button to W1 preview (#601)#628
Merged
Conversation
Match the W1-locked spec from docs/design/preview/message-flow.html (L263-271, spec row L1066) for the floating jump button: - size 32 → 30 (locked default control height) - hover swap from border tint to 4% black overlay (light) / 4% white overlay (dark), per the preview's hover gradient stack layered over --surface-raised Position, background, border, shadow, and cursor were already aligned. The 2 px size delta was a long-standing dev deviation flagged by the W1 audit comment on #601. E2E regression spec (session-w1-jump.spec.ts) locks the geometry (getComputedStyle width/height === 30px), cursor: pointer, hover gradient presence, and click-back-to-bottom behaviour.
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/message-timeline.tsx)).
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
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Playwright e2e spec that seeds a session, provides timeline DOM helpers, and validates the “Jump to latest” button is visible, sized 30×30 px, has ChangesJump to Latest Button Styling and Validation
Sequence DiagramsequenceDiagram
participant Test as Playwright Test
participant SDK as Session SDK
participant Page as Browser Page
participant Timeline as Timeline Element
Test->>SDK: create session via withSession()
Test->>SDK: seedSessionTurns(sessionID) -> promptAsync xN
Test->>Page: navigate to session URL
Test->>Page: poll timeline height until threshold reached
Test->>Page: scrollTimelineToTop() (set scrollTop=0 + dispatch scroll)
Test->>Page: query jump-to-latest button (getComputedStyle checks)
Test->>Page: hover button and poll computed backgroundImage (light)
Test->>Page: set document.documentElement.dataset.colorScheme = "dark"
Test->>Page: re-hover and poll computed backgroundImage (dark)
Test->>Page: click jump-to-latest button
Test->>Page: poll timelineDistanceFromBottom() until ≤ 8px
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/e2e/session/session-w1-jump.spec.ts (1)
55-122: ⚡ Quick winUse
withSession(...)for temporary session lifecycle in this spec.This test manually creates and cleans up a session; please switch this flow to the fixture-managed
withSession(sdk, title, callback)pattern for consistency and teardown safety across E2E specs.As per coding guidelines: "Use fixture-managed cleanup with
withSession(sdk, title, callback)for temporary sessions instead of callingsdk.session.delete(...)directly".🤖 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/session/session-w1-jump.spec.ts` around lines 55 - 122, Replace the manual session create/try/finally lifecycle with the fixture helper withSession(project.sdk, "w1 jump spec", async (session) => { ... });: move the body that currently runs after session creation (seedSessionTurns, project.gotoSession, all expects, scrollTimelineToTop/jump click checks) into the withSession callback and use session.id there (seedSessionTurns({ sdk: project.sdk, sessionID: session.id, ... })), and remove the explicit cleanupSession call and the outer try/finally; keep references to page, jumpButton locator, JUMP_BUTTON_SIZE_PX, scrollTimelineToTop, timelineDistanceFromBottom and other helpers unchanged.
🤖 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/session/session-w1-jump.spec.ts`:
- Around line 55-122: Replace the manual session create/try/finally lifecycle
with the fixture helper withSession(project.sdk, "w1 jump spec", async (session)
=> { ... });: move the body that currently runs after session creation
(seedSessionTurns, project.gotoSession, all expects, scrollTimelineToTop/jump
click checks) into the withSession callback and use session.id there
(seedSessionTurns({ sdk: project.sdk, sessionID: session.id, ... })), and remove
the explicit cleanupSession call and the outer try/finally; keep references to
page, jumpButton locator, JUMP_BUTTON_SIZE_PX, scrollTimelineToTop,
timelineDistanceFromBottom and other helpers unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0defbd58-c7f2-4386-af13-c17aaf3d0521
📒 Files selected for processing (2)
packages/app/e2e/session/session-w1-jump.spec.tspackages/app/src/pages/session/message-timeline.tsx
![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 introduces a new E2E test for the session 'jump-to-bottom' button and updates the button's styling to a fixed 30x30px size with specific hover gradients. Feedback was provided to improve the reliability of the E2E test by using Infinity as a fallback value in the scroll assertion, preventing false positives if the viewport fails to resolve.
Perf delta summaryComparator: pass
|
Address PR #628 review feedback: - coderabbitai: refactor manual session create/try/finally into the fixture-managed withSession(sdk, title, callback) pattern from e2e/actions.ts (L677). Consistency with other specs; teardown is guaranteed even if the callback throws. - gemini-code-assist: replace the `?? -1` fallback in the distance-from-bottom poll with `?? Infinity`. The old fallback could make `toBeLessThanOrEqual(8)` pass immediately if the viewport selector failed to resolve; Infinity correctly keeps the poll failing until a real measurement comes through.
Owner
Author
|
Both review items addressed in 0078340.
Re-ran locally — typecheck clean, |
The previous `dark:hover:bg-[...]` arbitrary-value class relied on Tailwind's default `dark:` variant, which resolves to `@media (prefers-color-scheme: dark)`. The app drives its color scheme through `:root[data-color-scheme="dark"]` (Settings → Appearance), and the project intentionally avoids `prefers-color-scheme` at the app entry (`shell-frame-contract.test.ts` asserts this). Under conflicting OS vs app preference, the hover overlay would invert: dark app on light OS got a black overlay, light app on dark OS got a white overlay. Use the existing `--row-hover-overlay` theme variable via the `hover:bg-row-hover-overlay` utility, which is the standard pattern in the sidebar, model picker, and slash popover. It flips through the same `:root[data-color-scheme]` source as the rest of the surface. Also strengthen the W1 jump E2E spec: scroll to top first, assert the button is genuinely visible (toBeVisible covers the wrapper's opacity-0 / scale-95 hidden state), drop the `force: true` from hover and click so Playwright actionability catches a real regression, and update the hover assertion to read computed `backgroundColor` against `rgba(0, 0, 0, 0.04)` since the new utility paints a background-color rather than a linear-gradient.
Add a dark theme assertion that flips data-color-scheme on the html element (the same attribute Settings → Appearance writes) and verifies the hover overlay resolves to rgba(255, 255, 255, 0.04). This locks the P2 fix: the overlay must follow the app attribute, not the OS prefers-color-scheme media query — Playwright defaults to a light OS preference, so a regression that re-introduces the Tailwind `dark:` variant would show the wrong overlay here and fail the test.
The previous `hover:bg-row-hover-overlay` swapped the background-color
entirely, so on hover bg-surface-raised disappeared and the button
looked transparent — the 4% overlay was painting straight onto whatever
sits behind the button. Preview L269-270 calls for a layered
background: a linear-gradient(overlay, overlay) image painted on top of
the surface-raised color, not in place of it.
Use an arbitrary-property utility to set background-image directly so
bg-surface-raised stays as the background-color underneath:
hover:[background-image:linear-gradient(
var(--row-hover-overlay), var(--row-hover-overlay))]
--row-hover-overlay flips through :root[data-color-scheme], keeping the
P2 fix: the overlay follows app theme, not OS prefers-color-scheme.
Switch the transition from transition-colors to
transition-[background-image] to match the property that actually
changes on hover.
Also drop cursor-pointer per product call on 2026-05-15 (preview L267
still locks pointer; flagged as `偏离` in the component and a follow-up
preview/DESIGN sync is owed).
E2E updated to match: drop the cursor assertion, assert
backgroundImage instead of backgroundColor for both light and dark
hover states. The dark assertion still flips data-color-scheme on the
html element to lock the P2 contract that the overlay tracks the app
attribute, not OS preference.
The wrapper used `transition-[opacity,transform]` with translate-y-2 and scale-95 in the hidden state, producing a two-stage exit where the button visibly drifts and shrinks before fading. W1 preview L263-268 locks only a `transition: background` on the button itself and does not call for an enter/exit animation, so the translate/scale layer was an undocumented embellishment that hurt fade perception. Drop translate-y/scale and switch to `transition-opacity` so the button enters and leaves with a single, calm opacity transition. Keep duration-200 ease-out and the pointer-events-none guard on the hidden state. Verified manually in Electron: enter/exit reads as a clean fade with no displacement or scaling.
The hidden state no longer applies scale-95 / translate-y (PR #628 collapsed the fade to opacity-only), so the comment justifying CSS reads via "parent's scale transform during transition does not skew the measurement" is misleading. Reduce the geometry comment to the preview citation it actually relies on.
The wrapper no longer applies scale-95 / translate-y in the hidden state — those were dropped when the fade was simplified to transition-opacity. Update the toBeVisible justification so it references only the classes that actually exist (opacity-0 and pointer-events-none).
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
Align the floating jump-to-bottom button at
packages/app/src/pages/session/message-timeline.tsx:940-957with the W1-locked spec fromdocs/design/preview/message-flow.htmlL263-271 / L1066, plus follow-up polish driven by manual review:size-8(32 px) →w-[30px] h-[30px](the locked default control height of 30).hover:[background-image:linear-gradient(var(--row-hover-overlay),var(--row-hover-overlay))], layering a 4 % overlay on top ofbg-surface-raisedrather than replacing it.--row-hover-overlayflips through:root[data-color-scheme], so dark/light parity tracks the app theme attribute, not OSprefers-color-scheme.cursor-pointer, button now keeps the default arrow.偏离from preview L267 (which lockscursor: pointer) — product call on 2026-05-15; preview / DESIGN sync is a follow-up.transition-[opacity,transform]+translate-y-2 scale-95totransition-opacityonly. Enter / exit is now a single calm opacity transition with no position drift or scale jump.Position, background, border, shadow, and overlap behaviour with staging are unchanged.
Why
First slice from the #601 W1 inventory comment (slice 1 / E1). The 2 px size delta and the wrong hover treatment were flagged by the crosscheck against the W1-locked preview. Two additional fixes came from in-PR review (manual Electron testing + multi-model crosscheck): the hover overlay had to layer over the raised surface rather than replace it, and the fade transition's drift / scale was hurting perceived smoothness. Both are inside the jump button's visual contract so they live in this PR rather than a follow-up.
Related Issue
#601 (slice 1 / E1 of the W1 inventory).
Human Review Status
Verified by the repo owner in Electron on 2026-05-15: geometry, light/dark hover overlay, cursor, and fade all confirmed visually. Final merge decision still pending — perf-probe-baseline must come back green.
Review Focus
hover:[background-image:linear-gradient(var(--row-hover-overlay),var(--row-hover-overlay))]keepsbg-surface-raisedas the background-color underneath. An earlier iteration usedhover:bg-row-hover-overlay, which swapped the background-color and made the button look transparent on hover — confirmed and rejected during manual testing.--row-hover-overlayis the same token used by sidebar / model-picker / slash-popover and flips via:root[data-color-scheme]. The project deliberately avoids@media (prefers-color-scheme: dark)at the app entry (shell-frame-contract.test.ts:49asserts this), and Tailwind v4's defaultdark:variant would have re-introduced exactly that media query. The new E2E flipsdocument.documentElement.dataset.colorScheme = "dark"to lock the contract that the overlay follows the app attribute, not the OS preference.偏离comment inmessage-timeline.tsx:948flags the intentional drift from preview L267. A follow-up will sync preview / DESIGN to match.translate-y/scalecollapses the enter/exit to opacity only. No accessibility tests changed because focus / keyboard paths were not exercised by the original spec either.Risk Notes
cursor: pointer; current code is the default arrow. The偏离comment in the production code is the only marker until the preview / DESIGN sync follow-up lands.transition-[background-image]does not animatelinear-gradientto/fromnone— the hover overlay snaps in instantly rather than fading. This matches the preview (which only transitionsbackground) and was visually confirmed; calling it out so the next reviewer doesn't expect a hover fade.How To Verify
Targeted local checks run in the worktree:
The spec
packages/app/e2e/session/session-w1-jump.spec.tslocks:getComputedStyle.width === '30px'andheight === '30px'toBeVisible()afterscrollTimelineToTop(covers the wrapper'sopacity-0hidden state)backgroundImage === "linear-gradient(rgba(0, 0, 0, 0.04), rgba(0, 0, 0, 0.04))"document.documentElement.dataset.colorScheme = "dark", hover, assertbackgroundImage === "linear-gradient(rgba(255, 255, 255, 0.04), rgba(255, 255, 255, 0.04))"distance ≤ 8)CI runs the full PR0 / e2e-artifacts / smoke-macos-arm64 / typecheck / unit-* matrix. As of
7905f7664everything else is green;perf-probe-baselineis the remaining gate.Manual desktop eyeball — completed 2026-05-15
The repo owner ran
bun run dev:desktop, opened a long session, and confirmed:--surface-raised(not transparent)Screenshots or Recordings
Not attached. The manual verification above was done by the human reviewer; on green E2E runs no artifact is emitted.
Checklist
bug,app,ui,P2)偏离from preview L267 noted in code + Risk Notes; preview / DESIGN sync is a follow-up)dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Style
Tests