Skip to content

feat(crews): play Lottie and sprite packs on crew avatars - #10108

Merged
bolichen97 merged 1 commit into
mainfrom
feat/pack-player
Sep 12, 2026
Merged

feat(crews): play Lottie and sprite packs on crew avatars#10108
bolichen97 merged 1 commit into
mainfrom
feat/pack-player

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

A crew could only wear an SVG appearance pack. Import a Lottie or sprite pack and its card greyed out with "Not supported for crews yet" — the pack installed fine and then could not be used.

Why it matters

A pack is how somebody else's character becomes your crew's face. Two of the three formats the library accepts were dead ends, so a user could import art and then be told no.

What changed (motivation → approach → change)

Every format the library holds now renders on a crew.

The face was a plain <img>, and an <img> cannot play a Lottie document or step a sprite sheet. Crew Companion already had both players, but Crew Companion is an optional app and core must not import from apps/ — so the players moved into core (components/appearancePacks/) and the Companion imports them back. PackAvatar reads the pack through React Query (usePackDetail, one request per pack however many avatars wear it, and an invalidation from the Library tab reaches every mounted roster row), resolves the state through the pack's own fallback chain, and picks a player from the resolved slot's format. The format has to be read from the pack because it is a property of the SLOT, which the per-slot route cannot answer before the request. That read inlines every file, and two of the three tiers never use the bytes, so packDetailFrom keeps content only for a Lottie slot; the cache never pins an svg or a base64 sheet that the slot route serves anyway. A content-free detail variant is the backend follow-up (#10195) if real packs prove heavier than the 1-4 KB samples.

The two JS players are bounded by visibility, by the user's motion preference, and by state. (The svg tier is a plain <img> on the slot route, as the picture tier already is; a document inside an <img> is reachable by neither the stylesheet nor a flag on the component, so a pack SVG that animates itself animates as any image does — bounding it would mean fetching and inlining every svg slot as a document, the cost the <img> was chosen to avoid.) Each avatar observes its own box (through a ref callback, so the fresh box a recovery mounts is observed too) and holds frame 0 while off screen — autoplay={false} on a Lottie instance, one drawImage and no timers on a sprite — and holds it everywhere when prefers-reduced-motion is set, because both players are JS-driven and the stylesheet's global rule cannot reach them. The bound is visibility rather than a size threshold because the dense roster's own avatars are 38px, so any threshold low enough to animate the crew card would animate every row in a list of dozens at once.

idle holds its first frame even on screen; motion is for a reaction. A roster of dozens of looping idle faces makes motion the wallpaper of the page, so an avatar moves while a turn runs, when it finishes, on an error, or for an author-named random clip — which is also what tells the eye something happened. The rule is on the requested state, not the art: a pack that draws only idle still moves while a turn runs. Library thumbnails ask for idle, so they are stills, and the Library hint says so ("Animated ones move while the crew is working") so a fresh pack sitting still is not read as broken. This is a product call the UX lane asked a human to weigh; it is one line in PackAvatar (state !== 'idle') if the other way is wanted.

flowchart LR
  subgraph Before
    A1[crew wears a pack]:::ctx --> B1["&lt;img src=/slot/{state}&gt;"]:::removed
    B1 --> C1[svg draws]:::ctx
    B1 --> D1[lottie / sprite greyed out]:::removed
  end
  subgraph After
    A2[crew wears a pack]:::ctx --> B2[PackAvatar reads the pack]:::added
    B2 --> E2["&lt;img&gt; for svg"]:::ctx
    B2 --> F2[LottieRenderer]:::added
    B2 --> G2[SpriteRenderer on its row]:::added
  end
  classDef added fill:#DCFCE7,stroke:#16A34A,color:#14532D,stroke-width:2px
  classDef changed fill:#FEF3C7,stroke:#D97706,color:#78350F,stroke-width:2px
  classDef removed fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D,stroke-dasharray:4 3
  classDef ctx fill:#E0F2FE,stroke:#0284C7,color:#0C4A6E
  linkStyle 0,2 stroke:#DC2626,stroke-dasharray:4 3
  linkStyle 3,5,6 stroke:#16A34A,stroke-width:2px
Loading

🟩 added · 🟨 changed · 🟥 removed · 🟦 unchanged

A pack is read before it is drawn, so a Lottie clip and a sprite row reach a player that can draw them.

A sprite config a sheet cannot be cut by is dropped at the read boundary. The store keeps manifest.sprite as the bundle wrote it, and SpriteRenderer counts frames as naturalWidth / frameWidth — so a hand-written "frameWidth": "0" was Infinity frames and a trailing-frame scan that never returned, on the main thread. packDetailFrom now keeps frameWidth/frameHeight only as integers of at least one (a fraction is dropped, not floored) and fps only as a positive finite number, dropping anything else so the renderer's default applies; and SpriteRenderer refuses the same geometry itself before fetching the sheet, reporting through onError, which also covers the Companion's own manifest reader. Once the sheet has decoded it also refuses a row of more than 512 frames: the empty-trailing-frame scan is one synchronous getImageData per candidate frame, so a 1px frame over a wide transparent sheet was thousands of readbacks with no early exit.

A Lottie clip that would make the player fetch something is refused. A pack's .json is third-party art on the gateway's own authenticated origin, and lottie-web resolves a document's assets and fonts by requesting them — the light player only removes expression evaluation. lottieSafety.ts::referencesRemoteAsset allows a reference only when it is provably inline (e: 1 and a data: URI, no u prefix) and refuses everything else before loadAnimation; the avatar then falls back to the seeded ghost, the same answer a missing pack gets.

A failed read follows the shared client's retry policy, and a query left in error state is refetched on the next focus, reconnect or Library invalidation — but only while the component that asked is still mounted to receive the answer. So PackAvatar draws the caller's fallback in place and stays mounted; CrewAvatar hands in the seeded ghost rather than latching the pack tier off the way it latches a broken picture. That is what makes an unreachable-gateway blip during a roster render a ghost only until the gateway is back, not until the tab is reloaded; a real 404 simply fails again. onError fires on the edge only, so a caller re-rendering while the pack is failed is not told twice, and a renderer refusal is reset when the art changes, so a re-import under the same id is tried afresh. Pinned through CrewAvatar with the real renderer and hook (CrewAvatarPackRecovery.test.tsx), not the renderer alone. Malformed art counts as unreadable too — the importer only checks that a pack's .json is non-empty and that a sheet is a PNG, so LottieRenderer refuses a document that is valid JSON but not a Lottie ({} parses and then fails inside the player with no event anyone listens to), catches a document the player throws on (layers: null passes the presence check and throws synchronously inside loadAnimation, before any listener exists), routes the player's own error/data_failed events to the caller, and reports a parse failure the same way; SpriteRenderer reports a sheet that will not decode. Either way the crew gets the ghost, not a blank tile, and a Library card whose art cannot be drawn shows an inline ErrorNotice in place of the tile that names the Import button as the recovery (no agent hand-off: one pack's art in an unsaved draft). While the read is pending the box shows the dashboard's loading skeleton.

Two things ride along, both forced by the boundary rule. splitOnPlaceholder sat in apps/crew-companion while pages/settings/McpManagement.tsx imported it; it moved to lib/, which is what lets the new boundary test hold. And the screenshot harness gains a formats scene fed from the shipped sample bundles, so the evidence below shows art a real import can produce.

Pack-carried sounds are deliberately not played here, in code or in fixtures. The backend half landed in #10087GET /api/appearances/{id}/sound/{state} serves a pack's cue today — but the client cannot honour a user turning it off: the editor's per-state "No sound" DELETES the key and normalizes a stored 'none' away on the next Apply, so "chose silence" and "chose nothing" are one stored value, and a cue that plays wherever the record says nothing would play through the user's own "No sound". The client half waits on the editor being able to express "no pack cue", and ships together with that control.

Tests

  • PackAvatar.test.tsx — format dispatch per tier, the slot fallback chain, one pack read however many avatars wear it, re-read after the library invalidates it, the retry and the give-up after it, recovery once the query is refetched, onError on an unreadable pack / an empty pack / a broken image / an unparseable clip, and the visibility bound in all three states (off screen, scrolled in, no observer at all).
  • usePackDetail.test.tsx — the query configuration: one request across mounts, the shared retry policy inherited rather than pinned, an error state that is not kept, and an invalidation that reaches a subscriber that is still MOUNTED.
  • lottieSafety.test.ts — 15 cases, both directions: embedded and precomp assets pass; external u prefixes, relative paths, e: 1 with a path, a missing e, a u beside a data URI, webfonts by path and by non-local origin are refused; junk is total.
  • appearancePackSlotResolution.test.ts — the fallback chain, and its parity with dashboard/appearances.py read out of that module rather than restated.
  • LottieRenderer.test.tsx / SpriteRenderer.test.tsx — the moved players from their new path: lifecycle, the paused first frame, the parse-failure, not-a-Lottie-shape and remote-asset refusals reported to the caller, a synchronous loadAnimation throw caught and reported rather than crashing the tree, the player's own failure events routed to onError, sprite row offset, negative-row refusal, the empty-trailing-frame count, geometry the sheet cannot be cut by (zero, negative, NaN, a string, a fraction) refused before the sheet is fetched, and a row of more frames than the ceiling refused before it is scanned (a row at the ceiling still scans).
  • appearancePackSlotResolution.test.ts also pins that the bytes are kept only for a Lottie slot (an svg or sprite slot keeps its format and an empty content, and the chain still lands on it), and the sprite-config boundary: whole-pixel dimensions and a positive finite rate kept, a string / zero / negative / NaN / Infinity / fraction / boolean width dropped, a non-object row map dropped.
  • CrewAvatarPackRecovery.test.tsx — through CrewAvatar with the REAL renderer and hook: both reads fail → the seeded ghost and one report; the query is refetched → the art, no second report; a fresh callback identity while failed is not a new failure; a recovered pack failing anew reports once more.
  • CrewAvatarLibraryTab.test.tsx — a lottie or sprite card is selectable, its thumbnail routes through PackAvatar while an svg card stays one request, an import invalidates the pack's query, a card whose art fails says so in place of the tile, and every card carries a radio ring — filled beside "Selected", empty on the rest — so the grid reads as one group.
  • appearancePacksCoreBoundary.test.ts — no core module imports from apps/crew-companion or apps/mochi, with a non-vacuity assertion so a renamed directory cannot empty the scan.
  • appearancePackFixtures.test.ts — the three sample bundles stay importable, their art stays readable by the player that will draw it, and the sprite sheet's two rows are two different SHAPES (compared as opaque masks), so a frame of it proves the row is being read.
  • PackAvatar.test.tsx also pins that the caller's fallback is drawn in place for every kind of failure (read, no slot, renderer), that a re-imported pack is tried afresh after its previous art was refused, and that the box a recovery mounts gets its own observer (the visibility bound survives a failure); that prefers-reduced-motion holds the frame even on screen and resumes when the preference flips, that idle holds its frame on screen while a reaction plays (and that a reaction resolving to the idle clip still plays), that a sprite sheet which will not decode reports and falls back, and that the pending box is a skeleton.
  • Every guard in this PR was revert-verified: mutating the remote-asset refusal, the Lottie shape check, the Lottie error report, the retry, the idle hold, the sprite-config boundary, the renderer's geometry refusal, the in-place fallback, the edge-only report, the fresh-chance reset, the frame-count ceiling, the loadAnimation catch, or the per-node observer each turns a test red.

Manual verification

kirocrew pod up pack-player --provision --seed minimal --json

Open the dashboard the pod prints, then in a crew's avatar builder → Library, import each of website/src/test/fixtures/appearance-packs/sample-svg.json, sample-lottie.json, sample-sprite.json. Wear one on a crew and run a turn:

  • expect the idle art still; the working art plays while the turn runs, then the done art;
  • scroll the crew far out of the roster → the avatar holds a still frame;
  • turn on "reduce motion" in the OS → every Lottie and sprite avatar holds its first frame, on screen or not;
  • import a Lottie whose assets names an https:// image → the crew shows the seeded ghost and the console names the refusal;
  • import a bundle whose manifest.sprite.frameWidth is the string "0" → the crew shows the ghost, and the tab does not hang.

Screenshots / video

Every format worn on a crew, at idle and at working. The lottie tiles are real
lottie-web documents and the sprite tiles are a real canvas stepping the row
that slot is assigned to — the harness refuses to photograph a blank canvas or an
unresolved box, so a tile here cannot be a placeholder.

Every pack format worn on a crew, dark theme

Light theme, and the rest of the surface

Every pack format worn on a crew, light theme

The Library tab: every card is selectable now, each with its own art — a lottie
and a sprite card draw through the renderer, an svg card stays one request — and
every card carries a radio ring, filled on the chosen one, so "Selected" is one of several.

The avatar builder Library tab, dark theme
The avatar builder Library tab, light theme

A pack whose art cannot be drawn (Glitch: a .json that is valid JSON but not
a Lottie document). The tile says so and names the recovery, at the 10px floor;
the card stays selectable so the crew editor's own warning can name the pack.

The Library tab with a pack whose art could not be drawn

One crew wearing an svg pack across all four states, plus the two locally-composed
ghost fallbacks (the built-in pack, and a pack that is gone).

A crew wearing an svg pack across idle, working, done and error

Regenerate them with the harness this PR updates:

cd website && npx vite --host 127.0.0.1 --port 6832 --strictPort   # in another shell
node scripts/capture-crew-pack-avatars.mjs http://127.0.0.1:6832 ../temp-screenshots/crew-pack-avatars

Related Issues

no linked issue: the three items of this work were handed over as a spec, not as tracked issues.

Follow-ups filed from review: #10195 (a content-free detail read / format map on the listing), #10249 (Mochi's vendored LottieRenderer is the one loadAnimation sink without the remote-asset fence; it draws only bundled presets today), #10271 (warn at import when a Lottie names a remote asset).

Completes what #4261 (refactor(apps): share sprite renderer, merged) started: that PR collapsed the two app copies of the sprite renderer into src/apps/shared/, a destination still inside apps/ that core cannot import. This PR moves that one copy into core (components/appearancePacks/SpriteRenderer.tsx) and deletes apps/shared/; the Companion imports core directly and Mochi's one-line re-export shim points at core, so the vendored importers' paths are untouched. The willReadFrequently hint #4261 added to the probe canvas rides along.

@iamwhatever
iamwhatever requested a review from a team September 11, 2026 10:31
@iamwhatever
iamwhatever requested a review from a team as a code owner September 11, 2026 10:31
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Intent: Let a crew wear any appearance-pack format the library holds — a Lottie document or a sprite row, not only an SVG — and let the pack carry its own per-state cue, so somebody else’s character can be a crew’s face and voice.
Not a goal: Changing the pack format, the library routes, the ghost tier, the picker’s layout, or Crew Companion’s own behaviour.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of f18abb3c098d0de8c10f57a5541f911ecf4628c1 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The diff matches the description in every area I sampled: the players moved into core with a pinned boundary test, the React Query read with staleTime: Infinity and invalidation, the conservative lottieSafety refusal before loadAnimation, the sprite-geometry validation at the read boundary plus the renderer's own refusal, the in-place fallback that keeps the query observed, and the spec (learn-cron-dashboard.md) updated in the same commit as the routing table requires. The known residual costs (full-pack detail read for heavy packs, Mochi's unfenced vendored renderer, import-time warning) are each tracked in filed follow-up issues (#10195, #10249, #10271), and the one genuine product judgment (idle holds its frame) is explicitly flagged for human weighing, reversible in one line, and pinned by tests. No phantom claims, no smuggled scope — the two ride-alongs (splitOnPlaceholder move, screenshot harness scene) are both forced by the boundary rule and accounted for. No one-way doors: this is frontend-only, no new backend surface, cleanly revertible.

Design-Verdict: PASS

Real dead-end fixed at the right layer: players moved into core per the boundary rule, costs bounded, residuals tracked in filed follow-ups.

[DESIGN-REVIEWED] f18abb3

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of f18abb3c098d0de8c10f57a5541f911ecf4628c1 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification done. The review contract requires my final message to be the structured review itself, so here it is:

First-Principles-Verdict: PASS

Verify the flagged default: every animated pack holds still at idle even on screen — the author's own "product call the UX lane asked a human to weigh".

Not justified as shipped

  1. "Applies here…" hint reworded to name the crew editor, in all 13 catalogs — rides along: a relabel with no named person who failed to find "Save changes"; harm-free, but it is not this change.

What this change ships

Inventory (10 items) — 9 justified

Intent: let a crew wear the Lottie and sprite packs the library already imports, instead of greying them out. ADDITION.

  1. A Lottie pack is selectable and plays on a crew's face — justified
  2. A sprite pack is selectable, stepping the row its sheet assigns per state — justified
  3. Both players moved from the apps into core; Companion/Mochi re-import them (pinned by a new boundary test; 0 core→app imports remain, grepped apps/crew-companion|apps/mochi over components/lib/hooks/pages) — justified
  4. The pack is read once per session through a new detail call + React Query hook (1 consumer each: PackAvatar, the Library tab's invalidation) — justified
  5. Animation is bounded: off-screen, idle, and prefers-reduced-motion all hold frame 0 — justified
  6. Unreadable or malformed art reports and falls back to the ghost; a failed read recovers without a reload; the Library card shows an inline notice — justified
  7. A Lottie clip referencing a remote asset is refused before load, Companion included — justified (external-content boundary)
  8. Sprite geometry the sheet cannot be cut by is dropped/refused (was Infinity frames on the main thread; new tests fail on base) — justified
  9. Every card carries a radio ring, filled on the chosen one — justified (the reader rated clicking a guess, twice)
  10. apply_hint copy change across locales — rides along (see above)

Rewritten pins (isWearableFormat test, the "no detail wrapper" comment, the sub-frame-width clamp test) each carry their reversal's evidence in the diff: the constraint they recorded — no player in core — is what this PR removes. The client-side fallback chain is a second spelling of the server's, counted and pinned by a parity test that reads dashboard/appearances.py; it cannot use the server's because the player is chosen before the request.

[FIRST-PRINCIPLES-REVIEWED] f18abb3

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of f18abb3c098d0de8c10f57a5541f911ecf4628c1 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The delete confirm with its cost note is pre-existing at base, so the reader's Delete hesitation isn't this PR's surface. Reconciliation is complete: the primary controls (the pack cards) were read correctly and confidently; the remaining signal is evidence gaps around motion and two small comprehension residues.

UX-Verdict: CONCERNS

Pack picking reads cleanly cold, but the PR's whole value is motion, and every artifact is a still — nothing shows play, hold, or loading.

Watch

  • Idle and Library thumbnails hold frame 0, so a clip whose first frame is sparse reads as failed art: on the Lottie card the blind reader saw "just a plain coloured square… makes me unsure it loaded properly" (shot-03). Hits every pack with a fade-in first frame, everywhere it sits still, every view; the Library hint mitigates only the Library. Consider a non-blank poster frame, or accept and note it.
  • The new empty radio ring, standing alone under "Kiro" with no word, was "only guessing it's a selector and not a loading dot" (blind read) — correct guess, but the lone-ring card is the weakest reading in the new radio-group idiom.

Evidence gaps

  • No recording: "Animated ones move while the crew is working", the idle hold, and the reduced-motion hold are states the diff introduces that stills cannot show — a short capture of a crew running a turn (working plays → done → idle holds) would close it.
  • The pending skeleton (pack-avatar-pending, the loading state PackAvatar adds) appears in no screenshot.

[UX-REVIEWED] f18abb3

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of f18abb3c098d0de8c10f57a5541f911ecf4628c1 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/components/appearancePacks/PackAvatar.tsx:215 -- Accepted sprite packs without dimensions use ?? size, so a 32px sheet is sliced at each avatar’s display size and can fall back instead of rendering -> Fix: default missing frame dimensions to the established 32px. (origin: validation)
[GPT-REVIEWED] f18abb3

False positive or not applicable? A repository writer can comment:
/ai-review override gpt f18abb3c098d0de8c10f57a5541f911ecf4628c1: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed f18abb3c098d0de8c10f57a5541f911ecf4628c1 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] f18abb3

Verdict parsed from the review's SHA-scoped output markers for commit f18abb3c098d0de8c10f57a5541f911ecf4628c1.

False positive or not applicable? A repository writer can comment:
/ai-review override fable f18abb3c098d0de8c10f57a5541f911ecf4628c1: <one-sentence reason>

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 11, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 11, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=d03a96f6b575 — Lottie packs can trigger attacker-controlled network requestsfixed in d359af26e9.

Upheld, and the reasoning holds for a whole class rather than one field. The light
player closes EXPRESSION evaluation only; it still resolves a document's assets
and fonts by requesting them, and the inert-content policy the gateway puts on
the per-slot route does not cover the inlined body the Lottie tier renders. Any
reference in a third-party clip that the player would fetch is therefore a request
the dashboard issues from its own authenticated origin, on the feature's ordinary
path.

lib/appearancePacks/lottieSafety.ts::referencesRemoteAsset now refuses such a
document before loadAnimation, and LottieRenderer reports the refusal through
onError so the avatar falls back to the seeded ghost — the same answer a missing
pack already gets. Refusing rather than stripping is deliberate: a stripped clip
draws with holes, which reads as a corrupt pack while leaving every other
reference in the document to audit.

The predicate is CONSERVATIVE, and this ruling covers that whole class: a
reference is allowed only when it is provably inline (e: 1 and a data: URI,
with no u prefix), so an asset with no embedded marker, an asset claiming e: 1
while carrying a path, a u prefix beside a data URI, a webfont by fPath, and a
webfont by non-local origin are all refused — as is any document shape this
module cannot prove inline. Findings that name a further Lottie field the player
would fetch are covered by this ruling unless they show a reference that reaches
loadAnimation despite the predicate.

Pinned by src/test/lottieSafety.test.ts (15 cases, both directions) and by two
cases in LottieRenderer.test.tsx; revert-verified — deleting the guard turns the
refusal test red.

self-added: no
mechanism: lib/appearancePacks/lottieSafety.ts — a conservative allow-only-if-provably-inline predicate on Lottie assets/fonts, applied at the single loadAnimation choke point so the Companion's own gallery inherits it.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=eed62df28fad — Delete peekPackDetailfixed in d359af26e9.

Correct, and the comment justifying it was describing a caller that does not
exist: the sound path awaits loadPackDetail rather than reading the cache
synchronously, so peekPackDetail had zero consumers and its own docstring
asserted otherwise. Deleted. It comes back in the commit that has a caller.

Taken as a class ruling on speculative cache surface: no further read accessor is
added to detailCache ahead of a consumer.

self-added: yes
mechanism: none — this removes one.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=d64fc2fd1c5a — Drop the invalidatePackDetail re-export from PackAvatar.tsxfixed in d359af26e9.

Correct. One symbol importable under two paths, with the single real consumer
using the indirect one, is a seam with nothing behind it. CrewAvatarLibraryTab
now imports invalidatePackDetail from lib/appearancePacks/detailCache, which
owns it, and PackAvatar re-exports nothing. Its test mocks the cache module
instead of the component's re-export, which is also the more honest stub — the
pane's fixtures never serve the detail route.

self-added: yes
mechanism: none — this removes one.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=ae68e3fa5fe4 — New exported cache peek peekPackDetail, zero consumersfixed in d359af26e9.

The grep is right and the docstring was the misleading part: it claimed "the sound
path's debounce bookkeeping reads this instead of awaiting", while
useCrewAvatarState awaits loadPackDetail and imports nothing else. The symbol
is gone rather than re-documented — a comment describing a caller that does not
exist is worse than no symbol.

self-added: yes
mechanism: none — this removes one.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=fe8ff1c3f353 — Suggestions: the pack tier is otherwise well-shapedrebutted (nothing to change).

Recorded rather than acted on: this item reports the shape it read back as
correct, so there is no defect to fix and no code changes for it. Noting the two
parts of it that later rounds should not undo, since both were deliberate and both
cost something:

The visibility bound is an IntersectionObserver, not a size threshold. The
threshold was rejected because the dense roster's own avatars are 38px, so any
threshold low enough to animate the crew card animates every row in a list of
dozens at once — which is the cost the bound exists to remove.

The detail read is owned by one module-level cache that also holds the in-flight
promise, because N avatars wearing one pack mount in a single commit and the route
inlines every file in the pack. A per-component read, or a cache without the
in-flight half, restores the fan-out this is here to prevent.

self-added: no
mechanism: none.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 11, 2026
@iamwhatever iamwhatever changed the title feat(crews): play Lottie and sprite packs on crew avatars, with pack-carried sounds feat(crews): play Lottie and sprite packs on crew avatars Sep 11, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=4e8f8c68fe2a — The silence control's promise breaks for pack-wearing crewsfixed in 1e22d257ff.

The pack-cue path is REMOVED in 1e22d257ff, not deferred in place. Three lanes
converged on it from different directions and all three were right: the editor's
per-state "No sound" deletes the key and normalizes a stored 'none' away on the
next Apply (CrewAvatarBuilder.tsx), so "chose silence" and "chose nothing" are
one stored value and no client-side rule can honour a user turning a cue off; and
the route that would serve the bytes, plus the sounds field on the detail
answer, do not exist on main, so the branch was client code with no server
counterpart that two PRs would have had to keep in step.

Gone: the hook's packId branch (the hook and CrewStateAvatar are restored
whole from origin/main), packSoundUrl, PackSounds and its parsing in
detail.ts, and the 245-line cue suite. The sample bundles keep their done.wav
and manifest sounds.done: that is bundle FORMAT data the backend PR consumes,
and nothing in this PR reads it. The cue ships with the backend PR that adds the
route and the editor control together.

This ruling covers every finding about the cue surface on this PR: the shape
contract, its reachability, its silence semantics, and its visibility in the UI.

self-added: yes
mechanism: none — this removes one.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=6fb0f2c96ee3 — apply_hint names a button this dialog does not haverebutted.

The button exists, one layer out: this dialog's "Apply" applies the avatar to
the crew EDITOR behind it, and the editor's own submit is "Save changes"
(submit_edit), which is what persists it — the two-step is the commit point
the spec describes. The hint names the button the user presses NEXT, deliberately,
because an "Apply" that looked final left users closing the editor and losing the
face. A pre-existing string on the builder's own surface, outside this item's
files; the screenshot shows it because the harness photographs the whole dialog.

self-added: yes
mechanism: none.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 12, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 12, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=befaef6ff6f3 — Malformed Lottie data can crash the UIfixed in 89cdec625b.

Upheld. isValidLottie tests key PRESENCE, so layers: null passes it and
loadAnimation throws synchronously — before the error/data_failed
listeners could be attached — and the throw left the effect and took the
subtree with it. The loadAnimation call is now wrapped: a throw is reported
through onError like every other refusal (console diagnostic names it), the
renderer draws nothing, and the caller falls back. Pinned in
LottieRenderer.test.tsx ("reports a document the player throws on, rather
than crashing the tree"); revert-verified.

This span (LottieRenderer.tsx / gpt-BLOCKING) is on its third head, with a
different instance each time — shape, Companion callers, a throw — so the
restructure this round makes is the one that ends the class: every path a
document can fail on now converges on onError — parse, shape, remote asset,
the player's build step, the player's own events. There is no remaining way
for pack art to reach the page as an exception.

self-added: yes
mechanism: one try/catch around an existing call.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=997ef1995836 — the visibility observer effect observes the first span onlyfixed in 89cdec625b.

Real, and a consequence of the in-place fallback: the fallback is the caller's
element, so the box is unmounted while it shows and a recovery mounts a fresh
span the []-keyed effect never saw — visible stayed at its last value. The
observer now follows the NODE through a ref callback (observeBox): it
disconnects on null and creates a new observer for each node handed in, so the
post-recovery box is observed like the first. Pinned in PackAvatar.test.tsx
("re-observes the box a recovery mounts"): the second box gets a second
observer instance whose entries drive playing; revert-verified (observing only
the first node turns it red).

self-added: yes
mechanism: none — the effect became a ref callback.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=d305143a6e69 — CrewAvatarLibraryTab.tsx module header still says only SVG is selectablefixed in 89cdec625b.

Fixed: the header now states the contract the code has — every format is
selectable, core ships both players, and the grid branches on format for cost
(an svg card is one <img> and reads no pack; a lottie or sprite card draws
through PackAvatar).

self-added: yes
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=43c1f75daa84 — "Import it again" names an action with no locatable affordancefixed in 89cdec625b.

Adopted: the tile now says "Art won't draw. Use “Import pack”." — the button's
own label (minus its "(.json)…" suffix), in all thirteen catalogues in each
language's own lib_import words, and the en.context.json note tells
translators to keep the two in step. The library-broken-art-dark.png frame on
the body is re-captured with it.

self-added: yes
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=efedb67eef95 — selecting a card still rated a guess; add a group label above the gridrebutted.

Not adopted, and this is where the affordance work stops. The grid already has a
group label: the lede sentence directly above it, "Pick one to put it on this
crew: a still drawing, an animation, or animated pixel art" — a second "Pick a
pack" line under it would say the same thing twice, one line apart. At rest the
grid now carries the instruction (lede), the affordance (one filled radio ring
among empty), and the state ("Selected"); the reader's read was correct and
dared on this head and the two before it. Confidence past that is a usability
session's finding, not a fourth cue. Recorded for the PR author to weigh, since
the lane itself marks it advisory.

self-added: yes
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=dd1e201175a3 — Render pack failures through ErrorNoticefixed in 60646854d0.

Upheld, and adopted as the rule prescribes: the broken-art tile is an inline
ErrorNotice (role alert, the shared icon and colour), carrying the same
message and sized to the 72px tile, with a No hand-off comment recording the
decision — the failure is one pack's art inside an unsaved avatar draft, and the
recovery is the Import button beside the grid, which the message names in that
button's own words. pickHint stays muted text by the same rule's other half (a
validation hint; nothing failed). Pinned in CrewAvatarLibraryTab.test.tsx
(role="alert", the message, the button whose words it echoes); frame
re-captured.

self-added: yes
mechanism: none — replaces a hand-written span with the shared notice.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=715891b9675c — the lottieSafety refusal covers 1 of 2 loadAnimation sinksaccepted-and-deferred.

Named, and linked: #10249
(deferred-finding, assigned, due 2026-10-09) tracks pointing Mochi's vendored
LottieRenderer at core's fenced player — or applying referencesRemoteAsset
before its own loadAnimation — and deleting the copy, the same collapse this PR
did for the Companion and for Mochi's SpriteRenderer. Out of this item's files
by the item's own boundary (Mochi is a separate app), and not a live bypass today:
Mochi draws only its bundled presets, never imported art. The PR body's Related
Issues now lists it beside the other two follow-ups.

self-added: no
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=30bb970dd518 — the fence covers 1 of 2 Lottie sinks (Mochi's is unfenced)accepted-and-deferred.

Named, and linked: #10249
(deferred-finding, assigned, due 2026-10-09) tracks pointing Mochi's vendored
LottieRenderer at core's fenced player — or applying referencesRemoteAsset
before its own loadAnimation — and deleting the copy, the same collapse this PR
did for the Companion and for Mochi's SpriteRenderer. Out of this item's files
by the item's own boundary (Mochi is a separate app), and not a live bypass today:
Mochi draws only its bundled presets, never imported art. The PR body's Related
Issues now lists it beside the other two follow-ups.

self-added: no
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=e394d7c102c0 — private useReducedMotion duplicates PipelineView.tsx:160fixed in 60646854d0.

Done: one definition, hooks/useReducedMotion.ts, imported by PackAvatar and by
the Issue Radar PipelineView (an app importing core is the allowed direction);
both private copies are deleted. The hook's header records WHY it is not
framer-motion's, with the line from that library's own source: const [shouldReduceMotion] = useState(prefersReducedMotion.current) — a snapshot at
mount that never re-renders on change — where this one subscribes to the media
query's change event, which PackAvatar.test.tsx ("resumes when the preference
flips") verifies rather than asserts.

self-added: yes
mechanism: none — 2 definitions → 1.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=464808073bd8 — useReducedMotion duplicated; the framer rejection asserted, not verifiedfixed in 60646854d0.

Done: one definition, hooks/useReducedMotion.ts, imported by PackAvatar and by
the Issue Radar PipelineView (an app importing core is the allowed direction);
both private copies are deleted. The hook's header records WHY it is not
framer-motion's, with the line from that library's own source: const [shouldReduceMotion] = useState(prefersReducedMotion.current) — a snapshot at
mount that never re-renders on change — where this one subscribes to the media
query's change event, which PackAvatar.test.tsx ("resumes when the preference
flips") verifies rather than asserts.

self-added: yes
mechanism: none — 2 definitions → 1.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=943524c0bdf9 — Delete the private useReducedMotion; hoist one to hooks/fixed in 60646854d0.

Done: one definition, hooks/useReducedMotion.ts, imported by PackAvatar and by
the Issue Radar PipelineView (an app importing core is the allowed direction);
both private copies are deleted. The hook's header records WHY it is not
framer-motion's, with the line from that library's own source: const [shouldReduceMotion] = useState(prefersReducedMotion.current) — a snapshot at
mount that never re-renders on change — where this one subscribes to the media
query's change event, which PackAvatar.test.tsx ("resumes when the preference
flips") verifies rather than asserts.

self-added: yes
mechanism: none — 2 definitions → 1.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=d49ccd5f84b2 — lib_hint promises animation but idle holds a stillfixed in 60646854d0.

Adopted, one sentence in the hint: "Animated ones move while the crew is
working." — in all thirteen catalogues — so a freshly applied Lottie or sprite
pack sitting still on the card and on the crew reads as at rest, not as broken.
The idle-hold itself stays the product call the body flags for the author.

self-added: yes
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=8e561f3c4348 — apply_hint references a button outside the dialogfixed in 60646854d0.

Adopted this time, since the reader hit it twice: "Applies here — press “Save
changes” in the crew editor to keep it." — in all thirteen catalogues, each using
the words its own submit_edit label uses. A catalogue-only change; the builder's
component is untouched.

self-added: yes
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=000ce94563f9 — pin lottie-web imports to the fenced renderer with a grep-style testaccepted-and-deferred.

Agreed on the shape (the same as appearancePacksCoreBoundary.test.ts, with a
non-vacuity assertion), and filed as the acceptance of
#10249, which is the one exemption
the test would otherwise need: Mochi's vendored copy. Once that copy routes
through core the pin is exact — lottie-web imported by one file — and a test
carrying a standing exemption for the very sink the fence misses would pin less
than it claims.

self-added: no
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=b6a486f37334 — the shared reduced-motion hook migrates only PipelineView; 5 live siblings remainaccepted-and-deferred.

Accepted, and deferred with an issue rather than widened here:
#10304 (deferred-finding, assigned,
due 2026-10-09) names the four remaining inline readers (ChatSidebar,
VoiceDictationPanel, ThemeExperienceLayer, apps/design-critique/hooks.ts)
and the acceptance (one live reader, or a stated reason per local copy). This PR
hoisted the copy the lane named on the previous head and the one it ships;
ChatSidebar.tsx and the theme layer are hot files other in-flight work touches,
and a pack-player PR editing them is the kind of ride-along the First Principles
lane subtracts. The shared hook is in place for those call sites to import.

self-added: no
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=24565ca4b7f3 — three spellings of "read the motion preference live" coexistaccepted-and-deferred.

Accepted, and deferred with an issue rather than widened here:
#10304 (deferred-finding, assigned,
due 2026-10-09) names the four remaining inline readers (ChatSidebar,
VoiceDictationPanel, ThemeExperienceLayer, apps/design-critique/hooks.ts)
and the acceptance (one live reader, or a stated reason per local copy). This PR
hoisted the copy the lane named on the previous head and the one it ships;
ChatSidebar.tsx and the theme layer are hot files other in-flight work touches,
and a pack-player PR editing them is the kind of ride-along the First Principles
lane subtracts. The shared hook is in place for those call sites to import.

self-added: no
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=ff3c600c1e01 — delete the remaining inline reduced-motion blocks and import the hookaccepted-and-deferred.

Accepted, and deferred with an issue rather than widened here:
#10304 (deferred-finding, assigned,
due 2026-10-09) names the four remaining inline readers (ChatSidebar,
VoiceDictationPanel, ThemeExperienceLayer, apps/design-critique/hooks.ts)
and the acceptance (one live reader, or a stated reason per local copy). This PR
hoisted the copy the lane named on the previous head and the one it ships;
ChatSidebar.tsx and the theme layer are hot files other in-flight work touches,
and a pack-player PR editing them is the kind of ride-along the First Principles
lane subtracts. The shared hook is in place for those call sites to import.

self-added: no
mechanism: none.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • span=9167a94d3904 — a freshly applied animated pack holds frame 0 until a turn runsrebutted.

This is the product call the PR body flags for the author, stated with its
one-line reversal (state !== 'idle' in PackAvatar), and the two readings are
in genuine tension: a roster of dozens of looping idles (this lane's own Watch on
an earlier head) against a fresh pack that sits still until the crew does
something. The PR ships the quieter default and says so where the user picks a
pack ("Animated ones move while the crew is working") and in the spec. The
author of record decides before merge — this session does not merge — and the
flip is one token if the other reading wins. Not adopted here because the choice
is the human's, not this loop's.

self-added: yes
mechanism: none.

A crew wearing an appearance pack now shows the pack's real art, whatever
format it is drawn in.

The face used to be a plain `<img>`, so only SVG packs rendered and the
picker greyed the rest out. `PackAvatar` reads the pack once, picks a
player per slot, and holds an off-screen avatar on its first frame so a
roster of dozens costs no per-frame work. Crew Companion's two players
moved into core to make that possible; the Companion imports them back.

A third-party Lottie clip that names a remote image or font is refused
before it reaches the player: lottie-web resolves those by requesting
them, from the dashboard's own authenticated origin.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Players moved into core to respect the apps/ boundary (pinned by a non-vacuous boundary test). Remote-asset refusal on Lottie, sprite geometry refusal before fetch and the 512-frame ceiling close real main-thread hang and SSRF-shaped holes. Visibility + reduced-motion bounds and the idle-holds-still call are sensible. Pack sounds correctly deferred until the editor can express "no cue".

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