Skip to content

Commit 5ab52c0

Browse files
xuyushun441-sysos-zhuangclaude
authored
feat(app-shell): useUrlOverlay primitive + URL-addressable shortcuts dialog (ADR-0054 Phase 2) (#1882)
Generalizes the Phase 1 command-palette fix into a reusable overlay primitive and brings a second navigable overlay to the contract. - **useUrlOverlay(key)** — router-aware hook storing a navigable overlay's open state in a `?<key>=1` URL param: idempotent open (C1), deep-linkable / restore-on-reload / back-forward (C3). `alias`/`value`/`replace` configurable. 8 unit tests (MemoryRouter). - **CommandPaletteProvider** refactored onto useUrlOverlay (`?palette=1`, `?cmdk=1` alias) — behavior unchanged, ~50 lines of bespoke URL logic removed. - **KeyboardShortcutsDialog** is now URL-addressable (`?shortcuts=1`) and gains a click entry in the header Help menu (C2) — it was previously reachable ONLY via the `?` key (now an accelerator) with toggle-as-open `useState`. Adds `data-testid="overlay:keyboard-shortcuts"`. - The shared overlay primitives already forward `data-testid` + emit Radix `data-state`; documented in the README (no code change needed). Verified in-browser (agent-browser / CDP): palette click-open + `?palette=1` deep-link still work post-refactor; the shortcuts dialog opens via the Help-menu click, via `?shortcuts=1` deep-link, and via the `?` accelerator; no a11y errors. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent c3749eb commit 5ab52c0

10 files changed

Lines changed: 332 additions & 83 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
"@object-ui/app-shell": patch
3+
---
4+
5+
feat(app-shell): useUrlOverlay primitive + URL-addressable keyboard-shortcuts dialog (ADR-0054 Phase 2)
6+
7+
Adds `useUrlOverlay(key)` — a reusable, router-aware hook that stores a navigable
8+
overlay's open state in a `?<key>=1` URL param (idempotent open, deep-linkable,
9+
restore-on-reload, back/forward; `alias`/`value`/`replace` configurable). The
10+
command palette is refactored onto it (behavior unchanged: `?palette=1`, `?cmdk=1`
11+
alias). The keyboard-shortcuts dialog becomes URL-addressable (`?shortcuts=1`) and
12+
gains a click entry in the header Help menu — previously it was only reachable via
13+
the `?` key (which remains an accelerator). Generalizes ADR-0054 invariants C1/C3
14+
beyond the Phase 1 reference fix; the shared overlay primitives already carry
15+
`data-testid` + Radix `data-state`, documented in the README.

packages/app-shell/README.md

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -462,6 +462,37 @@ Designed to be deterministic for automated (AI) browser testing — see
462462
input. Value-injection (`el.value = …`) does **not** fire React's `onChange`;
463463
drive it with a real-input / CDP-keystroke driver so the debounced fetch fires.
464464

465+
## URL-addressable overlays (`useUrlOverlay`)
466+
467+
`useUrlOverlay(key)` is the reusable building block behind the command palette's
468+
URL-addressable open state (ADR-0054 C3). It stores a navigable overlay's open
469+
state in a `?<key>=1` search param instead of component `useState`, so the
470+
overlay is deep-linkable, restores on reload, and works with back/forward — and
471+
its open path is idempotent (C1).
472+
473+
```tsx
474+
import { useUrlOverlay } from '@object-ui/app-shell';
475+
476+
function HelpMenu() {
477+
const { open, setOpen, openOverlay } = useUrlOverlay('shortcuts');
478+
// Header button (any component under the router): onClick={openOverlay}
479+
// Dialog (elsewhere, reads the same param): <Dialog open={open} onOpenChange={setOpen}>
480+
// Deep-link that opens on load: /apps/foo?shortcuts=1
481+
}
482+
```
483+
484+
Because state lives in the URL, a trigger and the overlay it controls need no
485+
shared provider or prop-drilling — they just use the same `key`. The
486+
command palette (`?palette=1`, `?cmdk=1` alias) and the keyboard-shortcuts dialog
487+
(`?shortcuts=1`, openable from the Help menu — no longer `?`-key-only) both build
488+
on it. `replace`/`alias`/`value` are configurable.
489+
490+
The shared overlay primitives in `@object-ui/components`
491+
(`Dialog`/`Sheet`/`Drawer`/`Popover`/`DropdownMenu`/`AlertDialog`) already forward
492+
a `data-testid` onto their content element and emit Radix `data-state="open|closed"`,
493+
so overlays are locatable and their open/closed state is machine-readable by
494+
construction (C4).
495+
465496
## Compatibility
466497

467498
- **React:** 18.x or 19.x

packages/app-shell/src/chrome/KeyboardShortcutsDialog.tsx

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* @module
66
*/
77

8-
import { useEffect, useState, useMemo } from 'react';
8+
import { useEffect, useMemo } from 'react';
99
import {
1010
Dialog,
1111
DialogContent,
@@ -14,6 +14,7 @@ import {
1414
DialogDescription,
1515
} from '@object-ui/components';
1616
import { useObjectTranslation } from '@object-ui/i18n';
17+
import { useUrlOverlay } from '../hooks/useUrlOverlay';
1718

1819
interface ShortcutEntry {
1920
keys: string[];
@@ -27,7 +28,10 @@ interface ShortcutGroup {
2728

2829
export function KeyboardShortcutsDialog() {
2930
const { t } = useObjectTranslation();
30-
const [open, setOpen] = useState(false);
31+
// URL-addressable (?shortcuts=1) so the dialog is deep-linkable and openable
32+
// from the header Help menu, not only via the `?` keyboard accelerator (ADR-0054
33+
// C1/C2/C3).
34+
const { open, setOpen, toggleOverlay } = useUrlOverlay('shortcuts');
3135

3236
const shortcutGroups: ShortcutGroup[] = useMemo(() => [
3337
{
@@ -82,17 +86,20 @@ export function KeyboardShortcutsDialog() {
8286

8387
if (e.key === '?' && !e.metaKey && !e.ctrlKey && !e.altKey) {
8488
e.preventDefault();
85-
setOpen(prev => !prev);
89+
toggleOverlay();
8690
}
8791
}
8892

8993
document.addEventListener('keydown', handleKeyDown);
9094
return () => document.removeEventListener('keydown', handleKeyDown);
91-
}, []);
95+
}, [toggleOverlay]);
9296

9397
return (
9498
<Dialog open={open} onOpenChange={setOpen}>
95-
<DialogContent className="sm:max-w-lg max-h-[80vh] overflow-y-auto">
99+
<DialogContent
100+
className="sm:max-w-lg max-h-[80vh] overflow-y-auto"
101+
data-testid="overlay:keyboard-shortcuts"
102+
>
96103
<DialogHeader>
97104
<DialogTitle>{t('console.shortcuts.title')}</DialogTitle>
98105
<DialogDescription>

packages/app-shell/src/context/CommandPaletteProvider.tsx

Lines changed: 22 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -4,41 +4,27 @@
44
* Single source of truth for whether the global ⌘K command palette is open,
55
* plus the shared, idempotent commands used to open / close it.
66
*
7-
* Why this exists — ADR-0054 "UI testability contract", Phase 1:
7+
* Open state is delegated to {@link useUrlOverlay} (`?palette=1`, `?cmdk=1`
8+
* alias), so it is URL-addressable (deep-linkable, restore-on-reload,
9+
* back/forward) per ADR-0054 invariant C3, and every "open" affordance — the
10+
* top-bar search button, a programmatic caller, a deep-link — shares the same
11+
* idempotent `openCommandPalette()` (never a toggle) per C1.
812
*
9-
* - **C1 (idempotent, direct triggers).** Every "open the palette" affordance —
10-
* the top-bar search button, a programmatic caller, a deep-link — calls the
11-
* SAME `openCommandPalette()`, which is idempotent (`setOpen(true)`), never a
12-
* `toggle()`. Re-issuing "open" when already open is a no-op, so an automated
13-
* driver can always *ensure* it is open. The previous header button re-emitted
14-
* a synthetic `⌘K` `KeyboardEvent` and relied on the palette's global listener
15-
* being mounted and the browser/OS not having reserved `⌘K` — which silently
16-
* did nothing under automation (and in `⌘K`-reserving browsers).
17-
* - **C3 (URL-addressable state).** Open state lives in the `?palette=1` search
18-
* param, not component `useState`, so the palette is deep-linkable
19-
* (`/apps/foo?palette=1`), restores on reload, and works with back/forward.
20-
*
21-
* `⌘K` stays an *accelerator*: the keydown handler toggles (close-on-repeat is a
22-
* keyboard nicety), but the OPEN path used by buttons / links / programmatic
13+
* `⌘K` stays an *accelerator*: the keydown handler toggles (close-on-repeat is
14+
* a keyboard nicety), but the OPEN path used by buttons / links / programmatic
2315
* callers is the idempotent `openCommandPalette()`.
2416
*
2517
* @module
2618
*/
2719

2820
import {
2921
createContext,
30-
useCallback,
3122
useContext,
3223
useEffect,
3324
useMemo,
3425
type ReactNode,
3526
} from 'react';
36-
import { useSearchParams } from 'react-router-dom';
37-
38-
/** Canonical URL param reflecting the palette's open state. */
39-
const PALETTE_PARAM = 'palette';
40-
/** Legacy/alias param accepted on read (e.g. `?cmdk=1`). */
41-
const PALETTE_PARAM_ALIAS = 'cmdk';
27+
import { useUrlOverlay } from '../hooks/useUrlOverlay';
4228

4329
export interface CommandPaletteContextValue {
4430
/** Whether the palette is currently open (derived from the URL). */
@@ -55,77 +41,35 @@ export interface CommandPaletteContextValue {
5541

5642
const CommandPaletteContext = createContext<CommandPaletteContextValue | null>(null);
5743

58-
function isOpenParam(params: URLSearchParams): boolean {
59-
return params.get(PALETTE_PARAM) === '1' || params.get(PALETTE_PARAM_ALIAS) === '1';
60-
}
61-
6244
export function CommandPaletteProvider({ children }: { children: ReactNode }) {
63-
const [searchParams, setSearchParams] = useSearchParams();
64-
65-
// Open state is URL-derived (C3): one source of truth that makes the palette
66-
// deep-linkable + restore-on-reload, and makes "ensure open" idempotent for
67-
// free.
68-
const open = isOpenParam(searchParams);
69-
70-
const setOpen = useCallback(
71-
(next: boolean) => {
72-
// Truly idempotent: when already in the target state, do nothing — no
73-
// redundant history navigation.
74-
if (next === isOpenParam(searchParams)) return;
75-
const sp = new URLSearchParams(searchParams);
76-
if (next) {
77-
sp.set(PALETTE_PARAM, '1');
78-
sp.delete(PALETTE_PARAM_ALIAS);
79-
} else {
80-
sp.delete(PALETTE_PARAM);
81-
sp.delete(PALETTE_PARAM_ALIAS);
82-
}
83-
// `replace` — toggling a transient overlay shouldn't pile up history.
84-
setSearchParams(sp, { replace: true });
85-
},
86-
[searchParams, setSearchParams],
87-
);
88-
89-
const openCommandPalette = useCallback(() => setOpen(true), [setOpen]);
90-
const closeCommandPalette = useCallback(() => setOpen(false), [setOpen]);
91-
92-
// Toggle uses the functional updater so it never depends on `open` — keeping
93-
// the keydown listener below stable across navigations.
94-
const toggleCommandPalette = useCallback(() => {
95-
setSearchParams(
96-
(prev) => {
97-
const sp = new URLSearchParams(prev);
98-
if (isOpenParam(sp)) {
99-
sp.delete(PALETTE_PARAM);
100-
sp.delete(PALETTE_PARAM_ALIAS);
101-
} else {
102-
sp.set(PALETTE_PARAM, '1');
103-
sp.delete(PALETTE_PARAM_ALIAS);
104-
}
105-
return sp;
106-
},
107-
{ replace: true },
108-
);
109-
}, [setSearchParams]);
45+
const { open, setOpen, openOverlay, closeOverlay, toggleOverlay } = useUrlOverlay('palette', {
46+
alias: 'cmdk',
47+
});
11048

11149
// ⌘K / Ctrl+K accelerator. Lives here (not in CommandPalette) so the command
11250
// and its shortcut share one definition and one open-state source. Toggle is
11351
// fine for the *keyboard* (close-on-repeat); buttons/links/programmatic open
114-
// paths use the idempotent openCommandPalette().
52+
// paths use the idempotent openOverlay().
11553
useEffect(() => {
11654
const onKeyDown = (e: KeyboardEvent) => {
11755
if (e.key === 'k' && (e.metaKey || e.ctrlKey)) {
11856
e.preventDefault();
119-
toggleCommandPalette();
57+
toggleOverlay();
12058
}
12159
};
12260
document.addEventListener('keydown', onKeyDown);
12361
return () => document.removeEventListener('keydown', onKeyDown);
124-
}, [toggleCommandPalette]);
62+
}, [toggleOverlay]);
12563

12664
const value = useMemo<CommandPaletteContextValue>(
127-
() => ({ open, openCommandPalette, closeCommandPalette, toggleCommandPalette, setOpen }),
128-
[open, openCommandPalette, closeCommandPalette, toggleCommandPalette, setOpen],
65+
() => ({
66+
open,
67+
openCommandPalette: openOverlay,
68+
closeCommandPalette: closeOverlay,
69+
toggleCommandPalette: toggleOverlay,
70+
setOpen,
71+
}),
72+
[open, openOverlay, closeOverlay, toggleOverlay, setOpen],
12973
);
13074

13175
return (
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
import { describe, it, expect } from 'vitest';
10+
import { renderHook, act } from '@testing-library/react';
11+
import { MemoryRouter, useLocation } from 'react-router-dom';
12+
import type { ReactNode } from 'react';
13+
import { useUrlOverlay } from '../useUrlOverlay';
14+
15+
function wrapperFor(initial: string) {
16+
return function Wrapper({ children }: { children: ReactNode }) {
17+
return <MemoryRouter initialEntries={[initial]}>{children}</MemoryRouter>;
18+
};
19+
}
20+
21+
function useHarness(key: string, opts?: Parameters<typeof useUrlOverlay>[1]) {
22+
const controls = useUrlOverlay(key, opts);
23+
const { search } = useLocation();
24+
return { ...controls, search };
25+
}
26+
27+
describe('useUrlOverlay', () => {
28+
it('is closed when the param is absent', () => {
29+
const { result } = renderHook(() => useHarness('palette'), { wrapper: wrapperFor('/') });
30+
expect(result.current.open).toBe(false);
31+
});
32+
33+
it('is open when the param is present on first render (deep-link)', () => {
34+
const { result } = renderHook(() => useHarness('palette'), {
35+
wrapper: wrapperFor('/apps/foo?palette=1'),
36+
});
37+
expect(result.current.open).toBe(true);
38+
});
39+
40+
it('openOverlay writes ?key=1 and is idempotent', () => {
41+
const { result } = renderHook(() => useHarness('palette'), { wrapper: wrapperFor('/') });
42+
43+
act(() => result.current.openOverlay());
44+
expect(result.current.open).toBe(true);
45+
expect(result.current.search).toBe('?palette=1');
46+
47+
// Idempotent: calling again leaves it open with the same URL (no toggle).
48+
act(() => result.current.openOverlay());
49+
expect(result.current.open).toBe(true);
50+
expect(result.current.search).toBe('?palette=1');
51+
});
52+
53+
it('closeOverlay removes the param', () => {
54+
const { result } = renderHook(() => useHarness('palette'), {
55+
wrapper: wrapperFor('/?palette=1'),
56+
});
57+
expect(result.current.open).toBe(true);
58+
act(() => result.current.closeOverlay());
59+
expect(result.current.open).toBe(false);
60+
expect(result.current.search).toBe('');
61+
});
62+
63+
it('toggleOverlay flips open state', () => {
64+
const { result } = renderHook(() => useHarness('palette'), { wrapper: wrapperFor('/') });
65+
act(() => result.current.toggleOverlay());
66+
expect(result.current.open).toBe(true);
67+
act(() => result.current.toggleOverlay());
68+
expect(result.current.open).toBe(false);
69+
});
70+
71+
it('preserves unrelated params when toggling', () => {
72+
const { result } = renderHook(() => useHarness('palette'), {
73+
wrapper: wrapperFor('/?tab=details'),
74+
});
75+
act(() => result.current.openOverlay());
76+
expect(result.current.search).toContain('tab=details');
77+
expect(result.current.search).toContain('palette=1');
78+
act(() => result.current.closeOverlay());
79+
expect(result.current.search).toBe('?tab=details');
80+
});
81+
82+
it('reads an alias param as open and normalizes it on write', () => {
83+
const { result } = renderHook(() => useHarness('palette', { alias: 'cmdk' }), {
84+
wrapper: wrapperFor('/?cmdk=1'),
85+
});
86+
// alias counts as open
87+
expect(result.current.open).toBe(true);
88+
// closing clears both the canonical key and the alias
89+
act(() => result.current.closeOverlay());
90+
expect(result.current.open).toBe(false);
91+
expect(result.current.search).toBe('');
92+
});
93+
94+
it('writes the canonical key (not the alias) when opening', () => {
95+
const { result } = renderHook(() => useHarness('palette', { alias: 'cmdk' }), {
96+
wrapper: wrapperFor('/'),
97+
});
98+
act(() => result.current.openOverlay());
99+
expect(result.current.search).toBe('?palette=1');
100+
});
101+
});

packages/app-shell/src/hooks/index.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@ export { useObjectActions } from './useObjectActions';
1616
export { useRecentItems, type RecentItem } from './useRecentItems';
1717
export { useRecordApprovals, type ApprovalRequestLite } from './useRecordApprovals';
1818
export { useResponsiveSidebar } from './useResponsiveSidebar';
19+
export {
20+
useUrlOverlay,
21+
type UseUrlOverlayOptions,
22+
type UrlOverlayControls,
23+
} from './useUrlOverlay';
1924
export { useTrackRouteAsRecent, type UseTrackRouteAsRecentOptions } from './useTrackRouteAsRecent';
2025
export {
2126
sanitizeChatMessagesForCache,

0 commit comments

Comments
 (0)