Conversation
added 2 commits
March 25, 2026 00:28
v0.16.8: Decouple server from dictating summary structure. - Worker now uses plugin-provided summary when available - Falls back to derive_video_summary() for legacy plugins - API route reads pre-computed summary from job.summary - API no longer parses results JSON, just checks file existence - Tests updated to set job.summary for completed jobs This allows plugins to define their own summary metrics without server changes.
- Fix duplicate polling race condition in App.tsx (one poller instead of two) - Add App.polling.test.tsx with 4 tests for polling behavior - Fix test_get_job_results_invalid_json to use UUID in output_path - Add coverage upload to CI workflow for web-ui The polling fix prevents stale state overwrites and race conditions by consolidating two separate polling effects into one authoritative poller.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughJob summaries are now computed by the worker and stored on the job record; the API returns the stored summary and only verifies result file existence. Frontend polling is consolidated into a single serialized poller per selected job. CI now generates and uploads web-ui test coverage artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Web UI (App)
participant Poller as Poller (in App)
participant API as API Server
participant Storage as Object Storage
participant WorkerDB as Worker DB
Note over Poller,App: Poller tied to selectedJob.job_id (serialized timeout)
Poller->>API: GET /jobs/{job_id}
API->>WorkerDB: read job record (status, summary, output_path)
alt job completed
API->>Storage: HEAD / check output_path exists
Storage-->>API: 200 / 404
API-->>Poller: {status: completed, result_url, summary (from DB)}
else job pending/running
API-->>Poller: {status: pending|running, summary (maybe null)}
end
Poller-->>App: update selectedJob (only if job_id matches)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web-ui/src/App.polling.test.tsx (1)
117-263: The suite still misses the out-of-order response race.Every
mockGetJobhere resolves immediately and in order, so none of these tests reproduces the case where an older poll resolves after a newer one or after the user selects a different job. That means the stale-overwrite regression this PR is meant to prevent can still slip through. Add one deferred-promise test that resolves the first poll last and asserts the late response is ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/App.polling.test.tsx` around lines 117 - 263, Add a new test in App.polling.test.tsx that simulates an out-of-order response for mockGetJob: use deferred promises so the first getJob call (started by selecting "select-pending-job") returns a promise you resolve later, then trigger a second getJob (e.g., by selecting a different job or causing an immediate poll) that resolves immediately to the newer state; after resolving the newer promise assert the UI/state reflects the newer result and then resolve the original (older) deferred promise and assert its late result was ignored (no overwrite). Target the existing test helpers and identifiers (mockGetJob, select-pending-job, select-completed-job, vi.advanceTimersByTime, and the polling behavior tested elsewhere) to locate where to add this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/app/api_routes/routes/jobs.py`:
- Around line 243-244: Guard the JSON parsing of job.summary the same way as
list_jobs(): wrap the json.loads(job.summary) call in a try/except catching
json.JSONDecodeError (and general Exception if you prefer) and set summary =
None on failure (optionally log the error). Specifically update the assignment
that currently reads summary = json.loads(job.summary) if job.summary else None
to perform safe parsing using json.loads inside a try/except block so a
malformed job.summary does not raise a 500 in the route handling function.
In `@server/app/workers/worker.py`:
- Around line 679-683: The code currently calls
derive_video_summary(output_data) whenever a plugin omits "summary", causing
failures for non-video jobs; change the logic around summary_dict so that after
summary_dict = output_data.get("summary") you only call
derive_video_summary(output_data) when job.job_type indicates a video job (e.g.,
"video" or "video_multi"); for image/image_multi and other non-video types leave
summary_dict as None (or the plugin-provided value). Apply the same guard to the
similar block that appears later (the other occurrence noted around the second
range).
In `@web-ui/src/App.tsx`:
- Around line 230-241: The poller can apply stale responses because async getJob
calls overlap; fix by serializing or tokenizing requests: introduce a per-poll
token/counter (e.g., latestPollTickRef or pollToken) incremented before calling
apiClient.getJob(jobId) and verify the token still matches (and that the
currently selected job id still equals jobId) before calling
setSelectedJob(job); alternatively, avoid overlapping calls by replacing
setInterval with a recursive async poll loop that awaits apiClient.getJob and
only schedules the next poll after completion (ensuring jobId, selectedJob, and
apiClient.getJob are re-checked before updating). Ensure you reference and
update jobId, apiClient.getJob, setSelectedJob, and selectedJob accordingly.
---
Nitpick comments:
In `@web-ui/src/App.polling.test.tsx`:
- Around line 117-263: Add a new test in App.polling.test.tsx that simulates an
out-of-order response for mockGetJob: use deferred promises so the first getJob
call (started by selecting "select-pending-job") returns a promise you resolve
later, then trigger a second getJob (e.g., by selecting a different job or
causing an immediate poll) that resolves immediately to the newer state; after
resolving the newer promise assert the UI/state reflects the newer result and
then resolve the original (older) deferred promise and assert its late result
was ignored (no overwrite). Target the existing test helpers and identifiers
(mockGetJob, select-pending-job, select-completed-job, vi.advanceTimersByTime,
and the polling behavior tested elsewhere) to locate where to add this test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abbe665e-6339-4d13-aa67-f8faaf0af79c
📒 Files selected for processing (7)
.github/workflows/ci.ymlserver/app/api_routes/routes/jobs.pyserver/app/workers/worker.pyserver/tests/api/routes/test_jobs_unified.pyserver/tests/app/workers/test_worker.pyweb-ui/src/App.polling.test.tsxweb-ui/src/App.tsx
added 2 commits
March 26, 2026 00:15
- worker.py: Only call derive_video_summary for video/video_multi jobs - App.tsx: Use serialized async polling with cancelled flag to prevent race conditions Fixes two issues: 1. Non-video jobs were failing because derive_video_summary expects video-shaped payloads 2. Polling could still replay stale job state due to overlapping async getJob calls
Match the pattern used in list_jobs() - wrap json.loads in try/except to handle malformed summary JSON gracefully instead of raising 500.
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
The polling fix prevents stale state overwrites and race conditions by consolidating two separate polling effects into one authoritative poller.
TEST-CHANGE Justification
The following test changes are included in this PR:
These test changes are required to validate the polling refactor and ensure no regressions.
Files Changed
web-ui/src/App.tsx- Unified job polling (lines 223-259)web-ui/src/App.polling.test.tsx- New test fileserver/tests/api/routes/test_jobs_unified.py- Fixed test.github/workflows/ci.yml- Added coverage uploadSummary by CodeRabbit