-
Notifications
You must be signed in to change notification settings - Fork 5
✨(template) insert template into message #389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughUpdated POT-Creation-Date and adjusted source-file line anchors across multiple backend PO files; added two frontend locale keys (en-US, fr-FR); introduced a MessageTemplateSelector React component and SCSS; integrated the selector into MessageComposer and imported its styles in main.scss. MailboxId prop made required. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Composer as MessageComposer
participant Selector as MessageTemplateSelector
participant API as Templates API
participant Modal as Template Modal
participant Editor as BlockNote Editor
User->>Composer: open composer
Composer->>Selector: render (mailboxId)
User->>Selector: click toolbar button
Selector->>API: fetch templates(mailboxId)
API-->>Selector: templates list
Selector->>Modal: show templates (or empty)
alt templates found
User->>Modal: choose template
Selector->>Selector: convert HTML → blocks
Selector->>Selector: optionally append signature
Selector->>Editor: insert blocks at cursor / replace empty block
Editor-->>User: template inserted
Selector->>Modal: close
else no templates
Modal-->>User: show "No templates available"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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 |
65057f6
to
1aeec15
Compare
1aeec15
to
71617ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/locale/br_FR/LC_MESSAGES/django.po (1)
13-16
: Fix broken Breton Plural-Forms header syntax and complete missing plural form definitions.Lines 13–16 contain multiple critical errors in the Plural-Forms expression:
- Line 13 uses
||
(OR) instead of&&
(AND):(n%100!=11 || n%100!=71 || n%100!=91)
should be(n%100!=11 && n%100!=71 && n%100!=91)
- Line 15 defines incorrect condition:
(n%10>=3 && n%10<=4)
should be(n%10==3 || n%10==4 || n%10==9)
per CLDR- Line 16 is truncated and incomplete: ends with
(n!=0 && n%1;
and lacks the final two plural form cases (3 and 4)Restore the correct Breton plural rule:
"Plural-Forms: nplurals=5; plural=(n%10==1 && n%100!=11 && n%100!=71 && n%100!=91) ? 0 : (n%10==2 && n%100!=12 && n%100!=72 && n%100!=92) ? 1 : ((n%10==3 || n%10==4 || n%10==9) && (n%100<10 || n%100>19) && (n%100<70 || n%100>79) && (n%100<90 || n%100>99)) ? 2 : (n!=0 && n%1000000==0) ? 3 : 4\n"
🧹 Nitpick comments (7)
src/backend/locale/fr_FR/LC_MESSAGES/django.po (2)
425-428
: Fix gender agreement: “libellé” is masculine; use “appliqué” (not “appliquée”).-msgstr "Si ce libellé doit être automatiquement appliquée par l'IA" +msgstr "Si ce libellé doit être automatiquement appliqué par l'IA"
621-623
: Consistency: prefer “domaine de messagerie” (used elsewhere) over “Domaine de courrier”.-msgstr "Domaine de courrier qui possède ce blob" +msgstr "Domaine de messagerie qui possède ce blob"src/backend/locale/ru_RU/LC_MESSAGES/django.po (1)
643-646
: Correct meaning: this string refers to attachment data, not a template.-msgstr "Ссылка на блок, содержащий данные шаблона в формате Json" +msgstr "Ссылка на блок, содержащий данные вложения"src/backend/locale/en_US/LC_MESSAGES/django.po (1)
3-18
: Header metadata seems copy-pasted from another project (lasuite-docs/backend-drive.pot). Align with messages backend for consistency across locales.-"Project-Id-Version: lasuite-docs\n" +"Project-Id-Version: lasuite-messages\n" ... -"X-Crowdin-Project: lasuite-docs\n" -"X-Crowdin-Project-ID: 754523\n" +"X-Crowdin-Project: lasuite-messages\n" +"X-Crowdin-Project-ID: 831182\n" ... -"X-Crowdin-File: backend-drive.pot\n" -"X-Crowdin-File-ID: 18\n" +"X-Crowdin-File: backend.pot\n" +"X-Crowdin-File-ID: 40\n"src/frontend/src/features/blocknote/message-template-block/index.tsx (3)
17-22
: Guard editor/context instead of asserting non-null to avoid runtime crashes during early renders/SSR.-export const MessageTemplateSelector = ({ mailboxId }: MessageTemplateSelectorProps) => { - const { t } = useTranslation(); - const editor = useBlockNoteEditor<MessageComposerBlockSchema, MessageComposerInlineContentSchema, MessageComposerStyleSchema>(); - const Components = useComponentsContext()!; +export const MessageTemplateSelector = ({ mailboxId }: MessageTemplateSelectorProps) => { + const { t } = useTranslation(); + const editor = useBlockNoteEditor<MessageComposerBlockSchema, MessageComposerInlineContentSchema, MessageComposerStyleSchema>(); + const Components = useComponentsContext(); + if (!editor || !Components) return null;
31-33
: Don’t block insertion when raw_body is absent; fall back to HTML-only insertion.- if (!template.raw_body || !template.id) return; + if (!template.id) return;
83-91
: Add i18n keys for “Loading templates...” to locale files to avoid fallback-only UI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/backend/locale/br_FR/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/de_DE/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/en_US/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/es_ES/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/fr_FR/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/it_IT/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/nl_NL/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/pt_PT/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/ru_RU/LC_MESSAGES/django.po
(5 hunks)src/backend/locale/uk_UA/LC_MESSAGES/django.po
(5 hunks)src/frontend/public/locales/common/en-US.json
(2 hunks)src/frontend/public/locales/common/fr-FR.json
(2 hunks)src/frontend/src/features/blocknote/message-template-block/_index.scss
(1 hunks)src/frontend/src/features/blocknote/message-template-block/index.tsx
(1 hunks)src/frontend/src/features/forms/components/message-composer/index.tsx
(4 hunks)src/frontend/src/styles/main.scss
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/frontend/src/features/forms/components/message-composer/index.tsx (1)
src/frontend/src/features/blocknote/message-template-block/index.tsx (1)
MessageTemplateSelector
(17-141)
src/frontend/src/features/blocknote/message-template-block/index.tsx (3)
src/frontend/src/features/forms/components/message-composer/index.tsx (4)
MessageComposerBlockSchema
(28-28)MessageComposerInlineContentSchema
(29-29)MessageComposerStyleSchema
(30-30)PartialMessageComposerBlockSchema
(31-31)src/frontend/src/features/api/gen/mailboxes/mailboxes.ts (2)
useMailboxesMessageTemplatesAvailableList
(1445-1479)mailboxesMessageTemplatesRenderRetrieve
(1077-1089)src/frontend/src/features/api/gen/models/read_only_message_template.ts (1)
ReadOnlyMessageTemplate
(13-36)
⏰ 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: build-front
- GitHub Check: test-back
🔇 Additional comments (13)
src/frontend/public/locales/common/en-US.json (1)
167-167
: LGTM! Translation keys are clear and consistent.The new translation keys for the template insertion feature are well-named and follow the existing conventions in the file.
Also applies to: 221-221
src/frontend/public/locales/common/fr-FR.json (1)
167-167
: LGTM! French translations are appropriate.The translations for the template insertion feature are grammatically correct and consistent with the existing French localization patterns.
Also applies to: 221-221
src/frontend/src/styles/main.scss (1)
67-67
: LGTM! Import is correctly positioned.The new SCSS module import is properly placed in the Blocknote custom blocks section and follows the existing pattern.
src/backend/locale/es_ES/LC_MESSAGES/django.po (1)
5-5
: LGTM! Routine translation metadata update.This is a standard gettext catalog metadata update reflecting line number changes in source files. No translation content was modified.
Also applies to: 61-753
src/frontend/src/features/forms/components/message-composer/index.tsx (2)
213-215
: LGTM! Template selector integration looks correct.The MessageTemplateSelector is properly integrated into the toolbar with the required mailboxId prop.
36-36
: No issues found. The breaking change is properly handled.The verification confirms that MessageComposer is used in only one location (
src/frontend/src/features/forms/components/message-form/index.tsx
at line 587), and this usage already provides the requiredmailboxId
prop. All call sites have been properly updated.src/backend/locale/uk_UA/LC_MESSAGES/django.po (1)
5-5
: LGTM! Routine translation metadata update.Standard gettext catalog maintenance with updated POT-Creation-Date and adjusted line references. No translation strings were modified.
Also applies to: 63-779
src/backend/locale/de_DE/LC_MESSAGES/django.po (1)
5-5
: LGTM! Routine translation metadata update.Standard gettext catalog update with refreshed POT-Creation-Date and realigned source code references. Translation content unchanged.
Also applies to: 61-753
src/backend/locale/fr_FR/LC_MESSAGES/django.po (1)
5-5
: Header timestamp update looks good. No issues.src/backend/locale/it_IT/LC_MESSAGES/django.po (1)
61-63
: Italian entries are largely untranslated. If intentional (fallback to English), fine; otherwise, sync with your TMS before release to avoid mixed-language UI.src/backend/locale/nl_NL/LC_MESSAGES/django.po (1)
5-5
: LGTM. Header and reference anchors updated; no translation changes.src/frontend/src/features/blocknote/message-template-block/index.tsx (1)
50-51
: Confirm HTML-to-blocks sanitization to prevent editor XSS when converting server-rendered HTML. If BlockNote doesn’t sanitize, sanitize on the backend or before tryParseHTMLToBlocks.src/backend/locale/pt_PT/LC_MESSAGES/django.po (1)
5-755
: LGTM! Routine metadata and source reference updates.The POT-Creation-Date timestamp and source code line-number references have been updated to reflect backend code changes. No translation content was modified, which is appropriate for this type of maintenance update.
#: core/models.py:1691 | ||
#, fuzzy | ||
#| msgid "Type of template (reply, new_message, signature)" | ||
msgid "Type of template (message, signature)" | ||
msgstr "Тип шаблона (ответить, новое_сообщение, подпись)" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve fuzzy and update text to match new msgid. Fuzzy strings won’t be compiled; this will fallback to English.
-#, fuzzy
-#| msgid "Type of template (reply, new_message, signature)"
msgid "Type of template (message, signature)"
-msgstr "Тип шаблона (ответить, новое_сообщение, подпись)"
+msgstr "Тип шаблона (сообщение, подпись)"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
#: core/models.py:1691 | |
#, fuzzy | |
#| msgid "Type of template (reply, new_message, signature)" | |
msgid "Type of template (message, signature)" | |
msgstr "Тип шаблона (ответить, новое_сообщение, подпись)" | |
#: core/models.py:1691 | |
msgid "Type of template (message, signature)" | |
msgstr "Тип шаблона (сообщение, подпись)" |
🤖 Prompt for AI Agents
In src/backend/locale/ru_RU/LC_MESSAGES/django.po around lines 748-753, the
entry is marked fuzzy and the msgstr doesn't match the updated msgid; remove the
", fuzzy" marker, update msgstr to a correct Russian translation matching the
new msgid "Type of template (message, signature)" (for example: "Тип шаблона
(сообщение, подпись)"), and ensure there are no leftover commented-out
msgid/msgstr variants so the translator string will be compiled.
.template-item { | ||
display: flex; | ||
align-items: center; | ||
gap: 1rem; | ||
padding: 1rem; | ||
border: 1px solid var(--border-default-grey); | ||
border-radius: 0.25rem; | ||
background: var(--background-default-grey); | ||
cursor: pointer; | ||
transition: all 0.2s ease; | ||
text-align: left; | ||
width: 100%; | ||
|
||
&:hover { | ||
background: var(--background-alt-grey); | ||
border-color: var(--border-default-blue); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add focus styles for keyboard accessibility.
The .template-item
has hover styles but lacks visible focus indicators for keyboard navigation. This creates an accessibility barrier for keyboard-only users.
Add focus styles to ensure keyboard users can see which item is currently focused:
&:hover {
background: var(--background-alt-grey);
border-color: var(--border-default-blue);
}
+
+ &:focus-visible {
+ outline: 2px solid var(--border-default-blue);
+ outline-offset: 2px;
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
.template-item { | |
display: flex; | |
align-items: center; | |
gap: 1rem; | |
padding: 1rem; | |
border: 1px solid var(--border-default-grey); | |
border-radius: 0.25rem; | |
background: var(--background-default-grey); | |
cursor: pointer; | |
transition: all 0.2s ease; | |
text-align: left; | |
width: 100%; | |
&:hover { | |
background: var(--background-alt-grey); | |
border-color: var(--border-default-blue); | |
} | |
.template-item { | |
display: flex; | |
align-items: center; | |
gap: 1rem; | |
padding: 1rem; | |
border: 1px solid var(--border-default-grey); | |
border-radius: 0.25rem; | |
background: var(--background-default-grey); | |
cursor: pointer; | |
transition: all 0.2s ease; | |
text-align: left; | |
width: 100%; | |
&:hover { | |
background: var(--background-alt-grey); | |
border-color: var(--border-default-blue); | |
} | |
&:focus-visible { | |
outline: 2px solid var(--border-default-blue); | |
outline-offset: 2px; | |
} |
🤖 Prompt for AI Agents
In src/frontend/src/features/blocknote/message-template-block/_index.scss around
lines 9 to 25, the .template-item only has hover styles and lacks keyboard focus
indicators; add accessible focus styles by defining :focus and preferably
:focus-visible rules that mirror the hover state (change background and
border-color) and add a non-invasive focus ring (outline or box-shadow) with
outline-offset to avoid layout shift, ensure cursor/appearance stays consistent
and include :focus-visible to avoid showing rings on mouse click.
// Parse template blocks for signature | ||
const blocks = JSON.parse(template.raw_body); | ||
const templateSignature = blocks.find((block: { type: string }) => block.type === "signature"); | ||
|
||
// Convert HTML to blocks using BlockNote's built-in parser | ||
const contentBlocks = await editor.tryParseHTMLToBlocks(renderedTemplate.html_body) as PartialMessageComposerBlockSchema[]; | ||
|
||
// Check if there's already a signature in the editor | ||
const editorSignature = editor.getBlock("signature"); | ||
|
||
// Add signature if needed | ||
if (templateSignature && !editorSignature) { | ||
contentBlocks.push({ | ||
...templateSignature, | ||
props: { | ||
...templateSignature.props, | ||
mailboxId | ||
} | ||
} as PartialMessageComposerBlockSchema); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix signature detection and add robustness checks. getBlock("signature") expects an ID, not a type; use a search by type. Also ensure parsed HTML produced blocks and raw_body JSON is an array before use.
- const blocks = JSON.parse(template.raw_body);
- const templateSignature = blocks.find((block: { type: string }) => block.type === "signature");
+ let templateSignature: PartialMessageComposerBlockSchema | undefined;
+ if (template.raw_body) {
+ const raw = JSON.parse(template.raw_body);
+ const blocks = Array.isArray(raw) ? raw : [];
+ templateSignature = blocks.find((b: { type?: string }) => b?.type === "signature");
+ }
@@
- const contentBlocks = await editor.tryParseHTMLToBlocks(renderedTemplate.html_body) as PartialMessageComposerBlockSchema[];
+ const contentBlocks = await editor.tryParseHTMLToBlocks(renderedTemplate.html_body) as PartialMessageComposerBlockSchema[] | null;
+ if (!contentBlocks || contentBlocks.length === 0) {
+ console.error("Rendering produced no blocks");
+ return;
+ }
@@
- const editorSignature = editor.getBlock("signature");
+ const editorSignature = editor.topLevelBlocks?.some((b) => b.type === "signature");
@@
- if (templateSignature && !editorSignature) {
+ if (templateSignature && !editorSignature) {
🤖 Prompt for AI Agents
In src/frontend/src/features/blocknote/message-template-block/index.tsx around
lines 45-64, the code assumes template.raw_body JSON is an array and that
editor.getBlock("signature") locates a signature by type; instead, wrap
JSON.parse(template.raw_body) in a safe parse (try/catch) and validate that the
result is an array before using .find, validate that contentBlocks from
editor.tryParseHTMLToBlocks is an array before pushing, and replace the
editor.getBlock("signature") call with a search over the editor's blocks (e.g.,
use editor.getBlocks() or equivalent and find(block => block.type ===
"signature")) so detection is by type not by ID; only push the template
signature when parsed template blocks exist, contentBlocks is an array, and no
signature block is found in the editor.
// Insert blocks at cursor position | ||
const currentBlock = editor.getTextCursorPosition().block; | ||
|
||
// if the current block is empty, replace it with the template blocks | ||
const currentBlockContent = editor.getBlock(currentBlock)?.content; | ||
if (currentBlock && (!currentBlockContent || (Array.isArray(currentBlockContent) && currentBlockContent.length === 0))) { | ||
editor.replaceBlocks([currentBlock], contentBlocks); | ||
} else { | ||
// Otherwise we insert after | ||
editor.insertBlocks(contentBlocks, currentBlock, "after"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the block instance you already have; getBlock(currentBlock) is incorrect (expects an ID). Also handle empty-content detection safely.
- const currentBlock = editor.getTextCursorPosition().block;
-
- // if the current block is empty, replace it with the template blocks
- const currentBlockContent = editor.getBlock(currentBlock)?.content;
- if (currentBlock && (!currentBlockContent || (Array.isArray(currentBlockContent) && currentBlockContent.length === 0))) {
+ const currentBlock = editor.getTextCursorPosition().block;
+ const currentBlockContent = currentBlock?.content;
+ if (
+ currentBlock &&
+ (!currentBlockContent || (Array.isArray(currentBlockContent) && currentBlockContent.length === 0))
+ ) {
editor.replaceBlocks([currentBlock], contentBlocks);
} else {
// Otherwise we insert after
editor.insertBlocks(contentBlocks, currentBlock, "after");
}
🤖 Prompt for AI Agents
In src/frontend/src/features/blocknote/message-template-block/index.tsx around
lines 66 to 76, you're calling editor.getBlock(currentBlock) even though
currentBlock is already a block instance and editor.getBlock expects an ID;
replace that call by reading currentBlock.content directly, guard access with
currentBlock && currentBlock.content, detect empty content safely by checking
for null/undefined, Array.isArray(content) && content.length === 0, or typeof
content === 'string' && content.trim() === '', and when calling
editor.replaceBlocks/insertBlocks pass the block ID (currentBlock.id) rather
than the block object.
} | ||
|
||
// Insert blocks at cursor position | ||
const currentBlock = editor.getTextCursorPosition().block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Select a template should replace the whole existing content nope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my view, selecting a template should not overwrite existing content but instead insert content at the chosen location. Templates should be additive.
src/frontend/src/features/blocknote/message-template-block/_index.scss
Outdated
Show resolved
Hide resolved
if (templates.length === 0) { | ||
return ( | ||
<Components.FormattingToolbar.Button | ||
icon={<Icon name="description" size={IconSize.SMALL} />} | ||
isDisabled={true} | ||
label={t("No templates available")} | ||
mainTooltip={t("No templates available")} | ||
/> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think to simply hide the button if there is no template ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that keeping the icon visible could encourage users to engage with the feature and help them better understand its purpose
d1a207c
to
448deda
Compare
Summary by CodeRabbit