frontend: include offline doc within binary and improve JSON schema#1520
Conversation
📝 WalkthroughWalkthroughThe PR implements a comprehensive schema documentation feature in the DAG editor UI. It introduces schema loading infrastructure via React context, enables cursor position tracking in Monaco Editor, adds schema navigation utilities for YAML paths, and implements a sidebar UI component tree to display schema properties at the current cursor location. The JSON Schema definition is enhanced with executor type configuration mappings and centralized validation rules. Changes
Sequence DiagramsequenceDiagram
participant User
participant DAGEditor as DAG Editor
participant DAGSpec
participant SchemaContext as Schema Context
participant Hooks as Hooks<br/>(useYamlCursorPath<br/>useSchemaLookup)
participant Sidebar as Schema Doc<br/>Sidebar
User->>DAGEditor: Move cursor
DAGEditor->>DAGEditor: Detect cursor change
DAGEditor->>DAGSpec: onCursorPositionChange(position)
DAGSpec->>DAGSpec: Debounce cursor position<br/>(delay 300ms)
DAGSpec->>Hooks: useYamlCursorPath(content,<br/>line, column)
Hooks->>Hooks: Parse YAML, find<br/>node at offset
Hooks->>DAGSpec: Return path & segments
DAGSpec->>SchemaContext: useSchema()
SchemaContext->>SchemaContext: Fetch & cache schema<br/>(dag.schema.json)
SchemaContext->>Hooks: useSchemaLookup(path)
Hooks->>Hooks: getSchemaAtPath,<br/>toPropertyInfo
Hooks->>DAGSpec: Return propertyInfo<br/>& siblings
DAGSpec->>Sidebar: Pass path, segments,<br/>isOpen
Sidebar->>Sidebar: Render breadcrumb,<br/>property info,<br/>nested tree
Sidebar->>User: Display schema docs
User->>User: Press Ctrl+Shift+D
User->>DAGSpec: Toggle sidebar
DAGSpec->>Sidebar: isOpen = !isOpen
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
🧹 Nitpick comments (11)
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaDocSidebar.tsx (1)
168-175: Consider cross-platform keyboard hint.The footer shows
Ctrl+Shift+Dbut Mac users typically expectCmd. Consider detecting the platform or showing both.🔎 Example platform-aware hint
<div className="px-3 py-1.5 border-t border-border bg-muted/20"> <span className="text-[10px] text-muted-foreground"> - Press <kbd className="px-1 py-0.5 bg-muted text-foreground rounded text-[9px]">Ctrl</kbd> + Press <kbd className="px-1 py-0.5 bg-muted text-foreground rounded text-[9px]">⌘/Ctrl</kbd> +<kbd className="px-1 py-0.5 bg-muted text-foreground rounded text-[9px]">Shift</kbd> +<kbd className="px-1 py-0.5 bg-muted text-foreground rounded text-[9px]">D</kbd> to toggle </span> </div>ui/src/features/dags/components/dag-editor/SchemaDocSidebar/NestedPropertiesTree.tsx (1)
59-91: Consider keyboard accessibility for expandable items.The expand/collapse functionality only responds to click events. For accessibility, consider adding keyboard support (Enter/Space to toggle).
🔎 Keyboard accessibility suggestion
<div className={cn( 'flex items-center gap-1 py-0.5 px-1 rounded hover:bg-muted/50', canExpand && 'cursor-pointer' )} onClick={() => canExpand && setIsExpanded(!isExpanded)} + onKeyDown={(e) => { + if (canExpand && (e.key === 'Enter' || e.key === ' ')) { + e.preventDefault(); + setIsExpanded(!isExpanded); + } + }} + tabIndex={canExpand ? 0 : undefined} + role={canExpand ? 'button' : undefined} + aria-expanded={canExpand ? isExpanded : undefined} >ui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPathBreadcrumb.tsx (1)
34-43: Hardcoded colors lack dark mode support.The array index styling uses hardcoded
rgba(196,158,106,0.15)and#9a7a4awhich won't adapt to dark mode. As per coding guidelines, use Tailwind dark mode class pairs.🔎 Dark mode compatible styling
<span className={cn( 'px-1 py-0.5 rounded text-foreground', segment.isArrayIndex - ? 'font-mono bg-[rgba(196,158,106,0.15)] text-[#9a7a4a]' + ? 'font-mono bg-amber-100 text-amber-700 dark:bg-amber-900/30 dark:text-amber-400' : 'bg-muted' )} >Based on coding guidelines, dark mode support is required for all UI components.
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPropertyInfo.tsx (1)
90-109: Consider truncating or collapsing large examples.The
JSON.stringify(example, null, 2)on line 104 produces formatted multi-line output. For complex nested objects, this could create very tall blocks. Thebreak-allclass handles horizontal overflow, but vertical space consumption might be a concern for the compact sidebar.Consider limiting example display or adding a collapsible wrapper for large examples if this becomes an issue in practice.
ui/src/contexts/SchemaContext.tsx (2)
47-72: Consider adding circular reference protection.The recursive
processNodefunction doesn't track visited nodes. If the schema contains circular$refdefinitions, this could cause a stack overflow. Consider adding aSetto track visited references.🔎 Example cycle detection
function processNode(node: unknown, visited = new Set<string>()): unknown { // ... if (typeof obj.$ref === 'string') { + if (visited.has(obj.$ref)) { + return obj; // Break cycle, return as-is + } + visited.add(obj.$ref); const resolved = resolveRef(obj.$ref); const { $ref, ...rest } = obj; - return processNode({ ...resolved, ...rest }); + return processNode({ ...resolved, ...rest }, visited); } // ... }
129-137:reloadfunction is not stable in useMemo dependencies.The
reloadfunction referencesloadSchemawhich is recreated on each render. However,reloadis excluded from theuseMemodependency array, meaning the context value will have a stalereloadreference untilschema,loading, orerrorchanges. This is likely fine in practice sincereloadis rarely called, but it's technically a closure issue.ui/src/hooks/useSchemaLookup.ts (1)
24-42: Array reference in useMemo dependency may cause excessive recalculations.The
patharray in the dependency array (line 42) triggers recalculation whenever a new array reference is passed, even if the contents are identical. If the consumer creates a new array on each render (e.g.,['steps', '0', 'name']), the memoization won't prevent recalculation.Consider using a stringified path or a custom comparison hook for stable memoization.
🔎 Suggested fix using JSON.stringify for stable dependency
+ const pathKey = JSON.stringify(path); + const result = useMemo(() => { if (!schema || path.length === 0) { return { propertyInfo: null, siblingProperties: [], }; } // ... rest of logic - }, [schema, path]); + }, [schema, pathKey]);ui/src/features/dags/components/dag-editor/DAGSpec.tsx (1)
269-280: Keyboard shortcut may trigger when typing in other inputs.The
Ctrl+Shift+Dshortcut is registered ondocument, so it will fire even when the user is typing in other form fields. WhileCtrl+Shift+Dis unlikely to conflict with normal typing, consider checkingevent.targetto avoid triggering when focused on input elements outside the editor.🔎 Example scope check
useEffect(() => { const handleKeyDown = (event: KeyboardEvent) => { + // Don't trigger shortcut when typing in form inputs + const target = event.target as HTMLElement; + if (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA') { + return; + } if ((event.ctrlKey || event.metaKey) && event.shiftKey && event.key === 'd') { event.preventDefault(); toggleSidebar(); } };ui/src/hooks/useYamlCursorPath.ts (2)
73-175: ThewalkNodefunction mutates outer scope arrays.The function modifies
pathandsegmentsarrays from the outer scope usinglength = 0andpush(...items). While functional, this pattern is error-prone and harder to reason about. Consider returning results from recursive calls instead.
243-270: Fallback array index counting logic may miscalculate in complex cases.The backward scan for counting array items (lines 247-257) counts
-markers at the same indent level. However, this doesn't account for nested arrays or multi-line array items, which could lead to incorrect index values in complex YAML structures. Since this is a fallback for when YAML parsing fails, it's acceptable but worth noting.schemas/dag.schema.json (1)
1595-1666: Consider addingrequired: ["source"]for archive executor config.The
sourceproperty description states "Required for all operations" (line 1601), but there's norequiredarray to enforce this at the schema level. Adding schema-level validation would catch configuration errors earlier.🔎 Proposed fix
"archiveExecutorConfig": { "type": "object", "additionalProperties": false, + "required": ["source"], "properties": { "source": { "type": "string", - "description": "Source path for archive operations. Required for all operations." + "description": "Source path for archive operations." },
📜 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 (18)
Makefileschemas/dag.schema.jsonui/package.jsonui/src/App.tsxui/src/contexts/SchemaContext.tsxui/src/features/dags/components/dag-editor/DAGEditor.tsxui/src/features/dags/components/dag-editor/DAGSpec.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/NestedPropertiesTree.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/PropertyTypeDisplay.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaDocSidebar.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPathBreadcrumb.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPropertyInfo.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/index.tsui/src/hooks/useDebouncedValue.tsui/src/hooks/useSchemaLookup.tsui/src/hooks/useYamlCursorPath.tsui/src/lib/schema-utils.tsui/webpack.dev.js
🧰 Additional context used
📓 Path-based instructions (2)
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/hooks/useDebouncedValue.tsui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPropertyInfo.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/NestedPropertiesTree.tsxui/webpack.dev.jsui/src/features/dags/components/dag-editor/SchemaDocSidebar/PropertyTypeDisplay.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/index.tsui/src/contexts/SchemaContext.tsxui/src/hooks/useSchemaLookup.tsui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPathBreadcrumb.tsxui/src/features/dags/components/dag-editor/DAGEditor.tsxui/src/hooks/useYamlCursorPath.tsui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaDocSidebar.tsxui/src/lib/schema-utils.tsui/src/App.tsxui/src/features/dags/components/dag-editor/DAGSpec.tsx
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/hooks/useDebouncedValue.tsui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPropertyInfo.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/NestedPropertiesTree.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/PropertyTypeDisplay.tsxui/src/features/dags/components/dag-editor/SchemaDocSidebar/index.tsui/src/contexts/SchemaContext.tsxui/src/hooks/useSchemaLookup.tsui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPathBreadcrumb.tsxui/src/features/dags/components/dag-editor/DAGEditor.tsxui/src/hooks/useYamlCursorPath.tsui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaDocSidebar.tsxui/src/lib/schema-utils.tsui/src/App.tsxui/src/features/dags/components/dag-editor/DAGSpec.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: PR descriptions must link related issues, outline risk areas, and attach UI screenshots or sample payloads when touching `ui/` or `api/` schemas
📚 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 consistent metadata styling with `bg-slate-200 dark:bg-slate-700` backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Applied to files:
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/PropertyTypeDisplay.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/**/.gitignore,** : Use pnpm for frontend package management instead of npm or yarn
Applied to files:
ui/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/**/Makefile : Ensure frontend builds without errors by running `make ui` before committing changes
Applied to files:
Makefile
🧬 Code graph analysis (5)
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPropertyInfo.tsx (5)
ui/src/lib/schema-utils.ts (1)
SchemaPropertyInfo(30-46)ui/src/features/dags/components/dag-editor/SchemaDocSidebar/index.ts (3)
SchemaPropertyInfo(3-3)PropertyTypeDisplay(4-4)NestedPropertiesTree(5-5)ui/src/lib/utils.ts (1)
cn(4-6)ui/src/features/dags/components/dag-editor/SchemaDocSidebar/PropertyTypeDisplay.tsx (1)
PropertyTypeDisplay(22-49)ui/src/features/dags/components/dag-editor/SchemaDocSidebar/NestedPropertiesTree.tsx (1)
NestedPropertiesTree(14-39)
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/PropertyTypeDisplay.tsx (2)
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/index.ts (1)
PropertyTypeDisplay(4-4)ui/src/lib/utils.ts (1)
cn(4-6)
ui/src/contexts/SchemaContext.tsx (1)
ui/src/lib/schema-utils.ts (1)
JSONSchema(5-28)
ui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPathBreadcrumb.tsx (3)
ui/src/hooks/useYamlCursorPath.ts (1)
YamlPathSegment(4-7)ui/src/features/dags/components/dag-editor/SchemaDocSidebar/index.ts (1)
SchemaPathBreadcrumb(2-2)ui/src/lib/utils.ts (1)
cn(4-6)
ui/src/App.tsx (1)
ui/src/contexts/SchemaContext.tsx (1)
SchemaProvider(99-142)
⏰ 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)
ui/webpack.dev.js (1)
14-24: ✓ Solid targeted fix for Monaco Editor development friction.The ResizeObserver error is indeed a known, benign issue with Monaco Editor's layout calculations. Your filter is minimal, safe (optional chaining handles missing error.message), and scoped specifically to development mode. This reduces noise without masking real errors.
ui/package.json (1)
111-112: LGTM - Appropriate runtime dependency addition.The
yamlpackage is a well-maintained library suitable for YAML parsing needs. Placing it independenciesis correct since it will be used at runtime for cursor path extraction in the editor.ui/src/hooks/useDebouncedValue.ts (1)
1-20: LGTM - Clean debounce hook implementation.The implementation follows React best practices with proper cleanup via
clearTimeoutin the effect's return function, preventing memory leaks and stale updates.ui/src/App.tsx (1)
112-174: LGTM - Schema context integration.The
SchemaProviderplacement is appropriate, making schema data available to all protected routes including the DAG editor. The provider correctly wraps the router, ensuring schema is loaded once and cached globally.ui/src/features/dags/components/dag-editor/SchemaDocSidebar/index.ts (1)
1-5: LGTM - Clean barrel exports.The index file correctly re-exports all SchemaDocSidebar components, enabling clean imports like
import { SchemaDocSidebar, SchemaPropertyInfo } from './SchemaDocSidebar'.Makefile (1)
364-364: LGTM - Schema asset copy follows existing patterns.The addition correctly copies the schema file to frontend assets alongside the build output. The source file
schemas/dag.schema.jsonexists and is valid JSON.ui/src/features/dags/components/dag-editor/SchemaDocSidebar/PropertyTypeDisplay.tsx (1)
22-48: Component structure looks good.The component correctly handles both single and array type props, uses proper className composition with
cn(), and renders badges with appropriate compact sizing (text-[10px],px-1.5 py-0.5). The conditional required badge rendering is clean.ui/src/features/dags/components/dag-editor/SchemaDocSidebar/SchemaPropertyInfo.tsx (2)
1-14: Imports and interface definition look correct.Clean import structure with type-only import for
SchemaPropertyInfoType. The component props interface is appropriately minimal.
15-166: Well-structured component with comprehensive property rendering.The component handles all schema property facets (type, description, default, enum, format, pattern, examples, oneOf, nested properties, array items) with consistent styling and appropriate conditional rendering. The compact styling with
text-[10px]andtext-xsaligns with coding guidelines for high information density.ui/src/features/dags/components/dag-editor/DAGEditor.tsx (2)
33-39: Good addition of cursor position tracking API.The
CursorPositioninterface is cleanly defined and exported. The optionalonCursorPositionChangeprop allows consumers to react to cursor movements. This design follows React patterns well.Also applies to: 57-58, 71-71
12-14: This pattern is safe and established throughout the codebase. ThegetConfig()function is defined globally inindex.htmlbefore any React/module code executes, and is declared as a global inindex.tsx. Module-level calls togetConfig()(as in this file andSchemaContext.tsx) are secure because they only execute after the page's initial script has run.ui/src/contexts/SchemaContext.tsx (1)
77-97: Caching implementation is well-designed.The caching logic correctly handles concurrent requests by storing the promise (
cachePromise) to avoid duplicate fetches. The error message includes the status text for debugging.ui/src/hooks/useSchemaLookup.ts (1)
54-68:useRootSchemaPropertieshook is clean and correct.Simple and effective extraction of root-level property names with proper memoization.
ui/src/features/dags/components/dag-editor/DAGSpec.tsx (3)
53-66: Good localStorage persistence pattern with error handling.The state initializer correctly wraps localStorage access in try-catch and provides a sensible default. Defaulting to open (
true) when no preference is saved is user-friendly.
68-76: Debounced cursor position is well-implemented.Using
useDebouncedValuewith 150ms delay prevents excessive re-renders during rapid cursor movement. The YAML path extraction is appropriately derived from the debounced position.
476-497: Layout structure handles the two-pane design correctly.The flex layout with
min-w-0on the editor container and proper flex-1 usage ensures the editor and sidebar coexist without overflow issues. TheSchemaDocSidebarreceives the derived path and segments correctly.ui/src/hooks/useYamlCursorPath.ts (1)
299-325: Hook implementation is correctly memoized.The
useYamlCursorPathhook properly memoizes both the offset calculation and path extraction with appropriate dependencies. This prevents unnecessary recalculations.ui/src/lib/schema-utils.ts (4)
5-46: Comprehensive type definitions for JSON Schema handling.Both
JSONSchemaandSchemaPropertyInfointerfaces cover the necessary fields for navigation and display. The separation between raw schema structure and display-friendly info is a good design choice.
51-119: Robust schema path resolution with good edge case handling.The
getSchemaAtPathfunction correctly handles:
- Direct property access
- Array index access with item schema resolution
- Union types (oneOf/anyOf)
- Composition (allOf)
- Additional properties fallback
The implementation is thorough and follows JSON Schema semantics.
280-296: Empty union type arrays could return'unknown'incorrectly.If
schema.oneOforschema.anyOfis an empty array, thetypesarray will be empty, andtypes[0] ?? 'unknown'will return'unknown'. While edge-case, an emptyoneOf/anyOfis invalid JSON Schema, so this is acceptable behavior.
217-270:toPropertyInfoconversion is well-structured.The function properly:
- Resolves types including from unions
- Tracks required status from parent
- Recursively processes nested properties and items
- Handles oneOf variants with indexed naming
Minor note: Deep schemas could cause significant recursion, but this is inherent to the data structure.
schemas/dag.schema.json (7)
558-571: Well-structured executor type definition with comprehensive enum.The explicit enum of executor types with aliases (e.g.,
docker/container,gha/github_action/github-action) provides better validation and IDE support while maintaining backward compatibility. TheoneOfpattern allowing both string shorthand and fullexecutorObjectis a clean API design.
1317-1358: HTTP executor config is well-defined.The configuration options cover the common HTTP request parameters with appropriate types. Good use of
additionalProperties: falsefor strict validation.
1404-1466: Docker executor config is comprehensive.Good coverage of Docker configuration options. The
additionalProperties: trueon nestedcontainer,host,network, andexecobjects appropriately allows passthrough of raw Docker API configurations.Minor observation:
autoRemoveaccepting both boolean and string (lines 1429-1432) is slightly unusual—verify this aligns with the actual executor implementation accepting string values like"true".
1467-1498: Mail executor config is well-defined with proper required fields.Required fields are correctly specified, and the
tofield appropriately supports both single recipients and arrays.
1499-1509: Simple and appropriate jq executor config.The minimal
rawoption aligns with jq's typical usage where the filter expression would be in the command field.
1667-1731: Excellent conditional validation structure for executor configs.The
allOfwithif/thenpattern correctly maps each executor type to its corresponding config schema. The explicit documentation for executors that don't useconfig(command/shell, dag/subworkflow, parallel) is helpful for users and tooling.
1510-1517: No action required. The default runnernode:24-bookwormis valid and appropriate. Node.js 24 was released in May 2025 and entered Active LTS in October 2025, making it the recommended LTS version for production. The Docker image is available on Docker Hub.
| "sshExecutorConfig": { | ||
| "type": "object", | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "user": { | ||
| "type": "string", | ||
| "description": "SSH username for authentication. Required." | ||
| }, | ||
| "host": { | ||
| "type": "string", | ||
| "description": "Hostname or IP address of the SSH server. Required." | ||
| }, | ||
| "ip": { | ||
| "type": "string", | ||
| "description": "Alias for 'host'. IP address of the SSH server." | ||
| }, | ||
| "port": { | ||
| "oneOf": [ | ||
| { "type": "string" }, | ||
| { "type": "integer" } | ||
| ], | ||
| "default": "22", | ||
| "description": "SSH port number. Defaults to 22." | ||
| }, | ||
| "key": { | ||
| "type": "string", | ||
| "description": "Path to SSH private key file. If not specified, defaults are tried: ~/.ssh/id_rsa, ~/.ssh/id_ecdsa, ~/.ssh/id_ed25519, ~/.ssh/id_dsa." | ||
| }, | ||
| "password": { | ||
| "type": "string", | ||
| "description": "SSH password for authentication. Not recommended; prefer key-based authentication." | ||
| }, | ||
| "strictHostKey": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Enable strict host key checking. When true (default), verifies server's host key against known_hosts." | ||
| }, | ||
| "knownHostFile": { | ||
| "type": "string", | ||
| "default": "~/.ssh/known_hosts", | ||
| "description": "Path to known_hosts file for host key verification." | ||
| } | ||
| }, | ||
| "description": "SSH executor configuration for remote command execution." | ||
| }, |
There was a problem hiding this comment.
Missing required array despite "Required." in descriptions.
The descriptions for user (line 1365) and host (line 1369) state "Required." but unlike the existing sshConfig definition (which has required: ["user", "host"]), this definition lacks a required array. This inconsistency may allow invalid configurations to pass validation.
🔎 Proposed fix
"sshExecutorConfig": {
"type": "object",
"additionalProperties": false,
+ "required": ["user", "host"],
"properties": {
"user": {
"type": "string",
- "description": "SSH username for authentication. Required."
+ "description": "SSH username for authentication."
},
"host": {
"type": "string",
- "description": "Hostname or IP address of the SSH server. Required."
+ "description": "Hostname or IP address of the SSH server."
},📝 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.
| "sshExecutorConfig": { | |
| "type": "object", | |
| "additionalProperties": false, | |
| "properties": { | |
| "user": { | |
| "type": "string", | |
| "description": "SSH username for authentication. Required." | |
| }, | |
| "host": { | |
| "type": "string", | |
| "description": "Hostname or IP address of the SSH server. Required." | |
| }, | |
| "ip": { | |
| "type": "string", | |
| "description": "Alias for 'host'. IP address of the SSH server." | |
| }, | |
| "port": { | |
| "oneOf": [ | |
| { "type": "string" }, | |
| { "type": "integer" } | |
| ], | |
| "default": "22", | |
| "description": "SSH port number. Defaults to 22." | |
| }, | |
| "key": { | |
| "type": "string", | |
| "description": "Path to SSH private key file. If not specified, defaults are tried: ~/.ssh/id_rsa, ~/.ssh/id_ecdsa, ~/.ssh/id_ed25519, ~/.ssh/id_dsa." | |
| }, | |
| "password": { | |
| "type": "string", | |
| "description": "SSH password for authentication. Not recommended; prefer key-based authentication." | |
| }, | |
| "strictHostKey": { | |
| "type": "boolean", | |
| "default": true, | |
| "description": "Enable strict host key checking. When true (default), verifies server's host key against known_hosts." | |
| }, | |
| "knownHostFile": { | |
| "type": "string", | |
| "default": "~/.ssh/known_hosts", | |
| "description": "Path to known_hosts file for host key verification." | |
| } | |
| }, | |
| "description": "SSH executor configuration for remote command execution." | |
| }, | |
| "sshExecutorConfig": { | |
| "type": "object", | |
| "additionalProperties": false, | |
| "required": ["user", "host"], | |
| "properties": { | |
| "user": { | |
| "type": "string", | |
| "description": "SSH username for authentication." | |
| }, | |
| "host": { | |
| "type": "string", | |
| "description": "Hostname or IP address of the SSH server." | |
| }, | |
| "ip": { | |
| "type": "string", | |
| "description": "Alias for 'host'. IP address of the SSH server." | |
| }, | |
| "port": { | |
| "oneOf": [ | |
| { "type": "string" }, | |
| { "type": "integer" } | |
| ], | |
| "default": "22", | |
| "description": "SSH port number. Defaults to 22." | |
| }, | |
| "key": { | |
| "type": "string", | |
| "description": "Path to SSH private key file. If not specified, defaults are tried: ~/.ssh/id_rsa, ~/.ssh/id_ecdsa, ~/.ssh/id_ed25519, ~/.ssh/id_dsa." | |
| }, | |
| "password": { | |
| "type": "string", | |
| "description": "SSH password for authentication. Not recommended; prefer key-based authentication." | |
| }, | |
| "strictHostKey": { | |
| "type": "boolean", | |
| "default": true, | |
| "description": "Enable strict host key checking. When true (default), verifies server's host key against known_hosts." | |
| }, | |
| "knownHostFile": { | |
| "type": "string", | |
| "default": "~/.ssh/known_hosts", | |
| "description": "Path to known_hosts file for host key verification." | |
| } | |
| }, | |
| "description": "SSH executor configuration for remote command execution." | |
| }, |
🤖 Prompt for AI Agents
In schemas/dag.schema.json around lines 1359 to 1403, the sshExecutorConfig
object documents that "user" and "host" are required but the schema lacks a
required array; add a required: ["user", "host"] entry to the sshExecutorConfig
definition (adjacent to properties) to match the existing sshConfig pattern so
validation enforces those fields.
| useEffect(() => { | ||
| if (!cachedSchema) { | ||
| loadSchema(); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Missing dependency in useEffect.
The useEffect references loadSchema in its body but excludes it from the dependency array. While loadSchema is defined inside the component and would change on every render, this pattern can lead to stale closure issues. Consider using useCallback for loadSchema or restructuring to avoid the lint warning.
🔎 Suggested restructure using useCallback
- const loadSchema = async () => {
+ const loadSchema = useCallback(async () => {
setLoading(true);
setError(null);
try {
const loadedSchema = await loadAndDereferenceSchema();
setSchema(loadedSchema);
} catch (err) {
setError(err instanceof Error ? err : new Error('Failed to load schema'));
} finally {
setLoading(false);
}
- };
+ }, []);
useEffect(() => {
if (!cachedSchema) {
loadSchema();
}
- }, []);
+ }, [loadSchema]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ui/src/contexts/SchemaContext.tsx around lines 117 to 121, the useEffect
calls loadSchema but omits it from the dependency array, causing a lint warning
and potential stale closures; refactor loadSchema into a stable callback (wrap
it with useCallback with its real dependencies) and then include that memoized
loadSchema in the useEffect dependency array, or alternatively move loadSchema
outside the component or inline the loading logic into the effect so the
effect’s dependency list is accurate and ESLint no longer warns.
|
|
||
| // Listen for cursor position changes | ||
| if (onCursorPositionChange) { | ||
| // Initial position | ||
| const position = editor.getPosition(); | ||
| if (position) { | ||
| onCursorPositionChange({ | ||
| lineNumber: position.lineNumber, | ||
| column: position.column, | ||
| }); | ||
| } | ||
|
|
||
| // Subscribe to cursor changes | ||
| editor.onDidChangeCursorPosition((e) => { | ||
| onCursorPositionChange({ | ||
| lineNumber: e.position.lineNumber, | ||
| column: e.position.column, | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing disposal of cursor position change listener.
The editor.onDidChangeCursorPosition subscription on line 151 returns an IDisposable that is not being tracked or disposed. When the component unmounts or when onCursorPositionChange changes, the old listener will persist, potentially causing memory leaks or stale callback invocations.
🔎 Proposed fix to properly dispose the listener
+ const cursorListenerRef = useRef<monaco.IDisposable | null>(null);
+
// Clean up editor on unmount
useEffect(() => {
return () => {
+ cursorListenerRef.current?.dispose();
editorRef.current?.dispose();
};
}, []);Then in editorDidMount:
// Subscribe to cursor changes
- editor.onDidChangeCursorPosition((e) => {
+ cursorListenerRef.current = editor.onDidChangeCursorPosition((e) => {
onCursorPositionChange({
lineNumber: e.position.lineNumber,
column: e.position.column,
});
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ui/src/features/dags/components/dag-editor/DAGEditor.tsx around lines 138 to
157, the IDiposable returned by editor.onDidChangeCursorPosition is not stored
or disposed; capture the returned disposable when subscribing and ensure you
call disposable.dispose() during the component/Editor cleanup (e.g., in the
editorDidMount return/cleanup or a useEffect cleanup when onCursorPositionChange
changes) so the listener is removed when the component unmounts or the callback
changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1520 +/- ##
==========================================
- Coverage 63.26% 63.23% -0.04%
==========================================
Files 211 211
Lines 23702 23709 +7
==========================================
- Hits 14996 14992 -4
- Misses 7276 7287 +11
Partials 1430 1430 see 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…1520) * **New Features** * Added schema documentation sidebar in the DAG editor for reference while editing. * New keyboard shortcut (Ctrl/Cmd+Shift+D) to toggle schema documentation panel. * Enhanced executor configuration validation with type-specific schemas. * **Improvements** * Improved cursor position tracking in the editor for better navigation context.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.