Skip to content

Add filedef-authoring skill for FileDef families and content-only rendering - #6178

Open
lukemelia wants to merge 2 commits into
mainfrom
cs-12591-document-filedef-rendering
Open

lukemelia wants to merge 2 commits into
mainfrom
cs-12591-document-filedef-rendering

Conversation

@lukemelia

Copy link
Copy Markdown
Contributor

What

Adds .claude/skills/filedef-authoring/SKILL.md, an agent skill covering FileDef authoring and file content rendering:

  • Family architecture — the four shared shells in packages/base/file-formats/, FilePreviewStage, static previewComponent as the family hook, and the pin-statics rule for prerender/hydration agreement.
  • The wrapped-default decision — the default FileDef templates are the inspector (file bar + metadata) by design; that is not a bug to fix.
  • Content-only rendering — the exported MarkdownPreview / ImagePreview / AudioPreview from the file-formats/index barrel, how to pass @model / @format / @displayContainer, kind-dispatch via filePreviewComponentFor, and the format↔@format projection invariant.
  • The anti-pattern to avoid — rendering a FileDef field with @displayContainer={{false}} plus :deep() chrome-hiding, and what to do instead.
  • Test gotchas — the file-formats/* module manifest in realm-indexing-test.gts and the fileDefFormats count assertions.

The in-app-assistant counterpart (the boxel-file-def skill in the skills realm) is handled separately.

Linear: CS-12591

…dering

Documents the file-formats family architecture (shared shells, static previewComponent), the wrapped-default template decision, the exported content-only preview components (MarkdownPreview/ImagePreview/AudioPreview) and how to embed them, the @displayContainer+:deep() anti-pattern to avoid, and the file-formats test gotchas (module manifest, fileDefFormats count).

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

Claude-Session: https://claude.ai/code/session_014GsYGsuCqHJz9GAti4jheG
Two things the published boxel-skills guidance supplies that this was
missing. A framework-driven `<@fields.file @Format='embedded' />` takes no
component args at all, so `@displayContainer` is not available to it and the
lever is an inherited custom property — markdown reads three — which is the
answer to "I want to tune a wrapped embed" that otherwise sends an author
back to `:deep()`.

The list of which families have a content-only renderer now points at the
barrel instead of restating it, so the skill cannot go stale behind it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lukemelia

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] Checked this against the published boxel-skills guidance and pushed two additions — ac06208.

  • The lever for a wrapped embed you cannot pass args to. boxel-ui-guidelines/references/delegated-render-control.md makes the point that a framework-driven <@fields.file @format='embedded' /> takes no component args at all, so @displayContainer is not available there — and the cross-boundary lever is an inherited custom property (markdown reads --md-preview-background, --md-preview-foreground, --md-preview-padding; all three verified in markdown-preview.gts). Without that, an author who wants to tune a wrapped embed is sent straight back to the :deep() this skill tells them not to use.
  • The barrel is the list. Which families have a content-only renderer now points at file-formats/index.ts instead of naming three, so the skill cannot go stale behind it — which it otherwise would the moment the video renderer lands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Several API, dependency, layering, and source-reference details need correction.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a Claude skill documenting FileDef families, content-only rendering, anti-patterns, and testing guidance.

Changes:

  • Documents shared shells and preview dispatch.
  • Explains Markdown, Image, and Audio content-only components.
  • Covers rendering pitfalls and test-manifest considerations.
File summaries
File Description
.claude/skills/filedef-authoring/SKILL.md Adds FileDef authoring and rendering guidance.
Review details

Suppressed comments (6)

.claude/skills/filedef-authoring/SKILL.md:61

  • [Claude Code 🤖] The cited field guide is itself the exact pattern this section forbids: it renders <@fields.file ... @displayContainer={{false}} /> and hides .markdown-embedded__title / unclamps .markdown-embedded__content with :deep() (packages/experiments-realm/filedef-fixtures/file-embedding-field-guide.gts:47,126-145). Linking it as a worked example sends authors to the wrong implementation; remove this reference or replace the fixture with a direct MarkdownPreview example.
This reaches into shell internals with `:deep()` to hide chrome and un-clamp content. The targeted classes are renderer-internal, not a stable contract, so the CSS breaks silently when the renderer's markup changes — and it duplicates what the content-only components already do. Render `<MarkdownPreview …>` (or `ImagePreview` / `AudioPreview`) directly instead. For worked examples, see `experiments-realm/filedef-fixtures/file-embedding-field-guide.gts` and boxel-home's `LegalDoc`.

.claude/skills/filedef-authoring/SKILL.md:32

  • [Claude Code 🤖] These names are not module-private content-only renderers in file-formats/: they are private classes in packages/base/*-file-def.gts implementing the shell's FilePreviewSignature. Calling them “the rest” of the barrel exports suggests they can be used like MarkdownPreview, but they are not exported by the barrel and do not accept a bare FileDef instance. Please describe them as family renderers or remove this list.
Which families have a content-only renderer is what the barrel exports — read `file-formats/index.ts` rather than a list here. The rest (`TextPreview`, `CsvPreview`, `JsonPreview`, `CodePreview`, `ArchivePreview`) are module-private; export one from the barrel as call sites need it.

.claude/skills/filedef-authoring/SKILL.md:14

  • [Claude Code 🤖] FilePreviewStage does not own a current-render cache or provenance overlay: its template renders thumbnails, previews/fallbacks, state panes, and stale/malformed indicators, while provenance is explicitly kept in FileIsolatedShell. Calling those concerns stage-owned can lead authors to put provenance or caching into the wrong layer.
- **`FilePreviewStage`** is the slot the shells project into. It owns the concerns every family shares — the current-render cache, loading / failure / staleness panes, provenance, and the generic fallback pane for a family with no renderer.
- **`static previewComponent`** is the family hook. A `FileDef` subclass sets `static previewComponent = XPreview`; `FilePreviewStage` mounts it. `filePreviewComponentFor(instance)` resolves the renderer a file's class declares (read off the instance's constructor, since a `linksTo(FileDef)` is routinely a subclass instance).

.claude/skills/filedef-authoring/SKILL.md:65

  • [Claude Code 🤖] The field wrapper does accept the generic @format and @displayContainer arguments (BoxComponentSignature), but it does not forward arbitrary arguments to the FileDef format component. Saying the invocation takes no component args is inaccurate and conflicts with the example immediately before it; describe the forwarding boundary instead.
To tune a wrapped embed you cannot pass args to at all — a framework-driven `<@fields.file @format='embedded' />`, which takes no component args — the cross-boundary lever is an inherited custom property, not a selector: markdown reads `--md-preview-background`, `--md-preview-foreground`, and `--md-preview-padding` from any ancestor. The boxel-skills reference `boxel-ui-guidelines/references/delegated-render-control.md` covers the general case (a `class` on the field, the theme cascade, what a child must never decorate).

.claude/skills/filedef-authoring/SKILL.md:17

  • [Claude Code 🤖] This makes a custom renderer mandatory for every new family, but FileDef.previewComponent is optional and VideoDef is a current family with no preview component; it relies on the generic fallback. Qualify this as the recipe for families that provide a custom preview so authors do not add a renderer merely to follow the skill.
So a new family = add `@field`s + a `previewComponent`. It should not touch the shells.

.claude/skills/filedef-authoring/SKILL.md:30

  • [Claude Code 🤖] The host example is not valid for host-side runtime modules: this repository loads base-realm value modules through loader.import(...) and uses @cardstack/base/... statically only for types. A static value import here can trigger the host build's missing-module failure; document the loader form for host code instead.
- Host / base TS: `import { MarkdownPreview } from '@cardstack/base/file-formats/index';`
  • Files reviewed: 1/1 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

- **Four shared shells** — `FileAtomShell`, `FileEmbeddedShell`, `FileFittedShell`, `FileIsolatedShell` (in `file-formats/`). Every family inherits these from `FileDef` and supplies only the renderer they mount, rather than shipping its own `isolated`/`embedded`/`fitted`/`atom` templates. The shells draw the chrome: the isolated shell's file bar (Download / Copy link) and the GENERAL / PARSER OUTPUT / DERIVED metadata inspector.
- **`FilePreviewStage`** is the slot the shells project into. It owns the concerns every family shares — the current-render cache, loading / failure / staleness panes, provenance, and the generic fallback pane for a family with no renderer.
- **`static previewComponent`** is the family hook. A `FileDef` subclass sets `static previewComponent = XPreview`; `FilePreviewStage` mounts it. `filePreviewComponentFor(instance)` resolves the renderer a file's class declares (read off the instance's constructor, since a `linksTo(FileDef)` is routinely a subclass instance).
- **Pin statics that pre- and post-hydration must agree on.** A prerender omits linked-card CSS, so a value the render depends on (a heading font, a first-child rule) belongs where both passes see it — see the pinning comments in `default-templates/markdown.gts` and the legal-doc masthead.

- **`@model`** takes the `FileDef` instance itself (the component projects it through `fileViewModel`) _or_ a prebuilt `FileViewModel` (what the shells pass).
- **`@format`** defaults to `'embedded'` (complete content); `'isolated'` is also complete content; `'fitted'` selects the budgeted snippet a collection cell draws. See `ContentPreviewSignature`.
- **`@displayContainer={{false}}`** opts out of the component's own container styles (padding, surface color, scroll) so the embedder owns geometry. This arg belongs to the content-only component's public signature — it is _not_ the workaround below.

## Test gotchas

- **Module manifest:** `packages/host/tests/integration/realm-indexing-test.gts` hard-codes the `@cardstack/base/file-formats/*` module-dependency list. Adding or removing a module in the barrel's dependency graph breaks those fixtures — update the manifest in the same change.
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.

2 participants