Repository navigation
Conversation
… history Eight focused areas (A01-A08) from the 10a plan. Reproduced defects on main 739fd68 stay executable as targeted test.fail() expected failures: freenet#723, freenet#508, freenet#507 (image and bulk removal), hidden-panel drain, render ceiling eviction, and trim/backfill oscillation on a tall viewport. Adds an example-data removeMessages hook, query-gated uneven-height fixture rooms, shared geometry/settle-gate helpers, and serves the static release build in CI instead of dx serve. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The oscillation starts as soon as the room opens, so paging into the tall rows first raced it (1 setup failure in 25 on Firefox). Observe the opened room with no reader input instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The scroll specs pointed at a plan doc that is not in the repo, and the delayed-image test could hang if the route was not installed before the request. Co-authored-by: Cursor <cursoragent@cursor.com>
Narrow module-only exports in history-geometry and drop the unread ReadingRow.nextKey. Fulfil the captured image route directly. Share one bounded row-count observer between the two A05 tests, installed before the return to the end. Let openOwnMessageEdit take an explicit message so the last-message edit test reuses it without a preparatory scroll. Full Playwright suite: 638 passed, 28 skipped (--retries=0). Expected failures still reach their final assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ui-playwright-tests job restores all of target/, so a build that wrote nothing left the restored index on disk and the disk-vs-HTTP bundle check passed against it. build-ui-example-no-sync now deletes the selected public directory and touches ui/src/main.rs before dx runs, locally and in CI, and the readiness step rejects a missing or empty index before starting the server. scripts/tests/playwright-bundle-provenance-test.sh extracts the real cleanup and readiness bodies, runs them against fixtures with stubbed python3/curl, and pins the task and CI wiring. CI runs it first in ui-playwright-tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- example_data: one fixture_requested(flag), merged Deep/UnevenTail retention arm, deep-history rooms built in a (name, depth) loop. - Specs: backfillOnce, expectParkedAwayFromEnd, CSS.escape row lookups, exported READING_ROW_BUDGET_PX, named fixture assumptions, corrected A01-A08 index and stale fixture comments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move the example-output cleanup and the Playwright readiness step into scripts/clean-ui-example-output.sh and scripts/serve-playwright-ui.sh, called from Makefile.toml and build.yml. The provenance suite now runs those scripts directly instead of extracting shell bodies with AWK, and keeps wiring checks for the build, CI order and server/base-URL port. The readiness poll tolerates empty captures under pipefail, so a poll made before the server listens retries instead of aborting the step; covered by a new case. Drops the redundant warm no-op case and folds the missing/empty index cases into one loop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three reader-position fixes from plan 10b, on the freenet#746 regression coverage: - Hidden panel: remember the reading row by message identity while the history has layout, stand geometry paths down while it is hidden, and restore a parked reader to that row on reveal. - Render ceiling: the rendered range is now [start, end). At the ceiling a parked reader's end is held (newer items withheld) instead of the start sliding past their row; newer history pages in from a bottom strip, and jump-to-latest / own send select the latest range first. - Trim loop: trim eligibility uses the measured retained tail, not an average-height estimate, and the deferred trim re-checks room, layout, position and eligibility before acting. Promotes the three matching known failures to ordinary tests and adds coverage for the Rooms route, repeated hidden bursts, breakpoint reveal, hidden bursts at the ceiling, bounded repeated bursts, newer paging and a stale deferred trim. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… test - The settle callback and the newer-history sentinel call `remember_reading_position` instead of copying its body. - The standalone render-ceiling test's premises (surviving row, prune, first arrival, catch-up button) move into `parkPastTheCeiling`, and the repeated-burst test keeps its first-burst row assertion, so the duplicate scenario is removed. - Comments now describe the measured retained tail as the current trim rule, and say that `ReaderPosition` cells never subscribe. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Shorten the comments added for the hidden-panel, held-range and measured trim changes: plain statements of what each piece does, no em dashes, and no restating of the history the tests already pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Arrivals and deletions no longer move the view (10c decisions 1, 2). The follow pin becomes one-shot, instant requests for opening, own send and Latest (3, 6); Latest shows once the newest bottom is off screen (4); height changes, a deleted reading row and post-request reflow keep the place (10, 7, 13). Tests: scroll specs rewritten for no-follow, new A02-A06 and end-hold cases. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every arrival and every tab hide marked the open room read, even with the newest message below the fold or behind a mobile panel. Conversation now publishes NEWEST_SEEN once that message's bottom is on screen with the tab visible (10c decision 5), and all seven mark-read sites stop there. Tests: A07 read cases, hidden-tab title, mobile panels, read_marker_update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An open DM thread no longer follows inbound DMs near the bottom (10c decision 1), gains an instant Latest control and an instant own-send jump (12), and advances DM_LAST_SEEN only up to an inbound DM that was on screen with the tab visible (11). The modal body is keyed by (room, peer). Tests: new dm-thread-scroll spec on populated threads via appendDms/deliverDm. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main carries freenet#746 and freenet#747 as squashes of this branch's 10a and 10b, plus the freenet#747 review rounds made after the local 10b copy. Each conflict starts from 10c; the review changes were ported, converted or dropped as follows. - ea39f78 (held end released when following resumes): fix moot, no following and a request selects the latest range. Kept the deep-history-retention fixture flag. Dropped its smooth-return test: Latest is instant, and the idle-reader test already covers a burst past the ceiling after an explicit request (view stays, DOM bounded, Latest reaches the newest). - d6b73cf (following resumes after the final newer page): fix moot. Dropped both final-page tests and the paging test's follow check; an arrival after a newer page is covered by the ported newer-page test below. - 5be0c33 (head slide compensated on a newer page): ported. The head reposition subscribes to window_items, plus "a newer page keeps the reader's row". The test failed on all five projects without the subscription (row moved ~6160px). - 33ca2dc: ported the shared key_at closure; the parked gate is gone in 10c. - c345eb5: ported the resolve_held doc links and the window doc (now on resolve_held, describing the end holding at the ceiling and keep), the keep, anchor, probe, trim and remember_reading_position doc fixes, and the shorter Latest comment; dropped the no-room hamburger capture. Kept 10c's bool restore_reading_anchor (the bottom-edge fallback uses it) and its settle comment; the dead_code allow on hidden was already there. - b02f904, 3f75aba: withheld moved into history-geometry.ts; dropped the room-list duplicate of the hidden drain and folded the two-burst test into it; parkAboveEnd; dropped implied premises (withheld after Latest, the paging row-held and DOM-bound checks, the hidden-ceiling chevron check). - 36928b7, babe31b: trimmed the trim-geometry test to its boundary pair, folded the held-end fallback into the paging test, and kept one both-neighbours case asserting 10c's previous-first order. - contract-version.txt: main's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`readerScrollsWithoutGesture` and `readerReturnsToEnd` now share one `scrollAndSettle(page, offset | "end")` that registers the `scrollend` listener before the move, returns at once when the offset did not change, and removes the listener and timer on every path. `holdTestImage` moves into `history-geometry.ts` and also drives the parked-image regression. The DM thread spec's three openings share `prepareThread(page, count)`. `ReadingRow`, `RowEdge` and `rowGapFromViewBottom` are no longer exported. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…guard
Replace the head reposition's `(String, i32, i32)` snapshot with a private
`RepositionAnchor { probe_key, probe_top, scroll_top }`, matching
`BackfillAnchor`, so the row offset and the scroll offset are no longer
told apart by tuple order. Capture, `take()` consumption, the pre-patch
scroll offset, the request/hidden-layout guards and
`drop_pending_corrections()` are unchanged.
State `read_marker_update`'s monotonicity as an explicit guard: look up the
existing marker's index, return `None` when it is at or past the seen
message, else mark the seen message. Same semantics as the nested
`is_none_or` form.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Delete tests that pinned the removed follow-to-bottom behaviour or that other tests already cover, and merge pairs that ran the same scenario: - autoscroll: drop the gesture-less scroll, parked windowed arrivals, parked resize and send-from-the-end tests; fold the first-burst row hold into parkPastTheCeiling and the arrival-after-send leg into the send-from-the-top test; cut the draft-open arrival loop to 2. - dm-thread-scroll: drop the opening and short-thread-overflow tests; fold the at-end inbound check into the read-rule test and the Latest read check into the Latest jump test; tag the read-rule describe @chromium-only. - room-unread-badge: drop the old-100px-band and short-arrival tests; keep only the members-panel A07 test and tag it @chromium-only. - Rust: merge the same-messages end-hold test into the shrinking-range one, merge the two marks-nothing read-marker tests, and drop the hide-tab mark test that read_marker_update's tests cover. Also drop the vestigial WheelEvent in readerScrollsTo: the app has no wheel, pointer or touch listener. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- example-room.ts: nextFrames and settle (re-exported from history-geometry, which imports example-room), hiddenTitleCount and roomUnreadBadge, replacing per-spec copies in the DM, room-unread and autoscroll specs. - history-geometry.ts: hideChatBehindMembers, now also used by the A07 members-panel test, and one NEWEST_IN_VIEW_SLACK_PX for the room and DM specs. expectRowHeld and expectHeldFromViewBottom use settle. - Rename BOTTOM_THRESHOLD_PX to WELL_AWAY_FROM_END_PX, its only meaning now, and drop the history of the removed bottom band. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep each scroll-anchoring rationale once, on its doc comment (`reposition_pending`, `grown_window`, `opening_snap_done`, `trim_is_due`, `BACKFILL_LEAD_PX`), and point to it from the other sites. Drop comments that narrate the old behaviour. Use `Cell::replace` for the DM auto-scroll triggers, add `with_current_room_mut` to the test hooks, compute the DM stamp step once, and narrow `thread_read_needs_write`, `mark_thread_read` and `mark_current_room_as_read_on_hide` to private. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…om and DM The room history and the DM thread each carried their own slack constant, sentinel-in-view check, instant scroll-to-end and Latest button markup. Move them into `components/scroll_to_latest.rs` (`NEWEST_IN_VIEW_SLACK_PX`, `sentinel_in_view`, `scroll_to_end`, `LatestButton`) and add a `dm_scroll_container()` helper for the DM's repeated lookup. Each caller keeps its own handler; the room's still defers its signal writes. Classes, test ids and aria labels are unchanged. `sentinel_in_view` keeps the DM's zero-height guard; the room only calls it after `history_has_layout`, so its behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mark_current_room_as_read now only advances the open room's marker to NEWEST_SEEN, and DocumentTitleUpdater's effect (always mounted in App) already runs it on every change to CURRENT_ROOM (.read()), ROOMS and NEWEST_SEEN (try_read, nudged on Err) and DOCUMENT_VISIBLE (.read() in update_document_title), behind signal_guard::anchor(). Each removed caller sat right after a write to one of those signals, so the effect re-ran and did the same thing: - room-list click: right after CURRENT_ROOM.write(). - room_synchronizer apply_delta_inner / update_room_state_inner: after ROOMS.with_mut; their visible-and-current gate was narrower than the read rule, which only marks what NEWEST_SEEN says was on screen. - response_handler post-load defer: after the ROOMS merge, also calling update_document_title, which the effect calls too. - tab becoming visible: right after DOCUMENT_VISIBLE.write(). The function is now private to document_title.rs, its caller list in the doc is trimmed, and the now-unused imports go. No source-scrape pin referenced these call sites; signal_guard's lists are unaffected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
window_anchor, window_tail, window_rendered and window_overgrown were four separate Rc hooks, cloned into every effect and callback that needed them. They now live in ReaderPosition with the rest of the render's inter-render memory, so install_scroll_settle_listener takes (window_items, reader). ReaderPosition::forget_range replaces the three copies of "drop the last range and end the hold". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d_held Both scrolled to the end, recorded the hold, remembered the reading position and checked the read rule, in the same four lines. hold_end does it once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The range calculation and its write-back to ReaderPosition move out of the
render body into resolve_rendered_range, a plain function of the display
items, the requested size, the open room and the reader's cells. The three
`if select_latest { None } else { .. }` reads become one tuple. The DOM probe
for the head reposition stays in the render, gated on the returned
head_removed exactly as before.
The history-window pin now also checks the render calls the helper, and its
write-back needles name `reader` instead of `reader_position`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ResizeObserver callback sat four closures deep inside Conversation(). It is now on_history_resize, with observe_history_resize installing it the way install_scroll_settle_listener installs the settle listener. Both installs share one effect, each with its own success flag, so each still retries on every message_groups change until it finds the container. The settle-listener pin now matches `listening.set(install_scroll_settle_listener(`, the call's new shape. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…thread `observe_sentinel` in scroll_to_latest.rs builds the IntersectionObserver both Latest controls use (same root margin and threshold) and returns a `SentinelObserver`, the DM thread's `NewestDmObserver` renamed, which disconnects on drop. The DM thread keeps holding it per mount; the room leaks it as before, since its component is never unmounted. Both callers now read the last queued entry, which the DM thread already did. With one observed target, entries only queue up when the sentinel crossed the edge more than once before the callback ran, and the last one is its current state; the room used to apply the first, stale one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The backfill restore, the head reposition and request completion were three effects; they are now one, in that order. `BackfillAnchor` and `RepositionAnchor` become one `ProbeCapture`, whose `shift` measures the probe row (the backfill keeps its `scrollHeight` fallback as `scroll_height: Some(..)`). Each correction keeps its own rule: the restore applies only `shift > 0`, the reposition needs layout, applies any non-zero shift and clamps at 0, and both stand down while a request is pending. Subscriptions: the restore now also wakes on `message_groups`. That is harmless: its capture is taken in the same handler that grows `window_items`, and effects only run once no scope is dirty, so the first pass after a capture is always the one after the grown render, as before. The order between the three was up to Dioxus's task wake order; it is now fixed with both corrections ahead of the completion that clears the request they defer to. Pins: the H3 pin now reads `correct_view_after_patch` and checks both captures are taken and the request guard returns before either applies (it counted two copies of the guard). The head-swap pin's measurement, pre-patch offset and probe-restore needles follow the new code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`the_window_trims_at_the_bottom_and_backfill_waits_for_the_opening` keeps the H2 and H3 races and the deferred trim's room re-check, which no browser test switches rooms for. The settle-handler trim, the slack gate, the deferred re-measure and the geometry gate go: the conversation-autoscroll "trims once and stays bounded", "stands down when the reader moves" and "very tall viewport" tests cover them on every project. `head_swaps_reposition_the_reader` keeps the pre-patch scrollTop (freenet#505) grep and the backfill probe measurement, which no browser test exercises (it needs an arrival batched into a backfill's patch). Row identity, the capture, the measured shift and the probe walk go: the batched at-cap drain, newer-page and backfill paging tests cover them on every project. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
holdNextDmPlacement holds the next DM thread placement (the opening or own-send jump) inside its task, before it touches the DOM, until releaseHeldDmPlacement runs it; heldDmPlacementCount and releasedDmPlacementsRun let a spec wait on both ends. Release throws when nothing is held. The thread routes the placement through the hook only in the wasm32 example-data + no-sync build; elsewhere it runs directly. appendDmsForPeer and deliverDmForPeer add a second fixed DM identity (peer 1, "Other DM Peer"); appendDms and deliverDm are unchanged for peer 0. The thread modal and its close button get data-testids. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A thread's queued opening or own-send placement finds the thread by the global dm-scroll-container / dm-bottom-sentinel ids, so if it runs after its thread unmounts it acts on whichever thread is open by then. The spec holds a real placement across the unmount and releases it over a replacement thread, over a reopened instance of the same thread, and with nothing open; a held placement whose thread stays open must still land, so a guard that disables every placement cannot pass. Against the current modal the three replacement cases fail on every project: the replacement thread jumps to its end (about 750-840px), the closed thread is marked read from the replacement's end, and the replacement's DM below the fold is marked read too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The opening and own-send placements run in a task queued after the render, and they and the read witness reach the thread through global DOM ids. A placement queued by a thread that has since closed scrolled whichever thread was open by then, and counted that thread's visible end as evidence that the closed one was read. Each mounted thread body now owns a lifetime cell, cleared synchronously by use_drop. The placement checks it as it runs, before any DOM lookup, and ThreadSeenWitness::check before measuring. A reopened thread gets a fresh cell, so (room, peer) equality cannot revive an old instance. Both hooks sit above the view's early return. Read cutoffs, the deferred mark_thread_read and the signal-safety gates are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All issues from #753 (review) have been solved |
sanity
left a comment
There was a problem hiding this comment.
PR review: #753 (head 6d34ed6f)
@glesage, thanks. This is a careful piece of work, and two of the three blockers from #732 are now fixed. It can't merge yet, though, for two reasons: removing automatic following needs a maintainer decision first, and there are code and test findings to fix regardless of how that goes.
Verdict: changes requested.
How this was reviewed. Full tier.
- Code: read by four reviewers: Claude (skeptical/state-machine, testing, big-picture + code-first) and OpenAI Codex. The diff was read for malicious changes first. Nothing was found, and the test hooks are compiled only into
example-data+no-syncbuilds. - Visual check: the reviewed head was then built in a credential-free sandbox and driven in a no-network browser on iPhone 13 (WebKit), Pixel 7 and desktop (Chromium), light and dark.
- CI is green on
6d34ed6f, and the PR merges cleanly into currentmain.
Blockers from #732
| # | #732 blocker | At 6d34ed6f |
|---|---|---|
| 1 | Hiding the chat column on mobile changed the reader's position state | Fixed. A hide only sets hidden, zeroes observed_height and clears the end hold. Captures and settles refuse to run without layout, and the reveal restores the last visible capture. Confirmed in the sandbox: on iPhone and Pixel, a reader scrolled 400px up who opens the room list while a message arrives comes back on the same content (distance from the end grows only by the new message). |
| 2 | Scroll-to-latest animation could clear the pin or be aborted | Fixed / moot. Every ScrollBehavior::Smooth is gone, Latest is instant, and there is no pin left to clear. |
| 3 | mobile-touch-ux.spec.ts:277 failure dismissed after isolation reruns |
Still open. See Must 2. |
Must fix
- Removing automatic following is a product decision, and the maintainer hasn't made it yet.
mainrecords following as current policy:conversation-autoscroll.spec.ts:844says "CURRENT POLICY: main intends to follow here. Dropping arrival-following would … change this test", and #486 treats "stops following" as the bug.- Nothing in this thread or the earlier ones records that decision being taken. Please don't treat it as settled until @sanity decides.
- What it means for users, confirmed in the sandbox build: with the reader at the end, three arrivals land entirely below the fold (about 350px on desktop, about 370px on phones). The only signal is the 40px ⌄ button in the corner, with no count or label.
- Nothing else announces the arrival either.
notifications.rs:1200skips the open, visible room, and the room-list and hamburger badges exclude the current room. A window left open on a second screen stays frozen at the last message. - For comparison, mainstream chat clients follow a reader who is at the end, and show a counted "new messages" pill only when the reader has scrolled up.
- If following stays removed: the arrival needs a real signal, at least a count or a "N new" label on Latest.
- If it comes back: the stateless pieces this PR adds (
request_end, the live sentinel measure, the pre-patch DOM read) look like enough to follow at the end without the old latched pin. For example: "the sentinel was in view before the patch and the patch is an arrival →request_end(room)". That would fix #723 and #508 without removing the behaviour. - Either way: please retitle (this is a behaviour change, not a
refactor, and the title becomes the squash commit and release-notes line), and give each of #486, #487, #507, #508, #723, #748 and #751 a stated disposition.
- The mobile-safari flake from #732 is unresolved, and the new end hold keeps its mechanism alive.
- "never more than one menu open at a time" (
mobile-touch-ux.spec.ts:277) is unchanged in this PR. It readskebabs.nth(2).boundingBox()afterselectRoomwith no settle, then clicks that coordinate. - The opening now installs an end hold that re-lands the view on row growth (decryption placeholders in "Your Private Room",
on_history_resize~3916). That is exactly a latescrollTopwrite in the window between the read and the click. - Please make the test wait for stable geometry (the same box on two consecutive frames, or the settle helper), and show it holding with
--retries=0 --repeat-each=20on mobile-safari under worker load, not in isolation.
- "never more than one menu open at a time" (
- The read rule's range guard has no browser test.
- The rule says a room isn't marked read unless the rendered range reaches the latest message (
if !history_window.has_newer { newest_rendered = … }, ~1371). Only the pureread_marker_updateis unit-tested, and it knows nothing about ranges. Dropping the guard would pass every spec. - In "reading down a held range pages the withheld messages in": read to the end of the held range, leave, assert the badge is still shown. Then page to the newest, leave, assert it's gone.
- The rule says a room isn't marked read unless the rendered range reaches the latest message (
Should fix
- An own send at the window ceiling withholds the sent message (
conversation.rs~3152–3191, ~5142).select_latestreadshas_newerfrom the previous render. With the range exactly at the cap and ending at the newest message,has_neweris false, so the send's new item makesend - start > cap.- With
RangeHold.parkedgone,hold.end.is_none()now caps the end atstart + cap. The sent message is withheld andcomplete_scroll_requestlands at the end of a range that doesn't contain it. mainslid the start here. When a request is pending, slide the start (start = end - cap) instead of capping the end.
- On mobile, an arrival in the current room while the chat column is hidden has no signal at all (
document_title.rs:639,room_list.rs:601,notifications.rs:1200).- The read marker correctly stays behind. But the hamburger total and room badge exclude the current room, notifications are suppressed for it, and Latest is inside the
display:nonecolumn. - Scenario: open Members or Rooms, a message arrives in the current room, and nothing indicates it until the user goes back to Chat.
- The read marker correctly stays behind. But the hamburger total and room badge exclude the current room, notifications are suppressed for it, and Latest is inside the
- An empty DM thread auto-scrolls on its first inbound DM (
dm_thread_modal.rs~488).- The placement effect waits until a bubble mounts, then
is_first = !first_scroll_done.replace(true)treats that bubble as the opening jump, whether it is inbound or outbound. - The first inbound DM into a thread opened empty therefore scrolls to the end, and the witness marks it read. That contradicts "inbound DMs never scroll".
- Count the opening as done once the thread has rendered, even when it's empty.
- The placement effect waits until a bubble mounts, then
- A contended
DM_LAST_SEEN.try_peek()drops the read advance with no retry (direct_messages.rs:87). The room path schedules a nudge on the same contention; this path should too. Otherwise the last witness after reaching Latest can leave the thread badged until it's reopened. - Deleted tests that still guarded live behaviour. Please restore these, or show what replaces them.
- "does not drag a parked reader down when a resize reflows the history" (old
conversation-autoscroll.spec.ts:365). The reflow test now only covers a reader at the end, and the new bottom-edge correction movesscrollTopon height changes. - The arrival after the final newer page is rendered, not withheld (old ~1205/1230, the #747 review item). "reading down a held range…" dropped its trailing arrival check.
- "arrivals do not move a reader parked in a windowed room's history" (old ~526). Only the Rust test remains.
- The "newer-page jump of ~6,000px" fix in the description. Please name the test that fails without it.
- "does not drag a parked reader down when a resize reflows the history" (old
- Tests for new rules that are stated but not pinned:
- Hidden panel with the reader at the end: hide, deliver, reveal, expect the row held and Latest shown. Then hide, grow a held image, reveal, expect the end still in view.
- The 4px end-hold release boundary: scroll up 3px (hold kept) and 6px (released). Today a threshold of 2 or 3 passes.
overflow-anchor: none: the PR's own argument is that CI's WebKit anchors where iOS doesn't, yet the only guard is aninclude_str!scrape. Assert the computed style in Playwright.test.skip(!hasScrollend, …)silently skips on an engine withoutonscrollend. Assert it's present on the projects that should run instead.
- The new agent rule files encode the contributor's choices as settled project policy.
.claude/rules/history-scrolling.mdsays the change "deliberately replaces automatic following", that it "supersedes" #486, and that "the tradeoff is explicit". Its "Accepted for now" notes (the end-hold release inside the padding, same-second DMs) record known defects as policy, so future agents will leave them alone.- Please reword it as a neutral description of current behaviour, at least until the maintainer decides.
- Dangling references: "decisions 6, 12" in
scroll_to_latest.rs;NEWEST_DM_IN_VIEW_SLACK_PXinhistory-geometry.ts(doesn't exist);KNOWN_FAILURE_SHA"main 739fd68".
- The description doesn't match the code in four places.
- "Up to the newest message seen" means "only once the newest message's bottom is on screen": reading 24 of 30 new messages marks none.
- "DM threads: same rules as rooms" isn't true. DMs have no end hold and no bottom-edge correction, as the rule file says.
- The edit-form reveal changed from smooth/start-aligned to instant/nearest, which isn't mentioned.
- At the 240-item ceiling, a reader idle at the end now gets a held range, which isn't mentioned.
Consider
- A backfill releases the end hold.
end_hold_survivesneedsnow.count <= before.count. On a tall screen where the opening tail is shorter thanclientHeight + BACKFILL_LEAD_PX, the first backfill clears the hold the opening just set, so images or decryption landing afterwards push the newest message off screen. Treat "count grew and the newest is unchanged" as a backfill that keeps the hold. - The end hold never expires. Any later growth (a reaction pill, a late image, minutes later) re-lands the view at the end until the reader scrolls up more than 4px.
- A modal over the history still counts as seen. In a short room, an arrival that fits on screen is marked read while a DM thread or member modal covers it.
- Sequencing. HostFat's open #743 and #745 touch
conversation.rs. Plan the rebase order with them. - Pre-existing, not from this PR. A send in a private room without its secret falls back to a public body that river-core rejects after the draft is cleared, so the message is lost silently. The new spec at diff ~7536 relies on that path; it needs its own issue.
Verified as fine
- Room switch with a pending request. It's replaced in render before the patch, the own send re-checks
CURRENT_ROOM, and captures from the old room are dropped. - Read rule interleavings. Every interleaving traced came out right. Nothing marks while the tab or panel is hidden, marking never goes backwards, and all four removed mark-read call sites (hydrate, synchronizer ×2, visibility) are covered by the title effect.
- DM lifecycle. The keyed body, the
use_droplifetime flag checked by queued placements and the witness, and inbound DMs never scrolling an open thread. - Signal safety. Raw callbacks defer every signal write, render touches only
Cell/RefCell, and the new effects anchor before their fallible reads. - The rewritten #486-family tests keep their triggering events and can fail. The test hooks drive real production paths (
releaseHeldDmPlacementthrows if nothing is held). - "UI only." All 27 files are under
ui/or.claude/rules/; there are no Cargo, WASM or contract changes.
HEAD SHA reviewed: 6d34ed6fd8e6f44eca6919ba91eb20d9931c152d
[AI-assisted - Claude]
…ge-and-scroll--anchoring
|
Updated the PR description for review item 11:
For item 1, the description now says removing automatic following needs a maintainer decision. That decision, arrival-signal requirements, and the remaining code/test findings are still open. |
|
Thanks for the review @sanity !
|
|
Fixed review item 8 in 39b47e9.
All five scenarios passed across all five browser projects: 25 checks, retries disabled, on a fresh static build. "A newer page keeps the reader's row" already covers the ~6,000px jump. Disabling the paging compensation made it fail in all five projects with ~6,200px shifts. I reverted that experiment. The automatic-following decision and the other Must fix items, including the held-range unread-badge check, remain open. |
|
@glesage, a decision on Must 1 from my review: keep automatic following. If the reader is at the end of the history when a message arrives, the view should move to show it, as it does on The goal of fixing the follow bugs (#723, #508) is still right. Please get there without the old latched pin, using the stateless pieces this PR already adds. Roughly:
The end hold, the bottom-edge correction and the reveal restore can stay as they are. Concretely, for this PR:
The other findings in the review (the mobile-safari flake, the read-rule range test, the own send at the ceiling, the DM first-arrival scroll, and so on) still apply. [AI-assisted - Claude] |
I responded in freenet builders River room as well but posting here for archive:
|
|
@glesage, thanks for the questions on River. Let's keep this discussion here on the PR, where it's easier to follow. Mobile-safari flake: agreed, it's on Range-guard test: agreed, it isn't blocking. With the 240-item ceiling against a 100-message default cap, it's close to unreachable, so skip it unless a cheap unit test falls out naturally. Following: I think two behaviours are getting mixed up here, and you're right about one of them.
#753 still jumps to the end when a room opens (in the description: "Opening, sending and Latest move to the end instantly"), so removing following doesn't actually fix the problem you hit when switching rooms or opening River in a new tab. #459 does. Please don't close this. Most of the work here stands on its own:
A path that gets you what you want:
If you see it differently, say so here and Ian can weigh in. [AI-assisted - Claude] |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every modal root, and the two popovers attached to a history row, mounts a ModalPresence. It registers the modal while mounted and bumps FOREGROUND_CHANGED when it unmounts; the tab becoming visible bumps it too. A source scan pins a presence in each root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The room and DM read rules now ask foreground::in_foreground, and re-check on FOREGROUND_CHANGED (the tab became visible or a modal closed) instead of on DOCUMENT_VISIBLE alone. An arrival on screen under a modal, or under a popover attached to a history row, stays unread until it closes; closing it reads what is on screen without scrolling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a render brings a newer newest message and, before the patch, the room was in the foreground with its newest message's bottom within 100px of the view's bottom edge, the render asks for one move to the end. Stateless: the check reads the pre-patch DOM, so nothing can latch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A followed burst, or an own send, whose arrivals take the range past the render ceiling used to land at the end of a held range with the newest messages still withheld. A pending request now takes the latest range instead, which also resolves the own-send-at-ceiling review item. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The freenet#486, freenet#501, freenet#723 and freenet#747 scenarios now assert that a reader at the end is followed, and parked readers keep their place. Tests whose premise was an arrival landing unread below a reader at the end park the reader first, or deliver in a hidden tab. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The render that first shows a newer DM measures, before the patch, whether the thread is in the foreground with its newest DM's bottom within 50px of the view's bottom edge; the placement effect then follows it. A thread opened empty has no opening to place, so its first DMs follow by the same rule (review item 6). Adds an admitDmPeer test hook for an empty thread. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Why did we add lines of code for functionality? because we fixed some bugs/handled cases that were broken before. Otherwise we would have only reduced the line count 🥳
Summary
This PR replaces the latched follow pin with stateless rules. An arrival follows a reader at the end of a conversation in the foreground, and any other arrival keeps the reader's place. Room read markers advance only after the rendered range reaches the latest message, its bottom is visible, and nothing covers the conversation; reading part of the unread history does not advance the marker step by step. DM threads share the same follow, Latest and read rules, with the exceptions below.
Per the review discussion, following is kept, but stateless. An arrival scrolls to the newest message only when both of these hold:
The check reads the pre-patch DOM in the render that brings the arrival, and nothing is remembered between arrivals. That rules out the latched pin behind #486, #508 and #723.
Only an arrival follows. A tab becoming visible, a modal closing or a mobile panel reopening never scrolls: arrivals in the meantime stay below, Latest appears, and they stay unread until the reader reaches them.
Browser scroll anchoring stays off (
overflow-anchor: none). Safari only supports it from 27.0, and Playwright's WebKit anchors where iOS 26 doesn't, so CI would miss the regression.Accepted limitation — same-second DMs: DM read markers use whole Unix seconds. Reading one inbound DM also treats other DMs from the same peer in the same room with that timestamp as read, including one that arrives later or remains below the fold. We accept this behavior; timestamp precision and read tracking are unchanged. This addresses review item 1 as an accepted limitation.
Accepted behavior for now — end hold: An upward scroll releases the hold once the scroller is more than 4px from its absolute end. Padding below the newest message means a small scroll can release the hold while that message is still visible and Latest stays hidden. We accept this behavior for now; the release boundary is unchanged. This addresses review item 2 as an accepted limitation.
Accepted trade-offs of following:
reader_moved_up_since, was the latch behind fix(ui): stale bottom-pin plus the max_scroll_top clamp can yank a reader who scrolls up just before a content-shrinking arrival #508.The rationale and current rules are in
.claude/rules/history-scrolling.md.Related issue dispositions
These dispositions describe the current implementation at
6fdd5d4d. No open issue is automatically closed by this PR.conversation-history-position.spec.ts.reader_moved_up_sinceand the old follow paths are deleted, and following reads live geometry. The shrinking at-cap burst before settle is a row-preservation regression test, with its expected-failure marker removed.UnevenTailRust fixture pin and the static-server restart note are not all completed here and remain follow-up work.Next: #459, opening a room at the first unread with a divider, as a separate PR.
Current behavior details
max(240, requested window + 60).Changes
conversation.rs:pinned_to_bottom,ScrollMark,reader_moved_up_since, the content-follow effect, the ResizeObserver follow branch,trim_landed,RangeHold.parked).force_scrollbecomes a one-shotScrollRequestthat carries its room.follow_arrival_at_endrequests one if the render brings a newer newest message (NewestKey/is_arrival) and the room was in the foreground within the band.ROOMSread, and the ceiling rule for pending requests.foreground.rs(new):in_foreground, the one predicate shared by following and both read rules. Each modal root and message-row popover mounts aModalPresence;FOREGROUND_CHANGEDis bumped when a modal closes or the tab becomes visible. A source-scan test fails if a modal root lacks a presence.document_title.rsand the seven mark-read sites: one rule,read_marker_update, fed byNEWEST_SEEN. Room history publishes its latest message only after the rendered range reaches it and its bottom passes the visibility check; intermediate messages do not advance the marker.dm_thread_modal.rs:DmKey/is_newer_dm, measured in render.ThreadSeenWitnessfor the timestamp read cutoff.test_hooks.rs(test build only): hook senders join as members; new hooks for clock skew, a failed send, DM threads, and admitting a DM peer with no DMs.Test coverage
--retries=0: 913 passed, 0 failed (28 skips, all present before this PR).cargo test -p river-ui --bins: 1078 passed.exceptname, the empty-thread rule, the modal-close and tab-visible re-checks, and the ceiling request rule.Screenshots
Redeploy
UI only. No changes under
common/, contracts, delegates orcli/, and the committed WASMs are untouched, so no migration or pointer-record signing. Publish withcargo make publish-riverafter merge.