Skip to content

fix(web): always bind the ⌘K command palette hotkey, including embedded mode - #5569

Merged
github-actions[bot] merged 1 commit into
mainfrom
zeyi/omni-5473-cmd-k-doesnt-work-in-managed-for-desktop-app
Aug 26, 2026
Merged

fix(web): always bind the ⌘K command palette hotkey, including embedded mode#5569
github-actions[bot] merged 1 commit into
mainfrom
zeyi/omni-5473-cmd-k-doesnt-work-in-managed-for-desktop-app

Conversation

@fanzeyi

@fanzeyi fanzeyi commented Aug 26, 2026

Copy link
Copy Markdown
Member

Related issue

Linear: OMNI-5473 (cmd + k doesn't work in managed for desktop app)

Summary

  • The ⌘K palette hotkey was gated off in embedded mode on the theory "⌘K
    belongs to the host page" (introduced with the palette in feat(web-ui): global command palette (Cmd/Ctrl+K) #1386). That host
    ⌘K doesn't exist, so on managed servers the chord was dead everywhere —
    most visibly in the desktop app, where the embed build is the whole window.
  • Drop the isEmbedded gate: the hotkey now binds in every mode. In the
    embedded build the host page is still loaded (CSS-hidden in the desktop
    shell), so the listener binds in the capture phase and claims the chord
    ahead of any host-page listener; the Monaco/terminal early-return still
    lets owned chords through.

Test Plan

  • npx vitest run src/hooks/useCommandPaletteHotkey.test.tsx — 12 pass,
    incl. capture-phase claiming the chord ahead of document-level listeners,
    and focused-Monaco chords still propagating to them.
  • npx vitest run src/shell/AppShell.test.tsx src/shell/AppShell.subagent-nav.test.tsx src/shell/CommandPalette.test.tsx — 129 pass.
  • npm run type-check, oxlint, prettier --check — clean.
  • Manual: desktop app on a managed server URL, press ⌘K → palette opens;
    same URL in a browser tab, ⌘K now opens the Omnigent palette there too.

Demo

Screen.Recording.2026-08-26.at.2.36.48.PM.mov

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Changelog

⌘K now opens the command palette everywhere, including the desktop app on managed servers

Signed-off-by: Zeyi (Rice) Fan zeyi.f@databricks.com

@github-actions github-actions Bot added the size/M Pull request size: M label Aug 26, 2026
@omnigent-ci

omnigent-ci Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Review: fix(web): enable ⌘K command palette in the desktop shell on managed servers

1. Blocking issues

None.

The core logic is sound and I verified it against the surrounding code:

  • enabled = !isEmbedded || isElectronShell() gives the correct truth table: standalone browser (enabled), browser-embedded on a real host page (disabled — host keeps ⌘K), desktop shell embedded (enabled). isElectronShell() correctly probes for the Electron bridge.
  • Moving the listener to the capture phase on window means the embed's handler runs before any host-page listener on document or below, and stopPropagation() prevents the chord from reaching them — exactly what's needed to wrest ⌘K away from the CSS-hidden host page.
  • The focusOwnsHotkey early-return happens before preventDefault/stopPropagation, so Monaco/terminal chords continue to propagate to those surfaces (and, when owned, to the host too). The two new tests correctly exercise both paths (chord claimed vs. chord passed through), and dispatch from a deep/focused node so propagation is realistic.

2. Security vulnerabilities

None. No lockfile, dependency, or extras changes; no auth/boundary/injection surface touched. Purely a client-side keydown routing change.

3. Non-blocking notes

  • useNewSessionHotkey left as !isEmbedded. The new-session hotkey stays disabled in the desktop shell even though the same "embed rendered over a hidden host page" reasoning that justifies enabling ⌘K would seem to apply to it too. This is out of scope for the issue, but if the intent is that the desktop shell fully owns its chords, the asymmetry may be a follow-up. Worth a one-line confirmation that it's deliberate.
  • stopPropagation vs. same-target listeners. Capture-on-window fires first and blocks anything on document and below (which the test covers). It would not block a host listener also registered in the capture phase on window itself, since stopPropagation doesn't stop other listeners on the same target. Given the host's ⌘K listener is described as living on the host page's document, this is almost certainly a non-issue, but if it ever regresses, stopImmediatePropagation would be the tool.
  • Monaco-focused case in desktop shell. When Monaco/terminal owns the chord, the event now propagates through to the hidden host page's listener as well. This matches the prior behavior (⌘K was fully disabled before, so the host always received it) and the host page is inert, so no regression — noting only for completeness.

4. Summary

A tight, well-reasoned bug fix. The capture-phase binding plus the isElectronShell() gate correctly restore ⌘K in the managed desktop shell without disturbing browser-embedded mode or the Monaco/terminal ownership rules, and the two added tests pin down exactly the behavior that matters (claim-ahead-of-host vs. pass-through-when-owned). No correctness or security concerns; the only follow-up worth a glance is whether useNewSessionHotkey should get the same treatment. Ready to merge.


Automated review by Polly · workflow run

…ed mode

## Related issue

Linear: OMNI-5473 (cmd + k doesn't work in managed for desktop app)

## Summary

- The ⌘K palette hotkey was gated off in embedded mode on the theory "⌘K
  belongs to the host page" (introduced with the palette in #1386). That host
  ⌘K doesn't exist, so on managed servers the chord was dead everywhere —
  most visibly in the desktop app, where the embed build is the whole window.
- Drop the `isEmbedded` gate: the hotkey now binds in every mode. In the
  embedded build the host page is still loaded (CSS-hidden in the desktop
  shell), so the listener binds in the capture phase and claims the chord
  ahead of any host-page listener; the Monaco/terminal early-return still
  lets owned chords through.

## Test Plan

- `npx vitest run src/hooks/useCommandPaletteHotkey.test.tsx` — 12 pass,
  incl. capture-phase claiming the chord ahead of document-level listeners,
  and focused-Monaco chords still propagating to them.
- `npx vitest run src/shell/AppShell.test.tsx src/shell/AppShell.subagent-nav.test.tsx src/shell/CommandPalette.test.tsx` — 129 pass.
- `npm run type-check`, `oxlint`, `prettier --check` — clean.
- Manual: desktop app on a managed server URL, press ⌘K → palette opens;
  same URL in a browser tab, ⌘K now opens the Omnigent palette there too.

## Demo

N/A — hotkey behavior fix, no visual change.

## Type of change

- [x] Bug fix
- [ ] Feature
- [ ] UI / frontend change
- [ ] Refactor / chore
- [ ] Docs
- [ ] Test / CI
- [ ] Breaking change

## Test coverage

- [x] Unit tests added / updated
- [ ] Integration tests added / updated
- [ ] E2E tests added / updated
- [ ] Manual verification completed
- [x] Existing tests cover this change
- [ ] Not applicable

## Changelog

⌘K now opens the command palette everywhere, including the desktop app on managed servers

Signed-off-by: Zeyi (Rice) Fan <zeyi.f@databricks.com>
@fanzeyi
fanzeyi force-pushed the zeyi/omni-5473-cmd-k-doesnt-work-in-managed-for-desktop-app branch from ce01003 to 21dc356 Compare August 26, 2026 21:36
@fanzeyi fanzeyi changed the title fix(web): enable ⌘K command palette in the desktop shell on managed servers fix(web): always bind the ⌘K command palette hotkey, including embedded mode Aug 26, 2026
@fanzeyi fanzeyi added skip-e2e-ui-test automerge Automatically Run Merge CI labels Aug 26, 2026
@github-actions
github-actions Bot enabled auto-merge (squash) August 26, 2026 21:41
@github-actions
github-actions Bot merged commit 5af5df7 into main Aug 26, 2026
71 of 76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically Run Merge CI size/M Pull request size: M skip-e2e-ui-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant