feat: display chat messages for dag-run#1578
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR adds new API endpoints to retrieve LLM chat messages from DAG run steps, introduces chat message data models across backend and frontend, implements secret masking in executor messages, and develops UI components to display chat history in DAG run details. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @api/v2/api.yaml:
- Around line 2043-2074: The sub-DAG chat messages endpoint description must
mirror the root endpoint by promising an empty array for non-chat steps; update
the description for the sub-DAG path
/dag-runs/{name}/sub-dags/{subDagName}/{dagRunId}/steps/{stepName}/messages (the
GET operation that parallels getDAGRunStepMessages) to append "Returns empty
array for non-chat steps" so the behavior guarantee is consistent across both
endpoints.
In @ui/src/components/ui/markdown.tsx:
- Around line 13-29: The container div rendering Markdown uses cn(...) to build
its className and currently lacks word-wrapping utilities; update the class list
passed into cn (the string array around 'text-xs prose prose-sm ...', used in
the div in markdown.tsx) to include 'break-words' and optionally
'whitespace-normal' so long unbroken tokens will wrap and not overflow the UI.
In @ui/src/features/dags/components/chat-history/ChatHistoryTab.tsx:
- Around line 64-83: The select in ChatHistoryTab.tsx lacks an accessible
label/id and dark-mode + focus styling: add an id (e.g., "step-select") and
aria-label referencing it (or connect a visible label), keep using selectedStep
and setSelectedStep for value/onChange, update className on the select to
include dark-mode variants (e.g., bg-white/dark:bg-surface,
text-foreground/dark:text-foreground,
border-neutral-300/dark:border-neutral-700) and keyboard focus styles (e.g.,
focus:outline-none focus-visible:ring-2 focus-visible:ring-ring
focus-visible:ring-offset-0 or the project’s focus utility), and ensure options
rendered from chatSteps remain unchanged (key/value use node.step.name) so
keyboard users and screen readers can navigate and identify the control; also
preserve isSelectedRunning and Loader2 behavior.
- Around line 22-49: The selectedStep state in ChatHistoryTab is initialized
from the memoized defaultStep so it never updates when defaultStep changes
(e.g., when dagRun.nodes arrive), leaving the select empty or stale; fix by
initializing selectedStep to undefined (or keep as is) and add a useEffect that
watches defaultStep (and/or chatSteps/run id) and calls
setSelectedStep(defaultStep) whenever defaultStep changes or the current
selectedStep is no longer present; update references to defaultStep and
selectedStep in the effect so selection resets appropriately when runs switch.
In @ui/src/features/dags/components/chat-history/StepMessagesTable.tsx:
- Around line 138-151: The StepMessagesTable component renders containers and
message items with hardcoded "bg-white" which breaks dark mode; update the
className usages in the component (the outer container and the per-message div
returned in messages.map inside StepMessagesTable) to include dark-mode variants
(for example replace "bg-white" with "bg-white dark:bg-neutral-900" or the
project’s approved dark background token) and ensure any roleConfig.borderColor
still reads correctly against the dark background; adjust any additional utility
classes in the same JSX block if needed so text and border contrast remain
accessible in dark mode.
🧹 Nitpick comments (7)
internal/runtime/builtin/chat/executor_test.go (1)
477-558: Good test coverage with one minor improvement opportunity.The test cases cover important scenarios well. However, the metadata preservation check at lines 553-555 only verifies the first message when metadata is present, but the test case "preserves role and metadata" has only one message. Consider adding a test case with multiple messages where metadata exists on a non-first message to ensure metadata is correctly preserved across all messages.
💡 Optional: Enhance metadata preservation test
{ name: "preserves role and metadata", secrets: map[string]string{"SECRET": "xyz"}, messages: []exec.LLMMessage{ + {Role: exec.RoleUser, Content: "First message"}, { Role: exec.RoleAssistant, Content: "Value: xyz", Metadata: &exec.LLMMessageMetadata{Model: "gpt-4"}, }, }, - wantMasked: []string{"Value: *******"}, + wantMasked: []string{"First message", "Value: *******"}, },Then update the metadata check:
// Verify metadata is preserved - if len(tt.messages) > 0 && tt.messages[0].Metadata != nil { - assert.Equal(t, tt.messages[0].Metadata, result[0].Metadata) + for i, msg := range tt.messages { + if msg.Metadata != nil { + assert.Equal(t, msg.Metadata, result[i].Metadata) + } }ui/src/features/dags/components/DAGStatus.tsx (1)
277-290: Tab reset logic is good; consider boolean-normalizinghasChatStepsfor readability.
const hasChatSteps = !!dagRun.nodes?.some(...)avoidsboolean | undefined(current code works, but this is cleaner).api/v2/api.yaml (1)
3260-3318: Schema set is solid; consider minimums for token counts.
If you want stronger validation, addminimum: 0forpromptTokens,completionTokens,totalTokens.ui/src/components/ui/markdown.tsx (1)
31-66: Usinginlineprop is not reliable in react-markdown v10 — check for language pattern instead.Fenced code blocks without a language specification do have no
className, so!codeClassNamewould incorrectly treat them as inline. However, theinlineprop was removed/changed in react-markdown v10 and causes TypeScript errors (see GitHub issue #776).Instead, detect block code by checking for the language pattern:
const isInline = !/^language-/.test(codeClassName || ''). This aligns with how react-markdown assigns language classes to fenced blocks.ui/src/features/dags/components/chat-history/StepMessagesTable.tsx (2)
86-100: Initial state uses stale data reference.The
expandedIndexesinitial value is computed frommessages.length - 1, but at initialization,messagesis[](data not yet fetched), resulting innew Set([-1]). While theuseEffectcorrects this when messages arrive, consider initializing with an empty set.Suggested fix
- // Only last message expanded by default - const [expandedIndexes, setExpandedIndexes] = useState<Set<number>>( - new Set([messages.length - 1]) - ); + // Track expanded message indexes; useEffect expands the latest when messages arrive + const [expandedIndexes, setExpandedIndexes] = useState<Set<number>>(new Set());
111-119: Silent failure on clipboard error.The empty catch block provides no feedback when clipboard access fails. Consider a brief visual indicator or console warning for debugging.
Optional: Add minimal feedback
const handleCopy = async (content: string, index: number) => { try { await navigator.clipboard.writeText(content); setCopiedIndex(index); setTimeout(() => setCopiedIndex(null), 2000); - } catch { - // Clipboard access denied + } catch (err) { + console.warn('Clipboard access denied:', err); } };api/v2/api.gen.go (1)
284-340: Chat message response model is fine; avoidmessages: nullat runtime.ChatMessagesResponse.Messagesis a non-omitempty slice, so if handlers leave itnil, clients will seenullinstead of[]. Recommend ensuring handlers always initialize to an empty slice when there are no messages.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
api/v2/api.gen.goapi/v2/api.yamlinternal/runtime/builtin/chat/executor.gointernal/runtime/builtin/chat/executor_test.gointernal/service/frontend/api/v2/dagruns.gointernal/service/frontend/api/v2/transformer.goui/CLAUDE.mdui/package.jsonui/src/api/v2/schema.tsui/src/components/ui/markdown.tsxui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/chat-history/ChatHistoryTab.tsxui/src/features/dags/components/chat-history/StepMessagesTable.tsxui/src/features/dags/components/chat-history/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/runtime/builtin/chat/executor.gointernal/service/frontend/api/v2/transformer.gointernal/runtime/builtin/chat/executor_test.gointernal/service/frontend/api/v2/dagruns.goapi/v2/api.gen.go
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/features/dags/components/chat-history/index.tsui/src/features/dags/components/DAGStatus.tsxui/src/components/ui/markdown.tsxui/src/features/dags/components/chat-history/StepMessagesTable.tsxui/src/features/dags/components/chat-history/ChatHistoryTab.tsxui/src/api/v2/schema.ts
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/features/dags/components/chat-history/index.tsui/src/features/dags/components/DAGStatus.tsxui/src/components/ui/markdown.tsxui/src/features/dags/components/chat-history/StepMessagesTable.tsxui/src/features/dags/components/chat-history/ChatHistoryTab.tsxui/src/api/v2/schema.ts
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/runtime/builtin/chat/executor_test.go
{api/v1,api/v2}/**/*.{proto,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
API definitions live in
api/v1andapi/v2; generated server stubs land ininternal/service, while matching TypeScript clients flow intoui/src/api
Files:
api/v2/api.yaml
🧠 Learnings (9)
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text with `whitespace-normal break-words`
Applied to files:
ui/src/components/ui/markdown.tsxui/src/features/dags/components/chat-history/StepMessagesTable.tsx
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/Makefile : Ensure frontend builds without errors by running `make ui` before committing changes
Applied to files:
ui/CLAUDE.md
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Frontend workflows: `cd ui && pnpm dev` for hot reload, `pnpm build` for production bundles, `pnpm lint` to auto-fix TypeScript/React style issues
Applied to files:
ui/CLAUDE.mdui/package.json
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/.gitignore,** : Use pnpm for frontend package management instead of npm or yarn
Applied to files:
ui/CLAUDE.md
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to ui/**/*.{ts,tsx} : The React + TypeScript frontend resides in `ui/`, with production bundles copied to `internal/service/frontend/assets` by `make ui`
Applied to files:
ui/CLAUDE.mdui/package.json
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: `make run` starts the Go scheduler and serves the compiled UI (fails fast if `ui/dist` is missing)
Applied to files:
ui/CLAUDE.md
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Applied to files:
ui/CLAUDE.md
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Verify `make lint` and `make test` locally; include unit or integration tests whenever behaviour or APIs change
Applied to files:
ui/CLAUDE.md
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to ui/**/*.{ts,tsx} : UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (`JobList.tsx`) and hooks with `use*` (`useJobs.ts`)
Applied to files:
ui/CLAUDE.md
🧬 Code graph analysis (7)
internal/runtime/builtin/chat/executor.go (3)
internal/core/llm.go (1)
LLMMessage(98-103)internal/runtime/context.go (1)
GetDAGContext(50-52)internal/llm/types.go (2)
Role(8-8)ChatRequest(126-143)
ui/src/features/dags/components/DAGStatus.tsx (4)
ui/src/api/v2/schema.ts (1)
components(1265-2210)ui/src/components/ui/tabs.tsx (1)
Tab(49-49)ui/src/features/dags/components/chat-history/ChatHistoryTab.tsx (1)
ChatHistoryTab(12-99)ui/src/features/dags/components/chat-history/index.ts (1)
ChatHistoryTab(1-1)
internal/service/frontend/api/v2/transformer.go (2)
internal/core/llm.go (1)
LLMMessage(98-103)api/v2/api.gen.go (3)
ChatMessage(285-294)ChatMessageRole(297-297)ChatMessageMetadata(300-315)
ui/src/components/ui/markdown.tsx (1)
ui/src/lib/utils.ts (1)
cn(4-6)
internal/runtime/builtin/chat/executor_test.go (3)
internal/runtime/context.go (1)
WithSecrets(38-38)internal/core/llm.go (1)
LLMMessage(98-103)internal/llm/types.go (4)
Role(8-8)RoleUser(14-14)RoleSystem(12-12)RoleAssistant(16-16)
ui/src/features/dags/components/chat-history/StepMessagesTable.tsx (5)
api/v2/api.gen.go (1)
ChatMessageRole(297-297)ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)ui/src/hooks/api.ts (1)
useQuery(28-28)ui/src/lib/utils.ts (1)
cn(4-6)ui/src/components/ui/markdown.tsx (1)
Markdown(11-71)
ui/src/api/v2/schema.ts (1)
api/v2/api.gen.go (1)
ChatMessageRole(297-297)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test on ubuntu-latest
- GitHub Check: Build
🔇 Additional comments (28)
internal/runtime/builtin/chat/executor_test.go (1)
464-475: LGTM! Well-structured test helper.The helper correctly handles nil secrets and builds the context with proper "KEY=value" format expected by
exec.WithSecrets.ui/package.json (1)
106-109: LGTM! Dependencies are appropriate for the new Markdown rendering feature.The
react-markdown^10.1.0 andremark-gfm^4.0.1 versions are compatible with React 19 and correctly placed in runtime dependencies.ui/src/features/dags/components/chat-history/index.ts (1)
1-1: LGTM!Clean barrel export following standard module organization patterns.
internal/service/frontend/api/v2/transformer.go (1)
304-331: LGTM! Clean transformer implementation.The function follows the established patterns in this file:
- Proper nil handling with empty slice return
- Pre-allocated slice with correct capacity
- Consistent use of
ptrOffor optional metadata fieldsThe direct cast
api.ChatMessageRole(msg.Role)works because both types are string-based.internal/runtime/builtin/chat/executor.go (2)
249-273: LGTM! Well-implemented secret masking for LLM provider calls.The function correctly:
- Returns early when no secrets exist (avoiding unnecessary processing)
- Preserves message Role and Metadata while only masking Content
- Creates a new slice rather than mutating the input
291-295: Verify: Intentional design to save unmasked messages.The code masks messages only for the LLM provider call (line 295), while
savedMessages(line 356) stores the original unmaskedallMessages. This means:
- Secrets are protected from external LLM APIs ✓
- Chat history displayed in UI will show unmasked content
Please confirm this is the intended behavior - users will see their original messages (including any secrets they typed) in the chat history UI.
ui/src/features/dags/components/DAGStatus.tsx (2)
12-43: Chat tab wiring + StatusTab update looks consistent.
Imports andStatusTabunion update are straightforward.
322-441: Conditional Chat tab + content render is clean and avoids showing non-functional UI.internal/service/frontend/api/v2/dagruns.go (2)
720-725: No action needed.node.Status.String()output exactly matches the OpenAPINodeStatusLabelenum values (lowercase with underscores, e.g.,"succeeded","not_started"). The code is correct as written.
677-775: No changes needed. The code already handles missing messages correctly.ReadStepMessages()returns(nil, nil)when the message file doesn't exist (per the test casesReadNonExistentStepMessagesandWriteEmptyMessages), so the handler never executes the error path for this scenario. The response correctly converts the nil messages to an empty array viatoChatMessages(), matching the OpenAPI contract ("Returns empty array for non-chat steps").If other error types (e.g., file read or JSON unmarshal failures) should also return 200 with empty messages rather than 500, that would be a separate concern worth addressing. The suggested detection of non-chat steps via
ExecutorConfig.Typeis also incomplete without specifying how to reliably identify non-chat step types from the available node data.Likely an incorrect or invalid review comment.
ui/src/features/dags/components/chat-history/StepMessagesTable.tsx (6)
1-7: LGTM!Imports are clean and appropriately organized. Good use of the custom
useQueryhook and typed schema imports.
9-18: LGTM!Well-defined interface with clear prop names. The sub-DAG context props (
subDAGRunId,rootDagName,rootDagRunId) are appropriately optional.
20-30: LGTM!Role configuration with semantic border colors is a clean pattern. The fallback
defaultRoleConfighandles unknown roles gracefully.
121-136: LGTM!Loading and empty states are compact and appropriate. The conditional
isLoading && messages.length === 0correctly shows stale data while fetching updates, aligning with coding guidelines.
152-180: LGTM!The header row implementation is well-structured:
- Compact styling with
px-2 py-1- Accessible button with
type="button"- Smart collapsed preview with truncation
- Proper rotation animation for the chevron
182-218: LGTM!Expanded content section is well-implemented:
- Good use of Markdown component for rich content rendering
- Metadata display is informative and compact
- Copy button with visual feedback is a nice UX touch
e.stopPropagation()correctly prevents toggle when clicking copyui/src/api/v2/schema.ts (2)
1312-1344: LGTM!The ChatMessage-related schemas are well-structured:
ChatMessagewith role, content, and optional metadata follows standard LLM conversation patternsChatMessageMetadatacaptures useful token usage and model infoChatMessagesResponseappropriately includes step status context andhasMorefor polling scenarios
5870-5875: LGTM!The
ChatMessageRoleenum values (system,user,assistant,tool) align with standard LLM API conventions used by OpenAI, Anthropic, and others.api/v2/api.gen.go (10)
31-38: ChatMessageRole enum values look consistent and usable. The typed constants for"assistant" | "system" | "tool" | "user"are a good addition for compile-time safety.
1688-1693: New params structs are consistent with the rest of the API. OptionalremoteNodequery param matches existing endpoint patterns.Also applies to: 1787-1792
2383-2386: Server interface expansion looks correct; confirm auth parity with logs. The two new handler methods match the new routes; please ensure the implementation applies the same authn/z + audit expectations as/logendpoints.Also applies to: 2410-2412
2668-2672: Unimplemented stubs are correctly generated.Also applies to: 2722-2726
4043-4103: Wrapper binding for new endpoints looks correct. Path params andremoteNodequery binding follow the established pattern and should integrate cleanly with middleware.Also applies to: 4681-4750
6563-6565: Route registrations are correct and non-conflicting. The new.../messagesGET routes are added alongside existing step log routes inHandlerWithOptions.Also applies to: 6590-6592
7740-7779: Request/response object types for strict mode look consistent.200returnsChatMessagesResponse,404/defaultfollow existing conventions.Also applies to: 8133-8173
9891-9894: StrictServerInterface additions are correctly wired.Also applies to: 9918-9920
10715-10742: strictHandler methods correctly delegate to StrictServerInterface. Middleware hook names and request object casting match the generated pattern.Also applies to: 10997-11025
12168-12442: Generated swaggerSpec update is expected; avoid manual edits here. Just ensureapi/v2/api.yamlis the source of truth and the generator version is consistent across environments.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1578 +/- ##
=======================================
Coverage 64.78% 64.79%
=======================================
Files 255 255
Lines 28380 28396 +16
=======================================
+ Hits 18387 18400 +13
- Misses 8346 8348 +2
- Partials 1647 1648 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Release Notes
New Features
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.