feat: Messages page with threading, channel badges, sender indicators (#787)#903
feat: Messages page with threading, channel badges, sender indicators (#787)#903
Conversation
WalkthroughReplaces the placeholder Messages page with a complete implementation: a populated Zustand messages store with async actions, request sequencing, WS event handling, and new-message ID tracking; a new 🚥 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. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches. Re-running this action after a short time may resolve the issue. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive messaging system, including a new Messages page, a Zustand store for state management, a custom hook for data orchestration, and various UI components for channels, threads, and filtering. Documentation and extensive test coverage are also included. Feedback focuses on addressing a potential race condition in the pagination logic, ensuring unique keys when rendering metadata, wiring up the message flashing feature, and adopting a schema validation library for WebSocket payloads.
| fetchMoreMessages: async (channel) => { | ||
| const { messages: existing, loadingMore } = get() | ||
| if (loadingMore) return | ||
| set({ loadingMore: true }) | ||
| try { | ||
| const result = await messagesApi.listMessages({ | ||
| channel, | ||
| limit: MESSAGES_FETCH_LIMIT, | ||
| offset: existing.length, | ||
| }) | ||
| set((s) => ({ | ||
| messages: [...s.messages, ...result.data], | ||
| total: result.total, | ||
| loadingMore: false, | ||
| })) | ||
| } catch (err) { | ||
| set({ loadingMore: false, error: getErrorMessage(err) }) | ||
| } | ||
| }, |
There was a problem hiding this comment.
This function is vulnerable to a race condition. If a full message refresh (from polling or channel switching) occurs while fetchMoreMessages is in flight, this function might append its results to a completely new list of messages, leading to data corruption (duplicates or out-of-order messages).
You can solve this by using the same sequence number pattern (messageRequestSeq) that you've correctly used in fetchMessages. By capturing the sequence number before the async call and checking it after, you can safely discard stale results.
fetchMoreMessages: async (channel) => {
const { messages: existing, loadingMore } = get()
if (loadingMore) return
const seq = messageRequestSeq
set({ loadingMore: true })
try {
const result = await messagesApi.listMessages({
channel,
limit: MESSAGES_FETCH_LIMIT,
offset: existing.length,
})
if (seq !== messageRequestSeq) return
set((s) => ({
messages: [...s.messages, ...result.data],
total: result.total,
loadingMore: false,
}))
} catch (err) {
if (seq !== messageRequestSeq) return
set({ loadingMore: false, error: getErrorMessage(err) })
}
},| {message.metadata.extra.map(([key, value]) => ( | ||
| <MetadataRow key={`extra-${key}-${value}`} label={key} value={value} mono /> | ||
| ))} |
There was a problem hiding this comment.
The key {"extra-${key}-${value}"} is not guaranteed to be unique if the extra array contains duplicate key-value pairs. This could lead to unpredictable rendering behavior in React.
A more robust solution is to include the array index in the key, which guarantees uniqueness for each rendered element. This is a standard React practice for rendering lists of non-unique items.
| {message.metadata.extra.map(([key, value]) => ( | |
| <MetadataRow key={`extra-${key}-${value}`} label={key} value={value} mono /> | |
| ))} | |
| {message.metadata.extra.map(([key, value], i) => ( | |
| <MetadataRow key={`extra-${i}-${key}`} label={key} value={value} mono /> | |
| ))} |
| <MessageList | ||
| messages={filtered} | ||
| expandedThreads={expandedThreads} | ||
| toggleThread={toggleThread} | ||
| onSelectMessage={handleSelectMessage} | ||
| hasMore={hasMore && !hasFilters} | ||
| loadingMore={loadingMore} | ||
| onLoadMore={fetchMore} | ||
| /> |
There was a problem hiding this comment.
The MessageList component and its children have a nice feature to visually flash new messages, which is controlled by the newMessageIds prop. However, this prop is not being passed, so the feature is currently disabled.
To enable it, you could:
- Track new message IDs from WebSocket events in
useMessagesStore. - Expose the set of new IDs from the
useMessagesDatahook. - Pass the IDs to the
MessageListhere. - Remember to add logic to clear the new IDs from the store after they've been rendered to avoid re-flashing.
web/src/stores/messages.ts
Outdated
| if (!payload.message || typeof payload.message !== 'object' || Array.isArray(payload.message)) return | ||
|
|
||
| const candidate = payload.message as Record<string, unknown> | ||
| if ( | ||
| typeof candidate.id !== 'string' || | ||
| typeof candidate.timestamp !== 'string' || | ||
| typeof candidate.sender !== 'string' || | ||
| typeof candidate.channel !== 'string' || | ||
| typeof candidate.content !== 'string' | ||
| ) { | ||
| console.error('[messages/ws] Received malformed message payload, skipping', { | ||
| id: sanitizeForLog(candidate.id), | ||
| hasSender: typeof candidate.sender === 'string', | ||
| hasChannel: typeof candidate.channel === 'string', | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
The manual validation of the WebSocket payload is a good start. To make this more robust, maintainable, and aligned with your TypeScript types, consider using a schema validation library like zod.
By defining a zod schema for your Message type, you can replace this block of if statements with a single schema.safeParse(payload.message) call. This provides stronger type safety, better error messages for malformed data, and ensures your validation logic always stays in sync with your type definitions.
There was a problem hiding this comment.
Actionable comments posted: 23
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/__tests__/hooks/useMessagesData.test.ts`:
- Around line 131-137: The test currently uses a dynamic import to reach the
mocked useWebSocket; instead access the mock directly by using the top-level
import and vi.mocked() instead of await import. Update the test that calls
renderHook(() => useMessagesData('#eng')) to get the call args via const
callArgs = vi.mocked(useWebSocket).mock.calls[0]![0] (or import useWebSocket at
the top and then vi.mocked(useWebSocket)), removing the dynamic import; this
uses the existing mock for assertions on bindings.channel.
In `@web/src/__tests__/pages/messages/ChannelListItem.test.tsx`:
- Around line 46-52: The JSX lines rendering ChannelListItem with long inline
makeChannel calls exceed the 88-character limit; fix by wrapping the props
across multiple lines or extracting the channel prop to a const before render
(e.g., const channel = makeChannel('#dm-alice', { type: 'direct' }) / const
channel = makeChannel('#all-hands', { type: 'broadcast' }) and then
render(<ChannelListItem {...defaultProps} channel={channel} />)) so each line
stays under 88 chars while keeping the same function calls (ChannelListItem,
makeChannel, defaultProps).
In `@web/src/__tests__/pages/messages/ChannelSidebar.test.tsx`:
- Around line 43-47: The test "hides unread badge when count is zero" in
ChannelSidebar.test.tsx uses screen.queryByText('0') which can match other
elements; update the assertion to use an exact-text matcher such as
screen.queryByText('0', { exact: true }) (or a strict regex like /^0$/) so it
only matches the unread-badge text; change the expect accordingly in the test
for the ChannelSidebar component.
In `@web/src/hooks/useMessagesData.ts`:
- Around line 79-91: The WebSocket handler in useMessagesData closes over a
stale activeChannel because bindings are registered once by useWebSocket; fix by
replacing the closed-over value with a ref: create an activeChannelRef in
useMessagesData, update it in an effect when activeChannel changes, and change
the bindings' handler to call useMessagesStore.getState().handleWsEvent(event,
activeChannelRef.current) so the live channel is read at event time;
alternatively, ensure the component using useMessagesData is remounted when
activeChannel changes (e.g., add key={activeChannel}) so useWebSocket
re-registers handlers.
In `@web/src/pages/messages/AttachmentList.tsx`:
- Around line 19-30: The mapped JSX inside attachments.map in AttachmentList is
too complex and must be extracted into a shared UI component; create a new
component named AttachmentBadge that accepts the attachment object (or props
like type and ref), uses the existing ATTACHMENT_ICONS mapping to select the
Icon, and renders the same span + Icon + ref markup and classes, then replace
the map body with a simple <AttachmentBadge key={`${att.type}-${att.ref}`}
att={att} /> so AttachmentList only maps to the new component.
- Line 23: The current key `${att.type}-${att.ref}` in AttachmentList can
collide for identical attachments; update the mapping to use a collision-safe
unique identifier (preferably an intrinsic unique field such as att.id or
att.uuid) when rendering attachments in AttachmentList, and if such a stable id
isn't available fall back to a deterministic composite that includes the array
index (e.g., `${att.type}-${att.ref}-${index}`) so each rendered element has a
stable, unique key during reconciliation.
In `@web/src/pages/messages/ChannelListItem.tsx`:
- Line 18: The ChannelListItem function signature exceeds the repo's 88-char
line limit; update the export function declaration for ChannelListItem to break
the props onto multiple lines (e.g., put the destructured props channel, active,
unreadCount, onClick each on their own line or grouped to keep lines under 88
chars) while preserving the ChannelListItemProps type annotation and existing
export function name so the component compiles and linting passes.
In `@web/src/pages/messages/ChannelSidebar.tsx`:
- Around line 64-85: Extract the JSX inside the TYPE_ORDER.map into a new
reusable UI component (e.g., ChannelGroupSection) under web/src/components/ui/
and replace the inline block with a single component call; the new
ChannelGroupSection should accept props: type (channel type), items
(grouped.get(type)), activeChannel, unreadCounts, and onSelectChannel, and
render the header using getChannelTypeLabel(type) and the ChannelListItem list
(using ch.name as key, ch for channel prop, active={ch.name === activeChannel},
unreadCount={unreadCounts[ch.name] ?? 0}, onClick={() =>
onSelectChannel(ch.name)}). Update the map to return <ChannelGroupSection
key={type} type={type} items={items} activeChannel={activeChannel}
unreadCounts={unreadCounts} onSelectChannel={onSelectChannel} /> so you avoid
>8-line JSX inside the .map().
In `@web/src/pages/messages/MessageDetailDrawer.stories.tsx`:
- Around line 53-55: The WithAttachments story currently duplicates the
FullMetadata state; update it to focus on attachments by replacing its
args.message reference (currently fullMessage) with a dedicated fixture that has
minimal metadata and multiple attachment variants (e.g., create
messageWithAttachments or modify fullMessage to strip non-essential metadata and
add several attachments), or remove the WithAttachments export entirely if you
prefer to avoid duplication; locate the WithAttachments export in
MessageDetailDrawer.stories.tsx and either change args: { message: fullMessage,
open: true } to args: { message: messageWithAttachments, open: true } (and
define messageWithAttachments) or delete the WithAttachments block.
In `@web/src/pages/messages/MessageDetailDrawer.tsx`:
- Line 24: Replace inline prop object types with named interfaces: create a new
interface (e.g., MessageDetailContentProps) for the props passed to the
MessageDetailContent function and use it in the function signature instead of
the inline { message: Message } type; do the same for the other inline prop
shape at the occurrence around line 93 (replace that inline object type with a
named interface and update the component signature accordingly). Ensure
interface names are exported/placed near other prop interfaces to match project
conventions and update any imports/types if needed.
- Around line 76-78: The React key for metadata rows in MessageDetailDrawer is
not guaranteed unique because `extra-${key}-${value}` can collide; update the
mapping over message.metadata.extra in the component (the map that renders
MetadataRow) to include the element index or another stable unique identifier in
the key (e.g., append the map index or use a stable id from the data) so keys
become unique and reconciliation is stable; reference the map producing
<MetadataRow key=... label={key} value={value} mono /> and change the key
construction to include the index or unique id.
In `@web/src/pages/messages/MessageFilterBar.tsx`:
- Line 145: Replace the inline prop type for the FilterPill component with a
named interface: declare an interface (e.g., FilterPillProps) that defines
label: string and onRemove: () => void, then update the function signature from
function FilterPill({ label, onRemove }: { label: string; onRemove: () => void
}) to function FilterPill({ label, onRemove }: FilterPillProps); keep the
destructuring and behavior unchanged and update any local references if needed.
In `@web/src/pages/messages/MessageList.stories.tsx`:
- Line 23: The story decorator is using a hardcoded pixel height class
`h-[500px]`; update the decorators array in MessageList.stories.tsx (the
decorators: [(Story) => <div className="..."><Story /></div>]) to use a standard
Tailwind height/token-based class instead of `h-[500px]`—for example replace
with a standard Tailwind size like `h-96` or, if your design system exposes a
density-aware token (e.g., `h-card` or another project token), use that to
comply with the `p-card`/`gap-*` spacing rules.
In `@web/src/pages/messages/MessagesSkeleton.tsx`:
- Around line 18-29: Extract the JSX block inside the Array.from map into a new
reusable component (e.g., MessageRowSkeleton) and replace the inline JSX in
MessagesSkeleton's map with <MessageRowSkeleton key={i} />; specifically, move
the div with className "flex gap-3 rounded-lg border border-border bg-card p-4"
and its children (Skeleton, SkeletonText, inner divs) into the new
MessageRowSkeleton component (no props needed), export it as a named/default
component from its module, import it into MessagesSkeleton, and ensure the map
simply returns the component instance to satisfy the "no complex JSX in .map()"
rule.
In `@web/src/pages/messages/MessageThread.stories.tsx`:
- Line 6: Rename the local variable meta (type MessageMetadata) to a
non-conflicting name such as messageMetadata or sampleMetadata and update all
usages (e.g., the objects inside threadMessages that set metadata) to reference
the new identifier; ensure the Storybook default export named storyMeta remains
unchanged and that no other symbol collisions occur with the export named
storyMeta or Storybook's conventions.
In `@web/src/pages/messages/MessageTypeBadge.stories.tsx`:
- Around line 14-17: The ALL_TYPES constant array exceeds the 88-character line
length; reformat the array literal for MessageType[] (symbol: ALL_TYPES) so each
string entry (or small groups) is placed on its own line or wrapped to keep
lines <= 88 characters, preserving the existing order and commas and ensuring
TypeScript syntax remains valid (no code logic changes required).
In `@web/src/pages/MessagesPage.tsx`:
- Around line 170-178: The MessageList usage is missing the optional
newMessageIds prop so new-message highlighting in MessageBubble is never
triggered; add a newMessageIds state (e.g., const [newMessageIds,
setNewMessageIds] = useState<Set<string>>(new Set())) and update it when new
messages arrive in your WebSocket/store handler, then pass
newMessageIds={newMessageIds} into the MessageList component call shown (and
clear IDs after a timeout or on user interaction), or if highlighting is not
needed remove the prop handling from MessageList/MessageBubble to avoid dead
code.
- Around line 46-49: Do not call setWasConnected(true) during render; instead
add a useEffect that watches wsConnected (and optionally wasConnected) and calls
setWasConnected(true) when wsConnected becomes true and wasConnected is false.
Move the logic that currently runs inline (the wsConnected && !wasConnected
check and setWasConnected call) into that useEffect so state updates happen in
an effect rather than during render.
In `@web/src/stores/messages.ts`:
- Around line 86-104: fetchMoreMessages can append stale results when the
channel changes mid-request; implement the same sequence guard used in
fetchMessages by capturing the current sequence/id before the await and
verifying it after the await (and before calling set) to ensure the response
still belongs to the same channel. In practice, read the store's sequence or
channel token via get() (the same symbol used by fetchMessages), save it to a
local variable, call messagesApi.listMessages(...), then after the await check
the store's sequence/token again and abort (or ignore the result) if it has
changed; only then update messages, total and loadingMore. Also ensure
loadingMore is reset in both the aborted and success paths and use
getErrorMessage(err) in the catch as before.
- Around line 106-126: The WebSocket handler handleWsEvent currently only
validates id, timestamp, sender, channel, and content before casting candidate
to Message, which can lead to runtime errors when other required Message
properties (e.g., type, priority, to, attachments, metadata) are missing; update
handleWsEvent to either (a) extend the runtime checks to validate those
additional fields on candidate (ensuring correct types) and reject the payload
with console.error if they are invalid, or (b) populate missing optional fields
with safe defaults (e.g., attachments: [], metadata: {}, to: null, priority:
'normal') before casting, or (preferably) plug a schema validator (e.g., Zod) to
validate/parse payload.message into Message and use the parsed result; locate
and modify the logic around payload.message / candidate / const message in
handleWsEvent to implement this change.
In `@web/src/utils/messages.ts`:
- Line 6: Rename the constant maps from SCREAMING_SNAKE_CASE to camelCase (e.g.,
rename MESSAGE_TYPE_LABELS -> messageTypeLabels) and do the same for the other
map constants referenced in this file (the ones at the other commented
locations), then update every usage/exports to use the new camelCase
identifiers; ensure you only change the variable names (preserve types like
Record<MessageType, string> and the object contents) and run a quick
project-wide search/replace to catch all references to the old names so imports
and internal calls compile.
- Around line 146-147: The code reads msg.metadata.task_id directly which can
throw when metadata is null; update the taskId assignment in messages.ts to
safely handle nullable metadata (e.g., use optional chaining or a null-check on
msg.metadata before accessing task_id) and ensure subsequent logic that uses
taskId (the if (!taskId) branch and any rendering paths) treats missing
metadata/task IDs as standalone cases so rendering does not break.
- Line 88: The long single-line date formatting expression using `d` (the
`return `${d.getFullYear()}-${String(d.getMonth() + 1).padStart(2,
'0')}-${String(d.getDate()).padStart(2, '0')}`` line) exceeds the 88-character
limit; refactor by extracting parts into named constants (e.g., `year`, `month`,
`day`) or build the string across multiple shorter concatenations so each line
stays under 88 characters, and apply the same change to the other occurrence
around line 127 in the same file (`web/src/utils/messages.ts`).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7aac61e6-8af6-41c5-bb75-78df769b190e
📒 Files selected for processing (41)
docs/design/communication.mddocs/design/page-structure.mdweb/src/__tests__/helpers/factories.tsweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/stores/messages.test.tsweb/src/__tests__/utils/messages.test.tsweb/src/hooks/useMessagesData.tsweb/src/pages/MessagesPage.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/stores/messages.tsweb/src/utils/messages.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Dashboard Test
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
Use camelCase for variable names and function identifiers
Files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/__tests__/utils/messages.test.tsweb/src/hooks/useMessagesData.tsweb/src/pages/MessagesPage.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/stores/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/utils/messages.tsweb/src/pages/messages/MessageList.stories.tsx
web/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.tsx: ALWAYS reuse existing shared components fromweb/src/components/ui/before creating new ones (e.g.,StatusBadge,MetricCard,AgentCard,DeptHealthBar,SectionCard)
Use Tailwind semantic classes (text-foreground,bg-card,text-accent,text-success) or CSS variables (var(--so-accent)); NEVER hardcode hex values or rgba() in .tsx files
Usefont-sansorfont-monofor typography (maps to Geist tokens); NEVER setfontFamilydirectly in CSS
Use density-aware spacing tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Do NOT recreate complex (>8 line) JSX inside.map()blocks; extract to a shared component inweb/src/components/ui/
Files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/MessagesPage.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageList.stories.tsx
**/*.{py,ts,tsx,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Lines must not exceed 88 characters (enforced by ruff for Python, configured in web/.eslintrc for TypeScript)
Files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/__tests__/utils/messages.test.tsweb/src/hooks/useMessagesData.tsweb/src/pages/MessagesPage.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/stores/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/utils/messages.tsweb/src/pages/messages/MessageList.stories.tsx
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Import
cnfrom@/lib/utilsfor conditional class merging in components
Files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/__tests__/utils/messages.test.tsweb/src/hooks/useMessagesData.tsweb/src/pages/MessagesPage.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/stores/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/utils/messages.tsweb/src/pages/messages/MessageList.stories.tsx
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When an implementation deviates from the spec (better approach found, scope evolved, etc.), update the relevant
docs/design/page to reflect the new reality
Files:
docs/design/communication.mddocs/design/page-structure.md
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation must be written in Markdown and built with Zensical (config:
mkdocs.yml); design spec lives indocs/design/(11 pages linked fromdocs/DESIGN_SPEC.md)
Files:
docs/design/communication.mddocs/design/page-structure.md
web/src/pages/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Lazy-load page components and place page-scoped sub-components in
pages/<page-name>/subdirectories (e.g.,pages/tasks/,pages/org-edit/)
Files:
web/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/MessagesPage.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageList.stories.tsx
🧠 Learnings (7)
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to web/src/__tests__/**/*.{ts,js} : Dashboard testing: Vitest unit tests organized by feature under `web/src/__tests__/`. Use fast-check for property-based testing (`fc.assert` + `fc.property`).
Applied to files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/utils/messages.test.tsweb/src/__tests__/stores/messages.test.ts
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : New shared components must have a corresponding `.stories.tsx` Storybook file with all component states (default, hover, loading, error, empty)
Applied to files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : Export component props as a TypeScript interface
Applied to files:
web/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to docs/design/**/*.md : When an implementation deviates from the spec (better approach found, scope evolved, etc.), update the relevant `docs/design/` page to reflect the new reality
Applied to files:
docs/design/page-structure.md
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to docs/design/*.md : Design spec pages: 7 pages in `docs/design/` — index, agents, organization, communication, engine, memory, operations
Applied to files:
docs/design/page-structure.md
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Do NOT recreate complex (>8 line) JSX inside `.map()` blocks; extract to a shared component in `web/src/components/ui/`
Applied to files:
web/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageList.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : ALWAYS reuse existing shared components from `web/src/components/ui/` before creating new ones (e.g., `StatusBadge`, `MetricCard`, `AgentCard`, `DeptHealthBar`, `SectionCard`)
Applied to files:
web/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessageBubble.tsx
🔇 Additional comments (43)
web/src/__tests__/helpers/factories.ts (1)
115-144: Factory additions look good and type-aligned.
makeMessage/makeChanneldefaults are coherent with the current API contracts
and give predictable fixtures for UI/store tests.web/src/pages/messages/AttachmentList.stories.tsx (1)
1-31: Story coverage is solid for this component.The three stories capture mixed/single/empty states and match component behavior.
web/src/__tests__/pages/messages/AttachmentList.test.tsx (1)
1-36: Good baseline coverage for AttachmentList.The suite exercises empty, single, multiple, and icon-rendering paths
effectively.web/src/pages/messages/TimestampDivider.tsx (1)
1-13: Implementation is clean and guideline-compliant.Props are defined via
interface, and the divider markup/styling is consistent.web/src/pages/messages/MessagesSkeleton.tsx (1)
5-17: Skeleton container/accessibility baseline looks good.
role="status"plus a clear loading label is appropriate, and the layout mirrors
the page structure well.Also applies to: 30-33
web/src/pages/messages/MessagesSkeleton.stories.tsx (1)
1-13: Story setup is correct and complete for this component.Typed meta/story config is clean, and the default state is appropriate here.
docs/design/communication.md (1)
115-135: No changes required. The docs/design/communication.md correctly shows the field name assender, which matches the backend implementation. Although the Message model defines an alias"from", API serialization uses the default Pydantic behavior (withoutby_alias=True), which outputs the canonical field namesender, not the alias. Frontend types and documentation are consistent with actual API payloads.> Likely an incorrect or invalid review comment.web/src/pages/messages/MessageTypeBadge.tsx (1)
1-22: Looks good — clean and guideline-aligned badge component.Props typing, class merging, and semantic styling usage are all solid here.
web/src/pages/messages/ChannelListItem.stories.tsx (1)
1-62: Story coverage is strong for channel state combinations.Type variants and active/unread permutations are well represented.
web/src/pages/messages/TimestampDivider.stories.tsx (1)
1-16: LGTM — clear, focused story states for date grouping labels.web/src/__tests__/pages/messages/MessageBubble.test.tsx (1)
1-70: Good component-level test coverage across display and interaction paths.This suite meaningfully exercises the core message bubble variants.
web/src/__tests__/pages/messages/MessageList.test.tsx (1)
1-86: Well-structured test suite with good coverage.The tests cover the key behaviors of
MessageList: empty state, message rendering, pagination controls (button visibility, click handler, loading state), threading grouping, and accessibility (aria-live). The mock foruseFlashis consistent with other message-related tests.web/src/__tests__/pages/messages/MessageThread.test.tsx (1)
1-82: Comprehensive test coverage for thread expand/collapse behavior.The suite correctly tests the conditional rendering logic (single message vs. collapsed thread vs. expanded thread), interaction handlers (
onToggle,onSelectMessage), and the empty array edge case. Good alignment with the component's expected behavior.web/src/__tests__/pages/MessagesPage.test.tsx (3)
28-31: Clever mock pattern to avoid module hoisting issues.The dynamic property access pattern with
hookNameworks around Vitest's module hoisting behavior, allowing the mock to return a mutablehookReturnobject that can be modified per-test.
41-44: Good test isolation with proper state reset.Resetting
hookReturnwith a freshexpandedThreads: new Set()before each test ensures no shared mutable state leaks between tests.
46-126: Solid page-level test coverage.Tests cover the key UI states: loading, error, WebSocket status, channel selection prompt, sidebar rendering, empty state, and message display. The URL-encoded channel parameter handling (
%23engfor#eng) is correct.web/src/pages/messages/MessageThread.stories.tsx (1)
37-47: Good story coverage for the main component states.The three stories (Collapsed, Expanded, SingleMessage) cover the primary visual states of the component and align with the test cases in
MessageThread.test.tsx.web/src/__tests__/hooks/useMessagesData.test.ts (3)
13-16: Correct use ofvi.hoisted()for mock references.Using
vi.hoisted()ensuresmockPollingStartandmockPollingStopare available when the mockedusePollingmodule is evaluated, avoiding reference errors from module hoisting.
35-54: Clean store reset pattern combining state and mocked actions.This approach allows tests to verify that the hook calls the expected store actions while also controlling the state that the hook reads. The explicit listing of all state fields ensures deterministic test starting conditions.
56-156: Comprehensive hook test coverage.The suite tests all key hook behaviors: channel/message fetching, unread reset, polling lifecycle, WebSocket setup, and state derivation (
hasMore,loading,error,unreadCounts). Good alignment with the hook's responsibilities.web/src/__tests__/pages/messages/MessageDetailDrawer.test.tsx (1)
1-83: Thorough test coverage for conditional rendering in the detail drawer.The tests correctly verify: metadata row visibility based on null values, attachment section presence, the null message fallback state, and priority badge rendering. The
extrametadata key-value pair test (lines 42-46) confirms the new schema field is properly displayed.web/src/pages/messages/ChannelSidebar.stories.tsx (1)
1-48: Comprehensive Storybook coverage for all sidebar states.The five stories cover the key visual states: default (no selection), active channel highlight, unread count badges, loading skeleton, and empty state. This aligns well with the component's behavior documented in the AI summary and test assertions.
docs/design/page-structure.md (2)
86-99: Documentation accurately reflects the implemented Messages page features.The feature list aligns with the PR objectives: channel sidebar with grouping/badges, message feed with sender indicators, timestamp grouping,
task_id-based threading, client-side filtering with URL sync, detail drawer, WebSocket real-time updates, and pagination. The URL params (channel,type,priority,search,message) match the implementation described in the AI summary.
238-239: Routing table updated with new deep-link patterns.The additions for
?type=:typefiltering and?message=:iddetail drawer align with the documented URL params and provide clear deep-linking support for the new functionality.web/src/pages/messages/MessageFilterBar.tsx (1)
22-143: Solid filter UX and state handling.Filter controls, pill behavior, and count rendering are well-implemented and consistent.
web/src/pages/messages/ChannelSidebar.tsx (1)
39-60: Loading and empty states are cleanly handled.State branching and fallback rendering look correct.
web/src/__tests__/utils/messages.test.ts (1)
13-186: Good utility test coverage depth.The suite exercises happy paths and important edge cases across all message utility helpers.
web/src/pages/messages/MessageThread.tsx (1)
23-89: Thread rendering logic looks correct.Single-message fallback, collapsed summary, and expanded threaded rendering are implemented cleanly.
web/src/__tests__/pages/messages/MessageFilterBar.test.tsx (1)
16-94: Strong behavioral coverage for filter interactions.The suite validates control rendering, update callbacks, and active-filter pill lifecycle comprehensively.
web/src/pages/messages/MessageBubble.tsx (2)
1-69: Well-structured component following project conventions.The implementation correctly:
- Uses
interfacefor props (coding guideline compliant)- Imports
cnfrom@/lib/utilsfor class merging- Reuses existing
Avatarfrom shared components- Uses semantic Tailwind classes (
text-foreground,text-muted-foreground,ring-accent)- Uses
font-monofor typography tokens- Implements flash animation with proper ref guard to prevent re-triggering
62-66: No issue found with attachments array access.The
Messageinterface definesattachmentsasreadonly attachments: readonly Attachment[]inweb/src/api/types.ts, guaranteeing it is always an array. The length check on line 62 is safe.web/src/__tests__/stores/messages.test.ts (3)
67-89: Excellent race condition test for stale response handling.This test properly validates the request-sequence guard pattern by:
- Starting a slow fetch
- Starting a fast fetch that completes first
- Resolving the slow fetch after
- Verifying the store retains the fast response
This ensures the module-level
messageRequestSeqcounter works correctly.
154-161: Console spy cleanup is correct.The
mockRestore()call ensures the spy doesn't leak to other tests.
1-208: Comprehensive store test coverage.The test suite covers:
- Channel/message fetching with success and error paths
- Pagination via
fetchMoreMessageswith skip-if-loading guard- WebSocket event handling for active/inactive channels
- Malformed payload rejection with console.error logging
- Thread toggle state management
- Unread count reset with no-op for missing channels
Well-structured and follows testing best practices.
web/src/pages/messages/MessageFilterBar.stories.tsx (1)
1-33: Good Storybook coverage for filter bar states.The stories cover key filter combinations:
- No filters (default state)
- Single type filter
- Multiple filters (type + priority + search)
- Search-only filter
The
a11y: { test: 'error' }parameter ensures accessibility testing is enforced.web/src/hooks/useMessagesData.ts (1)
72-77: Polling effect correctly manages start/stop lifecycle.The polling starts when
activeChannelchanges and stops on cleanup. The eslint-disable comment is justified sincepolling.startandpolling.stopare stable refs fromuseCallback.web/src/pages/messages/MessageBubble.stories.tsx (1)
1-66: Comprehensive Storybook coverage for MessageBubble.The stories cover:
- Default state with normal priority
- High and urgent priority with visual indicators
- Attachments rendering
- Different message types (delegation, task_update, escalation)
- New message flash animation state
The
baseMessagefixture provides a completeMessageobject structure for story variations.web/src/stores/messages.ts (1)
144-163: Thread toggle and unread reset implementations are clean.
toggleThreadcorrectly creates a newSetto trigger React re-renders.resetUnreaduses an early return optimization when the channel has no unread count, avoiding unnecessary state updates.web/src/pages/MessagesPage.tsx (2)
113-116: Precise skeleton loading condition.The condition
loading && messages.length === 0 && channelsLoading && channels.length === 0correctly shows the skeleton only during initial load when both channels and messages are loading and empty. This avoids skeleton flash on subsequent data refreshes.
1-196: Solid page implementation with URL-driven state.The page correctly:
- Syncs filters to URL search params with validation
- Handles channel/message selection via URL
- Shows appropriate loading, error, and empty states
- Gates the WS disconnect banner with
wasConnectedto avoid flash on initial load- Disables pagination when client-side filters are active
web/src/pages/messages/MessageList.tsx (3)
42-50: Auto-scroll logic is correct but has a subtle edge case.The auto-scroll triggers when
messages.length > prevLengthRef.current. This works for new messages arriving, but if messages are deduplicated or filtered, the count could increase without new messages appearing at the bottom (e.g., if older messages load via pagination).Since pagination prepends to the top (via "Load earlier messages"), and new WebSocket messages prepend to the store, this should work correctly in practice. The
wasAtBottomRefguard ensures scrolling only happens when the user was already at the bottom.
70-91: Good accessibility and pagination UX.
aria-live="polite"announces message changes to screen readers without interrupting- The "Load earlier messages" button is correctly disabled during
loadingMore- Loader spinner provides visual feedback during pagination
93-124: Message grouping and rendering structure is clean.The nested iteration over date groups → threads/standalone is well-organized:
- Each date group gets a
TimestampDivider- Threads are rendered via
MessageThreadwith expand/collapse state- Standalone messages use
MessageBubbledirectly- Keys are stable (
dateKey,taskId,msg.id)The JSX uses component composition rather than recreating complex markup inline, which aligns with the coding guideline about
.map()blocks.
| {attachments.map((att) => { | ||
| const Icon = ATTACHMENT_ICONS[att.type] | ||
| return ( | ||
| <span | ||
| key={`${att.type}-${att.ref}`} | ||
| className="inline-flex items-center gap-1 rounded border border-border bg-surface px-1.5 py-0.5 font-mono text-[10px] text-secondary" | ||
| > | ||
| <Icon className="size-3" aria-hidden="true" /> | ||
| {att.ref} | ||
| </span> | ||
| ) | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract mapped badge JSX to a shared component.
The .map() block contains complex JSX and breaches the repo rule for
web/src/**/*.tsx. Please move this badge rendering into a shared UI component
and map that component instead.
As per coding guidelines: "Do NOT recreate complex (>8 line) JSX inside .map()
blocks; extract to a shared component in web/src/components/ui/."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/messages/AttachmentList.tsx` around lines 19 - 30, The mapped
JSX inside attachments.map in AttachmentList is too complex and must be
extracted into a shared UI component; create a new component named
AttachmentBadge that accepts the attachment object (or props like type and ref),
uses the existing ATTACHMENT_ICONS mapping to select the Icon, and renders the
same span + Icon + ref markup and classes, then replace the map body with a
simple <AttachmentBadge key={`${att.type}-${att.ref}`} att={att} /> so
AttachmentList only maps to the new component.
| fetchMoreMessages: async (channel) => { | ||
| const { messages: existing, loadingMore } = get() | ||
| if (loadingMore) return | ||
| set({ loadingMore: true }) | ||
| try { | ||
| const result = await messagesApi.listMessages({ | ||
| channel, | ||
| limit: MESSAGES_FETCH_LIMIT, | ||
| offset: existing.length, | ||
| }) | ||
| set((s) => ({ | ||
| messages: [...s.messages, ...result.data], | ||
| total: result.total, | ||
| loadingMore: false, | ||
| })) | ||
| } catch (err) { | ||
| set({ loadingMore: false, error: getErrorMessage(err) }) | ||
| } | ||
| }, |
There was a problem hiding this comment.
fetchMoreMessages lacks channel-switch race protection.
Unlike fetchMessages, this action doesn't use a sequence guard. If a user switches channels while a "load more" request is in flight, the response will append stale messages from the previous channel to the new channel's list.
Proposed fix: Add sequence guard to fetchMoreMessages
fetchMoreMessages: async (channel) => {
+ const seq = ++messageRequestSeq
const { messages: existing, loadingMore } = get()
if (loadingMore) return
set({ loadingMore: true })
try {
const result = await messagesApi.listMessages({
channel,
limit: MESSAGES_FETCH_LIMIT,
offset: existing.length,
})
+ if (seq !== messageRequestSeq) return
set((s) => ({
messages: [...s.messages, ...result.data],
total: result.total,
loadingMore: false,
}))
} catch (err) {
+ if (seq !== messageRequestSeq) return
set({ loadingMore: false, error: getErrorMessage(err) })
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fetchMoreMessages: async (channel) => { | |
| const { messages: existing, loadingMore } = get() | |
| if (loadingMore) return | |
| set({ loadingMore: true }) | |
| try { | |
| const result = await messagesApi.listMessages({ | |
| channel, | |
| limit: MESSAGES_FETCH_LIMIT, | |
| offset: existing.length, | |
| }) | |
| set((s) => ({ | |
| messages: [...s.messages, ...result.data], | |
| total: result.total, | |
| loadingMore: false, | |
| })) | |
| } catch (err) { | |
| set({ loadingMore: false, error: getErrorMessage(err) }) | |
| } | |
| }, | |
| fetchMoreMessages: async (channel) => { | |
| const seq = ++messageRequestSeq | |
| const { messages: existing, loadingMore } = get() | |
| if (loadingMore) return | |
| set({ loadingMore: true }) | |
| try { | |
| const result = await messagesApi.listMessages({ | |
| channel, | |
| limit: MESSAGES_FETCH_LIMIT, | |
| offset: existing.length, | |
| }) | |
| if (seq !== messageRequestSeq) return | |
| set((s) => ({ | |
| messages: [...s.messages, ...result.data], | |
| total: result.total, | |
| loadingMore: false, | |
| })) | |
| } catch (err) { | |
| if (seq !== messageRequestSeq) return | |
| set({ loadingMore: false, error: getErrorMessage(err) }) | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/messages.ts` around lines 86 - 104, fetchMoreMessages can
append stale results when the channel changes mid-request; implement the same
sequence guard used in fetchMessages by capturing the current sequence/id before
the await and verifying it after the await (and before calling set) to ensure
the response still belongs to the same channel. In practice, read the store's
sequence or channel token via get() (the same symbol used by fetchMessages),
save it to a local variable, call messagesApi.listMessages(...), then after the
await check the store's sequence/token again and abort (or ignore the result) if
it has changed; only then update messages, total and loadingMore. Also ensure
loadingMore is reset in both the aborted and success paths and use
getErrorMessage(err) in the catch as before.
|
|
||
| // ── Message type labels ──────────────────────────────────── | ||
|
|
||
| const MESSAGE_TYPE_LABELS: Record<MessageType, string> = { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename constant maps to camelCase.
These map identifiers are variables and should follow camelCase in this codebase.
Proposed rename set
-const MESSAGE_TYPE_LABELS: Record<MessageType, string> = {
+const messageTypeLabels: Record<MessageType, string> = {
@@
export function getMessageTypeLabel(type: MessageType): string {
- return MESSAGE_TYPE_LABELS[type]
+ return messageTypeLabels[type]
}
@@
-const PRIORITY_COLOR_MAP: Partial<Record<MessagePriority, SemanticColor>> = {
+const priorityColorMap: Partial<Record<MessagePriority, SemanticColor>> = {
@@
export function getMessagePriorityColor(priority: MessagePriority): SemanticColor | null {
- return PRIORITY_COLOR_MAP[priority] ?? null
+ return priorityColorMap[priority] ?? null
}
@@
-const PRIORITY_DOT_CLASSES: Record<SemanticColor, string> = {
+const priorityDotClasses: Record<SemanticColor, string> = {
@@
export function getPriorityDotClass(color: SemanticColor): string {
- return PRIORITY_DOT_CLASSES[color]
+ return priorityDotClasses[color]
}
@@
-const PRIORITY_BADGE_CLASSES: Record<SemanticColor, string> = {
+const priorityBadgeClasses: Record<SemanticColor, string> = {
@@
export function getPriorityBadgeClasses(color: SemanticColor): string {
- return PRIORITY_BADGE_CLASSES[color]
+ return priorityBadgeClasses[color]
}
@@
-const CHANNEL_TYPE_ICONS: Record<ChannelType, string> = {
+const channelTypeIcons: Record<ChannelType, string> = {
@@
export function getChannelTypeIcon(type: ChannelType): string {
- return CHANNEL_TYPE_ICONS[type]
+ return channelTypeIcons[type]
}
@@
-const CHANNEL_TYPE_LABELS: Record<ChannelType, string> = {
+const channelTypeLabels: Record<ChannelType, string> = {
@@
export function getChannelTypeLabel(type: ChannelType): string {
- return CHANNEL_TYPE_LABELS[type]
+ return channelTypeLabels[type]
}As per coding guidelines, **/*.{ts,tsx}: Use camelCase for variable names and
function identifiers.
Also applies to: 25-25, 36-36, 47-47, 60-60, 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/utils/messages.ts` at line 6, Rename the constant maps from
SCREAMING_SNAKE_CASE to camelCase (e.g., rename MESSAGE_TYPE_LABELS ->
messageTypeLabels) and do the same for the other map constants referenced in
this file (the ones at the other commented locations), then update every
usage/exports to use the new camelCase identifiers; ensure you only change the
variable names (preserve types like Record<MessageType, string> and the object
contents) and run a quick project-wide search/replace to catch all references to
the old names so imports and internal calls compile.
| const taskId = msg.metadata.task_id | ||
| if (!taskId) { |
There was a problem hiding this comment.
Guard nullable metadata before reading task_id.
Line 146 assumes msg.metadata is always present. With nullable metadata, this can
throw and break message rendering; treat missing metadata/task IDs as standalone.
Proposed fix
- const taskId = msg.metadata.task_id
+ const taskId = msg.metadata?.task_id
if (!taskId) {
standalone.push(msg)
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const taskId = msg.metadata.task_id | |
| if (!taskId) { | |
| const taskId = msg.metadata?.task_id | |
| if (!taskId) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/utils/messages.ts` around lines 146 - 147, The code reads
msg.metadata.task_id directly which can throw when metadata is null; update the
taskId assignment in messages.ts to safely handle nullable metadata (e.g., use
optional chaining or a null-check on msg.metadata before accessing task_id) and
ensure subsequent logic that uses taskId (the if (!taskId) branch and any
rendering paths) treats missing metadata/task IDs as standalone cases so
rendering does not break.
23e0c3b to
ba58192
Compare
There was a problem hiding this comment.
Actionable comments posted: 23
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/communication.md`:
- Line 134: Update the docs text to correct the inaccurate "All metadata fields
are nullable" claim: state that all metadata fields are nullable except the
extra field, which is non-nullable and has a default empty tuple (extra:
tuple[...] = Field(default=(), ...)); revise the sentence to note that extra is
always present as an immutable empty tuple when no entries exist.
- Line 115: Documentation shows the JSON key as "sender" but the Message model
uses a Pydantic alias that serializes the field to "from" (see the Message class
and its aliased sender field and the persistence mapping that moves data["from"]
= data.pop("sender")); update the docs line to show "from": "sarah_chen" instead
of "sender" so the example matches the Message model's JSON representation and
the repository deserialization logic.
In `@web/src/__tests__/pages/messages/MessageDetailDrawer.test.tsx`:
- Around line 54-57: The inline metadata object assigned to the variable minimal
(created via makeMessage) exceeds the 88-character line limit; reformat the
metadata value for minimal so each key/value pair is on its own line (or
otherwise wrapped across multiple lines) to keep lines under 88 chars, e.g.,
break the object literal passed to makeMessage('msg-2', { ... }) into multiple
lines so metadata: { task_id: null, project_id: null, tokens_used: null,
cost_usd: null, extra: [] } becomes a wrapped object while preserving keys and
values.
In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx`:
- Around line 60-65: The test line is over the 88-character limit; split the
long JSX call so it stays under the limit by moving the filters prop onto its
own line or by extracting the filters object into a local const and passing that
const to MessageFilterBar; update the render call in the test for
MessageFilterBar (using defaultProps and filters) so each prop fits within 88
columns and run the test to verify no behavior change.
- Around line 72-79: The test line calling render(<MessageFilterBar
{...defaultProps} filters={{ type: 'delegation' }}
onFiltersChange={onFiltersChange} />) exceeds the 88-character limit; break the
JSX into multiple lines so each line is <=88 chars (e.g., put the component and
its props on separate lines) inside MessageFilterBar.test.tsx in the 'removes
filter when pill X is clicked' test, keeping the same props (defaultProps,
filters, onFiltersChange) and behavior (userEvent.setup, screen.getByLabelText,
expect call) unchanged.
- Around line 29-31: The test line in MessageFilterBar.test.tsx exceeds the
88-character limit; break the long render/assert line into shorter lines or
split props into a variable to keep each line under 88 characters. Locate the
render call in the test that uses MessageFilterBar with defaultProps, filters={{
type: 'delegation' }} and filteredCount={8} and refactor it (for example, assign
the props to a const like props = { ...defaultProps, filters: { type:
'delegation' }, filteredCount: 8 } and call render(<MessageFilterBar {...props}
/>) or split the JSX attributes across multiple lines) so no line exceeds 88
chars while preserving the same behavior and the subsequent
expect(screen.getByText('8 of 42')).toBeInTheDocument().
- Around line 81-88: The test line in MessageFilterBar.test.tsx exceeds the
88-character limit; split the long statement (the render(...) call that passes
defaultProps and filters) into multiple shorter lines so each stays under 88
chars and the test remains equivalent; specifically break out the JSX props or
move filters and onFiltersChange to separate lines in the it block (referencing
the render call and the onFiltersChange variable) to satisfy the line-length
rule without changing test behavior.
In `@web/src/__tests__/pages/messages/MessageList.test.tsx`:
- Around line 31-39: The test name asserts "date dividers" but only checks
messages; update the MessageList test that uses makeMessage to also assert the
date divider is rendered: either check for the expected formatted date string
for the provided timestamps (e.g., expect(screen.getByText('Mar 28,
2026')).toBeInTheDocument()) or query a divider element (e.g.,
expect(screen.getAllByTestId('date-divider')).toHaveLength(1)) so the test
verifies the divider output in addition to the message texts.
- Around line 33-34: Lines with long JSX/object literals (e.g., the makeMessage
calls in MessageList.test.tsx) exceed the 88-character limit; break those long
lines into multiple shorter lines so each line is <=88 chars. Specifically, for
the makeMessage('1', { timestamp: '2026-03-28T10:00:00Z', content: 'First
message' }) and makeMessage('2', { timestamp: '2026-03-28T14:00:00Z', content:
'Second message' }) calls, place the object properties on their own indented
lines (timestamp and content each on separate lines) or put each makeMessage
call on its own line with the object formatted across multiple lines; apply the
same wrapping style to the other long lines mentioned (lines containing
makeMessage or long JSX at the reported locations) so all lines conform to the
88-character limit.
In `@web/src/__tests__/pages/MessagesPage.test.tsx`:
- Around line 29-32: The test uses an indirect dynamic property name when
mocking useMessagesData which is fragile; replace that pattern by hoisting the
stub with vi.hoisted and returning a clear named export in the mock: use
vi.hoisted to create/get a getMessagesData mock (e.g., const { getMessagesData }
= vi.hoisted(() => ({ getMessagesData: vi.fn(() => hookReturn) }))), then change
the vi.mock callback to return an object with a concrete useMessagesData
property that calls getMessagesData(), ensuring the mock is hoisted and easy to
understand; update references to useMessagesData and getMessagesData
accordingly.
In `@web/src/hooks/useMessagesData.ts`:
- Around line 48-49: The code uses two separate useMessagesStore selectors
(accessing channels and channelsLoading) which creates two subscriptions and can
cause extra re-renders; consolidate them into a single selector call that
returns both properties at once (e.g., an object with channels and
channelsLoading) and use shallow equality comparison to avoid unnecessary
renders—update the selector usage in useMessagesData.ts (replace the two
useMessagesStore calls for channels and channelsLoading with one combined
selector) so related state changes are batched into a single subscription.
In `@web/src/pages/messages/ChannelListItem.tsx`:
- Around line 40-43: The unread badge currently exposes only a bare number;
update the span that renders unreadCount in ChannelListItem (the element
containing {unreadCount}) to provide semantic context for screen readers by
adding an accessible label—either add an aria-label like "{unreadCount} unread"
(or "{unreadCount} unread messages") on the same span, or include
visually-hidden text (e.g., a nested sr-only span) that reads the count plus the
word "unread"; ensure the numeric display remains visually unchanged and that
the accessible text updates with unreadCount.
In `@web/src/pages/messages/ChannelSidebar.tsx`:
- Line 32: The long className strings in the ChannelSidebar component (e.g., the
div with className "mb-1 px-2 text-[10px] font-semibold uppercase tracking-wider
text-muted-foreground" and the other className usages reported) exceed the
88-char rule; split those className values across lines to keep each source line
<=88 chars by using a multiline template literal (className={`...`} with
segments on separate lines) or an array joined pattern (className={[ 'mb-1',
'px-2', 'text-[10px]', ... ].join(' ')}), updating the className on the
identified div and the other long className occurrences in the ChannelSidebar
component.
In `@web/src/pages/messages/MessageBubble.stories.tsx`:
- Around line 38-40: The UrgentPriority story defines a single-line args object
that exceeds the 88-character limit; update the Story named UrgentPriority by
breaking the args.message object across multiple lines (reference symbols:
UrgentPriority, args, message, baseMessage) so each property (e.g., priority,
type, content) is on its own line and no line is longer than 88 characters,
preserving the same values and structure.
In `@web/src/pages/messages/MessageBubble.tsx`:
- Around line 31-34: The MessageBubble currently always renders a <button> with
the onClick prop even when onClick is undefined; change the JSX to render a
non-interactive container (e.g., a <div> with the same className computed by
cn(...) and role/presentation) or render the <button> with disabled when onClick
is absent so we do not expose interactive semantics without a handler; update
the component that uses the onClick prop (the element around className={cn(...)}
in MessageBubble.tsx) to conditionally choose between button (when onClick is
provided) and a non-interactive element or set disabled on the button when
onClick is undefined, preserving styling and accessibility attributes.
In `@web/src/pages/messages/MessageDetailDrawer.stories.tsx`:
- Line 14: The long single-line test fixture string assigned to the content
property in MessageDetailDrawer.stories.tsx exceeds the 88-char limit; split it
into a template literal or multiple shorter strings (e.g., use a backtick
multi-line template literal for content or an array of lines joined with '\n')
so each source line stays under 88 chars while preserving the exact text,
updating the content assignment where it appears in the story file.
In `@web/src/pages/messages/MessageFilterBar.tsx`:
- Line 91: The JSX line rendering the Search icon in MessageFilterBar.tsx
exceeds the 88-char limit; break the long line into multiple shorter lines
(e.g., place the component props each on their own line or extract the className
string to a const) so the Search element (<Search ... />) is wrapped to stay
under 88 characters while preserving the same attributes (className and
aria-hidden).
- Around line 8-11: The MESSAGE_TYPES constant currently has a line that exceeds
the 88-character limit; update the array declaration (MESSAGE_TYPES:
MessageType[]) to wrap items so no single line is longer than 88 characters —
e.g., place multiple entries per line or one entry per line while preserving the
same items and order (references: MESSAGE_TYPES, MessageType) and ensure
formatting/commas remain valid for the TSX file.
In `@web/src/pages/messages/MessageList.stories.tsx`:
- Around line 7-15: Several object literal lines in the MessageList stories
exceed the 88-char limit; update the long Message entries in the sampleMessages
array (and the baseMeta/threadMeta declarations if needed) to wrap string
literals and object properties across multiple lines so each line is <=88 chars
— for example break long content/timestamp/metadata fields onto separate lines
and align property entries for messages with id '1'..'5' to keep each line under
the limit while preserving the same keys and values.
In `@web/src/pages/messages/MessageList.tsx`:
- Around line 52-56: The client-side sort in MessageList (the useMemo that
produces sorted from messages) can be skipped when the server already returns
messages in ascending timestamp order; update the component to accept or detect
a preSorted flag (or rely on the API contract) and only perform the useMemo sort
when preSorted is false or absent, otherwise pass messages through directly to
avoid the expensive [...messages].sort operation on large lists (adjust the
sorted variable logic and any prop/type for preSorted accordingly).
In `@web/src/pages/messages/MessageThread.stories.tsx`:
- Around line 16-29: Several message object literal lines in
MessageThread.stories.tsx (e.g., the objects with id 'msg-1', 'msg-2', 'msg-3')
have long content string literals that exceed the 88-char line limit; split each
long content value into multiple shorter string literals (via string
concatenation or short template literals) so no source line exceeds 88
characters, keeping the surrounding properties (sender, channel, attachments,
metadata) unchanged and reformatting only the content lines; after edits, run
the linter/formatter to verify all lines in these message objects comply with
the 88-char rule.
In `@web/src/pages/MessagesPage.tsx`:
- Around line 124-127: The current render gate only shows MessagesSkeleton when
all four flags are true (loading && messages.length === 0 && channelsLoading &&
channels.length === 0), which can hide the skeleton if either messages or
channels are cached; update the condition in MessagesPage to show the skeleton
when either resource is loading with no cached items (e.g., use (loading &&
messages.length === 0) || (channelsLoading && channels.length === 0)) so
MessagesSkeleton appears whenever messages or channels are still loading and no
cached data exists; adjust the condition that currently references loading,
messages.length, channelsLoading, channels.length and keep returning
<MessagesSkeleton /> in that branch.
- Around line 51-60: The effect in MessagesPage.tsx directly calls
useMessagesStore.setState to clear newMessageIds, bypassing the store's action
layer; add a clearNewMessageIds action to the messages store (e.g., export
function or method named clearNewMessageIds on useMessagesStore) that sets
newMessageIds to an empty Set<string>, then replace the direct setState call in
the useEffect with useMessagesStore.getState().clearNewMessageIds() or the
equivalent action call so all mutations go through the store's action API (keep
the 2s timeout and cleanup logic unchanged).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 935cf8d6-f926-4f86-8a2d-bd61d3ddbdad
📒 Files selected for processing (41)
docs/design/communication.mddocs/design/page-structure.mdweb/src/__tests__/helpers/factories.tsweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/stores/messages.test.tsweb/src/__tests__/utils/messages.test.tsweb/src/hooks/useMessagesData.tsweb/src/pages/MessagesPage.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/stores/messages.tsweb/src/utils/messages.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Dashboard Test
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
Use camelCase for variable names and function identifiers
Files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/helpers/factories.tsweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/hooks/useMessagesData.tsweb/src/__tests__/utils/messages.test.tsweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/stores/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsxweb/src/utils/messages.tsweb/src/__tests__/stores/messages.test.ts
web/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.tsx: ALWAYS reuse existing shared components fromweb/src/components/ui/before creating new ones (e.g.,StatusBadge,MetricCard,AgentCard,DeptHealthBar,SectionCard)
Use Tailwind semantic classes (text-foreground,bg-card,text-accent,text-success) or CSS variables (var(--so-accent)); NEVER hardcode hex values or rgba() in .tsx files
Usefont-sansorfont-monofor typography (maps to Geist tokens); NEVER setfontFamilydirectly in CSS
Use density-aware spacing tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Do NOT recreate complex (>8 line) JSX inside.map()blocks; extract to a shared component inweb/src/components/ui/
Files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsx
**/*.{py,ts,tsx,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Lines must not exceed 88 characters (enforced by ruff for Python, configured in web/.eslintrc for TypeScript)
Files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/helpers/factories.tsweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/hooks/useMessagesData.tsweb/src/__tests__/utils/messages.test.tsweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/stores/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsxweb/src/utils/messages.tsweb/src/__tests__/stores/messages.test.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Import
cnfrom@/lib/utilsfor conditional class merging in components
Files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/helpers/factories.tsweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/hooks/useMessagesData.tsweb/src/__tests__/utils/messages.test.tsweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/stores/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsxweb/src/utils/messages.tsweb/src/__tests__/stores/messages.test.ts
web/src/pages/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Lazy-load page components and place page-scoped sub-components in
pages/<page-name>/subdirectories (e.g.,pages/tasks/,pages/org-edit/)
Files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsx
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When an implementation deviates from the spec (better approach found, scope evolved, etc.), update the relevant
docs/design/page to reflect the new reality
Files:
docs/design/communication.mddocs/design/page-structure.md
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation must be written in Markdown and built with Zensical (config:
mkdocs.yml); design spec lives indocs/design/(11 pages linked fromdocs/DESIGN_SPEC.md)
Files:
docs/design/communication.mddocs/design/page-structure.md
🧠 Learnings (14)
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Do NOT recreate complex (>8 line) JSX inside `.map()` blocks; extract to a shared component in `web/src/components/ui/`
Applied to files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsxweb/src/utils/messages.ts
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : ALWAYS reuse existing shared components from `web/src/components/ui/` before creating new ones (e.g., `StatusBadge`, `MetricCard`, `AgentCard`, `DeptHealthBar`, `SectionCard`)
Applied to files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/ChannelSidebar.tsx
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to web/src/__tests__/**/*.{ts,js} : Dashboard testing: Vitest unit tests organized by feature under `web/src/__tests__/`. Use fast-check for property-based testing (`fc.assert` + `fc.property`).
Applied to files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/utils/messages.test.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/stores/messages.test.ts
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : New shared components must have a corresponding `.stories.tsx` Storybook file with all component states (default, hover, loading, error, empty)
Applied to files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/__tests__/stores/messages.test.ts
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : Export component props as a TypeScript interface
Applied to files:
web/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : Use design token variables exclusively for shadows and borders (`var(--so-shadow-card-hover)`, `border-border`, `border-bright`); never hardcode shadow or border values
Applied to files:
web/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to **/*.{py,ts,tsx,go} : Lines must not exceed 88 characters (enforced by ruff for Python, configured in web/.eslintrc for TypeScript)
Applied to files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/utils/messages.ts
📚 Learning: 2026-03-21T11:08:01.542Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T11:08:01.542Z
Learning: Applies to **/*.{py,md} : Line length: 88 characters (ruff).
Applied to files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/utils/messages.ts
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to docs/design/*.md : Design spec pages: 7 pages in `docs/design/` — index, agents, organization, communication, engine, memory, operations
Applied to files:
docs/design/page-structure.md
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Use density-aware spacing tokens (`p-card`, `gap-section-gap`, `gap-grid-gap`) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Applied to files:
web/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Use Tailwind semantic classes (`text-foreground`, `bg-card`, `text-accent`, `text-success`) or CSS variables (`var(--so-accent)`); NEVER hardcode hex values or rgba() in .tsx files
Applied to files:
web/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to **/*.{ts,tsx} : Prefer `interface` for defining object shapes in TypeScript
Applied to files:
web/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageDetailDrawer.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/pages/**/*.tsx : Lazy-load page components and place page-scoped sub-components in `pages/<page-name>/` subdirectories (e.g., `pages/tasks/`, `pages/org-edit/`)
Applied to files:
web/src/pages/messages/ChannelSidebar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to **/*.{ts,tsx} : Use camelCase for variable names and function identifiers
Applied to files:
web/src/utils/messages.ts
🔇 Additional comments (50)
docs/design/communication.md (2)
126-126: LGTM: Correctly demonstrates nullable metadata field.Changing
project_idtonullaccurately reflects the implementation whereMessageMetadata.project_idis declared asNotBlankStr | None(src/synthorg/communication/message.py:50-54).
128-129: LGTM: Correctly demonstrates the extensibility mechanism.The
extrafield addition with format[["model", "example-medium-001"]]accurately reflects the implementation whereMessageMetadata.extrais defined astuple[tuple[str, str], ...](src/synthorg/communication/message.py:67-70). JSON serialization of tuples produces arrays, making the nested array format correct.The trailing comma on line 128 is a standard JSON formatting convention that facilitates future additions.
web/src/pages/messages/AttachmentList.tsx (2)
19-31: The.map()block still exceeds 8 lines of JSX.The mapped JSX (lines 21-30) remains complex. A previous review requested extracting this to a shared component in
web/src/components/ui/.
1-34: Overall implementation is sound.Good use of:
Record<AttachmentType, ...>for exhaustive icon mapping (TypeScript will error if a new type is added)readonly Attachment[]for immutability- Semantic Tailwind classes (
border-border,bg-surface,text-secondary)aria-hidden="true"on decorative icons- Early return for empty array
The collision-safe key with index fallback is acceptable given the ESLint suppression comment explaining the rationale.
web/src/utils/messages.ts (4)
6-17: Constant maps still use SCREAMING_SNAKE_CASE.This was flagged in a previous review. The coding guidelines specify camelCase for variable names in
**/*.{ts,tsx}files.
137-156: Thread grouping implementation is correct.The access to
msg.metadata.task_id(line 142) is safe becauseMessage.metadatais a required field (not nullable) per the type definition. Onlytask_idwithin metadata can benull, which theif (!taskId)check on line 143 correctly handles.
166-191: Clean client-side filtering implementation.The filter logic correctly:
- Short-circuits on missing filter values
- Uses case-insensitive search across
content,sender, andto- Returns a new array (immutable)
72-99: Date grouping utilities are well-implemented.Good handling of:
- Invalid timestamps (
'unknown'fallback)- Chronological order preservation via
Map- Efficient single-pass grouping
web/src/pages/messages/TimestampDivider.tsx (1)
1-13: Clean, accessible implementation.Good use of
role="separator"for accessibility, semantic Tailwind classes (border-border,text-muted-foreground), andfont-monofor typography consistency.web/src/pages/messages/TimestampDivider.stories.tsx (1)
1-16: Good Storybook coverage.The three stories (
Today,Yesterday,DateLabel) appropriately cover the primary use cases for this simple presentational component. Thea11y: { test: 'error' }configuration ensures accessibility regressions are caught.web/src/pages/messages/AttachmentList.stories.tsx (1)
1-31: Good Storybook coverage for all attachment scenarios.The stories cover:
MixedTypes: All threeAttachmentTypevalues (artifact,file,link)SingleArtifact: Single-item renderingEmpty: Validates thenullreturn behaviorThis ensures visual regression testing across the component's main states.
web/src/__tests__/pages/messages/AttachmentList.test.tsx (1)
1-36: Solid test coverage for the component's core behaviors.The tests appropriately verify:
- Empty state returns
null- Attachment
reftext is rendered- Multiple attachments render correctly
- Each attachment has an SVG icon
web/src/stores/messages.ts (3)
95-122: Race protection forfetchMoreMessagesis now in place.The implementation correctly captures the current
messageRequestSeq(line 98) and discards stale results whenfetchMessageshas been called in the interim (channel switch). The approach differs slightly from the original suggestion (no increment) but achieves the same goal: stale "load more" results are discarded when the user switches channels.
124-184: WebSocket validation is now comprehensive.The handler validates all required
Messagefields:id,timestamp,sender,to,channel,content,type,priority, plus structural checks forattachments(array) andmetadata(non-null object). Malformed payloads are logged and skipped, preventing runtime errors downstream.
49-63: Clean Zustand store structure.Good use of:
- Typed state interface with clear separation of concerns (channels, messages, unread, threads)
Set<string>for O(1) lookups inexpandedThreadsandnewMessageIds- Module-level sequence counters for request deduplication
web/src/__tests__/helpers/factories.ts (1)
127-156: Well-structured test factories.The
makeMessageandmakeChannelfactories:
- Follow the established pattern of other factories in this file
- Provide sensible defaults for all required fields
- Allow overrides via spread operator
- Correctly initialize all nullable
metadatafields asnullandextraas an empty array, matching theMessageMetadatatype definitionweb/src/__tests__/utils/messages.test.ts (1)
1-201: LGTM! Comprehensive test coverage for message utilities.The test suite thoroughly covers all exported utility functions with good edge case handling:
- Label/color mapping functions tested with all expected inputs
- Date grouping handles today, yesterday, older dates, and invalid inputs
- Thread grouping correctly separates threaded vs standalone messages
- Filter logic covers individual filters, combined filters, and empty filter objects
The use of
makeMessagefactory keeps tests clean and maintainable.web/src/pages/messages/MessageTypeBadge.tsx (1)
1-22: LGTM! Clean, well-typed component.The component correctly:
- Uses
interfacefor props definition- Imports
cnfrom@/lib/utilsfor class merging- Uses semantic Tailwind classes (
bg-surface,text-secondary,border-border)- Delegates label logic to the utility function
web/src/pages/messages/MessageTypeBadge.stories.tsx (2)
14-25: Previous line length issue addressed.The
ALL_TYPESarray is now properly formatted with one entry per line, complying with the 88-character line limit.
27-39: Good Storybook coverage.The stories effectively demonstrate all message type variants via the
AllTypesgrid and provide focused examples for common types.web/src/pages/messages/MessageFilterBar.tsx (1)
145-164: Previous inline props issue addressed.The
FilterPillPropsinterface is now properly defined, aligning with the project's TypeScript conventions.web/src/pages/messages/MessageFilterBar.stories.tsx (1)
1-33: LGTM! Good Storybook coverage for filter states.The stories effectively cover the key filter UI states:
- Empty filters (default view)
- Single filter active (type, search)
- Multiple filters active
The a11y testing configuration and centered decorator are appropriate.
web/src/__tests__/pages/messages/MessageFilterBar.test.tsx (1)
5-94: Test coverage is comprehensive.The test suite covers all key behaviors: control rendering, count display logic, filter change callbacks, pill rendering/removal, and the "Clear all" action. Good use of
userEventfor realistic interaction simulation.web/src/pages/messages/MessageDetailDrawer.tsx (3)
10-14: Previous inline props issues addressed.All component props now use properly named interfaces (
MessageDetailDrawerProps,MessageDetailContentProps,MetadataRowProps), aligning with TypeScript conventions.
80-88: Previous duplicate key issue addressed.The
metadata.extramapping now uses index-based keys (extra-${i}-${key}) with an appropriate ESLint disable comment explaining the rationale.
28-101: Well-structured component with good patterns.The implementation:
- Properly reuses shared
DrawerandAvatarcomponents- Uses semantic Tailwind classes throughout
- Handles nullable metadata fields correctly with conditional rendering
- Extracts
MetadataRowhelper to keep the metadata grid DRYweb/src/pages/messages/MessageDetailDrawer.stories.tsx (2)
53-72: Previous duplication concern addressed.The
WithAttachmentsstory now focuses specifically on the attachments scenario with minimal metadata (all fields null/empty) and includes an additional attachment type (link), differentiating it fromFullMetadata.
1-76: Good story coverage for drawer states.The stories effectively cover all meaningful states: full metadata, minimal metadata, attachment-focused view, and closed/null state. The a11y testing is properly configured.
web/src/__tests__/pages/messages/MessageThread.test.tsx (1)
22-81: Good coverage for thread states and interactions.This suite validates collapsed/expanded/single/empty rendering and both callback
paths (onToggle,onSelectMessage) clearly.web/src/__tests__/pages/messages/ChannelSidebar.test.tsx (1)
26-66: Sidebar behavior coverage looks solid.Grouping, active-state accessibility, unread-badge behavior, and loading/empty
states are all covered with focused assertions.docs/design/page-structure.md (1)
86-100: Design spec update is aligned with implementation.The
/messagesbehavior and deep-link query params are now documented at the
right level of detail.Also applies to: 238-241
web/src/__tests__/hooks/useMessagesData.test.ts (1)
64-160: Hook integration coverage is strong.The suite exercises the important lifecycle paths (fetching, polling start/stop,
websocket bindings, and derived state) with clear expectations.web/src/pages/MessagesPage.tsx (2)
187-196:newMessageIdsprop is now correctly passed.This addresses the previous review comment about missing new-message highlighting. The prop is wired through from
useMessagesDataand passed toMessageList.
48-49: Ref mutation during render is acceptable here.Unlike the previous review comment about
setState, this is a ref mutation (wasConnectedRef.current = true), which is safe during render. Refs are mutable and don't trigger re-renders. The pattern correctly tracks "was ever connected" state.web/src/pages/messages/MessageBubble.stories.tsx (1)
1-66: Good Storybook coverage for MessageBubble.The stories comprehensively cover the key render states: default, priority variants, attachments, message types, and new-message flash. The a11y testing configuration and decorator setup are well-structured.
web/src/hooks/useMessagesData.ts (2)
40-46: Good fix for the stale closure issue.The
activeChannelRefpattern correctly addresses the stale closure problem in WebSocket handlers. SinceuseWebSocketregisters bindings once on mount, readingactiveChannelvia ref ensures the handler always accesses the current value.
90-102: WebSocket bindings are correctly stable.The empty dependency array for
bindingsis intentional and correct—handlers readactiveChannelvia ref, so they don't need to be recreated whenactiveChannelchanges.web/src/pages/messages/ChannelSidebar.stories.tsx (2)
1-48: Good story coverage for sidebar states.The stories cover the essential UI states: default, active channel highlight, unread badges, loading skeleton, and empty state. The a11y configuration ensures accessibility testing.
15-21: No issues found. The stories correctly pass all required props that match the component interface:channels,activeChannel,unreadCounts,onSelectChannel, andloading. All prop names and types align with the component's actualChannelSidebarPropsinterface.web/src/__tests__/pages/MessagesPage.test.tsx (2)
1-186: Comprehensive test coverage for MessagesPage.The tests cover essential scenarios: loading states, error handling, WebSocket connection states, channel selection, message rendering, filtering, and edge cases. Good use of factories for test data.
158-171: No action required. The test assertion correctly matches themakeMessagefactory output. WhenmakeMessage('1')is called, it produces content'Message 1 content'via the template stringMessage ${id} content, which is exactly what the assertion expects.web/src/pages/messages/MessageThread.tsx (3)
16-34: Clean handling of single-message case.The early return for single messages (or empty) avoids unnecessary thread UI, and the null check on line 26 prevents potential runtime errors.
47-64: Good accessibility on thread toggle.The button properly uses
aria-expandedto communicate state, and the chevron icon is markedaria-hidden. The button text changes contextually between "Collapse thread" and "{n} more in thread".
66-88: Animation implementation looks correct.
AnimatePresencewithinitial={false}prevents animation on first render, and the height/opacity transitions provide smooth expand/collapse behavior.web/src/pages/messages/MessageList.tsx (3)
42-50: Auto-scroll logic is well-implemented.The pattern of tracking
wasAtBottomRefvia scroll handler and conditionally scrolling on new messages respects user scroll position while still auto-scrolling for users at the bottom.
70-77: Good accessibility setup.
aria-live="polite"ensures screen readers announce new messages without interrupting, andaria-label="Messages"provides a clear landmark label.
102-111: Props correctly match MessageThread interface.The props passed (
messages,expanded,onToggle,onSelectMessage,newMessageIds) align with theMessageThreadPropsinterface from context snippet 1.web/src/__tests__/stores/messages.test.ts (3)
68-90: Excellent test for race condition handling.This test properly validates that rapid channel switching discards stale responses. The promise-based approach to control resolution order is a robust pattern for testing async race conditions.
1-290: Comprehensive store test coverage.The tests thoroughly cover success/error paths, race conditions, deduplication, thread toggling, and unread management. The use of factories and careful state reset between tests ensures reliable, isolated test runs.
174-181: No additional event type testing is needed. ThehandleWsEventfunction is specific to the messages store and only handles themessage.sentevent type, which is the only message-related event type defined inWsEventType. Other event types likemessage.deletedorchannel.updateddo not exist in the system and would not be handled by this store anyway.> Likely an incorrect or invalid review comment.
docs/design/communication.md
Outdated
| } | ||
| ``` | ||
|
|
||
| All metadata fields are nullable. The `extra` field contains additional key-value pairs for extensibility. |
There was a problem hiding this comment.
Minor: Imprecise statement about nullable fields.
The statement "All metadata fields are nullable" is not entirely accurate. The extra field is not nullable - it has a default value of () (empty tuple) and is always present (src/synthorg/communication/message.py:67-70):
extra: tuple[tuple[str, str], ...] = Field(
default=(),
description="Immutable key-value pairs for arbitrary metadata",
)📝 Suggested refinement for precision
-All metadata fields are nullable. The `extra` field contains additional key-value pairs for extensibility.
+The metadata fields `task_id`, `project_id`, `tokens_used`, and `cost_usd` are nullable. The `extra` field contains additional key-value pairs for extensibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/communication.md` at line 134, Update the docs text to correct
the inaccurate "All metadata fields are nullable" claim: state that all metadata
fields are nullable except the extra field, which is non-nullable and has a
default empty tuple (extra: tuple[...] = Field(default=(), ...)); revise the
sentence to note that extra is always present as an immutable empty tuple when
no entries exist.
| const minimal = makeMessage('msg-2', { | ||
| sender: 'bob', | ||
| metadata: { task_id: null, project_id: null, tokens_used: null, cost_usd: null, extra: [] }, | ||
| }) |
There was a problem hiding this comment.
Wrap the minimal metadata object to satisfy the 88-char limit.
This inline object is over the repo’s line-length cap and may fail lint.
Suggested fix
it('hides optional metadata when null', () => {
const minimal = makeMessage('msg-2', {
sender: 'bob',
- metadata: { task_id: null, project_id: null, tokens_used: null, cost_usd: null, extra: [] },
+ metadata: {
+ task_id: null,
+ project_id: null,
+ tokens_used: null,
+ cost_usd: null,
+ extra: [],
+ },
})As per coding guidelines: **/*.{py,ts,tsx,go}: Lines must not exceed 88 characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const minimal = makeMessage('msg-2', { | |
| sender: 'bob', | |
| metadata: { task_id: null, project_id: null, tokens_used: null, cost_usd: null, extra: [] }, | |
| }) | |
| const minimal = makeMessage('msg-2', { | |
| sender: 'bob', | |
| metadata: { | |
| task_id: null, | |
| project_id: null, | |
| tokens_used: null, | |
| cost_usd: null, | |
| extra: [], | |
| }, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/__tests__/pages/messages/MessageDetailDrawer.test.tsx` around lines
54 - 57, The inline metadata object assigned to the variable minimal (created
via makeMessage) exceeds the 88-character line limit; reformat the metadata
value for minimal so each key/value pair is on its own line (or otherwise
wrapped across multiple lines) to keep lines under 88 chars, e.g., break the
object literal passed to makeMessage('msg-2', { ... }) into multiple lines so
metadata: { task_id: null, project_id: null, tokens_used: null, cost_usd: null,
extra: [] } becomes a wrapped object while preserving keys and values.
| render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} filteredCount={8} />) | ||
| expect(screen.getByText('8 of 42')).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
Line 29 exceeds 88-character limit.
Suggested fix
it('shows filtered count when filters active', () => {
- render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} filteredCount={8} />)
+ render(
+ <MessageFilterBar
+ {...defaultProps}
+ filters={{ type: 'delegation' }}
+ filteredCount={8}
+ />
+ )
expect(screen.getByText('8 of 42')).toBeInTheDocument()
})As per coding guidelines: **/*.{py,ts,tsx,go} lines must not exceed 88 characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} filteredCount={8} />) | |
| expect(screen.getByText('8 of 42')).toBeInTheDocument() | |
| }) | |
| it('shows filtered count when filters active', () => { | |
| render( | |
| <MessageFilterBar | |
| {...defaultProps} | |
| filters={{ type: 'delegation' }} | |
| filteredCount={8} | |
| /> | |
| ) | |
| expect(screen.getByText('8 of 42')).toBeInTheDocument() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx` around lines 29 -
31, The test line in MessageFilterBar.test.tsx exceeds the 88-character limit;
break the long render/assert line into shorter lines or split props into a
variable to keep each line under 88 characters. Locate the render call in the
test that uses MessageFilterBar with defaultProps, filters={{ type: 'delegation'
}} and filteredCount={8} and refactor it (for example, assign the props to a
const like props = { ...defaultProps, filters: { type: 'delegation' },
filteredCount: 8 } and call render(<MessageFilterBar {...props} />) or split the
JSX attributes across multiple lines) so no line exceeds 88 chars while
preserving the same behavior and the subsequent expect(screen.getByText('8 of
42')).toBeInTheDocument().
| it('renders filter pills for active filters', () => { | ||
| render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation', priority: 'high' }} />) | ||
| // Filter pills have remove buttons with accessible labels | ||
| expect(screen.getByLabelText('Remove Delegation filter')).toBeInTheDocument() | ||
| expect(screen.getByLabelText('Remove High filter')).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
Line 61 exceeds 88-character limit.
Suggested fix
it('renders filter pills for active filters', () => {
- render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation', priority: 'high' }} />)
+ render(
+ <MessageFilterBar
+ {...defaultProps}
+ filters={{ type: 'delegation', priority: 'high' }}
+ />
+ )
// Filter pills have remove buttons with accessible labelsAs per coding guidelines: **/*.{py,ts,tsx,go} lines must not exceed 88 characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('renders filter pills for active filters', () => { | |
| render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation', priority: 'high' }} />) | |
| // Filter pills have remove buttons with accessible labels | |
| expect(screen.getByLabelText('Remove Delegation filter')).toBeInTheDocument() | |
| expect(screen.getByLabelText('Remove High filter')).toBeInTheDocument() | |
| }) | |
| it('renders filter pills for active filters', () => { | |
| render( | |
| <MessageFilterBar | |
| {...defaultProps} | |
| filters={{ type: 'delegation', priority: 'high' }} | |
| /> | |
| ) | |
| // Filter pills have remove buttons with accessible labels | |
| expect(screen.getByLabelText('Remove Delegation filter')).toBeInTheDocument() | |
| expect(screen.getByLabelText('Remove High filter')).toBeInTheDocument() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx` around lines 60 -
65, The test line is over the 88-character limit; split the long JSX call so it
stays under the limit by moving the filters prop onto its own line or by
extracting the filters object into a local const and passing that const to
MessageFilterBar; update the render call in the test for MessageFilterBar (using
defaultProps and filters) so each prop fits within 88 columns and run the test
to verify no behavior change.
| it('removes filter when pill X is clicked', async () => { | ||
| const user = userEvent.setup() | ||
| const onFiltersChange = vi.fn() | ||
| render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} onFiltersChange={onFiltersChange} />) | ||
|
|
||
| await user.click(screen.getByLabelText('Remove Delegation filter')) | ||
| expect(onFiltersChange).toHaveBeenCalledWith({ type: undefined }) | ||
| }) |
There was a problem hiding this comment.
Line 75 exceeds 88-character limit.
Suggested fix
it('removes filter when pill X is clicked', async () => {
const user = userEvent.setup()
const onFiltersChange = vi.fn()
- render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} onFiltersChange={onFiltersChange} />)
+ render(
+ <MessageFilterBar
+ {...defaultProps}
+ filters={{ type: 'delegation' }}
+ onFiltersChange={onFiltersChange}
+ />
+ )As per coding guidelines: **/*.{py,ts,tsx,go} lines must not exceed 88 characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('removes filter when pill X is clicked', async () => { | |
| const user = userEvent.setup() | |
| const onFiltersChange = vi.fn() | |
| render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} onFiltersChange={onFiltersChange} />) | |
| await user.click(screen.getByLabelText('Remove Delegation filter')) | |
| expect(onFiltersChange).toHaveBeenCalledWith({ type: undefined }) | |
| }) | |
| it('removes filter when pill X is clicked', async () => { | |
| const user = userEvent.setup() | |
| const onFiltersChange = vi.fn() | |
| render( | |
| <MessageFilterBar | |
| {...defaultProps} | |
| filters={{ type: 'delegation' }} | |
| onFiltersChange={onFiltersChange} | |
| /> | |
| ) | |
| await user.click(screen.getByLabelText('Remove Delegation filter')) | |
| expect(onFiltersChange).toHaveBeenCalledWith({ type: undefined }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx` around lines 72 -
79, The test line calling render(<MessageFilterBar {...defaultProps} filters={{
type: 'delegation' }} onFiltersChange={onFiltersChange} />) exceeds the
88-character limit; break the JSX into multiple lines so each line is <=88 chars
(e.g., put the component and its props on separate lines) inside
MessageFilterBar.test.tsx in the 'removes filter when pill X is clicked' test,
keeping the same props (defaultProps, filters, onFiltersChange) and behavior
(userEvent.setup, screen.getByLabelText, expect call) unchanged.
|
|
||
| {/* Search */} | ||
| <div className="relative flex-1"> | ||
| <Search className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground" aria-hidden="true" /> |
There was a problem hiding this comment.
Line exceeds 88-character limit.
This JSX line is approximately 118 characters.
Suggested fix
- <Search className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground" aria-hidden="true" />
+ <Search
+ className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground"
+ aria-hidden="true"
+ />As per coding guidelines: **/*.{py,ts,tsx,go} lines must not exceed 88 characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Search className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground" aria-hidden="true" /> | |
| <Search | |
| className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground" | |
| aria-hidden="true" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/messages/MessageFilterBar.tsx` at line 91, The JSX line
rendering the Search icon in MessageFilterBar.tsx exceeds the 88-char limit;
break the long line into multiple shorter lines (e.g., place the component props
each on their own line or extract the className string to a const) so the Search
element (<Search ... />) is wrapped to stay under 88 characters while preserving
the same attributes (className and aria-hidden).
| const baseMeta: MessageMetadata = { task_id: null, project_id: null, tokens_used: null, cost_usd: null, extra: [] } | ||
| const threadMeta: MessageMetadata = { task_id: 'task-42', project_id: null, tokens_used: 500, cost_usd: 0.01, extra: [] } | ||
|
|
||
| const sampleMessages: Message[] = [ | ||
| { id: '1', timestamp: '2026-03-28T09:00:00Z', sender: 'alice', to: '#eng', type: 'announcement', priority: 'normal', channel: '#eng', content: 'Sprint planning in 30 minutes.', attachments: [], metadata: baseMeta }, | ||
| { id: '2', timestamp: '2026-03-28T10:00:00Z', sender: 'bob', to: '#eng', type: 'delegation', priority: 'normal', channel: '#eng', content: 'Please implement the auth endpoint.', attachments: [], metadata: threadMeta }, | ||
| { id: '3', timestamp: '2026-03-28T10:30:00Z', sender: 'carol', to: '#eng', type: 'task_update', priority: 'normal', channel: '#eng', content: 'Working on it.', attachments: [], metadata: threadMeta }, | ||
| { id: '4', timestamp: '2026-03-28T14:00:00Z', sender: 'carol', to: '#eng', type: 'review_request', priority: 'high', channel: '#eng', content: 'PR ready for review.', attachments: [{ type: 'artifact', ref: 'pr-42' }], metadata: threadMeta }, | ||
| { id: '5', timestamp: '2026-03-27T16:00:00Z', sender: 'dave', to: '#eng', type: 'status_report', priority: 'normal', channel: '#eng', content: 'EOD: Completed database migration.', attachments: [], metadata: baseMeta }, |
There was a problem hiding this comment.
Reflow long literals/callbacks to pass max-line lint.
Several lines in stories exceed 88 chars and should be wrapped.
♻️ Suggested refactor
-const baseMeta: MessageMetadata = { task_id: null, project_id: null, tokens_used: null, cost_usd: null, extra: [] }
-const threadMeta: MessageMetadata = { task_id: 'task-42', project_id: null, tokens_used: 500, cost_usd: 0.01, extra: [] }
+const baseMeta: MessageMetadata = {
+ task_id: null,
+ project_id: null,
+ tokens_used: null,
+ cost_usd: null,
+ extra: [],
+}
+const threadMeta: MessageMetadata = {
+ task_id: 'task-42',
+ project_id: null,
+ tokens_used: 500,
+ cost_usd: 0.01,
+ extra: [],
+}
@@
- toggleThread={(id) => setExpanded((s) => { const n = new Set(s); if (n.has(id)) n.delete(id); else n.add(id); return n })}
+ toggleThread={(id) =>
+ setExpanded((s) => {
+ const n = new Set(s)
+ if (n.has(id)) n.delete(id)
+ else n.add(id)
+ return n
+ })
+ }As per coding guidelines: "**/*.{py,ts,tsx,go}: Lines must not exceed 88
characters."
Also applies to: 35-35, 49-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/messages/MessageList.stories.tsx` around lines 7 - 15, Several
object literal lines in the MessageList stories exceed the 88-char limit; update
the long Message entries in the sampleMessages array (and the
baseMeta/threadMeta declarations if needed) to wrap string literals and object
properties across multiple lines so each line is <=88 chars — for example break
long content/timestamp/metadata fields onto separate lines and align property
entries for messages with id '1'..'5' to keep each line under the limit while
preserving the same keys and values.
| // Sort messages ascending by timestamp for display | ||
| const sorted = useMemo( | ||
| () => [...messages].sort((a, b) => a.timestamp.localeCompare(b.timestamp)), | ||
| [messages], | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Timestamp sorting on every render could be optimized.
The useMemo correctly memoizes on [messages], but for very large message lists, the sort could be expensive. Consider whether the API could return pre-sorted data to avoid client-side sorting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/messages/MessageList.tsx` around lines 52 - 56, The client-side
sort in MessageList (the useMemo that produces sorted from messages) can be
skipped when the server already returns messages in ascending timestamp order;
update the component to accept or detect a preSorted flag (or rely on the API
contract) and only perform the useMemo sort when preSorted is false or absent,
otherwise pass messages through directly to avoid the expensive
[...messages].sort operation on large lists (adjust the sorted variable logic
and any prop/type for preSorted accordingly).
| id: 'msg-1', timestamp: '2026-03-28T10:00:00Z', sender: 'sarah_chen', to: '#engineering', | ||
| type: 'delegation', priority: 'normal', channel: '#engineering', | ||
| content: 'Please implement the user auth endpoint.', attachments: [], metadata: sampleMetadata, | ||
| }, | ||
| { | ||
| id: 'msg-2', timestamp: '2026-03-28T10:30:00Z', sender: 'bob_dev', to: '#engineering', | ||
| type: 'task_update', priority: 'normal', channel: '#engineering', | ||
| content: 'Started working on it. Draft PR incoming.', attachments: [], metadata: sampleMetadata, | ||
| }, | ||
| { | ||
| id: 'msg-3', timestamp: '2026-03-28T11:00:00Z', sender: 'bob_dev', to: '#engineering', | ||
| type: 'review_request', priority: 'normal', channel: '#engineering', | ||
| content: 'PR ready for review.', attachments: [{ type: 'artifact', ref: 'pr-42' }], metadata: sampleMetadata, | ||
| }, |
There was a problem hiding this comment.
Break long message literals to satisfy the 88-char limit.
Several object-literal lines exceed the configured max length and risk lint
failure.
♻️ Suggested formatting
- id: 'msg-1', timestamp: '2026-03-28T10:00:00Z', sender: 'sarah_chen', to: '#engineering',
- type: 'delegation', priority: 'normal', channel: '#engineering',
- content: 'Please implement the user auth endpoint.', attachments: [], metadata: sampleMetadata,
+ id: 'msg-1',
+ timestamp: '2026-03-28T10:00:00Z',
+ sender: 'sarah_chen',
+ to: '#engineering',
+ type: 'delegation',
+ priority: 'normal',
+ channel: '#engineering',
+ content: 'Please implement the user auth endpoint.',
+ attachments: [],
+ metadata: sampleMetadata,As per coding guidelines: "**/*.{py,ts,tsx,go}: Lines must not exceed 88
characters."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/messages/MessageThread.stories.tsx` around lines 16 - 29,
Several message object literal lines in MessageThread.stories.tsx (e.g., the
objects with id 'msg-1', 'msg-2', 'msg-3') have long content string literals
that exceed the 88-char line limit; split each long content value into multiple
shorter string literals (via string concatenation or short template literals) so
no source line exceeds 88 characters, keeping the surrounding properties
(sender, channel, attachments, metadata) unchanged and reformatting only the
content lines; after edits, run the linter/formatter to verify all lines in
these message objects comply with the 88-char rule.
| // Loading state | ||
| if (loading && messages.length === 0 && channelsLoading && channels.length === 0) { | ||
| return <MessagesSkeleton /> | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Loading condition may show skeleton briefly with cached data.
The condition loading && messages.length === 0 && channelsLoading && channels.length === 0 requires all four conditions. If channels are cached but messages aren't, users see the full UI with an empty message area rather than the skeleton. This might be intentional UX, but verify it matches design expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/MessagesPage.tsx` around lines 124 - 127, The current render
gate only shows MessagesSkeleton when all four flags are true (loading &&
messages.length === 0 && channelsLoading && channels.length === 0), which can
hide the skeleton if either messages or channels are cached; update the
condition in MessagesPage to show the skeleton when either resource is loading
with no cached items (e.g., use (loading && messages.length === 0) ||
(channelsLoading && channels.length === 0)) so MessagesSkeleton appears whenever
messages or channels are still loading and no cached data exists; adjust the
condition that currently references loading, messages.length, channelsLoading,
channels.length and keep returning <MessagesSkeleton /> in that branch.
ba58192 to
6e57c7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (10)
docs/design/communication.md (1)
115-115:⚠️ Potential issue | 🔴 CriticalUse
"from"in the JSON example to match the implemented wire contract.Line 115 currently documents
"sender", but theMessagemodel serializes/deserializes via the JSON alias"from"(src/synthorg/communication/message.py,src/synthorg/persistence/sqlite/repositories.py). This will mislead API consumers.Proposed doc fix
- "sender": "sarah_chen", + "from": "sarah_chen",As per coding guidelines, "When an implementation deviates from the spec (better approach found, scope evolved, etc.), update the relevant
docs/design/page to reflect the new reality".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/communication.md` at line 115, The docs JSON example uses "sender" but the implementation uses the JSON alias "from" (see Message model and related repository serialization), so update the example to use "from": replace "sender": "sarah_chen" with "from": "sarah_chen" and ensure any surrounding text/examples reference the Message model's "from" wire field to match the implemented contract.web/src/pages/messages/MessageFilterBar.tsx (1)
8-11:⚠️ Potential issue | 🟡 MinorWrap long lines to satisfy the 88-character limit.
There are still lines exceeding the repo lint limit.
✂️ Suggested wrapping
const MESSAGE_TYPES: MessageType[] = [ - 'task_update', 'question', 'announcement', 'review_request', 'approval', - 'delegation', 'status_report', 'escalation', 'meeting_contribution', 'hr_notification', + 'task_update', + 'question', + 'announcement', + 'review_request', + 'approval', + 'delegation', + 'status_report', + 'escalation', + 'meeting_contribution', + 'hr_notification', ] @@ - <Search className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground" aria-hidden="true" /> + <Search + className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground" + aria-hidden="true" + />As per coding guidelines:
**/*.{py,ts,tsx,go}: Lines must not exceed 88
characters.Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/messages/MessageFilterBar.tsx` around lines 8 - 11, The MESSAGE_TYPES array in MessageFilterBar.tsx has lines exceeding the 88-character lint limit; fix by wrapping the array items so no line is longer than 88 chars (e.g., put one or two items per line or one item per line), preserve the const name MESSAGE_TYPES and its type annotation MessageType[], keep the trailing commas and original ordering, and ensure the resulting lines satisfy the repo lint rule for .ts/.tsx files.web/src/__tests__/pages/messages/MessageFilterBar.test.tsx (1)
28-85:⚠️ Potential issue | 🟡 MinorWrap long render lines to comply with the 88-character rule.
Several
render(<MessageFilterBar ... />)calls still exceed the configured
line limit.As per coding guidelines:
**/*.{py,ts,tsx,go}: Lines must not exceed 88
characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx` around lines 28 - 85, Several render(<MessageFilterBar ... />) calls in the tests (e.g., the cases using defaultProps, filters={{ type: 'delegation' }}, filters={{ type: 'delegation', priority: 'high' }}, filters={{ search: 'hello' }}, and those passing onFiltersChange) exceed the 88-character line length; break these JSX render calls across multiple lines (one prop per line or group props so each line ≤88 chars) so the MessageFilterBar render invocations and props like defaultProps, filters, filteredCount, and onFiltersChange are wrapped to comply with the 88-char rule.web/src/pages/messages/ChannelSidebar.tsx (1)
32-32:⚠️ Potential issue | 🟡 MinorWrap long className/nav lines to pass the 88-character lint limit.
A few JSX lines in these ranges are still above the configured max length.
As per coding guidelines:
**/*.{py,ts,tsx,go}: Lines must not exceed 88
characters.Also applies to: 80-82, 91-91, 102-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/messages/ChannelSidebar.tsx` at line 32, Several JSX attribute lines in ChannelSidebar.tsx (notably the div with className "mb-1 px-2 text-[10px] font-semibold uppercase tracking-wider text-muted-foreground" and the other long JSX lines around 80-82, 91, and 102) exceed the 88-character limit; fix by breaking long JSX attributes across multiple lines so each line is <=88 chars — for example, split the className string into multiple concatenated parts or use a template literal and place each group of classes on its own line, and apply the same wrapping approach to the other offending JSX attributes in the ChannelSidebar component.web/src/pages/messages/AttachmentList.tsx (1)
19-31: 🛠️ Refactor suggestion | 🟠 MajorExtract mapped attachment badge markup into a shared UI component.
The
.map()callback still contains complex JSX (Line 22–Line 30). Please move it toweb/src/components/ui/(for example,AttachmentBadge) and map component instances instead.Refactor sketch
+import { AttachmentBadge } from '@/components/ui/AttachmentBadge' ... - {attachments.map((att, i) => { - const Icon = ATTACHMENT_ICONS[att.type] - return ( - <span - // eslint-disable-next-line `@eslint-react/no-array-index-key` -- attachments lack stable IDs - key={`${att.type}-${att.ref}-${i}`} - className="inline-flex items-center gap-1 rounded border border-border bg-surface px-1.5 py-0.5 font-mono text-[10px] text-secondary" - > - <Icon className="size-3" aria-hidden="true" /> - {att.ref} - </span> - ) - })} + {attachments.map((att, i) => ( + <AttachmentBadge + // eslint-disable-next-line `@eslint-react/no-array-index-key` -- attachments lack stable IDs + key={`${att.type}-${att.ref}-${i}`} + attachment={att} + icon={ATTACHMENT_ICONS[att.type]} + /> + ))}As per coding guidelines: "Do NOT recreate complex (>8 line) JSX inside
.map()blocks; extract to a shared component inweb/src/components/ui/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/messages/AttachmentList.tsx` around lines 19 - 31, The attachments.map callback in AttachmentList.tsx renders complex JSX (Icon + span + classes) inline; extract that JSX into a new shared UI component (e.g., AttachmentBadge) under web/src/components/ui/, accepting props {type, ref} (or {att}) and using ATTACHMENT_ICONS to pick the Icon, aria props, and the same className and content, then replace the map body with a simple <AttachmentBadge ... /> instantiation (keep the same key generation `${att.type}-${att.ref}-${i}`) so the mapping is concise and follows the guideline against complex JSX in .map().web/src/hooks/useMessagesData.ts (1)
48-61: 🧹 Nitpick | 🔵 TrivialConsider consolidating store selectors to reduce subscriptions.
Each
useMessagesStoreselector creates a separate subscription. For frequently updating state, this could cause extra re-renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/useMessagesData.ts` around lines 48 - 61, Multiple separate useMessagesStore selectors cause many subscriptions and extra re-renders; consolidate them into a single selector call by selecting an object containing channels, channelsLoading, channelsError, unreadCounts, messages, total, loading, loadingMore, error, expandedThreads, toggleThread, and newMessageIds in one useMessagesStore(...) invocation (optionally with a shallow/equality comparator) so the component has a single subscription and fewer re-renders.web/src/pages/MessagesPage.tsx (1)
125-127: 🧹 Nitpick | 🔵 TrivialLoading condition may miss partial loading states.
The skeleton only shows when all four conditions are true (
loading && messages.length === 0 && channelsLoading && channels.length === 0). If channels are cached but messages aren't (or vice versa), users see the full UI with empty sections rather than the skeleton.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/MessagesPage.tsx` around lines 125 - 127, The current render check in MessagesPage only shows MessagesSkeleton if both messages and channels are simultaneously empty and loading; update the condition to show the skeleton whenever either resource is still loading and empty (i.e., when messages are loading and messages.length === 0 OR channels are loading and channels.length === 0) so partial loading states render the skeleton; adjust the conditional around MessagesSkeleton accordingly to reference the existing variables loading, messages, channelsLoading, and channels.web/src/__tests__/pages/MessagesPage.test.tsx (1)
29-33: 🧹 Nitpick | 🔵 TrivialComputed key mock pattern is non-obvious but functional.
The
[hookName]pattern avoids Vitest's hoist transform inlining issues. Consider usingvi.hoistedfor clarity, but the current approach works correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/pages/MessagesPage.test.tsx` around lines 29 - 33, The test uses a computed key to avoid Vitest's hoist transform (vi.mock('@/hooks/useMessagesData', ... { [hookName]: () => getMessagesData() })), which is non-obvious; replace this pattern by using vitest's hoisting helper: call vi.hoisted to return the mocked export for useMessagesData and export a stable named property that returns getMessagesData() so the mock is clearer and avoids inlining issues—update the vi.mock invocation surrounding useMessagesData and getMessagesData to use vi.hoisted instead of the computed [hookName] key.web/src/pages/messages/MessageBubble.tsx (1)
31-40:⚠️ Potential issue | 🟡 MinorRender a non-interactive element when
onClickis not provided.The button currently exposes interactive semantics (
<button>) even when noonClickhandler is passed. This can be confusing for users and assistive technologies.♻️ Suggested fix: conditionally render button vs div
export function MessageBubble({ message, isNew, onClick }: MessageBubbleProps) { const { triggerFlash, flashStyle } = useFlash() const hasTriggeredRef = useRef(false) useEffect(() => { if (isNew && !hasTriggeredRef.current) { hasTriggeredRef.current = true triggerFlash() } }, [isNew, triggerFlash]) const priorityColor = getMessagePriorityColor(message.priority) + const containerClasses = cn( + 'flex w-full gap-3 rounded-lg p-3 text-left transition-colors', + 'hover:bg-card-hover', + 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent', + ) + + const content = ( + <> + <Avatar name={message.sender} size="sm" /> + <div className="min-w-0 flex-1 space-y-1"> + {/* ... existing content ... */} + </div> + </> + ) + + if (!onClick) { + return ( + <div className={containerClasses} style={flashStyle}> + {content} + </div> + ) + } + return ( <button type="button" onClick={onClick} - className={cn( - 'flex w-full gap-3 rounded-lg p-3 text-left transition-colors', - 'hover:bg-card-hover', - 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent', - )} + className={containerClasses} style={flashStyle} > - <Avatar name={message.sender} size="sm" /> - {/* ... */} + {content} </button> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/messages/MessageBubble.tsx` around lines 31 - 40, MessageBubble currently always renders a <button> even when the onClick prop is undefined; change the JSX to conditionally render a non-interactive <div> when onClick is falsy and a <button> when onClick is provided. Keep the same className (cn(...)), style={flashStyle} and children in both branches, but do not attach onClick, role, or tabIndex to the non-interactive <div>; leave event handlers and interactive attributes only on the <button> branch to preserve correct semantics and accessibility. Use the existing identifiers MessageBubble, onClick, flashStyle, and cn to locate and refactor the element.web/src/pages/messages/MessageList.tsx (1)
52-56: 🧹 Nitpick | 🔵 TrivialClient-side sorting on each render.
The sort is memoized on
[messages], but for large message lists this could be expensive. Consider whether the API could return pre-sorted data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/messages/MessageList.tsx` around lines 52 - 56, The client is performing an expensive sort on every messages update via the useMemo-created variable "sorted" (useMemo(() => [...messages].sort(...), [messages])) which can be heavy for large lists; update the data flow so the API returns messages already sorted by timestamp and remove this client-side sort, or alternatively move the sorting to the data-fetching layer (where messages are loaded) so "messages" is populated in ascending timestamp order and the "sorted" useMemo and its sort call can be deleted; ensure consumers of "sorted" now use the original "messages" (or rename consistently) and keep the timestamp ordering contract documented in the fetch/response code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/page-structure.md`:
- Around line 238-241: Update the docs to clarify that message filter query
parameters are independent and combinable: either change the example routes to
standalone patterns (`/messages?type=...`, `/messages?priority=...`,
`/messages?search=...`, `/messages?message=...`) to match the Approvals pattern,
or add a concise note stating that filters can be combined in any combination
(not required to include `channel`); reference the MessagePageFilters interface
and filterMessages behavior to ensure wording matches the code (all filter
properties optional and applied independently).
In `@web/src/__tests__/helpers/factories.ts`:
- Around line 127-130: The factory makeMessage currently uses Date.now() which
makes default timestamps non-deterministic; change makeMessage to return a
fixed, deterministic ISO timestamp (e.g. a constant like
'2020-01-01T00:00:00.000Z') for the timestamp field and ensure the function
still respects overrides.timestamp when provided so tests that need relative
times can pass one in.
In `@web/src/__tests__/hooks/useMessagesData.test.ts`:
- Around line 58-159: Add unit tests to assert the remaining fields on the
UseMessagesDataReturn from useMessagesData: set useMessagesStore state for
channelsLoading, channelsError, loadingMore, wsSetupError, and newMessageIds
then renderHook(() => useMessagesData(...)) and expect
result.current.channelsLoading, channelsError, loadingMore, wsSetupError and
result.current.newMessageIds.has(...) to reflect the store; also add a test that
calls result.current.fetchMore() and asserts mockFetchMoreMessages was called
with the active channel (reference symbols: UseMessagesDataReturn,
useMessagesData, useMessagesStore, mockFetchMoreMessages).
In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx`:
- Around line 51-58: Add a new test in MessageFilterBar.test.tsx that renders
<MessageFilterBar {...defaultProps} onFiltersChange={onFiltersChange} />, uses
userEvent.setup() to type a whitespace-only string (e.g., ' ') into
screen.getByLabelText('Search messages'), and asserts that the onFiltersChange
mock was called with { search: undefined } to ensure whitespace-only input
clears the search filter; name the test clearly (e.g., "clears search when input
is whitespace-only") and reuse the existing onFiltersChange mock pattern.
In `@web/src/__tests__/pages/messages/MessageList.test.tsx`:
- Around line 51-65: Two test lines exceed the 88-character limit; break the
long lines in the 'renders Load earlier messages button when hasMore' and 'calls
onLoadMore when Load button clicked' tests by splitting JSX props and long calls
across multiple lines. For example, in the render calls for MessageList (in the
tests named it(...) above) put each prop on its own line (e.g.,
render(<MessageList {...defaultProps} messages={msgs} hasMore={true} />) →
render(<MessageList {...defaultProps} messages={msgs} hasMore={true}
onLoadMore={onLoadMore} />) split into multiple lines), and similarly split the
await user.click(screen.getByRole(...)) call by assigning the element to a short
variable before clicking; update tests 'renders Load earlier messages button
when hasMore' and 'calls onLoadMore when Load button clicked' accordingly so all
lines are <=88 chars.
In `@web/src/pages/messages/AttachmentList.tsx`:
- Line 25: The className string on the JSX element in AttachmentList.tsx exceeds
the 88-char limit; split the className into multiple shorter pieces and join
them (or use a multi-line template literal) so no line is >88 chars. Locate the
long className attribute (the one starting with "inline-flex items-center gap-1
...") and replace it with a multi-line expression such as an array of class
segments joined by ' ' (e.g., className={[ 'inline-flex items-center', 'gap-1
rounded border border-border', 'bg-surface px-1.5 py-0.5', 'font-mono
text-[10px] text-secondary' ].join(' ')}) or a backtick template literal split
across lines to ensure each source line stays under 88 chars.
In `@web/src/pages/messages/MessageBubble.stories.tsx`:
- Around line 6-17: The object literal for baseMessage has a single-line
metadata field that exceeds the 88-character limit; break the metadata value
onto multiple lines to wrap its properties for readability. Locate the
baseMessage constant and reformat the metadata property (e.g., place the opening
{ on its own line and each key: value pair on its own indented line, then close
the }), ensuring tokens_used, cost_usd, extra, etc. remain intact and the
overall file still compiles.
In `@web/src/pages/messages/MessageDetailDrawer.stories.tsx`:
- Around line 42-51: The metadata object in the MinimalMetadata story (export
const MinimalMetadata) exceeds the 88-character limit; reformat the
args.message.metadata object by breaking its properties onto multiple lines
(e.g., place each key: value pair on its own line or wrap the object across
lines) so the line length is under 88 chars while preserving the same keys and
values (task_id, project_id, tokens_used, cost_usd, extra).
In `@web/src/pages/messages/MessageFilterBar.tsx`:
- Around line 44-52: handleSearchChange currently forwards raw input including
whitespace-only strings into filters which makes hasFilters/showFilteredCount
treat them as active; change handleSearchChange (the useCallback) to trim the
input (e.g., const value = e.target.value.trim()) and call onFiltersChange with
search: value || undefined so empty/whitespace-only input clears the filter;
keep filters and onFiltersChange in the dependency array.
- Around line 58-88: The MessageFilterBar component is using bespoke <select>
controls for type and priority (see MESSAGE_TYPES, PRIORITIES,
getMessageTypeLabel, filters, handleTypeChange, handlePriorityChange); replace
these inline selects with the shared select/input primitives from
web/src/components/ui/ (use the shared Select/Option or equivalent and the
shared Input/PillButton components) wiring their value and onChange to the
existing filters and handlers so styling/behavior match the app-wide UI
primitives; preserve option generation logic (mapping MESSAGE_TYPES and
PRIORITIES) and aria-labels when integrating the shared components.
In `@web/src/stores/messages.ts`:
- Around line 78-80: The fetch entry points (fetchMessages and
fetchMoreMessages) must clear stale pagination state before starting a new
request: inside fetchMessages (where seq is set and set({ loading: true, error:
null }) currently runs), also reset loadingMore to false and ensure error is
cleared; likewise update fetchMoreMessages (around the lines 96-100) to clear
error and, when starting a fresh channel load, reset loadingMore so a previous
channel's in-flight "load earlier" won't leak into the new channel. Locate and
update the set({...}) calls in the fetchMessages and fetchMoreMessages functions
(use symbols fetchMessages, fetchMoreMessages, messageRequestSeq,
MESSAGES_FETCH_LIMIT) to include loadingMore: false and error: null at request
start.
- Around line 97-115: When appending paginated results from
messagesApi.listMessages in the messages store (use of messageRequestSeq,
MESSAGES_FETCH_LIMIT, set and loadingMore), filter out any items from
result.data that already exist in the current store (s.messages) by comparing a
unique identifier (e.g., message.id) before concatenating, so you avoid
re-inserting duplicates if a WebSocket prepended messages while the request was
inflight; keep the seq check intact and then set messages to [...s.messages,
...dedupedResultData], update total and loadingMore as before.
---
Duplicate comments:
In `@docs/design/communication.md`:
- Line 115: The docs JSON example uses "sender" but the implementation uses the
JSON alias "from" (see Message model and related repository serialization), so
update the example to use "from": replace "sender": "sarah_chen" with "from":
"sarah_chen" and ensure any surrounding text/examples reference the Message
model's "from" wire field to match the implemented contract.
In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx`:
- Around line 28-85: Several render(<MessageFilterBar ... />) calls in the tests
(e.g., the cases using defaultProps, filters={{ type: 'delegation' }},
filters={{ type: 'delegation', priority: 'high' }}, filters={{ search: 'hello'
}}, and those passing onFiltersChange) exceed the 88-character line length;
break these JSX render calls across multiple lines (one prop per line or group
props so each line ≤88 chars) so the MessageFilterBar render invocations and
props like defaultProps, filters, filteredCount, and onFiltersChange are wrapped
to comply with the 88-char rule.
In `@web/src/__tests__/pages/MessagesPage.test.tsx`:
- Around line 29-33: The test uses a computed key to avoid Vitest's hoist
transform (vi.mock('@/hooks/useMessagesData', ... { [hookName]: () =>
getMessagesData() })), which is non-obvious; replace this pattern by using
vitest's hoisting helper: call vi.hoisted to return the mocked export for
useMessagesData and export a stable named property that returns
getMessagesData() so the mock is clearer and avoids inlining issues—update the
vi.mock invocation surrounding useMessagesData and getMessagesData to use
vi.hoisted instead of the computed [hookName] key.
In `@web/src/hooks/useMessagesData.ts`:
- Around line 48-61: Multiple separate useMessagesStore selectors cause many
subscriptions and extra re-renders; consolidate them into a single selector call
by selecting an object containing channels, channelsLoading, channelsError,
unreadCounts, messages, total, loading, loadingMore, error, expandedThreads,
toggleThread, and newMessageIds in one useMessagesStore(...) invocation
(optionally with a shallow/equality comparator) so the component has a single
subscription and fewer re-renders.
In `@web/src/pages/messages/AttachmentList.tsx`:
- Around line 19-31: The attachments.map callback in AttachmentList.tsx renders
complex JSX (Icon + span + classes) inline; extract that JSX into a new shared
UI component (e.g., AttachmentBadge) under web/src/components/ui/, accepting
props {type, ref} (or {att}) and using ATTACHMENT_ICONS to pick the Icon, aria
props, and the same className and content, then replace the map body with a
simple <AttachmentBadge ... /> instantiation (keep the same key generation
`${att.type}-${att.ref}-${i}`) so the mapping is concise and follows the
guideline against complex JSX in .map().
In `@web/src/pages/messages/ChannelSidebar.tsx`:
- Line 32: Several JSX attribute lines in ChannelSidebar.tsx (notably the div
with className "mb-1 px-2 text-[10px] font-semibold uppercase tracking-wider
text-muted-foreground" and the other long JSX lines around 80-82, 91, and 102)
exceed the 88-character limit; fix by breaking long JSX attributes across
multiple lines so each line is <=88 chars — for example, split the className
string into multiple concatenated parts or use a template literal and place each
group of classes on its own line, and apply the same wrapping approach to the
other offending JSX attributes in the ChannelSidebar component.
In `@web/src/pages/messages/MessageBubble.tsx`:
- Around line 31-40: MessageBubble currently always renders a <button> even when
the onClick prop is undefined; change the JSX to conditionally render a
non-interactive <div> when onClick is falsy and a <button> when onClick is
provided. Keep the same className (cn(...)), style={flashStyle} and children in
both branches, but do not attach onClick, role, or tabIndex to the
non-interactive <div>; leave event handlers and interactive attributes only on
the <button> branch to preserve correct semantics and accessibility. Use the
existing identifiers MessageBubble, onClick, flashStyle, and cn to locate and
refactor the element.
In `@web/src/pages/messages/MessageFilterBar.tsx`:
- Around line 8-11: The MESSAGE_TYPES array in MessageFilterBar.tsx has lines
exceeding the 88-character lint limit; fix by wrapping the array items so no
line is longer than 88 chars (e.g., put one or two items per line or one item
per line), preserve the const name MESSAGE_TYPES and its type annotation
MessageType[], keep the trailing commas and original ordering, and ensure the
resulting lines satisfy the repo lint rule for .ts/.tsx files.
In `@web/src/pages/messages/MessageList.tsx`:
- Around line 52-56: The client is performing an expensive sort on every
messages update via the useMemo-created variable "sorted" (useMemo(() =>
[...messages].sort(...), [messages])) which can be heavy for large lists; update
the data flow so the API returns messages already sorted by timestamp and remove
this client-side sort, or alternatively move the sorting to the data-fetching
layer (where messages are loaded) so "messages" is populated in ascending
timestamp order and the "sorted" useMemo and its sort call can be deleted;
ensure consumers of "sorted" now use the original "messages" (or rename
consistently) and keep the timestamp ordering contract documented in the
fetch/response code.
In `@web/src/pages/MessagesPage.tsx`:
- Around line 125-127: The current render check in MessagesPage only shows
MessagesSkeleton if both messages and channels are simultaneously empty and
loading; update the condition to show the skeleton whenever either resource is
still loading and empty (i.e., when messages are loading and messages.length ===
0 OR channels are loading and channels.length === 0) so partial loading states
render the skeleton; adjust the conditional around MessagesSkeleton accordingly
to reference the existing variables loading, messages, channelsLoading, and
channels.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43866c57-5d18-411d-86b8-8ccc1e6daabc
📒 Files selected for processing (41)
docs/design/communication.mddocs/design/page-structure.mdweb/src/__tests__/helpers/factories.tsweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/stores/messages.test.tsweb/src/__tests__/utils/messages.test.tsweb/src/hooks/useMessagesData.tsweb/src/pages/MessagesPage.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/stores/messages.tsweb/src/utils/messages.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Dashboard Test
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When an implementation deviates from the spec (better approach found, scope evolved, etc.), update the relevant
docs/design/page to reflect the new reality
Files:
docs/design/communication.mddocs/design/page-structure.md
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation must be written in Markdown and built with Zensical (config:
mkdocs.yml); design spec lives indocs/design/(11 pages linked fromdocs/DESIGN_SPEC.md)
Files:
docs/design/communication.mddocs/design/page-structure.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
Use camelCase for variable names and function identifiers
Files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/__tests__/utils/messages.test.tsweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/hooks/useMessagesData.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/stores/messages.tsweb/src/utils/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsx
web/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.tsx: ALWAYS reuse existing shared components fromweb/src/components/ui/before creating new ones (e.g.,StatusBadge,MetricCard,AgentCard,DeptHealthBar,SectionCard)
Use Tailwind semantic classes (text-foreground,bg-card,text-accent,text-success) or CSS variables (var(--so-accent)); NEVER hardcode hex values or rgba() in .tsx files
Usefont-sansorfont-monofor typography (maps to Geist tokens); NEVER setfontFamilydirectly in CSS
Use density-aware spacing tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Do NOT recreate complex (>8 line) JSX inside.map()blocks; extract to a shared component inweb/src/components/ui/
Files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsx
**/*.{py,ts,tsx,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Lines must not exceed 88 characters (enforced by ruff for Python, configured in web/.eslintrc for TypeScript)
Files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/__tests__/utils/messages.test.tsweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/hooks/useMessagesData.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/stores/messages.tsweb/src/utils/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsx
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Import
cnfrom@/lib/utilsfor conditional class merging in components
Files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/__tests__/utils/messages.test.tsweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/hooks/useMessagesData.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/stores/messages.tsweb/src/utils/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsx
web/src/pages/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Lazy-load page components and place page-scoped sub-components in
pages/<page-name>/subdirectories (e.g.,pages/tasks/,pages/org-edit/)
Files:
web/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsx
🧠 Learnings (14)
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to web/src/__tests__/**/*.{ts,js} : Dashboard testing: Vitest unit tests organized by feature under `web/src/__tests__/`. Use fast-check for property-based testing (`fc.assert` + `fc.property`).
Applied to files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/utils/messages.test.tsweb/src/__tests__/stores/messages.test.tsweb/src/__tests__/pages/MessagesPage.test.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : New shared components must have a corresponding `.stories.tsx` Storybook file with all component states (default, hover, loading, error, empty)
Applied to files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Do NOT recreate complex (>8 line) JSX inside `.map()` blocks; extract to a shared component in `web/src/components/ui/`
Applied to files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/hooks/useMessagesData.tsweb/src/utils/messages.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/MessagesPage.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to **/*.{py,ts,tsx,go} : Lines must not exceed 88 characters (enforced by ruff for Python, configured in web/.eslintrc for TypeScript)
Applied to files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/utils/messages.ts
📚 Learning: 2026-03-21T11:08:01.542Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T11:08:01.542Z
Learning: Applies to **/*.{py,md} : Line length: 88 characters (ruff).
Applied to files:
web/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/utils/messages.ts
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : Export component props as a TypeScript interface
Applied to files:
web/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : ALWAYS reuse existing shared components from `web/src/components/ui/` before creating new ones (e.g., `StatusBadge`, `MetricCard`, `AgentCard`, `DeptHealthBar`, `SectionCard`)
Applied to files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : Use design token variables exclusively for shadows and borders (`var(--so-shadow-card-hover)`, `border-border`, `border-bright`); never hardcode shadow or border values
Applied to files:
web/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Use density-aware spacing tokens (`p-card`, `gap-section-gap`, `gap-grid-gap`) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Applied to files:
web/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Use Tailwind semantic classes (`text-foreground`, `bg-card`, `text-accent`, `text-success`) or CSS variables (`var(--so-accent)`); NEVER hardcode hex values or rgba() in .tsx files
Applied to files:
web/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/ChannelSidebar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to **/*.{ts,tsx} : Prefer `interface` for defining object shapes in TypeScript
Applied to files:
web/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/pages/**/*.tsx : Lazy-load page components and place page-scoped sub-components in `pages/<page-name>/` subdirectories (e.g., `pages/tasks/`, `pages/org-edit/`)
Applied to files:
web/src/pages/messages/ChannelSidebar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to **/*.{ts,tsx} : Use camelCase for variable names and function identifiers
Applied to files:
web/src/utils/messages.ts
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to docs/design/*.md : Design spec pages: 7 pages in `docs/design/` — index, agents, organization, communication, engine, memory, operations
Applied to files:
docs/design/page-structure.md
| export const MinimalMetadata: Story = { | ||
| args: { | ||
| message: { | ||
| ...fullMessage, | ||
| attachments: [], | ||
| metadata: { task_id: null, project_id: null, tokens_used: null, cost_usd: null, extra: [] }, | ||
| }, | ||
| open: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Wrap the metadata object to satisfy the 88-char limit.
Line 47 exceeds the configured max length.
Suggested fix
export const MinimalMetadata: Story = {
args: {
message: {
...fullMessage,
attachments: [],
- metadata: { task_id: null, project_id: null, tokens_used: null, cost_usd: null, extra: [] },
+ metadata: {
+ task_id: null,
+ project_id: null,
+ tokens_used: null,
+ cost_usd: null,
+ extra: [],
+ },
},
open: true,
},
}As per coding guidelines: **/*.{py,ts,tsx,go}: Lines must not exceed 88 characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const MinimalMetadata: Story = { | |
| args: { | |
| message: { | |
| ...fullMessage, | |
| attachments: [], | |
| metadata: { task_id: null, project_id: null, tokens_used: null, cost_usd: null, extra: [] }, | |
| }, | |
| open: true, | |
| }, | |
| } | |
| export const MinimalMetadata: Story = { | |
| args: { | |
| message: { | |
| ...fullMessage, | |
| attachments: [], | |
| metadata: { | |
| task_id: null, | |
| project_id: null, | |
| tokens_used: null, | |
| cost_usd: null, | |
| extra: [], | |
| }, | |
| }, | |
| open: true, | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/messages/MessageDetailDrawer.stories.tsx` around lines 42 - 51,
The metadata object in the MinimalMetadata story (export const MinimalMetadata)
exceeds the 88-character limit; reformat the args.message.metadata object by
breaking its properties onto multiple lines (e.g., place each key: value pair on
its own line or wrap the object across lines) so the line length is under 88
chars while preserving the same keys and values (task_id, project_id,
tokens_used, cost_usd, extra).
| <select | ||
| value={filters.type ?? ''} | ||
| onChange={handleTypeChange} | ||
| aria-label="Filter by message type" | ||
| className={cn( | ||
| 'h-7 rounded-md border border-border bg-surface px-2 text-xs text-foreground', | ||
| 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent', | ||
| )} | ||
| > | ||
| <option value="">All types</option> | ||
| {MESSAGE_TYPES.map((t) => ( | ||
| <option key={t} value={t}>{getMessageTypeLabel(t)}</option> | ||
| ))} | ||
| </select> | ||
|
|
||
| {/* Priority filter */} | ||
| <select | ||
| value={filters.priority ?? ''} | ||
| onChange={handlePriorityChange} | ||
| aria-label="Filter by priority" | ||
| className={cn( | ||
| 'h-7 rounded-md border border-border bg-surface px-2 text-xs text-foreground', | ||
| 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent', | ||
| )} | ||
| > | ||
| <option value="">All priorities</option> | ||
| {PRIORITIES.map((p) => ( | ||
| <option key={p} value={p}>{p.charAt(0).toUpperCase() + p.slice(1)}</option> | ||
| ))} | ||
| </select> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace bespoke form controls with shared ui primitives.
This component defines custom-styled <select>, <input>, and pill action
buttons inline instead of reusing shared controls from
web/src/components/ui/. Please switch to existing shared components where
equivalents exist to keep behavior and styling consistent.
As per coding guidelines: web/src/**/*.tsx: ALWAYS reuse existing shared
components from web/src/components/ui/ before creating new ones.
Also applies to: 132-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/messages/MessageFilterBar.tsx` around lines 58 - 88, The
MessageFilterBar component is using bespoke <select> controls for type and
priority (see MESSAGE_TYPES, PRIORITIES, getMessageTypeLabel, filters,
handleTypeChange, handlePriorityChange); replace these inline selects with the
shared select/input primitives from web/src/components/ui/ (use the shared
Select/Option or equivalent and the shared Input/PillButton components) wiring
their value and onChange to the existing filters and handlers so
styling/behavior match the app-wide UI primitives; preserve option generation
logic (mapping MESSAGE_TYPES and PRIORITIES) and aria-labels when integrating
the shared components.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
docs/design/page-structure.md (1)
238-241:⚠️ Potential issue | 🟡 MinorRoute examples still contradict the “channel is optional” rule.
Line 99 says
channelis not required, but Lines 238-241 only show variants that requirechannel. Please switch these to standalone examples (or add both standalone + combined examples) so the route map matches documented behavior.📘 Proposed doc fix
-| `/messages?channel=:name&type=:type` | Messages (filtered) | Filtered by message type | -| `/messages?channel=:name&priority=:level` | Messages (filtered) | Filtered by priority | -| `/messages?channel=:name&search=:query` | Messages (filtered) | Search by content/sender | -| `/messages?channel=:name&message=:id` | Messages (detail) | Side drawer for message detail | +| `/messages?type=:type` | Messages (filtered) | Filtered by message type | +| `/messages?priority=:level` | Messages (filtered) | Filtered by priority | +| `/messages?search=:query` | Messages (filtered) | Search by content/sender | +| `/messages?message=:id` | Messages (detail) | Side drawer for message detail |As per coding guidelines: “
docs/design/**/*.md: When an implementation deviates from the spec … update the relevantdocs/design/page to reflect the new reality”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/page-structure.md` around lines 238 - 241, The route examples in the table for `/messages` contradict the "channel is optional" rule: update the entries referring to `/messages?channel=:name&type=:type`, `/messages?channel=:name&priority=:level`, `/messages?channel=:name&search=:query`, and `/messages?channel=:name&message=:id` to include standalone variants without `channel` (e.g., `/messages?type=:type`, `/messages?priority=:level`, `/messages?search=:query`, `/messages?message=:id`) or add both forms so the docs show both combined and channel-optional usage; edit the table rows for those four routes, adjust their description text if needed to indicate channel is optional, and ensure consistency with the earlier "channel is not required" statement.web/src/__tests__/pages/messages/MessageFilterBar.test.tsx (1)
29-29:⚠️ Potential issue | 🟡 MinorWrap long test render lines to satisfy the 88-char rule.
These render statements exceed the configured line length limit (Line 29,
Line 77, Line 91, Line 100).✂️ Suggested formatting-only wrap
- render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} filteredCount={8} />) + render( + <MessageFilterBar + {...defaultProps} + filters={{ type: 'delegation' }} + filteredCount={8} + /> + ) @@ - render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation', priority: 'high' }} />) + render( + <MessageFilterBar + {...defaultProps} + filters={{ type: 'delegation', priority: 'high' }} + /> + ) @@ - render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} onFiltersChange={onFiltersChange} />) + render( + <MessageFilterBar + {...defaultProps} + filters={{ type: 'delegation' }} + onFiltersChange={onFiltersChange} + /> + ) @@ - render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation', priority: 'high' }} onFiltersChange={onFiltersChange} />) + render( + <MessageFilterBar + {...defaultProps} + filters={{ type: 'delegation', priority: 'high' }} + onFiltersChange={onFiltersChange} + /> + )As per coding guidelines:
**/*.{py,ts,tsx,go}: Lines must not exceed 88
characters.Also applies to: 77-77, 91-91, 100-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx` at line 29, The render calls for MessageFilterBar (e.g., render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation' }} filteredCount={8} />)) exceed the 88-char limit; fix by breaking the JSX props across multiple lines or by extracting props into a named object (e.g., const props = { ...defaultProps, filters: { type: 'delegation' }, filteredCount: 8 }) and then calling render(<MessageFilterBar {...props} />) so each line stays under 88 characters; apply the same wrapping strategy to the other long render lines referenced (lines with render and MessageFilterBar at 77, 91, 100).web/src/pages/messages/MessageFilterBar.tsx (2)
62-107:⚠️ Potential issue | 🟠 MajorUse shared
uiprimitives instead of bespoke controls.This block reimplements styled form controls and pills (
<select>,<input>,
and local pill/clear buttons) instead of reusing shared components from
web/src/components/ui/, which breaks consistency requirements.As per coding guidelines:
web/src/**/*.tsx: ALWAYS reuse existing shared
components fromweb/src/components/ui/before creating new ones.Also applies to: 136-143, 154-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/messages/MessageFilterBar.tsx` around lines 62 - 107, Replace the bespoke <select> and <input> controls in MessageFilterBar with the shared UI primitives from the project's ui library (use the shared Select and Input components and any shared Pill/Clear Button or Badge components) so styling and behavior match the other pages; wire the shared Select components to the existing handlers (handleTypeChange, handlePriorityChange) and values (filters.type, filters.priority, MESSAGE_TYPES, PRIORITIES) and wire the shared Input to handleSearchChange and filters.search, preserving aria-labels and options generation (getMessageTypeLabel for MESSAGE_TYPES and capitalizing PRIORITIES) and ensure any clear/pill UI uses the shared ClearButton/Pill component rather than local markup.
9-10:⚠️ Potential issue | 🟡 MinorFix 88-character limit violations in this file.
Several changed lines exceed the configured max length (e.g., Line 9, Line 10,
Line 95, Line 111, Line 126, Line 156).✂️ Suggested formatting-only wrap
const MESSAGE_TYPES: MessageType[] = [ - 'task_update', 'question', 'announcement', 'review_request', 'approval', - 'delegation', 'status_report', 'escalation', 'meeting_contribution', 'hr_notification', + 'task_update', + 'question', + 'announcement', + 'review_request', + 'approval', + 'delegation', + 'status_report', + 'escalation', + 'meeting_contribution', + 'hr_notification', ] @@ - <Search className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground" aria-hidden="true" /> + <Search + className="absolute left-2 top-1/2 size-3 -translate-y-1/2 text-muted-foreground" + aria-hidden="true" + /> @@ - {showFilteredCount ? `${filteredCount} of ${totalCount}` : `${totalCount} messages`} + {showFilteredCount + ? `${filteredCount} of ${totalCount}` + : `${totalCount} messages`}As per coding guidelines:
**/*.{py,ts,tsx,go}: Lines must not exceed 88
characters.Also applies to: 95-95, 111-111, 126-126, 156-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/messages/MessageFilterBar.tsx` around lines 9 - 10, Several lines in the MessageFilterBar component exceed the 88-character limit (notably the long string/array entries like 'task_update', 'question', ... and other long JSX/props at lines ~95,111,126,156); update the MessageFilterBar (the exported MessageFilterBar component and the array/constant that holds these message type strings) to wrap long arrays/strings and break long JSX prop expressions into multiple lines so each line <=88 chars (split the message types into one-per-line in the array, wrap long prop values/children across lines, or extract long expressions into named constants used by the component).web/src/stores/messages.ts (1)
96-100:⚠️ Potential issue | 🟡 MinorClear stale pagination errors before retrying.
A previous "load earlier" failure survives into the next retry because this
request start never resetserror. If the retry succeeds, the page can still
show the old error state next to fresh data.Possible fix
- set({ loadingMore: true }) + set({ loadingMore: true, error: null })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/messages.ts` around lines 96 - 100, The fetchMoreMessages flow leaves previous pagination errors in state, so before starting a new request (inside fetchMoreMessages where it reads messageRequestSeq and sets loadingMore) clear the stale error state; update the start of the request to reset the messages error (e.g., call set to null out the relevant error field) so retries don’t surface an old error after a successful fetch, and ensure this reset occurs before any early return guarded by loadingMore or before incrementing/using messageRequestSeq.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/stores/messages.ts`:
- Around line 107-109: The seq-mismatch branch currently resets shared state
(set({ loadingMore: false })) even for stale requests; change it so that when
seq !== messageRequestSeq the function simply returns without mutating state
(i.e., remove the set call from the early-return), and apply the same change to
the other similar branch around lines 125-127; alternatively implement a
dedicated request token for loadingMore, but for the minimal fix ensure
messageRequestSeq and seq checks never clear loadingMore for a newer request.
- Around line 78-92: fetchMessages currently replaces the shared messages array
unconditionally which loses same-channel live prepends and can leave old-channel
data visible on failures; modify the store so the messages state is tied to a
channel identifier (e.g., add a messagesChannel/currentChannel field alongside
messages) and in fetchMessages (and its catch block) only update
messages/total/newMessageIds/loading if seq === messageRequestSeq AND the
response channel matches that stored channel; when the response is for the same
channel, merge result.data with any already-prepended live messages (dedupe by
message id and preserve order) instead of blindly overwriting; ensure a failed
request for a different channel does not replace or keep the previous channel
feed by guarding all set(...) calls with the same-channel check.
---
Duplicate comments:
In `@docs/design/page-structure.md`:
- Around line 238-241: The route examples in the table for `/messages`
contradict the "channel is optional" rule: update the entries referring to
`/messages?channel=:name&type=:type`, `/messages?channel=:name&priority=:level`,
`/messages?channel=:name&search=:query`, and
`/messages?channel=:name&message=:id` to include standalone variants without
`channel` (e.g., `/messages?type=:type`, `/messages?priority=:level`,
`/messages?search=:query`, `/messages?message=:id`) or add both forms so the
docs show both combined and channel-optional usage; edit the table rows for
those four routes, adjust their description text if needed to indicate channel
is optional, and ensure consistency with the earlier "channel is not required"
statement.
In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx`:
- Line 29: The render calls for MessageFilterBar (e.g., render(<MessageFilterBar
{...defaultProps} filters={{ type: 'delegation' }} filteredCount={8} />)) exceed
the 88-char limit; fix by breaking the JSX props across multiple lines or by
extracting props into a named object (e.g., const props = { ...defaultProps,
filters: { type: 'delegation' }, filteredCount: 8 }) and then calling
render(<MessageFilterBar {...props} />) so each line stays under 88 characters;
apply the same wrapping strategy to the other long render lines referenced
(lines with render and MessageFilterBar at 77, 91, 100).
In `@web/src/pages/messages/MessageFilterBar.tsx`:
- Around line 62-107: Replace the bespoke <select> and <input> controls in
MessageFilterBar with the shared UI primitives from the project's ui library
(use the shared Select and Input components and any shared Pill/Clear Button or
Badge components) so styling and behavior match the other pages; wire the shared
Select components to the existing handlers (handleTypeChange,
handlePriorityChange) and values (filters.type, filters.priority, MESSAGE_TYPES,
PRIORITIES) and wire the shared Input to handleSearchChange and filters.search,
preserving aria-labels and options generation (getMessageTypeLabel for
MESSAGE_TYPES and capitalizing PRIORITIES) and ensure any clear/pill UI uses the
shared ClearButton/Pill component rather than local markup.
- Around line 9-10: Several lines in the MessageFilterBar component exceed the
88-character limit (notably the long string/array entries like 'task_update',
'question', ... and other long JSX/props at lines ~95,111,126,156); update the
MessageFilterBar (the exported MessageFilterBar component and the array/constant
that holds these message type strings) to wrap long arrays/strings and break
long JSX prop expressions into multiple lines so each line <=88 chars (split the
message types into one-per-line in the array, wrap long prop values/children
across lines, or extract long expressions into named constants used by the
component).
In `@web/src/stores/messages.ts`:
- Around line 96-100: The fetchMoreMessages flow leaves previous pagination
errors in state, so before starting a new request (inside fetchMoreMessages
where it reads messageRequestSeq and sets loadingMore) clear the stale error
state; update the start of the request to reset the messages error (e.g., call
set to null out the relevant error field) so retries don’t surface an old error
after a successful fetch, and ensure this reset occurs before any early return
guarded by loadingMore or before incrementing/using messageRequestSeq.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 669583f3-e349-46fb-93ea-52af7f2e8cce
📒 Files selected for processing (6)
docs/design/page-structure.mdweb/src/__tests__/helpers/factories.tsweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/stores/messages.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Dashboard Test
🧰 Additional context used
📓 Path-based instructions (7)
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When an implementation deviates from the spec (better approach found, scope evolved, etc.), update the relevant
docs/design/page to reflect the new reality
Files:
docs/design/page-structure.md
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation must be written in Markdown and built with Zensical (config:
mkdocs.yml); design spec lives indocs/design/(11 pages linked fromdocs/DESIGN_SPEC.md)
Files:
docs/design/page-structure.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript
Use camelCase for variable names and function identifiers
Files:
web/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/helpers/factories.tsweb/src/stores/messages.ts
web/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.tsx: ALWAYS reuse existing shared components fromweb/src/components/ui/before creating new ones (e.g.,StatusBadge,MetricCard,AgentCard,DeptHealthBar,SectionCard)
Use Tailwind semantic classes (text-foreground,bg-card,text-accent,text-success) or CSS variables (var(--so-accent)); NEVER hardcode hex values or rgba() in .tsx files
Usefont-sansorfont-monofor typography (maps to Geist tokens); NEVER setfontFamilydirectly in CSS
Use density-aware spacing tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing; NEVER hardcode pixel values for layout spacing
Do NOT recreate complex (>8 line) JSX inside.map()blocks; extract to a shared component inweb/src/components/ui/
Files:
web/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsx
**/*.{py,ts,tsx,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Lines must not exceed 88 characters (enforced by ruff for Python, configured in web/.eslintrc for TypeScript)
Files:
web/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/helpers/factories.tsweb/src/stores/messages.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Import
cnfrom@/lib/utilsfor conditional class merging in components
Files:
web/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/helpers/factories.tsweb/src/stores/messages.ts
web/src/pages/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Lazy-load page components and place page-scoped sub-components in
pages/<page-name>/subdirectories (e.g.,pages/tasks/,pages/org-edit/)
Files:
web/src/pages/messages/MessageFilterBar.tsx
🧠 Learnings (12)
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to docs/design/*.md : Design spec pages: 7 pages in `docs/design/` — index, agents, organization, communication, engine, memory, operations
Applied to files:
docs/design/page-structure.md
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : Export component props as a TypeScript interface
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to **/*.{ts,tsx} : Prefer `interface` for defining object shapes in TypeScript
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to **/*.{py,ts,tsx,go} : Lines must not exceed 88 characters (enforced by ruff for Python, configured in web/.eslintrc for TypeScript)
Applied to files:
web/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsx
📚 Learning: 2026-03-21T11:08:01.542Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T11:08:01.542Z
Learning: Applies to **/*.{py,md} : Line length: 88 characters (ruff).
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Do NOT recreate complex (>8 line) JSX inside `.map()` blocks; extract to a shared component in `web/src/components/ui/`
Applied to files:
web/src/pages/messages/MessageFilterBar.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : ALWAYS reuse existing shared components from `web/src/components/ui/` before creating new ones (e.g., `StatusBadge`, `MetricCard`, `AgentCard`, `DeptHealthBar`, `SectionCard`)
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : Use design token variables exclusively for shadows and borders (`var(--so-shadow-card-hover)`, `border-border`, `border-bright`); never hardcode shadow or border values
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/**/*.tsx : Use Tailwind semantic classes (`text-foreground`, `bg-card`, `text-accent`, `text-success`) or CSS variables (`var(--so-accent)`); NEVER hardcode hex values or rgba() in .tsx files
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-15T21:20:09.993Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:20:09.993Z
Learning: Applies to web/src/components/** : Vue components organized by feature (agents/, approvals/, budget/, common/, dashboard/, layout/, messages/, org-chart/, tasks/).
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T14:27:45.936Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T14:27:45.936Z
Learning: Applies to web/src/components/ui/**/*.tsx : New shared components must have a corresponding `.stories.tsx` Storybook file with all component states (default, hover, loading, error, empty)
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to web/src/__tests__/**/*.{ts,js} : Dashboard testing: Vitest unit tests organized by feature under `web/src/__tests__/`. Use fast-check for property-based testing (`fc.assert` + `fc.property`).
Applied to files:
web/src/__tests__/pages/messages/MessageFilterBar.test.tsx
🔇 Additional comments (4)
web/src/__tests__/helpers/factories.ts (3)
1-13: Type imports are correctly wired for the new factories.
ChannelandMessageare imported as types and used directly by the added helpers.
127-147:makeMessagedefaults are stable and schema-aligned.Good fix on deterministic timestamp (Line 130), and the default
metadatashape includes all required fields includingextra.
149-156:makeChannelmatches the API contract cleanly.Defaults for
typeandsubscribersare valid and make this helper reusable across channel tests.web/src/__tests__/pages/messages/MessageFilterBar.test.tsx (1)
60-74: Good edge-case coverage for whitespace-only search input.This test locks the expected normalization behavior (
search: undefined) and
prevents regressions in URL/filter state handling.
| fetchMessages: async (channel, limit = MESSAGES_FETCH_LIMIT) => { | ||
| const seq = ++messageRequestSeq | ||
| set({ loading: true, error: null, loadingMore: false }) | ||
| try { | ||
| const result = await messagesApi.listMessages({ channel, limit }) | ||
| if (seq !== messageRequestSeq) return | ||
| set({ | ||
| messages: result.data, | ||
| total: result.total, | ||
| loading: false, | ||
| newMessageIds: new Set<string>(), | ||
| }) | ||
| } catch (err) { | ||
| if (seq !== messageRequestSeq) return | ||
| set({ loading: false, error: getErrorMessage(err) }) |
There was a problem hiding this comment.
fetchMessages still overwrites live or stale-cross-channel feed state.
messages is supposed to be the active-channel feed, but this path keeps the
previous array alive and then blindly replaces it with result.data. If a
same-channel WebSocket message lands while the request is in flight, the prepend
on Lines 169-183 is lost here; if the channel changed, a failed request can
leave the old feed visible under the new channel. Track which channel the feed
belongs to and merge same-channel live arrivals instead of overwriting
messages unconditionally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/messages.ts` around lines 78 - 92, fetchMessages currently
replaces the shared messages array unconditionally which loses same-channel live
prepends and can leave old-channel data visible on failures; modify the store so
the messages state is tied to a channel identifier (e.g., add a
messagesChannel/currentChannel field alongside messages) and in fetchMessages
(and its catch block) only update messages/total/newMessageIds/loading if seq
=== messageRequestSeq AND the response channel matches that stored channel; when
the response is for the same channel, merge result.data with any
already-prepended live messages (dedupe by message id and preserve order)
instead of blindly overwriting; ensure a failed request for a different channel
does not replace or keep the previous channel feed by guarding all set(...)
calls with the same-channel check.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/stores/messages.ts (1)
78-92:⚠️ Potential issue | 🟠 MajorDon't replace the feed wholesale on refresh.
Line 84 still assigns
result.dataas the entire feed. That drops same-channel updates that land while the request is in flight, such as WS prepends or paginated appends, and a failed channel switch leaves the previous channel's messages in state. Track which channel ownsmessagesand merge/dedupe current state instead of replacing it wholesale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/messages.ts` around lines 78 - 92, fetchMessages currently replaces messages with result.data, which discards concurrent WS updates or paginated appends and can leave the wrong channel in state after a failed switch; change fetchMessages (and the code path after messagesApi.listMessages) to first confirm the store's current channel/owner (use messageRequestSeq check plus the store's channel value, e.g., get().channel or an ownerChannel field) matches the requested channel, then merge result.data with the existing messages array (by id dedupe and preserving proper order for prepend/append) rather than assigning it wholesale, update total/newMessageIds only when the channels match, and ensure you still bail early if seq !== messageRequestSeq to avoid races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/stores/messages.ts`:
- Around line 127-187: Split the large payload parsing/validation block out of
handleWsEvent into a small helper (e.g., parseWsMessagePayload or
validateAndParseWsMessage) that accepts event.payload.message, performs the type
checks/logging currently in handleWsEvent (including sanitizeForLog usage), and
returns a typed Message or null; then simplify handleWsEvent to call that
helper, bail if it returns null, and keep the existing state-mutation logic
(dedup/prepend via set for active channel and unreadCounts increment for
inactive channels) unchanged. Ensure the helper validates the same fields (id,
timestamp, sender, to, channel, content, type, priority, attachments array,
metadata object) and that handleWsEvent uses the helper's returned Message for
the set calls (preserving newMessageIds and total update behavior).
---
Duplicate comments:
In `@web/src/stores/messages.ts`:
- Around line 78-92: fetchMessages currently replaces messages with result.data,
which discards concurrent WS updates or paginated appends and can leave the
wrong channel in state after a failed switch; change fetchMessages (and the code
path after messagesApi.listMessages) to first confirm the store's current
channel/owner (use messageRequestSeq check plus the store's channel value, e.g.,
get().channel or an ownerChannel field) matches the requested channel, then
merge result.data with the existing messages array (by id dedupe and preserving
proper order for prepend/append) rather than assigning it wholesale, update
total/newMessageIds only when the channels match, and ensure you still bail
early if seq !== messageRequestSeq to avoid races.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 953d256c-af07-4628-abe8-e0281f52faea
📒 Files selected for processing (1)
web/src/stores/messages.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Use design tokens and semantic classes (text-foreground,bg-card,text-accent,text-success,bg-danger, etc.) or CSS variables (var(--so-accent)) -- NEVER hardcode hex values or rgba in.tsx/.tsfiles
Usefont-sansorfont-monofor typography (maps to Geist tokens) -- NEVER setfontFamilydirectly
Use density-aware spacing tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing -- NEVER hardcode pixel values for layout spacing
Use token variables for shadows/borders (var(--so-shadow-card-hover),border-border,border-bright) -- NEVER hardcode values
Do NOT recreate status dots inline -- use<StatusBadge>; do NOT build card-with-header layouts from scratch -- use<SectionCard>; do NOT create metric displays with text-metric classes -- use<MetricCard>
Do NOT render initials circles manually -- use<Avatar>; do NOT create complex (>8 line) JSX inside.map()-- extract to a shared component
Use TypeScript 6.0 features: remove deprecatedbaseUrl, accept thatesModuleInteropis always true, explicitly listtypesif needed, use"bundler"or"nodenext"formoduleResolution
Never use real vendor names (OpenAI, Claude, GPT, Anthropic) in web dashboard code -- use generic names:example-provider,example-large-001, etc.
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Files:
web/src/stores/messages.ts
…#787) Implement the Messages page as an investigative tool for examining agent-to-agent communications. Includes channel sidebar with unread badges, message threading by task_id, sender avatars, timestamp grouping, message type/priority indicators, URL-driven filters, WebSocket real-time updates, and detail drawer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-reviewed by 4 agents, 15 findings addressed: - Fix dynamic Tailwind class interpolation (production purge bug) - Add static class maps for priority dot/badge colors - Fix duplicate React keys in metadata.extra rendering - Add dedicated tests for MessageFilterBar, MessageDetailDrawer, ChannelListItem, AttachmentList (4 new test files, 35 new tests) - Add edge case tests (invalid timestamps, to-field search) - Update communication.md: from->sender, nullable metadata, extra field - Update page-structure.md: detailed Messages section, URL params Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…mini Critical fixes: - Extend WS message validation to all required fields (type, priority, to, attachments, metadata) preventing runtime crashes - Fix stale closure in WS handler via activeChannelRef -- handlers now read the live channel value instead of a stale closure - Add sequence guard to fetchMoreMessages preventing cross-channel message corruption on rapid channel switching Major fixes: - Wire newMessageIds through store -> hook -> page -> MessageList to enable flash animation on WS-delivered messages - Replace setWasConnected useState with ref to avoid set-state-in-render - Extract ChannelGroupSection from ChannelSidebar .map() (>8 line rule) - Extract MessageRowSkeleton from MessagesSkeleton .map() - Add named interfaces for MessageDetailContent, MetadataRow, FilterPill Medium fixes: - Add index to React keys for attachments and metadata.extra (collision) - Break long lines in utils/messages.ts and ChannelListItem.tsx - Replace hardcoded h-[500px] with h-96 in MessageList stories - Add missing URL routing table rows for priority and search params - Add WS message deduplication by ID on reconnection - Show both error banners when messages and channels errors differ - Use ReadonlyMap/readonly arrays in ThreadGroup interface - Remove unused getChannelTypeIcon function and its tests Minor fixes: - Deduplicate WithAttachments story with distinct fixture - Rename meta -> sampleMetadata in MessageThread stories - Reformat ALL_TYPES array for line length compliance - Fix ChannelListItem test line lengths - Use exact regex matcher for unread badge assertion - Replace dynamic import with top-level vi.mocked() in hook test Test improvements: - Add fetchMoreMessages error path and stale response tests - Add URL filter validation and dual-error-banner tests - Fix console.error spy cleanup with try/finally - Add getPriorityDotClass/getPriorityBadgeClasses utility tests - Add array payload.message and dedup rejection tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Clarify `extra` metadata field is non-nullable in design spec - Add aria-label to unread badge in ChannelListItem for a11y - Add clearNewMessageIds store action, replace direct setState - Assert date divider rendering in MessageList test - Add comment explaining Vitest mock hoisting workaround Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix whitespace-only search input treated as active filter (trim input) - Add deterministic timestamp to makeMessage factory - Clear loadingMore when fetchMessages starts (prevent stale leak) - Dedup pagination results against existing messages (WS race) - Clarify filter params are independent/combinable in design spec - Add hook field coverage tests (channelsLoading, loadingMore, etc.) - Add whitespace search clearing test for MessageFilterBar Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…retry
- Remove set({ loadingMore: false }) from seq-mismatch branches in
fetchMoreMessages -- fetchMessages already resets loadingMore, so stale
requests should silently return without touching state
- Clear error when fetchMoreMessages starts so retries after failure
show clean state
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract WS payload validation into a standalone parseWsMessage function that returns a typed Message or null. Simplifies handleWsEvent to a 3-line dispatch (parse, bail, route to active/inactive channel). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dded21d to
9318cb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
web/src/utils/messages.ts (1)
6-17: 🧹 Nitpick | 🔵 TrivialConsider renaming constant maps to camelCase.
The constant maps (
MESSAGE_TYPE_LABELS,PRIORITY_COLOR_MAP,PRIORITY_DOT_CLASSES,PRIORITY_BADGE_CLASSES,CHANNEL_TYPE_LABELS) useSCREAMING_SNAKE_CASE. While this is a common convention for constants, if the codebase convention is camelCase for all variables, consider aligning these for consistency.Also applies to: 25-28, 36-41, 47-52, 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/utils/messages.ts` around lines 6 - 17, The constant maps (MESSAGE_TYPE_LABELS, PRIORITY_COLOR_MAP, PRIORITY_DOT_CLASSES, PRIORITY_BADGE_CLASSES, CHANNEL_TYPE_LABELS) are using SCREAMING_SNAKE_CASE but the codebase prefers camelCase; rename these identifiers to camelCase (e.g., messageTypeLabels, priorityColorMap, priorityDotClasses, priorityBadgeClasses, channelTypeLabels) and update every usage/import of these symbols throughout the module to the new names to keep consistency; ensure exports (if any) and any tests or components that reference MESSAGE_TYPE_LABELS etc. are updated to the new camelCase identifiers.web/src/pages/messages/AttachmentList.tsx (1)
19-31: 🛠️ Refactor suggestion | 🟠 MajorExtract the attachment badge JSX to a separate component.
The JSX inside
.map()spans 11 lines, exceeding the 8-line threshold. Extract this into anAttachmentBadgecomponent for better maintainability.♻️ Proposed refactor
+interface AttachmentBadgeProps { + attachment: Attachment +} + +function AttachmentBadge({ attachment }: AttachmentBadgeProps) { + const Icon = ATTACHMENT_ICONS[attachment.type] + return ( + <span + className={cn( + 'inline-flex items-center gap-1 rounded border border-border bg-surface', + 'px-1.5 py-0.5 font-mono text-[10px] text-secondary', + )} + > + <Icon className="size-3" aria-hidden="true" /> + {attachment.ref} + </span> + ) +} export function AttachmentList({ attachments }: AttachmentListProps) { if (attachments.length === 0) return null return ( <div className="flex flex-wrap gap-2"> - {attachments.map((att, i) => { - const Icon = ATTACHMENT_ICONS[att.type] - return ( - <span - // eslint-disable-next-line `@eslint-react/no-array-index-key` -- attachments lack stable IDs - key={`${att.type}-${att.ref}-${i}`} - className="inline-flex items-center gap-1 rounded border border-border bg-surface px-1.5 py-0.5 font-mono text-[10px] text-secondary" - > - <Icon className="size-3" aria-hidden="true" /> - {att.ref} - </span> - ) - })} + {attachments.map((att, i) => ( + // eslint-disable-next-line `@eslint-react/no-array-index-key` -- attachments lack stable IDs + <AttachmentBadge key={`${att.type}-${att.ref}-${i}`} attachment={att} /> + ))} </div> ) }Add import for
cn:import { cn } from '@/lib/utils'As per coding guidelines: "Do NOT create complex (>8 line) JSX inside
.map()-- extract to a shared component."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/messages/AttachmentList.tsx` around lines 19 - 31, Extract the badge JSX inside AttachmentList's .map into a new functional component AttachmentBadge that accepts the attachment object (prop name like att) and the index (i) and renders the same span with Icon from ATTACHMENT_ICONS[att.type], aria-hidden on the icon, and {att.ref} inside; update AttachmentList to return <AttachmentBadge att={att} i={i} key={`${att.type}-${att.ref}-${i}`} /> from the map. Import and use the cn utility if needed for composing the className, and keep the existing class string and key behavior unchanged; ensure AttachmentBadge is exported or declared in the same module so the map can reference it.web/src/__tests__/pages/messages/MessageFilterBar.test.tsx (1)
97-104:⚠️ Potential issue | 🟡 MinorLine exceeds 88-character limit.
Line 100 exceeds the configured line length. Break the JSX props onto separate lines.
Suggested fix
it('clears all filters when Clear all is clicked', async () => { const user = userEvent.setup() const onFiltersChange = vi.fn() - render(<MessageFilterBar {...defaultProps} filters={{ type: 'delegation', priority: 'high' }} onFiltersChange={onFiltersChange} />) + render( + <MessageFilterBar + {...defaultProps} + filters={{ type: 'delegation', priority: 'high' }} + onFiltersChange={onFiltersChange} + />, + ) await user.click(screen.getByText('Clear all')) expect(onFiltersChange).toHaveBeenCalledWith({}) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx` around lines 97 - 104, The test's JSX render call for MessageFilterBar has a single long line exceeding the 88-character limit; update the render invocation in MessageFilterBar.test (the render(<MessageFilterBar ... />) call) to place each prop on its own line (e.g., <MessageFilterBar {...defaultProps} filters={{ type: 'delegation', priority: 'high' }} onFiltersChange={onFiltersChange} /> → break into multiple lines so each prop is on a separate line) to satisfy the line-length rule while keeping the same props and behavior.web/src/__tests__/pages/messages/MessageList.test.tsx (3)
61-61:⚠️ Potential issue | 🟡 MinorLine exceeds 88-character limit.
This render call is over 88 characters.
Suggested fix
- render(<MessageList {...defaultProps} messages={msgs} hasMore={true} onLoadMore={onLoadMore} />) + render( + <MessageList + {...defaultProps} + messages={msgs} + hasMore={true} + onLoadMore={onLoadMore} + />, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/pages/messages/MessageList.test.tsx` at line 61, The render call to MessageList is longer than 88 characters; split the JSX props across multiple lines to satisfy the line-length rule. Locate the render(<MessageList ... />) call and reformat it so props like {...defaultProps}, messages={msgs}, hasMore={true}, and onLoadMore={onLoadMore} are each on their own line (or group logically) within the JSX opening tag, keeping the closing /> on its own line so the line length is under 88 characters.
78-84:⚠️ Potential issue | 🟡 MinorInline metadata objects exceed 88-character limit.
Lines 78 and 83 contain inline metadata objects that are too long.
Suggested fix
makeMessage('1', { content: 'Thread start', timestamp: '2026-03-28T10:00:00Z', - metadata: { task_id: 'task-1', project_id: null, tokens_used: null, cost_usd: null, extra: [] }, + metadata: { + task_id: 'task-1', + project_id: null, + tokens_used: null, + cost_usd: null, + extra: [], + }, }), makeMessage('2', { content: 'Thread reply', timestamp: '2026-03-28T10:05:00Z', - metadata: { task_id: 'task-1', project_id: null, tokens_used: null, cost_usd: null, extra: [] }, + metadata: { + task_id: 'task-1', + project_id: null, + tokens_used: null, + cost_usd: null, + extra: [], + }, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/pages/messages/MessageList.test.tsx` around lines 78 - 84, The inline metadata objects passed to makeMessage('1', ...) and makeMessage('2', ...) exceed the 88-char line limit; extract each metadata object into a separate const (e.g., const metadataThread = { task_id: 'task-1', project_id: null, tokens_used: null, cost_usd: null, extra: [] }) or format the object across multiple lines, then pass that const (or the multi-line object) into makeMessage to keep each line under the limit and preserve the same keys and values.
54-54:⚠️ Potential issue | 🟡 MinorLine exceeds 88-character limit.
This line is over 88 characters and may fail lint.
Suggested fix
- expect(screen.getByRole('button', { name: /load earlier messages/i })).toBeInTheDocument() + expect( + screen.getByRole('button', { name: /load earlier messages/i }), + ).toBeInTheDocument()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/pages/messages/MessageList.test.tsx` at line 54, The test assertion line in MessageList.test.tsx exceeds the 88-character limit; shorten it by splitting or rephrasing the matcher call — for example, store the accessible name regex in a variable (e.g., const loadEarlierBtnName = /load earlier messages/i) or use getByRole parameters on separate lines, then call expect(screen.getByRole('button', { name: loadEarlierBtnName })).toBeInTheDocument(); update the assertion that uses screen.getByRole and the name regex so the line length is under 88 characters.docs/design/page-structure.md (1)
238-241:⚠️ Potential issue | 🟡 MinorAlign route-table examples with the optional/combinable filter contract.
Line 99 says
type/priority/search/messageare independent optional params, but Lines 238-241 still implychannelis required for each. This is contradictory in the same spec page.📝 Proposed doc fix
-| `/messages?channel=:name&type=:type` | Messages (filtered) | Filtered by message type | -| `/messages?channel=:name&priority=:level` | Messages (filtered) | Filtered by priority | -| `/messages?channel=:name&search=:query` | Messages (filtered) | Search by content/sender | -| `/messages?channel=:name&message=:id` | Messages (detail) | Side drawer for message detail | +| `/messages?type=:type` | Messages (filtered) | Filtered by message type | +| `/messages?priority=:level` | Messages (filtered) | Filtered by priority | +| `/messages?search=:query` | Messages (filtered) | Search by content/sender | +| `/messages?message=:id` | Messages (detail) | Side drawer for message detail |As per coding guidelines:
docs/design/**/*.md: “When approved deviations from the design spec occur during implementation, update the relevantdocs/design/page to reflect the new reality”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/page-structure.md` around lines 238 - 241, Update the route examples in the table to reflect that `channel`, `type`, `priority`, `search`, and `message` are independent optional and combinable query parameters: change each row that currently implies a required `/messages?channel=:name&...` to show `/messages?channel=:name&type=:type` as one possible combination and add rows (or a single consolidated row) showing `/messages`, `/messages?type=:type`, `/messages?priority=:level`, `/messages?search=:query`, `/messages?message=:id`, and an explicit example of combining params like `/messages?channel=:name&type=:type&priority=:level&search=:query` so the `route-table` aligns with the contract described on Line 99.web/src/pages/MessagesPage.tsx (1)
124-127:⚠️ Potential issue | 🟠 MajorDon't require both resources to be empty before showing a loading state.
If channels are cached or finish first,
loadingcan still be true whilemessagesis empty. This branch then falls through to the normal layout, so a still-loading channel briefly renders as an empty message view instead of a loading state.Minimal fix
- if (loading && messages.length === 0 && channelsLoading && channels.length === 0) { + if ( + (activeChannel && loading && messages.length === 0) || + (channelsLoading && channels.length === 0) + ) { return <MessagesSkeleton /> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/MessagesPage.tsx` around lines 124 - 127, The loading branch currently requires both resources to be empty; change the conditional in MessagesPage.tsx so it returns <MessagesSkeleton /> when either messages are still loading and empty (loading && messages.length === 0) OR channels are still loading and empty (channelsLoading && channels.length === 0), i.e., replace the combined && check with an || between those two clauses so a still-loading messages or channels state shows the skeleton.web/src/stores/messages.ts (1)
117-131:⚠️ Potential issue | 🔴 CriticalThe active feed still isn't tied to a channel.
fetchMessages()starts a new request but leaves the previous feed inmessagesuntil the await resolves, then blindly overwrites the array. Switching#eng →#product`` can therefore show stale#engmessages under `#product`; if the new request fails, the old feed stays visible, and a same-channel WS prepend that lands mid-request can also be lost when `result.data` replaces the list. Track the feed's channel explicitly, clear/reset it immediately on channel change, and merge/dedupe live arrivals instead of overwriting unconditionally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/messages.ts` around lines 117 - 131, fetchMessages currently starts a new request but doesn't mark the active feed/channel or clear the old messages, so stale messages can be shown or lost; add an explicit activeChannel (or currentChannel) field to the store and in fetchMessages set it immediately (and reset messages = [] and newMessageIds = new Set()) when the requested channel differs before awaiting messagesApi.listMessages, keep using messageRequestSeq to ignore out-of-order responses, and when the response arrives only update when seq matches AND activeChannel === channel; also change any WS prepend/arrival handler to merge into the existing messages array with a dedupe-by-id strategy instead of blindly replacing messages so live inserts are preserved across in-flight fetches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/communication.md`:
- Line 115: The docs use the JSON field name "sender" but the backend Message
model serializes that attribute as "from" (Pydantic alias); update this and
every related JSON example in this document to use "from" instead of "sender" so
the examples match the Message model's serialized output and backend behavior.
In `@web/src/hooks/useMessagesData.ts`:
- Around line 40-132: The useMessagesData hook is too large; split
responsibilities into smaller hooks: extract store selectors into a new hook
(e.g., useMessagesStoreSelectors) that returns channels, channelsLoading,
channelsError, unreadCounts, messages, total, loading, loadingMore, error,
expandedThreads, toggleThread, newMessageIds; extract lifecycle/fetch logic into
useMessagesLifecycle which calls useMessagesStore.getState().fetchChannels on
mount, calls fetchMessages and resetUnread when activeChannel changes (using
activeChannelRef and fetchMore), and wires polling (move pollFn and polling into
that hook using usePolling and expose start/stop or let hook manage it); extract
WebSocket wiring into useMessagesWebsocket which builds bindings (using
activeChannelRef) and calls useWebSocket to return wsConnected and wsSetupError;
finally have useMessagesData compose these smaller hooks and compute hasMore and
return the same shape — reference symbols: useMessagesData,
useMessagesStore.getState().fetchChannels, fetchMessages, resetUnread,
fetchMore, pollFn, polling, usePolling, MESSAGES_POLL_INTERVAL, bindings,
MESSAGES_WS_CHANNELS, useWebSocket, activeChannelRef, and hasMore.
In `@web/src/pages/messages/MessageList.tsx`:
- Around line 95-121: groupMessagesByThread currently separates threads and
standalone and renders all threads first then standalone, breaking chronological
order; change rendering to iterate the original msgs sequence for the date group
and emit each item in order, rendering a MessageBubble for standalone messages
and rendering the corresponding MessageThread exactly once when you encounter
the first message of a thread. Specifically, in the rendering block that now
uses threads/standalone, iterate over msgs (the original array returned for the
dateKey), keep a local Set (e.g., emittedThreads) to track which taskId threads
have already been output, and for each msg if msg.taskId is present and not in
emittedThreads render MessageThread with messages={threads.get(taskId)} and mark
taskId emitted, otherwise render MessageBubble for that msg; preserve existing
props (expandedThreads, toggleThread, onSelectMessage, newMessageIds) and keep
TimestampDivider/getDateGroupLabel as-is.
In `@web/src/pages/MessagesPage.tsx`:
- Around line 51-60: The current effect restarts on every replacement of
newMessageIds so IDs don't expire under steady traffic; change to per-ID expiry
instead of a single-set timer by either (a) storing timestamps (Map<id,ts>) in
the messages store and updating it in the message arrival handler, then have a
single interval/effect in MessagesPage that prunes ids older than 2000ms from
that Map and calls
useMessagesStore.getState().clearSpecificNewMessageIds(expiredIds), or (b)
schedule a per-id setTimeout when adding an id in the arrival code that calls a
new store method clearNewMessageId(id) after 2000ms; update/merge the Set rather
than replacing it so useEffect/useMessagesStore.clearNewMessageIds can operate
on individual IDs. Ensure to add/rename store methods (e.g., clearNewMessageId,
clearSpecificNewMessageIds) referenced above.
- Around line 23-214: MessagesPage is too large and does too many
responsibilities (URL parsing/sync, WS connection tracking, flash cleanup,
banner logic, selection, filtering, and rendering); split it into smaller units:
extract URL/filter logic into a custom hook (e.g., useMessagePageFilters) that
returns filters, handleFiltersChange, activeChannel, selectedMessageId,
handleSelectChannel, handleSelectMessage, handleCloseDrawer; extract WS tracking
and flash cleanup into small hooks (e.g., useWsConnectionTracker and
useAutoClearNewMessageIds) that encapsulate wasConnectedRef update and the
setTimeout clear of newMessageIds; move the main render block for the center
panel (filter bar, error banners, MessageList and EmptyState) into a new
presentational component (e.g., MessagePanel) that accepts props from the hooks
and from useMessagesData (channels, messages, loading, error, etc.); ensure each
new function/component (useMessagePageFilters, useWsConnectionTracker,
useAutoClearNewMessageIds, MessagePanel) keeps body under ~50 lines and replaces
the corresponding logic in MessagesPage so MessagesPage becomes a thin
coordinator that composes these pieces.
---
Duplicate comments:
In `@docs/design/page-structure.md`:
- Around line 238-241: Update the route examples in the table to reflect that
`channel`, `type`, `priority`, `search`, and `message` are independent optional
and combinable query parameters: change each row that currently implies a
required `/messages?channel=:name&...` to show
`/messages?channel=:name&type=:type` as one possible combination and add rows
(or a single consolidated row) showing `/messages`, `/messages?type=:type`,
`/messages?priority=:level`, `/messages?search=:query`, `/messages?message=:id`,
and an explicit example of combining params like
`/messages?channel=:name&type=:type&priority=:level&search=:query` so the
`route-table` aligns with the contract described on Line 99.
In `@web/src/__tests__/pages/messages/MessageFilterBar.test.tsx`:
- Around line 97-104: The test's JSX render call for MessageFilterBar has a
single long line exceeding the 88-character limit; update the render invocation
in MessageFilterBar.test (the render(<MessageFilterBar ... />) call) to place
each prop on its own line (e.g., <MessageFilterBar {...defaultProps} filters={{
type: 'delegation', priority: 'high' }} onFiltersChange={onFiltersChange} /> →
break into multiple lines so each prop is on a separate line) to satisfy the
line-length rule while keeping the same props and behavior.
In `@web/src/__tests__/pages/messages/MessageList.test.tsx`:
- Line 61: The render call to MessageList is longer than 88 characters; split
the JSX props across multiple lines to satisfy the line-length rule. Locate the
render(<MessageList ... />) call and reformat it so props like
{...defaultProps}, messages={msgs}, hasMore={true}, and onLoadMore={onLoadMore}
are each on their own line (or group logically) within the JSX opening tag,
keeping the closing /> on its own line so the line length is under 88
characters.
- Around line 78-84: The inline metadata objects passed to makeMessage('1', ...)
and makeMessage('2', ...) exceed the 88-char line limit; extract each metadata
object into a separate const (e.g., const metadataThread = { task_id: 'task-1',
project_id: null, tokens_used: null, cost_usd: null, extra: [] }) or format the
object across multiple lines, then pass that const (or the multi-line object)
into makeMessage to keep each line under the limit and preserve the same keys
and values.
- Line 54: The test assertion line in MessageList.test.tsx exceeds the
88-character limit; shorten it by splitting or rephrasing the matcher call — for
example, store the accessible name regex in a variable (e.g., const
loadEarlierBtnName = /load earlier messages/i) or use getByRole parameters on
separate lines, then call expect(screen.getByRole('button', { name:
loadEarlierBtnName })).toBeInTheDocument(); update the assertion that uses
screen.getByRole and the name regex so the line length is under 88 characters.
In `@web/src/pages/messages/AttachmentList.tsx`:
- Around line 19-31: Extract the badge JSX inside AttachmentList's .map into a
new functional component AttachmentBadge that accepts the attachment object
(prop name like att) and the index (i) and renders the same span with Icon from
ATTACHMENT_ICONS[att.type], aria-hidden on the icon, and {att.ref} inside;
update AttachmentList to return <AttachmentBadge att={att} i={i}
key={`${att.type}-${att.ref}-${i}`} /> from the map. Import and use the cn
utility if needed for composing the className, and keep the existing class
string and key behavior unchanged; ensure AttachmentBadge is exported or
declared in the same module so the map can reference it.
In `@web/src/pages/MessagesPage.tsx`:
- Around line 124-127: The loading branch currently requires both resources to
be empty; change the conditional in MessagesPage.tsx so it returns
<MessagesSkeleton /> when either messages are still loading and empty (loading
&& messages.length === 0) OR channels are still loading and empty
(channelsLoading && channels.length === 0), i.e., replace the combined && check
with an || between those two clauses so a still-loading messages or channels
state shows the skeleton.
In `@web/src/stores/messages.ts`:
- Around line 117-131: fetchMessages currently starts a new request but doesn't
mark the active feed/channel or clear the old messages, so stale messages can be
shown or lost; add an explicit activeChannel (or currentChannel) field to the
store and in fetchMessages set it immediately (and reset messages = [] and
newMessageIds = new Set()) when the requested channel differs before awaiting
messagesApi.listMessages, keep using messageRequestSeq to ignore out-of-order
responses, and when the response arrives only update when seq matches AND
activeChannel === channel; also change any WS prepend/arrival handler to merge
into the existing messages array with a dedupe-by-id strategy instead of blindly
replacing messages so live inserts are preserved across in-flight fetches.
In `@web/src/utils/messages.ts`:
- Around line 6-17: The constant maps (MESSAGE_TYPE_LABELS, PRIORITY_COLOR_MAP,
PRIORITY_DOT_CLASSES, PRIORITY_BADGE_CLASSES, CHANNEL_TYPE_LABELS) are using
SCREAMING_SNAKE_CASE but the codebase prefers camelCase; rename these
identifiers to camelCase (e.g., messageTypeLabels, priorityColorMap,
priorityDotClasses, priorityBadgeClasses, channelTypeLabels) and update every
usage/import of these symbols throughout the module to the new names to keep
consistency; ensure exports (if any) and any tests or components that reference
MESSAGE_TYPE_LABELS etc. are updated to the new camelCase identifiers.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb753606-80d9-47fe-8485-fc8bd523eddf
📒 Files selected for processing (41)
docs/design/communication.mddocs/design/page-structure.mdweb/src/__tests__/helpers/factories.tsweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/stores/messages.test.tsweb/src/__tests__/utils/messages.test.tsweb/src/hooks/useMessagesData.tsweb/src/pages/MessagesPage.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/stores/messages.tsweb/src/utils/messages.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Deploy Preview
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Dashboard Test
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Use design tokens and semantic classes (text-foreground,bg-card,text-accent,text-success,bg-danger, etc.) or CSS variables (var(--so-accent)) -- NEVER hardcode hex values or rgba in.tsx/.tsfiles
Usefont-sansorfont-monofor typography (maps to Geist tokens) -- NEVER setfontFamilydirectly
Use density-aware spacing tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing -- NEVER hardcode pixel values for layout spacing
Use token variables for shadows/borders (var(--so-shadow-card-hover),border-border,border-bright) -- NEVER hardcode values
Do NOT recreate status dots inline -- use<StatusBadge>; do NOT build card-with-header layouts from scratch -- use<SectionCard>; do NOT create metric displays with text-metric classes -- use<MetricCard>
Do NOT render initials circles manually -- use<Avatar>; do NOT create complex (>8 line) JSX inside.map()-- extract to a shared component
Use TypeScript 6.0 features: remove deprecatedbaseUrl, accept thatesModuleInteropis always true, explicitly listtypesif needed, use"bundler"or"nodenext"formoduleResolution
Never use real vendor names (OpenAI, Claude, GPT, Anthropic) in web dashboard code -- use generic names:example-provider,example-large-001, etc.
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/utils/messages.tsweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessageThread.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/stores/messages.tsweb/src/pages/MessagesPage.tsxweb/src/hooks/useMessagesData.tsweb/src/pages/messages/MessageList.tsxweb/src/pages/messages/MessageBubble.tsxweb/src/__tests__/utils/messages.test.ts
web/src/**/*.stories.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use Storybook 10 ESM-only imports:
storybook/test(not@storybook/test),storybook/actions(not@storybook/addon-actions); usedefinePreviewfrom@storybook/react-vite; setparameters.a11y.testfor WCAG compliance enforcement
Files:
web/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/MessageList.stories.tsx
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Design spec pages in
docs/design/are mandatory references before implementing features -- ALWAYS read the relevant design page
Files:
docs/design/communication.mddocs/design/page-structure.md
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When approved deviations from the design spec occur during implementation, update the relevant
docs/design/page to reflect the new reality
Files:
docs/design/communication.mddocs/design/page-structure.md
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.stories.tsx : Use Storybook 10 ESM-only imports: `storybook/test` (not `storybook/test`), `storybook/actions` (not `storybook/addon-actions`); use `definePreview` from `storybook/react-vite`; set `parameters.a11y.test` for WCAG compliance enforcement
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/components/ui/**/*.{tsx} : Create a `.stories.tsx` file alongside each new shared component with all states (default, hover, loading, error, empty) for Storybook documentation
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to web/src/__tests__/**/*.{ts,js} : Dashboard testing: Vitest unit tests organized by feature under `web/src/__tests__/`. Use fast-check for property-based testing (`fc.assert` + `fc.property`).
Applied to files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/__tests__/stores/messages.test.tsweb/src/__tests__/utils/messages.test.ts
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.stories.tsx : Use Storybook 10 ESM-only imports: `storybook/test` (not `storybook/test`), `storybook/actions` (not `storybook/addon-actions`); use `definePreview` from `storybook/react-vite`; set `parameters.a11y.test` for WCAG compliance enforcement
Applied to files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageBubble.test.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/__tests__/hooks/useMessagesData.test.tsweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/__tests__/stores/messages.test.tsweb/src/__tests__/utils/messages.test.ts
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/components/ui/**/*.{tsx} : Create a `.stories.tsx` file alongside each new shared component with all states (default, hover, loading, error, empty) for Storybook documentation
Applied to files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.stories.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/AttachmentList.stories.tsxweb/src/pages/messages/TimestampDivider.stories.tsxweb/src/__tests__/pages/messages/MessageThread.test.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/MessageFilterBar.stories.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/ChannelListItem.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/__tests__/pages/MessagesPage.test.tsxweb/src/pages/messages/ChannelSidebar.stories.tsxweb/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.{ts,tsx} : Do NOT render initials circles manually -- use `<Avatar>`; do NOT create complex (>8 line) JSX inside `.map()` -- extract to a shared component
Applied to files:
web/src/__tests__/pages/messages/AttachmentList.test.tsxweb/src/pages/messages/MessagesSkeleton.tsxweb/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/MessagesPage.tsxweb/src/pages/messages/MessageBubble.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.{ts,tsx} : Do NOT recreate status dots inline -- use `<StatusBadge>`; do NOT build card-with-header layouts from scratch -- use `<SectionCard>`; do NOT create metric displays with text-metric classes -- use `<MetricCard>`
Applied to files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageBubble.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/components/ui/**/*.{ts,tsx} : Always reuse existing shared components from `web/src/components/ui/` before creating new ones; export props as TypeScript interfaces; use design tokens exclusively; import `cn` from `@/lib/utils` for conditional class merging
Applied to files:
web/src/pages/messages/MessagesSkeleton.tsxweb/src/pages/messages/TimestampDivider.tsxweb/src/pages/messages/MessageTypeBadge.tsxweb/src/__tests__/pages/messages/ChannelSidebar.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/utils/messages.tsweb/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/pages/messages/MessageBubble.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.{ts,tsx} : Keep functions under 50 lines and files under 800 lines
Applied to files:
web/src/__tests__/pages/messages/MessageList.test.tsxweb/src/__tests__/pages/messages/MessageDetailDrawer.test.tsxweb/src/pages/messages/ChannelListItem.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/utils/messages.tsweb/src/__tests__/pages/messages/MessageFilterBar.test.tsxweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessageList.stories.tsxweb/src/stores/messages.ts
📚 Learning: 2026-03-21T11:08:01.542Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T11:08:01.542Z
Learning: Applies to **/*.{py,md} : Line length: 88 characters (ruff).
Applied to files:
web/src/__tests__/pages/messages/MessageList.test.tsxweb/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageTypeBadge.stories.tsxweb/src/pages/messages/MessageDetailDrawer.stories.tsxweb/src/utils/messages.tsweb/src/pages/messages/MessageThread.stories.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageBubble.stories.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/__tests__/pages/messages/ChannelListItem.test.tsxweb/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use design tokens and semantic classes (`text-foreground`, `bg-card`, `text-accent`, `text-success`, `bg-danger`, etc.) or CSS variables (`var(--so-accent)`) -- NEVER hardcode hex values or rgba in `.tsx`/`.ts` files
Applied to files:
web/src/pages/messages/AttachmentList.tsxweb/src/utils/messages.tsweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/ChannelSidebar.tsxweb/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use density-aware spacing tokens (`p-card`, `gap-section-gap`, `gap-grid-gap`) or standard Tailwind spacing -- NEVER hardcode pixel values for layout spacing
Applied to files:
web/src/pages/messages/AttachmentList.tsxweb/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to **/*.py : Line length must be 88 characters (enforced by ruff)
Applied to files:
web/src/pages/messages/AttachmentList.tsx
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to docs/design/*.md : Design spec pages: 7 pages in `docs/design/` — index, agents, organization, communication, engine, memory, operations
Applied to files:
docs/design/page-structure.md
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use token variables for shadows/borders (`var(--so-shadow-card-hover)`, `border-border`, `border-bright`) -- NEVER hardcode values
Applied to files:
web/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.tsxweb/src/pages/messages/MessageList.stories.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use TypeScript 6.0 features: remove deprecated `baseUrl`, accept that `esModuleInterop` is always true, explicitly list `types` if needed, use `"bundler"` or `"nodenext"` for `moduleResolution`
Applied to files:
web/src/pages/messages/MessageDetailDrawer.tsxweb/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-28T16:54:20.164Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T16:54:20.164Z
Learning: Applies to web/src/**/*.{ts,tsx} : Never use real vendor names (OpenAI, Claude, GPT, Anthropic) in web dashboard code -- use generic names: `example-provider`, `example-large-001`, etc.
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
📚 Learning: 2026-03-15T21:20:09.993Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:20:09.993Z
Learning: Applies to web/src/components/** : Vue components organized by feature (agents/, approvals/, budget/, common/, dashboard/, layout/, messages/, org-chart/, tasks/).
Applied to files:
web/src/pages/messages/MessageFilterBar.tsx
🔇 Additional comments (41)
web/src/__tests__/helpers/factories.ts (1)
183-212: LGTM!The
makeMessageandmakeChannelfactories are well-implemented:
makeMessageuses a deterministic timestamp ('2026-03-28T09:00:00.000Z'), addressing the previous review concern about test flakiness.- All required
MessageandMessageMetadatafields are correctly initialized with sensible defaults.- The spread pattern for
overridesis consistent with other factories in this file.web/src/__tests__/pages/messages/AttachmentList.test.tsx (1)
1-36: LGTM!The test suite provides good coverage for the
AttachmentListcomponent:
- Empty state correctly returns null
- Single and multiple attachments render their refs
- Icon rendering is verified via SVG element count
web/src/pages/messages/AttachmentList.stories.tsx (1)
1-31: LGTM!The Storybook module is well-configured:
- Correct
@storybook/reactimports forMetaandStoryObj- Accessibility testing enabled with
a11y: { test: 'error' }- Stories cover the relevant states: mixed types, single attachment, and empty
web/src/utils/messages.ts (2)
141-146: Metadata access is safe here.The previous review suggested guarding
msg.metadatawith optional chaining. However, perweb/src/api/types.ts,Message.metadatais required (typeMessageMetadata), not nullable. The existing!taskIdcheck on line 143 correctly handles the nullabletask_idfield. No change needed.
166-191: LGTM!The
filterMessagesfunction is clean and correct:
- Properly handles optional filters
- Case-insensitive search across content, sender, and to fields
- Returns a new array without mutating the input
web/src/pages/messages/MessageTypeBadge.tsx (1)
1-22: LGTM!Clean, well-structured component that:
- Uses
cn()for class composition- Applies semantic design tokens (
border-border,bg-surface,text-secondary)- Properly types props with a TypeScript interface
- Delegates label lookup to the utility function for maintainability
web/src/pages/messages/MessageTypeBadge.stories.tsx (1)
1-39: LGTM!The Storybook module is well-structured:
- Line length issue from previous review is resolved with one-item-per-line formatting
AllTypesstory provides visual regression coverage for all message types- Individual stories for common types enable focused testing
- Accessibility testing enabled with
a11y: { test: 'error' }web/src/__tests__/utils/messages.test.ts (2)
123-148: LGTM!Comprehensive test coverage for
groupMessagesByThread:
- Tests grouping by
task_id- Correctly handles standalone messages (null
task_id)- Covers empty array edge case
The long lines for metadata objects (lines 126-128) could be wrapped for readability, but this is a minor style consideration for test files.
150-201: LGTM!Excellent test coverage for
filterMessages:
- Tests individual filters (type, priority, search)
- Tests search across all searchable fields (content, sender, to)
- Tests combined filters
- Tests empty filter returning all messages
web/src/pages/messages/MessageDetailDrawer.stories.tsx (3)
14-14: Line exceeds 88-character limit.The long content string significantly exceeds the configured line length. Consider breaking it into a template literal or array join.
47-47: Line exceeds 88-character limit.The inline metadata object exceeds the line length limit. Wrap onto multiple lines.
1-12: LGTM on Storybook 10 setup and accessibility configuration.Correctly uses
storybook/testfor thefn()import and setsparameters.a11y.test: 'error'for WCAG compliance enforcement. The four stories provide good coverage of drawer states (full metadata, minimal, attachments-focused, closed).Also applies to: 28-40
web/src/__tests__/pages/messages/MessageDetailDrawer.test.tsx (2)
54-57: Line exceeds 88-character limit.The inline metadata object on line 56 exceeds the configured line length. Wrap onto multiple lines.
1-83: Good test coverage for MessageDetailDrawer.The test suite comprehensively covers:
- Sender name/avatar rendering
- Message content display
- Conditional metadata rows (present and null cases)
- Extra metadata key-value pairs
- Attachments rendering
- Null message fallback behavior
- Priority badge display
web/src/pages/messages/MessageFilterBar.tsx (4)
8-11: Lines exceed 88-character limit.The MESSAGE_TYPES array declaration has lines exceeding the configured limit. Place each item on its own line.
95-95: Line exceeds 88-character limit.The Search icon JSX line is approximately 118 characters. Break props onto separate lines.
44-53: LGTM on whitespace normalization.The
handleSearchChangecorrectly trims input before updating filters, ensuring whitespace-only input clears the search filter (search: undefined).
149-168: LGTM on FilterPill implementation.The
FilterPillPropsinterface is properly defined, and the component uses design tokens correctly (border-border,bg-surface,bg-card-hover,text-accent).web/src/pages/messages/MessageBubble.tsx (2)
31-40: Button exposes interactive semantics whenonClickis undefined.When
onClickis not provided, the button still renders with interactive semantics but performs no action. This can confuse assistive technology users.🛠️ Suggested fix
export function MessageBubble({ message, isNew, onClick }: MessageBubbleProps) { const { triggerFlash, flashStyle } = useFlash() const hasTriggeredRef = useRef(false) + const isInteractive = typeof onClick === 'function' useEffect(() => { if (isNew && !hasTriggeredRef.current) { hasTriggeredRef.current = true triggerFlash() } }, [isNew, triggerFlash]) const priorityColor = getMessagePriorityColor(message.priority) return ( <button type="button" onClick={onClick} + disabled={!isInteractive} + aria-disabled={!isInteractive} className={cn( 'flex w-full gap-3 rounded-lg p-3 text-left transition-colors', 'hover:bg-card-hover', 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent', + !isInteractive && 'cursor-default', )} style={flashStyle} >
17-26: LGTM on flash animation implementation.The
useRefguard (hasTriggeredRef) correctly ensures the flash animation triggers only once per component instance whenisNewbecomes true, preventing repeated animations on re-renders.web/src/__tests__/pages/messages/MessageFilterBar.test.tsx (4)
28-31: Line exceeds 88-character limit.Line 29 exceeds the configured line length. Break the JSX props onto separate lines.
76-81: Line exceeds 88-character limit.Line 77 exceeds the configured line length. Break the JSX props onto separate lines.
88-95: Line exceeds 88-character limit.Line 91 exceeds the configured line length. Break the JSX props onto separate lines.
60-74: LGTM on whitespace-only search test.Good addition of the edge case test ensuring whitespace-only input correctly clears the search filter by emitting
{ search: undefined }.web/src/pages/messages/MessageDetailDrawer.tsx (1)
1-116: LGTM!The component is well-structured with proper use of design tokens, shared components (
Drawer,Avatar,MessageTypeBadge,AttachmentList), and named interfaces for all props. The conditional rendering of metadata rows and the priority badge implementation usinggetMessagePriorityColor/getPriorityBadgeClassesare correct. The index-based key formetadata.extrawith the eslint-disable comment is an acceptable solution given the lack of stable IDs.docs/design/communication.md (1)
134-135: LGTM on nullability documentation.The clarification that all metadata fields are nullable except
extra(which defaults to an empty list) accurately reflects the implementation shown in the UI components and test fixtures.web/src/pages/messages/MessageFilterBar.stories.tsx (1)
1-33: LGTM!The Storybook file correctly uses
storybook/testimports, setsa11y.test: 'error'for WCAG compliance, and provides good coverage of filter states (no filters, single type filter, multiple filters, search only). The decorator ensuring consistent width is a nice touch.web/src/pages/messages/MessageThread.tsx (1)
1-90: LGTM!The component is well-structured with:
- Proper accessibility attributes (
aria-expanded,aria-hidden)- Correct use of design tokens and semantic classes
- Clean separation between single-message and multi-message thread rendering
- Appropriate animation using
springGentlefrom the motion library- Proper propagation of
isNewto trigger flash animations on new messagesweb/src/pages/messages/TimestampDivider.tsx (1)
1-13: LGTM!Clean, accessible implementation with proper semantic
role="separator"and design tokens (border-border,text-muted-foreground,font-mono).web/src/pages/messages/MessagesSkeleton.tsx (1)
1-39: LGTM!Well-structured skeleton with:
MessageRowSkeletonextracted to keep.map()callbacks simple (addressing prior review feedback)- Proper accessibility attributes (
role="status",aria-label="Loading messages")- Consistent use of design tokens
web/src/pages/messages/MessagesSkeleton.stories.tsx (1)
1-13: LGTM!Correctly configured Storybook story with WCAG compliance enforcement via
parameters.a11y.test: 'error'.web/src/__tests__/pages/messages/MessageBubble.test.tsx (1)
1-70: LGTM!Comprehensive test coverage including:
- Sender name, content, and avatar rendering
- Message type badges
- Priority indicators (high, urgent) and absence for normal priority
- Attachments rendering
- Click handler invocation
- Correct mock matching the
UseFlashReturninterfaceweb/src/__tests__/pages/messages/ChannelListItem.test.tsx (1)
1-72: LGTM!Good test coverage including:
- Accessibility validation (
aria-currentattribute presence/absence)- Unread badge visibility logic
- Channel type-specific rendering
- Click handler invocation
- Line length issues from prior review have been addressed
web/src/pages/messages/TimestampDivider.stories.tsx (1)
1-16: LGTM!Good Storybook coverage with:
- WCAG compliance enforcement via
parameters.a11y.test: 'error'- Multiple story variants covering different label formats (Today, Yesterday, formatted date)
- Decorator for consistent visual testing width
web/src/__tests__/pages/messages/MessageThread.test.tsx (1)
22-81: Strong behavioral coverage for thread UI states and interactions.This suite cleanly covers collapsed/expanded rendering, thread toggling, selection callbacks, and the empty-array guard.
web/src/pages/messages/ChannelListItem.tsx (1)
27-49: State rendering and accessibility hooks are implemented cleanly.
aria-currenton active selection and conditional unread badge behavior are both wired correctly.web/src/__tests__/pages/messages/ChannelSidebar.test.tsx (1)
26-66: Good coverage of sidebar grouping, selection, and conditional states.The suite validates the key user-facing paths, including loading/empty behavior and active-channel semantics.
web/src/pages/messages/MessageThread.stories.tsx (1)
32-53: Story coverage is well-scoped for the main thread presentation states.Collapsed, expanded, and single-message variants are all represented with a11y checks enabled.
web/src/pages/messages/ChannelListItem.stories.tsx (1)
16-62: State matrix forChannelListItemis solid.Type variants, active state, and unread combinations are all covered clearly.
web/src/pages/messages/ChannelSidebar.stories.tsx (1)
26-48: Great story coverage for conditional sidebar states.Including loading and empty alongside active/unread variants gives good visual and a11y regression coverage.
web/src/pages/messages/MessageBubble.stories.tsx (1)
30-66: Variant coverage forMessageBubbleis complete and practical.Priority, message type, attachments, and new-message states are all represented with clear args.
| "id": "msg-uuid", | ||
| "timestamp": "2026-02-27T10:30:00Z", | ||
| "from": "sarah_chen", | ||
| "sender": "sarah_chen", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current Message model field definition and alias configuration
# Check the Message model for sender field and any alias
rg -n -A3 -B3 'sender.*Field|Field.*sender|alias.*from|from.*alias' --type py
# Check if there's any JSON serialization config
ast-grep --pattern 'class Message($_) {
$$$
sender$_
$$$
}'Repository: Aureliolo/synthorg
Length of output: 9644
Update JSON field name from "sender" to "from" to match backend serialization.
The Message model serializes the sender field to "from" in JSON via Pydantic alias. Line 115 and related JSON examples throughout this section show "sender", but the actual serialized format is "from". Update the documentation to reflect what the backend actually outputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/communication.md` at line 115, The docs use the JSON field name
"sender" but the backend Message model serializes that attribute as "from"
(Pydantic alias); update this and every related JSON example in this document to
use "from" instead of "sender" so the examples match the Message model's
serialized output and backend behavior.
| export function useMessagesData(activeChannel: string | null): UseMessagesDataReturn { | ||
| // Ref avoids stale closure in WS handler (useWebSocket | ||
| // registers bindings once, never re-registers) | ||
| const activeChannelRef = useRef(activeChannel) | ||
| useEffect(() => { | ||
| activeChannelRef.current = activeChannel | ||
| }, [activeChannel]) | ||
|
|
||
| const channels = useMessagesStore((s) => s.channels) | ||
| const channelsLoading = useMessagesStore((s) => s.channelsLoading) | ||
| const channelsError = useMessagesStore((s) => s.channelsError) | ||
| const unreadCounts = useMessagesStore((s) => s.unreadCounts) | ||
|
|
||
| const messages = useMessagesStore((s) => s.messages) | ||
| const total = useMessagesStore((s) => s.total) | ||
| const loading = useMessagesStore((s) => s.loading) | ||
| const loadingMore = useMessagesStore((s) => s.loadingMore) | ||
| const error = useMessagesStore((s) => s.error) | ||
|
|
||
| const expandedThreads = useMessagesStore((s) => s.expandedThreads) | ||
| const toggleThread = useMessagesStore((s) => s.toggleThread) | ||
| const newMessageIds = useMessagesStore((s) => s.newMessageIds) | ||
|
|
||
| // Fetch channels on mount | ||
| useEffect(() => { | ||
| useMessagesStore.getState().fetchChannels() | ||
| }, []) | ||
|
|
||
| // Fetch messages when active channel changes; reset unread | ||
| useEffect(() => { | ||
| if (!activeChannel) return | ||
| useMessagesStore.getState().fetchMessages(activeChannel) | ||
| useMessagesStore.getState().resetUnread(activeChannel) | ||
| }, [activeChannel]) | ||
|
|
||
| // Polling for current channel refresh | ||
| const pollFn = useCallback(async () => { | ||
| if (!activeChannel) return | ||
| await useMessagesStore.getState().fetchMessages(activeChannel) | ||
| }, [activeChannel]) | ||
|
|
||
| const polling = usePolling(pollFn, MESSAGES_POLL_INTERVAL) | ||
|
|
||
| useEffect(() => { | ||
| if (!activeChannel) return | ||
| polling.start() | ||
| return () => polling.stop() | ||
| // eslint-disable-next-line @eslint-react/exhaustive-deps -- start/stop are stable useCallback refs | ||
| }, [activeChannel, polling.start, polling.stop]) | ||
|
|
||
| // WebSocket bindings (stable -- reads activeChannel via ref) | ||
| const bindings: ChannelBinding[] = useMemo( | ||
| () => | ||
| MESSAGES_WS_CHANNELS.map((channel) => ({ | ||
| channel, | ||
| handler: (event) => { | ||
| useMessagesStore | ||
| .getState() | ||
| .handleWsEvent(event, activeChannelRef.current) | ||
| }, | ||
| })), | ||
| [], | ||
| ) | ||
|
|
||
| const { connected: wsConnected, setupError: wsSetupError } = useWebSocket({ bindings }) | ||
|
|
||
| // Computed | ||
| const hasMore = useMemo(() => messages.length < total, [messages.length, total]) | ||
|
|
||
| const fetchMore = useCallback(() => { | ||
| if (!activeChannel) return | ||
| useMessagesStore.getState().fetchMoreMessages(activeChannel) | ||
| }, [activeChannel]) | ||
|
|
||
| return { | ||
| channels, | ||
| channelsLoading, | ||
| channelsError, | ||
| unreadCounts, | ||
| messages, | ||
| total, | ||
| loading, | ||
| loadingMore, | ||
| error, | ||
| hasMore, | ||
| expandedThreads, | ||
| toggleThread, | ||
| newMessageIds, | ||
| fetchMore, | ||
| wsConnected, | ||
| wsSetupError, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split useMessagesData into smaller hooks.
This hook is carrying store selection, initial loads, channel-change side effects, polling, WS setup, and derived state in one 90+ line function. Extracting the fetch lifecycle and realtime wiring into focused helpers will make it much easier to reason about and bring it back under the repo limit.
As per coding guidelines: "web/src/**/*.{ts,tsx}: Keep functions under 50 lines and files under 800 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/hooks/useMessagesData.ts` around lines 40 - 132, The useMessagesData
hook is too large; split responsibilities into smaller hooks: extract store
selectors into a new hook (e.g., useMessagesStoreSelectors) that returns
channels, channelsLoading, channelsError, unreadCounts, messages, total,
loading, loadingMore, error, expandedThreads, toggleThread, newMessageIds;
extract lifecycle/fetch logic into useMessagesLifecycle which calls
useMessagesStore.getState().fetchChannels on mount, calls fetchMessages and
resetUnread when activeChannel changes (using activeChannelRef and fetchMore),
and wires polling (move pollFn and polling into that hook using usePolling and
expose start/stop or let hook manage it); extract WebSocket wiring into
useMessagesWebsocket which builds bindings (using activeChannelRef) and calls
useWebSocket to return wsConnected and wsSetupError; finally have
useMessagesData compose these smaller hooks and compute hasMore and return the
same shape — reference symbols: useMessagesData,
useMessagesStore.getState().fetchChannels, fetchMessages, resetUnread,
fetchMore, pollFn, polling, usePolling, MESSAGES_POLL_INTERVAL, bindings,
MESSAGES_WS_CHANNELS, useWebSocket, activeChannelRef, and hasMore.
| export function MessageList({ | ||
| messages, | ||
| expandedThreads, | ||
| toggleThread, | ||
| onSelectMessage, | ||
| hasMore, | ||
| loadingMore, | ||
| onLoadMore, | ||
| newMessageIds, | ||
| }: MessageListProps) { | ||
| const containerRef = useRef<HTMLDivElement>(null) | ||
| const wasAtBottomRef = useRef(true) | ||
|
|
||
| // Track whether user is near the bottom | ||
| const handleScroll = useCallback(() => { | ||
| const el = containerRef.current | ||
| if (!el) return | ||
| wasAtBottomRef.current = el.scrollHeight - el.scrollTop - el.clientHeight < 80 | ||
| }, []) | ||
|
|
||
| // Auto-scroll to bottom when new messages arrive (if user was at bottom) | ||
| const prevLengthRef = useRef(messages.length) | ||
| useEffect(() => { | ||
| if (messages.length > prevLengthRef.current && wasAtBottomRef.current) { | ||
| const el = containerRef.current | ||
| if (el) el.scrollTop = el.scrollHeight | ||
| } | ||
| prevLengthRef.current = messages.length | ||
| }, [messages.length]) | ||
|
|
||
| // Sort messages ascending by timestamp for display | ||
| const sorted = useMemo( | ||
| () => [...messages].sort((a, b) => a.timestamp.localeCompare(b.timestamp)), | ||
| [messages], | ||
| ) | ||
|
|
||
| const dateGroups = useMemo(() => groupMessagesByDate(sorted), [sorted]) | ||
|
|
||
| if (messages.length === 0) { | ||
| return ( | ||
| <EmptyState | ||
| icon={MessageSquare} | ||
| title="No messages" | ||
| description="No messages in this channel yet." | ||
| /> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <div | ||
| ref={containerRef} | ||
| onScroll={handleScroll} | ||
| className="flex-1 overflow-y-auto" | ||
| aria-live="polite" | ||
| aria-label="Messages" | ||
| > | ||
| {/* Load more button */} | ||
| {hasMore && ( | ||
| <div className="flex justify-center py-2"> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={onLoadMore} | ||
| disabled={loadingMore} | ||
| > | ||
| {loadingMore && <Loader2 className="size-3 animate-spin" />} | ||
| {loadingMore ? 'Loading...' : 'Load earlier messages'} | ||
| </Button> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Date-grouped messages */} | ||
| {[...dateGroups.entries()].map(([dateKey, msgs]) => { | ||
| const { threads, standalone } = groupMessagesByThread(msgs) | ||
|
|
||
| return ( | ||
| <div key={dateKey}> | ||
| <TimestampDivider label={getDateGroupLabel(dateKey)} /> | ||
|
|
||
| {/* Threads */} | ||
| {[...threads.entries()].map(([taskId, threadMsgs]) => ( | ||
| <MessageThread | ||
| key={taskId} | ||
| messages={threadMsgs} | ||
| expanded={expandedThreads.has(taskId)} | ||
| onToggle={() => toggleThread(taskId)} | ||
| onSelectMessage={onSelectMessage} | ||
| newMessageIds={newMessageIds} | ||
| /> | ||
| ))} | ||
|
|
||
| {/* Standalone messages */} | ||
| {standalone.map((msg) => ( | ||
| <MessageBubble | ||
| key={msg.id} | ||
| message={msg} | ||
| isNew={newMessageIds?.has(msg.id)} | ||
| onClick={() => onSelectMessage(msg.id)} | ||
| /> | ||
| ))} | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor MessageList to reduce function size and extract mapped JSX.
This component is doing orchestration + rendering + nested mapped markup in one function. Split date-group rendering into a dedicated subcomponent (or helper renderer) to keep MessageList focused and testable.
♻️ Suggested shape
export function MessageList({...}: MessageListProps) {
...
- return (
- <div ...>
- ...
- {[...dateGroups.entries()].map(([dateKey, msgs]) => {
- ...
- })}
- </div>
- )
+ return (
+ <div ...>
+ ...
+ {[...dateGroups.entries()].map(([dateKey, msgs]) => (
+ <DateMessageGroup
+ key={dateKey}
+ dateKey={dateKey}
+ msgs={msgs}
+ expandedThreads={expandedThreads}
+ toggleThread={toggleThread}
+ onSelectMessage={onSelectMessage}
+ newMessageIds={newMessageIds}
+ />
+ ))}
+ </div>
+ )
}As per coding guidelines: "Keep functions under 50 lines and files under 800 lines" and "Do NOT create complex (>8 line) JSX inside .map() -- extract to a shared component".
Also applies to: 94-124
| const { threads, standalone } = groupMessagesByThread(msgs) | ||
|
|
||
| return ( | ||
| <div key={dateKey}> | ||
| <TimestampDivider label={getDateGroupLabel(dateKey)} /> | ||
|
|
||
| {/* Threads */} | ||
| {[...threads.entries()].map(([taskId, threadMsgs]) => ( | ||
| <MessageThread | ||
| key={taskId} | ||
| messages={threadMsgs} | ||
| expanded={expandedThreads.has(taskId)} | ||
| onToggle={() => toggleThread(taskId)} | ||
| onSelectMessage={onSelectMessage} | ||
| newMessageIds={newMessageIds} | ||
| /> | ||
| ))} | ||
|
|
||
| {/* Standalone messages */} | ||
| {standalone.map((msg) => ( | ||
| <MessageBubble | ||
| key={msg.id} | ||
| message={msg} | ||
| isNew={newMessageIds?.has(msg.id)} | ||
| onClick={() => onSelectMessage(msg.id)} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Thread-first rendering breaks chronological order within each date group.
Line 101 renders every thread block before Line 114 standalone messages, which can move standalone messages after later thread messages and distort timeline order. Render in original msgs sequence and emit each thread once at first encounter.
🔧 Proposed fix (preserve timeline order)
- const { threads, standalone } = groupMessagesByThread(msgs)
+ const { threads } = groupMessagesByThread(msgs)
+ const renderedThreads = new Set<string>()
return (
<div key={dateKey}>
<TimestampDivider label={getDateGroupLabel(dateKey)} />
- {/* Threads */}
- {[...threads.entries()].map(([taskId, threadMsgs]) => (
- <MessageThread
- key={taskId}
- messages={threadMsgs}
- expanded={expandedThreads.has(taskId)}
- onToggle={() => toggleThread(taskId)}
- onSelectMessage={onSelectMessage}
- newMessageIds={newMessageIds}
- />
- ))}
-
- {/* Standalone messages */}
- {standalone.map((msg) => (
- <MessageBubble
- key={msg.id}
- message={msg}
- isNew={newMessageIds?.has(msg.id)}
- onClick={() => onSelectMessage(msg.id)}
- />
- ))}
+ {msgs.map((msg) => {
+ const taskId = msg.metadata.task_id
+ if (!taskId) {
+ return (
+ <MessageBubble
+ key={msg.id}
+ message={msg}
+ isNew={newMessageIds?.has(msg.id)}
+ onClick={() => onSelectMessage(msg.id)}
+ />
+ )
+ }
+
+ if (renderedThreads.has(taskId)) return null
+ renderedThreads.add(taskId)
+ const threadMsgs = threads.get(taskId)
+ if (!threadMsgs) return null
+
+ return (
+ <MessageThread
+ key={taskId}
+ messages={threadMsgs}
+ expanded={expandedThreads.has(taskId)}
+ onToggle={() => toggleThread(taskId)}
+ onSelectMessage={onSelectMessage}
+ newMessageIds={newMessageIds}
+ />
+ )
+ })}
</div>
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/messages/MessageList.tsx` around lines 95 - 121,
groupMessagesByThread currently separates threads and standalone and renders all
threads first then standalone, breaking chronological order; change rendering to
iterate the original msgs sequence for the date group and emit each item in
order, rendering a MessageBubble for standalone messages and rendering the
corresponding MessageThread exactly once when you encounter the first message of
a thread. Specifically, in the rendering block that now uses threads/standalone,
iterate over msgs (the original array returned for the dateKey), keep a local
Set (e.g., emittedThreads) to track which taskId threads have already been
output, and for each msg if msg.taskId is present and not in emittedThreads
render MessageThread with messages={threads.get(taskId)} and mark taskId
emitted, otherwise render MessageBubble for that msg; preserve existing props
(expandedThreads, toggleThread, onSelectMessage, newMessageIds) and keep
TimestampDivider/getDateGroupLabel as-is.
| export default function MessagesPage() { | ||
| return <PlaceholderPage title="Messages" /> | ||
| const [searchParams, setSearchParams] = useSearchParams() | ||
| const wasConnectedRef = useRef(false) | ||
|
|
||
| const activeChannel = searchParams.get('channel') | ||
|
|
||
| const { | ||
| channels, | ||
| channelsLoading, | ||
| channelsError, | ||
| unreadCounts, | ||
| messages, | ||
| total, | ||
| loading, | ||
| loadingMore, | ||
| error, | ||
| hasMore, | ||
| expandedThreads, | ||
| toggleThread, | ||
| newMessageIds, | ||
| fetchMore, | ||
| wsConnected, | ||
| wsSetupError, | ||
| } = useMessagesData(activeChannel) | ||
|
|
||
| // Track WS connection to avoid flash on initial load | ||
| if (wsConnected) wasConnectedRef.current = true | ||
|
|
||
| // Auto-clear new-message flash IDs after animation | ||
| useEffect(() => { | ||
| if (newMessageIds.size === 0) return | ||
| const timer = setTimeout(() => { | ||
| useMessagesStore | ||
| .getState() | ||
| .clearNewMessageIds() | ||
| }, 2000) | ||
| return () => clearTimeout(timer) | ||
| }, [newMessageIds]) | ||
|
|
||
| // URL-synced filters | ||
| const filters: MessagePageFilters = useMemo(() => { | ||
| const rawType = searchParams.get('type') | ||
| const rawPriority = searchParams.get('priority') | ||
| return { | ||
| type: rawType && VALID_TYPES.has(rawType) ? rawType as MessageType : undefined, | ||
| priority: rawPriority && VALID_PRIORITIES.has(rawPriority) ? rawPriority as MessagePriority : undefined, | ||
| search: searchParams.get('search') ?? undefined, | ||
| } | ||
| }, [searchParams]) | ||
|
|
||
| const handleFiltersChange = useCallback((newFilters: MessagePageFilters) => { | ||
| setSearchParams((prev) => { | ||
| const next = new URLSearchParams(prev) | ||
| next.delete('type') | ||
| next.delete('priority') | ||
| next.delete('search') | ||
| if (newFilters.type) next.set('type', newFilters.type) | ||
| if (newFilters.priority) next.set('priority', newFilters.priority) | ||
| if (newFilters.search) next.set('search', newFilters.search) | ||
| return next | ||
| }) | ||
| }, [setSearchParams]) | ||
|
|
||
| const handleSelectChannel = useCallback((name: string) => { | ||
| setSearchParams((prev) => { | ||
| const next = new URLSearchParams(prev) | ||
| next.set('channel', name) | ||
| next.delete('message') | ||
| next.delete('type') | ||
| next.delete('priority') | ||
| next.delete('search') | ||
| return next | ||
| }) | ||
| }, [setSearchParams]) | ||
|
|
||
| const selectedMessageId = searchParams.get('message') | ||
| const selectedMessage = useMemo( | ||
| () => messages.find((m) => m.id === selectedMessageId) ?? null, | ||
| [messages, selectedMessageId], | ||
| ) | ||
|
|
||
| const handleSelectMessage = useCallback((id: string) => { | ||
| setSearchParams((prev) => { | ||
| const next = new URLSearchParams(prev) | ||
| next.set('message', id) | ||
| return next | ||
| }) | ||
| }, [setSearchParams]) | ||
|
|
||
| const handleCloseDrawer = useCallback(() => { | ||
| setSearchParams((prev) => { | ||
| const next = new URLSearchParams(prev) | ||
| next.delete('message') | ||
| return next | ||
| }) | ||
| }, [setSearchParams]) | ||
|
|
||
| // Client-side filtering | ||
| const filtered = useMemo(() => filterMessages(messages, filters), [messages, filters]) | ||
| const hasFilters = !!(filters.type || filters.priority || filters.search) | ||
|
|
||
| // Loading state | ||
| if (loading && messages.length === 0 && channelsLoading && channels.length === 0) { | ||
| return <MessagesSkeleton /> | ||
| } | ||
|
|
||
| return ( | ||
| <div className="flex h-[calc(100vh-theme(spacing.16))] gap-6"> | ||
| {/* Channel sidebar */} | ||
| <ErrorBoundary level="section"> | ||
| <ChannelSidebar | ||
| channels={channels} | ||
| activeChannel={activeChannel} | ||
| unreadCounts={unreadCounts} | ||
| onSelectChannel={handleSelectChannel} | ||
| loading={channelsLoading} | ||
| /> | ||
| </ErrorBoundary> | ||
|
|
||
| {/* Main content area */} | ||
| <div className="flex min-w-0 flex-1 flex-col gap-4"> | ||
| <h1 className="text-lg font-semibold text-foreground">Messages</h1> | ||
|
|
||
| {/* Error banners */} | ||
| {error && ( | ||
| <div className="flex items-center gap-2 rounded-lg border border-danger/30 bg-danger/5 px-4 py-2 text-sm text-danger"> | ||
| <AlertTriangle className="size-4 shrink-0" /> | ||
| {error} | ||
| </div> | ||
| )} | ||
| {channelsError && channelsError !== error && ( | ||
| <div className="flex items-center gap-2 rounded-lg border border-danger/30 bg-danger/5 px-4 py-2 text-sm text-danger"> | ||
| <AlertTriangle className="size-4 shrink-0" /> | ||
| {channelsError} | ||
| </div> | ||
| )} | ||
|
|
||
| {(wsSetupError || (wasConnectedRef.current && !wsConnected)) && !loading && ( | ||
| <div className="flex items-center gap-2 rounded-lg border border-warning/30 bg-warning/5 px-4 py-2 text-sm text-warning"> | ||
| <WifiOff className="size-4 shrink-0" /> | ||
| {wsSetupError ?? 'Real-time updates disconnected. Data may be stale.'} | ||
| </div> | ||
| )} | ||
|
|
||
| {/* No channel selected prompt */} | ||
| {!activeChannel && ( | ||
| <EmptyState | ||
| icon={MessageSquare} | ||
| title="Select a channel" | ||
| description="Choose a channel from the sidebar to view messages." | ||
| /> | ||
| )} | ||
|
|
||
| {/* Channel selected -- show filter bar and message list */} | ||
| {activeChannel && ( | ||
| <> | ||
| <MessageFilterBar | ||
| filters={filters} | ||
| onFiltersChange={handleFiltersChange} | ||
| totalCount={total} | ||
| filteredCount={hasFilters ? filtered.length : undefined} | ||
| /> | ||
|
|
||
| <ErrorBoundary level="section"> | ||
| <MessageList | ||
| messages={filtered} | ||
| expandedThreads={expandedThreads} | ||
| toggleThread={toggleThread} | ||
| onSelectMessage={handleSelectMessage} | ||
| hasMore={hasMore && !hasFilters} | ||
| loadingMore={loadingMore} | ||
| onLoadMore={fetchMore} | ||
| newMessageIds={newMessageIds} | ||
| /> | ||
| </ErrorBoundary> | ||
| </> | ||
| )} | ||
| </div> | ||
|
|
||
| {/* Detail drawer */} | ||
| <AnimatePresence> | ||
| {selectedMessageId && ( | ||
| <MessageDetailDrawer | ||
| message={selectedMessage} | ||
| open={!!selectedMessageId} | ||
| onClose={handleCloseDrawer} | ||
| /> | ||
| )} | ||
| </AnimatePresence> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Break MessagesPage into smaller units.
This component is handling URL parsing/sync, WS connection tracking, flash cleanup, banner logic, selection, filtering, and all render branches in one ~190-line function. Extract the search-param/filter state and the message-panel rendering into smaller hooks/components so this stays maintainable.
As per coding guidelines: "web/src/**/*.{ts,tsx}: Keep functions under 50 lines and files under 800 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/MessagesPage.tsx` around lines 23 - 214, MessagesPage is too
large and does too many responsibilities (URL parsing/sync, WS connection
tracking, flash cleanup, banner logic, selection, filtering, and rendering);
split it into smaller units: extract URL/filter logic into a custom hook (e.g.,
useMessagePageFilters) that returns filters, handleFiltersChange, activeChannel,
selectedMessageId, handleSelectChannel, handleSelectMessage, handleCloseDrawer;
extract WS tracking and flash cleanup into small hooks (e.g.,
useWsConnectionTracker and useAutoClearNewMessageIds) that encapsulate
wasConnectedRef update and the setTimeout clear of newMessageIds; move the main
render block for the center panel (filter bar, error banners, MessageList and
EmptyState) into a new presentational component (e.g., MessagePanel) that
accepts props from the hooks and from useMessagesData (channels, messages,
loading, error, etc.); ensure each new function/component
(useMessagePageFilters, useWsConnectionTracker, useAutoClearNewMessageIds,
MessagePanel) keeps body under ~50 lines and replaces the corresponding logic in
MessagesPage so MessagesPage becomes a thin coordinator that composes these
pieces.
| // Auto-clear new-message flash IDs after animation | ||
| useEffect(() => { | ||
| if (newMessageIds.size === 0) return | ||
| const timer = setTimeout(() => { | ||
| useMessagesStore | ||
| .getState() | ||
| .clearNewMessageIds() | ||
| }, 2000) | ||
| return () => clearTimeout(timer) | ||
| }, [newMessageIds]) |
There was a problem hiding this comment.
newMessageIds never ages out under steady traffic.
Each WS arrival replaces newMessageIds with a fresh Set, which restarts this effect and postpones cleanup for every older ID. On a busy channel the set can grow without bound and older bubbles stay marked "new" far longer than the intended 2 seconds; this needs per-ID expiry instead of one timer for the whole set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/MessagesPage.tsx` around lines 51 - 60, The current effect
restarts on every replacement of newMessageIds so IDs don't expire under steady
traffic; change to per-ID expiry instead of a single-set timer by either (a)
storing timestamps (Map<id,ts>) in the messages store and updating it in the
message arrival handler, then have a single interval/effect in MessagesPage that
prunes ids older than 2000ms from that Map and calls
useMessagesStore.getState().clearSpecificNewMessageIds(expiredIds), or (b)
schedule a per-id setTimeout when adding an id in the arrival code that calls a
new store method clearNewMessageId(id) after 2000ms; update/merge the Set rather
than replacing it so useEffect/useMessagesStore.clearNewMessageIds can operate
on individual IDs. Ensure to add/rename store methods (e.g., clearNewMessageId,
clearSpecificNewMessageIds) referenced above.
🤖 I have created a release *beep* *boop* --- #MAJOR CHANGES; We got a somewhat working webui :) ## [0.5.0](v0.4.9...v0.5.0) (2026-03-30) ### Features * add analytics trends and budget forecast API endpoints ([#798](#798)) ([16b61f5](16b61f5)) * add department policies to default templates ([#852](#852)) ([7a41548](7a41548)) * add remaining activity event types (task_started, tool_used, delegation, cost_incurred) ([#832](#832)) ([4252fac](4252fac)) * agent performance, activity, and history API endpoints ([#811](#811)) ([9b75c1d](9b75c1d)) * Agent Profiles and Detail pages (biography, career, performance) ([#874](#874)) ([62d7880](62d7880)) * app shell, Storybook, and CI/CD pipeline ([#819](#819)) ([d4dde90](d4dde90)) * Approvals page with risk grouping, urgency indicators, batch actions ([#889](#889)) ([4e9673d](4e9673d)) * Budget Panel page (P&L dashboard, breakdown charts, forecast) ([#890](#890)) ([b63b0f1](b63b0f1)) * build infrastructure layer (API client, auth, WebSocket) ([#815](#815)) ([9f01d3e](9f01d3e)) * CLI global options infrastructure, UI modes, exit codes, env vars ([#891](#891)) ([fef4fc5](fef4fc5)) * CodeMirror editor and theme preferences toggle ([#905](#905), [#807](#807)) ([#909](#909)) ([41fbedc](41fbedc)) * Company page (department/agent management) ([#888](#888)) ([cfb88b0](cfb88b0)) * comprehensive hint coverage across all CLI commands ([#900](#900)) ([937974e](937974e)) * config system extensions, per-command flags for init/start/stop/status/logs ([#895](#895)) ([32f83fe](32f83fe)) * configurable currency system replacing hardcoded USD ([#854](#854)) ([b372551](b372551)) * Dashboard page (metric cards, activity feed, budget burn) ([#861](#861)) ([7d519d5](7d519d5)) * department health, provider status, and activity feed endpoints ([#818](#818)) ([6d5f196](6d5f196)) * design tokens and core UI components ([#833](#833)) ([ed887f2](ed887f2)) * extend approval, meeting, and budget API responses ([#834](#834)) ([31472bf](31472bf)) * frontend polish -- real-time UX, accessibility, responsive, performance ([#790](#790), [#792](#792), [#791](#791), [#793](#793)) ([#917](#917)) ([f04a537](f04a537)) * implement human roles and access control levels ([#856](#856)) ([d6d8a06](d6d8a06)) * implement semantic conflict detection in workspace merge ([#860](#860)) ([d97283b](d97283b)) * interaction components and animation patterns ([#853](#853)) ([82d4b01](82d4b01)) * Login page + first-run bootstrap + Company page ([#789](#789), [#888](#888)) ([#896](#896)) ([8758e8d](8758e8d)) * Meetings page with timeline viz, token bars, contribution formatting ([#788](#788)) ([#904](#904)) ([b207f46](b207f46)) * Messages page with threading, channel badges, sender indicators ([#787](#787)) ([#903](#903)) ([28293ad](28293ad)) * Org Chart force-directed view and drag-drop reassignment ([#872](#872), [#873](#873)) ([#912](#912)) ([a68a938](a68a938)) * Org Chart page (living nodes, status, CRUD, department health) ([#870](#870)) ([0acbdae](0acbdae)) * per-command flags for remaining commands, auto-behavior wiring, help/discoverability ([#897](#897)) ([3f7afa2](3f7afa2)) * Providers page with backend rework -- health, CRUD, subscription auth ([#893](#893)) ([9f8dd98](9f8dd98)) * scaffold React + Vite + TypeScript + Tailwind project ([#799](#799)) ([bd151aa](bd151aa)) * Settings page with search, dependency indicators, grouped rendering ([#784](#784)) ([#902](#902)) ([a7b9870](a7b9870)) * Setup Wizard rebuild with template comparison, cost estimator, theme customization ([#879](#879)) ([ae8b50b](ae8b50b)) * setup wizard UX -- template filters, card metadata, provider form reuse ([#910](#910)) ([7f04676](7f04676)) * setup wizard UX overhaul -- mode choice, step reorder, provider fixes ([#907](#907)) ([ee964c4](ee964c4)) * structured ModelRequirement in template agent configs ([#795](#795)) ([7433548](7433548)) * Task Board page (rich Kanban, filtering, dependency viz) ([#871](#871)) ([04a19b0](04a19b0)) ### Bug Fixes * align frontend types with backend and debounce WS refetches ([#916](#916)) ([134c11b](134c11b)) * auto-cleanup targets newly pulled images instead of old ones ([#884](#884)) ([50e6591](50e6591)) * correct wipe backup-skip flow and harden error handling ([#808](#808)) ([c05860f](c05860f)) * improve provider setup in wizard, subscription auth, dashboard bugs ([#914](#914)) ([87bf8e6](87bf8e6)) * improve update channel detection and add config get command ([#814](#814)) ([6b137f0](6b137f0)) * resolve all ESLint warnings, add zero-warnings enforcement ([#899](#899)) ([079b46a](079b46a)) * subscription auth uses api_key, base URL optional for cloud providers ([#915](#915)) ([f0098dd](f0098dd)) ### Refactoring * semantic analyzer cleanup -- shared filtering, concurrency, extraction ([#908](#908)) ([81372bf](81372bf)) ### Documentation * brand identity and UX design system from [#765](#765) exploration ([#804](#804)) ([389a9f4](389a9f4)) * page structure and information architecture for v0.5.0 dashboard ([#809](#809)) ([f8d6d4a](f8d6d4a)) * write UX design guidelines with WCAG-verified color system ([#816](#816)) ([4a4594e](4a4594e)) ### Tests * add unit tests for agent hooks and page components ([#875](#875)) ([#901](#901)) ([1d81546](1d81546)) ### CI/CD * bump actions/deploy-pages from 4.0.5 to 5.0.0 in the major group ([#831](#831)) ([01c19de](01c19de)) * bump astral-sh/setup-uv from 7.6.0 to 8.0.0 in /.github/actions/setup-python-uv in the all group ([#920](#920)) ([5f6ba54](5f6ba54)) * bump codecov/codecov-action from 5.5.3 to 6.0.0 in the major group ([#868](#868)) ([f22a181](f22a181)) * bump github/codeql-action from 4.34.1 to 4.35.0 in the all group ([#883](#883)) ([87a4890](87a4890)) * bump sigstore/cosign-installer from 4.1.0 to 4.1.1 in the minor-and-patch group ([#830](#830)) ([7a69050](7a69050)) * bump the all group with 3 updates ([#923](#923)) ([ff27c8e](ff27c8e)) * bump wrangler from 4.76.0 to 4.77.0 in /.github in the minor-and-patch group ([#822](#822)) ([07d43eb](07d43eb)) * bump wrangler from 4.77.0 to 4.78.0 in /.github in the all group ([#882](#882)) ([f84118d](f84118d)) ### Maintenance * add design system enforcement hook and component inventory ([#846](#846)) ([15abc43](15abc43)) * add dev-only auth bypass for frontend testing ([#885](#885)) ([6cdcd8a](6cdcd8a)) * add pre-push rebase check hook ([#855](#855)) ([b637a04](b637a04)) * backend hardening -- eviction/size-caps and model validation ([#911](#911)) ([81253d9](81253d9)) * bump axios from 1.13.6 to 1.14.0 in /web in the all group across 1 directory ([#922](#922)) ([b1b0232](b1b0232)) * bump brace-expansion from 5.0.4 to 5.0.5 in /web ([#862](#862)) ([ba4a565](ba4a565)) * bump eslint-plugin-react-refresh from 0.4.26 to 0.5.2 in /web ([#801](#801)) ([7574bb5](7574bb5)) * bump faker from 40.11.0 to 40.11.1 in the minor-and-patch group ([#803](#803)) ([14d322e](14d322e)) * bump https://github.com/astral-sh/ruff-pre-commit from v0.15.7 to 0.15.8 ([#864](#864)) ([f52901e](f52901e)) * bump nginxinc/nginx-unprivileged from `6582a34` to `f99cc61` in /docker/web in the all group ([#919](#919)) ([df85e4f](df85e4f)) * bump nginxinc/nginx-unprivileged from `ccbac1a` to `6582a34` in /docker/web ([#800](#800)) ([f4e9450](f4e9450)) * bump node from `44bcbf4` to `71be405` in /docker/sandbox ([#827](#827)) ([91bec67](91bec67)) * bump node from `5209bca` to `cf38e1f` in /docker/web ([#863](#863)) ([66d6043](66d6043)) * bump picomatch in /site ([#842](#842)) ([5f20bcc](5f20bcc)) * bump recharts 2->3 and @types/node 22->25 in /web ([#802](#802)) ([a908800](a908800)) * Bump requests from 2.32.5 to 2.33.0 ([#843](#843)) ([41daf69](41daf69)) * bump smol-toml from 1.6.0 to 1.6.1 in /site ([#826](#826)) ([3e5dbe4](3e5dbe4)) * bump the all group with 3 updates ([#921](#921)) ([7bace0b](7bace0b)) * bump the minor-and-patch group across 1 directory with 2 updates ([#829](#829)) ([93e611f](93e611f)) * bump the minor-and-patch group across 1 directory with 3 updates ([#841](#841)) ([7010c8e](7010c8e)) * bump the minor-and-patch group across 1 directory with 3 updates ([#869](#869)) ([548cee5](548cee5)) * bump the minor-and-patch group in /site with 2 updates ([#865](#865)) ([9558101](9558101)) * bump the minor-and-patch group with 2 updates ([#867](#867)) ([4830706](4830706)) * consolidate Dependabot groups to 1 PR per ecosystem ([06d2556](06d2556)) * consolidate Dependabot groups to 1 PR per ecosystem ([#881](#881)) ([06d2556](06d2556)) * improve worktree skill with full dep sync and status enhancements ([#906](#906)) ([772c625](772c625)) * remove Vue remnants and document framework decision ([#851](#851)) ([bf2adf6](bf2adf6)) * update web dependencies and fix brace-expansion CVE ([#880](#880)) ([a7a0ed6](a7a0ed6)) * upgrade to Storybook 10 and TypeScript 6 ([#845](#845)) ([52d95f2](52d95f2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
task_idwith expand/collapse UITest plan
npm --prefix web run lint-- zero warningsnpm --prefix web run type-check-- no type errorsnpm --prefix web run test-- all 1745 tests passnpm --prefix web run build-- production build succeedsnpm --prefix web run storybook:build-- storybook buildsReview coverage
Pre-reviewed by 4 agents (frontend-reviewer, pr-test-analyzer, issue-resolution-verifier, docs-consistency). 15 findings addressed:
Closes #787
🤖 Generated with Claude Code