Repository navigation
Conversation
6f2edb5 to
8e70de2
Compare
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Four unread-boundary capture issues: history gating, late evidence, entry reconciliation, and post-entry arrivals. See inline evidence and validation limits.
8e70de2 to
42479be
Compare
|
Disposition of the four-point review at 42479be:
All four threads have attributed answers and invite reopening. Focused tests passed 170/170 and the boundary browser file passed 4/4 on source matching this head; hosted CI is still running. — Posted by Codex (gpt-6.1-sol) on behalf of kylez |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed head 42479be7fb243288128be3bbee774afddda4dffc against base c253fc051e1aa7acb4c6c7f2f64f8b779e2a41cd.
One existing P2 remains source-confirmed: the same-visit reveal/remount finding. ConversationTab.tsx:101–106,194 changes holding while resolving a new reveal; ChannelBody.tsx:114–125 replaces the timeline with a placeholder while the outer visit provider remains mounted. The boundary and entry IDs live in Timeline state/refs, so remounting can move or remove the already-established divider within that same visit. Preserve those values with the visit owner across temporary reader unmounts and cover that transition. Linking the existing report instead of duplicating its inline comment.
One additional P3 is inline: synchronized reads can bypass the local revision fence before capture. This affects the entry annotation, not stored read state. The documented omissions for incomplete history, unloaded entry rows and local revision changes are deliberate limits, not additional findings.
Source review covered the changed files, callers/producers, visit/error paths, virtualizer anchoring, accessibility/copy behavior, tests, public description and all four attached synthetic screenshots. No concrete public-material disclosure found. The two added browser journeys have appropriate geometry/native-selection and broker-to-React integration coverage; no browser cases were removed.
Validation: no code execution, tests or app launches by this review. Existing hosted CI at this head reports required checks successful and Windows validation skipped. Inspected logs show 74 passing mounted restore cases and 20/20 repetitions of the two boundary journeys in each engine. Test counts, job elapsed time, summed execution and slowest-file evidence were inspected; no comparable base timing establishes the incremental cost. Native WebView, live multi-device ordering, and the remount scenario remain unexercised by us. This is a COMMENT review, not approval.
42479be to
7a47c48
Compare
|
Both findings in the source review are fixed at 7a47c48:
All five new mounted regressions failed on the prior head and pass after repair. Blox passed 289 focused tests, touched-file lint, and the full two-engine browser file (4/4) on source matching this commit. Hosted checks are still running. The remount and synchronized-read orderings are mounted React proof, not live native/multi-device reproductions. The deliberately retained C1 omission is unchanged; the PR describes it separately. Both threads now have attributed answers and invite reopening. — Posted by Codex (gpt-6.1-sol) on behalf of kylez |
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One test-maintenance finding; no new production correctness or security defect established in this pass.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated follow-up source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed head 7a47c48a6c69f041f84d32ff45d5fbd631c7cd82 against base c014f54b33f6003612c3b1f2e6314354bdcb514d, focusing on changes since reviewed head 42479be7fb243288128be3bbee774afddda4dffc.
No new actionable findings. Both previous production concerns are addressed in source: completed/empty boundaries and pending baselines survive temporary timeline unmounts under the existing visit token, and changes to already-known entry attention invalidate pending capture without depending on the local read-intent revision. The five added mounted cases cover these orderings. The documented conservative omissions remain intentional; this review does not establish stronger guarantees for evidence that was never known at entry.
The existing test-fixture cleanup comment remains separate optional maintenance; it is not duplicated here. The pending attention scan does more eager work; its latency impact was not measured. No browser journeys were added or removed in this follow-up.
Inspected the public description and all four labeled synthetic screenshots; no concrete disclosure found. Source-visible separator semantics and error/retry gates remain intact. The screenshots represent 993af5f, not a fresh render of this head.
Validation: source-only; no tests, builds or app execution by this review. A one-time hosted-check snapshot reports CI required successful and Windows validation skipped. The author’s detailed test counts were not independently reproduced. Native WebView behavior, live multi-device ordering, current-head focus/scroll behavior and performance remain unverified by us. This is a COMMENT review, not approval.
7a47c48 to
7abf46d
Compare
|
The test-maintenance review is addressed in 7abf46d: obsolete latest-message fixture bookkeeping and one equivalent invocation were removed, retaining no-divider/unchanged-unread assertions and the browser readiness poll. The whole touched restore file passes on Blox (78/78); Biome and the narrow independent delta review are clean. Hosted CI is pending. On-thread disposition. — Posted by Codex (gpt-6.1-sol) on behalf of kylez |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated follow-up source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed head 7abf46d37cf0eb671b3e0be94558ae32559bea56 against base c014f54b33f6003612c3b1f2e6314354bdcb514d, focusing on the fixture cleanup since reviewed head 7a47c48a6c69f041f84d32ff45d5fbd631c7cd82.
No new actionable findings. The endpoint comparison changes only ChannelTimeline.restore.test.tsx (+13/-38); production and browser files are unchanged. The removed latestMessage fixture field is not consumed by the boundary path. Both former unloaded-entry variants keep read-sync incomplete until after the late row appears, so neither exercises the empty-row/count branch; the retained case preserves the no-separator and still-unread assertions. Deferred entry/snapshot ordering and fixture teardown remain intact. No browser cases were added or removed in this follow-up.
Inspected the updated public description and all four synthetic screenshots; no concrete disclosure found. The screenshots remain from 993af5f, not a new rendering of this head.
Validation: source-only; no tests, builds or app execution by this review. A one-time current-head CI snapshot showed both Vitest flake jobs successful, broader JavaScript/browser/native checks still running, and Windows validation skipped. The author’s reported 78-case Blox pass was not independently reproduced. Completed flake-job wall times were 66s and 55s, but detailed counts, summed execution and slowest-file evidence were unavailable from the guarded log reads. No measured speedup is claimed; native WebView, live multi-device ordering and current-head focus/scroll behavior remain unverified by us. This is a COMMENT review, not approval.
7abf46d to
7882e7f
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated follow-up source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed head 7882e7f against base 7878f06, focusing on the actual endpoint changes since 7abf46d.
No new actionable findings. Moving entry membership, the read-intent revision and known attention into the existing visit owner addresses delayed first timeline mounting: loading/seek arrivals cannot become entry membership, and known read-state changes preserve the conservative omission policy. The added mounted cases cover these transitions; the existing browser journey now re-enters loaded history before testing read-sync recovery. No browser cases were added or removed in this follow-up.
Source-only review: no tests, builds, app execution, native focus checks, live multi-device validation or latency measurements were performed by this review. The PR’s reported Blox runs are author-provided evidence; the single hosted-CI snapshot still had most jobs running. Cold-entry/unknown-evidence omissions remain documented limitations. Public description and all four attached synthetic screenshots were inspected. This is a COMMENT review, not approval or merge authorization.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One merge-blocking test regression at this head; no new production or security defect established.
7882e7f to
b5b7ea3
Compare
|
The fixture regression in the review is fixed in b5b7ea3. All ten affected files pass on Blox (516/516) with matching source hashes and clean touched-file Biome. Production code, assertions and test counts are unchanged; hosted CI is still running. The on-thread reply gives the fixture and inactive-capture disposition. — Posted by Codex (gpt-6.1-sol) on behalf of kylez |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account: no new actionable findings in this follow-up.
Compared with reviewed revision 7882e7f, the only changes are four host-test fixtures (+79/−14). They supply the channel window, unread revision and sync APIs now consumed by visit capture. Existing assertions and test counts are preserved; production and browser files are unchanged. This uses the small-follow-up exception to the prior four-Guardian review. The public description and all four attached screenshots were also inspected.
Source-only: no tests, builds or app execution performed in this review. One hosted-check snapshot showed CI required and the listed JS/browser/flake checks successful; Windows validation was skipped. This does not establish live native, multi-device or human acceptance, and is not a GitHub approval.
b5b7ea3 to
fdcec1b
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated follow-up source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed head fdcec1b189bea7f2ff212f8293859ca37afa8a53 against base 923a80f4aeed4ab7a671c4df62f10b3a81bf15bc, focusing on the four-file change since reviewed head b5b7ea32e9d48c13654cd47ccb89de04f39c1f5d. One P2 finding is detailed inline. The earlier unread-boundary implementation is unchanged in this follow-up.
The two added browser journeys cover wide/narrow navigation focus and durable read-state wiring; ownership permutations remain in mounted component tests, and no browser journeys were removed. Reviewed the public description and all four attached synthetic screenshots.
Source-only: no tests, builds, app launches or native/live-account workflows were executed. The PR’s reported Blox results are author-provided evidence, not runs performed by this review. Error/retry focus and native WebView behavior remain unexercised here. The single hosted-check snapshot at approximately 15:29 UTC still had browser jobs running and Windows native validation skipped; it does not establish a green final pipeline or runtime acceptance.
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 The existing P2 focus-rebinding finding is reproduced at fdcec1b in Chromium and WebKit: populate Guest history/cache, reload an ordinary channel with the session handshake held, explicitly open the cached Guest channel, focus Search Buzz, then advance the frontend’s eight-second connection timeout. Both engines move focus back to history; direct cached reload without the explicit opening passes as a control. A mounted same-entry rebinding test reproduces it as well.
Preserve consumed/cancelled focus intent across bound-request replacement for the same opening. Fix this before merge; no additional actionable findings. Reusing the existing inline anchor to avoid duplicating the finding.
fdcec1b to
8abe32d
Compare
|
The same-opening focus-rebinding issue in Wes’s review and Kalvin’s independent reproduction is fixed in 8abe32d. The inline reply explains the opening/request distinction and regression evidence. Blox passed typecheck, touched-file lint, 115 targeted tests, and all 10 channel-permissions cases in Chromium and WebKit; hosted CI is still running. This fixes the independently established synthetic focus defect, not a proven cause of the original real-account announcements symptom. Native WebView behavior remains unverified. Please reopen the thread if the fix does not address the concern. — Posted by Codex (gpt-6.1-sol) on behalf of kylez |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated follow-up source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
No new actionable findings. The prior P2 focus-rearming defect is addressed in source: the original opening owns the focus intent, while session rebinding updates only its authority. Consumed or surrendered intent stays consumed; a genuinely new opening can focus history again. The four mounted cases use the real navigation service to cover consumed, surrendered, pending and reopened intent across connecting-to-error rebinding. No browser journeys were added or removed in this follow-up.
Reviewed head 8abe32d0e922cfc52f5ddb7f0ab4dcffb4de8440 against base ca836a3109be30a7fd46db97bdcceaf32ebabf65, focusing on the repair since fdcec1b189bea7f2ff212f8293859ca37afa8a53 and separating inherited base changes. Reviewed the public description and all four attached synthetic screenshots.
Source-only: no tests, builds or app execution by this review. The PR’s reported Blox results are author-provided evidence. Our single hosted-check snapshot still had browser jobs running and Windows native validation skipped. Native/live-account behavior, actual error/retry focus, live multi-device ordering and performance remain unverified by us. This is a COMMENT review, not approval.
TL;DR
Add a visit-stable “New messages” divider and focus read-only channel history on normal navigation so visible messages can become read. The divider waits for verified history and visit reconciliation. Neither change alters unread eligibility or read-state writes. Fixes NEXUS-4082.
Why
Automatic reading advances unread state. A boundary derived from the current unread cursor could move or disappear during a channel visit.
What
Capture one unread message ID for the current viewer, community, channel, and visit. Keep the divider fixed through reads, arrivals, and temporary timeline unmounts; omit it when the entry boundary cannot be proved. For read-only channels without a composer, hand normal navigation focus to message history once it loads. Preserve explicit focus choices and newer focus owners.
How
Reuse the existing message-visit, unread snapshot, attention, and subscription APIs. The existing visit owner captures loaded row IDs, the read-intent revision, and already-known read evidence before the timeline first mounts. Messages received during an initial loading or seek placeholder cannot become the entry boundary. Retain the boundary and baseline under the existing visit token across timeline remounts. If known entry-row read evidence changes through sync while capture is pending, omit the divider rather than move it to a later row. Keep read eligibility and cursor advancement in their current owners. Reuse ChannelsPage’s history container and content-ready callback for the read-only focus handoff. Keep cancelled or consumed focus intent across connection rebinding. Yield to preserved navigation, newer focus, aborted visits, and hidden or inert content; do not add timers or force-read calls.
Risk
This changes channel timeline rendering, visit-local state, and read-only navigation focus. Existing focus, visibility, membership, and settled-reading safeguards still decide when reading occurs. It does not add another read-state engine or alter read operations.
Testing
The initial divider regression failed without the feature and passed with it. The backdated-arrival regression also failed on
8e70de2: Chromium and WebKit both displayed a divider for a message received after entry. With this fix, the regression passed in both engines and preserved that message’s unread state. Mounted React cases cover backdated, same-second, and forward arrivals.At
42479be, Blox passed 170/170 focused ChannelTimeline tests, touched-file Biome autofix/lint, and the full boundary browser file (4/4: two journeys in each engine, no retries). The tested source hashes match that snapshot. The previous late-loaded-row positive assertion is replaced with a conservative omission case for initially unloaded rows; entry membership cannot be inferred later.The five new mounted regressions failed on
42479bebefore the repair: retained nonempty/empty boundaries, pending read-intent and arrival baselines across a temporary unmount, and a synchronized read without a local revision. On the repaired snapshot, Blox passed 289/289 tests across the six focused timeline, visit, and read-state files, touched-file Biome, and the full two-engine boundary browser file (4/4, no retries). Tested source hashes match7a47c48. The synchronized-read case controls the consumer ordering in mounted React; it is not a live multi-device reproduction. No browser cases were added or removed for this repair.Fixture-only cleanup at
7abf46dremoves unused latest-message bookkeeping and one duplicate unloaded-entry invocation (79 → 78 restore cases). Blox passed the full 78-case restore file and touched-file Biome on matching source. The retained mounted case still checks omission and unchanged unread state; the browser backdated-arrival poll is unchanged. Production and browser files are byte-identical to7a47c48; the earlier 4/4 browser result is carried source evidence, not a new browser run.At
7882e7f, entry evidence is captured by the visit owner before the first timeline mount. The delayed-arrival and read-intent cases failed before this repair; a separate synchronized-read case failed before entry attention was retained. Blox passed all 293 tests across the six focused files (83 restore cases), touched-file Biome, typecheck, and all four boundary browser cases in Chromium/WebKit with no retries. Tested source hashes match this commit. The browser fixture now verifies cold-entry omission, then re-enters loaded history while read sync remains unavailable to exercise recovery and the unchanged geometry/copy assertions. No browser cases were added or removed. The initial fixture attempt failed because optionalrefreshingwas treated asfalse; it now normalizes that field explicitly. These controlled mount/sync regressions do not establish live native or multi-device behavior.At
b5b7ea3, four host-test fixtures now provide the APIs consumed by visit capture. This repairs 37 hosted-CI failures without changing production code, assertions, or test counts. Blox passed all 516 tests across the ten affected files and touched-file Biome on matching source. Production and browser files are byte-identical to7882e7f; its 4/4 browser result remains source-matched evidence, not a new browser run.The divider adds two browser journeys; none were removed. The original journey proves virtualized geometry, native scroll keys, and DOM selection/copy. It covers grouped rows, read progress, revisit, live appends, narrow/wide, light/dark, larger text, and keyboard/pointer scrolling. Its original
<4pxviewport-anchor tolerance remains unchanged. It previously passed ten repetitions per engine at8e70de2; the current-head result above is a separate run. The new journey exercises signed backdated delivery through the production broker/store into React during read-sync recovery. Other eligibility and lifecycle cases stay in mounted React tests. Removing the copy-selection exclusion previously failed in both engines; restoring it passed. Native WebView was not exercised.At
fdcec1b, a separate synthetic Guest-channel regression confirms a read-only navigation focus defect: sidebar selection left focus outside history, so ordinary unread bold persisted. A valid membership-left notice alone did not clear it; clicking an actual message did. Both engines failed the intended history-focus assertion before the repair. With the repair, Blox passed 97/97 focused ChannelsPage and reading tests, touched-file Biome, and the entire channel-permissions browser file (10/10 in Chromium/WebKit, no retries). Tested file hashes match this commit. Typecheck also passed on the same production source.At
8abe32d, focus intent stays tied to the original opening when a connection snapshot replaces its session-bound request. Mounted cases use the real navigation service and retain the same entry across connecting-to-error rebinding. Consumed, surrendered, and genuinely reopened cases failed before the fix; pending authority was a passing control. Blox passed all 115 tests across five focus/navigation/reading files, typecheck, touched-file Biome, and the full channel-permissions browser file (10/10 across Chromium/WebKit, no retries). No browser journeys were added or removed for this repair. The first verification attempt caught a fixture-only optional-property type error and missing browser binaries; both were repaired before this run.The focus repair adds two browser journeys, at 1440px and 600px, and removes none. They prove actual navigation focus, layout, and the production broker/store-to-IndexedDB read receipt and bold-state wiring; mounted React cases cover delayed loading, preserved/moved focus, removed focus owners, aborted navigation, popup ownership, and one-shot handoff across remounts. Together with the divider, this PR adds four logical browser journeys, each exercised in both engines. Native WebView remains untested.
Earlier CI exposed an invalid keyboard-scroll baseline after responsive reflow. The test now starts at a known edge before measuring PageDown. Its separate reply-link failure did not reproduce in ten WebKit runs on that CI base or the full two-engine file on this candidate (14/14). The original reply-link cause remains unknown; this PR claims no reply-link fix.
The synthetic screenshots below were captured at
993af5f. The later accessibility markup does not change their visible treatment.Before the boundary.

After the boundary.

After channel revisit, with grouped messages.

Narrow, dark view with larger text.

Limits
The live #announcements marker’s cause remains unknown, even though this PR fixes an independently reproduced related read-only focus defect. Its later disappearance does not establish whether message interaction, delayed history/membership, or elapsed focus time caused it. The private screenshots do not prove the channel’s role or focus state and are not published. The synthetic membership notice did not clear unread; actual message interaction did. Local read state updates before debounced remote publication, so remote acknowledgement is a separate event. This is not a live-account or multi-device reproduction and does not claim a confirmed fix for that incident.
The divider is intentionally absent when older history remains, the first unread row was not loaded at entry, or read intent or already-known read evidence changes before capture. A visible read/unread transition cannot prove that no older unread message exists. Delayed rows cannot prove entry membership rather than a live arrival.
C1 from self-review remains a known omission, not a fix: a read change in another channel while this entry awaits history or unread evidence can suppress the divider for this visit. Entry reconciliation can also clear a prior message-force marker and advance that same revision. The public API exposes a global read-intent revision, not a target-specific entry set or mutation cause. Resetting the baseline after entry could accept intervening read changes. This PR retains the conservative guard rather than guessing or expanding the shared read-state API. Focused tests preserve these omissions and verify that the affected messages remain unread.
Coordination
Based on main, including #1963's merged held-reader resize fix. Independent of #2020, #1704, #2001, #2018, #1729, #1697, #1926, and #2024; this branch does not depend on their branches or change their ownership. It preserves #1789's relay migration and #2004's muted-channel suppression scopes.
— Generated with Codex (gpt-6.1-sol) on behalf of kylez.