diff --git a/CHANGELOG.md b/CHANGELOG.md index 86a37bc..c711ac0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ This repo releases via the `release-same-version` label (see `.github/workflows/ Per-package detail lives in the GitHub release tagged `@`. +## Unreleased + +### Behavior changes + +- **`@modular-react/core`** — `buildNavigationManifest` (and therefore `useNavigation`) now breaks ties on `order` by preserving insertion order instead of label string comparison. Items declared first render first when `order` is unset or equal: modules in registration order, items in the order declared in each module's `navigation` array, plugin-contributed items last. Labels are no longer a tiebreaker — the previous fallback sorted by i18n-key name (e.g. `appShell.nav.assets` < `appShell.nav.projects`), which produced surprising orderings unrelated to translated text. Apps that relied (intentionally or not) on alphabetical-by-key fallback should set explicit `order` values to lock in the desired sequence. + ## 2026-04-19 — `@modular-react/*@1.2.0`, `@*-modules/*@2.3.0` Released alongside PR adjustments to PR #14 (Lokalise PoC gaps follow-up). diff --git a/docs/navigation.md b/docs/navigation.md index 1920021..48a4564 100644 --- a/docs/navigation.md +++ b/docs/navigation.md @@ -22,6 +22,17 @@ interface NavigationItem< `TAction` defaults to `never`, so the `action` field is absent from the item surface until an app opts in. `action` is how a nav item carries a dispatchable intent (start a journey, open a module as a tab, trigger a shell command) without overloading `meta`. The framework treats it as opaque — the shell's navbar renderer switches on `action.kind` and dispatches. See the `TAction` section below. +## Ordering + +`buildNavigationManifest` (and therefore `useNavigation`) sorts items with these rules, in order: + +- `order` is ascending — lower numbers render first. +- Items without an explicit `order` render after every item that has one — there's no sentinel cap, so `order: 9999` still sorts before an unordered item. +- Ties — including the common case where no item sets `order` — preserve insertion order: modules in the order they were registered with the registry, items in the order declared in each module's `navigation` array, plugin-contributed items (via `RegistryPlugin.contributeNavigation`) last. +- Label is **not** a tiebreaker. Labels are typically i18n keys (especially when `TLabel` is narrowed to `ParseKeys`), so sorting on label would sort by key naming rather than anything user-meaningful. + +Practical guidance: set explicit `order` when you care about the position. Rely on registration order when modules collaborate to express intent ("project's nav comes before assets'"). Don't rely on label-alphabetical ordering — it isn't a thing. + ## The typical pattern: one alias, used everywhere Declare the alias in your `app-shared` package so every module and the shell agree: diff --git a/packages/core/src/navigation.test.ts b/packages/core/src/navigation.test.ts index 3352a33..08c6be5 100644 --- a/packages/core/src/navigation.test.ts +++ b/packages/core/src/navigation.test.ts @@ -62,24 +62,58 @@ describe("buildNavigationManifest", () => { }); describe("sorting", () => { - it("sorts by order first, then label alphabetically", () => { + it("sorts by order ascending, then preserves insertion order on ties", () => { const m = mod([ { label: "Zebra", to: "/z", order: 1 }, { label: "Apple", to: "/a", order: 2 }, { label: "Banana", to: "/b", order: 1 }, ]); const result = buildNavigationManifest([m]); - expect(result.items.map((i) => i.label)).toEqual(["Banana", "Zebra", "Apple"]); + // order 1: Zebra then Banana (insertion order — Zebra was declared first). + // order 2: Apple. + expect(result.items.map((i) => i.label)).toEqual(["Zebra", "Banana", "Apple"]); }); - it("items without order sort after items with order, alphabetically among themselves", () => { + it("items without order sort after items with order, preserving insertion order among themselves", () => { const m = mod([ { label: "Cherry", to: "/c" }, { label: "Apple", to: "/a" }, { label: "Banana", to: "/b", order: 5 }, ]); const result = buildNavigationManifest([m]); - expect(result.items.map((i) => i.label)).toEqual(["Banana", "Apple", "Cherry"]); + expect(result.items.map((i) => i.label)).toEqual(["Banana", "Cherry", "Apple"]); + }); + + it("ties on order fall back to module registration order across modules", () => { + // Regression: with i18n-key labels, the previous label tiebreaker would + // put "appShell.nav.assets" before "appShell.nav.projects" purely + // because of key naming. Insertion order keeps the registration intent. + const m1 = mod([{ label: "appShell.nav.projects", to: "/p" }]); + const m2 = mod([{ label: "appShell.nav.assets", to: "/a" }]); + const result = buildNavigationManifest([m1, m2]); + expect(result.items.map((i) => i.label)).toEqual([ + "appShell.nav.projects", + "appShell.nav.assets", + ]); + }); + + it("explicit order values larger than any sentinel still sort before unordered items", () => { + // Pins the contract: missing order always sorts last, regardless of + // how large an explicit order value is. Guards against reintroducing a + // numeric sentinel (e.g. `?? 999`) that would silently flip the order + // of any item using order >= the sentinel. + const m = mod([ + { label: "Unordered", to: "/u" }, + { label: "Huge", to: "/h", order: 100_000 }, + ]); + const result = buildNavigationManifest([m]); + expect(result.items.map((i) => i.label)).toEqual(["Huge", "Unordered"]); + }); + + it("plugin extraItems land after module items when ties on order", () => { + const m = mod([{ label: "ModuleA", to: "/a" }]); + const result = buildNavigationManifest([m], [{ label: "PluginA", to: "/pa" }]); + expect(result.items.map((i) => i.label)).toEqual(["ModuleA", "PluginA"]); }); }); diff --git a/packages/core/src/navigation.ts b/packages/core/src/navigation.ts index 0b09e30..bc905ac 100644 --- a/packages/core/src/navigation.ts +++ b/packages/core/src/navigation.ts @@ -4,9 +4,11 @@ import type { NavigationManifest, NavigationGroup } from "./runtime-types.js"; /** * Collect navigation items from every module into a sorted + grouped manifest. * - * Items are sorted by `order` ascending (missing order sorts last), then by - * label alphabetically. Items with a `group` key land in the matching entry - * of `groups`; items without a group land in `ungrouped`. + * Items are sorted by `order` ascending (missing order sorts last); ties + * preserve insertion order (modules in registration order, items in declared + * order within each module, plugin items last). Items with a `group` key + * land in the matching entry of `groups`; items without a group land in + * `ungrouped`. * * Generic over `TNavItem` so host-specific NavigationItem subtypes (typed * labels, typed dynamic-href context, typed meta) are preserved end-to-end — @@ -33,18 +35,22 @@ export function buildNavigationManifest` comparison is deterministic and - // good enough for nav item ordering (labels are typically i18n keys or - // ASCII-dominant anyway). + // Sort by `order` ascending; missing `order` sorts after every explicit + // value (no upper-bound sentinel — an item with `order: 9999` still sorts + // before unordered items). Ties preserve insertion order + // (Array.prototype.sort is stable in ES2019+): modules in registration + // order, items in declared order within each module's `navigation` array, + // plugin `extraItems` last. Deterministic, SSR-safe, and matches developer + // intent — the things you declared first render first. Label is + // intentionally not a tiebreaker: labels are typed as i18n keys in host + // apps (`TLabel` defaults to `string` but narrows to `ParseKeys`), so + // lexicographic comparison would sort by translation-system artifact + // rather than anything meaningful. const sorted = [...allItems].sort((a, b) => { - const orderDiff = (a.order ?? 999) - (b.order ?? 999); - if (orderDiff !== 0) return orderDiff; - if (a.label === b.label) return 0; - return (a.label as string) < (b.label as string) ? -1 : 1; + if (a.order === b.order) return 0; // both unset, or both same number — preserve insertion order + if (a.order === undefined) return 1; + if (b.order === undefined) return -1; + return a.order - b.order; }); // Group items