Repository navigation
feat: add saved workflow views - #2497
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds shared, workspace-scoped workflow views. The API and persistence layers support workflow filters, sorting, and default views. The DAG page and table support selecting, saving, updating, resetting, defaulting, and deleting views with URL synchronization. ChangesScoped Workflow View Contracts and Persistence
Workflow View UI and DAG Flow
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant DAGsPage
participant WorkflowViewSelector
participant ViewsAPI
participant DAGTable
Browser->>DAGsPage: Open DAG page with URL filters
DAGsPage->>ViewsAPI: Load scoped workflow views
DAGsPage->>DAGsPage: Resolve active filters and view
DAGsPage->>DAGTable: Render active view and result count
WorkflowViewSelector->>DAGsPage: Select or mutate workflow view
DAGsPage->>ViewsAPI: Save, update, default, or delete view
DAGsPage->>DAGTable: Refresh workflow view state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 2
🧹 Nitpick comments (7)
ui/src/pages/dags/index.tsx (2)
514-544: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated filter-patch logic.
searchTextChange,searchLabelsChange, andhandleSortChangerepeat the same four steps: clone fromcurrentFiltersRef, write the ref, set component state, then sync the URL. A single helper removes the duplication and keeps future fields consistent.♻️ Proposed refactor
+ const patchFilters = React.useCallback( + (patch: Partial<DAGDefinitionsFilters>) => { + const next = cloneFilters({ ...currentFiltersRef.current, ...patch }); + currentFiltersRef.current = next; + setSearchText(next.searchText); + setSearchLabels(next.searchLabels); + setSortField(next.sortField); + setSortOrder(next.sortOrder); + updateFilterLocation(next, activeWorkflowViewId); + }, + [activeWorkflowViewId, updateFilterLocation] + ); + + const searchTextChange = (nextSearchText: string) => + patchFilters({ searchText: nextSearchText }); + + const searchLabelsChange = (labels: string[]) => + patchFilters({ searchLabels: labels }); + + const handleSortChange = (field: string, order: string) => + patchFilters({ sortField: field, sortOrder: order });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/dags/index.tsx` around lines 514 - 544, Extract the repeated filter update flow from searchTextChange, searchLabelsChange, and handleSortChange into a shared helper that patches currentFiltersRef.current, updates the relevant component state, and calls updateFilterLocation with activeWorkflowViewId. Refactor each handler to use the helper while preserving its existing field updates and labels copy behavior.
88-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider omitting empty filter parameters from the URL.
For the default state the URL becomes
?search=&labels=&sort=name&order=asc. The emptysearchandlabelsparams add noise to shared links. Set each param only when it carries a value.Keep
sortandorderunconditional if you rely on them to mark an explicit URL filter.♻️ Proposed change
if (viewId) { params.set('view', viewId); } - params.set('search', filters.searchText); - params.set('labels', filters.searchLabels.join(',')); + if (filters.searchText) { + params.set('search', filters.searchText); + } + if (filters.searchLabels.length > 0) { + params.set('labels', filters.searchLabels.join(',')); + } params.set('sort', filters.sortField); params.set('order', filters.sortOrder);Note: removing the empty
searchparam changes precedence, becauseparams.has('search')no longer reports an explicit filter. Verify the default-view precedence tests still pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/dags/index.tsx` around lines 88 - 98, Update the URL parameter handling in the viewId branch of the DAG page so search and labels are set only when filters.searchText or filters.searchLabels.join(',') has a value, while keeping sort and order unconditional. Preserve the ALL_WORKFLOWS_VIEW_PARAM behavior and verify default-view precedence logic relying on params.has('search') remains correct.ui/src/pages/dags/__tests__/index.test.tsx (2)
481-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the active view after the save round-trip.
updatePreferenceMockrecords the payload but never updatesuserPreferences, so the component never observes the saved view. The test therefore cannot detect a regression where the newly saved view fails to become active. Write the payload back intouserPreferences.workflowFilterViews, re-render, and assertactive-workflow-view.💚 Proposed addition
expect(scope.defaultViewId).toBe(scope.views[0].id); + + userPreferences.workflowFilterViews = storedPreferences; + expect(screen.getByTestId('active-workflow-view')).toHaveTextContent( + scope.views[0].id + );Adjust for re-render if the mocked preferences object requires a new render pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/dags/__tests__/index.test.tsx` around lines 481 - 498, Update the save-view test around updatePreferenceMock so its mocked preference payload is written back to userPreferences.workflowFilterViews, then re-render the component if needed to reflect the updated preferences. Assert the active-workflow-view element after this save round-trip, while preserving the existing stored-view payload assertions.
56-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the DAGTable mock to cover the remaining view handlers.
The mock exposes only
activeWorkflowViewId,onSaveWorkflowView, andonShowAllWorkflows.onUpdateWorkflowView,onResetWorkflowView,onSetDefaultWorkflowView, andonDeleteWorkflowVieware new handlers inui/src/pages/dags/index.tsx(lines 557-615) and have no coverage. Add controls for them and assert the persisted preference payload plus the resulting active view.A delete test is the most valuable one, because deleting the active view must also fall back to the "all workflows" state.
As per coding guidelines: "Add or update tests appropriate to the changed code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/dags/__tests__/index.test.tsx` around lines 56 - 93, Extend the DAGTable mock component in the test setup to accept and expose onUpdateWorkflowView, onResetWorkflowView, onSetDefaultWorkflowView, and onDeleteWorkflowView controls. Add tests that invoke these handlers, verify the persisted preference payload and active view updates, and prioritize deletion of the active view asserting fallback to the “all workflows” state.Source: Coding guidelines
ui/src/contexts/UserPreference.tsx (1)
8-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one shared source of truth for workflow-view types.
The supplied graph context shows duplicate workflow-view type declarations in
ui/src/pages/dags/index.tsx,ui/src/features/dags/components/dag-list/WorkflowViewSelector.tsx,ui/src/features/dags/components/dag-list/DAGTable.tsx, and the selector tests. Move these types to one neutral module and import them from the preference context and all consumers. Otherwise, the persisted schema and component contracts can drift independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/contexts/UserPreference.tsx` around lines 8 - 30, Move WorkflowFilterSet, WorkflowFilterView, WorkflowFilterViewScope, and WorkflowFilterViewPreferences from UserPreference.tsx into one neutral shared types module, then import and reuse those definitions in UserPreference.tsx, pages/dags/index.tsx, WorkflowViewSelector.tsx, DAGTable.tsx, and the selector tests. Remove the duplicate local declarations while preserving the existing persisted schema and component contracts.ui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsx (1)
169-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the card-view recovery action directly.
Line 170 clicks the first matching button. The desktop branch precedes the card branch in DOM order, so this test does not verify the changed card action. Render with a narrow
PanelWidthContextand query the card button directly.As per coding guidelines, add or update frontend unit tests appropriate to the changed code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsx` around lines 169 - 172, The test currently clicks the first button matching "Show all workflows" which targets the desktop branch, not the card branch that was changed. Update the test to render with a narrow PanelWidthContext to trigger card view, then query for and click the card button directly instead of using getAllByRole with [0] index. Verify that onShowAllWorkflows is called from the card-specific button interaction.Source: Coding guidelines
ui/src/features/dags/components/dag-list/WorkflowViewSelector.tsx (1)
124-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse compact sizing for the new selector and dialogs.
Line 127 uses
h-9for a select-like control. The new dialogs do not set compact padding, and the save form addspy-5. Useh-7controls and explicitp-3andpy-3spacing.Proposed compact styles
- className="h-9 min-w-[190px] max-w-[280px] justify-start px-3" + className="h-7 min-w-[190px] max-w-[280px] justify-start px-2" @@ - <DialogContent className="sm:max-w-[440px]"> + <DialogContent className="p-3 sm:max-w-[440px]"> @@ - <div className="space-y-4 py-5"> + <div className="space-y-3 py-3"> @@ placeholder="Production operations" onChange={(event) => setViewName(event.target.value)} + className="h-7 py-1" @@ - <DialogContent className="sm:max-w-[520px]"> + <DialogContent className="p-3 sm:max-w-[520px]">As per coding guidelines, use compact controls and
p-2orp-3modal spacing inui/**/*.tsx.Also applies to: 209-264, 265-273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-list/WorkflowViewSelector.tsx` around lines 124 - 146, Update the WorkflowViewSelector Button control to use compact sizing by changing the className from h-9 to h-7 and adding explicit p-3 padding. Additionally, update the dialogs referenced at lines 209-264 and 265-273 to use compact padding with p-3 and py-3 spacing instead of py-5, ensuring all new controls and modal forms follow the compact sizing guidelines for ui/**/*.tsx components.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/src/contexts/UserPreference.tsx`:
- Line 58: Add Vitest coverage for the UserPreference provider’s persistence
contract in UserPreference.tsx: verify loading legacy user_preferences records
without workflowFilterViews falls back to the default scope map, and verify
updatePreference('workflowFilterViews', ...) persists a scoped view. Keep the
tests focused on provider load/save behavior rather than relying on existing DAG
tests.
In `@ui/src/features/dags/components/dag-list/WorkflowViewSelector.tsx`:
- Around line 173-175: Update the long-text styling at all four sites: in
WorkflowViewSelector.tsx lines 173-175 and 303-305, replace truncate on the menu
and management row name elements with whitespace-normal break-words; in
DAGTable.tsx lines 1482-1484 and 1596-1598, add whitespace-normal break-words to
the desktop and card empty-state descriptions.
---
Nitpick comments:
In `@ui/src/contexts/UserPreference.tsx`:
- Around line 8-30: Move WorkflowFilterSet, WorkflowFilterView,
WorkflowFilterViewScope, and WorkflowFilterViewPreferences from
UserPreference.tsx into one neutral shared types module, then import and reuse
those definitions in UserPreference.tsx, pages/dags/index.tsx,
WorkflowViewSelector.tsx, DAGTable.tsx, and the selector tests. Remove the
duplicate local declarations while preserving the existing persisted schema and
component contracts.
In `@ui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsx`:
- Around line 169-172: The test currently clicks the first button matching "Show
all workflows" which targets the desktop branch, not the card branch that was
changed. Update the test to render with a narrow PanelWidthContext to trigger
card view, then query for and click the card button directly instead of using
getAllByRole with [0] index. Verify that onShowAllWorkflows is called from the
card-specific button interaction.
In `@ui/src/features/dags/components/dag-list/WorkflowViewSelector.tsx`:
- Around line 124-146: Update the WorkflowViewSelector Button control to use
compact sizing by changing the className from h-9 to h-7 and adding explicit p-3
padding. Additionally, update the dialogs referenced at lines 209-264 and
265-273 to use compact padding with p-3 and py-3 spacing instead of py-5,
ensuring all new controls and modal forms follow the compact sizing guidelines
for ui/**/*.tsx components.
In `@ui/src/pages/dags/__tests__/index.test.tsx`:
- Around line 481-498: Update the save-view test around updatePreferenceMock so
its mocked preference payload is written back to
userPreferences.workflowFilterViews, then re-render the component if needed to
reflect the updated preferences. Assert the active-workflow-view element after
this save round-trip, while preserving the existing stored-view payload
assertions.
- Around line 56-93: Extend the DAGTable mock component in the test setup to
accept and expose onUpdateWorkflowView, onResetWorkflowView,
onSetDefaultWorkflowView, and onDeleteWorkflowView controls. Add tests that
invoke these handlers, verify the persisted preference payload and active view
updates, and prioritize deletion of the active view asserting fallback to the
“all workflows” state.
In `@ui/src/pages/dags/index.tsx`:
- Around line 514-544: Extract the repeated filter update flow from
searchTextChange, searchLabelsChange, and handleSortChange into a shared helper
that patches currentFiltersRef.current, updates the relevant component state,
and calls updateFilterLocation with activeWorkflowViewId. Refactor each handler
to use the helper while preserving its existing field updates and labels copy
behavior.
- Around line 88-98: Update the URL parameter handling in the viewId branch of
the DAG page so search and labels are set only when filters.searchText or
filters.searchLabels.join(',') has a value, while keeping sort and order
unconditional. Preserve the ALL_WORKFLOWS_VIEW_PARAM behavior and verify
default-view precedence logic relying on params.has('search') remains correct.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 87c8f986-e3f9-4e7e-b301-75414f2f7d9a
📒 Files selected for processing (7)
ui/src/contexts/UserPreference.tsxui/src/features/dags/components/dag-list/DAGTable.tsxui/src/features/dags/components/dag-list/WorkflowViewSelector.tsxui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsxui/src/features/dags/components/dag-list/__tests__/WorkflowViewSelector.test.tsxui/src/pages/dags/__tests__/index.test.tsxui/src/pages/dags/index.tsx
|
Addressed the valid review feedback in
I kept the workflow-view types in |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
ui/src/contexts/UserPreference.tsx (1)
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer key selection over deleting one legacy field.
loadPreferencesspreads the whole parsed object into state. Any other stale key from an older build also survives and is written back byupdatePreferenceat Line 62. Deleting onlyworkflowFilterViewsfixes one case. Selecting the known keys removes the class of problem and types the parsed value.♻️ Proposed refactor
- const preferences = JSON.parse(saved); - delete preferences.workflowFilterViews; - return { ...defaultPreferences, ...preferences }; + const parsed = JSON.parse(saved) as Partial<UserPreferences> | null; + if (!parsed || typeof parsed !== 'object') { + return defaultPreferences; + } + const next = { ...defaultPreferences }; + for (const key of Object.keys(defaultPreferences) as Array< + keyof UserPreferences + >) { + if (parsed[key] !== undefined) { + next[key] = parsed[key] as UserPreferences[typeof key]; + } + } + return next;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/contexts/UserPreference.tsx` around lines 42 - 44, Update loadPreferences to parse saved data into a typed preferences value and construct the result by selecting only the known preference keys, rather than spreading the entire parsed object or deleting just workflowFilterViews. Preserve defaultPreferences as the fallback for missing recognized values and ensure updatePreference only receives the sanitized preference shape.ui/src/features/dags/components/dag-list/DAGTable.tsx (1)
1494-1510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared empty state.
The table empty state at Lines 1491-1511 and the card empty state at Lines 1610-1628 now render the same heading,
emptyStateDescription, conditionalShow all workflowsbutton, andCreateDAGModal. Both copies must change together. A localWorkflowsEmptyStatecomponent inside this file would keep them in sync.function WorkflowsEmptyState({ description, isAllWorkflowsView, onShowAllWorkflows, }: { description: string; isAllWorkflowsView: boolean; onShowAllWorkflows: () => void; }) { return ( <> <div className="text-6xl mb-4">🔍</div> <h3 className="text-lg font-medium text-foreground mb-2"> No workflows found </h3> <p className="text-sm text-muted-foreground text-center max-w-md mb-4 whitespace-normal break-words"> {description} </p> <div className="flex flex-wrap items-center justify-center gap-2"> {!isAllWorkflowsView && ( <Button type="button" variant="outline" onClick={onShowAllWorkflows}> Show all workflows </Button> )} <CreateDAGModal /> </div> </> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-list/DAGTable.tsx` around lines 1494 - 1510, Extract the duplicated empty-state markup into a local WorkflowsEmptyState component in DAGTable.tsx, accepting description, isAllWorkflowsView, and onShowAllWorkflows props. Replace both the table and card empty-state copies with this component, preserving the heading, description, conditional Show all workflows button, and CreateDAGModal behavior.ui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsx (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the remaining exports of the mocked barrel.
This factory replaces the whole
@/features/dags/components/commonmodule with two exports. IfDAGTablelater imports another symbol from that barrel, this test fails for a reason unrelated to the change under test. Spreading the original module keeps the mock narrow.♻️ Proposed change
-vi.mock('`@/features/dags/components/common`', () => ({ - CreateDAGModal: () => null, - DAGPagination: () => null, -})); +vi.mock('`@/features/dags/components/common`', async (importOriginal) => ({ + ...(await importOriginal< + typeof import('`@/features/dags/components/common`') + >()), + CreateDAGModal: () => null, + DAGPagination: () => null, +}));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsx` around lines 30 - 33, Update the `@/features/dags/components/common` mock used by DAGTable tests to preserve all original barrel exports, while overriding only CreateDAGModal and DAGPagination with the existing stubs. Load the original module in the mock factory and spread it into the returned object.ui/src/pages/dags/__tests__/index.test.tsx (1)
486-558: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for a workspace scope change.
These tests fix
workspaceSelectionto{ kind: WorkspaceKind.all }for the whole suite. ThescopeChangedbranch inui/src/pages/dags/index.tsxat Lines 392-446 is therefore never exercised. That branch discards URL filters, resolves the default view of the new scope, and performs a replacing navigation. It is the most branch-heavy logic added in this PR.Make
workspaceSelectiona parameter ofrenderPage, then re-render with a workspace-scoped selection and assert that the URL drops the previous filters and that the new scope's default view becomes active.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/dags/__tests__/index.test.tsx` around lines 486 - 558, Extend the test helper renderPage to accept a workspaceSelection parameter instead of always using WorkspaceKind.all, then add coverage that initially renders with URL filters and re-renders using a workspace-scoped selection. Assert the replacing navigation removes the prior filters and that the new scope’s default workflow view becomes active, exercising the scopeChanged branch in the page logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/persis/store/view.go`:
- Around line 57-65: Update the Create and Update flows around the primary
Put/CompareAndSwap and clearOtherDefaultsLocked calls so a committed view write
is reported as successful even when best-effort default clearing fails. Preserve
the primary write error as the operation result, and surface clearing failures
through a distinct identifiable non-fatal error or warning; update the frontend
handler to treat that condition as a warning rather than a create/update
failure.
In `@ui/src/pages/dags/index.tsx`:
- Around line 654-696: Update handleSaveWorkflowView and
handleUpdateWorkflowView to use the normalized view object returned by
createView/updateView when applying filters, rather than the pre-save local
filters. Pass the response view’s filters to applyFilters so trimmed labels and
dagName values are reflected immediately and the view does not appear edited
after saving.
---
Nitpick comments:
In `@ui/src/contexts/UserPreference.tsx`:
- Around line 42-44: Update loadPreferences to parse saved data into a typed
preferences value and construct the result by selecting only the known
preference keys, rather than spreading the entire parsed object or deleting just
workflowFilterViews. Preserve defaultPreferences as the fallback for missing
recognized values and ensure updatePreference only receives the sanitized
preference shape.
In `@ui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsx`:
- Around line 30-33: Update the `@/features/dags/components/common` mock used by
DAGTable tests to preserve all original barrel exports, while overriding only
CreateDAGModal and DAGPagination with the existing stubs. Load the original
module in the mock factory and spread it into the returned object.
In `@ui/src/features/dags/components/dag-list/DAGTable.tsx`:
- Around line 1494-1510: Extract the duplicated empty-state markup into a local
WorkflowsEmptyState component in DAGTable.tsx, accepting description,
isAllWorkflowsView, and onShowAllWorkflows props. Replace both the table and
card empty-state copies with this component, preserving the heading,
description, conditional Show all workflows button, and CreateDAGModal behavior.
In `@ui/src/pages/dags/__tests__/index.test.tsx`:
- Around line 486-558: Extend the test helper renderPage to accept a
workspaceSelection parameter instead of always using WorkspaceKind.all, then add
coverage that initially renders with URL filters and re-renders using a
workspace-scoped selection. Assert the replacing navigation removes the prior
filters and that the new scope’s default workflow view becomes active,
exercising the scopeChanged branch in the page logic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88c01e31-b1ed-4692-b94b-abc435711f02
📒 Files selected for processing (19)
api/v1/api.gen.goapi/v1/api.yamlinternal/persis/store/view.gointernal/persis/store/view_test.gointernal/service/frontend/api/v1/views.gointernal/service/frontend/api/v1/views_test.gointernal/view/view.gointernal/view/view_test.goui/src/api/v1/schema.tsui/src/contexts/UserPreference.tsxui/src/features/dags/components/dag-list/DAGTable.tsxui/src/features/dags/components/dag-list/WorkflowViewSelector.tsxui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsxui/src/features/dags/components/dag-list/__tests__/WorkflowViewSelector.test.tsxui/src/features/dags/components/dag-list/workflowViews.tsui/src/hooks/__tests__/useViews.test.tsxui/src/hooks/useViews.tsui/src/pages/dags/__tests__/index.test.tsxui/src/pages/dags/index.tsx
…iews # Conflicts: # api/v1/api.gen.go
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/view/view_test.go (1)
31-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative cases for the new workflow validation branches.
The tests cover the success path only.
ErrInvalidWorkspaceScope,ErrInvalidSortField, andErrInvalidSortOrderare never asserted. A regression that widensValidWorkspaceScope,ValidWorkflowSortField, orValidSortOrderwould pass.Note that
Normalizefills empty values, so the negative cases must set explicit invalid values (for exampleWorkspaceScope: view.WorkspaceScopeAlltogether withWorkspace: "production", orSortField: "created").💚 Proposed additional test
func TestView_ValidateWorkflowRejectsInvalidFields(t *testing.T) { tests := map[string]struct { mutate func(v *view.View) want error }{ "scope all with workspace": { mutate: func(v *view.View) { v.WorkspaceScope = view.WorkspaceScopeAll; v.Workspace = "production" }, want: view.ErrInvalidWorkspaceScope, }, "workspace scope without workspace": { mutate: func(v *view.View) { v.WorkspaceScope = view.WorkspaceScopeWorkspace; v.Workspace = "" }, want: view.ErrInvalidWorkspaceScope, }, "unknown sort field": { mutate: func(v *view.View) { v.SortField = "created" }, want: view.ErrInvalidSortField, }, "unknown sort order": { mutate: func(v *view.View) { v.SortOrder = "descending" }, want: view.ErrInvalidSortOrder, }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { v := &view.View{Name: "N", Type: view.TypeWorkflow} v.Normalize() tc.mutate(v) assert.ErrorIs(t, v.Validate(), tc.want) }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/view/view_test.go` around lines 31 - 47, Extend TestView_ValidateWorkflow with negative table-driven cases using explicit invalid WorkspaceScope, Workspace, SortField, and SortOrder values so Normalize does not mask them. Assert Validate returns ErrInvalidWorkspaceScope, ErrInvalidSortField, and ErrInvalidSortOrder with assert.ErrorIs, covering both invalid workspace-scope combinations and unknown workflow sort values.ui/src/pages/dags/index.tsx (1)
560-566: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
sortFieldandsortOrderbefore casting them to the generated enums.
sortFieldandsortOrderare plain strings that can originate from the URL (params.get('sort'),params.get('order')). The casts at Lines 564-565 assert enum membership without checking it. A hand-edited or stale link such as?sort=created&order=descendingproduces a spec that the backend rejects withErrInvalidSortFieldorErrInvalidSortOrder. The user then sees a save failure instead of a corrected sort.Normalize the values to the enum members, and fall back to the defaults.
♻️ Proposed normalization helpers
+const toSortField = (value: string): ViewSortField => + Object.values(ViewSortField).includes(value as ViewSortField) + ? (value as ViewSortField) + : ViewSortField.name; + +const toSortOrder = (value: string): ViewSortOrder => + Object.values(ViewSortOrder).includes(value as ViewSortOrder) + ? (value as ViewSortOrder) + : ViewSortOrder.asc;labels: [...filters.searchLabels], dagName: filters.searchText, intervalDays: 1, pinned, - sortField: filters.sortField as ViewSortField, - sortOrder: filters.sortOrder as ViewSortOrder, + sortField: toSortField(filters.sortField), + sortOrder: toSortOrder(filters.sortOrder), isDefault,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/dags/index.tsx` around lines 560 - 566, Validate and normalize filters.sortField and filters.sortOrder before constructing the spec in the surrounding save flow. Map only recognized values to the generated ViewSortField and ViewSortOrder members, and replace invalid URL-derived values with the existing default sort field and sort order instead of relying on casts; preserve valid selections unchanged.internal/persis/store/view_test.go (1)
108-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend this test to assert that reconciliation still clears competing defaults.
Both subtests insert a corrupt record but no competing default view. The test therefore cannot detect that
clearOtherDefaultsLockedaborts on the first decode failure and leaves later defaults set. See the related comment oninternal/persis/store/view.go.Add a second workflow view with
Default = truein the same scope, then assert it is demoted after the create or update completes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/persis/store/view_test.go` around lines 108 - 157, Extend TestViewStore_DefaultCleanupFailureDoesNotMaskCommittedWrite in both “create” and “update” subtests with a second workflow view marked Default in the same scope, then assert after Create or Update that this competing view is no longer default while the committed view remains default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/persis/store/view.go`:
- Around line 204-227: Update clearOtherDefaultsLocked in
internal/persis/store/view.go at lines 204-227 to log decode errors from
viewFromRecord and continue scanning subsequent records instead of returning
immediately. In internal/persis/store/view_test.go at lines 108-157, add a
competing workflow view with Default=true in the same scope to each relevant
subtest and assert it is demoted after create or update completes.
---
Nitpick comments:
In `@internal/persis/store/view_test.go`:
- Around line 108-157: Extend
TestViewStore_DefaultCleanupFailureDoesNotMaskCommittedWrite in both “create”
and “update” subtests with a second workflow view marked Default in the same
scope, then assert after Create or Update that this competing view is no longer
default while the committed view remains default.
In `@internal/view/view_test.go`:
- Around line 31-47: Extend TestView_ValidateWorkflow with negative table-driven
cases using explicit invalid WorkspaceScope, Workspace, SortField, and SortOrder
values so Normalize does not mask them. Assert Validate returns
ErrInvalidWorkspaceScope, ErrInvalidSortField, and ErrInvalidSortOrder with
assert.ErrorIs, covering both invalid workspace-scope combinations and unknown
workflow sort values.
In `@ui/src/pages/dags/index.tsx`:
- Around line 560-566: Validate and normalize filters.sortField and
filters.sortOrder before constructing the spec in the surrounding save flow. Map
only recognized values to the generated ViewSortField and ViewSortOrder members,
and replace invalid URL-derived values with the existing default sort field and
sort order instead of relying on casts; preserve valid selections unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eee4f3e0-ad9c-45ca-bdc0-bd35e827ef4f
📒 Files selected for processing (21)
api/v1/api.gen.goapi/v1/api.yamlinternal/persis/store/view.gointernal/persis/store/view_test.gointernal/service/frontend/api/v1/views.gointernal/service/frontend/api/v1/views_test.gointernal/view/view.gointernal/view/view_test.goui/src/__tests__/menu.test.tsxui/src/api/v1/schema.tsui/src/contexts/UserPreference.tsxui/src/features/dags/components/dag-list/DAGTable.tsxui/src/features/dags/components/dag-list/WorkflowViewSelector.tsxui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsxui/src/features/dags/components/dag-list/__tests__/WorkflowViewSelector.test.tsxui/src/features/dags/components/dag-list/workflowViews.tsui/src/hooks/__tests__/useViews.test.tsxui/src/hooks/useViews.tsui/src/menu.tsxui/src/pages/dags/__tests__/index.test.tsxui/src/pages/dags/index.tsx
|
Addressed the remaining review nitpicks in
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ui/src/pages/dags/__tests__/index.test.tsx (1)
577-618: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a failed workflow-view mutation.
The tests cover only successful create, update, default, pin, and delete calls.
ui/src/pages/dags/index.tsxalso setsworkflowViewErrorand rethrows on failure. One test that rejectscreateViewMockand asserts the error state would protect that path.As per coding guidelines, "Add or update tests appropriate to the changed code; ... frontend unit tests use Vitest".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/pages/dags/__tests__/index.test.tsx` around lines 577 - 618, Add a Vitest case in the workflow-view page tests that makes createViewMock reject, triggers the save workflow-view action, and asserts the resulting workflowViewError UI state. Verify the mutation error is surfaced through the page’s existing error handling while preserving the successful create coverage.Source: Coding guidelines
ui/src/features/dags/components/dag-list/workflowViews.ts (1)
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider typing the sort fields with the generated enums.
sortFieldandsortOrderare declared asstring. The API contract usesViewSortFieldandViewSortOrder, andui/src/pages/dags/index.tsxalready normalizes values to those enums innormalizeWorkflowSortFieldandnormalizeWorkflowSortOrder. Typing the fields as the enums removes the widening and prevents invalid values from flowing into saved view specs.♻️ Proposed typing change
-import { ViewWorkspaceScope } from '`@/api/v1/schema`'; +import { + ViewSortField, + ViewSortOrder, + ViewWorkspaceScope, +} from '`@/api/v1/schema`'; @@ export type WorkflowFilterSet = { searchText: string; searchLabels: string[]; - sortField: string; - sortOrder: string; + sortField: ViewSortField; + sortOrder: ViewSortOrder; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-list/workflowViews.ts` around lines 11 - 16, Update the WorkflowFilterSet type so sortField and sortOrder use the generated ViewSortField and ViewSortOrder enums instead of string, reusing the existing enum imports and normalization behavior from normalizeWorkflowSortField and normalizeWorkflowSortOrder.internal/service/frontend/api/v1/views_test.go (1)
136-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the response type assertion.
Line 138 uses an unchecked type assertion. If
ListViewsreturns another response type, the test panics instead of reporting the actual type. ThemustCreateViewhelper already checks the assertion, so match that style.♻️ Proposed change
listResp, err := api.ListViews(ctx, apigen.ListViewsRequestObject{}) require.NoError(t, err) - listed := listResp.(apigen.ListViews200JSONResponse) + listed, ok := listResp.(apigen.ListViews200JSONResponse) + require.True(t, ok, "expected 200, got %T", listResp)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/frontend/api/v1/views_test.go` around lines 136 - 146, Update the ListViews response handling in the test to use a checked type assertion, matching the existing mustCreateView helper style. Assert that the response is an apigen.ListViews200JSONResponse and fail the test with the actual response type instead of allowing a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/frontend/api/v1/views.go`:
- Line 245: Update the UpdateView flow around viewFromSpec and Normalize so
omitted workspaceScope, sortField, sortOrder, and isDefault fields preserve
their persisted values instead of being replaced by Go zero values. Apply the
same nil-aware handling already used for Columns, while retaining explicit
update behavior when those fields are provided.
---
Nitpick comments:
In `@internal/service/frontend/api/v1/views_test.go`:
- Around line 136-146: Update the ListViews response handling in the test to use
a checked type assertion, matching the existing mustCreateView helper style.
Assert that the response is an apigen.ListViews200JSONResponse and fail the test
with the actual response type instead of allowing a panic.
In `@ui/src/features/dags/components/dag-list/workflowViews.ts`:
- Around line 11-16: Update the WorkflowFilterSet type so sortField and
sortOrder use the generated ViewSortField and ViewSortOrder enums instead of
string, reusing the existing enum imports and normalization behavior from
normalizeWorkflowSortField and normalizeWorkflowSortOrder.
In `@ui/src/pages/dags/__tests__/index.test.tsx`:
- Around line 577-618: Add a Vitest case in the workflow-view page tests that
makes createViewMock reject, triggers the save workflow-view action, and asserts
the resulting workflowViewError UI state. Verify the mutation error is surfaced
through the page’s existing error handling while preserving the successful
create coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be219ae0-2d56-4cd1-a99f-fb17ba36a661
📒 Files selected for processing (21)
api/v1/api.gen.goapi/v1/api.yamlinternal/persis/store/view.gointernal/persis/store/view_test.gointernal/service/frontend/api/v1/views.gointernal/service/frontend/api/v1/views_test.gointernal/view/view.gointernal/view/view_test.goui/src/__tests__/menu.test.tsxui/src/api/v1/schema.tsui/src/contexts/UserPreference.tsxui/src/features/dags/components/dag-list/DAGTable.tsxui/src/features/dags/components/dag-list/WorkflowViewSelector.tsxui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsxui/src/features/dags/components/dag-list/__tests__/WorkflowViewSelector.test.tsxui/src/features/dags/components/dag-list/workflowViews.tsui/src/hooks/__tests__/useViews.test.tsxui/src/hooks/useViews.tsui/src/menu.tsxui/src/pages/dags/__tests__/index.test.tsxui/src/pages/dags/index.tsx
|
Addressed all three nitpicks from the final full review in
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ui/src/features/dags/components/dag-list/WorkflowViewSelector.tsx (1)
268-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the save-dialog spacing.
The body uses
py-5andspace-y-4. The repository convention is a compact, information-dense layout with minimal whitespace. Reduce the body padding and the vertical stack spacing.As per coding guidelines, "Use information-dense, compact UI design with minimal whitespace, small form element heights (h-7 or smaller for select boxes), and reduced padding".
♻️ Proposed spacing change
- <div className="space-y-4 py-5"> + <div className="space-y-3 py-3">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-list/WorkflowViewSelector.tsx` around lines 268 - 308, Reduce the save-dialog body spacing in the container wrapping the fields: replace the current py-5 padding and space-y-4 vertical gap with the repository’s more compact spacing values, while leaving the form controls and behavior unchanged.Source: Coding guidelines
ui/src/features/dags/components/dag-list/DAGTable.tsx (1)
1498-1517: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared empty state into one component.
The desktop branch and the card branch render the same heading, the same
emptyStateDescriptionparagraph, and the same conditional action block. Two copies must stay in sync for every future wording or action change. Extract a localWorkflowsEmptyStatecomponent and render it in both branches.♻️ Proposed extraction
function WorkflowsEmptyState({ description, isAllWorkflowsView, onShowAllWorkflows, }: { description: string; isAllWorkflowsView: boolean; onShowAllWorkflows: () => void; }) { return ( <div className="flex flex-col items-center justify-center py-8"> <div className="text-6xl mb-4">🔍</div> <h3 className="text-lg font-medium text-foreground mb-2"> No workflows found </h3> <p className="text-sm text-muted-foreground text-center max-w-md mb-4 whitespace-normal break-words"> {description} </p> <div className="flex flex-wrap items-center justify-center gap-2"> {!isAllWorkflowsView && ( <Button type="button" variant="outline" onClick={onShowAllWorkflows}> Show all workflows </Button> )} <CreateDAGModal /> </div> </div> ); }Also applies to: 1616-1634
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/dags/components/dag-list/DAGTable.tsx` around lines 1498 - 1517, Extract the duplicated empty-state markup into a local WorkflowsEmptyState component near the DAGTable implementation, accepting description, isAllWorkflowsView, and onShowAllWorkflows props. Replace both the desktop and card empty-state branches with this component while preserving the existing layout, wording, conditional “Show all workflows” action, and CreateDAGModal behavior.ui/src/contexts/UserPreference.tsx (1)
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a type annotation and avoid mutating the parsed object.
JSON.parse(saved)returns an implicitlyany-typed value. Add a type annotation forpreferences. Use destructuring to drop the deprecatedworkflowFilterViewskey instead of mutating the parsed object withdelete.As per coding guidelines,
ui/**/*.{ts,tsx}requires: "Use type annotations in TypeScript files."♻️ Proposed refactor
- const preferences = JSON.parse(saved); - delete preferences.workflowFilterViews; - return { ...defaultPreferences, ...preferences }; + const { workflowFilterViews, ...preferences }: Record<string, unknown> = + JSON.parse(saved); + return { ...defaultPreferences, ...preferences };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/contexts/UserPreference.tsx` around lines 42 - 44, Update the preferences parsing flow to explicitly type the JSON.parse result, then use destructuring to omit the deprecated workflowFilterViews property without mutating preferences. Preserve the existing merge with defaultPreferences and the resulting preference values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/src/features/dags/components/dag-list/workflowViews.ts`:
- Around line 53-62: Update workflowViewMatchesScope to normalize an omitted
view.workspaceScope to ViewWorkspaceScope.all before comparing it with
scope.workspaceScope. Preserve the existing workspace-name comparison for
workspace-scoped views and ensure legacy views with missing workspace_scope
match the all scope.
---
Nitpick comments:
In `@ui/src/contexts/UserPreference.tsx`:
- Around line 42-44: Update the preferences parsing flow to explicitly type the
JSON.parse result, then use destructuring to omit the deprecated
workflowFilterViews property without mutating preferences. Preserve the existing
merge with defaultPreferences and the resulting preference values.
In `@ui/src/features/dags/components/dag-list/DAGTable.tsx`:
- Around line 1498-1517: Extract the duplicated empty-state markup into a local
WorkflowsEmptyState component near the DAGTable implementation, accepting
description, isAllWorkflowsView, and onShowAllWorkflows props. Replace both the
desktop and card empty-state branches with this component while preserving the
existing layout, wording, conditional “Show all workflows” action, and
CreateDAGModal behavior.
In `@ui/src/features/dags/components/dag-list/WorkflowViewSelector.tsx`:
- Around line 268-308: Reduce the save-dialog body spacing in the container
wrapping the fields: replace the current py-5 padding and space-y-4 vertical gap
with the repository’s more compact spacing values, while leaving the form
controls and behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae9dcdb9-b211-4e3e-9ab0-da6fbe0ff690
📒 Files selected for processing (21)
api/v1/api.gen.goapi/v1/api.yamlinternal/persis/store/view.gointernal/persis/store/view_test.gointernal/service/frontend/api/v1/views.gointernal/service/frontend/api/v1/views_test.gointernal/view/view.gointernal/view/view_test.goui/src/__tests__/menu.test.tsxui/src/api/v1/schema.tsui/src/contexts/UserPreference.tsxui/src/features/dags/components/dag-list/DAGTable.tsxui/src/features/dags/components/dag-list/WorkflowViewSelector.tsxui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsxui/src/features/dags/components/dag-list/__tests__/WorkflowViewSelector.test.tsxui/src/features/dags/components/dag-list/workflowViews.tsui/src/hooks/__tests__/useViews.test.tsxui/src/hooks/useViews.tsui/src/menu.tsxui/src/pages/dags/__tests__/index.test.tsxui/src/pages/dags/index.tsx
Summary
Access model
All users who can access a workspace can see, select, and use its shared starred workflow views. Creating, updating, starring, setting the default, and deleting follow the Kanban permission model and require developer-level write access for that scope.
Behavior
Shared workflow views capture the workflow name filter, label filters, and server-side sort order. A star adds a shared sidebar shortcut; default controls only the initial Workflows filter. All-workspaces, default-workspace, and named-workspace selections are kept separate. Explicit URL filters take precedence over the shared default; when no default exists, session-restored filters remain the fallback.
The feature does not read or write workflow views through browser preferences.
Validation
make testmake checkgo test ./api/v1 ./internal/service/frontend/api/v1 -count=1pnpm test(126 files, 550 tests)pnpm typecheckpnpm buildSummary by CodeRabbit