feat: agent templates with built-in roles, import/export, and prompt overrides#54
Merged
Conversation
…overrides Add a Templates page with 9 built-in role templates (Frontend / Backend Engineer, Security, Code Reviewer, Tester, Refactor Specialist, Docs Writer, DevOps, Product Designer), each pre-wired with a curated set of marketplace skills and a production-grade system prompt grounded in Cursor / Cline / Roo Code / Anthropic Agent Skills patterns. Use a template → pick a project → agent is created and auto-started. Built-ins are editable; edits are stored as overrides keyed by built-in ID so the original keeps its slot and can be reset to defaults. Templates can be exported as .json and re-imported, single or in batches, for sharing across machines. Other quality-of-life changes that landed alongside: - Agents list sorts by created-at desc by default; createdAt is set on creation and backfilled from lastActivity for legacy agents. - "Save as template" action on each agent card. - Clickable "+" badges on missing skills install via the existing TerminalDialog flow and refresh the badge live on close. - Changelog entry under v1.2.8.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces comprehensive agent template management to Dorothy, featuring built-in role-based templates (frontend, backend, security, etc.), persistent user template creation/editing, template import/export, and a full UI for browsing, instantiating, and managing templates alongside agent records. Agents gain a ChangesAgent Template Management System
Sequence DiagramsequenceDiagram
participant User
participant TemplatesPage as Templates Page
participant Hook as useElectronTemplates
participant Preload as Electron Preload
participant Main as Main Process
participant Store as templates.json
User->>TemplatesPage: Open Templates page
TemplatesPage->>Hook: refresh()
Hook->>Preload: template.list()
Preload->>Main: ipcRenderer.invoke('template:list')
Main->>Store: loadStore()
Main->>Main: Merge built-ins + overrides + user templates
Main-->>Preload: [templates array]
Preload-->>Hook: Promise resolved
Hook->>TemplatesPage: builtinTemplates + userTemplates
TemplatesPage->>TemplatesPage: Render template cards
User->>TemplatesPage: Click "Create template"
TemplatesPage->>TemplatesPage: Show TemplateFormDialog
User->>TemplatesPage: Fill form + submit
TemplatesPage->>Hook: create(input)
Hook->>Preload: template.create(input)
Preload->>Main: ipcRenderer.invoke('template:create', input)
Main->>Store: buildFromInput(input)
Main->>Store: store.user.push(newTemplate)
Main->>Store: saveStore(store)
Main-->>Preload: {success, template}
Preload-->>Hook: Promise resolved
Hook->>Hook: refresh()
Hook-->>TemplatesPage: Updated templates
TemplatesPage->>TemplatesPage: Close dialog + update UI
User->>TemplatesPage: Click "Use this template"
TemplatesPage->>TemplatesPage: Show InstantiateDialog
User->>TemplatesPage: Select project + agent name
TemplatesPage->>TemplatesPage: createAgent(template fields)
TemplatesPage->>TemplatesPage: Navigate to /agents
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Add the keyboard-shortcut feature from #51 (already merged) to the v1.2.8 What's New entry so users of this release see it alongside the templates work.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Sidebar.tsx (1)
38-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTemplates and Skills share the same
Sparklesicon — indistinguishable in collapsed sidebarBoth
/templates(Line 38) and/skills(Line 42) useSparkles. In the collapsed sidebar (sidebarCollapsed = true), labels are hidden and only the icon is shown, so both entries render identically. Users cannot tell them apart without expanding the sidebar.Consider using a distinct icon for Templates — lucide-react ships several good candidates:
LayoutTemplate,BookMarked,FileStack, orBookmark.🎨 Proposed fix (example using `LayoutTemplate`)
import { LayoutDashboard, FolderKanban, Sparkles, + LayoutTemplate, Puzzle, ... } from 'lucide-react'; const navItems = [ { href: '/', icon: LayoutDashboard, label: 'Dashboard', shortcut: '1' }, { href: '/agents', icon: Bot, label: 'Agents', shortcut: '2' }, - { href: '/templates', icon: Sparkles, label: 'Templates', shortcut: 'T' }, + { href: '/templates', icon: LayoutTemplate, label: 'Templates', shortcut: 'T' }, ... { href: '/skills', icon: Sparkles, label: 'Skills', shortcut: '6' },🤖 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 `@src/components/Sidebar.tsx` around lines 38 - 42, The Templates menu item currently reuses the Sparkles icon (the entry with href '/templates' using Sparkles) which duplicates the icon used by the Skills entry; update the Templates entry to use a distinct icon such as LayoutTemplate (or BookMarked/FileStack/Bookmark) instead of Sparkles so the collapsed sidebar (sidebarCollapsed = true) shows a unique icon; modify the icon reference for the '/templates' menu object to LayoutTemplate and ensure the import for LayoutTemplate from lucide-react is added/updated.
🧹 Nitpick comments (1)
src/components/Templates/TemplateFormDialog.tsx (1)
80-82: ⚡ Quick winModal is missing ARIA dialog semantics
The overlay
<div>lacksrole="dialog",aria-modal="true", andaria-labelledbypointing to the<h2>title. Without these, screen readers will not announce or frame this as a dialog and may not restrict virtual cursor movement to the dialog content.♿ Proposed fix
- <div className="fixed inset-0 bg-background/80 backdrop-blur-sm z-50 flex items-center justify-center p-4"> - <div ref={dialogRef} className="bg-card border border-border w-full max-w-2xl max-h-[90vh] flex flex-col"> - <div className="px-5 py-4 border-b border-border flex items-center justify-between gap-3"> - <h2 className="font-semibold text-foreground"> + <div className="fixed inset-0 bg-background/80 backdrop-blur-sm z-50 flex items-center justify-center p-4"> + <div + ref={dialogRef} + role="dialog" + aria-modal="true" + aria-labelledby="template-dialog-title" + className="bg-card border border-border w-full max-w-2xl max-h-[90vh] flex flex-col" + > + <div className="px-5 py-4 border-b border-border flex items-center justify-between gap-3"> + <h2 id="template-dialog-title" className="font-semibold text-foreground">🤖 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 `@src/components/Templates/TemplateFormDialog.tsx` around lines 80 - 82, The modal overlay returned by TemplateFormDialog is missing ARIA dialog semantics; update the outer dialog container (the element using dialogRef in TemplateFormDialog) to include role="dialog", aria-modal="true", and aria-labelledby pointing to the title, and add a stable id on the <h2> title (e.g., templateDialogTitle or similar) so aria-labelledby can reference it; ensure the id is unique within the component and applied to the <h2> used as the dialog header.
🤖 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 `@electron/handlers/template-handlers.ts`:
- Around line 144-152: The 'template:get' IPC handler
(ipcMain.handle('template:get')) lacks a try-catch like the other template
handlers; wrap the existing body (calls to loadStore(), BUILTIN_TEMPLATES.find,
applyOverride, and store.user.find) in a try block and add a catch that logs the
error (use the same logger used elsewhere, e.g., processLogger.error) and
returns a safe response { template: null } so renderer receives a consistent
value on failure; ensure you reference the same symbols (loadStore,
BUILTIN_TEMPLATES, applyOverride) when implementing the change.
In `@src/app/templates/page.tsx`:
- Around line 71-83: handleDelete and handleDuplicate currently call
remove(template.id) and duplicate(template.id) without handling failures; wrap
those calls in try/catch like the handleSaveAsTemplate pattern (referencing
handleSaveAsTemplate) so errors are surfaced to the user and the UI updates on
success: call await remove/duplicate inside try, on success trigger the same
refresh/revalidate action used elsewhere (e.g., router.refresh or
revalidateTemplates) and show a success notification, and on catch log the error
and show an error notification/alert with the error message so failures are not
silently swallowed; apply the same error-handling pattern to handleReset if it
should surface failures too.
In `@src/components/Templates/ImportDialog.tsx`:
- Around line 150-165: The preview/parsed logic allows empty or non-importable
payloads; update the parsing and button-guard so parsed (and preview.count) is
set only when json.templates is an array with at least one entry that has a
non-empty displayName (e.g., compute an importableCount by filtering
json.templates for t?.displayName truthy), populate preview.names from those
importable entries, and disable the Import button unless importableCount > 0;
also update the UI that reads preview.count and preview.names (the JSX block
that renders the preview and the Import button enable condition) to use
importableCount instead of raw json.templates length so clicking Import cannot
submit zero-item payloads.
- Around line 84-87: The handleFile function can reject because file.text() may
throw and callers don't await it; wrap the file.text() call and subsequent
tryParse call in a try/catch inside handleFile (keep the async signature), only
call tryParse(content) on success, and in the catch log or surface the error
(e.g., call an error handler or processLogger/console.error) so the rejection is
handled instead of becoming unhandled; reference: function handleFile,
file.text(), and tryParse (and the external onChange caller) so you update the
function to catch and handle file.text() failures.
- Around line 102-110: The modal markup in ImportDialog is missing accessibility
attributes and a focus trap; update the container element (the div referenced by
dialogRef inside the ImportDialog component) to include role="dialog",
aria-modal="true" and aria-labelledby pointing to the heading (assign an id to
the h2), ensure the close button uses the provided onClose, and implement a
focus trap (either replace the wrapper with a native <dialog> or integrate a
small utility like focus-trap-react) so keyboard focus is trapped inside the
dialog, set initial focus on a meaningful element inside the dialog, restore
focus to the opener on close, and handle Escape key to call onClose.
In `@src/components/Templates/InstantiateDialog.tsx`:
- Around line 97-124: The modal lacks ARIA semantics and the "Agent name" label
isn't associated with its input; update the dialog container (the div using
dialogRef) to include role="dialog", aria-modal="true" and aria-labelledby
pointing to a new id on the title (the h2 showing "Use template:
{template.displayName}"), then add a matching id to that h2 and change the
"Agent name" label to use htmlFor with a unique id on the input (the input bound
to value={name} and onChange={e => setName(e.target.value)}), and ensure any
focus-management that uses dialogRef moves focus into the dialog after these
attributes are added.
In `@src/components/Templates/TemplateCard.tsx`:
- Around line 102-146: The footer action buttons in TemplateCard (the
conditional buttons using onDuplicate, onExport, onEdit, onReset, onDelete) rely
only on title attributes which are not a reliable accessible name for screen
readers; update each button to include an explicit aria-label (e.g.,
aria-label="Duplicate", "Export as JSON", "Edit", "Reset to default", "Delete")
matching the title/intent so assistive tech receives a consistent label when
rendering the Copy, Download, Pencil, RotateCcw and Trash2 icon-only buttons.
In `@src/components/Templates/TemplateFormDialog.tsx`:
- Around line 52-76: handleSubmit currently awaits onSubmit(...) without
catching rejections, so a thrown error will skip resetting state and leave
submitting=true; wrap the await call in try/catch/finally: call
setSubmitting(true) before the try, await the result inside try, on success call
onClose and on failure setError(result.error ?? 'Failed to save template'), in
catch setError(error?.message ?? String(error)), and in finally call
setSubmitting(false) to ensure the button is re-enabled; update references
inside handleSubmit to use onSubmit, setError, setSubmitting, and onClose
accordingly.
In `@src/types/electron.d.ts`:
- Line 847: The declaration for template.get is missing the optional error field
which other API methods include; update the return type of get to include
error?: string (i.e., change Promise<{ template: AgentTemplate | null }> to
Promise<{ template: AgentTemplate | null; error?: string }>) so callers can
distinguish not-found vs backend failure; adjust the type in
src/types/electron.d.ts for the get method of the template namespace (keeping
the existing id: string parameter and Promise wrapper).
---
Outside diff comments:
In `@src/components/Sidebar.tsx`:
- Around line 38-42: The Templates menu item currently reuses the Sparkles icon
(the entry with href '/templates' using Sparkles) which duplicates the icon used
by the Skills entry; update the Templates entry to use a distinct icon such as
LayoutTemplate (or BookMarked/FileStack/Bookmark) instead of Sparkles so the
collapsed sidebar (sidebarCollapsed = true) shows a unique icon; modify the icon
reference for the '/templates' menu object to LayoutTemplate and ensure the
import for LayoutTemplate from lucide-react is added/updated.
---
Nitpick comments:
In `@src/components/Templates/TemplateFormDialog.tsx`:
- Around line 80-82: The modal overlay returned by TemplateFormDialog is missing
ARIA dialog semantics; update the outer dialog container (the element using
dialogRef in TemplateFormDialog) to include role="dialog", aria-modal="true",
and aria-labelledby pointing to the title, and add a stable id on the <h2> title
(e.g., templateDialogTitle or similar) so aria-labelledby can reference it;
ensure the id is unique within the component and applied to the <h2> used as the
dialog header.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 468c4290-879b-4488-9f48-3e77eae0d0d6
📒 Files selected for processing (20)
electron/constants/builtin-templates.tselectron/core/agent-manager.tselectron/handlers/ipc-handlers.tselectron/handlers/template-handlers.tselectron/main.tselectron/preload.tselectron/types/index.tselectron/types/template.tssrc/app/agents/page.tsxsrc/app/templates/page.tsxsrc/components/AgentList/AgentManagementCard.tsxsrc/components/Sidebar.tsxsrc/components/Templates/ImportDialog.tsxsrc/components/Templates/InstantiateDialog.tsxsrc/components/Templates/TemplateCard.tsxsrc/components/Templates/TemplateFormDialog.tsxsrc/data/changelog.tssrc/hooks/useAgentFiltering.tssrc/hooks/useElectronTemplates.tssrc/types/electron.d.ts
Comment on lines
+144
to
+152
| ipcMain.handle('template:get', async (_event, id: string) => { | ||
| const store = loadStore(); | ||
| const builtin = BUILTIN_TEMPLATES.find(t => t.id === id); | ||
| if (builtin) { | ||
| return { template: applyOverride(builtin, store.overrides[id]) }; | ||
| } | ||
| const userTemplate = store.user.find(t => t.id === id); | ||
| return { template: userTemplate ?? null }; | ||
| }); |
There was a problem hiding this comment.
template:get is the only handler without a try-catch
Every other handler (template:create, template:update, template:delete, template:export, template:import, template:duplicate) wraps its body in try-catch. template:get at lines 144-152 does not. While loadStore() swallows its own errors, an unexpected throw here will surface as an unhandled IPC rejection in the renderer instead of a clean { template: null } response.
🛡️ Proposed fix
- ipcMain.handle('template:get', async (_event, id: string) => {
- const store = loadStore();
- const builtin = BUILTIN_TEMPLATES.find(t => t.id === id);
- if (builtin) {
- return { template: applyOverride(builtin, store.overrides[id]) };
- }
- const userTemplate = store.user.find(t => t.id === id);
- return { template: userTemplate ?? null };
- });
+ ipcMain.handle('template:get', async (_event, id: string) => {
+ try {
+ const store = loadStore();
+ const builtin = BUILTIN_TEMPLATES.find(t => t.id === id);
+ if (builtin) {
+ return { template: applyOverride(builtin, store.overrides[id]) };
+ }
+ const userTemplate = store.user.find(t => t.id === id);
+ return { template: userTemplate ?? null };
+ } catch (err) {
+ console.error('template:get error:', err);
+ return { template: null, error: err instanceof Error ? err.message : 'Failed to get template' };
+ }
+ });🤖 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 `@electron/handlers/template-handlers.ts` around lines 144 - 152, The
'template:get' IPC handler (ipcMain.handle('template:get')) lacks a try-catch
like the other template handlers; wrap the existing body (calls to loadStore(),
BUILTIN_TEMPLATES.find, applyOverride, and store.user.find) in a try block and
add a catch that logs the error (use the same logger used elsewhere, e.g.,
processLogger.error) and returns a safe response { template: null } so renderer
receives a consistent value on failure; ensure you reference the same symbols
(loadStore, BUILTIN_TEMPLATES, applyOverride) when implementing the change.
Comment on lines
+71
to
+83
| async function handleDelete(template: AgentTemplate) { | ||
| if (!confirm(`Delete template "${template.displayName}"? This cannot be undone.`)) return; | ||
| await remove(template.id); | ||
| } | ||
|
|
||
| async function handleReset(template: AgentTemplate) { | ||
| if (!confirm(`Reset "${template.displayName}" to its default settings?`)) return; | ||
| await remove(template.id); | ||
| } | ||
|
|
||
| async function handleDuplicate(template: AgentTemplate) { | ||
| await duplicate(template.id); | ||
| } |
There was a problem hiding this comment.
handleDelete and handleDuplicate swallow errors silently.
Both await remove(template.id) and await duplicate(template.id) discard their return values. If either fails, the user gets no feedback and the UI looks unchanged.
handleDelete is particularly important to surface: a user might believe the template was deleted when it wasn't.
🛡️ Proposed fix (consistent with the `handleSaveAsTemplate` pattern in `agents/page.tsx`)
async function handleDelete(template: AgentTemplate) {
if (!confirm(`Delete template "${template.displayName}"? This cannot be undone.`)) return;
- await remove(template.id);
+ const result = await remove(template.id);
+ if (!result.success) alert(`Could not delete template: ${result.error ?? 'unknown error'}`);
}
async function handleDuplicate(template: AgentTemplate) {
- await duplicate(template.id);
+ const result = await duplicate(template.id);
+ if (!result.success) alert(`Could not duplicate template: ${result.error ?? 'unknown error'}`);
}📝 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
| async function handleDelete(template: AgentTemplate) { | |
| if (!confirm(`Delete template "${template.displayName}"? This cannot be undone.`)) return; | |
| await remove(template.id); | |
| } | |
| async function handleReset(template: AgentTemplate) { | |
| if (!confirm(`Reset "${template.displayName}" to its default settings?`)) return; | |
| await remove(template.id); | |
| } | |
| async function handleDuplicate(template: AgentTemplate) { | |
| await duplicate(template.id); | |
| } | |
| async function handleDelete(template: AgentTemplate) { | |
| if (!confirm(`Delete template "${template.displayName}"? This cannot be undone.`)) return; | |
| const result = await remove(template.id); | |
| if (!result.success) alert(`Could not delete template: ${result.error ?? 'unknown error'}`); | |
| } | |
| async function handleReset(template: AgentTemplate) { | |
| if (!confirm(`Reset "${template.displayName}" to its default settings?`)) return; | |
| await remove(template.id); | |
| } | |
| async function handleDuplicate(template: AgentTemplate) { | |
| const result = await duplicate(template.id); | |
| if (!result.success) alert(`Could not duplicate template: ${result.error ?? 'unknown error'}`); | |
| } |
🤖 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 `@src/app/templates/page.tsx` around lines 71 - 83, handleDelete and
handleDuplicate currently call remove(template.id) and duplicate(template.id)
without handling failures; wrap those calls in try/catch like the
handleSaveAsTemplate pattern (referencing handleSaveAsTemplate) so errors are
surfaced to the user and the UI updates on success: call await remove/duplicate
inside try, on success trigger the same refresh/revalidate action used elsewhere
(e.g., router.refresh or revalidateTemplates) and show a success notification,
and on catch log the error and show an error notification/alert with the error
message so failures are not silently swallowed; apply the same error-handling
pattern to handleReset if it should surface failures too.
Comment on lines
+84
to
+87
| async function handleFile(file: File) { | ||
| const content = await file.text(); | ||
| tryParse(content); | ||
| } |
There was a problem hiding this comment.
handleFile can produce an unhandled promise rejection.
file.text() can fail (e.g., file revoked, memory pressure) and the calling onChange handler doesn't await it, so any rejection goes uncaught and the user sees nothing.
🛡️ Proposed fix
async function handleFile(file: File) {
- const content = await file.text();
- tryParse(content);
+ try {
+ const content = await file.text();
+ tryParse(content);
+ } catch {
+ setParseError('Failed to read file');
+ }
}🤖 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 `@src/components/Templates/ImportDialog.tsx` around lines 84 - 87, The
handleFile function can reject because file.text() may throw and callers don't
await it; wrap the file.text() call and subsequent tryParse call in a try/catch
inside handleFile (keep the async signature), only call tryParse(content) on
success, and in the catch log or surface the error (e.g., call an error handler
or processLogger/console.error) so the rejection is handled instead of becoming
unhandled; reference: function handleFile, file.text(), and tryParse (and the
external onChange caller) so you update the function to catch and handle
file.text() failures.
Comment on lines
+102
to
+110
| return ( | ||
| <div className="fixed inset-0 bg-background/80 backdrop-blur-sm z-50 flex items-center justify-center p-4"> | ||
| <div ref={dialogRef} className="bg-card border border-border w-full max-w-xl max-h-[90vh] flex flex-col"> | ||
| <div className="px-5 py-4 border-b border-border flex items-center justify-between gap-3"> | ||
| <h2 className="font-semibold text-foreground">Import templates</h2> | ||
| <button onClick={onClose} className="p-1 text-muted-foreground hover:text-foreground" title="Close"> | ||
| <X className="w-4 h-4" /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Dialog is missing ARIA attributes and a focus trap.
Without role="dialog", aria-modal="true", and aria-labelledby, screen readers won't announce this as a modal. More practically, there is no focus trap — keyboard-only users can Tab past the dialog and activate controls behind the backdrop.
♿ Proposed fix
- <div ref={dialogRef} className="bg-card border border-border w-full max-w-xl max-h-[90vh] flex flex-col">
- <div className="px-5 py-4 border-b border-border flex items-center justify-between gap-3">
- <h2 className="font-semibold text-foreground">Import templates</h2>
+ <div
+ ref={dialogRef}
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby="import-dialog-title"
+ className="bg-card border border-border w-full max-w-xl max-h-[90vh] flex flex-col"
+ >
+ <div className="px-5 py-4 border-b border-border flex items-center justify-between gap-3">
+ <h2 id="import-dialog-title" className="font-semibold text-foreground">Import templates</h2>For focus trapping, consider using the native <dialog> element or a small utility (e.g., focus-trap-react) to keep focus inside the modal.
🤖 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 `@src/components/Templates/ImportDialog.tsx` around lines 102 - 110, The modal
markup in ImportDialog is missing accessibility attributes and a focus trap;
update the container element (the div referenced by dialogRef inside the
ImportDialog component) to include role="dialog", aria-modal="true" and
aria-labelledby pointing to the heading (assign an id to the h2), ensure the
close button uses the provided onClose, and implement a focus trap (either
replace the wrapper with a native <dialog> or integrate a small utility like
focus-trap-react) so keyboard focus is trapped inside the dialog, set initial
focus on a meaningful element inside the dialog, restore focus to the opener on
close, and handle Escape key to call onClose.
Comment on lines
+150
to
+165
| {preview && ( | ||
| <div className="border border-border bg-secondary/30 px-3 py-2"> | ||
| <p className="text-xs font-medium text-foreground mb-1.5 flex items-center gap-1.5"> | ||
| <FileText className="w-3 h-3" /> | ||
| {preview.count} template{preview.count === 1 ? '' : 's'} ready to import | ||
| </p> | ||
| <ul className="text-xs text-muted-foreground list-disc pl-5 space-y-0.5"> | ||
| {preview.names.slice(0, 8).map((n, i) => <li key={`${n}-${i}`}>{n}</li>)} | ||
| {preview.names.length > 8 && <li>…and {preview.names.length - 8} more</li>} | ||
| </ul> | ||
| </div> | ||
| )} | ||
|
|
||
| {submitError && ( | ||
| <p className="text-xs text-destructive bg-destructive/10 border border-destructive/30 px-2 py-1.5">{submitError}</p> | ||
| )} |
There was a problem hiding this comment.
Import button enabled when zero templates would actually be imported.
Two scenarios slip through client-side validation with parsed set but nothing importable:
templates: []— an empty array passesArray.isArray(json.templates), soparsedis set andpreview.count = 0.- All template entries lack
displayName— same result.
In both cases the Import button is active. Clicking it sends the payload to the backend, which returns { success: false } (no error field), surfacing the unhelpful message "Import failed".
🛡️ Proposed fix
Tighten the client validation and the button guard:
- if (!Array.isArray(json.templates)) {
+ if (!Array.isArray(json.templates) || json.templates.length === 0) {
setParseError('Missing or invalid "templates" array'); <button
onClick={handleSubmit}
- disabled={!parsed || submitting}
+ disabled={!parsed || !preview || preview.count === 0 || submitting}Also applies to: 176-179
🤖 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 `@src/components/Templates/ImportDialog.tsx` around lines 150 - 165, The
preview/parsed logic allows empty or non-importable payloads; update the parsing
and button-guard so parsed (and preview.count) is set only when json.templates
is an array with at least one entry that has a non-empty displayName (e.g.,
compute an importableCount by filtering json.templates for t?.displayName
truthy), populate preview.names from those importable entries, and disable the
Import button unless importableCount > 0; also update the UI that reads
preview.count and preview.names (the JSX block that renders the preview and the
Import button enable condition) to use importableCount instead of raw
json.templates length so clicking Import cannot submit zero-item payloads.
Comment on lines
+97
to
+124
| return ( | ||
| <div className="fixed inset-0 bg-background/80 backdrop-blur-sm z-50 flex items-center justify-center p-4"> | ||
| <div ref={dialogRef} className="bg-card border border-border w-full max-w-lg max-h-[90vh] flex flex-col"> | ||
| <div className="px-5 py-4 border-b border-border flex items-center justify-between gap-3"> | ||
| <div className="flex items-center gap-3 min-w-0"> | ||
| <span className="text-2xl shrink-0">{template.icon}</span> | ||
| <div className="min-w-0"> | ||
| <h2 className="font-semibold text-foreground truncate">Use template: {template.displayName}</h2> | ||
| <p className="text-xs text-muted-foreground truncate">Pick a project, name your agent, and we'll set the rest up.</p> | ||
| </div> | ||
| </div> | ||
| <button onClick={onClose} className="p-1 text-muted-foreground hover:text-foreground" title="Close"> | ||
| <X className="w-4 h-4" /> | ||
| </button> | ||
| </div> | ||
|
|
||
| <div className="flex-1 overflow-y-auto px-5 py-4 space-y-4"> | ||
| <div> | ||
| <label className="block text-xs font-medium text-foreground mb-1.5">Agent name</label> | ||
| <input | ||
| type="text" | ||
| value={name} | ||
| onChange={e => setName(e.target.value)} | ||
| maxLength={40} | ||
| className="w-full px-2 py-1.5 bg-secondary border border-border text-sm text-foreground placeholder:text-muted-foreground outline-none focus:border-primary/40" | ||
| placeholder={template.displayName} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Custom modal missing required ARIA semantics and <label> associations.
The dialog <div> has no role="dialog", aria-modal, or aria-labelledby, so screen readers cannot identify it as a dialog or announce its title on open. The "Agent name" <label> is also not programmatically associated with its <input> (no htmlFor/id pair), violating WCAG 4.1.2.
Accessible modals require role="dialog", aria-modal="true", and aria-labelledby, and when the dialog is correctly labeled and focus is moved to an element inside the dialog, screen readers should announce the dialog's accessible role, name, and the focused element.
♿ Proposed fix (low-hanging fruit — ARIA attributes + label association)
+ const titleId = 'instantiate-dialog-title';
+
return (
<div className="fixed inset-0 bg-background/80 backdrop-blur-sm z-50 flex items-center justify-center p-4">
- <div ref={dialogRef} className="bg-card border border-border w-full max-w-lg max-h-[90vh] flex flex-col">
+ <div
+ ref={dialogRef}
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby={titleId}
+ className="bg-card border border-border w-full max-w-lg max-h-[90vh] flex flex-col"
+ >
<div className="px-5 py-4 border-b border-border flex items-center justify-between gap-3">
<div className="flex items-center gap-3 min-w-0">
<span className="text-2xl shrink-0">{template.icon}</span>
<div className="min-w-0">
- <h2 className="font-semibold text-foreground truncate">Use template: {template.displayName}</h2>
+ <h2 id={titleId} className="font-semibold text-foreground truncate">Use template: {template.displayName}</h2>Then associate the "Agent name" label:
- <label className="block text-xs font-medium text-foreground mb-1.5">Agent name</label>
+ <label htmlFor="agent-name-input" className="block text-xs font-medium text-foreground mb-1.5">Agent name</label>
<input
+ id="agent-name-input"
+ autoFocus
type="text"
value={name}🤖 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 `@src/components/Templates/InstantiateDialog.tsx` around lines 97 - 124, The
modal lacks ARIA semantics and the "Agent name" label isn't associated with its
input; update the dialog container (the div using dialogRef) to include
role="dialog", aria-modal="true" and aria-labelledby pointing to a new id on the
title (the h2 showing "Use template: {template.displayName}"), then add a
matching id to that h2 and change the "Agent name" label to use htmlFor with a
unique id on the input (the input bound to value={name} and onChange={e =>
setName(e.target.value)}), and ensure any focus-management that uses dialogRef
moves focus into the dialog after these attributes are added.
Comment on lines
+102
to
+146
| {onDuplicate && ( | ||
| <button | ||
| onClick={onDuplicate} | ||
| className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors" | ||
| title="Duplicate" | ||
| > | ||
| <Copy className="w-3.5 h-3.5" /> | ||
| </button> | ||
| )} | ||
| {onExport && ( | ||
| <button | ||
| onClick={onExport} | ||
| className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors" | ||
| title="Export as JSON" | ||
| > | ||
| <Download className="w-3.5 h-3.5" /> | ||
| </button> | ||
| )} | ||
| {onEdit && ( | ||
| <button | ||
| onClick={onEdit} | ||
| className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors" | ||
| title="Edit" | ||
| > | ||
| <Pencil className="w-3.5 h-3.5" /> | ||
| </button> | ||
| )} | ||
| {onReset && ( | ||
| <button | ||
| onClick={onReset} | ||
| className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors" | ||
| title="Reset to default" | ||
| > | ||
| <RotateCcw className="w-3.5 h-3.5" /> | ||
| </button> | ||
| )} | ||
| {onDelete && ( | ||
| <button | ||
| onClick={onDelete} | ||
| className="p-1.5 text-muted-foreground hover:text-destructive hover:bg-destructive/10 transition-colors" | ||
| title="Delete" | ||
| > | ||
| <Trash2 className="w-3.5 h-3.5" /> | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
Icon-only footer buttons lack accessible names.
The Duplicate, Export, Edit, Reset, and Delete buttons expose only title for labeling, but screen readers inconsistently expose title as the accessible name. Each button needs an explicit aria-label that matches or describes the action.
♿ Proposed fix
- {onDuplicate && (
- <button
- onClick={onDuplicate}
- className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors"
- title="Duplicate"
- >
+ {onDuplicate && (
+ <button
+ onClick={onDuplicate}
+ type="button"
+ className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors"
+ title="Duplicate"
+ aria-label="Duplicate template"
+ >
<Copy className="w-3.5 h-3.5" />
</button>
)}
{onExport && (
<button
onClick={onExport}
+ type="button"
className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors"
title="Export as JSON"
+ aria-label="Export template as JSON"
>
<Download className="w-3.5 h-3.5" />
</button>
)}
{onEdit && (
<button
onClick={onEdit}
+ type="button"
className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors"
title="Edit"
+ aria-label="Edit template"
>
<Pencil className="w-3.5 h-3.5" />
</button>
)}
{onReset && (
<button
onClick={onReset}
+ type="button"
className="p-1.5 text-muted-foreground hover:text-foreground hover:bg-secondary transition-colors"
title="Reset to default"
+ aria-label="Reset template to default"
>
<RotateCcw className="w-3.5 h-3.5" />
</button>
)}
{onDelete && (
<button
onClick={onDelete}
+ type="button"
className="p-1.5 text-muted-foreground hover:text-destructive hover:bg-destructive/10 transition-colors"
title="Delete"
+ aria-label="Delete template"
>
<Trash2 className="w-3.5 h-3.5" />
</button>
)}🤖 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 `@src/components/Templates/TemplateCard.tsx` around lines 102 - 146, The footer
action buttons in TemplateCard (the conditional buttons using onDuplicate,
onExport, onEdit, onReset, onDelete) rely only on title attributes which are not
a reliable accessible name for screen readers; update each button to include an
explicit aria-label (e.g., aria-label="Duplicate", "Export as JSON", "Edit",
"Reset to default", "Delete") matching the title/intent so assistive tech
receives a consistent label when rendering the Copy, Download, Pencil, RotateCcw
and Trash2 icon-only buttons.
| // Agent templates | ||
| template?: { | ||
| list: () => Promise<{ templates: AgentTemplate[]; error?: string }>; | ||
| get: (id: string) => Promise<{ template: AgentTemplate | null }>; |
There was a problem hiding this comment.
template.get response type is missing error?, inconsistent with every other API method.
All other template.* (and sibling namespace) methods include error?: string in their return shape. Without it, callers cannot distinguish "not found" from a backend failure — both surface as { template: null }.
🔧 Proposed fix
- get: (id: string) => Promise<{ template: AgentTemplate | null }>;
+ get: (id: string) => Promise<{ template: AgentTemplate | null; error?: string }>;📝 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
| get: (id: string) => Promise<{ template: AgentTemplate | null }>; | |
| get: (id: string) => Promise<{ template: AgentTemplate | null; error?: string }>; |
🤖 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 `@src/types/electron.d.ts` at line 847, The declaration for template.get is
missing the optional error field which other API methods include; update the
return type of get to include error?: string (i.e., change Promise<{ template:
AgentTemplate | null }> to Promise<{ template: AgentTemplate | null; error?:
string }>) so callers can distinguish not-found vs backend failure; adjust the
type in src/types/electron.d.ts for the get method of the template namespace
(keeping the existing id: string parameter and Promise wrapper).
Wrap the onSubmit/onImport await in try/finally so a thrown IPC error or rejected promise can't strand the submit button in its disabled spinner state. The user can now read the error and retry instead of having to close and reopen the dialog.
3 tasks
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
Adds a Templates page that lets non-technical users spin up a fully-configured agent in one click — pick a role, point at a project, done.
~/.dorothy/templates.jsonunderoverrides[builtinId]; built-ins keep their slot, gain a "Customized" badge and a one-click reset..json(kind: "dorothy.agent-template",version: 1) — single or batch, paste-or-upload UI for import.TerminalDialoginstall flow used on the Skills page; refresh on close so the badge flips to installed.Other QoL fixes that landed alongside:
createdAtdesc by default — newly instantiated agents appear at the top.createdAtis set on creation and backfilled fromlastActivityfor legacy agents.Changelog
Bumped to v1.2.8 in
src/data/changelog.ts. The What's New entry also includes the Ctrl+Tab / Ctrl+Shift+Tab dashboard cycling feature from #51 (already merged) so users on this release see both bundled together.Test plan
/agents, status flips to running, and the saved prompt shows in its terminal..jsondownloads. Open it, confirmkind,version, and one template entry.TerminalDialogopens → install completes → close → chip flips to grey/installed without manual reload./agents, click the bookmark+ icon on any agent → name it → confirm it appears under Your templates./whats-new→ confirm v1.2.8 entry at the top with the bullet list (including the Ctrl+Tab line); sidebar dot clears.