Skip to content

refactor(nav): unify section-id vocabulary + make off-route detection explicit #160

Description

@yigitdot

Follow-up from the PR #159 review (fix #157). Two related, non-blocking improvements deferred out of that bugfix's scope.

1. Unify the section-id vocabulary

"intro" and the section-id set are currently declared in three unrelated places with no compile-time link:

  • lib/scroll.ts — HOME_SECTION_ID = "intro"
  • components/site/Chrome.tsx — SECTION_IDS = ["intro", "compare", "method", "faq", "contact"]
  • components/site/MobileMenu.tsx — type SectionId = "intro" | "compare" | ...

Renaming the hero section id today means editing three files in lockstep with no type error to catch a miss.

Proposal: export a single SectionId union + SECTION_IDS tuple from lib/ (e.g. lib/scroll.ts or a sibling), import it in both components, and define HOME_SECTION_ID as a member of it. HomeNavTarget's section.id could then narrow to Exclude<SectionId, typeof HOME_SECTION_ID>. Touches the active-section observer typing in Chrome.tsx, so it's a deliberate refactor rather than a one-liner.

2. Make the off-route discriminator explicit

Both nav codepaths decide "am I off-route?" by element absence:

  • components/site/Chrome.tsx:70 — if (document.getElementById(HOME_SECTION_ID) === null) return;
  • components/site/MobileMenu.tsx cleanup — if (anchor && !targetEl) window.location.assign(...)

Correct today because every nav target is a synchronously-rendered home section and the Hero always mounts on /. But the check can't distinguish "wrong route" from "right route, target not mounted": a future lazy/Suspense-mounted target tapped on the home page would be misread as off-route and silently hard-reload (and the desktop return path could become a no-op dead-click if the Hero ever failed to mount).

Proposal: key the off-route branch on actual route mismatch (e.g. via usePathname(), already used in Chrome.tsx) rather than element absence, and/or extract a small testable isOnHomeRoute() / resolveLocalTarget() helper so the off-route-vs-missing distinction is explicit and unit-testable.

Both surfaced by the multi-agent review on #159; neither is a live bug.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions