feat: show resolved value in dep variables view#570
Conversation
|
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThe changes refactor how deployment variable values and their associated resources are handled and displayed. Resource-related data now includes resolved values and is consolidated into enriched arrays, removing separate count and hash properties. API logic for fetching deployment variables is centralized in a new module, and URL generation for filtered resource lists is updated to use compressed selectors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Webservice
participant API
participant RuleEngine
participant DB
Client->>Webservice: Request deployment variables page
Webservice->>API: Fetch variables by deployment ID
API->>DB: Query deployment, variables, values, resources
API->>RuleEngine: Resolve reference variable values (if needed)
RuleEngine-->>API: Return resolved values
API-->>Webservice: Return variables with enriched resources and resolved values
Webservice-->>Client: Render variable table with resolved resource values
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/api/src/router/deployment-variable.ts (4)
275-275: Typo in variable name.There's a typo in the variable name
resolvedVarliablesPromises. It should beresolvedVariablesPromises.- const resolvedVarliablesPromises = deploymentVariables.map( + const resolvedVariablesPromises = deploymentVariables.map(
275-371: Consider breaking down the complex function.The function that processes deployment variables and resolves references is quite long and complex. Consider breaking it down into smaller, more focused functions for better maintainability and readability.
You could extract separate functions for:
- Processing non-default values
- Matching resources to variable values
- Resolving reference values
- Handling default values
This would make the code easier to understand, test, and maintain.
313-316: Duplicated code for resolving reference values.There's duplicated logic for resolving reference values in two different places. Consider extracting this into a helper function to avoid duplication.
You could create a helper function like:
const resolveReference = async (resourceId: string, value: any) => { const resolvedValue = await getReferenceVariableValue( resourceId, value ); return resolvedValue; };And then use this function in both places where you're resolving references.
Also applies to: 345-348
373-373: Update variable name in Promise.all.Remember to update the variable name here to match the fixed typo suggested earlier.
- return Promise.all(resolvedVarliablesPromises); + return Promise.all(resolvedVariablesPromises);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableTable.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/variable-data.ts(1 hunks)apps/webservice/src/app/urls.ts(2 hunks)packages/api/src/router/deployment-variable.ts(4 hunks)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsxpackages/rule-engine/src/manager/variables/resolve-reference-variable.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/variable-data.tsapps/webservice/src/app/urls.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableTable.tsxpackages/api/src/router/deployment-variable.ts
🧬 Code Graph Analysis (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsx (1)
packages/db/src/schema/deployment.ts (1)
deployment(69-93)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
packages/logger/src/index.ts (1)
logger(48-48)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/variable-data.ts (1)
packages/db/src/schema/resource.ts (1)
Resource(105-105)
packages/api/src/router/deployment-variable.ts (5)
packages/db/src/schema/release.ts (1)
releaseTarget(20-42)packages/db/src/schema/resource.ts (2)
resource(59-87)Resource(105-105)packages/db/src/schema/deployment-variables.ts (2)
deploymentVariable(28-50)isDeploymentVariableValueReference(148-152)packages/db/src/selectors/index.ts (1)
selector(13-13)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
getReferenceVariableValue(8-31)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (15)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (2)
23-26: Improved null handling and explicit typing for reference variable resolution.The changes improve the function by:
- Explicitly returning
nullwhen the target resource is not found (instead of potentially returningundefined)- Extracting the resolved path to a variable with null coalescing
- Adding a type assertion to clarify the return type
This makes the function's behavior more predictable and type-safe.
29-29: Consistent null handling in error cases.This change ensures consistent behavior by explicitly returning
nullwhen the default value is undefined, matching the same pattern used in the success path.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsx (1)
37-37: Simplified data fetching with improved API.The code has been significantly simplified by replacing complex client-side processing with a single API call. This improves maintainability by moving the data enrichment logic to the backend.
apps/webservice/src/app/urls.ts (2)
1-3: Added necessary imports for resource filtering.These imports support the new URL filtering functionality by providing the required types and compression library.
96-108: Added method to generate resource URLs with compressed filter conditions.This new
filteredmethod provides a clean way to generate URLs for filtered resource views. The implementation:
- Returns the standard list URL when no filter is provided
- Uses LZString compression to efficiently encode complex filter objects in URLs
- Properly handles the query parameter format
This is a good approach for maintaining state in URLs without creating excessively long URLs.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/variable-data.ts (1)
3-7: Enhanced resource type with resolved values.The
resourcesproperty has been enhanced to include aresolvedValueproperty for each resource. This change:
- Streamlines the data structure by removing separate
resourceCountandconditionHashproperties- Makes the resolved values of reference variables directly accessible in the UI
- Provides better type safety with explicit typing of the resolved value
This change properly supports the display of resolved reference values in the UI.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableTable.tsx (5)
53-59: Props now support resolved values for reference variables.The
ResourceRowcomponent now properly supports showing resolved values for deployment variables that reference resources, with the addedvalueTypeprop to correctly differentiate display behavior.
78-84: Good conditional rendering of resolved values.The conditional rendering of resolved values when
valueTypeis "reference" improves the user experience by showing the concrete values from referenced resources. The code correctly handles different data types, including proper JSON stringification for object values.
265-267: Updated resource count display.The code now correctly uses the length of the resources array instead of a
resourceCountproperty, aligning with the backend changes.
287-287: Proper propagation of value type.The
valueTypeis correctly passed to child components, ensuring proper rendering of different value types.
299-301: Updated resource filtering URL generation.The code now uses the new
filteredmethod to generate URLs for filtered resources, which aligns with the backend changes.packages/api/src/router/deployment-variable.ts (4)
1-30: Updated imports to support the new implementation.The imports have been updated to include necessary components for the new implementation, including
Resource,selector,isDeploymentVariableValueReference,releaseTarget, andgetReferenceVariableValue. These imports align with the new approach to fetching and processing deployment variables.
263-273: Improved data fetching approach.The implementation now uses a multi-step process that first fetches release targets with resources and then retrieves deployment variables with their values. This approach is more explicit and provides better support for resolving reference values.
310-322: Good implementation of reference value resolution.The code correctly identifies reference-type values and resolves them for each matched resource. The implementation properly uses the
getReferenceVariableValuefunction and enriches resources with their resolved values.
332-368: Proper handling of default values.The code correctly handles default values separately, applying them to resources that don't match any non-default values. This ensures that all resources have appropriate variable values.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/api/src/router/deployment-variable/by-deployment-id.ts (1)
106-108: Minor typo –resolvedVarliablesPromisesSmall nit: the variable is spelt
resolvedVarliablesPromises. Correcting improves readability and avoids future grepping mishaps.-const resolvedVarliablesPromises = … +const resolvedVariablesPromises = …
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
packages/api/src/router/deployment-variable/by-deployment-id.ts(1 hunks)packages/api/src/router/deployment-variable/deployment-variable.ts(2 hunks)packages/api/src/router/deployment.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/api/src/router/deployment.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/deployment-variable/deployment-variable.tspackages/api/src/router/deployment-variable/by-deployment-id.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
packages/api/src/router/deployment-variable/deployment-variable.ts (1)
28-30: Module split greatly improves cohesion – nice!Extracting the
byDeploymentIdprocedure into its own module removes ~200 LOC of mixed concerns from this router and makes the file much easier to reason about. 👍packages/api/src/router/deployment-variable/by-deployment-id.ts (3)
41-50:resolvedValuemay beundefined– check typing & null-coalescing
getReferenceVariableValuecan legitimately returnundefined(e.g. when the reference cannot be resolved).
ResolvedResource["resolvedValue"]excludesundefined, so TypeScript will silently widen toany.Consider either:
- Extending the union type to
undefined, or- Converting
undefinedtonullbefore assignment:-const resolvedValue = await getReferenceVariableValue(r.id, val); +const resolved = await getReferenceVariableValue(r.id, val); +const resolvedValue = resolved ?? null;This avoids accidental
anyleakage and keeps JSON serialisable payloads predictable.
61-74: Default value ignores its own selector
getDefaultValueWithRemainingResourcesassigns the default value to all resources that were not matched earlier, even if the default itself has a non-nullresourceSelector.If the API ever allows defaults with selectors (e.g. “default for databases”), those resources will be over-matched.
Please either:
• Assertval.resourceSelector === nullfor defaults, or
• Apply the selector filter in the same way as for non-default values.
106-146: Missing stable sort may change client orderingThe previous inline implementation returned variables ordered by
key ASC; the new code relies on the natural order offindMany, which is undefined without anorderBy.If any UI tests or screenshots assume alphabetical ordering, this may introduce subtle diffs.
Add an explicit
orderBy: asc(schema.deploymentVariable.key)when queryingdeploymentVariable.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes