fix(messages): prevent image-driven timeline scroll snap-back - #31
Conversation
Signed-off-by: Fizz <400e8babadcee6a7f420103f10a2849d84c4a9c71d5bd04f3948c814216648a3@buzz.block.builderlab.xyz>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30017fe9b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ### Attachment layout and scrolling | ||
|
|
||
| Image attachments reserve their preview geometry before loading and across virtualized |
There was a problem hiding this comment.
The reviewed commit message has no Signed-off-by trailer, so this commit violates the repository's per-commit DCO requirement and will not satisfy the hosted DCO gate. Recreate the commit with git commit --signoff using the verified effective author identity before integration.
AGENTS.md reference: AGENTS.md:L51-L54
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head 30017fe9b935cdc272c62cca58062cc73bdab8aa against base/merge-base 54383673556b05de89b3efb9ac32fc6ae197a956.
Changes requested: one P2 regression-test defect. I found no material production defect in the bounded attachment-geometry change. The new test rejects a valid above-bottom setup on hosted WebKit before exercising its reading-anchor assertion; see the inline finding.
Validation: traced folding, channel/thread consumers, geometry-cache invalidation and scroll intent. The unchanged image-scroll test passed twice per engine locally (4/4, macOS, pinned Playwright 1.60.0). A diagnostic copy using a smaller wheel gesture/above-bottom setup threshold also passed the unchanged anchor, reload, bottom and responsive assertions in both engines (2/2). This diagnostic is not a proposed threshold fix or Linux validation. Inspected the failing Linux WebKit trace from run 34655047089; its synthetic merge commit has the same tree as the reviewed head.
Separate notes: the existing panel-resize journey also failed its wheel setup (384px versus >400px); I have not attributed that failure to this PR. DCO is passing and the commit contains Signed-off-by, so the existing DCO complaint is not supported. Explicit failed-image copy would be a follow-up improvement, not a new blocker: the old renderer lacked it too, and retaining the failed frame is intentional. Native/live-account acceptance was not performed.
Exit criterion for this finding: establish above-bottom reading and new image requests using bounded, observed scrolling rather than assuming one wheel event realizes a particular distance; preserve the anchor/remount assertions and demonstrate the case on hosted WebKit. No scroll-controller changes are requested.
| pending.clear(); | ||
| await feed.hover(); | ||
| await page.mouse.wheel(0, -6000); | ||
| await expect.poll(gap).toBeGreaterThan(5000); |
There was a problem hiding this comment.
[P2] Establish the reading setup without assuming one wheel-event distance
The required Linux WebKit job fails here at this head: wheel(0, -6000) leaves a stable 4740px bottom gap, but this poll requires >5000 and issues no further input. The hosted trace also shows newly held image requests (76/78/80), so the reader has moved well above bottom and the intended delayed-load setup is available. Nevertheless, the test times out before capturing the reading anchor or releasing those images. The same assumption is repeated at lines 95–97.
This is a failing test setup, not evidence of production snap-back. Use bounded real wheel gestures until the needed observed setup holds (above-bottom position and newly pending image requests), then capture the anchor and retain the existing preservation/remount checks. Avoid relying on a single gesture crossing an arbitrary 5000px cutoff. See the failed required job.
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head f8a45b115cd82049ef04cb4f8e771338090f3532 against base 17f90c18fff6b86bc029e710401fb2b60bc385ea.
Changes requested: one P2 regression-test defect remains. The new image journey still assumes a single wheel gesture reaches its intended position. Current hosted WebKit fails the remount setup, rather than the above-bottom setup reported on the older head. See the inline finding.
The production direction is sound: event-derived, bounded image frames in the shared web/Tauri message renderer, preserving HTTPS-only media handling and existing near-bottom/above-bottom scroll intent. Traced signed-event folding, legacy/invalid metadata fallback, restored heads and live/local projections, shared channel/thread rows, geometry-cache invalidation, image decode/failure and remounts. No material production defect found in these changed paths. No scroll-controller change or failed-image UI expansion is requested. Independent geometry review reached the same production conclusion. Coverage limits, not additional blockers: the fixture does not explicitly assert failed-image identity/frame stability or per-geometry-class remounts, and it does not exercise thread-image scrolling.
Validation was source/metadata-only on Wes’s Mac Studio; no checkout, PR-code execution, local tests or native/live-account acceptance. Inspected the existing hosted WebKit job and trace. Its checkout 28bd2c7948fcb96ed4f8713720516482e3c6eca8 and this head share tree 30334f6aa8db01a7e13f92e1c10817561eb0b176. That shard reports 65 passed and this one failure; JavaScript, Rust/tool integration, Chromium shards and the other WebKit shard passed. These are hosted results, not tests run by this review.
Exit criterion: use bounded, observed real gestures to establish above-bottom/new-image-request and return-to-bottom setup, checking progress rather than assuming wheel delta equals displacement. Preserve the anchor, repeated-remount/reload, bottom and responsive assertions, and demonstrate the complete journey on hosted WebKit. Do not replace this with arbitrary sleeps, a larger wheel delta, direct scrollTop assignment or weaker preservation assertions.
| await settle(page); | ||
| await expectAnchor(page, reading); | ||
| for (let i = 0; i < 3; i++) { | ||
| await end(page); |
There was a problem hiding this comment.
[P2] Make the remount setup observe real scroll progress
The new journey fails here on the current head in hosted Linux WebKit. The trace shows end(page) sending one wheel(0, 5750) through timeline.mjs:70-92, then the bottom gap settling at 2804px. The helper only polls after that single gesture, so it times out after 10 seconds waiting for <4px. The initial bottom/decode and above-bottom anchor assertions have already passed; the loop never completes and the reload/responsive assertions are not reached. This blocks the required browser check without establishing a production snap-back defect.
Use a bounded sequence of real gestures with observed progress to establish the requested edge before checking image stability. Apply the same setup principle to the -6000 / >5000 assumptions at lines 81-82 and 96-97; those were the earlier head’s failure point. Retain the actual bottom/anchor/remount assertions rather than loosening them. The smallest repair is test navigation, not new production scroll logic.
Bound real wheel gestures in the image-scroll journey and hold image responses until navigation completes. Keep the existing post-decode anchor, bottom, remount and responsive assertions unchanged. Cover partial wheel delivery and blocked input without corrective scrolling during preservation checks. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Test-navigation repair pushed
At Wes’s request, added follow-up commit 00e89a45d26a659a2b9ae070173f074d805e007f to this PR. Only tests/browser/image-scroll.spec.mjs changes; production code and shared navigation helpers are untouched. Prior commits are preserved.
The spec-local navigation helper uses at most eight real wheel gestures, caps each requested delta, checks progress after each gesture, and verifies the destination. Image responses remain held until setup ends; after release, the unchanged image-stability assertions run without corrective input. Added real-browser partial-input and blocked-input controls. Independent review of the exact final file found no material issue.
Validation on Wes’s Apple Silicon Studio
The committed diff exactly matches the validated patch against f8a45b115cd82049ef04cb4f8e771338090f3532 (image spec SHA-256 012728bd1af728ea47c757d793abc8895063295cdc97b80f91f56cbf33de2502); pre-commit made no further changes.
- Focused Chromium/WebKit: 4/4 passed, including the full image journey and partial/blocked controls. They also passed in the full browser run.
- Negative control: temporarily restoring the pre-PR attachment CSS fails the unchanged post-release bottom assertion: 320px Chromium / 322px WebKit, against
<4px. Production CSS was restored before committing. bin/just scan: checks, Node integration, 1,055 Vitest tests / 107 files, and plugin Rust tests passed. Full application browser run: 267 passed, 1 failed. The failure is in untouchedtypeahead.spec.mjs:353, expecting!limitbut receiving an empty composer. Not investigated or changed as part of this bounded repair. Scan stopped there, so its later build/design-browser/Clippy/native-app-test stages did not run. Pre-push passed TypeScript and 479 related unit tests at the clean committed head.
Not merge clearance: the known image test setup has been repaired and validated locally, but the prior review’s hosted Linux WebKit exit criterion remains unverified. The CI-tool preflight is unavailable in this session; no current CI/DCO pass is claimed. I have not dismissed the existing changes-requested review or approved the PR. Hosted checks, the separate typeahead failure if it recurs, and required code-owner review remain gates. No native/live-account acceptance or production scroll rewrite was performed.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head 00e89a45d26a659a2b9ae070173f074d805e007f against base 17f90c18fff6b86bc029e710401fb2b60bc385ea, focusing on the repair since previously reviewed head f8a45b115cd82049ef04cb4f8e771338090f3532.
Source review clear: no actionable code defect found in the repair. This is not merge clearance. The prior single-wheel setup assumption is removed. Hosted Linux WebKit acceptance remains unverified; this review neither claims that gate passed nor dismisses the earlier changes-requested reviews.
- Only
tests/browser/image-scroll.spec.mjschanged since the previous review. Its setup helper uses at most eight bounded real wheel gestures, requires directional progress after each gesture, settles geometry, and asserts the final destination (lines 9–40). Partial-input and blocked-input controls require convergence or an explicit first-gesture failure (lines 176–210). Independent review of direction, convergence and asynchronous cleanup found no material defect; I verified that conclusion against the final source. - Image responses stay held while setup navigates. Release occurs before the unchanged bottom/height, same-message anchor, repeated remount/reload and responsive assertions, with no corrective scrolling between release and those checks (lines 105–165). Production attachment parsing, bounded frame geometry, shared channel/thread rendering and the shared timeline helper are unchanged; no scroll-controller rewrite or broader failure-UI change is requested.
- Validation for this review was source/metadata-only on
BLKD2G9MWNRWY.local, using pinned objects in/Users/wesb/.buzz/REPOS/buzz-app.git diff --checkagainst the base passed. The spec SHA-256 matches the separately reported repair receipt:012728bd1af728ea47c757d793abc8895063295cdc97b80f91f56cbf33de2502. No PR code, tests or native/live-account workflow were executed by this review. Exact-head GitHub metadata showed DCO, Semgrep and zizmor successful; the Actions endpoint returned zero runs for this head. The Studio validation receipt reports focused browser passes and an unrelated typeahead failure, but is historical reported evidence, not fresh hosted validation.
Remaining acceptance: demonstrate the complete unchanged image journey on hosted Linux WebKit and satisfy the repository’s required checks/reviewer gates. Existing coverage limits remain non-blocking: no explicit failed-image identity/frame assertion or per-geometry-class remount assertion, and no thread-image scrolling journey.
Preserve attachment dimensions alongside relay membership activity contracts. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Resolved the merge conflict in b85c014, merging main b5d3f0c without rewriting existing commits. Validation on the clean integrated commit, on Wes’s Mac Studio:
This is conflict repair, not approval or merge. Hosted CI/DCO results remain unverified because the required CI tooling preflight failed. The earlier full-scan typeahead failure was not changed or rechecked; a new complete scan/native acceptance was not performed. Existing review state is unchanged. |
* origin/main: fix(messages): prevent native WebKit prepend blanking (#49) feat: add shared browser and standard desktop notifications (#38) Prevent whole-window scroll bounce (#41) test: replace timing races with explicit lifecycle barriers (#45) test(browser): isolate fixture optimizer caches (#46) test(browser): settle startup catch-up before request baselines (#43) feat(messages): show message-carried blurhash before decoded originals (#40) Add a shared full-page surface (#29) fix(messages): prevent image-driven timeline scroll snap-back (#31) Restore grouped channel membership activity (#30) feat: add broker-backed Agent Activity panel (#36) Signed-off-by: Clay Delk <clay.delk@gmail.com> # Conflicts: # src/bundled/channels/ChannelsPage.tsx # src/features/messages/ChannelTimeline.tsx # src/features/messages/MessageRow.tsx # src/features/relay/contracts.ts # src/features/relay/fold.ts
Summary
imeta dimdimensions in the attachment projection and bound previews to 360×320 without upscaling.Regression coverage
Real
ChannelTimeline,MessageRow, folding and session in a credential-free fixture, Chromium and WebKit:Negative control: restoring the original attachment CSS makes the browser regression fail after image load (320px bottom gap).
Validation
Full local
bin/just scanpassed on the exact change set committed here (precommit applied no changes; compared staged validation patch with commit diff):Hosted CI and required code-owner review remain separate gates. This does not claim verification against the reporter's exact native app/cache state; their running checkout was not modified.
Originating Buzz channel:
dec3c452-cf15-4472-bb71-8de17a571993(buzz-onedotzero), threadc6cfd2df4dc80a739893dd9fad60894f06e8910d10e7e6e4bb354e5576885f50.