feat(ui-v2): improve task run details page parity with Vue#38
Conversation
- Add copy button to Task Inputs tab using JsonInput copy prop - Add usePageTitle hook to set browser tab title based on task run name - Add useFavicon hook to change favicon based on task run state - Add state-specific SVG icons for favicon (cancelled, completed, crashed, failed, pending, running, scheduled) - Fix breadcrumb to link to /runs?tab=task-runs instead of just /runs - Show all detail fields consistently with 'None' when empty instead of hiding them - Add Cache Key, Cache Expiration, Retry Delay, and Retry Jitter Factor fields to always display Co-Authored-By: alex.s@prefect.io <ajstreed1@gmail.com>
Co-Authored-By: alex.s@prefect.io <ajstreed1@gmail.com>
Co-Authored-By: alex.s@prefect.io <ajstreed1@gmail.com>
Co-Authored-By: alex.s@prefect.io <ajstreed1@gmail.com>
WalkthroughThese changes enhance the task-run details page with dynamic page title and favicon management based on task run state, introduce a new favicon utility hook, and consolidate task run details rendering logic into standardized display components with consistent fallback handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@ui-v2/src/components/task-runs/task-run-details-page/index.tsx`:
- Around line 380-384: Guard against taskRun.task_inputs being undefined by
ensuring the value prop passed to JsonInput is always a string; update the code
around the JsonInput component (reference: JsonInput and taskRun.task_inputs) to
use a fallback empty string when taskRun.task_inputs is absent (so the value
becomes an empty string instead of undefined before it reaches useCodeMirror).
In `@ui-v2/src/components/task-runs/task-run-details/task-run-details.tsx`:
- Around line 176-181: The component currently hides a valid 0 value because it
uses a truthy check on taskRun.empirical_policy?.retry_jitter_factor; change
that to a nullish check (compare against null/undefined) so
taskRun.empirical_policy.retry_jitter_factor === 0 is shown (i.e., render the
value unless it is null or undefined) in the TaskRunDetails render where
retry_jitter_factor is displayed.
In `@ui-v2/src/hooks/use-state-favicon.ts`:
- Around line 49-63: When stateType becomes null/undefined the effect currently
leaves the previous favicon in place; update the effect in use-state-favicon
(the logic that reads stateType, favicon16, favicon32, and colorScheme) so that
in the branch where stateType is falsy you reset both favicon16 and favicon32 to
the appropriate default (use the dark variants when colorScheme === "dark",
otherwise the light variants), mirroring the cleanup logic; ensure this runs
whenever stateType or colorScheme changes so the icon updates immediately while
the component remains mounted.
- Around line 3-12: The StateType enum and useStateFavicon hook build favicon
paths via `${stateType.toLowerCase()}.svg` (see StateType and the favicon path
construction in use-state-favicon.ts), but cancelling.svg and paused.svg are
missing causing 404s; fix by either adding two SVG files named cancelling.svg
and paused.svg into ui-v2/public/ or update the favicon mapping inside
useStateFavicon to map "CANCELLING" and "PAUSED" to existing icons (e.g., reuse
"running.svg" or "paused-alias.svg") so that faviconPath generation (the code
that lowercases stateType) never points to non-existent assets. Ensure the
change touches StateType handling in use-state-favicon.ts (or the static assets
in public/) so all state values resolve to valid SVGs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
ui-v2/public/cancelled.svgis excluded by!**/*.svgui-v2/public/completed.svgis excluded by!**/*.svgui-v2/public/crashed.svgis excluded by!**/*.svgui-v2/public/failed.svgis excluded by!**/*.svgui-v2/public/pending.svgis excluded by!**/*.svgui-v2/public/running.svgis excluded by!**/*.svgui-v2/public/scheduled.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
ui-v2/src/components/task-runs/task-run-details-page/index.tsxui-v2/src/components/task-runs/task-run-details/task-run-details.tsxui-v2/src/hooks/use-state-favicon.ts
🧰 Additional context used
🧬 Code graph analysis (2)
ui-v2/src/components/task-runs/task-run-details-page/index.tsx (3)
ui-v2/src/hooks/use-page-title.ts (1)
usePageTitle(34-47)ui-v2/src/hooks/use-state-favicon.ts (1)
useStateFavicon(37-66)ui-v2/src/components/ui/json-input.tsx (1)
JsonInput(30-110)
ui-v2/src/components/task-runs/task-run-details/task-run-details.tsx (1)
ui-v2/src/components/ui/icons/icon.tsx (1)
Icon(8-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Pre-commit checks
🔇 Additional comments (3)
ui-v2/src/components/task-runs/task-run-details/task-run-details.tsx (1)
45-175: Consistent detail blocks and fallbacks look good.
The standardized DL sections with explicit “None” values and always-visible fields improve parity and scanning.ui-v2/src/components/task-runs/task-run-details-page/index.tsx (2)
48-77: Nice: dynamic page title + state favicon integration.
Good parity improvement and clearer context when navigating between task runs.
194-198: Breadcrumb now preserves the task-runs tab.
This keeps navigation consistent when returning to the runs page.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <JsonInput | ||
| value={JSON.stringify(taskRun.task_inputs, null, 2)} | ||
| disabled | ||
| copy | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for JsonInput component props interface/type
find ui-v2/src -type f -name "*json*input*" | head -20Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Search for JsonInputProps definition
rg -n "interface JsonInputProps|type JsonInputProps" -A 10 --type ts --type tsx ui-v2/srcRepository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Search for task_inputs definition in the codebase
rg -n "task_inputs" -B 2 -A 2 --type ts --type tsx ui-v2/src | head -100Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Read JsonInput component
cat -n ui-v2/src/components/ui/json-input.tsxRepository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 3627
🏁 Script executed:
#!/bin/bash
# Search for task_inputs in the codebase without file type restrictions
rg -n "task_inputs" ui-v2/src | head -50Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 922
🏁 Script executed:
#!/bin/bash
# Search for TaskRun type definition
rg -n "interface TaskRun|type TaskRun" ui-v2/src -A 10 | head -100Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 8712
🏁 Script executed:
#!/bin/bash
# Get the TaskRun interface definition from prefect.ts
sed -n '10470,10520p' ui-v2/src/api/prefect.tsRepository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 2051
🏁 Script executed:
#!/bin/bash
# Search for where TaskRun is used in the details page component
cat -n ui-v2/src/components/task-runs/task-run-details-page/index.tsx | sed -n '370,390p'Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Check what type taskRun is in the component
rg -B 10 "JSON.stringify(taskRun.task_inputs" ui-v2/src/components/task-runs/task-run-details-page/index.tsx | head -30Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Check how TaskInputs component is used/rendered
rg -n "TaskInputs" ui-v2/src/components/task-runs/task-run-details-page/index.tsx -B 2 -A 2Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 935
🏁 Script executed:
#!/bin/bash
# Check full TaskRun import and its definition
rg -n "import.*TaskRun" ui-v2/src/components/task-runs/task-run-details-page/index.tsx | head -5Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 331
🏁 Script executed:
#!/bin/bash
# Look for any guards or conditions around task_inputs before rendering
cat -n ui-v2/src/components/task-runs/task-run-details-page/index.tsx | sed -n '1,50p'Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 2074
Guard against JSON.stringify returning undefined when task_inputs is absent.
Since task_inputs is optional in the TaskRun type, JSON.stringify(taskRun.task_inputs, null, 2) will return undefined (not a string) when task_inputs is undefined. While JsonInput accepts an optional value, passing undefined to the underlying useCodeMirror could cause unexpected behavior. Use the nullish coalescing operator to default to an empty string:
- <JsonInput
- value={JSON.stringify(taskRun.task_inputs, null, 2)}
+ <JsonInput
+ value={JSON.stringify(taskRun.task_inputs, null, 2) ?? ""}
disabled
copy
/>📝 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.
| <JsonInput | |
| value={JSON.stringify(taskRun.task_inputs, null, 2)} | |
| disabled | |
| copy | |
| /> | |
| <JsonInput | |
| value={JSON.stringify(taskRun.task_inputs, null, 2) ?? ""} | |
| disabled | |
| copy | |
| /> |
🤖 Prompt for AI Agents
In `@ui-v2/src/components/task-runs/task-run-details-page/index.tsx` around lines
380 - 384, Guard against taskRun.task_inputs being undefined by ensuring the
value prop passed to JsonInput is always a string; update the code around the
JsonInput component (reference: JsonInput and taskRun.task_inputs) to use a
fallback empty string when taskRun.task_inputs is absent (so the value becomes
an empty string instead of undefined before it reaches useCodeMirror).
| <dl className="flex flex-col gap-1 mb-2"> | ||
| <dt className="text-gray-500">Retry Jitter Factor</dt> | ||
| <dd className=""> | ||
| {taskRun.empirical_policy?.retry_jitter_factor | ||
| ? taskRun.empirical_policy.retry_jitter_factor.toString() | ||
| : "None"} |
There was a problem hiding this comment.
Show retry jitter factor when it is 0.
The truthy check hides valid 0 values; use a nullish check instead.
🛠️ Proposed fix
- {taskRun.empirical_policy?.retry_jitter_factor
- ? taskRun.empirical_policy.retry_jitter_factor.toString()
- : "None"}
+ {taskRun.empirical_policy?.retry_jitter_factor != null
+ ? taskRun.empirical_policy.retry_jitter_factor.toString()
+ : "None"}📝 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.
| <dl className="flex flex-col gap-1 mb-2"> | |
| <dt className="text-gray-500">Retry Jitter Factor</dt> | |
| <dd className=""> | |
| {taskRun.empirical_policy?.retry_jitter_factor | |
| ? taskRun.empirical_policy.retry_jitter_factor.toString() | |
| : "None"} | |
| <dl className="flex flex-col gap-1 mb-2"> | |
| <dt className="text-gray-500">Retry Jitter Factor</dt> | |
| <dd className=""> | |
| {taskRun.empirical_policy?.retry_jitter_factor != null | |
| ? taskRun.empirical_policy.retry_jitter_factor.toString() | |
| : "None"} |
🤖 Prompt for AI Agents
In `@ui-v2/src/components/task-runs/task-run-details/task-run-details.tsx` around
lines 176 - 181, The component currently hides a valid 0 value because it uses a
truthy check on taskRun.empirical_policy?.retry_jitter_factor; change that to a
nullish check (compare against null/undefined) so
taskRun.empirical_policy.retry_jitter_factor === 0 is shown (i.e., render the
value unless it is null or undefined) in the TaskRunDetails render where
retry_jitter_factor is displayed.
| type StateType = | ||
| | "SCHEDULED" | ||
| | "PENDING" | ||
| | "RUNNING" | ||
| | "COMPLETED" | ||
| | "FAILED" | ||
| | "CANCELLED" | ||
| | "CANCELLING" | ||
| | "CRASHED" | ||
| | "PAUSED"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the actual hook implementation
cat -n ui-v2/src/hooks/use-state-favicon.tsRepository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 2368
🏁 Script executed:
#!/bin/bash
# Search for where SVG files might be located
fd -t f "\.svg$" | head -30Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 1200
🏁 Script executed:
#!/bin/bash
# Check for favicon or assets directory structure
find ui-v2 -type d -name "*asset*" -o -name "*icon*" -o -name "*svg*" 2>/dev/nullRepository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 1243
Add missing SVG assets for CANCELLING and PAUSED states.
The hook constructs favicon paths using /${stateType.toLowerCase()}.svg (line 50). While most state SVGs exist in ui-v2/public/, cancelling.svg and paused.svg are missing and will result in 404 errors when those states are used. Either add the missing SVGs or map these states to existing icons.
🤖 Prompt for AI Agents
In `@ui-v2/src/hooks/use-state-favicon.ts` around lines 3 - 12, The StateType enum
and useStateFavicon hook build favicon paths via
`${stateType.toLowerCase()}.svg` (see StateType and the favicon path
construction in use-state-favicon.ts), but cancelling.svg and paused.svg are
missing causing 404s; fix by either adding two SVG files named cancelling.svg
and paused.svg into ui-v2/public/ or update the favicon mapping inside
useStateFavicon to map "CANCELLING" and "PAUSED" to existing icons (e.g., reuse
"running.svg" or "paused-alias.svg") so that faviconPath generation (the code
that lowercases stateType) never points to non-existent assets. Ensure the
change touches StateType handling in use-state-favicon.ts (or the static assets
in public/) so all state values resolve to valid SVGs.
| if (stateType) { | ||
| const faviconPath = `/${stateType.toLowerCase()}.svg`; | ||
| favicon16?.setAttribute("href", faviconPath); | ||
| favicon32?.setAttribute("href", faviconPath); | ||
| } | ||
|
|
||
| return () => { | ||
| // Reset to default favicon on unmount | ||
| if (colorScheme === "dark") { | ||
| favicon16?.setAttribute("href", "/favicon-16x16-dark.png"); | ||
| favicon32?.setAttribute("href", "/favicon-32x32-dark.png"); | ||
| } else { | ||
| favicon16?.setAttribute("href", "/favicon-16x16.png"); | ||
| favicon32?.setAttribute("href", "/favicon-32x32.png"); | ||
| } |
There was a problem hiding this comment.
Reset favicon when stateType becomes null/undefined.
If stateType clears while the component stays mounted, the prior state icon persists. Consider resetting to defaults in that case.
💡 Proposed fix
useEffect(() => {
const colorScheme = getPreferredColorScheme();
const favicon16 =
colorScheme === "dark"
? document.getElementById("favicon-16-dark")
: document.getElementById("favicon-16");
const favicon32 =
colorScheme === "dark"
? document.getElementById("favicon-32-dark")
: document.getElementById("favicon-32");
- if (stateType) {
- const faviconPath = `/${stateType.toLowerCase()}.svg`;
- favicon16?.setAttribute("href", faviconPath);
- favicon32?.setAttribute("href", faviconPath);
- }
-
- return () => {
- // Reset to default favicon on unmount
- if (colorScheme === "dark") {
- favicon16?.setAttribute("href", "/favicon-16x16-dark.png");
- favicon32?.setAttribute("href", "/favicon-32x32-dark.png");
- } else {
- favicon16?.setAttribute("href", "/favicon-16x16.png");
- favicon32?.setAttribute("href", "/favicon-32x32.png");
- }
- };
+ const resetToDefault = () => {
+ if (colorScheme === "dark") {
+ favicon16?.setAttribute("href", "/favicon-16x16-dark.png");
+ favicon32?.setAttribute("href", "/favicon-32x32-dark.png");
+ } else {
+ favicon16?.setAttribute("href", "/favicon-16x16.png");
+ favicon32?.setAttribute("href", "/favicon-32x32.png");
+ }
+ };
+
+ if (stateType) {
+ const faviconPath = `/${stateType.toLowerCase()}.svg`;
+ favicon16?.setAttribute("href", faviconPath);
+ favicon32?.setAttribute("href", faviconPath);
+ } else {
+ resetToDefault();
+ }
+
+ return resetToDefault;
}, [stateType]);📝 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.
| if (stateType) { | |
| const faviconPath = `/${stateType.toLowerCase()}.svg`; | |
| favicon16?.setAttribute("href", faviconPath); | |
| favicon32?.setAttribute("href", faviconPath); | |
| } | |
| return () => { | |
| // Reset to default favicon on unmount | |
| if (colorScheme === "dark") { | |
| favicon16?.setAttribute("href", "/favicon-16x16-dark.png"); | |
| favicon32?.setAttribute("href", "/favicon-32x32-dark.png"); | |
| } else { | |
| favicon16?.setAttribute("href", "/favicon-16x16.png"); | |
| favicon32?.setAttribute("href", "/favicon-32x32.png"); | |
| } | |
| const resetToDefault = () => { | |
| if (colorScheme === "dark") { | |
| favicon16?.setAttribute("href", "/favicon-16x16-dark.png"); | |
| favicon32?.setAttribute("href", "/favicon-32x32-dark.png"); | |
| } else { | |
| favicon16?.setAttribute("href", "/favicon-16x16.png"); | |
| favicon32?.setAttribute("href", "/favicon-32x32.png"); | |
| } | |
| }; | |
| if (stateType) { | |
| const faviconPath = `/${stateType.toLowerCase()}.svg`; | |
| favicon16?.setAttribute("href", faviconPath); | |
| favicon32?.setAttribute("href", faviconPath); | |
| } else { | |
| resetToDefault(); | |
| } | |
| return resetToDefault; |
🤖 Prompt for AI Agents
In `@ui-v2/src/hooks/use-state-favicon.ts` around lines 49 - 63, When stateType
becomes null/undefined the effect currently leaves the previous favicon in
place; update the effect in use-state-favicon (the logic that reads stateType,
favicon16, favicon32, and colorScheme) so that in the branch where stateType is
falsy you reset both favicon16 and favicon32 to the appropriate default (use the
dark variants when colorScheme === "dark", otherwise the light variants),
mirroring the cleanup logic; ensure this runs whenever stateType or colorScheme
changes so the icon updates immediately while the component remains mounted.
1 participant
Benchmark PR from agentic-review-benchmarks#12
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.