feat: Encore Features system for optional feature gating

[pedramamini]

Summary

• Introduces the Encore Features system — a generic feature-gating framework where optional features are toggled on/off from Settings, defaulting to disabled
• Moves Director's Notes under Encore Features as the first gated feature
• When disabled, features are completely invisible: no keyboard shortcuts, no menu items, no command palette entries
• Serves as a precursor to a full plugin marketplace

Changes

Core Implementation

• Added EncoreFeatureFlags type to src/renderer/types/index.ts
• Added encoreFeatures state, persistence, and defaults to useSettings.ts
• Replaced Director's Notes tab in SettingsModal.tsx with a new Encore Features tab (FlaskConical icon) containing a generic panel with per-feature toggle sections
• encoreFeatures flows from App.tsx → SettingsModal as props (not local useSettings) so toggles propagate immediately

Feature Gating (Director's Notes)

• App.tsx: Gated modal rendering, callback props, and session list setters on encoreFeatures.directorNotes
• useMainKeyboardHandler.ts: Added ctx.encoreFeatures?.directorNotes guard to shortcut handler
• SessionList.tsx: Made setDirectorNotesOpen optional, conditionally renders menu button
• QuickActionsModal.tsx: Already conditional on handler existence — passing undefined is sufficient

Documentation

• Added comprehensive "Encore Features (Feature Gating)" section to CONTRIBUTING.md with when-to-use guide, architecture, step-by-step how-to, and existing features table
• Added section 12 to CLAUDE-PATTERNS.md with gating checklist and Director's Notes as reference implementation
• Updated CLAUDE.md docs index and Key Files table

Tests

• Updated SettingsModal.test.tsx: tab button, wrap-around navigation, Encore Features panel tests, DN toggle tests, nested DN-enabled describe block with prop-based overrides

Test plan

• Open Settings → Encore Features tab appears with toggle for Director's Notes (default: off)
• With DN off: no keyboard shortcut, no hamburger menu entry, no command palette entry
• Toggle DN on → menu entry, shortcut, and command palette entry appear immediately (no restart needed)
• DN settings (provider, lookback) visible only when toggle is on
• Toggle persists across app restarts
• Tab keyboard navigation (Cmd+Shift+[ / ]) wraps correctly through Encore tab

[claude]

PR Review — Encore Features System

Overall this is a solid, well-scoped feature. The architecture is clean, the documentation is thorough, and the pattern is easy to follow for future contributors. A few issues to address before merging.

---

Potential Bug: setDirectorNotesOpen called without null-check in keyboard handler

In useMainKeyboardHandler.ts, the guard checks ctx.encoreFeatures?.directorNotes before proceeding, which is correct. But ctx.setDirectorNotesOpen is now typed as optional in useSessionListProps.ts. If somehow the guard passes but setDirectorNotesOpen is undefined, the call at the next line will throw a runtime error:

} else if (ctx.isShortcut(e, 'directorNotes') && ctx.encoreFeatures?.directorNotes) {
    e.preventDefault();
    ctx.setDirectorNotesOpen(true);  // could blow up if setDirectorNotesOpen is undefined

In App.tsx, setDirectorNotesOpen is passed as encoreFeatures.directorNotes ? setDirectorNotesOpen : undefined to useSessionListProps. The keyboard handler receives it through ctx, so if both guards are in sync this won't fire in practice — but it's a latent time bomb. A defensive call ctx.setDirectorNotesOpen?.(true) would make the intent explicit and eliminate the risk.

---

Settings indentation inconsistency in SessionList.tsx

The new conditional wrapping of the hamburger menu button has mismatched indentation:

+			{setDirectorNotesOpen && (
 			<button
 				onClick={() => {
 					setDirectorNotesOpen(true);
...
 			</button>
+		)}

The opening {setDirectorNotesOpen && ( is at one indentation level, but the closing )} is at a shallower level. The <button> contents remain at their original indentation. This should be reformatted so the button is consistently indented inside the conditional wrapper. The codebase uses tabs — make sure the wrapper and button align.

---

IIFE pattern for rendered settings is an anti-pattern

SettingsModal.tsx uses an immediately-invoked function expression (IIFE) for the Director's Notes settings body:

{encoreFeatures.directorNotes && (() => {
    // 300+ lines of logic
    return (<div>...</div>);
})()}

This was already present before this PR, but it's been kept and expanded significantly here. IIFEs in JSX are generally discouraged — they bypass React's reconciliation, create a new function on every render, and are harder to read/test in isolation. The pattern would be much cleaner as a named sub-component (e.g., <DirectorNotesSettings /> defined above the main component) or at minimum with the handler functions extracted above the render return. Given the scope of this PR is already large, it's acceptable to leave this as a follow-up — but worth noting.

---

Agent detection runs even when Director's Notes is disabled

The useEffect that calls window.maestro.agents.detect() triggers whenever activeTab === 'encore', regardless of whether Director's Notes is enabled:

useEffect(() => {
    if (!isOpen || activeTab !== 'encore') return;
    // ... runs agent detection
}, [isOpen, activeTab]);

This means navigating to the Encore tab while DN is disabled still kicks off agent detection unnecessarily. This is a minor performance nit — the effect should also guard on encoreFeatures.directorNotes:

if (!isOpen || activeTab !== 'encore' || !encoreFeatures.directorNotes) return;

The encoreFeatures prop is already destructured in scope, so this is a one-line fix.

---

Missing test: toggle updates description text on enable

The new tests cover: tab navigation, description text visible by default, DN section visible but settings hidden when disabled, and the setEncoreFeatures call on toggle. What's missing is verifying that clicking the toggle when DN is already enabled calls setEncoreFeatures with { directorNotes: false }. There's currently only a test for false → true, not true → false. Consider adding:

it("should call setEncoreFeatures with false when toggling DN off", async () => {
    render(<SettingsModal {...createDefaultProps({ encoreFeatures: { directorNotes: true } })} />);
    // navigate to encore tab, click toggle...
    expect(mockSetEncoreFeatures).toHaveBeenCalledWith({ directorNotes: false });
});

---

Minor: encore-features.png screenshot file committed without content verification

docs/screenshots/encore-features.png is referenced in docs/director-notes.md. The binary file is included in the diff. This is fine, but confirm the screenshot is up-to-date with the final UI (toggle styling, FlaskConical icon) before merging.

---

What's done well

• The prop-threading architecture (encoreFeatures from App.tsx → SettingsModal as props, not via useSettings in SettingsModal) is the right call and the PR correctly documents why in CLAUDE-PATTERNS.md.
• DEFAULT_ENCORE_FEATURES merge pattern in loadAllSettings is correct — new flags default to false for existing users without a stored value, rather than crashing.
• Making setDirectorNotesOpen optional in both HamburgerMenuContentProps and SessionListProps is the right interface change; the {setDirectorNotesOpen && (...)} conditional guard is clean.
• Documentation in CONTRIBUTING.md and CLAUDE-PATTERNS.md is comprehensive and follows the existing structure well.
• Test restructuring with the nested describe('with Director\'s Notes enabled', ...) block is a good pattern for prop-controlled conditional rendering tests.

---

Summary: Two things to fix before merging — the potential undefined call on setDirectorNotesOpen in the keyboard handler, and the indentation issue in SessionList.tsx. The agent detection guard and missing toggle-off test are lower priority but worth addressing. The rest is solid.

[claude]

Code Review: feat: Encore Features system for optional feature gating

Overall this is a clean, well-structured implementation of a feature-gating system. The architecture is sound and the documentation is thorough. A few things worth addressing:

---

Issues / Bugs

1. directorNotesOpen state leak when feature is disabled

In App.tsx, gating the modal render with encoreFeatures.directorNotes && directorNotesOpen is correct, but directorNotesOpen state itself is not reset when the feature is toggled off. If a user opens Director's Notes, then disables the feature from Settings (immediate propagation is a stated goal), the directorNotesOpen state remains true. When they re-enable it, the modal would instantly reopen. Consider resetting it on disable:

```typescript
// In setEncoreFeatures or where the toggle is handled
if (!newFlags.directorNotes) {
setDirectorNotesOpen(false);
}
```

2. Keyboard shortcut fires even with feature disabled (edge case)

In useMainKeyboardHandler.ts, the guard is:
```typescript
} else if (ctx.isShortcut(e, 'directorNotes') && ctx.encoreFeatures?.directorNotes) {
```

The e.preventDefault() is only called inside the block, so if the shortcut matches but directorNotes is disabled, the browser/OS will handle the keypress. This is probably fine, but it's a behavior change from before gating. Document whether the keyboard shortcut should silently swallow the event when disabled (to prevent e.g. Cmd+Shift+O triggering something in a focused terminal). The ? optional chaining on ctx.encoreFeatures is unnecessary since encoreFeatures is always present in useSettings; this masks potential type errors.

3. initialTab: 'director-notes' removed but not validated

The internal tab state union previously included 'director-notes', now replaced with 'encore'. Any external callers (deep links, stored preferences, notification triggers) passing 'director-notes' as settingsTab will silently fall through to the default 'general' tab rather than erroring. Check if settingsTab is ever set to 'director-notes' from external sources (IPC handlers, notifications, etc.).

---

Design / Architecture Observations

4. setEncoreFeatures replaces the entire object

The toggle in SettingsModal.tsx calls:
```typescript
setEncoreFeatures({ directorNotes: !encoreFeatures.directorNotes })
```

This is a whole-object replacement. As more features are added to EncoreFeatureFlags, any toggle must spread the existing state or it will zero out other flags. The loadAllSettings correctly merges with DEFAULT_ENCORE_FEATURES, but toggling in the UI does not. Consider a helper:
```typescript
setEncoreFeatures({ ...encoreFeatures, directorNotes: !encoreFeatures.directorNotes })
```
or a setEncoreFeature(key, value) action that merges internally. This is a real footgun for contributors adding future features.

5. Props-drilling approach is documented but creates fragility

The stated reason for passing encoreFeatures as props to SettingsModal rather than calling useSettings() internally is to ensure immediate propagation. This works, but it means SettingsModal now has two pathways to settings (local useSettings() for most settings, props for encore). This asymmetry will surprise contributors. The CLAUDE-PATTERNS.md documents this correctly, but it's still an atypical pattern. Worth a brief inline comment in SettingsModal.tsx near the props declaration to prevent someone from "cleaning this up" later.

---

Minor / Nits

6. Test coverage gap: toggle-off resets sub-settings visibility

The tests verify that DN settings are hidden when encoreFeatures.directorNotes: false and visible when true, but there's no test that checks the toggle in the UI actually hides the sub-settings after clicking (the test calls mockSetEncoreFeatures but doesn't re-render with the new state to verify the DOM updates). This isn't critical given the mock structure, but noted.

7. isBeta hardcoded list in SettingsModal.tsx

```typescript
const isBeta = tile.id === 'codex' || tile.id === 'opencode' || tile.id === 'factory-droid';
```

This list already existed pre-PR and isn't introduced here, but it's duplicated in the refactored code. Not a blocker, just worth tracking as tech debt.

8. Documentation: "Encore Features" naming rationale

The CONTRIBUTING.md describes when to use it and how, but not why the system is called "Encore Features" (vs. "Labs", "Experimental", "Advanced"). For a feature-gating system that serves as a "precursor to a full plugin marketplace", the naming may evolve. A one-liner in the docs acknowledging this would help orient future contributors.

---

Positive Notes

• The DEFAULT_ENCORE_FEATURES merge pattern in loadAllSettings is exactly right — new flags added to the interface won't break existing users' persisted settings.
• Making setDirectorNotesOpen optional at the prop/interface level (rather than using a context or event bus) is clean and TypeScript-safe.
• Test restructuring into a nested describe('with Director\'s Notes enabled') block is the right pattern.
• Documentation coverage across CLAUDE.md, CLAUDE-PATTERNS.md, and CONTRIBUTING.md is thorough and follows the project's progressive disclosure approach.

The main actionable item before merge is issue #4 (whole-object replacement in setEncoreFeatures will cause flag clobbering as more features are added). The others are lower priority.