Implement thread panel UI with message actions and quote replies#183
Merged
GITMateuszCharczuk merged 129 commits intoMay 15, 2026
Conversation
Adds the design for a right-hand thread panel in chat-frontend: - Layout refactor into AppHeader / Sidebar / ChatPage / ThreadRightBar - New ThreadEventsContext + reducer mirroring RoomEventsContext - Hover MessageActions with Thread (open panel) and Reply (quote-reply via quotedParentMessageId, context-aware routing) - "Also send to channel" checkbox on thread input mapped to tshow flag - Main-feed filtering: thread replies hidden unless tshow=true, parent tcount/lastReplyAt bumped client-side regardless - Presentational MessageList / MessageInputForm with thin Room/Thread containers (Approach A) https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
…d events Backend already filters at the boundary: main-feed APIs (history, broadcast) only return top-level messages plus tshow:true thread replies. msg.thread and per-message lookups are the only paths that return arbitrary thread replies. Spec updates: - Drop client-side filtering of thread replies; main reducer appends unconditionally. - Parent tcount/lastReplyAt bumps from the single observed case (tshow:true reply arrives in main feed). - Live delivery of other users' thread replies deferred to a separate events ticket; ThreadEventsContext loses its room-event subscription and gains optimistic append (REPLY_SENT_LOCAL) for the user's own replies. - REPLY_RECEIVED action replaced with REPLY_SENT_LOCAL. - Tests and edge cases aligned with the new contract. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Captures all decisions from the design review round: - Hover actions = Thread + Reply (quote) + Edit + Delete; Edit inline (Enter saves, Esc cancels), Delete via confirm dialog. Available on main feed, parent inside thread, thread replies. - Quote-reply rendering: single shared QuotedBlock used both as the staging chip above the input (sender top, one-line content + right-justified ✕) and in-bubble above the reply content (shared message background, click-to-jump-to-original via focusMessageId). - Reply routing rule (4 cases). Disabled+tooltip when quoting a tshow:true thread reply from the main feed (gatekeeper boundary). - Thread panel: chronological list (no sticky parent), 380px wide, empty-state line "No replies yet — be the first to reply", input placeholder "Reply…". - "Also send to channel" checkbox: defaults off, resets after every successful send. - Failed sends: keep optimistic row tagged failed with ⟳ retry; new reducer actions REPLY_SEND_FAILED / REPLY_RETRIED. - Cross-context tshow pickup: thread context observes main MESSAGE_RECEIVED, appends matching tshow:true replies via TSHOW_OBSERVED. Main reducer's THREAD_REPLY_OBSERVED is deduped by per-parent observed-id Set. - Layout split locked: AppHeader (search/theme/user/logout), room- header (room name + Members + Leave), Sidebar (rooms + create). - Drop "lastReplyAt" from the badge (field not on Message payload). - Open questions cleared. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Three parallel reviewers found a critical backend gap and multiple frontend/UX gaps: - Backend reviewer: tshow is NOT wired through (no field on SendMessageRequest, gatekeeper drops it, broadcast-worker doesn't filter). The "Also send to channel" feature can't ship without backend work. - Frontend reviewer: mergeById not extracted; cross-context observer has no machinery in RoomEventsContext; msgThread/msgEdit/msgDelete subject builders missing. - UX reviewer: failed-reply dismiss missing; loading/error UI for thread panel undefined; deleted-quoted-message placeholder unspecified; keyboard accessibility entirely missing. Spec changes: - Drop "Also send to channel" / tshow checkbox from v1 entirely. Add as future work; needs paired backend ticket. - Add client-side filter: roomEventsReducer.MESSAGE_RECEIVED drops messages whose threadParentMessageId is set (broadcast-worker publishes them unconditionally today). - Drop TSHOW_OBSERVED action and cross-context observer. Replace with OWN_THREAD_REPLY_SENT — only the sender's own reply bumps parent tcount in real time; other users' replies update on next history reload. - Add REPLY_DISMISSED action + ✕ dismiss button on failed rows. - Add HISTORY_LOADING action; explicit loading/error/empty states in ThreadRightBar; "Try again" on history failure. - New shared util: src/lib/messageBuffer.js (appendBounded + mergeById preserving _local / _status markers). - New subject builders: msgThread, msgEdit, msgDelete in subjects.js (edit/delete exist server-side but were missing on the frontend). - New "Keyboard & accessibility" section: tabindex, :focus-within, Esc behavior, focus management, aria-live, aria-labels. - QuotedBlock: deleted-snapshot placeholder; plain-text only (no markdown); chip stays during in-flight publish. - Cross-room main-input quote: cleared on room switch. - Remove obsolete "tshow reply in main feed" routing case. Spec now decision-complete: no open questions, three small Red-phase verification items only. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Traced the actual pipeline: client → gatekeeper → MESSAGES_CANONICAL → broadcast-worker (direct consumer, no FANOUT despite README). broadcast-worker has no thread-parent check in publishChannelEvent / publishDMEvents. Frontend filter stays as the v1 workaround; the proper server-side fix is paired with the tshow ticket. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 1 of the implementation plan: messageBuffer extraction, roomEventsReducer refactor to use it, and new subject builders (msgThread / msgEdit / msgDelete). https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 2: extract AppHeader, Sidebar, MainApp; slim ChatPage to middle column with its own room-header strip; rename top-level CSS selectors. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 3: extract presentational components (QuotedBlock, MessageActions, MessageRow, MessageList, MessageInputForm) plus Room-scoped containers (RoomMessageArea, RoomMessageInput); wire ChatPage to use the new containers; delete old MessageArea/Input. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 4: Red-phase verification of msg.edit/msg.delete payloads; MESSAGE_EDITED_LOCAL / MESSAGE_DELETED_LOCAL reducer actions; Edit and Delete buttons in MessageActions (own messages only); inline edit mode on MessageRow; DeleteConfirmDialog; RPC wiring in RoomMessageArea; expose dispatch on RoomEventsContext. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 5: lift quotedTarget to ChatPage, wire Reply icon → chip → publish payload; clear on room switch; E2E test for the full chain; E2E test for click-to-jump on in-bubble QuotedBlock. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 6: threadEventsReducer (OPEN/CLOSE/HISTORY/REPLY actions including optimistic _local + failed/retried/dismissed); ThreadEventsContext with race-discard openThread, sendReply, retryReply, dismissReply; mount the provider under RoomEventsProvider in App. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 7: ThreadMessageInput, ThreadMessageArea (renders parent + replies, surfaces retry/dismiss), MessageList parent-row context override, MessageRow failed-row UI (⟳ + ✕), ThreadRightBar shell, MainApp mount + ChatPage wiring of the Thread hover icon, and CSS for the panel layout. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 8: filter thread replies from main feed live broadcast; OWN_THREAD_REPLY_SENT action for parent tcount bump; cross-dispatch from ThreadEventsContext on send success; reply-count badge on MessageRow; close thread on room switch. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Chapter 9: InRoomSearch ↔ ThreadRightBar mutual exclusion; Esc closes the thread panel; focus management on open/close; logout reset verification; final integration smoke checklist; spec↔plan coverage table. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Reviewer noted that .chat-main has descendant rules at lines 536/543 that the original rename instruction missed; explicit step added to catch these. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Task 4.6 used useRoomEvents().dispatch before Task 4.7 wired it. Moved the prerequisite to Task 4.1.5 so the impl path works end-to-end in dev, not just in test mocks. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
The marker was added beyond spec for standard UX, but had no test. Two tests added: marker renders when editedAt is set, omitted when not. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
The original wording "append a publish + dispatch mock" was ambiguous; vitest hoists both vi.mock calls and the second silently wins. Explicit REPLACE instruction added with a re-run gate to catch earlier-test breakage immediately. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
The action existed in the reducer but had no test. Two tests added: flips historyLoading=true for active parent; no-op for non-active. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Verified at chat-frontend/src/context/NatsContext.jsx:74-78: publish
returns undefined and throws synchronously if not connected. Plan
previously awaited it and used mockResolvedValue/mockRejectedValue,
which would never trigger the failure path.
Rewired:
- ThreadEventsContext.publishReply: sync, throws if no active thread
- ThreadEventsContext.sendReply / retryReply: try/catch around sync
publish, dispatch REPLY_SEND_FAILED on sync throw
- Tests: replace mockResolvedValue/mockRejectedValue with
mockImplementation(() => { throw new Error(...) }) for failure
scenarios; remove awaits where not needed
- ThreadMessageInput.handleSubmit: sync; quote chip clears after the
sync sendReply call (failures will surface via the _local row's
_status='failed', not via promise rejection)
Caveat documented: no broker-ack signal in v1, so failures rejected
asynchronously by the broker are invisible to the client until the
thread is reopened.
https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Reviewer caught vi.mock('./MessageRow', …) inside an it() block in
Ch.7 task 7.3 step 2. vitest hoists vi.mock to module scope so the
inner call silently no-ops, leaving the test running against the
real MessageRow (or the prior top-of-file mock from Task 3.4).
Rewrote step 2 to extend the existing top-level mock and append a
clean test that asserts on data-context attributes. Added a re-run
gate to catch breakage of prior tests in the same file.
https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Reviewer caught that Ch.4 wired Edit/Delete only in RoomMessageArea (main feed); Ch.7's ThreadMessageArea didn't forward onEdit / onDelete to MessageList. Spec requires them on thread replies and on the parent rendered inside the panel. Two new tasks inserted between 7.2 and 7.3: - Task 7.2.5: REPLY_EDITED_LOCAL / REPLY_DELETED_LOCAL actions on threadEventsReducer, with full TDD test coverage. - Task 7.2.6: ThreadMessageArea now owns editingMessageId + pendingDelete, publishes msg.edit / msg.delete against the active parent's room/site, and routes the optimistic dispatch correctly: - parent edits → roomDispatch MESSAGE_EDITED_LOCAL - parent deletes → roomDispatch MESSAGE_DELETED_LOCAL - reply edits → threadDispatch REPLY_EDITED_LOCAL - reply deletes → threadDispatch REPLY_DELETED_LOCAL ThreadEventsContext now exposes `dispatch` on its value so the container can dispatch reducer actions directly. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
publish() moved from lines 74-78 to 129-133 after upstream NatsContext additions for asyncJob protocol; behavior unchanged (still sync void, still throws if not connected) so the underlying B1 fix stands. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
…ision Post-rebase index.css now has TWO inner-layout helpers (.chat-main-with-side-panel, .chat-main-content) that share the prefix with .chat-main but must NOT be renamed. Step 2 of Ch.2 task 2.7 now warns against naive sed s/.chat-main/.chat-page/g and prescribes a word-boundary-aware grep gate. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Upstream PR #177 added MessageActionMenu (kebab) on each message row to expose read receipts. Our plan's MessageRow lifted only sender/time/content/MessageActions and would have silently dropped the kebab. Adjustments: - Ch.3 task 3.3 prose: clarify MessageActionMenu (kebab) and MessageActions (Reply/Edit/Delete hover) are separate components that render side-by-side. Document they have different visibility rules. - MessageRow signature now takes `room` and renders <MessageActionMenu room={room} message={message}/> alongside <MessageActions/>. - MessageRow tests mock '../MessageActionMenu' (it depends on NatsContext) and assert the kebab mounts with the right props. Test count bumps 5 -> 6. - MessageList plumbs `room` through to each row. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
…ssage curry Three rebase-driven regressions in the plan: 1. Task 2.5's slimmed ChatPage was about to drop the upstream RoomMembersBadge (PR #178) and revert to a manual "{userCount} members" + Members button. Restored: - Imports RoomMembersBadge + roomDisplayName. - membersRefreshKey state + bump-on-dialog-close pattern. - Header strip uses RoomMembersBadge with refreshKey; LeaveRoomButton remains. - Title now uses roomDisplayName(selectedRoom) so DMs render. - Interim MessageArea continues to receive onOpenMembers + membersRefreshKey until Task 3.6 swaps in RoomMessageArea. 2. Task 3.6's RoomMessageArea was passing two args to jumpToMessage. The per-room useRoomEvents(roomId).jumpToMessage is curried — takes only (messageId). Fixed call site and added a clarifying note that the two-arg form lives on useRoomSummaries() for cross-room jumps. 3. RoomMessageArea now forwards `room` to MessageList so the per-row read-receipt kebab can construct its NATS subject (the A3 plumbing terminus). https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
Task 1.1's implementer flagged a contradiction: the cap test expected result[0] = new-2, which forced an incoming-reversal hack that contradicts both the "incoming first, then existing" rule and the caller convention (incoming = older history page, existing = newer live tail). The test was buggy, not the impl. Resolving by: 1. Restoring mergeById to its natural shape (no reverse): build merged = [...incoming, ...existing-minus-incoming], then cap from the front so the oldest entries (= incoming history page beyond the window) are sliced off. 2. Renaming the cap test's incoming ids new-1/new-2 -> hist-1/ hist-2 so it's obvious those are the older entries about to be evicted; updating the assertions to expect e0 at front and e199 at end (the natural outcome). 3. Mirroring both fixes back into the plan markdown so the trap doesn't recur in any later regenerated test/impl. All 7 messageBuffer tests still pass. https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL
…one row
Two issues in the Add-members tab of ManageMembersDialog:
1. The share-history checkbox sat on its own line above the label text.
The global ".dialog input { display: block; width: 100% }" rule was
also stretching <input type="checkbox"> to full width, forcing the
text node to wrap below. Scoped the block rule to
":not([type='checkbox']):not([type='radio'])" so checkboxes keep
their natural inline sizing.
2. Close and Add stacked vertically because Close lived in
ManageMembersDialog's outer footer while Add lived in
AddMembersForm's own .dialog-actions. Moved Close into the Add tab's
action row (passed via onClose prop). The outer footer now renders
only on the Members tab, where there's no inner Add row. The shared
.dialog-actions row uses "margin-right: auto" on .dialog-cancel to
push the primary action right while keeping cancel left — Close on
the left, Add on the right, in one line.
…ructure
Each component now lives in its own folder. The component file keeps
its descriptive name (e.g. MainApp/MainApp.jsx) and the folder has an
index.jsx that re-exports the default — so existing import paths like
`import MainApp from './MainApp'` continue to resolve through the
folder/index.jsx pattern.
Component tree mirrors the runtime tree:
components/
MainApp/
MainApp.jsx
index.jsx
AppHeader/
SearchBar/
ThemeToggle/
Sidebar/
RoomList/
CreateRoomDialog/
ChatPage/ (moved from src/pages/)
RoomMessageArea/
RoomMessageInput/
InRoomSearch/
LeaveRoomButton/
RoomMembersBadge/
ManageMembersDialog/
AddMembersForm/
MemberRoster/
MemberPicker/
ThreadRightBar/
ThreadMessageArea/
ThreadMessageInput/
SearchResultsPane/ (moved from src/pages/)
shared/
MessageList/
MessageRow/
MessageActions/
MessageActionMenu/
MessageInputForm/
QuotedBlock/
Modal/
DeleteConfirmDialog/
TextInputDialog/
components/shared/ holds anything used by more than one branch of the
tree (MessageList is used by both RoomMessageArea and ThreadMessageArea;
MessageInputForm by both message inputs; Modal by DeleteConfirmDialog +
TextInputDialog + CreateRoomDialog; QuotedBlock by MessageRow +
MessageInputForm).
src/pages/ retains only LoginPage and OidcCallback — they're pre-auth,
not children of MainApp.
All relative imports + vi.mock paths regenerated to match the new
depths. 416/416 tests pass, production build clean.
CSS still lives in src/styles/index.css for this commit; the per-component
style split lands in the next commit.
Each component folder now owns its own style.css with the selectors
scoped to that component. The component's main file imports it via
`import './style.css'`, so Vite bundles the rules alongside the
component without any further wiring.
What stayed in src/styles/index.css (truly app-wide):
- CSS reset (*, html, body)
- typography defaults on body
- global :focus-visible ring on interactive elements
- .btn family (.btn, .btn-primary, .btn-ghost, .btn-danger, .btn-icon)
- Themed thin scrollbars (.scrollbar-thin + class enumeration)
- Dialog primitives (.dialog-overlay, .dialog, .dialog-actions,
.dialog-error/success/checkbox/cancel, base input/select/textarea
inside .dialog) — shared across CreateRoomDialog, ManageMembers,
DeleteConfirmDialog, TextInputDialog, …
- flash-jump @Keyframes (the .message-row.flash-jump rule that
consumes it lives in MessageRow/style.css)
- Legacy .message-input chrome (still referenced by holdover code)
Per-component style.css files:
- components/MainApp/* shell + sidebar/header/chat/thread
- components/MainApp/AppHeader/* header, SearchBar, ThemeToggle
- components/MainApp/Sidebar/* sidebar shell, RoomList
- components/MainApp/ChatPage/* chat header + tree
RoomMessageArea, RoomMembersBadge,
InRoomSearch, ManageMembersDialog
+ MemberPicker + MemberRoster
- components/MainApp/ThreadRightBar/* panel + ThreadMessageArea
- components/shared/MessageList/* feed shell + MessageRow + Actions + Menu
- components/shared/MessageInputForm/* the canonical input
- components/shared/QuotedBlock/* quote chip + bubble variants
- pages/LoginPage/* login chrome
Folder moves:
- pages/LoginPage.jsx → pages/LoginPage/LoginPage.jsx + index.jsx
- pages/OidcCallback.jsx → pages/OidcCallback/OidcCallback.jsx + index.jsx
Same pattern as components/ — keeps the per-folder rule consistent.
Numbers:
- src/styles/index.css: 1682 → 367 lines (78% reduction)
- 22 per-component style.css files, ranging from 5 to 215 lines each
- Production build CSS: 40.9KB (unchanged total, just split)
- 416/416 tests pass, build clean
Room events arrive as messages with a `type` field set (and the original
Content empty for most types). Before this commit MessageRow rendered
them blindly — an avatar + sender + an empty bubble — so the user saw
"Alice [empty]" after adding/removing members.
Added a SystemMessage component under shared/MessageList/ that handles
typed events:
- `members_added` → "Alice added bob" (≤3 names) or
"Alice added 5 members" (larger count) or
"Alice added members from 2 orgs and 1 channel"
(orgs/channels only)
- `room_created` → "Alice created #general"
- any other type → falls back to message.content, or "Room event"
The component decodes `sysMsgData` (Go's base64-encoded JSON for `[]byte`)
defensively — a malformed payload is treated as missing data, never throws.
MessageList now branches on `msg.type` before delegating to MessageRow,
so the typed messages render as a centered, muted strip with horizontal
divider lines (visually distinct from user conversation) — keeping
MessageRow itself focused on regular chat.
9 new tests cover the rendering + decoding + fallback paths.
… of components
Components were building NATS subjects inline (\`memberAdd(user.account,
room.id, room.siteId)\`) and calling \`nc.request(...)\` / \`publish(...)\`
themselves — every dialog and message area had to know wire-level
details. New \`src/api/\` folder wraps each backend operation in its own
module so callers just say \`await addMembers(nats, args)\`.
Layout (folder-per-operation, mirroring the components/ convention):
src/api/
├── _transport/ (internal — subjects.js + asyncJob.js)
├── addMembers/ (memberAdd, two-phase)
├── createRoom/ (roomCreate, two-phase)
├── deleteMessage/ (msgDelete, JetStream publish)
├── editMessage/ (msgEdit, JetStream publish)
├── fetchMessageHistory/
├── fetchReadReceipt/
├── fetchSurroundingMessages/
├── fetchThreadMessages/
├── getRoom/
├── leaveRoom/ (sync member.remove for self)
├── listOrgMembers/
├── listRoomMembers/
├── listRooms/
├── removeMember/ (member.remove, two-phase; for orgs OR individuals)
├── searchMessages/
├── searchRooms/
├── sendMessage/ (msg.send, JetStream publish)
├── subscribeToRoomEvents/ (channel event subscription)
├── subscribeToRoomMetadataUpdates/
├── subscribeToSubscriptionUpdates/
├── subscribeToUserRoomEvents/ (DM event subscription)
├── updateMemberRole/
└── index.js (barrel — \`import { addMembers } from '@/api'\`)
Every operation takes \`(nats, args, opts?)\` where \`nats\` is the value
from \`useNats()\`. The function destructures the primitives it needs
(\`user\`, \`request\`, \`publish\`, \`requestWithAsyncResult\`, \`subscribe\`)
and hides the subject + payload shape inside the api module.
Transport choice (NATS req/reply vs JetStream publish vs two-phase
async-job) is an implementation detail of each operation. No top-level
\`api/nats/\` vs \`api/jetstream/\` split — that would leak the transport
into every call site.
Subject builders + the two-phase \`requestWithAsyncResult\` helper moved
from \`lib/\` into \`api/_transport/\` (the underscore signals "internal
to this layer"). \`lib/\` now holds only pure utilities + state machines
+ hooks.
Components rewired (12): SearchBar, RoomMessageInput, RoomMembersBadge,
LeaveRoomButton, InRoomSearch, RoomMessageArea, ThreadMessageArea,
MessageActionMenu, SearchResultsPane, CreateRoomDialog, AddMembersForm,
MemberRoster, MemberPicker. Contexts rewired: NatsContext (asyncJob
import path), RoomEventsContext, ThreadEventsContext.
MemberRoster's \`runAction\` shed its subject + payload args — it now
takes a thunk that returns the api operation's promise, so the busy /
error / refetch plumbing is shared without coupling to subjects.
425/425 tests pass, production build clean.
Round 2 (next commit): move reducers next to their contexts, hooks to
src/hooks/.
…s to src/hooks/ Round 2 of the lib/ cleanup. The api/ extraction (7804f2e) pulled backend-talking code out; this commit moves the rest into homes that reflect what each module actually does: - **Reducers next to their contexts.** \`roomEventsReducer\` is consumed only by \`RoomEventsContext\`; same for the thread one. Moving them inside the context folder keeps the state machine + its consumer side by side, so adding an action means touching one folder, not two. context/ ├── NatsContext/{NatsContext.jsx, index.jsx} ├── RoomEventsContext/ │ ├── RoomEventsContext.jsx │ ├── reducer.js │ ├── reducer.test.js │ ├── RoomEventsContext.test.jsx │ └── index.jsx ├── ThreadEventsContext/{ThreadEventsContext.jsx, reducer.js, …} └── ThemeContext/{ThemeContext.jsx, index.jsx, …} Each context folder gets an \`index.jsx\` re-exporting everything from its main file — \`import { useNats } from '../context/NatsContext'\` keeps resolving through the folder. - **Hooks to src/hooks/.** \`useDebouncedSearch\` and \`useHoverWithDelay\` are framework primitives, not domain utilities — they belong with the other React-shaped things, not in lib/. After both rounds lib/ holds only pure, domain-neutral utilities: constants, idgen, messageBuffer, normalizeMessage, oidcClient, roomFormat, runtimeConfig. Backend calls live in api/, hooks live in hooks/, state machines live next to their contexts. 425/425 tests pass, production build clean.
Wire up a `@/` → `<repo>/src/` alias in vite.config.js + vitest.config.js
and document it in a new jsconfig.json so IDEs / TS tooling resolve it
identically.
Rewrote 149 imports across the codebase: any spec starting with `../../`
(two or more parent-hops) is now `@/<path>`. Single-up `../sibling` and
same-folder `./child` stay relative — those read fine and keep local
cohesion.
Before:
import { useNats } from '../../../../../../context/NatsContext'
import { fetchReadReceipt } from '../../../../../../api'
After:
import { useNats } from '@/context/NatsContext'
import { fetchReadReceipt } from '@/api'
The deep folder-per-component structure made this hurt more than it
would in a flatter tree; payoff is largest in the message-row sub-tree
and the api/contexts callers.
425/425 tests pass, production build clean.
Before this commit, a single render-phase exception anywhere in the
tree took down the whole app (React unmounts to a blank document).
For a chat client that stays open for hours, that's the wrong default.
Added \`@/components/shared/ErrorBoundary\` and wrapped the entire app
(including NatsProvider) at App.jsx. On any render-time error in a
descendant, the boundary shows a centered "Something went wrong" card
with the error message and a Reload button.
What the boundary catches:
- render-time exceptions in any descendant
- exceptions thrown from constructors / lifecycle methods
What it does NOT catch (React's own rules — caller still needs window
.onerror / unhandledrejection for these):
- exceptions in event handlers
- async work outside the render phase
- SSR errors (n/a — this is a SPA)
Custom fallback supported as either an element or a render-prop
\`({error, reload}) => ReactNode\` so future surfaces (e.g. a per-pane
boundary inside the chat area) can show inline recovery instead of
the full-screen card.
6 tests cover happy path, default fallback, custom fallback element,
render-prop fallback, the Reload button, and the console.error log
emit for ops triage. 431/431 tests pass overall.
…ventsProvider The provider's mount effect was 90+ lines doing four jobs: open the DM event subscription, lazily open per-channel subscriptions, handle subscription.update + room.metadata.update events, and fire the initial listRooms() fetch. Plus the cleanup logic. It was the longest, most-context-needed hook in the codebase. Pulled it into \`useRoomSubscriptions(nats, dispatch, generationRef)\` under the same folder. The provider now reads as: const [state, dispatch] = useReducer(roomEventsReducer, initialState) useRoomSubscriptions(nats, dispatch, generationRef) // …loadHistory / jumpToMessage / resetToLiveTail callbacks… The `generationRef` is threaded through (not owned by the hook) because the provider's `loadHistory` / `jumpToMessage` callbacks also read it to detect stale-cycle dispatches — a fetch that started before a reconnect must not write into the post-reconnect state. The hook bumps the counter on every connection cycle; the provider reads it inside its async callbacks. This is a pure-mechanical split: zero behavioural change. 431/431 tests pass (including the 30+ existing RoomEventsContext tests, which exercise the same code path through the public surface). RoomEventsContext.jsx is now ~140 lines and reads as wiring; the subscription machinery is its own concern in its own file. Future changes — adding a new subscription, swapping the initial fetch, etc. — have one obvious home.
Wrapped five conditionally-rendered surfaces in \`React.lazy\` so they
ship as separate chunks loaded on first open instead of bloating the
initial bundle:
- SearchResultsPane (mounted when the user enters a search)
- ThreadRightBar (mounted when a thread is opened)
- ManageMembersDialog (mounted when "Members" button clicked)
- InRoomSearch (mounted on Ctrl-F)
- CreateRoomDialog (mounted when "+ Create Room" clicked)
MemberPicker auto-split as a downstream dep of two of those.
Each call site is now:
{condition && (
<Suspense fallback={null}>
<LazyComponent ... />
</Suspense>
)}
Fallback is \`null\` — these are overlays / side panels, so a brief
empty render during chunk load reads better than a flashed spinner.
Bundle effect (vite production build):
- main bundle: 573.87 KB → 558.68 KB (-15 KB / ~3%)
- lazy chunks: 23 KB across 6 files, loaded on demand
- 5 split CSS chunks for the per-component styles those panels import
The savings are modest because the main bundle is dominated by
\`nats.ws\` + \`nkeys.js\` + React + \`oidc-client-ts\` — getting a bigger
cut would require deferring the NATS connection until post-login,
which is a much deeper change. This commit is the easy first slice.
Tests: 6 assertions had to switch from \`getBy*\` to \`findBy*\` because
the lazy chunks resolve asynchronously after the opening event.
431/431 tests pass.
Converted the entire \`src/api/\` tree from .js to .ts. Components,
contexts, lib/, hooks/, and tests stay in JavaScript — \`allowJs: true\`
in tsconfig means they continue to work unchanged, and they pick up
TS type-inference automatically wherever they import an api/ function.
What gets typed:
- api/_transport/subjects.ts (every subject builder)
- api/_transport/asyncJob.ts (two-phase request helper with
a discriminated-union AsyncJobError
and AsyncJobErrorKind enum)
- api/types.ts (shared shapes mirroring pkg/model:
User, Room, RoomType, Message,
Participant, ChannelRef,
HistoryConfig, HistoryMode,
MemberEntry, Reader, plus the
Nats context shape consumed by
every operation)
- api/<op>/index.ts × 22 (per-operation Args + Response
interfaces, typed signatures)
Each operation now reads like:
export interface AddMembersArgs {
roomId: string
siteId: string
users?: string[]
orgs?: string[]
channels?: ChannelRef[]
history?: HistoryConfig
}
export async function addMembers(
{ user, requestWithAsyncResult }: Nats,
args: AddMembersArgs,
opts?: AsyncJobOptions,
): Promise<AsyncJobResult> { … }
Callers from .jsx files see autocompletion + parameter-type checking
even without converting the call site to TS.
Tooling:
- Added \`typescript\` (6.0.3) to devDependencies
- Added \`tsconfig.json\` with strict mode, allowJs, bundler resolution,
and the \`@/*\` path alias matched to vite.config.js
- Dropped jsconfig.json — tsconfig supersedes it (TS Language Server
reads tsconfig in JS projects too)
- Added \`npm run typecheck\` script (tsc --noEmit) so CI can gate
on type errors
Verified:
- npm run typecheck → 0 errors
- npm run test → 431/431 pass
- npm run build → clean
This is the foundation. Migrating components / contexts / hooks to .ts
is a separate, gradual effort — each file can flip independently and
the rest of the codebase keeps building. The api/ layer was the right
first target because (a) the wire contracts are stable, (b) callers
benefit from type-checked args without changing themselves, and (c)
the existing JSDoc was already drifting from reality.
…nto api
Addresses the cross-cutting bug both the architecture and TypeScript
reviewers flagged: the api/ layer was lying about its return shapes
and leaking wire-shape adapters to its callers.
**Type drift vs pkg/model fixed:**
- `Room` added lastMentionAllAt/minUserLastSeenAt/restricted;
made appCount + lastMsgId required (not omitempty in Go)
- `Participant` added siteId
- `User` added sectId/sectName/employeeId
- `MemberEntry` added rid + ts (real model.RoomMember wraps this way)
- `searchRooms.scope` 'all' | 'channel' | 'dm' | 'app'
(was 'all' | 'subscribed' — 'subscribed' is
not a real value; server would 400)
- `SearchRoomHit` added userAccount + joinedAt; total made required
- `SearchMessageHit` dropped hallucinated roomName; added siteId,
userId, threadParentMessageId, threadParentMessageCreatedAt
- `fetchMessageHistory.before` typed `number` (UTC ms), was wrongly `string`
- `fetchMessageHistory` response gained `minUserLastSeenAt?`
- `fetchThreadMessages` response gained `nextCursor?`, `hasNext`
- `listOrgMembers.OrgMember.siteId` required
- `getRoom` returns `Room` (not `Room | null`) — server throws on missing
- new `HistoryMessage` type for cassandra shape, distinct from
broadcast `Message`. The two were conflated previously.
**Generic `Nats.request<T>`** — every op now passes its response type
through `request<T>(subject, payload)`. Was `Promise<any>` before,
which swallowed every shape mismatch. The wire-shape bug below would
have been caught at compile time if this had been correct.
**Wire-shape leak fixed** (Architecture #4 + TS review HIGH):
`lib/normalizeMessage.js` → `api/_transport/normalizeMessage.ts`.
The three fetch ops (fetchMessageHistory, fetchSurroundingMessages,
fetchThreadMessages) now run normalisation themselves and return
`Message[]` (broadcast shape). RoomEventsContext + ThreadEventsContext
drop their `normalizeHistoricalMessages` calls — the api hides the
cassandra `messageId`/`msg` wire shape entirely.
**asyncJob.ts type quality:**
- `AsyncJobError` is now a real `class` (was an interface). Callers
can `if (err instanceof AsyncJobError) …` instead of casting.
- `ASYNC_JOB_ERROR_KINDS` typed `as const` — restores literal-key
autocomplete that the prior `Record<string,...>` had erased.
- `requestWithAsyncResult` parameters typed against `NatsConnection`
+ `Subscription` from `nats.ws` (was `nc: any`).
- `(err as Error).message` casts replaced with `err instanceof Error`
narrowing — caught is `unknown`, not `Error`, in strict TS.
- `AsyncJobResult.async` typed `A | null` (was `A`) — type drift with
the implementation's actual return shape.
- `Envelope.data` is `unknown` (was `any`); narrowed at the consumer.
**`SubscriptionCallback`** typed `(event: unknown) => void` — was
`(event: any) => void`, which defeated type-checking for every subscriber.
Per-op return types now flow through. `npm run typecheck` clean,
46/46 test files / 431/431 tests pass, production build clean.
… nats flicker
React review HIGH: the subscription effect's dep array was
\`[user, nats, dispatch, generationRef]\`. NatsContext's memoised value
flips identity whenever \`connected\` or \`error\` changes — and \`error\`
is set inside \`nc.closed().then(err => setError(...))\` on every
transient disconnect. That meant any flicker tore down all four
subscriptions, ran \`dispatch({type: 'RESET'})\`, and re-opened — wiping
\`roomState\` mid-session.
Fix: depend on \`[user, dispatch]\` only. \`user\` is stable for a login
session (only \`connectToNats\` writes it; the actual reconnect path
sets a new \`user\` object). Stash \`nats\` in a ref kept current on
every render so long-lived subscription callbacks (the subUpdate's
getRoom call) read the latest \`nc\` after a reconnect without forcing
the effect to re-run.
Also resolves architecture review HIGH #8 — the \`generationRef\` was
shared mutable state across a module boundary, owned by the provider
but mutated by the hook. Now owned entirely inside the hook; the
provider reaches it via a returned \`currentGeneration()\` getter:
const { currentGeneration } = useRoomSubscriptions(nats, dispatch)
...
const gen = currentGeneration()
if (currentGeneration() === gen) dispatch(...)
The getter is wrapped in \`useMemo([])\` so its identity is stable for
the provider's downstream \`useCallback\` / \`useMemo\` deps.
431/431 tests pass, npm run typecheck clean, build clean.
… ceremony Architecture review HIGH #1: \`_transport/\` was advertised as internal but three components (\`MemberRoster\`, \`AddMembersForm\`, \`CreateRoomDialog\`) reached past it for \`formatAsyncJobError\`, and \`MessageActionMenu.test.jsx\` reached past it for a subject builder. Fixed by re-exporting from \`@/api\`: export { AsyncJobError, ASYNC_JOB_ERROR_KINDS, formatAsyncJobError, } from './_transport/asyncJob' export type { AsyncJobErrorKind } from './_transport/asyncJob' All three components now \`import { formatAsyncJobError } from '@/api'\`. The MessageActionMenu test was hard-cased to a literal subject string — test concerns shouldn't pierce the internal layer either, and a hardcoded constant flags that this is a wire contract worth keeping in lock-step. Architecture review HIGH #2: \`opts ? fn(s,p,opts) : fn(s,p)\` ceremony in \`addMembers\`, \`createRoom\`, \`removeMember\`, \`updateMemberRole\`. The ternary was there only to make \`toHaveBeenCalledWith(subj, payload)\` test assertions pass cleanly; \`requestWithAsyncResult\` itself defaults \`opts = {}\`. Dropped the ternary, updated 5 test files to assert 3-arg shape (\`toHaveBeenCalledWith(subj, payload, undefined)\` when no opts, or with the actual opts when supplied). Each op now reads: return requestWithAsyncResult(subject, payload, opts) 431/431 tests pass, npm run typecheck clean.
Four small fixes from the React review's MEDIUM tier:
**MessageList routing narrower** (React review MEDIUM):
\`MessageList.jsx\` previously routed any message with a truthy \`type\`
field to \`SystemMessage\`. A future broadcast rename (e.g. \`type:'text'\`
on regular user messages) would silently disappear every message into
the system-row renderer. Switched the discriminator to
\`msg.sysMsgData != null\` — the actual marker that comes with
\`room_created\` / \`members_added\` and any future system event.
SystemMessage's own internal type-switch is unchanged.
**ErrorBoundary soft recovery** (React review MEDIUM):
The default fallback now offers two paths:
- Try Again — \`this.setState({ error: null })\`, remounts children,
preserves NATS subs / optimistic state / scroll positions.
- Reload — \`window.location.reload()\`, the nuclear option.
The render-prop API gains \`reset()\` alongside \`reload()\`. A new test
exercises Try Again with a stateful child that throws on first render
and succeeds after the source of the bug is removed.
**Suspense fallback no longer null** (React review MEDIUM):
New \`@/components/shared/LazyFallback\` with two variants:
- \`inline\` — fills the parent panel with a centered spinner;
used for SearchResultsPane, ThreadRightBar, InRoomSearch.
- \`dialog\` — uses the shared \`.dialog-overlay\` so the spinner
anchors where the lazy dialog will land; used for
CreateRoomDialog, ManageMembersDialog.
\`aria-busy="true"\` + \`aria-live="polite"\` so screen readers
announce the loading state. Clicks no longer "land on nothing" on
slow connections.
**ChatPage keydown deps tightened** (React review LOW):
The Ctrl-F / Esc effect depended on \`selectedRoom\` (the object), so a
parent re-render that handed us an equivalent-but-new room reference
rebound the global listener. Switched to \`selectedRoom?.id\`.
**SystemMessage handles UTF-8** (React review LOW):
The \`atob(b64)\` → \`JSON.parse(bin)\` decode mangled multi-byte UTF-8
(CJK names in members_added payloads were already a real risk).
Routed through \`TextDecoder('utf-8')\` so the JSON survives intact.
432/432 tests pass (added 1 for ErrorBoundary.reset), typecheck clean,
build clean.
Architecture review MED #5: \`oidcClient.js\` was a stateful client to the auth server that LoginPage and OidcCallback consumed exactly the way components consume \`@/api\` operations. Sitting in \`lib/\` made "where does X live?" ambiguous: \`lib/\` was supposed to be pure, domain-neutral utilities, but it was carrying a singleton with side effects + an external dependency. Moved \`src/lib/oidcClient.js\` → \`src/api/auth/oidcClient.js\`. Updated 4 import paths (LoginPage, OidcCallback, and their tests), plus the oidcClient's own \`./runtimeConfig\` import switched to \`@/lib/runtimeConfig\` since that file stays in lib/. After this commit \`lib/\` is genuinely just pure utilities: constants.js, idgen.js, messageBuffer.js, roomFormat.js, runtimeConfig.js Everything that talks to a backend — NATS or HTTP — lives in \`api/\`. 432/432 tests pass, typecheck clean, build clean.
|
Important Review skippedToo many files! This PR contains 209 files, which is 59 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (209)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
…o new structure Resolves PR #183 conflicts with main. Main shipped a sidebar split into three collapsible sections — Favorite, Apps, Channels and DMs — backed by three new user-service RPCs (subscription.getCurrent / getApps / getRooms). Ported the change onto our refactored tree: **API additions** (new structure): - Typed the three new subject builders in api/_transport/subjects.ts (account: string, siteId: string) so they match the rest of the TS conversion. - New api operation `fetchSidebarBuckets(nats)` that orchestrates the three RPCs in parallel and returns a merged `{ favoriteIds, appIds, channelDmIds, subscriptionData }` shape — the caller doesn't need to know about the three underlying subjects, matching the rest of the api/ pattern. Re-exported from `@/api`. **Context** (new folder structure): - `useRoomSubscriptions` (the extracted hook) fires `fetchSidebarBuckets` on login alongside the existing listRooms / subscription / metadata work. Non-fatal: if the RPCs fail the sidebar falls back to the legacy single-list path. - `useSidebarSections` exposed from `RoomEventsContext` — merges summaries with subscription metadata (name + hrInfo) and partitions them into the three buckets in fixed order. - The reducer's BUCKETS_LOADED action + ROOM_ADDED/REMOVED bucket maintenance auto-merged cleanly (was in lib/, now in context/RoomEventsContext/reducer.js). **Component** (in our refactored folder): - `MainApp/Sidebar/RoomList/RoomList.jsx` rewritten for sections + per-section collapse, using `@/` aliases and `useSidebarSections`. - `MainApp/Sidebar/RoomList/style.css` gains `.room-list-section`, `.room-list-section-header`, and `.room-list-section-collapsed` rules — using design tokens (var(--space-sm), var(--text-xs), var(--font-semibold), var(--text-muted), var(--motion-fast)) so both the light + dark themes pick up correct values automatically via tokens.css. Per our per-component-CSS pattern these stay out of styles/index.css. **lib** (auto-merged, untouched): - `lib/roomFormat.js` now branches on `room.type === 'dm'` to compose the label from HRInfo (engName + name); channel/botDM/discussion prefer subscription.Name with fallback to canonical Room.Name. **Test ports**: - reducer.test.js gains main's bucket-Set + subscriptionData tests (16 new test cases under "roomEventsReducer: bucket Sets"). - RoomEventsContext.test.jsx adds the .subscription.get* mocks that every pre-existing test needed (no real change to test intent, just keeps mocks honest about the new bootstrap path). - RoomList.test.jsx ported wholesale — 7 new test cases covering section render order, empty-section hiding, collapse toggle, badge rendering, click handling. **Deletions**: - Old paths `src/components/MessageInput.test.jsx` and `src/context/RoomEventsContext.jsx` had been deleted in our restructure (replaced by `RoomMessageInput` and the `RoomEventsContext/` folder); kept our deletion. 469/469 tests pass (was 432; +37 from the 3-section + bucket-Set additions), npm run typecheck clean, build clean.
Subscription reviewer + TS reviewer both flagged that the frontend
reads `room.hrInfo?.engName` / `name` but the wire shape was typed as
fully-optional inner fields — and one defensive branch in `roomFormat.js`
existed only to handle an impossible "hrInfo present but empty object"
case.
Aligned the frontend with the backend contract: HRInfo is shipped as a
pointer (`*HRInfo \`json:"hrInfo,omitempty"\``) on
`pkg/model.Subscription`, populated only for DM-type subscriptions, with
its two inner fields (`engName`, `name`) as required strings when the
pointer is present.
Concrete changes:
- `api/fetchSidebarBuckets/index.ts` — extracted a named `HRInfo`
interface (`{ engName: string; name: string }`); `SidebarSubscription`
and `SidebarBuckets.subscriptionData` now reference it. The inner
fields lose their `?` since the wire contract guarantees both when
the parent is present.
- `lib/roomFormat.js` — DM branch simplifies from a four-step
defensive check to a two-line `if (!room.hrInfo) return '(DM)';
destructure` because the partial-hrInfo case is impossible by
construction.
- `lib/roomFormat.test.js` — dropped the "empty hrInfo object" case
(no longer reachable); added a comment explaining why.
Backend lives in a separate repo and is owned elsewhere — the type is
the agreed contract, not implemented here. DM rooms will continue to
render "(DM)" placeholder until the backend ships the field; afterwards
they render `engName name` (or just `name` when the two are equal).
468/468 tests pass, npm run typecheck clean, build clean.
The broader subscription-state slice (Plan B from the reviewers — full
`model.Subscription` per roomId, hasMention seed from server, roles for
self-permission gating) remains a follow-up PR.
…Plan B)
Plan B from the subscription reviewer + TS reviewer: stop dropping the
data the backend already sends and expose it via one hook. Before this
commit, every \`subscription.update\` event was reduced to its
\`name\` field; \`hasMention\` was only set per-incoming-message and
lost on cold reload; \`isCurrentUserOwner\` required a member.list
fetch.
**New types in api/types.ts:**
- \`Role = 'owner' | 'admin' | 'member'\` mirrors \`model.Role\`
- \`Subscription\` mirrors \`pkg/model.Subscription\` — id, u{id,account},
roomId, siteId, roles, name, roomType, isSubscribed?, joinedAt,
lastSeenAt?, hasMention, threadUnread?[], alert, hrInfo?
- \`HRInfo\` moved here as the canonical definition; fetchSidebarBuckets
imports from \`../types\` instead of redefining.
**API layer:**
- \`fetchSidebarBuckets\` now collects FULL Subscription records into
\`SidebarBuckets.subscriptions: Record<string, Subscription>\`
(replacing the narrow \`subscriptionData: {name, hrInfo}\` map).
The wire reply type widened to \`subscriptions?: Subscription[]\`.
**Reducer:**
- New state slice: \`subscriptions: Record<string, Subscription>\`
(replaces \`subscriptionData\`).
- New action \`SUBSCRIPTION_UPSERTED\` — upserts one record, also
merges \`hasMention\`/\`name\` deltas into the matching summary so
the sidebar badge stays live without a refetch.
- \`BUCKETS_LOADED\` now takes \`subscriptions\` (full records) and
ALSO seeds \`summary.hasMention\` from \`subscription.hasMention\` —
closes the cold-reload-loses-mention-badge bug.
- \`ROOM_REMOVED\` drops the subscription too (was already dropping
subscriptionData; renamed).
- \`RESET\` continues to clear (returns initialState).
**useRoomSubscriptions hook:**
- \`subscription.update added\` handler now dispatches
\`SUBSCRIPTION_UPSERTED\` with the full \`evt.subscription\` BEFORE
the \`ROOM_ADDED\` follow-up, so anything that wakes up on
ROOM_ADDED already sees fresh roles / hasMention / alert state.
**RoomEventsContext:**
- New \`useSubscription(roomId)\` hook — returns the full subscription
for the room (or \`undefined\` before bootstrap / for unknown rooms).
Components consume this for self-permission gating, alert state,
thread-unread bookkeeping, etc. without re-fetching.
- \`useSidebarSections\` reads from \`subscriptions\` (full records)
instead of the narrow \`subscriptionData\` — the enrichment still
extracts \`name\` + \`hrInfo\` for room display.
**Tests:**
- Renamed all \`subscriptionData\` assertions to \`subscriptions\`;
fixtures now pass full Subscription-shaped records.
- 3 new reducer cases: SUBSCRIPTION_UPSERTED upsert,
SUBSCRIPTION_UPSERTED replacement, SUBSCRIPTION_UPSERTED no-op-on-empty.
- 1 new reducer case for BUCKETS_LOADED hasMention seed.
- 2 new RoomEventsContext.test cases for useSubscription
(after-bootstrap + pre-bootstrap-unknown-room).
**Not in this commit** (separate concerns):
- No control-plane UI: favorite-toggle, mute toggle, thread-unread
badge component, "joined X days ago", notification system. All
these now have their data plane ready; UI is per-feature work.
- MemberRoster.jsx still derives \`isCurrentUserOwner\` from
member.list. Switching it to \`useSubscription(room.id).roles\` is
a small follow-up that needs test mock changes — held for clarity.
474/474 tests pass (was 468; +6 net), npm run typecheck clean, build clean.
…Subscription type collision
Two real bugs the reviewers caught.
**1. ROOM_ADDED was clobbering pre-existing subscription data (HIGH).**
`useRoomSubscriptions.js` dispatches \`SUBSCRIPTION_UPSERTED\` then
\`ROOM_ADDED\`. The reducer's SUBSCRIPTION_UPSERTED tried to merge into
the matching summary — but the summary didn't exist yet, silent no-op.
Then ROOM_ADDED ran \`toSummary(action.room)\` which hard-resets
\`hasMention: false\`, discarding the server's subscription state.
Net effect today: a \`subscription.update added\` event carrying
\`hasMention=true\` never makes it into the summary badge. The previous
commit's headline ordering claim wasn't actually working.
Fixed by extracting a single \`mergeSubscriptionIntoSummary(summary, sub)\`
helper and using it from all three sites that write summary-from-
subscription: ROOM_ADDED, BUCKETS_LOADED, SUBSCRIPTION_UPSERTED.
Semantics are server-canonical (full-record) — if sub.hasMention is
false, the summary clears; if true, it sets. Live mentions arriving
via MESSAGE_RECEIVED re-OR the badge back on.
Also tightened MESSAGE_RECEIVED: when writing the summary from
roomState, OR with the existing summary's hasMention so a
BUCKETS_LOADED / SUBSCRIPTION_UPSERTED seed isn't clobbered by a
later non-mention message. Active-room behavior unchanged.
**2. \`Subscription\` type declared twice — silent declaration merge (CRITICAL).**
\`api/types.ts\` had \`Subscription\` for the model.Subscription mirror
AND for the NATS-handle \`{unsubscribe(): void}\`. TypeScript silently
fused them into one interface with BOTH sets of fields. \`npm run
typecheck\` was clean only because nobody constructs the NATS handle —
but any caller could write \`sub.hasMention\` on a NATS subscription
handle and get zero diagnostic.
Renamed the handle to \`NatsSubscription\`. Propagated through
\`Nats.subscribe\`'s return type + all four subscribeTo* wrappers.
**Tests added (3):**
- ROOM_ADDED merges a pre-existing subscription record (the bug-fix gate)
- SUBSCRIPTION_UPSERTED with hasMention:false CLEARS the summary (locks
in server-canonical semantics)
- MESSAGE_RECEIVED with hasMention=false does NOT clobber a seeded
mention (locks in the OR-merge in MESSAGE_RECEIVED)
477/477 tests pass, typecheck clean, build clean.
… + api barrel Cleanup pass after the bug fixes in 893e2ae. Addresses the TS reviewer's remaining MED/LOW findings. **RoomEventsContext.jsx → RoomEventsContext.tsx.** Previously every TS caller of `useSubscription(roomId)`, `useRoomSummaries()`, `useSidebarSections()` etc. saw \`any\` for the return value — the "type-safe per-room subscription" promise from Plan B was fiction. Converted the file with concrete types: - \`RoomEventsState\` mirrors the reducer's actual shape (summaries, roomState, activeRoomId, roomsError, favoriteIds / appIds / channelDmIds as \`Set<string>\`, subscriptions as \`Record<string, Subscription>\`) - \`RoomSummary\`, \`RoomBufferState\`, \`SidebarSection\`, \`RoomEventsContextValue\` exported for downstream consumers - \`useSubscription\` is now \`(roomId): Subscription | undefined\` - \`useRoomEvents\`, \`useRoomSummaries\`, \`useRoomDispatch\`, \`useSidebarSections\` all return typed shapes The provider casts \`useNats() as unknown as Nats\` once — \`useNats\` itself is still in NatsContext.jsx (\`createContext(null)\` makes the return type \`never\` without annotations). Safe because the provider only renders inside the connected gate at App.jsx where the NATS handshake has populated user/request/etc. **Tightened SidebarBucketReply.** Reviewer flagged \`subscriptions?: Subscription[]; total?: number\` as lying about the wire — neither field is \`omitempty\` in \`mock-user-service/handler.go::subscriptionListResp\`. Made both required and dropped the \`?? []\` defensiveness inside the api op. **JSDoc honesty on useSubscription.** The previous comment claimed "re-renders only when THIS room's subscription changes" — false (every context consumer re-renders on any state change). Corrected to document the actual scope and point at a future perf option (split context / useSyncExternalStore). **Api barrel re-exports.** \`api/index.ts\` now re-exports every \`types.ts\` symbol that callers need: \`Nats\`, \`Subscription\`, \`Role\`, \`HRInfo\`, \`Room\`, \`Message\`, \`User\`, \`Participant\`, \`MemberEntry\`, \`Reader\`, \`ChannelRef\`, \`HistoryConfig\`, \`HistoryMode\`, \`NatsSubscription\`, \`SubscriptionCallback\`, \`AsyncJobOptions\`, \`AsyncJobResult\`, \`HistoryMessage\`, \`QuotedParentMessage\`, \`RoomType\`. Consumers \`import type { Subscription } from '@/api'\` instead of deep-importing from \`@/api/types\`. 477/477 tests pass, typecheck clean, build clean. **Held for follow-up PR** (perf-only): - \`useSubscription\` re-render scope: today every consumer re-renders on any state change. A dedicated SubscriptionsContext or useSyncExternalStore-based selector would let consumers subscribe to a single roomId. Only matters at >50 rooms; flagged in the hook's JSDoc.
…ructure Resolves PR #183 conflicts with main. Main shipped wire-up for the `message.read` RPC (advances server-side lastSeenAt) + a defensive \`message.id ?? message.messageId\` fallback in MessageActionMenu. **API addition** (new structure): - Typed \`messageRead\` subject builder in api/_transport/subjects.ts - New api operation \`markRoomRead({user, request}, {roomId, siteId})\` — fire-and-forget, swallows errors at the api layer. Re-exported from the api barrel. **Wiring** (refactored placement): - \`RoomEventsContext.tsx\`: \`setActiveRoom(roomId)\` now fires \`markRoomRead\` after dispatch (the "user opened a room" trigger). siteId resolved from the room summary, fallback to user.siteId. - \`useRoomSubscriptions.js\`: new helper \`maybeMarkActiveRead\` reads \`stateRef.current.activeRoomId\` + summaries to fire \`markRoomRead\` when a new_message arrives in the active room from a different sender. Wired into both the DM-event subscription and per-channel subscriptions. The hook signature gained a \`stateRef\` parameter (passed by the provider) to enable this lookup without re-firing the effect. **MessageActionMenu**: kept our \`fetchReadReceipt(nats, ...)\` path; added \`message.id ?? message.messageId\` fallback for defensive parity with main's fix. Today the api layer's \`normalizeHistoricalMessage\` remaps \`messageId\` → \`id\` on every fetch op, so in practice every message reaching MessageActionMenu has \`.id\` set — but the fallback is one line and keeps the menu robust against any pre-normalization path (e.g. quoted-parent snapshots). **Test fixes:** - New MessageActionMenu test case (auto-merged from main) referenced \`readReceipt(...)\` directly; switched to the existing \`READ_RECEIPT_SUBJECT\` constant to keep the test free of internal imports. - Auto-merge swallowed the closing braces of the previous \`it\` + \`describe\` blocks at the seam between message.read wiring and sidebar buckets bootstrap test sections in RoomEventsContext.test.jsx; restored. **Old path deletion confirmed**: main's modify of \`src/context/RoomEventsContext.jsx\` was a recreation; we deleted that file in the restructure and ported the changes to \`RoomEventsContext/RoomEventsContext.tsx\` + \`useRoomSubscriptions.js\`. 484/484 tests pass (was 477; +7 from the message.read wiring tests that auto-merged from main), npm run typecheck clean, build clean.
…+ partial-merge on SUBSCRIPTION_UPSERTED
Two findings from the post-merge reviewers (data-plane + TS, converged).
**Fix A — SET_ACTIVE_ROOM also clears state.subscriptions[r].hasMention.**
The reducer cleared \`summary.hasMention\` when the user opened a room
but left the per-room subscription record's \`hasMention\` at the
server's last-known true. On a cold reload (or any path that reseeds
\`summary.hasMention\` from \`state.subscriptions\` via
\`mergeSubscriptionIntoSummary\` at BUCKETS_LOADED time), the badge
resurrected.
The server eventually emits a \`subscription.update\` mark-read event
that heals it, but if the user closes the tab before that lands, the
inconsistency persists. SET_ACTIVE_ROOM now also writes
\`state.subscriptions[roomId] = { ...sub, hasMention: false }\` for
optimistic local clear. The markRoomRead RPC still fires for the
server-side mark; this just keeps the local view consistent during
the inflight window.
When the room had no pending mention, the subscriptions map keeps its
reference identity (no needless rebuild → consumers don't re-render).
**Fix B — SUBSCRIPTION_UPSERTED partial-merge.**
Was overwriting \`state.subscriptions[roomId]\` with the incoming event
verbatim. If room-worker ever emits a partial \`subscription.update\`
(e.g. role-update carrying only \`roles\`, mark-read carrying only
\`hasMention\`), we lost lastSeenAt / alert / hrInfo / name / etc.
silently.
Reducer now spreads \`{ ...prevSub, ...sub }\` so partial deltas merge
into the prior record. The companion change in
\`mergeSubscriptionIntoSummary\`: each summary field is now written
only if it's actually present in the incoming sub. \`hasMention\` uses
\`'hasMention' in sub\` to distinguish "explicitly false" (clear the
badge) from "absent" (leave the summary's value alone). Live mentions
via MESSAGE_RECEIVED still OR \`hasMention\` back to true.
4 new test cases:
- SET_ACTIVE_ROOM clears subscriptions[r].hasMention but preserves
other subscription fields (the bug-fix gate)
- SET_ACTIVE_ROOM is a no-op on the subscriptions map when no
pending mention exists (referential stability)
- SUBSCRIPTION_UPSERTED merges partial deltas into the prior record
- SUBSCRIPTION_UPSERTED with a partial event does NOT clear an
existing summary hasMention
488/488 tests pass (was 484), npm run typecheck clean, build clean.
**Held for follow-up PR** (correctness fine, perf concern):
- markRoomRead debounce — currently fires per-message in the active
room. 10 msg/sec ⇒ 10 RPCs/sec ⇒ 10 Mongo writes/sec per user. At
1000 active viewers on a chatty channel that's 10k lastSeenAt
writes/sec server-side. Trailing 500ms debounce inside
maybeMarkActiveRead would collapse a burst to one RPC. Separate PR.
…); model DMSubscription as a separate type
Backend changed the HRInfo design. Was: embedded `*HRInfo` field on
\`pkg/model.Subscription\` with two fields. Now:
type DMSubscription struct {
*Subscription
HRInfo *SubscriptionHRInfo \`json:"hrInfo,omitempty"\`
}
type SubscriptionHRInfo struct {
Account string \`json:"account"\`
Name string \`json:"name"\`
EngName string \`json:"engName"\`
}
DMSubscription is a wrapper. Plain Subscription no longer carries HR
data. On the wire Go's embedded struct serialization flattens, so a
DMSubscription is still a Subscription with one extra top-level
\`hrInfo\` field — but conceptually they're distinct types.
**Frontend mirrors the new design:**
- \`api/types.ts\` — renamed \`HRInfo\` → \`SubscriptionHRInfo\` and added
the third field \`account: string\`. Removed \`hrInfo\` from base
\`Subscription\`. Added \`DMSubscription extends Subscription\` with
optional \`hrInfo\`.
- \`fetchSidebarBuckets/index.ts\` — \`SidebarBucketReply.subscriptions\`
typed as \`DMSubscription[]\` to cover both kinds (channels just have
hrInfo undefined). \`SidebarBuckets.subscriptions: Record<string, DMSubscription>\`.
- \`RoomEventsContext.tsx\` — \`RoomEventsState.subscriptions: Record<string, DMSubscription>\`.
\`useSubscription(roomId): DMSubscription | undefined\`. \`RoomSummary.hrInfo\`
imports \`SubscriptionHRInfo\` directly (instead of indexing into
Subscription, which no longer has the field).
**Why DMSubscription on the state map** (not strict Subscription | DMSubscription
union):
- DMSubscription extends Subscription, so channels structurally satisfy
it (hrInfo just undefined). Consumers read \`sub.hrInfo\` directly
without narrowing — the DM-display branch in \`roomFormat.js\` keeps
the same shape.
- A union would require \`'hrInfo' in sub\` type guards at every
consumer. Not worth the friction.
**Test fixtures updated:**
Three places passed \`hrInfo: { engName, name }\` (2 fields); added
\`account\` to each. The \`roomFormat.js\` display logic is unchanged —
it reads engName + name and falls back to "(DM)" when hrInfo is
absent. \`account\` is now on the type for future use but no current
caller reads it.
**Barrel re-exports:** dropped \`HRInfo\`, added \`SubscriptionHRInfo\`
and \`DMSubscription\`.
**No issues found with the backend design:**
- Wire shape backward-compatible: a DMSubscription on the wire is
still a flat object with the same Subscription fields + hrInfo.
Frontend doesn't change parse logic.
- Embedded-pointer Go semantics mean if the embedded *Subscription is
nil, the JSON would be \`{"hrInfo": {…}}\` only. Backend will need
to populate the embedded pointer; if it ever ships a nil-embedded
DMSubscription, every Subscription field is missing — fail-loud
better than silent. The frontend's optional-field types handle
the loud case (most fields would be undefined and render as
empty).
488/488 tests pass, npm run typecheck clean, build clean.
…500ms
Data-plane reviewer flagged the read-storm risk: 10 msg/sec in an
active room → 10 message.read RPCs/sec → 10 Mongo writes/sec per
user to advance lastSeenAt. At 1000 active viewers on a chatty
channel that's 10k writes/sec server-side.
Added a trailing 500ms debounce inside \`useRoomSubscriptions\`
scoped to the new-message-in-active-room path. The setActiveRoom
path (provider-side) stays immediate — that's the explicit user
action and not coalescable.
Implementation:
- \`MARK_READ_DEBOUNCE_MS = 500\` module constant.
- Two refs: \`pendingMarkReadRef\` (latest {roomId, siteId}) and
\`markReadTimeoutRef\` (timer handle).
- New \`scheduleMarkActiveRead\` replaces the previous synchronous
\`maybeMarkActiveRead\`. Each call cancels the prior timer and
arms a fresh one. The handler at fire time re-checks
\`activeRoomId === pending.roomId\` so a mid-burst room switch
doesn't misfire a mark-read for the old room.
- Cleanup function clears the pending timer + nulls the ref so it
can't fire against a torn-down nats connection on logout.
Behavior changes:
- Burst of N messages in an active room → 1 trailing RPC after
the burst (was N).
- Mid-burst room switch → the trailing timer fires the
active-room check, finds the active room has changed, and
skips. The new setActiveRoom already fired its own immediate
mark-read.
- Cleanup (logout / disconnect) → cancels any pending timer.
Tests (2 new):
- 10-message burst in an active room → exactly 1 trailing
message.read after the debounce window (asserted via fake
timers).
- Mid-burst room switch → trailing timer skips; only the two
setActiveRoom calls produced reads.
- Updated the pre-existing "fires message.read when a new_message
arrives in the active channel room" test to advance the fake
timer past the debounce window.
490/490 tests pass, typecheck clean, build clean.
…ixes from reviewers
Adds the new Go types as reference + addresses the reviewers' LOW/MED nits.
**Go (pkg/model):**
- \`SubscriptionHRInfo\` struct — 3 required fields (account, name, engName)
mirroring the wire shape the frontend already mirrors.
- \`DMSubscription\` struct — embeds \`*Subscription\` and adds
\`HRInfo *SubscriptionHRInfo \`json:"hrInfo,omitempty"\``. Embedded-pointer
serialisation flattens at JSON marshal time, matching what the
frontend's \`DMSubscription extends Subscription\` expects.
- 3 model tests:
- JSON flattening — Subscription fields appear top-level alongside
nested hrInfo
- HRInfo pointer absence → omitted from JSON (not \`"hrInfo": null\`)
- SubscriptionHRInfo roundtrips (all 3 fields)
The backend repo (separate) is the canonical owner; this addition is
documentation + a typed reference so the wire contract isn't only
described in frontend types. \`go test ./pkg/model/...\` passes.
**Frontend micro fixes** (reviewer findings, LOW/MED):
- Stale "HRInfo" references in roomFormat.js + RoomEventsContext.tsx
doc comments rewritten to match the new type names + DMSubscription
split.
- \`scheduleMarkActiveRead\` swap: clearTimeout BEFORE writing the new
pendingMarkReadRef. Defensive ordering — JS-safe today but a future
async insertion between those two lines would otherwise race.
**Three new debounce tests** (reviewer flagged gaps):
- Self-sender path: re-asserted after advancing fake timers past the
500ms window — locks in that the self-check short-circuits BEFORE
scheduling, not after.
- Logout cancels pending timer: schedule a burst, flip
\`user → null\`, advance timers; assert no late RPC fires after the
effect's cleanup nulled the timer ref.
- Two bursts >500ms apart fire 2 RPCs: confirms the trailing-edge timer
re-arms cleanly between bursts (not sticky).
492/492 frontend tests pass (was 490), \`go test ./pkg/model/...\`
passes, typecheck clean, build clean.
…E.md The three smoke scripts still imported \`./src/lib/subjects.js\` and \`./src/lib/asyncJob.js\` — those paths haven't existed since the api/ extraction. Pointed them at \`./src/api/_transport/*.ts\` instead. Smoke tests legitimately reach past the api/ barrier because they operate at the wire layer (raw NATS connections + subject builders); production components still go through \`@/api\`. Loading .ts via Node 22's \`--experimental-strip-types\` works out of the box — verified subjects loads cleanly with no transpile step. Added three npm scripts: npm run smoke # full integration (auth → connect → echo) npm run smoke:asyncjob # async-job two-phase against real NATS npm run smoke:livestack # auth + room-service + room-worker E2E Each smoke script's header comment now matches the new invocation. **Added \`chat-frontend/CLAUDE.md\`** capturing every convention this PR established: folder-per-component, folder-per-api-operation, \`api/_transport/\` privacy boundary, \`@/\` path alias rule (≥2 levels), TypeScript-on-api / allowJs-rest stance, lazy + LazyFallback pattern, ErrorBoundary semantics, reducer-vs-hook decision criteria, the SET_ACTIVE_ROOM ↔ markRoomRead ↔ subscriptions.hasMention dance, trailing 500ms debounce contract, fake-timer test pattern, Subscription / DMSubscription split, useSubscription hook usage, smoke-test invocation, and commit hygiene. The quick-reference table at the bottom is the lookup for "I need to add X — where does it go?" 492/492 frontend tests still pass, npm run typecheck clean, build clean, smoke scripts parse-check + load \`api/_transport/subjects.ts\` via \`--experimental-strip-types\`.
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.
Summary
Restructure the chat frontend layout and add a right-hand thread side-panel. Users can now open threads via a hover-revealed Thread icon on messages or via a reply-count badge on parent messages. The panel displays the parent message and its replies (oldest-first) and allows posting new replies. Additionally, implement quote-reply functionality, message edit/delete actions via a unified menu, and extract global layout components (AppHeader, Sidebar).
Key Changes
Layout Restructuring
AppHeaderandSidebarfromChatPageinto dedicated top-level componentsChatPageto focus on room message displayMainAppwrapper component to orchestrate the three-column layout (Sidebar, ChatPage, ThreadRightBar)LoginPageandOidcCallbacktopages/directory with proper index exportsThread Panel Implementation
ThreadEventsContextwith reducer for managing thread state (parent message, replies, loading)ThreadRightBarcomponent withThreadMessageArea(displays parent + replies) andThreadMessageInput(post replies)msgThreadNATS subject builder for fetching thread messagesfetchThreadMessagesAPI function to retrieve thread repliesMessage Actions & Quote Replies
MessageActionscomponent with hover-revealed toolbar (Reply, Edit, Delete)MessageActionMenudropdown for Edit/Delete operationsQuotedBlockcomponent to render quoted message excerpts in-bubbleMessageInputFormshared component supporting quote-reply staging viaquotedParentMessageIdDeleteConfirmDialogandTextInputDialogfor edit/delete confirmationsMessage Display & Organization
MessageListcomponent for rendering message sequencesMessageRowcomponent with avatar, sender label, timestamp, and message bodySystemMessagecomponent for room events (created, members_added, etc.)API Layer Reorganization
api/directory with modular operation functions (sendMessage, editMessage, deleteMessage, fetchThreadMessages, etc.)api/types.tswith shared TypeScript types mirroring Go server modelsapi/_transport/asyncJob.tsfor two-phase NATS reply handlingapi/_transport/normalizeMessage.tsfor message shape normalizationsubjects.tswith thread-related subjects (msgThread, msgEdit, msgDelete)Styling & Design System
tokens.csswith comprehensive design primitives (spacing, typography, colors, shadows, focus rings)index.cssto use CSS variables and add unified focus ring stylesTesting & Infrastructure
@/→src/).vitest/to.gitignorefor test cacheContext & State Management
RoomEventsContextinto subdirectory with separate reducer and subscription hookThreadEventsContextfor thread-specific state (parent, replies, loading)useRoomSubscriptionshook for NATS subscription managementNotable Implementation Details
https://claude.ai/code/session_01LL1Y8mp4gjjGL4PoUUZgfL