refactor(platform): replace loose string validators with type-safe unions#558
Merged
Conversation
Greptile SummaryThis PR replaces loose string validators ( Key improvements:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| services/platform/lib/shared/schemas/wf_executions.ts | Added executionStatusSchema union type replacing loose string validators |
| services/platform/convex/lib/helpers/status_priority.ts | New shared constant to eliminate status priority duplication |
| services/platform/lib/permissions/get-default-settings-route.ts | New role-based settings route helper using ability checks |
| services/platform/convex/custom_agents/queries.ts | Fixed inverted status priority comparison (< changed to >) and uses shared STATUS_PRIORITY |
| services/platform/convex/workflows/schema.ts | Replaced v.string() with workflowStatusValidator and executionStatusValidator |
| services/platform/convex/members/queries.ts | Replaced local role constants with memberRoleSchema and memberRoleValidator |
| services/platform/app/routes/dashboard/$id/settings/index.tsx | Uses getDefaultSettingsRoute for role-based redirect instead of hardcoded path |
| services/platform/convex/workflows/executions/get_workflow_execution_stats.ts | Removed references to 'suspended' status (no longer a valid execution status) |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Loose String Validators] --> B{Refactor Type}
B --> C[Member Roles]
B --> D[Workflow Status]
B --> E[Execution Status]
B --> F[Role Restrictions]
C --> G[memberRoleSchema enum]
G --> H[memberRoleValidator convex]
H --> I[isMemberRole type guard]
I --> J[Member components & queries]
D --> K[workflowStatusSchema enum]
K --> L[workflowStatusValidator convex]
L --> M[STATUS_PRIORITY shared constant]
M --> N[Workflow queries & mutations]
E --> O[executionStatusSchema enum]
O --> P[executionStatusValidator convex]
P --> Q[Execution queries & stats]
Q --> R[Remove 'suspended' status]
F --> S[roleRestrictionValidator literal]
S --> T[Custom agent schema & mutations]
N --> U[Fix inverted priority comparison]
U --> V[Active > Draft > Archived]
Last reviewed commit: ece5445
📝 WalkthroughWalkthroughThis PR consolidates and strengthens type safety across role, status, and validation logic throughout the codebase. It introduces shared type guards (isMemberRole), exports previously internal types (WorkflowStatus, ExecutionStatus), centralizes validation into dedicated modules, replaces scattered local constants with shared helpers (DEFAULT_COUNT_CAP, STATUS_PRIORITY), narrows role restriction types to specific literals, and adds role-aware routing logic to dashboard settings. Changes maintain functional compatibility while improving type consistency and reducing duplication. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
services/platform/convex/approvals/queries.ts (1)
41-62: 🧹 Nitpick | 🔵 TrivialLGTM – but be aware of a pre-existing count sharing quirk.
The three
DEFAULT_COUNT_CAPusages are correct replacements. However, note that theresolvedbranch shares onecountvariable across both theapprovedandrejectedloops. If there are ≥ 20 approved records, the cap is hit before the rejected loop ever increments — effectively returning 20 while hiding the rejected count. This was true before this PR withAPPROVALS_COUNT_CAP, so it's not introduced here, but it's worth tracking separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/convex/approvals/queries.ts` around lines 41 - 62, The resolved branch currently reuses a single count variable while iterating approved then rejected approvals (queries on 'approvals' with index 'by_org_status'), so hitting DEFAULT_COUNT_CAP on approved records prevents counting rejected ones; change the logic to use separate counters (e.g., approvedCount and rejectedCount) each capped at DEFAULT_COUNT_CAP while iterating the approved and rejected queries, then combine them (sum or other intended aggregation) to produce the final count returned by the function.services/platform/convex/workflows/executions/types.ts (1)
81-89: 🛠️ Refactor suggestion | 🟠 Major
ListExecutionsArgs.statusandListExecutionsCursorArgs.statusstill use plainstring/string[].The corresponding Zod schemas in
wf_executions.tshave been narrowed toexecutionStatusSchema/z.array(executionStatusSchema), but these manual interfaces remainstring?/string[]?. For consistency with the PR's goal of replacing loose strings with type-safe unions, consider narrowing these as well:♻️ Suggested fix
export interface ListExecutionsArgs { wfDefinitionId: Doc<'wfDefinitions'>['_id']; - status?: string; + status?: ExecutionStatus; limit?: number; search?: string; triggeredBy?: string; dateFrom?: string; dateTo?: string; }export interface ListExecutionsCursorArgs { ... - status?: string[]; + status?: ExecutionStatus[]; ... }Also applies to: 95-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/convex/workflows/executions/types.ts` around lines 81 - 89, ListExecutionsArgs.status and ListExecutionsCursorArgs.status are still typed as plain string/string[]; update both to the narrowed execution status union used by the Zod schemas (e.g. use the exported ExecutionStatus type or the same union type backing executionStatusSchema and z.array(executionStatusSchema)) so the interfaces match the validated types; import the appropriate type from the module that defines executionStatusSchema and replace status?: string / status?: string[] with status?: ExecutionStatus / status?: ExecutionStatus[] in the ListExecutionsArgs and ListExecutionsCursorArgs interfaces.services/platform/convex/members/queries.ts (1)
179-193:⚠️ Potential issue | 🟠 MajorNormalize roles before returning to satisfy the validator.
WithmemberRoleValidatoron the response, any legacy/invalid role from BetterAuth will cause this query to throw. Align with the same fallback used elsewhere.✅ Proposed fix
- return orgs.map((o) => ({ - organizationId: o.organizationId, - role: o.role, - })); + return orgs.map((o) => { + const role: MemberRole = isValidRole(o.role) ? o.role : 'member'; + return { + organizationId: o.organizationId, + role, + }; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/convex/members/queries.ts` around lines 179 - 193, The handler returns org roles directly which can include legacy values that fail memberRoleValidator; normalize each o.role before returning by validating with memberRoleValidator (e.g., use memberRoleValidator.safeParse(o.role) and if invalid fall back to the canonical default role used elsewhere, e.g. 'member') so the returned objects from handler (in the same function using getAuthUserIdentity and getUserOrganizations) always satisfy memberRoleValidator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/routes/dashboard/`$id/settings/index.tsx:
- Around line 8-18: The redirect in beforeLoad uses
context.queryClient.getQueryData(...) which is synchronous and can return
undefined on cold loads, causing getDefaultSettingsRoute(null) -> account; fix
this by moving the member-context prefetch into a loader so the query is warmed
before beforeLoad runs: in the parent/dashboard or this route add a loader that
calls context.queryClient.ensureQueryData or triggers the
getCurrentMemberContext query (the same convexQuery key used in beforeLoad),
await its fetch so the cache contains memberContext, then keep beforeLoad using
getDefaultSettingsRoute(memberContext?.role ?? null) safely; reference
getCurrentMemberContext, context.queryClient.getQueryData, beforeLoad, and
getDefaultSettingsRoute to locate the changes.
In `@services/platform/convex/lib/helpers/status_priority.ts`:
- Around line 5-9: The current type Record<string, number> on STATUS_PRIORITY
falsely promises a number for every string key; replace the broad annotation
with a concrete, narrow type derived from the actual keys so unknown-key access
becomes a type error or shows undefined. Remove Record<string, number>, declare
the map as a const object (e.g. export const STATUS_PRIORITY = { active: 3,
draft: 2, archived: 1 } as const) and then derive a Status type via type Status
= keyof typeof STATUS_PRIORITY; if you need a writable numeric map use a derived
explicit type like const STATUS_PRIORITY: Record<Status, number> = { ... } so
the keyset is exhaustive and callers must cast or use STATUS_PRIORITY[status as
Status] when indexing by arbitrary strings.
In
`@services/platform/convex/workflows/definitions/list_workflows_with_best_version.ts`:
- Around line 64-65: STATUS_PRIORITY is declared as Record<string, number> which
allows missing WorkflowStatus keys to go unnoticed; change its type to
Record<WorkflowStatus, number> in the helper so the compiler enforces
exhaustiveness for all WorkflowStatus literals, update the STATUS_PRIORITY
declaration (and any related export) accordingly, and run the TypeScript build
to fix any missing entries by adding explicit priorities for new statuses; keep
usages like the lookups in list_workflows_with_best_version.ts (currentPriority
= STATUS_PRIORITY[workflow.status]) unchanged aside from satisfying the
tightened type.
In `@services/platform/lib/permissions/get-default-settings-route.ts`:
- Around line 3-6: The type alias SettingsRoute is currently local and should be
exported so callers can annotate variables; update the declaration of
SettingsRoute to "export type SettingsRoute = ..." (so consumers like code in
settings/index.tsx can use SettingsRoute directly) and ensure any related
references such as getDefaultSettingsRoute still compile against the exported
type.
In `@services/platform/lib/shared/schemas/role-guards.test.ts`:
- Around line 5-7: Replace the local duplicate of isMemberRole with the real
exported function: remove the local function definition and import isMemberRole
from the module that defines it (the production export referenced by
memberRoleSchema) so tests call the actual implementation; ensure tests
reference the imported isMemberRole and keep assertions the same (and keep
memberRoleSchema usage only if needed to construct test cases), then run tests
to verify behavior.
---
Outside diff comments:
In `@services/platform/convex/approvals/queries.ts`:
- Around line 41-62: The resolved branch currently reuses a single count
variable while iterating approved then rejected approvals (queries on
'approvals' with index 'by_org_status'), so hitting DEFAULT_COUNT_CAP on
approved records prevents counting rejected ones; change the logic to use
separate counters (e.g., approvedCount and rejectedCount) each capped at
DEFAULT_COUNT_CAP while iterating the approved and rejected queries, then
combine them (sum or other intended aggregation) to produce the final count
returned by the function.
In `@services/platform/convex/members/queries.ts`:
- Around line 179-193: The handler returns org roles directly which can include
legacy values that fail memberRoleValidator; normalize each o.role before
returning by validating with memberRoleValidator (e.g., use
memberRoleValidator.safeParse(o.role) and if invalid fall back to the canonical
default role used elsewhere, e.g. 'member') so the returned objects from handler
(in the same function using getAuthUserIdentity and getUserOrganizations) always
satisfy memberRoleValidator.
In `@services/platform/convex/workflows/executions/types.ts`:
- Around line 81-89: ListExecutionsArgs.status and
ListExecutionsCursorArgs.status are still typed as plain string/string[]; update
both to the narrowed execution status union used by the Zod schemas (e.g. use
the exported ExecutionStatus type or the same union type backing
executionStatusSchema and z.array(executionStatusSchema)) so the interfaces
match the validated types; import the appropriate type from the module that
defines executionStatusSchema and replace status?: string / status?: string[]
with status?: ExecutionStatus / status?: ExecutionStatus[] in the
ListExecutionsArgs and ListExecutionsCursorArgs interfaces.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
services/platform/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (42)
services/platform/app/features/settings/organization/components/member-delete-dialog.tsxservices/platform/app/features/settings/organization/components/member-edit-dialog.tsxservices/platform/app/features/settings/organization/utils/role-guards.tsservices/platform/app/routes/dashboard/$id/settings/index.tsxservices/platform/convex/agent_tools/delegation/create_delegation_tool.tsservices/platform/convex/agent_tools/workflows/helpers/read_all_workflows.tsservices/platform/convex/agent_tools/workflows/workflow_read_tool.tsservices/platform/convex/approvals/queries.tsservices/platform/convex/conversations/queries.tsservices/platform/convex/custom_agents/mutations.tsservices/platform/convex/custom_agents/queries.tsservices/platform/convex/custom_agents/schema.tsservices/platform/convex/custom_agents/system_defaults.tsservices/platform/convex/lib/helpers/count_items_in_org.tsservices/platform/convex/lib/helpers/status_priority.test.tsservices/platform/convex/lib/helpers/status_priority.tsservices/platform/convex/lib/rls/organization/get_user_organizations.tsservices/platform/convex/lib/rls/organization/validate_organization_access.tsservices/platform/convex/members/queries.tsservices/platform/convex/members/validators.tsservices/platform/convex/sso_providers/schema.tsservices/platform/convex/sso_providers/validators.tsservices/platform/convex/wf_definitions/internal_queries.tsservices/platform/convex/wf_definitions/queries.tsservices/platform/convex/wf_executions/internal_mutations.tsservices/platform/convex/workflows/definitions/get_automations_cursor.tsservices/platform/convex/workflows/definitions/list_automations_paginated.tsservices/platform/convex/workflows/definitions/list_workflows.tsservices/platform/convex/workflows/definitions/list_workflows_with_best_version.tsservices/platform/convex/workflows/definitions/update_workflow.tsservices/platform/convex/workflows/definitions/update_workflow_status.tsservices/platform/convex/workflows/definitions/validators.tsservices/platform/convex/workflows/executions/get_workflow_execution_stats.tsservices/platform/convex/workflows/executions/types.tsservices/platform/convex/workflows/executions/validators.tsservices/platform/convex/workflows/schema.tsservices/platform/lib/permissions/get-default-settings-route.test.tsservices/platform/lib/permissions/get-default-settings-route.tsservices/platform/lib/shared/schemas/members.tsservices/platform/lib/shared/schemas/role-guards.test.tsservices/platform/lib/shared/schemas/wf_definitions.tsservices/platform/lib/shared/schemas/wf_executions.ts
💤 Files with no reviewable changes (1)
- services/platform/convex/workflows/executions/get_workflow_execution_stats.ts
…ions Replace v.string() and z.string() with proper union validators for workflow status, member roles, custom agent role restrictions, and execution status. Introduces shared STATUS_PRIORITY constant and DEFAULT_COUNT_CAP export. Adds isMemberRole type guard and role-based settings route helper.
Use ensureQueryData in loader instead of getQueryData in beforeLoad for proper type inference in settings route. Remove unused ExecutionStatus type export. Import real isMemberRole in test instead of local duplicate.
Israeltheminer
force-pushed
the
refactor/type-safe-validators
branch
from
dd45f4f to
03157f2
Compare
yannickmonney
pushed a commit
that referenced
this pull request
Apr 8, 2026
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
1 participant
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.
Summary
v.string()andz.string()with proper union validators for workflow status, execution status, member roles, and custom agent role restrictionsSTATUS_PRIORITYconstant and exportDEFAULT_COUNT_CAPto eliminate duplication across queriesisMemberRoletype guard and role-based settings route helper (getDefaultSettingsRoute)Test plan
status_priority.test.ts,role-guards.test.ts,get-default-settings-route.test.tsSummary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes