fix: Reference Variable setting and getting in the UI #540
Conversation
WalkthroughThe changes introduce clear separation between direct and reference resource variables across the codebase. They add explicit handling and schema validation for both types, update API mutations, refactor UI components to display and manage them distinctly, and extend schema support for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
participant DB
User->>UI: Submit Direct Variable Form
UI->>API: create({ key, value, sensitive, resourceId, valueType: "direct" })
API->>DB: Insert direct variable (reference/path null)
DB-->>API: Variable record
API-->>UI: Return created variable
User->>UI: Submit Reference Variable Form
UI->>API: createReference({ key, reference, path, defaultValue, sensitive, resourceId, valueType: "reference" })
API->>DB: Insert reference variable (value null, with defaultValue)
DB-->>API: Variable record
API-->>UI: Return created reference variable
sequenceDiagram
participant UI
participant API
participant DB
UI->>API: Fetch Resource by ID
API->>DB: Query resource, variables, relationships, metadata
DB-->>API: Resource data
API->>API: Validate and split variables (direct/reference)
API->>API: Resolve reference variables (use defaultValue if unresolved)
API-->>UI: Return resource with directVariables and referenceVariables arrays
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/ReferenceVariableForm.tsx (1)
67-72: InitialdefaultValueshould beundefined, not empty stringUsing an empty string sentinel later (
defaultValue === "" ? undefined) is brittle and will briefly violate the schema (which forbids empty strings). Keeping the form state asundefinedavoids the workaround altogether.- defaultValue: undefined, + defaultValue: undefined, // keep it truly undefined from the startapps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/ResourceDrawer.tsx (1)
36-46: Utility could be strongly typed & side-effect free
vars: any[]discards compile-time safety. AcceptResourceVariable[](or whatever the actual union type is) to surface future breakage during refactors.-const getDirectVariables = (vars: any[]): DirectResourceVariable[] => +const getDirectVariables = ( + vars: Array<DirectResourceVariable | ReferenceResourceVariable>, +): DirectResourceVariable[] =>Also consider leaving objects as objects and letting the renderer stringify; serialising here permanently loses type information.
packages/api/src/router/resources.ts (1)
417-452: Comprehensive reference variable resolution logic.The implementation properly handles all edge cases for reference variables:
- Validates relationship existence
- Checks for target resource existence
- Handles empty paths by returning the entire target resource
- Extracts values from metadata or falls back to the provided default value
Consider adding error logging when relationships or target resources aren't found, to help debug issues with reference variables:
if (!relationshipInfo) + console.debug(`Reference variable ${v.key}: relationship ${v.reference} not found`); return { ...v, resolvedValue: v.defaultValue }; if (!targetResource) + console.debug(`Reference variable ${v.key}: target resource for ${relationshipInfo.target.id} not found`); return { ...v, resolvedValue: v.defaultValue };packages/api/src/router/resource-variables.ts (1)
52-86: New mutation for reference variables.The
createReferencemutation is well-implemented with:
- Proper input validation requiring reference-specific fields
- Setting the value to
nullas expected for reference variables- Handling the optional defaultValue correctly
- Same authorization checks as the direct variable mutation
- Enqueueing the created variable for background processing
Consider refactoring the common post-creation logic (parsing and enqueueing) into a shared helper function to avoid duplication across the create mutations:
// Add this helper function at the top of the file const processCreatedVariable = async (variable: unknown) => { const parsedVariable = schema.resourceVariableSchema.parse(variable); await updateResourceVariableQueue.add(parsedVariable.id, parsedVariable); return parsedVariable; }; // Then use it in both mutations const variable = await ctx.db .insert(schema.resourceVariable) .values(data) .returning() .then(takeFirst); return processCreatedVariable(variable);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/CreateResourceVariableDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/ResourceDrawer.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/VariablesContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/DirectVariableForm.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/ReferenceVariableForm.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx(4 hunks)apps/webservice/src/app/api/v1/resource-relationship-rules/route.ts(1 hunks)packages/api/src/router/resource-variables.ts(1 hunks)packages/api/src/router/resources.ts(2 hunks)packages/db/src/schema/resource.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)/_components/resources/drawer/VariablesContent.tsxapps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/CreateResourceVariableDialog.tsxapps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/ResourceDrawer.tsxapps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/DirectVariableForm.tsxapps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/ReferenceVariableForm.tsxpackages/db/src/schema/resource.tspackages/api/src/router/resources.tsapps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsxapps/webservice/src/app/api/v1/resource-relationship-rules/route.tspackages/api/src/router/resource-variables.ts
🧬 Code Graph Analysis (5)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/VariablesContent.tsx (1)
packages/db/src/schema/resource.ts (1)
DirectResourceVariable(513-513)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/ResourceDrawer.tsx (1)
packages/db/src/schema/resource.ts (2)
DirectResourceVariable(513-513)resource(59-87)
apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/ReferenceVariableForm.tsx (1)
packages/db/src/schema/resource.ts (1)
createResourceVariable(433-458)
packages/api/src/router/resources.ts (1)
packages/db/src/schema/resource.ts (2)
DirectResourceVariable(513-513)ReferenceResourceVariable(514-514)
packages/api/src/router/resource-variables.ts (2)
packages/api/src/trpc.ts (1)
protectedProcedure(173-173)packages/db/src/common.ts (1)
takeFirst(9-13)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (16)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/CreateResourceVariableDialog.tsx (1)
68-68: Added proper valueType to resource variable creationThis change explicitly specifies that the created variable is a "direct" type, which aligns with the broader refactoring that distinguishes between direct and reference resource variables in the system.
apps/webservice/src/app/api/v1/resource-relationship-rules/route.ts (1)
35-35: Adjusted permission level from SystemUpdate to WorkspaceUpdateThis change reduces the required permission level from a system-wide update permission to a workspace-specific update permission, making the API more accessible to workspace administrators who don't need system-wide privileges.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/VariablesContent.tsx (1)
121-121: Updated resourceVariables prop type for better type safetyThe type change from
ResourceVariable[]toDirectResourceVariable[]ensures this component explicitly works with direct variables only, aligning with the broader refactoring to separate direct and reference variable handling throughout the application.apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/DirectVariableForm.tsx (1)
69-75: Improved mutation payload constructionChanged from spreading the entire form data to explicitly specifying each field in the mutation payload, and added the necessary
valueType: "direct"parameter. This makes the API contract more explicit and aligns with the type separation refactoring.apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/ReferenceVariableForm.tsx (1)
60-61: Mutation renamed – check optimistic updates / cachesSwapping to
createReferenceis correct, but the surrounding cache-invalidation still targetsutils.resource.byId. If other screens rely onreferenceVariableslists, verify those query keys as well to avoid stale UI after creation.apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/ResourceDrawer.tsx (1)
229-230: Potentially empty array – guard against undefined
resource.variablesis nullable while the query is loading;getDirectVariablesreceivesundefinedduring the first render.
Either default to[]or add anenabled: !!resourceIdcheck before mapping.apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx (1)
31-35:rulesmay be undefined on older resources
const { directVariables, referenceVariables, rules } = resource;
Ifruleswas introduced in this PR, dereferencing on a pre-migration record will throw. Guard with?.or default to[].-const references = rules.map((r) => r.reference); +const references = (rules ?? []).map((r) => r.reference);packages/api/src/router/resources.ts (4)
397-397: Improved variable handling with target resolution.The code now correctly handles fetching relationship targets with their metadata, which is essential for the new reference variable resolution.
399-412: Keep existing variable parsing logic.This parsing logic correctly handles malformed variables by attempting to parse them through the schema validation and logging errors for any that fail.
413-415: Good separation of direct variables.The code appropriately filters direct variables using type guards to ensure type safety. This separation is crucial for the new distinction between variable types.
460-462: Return separated variable arrays.This change properly structures the API response to include both direct and reference variables as separate arrays, maintaining a clear distinction between the two types.
packages/api/src/router/resource-variables.ts (5)
14-15: Clear comment for direct variables section.Good documentation to indicate that this mutation is specifically for direct variables.
16-24: Properly constrained input schema for direct variables.The input schema now correctly includes the
valueType: "direct"literal type, ensuring that this mutation can only be used for direct variables.
32-35: Encryption handling for sensitive values.The encryption logic correctly handles sensitive values, converting them to strings before encryption.
36-50: Explicit null handling for reference fields.The code properly sets
referenceandpathtonullfor direct variables, ensuring a clear distinction between variable types in the database.
71-76: Clean handling of null default values.The code correctly handles the case where
defaultValuemight be null or undefined, converting it to undefined for the database insertion.
| defaultValue: z | ||
| .union([z.string(), z.number(), z.boolean(), z.null()]) | ||
| .optional(), |
There was a problem hiding this comment.
defaultValue union now inconsistent with earlier schemas
createResourceVariable (lines 433-441) still accepts z.record(z.any()) for defaultValue, but the new referenceVariableSchema omits that possibility and only allows primitives/null.
Persisting an object in default_value will therefore validate on insert/update but fail on subsequent fetch / validation against this discriminated union.
- defaultValue: z
- .union([z.string(), z.number(), z.boolean(), z.null()])
- .optional(),
+ defaultValue: z
+ .union([
+ z.string(),
+ z.number(),
+ z.boolean(),
+ z.record(z.any()),
+ z.null(),
+ ])
+ .optional(),Add the object arm (or, if objects are intentionally disallowed for references, tighten the earlier schemas instead) to keep the type-system and runtime validation in sync.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defaultValue: z | |
| .union([z.string(), z.number(), z.boolean(), z.null()]) | |
| .optional(), | |
| defaultValue: z | |
| .union([ | |
| z.string(), | |
| z.number(), | |
| z.boolean(), | |
| z.record(z.any()), | |
| z.null(), | |
| ]) | |
| .optional(), |
| <FormField | ||
| control={form.control} | ||
| name="defaultValue" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Default Value (Optional)</FormLabel> | ||
| <FormControl> | ||
| <Input | ||
| {...field} | ||
| value={field.value === undefined ? "" : String(field.value)} | ||
| onChange={(e) => { | ||
| const value = e.target.value.trim(); | ||
| field.onChange(value === "" ? undefined : value); | ||
| }} | ||
| placeholder="Fallback if reference is not available" | ||
| /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
String-only input silently drops numbers & booleans
defaultValue may be a number or boolean, yet the input always serialises to a string, causing every entry to arrive as "123" or "true".
Parse user input (e.g. JSON.parse) or provide a type selector so the backend receives correctly-typed data.
- const value = e.target.value.trim();
- field.onChange(value === "" ? undefined : value);
+ const raw = e.target.value.trim();
+ if (raw === "") {
+ field.onChange(undefined);
+ } else {
+ // attempt JSON parse – falls back to string on error
+ try {
+ field.onChange(JSON.parse(raw));
+ } catch {
+ field.onChange(raw);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <FormField | |
| control={form.control} | |
| name="defaultValue" | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormLabel>Default Value (Optional)</FormLabel> | |
| <FormControl> | |
| <Input | |
| {...field} | |
| value={field.value === undefined ? "" : String(field.value)} | |
| onChange={(e) => { | |
| const value = e.target.value.trim(); | |
| field.onChange(value === "" ? undefined : value); | |
| }} | |
| placeholder="Fallback if reference is not available" | |
| /> | |
| </FormControl> | |
| <FormMessage /> | |
| </FormItem> | |
| )} | |
| /> | |
| <FormField | |
| control={form.control} | |
| name="defaultValue" | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormLabel>Default Value (Optional)</FormLabel> | |
| <FormControl> | |
| <Input | |
| {...field} | |
| value={field.value === undefined ? "" : String(field.value)} | |
| onChange={(e) => { | |
| const raw = e.target.value.trim(); | |
| if (raw === "") { | |
| field.onChange(undefined); | |
| } else { | |
| // attempt JSON parse – falls back to string on error | |
| try { | |
| field.onChange(JSON.parse(raw)); | |
| } catch { | |
| field.onChange(raw); | |
| } | |
| } | |
| }} | |
| placeholder="Fallback if reference is not available" | |
| /> | |
| </FormControl> | |
| <FormMessage /> | |
| </FormItem> | |
| )} | |
| /> |
apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx
Outdated
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/ResourceDrawer.tsx
Outdated
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/ResourceDrawer.tsx
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Improvements
Bug Fixes