Skip to content

feat(client): support gif and video demos in help articles - #806

Draft
cgtorniado wants to merge 4 commits into
mainfrom
feat/help-media-embeds
Draft

feat(client): support gif and video demos in help articles#806
cgtorniado wants to merge 4 commits into
mainfrom
feat/help-media-embeds

Conversation

@cgtorniado

Copy link
Copy Markdown
Contributor

Summary

Plumbing for embedding demo media in Help Center articles (part of #24). Authors write plain markdown image syntax — ![Create a project](./media/create-project.gif) or .webm/.mp4 — and the help renderer does the rest. A follow-up PR will add the actual demo clips.

Renderer (apps/client)

  • New img override in the help markdown pipeline dispatches by extension:
    • GIFs/images render with native loading="lazy".
    • .webm/.mp4 render as gif-style videos (autoplay, muted, looping, controls for pause/scrub) that only mount once scrolled near the viewport (IntersectionObserver, 200px root margin), so media never affects initial Help Center load.
  • Relative media paths resolve against the article's file path (same base the validator uses), rewritten to /help-content/....
  • Styling uses Joy palette CSS variables, so light/dark mode both work. No sanitizer schema changes were needed — media uses the already-allowed img node.

Bundler (packages/scripts)

  • help:bundle-content now copies media referenced by bundled articles into apps/client/public/help-content/ (CDN-served via CloudFront on deploy) and cleans up stale assets.

Guards (help:validate, runs in CI + pre-commit)

  • Size caps as build errors: 1MB images, 3MB GIFs, 10MB videos.
  • Web-playable formats only — .mov/.avi/etc. are rejected with guidance to convert to .webm/.mp4.
  • Externally hosted media is rejected (must be committed to the docs tree).
  • Asset paths that escape the docs tree are rejected.

Testing

  • New media tests render through the exact exported production pipeline (extension dispatch, path resolution, lazy mount via mocked IntersectionObserver).
  • New validator tests for every guard; full 83-article corpus passes help:validate and renders through the corpus test.
  • Smoke-tested the bundler end to end with a temporary GIF: copied on reference, removed as stale when dereferenced.
  • typecheck, test, and lint green for both touched packages.

🤖 Generated with Claude Code

Help articles can now embed demo media with plain markdown image syntax:
GIFs and images render lazily, and .webm/.mp4 clips render as lazy,
gif-style looping videos (autoplay, muted, click-to-pause) that only load
when scrolled near the viewport. Relative media paths resolve against the
article's file path, matching link resolution.

The bundler now copies referenced media into public/help-content/ so it
ships on the app CDN, and the validator gains media guards: size caps
(1MB images / 3MB GIFs / 10MB videos), web-playable formats only,
no external hosts, and no paths escaping the docs tree.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cgtorniado
cgtorniado requested review from a team as code owners July 22, 2026 05:59
claude[bot]
claude Bot previously approved these changes Jul 22, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Automated review — approve.

Plumbing only for now (the demo clips land in a follow-up), and the seams are drawn where you'd want them: VIDEO_EXTENSIONS lives once in packages/scripts/help/utils.ts and is imported by all three consumers (renderer, validator, bundler) with an explicit "must stay in sync" comment; resolveAssetPath is shared between validator and bundler so they agree on where an asset lives on disk (and both refuse traversal via path.relative starting with ..). Renderer resolution mirrors the validator by resolving relative paths against the article's file path rather than its slug, matching the existing resolveHelpLinkSlug pattern.

Renderer correctness spot-checks:

  • resolveHelpMediaSrc./, bare, ../, /, and http(s):///// are each handled distinctly; resolveRelativePath's .md-only strip leaves .gif/.webm intact, so features/projects.md + ./media/x.webm correctly resolves to /help-content/features/media/x.webm. // scheme-relative passes through untouched (validator will reject it at build time — the renderer intentionally stays tolerant).
  • HelpVideo — lazy-init useState(() => typeof IntersectionObserver === 'undefined') means jsdom / very old browsers load immediately with no observer construction; when the observer fires, setInView(true) and disconnect() both run, and the useEffect cleanup's second disconnect() on next render is idempotent. containerRef is on the outer <span> (not the placeholder), so it stays valid across the inView flip.
  • Sanitizer interaction — <img> still passes rehype-sanitize's default allow-list; HelpMedia then rewrites the img HAST node into either <img loading="lazy" decoding="async"> or <video> in React output, which the sanitizer never sees. loading/decoding are safe here because they come from JSX, not raw HTML.
  • Markdown wrapping — using <span style={{display:'block'}}> (not a <div> or <Box>) preserves valid nesting since markdown wraps images in <p>, which only accepts phrasing content. <video> itself is embedded content and legal inside <p>.

Validator/bundler correctness:

  • The unsupported-extension check (.mov/.avi/…) runs before the asset-existence branch so a stray [link](demo.mov) produces the guidance message instead of a confusing broken-link error — a subtle but nice touch.
  • Size-cap branch only runs when mediaSizeLimit matches, so .pdf (in ASSET_EXTENSIONS, not in MEDIA_SIZE_LIMITS) is intentionally uncapped — reasonable, PDFs are downloads rather than embeds. size !== undefined guards a missing stat too.
  • External media is rejected but external document links are still allowed — the test exercises both in one article and confirms exactly that (nice regression fence).
  • Bundler's assetRelPaths is a Set keyed on path.relative(DOCS_ROOT, absSource), and every copied asset feeds into expectedFiles, so the stale-file sweep cleans up dereferenced media — confirmed matches the "removed as stale when dereferenced" line in the PR description.

Tests render through the exact exported remarkPlugins / rehypePlugins / markdownComponents (no drift), and the IntersectionObserver mock covers both the "deferred until intersection" and "no IO available" paths. beforeEach sets useHelpPanel's currentFilePath but never restores it — Zustand state persists across tests in the same worker, so this could leak into unrelated help tests; consider clearing it in an afterEach, though risk is low since other help tests set their own state before use.

Minor nits, not blockers:

  • HelpVideo sets aria-label={label} unconditionally; when the markdown alt is empty (![](x.webm)), this becomes aria-label="", which screen readers can treat as an explicit empty accessible name. aria-label={label || undefined} on both the video and the placeholder would be cleaner.
  • No unit test for bundle-help-content.ts's asset copy / stale-sweep — smoke-tested per the PR description, and the logic is thin, but a fixture-driven test would catch future regressions cheaply.
  • Placeholder has minHeight: 180 but no aspect-ratio hint — a wide video will pop layout on load. Not visible until real clips land, and easy to revisit then.

Public-repo hygiene, secrets, ASCII-only rules all clean. Approving.

Omit aria-label when markdown alt is empty, give the video placeholder a
16:9 aspect-ratio default instead of a fixed min-height to reduce layout
pop, and cover the bundler's asset copy and stale sweep with a
fixture-driven test (paths are now injectable and the CLI entry point is
guarded like validate-help-content.ts).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cgtorniado cgtorniado added the bot-review Trigger Claude bot re-review (auto-removed after run) label Jul 22, 2026
claude[bot]
claude Bot previously approved these changes Jul 22, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Automated review — approve.

The prior review's nits are cleanly addressed:

  • aria-label={label || undefined} on both the <video> and the placeholder — no more empty aria-labels leaking to screen readers.
  • Placeholder switched from a fixed minHeight: 180 to aspectRatio: '16 / 9' + width: 100%, so a wide screen-recording won't pop layout when the real video swaps in (off-ratio clips still shift, as the comment notes).
  • New fixture-driven bundle-help-content.test.ts covers article copies, referenced-asset copies (relative, absolute, and parent-relative resolved via resolveAssetPath), silent skips for external/missing/traversal refs, the stale sweep AND the emptied-media-directory prune, and the help-index.json not found throw. bundleHelpContent is now exportable and the CLI entry-point is guarded (process.argv[1]?.endsWith('bundle-help-content.ts')) the same way validate-help-content.ts does it, so importing from tests doesn't trigger a run.

Fresh spot-checks on the delta since the last review:

  • The throw new Error('help-index.json not found. Run help:build-index first.') swap from process.exit(1) composes cleanly with the test's rejects.toThrow('help-index.json not found') assertion, and preserves the original error message.
  • assetRelPaths (Set on path.relative(docsRoot, absSource)) plus expectedFiles.add(destPath) for each copied asset makes the stale sweep sound; the sweep-test confirms both file removal and empty-directory prune together.
  • isExternal and hasAssetExtension are now exported from validate-help-content.ts for reuse in the bundler — the bundler's !link.isImage && !hasAssetExtension(pathPart) skip matches how the validator classifies assets, so the two agree on what to bundle vs skip.

One concern to carry into the follow-up media PR (not a blocker for this plumbing-only PR, since no help article currently uses image syntax):

HelpMedia reads useHelpPanel.getState().currentFilePath at render time (HelpContent.tsx:312), but HelpContent only pushes filePath into the store from a useEffect (HelpContent.tsx:344-346) which fires after commit. getState() doesn't subscribe, so once HelpMedia has computed resolvedSrc from the stale value, the effect's later setCurrentFilePath doesn't cause it to re-render — the wrong URL is what the browser sees. handleLinkClick uses the same getState() pattern safely because clicks fire long after effects have run; the render-time read in HelpMedia doesn't have that grace period.

Concrete failure once real media exists:

  1. User previously visited admin/settings.md (contains ![](./media/setup.gif)); useHelpContent's query for that slug is now cached (5-min staleTime / 30-min gcTime).
  2. User is currently on features/notebooks.md; store's currentFilePath = 'features/notebooks.md'.
  3. User navigates back to admin/settings. useHelpContent('admin/settings') returns cached data synchronously — NO isLoading spinner phase between the old and new article's renders.
  4. On that first render with the new content, HelpMedia reads currentFilePath = 'features/notebooks.md' (stale), so resolveHelpMediaSrc('./media/setup.gif', 'features/notebooks.md') returns /help-content/features/media/setup.gif — pointing at features/, not admin/. Since that file doesn't exist in features/media/, the browser 404s and the demo silently doesn't render.
  5. HelpContent's effect then fires setCurrentFilePath('admin/settings.md'), but HelpMedia doesn't subscribe, so nothing re-renders and the wrong src sticks.

The current tests don't exercise this timing gap: HelpContent.media.test.tsx renders via ReactMarkdown directly (not through HelpContent) and pre-sets currentFilePath in beforeEach, and HelpContent.test.tsx mocks useHelpPanel entirely.

Any of these fixes closes it:

  • Subscribe in HelpMedia: const currentFilePath = useHelpPanel(s => s.currentFilePath) — one re-render per article after the effect updates the store; harmless with loading=\"lazy\"/deferred-video since the browser hasn't fetched yet.
  • Thread filePath through a React context that HelpContent provides from its useHelpContent result, so HelpMedia never touches the store for this.
  • Compute filePath inside HelpMedia from the current slug via useHelpIndex() directly, which cuts the store dependency entirely.

A regression test that navigates HelpContent between two articles in different directories (with cached content, so no spinner phase) and asserts the resolved src would fence this.

Public-repo hygiene, ASCII-only, secrets: clean.

@github-actions github-actions Bot removed the bot-review Trigger Claude bot re-review (auto-removed after run) label Jul 22, 2026
HelpMedia read useHelpPanel.getState().currentFilePath at render time, but
the store is only synced from a post-commit effect. On a cached-content
navigation (no loading phase between articles) the first render read the
PREVIOUS article's path, computed a wrong /help-content URL, and never
re-rendered to correct it. Thread filePath from useHelpContent through a
context instead, so media resolution is synchronous with the article being
rendered and independent of store timing. Click-time link resolution keeps
using the store, which is safe because effects have long since run.

Adds a regression test that renders HelpContent with a deliberately stale
store value and asserts the media src resolves from the article's own path,
plus coverage for the empty-alt aria-label omission.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cgtorniado cgtorniado added the bot-review Trigger Claude bot re-review (auto-removed after run) label Jul 22, 2026
claude[bot]
claude Bot previously approved these changes Jul 22, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Automated review — approve.

The store-timing race the last commit tackled is worth calling out on its own merits: HelpMedia used to read useHelpPanel.getState().currentFilePath at render time, but on a cached-content navigation (no loading phase — useHelpContent returns the new article synchronously) the store's post-commit effect hasn't run yet, so the first render would compute a 404 URL against the PREVIOUS article's directory and never self-correct. Threading filePath through HelpArticleFilePathContext from HelpContent fixes it, and the new HelpContent.media.test.tsx:154 test pins it down with a deliberately stale store — good regression coverage for a bug class that would otherwise be silently flaky.

Rest of the pipeline looks right:

  • VIDEO_EXTENSIONS, resolveAssetPath, hasAssetExtension, and isExternal all live once (packages/scripts/help/utils.ts / validate-help-content.ts) and are shared by renderer, validator, and bundler — the cross-reference comments spell out the sync contract.
  • Path traversal is enforced with rel.startsWith('..') || path.isAbsolute(rel) in the shared resolver.
  • HelpVideo short-circuits the observer when IntersectionObserver is undefined (jsdom / very old browsers), and disconnects after the first intersection — both covered by explicit tests.
  • Size caps, unsupported-format guidance, and no-external-media are validator-enforced with dedicated cases each.
  • Bundler expands expectedFiles with copied assets before the stale-sweep so referenced media survives cleanup, and the sweep test confirms deref → removal + empty-dir pruning.

Minor observations, non-blocking:

  1. External non-image hyperlinks with asset extensions now trip "external media" as well. [Handout](https://example.com/x.pdf) — a plain link (no !), not an embed — falls into the new external-media branch because hasAssetExtension('.../x.pdf') is true, so it errors with External media is not allowed. Commit the file next to the article.... That message assumes embedding; a plain hyperlink to an external file isn't an embed. Corpus doesn't hit this today (help:validate passes), and the intent is clearly to lock down the media surface, so noting it as future guidance rather than a bug.

  2. .pdf is in ASSET_EXTENSIONS but has no MEDIA_SIZE_LIMITS entry, so an oversized PDF embed wouldn't get flagged (existence is still checked). Nothing embeds PDFs today so this is theoretical, but worth deciding whether PDFs should get a cap when authors start using them.

  3. The aspect-ratio: 16 / 9 placeholder honestly notes it will reflow slightly for off-ratio clips. When the demo clips land in the follow-up PR, worth capturing per-clip aspect ratios (or standardizing on 16:9) so the placeholder-to-video swap stays layout-stable.

LGTM.

@github-actions github-actions Bot removed the bot-review Trigger Claude bot re-review (auto-removed after run) label Jul 22, 2026
A plain hyperlink to an external file ([handout](https://.../x.pdf)) is not
an embed and no longer trips the external-media error - only image-syntax
embeds must be committed to the docs tree. PDFs get a 5MB size cap since
referenced ones are bundled into the deploy like other assets.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cgtorniado cgtorniado added the bot-review Trigger Claude bot re-review (auto-removed after run) label Jul 22, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Automated review — approve.

Tight follow-up that closes both actionable items from the last review:

  • External-media guard now scoped to embeds only. validate-help-content.ts:297 narrows the reject condition from link.isImage || hasAssetExtension(pathPart) to just link.isImage, so [handout](https://example.com/guide.pdf) — a plain hyperlink, not an embed — no longer trips the "external media is not allowed" error. The updated test at __tests__/validate-help-content.test.ts:257-271 exercises all three shapes in one article (external embed rejected, external doc link allowed, external file hyperlink allowed) and now also pins the finding's line to `1` so a future regression that misidentifies which of the three tripped can't hide behind a passing count.

  • PDFs get a bundle-size cap. `MEDIA_SIZE_LIMITS.pdf` at 5MB (`validate-help-content.ts:66`) plugs into the existing `mediaSizeLimit` iteration without touching the asset-existence branch — `.pdf` was already in `ASSET_EXTENSIONS`, so the flow (resolve → exists → size) now finds a limit entry and enforces it. The new test at `tests/validate-help-content.test.ts:275-284` injects `fileSize` returning `MEDIA_SIZE_LIMITS.pdf.maxBytes + 1` and confirms one media finding whose message contains "pdf".

Spot-checks on the delta:

  • External `.mov`/`.avi` links loosened, but harmlessly. Before, `demo` would have been rejected by the old `hasAssetExtension` branch; now it's an ordinary allowed external link. The unsupported-format guidance (`convert to .webm or .mp4`) exists specifically for inline playback, which external links don't do, so the message wouldn't have applied anyway. No regression concern.
  • PDF cap is local-only by construction. External `.pdf` hyperlinks bypass the size-check branch (they short-circuit at the `isExternal` guard), so the cap only affects PDFs that will actually be bundled — which is what the comment on `MEDIA_SIZE_LIMITS.pdf` claims. Matches the bundler behavior at `bundle-help-content.ts:150-158`, where `!link.isImage && !hasAssetExtension(pathPart)` continues past non-image non-asset targets; a local `.pdf` reference gets copied through the same asset-copy + stale-sweep pipeline as images and videos.
  • `isExternal` short-circuit ordering intact. The unsupported-format check on line 311 sits below the external-media branch's `continue`, so an external `.mov` link never reaches it — consistent with the "external is external, format guidance is for local embeds" split.
  • Comment on `MEDIA_SIZE_LIMITS.pdf` correctly documents why (`aren't rendered inline but ARE bundled when referenced`) rather than restating the code — matches the repo's comment hygiene rule.

The remaining nit from the last review — the `aspect-ratio: 16 / 9` placeholder pop-in for off-ratio clips — is deferred to the follow-up demo-clips PR by design (no clips exist yet to measure against), which the PR description already flags. No new hygiene issues; ASCII-only, no secrets, public-repo-safe. LGTM.

@github-actions github-actions Bot removed the bot-review Trigger Claude bot re-review (auto-removed after run) label Jul 22, 2026
@cgtorniado
cgtorniado marked this pull request as draft July 22, 2026 10:03
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