feat(crews): play Lottie and sprite packs on crew avatars - #10108
Conversation
|
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. |
Design Review (Fable 5) — ✅ PASSDesign-level review of 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 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 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of 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 Not justified as shipped
What this change shipsInventory (10 items) — 9 justifiedIntent: let a crew wear the Lottie and sprite packs the library already imports, instead of greying them out. ADDITION.
Rewritten pins ( [FIRST-PRINCIPLES-REVIEWED] f18abb3 |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of 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
Evidence gaps
[UX-REVIEWED] f18abb3 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/components/appearancePacks/PackAvatar.tsx:215 -- Accepted sprite packs without dimensions use False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
4eaab2e to
fc4390d
Compare
fc4390d to
d359af2
Compare
self-added: no |
self-added: yes |
self-added: yes |
self-added: yes |
self-added: no |
d359af2 to
1e22d25
Compare
self-added: yes |
self-added: yes |
8e4f95b to
f7ca033
Compare
self-added: yes |
self-added: yes |
self-added: yes |
self-added: yes |
self-added: yes |
self-added: yes |
self-added: no |
self-added: no |
self-added: yes |
self-added: yes |
self-added: yes |
self-added: yes |
self-added: yes |
self-added: no |
self-added: no |
self-added: no |
self-added: no |
self-added: yes |
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
left a comment
There was a problem hiding this comment.
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".
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 fromapps/— so the players moved into core (components/appearancePacks/) and the Companion imports them back.PackAvatarreads 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, sopackDetailFromkeepscontentonly 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, onedrawImageand no timers on a sprite — and holds it everywhere whenprefers-reduced-motionis 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.idleholds 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 onlyidlestill moves while a turn runs. Library thumbnails ask foridle, 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 inPackAvatar(state !== 'idle') if the other way is wanted.flowchart LR subgraph Before A1[crew wears a pack]:::ctx --> B1["<img src=/slot/{state}>"]:::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["<img> 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🟩 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.spriteas the bundle wrote it, andSpriteRenderercounts frames asnaturalWidth / frameWidth— so a hand-written"frameWidth": "0"wasInfinityframes and a trailing-frame scan that never returned, on the main thread.packDetailFromnow keepsframeWidth/frameHeightonly as integers of at least one (a fraction is dropped, not floored) andfpsonly as a positive finite number, dropping anything else so the renderer's default applies; andSpriteRendererrefuses the same geometry itself before fetching the sheet, reporting throughonError, 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 synchronousgetImageDataper 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
.jsonis third-party art on the gateway's own authenticated origin, andlottie-webresolves a document'sassetsandfontsby requesting them — the light player only removes expression evaluation.lottieSafety.ts::referencesRemoteAssetallows a reference only when it is provably inline (e: 1and adata:URI, nouprefix) and refuses everything else beforeloadAnimation; 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
PackAvatardraws the caller'sfallbackin place and stays mounted;CrewAvatarhands 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.onErrorfires 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 throughCrewAvatarwith 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.jsonis non-empty and that a sheet is a PNG, soLottieRendererrefuses 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: nullpasses the presence check and throws synchronously insideloadAnimation, before any listener exists), routes the player's ownerror/data_failedevents to the caller, and reports a parse failure the same way;SpriteRendererreports 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 inlineErrorNoticein 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.
splitOnPlaceholdersat inapps/crew-companionwhilepages/settings/McpManagement.tsximported it; it moved tolib/, which is what lets the new boundary test hold. And the screenshot harness gains aformatsscene 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 #10087 —
GET /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,onErroron 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; externaluprefixes, relative paths,e: 1with a path, a missinge, aubeside 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 withdashboard/appearances.pyread 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 synchronousloadAnimationthrow caught and reported rather than crashing the tree, the player's own failure events routed toonError, 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.tsalso pins that the bytes are kept only for a Lottie slot (an svg or sprite slot keeps its format and an emptycontent, 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— throughCrewAvatarwith 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 throughPackAvatarwhile 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 fromapps/crew-companionorapps/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.tsxalso pins that the caller'sfallbackis 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); thatprefers-reduced-motionholds the frame even on screen and resumes when the preference flips, thatidleholds 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.loadAnimationcatch, or the per-node observer each turns a test red.Manual verification
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:assetsnames anhttps://image → the crew shows the seeded ghost and the console names the refusal;manifest.sprite.frameWidthis 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-webdocuments and the sprite tiles are a real canvas stepping the rowthat slot is assigned to — the harness refuses to photograph a blank canvas or an
unresolved box, so a tile here cannot be a placeholder.
Light theme, and the rest of the surface
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.
A pack whose art cannot be drawn (
Glitch: a.jsonthat is valid JSON but nota 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.
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).
Regenerate them with the harness this PR updates:
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
LottieRendereris the oneloadAnimationsink 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 intosrc/apps/shared/, a destination still insideapps/that core cannot import. This PR moves that one copy into core (components/appearancePacks/SpriteRenderer.tsx) and deletesapps/shared/; the Companion imports core directly and Mochi's one-line re-export shim points at core, so the vendored importers' paths are untouched. ThewillReadFrequentlyhint #4261 added to the probe canvas rides along.