Encore: 2 Bundled Encore Features and Settings UI Updates#419
Open
needmorecowbell wants to merge 19 commits intoRunMaestro:mainfrom
Open
Encore: 2 Bundled Encore Features and Settings UI Updates#419needmorecowbell wants to merge 19 commits intoRunMaestro:mainfrom
needmorecowbell wants to merge 19 commits intoRunMaestro:mainfrom
Conversation
…lity Catalog all architectural extension points in Maestro that a plugin system could leverage: ProcessManager events, process listener patterns, 35+ preload API namespaces, IPC handler registration, modal priority system, right panel tab system, marketplace manifest (prior art), stats API, and process API. Identifies 10 gaps requiring new infrastructure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verified all 4 required APIs exist in preload/process.ts: getActiveProcesses(), onUsage(), onData(), onToolExecution(). Documented complete data model including ActiveProcess, UsageStats, ToolExecutionEvent, and StatsAggregation types. Key findings: - Plugin can work as purely read-only (no write APIs needed) - Recommended floating modal for v1, Right Panel tab for v2 - Feasibility: Trivial — all APIs exist, only shared plugin infrastructure needed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Analyze event subscriptions, tool-execution data model per agent, risky operation detection patterns, and storage requirements. Rating: Moderate — all event APIs exist but needs plugin-scoped storage API (Gap RunMaestro#8) and main-process component for SQLite. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…bility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…bility Analyzed existing Fastify web server architecture against external integration needs. Found most high-value scenarios (dashboards, log sync, CI triggers) work via core web server enhancements rather than plugin infrastructure. Identified 4 core gaps (tool execution broadcasts, stats endpoints, session creation, auto-run triggers) and 1 plugin-specific gap (route registration). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Synthesizes findings from all 6 concept assessments into a single source-of-truth document covering feasibility ratings, 16 identified gaps ranked by difficulty, v1/v2 scope recommendations, dependency graph, reusable infrastructure inventory, and risk summary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Phase 02) Adds the foundational plugin infrastructure: - PluginManifest types with permissions, UI config, and settings schema - Plugin discovery from userData/plugins/ with manifest validation - PluginManager class with singleton pattern for lifecycle orchestration - IPC handlers (plugins:getAll, enable, disable, getDir, refresh) - Preload bridge (window.maestro.plugins namespace) - 31 tests covering manifest validation and plugin discovery Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add PluginHost class that provides permission-scoped API objects to plugins. Each plugin receives only the API namespaces it declared in its manifest permissions, preventing unauthorized access to Maestro internals. Key components: - PluginAPI types (process, processControl, stats, settings, storage, notifications, maestro) - PluginHost with factory methods that check permissions before exposing APIs - Settings namespaced to plugin:id:key to prevent cross-plugin interference - Storage with path traversal prevention for plugin-scoped file access - Event subscription tracking with automatic cleanup on plugin disable - Integration with PluginManager enable/disable lifecycle - Wired into main process startup after ProcessManager creation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ridge (Phase 04) Adds plugin module loading/activation lifecycle with Sentry error reporting, PluginStorage class with path traversal prevention, PluginIpcBridge for split-architecture plugins, auto-enable for first-party plugins, and preload bridge API. 26 new tests covering activation, storage, and IPC bridge. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create usePluginRegistry hook for renderer-side plugin state management
- Create PluginManager modal with enable/disable, permissions badges, refresh
- Create PluginTabContent component with sandboxed iframe rendering
- Extend RightPanel with dynamic plugin tab support via pluginTabs prop
- Wire Plugin Manager into App.tsx and SettingsModal with modalStore integration
- Add PLUGIN_MANAGER priority (455), pluginManager ModalId to modal system
- Fix RightPanelTab type to accept plugin tab IDs (string & {})
- Fix useAutoRunHandlers to use RightPanelTab type instead of narrow union
- Fix plugin-host.ts StatsDB method name (getAggregatedStats)
- Fix plugin IPC bridge handlers to return Record<string, unknown>
- Add 24 renderer tests (PluginManager, PluginTabContent, usePluginRegistry)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…test mocks Realigned the dependency graph in the feasibility README to reflect the revised phase ordering (middleware deferred to v2, replaced by main-process activation in Phase 04). Added plugin API mocks to test setup for Phase 05 renderer tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Agent Dashboard plugin: writes real-time agent status to status.json with debounced writes, token/cost tracking, tool execution monitoring - Notification Webhook plugin: sends HTTP POST on agent exit/error events with settings-gated behavior and 10s timeout - Wire PluginIpcBridge into main process startup (index.ts) so both PluginHost and registerPluginHandlers receive the bridge instance - 22 automated tests covering both plugins (schema validation, debounce, event subscriptions, settings gating, error handling) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ine (Phase 06b) - Add Plugins tab to SettingsModal with Ctrl+Shift+X shortcut - Implement plugin detail view with README rendering (react-markdown) and settings editor - Fix plugin settings not persisting (pluginManager.setSettingsStore was never called) - Add settings IPC handlers (plugins:settings:get, plugins:settings:set) with validation - Rename agent-dashboard → agent-status-exporter with heartbeat and configurable output path - Fix webhook IPv6 ECONNREFUSED (localhost → 127.0.0.1 for Node http.request) - Enrich webhook payloads with agentType, agentName, and lastOutput rolling buffer - Expose session name in PluginProcessAPI.getActiveProcesses via sessionsStore lookup - Add build:plugins step to copy src/plugins/ → dist/plugins/ (fixes dev bootstrap) - Clean up deprecated agent-dashboard directory on bootstrap - Fix toggle click target in plugin settings (label→div) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…se 06c) Documentation: - Add CLAUDE-PLUGINS.md with full plugin system architecture, API surface, build pipeline, manifest schema, and development guide - Update CLAUDE.md documentation index, architecture tree, and key files table Bug fixes from code review: - Fix tool.name → tool.toolName in agent-status-exporter (tool names were never exported) - Fix Rules of Hooks violation in PluginSettings (useState after conditional return) - Fix onStatsUpdate memory leak (no-op unsubscribe replaced with real listener removal) - Fix number input saving NaN on empty field (guard with isNaN check) - Fix plugin refresh double-activation (deactivate active plugins before re-init) - Fix ipcBridge handler leak (call unregisterAll on plugin deactivation) - Update tests for tool.toolName and getActiveProcesses name field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Aligns with upstream's Encore Features concept — the maintainer's equivalent extension system. All "plugin" terminology is now "encore" internally: file names, types, classes, functions, IPC channels, store keys, UI strings, comments, and documentation. Key changes: - Migrate plugins tab into upstream's Encore tab as flat feature list - Extract DirectorNotesSettings into self-contained component (~380 LOC) - Create shared EncoreFeatureCard toggle card component - Rename all files: plugin-*.ts → encore-*.ts, src/plugins/ → src/encores/ - Rename all types/classes: Plugin* → Encore* throughout - Update IPC channels: plugins:* → encores:*, preload bridge: window.maestro.encores - Update store key prefix: encore:<id>:<key> - Rewrite CLAUDE-ENCORES.md documentation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tarts Previously, first-party encores auto-enabled on startup unless a `userDisabled` flag was set, but enabling never cleared that flag — so encores toggled on then off would stay off forever. Third-party encores had no persistence at all. Now uses a single `encore:<id>:enabled` flag: - All encores start disabled (no auto-enable for first-party) - enableEncore() persists `enabled: true` - disableEncore() persists `enabled: false` - On startup, only encores with `enabled: true` are restored Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a complete "Encores" plugin system: discovery, manifest validation, scoped storage, IPC bridge, host/manager lifecycle, renderer UI and registry, preload IPC, two bundled first‑party encores, extensive tests, docs/research, and build/dev integration for packaging encores. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as "Renderer (UI)"
participant IPC as "Main IPC Handler"
participant Manager as "EncoreManager"
participant Host as "EncoreHost"
participant Module as "Encore Module"
User->>Renderer: Click "Enable" on encore
Renderer->>IPC: invoke 'encores:enable'(encoreId)
IPC->>Manager: enableEncore(encoreId)
Manager->>Host: activateEncore(LoadedEncore)
Host->>Host: resolve entry, require module
Host->>Host: create EncoreContext (permission-gated API)
Host->>Module: call activate(scopedApi)
Module-->>Host: subscribe to events / register IPC handlers
Host-->>Manager: activation success
Manager->>IPC: return success
IPC-->>Renderer: return success
Renderer->>Renderer: refresh registry, show active tab
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Author
|
Tested on Linux -- will also aim to test on other platforms. Did not directly test functionality for remote agents or worktrees, but all existing and new tests pass. |
Greptile SummaryThis PR implements a comprehensive extension system ("Encore Features") for Maestro with manifest-based discovery, permission-scoped APIs, and state persistence. Two bundled first-party encores ship with the release: Agent Status Exporter (real-time agent data to JSON) and Notification Webhook (HTTP notifications on agent events). Major Changes:
Security Highlights:
Code Quality:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start([App Startup]) --> Bootstrap[Bootstrap Bundled Encores]
Bootstrap --> Discover[Discover Encores from userData]
Discover --> Manager[EncoreManager Initialization]
Manager --> CheckEnabled{User Previously<br/>Enabled?}
CheckEnabled -->|Yes| Activate[EncoreHost.activateEncore]
CheckEnabled -->|No| Idle[Encore Remains Disabled]
Activate --> LoadModule[require encore main file]
LoadModule --> CreateAPI[Create Scoped EncoreAPI<br/>Based on Permissions]
CreateAPI --> CallActivate[Call encore.activate with API]
CallActivate --> Active[Encore State: Active]
Active --> Events[Process Manager Events]
Events --> OnData[onData]
Events --> OnExit[onExit]
Events --> OnUsage[onUsage]
Events --> OnTool[onToolExecution]
OnData --> AgentStatus[Agent Status Exporter<br/>Updates JSON]
OnExit --> Webhook[Notification Webhook<br/>Sends HTTP POST]
UI[Settings Modal: Encore Tab] --> Toggle{User Toggles<br/>Encore}
Toggle -->|Enable| IPC1[IPC: encores:enable]
Toggle -->|Disable| IPC2[IPC: encores:disable]
IPC1 --> Persist1[Set encore:id:enabled = true]
IPC2 --> Persist2[Set encore:id:enabled = false]
Persist1 --> Activate
Persist2 --> Deactivate[EncoreHost.deactivateEncore]
Deactivate --> Cleanup[Call encore.deactivate<br/>Cleanup subscriptions]
Last reviewed commit: 2fde662 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (20)
CLAUDE-ENCORES.md-19-23 (1)
19-23:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks (MD040).
🧹 Suggested fix
-``` +```text src/encores/ # Bundled first-party encore source ├── agent-status-exporter/ # Exports agent status to JSON └── notification-webhook/ # Sends webhooks on agent events -``` +``` -``` +```text Bootstrap → Discover → Validate → Auto-enable (first-party) → Activate ↓ Encore receives EncoreAPI (scoped by permissions) -``` +``` -``` +```text my-encore/ ├── manifest.json ├── index.js └── README.md (optional, displayed in UI) -``` +```Also applies to: 42-47, 272-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE-ENCORES.md` around lines 19 - 23, The MD040 lint failures are due to missing language identifiers on fenced code blocks; update the three shown blocks that start with "src/encores/ # Bundled first-party encore source", "Bootstrap → Discover → Validate → Auto-enable (first-party) → Activate", and "my-encore/" to use a language hint (e.g., ```text) instead of bare ```; apply the same change to the other occurrences noted (lines around 42-47 and 272-277) so every fenced block includes a language identifier.docs/research/plugin-feasibility/concept-external-integration.md-160-164 (1)
160-164:⚠️ Potential issue | 🟡 MinorCapitalize “Markdown” (proper noun).
✍️ Suggested fix
-| Obsidian vault sync | **Easy** | Pattern 1 + Gap A (tool executions) | Poll `/api/session/:id` for logs, format as markdown | +| Obsidian vault sync | **Easy** | Pattern 1 + Gap A (tool executions) | Poll `/api/session/:id` for logs, format as Markdown |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/research/plugin-feasibility/concept-external-integration.md` around lines 160 - 164, In the table row for "Obsidian vault sync" locate the cell that reads "Poll `/api/session/:id` for logs, format as markdown" and update the word "markdown" to the proper noun "Markdown" so the cell reads "Poll `/api/session/:id` for logs, format as Markdown".docs/research/plugin-feasibility/concept-agent-guardrails.md-37-47 (1)
37-47:⚠️ Potential issue | 🟡 MinorAdd a language to the fenced code block (MD040).
markdownlint flags the unlabeled fence here; usetext/plaintext.🧹 Suggested fix
-``` +```text Agent stdout → StdoutHandler.handleData() → StdoutHandler.processLine() → handleParsedEvent() → this.emitter.emit('tool-execution', sessionId, toolExecution) → this.emitter.emit('thinking-chunk', sessionId, text) → bufferManager.emitDataBuffered(sessionId, data) → DataBufferManager accumulates (max 8KB or 50ms) → flushDataBuffer() → emitter.emit('data', sessionId, data) -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/research/plugin-feasibility/concept-agent-guardrails.md` around lines 37 - 47, The fenced code block showing the event flow (starting with "Agent stdout → StdoutHandler.handleData()" and mentioning StdoutHandler.processLine(), handleParsedEvent(), and emitter.emit('tool-execution'/'thinking-chunk')) is missing a language label; update the markdown fence to use a language like text or plaintext (e.g., change ``` to ```text) so markdownlint MD040 is satisfied while preserving the exact block content and formatting.CLAUDE-ENCORES.md-42-53 (1)
42-53:⚠️ Potential issue | 🟡 MinorLifecycle doc mischaracterizes encore startup behavior. The documentation states "Auto-enable (first-party)", but the actual implementation shows all encores (including first-party) start disabled. Only encores previously enabled by the user are restored on startup. Replace the misleading "Auto-enable" terminology to accurately reflect this restore behavior.
✍️ Suggested wording update
-Bootstrap → Discover → Validate → Auto-enable (first-party) → Activate +Bootstrap → Discover → Validate → Restore previously enabled → Activate ... -4. **Auto-enable**: First-party encores activate unless user explicitly disabled them +4. **Restore**: Encores that were previously enabled by the user are re-activated; others remain disabled🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE-ENCORES.md` around lines 42 - 53, The lifecycle doc incorrectly says "Auto-enable (first-party)" but the code actually restores only previously enabled encores; update the wording to reflect "Restore previously enabled encores" (or similar) and adjust the step description to state that first-party encores are not auto-enabled on startup but are merely restored if the user had enabled them before; update the list item text and any surrounding explanatory lines that mention bootstrapBundledEncores, discoverEncores and EncoreHost.activate so the doc matches the implementation (bootstrapBundledEncores copies dist/encores, discoverEncores reads manifests, validation runs, then the system restores previously enabled encores before calling EncoreHost.activate to load modules and call activate(api)).docs/research/plugin-feasibility/concept-notifications.md-272-279 (1)
272-279:⚠️ Potential issue | 🟡 MinorEscape pipes in wiki links inside tables.
The
[[concept-agent-dashboard|Dashboard]]style adds extra table columns (MD056). Escape the|or switch to standard Markdown links.✅ Suggested fix
-| [[concept-agent-dashboard|Dashboard]] | Trivial | Yes | No | Minimal | -| **Notifications** | **Moderate** | **No** | **Yes (HTTP dispatch)** | **Plugin IPC bridge, storage** | -| [[concept-ai-auditor|Auditor]] | Moderate | No | Yes (SQLite) | Storage API | -| [[concept-agent-guardrails|Guardrails]] | Moderate-Hard | No | Yes (process control) | Process control API, storage | +| [[concept-agent-dashboard\|Dashboard]] | Trivial | Yes | No | Minimal | +| **Notifications** | **Moderate** | **No** | **Yes (HTTP dispatch)** | **Plugin IPC bridge, storage** | +| [[concept-ai-auditor\|Auditor]] | Moderate | No | Yes (SQLite) | Storage API | +| [[concept-agent-guardrails\|Guardrails]] | Moderate-Hard | No | Yes (process control) | Process control API, storage |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/research/plugin-feasibility/concept-notifications.md` around lines 272 - 279, The table contains wiki-style links with an unescaped pipe (e.g. [[concept-agent-dashboard|Dashboard]]) which breaks the Markdown table; fix by escaping the pipe as \| inside the wiki link (i.e. [[concept-agent-dashboard\|Dashboard]]) or replace the wiki link with a standard Markdown link like [Dashboard](concept-agent-dashboard) so the table columns render correctly—update occurrences including the Notifications row and any other [[...|...]] entries in that table.src/encores/agent-status-exporter/README.md-13-15 (1)
13-15:⚠️ Potential issue | 🟡 MinorAdd a language hint to the fenced code block.
markdownlint MD040 flags the path block without a language. Use
textfor clarity.✅ Suggested fix
-``` +```text ~/.config/maestro/plugins/agent-status-exporter/data/status.json</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/encores/agent-status-exporter/README.mdaround lines 13 - 15, The fenced
code block in README.md should include a language hint to satisfy markdownlint
MD040; update the block around the path string (the fenced block showing
"~/.config/maestro/plugins/agent-status-exporter/data/status.json") to usedocs/research/plugin-feasibility/concept-notifications.md-45-48 (1)
45-48:⚠️ Potential issue | 🟡 MinorEscape pipes in table cells to fix column count.
The
eventType: 'rename' | 'change'cell adds extra columns (MD056). Escape the pipe or replace it.✅ Suggested fix
-| `autorun:fileChanged` | `window.maestro.autorun.onFileChanged(cb)` | `{ folderPath, filename, eventType: 'rename' | 'change' }` | Optional: notify when auto-run documents are modified (useful for team awareness) | +| `autorun:fileChanged` | `window.maestro.autorun.onFileChanged(cb)` | `{ folderPath, filename, eventType: 'rename' \| 'change' }` | Optional: notify when auto-run documents are modified (useful for team awareness) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/research/plugin-feasibility/concept-notifications.md` around lines 45 - 48, The table row for the `autorun:fileChanged` event contains an unescaped pipe in the `eventType: 'rename' | 'change'` cell which breaks the Markdown table columns; update the cell in the docs/research/plugin-feasibility/concept-notifications.md entry for `autorun:fileChanged` (and the `window.maestro.autorun.onFileChanged(cb)` API reference) to either escape the pipe characters (e.g. `eventType: 'rename' \| 'change'`) or replace the pipe with a word like "or" (e.g. `eventType: 'rename' or 'change'`) so the table renders with the correct column count.docs/research/plugin-feasibility/concept-notifications.md-176-218 (1)
176-218:⚠️ Potential issue | 🟡 MinorSpecify a language for the diagram block.
markdownlint MD040 flags the fenced block without a language identifier. Use
text.✅ Suggested fix
-``` +```text ┌─────────────────────────────────────────────────────────┐ │ RENDERER (event observation + UI) │ @@ └─────────────────────────────────────────────────────────┘</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/research/plugin-feasibility/concept-notifications.mdaround lines 176 -
218, The fenced ASCII diagram block is missing a language identifier
(markdownlint MD040); update the triple-backtick that opens the diagram to
include the language token "text" (i.e., changetotext) in the diagram
block in concept-notifications.md so the block is properly annotated and the
linter warning is resolved.</details> </blockquote></details> <details> <summary>src/encores/notification-webhook/README.md-11-13 (1)</summary><blockquote> `11-13`: _⚠️ Potential issue_ | _🟡 Minor_ **Update setup step to match the “Encore” tab label.** The setup instructions still say “Plugins tab,” which is inconsistent with the current UI naming. <details> <summary>Proposed fix</summary> ```diff -1. Enable the plugin in the Plugins tab +1. Enable the encore in the Encore tab ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/encores/notification-webhook/README.md` around lines 11 - 13, Update the README step that reads "Enable the plugin in the Plugins tab" to match the current UI by changing the text to "Enable the plugin in the Encore tab"; locate the string in src/encores/notification-webhook/README.md and replace the "Plugins tab" phrase so the setup instructions match the UI label. ``` </details> </blockquote></details> <details> <summary>docs/research/plugin-feasibility/README.md-181-210 (1)</summary><blockquote> `181-210`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language tag to the fenced code block (MD040).** <details> <summary>Proposed fix</summary> ```diff -``` +```text Phase 02: Plugin Manifest + Loader (`#6`) │ ├── Phase 03: Plugin API Surface + Sandboxing (`#2`) │ │ │ └── Phase 04: Main-Process Activation + Storage (`#1`, `#3`, `#8`) │ │ │ └── Phase 05: Plugin UI Registration (`#10`, `#5`, `#4`) │ │ │ └── Phase 06: Reference Plugins │ │ │ ├── Agent Dashboard [TRIVIAL] — validates renderer-only plugins │ │ │ └── Notification Webhook [MODERATE] — validates main-process-only plugins │ │ │ └── Phase 07: Settings, Distribution, v2 Roadmap │ │ │ └── Phase 08: Documentation & Developer Guide ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/research/plugin-feasibility/README.mdaround lines 181 - 210, The
fenced code block containing the phase diagram lacks a language tag (MD040);
update the opening fence fromtotext so the block is tagged as plain
text (i.e., replace the opening triple backticks of the block that starts with
"Phase 02: Plugin Manifest + Loader (#6)" with "```text").</details> </blockquote></details> <details> <summary>src/__tests__/main/encore-reference.test.ts-66-69 (1)</summary><blockquote> `66-69`: _⚠️ Potential issue_ | _🟡 Minor_ **Avoid swallowing unexpected deactivate errors in tests.** The blanket catch can hide real regressions; only ignore a known “not activated” case and rethrow everything else. <details> <summary>Proposed fix</summary> ```diff - try { await dashboard.deactivate(); } catch { /* ignore if not activated */ } + await dashboard.deactivate().catch((err: unknown) => { + if (String(err).includes('not activated')) return; + throw err; + }); ``` </details> As per coding guidelines: "Handle only expected/recoverable errors explicitly with specific error codes. Let unexpected errors bubble up to Sentry." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/encore-reference.test.ts` around lines 66 - 69, The afterEach cleanup currently swallows all errors from dashboard.deactivate(); change it to catch only the expected "not activated" error and rethrow any other errors: call await dashboard.deactivate() inside try/catch in the afterEach, inspect the caught error (e.g., error.code === 'NOT_ACTIVATED' or error.message.includes('not activated') depending on how dashboard.deactivate() signals that state) and return/ignore only in that specific case, otherwise rethrow the error so unexpected failures surface; keep vi.useRealTimers() after the try/catch as before. ``` </details> </blockquote></details> <details> <summary>src/renderer/components/Settings/EncoreFeatureCard.tsx-65-67 (1)</summary><blockquote> `65-67`: _⚠️ Potential issue_ | _🟡 Minor_ **Add focus attributes required by renderer UI guidelines.** <details> <summary>Proposed fix</summary> ```diff - <button - className="w-full flex items-center justify-between p-4 text-left" + <button + className="w-full flex items-center justify-between p-4 text-left outline-none" + tabIndex={0} onClick={onToggle} > ``` </details> As per coding guidelines: "Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Settings/EncoreFeatureCard.tsx` around lines 65 - 67, The button in the EncoreFeatureCard component (the element with onClick={onToggle}) is missing required focus attributes per renderer UI guidelines; update that button to include an explicit tabIndex (use tabIndex={0} to make it keyboard focusable) and add the outline-none CSS class to its className so focus styling is handled correctly, ensuring the change is applied on the JSX element inside EncoreFeatureCard where onToggle is wired. ``` </details> </blockquote></details> <details> <summary>src/renderer/hooks/useEncoreRegistry.ts-40-42 (1)</summary><blockquote> `40-42`: _⚠️ Potential issue_ | _🟡 Minor_ **Avoid swallowing exceptions with `console.error`.** Per coding guidelines, exceptions should bubble up to Sentry for error tracking rather than being silently caught and logged. <details> <summary>Proposed fix</summary> ```diff } catch (err) { - console.error('Failed to fetch encores:', err); setEncores([]); + // Re-throw to allow error boundary or Sentry to capture + throw err; } finally { ``` </details> Alternatively, if this is an expected/recoverable error (e.g., encores API not available), use Sentry utilities for explicit error reporting with context. As per coding guidelines: "Do NOT silently swallow exceptions with try-catch-console.error blocks." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/useEncoreRegistry.ts` around lines 40 - 42, The catch block in useEncoreRegistry currently swallows errors with console.error and clears state (console.error, setEncores); instead either remove the try/catch so the exception bubbles to Sentry or explicitly report it using the project's Sentry helper (e.g., import and call Sentry.captureException or the app-specific reportError utility) with contextual info (e.g., action: "fetch encores") and then handle recovery (keep or clear setEncores only if it's a known-recoverable case) or rethrow the error to let global error handling pick it up; update the catch to call the Sentry/reporting function with err and context and avoid silent console.error so errors are tracked. ``` </details> </blockquote></details> <details> <summary>src/__tests__/main/encore-activation.test.ts-203-213 (1)</summary><blockquote> `203-213`: _⚠️ Potential issue_ | _🟡 Minor_ **Replace `resolves.not.toThrow()` with `resolves.toBeUndefined()`** — `toThrow()` expects a function; for promises that resolve to `undefined`, use the appropriate assertion instead. <details> <summary>🔧 Suggested fixes</summary> ```diff - await expect(host.deactivateEncore('test-encore')).resolves.not.toThrow(); + await expect(host.deactivateEncore('test-encore')).resolves.toBeUndefined(); ``` ```diff - await expect(storage.delete('missing.json')).resolves.not.toThrow(); + await expect(storage.delete('missing.json')).resolves.toBeUndefined(); ``` </details> This pattern appears across multiple test files (system.test.ts, group-chat-storage.test.ts, group-chat-moderator.test.ts, contextSummarizer.test.ts, contextGroomer.test.ts). <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/encore-activation.test.ts` around lines 203 - 213, The test "deactivation errors are logged but do not propagate" uses await expect(host.deactivateEncore('test-encore')).resolves.not.toThrow(), which is incorrect because toThrow() expects a function; update the assertion to await expect(host.deactivateEncore('test-encore')).resolves.toBeUndefined() (or another appropriate resolves matcher) and ensure the subsequent expect(host.getEncoreContext('test-encore')).toBeUndefined() remains; apply the same replacement in the other affected tests (system.test.ts, group-chat-storage.test.ts, group-chat-moderator.test.ts, contextSummarizer.test.ts, contextGroomer.test.ts) where makeEncore(), host.deactivateEncore(...), and host.getEncoreContext(...) are used. ``` </details> </blockquote></details> <details> <summary>src/renderer/components/SettingsModal.tsx-3067-3087 (1)</summary><blockquote> `3067-3087`: _⚠️ Potential issue_ | _🟡 Minor_ **The 'loaded' state is never assigned in the codebase.** The check at lines 3069 (`encore.state === 'active' || encore.state === 'loaded'`) will never match 'loaded' in practice. The encore-manager only assigns 'active', 'disabled', 'discovered', or 'error' states. Remove the `|| encore.state === 'loaded'` check to align with actual state usage, or document why 'loaded' is being preserved for future use. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsModal.tsx` around lines 3067 - 3087, The conditional checking encore.state includes a non-existent 'loaded' value; update the code in the EncoreFeatureCard rendering (where encore.state is evaluated) to remove the orphaned "|| encore.state === 'loaded'" check so enabled is only true for encore.state === 'active', and ensure any toggling logic using props.encoreRegistry.enableEncore / disableEncore remains unchanged; alternatively, if you intend to keep 'loaded' for future use, add a short code comment near the enabled calculation explaining why 'loaded' is preserved and referencing EncoreFeatureCard and props.encoreRegistry. ``` </details> </blockquote></details> <details> <summary>src/encores/notification-webhook/index.js-16-20 (1)</summary><blockquote> `16-20`: _⚠️ Potential issue_ | _🟡 Minor_ **Potential memory leak for orphaned sessions.** The `sessionOutput`, `sessionAgentType`, and `sessionAgentName` Maps are only cleaned up in the `onExit` handler. If a session crashes or is forcefully terminated without triggering an exit event, these Maps will retain stale entries indefinitely. Consider adding a periodic cleanup or TTL mechanism for entries. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/encores/notification-webhook/index.js` around lines 16 - 20, The three Maps (sessionOutput, sessionAgentType, sessionAgentName) can leak if a session never fires onExit; add a TTL-based cleanup: when you create/update entries in sessionOutput/sessionAgentType/sessionAgentName (the spots that currently set values), store a timestamp or wrap the value in an object with lastSeen, then add a periodic sweeper (setInterval) that scans these Maps and deletes entries older than a configurable threshold (e.g. minutes) to reclaim orphaned session data; also ensure the sweeper is cleared on shutdown and keep the existing onExit cleanup to remove normal sessions. ``` </details> </blockquote></details> <details> <summary>src/renderer/components/EncoreManager.tsx-124-126 (1)</summary><blockquote> `124-126`: _⚠️ Potential issue_ | _🟡 Minor_ **Silent exception swallowing in settings save.** Save failures should be surfaced to users or logged to Sentry rather than silently ignored. Users expect feedback when their settings fail to persist. <details> <summary>📝 Suggested improvement</summary> ```diff - } catch { - // Ignore save errors + } catch (err) { + console.error('[EncoreSettings] Failed to save setting:', err); + // Optionally show user feedback via toast/notification } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/EncoreManager.tsx` around lines 124 - 126, The catch block in EncoreManager.tsx that currently swallows save errors should surface failures: update the catch to capture and log the thrown error (e.g., call Sentry.captureException(error) or processLogger.error) and also surface a user-facing failure (set a component state like saveError or call your toast/notification helper) so the UI can show an error message when settings persist fails; keep the surrounding save logic (the try block) intact but replace the empty catch with error handling that logs the error and triggers user feedback. ``` </details> </blockquote></details> <details> <summary>src/renderer/components/EncoreManager.tsx-98-99 (1)</summary><blockquote> `98-99`: _⚠️ Potential issue_ | _🟡 Minor_ **Silent exception swallowing in settings load.** Per coding guidelines, exceptions should bubble up to Sentry for tracking rather than being silently ignored. Consider using `captureException` from Sentry utilities for unexpected errors. <details> <summary>📝 Suggested improvement</summary> ```diff } catch { - // Ignore load errors + // Load errors are non-critical for UI display } finally { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/EncoreManager.tsx` around lines 98 - 99, In EncoreManager.tsx replace the silent catch block used when loading settings with one that reports the error to Sentry: import and call the Sentry helper captureException (rather than swallowing the error) inside the catch that surrounds the settings load (the catch in EncoreManager where settings are loaded), and optionally rethrow or surface the error as needed so it’s tracked; ensure you reference the existing settings load flow in EncoreManager and call captureException(err) with the caught exception. ``` </details> </blockquote></details> <details> <summary>src/shared/encore-types.ts-103-104 (1)</summary><blockquote> `103-104`: _⚠️ Potential issue_ | _🟡 Minor_ **Misleading JSDoc comment about firstParty behavior.** The comment says "auto-enabled on discovery," but according to PR objectives, all encores start disabled by default and the `firstParty` flag no longer auto-enables encores. Update the comment to reflect the actual behavior. <details> <summary>📝 Suggested fix</summary> ```diff - /** Whether this is a first-party Maestro encore (auto-enabled on discovery) */ + /** Whether this is a first-party Maestro encore (bundled with Maestro) */ firstParty?: boolean; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/shared/encore-types.ts` around lines 103 - 104, The JSDoc for the firstParty property is incorrect about auto-enabling on discovery; update the comment above the firstParty field (the property named firstParty in the Encore/encore type in src/shared/encore-types.ts) to state that first-party encores are marked as first-party but are not auto-enabled and that, per current behavior, all encores start disabled by default; keep the note brief and accurate about current semantics (firstParty is a marker only, does not change enabled state). ``` </details> </blockquote></details> <details> <summary>src/main/encore-manager.ts-139-144 (1)</summary><blockquote> `139-144`: _⚠️ Potential issue_ | _🟡 Minor_ **State set to 'active' when host is null may cause inconsistency.** When `this.host` is null, the state is set to `'active'` without actual activation. This could lead to UI showing an encore as enabled when it isn't running. Consider keeping state as `'discovered'` or throwing if host is required. <details> <summary>📝 Suggested fix</summary> ```diff if (this.host) { await this.host.activateEncore(encore); // activateEncore sets state to 'active' or 'error' } else { - encore.state = 'active'; + logger.warn(`Cannot activate encore '${id}' without host`, LOG_CONTEXT); + return false; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/encore-manager.ts` around lines 139 - 144, The code unconditionally sets encore.state = 'active' when this.host is null, which falsely marks unactivated encores as active; update the else branch that currently sets encore.state to 'active' (adjacent to this.host.activateEncore(encore)) to not flip to 'active'—either leave encore.state as 'discovered' or throw/log an error if a host is mandatory; specifically, replace the else path to preserve the existing state (or set to 'discovered') and emit a warning/exception via the same manager (e.g., using the host-checking logic in EncoreManager) so the UI won't show an unactivated encore as enabled. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (14)</summary><blockquote> <details> <summary>package.json (1)</summary><blockquote> `22-31`: **Add defensive cleanup before copying to prevent stale encores in development.** While CI releases run on fresh VMs, local development workflows can accumulate stale files in `dist/encores` when encores are deleted or renamed. `fs.cpSync()` merges into the destination rather than replacing it, so adding a pre-copy cleanup is a safe best practice. <details> <summary>♻️ Suggested update</summary> ```diff - "build:encores": "node -e \"const fs=require('fs');fs.cpSync('src/encores','dist/encores',{recursive:true})\"", + "build:encores": "node -e \"const fs=require('fs');fs.rmSync('dist/encores',{recursive:true,force:true});fs.cpSync('src/encores','dist/encores',{recursive:true})\"", ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 22 - 31, The build:encores script uses fs.cpSync to copy src/encores into dist/encores which merges and can leave stale files; update the build:encores step to defensively remove the destination directory first (e.g., call fs.rmSync('dist/encores', { recursive: true, force: true }) or equivalent) and then recreate it or run fs.cpSync, ensuring you reference the same paths (src/encores and dist/encores) and functions (fs.rmSync / fs.cpSync) so deleted/renamed encores don't persist in development builds. ``` </details> </blockquote></details> <details> <summary>src/main/encore-storage.ts (1)</summary><blockquote> `63-70`: **Exception handling follows expected-error pattern.** The `read` method returns `null` for missing files, which is intentional behavior for this API. However, this catches all errors including unexpected ones (e.g., permission denied, disk errors). Consider catching only `ENOENT` explicitly and letting other errors bubble up for better diagnostics. <details> <summary>Proposed fix for explicit error handling</summary> ```diff async read(filename: string): Promise<string | null> { validateFilename(filename, this.baseDir); try { return await fs.readFile(path.join(this.baseDir, filename), 'utf-8'); - } catch { + } catch (err: unknown) { + if (err instanceof Error && 'code' in err && err.code === 'ENOENT') { + return null; + } + throw err; - return null; } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/encore-storage.ts` around lines 63 - 70, The read method currently swallows all errors; change its catch to only handle "file not found" by catching the error (e.g., catch (err: any)) and returning null when err.code === 'ENOENT', otherwise rethrow the error so unexpected issues (permission, disk) propagate; update the read function around validateFilename, fs.readFile and path.join to implement this explicit ENOENT check. ``` </details> </blockquote></details> <details> <summary>src/renderer/hooks/useEncoreRegistry.ts (1)</summary><blockquote> `52-66`: **Add error handling for enable/disable operations.** These functions call IPC APIs that may fail, but errors are not handled. Users won't know if an enable/disable operation failed. <details> <summary>Proposed approach</summary> ```diff const enableEncore = useCallback( async (id: string) => { - await window.maestro.encores.enable(id); - await refreshEncores(); + try { + const result = await window.maestro.encores.enable(id); + if (!result?.success) { + throw new Error(result?.error || 'Failed to enable encore'); + } + await refreshEncores(); + } catch (err) { + // Let error bubble up or report to Sentry + throw err; + } }, [refreshEncores] ); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/useEncoreRegistry.ts` around lines 52 - 66, The enableEncore and disableEncore callbacks call IPC APIs that can fail but currently lack error handling; wrap the body of both enableEncore and disableEncore (which call window.maestro.encores.enable(id) / window.maestro.encores.disable(id) and then refreshEncores()) in try/catch blocks, catch any thrown Error, log or surface the error (e.g., call a provided logger or window.maestro.notifyError) and optionally return/throw a failure indicator so the UI can show feedback, and still ensure refreshEncores() is called only on success or handle failures accordingly; update both functions (enableEncore and disableEncore) to implement this pattern. ``` </details> </blockquote></details> <details> <summary>src/__tests__/main/encore-loader.test.ts (1)</summary><blockquote> `185-195`: **Consider adding a test for README loading.** The `loadEncore` function attempts to load `README.md` (per the implementation in `src/main/encore-loader.ts` lines 135-140), but this test only mocks the manifest read. The README loading path isn't exercised. <details> <summary>Example test for README loading</summary> ```diff + it('loads README.md when present', async () => { + const manifest = validManifest(); + mockReadFile.mockImplementation((filePath: string) => { + if (filePath.endsWith('manifest.json')) { + return Promise.resolve(JSON.stringify(manifest)); + } + if (filePath.endsWith('README.md')) { + return Promise.resolve('# Test Encore\n\nDescription here.'); + } + return Promise.reject(new Error('ENOENT')); + }); + + const result = await loadEncore('/encores/test-encore'); + + expect(result.readme).toBe('# Test Encore\n\nDescription here.'); + }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/encore-loader.test.ts` around lines 185 - 195, The test currently only mocks the manifest read; extend the mock for the file reader used by loadEncore so it returns JSON.stringify(validManifest()) when called for the manifest path and a README string (e.g., '# Test Encore') when called for a path ending in 'README.md' (use mockReadFile.mockImplementation or conditional mocks), then call loadEncore('/encores/test-encore') and add an assertion that the returned result includes the README content (e.g., result.readme or result.manifestReadme field depending on loadEncore's return shape) in addition to the existing assertions. ``` </details> </blockquote></details> <details> <summary>src/renderer/components/EncoreTabContent.tsx (1)</summary><blockquote> `56-70`: **Add `tabIndex` and `outline-none` class to the container div.** Per coding guidelines, interactive containers in renderer components should include focus management attributes. <details> <summary>Suggested fix</summary> ```diff return ( - <div className="h-full w-full" data-encore-id={encoreId} data-tab-id={tabId}> + <div className="h-full w-full outline-none" tabIndex={-1} data-encore-id={encoreId} data-tab-id={tabId}> {/* iframe provides natural sandboxing for untrusted encore UI code. ``` </details> As per coding guidelines: `src/renderer/**/*.{tsx,jsx}`: Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/EncoreTabContent.tsx` around lines 56 - 70, The container div that wraps the iframe (the element with data-encore-id and data-tab-id in EncoreTabContent) needs explicit focus management: add a tabIndex (use tabIndex={0} to make it keyboard-focusable or tabIndex={-1} if you want programmatic focus only) and add the "outline-none" class to the div's className so it follows renderer focus guidelines and avoids default outlines when focused. ``` </details> </blockquote></details> <details> <summary>src/renderer/components/RightPanel.tsx (1)</summary><blockquote> `565-579`: **Consider extracting the IIFE to a named helper or component.** The inline IIFE pattern works but reduces readability. Extracting this to a small helper function would improve clarity. <details> <summary>Alternative approach</summary> ```diff + {/* Encore tab content */} + {activeRightTab.startsWith('encore:') && encoreList && ( + <EncoreTabContentWrapper + activeRightTab={activeRightTab} + theme={theme} + encoreList={encoreList} + /> + )} - {/* Encore tab content */} - {activeRightTab.startsWith('encore:') && encoreList && (() => { - const parts = activeRightTab.split(':'); - const pId = parts[1]; - const tId = parts.slice(2).join(':'); - return ( - <EncoreTabContent - encoreId={pId} - tabId={tId} - theme={theme} - encores={encoreList} - /> - ); - })()} ``` Where `EncoreTabContentWrapper` is a small component that handles the parsing logic. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/RightPanel.tsx` around lines 565 - 579, Replace the inline IIFE that renders EncoreTabContent with a named helper/component to improve readability: create a small component or function (e.g., EncoreTabContentWrapper) that accepts activeRightTab, encoreList, and theme, performs the parsing of activeRightTab into pId and tId, and returns <EncoreTabContent encoreId={pId} tabId={tId} theme={theme} encores={encoreList} />; then use that wrapper in the JSX conditional (keep the conditional activeRightTab.startsWith('encore:') && encoreList) and pass the same props, removing the IIFE from RightPanel.tsx. ``` </details> </blockquote></details> <details> <summary>src/renderer/components/EncoreManager.tsx (2)</summary><blockquote> `189-225`: **Missing tabIndex for keyboard accessibility.** Per coding guidelines for renderer components, input elements should have `tabIndex={0}` and `outline-none` class to ensure proper focus behavior. <details> <summary>♿ Suggested fix</summary> ```diff <input type="text" value={displayValue} + tabIndex={0} onChange={(e) => { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/EncoreManager.tsx` around lines 189 - 225, The input element inside EncoreManager.tsx (the input that uses displayValue/onChange/onBlur/onKeyDown and references setting.key, setLocalValues, setErrors, and handleBlurSave) is missing tabIndex for keyboard accessibility; add tabIndex={0} to that input element so it participates in tab order (the element already has the outline-none class, so no change needed to classes). ``` </details> --- `364-366`: **Error logged to console instead of Sentry.** Per coding guidelines, unexpected errors should bubble up to Sentry rather than being caught with `console.error`. Consider removing the try-catch or using Sentry's `captureException`. <details> <summary>📝 Suggested fix</summary> ```diff - } catch (err) { - console.error('Failed to open encores folder:', err); - } + } catch { + // Non-critical: folder open is a best-effort convenience action + } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/EncoreManager.tsx` around lines 364 - 366, Replace the console.error in the catch block that logs "Failed to open encores folder" inside the EncoreManager component with proper Sentry handling: call Sentry.captureException(err) (importing Sentry if not already) and then rethrow the error (or remove the try/catch entirely) so the exception bubbles to error reporting; locate the catch in the function/method in EncoreManager that contains the "Failed to open encores folder" message and update it accordingly. ``` </details> </blockquote></details> <details> <summary>src/main/encore-host.ts (3)</summary><blockquote> `120-125`: **Deactivation errors not reported to Sentry.** Activation errors are captured via `captureException`, but deactivation errors are only logged. For consistency and better error tracking, consider reporting deactivation errors to Sentry as well. <details> <summary>📝 Suggested fix</summary> ```diff } catch (err) { logger.error( `Encore '${encoreId}' threw during deactivation: ${err instanceof Error ? err.message : String(err)}`, LOG_CONTEXT ); + // Non-blocking: capture for observability but don't propagate + captureException(err, { encoreId, phase: 'deactivation' }); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/encore-host.ts` around lines 120 - 125, The deactivation catch block currently only logs errors; update it to also report the error to Sentry by calling captureException(err, { tags: { encoreId }, extra: LOG_CONTEXT }) (or similar) alongside the existing logger.error call so deactivation failures are captured the same way as activation ones; locate the catch around deactivation where encoreId, logger.error and LOG_CONTEXT are used and add the captureException invocation with the error and contextual info. ``` </details> --- `322-328`: **Type assertion `as any` bypasses range validation.** The `range` parameter is cast to `any` without validation. If `getAggregatedStats` expects specific range values, invalid input could cause runtime errors. <details> <summary>📝 Suggested improvement</summary> ```diff getAggregation: async (range: string): Promise<StatsAggregation> => { const db = getStatsDB(); if (!db) { throw new Error('Stats database not available'); } - return db.getAggregatedStats(range as any); + // Validate range against known values if needed + return db.getAggregatedStats(range as Parameters<typeof db.getAggregatedStats>[0]); }, ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/encore-host.ts` around lines 322 - 328, The getAggregation handler currently casts the range string to any which bypasses validation; update getAggregation to validate or coerce the incoming range before calling getAggregatedStats: retrieve the db via getStatsDB(), check that the provided range matches the allowed set (or parse it into the expected enum/type used by getAggregatedStats), throw a clear error for invalid values, and only then call db.getAggregatedStats(validatedRange). Ensure you reference getAggregation, getStatsDB, and getAggregatedStats when making the change so the validation maps to the exact type expected by getAggregatedStats. ``` </details> --- `88-90`: **Module loaded via require() is cached by Node.js.** The `require()` call caches the module. If an encore is deactivated and re-activated, it will use the cached module without reloading from disk. This is acceptable for v1 but worth documenting if hot-reloading is expected in the future. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/encore-host.ts` around lines 88 - 90, The require(entryPoint) call that assigns const encoreModule: EncoreModule = require(entryPoint) uses Node's module cache so re-activating an encore will reuse the cached module; update the code comment around this require to explicitly state that Node.js caches required modules and that encores will not be reloaded from disk on subsequent activations unless the cache is cleared (e.g., via delete require.cache[require.resolve(entryPoint)]) or a different loader is used, so hot-reload is not supported by the current implementation. ``` </details> </blockquote></details> <details> <summary>src/shared/encore-types.ts (1)</summary><blockquote> `141-142`: **Imports placed mid-file rather than at the top.** These imports are positioned after the manifest types section. While TypeScript allows this, placing imports at the top of the file improves readability and follows conventional module organization. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/shared/encore-types.ts` around lines 141 - 142, Move the two import lines for UsageStats and StatsAggregation from mid-file to the top of the module alongside the other imports so they appear before the manifest types section; specifically relocate the declarations importing UsageStats and StatsAggregation to the top import block, preserving their type-only import form, then run the typecheck to ensure no circular import issues and update any local references to those types if needed. ``` </details> </blockquote></details> <details> <summary>src/encores/notification-webhook/index.js (1)</summary><blockquote> `107-109`: **Silent exception swallowing in lookup.** While the intent is to gracefully handle lookup errors, silently swallowing exceptions deviates from the coding guidelines. Consider logging a debug message for observability. <details> <summary>📝 Suggested improvement</summary> ```diff } catch { - // Ignore lookup errors + // Non-critical: agent info lookup failed, continue without it } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/encores/notification-webhook/index.js` around lines 107 - 109, The catch block that swallows lookup errors in src/encores/notification-webhook/index.js should log the exception for observability instead of being silent; update the catch to capture the error (catch (err)) and call the module's logger (e.g., logger.debug or processLogger.debug) or console.debug with a short contextual message and the error object (e.g., "notification webhook lookup failed", err) so the lookup code path remains graceful but observable. ``` </details> </blockquote></details> <details> <summary>src/main/encore-manager.ts (1)</summary><blockquote> `94-97`: **Type safety bypassed with `as any` assertions.** Multiple uses of `as any` for settings store keys suggest the `MaestroSettings` type doesn't include dynamic encore-scoped keys. Consider using a typed helper or extending the type to properly handle `encore:${id}:*` keys. <details> <summary>📝 Suggested approach</summary> ```typescript // Option 1: Add a generic accessor to MaestroSettings type // Option 2: Create a typed wrapper private getEncoreStoreKey(encoreId: string, key: string): string { return `encore:${encoreId}:${key}`; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/encore-manager.ts` around lines 94 - 97, The isUserEnabled method currently bypasses type safety by using `as any`; replace this with a typed key helper or accessor so the MaestroSettings API is used correctly: add a helper like getEncoreStoreKey(encoreId: string, key: string) that returns the `encore:${encoreId}:${key}` string (or add a generic accessor on MaestroSettings), then call this helper inside isUserEnabled and remove the `as any` cast so settingsStore.get(...) uses a properly typed key; update any other encore-scoped settings accesses (same pattern) to use the new helper or accessor to preserve type safety. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary> ``` Verify each finding against the current code and only fix it if needed. Inline comments: In `@src/encores/agent-status-exporter/index.js`: - Around line 144-155: The try/catch around non-throwing property access should be removed and replaced with safe access and defaults: in the api.process.onToolExecution handler (where ensureAgent(sessionId) is called and debounceWriteStatus() is invoked), set agent.lastTool using optional chaining and nullish/boolean coalescing (e.g., tool?.toolName ?? 'unknown') and Date.now() for timestamp, without an empty catch block; ensure the code still calls ensureAgent(sessionId) and debounceWriteStatus() as before. - Around line 120-130: Replace the console.error swallow in the try/catch around api.process.getActiveProcesses() with Sentry reporting and rethrowing: call captureException(err) (or the project's Sentry wrapper) inside the catch, include context (e.g., LOG_TAG and a short message), then rethrow the error so it surfaces (i.e., captureException(err, { tags: { module: 'agent-status-exporter' } }) and throw err). Update the catch block surrounding api.process.getActiveProcesses(), keeping ensureAgent, agent.agentType, agent.pid, and agent.startTime assignments unchanged. - Around line 34-96: The catch-all in writeStatus currently swallows all errors; update the catch block in writeStatus to only ignore known permission errors (e.g., when api.settings.get throws a permissions/403 error or an error with a specific code) and for any other errors capture them with Sentry (e.g., Sentry.captureException(err)) or the repo's error-reporting API and then rethrow so the error is not silently lost; keep logging to console as needed for local debugging but ensure unexpected errors are reported and rethrown instead of being swallowed. In `@src/main/encore-ipc-bridge.ts`: - Around line 60-71: EncoreIpcBridge.send currently catches and logs handler errors which swallows exceptions; update the send method (referenced by EncoreIpcBridge.send, channelKey, handlers, logger, LOG_CONTEXT) to call the Sentry utility captureException from src/utils/sentry with contextual info (include the key, encoreId and channel) when an error occurs, still log via logger.error, and then rethrow the error so it bubbles up (i.e., captureException(err, { extra: { key, encoreId, channel } }); logger.error(...); throw err). In `@src/main/encore-loader.ts`: - Around line 226-230: The current copy logic uses fs.readdir and fs.copyFile to copy only top-level files from srcPath to destPath, which fails for nested dirs; replace that loop with a single recursive copy using fs.cp(srcPath, destPath, { recursive: true }) (or fallback to a recursive utility if older Node support is required) so all files and subdirectories under srcPath are copied into destPath; update any error handling around the existing loop to handle fs.cp's promise and ensure destPath exists/permissions are handled as before. In `@src/main/ipc/handlers/encores.ts`: - Around line 54-60: The current code calls getEncoreManager/createEncoreManager then calls manager.initialize().catch(...) which swallows failures; change this to await manager.initialize() (or explicitly call captureException/captureMessage from the Sentry utilities and then rethrow) so initialization failures surface to Sentry and propagate to fail-fast; update the block around getEncoreManager, createEncoreManager and manager.initialize to either await the promise or in the catch call src/utils/sentry.captureException/captureMessage with LOG_CONTEXT and then rethrow the error instead of only logging with logger.error. In `@src/renderer/components/Settings/DirectorNotesSettings.tsx`: - Around line 42-56: The agent-detection useEffect (the window.maestro.agents.detect promise handler) is swallowing errors; update the catch handlers across this file (including the handlers around lines 73-79, 107-122, 137-145) to call captureException from src/utils/sentry with useful context (e.g., { feature: 'DirectorNotesSettings', action: 'detect agents', agentsResult: ... }) instead of only console.error or silent ignores, and then set a visible error state or rethrow so the UI can show an error (use the existing setIsDetecting and setDetectedAgents state setters and add/set an error state like setDetectError). Ensure every promise/catch and try/catch in functions such as the agent detection useEffect and related handlers reports via captureException with contextual metadata. --- Nitpick comments: In `@package.json`: - Around line 22-31: The build:encores script uses fs.cpSync to copy src/encores into dist/encores which merges and can leave stale files; update the build:encores step to defensively remove the destination directory first (e.g., call fs.rmSync('dist/encores', { recursive: true, force: true }) or equivalent) and then recreate it or run fs.cpSync, ensuring you reference the same paths (src/encores and dist/encores) and functions (fs.rmSync / fs.cpSync) so deleted/renamed encores don't persist in development builds. In `@src/__tests__/main/encore-loader.test.ts`: - Around line 185-195: The test currently only mocks the manifest read; extend the mock for the file reader used by loadEncore so it returns JSON.stringify(validManifest()) when called for the manifest path and a README string (e.g., '# Test Encore') when called for a path ending in 'README.md' (use mockReadFile.mockImplementation or conditional mocks), then call loadEncore('/encores/test-encore') and add an assertion that the returned result includes the README content (e.g., result.readme or result.manifestReadme field depending on loadEncore's return shape) in addition to the existing assertions. In `@src/encores/notification-webhook/index.js`: - Around line 107-109: The catch block that swallows lookup errors in src/encores/notification-webhook/index.js should log the exception for observability instead of being silent; update the catch to capture the error (catch (err)) and call the module's logger (e.g., logger.debug or processLogger.debug) or console.debug with a short contextual message and the error object (e.g., "notification webhook lookup failed", err) so the lookup code path remains graceful but observable. In `@src/main/encore-host.ts`: - Around line 120-125: The deactivation catch block currently only logs errors; update it to also report the error to Sentry by calling captureException(err, { tags: { encoreId }, extra: LOG_CONTEXT }) (or similar) alongside the existing logger.error call so deactivation failures are captured the same way as activation ones; locate the catch around deactivation where encoreId, logger.error and LOG_CONTEXT are used and add the captureException invocation with the error and contextual info. - Around line 322-328: The getAggregation handler currently casts the range string to any which bypasses validation; update getAggregation to validate or coerce the incoming range before calling getAggregatedStats: retrieve the db via getStatsDB(), check that the provided range matches the allowed set (or parse it into the expected enum/type used by getAggregatedStats), throw a clear error for invalid values, and only then call db.getAggregatedStats(validatedRange). Ensure you reference getAggregation, getStatsDB, and getAggregatedStats when making the change so the validation maps to the exact type expected by getAggregatedStats. - Around line 88-90: The require(entryPoint) call that assigns const encoreModule: EncoreModule = require(entryPoint) uses Node's module cache so re-activating an encore will reuse the cached module; update the code comment around this require to explicitly state that Node.js caches required modules and that encores will not be reloaded from disk on subsequent activations unless the cache is cleared (e.g., via delete require.cache[require.resolve(entryPoint)]) or a different loader is used, so hot-reload is not supported by the current implementation. In `@src/main/encore-manager.ts`: - Around line 94-97: The isUserEnabled method currently bypasses type safety by using `as any`; replace this with a typed key helper or accessor so the MaestroSettings API is used correctly: add a helper like getEncoreStoreKey(encoreId: string, key: string) that returns the `encore:${encoreId}:${key}` string (or add a generic accessor on MaestroSettings), then call this helper inside isUserEnabled and remove the `as any` cast so settingsStore.get(...) uses a properly typed key; update any other encore-scoped settings accesses (same pattern) to use the new helper or accessor to preserve type safety. In `@src/main/encore-storage.ts`: - Around line 63-70: The read method currently swallows all errors; change its catch to only handle "file not found" by catching the error (e.g., catch (err: any)) and returning null when err.code === 'ENOENT', otherwise rethrow the error so unexpected issues (permission, disk) propagate; update the read function around validateFilename, fs.readFile and path.join to implement this explicit ENOENT check. In `@src/renderer/components/EncoreManager.tsx`: - Around line 189-225: The input element inside EncoreManager.tsx (the input that uses displayValue/onChange/onBlur/onKeyDown and references setting.key, setLocalValues, setErrors, and handleBlurSave) is missing tabIndex for keyboard accessibility; add tabIndex={0} to that input element so it participates in tab order (the element already has the outline-none class, so no change needed to classes). - Around line 364-366: Replace the console.error in the catch block that logs "Failed to open encores folder" inside the EncoreManager component with proper Sentry handling: call Sentry.captureException(err) (importing Sentry if not already) and then rethrow the error (or remove the try/catch entirely) so the exception bubbles to error reporting; locate the catch in the function/method in EncoreManager that contains the "Failed to open encores folder" message and update it accordingly. In `@src/renderer/components/EncoreTabContent.tsx`: - Around line 56-70: The container div that wraps the iframe (the element with data-encore-id and data-tab-id in EncoreTabContent) needs explicit focus management: add a tabIndex (use tabIndex={0} to make it keyboard-focusable or tabIndex={-1} if you want programmatic focus only) and add the "outline-none" class to the div's className so it follows renderer focus guidelines and avoids default outlines when focused. In `@src/renderer/components/RightPanel.tsx`: - Around line 565-579: Replace the inline IIFE that renders EncoreTabContent with a named helper/component to improve readability: create a small component or function (e.g., EncoreTabContentWrapper) that accepts activeRightTab, encoreList, and theme, performs the parsing of activeRightTab into pId and tId, and returns <EncoreTabContent encoreId={pId} tabId={tId} theme={theme} encores={encoreList} />; then use that wrapper in the JSX conditional (keep the conditional activeRightTab.startsWith('encore:') && encoreList) and pass the same props, removing the IIFE from RightPanel.tsx. In `@src/renderer/hooks/useEncoreRegistry.ts`: - Around line 52-66: The enableEncore and disableEncore callbacks call IPC APIs that can fail but currently lack error handling; wrap the body of both enableEncore and disableEncore (which call window.maestro.encores.enable(id) / window.maestro.encores.disable(id) and then refreshEncores()) in try/catch blocks, catch any thrown Error, log or surface the error (e.g., call a provided logger or window.maestro.notifyError) and optionally return/throw a failure indicator so the UI can show feedback, and still ensure refreshEncores() is called only on success or handle failures accordingly; update both functions (enableEncore and disableEncore) to implement this pattern. In `@src/shared/encore-types.ts`: - Around line 141-142: Move the two import lines for UsageStats and StatsAggregation from mid-file to the top of the module alongside the other imports so they appear before the manifest types section; specifically relocate the declarations importing UsageStats and StatsAggregation to the top import block, preserving their type-only import form, then run the typecheck to ensure no circular import issues and update any local references to those types if needed. ``` </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…ove dead code - Agent status exporter: differentiate expected filesystem errors from unexpected ones, remove unreachable try/catch around settings access and tool execution handler - IPC bridge send: report handler errors to Sentry with context - Encore loader: use fs.cp recursive to copy subdirectories during bootstrap - Encore IPC handlers: remove dead fallback that swallowed initialization failures; EncoreManager is always initialized before handler registration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
|
Nice work. I'm going to push merging this until after we get 0.15.0 out as it's already quite a bear. |
Contributor
|
I dig this, @pedramamini lets not wait to long as I do not want to have to port twice a bunch of features i have pending. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
docs/encores.md (1)
23-29: Add language specifier to fenced code blocks.Static analysis flagged these code blocks as missing language specification. For ASCII diagrams like these, use
textorplaintext:Suggested fix
-``` +```text discovered → (user enables) → active-``` +```text my-encore/ ├── manifest.jsonAlso applies to: 72-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/encores.md` around lines 23 - 29, The fenced ASCII diagrams in docs/encores.md are missing language specifiers; update the triple-backtick fences that surround the blocks containing "discovered → (user enables) → active" and the "my-encore/" tree to use a language tag (e.g., ```text) and do the same for the other ASCII block referenced (lines ~72-77) so static analysis no longer flags them.src/main/encore-loader.ts (2)
244-249: Consider capturing unexpected readdir failure to Sentry.After
mkdirsucceeds (line 241), areaddirfailure on the same directory is unexpected and may indicate a filesystem or permissions issue worth tracking. Per guidelines, unexpected errors should bubble up to Sentry.Suggested enhancement
try { entries = await fs.readdir(encoresDir); } catch (err) { logger.error(`Failed to read encores directory: ${err instanceof Error ? err.message : String(err)}`, LOG_CONTEXT); + // Import captureException at top: import { captureException } from './utils/sentry'; + captureException(err, { context: 'discoverEncores', encoresDir }); return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/encore-loader.ts` around lines 244 - 249, The readdir failure for encoresDir is unexpected and should be reported to Sentry: inside the catch block that currently calls logger.error(...) add a Sentry.captureException(err, {extra: {encoresDir}}) (or equivalent capture method used in the project) so the error is sent to monitoring before returning []; ensure the module imports the Sentry client (e.g. Sentry or captureException) and include LOG_CONTEXT or relevant tags/extra metadata to aid investigation.
202-202: JSON.parse on bundled manifest could throw with unclear context.If a bundled encore's manifest.json is malformed,
JSON.parse(srcManifestRaw)throws and is caught by the outer try-catch on line 228. The resulting warning mentions "Failed to bootstrap bundled encore" but doesn't indicate the manifest was unparseable. Consider wrapping this parse in a try-catch with a specific log message, or trust that bundled encores are validated at build time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/encore-loader.ts` at line 202, Wrap the JSON.parse of srcManifestRaw in a dedicated try-catch to surface a clear error when the bundled manifest is unparseable: around the const srcManifest = JSON.parse(srcManifestRaw) expression catch the SyntaxError (or any throw) and call the same logger used elsewhere with a specific message like "Failed to parse bundled manifest.json" including srcManifestRaw (or a truncated preview) and the error, then rethrow or return early so the outer "Failed to bootstrap bundled encore" warning isn't the only clue; update references to srcManifest/srcManifestRaw accordingly so downstream code only runs on a successfully parsed manifest.src/main/index.ts (1)
392-395: Consider capturing encore initialization failure to Sentry.The catch block logs the error but doesn't report to Sentry. While graceful degradation is appropriate here (the app continues without encores), tracking initialization failures in Sentry would help diagnose recurring issues. This pattern applies to other initialization blocks too (history manager, stats DB).
Suggested enhancement
} catch (error) { + // Import at top: import { captureException } from './utils/sentry'; + captureException(error, { context: 'encore-initialization' }); logger.error(`Failed to initialize encore system: ${error}`, 'Startup'); logger.warn('Continuing without encores - encore features will be unavailable', 'Startup'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` around lines 392 - 395, The encore initialization catch currently only logs the error; call Sentry.captureException(error) (or the project's Sentry helper) inside the catch that wraps the encore init so the failure is reported to Sentry with context; include the same 'Startup' tag/label or pass relevant extra info (e.g., which module failed) when capturing the exception. Also apply the same pattern to the other init catch blocks (history manager, stats DB) so all initialization failures are sent to Sentry for observability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/encores/agent-status-exporter/index.js`:
- Around line 128-138: The catch around api.process.getActiveProcesses currently
logs and swallows errors which is intentional to allow the exporter to continue
via event subscriptions; leave the graceful-degradation behavior as-is but add a
short comment above the try/catch (referencing getActiveProcesses, ensureAgent,
and LOG_TAG) stating that failing to seed active processes is acceptable and
will be recovered via subscriptions so reviewers understand the suppression is
deliberate.
---
Nitpick comments:
In `@docs/encores.md`:
- Around line 23-29: The fenced ASCII diagrams in docs/encores.md are missing
language specifiers; update the triple-backtick fences that surround the blocks
containing "discovered → (user enables) → active" and the "my-encore/" tree to
use a language tag (e.g., ```text) and do the same for the other ASCII block
referenced (lines ~72-77) so static analysis no longer flags them.
In `@src/main/encore-loader.ts`:
- Around line 244-249: The readdir failure for encoresDir is unexpected and
should be reported to Sentry: inside the catch block that currently calls
logger.error(...) add a Sentry.captureException(err, {extra: {encoresDir}}) (or
equivalent capture method used in the project) so the error is sent to
monitoring before returning []; ensure the module imports the Sentry client
(e.g. Sentry or captureException) and include LOG_CONTEXT or relevant tags/extra
metadata to aid investigation.
- Line 202: Wrap the JSON.parse of srcManifestRaw in a dedicated try-catch to
surface a clear error when the bundled manifest is unparseable: around the const
srcManifest = JSON.parse(srcManifestRaw) expression catch the SyntaxError (or
any throw) and call the same logger used elsewhere with a specific message like
"Failed to parse bundled manifest.json" including srcManifestRaw (or a truncated
preview) and the error, then rethrow or return early so the outer "Failed to
bootstrap bundled encore" warning isn't the only clue; update references to
srcManifest/srcManifestRaw accordingly so downstream code only runs on a
successfully parsed manifest.
In `@src/main/index.ts`:
- Around line 392-395: The encore initialization catch currently only logs the
error; call Sentry.captureException(error) (or the project's Sentry helper)
inside the catch that wraps the encore init so the failure is reported to Sentry
with context; include the same 'Startup' tag/label or pass relevant extra info
(e.g., which module failed) when capturing the exception. Also apply the same
pattern to the other init catch blocks (history manager, stats DB) so all
initialization failures are sent to Sentry for observability.
Collaborator
|
@openasocket yes exactly. allows us to add feature velocity while not having to worry about bloat and is a great stop gap until we figure out a proper plugin system one day that won't require being part of the build. @needmorecowbell can you decouple your features from the encore feature enhancements like we discussed and ping when ready? |
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
3 participants
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
EncoreFeatureCard, creating a flat, uniform feature list in Settings → Encore tabWhat's included
Core system (
src/main/encore-*.ts,src/shared/encore-types.ts):process:read,storage,settings:*,notifications,network)dist/encores/→userData/encores/.., absolute paths, null bytes, separators)encore:<id>:<channel>message routingUI (
src/renderer/components/):EncoreFeatureCard— shared toggle card wrapper used by both Director's Notes and PluginsDirectorNotesSettings— extracted ~380 LOC from SettingsModal into self-contained componentEncoreManager— browse/enable/disable encores with per-encore settings panelsEncoreTabContent— sandboxed iframe renderer for encores with UICtrl+Shift+Xopens Encore tab (when plugins feature enabled)State persistence fix:
userDisabledflag with symmetricencore:<id>:enabledbooleanenableEncore()→ sets true,disableEncore()→ sets false, startup restores only explicittrueTest plan
npm run lintpassesnpm run devrestarts; toggle off → stays off🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation