Skip to content

fix: add mobile sidebar trigger to settings (MUL-2361)#2761

Open
biubiu0002 wants to merge 2 commits into
multica-ai:mainfrom
biubiu0002:fix-MAC-65-mobile-settings-sidebar-trigger
Open

fix: add mobile sidebar trigger to settings (MUL-2361)#2761
biubiu0002 wants to merge 2 commits into
multica-ai:mainfrom
biubiu0002:fix-MAC-65-mobile-settings-sidebar-trigger

Conversation

@biubiu0002
Copy link
Copy Markdown

@biubiu0002 biubiu0002 commented May 18, 2026

What does this PR do?

Fixes a mobile Settings page navigation dead-end by reusing the shared PageHeader, which includes the standard mobile SidebarTrigger.

On mobile, Settings now shows the same sidebar toggle affordance as other dashboard pages. On desktop, the existing settings side-nav layout is preserved.

Thinking path: other dashboard pages already use PageHeader for the mobile sidebar trigger, while Settings rendered its tabs directly and skipped that shared header. Reusing the existing shared header keeps the interaction consistent and avoids introducing a parallel sidebar button implementation.

Context:

  • Mobile Settings could be reached from the sidebar, but it did not expose a way to reopen that sidebar on small screens.
  • This PR is a clean community branch against multica-ai/multica:main; the diff is limited to the Settings page fix and the related Playwright coverage.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Refactor / code improvement (no behavior change)
  • Documentation update
  • Tests (adding or improving test coverage)
  • CI / infrastructure

Changes Made

  • packages/views/settings/components/settings-page.tsx: adds mobile-only shared PageHeader so Settings renders the standard sidebar trigger; keeps the existing Settings title inside the desktop side nav only.
  • e2e/settings.spec.ts: adds a mobile regression test that opens Settings, uses the sidebar trigger, opens the mobile sidebar, and navigates back to Issues.
  • e2e/fixtures.ts: adds workspace id/slug accessors and dismissStarterContent() so the regression can reuse the cached E2E workspace and avoid being blocked by the first-workspace starter modal.

Reviewer Feedback Addressed

  • Kept token-before-navigation scoped to this Settings regression instead of changing the shared loginAsDefault() helper for every e2e spec.
  • Removed the duplicate workspace setup and second login round-trip from the new Settings test.
  • Documented that dismissStarterContent() is one-way for the shared E2E user and is only used here to prevent the starter modal from covering the mobile sidebar trigger.
  • Replaced the fragile [data-slot="sidebar-trigger"] .first() path with a role-based Toggle Sidebar locator.
  • Added an inline note explaining why Settings uses a mobile-only PageHeader.

How to Test

  • pnpm typecheck
  • pnpm --filter @multica/web build
  • Focused mobile Playwright regression passed during validation:
    PLAYWRIGHT_BASE_URL=http://localhost:3102 DATABASE_URL=... NEXT_PUBLIC_API_URL=http://localhost:8080 pnpm exec playwright test e2e/settings.spec.ts --grep 'mobile settings page can reopen sidebar and navigate away'

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots
  • I have updated relevant documentation to reflect my changes
  • If I added a new runtime / coding tool / UI tab, I synced the change to landing copy (apps/web/features/landing/i18n/), starter-content (packages/views/onboarding/utils/starter-content-content-*.ts), and relevant docs (apps/docs/content/docs/)
  • If this PR touches Chinese product copy, I checked it against apps/docs/content/docs/developers/conventions.zh.mdx (terminology, mixed-rule for task / issue / skill)
  • I have considered and documented any risks above
  • I will address all reviewer comments before requesting merge

Checklist notes:

  • UI evidence: the focused mobile Playwright regression verifies the before/after behavior: Settings exposes the standard mobile sidebar trigger, opens the mobile sidebar sheet, and can navigate to Issues. Before this change, Settings skipped the shared header and that trigger was absent.
  • Documentation: no docs content changed because this is a bugfix to an existing navigation affordance, not a user-facing feature or workflow change.
  • Landing/starter-content/docs sync: N/A because this PR does not add a runtime, coding tool, or UI tab.
  • Chinese copy glossary: N/A because this PR does not add or edit Chinese product copy.

AI Disclosure

AI tool used: Multica coding-agent / Codex

Prompt / approach: Investigated the mobile Settings navigation issue, compared Settings with other dashboard pages, reused the shared PageHeader, preserved the focused Playwright regression, and validated the clean community branch with typecheck and web build.

Screenshots

Before: mobile Settings rendered directly into the settings tabs and had no global sidebar trigger.

Before - mobile Settings without sidebar trigger

After: mobile Settings renders the shared PageHeader with the standard sidebar trigger in the top-left.

After - mobile Settings with sidebar trigger

@vercel
Copy link
Copy Markdown

vercel Bot commented May 18, 2026

@biubiu0002 is attempting to deploy a commit to the IndexLabs Team on Vercel.

A member of the Team first needs to authorize it.

@Bohan-J Bohan-J changed the title fix: add mobile sidebar trigger to settings fix: add mobile sidebar trigger to settings (MUL-2361) May 18, 2026
@Bohan-J
Copy link
Copy Markdown
Collaborator

Bohan-J commented May 18, 2026

Thanks @biubiu0002 — review summary below. The main fix looks good and I'm in favor of merging once nits 2–5 are addressed (nit 1 is fine as follow-up). Detail:

Required before merge:

  1. e2e/helpers.ts scope creep. Switching from goto("/login") + page.evaluate to addInitScript changes login bootstrap for every e2e spec. The direction is right (token injected before first navigation), but it's a separate concern from the mobile sidebar fix. Either split it out into its own commit/PR, or at minimum call it out explicitly in the PR description so the side effect is visible to reviewers. Please also double-check that specs still doing manual /login (e.g. auth.spec.ts:25) aren't disturbed.

  2. Duplicate setup in e2e/settings.spec.ts:9-12.

    const api = await createTestApi();                              // already login + ensureWorkspace
    const workspace = await api.ensureWorkspace("E2E Workspace", "e2e-workspace"); // duplicate
    await api.dismissStarterContent(workspace.id);
    const workspaceSlug = await loginAsDefault(page);              // login + ensureWorkspace again

    createTestApi() already calls ensureWorkspace and caches api.workspaceId / api.workspaceSlug. Reuse those and drop the redundant ensureWorkspace / second login round-trip.

  3. dismissStarterContent has a permanent side effect. Per server/internal/handler/onboarding.go:643, the state machine is one-way (NULL → dismissed); a second call returns 409. Once this spec runs on the shared E2E user, starter content is dismissed forever, which will silently break any future spec that depends on the starter modal being visible. Today nothing depends on it, but please add a comment in the spec calling this out, or use a dedicated workspace/user so the side effect is scoped.

  4. Fragile .first() selector (e2e/settings.spec.ts:16). page.locator('[data-slot="sidebar-trigger"]').first() assumes there is exactly one visible sidebar trigger on mobile. If anyone later adds another PageHeader inside Settings, this picks the wrong element silently. Prefer page.getByRole("button", { name: /toggle sidebar/i }), or scope the locator to the new mobile header container.

Not blocking:

  1. Settings is currently the only page that gives PageHeader a md:hidden modifier — every other dashboard page renders it unconditionally. The reason is sound (desktop already has a side-nav title), but it's an outlier worth a one-line comment near settings-page.tsx:101 so future contributors don't copy the pattern blindly.

PTAL — happy to re-review once 2–5 are pushed.

@biubiu0002
Copy link
Copy Markdown
Author

Thank you for your review. I will review and fix these nits later

Multica Coding Agent and others added 2 commits May 21, 2026 21:47
Co-authored-by: multica-agent <github@multica.ai>
Co-authored-by: multica-agent <github@multica.ai>
@biubiu0002 biubiu0002 force-pushed the fix-MAC-65-mobile-settings-sidebar-trigger branch from 5aa290c to e776d62 Compare May 21, 2026 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants