Repository navigation
fix(ui): close first-contact UX gaps across the web interface - #2531
Merged
Merged
Conversation
The mount-time formatDocument rewrote files the user never touched, lighting up Save/Discard and arming the external-change conflict dialog on first open. formatOnType reformatted the whole document on every Enter. Manual format (Shift+Alt+F) still works.
Backend validation errors (line:column prefixes and caret excerpts) were passed as the modal's hint, which collapses newlines into one muted proportional-font paragraph. Add a details channel rendered monospace with line breaks preserved, and stop treating an empty errors array as a failure on spec save.
The retry action re-executes only the selected step, but the tooltip and dialog title said "Retry from this step". Rename to match the API contract, and give the success path the same toast plus refresh the run-level retry already has instead of silently closing the dialog.
The header button said Enqueue while its tooltip, the modal it opens, and the CLI all say start. The modal still switches to Enqueue wording when the queue toggle is on.
The executions list rendered "No DAG runs found" while the first page was still loading, then swapped in the table. Thread the pagination hook's initial-loading flag into both list views and show a quiet inline loading row instead.
The workflow list told users with zero workflows to adjust filters they never set. Branch the copy: a pristine all-workflows view now invites creating the first workflow. The runs empty state is reworded to be range-aware instead of presuming filters, and the duplicated workflow empty-state markup is extracted into one component.
The schema doc sidebar has an Examples panel, and monaco-yaml surfaces schema examples in hover, value completion, and property snippets, but the schema carried none. Seed concise examples for the most-used DAG and step properties.
Every tab, bookmark, and history entry read the static "Dagu". Pages already publish their titles through AppBarContext; mirror that into document.title.
Sharing a workflow required select-all inside Monaco, and sharing a run meant grabbing the address bar. Add a Copy button to the spec editor header (visible to read-only viewers too) and a Copy link button on the run header. The three hand-rolled copy-feedback blocks consolidate into a useCopyFeedback hook.
…used The v1 executor key was rejected with a bare invalid-keys error while 70+ other legacy keys get migration hints, and renamed keys (precondition, dir) were mislabeled as snake_case fixes. Split the hint map into casing renames and removed keys with full replacement clauses, and drop the dead run->call entry (run is a valid v2 step key).
📝 WalkthroughWalkthroughThe PR adds schema examples and legacy-key migration hints. It extends structured validation-error display, centralizes copy feedback, adds DAG run loading states, and updates workflow, graph, editor, title, and retry UI behavior with tests. ChangesDAG schema and diagnostics
UI behavior
Estimated code review effort: 4 (Complex) | ~45 minutes 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 (5)
ui/src/hooks/__tests__/useCopyFeedback.test.tsx (1)
20-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest timer replacement after repeated copies.
Call
copy()a second time before the first reset expires. Assert thatcopiedremains true untilresetMsafter the second call. This verifies the timer replacement atuseCopyFeedback.tsLine 23.As per coding guidelines, TypeScript changes require appropriate tests.
Proposed test
+ it('restarts feedback after a later successful copy', async () => { + vi.useFakeTimers(); + copyTextMock.mockResolvedValue(true); + const { result } = renderHook(() => useCopyFeedback()); + + await act(async () => { + await result.current.copy('first'); + }); + act(() => vi.advanceTimersByTime(1000)); + await act(async () => { + await result.current.copy('second'); + }); + + act(() => vi.advanceTimersByTime(1000)); + expect(result.current.copied).toBe(true); + act(() => vi.advanceTimersByTime(1000)); + expect(result.current.copied).toBe(false); + });🤖 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/hooks/__tests__/useCopyFeedback.test.tsx` around lines 20 - 45, Add a test alongside the existing useCopyFeedback cases that performs two successful copy calls before the first reset expires, advances fake timers to confirm copied remains true, then advances through resetMs measured from the second call and confirms it becomes false. Reuse the existing copyTextMock, renderHook, and timer setup to verify the reset timer replacement in useCopyFeedback.Source: Coding guidelines
ui/src/features/dag-runs/components/dag-run-details/DAGRunHeader.tsx (1)
44-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for both generated run-link modes.
The existing test covers root and sub-run URL shapes, but this copy-link path is also in
DAGRunHeader.tsxand uses non-rootconfig.basePath. Add coverage for both run types with a non-root base path if it is not already present.🤖 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/dag-runs/components/dag-run-details/DAGRunHeader.tsx` around lines 44 - 54, Add tests for the DAGRunHeader copyRunLink flow covering both root and sub-run links when config.basePath is a non-root value. Verify the copied URLs include the configured base path and the correct run-path shape, reusing the existing root/sub-run URL test setup where possible.Source: Coding guidelines
ui/src/pages/dag-runs/__tests__/index.test.tsx (1)
198-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the grouped-view loading prop.
This test captures
DAGRunTableprops only. It does not verify thatDAGRunspassesisLoadingtoDAGRunGroupedViewatui/src/pages/dag-runs/index.tsxLine 1061. Add grouped-view prop capture or run the assertion for both view modes.As per coding guidelines,
ui/**/*.{ts,tsx}changes must add or update appropriate tests; this test does not exercise the grouped-view wiring.🤖 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/dag-runs/__tests__/index.test.tsx` around lines 198 - 213, Extend the initial-loading test around renderPage and the view-mode setup to exercise the grouped view as well as the table view. Capture DAGRunGroupedView props, render with grouped view selected, and assert its isLoading prop is true, while preserving the existing DAGRunTable assertion.Source: Coding guidelines
ui/src/features/dags/components/dag-list/DAGTable.tsx (1)
1004-1005: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the prescribed text color hierarchy.
WorkflowsEmptyStateusestext-foregroundfor primary text andtext-muted-foregroundfor muted text. Use the specified slate classes for this new UI.As per coding guidelines, use
text-slate-800 dark:text-slate-200for primary text andtext-slate-500 dark:text-slate-500for muted text.Proposed class update
- <h3 className="text-lg font-medium text-foreground mb-2">{heading}</h3> + <h3 className="text-lg font-medium text-slate-800 dark:text-slate-200 mb-2"> + {heading} + </h3> - <p className="text-sm text-muted-foreground text-center max-w-md mb-4 whitespace-normal break-words"> + <p className="text-sm text-slate-500 dark:text-slate-500 text-center max-w-md mb-4 whitespace-normal break-words">🤖 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 1004 - 1005, Update the heading and descriptive paragraph in the empty-state UI to use the prescribed slate color hierarchy: apply text-slate-800 dark:text-slate-200 to the primary heading and text-slate-500 dark:text-slate-500 to the muted paragraph, replacing the existing text-foreground and text-muted-foreground classes.Source: Coding guidelines
ui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsx (1)
422-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining empty-state branches.
This test covers only the pristine all-workflows branch. Add cases for filtered results and a named workflow view. Assert the generic message and the
Show all workflowsaction where applicable.As per coding guidelines, TypeScript changes should include 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 422 - 430, Expand the DAGTable empty-state tests around the existing “no workflows yet” case to cover an empty filtered result and an empty named-workflow view. Assert the generic “No workflows found” message and the “Show all workflows” action for the filtered branch where applicable, while preserving the existing pristine-state assertions.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/__tests__/App.test.tsx`:
- Around line 217-221: Update the “falls back to the configured title when a
page sets none” test to provide a non-default custom config.title before
rendering /queues, then assert document.title equals that custom value while
preserving the existing heading assertion. Ensure the test exercises
configured-title propagation rather than only the default “Dagu” fallback.
In `@ui/src/features/dags/components/visualization/Graph.tsx`:
- Line 393: Update the graph memoization dependency list in Graph so it includes
the type value used by the type === 'status' branch, ensuring the graph
recomputes when type changes while preserving the existing dependencies.
---
Nitpick comments:
In `@ui/src/features/dag-runs/components/dag-run-details/DAGRunHeader.tsx`:
- Around line 44-54: Add tests for the DAGRunHeader copyRunLink flow covering
both root and sub-run links when config.basePath is a non-root value. Verify the
copied URLs include the configured base path and the correct run-path shape,
reusing the existing root/sub-run URL test setup where possible.
In `@ui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsx`:
- Around line 422-430: Expand the DAGTable empty-state tests around the existing
“no workflows yet” case to cover an empty filtered result and an empty
named-workflow view. Assert the generic “No workflows found” message and the
“Show all workflows” action for the filtered branch where applicable, while
preserving the existing pristine-state assertions.
In `@ui/src/features/dags/components/dag-list/DAGTable.tsx`:
- Around line 1004-1005: Update the heading and descriptive paragraph in the
empty-state UI to use the prescribed slate color hierarchy: apply text-slate-800
dark:text-slate-200 to the primary heading and text-slate-500
dark:text-slate-500 to the muted paragraph, replacing the existing
text-foreground and text-muted-foreground classes.
In `@ui/src/hooks/__tests__/useCopyFeedback.test.tsx`:
- Around line 20-45: Add a test alongside the existing useCopyFeedback cases
that performs two successful copy calls before the first reset expires, advances
fake timers to confirm copied remains true, then advances through resetMs
measured from the second call and confirms it becomes false. Reuse the existing
copyTextMock, renderHook, and timer setup to verify the reset timer replacement
in useCopyFeedback.
In `@ui/src/pages/dag-runs/__tests__/index.test.tsx`:
- Around line 198-213: Extend the initial-loading test around renderPage and the
view-mode setup to exercise the grouped view as well as the table view. Capture
DAGRunGroupedView props, render with grouped view selected, and assert its
isLoading prop is true, while preserving the existing DAGRunTable assertion.
🪄 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: 5d2acfbf-1429-4a35-bae6-df895f6790af
📒 Files selected for processing (37)
internal/cmn/schema/dag.schema.jsoninternal/core/spec/builder.gointernal/core/spec/dag.gointernal/core/spec/defaults.gointernal/core/spec/key_hints.gointernal/core/spec/loader_test.gointernal/core/spec/manifest_decoder.goui/e2e/helpers/e2e.tsui/src/App.tsxui/src/__tests__/App.test.tsxui/src/components/ui/__tests__/error-modal.test.tsxui/src/components/ui/error-modal.tsxui/src/features/dag-runs/components/dag-run-details/DAGRunHeader.tsxui/src/features/dag-runs/components/dag-run-list/DAGRunGroupedView.tsxui/src/features/dag-runs/components/dag-run-list/DAGRunTable.tsxui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunGroupedView.test.tsxui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunTable.test.tsxui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/common/DAGActions.tsxui/src/features/dags/components/dag-details/DAGHeader.tsxui/src/features/dags/components/dag-details/NodeStatusTableRow.tsxui/src/features/dags/components/dag-details/__tests__/NodeStatusTableRow.test.tsxui/src/features/dags/components/dag-editor/DAGEditor.tsxui/src/features/dags/components/dag-editor/DAGSpec.tsxui/src/features/dags/components/dag-editor/DAGSpecReadOnly.tsxui/src/features/dags/components/dag-list/DAGTable.tsxui/src/features/dags/components/dag-list/__tests__/DAGTable.test.tsxui/src/features/dags/components/visualization/DAGGraph.tsxui/src/features/dags/components/visualization/Graph.tsxui/src/features/dags/components/visualization/__tests__/Graph.test.tsxui/src/hooks/__tests__/useCopyFeedback.test.tsxui/src/hooks/useCopyFeedback.tsui/src/pages/base-config/index.tsxui/src/pages/dag-runs/__tests__/index.test.tsxui/src/pages/dag-runs/index.tsxui/src/pages/docs/components/DocEditor.tsxui/src/styles/global.css
💤 Files with no reviewable changes (2)
- ui/src/features/dags/components/DAGStatus.tsx
- ui/src/features/dags/components/visualization/tests/Graph.test.tsx
Comment on lines
+217
to
+221
| it('falls back to the configured title when a page sets none', async () => { | ||
| renderAt('/queues'); | ||
|
|
||
| expect(await screen.findByRole('heading', { name: 'Queues' })).toBeVisible(); | ||
| expect(document.title).toBe('Dagu'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test a non-default configured title.
Lines 217-221 only verify the default value, Dagu. Pass a custom config.title and expect that value. The test must detect a regression that ignores the configured application title.
Proposed test update
it('falls back to the configured title when a page sets none', async () => {
- renderAt('/queues');
+ renderAt('/queues', makeConfig({ title: 'Operations' }));
expect(await screen.findByRole('heading', { name: 'Queues' })).toBeVisible();
- expect(document.title).toBe('Dagu');
+ expect(document.title).toBe('Operations');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('falls back to the configured title when a page sets none', async () => { | |
| renderAt('/queues'); | |
| expect(await screen.findByRole('heading', { name: 'Queues' })).toBeVisible(); | |
| expect(document.title).toBe('Dagu'); | |
| it('falls back to the configured title when a page sets none', async () => { | |
| renderAt('/queues', makeConfig({ title: 'Operations' })); | |
| expect(await screen.findByRole('heading', { name: 'Queues' })).toBeVisible(); | |
| expect(document.title).toBe('Operations'); |
🤖 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/__tests__/App.test.tsx` around lines 217 - 221, Update the “falls back
to the configured title when a page sets none” test to provide a non-default
custom config.title before rendering /queues, then assert document.title equals
that custom value while preserving the existing heading assertion. Ensure the
test exercises configured-title propagation rather than only the default “Dagu”
fallback.
|
|
||
| return dat.join('\n'); | ||
| }, [steps, onClickNode, flowchart, showIcons, isDarkMode]); | ||
| }, [steps, onClickNode, flowchart, isDarkMode]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 \
'const graph = React\.useMemo|type === .status.|^\s*\}, \[steps,.*flowchart' \
ui/src/features/dags/components/visualization/Graph.tsxRepository: dagucloud/dagu
Length of output: 2957
Add type to the graph memo dependencies.
graph branches on type === 'status', but the dependency list omits type. When type changes without steps, graph can still use the previous graph definition.
Proposed fix
- }, [steps, onClickNode, flowchart, isDarkMode]);
+ }, [steps, type, onClickNode, flowchart, isDarkMode]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| }, [steps, onClickNode, flowchart, isDarkMode]); | |
| }, [steps, type, onClickNode, flowchart, isDarkMode]); |
🤖 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/visualization/Graph.tsx` at line 393, Update
the graph memoization dependency list in Graph so it includes the type value
used by the type === 'status' branch, ensuring the graph recomputes when type
changes while preserving the existing dependencies.
This was referenced Aug 8, 2026
The graph definition memo branches on the graph type but omitted it from its dependency list, and the fallback-title test only exercised the default value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eleven small, self-contained fixes from a UX audit of the web UI, each targeting a moment that shapes a newcomer's first impression:
Validation
Summary by cubic
Improves first‑time UX across the web UI with small, focused changes. Adds clearer actions, better errors, helpful examples, and easier sharing.
New Features
showIcons/animateprops.useCopyFeedback.examplesfor common fields to power docs, hover cards, and editor completion.Bug Fixes
executor->action+with,precondition->preconditions,dir->working_dir).Written for commit e9525df. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements