feat(header): ✨ add navigation menu with dropdown support - #814
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0473ff85ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| discover: t(HEADER_NAV_MESSAGE_KEYS.discover), | ||
| search: t(HEADER_NAV_MESSAGE_KEYS.search), | ||
| }), | ||
| ...EXAMPLE_NAV_ITEMS, |
There was a problem hiding this comment.
Every consumer of MainHeader, including the fresh create-vitnode-app scaffold, now receives these demo Community/Resources entries unconditionally. That scaffold has no plugins and its registered core routes include /discover and /search but not /docs, so selecting Resources → Documentation reaches the not-found page; the hard-coded English labels also bypass the active locale. Remove the examples from the library default or accept real application-provided navigation instead.
Useful? React with 👍 / 👎.
| {...props} | ||
| /> | ||
| > | ||
| <span |
There was a problem hiding this comment.
NavigationMenuPrimitive.List renders an HTMLUListElement, but this adds a <span> as its direct child alongside the menu's <li> items. A <ul> may only contain list items and script-supporting elements, so this produces invalid list markup and can interfere with assistive technology's list representation; render the decoration outside the list or through CSS instead.
AGENTS.md reference: AGENTS.md:L56-L56
Useful? React with 👍 / 👎.
| onFocus={event => { | ||
| const item = itemUnder(event.target); | ||
| if (item) moveHighlightTo(item); | ||
| onFocus?.(event); | ||
| }} |
There was a problem hiding this comment.
When a keyboard user focuses an inactive menu item and then tabs completely out of the navigation, this handler makes the highlight visible but no blur handler calls settleHighlight or hides it. With no open trigger or active link mutation to wake the observers, the last focused item remains visibly highlighted after focus has moved elsewhere; handle focus leaving the list analogously to onPointerLeave.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2c0fa327
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| return ( | ||
| <NavigationMenuItem> | ||
| <NavigationMenuTrigger className="text-muted-foreground hover:text-foreground data-popup-open:text-foreground"> |
There was a problem hiding this comment.
When the current route matches a dropdown child, only the link inside the closed popup receives active; the visible trigger has styling only for hover and data-popup-open. As a result, users on /docs or another child route get no persistent indication of their current top-level section until they open the menu. Derive the trigger's active state from its children and expose a corresponding active style.
Useful? React with 👍 / 👎.
| > | ||
| <span className="font-medium">{child.label}</span> | ||
| {child.description ? ( | ||
| <span className="text-muted-foreground text-xs leading-relaxed text-pretty"> |
There was a problem hiding this comment.
Every dropdown description is rendered with Tailwind's text-xs (12px), below the repository's explicit 14px minimum for body text. This makes the supporting navigation copy unnecessarily difficult to read; use text-sm or another size of at least 14px.
AGENTS.md reference: AGENTS.md:L71-L71
Useful? React with 👍 / 👎.
| /** One entry in the main nav - a link, or a dropdown when it carries `items`. */ | ||
| export interface HeaderNavItem { | ||
| href: string; | ||
| items?: HeaderNavChildItem[]; |
There was a problem hiding this comment.
This adds the items contract and dropdown behavior without documenting how navigation entries, child descriptions, and active routes are intended to be configured; a repo-wide documentation search found no HeaderNavItem or HeaderNavMenu guidance. Add concise documentation for the feature as required by the repository contribution rules.
AGENTS.md reference: AGENTS.md:L76-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b32e74e8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const installed = getDefaultBlockRegistry(); | ||
|
|
||
| if (installed) return installed; | ||
|
|
||
| coreOnly ??= createBlockRegistry([coreBlocks]); |
There was a problem hiding this comment.
On a fresh web process that opens /settings, nothing evaluates the generated src/blocks.gen.ts, so getDefaultBlockRegistry() is undefined and this silently falls back to the three core blocks. As a result, installed plugin blocks are missing from the editor catalog, and any plugin block already stored in these wildcard zones is treated as unregistered and omitted from the page, even though the API builds its registry from all plugins and accepts that block. The generated application registry needs to be loaded or injected before PageWidgetsZone resolves it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Corrigé en dffc170 : les deux preuves entrent dans cette PR plutôt que d'attendre la #92. docs/ia/livraisons/PILOTE-NATIF-HD-20260919.md (5 287 octets) et docs/ia/livraisons/preuves/PILOTE-NATIF-HD-20260919.json (38 666 octets) sont légers et se suffisent à eux-mêmes ; ils sont repris tels quels de origin/codex/qualite-suivi-20260919. Les chemins cités par la campagne sont donc résolubles dès la fusion.
| export const moderatorPermissionsQueryKey = () => | ||
| ["vitnode", "users", "permissions", "moderator"] as const; |
There was a problem hiding this comment.
This new cache entry contains identity-specific permissions, but every sign-in, sign-out, sign-up, and SSO transition calls removeUserIdentityQueries, which currently removes only file and device queries. Switching accounts can therefore retain the previous user's permission result; loadPageWidgets may reuse it with staleTime: "static", causing an unauthorized user to temporarily receive the edit UI or a newly authorized user to receive no editor until a later refetch. Include this query under the identity cleanup performed by those auth flows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40df5a8745
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const [leaf, leafValue] of Object.entries(inner)) { | ||
| const nestedIssue = leafIssue(`${name}.${leaf}`, leafValue, nested[leaf]); | ||
| if (nestedIssue !== null) return nestedIssue; |
There was a problem hiding this comment.
When a block contains a group inside another group, this loop calls leafIssue on the inner group but never traverses its fields. If a later plugin version removes or renames a required field at that depth, previously stored data passes the production shape check and reaches the component with the missing property, defeating the drift guard and potentially crashing the public page. assertBlockFields accepts group as a group leaf, so either recurse here or reject nested groups when defining blocks.
Useful? React with 👍 / 👎.
| export const settingsPage = defineEditablePage({ | ||
| id: 'example:settings', | ||
|
|
||
| permission: { module: 'widgets', permission: 'can_edit' }, |
There was a problem hiding this comment.
The guide later reuses this exact settingsPage with <PageWidgets> and states that the definition names its permission plugin, but this declaration omits it. Copying the documented three-file example therefore leaves permission.plugin undefined, and PageWidgets hard-codes that case to canEdit = false, so even authorized moderators never see Edit widgets. Add the registering plugin id here or change the wrapper contract so the documented server-side default is usable in the browser.
AGENTS.md reference: AGENTS.md:L74-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 155f249333
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .remember(NAVIGATION_CACHE_KEY, NAVIGATION_CACHE_TTL_SECONDS, async () => | ||
| toPublicNavigation( | ||
| await readNavigationRecords(c), | ||
| c.get("core").navigation, | ||
| ), |
There was a problem hiding this comment.
When Redis is enabled, this caches the fully resolved preset href and icon for 24 hours under a constant key. Because startup/deployment does not expire that key, updating or uninstalling a plugin can leave the header serving its old URL—or even an item that toPublicNavigation would now omit—until the TTL elapses or an admin happens to mutate the menu. Include installed preset metadata/versioning in the key or invalidate this cache when the API boots.
Useful? React with 👍 / 👎.
| await saveNavigationWords(c, item.id, { | ||
| description: body.description, | ||
| title: body.title, | ||
| }); |
There was a problem hiding this comment.
If saving the description fails after the item and title have been written—for example, the request supplies a description languageCode that passes this route's schema but violates core_languages_words' foreign key—the endpoint returns an error while leaving the navigation row and title committed. For preset items, retrying then returns 409 because the supposedly failed create already consumed the preset. Wrap the row insertion and both language-word writes in one transaction so a failed request cannot leave a partial menu item.
Useful? React with 👍 / 👎.
…nd-drop functionality
155f249 to
5a0ed50
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a0ed504c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stored: data?.description ?? [], | ||
| }), | ||
| ), | ||
| isOpenInNewTab: z.boolean().default(data?.isOpenInNewTab ?? false), |
There was a problem hiding this comment.
When creating a preset whose declaration has isOpenInNewTab: true, the form initializes this field to false and selecting the preset never updates it. useAdminNavigationMutations then always sends that boolean, so the API's body.isOpenInNewTab ?? preset.isOpenInNewTab fallback is unreachable and the newly created item incorrectly opens in the same tab unless the administrator manually toggles the switch.
Useful? React with 👍 / 👎.
| await queryClient.query({ | ||
| ...intlQueryOptions({ | ||
| locale, | ||
| namespaces: [...ADMIN_NAVIGATION_NAMESPACES, ...namespaces], | ||
| }), |
There was a problem hiding this comment.
With 14 or more plugins declaring navigation presets, this combines at least 14 plugin namespaces with the three fixed admin namespaces and passes more than 16 entries to one intlQueryOptions request. validateIntlInput explicitly rejects requests above MAX_NAMESPACES (16), so opening the Navigation admin screen fails before it renders; the public header has the same failure once its active menu spans more than 16 plugin namespaces. Split these namespace loads into bounded requests, as the staff-permission loader already does.
Useful? React with 👍 / 👎.
2 participants
Improving Documentation
pnpm lint:fixto fix formatting issues before opening the PR.Description
What?
Why?