refactor(sdl): placement-first form values schema#3263
Open
ygrishajev wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
🚧 Files skipped from review as they are similar to previous changes (22)
📝 WalkthroughWalkthroughThis PR refactors the SDL builder data model to separate placements from services: placements move to a top-level form array, services hold ChangesSDL builder placement refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/deploy-web/src/components/sdl/LogCollectorControl/LogCollectorControl.tsx (1)
70-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync
pricingindependently fromplacementId.After this refactor, pricing lives on the service. Right now the log collector only copies
pricingwhenplacementIdchanges, so editing the target service price leaves the generated log-collector service stale.Suggested fix
if (targetService.placementId !== logCollectorService.placementId) { changes.placementId = targetService.placementId; + } + + if ( + targetService.pricing.amount !== logCollectorService.pricing.amount || + targetService.pricing.denom !== logCollectorService.pricing.denom + ) { changes.pricing = targetService.pricing; } @@ - [logCollectorService, logCollectorServiceIndex, targetService.placementId, targetService.title, update, env] + [ + logCollectorService, + logCollectorServiceIndex, + targetService.placementId, + targetService.pricing.amount, + targetService.pricing.denom, + targetService.title, + update, + env + ] );🤖 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 `@apps/deploy-web/src/components/sdl/LogCollectorControl/LogCollectorControl.tsx` around lines 70 - 88, The current update logic only copies targetService.pricing when targetService.placementId changes, leaving logCollectorService.pricing stale; modify the change detection to compare pricing separately and set changes.pricing when logCollectorService.pricing !== targetService.pricing (in addition to the existing placementId check). Update the block that builds the changes object (referencing logCollectorService, targetService, and changes) so pricing is assigned whenever it differs, then call update(logCollectorServiceIndex, { ...logCollectorService, ...changes }) as before; ensure you still include placementId handling that also sets pricing when placementId changes if needed.apps/deploy-web/src/utils/sdl/data.ts (1)
39-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClone
nextCaseswhen seeding default HTTP options.Each service gets a fresh
httpOptionsobject, butnextCasesstill points at the shareddefaultHttpOptions.nextCasesarray. Mutating retry cases on one service will leak into every other service created from this factory.Proposed fix
httpOptions: { maxBodySize: defaultHttpOptions.maxBodySize, readTimeout: defaultHttpOptions.readTimeout, sendTimeout: defaultHttpOptions.sendTimeout, - nextCases: defaultHttpOptions.nextCases, + nextCases: [...defaultHttpOptions.nextCases], nextTries: defaultHttpOptions.nextTries, nextTimeout: defaultHttpOptions.nextTimeout }🤖 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 `@apps/deploy-web/src/utils/sdl/data.ts` around lines 39 - 89, The defaultService factory assigns httpOptions.nextCases by reference to defaultHttpOptions.nextCases, causing mutations to leak across services; update the httpOptions assignment inside defaultService to deep-clone nextCases (e.g., replace nextCases: defaultHttpOptions.nextCases with a cloned copy such as nextCases: defaultHttpOptions.nextCases.map(c => ({ ...c })) or use structuredClone/default deep-copy helper) so each ServiceType gets its own independent nextCases array; modify only the httpOptions construction in defaultService (referencing defaultService, httpOptions, and defaultHttpOptions.nextCases).
🧹 Nitpick comments (2)
apps/deploy-web/src/components/sdl/LogCollectorControl/LogCollectorControl.spec.tsx (1)
85-104: ⚡ Quick winExtend this propagation test to cover pricing too.
This scenario now verifies
placementIdonly, so the stale-pricing regression inLogCollectorControlstill passes. Adding a price change plus an assertion onservices.1.pricingwould lock the new contract down.🤖 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 `@apps/deploy-web/src/components/sdl/LogCollectorControl/LogCollectorControl.spec.tsx` around lines 85 - 104, The test "updates log-collector placement when target service placement is changed" only asserts placementId propagation and misses pricing; extend it to also change the target service pricing and assert the log-collector service pricing updates. After setting newPlacementId via form.setValue("services.0.placementId", newPlacementId), also set a new pricing value (e.g., const newPricing = { /* minimal contract shape used in tests */ }) using form.setValue("services.0.pricing", newPricing) inside act, then add a waitFor assertion that expect(form.getValues("services.1.pricing")).toEqual(newPricing). Use the same helpers already in the test (act, vi.waitFor, form.getValues) so the test verifies both placementId and pricing propagation for the LogCollectorControl.apps/deploy-web/src/utils/sdl/data.spec.ts (1)
20-25: ⚡ Quick winExercise a nested field in the isolation test.
This only proves top-level fields are independent. A nested alias like
expose[0].httpOptions.nextCasescan still be shared while this test stays green.Suggested test shape
it("defaultService returns independent copies", () => { const placement = defaultPlacement(); const a = defaultService(placement.id); const b = defaultService(placement.id); - a.title = "modified"; - expect(b.title).not.toBe("modified"); + a.expose[0].httpOptions.nextCases.push("403"); + expect(b.expose[0].httpOptions.nextCases).not.toContain("403"); });🤖 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 `@apps/deploy-web/src/utils/sdl/data.spec.ts` around lines 20 - 25, The test for defaultService independence only checks a top-level field; update it to also mutate and assert on a nested field to ensure deep copies are returned. In the test that calls defaultPlacement(), defaultService(placement.id) and creates a and b, modify a nested property such as a.expose[0].httpOptions.nextCases (e.g., push/replace an element or change a value) and add an assertion that b.expose[0].httpOptions.nextCases did not change; keep references to defaultService and defaultPlacement so the test targets the same factory functions.
🤖 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 `@apps/deploy-web/src/components/new-deployment/SdlBuilder.tsx`:
- Around line 95-102: When generateSdl(data) throws once, setError is never
cleared on subsequent successful generations so the error alert persists; update
the watch callback's success path (inside the try block where generateSdl,
lastSyncedSdlRef.current and setEditedManifest are used) to clear the error
(e.g., call setError("") or null) after a successful generation so the previous
error is removed.
In `@apps/deploy-web/src/components/sdl/PlacementFormModal.tsx`:
- Around line 185-194: The hook usePlacementIndexForService currently remaps a
missing placementId to 0 which causes bindings to target placements[0]; change
it to return the raw found index (allow returning -1 when not found) instead of
coercing to 0 so callers can detect invalid references (i.e., return index
as-is, not index >= 0 ? index : 0), update the function comment to document that
-1 means unresolved, and ensure callers (e.g., the modal fields) gate on
placementIndex >= 0 before binding to placements[placementIndex].
In `@apps/deploy-web/src/components/sdl/SimpleServiceFormControl.tsx`:
- Around line 97-102: The code sets currentPlacementIndex to 0 when
placements.findIndex(...) returns -1, causing a broken placementId to display
placements[0]; change the logic around useWatch/placements/currentPlacementIndex
so that if findIndex(...) === -1 you do not default to 0 but instead leave
currentPlacement undefined (e.g., set currentPlacementIndex to -1 or directly
compute currentPlacement = undefined when not found), and update the UI paths
that reference currentPlacement (the summary/edit affordance) to only render
when currentPlacement is truthy; refer to useWatch, placements,
currentPlacementIndex, currentPlacement and currentService.
In `@apps/deploy-web/src/types/sdlBuilder/sdlBuilder.ts`:
- Around line 387-405: The schema SdlBuilderFormValuesSchema currently only
ensures referenced placementIds exist but does not enforce that placement ids
are unique; update the superRefine on SdlBuilderFormValuesSchema to detect
duplicate placement ids in data.placements (iterate placements, track seen ids,
and when a duplicate is found call ctx.addIssue with code z.ZodIssueCode.custom,
a clear message like "Placement id must be unique.", path ["placements",
<index>, "id"], and fatal:true) so duplicates are rejected before service
resolution; keep the existing service->placement existence check intact.
In `@apps/deploy-web/src/utils/sdl/sdlImport.ts`:
- Around line 25-33: The function importSimpleSdl should guard against
non-object YAML roots returned by yaml.load before accessing yamlJson.services:
in importSimpleSdl, after const yamlJson = yaml.load(yamlStr) as any, validate
that yamlJson is a non-null object (e.g., typeof yamlJson === "object" &&
yamlJson !== null && !Array.isArray(yamlJson)) and if not either return the
empty { placements, services } or throw a clear validation error; update the
check that currently does if (!yamlJson.services) to first verify yamlJson is an
object so you don't get a TypeError when yaml.load returns undefined or a
scalar.
- Around line 125-139: The code dereferences placementProfile.pricing[svcName]
without checking it, which can cause a generic runtime crash; before using
placementPricing (the variable) validate that placementProfile.pricing exists
and that placementProfile.pricing[svcName] is defined, and if not throw a
CustomValidationError with a clear message including placementName and svcName;
update the block that sets placementPricing and service.pricing (and any code
that expects placementPricing.amount/denom) to perform this null/undefined check
and throw the CustomValidationError rather than allowing a TypeError.
---
Outside diff comments:
In
`@apps/deploy-web/src/components/sdl/LogCollectorControl/LogCollectorControl.tsx`:
- Around line 70-88: The current update logic only copies targetService.pricing
when targetService.placementId changes, leaving logCollectorService.pricing
stale; modify the change detection to compare pricing separately and set
changes.pricing when logCollectorService.pricing !== targetService.pricing (in
addition to the existing placementId check). Update the block that builds the
changes object (referencing logCollectorService, targetService, and changes) so
pricing is assigned whenever it differs, then call
update(logCollectorServiceIndex, { ...logCollectorService, ...changes }) as
before; ensure you still include placementId handling that also sets pricing
when placementId changes if needed.
In `@apps/deploy-web/src/utils/sdl/data.ts`:
- Around line 39-89: The defaultService factory assigns httpOptions.nextCases by
reference to defaultHttpOptions.nextCases, causing mutations to leak across
services; update the httpOptions assignment inside defaultService to deep-clone
nextCases (e.g., replace nextCases: defaultHttpOptions.nextCases with a cloned
copy such as nextCases: defaultHttpOptions.nextCases.map(c => ({ ...c })) or use
structuredClone/default deep-copy helper) so each ServiceType gets its own
independent nextCases array; modify only the httpOptions construction in
defaultService (referencing defaultService, httpOptions, and
defaultHttpOptions.nextCases).
---
Nitpick comments:
In
`@apps/deploy-web/src/components/sdl/LogCollectorControl/LogCollectorControl.spec.tsx`:
- Around line 85-104: The test "updates log-collector placement when target
service placement is changed" only asserts placementId propagation and misses
pricing; extend it to also change the target service pricing and assert the
log-collector service pricing updates. After setting newPlacementId via
form.setValue("services.0.placementId", newPlacementId), also set a new pricing
value (e.g., const newPricing = { /* minimal contract shape used in tests */ })
using form.setValue("services.0.pricing", newPricing) inside act, then add a
waitFor assertion that
expect(form.getValues("services.1.pricing")).toEqual(newPricing). Use the same
helpers already in the test (act, vi.waitFor, form.getValues) so the test
verifies both placementId and pricing propagation for the LogCollectorControl.
In `@apps/deploy-web/src/utils/sdl/data.spec.ts`:
- Around line 20-25: The test for defaultService independence only checks a
top-level field; update it to also mutate and assert on a nested field to ensure
deep copies are returned. In the test that calls defaultPlacement(),
defaultService(placement.id) and creates a and b, modify a nested property such
as a.expose[0].httpOptions.nextCases (e.g., push/replace an element or change a
value) and add an assertion that b.expose[0].httpOptions.nextCases did not
change; keep references to defaultService and defaultPlacement so the test
targets the same factory functions.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 605838cb-438d-4d30-99bd-de65c8bd6c61
📒 Files selected for processing (24)
apps/deploy-web/src/components/new-deployment/SdlBuilder.tsxapps/deploy-web/src/components/remote-deploy/update/RemoteDeployUpdate.tsxapps/deploy-web/src/components/sdl/AttributesFormControl.tsxapps/deploy-web/src/components/sdl/ImportSdlModal.tsxapps/deploy-web/src/components/sdl/LogCollectorControl/LogCollectorControl.spec.tsxapps/deploy-web/src/components/sdl/LogCollectorControl/LogCollectorControl.tsxapps/deploy-web/src/components/sdl/PlacementFormModal.tsxapps/deploy-web/src/components/sdl/SignedByFormControl.tsxapps/deploy-web/src/components/sdl/SimpleSdlBuilderForm.tsxapps/deploy-web/src/components/sdl/SimpleServiceFormControl.tsxapps/deploy-web/src/hooks/useImportSimpleSdl.tsapps/deploy-web/src/hooks/useSdlEnv/useSdlEnv.spec.tsxapps/deploy-web/src/hooks/useSdlServiceManager/useSdlServiceManager.spec.tsxapps/deploy-web/src/hooks/useSdlServiceManager/useSdlServiceManager.tsapps/deploy-web/src/types/sdlBuilder/sdlBuilder.spec.tsapps/deploy-web/src/types/sdlBuilder/sdlBuilder.tsapps/deploy-web/src/utils/sdl/data.spec.tsapps/deploy-web/src/utils/sdl/data.tsapps/deploy-web/src/utils/sdl/sdlGenerator.spec.tsapps/deploy-web/src/utils/sdl/sdlGenerator.tsapps/deploy-web/src/utils/sdl/sdlImport.spec.tsapps/deploy-web/src/utils/sdl/sdlImport.tsapps/deploy-web/src/utils/sdl/transformCustomSdlFields.tsapps/deploy-web/tests/seeders/sdlService.ts
ygrishajev
force-pushed
the
refactor/sdl-placement-first-form-values
branch
from
9aa6f53 to
d71f5a8
Compare
baktun14
reviewed
Jun 4, 2026
baktun14
approved these changes
Jun 4, 2026
…lues schema Reshapes SdlBuilderFormValuesSchema so placements are first-class: each placement carries a stable id and an optional region; services reference one via placementId and own their own pricing. generateSdl and importSimpleSdl read/write the new shape; SDL YAML output is byte-equivalent to the previous structure for the same logical input. The legacy single-placement wizard's form controls are rebound to the new field paths and behave identically. Bonus pre-existing bug fixes surfaced by the refactor: - signedBy.allOf-only placements were silently dropped from SDL output (typo in the legacy guard tested anyOf twice instead of anyOf || allOf). - A second IP-using service overwrote the first's endpoints block due to non-idempotent endpoints initialization.
ygrishajev
force-pushed
the
refactor/sdl-placement-first-form-values
branch
from
9b98baf to
f1f7d1d
Compare
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
2 participants
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.
Why
Closes CON-413. Foundational data-layer change for the redesigned configure-deployment screen (CON-411): every subsequent pane (CON-414..427) reads or writes one shared deployment model, and that model now treats placements as first-class entities. With the legacy embedded `service.placement` shape, empty placements have no host, renames cascade through every referencing service, and region is a global `generateSdl` arg with no per-placement granularity — none of which work for a UI that lets users create multiple placements and pick a provider per placement.
The new screen also needs a single source of truth that SDL generation and validation can reuse without re-authoring rules. Evolving the existing schema in place (rather than branching a parallel model) keeps the legacy wizard and the new redesign on one shape, and the legacy form continues to render identically against it.
What
Bonus pre-existing bug fixes surfaced by the refactor
Out of scope
Test plan
Summary by CodeRabbit
Refactor
Bug Fixes
Tests