Fix active huddle discovery - #68
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change centralizes active huddle reconstruction, adds multi-channel relay subscriptions, propagates summaries through sidebar sections, and renders participant badges. Replay tests and an end-to-end test cover active, ended, stale, isolated, and late-mounted huddle states. ChangesActive huddle discovery
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AppSidebar
participant useActiveHuddlesByChannelId
participant RelayClient
participant HuddleState
participant ChannelMenuButton
AppSidebar->>useActiveHuddlesByChannelId: provide channel IDs
useActiveHuddlesByChannelId->>RelayClient: subscribe to huddle events
RelayClient-->>useActiveHuddlesByChannelId: deliver lifecycle events
useActiveHuddlesByChannelId->>HuddleState: reconstruct active summaries
HuddleState-->>useActiveHuddlesByChannelId: return summaries by parent channel
useActiveHuddlesByChannelId-->>AppSidebar: provide active huddles
AppSidebar->>ChannelMenuButton: pass channel huddle summary
ChannelMenuButton-->>AppSidebar: render participant badge
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
a5f0871 to
63dab0b
Compare
![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: 63dab0b55e
ℹ️ 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".
| kinds: [48100, 48101, 48102, 48103], | ||
| "#h": ids, | ||
| limit: Math.min(1000, Math.max(100, ids.length * 20)), |
There was a problem hiding this comment.
When the sidebar contains more than one channel, this multi-value #h filter is classified as global by extract_channel_id_from_filters (crates/buzz-relay/src/handlers/req.rs:1009-1032). The registry intentionally does not fan channel-scoped events out to global subscriptions (crates/buzz-relay/src/subscription.rs:271-327), so the callback receives historical replay but no newly published 48100–48103 events; the sidebar badge therefore does not update live under the real relay, although the modified mock does. Use independently channel-scoped subscriptions or add real multi-channel subscription support.
Useful? React with 👍 / 👎.
| function updateState() { | ||
| if (disposed) return; | ||
| setActiveHuddles( | ||
| reconstructActiveHuddlesByParentChannel(seenEvents.values()), | ||
| ); |
There was a problem hiding this comment.
If a start event is followed by no lifecycle event, updateState runs only when that event arrives, so the Date.now()-based one-hour stale check in reconstruction is never evaluated again. After the ephemeral channel expires, the sidebar can continue showing a joinable huddle indefinitely and send users to an archived channel; schedule a recomputation for the nearest expiry or maintain a periodic expiry timer.
Useful? React with 👍 / 👎.
| async subscribeToHuddleEventsForChannels( | ||
| channelIds: string[], | ||
| onEvent: (event: RelayEvent) => void, | ||
| ) { |
There was a problem hiding this comment.
subscribeToHuddleEventsForChannels is a new public method on the exported RelayClient, but it has no doc comment. Document its historical/live behavior, channel handling, and result limits as required for new public APIs.
AGENTS.md reference: AGENTS.md:L116-L119
Useful? React with 👍 / 👎.
63dab0b to
40dddcd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40dddcd3ee
ℹ️ 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 freshnessAnchor = state.startedAt ?? state.lastEventAt; | ||
| if (isHuddleStartStale(freshnessAnchor, nowMs)) continue; |
There was a problem hiding this comment.
When the 100-event subscription window drops the start event, startedAt is null and this falls back to the most recent participant event. A join or leave at minute 59 therefore keeps the sidebar badge joinable until minute 119, even though the ephemeral channel's TTL is fixed at one hour from creation, so users can be routed to an archived channel. Fresh evidence beyond the previously reported timer issue is this missing-start path, which still extends the expiry after the timer was added; recover the original start time or avoid extending expiry from participant activity.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
|
|
||
| export function huddleParentChannelId(event: RelayEvent): string | null { |
There was a problem hiding this comment.
The new module exports its summary type, lifecycle parsers, and reconstruction helper without doc comments, leaving ordering, stale-event handling, and one-huddle-per-parent selection as undocumented API contracts. Add doc comments to these new exports as required by the repository contributor rules.
AGENTS.md reference: AGENTS.md:L116-L119
Useful? React with 👍 / 👎.
40dddcd to
6833404
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/features/huddle/components/HuddleIndicator.tsx (1)
48-97: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftDuplicate live relay subscriptions for the same channel's huddle events when it's both open and visible in the sidebar.
HuddleIndicatoranduseActiveHuddlesByChannelIdeach independently call intoRelayClientwith the identical filter (kinds 48100-48103,#h: [channelId]) and each maintain their ownseenEvents/reconstruction, because there's no shared subscription/state layer between the "current channel" indicator and the sidebar-wide hook.
desktop/src/features/huddle/components/HuddleIndicator.tsx#L48-L97: currently opens its ownsubscribeToHuddleEvents(channelId, ...); consider sourcingactiveHuddlefor the currently-open channel from the same mapuseActiveHuddlesByChannelIdalready computes at the sidebar/app level (passed down as a prop) instead of re-subscribing.desktop/src/features/huddle/lib/useActiveHuddlesByChannelId.ts#L26-L93: already subscribes to the current channel's huddle events as part of the full channel set — this is the subscriptionHuddleIndicatorshould be able to reuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/features/huddle/components/HuddleIndicator.tsx` around lines 48 - 97, The current-channel indicator creates a duplicate relay subscription instead of reusing sidebar state. In desktop/src/features/huddle/components/HuddleIndicator.tsx lines 48-97, remove the local subscribeToHuddleEvents, seenEvents, reconstruction, and cleanup flow, and consume the active-huddle entry from the map produced by useActiveHuddlesByChannelId via a prop or existing shared state. In desktop/src/features/huddle/lib/useActiveHuddlesByChannelId.ts lines 26-93, retain its existing subscription and reconstruction as the single source for both views; no direct change is required unless needed to expose the map to HuddleIndicator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/features/huddle/components/HuddleIndicator.tsx`:
- Around line 165-168: The participant-count floor logic is duplicated and can
diverge between badges. Add and export a shared getHuddleParticipantCount helper
from activeHuddleState.ts, then update HuddleIndicator.tsx lines 165-168 and
SidebarSection.tsx lines 156-187 to use it instead of local Math.max(1,
participantPubkeys.size) calculations, preserving the current minimum of one.
In `@desktop/src/shared/api/relayClientSession.ts`:
- Around line 391-409: Update subscribeToHuddleEventsForChannels to use
Promise.allSettled when subscribing across ids, retain the disposers from
fulfilled results, and dispose them all if any subscription rejects before
rethrowing or surfacing the failure. Preserve the existing returned cleanup
function for the case where every channel subscription succeeds.
In `@desktop/tests/e2e/channels.spec.ts`:
- Around line 219-274: The local waitForMockHuddleSubscription helper duplicates
the shared live-subscription polling logic. Remove it and replace its call sites
with waitForMockLiveSubscription(page, "random", KIND_HUDDLE_STARTED), relying
on the existing beforeEach-installed mock bridge.
---
Outside diff comments:
In `@desktop/src/features/huddle/components/HuddleIndicator.tsx`:
- Around line 48-97: The current-channel indicator creates a duplicate relay
subscription instead of reusing sidebar state. In
desktop/src/features/huddle/components/HuddleIndicator.tsx lines 48-97, remove
the local subscribeToHuddleEvents, seenEvents, reconstruction, and cleanup flow,
and consume the active-huddle entry from the map produced by
useActiveHuddlesByChannelId via a prop or existing shared state. In
desktop/src/features/huddle/lib/useActiveHuddlesByChannelId.ts lines 26-93,
retain its existing subscription and reconstruction as the single source for
both views; no direct change is required unless needed to expose the map to
HuddleIndicator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd6af4ee-a43d-4fdb-9283-499b4cac069b
📒 Files selected for processing (10)
desktop/src/features/huddle/components/HuddleIndicator.tsxdesktop/src/features/huddle/lib/activeHuddleState.test.mjsdesktop/src/features/huddle/lib/activeHuddleState.tsdesktop/src/features/huddle/lib/useActiveHuddlesByChannelId.tsdesktop/src/features/sidebar/ui/AppSidebar.tsxdesktop/src/features/sidebar/ui/CustomChannelSection.tsxdesktop/src/features/sidebar/ui/SidebarSection.tsxdesktop/src/shared/api/relayClientSession.tsdesktop/src/testing/e2eBridge.tsdesktop/tests/e2e/channels.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Desktop E2E Integration (2/2)
- GitHub Check: Desktop E2E Integration (1/2)
- GitHub Check: Desktop Core
- GitHub Check: Desktop Smoke E2E (4)
- GitHub Check: Desktop Build (macOS)
- GitHub Check: Desktop Smoke E2E (2)
- GitHub Check: Desktop Smoke E2E (3)
- GitHub Check: Desktop Smoke E2E (1)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (3)
desktop/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use rem-based Tailwind text tokens rather than arbitrary px, rem, or em text-size literals; prefer stock tokens,
text-2xs, ortext-3xs.
Files:
desktop/src/testing/e2eBridge.tsdesktop/src/features/huddle/lib/activeHuddleState.tsdesktop/src/features/sidebar/ui/AppSidebar.tsxdesktop/src/features/huddle/lib/useActiveHuddlesByChannelId.tsdesktop/src/features/sidebar/ui/CustomChannelSection.tsxdesktop/src/shared/api/relayClientSession.tsdesktop/src/features/huddle/components/HuddleIndicator.tsxdesktop/src/features/sidebar/ui/SidebarSection.tsx
desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
desktop/src/**/*.{ts,tsx}: Chat body and author text must usetext-base; surrounding timeline elements should use deliberate steps on the stock text ramp.
When adding a community-scoped module-level cache, Map, or class instance, add its reset toresetCommunityState()indesktop/src/features/communities/useCommunityInit.ts.
Files:
desktop/src/testing/e2eBridge.tsdesktop/src/features/huddle/lib/activeHuddleState.tsdesktop/src/features/sidebar/ui/AppSidebar.tsxdesktop/src/features/huddle/lib/useActiveHuddlesByChannelId.tsdesktop/src/features/sidebar/ui/CustomChannelSection.tsxdesktop/src/shared/api/relayClientSession.tsdesktop/src/features/huddle/components/HuddleIndicator.tsxdesktop/src/features/sidebar/ui/SidebarSection.tsx
desktop/tests/e2e/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
desktop/tests/e2e/**/*.ts: Every desktop Playwright E2E test must callinstallMockBridge(page)for mock Tauri IPC.
Callpage.addInitScriptbeforeinstallMockBridge(page)when seeding local storage or other initial state.
CallwaitForMockLiveSubscription(page, channelName)before emitting mock live messages.
CallwaitForAnimations(page)before everypage.screenshot()orlocator.screenshot().
Scope screenshots to their subject withlocator.screenshot()or an appropriateclip; verify multiple screenshots have distinct hashes before posting.
Files:
desktop/tests/e2e/channels.spec.ts
🔇 Additional comments (10)
desktop/src/features/huddle/lib/activeHuddleState.ts (2)
1-137: LGTM!
138-158: 🎯 Functional CorrectnessStart-based staleness is intentional here.
HUDDLE_JOINABLE_WINDOW_SECONDSis a hard joinable TTL fromstartedAt, and bothreconstructActiveHuddlesByParentChannelanduseActiveHuddlesByChannelIduse the same window. Confidence 92%.> Likely an incorrect or invalid review comment.desktop/src/features/huddle/lib/activeHuddleState.test.mjs (1)
1-182: LGTM!desktop/src/features/huddle/components/HuddleIndicator.tsx (1)
14-19: LGTM!Also applies to: 44-45, 57-63, 168-168
desktop/src/shared/api/relayClientSession.ts (1)
361-366: LGTM!desktop/src/features/sidebar/ui/AppSidebar.tsx (1)
16-16: LGTM!Also applies to: 242-242, 633-633, 685-685, 741-741, 783-783, 831-831
desktop/src/features/sidebar/ui/CustomChannelSection.tsx (1)
58-58: LGTM!Also applies to: 342-386, 445-446, 459-462, 552-593, 752-754
desktop/src/features/sidebar/ui/SidebarSection.tsx (1)
8-8: LGTM!Also applies to: 21-21, 289-301, 350-356, 382-410, 485-488
desktop/tests/e2e/channels.spec.ts (1)
18-18: LGTM!desktop/src/testing/e2eBridge.ts (1)
8753-8790: 🎯 Functional CorrectnessNo issue: live
#hfilters stay single-channel — all current callers pass one channel id, so theGLOBAL_MOCK_SUBSCRIPTIONfallback is not exercised and does not over-broadcast. Confidence 94%.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68334040c8
ℹ️ 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 disposes = await Promise.all( | ||
| ids.map((id) => | ||
| this.subscribe( | ||
| { kinds: [48100, 48101, 48102, 48103], "#h": [id], limit: 100 }, | ||
| onEvent, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
When a user has many joined channels, this Promise.all immediately sends one REQ per channel; the relay's default admission budget is only 50 WebSocket events per five-second burst (crates/buzz-auth/src/rate_limit.rs and crates/buzz-relay/src/admission.rs), shared with the rest of desktop startup. A sidebar with roughly 50 channels—or fewer when other startup subscriptions run—therefore gets CLOSED responses and can also delay the visible timeline while retries wait for the rate-limit window. Batch these initial REQs as reconnect replay already does in desktop/src/shared/api/relayReconnectReplay.ts.
Useful? React with 👍 / 👎.
| this.subscribe( | ||
| { kinds: [48100, 48101, 48102, 48103], "#h": [id], limit: 100 }, | ||
| onEvent, |
There was a problem hiding this comment.
For an active huddle that accumulates more than 100 join/leave events, a fresh sidebar or header subscription receives only the newest 100 rows, so the start event is omitted; reconstruction then explicitly discards the state at activeHuddleState.ts:141, hiding the active huddle and its join action until another session starts. Fresh evidence beyond the earlier missing-start expiry concern is that this revision now drops startless histories entirely rather than merely anchoring them incorrectly; fetch the recent start separately or otherwise guarantee it remains in the replay.
Useful? React with 👍 / 👎.
| const disposes = await Promise.all( | ||
| ids.map((id) => | ||
| this.subscribe( | ||
| { kinds: [48100, 48101, 48102, 48103], "#h": [id], limit: 100 }, | ||
| onEvent, | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
If a socket failure makes any per-channel subscribe reject after other calls in this Promise.all have succeeded, the method rejects without returning or invoking the successful disposers. Those live subscriptions remain registered in RelayClient; after the channel list changes, the old hook cannot close them and the replacement effect adds another full set, leaking subscriptions until a community disconnect and potentially reaching the relay's 1,024-subscription cap. Clean up every successfully created subscription on the rejection path.
Useful? React with 👍 / 👎.
| export function useActiveHuddlesByChannelId( | ||
| channels: readonly Channel[], | ||
| ): ReadonlyMap<string, ActiveHuddleSummary> { |
There was a problem hiding this comment.
Add a doc comment to this new exported hook describing its per-channel historical/live subscription behavior, expiry handling, and returned summary map; without it, the new public API leaves important lifecycle semantics undocumented and violates the repository's explicit public-API requirement.
AGENTS.md reference: AGENTS.md:L116-L119
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1e7463978
ℹ️ 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 existing = activeByParent.get(state.parentChannelId); | ||
| if (existing && existing.lastEventAt >= state.lastEventAt) continue; |
There was a problem hiding this comment.
When an older session has no 48103 row (a failure mode already acknowledged by HuddleIndicator) and a newer huddle in the same parent subsequently starts and ends, the newer ended state is skipped while this comparison still selects the older non-ended state. The sidebar and header then resurface the superseded huddle and offer a link to its already archived ephemeral channel. Treat a newer start as superseding prior sessions for that parent, even after the newer session ends.
Useful? React with 👍 / 👎.
| const sorted = [...events].sort( | ||
| (left, right) => | ||
| left.created_at - right.created_at || | ||
| eventOrder(left.kind) - eventOrder(right.kind) || | ||
| left.id.localeCompare(right.id), |
There was a problem hiding this comment.
When a participant disconnects and rejoins within one second, both relay-generated lifecycle rows have the same second-resolution created_at, but this ordering always places the 48101 join before the 48102 leave regardless of their actual emission order. Reconstruction therefore deletes the rejoined participant and understates the active count in both the sidebar and header; preserve a causal ordering for same-second join/leave transitions instead of ordering them by kind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aa7eb03d3
ℹ️ 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 id of ids) | ||
| disposes.push( | ||
| await this.subscribe( | ||
| { kinds: [48100, 48101, 48102, 48103], "#h": [id], limit: 500 }, |
There was a problem hiding this comment.
For users with roughly 50 or more joined channels, this sequential loop can still exhaust the relay's 50-events-per-five-second admission budget, especially alongside other startup subscriptions. Fresh evidence beyond the previously reported Promise.all burst is that subscribe() resolves as soon as EOSE arrives—and even a rate-limited CLOSED resolves its readiness promise—so await does not enforce pacing and the loop can emit REQs as quickly as the relay answers them. The resulting CLOSED retries delay huddle discovery and can rate-limit unrelated startup traffic; explicitly pace or batch these requests through the existing replay/rate-limit machinery.
Useful? React with 👍 / 👎.
| case KIND_HUDDLE_PARTICIPANT_LEFT: | ||
| return 1; | ||
| case KIND_HUDDLE_PARTICIPANT_JOINED: | ||
| return 2; |
There was a problem hiding this comment.
When a participant joins and then disconnects within the same second while other people keep the huddle active, both relay-generated rows have the same second-resolution created_at; this fixed leave-before-join ordering replays the leave first and then re-adds the participant, overstating the sidebar and header count. Fresh evidence beyond the prior rejoin report is this inverse transition: changing the global tiebreak fixes leave-then-rejoin but necessarily breaks join-then-leave, so same-second lifecycle rows need a causal ordering rather than a kind-only order.
Useful? React with 👍 / 👎.
| if (disposed || seenEvents.has(event.id)) return; | ||
| seenEvents.set(event.id, event); | ||
| updateState(); |
There was a problem hiding this comment.
When several joined channels have substantial lifecycle history, the replay callback invokes updateState() once per returned row, and each invocation re-sorts every row accumulated so far and publishes a fresh Map. With the new 500-row limit per channel, startup can therefore perform quadratic replay work on the UI thread and repeatedly invalidate the entire sidebar, causing a visible freeze for large or high-churn communities; coalesce buffered historical callbacks and reconstruct once per batch, while retaining the previous map when its content is unchanged.
AGENTS.md reference: AGENTS.md:L422-L422
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #63. Part of #36/#1.
Local proof on a5f0871
Proof boundary
This proves source behavior and mock-relay discovery only. It does not claim installed two-client huddle runtime proof; that remains for the cumulative Internal Canary artifact.