feat(core,cli): media-use interop — shared index.md regen + description/entity on figma imports#1927
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
…on/entity on figma imports Post-release review of media-use ↔ figma coupling (spec §13.1): - figma asset imports now regenerate .media/index.md, the agent-readable inventory media-use maintains — format locked byte-identical via a cross-runner parity test against media-use's own index-gen.mjs - figma asset --description/--entity land in the manifest record, the index table, and <img alt>; component rasterize auto-describes with the node name. Named brand marks become visible to media-use's resolve --entity lookups. - spec §13.1 records the review verdict (loose coupling correct) and the follow-up queue (shared media-ledger module, global cache for figma assets, media-use version-keyed idempotency) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d23464d to
45c0a2e
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
Review — media-use interop: shared index.md regen + description/entity on figma imports
Verdict: LGTM with nits — solid interop work. The core design decisions (byte-parity via test, best-effort index regen, tuple-aware reuse, icon/image equivalence for entity hits) are all well-considered. Two findings worth addressing before merge; the rest is nits.
Findings
1. Row-selection parity gap between generators (low severity, nit)
mediaIndex.ts filters through isRow() which requires typeof value === "object" && value !== null, while media-use's readManifest() pushes every JSON.parse() result (including bare primitives like "hello" or 42 if someone hand-edited the JSONL). The byte-parity test covers the JUNK_ROW object case but not bare-primitive JSONL lines.
In practice this doesn't matter — no writer emits bare primitives — and the test guards the realistic scenarios. But if the stated goal is "selection parity, not assumed," tightening isRow to match (accept anything JSON.parse returns) or adding a bare-primitive JSONL line to the parity test would close the gap completely. Leaving as a nit since the parity test is the real guard.
2. updateRecord uses non-atomic write pattern (medium severity)
updateRecord does readFileSync → map → writeFileSync. If the process crashes between read and write, the file could be truncated/corrupted. The appendRecord path is crash-safe (append-only), but updateRecord rewrites the entire file. Consider:
- Writing to a temp file +
renameSyncfor atomicity, or - At minimum, documenting this as a known limitation.
This matters more if concurrent processes (media-use scripts + figma CLI) could both write to manifest.jsonl simultaneously. The regenerateIndex path has the same pattern but is guarded by safeRegenerateIndex's try-catch so a partial write to index.md is recoverable (regen on next import). The manifest itself is the durable record, so corrupting it during updateRecord is the real risk.
3. isFigmaManifestRecord won't match media-use rows (by design, confirming)
The type guard requires provenance.source === "figma" with fileKey and nodeId. This means readManifest() in manifest.ts intentionally filters out media-use rows (bgm, sfx, voice, non-figma images). That's correct for figma operations — but regenerateIndex needs ALL rows, and it correctly uses readJsonlValues directly instead of readManifest. Just confirming this is intentional and correct.
4. cols.desc omitted from TypeScript generator (harmless)
Media-use has cols = { id: 4, type: 5, dur: 4, dims: 5, path: 5, desc: 11 } while mediaIndex.ts omits desc. Since desc is computed but never read in either implementation (description is the final unpadded column), this produces identical output. But if media-use ever starts padding descriptions, the drift would be silent — the parity test would catch it, so this is fine as-is.
5. typesMatch — consider extracting the visual set (nit)
function typesMatch(a, b) {
if (a === b) return true;
const visual = new Set(["icon", "image"]);
return visual.has(a) && visual.has(b);
}The Set is recreated on every call. This is called in a hot path (entity search iterates all records). For a manifest with dozens of records this is negligible, but hoisting const VISUAL_TYPES = new Set(["icon", "image"]) to module scope is free and idiomatic. Pure nit.
6. Component registry-item.json fix is correct and important
The old code listed phantom ${slug}.svg paths that were never written. The new code uses frozenAssets — the actual paths the asset import wrote. This is a real bug fix that prevents agents from referencing nonexistent files. Well done.
7. Test coverage is thorough
109 figma tests + 13 media-use tests covering: reuse metadata upsert, any-tuple reuse, whitespace flattening, index regen on both paths, byte-parity vs the real generator across all format branches + junk-row selection, and the new cross-type entity hit. The byte-parity test that actually executes index-gen.mjs and asserts string equality is the right guard for format drift.
Ponytail lens (over-engineering audit)
The PR is lean. normalizeMeta is a 3-line function that does exactly what it needs to. safeRegenerateIndex is a minimal best-effort wrapper. The len helper avoids repeating String(value ?? "").length. The isRow type guard is one line. No unnecessary abstractions, no premature generalization.
The only candidate for trimming is the desc: 11 difference (mentioned above) — adding it to the TS version for visual parity would add 1 line but doesn't change behavior.
Lean already. Ship.
Diff stats
12 files changed, +418 / -33
— Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 45c0a2e.
Verdict
🟢 approve with one 🟡 nit and two low-severity concerns. The two load-bearing claims — byte-identical index-gen and any-tuple reuse — both hold up, and the parity test is real (executes skills/media-use/scripts/lib/index-gen.mjs on shared input and asserts ===). The 11/13 reuse-path findings from the max-effort review look correctly landed.
What this PR actually does
- Adds
packages/core/src/figma/mediaIndex.tsas a second implementation ofgenerateIndexContentmirroring media-use'sindex-gen.mjs, and calls it fromrunAssetImporton both mint and reuse paths via asafeRegenerateIndexwrapper (try/catch → warn, per PR body: "the import is durable before it runs"). - Replaces the
findByFigmaNode(oldest row) reuse gate withfindAllByFigmaNode(...).find(matching tuple). AddsupdateRecordto rewrite a manifest row in place by id, preserving other writers' rows. - Adds
description+entitytoFigmaManifestRecordand threads--description/--entitythroughrunAssetImport→ manifest, index.md, and<img alt>. Component rasterize auto-fillsdescriptionfrom the figma node name. component.tsnow emitsregistry-item.jsonentries at the paths files actually landed at (image_NNN.svg) instead of the phantom${slug}.svg.skills/media-use/scripts/resolve.mjs— entity gate treatsicon/imageas interchangeable via a newtypesMatchhelper.
Byte-identical format claim
Verified via the parity test at packages/core/src/figma/mediaIndex.test.ts:97-107, which builds ALL_ROWS (media-use bgm/image/icon shapes + figma image + junk row), calls the local generator, spawns node --input-type=module on the actual skills/media-use/scripts/lib/index-gen.mjs, and asserts expect(ours).toBe(theirs). Covers the durations / width×height / icon+transparent → svg / no-dims / junk-row branches. The Windows fix (pathToFileURL + resolve relative to import.meta.url) is correct — matches the shape the prior review flagged.
Manual diff of generateIndexContent between mediaIndex.ts and index-gen.mjs shows only cosmetic drift (String(x) template-literal wrappers, and media-use's unused desc: 11 col-init). Both produce identical strings for every input the parity test exercises. Rendering-every-parseable-row is preserved — no shape filter, so the file doesn't flip-flop by writer.
Nit: the local generator's formatDur / formatDims use String(r.duration) / String(r.width) while media-use relies on template-literal coercion. Same output today, but drift-prone — the two generators are now the parity-test's job forever. packages/core/src/figma/mediaIndex.ts:32-39.
Reuse dedup key
(format, scale, version) tuple compared with existing.provenance.scale ?? 1 on both sides (canonical default). File-existence check (existsSync) is preserved — a deleted frozen file falls through to re-import. .find on the filtered node rows correctly picks the first matching tuple, so pre-existing duplicate rows from the mint-forever era stop growing without needing a backfill.
Version comparison is === on the string returned by client.fileVersion(ref.fileKey) — figma's file version is a monotonic string, so equality is right. No timestamp / semver comparison needed.
Upsert semantics
packages/cli/src/commands/figma/asset.ts:79-94 — the upsert applies only if description or entity was actually supplied on the re-import and differs from the existing row. Absent flags don't clobber prior values (...(description !== undefined && { description })). --description "" normalizes to undefined and thus can't clear a stored description — a known CLI ergonomics limitation, not a bug.
Concurrency note: updateRecord is read-modify-write (readFileSync → map → writeFileSync, not atomic). If two hyperframes figma asset invocations upsert the same node concurrently, the later writeFileSync wins and the earlier update is silently lost. Both writers regenerate index.md from the file, so it stays JSONL-valid, but the losing row's metadata evaporates. Probably fine (interactive CLI, not a service), but worth noting since media-use's appendRecord is append-only and colision-free, and now this shared file has one writer that does full rewrites.
Pre-PR duplicate cleanup
PR body doesn't mention it. Existing repos with duplicate image_NNN files/rows from the pre-PR mint-forever bug keep the stale rows and files forever. The gate stops growing (verified above), and index.md dedupes visually only insofar as .find picks one, but the extra rows stay in the manifest and render as extra index.md rows. Suggest a one-line note in the PR body ("existing duplicates are not backfilled; new imports stop growing them") or a short one-shot dedup pass. Low severity — cosmetic + disk.
media-use cross-repo contract
Same repo (skills/media-use/), so no cross-repo semver problem. The shared contract is: both writers regenerate .media/index.md from the full manifest.jsonl after their own writes. Verified:
- media-use writers regenerate:
resolve.mjs:98,111,125,196+adopt.mjs:95all callregenerateIndex. - figma writer regenerates:
runAssetImport→safeRegenerateIndexon both mint and reuse paths. - Both use the same row-selection rule (every JSON-parseable row).
manifest.ts:8-12docstring explicitly points at the media-use.media/layout for interop.
New consumer of the shared entity gate — typesMatch(a, b) treats {icon, image} as one bucket in resolve.mjs:76-79,124. Sound: media-use's TYPE_DIRS already collapses image/icon/brand to images/, and figma always writes type: "image". Test at skills/media-use/scripts/resolve.test.mjs:71-102 pins the cross-type hit.
updateRecord — byte-preservation nit
Docstring at packages/core/src/figma/manifest.ts:89-90 says "preserving every other line (other writers' rows included) byte-for-byte". Implementation is split(/\r?\n/) + writeFileSync(out.join("\n")). On a CRLF-terminated manifest (Windows-native fresh write, or a repo committed with core.autocrlf=true), the \r is stripped by the split regex and the rewrite emits LF-only. JSONL stays valid, but "byte-for-byte" is stronger than the code delivers. Options: soften the docstring, or capture the terminator via a non-regex split. Nit — no functional impact.
Findings (severity-ordered)
- 🟡
packages/core/src/figma/manifest.ts:105—updateRecordsilently converts CRLF to LF on Windows-terminated manifests; docstring claims byte-for-byte preservation of other writers' rows. Soften docstring or preserve terminator. - 🟡 PR body / release notes — pre-PR duplicate rows/files are NOT backfilled. Fix stops new mints but leaves stale entries. One-line acknowledgement in the PR description (or a one-shot dedup helper) would close the loop.
- 🟡
packages/cli/src/commands/figma/component.ts:71-77— each rasterize target now triggers a fullregenerateIndex(read whole manifest + write index.md). For a component with N rasterize targets, that's N regens; O(N × manifest_size). Same shape as media-useadopt.mjs, so consistent. Could hoist to a single regen after the loop; nit only. - 🟡
packages/core/src/figma/mediaIndex.ts:32-39—String(r.duration)/String(r.width)wrappers diverge cosmetically from media-use's template-literal coercion. Parity test catches functional drift; suggest a comment noting the parity test is the contract enforcer so future maintainers don't "clean up" one side and break the other.
Test coverage
Strong. Every load-bearing branch has a test:
applies --description/--entity on a reuse hit— pins the upsert (asset.test.ts:25-40).reuses against ANY matching tuple— pins the multi-tuple gate (asset.test.ts:42-50).flattens whitespace in descriptions— pinsnormalizeMeta(asset.test.ts:52-60).matches media-use's index-gen output byte-for-byte— pins the parity contract (mediaIndex.test.ts:73-107).entity hit matches across icon/image— pins the cross-type gate on the media-use side (resolve.test.mjs:71-102).
Gaps I noticed but wouldn't block on:
- No test for the CRLF/LF preservation claim on
updateRecord. - No test for concurrent upserts to the same row (last-writer-wins is arguably the intended behavior; would just be documenting).
- No test for the "pre-existing duplicates keep working, don't grow" path — a fixture with two pre-PR rows for the same node would nail it.
Cross-repo consumer awareness
Same repo. media-use .media/index.md and manifest.jsonl are the shared surface, both correctly co-owned.
AI-trailer sweep
Clean — no Claude/Codex trailers in the PR files.
Peer awareness
No prior reviews on this PR at HEAD. Verified via gh pr view 1927 --json reviews → [].
— Rames D Jusso
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed the interop pass at 45c0a2ec. The load-bearing pieces are sound: figma asset import now regenerates the shared .media/index.md on fresh and reuse paths, threads normalized description / entity through manifest, index, and escaped <img alt>, and the reuse gate now checks all matching (format, scale ?? 1, version) tuples with file existence instead of only the oldest figma row.
The byte-parity guard is the right contract: packages/core/src/figma/mediaIndex.test.ts runs the actual skills/media-use/scripts/lib/index-gen.mjs against the same rows and asserts exact string equality. regenerateIndex intentionally reads raw JSONL values instead of figma-filtered readManifest, so media-use rows stay in the shared inventory. The registry-item.json path fix in component.ts now points to actual frozen assets, and the media-use icon/image entity bridge is scoped to entity reuse.
Nits from Miga/Rames still stand and are non-blocking: updateRecord is read-map-write / CRLF-normalizing despite the “byte-for-byte” docstring, pre-PR duplicate rows are stopped but not backfilled, component rasterize can regenerate index N times, and typesMatch can hoist its visual set.
CI is green at 45c0a2ec.
Verdict: APPROVE
Reasoning: The shared media-index and reuse semantics are correctly implemented and tested, and the remaining issues are durability/perf/documentation nits rather than merge blockers.

What
Post-release interop pass between the figma import module and the media-use skill — they share one
.media/inventory, and this PR makes the two writers actually behave like it (spec §13.1 records the full review; items 1–2 shipped here, 3–5 queued).index.mdregeneration (core)hyperframes figma assetnow regenerates.media/index.md— the agent-readable inventory table media-use maintains — after every import, fresh or reused. Newpackages/core/src/figma/mediaIndex.tsmirrors media-use'sindex-gen.mjsincluding its render-every-parseable-row selection (a shape filter would make the file flip-flop between writers). Parity is enforced, not assumed: a test executes the actualindex-gen.mjson identical rows (duration, width×height, icon+transparent, junk-row branches) and asserts byte-for-byte equality. Regen is best-effort — the import is durable before it runs, so an index write failure warns instead of failing the command.--description/--entityon figma importsBoth flags land in the manifest record, the index table, and the
<img alt>(previouslyalt="image_001"). Component rasterize auto-describes with the node name.entitymakes figma-imported brand marks discoverable by media-use'sresolve --entitycascade — and to make that promise real, media-use's entity gate now treatsicon/imageas equivalent (figma rows are alwaystype: image; agents ask for logos astype: icon). Descriptions are whitespace-flattened so a newline can't corrupt the table.Reuse-path correctness (from max-effort review, 11/13 findings fixed)
updateRecord, in-place line rewrite preserving other writers' rows) instead of being silently dropped.findAllByFigmaNode).registry-item.jsonhonesty — rasterized assets now listed at theimage_NNN.svgpaths actually frozen (and referenced by the emitted HTML) instead of phantom${slug}.svgfiles never written.file://URL (the cwd-regex + raw-path interpolation was fatal on windows-latest).Deferred (spec §13.1 follow-up queue): shared media-ledger module (the generator duplication is guarded by the parity test until then), content-addressed global cache for figma assets, media-use version-keyed idempotency.
Testing
imagerow resolved via--type icon --entity).tsc --noEmitclean on core + cli; oxlint/oxfmt clean.🤖 Generated with Claude Code