mirror of
https://github.com/tiennm99/tiennm99bot.git
synced 2026-10-13 12:15:06 +00:00
docs(plans): add UI/UX review reports and the refinement plan
Three advisory reviews with measured evidence: the confetti celebration, the wheel and theme design, and the API/operator experience. The API findings are untouched work - broken render:local CLI examples, 400/413/415 conditions surfacing as 500s, an unenforced timeout guarantee, and a production token default in compose.yml. The refinement plan covers the second pass and records what was deliberately excluded, including the bezel, the font-size cap, a night theme, and narrowing the request schema.
This commit is contained in:
tiennm99 committed
1 parent
11750ac552
commit
8a9e3696c4
8 files changed
+1470
No files matched your search
+131
@@ -0,0 +1,131 @@
|
||||
# Phase 01 — Motion truth
|
||||
|
||||
Make the spin behave like a physical wheel at every option count: never advance
|
||||
more than one slice per frame, and stop with a settle instead of an asymptotic
|
||||
crawl.
|
||||
|
||||
## Context
|
||||
|
||||
- `src/remotion/wheel-layout.js` — `getSpinProgress`, `getFinalWheelRotationDegrees`,
|
||||
`defaultFullTurns`, `getPointerDeflectionDegrees`
|
||||
- `src/remotion/WheelComposition.jsx:52-62` — computes `finalRotation` and `rotation`
|
||||
- `test/wheel-layout.test.js` — existing invariant tests
|
||||
|
||||
## Problem 1: dense wheels run backwards
|
||||
|
||||
Peak spin speed is ~39.9 deg/frame at every option count, because total rotation
|
||||
is fixed at 5 turns regardless of how wide a slice is. Measured aliasing frames
|
||||
out of 98, at the 512/15fps default:
|
||||
|
||||
| Options | 2 | 8 | 10 | 12 | 16 | 24 | 32 |
|
||||
|---|---:|---:|---:|---:|---:|---:|---:|
|
||||
| Frames advancing >1 slice | 0 | 0 | 8 | 21 | 38 | 56 | 65 |
|
||||
|
||||
At 32 options two thirds of the spin reads as jitter or reverse rotation.
|
||||
|
||||
### Fix: derive turn count from the frame budget
|
||||
|
||||
Peak speed in progress units is the slope at the launch/deceleration handoff,
|
||||
already named `handoffSlope` in `wheel-layout.js`. Requiring
|
||||
`handoffSlope * turns * 360 / spinFrames <= 360 / optionCount` gives:
|
||||
|
||||
```js
|
||||
export const getSpinTurns = (spinFrames, optionCount) =>
|
||||
Math.max(1, Math.min(7, Math.floor(spinFrames / (handoffSlope * Math.max(1, optionCount)))));
|
||||
```
|
||||
|
||||
**Floor, not round.** Rounding to nearest pushes 8 options from 5.63 to 6 turns
|
||||
and peak to 47.9 deg/frame against a 45 degree slice — worse than today.
|
||||
Flooring keeps the current 5 turns for the default wheel.
|
||||
|
||||
**Must stay an integer.** The winner lands under the pointer only because
|
||||
`turns * 360 mod 360 === 0`. A fractional 2.8 turns leaves the wheel 288 degrees
|
||||
off. Any change here must preserve integrality.
|
||||
|
||||
Resulting turn counts and outcomes:
|
||||
|
||||
| Config (spin frames) | n=2 | n=8 | n=12 | n=16 | n=24 | n=32 |
|
||||
|---|---|---|---|---|---|---|
|
||||
| default 6500/15fps (98) | 7 | 5 | 3 | 2 | 1 | 1 |
|
||||
| 6500/12fps (78) | 7 | 4 | 2 | 2 | 1 | 1 |
|
||||
| max 10000/20fps (200) | 7 | 7 | 7 | 5 | 3 | 2 |
|
||||
| min 3000/12fps (36) | 7 | 2 | 1 | 1 | 1 ALIAS | 1 ALIAS |
|
||||
|
||||
Only the minimum-duration dense cases still alias: 36 spin frames cannot carry
|
||||
even one turn past 24 slices. Assert that boundary rather than pretend it is
|
||||
solved.
|
||||
|
||||
Wire it in `WheelComposition.jsx` by passing
|
||||
`getSpinTurns(spinFrames, props.options.length)` into
|
||||
`getFinalWheelRotationDegrees`, and keep `defaultFullTurns` as the fallback for
|
||||
direct callers.
|
||||
|
||||
### Risk to validate before accepting
|
||||
|
||||
At 32 options with the default duration this yields **1 turn over 6.5 seconds**.
|
||||
Correct by the aliasing metric, but it may read as a broken or barely-moving
|
||||
wheel rather than a slow one. Render 24 and 32 options and judge before
|
||||
committing. If it looks wrong, the fallback is a floor of 2 turns with the
|
||||
remaining aliasing documented — a deliberate choice of drama over correctness on
|
||||
dense wheels, not a silent regression.
|
||||
|
||||
## Problem 2: the wheel stops without settling
|
||||
|
||||
`getSpinProgress` approaches 1 asymptotically, so the wheel eases to a halt with
|
||||
no impact. Real wheels overshoot the final detent slightly and rock back.
|
||||
|
||||
### Fix: damped recoil in the deceleration tail
|
||||
|
||||
Add a decaying oscillation to the last portion of the spin that is exactly zero
|
||||
at `frame === spinFrames`, so the landing guarantee is untouched:
|
||||
|
||||
```js
|
||||
const recoilFraction = 0.18; // last 18% of the spin
|
||||
const recoilDegrees = 2.2; // peak overshoot, scaled below
|
||||
```
|
||||
|
||||
Apply the recoil as a rotation offset in degrees rather than inside
|
||||
`getSpinProgress`, so progress stays monotonic in [0, 1] and the existing
|
||||
invariant tests keep their meaning. The offset must:
|
||||
|
||||
- be 0 for `frame <= spinFrames * (1 - recoilFraction)`
|
||||
- be exactly 0 at `frame >= spinFrames`
|
||||
- peak around half a slice at most, and never more than `recoilDegrees`, so a
|
||||
neighbouring slice never touches the pointer
|
||||
|
||||
Scale the peak by `Math.min(recoilDegrees, sliceDegrees * 0.25)` so a 2-option
|
||||
wheel does not visibly swing.
|
||||
|
||||
Because the celebration fires at `revealFrame` (about 5 frames before
|
||||
`spinFrames`), the recoil and the confetti burst overlap. That is the desired
|
||||
read: impact, recoil, confetti.
|
||||
|
||||
### Interaction with the pointer flapper
|
||||
|
||||
`getPointerDeflectionDegrees` is driven by `rotation`. Feeding it the recoiled
|
||||
rotation makes the flapper twitch during the settle, which is free realism, but
|
||||
the rest-neutral proof depends on the final rotation being exactly the detent
|
||||
value. Since the recoil is exactly 0 at `spinFrames` and beyond, the proof holds.
|
||||
Add a test rather than relying on that argument.
|
||||
|
||||
## Tests
|
||||
|
||||
- `getSpinTurns` returns an integer in [1, 7] for every combination of
|
||||
`spinFrames` in {36, 78, 98, 200} and option counts 2..32.
|
||||
- Winner lands at exactly screen angle 0 for every option count 2..32 and every
|
||||
winner index, using the adaptive turn count.
|
||||
- No frame advances more than one slice, for all option counts, except the
|
||||
documented minimum-duration dense cases, which are asserted explicitly as
|
||||
known-aliasing so the boundary cannot move silently.
|
||||
- Recoil offset is exactly 0 at and after `spinFrames`, and never exceeds a
|
||||
quarter slice.
|
||||
- Flapper still parks at exactly 0 at rest with recoil applied.
|
||||
- Existing tests updated: `caps peak spin speed regardless of option count` and
|
||||
`never advances a full slice in one frame at typical option counts` now cover
|
||||
all counts, so the "typical" qualifier goes away.
|
||||
|
||||
## Validation
|
||||
|
||||
Render 2, 8, 16, 24, 32 options at the default, plus the minimum and maximum
|
||||
duration cases. Confirm the winner sits under the pointer in the last frame of
|
||||
each, and judge the 24/32 slowness question above.
|
||||
+102
@@ -0,0 +1,102 @@
|
||||
# Phase 02 — Confetti craft
|
||||
|
||||
Two additions to a burst that already works: fake the motion blur a GIF cannot
|
||||
have, and keep long holds populated to the end.
|
||||
|
||||
## Context
|
||||
|
||||
- `src/remotion/confetti-layout.js` — particle construction and closed-form flight
|
||||
- `src/remotion/Confetti.jsx` — rendering, opacity envelope, launch gating
|
||||
- `test/confetti-layout.test.js`
|
||||
|
||||
Current measured behaviour at 512/15fps, default hold (22 frame window):
|
||||
on-screen count `25,37,46,53,59,67,70,...,70,69,68,66,62`, peak step 50.9px
|
||||
against a 16.3px median body.
|
||||
|
||||
## Problem 1: fast chips read as disconnected dots
|
||||
|
||||
Peak travel is about 3.1x body length per frame in the opening frames, and a GIF
|
||||
has no motion blur to tie those samples together. Real footage of a confetti
|
||||
cannon shows streaks, not dots.
|
||||
|
||||
### Fix: elongate along the direction of travel
|
||||
|
||||
Stretch each particle along its current velocity vector in proportion to speed,
|
||||
so fast chips become short ribbons and slow ones stay square. Deterministic and
|
||||
closed form, since velocity is already analytic:
|
||||
|
||||
```js
|
||||
// velocity at t, from the same closed form as position
|
||||
const speedNow = Math.hypot(vx, vy) * Math.exp(-drag * t);
|
||||
const stretch = 1 + Math.min(maxStretch, speedNow / stretchReference);
|
||||
```
|
||||
|
||||
Apply as a `scaleX` in the particle's own rotated frame, with the rotation set to
|
||||
the direction of travel rather than the tumble angle while `stretch` is
|
||||
significant. Blending between "oriented along velocity when fast" and "tumbling
|
||||
when slow" is what sells it.
|
||||
|
||||
Constraints:
|
||||
|
||||
- `maxStretch` around 1.8. Beyond that the chips look like rain.
|
||||
- The stretched long axis must stay under about 40px at the reference size, or
|
||||
the burst reads as streaks of paint.
|
||||
- The 3px minimum thickness from `minHeight` still applies to the short axis
|
||||
after stretching.
|
||||
|
||||
Cheaper alternative if the velocity-orientation blend gets fiddly: scale only the
|
||||
width along the existing rotation, accepting that the streak direction is
|
||||
approximate. Try the correct version first; it is a few lines.
|
||||
|
||||
## Problem 2: a maximum hold outlives the burst
|
||||
|
||||
At `holdMs` 2500 the field drains from frame 33 of 37, leaving the last few
|
||||
frames nearly empty. Confirmed by probe: `...,12,8,4,4,2,1,0,0`.
|
||||
|
||||
### Fix: a second volley sized to the window
|
||||
|
||||
Give a fraction of the particles a launch delay in the second half of the
|
||||
celebration window, but only when the window is long enough to need it. The
|
||||
particle builder does not currently know the window length, so pass it in:
|
||||
|
||||
```js
|
||||
createConfettiParticles({count, size, colors, random, windowSeconds})
|
||||
```
|
||||
|
||||
Then a particle in the second wave gets
|
||||
`delay = windowSeconds * lerp(rand('wave'), 0.42, 0.55)`.
|
||||
|
||||
At the default 1.47s window the second wave lands around 0.62-0.81s, which
|
||||
overlaps the first wave's peak and simply thickens it. At a 2.47s window it lands
|
||||
around 1.04-1.36s and refills the tail. At the 0.5s minimum hold the window is
|
||||
short enough that the second wave collapses into the first, which is correct
|
||||
behaviour rather than a special case.
|
||||
|
||||
Split roughly 30% of particles into the second wave. Do **not** raise `count` to
|
||||
compensate: measured byte cost of particle count is negligible (70 to 40
|
||||
particles changed the GIF by 1.1%), so if density needs a lift, raising `count`
|
||||
to 90 is nearly free. Confirm that against a render rather than assuming.
|
||||
|
||||
## Interaction with the opacity tail
|
||||
|
||||
`Confetti.jsx` fades over the last `min(4, round(lifetime * 0.25))` frames and
|
||||
quantizes opacity to quarters. A second wave arriving late must not still be
|
||||
mid-flight when the fade starts, or particles vanish in mid-air. Check that the
|
||||
second wave's latest launch plus its rise time lands before the fade begins; if
|
||||
not, pull the 0.55 upper bound down.
|
||||
|
||||
## Tests
|
||||
|
||||
- Stretch factor is 1.0 at rest and bounded by `maxStretch` at launch.
|
||||
- Short axis stays at or above 3px after stretching, at sizes 384, 480, 512.
|
||||
- Every particle still launches within the celebration window for `holdMs` 500,
|
||||
1200, and 2500.
|
||||
- Second-wave particles have strictly larger delays than first-wave ones.
|
||||
- Determinism unchanged: same seed produces identical particles.
|
||||
|
||||
## Validation
|
||||
|
||||
Re-run the density probe for `holdMs` 500, 1200, 2500 at 384/12fps and 512/15fps.
|
||||
Target: no frame in the celebration window below 40 live particles, and the last
|
||||
frame of a 2500ms hold above 25. Then render and view frames at the reveal, mid
|
||||
burst, and the end.
|
||||
+78
@@ -0,0 +1,78 @@
|
||||
# Phase 03 — Label orientation at the vertical case
|
||||
|
||||
## Context
|
||||
|
||||
- `src/remotion/wheel-label-layout.js` — `getRadialLabelLayout`, the rest-frame flip
|
||||
- `src/remotion/WheelComposition.jsx` — renders `label.rotation`
|
||||
- `test/wheel-label-layout.test.js`
|
||||
|
||||
The rest-frame flip added in the previous pass makes every label upright when the
|
||||
wheel stops, except at the boundaries. A label's rest angle is
|
||||
`(centerDegrees - winnerCenterDegrees) mod 360`, and the flip triggers strictly
|
||||
inside (90, 270). A label sitting at exactly 90 or 270 is not mirrored, so it is
|
||||
not flipped, but it renders stacked vertically.
|
||||
|
||||
Observed in the 8-option sample: `heidi` and `dave` read vertically while every
|
||||
other label is horizontal.
|
||||
|
||||
## When it happens
|
||||
|
||||
Rest angles are `360 * (i - winnerIndex) / optionCount`. Exactly 90 or 270
|
||||
requires `(i - winnerIndex) / optionCount` to be 1/4 or 3/4, so:
|
||||
|
||||
- `optionCount` divisible by 4: exactly two labels land vertical, every time.
|
||||
- Otherwise: no label is exactly vertical, but the nearest is within
|
||||
`180 / optionCount` degrees of it, which is still steeply tilted on dense
|
||||
wheels.
|
||||
|
||||
So this is not a rare edge case. Every 4, 8, 12, 16, 20, 24, 28, 32-option wheel
|
||||
has two vertical labels in its poster frame.
|
||||
|
||||
## Options
|
||||
|
||||
### A. Consistent vertical direction (cheap)
|
||||
|
||||
Move the flip boundary so 90 and 270 resolve the same way, e.g. flip on
|
||||
`restAngle >= 90 && restAngle < 270`. Both vertical labels then read in the same
|
||||
direction instead of opposite ones, so a viewer tilts their head one way for
|
||||
both. Two characters of change, no layout work, no new failure modes. Does not
|
||||
make them horizontal.
|
||||
|
||||
### B. Tangential fallback for near-vertical labels (real fix)
|
||||
|
||||
When a label's rest angle is within a threshold of vertical, lay it out
|
||||
tangentially — rotated 90 degrees from radial — so it reads horizontally at rest.
|
||||
This means a different track geometry for those labels: the available box becomes
|
||||
the slice's chord width rather than its radial length, so the existing
|
||||
`getLabelTrack` and wrap logic do not directly apply.
|
||||
|
||||
Cost: a second layout path in a module that is already the most intricate in the
|
||||
codebase, and it partially reopens the settled radial-labels decision. Benefit:
|
||||
every label horizontal in the poster frame.
|
||||
|
||||
### C. Accept it
|
||||
|
||||
Radial labels on a wheel are conventional, including on wheelofnames.com, and a
|
||||
vertical label is legible with a head tilt. Document and move on.
|
||||
|
||||
## Recommendation
|
||||
|
||||
Start with A, then judge B from a render. A is nearly free and strictly better
|
||||
than today. B is only worth its complexity if the vertical labels actually look
|
||||
wrong in the poster frame at 200px, which is the size that matters — check that
|
||||
before building it.
|
||||
|
||||
Whichever is chosen, it must not regress the rest-frame flip guarantee: the
|
||||
existing test asserting no label rests mirrored has to keep passing.
|
||||
|
||||
## Tests
|
||||
|
||||
- No label rests with a screen angle strictly inside (90, 270), unchanged.
|
||||
- Labels at exactly 90 and 270 resolve to the same reading direction.
|
||||
- If B is built: no label rests within 15 degrees of vertical, for option counts
|
||||
2..32 and winner indices across the range.
|
||||
|
||||
## Validation
|
||||
|
||||
Render 4, 8, 12, 16 options and inspect the poster frame at both 512 and a
|
||||
200px downscale, which is the realistic chat display size.
|
||||
+87
@@ -0,0 +1,87 @@
|
||||
# Phase 04 — Verification and documentation
|
||||
|
||||
Runs after phases 1-3. Two jobs: re-measure honestly, and fix a documentation
|
||||
error already sitting on `main`.
|
||||
|
||||
## Documentation correction (do this even if phases 1-3 slip)
|
||||
|
||||
`docs/render-benchmarks.md` currently misattributes where the GIF growth came
|
||||
from. It says the extra bytes are split between denser confetti and grayscale
|
||||
antialiasing. Measurement says otherwise:
|
||||
|
||||
| Build | Bytes | vs baseline |
|
||||
|---|---:|---:|
|
||||
| baseline before the celebration rework | 2,066,068 | — |
|
||||
| every change **except** the AA fix | 2,133,986 | +3.3% |
|
||||
| every change, AA fix included | 2,597,190 | +25.7% |
|
||||
|
||||
The confetti rework, winner pill, spin retune, palette reorder and SVG pointer
|
||||
together account for +3.3%. Grayscale AA alone accounts for +22.4%, because it
|
||||
affects all 98 spin frames rather than only the celebration.
|
||||
|
||||
The "levers if a deployment needs bytes back" list is wrong for the same reason.
|
||||
Lowering the `Confetti` particle count from 70 to 40 changes the GIF by **1.1%**
|
||||
(2,597,190 to 2,568,902), so it is not a lever. Measured alternatives:
|
||||
|
||||
| Change | Bytes | vs default |
|
||||
|---|---:|---:|
|
||||
| `count` 70 to 40 | 2,568,902 | -1.1% |
|
||||
| `holdMs` 1200 to 500 | 2,492,491 | -4.0% |
|
||||
| `holdMs` 1200 to 2500 | 2,859,979 | +10.1% |
|
||||
| drop the AA fix | 2,133,986 | -17.8% |
|
||||
|
||||
The only large lever is the AA fix itself, and keeping it is a decided trade.
|
||||
Rewrite the section to say that plainly instead of offering a knob that does
|
||||
nothing.
|
||||
|
||||
## Re-measurement after phases 1-3
|
||||
|
||||
Adaptive turn count changes total rotation at most option counts, which changes
|
||||
per-frame pixel deltas and therefore file size — likely downward on dense wheels,
|
||||
since they now turn less. Confetti streaks and a second wave push the other way.
|
||||
Neither is predictable; measure rather than estimate.
|
||||
|
||||
Capture before and after for each, on one machine, back to back, warm bundle:
|
||||
|
||||
- smoke, 384 @12fps, 4 options
|
||||
- classic, 512 @15fps, 8 options
|
||||
- vietnamese fixture
|
||||
- sixteen-options fixture
|
||||
- mono, 512 @15fps, 6 options
|
||||
- schema maximum: 32 options, 512, 20fps, 10000+2500
|
||||
|
||||
Note on method: render times in the last session varied by more than 2x on
|
||||
repeat runs of an identical input (one 512 render took 23.5s against a 4.3s
|
||||
median) because of background load. Sizes are deterministic and trustworthy;
|
||||
times need a median of at least three runs before any conclusion.
|
||||
|
||||
Then update both tables in `docs/render-benchmarks.md`.
|
||||
|
||||
## Visual verification checklist
|
||||
|
||||
Render and inspect, at 512 and downscaled to 200px:
|
||||
|
||||
- Poster frame, classic 8 options: winner pill legible, every label upright,
|
||||
winner slice outlined, pointer outlined with margin
|
||||
- Poster frame, mono: wheel has a silhouette, confetti visible against the
|
||||
background
|
||||
- Reveal frame: burst is a compact pop, not an empty frame
|
||||
- Mid-burst frame: field crosses the wheel, streaks read as motion
|
||||
- Final frame at `holdMs` 2500: field still populated
|
||||
- 24 and 32 options: judge whether the reduced turn count reads as slow or as
|
||||
broken, which is the open risk from phase 1
|
||||
- Long Vietnamese winner name: pill stays two lines and diacritics are complete
|
||||
|
||||
## Gates
|
||||
|
||||
`pnpm lint`, `pnpm typecheck`, `pnpm test`, `pnpm api:smoke`, `pnpm render:fixtures`.
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
1. Does 1 turn at 24-32 options read as intentional? Decided by looking at a
|
||||
render, not by argument. Fallback is a floor of 2 turns with the residual
|
||||
aliasing documented.
|
||||
2. Is the `night` theme wanted? Still open from the previous pass, still a
|
||||
contract addition, still out of scope here.
|
||||
3. Does the 14.4MB schema-maximum GIF matter in practice? Depends on whether any
|
||||
caller sends those parameters, which cannot be answered from this repo.
|
||||
@@ -0,0 +1,80 @@
|
||||
# Animation and Confetti Refinement
|
||||
|
||||
**Status:** complete. Phases 1-4 implemented and verified on 2026-07-25.
|
||||
**Branch:** main
|
||||
**Depends on:** the celebration rework already on `main` (two-cannon confetti,
|
||||
winner pill, SVG pointer, slope-matched spin, grayscale label AA)
|
||||
|
||||
Second pass over the rendered animation. The first pass fixed defects; this pass
|
||||
is about motion truth and craft, plus one documentation correction.
|
||||
|
||||
## Accepted trade already decided
|
||||
|
||||
Grayscale label antialiasing stays, at +22.4% GIF size (+463KB on the 512
|
||||
default). Clean text and an unstarved palette are worth the bytes. The offset
|
||||
programme that would claw them back (opaque bezel replacing the soft
|
||||
`wheelShadow`, halo cut from 4 offsets to 2) is explicitly **out of scope** —
|
||||
see Not doing.
|
||||
|
||||
## Phases
|
||||
|
||||
| # | Phase | Depends on | Risk | Outcome |
|
||||
|---|---|---|---|---|
|
||||
| 1 | [Motion truth](phase-01-motion-truth.md) — adaptive turn count, settle recoil | — | medium | done |
|
||||
| 2 | [Confetti craft](phase-02-confetti-craft.md) — motion streaks, second wave | — | low | done |
|
||||
| 3 | [Label orientation](phase-03-label-orientation.md) — the 90/270 vertical case | — | medium | done, option A only |
|
||||
| 4 | [Verification and docs](phase-04-verification-and-docs.md) | 1, 2, 3 | low | done |
|
||||
|
||||
Phases 1-3 touch disjoint files and can run in any order or in parallel. Phase 4
|
||||
re-measures everything once they land.
|
||||
|
||||
## Outcome
|
||||
|
||||
Turn count now scales with the frame budget: 5 turns at 8 options as before, 2 at
|
||||
16, 1 at 24 and above. Aliasing goes from 65 of 98 frames at 32 options to zero
|
||||
at every count the frame budget can serve.
|
||||
|
||||
The open risk — that one turn would read as a broken wheel rather than a slow
|
||||
one — did not materialise. At 32 options the rim still moves 30px per frame and
|
||||
77 of 98 frames are perceptibly in motion, so the fallback floor of 2 turns was
|
||||
not needed.
|
||||
|
||||
Confetti now holds a populated frame throughout: a 2500ms hold ended with 0
|
||||
particles before and 68 after. Streaks are strongest at launch and relax as chips
|
||||
slow.
|
||||
|
||||
Reducing turns on dense wheels also cut the schema-maximum render from 14.4MB and
|
||||
8999ms to 12.0MB and 7800ms.
|
||||
|
||||
`88 tests passing`, lint and typecheck clean.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- No spin frame advances more than one slice, at every option count, except
|
||||
where the frame budget makes it impossible (3000ms at 12fps leaves 36 spin
|
||||
frames; 24+ slices cannot avoid it). That exception is asserted, not ignored.
|
||||
- The winner still lands at exactly screen angle 0 for every option count and
|
||||
winner index, including through the recoil.
|
||||
- Confetti holds a populated frame for the whole celebration at `holdMs` 500,
|
||||
1200, and 2500.
|
||||
- No label rests within 15 degrees of vertical, or the vertical case is handled
|
||||
deliberately and consistently.
|
||||
- `pnpm lint`, `pnpm typecheck`, `pnpm test` clean. Every new behavior has a test
|
||||
asserting the invariant rather than a magic number.
|
||||
- `docs/render-benchmarks.md` states the correct byte attribution.
|
||||
|
||||
## Not doing
|
||||
|
||||
Carried from the two review reports, deliberately excluded:
|
||||
|
||||
- Bezel ring and reduced `wheelShadow`. Would free ~90 palette entries, but the
|
||||
size trade was decided against pursuing.
|
||||
- Text halo reduced from 4 offsets to 2.
|
||||
- Raising the label font-size cap (`radius * 0.15`), which would grow text ~31%
|
||||
on 2-6 option wheels.
|
||||
- A 4th `night` theme. Additive schema change, still needs a call.
|
||||
- Narrowing the request schema to cap total frames. The 32-option maximum is a
|
||||
14.4MB GIF, but render time is back at baseline (8999ms) so nothing is
|
||||
breaching a timeout today. Revisit only if a container actually times out.
|
||||
- `numberOfGifLoops`. The GIF keeps looping forever.
|
||||
- Everything in the API/DX review — separate track.
|
||||
+390
@@ -0,0 +1,390 @@
|
||||
# API / DX Review — Integrator and Operator Experience
|
||||
|
||||
Date: 2026-07-25
|
||||
Scope: API contract, error handling, config, scripts, docs. Visual design excluded (covered by other reviewers).
|
||||
Method: source read + empirical probes via `app.inject`, `loadConfig`, and real CLI runs. All status codes and bodies below are captured output, not inferred.
|
||||
|
||||
---
|
||||
|
||||
## 1. Verdict
|
||||
|
||||
The happy path is solid: validation runs before the semaphore, the bundle is warm-started before `listen`, the semaphore releases in `finally`, temp dirs clean up in `finally`, the token compare is constant-time, and no stack traces leak to callers. All three quality gates pass. The defects are concentrated in the *unhappy* paths an integrator actually hits: malformed JSON, an empty body, and an oversized body all return `500 {"error":"internal_error"}` because the blanket error handler discards Fastify's own `statusCode`, so a bot will retry a permanently-broken request forever. Every `pnpm render:local -- ...` invocation in the README is broken on the installed pnpm — the literal `--` reaches `parseArgs` and the command dies before rendering, and CI never runs it. The wire contract's accepted ranges (`durationMs` 3000-10000, `fps` 12|15|20, `size` 384|480|512, option count/length limits) exist only in the zod schema and are documented in no README, doc, or error-code table, and `docs/deployment.md:47` makes a timeout claim the code does not implement.
|
||||
|
||||
---
|
||||
|
||||
## 2. Quality gate results
|
||||
|
||||
| Gate | Command | Result | Output |
|
||||
|---|---|---|---|
|
||||
| Lint | `pnpm lint` | **PASS** | `eslint .` — clean, exit 0 |
|
||||
| Typecheck | `pnpm typecheck` | **PASS** | `tsc --noEmit` — clean, exit 0 |
|
||||
| Test | `pnpm test` | **PASS** | `Test Files 9 passed (9) / Tests 58 passed (58)`, 2.94s |
|
||||
| API smoke | `pnpm api:smoke` | **PASS** | `api-smoke: 441166 bytes`, render 1461ms, 4.1s wall (warm `.tmp/remotion-bundle` on disk) |
|
||||
| CLI help | `pnpm render:local -- --help` | **FAIL** | `Unable to render GIF: Unexpected argument '--help'. This command does not take positional arguments`, exit 1 |
|
||||
| CLI help | `node scripts/render-local.js --help` | PASS | usage printed, exit 0 |
|
||||
|
||||
Environment: pnpm 11.1.1, Node v24.14.0. Chromium present (`node_modules/.remotion/chrome-headless-shell`), so no gate was skipped for a missing browser.
|
||||
|
||||
Note: CI (`.github/workflows/ci.yml`) and the Dockerfile pin pnpm 11.9.0; `package.json` has no `packageManager` field, so local runs float. CI runs `render:smoke` and `api:smoke` but never `render:local`, which is why the broken CLI path ships green.
|
||||
|
||||
---
|
||||
|
||||
## 3. Findings
|
||||
|
||||
### HIGH-1 — Blanket error handler turns 400/413/415 into 500
|
||||
|
||||
`src/server.js:33-36`
|
||||
|
||||
```js
|
||||
app.setErrorHandler(async (error, _request, reply) => {
|
||||
app.log.error(error);
|
||||
return reply.code(500).send({error: 'internal_error'});
|
||||
});
|
||||
```
|
||||
|
||||
Fastify's content-type-parser errors carry a correct `statusCode`; the handler throws it away. Verified directly — the error object is intercepted and its real code printed:
|
||||
|
||||
| Request | Fastify error | Real statusCode | Caller receives |
|
||||
|---|---|---|---|
|
||||
| `POST /api/gif` with no body | `FST_ERR_CTP_EMPTY_JSON_BODY` | 400 | `500 {"error":"internal_error"}` |
|
||||
| Body `{"options":[` | `FST_ERR_CTP_INVALID_JSON_BODY` | 400 | `500 {"error":"internal_error"}` |
|
||||
| 40 KB body (limit 32 KB) | `FST_ERR_CTP_BODY_TOO_LARGE` | 413 | `500 {"error":"internal_error"}` |
|
||||
| `content-type: application/xml` | `FST_ERR_CTP_INVALID_MEDIA_TYPE` | 415 | `500 {"error":"internal_error"}` |
|
||||
|
||||
Scenario: the bot serializes a payload with a trailing comma or an oversized option list. It gets a 5xx, which by every retry convention means "server fault, retry" — so it retries a request that can never succeed, and each attempt writes a full stack trace at `level:50` in the operator's logs. The client-visible symptom is indistinguishable from a Chromium crash.
|
||||
|
||||
Fix (`src/server.js:33`):
|
||||
|
||||
```js
|
||||
app.setErrorHandler(async (error, request, reply) => {
|
||||
const status = error.statusCode ?? 500;
|
||||
if (status >= 500) {
|
||||
request.log.error(error);
|
||||
return reply.code(500).send({error: 'internal_error'});
|
||||
}
|
||||
request.log.warn({err: error}, 'request rejected');
|
||||
return reply.code(status).send({error: error.code ?? 'bad_request', message: error.message});
|
||||
});
|
||||
```
|
||||
|
||||
This also fixes HIGH-4's logging problem (`request.log` carries `reqId`; `app.log` does not — see MEDIUM-4).
|
||||
|
||||
---
|
||||
|
||||
### HIGH-2 — Every documented `pnpm render:local -- ...` invocation fails
|
||||
|
||||
`README.md:70-77` (PowerShell), `README.md:81-88` (bash), `README.md:91` (`--help`), and the usage string itself at `scripts/render-local.js:7`.
|
||||
|
||||
pnpm 11 passes the literal `--` through to the script. Captured:
|
||||
|
||||
```
|
||||
$ pnpm render:local -- --output .tmp/probe.gif --option "alpha" --option "beta" --winner 1
|
||||
$ node scripts/render-local.js "--" "--output" ".tmp/probe.gif" "--option" "alpha" ...
|
||||
Unable to render GIF: Unexpected argument '--output'. This command does not take positional arguments
|
||||
```
|
||||
|
||||
`node:util.parseArgs` treats `--` as the options terminator, so everything after it becomes a positional, and `allowPositionals: false` (`scripts/render-local-args.js:41`) rejects it. Both the PowerShell and the bash variants fail identically — this is not a shell-quoting difference. The form that works is without `--`:
|
||||
|
||||
```
|
||||
$ pnpm render:local --option alpha --option beta --winner 1
|
||||
```
|
||||
|
||||
Scenario: a new contributor follows the README's primary CLI example verbatim and the tool refuses to run, printing a usage block that repeats the same broken syntax.
|
||||
|
||||
Fix: drop `--` from `README.md:71`, `README.md:82`, `README.md:91`, and `scripts/render-local.js:7`. Optionally add `pnpm render:local --help` to CI so the path is exercised.
|
||||
|
||||
---
|
||||
|
||||
### HIGH-3 — Accepted ranges and defaults are documented nowhere
|
||||
|
||||
`README.md:14-24` and `docs/miti99bot-integration.md:18-28` show one example payload with no ranges. The real constraints live only in `src/schemas/wheel-request.js:26-53`:
|
||||
|
||||
| Field | Required | Constraint | Default |
|
||||
|---|---|---|---|
|
||||
| `options` | yes | array of strings, trimmed, each 1..`MAX_OPTION_CHARS` (40) after trim, 2..`MAX_OPTIONS` (32) items | — |
|
||||
| `winnerIndex` | no | integer, `0 <= n < options.length` | random (`crypto.randomInt`) |
|
||||
| `durationMs` | no | integer 3000..10000 | 6500 |
|
||||
| `holdMs` | no | integer 500..2500 | 1200 |
|
||||
| `fps` | no | exactly 12, 15, or 20 | 15 |
|
||||
| `size` | no | exactly 384, 480, or 512 | 512 |
|
||||
| `theme` | no | `classic` \| `festival` \| `mono` | `classic` |
|
||||
| unknown keys | — | rejected (`.strict()`) | — |
|
||||
| body size | — | 32 KB hard cap (`src/server.js:29`) | — |
|
||||
|
||||
Two of these are actively surprising and undocumented: `.strict()` means a typo'd key is a hard 400 (`{"colour":"red"}` -> `unrecognized_keys`), and `size`/`fps` are literal unions, so `size: 500` is rejected rather than snapped to the nearest supported value. `MAX_OPTIONS`/`MAX_OPTION_CHARS` are per-deployment, so a hardcoded client limit can disagree with the server's.
|
||||
|
||||
Fix: replace the bare example in `README.md:14-24` with the table above; link to it from `docs/miti99bot-integration.md`.
|
||||
|
||||
---
|
||||
|
||||
### HIGH-4 — No documented error-code contract for the consuming bot
|
||||
|
||||
`docs/miti99bot-integration.md:1-39` documents only the 200 response. Step 6 ("If the service fails, bot falls back") gives no way to distinguish retryable from terminal. Captured actual behavior:
|
||||
|
||||
| Condition | Status | Body | Retryable? |
|
||||
|---|---|---|---|
|
||||
| Missing / wrong / non-`Bearer` token | 401 | `{"error":"unauthorized"}` | no |
|
||||
| Schema violation | 400 | `{"error":"invalid_request","issues":[...]}` | no |
|
||||
| Malformed/empty/oversized body | **500** | `{"error":"internal_error"}` | **misleading — see HIGH-1** |
|
||||
| Concurrency saturated | 429 | `{"error":"too_many_renders"}`, **no `Retry-After`** | yes |
|
||||
| Render exceeded timeout | 504 | `{"error":"render_timeout"}` | maybe |
|
||||
| Render crash | 500 | `{"error":"internal_error"}` | maybe |
|
||||
| `GET /api/gif` | 404 | `{"message":"Route GET:/api/gif not found","error":"Not Found","statusCode":404}` | no |
|
||||
|
||||
Two things to note. The 404 body uses Fastify's default `{message, error, statusCode}` shape, inconsistent with the `{error: "snake_case"}` convention everywhere else — a client parsing `body.error` gets `"Not Found"` instead of a machine token. And the 401 is identical for "no header", "wrong token", and "token sent without the `Bearer ` prefix"; the last is the single most common integration mistake and the response gives no hint.
|
||||
|
||||
Fix: add this table to `docs/miti99bot-integration.md`. Set `app.setNotFoundHandler` to the `{error: 'not_found'}` shape for consistency.
|
||||
|
||||
---
|
||||
|
||||
### HIGH-5 — 429 on saturation with no `Retry-After` and no queue
|
||||
|
||||
`src/routes/gif.js:71-73`
|
||||
|
||||
```js
|
||||
if (!deps.semaphore.tryAcquire()) {
|
||||
return reply.code(429).send({error: 'too_many_renders'});
|
||||
}
|
||||
```
|
||||
|
||||
With the recommended `MAX_CONCURRENT_RENDERS=1` (`README.md:118-119`, `.env.example:4`, `compose.yml:9`) and a measured render of ~1.5s at 384px/12fps — and `docs/render-benchmarks.md:11` recording 13917ms for the same fixture in Docker — the *second* concurrent chat user gets an instant 429. This is the single most likely production failure, and the response carries no `Retry-After`, no indication of how long a render takes, and no queue. A naive bot retry loop will hot-spin.
|
||||
|
||||
Fix: one header plus a doc line.
|
||||
|
||||
```js
|
||||
return reply.code(429).header('Retry-After', '5').send({error: 'too_many_renders'});
|
||||
```
|
||||
|
||||
Document in `docs/miti99bot-integration.md` that the bot must serialize its calls or back off on 429.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-1 — `RENDER_TIMEOUT_MS` is not a total timeout, contrary to the docs
|
||||
|
||||
`docs/deployment.md:47` states: "`RENDER_TIMEOUT_MS` is a total render timeout." The implementation does not guarantee that.
|
||||
|
||||
In `src/render/render-gif.js:34-61`, the `cancelSignal` from `makeCancelSignal()` is passed **only** to `renderMedia` (line 52). It is not passed to `selectComposition` (lines 44-49) — and cannot be: `selectComposition` does not accept `cancelSignal` in this version (verified against `node_modules/@remotion/renderer/dist/select-composition.d.ts`, which exposes `timeoutInMilliseconds` but no `cancelSignal`). `getRemotionServeUrl()` (line 43) has no timeout at all.
|
||||
|
||||
So when the timer fires during browser launch or composition selection, `cancel()` fires into a signal nobody is listening to and the call runs on under Remotion's own per-operation timeout. If browser launch wedges (OOM-killed Chromium, exhausted file descriptors), the promise may never settle: the `finally` never runs, the semaphore slot is never released, and every subsequent request gets 429 forever while `/api/healthz` still returns `{"ok":true}`. Restart is the only recovery, and nothing signals that a restart is needed.
|
||||
|
||||
Fix — bound the whole operation, not just `renderMedia`:
|
||||
|
||||
```js
|
||||
const deadline = new Promise((_resolve, reject) => {
|
||||
timeoutId = setTimeout(() => {
|
||||
timedOut = true;
|
||||
cancel();
|
||||
reject(new RenderTimeoutError(options.timeoutInMilliseconds));
|
||||
}, options.timeoutInMilliseconds);
|
||||
});
|
||||
// then: await Promise.race([deadline, renderPipeline()])
|
||||
```
|
||||
|
||||
Either implement the guarantee or correct `docs/deployment.md:47` to say it bounds the render phase only.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-2 — Client disconnect does not cancel the render
|
||||
|
||||
`src/routes/gif.js:75-100`. Nothing wires `request.raw` aborts to the existing `cancelSignal`. If the bot's HTTP client times out at 10s and retries, the abandoned render keeps the only slot for its full duration; the retry gets 429. Under `MAX_CONCURRENT_RENDERS=1` a client with a timeout shorter than the render time can deadlock itself indefinitely.
|
||||
|
||||
Fix: plumb an abort into `renderWheelGif`, or at minimum document in `docs/miti99bot-integration.md` that the client timeout must exceed `RENDER_TIMEOUT_MS`.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-3 — Config errors are silently swallowed at boot
|
||||
|
||||
`src/config.js:20-27`. `parsePositiveInt` returns the fallback for anything unparseable, with no log. Captured from `loadConfig`:
|
||||
|
||||
| Env | Result | Problem |
|
||||
|---|---|---|
|
||||
| `MAX_CONCURRENT_RENDERS=abc` | `1` | typo silently ignored |
|
||||
| `MAX_CONCURRENT_RENDERS=0` | `1` | intent (disable) silently inverted |
|
||||
| `RENDER_TIMEOUT_MS="ten seconds"` | `15000` | silently ignored |
|
||||
| `MAX_OPTION_CHARS=0` | `40` | silently ignored |
|
||||
| `PORT=99999` | `99999` | accepted, then `listen` fails with a raw `ERR_SOCKET_BAD_PORT` |
|
||||
| `MAX_OPTIONS=1` | `1` | **every request 400s forever** |
|
||||
| `REQUIRE_API_TOKEN=TRUE` | `requiresApiToken: false` | case-sensitive `=== 'true'` (`src/config.js:35`); operator believes auth is enforced when it is not |
|
||||
|
||||
`MAX_OPTIONS=1` is the worst: the schema becomes `.min(2).max(1)`, unsatisfiable. Verified — every payload returns `{"code":"too_big","maximum":1,...}`. The service boots healthy and 400s 100% of traffic.
|
||||
|
||||
The `REQUIRE_API_TOKEN=TRUE` case is a security-relevant silent failure: the operator sets what they believe is the auth switch, boot succeeds, and the endpoint is wide open.
|
||||
|
||||
Fix in `loadConfig`: throw on a set-but-unparseable value instead of falling back, and reject `maxOptions < 2`. Compare `REQUIRE_API_TOKEN` case-insensitively.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-4 — Error logs have no `reqId` and no request context
|
||||
|
||||
`src/server.js:34` uses `app.log.error(error)`. Captured output for a simulated Chromium crash:
|
||||
|
||||
```
|
||||
{"level":50,"time":...,"err":{"type":"Error","message":"chromium crashed: page closed unexpectedly","stack":"..."},"msg":"chromium crashed..."}
|
||||
```
|
||||
|
||||
No `reqId` field — compare the surrounding `incoming request` / `request completed` lines, which both carry `"reqId":"req-1"`. With `MAX_CONCURRENT_RENDERS>1` an operator cannot correlate a stack trace with the request that produced it. There is also no render context anywhere: option count, `size`, `fps`, `durationMs` are never logged, so "renders started failing" cannot be narrowed to "only 512px/20fps fails".
|
||||
|
||||
Fix: `request.log.error(error)` (included in the HIGH-1 patch), and log a one-line success/failure record in the route with `{optionCount, size, fps, durationMs, byteLength}`.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-5 — `/api/healthz` proves only that the event loop is alive
|
||||
|
||||
`src/routes/health.js:4-9` returns a static object. It cannot detect a wedged semaphore (MEDIUM-1), a failed bundle, or a missing Chromium. `compose.yml:16` wires the container healthcheck to it, so Docker will report a permanently-429ing container as healthy.
|
||||
|
||||
Mitigating fact worth noting: `src/server.js:24-26` awaits the bundle *before* `Fastify()` and `listen`, so the port does not open until the bundle exists. Readiness is therefore implicitly correct at startup — the gap is steady-state liveness only.
|
||||
|
||||
Fix (small): report semaphore state so saturation is observable.
|
||||
|
||||
```js
|
||||
app.get('/api/healthz', async () => ({ok: true, service: 'wheelofnames', activeRenders: semaphore.active, maxConcurrentRenders: config.maxConcurrentRenders}));
|
||||
```
|
||||
|
||||
Requires exposing a read-only `active` from `createRenderSemaphore`.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-6 — Auth silently disabled when `API_TOKEN` is unset outside production
|
||||
|
||||
`src/routes/gif.js:47` gates on `deps.config.apiToken` being truthy. If it is unset and `NODE_ENV !== 'production'`, the boot check at `src/config.js:37-39` does not fire and `/api/gif` is fully open — no warning logged, no mention in any doc. A deploy target that does not set `NODE_ENV=production` by default (Cloud Run and Railway among the recommended targets at `docs/deployment.md:8-15`) yields an unauthenticated GIF-rendering endpoint: unmetered CPU for anyone who finds it.
|
||||
|
||||
Note `config.requiresApiToken` is computed and returned but never read by the route — the only enforcement is the boot check.
|
||||
|
||||
Fix: log a startup warning when `apiToken` is undefined, and state the "no token = no auth" rule in `docs/deployment.md`.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-7 — `compose.yml` defaults the production token to `change-me`
|
||||
|
||||
`compose.yml:5,8`: `NODE_ENV` defaults to `production` and `API_TOKEN` defaults to `change-me`. `docker compose up` with no `.env` produces a production deployment whose bearer token is published in this repo. The boot guard at `src/config.js:37` is satisfied and never fires.
|
||||
|
||||
Fix: `API_TOKEN: "${API_TOKEN:?API_TOKEN must be set}"` so compose refuses to start instead of defaulting. Same for the `docker run -e API_TOKEN=change-me` example at `README.md:115` — mark it explicitly as a placeholder.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-8 — `.env.example` is orphaned: nothing loads it, nothing documents it
|
||||
|
||||
`.env.example` exists with 8 variables. There is no `dotenv` dependency, no `--env-file` flag in any `package.json` script, and `grep` finds zero references to `.env.example` in `README.md`, `AGENTS.md`, or `docs/`. A developer who does the conventional `cp .env.example .env` and runs `pnpm dev` gets no config applied and no error — the defaults silently win.
|
||||
|
||||
Also, `.env.example:3` sets `NODE_ENV=production`, which is wrong for the local-dev file it appears to be.
|
||||
|
||||
Fix: either add `--env-file=.env` to the `dev` script (Node 24 supports it natively; no dependency needed) and document the copy step in README, or delete `.env.example` and keep `docs/deployment.md`'s shell-variable model as the single story.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-9 — No graceful shutdown
|
||||
|
||||
No `SIGTERM`/`SIGINT` handler exists in `src/` (grep confirms the only `app.close()` is in `scripts/api-smoke.js:49`). `Dockerfile:48` uses `CMD ["pnpm", "start"]`, so PID 1 is pnpm and signal forwarding to Node is unreliable. On `docker stop` / a rolling deploy, an in-flight render is killed mid-request and the client sees a connection reset rather than a clean 5xx; the container takes the full 10s kill grace period.
|
||||
|
||||
Fix: `CMD ["node", "src/server.js"]`, plus a handler calling `app.close()`.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-10 — A failed bundle is cached as a permanently rejected promise
|
||||
|
||||
`src/render/remotion-bundle.js:11-21`. `bundlePromise` is assigned once; if `bundle()` rejects, every later call returns the same rejected promise. On the server path the boot warm-up makes this mostly moot (the process dies). It bites `scripts/render-fixtures.js` and any caller running with `warmRemotionBundle: false`: one transient failure poisons the module for the process lifetime with no retry.
|
||||
|
||||
Fix: clear the cache on rejection.
|
||||
|
||||
```js
|
||||
bundlePromise = bundle({...}).catch((error) => { bundlePromise = undefined; throw error; });
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-11 — CLI ignores the deployment's configured limits
|
||||
|
||||
`scripts/render-local-args.js:5-8` hardcodes `{maxOptions: 32, maxOptionChars: 40}` instead of reading `loadConfig()`. An operator who raises `MAX_OPTIONS` cannot reproduce a production payload locally — the CLI rejects it with a limit the server would accept.
|
||||
|
||||
Fix: `const limits = loadConfig();` — `AppConfig` already carries both fields.
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM-12 — Test coverage gaps on exactly the failure modes above
|
||||
|
||||
58 tests pass, but the untested paths are the risky ones.
|
||||
|
||||
Not covered at all: `src/render/render-gif.js` (timeout classification, temp-dir cleanup on throw), `src/render/remotion-bundle.js`, `src/lib/tmp-files.js`, `src/routes/health.js`.
|
||||
|
||||
Covered module, uncovered behavior in `test/gif-route.test.js`:
|
||||
- malformed / empty / oversized body — would have caught HIGH-1
|
||||
- `X-Wheel-Winner` percent-encoding: line 39 asserts `'beta'`, which is ASCII, so `encodeURIComponent` is never exercised. The behavior does work (probe: `Chiều nay uống CraneTea` -> `Chi%E1%BB%81u%20nay%20u%E1%BB%91ng%20CraneTea`), but the header contract README advertises is untested.
|
||||
- 401 discrimination between missing header, wrong token, and missing `Bearer ` prefix
|
||||
- absence/presence of `Retry-After` on 429
|
||||
- 404 body shape
|
||||
|
||||
Highest-value additions: an oversized-body test and a non-ASCII winner-header test. Both are three lines each in the existing file.
|
||||
|
||||
---
|
||||
|
||||
### LOW
|
||||
|
||||
- **`scripts/render-local.js:37`** — a validation failure prints the raw `ZodError` JSON. Actual output for a missing `--option`: a 12-line pretty-printed array. Format to one line per issue (`path.join('.')` + `message`).
|
||||
- **`README.md:10`** — `Accept: image/gif` is shown in the request example but the server never reads `Accept`. Decorative; either honor it or drop it.
|
||||
- **`scripts/render-local-args.js:57-60`** — `--timeout` is validated before the `--help` branch (line 62), so `--help --timeout 100` errors instead of showing help. Move the help check first.
|
||||
- **`scripts/render-local.js:17`** — `--theme` is the only flag whose usage line omits its default (`classic`).
|
||||
- **`docs/render-benchmarks.md:13-14`** — two rows still `TBD` under a heading that says "Fill this after running `pnpm render:fixtures`". Either fill or delete the rows.
|
||||
- **Duplicate options** are accepted (probe: `["dup","dup"]` -> 200). This is intentional — `README.md:73-74` deliberately repeats an option — but it means `X-Wheel-Winner` is ambiguous and only `X-Wheel-Winner-Index` identifies the slot. Worth one sentence in `docs/miti99bot-integration.md`.
|
||||
- **`Dockerfile`** — no `HEALTHCHECK` (only `compose.yml` has one, so plain `docker run` gets none) and no `USER node`, so the service runs as root. `package.json` has no `packageManager` field despite CI and the Dockerfile both pinning pnpm 11.9.0.
|
||||
- **`src/server.js:29`** — `bodyLimit` is hardcoded at 32 KB while `MAX_OPTIONS`/`MAX_OPTION_CHARS` are configurable. `MAX_OPTIONS` above ~700 makes a legitimate maximum payload exceed the body limit, surfacing as HIGH-1's bogus 500.
|
||||
|
||||
---
|
||||
|
||||
## 4. Doc-accuracy table
|
||||
|
||||
| Claim | Location | Actual behavior | Verdict |
|
||||
|---|---|---|---|
|
||||
| "`RENDER_TIMEOUT_MS` is a total render timeout." | `docs/deployment.md:47` | Bounds `renderMedia` only; `selectComposition` cannot take the cancel signal, `getRemotionServeUrl()` is unbounded | **Wrong** |
|
||||
| `pnpm render:local -- --option ...` (PowerShell) | `README.md:71` | Fails: `Unexpected argument '--output'` | **Wrong** |
|
||||
| `pnpm render:local -- --option ...` (bash) | `README.md:82` | Fails identically | **Wrong** |
|
||||
| "Run `pnpm render:local -- --help`" | `README.md:91` | Fails: `Unexpected argument '--help'` | **Wrong** |
|
||||
| Usage line `pnpm render:local -- --option <text>` | `scripts/render-local.js:7` | Same broken form echoed in the tool's own help | **Wrong** |
|
||||
| "Required env vars: PORT, HOST, MAX_CONCURRENT_RENDERS, RENDER_TIMEOUT_MS, MAX_OPTIONS, MAX_OPTION_CHARS, API_TOKEN" | `docs/deployment.md:24-34` | All have defaults; only `API_TOKEN` is required, and only in production | **Misleading** |
|
||||
| "`API_TOKEN` is required when `NODE_ENV=production`" | `README.md:121` | True, but omits the `REQUIRE_API_TOKEN=true` path (`src/config.js:35`), which is documented nowhere | **Incomplete** |
|
||||
| Example payload as the API contract | `README.md:14-24`, `docs/miti99bot-integration.md:18-28` | No ranges, no defaults, no `.strict()` warning, no error codes | **Incomplete** |
|
||||
| `.env.example` implies a loadable env file | `.env.example` (whole file) | No loader, no dependency, zero doc references | **Misleading** |
|
||||
| "`RENDER_TIMEOUT_MS` defaults to `15000` and is raised to Remotion's `7000ms` floor when lower" | `README.md:119-120`, `.env.example:5-6`, `docs/deployment.md:47-48` | Matches `src/config.js:45-48`; verified `RENDER_TIMEOUT_MS=3000` -> `7000` | **Accurate** |
|
||||
| "`X-Wheel-Winner` is URL-encoded so non-ASCII labels are safe" | `README.md:32`, `docs/miti99bot-integration.md:38` | Verified by probe | **Accurate** |
|
||||
| "compose.yml forwards these values with `${VAR:-default}` fallbacks" | `docs/deployment.md:36-37` | Matches `compose.yml:5-12` — but omits that `API_TOKEN` falls back to `change-me` | **Accurate but incomplete** |
|
||||
| "Docker image runs `pnpm browser:ensure` during build so production requests do not download Chrome on first render" | `docs/deployment.md:44-45` | Matches `Dockerfile:43`. Does not mention that the Remotion *bundle* is still built at boot (`src/server.js:24-26`), which delays first listen | **Accurate but incomplete** |
|
||||
| Fixture set: `smoke.gif`, `vietnamese.gif`, `sixteen-options.gif` | `README.md:61-62` | Matches `scripts/render-fixtures.js:9-46` | **Accurate** |
|
||||
| Root `wheel.gif` and `fixtures/*.gif` are git-ignored | `README.md:92-94` | Matches `.gitignore`; `git ls-files` confirms neither is tracked | **Accurate** |
|
||||
| Themes `classic`, `festival`, `mono` | `README.md:22`, `scripts/render-local.js:17` | Matches `src/remotion/themes.js:2,14,26` | **Accurate** |
|
||||
| "API validation must reject oversized requests before render work starts" | `AGENTS.md:24` | `src/routes/gif.js:54-73` validates before `tryAcquire` | **Accurate** |
|
||||
| "Rendering must not rebundle Remotion per request" | `AGENTS.md:25` | Memoized in `src/render/remotion-bundle.js`, warmed at boot | **Accurate** |
|
||||
|
||||
---
|
||||
|
||||
## 5. Prioritized actions
|
||||
|
||||
**Quick wins (each under ~15 minutes, no design decisions)**
|
||||
|
||||
1. HIGH-2 — remove `--` from the four documented CLI invocations. Pure doc/string edit; unblocks the primary local workflow.
|
||||
2. HIGH-1 — honor `error.statusCode` in the error handler and switch to `request.log`. ~6 lines; also resolves MEDIUM-4's missing `reqId`.
|
||||
3. HIGH-5 — add `Retry-After: 5` to the 429. One line.
|
||||
4. MEDIUM-7 — `${API_TOKEN:?...}` in `compose.yml`. One line; closes the `change-me` default.
|
||||
5. MEDIUM-9 — `CMD ["node", "src/server.js"]` in the Dockerfile. One line (the `app.close()` handler can follow separately).
|
||||
6. MEDIUM-10 — reset `bundlePromise` on rejection. One line.
|
||||
7. MEDIUM-11 — `loadConfig()` in `scripts/render-local-args.js` instead of hardcoded limits. Two lines.
|
||||
8. HIGH-3 / HIGH-4 — add the field-constraint table and the error-code table to `README.md` and `docs/miti99bot-integration.md`. Doc-only, highest integrator value per minute spent.
|
||||
9. MEDIUM-12 (partial) — add the oversized-body and non-ASCII-winner-header tests to `test/gif-route.test.js`. Six lines, locks in HIGH-1 and the advertised header contract.
|
||||
10. Fix `docs/deployment.md:24` ("Required env vars" -> "Environment variables and defaults") and either fill or drop the `TBD` rows in `docs/render-benchmarks.md:13-14`.
|
||||
|
||||
**Larger work (needs a decision, not just an edit)**
|
||||
|
||||
1. MEDIUM-1 — enforce a real wall-clock bound with `Promise.race`, then either keep or correct the `docs/deployment.md:47` claim. This is the finding with the worst tail: a wedged slot is unrecoverable without a restart and invisible to the healthcheck.
|
||||
2. MEDIUM-3 — decide the config-validation posture: fail fast on any set-but-invalid value, or keep silent fallbacks and log them. Either way, `MAX_OPTIONS < 2` and case-insensitive `REQUIRE_API_TOKEN` should be handled.
|
||||
3. MEDIUM-6 — decide whether an unauthenticated `/api/gif` is ever acceptable. Minimum: a loud startup warning.
|
||||
4. MEDIUM-2 — client-disconnect cancellation, or a documented client-timeout contract. Interacts with MEDIUM-1; do them together.
|
||||
5. MEDIUM-5 — decide how much state `/api/healthz` should expose (`activeRenders` is the cheap 80%).
|
||||
6. MEDIUM-8 — keep `.env.example` and wire `--env-file`, or delete it. Do not leave it orphaned.
|
||||
|
||||
---
|
||||
|
||||
## 6. Unresolved questions
|
||||
|
||||
1. **Is `MAX_CONCURRENT_RENDERS=1` plus immediate-429 the intended contract, or is a short queue expected?** With a single chat bot and multi-second renders, the second concurrent user always fails. Whether that is acceptable depends on the bot's fallback behavior, which lives outside this repo (`docs/miti99bot-integration.md:12` mentions a local fallback renderer). This changes whether HIGH-5 is "add a header" or "add a queue".
|
||||
2. **Which deploy target is actually in use?** MEDIUM-6's severity depends on whether the target sets `NODE_ENV=production` by default. `docs/deployment.md:8-15` lists six options with different defaults.
|
||||
3. **Is `POST /api/gif` internet-exposed or on a private network?** Determines whether the bearer token is the only control and how hard MEDIUM-7 bites.
|
||||
4. **Should `size`/`fps` reject unsupported values or snap to the nearest supported one?** Current strictness is defensible but undocumented, and `size: 500` is an easy client mistake.
|
||||
5. **Cold-start budget?** The measured 4.1s `api:smoke` used an on-disk `.tmp/remotion-bundle`. A fresh container builds the bundle before `listen`, and that duration was not measured here (would require a clean Docker build). Relevant to platforms with startup-probe deadlines.
|
||||
+233
@@ -0,0 +1,233 @@
|
||||
# Winner Celebration Visual Review — Confetti Burst
|
||||
|
||||
**Date:** 2026-07-25 · **Scope:** rendered motion design of the winner celebration (`Confetti.jsx`, `confetti-layout.js`, winner emphasis in `WheelComposition.jsx`, theme palettes)
|
||||
**Mode:** advisory only, no source files modified.
|
||||
|
||||
---
|
||||
|
||||
## 1. Verdict
|
||||
|
||||
The celebration currently reads as a **small grey-to-colored dust puff at the hub**, not a confetti burst. Three compounding reasons: the burst fires 8-10 frames *after* the wheel has already visually stopped (so it has no beat to land on), it empties the frame in ~0.8s of a 1.13s window (70 → 3 on-screen particles), and it spends its densest frames clustered on top of the hub in colors identical to the wheel slices, so it looks like speckle noise rather than paper. The winner emphasis that is supposed to carry the moment — an animated `drop-shadow` glow — is at the perceptual threshold (mean ΔRGB 15 vs no-glow, ΔRGB 10 for the whole pulse) while paying for a dithered gradient ring on every hold frame. In `mono` the confetti is effectively invisible: 5 of 6 palette colors sit at 1.00-1.79:1 against the background. Net: the two celebration signals do not compete, because neither is strong enough to compete.
|
||||
|
||||
---
|
||||
|
||||
## 2. Measured baseline (empirical, this machine)
|
||||
|
||||
Verified by rendering, not by reading:
|
||||
|
||||
| Fact | Value |
|
||||
| --- | --- |
|
||||
| Default composition (`durationMs` 6500 + `holdMs` 1200 @15fps) | 116 frames; `spinFrames` 98; confetti `lifetimeFrames` **17** (1.133 s) |
|
||||
| Render time, 512@15fps, 6 options | **4.1 s** (`smoke` 384@12fps: 2.5 s) — well inside `RENDER_TIMEOUT_MS` 15000 |
|
||||
| GIF size, default | **1.80 MB**; spin phase 1.64 MB (88%), **hold phase 226 KB (12%)** |
|
||||
| GIF loop count | `Iterations: 0` → **loops forever** |
|
||||
| Particle bodies @512 | width 6.0-12.0 px, height 2.48-11.2 px |
|
||||
| Particle speed | 326-802 px/s → **21.7-53.5 px per frame** at 15fps |
|
||||
| On-screen particles, f+0 … f+17 | 70,70,70,70,70,70,68,63,57,51,45,36,26,23,14,11,6,**3** |
|
||||
| Opacity, f+0 | **0.00** (first burst frame renders 70 invisible divs) |
|
||||
| Winner-glow salience (60 probe points on winner arc, r=213..230) | no-glow→glow: mean ΔRGB 15.3 / max 46. glow-base→glow-peak: mean ΔRGB **10.2** / max 31 |
|
||||
| Wheel rim motion before the burst | 3.1 px/frame at f=87, 1.6 px/frame at f=90, 0.2 px/frame at f=95 |
|
||||
|
||||
Commands used (outputs deleted afterwards; `fixtures/*.gif` is git-ignored):
|
||||
`pnpm render:smoke`, then `node scripts/render-local.js -o fixtures/rev-<theme>.gif --option ... --winner 1 --theme <theme>`, frames extracted with `magick <gif> -coalesce`.
|
||||
|
||||
---
|
||||
|
||||
## 3. Findings
|
||||
|
||||
### F1 — HIGH · Ballistic-only physics empties the frame before the window ends
|
||||
`src/remotion/confetti-layout.js:56` (`speed 320..820`), `:66` (`gravity: 1500`), `:81-85` (pure ballistic `x = x0 + vx·t`, no drag).
|
||||
|
||||
Measured depletion: 70 on-screen through f+5, then 45 at f+10, 26 at f+12, **3 at f+17**. The last third of the hold is a static wheel. Real confetti decelerates hard in air, then flutters; this launches like buckshot and leaves.
|
||||
|
||||
**Fix — closed-form exponential drag (still a pure function of `t`, no state):**
|
||||
|
||||
```js
|
||||
// confetti-layout.js — replace getConfettiParticleState
|
||||
const decay = 1 - Math.exp(-particle.drag * t);
|
||||
const terminalY = particle.gravity / particle.drag;
|
||||
x = particle.originX + (particle.vx / particle.drag) * decay;
|
||||
y = particle.originY + ((particle.vy - terminalY) / particle.drag) * decay + terminalY * t;
|
||||
```
|
||||
|
||||
Constants: `gravity: 1300 * scale`, `speed = lerp(rand('speed'), 700, 1500) * scale`, `drag = lerp(1 - sizeNorm, 2.4, 3.6)` (small chips decelerate faster — free depth cue, one line).
|
||||
|
||||
Simulated result with the same 17-frame window: on-screen `0,46,62,74,83,89,88,84,83,81,82,82,80,77,77,75,75,75`. Frame-to-frame step drops from 53 px at f+1 (the pop) to 14-25 px from f+7 on (readable flutter).
|
||||
|
||||
### F2 — HIGH · The first burst frame is fully transparent
|
||||
`src/remotion/Confetti.jsx:28` — `interpolate(progress, [0, 0.06, 0.7, 1], [0, 1, 1, 0])` with `progress = localFrame / lifetimeFrames`, so at `localFrame = 0` opacity is exactly 0. Confirmed in the render: frame 98 shows zero confetti.
|
||||
|
||||
The only frame where all particles are still compact at the origin — the actual "pop" — is thrown away, and by f+1 they have already travelled 35 px average. Nothing in the GIF ever shows a dense cluster.
|
||||
|
||||
**Fix:** no fade-in. Opacity starts at 1 (see F9 for the tail).
|
||||
|
||||
### F3 — HIGH · `mono` confetti is invisible
|
||||
`src/remotion/themes.js:36`, consumed at `src/remotion/WheelComposition.jsx:186` (`colors={theme.slices}`). Contrast vs background `#f4f4f5`:
|
||||
|
||||
`#f4f4f5` **1.00** · `#e4e4e7` **1.15** · `#d4d4d8` **1.34** · `#c4c4ca` **1.58** · `#b8b8bf` **1.79** · `#a1a1aa` 2.33
|
||||
|
||||
5 of 6 colors are below 1.8:1. The render confirms it: the burst reads as dust or sensor noise on a light grey field.
|
||||
|
||||
**Fix — add a `confetti` key per theme** (internal, no request-schema change), fall back to `slices` if absent:
|
||||
|
||||
```js
|
||||
classic: ['#f97316', '#14b8a6', '#f59e0b', '#3b82f6', '#ec4899', '#8b5cf6', '#10b981', '#f43f5e'] // min 2.05 vs #f8fafc
|
||||
festival: ['#f43f5e', '#ea580c', '#65a30d', '#0891b2', '#7c3aed', '#db2777', '#16a34a', '#d97706'] // min 2.91 vs #fff7ed
|
||||
mono: ['#18181b', '#3f3f46', '#52525b', '#71717a', '#a1a1aa'] // 16.1 / 9.5 / 7.0 / 4.4 / 2.3
|
||||
```
|
||||
|
||||
`classic` also drops `#facc15` (1.46 vs `#f8fafc`) and `#34d399` (1.84) — both effectively invisible as 8 px chips today even though they work as large slices. Cost: zero extra palette pressure (flat colors), no new DOM.
|
||||
|
||||
### F4 — HIGH · The burst fires ~0.6 s after the wheel has visually stopped
|
||||
`src/remotion/wheel-layout.js:56` — `1 - (1 - p) ** 3`. Any polynomial ease-out has zero terminal velocity, so the last frames are motionless:
|
||||
|
||||
| frame | rim motion | remaining rotation |
|
||||
| --- | --- | --- |
|
||||
| 87 | 3.1 px/frame | 3.41° |
|
||||
| 90 | 1.6 px/frame | 1.31° |
|
||||
| 93 | 0.6 px/frame | 0.32° |
|
||||
| 95 | 0.2 px/frame | 0.07° |
|
||||
|
||||
`Confetti startFrame = holdStartFrame = spinFrames = 98` (`WheelComposition.jsx:31,186`). The viewer perceives the stop around frame 88-90, then watches ~9 dead frames, then confetti appears out of nowhere. The celebration has no cause.
|
||||
|
||||
**Fix (cheap, no schema change):** one shared reveal frame, 5 frames earlier at 15fps:
|
||||
|
||||
```js
|
||||
const revealFrame = Math.max(0, spinFrames - Math.round(fps * 0.33));
|
||||
```
|
||||
|
||||
Cost: the wheel is 0.32° (1.2 px at the rim) short of final when the burst starts — imperceptible, and the winner slice is already fully under the pointer. Payoff: burst lands on the perceived stop *and* the window grows from 17 to 22 frames (+29%) for free. Drive both `Confetti startFrame` and the winner pulse from `revealFrame`.
|
||||
|
||||
### F5 — MEDIUM · Particles strobe at 15fps
|
||||
Measured step 26-35 px average over f+0..f+5 against 6-12 px bodies — 3-5× their own length per frame, with no motion blur available in a GIF. They read as discrete jumping dots, not motion.
|
||||
|
||||
Fixed by F1 (step falls to 14-25 px from f+7). Optional extra, one line, deterministic: elongate the body along the launch direction in proportion to launch speed, `width *= 1 + speedNorm * 0.5`, which fakes motion blur with a static shape.
|
||||
|
||||
### F6 — MEDIUM · Confetti wears the wheel's own colors, over the wheel
|
||||
`WheelComposition.jsx:186` passes `theme.slices` as the confetti palette, and the origin is the wheel center, so the densest frames sit entirely on the disc. With 6-8 slices, roughly 1 particle in 6-8 is on a slice of its own color at any moment — orange chips vanish on the orange slice. This is the main reason frames 99-103 read as "dirt on the wheel" rather than paper in the air.
|
||||
|
||||
**Fix — hard 1 px light ring, no blur:**
|
||||
|
||||
```js
|
||||
boxShadow: '0 0 0 1px #ffffff',
|
||||
```
|
||||
|
||||
`#ffffff` is already in the palette (slice strokes, `WheelComposition.jsx:115`), and a zero-blur spread produces no gradient, so this costs **zero new palette entries and zero dithering** while guaranteeing separation over any slice. Side benefit: it grows the effective particle by 2 px, which is exactly what the 384 px / 15 fps case needs.
|
||||
|
||||
### F7 — MEDIUM · The emission box is entirely inside the hub
|
||||
`confetti-layout.js:63-64` — origin jitter is `±0.06 · size` = ±31 px at 512. The hub ring is `centerRingRadius = 0.09 · size` = 46 px (`WheelComposition.jsx:41,161`). Every particle is born inside the hub disc, so the launch reads as "the axle exploded".
|
||||
|
||||
**Fix, two options:**
|
||||
|
||||
- *Quick (2 lines):* origin on the winner slice, pointer side. `originX = center + radius * 0.55` (≈371 px at 512), `originY = center + lerp(rand, -0.03, 0.03) * size`, fan `-175..-45°`. Ties the burst to the thing being announced.
|
||||
- *Better (small rework):* two cannons framing the wheel. Even index → `origin (0.06·size, 0.90·size)`, fan `-75..-30°`; odd index → `(0.94·size, 0.90·size)`, fan `-150..-105°`. With F1 constants (`v0` 700-1500, `drag` 3.0, `g` 1300) particles crest around `y ≈ 0.40 · size`, sweeping diagonally across the disc. The launch frames happen over the plain corner background, where the pop is legible, and the classic party-popper read is unmistakable.
|
||||
|
||||
### F8 — MEDIUM · Winner glow is near-threshold and fights the wheel's own shadow
|
||||
`WheelComposition.jsx:118` — `drop-shadow(0 0 ${10 + winnerPulse * 10}px rgba(250,204,21,0.45))`, sitting in the same 15-25 px band outside the disc as `theme.wheelShadow` `0 18px 52px rgba(15,23,42,0.22)` (`:75`). The yellow at 45% alpha over a cool grey shadow over a near-white background produces a muddy cream tint: measured mean ΔRGB **15.3** vs no glow, and the entire 10→20 px pulse is only mean ΔRGB **10.2**. A contrast-stretched crop shows the halo is visibly **dithered speckle** — the worst possible payload for a 256-color GIF, animated on every hold frame.
|
||||
|
||||
**Fix — make the emphasis geometric instead of gradient.** Scale-pop the winner `Pie` about the wheel center:
|
||||
|
||||
```js
|
||||
const pop = frame >= revealFrame
|
||||
? Math.sin(Math.min(1, (frame - revealFrame) / Math.round(fps * 0.4)) * Math.PI) * 0.045
|
||||
: 0;
|
||||
// on the winner Pie only
|
||||
transform: `scale(${1 + pop})`, transformOrigin: '50% 50%',
|
||||
```
|
||||
|
||||
A 4.5% scale on a 210 px radius moves the arc edge **9.5 px** — an order of magnitude more legible than ΔRGB 10, and it moves existing flat colors instead of creating new blended ones. Then either delete the animated glow or make it a fixed, tighter, more opaque halo (`drop-shadow(0 0 8px rgba(250,204,21,0.75))`, constant for the whole hold) so it compresses as one static delta rather than 18 changing gradients. For `mono`, `winnerGlow: rgba(161,161,170,0.45)` on `#f4f4f5` is hopeless — use `rgba(24,24,27,0.35)` there.
|
||||
|
||||
### F9 — MEDIUM · The fade-out window is wasted and progress-relative
|
||||
`Confetti.jsx:28` — fade starts at `progress 0.7`. At `holdMs` 1200 that is f+11.9, when only 36 of 70 particles remain, and it reaches 0 with 3 left. At `holdMs` 2500 it starts at f+26, when **zero** remain. So the fade never does visual work; it only spends 5 frames of alpha-blended particles (new blend colors → palette pressure) on nothing. It does not "fight the winner reveal" today — it is simply inert.
|
||||
|
||||
**Fix — frame-relative, quantized tail:**
|
||||
|
||||
```js
|
||||
const fadeFrames = Math.min(4, Math.max(1, Math.round(lifetimeFrames * 0.25)));
|
||||
const raw = interpolate(localFrame, [lifetimeFrames - fadeFrames, lifetimeFrames], [1, 0],
|
||||
{extrapolateLeft: 'clamp', extrapolateRight: 'clamp'});
|
||||
const opacity = Math.ceil(raw * 4) / 4; // at most 5 alpha levels → bounded palette cost
|
||||
```
|
||||
|
||||
Quantizing to quarters caps the number of new blended colors instead of generating a fresh set per frame. It also guarantees a confetti-free final frame, which matters because the GIF loops forever (F15).
|
||||
|
||||
### F10 — MEDIUM · Long holds buy dead air, not a longer celebration
|
||||
Rendered `--hold 2500` (37 hold frames): confetti is gone by f+19 and the last **18 frames (1.2 s, half the hold)** are a completely static wheel. Burst length is governed by physics, not by the composition, so raising `holdMs` today makes the GIF worse. Do not raise the `holdMs` default as a fix. With F1 constants the same window drains at f+33, leaving 4 dead frames instead of 18.
|
||||
|
||||
### F11 — LOW · Sub-3px dimension at `size: 384`
|
||||
`confetti-layout.js:62` — `height = width * lerp(rand('ratio'), 0.4, 1)`. At 384 (`scale` 0.75) minimum height is **1.87 px**, and 5 of 70 particles are under 2.5 px. After PNG→GIF quantization these are a single dim row of pixels.
|
||||
|
||||
**Fix:** `height: Math.max(3, width * ratio)`.
|
||||
|
||||
### F12 — LOW · One shape, linear spin, no air
|
||||
All 70 particles are the same rounded rect, rotating at a constant `±540 deg/s` (`:65`) — 36°/frame at 15fps, which samples as flicker rather than tumble — with no lateral motion at all.
|
||||
|
||||
**Fix, three numbers, no new abstraction:**
|
||||
|
||||
- Aspect ratio `lerp(rand('ratio'), 0.25, 2.6)` with `height = clamp(width * ratio, 3, 26)`. One distribution yields chips, near-squares, and long ribbons — no `type` field, no branching.
|
||||
- Size skew `width = lerp(rand('width') ** 1.7, 6, 18) * scale` — mostly 6-10 px with a few 14-18 px anchors that survive 15 fps and quantization.
|
||||
- `rotationSpeed = lerp(rand('spin'), -300, 300)` — max 20°/frame, reads as a coherent tumble.
|
||||
- Sway, added to `x`: `Math.sin(phase + t * hz * 2π) * amp * Math.min(1, t * 2)`, with `hz = lerp(rand('hz'), 0.5, 0.9)` and `amp = lerp(rand('sway'), 8, 20) * scale`. At 0.5-0.9 Hz that is a 17-30 frame cycle — well above the Nyquist limit at 15 fps, unlike a per-frame flip. **Do not** add 3D foreshortening (`scaleY(cos …)`): at 15 fps the flip aliases, and it drives the short axis below the 3 px floor from F11.
|
||||
|
||||
### F13 — LOW · `borderRadius: 1` on 6-12 px chips
|
||||
`Confetti.jsx:50`. Invisible at this size but produces ~12 antialiased corner pixels per particle (≈850/frame at 70 particles) that all need palette slots. Set `0`.
|
||||
|
||||
### F14 — LOW · Non-winner labels are upside-down at rest
|
||||
Not confetti, but it is what the reveal frame looks like. Final rotation is `7·360 − sliceCenter` (`wheel-layout.js:27`), e.g. 225° for winner 1 of 6, so labels in the upper-left read inverted (visible in every rendered hold frame). The **winner** label is always upright by construction, so the announcement itself is legible — the surrounding frame just looks sloppy. Out of scope here; worth a separate look.
|
||||
|
||||
### F15 — LOW · Infinite loop with a non-empty last frame
|
||||
`identify` reports `Iterations: 0` and `render-gif.js:51-61` sets no `numberOfGifLoops`. At `holdMs` 1200 the final frame still carries 3 particles, so the loop point snaps confetti away at the same instant the wheel jumps back to `-24°`. F9's guaranteed-empty final frame removes half of that discontinuity for free.
|
||||
|
||||
---
|
||||
|
||||
## 4. Prioritized recommendations
|
||||
|
||||
### Quick wins (each well under 30 min, no contract change)
|
||||
|
||||
| # | Change | Files | Visual payoff | Risk |
|
||||
| --- | --- | --- | --- | --- |
|
||||
| 1 | Opacity starts at 1; frame-relative quantized tail (F2, F9) | `Confetti.jsx:28` | Recovers the pop frame; removes an inert 5-frame ramp | none to determinism; **reduces** palette churn |
|
||||
| 2 | `theme.confetti` palettes (F3) | `themes.js`, `WheelComposition.jsx:186` | `mono` goes from invisible to legible; `classic` loses two dead colors | none; flat colors, no size change |
|
||||
| 3 | Hard 1 px `#ffffff` ring (F6) | `Confetti.jsx:48-58` | Particles stop reading as dirt on the wheel face | none; `#ffffff` already in palette, no blur |
|
||||
| 4 | `revealFrame = spinFrames − round(fps·0.33)` for confetti **and** winner pulse (F4) | `WheelComposition.jsx:31-37,186` | The burst finally lands on a beat; +29% window | 0.32° rotation error, imperceptible |
|
||||
| 5 | `Math.max(3, …)` height floor; `borderRadius: 0` (F11, F13) | `confetti-layout.js:62`, `Confetti.jsx:50` | Nothing vanishes at 384; crisper edges | none; slightly smaller GIF |
|
||||
| 6 | Shape/size/spin/sway numbers (F12) | `confetti-layout.js:55-70` | Ribbons + chips + tumble instead of uniform specks | none (all seeded); +1 `Math.sin` per particle per frame |
|
||||
|
||||
### Larger reworks
|
||||
|
||||
| # | Change | Payoff | Risk |
|
||||
| --- | --- | --- | --- |
|
||||
| A | **Exponential drag physics** (F1) — the single highest-payoff change | 75-90 particles on screen at the last frame instead of 3; punchy launch then real flutter; long holds stop being dead air | Determinism: safe, closed form in `t`. Render time: unchanged (same node count, 2 extra `Math.exp`). GIF: more moving pixels for ~10 extra frames; hold phase is 12% of bytes today, so expect roughly +5-8% total, i.e. ~1.9 MB. Needs `pnpm render:fixtures` re-measurement and a `docs/render-benchmarks.md` update. |
|
||||
| B | **Two-cannon origin** (F7, option 2) | The clearest "celebration" read; launch frames happen over plain background where they are legible; stops the hub-explosion look | Determinism safe. Composition risk: verify particles do not pile on the winner label — with 90 particles at ~90 px² each, total coverage is ~3% of the frame, so occlusion is not a practical concern |
|
||||
| C | **Geometric winner emphasis** (F8) — scale-pop, glow made static or removed | Winner emphasis becomes actually visible (9.5 px edge move vs ΔRGB 10); removes 18 frames of animated dithered gradient | Changes the winner's silhouette by 4.5% — check the 2 px white stroke and slice seams do not tear at `size: 384`. Likely a **net GIF-size win** |
|
||||
| D | **Particle count 70 → 90** | Density during the good frames | +20 divs/frame; render cost negligible at 4.1 s total. Do this only after A, since A alone triples effective on-screen density |
|
||||
|
||||
### Explicitly do NOT do
|
||||
|
||||
- Per-particle opacity fades, blur, soft shadows, or gradient particle fills — each multiplies palette entries and produces the same dither speckle already visible in the winner glow.
|
||||
- A back/front depth split (confetti behind the wheel). The disc occupies ~82% of the canvas width, so a back layer would be hidden for most of its life. Rejected on payoff, not on cost.
|
||||
- Raising the `holdMs` default. F10 shows the current burst cannot fill even 1.2 s; fix physics first. Changing schema defaults would also be a user-visible behavior change.
|
||||
- Any particle-system abstraction. Every finding above is a constant, a distribution range, or one closed-form expression in the two files that already exist.
|
||||
|
||||
### Suggested implementation order
|
||||
|
||||
1, 2, 3, 5 (independent, all low-risk) → A → 4 → C → B → D, re-rendering all three themes plus `--hold 500` and `--hold 2500` and `--size 384` after A, B and C.
|
||||
|
||||
---
|
||||
|
||||
## 5. Not verified empirically
|
||||
|
||||
- **GIF size and render time after the proposed changes.** All size/time figures are baseline measurements of the current code. The +5-8% estimate for drag physics is extrapolated from the measured 226 KB / 12% hold-phase share, not measured.
|
||||
- **The two-cannon and scale-pop designs were not rendered** (advisory-only, no source edits). Trajectory claims for the cannons come from the same closed-form simulation used to tune the drag constants, not from a Chromium render.
|
||||
- **Perceptual thresholds** — statements like "the viewer perceives the stop around frame 88-90" derive from rim displacement in px/frame (3.1 px at f=87, 1.6 px at f=90), not from user testing.
|
||||
- **`fps: 12` and `fps: 20` with the proposed constants** were simulated (both hold density well) but not rendered.
|
||||
- **Frame timing granularity**: `identify` shows delays alternating 6/7 centiseconds for 15 fps. Players that clamp low delays could shift the perceived envelope; not tested in any real client.
|
||||
|
||||
---
|
||||
|
||||
## 6. Unresolved questions
|
||||
|
||||
1. Is the infinite GIF loop intentional? The wheel snapping from the winner back to `-24°` forever undercuts the reveal. `numberOfGifLoops: 1` in `render-gif.js` would end on the winner — but that is a user-visible behavior change and some chat clients handle non-looping GIFs poorly.
|
||||
2. Should the confetti palette stay tied to the theme, or become a fixed festive palette in all three themes? A fixed palette is the strongest visual answer for `mono`, but it breaks `mono`'s design intent (a deliberately restrained theme). The proposal above keeps `mono` monochrome by inverting to darks; confirm that reads as intended rather than as "black litter".
|
||||
3. Is a spin easing with non-zero terminal velocity (or a small settle recoil) acceptable? That is the root cause of F4; the `revealFrame` shift is a workaround that leaves the wheel visually frozen for ~5 frames before the pop.
|
||||
4. Is `mono`'s `winnerGlow` supposed to be visible at all? At `rgba(161,161,170,0.45)` on `#f4f4f5` it is unmeasurable, so today `mono` has no winner emphasis whatsoever.
|
||||
+369
@@ -0,0 +1,369 @@
|
||||
# Wheel Animation Visual Design Review (advisory)
|
||||
|
||||
**Date:** 2026-07-25 · **Scope:** rendered GIF visual + motion design, excluding confetti particle design
|
||||
**Verified empirically:** yes — 30 PNG stills + 3 GIFs rendered locally (Chromium present), contrast/ΔE/spin-curve computed numerically. No source file modified. No files written outside this report.
|
||||
|
||||
---
|
||||
|
||||
## 1. Verdict
|
||||
|
||||
The wheel is competently built and the winner never lands ambiguously, but it reads as a *pie chart that stopped*, not as a wheel that was spun. Motion is a single cubic ease-out over 7 turns: the first 22 frames move more than a full slice per frame (visual strobe/reverse illusion) and the last ~35 frames crawl under 4°/frame, so nearly 2.5 s of a 7.7 s GIF is visually static with no tick, no anticipation and no settle. Legibility is the bigger problem: roughly half of all labels render **upside down** at rest (`rotation: centerDegrees` with no 90–270° flip), so the poster frame of an 8-option wheel has 3–4 unreadable names; at a realistic ~200 px chat display almost nothing is readable and the winner is communicated only by a 17 px arrow at the frame edge. `mono` is the weakest theme — one slice is literally the background colour, dividers are white-on-near-white (1.10:1), and it reads as an unfinished grey blob. Finally there are three measurable production defects: LCD sub-pixel text antialiasing puts 268 unique colours into a single 90×30 px label crop (starving the 256-colour GIF palette), `pointerStroke`/`boxShadow` on the pointer are silently discarded by `clip-path`, and the max accepted request produces a **14.3 MB GIF in 10.3 s** on a fast dev box.
|
||||
|
||||
---
|
||||
|
||||
## 2. Contrast table (computed, WCAG 2.1 relative luminance)
|
||||
|
||||
`theme.text` on each slice. Pass thresholds: 3:1 large text (≥18.66px bold), 4.5:1 normal.
|
||||
|
||||
| Theme | Slice | text:slice | 3:1 | 4.5:1 | slice:bg | ΔE to next slice (normal) | ΔE deutan | ΔE protan |
|
||||
|---|---|---:|:--:|:--:|---:|---:|---:|---:|
|
||||
| classic (`#111827` / bg `#f8fafc`) | `#f97316` | 6.33 | pass | pass | 2.68 | 113 | 116 | 97 |
|
||||
| | `#14b8a6` | 7.13 | pass | pass | 2.38 | 96 | 104 | 92 |
|
||||
| | `#facc15` | 11.58 | pass | pass | 1.46 | 132 | 124 | 121 |
|
||||
| | `#60a5fa` | 6.98 | pass | pass | 2.43 | 65 | 92 | 76 |
|
||||
| | `#f472b6` | 6.70 | pass | pass | 2.53 | 46 | 65 | 59 |
|
||||
| | `#a78bfa` | 6.52 | pass | pass | 2.60 | 113 | **16** | **31** |
|
||||
| | `#34d399` | 9.23 | pass | pass | 1.84 | 109 | 79 | 53 |
|
||||
| | `#fb7185` | 6.59 | pass | pass | 2.57 | 53 | **25** | **30** |
|
||||
| festival (`#1f2937` / bg `#fff7ed`) | `#fb7185` | 5.45 | pass | pass | 2.53 | 68 | **18** | **25** |
|
||||
| | `#f59e0b` | 6.83 | pass | pass | 2.02 | 70 | 50 | 35 |
|
||||
| | `#84cc16` | 7.43 | pass | pass | 1.86 | 100 | 80 | 78 |
|
||||
| | `#06b6d4` | 6.05 | pass | pass | 2.29 | 67 | **29** | **12** |
|
||||
| | `#a78bfa` | 5.39 | pass | pass | 2.56 | 46 | 65 | 59 |
|
||||
| | `#f472b6` | 5.54 | pass | pass | 2.49 | 130 | 51 | 26 |
|
||||
| | `#22c55e` | 6.44 | pass | pass | 2.15 | 97 | 84 | 62 |
|
||||
| | `#fb923c` | 6.49 | pass | pass | 2.13 | 50 | **17** | **22** |
|
||||
| mono (`#18181b` / bg `#f4f4f5`) | `#e4e4e7` | 13.96 | pass | pass | 1.15 | 24 | 24 | 24 |
|
||||
| | `#a1a1aa` | 6.91 | pass | pass | 2.33 | 19 | 19 | 19 |
|
||||
| | `#d4d4d8` | 11.99 | pass | pass | 1.34 | **10** | **10** | **10** |
|
||||
| | `#b8b8bf` | 8.98 | pass | pass | 1.79 | 22 | 21 | 21 |
|
||||
| | `#f4f4f5` | 16.12 | pass | pass | **1.00** | 17 | 17 | 17 |
|
||||
| | `#c4c4ca` | 10.20 | pass | pass | 1.58 | **12** | **11** | **12** |
|
||||
|
||||
Text-on-slice contrast **passes everywhere** — the 4-direction halo is not needed for contrast (see F-11). The failures are elsewhere:
|
||||
|
||||
Non-text contrast (WCAG 1.4.11 needs 3:1 for meaningful graphics):
|
||||
|
||||
| Pair | Ratio | Verdict |
|
||||
|---|---:|---|
|
||||
| classic pointer `#ef4444` vs bg `#f8fafc` | 3.60 | pass |
|
||||
| classic pointer vs slice it overlaps `#f97316` | **1.34** | fail — tip vanishes on orange |
|
||||
| classic pointer vs `#fb7185` | **1.40** | fail |
|
||||
| festival pointer `#dc2626` vs `#fb7185` | **1.79** | fail |
|
||||
| festival pointer vs bg `#fff7ed` | 4.55 | pass |
|
||||
| mono pointer `#27272a` vs bg | 13.55 | pass |
|
||||
| slice divider `#ffffff` vs mono `#f4f4f5` | **1.10** | fail — dividers invisible |
|
||||
| slice divider `#ffffff` vs mono `#fafafa` (proposed) | 1.04 | fail |
|
||||
|
||||
ΔE guide: <15 = not reliably distinguishable at small display size. `min` cyclic adjacent ΔE across normal+deutan+protan: **classic 15.9, festival 11.7, mono 10.1** — all below the safe floor.
|
||||
|
||||
---
|
||||
|
||||
## 3. Findings
|
||||
|
||||
### F-1 — HIGH — Half of all labels render upside down
|
||||
`src/remotion/WheelComposition.jsx:144` + `src/remotion/wheel-label-layout.js:208` (`rotation: centerDegrees`).
|
||||
Any slice whose centre angle falls in 90°–270° gets text rotated past vertical, i.e. mirrored-looking (`ǝɔɐɹƃ`). In the default 8-option render at the final frame, `heidi grace frank erin` are all unreadable; at 16 options 8 names are unreadable; at 2 options `Yes` is unreadable. This is the single largest legibility loss and it is worst in the poster frame.
|
||||
|
||||
**Fix** (keeps radial orientation — settled decision untouched). Flip by the *rest-frame* screen angle so every label is upright in the final frame:
|
||||
|
||||
```js
|
||||
// wheel-label-layout.js — new input: winnerCenterDegrees
|
||||
const restAngle = ((centerDegrees - winnerCenterDegrees) % 360 + 360) % 360;
|
||||
const flip = restAngle > 90 && restAngle < 270 ? 180 : 0;
|
||||
// ...
|
||||
rotation: centerDegrees + flip,
|
||||
```
|
||||
Caller: `winnerCenterDegrees: getSliceCenterDegrees(props.options.length, props.winnerIndex)`.
|
||||
During the spin, upside-down-ness is unchanged (it is fixed in wheel space either way) — pure win at rest. Determinism: unaffected. GIF size: unaffected. Render time: unaffected.
|
||||
Cheaper variant if you prefer winner-independence: `flip = centerDegrees % 360 > 90 && < 270`, which fixes the left half but leaves ~half the labels upside down at rest. Recommend the rest-frame version.
|
||||
|
||||
### F-2 — HIGH — Winner is not legible in the poster frame
|
||||
`WheelComposition.jsx:32-37, 118`. During the hold the only winner signal is a `drop-shadow` glow on the winner `Pie` plus the edge pointer. At 200 px the pointer is ~17 px and the winner name ~9 px. The frame most chat clients show as a static thumbnail does not say who won.
|
||||
|
||||
**Fix** — winner-name pill over the hub during the hold. Reads at 200 px, costs 1–2 DOM nodes for 18 frames, 2 palette colours, and matches wheelofnames.com's winner modal convention:
|
||||
|
||||
```jsx
|
||||
{frame >= holdStartFrame && (
|
||||
<div style={{
|
||||
alignItems: 'center', background: theme.winnerPillBg, borderRadius: size * 0.03,
|
||||
color: theme.winnerPillText, display: 'flex', fontSize: winnerFontSize, fontWeight: 800,
|
||||
justifyContent: 'center', left: '50%', lineHeight: 1.15, maxWidth: size * 0.74,
|
||||
outline: `${Math.round(size * 0.008)}px solid ${theme.winnerPillRing}`,
|
||||
padding: `${size * 0.022}px ${size * 0.042}px`, position: 'absolute', textAlign: 'center',
|
||||
top: '50%', transform: `translate(-50%, -50%) scale(${0.72 + 0.28 * pillPop})`,
|
||||
whiteSpace: 'pre-line', zIndex: 12,
|
||||
}}>{winnerLines.join('\n')}</div>
|
||||
)}
|
||||
```
|
||||
with `const pillPop = interpolate(frame, [holdStartFrame, holdStartFrame + 4], [0, 1], {easing: Easing.out(Easing.cubic), extrapolateLeft: 'clamp', extrapolateRight: 'clamp'})`, `winnerFontSize` derived from the existing `estimateTextWidth` against `size * 0.66` starting at `size * 0.085` (≈43 px at 512) and floored at `size * 0.045`, `winnerLines = getLabelLines(option, winnerFontSize, size * 0.66, 2)`.
|
||||
New theme tokens: classic `winnerPillBg '#111827'`, `winnerPillText '#ffffff'`, `winnerPillRing '#facc15'`; festival `'#7f1d1d' / '#fffbeb' / '#f59e0b'`; mono `'#18181b' / '#fafafa' / '#a1a1aa'`. Contrast: 17.7:1, 14.2:1, 16.9:1.
|
||||
`zIndex: 12` puts it above the confetti layer (`Confetti.jsx` uses `zIndex: 10`) so the name is never occluded — that is the only confetti interaction in scope here.
|
||||
Determinism: pure `frame` function. GIF size: flat solid fill, adds ~2 palette entries; the pill is static for 14 of 18 hold frames so inter-frame delta is near zero. Render time: negligible.
|
||||
|
||||
### F-3 — HIGH — Spin curve strobes at the start and dies at the end
|
||||
`src/remotion/wheel-layout.js:27` (`fullTurns = 7`) and `:52-57` (`1 - (1 - p)**3`).
|
||||
Measured for the default 8 options / 15 fps / 6500 ms spin (98 spin frames, 2432° total):
|
||||
|
||||
| | current cubic, 7 turns | easeOut^2, 4 turns | **proposed: 12% ease-in + easeOut^2.2, 5 turns** |
|
||||
|---|---:|---:|---:|
|
||||
| peak Δ/frame | **73.7°** (1105°/s) | 27.4° | 40.6° |
|
||||
| frames > 45° (one slice) → strobe | **22** (1.47 s) | 0 | 0 |
|
||||
| frames < 4°/frame → crawl | **23** (1.53 s) | — | 15 |
|
||||
| frames < 1.5°/frame → sub-perceptible | **14** | — | 6 |
|
||||
| rotation per second | 954 / 665 / 428 / 243 / 110 / 30 / 1 | — | 233 / 525 / 403 / 286 / 177 / 79 / 9 |
|
||||
|
||||
74°/frame against a 45° slice is above the stroboscopic threshold: the wheel appears to jitter or run backwards for the first 1.5 s (19 % of the GIF). Then the last 1.5 s crawl, and the wheel is fully frozen from f91 — 0.47 s dead tail *plus* the 1.20 s hold = 1.67 s of a still image before the GIF loops.
|
||||
|
||||
**Fix** — replace `getSpinProgress`:
|
||||
|
||||
```js
|
||||
const accelFraction = 0.12;
|
||||
|
||||
export const getSpinProgress = (frame, spinFrames) => {
|
||||
const p = clamp(frame / Math.max(1, spinFrames), 0, 1);
|
||||
if (p < accelFraction) {
|
||||
return 0.5 * (p / accelFraction) ** 2 * accelFraction;
|
||||
}
|
||||
const q = (p - accelFraction) / (1 - accelFraction);
|
||||
return accelFraction / 2 + (1 - (1 - q) ** 2.2) * (1 - accelFraction / 2);
|
||||
};
|
||||
```
|
||||
and `fullTurns = 5` at `wheel-layout.js:27`. Verified numerically: continuous, monotonic, `progress(spinFrames) === 1` exactly, so the winner still lands dead centre under the pointer (checked: `rot(98) = 1687.500` = target). Determinism: still a pure function of `frame`. GIF size: **smaller** — 1712° instead of 2432° means smaller per-frame pixel deltas and better LZW runs. Render time: unchanged.
|
||||
Existing tests on `getSpinProgress` will need updated expectations — flag as an intentional behaviour change, not a public contract change.
|
||||
|
||||
### F-4 — HIGH — LCD sub-pixel antialiasing poisons the GIF palette
|
||||
`WheelComposition.jsx:124-154`. Sampled pixels at a glyph edge on `#f97316`: `#62B4EC`, `#111F7D`, `#C2F6BC`, `#F97391` — saturated cyan/blue/green fringes where only browns are possible. Chromium is rendering the HTML labels with RGB sub-pixel AA. Remotion already passes `--font-render-hinting=none` (`node_modules/@remotion/renderer/dist/open-browser.js:150`) but **not** `--disable-lcd-text`, and `chromiumOptions` does not accept arbitrary flags.
|
||||
|
||||
Measured palette pressure per region (source PNG, before quantisation):
|
||||
|
||||
| region | unique colours |
|
||||
|---|---:|
|
||||
| one 90×30 label crop (`carol`) | **268** |
|
||||
| mono 100×30 label crop | 236 |
|
||||
| shadow band (512×60, bottom) | 119 |
|
||||
| mono flat slice interior 60×60 | 46 |
|
||||
| whole 512² still (classic) | 6350 |
|
||||
|
||||
Consequence: GIF frame 0 uses **249/256** colours in classic and **254/256** in `mono` — a theme with ~15 intended colours exhausts the palette. Everything else then dithers.
|
||||
|
||||
**Fix** — force grayscale AA by promoting the label to a composited layer. Append `translateZ(0)` to the existing transform at `WheelComposition.jsx:144`:
|
||||
```js
|
||||
transform: `translate(-50%, -50%) rotate(${label.rotation}deg) translateZ(0)`,
|
||||
```
|
||||
Zero DOM/render cost, deterministic. **Confidence high but unverified in this repo** (I could not test it without editing source). Verification recipe: render one still, then
|
||||
`magick still.png -crop 90x30+345+240 +repage -format "%k\n" info:` — expect the 268 to drop below ~40. If `translateZ(0)` is insufficient, `opacity: 0.999` on the same element has the same layer-promotion effect.
|
||||
Expected payoff: the largest single lever on both GIF size and dither quality.
|
||||
|
||||
### F-5 — HIGH — `mono` theme is not a usable wheel
|
||||
`src/remotion/themes.js:26-37`. `slices[4] = '#f4f4f5'` is **identical to `background`** (contrast 1.00) so the wheel silhouette disappears wherever that slice sits; `stroke="#ffffff"` (`WheelComposition.jsx:115`) gives 1.10:1 against it so the dividers vanish too; two adjacent pairs are ΔE 10–12. At 200 px it reads as a grey blob, not a wheel.
|
||||
|
||||
**Fix** — three changes:
|
||||
1. Theme-specific divider. Replace the hardcoded stroke with a token: `stroke={theme.sliceStroke}` — classic/festival `'#ffffff'`, mono `'#71717a'` (4.0:1 against the lightest slice).
|
||||
2. Re-ramp mono to even L\* steps in a band that keeps `#18181b` above 4.5:1, ordered to maximise adjacent ΔE (verified: min adjacent ΔE **16.4**, up from 10.1; darkest slice still 4.97:1 for text):
|
||||
```js
|
||||
slices: ['#87878f', '#cacad0', '#9d9da5', '#e1e1e5', '#b3b3ba', '#f9f9fa'],
|
||||
```
|
||||
Text contrasts: 4.97 / 10.86 / 6.58 / 13.59 / 8.50 / 16.84 — all pass 4.5:1.
|
||||
3. Add the bezel from F-6, which is what actually rescues the silhouette.
|
||||
Determinism/render time: unaffected. GIF size: fewer, better-separated colours → slightly smaller.
|
||||
|
||||
### F-6 — MEDIUM — Large soft `wheelShadow` buys little and costs palette; the wheel has no silhouette
|
||||
`WheelComposition.jsx:74` with `themes.js:4,16,28` (`0 18px 52px rgba(...)`). At 300 % zoom the ramp is actually smooth — no visible banding — and because the shadow div does not rotate it is byte-cheap after frame 1. But it consumes 119 palette entries and it does nothing for the real problem: on a dark chat background the GIF is a bright square with no defined wheel edge, and in `mono` there is no edge at all.
|
||||
|
||||
**Fix** — trade the diffuse shadow for an opaque bezel ring plus a tight drop:
|
||||
```js
|
||||
// under the rotating layer, above the shadow div
|
||||
<Circle fill={theme.bezel} radius={radius + size * 0.016}
|
||||
style={{left: center - radius - size * 0.016, position: 'absolute', top: center - radius - size * 0.016}} />
|
||||
```
|
||||
tokens: classic `bezel '#1f2937'` (14.0:1 vs bg), festival `'#7c2d12'`, mono `'#3f3f46'`; and shrink the shadow to `'0 6px 14px rgba(15, 23, 42, 0.20)'` (footprint ~4× smaller, ~30 palette entries instead of 119).
|
||||
Payoff: the wheel reads as a deliberate object on both light and dark chat backgrounds, mono gets an edge, and ~90 palette slots go back to the slices. Determinism: static. Render time: +1 SVG circle.
|
||||
Note the bezel must be inside the *non-rotating* stack so it does not add per-frame delta.
|
||||
|
||||
### F-7 — MEDIUM — Adjacent slices collide under colour-vision deficiency; palettes are orderable for free
|
||||
`themes.js:12,24,36`. Brute-forced all cyclic orderings, scoring each adjacent pair by `min(ΔE_normal, ΔE_deutan, ΔE_protan)`:
|
||||
|
||||
| theme | current min | reordered min | change |
|
||||
|---|---:|---:|---|
|
||||
| classic | 15.9 | **48.1** | reorder only, same 8 hexes |
|
||||
| festival | 11.7 | **37.9** | reorder only, same 8 hexes |
|
||||
| mono (current hexes) | 10.1 | 11.3 | reorder cannot fix — needs F-5 re-ramp |
|
||||
|
||||
```js
|
||||
// classic
|
||||
slices: ['#f97316', '#14b8a6', '#f472b6', '#60a5fa', '#facc15', '#a78bfa', '#fb7185', '#34d399'],
|
||||
// festival
|
||||
slices: ['#fb7185', '#06b6d4', '#84cc16', '#a78bfa', '#f59e0b', '#f472b6', '#fb923c', '#22c55e'],
|
||||
```
|
||||
Zero new colours, zero render/size cost, 3× better worst-case separation. Worst current offenders that disappear: classic `#a78bfa`→`#34d399` (ΔE 16 deutan — purple and green look identical), festival `#06b6d4`→`#a78bfa` (ΔE **12 protan**).
|
||||
|
||||
### F-8 — MEDIUM — First and last slice share a colour when `optionCount % paletteLength === 1`
|
||||
`WheelComposition.jsx:98`. With 9 options on the 8-colour palettes, slice 8 and slice 0 are both `#f97316` and sit adjacent — verified visually, they merge into one double-width wedge with only a 2 px white line between. Same at 17 options, and at 7/13 options in `mono` (6 colours).
|
||||
|
||||
**Fix**
|
||||
```js
|
||||
const paletteLength = theme.slices.length;
|
||||
const colorIndex =
|
||||
index === props.options.length - 1 && props.options.length % paletteLength === 1
|
||||
? Math.floor(paletteLength / 2)
|
||||
: index % paletteLength;
|
||||
const color = theme.slices[colorIndex] ?? theme.slices[0] ?? '#cccccc';
|
||||
```
|
||||
Worth a unit test asserting min adjacent ΔE ≥ 30 for every `optionCount` in 2..32 across all three themes.
|
||||
|
||||
### F-9 — MEDIUM — `pointerStroke` and the pointer `boxShadow` are silently discarded
|
||||
`WheelComposition.jsx:173-175`. `clip-path` clips the element's whole rendering, including border and box-shadow. Result (confirmed in a 500 % crop): the diagonal edges have no outline at all, the box-shadow is entirely gone, and only a 2 px sliver of `#991b1b` survives on the vertical back edge — reading as an accidental dark stripe. Combined with contrast 1.34:1 against `#f97316`, the pointer tip is invisible when the winner slice is orange or salmon.
|
||||
|
||||
**Fix** — draw the pointer as SVG so the stroke is real:
|
||||
```jsx
|
||||
<svg width={size * 0.10} height={size * 0.105} viewBox="0 0 100 105" style={{
|
||||
left: center + radius - size * 0.045, position: 'absolute', top: center - size * 0.0525, zIndex: 4,
|
||||
}}>
|
||||
<polygon points="4,52.5 96,5 96,100" fill={theme.pointer}
|
||||
stroke={theme.pointerStroke} strokeWidth={7} strokeLinejoin="round" />
|
||||
</svg>
|
||||
```
|
||||
Drop the `boxShadow` entirely (it never rendered and a blurred shadow costs palette). The rounded-join dark outline gives ≥3:1 against every slice in every theme. 1 extra DOM node, no filter, deterministic.
|
||||
|
||||
### F-10 — MEDIUM — Pointer has 0.5 % framing margin and no reaction to slice boundaries
|
||||
`WheelComposition.jsx:176-180`. Verified geometry: at `size=512`, `left = 256 + 209.92 - 10.24 = 455.68`, `width = 53.76`, so the back edge sits at **x = 509.44 — 2.56 px from the canvas edge**; at `size=384` it is **1.92 px**. Not clipped, but visually flush with the frame, and `AbsoluteFill`'s `overflow: hidden` means any outline or size increase *will* clip. Meanwhile the tip overlaps the rim by only `size*0.02` (10 px) and the pointer never moves — the strongest "this is a real wheel" cue is missing.
|
||||
|
||||
**Fix (a) framing** — the F-9 snippet already moves it: `left = center + radius - size * 0.045`, `width = size * 0.10` → right edge 494 px, **18 px** margin, and tip overlap rises to 23 px. The 0.41 radius leaves 46 px margins, so this fits comfortably; no need to change `radius`.
|
||||
|
||||
**Fix (b) tick/flapper** — deterministic, zero DOM cost, resolves to exactly 0 at rest (proved below):
|
||||
```js
|
||||
// wheel-layout.js
|
||||
export const getPointerDeflectionDegrees = (rotationDegrees, sliceDegrees, maxDeflection = 9) => {
|
||||
const phase = (((rotationDegrees % sliceDegrees) + sliceDegrees) % sliceDegrees) / sliceDegrees;
|
||||
return -maxDeflection * Math.max(0, 1 - phase * 3);
|
||||
};
|
||||
```
|
||||
applied as `transform: `rotate(${deflection}deg)`` with `transformOrigin: '100% 50%'` on the pointer SVG.
|
||||
Rest proof: `finalRotation % sliceDegrees = (turns*360 - winnerIndex*slice - slice/2) % slice`; `360 = n*slice`, so the first two terms are exact multiples and the remainder is always `slice/2` → `phase = 0.5` → deflection `0`. The flapper always parks neutral, whatever the winner or option count.
|
||||
During the slow tail this produces 4–6 legible flicks — by far the cheapest large gain in "reads as a real spin". Determinism: pure `frame` function via `rotation`. GIF size: a few px of the pointer change per frame; negligible.
|
||||
|
||||
### F-11 — MEDIUM — Winner glow is an asymmetric smear, not a glow
|
||||
`WheelComposition.jsx:118`. `drop-shadow` on a `Pie` renders *outside* the shape, so it is overpainted by every later-indexed sibling `Pie` and only survives on the counter-clockwise neighbour. In the 8-option render the yellow glow appears as a dirty greenish smudge bleeding onto the teal `bob` slice and nowhere else. Also `winnerPulse = Math.sin(settle * Math.PI)` is 0 at both ends of the hold, so the final frame gets the *minimum* glow (10 px) — the poster frame gets the weakest treatment. And `filter`+blur is the worst possible effect for a 256-colour palette.
|
||||
|
||||
**Fix** — replace the blur with an opaque stroke on the winner slice; drop the `drop-shadow` line entirely:
|
||||
```jsx
|
||||
<Pie
|
||||
fill={color}
|
||||
stroke={isWinner && frame >= holdStartFrame ? theme.winnerStroke : theme.sliceStroke}
|
||||
strokeWidth={isWinner && frame >= holdStartFrame ? 4 + Math.round(winnerPulse * 3) : 2}
|
||||
...
|
||||
/>
|
||||
```
|
||||
tokens: classic `winnerStroke '#111827'`, festival `'#7f1d1d'`, mono `'#18181b'`. Adds 1 colour, no blur, no z-order artefact, 4 discrete pulse states (cheap inter-frame delta). Keep `winnerGlow` only if the confetti reviewer still needs it; otherwise delete the token.
|
||||
Also rename or repurpose `settle` (`WheelComposition.jsx:32`) — it drives only the pulse; nothing settles.
|
||||
|
||||
### F-12 — MEDIUM — `lineHeight: 1` collides Vietnamese diacritics between wrapped lines
|
||||
`WheelComposition.jsx:137` + `wheel-label-layout.js:206` (`height: Math.min(availableHeight, fontSize * lines.length)`), with `overflow: 'hidden'` at `:138`.
|
||||
Rendered `Bún đậu mắm tôm Hà Nội` over 3 lines: the dot-below of `ậ` visually merges with the breve of `ắ` on the next line. No hard clipping was observed at 512 with Quicksand (`Ổn`, `Ừ`, `Ạ`, `Nghĩ`, `Phạm Dũng` all render complete), but the box height equals exactly `fontSize × lines`, so the margin is zero — any font fallback with taller metrics *will* clip, and the project explicitly supports Vietnamese.
|
||||
|
||||
**Fix** — give the line box breathing room and shrink to fit rather than clip:
|
||||
```js
|
||||
// WheelComposition.jsx
|
||||
lineHeight: 1.16,
|
||||
// wheel-label-layout.js, after fontSize is chosen
|
||||
const lineHeightRatio = 1.16;
|
||||
const blockHeight = fontSize * lineHeightRatio * lines.length;
|
||||
const fittedFontSize = blockHeight > availableHeight
|
||||
? Math.max(minLabelFontSize, Math.floor(fontSize * (availableHeight / blockHeight)))
|
||||
: fontSize;
|
||||
// return fontSize: fittedFontSize, height: Math.min(availableHeight, fittedFontSize * lineHeightRatio * lines.length)
|
||||
```
|
||||
Also update `maxLines` at `:185` to use the same ratio: `Math.floor(availableHeight / (baseFontSize * lineHeightRatio))`.
|
||||
Does not re-litigate the settled wrap/adaptive-size decisions — it only stops the box from being exactly flush.
|
||||
|
||||
### F-13 — MEDIUM — Font size is capped at 24 px, wasting low-option wheels
|
||||
`wheel-label-layout.js:24,156`. `baseSize = max(10, min(24, radius / max(5.8, optionCount * 0.33)))`. Because the `5.8` floor dominates until `optionCount > 17`, `radius / 5.8 = 36.2` at 512 — so **the 24 cap is active for every option count from 2 to 17**. A 2-option wheel with `Yes` / `No` renders 24 px text inside a 210 px-wide, 417 px-tall track: at a 200 px display that is 9.4 px. Verified in the 2-option still — enormous empty slices, tiny text.
|
||||
|
||||
**Fix** — raise the cap and clamp it by the wedge height so dense wheels do not regress:
|
||||
```js
|
||||
// in getRadialLabelLayout, before getLabelFontSize
|
||||
const maxFontSize = Math.min(radius * 0.15, availableHeight * 0.82); // 31.5px @512, and 21px @32 options
|
||||
```
|
||||
and pass it through instead of the hardcoded `24`. At 32 options `availableHeight = 26`, so `maxFontSize = 21` < the current 24 — which also removes a latent vertical clip that exists today. At 2 options it becomes 31.5 px (+31 % legibility at thumbnail size); width-fitting still shrinks long labels exactly as before, so no regression for `Nguyễn Văn 32`-style labels.
|
||||
|
||||
### F-14 — MEDIUM — Maximum accepted request produces an unusable artefact
|
||||
`src/schemas/wheel-request.js:38-41` allows `durationMs 10000 + holdMs 2500 + fps 20 + size 512` = 250 frames. Measured on this dev machine (single render, warm bundle):
|
||||
|
||||
| case | bytes | render |
|
||||
|---|---:|---:|
|
||||
| classic, 8 opts, 512, 15 fps, 7.7 s (default) | **2,036,868** | 4,255 ms |
|
||||
| mono, 6 opts, 512, 15 fps, 7.7 s | 1,775,252 | 2,688 ms |
|
||||
| festival, **32 opts, 512, 20 fps, 12.5 s** (schema max) | **14,261,781** | **10,274 ms** |
|
||||
|
||||
`docs/render-benchmarks.md:13-14` still says TBD for the 512 fixtures; these numbers fill that gap. A 14 MB GIF is hostile in a chat client, and 10.3 s here will exceed `RENDER_TIMEOUT_MS=15000` on the documented 1–2 vCPU container. Even the *default* 2.0 MB is heavy for inline playback on mobile.
|
||||
|
||||
**Fix, non-breaking first** — F-4 (grayscale AA) and F-6 (smaller shadow) free ~200 palette entries, and F-3 cuts total rotation from 2432° to 1712°; all three shrink per-frame entropy. I could not measure the combined saving without editing source — flag as expected, not proven.
|
||||
**If that is not enough**, the breaking option is to narrow the schema: reject `durationMs + holdMs` such that total frames > 160 (e.g. `superRefine` with a clear message), or drop `fps: 20`. That rejects requests that are valid today → **breaking change**, only justified if the container actually times out; measure in Docker first.
|
||||
|
||||
### F-15 — LOW — Light-only backgrounds look like a bright box on dark chat themes
|
||||
`themes.js:3,15,27` (`#f8fafc`, `#fff7ed`, `#f4f4f5`). Composited the three 200 px final frames on `#1e1e1e`: all three read as leaked white rectangles, and `mono` additionally has no discernible wheel edge.
|
||||
|
||||
**On transparency — do not pursue.** GIF transparency is a single binary palette index: the wheel's antialiased rim, the soft shadow and the text halo cannot blend against an unknown background, so you get hard white/grey fringes on every curve and glyph. On top of that, Telegram's `sendAnimation` re-encodes GIFs to MP4 for most clients, which drops alpha entirely — so the cost is real and the benefit does not survive the transport. (Whether Remotion's `codec: 'gif'` even emits alpha I did not verify; moot given the transport.)
|
||||
|
||||
**Recommended instead:** ship F-6's dark bezel — it gives the wheel an intentional edge on any background at the cost of one palette colour, which is 90 % of the benefit for none of the risk.
|
||||
**Optional, additive:** a 4th `night` theme (`background '#0b1220'`, `text '#0b1220'` on light slices, `textHalo 'rgba(255,255,255,0.9)'`, `bezel '#e2e8f0'`). Adding a value to the `theme` union is backward compatible for existing clients (no old value changes meaning), but it does extend the documented request contract and needs a README/`docs/miti99bot-integration.md` update — flagging it as a contract *addition*, and it conflicts with "three theme names are settled", so it needs your explicit call.
|
||||
|
||||
### F-16 — LOW — 4-direction text halo costs 4× text rasterisation for contrast that is already met
|
||||
`WheelComposition.jsx:142`. Every label paints 4 extra offset copies — 128 extra text draws per frame at 32 options. Section 2 shows `text:slice` is ≥5.39:1 in every theme on every slice, so the halo is not doing contrast work; it earns its keep only where confetti or a slice boundary crosses a glyph. It also thickens glyphs at small sizes.
|
||||
**Fix (low priority):** drop to 2 offsets (`0 1px 0`, `0 -1px 0`) — keeps the vertical separation that matters against slice borders at half the cost. Alternatively keep as-is; this is the least urgent item and worth re-checking only after F-4 lands, since the halo's colour cost is currently masked by the AA fringing.
|
||||
|
||||
### F-17 — LOW — Centre hub is decorative only and will be covered by the winner pill
|
||||
`WheelComposition.jsx:160-169`. Two concentric circles (`0.09` ring, `0.06` core) look clean and hide the label-track convergence — keep them. Just note they sit *under* the F-2 pill, which is fine (the pill is the payoff). No change needed; recorded so the hub is not "fixed" later by accident.
|
||||
|
||||
---
|
||||
|
||||
## 4. Prioritised list
|
||||
|
||||
### Quick wins (<30 min each)
|
||||
|
||||
| # | Change | Payoff | Risk |
|
||||
|---|---|---|---|
|
||||
| 1 | F-1 label flip (one derived value + `+ flip`) | Removes 3–8 unreadable names per render; biggest legibility gain | none — deterministic, no size/time cost |
|
||||
| 2 | F-7 palette reorder (two array literals) | worst-case adjacent ΔE 15.9→48.1 / 11.7→37.9 under CVD | none — same hexes |
|
||||
| 3 | F-4 `translateZ(0)` on the label | 268→(expected) <40 colours per label crop; unlocks the whole palette | low; **verify with `%k` count** — unproven |
|
||||
| 4 | F-3 spin curve + `fullTurns = 5` | kills 22 strobe frames and 14 dead frames; also shrinks the GIF | low; existing `getSpinProgress` test expectations change |
|
||||
| 5 | F-10b pointer tick | strongest "real wheel" cue; provably neutral at rest | none — pure `frame` function |
|
||||
| 6 | F-11 winner stroke instead of `drop-shadow` | removes the one-sided smear; winner strongest in the poster frame; drops a blur filter | none — fewer effects |
|
||||
| 7 | F-8 last-slice colour guard | fixes merged wedges at 7/9/13/17 options | none |
|
||||
| 8 | F-12 `lineHeight: 1.16` + fit-not-clip | ends diacritic collisions; removes latent clipping | low; slightly smaller 3-line labels |
|
||||
| 9 | F-9 pointer as SVG + F-10a reposition | pointer gets a real outline (≥3:1 on every slice) and 18 px framing margin | low; +1 DOM node |
|
||||
|
||||
### Larger reworks
|
||||
|
||||
| # | Change | Payoff | Risk |
|
||||
|---|---|---|---|
|
||||
| A | F-2 winner-name pill (new tokens, fit logic, pop easing) | The poster frame finally states the winner at 200 px — highest user-visible value in the report | medium: needs a size-fit path and 2-line wrap; +2 palette colours; must sit above `Confetti`'s `zIndex: 10` |
|
||||
| B | F-5 + F-6 mono re-ramp, `sliceStroke` token, bezel ring | Makes `mono` a real theme; gives all themes a silhouette on dark chat backgrounds; frees ~90 palette entries | medium: touches all three theme objects + the hardcoded `stroke="#ffffff"`; snapshot/visual review needed |
|
||||
| C | F-13 font-size cap by `radius * 0.15` clamped to `availableHeight * 0.82` | +31 % text size on 2–6 option wheels; removes a latent clip at 32 options | medium: changes sizing for every render — re-check 2, 8, 16, 24, 32 options |
|
||||
| D | F-14 GIF weight programme (measure in Docker after 3/4/6/B land, then decide) | Default 2.0 MB and max 14.3 MB are both too heavy | high if it ends in a schema narrowing — that is a **breaking change**; do the non-breaking levers first |
|
||||
| E | F-15 optional `night` theme | Fixes dark-background presentation properly | contract addition + conflicts with the settled 3-theme decision; needs your approval |
|
||||
|
||||
Suggested order: 1, 2, 3, 4, 5, 6, 7, 8, 9 → A → B → C → measure D.
|
||||
|
||||
---
|
||||
|
||||
## 5. Not verified empirically
|
||||
|
||||
- **F-4 fix effectiveness.** The sub-pixel fringing is measured and certain; that `translateZ(0)` (or `opacity: 0.999`) removes it in this Chromium build is high-confidence but untested, because testing requires editing `WheelComposition.jsx`. Verification recipe given in F-4.
|
||||
- **GIF size saving from F-3/F-4/F-6.** Direction is certain (less rotation, fewer colours, smaller shadow footprint); the magnitude is not measured. Palette pressure was measured indirectly (249–254 of 256 colours used in frame 0; 268 colours in one label crop; 119 in the shadow band).
|
||||
- **Render timeout risk in the deployment target.** 10,274 ms for the schema-max case was measured on this Windows dev machine, not on the documented 1–2 vCPU container. The `RENDER_TIMEOUT_MS=15000` breach is inferred, not observed.
|
||||
- **Remotion `codec: 'gif'` alpha support.** Not tested — moot, since F-15 recommends against transparency on transport grounds.
|
||||
- **Motion perception.** Strobing was derived numerically (22 frames above 45°/frame) from still-frame geometry, not judged by watching the GIF frame-stepped at 15 fps in a chat client.
|
||||
- **Font fallback.** All Vietnamese checks used Quicksand as resolved in this environment. Diacritic clipping margins on the `Inter` / `Noto Sans` / `Segoe UI` fallbacks in the `baseFont` stack (`WheelComposition.jsx:16-17`) were not tested — an extra reason to adopt F-12 rather than rely on `lineHeight: 1` fitting exactly.
|
||||
- `pnpm render:fixtures` was **not** run (it would rewrite `fixtures/*.gif`). Equivalent and broader coverage was rendered to the scratchpad instead; the repo is clean apart from other agents' reports.
|
||||
|
||||
---
|
||||
|
||||
## 6. Unresolved questions
|
||||
|
||||
1. **Winner pill (F-2)** — is covering the hub during the hold acceptable, or must the wheel stay fully visible? If it must, the only alternative at a square 512 canvas is shrinking `radius` from 0.41 to ~0.35 to open a caption band, which costs label size everywhere.
|
||||
2. **`night` theme (F-15)** — the three theme names are settled, but "GIFs look like a bright box on dark chat backgrounds" is the concern the brief itself raised. Add a 4th value (additive, backward compatible, needs docs) or accept the bezel-only mitigation?
|
||||
3. **GIF weight (F-14)** — is the 2.0 MB default acceptable to miti99bot, and has a 14 MB max-parameter render ever been requested in practice? The answer decides whether D ends in a schema narrowing.
|
||||
4. **Spin curve test expectations (F-3)** — existing `getSpinProgress` tests encode the cubic. Confirm they are treated as implementation detail, not a public contract.
|
||||
5. **`fullTurns = 5`** — 7 turns is currently hardcoded with a default parameter at `wheel-layout.js:27`. Should turn count stay fixed, or scale with `durationMs` (e.g. `Math.round(durationMs / 1300)`) so a 3 s request does not feel rushed and a 10 s one does not crawl?
|
||||
Reference in new issue
Block a user