Preserve thread notifications when opening channels - #6153
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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".
| // makes the relay-side filter strict-newer; the client-side | ||
| // `> readAt` check below is the belt to the suspenders. | ||
| const sinceParam = readAt === null ? 0 : readAt + 1; | ||
| const sinceParam = channelReadAt === null ? 0 : channelReadAt + 1; |
There was a problem hiding this comment.
On a fresh second device, a channel that has only been opened passively can have a recent timeline marker but a very old or null bare marker. This query then scans from that stale marker but retains only the newest CATCH_UP_LIMIT events; because the relay returns created_at DESC and the channel is marked caught up after this single request, any still-unread thread reply older than the newest 1,000 matching events is silently missed. The split frontiers make this range grow indefinitely, so this path needs pagination or a query that cannot truncate qualifying thread activity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real — flagging to the author for a call rather than reshaping the core sync path unilaterally.
This is a genuine latent edge: on a fresh second device, a passively-opened channel can carry a recent channel-timeline: marker but a stale/null bare frontier. The catch-up scan starts from that stale bare marker and keeps only the newest CATCH_UP_LIMIT events, so old unread thread replies beyond the window can be silently dropped. The split-marker design widens the gap between the two frontiers, which makes the window more likely to matter.
The fix is pagination (cursor until/limit to exhaustion, modeled on assignmentOperationFetch.ts), but that forces an unbounded-fetch-vs-truncation product/performance tradeoff on the core sync path. Deferring to the PR author on whether to fix here or track separately; leaving this thread open pending that call.
jedwards27
left a comment
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.
Passive channel-open advances only channel-timeline:<id>, but the per-message parent resolver folds only the bare channel context. A reply the user browsed past (older than the frontier, never expanded, so no msg:<id> marker) therefore re-lit the collapsed in-panel branch badge — over-counting by one and breaking the pre-existing thread-unread 05/06 rows. Route all three active-channel badge/predicate paths (threadReplyUnreadCounts, threadUnreadCounts, isMessageUnread) through one getActiveMessageReadAt resolver that folds getChannelReadAt(active)=max(bare, channel-timeline) over each message's own marker. Browsed-past replies read again; genuinely newer replies stay lit; roots and branches share one frontier so they cannot disagree. The sidebar thread-notification preservation lives in useUnreadChannels and is untouched. Signed-off-by: kenny lopez <klopez4212@gmail.com> Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz> Co-authored-by: kenny lopez <klopez4212@gmail.com>
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".
| maxReadAt( | ||
| getMessageReadAt(messageId), | ||
| activeChannelId ? getChannelReadAt(activeChannelId) : null, | ||
| ), |
There was a problem hiding this comment.
When an unread thread reply is older than a newer top-level message, opening the channel advances the timeline frontier to that top-level message and this fold applies it to every reply's per-message read resolver. computeThreadBadgeCounts and isMessageUnread consequently treat the unopened reply as read and remove its thread-summary badge, even though the sidebar intentionally keeps the channel unread; the user is left with an unread channel indicator but no in-channel indication of which thread needs attention. Thread reply predicates should continue using the bare channel/thread/message frontiers rather than the passive top-level timeline frontier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This fold is deliberate, and reverting it to the bare frontier re-breaks a blocking human review — so I'm leaving the thread open for the author to arbitrate rather than acting on it.
The tension is real but the two cases are indistinguishable with the markers we have. Both (a) a reply the user already browsed past in the timeline and (b) a genuinely-unread reply older than a newer top-level message are older than the channel-timeline: frontier and neither has a msg:<id> marker. Pre-existing e2e thread-unread.spec.ts 05 and 06 REQUIRE case (a) to read as read; using the bare frontier instead of the timeline frontier reverts that and re-breaks 05/06, which is exactly the CHANGES_REQUESTED regression jedwards27 blocked on.
For context, folding max(bare, channel-timeline) here reconstructs main's in-channel behavior exactly: on main the parent resolver mapped msg:<id> -> bare channel and passive channel-open advanced the bare marker, so a browsed-past reply read as read there too. This PR only relocated that passive advance from the bare context to channel-timeline:; folding it back into the active-channel predicates restores the prior behavior.
Separating (a) from (b) requires a new "reply actually seen" signal that doesn't exist today — a larger change to the read-state core. Deferring to the PR author on whether that's in scope; leaving this open pending that decision.
|
@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
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.
Pull in h2 0.4.16 (RUSTSEC-2026-0258 fix, #6222) to clear the Security advisory. No feature-code changes. Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
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.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
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".
| "since": channel.read_at.map_or(0, |value| value.saturating_add(1)), | ||
| "limit": CATCH_UP_LIMIT, |
There was a problem hiding this comment.
When the bare channel marker is stale but timelineReadAt is recent—the normal long-term state for channels opened passively—this since value causes the loop to download and retain every channel event since the bare marker, even though nearly all top-level events are later discarded against the timeline marker. Because there is no page, byte, or total-time bound and catch-up repeats each app session, a busy channel can require hundreds of sequential REQs and hold its complete history in memory before any result is classified, delaying unread restoration or exhausting the desktop process; the query needs to avoid scanning irrelevant top-level history rather than accumulating an indefinitely growing range.
Useful? React with 👍 / 👎.
| let page_len = page.len(); | ||
| let oldest = page.iter().map(|event| event.created_at.as_secs()).min(); | ||
| events.extend(page); | ||
| let Some(next) = next_page_until(page_len, oldest, until) else { |
There was a problem hiding this comment.
When at least 1,000 matching events share the oldest second in a page, subtracting one second from oldest skips every event from that second that did not fit in the first page. The relay orders these rows by created_at DESC, id ASC, so a qualifying thread reply later in that same-second ordering is silently omitted and the channel is still considered caught up; pagination needs a stable timestamp-plus-event-ID cursor instead of dropping the entire boundary second.
Useful? React with 👍 / 👎.
| const preservedUnreadMessageIds = React.useMemo( | ||
| () => | ||
| new Set( | ||
| unreadThreadFeedItems | ||
| .filter((item) => item.channelId === activeChannelId) | ||
| .map((item) => item.id), |
There was a problem hiding this comment.
When more than 100 unread thread activities exist across channels, the global activity buffer drops the oldest IDs, so this set no longer contains all replies that the observed-unread store still considers unread. Opening a channel containing one of those omitted replies then folds the newer timeline frontier over it and clears its in-channel badge while the sidebar remains unread. Fresh evidence in this revision is that the preservation logic now depends on unreadThreadFeedItems, while both native catch-up and threadActivityStorage.ts cap that source at 100; preservation should derive from the authoritative observed unread rows rather than the capped preview feed.
Useful? React with 👍 / 👎.
jedwards27
left a comment
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.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
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".
| }), | ||
| cursor.as_ref(), | ||
| )?; | ||
| events.extend(page); |
There was a problem hiding this comment.
When the timeline marker predates the catch-up request, every top_level response includes a relay-signed kind 39006 window-bounds overlay (crates/buzz-relay/src/api/bridge.rs:563-582), but this code appends that overlay to the message batch. classify_batch treats it as an external top-level event with a current timestamp and records it as observed unread, so even a channel with no new messages acquires a phantom unread dot after startup. Separate the bounds event from actual rows before extending events.
Useful? React with 👍 / 👎.
| let next = next_page_cursor( | ||
| page.len(), |
There was a problem hiding this comment.
For channels with more than 200 top-level events beyond the timeline frontier, this page-length test always stops after the first window: the relay caps top_level rows at 200 and appends one bounds overlay (crates/buzz-relay/src/api/bridge.rs:455-475,563-582), so page.len() is at most 201 rather than CATCH_UP_LIMIT (1,000). Older qualifying rows—such as a mention or broadcast behind 200 newer ordinary messages—are never classified, and their notification/app badge is silently lost; parse has_more and next_cursor from the 39006 overlay instead.
Useful? React with 👍 / 👎.
| (order, channel, result) | ||
| }); | ||
| } | ||
| let api_base = crate::relay::relay_http_base_url(&relay_url); |
There was a problem hiding this comment.
When Desktop is configured with a separate BUZZ_RELAY_HTTP/BUZZ_DESKTOP_BUILD_RELAY_HTTP endpoint and no workspace override, catch-up now derives HTTP from the WebSocket URL instead of using the established relay_api_base_url_with_override precedence. Other HTTP-backed commands continue reaching the configured bridge, while unread catch-up sends /query to the WebSocket host and repeatedly fails, preventing historical unread restoration in that supported deployment; resolve the API base through the same helper used elsewhere.
Useful? React with 👍 / 👎.
jedwards27
left a comment
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.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
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".
| for (channel, mut events) in discovery { | ||
| match fetch_relevant_events(&state, &api_base, &keys, &channel, &roots).await { |
There was a problem hiding this comment.
For users with many joined channels and a long participation history, roots is the global membership set, but this sequential loop passes the complete set to every channel even though almost all roots belong to other channels. With 100 channels and 1,000 retained roots, startup performs at least 500 sequential thread-filter HTTP requests in addition to discovery and top-level requests, repeatedly triggering relay admission/rate limits and delaying or preventing unread restoration. Fresh evidence relative to the earlier bounded-catch-up finding is this new cross-product of global roots and channels; partition roots by channel or use a bounded server-side query instead.
AGENTS.md reference: AGENTS.md:L20-L24
Useful? React with 👍 / 👎.
| preservedUnreadMessageIds.has(messageId) && | ||
| createdAt !== undefined && | ||
| (openFrontierSeconds === null || createdAt > openFrontierSeconds) |
There was a problem hiding this comment.
When an unread reply predates an already-persisted channel-timeline marker, leaving and reopening the channel makes openFrontierSeconds equal that newer timeline marker. This new createdAt > openFrontierSeconds guard then rejects the reply even though its ID is present in the authoritative unread projection, falls through to the timeline-frontier fold, and removes its badge while the sidebar still reports the channel unread. Fresh evidence relative to the earlier badge finding is this newly added open-frontier condition; authoritative preserved IDs must remain exempt across revisits and restarts, not only during the first passive open.
Useful? React with 👍 / 👎.
jedwards27
left a comment
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.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
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".
| serde_json::json!({ | ||
| "kinds": catch_up_kinds(channel_type), | ||
| "#e": roots, | ||
| "since": since, | ||
| "limit": super::CATCH_UP_LIMIT, |
There was a problem hiding this comment.
When catch-up runs for only a newly joined channel, this filter combines the global retained root set with no #h constraint; the bridge consequently searches every accessible channel (apply_channel_scope_to_query treats filters without #h as full accessible scope), potentially downloading the user's entire relevant-thread history from existing channels before bucket_requested_events discards it. Include the requested channel IDs as an #h filter so incremental catch-up remains bounded to its input channels.
AGENTS.md reference: AGENTS.md:L167-L169
Useful? React with 👍 / 👎.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
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".
| for channel in request.channels.iter().cloned() { | ||
| match fetch_discovery_events(&state, &api_base, &keys, &channel, &owner).await { |
There was a problem hiding this comment.
When a user belongs to many channels, this awaited loop serializes every discovery request, and the second loop at lines 391–392 then serializes every top-level window request as well. For 100 channels, catch-up therefore incurs at least 200 sequential network round trips before returning any unread state, even when every channel needs only one page; on a remote relay this can delay unread restoration by tens of seconds. Use bounded concurrency for these independent per-channel fetches while retaining the per-channel error results.
Useful? React with 👍 / 👎.
jedwards27
left a comment
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.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
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".
| let mut filter = base_filter(channel, since); | ||
| filter["top_level"] = serde_json::json!(true); |
There was a problem hiding this comment.
For every established channel, this constructs a top_level filter with since, but the special bridge handler in crates/buzz-relay/src/api/bridge.rs:465-473 calls get_channel_window_with_session without passing or applying that field. The loop therefore paginates the channel's entire top-level history and retains it all in memory before classification, repeating on each app session even when timelineReadAt is recent. Fresh evidence relative to the earlier bounded-catch-up finding is that the replacement window path itself discards the intended lower bound; either the window API must honor since or the client must stop at that frontier.
Useful? React with 👍 / 👎.
| // in context; it must not later become the positive evidence that | ||
| // overrides the passive timeline frontier and relights a badge. Inactive | ||
| // channel replies are the ones this activity buffer exists to preserve. | ||
| if (channelId === activeChannelId) return; |
There was a problem hiding this comment.
When a reply arrives in the currently visible channel, useLiveChannelUpdates invokes onChannelMessage before this callback, so handleChannelMessage has already inserted the reply into the native observed-unread store. Returning here only skips the capped activity preview; the subsequent native projection still exposes the reply through unreadThreadEventIdsByChannel, and readAtForPreservedUnreadMessage preserves it because it is newer than the open frontier. Thus a collapsed thread can gain an unread badge while the user is actively viewing the channel, contrary to this guard's stated behavior; the observed event must also be excluded or acknowledged for this active-channel case.
Useful? React with 👍 / 👎.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
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".
| let since = channels | ||
| .iter() | ||
| .map(|channel| channel.read_at.map_or(0, |value| value.saturating_add(1))) | ||
| .min() | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
When any requested channel has a null or old bare marker, this minimum applies that channel's frontier to every #h value in the combined filter. Catch-up then paginates and retains old relevant-thread events from otherwise recently read channels before per-channel classification discards them; a newly joined channel can therefore make startup scan the relevant history of the entire workspace. Partition channels by their read_at frontier, or otherwise apply each channel's lower bound server-side.
Useful? React with 👍 / 👎.
| let page = | ||
| crate::relay::query_relay_at_with_keys(state, api_base, &[filter], keys, None).await?; |
There was a problem hiding this comment.
When a relay or proxy accepts the connection but stalls while returning /query, this await has no request timeout: the shared AppState HTTP client configures pooling but no timeout, while the replaced WebSocket path explicitly bounded each request to 10 seconds. A single stalled discovery page consequently prevents the whole catch-up command from returning and leaves the renderer's channels claimed for the rest of the mounted session; wrap each page request in a finite timeout.
Useful? React with 👍 / 👎.
jedwards27
left a comment
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
Signed-off-by: kenny lopez <klopez4212@gmail.com>
jedwards27
left a comment
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.
2 participants
Summary
Testing
just ci