Skip to content

Markdown embed chooser: show a size-matched card placeholder while a pick loads#5602

Open
FadhlanR wants to merge 2 commits into
mainfrom
markdown-embed-preview-panel-bug
Open

Markdown embed chooser: show a size-matched card placeholder while a pick loads#5602
FadhlanR wants to merge 2 commits into
mainfrom
markdown-embed-preview-panel-bug

Conversation

@FadhlanR

@FadhlanR FadhlanR commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Background and Goal

In the markdown embed chooser modal, the right preview pane showed the empty "Search for a card/file & preview its format here" placeholder while a picked card/file was still resolving — so a slow (cold) load read as "nothing selected" rather than "loading". Reported in CS-12321.

Now, while a pick resolves, the pane stays mounted (format selector + placement/size controls) and its embed slot shows a size-matched card placeholder — the resolved embed's footprint (fitted W×H, inline embedded/isolated footprint, or atom pill) with a small centered spinner — which swaps to the real embed once it loads. The placeholder reads the shared format/size selection live, so changing the format resizes it. The CTA is disabled until the instance (or an error) lands.

Where to start

  • packages/host/app/components/markdown-embed-chooser/preview/index.gts — the Embed primitive renders a spinner in place of the card body when @loading, reusing the existing sizeStyle/footprint logic so the placeholder matches the eventual embed. Tagged data-test-markdown-embed-preview-loading (and it omits the resolved data-test-markdown-embed-preview while loading).
  • packages/host/app/components/markdown-embed-chooser/pane.gts — forwards @loading to the preview and holds the CTA disabled while loading.
  • packages/host/app/components/markdown-embed-chooser/tab-panel.gts — renders the pane whenever a pick is loading, resolved, or errored (hasPreview || isLoading) so there's no jump to a bare spinner; the empty placeholder shows only before anything is picked. loadTarget is wrapped so a superseding pick's cancelation re-throws (newer load wins) while an unexpected rejection routes to the existing broken-ref visual instead of spinning forever.

Tests

Two integration tests in markdown-embed-chooser-modal-test.gts:

  • while a pick resolves, the pane stays mounted, the sized loading placeholder shows (not the empty placeholder), and the resolved embed appears only after the load completes;
  • a load that throws surfaces the broken-ref visual rather than a stuck loading/empty state.

Notes

Scope is the loading state only — Replace and Remove are unchanged from main (Remove keeps its existing remove-and-close behavior).

Screen Recording

Screen.Recording.2026-07-24.at.19.46.34.mov

🤖 Generated with Claude Code

The right preview pane showed the "nothing selected" placeholder while a
picked card/file was still loading, so a slow (cold) load read as "no
selection" rather than "loading". Add a loading state (rendered when the
load task is running, in place of the empty placeholder), and route an
unexpected load rejection to the existing broken-ref visual so the pane
can't spin forever.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Preview deployments

Host Test Results

    1 files      1 suites   3h 0m 48s ⏱️
3 655 tests 3 639 ✅ 15 💤 0 ❌ 1 🔥
3 674 runs  3 657 ✅ 15 💤 1 ❌ 1 🔥

Results for commit 3cfa8a1.

For more details on these errors, see this check.

Realm Server Test Results

    1 files  ±  0      1 suites  ±0   13m 33s ⏱️ + 2m 18s
1 934 tests +409  1 934 ✅ +409  0 💤 ±0  0 ❌ ±0 
2 013 runs  +441  2 013 ✅ +441  0 💤 ±0  0 ❌ ±0 

Results for commit 3cfa8a1. ± Comparison against earlier commit 0dba7cc.

…lder

Instead of blanking the whole preview pane with a centered spinner, keep the
pane (format selector + controls) mounted while a pick resolves and show a
card-shaped placeholder in the embed slot: the resolved embed's footprint
(fitted W×H, inline embedded/isolated footprint, or atom pill) with a small
centered spinner. It reads the shared selection live, so changing the format
resizes the placeholder. The CTA is disabled until the instance (or an error)
lands.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@FadhlanR FadhlanR changed the title Show a loading indicator in the markdown embed chooser preview while a pick resolves Markdown embed chooser: show a size-matched card placeholder while a pick loads Jul 24, 2026

@FadhlanR FadhlanR left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Reviewed for correctness of the loading state machine, the twin render paths in the preview component, and the concurrency handling in the load task — that's where a "keep the pane mounted while a pick resolves" change is most likely to go wrong.

Bottom line: no blocking issues. The change is sound and lands cleanly. One inert CSS line worth dropping (inline), plus a couple of coverage notes below. Safe to merge as-is.

What lands right

  • showPane = hasPreview || isLoading closes the flicker gap correctly. loadTarget clears selectedTarget/selectedError synchronously before its first await, and isLoading (loadTarget.isRunning) is already true by the time any render runs — so a superseding pick transitions resolved → loading placeholder with no intermediate frame of the empty placeholder or a stale embed. On completion, selectedTarget is set inside the task body before it returns (no await in between), so hasPreview is true before isRunning flips false — no empty-placeholder flash on the way out either.
  • The didCancel(e) rethrow is correct and matches the house convention. store.get returns Promise<T | CardErrorJSONAPI> — it resolves its own load failures rather than throwing them — so the non-cancel catch branch genuinely only fires on an unexpected throw, and routing that to the broken-ref visual is the right call (no infinite spinner). The rethrow-cancellation-inside-the-task-body shape mirrors resources/search.ts (if (didCancel(err)) throw err;), so it's consistent with how the rest of the host handles restartable-task supersession.
  • target? made optional + {{#if @target}} guard on both the inline (<span>) and block (<div>) render paths — the loading branch was added to both twins, so inline/block placement stays symmetric. Embed is module-local, so there are no external callers to break.
  • The size-matched footprint reuses the existing sizeStyle/frame logic, so the placeholder tracks the live format/size selection — exactly the design goal.

Coverage notes (non-blocking, follow-up)

  1. The headline behavior — that the placeholder is size-matched (carries the resolved embed's footprint/format, not just a bare spinner) — isn't asserted; the tests only check that [data-test-markdown-embed-preview-loading] exists. Consider asserting the loading element also carries the fitted style/frame class for the picked format so a future refactor can't silently drop the footprint.
  2. The trickiest added logic — a superseding pick canceling an in-flight load (the didCancel rethrow path) — has no test. A test that gates pick A, fires pick B before A resolves, then releases both and asserts B's target wins with no error state would pin that contract.
  3. ctaDisabled returning true while @loading has no direct assertion; cheap to add alongside test 1.

Adjacent, out of scope

The working tree also has uncommitted edits to two experiments-realm JSON files and an untracked docs/cs-*-plan.md; none are in this PR's diff, just flagging so they don't get swept into a later commit — the plan doc in particular shouldn't land in merged history.

Comment on lines +144 to +147
/* Small enough to sit inside an atom pill, unobtrusive in larger frames. */
.markdown-embed-preview__spinner {
--boxel-loading-indicator-size: 1.25rem;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Non-blocking cleanup. This override is a no-op: LoadingIndicator sizes itself from var(--boxel-loading-indicator-size, var(--boxel-icon-sm)), and --boxel-icon-sm is defined as 1.25rem in boxel-ui's variables.css — so setting --boxel-loading-indicator-size: 1.25rem here just re-states the default. The rendered spinner is the same size with or without this rule (and the class='markdown-embed-preview__spinner' that carries it). Either drop both, or — if a distinct size was intended — pick a value that actually differs from the default (compare operator-mode/workspace-chooser/workspace.gts, which sets 2rem for a genuine override). The comment (“small enough to sit inside an atom pill”) also reads as if it shrinks the spinner, which it doesn't.

@FadhlanR
FadhlanR marked this pull request as ready for review July 24, 2026 13:06
@FadhlanR
FadhlanR requested a review from a team July 24, 2026 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant