feat: Symphony as Encore Feature + multi-URL registry support#516
Open
pedramamini wants to merge 6 commits intomainfrom
Open
feat: Symphony as Encore Feature + multi-URL registry support#516pedramamini wants to merge 6 commits intomainfrom
pedramamini wants to merge 6 commits intomainfrom
Conversation
…URL registry support - Add `symphony: boolean` (default true) to EncoreFeatureFlags - Gate Symphony modal, menu item, keyboard shortcut (⇧⌘Y), and command palette entry - Add `symphonyRegistryUrls` setting for user-configured additional registry URLs - Replace single `fetchRegistry()` with `fetchRegistries()` that fetches default + custom URLs in parallel - Merge repositories by slug (default registry wins on conflicts), isolated per-URL error handling - Add Symphony toggle + Registry Sources UI in Settings > Encore tab - Update tests for new symphony flag across all encore feature assertions
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Encore feature flags (usageStats, symphony), exposes symphonyRegistryUrls in settings (store + hook + UI), gates related UI/shortcuts behind flags, and implements multi-source Symphony registry fetching in main process with per-URL error isolation, dedupe/merge, and cache bypass when custom sources exist. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Renderer
participant SettingsStore as Settings Store
participant Main as Main Process (IPC)
participant External as Registry Endpoint
Renderer->>SettingsStore: read symphonyRegistryUrls
Renderer->>Main: IPC -> getRegistry()
Main->>SettingsStore: read symphonyRegistryUrls (via settingsStore)
Main->>External: fetch default registry URL
Main->>External: fetch custom registry URL 1
Main->>External: fetch custom registry URL N
External-->>Main: registry data or error (per-URL)
Main->>Main: redactUrlForLog, dedupe by slug, merge repos
alt all sources failed
Main-->>Renderer: return network error
else some success
Main-->>Renderer: return merged registry payload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/__tests__/renderer/components/SettingsModal.test.tsx (1)
114-116: Add direct tests for Registry Sources mutations and persistence path.Current additions validate Symphony visibility/toggling, but they still don’t assert that Registry Sources interactions call
setSymphonyRegistryUrls(add/remove/invalid URL). Consider exposing a shared mock setter and asserting mutation calls explicitly.Proposed test-focused refactor
const mockSetDirectorNotesSettings = vi.fn(); const mockSetEncoreFeatures = vi.fn(); +const mockSetSymphonyRegistryUrls = vi.fn(); // Mock useSettings hook vi.mock('../../../renderer/hooks/settings/useSettings', () => ({ useSettings: () => ({ // Symphony registry URLs symphonyRegistryUrls: [], - setSymphonyRegistryUrls: vi.fn(), + setSymphonyRegistryUrls: mockSetSymphonyRegistryUrls, ...mockUseSettingsOverrides, }), }));+it('should append a valid custom registry URL', async () => { + mockSetSymphonyRegistryUrls.mockClear(); + render(<SettingsModal {...createDefaultProps()} />); + // open Encore Features, add URL, submit... + // expect(mockSetSymphonyRegistryUrls).toHaveBeenCalledWith([ + // 'https://raw.githubusercontent.com/maestro-ai/maestro/main/registry/registry.json', + // 'https://example.com/registry.json' + // ]); +}); + +it('should not save invalid registry URL input', async () => { + mockSetSymphonyRegistryUrls.mockClear(); + render(<SettingsModal {...createDefaultProps()} />); + // enter invalid URL and submit... + // expect(mockSetSymphonyRegistryUrls).not.toHaveBeenCalled(); +});Based on learnings: "Ensure settings persist by verifying wrapper functions call window.maestro.settings.set() and loading code in useSettings.ts useEffect".
Also applies to: 2274-2403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/SettingsModal.test.tsx` around lines 114 - 116, Add explicit tests that assert Registry Sources mutations call the provided setter and persist via the Maestro settings API: expose and reuse a shared mock setter (setSymphonyRegistryUrls) in SettingsModal tests, simulate add/remove/invalid-URL interactions and assert setSymphonyRegistryUrls was called with the expected arrays for each case, and also assert window.maestro.settings.set was invoked with the updated symphonyRegistryUrls to verify persistence; additionally create a test for useSettings (or the effect in useSettings.ts) that mounts the hook/component and verifies it reads initial symphonyRegistryUrls from window.maestro.settings.get (or equivalent) in the useEffect so settings are loaded on mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 1078-1082: The registry cache can be stale when the configured
symphonyRegistryUrls change and the settings value is unsafely cast; fix by
runtime-validating settingsStore.get('symphonyRegistryUrls') (ensure it's an
array of strings, sanitize/normalize URLs, fallback to [] if malformed) before
passing to fetchRegistries, and update getRegistry/cache logic to include the
current normalized URL list in the cache key or explicitly
invalidate/forceRefresh the cache when the URL list differs from the cached
metadata (e.g., compare normalized lists and call
fetchRegistries/enrichWithStars with !!forceRefresh when they differ).
In `@src/renderer/App.tsx`:
- Line 11780: The modal-open boolean flags (e.g., usageDashboardOpen) can remain
true when an encoreFeatures toggle is turned off, so add logic in the App
component to reset those state flags whenever their corresponding feature flag
transitions to false: detect changes to encoreFeatures (useEffect watching
encoreFeatures) and call the relevant setters (e.g.,
setUsageDashboardOpen(false) and the other modal setters referenced around the
same places) when the matching encoreFeatures property is false; keep the
existing JSX guard (encoreFeatures.usageStats && usageDashboardOpen) but ensure
the state is cleared when the feature is disabled to avoid reopening the modal
when the feature is re-enabled.
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 3410-3417: The icon-only remove button lacks an explicit
accessible name; update the button in the block that calls
setSymphonyRegistryUrls (the onClick using symphonyRegistryUrls.filter) to add
an ARIA label (e.g. aria-label or aria-labelledby) instead of relying solely on
title so screen readers can announce it—include the URL in the label like
"Remove registry URL: {url}" for clarity and keep the existing title and styling
intact; the change should target the <button> that renders the <X /> icon.
- Around line 393-420: The duplicate/default checks in handleAddRegistryUrl are
using raw strings; normalize URLs first (e.g., parse newRegistryUrl into a URL
object, lowercase hostname, remove fragment, normalize default ports, ensure
consistent trailing slash handling for pathname) and use this normalized string
for comparisons against SYMPHONY_REGISTRY_URL and entries in
symphonyRegistryUrls; update handleAddRegistryUrl to compute normalizedNewUrl
and compare/add that normalized value (and clear input/state accordingly) so
equivalent URLs don’t bypass the duplicate/default checks.
In `@src/renderer/stores/settingsStore.ts`:
- Around line 1625-1627: The code currently casts
allSettings['symphonyRegistryUrls'] into patch.symphonyRegistryUrls without
validation; change this to validate that allSettings has a true string[] by
checking Array.isArray(allSettings['symphonyRegistryUrls']) and that every
element passes typeof === 'string' before assigning to
patch.symphonyRegistryUrls (otherwise skip assignment or assign an empty
array/default), so update the logic around allSettings, symphonyRegistryUrls and
patch in settingsStore.ts to perform this guard to avoid malformed values
entering state.
- Around line 311-312: The new action setSymphonyRegistryUrls was added to
SettingsStoreActions but not exposed to non-React consumers; update
getSettingsActions() to include setSymphonyRegistryUrls in the returned actions
object (alongside existing actions like setDirectorNotesSettings) so callers can
invoke it—locate getSettingsActions, add the setSymphonyRegistryUrls property
pointing to the store action implementation, and ensure its signature matches
SettingsStoreActions.
---
Nitpick comments:
In `@src/__tests__/renderer/components/SettingsModal.test.tsx`:
- Around line 114-116: Add explicit tests that assert Registry Sources mutations
call the provided setter and persist via the Maestro settings API: expose and
reuse a shared mock setter (setSymphonyRegistryUrls) in SettingsModal tests,
simulate add/remove/invalid-URL interactions and assert setSymphonyRegistryUrls
was called with the expected arrays for each case, and also assert
window.maestro.settings.set was invoked with the updated symphonyRegistryUrls to
verify persistence; additionally create a test for useSettings (or the effect in
useSettings.ts) that mounts the hook/component and verifies it reads initial
symphonyRegistryUrls from window.maestro.settings.get (or equivalent) in the
useEffect so settings are loaded on mount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c994954-32c0-4a33-ac61-69bdade16c34
📒 Files selected for processing (13)
src/__tests__/renderer/components/SettingsModal.test.tsxsrc/main/index.tssrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/symphony.tssrc/renderer/App.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/SettingsModal.tsxsrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/hooks/settings/useSettings.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/index.ts
Comment on lines
+393
to
+420
| const handleAddRegistryUrl = () => { | ||
| const trimmed = newRegistryUrl.trim(); | ||
| if (!trimmed) { | ||
| setRegistryUrlError('URL cannot be empty'); | ||
| return; | ||
| } | ||
| try { | ||
| const parsed = new URL(trimmed); | ||
| if (parsed.protocol !== 'https:' && parsed.protocol !== 'http:') { | ||
| setRegistryUrlError('URL must use HTTP or HTTPS'); | ||
| return; | ||
| } | ||
| } catch { | ||
| setRegistryUrlError('Invalid URL format'); | ||
| return; | ||
| } | ||
| if (trimmed === SYMPHONY_REGISTRY_URL) { | ||
| setRegistryUrlError('This is the default registry URL'); | ||
| return; | ||
| } | ||
| if (symphonyRegistryUrls.includes(trimmed)) { | ||
| setRegistryUrlError('URL already added'); | ||
| return; | ||
| } | ||
| setSymphonyRegistryUrls([...symphonyRegistryUrls, trimmed]); | ||
| setNewRegistryUrl(''); | ||
| setRegistryUrlError(null); | ||
| }; |
There was a problem hiding this comment.
Normalize registry URLs before duplicate/default checks.
Current duplicate/default checks use raw string equality, so equivalent URLs (e.g., differing by fragment, host case, or trailing slash normalization) can slip through and be fetched twice.
Proposed fix
const handleAddRegistryUrl = () => {
const trimmed = newRegistryUrl.trim();
if (!trimmed) {
setRegistryUrlError('URL cannot be empty');
return;
}
+ const canonicalize = (raw: string) => {
+ const u = new URL(raw.trim());
+ u.hash = '';
+ return u.href;
+ };
+
+ let canonical: string;
try {
const parsed = new URL(trimmed);
if (parsed.protocol !== 'https:' && parsed.protocol !== 'http:') {
setRegistryUrlError('URL must use HTTP or HTTPS');
return;
}
+ canonical = canonicalize(trimmed);
} catch {
setRegistryUrlError('Invalid URL format');
return;
}
- if (trimmed === SYMPHONY_REGISTRY_URL) {
+ const canonicalDefault = canonicalize(SYMPHONY_REGISTRY_URL);
+ if (canonical === canonicalDefault) {
setRegistryUrlError('This is the default registry URL');
return;
}
- if (symphonyRegistryUrls.includes(trimmed)) {
+ const existing = new Set(
+ symphonyRegistryUrls.map((u) => {
+ try {
+ return canonicalize(u);
+ } catch {
+ return u.trim();
+ }
+ })
+ );
+ if (existing.has(canonical)) {
setRegistryUrlError('URL already added');
return;
}
- setSymphonyRegistryUrls([...symphonyRegistryUrls, trimmed]);
+ setSymphonyRegistryUrls([...symphonyRegistryUrls, canonical]);
setNewRegistryUrl('');
setRegistryUrlError(null);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SettingsModal.tsx` around lines 393 - 420, The
duplicate/default checks in handleAddRegistryUrl are using raw strings;
normalize URLs first (e.g., parse newRegistryUrl into a URL object, lowercase
hostname, remove fragment, normalize default ports, ensure consistent trailing
slash handling for pathname) and use this normalized string for comparisons
against SYMPHONY_REGISTRY_URL and entries in symphonyRegistryUrls; update
handleAddRegistryUrl to compute normalizedNewUrl and compare/add that normalized
value (and clear input/state accordingly) so equivalent URLs don’t bypass the
duplicate/default checks.
Comment on lines
+3410
to
+3417
| <button | ||
| type="button" | ||
| onClick={() => setSymphonyRegistryUrls(symphonyRegistryUrls.filter(u => u !== url))} | ||
| className="p-0.5 rounded hover:bg-white/10 transition-colors flex-shrink-0" | ||
| style={{ color: theme.colors.error }} | ||
| title="Remove registry URL" | ||
| > | ||
| <X className="w-3 h-3" /> |
There was a problem hiding this comment.
Add an explicit accessible name to the icon-only remove button.
title is helpful, but screen readers need a reliable accessible label for icon-only controls.
Proposed fix
<button
type="button"
onClick={() => setSymphonyRegistryUrls(symphonyRegistryUrls.filter(u => u !== url))}
className="p-0.5 rounded hover:bg-white/10 transition-colors flex-shrink-0"
style={{ color: theme.colors.error }}
title="Remove registry URL"
+ aria-label={`Remove registry URL ${url}`}
>
<X className="w-3 h-3" />
</button>📝 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
| <button | |
| type="button" | |
| onClick={() => setSymphonyRegistryUrls(symphonyRegistryUrls.filter(u => u !== url))} | |
| className="p-0.5 rounded hover:bg-white/10 transition-colors flex-shrink-0" | |
| style={{ color: theme.colors.error }} | |
| title="Remove registry URL" | |
| > | |
| <X className="w-3 h-3" /> | |
| <button | |
| type="button" | |
| onClick={() => setSymphonyRegistryUrls(symphonyRegistryUrls.filter(u => u !== url))} | |
| className="p-0.5 rounded hover:bg-white/10 transition-colors flex-shrink-0" | |
| style={{ color: theme.colors.error }} | |
| title="Remove registry URL" | |
| aria-label={`Remove registry URL ${url}`} | |
| > | |
| <X className="w-3 h-3" /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SettingsModal.tsx` around lines 3410 - 3417, The
icon-only remove button lacks an explicit accessible name; update the button in
the block that calls setSymphonyRegistryUrls (the onClick using
symphonyRegistryUrls.filter) to add an ARIA label (e.g. aria-label or
aria-labelledby) instead of relying solely on title so screen readers can
announce it—include the URL in the label like "Remove registry URL: {url}" for
clarity and keep the existing title and styling intact; the change should target
the <button> that renders the <X /> icon.
…ed component structure - Resolved 3 App.tsx conflicts: adapted to new useSessionListProps hook pattern, removed usageDashboardOpen prop (now store-sourced), kept Symphony encore gating - Resolved 4 SettingsModal.test.tsx conflicts: adopted mockUseSettingsOverrides pattern, added symphony/usageStats to default encoreFeatures mock - SessionList.tsx (modify/delete): removed old monolith, applied encore feature gating to new HamburgerMenuContent.tsx (Symphony, Usage Dashboard, Director's Notes) - SettingsModal.tsx (modify/delete): removed old file, ported Symphony toggle + registry URL management and Usage & Stats toggle to new EncoreTab.tsx, gated stats section in GeneralTab.tsx behind encoreFeatures.usageStats
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/ipc/handlers/symphony.ts (1)
1122-1138:⚠️ Potential issue | 🟠 MajorCache keying ignores registry source changes, and custom URLs are unsafely cast.
Line 1122 reuses cache before considering current
symphonyRegistryUrls, so changed sources can return stale registry data. Line 1137 also trusts store data via direct cast without runtime validation.🧩 Suggested fix
async (forceRefresh?: boolean): Promise<Omit<GetRegistryResponse, 'success'>> => { const cache = await readCache(app); + const rawCustomUrls = settingsStore.get('symphonyRegistryUrls'); + const customUrls = Array.isArray(rawCustomUrls) + ? rawCustomUrls + .filter((u): u is string => typeof u === 'string' && u.trim().length > 0) + .map((u) => u.trim()) + : []; + const hasCustomSources = customUrls.length > 0; // Check cache validity if ( !forceRefresh && + !hasCustomSources && cache?.registry && isCacheValid(cache.registry.fetchedAt, REGISTRY_CACHE_TTL_MS) ) { const enriched = await enrichWithStars(cache.registry.data, cache, false); return { @@ // Fetch fresh data from all configured registries try { - const customUrls = (settingsStore.get('symphonyRegistryUrls') as string[] | undefined) ?? []; const registry = await fetchRegistries(customUrls);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 1122 - 1138, The cache is being reused without considering changes to symphonyRegistryUrls and settingsStore.get('symphonyRegistryUrls') is unsafely cast; update the logic around cache usage in the registry flow (the block using cache, isCacheValid, enrichWithStars and REGISTRY_CACHE_TTL_MS) to include the registry source in the cache key or compare the currently configured registry URLs to whatever metadata is stored with cache.registry (e.g., store and check a sourceUrls or fetchedFrom field) and only return cached data if they match; additionally, validate the settingsStore.get('symphonyRegistryUrls') result at runtime before passing to fetchRegistries (ensure it's an array of strings, sanitize/normalize URLs, and fallback to an empty array) so fetchRegistries and subsequent logic never rely on an unsafe cast.
🧹 Nitpick comments (2)
src/__tests__/renderer/components/SettingsModal.test.tsx (2)
276-278: MakesetSymphonyRegistryUrlsa shared mock for direct assertions.Using an inline
vi.fn()here makes it harder to assert Symphony registry URL updates in upcoming tests. Promote it to a top-level shared mock (same pattern as other setters) so add/remove registry tests can verify exact calls.Proposed diff
const mockSetEncoreFeatures = vi.fn(); const mockSetDirectorNotesSettings = vi.fn(); +const mockSetSymphonyRegistryUrls = vi.fn(); ... // Symphony registry URLs symphonyRegistryUrls: [], - setSymphonyRegistryUrls: vi.fn(), + setSymphonyRegistryUrls: mockSetSymphonyRegistryUrls,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/SettingsModal.test.tsx` around lines 276 - 278, Promote the inline vi.fn() used for setSymphonyRegistryUrls into a top-level shared mock (like the other setter mocks) so tests can assert calls directly; locate the SettingsModal test props where symphonyRegistryUrls and setSymphonyRegistryUrls are defined, replace the inline setSymphonyRegistryUrls: vi.fn() with a reference to a shared mock variable (e.g., const mockSetSymphonyRegistryUrls = vi.fn()) declared at the top of the test file, and update any usages/afterEach resets to reuse/mockReset that shared mock so add/remove registry tests can verify exact calls to setSymphonyRegistryUrls.
2414-2502: Registry Sources coverage is visibility-only; add interaction tests.These tests validate show/hide behavior, but the feature’s core flows still need assertions for adding/removing custom URLs and preserving the locked default source (including setter calls). That would protect the new multi-URL registry behavior from silent regressions.
If you want, I can draft the exact RTL tests for add/remove + locked-default behavior next.
Based on learnings: Ensure settings persist by verifying wrapper functions call window.maestro.settings.set() and loading code in useSettings.ts useEffect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/SettingsModal.test.tsx` around lines 2414 - 2502, Tests for "Registry Sources" only check visibility; add RTL tests inside SettingsModal tests that simulate user interactions to add a custom registry URL, remove it, and assert the expected calls to the underlying persistence API (window.maestro.settings.set) and state setter. Specifically, add tests that: render SettingsModal, open the "Encore Features" panel, open "Registry Sources", fill the add-URL input and submit then expect window.maestro.settings.set (or the exported setter used by useSettings) to be called with the new URL appended; then simulate deleting the custom URL and expect window.maestro.settings.set called with it removed; finally assert the locked default source cannot be removed (attempt to delete it and assert no removal call or that UI shows it disabled). Ensure the tests reference SettingsModal, the "Registry Sources" UI labels, window.maestro.settings.set (or the setter used in useSettings.ts), and the useEffect loading behavior in useSettings.ts when verifying initial load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 392-403: The logs in the registry-fetching block are printing the
full variable url (seen in the fetch block and logger.warn/info calls), which
can leak tokens; implement a small helper (e.g., redactUrl or sanitizeUrl) that
takes the url string and removes sensitive parts (userinfo, query string,
fragment) and use that helper when calling logger.warn and logger.info in the
function that fetches the SymphonyRegistry (references: variable url,
logger.warn, logger.info, and the code that handles response.json() into
SymphonyRegistry/data.repositories). Replace all direct uses of url in log
messages with the redacted value.
---
Duplicate comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 1122-1138: The cache is being reused without considering changes
to symphonyRegistryUrls and settingsStore.get('symphonyRegistryUrls') is
unsafely cast; update the logic around cache usage in the registry flow (the
block using cache, isCacheValid, enrichWithStars and REGISTRY_CACHE_TTL_MS) to
include the registry source in the cache key or compare the currently configured
registry URLs to whatever metadata is stored with cache.registry (e.g., store
and check a sourceUrls or fetchedFrom field) and only return cached data if they
match; additionally, validate the settingsStore.get('symphonyRegistryUrls')
result at runtime before passing to fetchRegistries (ensure it's an array of
strings, sanitize/normalize URLs, and fallback to an empty array) so
fetchRegistries and subsequent logic never rely on an unsafe cast.
---
Nitpick comments:
In `@src/__tests__/renderer/components/SettingsModal.test.tsx`:
- Around line 276-278: Promote the inline vi.fn() used for
setSymphonyRegistryUrls into a top-level shared mock (like the other setter
mocks) so tests can assert calls directly; locate the SettingsModal test props
where symphonyRegistryUrls and setSymphonyRegistryUrls are defined, replace the
inline setSymphonyRegistryUrls: vi.fn() with a reference to a shared mock
variable (e.g., const mockSetSymphonyRegistryUrls = vi.fn()) declared at the top
of the test file, and update any usages/afterEach resets to reuse/mockReset that
shared mock so add/remove registry tests can verify exact calls to
setSymphonyRegistryUrls.
- Around line 2414-2502: Tests for "Registry Sources" only check visibility; add
RTL tests inside SettingsModal tests that simulate user interactions to add a
custom registry URL, remove it, and assert the expected calls to the underlying
persistence API (window.maestro.settings.set) and state setter. Specifically,
add tests that: render SettingsModal, open the "Encore Features" panel, open
"Registry Sources", fill the add-URL input and submit then expect
window.maestro.settings.set (or the exported setter used by useSettings) to be
called with the new URL appended; then simulate deleting the custom URL and
expect window.maestro.settings.set called with it removed; finally assert the
locked default source cannot be removed (attempt to delete it and assert no
removal call or that UI shows it disabled). Ensure the tests reference
SettingsModal, the "Registry Sources" UI labels, window.maestro.settings.set (or
the setter used in useSettings.ts), and the useEffect loading behavior in
useSettings.ts when verifying initial load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 114d301f-bd34-4d36-834f-9687a09c3f7f
📒 Files selected for processing (3)
src/__tests__/renderer/components/SettingsModal.test.tsxsrc/main/index.tssrc/main/ipc/handlers/symphony.ts
- Redact registry URLs before logging to prevent credential leakage - Skip registry cache when custom source URLs are configured (stale cache fix) - Runtime-validate symphonyRegistryUrls from settings store - Reset modal-open flags when Encore Feature toggles are disabled - Normalize registry URLs before duplicate/default checks - Add aria-label to icon-only registry URL remove button - Expose setSymphonyRegistryUrls in getSettingsActions() - Validate persisted symphonyRegistryUrls with Array.isArray guard
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/ipc/handlers/symphony.ts (1)
1138-1146: Add.map((u) => u.trim())to sanitize URLs before use.The validation filters non-empty strings but doesn't actually trim them. If a malformed setting contains whitespace-padded URLs (e.g.,
" https://example.com "), they would pass the filter but the URL would be passed with whitespace tofetchRegistries, potentially causing fetch failures.🔧 Proposed fix
// Runtime-validate custom URLs from settings const rawCustomUrls = settingsStore.get('symphonyRegistryUrls'); const customUrls = Array.isArray(rawCustomUrls) - ? rawCustomUrls.filter((u): u is string => typeof u === 'string' && u.trim().length > 0) + ? rawCustomUrls + .filter((u): u is string => typeof u === 'string' && u.trim().length > 0) + .map((u) => u.trim()) : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 1138 - 1146, The custom URL array is filtered for non-empty strings but not trimmed, so whitespace-padded entries in settingsStore.get('symphonyRegistryUrls') can slip through and break fetchRegistries; update the creation of customUrls (derived from rawCustomUrls) to map each string through .trim() before filtering (i.e., apply .map((u) => u.trim()) on rawCustomUrls) so customUrls contains normalized, whitespace-free strings and hasCustomSources accurately reflects usable entries before calling fetchRegistries.src/renderer/components/Settings/tabs/EncoreTab.tsx (1)
74-79: Consider logging if default URL comparison fails.The empty catch block silently swallows any error if
SYMPHONY_REGISTRY_URLfails to parse. While this is extremely unlikely for a hardcoded constant, a brief log would help diagnose unexpected issues.🔧 Proposed fix
try { if (canonical === canonicalizeUrl(SYMPHONY_REGISTRY_URL)) { setRegistryUrlError('This is the default registry URL'); return; } - } catch { /* default URL should always parse */ } + } catch (e) { + console.warn('Failed to canonicalize default registry URL', e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Settings/tabs/EncoreTab.tsx` around lines 74 - 79, The empty catch in the canonical URL check swallows parse errors for SYMPHONY_REGISTRY_URL; update the catch in the block that compares canonical === canonicalizeUrl(SYMPHONY_REGISTRY_URL) to log the caught error (include SYMPHONY_REGISTRY_URL and the error object) before swallowing it so behavior stays the same; use console.error or the repository's logger if available and keep the existing setRegistryUrlError logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 1138-1146: The custom URL array is filtered for non-empty strings
but not trimmed, so whitespace-padded entries in
settingsStore.get('symphonyRegistryUrls') can slip through and break
fetchRegistries; update the creation of customUrls (derived from rawCustomUrls)
to map each string through .trim() before filtering (i.e., apply .map((u) =>
u.trim()) on rawCustomUrls) so customUrls contains normalized, whitespace-free
strings and hasCustomSources accurately reflects usable entries before calling
fetchRegistries.
In `@src/renderer/components/Settings/tabs/EncoreTab.tsx`:
- Around line 74-79: The empty catch in the canonical URL check swallows parse
errors for SYMPHONY_REGISTRY_URL; update the catch in the block that compares
canonical === canonicalizeUrl(SYMPHONY_REGISTRY_URL) to log the caught error
(include SYMPHONY_REGISTRY_URL and the error object) before swallowing it so
behavior stays the same; use console.error or the repository's logger if
available and keep the existing setRegistryUrlError logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b052bec-2abc-497f-a03b-19a702fd7b69
📒 Files selected for processing (4)
src/main/ipc/handlers/symphony.tssrc/renderer/App.tsxsrc/renderer/components/Settings/tabs/EncoreTab.tsxsrc/renderer/stores/settingsStore.ts
…ures tab Stats collection, time range, database management, and WakaTime settings now live under their respective Encore Feature toggle instead of General.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/renderer/components/Settings/tabs/EncoreTab.test.tsx (1)
99-123:⚠️ Potential issue | 🟠 MajorUse the full Encore flag shape in this mock.
usageStatsandsymphonyare omitted here, so the component sees them as falsey in every test even though the real defaults are on. That means this suite is exercising a state users will not hit and can miss regressions in the new default-visible sections.🧪 Suggested fix
useSettings: () => ({ - encoreFeatures: { directorNotes: false }, + encoreFeatures: { directorNotes: false, usageStats: true, symphony: true },Apply the same full shape in any per-test
mockUseSettingsOverridesthat currently only overridesdirectorNotes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/Settings/tabs/EncoreTab.test.tsx` around lines 99 - 123, The mocked useSettings return value uses an incomplete encoreFeatures shape so tests see usageStats and symphony as falsy; update the mock used by useSettings (and any per-test mockUseSettingsOverrides) to include the full encoreFeatures shape with usageStats and symphony keys (matching the real defaults) and keep existing keys like directorNotes, setEncoreFeatures, directorNotesSettings, etc., so the component renders with the same feature flags as production.
🧹 Nitpick comments (1)
src/renderer/components/Settings/tabs/EncoreTab.tsx (1)
90-137: Gate the new side effects behindusageStats.These two effects still run whenever the Encore tab opens, even when the Usage & Stats feature is toggled off. That keeps a disabled feature touching
window.maestro.statsandwindow.maestro.wakatimein the background.♻️ Suggested fix
- useEffect(() => { - if (!isOpen || !wakatimeEnabled) return; + useEffect(() => { + if (!isOpen || !encoreFeatures.usageStats || !wakatimeEnabled) return; let cancelled = false; let retryTimer: ReturnType<typeof setTimeout> | null = null; @@ - }, [isOpen, wakatimeEnabled]); + }, [isOpen, encoreFeatures.usageStats, wakatimeEnabled]); - useEffect(() => { - if (!isOpen) return; + useEffect(() => { + if (!isOpen || !encoreFeatures.usageStats) return; @@ - }, [isOpen]); + }, [isOpen, encoreFeatures.usageStats]);Also applies to: 139-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Settings/tabs/EncoreTab.tsx` around lines 90 - 137, The effect that checks WakaTime CLI (the useEffect referencing window.maestro.wakatime.checkCli and state setter setWakatimeCliStatus) should be gated by the Usage & Stats toggle; update the effect to return early unless usageStats is true (in addition to isOpen and wakatimeEnabled), add usageStats to the dependency array, and ensure the same gating is applied to the other effect that touches window.maestro.stats (the effect that calls window.maestro.stats.* and sets related state) so neither window.maestro.wakatime nor window.maestro.stats is called when usageStats is disabled; keep the existing cancellation and retryTimer cleanup logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/Settings/tabs/EncoreTab.tsx`:
- Around line 659-683: The header buttons render a decorative switch but lack
proper switch semantics; update the button that toggles
encoreFeatures.usageStats (and the analogous Symphony header button) to expose
ARIA switch state by adding role="switch" and
aria-checked={encoreFeatures.usageStats} (or the corresponding symphony state)
and ensure the accessible name is preserved (use existing children or
aria-label/aria-labelledby if needed); keep the onClick handler
(setEncoreFeatures) and visual styling unchanged so assistive tech can read the
current on/off state.
- Around line 781-807: After a successful clear in the onClick handler (the
async callback that calls window.maestro.stats.clearOldData and sets
setStatsClearResult / setStatsDbSize), also refresh the earliest-stats timestamp
by calling the appropriate API (e.g. await
window.maestro.stats.getEarliestEventDate() or getStatsEarliestDate()) and then
call setStatsEarliestDate(...) with the returned value so the "since ..." label
is updated; do this inside the if (result.success) block after updating
setStatsDbSize and before exiting the try.
- Around line 950-979: The clear-button currently causes the input to blur and
triggers the onBlur validation of the old key; prevent that by adding
onMouseDown={(e) => e.preventDefault()} to the button element so the input won’t
lose focus and the blur handler on the wakatime input won’t run; additionally,
in handleWakatimeApiKeyChange('') ensure it resets validation state (call
setWakatimeKeyValid(null) and setWakatimeKeyValidating(false)) so clearing the
key never leaves a stale success/error icon. Reference: the input's onBlur
handler, the wakatimeApiKey/wakatimeKeyValid/wakatimeKeyValidating state
setters, and the clear button that calls handleWakatimeApiKeyChange.
---
Outside diff comments:
In `@src/__tests__/renderer/components/Settings/tabs/EncoreTab.test.tsx`:
- Around line 99-123: The mocked useSettings return value uses an incomplete
encoreFeatures shape so tests see usageStats and symphony as falsy; update the
mock used by useSettings (and any per-test mockUseSettingsOverrides) to include
the full encoreFeatures shape with usageStats and symphony keys (matching the
real defaults) and keep existing keys like directorNotes, setEncoreFeatures,
directorNotesSettings, etc., so the component renders with the same feature
flags as production.
---
Nitpick comments:
In `@src/renderer/components/Settings/tabs/EncoreTab.tsx`:
- Around line 90-137: The effect that checks WakaTime CLI (the useEffect
referencing window.maestro.wakatime.checkCli and state setter
setWakatimeCliStatus) should be gated by the Usage & Stats toggle; update the
effect to return early unless usageStats is true (in addition to isOpen and
wakatimeEnabled), add usageStats to the dependency array, and ensure the same
gating is applied to the other effect that touches window.maestro.stats (the
effect that calls window.maestro.stats.* and sets related state) so neither
window.maestro.wakatime nor window.maestro.stats is called when usageStats is
disabled; keep the existing cancellation and retryTimer cleanup logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc66d2c1-4b90-43fe-b102-85cd89858940
📒 Files selected for processing (5)
src/__tests__/renderer/components/Settings/tabs/EncoreTab.test.tsxsrc/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsxsrc/__tests__/renderer/components/SettingsModal.test.tsxsrc/renderer/components/Settings/tabs/EncoreTab.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsx
Comment on lines
+659
to
+683
| <button | ||
| className="w-full flex items-center justify-between p-4 text-left" | ||
| onClick={() => setEncoreFeatures({ ...encoreFeatures, usageStats: !encoreFeatures.usageStats })} | ||
| > | ||
| <div className="flex items-center gap-3"> | ||
| <Database className="w-5 h-5" style={{ color: encoreFeatures.usageStats ? theme.colors.accent : theme.colors.textDim }} /> | ||
| <div> | ||
| <div className="text-sm font-bold" style={{ color: theme.colors.textMain }}> | ||
| Usage & Stats | ||
| </div> | ||
| <div className="text-xs mt-0.5" style={{ color: theme.colors.textDim }}> | ||
| Track queries, Auto Run sessions, and view the Usage Dashboard | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div | ||
| className={`relative w-10 h-5 rounded-full transition-colors ${encoreFeatures.usageStats ? '' : 'opacity-50'}`} | ||
| style={{ backgroundColor: encoreFeatures.usageStats ? theme.colors.accent : theme.colors.border }} | ||
| > | ||
| <div | ||
| className="absolute top-0.5 w-4 h-4 rounded-full bg-white transition-transform" | ||
| style={{ transform: encoreFeatures.usageStats ? 'translateX(22px)' : 'translateX(2px)' }} | ||
| /> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
Expose these feature headers as stateful toggles.
Both new headers are plain buttons with a decorative switch, so assistive tech gets the label but not whether the feature is currently enabled. Add switch semantics to the clickable header itself.
♿ Suggested fix
<button
+ type="button"
+ role="switch"
+ aria-checked={encoreFeatures.usageStats}
className="w-full flex items-center justify-between p-4 text-left"
onClick={() => setEncoreFeatures({ ...encoreFeatures, usageStats: !encoreFeatures.usageStats })}
>Apply the same change to the Symphony header button as well.
Also applies to: 1000-1024
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/tabs/EncoreTab.tsx` around lines 659 - 683,
The header buttons render a decorative switch but lack proper switch semantics;
update the button that toggles encoreFeatures.usageStats (and the analogous
Symphony header button) to expose ARIA switch state by adding role="switch" and
aria-checked={encoreFeatures.usageStats} (or the corresponding symphony state)
and ensure the accessible name is preserved (use existing children or
aria-label/aria-labelledby if needed); keep the onClick handler
(setEncoreFeatures) and visual styling unchanged so assistive tech can read the
current on/off state.
Comment on lines
+781
to
+807
| onClick={async () => { | ||
| const select = document.getElementById('clear-stats-period') as HTMLSelectElement; | ||
| const days = parseInt(select.value, 10); | ||
| if (!days || isNaN(days)) { | ||
| return; | ||
| } | ||
| setStatsClearing(true); | ||
| setStatsClearResult(null); | ||
| try { | ||
| const result = await window.maestro.stats.clearOldData(days); | ||
| setStatsClearResult(result); | ||
| if (result.success) { | ||
| const newSize = await window.maestro.stats.getDatabaseSize(); | ||
| setStatsDbSize(newSize); | ||
| } | ||
| } catch (err) { | ||
| console.error('Failed to clear old stats:', err); | ||
| setStatsClearResult({ | ||
| success: false, | ||
| deletedQueryEvents: 0, | ||
| deletedAutoRunSessions: 0, | ||
| deletedAutoRunTasks: 0, | ||
| error: err instanceof Error ? err.message : 'Unknown error', | ||
| }); | ||
| } finally { | ||
| setStatsClearing(false); | ||
| } |
There was a problem hiding this comment.
Refresh statsEarliestDate after a successful purge.
On success this only reloads statsDbSize. If the clear operation removes the oldest rows, the "since ..." label stays stale until the tab remounts.
🧹 Suggested fix
try {
const result = await window.maestro.stats.clearOldData(days);
setStatsClearResult(result);
if (result.success) {
- const newSize = await window.maestro.stats.getDatabaseSize();
- setStatsDbSize(newSize);
+ const [newSize, earliestTimestamp] = await Promise.all([
+ window.maestro.stats.getDatabaseSize(),
+ window.maestro.stats.getEarliestTimestamp(),
+ ]);
+ setStatsDbSize(newSize);
+ setStatsEarliestDate(
+ earliestTimestamp
+ ? new Date(earliestTimestamp).toISOString().split('T')[0]
+ : null
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/tabs/EncoreTab.tsx` around lines 781 - 807,
After a successful clear in the onClick handler (the async callback that calls
window.maestro.stats.clearOldData and sets setStatsClearResult /
setStatsDbSize), also refresh the earliest-stats timestamp by calling the
appropriate API (e.g. await window.maestro.stats.getEarliestEventDate() or
getStatsEarliestDate()) and then call setStatsEarliestDate(...) with the
returned value so the "since ..." label is updated; do this inside the if
(result.success) block after updating setStatsDbSize and before exiting the try.
Comment on lines
+950
to
+979
| onBlur={() => { | ||
| if (wakatimeApiKey) { | ||
| setWakatimeKeyValidating(true); | ||
| setWakatimeKeyValid(null); | ||
| window.maestro.wakatime | ||
| .validateApiKey(wakatimeApiKey) | ||
| .then((result) => setWakatimeKeyValid(result.valid)) | ||
| .catch(() => setWakatimeKeyValid(false)) | ||
| .finally(() => setWakatimeKeyValidating(false)); | ||
| } | ||
| }} | ||
| className="bg-transparent flex-1 text-sm outline-none" | ||
| style={{ color: theme.colors.textMain }} | ||
| placeholder="waka_..." | ||
| /> | ||
| {wakatimeKeyValidating && <span className="ml-2 text-xs opacity-50">...</span>} | ||
| {!wakatimeKeyValidating && wakatimeKeyValid === true && ( | ||
| <Check className="w-4 h-4 ml-2" style={{ color: theme.colors.success }} /> | ||
| )} | ||
| {!wakatimeKeyValidating && wakatimeKeyValid === false && wakatimeApiKey && ( | ||
| <X className="w-4 h-4 ml-2" style={{ color: theme.colors.error }} /> | ||
| )} | ||
| {wakatimeApiKey && ( | ||
| <button | ||
| onClick={() => handleWakatimeApiKeyChange('')} | ||
| className="ml-2 opacity-50 hover:opacity-100" | ||
| title="Clear API key" | ||
| > | ||
| <X className="w-3 h-3" /> | ||
| </button> |
There was a problem hiding this comment.
Clearing the API key should not validate the key you're removing.
Clicking the clear affordance blurs the input first, so the blur handler can still validate the old key and later paint a success/error icon on an empty field.
🔑 Suggested fix
{wakatimeApiKey && (
<button
+ onMouseDown={(e) => e.preventDefault()}
onClick={() => handleWakatimeApiKeyChange('')}
className="ml-2 opacity-50 hover:opacity-100"
title="Clear API key"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Settings/tabs/EncoreTab.tsx` around lines 950 - 979,
The clear-button currently causes the input to blur and triggers the onBlur
validation of the old key; prevent that by adding onMouseDown={(e) =>
e.preventDefault()} to the button element so the input won’t lose focus and the
blur handler on the wakatime input won’t run; additionally, in
handleWakatimeApiKeyChange('') ensure it resets validation state (call
setWakatimeKeyValid(null) and setWakatimeKeyValidating(false)) so clearing the
key never leaves a stale success/error icon. Reference: the input's onBlur
handler, the wakatimeApiKey/wakatimeKeyValid/wakatimeKeyValidating state
setters, and the clear button that calls handleWakatimeApiKeyChange.
- Reorder Encore Features tab: Usage & Stats → Symphony → Director's Notes - Remove BETA badge from Usage Dashboard modal header - Director's Notes retains BETA badge as it's still in beta - Usage & Stats and Symphony are default-on stable features
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
Changes
symphony: booleantoEncoreFeatureFlagssymphonyRegistryUrls: string[]with persistencesetSymphonyModalOpen, andonOpenSymphonypropsopenSymphonyshortcut withencoreFeatures.symphonysetSymphonyModalOpenoptional, wrapped menu button in conditionalfetchRegistry()withfetchRegistries(customUrls)— parallel fetch viaPromise.allSettled, merge by slug (default wins), isolated per-URL error handlingsettingsStoretoregisterSymphonyHandlersin both registration sitesTest plan
symphony: trueSummary by CodeRabbit
New Features
Chores
Tests
Docs