Repository navigation
Full TypeScript migration, one-clock smoothness, single-controller guard - #6
Merged
Merged
Conversation
The browser produced frames at a hard-coded 40 fps while the relay ships at min(panel-measured, 37.5) fps. The ~2.5 fps mismatch meant a fresh frame was overwritten before sending roughly every 400 ms — a visible judder beat during motion studies. Frame production now adapts to the panel: applyStatus feeds the relay target into frame_timing, which paces uploads 1.5 fps under it (36 fps for a 37.5 relay). The producer never outruns the relay, so every rendered frame ships and the relay pads with imperceptible repeats instead of dropping visible animation steps. Measured on hardware: uploads at 35.99/s against the 37.5 relay with zero missed deadlines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two clients streaming concurrently fight last-writer-wins for the wall, which shows up as heavy frame drops (a hidden background tab did exactly this). The server now tracks which tagged source owns the frame stream: frames from any other source are rejected with 409 until the owner goes idle for 2s or the newcomer claims the wall from a real user gesture. The UI stops all of its producer loops on 409 and tells the user, and tags every frame with a per-tab id plus a claim flag set within 1.5s of a click or keypress. Untagged API callers remain ungoverned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two beat frequencies survived the 36 fps pacing fix. The relay ran its own 37.5 fps clock and re-sent the latest frame each tick, so a 36 fps producer beat against it about 1.5 times a second — one frame shown twice, felt as a rhythmic hiccup. And requestAnimationFrame quantized the producer to the display's 60 Hz grid, turning the 27.8 ms cadence into 33/33/33/17 ms bursts. The wall now follows a single clock: the relay forwards each fresh frame the moment it arrives (idle keepalives, which are visual no-ops, hold realtime mode open), and producers run on a drift-corrected timer with fixed motion steps instead of rAF wall-clock jumps. Telemetry now tracks fresh-frame cadence separately from keepalive repeats, and the stream health panel judges smoothness by that fresh cadence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…here Cradle-to-grave type safety without giving up the local-first design: - All 42 frontend modules are now TypeScript compiled by tsc --strict (zero errors). Sources live beside their emitted JS in public/, so the Python server still serves plain files from disk and the runtime keeps zero dependencies — typescript is build-time only. - Shared domain shapes live in public/types.ts; the emitted JS is generated by 'npm run build' and no longer tracked. - index.html (911 lines) is assembled byte-identically from five partials by a dependency-free build script, and scenes.ts split into browser UI + scene_apply.ts engine — every source file in the repo is now under 500 non-blank lines. - Backend types are enforced: mypy --strict passes over src/ and server.py (7 findings fixed) and runs in CI beside the coverage gate. - Fixed a real bug the migration surfaced: movie bake capture canceled the media sampling loop with cancelAnimationFrame, which no-ops on the paced-loop cancel functions introduced in d7f7b5d, leaving the loop running forever during and after a bake. - CI builds the frontend and runs browser tests against the compiled output; README documents the new build step and honest counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe pull request converts the frontend to strict TypeScript, assembles HTML from partials, adds paced frame delivery and fresh-frame telemetry, introduces tagged stream ownership arbitration, and updates CI, tests, documentation, and developer tooling. ChangesSquares Controller modernization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/draw_tools.ts (1)
169-199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the paced loop for clip playback.
requestAnimationFramecan select clip frames faster than the relay target rate. The sender can then coalesce updates and skip clip frames.Use
startPacedLoop(tick)forstate.animationFrame. Remove bothrequestAnimationFrameschedules from this function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/draw_tools.ts` around lines 169 - 199, Update startClipPlayback to use the paced playback scheduler: replace both requestAnimationFrame scheduling points in its tick flow with startPacedLoop(tick), assigning the returned animation handle to state.animationFrame as appropriate. Preserve the existing frame-index, rendering, and clip-selection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/grade-report.md:
- Line 5: Regenerate .claude/grade-report.md from the current repository tree,
updating the stack description to reflect TypeScript and its development
dependency, correcting the test count to match README.md, and replacing the
claims that no type checker exists with the strict mypy configuration and CI
execution details; alternatively, clearly label the report as a pre-PR snapshot.
In @.github/workflows/test.yml:
- Line 34: Add Node.js 20 alongside Node.js 22 in the frontend CI job matrix,
ensuring the workflow exercises both supported runtime versions while preserving
the existing Node 22 coverage.
- Around line 31-36: Update the workflow’s actions/checkout step to set
persist-credentials to false, and change the npm ci command to include
--ignore-scripts. Leave the explicit npm run build step unchanged.
In `@package.json`:
- Line 15: Update the package.json “watch” script so it also observes
public/partials/*.html and regenerates public/index.html through
scripts/build_html.mjs when those partials change, keeping the existing
TypeScript watch behavior intact.
In `@public/frame_timing.ts`:
- Around line 8-21: Update setUploadTargetFps so the computed upload rate never
exceeds the valid relayFps, especially when relayFps is below MIN_UPLOAD_FPS
plus UPLOAD_HEADROOM_FPS; retain headroom by selecting a positive rate below
relayFps while respecting the existing upper bound. Add regression coverage for
relay rates below 10 FPS and verify the resulting upload rate remains slower
than the relay.
In `@public/library_model.ts`:
- Around line 49-52: Update parseSceneTags so it accepts only string values or
arrays, ignoring other payload types instead of coercing them with String(value
?? ""). Preserve the existing array-processing behavior and return no tags for
objects, numbers, null, or other unsupported values.
In `@public/movie_model.ts`:
- Around line 3-6: Update controllerMovieFps so valid positive measured rates
produce a floored frame rate clamped to at least 1, while preserving the
existing 30 fallback for non-finite or non-positive inputs.
In `@public/output_model.ts`:
- Line 139: Reduce cognitive complexity in outputSignalPath by extracting the
offline, realtime, local, and idle path-construction branches into focused
helper functions. Keep outputSignalPath responsible only for selecting the
appropriate helper based on ControllerStatus, preserving all existing paths and
behavior.
In `@public/palette_model.ts`:
- Around line 43-63: Update normalizeSavedPalette to require typeof raw.id ===
"string" before applying SAFE_ID.test, and remove the reliance on String(raw.id)
coercion. Keep returning null for missing or non-string IDs while preserving the
existing validation for valid string IDs.
In `@public/scene_apply.ts`:
- Around line 51-75: The scene transition loop in the Promise currently uses
requestAnimationFrame directly, bypassing upload pacing. Replace the
requestAnimationFrame scheduling in the transition flow with startPacedLoop, and
cancel that paced loop when progress reaches 1 or when token no longer matches
state.transitionToken, preserving false resolution for superseded transitions
and true resolution on completion.
In `@public/segment_model.ts`:
- Around line 38-58: Update normalizeSegment’s effect assignment to validate the
normalized effect against the available effectPainters registry, rather than
relying only on SAFE_EFFECT. Preserve valid registered effects and fall back to
"tide" for missing, unknown, or unregistered values so
effectPainters[segment.effect] remains safe.
In `@public/transition_model.ts`:
- Around line 36-43: Reduce the cognitive complexity of transitionFrame by
moving each transition algorithm into its own dedicated helper, keeping shared
frame validation in transitionFrame. After validation, dispatch to the
appropriate helper based on the transition type while preserving existing
behavior and return values.
In `@public/types.ts`:
- Around line 170-193: Update the scene library boundary around normalizeLibrary
so inputs containing only id and name are not typed as fully populated
SceneSnapshot values. Separate the persisted/built-in input shape from
SceneSnapshot, or validate and supply required width, height, and pixels before
treating entries as SceneSnapshot; remove the built-in presets’ unknown cast and
preserve the normalized SceneSnapshot contract.
In `@README.md`:
- Around line 162-164: Update the test-suite description in README.md to remove
the claim that npm test performs a strict mypy pass, or move that check to a
separately documented local verification command; ensure the paragraph
accurately describes the checks actually run by npm test, including its frontend
build, Python tests, and Node tests.
In `@scripts/build_html.mjs`:
- Around line 14-20: Update the shell-loading try/catch around
readFile(shellPath, "utf8") to remove the success-path fallback for all read
errors; allow failures reading _shell.html to propagate and fail the build,
while preserving normal assignment to shell when the file is readable.
In `@src/http_api.py`:
- Around line 388-395: Update _execute_frame and _claim_frame_source so
ownership validation occurs before execute_command without mutating
frame_source_id or frame_source_seen. After execute_command completes
successfully, commit the new source and timestamp; preserve the existing
rejection behavior for competing sources and avoid changing ownership when
command validation or execution fails.
In `@test/net.test.mjs`:
- Around line 177-185: Update the flushFrame test around the mocked
globalThis.fetch response to queue another frame during the first failing
upload, then assert the 409 handling prevents a second upload. Track the upload
invocation count and preserve the existing assertions that the frame queue is
cleared and the connection remains active.
- Around line 164-167: Extend the assertions for the captured request bodies in
this test to verify that the pre-gesture request, bodies[0], does not claim
control. Keep the existing bodies[1].claim assertion to confirm only the
post-gesture request claims control.
---
Outside diff comments:
In `@public/draw_tools.ts`:
- Around line 169-199: Update startClipPlayback to use the paced playback
scheduler: replace both requestAnimationFrame scheduling points in its tick flow
with startPacedLoop(tick), assigning the returned animation handle to
state.animationFrame as appropriate. Preserve the existing frame-index,
rendering, and clip-selection behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 517c68e2-12f1-45b3-b971-f3e19669b440
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (76)
.claude/grade-report.md.claude/launch.json.github/workflows/test.yml.gitignoreREADME.mdpackage.jsonpublic/app.tspublic/app_state.tspublic/automation_model.tspublic/blend_model.jspublic/blend_model.tspublic/clip_model.tspublic/color_utils.tspublic/dom.jspublic/dom.tspublic/draw_tools.tspublic/effect_catalog.tspublic/effect_paint_utils.tspublic/effect_painters.tspublic/effect_painters_field.tspublic/effect_painters_particles.tspublic/effect_preview_model.tspublic/effects_ui.tspublic/frame_timing.jspublic/frame_timing.tspublic/index.htmlpublic/library_model.tspublic/library_sync.tspublic/live_audio.tspublic/live_input_model.tspublic/media_input.tspublic/media_model.jspublic/media_model.tspublic/monitor.tspublic/movie_model.tspublic/movies_bake.tspublic/net.tspublic/openapi.jsonpublic/output_model.tspublic/palette_model.tspublic/palettes.tspublic/partials/_shell.htmlpublic/partials/rack-media-text.htmlpublic/partials/rack-power-effects.htmlpublic/partials/rack-scenes-system.htmlpublic/partials/stage.htmlpublic/playback.tspublic/playlists.tspublic/render_core.tspublic/scene_apply.tspublic/scenes.tspublic/schedule_policy.tspublic/segment_model.tspublic/status_sync.tspublic/status_view.tspublic/stream_model.jspublic/stream_model.tspublic/text_studio.tspublic/transition_model.tspublic/types.tspublic/zone_model.tspublic/zones_segments.tspyproject.tomlscripts/build_html.mjssrc/http_api.pysrc/stream_telemetry.pysrc/twinkly_client.pysrc/twinkly_movies.pysrc/twinkly_protocol.pytest/frame_timing.test.mjstest/net.test.mjstest/paced_loop.test.mjstest/stream_model.test.mjstest/test_server_routes.pytest/test_twinkly_client.pytsconfig.json
💤 Files with no reviewable changes (6)
- public/frame_timing.js
- public/dom.js
- public/index.html
- public/stream_model.js
- public/media_model.js
- public/blend_model.js
…ents - media_input: make the blob:-URL invariant explicit before assigning media sources (CodeQL js/xss-through-dom). - Split transitionFrame, renderAudioFrame, and outputSignalPath into per-type/per-mode/per-state helpers (cognitive complexity), preserving every arithmetic expression. - Native elements over ARIA roles: toast is an <output>, the playlist progress is a real <progress>, brush and rotation clusters are <fieldset>s, with CSS neutralizing the fieldset/progress chrome. - Replace the boolean-selector brightnessFromStatus helper with the inline decision at its only call site. - Mechanical: crypto-based tab-id fallback instead of Math.random, ??= for the frame defer timer, optional-chain image cache check, top-level await at boot, Math.trunc in hashUnit (bit-identical via int32 coercion), catch parameters renamed, unused import dropped, npm ci --ignore-scripts in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion - frame_timing: slow relays get a proportional 20% margin so the producer can never be paced above the relay rate; regression tests. - http_api: frame ownership commits only after the frame command succeeds, so an invalid claimed frame cannot steal the wall; route test added. - Clip playback and scene transitions now run on the paced producer loop like every other frame source. - movie_model: floor controller movie fps at 1 (sub-1 fps measurement divided by zero downstream). - palette/segment models: ids and effects must be real strings — the 'undefined'-passes-the-regex coercion quirk is gone. - build_html: only a missing shell skips assembly; other read errors fail the build. - CI: persist-credentials disabled on checkouts; frontend matrix now exercises the documented Node 20 floor plus 22. - npm run watch:html for partial edits; README test-section wording matches what each command actually runs; grade report labeled as a pre-migration snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http_api.py (1)
163-166: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject valid JSON values that are not objects.
Line 163 only changes the static type.
json.loads()can return a list, string, number, ornull. A request such as[]reaches a route that callsbody.get(...), raises an uncaughtAttributeError, and does not return the expected 400 response.Validate the parsed value before returning it. Add a regression test for a non-object JSON body.
Proposed fix
- body: dict[str, Any] = json.loads(self.rfile.read(length)) + body = json.loads(self.rfile.read(length)) except json.JSONDecodeError as error: raise ValueError("Request body must be valid JSON.") from error + if not isinstance(body, dict): + raise ValueError("Request body must be a JSON object.") return body🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http_api.py` around lines 163 - 166, Update the request-body parsing method around json.loads to validate that the parsed value is a dictionary before returning it; reject arrays, scalars, and null with the existing ValueError path so routes receive the expected 400 response. Add a regression test covering a non-object JSON body such as an array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 16: Update the watch:html script to use a Linux-compatible file-watching
approach while preserving rebuilds through scripts/build_html.mjs when
public/partials changes; avoid relying on Node’s unsupported --watch-path option
on Linux.
In `@public/partials/rack-scenes-system.html`:
- Line 118: Add the missing closing fieldset tag in
public/partials/rack-scenes-system.html at lines 118-118 after the
display-orientation controls, and in public/partials/stage.html at lines 57-57
after the brush-size controls. Ensure each opened fieldset is properly closed
without changing the contained controls.
In `@public/transition_model.ts`:
- Around line 52-63: Update paintPush so incoming columns sample the rightmost
exposed portion of to during the transition, rather than using the
final-coordinate x directly; preserve from sampling for the remaining columns
and ensure the completed frame remains correctly aligned when progress reaches
the end.
---
Outside diff comments:
In `@src/http_api.py`:
- Around line 163-166: Update the request-body parsing method around json.loads
to validate that the parsed value is a dictionary before returning it; reject
arrays, scalars, and null with the existing ValueError path so routes receive
the expected 400 response. Add a regression test covering a non-object JSON body
such as an array.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 216c22a2-0851-4729-a570-714df5baef3f
📒 Files selected for processing (35)
.claude/grade-report.md.github/workflows/test.ymlREADME.mdpackage.jsonpublic/app.tspublic/draw_tools.tspublic/effect_catalog.tspublic/frame_timing.tspublic/library_model.tspublic/live_input_model.tspublic/media_input.tspublic/monitor.tspublic/movie_model.tspublic/net.tspublic/output_model.tspublic/palette_model.tspublic/partials/_shell.htmlpublic/partials/rack-media-text.htmlpublic/partials/rack-scenes-system.htmlpublic/partials/stage.htmlpublic/render_core.tspublic/scene_apply.tspublic/segment_model.tspublic/status_sync.tspublic/status_view.tspublic/styles-automation.csspublic/styles-composer.csspublic/styles-stage.csspublic/transition_model.tsscripts/build_html.mjssrc/http_api.pytest/brightness_status.test.mjstest/frame_timing.test.mjstest/net.test.mjstest/test_server_routes.py
💤 Files with no reviewable changes (1)
- public/status_sync.ts
…bject-only JSON bodies - The div→fieldset element swap left the original closing </div> tags, so both fieldsets swallowed their sibling controls; HTMLHint caught it. - paintPush now slides the incoming frame in from the left (its rightmost revealed columns show) instead of pinning it to final coordinates, which popped the right side in at completion; test asserts the slide and the jump-free endpoint. - build_html gains a --watch mode using plain fs.watch (node's --watch-path is unsupported on Linux). - read_json rejects valid-JSON non-objects (a [] body previously reached routes and raised AttributeError → 500); route test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Branch protection pins the check name 'browser-models'. Renaming that job to a 'frontend' matrix removed the context permanently, so the required check could never report again. Keep the matrix (Node 20 + 22) and add a fixed-name gate job that fails unless the whole matrix succeeded, so protection has one contract that survives matrix changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five commits that finish the smoothness work started in #5 and take the codebase to full type safety:
tsc --strictwith zero errors; shared domain types inpublic/types.ts; emitted JS is generated bynpm run buildand untracked.mypy --strictnow gates the backend in CI. index.html is assembled byte-identically from five partials, and every source file is under 500 non-blank lines. The migration surfaced and fixed a real bug: movie bake capture could not stop the media sampling loop (cancelAnimationFrameon a paced-loop cancel function no-ops), leaving it running forever.Changes
public/*.ts(43 files) — typed frontend sources;public/partials/— index.html partialstsconfig.json,package.json,scripts/build_html.mjs— build pipeline (TypeScript is the only dev dependency; runtime remains stdlib + static files)src/twinkly_client.py,src/stream_telemetry.py,src/http_api.py,src/twinkly_protocol.py— event-driven relay, frame-source arbitration, fresh-cadence telemetry, mypy-strict fixes.github/workflows/test.yml— frontend job builds before testing; mypy gate addedtest/— arbitration route tests, event-driven relay tests, paced-loop timer tests, updated telemetry/health testsTest plan
tsc --strict: 0 errors;mypy --strict: 0 issues🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Developer Experience