chore: wire together deployment planning#953
Conversation
📝 WalkthroughWalkthroughDeployment plan target shape moved per-target rendered fields into a new per-result array. Targets now include environment/resource IDs, a required boolean Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Pull request overview
Wires together deployment planning by reshaping the deployment plan response schema (targets + per-target results) and updating OpenAPI/SDK artifacts to match the API’s updated response shapes.
Changes:
- Split deployment plan data into
DeploymentPlanTarget(env/resource info) +DeploymentPlanTargetResult[](per-result status/output fields). - Update deployment plan endpoints to always return a non-null
summaryobject. - Regenerate OpenAPI JSON and generated TS types (API + workspace-engine SDK) to reflect the new schema/response codes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/workspace-engine-sdk/src/schema.ts | Adds a documented 404 response for getJobAgentsForDeployment in the generated SDK schema. |
| apps/api/src/types/openapi.ts | Updates generated API TS types for the new deployment plan target/result split. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Returns non-null plan summaries and maps plan targets/results into the new response shape. |
| apps/api/openapi/schemas/deployments.jsonnet | Updates the source OpenAPI schema definitions for deployment planning targets/results. |
| apps/api/openapi/openapi.json | Updates the generated OpenAPI JSON to match the revised schemas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hasChanges: r.hasChanges ?? false, | ||
| contentHash: r.contentHash ?? "", | ||
| current: r.current ?? "", | ||
| proposed: r.proposed ?? "", | ||
| message: r.message ?? "", |
There was a problem hiding this comment.
Coalescing nullable DB fields to false/"" here changes semantics and can make the response internally inconsistent: if r.hasChanges is NULL while status is computing, the API will return hasChanges: false for that result, but the summary.unchanged count is computed from the raw DB rows (r.hasChanges === false) and will not include NULLs. This can mislead clients into thinking there are unchanged results and also makes targets[].results[].hasChanges disagree with summary. Prefer omitting unknown fields (e.g., hasChanges: r.hasChanges ?? undefined, contentHash/current/proposed/message: ... ?? undefined) or alternatively compute the summary using the same coerced values so the counts match the returned results.
| hasChanges: r.hasChanges ?? false, | |
| contentHash: r.contentHash ?? "", | |
| current: r.current ?? "", | |
| proposed: r.proposed ?? "", | |
| message: r.message ?? "", | |
| hasChanges: r.hasChanges ?? undefined, | |
| contentHash: r.contentHash ?? undefined, | |
| current: r.current ?? undefined, | |
| proposed: r.proposed ?? undefined, | |
| message: r.message ?? undefined, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/openapi/openapi.json`:
- Around line 574-577: The boolean properties DeploymentPlanTarget.hasChanges
and DeploymentPlanTargetResult.hasChanges (and the other similar boolean fields
noted) are declared but not listed in their objects' required arrays, causing
generated clients to treat them as boolean | undefined; decide the intended
semantics and either add each field to the corresponding schema object's
required array (e.g., add "hasChanges" to the required list for
DeploymentPlanTarget and for DeploymentPlanTargetResult) if the server always
emits them, or update each property's description to explicitly document when
the field may be omitted (e.g., "optional; omitted until X" or similar) so
clients know how to distinguish false vs absent. Ensure you update every
occurrence mentioned so all schemas are consistent.
In `@apps/api/src/routes/v1/workspaces/deployments.ts`:
- Around line 437-445: The map that builds the serialized result objects (the
results: t.results.map block) currently forces nullable fields to concrete
values (hasChanges => false, contentHash/current/proposed/message => ""), which
hides "not yet produced" state for rows still computing; change the mapper to
preserve undefined for those fields (omit the null-coalescing defaults) so
hasChanges stays undefined when DB value is null, and likewise leave
contentHash, current, proposed, message as undefined until populated, or
alternatively compute the derived summary from the same normalized object so
both summary and exported fields reflect the same nullable/undefined semantics;
update the results mapping code (the mapper that returns id, status, hasChanges,
contentHash, current, proposed, message) accordingly.
🪄 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: 868025e0-f5d1-47f2-8b68-7c504ddfe017
📒 Files selected for processing (5)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/deployments.jsonnetapps/api/src/routes/v1/workspaces/deployments.tsapps/api/src/types/openapi.tspackages/workspace-engine-sdk/src/schema.ts
| results: t.results.map((r) => ({ | ||
| id: r.id, | ||
| status: r.status, | ||
| hasChanges: r.hasChanges, | ||
| current: r.current, | ||
| proposed: r.proposed, | ||
| contentHash: r.contentHash, | ||
| message: r.message, | ||
| hasChanges: r.hasChanges ?? false, | ||
| contentHash: r.contentHash ?? "", | ||
| current: r.current ?? "", | ||
| proposed: r.proposed ?? "", | ||
| message: r.message ?? "", | ||
| })), |
There was a problem hiding this comment.
Don’t serialize unknown result state as concrete values.
For rows that are still computing, hasChanges is still unknown in the database. Converting that to false here makes pending results look unchanged, while summary is still derived from the raw nullable value. contentHash/current/proposed/message have the same problem: "" is no longer distinguishable from “not produced yet”. Preserve undefined until those fields are actually populated, or compute the summary from the same normalized object.
Possible fix
results: t.results.map((r) => ({
id: r.id,
status: r.status,
- hasChanges: r.hasChanges ?? false,
- contentHash: r.contentHash ?? "",
- current: r.current ?? "",
- proposed: r.proposed ?? "",
- message: r.message ?? "",
+ hasChanges: r.hasChanges ?? undefined,
+ contentHash: r.contentHash ?? undefined,
+ current: r.current ?? undefined,
+ proposed: r.proposed ?? undefined,
+ message: r.message ?? undefined,
})),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/workspaces/deployments.ts` around lines 437 - 445, The
map that builds the serialized result objects (the results: t.results.map block)
currently forces nullable fields to concrete values (hasChanges => false,
contentHash/current/proposed/message => ""), which hides "not yet produced"
state for rows still computing; change the mapper to preserve undefined for
those fields (omit the null-coalescing defaults) so hasChanges stays undefined
when DB value is null, and likewise leave contentHash, current, proposed,
message as undefined until populated, or alternatively compute the derived
summary from the same normalized object so both summary and exported fields
reflect the same nullable/undefined semantics; update the results mapping code
(the mapper that returns id, status, hasChanges, contentHash, current, proposed,
message) accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/openapi/openapi.json`:
- Around line 574-597: The OpenAPI schema forces state-dependent fields to be
present for all DeploymentPlanTargetResult entries (fields: hasChanges,
contentHash, current, proposed, message) and derives
DeploymentPlanTarget.hasChanges from them, which prevents distinguishing
“computing/accepted” states; update the schema so those fields are optional/null
when status != "completed" or replace DeploymentPlanTargetResult with a
discriminated oneOf union keyed by status (e.g., completed vs
computing/errored/unsupported) so completed results require
hasChanges/contentHash/current/proposed/message while other statuses omit or
allow nulls, and ensure DeploymentPlanTarget.hasChanges is computed only from
completed-result variants.
🪄 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: f334695d-cb92-4402-b8d9-558e4288b207
📒 Files selected for processing (3)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/deployments.jsonnetapps/api/src/types/openapi.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/openapi/schemas/deployments.jsonnet
| "hasChanges": { | ||
| "description": "True if any result for this target has changes", | ||
| "type": "boolean" | ||
| }, | ||
| "resourceId": { | ||
| "type": "string" | ||
| }, | ||
| "environmentName": { | ||
| "resourceName": { | ||
| "type": "string" | ||
| }, | ||
| "results": { | ||
| "items": { | ||
| "$ref": "#/components/schemas/DeploymentPlanTargetResult" | ||
| }, | ||
| "type": "array" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "environmentId", | ||
| "environmentName", | ||
| "resourceId", | ||
| "resourceName", | ||
| "hasChanges", | ||
| "results" |
There was a problem hiding this comment.
Keep state-dependent plan fields nullable/variant by status.
DeploymentPlanTargetResult still allows "computing", "errored", and "unsupported", but the schema now requires hasChanges, contentHash, current, proposed, and message for every result, and DeploymentPlanTarget.hasChanges is derived from those results. That forces the API to invent false / empty-string placeholders for states where those values are unknown or not applicable, especially for the 202 accepted plan response. Clients then can't distinguish “unchanged” from “not computed yet” or “empty output” from “no output”.
Prefer either:
- keeping these fields nullable/optional outside
"completed", or - splitting the result schema with
oneOfkeyed bystatus.
Also applies to: 603-623, 635-642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/openapi/openapi.json` around lines 574 - 597, The OpenAPI schema
forces state-dependent fields to be present for all DeploymentPlanTargetResult
entries (fields: hasChanges, contentHash, current, proposed, message) and
derives DeploymentPlanTarget.hasChanges from them, which prevents distinguishing
“computing/accepted” states; update the schema so those fields are optional/null
when status != "completed" or replace DeploymentPlanTargetResult with a
discriminated oneOf union keyed by status (e.g., completed vs
computing/errored/unsupported) so completed results require
hasChanges/contentHash/current/proposed/message while other statuses omit or
allow nulls, and ensure DeploymentPlanTarget.hasChanges is computed only from
completed-result variants.
Resolves #950
Summary by CodeRabbit
Bug Fixes
Refactor