Preserve thread notifications when opening channels - #6153
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 146d7b1f45
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
Review of 146d7b1f4519844714612fe86e7ca33020d156cc
Requesting changes for one user-visible unread-count regression.
Major — collapsed thread badges count an already-read ancestor as new
desktop/src/features/channels/ui/useChannelUnreadState.ts:390-403 now evaluates every top-level thread root against the channel timeline marker, while the branch-badge paths at :308-370 still evaluate each message against its effective msg:<id> marker. Opening a thread marks the revealed replies at :270-282, not its root. Those predicates therefore disagree despite the one-source-of-truth contract documented at :378-388.
In the established workflow—open and close a thread, switch away, receive deep replies, return, then reopen the panel—the collapsed branch badge includes one previously seen child:
desktop/tests/e2e/thread-unread.spec.ts:488: expected2, rendered(3 new)desktop/tests/e2e/thread-unread.spec.ts:554: expected1, rendered(2 new)
This is deterministic on the exact head: Actions run 32056994692, job 95469495567, reproduced both failures on the initial attempt and both retries. I also reproduced both locally from a fresh pnpm build:e2e, served on an isolated port to avoid Playwright's stale-server reuse:
# HEAD verified as 146d7b1f4519844714612fe86e7ca33020d156cc
cd desktop
pnpm build:e2e
pnpm exec playwright test tests/e2e/thread-unread.spec.ts \
--config=<isolated-port config, reuseExistingServer=false> \
--project=smoke \
--grep '05-thread-in-panel-subtree-badge|06-in-panel-badge-bumps-on-live-reply'
# 2 failed: expected 2 / received (3 new); expected 1 / received (2 new)The new channel-activity-popover.spec.ts row proves the sidebar dot survives passive channel opening, but it never opens the preserved thread and misses this adjacent regression. Please keep passive opening scoped so it does not clear thread notifications, while retaining the per-message frontier for roots/branch counts, then require these two existing rows and the new channel-activity row to pass unchanged.
Other changed-path systems checks were clear: 55 focused unit tests passed, pnpm typecheck passed, and no additional material tenancy, persistence, async recovery, security, accessibility, or platform issue was found. The separate Desktop Core ETXTBSY failure is outside this diff; it does not explain the deterministic changed-surface E2E failures above.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 941c3f109e
ℹ️ 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".
|
@jedwards27 Fixed the collapsed-badge regression in You were exactly right about the root cause: the root-badge path evaluated top-level messages against the channel-timeline marker while the branch-badge paths still evaluated per-message This restores Verification at |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed base 85bacea52b8359999f22c6ac07207a130809c488 through exact head 941c3f109ee8ea0c8ce07191a55881e36c9e9e1e.
Major — the preserved unread dot becomes non-discoverable and non-actionable after opening the channel
desktop/src/features/channels/ui/useChannelUnreadState.ts:171-190 folds the active channel's effective marker—including channel-timeline:<id>, which passive channel navigation advances—into getActiveMessageReadAt. That resolver is then used for main-timeline thread badge state at desktop/src/features/channels/ui/useChannelUnreadState.ts:365-391. Independently, desktop/src/app/useChannelActivityProjection.ts:103-123 filters the synthetic thread activity rows against the advanced frontier, while desktop/src/features/sidebar/ui/ChannelActivityPopover.tsx:252-300 only mounts the preview when rows survive. The sidebar dot comes from a separate observed-event path (desktop/src/features/channels/useUnreadChannels.ts:817-850), so these projections disagree.
For an unread thread reply older than a newer top-level message, passive-opening the channel leaves the sidebar dot visible but removes both ways to locate the preserved activity: there is no inline thread unread badge, and hovering the channel yields no activity popover or “Open thread” controls. The notification survives only as an unexplained dot. In a busy channel, that is not a usable preservation of thread activity.
The checked-in regression at desktop/tests/e2e/channel-activity-popover.spec.ts:412-428 asserts only that the dot survives before explicitly marking the channel read; it does not assert that the preserved replies remain discoverable or actionable after navigation.
Reviewer-only exact-head workflow probes reproduced both manifestations:
- extending the existing older-thread/newer-top-level scenario to require a timeline
thread-unread-badgefailed 3/3; changing only the badge computation back to the message marker made the probe pass 1/1 after a fresh E2E build; - a before-open control found both synthetic thread activity rows, but after opening the channel the activity popover did not mount and a direct row probe found 0 preserved rows.
Please keep synthetic thread rows and main-timeline badges under a frontier that does not treat a passive top-level timeline read as reading older thread replies. Strengthen this regression to assert, after passive navigation, the corresponding inline badge and both activity rows/“Open thread” controls; exercise one control; then verify explicit channel mark-read clears the dot and rows. Keep the established thread-count regressions green.
Validation on clean detached exact head: pnpm typecheck passed; full Desktop unit suite passed 4,986/4,986; fresh E2E build plus touched workflow specs passed 24/24; focused workflow probes above reproduced the gap. Exact-head Desktop unit/smoke/integration/build CI is green. The remaining Security failure is a repository-level Rust dependency advisory outside this frontend delta, so it is not attributed to this change. Ownership/tenancy, relay+pubkey scoping, persistence fencing, lifecycle/reset, failure recovery, security exposure, and compatibility were also checked in the changed paths with no additional material findings.
|
Intentional — declining the revert. This is the designed behavior. The preserved unread dot surviving a passive channel-open, and staying discoverable via the hover activity popover, is the intended product behavior (confirmed by the author). The "Major" here rests on the premise that the sidebar dot and the popover mount off different read frontiers and therefore disagree. On this head they do not:
The one place that does fold the timeline frontier is the in-panel thread badge ( Net: the preserved activity remains discoverable and actionable after navigation via the sidebar hover popover — that is the design, not a regression. Resolving as intentional. |
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed base 93114c9c65138397de39729fde0a816eb9f314ab through exact head c8e35ce15d75e746c27ae90d6937ca8c63fe0e17.
Major — cold catch-up can permanently miss preserved thread notifications beyond the newest 1,000 events
desktop/src/features/channels/useUnreadChannels.ts:72-77 caps each channel catch-up request at 1,000 events. The one-shot recovery path at :581-597 starts from the bare channel marker, fetches only that single newest-first page, and the completion path at :708-759 treats the channel as caught up without paginating to the timeline marker or exhausting the interval.
The marker split makes this truncation materially reachable: passive channel reads advance channel-timeline:<id> while deliberately leaving the bare channel marker stale. After a restart or on a new device, a still-unread eligible thread reply can therefore sit older than 1,000 newer matching events. The relay page omits it, the client records no observed unread/thread activity for it, and this session never requests the missing range again. A genuine unread reply is silently lost from recovery.
This is not hypothetical source speculation: the PR discussion independently confirms the path as a “genuine latent edge” and identifies pagination as the fix, but exact head still issues one bounded request. The focused changed-path tests and green Desktop CI do not cover a catch-up interval larger than the cap.
Please paginate with a stable until cursor until the relevant interval is exhausted (or use a relay query whose result cannot truncate eligible thread activity), and add a regression with more than 1,000 newer matching events proving an older eligible thread reply is recovered after cold startup. The test should also prove self-authored/non-trigger traffic cannot hide the eligible reply at a page boundary.
Major — the natural post-click hover state has no actionable thread rows
The two reviewer lanes reconciled their initially contradictory probes. In the natural sequence—click the dotted channel, leave the pointer on that sidebar row, then inspect its hover surface—the activity popover contains zero thread rows / “Open thread” controls even though the unread dot remains. Moving the pointer away and re-entering later makes both rows appear, which is why the first controlled probe passed; removing only that pointer move made the exact-head probe fail 3/3 with zero rows. The defect is therefore timing/state-transition dependent, not evidence that the rows are permanently deleted.
The checked-in row at desktop/tests/e2e/channel-activity-popover.spec.ts:412-428 stops after dot retention and never exercises this immediate post-navigation action path. Preserve or republish the projected thread items coherently during channel-open transition, and extend this exact workflow—without an artificial pointer reset—to require both seeded rows, two “Open thread” controls, and successful click-through before explicit mark-read clears them.
No additional material issue was found in the reviewed ownership/tenancy, persistence fencing, reset/cancellation, explicit mark-read, accessibility, or platform paths.
Validation at this head: 60 focused changed-path unit tests passed; the relevant channel-activity and thread-unread Playwright specs passed 24/24; exact-head Desktop core/build/smoke/integration checks are green. The PR is currently conflicted with live base, so conflict resolution will create a new head that requires re-review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 185639ec6d
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed base f88cda9eb886500ec7d205e1d265ac6f654aa433 → exact live head 185639ec6d995dc2b7185704b317328fca569cbb from a clean worktree.
Changes requested
[P2] Use the relay's composite cursor or cold catch-up skips replies sharing the page-boundary second.
desktop/src-tauri/src/unread_catch_up.rs:26-35,197-218 takes the oldest timestamp from each full 1,000-event page and fetches the next page with until = oldest - 1. That is incompatible with the relay's deterministic ordering and cursor contract: events are ordered by (created_at DESC, id ASC), and continuation within a tied second requires (until, before_id) (crates/buzz-db/src/event.rs:504-519,564-573).
If more than 1,000 matching events share the page's oldest second, page 1 returns only the first IDs from that second. Subtracting one from the timestamp excludes every remaining event in that second. A qualifying participated-thread reply after the first 1,000 IDs is therefore never classified, while the renderer retains the successful channel catch-up claim (desktop/src/features/channels/useUnreadChannels.ts:626-628,669-720). The unread notification is permanently lost for that cold-recovery session.
Please carry the last event's (created_at, id) into the next request as until + before_id and continue until exhaustion. Add a fetch-level regression with more than 1,000 events at the same timestamp, placing the qualifying reply beyond page 1, and assert both the request cursor sequence and recovered result. The current complete_paginated_batch_recovers_old_reply_behind_non_trigger_traffic test (unread_catch_up.rs:620-669) injects all 1,002 rows directly into classify_batch, so it bypasses page fetch and cursor construction; full_pages_advance_the_until_cursor currently codifies the lossy timestamp-only behavior.
What is fixed
The prior immediate post-click empty activity-row defect is resolved. The popover retains projected rows through the transient channel-open state change, preserves accessible button semantics and thread click-through, and clears only after an explicit read action (ChannelActivityPopover.tsx:237-327,357-381,409-486; channel-activity-popover.spec.ts:412-448). The regression was mutation-checked: restoring the prior component made the focused workflow fail with zero rows instead of two, and restoring this head returned it green.
Validation at this head
- Desktop unit package: 5,101/5,101 passed.
pnpm typecheck: passed.- Fresh
pnpm build:e2e: passed. - Changed browser workflows (
channel-activity-popover.spec.ts+thread-unread.spec.ts): 24/24 passed. - Focused post-click workflow: 1/1 passed, including keyboard activation.
git diff --check f88cda9eb886500ec7d205e1d265ac6f654aa433...HEAD: passed.- Full Tauri package test was not completed locally: the checkout lacks
desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwin; this is an environment/setup blocker, not a product-test failure. - Native Desktop GUI was not launched; browser E2E covered the React state transition without risking the installed app on the shared machine.
- At final review submission, several exact-head CI jobs were still pending. Completed Desktop integration, relay, macOS build, lint, and sampled smoke jobs were green. Green CI would not cover the tied-second cursor defect above.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfe5576edc
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — changes requested at exact head bfe5576edc20bb7367f3ac21d24992c0eafb0449.
-
[P1] Parse the mandatory
39006window-bounds overlay instead of treating it as a message/page cursor (desktop/src-tauri/src/unread_catch_up.rs:218-245).top_level_filteropts this request into the channel-window bridge, whose response always appends one relay-signed kind39006bounds event (crates/buzz-relay/src/api/bridge.rs:563-582); the protocol explicitly makes that overlay the only exhaustion authority (crates/buzz-core/src/kind.rs:436-439). The current code includes the overlay in classification, uses it inpage.len(), and takes it aspage.last(). A zero-row/non-full response can therefore manufacture unread state because top-level events passshould_notify, while 999 rows spuriously look full and 1,000 rows produce length 1,001; pagination then uses the overlay timestamp/id rather than{has_more,next_cursor}. Remove and validate the bounds event (channel and request cursor), classify only actual rows, and paginate solely from its content. Add fetch-boundary regressions for zero rows + bounds, 999/1,000 rows, and exact-multiple exhaustion; mutation-check by restoring the row-count/last-event inference. -
[P1] Clicking a channel with unread thread activity can open its hover popover without navigating (
desktop/src/features/sidebar/ui/ChannelActivityPopover.tsx:413-445). In the existing natural-pointer workflowdesktop/tests/e2e/thread-unread.spec.ts:716-755, after an unread reply lands ingeneralwhilerandomis active, clickingchannel-generalleaves the title atrandomand opens thegeneralChannel activity popover. This defeats the stated feature: the notification survives, but its ordinary follow-through action does not reach the channel. The exact-head changed-workflow run failed 2/24, and this row failed again alone at lines 747-748; I also inspected the failure screenshot, which showsrandomstill active beneath thegeneralactivity card. Ensure nested channel activation wins over or coexists with hover preview, then retain a deterministic pointer regression asserting both navigation and preserved thread unread state.
Validation at this exact head: clean worktrees; git diff --check passed; Desktop JS package passed 5,101/5,101; Desktop typecheck and E2E build passed. The changed browser workflow suite failed 2/24, with the navigation failure reproduced alone. Full Tauri package validation was blocked because desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwin is absent. Native GUI was not launched on the shared machine. CI was still in progress when rechecked, with completed integration jobs green; green CI would not exercise away either contract defect above.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e20d1eede1
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed base f88cda9eb886500ec7d205e1d265ac6f654aa433 through exact live head e20d1eede1b95c2308b6bcfb8d348d28d1471e62 (clean detached tree; remote PR head rechecked equal).
Blocking
P1 — ordinary sidebar activation is nondeterministically swallowed when the unread-activity popover opens.
The natural-pointer workflow still fails at this head: establish/read a thread frontier in general, leave for random, receive a new reply in general, then click the dotted general row (desktop/tests/e2e/thread-unread.spec.ts:716-755). In 20 single-worker repetitions, 3 clicks failed to navigate at all (chat-title remained random for the 5-second assertion) while the pointer-induced Channel activity card was visibly open over the sidebar; a fourth repetition navigated but lost the expected unread dot. Result: 16/20 pass, 4/20 fail.
The channel control remains wrapped by the controlled popover anchor (desktop/src/components/sidebar/SidebarSection.tsx:302-370), while hover/focus opens the preview (desktop/src/components/sidebar/ChannelActivityPopover.tsx:413-445). This head's pagination delta does not repair that interaction or add a deterministic pointer regression. The primary action therefore remains unreliable and can leave the selected channel content contradicting the row/activity card the user just activated.
Please make channel-row activation reliably win or safely coexist with preview opening, then add a deterministic pointer test that dwells/moves over a dotted row before clicking and asserts both chat-title=general and preservation of the unopened thread notification. Mutation-prove that regression by restoring the swallowing behavior.
Other reviewed boundary
The prior kind 39006 pagination blocker is cleared: the top-level path now consumes the relay's request-keyed bounds overlay, excludes that synthetic event from user rows, validates cursor progression, and preserves relay/identity scope across completion. Full Tauri package validation passed locally (2692 passed, 0 failed, 18 ignored in lib; integration targets 7/7 + 3/3), along with format, diff-check, restored pagination rows, and two causal pagination mutations. There remains no live multi-page Desktop→relay run, but source contracts and relay-side exact-multiple coverage make that a non-blocking residual gap for this revision.
CI was still in progress at review time. Regardless of its eventual result, the repeated user-path failure above blocks merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53ff7c43b5
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd24666746
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed base f88cda9eb886500ec7d205e1d265ac6f654aa433 through exact live head cd24666746b8415b835520650eb801422e9b21a8 from clean detached worktrees. Changes requested.
P1 — cold catch-up fails wholesale for users with more than 128 channels
The renderer submits every not-yet-caught-up channel in one command without a bound (desktop/src/features/channels/useUnreadChannels.ts:618-621,645-660). Native then builds one relay filter containing all successfully discovered channel IDs (desktop/src-tauri/src/unread_catch_up/relevant_threads.rs:18-28,64-77). That crosses the relay's hard aggregate explicit #h limit of 128 (crates/buzz-relay/src/handlers/req.rs:36-42,1099-1112; enforced at crates/buzz-relay/src/api/bridge.rs:975-985).
At 129 channels, the shared query is rejected before database work. Native maps that one error onto every discovered channel (desktop/src-tauri/src/unread_catch_up.rs:372-387), while the renderer releases their claims and later retries the same oversized batch (useUnreadChannels.ts:670-675). Consequently, none of those channels can restore historical unread/thread notifications after restart—even channels whose individual discovery completed successfully.
Please chunk the shared query at or below the relay contract and attribute failures per chunk. Add a 129-channel regression that fails against this head; the current two-channel test (relevant_threads.rs:98-112) cannot exercise the boundary.
P1 — viewing and leaving a channel can still erase an unopened thread notification
The checked-in user contract at desktop/tests/e2e/thread-unread.spec.ts:714-755 opens general, establishes a thread frontier, leaves for random, receives a reply in the unopened general thread, passively views general, then leaves again. The general dot must survive. On this exact head, 20 single-worker repetitions produced 15 passes / 5 failures. Every failure completed navigation back to random, but channel-unread-dot-general remained absent for the full five-second assertion; the captured failure state also showed random selected with no general dot.
The preservation path (desktop/src/features/channels/ui/useChannelUnreadState.ts:195-229 → desktop/src/features/channels/ui/unreadThreadEventIds.ts:22-39) depends on asynchronously projected preservedUnreadMessageIds plus a first-open snapshot. The repeated failure shows that exemption is not stable through the view→leave lifecycle. The new row at desktop/tests/e2e/channel-activity-popover.spec.ts:412-448 checks preservation only while still inside general, then explicitly marks it read, so its 20/20 pass does not cover this transition.
Please keep preserved thread-unread identity authoritative through channel leave/revisit and add a deterministic lifecycle regression that waits for projection settlement, leaves, and requires the dot to remain. Mutation-prove it against absent/stale preservation settlement.
Validation and residual risk
- Full Desktop JS package: 5,104/5,104 passed.
- Full Tauri package: 2,693 passed / 0 failed / 18 ignored; integration targets 7/7 + 3/3. Temporary empty aarch64 sidecar stubs were used only to satisfy Tauri build-time resource lookup and were removed; tree remained clean.
cargo fmt --manifest-path desktop/src-tauri/Cargo.toml --all -- --check,pnpm typecheck,pnpm check, and base/headgit diff --check: passed.pnpm checkreported only existing warnings/information outside changed files.channel-activity-popover.spec.ts: 11/11 passed; its new preservation row passed 20/20, and removingreadAtForPreservedUnreadMessagemade it fail causally.- Natural pointer dwell/click and keyboard focus/Enter probes passed 20/20 each, so the prior swallowed-navigation symptom is cleared in this revision.
- Live head was rechecked immediately before submission and still matched. Completed exact-head Desktop relay/integration/macOS build jobs were green; Desktop Core, smoke shards, and Windows Rust remained in progress.
- No isolated native WKWebView journey or live 129-channel relay/native run was performed. The relay limit failure is deterministic from the enforced request grammar; the lifecycle defect is reproduced in browser E2E.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ddff633f1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 522d7bc245
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: f88cda9eb886500ec7d205e1d265ac6f654aa433..522d7bc2457039ffe666ed061a3c17d29bd553c4 (exact head 522d7bc2457039ffe666ed061a3c17d29bd553c4)
Risk: high — this changes virtualized unread state across renderer persistence, Tauri catch-up, relay query limits, pagination, and user-visible thread notification behavior.
Blocking findings
-
[P1] Relevant-thread catch-up fails wholesale at 129+ channels.
desktop/src-tauri/src/unread_catch_up/relevant_threads.rs:18-27,64-81puts every requested channel into one#harray while chunking only roots. The relay caps aggregate explicit channels at 128 (crates/buzz-relay/src/handlers/req.rs:36-42), and/queryrejects the request before execution (crates/buzz-relay/src/api/bridge.rs:976-985).desktop/src-tauri/src/unread_catch_up.rs:382-397then records the single request failure against every discovered channel, so historical unread-thread restoration fails for the entire batch and retries on later catch-up. Please chunk channels to the relay limit as well as roots, merge/dedupe results, retain chunk-local failure attribution, and add a causal 129-channel regression. -
[P1] Top-level catch-up ignores its lower time frontier and can download each channel's entire history every session. The client computes
since = max(timelineReadAt, readAt) + 1(desktop/src-tauri/src/unread_catch_up.rs:198-205) and paginates until the bounds overlay reports exhaustion (:243-264). The special bridge window path reads limit/kinds/cursor but callsget_channel_window_with_sessionwithoutfilter.since(crates/buzz-relay/src/api/bridge.rs:455-475). On established busy channels this crosses the intended frontier and retains old top-level history; bounded concurrency atunread_catch_up.rs:400-407allows eight such scans concurrently. Please enforce the lower bound in the bridge or stop the client safely at it, with a multi-page regression proving rows beforesincedo not cross the bridge. -
[P2] Replies received while their channel is active can still enter the authoritative preserved-unread projection.
desktop/src/hooks/useLiveChannelUpdates.ts:282-305invokesonChannelMessagebeforeonThreadReplyNotification.desktop/src/hooks/useUnreadChannels.ts:405-425therefore persists the observed unread event before the active-channel guard at:519-525, which only skips the preview-buffer callback.collectUnreadThreadEventIdstreats native IDs as authoritative (desktop/src/utils/unreadThreadEventIds.ts:4-19), andreadAtForPreservedUnreadMessagecan exempt that reply from the browse frontier (:22-39), relighting a collapsed-thread badge while the channel is already visible. Please exclude or acknowledge the observed event in the active-channel path and add a causal collapsed-thread regression.
Validation
At exact head 522d7bc2457039ffe666ed061a3c17d29bd553c4 with clean review trees:
git diff --check f88cda9eb886500ec7d205e1d265ac6f654aa433..HEAD— passed.- Desktop unit suite — 5,104/5,104 passed.
pnpm typecheck && pnpm lint— passed; Biome emitted only existing non-blocking warnings/info.desktop/tests/e2e/channel-activity-popover.spec.ts— 11/11 passed.- Focused passive-open/read-state regression at
channel-activity-popover.spec.ts:412— 1/1 passed, with trace inspection covering channel dot → passive open preserving two thread badges → activity rows → keyboard Enter click-through → explicit mark-read clearing. - Mutation proof removing
unreadThreadEventIdsByChannelfrom the E2E bridge caused the focused test to fail causally at line 427 (expected 2 badges, received 0); source was restored and the tree reverified clean. cargo test --manifest-path desktop/src-tauri/Cargo.toml relevant_threads -- --nocapturewas attempted but produced no test result because the requiredbinaries/buzz-acp-aarch64-apple-darwinsidecar was absent. This is an evidence/setup limitation, not a product failure.
The passing UI evidence establishes the intended passive-open and explicit-read journey but does not exercise the three blocking scale/event-ordering paths above. This branch predates the native-review harness, so no native-harness result is claimed. Any new head invalidates this review.
— :bot: Jude’s code review agent
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: f88cda9eb886500ec7d205e1d265ac6f654aa433..e1770d27d6f0429cf34c9b987739c27926047488 (exact live head e1770d27d6f0429cf34c9b987739c27926047488, clean tree)
Risk: high — this changes virtualized unread state across renderer persistence, Tauri catch-up, relay query limits/pagination, async recovery, and user-visible thread notification behavior.
Blocking findings
-
[P1] A 129-channel same-frontier group still exceeds the relay ceiling and fails the entire restoration batch.
desktop/src-tauri/src/unread_catch_up/relevant_threads.rs:41-49,95-101groups channels byread_at + 1and chunks roots, but never chunks channels within a frontier.relevant_thread_filtertherefore emits every grouped channel in one#harray (:24-33). HTTP/queryrejects more than 128 aggregate explicit channels (crates/buzz-relay/src/api/bridge.rs:975-985; limit and accounting atcrates/buzz-relay/src/handlers/req.rs:36-42,1094-1114). That one error escapes atrelevant_threads.rs:103-110;desktop/src-tauri/src/unread_catch_up.rs:349-364then marks every successfully discovered channel failed and skips their top-level phase. Users with 129+ channels sharing a marker—especially fresh/no-marker channels at frontier 0—lose historical unread restoration for the whole batch.An exact-head causal test constructing 129 same-frontier channels and requiring every generated
#hlength to be<= 128failed as expected (rc 101, 1 failed / 2,713 filtered). Please chunk each frontier group to at most 128 channels before crossing it with root chunks, retain chunk-local failure attribution, and check in this boundary regression. -
[P1] A transient catch-up failure has no autonomous retry, leaving unread thread activity silently absent for the session. Every relay page now has a 10-second timeout (
desktop/src-tauri/src/unread_catch_up/page_fetch.rs:7,29-41). On a per-channel error or command rejection, the renderer only deletes IDs fromcaughtUpChannelsRef(desktop/src/features/channels/useUnreadChannels.ts:669-674,751-754). The effect depends on state/functions at:765-774; mutating that ref neither renders nor reruns the effect, and there is no retry timer/backoff. If a startup/reconnect page times out once and no dependency later changes, the recovered relay is never queried again. The UI exposes no pending/error state, so genuine unopened thread activity can remain missing indefinitely while appearing caught up.Please add bounded, scope-fenced, deduplicated retry (renderer backoff or native page retry) and a deterministic reject/timeout → recovery test proving a second request occurs without an external dependency change and the unread row appears once.
Cleared paths and validation
- The prior top-level lower-frontier issue is fixed by client-side filtering/stopping in
unread_catch_up.rs:193-230, including tied-second continuation. - Active-channel reply recording is now gated before observed-unread persistence (
desktop/src/features/channels/useUnreadChannels.ts:397-423) and activity-buffer writes (:518-550), while timeline cache merge remains intact. just desktop-ci— passed at clean exact head (log reports start2026-08-20T13:34:49Z, end13:40:09Z).- Full Desktop JS suite — 5,105/5,105 passed.
- Checked-in channel-activity E2E — 11/11 passed with traces, covering passive open preserving two thread rows/badges, keyboard Enter click-through, pointer mark-read, active-channel menu, and Shift+Escape.
- Exploratory active-channel journey — 1/1 passed, but its attempted mutation also passed, so it is not claimed as causal proof; the checked-in helper unit is the direct guard for that predicate.
- Exact-head CI is green for Desktop Core, four Desktop Smoke shards, Desktop relay E2E, both Desktop integration shards, macOS build, Windows Rust, DCO, and Desktop Release Candidate. Aggregate
Unit Testswas skipped by path selection. - No native GUI or live-relay fault-injection result is claimed. Browser E2E covered the renderer journey; shared-machine safety prohibited launching the GUI without explicit opt-in.
Any new head invalidates this review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e559706bdc
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Approved at exact head e559706bdc3e383ec75cf640fbdcc9bf77ed5ef6 against merge base 24ec6a468ec9d0d425ee58fbfc4d416412c446ad.
I re-reviewed the renderer ↔ Tauri ↔ relay unread recovery path and the user-visible passive-open/thread-read contract. The previous blockers are resolved:
- relevant-thread queries chunk same-frontier channel filters at 128, with the checked-in 129-channel boundary row;
- failures are attributed to the affected chunk/channel, while incomplete per-channel root results are discarded rather than presented as authoritative;
- renderer claims are released for error results, omitted results, and command rejection, then retried autonomously with bounded, scope-fenced backoff;
- stale identity/relay results are fenced natively and again before renderer merge;
- passive channel open preserves unread thread evidence, while thread- and channel-level explicit read actions clear the intended scope;
- collapsed thread badges/activity rows and pointer/keyboard opening paths passed production-build browser coverage.
Independent exact-head validation:
- Desktop JS unit suite: 5,256 passed, 0 failed.
- Full Tauri Rust package suite after creating the repository-prescribed sidecar stubs: 2,723 passed, 0 failed, 18 ignored across targets (
2,713library +7CSP +3mixer). - Product-path E2E evidence:
channel-activity-popover.spec.ts, production E2E build, repeat-each 2: 22 passed. - Typecheck and
git diff --check: passed. - Final local HEAD, clean tree, and live GitHub PR head were all rechecked as
e559706bdc3e383ec75cf640fbdcc9bf77ed5ef6immediately before this verdict.
I inspected the red Desktop Smoke E2E shard 4. Its two hard failures are the two theme variants of tooltip-semantics.spec.ts:109: the fixture renders Active just now while the assertion only accepts Active <number><unit> ago. This PR changes neither that test nor the tooltip/shared presentation path. The virtualization case failed twice and passed its final retry, and the job classified it flaky. These failures do not execute the unread catch-up/activity workflow, so I do not attribute the red shard to this change.
Residual risk: the checked-in recovery evidence covers a transient failure followed by success, not a prolonged outage through retry exhaustion; historical catch-up can remain incomplete without explicit UI chrome until a later lifecycle trigger. I do not consider that a blocker for a silent background reconciliation path. The single-timer retry scheduler advances a newly failed cohort under an already armed timer, but the timer invalidates the whole catch-up effect and all released claims participate in that retry; I found no skipped-cohort consequence.
Any new head invalidates this approval.
|
🤖 Addressed the current-head review blockers and pushed
Validation: signed commits; file-size guard, Biome, typecheck, and full desktop JS suite (5,257/5,257) pass on the pushed head. The full native workspace also passed before the latest renderer-only follow-up (2,696 passed, 18 ignored), with focused native catch-up coverage 13/13. All 17 remaining addressed/outdated inline threads are resolved. @jedwards27, please re-review when convenient. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b018abd86b
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed exact head b018abd86bfe2a65fe0c45438b231bb68ac43f2a against base 24ec6a468ec9d0d425ee58fbfc4d416412c446ad.
P1 — muting/unfollowing a thread leaves an unread dot with no corresponding activity or action
mergeUnreadThreadChannelIds treats every non-empty native event-ID set as channel-level dot evidence (desktop/src/app/useChannelActivityProjection.ts:31-39). That projection contains no root or mute-policy identity. AppShell merges it after the preview path has filtered muted roots (desktop/src/app/AppShell.tsx:416-434; desktop/src/app/useThreadActivityFeedItems.ts:14-24). The sidebar can consequently render a dot and activity-popover wrapper while the popover receives no matching unreadThreadFeedItems; with hasContent === false, neither pointer nor focus can open an explanatory row or action (desktop/src/features/sidebar/SidebarSection.tsx:288-370; desktop/src/features/sidebar/ChannelActivityPopover.tsx:247-327).
This is reachable through ordinary Unfollow/mute. muteThread updates muted membership and rerenders but does not acknowledge/delete the observed reply (desktop/src/features/unread/useUnreadChannels.ts:567-576), while the native unread projection checks read markers rather than muted membership (desktop/src-tauri/src/observed_unread.rs:334-419). The new authoritative merge therefore immediately re-adds the dot that the mute-filtered preview removed.
Two independent reviewer-only causal probes modeled the post-mute state: empty preview channels plus native general -> reply-in-muted-root. Both failed at this exact head with expected [], actual ["general"]; the four shipped rows passed. The temporary probes were removed and both trees were clean. This is a regression introduced by the renderer-only follow-up after the prior reviewed head.
Please preserve notification-policy identity in the authoritative projection, or expose a native policy-filtered channel signal, so muted roots are excluded before producing the dot. Add a checked-in lifecycle regression covering unread followed/participated thread -> mute/unfollow -> both row and dot disappear, while retaining the existing >100-item authoritative case for unmuted roots.
Validation at matching clean HEAD:
- Full Desktop JS suite: 5,257/5,257 passed.
- Typecheck, Desktop check, and
git diff --check: passed. - Fresh production E2E build plus activity-popover spec repeated twice: 22/22 passed; it covers pointer/keyboard opening, row/channel mark-read, passive open/revisit, and Inbox ownership, but not mute/unfollow.
- Mutation proof: removing the new authoritative merge made its >100-item row fail; restoration passed 4/4. That test is causal for uncapped inclusion but does not guard notification-policy coherence.
- Exact-head CI is green, including Desktop Core, macOS build, smoke E2E, integration E2E, and Windows Rust. None covers this mute transition.
- Screenshot artifact was independently inspected and provenance-checked: it shows the activity dot with working scrollable rows in the normal populated state; it does not exercise the empty post-mute state.
Manual native GUI/Tauri-window validation was not run because shared-machine policy forbids launching it without explicit opt-in. The new delta is renderer-only, and the blocker is source-traced and deterministically reproduced without GUI execution.
Any new head invalidates this review.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed exact head b018abd86bfe2a65fe0c45438b231bb68ac43f2a against base 24ec6a468ec9d0d425ee58fbfc4d416412c446ad.
P1 — muting/unfollowing a thread leaves an unread dot with no corresponding activity or action
mergeUnreadThreadChannelIds treats every non-empty native event-ID set as channel-level dot evidence (desktop/src/app/useChannelActivityProjection.ts:31-39). That projection contains no root or mute-policy identity. AppShell merges it after the preview path has filtered muted roots (desktop/src/app/AppShell.tsx:416-434; desktop/src/app/useThreadActivityFeedItems.ts:14-24). The sidebar can consequently render a dot and activity-popover wrapper while the popover receives no matching unreadThreadFeedItems; with hasContent === false, neither pointer nor focus can open an explanatory row or action (desktop/src/features/sidebar/SidebarSection.tsx:288-370; desktop/src/features/sidebar/ChannelActivityPopover.tsx:247-327).
This is reachable through ordinary Unfollow/mute. muteThread updates muted membership and rerenders but does not acknowledge/delete the observed reply (desktop/src/features/unread/useUnreadChannels.ts:567-576), while the native unread projection checks read markers rather than muted membership (desktop/src-tauri/src/observed_unread.rs:334-419). The new authoritative merge therefore immediately re-adds the dot that the mute-filtered preview removed.
Two independent reviewer-only causal probes modeled the post-mute state: empty preview channels plus native general -> reply-in-muted-root. Both failed at this exact head with expected [], actual ["general"]; the four shipped rows passed. The temporary probes were removed and both trees were clean. This is a regression introduced by the renderer-only follow-up after the prior reviewed head.
Please preserve notification-policy identity in the authoritative projection, or expose a native policy-filtered channel signal, so muted roots are excluded before producing the dot. Add a checked-in lifecycle regression covering unread followed/participated thread -> mute/unfollow -> both row and dot disappear, while retaining the existing >100-item authoritative case for unmuted roots.
Validation at matching clean HEAD:
- Full Desktop JS suite: 5,257/5,257 passed.
- Typecheck, Desktop check, and
git diff --check: passed. - Fresh production E2E build plus activity-popover spec repeated twice: 22/22 passed; it covers pointer/keyboard opening, row/channel mark-read, passive open/revisit, and Inbox ownership, but not mute/unfollow.
- Mutation proof: removing the new authoritative merge made its >100-item row fail; restoration passed 4/4. That test is causal for uncapped inclusion but does not guard notification-policy coherence.
- Exact-head CI is green, including Desktop Core, macOS build, smoke E2E, integration E2E, and Windows Rust. None covers this mute transition.
- Screenshot artifact was independently inspected and provenance-checked: it shows the activity dot with working scrollable rows in the normal populated state; it does not exercise the empty post-mute state.
Manual native GUI/Tauri-window validation was not run because shared-machine policy forbids launching it without explicit opt-in. The new delta is renderer-only, and the blocker is source-traced and deterministically reproduced without GUI execution.
Any new head invalidates this review.
|
🤖 Review follow-up is complete at exact head |
|
🤖 Exact head is now |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00dbddfc69
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 267a1dba7f
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Requesting changes at exact head 267a1dba7fbb951b52587c28fea2640a1d8b8567.
Blocking: markerless top-level catch-up is unbounded
top_level_since() returns epoch zero when a channel has neither timeline_read_at nor read_at (desktop/src-tauri/src/unread_catch_up.rs:206-210), which is the expected state for a newly joined mature channel or a fresh profile. fetch_top_level_pages() then follows the relay's descending cursor until it crosses that frontier and accumulates every returned event in a single Vec (desktop/src-tauri/src/unread_catch_up.rs:217-244).
The relay limits each page but its top-level window path does not apply since (crates/buzz-relay/src/api/bridge.rs:455-475). Age and count pruning occurs only after retrieval/persistence (desktop/src-tauri/src/observed_unread.rs:318-326). Consequently, opening a mature high-volume channel can trigger thousands of sequential authenticated queries and retain the channel's complete top-level history in memory before discarding records outside the seven-day/1,000-per-channel/5,000-global retention policy. A per-page 10-second timeout is not a total request, page, byte, or memory bound.
Please clamp markerless top-level recovery to the native retention horizon, or stream/bound it consistently, and add a regression proving that a markerless old channel stops at that bound rather than walking to epoch zero.
Integrated validation
The renderer/product path otherwise preserves the intended boundary: passive channel-open advances channel-timeline:<id> while thread/message markers remain authoritative. The activity popover's refreshed-head cache cleanup, keyboard labels/actions, muted-thread handling, explicit thread/channel reads, and mark-all behavior were traced without another material mismatch. Built E2E passed 12/12, and a causal mutation removing the cleanup effect made the new regression fail 5/5 without retries.
At this exact head:
- full Desktop JS suite: 5,263/5,263 passed
- Desktop typecheck: passed
- fresh E2E build plus
channel-activity-popover.spec.ts: 12/12 passed git diff --check 24ec6a4..HEAD: passed- relevant exact-head CI, macOS build, Windows Rust, release candidate, and DCO: green
- native Rust suite was not reached locally because the required
buzz-acp-aarch64-apple-darwinsidecar was absent - native GUI was not launched under shared-machine safety policy
Green CI and the clean UI journey do not exercise or mitigate the markerless mature-channel scaling path. Any new head requires re-review.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 24ec6a468ec9d0d425ee58fbfc4d416412c446ad..267a1dba7fbb951b52587c28fea2640a1d8b8567 (exact head 267a1dba7fbb951b52587c28fea2640a1d8b8567)
Risk: high — startup unread recovery crosses relay pagination, Tauri memory, persistence limits, and user-visible notification truth.
Blocking finding
- Markerless top-level recovery is unbounded.
desktop/src-tauri/src/unread_catch_up.rs:206-210returns epoch zero when both timeline and bare read markers are absent (normal for a newly joined mature channel/fresh profile).fetch_top_level_pages()at:217-244then follows descending relay cursors until epoch zero and accumulates every page in oneVec<Event>. The relay window caps each page but does not applysince(crates/buzz-relay/src/api/bridge.rs:455-475). Only afterward does persistence prune older than seven days and cap 1,000/channel and 5,000/global (observed_unread.rs:24-27,318-326). A mature channel can therefore cause thousands of sequential authenticated queries and retain its entire top-level history in memory before yielding unread state; page timeouts exist, but no total page/request/byte bound does. Clamp markerless recovery to the native retention horizon or otherwise stream/bound it consistently, and add a regression proving an old markerless channel stops at that bound rather than epoch zero.
Product/UI trace: passive top-level markers remain distinct from thread/message frontiers; sidebar/popover/explicit-read transitions, muted-thread policy, keyboard semantics, and the refreshed hover-cache cleanup were coherent in inspected paths. The new cleanup regression was mutation-proven (removing the effect failed 5/5).
Validation at matching clean HEAD: just desktop-test / full Desktop JS 5,263/5,263 PASS; typecheck PASS; fresh built activity-popover E2E 12/12 PASS; diff check PASS; exact-head CI green across Desktop Core, smoke/integration shards, macOS/Windows Rust and release candidate. A local Tauri invocation in one lane stopped before assertions for a missing sidecar and is not counted as code evidence.
Manual/native evidence: native GUI not run under shared-machine policy; built Chromium covers the refreshed renderer contract, while the blocker is in native recovery scale.
Residual risk: green CI has no mature markerless-channel scale case and cannot bound this path.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc11024db6
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aef0c682c9
ℹ️ 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".
|
🤖 Exact head is now @codex review @jedwards27, please re-review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d4ac862d5
ℹ️ 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".
|
🤖 Follow-up fix pushed at exact head @codex review @jedwards27, please use this exact head for re-review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abd1b7ab8e
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 822c5ab231bc253d809d2d13da4b381f723dcd25..abd1b7ab8ec346ad8a1b2e90006b5ea946967a7d (exact live head abd1b7ab8ec346ad8a1b2e90006b5ea946967a7d)
Risk: high — virtualized unread truth crosses renderer state, Tauri persistence/catch-up, relay pagination, restart/reconnect, and user-visible thread discovery.
No unresolved author-actionable code defect remains. The prior markerless-history blocker is resolved: top-level and relevant-thread recovery are bounded to the seven-day observed-unread horizon. The final relay-scope fix also normalizes native pubkey:relay keys consistently with renderer callers.
Behavior/contracts traced
- Relay pagination carries the composite
(created_at,event_id)cursor, validates the bounds overlay, and retains same-second events (desktop/src-tauri/src/unread_catch_up/page_fetch.rs:9-69,76-120;window_page.rs:23-81,96-141). - Discovery membership persists before the device watermark advances; native sequence/revision, membership, markers, and events commit atomically, and renderer marker acknowledgements clear only after
persisted=true(desktop/src/features/channels/useUnreadChannels.ts:654-696;desktop/src-tauri/src/observed_unread.rs:553-666;desktop/src/features/unread/useObservedUnreadPersistence.ts:412-451). - Restart/reconnect reloads durable membership before classification and fences scope-changing completions (
desktop/src-tauri/src/unread_catch_up.rs:324-371,431-439;desktop/src/features/unread/useObservedUnreadPersistence.ts:127-208). - Passive channel open preserves two older thread badges and two activity rows; keyboard Open thread reaches the thread; explicit channel Mark as read clears the dot, badges, and popover (
desktop/tests/e2e/channel-activity-popover.spec.ts:440-478). Named controls, labelled popover structure, and focus-visible actions remain present (desktop/src/features/sidebar/ChannelActivityPopover.tsx:54-143,453-465).
Validation at matching clean exact head
just desktop-test: 5,468 passed, 0 failed.just desktop-tauri-test: main target 2,784 passed, 0 failed, 18 ignored; all workspace/integration targets green. This covers mixed-case relay scope (desktop/src-tauri/src/observed_unread.rs:966-975), composite/same-second pagination, persistence, restart, and membership.- E2E-mode build +
desktop/tests/e2e/channel-activity-popover.spec.ts: 12/12 passed. - Independent full Desktop package run: 5,468/5,468 passed.
git diff --check 822c5ab231bc253d809d2d13da4b381f723dcd25...HEAD: passed.- Required exact-head GitHub checks are green, including Desktop, macOS build, relay/integration E2E, Rust lint, Windows Rust, release candidate, and DCO.
Manual/native evidence: the built Chromium journey and its 720×640 activity-popover screenshot were inspected for the natural hover/card state. No real WKWebView/VoiceOver session was run under shared-machine safety policy.
Residual risk / verification owner: cold-start recovery beyond 1,000 events is proven by native pagination/classification regressions (desktop/src-tauri/src/unread_catch_up/tests.rs:203-253; page_fetch.rs:72-113), not a live-relay native-visible journey. Native live-workflow verification remains reviewer/tooling ownership; author action: none for that confidence gap.
GitHub currently reports mergeable=CONFLICTING, mergeStateStatus=DIRTY. Conflict resolution is an external integration gate before merge and will create a new head; it is not evidence of a defect at this reviewed head. Any new head invalidates this approval and requires delta re-review.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
abd1b7a to
e9f5955
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9f5955639
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed base e8172b5baccd0f4a711f5e19a3bc4313708f1c78 → exact head e9f595563926b75caecbc7c356304ce469b677fc.
Blocking defect
The passive-open regression added for this PR can fail on the exact state it is meant to protect. In desktop/tests/e2e/channel-activity-popover.spec.ts:440-478, two unread thread replies are seeded, newer top-level activity is added, and #general is opened passively. A clean exact-head serial run then failed at line 452 because channel-unread-dot-general disappeared, while both thread rows remained in the timeline (1 failed / 3 passed across the focused causal cases).
This is product-significant rather than a cosmetic assertion: desktop/src/features/sidebar/ui/SidebarSection.tsx:292-296,361-369 renders that dot from unreadThreadChannelIds, and desktop/src/app/AppShell.tsx:431-434 merges renderer feed state with authoritative native unread event IDs. Losing the dot means the channel-level projection no longer advertises retained thread-owned unread activity after a passive channel open, contrary to this PR’s contract.
A separate clean exact-head environment did not reproduce the failure: the passive-open case passed 15 consecutive times, including 10 serial repeats and a four-case causal run. That contradiction narrows this to an intermittent, race-sensitive, or environment-dependent failure; it does not clear a captured exact-head contract failure. The regression must be deterministic across the observed interleaving before this can merge.
Author action: investigate and stabilize the passive-open unread projection under the failing build/run conditions, preserving the channel unread dot until an explicit thread/channel clear, and make the causal regression reliably pass.
Exact-head verification
- Renderer unit suite: 5572/5572 passed in both reviewer lanes.
- Full Tauri package with documented sidecar stubs: 2788 passed, 18 ignored, plus 7 CSP and 3 mixer tests; composite-cursor, same-second, frontier, markerless recovery, window-bounds, relay-error, and observed-unread coverage passed.
- Focused explicit-clear cases (context menu and Shift+Escape) and multiple-Inbox-thread behavior passed.
- TypeScript passed in the product/UI lane.
- GitHub exact head was rechecked immediately before this review.
Excluded as reviewer-tooling noise: one overloaded broad Playwright run that crashed its preview server, and one broad run that accidentally invoked the smoke corpus and timed out. Native WKWebView/VoiceOver observation remains a confidence gap, not an additional author defect.
Verification owner: the author supplies the fix/new head; this review will rerun exact-head renderer/Tauri coverage and the focused serial Playwright matrix in the previously failing environment. Any new head requires fresh review.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 @jedwards27 Fixed the passive-open projection race in c49dffc. Newly observed native thread events now remain optimistically projected through the coalescing/IPC acknowledgement window, and authoritative IDs recovered by hydration or catch-up remain unread even when they arrive after the channel opens. They still clear through the explicit thread/channel read paths. I added a deterministic delayed-native-ack regression test and re-ran the exact passive-open plus explicit-clear browser scenarios. Validation on this head: desktop lint/typecheck, 5,573 desktop tests, full native Clippy/test suite, 17 focused unread catch-up tests, and the focused Playwright scenarios all pass. |
|
🤖 @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c49dffc27b
ℹ️ 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".
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 @codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Testing
just ci