fix: add mobile sidebar trigger to settings (MUL-2361)#2761
Conversation
|
@biubiu0002 is attempting to deploy a commit to the IndexLabs Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
Not blocking:
PTAL — happy to re-review once 2–5 are pushed. |
|
Thank you for your review. I will review and fix these nits later |
Co-authored-by: multica-agent <github@multica.ai>
Co-authored-by: multica-agent <github@multica.ai>
5aa290c to
e776d62
Compare
What does this PR do?
Fixes a mobile Settings page navigation dead-end by reusing the shared
PageHeader, which includes the standard mobileSidebarTrigger.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
PageHeaderfor 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:
multica-ai/multica:main; the diff is limited to the Settings page fix and the related Playwright coverage.Type of Change
Changes Made
packages/views/settings/components/settings-page.tsx: adds mobile-only sharedPageHeaderso 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 anddismissStarterContent()so the regression can reuse the cached E2E workspace and avoid being blocked by the first-workspace starter modal.Reviewer Feedback Addressed
loginAsDefault()helper for every e2e spec.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.[data-slot="sidebar-trigger"] .first()path with a role-basedToggle Sidebarlocator.PageHeader.How to Test
pnpm typecheckpnpm --filter @multica/web buildPLAYWRIGHT_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
apps/web/features/landing/i18n/), starter-content (packages/views/onboarding/utils/starter-content-content-*.ts), and relevant docs (apps/docs/content/docs/)apps/docs/content/docs/developers/conventions.zh.mdx(terminology, mixed-rule fortask/issue/skill)Checklist notes:
Issues. Before this change, Settings skipped the shared header and that trigger was absent.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.
After: mobile Settings renders the shared PageHeader with the standard sidebar trigger in the top-left.