UI updates for accessibility and compliance with WCAG 2.1 #73
Conversation
patclarknvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
|
patclarknvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
patclarknvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
patclarknvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
… loading, and the spinner never says "Loading"
patclarknvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
patclarknvidia
had a problem deploying
to
internal-ci
GitHub Actions
Error
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughComprehensive UI refactoring restructures layout architecture, standardizes spacing tokens globally (gap-3/p-3 → gap-global/p-global), replaces header components (GenericHeader → PageHeader), removes Layout wrappers across routes, introduces status filtering via enums (StatusFilterType) replacing boolean flags, adds row identification to React Tables via getRowId, and enhances accessibility through ARIA attributes and semantic markup across the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant PageHeader as PageHeader<br/>Component
participant PageHeaderProvider as PageHeaderProvider<br/>Context
participant HeaderOutlet as HeaderOutlet<br/>Renderer
participant Route as Route Page
User->>Route: Navigate to page
Route->>PageHeaderProvider: Render with provider wrap
Route->>PageHeader: Mount PageHeader
PageHeader->>PageHeaderProvider: setHeaderNode(headerDOM)<br/>setTitle(title)
PageHeaderProvider->>PageHeaderProvider: Update context state
PageHeaderProvider->>HeaderOutlet: Trigger re-render
HeaderOutlet->>HeaderOutlet: Render current headerNode from context
User->>User: See updated header
Note over PageHeader,HeaderOutlet: Header rendering is decoupled<br/>from route via context provider
sequenceDiagram
participant User as User
participant TaskFilter as TasksFilter<br/>Component
participant StatusFilter as StatusFilter<br/>Component
participant StateUpdate as URL/State<br/>Manager
User->>TaskFilter: Open filter panel
TaskFilter->>StatusFilter: Render with statusFilterType
User->>StatusFilter: Select filter type (ALL/CURRENT/COMPLETED/FAILED/CUSTOM)
alt statusFilterType is CUSTOM
User->>StatusFilter: Toggle individual status checkboxes
StatusFilter->>StatusFilter: Update statusMap<br/>(Map<TaskStatusType, boolean>)
StatusFilter->>TaskFilter: Emit updated statusMap
else statusFilterType is preset
StatusFilter->>StatusFilter: Use predefined status array<br/>via getTaskStatusArray()
end
TaskFilter->>StateUpdate: Submit filters
StateUpdate->>StateUpdate: Derive statuses from<br/>statusFilterType + statusMap
StateUpdate->>User: Update URL with new filters
sequenceDiagram
participant User as User
participant Workflow as Workflow<br/>Page
participant TaskNav as Task<br/>Navigation
participant TaskDetails as TaskDetails<br/>Component
participant FocusManager as Focus<br/>Manager
User->>Workflow: View workflow with multiple tasks
Workflow->>Workflow: Extract flatTasks array
Workflow->>Workflow: Set view = SingleTask
Workflow->>TaskDetails: Render with hasNavigation=true<br/>+ navigation callbacks
User->>TaskDetails: Click Next button
TaskDetails->>TaskNav: Call onNext()
TaskNav->>Workflow: Update URL with next task
Workflow->>Workflow: Increment selectedTaskIndex
Workflow->>TaskDetails: Re-render with new task
Note over Workflow,FocusManager: Focus restoration on<br/>closing task details
User->>Workflow: Close task details
Workflow->>FocusManager: Restore focus to<br/>captured action element<br/>via getActionId()
FocusManager->>User: Return focus to action button
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
ui/src/app/datasets/components/FileBrowser.tsx (1)
304-309: MovesetChonkyDefaultsoutside the render path.
setChonkyDefaultsis a global configuration that should be called once, not on every render. Calling it repeatedly can cause performance issues and unexpected behavior.Move this configuration to module level or inside a
useEffectwith an empty dependency array:+// Module level (outside component) +setChonkyDefaults({ + iconComponent: ChonkyIconFA as any, + disableDragAndDropProvider: true, +}); + const FileBrowser: React.FC<DatasetFileBrowserProps> = ({ dataset, currentVersion, onOpenFile }) => {Or inside the component:
+ useEffect(() => { + setChonkyDefaults({ + iconComponent: ChonkyIconFA as any, + disableDragAndDropProvider: true, + }); + }, []); + if (!isSuccess) { return null; } return ( - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore <ChonkyFileBrowserui/src/components/Multiselect.tsx (2)
73-102: Avoidrole="listitem"on interactive elements like buttons.Applying
role="listitem"to<button>elements (line 80) overrides the button's native semantics and can confuse screen readers. Interactive elements should retain their semantic meaning.Consider one of these approaches:
Option 1 (Recommended): Wrap the button in a proper
<li>element:- <div + <ul className="flex flex-row flex-wrap gap-1 p-1" - role="list" aria-label="Selected options" > {selectedOptions.map((o) => ( - <button - role="listitem" + <li key={o.value}> + <button type="button" className="tag-container" - key={o.value} onClick={() => { setFilter(new Map(filter.set(o.value, false))); searchInputRef.current?.focus(); setLastActionText(`Removed ${o.label}`); setTimeout(() => { setLastActionText(""); }, 3000); }} > <Tag color={Colors.pool} size={TagSizes.xs} > {o.label} <OutlinedIcon name="close" /> </Tag> </button> + </li> ))} - </div> + </ul>Option 2: Use a native
<ul>with<li>elements containing buttons (no role attributes needed).
119-148: Apply the same list/button structure fix to available options.This section has the same semantic issue as the selected options:
role="listitem"is applied to<button>elements (line 127), which overrides native button semantics.Apply the same structural fix recommended for lines 73-102:
- <div + <ul className="flex flex-row flex-wrap gap-1 p-1" - role="list" aria-label="Available options" > {filteredOptions.map((o) => ( - <button + <li key={o.value}> + <button type="button" - role="listitem" className="tag-container" - key={o.value} onClick={() => { setFilter(new Map(filter.set(o.value, true))); searchInputRef.current?.focus(); setLastActionText(`Added ${o.label}`); setTimeout(() => { setLastActionText(""); }, 3000); }} > <Tag color={Colors.pool} size={TagSizes.xs} > <OutlinedIcon name="add" /> {o.label} </Tag> </button> + </li> ))} - </div> + </ul>Note: The timeout cleanup issue mentioned in the previous comment also applies to lines 133-136.
ui/src/app/datasets/[bucket]/[name]/components/PagedFilePreviewer.tsx (1)
35-60: Addaria-labelattributes to icon-only navigation buttons.The "Previous" and "Next" navigation buttons contain only icons and lack
aria-labelattributes. Whiletitleattributes provide tooltips on hover, they aren't reliably announced by screen readers. This is a WCAG 2.4.4 and 4.1.2 compliance gap in a PR specifically targeting accessibility.Apply this diff:
<button className="btn btn-action" onClick={() => { updateIndex(-1); }} title="Previous file" + aria-label="Previous file" > <OutlinedIcon name="keyboard_double_arrow_left" className="text-lg!" /> </button> <h2>{selectedFile.name}</h2> <button className="btn btn-action" onClick={() => { updateIndex(1); }} title="Next file" + aria-label="Next file" > <OutlinedIcon name="keyboard_double_arrow_right" className="text-lg!" /> </button>ui/src/components/RecordBuilder.tsx (3)
87-95: Critical: Use function parameters instead of cleared state variables.Line 93 references
newFieldKeyandnewFieldValue, but these were cleared on lines 91-92. This causes the screen reader announcement to be empty or contain stale values instead of the actual added field.Apply this diff to use the correct variables:
const addField = (fieldKey: string, fieldValue: unknown) => { if (fieldKey && fieldValue) { setFlattenedData({ ...flattenedData, [fieldKey]: fieldValue }); delete deletedFields[fieldKey]; + setLastActionText(`Added ${fieldKey}: ${fieldValue as string}`); setNewFieldKey(""); setNewFieldValue(""); - setLastActionText(`Added ${newFieldKey}: ${newFieldValue}`); } };
133-159: Invalid ARIA:role="listitem"requires parent withrole="list".Line 141 applies
role="listitem"to buttons, but the parent container (line 135) hasrole="group". According to ARIA specification,listitemelements must be contained within an element withrole="list".Apply this diff to fix the ARIA structure:
<div className="flex flex-wrap gap-1" - role="group" + role="list" aria-labelledby="current-labels" >Note: The same issue exists in the "Deleted Labels" section at line 205 (though it doesn't currently have a role attribute, adding listitem roles there would also require the parent to be a list).
240-255:aria-disabledwithoutdisabledallows unwanted interaction.Lines 243 and 251 use
aria-disabledbut not thedisabledattribute on Reset and Save buttons. This means users can still focus and click these buttons even whenisModifiedis false, though the click handlers have guards that will prevent any action. For proper accessibility and UX, these buttons should be fully disabled.Apply this diff to properly disable both buttons:
<button onClick={handleUndoChanges} className="btn btn-secondary" - aria-disabled={!isModified} + disabled={!isModified} > <OutlinedIcon name="undo" /> Reset </button> <button onClick={handleSaveChanges} className="btn btn-primary" - aria-disabled={!isModified} + disabled={!isModified} >ui/src/app/datasets/[bucket]/[name]/components/DatasetVersionDetails.tsx (1)
94-115: Incorrect use of ARIA list roles on interactive elements.Using
role="list"on a container withrole="listitem"on<button>elements is semantically incorrect. Buttons are interactive controls, not list items. This can confuse assistive technologies.Additionally, there's a typo in the aria-label: "Dataset VersionActions" should be "Dataset Version Actions".
Consider using a proper list structure or removing the list semantics:
<div className={`dag-actions body-footer w-full`} - role="list" - aria-label="Dataset VersionActions" + aria-label="Dataset Version Actions" > <button className="btn btn-action whitespace-nowrap" - role="listitem" onClick={() => toolParamUpdater({ tool: ToolType.Tags })} > <OutlinedIcon name="data_object" /> Edit Tags </button> <button className="btn btn-action whitespace-nowrap" - role="listitem" onClick={() => toolParamUpdater({ tool: ToolType.Metadata })} > <OutlinedIcon name="data_object" /> Metadata </button> </div>ui/src/app/taskssummary/page.tsx (1)
336-358: Fixaria-labelledbyreferencing non-existent ID.The
<dl aria-labelledby="total-resources-header">references an ID that doesn't exist. Theheader="Total Resources"prop on SlideOut is a string, not an element with an ID.Either add the ID to the header or remove the invalid reference:
<SlideOut animate={true} id="total-resources" open={showTotalResources} onClose={() => setShowTotalResources(false)} - header="Total Resources" + header={<span id="total-resources-header">Total Resources</span>} className="mr-20 border-t-0" >Or remove the
aria-labelledby:<dl className="grid-cols-2" - aria-labelledby="total-resources-header" >
♻️ Duplicate comments (1)
ui/src/components/RecordBuilder.tsx (1)
205-228: Invalid ARIA:role="listitem"requires parent withrole="list"(duplicate issue).Line 209 has the same ARIA violation as line 141. The
role="listitem"requires a parent withrole="list", but the parent div (line 205) has no role attribute.Apply this diff to fix the ARIA structure:
- <div className="flex flex-wrap gap-1"> + <div className="flex flex-wrap gap-1" role="list">
🟡 Minor comments (11)
ui/src/components/Spinner.tsx-33-33 (1)
33-33: Remove redundantaria-labelattribute from status region.The
gap-globaltoken is correctly defined in your Tailwind configuration and CSS variables, so no action needed there. However, thearia-labelattribute on the status div should be removed as it's redundant. Thearia-labelattribute should be used to provide a text alternative to an element that has no visible text on the screen. Here, the element only renders whendescriptionis truthy (line 37 guard), soaria-label={description ?? "Loading..."}will always duplicate the visible text content. If the element has text content, you DON'T need to add aria-label, as it is redundant. Therole="status"combined with visible text content is sufficient for screen readers.Remove the
aria-labelattribute entirely.ui/src/app/profile/components/ProfileSettings.tsx-31-31 (1)
31-31: Grid columns utility requires display: grid.The
grid-cols-2utility defines the column template but won't take effect withoutgrid(orinline-grid) on the same element.Apply this diff to fix the grid layout:
- <dl className="grid-cols-2"> + <dl className="grid grid-cols-2">ui/src/app/resources/[name]/page.tsx-47-54 (1)
47-54: Incorrectaria-labelon span element.The
aria-label="List"on the span is problematic:
- The value "List" doesn't match the visible content "Task History"
aria-labelon a<span>containing visible text creates a disconnect for screen readersSince the text is hidden on small screens (
hidden lg:block), consider adding a visually-hidden accessible label instead:<Link href={getTaskHistoryUrl(params.name)} className="btn btn-secondary" + aria-label="Task History" > <FilledIcon name="history" /> - <span - className="hidden lg:block" - aria-label="List" - > + <span className="hidden lg:block"> Task History </span> </Link>This ensures the link is always announced as "Task History" regardless of viewport size.
Committable suggestion skipped: line range outside the PR's diff.
ui/src/app/datasets/[bucket]/[name]/components/CollectionDetails.tsx-89-124 (1)
89-124: Incorrect ARIA role for actions container.The actions container uses
role="list"with buttons havingrole="listitem", but a collection of action buttons should use semantic markup (nav or just div) without list roles, or use an actual<ul>element.Apply this diff:
<div - className={`dag-actions body-footer w-full`} - role="list" - aria-label="Collection Actions" + className="dag-actions body-footer w-full" > <button className="btn btn-action whitespace-nowrap" - role="listitem" onClick={() => { toolParamUpdater({ tool: ToolType.Labels }); }} > <OutlinedIcon name="snippet_folder" /> Edit Labels </button> <button className="btn btn-action whitespace-nowrap" - role="listitem" onClick={() => { toolParamUpdater({ tool: ToolType.Delete }); }} > <OutlinedIcon name="delete" /> Delete Collection </button> <button className="btn btn-action whitespace-nowrap" - role="listitem" onClick={() => { toolParamUpdater({ tool: ToolType.Rename }); }} > <OutlinedIcon name="snippet_folder" /> Rename Collection </button> </div>ui/src/app/pools/components/PoolDetails.tsx-131-141 (1)
131-141: Misuse ofrole="list"on grid container.The grid container uses
role="list"but wrapsAggregatePanelswhich itself renders items withrole="listitem". The grid should not have a list role; instead,AggregatePanelsshould handle the list semantics internally (which it already does based on the snippet).Apply this diff:
<div - className="grid lg:grid-cols-2 overflow-x-hidden" - role="list" + className="grid lg:grid-cols-2 overflow-x-hidden" > <AggregatePanels totals={pool.resource_usage} isLoading={false} isShowingUsed={isShowingUsed ?? true} /> </div>ui/src/components/FilterButton.tsx-23-24 (1)
23-24: Props spread could override accessibility label.Spreading
{...props}afteraria-labelallows callers to accidentally override the dynamically computed accessibility label, which includes the active filter count.Apply this diff to protect the aria-label:
+ {...props} className={`relative btn ${showFilters ? "btn-primary" : ""}`} onClick={() => { setShowFilters(!showFilters); }} - {...props} aria-label={`Filters${filterCount > 0 ? ` (${filterCount} active filters)` : ""}`}Committable suggestion skipped: line range outside the PR's diff.
ui/src/app/datasets/components/Datasets.tsx-253-266 (1)
253-266:aria-controlsand SlideOutidmismatch.The
FilterButtonspecifiesaria-controls="datasets-filters", but theSlideOuthasid="workflows-filters". This appears to be a copy-paste artifact. The IDs should match and reflect the current page context (datasets).<FilterButton showFilters={showFilters} setShowFilters={setShowFilters} filterCount={filterCount} aria-controls="datasets-filters" /> </PageHeader> <div className="h-full w-full overflow-x-auto relative"> <SlideOut - id="workflows-filters" + id="datasets-filters" open={showFilters} onClose={() => setShowFilters(false)} className="w-100 border-t-0" aria-label="Datasets Filter" >ui/src/app/resources/page.tsx-166-175 (1)
166-175:aria-controlsID mismatch with SlideOut.The
FilterButtonspecifiesaria-controls="resources-filters"(plural), but theSlideOutcomponent hasid="resources-filter"(singular). These must match for the ARIA relationship to work correctly.<FilterButton showFilters={showFilters} setShowFilters={setShowFilters} filterCount={filterCount} - aria-controls="resources-filters" + aria-controls="resources-filter" />ui/src/app/datasets/[bucket]/[name]/components/ToolsModal.tsx-44-57 (1)
44-57: Misleading headeridfor non-Delete tools.Both branches of the header memo use
id="delete-collection-header", which is semantically incorrect when the tool is not Delete (e.g., Metadata, Tags, Labels, Rename). Consider using a generic or dynamic ID.const header = useMemo(() => { if (tool === ToolType.Delete) { - return <h2 id="delete-collection-header">Delete Collection</h2>; + return <h2 id="tools-modal-header">Delete Collection</h2>; } return ( <h2 className="capitalize" - id="delete-collection-header" + id="tools-modal-header" > {tool} </h2> ); }, [tool]);And update the
aria-labelledbyreference accordingly:- aria-labelledby="delete-collection-header" + aria-labelledby="tools-modal-header"Committable suggestion skipped: line range outside the PR's diff.
ui/src/app/pools/page.tsx-83-92 (1)
83-92: Fixaria-controlsandidmismatch.The FilterButton's
aria-controls="pools-filters"(plural) doesn't match the SlideOut'sid="pools-filter"(singular). This breaks the ARIA relationship for screen readers.<FilterButton showFilters={showFilters} setShowFilters={setShowFilters} filterCount={filterCount} - aria-controls="pools-filters" + aria-controls="pools-filter" />ui/src/app/workflows/[name]/page.tsx-144-154 (1)
144-154: Potential issue:selectedTaskIndexset to-1when task not found.When
findIndexreturns-1(task not found),selectedTaskIndexis set to-1rather thanundefined. This could cause issues innextTask/previousTaskcalculations sinceselectedTaskIndex !== undefinedwould betrueeven when the task isn't found.Apply this diff to handle the not-found case explicitly:
useEffect(() => { if (flatTasks && selectedTaskName && retryId !== undefined) { const taskIndex = flatTasks.findIndex((t) => t.name === selectedTaskName && t.retry_id === retryId); - setSelectedTask(taskIndex === undefined ? undefined : flatTasks[taskIndex]); - setSelectedTaskIndex(taskIndex); + if (taskIndex === -1) { + setSelectedTask(undefined); + setSelectedTaskIndex(undefined); + } else { + setSelectedTask(flatTasks[taskIndex]); + setSelectedTaskIndex(taskIndex); + } } else { setSelectedTask(undefined); setSelectedTaskIndex(undefined); } }, [flatTasks, selectedTaskName, retryId]);
🧹 Nitpick comments (32)
ui/src/app/workflows/components/CancelWorkflow.tsx (1)
92-103: Persistent live region pattern looks good, but consider spacing in error display.The refactored
InlineBannerwith persistentaria-live="polite"region (even when empty) aligns with the PR's accessibility objectives for screen reader announcements. The simplified status prop is cleaner than the previous conditional rendering.However, the error display div (line 96) appears to be missing spacing between the two paragraphs:
<div className="flex flex-col"> <p>Error canceling workflow</p> <p>{error}</p> </div>Consider adding gap spacing for better readability:
-<div className="flex flex-col"> +<div className="flex flex-col gap-2"> <p>Error canceling workflow</p> <p>{error}</p> </div>ui/src/components/Accordion.tsx (1)
84-96: Consider using stable keys instead of array indices.Using array indices as React keys (line 86) can lead to issues if the items array is reordered or items are added/removed dynamically. If
AccordionItemDataitems have or could have a unique identifier, consider adding anidfield to the interface and using it as the key.If stable identifiers are available, apply this pattern:
interface AccordionItemData { + id?: string; title?: string | React.ReactNode; ariaLabel?: string; content: React.ReactNode; slotLeft?: React.ReactNode; className?: string; disabled?: boolean; }{items.map((item, index) => ( <AccordionItem - key={index} + key={item.id ?? index} {...item} isOpen={openIndex === index}ui/src/app/datasets/components/FileBrowser.tsx (2)
232-238: Non-null assertion before guard checks is confusing.Line 232 uses
filesData!before any guard checks (lines 254, 265, 300), which suggests the data is guaranteed to exist when it's not. WhiletransformFilesDatahandles undefined via optional chaining at line 78, this pattern is confusing for maintainers.Consider moving these transformations after the guard checks, or ensure consistent use of optional chaining/type narrowing.
311-313: Redundant guard check.The
isSuccesscheck is unnecessary because line 300 already ensuresfilesDataexists. IffilesDatais defined, the query must have succeeded (isSuccesswould betrue).Consider removing this redundant check:
- if (!isSuccess) { - return null; - } - return (ui/src/app/datasets/components/DatasetPicker.tsx (1)
74-104: Consider adding form labeling for enhanced accessibility.While the Select has a proper
aria-label, the form itself lacks an accessible name. For full WCAG 2.1 compliance, consider wrapping the form content in a<fieldset>with a<legend>describing the form's purpose (e.g., "Select Dataset Tag"). This would provide better context for screen reader users.Example enhancement:
<form onSubmit={handleSubmit} className="p-global w-full" > + <fieldset className="border-0 p-0 m-0"> + <legend className="sr-only">Select Dataset Tag</legend> <div className="grid grid-cols-[1fr_auto] gap-global w-full"> <Select id="tag" aria-label="Tag" value={selectedTag} onChange={(e: React.ChangeEvent<HTMLSelectElement>) => setSelectedTag(e.target.value)} className="w-full" > {Array.from(tags).map((tag) => ( <option key={tag} value={tag} > {tag} </option> ))} </Select> <button className="btn btn-primary" title="Save" type="submit" > Update </button> </div> + </fieldset> </form>Note: The
sr-onlyclass provides visually hidden but screen-reader accessible text, and border/padding resets prevent default fieldset styling.ui/src/components/Multiselect.tsx (1)
116-116: Consideraria-described-byfor broader screen reader compatibility if supporting older or less common assistive technologies.While Chrome, Edge, and Firefox currently appear to have good support for aria-description, screen reader support remains surprisingly limited, with only NVDA and iOS VoiceOver currently handling it well. For environments targeting only modern browsers and commonly-used screen readers, the current approach is acceptable. However, if broader compatibility is needed, consider using
aria-describedbypointing to a visually-hidden live region as a more established pattern with consistent support across assistive technologies.ui/src/app/datasets/[bucket]/[name]/components/PagedFilePreviewer.tsx (1)
62-63: Consider adding accessibility attributes to the file counter.The file position counter could benefit from an
aria-live="polite"attribute or a descriptivearia-labelon its container to announce position changes to screen reader users when navigating between files.Example:
- <p> + <p aria-live="polite" aria-atomic="true"> {selectedIndex + 1}/{files.length} </p>ui/src/components/Breadcrumbs.tsx (1)
24-51: Enhance breadcrumbs accessibility to align with WCAG 2.1 and ARIA best practices.Since this PR focuses on WCAG 2.1 compliance, consider improving the breadcrumbs component to follow the ARIA breadcrumb pattern:
- Wrap in a
<nav>element witharia-label="Breadcrumb"- Add
aria-current="page"to the last breadcrumb item- Mark the chevron icon as
aria-hidden="true"(decorative separator)Apply this diff to enhance accessibility:
const Breadcrumbs = ({ segments }: { segments: ReactNode[] }) => { const count = segments.length; return ( - <div className="flex flex-row gap-global items-center breadcrumbs"> + <nav aria-label="Breadcrumb" className="flex flex-row gap-global items-center breadcrumbs"> {segments.map((segment, index) => index < count - 1 ? ( <Link href={`/${segments.slice(0, index + 1).join("/")}`} key={index} className="flex flex-row gap-1 items-center capitalize" > {segment} <FilledIcon name="chevron_right" className="text-base opacity-50" + aria-hidden="true" /> </Link> ) : ( <div key={index} className="capitalize font-semibold" + aria-current="page" > {segment} </div> ), )} - </div> + </nav> ); };ui/src/components/Spinner.tsx (1)
37-45: Remove redundant nullish coalescing and consider removing duplicatearia-label.Two minor refinements:
- Line 41: The nullish coalescing operator
?? "Loading..."is redundant because Line 37 already ensuresdescriptionis truthy.- Accessibility pattern: When using
role="status"with meaningful text content, adding anaria-labelwith identical text is redundant—screen readers will announce the text content. Thearia-labeltypically overrides the content, so having both with the same value adds unnecessary duplication.Apply this diff to simplify the code:
{description && ( <div className="text-sm" role="status" - aria-label={description ?? "Loading..."} > {description} </div> )}Note: If you prefer to keep explicit labeling, you could alternatively remove the nullish coalescing:
- aria-label={description ?? "Loading..."} + aria-label={description}ui/src/app/workflows/components/WorkflowActions.tsx (1)
52-52: Accessibility improvements look good.The added
idandaria-labelledbyattributes properly associate the action lists with their container. However, both inner lists share the same label viaaria-labelledby, which means screen readers will announce both as "Workflow Actions". Consider adding distinct labels (e.g., "Workflow information" for the first list and "Workflow operations" for the second) to help users distinguish between the two action groups.Optional: Add distinct labels for each action list
<div className="flex flex-col gap-1" role="list" - aria-labelledby="workflow-actions-list" + aria-label="Workflow information" ><div className="flex flex-col gap-1" role="list" - aria-labelledby="workflow-actions-list" + aria-label="Workflow operations" >Also applies to: 57-57, 121-121
ui/src/app/datasets/components/DatasetsFilter.tsx (1)
232-238: Consider conditionally rendering the banner to avoid empty DOM elements.The
InlineBanneris always rendered, even when there are no errors. While this preserves thearia-liveregion for screen reader announcements (which is good for accessibility), it results in an empty container with nested divs whenerrors.length === 0.If the persistent live region is intentional for accessibility, this is fine. Otherwise, consider conditional rendering:
- <InlineBanner status={errors.length > 0 ? "error" : "none"}> - <div className="flex flex-col"> - {errors.map((error, index) => ( - <div key={index}>{error}</div> - ))} - </div> - </InlineBanner> + {errors.length > 0 && ( + <InlineBanner status="error"> + <div className="flex flex-col"> + {errors.map((error, index) => ( + <div key={index}>{error}</div> + ))} + </div> + </InlineBanner> + )}ui/src/app/workflows/components/WorkflowTableRowAction.tsx (1)
31-31: Remove unusedbuttonRef.The
buttonRefis created but no longer used after removing theuseEffectthat handled scroll-into-view behavior.-import { useRef } from "react"; + import { formatForWrapping } from "~/utils/string";export const WorkflowTableRowAction = ({ id, name, selected, updateUrl, extraParams }: WorkflowTableRowActionProps) => { - const buttonRef = useRef<HTMLButtonElement>(null); - return ( <button id={id} className={`btn ${selected ? "btn-primary" : "btn-secondary"} table-action`} - ref={buttonRef} onClick={(event) => {ui/src/app/workflows/components/TaskActions.tsx (1)
39-84: Consider refining the ARIA labeling structure.The current implementation has both inner lists using
aria-labelledby="task-actions-list"to reference the parent container'sid, while the parent also hasaria-label="Task Actions". This creates some redundancy:
- Parent
divhas botharia-label="Task Actions"(line 41) andid="task-actions-list"(line 42)- Both child
role="list"elements reference thisidviaaria-labelledby(lines 47, 83)Optional refinement: Consider one of these approaches:
- Keep the parent's
aria-labeland removearia-labelledbyfrom children (they inherit context)- Create a visible heading with the id and remove the parent's
aria-label- Give each list its own descriptive
aria-label(e.g., "Log Actions", "External Actions")The current implementation is functional but could be clearer.
ui/src/app/workflows/submit/groot/page.tsx (1)
24-26: Consider using the title prop for consistency.The PageHeader component accepts an optional
titleprop. Using it instead of passing an h2 as children would be more consistent with the component's API design.Apply this diff for a cleaner approach:
- <PageHeader> - <h2 className="grow">Groot</h2> - </PageHeader> + <PageHeader title="Groot" />Note: If additional header controls are needed in the future, the children approach provides more flexibility.
ui/src/app/workflows/submit/isaac_sim_sdg/page.tsx (1)
24-26: Consider using the title prop for consistency.Similar to other workflow submit pages, consider using the PageHeader's
titleprop instead of passing an h2 as children for a more consistent API usage.Apply this diff:
- <PageHeader> - <h2 className="grow">Isaac Sim SDG</h2> - </PageHeader> + <PageHeader title="Isaac Sim SDG" />ui/src/app/workflows/submit/reinforcement_learning/page.tsx (1)
21-29: Pattern works correctly, but note the inconsistency with sibling pages.Using
PageHeaderwith anh2child provides good semantic structure. However, this differs fromui/src/app/workflows/submit/page.tsxwhich uses thetitleprop instead. Consider standardizing on one approach across submit pages for consistency—either always use thetitleprop for simple headers or always use children withh2elements.ui/src/app/datasets/[bucket]/[name]/components/DatasetDetails.tsx (2)
33-45: Good semantic structure with section and heading.The use of
<section>witharia-labelledbyand a proper heading hierarchy improves accessibility. However, thearia-labelledbyon the<dl>element (line 45) referencing the same title is redundant since the<dl>is already within the labeled section.<dl className="p-global grow" - aria-labelledby="dataset-details-title" >
92-96: Reconsiderrole="list"for action buttons.Using
role="list"withrole="listitem"on action buttons is semantically misleading. These are interactive actions, not list items. Consider using a more appropriate pattern:<div className={`dag-actions body-footer w-full`} - role="list" - aria-label="Dataset Actions" + role="group" + aria-label="Dataset Actions" >And remove
role="listitem"from the buttons (lines 99, 109).ui/src/app/taskssummary/components/TasksTable.tsx (1)
155-155: Consider extracting ID generation for consistency.Unlike
ui/src/app/tasks/components/TasksTable.tsxwhich exports agetActionIdhelper, this file hardcodes the ID inline. For consistency and testability across the codebase, consider extracting the ID generation logic.Example refactor:
export const getWorkflowActionId = (workflowId: string) => { return `workflow-${workflowId}`; };Then use it in the cell renderer:
<WorkflowTableRowAction - id={`workflow-${row.original.workflow_id}`} + id={getWorkflowActionId(row.original.workflow_id)} name={row.original.workflow_id}ui/src/app/workflows/components/TaskTableRowAction.tsx (1)
16-44: UnuseduseRefandbuttonRef.The
buttonRefis declared and assigned to the button but never used elsewhere in the component. The auto-scroll behavior that previously used this ref was removed. Consider removing the unused ref to clean up the code.-import { useRef } from "react"; +export const TaskTableRowAction = ({ id, name, retry_id, lead, selected, verbose, updateUrl, extraParams, }: TaskTableRowActionProps) => { - const buttonRef = useRef<HTMLButtonElement>(null); return ( <button id={id} className={`btn ${selected ? "btn-primary disabled:opacity-100" : "btn-secondary"} relative border-gray-400 enabled:hover:border-black`} - ref={buttonRef} onClick={() => {ui/src/app/datasets/[bucket]/[name]/components/DatasetOverview.tsx (1)
134-139: Consider addingaria-labelto the Versions IconButton.For accessibility, the IconButton should have an
aria-labelto describe its action, especially since it controls the versions modal.<IconButton className="btn btn-primary" onClick={() => toolParamUpdater({ showVersions: true })} icon="layers" text="Versions" + aria-label="Show all versions" + aria-haspopup="dialog" />ui/src/app/workflows/components/TaskDetails.tsx (1)
72-103: Consider usingdisabledattribute alongsidearia-disabled.The navigation buttons use
aria-disabledbut remain clickable (the click handler checks the condition). For better accessibility, consider using thedisabledattribute which prevents interaction entirely, or add visual styling to indicate the disabled state.<button className="no-underline p-0 m-1" onClick={() => { if (hasPrevious) { onPrevious?.(); } }} title="Previous Task" aria-disabled={!hasPrevious} + disabled={!hasPrevious} >Alternatively, keep
aria-disabledbut add conditional styling:<button - className="no-underline p-0 m-1" + className={`no-underline p-0 m-1 ${!hasPrevious ? "opacity-50 cursor-not-allowed" : ""}`}ui/src/app/workflows/page.tsx (1)
312-318: Consider adding a comment explaining the 500ms timeout rationale.The 500ms delay is presumably to allow the slide-out close animation to complete before focusing. While this works, it couples the timing to the animation duration. A minor documentation note would help future maintainers understand this dependency.
onClose={() => { setSafeTimeout(() => { - // focus-trap-react does not work well with useReactTable as the ref for button inside the table that triggered the slideout is not persistent + // focus-trap-react does not work well with useReactTable as the ref for button inside the table that triggered the slideout is not persistent. + // The 500ms delay allows the slide-out close animation to complete before restoring focus. const el = focusReturnIdRef.current ? document.getElementById(focusReturnIdRef.current) : undefined; el?.focus(); }, 500); updateUrl({ workflow: null }); }}ui/src/app/workflows/components/StatusFilter.tsx (1)
53-91: Consider adding accessible labeling to the checkbox group.For screen reader users, wrapping the custom checkboxes in a
fieldsetwith alegendwould provide better context about what the checkboxes represent.return ( <div className={className}> <StatusFilterCommon statusFilterType={statusFilterType} setStatusFilterType={setStatusFilterType} /> {statusFilterType === StatusFilterType.CUSTOM && + <fieldset> + <legend className="sr-only">Custom workflow status filters</legend> WorkflowStatusValues.map((name) => { const checked = Boolean(statusMap.get(name)); return ( <CheckboxWithLabel key={name} label={name} checked={checked} containerClassName="p-1" onChange={(event) => { const newMap = new Map(statusMap); newMap.set(name, Boolean(event.target.checked)); setStatusMap(newMap); }} /> ); })} + </fieldset> + } </div> );ui/src/app/workflows/hooks/useToolParamUpdater.ts (1)
149-149: Casting URL param directly to ViewType may cause issues.If an invalid or malicious value is passed in the URL, casting
as ViewTypedoesn't validate it. Consider validating against known values.- setView(urlParams.get(PARAM_KEYS.view) as ViewType); + const viewParam = urlParams.get(PARAM_KEYS.view); + setView(Object.values(ViewType).includes(viewParam as ViewType) ? viewParam as ViewType : undefined);ui/src/app/tasks/components/StatusFilter.tsx (1)
20-51: Consider consolidating duplicate status filter utilities.This file closely mirrors
ui/src/app/workflows/components/StatusFilter.tsxwith the same structure but different status values. A generic utility accepting status values as a parameter could reduce duplication.Example approach for a shared utility:
// ~/components/StatusFilterUtils.ts export const createStatusArrayGetter = <T extends string>( allValues: readonly T[], currentStatuses: T[], failedPredicate: (status: T) => boolean, additionalFailedStatuses: T[] = [] ) => ( statusFilterType?: StatusFilterType, statusMap?: Map<T, boolean> ): string[] => { if (statusFilterType === StatusFilterType.ALL) return [...allValues]; if (statusFilterType === StatusFilterType.CURRENT) return currentStatuses; if (statusFilterType === StatusFilterType.COMPLETED) return ["COMPLETED"]; if (statusFilterType === StatusFilterType.FAILED) { return allValues.filter(failedPredicate).concat(additionalFailedStatuses as T[]); } if (!statusFilterType || !statusMap) return []; return Array.from(statusMap.entries()) .filter(([_, enabled]) => enabled) .map(([status]) => status); };This is optional given the PR's accessibility focus, but worth considering for future maintainability.
ui/src/app/workflows/components/TasksFilter.tsx (2)
122-136: Reset setsStatusFilterType.ALLbut verify this matches expected defaults.The
handleResetfunction setslocalStatusFilterTypetoStatusFilterType.ALLand resets all local state. However, theupdateUrlcall (lines 130-135) doesn't explicitly setstatusFilterTypein the URL parameters. This could cause a mismatch between local state and URL state after reset.Consider including
statusFilterTypein the reset URL update:updateUrl({ filterName: null, nodes: undefined, status: undefined, pod_ip: undefined, + statusFilterType: StatusFilterType.ALL, });
194-204: Avoid rendering empty content inside InlineBanner.When there are no errors, an empty string is rendered (line 202). This results in the
InlineBannerstill being rendered withstatus="none", which may still occupy space due tomin-h-10in the component's styling.Consider conditionally rendering the banner only when errors exist:
- <InlineBanner status={errors.length > 0 ? "error" : "none"}> - {errors.length > 0 ? ( - <div className="flex flex-col"> - {errors.map((error, index) => ( - <div key={index}>{error}</div> - ))} - </div> - ) : ( - "" - )} - </InlineBanner> + {errors.length > 0 && ( + <InlineBanner status="error"> + <div className="flex flex-col"> + {errors.map((error, index) => ( + <div key={index}>{error}</div> + ))} + </div> + </InlineBanner> + )}ui/src/app/workflows/[name]/page.tsx (1)
403-409: Consider usingaria-liveonly when content changes dynamically.The "No tasks found" message has
role="alert", which announces immediately to screen readers. If this is displayed on initial load rather than after a user action, consider usingrole="status"instead to avoid disruptive announcements.ui/src/app/tasks/page.tsx (1)
430-436: Magic number timeout for focus restoration.The 500ms timeout for focus restoration is a workaround for DOM timing issues. Consider extracting this to a named constant and adding a comment explaining why this specific duration was chosen, or using a more robust approach like
requestAnimationFramewith a fallback.+const FOCUS_RESTORE_DELAY_MS = 500; // Allow DOM to settle after slideout close animation + // ... in component returnFocusOnDeactivate={false} onClose={() => { setSafeTimeout(() => { // focus-trap-react does not work well with useReactTable as the ref for button inside the table that triggered the slideout is not persistent const el = focusReturnIdRef.current ? document.getElementById(focusReturnIdRef.current) : undefined; el?.focus(); - }, 500); + }, FOCUS_RESTORE_DELAY_MS);ui/src/app/tasks/components/TasksFilters.tsx (2)
252-280: Reset logic retains currentnodesvalue inlocalNodesinitialization.In
handleReset,initNodes(nodes, poolNodes ?? [])is called with the currentnodesprop value (line 261), which means local node selections may not fully reset ifnodesstill contains values from the URL. Consider passing an empty string to clear selections:- setLocalNodes(initNodes(nodes, poolNodes ?? [])); + setLocalNodes(initNodes("", poolNodes ?? []));
383-389: Same InlineBanner pattern as TasksFilter - consider consistency.This follows the same pattern as
TasksFilter.tsxwhere the banner is always rendered. For consistency and to avoid empty banner rendering, consider the conditional render pattern suggested earlier.- <InlineBanner status={errors.length > 0 ? "error" : "none"}> - <div className="flex flex-col"> - {errors.map((error, index) => ( - <div key={index}>{error}</div> - ))} - </div> - </InlineBanner> + {errors.length > 0 && ( + <InlineBanner status="error"> + <div className="flex flex-col"> + {errors.map((error, index) => ( + <div key={index}>{error}</div> + ))} + </div> + </InlineBanner> + )}
ui/src/app/datasets/[bucket]/[name]/components/DeleteCollection.tsx
Outdated
Show resolved
Hide resolved
elookpotts-nvidia
left a comment
elookpotts-nvidia
left a comment
There was a problem hiding this comment.
First pass - seems like layout files are now unused except for page titles which can be moved into the page.tsx.
patclarknvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
patclarknvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
patclarknvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
Description
Many UI improvements and Accessibility compliance changes.
Issue - None
Checklist
Summary by CodeRabbit
Release Notes
New Features
Accessibility Improvements
UI/UX Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.