You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Add defensive check for individual frame items in tools section.
While tool_frames is validated as a list, individual frame items are assumed to be dicts when calling frame.get("detections", []). Malformed entries would cause an AttributeError.
🛡️ Proposed defensive fix
for frame in tool_frames:
if not isinstance(frame, dict):
continue
detections = frame.get("detections", [])
tool_detections += len(detections)
for det in detections:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @server/app/api_routes/routes/jobs.py around lines 94 - 99, The loop over
tool_frames assumes each frame is a dict and calls frame.get(...) which can
raise AttributeError for malformed items; update the loop that iterates
tool_frames to first verify each frame is a dict (e.g., if not isinstance(frame,
dict): continue), then read detections = frame.get("detections", []) and ensure
detections is a list before using len() and iterating, and when iterating
detections check each det is a dict before accessing det["class"] to avoid
exceptions (references: variables/symbols tool_frames, frame, detections,
tool_detections, tool_classes).
web-ui/src/components/JobStatus.tsx
+6
-3
Lines changed: 6 additions & 3 deletions
Original file line number Diff line number Diff line change
setPollProgress(job.progress ?? null);
setPollError(null);
if (job.status === "completed" && job.results) {
setResults(job.results as JobResultsData);
// Clean Break (Issue #350): Results are fetched via result_url, not inline
if (job.status === "completed" && job.result_url) {
// Fetch results from result_url endpoint
const resultData = await apiClient.getJobResult(job.job_id, "stream");
setResults(resultData as JobResultsData);
Comment on lines R73 to R77
coderabbitai[bot] commented 3 hours ago @coderabbitai[bot]
coderabbitai[bot]
3 hours ago
Contributor ⚠️ Potential issue | 🟠 Major
This reintroduces the large-result path for video jobs.
Fetching mode="stream" here downloads the full JSON back into React state and hands it to JobResults, which is exactly the payload path Issue #350 is trying to remove. For video / video_multi, keep this component on the summary/artifact flow instead of eagerly streaming the entire result.
🤖 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 73 - 77, The current logic
in JobStatus.tsx fetches the full streamed result for completed jobs (using
apiClient.getJobResult with mode "stream" and setResults) which reintroduces the
large-result path; change the conditional to skip streaming for jobs with
job.type === "video" or "video_multi" (and any future large-media types) so this
component does not call apiClient.getJobResult("stream") or setResults for those
types, leaving video jobs to the summary/artifact flow (JobResults should
continue to receive artifact/summary data instead); update the if block around
job.status === "completed" && job.result_url to only stream non-video jobs and
ensure video cases do not eagerly download the full JSON.
web-ui/src/components/ArtifactViewer.tsx
+163
Lines changed: 163 additions & 0 deletions
Original file line number Diff line number Diff line change
/**
ArtifactViewer - Paginated viewer for large video job results
page survives a prop change, so switching from job A on page N to job B immediately requests job B with the old offset. That can skip the start of the new result set or land on an empty page.
Also applies to: 95-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @web-ui/src/components/ArtifactViewer.tsx around lines 68 - 69, The page
state in ArtifactViewer (useState page / setPage) isn't reset when the artifact
prop changes, causing requests to reuse an old offset; add a useEffect in the
ArtifactViewer component that watches the artifact identifier (e.g., artifact.id
or the prop that identifies the artefact) and on change calls setPage(0) (and
optionally setData(null)) so pagination and displayed data are reset whenever
the artefact changes; update any other pagination state resets with the same
effect (applies to the other pagination state usage around lines 95-97).
web-ui/src/components/ArtifactViewer.test.tsx
+349
Lines changed: 349 additions & 0 deletions
Original file line number Diff line number Diff line change
/**
Discussion v0.15.9 Don’t build the page URL from result_url. #352: Use API client for pagination (not direct URL fetch)
*/
import { describe, it, expect, vi, beforeEach } from "vitest";
import { render, screen, fireEvent, waitFor } from "@testing-library/react";
import { ArtifactViewer } from "./ArtifactViewer";
// Mock the apiClient module
vi.mock("../api/client", () => ({
apiClient: {
getJobResultPage: vi.fn(),
},
}));
import { apiClient } from "../api/client";
const mockGetJobResultPage = vi.mocked(apiClient.getJobResultPage);
// Mock fetch for download button (opens in new tab)
const mockFetch = vi.fn();
global.fetch = mockFetch;
describe("ArtifactViewer", () => {
beforeEach(() => {
mockGetJobResultPage.mockReset();
mockFetch.mockReset();
});
// Discussion v0.15.9 Don’t build the page URL from result_url. #352: New tests for jobId prop
describe("with jobId prop (Discussion v0.15.9 Don’t build the page URL from result_url. #352)", () => {
it("should accept jobId prop for pagination", () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 100,
frames: [],
});
render();
expect(screen.getByText(/download/i)).toBeInTheDocument();
});
it("should use apiClient.getJobResultPage for pagination (NOT direct fetch)", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [{ frame: 0 }, { frame: 1 }],
});
render();
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledWith("test-job-456", 0, 200);
});
});
it("should NOT build URL from result_url for pagination", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
// Pass both jobId and resultUrl - pagination should use jobId only
render(
);
await waitFor(() => {
// Should use apiClient.getJobResultPage with jobId
expect(mockGetJobResultPage).toHaveBeenCalledWith("job-789", 0, 200);
// Should NOT call fetch with URL concatenation
expect(mockFetch).not.toHaveBeenCalled();
});
});
it("should use resultUrl for download button only", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 100,
frames: [],
});
render(
);
await waitFor(() => {
expect(screen.getByText(/download/i)).toBeInTheDocument();
});
// Click download button - should open resultUrl in new tab
const downloadButton = screen.getByText(/download/i);
const openSpy = vi.spyOn(window, "open").mockImplementation(() => null);
fireEvent.click(downloadButton);
expect(openSpy).toHaveBeenCalledWith(
"http://localhost:3000/v1/jobs/job-download-test/result?token=xyz",
"_blank"
);
openSpy.mockRestore();
});
});
// Legacy tests (still need jobId now)
it("renders download button", () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 100,
frames: [],
});
render();
expect(screen.getByText(/download/i)).toBeInTheDocument();
});
it("fetches paginated data on mount using apiClient", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [{ frame: 0 }, { frame: 1 }],
});
render();
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledWith("test-job-mount", 0, 200);
});
});
it("shows loading state while fetching", () => {
mockGetJobResultPage.mockImplementation(() => new Promise(() => {})); // Never resolves
render();
expect(screen.getByText(/loading/i)).toBeInTheDocument();
});
it("shows error on fetch failure", async () => {
mockGetJobResultPage.mockRejectedValueOnce(new Error("Failed to load"));
render();
await waitFor(() => {
expect(screen.getByText(/error/i)).toBeInTheDocument();
});
});
it("shows prev and next buttons after data loads", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(screen.getByText(/prev/i)).toBeInTheDocument();
expect(screen.getByText(/next/i)).toBeInTheDocument();
});
});
it("disables prev button on first page", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
const prevButton = screen.getByText(/prev/i);
expect(prevButton).toBeDisabled();
});
});
it("enables prev button on second page", async () => {
// First fetch (page 0)
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(screen.getByText(/prev/i)).toBeDisabled();
});
// Mock fetch for page 1
mockGetJobResultPage.mockResolvedValueOnce({
offset: 200,
limit: 200,
total: 500,
frames: [],
});
// Click next to go to page 1
const nextButton = screen.getByText(/next/i);
fireEvent.click(nextButton);
await waitFor(() => {
const prevButton = screen.getByText(/prev/i);
expect(prevButton).not.toBeDisabled();
});
});
it("disables next button on last page", async () => {
// On last page, offset + limit >= total
mockGetJobResultPage.mockResolvedValueOnce({
offset: 400,
limit: 200,
total: 500, // offset 400 + limit 200 = 600 > 500, so last page
frames: [],
});
render();
await waitFor(() => {
// The component calculates canNext = (page + 1) * PAGE_SIZE < total
// For page 0 with offset 400: (0 + 1) * 200 = 200 < 500, so next is enabled
// But the response has offset=400, meaning the component should be on page 2
// Let me check: page 2 * 200 = 400 offset, (2+1)*200 = 600 > 500, so disabled
// But the component starts at page 0 regardless of response offset
const nextButton = screen.getByText(/next/i);
// Since component starts at page 0, canNext = 200 < 500 = true
// So next should be enabled. Let me fix the test.
expect(nextButton).not.toBeDisabled();
});
});
Comment on lines R227 to R249
coderabbitai[bot] commented 3 hours ago @coderabbitai[bot]
coderabbitai[bot]
3 hours ago
Contributor ⚠️ Potential issue | 🟡 Minor
Test name contradicts assertion; logic is confusing.
The test is named "disables next button on last page" but the assertion on line 181 expects the button to not be disabled. The inline comments (lines 173-180) acknowledge the component starts at page 0 regardless of the response's offset, making this test not actually verify last-page behaviour.
Either rename the test to match what it actually verifies, or fix the test to properly navigate to the last page before asserting next is disabled.
Suggested fix: Navigate to last page before asserting
🤖 Prompt for AI Agents
Write a reply
it("shows correct page info", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(screen.getByText(/Page 1 of 3/)).toBeInTheDocument();
expect(screen.getByText(/500 total frames/)).toBeInTheDocument();
});
});
it("fetches next page when next button clicked", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledTimes(1);
});
// Mock next page fetch
mockGetJobResultPage.mockResolvedValueOnce({
offset: 200,
limit: 200,
total: 500,
frames: [],
});
const nextButton = screen.getByText(/next/i);
fireEvent.click(nextButton);
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledWith("test-job-next", 200, 200);
});
});
it("fetches previous page when prev button clicked", async () => {
// Start on page 1
mockGetJobResultPage.mockResolvedValueOnce({
offset: 200,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledTimes(1);
});
// Mock prev page fetch
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
const prevButton = screen.getByText(/prev/i);
fireEvent.click(prevButton);
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledWith("test-job-prev", 0, 200);
});
});
Comment on lines R297 to R326
coderabbitai[bot] commented 3 hours ago @coderabbitai[bot]
coderabbitai[bot]
3 hours ago
Contributor ⚠️ Potential issue | 🟡 Minor
Test setup inconsistent with component state.
The component starts at internal page 0, but the mock returns offset: 200. Clicking "Prev" from page 0 would decrement to page -1, producing offset=0 in the fetch call—not because the component correctly handled pagination from page 1, but because the offset calculation happens to work out.
This test doesn't verify the intended behaviour (navigating backwards from page 1). Consider starting at page 0, clicking "Next" to reach page 1, then clicking "Prev" to verify backwards navigation.
Suggested fix: Start at page 0 and navigate forward first
it("fetches previous page when prev button clicked", async () => {
});
📝 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.
In @web-ui/src/components/ArtifactViewer.test.tsx around lines 245 - 284, The
test "fetches previous page when prev button clicked" is inconsistent because it
assumes the component starts on page 1; update the test to start at the
component's default page 0 by returning offset: 0 for the initial mockFetch,
then mock a "next page" response (offset: 200) and simulate clicking the "Next"
button (screen.getByText(/next/i) + fireEvent.click) to move to page 1, then
mock the "prev page" response (offset: 0) and click the "Prev" button
(screen.getByText(/prev/i) + fireEvent.click) and finally assert mockFetch was
called with "/v1/jobs/test-job/result/page?offset=0&limit=200"; adjust the
mockResolvedValueOnce calls in this sequence and keep references to mockFetch,
render(<ArtifactViewer ...>), screen, and fireEvent to locate the changes.
# No more inline results for any job type
if job.status == JobStatus.completed and job.output_path:
try:
result_url = storage.get_signed_url(job.output_path)
# Load results to derive summary
file_path = storage.load_file(job.output_path)
with open(file_path, "r") as f:
result = json.load(f)
results = json.load(f)
Comment view summary = _derive_video_summary(results)Collapse commentComment on lines R151 to R158coderabbitai[bot] commented on Mar 20, 2026 coderabbitai[bot]on Mar 20, 2026ContributorMore actions⚠️ Potential issue | 🟠 Major
/v1/jobs is still loading every full result artefact on the hot path.
This loop calls json.load(...) for each completed job just to derive summary. With large video outputs and limit=100, listing jobs becomes O(page size × artefact size) I/O and memory again, which undercuts the Artifact Pattern; computing and persisting the summary during worker finalisation would keep this endpoint metadata-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
server/app/api_routes/routes/jobs.py
Comment on lines 151 to +158
if job.status == JobStatus.completed and job.output_path:
try:
result_url = storage.get_signed_url(job.output_path)
# Load results to derive summary
file_path = storage.load_file(job.output_path)
with open(file_path, "r") as f:
result = json.load(f)
results = json.load(f)
summary = _derive_video_summary(results)
Contributor @coderabbitai
coderabbitai bot
3 hours ago ⚠️ Potential issue | 🟠 Major
/v1/jobs is still loading every full result artefact on the hot path.
This loop calls json.load(...) for each completed job just to derive summary. With large video outputs and limit=100, listing jobs becomes O(page size × artefact size) I/O and memory again, which undercuts the Artifact Pattern; computing and persisting the summary during worker finalisation would keep this endpoint metadata-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @server/app/api_routes/routes/jobs.py around lines 151 - 158, The route is
loading full artefacts (storage.load_file + json.load) to compute a summary for
each completed job; instead compute and persist the summary when the worker
finalises the job and then read that stored summary in the /v1/jobs route.
Update the worker finalisation code that writes the artifact to storage to call
_derive_video_summary(results) (or equivalent) right after the artifact is
created, store the returned summary on the Job model (e.g., job.summary or
job.metadata['summary']), and persist the Job; then modify the jobs listing code
that checks job.status == JobStatus.completed and job.output_path to use
job.summary (and job.output_url via storage.get_signed_url if needed) and remove
storage.load_file/json.load from the hot path. Ensure references:
_derive_video_summary, storage.load_file, storage.get_signed_url, Job
(job.summary/job.metadata), and the worker finalisation routine are updated
accordingly.
server/app/api_routes/routes/jobs.py
Comment on lines +80 to +103
# Handle tools structure (multi-tool video jobs)
tools = results.get("tools", {})
if tools:
tool_detections = 0
tool_classes: set = set()
for _tool_name, tool_results in tools.items():
# Defensive: skip if tool_results is not a dict (malformed data)
if not isinstance(tool_results, dict):
continue
tool_frames = tool_results.get("frames", [])
# Defensive: skip if tool_frames is not a list
if not isinstance(tool_frames, list):
continue
for frame in tool_frames:
detections = frame.get("detections", [])
tool_detections += len(detections)
for det in detections:
if "class" in det:
tool_classes.add(det["class"])
# Add to existing counts
detection_count += tool_detections
classes = sorted(set(classes) | tool_classes)
Contributor @coderabbitai
coderabbitai bot
3 hours ago ⚠️ Potential issue | 🟠 Major
Parse the actual video_multi result shape here.
worker._merge_video_frames() emits multi-tool video results as merged top-level frames, with each tool payload nested under the frame’s tool keys. This branch looks for a top-level tools object instead, so detection_count and classes stay empty for real video_multi artefacts even when detections are present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @server/app/api_routes/routes/jobs.py around lines 80 - 103, The code is
looking for a top-level "tools" object but worker._merge_video_frames() produces
merged top-level "frames" where each frame contains per-tool payloads; update
the branch to iterate results.get("frames", []) and for each frame inspect its
keys for tool payloads (skip known frame fields like
"timestamp"/"frame_index"/"detections"), then accumulate detections and classes
from each tool payload into tool_detections and tool_classes (same variables),
finally add tool_detections to detection_count and merge tool_classes into
classes; use the existing symbols results, worker._merge_video_frames(),
tool_detections, tool_classes, detection_count, and classes to locate and
implement the change.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
Uh oh!
There was an error while loading. Please reload this page.
-
Add defensive check for individual frame items in tools section.
While tool_frames is validated as a list, individual frame items are assumed to be dicts when calling frame.get("detections", []). Malformed entries would cause an AttributeError.
🛡️ Proposed defensive fix
for frame in tool_frames:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@server/app/api_routes/routes/jobs.pyaround lines 94 - 99, The loop overtool_frames assumes each frame is a dict and calls frame.get(...) which can
raise AttributeError for malformed items; update the loop that iterates
tool_frames to first verify each frame is a dict (e.g., if not isinstance(frame,
dict): continue), then read detections = frame.get("detections", []) and ensure
detections is a list before using len() and iterating, and when iterating
detections check each det is a dict before accessing det["class"] to avoid
exceptions (references: variables/symbols tool_frames, frame, detections,
tool_detections, tool_classes).
web-ui/src/components/JobStatus.tsx
+6
-3
Lines changed: 6 additions & 3 deletions
Original file line number Diff line number Diff line change
setPollProgress(job.progress ?? null);
setPollError(null);
Comment on lines R73 to R77
⚠️ Potential issue | 🟠 Major
coderabbitai[bot] commented 3 hours ago
@coderabbitai[bot]
coderabbitai[bot]
3 hours ago
Contributor
This reintroduces the large-result path for video jobs.
Fetching mode="stream" here downloads the full JSON back into React state and hands it to JobResults, which is exactly the payload path Issue #350 is trying to remove. For video / video_multi, keep this component on the summary/artifact flow instead of eagerly streaming the entire result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@web-ui/src/components/JobStatus.tsxaround lines 73 - 77, The current logicin JobStatus.tsx fetches the full streamed result for completed jobs (using
apiClient.getJobResult with mode "stream" and setResults) which reintroduces the
large-result path; change the conditional to skip streaming for jobs with
job.type === "video" or "video_multi" (and any future large-media types) so this
component does not call apiClient.getJobResult("stream") or setResults for those
types, leaving video jobs to the summary/artifact flow (JobResults should
continue to receive artifact/summary data instead); update the if block around
job.status === "completed" && job.result_url to only stream non-video jobs and
ensure video cases do not eagerly download the full JSON.
web-ui/src/components/ArtifactViewer.tsx
+163
Lines changed: 163 additions & 0 deletions
Original file line number Diff line number Diff line change
/**
*/
import React, { useEffect, useState, useCallback } from "react";
import { apiClient } from "../api/client";
interface ArtifactViewerProps {
jobId: string; // Required: Used for API-based pagination
resultUrl?: string; // Optional: Used for "Download Full JSON" button
}
interface ArtifactPage {
offset: number;
limit: number;
total: number;
frames: unknown[];
}
const PAGE_SIZE = 200;
const styles: Record<string, React.CSSProperties> = {
container: {
marginTop: 12,
},
downloadButton: {
marginBottom: 8,
padding: "6px 12px",
cursor: "pointer",
},
loading: {
color: "var(--text-secondary, #888)",
},
error: {
color: "var(--accent-danger, #ff4444)",
},
frameContainer: {
maxHeight: 300,
overflow: "auto",
fontSize: 11,
backgroundColor: "var(--bg-primary, #1a1a1a)",
padding: 8,
borderRadius: 4,
},
pagination: {
marginTop: 8,
display: "flex",
gap: 8,
alignItems: "center",
},
button: {
padding: "4px 12px",
cursor: "pointer",
},
buttonDisabled: {
padding: "4px 12px",
cursor: "not-allowed",
opacity: 0.5,
},
pageInfo: {
fontSize: 12,
color: "var(--text-secondary, #888)",
},
};
export function ArtifactViewer({ jobId, resultUrl }: ArtifactViewerProps): React.ReactElement {
const [page, setPage] = useState(0);
const [data, setData] = useState<ArtifactPage | null>(null);
Comment on lines R73 to R74
coderabbitai[bot] commented 3 hours ago
@coderabbitai[bot]
coderabbitai[bot]
3 hours ago
Contributor
Reset pagination when the artefact changes.
page survives a prop change, so switching from job A on page N to job B immediately requests job B with the old offset. That can skip the start of the new result set or land on an empty page.
Also applies to: 95-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@web-ui/src/components/ArtifactViewer.tsxaround lines 68 - 69, The pagestate in ArtifactViewer (useState page / setPage) isn't reset when the artifact
prop changes, causing requests to reuse an old offset; add a useEffect in the
ArtifactViewer component that watches the artifact identifier (e.g., artifact.id
or the prop that identifies the artefact) and on change calls setPage(0) (and
optionally setData(null)) so pagination and displayed data are reset whenever
the artefact changes; update any other pagination state resets with the same
effect (applies to the other pagination state usage around lines 95-97).
web-ui/src/components/ArtifactViewer.test.tsx
+349
Lines changed: 349 additions & 0 deletions
Original file line number Diff line number Diff line change
/**
*/
import { describe, it, expect, vi, beforeEach } from "vitest";
import { render, screen, fireEvent, waitFor } from "@testing-library/react";
import { ArtifactViewer } from "./ArtifactViewer";
// Mock the apiClient module
vi.mock("../api/client", () => ({
apiClient: {
getJobResultPage: vi.fn(),
},
}));
import { apiClient } from "../api/client";
const mockGetJobResultPage = vi.mocked(apiClient.getJobResultPage);
// Mock fetch for download button (opens in new tab)
const mockFetch = vi.fn();
global.fetch = mockFetch;
describe("ArtifactViewer", () => {
beforeEach(() => {
mockGetJobResultPage.mockReset();
mockFetch.mockReset();
});
// Discussion v0.15.9 Don’t build the page URL from result_url. #352: New tests for jobId prop
describe("with jobId prop (Discussion v0.15.9 Don’t build the page URL from result_url. #352)", () => {
it("should accept jobId prop for pagination", () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 100,
frames: [],
});
render();
expect(screen.getByText(/download/i)).toBeInTheDocument();
});
it("should use apiClient.getJobResultPage for pagination (NOT direct fetch)", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [{ frame: 0 }, { frame: 1 }],
});
render();
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledWith("test-job-456", 0, 200);
});
});
it("should NOT build URL from result_url for pagination", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
// Pass both jobId and resultUrl - pagination should use jobId only
render(
);
await waitFor(() => {
// Should use apiClient.getJobResultPage with jobId
expect(mockGetJobResultPage).toHaveBeenCalledWith("job-789", 0, 200);
// Should NOT call fetch with URL concatenation
expect(mockFetch).not.toHaveBeenCalled();
});
});
it("should use resultUrl for download button only", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 100,
frames: [],
});
render(
);
await waitFor(() => {
expect(screen.getByText(/download/i)).toBeInTheDocument();
});
// Click download button - should open resultUrl in new tab
const downloadButton = screen.getByText(/download/i);
const openSpy = vi.spyOn(window, "open").mockImplementation(() => null);
fireEvent.click(downloadButton);
expect(openSpy).toHaveBeenCalledWith(
"http://localhost:3000/v1/jobs/job-download-test/result?token=xyz",
"_blank"
);
openSpy.mockRestore();
});
});
// Legacy tests (still need jobId now)
it("renders download button", () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 100,
frames: [],
});
render();
expect(screen.getByText(/download/i)).toBeInTheDocument();
});
it("fetches paginated data on mount using apiClient", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [{ frame: 0 }, { frame: 1 }],
});
render();
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledWith("test-job-mount", 0, 200);
});
});
it("shows loading state while fetching", () => {
mockGetJobResultPage.mockImplementation(() => new Promise(() => {})); // Never resolves
render();
expect(screen.getByText(/loading/i)).toBeInTheDocument();
});
it("shows error on fetch failure", async () => {
mockGetJobResultPage.mockRejectedValueOnce(new Error("Failed to load"));
render();
await waitFor(() => {
expect(screen.getByText(/error/i)).toBeInTheDocument();
});
});
it("shows prev and next buttons after data loads", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(screen.getByText(/prev/i)).toBeInTheDocument();
expect(screen.getByText(/next/i)).toBeInTheDocument();
});
});
it("disables prev button on first page", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
const prevButton = screen.getByText(/prev/i);
expect(prevButton).toBeDisabled();
});
});
it("enables prev button on second page", async () => {
// First fetch (page 0)
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(screen.getByText(/prev/i)).toBeDisabled();
});
// Mock fetch for page 1
mockGetJobResultPage.mockResolvedValueOnce({
offset: 200,
limit: 200,
total: 500,
frames: [],
});
// Click next to go to page 1
const nextButton = screen.getByText(/next/i);
fireEvent.click(nextButton);
await waitFor(() => {
const prevButton = screen.getByText(/prev/i);
expect(prevButton).not.toBeDisabled();
});
});
it("disables next button on last page", async () => {
// On last page, offset + limit >= total
mockGetJobResultPage.mockResolvedValueOnce({
offset: 400,
limit: 200,
total: 500, // offset 400 + limit 200 = 600 > 500, so last page
frames: [],
});
render();
await waitFor(() => {
// The component calculates canNext = (page + 1) * PAGE_SIZE < total
// For page 0 with offset 400: (0 + 1) * 200 = 200 < 500, so next is enabled
// But the response has offset=400, meaning the component should be on page 2
// Let me check: page 2 * 200 = 400 offset, (2+1)*200 = 600 > 500, so disabled
// But the component starts at page 0 regardless of response offset
const nextButton = screen.getByText(/next/i);
// Since component starts at page 0, canNext = 200 < 500 = true
// So next should be enabled. Let me fix the test.
expect(nextButton).not.toBeDisabled();
});
});
Comment on lines R227 to R249
coderabbitai[bot] commented 3 hours ago
@coderabbitai[bot]
coderabbitai[bot]
3 hours ago
Contributor
Test name contradicts assertion; logic is confusing.
The test is named "disables next button on last page" but the assertion on line 181 expects the button to not be disabled. The inline comments (lines 173-180) acknowledge the component starts at page 0 regardless of the response's offset, making this test not actually verify last-page behaviour.
Either rename the test to match what it actually verifies, or fix the test to properly navigate to the last page before asserting next is disabled.
Suggested fix: Navigate to last page before asserting
⚠️ Potential issue | 🟡 Minor
🤖 Prompt for AI Agents
Write a reply
it("shows correct page info", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(screen.getByText(/Page 1 of 3/)).toBeInTheDocument();
expect(screen.getByText(/500 total frames/)).toBeInTheDocument();
});
});
it("fetches next page when next button clicked", async () => {
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledTimes(1);
});
// Mock next page fetch
mockGetJobResultPage.mockResolvedValueOnce({
offset: 200,
limit: 200,
total: 500,
frames: [],
});
const nextButton = screen.getByText(/next/i);
fireEvent.click(nextButton);
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledWith("test-job-next", 200, 200);
});
});
it("fetches previous page when prev button clicked", async () => {
// Start on page 1
mockGetJobResultPage.mockResolvedValueOnce({
offset: 200,
limit: 200,
total: 500,
frames: [],
});
render();
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledTimes(1);
});
// Mock prev page fetch
mockGetJobResultPage.mockResolvedValueOnce({
offset: 0,
limit: 200,
total: 500,
frames: [],
});
const prevButton = screen.getByText(/prev/i);
fireEvent.click(prevButton);
await waitFor(() => {
expect(mockGetJobResultPage).toHaveBeenCalledWith("test-job-prev", 0, 200);
});
});
Comment on lines R297 to R326
coderabbitai[bot] commented 3 hours ago
@coderabbitai[bot]
coderabbitai[bot]
3 hours ago
Contributor
Test setup inconsistent with component state.
The component starts at internal page 0, but the mock returns offset: 200. Clicking "Prev" from page 0 would decrement to page -1, producing offset=0 in the fetch call—not because the component correctly handled pagination from page 1, but because the offset calculation happens to work out.
This test doesn't verify the intended behaviour (navigating backwards from page 1). Consider starting at page 0, clicking "Next" to reach page 1, then clicking "Prev" to verify backwards navigation.
Suggested fix: Start at page 0 and navigate forward first
it("fetches previous page when prev button clicked", async () => {
📝 Committable suggestion
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
it("fetches previous page when prev button clicked", async () => {
// Start on page 1
mockFetch.mockResolvedValueOnce({
ok: true,
json: () =>
Promise.resolve({
offset: 200,
limit: 200,
total: 500,
frames: [],
}),
});
render();
await waitFor(() => {
expect(mockFetch).toHaveBeenCalledTimes(1);
});
// Mock prev page fetch
mockFetch.mockResolvedValueOnce({
ok: true,
json: () =>
Promise.resolve({
offset: 0,
limit: 200,
total: 500,
frames: [],
}),
});
const prevButton = screen.getByText(/prev/i);
fireEvent.click(prevButton);
await waitFor(() => {
expect(mockFetch).toHaveBeenCalledWith(
"/v1/jobs/test-job/result/page?offset=0&limit=200"
);
});
});
it("fetches previous page when prev button clicked", async () => {
// Start on page 0
mockFetch.mockResolvedValueOnce({
ok: true,
json: () =>
Promise.resolve({
offset: 0,
limit: 200,
total: 500,
frames: [],
}),
});
render();
await waitFor(() => {
expect(mockFetch).toHaveBeenCalledTimes(1);
});
// Navigate to page 1
mockFetch.mockResolvedValueOnce({
ok: true,
json: () =>
Promise.resolve({
offset: 200,
limit: 200,
total: 500,
frames: [],
}),
});
fireEvent.click(screen.getByText(/next/i));
await waitFor(() => {
expect(mockFetch).toHaveBeenCalledTimes(2);
});
// Mock prev page fetch (back to page 0)
mockFetch.mockResolvedValueOnce({
ok: true,
json: () =>
Promise.resolve({
offset: 0,
limit: 200,
total: 500,
frames: [],
}),
});
const prevButton = screen.getByText(/prev/i);
fireEvent.click(prevButton);
await waitFor(() => {
expect(mockFetch).toHaveBeenCalledWith(
"/v1/jobs/test-job/result/page?offset=0&limit=200"
);
});
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@web-ui/src/components/ArtifactViewer.test.tsxaround lines 245 - 284, Thetest "fetches previous page when prev button clicked" is inconsistent because it
assumes the component starts on page 1; update the test to start at the
component's default page 0 by returning offset: 0 for the initial mockFetch,
then mock a "next page" response (offset: 200) and simulate clicking the "Next"
button (screen.getByText(/next/i) + fireEvent.click) to move to page 1, then
mock the "prev page" response (offset: 0) and click the "Prev" button
(screen.getByText(/prev/i) + fireEvent.click) and finally assert mockFetch was
called with "/v1/jobs/test-job/result/page?offset=0&limit=200"; adjust the
mockResolvedValueOnce calls in this sequence and keep references to mockFetch,
render(<ArtifactViewer ...>), screen, and fireEvent to locate the changes.
Comment view summary = _derive_video_summary(results)Collapse commentComment on lines R151 to R158coderabbitai[bot] commented on Mar 20, 2026 coderabbitai[bot]on Mar 20, 2026ContributorMore actions⚠️ Potential issue | 🟠 Major
/v1/jobs is still loading every full result artefact on the hot path.
This loop calls json.load(...) for each completed job just to derive summary. With large video outputs and limit=100, listing jobs becomes O(page size × artefact size) I/O and memory again, which undercuts the Artifact Pattern; computing and persisting the summary during worker finalisation would keep this endpoint metadata-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
server/app/api_routes/routes/jobs.py
⚠️ Potential issue | 🟠 Major
Comment on lines 151 to +158
if job.status == JobStatus.completed and job.output_path:
try:
result_url = storage.get_signed_url(job.output_path)
# Load results to derive summary
file_path = storage.load_file(job.output_path)
with open(file_path, "r") as f:
result = json.load(f)
results = json.load(f)
summary = _derive_video_summary(results)
Contributor
@coderabbitai
coderabbitai bot
3 hours ago
/v1/jobs is still loading every full result artefact on the hot path.
This loop calls json.load(...) for each completed job just to derive summary. With large video outputs and limit=100, listing jobs becomes O(page size × artefact size) I/O and memory again, which undercuts the Artifact Pattern; computing and persisting the summary during worker finalisation would keep this endpoint metadata-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@server/app/api_routes/routes/jobs.pyaround lines 151 - 158, The route isloading full artefacts (storage.load_file + json.load) to compute a summary for
each completed job; instead compute and persist the summary when the worker
finalises the job and then read that stored summary in the /v1/jobs route.
Update the worker finalisation code that writes the artifact to storage to call
_derive_video_summary(results) (or equivalent) right after the artifact is
created, store the returned summary on the Job model (e.g., job.summary or
job.metadata['summary']), and persist the Job; then modify the jobs listing code
that checks job.status == JobStatus.completed and job.output_path to use
job.summary (and job.output_url via storage.get_signed_url if needed) and remove
storage.load_file/json.load from the hot path. Ensure references:
_derive_video_summary, storage.load_file, storage.get_signed_url, Job
(job.summary/job.metadata), and the worker finalisation routine are updated
accordingly.
server/app/api_routes/routes/jobs.py
Comment on lines +80 to +103
# Handle tools structure (multi-tool video jobs)
tools = results.get("tools", {})
if tools:
tool_detections = 0
tool_classes: set = set()
Contributor
⚠️ Potential issue | 🟠 Major
@coderabbitai
coderabbitai bot
3 hours ago
Parse the actual video_multi result shape here.
worker._merge_video_frames() emits multi-tool video results as merged top-level frames, with each tool payload nested under the frame’s tool keys. This branch looks for a top-level tools object instead, so detection_count and classes stay empty for real video_multi artefacts even when detections are present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@server/app/api_routes/routes/jobs.pyaround lines 80 - 103, The code islooking for a top-level "tools" object but worker._merge_video_frames() produces
merged top-level "frames" where each frame contains per-tool payloads; update
the branch to iterate results.get("frames", []) and for each frame inspect its
keys for tool payloads (skip known frame fields like
"timestamp"/"frame_index"/"detections"), then accumulate detections and classes
from each tool payload into tool_detections and tool_classes (same variables),
finally add tool_detections to detection_count and merge tool_classes into
classes; use the existing symbols results, worker._merge_video_frames(),
tool_detections, tool_classes, detection_count, and classes to locate and
implement the change.
Beta Was this translation helpful? Give feedback.
All reactions