Front/fold message quotes #382
Conversation
WalkthroughRefactors ThreadView into a provider-driven component with per-message readiness; adds UnquoteMessage utility and tests; integrates it into MessageBody; updates mailbox invalidation API and callers; UI/SCSS tweaks (LabelBadge, ContactChip, thread message layout), attachment key change, locales, and Makefile test forwarding. Changes
Sequence Diagram(s)sequenceDiagram
participant TVProv as ThreadViewProvider
participant TV as ThreadViewComponent
participant TM as ThreadMessage
participant MB as MessageBody
participant UQ as UnquoteMessage
participant CID as CID→BlobMap
TV->>TVProv: mount(messagesIds)
TV->>TM: render messages
TM->>TVProv: useThreadViewContext() (isMessageReady / setMessageReadiness)
TM->>MB: render(rawHtml, rawText, attachments, isHidden, onLoad)
alt visible & ready
MB->>CID: lazily map CIDs to blob URLs
MB->>UQ: UnquoteMessage(sanitizedHtml/text, options)
UQ-->>MB: return unquoted content
MB->>TVProv: onLoad → setMessageReadiness(id, true)
else hidden or not ready
MB-->>TM: render collapsed placeholder / hidden iframe
end
Note over TVProv: Actions may call Mailbox.invalidateThreadMessages(source?)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jbpenrath
force-pushed
the
front/fold-message-quotes
branch
8 times, most recently
from
ff161b1 to
b74b7ff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
57-81: Fix hook lifecycle, remove dead onload/postMessage, and add defensive sanitization passThree issues prevent safe and reliable message rendering:
DomPurify.addHook accumulates on every render — move the hook registration to a
useEffectwith cleanup viaremoveHookto prevent duplicate hooks and memory leaks.Inline
onloadattribute is blocked and dead — the CSPscript-src 'none'directive blocks all inline event handlers. Remove theonload="window.parent.postMessage(...)"and rely on the ReactonLoadcallback (handleIframeLoad) which already handles iframe setup and toggle listeners.Plain-text path skips final sanitization — when
rawHtmlBodyis absent,getText().contentreturns plain text directly without a sanitization pass. Add a finalDomPurify.sanitize()call afterUnquoteMessageprocesses both paths to ensure consistency and catch any edge cases in text conversion.Additionally, confirm that
DomPurify.removeHookexists at runtime (older versions may not export it); use a try/catch or type guard if needed. Also applies to lines 83–97, 232–235, 246–263, 269–274.
🧹 Nitpick comments (4)
src/frontend/src/features/ui/components/label-badge/_index.scss (1)
11-13: Consider removing redundant padding reset.The
:has(a.label-badge__label)selector resetspadding: 0, but the root.label-badgealready haspadding: 0set on line 2. Unless this serves a specificity override purpose that isn't apparent from the current context, this rule appears redundant.Apply this diff to remove the redundant rule:
- &:has(a.label-badge__label) { - padding: 0; - } -Makefile (1)
246-253: Argument forwarding via MAKECMDGOALS is brittle; prefer explicit ARGSMAKECMDGOALS loses quoting and cannot pass values with spaces. Provide an ARGS var while keeping current behavior for convenience.
-front-test: ## run the frontend tests - @args="$(filter-out $@,$(MAKECMDGOALS))" && \ - $(COMPOSE) run --rm frontend-tools npm run test -- $${args:-${1}} +front-test: ## run the frontend tests (use: make front-test ARGS="-- <jest args>") + @args="$(filter-out $@,$(MAKECMDGOALS))" ; \ + test_args="$${ARGS:-$$args}" ; \ + $(COMPOSE) run --rm frontend-tools npm run test -- $${test_args} .PHONY: front-test -front-test-amd64: ## run the frontend tests in amd64 - @args="$(filter-out $@,$(MAKECMDGOALS))" && \ - $(COMPOSE) run --rm frontend-tools-amd64 npm run test -- $${args:-${1}} +front-test-amd64: ## run the frontend tests in amd64 (use: make front-test-amd64 ARGS="-- <jest args>") + @args="$(filter-out $@,$(MAKECMDGOALS))" ; \ + test_args="$${ARGS:-$$args}" ; \ + $(COMPOSE) run --rm frontend-tools-amd64 npm run test -- $${test_args} .PHONY: front-test-amd64Run: make front-test ARGS="-- -t unquote-message" and ensure it filters correctly.
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
42-54: Minor typos in test names; add CID image case
- Spelling: “Freskdesk” → “Freshdesk”, “shoudl” → “should”. Keeps test output clean.
- Consider a test for IMG cid: transformation to ensure the DomPurify hook path works end-to-end.
Example test skeleton:
it("rewrites cid: image sources to blob URLs", () => { // compose html with <img src="cid:abc123"> and provide attachments map // assert that resulting HTML contains src equal to the computed blob URL });Also applies to: 236-252
src/frontend/src/features/utils/unquote-message/index.ts (1)
133-133: Dead code: unreachable break condition at line 133.The condition
if (quotedElements.size > 0) break;will never execute becausequotedElementsis empty at this point in the handler loop. Elements are only added toquotedElementsafter the loop completes (lines 156-162) during depth filtering. The test "should handle multiple quote markers" confirms the loop correctly processes all handlers without breaking early.Remove this unreachable break condition or clarify its intended purpose if this was meant to check
elementsToHandle.size > 0instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Makefile(1 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx(6 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(4 hunks)src/frontend/src/features/ui/components/label-badge/_index.scss(2 hunks)src/frontend/src/features/ui/components/label-badge/index.tsx(4 hunks)src/frontend/src/features/utils/unquote-message/constants.ts(1 hunks)src/frontend/src/features/utils/unquote-message/handlers.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.test.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.ts(1 hunks)src/frontend/src/features/utils/unquote-message/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx (2)
src/backend/core/tests/api/test_attachments.py (1)
attachment(169-173)src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx (1)
AttachmentItem(27-97)
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (2)
html(242-247)UnquoteMessage(34-385)
src/frontend/src/features/ui/components/label-badge/index.tsx (3)
src/frontend/src/features/api/gen/models/thread_label.ts (1)
ThreadLabel(12-38)src/frontend/src/features/api/gen/labels/labels.ts (2)
useLabelsRemoveThreadsCreate(815-838)useLabelsAddThreadsCreate(707-727)src/frontend/src/features/ui/components/badge/index.tsx (1)
Badge(7-13)
src/frontend/src/features/utils/unquote-message/types.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (3)
HTMLElement(277-289)UnquoteResult(97-172)UnquoteResult(177-225)
src/frontend/src/features/utils/unquote-message/handlers.ts (1)
src/frontend/src/features/utils/unquote-message/types.ts (1)
CustomHandler(9-19)
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
src/frontend/src/features/ui/components/label-badge/index.tsx (1)
LabelBadge(23-85)
src/frontend/src/features/utils/unquote-message/index.ts (3)
src/frontend/src/features/utils/unquote-message/types.ts (2)
UnquoteOptions(26-48)UnquoteResult(55-62)src/frontend/src/features/utils/unquote-message/handlers.ts (1)
HANDLERS(12-299)src/frontend/src/features/utils/unquote-message/constants.ts (2)
REPLY_PATTERNS(41-146)FORWARD_PATTERNS(5-34)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
src/frontend/src/features/utils/unquote-message/index.ts (1)
UnquoteMessage(34-385)
src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
ThreadSummary(19-148)src/frontend/src/features/layouts/components/thread-view/components/thread-view-labels-list/index.tsx (1)
ThreadViewLabelsList(11-17)
🪛 ast-grep (0.39.6)
src/frontend/src/features/utils/unquote-message/index.ts
[warning] 281-281: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 281-281: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ 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). (5)
- GitHub Check: docker-publish-mta-in / docker-build-push
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: check-api-state
- GitHub Check: lint-back
- GitHub Check: build-front
🔇 Additional comments (20)
src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss (1)
8-8: LGTM! Syntax error corrected.The extra semicolon has been properly removed.
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
88-88: LGTM! Compact mode correctly applied.The
compactprop is properly passed toLabelBadge, enabling the compact variant for thread list labels.src/frontend/src/features/ui/components/label-badge/_index.scss (2)
2-9: LGTM! Padding restructuring supports new component architecture.Moving padding from the root
.label-badgeto the nested.label-badge__labelelement correctly accommodates the new DOM structure where label content is wrapped in a dedicated element.
36-40: LGTM! Compact modifier correctly implemented.The compact variant appropriately reduces padding while maintaining the same structural approach.
src/frontend/src/features/ui/components/label-badge/index.tsx (3)
20-20: LGTM! Compact prop properly introduced.The
compactprop is correctly typed, has an appropriate default value offalse, and maintains backward compatibility with existing usage.Also applies to: 23-23
68-69: LGTM! Structural changes align with styling updates.The conditional
label-badge--compactclass viaclsxand the new.label-badge__labelwrapper correctly implement the compact variant and align with the corresponding SCSS refactoring.
78-78: LGTM! Consistent icon usage.Replacing the Material Icons string with the
Iconcomponent improves consistency with the ui-kit design system.src/frontend/src/features/layouts/components/thread-view/_index.scss (1)
59-62: Header spacing addition looks goodNew .thread-view__header padding aligns with the messages list padding and fixes visual imbalance. No issues.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss (1)
90-90: Spacing refactor LGTMReplacing margins with CSS variables and using flex+gap in footer improves consistency and responsiveness. No functional concerns.
Also applies to: 96-99
src/frontend/src/features/utils/unquote-message/handlers.ts (1)
12-299: Handlers coverage looks solidSelector ordering and detect logic cover major clients (Gmail, Outlook variants, Apple, Thunderbird, Yahoo, Zoho, etc.). No blocking issues spotted.
Please confirm :has(...) selectors are supported in all target environments where unquoting runs (jsdom tests, browser engines you support). If needed, provide a fallback path for older engines.
src/frontend/src/features/utils/unquote-message/constants.ts (1)
5-34: The original review comment is based on incorrect assumptions about the codebase.The code already properly manages regex state:
- Line 192 explicitly resets
pattern.lastIndex = 0after each.exec()call, which is the exact pattern the review suggested as a solution.- FORWARD_PATTERNS usage with
.match()at line 235 inherently avoids lastIndex state issues, so no changes are needed there.Both call sites demonstrate correct understanding of regex statefulness. The patterns are not carrying problematic state between invocations.
Likely an incorrect or invalid review comment.
src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
43-43: LGTM! Spacing improvements enhance readability.The consistent spacing around operators and conditions aligns with standard formatting practices.
Also applies to: 62-62, 94-94
128-144: Excellent semantic HTML restructuring!Wrapping the subject and summary in a
<header>element improves document structure and accessibility. The fallback text for missing subjects enhances UX.src/frontend/src/features/utils/unquote-message/index.ts (7)
45-91: LGTM! Clean public API with proper defaults.The constructor properly initializes defaults (mode: "remove", ignoreFirstForward: false, depth: Infinity), and the public methods handle empty input gracefully with early returns.
177-225: LGTM! Robust pattern matching with proper regex state management.The method correctly finds the earliest quote marker across multiple patterns and properly resets regex state with
pattern.lastIndex = 0. The additional handling of ">" prefixed lines by scanning backwards is a nice touch for plain text emails.
230-237: LGTM! Forward content detection logic is straightforward.The method correctly uses pattern matching to detect forwarded messages.
277-289: Safe use of innerHTML with literal content.The static analysis warning about
innerHTMLon line 282 is a false positive. The assigned value"<span>…</span>"is a hardcoded literal string with no user input, making it safe from XSS vulnerabilities.
295-310: LGTM! Efficient depth calculation with early termination.The optimization at line 302 to stop traversing when
depth <= this.#options.depthprevents unnecessary DOM walking for deeply nested structures.
315-362: Wrapping logic is complex - verify adjacency detection.The grouping algorithm (lines 322-345) groups consecutive elements based on adjacency. The adjacency check at lines 328-330 uses:
prevElement.nextElementSibling === element || prevElement.parentElement === element.parentElementThe second condition (
prevElement.parentElement === element.parentElement) will group all siblings together, even if they're not consecutive. This might be intentional for wrapping all quoted elements under the same parent, but could also group non-consecutive elements.Consider whether non-consecutive siblings should be grouped together, or if the adjacency check should be stricter to only group truly consecutive elements.
370-388: LGTM! Convenient static helpers and default export.The static methods
unquoteHtmlandunquoteTextprovide a convenient API for one-off operations without instantiating the class. The default export enhances usability.
...tend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
57-81: Move DomPurify.addHook into useEffect with cleanup to prevent hook accumulation on every render.The hook is currently added on every render with no cleanup, causing memory leaks and double-processing. Since
cidToBlobUrlMapchanges when attachments change, the hook also captures a stale reference.DomPurify 3.2.6 supports
removeHook(), so the suggested fix is viable:+ useEffect(() => { + const hook = function (node: Element) { // Allow anchor tags to be opened in the parent window if the href is an anchor // Other links are opened in a new tab and safe rel attributes is set if(node.tagName === 'A') { if (!node.getAttribute('href')?.startsWith('#')) { node.setAttribute('target', '_blank'); } node.setAttribute('rel', 'noopener noreferrer'); } // Transform CID references in img src attributes if (node.tagName === 'IMG' && cidToBlobUrlMap.size > 0) { const src = node.getAttribute('src'); if (src && src.startsWith('cid:')) { const cid = src.substring(4); // Remove 'cid:' prefix const blobUrl = cidToBlobUrlMap.get(cid); if (blobUrl) { node.setAttribute('src', blobUrl); } } } - DomPurify.addHook( - 'afterSanitizeAttributes', - function (node) { - // ... - } - ); + }; + DomPurify.addHook('afterSanitizeAttributes', hook); + return () => DomPurify.removeHook('afterSanitizeAttributes'); + }, [cidToBlobUrlMap]);Ensure the hook dependency array includes
cidToBlobUrlMapso the hook updates when CIDs change.
🧹 Nitpick comments (12)
src/frontend/src/features/ui/components/label-badge/_index.scss (1)
15-18: Prefer logical properties for consistency.Line 16 uses the physical property
padding-right, while line 17 uses the logical propertypadding-block. For RTL support and consistency with the rest of the codebase, consider usingpadding-inline-endinstead.Apply this diff:
&:has(.label-badge__remove-cta) { - padding-right: 0; + padding-inline-end: 0; padding-block: 0; }src/frontend/src/features/utils/unquote-message/handlers.ts (3)
70-76: Avoid :has() for broader engine/jsdom supportThe selector uses :has(), which is still spotty in some selector engines (e.g., older Safari, some test environments like jsdom). Since detect already validates adjacency, match blockquotes directly and verify the preceding sibling in detect to keep behavior while improving compatibility.
- selector: 'div:has(> blockquote[type="cite"]) + blockquote[type="cite"]', - detect: (element) => { - const previousSibling = element.previousElementSibling as HTMLElement; - return [previousSibling, element]; - } + selector: 'blockquote[type="cite"]', + detect: (element) => { + const prev = element.previousElementSibling as HTMLElement | null; + if (prev && prev.querySelector && prev.querySelector('blockquote[type="cite"]')) { + return [prev, element]; + } + return []; + }If your CI runs on jsdom, this eliminates failures due to unsupported selectors. Please confirm test matrix engines. [Based on learnings]
90-101: Simplify sibling traversal (current while is a no-op)You start from nextElementSibling, then only enter the while-loop if the node is not an element — which can never happen. Either remove the loop or start from nextSibling and advance with nextSibling. Low-risk cleanup.
- let nextSibling = element.nextElementSibling; - while (nextSibling && nextSibling.nodeType !== Node.ELEMENT_NODE) { - nextSibling = nextSibling.nextSibling as Element | null; - } + let nextSibling = element.nextElementSibling;
147-184: Outlook style check is brittle (string endsWith on style attribute)Comparing a full style string via endsWith is fragile (ordering/spacing). Consider parsing into a CSSStyleDeclaration (create a temp element, setAttribute('style', style), then inspect computed properties) or match individual properties with regex. Keeps false negatives down across clients.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
246-263: Stale postMessage handling (unused branches)The iframe posts a number on load, not 'iframe-loaded' or 'resize'. These branches never run. Consider removing them to reduce noise, since resize is already handled on load and on details.toggle.
src/frontend/src/features/utils/unquote-message/constants.ts (2)
41-146: Regex set is broad; ensure performance and correctness in worst-case inputs
- Many patterns are global/multiline and start with ^; across large texts this can be expensive. Consider short-circuiting by scanning only the first N KB or first M lines.
- A few patterns (e.g., Lines 99-107) use '.' where '-' may be intended between date components; verify intended tolerance vs. accidental overmatch.
111-138: Dash-delimiter RegExp: precompile once (ok), but anchor to line starts consistentlyYou anchor with ^ but also allow optional leading '>' or whitespace; good. Consider adding 'm' consistently for all line-anchored variants for uniform behavior (some already have 'm' via gim). Minor.
src/frontend/src/features/utils/unquote-message/types.ts (1)
26-48: Option to inject summary text/i18n hookUnquoteMessage creates a summary label via i18n.t(...) internally (see index.ts). To avoid global coupling, consider adding an optional summaryText or translate callback in UnquoteOptions so the util stays framework-agnostic and test-friendly.
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
236-252: Typo in test title'shoudl' -> 'should'. Nit, but keeps test output clean.
src/frontend/src/features/utils/unquote-message/index.ts (3)
60-75: HTML path lacks documented “pattern fallback.”getHtml promises a fallback to pattern-based detection but returns handlers-only result. Consider: if no elements found, detect reply markers in text nodes or fall back to plain-text logic on a text-stripped clone.
252-272: Avoid shadowing global ‘document’ identifier.Rename parameter for clarity.
- #cleanupEmptyElements(document: Document): void { - const emptyElements: Element[] = []; - // Find all empty elements - document.querySelectorAll("*").forEach((el) => { + #cleanupEmptyElements(doc: Document): void { + const emptyElements: Element[] = []; + // Find all empty elements + doc.querySelectorAll("*").forEach((el) => { const text = el.textContent?.trim() || ""; const hasChildren = el.children.length > 0; @@ - // Remove empty elements - emptyElements.forEach((el) => el.remove()); + // Remove empty elements + emptyElements.forEach((el) => el.remove()); }
242-247: Optional: detect DOMParser parse errors.ParseFromString returns a Document even on errors (with a parsererror subtree). Consider checking and short-circuiting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Makefile(1 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx(6 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(4 hunks)src/frontend/src/features/ui/components/label-badge/_index.scss(2 hunks)src/frontend/src/features/ui/components/label-badge/index.tsx(4 hunks)src/frontend/src/features/utils/unquote-message/constants.ts(1 hunks)src/frontend/src/features/utils/unquote-message/handlers.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.test.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.ts(1 hunks)src/frontend/src/features/utils/unquote-message/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx (1)
AttachmentItem(27-97)
src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
ThreadSummary(19-148)src/frontend/src/features/layouts/components/thread-view/components/thread-view-labels-list/index.tsx (1)
ThreadViewLabelsList(11-17)
src/frontend/src/features/utils/unquote-message/types.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (3)
HTMLElement(277-289)UnquoteResult(97-172)UnquoteResult(177-225)
src/frontend/src/features/utils/unquote-message/handlers.ts (1)
src/frontend/src/features/utils/unquote-message/types.ts (1)
CustomHandler(9-19)
src/frontend/src/features/utils/unquote-message/index.ts (3)
src/frontend/src/features/utils/unquote-message/types.ts (2)
UnquoteOptions(26-48)UnquoteResult(55-62)src/frontend/src/features/utils/unquote-message/handlers.ts (1)
HANDLERS(12-299)src/frontend/src/features/utils/unquote-message/constants.ts (2)
REPLY_PATTERNS(41-146)FORWARD_PATTERNS(5-34)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
src/frontend/src/features/utils/unquote-message/index.ts (1)
UnquoteMessage(34-385)
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (2)
html(242-247)UnquoteMessage(34-385)
src/frontend/src/features/ui/components/label-badge/index.tsx (3)
src/frontend/src/features/api/gen/models/thread_label.ts (1)
ThreadLabel(12-38)src/frontend/src/features/api/gen/labels/labels.ts (2)
useLabelsRemoveThreadsCreate(815-838)useLabelsAddThreadsCreate(707-727)src/frontend/src/features/ui/components/badge/index.tsx (1)
Badge(7-13)
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
src/frontend/src/features/ui/components/label-badge/index.tsx (1)
LabelBadge(23-85)
🪛 ast-grep (0.39.6)
src/frontend/src/features/utils/unquote-message/index.ts
[warning] 281-281: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 281-281: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🔇 Additional comments (18)
Makefile (1)
245-253: Excellent alignment with existing test target patterns.Both
front-testandfront-test-amd64now support passing arbitrary test arguments via MAKECMDGOALS, matching the pattern used inback-test(line 236) andback-test-parallel(line 241). The implementation correctly filters out the target name and forwards remaining arguments to npm test.Usage examples:
make front-test— runs all tests (default behavior preserved)make front-test src/components/__tests__/button.spec.ts— runs specific test filemake front-test-amd64 --watch— runs amd64 tests in watch modeThe variable escaping (
$$for Make,${...}for shell) and the fallback${1}are correct and maintain backward compatibility.src/frontend/src/features/layouts/components/thread-view/_index.scss (1)
40-40: LGTM! Clean syntax fix and consistent spacing.The trailing semicolon fix and the new header block with theme-based padding values align well with the header restructuring in the TypeScript file.
Also applies to: 58-62
src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss (1)
90-90: LGTM! Improved spacing consistency.Migrating to theme spacing variables and using flexbox with
gapinstead of individual margins improves maintainability and aligns with modern CSS practices.Also applies to: 95-98
src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
43-43: LGTM! Minor formatting improvements.These spacing adjustments improve code readability without affecting functionality.
Also applies to: 62-62, 94-94
128-144: LGTM! Improved semantic structure and UX.The header restructuring provides several benefits:
- Uses semantic HTML with the
<header>element- Adds a user-friendly fallback for threads without subjects
- Logically groups header content (subject, summary, labels)
- Aligns with the new
.thread-view__headerstylessrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
88-88: LGTM!The
compactprop addition correctly integrates the new compact rendering mode for label badges in thread items, improving visual density in the thread list view.src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss (1)
8-8: LGTM!The SCSS syntax error fix removes the extra semicolon, ensuring proper parsing.
src/frontend/src/features/ui/components/label-badge/_index.scss (2)
2-9: LGTM!The refactoring to move padding from the outer badge to the inner
.label-badge__labelelement provides better control for the compact modifier and when the remove CTA is present.
36-40: LGTM!The compact modifier correctly reduces vertical padding to
0.25remwhile maintaining horizontal padding at0.5rem, providing a denser visual presentation for label badges in thread lists.src/frontend/src/features/ui/components/label-badge/index.tsx (3)
6-6: LGTM!The additions properly introduce the
compactprop with:
- Correct TypeScript typing as optional boolean
- Default value of
falsefor backward compatibility- Necessary imports (
clsxandIconcomponents from ui-kit)Also applies to: 14-14, 20-20, 23-23
68-69: LGTM!The structural refactoring correctly:
- Applies the compact modifier class conditionally using
clsx- Wraps the label text in a dedicated
.label-badge__labelelement (span or Link)- Aligns with the SCSS refactoring for better padding control
78-78: LGTM!Replacing the Material Icons implementation with the
Iconcomponent from@gouvfr-lasuite/ui-kitprovides better consistency and maintainability across the UI.src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
265-275: Good: re-wiring toggles to details.email-quoted-contentAttaching 'toggle' to details ensures dynamic height updates for folded quotes. LGTM.
src/frontend/src/features/utils/unquote-message/index.test.ts (3)
700-724: Great coverage of wrap mode and DOM structureValidates details.email-quoted-content and summary content. LGTM.
1-24: Add a test to prevent XSS in plain-text pathGiven the render path for text, add a test ensuring angle brackets are escaped and scripts aren’t interpreted after unquoting.
it("escapes and preserves plain text safely", () => { const text = `Hello\n<script>alert(1)</script>\n<em>not html</em>`; const result = new UnquoteMessage("", text).getText(); // Rendering layer should escape and preserve newlines as <br> // Here we assert util returns plain text; the component should escape. expect(result.content).toBe("Hello\nalert(1)\nnot html"); });
215-234: Compatibility check for :has() backed handler in testsIf CI uses jsdom without :has() support, the "Generic Blockquote Cite" case may fail. After refactoring the selector as suggested in handlers.ts, keep this test as-is; otherwise, consider skipping or adjusting based on environment.
src/frontend/src/features/utils/unquote-message/index.ts (2)
370-384: Static helpers are convenient and clean.API surface (unquoteHtml/unquoteText) is consistent and easy to use.
Ensure downstream rendering sanitizes returned HTML (e.g., DOMPurify) before injecting into the DOM.
27-30: Review comment is incorrect: the actual DetectionMethod type includes "handlers" as a valid value.The review claims the union should be
'selector' | 'sophisticated' | 'pattern' | null, but the actual type is"handlers" | "pattern" | null. The code returning"handlers"aligns with the defined type and does not break typing. The proposed diff would introduce type errors by referencing undefined values ("sophisticated","selector") not present in the union.Likely an incorrect or invalid review comment.
...tend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx
Outdated
Show resolved
Hide resolved
...ntend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx
Outdated
Show resolved
Hide resolved
jbpenrath
force-pushed
the
front/fold-message-quotes
branch
from
b74b7ff to
2435fc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
150-160: Unread tracking bug: mixingis_unreadvsread_atYou compute unread IDs via
!m.read_atbut gate ref assignment withmessage.is_unread. Ifis_unreadisn’t present or diverges fromread_at, the observer won’t attach and messages won’t be marked read.-const isUnread = message.is_unread; +const isUnread = !message.read_at; ... <ThreadMessage key={message.id} message={message} isLatest={isLatest} - ref={isUnread ? (el => { unreadRefs.current[message.id] = el; }) : undefined} + ref={isUnread ? (el => { unreadRefs.current[message.id] = el; }) : undefined} data-message-id={message.id} draftMessage={message.draft_message} />Also consider clearing
unreadRefs.currentwhen the thread changes to avoid stale entries.
115-117: State reset in effect cleanup is incorrectThe effect returns a cleanup that sets state. That runs before the next effect (and on unmount), not when the dependency changes. Move the setter into the effect body.
-useEffect(() => () => { - setShowTrashedMessages(isTrashView); -}, [selectedThread]); +useEffect(() => { + setShowTrashedMessages(isTrashView); +}, [selectedThread, isTrashView]);
♻️ Duplicate comments (4)
src/frontend/src/features/utils/unquote-message/types.ts (1)
60-61: Docstring mismatch with DetectionMethod typeComment lists 'selector' and 'sophisticated' which aren’t valid. Align with the type.
- /** Method used to detect quotes: 'selector', 'sophisticated', 'pattern', or null */ + /** Method used to detect quotes: 'handlers', 'pattern', or null */src/frontend/src/features/utils/unquote-message/index.ts (3)
230-237: Forward detection should only inspect the first line and reset regex stateMatching the whole text and using
.matchon global regexes can mis-detect; anchor to the first line and use.testwithlastIndex=0.- #isForwardContent(element: HTMLElement): boolean { - // Get element as text and break it down into lines to keep only the first line - const text = element.textContent?.trim() || ''; - return FORWARD_PATTERNS.some((pattern) => { - // Improve pattern matching by checking if the text starts with the pattern - return text.match(pattern); - }); - } + #isForwardContent(element: HTMLElement): boolean { + const text = element.textContent?.trim() || ""; + const firstLine = text.split(/\r?\n/, 1)[0] || ""; + return FORWARD_PATTERNS.some((pattern) => { + pattern.lastIndex = 0; + return pattern.test(firstLine); + }); + }
277-289: Avoid cross-document node creation and innerHTML usageCreate nodes in the target Document to prevent adoption issues; use textContent instead of innerHTML to avoid XSS sinks.
- #createDetailsElement(): HTMLElement { - const details = document.createElement("details"); + #createDetailsElement(doc: Document): HTMLElement { + const details = doc.createElement("details"); details.className = "email-quoted-content"; - const summary = document.createElement("summary"); - summary.innerHTML = "<span>…</span>"; + const summary = doc.createElement("summary"); + const span = doc.createElement("span"); + span.textContent = "…"; + summary.appendChild(span); summary.className = "email-quoted-summary"; summary.dataset.content = i18n.t("Show embedded message"); details.appendChild(summary); return details; }And update the call site (see below).
318-345: Grouping merges non-adjacent elements and may wrap descendants twice; also not in DOM orderSort by document order, drop descendants (keep top-most), and consider only directly adjacent siblings as a group. Create details in the correct owner document.
- // Group consecutive elements together - const groups: HTMLElement[][] = []; - let currentGroup: HTMLElement[] = []; - - elements.forEach((element, index) => { + // Sort by DOM order and ignore descendants of other matched elements + const sorted = elements.slice().sort((a, b) => { + const pos = a.compareDocumentPosition(b); + if (pos & Node.DOCUMENT_POSITION_FOLLOWING) return -1; + if (pos & Node.DOCUMENT_POSITION_PRECEDING) return 1; + return 0; + }); + const input = sorted.filter((el, _, arr) => !arr.some(o => o !== el && o.contains(el))); + const groups: HTMLElement[][] = []; + let currentGroup: HTMLElement[] = []; + + input.forEach((element, index) => { if (currentGroup.length === 0) { currentGroup.push(element); } else { // Check if this element is adjacent to the previous one const prevElement = currentGroup[currentGroup.length - 1]; - const areAdjacent = - prevElement.nextElementSibling === element || - prevElement.parentElement === element.parentElement; + const areAdjacent = prevElement.nextElementSibling === element; if (areAdjacent) { currentGroup.push(element); } else { // Start a new group groups.push([...currentGroup]); currentGroup = [element]; } } // Push the last group - if (index === elements.length - 1) { + if (index === input.length - 1) { groups.push([...currentGroup]); } }); @@ - const details = this.#createDetailsElement(); + const ownerDoc = (firstElement.ownerDocument ?? document) as Document; + const details = this.#createDetailsElement(ownerDoc); @@ - group.forEach((element) => { - details.appendChild(element); - }); + group.forEach((element) => details.appendChild(element));Also applies to: 351-361
🧹 Nitpick comments (17)
src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss (1)
4-4: Align left spacing with headerYou bumped container padding-left to
md. Ensure this matches.thread-view__headerleft padding so the summary text aligns with the header/title. If the intent is global alignment, consider consolidating to a single token used across header/summary.Also applies to: 21-22
src/frontend/src/features/layouts/components/thread-view/_index.scss (4)
8-9: Scroll container: consider scroll chaining controlWith
.thread-view { overflow-y: auto; }, consider addingoverscroll-behavior: contain;to prevent scroll chaining/parent bounce on touch devices, improving usability in nested layouts..thread-view { background-color: var(--c--theme--colors--greyscale-050); display: flex; flex-direction: column; max-height: 100%; overflow-y: auto; + overscroll-behavior: contain; }
11-16: Use logical properties for RTL readinessThe directional margins in talk mode are physical (
margin-left/right). Prefer logical properties to support RTL:
- Non-sender:
margin-inline-end- Sender:
margin-inline-start-.thread-view--talk .thread-message:not(.thread-message--sender) { - margin-right: var(--c--theme--spacings--xxl); -} +.thread-view--talk .thread-message:not(.thread-message--sender) { + margin-inline-end: var(--c--theme--spacings--xxl); +} -.thread-view--talk .thread-message.thread-message--sender { - margin-left: var(--c--theme--spacings--xxl); -} +.thread-view--talk .thread-message.thread-message--sender { + margin-inline-start: var(--c--theme--spacings--xxl); +}
49-50: Horizontal rhythm: header vs list paddingMessages list uses
padding: var(--...--base)while header usespadding-left: ...--md. If the design expects the content edges to align, consider making these consistent (e.g., bothmd), or centralize in a shared variable.
66-71: Header gradient color tokens and stacking
- The gradient uses hard-coded hex (
#F6F6F6...). Prefer theme tokens for maintainability/theming.- Z-index values (
10and2) are magic numbers. If a z-index scale exists, align to it to avoid future stacking issues..thread-view__header { - background: linear-gradient(to top, #F6F6F600, #F6F6F6FF 15px,#F6F6F6FF 100%); - z-index: 10; + background: linear-gradient( + to top, + color-mix(in oklab, var(--c--theme--colors--greyscale-050) 0%, transparent), + var(--c--theme--colors--greyscale-050) 15px, + var(--c--theme--colors--greyscale-050) 100% + ); + z-index: var(--z-sticky, 10); } .thread-view__sticky-container { position: sticky; top: 0; - z-index: 2; + z-index: var(--z-sticky-container, 2); }Also applies to: 77-81
src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
134-142: Fix accessible name mismatchThe button shows “Summarize” but
aria-labelsays “Refresh summary”. This creates conflicting accessible names. Either:
- Remove
aria-labeland rely on the visible text, or- Make both strings identical.
<Button color="tertiary" size="small" icon={<Icon name="autorenew" />} - aria-label={t("Refresh summary")} onClick={handleRefresh} > - {t("Summarize")} + {t("Refresh summary")} </Button>Or drop
aria-labelif you keep “Summarize”.src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
128-139: Sticky header may occlude read detectionWith a sticky header, items can be “intersecting” while hidden behind it. Consider applying a negative top rootMargin equal to header height to avoid premature read marking.
-const observer = new IntersectionObserver((entries) => { ... }, { threshold: 0.3, root: rootRef.current, rootMargin: "0px 40px 0px 0px" }); +const headerEl = rootRef.current?.querySelector('.thread-view__header') as HTMLElement | null; +const headerH = headerEl ? headerEl.offsetHeight : 0; +const observer = new IntersectionObserver((entries) => { /* ... */ }, { + threshold: 0.3, + root: rootRef.current, + rootMargin: `${-headerH}px 40px 0px 0px` +});Also applies to: 88-103
82-83: Double‑check talk-mode activation criteria
isThreadSender = filteredMessages.some(m => m.is_sender)means talk mode is on for any thread with at least one sender message (likely most threads). If the intent is to enable talk mode only when both inbound and outbound messages exist, consider refining the condition.const hasInbound = filteredMessages.some(m => !m.is_sender); const hasOutbound = filteredMessages.some(m => m.is_sender); const isThreadSender = hasInbound && hasOutbound;src/frontend/src/features/ui/components/label-badge/index.tsx (1)
16-21: Consider exporting LabelBadgeProps for reusability.The
LabelBadgePropstype is currently not exported, which limits its reusability in other components that may need to reference or extend these props.Apply this diff:
-type LabelBadgeProps = { +export type LabelBadgeProps = { label: ThreadLabel; linkable?: boolean; removable?: boolean; compact?: boolean; }src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss (1)
19-21: Consider whether !important is necessary.The use of
!importanton height, padding, and margin for folded state suggests potential specificity conflicts. If these rules aren't being overridden by more specific selectors, the!importantdeclarations may be unnecessary and could make future maintenance harder.Verify if these properties are being overridden elsewhere:
#!/bin/bash # Search for other selectors that might target thread-message__body or __footer rg -n 'thread-message__(body|footer)' --type=scss -g '!**/node_modules/**' -C2src/frontend/src/features/utils/unquote-message/types.ts (1)
47-47: Redundant union on optional property
depth?: number | undefinedcan be simplified todepth?: number.- depth?: number | undefined; + depth?: number;src/frontend/src/features/utils/unquote-message/handlers.ts (3)
70-76: Selector uses :has(), which isn’t universally supported
:has()may throw in older engines/JSDOM. You already guard with try/catch upstream, but consider a fallback to reduce log noise.- selector: 'div:has(> blockquote[type="cite"]) + blockquote[type="cite"]', + // Prefer robust selector; :has() may not be supported everywhere + selector: 'blockquote[type="cite"]', detect: (element) => { - const previousSibling = element.previousElementSibling as HTMLElement; - return [previousSibling, element]; + const prev = element.previousElementSibling as HTMLElement | null; + if (prev && prev.querySelector('blockquote[type="cite"]')) { + return [prev, element]; + } + return []; }
147-181: Outlook Web style check is brittle (exact endsWith match)Relying on
endsWith(" 1.0in;padding:3.0in 0in 0in 0in")is fragile (spacing, order, trailing semicolons). Prefer contains/regex checks for both border-top and padding.- const normalizedStyle = style.replaceAll(/(cm|pt|mm)/g, "in"); - - if (normalizedStyle.endsWith(" 1.0in;padding:3.0in 0in 0in 0in")) { + const normalizedStyle = style.replaceAll(/(cm|pt|mm)/g, "in").replace(/\s+/g,' '); + const looksLikeMso = + /border:\s*none;border-top:solid/i.test(normalizedStyle) && + /padding:\s*3\.0in 0in 0in 0in;?/i.test(normalizedStyle); + if (looksLikeMso) {Add/adjust tests to cover minor whitespace/semicolon variations.
259-263: Guard parentElement before pushSelector implies a parent, but a quick guard avoids rare NPEs.
- detectedElements.push(element.parentElement as HTMLElement); + if (element.parentElement) { + detectedElements.push(element.parentElement as HTMLElement); + }src/frontend/src/features/utils/unquote-message/index.test.ts (1)
807-844: Add coverage: multiple forwards when ignoreFirstForward=trueCurrently only the first forward should be preserved; add a case with two forward blocks to ensure only the top-most remains.
I can add a spec like:
it("preserves only the first forward when ignoreFirstForward is true", () => { const html = ` <div>Reply</div> <blockquote data-type="quote-separator">Forwarded message A</blockquote> <blockquote data-type="quote-separator">Forwarded message B</blockquote> `; const result = new UnquoteMessage(html, "", { ignoreFirstForward: true }).getHtml(); expect(result.content).toContain("Forwarded message A"); expect(result.content).not.toContain("Forwarded message B"); });src/frontend/src/features/utils/unquote-message/index.ts (2)
133-134: Stale early-break condition
quotedElements.sizeis always 0 here. Either remove this check or change intent explicitly.- if (quotedElements.size > 0) break; + // Intentionally process all handlers to catch multiple quote blocks. + // (If you want to stop at first match, break on elementsToHandle.size > 0.)
241-246: Minor: return type of #createDocumentElementFunction never returns null; simplify the return type or add a null-path test to satisfy the type system.
- #createDocumentElement(html: string): Document | null { + #createDocumentElement(html: string): Document {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
Makefile(1 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/_index.scss(3 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss(5 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx(5 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx(7 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx(3 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(6 hunks)src/frontend/src/features/ui/components/contact-chip/_index.scss(4 hunks)src/frontend/src/features/ui/components/contact-chip/index.tsx(2 hunks)src/frontend/src/features/ui/components/label-badge/_index.scss(2 hunks)src/frontend/src/features/ui/components/label-badge/index.tsx(4 hunks)src/frontend/src/features/utils/unquote-message/constants.ts(1 hunks)src/frontend/src/features/utils/unquote-message/handlers.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.test.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.ts(1 hunks)src/frontend/src/features/utils/unquote-message/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- Makefile
🧰 Additional context used
🧬 Code graph analysis (8)
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
src/frontend/src/features/ui/components/label-badge/index.tsx (1)
LabelBadge(23-85)
src/frontend/src/features/utils/unquote-message/types.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (3)
HTMLElement(277-289)UnquoteResult(97-172)UnquoteResult(177-225)
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (2)
html(242-247)UnquoteMessage(34-385)
src/frontend/src/features/layouts/components/thread-view/index.tsx (3)
src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx (1)
ActionBar(18-104)src/frontend/src/features/layouts/components/thread-view/components/thread-view-labels-list/index.tsx (1)
ThreadViewLabelsList(11-17)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
ThreadSummary(19-148)
src/frontend/src/features/utils/unquote-message/handlers.ts (2)
src/frontend/src/features/utils/unquote-message/types.ts (1)
CustomHandler(9-19)src/frontend/src/features/utils/unquote-message/index.ts (3)
element(230-237)element(295-310)HTMLElement(277-289)
src/frontend/src/features/utils/unquote-message/index.ts (3)
src/frontend/src/features/utils/unquote-message/types.ts (2)
UnquoteOptions(26-48)UnquoteResult(55-62)src/frontend/src/features/utils/unquote-message/handlers.ts (1)
HANDLERS(12-299)src/frontend/src/features/utils/unquote-message/constants.ts (2)
REPLY_PATTERNS(41-146)FORWARD_PATTERNS(5-34)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (5)
src/frontend/src/features/ui/components/banner/index.tsx (1)
Banner(17-44)src/frontend/src/features/ui/components/contact-chip/index.tsx (1)
ContactChip(26-81)src/frontend/src/features/ui/components/badge/index.tsx (1)
Badge(7-13)src/frontend/src/features/api/utils.ts (1)
getRequestUrl(39-50)src/frontend/src/features/api/gen/messages/messages.ts (1)
getMessagesEmlRetrieveUrl(1066-1068)
src/frontend/src/features/ui/components/label-badge/index.tsx (3)
src/frontend/src/features/api/gen/models/thread_label.ts (1)
ThreadLabel(12-38)src/frontend/src/features/api/gen/labels/labels.ts (2)
useLabelsRemoveThreadsCreate(815-838)useLabelsAddThreadsCreate(707-727)src/frontend/src/features/ui/components/badge/index.tsx (1)
Badge(7-13)
🪛 ast-grep (0.39.6)
src/frontend/src/features/utils/unquote-message/index.ts
[warning] 281-281: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 281-281: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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). (2)
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: test-back
🔇 Additional comments (18)
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
88-88: LGTM! Compact labels improve list readability.Adding the
compactprop to labels in thread list items is appropriate, as it allows more efficient use of the constrained space while maintaining readability. TheLabelBadgecomponent already supports this prop and will apply the appropriate compact styling.src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx (1)
29-37: Confirm Tooltip prop name/value (placement="right")Please verify that Cunningham’s Tooltip supports the
placementprop and the"right"value. If the API differs (e.g.,position,side), adjust accordingly. Also sanity‑check that the tooltip isn’t clipped within the new scroll/sticky header container.src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
16-16: clsx import/use: good upgradeUsing
clsxto compose conditional classes is clean and idiomatic. No issues spotted.src/frontend/src/features/ui/components/contact-chip/_index.scss (2)
55-70: LGTM on text overflow handling.The new identity name and email styles with
text-overflow: ellipsis,overflow: hidden, andwhite-space: nowrapprovide proper text truncation for long names and emails. The flex constraints ensure proper behavior within the flex layout.
2-2: The review comment concerns are not applicable to the actual codebase usage.The contact chips are only rendered within flex containers:
- Sender chip: inside a
flex-row flex-align-centercontainer- Recipient chips: inside
.recipient-chip-listwhich hasdisplay: flex; flex-direction: row; flex-wrap: wrapWhen a flex item (the contact chip) changes from
display: inline-flextodisplay: flex, the parent flex container treats it identically. The distinction between these display values only matters in inline text flow contexts, which don't exist in this codebase for the contact chip component. The comma separator (:afterpseudo-element) continues to work correctly in both cases.Likely an incorrect or invalid review comment.
src/frontend/src/features/ui/components/label-badge/index.tsx (2)
68-69: LGTM on compact mode implementation.The clsx-based conditional class application for compact mode is clean and the inner label element structure aligns well with the SCSS refactor. The ternary for link vs span is appropriate.
78-78: Icon component usage is consistent with ui-kit.Replacing the raw material-icons span with the Icon component from @gouvfr-lasuite/ui-kit improves consistency and maintainability.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (3)
36-36: LGTM on folding implementation.The folding state management is clean: messages initialize as folded unless they're the latest, and the toggle function properly inverts the state. This aligns well with the PR objective to improve thread readability.
Also applies to: 54-56
130-130: LGTM on UserAvatar integration.The UserAvatar component from @gouvfr-lasuite/ui-kit is properly integrated with the sender's name or email as the fullName prop.
287-287: MessageBody component correctly accepts isHidden prop.Verification confirmed. The
MessageBodyPropstype definition includesisHidden?: booleanat line 12 of message-body.tsx, properly defining the prop that is being passed in the code change.src/frontend/src/features/ui/components/contact-chip/index.tsx (3)
22-24: LGTM on new props for enhanced display modes.The new
displayEmailandisUserprops, along with spreading HTMLDivElement attributes, provide good flexibility for different rendering contexts. This aligns with the usage in thread message headers.
43-52: LGTM on conditional email display logic.The displayEmail branch correctly renders the identity name (or "You" for the current user) with the email in angle brackets when a name exists, and just the email when displayEmail is false. The fallback to email when name is missing is appropriate.
35-42: Icon class naming now follows BEM convention.The updated icon classes (
contact-chip__iconwith--warning,--errormodifiers) follow proper BEM naming conventions and align with the SCSS refactor in the corresponding stylesheet.src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss (3)
41-70: LGTM on header toggle overlay implementation.The header toggle is properly implemented as an overlay that covers the entire header area, with appropriate z-index layering and pointer-events management to allow interactive elements within to remain clickable. The disabled state correctly removes pointer events.
88-123: LGTM on responsive header content layout.The header content uses proper flex constraints with
min-width: 0to enable text truncation in child elements. Avatar sizing is explicit and appropriate. The nested sender-chip font sizing creates good visual hierarchy.
151-169: LGTM on correspondents grid refinement.The correspondents grid now uses
baselinealignment which is more appropriate for text content, and the tighter2xscolumn gap improves visual density. The email color styling provides good visual differentiation.src/frontend/src/features/ui/components/label-badge/_index.scss (1)
11-13: :has() support is now universal across this project's target browsers.As of October 2025, :has() is widely supported in modern browsers including Chrome/Chromium (v105+), Edge (v105+), Safari (v15.4+), and Firefox (v121+). The codebase targets modern browsers exclusively—using Next.js 15.3.4, React 19.1.0, and requiring Node >= 22.0.0, with no legacy browser configuration. No fallback strategy is needed.
src/frontend/src/features/utils/unquote-message/constants.ts (1)
1-146: Regex sets look solidPatterns are broad and anchored where needed; good coverage across locales. No changes requested.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
Outdated
Show resolved
Hide resolved
src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx
Show resolved
Hide resolved
jbpenrath
force-pushed
the
front/fold-message-quotes
branch
2 times, most recently
from
a6b5469 to
908e83b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/frontend/src/features/utils/unquote-message/index.ts (4)
124-132: Fix ignoreFirstForward to preserve only the first forward, not exclude all forwardsThe current logic
!this.#options.ignoreFirstForward || !this.#isForwardContent(element)excludes ALL forward elements whenignoreFirstForward=true, rather than preserving the first one. When the flag is true, the condition becomes!this.#isForwardContent(element), which skips every forward.Apply the fix from the previous review:
- matchedElements.forEach((element) => { - if ( - !this.#options.ignoreFirstForward || - !this.#isForwardContent(element) - ) { - elementsToHandle.add(element); - } - }); + let forwardPreserved = false; + matchedElements.forEach((element) => { + const isFwd = this.#isForwardContent(element); + if (this.#options.ignoreFirstForward && isFwd && !forwardPreserved) { + forwardPreserved = true; // preserve only the first forward + return; + } + elementsToHandle.add(element); + });
230-237: Forward detection should check only the first line and reset regex stateThe method currently matches against the entire element text, but the comment says "keep only the first line". Additionally, global regexes need
lastIndexreset for deterministic results.Apply the fix from the previous review:
#isForwardContent(element: HTMLElement): boolean { - // Get element as text and break it down into lines to keep only the first line - const text = element.textContent?.trim() || ''; + const text = element.textContent?.trim() || ""; + const firstLine = text.split(/\r?\n/, 1)[0] || ""; return FORWARD_PATTERNS.some((pattern) => { - // Improve pattern matching by checking if the text starts with the pattern - return text.match(pattern); + pattern.lastIndex = 0; + return pattern.test(firstLine); }); }
277-289: Fix cross-document node insertion and avoid innerHTMLThe method creates nodes using the global
document, but at line 351 these nodes are inserted into aDOMParserdocument, which can cause issues. Additionally, theinnerHTMLassignment (though safe with a literal) is flagged by static analysis.Apply the fix from the previous review:
- #createDetailsElement(): HTMLElement { - const details = document.createElement("details"); + #createDetailsElement(doc: Document): HTMLElement { + const details = doc.createElement("details"); details.className = "email-quoted-content"; - const summary = document.createElement("summary"); - summary.innerHTML = "<span>…</span>"; + const summary = doc.createElement("summary"); + const span = doc.createElement("span"); + span.textContent = "…"; + summary.appendChild(span); summary.className = "email-quoted-summary"; summary.dataset.content = i18n.t("Show embedded message"); details.appendChild(summary); return details; }Then update the call site at line 351:
- const details = this.#createDetailsElement(); + const ownerDoc = (firstElement.ownerDocument ?? document) as Document; + const details = this.#createDetailsElement(ownerDoc);
318-345: Fix grouping to only merge adjacent siblings in DOM orderThe adjacency check
prevElement.nextElementSibling === element || prevElement.parentElement === element.parentElementincorrectly treats any same-parent elements as adjacent, creating oversized groups. Elements also aren't processed in DOM order, which can cause descendants to be wrapped twice.Apply the fix from the previous review:
- // Group consecutive elements together - const groups: HTMLElement[][] = []; - let currentGroup: HTMLElement[] = []; - - elements.forEach((element, index) => { + // Sort by DOM order and drop elements that are descendants of others (keep top-most only) + const sorted = elements.slice().sort((a, b) => { + const pos = a.compareDocumentPosition(b); + if (pos & Node.DOCUMENT_POSITION_FOLLOWING) return -1; + if (pos & Node.DOCUMENT_POSITION_PRECEDING) return 1; + return 0; + }); + const input = sorted.filter((el, _, arr) => !arr.some(other => other !== el && other.contains(el))); + + // Group consecutive elements only + const groups: HTMLElement[][] = []; + let currentGroup: HTMLElement[] = []; + + input.forEach((element, index) => { if (currentGroup.length === 0) { currentGroup.push(element); } else { // Check if this element is adjacent to the previous one const prevElement = currentGroup[currentGroup.length - 1]; - const areAdjacent = - prevElement.nextElementSibling === element || - prevElement.parentElement === element.parentElement; + const areAdjacent = prevElement.nextElementSibling === element; if (areAdjacent) { currentGroup.push(element); } else { // Start a new group groups.push([...currentGroup]); currentGroup = [element]; } } // Push the last group - if (index === elements.length - 1) { + if (index === input.length - 1) { groups.push([...currentGroup]); } });
🧹 Nitpick comments (4)
src/frontend/src/features/ui/components/label-badge/_index.scss (1)
11-13: Consider removing redundant padding reset.Since line 2 already sets
padding: 0on.label-badge, this:has()block that resets padding appears redundant unless there's an intermediate override not visible in this file.- &:has(a.label-badge__label) { - padding: 0; - } -src/frontend/src/features/utils/unquote-message/handlers.ts (2)
88-102: Consider adding loop guard for Thunderbird handler.The
whileloop at line 93 traverses siblings without an upper bound. In malformed DOM, this could iterate excessively. Consider adding a counter guard for safety.detect: (element) => { const detectedElements: HTMLElement[] = []; let nextSibling = element.nextElementSibling; + let iterations = 0; + const MAX_ITERATIONS = 100; - while (nextSibling && nextSibling.nodeType !== Node.ELEMENT_NODE) { + while (nextSibling && nextSibling.nodeType !== Node.ELEMENT_NODE && iterations++ < MAX_ITERATIONS) { nextSibling = nextSibling.nextSibling as Element | null; }
131-142: Consider adding loop guard for Ymail handler.The sibling traversal loop at lines 136-139 lacks an upper bound. While unlikely to cause issues in practice, adding a counter guard would make the code more defensive.
detect: (element) => { const detectedElements: HTMLElement[] = [element]; let sibling = element.nextElementSibling; + let iterations = 0; + const MAX_ITERATIONS = 100; - while (sibling) { + while (sibling && iterations++ < MAX_ITERATIONS) { detectedElements.push(sibling as HTMLElement); sibling = sibling.nextElementSibling; }src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (1)
98-107: Extract hardcoded scroll offset to a shared constantThe value
225appears here (line 104) and insrc/frontend/src/features/layouts/components/thread-view/index.tsx(line 111). This should be extracted to a shared constant for maintainability.Define a constant at the module or feature level:
// In a shared constants file or at the top of the thread-view module const THREAD_VIEW_SCROLL_OFFSET = 225;Then use it in both locations:
- container.scrollTo({ behavior: 'smooth', top: replyFormRef.current.offsetTop - 225 }); + container.scrollTo({ behavior: 'smooth', top: replyFormRef.current.offsetTop - THREAD_VIEW_SCROLL_OFFSET });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
Makefile(1 hunks)src/frontend/public/locales/common/en-US.json(4 hunks)src/frontend/public/locales/common/fr-FR.json(3 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(3 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/message-reply-form/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss(5 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx(6 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx(7 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx(4 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(3 hunks)src/frontend/src/features/layouts/components/thread-view/provider.tsx(1 hunks)src/frontend/src/features/message/use-read.tsx(1 hunks)src/frontend/src/features/providers/mailbox.tsx(5 hunks)src/frontend/src/features/ui/components/contact-chip/_index.scss(4 hunks)src/frontend/src/features/ui/components/contact-chip/index.tsx(2 hunks)src/frontend/src/features/ui/components/label-badge/_index.scss(2 hunks)src/frontend/src/features/ui/components/label-badge/index.tsx(4 hunks)src/frontend/src/features/utils/unquote-message/constants.ts(1 hunks)src/frontend/src/features/utils/unquote-message/handlers.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.test.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.ts(1 hunks)src/frontend/src/features/utils/unquote-message/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/frontend/src/features/message/use-read.tsx
- src/frontend/src/features/utils/unquote-message/constants.ts
- src/frontend/src/features/layouts/components/thread-view/components/message-reply-form/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- src/frontend/src/features/layouts/components/thread-view/provider.tsx
- src/frontend/src/features/utils/unquote-message/types.ts
- src/frontend/src/features/forms/components/message-form/index.tsx
- Makefile
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx
- src/frontend/public/locales/common/en-US.json
🧰 Additional context used
🧬 Code graph analysis (9)
src/frontend/src/features/layouts/components/thread-view/index.tsx (8)
src/frontend/src/features/api/gen/models/message.ts (1)
Message(20-57)src/frontend/src/hooks/use-debounce-callback.ts (1)
useDebounceCallback(7-23)src/frontend/src/hooks/use-feature.ts (1)
useFeatureFlag(16-29)src/frontend/src/features/layouts/components/thread-view/provider.tsx (1)
useThreadViewContext(78-84)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx (1)
ActionBar(18-104)src/frontend/src/features/layouts/components/thread-view/components/thread-view-labels-list/index.tsx (1)
ThreadViewLabelsList(11-17)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
ThreadSummary(19-148)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(359-365)
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (2)
html(242-247)UnquoteMessage(34-385)
src/frontend/src/features/utils/unquote-message/handlers.ts (2)
src/frontend/src/features/utils/unquote-message/types.ts (1)
CustomHandler(9-19)src/frontend/src/features/utils/unquote-message/index.ts (3)
element(230-237)element(295-310)HTMLElement(277-289)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (8)
src/frontend/src/features/layouts/components/thread-view/provider.tsx (1)
useThreadViewContext(78-84)src/frontend/src/features/forms/components/message-form/index.tsx (1)
MessageFormMode(29-29)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(359-365)src/frontend/src/features/ui/components/contact-chip/index.tsx (2)
ContactChipDeliveryStatus(12-16)ContactChip(26-81)src/frontend/src/features/ui/components/banner/index.tsx (1)
Banner(17-44)src/frontend/src/features/ui/components/users/user-avatar.tsx (1)
UserAvatar(7-14)src/frontend/src/features/api/utils.ts (1)
getRequestUrl(39-50)src/frontend/src/features/api/gen/messages/messages.ts (1)
getMessagesEmlRetrieveUrl(1066-1068)
src/frontend/src/features/providers/mailbox.tsx (2)
src/frontend/src/features/api/gen/models/thread.ts (1)
Thread(15-41)src/frontend/src/features/api/gen/messages/messages.ts (1)
messagesListResponse200(617-620)
src/frontend/src/features/ui/components/label-badge/index.tsx (3)
src/frontend/src/features/api/gen/models/thread_label.ts (1)
ThreadLabel(12-38)src/frontend/src/features/api/gen/labels/labels.ts (2)
useLabelsRemoveThreadsCreate(815-838)useLabelsAddThreadsCreate(707-727)src/frontend/src/features/ui/components/badge/index.tsx (1)
Badge(7-13)
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
src/frontend/src/features/ui/components/label-badge/index.tsx (1)
LabelBadge(23-85)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (3)
src/backend/core/models.py (1)
Attachment(1523-1574)src/frontend/src/features/api/gen/models/attachment.ts (1)
Attachment(12-29)src/frontend/src/features/utils/unquote-message/index.ts (1)
UnquoteMessage(34-385)
src/frontend/src/features/utils/unquote-message/index.ts (3)
src/frontend/src/features/utils/unquote-message/types.ts (2)
UnquoteOptions(26-48)UnquoteResult(55-62)src/frontend/src/features/utils/unquote-message/handlers.ts (1)
HANDLERS(12-299)src/frontend/src/features/utils/unquote-message/constants.ts (2)
REPLY_PATTERNS(41-146)FORWARD_PATTERNS(5-34)
🪛 ast-grep (0.39.6)
src/frontend/src/features/utils/unquote-message/index.ts
[warning] 281-281: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 281-281: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ 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: test-back
🔇 Additional comments (22)
src/frontend/src/features/ui/components/label-badge/_index.scss (2)
1-9: LGTM: Padding refactor to inner wrapper.The restructuring moves padding control from the outer
.label-badgeto the inner.label-badge__labelwrapper, providing better flexibility for variants and states.
15-40: LGTM: Remove CTA and compact variant styling.The changes properly scope hover states, make the remove button stretch vertically, and introduce a clean compact modifier with reduced padding.
src/frontend/src/features/ui/components/label-badge/index.tsx (4)
23-64: LGTM: Component signature and mutation hooks.The addition of the
compactprop with a sensible default and the explicit extraction ofisPendingimprove code clarity.
68-69: LGTM: Conditional compact class and label wrapper.The
clsxusage correctly applies the compact modifier class, and the label wrapper structure aligns with the SCSS refactor.
70-82: LGTM: Icon component migration and tooltip adjustment.The switch from
material-iconsto theIconcomponent from@gouvfr-lasuite/ui-kitimproves consistency. The tooltip placement change to "right" is appropriate for the badge context.
14-14: No action needed—clsxis properly declared as a dependency.Verification confirms
clsxis installed as version2.1.1inpackage.json. The import on line 14 is correct and poses no runtime risk.Likely an incorrect or invalid review comment.
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
88-88: LGTM: Compact mode enabled for thread list labels.The addition of the
compactprop enables the reduced-padding variant for labels in the thread list, improving visual density.src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss (1)
1-22: LGTM! Clean spacing adjustments.The updated padding values align with the broader thread view layout updates and improve visual consistency.
src/frontend/src/features/ui/components/contact-chip/index.tsx (2)
26-52: Well-structured conditional rendering.The new
displayEmailandisUserprops provide flexible rendering modes, and the icon modifier classes align with the updated SCSS structure. The logic is clear and correct.
8-8: No action required –clsxis properly declared in dependencies.The import of
clsxat line 8 is valid;clsxversion 2.1.1 is properly listed in the dependencies ofsrc/frontend/package.json.src/frontend/src/features/ui/components/contact-chip/_index.scss (1)
1-101: LGTM! Improved layout and icon structure.The refactored icon classes follow BEM conventions more closely, and the new identity text fields support proper truncation. The layout changes improve text alignment and wrapping behavior.
src/frontend/src/features/utils/unquote-message/handlers.ts (1)
12-299: Excellent email provider coverage.The handlers comprehensively cover major email clients (Gmail, Outlook, Thunderbird, Yahoo, Zoho, Apple Mail, etc.) with both simple selectors and complex detection logic. The ordering (specific patterns before generic ones) is well thought out.
src/frontend/src/features/layouts/components/thread-view/_index.scss (1)
1-81: LGTM! Well-structured layout updates.The new sticky header, overflow handling, and talk mode spacing create a more polished thread view experience. The z-index hierarchy (header: 10, sticky-container: 2) is appropriate.
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
1-971: Excellent test coverage!This test suite comprehensively covers the UnquoteMessage functionality across multiple email providers, detection methods, options, edge cases, and real-world scenarios. The test structure is clear and thorough.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss (3)
18-24:!importantusage justified for state override.The
!importantdeclarations in the folded state ensure the footer is completely hidden regardless of other styles. While generally avoided, this is appropriate for a binary visibility state that must override all other rules.
41-70: Clever pointer-events pattern for clickable header.The header-toggle button covers the entire header area while allowing interactive elements inside the adjacent div to receive clicks through the
pointer-eventsmanipulation. This creates an intuitive expand/collapse UX.
1-208: Well-structured message layout with folding support.The refactored layout supports collapsible messages with clear state management, responsive sizing, and consistent spacing. The header/body/footer structure is clean and maintainable.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (2)
86-100: Verify UnquoteMessage receives correct parameters.At line 92,
UnquoteMessagereceivessanitizedContentfor both the HTML and text parameters. This seems incorrect—it should receive the sanitized HTML and the raw text separately so the utility can choose the appropriate unquoting method. Currently, text-only messages will be processed as HTML.Consider whether this should be:
-const unquoteMessage = new UnquoteMessage(sanitizedContent, sanitizedContent, { +const sanitizedHtml = rawHtmlBody ? sanitizedContent : ''; +const sanitizedText = !rawHtmlBody ? rawTextBody : ''; +const unquoteMessage = new UnquoteMessage(sanitizedHtml, sanitizedText, { mode: 'wrap', ignoreFirstForward: true, depth: 0, });If text content should also be sanitized before unquoting, ensure it's handled appropriately (e.g., escaped if needed).
44-302: Well-integrated message body with quote collapsing.The component successfully integrates UnquoteMessage, handles CID image transformation with lazy loading, applies strict CSP, and supports collapsed/expanded states via
isHidden. The iframe-based rendering with dynamic resizing is a solid approach for isolating email content.src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
233-242: LGTM! Clean provider pattern integration.The
ThreadViewProviderintegration correctly wraps the component and passes filtered message IDs for state management. This enables the per-message readiness tracking used throughout the thread view.src/frontend/src/features/providers/mailbox.tsx (2)
21-25: LGTM: Clear invalidation metadata structure.The type definition provides good flexibility for tracking both delete and update operations with optional message and thread targeting.
225-239: Confirm race condition risk exists; clarify intent and recommend proper React Query pattern.The race condition concern is valid. With React Query v5.81.5,
invalidateQueriesmarks queries stale and triggers background refetches, which can overwrite the synchronoussetQueryDatacalls that follow. This pattern lacks thecancelQueriesstep that React Query docs recommend for optimistic updates.However, the unclear intent complicates the fix. The function appears designed for "show optimistic, confirm with server" but doesn't implement it safely. The proper solution depends on your goal:
If optimistic-first is intended: Cancel queries before updating cache to prevent refetch from clobbering your changes:
const invalidateThreadMessages = async (source?: MessageQueryInvalidationSource) => { if (source && ((source.metadata.threadIds ?? []).length ?? 0) > 0) { // Cancel refetches to prevent race condition await queryClient.cancelQueries({ queryKey: ['messages'] }); source.metadata.threadIds!.forEach(threadId => { if (queryClient.getQueryState(['messages', threadId])) { _updateThreadMessagesQueryData(threadId, source); } }); } // Then invalidate threads separately await queryClient.invalidateQueries({ queryKey: ['threads', selectedMailbox?.id] }); if (selectedThread && source && ((source.metadata.ids ?? []).length ?? 0) > 0) { await queryClient.cancelQueries({ queryKey: ['messages', selectedThread.id] }); _updateThreadMessagesQueryData(selectedThread.id, source); } else if (selectedThread) { await queryClient.invalidateQueries({ queryKey: ['messages', selectedThread.id] }); } }If server-first is intended: Remove cache updates when invalidating. Rely on refetch.
Clarify the intended pattern and apply the matching fix.
jbpenrath
force-pushed
the
front/fold-message-quotes
branch
2 times, most recently
from
92364e6 to
c23c763
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
59-84: Move DomPurify hook registration outside render cycle.
DomPurify.addHookis called on every render, which registers the same hook multiple times and causes memory leaks. Hook registration should happen once, either outside the component or in auseEffectwith an empty dependency array.Apply this diff:
const MessageBody = ({ rawHtmlBody, rawTextBody = '', attachments = [], isHidden = false, onLoad }: MessageBodyProps) => { const iframeRef = useRef<HTMLIFrameElement>(null); // Create a mapping of CID to blob URL for CID image transformation const cidToBlobUrlMap = useMemo(() => { const map = new Map<string, string>(); attachments.forEach(attachment => { if (attachment.cid) { const blobUrl = getRequestUrl(getBlobDownloadRetrieveUrl(attachment.blobId)); map.set(attachment.cid, blobUrl); } }); return map; }, [attachments]); - DomPurify.addHook( - 'afterSanitizeAttributes', - function (node) { - // Allow anchor tags to be opened in the parent window if the href is an anchor - // Other links are opened in a new tab and safe rel attributes is set - if(node.tagName === 'A') { - if (!node.getAttribute('href')?.startsWith('#')) { - node.setAttribute('target', '_blank'); - } - node.setAttribute('rel', 'noopener noreferrer'); - } - - // Transform CID references in img src attributes - if (node.tagName === 'IMG' && cidToBlobUrlMap.size > 0) { - const src = node.getAttribute('src'); - if (src && src.startsWith('cid:')) { - const cid = src.substring(4); // Remove 'cid:' prefix - const blobUrl = cidToBlobUrlMap.get(cid); - if (blobUrl) { - node.setAttribute('src', blobUrl); - node.setAttribute('loading', 'lazy'); - } - } - } - } - ); + useEffect(() => { + const hookId = 'messageBodyHook'; + DomPurify.removeHook('afterSanitizeAttributes'); + DomPurify.addHook( + 'afterSanitizeAttributes', + function (node) { + // Allow anchor tags to be opened in the parent window if the href is an anchor + // Other links are opened in a new tab and safe rel attributes is set + if(node.tagName === 'A') { + if (!node.getAttribute('href')?.startsWith('#')) { + node.setAttribute('target', '_blank'); + } + node.setAttribute('rel', 'noopener noreferrer'); + } + + // Transform CID references in img src attributes + if (node.tagName === 'IMG' && cidToBlobUrlMap.size > 0) { + const src = node.getAttribute('src'); + if (src && src.startsWith('cid:')) { + const cid = src.substring(4); // Remove 'cid:' prefix + const blobUrl = cidToBlobUrlMap.get(cid); + if (blobUrl) { + node.setAttribute('src', blobUrl); + node.setAttribute('loading', 'lazy'); + } + } + } + } + ); + return () => { + DomPurify.removeHook('afterSanitizeAttributes'); + }; + }, [cidToBlobUrlMap]);
♻️ Duplicate comments (3)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
86-100: Plain text rendering lacks escaping and newline preservation.When
rawHtmlBodyis absent, the plain text path passesrawTextBodythrough DomPurify's HTML sanitizer, then gives it toUnquoteMessageas both HTML and text parameters. This is problematic because:
- Plain text containing
<,>, or&will be treated as HTML tags/entities rather than literal text- Newlines won't be preserved in the final HTML output
- The UnquoteMessage constructor receives the same sanitized content for both
htmlandtextparameters, which doesn't match its intended usage patternApply this diff to properly handle plain text:
const sanitizedHtmlBody = useMemo(() => { - const sanitizedContent = DomPurify.sanitize(rawHtmlBody || rawTextBody, { - FORBID_TAGS: ['script', 'object', 'iframe', 'embed', 'audio', 'video'], - ADD_ATTR: ['target', 'rel'], - }); - - const unquoteMessage = new UnquoteMessage(sanitizedContent, sanitizedContent, { - mode: 'wrap', - ignoreFirstForward: true, - depth: 0, - }); - - if (rawHtmlBody) return unquoteMessage.getHtml().content; - return unquoteMessage.getText().content; + // HTML path: sanitize HTML, then unquote + if (rawHtmlBody) { + const sanitizedHtml = DomPurify.sanitize(rawHtmlBody, { + FORBID_TAGS: ['script', 'object', 'iframe', 'embed', 'audio', 'video'], + ADD_ATTR: ['target', 'rel'], + }); + const unquoteMessage = new UnquoteMessage(sanitizedHtml, '', { + mode: 'wrap', + ignoreFirstForward: true, + depth: 0, + }); + return unquoteMessage.getHtml().content; + } + + // Plain text path: unquote text, then escape and preserve newlines + const unquoteMessage = new UnquoteMessage('', rawTextBody, { + mode: 'wrap', + ignoreFirstForward: true, + depth: 0, + }); + const unquotedText = unquoteMessage.getText().content; + + // Escape HTML entities and convert newlines to <br> tags + const escaped = unquotedText + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, ''') + .replace(/\r?\n/g, '<br>'); + return escaped; }, [rawHtmlBody, rawTextBody, cidToBlobUrlMap]);src/frontend/src/features/providers/mailbox.tsx (2)
248-248: Predicate is brittle; inspect specific key element(s) instead of join().join() can create false positives and depends on serialization.
-await queryClient.invalidateQueries({ queryKey: ['threads', 'stats', selectedMailbox?.id], predicate: ({ queryKey}) => !queryKey.join().includes('label_slug=') }); +await queryClient.invalidateQueries({ + queryKey: ['threads', 'stats', selectedMailbox?.id], + predicate: ({ queryKey }) => !queryKey.some(k => typeof k === 'string' && k.includes('label_slug=')) +});
194-199: Delete logic inverted for threadIds.When source.type === 'delete', messages within listed threadIds should be removed, not kept.
Apply:
- newResults = newResults.filter((message: Message) => { - if ((source.metadata.threadIds ?? []).includes(threadId)) return true; - return !(source.metadata.ids ?? []).includes(message.id); - }); + newResults = newResults.filter((message: Message) => { + if ((source.metadata.threadIds ?? []).includes(threadId)) return false; + return !(source.metadata.ids ?? []).includes(message.id); + });Optional: precompute Sets to reduce O(n·m):
+ const idSet = new Set(source.metadata.ids ?? []); + const threadHit = (source.metadata.threadIds ?? []).includes(threadId); - newResults = newResults.filter((message: Message) => { - if ((source.metadata.threadIds ?? []).includes(threadId)) return false; - return !(source.metadata.ids ?? []).includes(message.id); - }); + newResults = newResults.filter((message: Message) => !threadHit && !idSet.has(message.id));
🧹 Nitpick comments (7)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss (2)
18-24: Simplify footer hiding with fewer !important rules.The folded state uses multiple
!importantdeclarations to hide the footer. This is excessive and can create maintenance issues. A singledisplay: noneis sufficient to hide an element.Apply this diff:
.thread-message--folded { .thread-message__header { border-bottom: none; } .thread-message__footer { - height: 0 !important; - padding: 0 !important; - margin: 0 !important; - visibility: hidden; display: none; } }
60-68: Document the pointer-events pattern for maintainability.The pattern of disabling pointer-events on the container div and re-enabling them for interactive elements is clever but non-obvious. Consider adding a comment explaining why this pattern is necessary (to allow click-through to the header-toggle button while keeping header elements interactive).
src/frontend/src/features/layouts/components/thread-view/_index.scss (1)
49-50: Consider documenting the 300px bottom margin rationale.The
margin-bottom: 300pxon the messages list seems arbitrary. If this is intentional spacing for UX purposes (e.g., to allow scrolling the last message away from the bottom), consider adding a comment or using a CSS custom property with a semantic name.src/frontend/src/features/ui/components/contact-chip/index.tsx (1)
22-22: Consider renamingdisplayEmailfor clarity.The prop name
displayEmailis slightly misleading: whenfalse, it still displays the email (just without the name). Whentrue, it displays the full identity format (name + email). Consider renaming todisplayFullIdentity,showNameAndEmail, orexpandedFormatto better convey the behavior.Also applies to: 43-51
src/frontend/src/features/providers/mailbox.tsx (3)
200-208: Confirm: thread-wide updates apply payload to all messages.This maps every message when threadIds contains threadId. Ensure payload is safe for bulk-apply; otherwise restrict to ids only.
If only some fields should be bulk-updated, guard per-field or per-id.
216-217: Count semantics: confirm equals visible results length.Setting count = newResults.length assumes server count === client-visible length. If API’s count is total (e.g., for pagination), keep oldData.data.count instead.
225-233: Apply optimistic updates before invalidation to prevent UI flicker; simplify length checks.
- Reorder
_updateThreadMessagesQueryDatacalls to execute beforeinvalidateQueriesfor both the threads loop and the selected thread block to ensure optimistic updates are visible before refetch triggers.- Simplify
((arr ?? []).length ?? 0)to(arr?.length ?? 0)using optional chaining for cleaner syntax.const invalidateThreadMessages = async (source?: MessageQueryInvalidationSource) => { - await queryClient.invalidateQueries({ queryKey: ['threads', selectedMailbox?.id] }); - if (source && ((source.metadata.threadIds ?? []).length ?? 0) > 0) { + if (source && (source.metadata.threadIds?.length ?? 0) > 0) { source.metadata.threadIds!.forEach(threadId => { if (queryClient.getQueryState(['messages', threadId])) { _updateThreadMessagesQueryData(threadId, source); } }); } + await queryClient.invalidateQueries({ queryKey: ['threads', selectedMailbox?.id] }); if (selectedThread) { - await queryClient.invalidateQueries({ queryKey: ['messages', selectedThread.id] }); - if (source && ((source.metadata.ids ?? []).length ?? 0) > 0) { + if (source && (source.metadata.ids?.length ?? 0) > 0) { _updateThreadMessagesQueryData(selectedThread.id, source); } + await queryClient.invalidateQueries({ queryKey: ['messages', selectedThread.id] }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/frontend/public/locales/common/en-US.json(4 hunks)src/frontend/public/locales/common/fr-FR.json(3 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(3 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/message-reply-form/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss(5 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx(6 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx(7 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx(4 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(3 hunks)src/frontend/src/features/layouts/components/thread-view/provider.tsx(1 hunks)src/frontend/src/features/message/use-read.tsx(1 hunks)src/frontend/src/features/providers/mailbox.tsx(5 hunks)src/frontend/src/features/ui/components/contact-chip/_index.scss(4 hunks)src/frontend/src/features/ui/components/contact-chip/index.tsx(3 hunks)src/frontend/src/features/ui/components/label-badge/_index.scss(2 hunks)src/frontend/src/features/ui/components/label-badge/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss
- src/frontend/src/features/layouts/components/thread-view/provider.tsx
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/src/features/message/use-read.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx
- src/frontend/src/features/forms/components/message-form/index.tsx
- src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/frontend/src/features/providers/mailbox.tsx (2)
src/frontend/src/features/api/gen/models/thread.ts (1)
Thread(15-41)src/frontend/src/features/api/gen/messages/messages.ts (1)
messagesListResponse200(617-620)
src/frontend/src/features/ui/components/label-badge/index.tsx (3)
src/frontend/src/features/api/gen/models/thread_label.ts (1)
ThreadLabel(12-38)src/frontend/src/features/api/gen/labels/labels.ts (2)
useLabelsRemoveThreadsCreate(815-838)useLabelsAddThreadsCreate(707-727)src/frontend/src/features/ui/components/badge/index.tsx (1)
Badge(7-13)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (2)
src/frontend/src/features/api/gen/models/attachment.ts (1)
Attachment(12-29)src/frontend/src/features/utils/unquote-message/index.ts (1)
UnquoteMessage(34-385)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (9)
src/frontend/src/features/layouts/components/thread-view/provider.tsx (1)
useThreadViewContext(93-99)src/frontend/src/features/forms/components/message-form/index.tsx (1)
MessageFormMode(29-29)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(359-365)src/frontend/src/features/api/gen/models/message_recipient.ts (1)
MessageRecipient(14-24)src/frontend/src/features/ui/components/contact-chip/index.tsx (2)
ContactChipDeliveryStatus(12-16)ContactChip(26-81)src/frontend/src/features/ui/components/banner/index.tsx (1)
Banner(17-44)src/frontend/src/features/ui/components/users/user-avatar.tsx (1)
UserAvatar(7-14)src/frontend/src/features/api/utils.ts (1)
getRequestUrl(39-50)src/frontend/src/features/api/gen/messages/messages.ts (1)
getMessagesEmlRetrieveUrl(1066-1068)
src/frontend/src/features/layouts/components/thread-view/index.tsx (8)
src/frontend/src/features/api/gen/models/message.ts (1)
Message(20-57)src/frontend/src/hooks/use-debounce-callback.ts (1)
useDebounceCallback(7-23)src/frontend/src/hooks/use-feature.ts (1)
useFeatureFlag(16-29)src/frontend/src/features/layouts/components/thread-view/provider.tsx (1)
useThreadViewContext(93-99)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx (1)
ActionBar(18-104)src/frontend/src/features/layouts/components/thread-view/components/thread-view-labels-list/index.tsx (1)
ThreadViewLabelsList(11-17)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
ThreadSummary(19-148)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(359-365)
⏰ 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: docker-publish-mta-in / docker-build-push
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: build-front
- GitHub Check: check-api-state
- GitHub Check: lint-back
- GitHub Check: test-back
🔇 Additional comments (25)
src/frontend/public/locales/common/en-US.json (1)
138-138: New translation keys look good.The other three new keys align with the PR objective of message folding feature and use proper English text.
Also applies to: 324-324, 338-338
src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx (1)
29-29: LGTM! Good UX improvement.Adding
placement="right"to the tooltip for the "Close this thread" button makes sense since it's positioned on the left side of the action bar. This ensures the tooltip appears in a natural position to the right of the button rather than potentially being clipped or appearing in an awkward location.src/frontend/src/features/layouts/components/thread-view/components/message-reply-form/index.tsx (1)
25-25: LGTM! Simplifies form closure logic.The change safely removes the draft-dependent conditional by always passing
handleCloseto theonCloseprop. TheMessageFormcomponent properly guards its usage with!draft &&checks (line 651 in message-form/index.tsx) and optional chaining (line 266), so both draft and non-draft closure scenarios work correctly regardless of whetheronCloseis always provided.src/frontend/src/features/ui/components/label-badge/_index.scss (4)
2-9: LGTM: Clean padding delegation to inner wrapper.The relocation of padding control from the container to the inner
.label-badge__labelwrapper is a solid pattern that enables flexible styling variants like the compact mode introduced later.
30-33: LGTM: Hover state properly scoped to linkable badges.The updated selector
& > a.label-badge__label:hoverensures hover effects apply only to linkable badges (anchors), while non-linkable badges (spans) remain static—an appropriate UX distinction.
35-40: LGTM: Compact modifier properly implemented.The
--compactmodifier cleanly reduces vertical padding while maintaining horizontal spacing, following BEM conventions and working correctly with the new wrapper structure.
20-28: LGTM: Remove button styling updated for new layout.The
display: blockandalign-self: stretchproperties ensure the remove button aligns properly within the flex container created by the new wrapper structure.src/frontend/src/features/ui/components/label-badge/index.tsx (5)
6-6: LGTM: Necessary imports added.The Icon components and clsx utility are both actively used in the component for the new compact feature and updated delete button UI.
Also applies to: 14-14
20-20: LGTM: Compact prop properly typed and defaulted.The optional
compactprop with a default value offalseensures backward compatibility while enabling the new compact badge variant.Also applies to: 23-23
68-68: LGTM: Conditional class application is correct.The
clsxusage properly applies thelabel-badge--compactmodifier class when the compact prop is true, aligning with the SCSS implementation.
69-69: LGTM: Label wrapper structure matches SCSS expectations.Both the
Linkandspanelements correctly receive thelabel-badge__labelclass, ensuring consistent padding and layout as defined in the stylesheet.
35-35: LGTM: UI updates are well-implemented.The changes enhance the delete action UI:
isPendingstate properly tracked for the delete mutation and used fordisabledandaria-busystates- The
addLabelMutation(undo) correctly omitsisPendingas undo actions typically don't require loading feedback- Tooltip placement updated to
"right"for better positioning- Icon component properly replaces the previous implementation with appropriate size, type, and conditional spinner rendering
Also applies to: 57-57, 71-71, 78-78
src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
67-95: LGTM! Intersection observer logic is well-implemented.The intersection observer correctly:
- Waits for readiness before observing
- Accounts for sticky header height in the root margin
- Debounces the mark-as-read API calls
- Cleans up on unmount
The dependency array correctly uses
unreadMessageIds.join(",")to detect changes in the ID list.
97-114: Auto-scroll implementation looks correct.The auto-scroll logic properly prioritizes draft messages → unread messages → latest message, and uses
behavior: 'instant'for the initial scroll position (avoiding animation on load).src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (2)
102-111: Verify thread-view container selector is stable.The auto-scroll logic queries
.thread-viewby class name. If multiple thread views could exist in the DOM simultaneously (e.g., in a modal or split view), this selector might target the wrong element. Consider using a ref instead.If there's only ever one
.thread-viewelement in the DOM at a time, this is fine. Otherwise, consider passing a container ref through context or props for more robust targeting.
333-336: LGTM! Message body integrates well with readiness context.The
MessageBodycorrectly receivesisHiddenbased on both folding and readiness state, and calls back to mark the message as ready on load. This enables smooth progressive rendering of thread messages.src/frontend/src/features/ui/components/contact-chip/index.tsx (4)
8-8: Good extensibility improvements.The clsx import and the extension of props with
HTMLAttributes<HTMLDivElement>allow for flexible className composition and standard HTML attribute support, improving component reusability.Also applies to: 22-24
26-26: Props spreading enables flexible composition.The destructuring pattern with
...propsand the use of clsx for className merging allow consumers to pass additional HTML attributes and custom classes to the wrapper. The pattern is sound—any event handlers spread onto the wrapper div will work alongside the button's own handlers.Also applies to: 32-32
35-35: Consistent icon class refactoring.The updated BEM pattern (base
contact-chip__icon+ modifier classes) aligns with the CSS refactoring in_index.scssand improves maintainability.Also applies to: 38-38, 41-42
94-94: Good readability improvement.The explicit parentheses around the condition improve clarity without changing behavior.
src/frontend/src/features/ui/components/contact-chip/_index.scss (4)
33-42: Layout changes properly support text truncation.The switch to
align-items: baselineimproves text-icon alignment, and the added flex properties withmin-width: 0enable proper text truncation behavior for long email addresses or names.
55-70: Verify text truncation behavior in narrow containers.The
flex-shrink: 0on.contact-chip__identity-nameprevents the name from shrinking, which prioritizes name visibility. However, in very narrow containers where both name and email are long, the email may be heavily truncated or completely hidden while the name refuses to shrink. Please test the rendering with long names and emails in constrained widths to ensure acceptable UX.Consider this alternative if name should also truncate when space is limited:
.contact-chip__identity-name { flex-shrink: 1; // Allow name to shrink too min-width: 0; // Enable truncation white-space: nowrap; overflow: hidden; text-overflow: ellipsis; max-width: 60%; // Ensure email gets at least 40% of space }
87-100: Clean icon class refactoring.The consolidation to a base
contact-chip__iconclass with modifier variants follows BEM conventions and aligns with the TypeScript component changes. The consistenttranslateY(2px)adjustment improves icon-text alignment.
2-2: No layout concerns identified—change is safe.The display change from
inline-flextoflexis not a breaking change in this codebase. ContactChips are always rendered within flex containers set toflex-direction: row(either viarecipient-chip-listorflex-rowutility classes), which means both display values produce identical horizontal layouts. The chip's truncation properties (min-width: 0,max-width: 100%) work correctly with the flex parent, and the pseudo-element comma separators confirm horizontal inline flow is preserved.src/frontend/src/features/providers/mailbox.tsx (1)
21-25: Remove field name errors in payload type suggestion; the proposedhas_*fields do not exist in Message type.The review's intent to constrain
payloadto mutable fields and renameidstomessageIdsis reasonable, but the suggested field names are incorrect. The Message type usesis_unread,is_starred,is_trashed,is_archived(nothas_*variants).Current payload usage confirms:
use-read.tsxalready usesis_unreadandread_at, which are the only mutable fields being updated. The suggestedMessagePatchtype should be:type MessagePatch = Partial<Pick<Message, 'is_unread' | 'is_trashed' | 'is_archived' | 'is_starred' | 'read_at' >>;Rename
metadata.idstometadata.messageIdsis valid and requires updates at 4 call sites: use-read.tsx line 24, message-form/index.tsx line 268, mailbox.tsx lines 197, 203, 236.Likely an incorrect or invalid review comment.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
Show resolved
Hide resolved
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
Show resolved
Hide resolved
...ntend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx
Show resolved
Hide resolved
jbpenrath
force-pushed
the
front/fold-message-quotes
branch
2 times, most recently
from
47acb43 to
bbc0a87
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (11)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx (1)
28-28: Use stableattachment.idinstead of composite key.The composite key
attachment-${attachment.name}-${attachment.size}-${attachment.created_at}cannot guarantee uniqueness (multiple attachments with identical name and size uploaded at the same time would collide) and breaks React reconciliation when attachment metadata changes. BothAttachmentandDriveFiletypes have a stable, uniqueidfield available.Replace with:
- {attachments.map((attachment) => <AttachmentItem key={`attachment-${attachment.name}-${attachment.size}-${attachment.created_at}`} attachment={attachment} />)} + {attachments.map((attachment) => <AttachmentItem key={attachment.id} attachment={attachment} />)}src/frontend/src/features/providers/mailbox.tsx (1)
190-220: Fix inverted delete filter logic on line 196.In the delete branch, when
threadIdis found insource.metadata.threadIds, the filter returnstrue(keeping all messages in that thread). This is inverted: deletion by thread ID should remove messages from those threads, not preserve them.Apply this fix:
if (source.type === 'delete') { newResults = newResults.filter((message: Message) => { - if ((source.metadata.threadIds ?? []).includes(threadId)) return true; + if ((source.metadata.threadIds ?? []).includes(threadId)) return false; return !(source.metadata.ids ?? []).includes(message.id); }); }src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
27-28: Sync localSummary when thread or summary changes.
localSummaryis initialized from thesummaryprop but never updated when the user navigates to a different thread or when thesummaryprop changes. This causes stale UI where the old thread's summary persists.Add this effect:
const { t } = useTranslation(); const [localSummary, setLocalSummary] = useState(summary); + + useEffect(() => { + setLocalSummary(summary); + }, [summary, threadId]);src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (2)
287-291: Include resizeIframe in effect deps to avoid stale closure- useEffect(() => { + useEffect(() => { if (!isHidden) { resizeIframe(); } - }, [isHidden]); + }, [isHidden, resizeIframe]);
90-105: Plain-text path loses newlines and treats text as HTMLSplit HTML vs text paths. For text: unquote the raw text, then escape and convert newlines to
.- const sanitizedHtmlBody = useMemo(() => { - const sanitizedContent = domPurify.sanitize(rawHtmlBody || rawTextBody, { - FORBID_TAGS: ['script', 'object', 'iframe', 'embed', 'audio', 'video'], - ADD_ATTR: ['target', 'rel'], - }); - - const unquoteMessage = new UnquoteMessage(sanitizedContent, sanitizedContent, { - mode: 'wrap', - ignoreFirstForward: true, - depth: 0, - }); - - if (rawHtmlBody) return unquoteMessage.getHtml().content; - return unquoteMessage.getText().content; - }, [rawHtmlBody, rawTextBody, domPurify]); + const sanitizedHtmlBody = useMemo(() => { + if (rawHtmlBody) { + const sanitized = domPurify.sanitize(rawHtmlBody, { + FORBID_TAGS: ['script', 'object', 'iframe', 'embed', 'audio', 'video'], + ADD_ATTR: ['target', 'rel'], + }); + return new UnquoteMessage(sanitized, '', { + mode: 'wrap', + ignoreFirstForward: true, + depth: 0, + }).getHtml().content; + } + // Plain text: unquote, escape, preserve newlines + const unquoted = new UnquoteMessage('', rawTextBody, { + ignoreFirstForward: true, + depth: 0, + }).getText().content; + const escaped = unquoted + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, ''') + .replace(/\r?\n/g, '<br>'); + return escaped; + }, [rawHtmlBody, rawTextBody, domPurify]);src/frontend/src/features/utils/unquote-message/index.ts (4)
277-289: Avoid innerHTML; build nodes safely- #createDetailsElement(root: Document): HTMLElement { + #createDetailsElement(root: Document): HTMLElement { const details = root.createElement("details"); details.className = "email-quoted-content"; const summary = root.createElement("summary"); - summary.innerHTML = "<span>…</span>"; + const span = root.createElement("span"); + span.textContent = "…"; + summary.appendChild(span); summary.className = "email-quoted-summary"; summary.dataset.content = i18n.t("Show embedded message"); details.appendChild(summary); return details; }
318-361: Fix grouping: enforce DOM order, adjacency, and drop descendantsPrevents merging non-adjacent siblings and double-wrapping nested nodes.
- // Group consecutive elements together - const groups: HTMLElement[][] = []; - let currentGroup: HTMLElement[] = []; - - elements.forEach((element, index) => { + // Sort by DOM order and drop descendants (keep top-most only) + const sorted = elements.slice().sort((a, b) => { + const pos = a.compareDocumentPosition(b); + if (pos & Node.DOCUMENT_POSITION_FOLLOWING) return -1; + if (pos & Node.DOCUMENT_POSITION_PRECEDING) return 1; + return 0; + }); + const input = sorted.filter((el, _, arr) => !arr.some(other => other !== el && other.contains(el))); + + const groups: HTMLElement[][] = []; + let currentGroup: HTMLElement[] = []; + + input.forEach((element, index) => { if (currentGroup.length === 0) { currentGroup.push(element); } else { // Check if this element is adjacent to the previous one const prevElement = currentGroup[currentGroup.length - 1]; - const areAdjacent = - prevElement.nextElementSibling === element || - prevElement.parentElement === element.parentElement; + const areAdjacent = prevElement.nextElementSibling === element; if (areAdjacent) { currentGroup.push(element); } else { // Start a new group groups.push([...currentGroup]); currentGroup = [element]; } } // Push the last group - if (index === elements.length - 1) { + if (index === input.length - 1) { groups.push([...currentGroup]); } }); - - // Wrap each group in a details element + // Wrap each group in a details element groups.forEach((group) => { if (group.length === 0) return; const details = this.#createDetailsElement(document); const firstElement = group[0]; // Insert details before the first element firstElement.parentElement?.insertBefore(details, firstElement); // Move all elements into the details - group.forEach((element) => { - details.appendChild(element); - }); + group.forEach((element) => details.appendChild(element)); });
124-132: ignoreFirstForward preserves none; should preserve only the firstTrack the first forward and exclude only subsequent ones.
- matchedElements.forEach((element) => { - if ( - !this.#options.ignoreFirstForward || - !this.#isForwardContent(element) - ) { - elementsToHandle.add(element); - } - }); + let forwardPreserved = false; + matchedElements.forEach((element) => { + const isFwd = this.#isForwardContent(element); + if (this.#options.ignoreFirstForward && isFwd && !forwardPreserved) { + forwardPreserved = true; // keep the first forward + return; + } + elementsToHandle.add(element); + });Please add a test ensuring only the first forward is preserved (see my test suggestion in index.test.ts).
230-237: Forward detection should inspect only the first line and reset regex state- #isForwardContent(element: HTMLElement): boolean { - // Get element as text and break it down into lines to keep only the first line - const text = element.textContent?.trim() || ''; - return FORWARD_PATTERNS.some((pattern) => { - // Improve pattern matching by checking if the text starts with the pattern - return text.match(pattern); - }); - } + #isForwardContent(element: HTMLElement): boolean { + const text = element.textContent?.trim() || ""; + const firstLine = text.split(/\r?\n/, 1)[0] || ""; + return FORWARD_PATTERNS.some((pattern) => { + pattern.lastIndex = 0; + return pattern.test(firstLine); + }); + }src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (2)
98-100: Add missing dependency to useEffect.The
useEffecton line 98 updatesisFoldedbased onmessage.is_unreadbut doesn't include it in the dependency array. This was flagged in a previous review but remains unresolved. When a message's unread status changes, the folded state won't update correctly.Apply this diff:
useEffect(() => { setIsFolded(!isLatest && !message.is_unread && !draftMessage?.is_draft); - }, [isLatest, draftMessage?.is_draft]) + }, [isLatest, message.is_unread, draftMessage?.is_draft]);
157-157: Fix UserAvatar prop mismatch.The
UserAvatarcomponent expects auserobject with afull_nameproperty (as shown in the relevant code snippets), but the code is passing afullNameprop directly. This was flagged in a previous review but remains unresolved. The avatar will not render correctly.Apply this diff:
- <UserAvatar fullName={message.sender.name || message.sender.email} /> + <UserAvatar user={{ full_name: message.sender.name || message.sender.email }} />
🧹 Nitpick comments (9)
src/frontend/src/features/ui/components/contact-chip/_index.scss (1)
33-36: Remove redundantflex-direction: row.Line 35 explicitly sets
flex-direction: row, which is the default for flex containers. Removing it reduces noise without changing behavior.display: flex; align-items: baseline; - flex-direction: row; flex-wrap: nowrap;src/frontend/src/features/layouts/components/thread-view/provider.tsx (3)
33-43: Use functional setState for membership check to avoid stale readsCheck membership and update within the same updater to prevent TOCTOU with concurrent renders.
- const setMessageReadiness = (messageId: string, isReady: boolean) => { - if (messagesReadiness.has(messageId)) { - setMessagesReadiness(prev => { - const newMap = new Map(prev); - newMap.set(messageId, isReady); - return newMap; - }); - } else { - console.warn(`Message ${messageId} not registered in the readiness state context`); - } - } + const setMessageReadiness = (messageId: string, isReady: boolean) => { + setMessagesReadiness(prev => { + if (!prev.has(messageId)) { + console.warn(`Message ${messageId} not registered in the readiness state context`); + return prev; + } + const next = new Map(prev); + next.set(messageId, isReady); + return next; + }); + }
23-25: Clarify isReady semantics when there are zero messagesevery([]) is true; do you want isReady=true for an empty thread? If not, gate by messageIds.length > 0.
- const isReady = useMemo(() => { - return Array.from(messagesReadiness.values()).every((isReady) => isReady === true); - }, [messagesReadiness]); + const isReady = useMemo(() => { + return messageIds.length > 0 && + Array.from(messagesReadiness.values()).every(Boolean); + }, [messagesReadiness, messageIds.length]);
69-85: Effect closes over messagesReadiness; prefer functional update and simpler depsSafer and clearer to compute next state from prev inside setState; depend on messageIds identity instead of join.
- useEffect(() => { - const currentMessageIds = Array.from(messagesReadiness.keys()); - const newMessageIds = messageIds.filter((id) => !currentMessageIds.includes(id)); - const removedMessageIds = currentMessageIds.filter((id) => !messageIds.includes(id)); - if (newMessageIds.length > 0 || removedMessageIds.length > 0) { - const nextState = new Map(messagesReadiness); - for (const messageId of newMessageIds) { - nextState.set(messageId, false); - } - for (const messageId of removedMessageIds) { - nextState.delete(messageId); - } - setMessagesReadiness(nextState); - } - }, [messageIds.join(',')]); + useEffect(() => { + setMessagesReadiness(prev => { + const currentIds = Array.from(prev.keys()); + const adds = messageIds.filter(id => !currentIds.includes(id)); + const dels = currentIds.filter(id => !messageIds.includes(id)); + if (adds.length === 0 && dels.length === 0) return prev; + const next = new Map(prev); + adds.forEach(id => next.set(id, false)); + dels.forEach(id => next.delete(id)); + return next; + }); + }, [messageIds]);src/frontend/src/features/utils/unquote-message/index.test.ts (1)
824-861: Add test: preserve only the first forward when ignoreFirstForward=trueCovers regression where all forwards could be skipped or all preserved.
describe("Options - ignoreFirstForward", () => { + it("should preserve only the first forward when multiple are present", () => { + const html = ` + <div>New message</div> + <blockquote data-type="quote-separator">Forwarded Message A</blockquote> + <blockquote data-type="quote-separator">Forwarded Message B</blockquote> + <div class="gmail_quote"><blockquote class="gmail_quote">Quoted</blockquote></div> + `; + const result = new UnquoteMessage(html, "", { ignoreFirstForward: true }).getHtml(); + expect(result.hadQuotes).toBe(true); + expect(result.content).toContain("Forwarded Message A"); + expect(result.content).not.toContain("Forwarded Message B"); + });src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (2)
109-113: Prefer removing and rely on per-link attributesBase target affects in-document hash links; your sanitize hook already sets target/rel per anchor.
- <base target="_blank">
255-273: postMessage wiring appears unusedNo script in srcDoc posts 'iframe-loaded' or handles 'add-toggle-listeners'. Either inject a minimal script or remove this listener.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss (2)
18-24: Consider alternatives to !important overrides.Using multiple
!importantdeclarations (lines 19-23) to force hide the footer makes the styles harder to maintain and override. Consider using higher specificity selectors or restructuring the CSS to avoid the need for!important.For example, you could use a more specific selector:
-.thread-message--folded { +.thread-message.thread-message--folded { .thread-message__header { border-bottom: none; } .thread-message__footer { - height: 0 !important; - padding: 0 !important; - margin: 0 !important; + height: 0; + padding: 0; + margin: 0; visibility: hidden; display: none; } }
60-68: Pointer-events manipulation may be fragile.The approach of disabling pointer-events on the container (line 62) and then selectively re-enabling them for interactive elements (lines 65-67) works, but can be fragile if new interactive elements are added without updating the selector list. Consider documenting this pattern or using a more maintainable approach like a CSS class for interactive elements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
Makefile(1 hunks)src/frontend/public/locales/common/en-US.json(4 hunks)src/frontend/public/locales/common/fr-FR.json(4 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(3 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/message-reply-form/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss(5 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx(6 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx(6 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx(4 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(3 hunks)src/frontend/src/features/layouts/components/thread-view/provider.tsx(1 hunks)src/frontend/src/features/message/use-read.tsx(1 hunks)src/frontend/src/features/providers/mailbox.tsx(6 hunks)src/frontend/src/features/ui/components/contact-chip/_index.scss(4 hunks)src/frontend/src/features/ui/components/contact-chip/index.tsx(3 hunks)src/frontend/src/features/ui/components/label-badge/_index.scss(2 hunks)src/frontend/src/features/ui/components/label-badge/index.tsx(4 hunks)src/frontend/src/features/utils/unquote-message/constants.ts(1 hunks)src/frontend/src/features/utils/unquote-message/handlers.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.test.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.ts(1 hunks)src/frontend/src/features/utils/unquote-message/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx
- src/frontend/src/features/utils/unquote-message/constants.ts
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss
- src/frontend/public/locales/common/en-US.json
- src/frontend/src/features/layouts/components/thread-view/components/message-reply-form/index.tsx
- src/frontend/src/features/layouts/components/thread-view/_index.scss
- src/frontend/src/features/ui/components/contact-chip/index.tsx
- src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
src/frontend/src/features/utils/unquote-message/handlers.ts (3)
src/frontend/src/features/utils/unquote-message/types.ts (1)
CustomHandler(9-19)src/frontend/src/features/utils/unquote-message/index.ts (2)
element(230-237)element(295-310)src/frontend/src/features/utils/unquote-message/constants.ts (2)
REPLY_PATTERNS(41-146)FORWARD_PATTERNS(5-34)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx (1)
AttachmentItem(27-97)
src/frontend/src/features/providers/mailbox.tsx (2)
src/frontend/src/features/api/gen/models/thread.ts (1)
Thread(15-41)src/frontend/src/features/api/gen/messages/messages.ts (1)
messagesListResponse200(617-620)
src/frontend/src/features/utils/unquote-message/types.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (2)
UnquoteResult(97-172)UnquoteResult(177-225)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (2)
src/frontend/src/features/api/gen/models/attachment.ts (1)
Attachment(12-29)src/frontend/src/features/utils/unquote-message/index.ts (1)
UnquoteMessage(34-385)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (8)
src/frontend/src/features/layouts/components/thread-view/provider.tsx (1)
useThreadViewContext(93-99)src/frontend/src/features/forms/components/message-form/index.tsx (1)
MessageFormMode(29-29)src/frontend/src/features/api/gen/models/message_recipient.ts (1)
MessageRecipient(14-24)src/frontend/src/features/ui/components/contact-chip/index.tsx (2)
ContactChipDeliveryStatus(12-16)ContactChip(26-81)src/frontend/src/features/ui/components/banner/index.tsx (1)
Banner(17-44)src/frontend/src/features/ui/components/users/user-avatar.tsx (1)
UserAvatar(7-14)src/frontend/src/features/api/utils.ts (1)
getRequestUrl(39-50)src/frontend/src/features/api/gen/messages/messages.ts (1)
getMessagesEmlRetrieveUrl(1066-1068)
src/frontend/src/features/ui/components/label-badge/index.tsx (3)
src/frontend/src/features/api/gen/models/thread_label.ts (1)
ThreadLabel(12-38)src/frontend/src/features/api/gen/labels/labels.ts (2)
useLabelsRemoveThreadsCreate(815-838)useLabelsAddThreadsCreate(707-727)src/frontend/src/features/ui/components/badge/index.tsx (1)
Badge(7-13)
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (2)
html(242-247)UnquoteMessage(34-385)
src/frontend/src/features/layouts/components/thread-view/index.tsx (9)
src/frontend/src/features/api/gen/models/message.ts (1)
Message(20-57)src/frontend/src/features/api/gen/models/thread.ts (1)
Thread(15-41)src/frontend/src/hooks/use-debounce-callback.ts (1)
useDebounceCallback(7-23)src/frontend/src/hooks/use-feature.ts (1)
useFeatureFlag(16-29)src/frontend/src/features/layouts/components/thread-view/provider.tsx (1)
useThreadViewContext(93-99)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx (1)
ActionBar(18-104)src/frontend/src/features/layouts/components/thread-view/components/thread-view-labels-list/index.tsx (1)
ThreadViewLabelsList(11-17)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
ThreadSummary(19-148)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(362-368)
src/frontend/src/features/utils/unquote-message/index.ts (3)
src/frontend/src/features/utils/unquote-message/types.ts (2)
UnquoteOptions(26-48)UnquoteResult(55-62)src/frontend/src/features/utils/unquote-message/handlers.ts (1)
HANDLERS(13-318)src/frontend/src/features/utils/unquote-message/constants.ts (2)
REPLY_PATTERNS(41-146)FORWARD_PATTERNS(5-34)
🪛 ast-grep (0.39.6)
src/frontend/src/features/utils/unquote-message/index.ts
[warning] 281-281: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 281-281: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ 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: docker-publish-mta-in / docker-build-push
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: check-api-state
- GitHub Check: build-front
- GitHub Check: lint-back
- GitHub Check: test-back
🔇 Additional comments (22)
Makefile (1)
245-248: ✅ Good pattern alignment for frontend test forwarding.The changes correctly implement the same argument-forwarding pattern already used by
back-testandback-test-parallel(lines 235-243). This enables developers to pass test paths/filters directly: e.g.,make front-test src/path/to/test.js. The implementation is backwards compatible and maintains consistency across the Makefile.Also applies to: 250-253
src/frontend/src/features/ui/components/contact-chip/_index.scss (2)
87-100: LGTM: Icon styling is consistent and correct.The base
.contact-chip__iconclass applies a vertical transform to align icons, and the modifier classes cleanly separate color concerns. The structure follows BEM conventions and avoids duplication.
55-70: The min-width asymmetry is correct and intentional; the review concern is incorrect.Based on CSS Flexbox specifications, the initial setting on flex items is min-width: auto, which means the length of text that is not wrapping sets a minimum size for parent flex items. However, one way to enable flex items to shrink past their content is to set a flex item to min-width: 0.
The key insight is that
min-width: 0is only necessary for flex items that can shrink (flex-shrink > 0). Since.contact-chip__identity-namehasflex-shrink: 0, it will never shrink regardless of the min-width value. Therefore, it doesn't requiremin-width: 0. In contrast,.contact-chip__identity-emailusesflex: 0 1 auto(flex-shrink: 1), so it correctly includesmin-width: 0to enable proper text truncation.Both classes already have the necessary properties for ellipsis to work:
overflow: hidden,text-overflow: ellipsis,white-space: nowrap, andmax-width: 100%. The asymmetry is intentional—the name preserves its width while the email shrinks first when space is constrained, which aligns with the component's structure where identity-email is conditionally rendered.Likely an incorrect or invalid review comment.
src/frontend/src/features/ui/components/label-badge/_index.scss (2)
36-40: Compact modifier implementation looks good.The
--compactmodifier cleanly reduces padding from0.5rem 0.5remto0.25rem 0.5rem, maintaining horizontal spacing while reducing vertical space. This follows BEM conventions and integrates well with the wrapper structure.
11-13: No action required; :has() usage is consistent with project standards.The codebase already uses
:has()in 7+ locations across 6 SCSS files (message-form, search-input, dropdown-button, thread-item, label-badge, modal-create-update-mailbox) with no feature detection fallbacks. This indicates the project has already accepted:has()as a baseline requirement, consistent with Next.js 15.3.4 and Node >= 22.0.0 targets. The compatibility concern in the original comment, while technically accurate, is redundant and misaligned with the established project pattern.src/frontend/src/features/ui/components/label-badge/index.tsx (5)
6-6: Import additions align with component updates.The Icon components and clsx imports support the new compact mode and icon system changes. These imports are properly used throughout the component.
Also applies to: 14-14
20-20: Compact prop properly typed.The
compact?: booleanprop is correctly typed as optional, which aligns with the default value offalsein the component signature.
68-68: Class name application correctly implements compact mode.The
clsxusage cleanly applies thelabel-badge--compactmodifier class when the compact prop is true, matching the SCSS modifier structure.
69-69: Wrapper class names match SCSS structure.The
label-badge__labelclass is consistently applied to both the Link and span elements, aligning with the SCSS wrapper structure introduced in the stylesheet.
78-78: Verify Icon component visual consistency against design requirements.The Icon component is from the external library
@gouvfr-lasuite/ui-kit. While thesize={IconSize.SMALL}andtype={IconType.OUTLINED}props follow established patterns in the codebase, other "close" icon usages in the same codebase (chip.tsx, attachment-item.tsx, thread-action-bar/index.tsx) use simpler instantiation without explicit size/type parameters.Manually verify that:
- The rendered "close" icon appearance matches your design expectations
- The
SMALLsize is visually appropriate in the delete button context- The icon appearance is consistent with the previous
material-iconsimplementation if this was a recent replacementsrc/frontend/public/locales/common/fr-FR.json (1)
138-138: LGTM!The French translations for the new UI strings are accurate and consistent with existing locale patterns.
Also applies to: 279-279, 326-326, 340-340
src/frontend/src/features/providers/mailbox.tsx (1)
248-251: Predicate improvement looks good.The updated predicate now directly inspects the last queryKey element rather than concatenating the entire array, reducing false positives.
src/frontend/src/features/message/use-read.tsx (1)
21-26: Structured cache invalidation is correct.The updated
onSuccesshandler properly passes a structuredMessageQueryInvalidationSourcetoinvalidateThreadMessages, enabling targeted cache updates without full refetches.src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx (1)
29-29: LGTM!Adding explicit tooltip placement improves the UX by ensuring the tooltip doesn't obscure adjacent UI elements.
src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
134-142: Button with visible text is a good UX improvement.Replacing the icon-only button with a labeled "Summarize" button improves discoverability and accessibility.
src/frontend/src/features/utils/unquote-message/handlers.ts (1)
13-86: Handler ordering and selector patterns are well-structured.The handlers correctly prioritize specific patterns (Mozilla, Gmail) before generic ones (blockquote cite), and use appropriate DOM traversal with guard conditions in most cases.
src/frontend/src/features/forms/components/message-form/index.tsx (1)
266-273: Draft deletion flow is correctly implemented.The updated flow properly:
- Calls
onCloseto dismiss the form- Uses structured invalidation payload for cache updates
- Conditionally unselects the thread only when
has_draft=1is present in the URLsrc/frontend/src/features/layouts/components/thread-view/index.tsx (2)
36-40: Verify the reduced debounce delay.The debounce delay was reduced from 300ms to 150ms. This will cause messages to be marked as read more quickly when scrolling. Ensure this change is intentional and aligns with the desired user experience.
97-114: Auto-scroll logic looks good.The auto-scroll prioritization (draft → unread → latest) is well-structured and handles the different message states appropriately.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (3)
102-111: Smooth scroll implementation looks good.The smooth scroll effect correctly waits for readiness and only triggers when the reply form is newly opened, preventing unnecessary scrolling during initial render.
119-126: Header toggle button implementation is correct.The toggle button is properly disabled for the latest message and has appropriate ARIA attributes for accessibility.
328-336: Message body visibility handling is well-implemented.The
isHiddenprop correctly combines the folded state with message readiness, and theonLoadcallback properly updates the context when the message finishes loading.
Currently threads are hard to read mainly because embedded replies and other kind of messages are not collapsed like other mail application does. So we implement a logic to detect empirically those kind of message and wrap them into a details element.
Rework the thread view component to greatly improve layout and usability. - Remove the message subject and put ahead the sender identity instead - Collapse read messages by default - On thread opening, scroll to the most relevant message (last unread, with draft or the default) - Smoothly scroll to the message composer when the user open the reply form - Fix several bugs found on the road
jbpenrath
force-pushed
the
front/fold-message-quotes
branch
from
bbc0a87 to
1e30a04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx (1)
28-28: Trailing "y" typo removed, but composite key still problematic—preferattachment.id.Good news: the trailing "y" has been removed. However, the composite key
attachment-${attachment.name}-${attachment.size}-${attachment.created_at}still suffers from the issues raised in previous reviews:
- Not guaranteed unique: Multiple attachments with identical name, size, and timestamp will collide.
- Potentially undefined fields:
created_atmay not always be populated, leading to keys likeattachment-file.pdf-1024-undefined.- Breaks reconciliation: If attachment metadata changes (e.g., timestamp updated), React treats it as a new item, destroying and recreating the component unnecessarily.
Both
AttachmentandDriveFilehave a stable, uniqueidfield (UUID). Use it directly:-{attachments.map((attachment) => <AttachmentItem key={`attachment-${attachment.name}-${attachment.size}-${attachment.created_at}`} attachment={attachment} />)} +{attachments.map((attachment) => <AttachmentItem key={attachment.id} attachment={attachment} />)}src/frontend/src/features/utils/unquote-message/index.ts (4)
276-288: Avoid innerHTML for creating static content.While the hardcoded
"<span>…</span>"doesn't pose an actual XSS risk, usingtextContentandappendChildis better practice and was suggested in a previous review:- const summary = root.createElement("summary"); - summary.innerHTML = "<span>…</span>"; + const summary = root.createElement("summary"); + const span = root.createElement("span"); + span.textContent = "…"; + summary.appendChild(span);
124-132: ignoreFirstForward still excludes all forwards, not just preserving the first.The current logic
!this.#options.ignoreFirstForward || !this.#isForwardContent(element)still skips every forward element when the flag is true, rather than preserving only the first forward and removing subsequent ones as intended.This matches the previously raised issue that was marked as addressed. Please verify the fix was properly applied.
229-236: Forward detection still matches entire text instead of first line only.The comment states the intent to check only the first line, but the implementation still matches against the entire element text. This was flagged in a previous review.
The previously suggested fix to extract the first line and reset regex state should be applied.
317-361: Grouping logic incorrectly treats same-parent elements as adjacent.The condition
prevElement.parentElement === element.parentElementgroups any siblings together, not just consecutive ones. This was identified as a major issue in a previous review.The suggested fix includes:
- Sort elements by DOM order
- Check only
prevElement.nextElementSibling === elementfor adjacency- Filter out elements that are descendants of others
Please apply the previously suggested fix for proper consecutive grouping.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1)
287-291: Add resizeIframe to useEffect dependency array.The
useEffectcallsresizeIframebut doesn't include it in the dependency array, which can cause stale closures:useEffect(() => { if (!isHidden) { resizeIframe(); } -}, [isHidden]); +}, [isHidden, resizeIframe]);src/frontend/src/features/providers/mailbox.tsx (1)
190-220: Critical: Delete logic inverted on line 196.The delete filter at line 196 returns
true(keeps messages) whenthreadIdis insource.metadata.threadIds, which inverts the intended deletion behavior. When deleting by thread ID, all messages in that thread should be removed, not preserved.Apply this fix:
if (source.type === 'delete') { newResults = newResults.filter((message: Message) => { - if ((source.metadata.threadIds ?? []).includes(threadId)) return true; + if ((source.metadata.threadIds ?? []).includes(threadId)) return false; return !(source.metadata.ids ?? []).includes(message.id); }); }This ensures messages are excluded (return
false) when their thread is in the deletion list, making the logic consistent with the delete operation's intent.src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (2)
98-100: Add missing dependency to prevent stale folded state.The effect at line 99 uses
message.is_unreadto computeisFolded, but the dependency array on line 100 omitsmessage.is_unread. This can cause the folded state to become stale when a message's unread status changes.useEffect(() => { setIsFolded(!isLatest && !message.is_unread && !draftMessage?.is_draft); - }, [isLatest, draftMessage?.is_draft]); + }, [isLatest, message.is_unread, draftMessage?.is_draft]);
157-157: Fix UserAvatar prop mismatch.The
UserAvatarcomponent expects auserobject with afull_nameproperty (seesrc/frontend/src/features/ui/components/users/user-avatar.tsxlines 6-13), but the code passes afullNameprop directly. This mismatch will prevent the avatar from rendering correctly.- <UserAvatar fullName={message.sender.name || message.sender.email} /> + <UserAvatar user={{ full_name: message.sender.name || message.sender.email }} />
🧹 Nitpick comments (5)
src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx (1)
32-34: Consider simplifying the onSuccess handlers.The arrow function wrappers around
invalidateThreadMessages()appear unnecessary since no arguments are being passed. You could simplify to direct references:- const { mutate: removeThreadAccess } = useThreadsAccessesDestroy({ mutation: { onSuccess: () => invalidateThreadMessages() } }); - const { mutate: createThreadAccess } = useThreadsAccessesCreate({ mutation: { onSuccess: () => invalidateThreadMessages() } }); - const { mutate: updateThreadAccess } = useThreadsAccessesUpdate({ mutation: { onSuccess: () => invalidateThreadMessages() } }); + const { mutate: removeThreadAccess } = useThreadsAccessesDestroy({ mutation: { onSuccess: invalidateThreadMessages } }); + const { mutate: createThreadAccess } = useThreadsAccessesCreate({ mutation: { onSuccess: invalidateThreadMessages } }); + const { mutate: updateThreadAccess } = useThreadsAccessesUpdate({ mutation: { onSuccess: invalidateThreadMessages } });This assumes
invalidateThreadMessagesdoesn't need to avoid receiving mutation callback parameters.src/frontend/src/features/layouts/components/thread-view/provider.tsx (2)
54-60: Consider removing messagesReadiness from useMemo dependencies.Including
messagesReadinessin the context memo dependencies causes the entire context to update whenever readiness state changes, which triggers re-renders in all consumers. Since the functions (isMessageReady,setMessageReadiness,reset) don't close overmessagesReadinessdirectly (they use the state setter), you might not need it:const context = useMemo(() => ({ isReady, isMessageReady, setMessageReadiness, reset, - messagesReadiness, -}), [isReady, setMessageReadiness, isMessageReady, reset, messagesReadiness]); +}), [isReady, isMessageReadiness, setMessageReadiness, reset]);However, if consumers need reactive updates when individual message readiness changes, keep it as is.
65-80: Verify messageIds.join(',') dependency pattern.Using
messageIds.join(',')as a dependency works but has edge cases (e.g.,['a,b']vs['a', 'b']). Consider using a ref with manual comparison or accepting the array directly:-}, [messageIds.join(',')]); +}, [messageIds]);React 19's improved dependency checking should handle the array properly. If you prefer the string approach for performance, add a comment explaining the choice.
src/frontend/src/features/providers/mailbox.tsx (1)
248-251: Strengthen predicate with type guard for robustness.The predicate assumes the last queryKey element is a string, which could throw if the structure differs. Adding a type check would make this more defensive.
await queryClient.invalidateQueries({ queryKey: ['threads', 'stats', selectedMailbox?.id], - predicate: ({ queryKey }) => !(queryKey[queryKey.length - 1] as string).startsWith('label_slug=') + predicate: ({ queryKey }) => { + const lastKey = queryKey[queryKey.length - 1]; + return typeof lastKey !== 'string' || !lastKey.startsWith('label_slug='); + } });src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
97-114: Consider extracting hard-coded scroll offset.The scroll offset
225is hard-coded here and also appears inthread-message/index.tsxline 108. Since the sticky container height is calculated dynamically at line 70, consider either:
- Reusing
stickyContainerRef.current?.getBoundingClientRect().heightfor consistency, or- Extracting the offset as a named constant if it represents a fixed layout value.
+const SCROLL_OFFSET = 225; + const ThreadViewComponent = ({ messages, mailboxId, thread, showTrashedMessages, setShowTrashedMessages, searchParams }: ThreadViewComponentProps) => { // ... const el = document.querySelector<HTMLElement>(selector); if (el) { - rootRef.current?.scrollTo({ top: el.offsetTop - 225, behavior: 'instant' }); + rootRef.current?.scrollTo({ top: el.offsetTop - SCROLL_OFFSET, behavior: 'instant' }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
Makefile(1 hunks)src/frontend/public/locales/common/en-US.json(4 hunks)src/frontend/public/locales/common/fr-FR.json(4 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(3 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/message-reply-form/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss(5 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx(6 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx(6 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss(2 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx(4 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(3 hunks)src/frontend/src/features/layouts/components/thread-view/provider.tsx(1 hunks)src/frontend/src/features/message/use-read.tsx(1 hunks)src/frontend/src/features/providers/mailbox.tsx(6 hunks)src/frontend/src/features/ui/components/contact-chip/_index.scss(4 hunks)src/frontend/src/features/ui/components/contact-chip/index.tsx(3 hunks)src/frontend/src/features/ui/components/label-badge/_index.scss(2 hunks)src/frontend/src/features/ui/components/label-badge/index.tsx(4 hunks)src/frontend/src/features/utils/unquote-message/constants.ts(1 hunks)src/frontend/src/features/utils/unquote-message/handlers.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.test.ts(1 hunks)src/frontend/src/features/utils/unquote-message/index.ts(1 hunks)src/frontend/src/features/utils/unquote-message/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/frontend/src/features/utils/unquote-message/handlers.ts
- src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx
- src/frontend/src/features/utils/unquote-message/types.ts
- src/frontend/src/features/utils/unquote-message/constants.ts
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx
- src/frontend/src/features/forms/components/message-form/index.tsx
- src/frontend/src/features/ui/components/label-badge/_index.scss
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- Makefile
- src/frontend/src/features/layouts/components/thread-view/components/message-reply-form/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/_index.scss
🧰 Additional context used
🧬 Code graph analysis (10)
src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx (1)
src/frontend/src/features/api/gen/thread-access/thread-access.ts (3)
useThreadsAccessesDestroy(787-807)useThreadsAccessesCreate(364-392)useThreadsAccessesUpdate(506-536)
src/frontend/src/features/ui/components/label-badge/index.tsx (3)
src/frontend/src/features/api/gen/models/thread_label.ts (1)
ThreadLabel(12-38)src/frontend/src/features/api/gen/labels/labels.ts (2)
useLabelsRemoveThreadsCreate(815-838)useLabelsAddThreadsCreate(707-727)src/frontend/src/features/ui/components/badge/index.tsx (1)
Badge(7-13)
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
src/frontend/src/features/utils/unquote-message/index.ts (2)
html(241-246)UnquoteMessage(34-384)
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
src/frontend/src/features/ui/components/label-badge/index.tsx (1)
LabelBadge(23-85)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (2)
src/frontend/src/features/api/gen/models/attachment.ts (1)
Attachment(12-29)src/frontend/src/features/utils/unquote-message/index.ts (1)
UnquoteMessage(34-384)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsx (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx (1)
AttachmentItem(27-97)
src/frontend/src/features/layouts/components/thread-view/index.tsx (9)
src/frontend/src/features/api/gen/models/message.ts (1)
Message(20-57)src/frontend/src/features/api/gen/models/thread.ts (1)
Thread(15-41)src/frontend/src/hooks/use-debounce-callback.ts (1)
useDebounceCallback(7-23)src/frontend/src/hooks/use-feature.ts (1)
useFeatureFlag(16-29)src/frontend/src/features/layouts/components/thread-view/provider.tsx (1)
useThreadViewContext(89-95)src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx (1)
ActionBar(18-104)src/frontend/src/features/layouts/components/thread-view/components/thread-view-labels-list/index.tsx (1)
ThreadViewLabelsList(11-17)src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx (1)
ThreadSummary(19-148)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(362-368)
src/frontend/src/features/providers/mailbox.tsx (2)
src/frontend/src/features/api/gen/models/thread.ts (1)
Thread(15-41)src/frontend/src/features/api/gen/messages/messages.ts (1)
messagesListResponse200(617-620)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (9)
src/frontend/src/features/layouts/components/thread-view/provider.tsx (1)
useThreadViewContext(89-95)src/frontend/src/features/forms/components/message-form/index.tsx (1)
MessageFormMode(29-29)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(362-368)src/frontend/src/features/api/gen/models/message_recipient.ts (1)
MessageRecipient(14-24)src/frontend/src/features/ui/components/contact-chip/index.tsx (2)
ContactChipDeliveryStatus(12-16)ContactChip(26-81)src/frontend/src/features/ui/components/banner/index.tsx (1)
Banner(17-44)src/frontend/src/features/ui/components/users/user-avatar.tsx (1)
UserAvatar(7-14)src/frontend/src/features/api/utils.ts (1)
getRequestUrl(39-50)src/frontend/src/features/api/gen/messages/messages.ts (1)
getMessagesEmlRetrieveUrl(1066-1068)
src/frontend/src/features/utils/unquote-message/index.ts (3)
src/frontend/src/features/utils/unquote-message/types.ts (2)
UnquoteOptions(26-48)UnquoteResult(55-62)src/frontend/src/features/utils/unquote-message/handlers.ts (1)
HANDLERS(13-318)src/frontend/src/features/utils/unquote-message/constants.ts (2)
REPLY_PATTERNS(41-146)FORWARD_PATTERNS(5-34)
🪛 ast-grep (0.39.6)
src/frontend/src/features/utils/unquote-message/index.ts
[warning] 280-280: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 280-280: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: summary.innerHTML = "…"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ 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). (5)
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: check-api-state
- GitHub Check: build-front
- GitHub Check: test-back
- GitHub Check: lint-back
🔇 Additional comments (35)
src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
88-88: LGTM! Compact labels improve thread list readability.The addition of the
compactprop to LabelBadge enables more space-efficient label rendering in the thread list, which aligns with the PR's broader goal of improving thread readability.src/frontend/src/features/ui/components/label-badge/index.tsx (4)
6-6: LGTM! Clean import additions for compact mode.The Icon components from the UI kit and clsx utility are appropriately imported to support the new compact mode feature and icon standardization.
Also applies to: 14-14
20-20: LGTM! Proper implementation of compact prop.The compact prop is correctly typed as optional and given a sensible default value, ensuring backward compatibility for existing usage.
Also applies to: 23-23
35-35: LGTM! Improved naming and cleanup.The rename of
isPendingtoisDeletingLabelon line 35 improves clarity. The trailing comma on line 57 suggests cleanup of an unused variable, which is appropriate.Also applies to: 57-57
68-69: LGTM! Well-executed UI refinements.The rendering updates are well-implemented:
- Conditional compact styling using
clsxis clean- Consistent
label-badge__labelclassName enables targeted styling- Tooltip placement improvement enhances UX
- Icon component usage follows UI kit patterns correctly
- Accessibility attributes properly maintained
Also applies to: 71-71, 78-78
src/frontend/public/locales/common/fr-FR.json (1)
138-138: LGTM! French translations are accurate.The four new localization keys are correctly translated and properly support the message folding/unfolding feature.
Also applies to: 279-279, 326-326, 340-340
src/frontend/public/locales/common/en-US.json (1)
138-138: LGTM! English translations are correct.All four localization keys are properly translated to English. The previous issue with French text in the "Summarize" key (mentioned in past review comments) has been resolved.
Also applies to: 279-279, 326-326, 340-340
src/frontend/src/features/ui/components/contact-chip/index.tsx (6)
8-8: LGTM! Type extensions are well-designed.The addition of
clsxfor className merging, the new optional propsdisplayEmailandisUser, and spreadingHTMLAttributes<HTMLDivElement>all follow React best practices and enable proper component composition.Also applies to: 22-24
26-26: LGTM! Component signature is backward compatible.The new props have sensible defaults (
displayEmail = false,isUser = false) that maintain backward compatibility while enabling new functionality.
32-32: LGTM! Props spreading enables composition.Using
clsxto merge classNames and spreading props onto the wrapper div follows React composition patterns and allows consumers to pass additional HTML attributes.
35-35: LGTM! Icon classes follow BEM conventions.The updated icon class structure uses a shared base class (
.contact-chip__icon) with BEM modifiers (--warning,--error), which improves maintainability and aligns with the SCSS changes.Also applies to: 38-38, 41-42
43-52: LGTM! Email display logic correctly handles all cases.The conditional rendering properly handles the display of identity name and email. The past critical issue where the email wouldn't display when
isUser=truebutcontact.namewas falsy has been resolved—line 46 now correctly checks(contact.name || isUser), ensuring the email is always shown alongside "You" whenisUseris true.
94-94: LGTM! Explicit parentheses improve clarity.Adding parentheses around the OR condition
(status.timestamp || status.message)removes any ambiguity in operator precedence and makes the intent explicit.src/frontend/src/features/ui/components/contact-chip/_index.scss (3)
2-2: LGTM! Layout changes enable proper text truncation.The flex layout changes with
min-width: 0andmax-width: 100%are essential for enabling text truncation with ellipsis in flexbox containers. Usingalign-items: baselineensures proper alignment when mixing regular and bold text.Also applies to: 8-9, 33-37, 41-42
55-70: LGTM! Text truncation hierarchy is well-designed.The identity classes implement a smart truncation strategy:
.contact-chip__identity-nameusesflex-shrink: 0to preserve the name, while.contact-chip__identity-emailusesflex: 0 1 autoto allow the email to shrink first when space is constrained. Both properly implement ellipsis truncation.
87-100: LGTM! Icon styles follow BEM conventions.The consolidated icon styling with a base
.contact-chip__iconclass and BEM modifiers (--warning,--error,--success) is maintainable and aligns perfectly with the TypeScript changes. The sharedtransform: translateY(2px)ensures consistent baseline alignment.src/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scss (1)
1-206: LGTM!The SCSS changes appropriately support the collapsible message functionality described in the PR objectives. The new folded state, header toggle button with expanded click area, and pointer-events handling for interactive elements are well-implemented. The styling refinements for header content, correspondents, and spacing align with the updated message layout.
src/frontend/src/features/utils/unquote-message/index.test.ts (1)
1-978: LGTM!Comprehensive test coverage for the UnquoteMessage utility, including:
- Multiple email client quote formats (Gmail, Outlook, Thunderbird, ProtonMail, etc.)
- Plain text quote patterns in various languages
- Edge cases (empty content, no quotes, whitespace)
- Options (wrap mode, depth limits, ignoreFirstForward)
- Complex real-world scenarios
The test structure is clear and descriptive.
src/frontend/src/features/message/use-read.tsx (1)
19-45: LGTM!The updated mutation flow correctly passes structured metadata to
invalidateThreadMessagesfor optimistic cache updates. The payload structure withtype: 'update', message/thread IDs, and read status is appropriate. The refactor ofmarkAsmaintains the same behavior with better readability.src/frontend/src/features/layouts/components/thread-view/_index.scss (2)
49-50: Verify the 300px bottom margin is intentional.The
margin-bottom: 300pxon the messages list is quite large. Please confirm this spacing is intentional for the scroll behavior or if it should be a smaller value.
66-81: LGTM!The new header and sticky container structure with gradient background and proper z-indexing supports the updated thread view layout well.
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (3)
107-246: LGTM!The iframe HTML wrapper includes comprehensive styling for email content rendering, with excellent support for collapsible quoted content via
details.email-quoted-content. The progressive blockquote border colors, hover effects, and tooltip-style summary work well together.
293-306: LGTM!The conditional rendering based on
isHiddenproperly collapses the iframe while keeping it in the DOM, which maintains the iframe lifecycle for when messages are expanded again.
90-104: The review comment is incorrect and should be disregarded.The current code at line 96 works correctly. While it redundantly passes
sanitizedContentto both parameters, this causes no functional issue because:
- When
rawHtmlBodyexists,getHtml()is called, which uses only the first parameter. The second parameter is ignored.- When only
rawTextBodyexists,getText()is called, which uses only the second parameter. The first parameter is ignored.The claim that "plain text to be processed as HTML" is unfounded. The
getText()method processes text exclusively through#removeTextQuotesByPattern()and never accesses#htmlContent.The suggested fix contains a defect: it would pass the raw, unsanitized
rawTextBodyprop instead of the sanitized content, bypassing the security sanitization when only text content exists.Likely an incorrect or invalid review comment.
src/frontend/src/features/providers/mailbox.tsx (2)
21-25: LGTM! Well-structured invalidation source type.The
MessageQueryInvalidationSourcetype provides a clear contract for cache invalidation operations, supporting both delete and update semantics with appropriate metadata.
225-239: Well-integrated cache update logic.The implementation properly coordinates optimistic cache updates with query invalidation, updating cached data when source metadata is provided before triggering refetches. The iteration pattern and conditionals correctly handle both thread-level and message-level updates.
src/frontend/src/features/layouts/components/thread-view/index.tsx (4)
1-30: Clean refactoring to provider-based architecture.The new imports and
ThreadViewComponentPropstype definition establish a clear separation between the wrapper (ThreadView) and the internal component (ThreadViewComponent), with the provider handling cross-message coordination.
67-95: IntersectionObserver setup correctly guards on readiness.The observer configuration properly waits for
isReadybefore setting up intersection tracking, and dynamically calculates therootMarginto account for the sticky header. The dependency array correctly usesunreadMessageIds.join(",")to detect array changes.
194-216: Message filtering logic correctly handles draft nesting and trash views.The implementation properly nests draft messages under their parents and filters based on trash view state. The memoization dependencies ensure the derived data stays synchronized with the message list.
222-243: ThreadView wrapper cleanly delegates to provider-wrapped component.The refactored
ThreadViewfunction correctly guards against null mailbox/thread, handles loading state, and wrapsThreadViewComponentwithThreadViewProvider, passing the filtered message IDs for per-message readiness tracking.src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (5)
1-62: Enhanced imports and context integration support new UI features.The additional imports bring in necessary UI components (Icon variants, Spinner, UserAvatar) and utilities (clsx, useThreadViewContext, usePrevious) that enable the message folding, loading states, and provider-based readiness tracking.
66-82: Delivery status mapping correctly handles all scenarios.The helper properly maps
MessageDeliveryStatusChoicestoContactChipDeliveryStatusobjects, including the edge case where sender messages have null delivery status but should show as "delivering."
194-274: Well-structured header actions with appropriate conditionals.The action buttons and dropdown menu options are properly organized with correct conditional rendering based on message state (folded, trashed, sender permissions). The combination of inline actions and overflow menu follows good UX patterns.
328-336: MessageBody integration correctly coordinates with readiness tracking.The
isHiddenprop appropriately combines folding and readiness states, and theonLoadcallback properly signals the ThreadViewContext when the message content has loaded, enabling accurate intersection observation timing.
337-391: Footer structure properly handles attachments, actions, and loading states.The footer correctly places the intersection trigger for read tracking, conditionally renders reply actions for the latest message, includes the reply form section with a ref for smooth scrolling, and displays a spinner while message content loads.
1 participant
Purpose
Currently threads are hard to read mainly because embedded
replies and other kind of messages are not collapsed like other
mail application does. So we implement a logic to detect empirically
those kind of message and wrap them into a details element.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Tests