Conversation
- Add initialStatus prop to JobStatus component - Initialize pollStatus from initialStatus instead of hardcoded "pending" - Pass initialStatus from App.tsx for selectedJob and uploadResult - Add tests for initialStatus prop behavior This prevents unnecessary polling when a completed job is selected from the Jobs view. Previously, JobStatus always initialized pollStatus to "pending" on mount, causing polling to start even for terminal states. Fixes #363
Contributor
WalkthroughIntroduces an optional Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web-ui/src/components/JobStatus.test.tsx (2)
500-504: Consider using fake timers instead of real setTimeout.The 100ms delay with
setTimeoutcould cause flaky tests in CI environments. Vitest's fake timers provide more reliable timing control:♻️ Suggested improvement
+ beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); it("should NOT poll when initialStatus is 'completed'", async () => { // ... setup ... render(<JobStatus jobId="job-123" initialStatus="completed" />); expect(screen.getByText(/completed/i)).toBeInTheDocument(); - await new Promise((resolve) => setTimeout(resolve, 100)); + await vi.advanceTimersByTimeAsync(100); expect(mockGetJob).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/JobStatus.test.tsx` around lines 500 - 504, Replace the real 100ms delay in JobStatus.test.tsx with Vitest fake timers: wrap the test or the specific section with vi.useFakeTimers(), replace the await new Promise((resolve) => setTimeout(resolve, 100)) with advancing the fake timers (e.g., vi.advanceTimersByTime or vi.runAllTimers) so that mockGetJob is deterministically not called, then restore timers with vi.useRealTimers() (or vi.resetAllTimers()) after the assertion; reference the test’s mockGetJob expectation and the setTimeout call to locate where to apply vi.useFakeTimers()/vi.advanceTimersByTime()/vi.useRealTimers().
475-505: Consider adding test for WebSocket connection prevention.The tests verify HTTP polling is prevented, but the
useJobProgresshook is still called unconditionally. If you address the WebSocket connection issue inJobStatus.tsx, consider adding a test to verifyuseJobProgressreceivesnullfor terminal states:it("should not connect WebSocket when initialStatus is terminal", () => { render(<JobStatus jobId="job-123" initialStatus="completed" />); // Verify useJobProgress was called with null (no connection) expect(mockUseJobProgress).toHaveBeenCalledWith(null); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/JobStatus.test.tsx` around lines 475 - 505, The useJobProgress hook is being invoked unconditionally causing unwanted WebSocket connections even when initialStatus is a terminal state; update JobStatus to determine terminal statuses (e.g., "completed", "failed") and call useJobProgress with null instead of the jobId when initialStatus is terminal (so change the hook invocation in JobStatus from useJobProgress(jobId) to useJobProgress(initialStatusIsTerminal ? null : jobId)), and add a test asserting mockUseJobProgress was called with null when rendering <JobStatus jobId="job-123" initialStatus="completed" /> to prevent WebSocket connection.web-ui/src/components/JobStatus.tsx (1)
13-20: WebSocket still connects for terminal jobs.While the
initialStatusfix correctly prevents HTTP polling for completed/failed jobs, theuseJobProgress(jobId)hook is called unconditionally and will still open a WebSocket connection. According to the context snippet fromuseJobProgress.ts(lines 64-194),connect()is called immediately whenjobIdchanges, with no guard for terminal states.Consider passing
initialStatusto the hook or conditionally calling it:- const { - progress: wsProgress, - status: wsStatus, - error: wsError, - isConnected, - } = useJobProgress(jobId); + const skipWebSocket = initialStatus === "completed" || initialStatus === "failed"; + const { + progress: wsProgress, + status: wsStatus, + error: wsError, + isConnected, + } = useJobProgress(skipWebSocket ? null : jobId);This prevents unnecessary WebSocket connections for jobs that are already in a terminal state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/JobStatus.tsx` around lines 13 - 20, The JobStatus component currently calls useJobProgress(jobId) unconditionally which lets the hook (whose connect() runs when jobId changes) open a WebSocket for terminal jobs; update JobStatus to either pass the initialStatus into useJobProgress (e.g., useJobProgress(jobId, initialStatus) and have the hook skip connect() when initialStatus is terminal) or only call useJobProgress when initialStatus is not a terminal state (e.g., check initialStatus/terminal enum before invoking useJobProgress) so that connect() in the hook is never invoked for completed/failed jobs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/src/components/JobStatus.test.tsx`:
- Around line 500-504: Replace the real 100ms delay in JobStatus.test.tsx with
Vitest fake timers: wrap the test or the specific section with
vi.useFakeTimers(), replace the await new Promise((resolve) =>
setTimeout(resolve, 100)) with advancing the fake timers (e.g.,
vi.advanceTimersByTime or vi.runAllTimers) so that mockGetJob is
deterministically not called, then restore timers with vi.useRealTimers() (or
vi.resetAllTimers()) after the assertion; reference the test’s mockGetJob
expectation and the setTimeout call to locate where to apply
vi.useFakeTimers()/vi.advanceTimersByTime()/vi.useRealTimers().
- Around line 475-505: The useJobProgress hook is being invoked unconditionally
causing unwanted WebSocket connections even when initialStatus is a terminal
state; update JobStatus to determine terminal statuses (e.g., "completed",
"failed") and call useJobProgress with null instead of the jobId when
initialStatus is terminal (so change the hook invocation in JobStatus from
useJobProgress(jobId) to useJobProgress(initialStatusIsTerminal ? null :
jobId)), and add a test asserting mockUseJobProgress was called with null when
rendering <JobStatus jobId="job-123" initialStatus="completed" /> to prevent
WebSocket connection.
In `@web-ui/src/components/JobStatus.tsx`:
- Around line 13-20: The JobStatus component currently calls
useJobProgress(jobId) unconditionally which lets the hook (whose connect() runs
when jobId changes) open a WebSocket for terminal jobs; update JobStatus to
either pass the initialStatus into useJobProgress (e.g., useJobProgress(jobId,
initialStatus) and have the hook skip connect() when initialStatus is terminal)
or only call useJobProgress when initialStatus is not a terminal state (e.g.,
check initialStatus/terminal enum before invoking useJobProgress) so that
connect() in the hook is never invoked for completed/failed jobs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d4cdad5-adfd-4178-8757-8dd542aa2131
📒 Files selected for processing (3)
web-ui/src/App.tsxweb-ui/src/components/JobStatus.test.tsxweb-ui/src/components/JobStatus.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/design/frontend-state-flow.md (2)
91-91: Use “auto-populated” for compound consistency.Line 91 can be normalised to the preferred compound form reported by the linter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/frontend-state-flow.md` at line 91, Change the phrase on line 91 to use the linter-preferred compound form: replace "auto-populated" with "auto populated" in the text referencing App.selectedTools so the sentence reads "App.selectedTools auto populated with first tool" (update the documentation string that mentions App.selectedTools to match the preferred compound style).
48-54: Add language identifiers to fenced code blocks (markdownlint MD040).Multiple fenced blocks are missing a language tag. Please label them (
text,typescript, etc.) to satisfy lint and improve readability.Also applies to: 72-80, 102-108, 202-213, 237-249, 275-281, 306-312, 338-350, 353-361, 440-474
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/frontend-state-flow.md` around lines 48 - 54, The fenced code blocks in docs/design/frontend-state-flow.md (e.g., the PluginSelector mount block showing "useEffect([]) → apiClient.getPlugins()" and similar blocks at the other ranges) lack language identifiers and trigger markdownlint MD040; update each triple-backtick fence to include an appropriate language tag (for instance use ```text for plain flow diagrams or ```typescript/```json where the block contains code/config) so all blocks (including those around lines 72-80, 102-108, 202-213, 237-249, 275-281, 306-312, 338-350, 353-361, 440-474) have language specifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/frontend-state-flow.md`:
- Around line 36-38: The doc currently states JobStatus.pollStatus is always
initialized to "pending" which is outdated after the initialStatus fix; update
the JobStatus initial-state descriptions so they say pollStatus is initialized
from initialStatus (or can reflect the job's configured/previous status) rather
than hardcoding "pending". Edit all occurrences that describe pollStatus (search
for JobStatus and pollStatus in the doc) and replace the fixed `"pending"`
initial-state text with a note that pollStatus is set from initialStatus (or the
resolved initial value) so the state model is consistent with the runtime
behavior.
- Around line 478-483: The checklist item "Add tests for polling stop
conditions" under "Action Items" is stale; update that checklist entry to
reflect that the PR added those tests by changing "3. **[ ] Add tests for
polling stop conditions**" to a completed checkbox "3. **[x] Add tests for
polling stop conditions**" (or alternatively reword it to "Add additional/future
coverage for polling stop conditions" if you prefer to indicate ongoing work),
and include a short note referencing the PR that introduced the tests for
clarity.
---
Nitpick comments:
In `@docs/design/frontend-state-flow.md`:
- Line 91: Change the phrase on line 91 to use the linter-preferred compound
form: replace "auto-populated" with "auto populated" in the text referencing
App.selectedTools so the sentence reads "App.selectedTools auto populated with
first tool" (update the documentation string that mentions App.selectedTools to
match the preferred compound style).
- Around line 48-54: The fenced code blocks in
docs/design/frontend-state-flow.md (e.g., the PluginSelector mount block showing
"useEffect([]) → apiClient.getPlugins()" and similar blocks at the other ranges)
lack language identifiers and trigger markdownlint MD040; update each
triple-backtick fence to include an appropriate language tag (for instance use
```text for plain flow diagrams or ```typescript/```json where the block
contains code/config) so all blocks (including those around lines 72-80,
102-108, 202-213, 237-249, 275-281, 306-312, 338-350, 353-361, 440-474) have
language specifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59db84bc-6f4a-418e-bd34-d7c138a8cc8b
📒 Files selected for processing (1)
docs/design/frontend-state-flow.md
Comment on lines
+36
to
+38
| | `JobStatus` | `pollProgress` | `number \| null` | `null` | | ||
| | `JobStatus` | `pollStatus` | `Status` | **`"pending"`** | | ||
| | `JobStatus` | `pollError` | `string \| null` | `null` | |
Contributor
There was a problem hiding this comment.
Update the JobStatus initial-state description to match current behaviour.
Line 37, Line 170, Line 192, and Line 471 still describe pollStatus as always initialised to "pending". That is now inaccurate after the initialStatus fix and makes the state model contradictory inside the same document.
Suggested doc patch
-| `JobStatus` | `pollStatus` | `Status` | **`"pending"`** |
+| `JobStatus` | `pollStatus` | `Status` | `initialStatus ?? "pending"` |
-const [pollStatus, setPollStatus] = useState<Status>("pending"); // ALWAYS "pending" on mount!
+const [pollStatus, setPollStatus] = useState<Status>(initialStatus ?? "pending");
-**BUG:** `pollStatus` initializes to `"pending"` on EVERY mount/remount!
+**Previous bug (fixed in PR `#364`):** `pollStatus` used to initialise to `"pending"` on every mount/remount.
- │ │ pollStatus="pending" │◄── BUG: Always "pending"!
+ │ │ pollStatus=initialStatus ?? "pending" │Also applies to: 167-195, 471-472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/frontend-state-flow.md` around lines 36 - 38, The doc currently
states JobStatus.pollStatus is always initialized to "pending" which is outdated
after the initialStatus fix; update the JobStatus initial-state descriptions so
they say pollStatus is initialized from initialStatus (or can reflect the job's
configured/previous status) rather than hardcoding "pending". Edit all
occurrences that describe pollStatus (search for JobStatus and pollStatus in the
doc) and replace the fixed `"pending"` initial-state text with a note that
pollStatus is set from initialStatus (or the resolved initial value) so the
state model is consistent with the runtime behavior.
Comment on lines
+478
to
+483
| ## Action Items | ||
|
|
||
| 1. **[x] Fix JobStatus initialStatus** - Add prop, pass from parent ✅ PR #364 | ||
| 2. **[ ] Fix JobList fetch-once bug** - Re-fetch on viewMode change (Issue #365) | ||
| 3. **[ ] Add tests for polling stop conditions** - TDD approach | ||
| 4. **[x] Document polling behavior** - This document |
Contributor
There was a problem hiding this comment.
Action item status is stale for test coverage.
Line 482 says polling stop-condition tests are still pending, but this PR explicitly adds those tests. Please mark this item as completed (or reword it to future coverage work only).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/frontend-state-flow.md` around lines 478 - 483, The checklist
item "Add tests for polling stop conditions" under "Action Items" is stale;
update that checklist entry to reflect that the PR added those tests by changing
"3. **[ ] Add tests for polling stop conditions**" to a completed checkbox "3.
**[x] Add tests for polling stop conditions**" (or alternatively reword it to
"Add additional/future coverage for polling stop conditions" if you prefer to
indicate ongoing work), and include a short note referencing the PR that
introduced the tests for clarity.
- Add viewMode prop to JobList component - Only fetch jobs when viewMode === 'jobs' - Re-fetch when viewMode changes to 'jobs' - Pass viewMode from App.tsx - Add tests for viewMode prop behavior This fixes the "failed to fetch" stuck state after single-tool video upload. Previously, JobList only fetched once on mount with useEffect([]), causing it to get stuck in error state if the initial fetch failed. Fixes #365
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/components/JobList.tsx`:
- Around line 19-20: The effect in JobList.tsx exits early when viewMode !==
"jobs" but never clears the component's initial loading state, leaving loading
true; update the effect that checks viewMode (the conditional using viewMode !==
undefined && viewMode !== "jobs") to set loading to false before returning so
the component doesn't stay stuck, and apply the same change to the other
identical early-return in the file (the other viewMode check around line 38) so
both branches clear loading when skipping the fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee8db8c1-89ad-4ae9-8f15-624d07623f3a
📒 Files selected for processing (3)
web-ui/src/App.tsxweb-ui/src/components/JobList.test.tsxweb-ui/src/components/JobList.tsx
Comment on lines
+19
to
+20
| // Issue #365: Only fetch when viewMode is 'jobs' (or not provided for backward compat) | ||
| if (viewMode !== undefined && viewMode !== "jobs") return; |
Contributor
There was a problem hiding this comment.
Non-jobs branch can leave the component stuck in loading state.
When viewMode is not "jobs", the effect returns immediately, but loading stays true from initial state. If this component is mounted outside the Jobs view, it can show “Loading jobs...” indefinitely.
Suggested fix
useEffect(() => {
- // Issue `#365`: Only fetch when viewMode is 'jobs' (or not provided for backward compat)
- if (viewMode !== undefined && viewMode !== "jobs") return;
+ // Issue `#365`: Only fetch when viewMode is 'jobs' (or not provided for backward compat)
+ if (viewMode !== undefined && viewMode !== "jobs") {
+ setLoading(false);
+ return;
+ }
+
+ setLoading(true);
const loadJobs = async () => {
try {
const data = await apiClient.listJobs();
setJobs(data);
setError(null);Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/JobList.tsx` around lines 19 - 20, The effect in
JobList.tsx exits early when viewMode !== "jobs" but never clears the
component's initial loading state, leaving loading true; update the effect that
checks viewMode (the conditional using viewMode !== undefined && viewMode !==
"jobs") to set loading to false before returning so the component doesn't stay
stuck, and apply the same change to the other identical early-return in the file
(the other viewMode check around line 38) so both branches clear loading when
skipping the fetch.
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.
Problem
When a user clicks on a completed job in the Jobs view, the
JobStatuscomponent starts polling even though the job is already completed. This causes unnecessary API requests and can lead to intermittent "infinite polling" behavior.Root Cause
JobStatus.tsx:23always initializedpollStatusto"pending"on every mount:When the component remounts (view switch, React re-render), polling restarts until the first fetch confirms the job is completed.
Solution
initialStatusprop toJobStatuscomponentpollStatusfrominitialStatusinstead of hardcoded"pending"initialStatusfromApp.tsxforselectedJobanduploadResultChanges
JobStatus.tsx:
initialStatus?: StatuspropuseState<Status>(initialStatus || "pending")App.tsx:
initialStatus={selectedJob.status}in Jobs viewinitialStatus={uploadResult.status}in Upload viewinitialStatus={uploadResult.status}in Video Upload viewTests Added
should NOT poll when initialStatus is 'completed'should NOT poll when initialStatus is 'failed'should poll when initialStatus is 'pending' (default behavior)should work without initialStatus prop (backward compatibility)TEST-CHANGE JUSTIFICATION
Tests added to verify the new
initialStatusprop behavior:Fixes #363
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation