Skip to content

Commit eef116e

Browse files
fix(app-shell): on a phone, Back with the nav drawer open closes the drawer and stays on the page (#29323)
domain:ui#4 · zhuangjianguo · session_01QsR2jR7ZFkpqmm4U8S1f9y <!-- claude-projects-attribution: {"github_login":"zhuangjianguo"} --> _Requested by **Steedos** · [project thread](https://claude.ai/code/project/chan_018ETKaNwMZCNRYD7D8Q9Ubb?thread=cmsg_018ETKaNwMZCNRYD7D8Q9UbbHny66pXvhYZi1hTZTHzAkc)_ Fixes #29131 Clause-②: no Metadata-protocol: none ## 维护者速读 - **改了什么**:手机宽度下,侧边抽屉打开时按返回键(浏览器返回或 Android 返回手势)只关抽屉、留在原页面。抽屉打开时占一条路由历史记录;用其它方式关掉抽屉(点遮罩、Esc、侧栏按钮)会把这条记录退掉;在抽屉里点入口则替换这条记录,所以从新页面返回会回到打开抽屉前的页面。刷新后抽屉保持关闭,不留抽屉记录。桌面宽度不写任何历史。 - **为什么改**:#29075 审计实测,CRM 与 showcase 在 375/390 px 下 4/4 次:打开抽屉再按返回,直接离开列表跳到 /home。手机用户(含 WebView 外壳的 Android 返回)习惯用返回键关菜单。 - **风险与回滚**:低到中。只在 `@object-ui/app-shell` 新增一个 hook,在 `ConsoleLayout` 挂一行;不改 `UnifiedSidebar.tsx` 和任何 `components/src/ui/**` 原件。风险点是历史栈:手机上打开抽屉多一条同 URL 的记录,抽屉内链接点击被改为 replace 导航。回滚即 revert 本 PR。 - **先看**:① `ui/packages/app-shell/src/layout/usePhoneNavDrawerHistory.ts` 的三个 effect,尤其抽屉内链接的捕获监听与 popstate 关闭;② `ConsoleLayout.tsx` 的挂载行;③ `urlParams.ts` 新常量 `NAV_DRAWER_OPEN_STATE`。 - **两分钟验证**:`pnpm dev:crm`,浏览器 375 px 打开 Leads 列表,点 ☰ 打开抽屉,按浏览器返回:抽屉关上,地址仍是 Leads 列表;再按一次返回才离开。 - **需要你做的**:无,席位落地。 ## What changes At phone width (below 768 px) the Console's sidebar is a drawer over the page. Back with the drawer open used to leave the page; now it closes the drawer and the URL stays the page it was on. Implemented as the seat ruled on the card: one router-owned history entry while the drawer is open, no popstate guard. - **New hook `usePhoneNavDrawerHistory`** (`ui/packages/app-shell/src/layout/`), mounted once in `ConsoleLayoutInner` beside `useResponsiveSidebar`, inside the sidebar provider. It acts only while `useSidebar().isMobile` is true: - Opening the drawer (trigger, edge swipe, shortcut) pushes one entry through the router with the same pathname, search and hash and `NAV_DRAWER_OPEN_STATE: true` in its state. - Arriving on an entry without the flag while the drawer is open closes the drawer (Back). A `popstate` listener closes it too: `BrowserRouter` commits the push and a Back pressed before the push committed as one transition, so the router reports no location change at all in that case (found by the live spec, see Tests). - Every other close (outside tap, Escape, the trigger) calls `navigate(-1)`, only when the current entry carries the flag. - A link chosen inside the drawer replaces the flagged entry: a capture-phase click listener on the document calls `navigate(to, { replace: true })` and prevents the default, so `Link` skips its own push. A link to the page already shown pops the entry instead. This covers the navigation, favorite and recent rows and the drawer's "All apps" link without editing `UnifiedSidebar.tsx` or `@object-ui/layout`. - A flagged entry this document did not push (a reload keeps `history.state`) is replaced by one without the flag, and the drawer stays closed. Forward to an entry this document pushed reopens the drawer. - **`NAV_DRAWER_OPEN_STATE`** sits in `urlParams.ts` beside `RECORD_RELATED_ENLARGE_PUSHED_STATE`. Neither the hook nor the constant is exported from the package entry. - Desktop is unchanged: with `isMobile` false the hook writes no history. - No metadata key, no server change, no edit to `UnifiedSidebar.tsx`, `sidebar.tsx` or any other `components/src/ui/**` primitive. The native shell's Android Back goes through the WebView's history and needs no bridge code. ## Tests - **Unit** (`usePhoneNavDrawerHistory-29131.test.tsx`, memory router, real `SidebarProvider`, real `UnifiedSidebar` and `NavigationRenderer` links, the sidebar's own route-change close running beside the hook): Back closes the drawer on the same URL; Escape pops the entry and Back goes to the previous page; a nav entry replaces the drawer entry and Back returns to the page the drawer was opened on; the "All apps" link (which closes the drawer itself) does not pop as well; a reload on a flagged entry leaves the drawer closed with the flag gone; Forward reopens the drawer; CONTROL at 1280 px: toggling the sidebar commits no location. - **Live e2e** (`ui/e2e/live/phone-nav-drawer-back-29131.spec.ts`, Chromium with touch at 375 and 390 px, real history): Back closes the drawer and stays on the list; an outside tap pops the entry and Back goes to the previous page; a nav entry then Back returns to the list; a reload leaves the drawer closed with no flagged entry and the same history length; CONTROL at 1280 px: no history entry. Runs in `test:e2e:live:nightly`, not the `:ci` allowlist (see Acceptance notes). <details><summary>Reverse checks and ablations</summary> One-shot, nothing left in the tree (each restore checked: blob equals HEAD and `git diff HEAD` empty). - E2E with the hook unmounted (the `ConsoleLayout` mount line taken from the base commit, Console rebuilt and served by `vite preview` against a showcase backend): 8 of 8 phone tests red, desktop CONTROL green. - E2E with only the `popstate` close ablated (`scripts/ablation-replace.mjs`, anchor 1 to 0): exactly the two "Back with the drawer open" tests red (375 and 390), the other 7 green. - Unit with the hook body ablated (early return): 6 red, desktop CONTROL green. - Unit with the link capture listener ablated: exactly the two link tests red (nav entry, "All apps"), 5 green. </details> ## Acceptance notes **Three deviations from the ruling's text, accepted by the seat:** 1. **A `popstate` close.** The ruling's location-driven close misses a Back pressed before the push commits, because `BrowserRouter` then reports no location change. Measured red in the live spec; the ablation above reds exactly that pair. 2. **Forward to a drawer entry this document pushed reopens the drawer.** Otherwise reopening after a Forward would push a second flagged entry, and Back would stop on a no-op. 3. **A drawer link to the page already shown pops the drawer entry** instead of replacing it, for the same no-op reason. - **The race the live spec found.** The first live run was red on "Back with the drawer open" at both widths while a manual run passed: the spec pressed Back right after the push. `BrowserRouter` commits locations inside `startTransition`, so the push and the pop collapsed into one commit with an unchanged location and the drawer stayed open. The `popstate` close fixes it; the ablation above shows it is the load-bearing part. - **Mounted in `ConsoleLayout.tsx`, not `UnifiedSidebar.tsx`**, so this PR does not touch the file #29141 edits. - **A nav entry of kind `action` that navigates in code** pushes as before (only links are intercepted). Back from that page returns to the drawer entry, and the drawer reopens there (the Forward rule), then the next Back closes it. - **After a reload with the drawer open** the flagged entry is replaced in place, as ruled, so the history keeps two entries with the same URL; one Back after such a reload stays on the page. - **The live spec runs in `test:e2e:live:nightly`** (claim amendment 6576569654 added that one `ui/package.json` script line). It stays out of `:ci` for that job's two-minute budget; promotion is a later call once nightly has a run population for it. - NOT MEASURED: Android Back inside the native shell, reason: no native shell in this container. It goes through the same WebView history the browser's Back exercises. ## Verification Step 07 of the card on the real Console: production build served at `/_console` by `objectstack dev --fresh` (`examples/app-crm` on its lead list, `examples/app-showcase` on its task list), Chromium with `isMobile` and touch, DPR 2. "Before" is the Console built without the mount line (equivalent to `main` 07eaec2); "after" is the final hook. Script and every screenshot: project files `issue-29131/` (`step07.cjs`, `before/`, `after/`). | Check | crm 375 | crm 390 | showcase 375 | showcase 390 | |---|---|---|---|---| | 07a Back with the drawer open closes it, URL unchanged | FAIL → pass | FAIL → pass | FAIL → pass | FAIL → pass | | 07b outside tap closes it, then Back goes to the previous page | pass → pass | pass → pass | pass → pass | pass → pass | | 07c a nav entry, then Back returns to the list, drawer closed | pass → pass | pass → pass | pass → pass | pass → pass | | 07d reload with the drawer open: closed, no flagged entry, same history length | pass → pass | pass → pass | pass → pass | pass → pass | | 1280 px: toggling the sidebar writes no history | pass → pass | pass → pass | pass → pass | pass → pass | Before, 07a: `{"urlBefore":".../_console/apps/com.example.crm/crm_lead","urlAfter":".../_console/home","drawerStill":false}`, the card's reading, in all four runs. After: `urlAfter` equals `urlBefore`, `drawerStill` false, flag false. <details><summary>Local gates at head 1789809 (merged main 7788eed)</summary> The 34 families derived for the six source paths ran at 1789809. Head a76bb1a merged ten more `main` commits (to 504ba51) touching none of those paths. Head 9626c78 adds the nightly script line in `ui/package.json`; it derives 12 more families, run there: 12 of 12 exit 0. `--ran` over all 46: 46 derived, 46 run, 0 NOT-MEASURED (a derived zero). - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`: 34 families for the 6 changed paths. Each ran with its exit code captured before any pipe: 34 of 34 exit 0, `check:type-check-debt` and `pnpm --dir ui exec vitest run scripts/` (169 files, 4900 tests) under `os-verify-lock`. `--ran` reconciliation: 34 derived, 34 run, 0 NOT-MEASURED (a derived zero). - `node scripts/check-sdui-manifest.mjs`: exit 0 (and `--self-test` exit 0). - Unit, on the merged head: `pnpm --dir ui exec vitest run` over the new file plus every `ConsoleLayout.*` and `UnifiedSidebar.*` test in `app-shell/src/layout/__tests__`: 31 files, 152 tests passed. - `pnpm --filter @object-ui/app-shell run type-check` (source and test configs): exit 0. `pnpm --dir ui run type-check:e2e`: exit 0. ESLint on the changed files: 0 errors (4 warnings, all on existing `ConsoleLayout.tsx` lines). - Live e2e on the final hook (Console `vite preview`, showcase backend): 9 of 9 passed. - Diff: 626 changed lines in 7 files, under 3000. Families CI runs are read from this PR's check runs. </details> --- _Generated by [Claude Code](https://claude.ai/code/session_01QsR2jTvKAh25Doey2NM9Qw)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 239e3a8 commit eef116e

7 files changed

Lines changed: 625 additions & 1 deletion

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+
fix(app-shell): on a phone, Back with the navigation drawer open closes the drawer and stays on the page
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
Below 768 px the Console's sidebar is a drawer over the page. Back with the drawer open (the browser's Back, or Android's back gesture in a WebView host) used to leave the page the drawer was opened on. The open drawer now owns one history entry:
11+
12+
- **Back closes the drawer.** The URL stays the page it was on; the next Back leaves the page as before.
13+
- **No stray entries.** Closing the drawer any other way (an outside tap, Escape, the sidebar trigger) removes its entry, so Back never stops on a page that does not change. Choosing an entry in the drawer replaces it, so Back from the new page returns to the page the drawer was opened on.
14+
- **A reload does not reopen it.** A reload with the drawer open comes back with the drawer closed and no drawer entry left; Forward to a drawer entry reopens the drawer.
15+
- Wider windows are unchanged: opening and closing the sidebar there writes no history.
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
import { test, expect, type Page } from '@playwright/test';
2+
3+
/**
4+
* Live e2e: at phone width, Back with the nav drawer open closes the drawer
5+
* and stays on the page (objectstack-ai/objectstack#29131). Before, Back left
6+
* the list for the page behind it.
7+
*
8+
* The drawer owns one history entry while it is open
9+
* (`usePhoneNavDrawerHistory` in `@object-ui/app-shell`); the unit pins of
10+
* every path are beside the hook, in a memory router. This file drives the
11+
* real Console in Chromium with touch, against the real history: Back is the
12+
* browser's own, the close is an outside tap, and the reload is a real one,
13+
* which keeps `history.state`.
14+
*
15+
* In `test:e2e:live:nightly`, not the required `:ci` allowlist.
16+
*/
17+
18+
const APP = 'com.example.showcase';
19+
const LIST = `/apps/${APP}/showcase_task`;
20+
const DRAWER = '[data-sidebar="sidebar"][data-mobile="true"]';
21+
const TRIGGER = 'button[data-sidebar="trigger"]';
22+
const FLAG = '__navDrawerOpen';
23+
24+
async function openList(page: Page) {
25+
// A page behind the list, so Back has somewhere to go.
26+
await page.goto('/home');
27+
await expect(page.locator('#root')).not.toBeEmpty();
28+
await page.goto(LIST);
29+
await expect(page.locator(TRIGGER).first()).toBeVisible();
30+
}
31+
32+
async function openDrawer(page: Page) {
33+
await page.locator(TRIGGER).first().tap();
34+
await expect(page.locator(DRAWER)).toBeVisible();
35+
await expect.poll(() => drawerFlag(page)).toBe(true);
36+
}
37+
38+
function drawerFlag(page: Page): Promise<boolean> {
39+
return page.evaluate((flag) => {
40+
const usr = (window.history.state as { usr?: Record<string, unknown> } | null)?.usr;
41+
return !!usr && usr[flag] === true;
42+
}, FLAG);
43+
}
44+
45+
function historyLength(page: Page): Promise<number> {
46+
return page.evaluate(() => window.history.length);
47+
}
48+
49+
for (const width of [375, 390]) {
50+
test.describe(`phone nav drawer and Back at ${width} px`, () => {
51+
test.use({ viewport: { width, height: 844 }, isMobile: true, hasTouch: true });
52+
53+
test('Back with the drawer open closes it and stays on the list', async ({ page }) => {
54+
await openList(page);
55+
const url = page.url();
56+
await openDrawer(page);
57+
58+
await page.goBack();
59+
60+
await expect(page.locator(DRAWER)).toBeHidden();
61+
expect(page.url()).toBe(url);
62+
expect(await drawerFlag(page)).toBe(false);
63+
});
64+
65+
test('an outside tap closes the drawer and pops its entry: Back goes to the previous page', async ({ page }) => {
66+
await openList(page);
67+
const before = await historyLength(page);
68+
await openDrawer(page);
69+
70+
// Right of the 288 px drawer, on the overlay.
71+
await page.touchscreen.tap(width - 20, 420);
72+
await expect(page.locator(DRAWER)).toBeHidden();
73+
await expect.poll(() => drawerFlag(page)).toBe(false);
74+
expect(page.url()).toContain(LIST);
75+
76+
await page.goBack();
77+
await expect(page).toHaveURL(/\/home(?:[?#]|$)/);
78+
// The popped entry is forward history now, not an extra stop behind the list.
79+
expect(await historyLength(page)).toBe(before + 1);
80+
});
81+
82+
test('choosing a nav entry replaces the drawer entry: Back returns to the list', async ({ page }) => {
83+
await openList(page);
84+
const listUrl = page.url();
85+
await openDrawer(page);
86+
87+
const target = await page.evaluate(({ drawer, list }) => {
88+
const anchors = Array.from(document.querySelectorAll<HTMLAnchorElement>(`${drawer} a[href]`));
89+
const other = anchors.find((a) => {
90+
const path = new URL(a.href).pathname;
91+
return path.includes('/apps/') && !path.endsWith(list);
92+
});
93+
return other?.getAttribute('href') ?? null;
94+
}, { drawer: DRAWER, list: LIST });
95+
expect(target, 'the drawer offers another entry').toBeTruthy();
96+
97+
await page.locator(`${DRAWER} a[href="${target}"]`).first().tap();
98+
await expect(page.locator(DRAWER)).toBeHidden();
99+
await expect.poll(() => new URL(page.url()).pathname).toBe(target);
100+
101+
await page.goBack();
102+
await expect.poll(() => page.url()).toBe(listUrl);
103+
await expect(page.locator(DRAWER)).toBeHidden();
104+
expect(await drawerFlag(page)).toBe(false);
105+
});
106+
107+
test('a reload with the drawer open leaves it closed, with no flagged entry left', async ({ page }) => {
108+
await openList(page);
109+
await openDrawer(page);
110+
const length = await historyLength(page);
111+
112+
await page.reload();
113+
await expect(page.locator(TRIGGER).first()).toBeVisible();
114+
115+
await expect.poll(() => drawerFlag(page)).toBe(false);
116+
await expect(page.locator(DRAWER)).toBeHidden();
117+
expect(page.url()).toContain(LIST);
118+
expect(await historyLength(page)).toBe(length);
119+
});
120+
});
121+
}
122+
123+
test.describe('desktop sidebar', () => {
124+
test.use({ viewport: { width: 1280, height: 800 } });
125+
126+
test('CONTROL — opening and closing the sidebar writes no history entry', async ({ page }) => {
127+
await openList(page);
128+
const url = page.url();
129+
const length = await historyLength(page);
130+
131+
await page.locator(TRIGGER).first().click();
132+
await page.locator(TRIGGER).first().click();
133+
134+
expect(await historyLength(page)).toBe(length);
135+
expect(page.url()).toBe(url);
136+
});
137+
});

‎ui/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@
111111
"test:e2e:ui": "playwright test --ui",
112112
"test:e2e:live": "playwright test --config=playwright.live.config.ts",
113113
"test:e2e:live:ci": "playwright test --config=playwright.live.config.ts screen-flow.spec.ts action-modal.spec.ts master-detail.spec.ts saved-view-filter.spec.ts studio-object-designer.spec.ts studio-editor.spec.ts record-form-direct-url.spec.ts list-row-action-cel.spec.ts",
114-
"test:e2e:live:nightly": "playwright test --config=playwright.live.config.ts cascading-options.spec.ts declared-sharing-rules-reach-13091.spec.ts field-conditional-rules.spec.ts form-view-subforms.spec.ts grid-conditional-rules.spec.ts grid-file-upload.spec.ts grid-parent-rules.spec.ts inline-edit-polish-2572.spec.ts read-only-member-record.spec.ts record-history-display.spec.ts required-when-submit.spec.ts summary-rollup.spec.ts user-filters.spec.ts",
114+
"test:e2e:live:nightly": "playwright test --config=playwright.live.config.ts cascading-options.spec.ts declared-sharing-rules-reach-13091.spec.ts field-conditional-rules.spec.ts form-view-subforms.spec.ts grid-conditional-rules.spec.ts grid-file-upload.spec.ts grid-parent-rules.spec.ts inline-edit-polish-2572.spec.ts read-only-member-record.spec.ts record-history-display.spec.ts required-when-submit.spec.ts summary-rollup.spec.ts user-filters.spec.ts phone-nav-drawer-back-29131.spec.ts",
115115
"test:e2e:import-harness": "playwright test --config=playwright.import-harness.config.ts",
116116
"test:e2e:import-console": "playwright test --config=playwright.import-console.config.ts"
117117
},

‎ui/packages/app-shell/src/layout/ConsoleLayout.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import { ConsoleNotificationBanners } from './ConsoleNotificationBanners.js';
3030
import { UnifiedSidebar } from './UnifiedSidebar.js';
3131
import { AppHeader } from './AppHeader.js';
3232
import { useResponsiveSidebar } from '../hooks/useResponsiveSidebar.js';
33+
import { usePhoneNavDrawerHistory } from './usePhoneNavDrawerHistory.js';
3334
import { usePhoneViewport } from '../hooks/usePhoneViewport.js';
3435
import { primeConsoleSidebarOpen } from './consoleSidebarRail.js';
3536
import { useAiSurfaceEnabled } from '../hooks/useAiSurface.js';
@@ -131,6 +132,7 @@ export function isRecordPagePath(pathname: string): boolean {
131132
/** Inner component that can access SidebarProvider context */
132133
function ConsoleLayoutInner({ children }: { children: React.ReactNode }) {
133134
useResponsiveSidebar();
135+
usePhoneNavDrawerHistory();
134136
useAdvertiseShortcut(TOGGLE_SIDEBAR_SHORTCUT);
135137
return <>{children}</>;
136138
}

0 commit comments

Comments
 (0)