refactor(api): name the REST request payloads into shared types (spec 76 wave 2.4) - #200
Merged
Conversation
Forty-two request and query shapes lived as anonymous structs inside daemon route modules, so no client could reference them and every consumer re-derived the shape by hand. They now live in hypercolor-types::api under their domain module, matching the wire field-for-field: same serde attributes, same defaults, same optionality. The daemon modules re-export them, so route handlers, the OpenAPI catalog, and the MCP adapters keep their existing paths. New api modules: assets, attachments, config, controls, diagnose, displays, layers, layouts, library, profiles, settings, simulators. The devices and effects modules grow their missing request types. Two structural notes. The layer request conversions move with their types, except the broadcast expansion, which reaches into hypercolor-core's SceneGroupLayerInsert and stays daemon-local as a free function. PatchLayerControlsRequest deliberately keeps its controls field free of serde(default) because the published schema marks it required and Option already admits an absent field. The config write body stays an untyped serde_json::Value: on that route the value itself is the body, so there is no shape to name. Every pinned suite passes unedited, which is the fence this wave was supposed to hold. The regenerated Python client carries only new description strings from the doc comments; no schema shape moved. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
The CLI had no dependency on hypercolor-types, so every request body was assembled with serde_json::json! against field names copied from the daemon by eye. It now depends on the types crate and constructs the shared request structs directly, which puts the daemon's contract behind the compiler for brightness, diagnose, discovery, identify, pairing, control-surface values and actions, effect apply, output power, active controls, control reset, effect layout, favorites, presets, playlists, profiles, and scene creation. Control values get the largest win. The CLI hand-wrote the driver algebra's kind/value tagging in fourteen match arms; parse_control_value now returns a real ControlValue and serde emits the same tagging, so a new variant is a compile error instead of a typo waiting to happen. Emitted bodies stay semantically identical. The one visible difference is that omitted optional fields are now absent rather than explicitly null, which every daemon type already accepted through Option. The pinned request-shape suite passes unedited. Two call sites keep an untyped body because no daemon contract accepts what they send, each now carrying a comment saying so: devices set-color posts a color field the device update contract does not define, and scenes activate posts transition_ms to a route with no request body. Both are pre-existing defects that need a contract decision. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
The web UI carried eleven hand-copied request structs and the TUI a twelfth, none of them fenced by anything: a daemon-side field rename would have compiled clean on both sides and failed at runtime. They now import the shared definitions, which deletes the copies and turns that class of drift into a compile error. Three of the copies had drifted in name only and are renamed at their call sites: UpdateLayoutApiRequest to UpdateLayoutRequest, CreatePresetRequest to SavePresetRequest, and the UI's ComponentBindingRequest to the ComponentBinding it duplicated. A fourth, SavePresetRequest, gains the daemon's Option around its controls field. Eight sites that hand-rolled bodies with serde_json become typed: identify, attachment identify, brightness, favorites, layer controls, display-face controls, output power, and active controls. The UI's From<&SceneLayer> impl becomes update_request_from_layer, since an inherent trait impl cannot follow the type into another crate. A new hypercolor-types suite fences the properties clients depend on: unset optionals serialize as absent, the identify attachment request flattens its base, control values carry the driver kind tagging, and playlist targets stay internally tagged. One pinned assertion changed. The UI's attachment binding test asserted that an unset name is omitted, which was true of the deleted mirror but not of the shared ComponentBinding, which emits an explicit null. The daemon reads both to None, proven by the new component_binding_accepts_absent_and_explicit_null_names test, so the pin now states the shared type's emission and keeps its original point that the UI sends defaults explicitly. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
The favorites POST was the last UI body still built with serde_json; it now constructs AddFavoriteRequest like every other call in the module. Regenerating the Python client picks up the layer route doc comments, which move only description strings. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 1 included review currently available under your plan. 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: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (20)
📒 Files selected for processing (62)
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 |
Naming the payloads into shared types changed what several clients put on the wire wherever a hand-built body spelled an unset optional differently from the shared struct. Nine bodies now omit a key they used to send as an explicit null, because serde_json::json! renders None as null while the shared types carry skip_serializing_if. Two move the other way and state a key their predecessor omitted, because the shared field carries serde(default) without skip_serializing_if. Every field involved is an Option or carries serde(default), so absent, explicit null, and empty all deserialize to the same Rust value and no handler branches on which spelling arrived. That was an argument in the PR body and is now a test: a macro decodes each affected type with the fields absent and again with explicit nulls, then asserts the two agree. Coverage spans the asset update, profile and scene creation, preset and playlist saves, playlist items, and both layer requests, with the pairing values map checked for the absent-versus-empty pair. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
The equivalence macro took field names as string literals, and no type in the crate declares deny_unknown_fields, so serde silently ignored a name that no longer matched the struct and the assertion held over nothing. A verifier demonstrated it three ways: a field that never existed, a misspelling, and a wrong name all passed green. Fields are now identifiers. The macro binds each one through a closure before building the payload, so a rename or a typo is a compile error, and the JSON key comes from the same identifier via stringify. All three attacks now fail to compile with E0609. The fixture also has to differ from the null-bearing payload, which catches a base that already carries the fields. Co-Authored-By: Nova (Claude Opus 5) <noreply@anthropic.com>
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.
💜 What this does
Spec 76 wave 2.4. Forty-two REST request and query shapes lived as anonymous structs inside daemon route modules. Nothing outside the daemon could name them, so every client rebuilt each shape by hand: the CLI assembled bodies with
serde_json::json!against field names copied by eye, and the web UI and TUI carried twelve mirror structs between them. A daemon-side field rename compiled clean on all four sides and failed at runtime.All forty-two now live in
hypercolor-types::apiunder their domain module, and the daemon, CLI, web UI, and TUI consume the single definition. That class of drift is now a compile error.🔮 The shared types
Twelve new modules join the four that already existed:
assetsAssetUploadQuery,AssetUpdateRequestattachmentsListTemplatesQueryconfigConfigApplyQuerycontrolsControlSurfaceListQuery,InvokeControlActionRequestdevices(extended)ListDevicesQuery,IdentifyAttachmentRequest,UpdateAttachmentsRequest,DiscoverRequest,ListLogicalDevicesQuery,CreateLogicalDeviceRequest,UpdateLogicalDeviceRequestdiagnoseDiagnoseRequestdisplaysDisplayFaceScope,SetDisplayFaceRequest,DisplayFaceScopeQuery,UpdateDisplayFaceControlsRequest,UpdateDisplayFaceCompositionRequesteffects(extended)SetEffectLayoutRequestlayersCreateLayerQuery,CreateLayerRequest,UpdateLayerRequest,LayerOrderRequest,PatchLayerControlsRequest,BroadcastMediaLayerTarget,BroadcastMediaLayerRequestlayoutsLayoutListQuery,CreateLayoutRequest,UpdateLayoutRequestlibraryAddFavoriteRequest,PlaylistTargetRequest,PlaylistItemRequest,SavePlaylistRequest,SavePresetRequest,ApplyPresetRequestprofilesCreateProfileRequest,UpdateProfileRequest,ApplyProfileRequestsettingsSetBrightnessRequestsimulatorsCreateSimulatedDisplayRequest,UpdateSimulatedDisplayRequestEach one matches the shape it replaced field for field, including the serde attributes that govern what the daemon accepts. The daemon modules re-export them, so route handlers, the OpenAPI catalog, and the MCP adapters keep the paths they already used.
⚡ Clients
The CLI gains a
hypercolor-typesdependency and builds real structs for brightness, diagnostics, discovery, identify, pairing, control-surface values and actions, effect apply, output power, active controls, control reset, effect layout, favorites, presets, playlists, profiles, and scene creation.The largest single win is control values. The CLI hand-wrote the driver algebra's
kind/valuetagging across fourteen match arms;parse_control_valuenow returns a realControlValueand serde emits the same tagging, so a new variant is a compile error rather than a typo nobody notices.The web UI drops eleven mirror structs and the TUI one, and fourteen hand-rolled bodies across both become typed. Three mirrors had drifted in name only and are renamed at their call sites:
UpdateLayoutApiRequestbecomesUpdateLayoutRequest,CreatePresetRequestbecomesSavePresetRequest, and the UI'sComponentBindingRequestbecomes theComponentBindingit duplicated.💎 Wire preservation
This wave changes no wire shape. The fence is that every pinned suite passes with its assertions untouched: the REST v1 compat matrix,
api_tests, the OpenAPI catalog test, and the CLI'srequest_shape_tests, which drives the real binary against a capturing server and asserts exact JSON bodies.What clients put on the wire does shift, in both directions, wherever a hand-built body spelled an unset optional differently from the shared type. Every such flip is listed below.
Eight request shapes now omit a key they used to send unconditionally, because
serde_json::json!renders aNoneasnulland the replaced UI mirrors declared these fields withoutskip_serializing_if:CreateProfileRequestdescriptionCreateSceneRequestdescriptionSavePresetRequestdescriptionSavePlaylistRequestdescriptionPlaylistItemRequestduration_ms,transition_msCreateLayerRequest,UpdateLayerRequestnameAssetUpdateRequestname,tagsSavePresetRequestcontrolsTwo move the other way and gain a key their predecessor omitted, because the shared field carries
serde(default)withoutskip_serializing_if:PairDeviceRequestvalues, now stated as{}ComponentBindinginsideUpdateAttachmentsRequestname, now stated asnullThe equivalence argument is the same for all of them: every field involved is an
Optionor carriesserde(default)on the daemon, so absent, explicitnull, and empty all deserialize to the identical Rust value, and no handler branches on which spelling arrived. That is now a fence rather than an argument.absent_and_explicit_null_optional_fields_decode_alikedecodes each affected type both ways and asserts equality, taking its field names as identifiers so a rename or typo is a compile error rather than a key serde would silently ignore,absent_and_empty_pairing_values_decode_alikedoes the same for the pairing map, andcomponent_binding_accepts_absent_and_explicit_null_namescovers the binding. The suite also pins the identify request's flattening, the driver control-value tagging, the display-face scope spellings, and the playlist target tagging.Two of those are type-level only, listed for completeness rather than because any request changes.
AssetUpdateRequest's sole caller athypercolor-ui/src/pages/media.rs:474always suppliesSomefor both fields.SavePresetRequest.controlswas a bareserde_json::Valueon the UI mirror, so the key was unconditional; the shared type wraps it inOption, and all three construction sites incomponents/preset_panel.rspassSome. Everything else on the list is reachable, including the layernameflip:hypercolor-ui/src/app/effect_state.rs:113creates layers withname: None.The regenerated Python client carries sixteen added
descriptionstrings and nothing else. Norequiredarray, property, type, or enum moved.PatchLayerControlsRequest.controlsdeliberately keeps noserde(default)so the published schema still marks it required, matching what ships; serde admits an absent field throughOptionregardless.🎯 One pinned assertion changed
crates/hypercolor-ui/tests/display_api_tests.rs::attachment_binding_request_keeps_explicit_defaults_on_wireasserted that an unsetnameis omitted from the wire. That was true of the deleted UI mirror, which carriedskip_serializing_if, and is not true of the sharedComponentBinding, which emits an explicitnull.The daemon reads both forms to
None, proven by the newcomponent_binding_accepts_absent_and_explicit_null_namestest, so nothing the daemon accepts has changed. The pin now states the shared type's emission and keeps its original point, which is that the UI sends defaults explicitly instead of letting the daemon reconstruct them.hypercolor_types::attachment::ComponentBindingitself is untouched, so the persisted and response forms of that type are unaffected.🦋 Two defects this surfaced
Typing the CLI's bodies exposed two commands that post payloads no daemon contract accepts. Both keep an untyped body with a comment saying why, because fixing either is a contract decision rather than a naming one.
hypercolor devices set-colorposts{"color": ...}toPUT /devices/{id}, which deserializesUpdateDeviceRequest. That type carriesname,enabled, andbrightnessand does not reject unknown fields, socoloris dropped silently; the handler then answers 422 from its own guard atdevices/mod.rs:277for having received none of the three fields it accepts. The command cannot have worked against this handler.hypercolor scenes activate --transitionposts{"transition_ms": ...}toPOST /scenes/{id}/activate, which takes no request body at all. Axum reads and discards it. The profiles equivalent does accepttransition_ms; the scenes route never grew it.🧪 Verification
just verifyis green apart from one GPU-timing test inrender_thread_teststhat fails under parallel load and reproduces on the base commit; the suite is 46/46 with--test-threads=1. Workspace clippy is clean, the out-of-workspace UI crate checks and tests separately, andgenerate_openapi_client.py --checkexits 0.🤖 Generated with Claude Code