fix(mobile): close a menu sheet on Back instead of switching panes by devswha · Pull Request #837 · devswha/ovis

devswha · 2026-10-11T20:53:55Z

Problem

On a phone, with a ⋯ menu open as a bottom sheet (header More, tab menu, row menus), the system Back button did not close the sheet: it went back to the previously opened pane, because #762 made every pane switch a history entry and the sheet had none.

Change

New src/lib/sheetHistory.ts with useSheetHistory(active, onBack):
- while a sheet is open it holds a history entry carrying the same address, so Back closes the sheet and the pane stays;
- a sheet closed any other way (Cancel, swipe, scrim, an item) steps off its entry after the closing click's commit and effects; if an item pushed its own entry first (opened another pane), the sheet's entry is left under it and skipped when Back lands on it, since the entry below names the same place;
- other code that spreads history.state into a new entry (the file viewer) does not copy the sheet's mark;
- a reload turns a leftover sheet entry into an ordinary one;
- StrictMode's remount reuses the same entry.
RowMenu uses it in sheet mode only. Desktop popovers and other modals (palette, dialogs) are unchanged.

Tests

deep-link-demo-regression gains a phone case: Back with the More sheet open closes it and keeps the pane; Cancel then Back goes to the previous pane in one press. It fails without the fix ("an open sheet is an entry").
bun run check fast, tab-menu-regression, mobile-tabs-regression, ui-regression and file-viewer-mobile-regression pass.

🤖 Generated with Claude Code

Since deep links made each pane a history entry, the system Back on a phone with a ⋯ sheet open went to the previous pane and left the sheet up. A sheet now holds a history entry of its own: Back closes it, and a sheet closed any other way steps off its entry once the closing click's effects have run, so no Back press is wasted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-11T20:54:25Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 13 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: devswha/herdr-web-ui/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: c8e6e5de-b1c7-4793-99e2-1bdd4a42df3d

📥 Commits

Reviewing files that changed from the base of the PR and between 1e440bc and 77f914b.

📒 Files selected for processing (3)

scripts/deep-link-demo-regression.ts
src/lib/sheetHistory.test.ts
src/lib/sheetHistory.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: devswha/herdr-web-ui/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 0c133189-92c5-4b0e-94c3-6c056115edce

📥 Commits

Reviewing files that changed from the base of the PR and between d92df49 and 1e440bc.

📒 Files selected for processing (4)

CHANGELOG.md
scripts/deep-link-demo-regression.ts
src/components/RowMenu.tsx
src/lib/sheetHistory.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

📝 Summary

Summary by CodeRabbit

Bug Fixes
- On phones, pressing the system Back button while the More menu is open now closes the menu instead of navigating to the previously selected pane.
- Closing the menu with Cancel, a swipe, or a menu item no longer leaves an extra Back step in browser history. The next Back action returns to the previously selected pane as expected.

📝 Summary

Walkthrough

The More sheet now uses browser history so system Back closes the sheet before navigating to the previously selected pane. A mobile-browser regression test checks Back and Cancel behavior.

Changes

More Sheet History

Layer / File(s)	Summary
Sheet history entries `src/lib/sheetHistory.ts`	The module adds sheet markers and tracks open and pending-leave sheets. It creates a marked history entry or reuses a pending entry when that entry is still current.
Back handling and sheet lifecycle `src/lib/sheetHistory.ts`	The module handles sheet cleanup, Back events, stale marked entries, and reloads. The new `useSheetHistory` hook links an active sheet to history.
RowMenu integration and regression coverage `src/components/RowMenu.tsx`, `scripts/deep-link-demo-regression.ts`, `CHANGELOG.md`	`RowMenu` uses the hook to close the sheet on system Back. The mobile-browser test checks that Back preserves the selected pane and that Cancel leaves no extra Back step. The changelog records the behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: jiunshinn, phirschybar

Merge Risk: ⚪ Minimal · up to 1e440

The More sheet’s Back behavior is mergeable after normal checks; no actionable issue remains from this review.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check		The title clearly summarizes the main change: system Back closes a mobile menu sheet instead of switching panes.
Description check		The description explains the problem, implementation, affected behavior, and validation results. It uses a "Tests" heading instead of the template's "Validation" heading, but it provides the required …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Review findings on the sheet entry (Codex, pre-merge): - More → New tab closed the dialog by itself: the app replaced the sheet entry's address with the dialog's, and the sheet's cleanup then stepped back off it. An entry whose address changed while the sheet left is kept. - A navigation an item pushed (another pane, Settings, a file) left the sheet's entry between, so Forward landed on a duplicate. The push now replaces the sheet's entry; a sheet opened from a sheet reuses it. - Forward onto the entry of a sheet Back closed went back again; it goes on. Unit tests drive the module through a fake history; the phone deep-link check covers New tab and Move pane to… from the More sheet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

CHANGELOG: one Fixed heading under Unreleased with #837, #835 and this entry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…842) ## Problem #837 made a phone's ⋯ menu sheets (RowMenu) close on Back. The other dialogs a phone draws as bottom sheets still had no history entry, so Back opened the previously selected pane under them and left the dialog up. Seen on the plan sheet (header `1/3` button) during mobile testing. ## Change - `usePhoneSheetBack(onBack, open = true)` in `src/lib/sheetHistory.ts`: `useSheetHistory` gated on `SHEET_QUERY` (≤640px), so wider screens keep today's behavior (the desktop test that Back under the palette selects a pane is unchanged). - Applied to `PlanDialog`, `CommandPalette` (with its `open`), `ConfirmDialog`, `FilesDialog`, `WorktreeDialog`, `PaneRename` and `MermaidLightbox`. A dialog busy with its action (`pending`/`running`/`busy`) ignores Back, as its swipe-to-close already does. - Settings, New workspace and the file viewer are left alone: they already own history entries. - The merged `sheetHistory` handles the rest: a dialog opened from a ⋯ sheet (More → Command palette) reuses the sheet's entry, and a navigation made from a dialog takes it over. ## Tests - `deep-link-demo-regression`'s phone case now also opens the plan (Back closes it, pane kept) and the palette from More (takes the sheet's entry, Back closes it), then walks Back to the earlier panes. It fails without the component changes ("the plan is an entry"). - `bun run check fast`, `tab-menu-regression`, `mobile-tabs-regression`, `ui-regression`, `file-viewer-mobile-regression`, `file-viewer-regression` and `settings-pages-demo-regression` pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

## Change Release 0.4.8, from main at ca6db6e (20 PRs since v0.4.7: the 0.4.7 mobile walk-through fixes, two contributor PRs and the revived #793). - `package.json` and `herdr-plugin.toml`: 0.4.8. - CHANGELOG: `[Unreleased]` becomes `[0.4.8] - 2026-10-12`, with the compare links. Twelve entries that had no PR link get theirs (#835 #837 #838 #840 #842 #843 #844+#848 #845 #847 #849 #851 #852). A Maintenance line says the remote-PC runtime is now version 27. - `release-summaries.json`: 0.4.8 in en/ko/ja/zh/tr (3 new, 6 improved, 8 fixed). The Turkish lines are not checked by a native speaker. - `REMOTE_BUNDLE_VERSION` 26 -> 27: #836 adds the `pending-hold` server feature and #787 serves precompressed assets, so `remote-v27` is published before the app release. Every PR merged since v0.4.7 is linked in the section: #787 #788 #793 #835 #836 #837 #838 #839 #840 #841 #842 #843 #844 #845 #846 #847 #848 #849 #851 #852. ## Validation - `bun scripts/release-notes.ts 0.4.8` renders the summaries and the full changelog (116 lines). - `bun run check fast`: generated types, typecheck, build and 2800+ unit tests pass. ## Release After this merges and its CI on `main` passes: push `remote-v27`, wait for its release, then run the Release workflow with `0.4.8`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

devswha merged commit 17f053a into main Oct 11, 2026
9 checks passed

devswha deleted the fix/phone-back-closes-sheet branch October 11, 2026 21:35

devswha added a commit that referenced this pull request Oct 11, 2026

Merge origin/main into fix/phone-hidden-sibling-keeps-grid

2d8f323

CHANGELOG: one Fixed heading under Unreleased with #837, #835 and this entry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

devswha mentioned this pull request Oct 11, 2026

fix(mobile): close every phone dialog on Back, not only the ⋯ menus #842

Merged

devswha mentioned this pull request Oct 11, 2026

chore(release): prepare 0.4.8 and remote runtime v27 #853

Merged

Navigation Menu

Repository navigation

fix(mobile): close a menu sheet on Back instead of switching panes - #837

fix(mobile): close a menu sheet on Back instead of switching panes#837
devswha merged 2 commits into
mainfrom
fix/phone-back-closes-sheet

devswha commented Oct 11, 2026

Uh oh!

coderabbitai Bot commented Oct 11, 2026 •

edited

Loading

Review limit reached

Summary by CodeRabbit

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

devswha commented Oct 11, 2026

Problem

Change

Tests

Uh oh!

coderabbitai Bot commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Summary by CodeRabbit

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Oct 11, 2026 •

edited

Loading