feat(app): add variables to header component#3
feat(app): add variables to header component#3franciscobalmaceda wants to merge 6 commits intorendis:mainfrom
Conversation
rendis
left a comment
rendis
left a comment
There was a problem hiding this comment.
Good direction overall — enabling header variables in the editor is a useful improvement.
That said, I don’t think this is merge-ready yet because the PR only completes the UI/export part of the feature, while the rest of the contract is still out of sync.
What this PR does well:
- it enables generic injectors on the header surface,
- it routes variable insertion to the active header/body editor,
- and it exports variableIds referenced from header.content.
What is still missing:
- frontend semantic validation still does not traverse header.content for generic injectors,
- backend publish validation still focuses on body injector nodes,
- the current capability/docs still describe generic header text injectors as unsupported / not safe-by-default,
- and there is no regression coverage for this new workflow.
So the issue is not that the feature is fake — it clearly adds real behavior — but that it introduces a new supported-looking workflow without aligning validation, documentation, and test coverage.
Before merging, I think this needs to be completed end-to-end:
- add frontend semantic validation for injectors inside header.content,
- add equivalent backend validation for header injectors during publish/render validation,
- add regression coverage for header injector export/import/publish flows,
- update the capability matrix/docs to reflect that header text injectors are now supported.
Backend note: validation still appears to validate generic injector nodes through the body document traversal only (core/internal/core/service/template/contentvalidator/variables.go). If header injectors are now an officially supported feature, backend validation should also inspect header.content for injector references — not just variableIds and header image injectables. Otherwise the UI/export contract moves ahead of publish validation.
Good feature direction, but I’d like to see the contract aligned before this goes in.
| StoredMarksPersistenceExtension, | ||
| LineSpacingExtension, | ||
| HeaderEnterAsBreakExtension, | ||
| InjectorExtension, |
There was a problem hiding this comment.
This exposes a new product capability on the header surface, but the surrounding contract is not aligned yet. The current capability docs still classify generic header injector placeholders as unsupported / not safe-by-default, and neither frontend nor backend validation has been updated accordingly. I think this needs to land together with the validation/docs changes; otherwise we’re shipping a UI feature that the rest of the system does not fully recognize yet.
There was a problem hiding this comment.
Updated docs to reflect new capability
| if (headerState.imageInjectableId) { | ||
| variableIds.add(headerState.imageInjectableId) | ||
| } | ||
| if (Array.isArray(headerState.content?.content)) { |
There was a problem hiding this comment.
Good catch including variable IDs coming from header.content, but this is incomplete unless the rest of the semantic pipeline also understands header injectors. Right now document-validator.ts / getUsedVariableIds() still only traverse body content for generic injector references, so export and semantic validation become inconsistent. I’d strongly prefer to update both sides in the same PR.
There was a problem hiding this comment.
Add validation traverse over header nodes for injectors
| const handleVariableClick = useCallback( | ||
| (data: VariableDragData) => { | ||
| insertVariable(data) | ||
| const targetEditor = |
There was a problem hiding this comment.
This makes the editor flow work for header insertion, but I don’t see coverage proving that the resulting document survives the full lifecycle consistently. Since this is now a new editing surface for injectors, I think we need regression coverage for at least click insertion into header, drag/drop into header, export/import roundtrip, and publish/validation without inconsistent warnings or errors.
There was a problem hiding this comment.
Added tests going over validation of header insertion, inluding full lifecycle of export/import and publish
2 participants
Description
Brief description of what this PR does.
Fixes #(issue number)
Type of Change
Changes Made
Add variable support for headers
Add drag and drop for variables into header
Fix preview variables values to scan for variables inside the header
Testing Done
Describe the tests you ran to verify your changes:
Test Configuration:
Checklist
make lintand fixed all issuesmake testand all tests passmake buildand it succeedsmake swagger) if API changedcore/internal/(NOTcore/extensions/)Screenshots (if applicable)
Add screenshots for UI changes.
Additional Notes
Any additional information reviewers should know.