From 28a2fde84f38f6f782672995dd11c6aba9076fb2 Mon Sep 17 00:00:00 2001 From: Test User Date: Mon, 24 Aug 2026 20:08:52 -0500 Subject: [PATCH 1/2] feat(cli): group icon/color flags and a new update-group verb Adds end-to-end CLI management of group appearance and hierarchy so a bootstrap script or CI job can reproduce a workspace's group layout without anyone clicking through the desktop UI. - `create-group` takes `--icon` and `--color`, and now resolves `--parent` through `resolveGroupId` like every other group verb. - New `update-group `: `--name`, `--emoji`, `--icon`, `--color`, `--parent`, plus explicit `--clear-emoji` / `--clear-icon` / `--clear-color` / `--clear-parent`. `rename-group` stays for backward compatibility. - `list groups --json` reports `icon` and `color` alongside `parentGroupId`. Writes are verified by reading `maestro-groups.json` back and comparing against the request, rather than trusting the desktop's ack. That is one check for a version mismatch, a silently ignored field, and a clear that did not take. The renderer flushes the group list to disk before acking so the readback is not racing the store's effect-driven persistence - same reasoning as the remote session-rename handler beside it. `src/shared/groupAppearance.ts` is the one UI-independent catalog of icon IDs and label colors, plus the normalization and validation over them. The renderer's picker now sources its IDs from it and keeps only the icon-ID -> Lucide mapping. Validation runs at the WebSocket boundary too, not just in the CLI, so a client writing straight to the socket cannot persist an icon the picker is unable to draw. Closes #1276 --- docs/agent-guides/CLI-UI-PARITY.md | 1 + docs/agent-guides/SHARED-UTILS.md | 26 ++ docs/cli-reference.md | 67 +++-- docs/cli.md | 68 ++++- .../cli/commands/create-group.test.ts | 247 +++++++++++----- .../cli/commands/list-groups.test.ts | 18 ++ .../cli/commands/update-group.test.ts | 240 +++++++++++++++ .../preload/process/groupCrudRemote.test.ts | 71 ++++- .../handlers/messageHandlers.test.ts | 104 ++++++- .../web-server/web-server-factory.test.ts | 36 ++- .../useAppRemoteEventListenersGroups.test.ts | 275 ++++++++++++++++++ .../hooks/useRemoteIntegration.test.ts | 4 + src/__tests__/shared/groupAppearance.test.ts | 165 +++++++++++ src/cli/commands/create-group.ts | 110 ++++--- src/cli/commands/list-groups.ts | 2 + src/cli/commands/update-group.ts | 105 +++++++ src/cli/index.ts | 27 ++ src/cli/services/group-appearance.ts | 99 +++++++ src/main/preload/process/groupCrudRemote.ts | 29 +- src/main/web-server/WebServer.ts | 16 +- .../callbacks/groupCrudCallbacks.ts | 95 +++--- .../WebSocketMessageHandler.ts | 5 + .../handlers/messageHandlers/groups.ts | 108 ++++++- .../handlers/messageHandlers/types.ts | 5 +- .../web-server/managers/CallbackRegistry.ts | 18 +- src/main/web-server/types.ts | 10 +- .../components/ui/groupAppearanceOptions.ts | 58 ++-- src/renderer/global.d.ts | 9 + .../remote/useAppRemoteEventListeners.ts | 146 +++++++++- .../hooks/remote/useRemoteIntegration.ts | 15 +- src/shared/groupAppearance.ts | 256 ++++++++++++++++ 31 files changed, 2170 insertions(+), 265 deletions(-) create mode 100644 src/__tests__/cli/commands/update-group.test.ts create mode 100644 src/__tests__/renderer/hooks/remote/useAppRemoteEventListenersGroups.test.ts create mode 100644 src/__tests__/shared/groupAppearance.test.ts create mode 100644 src/cli/commands/update-group.ts create mode 100644 src/cli/services/group-appearance.ts create mode 100644 src/shared/groupAppearance.ts 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..3dcfa86b8c 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, newGroup]); 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 }; +} From 6eefcb986196901e755939e9e208b1355c1d5cac Mon Sep 17 00:00:00 2001 From: Pedram Amini Date: Tue, 25 Aug 2026 08:43:40 -0500 Subject: [PATCH 2/2] fix(remote): stop the create-group flush from writing the group twice setGroups is a synchronous zustand setter bound from useSessionStore.getState(), so the store already holds newGroup by the time the flush payload is built. Appending it again put two entries with the same id into maestro-groups.json until the next persistence effect overwrote them. Read the store back instead. --- src/renderer/hooks/remote/useAppRemoteEventListeners.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/hooks/remote/useAppRemoteEventListeners.ts b/src/renderer/hooks/remote/useAppRemoteEventListeners.ts index 3dcfa86b8c..53cc714bc6 100644 --- a/src/renderer/hooks/remote/useAppRemoteEventListeners.ts +++ b/src/renderer/hooks/remote/useAppRemoteEventListeners.ts @@ -1831,7 +1831,7 @@ export function useAppRemoteEventListeners(deps: UseAppRemoteEventListenersDeps) collapsed: false, }; setGroups((prev: Group[]) => [...prev, newGroup]); - await flushGroupsToDisk([...useSessionStore.getState().groups, newGroup]); + await flushGroupsToDisk(useSessionStore.getState().groups); window.maestro.process.sendRemoteCreateGroupResponse(responseChannel, { id: newGroupId }); });