refactor(api): shared response types for effects and library (spec 76 wave 3.1c) - #205
Open
hyperb1iss wants to merge 4 commits into
Open
refactor(api): shared response types for effects and library (spec 76 wave 3.1c)#205hyperb1iss wants to merge 4 commits into
hyperb1iss wants to merge 4 commits into
Conversation
Nine effect responses move into hypercolor-types::api::effects so the daemon and both clients read one definition instead of a struct on the daemon side and a hand-rolled decode on the client side. RescanResponse was already a struct; the other eight were anonymous serde_json::json! literals covering stop, the three effect-to-layout link routes, both control PATCH routes, the control binding PUT, and the controls reset. Shape-preserving, deliberately. serde_json is built with preserve_order in the daemon's graph, so a json! literal emits keys in literal order and a struct emits them in declaration order: every replacement struct declares its fields in the literal's exact order. A literal also emits an explicit null for a None, so no field carries skip_serializing_if, which would drop the key instead. The serde(default) markers are deserialize-only and preserve client tolerance. The web UI's private ControlsVersionResponse decoded the effect-id controls PATCH through a one-field projection; it now reads the shared UpdateEffectControlsResponse, which carries the applied and rejected control maps the projection was discarding. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
Fourteen favorites, presets, and playlists responses move into hypercolor-types::api::library. Five were daemon-local structs and nine were anonymous serde_json::json! literals: both delete acks, the favorite add ack, the preset apply ack, and the playlist activate, active, and stop acks. Field order in every replacement struct matches its literal's key order, because preserve_order makes that order part of the wire, and no field carries skip_serializing_if. The mirrors the clients kept were narrower than what the daemon sends, so deleting them widens what those clients can see. Both the web UI and the TUI dropped the pagination envelope from their favorites and preset list decodes; the TUI's favorite rows kept only effect_id, discarding the resolved effect name and the timestamp; the UI's PresetSummary retyped the preset record's controls map as raw JSON values rather than the typed ControlValue the daemon serializes. The preset routes return the stored EffectPreset verbatim, so the UI now decodes that. EffectPreset gains serde(default) on its two timestamps. The deleted UI mirror tolerated their absence, and promoting a shared type must not narrow the set of JSON a client accepts. Defaults only affect deserialization, so the daemon still serializes both unconditionally. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
The template catalog's eight response shapes move into hypercolor-types::api::attachments: the listing and its summary, the single-template detail returned by GET, POST, and PUT, the delete ack that was a json! literal, and the category and vendor facet lists. Declaration order matches the daemon's prior order in every case. This closes real drift rather than only deduplicating. The web UI's TemplateSummary mirror had no image_url field at all, so the catalog's artwork was invisible to it, and it typed origin as an Option where the daemon sends the value unconditionally, which made every built-in template indistinguishable from a user-authored one at the type level. Its TemplateListResponse dropped the pagination envelope, leaving the UI unable to page a catalog it fetches with a hardcoded limit of 200. Deleting those mirrors hands the UI all three. The daemon's bytes do not move: these fields were always on the wire, only unread. create_attachment_template also decoded the POST response, which is a full TemplateDetail, into the narrower summary. It now decodes the detail, so a freshly created template's topology and LED positions survive the round trip. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
Adversarial review caught a wire change in the first pass. `json!` routes every value through `serde_json::to_value`, which stores an f32 as f64, and the widening is visible in the printed digits: 0.1f32 becomes 0.10000000149011612. A derived struct writes the f32 straight out as 0.1. So promoting a literal that carries a float reprints it, on top of the key-order and explicit-null hazards the wave already accounted for. Four responses carry f32 and stay literals: both control PATCH bodies, the control binding PUT, and the preset apply. Their payloads are `ControlValue` and `ControlBinding`, so the reprint would hit every slider, color, gradient stop, and binding bound rather than some edge case. Each site says why it was left alone, and the web UI keeps its narrow controls-version decode for the same reason. The remaining twenty-one promotions are byte-identical and unaffected. The list responses now mark `pagination` with serde(default), which needs Default on Pagination. The client mirrors this wave deletes had no pagination field at all and ignored the envelope, so without the default a body omitting it would newly fail to parse in both the web UI and the TUI. Pagination's explanation sits in a line comment because utoipa publishes the doc comment as the schema description, and editing it moves the generated OpenAPI client. Also documents why the UI keeps a local ActiveEffectResponse: it is a projection that unwraps the wire type's Option id and name for consumers that have already branched on `state`, not a mirror of the wire shape. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change centralizes API response contracts in ChangesShared API contract unification
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This PR centralizes shared API response types and updates clients without introducing an evidenced correctness or deployment risk; it is merge-ready after normal checks and review, with no actionable merge-blocking risk remaining. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
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.
Wave 3.1c of the Spec 76 internal API unification. The effects domain, the library domain, and the attachment-template catalog each had their REST response shapes defined on the daemon side and re-declared by hand on the client side. This moves all of them into
hypercolor-types::api, so there is one definition per shape and drift between the daemon and its clients becomes a compile error instead of a runtime parse failure.One commit per domain, plus a fourth that pulls back the four responses whose payloads carry floats. The daemon re-exports every promoted type from the module it used to own, so daemon-internal paths like
api::effects::RescanResponsestill resolve and no handler construction site changed shape.💎 What moves
Effects (
hypercolor-types::api::effects).RescanResponsewas already a struct. The other five were anonymousserde_json::json!literals: the stop acknowledgement, all three effect-to-layout link routes (GET, PUT, DELETE), and the controls reset.Library (
hypercolor-types::api::library). Five daemon-local structs (FavoriteSummary,FavoriteListResponse,PresetListResponse,PlaylistListResponse,ActivePlaylistResponse) and eightjson!literals covering both delete acknowledgements, the favorite add, and the playlist activate, active, and stop acknowledgements.Attachments (
hypercolor-types::api::attachments). The catalog listing and its summary, the single-template detail returned by GET, POST, and PUT, the delete acknowledgement, and the category and vendor facet lists.Twenty-one shapes in total. Client mirrors deleted: four in the web UI (
PresetSummary,PresetListResponse,FavoriteSummary,FavoriteListResponse), two more in its devices client (TemplateSummary,TemplateListResponse), and two in the TUI's REST client (FavoriteListResponse,FavoriteSummaryResponse).⚡ Why key order is the whole story here
serde_jsonis built with thepreserve_orderfeature in the daemon's dependency graph, which arrives transitively throughservo-script. Confirm it withcargo tree -e features -p hypercolor-daemon -i serde_json. That single fact makes JSON key order part of the wire contract: ajson!literal emits keys in literal order, a derived struct emits them in declaration order, and converting one to the other with the fields in a different sequence is a wire change rather than a refactor. Every replacement struct declares its fields in its literal's exact order.The second hazard is nulls. A
json!literal emits"key": nullfor aNone, so a replacement field carryingskip_serializing_ifwould silently drop the key. No promoted response field carries it. Theserde(default)markers throughout affect deserialization only and leave what the daemon writes untouched.The existing suite cannot see either hazard.
rest_v1_compat_tests.rs'sassert_keyssorts both sides before comparing, so it freezes the key set and never the key order, and every other daemon assertion isserde_json::Value-indexed and therefore order-agnostic. Nothing in the tree compares raw JSON strings. Shape preservation was proven instead with a standalone harness that serializes each promoted type withto_stringunderpreserve_orderand compares the resulting strings against the literals reconstructed verbatim from the pre-change source: 38 checks, all passing, with three negative controls (a swapped field order, an addedskip_serializing_if, and a directpreserve_orderliveness probe) that all trip as required. Types that were already structs are compared against verbatim replicas of their old definitions rather than throughjson!, for the float reason covered in the next section.🔥 Four responses stay literals, because floats reprint
There is a third hazard beyond key order and explicit nulls, and it is the one that nearly shipped.
json!routes every value throughserde_json::to_value, which stores anf32as anf64. The widening is visible in the printed digits:0.1f32becomes0.10000000149011612, while a derived struct writes thef32straight to the output and prints0.1. Promoting a literal that carries a float therefore changes the bytes.Four responses carry one, so they keep their literals and are not promoted in this wave: both control PATCH bodies (
/effects/active/controlsand/effects/{effect_id}/controls), the control binding PUT, and the preset apply. Their payloads areControlValueandControlBinding, which means the reprint would land on every slider value, every color picker, every gradient stop, and all six binding bounds rather than on some rare edge case. Each site carries a comment saying why it was left alone, and the web UI keeps its narrowControlsVersionResponsedecode for the same reason.The daemon is already inconsistent with itself here.
GET /api/v1/effects/activereturns a shared struct and so emits0.1, while the PATCH route emits0.10000000149011612for the same stored value. Converging them is a real improvement and a real wire change, so it belongs in a proposal with a test that pins the chosen format, not in a lockstep wave.The suite could not have caught this. Every float in the daemon's assertions for these routes is exactly representable (
7.5), andf32 as f64 == f64holds for those, so the assertions pass either way.🦋 The attachment catalog fix, and why it is client-side
The web UI's
TemplateSummarymirror had drifted from what the daemon sends. It had noimage_urlfield at all, so catalog artwork was invisible to it. It typedoriginas anOption<ComponentOrigin>where the daemon sends the value unconditionally, which left built-in and user-authored templates indistinguishable at the type level. And itsTemplateListResponsedropped the pagination envelope entirely, so the UI could not page a catalog it fetches with a hardcodedlimit=200.Deleting those mirrors in favour of the daemon's real shapes hands the UI all three. This is a client capability change, not a wire change: those fields were always on the wire, only unread. The daemon's serialized bytes for the catalog routes are identical before and after, which the string harness covers directly.
The UI's
create_attachment_templatewas also decoding the POST response, which is a fullTemplateDetail, into the narrower summary. It now decodes the detail, so a newly created template's topology and LED positions survive the round trip.🎯 Tolerance is preserved, not narrowed
Promoting a mirror must not shrink the set of JSON a client accepts, so every deleted mirror's
serde(default)markers were carried onto the shared type. Two of them landed outside the API modules:EffectPreset::created_at_msandupdated_at_msinhypercolor-types::librarygain defaults because the UI mirror they replace tolerated their absence. Defaults are deserialize-only, so the daemon still writes both fields unconditionally.That change also shrinks a blast radius. The whole library file parses in one
from_str, so under the old strictness a single preset missing a timestamp failed the entire load, taking favorites and playlists with it.The list responses mark
paginationwithserde(default)for the same reason, which is whyPaginationnow derivesDefault. The deleted client mirrors had nopaginationfield at all and ignored the envelope, so without it a body omitting pagination would newly fail to parse in both clients.Pagination's explanation sits in a line comment rather than a doc comment because utoipa publishes the doc comment as the schema description, and editing it moves the generated OpenAPI client.Three narrowings are accepted rather than papered over, none of which breaks against the real daemon. The preset record's
idandeffect_idare now thePresetIdandEffectIdnewtypes instead of bare strings, so a non-UUID id no longer parses. Itscontrolsmap isControlValueinstead of raw JSON. AndTemplateSummary.originis a bareComponentOrigin, so a missing key still defaults toBuiltInbut an explicitnullnow errors where it previously yieldedNone. These are the daemon's wire truth, which the contract module takes as its rule.What is deliberately unchanged
No
ToSchemaderives were added. None of these routes carry autoipa::pathattribute and none of their types appear incomponents(schemas), so a derive would emit nothing while registering them would publish orphan schemas.just python-generate-checkpasses with no movement in the generated client.The UI keeps its local
ActiveEffectResponse. It is a non-optional convenience projection built from the shared wire type rather than a mirror of it, and it stays documented as such.The install route's validation-error payload stays an anonymous literal. It is a detail object inside the error envelope rather than a response body, and the error surface belongs to a different domain.
Zero edits under
crates/hypercolor-daemon/tests/. The compat matrix and API pins pass as written.🧪 Gates
just verifycargo clippy --workspace --all-targets -- -D warningscargo check --all-targetscargo check --target wasm32-unknown-unknownjust ui-testjust python-generate-checkpython/render_thread_testsis flaky under a full-workspace run: thepipeline_gpu_*cases intermittently die with a SIGSEGV at process exit when every test binary is competing for the GPU. The suite passes 46 of 46 when the crate runs on its own, twice in a row, and this branch touches no render or engine code. It is the flake already tracked against the render thread, not a regression here.Running
cargo clippy -- -D warningsinsidecrates/hypercolor-uifails on two lints,too_many_argumentsinsrc/app/effect_state.rsandcollapsible_ifinsrc/pages/settings.rs. Both files are byte-identical tomainon this branch and no workflow runs clippy against the UI crate, so this is standing debt rather than anything this branch introduced. It is left alone.🔮 Proposals, not changes
Shape improvements are out of scope under the wave's lockstep doctrine, so these are written down rather than made.
Converge the float formatting. The four held-back responses should eventually be promoted, which means deciding that
0.1is the right output and pinning it with a daemon test that asserts a raw body containing a non-representable value. That also settles the existing disagreement betweenGET /effects/activeand the PATCH routes.The six list endpoints with fabricated pagination (effects, favorites, presets, playlists, scenes, profiles) still hardcode
offset: 0,limit: 50,has_more: falseand discard any paging query parameters. The compat suite freezes that behaviour deliberately. Making them page for real is its own change with its own compatibility story.PATCH /api/v1/effects/active/controlsreturnseffectas a bare name string while every sibling route returns an{ id, name }reference. The promoted type records the inconsistency in its doc comment rather than fixing it.The category and vendor facet lists carry no pagination envelope while the template listing does.
Nothing in the tree compares raw JSON strings, so no in-tree test can catch a key-order, explicit-null, or float-formatting regression on the REST wire. All three hazards in this wave were found by hand-built harnesses rather than by the suite. One golden raw-body fixture per promoted route would turn the whole class into ordinary test failures, and with several promotion waves still to come it is worth its own wave.
The UI's
ActiveEffectResponseprojection typesidandnameas non-Optionwhile the wire types them asOptionfor the idle body, so it cannot deserialize the daemon's own idle response.fetch_active_effectdecodes the shared type first and maps idle toNone, so nothing is broken today, but the projection is a decode-failure shape waiting for a caller that skips that step.Summary by CodeRabbit