Skip to content

refactor(frontend): move mobile nav and header projection to a module - #1340

Merged
IAmJSD merged 5 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-215
Oct 24, 2026
Merged

IAmJSD merged 5 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-215

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

frontend/src/app.ts was 1052 lines, more than twice the 500-line ceiling in CLAUDE.md, and the mobile drawer plus the topbar-to-drawer projection was its largest self-contained chunk. This PR moves that code verbatim into frontend/src/nav/mobile-nav.ts and gives it a small typed API. There is no user-visible change.

Refs #215
Partial: app.ts is now 850 lines, still over 500. The acceptance criteria allow that if the remaining excess becomes its own follow-up; the next extraction target is named under "Notes for review" but no follow-up issue is filed yet.

What changed

  • dd32b27d: copies MOBILE_NAV_QUERY, DRAWER_PROJECTED_IDS, syncHeaderPlacement and setupMobileNav (with its inner openDrawer/closeDrawer/applyBreakpointState transitions) verbatim into the new frontend/src/nav/mobile-nav.ts. The only edit is exporting MOBILE_NAV_QUERY.
  • 9c33555e: app.ts imports setupMobileNav from the module and drops the inline copy (1052 to 850 lines). __tests__/mobile-nav.test.ts imports from the module, and the comments in index.html (the #sidebar-extras slot) and styles/responsive.css that named app.ts now name nav/mobile-nav.ts.
  • d6e7fded: setupMobileNav() returns a MobileNavHandle (open(), close(), isOpen()) backed by the same transitions the hamburger, overlay and Escape key use, or null when the page has no drawer. app.ts ignores the return value, so startup behavior is unchanged. Two new jest tests cover it.

Why this approach

It follows the extraction target the issue proposes. Returning a small handle meets the "typed API" ask without changing startup behavior; a class or barrel object would have meant rewriting the code instead of moving it. The work is split into copy, switch and handle commits so each stays under 400 lines and the move can be checked line by line.

Screenshots

No visible change: this is a pure refactor.

Verification

Run on this branch's head d6e7fded on top of main a14dc8f2:

  • Lint: 0 errors (142 pre-existing warnings, none in touched files). Typecheck and build pass.
  • Jest: 109 suites, 3488 passed, 1 skipped. mobile-nav.test.ts re-run with --randomize: 24/24, including 2 new tests (setupMobileNav returns null with nothing to wire; the handle's open/close drive the same aria-expanded, inert and focus changes as the hamburger).
  • Playwright via e2e-local.sh: 157/157, including the unchanged mobile-header-drawer.spec.ts.
  • git-secrets scan clean (planted key flagged); scripts/test-git-secrets-allowlist.sh: 137 pass, 0 fail.

Notes for review

  • app.ts is still 850 lines. The obvious next target is the purchase-execution flow (handleExecutePurchase, the fan-out bucket submit and describeDuplicateSubmit, about 450 lines), which could move to something like frontend/src/purchase/execute.ts; setupFeedbackLink (about 65 lines) could also move out.
  • MobileNavHandle is new API surface that nothing uses yet. If setupMobileNav should keep returning void, the third commit can be dropped and the first two still stand.

Overlaps

Those PRs will be opened alongside this one and merged in sequence, with conflicts resolved on the later branch.

Note for the UI owner

This changes UI code that @cristim owns. IAmJSD reviewed the before/after screenshots and asked for it to be merged; any design point you disagree with can be a follow-up.

🤖 Generated with Claude Code

IAmJSD and others added 3 commits October 24, 2026 01:32
…ader projection

app.ts is more than twice the 500-line ceiling in CLAUDE.md, and the mobile
drawer plus the topbar-to-drawer projection is its largest self-contained
chunk. This commit copies that code verbatim into its own bounded-context
module, frontend/src/nav/mobile-nav.ts; the next commit switches app.ts and
the tests over and deletes the original, so the move stays reviewable as
two small diffs.

The only edit is exporting MOBILE_NAV_QUERY so the breakpoint is part of
the module's public surface.

Refs LeanerCloud#215

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… copy

Removes setupMobileNav, syncHeaderPlacement and their constants from app.ts
(1052 -> 849 lines) and imports setupMobileNav from the new module instead.
The jest suite now imports from the module directly, and the comments in
index.html and responsive.css that pointed at app.ts name the new home.

No behavior change: the code is the same, it is only called from a new
file.

Refs LeanerCloud#215

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue LeanerCloud#215 asks for the extracted module to expose a typed API rather than
loose functions. setupMobileNav now returns a MobileNavHandle with open(),
close() and isOpen(), backed by the same openDrawer/closeDrawer transitions
the hamburger, overlay and Escape key use, or null when the page has no
drawer to wire. app.ts ignores the return value, so startup behavior is
unchanged.

Adds jest coverage for the null case and for the handle driving the same
aria-expanded, inert and focus changes as the hamburger.

Refs LeanerCloud#215

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added impact/internal Team-internal only type/chore Maintenance / non-user-visible triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/m Days labels Oct 24, 2026
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 5 billable files and costs up to $1.25.

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 6122fa2 and 9c867cf.

Files selected for processing: 5
  • frontend/src/__tests__/mobile-nav.test.ts
  • frontend/src/app.ts
  • frontend/src/index.html
  • frontend/src/nav/mobile-nav.ts
  • frontend/src/styles/responsive.css
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

IAmJSD and others added 2 commits October 24, 2026 16:32
Conflicts:
- frontend/src/app.ts: import block. Kept both this branch's
  `import { setupMobileNav } from './nav/mobile-nav'` and main's
  `import { renderInitError } from './init-error'` (LeanerCloud#1338). No main commit
  touched the moved mobile-nav code, so nav/mobile-nav.ts needs no re-apply.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts:
- frontend/src/app.ts: import block only. This branch imports
  setupMobileNav from ./nav/mobile-nav; main (LeanerCloud#190, purchasing section
  links) added initPurchasingNav from ./purchasing-nav at the same spot.
  Kept both imports. Main's other app.ts changes (initPurchasingNav()
  call, LeanerCloud#331 close-button cleanup) merged cleanly and are untouched by
  the mobile-nav extraction.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE 9c867cf0a89d637684268ce7a741ed36440aa873

Blocking:

  • none
    Non-blocking:
  • frontend/src/nav/mobile-nav.ts:214: the comment "same guard as tab switching above" now points at code that stayed behind in app.ts.
  • frontend/src/nav/mobile-nav.ts: the new nav/ directory sits beside main's src/purchasing-nav.ts (feat(frontend): Admin Purchasing policies page is very long with a mid-page partial load #190), so two nav modules now live in different places. Consider co-locating them in a follow-up.
    Attacked: Diffed the mobile-nav block in origin/main:app.ts against nav/mobile-nav.ts. The body is byte-identical apart from export on MOBILE_NAV_QUERY, the handle return/null and docs. Grepped src/ and tests-e2e/ for remaining imports of setupMobileNav/syncHeaderPlacement from ../app and found none. Both merges (5c33858, 9c867cf) conflicted only in the import block. Checked with git show --cc: main's renderInitError/initPurchasingNav imports and call, and the fix(frontend): Escape leaves stale purchase buckets for the next modal submission #331 close-button change, are intact. Main's newest commit f2ab673 touches app.ts but merge-tree is clean. The handle tests check aria-expanded, inert and focus.

@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 9c867cf0 merged onto current main (11b0f679; merge-tree 72c81678; Linux, Node, headless Chromium):

  • This branch's own checks passed at this head: lint, typecheck, build, the full jest suite, the full Playwright e2e suite, and the git-secrets checks (see Verification in the PR body).
  • The combined tree of every UI PR in this merge wave (tree 04af8e9b, main plus 5 PRs) passes npm ci, lint, typecheck, build, jest (1 skipped, 4122 passed, 4123 total), and the full Playwright suite (238 passed).
  • CI is green on 9c867cf0 (CI Success, Run pre-commit hooks).
  • The independent review above approves this exact head.
  • Nothing was applied to any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 24, 2026 20:51
@IAmJSD
IAmJSD merged commit 2907777 into LeanerCloud:main Oct 24, 2026
29 checks passed
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Marked ready and merged by IAmJSD's agent, at IAmJSD's request (IAmJSD reviewed the before/after screenshots; cristim is traveling), after the independent review and local verification above, all at 9c867cf0.

IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
Conflicts:
- frontend/src/app.ts: main (LeanerCloud#1340) moved setupMobileNav and
  syncHeaderPlacement into frontend/src/nav/mobile-nav.ts. Kept both
  imports (wireTablistKeyboard from this branch, setupMobileNav from
  main) and took main's removal of the inline drawer code.
- frontend/src/nav/mobile-nav.ts (merged cleanly, edited by hand):
  ported this branch's drawer change into the moved module.
  closeDrawer takes returnFocus 'always' | 'if-inside', and a sidebar
  link click closes with 'if-inside' so focus stays on the new page
  heading. The MobileNavHandle.close() main added wraps
  closeDrawer() so it keeps its always-return-focus behavior.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
Conflicts:
- frontend/src/app.ts: import block. Kept this branch's
  setOpenAccountsHandler import (first-run empty state, LeanerCloud#756) and
  main's setupMobileNav import from nav/mobile-nav (LeanerCloud#1340). The rest of
  app.ts merged cleanly; main's removal of the inline drawer code
  stands and the setOpenAccountsHandler wiring is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
Conflicts:
- frontend/src/app.ts: import block. Kept this branch's fanout-retry
  import (showFanOutRetry, fanOutRetryLabel, FanOutBucketOutcome) and
  main's setupMobileNav import from nav/mobile-nav (LeanerCloud#1340).

handleFanOutExecute merged cleanly: main's combined bulk-approval
totals and upfront-naming confirm label (LeanerCloud#1362) now sit alongside this
branch's retry-label restore in the finally block. A retry passes only
the narrowed failed buckets, so the confirm totals cover just the
requests being resent.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant