feat: real-time task run logs on /tasks/[runId]#1531
Conversation
Add polling-based observability UI that shows live task progress: - getTaskRunStatus: fetch function for GET /api/tasks/runs - useTaskRunStatus: polling hook (3s interval, stops on terminal state) - RunLogsList: auto-scrolling log viewer - RunPage: composed page with status indicator, current step, and logs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntroduces task run monitoring functionality with a new React hook ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as RunPage Component
participant Hook as useTaskRunStatus Hook
participant API as getTaskRunStatus Function
participant External as Recoup API
UI->>Hook: runId provided, component mounts
Hook->>API: getTaskRunStatus(runId, accessToken)
API->>External: GET /runs?runId={runId}
External-->>API: {status, metadata, logs, ...}
API-->>Hook: TaskRunStatus
Hook-->>UI: {data, loading, error}
UI->>UI: Render status, logs, duration
Note over Hook: Poll every 3s if not terminal
Hook->>API: getTaskRunStatus(runId, accessToken)
API->>External: GET /runs?runId={runId}
External-->>API: {status: "complete", ...}
API-->>Hook: TaskRunStatus
Hook-->>UI: Updated data (status terminal)
Hook->>Hook: Disable refetch (terminal reached)
UI->>UI: Render final state
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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: 164a794a6c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export default function RunPage({ runId }: RunPageProps) { | ||
| const { data, isLoading, error } = useTaskRunStatus(runId); | ||
|
|
||
| if (isLoading || !data) { |
There was a problem hiding this comment.
useTaskRunStatus can produce error with data === undefined (for example with an invalid runId, auth failure, or network failure), and this branch runs first because it checks isLoading || !data. In those cases the page stays on the loading spinner forever and never renders the error UI, so users cannot see what went wrong. Evaluate error before the !data fallback (or only show loading when isLoading is true).
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@components/TasksPage/Run/RunLogsList.tsx`:
- Around line 24-32: The container div in RunLogsList (the element that renders
logs.map and uses bottomRef) should be marked with role="log" for accessibility;
update the outer div in RunLogsList.tsx to include role="log" (leaving bottomRef
and existing className/children unchanged) so screen readers treat it as a live
log region.
In `@components/TasksPage/Run/RunPage.tsx`:
- Around line 34-52: The guard in RunPage currently returns the loading UI when
isLoading || !data, which masks the error state because a failed useQuery yields
isLoading=false but data=undefined; reorder or combine the checks so error is
evaluated before the "no data" loading branch: check error first (the error
variable from the useQuery) and render the error UI if present, then handle
loading/no-data with isLoading || !data; update the conditional logic in the
RunPage component around the existing if blocks (the ones referencing isLoading,
data, and error) accordingly.
🧹 Nitpick comments (5)
components/TasksPage/Run/RunLogsList.tsx (2)
5-7: ExportRunLogsListPropsper project conventions.The coding guidelines require exporting component prop types with the
ComponentNamePropsnaming convention. The interface is correctly named but not exported.-interface RunLogsListProps { +export interface RunLogsListProps { logs: string[]; }As per coding guidelines, "Export component prop types with explicit
ComponentNamePropsnaming convention".
12-14: Dependency onlogs.lengthcan miss updates where content changes but count stays the same.If a log entry is replaced rather than appended (unlikely but possible), the effect won't fire. For an append-only log stream this is fine in practice, but using the
logsarray reference directly would be more robust. React'suseEffectperforms a shallow comparison, so a new array reference would still trigger the scroll.useEffect(() => { bottomRef.current?.scrollIntoView({ behavior: "smooth" }); - }, [logs.length]); + }, [logs]);components/TasksPage/Run/RunPage.tsx (2)
81-81: Unnecessaryas string[]type assertion.
data.metadata?.logsis already typed asstring[] | undefinedviaTaskRunMetadata. The?? []fallback handles the undefined case, so the assertion adds no safety and could mask a runtime type mismatch if the API returns unexpected data.- <RunLogsList logs={logs as string[]} /> + <RunLogsList logs={logs} />On line 55, the inferred type of
logsfromdata.metadata?.logs ?? []is alreadystring[], making the assertion redundant.
93-103: Copy-to-clipboard: consider adding error handling and an accessible label.
navigator.clipboard.writeTextcan reject (non-secure context, permission denied). A silent catch would prevent an unhandled promise rejection. Additionally, anaria-labelwould improve accessibility for the copy button.♻️ Suggested improvement
<button - className="inline-flex cursor-pointer items-center gap-1 hover:text-foreground" + aria-label={copied ? "Run ID copied" : "Copy run ID to clipboard"} + className="inline-flex cursor-pointer items-center gap-1 hover:text-foreground" onClick={() => { - navigator.clipboard.writeText(runId); - setCopied(true); - setTimeout(() => setCopied(false), 2000); + navigator.clipboard.writeText(runId).then(() => { + setCopied(true); + setTimeout(() => setCopied(false), 2000); + }).catch(() => { + // clipboard write failed silently + }); }} >lib/tasks/getTaskRunStatus.ts (1)
38-42:response.json()can throw on non-JSON error responses.If the server returns a non-JSON body (e.g., a 502 HTML page from a gateway),
response.json()will throw aSyntaxErrorbefore you reach theresponse.okcheck. This produces a confusing error message instead of a meaningful one.🛡️ Defensive fix
- const data = await response.json(); - - if (!response.ok) { - throw new Error(data.error || "Failed to fetch task run status"); + let data: Record<string, unknown>; + try { + data = await response.json(); + } catch { + throw new Error(`Failed to fetch task run status (HTTP ${response.status})`); + } + + if (!response.ok) { + throw new Error((data.error as string) || "Failed to fetch task run status"); }
| return ( | ||
| <div className="max-h-80 overflow-y-auto rounded-md border bg-muted/30 p-3 font-mono text-sm"> | ||
| {logs.map((log, i) => ( | ||
| <p key={i} className="py-0.5 text-muted-foreground"> | ||
| {log} | ||
| </p> | ||
| ))} | ||
| <div ref={bottomRef} /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add role="log" for accessibility on the log container.
This is a live-updating log viewer — the role="log" ARIA role is purpose-built for this exact use case. It implies aria-live="polite" and communicates to assistive technology that new content is being appended.
♻️ Proposed fix
- <div className="max-h-80 overflow-y-auto rounded-md border bg-muted/30 p-3 font-mono text-sm">
+ <div
+ role="log"
+ aria-label="Task run logs"
+ className="max-h-80 overflow-y-auto rounded-md border bg-muted/30 p-3 font-mono text-sm"
+ >As per coding guidelines, "Use semantic HTML elements appropriate to the component's role" and "Always include accessibility in components: use semantic HTML, ARIA attributes, and keyboard navigation".
📝 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.
| return ( | |
| <div className="max-h-80 overflow-y-auto rounded-md border bg-muted/30 p-3 font-mono text-sm"> | |
| {logs.map((log, i) => ( | |
| <p key={i} className="py-0.5 text-muted-foreground"> | |
| {log} | |
| </p> | |
| ))} | |
| <div ref={bottomRef} /> | |
| </div> | |
| return ( | |
| <div | |
| role="log" | |
| aria-label="Task run logs" | |
| className="max-h-80 overflow-y-auto rounded-md border bg-muted/30 p-3 font-mono text-sm" | |
| > | |
| {logs.map((log, i) => ( | |
| <p key={i} className="py-0.5 text-muted-foreground"> | |
| {log} | |
| </p> | |
| ))} | |
| <div ref={bottomRef} /> | |
| </div> |
🤖 Prompt for AI Agents
In `@components/TasksPage/Run/RunLogsList.tsx` around lines 24 - 32, The container
div in RunLogsList (the element that renders logs.map and uses bottomRef) should
be marked with role="log" for accessibility; update the outer div in
RunLogsList.tsx to include role="log" (leaving bottomRef and existing
className/children unchanged) so screen readers treat it as a live log region.
| if (isLoading || !data) { | ||
| return ( | ||
| <div className="flex h-screen flex-col items-center justify-center p-4"> | ||
| <Loader2 className="size-8 animate-spin text-muted-foreground" /> | ||
| <p className="mt-3 text-sm text-muted-foreground">Loading run status...</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (error) { | ||
| return ( | ||
| <div className="flex h-screen flex-col items-center justify-center p-4"> | ||
| <XCircle className="size-8 text-red-500" /> | ||
| <p className="mt-3 text-sm text-red-500"> | ||
| {error instanceof Error ? error.message : "Failed to load run status"} | ||
| </p> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: Error state is unreachable when there's no cached data.
When useQuery fails and there's no prior cached data, isLoading is false but data is undefined. The guard isLoading || !data on line 34 evaluates to true, so the loading spinner is shown — the error check on line 43 is never reached.
Swap the order or combine the conditions:
🐛 Proposed fix
- if (isLoading || !data) {
+ if (error) {
return (
- <div className="flex h-screen flex-col items-center justify-center p-4">
- <Loader2 className="size-8 animate-spin text-muted-foreground" />
- <p className="mt-3 text-sm text-muted-foreground">Loading run status...</p>
+ <div className="flex h-screen flex-col items-center justify-center p-4">
+ <XCircle className="size-8 text-red-500" />
+ <p className="mt-3 text-sm text-red-500">
+ {error instanceof Error ? error.message : "Failed to load run status"}
+ </p>
</div>
);
}
- if (error) {
+ if (isLoading || !data) {
return (
- <div className="flex h-screen flex-col items-center justify-center p-4">
- <XCircle className="size-8 text-red-500" />
- <p className="mt-3 text-sm text-red-500">
- {error instanceof Error ? error.message : "Failed to load run status"}
- </p>
+ <div className="flex h-screen flex-col items-center justify-center p-4">
+ <Loader2 className="size-8 animate-spin text-muted-foreground" />
+ <p className="mt-3 text-sm text-muted-foreground">Loading run status...</p>
</div>
);
}🤖 Prompt for AI Agents
In `@components/TasksPage/Run/RunPage.tsx` around lines 34 - 52, The guard in
RunPage currently returns the loading UI when isLoading || !data, which masks
the error state because a failed useQuery yields isLoading=false but
data=undefined; reorder or combine the checks so error is evaluated before the
"no data" loading branch: check error first (the error variable from the
useQuery) and render the error UI if present, then handle loading/no-data with
isLoading || !data; update the conditional logic in the RunPage component around
the existing if blocks (the ones referencing isLoading, data, and error)
accordingly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
RunPage now only manages state routing (skeleton, error, loaded). RunDetails handles all loaded data rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
getTaskRunStatusfetch function forGET /api/tasks/runswith typedTaskRunStatusresponseuseTaskRunStatuspolling hook (3s interval, stops on terminal state) followinguseReportDatapatternRunLogsListcomponent with auto-scroll for log entriesRunPagewith full observability UI: status indicator, current step, logs, error display, durationTest plan
pnpm buildpasses type checks/tasks/[runId]🤖 Generated with Claude Code
Summary by CodeRabbit