Repository navigation
feat: workflow automation with 6 critical bug fixes - #710
Merged
Merged
Conversation
Extend the workflow system beyond cron schedules to support event-triggered execution. Workflows can now fire when activity happens in a drive — e.g., file uploads, page creation, member additions — connecting the existing activity logging system to the workflow executor via a new hook pattern. - Schema: add triggerType enum (cron/event), eventTriggers jsonb, watchedFolderIds, eventDebounceSecs columns; make cronExpression nullable - Event engine: emitWorkflowEvent() with matching, folder scoping, in-memory debounce, context injection, and recursive trigger prevention - Activity logger: add workflowTriggerHook (fire-and-forget) alongside existing broadcastHook in both logActivity() and logActivityWithTx() - API routes: update Zod schemas, add refine validation, filter cron endpoint to exclude event workflows - Frontend: trigger type toggle, event type checkboxes, debounce input, trigger type indicators (Clock/Zap icons) in workflow list - Calendar: event-triggered workflow runs appear as historical point-in-time events (amber) vs cron schedules as future occurrences (purple) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ce re-validation, cross-drive leak P1 fixes: - Defer workflow triggers until after transaction commit (activity-logger, page-mutation-service, page-service, restore route, tasks route) - Re-validate workflow state after debounce window before execution (event-trigger) - Restrict context page lookup to workflow's drive to prevent cross-drive data leak (workflow-executor) P2 fixes: - Add triggerType guard on nextRunAt computation for manual runs (run route) - Add timezone validation on POST/PATCH before getNextRunDate (workflows routes, cron-utils) - Fire deferred triggers from external tx callers (restore route, tasks route) 83 tests passing across 7 test files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a workflows subsystem: DB schema and migrations, cron and event-trigger runtimes (debounced), execution engine, cron scheduler endpoint and job, management APIs and UI, deferred post-transaction triggers, cron utilities, and extensive tests across runtime, APIs, and UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Cron Service
participant API as /api/cron/workflows
participant DB as Database
participant Exec as Workflow Executor
participant AI as AI Provider
Cron->>API: POST /api/cron/workflows
API->>DB: mark stuck cron runs (set error, compute nextRunAt)
API->>DB: select due cron workflows (isEnabled & nextRunAt <= now)
DB-->>API: due workflows
API->>DB: atomically claim batch (set running, lastRunAt) -> claimed rows
par concurrent executions
API->>Exec: executeWorkflow(workflow)
Exec->>DB: load agent/drive/context
Exec->>AI: generateText / tool calls
AI-->>Exec: response
Exec->>DB: persist messages & usage
Exec-->>API: result
API->>DB: update workflow (status, lastRun*, nextRunAt)
end
API-->>Cron: response summary (executed, total, errors?)
sequenceDiagram
participant App as Application Tx
participant Activity as Activity Logger
participant Hook as Workflow Hook
participant EventSys as Event Trigger System
participant DB as Database
participant Exec as Workflow Executor
App->>Activity: logActivityWithTx(change)
Activity->>Hook: register/fire -> returns DeferredWorkflowTrigger
Activity-->>App: deferredTrigger
App->>App: commit transaction
App->>Hook: call deferredTrigger()
Hook->>EventSys: emitWorkflowEvent(event)
EventSys->>DB: query enabled event workflows for drive
DB-->>EventSys: workflows list
EventSys->>EventSys: match triggers, check folders, store/debounce pending context
Note right of EventSys: debounce window elapses per workflow
EventSys->>DB: re-validate & claim workflow (set running)
EventSys->>Exec: executeWorkflow(with injected event context)
Exec->>DB: update workflow last run metrics
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 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)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e033d5b6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const [updated] = await db | ||
| .update(workflows) | ||
| .set({ | ||
| ...data, |
There was a problem hiding this comment.
The PATCH handler accepts cronExpression: null, validates cron workflows using data.cronExpression ?? workflow.cronExpression, and then writes ...data directly, so a request can keep triggerType: 'cron' while persisting cronExpression = null. That leaves invalid cron workflows in the database, and later cron/manual execution paths call getNextRunDate(workflow.cronExpression!, ...), which can throw and leave runs stuck in running. Reject null for effective cron workflows (or normalize it before .set).
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch. Fixed in 7ffb9df:
[workflowId]/route.ts:115-119: Changeddata.cronExpression ?? workflow.cronExpressiontodata.cronExpression !== undefined ? data.cronExpression : workflow.cronExpression. This correctly treats an explicitnullpayload as "clear it" (which then fails the!cronExprcheck), whileundefined(field not sent) falls through to the existing value.- Added test: PATCH with
{ cronExpression: null }on a cron workflow now returns 400.
| .from(workflows) | ||
| .where(eq(workflows.id, workflow.id)); | ||
|
|
||
| if (!current || !current.isEnabled) return; |
There was a problem hiding this comment.
After the debounce window, the revalidation only checks existence and isEnabled, so a second event can schedule and execute while the first event-triggered run is still in progress. In that scenario, executeEventWorkflow starts another run anyway, causing overlapping executions and racing status updates/side effects; this is inconsistent with manual and cron paths that explicitly block when status is running. Add a running-state guard (or atomic compare-and-set) before launching.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Agreed — overlapping executions could cause racing status updates. Fixed in 7ffb9df:
event-trigger.ts:124: Addedif (current.lastRunStatus === 'running') return;after the re-validation check. This skips execution if another run is already in progress, consistent with how cron and manual paths handle this.- Added test: workflow with
lastRunStatus: 'running'during debounce re-fetch is not executed.
| { label: 'Page created', description: 'When a new page is created', operation: 'create', resourceType: 'page' }, | ||
| { label: 'File uploaded', description: 'When a file is uploaded', operation: 'upload', resourceType: 'file' }, | ||
| { label: 'Page moved', description: 'When a page is moved to a folder', operation: 'move', resourceType: 'page' }, | ||
| { label: 'Agent created', description: 'When a new AI agent is created', operation: 'create', resourceType: 'agent' }, |
There was a problem hiding this comment.
This preset uses operation: 'create' with resourceType: 'agent', but create flows emit page-creation activity as resourceType: 'page' and agent-specific activity uses operation: 'agent_config_update'. As a result, selecting “Agent created” creates workflows that never match any emitted event and silently never run. The preset should match an actually emitted tuple (or be removed until one exists).
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch — the create + agent tuple is never emitted by the activity logger. Removed in 7ffb9df:
WorkflowForm.tsx:67: Removed the "Agent created" preset entirely. If we add agent-specific activity events in the future, we can re-add it with the correct operation/resourceType tuple.
… dead preset, lint - Add running-state guard before event workflow execution (prevents overlapping runs) - Reject null cronExpression on cron workflows in PATCH handler - Remove unreachable "Agent created" trigger preset (no matching event emitted) - Fix unused variable lint error (andCalls in workflow-executor test) 85 tests passing across 7 test files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/lib/src/monitoring/activity-logger.ts (1)
596-657:⚠️ Potential issue | 🔴 CriticalDeferred trigger pattern is correct, but
logRollbackActivityhas a critical bug: rollback workflow triggers are lost when using transactions.The return type change from
Promise<void>toPromise<DeferredWorkflowTrigger | undefined>is backward-compatible and properly implemented inpage-mutation-service.tsandpage-service.ts, which capture and fire the deferred triggers after their transactions commit.However,
logRollbackActivity(line 1184) callslogActivityWithTxwith the transaction but discards the returned trigger. SincelogRollbackActivityreturnsPromise<void>and has no mechanism to return the trigger to its callers, the deferred trigger is lost entirely. The caller inrollback-service.ts(line 1785) passestxvialogOptionsbut cannot access the trigger.This means rollback operations executed within a transaction will never fire their workflow triggers—a functional bug that breaks the audit trail for this critical operation.
logRollbackActivityshould either:
- Return
Promise<DeferredWorkflowTrigger | undefined>to match the pattern used by other activity logging, or- Omit the
txoption if fire-and-forget is the intended behavior🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/monitoring/activity-logger.ts` around lines 596 - 657, logRollbackActivity currently discards the DeferredWorkflowTrigger returned by logActivityWithTx causing rollback workflow triggers to be lost when called with a transaction; update logRollbackActivity to return Promise<DeferredWorkflowTrigger | undefined> (or remove the tx option if you intend fire-and-forget) and propagate the trigger returned from logActivityWithTx so callers (e.g., rollback-service) can capture and fire it after the tx commits; adjust the function signature of logRollbackActivity and its call sites to accept and forward the DeferredWorkflowTrigger similarly to how page-mutation-service.ts and page-service.ts handle triggers.apps/web/src/app/api/calendar/events/route.ts (1)
319-321:⚠️ Potential issue | 🟡 MinorEarly return omits
workflowEventskey — inconsistent response shape.Line 320 returns
{ events: [] }without aworkflowEventskey, while the normal paths at Lines 274 and 354 return{ events, workflowEvents }. Consumers need to handle the missing key (e.g.,response.workflowEvents ?? []), or this could cause issues if the frontend expects a consistent shape.Proposed fix
- return NextResponse.json({ events: [] }); + return NextResponse.json({ events: [], workflowEvents: [] });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/calendar/events/route.ts` around lines 319 - 321, The early return when conditions.length === 0 returns NextResponse.json({ events: [] }) but omits the workflowEvents key, causing an inconsistent response shape; update that return to include workflowEvents (e.g., return NextResponse.json({ events: [], workflowEvents: [] })) so it matches the other paths that return { events, workflowEvents } — change the NextResponse.json call in the route handler where conditions is checked to include the workflowEvents key.
🧹 Nitpick comments (16)
apps/web/src/lib/workflows/cron-utils.ts (1)
21-28: Consider usingnew Intl.DateTimeFormat(...)for clarity.While calling
Intl.DateTimeFormat()withoutnewis valid per the ECMAScript spec, the more idiomatic form isnew Intl.DateTimeFormat(undefined, { timeZone: timezone }). This is a minor style nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/cron-utils.ts` around lines 21 - 28, The validateTimezone function uses Intl.DateTimeFormat as a call; change it to the idiomatic constructor form by using new Intl.DateTimeFormat(undefined, { timeZone: timezone }) inside validateTimezone so the try/catch remains but the instance is created via new for clarity and style.apps/web/src/components/workflows/WorkflowStatusBadge.tsx (1)
5-17: Type the status prop as a union to remove the cast.
This keeps the component self-documenting and prevents invalid status values at compile time.As per coding guidelines: Write code that is explicit over implicit and self-documenting.♻️ Suggested refactor
const STATUS_CONFIG = { never_run: { label: 'Never run', variant: 'secondary' as const }, success: { label: 'Success', variant: 'default' as const }, error: { label: 'Error', variant: 'destructive' as const }, running: { label: 'Running', variant: 'outline' as const }, } as const; -export function WorkflowStatusBadge({ status }: { status: string }) { - const config = STATUS_CONFIG[status as keyof typeof STATUS_CONFIG] ?? STATUS_CONFIG.never_run; +type WorkflowStatus = keyof typeof STATUS_CONFIG; + +export function WorkflowStatusBadge({ status }: { status: WorkflowStatus }) { + const config = STATUS_CONFIG[status] ?? STATUS_CONFIG.never_run;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/workflows/WorkflowStatusBadge.tsx` around lines 5 - 17, Change the status prop on WorkflowStatusBadge to a string literal union matching the keys of STATUS_CONFIG (e.g. 'never_run' | 'success' | 'error' | 'running') instead of string so the component type-safely indexes STATUS_CONFIG without a cast; update the function signature for WorkflowStatusBadge to use that union, remove the status as keyof typeof STATUS_CONFIG cast when computing config, and ensure any callers pass one of the union values (or adjust callers) so invalid statuses are caught at compile time.apps/web/src/app/api/pages/[pageId]/tasks/[taskId]/route.ts (1)
195-288: Guard deferred trigger failures to avoid 500s after commit.
IfdeferredTriggerthrows, the task update has already committed but the request will fail; consider catching and logging.🛡️ Suggested guard
- deferredTrigger?.(); + try { + deferredTrigger?.(); + } catch (error) { + console.error('[WorkflowTrigger] deferred trigger failed', error); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/pages/`[pageId]/tasks/[taskId]/route.ts around lines 195 - 288, The deferredTrigger call after the db.transaction is unguarded and can throw after the DB commit; wrap the invocation of deferredTrigger (the deferredTrigger?.() call immediately after awaiting db.transaction) in a try/catch, log the error with context (e.g., taskId, pageId, userId, and that it occurred in deferredTrigger) and do not rethrow so the HTTP response does not turn into a 500; ensure the catch only handles errors from deferredTrigger and does not alter the successful transaction result from db.transaction.apps/web/src/app/api/workflows/[workflowId]/run/__tests__/route.test.ts (1)
241-255: Missing test for thrown exception during execution.This test covers
executeWorkflowreturning{ success: false, error: '...' }, but there's no test for whenexecuteWorkflowrejects with a thrownError. This matters because the route handler currently lacks atry/catcharound execution (flagged separately on the route file) — once that's fixed, you'll want a test asserting the workflow status is reset to'error'and a proper response is returned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/workflows/`[workflowId]/run/__tests__/route.test.ts around lines 241 - 255, Add a test in route.test.ts that mocks executeWorkflow to reject with a thrown Error (e.g., vi.mocked(executeWorkflow).mockRejectedValue(new Error('Agent crashed'))) then call the POST handler (POST with createContext('wf_1')) and assert the response indicates failure (response.json().success === false and response.json().error contains the thrown error message) and assert the workflow status was reset to 'error' (verify whatever persistence/update call your route uses to set status back to 'error' was invoked with 'error'); reference executeWorkflow, POST, and createContext when locating where to add the new test.apps/web/src/app/dashboard/[driveId]/workflows/page.tsx (1)
22-33: Duplicated skeleton markup.The loading skeleton JSX at lines 24–33 is identical to the Suspense fallback at lines 51–59. Consider extracting it into a small component (e.g.,
WorkflowsSkeleton) to keep things DRY.♻️ Proposed refactor
+function WorkflowsSkeleton() { + return ( + <div className="h-full overflow-y-auto"> + <div className="container mx-auto px-4 py-10 sm:px-6 lg:px-10 max-w-5xl"> + <div className="space-y-6"> + <Skeleton className="h-8 w-48" /> + <Skeleton className="h-10 w-full" /> + <Skeleton className="h-96" /> + </div> + </div> + </div> + ); +} + function WorkflowsContent() { // ... if (isLoading) { - return ( - <div className="h-full overflow-y-auto"> - <div className="container mx-auto px-4 py-10 sm:px-6 lg:px-10 max-w-5xl"> - <div className="space-y-6"> - <Skeleton className="h-8 w-48" /> - <Skeleton className="h-10 w-full" /> - <Skeleton className="h-96" /> - </div> - </div> - </div> - ); + return <WorkflowsSkeleton />; } // ... } export default function WorkflowsPage() { return ( - <Suspense - fallback={ - <div className="h-full overflow-y-auto"> - <div className="container mx-auto px-4 py-10 sm:px-6 lg:px-10 max-w-5xl"> - <div className="space-y-6"> - <Skeleton className="h-8 w-48" /> - <Skeleton className="h-10 w-full" /> - <Skeleton className="h-96" /> - </div> - </div> - </div> - } - > + <Suspense fallback={<WorkflowsSkeleton />}> <WorkflowsContent /> </Suspense> ); }Also applies to: 49-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/dashboard/`[driveId]/workflows/page.tsx around lines 22 - 33, The duplicated loading skeleton markup should be extracted into a small reusable component (e.g., WorkflowsSkeleton) so both the isLoading branch and the Suspense fallback use the same JSX; create a WorkflowsSkeleton component (defined in this file or imported) that returns the three Skeleton elements and replace the inline blocks in the isLoading return and the Suspense fallback with <WorkflowsSkeleton /> to remove duplication and keep behavior identical.apps/web/src/app/api/cron/workflows/route.ts (3)
9-9: Constant should useUPPER_SNAKE_CASEper coding guidelines.
maxConcurrentWorkflowsis a module-level constant and should follow the project's naming convention.Proposed fix
-const maxConcurrentWorkflows = 5; +const MAX_CONCURRENT_WORKFLOWS = 5;Update the reference on Line 79 accordingly.
As per coding guidelines: "Use UPPER_SNAKE_CASE for constants"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/cron/workflows/route.ts` at line 9, Rename the module-level constant maxConcurrentWorkflows to follow UPPER_SNAKE_CASE (MAX_CONCURRENT_WORKFLOWS) and update all usages to the new name (e.g., where it is referenced in the workflows route handler). Ensure the declaration (const MAX_CONCURRENT_WORKFLOWS = 5) and every reference (previously maxConcurrentWorkflows) are updated consistently.
49-56: TOCTOU gap between SELECT and marking as running.Between Lines 31–41 (selecting due workflows) and Lines 50–55 (marking them as running), a concurrent cron invocation could select the same workflows. While cron jobs are typically serialized, consider using an atomic approach (e.g.,
UPDATE ... WHERE lastRunStatus != 'running' RETURNING *) to atomically claim workflows for execution. This would also eliminate the separate SELECT + UPDATE round-trips.This isn't critical given typical cron invocation patterns, but worth considering for robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/cron/workflows/route.ts` around lines 49 - 56, The current two-step SELECT-then-UPDATE (selecting dueWorkflows then calling db.update(workflows).set(...).where(eq(workflows.id, wf.id))) has a TOCTOU race; replace it with a single atomic claim that updates and returns rows so only non-running workflows are claimed. Change the logic that builds dueWorkflows to perform an UPDATE on workflows with a WHERE that filters due criteria AND lastRunStatus != 'running', set lastRunStatus='running' and lastRunAt=new Date(), and use RETURNING (or the ORM's returning() equivalent) to get the claimed workflows for execution; reference the existing symbols lastRunStatus, lastRunAt, workflows, db.update(workflows) and use the returned rows instead of the separate dueWorkflows select and subsequent per-id updates.
59-75: Non-null assertion oncronExpressionrelies on data invariant.
workflow.cronExpression!on Lines 61 and 107 assumes cron workflows always have a non-nullcronExpression. While the query filters fortriggerType === 'cron', the DB column is nullable, so inconsistent data could cause a runtime error ingetNextRunDate. A defensive guard (skip + log warning for null expressions) would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/cron/workflows/route.ts` around lines 59 - 75, The code assumes workflow.cronExpression is non-null in executeOne when calling getNextRunDate; add a defensive check for workflow.cronExpression before calling getNextRunDate and handle the null case by logging a warning (include workflow.id) and setting nextRunAt to null (and optionally set lastRunError/lastRunStatus to indicate the invalid cron) before performing the db.update(workflows).set(...) so a malformed DB row doesn't throw at runtime; update references are in executeOne and the call to getNextRunDate.apps/web/src/components/workflows/WorkflowForm.tsx (2)
295-300: Raw<input type="checkbox">instead of shadcn/uiCheckboxcomponent.The rest of the form consistently uses shadcn/ui primitives (
Switch,Select,Input,Button). Consider using the shadcn/uiCheckboxcomponent here for visual and behavioral consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/workflows/WorkflowForm.tsx` around lines 295 - 300, Replace the raw <input type="checkbox"> with the shadcn/ui Checkbox component to match existing primitives; use the Checkbox's checked and onCheckedChange props to read isEventTriggerSelected(preset.operation, preset.resourceType) and call toggleEventTrigger(preset.operation, preset.resourceType) (wrap the call in an arrow to accept the boolean param if needed), and add the same styling/className used elsewhere; also add an import for Checkbox from the shadcn/ui package and ensure types line up (convert onCheckedChange value to boolean if required).
95-101: ConsideruseMemoinstead ofuseEffect+setStatefor cron preview.
getHumanReadableCronis synchronous (cronstrue-based), so deriving it as a memo avoids the extra render cycle and the intermediate empty state.Proposed refactor
- const [cronPreview, setCronPreview] = useState(''); ... - useEffect(() => { - if (!cronExpression) { - setCronPreview(''); - return; - } - setCronPreview(getHumanReadableCron(cronExpression)); - }, [cronExpression]); + const cronPreview = useMemo(() => { + if (!cronExpression) return ''; + return getHumanReadableCron(cronExpression); + }, [cronExpression]);Add
useMemoto the React import on Line 3.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/workflows/WorkflowForm.tsx` around lines 95 - 101, The current useEffect that computes cron preview causes an extra render by setting state from a synchronous function; replace this pattern by deriving the preview with useMemo: remove the useEffect that references cronExpression and setCronPreview, import useMemo, and create a const cronPreview = useMemo(() => cronExpression ? getHumanReadableCron(cronExpression) : '', [cronExpression]); then use cronPreview wherever state was used (and remove setCronPreview and related state initialization).apps/web/src/app/api/calendar/events/route.ts (1)
106-110: Extract the hardcoded 5-minute virtual event duration into a named constant.The magic number
5 * 60 * 1000appears on both Lines 110 and 137 for different trigger types. A named constant improves readability and makes it easier to adjust.Proposed refactor
+const VIRTUAL_EVENT_DURATION_MS = 5 * 60 * 1000; // 5 minutes ... - endAt: new Date(wf.lastRunAt.getTime() + 5 * 60 * 1000), + endAt: new Date(wf.lastRunAt.getTime() + VIRTUAL_EVENT_DURATION_MS), ... - endAt: new Date(next.getTime() + 5 * 60 * 1000), + endAt: new Date(next.getTime() + VIRTUAL_EVENT_DURATION_MS),Also applies to: 133-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/calendar/events/route.ts` around lines 106 - 110, Extract the hardcoded 5-minute duration into a named constant (e.g., VIRTUAL_EVENT_DURATION_MS = 5 * 60 * 1000) and use it where virtual events are created; update the two places that currently compute endAt as new Date(wf.lastRunAt.getTime() + 5 * 60 * 1000) (the virtualEvents push for workflow events and the other trigger-type virtualEvents creation) to reference VIRTUAL_EVENT_DURATION_MS instead, so both uses (in the logic that builds events from wf.lastRunAt) share the single named constant for clarity and maintainability.apps/web/src/lib/workflows/workflow-executor.ts (3)
98-99: Hardcoded default model name.
'glm-4.5-air'is a magic string. Consider extracting it to a named constant (e.g.,DEFAULT_PAGESPACE_MODEL) co-located with other AI configuration defaults to improve discoverability and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/workflow-executor.ts` around lines 98 - 99, The hardcoded model name 'glm-4.5-air' should be extracted to a named constant and used where selectedModel is computed; define DEFAULT_PAGESPACE_MODEL (co-located with other AI configuration defaults) and replace the magic string in the selectedModel assignment: use agent.aiModel || (selectedProvider === 'pagespace' ? DEFAULT_PAGESPACE_MODEL : undefined), and update any imports/exports so the constant is discoverable and maintainable alongside other AI defaults.
148-176: Reduce duplication between the twogenerateTextbranches.The with-tools and without-tools branches share ~90% of the configuration. The only difference is the
toolsandtoolChoiceproperties. Consider building a shared config object and conditionally adding the tool properties.♻️ Proposed refactor
- const result = Object.keys(availableTools).length > 0 - ? await generateText({ - model: providerResult.model, - system: enhancedSystemPrompt, - messages: convertToModelMessages(messages.map(m => ({ - role: m.role as 'user' | 'assistant', - content: m.content, - parts: [{ type: 'text' as const, text: m.content }], - }))), - tools: availableTools, - toolChoice: 'auto', - temperature: 0.7, - maxRetries: 3, - experimental_context: executionContext, - stopWhen: stepCountIs(100), - }) - : await generateText({ - model: providerResult.model, - system: enhancedSystemPrompt, - messages: convertToModelMessages(messages.map(m => ({ - role: m.role as 'user' | 'assistant', - content: m.content, - parts: [{ type: 'text' as const, text: m.content }], - }))), - temperature: 0.7, - maxRetries: 3, - experimental_context: executionContext, - stopWhen: stepCountIs(100), - }); + const hasTools = Object.keys(availableTools).length > 0; + const modelMessages = convertToModelMessages(messages.map(m => ({ + role: m.role as 'user' | 'assistant', + content: m.content, + parts: [{ type: 'text' as const, text: m.content }], + }))); + + const generateOptions = { + model: providerResult.model, + system: enhancedSystemPrompt, + messages: modelMessages, + temperature: 0.7, + maxRetries: 3, + experimental_context: executionContext, + stopWhen: stepCountIs(100), + ...(hasTools && { tools: availableTools, toolChoice: 'auto' as const }), + }; + + const result = await generateText(generateOptions);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/workflow-executor.ts` around lines 148 - 176, The two generateText calls are nearly identical; create a single shared config object (e.g., baseConfig) that includes model: providerResult.model, system: enhancedSystemPrompt, messages: convertToModelMessages(messages.map(...)), temperature: 0.7, maxRetries: 3, experimental_context: executionContext, and stopWhen: stepCountIs(100), then conditionally add tools and toolChoice when Object.keys(availableTools).length > 0, and call generateText once with that merged config (referencing generateText, availableTools, providerResult.model, enhancedSystemPrompt, convertToModelMessages, messages, executionContext, and stepCountIs).
27-255: Add timeout/abort mechanism togenerateTextcalls to prevent indefinite hangs.The
generateTextcalls at lines 188–207 lack any timeout or abort mechanism. WhilemaxRetries: 3handles transient failures, it cannot timeout slow or unresponsive AI providers. Since workflows can be triggered automatically (via events or cron), an unresponsive call will hang indefinitely, blocking execution. The Vercel AI SDK'sgenerateTextsupportsabortSignal—implement a timeout usingAbortController(e.g., 5 minutes) to ensure bounded execution time.🛡️ Example timeout addition
+ const WORKFLOW_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes + const abortController = new AbortController(); + const timeout = setTimeout(() => abortController.abort(), WORKFLOW_TIMEOUT_MS); + + try { const result = await generateText({ ...generateOptions, + abortSignal: abortController.signal, }); + } finally { + clearTimeout(timeout); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/workflow-executor.ts` around lines 27 - 255, The generateText calls in executeWorkflow lack an abort/timeout; create an AbortController before invoking generateText (e.g., const ac = new AbortController()), start a timeout (setTimeout to ~5 minutes) that calls ac.abort(), pass ac.signal as abortSignal to both generateText invocations (the branched calls where generateText is called), and clear the timeout after generateText resolves; also handle AbortError in the catch block to return a clear timeout error from executeWorkflow and log it via loggers.api.error. Ensure you reference generateText, executeWorkflow, and the branch that uses stepCountIs(100) so both call sites receive abortSignal.apps/web/src/app/api/workflows/[workflowId]/route.ts (1)
138-148: EnsureupdatedAtisn't double-set.The schema defines
updatedAtwith$onUpdate(() => new Date()), which means Drizzle automatically sets it on every.update()call. The explicitupdatedAt: new Date()on line 143 is redundant — it works, but it sets the timestamp twice (once explicitly, once via$onUpdate). Consider removing the explicit assignment to rely on the schema hook, keeping the update logic DRY with the rest of the codebase.🔧 Proposed fix
.set({ ...data, nextRunAt, - updatedAt: new Date(), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/workflows/`[workflowId]/route.ts around lines 138 - 148, Remove the explicit updatedAt assignment in the update call so the schema-level $onUpdate hook controls timestamps: inside the db.update(...).set({...}) call that updates the workflows table (the block that builds the object from data and nextRunAt), delete the updatedAt: new Date() property and rely on the schema’s $onUpdate(() => new Date()) for updatedAt; keep the rest of the update logic and the returning() call unchanged.apps/web/src/lib/workflows/event-trigger.ts (1)
64-92: Folder scoping: DB query per watched workflow per event could be expensive.For each event, every matching workflow with
watchedFolderIdstriggers a separate DB query (line 78-81) to resolve the resource'sparentId. Under high event throughput with many workflows watching folders, this could become a hot path. Consider batching the parentId lookup once per event before the workflow loop.♻️ Sketch: hoist the parentId lookup before the loop
+ // Pre-fetch resource parentId once for folder-scoping checks + let resourceParentId: string | null = null; + if (event.resourceId) { + try { + const [resource] = await db + .select({ parentId: pages.parentId }) + .from(pages) + .where(eq(pages.id, event.resourceId)); + resourceParentId = resource?.parentId ?? null; + } catch { + // Resource might not be a page + } + } + for (const workflow of matchingWorkflows) { // ...matching logic... const watchedFolderIds = (workflow.watchedFolderIds as string[] | null) ?? []; if (watchedFolderIds.length > 0) { let folderMatch = false; if (event.pageId && watchedFolderIds.includes(event.pageId)) { folderMatch = true; } - if (!folderMatch && event.resourceId) { - try { - const [resource] = await db - .select({ parentId: pages.parentId }) - .from(pages) - .where(eq(pages.id, event.resourceId)); - if (resource?.parentId && watchedFolderIds.includes(resource.parentId)) { - folderMatch = true; - } - } catch { } + if (!folderMatch && resourceParentId && watchedFolderIds.includes(resourceParentId)) { + folderMatch = true; } if (!folderMatch) continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/event-trigger.ts` around lines 64 - 92, The per-workflow DB lookup for resource parentId inside the loop (the db.select of pages.parentId when checking watchedFolderIds, using symbols watchedFolderIds, folderMatch, event.resourceId and pages.parentId) should be hoisted and cached: before iterating workflows, if event.resourceId exists perform a single lookup for the resource parentId (using db.select({ parentId: pages.parentId }).from(pages).where(eq(pages.id, event.resourceId))) and store the result (e.g., resourceParentId or null); then inside the loop use that cached resourceParentId to evaluate watchedFolderIds and set folderMatch (and keep the existing short-circuit for event.pageId checks) so you eliminate one DB call per workflow per event.
🤖 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/web/src/app/api/pages/`[pageId]/restore/route.ts:
- Line 112: The deferredTriggers.forEach(t => t()) call discards returned
Promises and can swallow errors; replace it by iterating the deferredTriggers
array with a for...of loop and invoke each trigger with explicit fire-and-forget
handling (e.g., use void t()) or await inside the loop, or wrap each call in
try/catch and log errors via the same logger used in restore/route.ts;
specifically update the place where deferredTriggers.forEach is used so you call
each trigger in a for (const t of deferredTriggers) { try { await t() } catch
(err) { logger.error(...) } } or, if truly fire-and-forget, use for (const t of
deferredTriggers) { void t().catch(err => logger.error(...)) } to satisfy the
Biome lint and surface errors.
In `@apps/web/src/app/api/workflows/`[workflowId]/run/route.ts:
- Around line 37-46: There is a TOCTOU race between checking
workflow.lastRunStatus === 'running' and the subsequent update; replace the
separate guard + update with an atomic conditional update on the workflows table
(use the same db.update(workflows).set(...) call but add a WHERE that includes
eq(workflows.id, workflowId) AND lastRunStatus != 'running' or
neq(workflows.lastRunStatus, 'running') ), then inspect the rowsAffected (or
returned count) from that update and return the 409 response if zero rows were
updated; keep references to workflowId, workflows.id, and lastRunStatus to
locate the code.
- Around line 42-65: The workflow can remain stuck in 'running' if
executeWorkflow throws; wrap the executeWorkflow call and the subsequent db
update in a try/catch (or try/catch/finally) so that any thrown error is caught
and you always update the workflows row: set lastRunAt, lastRunStatus to
'error', lastRunError to the caught error message, lastRunDurationMs (if
available or null), and compute nextRunAt via getNextRunDate only for cron
workflows; use the same db.update(...).where(eq(workflows.id, workflowId))
pattern to persist these fields and still rethrow or return the result as
needed.
In `@apps/web/src/app/api/workflows/route.ts`:
- Around line 64-75: The POST handler's call to await request.json() can throw
on malformed or missing JSON; wrap that call in a try/catch inside the POST
function (around where body is assigned) and return a NextResponse.json({...}, {
status: 400 }) with a clear error message when parsing fails; keep using
createWorkflowSchema.safeParse on the parsed body (variable body) if parsing
succeeds and ensure any thrown error is caught and converted to a 400 response
instead of letting it bubble to a 500.
In `@apps/web/src/components/workflows/WorkflowForm.tsx`:
- Line 70: The fetcher used by SWR (the fetcher constant that does
fetch(url).then(res => res.json())) swallows HTTP errors; update the fetcher to
check res.ok and, on non-2xx responses, parse the error body (await res.text()
or res.json()) and throw an Error (or reject) that includes the status and error
details so SWR treats it as an error; ensure the rest of the component that
reads pagesData (and the agent dropdown) relies on SWR's error state instead of
treating an error response as valid data.
In `@apps/web/src/components/workflows/WorkflowsDashboard.tsx`:
- Around line 25-60: Replace the duplicated inline type annotation in
handleCreate and handleUpdate with the shared WorkflowFormData type exported
from WorkflowForm.tsx: add an import for WorkflowFormData at the top of this
file and change the parameter type for both functions to WorkflowFormData (keep
all logic the same), removing the local inline interface declarations so the
component uses the single canonical type from WorkflowForm.tsx.
In `@apps/web/src/lib/workflows/event-trigger.ts`:
- Around line 111-128: The setTimeout async callback creates an unhandled
floating promise; wrap the entire callback body in a try/catch so any thrown
errors from the DB re-validation (db.select().from(workflows).where(...)) or
executeEventWorkflow are caught and logged. Ensure you still perform the
existing cleanup steps (debounceTimers.delete(workflow.id),
pendingEventContexts.get/delete) and the early-return checks after re-validation
inside the try block, and in the catch log the error (using your logger) and
avoid swallowing it silently; keep the timer variable (timer) behavior
unchanged.
- Around line 161-211: The non-atomic pattern around setting lastRunStatus in
executeEventWorkflow can race with other triggers; replace the separate
read-then-write with an atomic conditional update: attempt to set
lastRunStatus='running' (and lastRunAt=new Date()) via a single
db.update(workflows).set(...).where(eq(workflows.id, workflow.id),
ne(workflows.lastRunStatus, 'running')) and check the affected rows — if zero
rows were updated, abort execution early; keep the rest of executeEventWorkflow
(result handling and final status update) unchanged and use the same
workflow.id/workflow.name symbols for logging and error updates.
In `@apps/web/src/services/api/page-service.ts`:
- Line 584: The Biome lint warning is caused by the implicit return in the arrow
callback used with deferredTriggers.forEach; update the callback to use a block
body to make the intent explicit (e.g., replace the concise arrow t => t() with
a block-bodied form in the deferredTriggers.forEach call inside the file so the
function invoked by deferredTriggers is executed for side effects only). Ensure
you modify the deferredTriggers.forEach(...) expression so the callback body is
wrapped in braces and calls t() without returning a value.
---
Outside diff comments:
In `@apps/web/src/app/api/calendar/events/route.ts`:
- Around line 319-321: The early return when conditions.length === 0 returns
NextResponse.json({ events: [] }) but omits the workflowEvents key, causing an
inconsistent response shape; update that return to include workflowEvents (e.g.,
return NextResponse.json({ events: [], workflowEvents: [] })) so it matches the
other paths that return { events, workflowEvents } — change the
NextResponse.json call in the route handler where conditions is checked to
include the workflowEvents key.
In `@packages/lib/src/monitoring/activity-logger.ts`:
- Around line 596-657: logRollbackActivity currently discards the
DeferredWorkflowTrigger returned by logActivityWithTx causing rollback workflow
triggers to be lost when called with a transaction; update logRollbackActivity
to return Promise<DeferredWorkflowTrigger | undefined> (or remove the tx option
if you intend fire-and-forget) and propagate the trigger returned from
logActivityWithTx so callers (e.g., rollback-service) can capture and fire it
after the tx commits; adjust the function signature of logRollbackActivity and
its call sites to accept and forward the DeferredWorkflowTrigger similarly to
how page-mutation-service.ts and page-service.ts handle triggers.
---
Nitpick comments:
In `@apps/web/src/app/api/calendar/events/route.ts`:
- Around line 106-110: Extract the hardcoded 5-minute duration into a named
constant (e.g., VIRTUAL_EVENT_DURATION_MS = 5 * 60 * 1000) and use it where
virtual events are created; update the two places that currently compute endAt
as new Date(wf.lastRunAt.getTime() + 5 * 60 * 1000) (the virtualEvents push for
workflow events and the other trigger-type virtualEvents creation) to reference
VIRTUAL_EVENT_DURATION_MS instead, so both uses (in the logic that builds events
from wf.lastRunAt) share the single named constant for clarity and
maintainability.
In `@apps/web/src/app/api/cron/workflows/route.ts`:
- Line 9: Rename the module-level constant maxConcurrentWorkflows to follow
UPPER_SNAKE_CASE (MAX_CONCURRENT_WORKFLOWS) and update all usages to the new
name (e.g., where it is referenced in the workflows route handler). Ensure the
declaration (const MAX_CONCURRENT_WORKFLOWS = 5) and every reference (previously
maxConcurrentWorkflows) are updated consistently.
- Around line 49-56: The current two-step SELECT-then-UPDATE (selecting
dueWorkflows then calling db.update(workflows).set(...).where(eq(workflows.id,
wf.id))) has a TOCTOU race; replace it with a single atomic claim that updates
and returns rows so only non-running workflows are claimed. Change the logic
that builds dueWorkflows to perform an UPDATE on workflows with a WHERE that
filters due criteria AND lastRunStatus != 'running', set lastRunStatus='running'
and lastRunAt=new Date(), and use RETURNING (or the ORM's returning()
equivalent) to get the claimed workflows for execution; reference the existing
symbols lastRunStatus, lastRunAt, workflows, db.update(workflows) and use the
returned rows instead of the separate dueWorkflows select and subsequent per-id
updates.
- Around line 59-75: The code assumes workflow.cronExpression is non-null in
executeOne when calling getNextRunDate; add a defensive check for
workflow.cronExpression before calling getNextRunDate and handle the null case
by logging a warning (include workflow.id) and setting nextRunAt to null (and
optionally set lastRunError/lastRunStatus to indicate the invalid cron) before
performing the db.update(workflows).set(...) so a malformed DB row doesn't throw
at runtime; update references are in executeOne and the call to getNextRunDate.
In `@apps/web/src/app/api/pages/`[pageId]/tasks/[taskId]/route.ts:
- Around line 195-288: The deferredTrigger call after the db.transaction is
unguarded and can throw after the DB commit; wrap the invocation of
deferredTrigger (the deferredTrigger?.() call immediately after awaiting
db.transaction) in a try/catch, log the error with context (e.g., taskId,
pageId, userId, and that it occurred in deferredTrigger) and do not rethrow so
the HTTP response does not turn into a 500; ensure the catch only handles errors
from deferredTrigger and does not alter the successful transaction result from
db.transaction.
In `@apps/web/src/app/api/workflows/`[workflowId]/route.ts:
- Around line 138-148: Remove the explicit updatedAt assignment in the update
call so the schema-level $onUpdate hook controls timestamps: inside the
db.update(...).set({...}) call that updates the workflows table (the block that
builds the object from data and nextRunAt), delete the updatedAt: new Date()
property and rely on the schema’s $onUpdate(() => new Date()) for updatedAt;
keep the rest of the update logic and the returning() call unchanged.
In `@apps/web/src/app/api/workflows/`[workflowId]/run/__tests__/route.test.ts:
- Around line 241-255: Add a test in route.test.ts that mocks executeWorkflow to
reject with a thrown Error (e.g.,
vi.mocked(executeWorkflow).mockRejectedValue(new Error('Agent crashed'))) then
call the POST handler (POST with createContext('wf_1')) and assert the response
indicates failure (response.json().success === false and response.json().error
contains the thrown error message) and assert the workflow status was reset to
'error' (verify whatever persistence/update call your route uses to set status
back to 'error' was invoked with 'error'); reference executeWorkflow, POST, and
createContext when locating where to add the new test.
In `@apps/web/src/app/dashboard/`[driveId]/workflows/page.tsx:
- Around line 22-33: The duplicated loading skeleton markup should be extracted
into a small reusable component (e.g., WorkflowsSkeleton) so both the isLoading
branch and the Suspense fallback use the same JSX; create a WorkflowsSkeleton
component (defined in this file or imported) that returns the three Skeleton
elements and replace the inline blocks in the isLoading return and the Suspense
fallback with <WorkflowsSkeleton /> to remove duplication and keep behavior
identical.
In `@apps/web/src/components/workflows/WorkflowForm.tsx`:
- Around line 295-300: Replace the raw <input type="checkbox"> with the
shadcn/ui Checkbox component to match existing primitives; use the Checkbox's
checked and onCheckedChange props to read
isEventTriggerSelected(preset.operation, preset.resourceType) and call
toggleEventTrigger(preset.operation, preset.resourceType) (wrap the call in an
arrow to accept the boolean param if needed), and add the same styling/className
used elsewhere; also add an import for Checkbox from the shadcn/ui package and
ensure types line up (convert onCheckedChange value to boolean if required).
- Around line 95-101: The current useEffect that computes cron preview causes an
extra render by setting state from a synchronous function; replace this pattern
by deriving the preview with useMemo: remove the useEffect that references
cronExpression and setCronPreview, import useMemo, and create a const
cronPreview = useMemo(() => cronExpression ?
getHumanReadableCron(cronExpression) : '', [cronExpression]); then use
cronPreview wherever state was used (and remove setCronPreview and related state
initialization).
In `@apps/web/src/components/workflows/WorkflowStatusBadge.tsx`:
- Around line 5-17: Change the status prop on WorkflowStatusBadge to a string
literal union matching the keys of STATUS_CONFIG (e.g. 'never_run' | 'success' |
'error' | 'running') instead of string so the component type-safely indexes
STATUS_CONFIG without a cast; update the function signature for
WorkflowStatusBadge to use that union, remove the status as keyof typeof
STATUS_CONFIG cast when computing config, and ensure any callers pass one of the
union values (or adjust callers) so invalid statuses are caught at compile time.
In `@apps/web/src/lib/workflows/cron-utils.ts`:
- Around line 21-28: The validateTimezone function uses Intl.DateTimeFormat as a
call; change it to the idiomatic constructor form by using new
Intl.DateTimeFormat(undefined, { timeZone: timezone }) inside validateTimezone
so the try/catch remains but the instance is created via new for clarity and
style.
In `@apps/web/src/lib/workflows/event-trigger.ts`:
- Around line 64-92: The per-workflow DB lookup for resource parentId inside the
loop (the db.select of pages.parentId when checking watchedFolderIds, using
symbols watchedFolderIds, folderMatch, event.resourceId and pages.parentId)
should be hoisted and cached: before iterating workflows, if event.resourceId
exists perform a single lookup for the resource parentId (using db.select({
parentId: pages.parentId }).from(pages).where(eq(pages.id, event.resourceId)))
and store the result (e.g., resourceParentId or null); then inside the loop use
that cached resourceParentId to evaluate watchedFolderIds and set folderMatch
(and keep the existing short-circuit for event.pageId checks) so you eliminate
one DB call per workflow per event.
In `@apps/web/src/lib/workflows/workflow-executor.ts`:
- Around line 98-99: The hardcoded model name 'glm-4.5-air' should be extracted
to a named constant and used where selectedModel is computed; define
DEFAULT_PAGESPACE_MODEL (co-located with other AI configuration defaults) and
replace the magic string in the selectedModel assignment: use agent.aiModel ||
(selectedProvider === 'pagespace' ? DEFAULT_PAGESPACE_MODEL : undefined), and
update any imports/exports so the constant is discoverable and maintainable
alongside other AI defaults.
- Around line 148-176: The two generateText calls are nearly identical; create a
single shared config object (e.g., baseConfig) that includes model:
providerResult.model, system: enhancedSystemPrompt, messages:
convertToModelMessages(messages.map(...)), temperature: 0.7, maxRetries: 3,
experimental_context: executionContext, and stopWhen: stepCountIs(100), then
conditionally add tools and toolChoice when Object.keys(availableTools).length >
0, and call generateText once with that merged config (referencing generateText,
availableTools, providerResult.model, enhancedSystemPrompt,
convertToModelMessages, messages, executionContext, and stepCountIs).
- Around line 27-255: The generateText calls in executeWorkflow lack an
abort/timeout; create an AbortController before invoking generateText (e.g.,
const ac = new AbortController()), start a timeout (setTimeout to ~5 minutes)
that calls ac.abort(), pass ac.signal as abortSignal to both generateText
invocations (the branched calls where generateText is called), and clear the
timeout after generateText resolves; also handle AbortError in the catch block
to return a clear timeout error from executeWorkflow and log it via
loggers.api.error. Ensure you reference generateText, executeWorkflow, and the
branch that uses stepCountIs(100) so both call sites receive abortSignal.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
apps/web/package.jsonapps/web/src/app/api/calendar/events/route.tsapps/web/src/app/api/cron/workflows/__tests__/route.test.tsapps/web/src/app/api/cron/workflows/route.tsapps/web/src/app/api/pages/[pageId]/restore/route.tsapps/web/src/app/api/pages/[pageId]/tasks/[taskId]/route.tsapps/web/src/app/api/workflows/[workflowId]/__tests__/route.test.tsapps/web/src/app/api/workflows/[workflowId]/route.tsapps/web/src/app/api/workflows/[workflowId]/run/__tests__/route.test.tsapps/web/src/app/api/workflows/[workflowId]/run/route.tsapps/web/src/app/api/workflows/__tests__/route.test.tsapps/web/src/app/api/workflows/agents/route.tsapps/web/src/app/api/workflows/route.tsapps/web/src/app/dashboard/[driveId]/workflows/page.tsxapps/web/src/components/layout/left-sidebar/DriveFooter.tsxapps/web/src/components/workflows/DeleteWorkflowDialog.tsxapps/web/src/components/workflows/WorkflowForm.tsxapps/web/src/components/workflows/WorkflowList.tsxapps/web/src/components/workflows/WorkflowStatusBadge.tsxapps/web/src/components/workflows/WorkflowsDashboard.tsxapps/web/src/components/workflows/types.tsapps/web/src/hooks/useWorkflows.tsapps/web/src/instrumentation.tsapps/web/src/lib/workflows/__tests__/cron-utils.test.tsapps/web/src/lib/workflows/__tests__/event-trigger.test.tsapps/web/src/lib/workflows/__tests__/workflow-executor.test.tsapps/web/src/lib/workflows/cron-utils.tsapps/web/src/lib/workflows/event-trigger.tsapps/web/src/lib/workflows/workflow-executor.tsapps/web/src/services/api/page-mutation-service.tsapps/web/src/services/api/page-service.tsdocker/cron/crontabpackages/db/drizzle/0087_add_workflows.sqlpackages/db/drizzle/0088_add_workflow_event_triggers.sqlpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema.tspackages/db/src/schema/workflows.tspackages/lib/src/monitoring/activity-logger.ts
Comment on lines
+64
to
+75
| // POST /api/workflows - Create a new workflow | ||
| export async function POST(request: Request) { | ||
| const auth = await authenticateRequestWithOptions(request, AUTH_OPTIONS); | ||
| if (isAuthError(auth)) return auth.error; | ||
| const userId = auth.userId; | ||
|
|
||
| const body = await request.json(); | ||
| const parsed = createWorkflowSchema.safeParse(body); | ||
|
|
||
| if (!parsed.success) { | ||
| return NextResponse.json({ error: 'Invalid input', details: parsed.error.flatten() }, { status: 400 }); | ||
| } |
Contributor
There was a problem hiding this comment.
Unhandled JSON parse error on malformed request body.
If the client sends a non-JSON body (or no body), await request.json() at line 70 will throw, resulting in a 500 error instead of a clean 400. Consider wrapping it in a try/catch.
🛡️ Proposed fix
- const body = await request.json();
+ let body: unknown;
+ try {
+ body = await request.json();
+ } catch {
+ return NextResponse.json({ error: 'Invalid JSON body' }, { status: 400 });
+ }
const parsed = createWorkflowSchema.safeParse(body);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/workflows/route.ts` around lines 64 - 75, The POST
handler's call to await request.json() can throw on malformed or missing JSON;
wrap that call in a try/catch inside the POST function (around where body is
assigned) and return a NextResponse.json({...}, { status: 400 }) with a clear
error message when parsing fails; keep using createWorkflowSchema.safeParse on
the parsed body (variable body) if parsing succeeds and ensure any thrown error
is caught and converted to a 400 response instead of letting it bubble to a 500.
Owner
Author
There was a problem hiding this comment.
Noted. This follows the existing pattern across the codebase where request.json() is called directly and Next.js's error handling returns a 500. Adding a try/catch wrapper for JSON parsing is a broader codebase concern — out of scope for this PR but worth a follow-up.
Contributor
There was a problem hiding this comment.
@2witstudios, understood! That makes sense as a broader refactoring concern.
Would you like me to open a GitHub issue to track adding try/catch wrappers for request.json() calls across the API routes? I can create it with a reference back to this PR and comment.
🧠 Learnings used
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : In Route Handlers, get request body with `const body = await request.json();`
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T20:04:40.910Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : Get request body using `const body = await request.json();`
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-24T23:22:50.133Z
Learning: Applies to app/api/**/*.{ts,tsx} : Use `const body = await request.json();` to extract request bodies, `const { searchParams } = new URL(request.url);` for query parameters, and `return Response.json(data)` or `return NextResponse.json(data)` for responses
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-28T21:33:14.771Z
Learning: Applies to apps/web/app/**/*.ts : Use `const body = await request.json();` to read request body in route handlers
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-20T17:23:53.244Z
Learning: Tech stack: Next.js 15 App Router + TypeScript + Tailwind + shadcn/ui (frontend), PostgreSQL + Drizzle ORM (database), Ollama + Vercel AI SDK + OpenRouter + Google AI SDK (AI), custom JWT auth, local filesystem storage, Socket.IO for real-time, Docker deployment
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T20:04:40.910Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : In Next.js 15 route handlers, `params` in dynamic routes are Promise objects and MUST be awaited before destructuring
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-20T17:23:53.244Z
Learning: This is a monorepo using pnpm workspaces with structure: `apps/web` (Next.js frontend/backend), `apps/realtime` (Socket.IO service), `apps/processor` (Express file/OCR pipeline), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 258
File: apps/realtime/src/validation.ts:0-0
Timestamp: 2026-01-27T03:45:52.322Z
Learning: Enforce using paralleldrive/cuid2 for ID generation across the TypeScript codebase (not UUIDs). IDs should follow the CUID2 format: lowercase alphanumeric starting with a letter, matching ^[a-z][a-z0-9]{1,31}$ with a maximum length of 32 characters. Audit code paths that generate IDs (e.g., new UUID usages) and replace with cuid2 equivalents; ensure generated IDs are consistently lowercase and validated against the regex, and document any exceptions where IDs may differ in semantic meaning.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 406
File: apps/web/src/app/api/integrations/providers/route.ts:68-77
Timestamp: 2026-02-06T16:00:02.410Z
Learning: Maintain the existing pattern of using validation.error.flatten().fieldErrors across API routes. A future, repo-wide migration to Zod v4's z.flattenError() is planned; when migrating, update all affected routes to use z.flattenError() and adjust error handling accordingly, ensuring the fieldErrors structure remains consistent.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 699
File: apps/marketing/src/app/docs/self-hosting/environment/page.tsx:34-37
Timestamp: 2026-02-18T05:15:03.695Z
Learning: Ensure that cross-subdomain cookie handling uses two environment variables: COOKIE_DOMAIN (server-side, for Set-Cookie headers in server code like apps/web/src/lib/auth/cookie-config.ts) and NEXT_PUBLIC_COOKIE_DOMAIN (client-side, for document.cookie interactions in theme-cookie.ts in both apps/web and apps/marketing). This pattern should be verified across all related files that set or rely on cookie domains to maintain consistent domain scoping and enable cross-subdomain functionality.
Comment on lines
+25
to
+60
| const handleCreate = async (data: { | ||
| name: string; | ||
| agentPageId: string; | ||
| prompt: string; | ||
| contextPageIds: string[]; | ||
| triggerType: 'cron' | 'event'; | ||
| cronExpression?: string; | ||
| timezone: string; | ||
| isEnabled: boolean; | ||
| eventTriggers?: { operation: string; resourceType: string }[]; | ||
| watchedFolderIds?: string[]; | ||
| eventDebounceSecs?: number; | ||
| }) => { | ||
| await post('/api/workflows', { ...data, driveId }); | ||
| mutate(); | ||
| toast.success('Workflow created'); | ||
| }; | ||
|
|
||
| const handleUpdate = async (data: { | ||
| name: string; | ||
| agentPageId: string; | ||
| prompt: string; | ||
| contextPageIds: string[]; | ||
| triggerType: 'cron' | 'event'; | ||
| cronExpression?: string; | ||
| timezone: string; | ||
| isEnabled: boolean; | ||
| eventTriggers?: { operation: string; resourceType: string }[]; | ||
| watchedFolderIds?: string[]; | ||
| eventDebounceSecs?: number; | ||
| }) => { | ||
| if (!editingWorkflow) return; | ||
| await patch(`/api/workflows/${editingWorkflow.id}`, data); | ||
| mutate(); | ||
| toast.success('Workflow updated'); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and inspect WorkflowForm.tsx to check error handling
fd "WorkflowForm\.tsx" --type f | head -5Repository: 2witstudios/PageSpace
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Once we find WorkflowForm, check its onSubmit handling and error catching
fd "WorkflowForm\.tsx" --type f --exec cat {} \;Repository: 2witstudios/PageSpace
Length of output: 15184
Type duplication: import WorkflowFormData from WorkflowForm.tsx instead of redefining it.
The WorkflowFormData interface is already defined and exported in WorkflowForm.tsx (lines 25–32). Replace the inline type definitions in both handleCreate and handleUpdate with an import to avoid duplication.
Error handling is already covered — WorkflowForm.handleSubmit wraps the onSubmit callback in a try/catch block and displays errors to the user via form state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/workflows/WorkflowsDashboard.tsx` around lines 25 -
60, Replace the duplicated inline type annotation in handleCreate and
handleUpdate with the shared WorkflowFormData type exported from
WorkflowForm.tsx: add an import for WorkflowFormData at the top of this file and
change the parameter type for both functions to WorkflowFormData (keep all logic
the same), removing the local inline interface declarations so the component
uses the single canonical type from WorkflowForm.tsx.
Owner
Author
There was a problem hiding this comment.
Acknowledged. This component follows the existing dashboard patterns. Error boundary improvements can be a separate follow-up.
Contributor
There was a problem hiding this comment.
@2witstudios, understood! Happy to help track this as a follow-up if you'd like. Would you like me to create an issue for refactoring the WorkflowFormData type duplication across the dashboard components?
🧠 Learnings used
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 258
File: apps/realtime/src/validation.ts:0-0
Timestamp: 2026-01-27T03:45:52.322Z
Learning: Enforce using paralleldrive/cuid2 for ID generation across the TypeScript codebase (not UUIDs). IDs should follow the CUID2 format: lowercase alphanumeric starting with a letter, matching ^[a-z][a-z0-9]{1,31}$ with a maximum length of 32 characters. Audit code paths that generate IDs (e.g., new UUID usages) and replace with cuid2 equivalents; ensure generated IDs are consistently lowercase and validated against the regex, and document any exceptions where IDs may differ in semantic meaning.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 699
File: apps/marketing/src/app/docs/self-hosting/environment/page.tsx:34-37
Timestamp: 2026-02-18T05:15:03.695Z
Learning: Ensure that cross-subdomain cookie handling uses two environment variables: COOKIE_DOMAIN (server-side, for Set-Cookie headers in server code like apps/web/src/lib/auth/cookie-config.ts) and NEXT_PUBLIC_COOKIE_DOMAIN (client-side, for document.cookie interactions in theme-cookie.ts in both apps/web and apps/marketing). This pattern should be verified across all related files that set or rely on cookie domains to maintain consistent domain scoping and enable cross-subdomain functionality.
…est assertions - Fix calendar/events/route.ts logger call: use object instead of Error for LogInput - Fix types.ts: import EventTrigger locally before re-exporting (was only re-exported) - Fix test file: add non-null assertions on response to satisfy strict null checks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ic fixes - Wrap executeWorkflow in try/catch in manual run route to reset status on throw - Wrap setTimeout async callback in IIFE with try/catch to prevent unhandled rejections - Replace forEach(t => t()) with for...of to avoid implicit return values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…snapshots - Cron route: replace SELECT+UPDATE with atomic UPDATE...RETURNING to prevent double-execution on overlapping cron invocations - Manual run route: replace check-then-set with atomic UPDATE...RETURNING to prevent race on rapid double-clicks - WorkflowForm: add res.ok check to SWR fetcher so non-2xx responses throw instead of being silently treated as data - Add missing Drizzle snapshots for migrations 0087/0088 (hand-written migrations lacked snapshot JSON files, breaking future db:generate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/calendar/events/route.ts (1)
319-321:⚠️ Potential issue | 🟡 MinorKeep the response shape consistent when no conditions match.
With
workflowEventsnow included in other paths, this early return drops the field entirely. That can break clients expecting a stable shape. Consider returning an empty array forworkflowEventshere.🔧 Suggested fix
- if (conditions.length === 0) { - return NextResponse.json({ events: [] }); - } + if (conditions.length === 0) { + return NextResponse.json({ events: [], workflowEvents: [] }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/calendar/events/route.ts` around lines 319 - 321, The early return in route.ts that fires when conditions.length === 0 returns NextResponse.json({ events: [] }) but omits workflowEvents, breaking clients expecting a stable shape; update that branch to return the same response shape used elsewhere (e.g., NextResponse.json({ events: [], workflowEvents: [] })) so workflowEvents is always present, and ensure any other early-return paths in the same handler (the function around the conditions check) follow the same shape.
🧹 Nitpick comments (2)
apps/web/src/app/api/workflows/[workflowId]/run/route.ts (1)
49-51: Add an explicit type annotation toresult.
let result;relies on TypeScript's control-flow narrowing (since thecatchalways returns,resultisWorkflowExecutionResultafter the block). It's correct at runtime, but an explicit annotation is more "explicit over implicit" per the coding guidelines.♻️ Proposed fix
+import { executeWorkflow, type WorkflowExecutionResult } from '@/lib/workflows/workflow-executor'; ... - let result; + let result: WorkflowExecutionResult; try { result = await executeWorkflow(workflow);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/workflows/`[workflowId]/run/route.ts around lines 49 - 51, Declare an explicit type for the local variable `result` instead of `let result;` — change its declaration to `let result: WorkflowExecutionResult;` (or `let result: WorkflowExecutionResult | undefined` if initialization paths require it) so the return from `executeWorkflow(workflow)` is explicitly typed; also add/import the `WorkflowExecutionResult` type at the top of the file if it's not already imported.apps/web/src/app/api/workflows/[workflowId]/run/__tests__/route.test.ts (1)
253-267: Add a test for theexecuteWorkflowthrow path (500 + status reset).The existing test at line 253 covers
executeWorkflowresolving with{ success: false }, but thecatchblock at route lines 52–59 (whenexecuteWorkflowthrows) is untested. That path resetslastRunStatusto'error'and returns a 500 — both behaviours should be asserted.A test for
access.drive === null→ 404 "Drive not found" (route lines 30–32) is also absent.🧪 Suggested additional tests
test('returns 500 and resets status when executeWorkflow throws', async () => { vi.mocked(executeWorkflow).mockRejectedValue(new Error('Unexpected crash')); const request = new Request('https://example.com/api/workflows/wf_1/run', { method: 'POST' }); const response = await POST(request, createContext('wf_1')); expect(response.status).toBe(500); const body = await response.json(); expect(body.success).toBe(false); expect(body.error).toBe('Unexpected crash'); // Verify status was reset to 'error' expect(mockUpdateSet).toHaveBeenCalledWith( expect.objectContaining({ lastRunStatus: 'error', lastRunError: 'Unexpected crash' }), ); }); test('returns 404 when drive is not found', async () => { vi.mocked(checkDriveAccess).mockResolvedValue(createAccessFixture({ drive: null })); const request = new Request('https://example.com/api/workflows/wf_1/run', { method: 'POST' }); const response = await POST(request, createContext('wf_1')); expect(response.status).toBe(404); const body = await response.json(); expect(body.error).toBe('Drive not found'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/workflows/`[workflowId]/run/__tests__/route.test.ts around lines 253 - 267, Add two tests: one that mocks executeWorkflow to reject (vi.mocked(executeWorkflow).mockRejectedValue(new Error('...'))) and calls the POST route handler (POST with createContext('wf_1')), asserting response.status === 500, body.success === false and body.error matches the thrown message, and that mockUpdateSet was called with an object containing lastRunStatus: 'error' and lastRunError set to the error message; and another that mocks checkDriveAccess to resolve to an access fixture with drive: null (vi.mocked(checkDriveAccess).mockResolvedValue(createAccessFixture({ drive: null }))), calls POST(createContext('wf_1')) and asserts response.status === 404 and body.error === 'Drive not found'.
🤖 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/web/src/app/api/cron/workflows/__tests__/route.test.ts`:
- Around line 65-83: Rename the test fixture constant mockWorkflow to
UPPER_SNAKE_CASE (e.g., MOCK_WORKFLOW) and update every reference to it within
the tests in route.test.ts so they point to the new name; ensure the declaration
stays a const and that any places using mockWorkflow in test bodies,
setup/teardown, or assertions (search for "mockWorkflow") are updated to
"MOCK_WORKFLOW".
In `@apps/web/src/app/api/workflows/`[workflowId]/run/__tests__/route.test.ts:
- Around line 101-120: The mockWorkflow object used in the tests is missing
nullable DB fields (eventTriggers, watchedFolderIds, eventDebounceSecs) that
workflows.$inferSelect expects; update the mockWorkflow definition to include
those properties with appropriate nullable values (e.g., eventTriggers: null or
empty array, watchedFolderIds: null or empty array, eventDebounceSecs: null or a
number) so the object matches the inferred workflow type and avoids silent type
mismatches in tests that reference executeWorkflow or assert these fields.
---
Outside diff comments:
In `@apps/web/src/app/api/calendar/events/route.ts`:
- Around line 319-321: The early return in route.ts that fires when
conditions.length === 0 returns NextResponse.json({ events: [] }) but omits
workflowEvents, breaking clients expecting a stable shape; update that branch to
return the same response shape used elsewhere (e.g., NextResponse.json({ events:
[], workflowEvents: [] })) so workflowEvents is always present, and ensure any
other early-return paths in the same handler (the function around the conditions
check) follow the same shape.
---
Nitpick comments:
In `@apps/web/src/app/api/workflows/`[workflowId]/run/__tests__/route.test.ts:
- Around line 253-267: Add two tests: one that mocks executeWorkflow to reject
(vi.mocked(executeWorkflow).mockRejectedValue(new Error('...'))) and calls the
POST route handler (POST with createContext('wf_1')), asserting response.status
=== 500, body.success === false and body.error matches the thrown message, and
that mockUpdateSet was called with an object containing lastRunStatus: 'error'
and lastRunError set to the error message; and another that mocks
checkDriveAccess to resolve to an access fixture with drive: null
(vi.mocked(checkDriveAccess).mockResolvedValue(createAccessFixture({ drive: null
}))), calls POST(createContext('wf_1')) and asserts response.status === 404 and
body.error === 'Drive not found'.
In `@apps/web/src/app/api/workflows/`[workflowId]/run/route.ts:
- Around line 49-51: Declare an explicit type for the local variable `result`
instead of `let result;` — change its declaration to `let result:
WorkflowExecutionResult;` (or `let result: WorkflowExecutionResult | undefined`
if initialization paths require it) so the return from
`executeWorkflow(workflow)` is explicitly typed; also add/import the
`WorkflowExecutionResult` type at the top of the file if it's not already
imported.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web/src/app/api/calendar/events/route.tsapps/web/src/app/api/cron/workflows/__tests__/route.test.tsapps/web/src/app/api/cron/workflows/route.tsapps/web/src/app/api/pages/[pageId]/restore/route.tsapps/web/src/app/api/workflows/[workflowId]/__tests__/route.test.tsapps/web/src/app/api/workflows/[workflowId]/run/__tests__/route.test.tsapps/web/src/app/api/workflows/[workflowId]/run/route.tsapps/web/src/components/workflows/WorkflowForm.tsxapps/web/src/components/workflows/types.tsapps/web/src/lib/workflows/event-trigger.tsapps/web/src/services/api/page-service.tspackages/db/drizzle/meta/0087_snapshot.jsonpackages/db/drizzle/meta/0088_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/components/workflows/types.ts
- apps/web/src/components/workflows/WorkflowForm.tsx
- apps/web/src/lib/workflows/event-trigger.ts
- apps/web/src/app/api/cron/workflows/route.ts
- apps/web/src/app/api/workflows/[workflowId]/tests/route.test.ts
…-by-one, and trigger type guard - WorkflowForm: replace plain fetch with fetchJSON from auth-fetch so agent dropdown works in token-based desktop/mobile sessions - Cron route: advance nextRunAt when resetting stuck workflows so they aren't immediately re-claimed in the same invocation - Calendar events: offset cron-parser currentDate by -1ms so occurrences exactly at range start are included (next() is exclusive of currentDate) - Event trigger debounce: re-validate triggerType === 'event' before executing, preventing stale timers from firing after mode switch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/web/src/lib/workflows/event-trigger.ts (3)
154-154: Constant should useUPPER_SNAKE_CASE.Per coding guidelines, constants should use
UPPER_SNAKE_CASE.Proposed fix
-const maxContextFieldLength = 200; +const MAX_CONTEXT_FIELD_LENGTH = 200; -const truncate = (value: unknown, max = maxContextFieldLength): string => { +const truncate = (value: unknown, max = MAX_CONTEXT_FIELD_LENGTH): string => {As per coding guidelines: "Use UPPER_SNAKE_CASE for constants".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/event-trigger.ts` at line 154, Rename the constant maxContextFieldLength to follow UPPER_SNAKE_CASE as MAX_CONTEXT_FIELD_LENGTH; update every reference to maxContextFieldLength in this file (e.g., in event-trigger.ts functions that use it) to the new identifier, keep the same numeric value (200) and type, and adjust any exports, imports, or tests that reference the old name so compilation passes.
176-201: RedundantlastRunAtwrite and unguarded error-path DB update.Two observations:
Redundant
lastRunAt: Line 181 setslastRunAt: new Date(), then Line 196 overwrites it with anothernew Date(). The first write is always overwritten on the success path. If the intent is to record the start time, consider storing it in a local variable and reusing it, or removing the first write.Error-path DB update is unguarded: If
executeWorkflow(Line 190) throws and the subsequent error-status DB update (Lines 216–222) also fails (e.g., DB is temporarily unreachable), the exception propagates to the outer catch in the debounce callback where it's logged — but the workflow row remains stuck in'running'status. Wrapping the error-path update in its own try/catch would at least log the secondary failure explicitly.Proposed fix for the error-path guard
} catch (error) { const errorMsg = error instanceof Error ? error.message : String(error); loggers.api.error('Event-triggered workflow failed', { workflowId: workflow.id, error: errorMsg, }); - await db - .update(workflows) - .set({ - lastRunStatus: 'error', - lastRunError: errorMsg, - }) - .where(eq(workflows.id, workflow.id)); + try { + await db + .update(workflows) + .set({ + lastRunStatus: 'error', + lastRunError: errorMsg, + }) + .where(eq(workflows.id, workflow.id)); + } catch (dbError) { + loggers.api.error('Failed to update workflow error status', { + workflowId: workflow.id, + error: dbError instanceof Error ? dbError.message : String(dbError), + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/event-trigger.ts` around lines 176 - 201, The first update that sets lastRunAt to new Date() is redundant because it gets overwritten later — capture the start time in a local variable (e.g., startTime = new Date()) and use that timestamp for any "start" vs "end" fields, or remove the initial db.update that sets lastRunAt entirely and only write the final lastRunAt when persisting result; additionally, guard the error-path DB update that flips lastRunStatus from 'running' to 'error' by wrapping the db.update(workflows).set(... lastRunStatus: 'error' ...) call (the update after executeWorkflow or in the catch block around executeWorkflow) in its own try/catch so a secondary DB failure is caught and logged (use the existing logger) to avoid leaving the row stuck in 'running' if the secondary update fails.
97-106: Debounce does not reset the timer on subsequent events — verify this is intended.When a timer already exists (Line 101), the code updates the pending context but does not clear and restart the timer. This means the debounce window is anchored to the first event, not the latest. Subsequent events arriving near the end of the window won't extend it.
This is a valid "leading-edge" debounce strategy but differs from a typical "trailing-edge reset" debounce. The test on Line 197 (
rapid events coalesce into single execution) passes because all three events fire before the timer expires, but it would not catch a scenario where a fourth event arrives at T=9s (with a 10s debounce) and should arguably reset the window.If this is intentional (bounded latency), a brief inline comment would help future readers. If not,
clearTimeout+ re-set is the fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/event-trigger.ts` around lines 97 - 106, The debounce currently leaves the original timeout running when debounceTimers.get(workflow.id) returns an existingTimer, so subsequent events only update pendingEventContexts but do not extend the debounce window; update the branch handling existingTimer to clearTimeout(existingTimer) and create/register a new timer (using the same handler logic as where you initially set the timer) so the debounce is trailing-edge (or alternatively add an explicit inline comment near debounceTimers/get and pendingEventContexts.set to document that the leading-edge behavior is intentional if you prefer bounded latency); reference debounceTimers, pendingEventContexts, workflow.id and debounceSecs when applying the change.apps/web/src/lib/workflows/__tests__/event-trigger.test.ts (2)
375-405: Consider adding a test forexecuteWorkflowthrowing an exception.The current tests cover
success: trueandsuccess: falsereturn values, but there's no test for the case whereexecuteWorkflowthrows an error (e.g., network failure). The source code handles this in thecatchblock ofexecuteEventWorkflow(Lines 209–223 in event-trigger.ts), which writes error status to the DB — this path is currently untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/__tests__/event-trigger.test.ts` around lines 375 - 405, Add a test that simulates executeWorkflow throwing an exception to verify executeEventWorkflow's catch path writes error status to the DB: mock executeWorkflow to reject (throw) with an Error (e.g., new Error('network')), call emitWorkflowEvent(createEvent()) after stubbing mockSelectWhere to return a workflow, advance timers, and assert that mockUpdateSet (or mockUpdate) was called with an object containing lastRunStatus: 'error' and lastRunError matching the thrown error message; reference the existing tests for mark-running behavior and use the same helpers (emitWorkflowEvent, createEvent, createWorkflow, mockSelectWhere, mockUpdateSet) to add this test.
232-255: FragilecallCount-based mock — consider a more resilient approach.The sequential
callCountmock couples the test to the exact number and order of internal DB queries. If the implementation adds a query (e.g., an audit log or an extra validation), these tests silently break or pass for the wrong reasons.A more resilient alternative is to inspect the arguments passed to
mockSelectWhere(or the preceding chain methods) and return different results based on the query shape. That said, this is a common trade-off in unit tests and not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/workflows/__tests__/event-trigger.test.ts` around lines 232 - 255, The test's fragile callCount-based mock for mockSelectWhere should be replaced with an argument-inspection mock that returns results based on the query shape rather than call order: update the mockSelect and mockSelectFrom chain (mockSelect -> mockSelectFrom -> mockSelectWhere) so mockSelectWhere inspects its incoming "where" argument (or the preceding "from" value) to detect the workflow lookup (e.g., watchedFolderIds lookup), the resource parent lookup (e.g., a query filtering by resource id or type), and the re-validation query, and return [workflow], [{ parentId: 'folder_b' }], or [workflow] respectively; keep using emitWorkflowEvent(createEvent(...)) and assert executeWorkflow was called once.
🤖 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/web/src/app/api/cron/workflows/route.ts`:
- Around line 63-76: The code calls getNextRunDate(workflow.cronExpression!,
workflow.timezone) directly in executeOne (and in the error handler later) which
can throw and break the whole cron flow; create a small safe wrapper (e.g.,
safeGetNextRunDate) that catches exceptions from getNextRunDate and returns null
(or the existing workflow.nextRunAt) while logging the error, then replace
direct calls in executeOne and the error-path (the update block around
getNextRunDate and the error handler at lines ~106-112) to use
safeGetNextRunDate(workflow) so a single bad cron/timezone won’t cascade-fail
the job.
- Line 9: Rename the constant maxConcurrentWorkflows to UPPER_SNAKE_CASE
(MAX_CONCURRENT_WORKFLOWS) and update every usage site to the new name; ensure
you change its declaration (const maxConcurrentWorkflows = 5) and all references
inside the module (and any exports/imports if present). Also locate and rename
the other workflow-related constants mentioned in the review to UPPER_SNAKE_CASE
as well so the code follows the constant naming guideline consistently.
- Around line 18-29: The update currently resets any workflow stuck in 'running'
older than stuckCutoff; restrict this to cron-triggered jobs by adding a filter
to the db.update(workflows) query (the same clause that builds stuckWorkflows)
such that only workflows with triggerType === 'cron' (or cronExpression IS NOT
NULL) are matched—update the where(...) call that uses
and(eq(workflows.lastRunStatus, 'running'), lte(workflows.lastRunAt,
stuckCutoff)) to include the cron filter so only cron workflows are marked error
and advanced.
---
Nitpick comments:
In `@apps/web/src/lib/workflows/__tests__/event-trigger.test.ts`:
- Around line 375-405: Add a test that simulates executeWorkflow throwing an
exception to verify executeEventWorkflow's catch path writes error status to the
DB: mock executeWorkflow to reject (throw) with an Error (e.g., new
Error('network')), call emitWorkflowEvent(createEvent()) after stubbing
mockSelectWhere to return a workflow, advance timers, and assert that
mockUpdateSet (or mockUpdate) was called with an object containing
lastRunStatus: 'error' and lastRunError matching the thrown error message;
reference the existing tests for mark-running behavior and use the same helpers
(emitWorkflowEvent, createEvent, createWorkflow, mockSelectWhere, mockUpdateSet)
to add this test.
- Around line 232-255: The test's fragile callCount-based mock for
mockSelectWhere should be replaced with an argument-inspection mock that returns
results based on the query shape rather than call order: update the mockSelect
and mockSelectFrom chain (mockSelect -> mockSelectFrom -> mockSelectWhere) so
mockSelectWhere inspects its incoming "where" argument (or the preceding "from"
value) to detect the workflow lookup (e.g., watchedFolderIds lookup), the
resource parent lookup (e.g., a query filtering by resource id or type), and the
re-validation query, and return [workflow], [{ parentId: 'folder_b' }], or
[workflow] respectively; keep using emitWorkflowEvent(createEvent(...)) and
assert executeWorkflow was called once.
In `@apps/web/src/lib/workflows/event-trigger.ts`:
- Line 154: Rename the constant maxContextFieldLength to follow UPPER_SNAKE_CASE
as MAX_CONTEXT_FIELD_LENGTH; update every reference to maxContextFieldLength in
this file (e.g., in event-trigger.ts functions that use it) to the new
identifier, keep the same numeric value (200) and type, and adjust any exports,
imports, or tests that reference the old name so compilation passes.
- Around line 176-201: The first update that sets lastRunAt to new Date() is
redundant because it gets overwritten later — capture the start time in a local
variable (e.g., startTime = new Date()) and use that timestamp for any "start"
vs "end" fields, or remove the initial db.update that sets lastRunAt entirely
and only write the final lastRunAt when persisting result; additionally, guard
the error-path DB update that flips lastRunStatus from 'running' to 'error' by
wrapping the db.update(workflows).set(... lastRunStatus: 'error' ...) call (the
update after executeWorkflow or in the catch block around executeWorkflow) in
its own try/catch so a secondary DB failure is caught and logged (use the
existing logger) to avoid leaving the row stuck in 'running' if the secondary
update fails.
- Around line 97-106: The debounce currently leaves the original timeout running
when debounceTimers.get(workflow.id) returns an existingTimer, so subsequent
events only update pendingEventContexts but do not extend the debounce window;
update the branch handling existingTimer to clearTimeout(existingTimer) and
create/register a new timer (using the same handler logic as where you initially
set the timer) so the debounce is trailing-edge (or alternatively add an
explicit inline comment near debounceTimers/get and pendingEventContexts.set to
document that the leading-edge behavior is intentional if you prefer bounded
latency); reference debounceTimers, pendingEventContexts, workflow.id and
debounceSecs when applying the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/src/app/api/calendar/events/route.tsapps/web/src/app/api/cron/workflows/route.tsapps/web/src/components/workflows/WorkflowForm.tsxapps/web/src/lib/workflows/__tests__/event-trigger.test.tsapps/web/src/lib/workflows/event-trigger.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/app/api/calendar/events/route.ts
- apps/web/src/components/workflows/WorkflowForm.tsx
…entions - Limit stuck-workflow reset query to triggerType='cron' — prevents incorrectly marking long-running event workflows as errored - Guard getNextRunDate() calls with try/catch in executeOne and error handler — prevents a single bad cron expression from cascading and crashing the entire cron job - Rename maxConcurrentWorkflows → MAX_CONCURRENT_WORKFLOWS (UPPER_SNAKE_CASE) - Rename mockWorkflow → MOCK_WORKFLOW in cron route tests - Add missing nullable schema fields (eventTriggers, watchedFolderIds, eventDebounceSecs) to test fixtures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rval
- Claim cron workflows per batch (not all at once) to prevent the stuck-
workflow timeout from racing against not-yet-started items in later
batches — eliminates duplicate execution risk for large batches
- Reject PATCH { eventTriggers: null } on event workflows — uses !== undefined
instead of ?? so explicit null is treated as "clear triggers" and fails
validation, preventing workflows with no trigger definitions
- Enforce 5-minute minimum cron interval in validateCronExpression() —
aligns validation with the cron polling cadence so per-minute schedules
are rejected instead of silently dropping intermediate occurrences
- Add tests for null eventTriggers rejection, sub-5-minute cron rejection,
and update cron route tests for per-batch claiming pattern
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/web/src/app/api/workflows/[workflowId]/run/__tests__/route.test.ts (1)
101-123: RenamemockWorkflowtoMOCK_WORKFLOWfor guideline compliance.This is the same constant-fixture pattern that was already renamed in the cron test file (
MOCK_WORKFLOW). Apply the same convention here for consistency.♻️ Suggested rename
-const mockWorkflow = { +const MOCK_WORKFLOW = { id: 'wf_1',Then update all references (lines 140, 219, 228):
- mockSelectWhere.mockResolvedValue([mockWorkflow]); + mockSelectWhere.mockResolvedValue([MOCK_WORKFLOW]); ... - expect(executeWorkflow).toHaveBeenCalledWith(mockWorkflow); + expect(executeWorkflow).toHaveBeenCalledWith(MOCK_WORKFLOW); ... - ...mockWorkflow, + ...MOCK_WORKFLOW,As per coding guidelines, "Use UPPER_SNAKE_CASE for constants."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/workflows/`[workflowId]/run/__tests__/route.test.ts around lines 101 - 123, Rename the test fixture variable mockWorkflow to UPPER_SNAKE_CASE (MOCK_WORKFLOW) to follow the constant naming guideline; update the declaration (the const mockWorkflow object) to const MOCK_WORKFLOW and replace every usage/occurrence of mockWorkflow in this test file (all assertions, function calls, and helper references that consume the fixture) so the tests compile and reference the new identifier.apps/web/src/app/api/cron/workflows/route.ts (1)
124-143: Error-recovery DB update in the rejected handler is not wrapped in try/catch.If the
db.updateat line 135 throws (e.g., connection error), the exception propagates out of theforloop and hits the top-level catch at line 155, aborting all remaining batches. Remaining un-processed batches won't be claimed, so there's no data-corruption risk, but any already-counted results from prior batches are lost (the 500 response replaces the partial-success response).A try/catch here would let the loop continue to the next batch and include the DB failure in the
errorsarray.♻️ Suggested fix
} else { const workflow = claimed[j]; const errorMsg = settled.reason instanceof Error ? settled.reason.message : String(settled.reason); loggers.api.error(`Workflow cron: Failed for workflow ${workflow.id}`, { error: errorMsg }); errors.push(`${workflow.name}: ${errorMsg}`); + try { let nextRunAt: Date | undefined; try { nextRunAt = getNextRunDate(workflow.cronExpression!, workflow.timezone); } catch { /* invalid cron — leave nextRunAt as-is */ } await db .update(workflows) .set({ lastRunStatus: 'error', lastRunError: errorMsg, ...(nextRunAt ? { nextRunAt } : {}), }) .where(eq(workflows.id, workflow.id)); + } catch (dbErr) { + loggers.api.error(`Workflow cron: Failed to persist error state for ${workflow.id}`, { + error: dbErr instanceof Error ? dbErr.message : String(dbErr), + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/cron/workflows/route.ts` around lines 124 - 143, The rejected-handler's DB update call (the await db.update(...).where(eq(workflows.id, workflow.id)) inside the else branch handling settled rejections) isn't wrapped in try/catch; wrap that update in a try/catch so DB failures don't break the outer loop — on catch, log the error via loggers.api.error including workflow.id and the caught error, push a descriptive message into the existing errors array (e.g., `${workflow.name}: DB update failed: ${err.message || String(err)}`), and continue processing the next claimed workflow; keep nextRunAt calculation and lastRunStatus/lastRunError behavior unchanged and ensure the original errorMsg (from settled.reason) is preserved in the errors list when the DB update succeeds.apps/web/src/app/api/cron/workflows/__tests__/route.test.ts (1)
144-163:mockReturningis shared across both the stuck-reset and atomic-claim code paths.When
mockReturning.mockResolvedValue([MOCK_WORKFLOW])is set at line 146, the stuck-workflow reset (which also calls.returning()) will receive[MOCK_WORKFLOW]too, inadvertently exercising the stuck-workflow-advance path. This doesn't break the current assertions, but it makes the test less precise — if future assertions checkgetNextRunDatecall counts ormockUpdateinvocation count, they'll be off.Consider using
mockReturning.mockResolvedValueOnce([]).mockResolvedValueOnce([MOCK_WORKFLOW])to return[]for the stuck-reset call and[MOCK_WORKFLOW]for the atomic claim call only. The same applies to the error/exception tests below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/cron/workflows/__tests__/route.test.ts` around lines 144 - 163, The test currently stubs mockReturning globally which causes the stuck-workflow reset path to see the same returned row; update the test to sequence the returning() responses so the stuck-reset receives an empty array and the atomic-claim receives MOCK_WORKFLOW by replacing mockReturning.mockResolvedValue([MOCK_WORKFLOW]) with mockReturning.mockResolvedValueOnce([]).mockResolvedValueOnce([MOCK_WORKFLOW]) in this test (and apply the same pattern in the related error/exception tests), leaving mockSelectWhere, getNextRunDate, executeWorkflow and the POST invocation unchanged.
🤖 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/web/src/app/api/workflows/`[workflowId]/route.ts:
- Around line 30-41: Replace the local isOwner/isAdmin checks in
getWorkflowWithAuth with the centralized permission helpers from
`@pagespace/lib/permissions`: import and call getUserAccessLevel (or
canUserEditPage) with the driveId and userId (or with the checkDriveAccess
result if the helper expects it), use the helper's result to decide whether the
user can manage workflows, and return the same 403 NextResponse when the helper
denies edit rights; keep the existing drive-not-found handling (access.drive)
and only substitute the manual access.isOwner && access.isAdmin logic with the
centralized permission check.
- Around line 74-80: The response uses parsed.error.flatten() instead of the
project's standard parsed.error.flatten().fieldErrors, causing inconsistent
validation error shape; update the error return in route handler (where
updateWorkflowSchema.safeParse(body) is used and parsed is checked) to return
NextResponse.json({ error: 'Invalid input', details:
parsed.error.flatten().fieldErrors }, { status: 400 }) so the API matches other
routes' fieldErrors format.
---
Nitpick comments:
In `@apps/web/src/app/api/cron/workflows/__tests__/route.test.ts`:
- Around line 144-163: The test currently stubs mockReturning globally which
causes the stuck-workflow reset path to see the same returned row; update the
test to sequence the returning() responses so the stuck-reset receives an empty
array and the atomic-claim receives MOCK_WORKFLOW by replacing
mockReturning.mockResolvedValue([MOCK_WORKFLOW]) with
mockReturning.mockResolvedValueOnce([]).mockResolvedValueOnce([MOCK_WORKFLOW])
in this test (and apply the same pattern in the related error/exception tests),
leaving mockSelectWhere, getNextRunDate, executeWorkflow and the POST invocation
unchanged.
In `@apps/web/src/app/api/cron/workflows/route.ts`:
- Around line 124-143: The rejected-handler's DB update call (the await
db.update(...).where(eq(workflows.id, workflow.id)) inside the else branch
handling settled rejections) isn't wrapped in try/catch; wrap that update in a
try/catch so DB failures don't break the outer loop — on catch, log the error
via loggers.api.error including workflow.id and the caught error, push a
descriptive message into the existing errors array (e.g., `${workflow.name}: DB
update failed: ${err.message || String(err)}`), and continue processing the next
claimed workflow; keep nextRunAt calculation and lastRunStatus/lastRunError
behavior unchanged and ensure the original errorMsg (from settled.reason) is
preserved in the errors list when the DB update succeeds.
In `@apps/web/src/app/api/workflows/`[workflowId]/run/__tests__/route.test.ts:
- Around line 101-123: Rename the test fixture variable mockWorkflow to
UPPER_SNAKE_CASE (MOCK_WORKFLOW) to follow the constant naming guideline; update
the declaration (the const mockWorkflow object) to const MOCK_WORKFLOW and
replace every usage/occurrence of mockWorkflow in this test file (all
assertions, function calls, and helper references that consume the fixture) so
the tests compile and reference the new identifier.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/src/app/api/cron/workflows/__tests__/route.test.tsapps/web/src/app/api/cron/workflows/route.tsapps/web/src/app/api/workflows/[workflowId]/__tests__/route.test.tsapps/web/src/app/api/workflows/[workflowId]/route.tsapps/web/src/app/api/workflows/[workflowId]/run/__tests__/route.test.tsapps/web/src/lib/workflows/__tests__/cron-utils.test.tsapps/web/src/lib/workflows/cron-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/lib/workflows/cron-utils.ts
- apps/web/src/lib/workflows/tests/cron-utils.test.ts
Comment on lines
+30
to
+41
| async function getWorkflowWithAuth(workflowId: string, userId: string) { | ||
| const [workflow] = await db | ||
| .select() | ||
| .from(workflows) | ||
| .where(eq(workflows.id, workflowId)); | ||
|
|
||
| if (!workflow) return { error: NextResponse.json({ error: 'Workflow not found' }, { status: 404 }) }; | ||
|
|
||
| const access = await checkDriveAccess(workflow.driveId, userId); | ||
| if (!access.drive) return { error: NextResponse.json({ error: 'Drive not found' }, { status: 404 }) }; | ||
| if (!access.isOwner && !access.isAdmin) { | ||
| return { error: NextResponse.json({ error: 'Only drive owners and admins can manage workflows' }, { status: 403 }) }; |
Contributor
There was a problem hiding this comment.
Use centralized permission helpers for access checks.
Line 38-41 manually checks isOwner/isAdmin; the guideline requires centralized permission functions (e.g., getUserAccessLevel, canUserEditPage) to keep policy logic consistent.
As per coding guidelines, "Use centralized permission functions from @pagespace/lib/permissions (e.g., getUserAccessLevel, canUserEditPage) instead of implementing permission logic locally".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/workflows/`[workflowId]/route.ts around lines 30 - 41,
Replace the local isOwner/isAdmin checks in getWorkflowWithAuth with the
centralized permission helpers from `@pagespace/lib/permissions`: import and call
getUserAccessLevel (or canUserEditPage) with the driveId and userId (or with the
checkDriveAccess result if the helper expects it), use the helper's result to
decide whether the user can manage workflows, and return the same 403
NextResponse when the helper denies edit rights; keep the existing
drive-not-found handling (access.drive) and only substitute the manual
access.isOwner && access.isAdmin logic with the centralized permission check.
Owner
Author
There was a problem hiding this comment.
Acknowledged. The manual isOwner/isAdmin check here is consistent with the pattern used across the other workflow routes in this PR. Migrating to centralized permission helpers (getUserAccessLevel/canUserEditPage) is a good improvement but would be a broader refactor across all workflow routes — out of scope for this PR but worth a follow-up.
Contributor
There was a problem hiding this comment.
@2witstudios, understood — deferring this refactor to maintain consistency within the current PR scope is reasonable.
Would you like me to open a GitHub issue to track migrating all workflow routes to use the centralized permission helpers (getUserAccessLevel/canUserEditPage) from @pagespace/lib/permissions? This would ensure the technical debt is captured for a future PR.
🧠 Learnings used
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : For permission logic, use centralized functions from `pagespace/lib/permissions`: `getUserAccessLevel()`, `canUserEditPage()`
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T20:04:40.910Z
Learning: Applies to **/*.{ts,tsx} : Use centralized permission functions from `pagespace/lib/permissions` (e.g., `getUserAccessLevel`, `canUserEditPage`) instead of implementing permission logic locally
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-20T19:15:28.439Z
Learning: Applies to apps/**/*.ts : Always use centralized permission functions from `pagespace/lib/permissions` for access control logic (e.g., `getUserAccessLevel`, `canUserEditPage`)
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-18T17:02:30.939Z
Learning: Applies to app/**/*.{ts,tsx} : Import and use `getUserAccessLevel()` and `canUserEditPage()` from `pagespace/lib/permissions` for centralized permission logic
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 555
File: apps/processor/src/services/rbac.ts:78-106
Timestamp: 2026-02-12T02:17:26.370Z
Learning: In `apps/processor/src/services/rbac.ts`, the `assertDeleteFileAccess` function intentionally enforces a 403-before-409 error ordering: users without `canDelete` permission on linked pages receive "access denied" (403) before seeing "file is referenced" (409), following the principle of revealing the most actionable error first.
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/**/*.{ts,tsx} : Use Next.js 15 App Router and TypeScript for all routes and components
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-20T17:23:53.244Z
Learning: This is a monorepo using pnpm workspaces with structure: `apps/web` (Next.js frontend/backend), `apps/realtime` (Socket.IO service), `apps/processor` (Express file/OCR pipeline), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-25T00:38:08.816Z
Learning: Applies to **/*.{ts,tsx} : Keep commits and diffs minimal and focused on specific changes
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 258
File: apps/realtime/src/validation.ts:0-0
Timestamp: 2026-01-27T03:45:52.322Z
Learning: Enforce using paralleldrive/cuid2 for ID generation across the TypeScript codebase (not UUIDs). IDs should follow the CUID2 format: lowercase alphanumeric starting with a letter, matching ^[a-z][a-z0-9]{1,31}$ with a maximum length of 32 characters. Audit code paths that generate IDs (e.g., new UUID usages) and replace with cuid2 equivalents; ensure generated IDs are consistently lowercase and validated against the regex, and document any exceptions where IDs may differ in semantic meaning.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 406
File: apps/web/src/app/api/integrations/providers/route.ts:68-77
Timestamp: 2026-02-06T16:00:02.410Z
Learning: Maintain the existing pattern of using validation.error.flatten().fieldErrors across API routes. A future, repo-wide migration to Zod v4's z.flattenError() is planned; when migrating, update all affected routes to use z.flattenError() and adjust error handling accordingly, ensuring the fieldErrors structure remains consistent.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 699
File: apps/marketing/src/app/docs/self-hosting/environment/page.tsx:34-37
Timestamp: 2026-02-18T05:15:03.695Z
Learning: Ensure that cross-subdomain cookie handling uses two environment variables: COOKIE_DOMAIN (server-side, for Set-Cookie headers in server code like apps/web/src/lib/auth/cookie-config.ts) and NEXT_PUBLIC_COOKIE_DOMAIN (client-side, for document.cookie interactions in theme-cookie.ts in both apps/web and apps/marketing). This pattern should be verified across all related files that set or rely on cookie domains to maintain consistent domain scoping and enable cross-subdomain functionality.
Comment on lines
+74
to
+80
| const workflow = result.workflow; | ||
| const body = await request.json(); | ||
| const parsed = updateWorkflowSchema.safeParse(body); | ||
|
|
||
| if (!parsed.success) { | ||
| return NextResponse.json({ error: 'Invalid input', details: parsed.error.flatten() }, { status: 400 }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify existing API routes use the fieldErrors-only shape
echo "=== Checking for flatten().fieldErrors pattern ==="
rg -n "flatten\(\)\.fieldErrors" apps/web/src/app/api --type ts
echo ""
echo "=== Total count ==="
rg "flatten\(\)\.fieldErrors" apps/web/src/app/api --type ts | wc -lRepository: 2witstudios/PageSpace
Length of output: 5327
🏁 Script executed:
#!/bin/bash
# Check the specific file in question
echo "=== Checking target file ==="
sed -n '70,85p' apps/web/src/app/api/workflows/\[workflowId\]/route.tsRepository: 2witstudios/PageSpace
Length of output: 650
Use .fieldErrors to match the established validation error pattern.
Line 79 returns parsed.error.flatten() instead of parsed.error.flatten().fieldErrors, inconsistent with the 35+ other API routes in the codebase that extract field errors only.
♻️ Proposed fix
- return NextResponse.json({ error: 'Invalid input', details: parsed.error.flatten() }, { status: 400 });
+ return NextResponse.json({ error: 'Invalid input', details: parsed.error.flatten().fieldErrors }, { status: 400 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/workflows/`[workflowId]/route.ts around lines 74 - 80,
The response uses parsed.error.flatten() instead of the project's standard
parsed.error.flatten().fieldErrors, causing inconsistent validation error shape;
update the error return in route handler (where
updateWorkflowSchema.safeParse(body) is used and parsed is checked) to return
NextResponse.json({ error: 'Invalid input', details:
parsed.error.flatten().fieldErrors }, { status: 400 }) so the API matches other
routes' fieldErrors format.
Owner
Author
There was a problem hiding this comment.
Good observation. The flatten() shape used here is consistent with the POST route in the same file and other workflow routes. Migrating to flatten().fieldErrors across all routes would be a nice follow-up improvement.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check flatten() usage in workflow routes specifically
echo "=== Workflow routes flatten() usage ==="
rg -n "flatten\(\)" apps/web/src/app/api/workflows --type ts -A 1 -B 1
echo ""
echo "=== Comparing with non-workflow API routes ==="
rg -n "flatten\(\)\.fieldErrors" apps/web/src/app/api --type ts | grep -v workflows | head -20
Length of output: 3580
@2witstudios, you're right—the two workflow routes are internally consistent with each other. The script confirms both use flatten() without .fieldErrors, while the rest of the API routes (~35+ files) use flatten().fieldErrors.
A follow-up PR to align the workflow routes with the broader codebase pattern sounds like a good plan. Thanks for the context!
🧠 Learnings used
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : For permission logic, use centralized functions from `pagespace/lib/permissions`: `getUserAccessLevel()`, `canUserEditPage()`
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : In Route Handlers, get request body with `const body = await request.json();`
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 406
File: apps/web/src/app/api/integrations/providers/route.ts:68-77
Timestamp: 2026-02-06T16:00:02.410Z
Learning: Maintain the existing pattern of using validation.error.flatten().fieldErrors across API routes. A future, repo-wide migration to Zod v4's z.flattenError() is planned; when migrating, update all affected routes to use z.flattenError() and adjust error handling accordingly, ensuring the fieldErrors structure remains consistent.
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/**/*.{ts,tsx} : Use Next.js 15 App Router and TypeScript for all routes and components
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-20T17:23:53.244Z
Learning: This is a monorepo using pnpm workspaces with structure: `apps/web` (Next.js frontend/backend), `apps/realtime` (Socket.IO service), `apps/processor` (Express file/OCR pipeline), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-25T00:38:08.816Z
Learning: Applies to **/*.{ts,tsx} : Keep commits and diffs minimal and focused on specific changes
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 258
File: apps/realtime/src/validation.ts:0-0
Timestamp: 2026-01-27T03:45:52.322Z
Learning: Enforce using paralleldrive/cuid2 for ID generation across the TypeScript codebase (not UUIDs). IDs should follow the CUID2 format: lowercase alphanumeric starting with a letter, matching ^[a-z][a-z0-9]{1,31}$ with a maximum length of 32 characters. Audit code paths that generate IDs (e.g., new UUID usages) and replace with cuid2 equivalents; ensure generated IDs are consistently lowercase and validated against the regex, and document any exceptions where IDs may differ in semantic meaning.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 699
File: apps/marketing/src/app/docs/self-hosting/environment/page.tsx:34-37
Timestamp: 2026-02-18T05:15:03.695Z
Learning: Ensure that cross-subdomain cookie handling uses two environment variables: COOKIE_DOMAIN (server-side, for Set-Cookie headers in server code like apps/web/src/lib/auth/cookie-config.ts) and NEXT_PUBLIC_COOKIE_DOMAIN (client-side, for document.cookie interactions in theme-cookie.ts in both apps/web and apps/marketing). This pattern should be verified across all related files that set or rely on cookie domains to maintain consistent domain scoping and enable cross-subdomain functionality.
…rejection, deferred triggers - Add eligibility predicates (isEnabled, triggerType, nextRunAt) to cron batch claim WHERE clause to prevent claiming workflows that changed between discovery and claim - Replace unconditional event workflow UPDATE with atomic claim via UPDATE...WHERE lastRunStatus != 'running' + .returning() to prevent double-execution - Add eq(pages.isTrashed, false) to agent page validation in both POST and PATCH workflow routes to reject trashed AI agents - Propagate DeferredWorkflowTrigger from logRollbackActivity instead of discarding it, and fire deferred triggers after transaction commit in all rollback callers - Update event-trigger test mocks to support atomic claim pattern (ne export, .returning()) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
Review Fixes (62ca387)Addressed 4 code review findings: [P1] Cron batch claim eligibility predicates — [P1] Atomic event workflow claim — [P2] Trashed agent page rejection — [P2] Deferred workflow trigger propagation — All 118 targeted tests pass (88 workflow + 30 activity logger). |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/web/src/app/api/cron/workflows/route.ts (1)
134-137: SamecronExpression!inconsistency as inexecuteOne.The rejection handler has the same
workflow.cronExpression!non-null assertion (line 136) without an explicit null guard, identical to the issue at lines 69–73. Apply the same refactor here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/cron/workflows/route.ts` around lines 134 - 137, The code calls getNextRunDate with workflow.cronExpression using a non-null assertion (workflow.cronExpression!) inside the catch block — mirror the same fix applied to executeOne: guard against null/undefined before calling getNextRunDate by checking if workflow.cronExpression is truthy (and optionally validate timezone), then call getNextRunDate(workflow.cronExpression, workflow.timezone) inside the try/catch; leave nextRunAt undefined when cronExpression is missing or when the call throws. Ensure you update the block that declares nextRunAt and the try/catch around getNextRunDate accordingly.
🧹 Nitpick comments (1)
apps/web/src/app/api/cron/workflows/route.ts (1)
68-73: Prefer explicit null guard over!assertion forcronExpression.
workflow.cronExpression!relies on the surroundingtry/catchto silently absorb theTypeErrorwhencronExpressionis actuallynull(the DB schema has it nullable). The same file already uses the cleaner explicit guard pattern at line 36 (if (wf.cronExpression)). Consistently using that avoids surprising control-flow-via-exception and is more self-documenting.♻️ Proposed refactor
- let nextRunAt: Date | undefined; - try { - nextRunAt = getNextRunDate(workflow.cronExpression!, workflow.timezone); - } catch { - loggers.api.error(`Workflow cron: Failed to compute nextRunAt for ${workflow.id}`); - } + let nextRunAt: Date | undefined; + if (workflow.cronExpression) { + try { + nextRunAt = getNextRunDate(workflow.cronExpression, workflow.timezone); + } catch { + loggers.api.error(`Workflow cron: Failed to compute nextRunAt for ${workflow.id}`); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/cron/workflows/route.ts` around lines 68 - 73, Replace the non-null assertion on workflow.cronExpression when calling getNextRunDate with an explicit null/undefined guard: check workflow.cronExpression (and optionally workflow.timezone) before invoking getNextRunDate(workflow.cronExpression, workflow.timezone), and only call getNextRunDate when the value exists; if it doesn't exist, skip the call and log or handle accordingly instead of relying on a try/catch to swallow a TypeError — update the block around nextRunAt, keeping the existing loggers.api.error message for failures that occur during getNextRunDate(workflow.cronExpression, workflow.timezone) and still include workflow.id in logs.
🤖 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/web/src/app/api/cron/workflows/route.ts`:
- Around line 139-146: The DB update in the error path (inside executeOne /
executeWorkflow) currently omits lastRunDurationMs so it remains stale; modify
the update in the error handler that calls db.update(workflows).set(...) to
explicitly clear or reset lastRunDurationMs (e.g., set to null or 0) along with
lastRunStatus and lastRunError, and keep the existing nextRunAt conditional
logic so the field is not left with the previous run's value.
- Around line 159-161: The catch block in route.ts is unsafely casting the
caught value with "error as Error"; change it to safely handle unknown by
checking type before logging: use "if (error instanceof Error)" to log
error.message and error.stack via loggers.api.error (or otherwise stringify the
value with JSON.stringify for non-Error values) so the logger never relies on an
invalid cast, and keep the NextResponse.json({ error: 'Workflow cron job failed'
}, { status: 500 }) return but ensure the logged payload is the normalized
message/stack or stringified error when calling loggers.api.error.
In `@apps/web/src/app/api/workflows/route.ts`:
- Around line 70-75: The response currently returns parsed.error.flatten() (full
flatten payload); change it to return only the fieldErrors to match existing API
convention: when createWorkflowSchema.safeParse(body) yields !parsed.success,
call NextResponse.json with { error: 'Invalid input', fieldErrors:
parsed.error.flatten().fieldErrors } and status 400 so the route.ts response
shape aligns with other endpoints that expect fieldErrors (use the existing
variables body, parsed and createWorkflowSchema.safeParse).
- Around line 10-32: Rename the module-level Zod schema constants
eventTriggerSchema and createWorkflowSchema to UPPER_SNAKE_CASE (e.g.,
EVENT_TRIGGER_SCHEMA, CREATE_WORKFLOW_SCHEMA) throughout the file; update every
reference to those identifiers (including where they are exported or used in
request validation) to the new names to keep the module consistent with the
constant naming guideline. Ensure the refined schema logic and all chained calls
(.refine, .default, .optional, etc.) remain unchanged when renaming.
In `@apps/web/src/lib/workflows/event-trigger.ts`:
- Around line 154-158: Rename the fixed numeric constant maxContextFieldLength
to UPPER_SNAKE_CASE (MAX_CONTEXT_FIELD_LENGTH) and update any references,
including the default parameter in the truncate function signature
(truncate(value: unknown, max = MAX_CONTEXT_FIELD_LENGTH): string => { ... }),
so the constant follows the coding guideline while preserving existing behavior;
ensure only the identifier changes and no other logic is modified.
---
Duplicate comments:
In `@apps/web/src/app/api/cron/workflows/route.ts`:
- Around line 134-137: The code calls getNextRunDate with
workflow.cronExpression using a non-null assertion (workflow.cronExpression!)
inside the catch block — mirror the same fix applied to executeOne: guard
against null/undefined before calling getNextRunDate by checking if
workflow.cronExpression is truthy (and optionally validate timezone), then call
getNextRunDate(workflow.cronExpression, workflow.timezone) inside the try/catch;
leave nextRunAt undefined when cronExpression is missing or when the call
throws. Ensure you update the block that declares nextRunAt and the try/catch
around getNextRunDate accordingly.
---
Nitpick comments:
In `@apps/web/src/app/api/cron/workflows/route.ts`:
- Around line 68-73: Replace the non-null assertion on workflow.cronExpression
when calling getNextRunDate with an explicit null/undefined guard: check
workflow.cronExpression (and optionally workflow.timezone) before invoking
getNextRunDate(workflow.cronExpression, workflow.timezone), and only call
getNextRunDate when the value exists; if it doesn't exist, skip the call and log
or handle accordingly instead of relying on a try/catch to swallow a TypeError —
update the block around nextRunAt, keeping the existing loggers.api.error
message for failures that occur during getNextRunDate(workflow.cronExpression,
workflow.timezone) and still include workflow.id in logs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/src/app/api/activities/[activityId]/rollback/route.tsapps/web/src/app/api/cron/workflows/route.tsapps/web/src/app/api/workflows/[workflowId]/route.tsapps/web/src/app/api/workflows/route.tsapps/web/src/lib/workflows/__tests__/event-trigger.test.tsapps/web/src/lib/workflows/event-trigger.tsapps/web/src/services/api/ai-undo-service.tsapps/web/src/services/api/rollback-service.tsapps/web/src/services/api/rollback-to-point-service.tspackages/lib/src/monitoring/activity-logger.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/api/workflows/[workflowId]/route.ts
Comment on lines
+139
to
+146
| await db | ||
| .update(workflows) | ||
| .set({ | ||
| lastRunStatus: 'error', | ||
| lastRunError: errorMsg, | ||
| ...(nextRunAt ? { nextRunAt } : {}), | ||
| }) | ||
| .where(eq(workflows.id, workflow.id)); |
Contributor
There was a problem hiding this comment.
Rejection handler leaves lastRunDurationMs stale.
When executeOne rejects (i.e., executeWorkflow itself throws), the error-path DB update omits lastRunDurationMs. The field will retain whatever value it held from the previous run instead of being reset/cleared, which can mislead operators inspecting run history.
🛠️ Suggested fix
await db
.update(workflows)
.set({
lastRunStatus: 'error',
lastRunError: errorMsg,
+ lastRunDurationMs: null,
...(nextRunAt ? { nextRunAt } : {}),
})
.where(eq(workflows.id, workflow.id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/cron/workflows/route.ts` around lines 139 - 146, The DB
update in the error path (inside executeOne / executeWorkflow) currently omits
lastRunDurationMs so it remains stale; modify the update in the error handler
that calls db.update(workflows).set(...) to explicitly clear or reset
lastRunDurationMs (e.g., set to null or 0) along with lastRunStatus and
lastRunError, and keep the existing nextRunAt conditional logic so the field is
not left with the previous run's value.
Comment on lines
+159
to
+161
| } catch (error) { | ||
| loggers.api.error('Workflow cron error:', error as Error); | ||
| return NextResponse.json({ error: 'Workflow cron job failed' }, { status: 500 }); |
Contributor
There was a problem hiding this comment.
error as Error is an unsafe type cast.
The outer catch block binds error as unknown. Casting it directly with as Error bypasses the type system — if the thrown value is a plain string or object, the cast silently succeeds but the logger may behave unexpectedly.
🛠️ Suggested fix
- loggers.api.error('Workflow cron error:', error as Error);
+ loggers.api.error('Workflow cron error:', {
+ error: error instanceof Error ? error.message : String(error),
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/cron/workflows/route.ts` around lines 159 - 161, The
catch block in route.ts is unsafely casting the caught value with "error as
Error"; change it to safely handle unknown by checking type before logging: use
"if (error instanceof Error)" to log error.message and error.stack via
loggers.api.error (or otherwise stringify the value with JSON.stringify for
non-Error values) so the logger never relies on an invalid cast, and keep the
NextResponse.json({ error: 'Workflow cron job failed' }, { status: 500 }) return
but ensure the logged payload is the normalized message/stack or stringified
error when calling loggers.api.error.
Comment on lines
+10
to
+32
| const eventTriggerSchema = z.object({ | ||
| operation: z.string().min(1), | ||
| resourceType: z.string().min(1), | ||
| }); | ||
|
|
||
| const createWorkflowSchema = z.object({ | ||
| driveId: z.string().min(1), | ||
| name: z.string().min(1).max(200), | ||
| agentPageId: z.string().min(1), | ||
| prompt: z.string().min(1), | ||
| contextPageIds: z.array(z.string()).default([]), | ||
| triggerType: z.enum(['cron', 'event']).default('cron'), | ||
| cronExpression: z.string().min(1).optional(), | ||
| timezone: z.string().default('UTC'), | ||
| isEnabled: z.boolean().default(true), | ||
| eventTriggers: z.array(eventTriggerSchema).optional(), | ||
| watchedFolderIds: z.array(z.string()).optional(), | ||
| eventDebounceSecs: z.number().int().min(5).max(3600).default(30), | ||
| }).refine(data => { | ||
| if (data.triggerType === 'cron') return !!data.cronExpression; | ||
| if (data.triggerType === 'event') return data.eventTriggers && data.eventTriggers.length > 0; | ||
| return true; | ||
| }, { message: 'Cron workflows need cronExpression; event workflows need eventTriggers' }); |
Contributor
There was a problem hiding this comment.
Rename schema constants to UPPER_SNAKE_CASE.
These are module-level constants and should follow the constant naming rule.
♻️ Suggested rename
-const eventTriggerSchema = z.object({
+const EVENT_TRIGGER_SCHEMA = z.object({
operation: z.string().min(1),
resourceType: z.string().min(1),
});
-const createWorkflowSchema = z.object({
+const CREATE_WORKFLOW_SCHEMA = z.object({
driveId: z.string().min(1),
name: z.string().min(1).max(200),
agentPageId: z.string().min(1),
prompt: z.string().min(1),
contextPageIds: z.array(z.string()).default([]),
triggerType: z.enum(['cron', 'event']).default('cron'),
cronExpression: z.string().min(1).optional(),
timezone: z.string().default('UTC'),
isEnabled: z.boolean().default(true),
- eventTriggers: z.array(eventTriggerSchema).optional(),
+ eventTriggers: z.array(EVENT_TRIGGER_SCHEMA).optional(),
watchedFolderIds: z.array(z.string()).optional(),
eventDebounceSecs: z.number().int().min(5).max(3600).default(30),
}).refine(/* ... */);
-const parsed = createWorkflowSchema.safeParse(body);
+const parsed = CREATE_WORKFLOW_SCHEMA.safeParse(body);As per coding guidelines "Use UPPER_SNAKE_CASE for constants".
📝 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.
Suggested change
| const eventTriggerSchema = z.object({ | |
| operation: z.string().min(1), | |
| resourceType: z.string().min(1), | |
| }); | |
| const createWorkflowSchema = z.object({ | |
| driveId: z.string().min(1), | |
| name: z.string().min(1).max(200), | |
| agentPageId: z.string().min(1), | |
| prompt: z.string().min(1), | |
| contextPageIds: z.array(z.string()).default([]), | |
| triggerType: z.enum(['cron', 'event']).default('cron'), | |
| cronExpression: z.string().min(1).optional(), | |
| timezone: z.string().default('UTC'), | |
| isEnabled: z.boolean().default(true), | |
| eventTriggers: z.array(eventTriggerSchema).optional(), | |
| watchedFolderIds: z.array(z.string()).optional(), | |
| eventDebounceSecs: z.number().int().min(5).max(3600).default(30), | |
| }).refine(data => { | |
| if (data.triggerType === 'cron') return !!data.cronExpression; | |
| if (data.triggerType === 'event') return data.eventTriggers && data.eventTriggers.length > 0; | |
| return true; | |
| }, { message: 'Cron workflows need cronExpression; event workflows need eventTriggers' }); | |
| const EVENT_TRIGGER_SCHEMA = z.object({ | |
| operation: z.string().min(1), | |
| resourceType: z.string().min(1), | |
| }); | |
| const CREATE_WORKFLOW_SCHEMA = z.object({ | |
| driveId: z.string().min(1), | |
| name: z.string().min(1).max(200), | |
| agentPageId: z.string().min(1), | |
| prompt: z.string().min(1), | |
| contextPageIds: z.array(z.string()).default([]), | |
| triggerType: z.enum(['cron', 'event']).default('cron'), | |
| cronExpression: z.string().min(1).optional(), | |
| timezone: z.string().default('UTC'), | |
| isEnabled: z.boolean().default(true), | |
| eventTriggers: z.array(EVENT_TRIGGER_SCHEMA).optional(), | |
| watchedFolderIds: z.array(z.string()).optional(), | |
| eventDebounceSecs: z.number().int().min(5).max(3600).default(30), | |
| }).refine(data => { | |
| if (data.triggerType === 'cron') return !!data.cronExpression; | |
| if (data.triggerType === 'event') return data.eventTriggers && data.eventTriggers.length > 0; | |
| return true; | |
| }, { message: 'Cron workflows need cronExpression; event workflows need eventTriggers' }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/workflows/route.ts` around lines 10 - 32, Rename the
module-level Zod schema constants eventTriggerSchema and createWorkflowSchema to
UPPER_SNAKE_CASE (e.g., EVENT_TRIGGER_SCHEMA, CREATE_WORKFLOW_SCHEMA) throughout
the file; update every reference to those identifiers (including where they are
exported or used in request validation) to the new names to keep the module
consistent with the constant naming guideline. Ensure the refined schema logic
and all chained calls (.refine, .default, .optional, etc.) remain unchanged when
renaming.
Comment on lines
+70
to
+75
| const body = await request.json(); | ||
| const parsed = createWorkflowSchema.safeParse(body); | ||
|
|
||
| if (!parsed.success) { | ||
| return NextResponse.json({ error: 'Invalid input', details: parsed.error.flatten() }, { status: 400 }); | ||
| } |
Contributor
There was a problem hiding this comment.
Return only fieldErrors to keep validation error shape consistent.
The API convention elsewhere is to expose fieldErrors instead of the full flatten payload.
✅ Suggested adjustment
- return NextResponse.json({ error: 'Invalid input', details: parsed.error.flatten() }, { status: 400 });
+ return NextResponse.json(
+ { error: 'Invalid input', details: parsed.error.flatten().fieldErrors },
+ { status: 400 }
+ );Based on learnings "Maintain the existing pattern of using validation.error.flatten().fieldErrors across API routes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/workflows/route.ts` around lines 70 - 75, The response
currently returns parsed.error.flatten() (full flatten payload); change it to
return only the fieldErrors to match existing API convention: when
createWorkflowSchema.safeParse(body) yields !parsed.success, call
NextResponse.json with { error: 'Invalid input', fieldErrors:
parsed.error.flatten().fieldErrors } and status 400 so the route.ts response
shape aligns with other endpoints that expect fieldErrors (use the existing
variables body, parsed and createWorkflowSchema.safeParse).
Comment on lines
+154
to
+158
| const maxContextFieldLength = 200; | ||
|
|
||
| const truncate = (value: unknown, max = maxContextFieldLength): string => { | ||
| const str = String(value ?? ''); | ||
| return str.length > max ? `${str.slice(0, max)}...` : str; |
Contributor
There was a problem hiding this comment.
Rename constant to UPPER_SNAKE_CASE.
This is a fixed numeric constant and should use constant casing.
♻️ Suggested rename
-const maxContextFieldLength = 200;
+const MAX_CONTEXT_FIELD_LENGTH = 200;
-const truncate = (value: unknown, max = maxContextFieldLength): string => {
+const truncate = (value: unknown, max = MAX_CONTEXT_FIELD_LENGTH): string => {As per coding guidelines "Use UPPER_SNAKE_CASE for constants".
📝 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.
Suggested change
| const maxContextFieldLength = 200; | |
| const truncate = (value: unknown, max = maxContextFieldLength): string => { | |
| const str = String(value ?? ''); | |
| return str.length > max ? `${str.slice(0, max)}...` : str; | |
| const MAX_CONTEXT_FIELD_LENGTH = 200; | |
| const truncate = (value: unknown, max = MAX_CONTEXT_FIELD_LENGTH): string => { | |
| const str = String(value ?? ''); | |
| return str.length > max ? `${str.slice(0, max)}...` : str; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/workflows/event-trigger.ts` around lines 154 - 158, Rename
the fixed numeric constant maxContextFieldLength to UPPER_SNAKE_CASE
(MAX_CONTEXT_FIELD_LENGTH) and update any references, including the default
parameter in the truncate function signature (truncate(value: unknown, max =
MAX_CONTEXT_FIELD_LENGTH): string => { ... }), so the constant follows the
coding guideline while preserving existing behavior; ensure only the identifier
changes and no other logic is modified.
This was referenced Apr 10, 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
Adds event-triggered and cron-scheduled workflow automation system (Epics 8-13), with 15 bug fixes identified during code review:
P1 — Data Correctness:
executeWorkflowthrows in manual run routetriggerType='cron'— prevents incorrectly resetting long-running event workflowsP2 — Input Validation & Robustness:
triggerType === 'cron'guard onnextRunAt— prevents phantom scheduled runs for event workflowsgetNextRunDate— returns 400 instead of 500 on invalid inputcronExpressionon cron workflow PATCH — prevents invalid DB stateeventTriggerson event workflow PATCH — prevents workflows with no trigger definitionsgetNextRunDate()with try/catch in cron route — prevents cascading failuresFixes from review feedback:
forEach(t => t())withfor...ofto avoid implicit returnMAX_CONCURRENT_WORKFLOWS,MOCK_WORKFLOW)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit