Honor embed format and custom size in the compose editor#5548
Conversation
The CodeMirror compose editor dropped a BFM embed directive's format and size specifier: `CardWidget` never recorded them, the target notifier hardcoded `atom`/`embedded`, and the live-preview template rendered the card with no sizing. So `::card[url | fitted w:400 h:300]` and `| isolated` rendered at the wrong size in compose, while the saved view and edit preview rendered them correctly. Thread format/width/height through the widget -> DOM data-attrs -> target -> template chain, reusing `parseBfmSizeSpec` / `bfmRefFormatAndSize` so the compose editor matches the saved and preview markdown renderers (including `overflow: hidden` for fitted). Applies to both card and file embeds. `CardWidget.eq` and the target skip-check now account for the size fields so a size-only edit re-renders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Preview deploymentsHost Test Results127 tests 127 ✅ 8m 10s ⏱️ Results for commit de2c330. Realm Server Test Results 1 files ±0 1 suites ±0 12m 11s ⏱️ -7s Results for commit de2c330. ± Comparison against earlier commit ec1d83d. |
Route inline non-atom embeds (embedded/fitted/isolated) through a chrome-less inline-block slot (codemirror-card-slot--inline-embed) instead of the atom pill, so a sized or isolated inline card occupies its requested footprint the same way the saved and preview markdown renderers do. The atom pill path is unchanged. Type CardWidget.format and parseCardWidgetContent as BfmRefFormat. Add compose-editor coverage: block fitted and prefix-form (@-RRI) previews resolve and size their cards, inline embeds flow by format (atom pill vs sized inline-block), and a custom fitted chooser selection inserts a directive carrying its size rather than a bare `fitted`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "selecting a fitted format inserts a directive carrying its size" test asserts that choosing the Custom-size fitted option inserts a directive with explicit dimensions. Selecting Custom without entering a width/height currently serializes to a bare `| fitted` (selectFormat seeds dimensions only for the named fitted variants, not for Custom), so the test pins behavior that is not yet implemented and does not belong on this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Selecting the Custom-size fitted option seeds no width/height, so inserting immediately would emit a size-less bare `::card[url | fitted]`. Gate the insert CTA on EmbedFormatSelection.hasValidSize — always true for the named formats, and for Custom only once at least one dimension parses — and guard the insert action so a disabled CTA can't fire. Cover the disabled state and the sized insert with an acceptance test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2c330d3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.category !== 'custom') { | ||
| return true; | ||
| } | ||
| return this.width !== undefined || this.height !== undefined; |
There was a problem hiding this comment.
When the chooser opens in edit mode for an existing ::card[url | fitted] directive, deriveFormatSeeds treats that valid bare fitted spec as category === 'custom' with empty width/height, so this returns false and the pane disables the Done/Accept button. The markdown parser/renderer explicitly supports bare fitted with no size attributes, so users editing existing/manual content in that format cannot accept the unchanged embed unless they add an arbitrary dimension; please only block blank Custom for new inserts or preserve seeded bare-fitted edit state as valid.
Useful? React with 👍 / 👎.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed for parity with the saved/preview markdown renderers. The fix's stated contract is that the compose editor should size and format embeds exactly as the saved view and edit-preview already do, so I traced the widget → data-boxel-bfm-* attr → bfmRefFormatAndSize → target → template chain against rendered-markdown.gts and verified each hop.
Bottom line: the threading is correct and the twin parity genuinely holds. One non-blocking regression in the edit-mode CTA — already flagged by the automated reviewer, now verified with a concrete repro.
What lands right:
- The widget → DOM attrs → target → template chain reuses the same helpers the render side uses, so
fitted/isolated/ explicit-size embeds now derive identically in compose and in saved views (verified hop by hop — see the confirmation oncodemirror-context.ts). - Both re-render guards are needed and correct:
CardWidget.eq(format/width/height) forces CM6 to rebuild the widget DOM on a size-only edit, and the_applyTargetsstructural skip-check (format/style, guarded by thelengthcheck) forces the Glimmer re-render. Without both, aw:400 → w:600edit would silently no-op. - The new integration tests exercise the real resolved-card path, not just target shape — the fitted slot's computed
width/height/overfloware asserted on the rendered element.
Recommendation (non-blocking):
- Keep a bare
::card[url | fitted]editable — opening the chooser on one currently disables "Done" because the seed lands in thecustomcategory with no dimensions. Gate the disable on dirtiness (or scope it to new inserts) and add the edit-path test. Detail in the thread onformat-selection.ts.
Adjacent, out of scope:
CardWidgetTargetis declared twice — incodemirror-editor.gts(base) andcodemirror-context.ts(host) — and this change extends both in lockstep. They're structurally identical; worth collapsing to one exported type before they drift, but not this PR's job.- The compose editor's loading/unresolved fallback (
codemirror-card-fallback) carries no size style, whereasrendered-markdown.gtssizes its loading shimmer — so a block fitted embed doesn't reserve its footprint until the card resolves. Minor, transient, pre-existing structure; flagging only because the PR's goal is renderer parity.
Generated by Claude Code
| get hasValidSize(): boolean { | ||
| if (this.category !== 'custom') { | ||
| return true; | ||
| } | ||
| return this.width !== undefined || this.height !== undefined; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] Editing an existing bare ::card[url | fitted] disables the "Done" CTA, so the unchanged embed can't be accepted. The automated reviewer flagged this; I traced it end to end and it reproduces.
The path. Bare fitted is a first-class supported form — parseBfmSizeSpec('fitted') returns { format: 'fitted' }, and serializeBfmSizeSpec emits bare fitted for a dimensionless fitted spec. When the pencil opens the chooser on such a directive, codemirror-editor.gts calls editEmbed({ sizeSpec: 'fitted', kind: 'block' }), and deriveFormatSeeds('fitted', 'block') takes the fitted default branch → { format: 'custom', width: undefined, height: undefined, kind: 'block' }. The constructed EmbedFormatSelection therefore has category === 'custom' with empty widthInput/heightInput, so hasValidSize returns false → pane.gts's ctaDisabled is true → the "Done" button is disabled. The user can't accept the embed they opened without either typing a dimension (which silently rewrites the directive) or removing it.
Scope: regression introduced here — before this guard the CTA was always enabled, so the edit round-trip worked. It's narrow (only bare fitted; named variants and explicit W×H seed dimensions and are unaffected), so non-blocking — but it does break a valid edit round-trip.
A way out. The guard wants to block a new Custom insert left empty, not a seeded embed opened unchanged; dirtiness distinguishes the two — e.g. in pane.gts:
private get ctaDisabled(): boolean {
return !this.args.selection.hasValidSize && this.args.selection.isDirty;
}An unchanged bare-fitted edit has isDirty === false, so "Done" stays enabled and re-serializes the same | fitted; a fresh Custom-with-no-dims pick is dirty (selection moved off atom) and stays blocked — so the acceptance test added here still passes. Alternatively, scope the block to new inserts / preserve seeded bare-fitted as valid, as the automated reviewer suggested.
Coverage. The new acceptance test covers the insert path (pick Custom → disabled → enter dims → enabled). The gap is the edit path: open the chooser on an existing ::card[url | fitted] and assert "Done" is enabled and accepting re-inserts the bare fitted.
Generated by Claude Code
| let style = | ||
| format === 'fitted' | ||
| ? sizeStyle | ||
| ? `${sizeStyle}; overflow: hidden` | ||
| : 'overflow: hidden' | ||
| : undefined; |
There was a problem hiding this comment.
[Claude Code 🤖] Confirming this is correct, and documenting the invariant it now depends on. This block is the compose-editor twin of the saved/preview renderer in operator-mode/preview-panel/rendered-markdown.gts, and I verified the two match hop for hop: the same bfmRefFormatAndSize(..., kind === 'inline' ? 'atom' : 'embedded') call, and the identical fitted-style formula (${sizeStyle}; overflow: hidden, else overflow: hidden). The atom-vs-embed class split added in codemirror-editor.gts mirrors markdown-bfm-card-slot--inline / --inline-embed there as well. That parity is the whole point of the fix and the thing a future edit must preserve — the two derivations have no shared home, so a change to one silently drifts from the other. Extracting a single shared helper ("derive slot format + resolved style from BFM attrs") would remove the drift risk, but that's a follow-up, not this PR.
Also confirming htmlSafe(target.style) in the template is safe here: style is assembled only from bfmRefFormatAndSize, which admits width/height only after /^\d+%$/ / /^\d+$/ gates, so no free-form text reaches the style attribute.
Generated by Claude Code
2 participants
Summary
In the field's edit (compose) view, embedded cards/files didn't honor a BFM directive's format or custom size — most visibly
fitted(andisolated) width/height was ignored, so the embed rendered at the wrong height. The saved views and the edit "preview" sub-mode rendered sizes correctly, so the bug was specific to the CodeMirror compose editor.Fixes CS-12112.
Root cause
The format/size specifier (
::card[url | fitted w:400 h:300],| isolated, …) was dropped at three points in the compose pipeline:CardWidgetnever recorded the directive's format/size —toDOMemitted only id/kind/ref-type.format: kind === 'inline' ? 'atom' : 'embedded'.<RefComponent @format=… />with no sizing, andCardWidgetTarget.formatcouldn't even representfitted/isolated.Fix
Thread format/width/height through the widget → DOM data-attrs → target → template chain, reusing the existing
parseBfmSizeSpec/bfmRefFormatAndSizehelpers so the compose editor matches the saved and preview markdown renderers exactly — includingoverflow: hiddenfor fitted. Applies to both:cardand:fileembeds.Two equality guards also had to learn about the size fields so a size-only edit (e.g.
w:400→w:600) actually re-renders:CardWidget.eq(else CodeMirror reuses the stale DOM element) and the_applyTargetsstructural-identity skip check (else the Glimmer re-render is skipped).Testing
pnpm lint:typesclean;pnpm lint:jsclean.codemirror-contextintegration tests cover block fitted with explicit size, block isolated, atom/embedded defaults, and file-embed parity. Full module 51/51.codemirror embed toolbar8/8 andRichMarkdownField25/25 — no regressions.Screenshot
Before
After
🤖 Generated with Claude Code