diff --git a/docs/agent-guides/CLI-UI-PARITY.md b/docs/agent-guides/CLI-UI-PARITY.md index 3338a9de2f..b2ebc06464 100644 --- a/docs/agent-guides/CLI-UI-PARITY.md +++ b/docs/agent-guides/CLI-UI-PARITY.md @@ -66,6 +66,7 @@ of taking a second round trip or trusting a value the caller guessed. | SSH remote execution config | `update-agent --ssh-remote / --ssh-cwd`, `create-ssh-remote` | | Focus an agent, switch AI/Shell mode | `focus-agent`, `switch-mode` | | Create / rename / remove a group | `create-group`, `rename-group`, `remove-group` | +| Group icon, color, and nesting | `create-group --icon/--color/--parent`, `update-group` | | Create a worktree agent | `create-worktree` | | New / close / rename a tab | `tab new`, `tab close`, `tab rename` | | Star a tab (Cmd+Shift+S) | `tab star` / `tab unstar` | diff --git a/docs/agent-guides/SHARED-UTILS.md b/docs/agent-guides/SHARED-UTILS.md index 39cbd1566c..2f156ab57c 100644 --- a/docs/agent-guides/SHARED-UTILS.md +++ b/docs/agent-guides/SHARED-UTILS.md @@ -301,6 +301,32 @@ span so a platform that DID fire a hide/show pair can't subtract the same sleep --- +## Group Appearance (`src/shared/groupAppearance.ts` - Both) + +The one catalog of Left Bar group icon IDs and label colors, plus the +normalization and validation over them. Three consumers read it: the renderer's +picker (`renderer/components/ui/groupAppearanceOptions.ts`, which adds the only +renderer-owned piece, the icon-ID -> Lucide mapping), the WebSocket +`create_group` / `update_group` handlers, and the `create-group` / +`update-group` CLI commands. Do NOT write a second icon-ID list: the CLI would +happily accept an icon the picker cannot draw. + +Values are normalized, not merely checked, so `#ef4444` and `#EF4444` persist +identically and a readback comparison is a plain string equal. + +| Export | Signature | Purpose | +| -------------------------------- | ----------------------------------------------------- | ----------------------------------------------------------------------------------- | +| `GROUP_ICON_CATALOG` | `readonly { id, label }[]` | Built-in icons, in picker order. | +| `GROUP_ICON_IDS` | `readonly string[]` | Just the IDs, for validation and error text. | +| `GROUP_LABEL_COLORS` | `readonly { value, label }[]` | Built-in label colors; `value` is the persisted uppercase `#RRGGBB`. | +| `normalizeGroupIconId(raw)` | `(string) => string \| null` | Canonical icon ID (built-in or `plugin/pack/local`), or `null` if unrecognized. | +| `normalizeGroupColor(raw)` | `(string) => string \| null` | Uppercased `#RRGGBB` or a namespaced plugin color ID, or `null`. | +| `validateGroupAppearance(input)` | `(GroupAppearanceInput) => GroupAppearanceValidation` | Enforces emoji/icon exclusivity and normalizes. Run BEFORE mutating any state. | +| `validateGroupUpdate(request)` | `(GroupUpdateRequest) => GroupUpdateValidation` | The above plus the clear-list rules and "an update must change something". | +| `GROUP_CLEARABLE_FIELDS` | `readonly ['emoji','icon','color','parent']` | What an update may clear. Clearing is explicit, never a `null` value over the wire. | + +--- + ## Git Utilities (`src/shared/gitUtils.ts` - Both) | Function | Signature | Purpose | diff --git a/docs/cli-reference.md b/docs/cli-reference.md index e2ee2c7319..a29223f963 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -613,35 +613,37 @@ Print the full command reference (Markdown, or --format json) Create a new agent in the Maestro desktop app -| Option | Description | Default | -| --------------------------------- | ------------------------------------------------------------------------------------------------ | --------------- | -| `-d, --cwd ` | Working directory for the agent | - | -| `-t, --type ` | Agent type (claude-code, codex, opencode, factory-droid, copilot-cli, gemini-cli, qwen3-coder) | `"claude-code"` | -| `-g, --group ` | Group ID to assign the agent to | - | -| `--nudge ` | Nudge message appended to every user message | - | -| `--new-session-message ` | Message prefixed to first message in new sessions | - | -| `--custom-path ` | Custom binary path for the agent | - | -| `--custom-args ` | Custom CLI arguments for the agent | - | -| `--env ` | Environment variable (repeatable) | `[]` | -| `--model ` | Model override (e.g., sonnet, opus) | - | -| `--effort ` | Effort/reasoning level override | - | -| `--context-window ` | Context window size in tokens | - | -| `--provider-path ` | Custom provider path | - | -| `--ssh-remote ` | SSH remote ID for remote execution | - | -| `--ssh-cwd ` | Working directory override on SSH remote | - | -| `--sync-history-to-remote ` | Sync history entries to .maestro/history/ on the remote host (true/false; requires --ssh-remote) | - | -| `--auto-run-folder ` | Path to the agent Auto Run / playbooks folder (overrides the default /.maestro/playbooks) | - | -| `--json` | Output as JSON (for scripting) | - | +| Option | Description | Default | +| --------------------------------- | ----------------------------------------------------------------------------------------------------------- | --------------- | +| `-d, --cwd ` | Working directory for the agent | - | +| `-t, --type ` | Agent type (claude-code, codex, opencode, factory-droid, copilot-cli, antigravity, gemini-cli, qwen3-coder) | `"claude-code"` | +| `-g, --group ` | Group ID to assign the agent to | - | +| `--nudge ` | Nudge message appended to every user message | - | +| `--new-session-message ` | Message prefixed to first message in new sessions | - | +| `--custom-path ` | Custom binary path for the agent | - | +| `--custom-args ` | Custom CLI arguments for the agent | - | +| `--env ` | Environment variable (repeatable) | `[]` | +| `--model ` | Model override (e.g., sonnet, opus) | - | +| `--effort ` | Effort/reasoning level override | - | +| `--context-window ` | Context window size in tokens | - | +| `--provider-path ` | Custom provider path | - | +| `--ssh-remote ` | SSH remote ID for remote execution | - | +| `--ssh-cwd ` | Working directory override on SSH remote | - | +| `--sync-history-to-remote ` | Sync history entries to .maestro/history/ on the remote host (true/false; requires --ssh-remote) | - | +| `--auto-run-folder ` | Path to the agent Auto Run / playbooks folder (overrides the default /.maestro/playbooks) | - | +| `--json` | Output as JSON (for scripting) | - | ## `maestro-cli create-group ` Create a new group in the Maestro desktop app -| Option | Description | Default | -| --------------------- | ------------------------------ | ------- | -| `-e, --emoji ` | Emoji icon for the group | - | -| `--parent ` | Create inside this root group | - | -| `--json` | Output as JSON (for scripting) | - | +| Option | Description | Default | +| --------------------- | ------------------------------------------------------------------------------------------------------ | ------- | +| `-e, --emoji ` | Emoji icon for the group | - | +| `--icon ` | Built-in icon ID (folder, briefcase, rocket, ...) or a plugin icon ID. Mutually exclusive with --emoji | - | +| `--color ` | Label color as #RRGGBB, or a plugin color ID | - | +| `--parent ` | Create inside this root group | - | +| `--json` | Output as JSON (for scripting) | - | ## `maestro-cli remove-group ` @@ -660,6 +662,23 @@ Rename a group in the Maestro desktop app | -------- | ------------------------------ | ------- | | `--json` | Output as JSON (for scripting) | - | +## `maestro-cli update-group ` + +Update a group's name, icon, color, or parent in the Maestro desktop app + +| Option | Description | Default | +| --------------------- | ------------------------------------------------------------------------------------------------------ | ------- | +| `-n, --name ` | New group name | - | +| `-e, --emoji ` | Emoji icon for the group. Mutually exclusive with --icon | - | +| `--icon ` | Built-in icon ID (folder, briefcase, rocket, ...) or a plugin icon ID. Mutually exclusive with --emoji | - | +| `--color ` | Label color as #RRGGBB, or a plugin color ID | - | +| `--parent ` | Move the group inside this root group | - | +| `--clear-emoji` | Reset the emoji to the default folder | - | +| `--clear-icon` | Remove the icon | - | +| `--clear-color` | Remove the label color | - | +| `--clear-parent` | Promote the group to the top level | - | +| `--json` | Output as JSON (for scripting) | - | + ## `maestro-cli create-worktree` Create a new agent in a git worktree branched off an existing parent agent diff --git a/docs/cli.md b/docs/cli.md index 3cc876641f..8e005b76d8 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -491,9 +491,9 @@ The flag table below covers `create-agent`: | `--auto-run-folder ` | Auto Run / playbooks folder for this agent | `/.maestro/playbooks` | | `--json` | Machine-readable JSON output | - | -### Creating and Removing Groups +### Creating, Updating, and Removing Groups -Manage Left Bar groups from the command line. Requires the Maestro desktop app to be running. Use a group ID with `create-agent -g` or `update-agent --group` to place agents into it, and `update-agent --group none` to move an agent back out. +Manage Left Bar groups from the command line, including their appearance and nesting, so a bootstrap script or CI job can reproduce a whole workspace layout without anyone clicking through the desktop UI. Requires the Maestro desktop app to be running. Use a group ID with `create-agent -g` or `update-agent --group` to place agents into it, and `update-agent --group none` to move an agent back out. ```bash # Create a group @@ -502,27 +502,79 @@ maestro-cli create-group "Backend" # Create a group with an emoji icon maestro-cli create-group "Backend" -e 🔧 -# Machine-readable output (returns the new group ID) +# Create a group with a built-in icon and a label color +maestro-cli create-group "Backend" --icon rocket --color '#EF4444' + +# Create a group nested inside a root group +maestro-cli create-group "API" --parent + +# Machine-readable output (returns the new group ID and its stored appearance) maestro-cli create-group "Backend" --json +# Change a group's name, icon, and color +maestro-cli update-group --name "Frontend" --icon layers --color '#3B82F6' + +# Move a group inside a root group, or promote it back to the top level +maestro-cli update-group --parent +maestro-cli update-group --clear-parent + +# Remove appearance you set earlier +maestro-cli update-group --clear-icon --clear-color + # Remove an (empty) group maestro-cli remove-group # Remove a group that still has agents (ungroups them first) maestro-cli remove-group --force -# Rename a group +# Rename a group (kept for backward compatibility; update-group --name does the same) maestro-cli rename-group "Frontend" ``` Removing a group never deletes the agents inside it: the desktop ungroups any members (moves them to no group) and then removes the group. `remove-group` refuses a non-empty group unless you pass `--force`, so you don't accidentally scatter a populated group. Group IDs support partial-ID resolution. +#### Group appearance + +`--emoji` and `--icon` are mutually exclusive: a group shows one or the other. `--color` combines with either, and it is also fine on its own. + +Built-in icon IDs: `folder`, `briefcase`, `rocket`, `code`, `star`, `heart`, `lightbulb`, `target`, `calendar`, `book`, `layers`, `shield`, `wrench`, `palette`, `archive`, `zap`. Icons contributed by a plugin use their namespaced ID (`my-plugin/my-pack/my-icon`) and round-trip unchanged. + +Colors are `#RRGGBB` hex values, normalized to uppercase before they are stored, or a plugin's namespaced color ID. + +Notes on behavior: + +- **Nothing half-applies.** Every flag is validated before the command talks to the desktop, so an invalid icon or color leaves the group exactly as it was. +- **Clearing is explicit.** Passing `--icon` never silently drops an emoji you set earlier, and vice versa; use the `--clear-*` flags to remove a value. `--clear-emoji` restores the default folder emoji. `--clear-parent` promotes a nested group to the top level. +- **Writes are verified.** After the desktop reports success, the CLI reads the group back from storage and confirms it matches what you asked for. If a desktop app older than your CLI accepted the command and ignored the icon or color, the command fails with a version-mismatch message instead of reporting a success that did not happen. +- **Icon and color survive the Groups+ feature gate.** Turning Groups+ off falls back to the legacy emoji presentation but does not discard stored icon and color; turning it back on restores them. +- Group nesting is one level deep: a root group can hold child groups, but a child group cannot hold its own children. + +`maestro-cli list groups --json` reports `icon`, `color`, and `parentGroupId` alongside the existing fields, so a script can read back exactly what it set. + `create-group` flags: -| Flag | Description | Default | -| --------------------- | ---------------------------- | ------- | -| `-e, --emoji ` | Emoji icon for the group | - | -| `--json` | Machine-readable JSON output | - | +| Flag | Description | Default | +| --------------------- | ----------------------------------------------------------------------- | ------- | +| `-e, --emoji ` | Emoji icon for the group. Mutually exclusive with `--icon` | - | +| `--icon ` | Built-in icon ID or a plugin icon ID. Mutually exclusive with `--emoji` | - | +| `--color ` | Label color as `#RRGGBB`, or a plugin color ID | - | +| `--parent ` | Create inside this root group | - | +| `--json` | Machine-readable JSON output | - | + +`update-group` flags: + +| Flag | Description | Default | +| --------------------- | ----------------------------------------------------------------------- | ------- | +| `-n, --name ` | New group name | - | +| `-e, --emoji ` | Emoji icon for the group. Mutually exclusive with `--icon` | - | +| `--icon ` | Built-in icon ID or a plugin icon ID. Mutually exclusive with `--emoji` | - | +| `--color ` | Label color as `#RRGGBB`, or a plugin color ID | - | +| `--parent ` | Move the group inside this root group | - | +| `--clear-emoji` | Reset the emoji to the default folder | - | +| `--clear-icon` | Remove the icon | - | +| `--clear-color` | Remove the label color | - | +| `--clear-parent` | Promote the group to the top level | - | +| `--json` | Machine-readable JSON output | - | `remove-group` flags: diff --git a/src/__tests__/cli/commands/create-group.test.ts b/src/__tests__/cli/commands/create-group.test.ts index de87e05ad6..a193044546 100644 --- a/src/__tests__/cli/commands/create-group.test.ts +++ b/src/__tests__/cli/commands/create-group.test.ts @@ -4,12 +4,20 @@ */ import { describe, it, expect, vi, beforeEach, type MockInstance } from 'vitest'; +import type { Group } from '../../../shared/types'; // Mock maestro-client vi.mock('../../../cli/services/maestro-client', () => ({ withMaestroClient: vi.fn(), })); +vi.mock('../../../cli/services/storage', () => ({ + resolveGroupId: vi.fn((id: string) => id), + resolveAgentId: vi.fn((id: string) => id), + readActiveAgentId: vi.fn(() => undefined), + readGroups: vi.fn(() => [] as Group[]), +})); + // Mock formatter vi.mock('../../../cli/output/formatter', () => ({ formatError: vi.fn((msg) => `Error: ${msg}`), @@ -18,14 +26,38 @@ vi.mock('../../../cli/output/formatter', () => ({ import { createGroup } from '../../../cli/commands/create-group'; import { withMaestroClient } from '../../../cli/services/maestro-client'; +import { readGroups, resolveGroupId } from '../../../cli/services/storage'; import { formatError, formatSuccess } from '../../../cli/output/formatter'; +/** Capture the payload the command sends, and reply with `result`. */ +function mockSend(result: Record) { + let captured: Record = {}; + vi.mocked(withMaestroClient).mockImplementation(async (action) => + action({ + sendCommand: vi.fn().mockImplementation((payload: Record) => { + captured = payload; + return Promise.resolve(result); + }), + } as never) + ); + return () => captured; +} + +/** Pretend the desktop persisted this group, so the readback check passes. */ +function persisted(group: Partial & { id: string }): void { + vi.mocked(readGroups).mockReturnValue([ + { name: 'GROUP', emoji: '\u{1F4C2}', collapsed: false, ...group } as Group, + ]); +} + describe('create-group command', () => { let consoleSpy: MockInstance; let processExitSpy: MockInstance; beforeEach(() => { vi.clearAllMocks(); + vi.mocked(resolveGroupId).mockImplementation((id: string) => id); + vi.mocked(readGroups).mockReturnValue([]); consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(console, 'error').mockImplementation(() => {}); processExitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); @@ -33,93 +65,97 @@ describe('create-group command', () => { describe('successful creation', () => { it('should create a group with just a name', async () => { - let sentPayload: Record = {}; - vi.mocked(withMaestroClient).mockImplementation(async (action) => { - const mockClient = { - sendCommand: vi.fn().mockImplementation((payload) => { - sentPayload = payload; - return Promise.resolve({ - type: 'create_group_result', - success: true, - groupId: 'group-id-123', - }); - }), - }; - return action(mockClient as never); + const payload = mockSend({ + type: 'create_group_result', + success: true, + groupId: 'group-id-123', }); + persisted({ id: 'group-id-123', name: 'MY GROUP' }); await createGroup('My Group', {}); - expect(sentPayload.type).toBe('create_group'); - expect(sentPayload.name).toBe('My Group'); - expect(sentPayload.emoji).toBeUndefined(); - expect(sentPayload).not.toHaveProperty('parentGroupId'); + expect(payload().type).toBe('create_group'); + expect(payload().name).toBe('My Group'); + expect(payload().emoji).toBeUndefined(); + expect(payload()).not.toHaveProperty('parentGroupId'); expect(formatSuccess).toHaveBeenCalledWith('Created group "My Group"'); expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('group-id-123')); expect(processExitSpy).not.toHaveBeenCalled(); }); it('should send emoji when provided', async () => { - let sentPayload: Record = {}; - vi.mocked(withMaestroClient).mockImplementation(async (action) => { - const mockClient = { - sendCommand: vi.fn().mockImplementation((payload) => { - sentPayload = payload; - return Promise.resolve({ - type: 'create_group_result', - success: true, - groupId: 'id-1', - }); - }), - }; - return action(mockClient as never); - }); + const payload = mockSend({ type: 'create_group_result', success: true, groupId: 'id-1' }); + persisted({ id: 'id-1', name: 'TEAM', emoji: '🚀' }); await createGroup('Team', { emoji: '🚀' }); - expect(sentPayload.emoji).toBe('🚀'); - }); - - it('should send parent group when provided', async () => { - let sentPayload: Record = {}; - vi.mocked(withMaestroClient).mockImplementation(async (action) => { - const mockClient = { - sendCommand: vi.fn().mockImplementation((payload) => { - sentPayload = payload; - return Promise.resolve({ - type: 'create_group_result', - success: true, - groupId: 'id-1', - }); - }), - }; - return action(mockClient as never); - }); + expect(payload().emoji).toBe('🚀'); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should send a built-in icon and normalize its case', async () => { + const payload = mockSend({ type: 'create_group_result', success: true, groupId: 'id-1' }); + persisted({ id: 'id-1', name: 'TEAM', icon: 'rocket' }); + + await createGroup('Team', { icon: 'Rocket' }); + + expect(payload().icon).toBe('rocket'); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should uppercase a hex color before sending it', async () => { + const payload = mockSend({ type: 'create_group_result', success: true, groupId: 'id-1' }); + persisted({ id: 'id-1', name: 'TEAM', color: '#EF4444' }); + + await createGroup('Team', { color: '#ef4444' }); + + expect(payload().color).toBe('#EF4444'); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should accept a plugin-namespaced icon id', async () => { + const payload = mockSend({ type: 'create_group_result', success: true, groupId: 'id-1' }); + persisted({ id: 'id-1', name: 'TEAM', icon: 'my-plugin/my-pack/my-icon' }); + + await createGroup('Team', { icon: 'my-plugin/my-pack/my-icon' }); + + expect(payload().icon).toBe('my-plugin/my-pack/my-icon'); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should combine an icon with a color', async () => { + const payload = mockSend({ type: 'create_group_result', success: true, groupId: 'id-1' }); + persisted({ id: 'id-1', name: 'TEAM', icon: 'shield', color: '#22C55E' }); + + await createGroup('Team', { icon: 'shield', color: '#22c55e' }); + + expect(payload().icon).toBe('shield'); + expect(payload().color).toBe('#22C55E'); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should resolve a partial parent group ID', async () => { + const payload = mockSend({ type: 'create_group_result', success: true, groupId: 'id-1' }); + vi.mocked(resolveGroupId).mockReturnValue('group-company-full'); + persisted({ id: 'id-1', name: 'PROJECT', parentGroupId: 'group-company-full' }); await createGroup('Project', { parent: 'company' }); - expect(sentPayload.parentGroupId).toBe('company'); + expect(resolveGroupId).toHaveBeenCalledWith('company'); + expect(payload().parentGroupId).toBe('group-company-full'); + expect(processExitSpy).not.toHaveBeenCalled(); }); - it('should output JSON when --json flag is set', async () => { - vi.mocked(withMaestroClient).mockImplementation(async (action) => { - const mockClient = { - sendCommand: vi.fn().mockResolvedValue({ - type: 'create_group_result', - success: true, - groupId: 'json-id', - }), - }; - return action(mockClient as never); - }); + it('should output the persisted group when --json is set', async () => { + mockSend({ type: 'create_group_result', success: true, groupId: 'json-id' }); + persisted({ id: 'json-id', name: 'JSON GROUP', icon: 'star', color: '#3B82F6' }); - await createGroup('JSON Group', { json: true }); + await createGroup('JSON Group', { json: true, icon: 'star', color: '#3b82f6' }); - const output = consoleSpy.mock.calls[0][0]; - const parsed = JSON.parse(output); + const parsed = JSON.parse(consoleSpy.mock.calls[0][0]); expect(parsed.success).toBe(true); expect(parsed.groupId).toBe('json-id'); - expect(parsed.name).toBe('JSON Group'); + expect(parsed.group).toMatchObject({ icon: 'star', color: '#3B82F6' }); }); }); @@ -134,24 +170,80 @@ describe('create-group command', () => { it('should reject an empty name in JSON mode', async () => { await createGroup('', { json: true }); - const output = consoleSpy.mock.calls[0][0]; - const parsed = JSON.parse(output); + const parsed = JSON.parse(consoleSpy.mock.calls[0][0]); expect(parsed.success).toBe(false); expect(parsed.error).toContain('must not be empty'); }); + + it('should reject --emoji and --icon together', async () => { + await createGroup('Team', { emoji: '🚀', icon: 'rocket' }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('not both')); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + + it('should reject an unknown icon before sending anything', async () => { + await createGroup('Team', { icon: 'not-an-icon' }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('Unknown icon')); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + + it('should reject a malformed color before sending anything', async () => { + await createGroup('Team', { color: 'reddish' }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('Invalid color')); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + + it('should fail without sending when the parent cannot be resolved', async () => { + vi.mocked(resolveGroupId).mockImplementation(() => { + throw new Error('Group not found: nope'); + }); + + await createGroup('Project', { parent: 'nope' }); + + expect(formatError).toHaveBeenCalledWith('Group not found: nope'); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + }); + + describe('version-mismatch guard', () => { + it('should fail when the desktop reported success but stored no icon', async () => { + mockSend({ type: 'create_group_result', success: true, groupId: 'id-1' }); + // An older desktop accepts create_group and drops the icon field. + persisted({ id: 'id-1', name: 'TEAM' }); + + await createGroup('Team', { icon: 'rocket' }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('not stored as requested')); + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('older than this CLI')); + expect(processExitSpy).toHaveBeenCalledWith(1); + }); + + it('should fail when the group is missing entirely after the write', async () => { + mockSend({ type: 'create_group_result', success: true, groupId: 'id-1' }); + vi.mocked(readGroups).mockReturnValue([]); + + await createGroup('Team', {}); + + expect(formatError).toHaveBeenCalledWith( + expect.stringContaining('was not found after the write') + ); + expect(processExitSpy).toHaveBeenCalledWith(1); + }); }); describe('error handling', () => { it('should handle server returning failure', async () => { - vi.mocked(withMaestroClient).mockImplementation(async (action) => { - const mockClient = { - sendCommand: vi.fn().mockResolvedValue({ - type: 'create_group_result', - success: false, - error: 'Group creation not configured', - }), - }; - return action(mockClient as never); + mockSend({ + type: 'create_group_result', + success: false, + error: 'Group creation not configured', }); await createGroup('Nope', {}); @@ -174,8 +266,7 @@ describe('create-group command', () => { await createGroup('No App', { json: true }); - const output = consoleSpy.mock.calls[0][0]; - const parsed = JSON.parse(output); + const parsed = JSON.parse(consoleSpy.mock.calls[0][0]); expect(parsed.success).toBe(false); expect(parsed.error).toBe('Connection refused'); }); diff --git a/src/__tests__/cli/commands/list-groups.test.ts b/src/__tests__/cli/commands/list-groups.test.ts index 4bd7e8210a..4ced22f970 100644 --- a/src/__tests__/cli/commands/list-groups.test.ts +++ b/src/__tests__/cli/commands/list-groups.test.ts @@ -149,6 +149,24 @@ describe('list-groups command', () => { expect(JSON.parse(output)[1]).toMatchObject({ parentGroupId: 'company' }); }); + it('includes icon and color so automation can read appearance back', () => { + vi.mocked(readGroups).mockReturnValue([ + { + id: 'group-1', + name: 'Test Group', + emoji: '🔧', + icon: 'rocket', + color: '#EF4444', + collapsed: false, + }, + ]); + + listGroups({ json: true }); + + const output = consoleSpy.mock.calls[0][0]; + expect(JSON.parse(output)[0]).toMatchObject({ icon: 'rocket', color: '#EF4444' }); + }); + it('should output empty JSON array for no groups', () => { vi.mocked(readGroups).mockReturnValue([]); diff --git a/src/__tests__/cli/commands/update-group.test.ts b/src/__tests__/cli/commands/update-group.test.ts new file mode 100644 index 0000000000..30ee369f35 --- /dev/null +++ b/src/__tests__/cli/commands/update-group.test.ts @@ -0,0 +1,240 @@ +/** + * @file update-group.test.ts + * @description Tests for the update-group CLI command: flag parsing, explicit + * clearing, reparenting, and the readback guard that stops the command + * reporting success when the desktop silently ignored a field. + */ + +import { describe, it, expect, vi, beforeEach, type MockInstance } from 'vitest'; +import type { Group } from '../../../shared/types'; + +vi.mock('../../../cli/services/maestro-client', () => ({ withMaestroClient: vi.fn() })); +vi.mock('../../../cli/services/storage', () => ({ + resolveGroupId: vi.fn((id: string) => id), + resolveAgentId: vi.fn((id: string) => id), + readActiveAgentId: vi.fn(() => undefined), + readGroups: vi.fn(() => [] as Group[]), +})); +vi.mock('../../../cli/output/formatter', () => ({ + formatError: vi.fn((msg) => `Error: ${msg}`), + formatSuccess: vi.fn((msg) => `Success: ${msg}`), +})); + +import { updateGroup } from '../../../cli/commands/update-group'; +import { withMaestroClient } from '../../../cli/services/maestro-client'; +import { readGroups, resolveGroupId } from '../../../cli/services/storage'; +import { formatError, formatSuccess } from '../../../cli/output/formatter'; + +function mockSend(result: Record) { + let captured: Record = {}; + vi.mocked(withMaestroClient).mockImplementation(async (action) => + action({ + sendCommand: vi.fn().mockImplementation((payload: Record) => { + captured = payload; + return Promise.resolve(result); + }), + } as never) + ); + return () => captured; +} + +function persisted(group: Partial & { id: string }): void { + vi.mocked(readGroups).mockReturnValue([ + { name: 'TEAM', emoji: '\u{1F4C2}', collapsed: false, ...group } as Group, + ]); +} + +describe('update-group command', () => { + let consoleSpy: MockInstance; + let processExitSpy: MockInstance; + + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(resolveGroupId).mockImplementation((id: string) => id); + vi.mocked(readGroups).mockReturnValue([]); + consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + processExitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); + }); + + describe('setting fields', () => { + it('should send a rename as update_group', async () => { + const payload = mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1', name: 'NEW NAME' }); + + await updateGroup('g1', { name: 'New Name' }); + + expect(payload().type).toBe('update_group'); + expect(payload().groupId).toBe('g1'); + expect(payload().name).toBe('New Name'); + expect(formatSuccess).toHaveBeenCalledWith('Updated group g1'); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should normalize icon case and color case before sending', async () => { + const payload = mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1', icon: 'briefcase', color: '#A855F7' }); + + await updateGroup('g1', { icon: 'Briefcase', color: '#a855f7' }); + + expect(payload().icon).toBe('briefcase'); + expect(payload().color).toBe('#A855F7'); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should resolve a partial parent group ID', async () => { + const payload = mockSend({ type: 'update_group_result', success: true }); + vi.mocked(resolveGroupId).mockImplementation((id: string) => + id === 'comp' ? 'group-company' : id + ); + persisted({ id: 'g1', parentGroupId: 'group-company' }); + + await updateGroup('g1', { parent: 'comp' }); + + expect(payload().parentGroupId).toBe('group-company'); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should output the persisted group when --json is set', async () => { + mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1', name: 'TEAM', icon: 'zap', color: '#EAB308' }); + + await updateGroup('g1', { json: true, icon: 'zap', color: '#eab308' }); + + const parsed = JSON.parse(consoleSpy.mock.calls[0][0]); + expect(parsed.success).toBe(true); + expect(parsed.groupId).toBe('g1'); + expect(parsed.group).toMatchObject({ icon: 'zap', color: '#EAB308' }); + }); + }); + + describe('clearing fields', () => { + it('should send an explicit clear list', async () => { + const payload = mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1' }); + + await updateGroup('g1', { clearIcon: true, clearColor: true }); + + expect(payload().clear).toEqual(['icon', 'color']); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should promote to top level when --clear-parent is passed', async () => { + const payload = mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1' }); + + await updateGroup('g1', { clearParent: true }); + + expect(payload().clear).toEqual(['parent']); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should accept a cleared emoji that fell back to the default folder', async () => { + mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1', emoji: '\u{1F4C2}' }); + + await updateGroup('g1', { clearEmoji: true }); + + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should fail when a cleared field is still stored', async () => { + mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1', color: '#EF4444' }); + + await updateGroup('g1', { clearColor: true }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('color is still #EF4444')); + expect(processExitSpy).toHaveBeenCalledWith(1); + }); + }); + + describe('validation errors', () => { + it('should reject an update that changes nothing', async () => { + await updateGroup('g1', {}); + + expect(formatError).toHaveBeenCalledWith('Nothing to update'); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + + it('should reject --emoji together with --icon', async () => { + await updateGroup('g1', { emoji: '🚀', icon: 'rocket' }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('not both')); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + + it('should reject setting and clearing the same field', async () => { + await updateGroup('g1', { color: '#EF4444', clearColor: true }); + + expect(formatError).toHaveBeenCalledWith('Cannot both set and clear color'); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + + it('should reject an unknown icon before sending anything', async () => { + await updateGroup('g1', { icon: 'sparkle-pony' }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('Unknown icon')); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + + it('should fail when the group ID cannot be resolved', async () => { + vi.mocked(resolveGroupId).mockImplementation(() => { + throw new Error('Group not found: zz'); + }); + + await updateGroup('zz', { name: 'X' }); + + expect(formatError).toHaveBeenCalledWith('Group not found: zz'); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(withMaestroClient).not.toHaveBeenCalled(); + }); + }); + + describe('version-mismatch guard', () => { + it('should fail when the desktop reported success but stored no color', async () => { + mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1' }); + + await updateGroup('g1', { color: '#EF4444' }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('not stored as requested')); + expect(processExitSpy).toHaveBeenCalledWith(1); + }); + + it('should fail when the reparent did not take', async () => { + mockSend({ type: 'update_group_result', success: true }); + persisted({ id: 'g1' }); + + await updateGroup('g1', { parent: 'group-company' }); + + expect(formatError).toHaveBeenCalledWith(expect.stringContaining('parent is (top level)')); + expect(processExitSpy).toHaveBeenCalledWith(1); + }); + }); + + describe('error handling', () => { + it('should report a desktop-side rejection', async () => { + mockSend({ type: 'update_group_result', success: false, error: 'Group not found' }); + + await updateGroup('g1', { name: 'X' }); + + expect(formatError).toHaveBeenCalledWith('Group not found'); + expect(processExitSpy).toHaveBeenCalledWith(1); + }); + + it('should report a connection failure in JSON mode', async () => { + vi.mocked(withMaestroClient).mockRejectedValue(new Error('Connection refused')); + + await updateGroup('g1', { name: 'X', json: true }); + + const parsed = JSON.parse(consoleSpy.mock.calls[0][0]); + expect(parsed.success).toBe(false); + expect(parsed.error).toBe('Connection refused'); + }); + }); +}); diff --git a/src/__tests__/main/preload/process/groupCrudRemote.test.ts b/src/__tests__/main/preload/process/groupCrudRemote.test.ts index b5cb2c1ffc..846c6bb9b4 100644 --- a/src/__tests__/main/preload/process/groupCrudRemote.test.ts +++ b/src/__tests__/main/preload/process/groupCrudRemote.test.ts @@ -25,26 +25,65 @@ describe('Process GroupCrudRemote Preload API', () => { }); describe('onRemoteCreateGroup', () => { - it('forwards a parent group ID in its fixed IPC argument position', () => { - const callback = vi.fn(); - let registeredHandler: ( - event: unknown, - name: string, - emoji: string | undefined, - parentGroupId: string | undefined, - responseChannel: string - ) => void; - - mockOn.mockImplementation((channel: string, handler: typeof registeredHandler) => { - if (channel === 'remote:createGroup') { - registeredHandler = handler; - } + /** Grab the handler the API registered for `channel`. */ + function registerFor(channel: string): (...args: any[]) => void { + let registered: ((...args: any[]) => void) | undefined; + mockOn.mockImplementation((ch: string, handler: (...args: any[]) => void) => { + if (ch === channel) registered = handler; }); + return (...args: any[]) => registered!(...args); + } + + it('forwards a parent group ID and appearance in their fixed IPC argument positions', () => { + const callback = vi.fn(); + const fire = registerFor('remote:createGroup'); + + api.onRemoteCreateGroup(callback); + fire({}, 'Project', '📁', 'company', { icon: 'rocket' }, 'response-channel'); + + expect(callback).toHaveBeenCalledWith( + 'Project', + '📁', + 'company', + { icon: 'rocket' }, + 'response-channel' + ); + }); + + it('substitutes an empty appearance when an older main process omits it', () => { + const callback = vi.fn(); + const fire = registerFor('remote:createGroup'); api.onRemoteCreateGroup(callback); - registeredHandler!({}, 'Project', '📁', 'company', 'response-channel'); + fire({}, 'Project', '📁', 'company', undefined, 'response-channel'); + + expect(callback).toHaveBeenCalledWith('Project', '📁', 'company', {}, 'response-channel'); + }); + }); + + describe('onRemoteUpdateGroup', () => { + it('forwards the group ID and update payload', () => { + const callback = vi.fn(); + let registered: ((...args: any[]) => void) | undefined; + mockOn.mockImplementation((ch: string, handler: (...args: any[]) => void) => { + if (ch === 'remote:updateGroup') registered = handler; + }); + + api.onRemoteUpdateGroup(callback); + registered!({}, 'group-1', { icon: 'shield', clear: ['color'] }, 'response-channel'); + + expect(callback).toHaveBeenCalledWith( + 'group-1', + { icon: 'shield', clear: ['color'] }, + 'response-channel' + ); + }); + + it('unsubscribes from the IPC channel', () => { + const unsubscribe = api.onRemoteUpdateGroup(vi.fn()); + unsubscribe(); - expect(callback).toHaveBeenCalledWith('Project', '📁', 'company', 'response-channel'); + expect(mockRemoveListener).toHaveBeenCalledWith('remote:updateGroup', expect.any(Function)); }); }); }); diff --git a/src/__tests__/main/web-server/handlers/messageHandlers.test.ts b/src/__tests__/main/web-server/handlers/messageHandlers.test.ts index f221862cb3..44ecf69526 100644 --- a/src/__tests__/main/web-server/handlers/messageHandlers.test.ts +++ b/src/__tests__/main/web-server/handlers/messageHandlers.test.ts @@ -166,6 +166,7 @@ function createMockCallbacks(): MessageHandlerCallbacks { getGroups: vi.fn().mockReturnValue([]), createGroup: vi.fn().mockResolvedValue({ id: 'group-1' }), renameGroup: vi.fn().mockResolvedValue(true), + updateGroup: vi.fn().mockResolvedValue(true), deleteGroup: vi.fn().mockResolvedValue(true), moveSessionToGroup: vi.fn().mockResolvedValue(true), createSession: vi.fn().mockResolvedValue({ sessionId: 'new-session-1' }), @@ -3880,8 +3881,50 @@ describe('WebSocketMessageHandler', () => { }); await vi.waitFor(() => { - expect(callbacks.createGroup).toHaveBeenCalledWith('Project', '📁', 'company'); + expect(callbacks.createGroup).toHaveBeenCalledWith('Project', '📁', 'company', { + emoji: '📁', + }); + }); + }); + + it('forwards a normalized icon and color when creating a group', async () => { + handler.handleMessage(client, { + type: 'create_group', + name: 'Project', + icon: 'Rocket', + color: '#ef4444', }); + + await vi.waitFor(() => { + expect(callbacks.createGroup).toHaveBeenCalledWith('Project', undefined, undefined, { + icon: 'rocket', + color: '#EF4444', + }); + }); + }); + + it('rejects an unknown icon at the socket boundary', () => { + handler.handleMessage(client, { + type: 'create_group', + name: 'Project', + icon: 'sparkle-pony', + }); + + expect(callbacks.createGroup).not.toHaveBeenCalled(); + const payload = JSON.parse((client.socket.send as any).mock.calls[0][0]); + expect(payload.type).toBe('error'); + expect(payload.message).toContain('Unknown icon'); + }); + + it('rejects an emoji and an icon together at the socket boundary', () => { + handler.handleMessage(client, { + type: 'create_group', + name: 'Project', + emoji: '🚀', + icon: 'rocket', + }); + + expect(callbacks.createGroup).not.toHaveBeenCalled(); }); it('rejects non-string parentGroupId values instead of creating a root group', () => { @@ -3893,6 +3936,65 @@ describe('WebSocketMessageHandler', () => { expect(callbacks.createGroup).not.toHaveBeenCalled(); }); + + it('forwards a validated update_group request', async () => { + handler.handleMessage(client, { + type: 'update_group', + groupId: 'group-1', + name: 'Renamed', + icon: 'Shield', + color: '#22c55e', + requestId: 'request-2', + }); + + await vi.waitFor(() => { + expect(callbacks.updateGroup).toHaveBeenCalledWith('group-1', { + name: 'Renamed', + icon: 'shield', + color: '#22C55E', + }); + }); + }); + + it('forwards an explicit clear list on update_group', async () => { + handler.handleMessage(client, { + type: 'update_group', + groupId: 'group-1', + clear: ['icon', 'parent'], + }); + + await vi.waitFor(() => { + expect(callbacks.updateGroup).toHaveBeenCalledWith('group-1', { + clear: ['icon', 'parent'], + }); + }); + }); + + it('rejects an update_group with no groupId', () => { + handler.handleMessage(client, { type: 'update_group', name: 'Renamed' }); + + expect(callbacks.updateGroup).not.toHaveBeenCalled(); + const payload = JSON.parse((client.socket.send as any).mock.calls[0][0]); + expect(payload.message).toContain('groupId'); + }); + + it('rejects an update_group that changes nothing', () => { + handler.handleMessage(client, { type: 'update_group', groupId: 'group-1' }); + + expect(callbacks.updateGroup).not.toHaveBeenCalled(); + const payload = JSON.parse((client.socket.send as any).mock.calls[0][0]); + expect(payload.message).toContain('Nothing to update'); + }); + + it('rejects an update_group with an unknown clear target', () => { + handler.handleMessage(client, { + type: 'update_group', + groupId: 'group-1', + clear: ['collapsed'], + }); + + expect(callbacks.updateGroup).not.toHaveBeenCalled(); + }); }); }); diff --git a/src/__tests__/main/web-server/web-server-factory.test.ts b/src/__tests__/main/web-server/web-server-factory.test.ts index 0ce72d7ad8..befc546b69 100644 --- a/src/__tests__/main/web-server/web-server-factory.test.ts +++ b/src/__tests__/main/web-server/web-server-factory.test.ts @@ -85,6 +85,7 @@ vi.mock('../../../main/web-server/WebServer', () => { broadcastSettingsChanged = vi.fn(); setCreateGroupCallback = vi.fn(); setRenameGroupCallback = vi.fn(); + setUpdateGroupCallback = vi.fn(); setDeleteGroupCallback = vi.fn(); setMoveSessionToGroupCallback = vi.fn(); setCreateSessionCallback = vi.fn(); @@ -2104,13 +2105,46 @@ describe('web-server/web-server-factory', () => { const server = createWebServer() as any; const callback = server.setCreateGroupCallback.mock.calls[0][0]; - void callback('My Group', '🚀', null); + void callback('My Group', '🚀', null, { emoji: '🚀', color: '#EF4444' }); expect(mockWebContents.send).toHaveBeenCalledWith( 'remote:createGroup', 'My Group', '🚀', null, + { emoji: '🚀', color: '#EF4444' }, + expect.any(String) + ); + }); + + it('setCreateGroupCallback sends an empty appearance when none was requested', () => { + const createWebServer = createWebServerFactory(deps); + const server = createWebServer() as any; + const callback = server.setCreateGroupCallback.mock.calls[0][0]; + + void callback('My Group', undefined, undefined); + + expect(mockWebContents.send).toHaveBeenCalledWith( + 'remote:createGroup', + 'My Group', + undefined, + undefined, + {}, + expect.any(String) + ); + }); + + it('setUpdateGroupCallback forwards the update to the renderer', () => { + const createWebServer = createWebServerFactory(deps); + const server = createWebServer() as any; + const callback = server.setUpdateGroupCallback.mock.calls[0][0]; + + void callback('group-1', { icon: 'rocket', clear: ['color'] }); + + expect(mockWebContents.send).toHaveBeenCalledWith( + 'remote:updateGroup', + 'group-1', + { icon: 'rocket', clear: ['color'] }, expect.any(String) ); }); diff --git a/src/__tests__/renderer/hooks/remote/useAppRemoteEventListenersGroups.test.ts b/src/__tests__/renderer/hooks/remote/useAppRemoteEventListenersGroups.test.ts new file mode 100644 index 0000000000..87c9270605 --- /dev/null +++ b/src/__tests__/renderer/hooks/remote/useAppRemoteEventListenersGroups.test.ts @@ -0,0 +1,275 @@ +/** + * Covers the remote group create/update handlers in useAppRemoteEventListeners - + * the path `maestro-cli create-group` / `update-group` drive. + * + * The invariants under test: appearance is re-validated in the renderer (this + * listener is reachable from any WebSocket client, not just our CLI), the group + * list is flushed to disk before the ack (so a CLI readback is not racing the + * store's effect-driven persistence), setting an icon never silently discards + * the emoji the Groups+-disabled view falls back to, clearing is explicit, and + * an illegal reparent is refused instead of half-applied. + */ +import { renderHook } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import { useAppRemoteEventListeners } from '../../../../renderer/hooks/remote/useAppRemoteEventListeners'; +import type { Group } from '../../../../shared/types'; + +const storeState: { groups: Group[] } = { groups: [] }; + +vi.mock('../../../../renderer/stores/sessionStore', () => ({ + useSessionStore: Object.assign(vi.fn(), { getState: vi.fn(() => storeState) }), + selectSessionById: vi.fn(), +})); +vi.mock('../../../../renderer/stores/settingsStore', () => ({ + useSettingsStore: Object.assign(vi.fn(), { getState: vi.fn(() => ({})) }), +})); +vi.mock('../../../../renderer/hooks/batch/batchUtils', () => ({ DEFAULT_BATCH_PROMPT: '' })); +vi.mock('../../../../renderer/services/git', () => ({ gitService: {} })); +vi.mock('../../../../renderer/utils/worktreeSpawn', () => ({ + spawnWorktreeAgentAndDispatch: vi.fn(), +})); +vi.mock('../../../../renderer/stores/notificationStore', () => ({ notifyToast: vi.fn() })); +vi.mock('../../../../renderer/utils/browserTabPersistence', () => ({ + getBrowserTabPartition: () => 'persist:test', +})); +vi.mock('../../../../renderer/utils/ids', () => ({ generateId: () => 'new-group' })); + +const createAck = vi.fn(); +const updateAck = vi.fn(); +const setAll = vi.fn().mockResolvedValue(undefined); + +const DEFAULT_EMOJI = '\u{1F4C2}'; + +function setup() { + const setGroups = vi.fn(); + renderHook(() => + useAppRemoteEventListeners({ + sessionsRef: { current: [] }, + setActiveSessionId: vi.fn(), + setSessions: vi.fn(), + setGroups, + handleOpenFileTab: vi.fn(), + refreshFileTree: vi.fn(), + handleAutoRunRefresh: vi.fn(), + startBatchRun: vi.fn(), + stopBatchRun: vi.fn(), + resumeAfterError: vi.fn(), + skipCurrentDocument: vi.fn(), + abortBatchOnError: vi.fn(), + } as any) + ); + return { setGroups }; +} + +/** The group list the handler asked the store to hold. */ +function resultingGroups(setGroups: Mock, prev: Group[]): Group[] { + const arg = setGroups.mock.calls[0][0]; + return typeof arg === 'function' ? arg(prev) : arg; +} + +const flush = () => new Promise((resolve) => setTimeout(resolve, 0)); + +function dispatchCreate(detail: Record) { + window.dispatchEvent( + new CustomEvent('maestro:remoteCreateGroup', { + detail: { responseChannel: 'ch', ...detail }, + }) + ); +} + +function dispatchUpdate(groupId: string, update: Record) { + window.dispatchEvent( + new CustomEvent('maestro:remoteUpdateGroup', { + detail: { groupId, update, responseChannel: 'ch' }, + }) + ); +} + +function group(overrides: Partial & { id: string }): Group { + return { name: 'TEAM', emoji: DEFAULT_EMOJI, collapsed: false, ...overrides } as Group; +} + +beforeEach(() => { + vi.clearAllMocks(); + storeState.groups = []; + (window as any).maestro = { + process: { + sendRemoteCreateGroupResponse: createAck, + sendRemoteUpdateGroupResponse: updateAck, + kill: vi.fn().mockResolvedValue(undefined), + }, + groups: { setAll }, + sessions: { setMany: vi.fn().mockResolvedValue(undefined) }, + }; +}); + +describe('maestro:remoteCreateGroup', () => { + it('stores a normalized icon and color and flushes before acking', async () => { + const { setGroups } = setup(); + + dispatchCreate({ name: 'Team', appearance: { icon: 'rocket', color: '#EF4444' } }); + await flush(); + + const [created] = resultingGroups(setGroups, []); + expect(created).toMatchObject({ + id: 'group-new-group', + name: 'TEAM', + icon: 'rocket', + color: '#EF4444', + }); + expect(setAll).toHaveBeenCalled(); + expect(createAck).toHaveBeenCalledWith('ch', { id: 'group-new-group' }); + // The disk write has to happen before the ack, or a CLI readback races it. + expect(setAll.mock.invocationCallOrder[0]).toBeLessThan(createAck.mock.invocationCallOrder[0]); + }); + + it('falls back to the default emoji when no appearance is requested', async () => { + const { setGroups } = setup(); + + dispatchCreate({ name: 'Team', appearance: {} }); + await flush(); + + expect(resultingGroups(setGroups, [])[0].emoji).toBe(DEFAULT_EMOJI); + }); + + it('refuses an icon the picker cannot draw, even straight off the socket', async () => { + const { setGroups } = setup(); + + dispatchCreate({ name: 'Team', appearance: { icon: 'sparkle-pony' } }); + await flush(); + + expect(setGroups).not.toHaveBeenCalled(); + expect(createAck).toHaveBeenCalledWith('ch', null); + }); + + it('refuses an emoji and an icon together', async () => { + const { setGroups } = setup(); + + dispatchCreate({ name: 'Team', emoji: '🚀', appearance: { icon: 'rocket' } }); + await flush(); + + expect(setGroups).not.toHaveBeenCalled(); + expect(createAck).toHaveBeenCalledWith('ch', null); + }); +}); + +describe('maestro:remoteUpdateGroup', () => { + it('applies an icon and color and flushes before acking', async () => { + storeState.groups = [group({ id: 'g1' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { icon: 'shield', color: '#22C55E' }); + await flush(); + + expect(resultingGroups(setGroups, storeState.groups)[0]).toMatchObject({ + icon: 'shield', + color: '#22C55E', + }); + expect(updateAck).toHaveBeenCalledWith('ch', true); + expect(setAll.mock.invocationCallOrder[0]).toBeLessThan(updateAck.mock.invocationCallOrder[0]); + }); + + it('keeps the emoji when an icon is set, so the Groups+-off view still has a glyph', async () => { + storeState.groups = [group({ id: 'g1', emoji: '🚀' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { icon: 'shield' }); + await flush(); + + expect(resultingGroups(setGroups, storeState.groups)[0]).toMatchObject({ + emoji: '🚀', + icon: 'shield', + }); + }); + + it('uppercases a new name, matching the rename path', async () => { + storeState.groups = [group({ id: 'g1' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { name: 'Team Alpha' }); + await flush(); + + expect(resultingGroups(setGroups, storeState.groups)[0].name).toBe('TEAM ALPHA'); + }); + + it('removes icon and color on an explicit clear', async () => { + storeState.groups = [group({ id: 'g1', icon: 'shield', color: '#22C55E' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { clear: ['icon', 'color'] }); + await flush(); + + const updated = resultingGroups(setGroups, storeState.groups)[0]; + expect(updated).not.toHaveProperty('icon'); + expect(updated).not.toHaveProperty('color'); + }); + + it('restores the default folder when the emoji is cleared', async () => { + storeState.groups = [group({ id: 'g1', emoji: '🚀' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { clear: ['emoji'] }); + await flush(); + + expect(resultingGroups(setGroups, storeState.groups)[0].emoji).toBe(DEFAULT_EMOJI); + }); + + it('reparents a group under a root group', async () => { + storeState.groups = [group({ id: 'g1' }), group({ id: 'root', name: 'ROOT' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { parentGroupId: 'root' }); + await flush(); + + const updated = resultingGroups(setGroups, storeState.groups).find((g) => g.id === 'g1'); + expect(updated?.parentGroupId).toBe('root'); + expect(updateAck).toHaveBeenCalledWith('ch', true); + }); + + it('promotes a group to the top level on clear-parent', async () => { + storeState.groups = [group({ id: 'g1', parentGroupId: 'root' }), group({ id: 'root' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { clear: ['parent'] }); + await flush(); + + const updated = resultingGroups(setGroups, storeState.groups).find((g) => g.id === 'g1'); + expect(updated?.parentGroupId).toBeUndefined(); + }); + + it('refuses an illegal reparent without writing anything', async () => { + // Nesting is one level deep, so a group that already has a child cannot + // itself become a child. + storeState.groups = [group({ id: 'g1' }), group({ id: 'child', parentGroupId: 'g1' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { parentGroupId: 'child' }); + await flush(); + + expect(setGroups).not.toHaveBeenCalled(); + expect(setAll).not.toHaveBeenCalled(); + expect(updateAck).toHaveBeenCalledWith('ch', false); + }); + + it('refuses an update to a group that no longer exists', async () => { + storeState.groups = []; + const { setGroups } = setup(); + + dispatchUpdate('gone', { icon: 'shield' }); + await flush(); + + expect(setGroups).not.toHaveBeenCalled(); + expect(updateAck).toHaveBeenCalledWith('ch', false); + }); + + it('refuses an update that sets and clears the same field', async () => { + storeState.groups = [group({ id: 'g1' })]; + const { setGroups } = setup(); + + dispatchUpdate('g1', { icon: 'shield', clear: ['icon'] }); + await flush(); + + expect(setGroups).not.toHaveBeenCalled(); + expect(updateAck).toHaveBeenCalledWith('ch', false); + }); +}); diff --git a/src/__tests__/renderer/hooks/useRemoteIntegration.test.ts b/src/__tests__/renderer/hooks/useRemoteIntegration.test.ts index cb73cec0da..eaeeec2ae7 100644 --- a/src/__tests__/renderer/hooks/useRemoteIntegration.test.ts +++ b/src/__tests__/renderer/hooks/useRemoteIntegration.test.ts @@ -273,6 +273,10 @@ describe('useRemoteIntegration', () => { return () => {}; }), sendRemoteRenameGroupResponse: vi.fn(), + onRemoteUpdateGroup: vi.fn().mockImplementation(() => { + return () => {}; + }), + sendRemoteUpdateGroupResponse: vi.fn(), onRemoteDeleteGroup: vi.fn().mockImplementation(() => { return () => {}; }), diff --git a/src/__tests__/shared/groupAppearance.test.ts b/src/__tests__/shared/groupAppearance.test.ts new file mode 100644 index 0000000000..22bb62de99 --- /dev/null +++ b/src/__tests__/shared/groupAppearance.test.ts @@ -0,0 +1,165 @@ +/** + * @file groupAppearance.test.ts + * @description Tests for the shared group appearance catalog: normalization, + * validation, and the update-request rules that both the CLI and the WebSocket + * message handlers rely on. + */ + +import { describe, it, expect } from 'vitest'; +import { + GROUP_ICON_CATALOG, + GROUP_ICON_IDS, + GROUP_LABEL_COLORS, + normalizeGroupColor, + normalizeGroupIconId, + validateGroupAppearance, + validateGroupUpdate, +} from '../../shared/groupAppearance'; + +describe('group appearance catalog', () => { + it('exposes the documented built-in icon ids', () => { + expect(GROUP_ICON_IDS).toEqual([ + 'folder', + 'briefcase', + 'rocket', + 'code', + 'star', + 'heart', + 'lightbulb', + 'target', + 'calendar', + 'book', + 'layers', + 'shield', + 'wrench', + 'palette', + 'archive', + 'zap', + ]); + }); + + it('labels every icon', () => { + expect(GROUP_ICON_CATALOG.every((entry) => entry.label.length > 0)).toBe(true); + }); + + it('stores every built-in color as an uppercase hex value', () => { + for (const color of GROUP_LABEL_COLORS) { + expect(color.value).toMatch(/^#[0-9A-F]{6}$/); + } + }); +}); + +describe('normalizeGroupIconId', () => { + it('accepts a built-in id regardless of case or padding', () => { + expect(normalizeGroupIconId(' Rocket ')).toBe('rocket'); + }); + + it('accepts a plugin-namespaced id', () => { + expect(normalizeGroupIconId('my-plugin/my-pack/my-icon')).toBe('my-plugin/my-pack/my-icon'); + }); + + it('rejects an unknown bare id', () => { + expect(normalizeGroupIconId('sparkle-pony')).toBeNull(); + }); + + it('rejects a namespaced id with an empty segment', () => { + expect(normalizeGroupIconId('my-plugin//my-icon')).toBeNull(); + }); + + it('rejects an empty string', () => { + expect(normalizeGroupIconId(' ')).toBeNull(); + }); +}); + +describe('normalizeGroupColor', () => { + it('uppercases a hex value', () => { + expect(normalizeGroupColor('#ef4444')).toBe('#EF4444'); + }); + + it('accepts a plugin-namespaced color id', () => { + expect(normalizeGroupColor('my-plugin/my-pack/brand')).toBe('my-plugin/my-pack/brand'); + }); + + it('rejects a three-digit hex value', () => { + expect(normalizeGroupColor('#f44')).toBeNull(); + }); + + it('rejects a CSS color name', () => { + expect(normalizeGroupColor('red')).toBeNull(); + }); +}); + +describe('validateGroupAppearance', () => { + it('returns only the supplied fields', () => { + const result = validateGroupAppearance({ icon: 'star' }); + expect(result).toEqual({ ok: true, value: { icon: 'star' } }); + }); + + it('rejects emoji and icon together', () => { + const result = validateGroupAppearance({ emoji: '🚀', icon: 'rocket' }); + expect(result.ok).toBe(false); + }); + + it('allows a color alongside an emoji', () => { + const result = validateGroupAppearance({ emoji: '🚀', color: '#22c55e' }); + expect(result).toEqual({ ok: true, value: { emoji: '🚀', color: '#22C55E' } }); + }); + + it('allows a color alongside an icon', () => { + const result = validateGroupAppearance({ icon: 'shield', color: '#22c55e' }); + expect(result).toEqual({ ok: true, value: { icon: 'shield', color: '#22C55E' } }); + }); + + it('names the offending value in an icon error', () => { + const result = validateGroupAppearance({ icon: 'nope' }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain('"nope"'); + }); +}); + +describe('validateGroupUpdate', () => { + it('rejects an update with nothing in it', () => { + const result = validateGroupUpdate({}); + expect(result).toEqual({ ok: false, error: 'Nothing to update' }); + }); + + it('rejects setting and clearing the same field', () => { + const result = validateGroupUpdate({ icon: 'star', clear: ['icon'] }); + expect(result).toEqual({ ok: false, error: 'Cannot both set and clear icon' }); + }); + + it('allows clearing an emoji while setting an icon', () => { + const result = validateGroupUpdate({ icon: 'star', clear: ['emoji'] }); + expect(result).toEqual({ ok: true, value: { icon: 'star', clear: ['emoji'] } }); + }); + + it('rejects an explicitly empty name', () => { + const result = validateGroupUpdate({ name: ' ' }); + expect(result).toEqual({ ok: false, error: 'Group name must not be empty' }); + }); + + it('rejects an explicitly empty parent', () => { + const result = validateGroupUpdate({ parentGroupId: '' }); + expect(result).toEqual({ ok: false, error: 'Parent group ID must not be empty' }); + }); + + it('rejects an unknown clear target', () => { + const result = validateGroupUpdate({ clear: ['collapsed' as never] }); + expect(result.ok).toBe(false); + }); + + it('normalizes appearance values it passes through', () => { + const result = validateGroupUpdate({ icon: 'Rocket', color: '#a855f7' }); + expect(result).toEqual({ ok: true, value: { icon: 'rocket', color: '#A855F7' } }); + }); + + it('trims a name but leaves its case to the desktop', () => { + const result = validateGroupUpdate({ name: ' Team Alpha ' }); + expect(result).toEqual({ ok: true, value: { name: 'Team Alpha' } }); + }); + + it('carries a clear-parent request through', () => { + const result = validateGroupUpdate({ clear: ['parent'] }); + expect(result).toEqual({ ok: true, value: { clear: ['parent'] } }); + }); +}); diff --git a/src/cli/commands/create-group.ts b/src/cli/commands/create-group.ts index b3df99ea37..ee6180c782 100644 --- a/src/cli/commands/create-group.ts +++ b/src/cli/commands/create-group.ts @@ -1,66 +1,82 @@ // Create group command - create a new group in the Maestro desktop app -import { withMaestroClient } from '../services/maestro-client'; -import { formatError, formatSuccess } from '../output/formatter'; +import { resolveGroupId } from '../services/storage'; +import { sendSimpleCommand, failCommand } from '../services/session-command'; +import { verifyPersistedGroup, describePersistedGroup } from '../services/group-appearance'; +import { validateGroupAppearance } from '../../shared/groupAppearance'; +import { formatSuccess } from '../output/formatter'; +import { isQuiet } from '../output/verbosity'; interface CreateGroupOptions { emoji?: string; + icon?: string; + color?: string; parent?: string; json?: boolean; } export async function createGroup(name: string, options: CreateGroupOptions): Promise { if (!name || !name.trim()) { - const msg = 'Group name must not be empty'; - if (options.json) { - console.log(JSON.stringify({ success: false, error: msg })); - } else { - console.error(formatError(msg)); - } - process.exit(1); + return failCommand('Group name must not be empty', options.json); } - // Build the WebSocket message payload - const payload: Record = { - type: 'create_group', - name, - }; - if (options.emoji) payload.emoji = options.emoji; - if (options.parent) payload.parentGroupId = options.parent; + // Validate everything before the first byte goes over the wire, so a bad + // color can never leave a half-configured group behind. + const appearance = validateGroupAppearance({ + emoji: options.emoji, + icon: options.icon, + color: options.color, + }); + if (!appearance.ok) { + return failCommand(appearance.error, options.json); + } - try { - const result = await withMaestroClient(async (client) => { - return client.sendCommand<{ - type: string; - success: boolean; - groupId?: string; - error?: string; - }>(payload, 'create_group_result'); - }); + const payload: Record = { type: 'create_group', name }; + if (appearance.value.emoji) payload.emoji = appearance.value.emoji; + if (appearance.value.icon) payload.icon = appearance.value.icon; + if (appearance.value.color) payload.color = appearance.value.color; - if (result.success) { - if (options.json) { - console.log(JSON.stringify({ success: true, groupId: result.groupId, name })); - } else { - console.log(formatSuccess(`Created group "${name}"`)); - console.log(` ID: ${result.groupId}`); - } - } else { - const msg = result.error || 'Failed to create group'; - if (options.json) { - console.log(JSON.stringify({ success: false, error: msg })); - } else { - console.error(formatError(msg)); - } - process.exit(1); + let parentGroupId: string | undefined; + if (options.parent) { + try { + // Accept a partial group ID here for the same reason every other + // group verb does - a caller pasting a prefix should not get a + // generic "failed to create group" from the desktop. + parentGroupId = resolveGroupId(options.parent); + } catch (error) { + return failCommand(error instanceof Error ? error.message : String(error), options.json); } + payload.parentGroupId = parentGroupId; + } + + let result; + try { + result = await sendSimpleCommand(payload, 'create_group_result'); } catch (error) { - const msg = error instanceof Error ? error.message : String(error); - if (options.json) { - console.log(JSON.stringify({ success: false, error: msg })); - } else { - console.error(formatError(msg)); - } - process.exit(1); + return failCommand(error instanceof Error ? error.message : String(error), options.json); + } + + if (!result.success || !result.groupId) { + return failCommand(String(result.error || 'Failed to create group'), options.json); + } + + const groupId = String(result.groupId); + const mismatch = verifyPersistedGroup(groupId, { + name, + emoji: appearance.value.emoji, + icon: appearance.value.icon, + color: appearance.value.color, + parentGroupId, + }); + if (mismatch) { + return failCommand(mismatch, options.json); + } + + if (options.json) { + console.log(JSON.stringify({ success: true, groupId, group: describePersistedGroup(groupId) })); + return; } + if (isQuiet()) return; + console.log(formatSuccess(`Created group "${name}"`)); + console.log(` ID: ${groupId}`); } diff --git a/src/cli/commands/list-groups.ts b/src/cli/commands/list-groups.ts index 0dae38229e..25e1beb60f 100644 --- a/src/cli/commands/list-groups.ts +++ b/src/cli/commands/list-groups.ts @@ -18,6 +18,8 @@ export function listGroups(options: ListGroupsOptions): void { id: g.id, name: g.name, emoji: g.emoji, + icon: g.icon, + color: g.color, collapsed: g.collapsed, parentGroupId: g.parentGroupId, })); diff --git a/src/cli/commands/update-group.ts b/src/cli/commands/update-group.ts new file mode 100644 index 0000000000..b843f6c895 --- /dev/null +++ b/src/cli/commands/update-group.ts @@ -0,0 +1,105 @@ +// Update group command - change a group's name, appearance, or parent in the +// running desktop app via the update_group WS message. +// +// `rename-group` stays as-is for backward compatibility; this is the verb that +// covers everything the Left Bar's group editor can do, so a bootstrap script +// can reproduce a workspace's group structure and appearance without anyone +// clicking through the UI. + +import { resolveGroupId } from '../services/storage'; +import { sendSimpleCommand, failCommand } from '../services/session-command'; +import { verifyPersistedGroup, describePersistedGroup } from '../services/group-appearance'; +import { validateGroupUpdate, type GroupClearableField } from '../../shared/groupAppearance'; +import { formatSuccess } from '../output/formatter'; +import { isQuiet } from '../output/verbosity'; + +interface UpdateGroupOptions { + name?: string; + emoji?: string; + icon?: string; + color?: string; + parent?: string; + clearEmoji?: boolean; + clearIcon?: boolean; + clearColor?: boolean; + clearParent?: boolean; + json?: boolean; +} + +export async function updateGroup(groupId: string, options: UpdateGroupOptions): Promise { + let resolvedGroupId: string; + try { + resolvedGroupId = resolveGroupId(groupId); + } catch (error) { + return failCommand(error instanceof Error ? error.message : String(error), options.json); + } + + const clear: GroupClearableField[] = []; + if (options.clearEmoji) clear.push('emoji'); + if (options.clearIcon) clear.push('icon'); + if (options.clearColor) clear.push('color'); + if (options.clearParent) clear.push('parent'); + + let parentGroupId: string | undefined; + if (options.parent) { + try { + parentGroupId = resolveGroupId(options.parent); + } catch (error) { + return failCommand(error instanceof Error ? error.message : String(error), options.json); + } + } + + // Validate the whole request up front. The desktop validates again at the WS + // boundary (other clients speak the same protocol), but failing here means a + // bad flag never reaches the app at all, so there is nothing to half-apply. + const validated = validateGroupUpdate({ + ...(options.name !== undefined ? { name: options.name } : {}), + ...(options.emoji !== undefined ? { emoji: options.emoji } : {}), + ...(options.icon !== undefined ? { icon: options.icon } : {}), + ...(options.color !== undefined ? { color: options.color } : {}), + ...(parentGroupId ? { parentGroupId } : {}), + ...(clear.length > 0 ? { clear } : {}), + }); + if (!validated.ok) { + return failCommand(validated.error, options.json); + } + + let result; + try { + result = await sendSimpleCommand( + { type: 'update_group', groupId: resolvedGroupId, ...validated.value }, + 'update_group_result' + ); + } catch (error) { + return failCommand(error instanceof Error ? error.message : String(error), options.json); + } + + if (!result.success) { + return failCommand(String(result.error || 'Failed to update group'), options.json); + } + + const mismatch = verifyPersistedGroup(resolvedGroupId, { + name: validated.value.name, + emoji: validated.value.emoji, + icon: validated.value.icon, + color: validated.value.color, + parentGroupId: validated.value.parentGroupId, + cleared: clear, + }); + if (mismatch) { + return failCommand(mismatch, options.json); + } + + if (options.json) { + console.log( + JSON.stringify({ + success: true, + groupId: resolvedGroupId, + group: describePersistedGroup(resolvedGroupId), + }) + ); + return; + } + if (isQuiet()) return; + console.log(formatSuccess(`Updated group ${resolvedGroupId}`)); +} diff --git a/src/cli/index.ts b/src/cli/index.ts index a9134a880a..2f2888e4e0 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -83,6 +83,7 @@ import { import { stats, statsQuery } from './commands/stats'; import { renameAgent } from './commands/rename-agent'; import { renameGroup } from './commands/rename-group'; +import { updateGroup } from './commands/update-group'; import { stopAutoRun, resumeAutoRun, @@ -848,6 +849,11 @@ program .command('create-group ') .description('Create a new group in the Maestro desktop app') .option('-e, --emoji ', 'Emoji icon for the group') + .option( + '--icon ', + 'Built-in icon ID (folder, briefcase, rocket, ...) or a plugin icon ID. Mutually exclusive with --emoji' + ) + .option('--color ', 'Label color as #RRGGBB, or a plugin color ID') .option('--parent ', 'Create inside this root group') .option('--json', 'Output as JSON (for scripting)') .action(createGroup); @@ -870,6 +876,27 @@ program .option('--json', 'Output as JSON (for scripting)') .action((groupId, newName, options) => renameGroup(groupId, newName, options)); +// Update group command - change a group's name, appearance, or parent. Covers +// everything the Left Bar's group editor does; rename-group stays for +// backward compatibility. +program + .command('update-group ') + .description("Update a group's name, icon, color, or parent in the Maestro desktop app") + .option('-n, --name ', 'New group name') + .option('-e, --emoji ', 'Emoji icon for the group. Mutually exclusive with --icon') + .option( + '--icon ', + 'Built-in icon ID (folder, briefcase, rocket, ...) or a plugin icon ID. Mutually exclusive with --emoji' + ) + .option('--color ', 'Label color as #RRGGBB, or a plugin color ID') + .option('--parent ', 'Move the group inside this root group') + .option('--clear-emoji', 'Reset the emoji to the default folder') + .option('--clear-icon', 'Remove the icon') + .option('--clear-color', 'Remove the label color') + .option('--clear-parent', 'Promote the group to the top level') + .option('--json', 'Output as JSON (for scripting)') + .action((groupId, options) => updateGroup(groupId, options)); + // Create-worktree command - create a new agent in a git worktree off a parent // agent, without an Auto Run playbook. The parent agent must already exist in // the running desktop app. diff --git a/src/cli/services/group-appearance.ts b/src/cli/services/group-appearance.ts new file mode 100644 index 0000000000..f5293daec9 --- /dev/null +++ b/src/cli/services/group-appearance.ts @@ -0,0 +1,99 @@ +// Shared plumbing for the CLI verbs that write group appearance +// (`create-group`, `update-group`). +// +// The important piece here is `verifyPersistedGroup`. The desktop answers +// `{ success: true }` as soon as the renderer accepted the message, and an +// older desktop that does not know the `icon` / `color` fields accepts the +// message and drops them - reporting success for a change that never happened. +// So after every write we read `maestro-groups.json` back and compare the +// stored values against what was asked for. That catches a version mismatch, a +// silently ignored field, and a clear that did not take, all with one check, +// and it does not rely on the desktop echoing anything back to us. + +import { readGroups } from './storage'; +import type { Group } from '../../shared/types'; + +/** What the persisted group is expected to look like after a write. */ +export interface ExpectedGroupState { + name?: string; + emoji?: string; + icon?: string; + color?: string; + parentGroupId?: string; + /** Fields that must be absent (or, for emoji, back at the default) afterwards. */ + cleared?: readonly ('emoji' | 'icon' | 'color' | 'parent')[]; +} + +/** The default emoji the desktop assigns a group with no explicit one. */ +export const DEFAULT_GROUP_EMOJI = '\u{1F4C2}'; + +const VERSION_MISMATCH_HINT = + 'The running Maestro desktop app accepted the command but did not store it. This usually means the desktop app is older than this CLI and silently ignored the new fields - update the desktop app and retry.'; + +/** + * Read the group back from disk and confirm it matches what was requested. + * Returns `null` when everything matches, or the error text to fail with. + */ +export function verifyPersistedGroup(groupId: string, expected: ExpectedGroupState): string | null { + let stored: Group | undefined; + try { + stored = readGroups().find((group) => group.id === groupId); + } catch (error) { + return `Could not verify the group after writing it: ${ + error instanceof Error ? error.message : String(error) + }`; + } + + if (!stored) { + return `Group ${groupId} was not found after the write. ${VERSION_MISMATCH_HINT}`; + } + + const mismatches: string[] = []; + const cleared = new Set(expected.cleared ?? []); + + // The desktop upper-cases group names, so compare case-insensitively - + // otherwise every rename would look like a mismatch. + if (expected.name && stored.name.toUpperCase() !== expected.name.toUpperCase()) { + mismatches.push(`name is "${stored.name}", expected "${expected.name.toUpperCase()}"`); + } + if (expected.emoji && stored.emoji !== expected.emoji) { + mismatches.push(`emoji is ${stored.emoji || '(none)'}, expected ${expected.emoji}`); + } + if (expected.icon && stored.icon !== expected.icon) { + mismatches.push(`icon is ${stored.icon || '(none)'}, expected ${expected.icon}`); + } + if (expected.color && stored.color !== expected.color) { + mismatches.push(`color is ${stored.color || '(none)'}, expected ${expected.color}`); + } + if (expected.parentGroupId && stored.parentGroupId !== expected.parentGroupId) { + mismatches.push( + `parent is ${stored.parentGroupId || '(top level)'}, expected ${expected.parentGroupId}` + ); + } + + if (cleared.has('emoji') && stored.emoji && stored.emoji !== DEFAULT_GROUP_EMOJI) { + mismatches.push(`emoji is still ${stored.emoji}`); + } + if (cleared.has('icon') && stored.icon) mismatches.push(`icon is still ${stored.icon}`); + if (cleared.has('color') && stored.color) mismatches.push(`color is still ${stored.color}`); + if (cleared.has('parent') && stored.parentGroupId) { + mismatches.push(`parent is still ${stored.parentGroupId}`); + } + + if (mismatches.length === 0) return null; + return `Group ${groupId} was not stored as requested (${mismatches.join('; ')}). ${VERSION_MISMATCH_HINT}`; +} + +/** The appearance/hierarchy fields of a stored group, for JSON output. */ +export function describePersistedGroup(groupId: string): Partial { + const stored = readGroups().find((group) => group.id === groupId); + if (!stored) return {}; + return { + id: stored.id, + name: stored.name, + emoji: stored.emoji, + ...(stored.icon ? { icon: stored.icon } : {}), + ...(stored.color ? { color: stored.color } : {}), + ...(stored.parentGroupId ? { parentGroupId: stored.parentGroupId } : {}), + }; +} diff --git a/src/main/preload/process/groupCrudRemote.ts b/src/main/preload/process/groupCrudRemote.ts index dade296437..c317661e76 100644 --- a/src/main/preload/process/groupCrudRemote.ts +++ b/src/main/preload/process/groupCrudRemote.ts @@ -1,4 +1,5 @@ import { ipcRenderer } from 'electron'; +import type { GroupAppearance, GroupUpdateRequest } from '../../../shared/groupAppearance'; export function createGroupCrudRemoteApi() { return { @@ -11,6 +12,7 @@ export function createGroupCrudRemoteApi() { name: string, emoji: string | undefined, parentGroupId: string | undefined, + appearance: GroupAppearance, responseChannel: string ) => void ): (() => void) => { @@ -19,9 +21,10 @@ export function createGroupCrudRemoteApi() { name: string, emoji: string | undefined, parentGroupId: string | undefined, + appearance: GroupAppearance, responseChannel: string ) => { - callback(name, emoji, parentGroupId, responseChannel); + callback(name, emoji, parentGroupId, appearance ?? {}, responseChannel); }; ipcRenderer.on('remote:createGroup', handler); return () => ipcRenderer.removeListener('remote:createGroup', handler); @@ -57,6 +60,30 @@ export function createGroupCrudRemoteApi() { ipcRenderer.send(responseChannel, success); }, + /** + * Subscribe to remote group updates (name / appearance / parent). + * Uses request-response pattern with a unique responseChannel. + */ + onRemoteUpdateGroup: ( + callback: (groupId: string, update: GroupUpdateRequest, responseChannel: string) => void + ): (() => void) => { + const handler = ( + _: unknown, + groupId: string, + update: GroupUpdateRequest, + responseChannel: string + ) => callback(groupId, update, responseChannel); + ipcRenderer.on('remote:updateGroup', handler); + return () => ipcRenderer.removeListener('remote:updateGroup', handler); + }, + + /** + * Send response for remote update group + */ + sendRemoteUpdateGroupResponse: (responseChannel: string, success: boolean): void => { + ipcRenderer.send(responseChannel, success); + }, + /** * Subscribe to remote delete group from web interface (fire-and-forget) */ diff --git a/src/main/web-server/WebServer.ts b/src/main/web-server/WebServer.ts index de7ebf6db7..1d45228df2 100644 --- a/src/main/web-server/WebServer.ts +++ b/src/main/web-server/WebServer.ts @@ -30,6 +30,7 @@ import { randomUUID } from 'crypto'; import path from 'path'; import { existsSync } from 'fs'; import { logger } from '../utils/logger'; +import type { GroupAppearance, GroupUpdateRequest } from '../../shared/groupAppearance'; import { getLocalIpAddress } from '../utils/networkUtils'; import { captureException } from '../utils/sentry'; import { WebSocketMessageHandler } from './handlers'; @@ -103,6 +104,7 @@ import type { GetGroupsCallback, CreateGroupCallback, RenameGroupCallback, + UpdateGroupCallback, DeleteGroupCallback, MoveSessionToGroupCallback, CreateSessionCallback, @@ -596,6 +598,10 @@ export class WebServer { this.callbackRegistry.setRenameGroupCallback(callback); } + setUpdateGroupCallback(callback: UpdateGroupCallback): void { + this.callbackRegistry.setUpdateGroupCallback(callback); + } + setDeleteGroupCallback(callback: DeleteGroupCallback): void { this.callbackRegistry.setDeleteGroupCallback(callback); } @@ -1046,10 +1052,16 @@ export class WebServer { getSettings: () => this.callbackRegistry.getSettings(), setSetting: async (key: string, value: any) => this.callbackRegistry.setSetting(key, value), getGroups: () => this.callbackRegistry.getGroups(), - createGroup: async (name: string, emoji?: string, parentGroupId?: string) => - this.callbackRegistry.createGroup(name, emoji, parentGroupId), + createGroup: async ( + name: string, + emoji?: string, + parentGroupId?: string, + appearance?: GroupAppearance + ) => this.callbackRegistry.createGroup(name, emoji, parentGroupId, appearance), renameGroup: async (groupId: string, name: string) => this.callbackRegistry.renameGroup(groupId, name), + updateGroup: async (groupId: string, update: GroupUpdateRequest) => + this.callbackRegistry.updateGroup(groupId, update), deleteGroup: async (groupId: string) => this.callbackRegistry.deleteGroup(groupId), moveSessionToGroup: async (sessionId: string, groupId: string | null) => this.callbackRegistry.moveSessionToGroup(sessionId, groupId), diff --git a/src/main/web-server/callbacks/groupCrudCallbacks.ts b/src/main/web-server/callbacks/groupCrudCallbacks.ts index 6763fe0d0d..bdad57faae 100644 --- a/src/main/web-server/callbacks/groupCrudCallbacks.ts +++ b/src/main/web-server/callbacks/groupCrudCallbacks.ts @@ -4,57 +4,72 @@ import type { WebServer } from '../WebServer'; import type { WebServerFactoryDependencies } from '../web-server-factory'; import { logger } from '../../utils/logger'; import { isWebContentsAvailable } from '../../utils/safe-send'; +import { createRemoteRequest } from './remoteRequest'; +import type { GroupAppearance, GroupUpdateRequest } from '../../../shared/groupAppearance'; export function registerGroupCrudCallbacks( server: WebServer, deps: Pick ): void { const { getMainWindow } = deps; + const remoteRequest = createRemoteRequest(getMainWindow); // Set up callback for web server to create a group // Uses IPC request-response pattern - server.setCreateGroupCallback(async (name: string, emoji?: string, parentGroupId?: string) => { - const mainWindow = getMainWindow(); - if (!mainWindow) { - logger.warn('mainWindow is null for createGroup', 'WebServer'); - return null; - } - - return new Promise((resolve) => { - const responseChannel = `remote:createGroup:response:${randomUUID()}`; - let resolved = false; - - const handleResponse = (_event: Electron.IpcMainEvent, result: any) => { - if (resolved) return; - resolved = true; - clearTimeout(timeoutId); - resolve(result || null); - }; - - ipcMain.once(responseChannel, handleResponse); - if (!isWebContentsAvailable(mainWindow)) { - logger.warn('webContents is not available for createGroup', 'WebServer'); - ipcMain.removeListener(responseChannel, handleResponse); - resolve(null); - return; + server.setCreateGroupCallback( + async (name: string, emoji?: string, parentGroupId?: string, appearance?: GroupAppearance) => { + const mainWindow = getMainWindow(); + if (!mainWindow) { + logger.warn('mainWindow is null for createGroup', 'WebServer'); + return null; } - mainWindow.webContents.send( - 'remote:createGroup', - name, - emoji, - parentGroupId, - responseChannel - ); - const timeoutId = setTimeout(() => { - if (resolved) return; - resolved = true; - ipcMain.removeListener(responseChannel, handleResponse); - logger.warn(`createGroup callback timed out`, 'WebServer'); - resolve(null); - }, 5000); - }); - }); + return new Promise((resolve) => { + const responseChannel = `remote:createGroup:response:${randomUUID()}`; + let resolved = false; + + const handleResponse = (_event: Electron.IpcMainEvent, result: any) => { + if (resolved) return; + resolved = true; + clearTimeout(timeoutId); + resolve(result || null); + }; + + ipcMain.once(responseChannel, handleResponse); + if (!isWebContentsAvailable(mainWindow)) { + logger.warn('webContents is not available for createGroup', 'WebServer'); + ipcMain.removeListener(responseChannel, handleResponse); + resolve(null); + return; + } + mainWindow.webContents.send( + 'remote:createGroup', + name, + emoji, + parentGroupId, + appearance ?? {}, + responseChannel + ); + + const timeoutId = setTimeout(() => { + if (resolved) return; + resolved = true; + ipcMain.removeListener(responseChannel, handleResponse); + logger.warn(`createGroup callback timed out`, 'WebServer'); + resolve(null); + }, 5000); + }); + } + ); + + // Set up callback for web server to update a group's name, appearance, or + // parent. Uses the shared IPC request-response helper; the renderer answers + // `false` when the group is gone or the reparent is illegal. + server.setUpdateGroupCallback(async (groupId: string, update: GroupUpdateRequest) => + remoteRequest('updateGroup', 'updateGroup', false, (mainWindow, responseChannel) => + mainWindow.webContents.send('remote:updateGroup', groupId, update, responseChannel) + ) + ); // Set up callback for web server to rename a group // Uses IPC request-response pattern diff --git a/src/main/web-server/handlers/messageHandlers/WebSocketMessageHandler.ts b/src/main/web-server/handlers/messageHandlers/WebSocketMessageHandler.ts index 117845669f..709a5b3c16 100644 --- a/src/main/web-server/handlers/messageHandlers/WebSocketMessageHandler.ts +++ b/src/main/web-server/handlers/messageHandlers/WebSocketMessageHandler.ts @@ -113,6 +113,7 @@ import { handleGetGroups, handleCreateGroup, handleRenameGroup, + handleUpdateGroup, handleDeleteGroup, handleMoveSessionToGroup, } from './groups'; @@ -449,6 +450,10 @@ export class WebSocketMessageHandler { handleRenameGroup(this.ctx, client, message); break; + case 'update_group': + handleUpdateGroup(this.ctx, client, message); + break; + case 'delete_group': handleDeleteGroup(this.ctx, client, message); break; diff --git a/src/main/web-server/handlers/messageHandlers/groups.ts b/src/main/web-server/handlers/messageHandlers/groups.ts index 87df2c5e68..4e5e1dd80c 100644 --- a/src/main/web-server/handlers/messageHandlers/groups.ts +++ b/src/main/web-server/handlers/messageHandlers/groups.ts @@ -6,6 +6,24 @@ */ import type { WebClient, WebClientMessage, MessageHandlerContext } from './types'; +import { + isGroupClearableField, + validateGroupAppearance, + validateGroupUpdate, + type GroupClearableField, + type GroupUpdateRequest, +} from '../../../../shared/groupAppearance'; + +/** Read an optional string field, rejecting a present-but-wrong-typed value. */ +function optionalString( + message: WebClientMessage, + key: string +): { ok: true; value?: string } | { ok: false } { + const raw = (message as Record)[key]; + if (raw === undefined) return { ok: true }; + if (typeof raw !== 'string') return { ok: false }; + return { ok: true, value: raw }; +} /** * Handle get_groups message - return list of groups @@ -37,7 +55,6 @@ export function handleCreateGroup( message: WebClientMessage ): void { const name = message.name as string; - const emoji = message.emoji as string | undefined; const requestedParentGroupId = message.parentGroupId; if ( @@ -55,13 +72,34 @@ export function handleCreateGroup( return; } + const emojiField = optionalString(message, 'emoji'); + const iconField = optionalString(message, 'icon'); + const colorField = optionalString(message, 'color'); + if (!emojiField.ok || !iconField.ok || !colorField.ok) { + ctx.sendError(client, 'Invalid group appearance'); + return; + } + + // Validate at the WebSocket boundary, not just in the CLI: every client + // speaking this protocol reaches the same renderer state, so an unvalidated + // direct socket write would persist an icon id the picker cannot draw. + const appearance = validateGroupAppearance({ + emoji: emojiField.value, + icon: iconField.value, + color: colorField.value, + }); + if (!appearance.ok) { + ctx.sendError(client, appearance.error); + return; + } + if (!ctx.callbacks.createGroup) { ctx.sendError(client, 'Group creation not configured'); return; } ctx.callbacks - .createGroup(name, emoji, parentGroupId) + .createGroup(name, appearance.value.emoji, parentGroupId, appearance.value) .then((result) => { ctx.send(client, { type: 'create_group_result', @@ -116,6 +154,72 @@ export function handleRenameGroup( }); } +/** + * Handle update_group message - change a group's name, appearance, or parent. + * + * Everything is validated here, before the renderer is asked to mutate + * anything, so a request carrying one bad field cannot half-apply. The renderer + * still rejects a reparent that would break the one-level nesting rule, because + * only it holds the group list. + */ +export function handleUpdateGroup( + ctx: MessageHandlerContext, + client: WebClient, + message: WebClientMessage +): void { + const groupId = message.groupId as string; + + if (!groupId || typeof groupId !== 'string') { + ctx.sendError(client, 'Missing groupId'); + return; + } + + const fields = ['name', 'emoji', 'icon', 'color', 'parentGroupId'] as const; + const request: GroupUpdateRequest = {}; + for (const field of fields) { + const read = optionalString(message, field); + if (!read.ok) { + ctx.sendError(client, `Invalid ${field}`); + return; + } + if (read.value !== undefined) request[field] = read.value; + } + + const rawClear = (message as Record).clear; + if (rawClear !== undefined) { + if (!Array.isArray(rawClear) || !rawClear.every(isGroupClearableField)) { + ctx.sendError(client, 'Invalid clear list'); + return; + } + request.clear = rawClear as GroupClearableField[]; + } + + const validated = validateGroupUpdate(request); + if (!validated.ok) { + ctx.sendError(client, validated.error); + return; + } + + if (!ctx.callbacks.updateGroup) { + ctx.sendError(client, 'Group updating not configured'); + return; + } + + ctx.callbacks + .updateGroup(groupId, validated.value) + .then((success) => { + ctx.send(client, { + type: 'update_group_result', + success, + groupId, + requestId: message.requestId, + }); + }) + .catch((error) => { + ctx.sendError(client, `Failed to update group: ${error.message}`); + }); +} + /** * Handle delete_group message - delete a group */ diff --git a/src/main/web-server/handlers/messageHandlers/types.ts b/src/main/web-server/handlers/messageHandlers/types.ts index 4a9cc0fcc9..bb480e3d25 100644 --- a/src/main/web-server/handlers/messageHandlers/types.ts +++ b/src/main/web-server/handlers/messageHandlers/types.ts @@ -33,6 +33,7 @@ import type { GetSessionHistoryOptions, EnqueueCommandResult, } from '../../types'; +import type { GroupAppearance, GroupUpdateRequest } from '../../../../shared/groupAppearance'; import type { CadenzaPayload } from '../../../../shared/cadenza-types'; import type { MovementPayload, MovementStateSnapshot } from '../../../../shared/movement-types'; import type { @@ -232,9 +233,11 @@ export interface MessageHandlerCallbacks { createGroup: ( name: string, emoji?: string, - parentGroupId?: string + parentGroupId?: string, + appearance?: GroupAppearance ) => Promise<{ id: string } | null>; renameGroup: (groupId: string, name: string) => Promise; + updateGroup: (groupId: string, update: GroupUpdateRequest) => Promise; deleteGroup: (groupId: string) => Promise; moveSessionToGroup: (sessionId: string, groupId: string | null) => Promise; createSession: ( diff --git a/src/main/web-server/managers/CallbackRegistry.ts b/src/main/web-server/managers/CallbackRegistry.ts index d7c5ec3e12..52383ce86a 100644 --- a/src/main/web-server/managers/CallbackRegistry.ts +++ b/src/main/web-server/managers/CallbackRegistry.ts @@ -70,6 +70,7 @@ import type { GetGroupsCallback, CreateGroupCallback, RenameGroupCallback, + UpdateGroupCallback, DeleteGroupCallback, MoveSessionToGroupCallback, CreateSessionCallback, @@ -134,6 +135,7 @@ import type { DesktopSessionEntry, SessionHistoryResult, } from '../types'; +import type { GroupAppearance, GroupUpdateRequest } from '../../../shared/groupAppearance'; import type { CadenzaPayload } from '../../../shared/cadenza-types'; import type { MovementPayload, MovementStateSnapshot } from '../../../shared/movement-types'; @@ -193,6 +195,7 @@ export interface WebServerCallbacks { getGroups: GetGroupsCallback | null; createGroup: CreateGroupCallback | null; renameGroup: RenameGroupCallback | null; + updateGroup: UpdateGroupCallback | null; deleteGroup: DeleteGroupCallback | null; moveSessionToGroup: MoveSessionToGroupCallback | null; createSession: CreateSessionCallback | null; @@ -289,6 +292,7 @@ export class CallbackRegistry { getGroups: null, createGroup: null, renameGroup: null, + updateGroup: null, deleteGroup: null, moveSessionToGroup: null, createSession: null, @@ -666,10 +670,11 @@ export class CallbackRegistry { async createGroup( name: string, emoji?: string, - parentGroupId?: string + parentGroupId?: string, + appearance?: GroupAppearance ): Promise<{ id: string } | null> { if (!this.callbacks.createGroup) return null; - return this.callbacks.createGroup(name, emoji, parentGroupId); + return this.callbacks.createGroup(name, emoji, parentGroupId, appearance); } async renameGroup(groupId: string, name: string): Promise { @@ -677,6 +682,11 @@ export class CallbackRegistry { return this.callbacks.renameGroup(groupId, name); } + async updateGroup(groupId: string, update: GroupUpdateRequest): Promise { + if (!this.callbacks.updateGroup) return false; + return this.callbacks.updateGroup(groupId, update); + } + async deleteGroup(groupId: string): Promise { if (!this.callbacks.deleteGroup) return false; return this.callbacks.deleteGroup(groupId); @@ -1172,6 +1182,10 @@ export class CallbackRegistry { this.callbacks.renameGroup = callback; } + setUpdateGroupCallback(callback: UpdateGroupCallback): void { + this.callbacks.updateGroup = callback; + } + setDeleteGroupCallback(callback: DeleteGroupCallback): void { this.callbacks.deleteGroup = callback; } diff --git a/src/main/web-server/types.ts b/src/main/web-server/types.ts index fdb769cffb..52c87cb976 100644 --- a/src/main/web-server/types.ts +++ b/src/main/web-server/types.ts @@ -4,6 +4,7 @@ */ import type { DesktopTabEntry } from '../../shared/desktopTabs'; +import type { GroupAppearance, GroupUpdateRequest } from '../../shared/groupAppearance'; import type { WebSocket } from 'ws'; import type { Theme } from '../../shared/theme-types'; import type { Shortcut } from '../../shared/shortcut-types'; @@ -931,9 +932,16 @@ export type GetGroupsCallback = () => GroupData[]; export type CreateGroupCallback = ( name: string, emoji?: string, - parentGroupId?: string + parentGroupId?: string, + appearance?: GroupAppearance ) => Promise<{ id: string } | null>; export type RenameGroupCallback = (groupId: string, name: string) => Promise; +/** + * Apply a validated group update. Resolves `false` when the group is gone or + * the requested reparent would break the one-level nesting rule - the renderer + * is the only place that can answer either question. + */ +export type UpdateGroupCallback = (groupId: string, update: GroupUpdateRequest) => Promise; export type DeleteGroupCallback = (groupId: string) => Promise; export type MoveSessionToGroupCallback = ( sessionId: string, diff --git a/src/renderer/components/ui/groupAppearanceOptions.ts b/src/renderer/components/ui/groupAppearanceOptions.ts index bad12f5ae3..c9b395ca34 100644 --- a/src/renderer/components/ui/groupAppearanceOptions.ts +++ b/src/renderer/components/ui/groupAppearanceOptions.ts @@ -18,6 +18,9 @@ import { type LucideIcon, } from 'lucide-react'; import type { IconPackContribution } from '../../../shared/plugins/contributions'; +import { GROUP_ICON_CATALOG, GROUP_LABEL_COLORS } from '../../../shared/groupAppearance'; + +export { GROUP_LABEL_COLORS }; export interface GroupIconOption { id: string; @@ -35,35 +38,34 @@ export interface ResolvedGroupAppearance { color: string | undefined; } -export const GROUP_ICON_OPTIONS: readonly GroupIconOption[] = [ - { id: 'folder', label: 'Folder', Icon: Folder }, - { id: 'briefcase', label: 'Briefcase', Icon: Briefcase }, - { id: 'rocket', label: 'Rocket', Icon: Rocket }, - { id: 'code', label: 'Code', Icon: Code2 }, - { id: 'star', label: 'Star', Icon: Star }, - { id: 'heart', label: 'Heart', Icon: Heart }, - { id: 'lightbulb', label: 'Lightbulb', Icon: Lightbulb }, - { id: 'target', label: 'Target', Icon: Target }, - { id: 'calendar', label: 'Calendar', Icon: Calendar }, - { id: 'book', label: 'Book', Icon: BookOpen }, - { id: 'layers', label: 'Layers', Icon: Layers }, - { id: 'shield', label: 'Shield', Icon: Shield }, - { id: 'wrench', label: 'Wrench', Icon: Wrench }, - { id: 'palette', label: 'Palette', Icon: Palette }, - { id: 'archive', label: 'Archive', Icon: Archive }, - { id: 'zap', label: 'Zap', Icon: Zap }, -]; +/** + * Icon-id -> Lucide component. The id list itself lives in the shared catalog + * (`shared/groupAppearance.ts`) so the CLI and the WebSocket handlers validate + * against the same ids; only this mapping is renderer-owned, because Lucide + * cannot be imported outside the renderer bundle. + */ +const GROUP_ICON_COMPONENTS: Record = { + folder: Folder, + briefcase: Briefcase, + rocket: Rocket, + code: Code2, + star: Star, + heart: Heart, + lightbulb: Lightbulb, + target: Target, + calendar: Calendar, + book: BookOpen, + layers: Layers, + shield: Shield, + wrench: Wrench, + palette: Palette, + archive: Archive, + zap: Zap, +}; -export const GROUP_LABEL_COLORS = [ - { value: '#EF4444', label: 'Red' }, - { value: '#F97316', label: 'Orange' }, - { value: '#EAB308', label: 'Yellow' }, - { value: '#22C55E', label: 'Green' }, - { value: '#14B8A6', label: 'Teal' }, - { value: '#3B82F6', label: 'Blue' }, - { value: '#EC4899', label: 'Pink' }, - { value: '#A855F7', label: 'Purple' }, -] as const; +export const GROUP_ICON_OPTIONS: readonly GroupIconOption[] = GROUP_ICON_CATALOG.filter( + (entry) => entry.id in GROUP_ICON_COMPONENTS +).map((entry) => ({ id: entry.id, label: entry.label, Icon: GROUP_ICON_COMPONENTS[entry.id] })); /** * Resolves a stored group appearance against the current host and plugin option diff --git a/src/renderer/global.d.ts b/src/renderer/global.d.ts index 860770d70b..8478008eb3 100644 --- a/src/renderer/global.d.ts +++ b/src/renderer/global.d.ts @@ -813,6 +813,7 @@ interface MaestroAPI { name: string, emoji: string | undefined, parentGroupId: string | undefined, + appearance: import('../shared/groupAppearance').GroupAppearance, responseChannel: string ) => void ) => () => void; @@ -821,6 +822,14 @@ interface MaestroAPI { callback: (groupId: string, name: string, responseChannel: string) => void ) => () => void; sendRemoteRenameGroupResponse: (responseChannel: string, success: boolean) => void; + onRemoteUpdateGroup: ( + callback: ( + groupId: string, + update: import('../shared/groupAppearance').GroupUpdateRequest, + responseChannel: string + ) => void + ) => () => void; + sendRemoteUpdateGroupResponse: (responseChannel: string, success: boolean) => void; onRemoteDeleteGroup: (callback: (groupId: string) => void) => () => void; onRemoteMoveSessionToGroup: ( callback: (sessionId: string, groupId: string | null, responseChannel: string) => void diff --git a/src/renderer/hooks/remote/useAppRemoteEventListeners.ts b/src/renderer/hooks/remote/useAppRemoteEventListeners.ts index d85e8ed626..53cc714bc6 100644 --- a/src/renderer/hooks/remote/useAppRemoteEventListeners.ts +++ b/src/renderer/hooks/remote/useAppRemoteEventListeners.ts @@ -32,8 +32,76 @@ import { spawnWorktreeAgentAndDispatch } from '../../utils/worktreeSpawn'; import { notifyToast } from '../../stores/notificationStore'; import { canCreateGroupInside, + canSetGroupParent, removeGroupAndPromoteChildren, + setGroupParent, } from '../../../shared/groupHierarchy'; +import { + validateGroupAppearance, + validateGroupUpdate, + type GroupUpdateRequest, +} from '../../../shared/groupAppearance'; + +// ============================================================================ +// Group update helpers +// ============================================================================ + +/** + * Write the group list straight to disk and wait for it. + * + * The store's own persistence runs from a React effect, so it lands after this + * listener has already answered the caller. A CLI that verifies its write by + * reading `maestro-groups.json` back would then see the pre-update list and + * report a false mismatch. Flushing before responding makes the readback + * deterministic; the effect's later write is the same data and is idempotent. + * Same reasoning as the remote session-rename handler above. + */ +async function flushGroupsToDisk(groups: Group[]): Promise { + try { + await window.maestro.groups.setAll(groups); + } catch (error) { + logger.error('[Remote] Failed to persist group change:', undefined, error); + } +} + +/** + * Apply a validated update to one group. Pure so the ordering rules are + * testable: the parent move runs first (it can reject on its own terms and + * returns the list unchanged when it does), then the field-level sets and + * clears are applied to the moved list. + */ +function applyGroupUpdate( + groups: Group[], + groupId: string, + request: GroupUpdateRequest, + clear: Set +): Group[] { + let next = groups; + if (request.parentGroupId) { + next = setGroupParent(next, groupId, request.parentGroupId); + } else if (clear.has('parent')) { + next = setGroupParent(next, groupId, undefined); + } + + return next.map((group) => { + if (group.id !== groupId) return group; + const updated: Group = { ...group }; + if (request.name) updated.name = request.name.toUpperCase(); + // An icon and an emoji are alternative presentations of the same group, + // and the Groups+ gate falls back to the emoji, so setting one never + // discards the other - clearing is always explicit. + if (request.emoji) updated.emoji = request.emoji; + if (request.icon) updated.icon = request.icon; + if (request.color) updated.color = request.color; + // The emoji is non-optional on Group and the Left Bar renders it when + // Groups+ is off, so clearing it restores the default folder rather + // than leaving a group with no glyph at all. + if (clear.has('emoji')) updated.emoji = '\u{1F4C2}'; + if (clear.has('icon')) delete updated.icon; + if (clear.has('color')) delete updated.color; + return updated; + }); +} // ============================================================================ // Dependencies interface @@ -1718,11 +1786,12 @@ export function useAppRemoteEventListeners(deps: UseAppRemoteEventListenersDeps) // --- Group CRUD --- // Handle remote create group from web interface - useEventListener('maestro:remoteCreateGroup', (e: Event) => { + useEventListener('maestro:remoteCreateGroup', async (e: Event) => { const { name, emoji, parentGroupId: requestedParentGroupId, + appearance, responseChannel, } = (e as CustomEvent).detail; const trimmed = name.trim(); @@ -1738,18 +1807,31 @@ export function useAppRemoteEventListeners(deps: UseAppRemoteEventListenersDeps) window.maestro.process.sendRemoteCreateGroupResponse(responseChannel, null); return; } + // Re-validate here rather than trusting the sender: this listener is + // reachable from any client on the WS bridge, and a bad icon id written + // into the group list would survive every later read. + const validated = validateGroupAppearance({ + emoji, + icon: appearance?.icon, + color: appearance?.color, + }); + if (!validated.ok) { + window.maestro.process.sendRemoteCreateGroupResponse(responseChannel, null); + return; + } const newGroupId = `group-${generateId()}`; - setGroups((prev: Group[]) => [ - ...prev, - { - id: newGroupId, - name: trimmed.toUpperCase(), - emoji: emoji || '\u{1F4C2}', - kind: 'user', - ...(parentGroupId ? { parentGroupId } : {}), - collapsed: false, - }, - ]); + const newGroup: Group = { + id: newGroupId, + name: trimmed.toUpperCase(), + emoji: validated.value.emoji || '\u{1F4C2}', + kind: 'user', + ...(validated.value.icon ? { icon: validated.value.icon } : {}), + ...(validated.value.color ? { color: validated.value.color } : {}), + ...(parentGroupId ? { parentGroupId } : {}), + collapsed: false, + }; + setGroups((prev: Group[]) => [...prev, newGroup]); + await flushGroupsToDisk(useSessionStore.getState().groups); window.maestro.process.sendRemoteCreateGroupResponse(responseChannel, { id: newGroupId }); }); @@ -1767,6 +1849,46 @@ export function useAppRemoteEventListeners(deps: UseAppRemoteEventListenersDeps) window.maestro.process.sendRemoteRenameGroupResponse(responseChannel, true); }); + // Handle a remote group update (name / appearance / parent). The payload is + // already validated by the WS handler; what only the renderer can decide is + // whether the group exists and whether the requested reparent is legal, so + // both are checked before any state is written. + useEventListener('maestro:remoteUpdateGroup', async (e: Event) => { + const { groupId, update, responseChannel } = (e as CustomEvent).detail as { + groupId: string; + update: GroupUpdateRequest; + responseChannel: string; + }; + const respond = (success: boolean) => + window.maestro.process.sendRemoteUpdateGroupResponse(responseChannel, success); + + const validated = validateGroupUpdate(update ?? {}); + if (!validated.ok) { + respond(false); + return; + } + const request = validated.value; + const clear = new Set(request.clear ?? []); + + const currentGroups = useSessionStore.getState().groups; + if (!currentGroups.some((g) => g.id === groupId)) { + respond(false); + return; + } + if ( + request.parentGroupId && + !canSetGroupParent(currentGroups, groupId, request.parentGroupId) + ) { + respond(false); + return; + } + + const nextGroups = applyGroupUpdate(currentGroups, groupId, request, clear); + setGroups(() => nextGroups); + await flushGroupsToDisk(nextGroups); + respond(true); + }); + // Handle remote delete group from web interface (fire-and-forget) useEventListener('maestro:remoteDeleteGroup', (e: Event) => { const { groupId } = (e as CustomEvent).detail; diff --git a/src/renderer/hooks/remote/useRemoteIntegration.ts b/src/renderer/hooks/remote/useRemoteIntegration.ts index 2e2db1dcbe..fd83593464 100644 --- a/src/renderer/hooks/remote/useRemoteIntegration.ts +++ b/src/renderer/hooks/remote/useRemoteIntegration.ts @@ -1,6 +1,7 @@ import { useEffect, useRef } from 'react'; import { flushSync } from 'react-dom'; import type { Session, SessionState, ThinkingMode, QueuedItem } from '../../types'; +import type { GroupAppearance, GroupUpdateRequest } from '../../../shared/groupAppearance'; import { cueService } from '../../services/cue'; import { captureException } from '../../utils/sentry'; import { aiTabFocusFields, createTab, closeTab, getActiveTab } from '../../utils/tabHelpers'; @@ -1657,11 +1658,22 @@ export function useRemoteIntegration(deps: UseRemoteIntegrationDeps): UseRemoteI name: string, emoji: string | undefined, parentGroupId: string | undefined, + appearance: GroupAppearance, responseChannel: string ) => { window.dispatchEvent( new CustomEvent('maestro:remoteCreateGroup', { - detail: { name, emoji, parentGroupId, responseChannel }, + detail: { name, emoji, parentGroupId, appearance, responseChannel }, + }) + ); + } + ); + + const unsubscribeUpdateGroup = window.maestro.process.onRemoteUpdateGroup( + (groupId: string, update: GroupUpdateRequest, responseChannel: string) => { + window.dispatchEvent( + new CustomEvent('maestro:remoteUpdateGroup', { + detail: { groupId, update, responseChannel }, }) ); } @@ -1704,6 +1716,7 @@ export function useRemoteIntegration(deps: UseRemoteIntegrationDeps): UseRemoteI unsubscribeUpdateSessionConfig(); unsubscribeCreateGroup(); unsubscribeRenameGroup(); + unsubscribeUpdateGroup(); unsubscribeDeleteGroup(); unsubscribeMoveSessionToGroup(); }; diff --git a/src/shared/groupAppearance.ts b/src/shared/groupAppearance.ts new file mode 100644 index 0000000000..2d2dbe2397 --- /dev/null +++ b/src/shared/groupAppearance.ts @@ -0,0 +1,256 @@ +/** + * UI-independent catalog and validation for group appearance (icon + label + * color). + * + * Lives in `shared/` because three consumers must agree on exactly one set of + * ids: the renderer's picker (`components/ui/groupAppearanceOptions.ts`, which + * adds the icon-id -> Lucide mapping on top of this), the WebSocket message + * handlers that accept `create_group` / `update_group` from any client, and the + * CLI's `create-group` / `update-group` commands. A second copy of the id list + * would let the CLI accept an icon the picker cannot draw. + * + * Values are normalized rather than merely checked, so `#ef4444` and `#EF4444` + * persist identically and a later readback comparison is a plain string equal. + */ + +/** One built-in group icon. The renderer maps `id` to a Lucide component. */ +export interface GroupIconCatalogEntry { + id: string; + label: string; +} + +/** One built-in label color. `value` is the persisted `#RRGGBB` string. */ +export interface GroupColorCatalogEntry { + value: string; + label: string; +} + +export const GROUP_ICON_CATALOG: readonly GroupIconCatalogEntry[] = [ + { id: 'folder', label: 'Folder' }, + { id: 'briefcase', label: 'Briefcase' }, + { id: 'rocket', label: 'Rocket' }, + { id: 'code', label: 'Code' }, + { id: 'star', label: 'Star' }, + { id: 'heart', label: 'Heart' }, + { id: 'lightbulb', label: 'Lightbulb' }, + { id: 'target', label: 'Target' }, + { id: 'calendar', label: 'Calendar' }, + { id: 'book', label: 'Book' }, + { id: 'layers', label: 'Layers' }, + { id: 'shield', label: 'Shield' }, + { id: 'wrench', label: 'Wrench' }, + { id: 'palette', label: 'Palette' }, + { id: 'archive', label: 'Archive' }, + { id: 'zap', label: 'Zap' }, +] as const; + +export const GROUP_LABEL_COLORS: readonly GroupColorCatalogEntry[] = [ + { value: '#EF4444', label: 'Red' }, + { value: '#F97316', label: 'Orange' }, + { value: '#EAB308', label: 'Yellow' }, + { value: '#22C55E', label: 'Green' }, + { value: '#14B8A6', label: 'Teal' }, + { value: '#3B82F6', label: 'Blue' }, + { value: '#EC4899', label: 'Pink' }, + { value: '#A855F7', label: 'Purple' }, +] as const; + +/** Built-in icon ids, in picker order. */ +export const GROUP_ICON_IDS: readonly string[] = GROUP_ICON_CATALOG.map((entry) => entry.id); + +const HEX_COLOR_PATTERN = /^#[0-9a-fA-F]{6}$/; + +/** + * A plugin-contributed icon or color id: two or more `LOCAL_ID_PATTERN` + * segments joined by `/` (`//`). Kept in sync with + * `LOCAL_ID_PATTERN` in `shared/plugins/contributions.ts` - a namespaced id the + * CLI accepts but the contribution loader would reject can never resolve. + */ +const NAMESPACED_ID_PATTERN = + /^[a-z][a-z0-9]*([._-][a-z0-9]+)*(\/[a-z][a-z0-9]*([._-][a-z0-9]+)*)+$/; + +/** + * Canonical form of an icon id, or `null` when it is neither a built-in nor a + * plugin-namespaced id. Built-ins and namespaced ids are both lowercased so a + * `--icon Rocket` and a `--icon rocket` persist the same value. + */ +export function normalizeGroupIconId(raw: string): string | null { + const candidate = raw.trim().toLowerCase(); + if (!candidate) return null; + if (GROUP_ICON_IDS.includes(candidate)) return candidate; + if (NAMESPACED_ID_PATTERN.test(candidate)) return candidate; + return null; +} + +/** + * Canonical form of a label color, or `null` when unrecognized. `#RRGGBB` is + * uppercased (the built-in catalog is stored uppercase, so a user passing the + * lowercase hex of a built-in color lands on the exact catalog entry rather + * than a near-duplicate that the picker cannot highlight). + */ +export function normalizeGroupColor(raw: string): string | null { + const candidate = raw.trim(); + if (!candidate) return null; + if (HEX_COLOR_PATTERN.test(candidate)) return candidate.toUpperCase(); + const lowered = candidate.toLowerCase(); + if (NAMESPACED_ID_PATTERN.test(lowered)) return lowered; + return null; +} + +/** Appearance fields accepted on a create or update, before validation. */ +export interface GroupAppearanceInput { + emoji?: string; + icon?: string; + color?: string; +} + +/** Appearance fields after normalization, ready to persist. */ +export interface GroupAppearance { + emoji?: string; + icon?: string; + color?: string; +} + +export type GroupAppearanceValidation = + | { ok: true; value: GroupAppearance } + | { ok: false; error: string }; + +/** Human-readable list of built-in icon ids, for error messages. */ +export function describeGroupIconIds(): string { + return GROUP_ICON_IDS.join(', '); +} + +/** + * Validate and normalize an appearance request. Returns the normalized fields + * (only the ones actually supplied) or a single error string. + * + * Callers must apply this BEFORE mutating any state: a request carrying a good + * name and a bad color has to fail whole, not persist the name and drop the + * color. + */ +export function validateGroupAppearance(input: GroupAppearanceInput): GroupAppearanceValidation { + const emoji = input.emoji?.trim(); + const iconRaw = input.icon?.trim(); + const colorRaw = input.color?.trim(); + + if (emoji && iconRaw) { + return { + ok: false, + error: 'Use either --emoji or --icon, not both (a group shows one or the other)', + }; + } + + const value: GroupAppearance = {}; + if (emoji) value.emoji = emoji; + + if (iconRaw) { + const icon = normalizeGroupIconId(iconRaw); + if (!icon) { + return { + ok: false, + error: `Unknown icon "${iconRaw}". Built-in icons: ${describeGroupIconIds()}. Plugin icons use a namespaced id like my-plugin/my-pack/my-icon.`, + }; + } + value.icon = icon; + } + + if (colorRaw) { + const color = normalizeGroupColor(colorRaw); + if (!color) { + return { + ok: false, + error: `Invalid color "${colorRaw}". Use a #RRGGBB hex value (for example ${GROUP_LABEL_COLORS[0].value}) or a plugin color id like my-plugin/my-pack/my-color.`, + }; + } + value.color = color; + } + + return { ok: true, value }; +} + +/** Appearance/hierarchy fields an `update_group` request may clear. */ +export const GROUP_CLEARABLE_FIELDS = ['emoji', 'icon', 'color', 'parent'] as const; +export type GroupClearableField = (typeof GROUP_CLEARABLE_FIELDS)[number]; + +/** + * The wire shape of an `update_group` request. Clearing is explicit via + * `clear` rather than a `null` value, because JSON round-trips lose the + * difference between "field absent" and "field set to undefined", and a group + * update has to be able to say "leave the color alone" and "remove the color" + * in the same message shape. + */ +export interface GroupUpdateRequest { + name?: string; + emoji?: string; + icon?: string; + color?: string; + parentGroupId?: string; + clear?: GroupClearableField[]; +} + +export function isGroupClearableField(value: unknown): value is GroupClearableField { + return typeof value === 'string' && (GROUP_CLEARABLE_FIELDS as readonly string[]).includes(value); +} + +export type GroupUpdateValidation = + | { ok: true; value: GroupUpdateRequest } + | { ok: false; error: string }; + +/** + * Validate and normalize a whole update request: appearance rules above, plus + * the clear list and the "an update must actually change something" rule. A + * field cannot be both set and cleared in one call - that is a scripting bug, + * and silently picking a winner would make the result depend on our internal + * ordering. + */ +export function validateGroupUpdate(request: GroupUpdateRequest): GroupUpdateValidation { + const clear = request.clear ?? []; + for (const field of clear) { + if (!isGroupClearableField(field)) { + return { ok: false, error: `Unknown clear target "${String(field)}"` }; + } + } + + const conflicts: Array<[GroupClearableField, string | undefined]> = [ + ['emoji', request.emoji], + ['icon', request.icon], + ['color', request.color], + ['parent', request.parentGroupId], + ]; + for (const [field, supplied] of conflicts) { + if (clear.includes(field) && supplied !== undefined) { + return { ok: false, error: `Cannot both set and clear ${field}` }; + } + } + + // An icon replaces an emoji and vice versa, so clearing one while setting + // the other is coherent - only reject setting both at once. + const appearance = validateGroupAppearance({ + emoji: request.emoji, + icon: request.icon, + color: request.color, + }); + if (!appearance.ok) return appearance; + + const value: GroupUpdateRequest = {}; + const name = request.name?.trim(); + if (name) value.name = name; + if (appearance.value.emoji) value.emoji = appearance.value.emoji; + if (appearance.value.icon) value.icon = appearance.value.icon; + if (appearance.value.color) value.color = appearance.value.color; + const parentGroupId = request.parentGroupId?.trim(); + if (parentGroupId) value.parentGroupId = parentGroupId; + if (clear.length > 0) value.clear = [...clear]; + + if (request.name !== undefined && !name) { + return { ok: false, error: 'Group name must not be empty' }; + } + if (request.parentGroupId !== undefined && !parentGroupId) { + return { ok: false, error: 'Parent group ID must not be empty' }; + } + if (Object.keys(value).length === 0) { + return { ok: false, error: 'Nothing to update' }; + } + + return { ok: true, value }; +}