Conversation
… diff review Introduces a full in-browser skill editor reachable from the skill detail page, replacing the previous read-only detail view for edits. Editor - Two-pane layout with Markdown textarea, live preview, and synced scroll - Edit / Split / Preview mode toggle in the status bar (⌘P to cycle) - Keyboard shortcuts: ⌘S save (with diff review), Esc cancel - Outline drawer with jump-to-heading - Token / word / line / file count in status bar, 5K budget warning Frontmatter - Grouped field editor (Identity / Invocation / Execution / Metadata) matching the 13 official SKILL.md fields - Shared 1,536-char budget enforced across description + when_to_use - Switch toggles for booleans, segmented control for enums, chip array inputs for list-valued fields - Metadata group edits nested metadata.* keys directly, with stable row ids so typing key → clicking value no longer remounts the inputs - Special handling for metadata.targets so values serialize as a YAML list (backend ParseFrontmatterList requires arrays) - YAML mode for raw editing; Fields ↔ YAML round-trips cleanly - Legacy root-level 'targets:' migrated to metadata.targets on load Diff review - Save triggers a side-by-side diff modal built on the shared DialogShell with focus trap and backdrop, using the Button component for actions - YAML serializer emits plain scalars when safe instead of wrapping every value containing ':', '*', '#', or '"', eliminating spurious churn in the save diff Misc - DialogShell gains 4xl–7xl maxWidth presets - Resource detail sidebar shows a Targets row when metadata.targets is set Backend additions (server handlers): - POST /api/resources/:name/content save SKILL.md content - POST /api/resources/:name/open open in local editor
…rop dead code
- Drop unused EditorCustomField / initialCustomFields / draftTargets / FileText
re-export; narrow onSaved signature to (nextContent: string).
- Gate DiffView render on showDiff so the O(n·m) LCS + composeSkillMarkdown
don't run on every keystroke while the dialog is closed.
- Route the markdown preview and Outline through useDeferredValue so typing
stays responsive while react-markdown catches up.
- Serialize frontmatter YAML lazily (only when yamlMode is active).
- Equality-guard the metadata-row reconcile effect to avoid no-op setState
storms on every frontmatter keystroke.
- Memoize derived stats (tokens/words/lines) into a single pass instead of
two splits per render.
- Replace the private YAML parser in ResourceDetailPage with
lib/frontmatter's parseSkillMarkdown; removes ~60 LOC of duplicate logic.
- Remove WHAT-narrating comments ("Keyboard shortcuts", "Derived stats",
"Stage 1: Content Stats Bar", etc.).
Follow-up to the earlier simplify pass — addresses the larger refactors called out by the three review agents: - Drop local SegmentedField in favour of the shared SegmentedControl used across the dashboard; reuse it for the Fields/YAML toggle, the Edit/Split/Preview mode switcher, and the enum frontmatter fields. Removes ~34 lines of duplicated .seg-group / .seg-btn CSS. - Split the 80-line FrontmatterField if/else into per-type subcomponents (EnumField / BoolField / ArrayField / TextField) dispatched from a small FieldControl switch. The synthetic 'conditional' field type is gone — visibility is now a plain 'showWhen' predicate on a 'text' field, with an optional 'placeholder' override. - Extract a shared CollapsibleGroup wrapper; both FrontmatterGroup and FrontmatterMetadataGroup use it, deleting the copy-pasted fm-group chrome. - requestAnimationFrame-throttle the synced-scroll handlers. Previously every onScroll event (~60×/s) ran buildSyncAnchors, which measures scrollHeight / getComputedStyle / querySelectorAll and sometimes the DOM mirror. Now one pass per frame per scroll source, with RAF + timer cleanup on unmount. useLineDiff consolidation was considered but skipped — computeLineDiff returns only add/remove rows, whereas DiffView needs the eq rows for the side-by-side view, so merging would complicate the shared hook.
resolveFieldPath() was intentionally updated in 7c91308 to treat bare list values like "- skill-a" as resolvable keys (for short-form target name docs lookup), but the corresponding test still asserted the old null-returning behaviour and had been failing silently ever since.
The shared SegmentedControl has no gap between icon and label, making Edit/Split/Preview look cramped. Revert the visual to the original editor-themed .seg-btn/.seg-group CSS (4px icon gap, paper background, shadow-sm active state). Keep the DRY JSX win by introducing a small EditorSegment helper that renders the same markup the three inline call sites previously duplicated.
Replace all hardcoded English strings in DashboardPage.tsx with t() calls using the existing useT() hook from the custom i18n system. Added ~80 new dashboard.* keys to en.json covering: - Stats cards (skills, agents, targets, extras) - Source directories section - Star CTA reminder - Tracked repositories (labels, toasts, confirm dialogs) - Skill updates section (status badges, check buttons) - Security audit section (summary stats, findings) - Targets health section (drift warnings, sync status) - Version status section (CLI/skill version badges) - Quick actions (sync, audit, browse, update all) - Toast messages with parameterized interpolation Also renamed loop variable 't' to 'tgt' in TargetsHealthSection to avoid shadowing the t() translation function.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive web-based skill editor and full UI localization supporting 11 languages. Key backend enhancements include new endpoints for atomic skill content updates and opening files in local editors, alongside a more robust structured error reporting system. The frontend features a new multi-pane Markdown editor with live preview, a structured frontmatter editor, and a side-by-side diff review modal. Feedback identifies a high-severity security risk where arbitrary binaries could be executed from the system PATH, a potential path validation bug on case-insensitive filesystems, and performance concerns regarding the diff algorithm's memory footprint and excessive DOM querying during scroll synchronization.
| if _, err := exec.LookPath(requested); err == nil { | ||
| return editorCandidate{name: requested, bin: requested}, requested, nil | ||
| } | ||
| return editorCandidate{}, "", fmt.Errorf("%s not found on PATH", requested) | ||
| } |
There was a problem hiding this comment.
The pickEditor function allows executing any binary found on the system's PATH if the requested editor is not a known alias. This is a security risk as it could allow arbitrary command execution if the API endpoint is exposed or triggered via a cross-site request (e.g., if the server lacks CSRF protection). It is safer to restrict the allowed editors to the knownEditors whitelist.
return editorCandidate{}, "", fmt.Errorf("unsupported editor: %s", requested)| func withinDir(path, dir string) bool { | ||
| abs := filepath.Clean(path) | ||
| base := filepath.Clean(dir) + string(filepath.Separator) | ||
| // If dir is exactly the file (e.g. single-file agent), treat as within. | ||
| if abs == filepath.Clean(dir) { | ||
| return true | ||
| } | ||
| return strings.HasPrefix(abs, base) | ||
| } |
There was a problem hiding this comment.
The withinDir implementation relies on strings.HasPrefix, which is case-sensitive. This can cause issues on case-insensitive file systems (like Windows or macOS) if the path casing differs. Using filepath.Rel is a more robust way to verify that a path is contained within a directory.
func withinDir(path, dir string) bool {
rel, err := filepath.Rel(dir, path)
return err == nil && !strings.HasPrefix(rel, "..")
}| export function diffLines(a: string[], b: string[]): DiffOp[] { | ||
| const n = a.length; | ||
| const m = b.length; | ||
| const dp: Int32Array[] = Array.from({ length: n + 1 }, () => new Int32Array(m + 1)); |
There was a problem hiding this comment.
The diff algorithm uses a dynamic programming table with
| if (taMax === 0 || pvMax === 0) return null; | ||
| const lh = getTextareaLineHeight(ta); | ||
| const leftCache = leftAnchorsRef.current; | ||
| const nodes = Array.from(pv.querySelectorAll<HTMLElement>('[data-slug]')); |
There was a problem hiding this comment.
Executing querySelectorAll on every scroll frame (via requestAnimationFrame) can be performance-intensive, especially for large documents with many headings. It is more efficient to cache the list of elements with data-slug attributes and only refresh the cache when the markdown content changes.
- pickEditor now rejects unknown editor aliases instead of executing arbitrary binaries from PATH (security) - withinDir uses filepath.Rel instead of strings.HasPrefix for correctness on case-insensitive filesystems
1 participant
SKILL.mdfiles directly in the browser with a two-pane Markdown editor, structured frontmatter fields, and a diff review step before saving feat(ui): add markdown editing #128$EDITOR)