Fix/composer issues#662
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds BlockNote table rendering, shared test factories, and scoped composer CSS; implements tiered blob storage with encryption, offload, GC, reservation, and reconciliation; introduces thread assignment/mention services, new ThreadEvent types, serializers, viewset behavior, and migrations; rewrites RFC5322 composer to stdlib email; plus many tests, docs, CI, and runtime bumps. ChangesFrontend: BlockNote tables, tests, CSS, and minor wiring
Backend: Tiered blob storage, GC, reservations, offload tasks, and DB model/migration changes
Backend: Thread assignment/mention feature, permissions, serializers, viewsets, admin and cleanup wiring
Backend: RFC5322 composer rewrite, outbound flow changes, and tests
Miscellaneous, infra, docs, and test harness updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~240 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/features/blocknote/blocknote-view-field/_index.scss`:
- Around line 57-60: The SCSS block has stylelint errors caused by stray empty
lines before declarations/comments; remove the blank line(s) immediately
preceding the declarations such as --min-height, --toolbar-height, border-radius
and border-width and the other affected blocks around the later scoped block
(the region that contains the same CSS custom properties at lines ~116-124),
ensuring no empty line before any declaration or comment and consistent single
newline between rule blocks, then re-run stylelint to confirm the file
(blocknote-view-field/_index.scss) is lint-clean.
In `@src/frontend/src/features/blocknote/email-exporter/index.test.tsx`:
- Around line 750-905: Add a regression test in
src/frontend/src/features/blocknote/email-exporter/index.test.tsx that ensures
exportBlocks handles malformed table data without throwing (e.g., a row that's
null, a cell that's null, or a row object missing a cells array); create a test
case using the existing table/tableCell helpers to build a table-like block
where one row is null and another row is { /* missing cells */ } cast to
AnyBlock, call exportBlocks([...]) and assert it does not throw and does not
emit a <table> (or returns null/no table HTML). Locate the test near the other
'table' describe block and reference exportBlocks, table, tableCell to validate
fail-soft behavior.
In `@src/frontend/src/features/blocknote/email-exporter/index.tsx`:
- Around line 436-444: normalizeTableCell and the table row rendering assume
cells and rows are arrays/objects, causing exceptions for null/undefined or
legacy shapes; update normalizeTableCell to defensively handle
non-array/non-object inputs by returning { content: [], props: {} } for
null/undefined or primitive values, treat Array inputs as content, and ensure
props defaults to {}; likewise, in the table row rendering (the code that
iterates over row.cells) guard against rows that are null or where row.cells is
not an array by skipping those rows or coercing row.cells to an empty array
before mapping, and ensure each cell is passed through normalizeTableCell so
malformed cells yield safe defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21c4ee7e-0e48-42d5-851b-9f49defbb05f
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/frontend/package.jsonsrc/frontend/src/features/blocknote/blocknote-view-field/_index.scsssrc/frontend/src/features/blocknote/email-exporter/index.test.tsxsrc/frontend/src/features/blocknote/email-exporter/index.tsx
f6f21f5 to
4e25906
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/forms/components/message-form/index.tsx (1)
369-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore draft sync after sender transfer.
In the non-
newflow, this deletes the current draft and creates a replacement, but the redirect is gone and localdraftstate is not updated. The form stays bound to the deleted draft id, so the next save/send can target a message that no longer exists.If you want to keep the user on the same route, update `draft`/thread state from `response.data` before returning instead.Minimal fix
- // router.replace(`/mailbox/${mailboxId}/thread/${threadId}?has_draft=1`); + await router.replace(`/mailbox/${mailboxId}/thread/${threadId}?has_draft=1`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/forms/components/message-form/index.tsx` around lines 369 - 390, handleChangeSender currently deletes the old draft, creates a replacement via draftCreateMutation, but in the non-"new" path never updates local state or navigates, leaving the form bound to a deleted id; to fix, after receiving response from draftCreateMutation update the local draft state (call setDraft(response.data as Message) and any thread id fields) and then either perform the redirect (router.replace(`/mailbox/${data.senderId}/thread/${response.data.thread_id}?has_draft=1`)) or ensure form bindings use the new draft id so subsequent save/send targets the new message; adjust code around deleteMessageMutation, draftCreateMutation, setDraft and router.replace to keep UI and route in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/features/forms/components/message-form/index.tsx`:
- Around line 102-113: The reply prefill logic uses selectedMailbox but
defaultSenderId can differ after filtering (availableMailboxes), causing the
chosen sender to be treated as external; compute the actual reply sender email
(e.g., replySenderEmail derived from defaultSenderId by looking up
availableMailboxes.find(m => m.id === defaultSenderId)?.email or from
draft.sender.email) and then replace uses of selectedMailbox?.email in the
toRecipients/ccRecipients filters and in the "reply to self" check with that
replySenderEmail so the filtered recipients and self-reply branch align with the
filtered sender selection (refer to availableMailboxes, defaultSenderId,
selectedMailbox, draft, toRecipients, ccRecipients).
---
Outside diff comments:
In `@src/frontend/src/features/forms/components/message-form/index.tsx`:
- Around line 369-390: handleChangeSender currently deletes the old draft,
creates a replacement via draftCreateMutation, but in the non-"new" path never
updates local state or navigates, leaving the form bound to a deleted id; to
fix, after receiving response from draftCreateMutation update the local draft
state (call setDraft(response.data as Message) and any thread id fields) and
then either perform the redirect
(router.replace(`/mailbox/${data.senderId}/thread/${response.data.thread_id}?has_draft=1`))
or ensure form bindings use the new draft id so subsequent save/send targets the
new message; adjust code around deleteMessageMutation, draftCreateMutation,
setDraft and router.replace to keep UI and route in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 245b3f4d-9d52-422e-8d60-059988fc63f8
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
src/frontend/package.jsonsrc/frontend/src/features/blocknote/__tests__/block-factories.tssrc/frontend/src/features/blocknote/blocknote-view-field/_index.scsssrc/frontend/src/features/blocknote/email-exporter/index.test.tsxsrc/frontend/src/features/blocknote/email-exporter/index.tsxsrc/frontend/src/features/blocknote/markdown-exporter.test.tssrc/frontend/src/features/blocknote/utils.test.tssrc/frontend/src/features/forms/components/message-composer/index.tsxsrc/frontend/src/features/forms/components/message-form/index.tsx
4e25906 to
91a507e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/features/forms/components/message-form/index.tsx`:
- Around line 102-108: Detect when availableMailboxes is empty (e.g., compute a
hasNoEditAccess flag alongside the existing useMemo for availableMailboxes) and
render a user-facing banner before the form when hasNoEditAccess is true;
specifically, after computing availableMailboxes (which currently filters by
ThreadAccessRoleChoices.editor using selectedThread, mailboxes and mode), add
logic to check availableMailboxes.length === 0 and display an error/info Banner
telling the user they lack permission to reply/forward from any of their
mailboxes for this thread so the sender dropdown isn’t simply empty and
confusing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c870d003-14e4-4427-9d89-a08a385ceafb
📒 Files selected for processing (7)
src/frontend/src/features/blocknote/__tests__/block-factories.tssrc/frontend/src/features/blocknote/email-exporter/index.test.tsxsrc/frontend/src/features/blocknote/email-exporter/index.tsxsrc/frontend/src/features/blocknote/markdown-exporter.test.tssrc/frontend/src/features/blocknote/utils.test.tssrc/frontend/src/features/forms/components/message-composer/index.tsxsrc/frontend/src/features/forms/components/message-form/index.tsx
efc501d to
18fd138
Compare
Remove a bug that prevent to use backspace in an empty block. TypeCellOS/BlockNote#2610
Improve email exporter to support table elements. We do not add blocknote tool to add explicitly table but we allow user to copy/paste it and render it properly.
18fd138 to
7d95dea
Compare
Purpose
Remove a bug that prevent to use backspace in an empty block.
TypeCellOS/BlockNote#2610
Improve email exporter to support table elements. We do not add
blocknote tool to add explicitly table but we allow user to
copy/paste it and render it properly.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores