Skip to content

✨ Expand thread-list rows to show per-message summaries#757

Open
nicolasaunai wants to merge 15 commits into
suitenumerique:mainfrom
nicolasaunai:feature/thread-dropdown-expand
Open

✨ Expand thread-list rows to show per-message summaries#757
nicolasaunai wants to merge 15 commits into
suitenumerique:mainfrom
nicolasaunai:feature/thread-dropdown-expand

Conversation

@nicolasaunai

@nicolasaunai nicolasaunai commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

alternative to #751

Summary

  • Adds a chevron on thread-list rows (shown when a thread has more than one message) that expands to indented per-message summaries — sender, date, text snippet, read state — inspired by Apple Mail. Clicking a summary jumps directly to that message in the thread view, whether it's a different thread (fresh navigation) or the thread already open on the right (reactive hash scroll, no remount needed).
  • Backend: Thread.message_count (reproduced independently from the sibling feature/thread-message-count-badge branch — no dependency between the two, they're alternative UX explorations of the same underlying need), a persisted Message.snippet populated at every message-creation path (inbound MDA + MBOX/PST import, outbound send, autoreply), a lightweight MessageSummarySerializer, and a ?summary=true opt-in on the existing messages list endpoint (default behavior unchanged).
  • Frontend: expand/collapse state at the thread-list level (in-memory only), an accessible chevron built as a DOM sibling of the thread row's link (not nested — interactive content can't nest inside an anchor), a hand-written useMessageSummaries hook (the generated API client is typed for the full message shape and would mistype this lighter response), and a fix making the existing hash-based deep-link scroll/highlight mechanism reactive to hash changes, not just to mount.

How it fits together

classDiagram
    class ThreadPanel {
        -expandedThreadIds: Set~string~
        +toggleThreadExpand(threadId)
    }
    class ThreadItem {
        +isExpanded: bool
        +onToggleExpand()
        +thread: Thread
    }
    class ThreadItemMessageSummaries {
        +threadId: string
        +mailboxId: string
    }
    class useMessageSummaries {
        <<hook>>
        +(threadId, mailboxId, enabled) MessageSummary[]
    }
    class ThreadView {
        -hash: string (useLocation)
        +scroll/highlight effect
    }
    class ThreadMessage {
        -isFolded: bool
        +unfold-on-hash effect
    }

    ThreadPanel "1" o-- "*" ThreadItem : renders, owns expand state
    ThreadItem "1" *-- "0..1" ThreadItemMessageSummaries : renders when expanded
    ThreadItemMessageSummaries ..> useMessageSummaries : uses
    ThreadItemMessageSummaries ..> ThreadView : Link hash=#thread-message-id
    ThreadView "1" o-- "*" ThreadMessage : renders

    class MessageViewSet {
        +get_serializer_class()
    }
    class MessageSummarySerializer {
        +id
        +sender
        +sent_at
        +is_unread
        +is_draft
        +has_attachments
        +snippet
    }
    class Message {
        +snippet: str
    }
    class Thread {
        +message_count: int
    }

    useMessageSummaries ..> MessageViewSet : GET /messages/?thread_id&mailbox_id&summary=true
    MessageViewSet ..> MessageSummarySerializer : when summary=true
    MessageSummarySerializer ..> Message : reads snippet (no MIME re-parse)
    ThreadItem ..> Thread : message_count > 1 ?
Loading

Sequence: expanding a thread and jumping to a message

sequenceDiagram
    actor User
    participant TI as ThreadItem (chevron)
    participant TP as ThreadPanel (expand state)
    participant Sum as ThreadItemMessageSummaries
    participant Hook as useMessageSummaries
    participant API as GET /messages/?summary=true
    participant TV as ThreadView (right pane)
    participant TM as ThreadMessage

    User->>TI: click chevron
    TI->>TP: onToggleExpand(threadId)
    TP-->>TI: isExpanded = true
    TI->>Sum: mount
    Sum->>Hook: useMessageSummaries(threadId, mailboxId, {enabled:true})
    Hook->>API: fetch (thread_id, mailbox_id, summary=true)
    API-->>Hook: [{id, sender, sent_at, snippet, is_unread, is_draft, has_attachments}, ...]
    Hook-->>Sum: rows
    Sum-->>User: renders indented summary rows

    alt clicked thread is a DIFFERENT thread than the one open
        User->>Sum: click summary row (hash=#thread-message-X)
        Sum->>TV: navigate (fresh mount)
        TV->>TV: effect [isReady, hash] runs on mount, hashMatch found
        TV->>TM: scroll + highlight target message
    else clicked thread is ALREADY open on the right
        User->>Sum: click summary row (hash=#thread-message-Y)
        Sum->>TV: navigate (same route, hash only changes)
        Note over TV: no remount, but hash is read reactively via useLocation
        TV->>TV: effect re-runs on hash change (guard: hashMatch OR !hasBeenInitialized)
        TV->>TM: scroll + highlight new target
        TM->>TM: fold effect also re-runs on hash change, unfolds target message
    end
Loading

Test plan

  • Backend: bin/pytest core/tests/api/test_threads_list.py core/tests/mda/ core/tests/api/test_messages_list.py core/tests/api/test_message_summary_serializer.py — all passing, plus full make test-back-parallel (3092 passed; 9 pre-existing failures confirmed present on main too, unrelated Django-admin-staticfiles-manifest issue)
  • Frontend: npm run test (465 passed; 13 pre-existing failures matching this environment's documented localStorage-in-jsdom baseline)
  • make lint-back (ruff + pylint 10.00/10), npm run lint, npm run ts:check — all clean, no new warnings vs. baseline
  • Manual click-through in a browser (chevron expand, click a summary in a different thread, click a summary in the already-open thread) — not yet done, flagging as a follow-up before merge

Known limitations (intentionally out of scope for this PR)

  • Clicking a draft summary row navigates/scrolls like any other row rather than opening the composer.
  • The indentation/connector SCSS is a functional starting point, not a reviewed visual design.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Expand threads with multiple messages to view sender, date, read status, drafts, attachments, and message snippets.
    • Select a message summary to jump directly to that message.
    • Thread rows now display the total number of non-draft messages.
    • Message snippets are available for inbound, outbound, and automatic reply messages.
  • Bug Fixes

    • Deep links now reliably scroll to and expand the targeted message, including when the URL hash changes.

Needed by the thread-list dropdown chevron to decide whether a thread has
more than one message worth expanding. Counts non-draft messages, distinct
to avoid JOIN multiplication from multiple ThreadAccess rows.

Signed-off-by: Nicolas Aunai <[email protected]>
Per-message text preview, persisted at write time (see follow-up commits
for the 3 creation sites) rather than computed on read — Message.get_parsed_data()
hits object storage and re-parses MIME on every call, which would be too
expensive to run once per message every time a thread's summary list is
requested.

Signed-off-by: Nicolas Aunai <[email protected]>
Covers inbound MDA delivery and MBOX/PST import, which both route through
_create_message_from_inbound. Reuses the same thread_snippet() call already
made for Thread.snippet, so no extra parsing cost.

Signed-off-by: Nicolas Aunai <[email protected]>
Computed from the already-in-memory composed body (or the caller-supplied
raw MIME's parsed body), never by re-fetching the just-created blob. Also
resolves the pre-existing "set the thread snippet?" TODO at the message
level (the thread-level snippet gap is untouched, out of scope here).

Signed-off-by: Nicolas Aunai <[email protected]>
send_autoreply_for_message already gets message.snippet set in memory via
compose_and_sign_mime; it just wasn't in the save()'s update_fields.

Signed-off-by: Nicolas Aunai <[email protected]>
Review finding: the deterministic snippet value was available from the
fixture's text_body and should have been asserted directly rather than
loosely checking != "".

Signed-off-by: Nicolas Aunai <[email protected]>
Lightweight per-message fields for the thread-list expand dropdown —
id/sender/sent_at/is_unread/has_attachments/snippet only, no body parsing.

Signed-off-by: Nicolas Aunai <[email protected]>
Switches to MessageSummarySerializer for the thread-list expand dropdown.
Default behavior (full MessageSerializer) is unchanged.

Signed-off-by: Nicolas Aunai <[email protected]>
Chevron and thread Link are siblings, not nested (interactive content
cannot nest inside an anchor — same constraint mailbox-list's FolderItem
already follows for its own disclosure chevron). Expansion state lives in
ThreadPanel as a Set<threadId>, in-memory only (resets on reload, survives
SPA navigation).

Signed-off-by: Nicolas Aunai <[email protected]>
Hand-written against the ?summary=true endpoint rather than the generated
useMessagesList, which is typed for the full Message shape and would
mistype this lightweight response.

Signed-off-by: Nicolas Aunai <[email protected]>
Indented, Apple-Mail-style sub-rows under an expanded thread: sender, date,
snippet, unread weight, attachment icon. Non-blocking inline error + retry
on fetch failure.

Signed-off-by: Nicolas Aunai <[email protected]>
…mount

Clicking a different message's summary row in the thread list, for a
thread already open on the right, only changes the URL hash — no remount.
Both the scroll/highlight effect and the per-message unfold effect were
gated to run once per mount, so this case was silently ignored. Keep the
once-only behavior for the draft/unread/latest fallback heuristics, but
let a hash match re-trigger every time.

Signed-off-by: Nicolas Aunai <[email protected]>
…e once-only regression test

Review finding: the previous version of this test asserted the absence of
thread-view__highlight, but that class is never applied on the no-hash
fallback path regardless of whether the once-only guard actually holds —
the test would pass even if the guard were removed. Assert on scrollTo
call count instead, which is the real observable side effect the guard
protects.

Signed-off-by: Nicolas Aunai <[email protected]>
ruff flagged a useless attribute access in the MessageSummarySerializer
blob-storage guard test (now asserts the returned snippet instead of
discarding the result) and reformatted a long set literal in the
messages-list summary test.

Signed-off-by: Nicolas Aunai <[email protected]>
…mmary row

The summary row's Link omitted `search`, silently dropping the active
thread-list filter (e.g. "unread only") on click — the main thread row's
Link already preserves it via search={true}. Also adds the missing
CHANGELOG entry for this feature.

Found during the final whole-branch review.

Signed-off-by: Nicolas Aunai <[email protected]>
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds persisted message snippets, lightweight message-summary and non-draft message-count API data, expandable thread-list summaries, and reactive hash-based message unfolding, scrolling, and highlighting.

Changes

Thread summary data flow

Layer / File(s) Summary
Persist message snippets
src/backend/core/models.py, src/backend/core/migrations/..., src/backend/core/mda/*, src/backend/core/tests/mda/*
Messages gain a stored snippet; inbound, outbound, and autoreply flows populate and persist it.
Expose summary API data
src/backend/core/api/*, src/backend/core/tests/api/*
Thread responses expose non-draft message_count, while summary=true returns lightweight message fields without blob parsing.
Add expandable thread summaries
src/frontend/src/features/layouts/components/thread-panel/..., CHANGELOG.md
Thread rows gain disclosure controls, fetched message summaries, loading/error states, direct message links, styling, and coverage.
React to message hash links
src/frontend/src/features/layouts/components/thread-view/...
Message unfolding and thread scrolling/highlighting respond to router hash changes, with initial and subsequent hash behavior tested.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant ThreadPanel
  participant ThreadItem
  participant MessageAPI
  participant ThreadView
  User->>ThreadItem: Click disclosure chevron
  ThreadItem->>MessageAPI: Request summary=true
  MessageAPI-->>ThreadItem: Return message summaries
  ThreadItem-->>User: Show linked summary rows
  User->>ThreadItem: Click message summary
  ThreadItem->>ThreadView: Navigate with message hash
  ThreadView-->>User: Unfold, scroll, and highlight target message
Loading

Possibly related PRs

Suggested reviewers: jbpenrath

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 96.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: expanding thread-list rows to show per-message summaries.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/backend/core/mda/inbound_create.py`:
- Around line 476-479: Update the thread_snippet call in the inbound create flow
to use an empty-string fallback instead of the English "(No snippet available)"
text, matching the outbound.py behavior and leaving localized empty-snippet
presentation to the frontend.

In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx`:
- Around line 147-189: Update the chevron button in the thread item row to set
tabIndex={-1}, keeping it programmatically focusable without entering the
document tab order managed by the roving role="option" tabindex. Preserve the
existing expand/collapse behavior and ARIA attributes.
- Around line 345-352: Update the mailboxId prop in the
ThreadItemMessageSummaries usage within the expanded thread summary block to
fall back to the thread’s first access mailbox ID when params?.mailboxId is
absent, rather than an empty string. Preserve the URL mailbox ID when it is
available.

In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 280-311: Track all timers and event-listener callbacks created
during each hash-driven effect run, including the scrollend safety timer and
highlight cleanup timer, and return an effect cleanup that clears the timers and
removes the scrollend and animationend listeners. Update the relevant
scheduleHighlight/startHighlight flow so stale runs cannot invoke startHighlight
after a subsequent hash change, while preserving the existing highlighting
behavior for the active run.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 159fdd6c-7980-4139-8ebe-afba8a4399f9

📥 Commits

Reviewing files that changed from the base of the PR and between f058ee4 and 06bf1d0.

⛔ Files ignored due to path filters (1)
  • src/frontend/src/features/api/gen/models/thread.ts is excluded by !**/gen/**
📒 Files selected for processing (29)
  • CHANGELOG.md
  • src/backend/core/api/openapi.json
  • src/backend/core/api/serializers.py
  • src/backend/core/api/viewsets/message.py
  • src/backend/core/api/viewsets/thread.py
  • src/backend/core/mda/autoreply.py
  • src/backend/core/mda/inbound_create.py
  • src/backend/core/mda/outbound.py
  • src/backend/core/migrations/0033_message_snippet.py
  • src/backend/core/models.py
  • src/backend/core/tests/api/test_message_summary_serializer.py
  • src/backend/core/tests/api/test_messages_list.py
  • src/backend/core/tests/api/test_threads_list.py
  • src/backend/core/tests/mda/test_autoreply.py
  • src/backend/core/tests/mda/test_inbound.py
  • src/backend/core/tests/mda/test_outbound.py
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/_index.scss
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.test.tsx
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/thread-item-message-summaries.test.tsx
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/thread-item-message-summaries.tsx
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/types.ts
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/use-message-summaries.test.tsx
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/use-message-summaries.ts
  • src/frontend/src/features/layouts/components/thread-panel/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.test.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/index.test.tsx
  • src/frontend/src/features/layouts/components/thread-view/index.tsx

Comment on lines +476 to +479
snippet=thread_snippet(
parsed_email,
fallback=subject or "(No snippet available)",
),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Avoid storing English UI text in the database.

Using "(No snippet available)" bakes localized UI text directly into the database, making it impossible for the frontend to translate this fallback state for non-English users. It's also inconsistent with outbound.py, which correctly falls back to "".

Consider falling back to an empty string "" and letting the frontend display the appropriate localized text when a snippet is missing.

♻️ Proposed fix
                     snippet=thread_snippet(
                         parsed_email,
-                        fallback=subject or "(No snippet available)",
+                        fallback=subject or "",
                     ),
📝 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.

Suggested change
snippet=thread_snippet(
parsed_email,
fallback=subject or "(No snippet available)",
),
snippet=thread_snippet(
parsed_email,
fallback=subject or "",
),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/core/mda/inbound_create.py` around lines 476 - 479, Update the
thread_snippet call in the inbound create flow to use an empty-string fallback
instead of the English "(No snippet available)" text, matching the outbound.py
behavior and leaving localized empty-snippet presentation to the frontend.

Comment on lines +147 to +189
<div className="thread-item-row">
{thread.message_count > 1 && (
// Sibling of the Link below, not nested inside it: interactive
// content cannot be nested inside an <a> (same constraint
// mailbox-list's FolderItem already follows for its own
// disclosure chevron).
<button
type="button"
className={clsx("thread-item__chevron", {
"thread-item__chevron--collapsed": !isExpanded,
})}
onClick={onToggleExpand}
aria-expanded={isExpanded}
aria-controls={`thread-item-summaries-${thread.id}`}
aria-label={isExpanded ? t('Collapse messages') : t('Expand messages')}
>
<Icon name="expand_more" type={IconType.OUTLINED} aria-hidden="true" />
</button>
)}
data-unread={hasUnread}
draggable
onDragStart={handleDragStart}
onDragEnd={handleDragEnd}
onClick={handleItemClick}
onFocus={onFocusItem}
tabIndex={tabIndex}
ref={itemRef}
role="option"
aria-selected={isSelected}
>
<div>
<Link
to="/mailbox/$mailboxId/thread/$threadId"
params={{ mailboxId: params?.mailboxId ?? '', threadId: thread.id }}
search={true}
className={clsx(
'thread-item',
{
'thread-item--active': thread.id === params?.threadId,
'thread-item--dragging': isDragging,
'thread-item--selected': isSelected,
},
)}
data-unread={hasUnread}
draggable
onDragStart={handleDragStart}
onDragEnd={handleDragEnd}
onClick={handleItemClick}
onFocus={onFocusItem}
tabIndex={tabIndex}
ref={itemRef}
role="option"
aria-selected={isSelected}
>
<div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Fix keyboard tab trap and ARIA listbox semantics.

The disclosure chevron <button> lacks a tabIndex, so it defaults to 0 and enters the document's tab order. Because the parent container manages a roving tabindex for the listbox (only the active option has tabIndex={0}), pressing Tab from an active thread will move focus to the next chevron instead of leaving the listbox. This traps keyboard users in the list.

Additionally, rendering an interactive <button> sibling and an expandable <ul> block directly inside a role="listbox" violates the ARIA specification, which expects only option or group elements as direct children in the accessibility tree.

To fix the immediate keyboard trap, add tabIndex={-1} to the chevron. To ensure it remains operable by keyboard in the future, you will need to handle expansion via keyboard events (e.g., Right/Left arrows) on the focused role="option" element, or consider refactoring the list to a role="tree" or role="treegrid" which natively support expandable nodes and interactive children.

🐛 Proposed fix for the tab trap
                     <button
                         type="button"
+                        tabIndex={-1}
                         className={clsx("thread-item__chevron", {
                             "thread-item__chevron--collapsed": !isExpanded,
📝 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.

Suggested change
<div className="thread-item-row">
{thread.message_count > 1 && (
// Sibling of the Link below, not nested inside it: interactive
// content cannot be nested inside an <a> (same constraint
// mailbox-list's FolderItem already follows for its own
// disclosure chevron).
<button
type="button"
className={clsx("thread-item__chevron", {
"thread-item__chevron--collapsed": !isExpanded,
})}
onClick={onToggleExpand}
aria-expanded={isExpanded}
aria-controls={`thread-item-summaries-${thread.id}`}
aria-label={isExpanded ? t('Collapse messages') : t('Expand messages')}
>
<Icon name="expand_more" type={IconType.OUTLINED} aria-hidden="true" />
</button>
)}
data-unread={hasUnread}
draggable
onDragStart={handleDragStart}
onDragEnd={handleDragEnd}
onClick={handleItemClick}
onFocus={onFocusItem}
tabIndex={tabIndex}
ref={itemRef}
role="option"
aria-selected={isSelected}
>
<div>
<Link
to="/mailbox/$mailboxId/thread/$threadId"
params={{ mailboxId: params?.mailboxId ?? '', threadId: thread.id }}
search={true}
className={clsx(
'thread-item',
{
'thread-item--active': thread.id === params?.threadId,
'thread-item--dragging': isDragging,
'thread-item--selected': isSelected,
},
)}
data-unread={hasUnread}
draggable
onDragStart={handleDragStart}
onDragEnd={handleDragEnd}
onClick={handleItemClick}
onFocus={onFocusItem}
tabIndex={tabIndex}
ref={itemRef}
role="option"
aria-selected={isSelected}
>
<div>
<div className="thread-item-row">
{thread.message_count > 1 && (
// Sibling of the Link below, not nested inside it: interactive
// content cannot be nested inside an <a> (same constraint
// mailbox-list's FolderItem already follows for its own
// disclosure chevron).
<button
type="button"
tabIndex={-1}
className={clsx("thread-item__chevron", {
"thread-item__chevron--collapsed": !isExpanded,
})}
onClick={onToggleExpand}
aria-expanded={isExpanded}
aria-controls={`thread-item-summaries-${thread.id}`}
aria-label={isExpanded ? t('Collapse messages') : t('Expand messages')}
>
<Icon name="expand_more" type={IconType.OUTLINED} aria-hidden="true" />
</button>
)}
<Link
to="/mailbox/$mailboxId/thread/$threadId"
params={{ mailboxId: params?.mailboxId ?? '', threadId: thread.id }}
search={true}
className={clsx(
'thread-item',
{
'thread-item--active': thread.id === params?.threadId,
'thread-item--dragging': isDragging,
'thread-item--selected': isSelected,
},
)}
data-unread={hasUnread}
draggable
onDragStart={handleDragStart}
onDragEnd={handleDragEnd}
onClick={handleItemClick}
onFocus={onFocusItem}
tabIndex={tabIndex}
ref={itemRef}
role="option"
aria-selected={isSelected}
>
<div>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx`
around lines 147 - 189, Update the chevron button in the thread item row to set
tabIndex={-1}, keeping it programmatically focusable without entering the
document tab order managed by the roving role="option" tabindex. Preserve the
existing expand/collapse behavior and ARIA attributes.

Comment on lines +345 to +352
{isExpanded && (
<div id={`thread-item-summaries-${thread.id}`}>
<ThreadItemMessageSummaries
threadId={thread.id}
mailboxId={params?.mailboxId ?? ''}
/>
</div>
)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Provide a valid fallback for missing mailboxId.

In views where params.mailboxId is undefined (like a cross-mailbox search or unified inbox), the prop falls back to an empty string "". As documented in useMessageSummaries, the backend requires a valid mailbox_id to correctly annotate the is_unread state; omitting it or passing an empty string silently marks all summaries as read.

Consider falling back to the thread's first access mailbox ID when the URL parameter is absent.

🐛 Proposed fix to resolve the backend requirement
                 {isExpanded && (
                     <div id={`thread-item-summaries-${thread.id}`}>
                         <ThreadItemMessageSummaries
                             threadId={thread.id}
-                            mailboxId={params?.mailboxId ?? ''}
+                            mailboxId={params?.mailboxId ?? thread.accesses[0]?.mailbox.id ?? ''}
                         />
                     </div>
                 )}
📝 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.

Suggested change
{isExpanded && (
<div id={`thread-item-summaries-${thread.id}`}>
<ThreadItemMessageSummaries
threadId={thread.id}
mailboxId={params?.mailboxId ?? ''}
/>
</div>
)}
{isExpanded && (
<div id={`thread-item-summaries-${thread.id}`}>
<ThreadItemMessageSummaries
threadId={thread.id}
mailboxId={params?.mailboxId ?? thread.accesses[0]?.mailbox.id ?? ''}
/>
</div>
)}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx`
around lines 345 - 352, Update the mailboxId prop in the
ThreadItemMessageSummaries usage within the expanded thread summary block to
fall back to the thread’s first access mailbox ID when params?.mailboxId is
absent, rather than an empty string. Preserve the URL mailbox ID when it is
available.

Comment on lines +280 to 311
// Defer the highlight until the smooth scroll has actually
// landed. `scrollend` doesn't emit when no scroll happens
// nor on older browsers, so the 700ms safety acts as a fallback.
const scheduleHighlight = (willScroll: boolean) => {
const root = rootRef.current;
if (!root || scrollBehavior !== 'smooth' || !willScroll) {
startHighlight();
return;
}
let fired = false;
const fire = () => {
if (fired) return;
fired = true;
root.removeEventListener('scrollend', fire);
clearTimeout(safety);
startHighlight();
};
root.addEventListener('scrollend', fire);
const safety = setTimeout(fire, 700);
};

if (hashMatch) {
requestAnimationFrame(() => requestAnimationFrame(() => {
const willScroll = performScroll();
scheduleHighlight(willScroll);
}));
} else {
if (hashMatch) {
requestAnimationFrame(() => requestAnimationFrame(() => {
const willScroll = performScroll();
scheduleHighlight(willScroll);
}
setHasBeenInitialized(true);
}));
} else {
const willScroll = performScroll();
scheduleHighlight(willScroll);
}
setHasBeenInitialized(true);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

No cleanup for pending scroll/highlight timers on hash re-run.

Since the effect now re-runs on every hash change (rather than once on mount), a rapid hash switch can leave a prior run's scrollend listener and setTimeout(fire, 700) / setTimeout(cleanup, 2200) pending. When they fire, startHighlight() re-queries and highlights the previous target, so two messages can flash the highlight class briefly before self-recovering.

Consider returning a cleanup from the effect that clears the outstanding timeouts and removes the scrollend/animationend listeners registered during that run.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/frontend/src/features/layouts/components/thread-view/index.tsx` around
lines 280 - 311, Track all timers and event-listener callbacks created during
each hash-driven effect run, including the scrollend safety timer and highlight
cleanup timer, and return an effect cleanup that clears the timers and removes
the scrollend and animationend listeners. Update the relevant
scheduleHighlight/startHighlight flow so stale runs cannot invoke startHighlight
after a subsequent hash change, while preserving the existing highlighting
behavior for the active run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant