fix: include deployment agent configs in v1 responses#910
fix: include deployment agent configs in v1 responses#910adityachoudhari26 wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughA new function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
This PR updates the v1 workspace deployments endpoints to include deployment job-agent link data (agent reference + per-deployment config) in the deployment objects returned by the API.
Changes:
- Added a DB lookup helper to fetch
deployment_job_agentrows for a set of deployment IDs. - Included
jobAgentsin thedeploymentobject returned by both list and get deployment endpoints. - Minor formatting/indentation adjustments around a few query blocks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import type { AsyncTypedHandler } from "@/types/api.js"; | ||
| import { ApiError, asyncHandler } from "@/types/api.js"; | ||
| import { Router } from "express"; | ||
| import _ from "lodash"; |
There was a problem hiding this comment.
lodash is imported here, but apps/api/package.json does not declare lodash as a dependency (only @types/lodash). With pnpm's strict node_modules layout this will likely fail at runtime/build. Either add lodash to this package's dependencies or replace the grouping logic with a small local reducer/Map to avoid the extra dependency.
| // 1 hour | ||
|
|
||
| import { validResourceSelector } from "../valid-selector.js"; | ||
| import { listDeploymentVariablesByDeploymentRouter } from "./deployment-variables.js"; | ||
|
|
There was a problem hiding this comment.
The // 1 hour comment is now separated from PLAN_TTL_MS and sits between import blocks, which makes it easy to miss/misread. Move the comment to be adjacent to PLAN_TTL_MS (or inline with the constant) so it documents the value it refers to.
| deployment: { | ||
| ...formatDeployment(dep), | ||
| jobAgents: (agentsByDeploymentId[dep.id] ?? []).map( | ||
| ({ ref, config }) => ({ ref, config }), |
There was a problem hiding this comment.
DeploymentJobAgent in the generated OpenAPI types includes a required selector field, but the response objects added here only contain { ref, config }. This makes the v1 response shape inconsistent with the documented schema/clients. Either include a selector value (e.g. default to "true") or update the OpenAPI schema/types to make selector optional/removed to match the persisted model.
| ({ ref, config }) => ({ ref, config }), | |
| ({ ref, config, selector }) => ({ | |
| ref, | |
| config, | |
| selector: selector ?? "true", | |
| }), |
| deployment: { | ||
| ...formatDeployment(dep), | ||
| jobAgents: (agentsByDeploymentId[dep.id] ?? []).map( | ||
| ({ ref, config }) => ({ ref, config }), |
There was a problem hiding this comment.
Same as in listDeployments: the jobAgents objects returned here omit the selector field that DeploymentJobAgent is documented to require in the generated OpenAPI types. Align the runtime response with the schema by including selector (or adjust the schema/types if selector is no longer part of the model).
| ({ ref, config }) => ({ ref, config }), | |
| ({ ref, config, selector }) => ({ ref, config, selector }), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/api/src/routes/v1/workspaces/deployments.ts (3)
128-135: Redundant object mapping.The
.map(({ ref, config }) => ({ ref, config }))on lines 130-132 destructures and reconstructs objects that already have the exact{ ref, config }shape returned bygetJobAgentsByDeploymentId.♻️ Suggested simplification
deployment: { ...formatDeployment(dep), - jobAgents: (agentsByDeploymentId[dep.id] ?? []).map( - ({ ref, config }) => ({ ref, config }), - ), + jobAgents: agentsByDeploymentId[dep.id] ?? [], },🤖 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 128 - 135, The mapping that rebuilds job agent objects is redundant: in the deployment assembly where you use formatDeployment(dep) and jobAgents: (agentsByDeploymentId[dep.id] ?? []).map(({ ref, config }) => ({ ref, config })), remove the .map and just use (agentsByDeploymentId[dep.id] ?? []) directly because getJobAgentsByDeploymentId already returns objects shaped as { ref, config }; update the deployment construction to assign jobAgents from agentsByDeploymentId without destructuring/reconstructing.
196-201: Same redundant mapping as inlistDeployments.The object mapping here is also redundant since
getJobAgentsByDeploymentIdalready returns the correct shape.♻️ Suggested simplification
deployment: { ...formatDeployment(dep), - jobAgents: (agentsByDeploymentId[dep.id] ?? []).map( - ({ ref, config }) => ({ ref, config }), - ), + jobAgents: agentsByDeploymentId[dep.id] ?? [], },🤖 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 196 - 201, The mapping that recreates { ref, config } is redundant because getJobAgentsByDeploymentId already returns that shape; in the deployment object building replace jobAgents: (agentsByDeploymentId[dep.id] ?? []).map(({ ref, config }) => ({ ref, config })) with jobAgents: agentsByDeploymentId[dep.id] ?? [] so the code uses the original array directly; update the deployment construction where formatDeployment(dep) and agentsByDeploymentId[dep.id] are used to remove the unnecessary .map step.
4-4: Consider using a named import for better tree-shaking.The default lodash import includes the entire library. Since only
chainis used, a targeted import would reduce bundle size.♻️ Suggested change
-import _ from "lodash"; +import { chain } from "lodash";Then update line 31:
- return _.chain(links) + return chain(links)🤖 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` at line 4, Replace the default lodash import with a targeted import for chain: remove the "import _ from 'lodash';" and instead import chain directly (either "import { chain } from 'lodash';" or "import chain from 'lodash/chain';") and update any usages of _.chain to call chain(...) so only the needed function is bundled.
🤖 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/src/routes/v1/workspaces/deployments.ts`:
- Around line 16-21: The inline comment "// 1 hour" is detached from the
constant it documents; move that comment so it sits directly above the
PLAN_TTL_MS declaration to make the intent clear. Locate the PLAN_TTL_MS
constant and reposition the comment immediately above it (remove or relocate the
stray comment before the imports) so the comment clearly documents PLAN_TTL_MS.
---
Nitpick comments:
In `@apps/api/src/routes/v1/workspaces/deployments.ts`:
- Around line 128-135: The mapping that rebuilds job agent objects is redundant:
in the deployment assembly where you use formatDeployment(dep) and jobAgents:
(agentsByDeploymentId[dep.id] ?? []).map(({ ref, config }) => ({ ref, config
})), remove the .map and just use (agentsByDeploymentId[dep.id] ?? []) directly
because getJobAgentsByDeploymentId already returns objects shaped as { ref,
config }; update the deployment construction to assign jobAgents from
agentsByDeploymentId without destructuring/reconstructing.
- Around line 196-201: The mapping that recreates { ref, config } is redundant
because getJobAgentsByDeploymentId already returns that shape; in the deployment
object building replace jobAgents: (agentsByDeploymentId[dep.id] ?? []).map(({
ref, config }) => ({ ref, config })) with jobAgents:
agentsByDeploymentId[dep.id] ?? [] so the code uses the original array directly;
update the deployment construction where formatDeployment(dep) and
agentsByDeploymentId[dep.id] are used to remove the unnecessary .map step.
- Line 4: Replace the default lodash import with a targeted import for chain:
remove the "import _ from 'lodash';" and instead import chain directly (either
"import { chain } from 'lodash';" or "import chain from 'lodash/chain';") and
update any usages of _.chain to call chain(...) so only the needed function is
bundled.
🪄 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: ebe84f9f-1c57-4009-8cb2-fdc952275d50
📒 Files selected for processing (1)
apps/api/src/routes/v1/workspaces/deployments.ts
| // 1 hour | ||
|
|
||
| import { validResourceSelector } from "../valid-selector.js"; | ||
| import { listDeploymentVariablesByDeploymentRouter } from "./deployment-variables.js"; | ||
|
|
||
| const parseSelector = ( | ||
| raw: string | null | undefined, | ||
| ): string | undefined => { | ||
| const PLAN_TTL_MS = 60 * 60 * 1000; |
There was a problem hiding this comment.
Comment is detached from the constant it describes.
The // 1 hour comment on line 16 appears before an import statement, making it unclear what it refers to. It should be directly above PLAN_TTL_MS.
📝 Suggested fix
-// 1 hour
-
import { validResourceSelector } from "../valid-selector.js";
import { listDeploymentVariablesByDeploymentRouter } from "./deployment-variables.js";
+// 1 hour
const PLAN_TTL_MS = 60 * 60 * 1000;🤖 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 16 - 21, The
inline comment "// 1 hour" is detached from the constant it documents; move that
comment so it sits directly above the PLAN_TTL_MS declaration to make the intent
clear. Locate the PLAN_TTL_MS constant and reposition the comment immediately
above it (remove or relocate the stray comment before the imports) so the
comment clearly documents PLAN_TTL_MS.
Summary by CodeRabbit