Skip to content

feat: [2/n] Implement new Workflows List Table#1229

Merged
adhityamamallan merged 11 commits intocadence-workflow:masterfrom
adhityamamallan:pr2-workflows-list-grid
Apr 8, 2026
Merged

feat: [2/n] Implement new Workflows List Table#1229
adhityamamallan merged 11 commits intocadence-workflow:masterfrom
adhityamamallan:pr2-workflows-list-grid

Conversation

@adhityamamallan
Copy link
Copy Markdown
Member

@adhityamamallan adhityamamallan commented Apr 1, 2026

Summary

  • New workflows list grid component:
    • Implemented WorkflowsList component with grid layout, horizontal scrolling, and infinite scroll pagination
    • Added styled components for grid header, rows, cells, and scroll container
  • Data fetching integration:
    • Connected DomainWorkflowsList and DomainWorkflowsArchivalList to useListWorkflows hook
    • Added loading states, error handling, and pagination support

Test plan

Unit tests + ran locally.

Screenshot 2026-04-01 at 12 58 05

On narrow screens + a preview of the placeholder when value is absent

Screen.Recording.2026-04-01.at.15.12.31.mov

@adhityamamallan adhityamamallan force-pushed the pr2-workflows-list-grid branch from 280111b to a2cac35 Compare April 1, 2026 12:09
@adhityamamallan adhityamamallan force-pushed the pr2-workflows-list-grid branch from bbd1b31 to bf9a8b5 Compare April 7, 2026 08:53
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
@adhityamamallan adhityamamallan force-pushed the pr2-workflows-list-grid branch from bf9a8b5 to af431d6 Compare April 7, 2026 12:56
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements the new workflows list as a grid/table component with infinite-scroll pagination, and wires it into the domain workflows (default + archival) views via useListWorkflows.

Changes:

  • Added WorkflowsList grid UI (header/rows/cells), scroll container styling, and footer infinite-scroll loader integration.
  • Introduced getSearchAttributeValue helper and updated column-generation/rendering to support null for missing values (to enable placeholders).
  • Integrated DomainWorkflowsList and DomainWorkflowsArchivalList with useListWorkflows, including loading/error states, plus added/updated unit tests and fixtures.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/views/shared/workflows-list/workflows-list.types.ts Updates list component prop types (removes isLoading, adds pagination fetch state).
src/views/shared/workflows-list/workflows-list.tsx Replaces placeholder with the new grid-based list UI and row linking + infinite scroll footer.
src/views/shared/workflows-list/workflows-list.styles.ts Adds BaseUI styled components for grid layout, scrolling container, and placeholders.
src/views/shared/workflows-list/helpers/get-workflows-list-column-from-search-attribute.ts Uses shared search-attribute value decoding and returns null for missing custom attributes.
src/views/shared/workflows-list/helpers/get-search-attribute-value.ts New helper to decode/format search attribute payload values.
src/views/shared/workflows-list/helpers/tests/get-workflows-list-column-from-search-attribute.test.ts Updates tests to use shared fixtures and to expect null when custom values are missing.
src/views/shared/workflows-list/helpers/tests/get-search-attribute-value.test.ts Adds unit tests for decoding and missing-attribute behavior.
src/views/shared/workflows-list/config/workflows-list-columns.config.ts Updates column renderers to return null for missing values and to use getSearchAttributeValue.
src/views/shared/workflows-list/tests/workflows-list.test.tsx Adds component tests covering headers, rows, links, loader props, and null placeholder behavior.
src/views/shared/workflows-list/fixtures/mock-workflows-list-columns.ts Adds reusable mock columns/column-config fixtures for tests.
src/views/domain-workflows/domain-workflows-list/domain-workflows-list.types.ts Extends props to accept visibleColumns.
src/views/domain-workflows/domain-workflows-list/domain-workflows-list.tsx Wires domain workflows list to useListWorkflows with loading/error handling and passes data into WorkflowsList.
src/views/domain-workflows/domain-workflows-list/tests/domain-workflows-list.test.tsx Updates tests to cover loading and loaded states using MSW.
src/views/domain-workflows/domain-workflows-advanced/domain-workflows-advanced.tsx Fetches visibleColumns via useWorkflowsListColumns and passes them into DomainWorkflowsList.
src/views/domain-workflows-archival/domain-workflows-archival.tsx Fetches visibleColumns via useWorkflowsListColumns and passes them into archival list rendering.
src/views/domain-workflows-archival/domain-workflows-archival-list/domain-workflows-archival-list.types.ts Extends props to accept visibleColumns.
src/views/domain-workflows-archival/domain-workflows-archival-list/domain-workflows-archival-list.tsx Wires archival workflows list to useListWorkflows with loading/error handling and passes data into WorkflowsList.
src/views/domain-workflows-archival/domain-workflows-archival-list/tests/domain-workflows-archival-list.test.tsx Updates tests to validate loaded state using MSW.
Comments suppressed due to low confidence (1)

src/views/shared/workflows-list/config/workflows-list-columns.config.ts:103

  • For datetime search-attribute columns, when the attribute is missing or unparseable value ends up null and this code returns an empty string. That bypasses the table's "None" placeholder behavior (which is triggered by null). Consider returning null instead when value is null/empty or when parsing fails, so missing datetime values render consistently with other columns.
      if (timestamp != null && !isNaN(timestamp)) {
        return createElement(FormattedDate, { timestampMs: timestamp });
      }
      return String(value ?? '');
    },

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{hasWorkflows &&
workflows.map((workflow, index) => (
<styled.GridRow
key={`${workflow.workflowID}-${workflow.runID}-${index}`}
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The React key includes index, which makes keys unstable if the list order changes (e.g., sorting/filter changes) and can cause unnecessary unmount/remount of rows. Prefer a stable unique key derived only from workflow identity (e.g., workflowID + runID).

Suggested change
key={`${workflow.workflowID}-${workflow.runID}-${index}`}
key={`${workflow.workflowID}-${workflow.runID}`}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorting/filter changes cause the entire table to re-render anyway, since new data is being loaded

Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
@adhityamamallan adhityamamallan marked this pull request as ready for review April 7, 2026 13:36
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 8, 2026

Code Review ✅ Approved 6 resolved / 6 findings

Implements the new Workflows List Table component with comprehensive fixes addressing datetime matching, test data alignment, null/undefined placeholder handling, loading state propagation, and array iteration logic. All identified issues have been resolved.

✅ 6 resolved
Bug: DATETIME type matcher will never match — wrong string value

📄 src/views/shared/workflows-list/config/workflows-list-columns.config.ts:100 📄 src/views/shared/workflows-list/config/tests/workflows-list-columns.config.test.ts:27 📄 src/views/shared/workflows-list/config/tests/workflows-list-columns.config.test.ts:45 📄 src/views/shared/workflows-list/config/tests/workflows-list-columns.config.test.ts:148 📄 src/views/shared/workflows-list/config/workflows-list-columns.config.ts:13 📄 src/views/shared/workflows-list/config/workflows-list-columns.config.ts:20 📄 src/views/shared/workflows-list/config/workflows-list-columns.config.ts:101
The catch-all DATETIME matcher at line 100 checks type === 'DATETIME', but the actual IndexedValueType enum values from the API use the prefix format 'INDEXED_VALUE_TYPE_DATETIME'. This means the matcher will never fire for custom datetime search attributes — they'll fall through to the generic formatPayload fallback in get-workflows-list-column-from-search-attribute.ts instead of rendering a FormattedDate component.

The unit tests for this matcher (lines 142-185 in the test file) also pass 'INDEXED_VALUE_TYPE_DATETIME' to match(), which means they will fail at runtime since the condition won't match.

Bug: Test expects old DEFAULT_WORKFLOWS_LIST_COLUMN_WIDTH value

📄 src/views/shared/workflows-list/helpers/tests/get-workflows-list-column-from-search-attribute.test.ts:99 📄 src/views/shared/workflows-list/workflows-list.constants.ts:10
The constant DEFAULT_WORKFLOWS_LIST_COLUMN_WIDTH was changed from '2fr' to '200px' in commit a2cac35, but the test at line 99 of get-workflows-list-column-from-search-attribute.test.ts still asserts '2fr'. This test will fail.

Edge Case: Null placeholder not tested in new WorkflowsList tests

📄 src/views/shared/workflows-list/tests/workflows-list.test.tsx:96-104 📄 src/views/shared/workflows-list/fixtures/mock-workflows-list-columns.ts:35-49 📄 src/views/shared/workflows-list/workflows-list.tsx:49-53 📄 src/views/shared/workflows-list/workflows-list.tsx:49-55
The main feature of this commit is rendering a "None" placeholder when renderCell returns null, but the new test file doesn't cover this behavior. The mock columns in mockWorkflowsListColumns always return truthy values (row.workflowID, row.status), so the <CellPlaceholder> path (line 52) is never exercised.

Consider adding a test with a column whose renderCell returns null and asserting that "None" appears in the rendered output.

Quality: isLoading prop to WorkflowsList is always false

📄 src/views/domain-workflows/domain-workflows-list/domain-workflows-list.tsx:48-49 📄 src/views/domain-workflows/domain-workflows-list/domain-workflows-list.tsx:76 📄 src/views/domain-workflows-archival/domain-workflows-archival-list/domain-workflows-archival-list.tsx:50-51 📄 src/views/domain-workflows-archival/domain-workflows-archival-list/domain-workflows-archival-list.tsx:73 📄 src/views/shared/workflows-list/workflows-list.tsx:13 📄 src/views/shared/workflows-list/workflows-list.tsx:24
Both DomainWorkflowsList and DomainWorkflowsArchivalList return <SectionLoadingIndicator /> when isLoading is true, so WorkflowsList is only ever rendered when isLoading is false. This makes the isLoading prop and the shouldShowResults guard in WorkflowsList dead code on the current render path.

Note: React Query's isLoading is only true for the initial load (not refetches), so it won't flip back to true once WorkflowsList is mounted.

This isn't a bug, but it makes the component contract misleading — readers may think WorkflowsList handles its own loading skeleton when it actually never receives that state.

Edge Case: Strict !== null check won't show placeholder for undefined

📄 src/views/shared/workflows-list/workflows-list.tsx:48
In workflows-list.tsx, the cell placeholder logic uses content !== null (strict equality). Since renderCell returns React.ReactNode (which includes undefined), any column that returns undefined for a missing value will render an empty cell instead of the "None" placeholder. Current column configs happen to avoid this (required fields are always strings, optional fields use ?? null), but the contract is fragile — a future renderCell returning undefined would silently skip the placeholder.

...and 1 more resolved from earlier reviews

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@adhityamamallan adhityamamallan requested a review from Copilot April 8, 2026 16:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@adhityamamallan adhityamamallan merged commit e8043d7 into cadence-workflow:master Apr 8, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants