Skip to content

Commit 5709b50

Browse files
vdavidvdavid
committed
Command bus: type command ids end to end
- Add `COMMAND_IDS` `as const` tuple + derived `CommandId` union in a new `command-ids.ts`; the `commands` array stays a mutable `Command[]` (so `updateLicenseCommandName` and the conflict detector keep writing it), with `Command.id: CommandId` enforcing tuple ⊇ registry and a set-equality test enforcing registry ⊇ tuple. - Add `CommandArgs` / `CommandDispatchArgs` (arg-less commands → `NoCommandArgs`, the `dispatch<K>(id, ...args)` tuple shape) as the foundation later milestones extend; no arg-carrying entries yet. - Type the dispatch entry point: `handleCommandExecute(commandId: CommandId, ctx)`, `lookupCommand(): CommandId | undefined`, and the palette `onExecute` prop. Narrow the un-typed string edges (Rust `execute-command`, cross-window `LicenseSection` emit, selection-dialog `onCommand`) with a runtime `isCommandId()` guard, never an `as` cast. - Add the `cmdr/no-raw-command-dispatch` ESLint rule (+ RuleTester test) banning string-literal dispatch ids outside the registry, registered under the `cmdr` plugin block. - Add a Rust↔FE drift test parsing `menu/mod.rs` `menu_id_to_command` and the `LicenseSection.svelte` emit, asserting every Rust-emitted id ∈ `COMMAND_IDS`, with cross-pointing comments in both files. - No behavior change; full suite, `desktop-e2e-linux`, and the new tests green.
1 parent 3c8716d commit 5709b50

19 files changed

Lines changed: 680 additions & 32 deletions

‎apps/desktop/eslint-plugins/no-raw-command-dispatch.js‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* ESLint rule: ban string literals as command ids in dispatch calls.
3+
*
4+
* Rationale (invariant A3): the command bus dispatches `CommandId`-typed values
5+
* end to end. A string literal like `handleCommandExecute('file.rename')` sneaks
6+
* a magic string past the type system — it compiles only because the literal
7+
* happens to be assignable to the union today, and silently rots the moment a
8+
* command is renamed. Every dispatch site must pass a `CommandId`-typed value
9+
* (a registry lookup, a narrowed payload, a forwarded prop), not a literal.
10+
*
11+
* This is the command-bus twin of `no-raw-tauri-invoke`: keep magic strings out
12+
* of cross-layer dispatch so renames are caught at compile time.
13+
*
14+
* ## What this rule catches
15+
*
16+
* A string-literal (or template-literal-with-no-expressions) FIRST argument to a
17+
* call whose callee name is a known dispatch entry point:
18+
* - `dispatch('file.rename')`
19+
* - `handleCommandExecute('file.rename', ctx)`
20+
* - `onExecute('file.rename')` / `onCommand('file.rename')` (the prop channels)
21+
* Renamed-import aliases are intentionally NOT chased (same stance as
22+
* `no-raw-tauri-invoke`): an alias is extra effort that reads as suspicious in
23+
* review.
24+
*
25+
* ## What it deliberately does NOT catch
26+
*
27+
* - Calls that pass a non-literal (a variable, a member access, a narrowed
28+
* value). Those are the typed path the rule wants you on.
29+
* - The registry file, test files, and `/test/` dirs (allowed-path fragments):
30+
* the registry IS where command-id literals legitimately live, and tests poke
31+
* the dispatcher with literal ids on purpose.
32+
*
33+
* Opt out per-line if you really must:
34+
*
35+
* // eslint-disable-next-line cmdr/no-raw-command-dispatch -- <reason>
36+
*/
37+
38+
// Path fragments that opt a file out entirely. The registry declares the id
39+
// literals; tests dispatch literal ids on purpose.
40+
const allowedPathFragments = ['/command-registry.ts', '/command-ids.ts', '.test.', '/test/']
41+
42+
// Call-site names that mean "dispatch a command". A literal first argument to any
43+
// of these is a raw command-id string.
44+
const DISPATCH_CALLEES = new Set(['dispatch', 'handleCommandExecute', 'dispatchCommand', 'onExecute', 'onCommand'])
45+
46+
/** @type {import('eslint').Rule.RuleModule} */
47+
export default {
48+
meta: {
49+
type: 'problem',
50+
docs: {
51+
description: 'Pass a `CommandId`-typed value to the command bus, never a raw string literal.',
52+
recommended: true,
53+
},
54+
messages: {
55+
rawDispatch:
56+
"Don't dispatch the string literal `'{{ command }}'`. Pass a `CommandId`-typed value so the id is " +
57+
'checked at compile time and survives a registry rename (invariant A3). Look it up from the registry, ' +
58+
'narrow it with `isCommandId`, or forward an already-typed value. See `lib/commands/CLAUDE.md`.',
59+
},
60+
schema: [],
61+
},
62+
create(context) {
63+
const filename = context.filename || context.getFilename() || ''
64+
if (allowedPathFragments.some((fragment) => filename.includes(fragment))) {
65+
return {}
66+
}
67+
68+
return {
69+
CallExpression(node) {
70+
const callee = node.callee
71+
// Match a bare identifier call (`dispatch(...)`) or a member call
72+
// (`ctx.dispatch(...)`, `explorer.onCommand(...)`) by the final name.
73+
let calleeName
74+
if (callee.type === 'Identifier') {
75+
calleeName = callee.name
76+
} else if (callee.type === 'MemberExpression' && !callee.computed && callee.property.type === 'Identifier') {
77+
calleeName = callee.property.name
78+
} else {
79+
return
80+
}
81+
if (!DISPATCH_CALLEES.has(calleeName)) return
82+
if (node.arguments.length < 1) return
83+
84+
const firstArg = node.arguments[0]
85+
const literalString =
86+
firstArg.type === 'Literal' && typeof firstArg.value === 'string'
87+
? firstArg.value
88+
: firstArg.type === 'TemplateLiteral' && firstArg.expressions.length === 0
89+
? firstArg.quasis[0].value.cooked
90+
: undefined
91+
if (literalString === undefined) return
92+
93+
context.report({
94+
node: firstArg,
95+
messageId: 'rawDispatch',
96+
data: { command: literalString },
97+
})
98+
},
99+
}
100+
},
101+
}

‎apps/desktop/eslint-plugins/no-raw-command-dispatch.test.js‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
import { RuleTester } from 'eslint'
2+
import rule from './no-raw-command-dispatch.js'
3+
4+
// Flat-config RuleTester (ESLint 9+): module source, latest ECMA. RuleTester
5+
// auto-detects Vitest's `describe`/`it` globals and emits one test per case, so
6+
// `run` is called at the top level (it can't be nested inside our own `it`).
7+
const ruleTester = new RuleTester({
8+
languageOptions: { ecmaVersion: 'latest', sourceType: 'module' },
9+
})
10+
11+
ruleTester.run('no-raw-command-dispatch', rule, {
12+
valid: [
13+
// A `CommandId`-typed local is the whole point — never reported.
14+
{
15+
code: `handleCommandExecute(commandId, ctx)`,
16+
filename: 'src/routes/(main)/+page.svelte.ts',
17+
},
18+
// A member access (a narrowed / forwarded value) is fine.
19+
{
20+
code: `dispatch(args.id)`,
21+
filename: 'src/routes/(main)/mcp-listeners.ts',
22+
},
23+
// Forwarding a prop reference (not a call with a literal) is fine.
24+
{
25+
code: `onExecute(id)`,
26+
filename: 'src/lib/command-palette/CommandPalette.svelte.ts',
27+
},
28+
// A literal first arg to some *other* function is unrelated.
29+
{
30+
code: `log.info('file.rename')`,
31+
filename: 'src/routes/(main)/command-dispatch.ts',
32+
},
33+
// The registry file may hold command-id literals — exempt by path.
34+
{
35+
code: `dispatch('file.rename')`,
36+
filename: 'src/lib/commands/command-registry.ts',
37+
},
38+
// The id tuple may hold command-id literals — exempt by path.
39+
{
40+
code: `handleCommandExecute('file.rename')`,
41+
filename: 'src/lib/commands/command-ids.ts',
42+
},
43+
// Test files dispatch literal ids on purpose — exempt by path.
44+
{
45+
code: `handleCommandExecute('file.rename', ctx)`,
46+
filename: 'src/routes/(main)/command-dispatch.test.ts',
47+
},
48+
// A non-dispatch member call with a literal is unrelated.
49+
{
50+
code: `store.subscribe('file.rename')`,
51+
filename: 'src/lib/whatever.ts',
52+
},
53+
],
54+
invalid: [
55+
// Bare-identifier dispatch of a literal.
56+
{
57+
code: `handleCommandExecute('file.rename')`,
58+
filename: 'src/routes/(main)/+page.svelte.ts',
59+
errors: [{ messageId: 'rawDispatch' }],
60+
},
61+
// Literal id with a trailing ctx argument.
62+
{
63+
code: `dispatchCommand('file.rename', ctx)`,
64+
filename: 'src/routes/(main)/+page.svelte.ts',
65+
errors: [{ messageId: 'rawDispatch' }],
66+
},
67+
// `dispatch(...)` with a literal.
68+
{
69+
code: `dispatch('sort.byName')`,
70+
filename: 'src/routes/(main)/mcp-listeners.ts',
71+
errors: [{ messageId: 'rawDispatch' }],
72+
},
73+
// Member-call dispatch entry point (`ctx.onCommand('…')`).
74+
{
75+
code: `ctx.onCommand('selection.selectFiles')`,
76+
filename: 'src/lib/file-explorer/pane/FilePane.svelte.ts',
77+
errors: [{ messageId: 'rawDispatch' }],
78+
},
79+
// Template literal with no expressions is still a constant string.
80+
{
81+
code: 'handleCommandExecute(`file.rename`)',
82+
filename: 'src/routes/(main)/+page.svelte.ts',
83+
errors: [{ messageId: 'rawDispatch' }],
84+
},
85+
],
86+
})

‎apps/desktop/eslint.config.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import noIsolatedTests from './eslint-plugins/no-isolated-tests.js'
3131
import noErrorStringMatch from './eslint-plugins/no-error-string-match.js'
3232
import noRawTauriInvoke from './eslint-plugins/no-raw-tauri-invoke.js'
3333
import noExplorerStateWrites from './eslint-plugins/no-explorer-state-writes.js'
34+
import noRawCommandDispatch from './eslint-plugins/no-raw-command-dispatch.js'
3435
import noArbitrarySleepInE2E from './eslint-plugins/no-arbitrary-sleep-in-e2e.js'
3536

3637
/* global process */
@@ -225,20 +226,24 @@ export default tseslint.config(
225226
// command names should be type-checked, not magic strings.
226227
// Assigning to the explorer store's surface is banned outside the store
227228
// module: state changes go through its named mutators (invariant A2).
229+
// Dispatching a raw command-id string literal is banned: the bus carries
230+
// `CommandId`-typed values so renames are caught at compile time (A3).
228231
files: ['src/**/*.{ts,svelte.ts,svelte}', 'src/**/*.test.ts'],
229232
plugins: {
230233
cmdr: {
231234
rules: {
232235
'no-error-string-match': noErrorStringMatch,
233236
'no-raw-tauri-invoke': noRawTauriInvoke,
234237
'no-explorer-state-writes': noExplorerStateWrites,
238+
'no-raw-command-dispatch': noRawCommandDispatch,
235239
},
236240
},
237241
},
238242
rules: {
239243
'cmdr/no-error-string-match': 'error',
240244
'cmdr/no-raw-tauri-invoke': 'error',
241245
'cmdr/no-explorer-state-writes': 'error',
246+
'cmdr/no-raw-command-dispatch': 'error',
242247
},
243248
},
244249
{

‎apps/desktop/src-tauri/src/menu/mod.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,11 @@ pub enum ViewMode {
194194
/// Maps a menu item ID to its command registry ID and scope.
195195
/// Returns `None` for items handled specially (CheckMenuItems, close-tab, viewer word wrap,
196196
/// tab context menu, context menu file actions, sort items).
197+
///
198+
/// Each `Some(("…", …))` command id here is emitted across IPC and must exist in the
199+
/// frontend `COMMAND_IDS` tuple (`src/lib/commands/command-ids.ts`). The
200+
/// `rust-command-id-drift.test.ts` Vitest test parses this function and asserts that
201+
/// — the IPC boundary is un-typed, so that test is the backstop for drift.
197202
pub fn menu_id_to_command(menu_id: &str) -> Option<(&'static str, CommandScope)> {
198203
match menu_id {
199204
// App-level commands (always emit)

‎apps/desktop/src/lib/command-palette/CommandPalette.svelte‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@
99
* - Blocks keyboard events from propagating to file explorer
1010
*/
1111
import { onDestroy, onMount, tick } from 'svelte'
12-
import { searchCommands, getPaletteCommands, type CommandMatch } from '$lib/commands'
12+
import { searchCommands, getPaletteCommands, type CommandMatch, type CommandId } from '$lib/commands'
1313
import { pruneRecentCommands, pushRecentCommand } from '$lib/app-status-store'
1414
1515
interface Props {
1616
/** Called when user selects a command */
17-
onExecute: (commandId: string) => void
17+
onExecute: (commandId: CommandId) => void
1818
/** Called when palette is closed */
1919
onClose: () => void
2020
}

‎apps/desktop/src/lib/commands/CLAUDE.md‎

Lines changed: 46 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,25 @@ Centralized command registry and fuzzy search engine for the command palette.
44

55
## Files
66

7-
| File | Purpose |
8-
| ---------------------- | ---------------------------------------------------------------------------------------- |
9-
| `types.ts` | `Command`, `CommandMatch`, `CommandScope` types |
10-
| `command-registry.ts` | The `commands` array (single source of truth). `getPaletteCommands()` filter. |
11-
| `fuzzy-search.ts` | `searchCommands(query, recentCommandIds?)` using `@leeoniya/ufuzzy` |
12-
| `index.ts` | Barrel re-export |
13-
| `fuzzy-search.test.ts` | Vitest tests: empty query, exact/fuzzy matches, ranking, index bounds, palette filtering |
7+
| File | Purpose |
8+
| ------------------------------- | -------------------------------------------------------------------------------------------------------------------------- |
9+
| `command-ids.ts` | `COMMAND_IDS` (the `as const` id tuple), the derived `CommandId` union, and the `isCommandId()` boundary guard |
10+
| `types.ts` | `Command`, `CommandMatch`, `CommandScope`, plus `CommandArgs` / `CommandDispatchArgs` (the dispatch arg-tuple shape) |
11+
| `command-registry.ts` | The `commands` array (single source of truth). `getPaletteCommands()` filter. `updateLicenseCommandName()` in-place write. |
12+
| `fuzzy-search.ts` | `searchCommands(query, recentCommandIds?)` using `@leeoniya/ufuzzy` |
13+
| `index.ts` | Barrel re-export |
14+
| `fuzzy-search.test.ts` | Vitest tests: empty query, exact/fuzzy matches, ranking, index bounds, palette filtering |
15+
| `command-registry.test.ts` | Set-equality guard (tuple ↔ registry), `isCommandId`, `updateLicenseCommandName` in-place mutation |
16+
| `command-types.test.ts` | Compile-time `@ts-expect-error` guards for the `CommandId` union and arg-tuple shapes |
17+
| `rust-command-id-drift.test.ts` | Parses `menu/mod.rs` + `LicenseSection.svelte`; asserts every Rust-emitted command id ∈ `COMMAND_IDS` |
1418

1519
## Types
1620

1721
```ts
22+
type CommandId = (typeof COMMAND_IDS)[number] // closed union of every id (command-ids.ts)
23+
1824
interface Command {
19-
id: string // dot-namespaced: 'app.quit', 'file.rename', 'nav.parent'
25+
id: CommandId // dot-namespaced: 'app.quit', 'file.rename', 'nav.parent'
2026
name: string // shown in palette
2127
scope: CommandScope // hierarchical, display-only (does not enforce routing)
2228
showInPalette: boolean
@@ -28,8 +34,31 @@ interface CommandMatch {
2834
command: Command
2935
matchedIndices: number[] // flat char indices in command.name for highlight rendering
3036
}
37+
38+
// Dispatch arg foundation. Every command is arg-less today (→ `void`), so
39+
// `dispatch('file.rename')` needs no second arg. Arg-carrying commands (the
40+
// per-pane MCP variants in later milestones) override their `CommandArgs` entry.
41+
type CommandArgs = { [K in CommandId]: void }
42+
type CommandDispatchArgs<K extends CommandId> = CommandArgs[K] extends void ? [] : [args: CommandArgs[K]]
3143
```
3244
45+
### Typed ids and the dispatch boundary
46+
47+
`COMMAND_IDS` (in `command-ids.ts`) is an `as const` tuple; `CommandId` is its element union. The registry array stays a
48+
**mutable** `Command[]` — not `as const satisfies readonly Command[]` — because `updateLicenseCommandName` rewrites an
49+
entry's `.name` in place, and `getPaletteCommands()` plus the shortcuts conflict detector consume a mutable `Command[]`.
50+
`Command.id: CommandId` enforces tuple ⊇ registry at compile time; the set-equality test enforces registry ⊇ tuple.
51+
52+
`isCommandId(value: string): value is CommandId` narrows the un-typed string edges where ids enter the frontend — the
53+
Rust `execute-command` event payload (`+page.svelte`), the cross-window emit from `LicenseSection.svelte`, and the
54+
selection-dialog `onCommand` prop. Never `as CommandId`-cast at these edges; a stale id would slip to the dispatcher's
55+
switch `default` and silently no-op. The IPC boundary is un-typed (Rust emits a bare `json!`), so
56+
`rust-command-id-drift.test.ts` is the backstop that keeps Rust ids and `COMMAND_IDS` in sync.
57+
58+
`handleCommandExecute(commandId: CommandId, ctx)` is the typed dispatch entry point. Its big `switch` stays (the flat
59+
`Record<CommandId, Handler>` conversion is a deferred, optional follow-up); it's already exhaustive-safe because
60+
`commandId` is the closed union.
61+
3362
`CommandScope` is a union of string literals: `'App'`, `'Main window'`, `'Main window/File list'`,
3463
`'Main window/Brief mode'`, `'Main window/Full mode'`, `'Main window/Network'`, `'Main window/Share browser'`,
3564
`'Main window/Volume chooser'`, `'About window'`, `'Onboarding'`, `'Command palette'`. Scope is documentation-only;
@@ -88,12 +117,16 @@ added an IPC + Tauri-event + Svelte-effect hop between the keystroke and the DOM
88117
89118
## Adding a command
90119
91-
1. Add an entry to the `commands` array in `command-registry.ts`.
92-
2. Add a `case` for its `id` in the `handleCommandExecute` switch in `routes/(main)/command-dispatch.ts`.
93-
3. No changes needed to the palette, fuzzy search, types, or keyboard dispatch. Commands with `showInPalette: true` are
120+
1. Add the id to the `COMMAND_IDS` tuple in `command-ids.ts`. (Skipping this makes the registry entry a compile error,
121+
since `Command.id` is `CommandId`.)
122+
2. Add an entry to the `commands` array in `command-registry.ts`. (Skipping this fails the set-equality test in
123+
`command-registry.test.ts`.)
124+
3. Add a `case` for its `id` in the `handleCommandExecute` switch in `routes/(main)/command-dispatch.ts`.
125+
4. No changes needed to the palette, fuzzy search, or keyboard dispatch. Commands with `showInPalette: true` are
94126
automatically dispatched from keyboard shortcuts via centralized dispatch (`../shortcuts/shortcut-dispatch.ts`).
95-
4. If the command has a native menu item, add a mapping in `menu.rs` (`menu_id_to_command` and `command_id_to_menu_id`)
96-
and add its ID to the `menuCommands` array in `shortcuts-store.ts`.
127+
5. If the command has a native menu item, add a mapping in `menu.rs` (`menu_id_to_command` and `command_id_to_menu_id`)
128+
and add its ID to the `menuCommands` array in `shortcuts-store.ts`. The `rust-command-id-drift.test.ts` test will
129+
fail if `menu_id_to_command` emits an id that isn't in `COMMAND_IDS`.
97130
98131
## Key decisions
99132

0 commit comments

Comments
 (0)
⚡