✨(template) allow to insert variable in message template #385
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdded template-variable placeholder support to the template composer: extended BlockNote inlineContentSpecs, fetched placeholders via usePlaceholdersRetrieve (refetch on mount and focus), rendered TemplateVariableSelector in the toolbar, and moved/added stylesheet rules for inline template-variable presentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Mount as TemplateComposer Mount
participant Hook as usePlaceholdersRetrieve
participant API as Placeholders API
participant Composer as TemplateComposer
participant Selector as TemplateVariableSelector
Mount->>Hook: initialize (on mount)
Hook->>API: fetch placeholders
API-->>Hook: placeholders data
Hook-->>Composer: update placeholders & loading
Composer->>Composer: extend BlockNote schema with 'template-variable'
Composer->>Selector: render with placeholders & loading
Note over Selector: user chooses placeholder → inserted as inline content
rect rgb(240,248,255)
Note over Mount,Hook: refetch on window focus
Mount-->>Hook: trigger refetch
Hook->>API: re-fetch placeholders
API-->>Hook: updated data
Hook-->>Composer: update placeholders
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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 |
![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
🧹 Nitpick comments (1)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/_index.scss (1)
20-22: Use theme variable instead of hardcoded color.The selected node styling uses a hardcoded color
#94badc, which breaks consistency with the theme-based approach used elsewhere in the file (e.g., lines 12-13, 15).Consider using a theme variable for consistency:
- .node-template-variable.ProseMirror-selectednode span[data-inline-content-type="template-variable"] { + .node-template-variable.ProseMirror-selectednode span[data-inline-type="template-variable"] { - background: #94badc; + background: var(--c--theme--colors--primary-200);Note: Also corrected the attribute selector to match the fix in the previous comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/_index.scss(1 hunks)src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/template-composer.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/template-composer.tsx (2)
src/frontend/src/features/blocknote/inline-template-variable/index.tsx (2)
InlineTemplateVariable(63-83)TemplateVariableSelector(14-60)src/frontend/src/features/api/gen/placeholders/placeholders.ts (1)
usePlaceholdersRetrieve(168-196)
⏰ 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: test-back
- GitHub Check: lint-back
- GitHub Check: check-api-state
- GitHub Check: build-front
🔇 Additional comments (2)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/template-composer.tsx (2)
179-182: Verify serialization of template variables.The template variable feature is being integrated, but there's a TODO comment in the
InlineTemplateVariablecomponent (seesrc/frontend/src/features/blocknote/inline-template-variable/index.tsx, line 73-74) indicating that serialization/deserialization during export and parsing may not be fully implemented. This could affect saving and loading templates with variables.Please confirm that template variables correctly serialize when saving templates and deserialize when loading them. Test the following workflow:
- Insert a template variable into a template
- Save the template
- Reload/reopen the template
- Verify the variable displays correctly and can be edited
If serialization is incomplete, the variables may not persist correctly across saves.
3-3: Good integration of template variable feature.The integration of
InlineTemplateVariableandTemplateVariableSelectorfollows the established pattern in the codebase. The schema extension, placeholder fetching, and toolbar integration are implemented correctly.Also applies to: 23-23, 179-182
...frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/_index.scss
Outdated
Show resolved
Hide resolved
...rc/features/layouts/components/mailbox-settings/modal-compose-template/template-composer.tsx
Show resolved
Hide resolved
sdemagny
force-pushed
the
sdem/insert_variable_into_msg_template
branch
from
67d4660 to
7fc7a55
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/template-composer.tsx (1)
46-51: Error handling for placeholders API is already flagged.This concern about missing error state handling (
isError,error) was raised in previous review comments. The current implementation will silently fail with an empty object if the API call fails.
🧹 Nitpick comments (1)
src/frontend/src/features/blocknote/inline-template-variable/_index.scss (1)
14-16: Consider using a CSS custom property for the selected background.The selected-node background uses a hardcoded color
#94badcwhile other colors use CSS custom properties. For consistency and maintainability, consider defining this in the theme variables.-.node-template-variable.ProseMirror-selectednode span[data-inline-content-type="template-variable"] { - background: #94badc; +.node-template-variable.ProseMirror-selectednode span[data-inline-type="template-variable"] { + background: var(--c--theme--colors--primary-200); }Note: Also update the selector to match the fix in the previous comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/frontend/src/features/blocknote/inline-template-variable/_index.scss(1 hunks)src/frontend/src/features/layouts/components/admin/modal-compose-signature/_index.scss(0 hunks)src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/template-composer.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- src/frontend/src/features/layouts/components/admin/modal-compose-signature/_index.scss
🧰 Additional context used
🧬 Code graph analysis (1)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/template-composer.tsx (2)
src/frontend/src/features/blocknote/inline-template-variable/index.tsx (2)
InlineTemplateVariable(63-83)TemplateVariableSelector(14-60)src/frontend/src/features/api/gen/placeholders/placeholders.ts (1)
usePlaceholdersRetrieve(168-196)
⏰ 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 (1)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-template/template-composer.tsx (1)
3-3: LGTM! Template variable integration looks solid.The integration properly:
- Imports the necessary components
- Extends the BlockNote schema with the new inline content type
- Renders the selector in the toolbar with correct props
The implementation follows the same pattern as the existing
SignatureTemplateSelector.Also applies to: 23-23, 179-182
src/frontend/src/features/blocknote/inline-template-variable/_index.scss
Show resolved
Hide resolved
sdemagny
force-pushed
the
sdem/insert_variable_into_msg_template
branch
from
7fc7a55 to
bf77b07
Compare
1 participant
Summary by CodeRabbit
New Features
Style