feat(ui-v2): improve task run details page parity with Vue#179
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>
Review Summary by QodoImprove task run details page parity with Vue
WalkthroughsDescription• Add useStateFavicon hook to dynamically set favicon based on task run state • Add usePageTitle hook to set browser tab title with task run name • Enable copy functionality for Task Inputs JSON display • Fix breadcrumb navigation to include task-runs tab parameter • Display all detail fields consistently with 'None' for empty values • Always show Cache Key, Cache Expiration, Retry Delay, and Retry Jitter Factor fields Diagramflowchart LR
A["Task Run Details Page"] --> B["usePageTitle Hook"]
A --> C["useStateFavicon Hook"]
A --> D["Task Inputs Component"]
A --> E["Breadcrumb Navigation"]
A --> F["Task Run Details Display"]
C --> G["State-based Favicon SVG"]
B --> H["Browser Tab Title"]
D --> I["Copy Button"]
E --> J["Task-runs Tab Filter"]
F --> K["Consistent None Display"]
File Changes1. ui-v2/src/hooks/use-state-favicon.ts
|
Code Review by Qodo
1. Broken favicon reset paths
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+55
to
+63
| 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.
1. Broken favicon reset paths 🐞 Bug ✓ Correctness
useStateFavicon cleanup resets favicon hrefs to /favicon-16x16*.png and /favicon-32x32*.png, but the app’s default favicons are under /ico/..., so navigating away can leave the favicon pointing at wrong URLs for the rest of the session.
Agent Prompt
### Issue description
`useStateFavicon` resets favicon hrefs to root-level `/favicon-16x16*.png` and `/favicon-32x32*.png`, but the app’s default favicon link elements point to `/ico/...`. This causes the favicon to break after leaving the page (and during effect cleanup between state changes).
### Issue Context
The hook targets link elements by id (`favicon-16`, `favicon-32`, `favicon-16-dark`, `favicon-32-dark`) which are defined in `ui-v2/index.html` and currently use `/ico/` paths.
### Fix Focus Areas
- ui-v2/src/hooks/use-state-favicon.ts[55-63]
- ui-v2/index.html[5-10]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+14
to
+53
| function getPreferredColorScheme(): "dark" | "light" | "no-preference" { | ||
| if (window.matchMedia("(prefers-color-scheme: dark)").matches) { | ||
| return "dark"; | ||
| } | ||
| if (window.matchMedia("(prefers-color-scheme: light)").matches) { | ||
| return "light"; | ||
| } | ||
| return "no-preference"; | ||
| } | ||
|
|
||
| /** | ||
| * A hook that sets the browser favicon based on the provided state type. | ||
| * Resets the favicon to the default when the component unmounts. | ||
| * | ||
| * @param stateType - The state type to display in the favicon (e.g., "COMPLETED", "FAILED") | ||
| * @returns void | ||
| * | ||
| * @example | ||
| * ```tsx | ||
| * // Set favicon based on task run state | ||
| * useStateFavicon(taskRun.state_type); | ||
| * ``` | ||
| */ | ||
| export function useStateFavicon(stateType: StateType | null | undefined): void { | ||
| 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); | ||
| } |
There was a problem hiding this comment.
2. No-preference icon not updated 🐞 Bug ⛯ Reliability
When prefers-color-scheme is neither dark nor light, useStateFavicon still only updates the light/dark favicon link elements (which are gated by media=(prefers-color-scheme:...)), leaving the no-preference favicon link unchanged so state-based favicons may not appear.
Agent Prompt
### Issue description
`useStateFavicon` only updates the light/dark favicon link elements by id. If the browser is in `prefers-color-scheme: no-preference`, the app’s dedicated no-preference favicon link is not updated, so the state favicon may not show.
### Issue Context
`ui-v2/index.html` includes a favicon link with `media=(prefers-color-scheme:no-preference)` that has no id.
### Fix Focus Areas
- ui-v2/src/hooks/use-state-favicon.ts[14-53]
- ui-v2/index.html[12-14]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+176
to
+182
| <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"} | ||
| </dd> |
There was a problem hiding this comment.
3. Jitter factor zero hidden 🐞 Bug ✓ Correctness
TaskRunDetails uses a truthiness check for retry_jitter_factor, so a valid value of 0 is rendered as "None" instead of "0".
Agent Prompt
### Issue description
The Retry Jitter Factor field treats `0` as falsy and displays `"None"`, even though `0` is a valid numeric value.
### Issue Context
The API type is `number | null`, so 0 must be rendered.
### Fix Focus Areas
- ui-v2/src/components/task-runs/task-run-details/task-run-details.tsx[176-182]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Benchmark PR from agentic-review-benchmarks#12