fix(desktop): quick-help drawer advertised terminal-only commands - #66646
fix(desktop): quick-help drawer advertised terminal-only commands#66646joelbrilliant wants to merge 3 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
{
"event": "APPROVE",
Code Review Summary
Verdict: Approved
Fixes the quick-help drawer (/help) advertising terminal-only commands (/clear, /details, /copy, /quit) that the desktop cannot actually run, causing errors when tapped. Replaces the hardcoded list with DESKTOP_COMMON_COMMANDS from lib/desktop-slash-commands.ts. New test file explicitly guards against terminal-only commands appearing in the DOM.
Looks Good
- Targeted removal of the hardcoded list
- Regression tests: verifies all
DESKTOP_COMMON_COMMANDentries render, confirms terminal-only commands are absent, and validates all listed commands are desktop-runnable
Reviewed by Hermes Agent",
"comments": []
}
|
Thanks for the focused desktop fix. The premise is verified on current main: The PR replaces that divergent drawer list with entries resolved from the desktop command spec, retains localized descriptions where available, and adds both spec-level and rendered-DOM regression guards. Its base SHA matches current main, so salvage should be mechanical. Automated hermes-sweeper review. |
…c table The composer's ? quick-help drawer hardcoded /clear, /details, /copy, and /quit as "common commands" — 4 of its 6 entries sit in NO_DESKTOP_SURFACE.terminal, so tapping any of them errored with "only available in the terminal interface". The drawer advertised commands the desktop refuses to run, and i18n even shipped '/quit': 'exit hermes'. Derive the advertised list from the spec table instead: - desktop-slash-commands.ts exports DESKTOP_COMMON_COMMANDS (/help, /new, /resume, /compress, /usage, /status), filtered through the spec table so a command without a real desktop surface can never be advertised - help-hint.tsx consumes it, with locale descriptions falling back to the spec-table English so partially translated locales degrade gracefully - retire the dead /clear, /details, /copy, /quit commandDescs entries in en, ja, zh, and zh-hant - tests pin the advertised list and assert every entry passes isDesktopSlashCommand() and isDesktopSlashSuggestion(), so the regression cannot return Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mounts the real HelpHint component and asserts it renders one row per DESKTOP_COMMON_COMMAND, that none of the retired terminal-only commands (/clear, /details, /copy, /quit) reach the DOM, and that every advertised command passes isDesktopSlashCommand(). Complements the unit test on the derived list with a DOM-level regression pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Arabic locale predated this branch, so its commandDescs still advertised /clear, /details, /copy and /quit - the exact terminal-only commands the desktop refuses, which is the bug this PR exists to fix. Arabic users would have kept seeing the misleading drawer. Aligned with ja/zh/zh-hant: keep /help and /resume, fall back to English for the desktop-supported additions. tsc clean, help-hint + i18n suites 31 passed, lint clean.
fbca74d to
aa29610
Compare
|
Rebased onto current main (1728 behind, no conflicts), plus one real gap the rebase exposed. Arabic landed in the catalog after this branch was cut, and Aligned it with ja/zh/zh-hant: keep Verified: tsc clean, help-hint + i18n suites 31 passed, lint clean. |
defineLocale() deep-merges a locale's overrides onto en and makes every key optional, so a partially translated locale falls back to English. That is deliberate. What it hides is the opposite case: a locale key English NO LONGER HAS. The stale override survives the merge and wins over English. TypeScript cannot catch it. Blocks like composer.commandDescs are typed Record<string, string>, so any key satisfies the type - verified by re-adding a removed key and watching tsc --noEmit still exit 0. That is not cosmetic. NousResearch#66646 removed terminal-only commands (/clear, /details, /copy, /quit) from the desktop quick-help drawer because the desktop refuses them, but a locale still defining those keys kept advertising commands that do not work - the very bug the change fixed, staying live in one language while every other locale was corrected. The guard fails on exactly that: a key a locale defines that English does not have. Missing keys are NOT flagged, since falling back is the design. Two exemptions, both documented, because the guard cannot protect them: - blocks English leaves deliberately empty (messaging.platformIntro is {}) - runtime contribution registries (sidebar.nav, SIDEBAR_NAV_AREA in app/routes.ts), where plugins register the ids Running it on current main found two genuinely dead keys in ar.ts, both removed here after checking the app for any remaining reference: - keybinds.actions 'view.closePreviewTab' - no such action exists - onboarding.flowSubtitles 'loopback' - desktop handles pkce, device_code and external only 39 passed in src/i18n, tsc clean, lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bug, by upstream's own spec table.
apps/desktop/src/app/chat/composer/help-hint.tsx:8hardcodedCOMMON_COMMAND_KEYS = ['/help', '/clear', '/resume', '/details', '/copy', '/quit']. Four of those six are declared terminal-only by the desktop's own command spec —NO_DESKTOP_SURFACE.terminalatapps/desktop/src/lib/desktop-slash-commands.ts:187-219lists/clear,/copy,/details,/quit— so tapping them renders "is only available in the terminal interface" (UNAVAILABLE_MESSAGE.terminal, desktop-slash-commands.ts:243). The i18n bundle even ships'/quit': 'exit hermes'(apps/desktop/src/i18n/en.ts:1733-1740), an instruction the desktop refuses to run.Fix. Stop maintaining a parallel hand-written list; derive it from the spec table the file itself calls "THE source of truth".
desktop-slash-commands.tsexportsDESKTOP_COMMON_COMMANDS(/help,/new,/resume,/compress,/usage,/status), resolved through the spec table with anything lacking a real desktop surface filtered out.help-hint.tsxconsumes it; locale descriptions fall back to the spec-table English (the old fallback at help-hint.tsx:30 was?? ''— a blank row), so partially translated locales degrade gracefully. DeadcommandDescskeys retired from en/ja/zh/zh-hant; new-command translations left to translators, the English fallback covers them meanwhile.Tests.
apps/desktop/src/lib/desktop-slash-commands.test.ts+2: the advertised list is pinned, and every entry must passisDesktopSlashCommand()andisDesktopSlashSuggestion()— re-adding a terminal-only command fails CI.apps/desktop/src/app/chat/composer/help-hint.test.tsx(new, +3): mounts the realHelpHintand asserts it renders a row per advertised command, that none of the retired/clear/details/copy/quitreach the DOM, and that every rendered command is desktop-runnable. Scoped run 21 passed. Fullapps/desktopsuite: 218 files, 1864 passed / 1 skipped, 0 failed.npx tsc -p apps/desktop --noEmitclean.Overlap. No open PR touches
help-hint.tsx. #40988 touchesdesktop-slash-commands.tsbut in extension-command classification (isDesktopSlashExtensionCommand); this change adds a derived export and doesn't alter that logic — no conflict.