fix(workflows): guard workflow execution polling state#1599
Conversation
📝 WalkthroughWalkthroughCentralize transformation of run+events into frontend state with buildWorkflowRunQueryData, update WorkflowExecution to use it and guard polling access, add unit tests for builder behaviors, and include the component test in the package test script. ChangesWorkflow run query builder & consumer
Sequence DiagramsequenceDiagram
participant Client
participant WorkflowExecution
participant REST_API
participant buildWorkflowRunQueryData
Client->>WorkflowExecution: open execution page (runId)
WorkflowExecution->>REST_API: fetch run + events
REST_API->>WorkflowExecution: run + events response
WorkflowExecution->>buildWorkflowRunQueryData: buildWorkflowRunQueryData(runId, data)
buildWorkflowRunQueryData->>WorkflowExecution: WorkflowRunQueryData (workflowState, events, platform ids)
WorkflowExecution->>Client: render execution UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
08783c5 to
b718114
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/web/src/components/workflows/WorkflowExecution.tsx (2)
61-61: 💤 Low valueConsider a named
interfaceand tighter prop type forStatusBadge.The inline
{ status: string }type is both broader than needed and not a namedinterface. The coding guideline prefersinterfacefor object shapes, and narrowing toWorkflowRunStatuswould catch accidental misuse at the call site.♻️ Proposed refactor
+interface StatusBadgeProps { + status: WorkflowRunStatus; +} + -function StatusBadge({ status }: { status: string }): React.ReactElement { +function StatusBadge({ status }: StatusBadgeProps): React.ReactElement {As per coding guidelines: "prefer
interfacefor defining object shapes rather thantype" and "Use strict TypeScript configuration with complete type annotations on all functions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/components/workflows/WorkflowExecution.tsx` at line 61, Replace the inline prop type on StatusBadge with a named interface and tighten the status type to the domain union: define an interface (e.g. StatusBadgeProps) with status: WorkflowRunStatus and update the StatusBadge signature to function StatusBadge(props: StatusBadgeProps): React.ReactElement (or destructure as function StatusBadge({ status }: StatusBadgeProps)). Ensure WorkflowRunStatus is the correct union/enum imported or declared in the same module.
212-216: 💤 Low valuePolling guard is correct for TanStack Query v5.
In v5,
refetchIntervaldropped the leadingdataparameter and now receives only thequeryobject, with data accessible viaquery.state.data. The optional chain ondatais necessary (undefined before the first successful fetch), and the extra?.onworkflowStateis redundant from TypeScript's perspective but harmless as a runtime guard.One minor point: the
queryparameter has no explicit type annotation. Under the strict-typing guideline this should be(query: Query<WorkflowRunQueryData, Error, WorkflowRunQueryData, ['workflowRun', string]>): number | false => ..., importingQueryfrom@tanstack/react-query. In practice TypeScript infers this correctly from theuseQuerycontext, so this is a style nit only.As per coding guidelines: "Use strict TypeScript configuration with complete type annotations on all functions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/components/workflows/WorkflowExecution.tsx` around lines 212 - 216, The refetchInterval callback is missing an explicit type for its parameter; update the arrow function signature for refetchInterval to annotate the parameter as Query<WorkflowRunQueryData, Error, WorkflowRunQueryData, ['workflowRun', string]> (import Query from `@tanstack/react-query` and use the existing WorkflowRunQueryData type) so the function becomes typed correctly while keeping the existing access to query.state.data and the terminal-status guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/web/src/components/workflows/WorkflowExecution.tsx`:
- Line 61: Replace the inline prop type on StatusBadge with a named interface
and tighten the status type to the domain union: define an interface (e.g.
StatusBadgeProps) with status: WorkflowRunStatus and update the StatusBadge
signature to function StatusBadge(props: StatusBadgeProps): React.ReactElement
(or destructure as function StatusBadge({ status }: StatusBadgeProps)). Ensure
WorkflowRunStatus is the correct union/enum imported or declared in the same
module.
- Around line 212-216: The refetchInterval callback is missing an explicit type
for its parameter; update the arrow function signature for refetchInterval to
annotate the parameter as Query<WorkflowRunQueryData, Error,
WorkflowRunQueryData, ['workflowRun', string]> (import Query from
`@tanstack/react-query` and use the existing WorkflowRunQueryData type) so the
function becomes typed correctly while keeping the existing access to
query.state.data and the terminal-status guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dddc7ef7-7d69-4f91-b36d-ad356c80ae22
📒 Files selected for processing (1)
packages/web/src/components/workflows/WorkflowExecution.tsx
Review SummaryVerdict: blocking-issues Your PR fix for React Query polling edge cases in Blocking issues
Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality. |
b718114 to
77beac4
Compare
Review SummaryVerdict: minor-fixes-needed The refactoring cleanly extracts Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage. |
Review SummaryVerdict: minor-fixes-needed The extraction of Blocking issues(none — no CRITICAL findings) Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality. |
77beac4 to
01257d6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/lib/workflow-run-query.test.ts (1)
5-5: ⚡ Quick winConsider resetting the global event counter per test.
The global
eventSequencevariable is mutated across tests (incremented on line 29), which reduces test isolation. While the tests don't currently assert on specific ID values, this pattern could cause order-dependent behavior or confusion during debugging.♻️ Suggested fix
let eventSequence = 0; +describe('buildWorkflowRunQueryData', () => { + beforeEach(() => { + eventSequence = 0; + }); +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/workflow-run-query.test.ts` at line 5, The file-level mutable variable eventSequence is shared across tests and should be reset before each test to preserve isolation; add a beforeEach hook in workflow-run-query.test.ts that sets eventSequence = 0 so every test starts with a fresh counter, ensuring mocks that use eventSequence++ (the incrementing usage) produce predictable IDs per test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/web/src/lib/workflow-run-query.test.ts`:
- Line 5: The file-level mutable variable eventSequence is shared across tests
and should be reset before each test to preserve isolation; add a beforeEach
hook in workflow-run-query.test.ts that sets eventSequence = 0 so every test
starts with a fresh counter, ensuring mocks that use eventSequence++ (the
incrementing usage) produce predictable IDs per test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d834c0a6-26dc-4be6-a4a9-b458146e1514
📒 Files selected for processing (5)
packages/web/package.jsonpackages/web/src/components/workflows/WorkflowExecution.test.tsxpackages/web/src/components/workflows/WorkflowExecution.tsxpackages/web/src/lib/workflow-run-query.test.tspackages/web/src/lib/workflow-run-query.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/src/lib/workflow-run-query.ts
- packages/web/src/components/workflows/WorkflowExecution.tsx
|
Thanks for the thorough review @Wirasm! You're right that the test coverage for |
|
@$guanghuang related to #1578 — overlapping area or partial fix. |
|
@$guanghuang related to #1598 — overlapping area or partial fix. |
|
@$guanghuang related to #1578 — overlapping area or partial fix. |
|
@$guanghuang related to #1598 — overlapping area or partial fix. |
Summary
workflowState.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
WorkflowExecutiongetWorkflowRunWorkflowExecutionrefetchIntervalWorkflowExecutionLabel Snapshot
risk: lowsize: XSwebweb:workflow-executionChange Metadata
bugwebLinked Issue
Validation Evidence (required)
Commands and result summary:
WorkflowExecution.tsxpolling status access; the code now guards that path.bun run validatewas skipped because this is a one-line web route guard and focused type-check/test coverage was run.Security Impact (required)
No)No)No)No)Yes, describe risk and mitigation: Not applicable.Compatibility / Migration
Yes)No)No)Human Verification (required)
What was personally validated beyond CI:
refetchInterval; missingrunresponse now throws a normal query error instead of causing nested property access failures.Side Effects / Blast Radius (required)
Rollback Plan (required)
Cannot read properties of undefined (reading 'status').Risks and Mitigations
runcould still prevent the page from rendering that run.data.runand crashing unpredictably.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores