fix(chat-frontend): sidebar bucket fetch + always-render section headers#188
Merged
Conversation
useSidebarSections partitions summaries strictly by favoriteIds / appIds / channelDmIds. listRooms is independent of the three subscription.get* RPCs that populate those Sets, so if the bucket RPCs fail or haven't landed yet, every room in `summaries` is dropped by the partition and the sidebar renders blank — the user sees an "empty page" with no errors, just a console.warn from the fetchSidebarBuckets catch. Cold-start safety: when all three bucket Sets are empty, push every summary into Channels and DMs as a flat list. The next BUCKETS_LOADED dispatch re-partitions normally. Strict partitioning is preserved whenever any bucket has content.
📝 WalkthroughWalkthroughSidebar bootstrap now collects three subscription RPCs (getCurrent/getApps/getRooms) via fetchSidebarBuckets, merges subscriptions into per-room summaries, and returns room records; useSidebarSections falls back to listing unmatched rooms under Channels and DMs during cold start; RoomList always renders section headers with chevrons and empty-state/note handling; reducer/hooks/types/tests updated to support subscription update events and idempotency. ChangesSidebar bootstrap and bucket fetch
RoomList rendering, tests, and styling
RoomEventsContext logic and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`subscription.getRooms` exists as a subject (pkg/subject/subject.go)
and is wired on both mock-user-service and the real user-service, but
on the real backend it doesn't return the user's normal channel/DM
subscriptions — only `getCurrent` (no payload) does. Without a
populated `channelDmIds` Set, `useSidebarSections`'s strict
partitioning dropped every room from `listRooms` after a refresh,
leaving the sidebar blank.
Bootstrap now hits two endpoints:
- `getCurrent()` (no payload) — canonical full subscription list,
feeds `state.subscriptions` (source of truth for `useSubscription`)
and seeds `channelDmIds`. Channels and DMs are partitioned at
render time from this single list, by roomType.
- `getCurrent({ favorite: true })` — favorited IDs for the Favorite
section.
- `getApps()` — kept for the Apps section.
The empty-bucket fallback from the previous commit stays as a safety
net for any future RPC failure, but the normal path now actually
populates the buckets.
Also: gitignore `chat-frontend/junit.xml` (vitest CI reporter output).
Favorite / Apps / Channels and DMs each render with their collapsible header even when the bucket is empty, so the user always sees the sidebar structure. Adds a chevron (▾) in every header that rotates 90° via CSS when the section is collapsed, and an italic "No rooms" placeholder under an empty expanded section. Before: empty sections were hidden entirely (return null) — if you had no favorites you couldn't see the Favorite header at all, and a user with zero rooms saw a completely blank sidebar.
GITMateuszCharczuk
force-pushed
the
claude/sidebar-flat-list-fallback
branch
from
f1fbde5 to
343b855
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx (1)
47-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a semantic button for collapsible section headers.
The collapsible header is click-only on a
div, so keyboard users can’t reliably operate it. Make it abutton(or add full keyboard semantics) to avoid an accessibility blocker.Proposed fix
- <div + <button + type="button" className="room-list-section-header" onClick={() => toggle(section.key)} > <span className="room-list-section-chevron" aria-hidden="true">▾</span> {section.title} - </div> + </button>🤖 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 `@chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx` around lines 47 - 53, The collapsible header currently uses a click-only div (className "room-list-section-header") which is inaccessible; replace that div with a semantic <button> (or React button element) inside RoomList.jsx and attach the existing onClick handler (toggle(section.key)) to it, add aria-expanded={isExpanded} (compute from your section state) and aria-controls pointing to the collapsible region id, keep the chevron span (aria-hidden="true") and the section.title inside the button, and remove any custom keyboard handlers since a native button provides proper keyboard semantics.
🤖 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 `@chat-frontend/src/api/fetchSidebarBuckets/index.ts`:
- Around line 50-56: The fetchSidebarBuckets API currently calls request(...)
with two args; update the function signature for fetchSidebarBuckets to accept
an optional opts parameter and pass that opts as the third argument to all three
request calls (the requests using subjects userSubscriptionGetCurrent and
userSubscriptionGetApps) so each request uses request(subject, payload, opts)
consistently; ensure the opts parameter is threaded through even when undefined
to match API/test conventions.
- Around line 50-57: The Promise.all call can throw away the successful
canonical payload (allResp) if either the favorites or apps requests reject;
change the logic to run the three requests with Promise.allSettled (or await
each with try/catch) so that userSubscriptionGetCurrent(user.account,
user.siteId) is preserved even when the other two fail, then map rejected
results for the favorite and apps calls to safe empty SidebarBucketReply
fallbacks and continue to use allResp to seed state.subscriptions and
channelDmIds; update references to allResp, favResp, and appResp accordingly so
only the failed auxiliaries are ignored rather than aborting the whole
bootstrap.
---
Outside diff comments:
In `@chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx`:
- Around line 47-53: The collapsible header currently uses a click-only div
(className "room-list-section-header") which is inaccessible; replace that div
with a semantic <button> (or React button element) inside RoomList.jsx and
attach the existing onClick handler (toggle(section.key)) to it, add
aria-expanded={isExpanded} (compute from your section state) and aria-controls
pointing to the collapsible region id, keep the chevron span
(aria-hidden="true") and the section.title inside the button, and remove any
custom keyboard handlers since a native button provides proper keyboard
semantics.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 520dea54-8b82-4a4f-bd5d-c4c4cb2d0294
📒 Files selected for processing (8)
.gitignorechat-frontend/src/api/_transport/subjects.test.jschat-frontend/src/api/_transport/subjects.tschat-frontend/src/api/fetchSidebarBuckets/index.tschat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsxchat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.test.jsxchat-frontend/src/components/MainApp/Sidebar/RoomList/style.csschat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsx
💤 Files with no reviewable changes (1)
- chat-frontend/src/api/_transport/subjects.test.js
console.log each of the three subscription bootstrap RPCs (the two getCurrent calls and getApps) with their subject + response so we can verify what the real user-service returns. Remove once the live backend behaviour is confirmed.
GITMateuszCharczuk
changed the title
Pairs with the existing [sidebar-bootstrap] logs in fetchSidebarBuckets so we can diff the listRooms reply against the subscription RPC replies. A room that appears in listRooms but not in any of the three subscription buckets is silently dropped by useSidebarSections's strict partition (unless every bucket is empty, in which case the all-empty fallback catches it). The diff in console makes that gap visible. Remove once the live backend behaviour is verified.
…ction
Two changes addressing reviewer findings on the subscription pipeline:
1. fetchSidebarBuckets now uses Promise.allSettled so a single
subscription RPC failure no longer black-holes the entire sidebar
bootstrap (Reviewer 3 H1). Each rejection is logged with its subject
and the bucket degrades to empty rather than rejecting the whole
bucket fetch.
2. The favorite RPC (`getCurrent {favorite:true}`) is soft-disabled —
the call is commented out (not deleted, so re-enabling is one line)
and `useSidebarSections` now marks the Favorite section with a
`note: 'Favorites are not yet supported'`. RoomList renders the
note in place of room items / empty placeholder. Rationale:
`pkg/model.Subscription` has no Favorite field today (Reviewer 1
H2 / Reviewer 2 H2), so the filter is unenforceable end-to-end and
we'd be misleading the user by pretending it works.
The Favorite section header still renders (collapsible, with chevron)
for layout continuity. Clear the note + uncomment the RPC once the
favorite path lands on the backend.
Tests updated: the bucket-bootstrap test now asserts 3 calls (was 4),
the failure-tolerance test exercises a failing getApps with getCurrent
still succeeding, and the favorite-exclusivity test asserts the
section stays empty regardless of mocked favoriteIds.
…removed useRoomSubscriptions previously only branched on `added` and `removed`, so live `role_updated` events from room-worker (`handler.go:197`) were silently dropped. An admin demoting / promoting a user mid-session left the frontend with stale `state.subscriptions[r].roles` until reload, which manifested as the user still seeing owner-only UI affordances they no longer had access to. The handler now adds a catch-all branch: any subscription.update event that carries a `subscription.roomId` and isn't add/remove dispatches SUBSCRIPTION_UPSERTED. The reducer already partial-merges so a payload with only some fields won't drop lastSeenAt / hasMention / alert from the prior record. Forward-compatible with mute / favorite / mark-read the moment those backend paths start publishing events. Closes Reviewer 4 H2.
Three reviewer-flagged issues, all in the same blast radius: 1. Mirror SubscriptionUpdateEvent on the TS side (Reviewer 3 M2). subscribeToSubscriptionUpdates's callback was typed (event: unknown) => void — every consumer had to hand-narrow. New types: SubscriptionUpdateEvent + SubscriptionUpdateAction (union ∪ string for forward compat) in api/types.ts; callback re-typed to the parsed event. Re-exported from the api barrel. 2. ROOMS_LOADED race with BUCKETS_LOADED (Reviewer 4 M1). When the parallel cold-start fetches resolved bucket-first, the subsequent ROOMS_LOADED replaced state.summaries via toSummary's zero-init, wiping the server-canonical hasMention / subscriptionName that BUCKETS_LOADED had just seeded into state.subscriptions. The reducer now enriches each incoming summary via mergeSubscriptionIntoSummary using whatever's already in state.subscriptions — order-independent. 3. ROOM_ADDED early-return skipped bucket-Set maintenance (Reviewer 4 H3). If listRooms returned the room first (ROOMS_LOADED seeds it into summaries) and a live `subscription.update added` then arrived, the existing-summary check at the top of the case bailed out — leaving the room in summaries but in NO bucket Set. The strict partition in useSidebarSections then silently dropped it (until the all-empty fallback kicks in, which only catches the total-empty case). The case now always runs bucket maintenance; summary append is skipped when the row already exists. An object-identity short-circuit at the end keeps the true no-op (duplicate event, room already in correct bucket) from triggering re-renders. Reducer tests added for both races; the no-op idempotency contract is also pinned (`after2 === after1`).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
chat-frontend/src/api/fetchSidebarBuckets/index.ts (1)
65-67: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winThread
optsthrough NATS requests in this API operation.These calls still use the 2-argument request shape. Please add optional
optstofetchSidebarBucketsand passrequest(subject, payload, opts)consistently.As per coding guidelines, "Always pass
optsparameter through to NATS primitives in API operations, even when undefined — use three-argument shape for consistency with tests".🤖 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 `@chat-frontend/src/api/fetchSidebarBuckets/index.ts` around lines 65 - 67, Add an optional opts parameter to fetchSidebarBuckets and thread it into the NATS requests: change the two-argument calls to request(currentSubject, {}, opts) and request(appsSubject, {}, opts) (use the three-argument request(subject, payload, opts) shape) so both Promise.allSettled entries pass opts consistently; update the fetchSidebarBuckets signature to accept opts (optional) and forward it to any other request(...) invocations in that function.
🤖 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 `@chat-frontend/src/api/fetchSidebarBuckets/index.ts`:
- Around line 34-42: The docblock is out of sync: it claims getCurrent({
favorite: true }) is fetched for Favorites but that code path is disabled;
update the comment around the bootstrap description to reflect the actual
runtime flow (only getCurrent() is fetched as the canonical subscription list
which seeds state.subscriptions and channelDmIds, and getApps() is fetched for
the Apps section) and note that Favorites are derived from state.subscriptions
at render time (or that favorite-specific fetching is intentionally disabled)
instead of claiming getCurrent({ favorite: true }) is called.
- Around line 75-79: Remove the temporary debug that logs full subscription
payloads: delete the console.log('[sidebar-bootstrap]', label, result.value)
call in the fetch/sidebar bootstrap code so that label and result.value are not
emitted to logs; keep the return result.value behavior unchanged, or if you need
retained diagnostics replace it with a sanitized/logger-guarded message (e.g.,
log only label or a redacted summary) using the existing logger instead of raw
console.log.
In `@chat-frontend/src/api/listRooms/index.ts`:
- Around line 10-11: Update the listRooms API to accept an optional opts
parameter and pass it through to the NATS request call: add an opts parameter to
the function signature where roomsList(user.account) / subject is created, and
call request<ListRoomsResponse>(subject, {}, opts) (three-argument form) instead
of the current two-argument call; ensure you preserve the payload shape and
types and thread opts through even when undefined to satisfy tests and coding
guidelines.
- Around line 12-16: Remove the temporary debug logging in the listRooms API
handler: delete the console.log call that prints '[sidebar-bootstrap]' along
with subject and resp (located in chat-frontend/src/api/listRooms/index.ts
inside the listRooms function or exported handler) so the full rooms response
payload is not logged; ensure no other leftover debug console.* calls remain and
return resp unchanged.
---
Duplicate comments:
In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts`:
- Around line 65-67: Add an optional opts parameter to fetchSidebarBuckets and
thread it into the NATS requests: change the two-argument calls to
request(currentSubject, {}, opts) and request(appsSubject, {}, opts) (use the
three-argument request(subject, payload, opts) shape) so both Promise.allSettled
entries pass opts consistently; update the fetchSidebarBuckets signature to
accept opts (optional) and forward it to any other request(...) invocations in
that function.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 016dec88-e760-4f10-8ed1-0b9707beb82a
📒 Files selected for processing (13)
chat-frontend/src/api/fetchSidebarBuckets/index.tschat-frontend/src/api/index.tschat-frontend/src/api/listRooms/index.tschat-frontend/src/api/subscribeToSubscriptionUpdates/index.tschat-frontend/src/api/types.tschat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsxchat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.test.jsxchat-frontend/src/components/MainApp/Sidebar/RoomList/style.csschat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsxchat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsxchat-frontend/src/context/RoomEventsContext/reducer.jschat-frontend/src/context/RoomEventsContext/reducer.test.jschat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js
🚧 Files skipped from review as they are similar to previous changes (3)
- chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx
- chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.test.jsx
- chat-frontend/src/components/MainApp/Sidebar/RoomList/style.css
Two pre-test-env tweaks from the final reviewer round:
1. Favorite section's `note: 'Favorites are not yet supported'` is now
set only when `favorite.length === 0`. The favorite RPC is still
commented out so favoriteIds stays empty in production today, but
if anything ever lands a roomId in favoriteIds (re-enabled RPC,
live event), the section will render the rooms instead of silently
hiding them behind the unsupported-note. Belt-and-braces against a
reviewer-flagged HIGH that would have manifested as missing rooms
when the favorite path is eventually wired up.
2. TEMP `[sidebar-bootstrap]` console.log payloads compacted from full
reply objects to `{ count, roomIds }`. Accounts with many rooms
used to produce a wall of JSON per RPC; now each log line fits on
a screen and shows exactly the data needed to diff the
subscription RPCs against listRooms. Still TEMP — remove after the
live backend behaviour is verified.
mliu33
requested changes
May 18, 2026
Collaborator
mliu33
left a comment
mliu33
left a comment
There was a problem hiding this comment.
Not sure what we are trying to fix in this PR ?
mliu33
reviewed
May 18, 2026
Addresses three review comments from mliu33 on PR #188: - "These are actually correct in real user-service" (re: the three `subscription.get*` RPCs) - "This should not get dropped" (re: `userSubscriptionGetRooms` subject builder) - "listRooms should be dropped because all three get-user-subscriptions rpc will return room info in the response (from real user service)" **What changed** Sidebar bootstrap now runs three subscription RPCs (the original shape from PR #181, before this branch's `getCurrent`-canonical detour): - `subscription.getCurrent { favorite: true }` → favorites bucket - `subscription.getApps {}` → apps bucket - `subscription.getRooms {}` → channels/DMs bucket Each reply embeds room metadata (userCount, lastMsgAt, lastMsgId, siteId) inline on every subscription record — the real user-service joins room data into the response, so a separate `rooms.list` RPC is not needed. - Restored `userSubscriptionGetRooms` subject builder in `_transport/subjects.ts` (and its test). - `fetchSidebarBuckets` rewritten: 3 RPCs via Promise.allSettled (per-bucket degradation preserved), unions the subscriptions, and returns a derived `rooms[]` for the reducer. - `state.subscriptions` is the canonical source of truth for both per-room state AND for building summaries. - `Subscription` TS type extended with optional `userCount`, `lastMsgAt`, `lastMsgId` to mirror the wire shape the real user-service emits. - `BUCKETS_LOADED` reducer case now constructs summaries from `action.rooms` when provided (cold-start path); falls back to the enrich-existing-summaries behaviour when `rooms` is omitted (test fixtures, future partial-refresh paths). - `listRooms` op deleted: `src/api/listRooms/index.ts` removed, barrel export gone, `useRoomSubscriptions` no longer dispatches `ROOMS_LOADED` (the reducer action stays as a primitive for tests and future use). - Favorite section: "Favorites are not yet supported" note removed — favorites are real on the live backend, so the section populates from `getCurrent { favorite: true }`. **Tests** 47 files / 512 tests passing, typecheck clean. Test fixtures migrated from `rooms.list` mocks to `subscription.getRooms` returning room-bearing subscriptions via a new `roomToSub()` helper. The bootstrap-fires-three-RPCs and Promise.allSettled-degradation tests both refactored to the new wire shape.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
chat-frontend/src/api/fetchSidebarBuckets/index.ts (2)
76-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove temporary sidebar bootstrap debug logs before merge.
The cold-start
console.logstill emits per-user room identifiers. Keep failure logging if needed, but drop payload-style success logs.Suggested fix
- if (result.status === 'fulfilled') { - // TEMP DEBUG: compact summary so we can verify what each - // subscription RPC returns on cold start. Remove once verified. - console.log('[sidebar-bootstrap]', label, { - count: result.value.subscriptions.length, - roomIds: result.value.subscriptions.map((s) => s.roomId), - }) - return result.value - } + if (result.status === 'fulfilled') { + return result.value + }🤖 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 `@chat-frontend/src/api/fetchSidebarBuckets/index.ts` around lines 76 - 81, Remove the temporary debug console.log that prints per-user subscription payloads in the sidebar bootstrap path; locate the log call that uses '[sidebar-bootstrap]', label and result.value.subscriptions (and the mapped roomIds) inside the fetchSidebarBuckets flow and delete that statement, leaving only existing error/failure logging in place so success paths no longer emit payload-style logs.
61-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread
optsthrough this API operation and all NATS requests.This operation still uses a 2-arg
request(...)shape and does not accept{ args, opts }, which breaks the API operation contract used acrosssrc/api/*/index.ts.Suggested fix
-export async function fetchSidebarBuckets({ user, request }: Nats): Promise<SidebarBuckets> { +export interface FetchSidebarBucketsArgs {} +export interface FetchSidebarBucketsOpts { + timeout?: number +} + +export async function fetchSidebarBuckets( + { user, request }: Nats, + _args: FetchSidebarBucketsArgs = {}, + opts?: FetchSidebarBucketsOpts, +): Promise<SidebarBuckets> { @@ - request<SidebarBucketReply>(favSubject, { favorite: true }), - request<SidebarBucketReply>(appsSubject, {}), - request<SidebarBucketReply>(roomsSubject, {}), + request<SidebarBucketReply>(favSubject, { favorite: true }, opts), + request<SidebarBucketReply>(appsSubject, {}, opts), + request<SidebarBucketReply>(roomsSubject, {}, opts), ])As per coding guidelines, "
chat-frontend/src/api/*/index.tsAPI operations must have the(..., args, opts?)signature and always passoptsto NATS primitives using the three-argument shape."🤖 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 `@chat-frontend/src/api/fetchSidebarBuckets/index.ts` around lines 61 - 69, fetchSidebarBuckets currently uses the 2-arg request(...) shape and doesn't accept opts; update the function signature fetchSidebarBuckets({ user, request }: Nats, args?, opts?) to follow the project's API contract and thread opts into each NATS call: replace request<SidebarBucketReply>(favSubject, { favorite: true }) and the two other request calls with the three-argument form request<SidebarBucketReply>(favSubject, { favorite: true }, opts) (and likewise for appsSubject and roomsSubject) and ensure Promise.allSettled still collects those returned promises.
🤖 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.
Duplicate comments:
In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts`:
- Around line 76-81: Remove the temporary debug console.log that prints per-user
subscription payloads in the sidebar bootstrap path; locate the log call that
uses '[sidebar-bootstrap]', label and result.value.subscriptions (and the mapped
roomIds) inside the fetchSidebarBuckets flow and delete that statement, leaving
only existing error/failure logging in place so success paths no longer emit
payload-style logs.
- Around line 61-69: fetchSidebarBuckets currently uses the 2-arg request(...)
shape and doesn't accept opts; update the function signature
fetchSidebarBuckets({ user, request }: Nats, args?, opts?) to follow the
project's API contract and thread opts into each NATS call: replace
request<SidebarBucketReply>(favSubject, { favorite: true }) and the two other
request calls with the three-argument form
request<SidebarBucketReply>(favSubject, { favorite: true }, opts) (and likewise
for appsSubject and roomsSubject) and ensure Promise.allSettled still collects
those returned promises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e70d604-2ada-4247-9e9b-5d553137404c
📒 Files selected for processing (10)
chat-frontend/src/api/_transport/subjects.test.jschat-frontend/src/api/_transport/subjects.tschat-frontend/src/api/fetchSidebarBuckets/index.tschat-frontend/src/api/index.tschat-frontend/src/api/listRooms/index.tschat-frontend/src/api/types.tschat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsxchat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsxchat-frontend/src/context/RoomEventsContext/reducer.jschat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js
💤 Files with no reviewable changes (1)
- chat-frontend/src/api/listRooms/index.ts
✅ Files skipped from review due to trivial changes (2)
- chat-frontend/src/api/_transport/subjects.ts
- chat-frontend/src/api/_transport/subjects.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx
- chat-frontend/src/api/index.ts
mliu33
approved these changes
May 18, 2026
5 tasks
GITMateuszCharczuk
pushed a commit
that referenced
this pull request
May 18, 2026
Actionable findings I applied:
**Frontend**
- Strip TEMP [thread-debug] logs from fetchThreadMessages and
ThreadEventsContext.openThread — the bug is confirmed fixed in
test env, the diagnostics served their purpose.
- editMessage / deleteMessage now pass response type through the
generic `request<T>` (EditMessageResponse / DeleteMessageResponse)
per the project's API contract; bodies define the wire shape even
though the frontend swallows the response today.
- fanThreadReply in useRoomSubscriptions wraps the registered handler
invocation in try/catch — a downstream consumer exception no
longer breaks MESSAGE_RECEIVED dispatch or scheduleMarkActiveRead
in the same callback.
**Backend**
- All three new ERROR/WARN logs (history-service threads.go +
message-worker handler.go + store_cassandra.go) now include
`request_id` from the context, per the root CLAUDE.md mandate
("propagate via context.Context, include in all log lines"). Lets
the silent-stamp-miss log line correlate across handler → store →
history-service for incident triage.
Skipped findings (with rationale):
- Adding `opts` parameter to fetchThreadMessages / editMessage /
deleteMessage: `Nats.request` is a 2-arg primitive
(`(subject, data?) => Promise<T>`) by design — the 3-arg opts
contract applies to `requestWithAsyncResult` (async-job ops), not
request-based ops. Sibling request ops (getRoom, listRoomMembers,
markRoomRead, getUnreadCount, etc.) all use the 2-arg shape. Same
argument as PR #188 review.
- Making UpdateParentMessageThreadRoomID return an error on
IF EXISTS applied=false: the root cause that produced this miss
(frontend optimistic createdAt) is fixed in this PR; converting
the log to a hard error risks JetStream redelivery loops if any
other timestamp-mismatch sneaks in. Worth doing as a separate
defense-in-depth PR with retry/DLQ semantics designed alongside.
49 files / 525 frontend tests pass, go test ok on touched packages,
make lint clean.
This was referenced May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
On the real user-service: after PR #181 (three-section sidebar), users who previously saw 3 rooms in the sidebar now see 0 rooms on cold start. Creating a new room makes it appear live, but a refresh wipes it again. No red errors in the console — just a yellow
console.warnyou'd have to be looking for.Root cause
Two distinct problems stacked together:
1. Wrong endpoint for the Channels and DMs bucket
PR #181 wired
fetchSidebarBucketsto call three user-service RPCs in parallel:subscription.getRoomsdoes exist as a subject (pkg/subject/subject.go:492) and is wired on bothmock-user-serviceand the real user-service. But on the real backend, it doesn't return the user's normal channel/DM subscriptions — onlysubscription.getCurrent(no payload) does.2. Strict bucket partitioning silently drops un-bucketed rooms
useSidebarSectionspartitionsstate.summariesstrictly byfavoriteIds / appIds / channelDmIds:RoomList.jsxthen renders nothing for any section withrooms.length === 0. So even thoughlistRoomscorrectly populatedstate.summarieswith the user's 3 rooms, none of those IDs were in any bucket Set, and the entire sidebar rendered blank.The failure was swallowed by:
A
console.warn, not an error — doesn't trip ErrorBoundary or surface as red in DevTools.Fix (three commits)
6bbe9db— empty-bucket flat-list fallbackuseSidebarSections: when all three bucket Sets are empty, push every summary into Channels and DMs as a flat list. Safety net for any future RPC failure; preserves strict partitioning whenever any bucket has content.6556eb1— dropgetRooms, usegetCurrent()as canonical fetchfetchSidebarBucketsnow hits 2 endpoints (3 calls):getCurrent()(no payload) — canonical full subscription list, feeds bothstate.subscriptions(source of truth foruseSubscription) andchannelDmIdsgetCurrent({ favorite: true })— favorited IDs for the Favorite sectiongetApps()— kept for the Apps sectionThe
userSubscriptionGetRoomssubject builder is dropped fromapi/_transport/subjects.ts.Render-time partition stays
favorite > apps > channelDm, so a favorited app/channel still appears only under Favorite even though its ID is inchannelDmIdstoo.343b855— always render all three section headersFavorite / Apps / Channels and DMs each render with their collapsible header even when the bucket is empty, so the user always sees the sidebar structure. Adds a chevron (
▾) in every header that rotates 90° via CSS when collapsed, and an italic "No rooms" placeholder under an empty expanded section.1e65860— TEMP debug loggingconsole.logthe three subscription RPC replies (subject + response) on cold start to verify the real user-service behaviour. Remove before final merge.Why
listRoomsANDsubscription.getCurrentboth run on cold startThey carry different data:
listRooms(rooms.list) returns canonicalRoomrecords — userCount, lastMsgAt, canonical room name, type. Populatesstate.summaries.subscription.getCurrentreturns the user's per-room state — roles, hasMention, alert, lastSeenAt, hrInfo for DMs, subscription-local name override. Populatesstate.subscriptions(source of truth foruseSubscription) and the bucket Sets.mergeSubscriptionIntoSummaryjoins the two at render time. Subscriptions alone can't drive the sidebar because they don't carryuserCount/lastMsgAt;listRoomsalone can't carry per-user state like role / mention / DM friend metadata.Test plan
[sidebar-bootstrap]console logs show the expected replies506/506 tests passing, typecheck clean.
Summary by CodeRabbit
New Features
Bug Fixes