Show status and huddle indicators beside names - #7112
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🔐 Codex Security Review
|
![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: 3f7dca759a
ℹ️ 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".
| if (event.kind === KIND_HUDDLE_PARTICIPANT_JOINED) { | ||
| session.participants.add(participant); | ||
| sessions.set(sessionId, session); |
There was a problem hiding this comment.
A member with channel write access can publish kind 48101/48102 events with an arbitrary p tag because validate_huddle_lifecycle_event only validates start/end events. Accepting every matching join here therefore lets that member mark any pubkey as being in a fabricated huddle, and the community-wide indicator displays the spoof beside the victim's name. Filter participant events to the relay signer and anchor sessions to a canonical start, as the existing authenticatedLifecycleEvents path does.
Useful? React with 👍 / 👎.
| } else if (event.kind === KIND_HUDDLE_PARTICIPANT_LEFT) { | ||
| session.participants.delete(participant); | ||
| sessions.set(sessionId, session); |
There was a problem hiding this comment.
When the same pubkey joins through two concurrent admissions, such as desktop and mobile, the Set collapses both joins into one entry and the first device's 48102 event deletes the user even though the other admission remains connected. Relay lifecycle events include distinct admission_id values for this case, and the existing participant-roster reconstruction retains the pubkey until all admission IDs have left; this community-wide reconstruction needs the same accounting.
Useful? React with 👍 / 👎.
| left.created_at - right.created_at || | ||
| lifecyclePhase(left.kind) - lifecyclePhase(right.kind) || | ||
| left.kind - right.kind || | ||
| left.id.localeCompare(right.id), |
There was a problem hiding this comment.
If a participant disconnects and rejoins within the same Nostr timestamp second, the old leave and new join can share created_at; this comparator always places kind 48101 before kind 48102, so reconstruction applies the leave last and incorrectly hides a currently connected user. The relay supplies roster_revision and admission_id specifically to order these lifecycle mutations, and those values need to be considered before the kind/id fallback.
Useful? React with 👍 / 👎.
| KIND_HUDDLE_PARTICIPANT_LEFT, | ||
| KIND_HUDDLE_ENDED, | ||
| ], | ||
| limit: 1000, |
There was a problem hiding this comment.
In a busy community where more than 1,000 lifecycle records occur after a still-running but quiet huddle began, the relay replay omits that huddle's start and joins, so a fresh app mount or reconnect shows none of its participants until another event occurs. Because this limit is shared across every channel rather than per active session, increasing it only postpones the regression; bound the query by the active-huddle lifetime or discover active sessions and fetch their lifecycle independently.
Useful? React with 👍 / 👎.
| const statusQuery = useUserStatusQuery( | ||
| normalizedPubkey ? [normalizedPubkey] : [], | ||
| ); |
There was a problem hiding this comment.
Every rendered message author and one-to-one DM row now mounts an independent status query keyed to a single pubkey. Opening a channel containing many distinct authors consequently opens one relay history subscription per author and repeats those separate requests on the two-minute polling cadence, even though useUserStatusQuery already accepts a pubkey array. Collect the displayed pubkeys at the list/shell level and share one batched lookup to avoid an N+1 relay workload during channel navigation.
Useful? React with 👍 / 👎.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 Updated desktop status and huddle snapshots, including the refined status editor. Profile status controlThe active status emoji replaces the generic set-status smiley.
DM status and huddle indicatorsThe DM header, message rows, and direct-message sidebar show status and huddle emoji.
Status tooltipHovering the status emoji shows the status without a duplicate browser tooltip.
Duration presetsThe desktop modal offers stable preset durations that reopen with their original labels.
Custom dateCustom expiration uses the ShadCN calendar picker.
Custom timeThe half-hour time menu keeps ten choices visible and scrolls for the rest.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1db4e8159c
ℹ️ 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".
| const isExpired = | ||
| parsed.expiresAt !== undefined && | ||
| parsed.expiresAt <= Math.floor(Date.now() / 1_000); |
There was a problem hiding this comment.
When a status expires after this query or the live subscription has cached it, isExpired is never evaluated again until another fetch occurs because expiration itself emits no event or cache update. This leaves expired indicators visible indefinitely while the app is unfocused, and for up to the two-minute polling interval after returning to it; schedule invalidation at expiresAt or derive the cached value against the current time.
Useful? React with 👍 / 👎.
| events.set(event.id, event); | ||
| setParticipantPubkeys(reconstructHuddlePresence(events.values())); |
There was a problem hiding this comment.
In a long-running desktop session in a huddle-active community, every live lifecycle event is retained in events forever, and each arrival copies and sorts that entire history inside reconstructHuddlePresence. The initial limit: 1000 does not bound events received afterward, so memory grows without limit and cumulative reconstruction work becomes quadratic; discard histories for ended sessions or maintain the active-session state incrementally.
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 3ed623bb217bf9697b0ce4562529254977e0ea04..1db4e8159cb24f9ed829e85dcf83e3c3c58c9353 (exact head 1db4e8159cb24f9ed829e85dcf83e3c3c58c9353)
Risk: high — this adds community-wide relay lifecycle reconstruction and user-visible availability state.
Blocking findings
-
Huddle presence trusts forgeable lifecycle events and loses overlapping admissions (
desktop/src/features/huddle/lib/huddlePresence.ts:26-29,38-78;desktop/src/features/huddle/HuddlePresenceContext.tsx:34-49). The new global reducer accepts JOINED/LEFT events without authenticating their signer and stores only one bit per participant. Relay ingest intentionally skips START/END validation for JOINED/LEFT (crates/buzz-relay/src/handlers/ingest.rs:81-89), while the established roster requires the relay signer and understandsadmission_id/revision semantics (desktop/src/features/huddle/hooks/useHuddleParticipantRoster.ts:28-54,74-114,123-164). Consequently, a channel writer can publishp=<victim>and falsely add/remove that person's 🎧 indicator; independently, two active admissions followed by one LEFT incorrectly clear the participant. A temporary causal test reproduced the latter (false !== true) and was removed without changing HEAD.Author action: authenticate JOINED/LEFT against the relay self pubkey, retain active admission IDs per
(session, participant), and remove presence only after the final admission leaves—preferably by extracting/reusing the established roster semantics. Add regressions for a forged non-relay signer and two admissions followed by one leave.Verification owner: author for focused regressions; reviewer for source review and exact-head package rerun; product reviewer for visible join/leave transitions.
-
Expiring statuses remain visibly active after their deadline (
desktop/src/features/user-status/hooks.ts:48-63,83-105,117-141;desktop/src/features/user-status/ui/UserNameIndicators.tsx:21-31,56-76). Expiry is evaluated only when an event is fetched or received. Once a future-expiring value enters the query cache, no deadline timer invalidates it; focused polling is only a 120-second backstop. Statuses such as “In a meeting” can therefore remain beside names and in self UI for up to two minutes beyond the user-selected boundary. Mobile already schedules the nearest expiry and clears due entries (mobile/lib/features/profile/user_status_cache_provider.dart:192-239).Author action: add one shared/cache-level nearest-expiry scheduler that clears or invalidates due statuses, reschedules after query/subscription/mutation updates, and cleans up on session/community teardown. Add a deterministic fake-clock test proving status disappears at the deadline without another relay event or poll.
Verification owner: author for implementation/regression; reviewer for timer/cache lifecycle; product reviewer for deadline UI behavior.
Non-blocking confidence gaps
- Tooltip triggers are non-focusable
<span role="img">nodes (UserNameIndicators.tsx:41-73), so sighted keyboard-only users cannot reveal tooltip text, although screen readers receivearia-label. Recommended author action: use a focusable non-activating trigger or document/test why the persistent emoji plus accessible label meets the intended contract. Verification owner: product/accessibility reviewer. - Initial huddle
subscribeLivefailure is logged without retry, and the globallimit: 1000window may include JOINED history without its canonical STARTED event. These were not promoted without focused reproduction. Verification owner: reviewer after the blocking reconstruction is repaired. - Native Desktop, dark theme, zoom, reduced motion, and real native AX were not independently run. This is a reviewer/tooling confidence gap, not additional author rework.
Validation at matching exact HEAD
- PASS: clean worktree and
git diff --check. - PASS:
pnpm check;pnpm typecheck;pnpm test(5,802 passed, 0 failed);pnpm build(only standard warnings). - PASS: E2E-mode build and focused Playwright status spec (3/3).
- PASS: six distinct posted screenshots inspected; they establish initial/hover visuals, not expiry, leave/reconnect, keyboard tooltip access, or native AX.
- CI was still running for Desktop Core, relay E2E, smoke shards, Windows Rust, and container builds when this review was submitted; completed unit, web, security, Rust lint, macOS Desktop build, and release-candidate checks were green. The source defects above are independently blocking.
Smallest safe merge path: repair authenticated, admission-aware huddle reconstruction and deadline-driven status expiry; add causal regressions; then rerun the affected package and transition evidence.
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57f52b3b8f
ℹ️ 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".
| const nowSeconds = Math.floor(Date.now() / 1_000); | ||
| expireUserStatusQueries(queryClient, nowSeconds); |
There was a problem hiding this comment.
Once the self-status query resolves, scheduleExpiration() calls expireUserStatusQueries(), whose setQueriesData operation emits a user-status query-cache update even when its updater returns the existing value. The cache listener responds by queueing another scheduleExpiration() microtask, which performs the same write and queues itself again; because the app always mounts a self-status query, this endless microtask chain can starve rendering and timers after startup. Only reschedule when the expiration set actually changes, or ignore updates caused by the expiration sweep.
Useful? React with 👍 / 👎.
| relayClient.fetchEvents(filter), | ||
| ); | ||
| if (disposed) return; | ||
| for (const event of history) tracker.apply(event); |
There was a problem hiding this comment.
Relay history is returned newest-first (query_events orders by created_at DESC in crates/buzz-db/src/store/event.rs), but this loop feeds it directly into a tracker that requires the start event to exist before accepting joins, leaves, or ends. On every fresh hydration, newer lifecycle events are therefore ignored before the older start is processed: active huddles show only their creator, while recently ended huddles can be resurrected as active. Apply the same chronological lifecycle ordering used by reconstructHuddlePresence before mutating the tracker.
Useful? React with 👍 / 👎.
| KIND_HUDDLE_PARTICIPANT_LEFT, | ||
| KIND_HUDDLE_ENDED, | ||
| ], | ||
| limit: 0, |
There was a problem hiding this comment.
If the socket disconnects while someone joins, leaves, or ends a huddle, this subscription is restored with limit: 0. The generic reconnect replay preserves that zero limit for non-channel-scoped subscriptions, so none of the events missed during the outage are delivered, and this provider never reruns fetchActiveHuddleLifecycle. The indicator can consequently remain stale for the rest of the session unless another lifecycle event happens to correct it; rehydrate the bounded lifecycle window on reconnect or use a replay-capable subscription.
Useful? React with 👍 / 👎.
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e60dfaec7b
ℹ️ 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".
| type FetchEvents = (filter: RelaySubscriptionFilter) => Promise<RelayEvent[]>; | ||
|
|
||
| export const HUDDLE_LIFECYCLE_PAGE_LIMIT = 500; | ||
| export const HUDDLE_ACTIVE_LOOKBACK_SECONDS = 3_900; |
There was a problem hiding this comment.
When participants remain connected for more than 3,900 seconds without another lifecycle event, a fresh mount omits the start and joins and shows no huddle indicators. Fresh evidence in this revision is the fixed lookback replacing the prior global cap: the reaper in crates/buzz-relay/src/main.rs archives the backing channel but does not terminate its audio room, and BUZZ_EPHEMERAL_TTL_OVERRIDE can also exceed this horizon, so the backing-channel TTL is not a bound on active audio lifetime. Discover active sessions or retain their canonical starts rather than truncating them at a fixed 65 minutes.
Useful? React with 👍 / 👎.
| content.rosterRevision !== null && | ||
| session.latestRosterRevision !== null && | ||
| content.rosterRevision <= session.latestRosterRevision | ||
| ) { | ||
| return false; |
There was a problem hiding this comment.
If the relay process restarts while a huddle is active, its in-memory audio room recreates roster_revision from zero while this tracker retains the larger revisions from persisted lifecycle history. Reconnecting clients then emit valid joins and leaves with lower revisions, but this guard rejects every one, leaving pre-crash participants displayed and hiding newly joined participants until the huddle ends. Revision ordering needs a persisted/session epoch or a reset boundary that distinguishes a newly created room generation.
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: bc006f67087b049e2f9c4d8a2f26faceff628225..e60dfaec7be7f16f4b35244a9708383a586436b8 (exact head e60dfaec7be7f16f4b35244a9708383a586436b8)
Risk: high — this adds community-wide relay lifecycle reconstruction and a cache-level deadline scheduler to user-visible availability state.
The two blockers from my prior review are repaired at this head: participant transitions now require the relay signer and track admission IDs, with causal regressions; status expiry now has deadline scheduling and cleanup. This changed tree introduces two new blockers.
Blocking findings
-
[High] The status-expiration scheduler recursively writes its own watched cache and can starve the Desktop UI thread (
desktop/src/features/user-status/hooks.ts:34-53,177-207).scheduleExpiration()always callsexpireUserStatusQueries(), which always callssetQueriesData()even when every updater returns its original value. React Query still emits anupdated/successcache event for that operation; the cache subscriber queuesscheduleExpiration()again, producing an unbounded microtask chain. A real-QueryClientprobe reproduced the feedback loop. In the exact-head Playwright journey, the rendered app stopped responding at the first settings click; all three targeted status tests failed there, and the traced single-test browser responded only during timeout teardown.Author action: do not write the cache when no entry expired. For example, inspect
getQueriesData()and callsetQueryData()only for individual lookups that actually need a changed value. Add a real-QueryClientregression proving an ordinaryuser-statusupdate settles without recursively scheduling writes, then rerun the targeted indicator/tooltip E2E journey.Verification owner: product/UI reviewer for the real-cache regression and Playwright journey; integration reviewer for exact-head package/CI freshness.
-
[P2] Snapshot compaction deletes legacy LEFT ordering tombstones, allowing an older JOIN replay to restore false huddle presence (
desktop/src/features/huddle/lib/huddlePresence.ts:144-155,299-303,325-331;desktop/src/features/huddle/HuddlePresenceContext.tsx:59,102). Legacy participant events withoutadmission_id/roster_revisionare explicitly accepted, but every productionsnapshot()removes a non-present legacy state. A delayed/replayed older JOIN is then compared against no state and is accepted, falsely restoring the global 🎧 indicator. A production-shaped exact-head regression — START → legacy JOIN → snapshot → newer legacy LEFT → snapshot → replay old JOIN — failstrue !== falseat the stale replay assertion.Author action: preserve the latest legacy ordering tombstone for the active session, or retain an equivalent bounded replay/idempotency ledger. Add the snapshot-between-events regression while retaining the signer, admission, and revision regressions.
Verification owner: systems reviewer for the causal regression/full Desktop package; integration reviewer for exact-head CI.
Validation at matching exact HEAD
- PASS:
pnpm check(unrelated existing warnings/info only),pnpm typecheck, fullpnpm test(5,816 passed, 0 failed), focused repaired-blocker suite (17/17), andgit diff --check; final reviewed worktrees were clean ate60dfaec.... - FAIL: targeted
profile-custom-emoji-status.spec.tsPlaywright journey (3/3) at the first interaction; the traced single-test reproduction and real React Query probe establish the cache-loop cause above. - CI: live GitHub head matched
e60dfaec.... Desktop Core, relay E2E, Web, macOS Desktop build, Semgrep, zizmor, DCO, and container builds were green. Both Desktop E2E Integration shards were externally cancelled; the aggregator therefore failed withDesktop E2E Integration shards finished with: cancelled. Smoke shards were still running when checked. This CI condition is a verification gap rather than separate author rework; the source/runtime defects above independently block.
Non-blocking confidence gaps
- Initial huddle subscription failure still lacks provider-level retry; no impact beyond the relay session's reconnect machinery was reproduced. Verification owner: integration reviewer after blockers are repaired.
- Native Tauri lifecycle/reconnect and real native accessibility behavior were not manually exercised at this head. Verification owner: product/integration reviewers on the replacement head.
- Long-history reconstruction was validated from source/unit coverage, not against a live relay. Verification owner: integration reviewer as risk warrants.
Smallest safe merge path: break the status cache feedback cycle and add its real-cache regression; preserve legacy huddle ordering state and add the snapshot/replay regression; then rerun full Desktop tests, the targeted interaction journey, and fresh exact-head CI.
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: 0c75315974
ℹ️ 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".
| if (hydrationFailed) { | ||
| void dispose(); | ||
| cleanup = null; | ||
| pendingLiveEvents.length = 0; | ||
| setParticipantPubkeys(EMPTY_HUDDLE_PRESENCE); | ||
| return; |
There was a problem hiding this comment.
When the initial lifecycle backfill fails, such as from a one-off relay timeout, this branch disposes the already-established live subscription and leaves the presence set empty. Because the effect depends only on the effectively static relay pubkey/error state, a later reconnect or successful request does not restart hydration, so every huddle indicator remains disabled until the app subtree remounts; retry the backfill or reinitialize it on reconnect instead of permanently disposing the subscription.
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: bc006f67087b049e2f9c4d8a2f26faceff628225..0c753159749e5f85eaab3239fed6cd523d43426f (exact head 0c753159749e5f85eaab3239fed6cd523d43426f)
Risk: high — this adds community-wide relay lifecycle state whose correctness depends on disconnect/reconnect recovery, alongside user-visible status and huddle indicators.
Blocking finding
desktop/src/features/huddle/HuddlePresenceContext.tsx:62-72 creates the lifecycle subscription with limit: 0, while its history hydration at :81-103 runs only once when the provider mounts. On reconnect, desktop/src/shared/api/relayReconnectReplay.ts:82-94,316-320 preserves that zero limit for this non-channel subscription, so the replay request cannot return lifecycle events missed while disconnected.
A join, leave, or end missed during an outage is therefore not recovered. The status-adjacent huddle indicator can remain absent or stale until another lifecycle event arrives or the provider remounts. The repository already captures this same limit: 0 reconnect failure mode for another subscription in desktop/src/shared/api/observerRelay.test.mjs:7-29.
Author action: make this lifecycle subscription replay-capable with a bounded since and positive limit, or explicitly rehydrate lifecycle history after reconnect. Add a regression test that disconnects, misses join/leave/end, reconnects, and proves presence converges.
Verification owner: reviewer — focused reconnect regression, then the exact-head Desktop unit suite and typecheck.
Integrated review results
No additional author-actionable product/UI defect was found. The changed head removes the prior status-cache write loop; mutation testing restored the old body and made statusExpiration.test.mjs fail as expected. Status expiration scheduling, unchanged/edit/revert Save behavior, indicator accessible names/tooltips, and the status editor's duration/calendar/time states were exercised successfully. Systems review also confirmed same-second lifecycle ordering by roster revision, ended-channel tombstones, event-ID deduplication, and the participant/start/end authorization and admission checks.
Validation at clean exact head: just desktop-check passed; just desktop-typecheck passed; just desktop-test passed (5,818/5,818); pnpm build passed; pnpm test:e2e:smoke -- --grep status passed (3/3). Local HEAD and remote PR head both matched 0c753159749e5f85eaab3239fed6cd523d43426f before submission.
Manual/native evidence: browser E2E artifacts under desktop/test-results/profile-status/ covered the status editor and indicator states. A real Tauri/native accessibility snapshot, OS keyboard-focus tooltip journey, wall-clock visible expiration journey, and measured large-roster latency cohort were not run.
Residual risk / ownership: those native and performance observations remain reviewer/tooling or release-validation confidence gaps, not separate author defects. At the final snapshot, several exact-head CI jobs were still in progress; CI owns those gates. They do not change the reconnect defect above.
— :bot: Jude’s code review agent
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Verdict: REQUEST CHANGES
Reviewed bc006f67087b049e2f9c4d8a2f26faceff628225..0c753159749e5f85eaab3239fed6cd523d43426f. This review independently checks the existing current-head inline findings, rather than treating the previous summary or green CI as proof. The remaining defects affect the new huddle indicators, not audio transport itself.
1. [P2] Apply lifecycle ordering across the entire hydration result
desktop/src/features/huddle/HuddlePresenceContext.tsx:82-86 feeds history directly into the incremental tracker. Precision correction to the existing comment: Desktop sorts each page ascending by (created_at,id) (relayClosedRecovery.ts:224, relayClientShared.ts:114-124). That still does not order START before participant events/END within a second, or participants by roster revision; fetchActiveHuddleLifecycle also appends older pages after the newest page (huddlePresence.ts:174-198). The lifecycle-aware comparator exists, but only the unused production reconstruction helper calls it.
Production-function probes reproduced four failures: same-second JOIN before START loses a participant; END before START resurrects the creator; cross-participant revision 2 before revision 1 drops the latter's valid JOIN; START on page two loses page-one joins. The lifecycle-sorted helper produces correct results for the same fixtures. Fix: globally lifecycle-order the history used to initialize the live tracker, preserving the hydration/live handoff. Add provider-shaped regressions for same-second phase/revision and multi-page hydration, not only tests of the standalone sorted helper.
2. [P2] Recover lifecycle events missed during disconnect
HuddlePresenceContext.tsx:62-72 subscribes with limit: 0, and its backfill runs only during effect setup. The generic replay keeps that zero limit for this non-channel subscription (relayReconnectReplay.ts:82-94,104-110,316-320). A JOIN, LEFT or END during an outage is therefore not delivered on reconnect; indicators can remain missing or stale indefinitely despite a healthy socket.
Fix: rehydrate/reconcile on reconnect or use a replay-capable lifecycle subscription with an adequate history boundary. Prove convergence after separately missed JOIN, LEFT and END, without remounting the provider. This confirms the existing current-head reconnect blocker.
3. [P2] Retry failed initial hydration instead of disabling presence for the session
A single failed backfill sets hydrationFailed, disposes the working live subscription, and leaves an empty set (HuddlePresenceContext.tsx:87-97). The effect depends only on relay identity/error (:116), which is effectively static; reconnect does not restart it. A one-off history timeout therefore disables every huddle indicator until the community subtree remounts.
Fix: retain a bounded recovery path after transient backfill/subscription failure, including reconnect, while avoiding an incomplete presence claim. Add a fail-once-then-succeed test that proves the provider actually resubscribes/rebuilds state, plus teardown coverage.
4. [P2] Do not treat backing-channel TTL as the maximum audio lifetime
huddlePresence.ts:157-170 excludes all lifecycle records older than 3,900 seconds because it assumes huddles cannot still be active. The reaper archives the backing channel and evicts its Nostr subscriptions (crates/buzz-relay/src/main.rs:680-737); it does not terminate already-connected audio. The audio receive loop continues until leave/disconnect/cancellation (audio/handler.rs:1040-1146), and the configurable ephemeral TTL can also exceed the hardcoded horizon.
Consequently, opening Desktop during a quiet call older than 65 minutes drops its canonical START and participants. A production-fetcher probe returned no rows for that still-active fixture. Fix: reconstruct from an authoritative active-session boundary/canonical starts rather than a time limit the producer does not guarantee. Test a still-connected session beyond the horizon; merely increasing the constant postpones the error. This does not request changing call duration as a side effect of an indicator fix.
5. [P2] Do not compare revisions across different room generations
The tracker rejects any participant revision at or below the session-wide maximum (huddlePresence.ts:274-303). The relay constructs fresh process-local rooms on restart (state.rs:915, audio/room.rs:184-193,580-595) and starts their revisions at zero, while lifecycle history persists. The emitted payload carries admission ID and revision but no room-generation identity (audio/handler.rs:1367-1381).
After an unclean restart and rejoin to the still-unarchived backing channel, valid revision-1 joins/leaves are rejected against pre-crash revision N. A real-tracker probe rejected a new admission's revision-1 JOIN after persisted revision 20. Fix: reconcile against the authoritative current room or make ordering generation-aware; do not blindly remove stale-replay protection. Cover new-generation JOIN/LEFT while retaining the existing delayed-replay/admission regressions.
Validation and boundaries
- Repository commands and focused production-function probes ran only on Blox
carl-review-7112-0831at the reviewed SHA. No production files were changed, no dependencies installed, and no commits/pushes/approvals performed. Probes use actual imported functions with controlled events/transport responses; they are not a live relay crash or long-call test. - Existing exact-head GitHub CI is green for Desktop Core, all four smoke shards, both integration shards, relay E2E and macOS build: https://github.com/block/buzz/actions/runs/33437298040. Broad suites were not duplicated. The separate automated security review is skipped/stale, not a newly established code defect.
- Signer/linkage checks, admission identity and the legacy leave tombstone repair are coherent. The previous status-expiration cache loop is repaired. Status/UI review found no additional blocker within the stated surfaces: ordinary/shared-thread message authors, 1:1 DM header/sidebar and self-profile status control. Group titles and unrelated forum/Home author renderers are not an unstated expansion requirement. Native Tauri/AX and new browser/live-relay journeys were not run. Author-set status fetch flicker and the previously noted non-focusable tooltip triggers remain non-blocking follow-ups.
Stable exit contract: ordered initial reconstruction; recovery after disconnect and transient initialization failure; correct active-session lifetime and room-generation handling. Add causal regressions at these integration boundaries, then use fresh exact-head Desktop/CI and targeted workflow evidence. No general huddle redesign or unrelated UI expansion is requested.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
This scheduled source-only pass supplements the earlier current-head review, which has no Auto PR Review v1 marker. It confirms the hydration/recovery defects and adds incremental-ordering and editor-transition findings. It does not withdraw the earlier review’s other findings or claim its runtime evidence.
Reviewed head 0c753159749e5f85eaab3239fed6cd523d43426f against base bc006f67087b049e2f9c4d8a2f26faceff628225. The intended contract is current status/huddle indicators beside desktop names, with an expiry-aware editor that preserves existing status data and community isolation.
1. [P2] Apply hydrated huddle history in lifecycle order
HuddlePresenceContext.tsx:82–86
fetchEvents sorts each page by (created_at, id), but fetchActiveHuddleLifecycle appends newest-page blocks before older blocks. This loop feeds that result directly into the tracker instead of using the lifecycle ordering in reconstructHuddlePresence. With more than 500 visible lifecycle events in the active window, a session’s JOIN/LEFT/END can be applied before its older STARTED and discarded by if (!session) return false. Loading an already-ended session can consequently mark its creator as present; active participants can disappear. Even one page can misorder same-second STARTED/END or roster revisions because IDs are not lifecycle sequence numbers. Sort the complete hydration set with lifecycle semantics before applying it and exercise the actual provider fetch-to-tracker path, including cross-page and same-second cases.
2. [P2] Reconcile huddle presence after reconnect or failed hydration
HuddlePresenceContext.tsx:62–74 and 92–116
The new subscription has limit: 0. The shared reconnect implementation preserves that zero limit (relayReconnectReplay.ts:82–110, 315–320), so restoring the subscription does not replay missed JOIN/LEFT/END events. The provider neither subscribes to reconnect notifications nor refetches its state. Reproduce by observing an active huddle, disconnecting this desktop, ending the huddle elsewhere, then reconnecting: the name indicator remains present until a remount. A transient history failure is worse: lines 92–97 dispose the live subscription and leave the feature empty without any retry. Add recoverable, cancellation-safe reconciliation and retain the history/live overlap during it; cover an offline END and a failed initial fetch followed by recovery.
3. [P2] Do not discard another participant’s delayed lifecycle delta
latestRosterRevision is session-wide, but JOIN/LEFT events are individual admission deltas, not full roster snapshots. The relay allocates revisions under the room mutex (audio/room.rs:328–341), then emits Nostr lifecycle events from independent connection handlers after releasing it (audio/handler.rs:702–714, 907–919); persistence and fanout are asynchronous (1417–1463). Thus Bob’s revision-2 JOIN can arrive before Alice’s revision-1 JOIN. This gate permanently drops Alice’s valid delta merely because Bob’s arrived first; the same race can retain someone whose LEFT was delayed. Sorting hydration alone cannot repair the live path. Preserve per-admission ordering/tombstones or explicitly reconcile revision gaps rather than treating a later unrelated delta as a snapshot. Test out-of-order events for different participants/admissions through the incremental path.
4. [P2] Keep an open status draft when the saved status expires
Open an existing status shortly before expiry, edit its text and choose a later deadline, but do not save yet. The new expiry scheduler nulls the self-status query at the original deadline; AppShell → SidebarProfileCard → ProfilePopover passes empty initial values into this still-open dialog. This effect then erases the unsaved draft and resets its duration. Initialize on the opening transition, or otherwise prevent persisted-status updates from overwriting a dirty draft. Add an open-editor expiry regression case.
5. [P2] Allow applying Today to an existing non-expiring status
All statuses created by the previous desktop editor have no expiration. Opening one sets both duration and initialDuration to Today. Explicitly selecting Today still leaves isDirty false, so Save stays disabled unless the user also changes the content or chooses a different duration. The UI shows Today while the persisted status remains indefinite. Distinguish absent expiry from a selected expiry so the user can apply Today without editing their status text/emoji.
6. [P2] Preserve an existing custom deadline when editing only text
Mobile already supports minute-precision custom deadlines (mobile/lib/features/profile/set_status_sheet.dart:448–453, 473–484). Open a mobile status expiring tomorrow at 10:10, change only its text, and save: initialization rounds the deadline to 10:30, and expirationUnixSeconds() publishes that rounded value even though the user never edited expiry. The dirty baseline also rounds it, hiding the change. Keep the original Unix timestamp until the user explicitly changes expiration; the half-hour menu can remain the selection UI without rewriting existing values.
Review coverage
Source-only review of changed message/DM/sidebar/profile surfaces, provider/community ownership, status publication/cache/expiry, huddle history/live/reconnect and relay lifecycle producers, and editor/calendar transitions. No checkout, build, test execution, or runtime validation was performed. The added tests cover sorted reconstruction and selected editor happy paths, not the failing production seams above. Existing client-signed STARTED behavior is intentional, not an authentication finding; community remount ownership is intact.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
…atus-indicators Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 @wesbillman @jedwards27 Addressed the current review feedback in 84a7766. Huddle presence now rebuilds complete lifecycle history, reconciles missed join/leave/end events after reconnect, retries failed hydration, and avoids treating roster revisions as session-wide or cross-restart. The status editor now preserves open drafts and exact custom deadlines, and allows a non-expiring status to be changed to Today. Added focused unit and rendered E2E regressions; the post-merge pre-push suite passed 5,866/5,866. Please re-review when you have a chance. |
There was a problem hiding this comment.
💡 Codex Review
buzz/desktop/src/features/user-status/hooks.ts
Lines 128 to 132 in 84a7766
Chunk status lookups at the relay page limit
When the provider registers more than 1,000 authors with stored status heads—for example, a large expanded DM sidebar—this requests one event per author, but the relay clamps every REQ to buzz_db::DEFAULT_MAX_PAGE_LIMIT (1,000). The omitted authors are initialized to null, and every refetch repeats the same capped query, so their indicators can remain missing indefinitely; split the authors into bounded chunks and merge the results.
ℹ️ 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".
| function expirationUnixSeconds(): number | undefined { | ||
| if (!durationTouched) return baseline.expiresAt; |
There was a problem hiding this comment.
When creating a new status, the dialog displays Today, but durationTouched remains false unless the user opens the duration menu. Typing a status or choosing a quick preset and immediately saving therefore returns baseline.expiresAt (undefined), publishing an indefinite status that contradicts the selected UI value. Compute the displayed Today deadline for new statuses while preserving an existing expiration only when editing an existing status.
Useful? React with 👍 / 👎.
| for (;;) { | ||
| const page = await fetchEvents({ | ||
| kinds: [ | ||
| KIND_HUDDLE_STARTED, | ||
| KIND_HUDDLE_PARTICIPANT_JOINED, | ||
| KIND_HUDDLE_PARTICIPANT_LEFT, | ||
| KIND_HUDDLE_ENDED, | ||
| ], | ||
| ...(until === undefined ? {} : { until }), | ||
| limit: HUDDLE_LIFECYCLE_PAGE_LIMIT, | ||
| }); | ||
| for (const event of page) events.set(event.id, event); | ||
| if (page.length < HUDDLE_LIFECYCLE_PAGE_LIMIT) break; |
There was a problem hiding this comment.
In a mature community, every desktop startup and reconnect now pages from the newest lifecycle event all the way to the first one ever stored, retaining the complete result in events before reconstruction. The request count, transfer size, memory, and time before any huddle presence appears therefore grow permanently with historical huddle activity and multiply across clients; discover active sessions or otherwise fetch a bounded active-session representation instead of accumulating all completed history.
AGENTS.md reference: AGENTS.md:L194-L202
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head 84a77669a8199219f0057aeeac2503bfedb567d3 against exact base 571c1902d0ca55cfd4ccf6b91eeb731909cc10be. The contract remains accurate current desktop status/huddle indicators, an expiry-aware editor that preserves existing deadlines and drafts, and community isolation.
The corrective changes address global hydration ordering, reconnect reconciliation, failed-hydration recovery, unrelated-admission revision rejection, the unsupported 65-minute horizon, open-draft reset, explicit Today selection on an indefinite status, and minute-precision deadline preservation. Four actionable defects remain, including a live-routing integration failure not identified in the earlier review.
1. [P2] Subscribe within the relay's channel scope to receive live huddle events
huddlePresenceRuntime.ts:160–167
This filter has no #h. relayClient.subscribeLive forwards it unchanged, and the real relay registers it in the global subscription index (req.rs:277–307). Huddle JOIN/LEFT/END are persisted with the parent channel and delivered on that channel's topic (audio/handler.rs:1414–1463). The fanout registry deliberately excludes global subscriptions from channel events (subscription.rs:387–425, 487–492). Local and Redis delivery both use that registry.
Reproduction: let the provider hydrate an active huddle, keep the observer's connection healthy, and have another participant leave or end the huddle. The callback receives no lifecycle update, so the name indicator remains stale until reconnect/remount; later joins likewise do not appear. History can succeed because its query includes accessible channels, which does not grant a global subscription channel-scoped live delivery.
Fix: maintain lifecycle subscriptions over the authorized parent-channel set, including membership changes, or use an existing channel-scoped event source. Preserve the relay's isolation boundary rather than broadening global fanout. Add a relay-shaped live JOIN/LEFT/END regression without reconnecting. The current browser mock masks this: e2eBridge.ts:4791–4800 explicitly delivers channel events to GLOBAL_MOCK_SUBSCRIPTION, unlike production; the standalone runtime tests inject callbacks directly.
2. [P2] Apply the displayed Today default when creating a status
With no existing status, open the dialog, enter text or choose a quick status, and Save without touching Duration. Initialization displays Today but captures baseline.expiresAt = undefined and durationTouched = false. This unconditional preservation guard returns undefined; the publisher omits the expiration tag. The status remains indefinite despite the displayed Today duration. The previous head calculated local midnight on this path.
Fix: distinguish creating a status from preserving an existing status's deadline. A new status must apply its displayed default. Test creation through text/quick-status selection without opening Duration, and assert the signed expiration is local midnight. The existing duration test explicitly selects 8 hours before Save and misses this path.
3. [P2] Validate the preserved deadline before closing an expired status draft
SetStatusDialog.tsx:272–275, 287–315
Open an existing 1-hour status shortly before its deadline, let it expire while the editor remains open, then edit only the text and Save. The draft correctly survives now, but the captured deadline is in the past. canSave checks future validity only for Custom; the untouched 1-hour duration is allowed, expirationUnixSeconds() returns the expired baseline, and handleSave closes the dialog. On successful publication the status cache immediately expires the edit (hooks.ts:176–205, 286–310); receiving clients also discard it. A valid-looking Save therefore does not establish a current status.
Fix: validate the actual outgoing deadline for every duration at Save time, including preserved deadlines. If it has expired, retain the draft and require a future selection rather than silently closing. Add a natural-expiry → text-only edit → Save regression. The new draft-expiry E2E selects This week before injecting expiry and never saves afterward, so it does not cover this transition.
4. [P2] Retire dead admissions when reconciling a restarted room
huddlePresence.ts:120–127, 270–277
The revision-rejection part of the earlier restart finding is fixed: a new admission with revision 1 is accepted. The old generation's admission is not retired, however, and this OR treats any historical present: true admission as current.
Reproduction for one still-unarchived backing channel: persist Bob's JOIN under admission A/revision 20; crash the relay before its leave cleanup; rejoin Bob under fresh admission B/revision 1 and also rejoin Charlie; let Bob leave B while Charlie remains connected. Rehydrate the observer. B is absent, but A is still present, so Bob keeps the huddle indicator despite having no current admission. Keeping Charlie connected is essential: the last participant leaving normally emits END and clears the session.
At this exact head, rooms are process-local (state.rs:915), revisions start at zero and admissions receive fresh UUIDs (room.rs:184–193, 314–329). The lifecycle payload has no room-generation identity (handler.rs:1346–1381); LEFT names only the new admission and END is conditional on the room emptying (907–952). Replaying all history cannot invent the missing retirement.
Fix: reconcile against authoritative current admissions or introduce a trustworthy room-generation/reset boundary that retires old admissions. Do not clear every earlier admission on a lower revision; that would regress the valid concurrent-admission/delayed-delta cases just fixed. Extend the new restart test through a post-restart LEFT while another participant remains, and verify both incremental and rehydrated state. This is the remaining restart contract, not a request to redesign call duration.
Coverage and exit criteria
Source-only, using pinned GitHub blobs/diffs and metadata. No checkout, build, test execution, CI query, or native/browser runtime validation was performed. The added tests were inspected as source, not claimed as passing. Two independent lanes reviewed editor transitions and tracker/producers; I verified and integrated their findings and traced the shared relay/runtime boundary.
Covered ordinary/shared-thread message authors, 1:1 DM header/sidebar, self-profile control, status publication/cache/expiry, huddle hydration/live/reconnect/retry, relay producers and community-keyed teardown. Group titles and unrelated forum/Home renderers remain outside the agreed surfaces. Existing non-focusable tooltip triggers remain a non-blocking follow-up, not new corrective scope.
Stable exit: real channel-scoped live delivery; default creation deadline honored; expired preserved deadlines cannot silently lose a saved draft; dead room generations do not contribute current presence. Preserve the credited fixes and add causal regressions at those four boundaries. No approval is submitted by this automation.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed base 571c1902d0ca55cfd4ccf6b91eeb731909cc10be → exact head 84a77669a8199219f0057aeeac2503bfedb567d3. The previous lifecycle/editor blockers were re-checked against the changed head; most are repaired, but three current-head defects remain.
1. Same-second canonical huddle restart keeps dead-generation presence
HuddlePresenceTracker.apply() rejects a same-created_at start when the candidate event ID is lexicographically lower (desktop/src/features/huddle/lib/huddlePresence.ts:206-227, especially 213-216). This is backwards for this tree's Nostr head ordering, where lower event ID wins on equal timestamps (desktop/src/features/agents/lib/usePersonaSync.ts:68-72). A black-box probe using old 48100 → Bob admission 48101 → same-second lower-ID canonical 48100 left Bob visible from the replaced generation.
Author action: use the canonical lower-ID-wins comparator for equal timestamps and add a regression proving a superseding same-second start drops participants admitted only to the old generation.
2. Bulk status lookup silently drops authors above the relay's 1,000-result ceiling
useUserStatusQuery() sends every author in one request and sets limit to the author count (desktop/src/features/user-status/hooks.ts:117-159, especially 128-133). The relay clamps each request to DEFAULT_MAX_PAGE_LIMIT (crates/buzz-relay/src/handlers/req.rs:957-960), defined as 1,000 (crates/buzz-db/src/store/event.rs:28-33,397-399). Since the hook initializes omitted authors to null, users beyond the returned 1,000 status heads silently lose their indicators.
Author action: chunk authors into bounded requests and merge their latest heads, or implement exhaustive keyset pagination; add a regression with more than 1,000 distinct authors proving every current head is returned.
3. Draft-preservation E2E waits on a nonexistent element and cannot catch the regression
desktop/tests/e2e/profile-custom-emoji-status.spec.ts:224 waits for data-testid="sidebar-user-status", but that test ID does not exist under desktop/src; the actual element is sidebar-profile-user-status at desktop/src/components/SidebarProfileCard.tsx:215. toHaveCount(0) therefore succeeds immediately without proving expiration reached the mounted query. Mutation evidence is causal: removing the one-shot dialog-open guard at SetStatusDialog.tsx:217-218 still passed the shipped test; correcting only the locator made the mutant fail (empty input instead of Unsaved draft), while restored production code passed. This does not meet TESTING.md:25-31's falsifiable production-seam requirement.
Author action: wait on sidebar-profile-user-status (or another positive signal that expiration propagated), retain the draft assertions, and verify the corrected test fails when the one-shot guard is removed and passes when restored.
What cleared / validation
- Source and focused coverage cleared pagination/deduplication, reconnect replay and reconciliation, stale-generation rejection, failed-hydration retry/disposal, relay-signed participant validation, admissions/revisions, authorized end handling, cache expiry, and session-scoped relay ownership except for finding 1.
- Product behavior passed for draft preservation with the corrected wait, indefinite → Today, exact custom deadlines, expiration hiding, status/huddle labels and tooltips, 14px indicator sizing, and mocked reconnect convergence. The changed-head rendered smoke spec passed 6/6; screenshot inspected:
test-results/profile-status/07-name-indicators.png(SHA-256 prefix37c00a65). - At exact head with a clean worktree: focused huddle/status tests 23/23, full Desktop unit suite 5,866/5,866,
pnpm check,pnpm typecheck, andpnpm buildall passed.
Confidence gaps (not additional author blockers)
- GitHub CI still has
Desktop Smoke E2E (3)failing andDesktop Corein progress. Failed logs are unavailable until the run completes, so PR causality is not yet classified. Verification owner: reviewer/CI follow-up. - Native macOS AX/keyboard-focus and wall-clock Desktop expiration were not run; browser E2E uses mocked reconnect rather than a real relay disconnect. Verification owner: native/release validation.
- Backend/Rust jobs skipped under changed-path policy provide no additional evidence for the touched cross-boundary assumptions. Verification owner: CI/release policy.
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: 6de985d776
ℹ️ 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 (const session of this.sessions.values()) { | ||
| if (session.endState) continue; |
There was a problem hiding this comment.
After an ungraceful relay restart while a huddle is active, the persisted lifecycle contains starts/joins but no leaves or end event, and this snapshot continues treating every session without endState as active forever. The ephemeral-channel reaper in crates/buzz-relay/src/main.rs only archives the channel and evicts subscriptions—it does not emit kind 48103—so reconnect hydration repeatedly resurrects the dead room and permanently shows its former participants as huddling unless a new lifecycle event happens to reuse that room. Add a relay-authoritative liveness/termination boundary rather than interpreting an unterminated persisted session as indefinitely active.
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed base 571c1902d0ca55cfd4ccf6b91eeb731909cc10be → exact head 6de985d776dcb0de1f713968636db15480d3c6b8. The three previous blockers are repaired, but the changed-head lifecycle still has one author-actionable correctness defect.
Dead huddles resurrect indefinitely after relay restart or TTL reaping
HuddlePresenceTracker.snapshot() treats every session without endState as active (desktop/src/features/huddle/lib/huddlePresence.ts:328-339). A terminal KIND_HUDDLE_ENDED event is emitted during normal empty-room cleanup (crates/buzz-relay/src/audio/handler.rs:921-952), but the TTL reaper archives the backing channel, emits discovery/system events, and evicts subscriptions without persisting a 48103 terminal event (crates/buzz-relay/src/main.rs:680-738). An ungraceful relay restart has the same persisted-history shape: start/join events remain, but there is no trustworthy terminal event.
On the next startup or reconnect, full-history hydration therefore reconstructs that dead room and exposes its participants as active forever. The new lower-roster-revision heuristic does not close this hole: it runs only if a later participant event arrives for the same room (huddlePresence.ts:268-286). With no later lifecycle event, stale presence never clears. The current regression at desktop/src/features/huddle/lib/huddlePresence.test.mjs:215-293 covers a later join from a new relay generation, not restart/reaping with no later event.
This is independently corroborated by the current-head Codex finding: #7112 (comment)
Author action: establish a relay-authoritative liveness/termination boundary during hydration, or ensure archive/restart recovery persists a trustworthy terminal event. Add a regression covering persisted start + join, relay restart/reap, and no subsequent lifecycle event; the participants must not reappear as active.
Previous blockers cleared
- Same-second canonical restart: lower-ID equal-second starts are now accepted (
huddlePresence.ts:215-235). The focused regression passes; reversing the comparison makes that regression fail. - More than 1,000 status authors: requests are chunked at 1,000 and merged (
desktop/src/features/user-status/hooks.ts:122-177), matching the relay clamp (crates/buzz-relay/src/handlers/req.rs:957-960). The new fake-fetch test does not model relay truncation—raising the chunk size to 10,000 still passes—so causal test hardening remains a confidence gap, not a second functional blocker. - Draft preservation: the E2E now waits on the real
sidebar-profile-user-statussignal (desktop/tests/e2e/profile-custom-emoji-status.spec.ts:196-228; production locator atdesktop/src/features/sidebar/ui/SidebarProfileCard.tsx:215). Removing the one-shot initialization guard makes the corrected journey fail with the draft erased; restored production passes.
Exact-head validation
At a clean 6de985d776dcb0de1f713968636db15480d3c6b8 tree:
- Focused lifecycle/status tests: 24/24 passed.
- Full Desktop unit package: 5,869/5,869 passed.
- Focused rendered status/indicator E2E: 7/7 passed.
pnpm typecheck,pnpm check, andgit diff --checkpassed (pnpm checkreported nine pre-existing/out-of-diff warnings or infos).- Rendered evidence covered status editor expiry/recovery, Today and exact-deadline transitions, indicator labels/tooltips, 14px sizing, calendar/time controls, and bounded scrolling.
Confidence gaps (not additional author blockers)
- The >1,000-author test should model the relay's hard cap so it fails when chunking exceeds 1,000. Verification owner: reviewer/test hardening.
- Keyboard-only tooltip discoverability, native Tauri AX/dark-theme/zoom behavior, and real OS keyboard behavior were not natively exercised. Verification owner: accessibility/native release validation.
- At final preflight, live GitHub head still matched this review. Desktop integrations, macOS/Windows builds, Web, security checks, and public push gateway builds were green; Desktop Core, smoke shards, and two Docker builds were still running. Verification owner: CI/release follow-up; author action only if a PR-caused failure emerges.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed base 571c1902d0ca55cfd4ccf6b91eeb731909cc10be through exact head 6de985d776dcb0de1f713968636db15480d3c6b8.
The changed head repairs the three prior blockers: canonical same-second start ordering is corrected with mutation-sensitive coverage; status lookups are chunked at the relay ceiling; and the draft-preservation E2E now waits on the real mounted expiration signal and fails when the one-shot guard is removed. One current systems defect remains.
Dead huddles can resurrect indefinitely after relay restart or TTL reaping
HuddlePresenceTracker.snapshot() treats every session without endState as active (desktop/src/features/huddle/lib/huddlePresence.ts:328-339). However, KIND_HUDDLE_ENDED is emitted only by normal empty-room cleanup (crates/buzz-relay/src/audio/handler.rs:921-952). The TTL reaper archives the backing channel, emits discovery/system events, and evicts subscriptions without persisting a 48103 terminal lifecycle event (crates/buzz-relay/src/main.rs:680-738). An ungraceful relay restart likewise leaves persisted start/join events without a terminal event.
On each Desktop startup or reconnect, full-history hydration therefore reconstructs the abandoned session and shows its participants as still in a huddle indefinitely. The new lower-roster-revision heuristic at huddlePresence.ts:268-286 cannot repair this case because it runs only when a later participant event arrives for the same room. The regression at huddlePresence.test.mjs:215-293 covers a later new-generation join, not restart/reap with no subsequent lifecycle event.
Author action: establish a relay-authoritative liveness/termination boundary during hydration, or make archive/restart recovery persist a trustworthy terminal event. Add a causal regression covering persisted start + join, relay restart or TTL reap, and no later lifecycle event; the participant must not be resurrected.
Verification owner: author for the implementation/regression; systems reviewer for exact-head restart/reap tracing and mutation-shaped verification.
Exact-head validation
- Full Desktop unit package: 5,869/5,869 passed.
- Focused lifecycle/status suite: 24/24 passed.
- E2E build and full focused status/indicator spec: 7/7 passed.
pnpm check,pnpm typecheck, andgit diff --checkpassed on clean exact-head trees.- Product behavior cleared: draft survives expiration with causal mutation proof; indefinite → Today, exact custom deadlines, expired-deadline recovery, status/huddle labels, tooltips, sizing, and rendered calendar/time states passed.
Confidence gaps — not additional author actions
The new >1,000-author unit test does not itself model relay truncation—raising the chunk size still passed because its fake returns every requested author—although the production chunking implementation matches the documented 1,000-row relay clamp. Native Tauri AX/dark-theme/zoom/OS-keyboard behavior and a real-relay disconnect were not exercised. Several exact-head CI jobs were still running at submission. These are reviewer/tooling or CI-owned verification gaps, not extra blockers.
Duplicate exact-head review submitted concurrently; superseded by review 5075592006.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head 6de985d776dcb0de1f713968636db15480d3c6b8 against base 571c1902d0ca55cfd4ccf6b91eeb731909cc10be. This is a focused source-only re-review of the corrective delta and the previously agreed contracts: accessible-channel huddle indicators must reflect current admissions, and status editing must preserve drafts and publish the selected expiration. No checkout, build, test execution, or live-runtime validation was performed.
The explicit member-channel subscriptions fix the previous live-routing mismatch, and both reported editor-expiration defects are corrected. Two huddle issues remain:
[P2] A repeated roster revision does not retire pre-restart admissions
Anchor: huddlePresence.ts:278–293.
The new reset requires a strictly lower revision. A normal first-admission restart never satisfies that condition: Bob is the only admitted participant at revision 1; the relay crashes without emitting a leave; Bob rejoins the still-active backing huddle with a new admission at revision 1; Charlie joins at revision 2; Bob leaves his new admission at revision 3 while Charlie remains. The stored revisions are 1, 1, 2, 3, so the old Bob admission is never cleared. participantIsPresent still finds that admission present and shows Bob as in a huddle, including after complete history reconstruction.
This sequence follows the producer: room revisions initialize at zero, each admission increments them and gets a fresh UUID, and lifecycle payloads contain the revision/admission but no room-generation identity (room.rs:181–192, room.rs:314–341, handler.rs:1367–1382). The new regression starts the old generation at revision 20, so it only proves the strict-decrease case.
Use authoritative room-generation/current-roster evidence for retirement rather than assuming a revision decrease identifies every restart. Preserve overlapping live admissions and delayed-delta handling. Add the equal-revision restart sequence to both incremental and hydration coverage.
[P2] Ordinary messages tear down and rebuild all huddle presence
Anchor: HuddlePresenceContext.tsx:26–32.
The memo is keyed by the entire channelsQuery.data array, but the effect depends on the newly allocated channelIds array. Receiving an ordinary message with a newer channel timestamp changes that query array: useLiveChannelUpdates.ts:251–255 calls channelRecency.ts:35–61. Membership and the sorted ID values have not changed, but React sees a new dependency and runs cleanup: clear every indicator, close all lifecycle subscriptions, recreate them, and fetch the complete lifecycle history for every channel chunk. This happens on normal chat traffic, not just joining/leaving a channel. Sustained traffic can repeatedly dispose an in-flight rebuild, keeping indicators empty and multiplying full-history requests.
Make the effect depend on a content-stable membership/ID key (or preserve the array reference when its values match). Add a provider-level regression that changes only lastMessageAt and verifies presence, subscriptions, and history-fetch counts remain unchanged; actual membership changes must still reconfigure the runtime.
3 participants
Summary
Validation
Updated visual snapshots are attached in the PR comments.