-
Notifications
You must be signed in to change notification settings - Fork 7
Paint the navbar complete at first frame: eager logo, server-rendered search shell #1665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
0f02159
Paint the navbar complete: eager logo, server-rendered search shell (…
feruzm add3803
Keep the search shell visible through the dynamic chunk handoff
feruzm ee10b7f
Add explicit return type to NavbarSearchShell
feruzm 61f639c
Import JSX type from react for the explicit return type
feruzm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
20 changes: 20 additions & 0 deletions
20
apps/web/src/features/shared/navbar/navbar-search-dynamic.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| "use client"; | ||
|
|
||
| import dynamic from "next/dynamic"; | ||
| import { NavbarSearchShell } from "./navbar-search-shell"; | ||
|
|
||
| /* | ||
| The desktop search (input + suggester + transfer/bookmarks/drafts/gallery | ||
| modules) is heavy. The desktop navbar is `hidden md:flex` but still mounts on | ||
| mobile, so a static import would ship all of that into the mobile critical | ||
| path purely as waste. Load it as a separate chunk; the caller gates mounting | ||
| on a confirmed desktop viewport so the chunk never loads on phones. | ||
|
|
||
| The loading fallback is the same pixel-identical shell the caller renders | ||
| before the gate opens: without it the slot would flash empty between the | ||
| moment isDesktop flips true and the moment the chunk arrives (#1665 review). | ||
| */ | ||
| export const Search = dynamic( | ||
| () => import("@/features/shared/navbar/search").then((m) => ({ default: m.Search })), | ||
| { ssr: false, loading: () => <NavbarSearchShell /> } | ||
| ); |
27 changes: 27 additions & 0 deletions
27
apps/web/src/features/shared/navbar/navbar-search-shell.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import i18next from "i18next"; | ||
| import type { JSX } from "react"; | ||
| import { SearchBox } from "../search-box"; | ||
|
|
||
| /** | ||
| * Server-renderable stand-in for the desktop navbar Search (#1664). | ||
| * | ||
| * The real Search is a `dynamic({ssr:false})` chunk gated on a client-set | ||
| * `isDesktop`, so its slot used to server-render empty and the input popped | ||
| * in seconds after first paint. This shell reuses the same lightweight | ||
| * SearchBox inside the same idle SuggestionList wrapper markup | ||
| * (`suggestion relative` + trailing div), so it is pixel-identical to the | ||
| * idle live component and is replaced in place when Search mounts. | ||
| * | ||
| * Deliberately NOT the live placeholder: the live one interpolates | ||
| * searchIndexCount, which is client data; the shell always shows the plain | ||
| * placeholder so the server output is stable. The input is readOnly until | ||
| * the live component takes over. | ||
| */ | ||
| export function NavbarSearchShell(): JSX.Element { | ||
| return ( | ||
| <div className="suggestion relative"> | ||
| <SearchBox placeholder={i18next.t("search.placeholder")} value="" readOnly={true} /> | ||
| <div /> | ||
| </div> | ||
| ); | ||
| } |
60 changes: 60 additions & 0 deletions
60
apps/web/src/specs/features/shared/navbar-first-paint.spec.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { vi, describe, it, expect } from "vitest"; | ||
| import { render } from "@testing-library/react"; | ||
| import { renderToString } from "react-dom/server"; | ||
| import "@testing-library/jest-dom"; | ||
|
|
||
| const captured = vi.hoisted(() => ({ dynamicOptions: [] as Array<Record<string, unknown>> })); | ||
| vi.mock("next/dynamic", () => ({ | ||
| default: (_importer: unknown, opts?: Record<string, unknown>) => { | ||
| captured.dynamicOptions.push(opts ?? {}); | ||
| return () => null; | ||
| } | ||
| })); | ||
|
|
||
| import { NavbarMainSidebarToggle } from "@/features/shared/navbar/navbar-main-sidebar-toggle"; | ||
| import { NavbarSearchShell } from "@/features/shared/navbar/navbar-search-shell"; | ||
| import "@/features/shared/navbar/navbar-search-dynamic"; | ||
|
|
||
|
qodo-code-review[bot] marked this conversation as resolved.
|
||
| /* | ||
| Pins for #1664: the navbar must be complete at first paint. | ||
| */ | ||
| describe("NavbarMainSidebarToggle logo", () => { | ||
| it("is not lazy-loaded (paints with the first frame)", () => { | ||
| const { container } = render(<NavbarMainSidebarToggle onClick={vi.fn()} />); | ||
| const img = container.querySelector("img.logo"); | ||
| expect(img).toBeInTheDocument(); | ||
| // next/image without `priority` emits loading="lazy", which deferred the | ||
| // request past the first layout and made the logo pop in at ~2.4s. | ||
| expect(img).not.toHaveAttribute("loading", "lazy"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Search dynamic handoff", () => { | ||
| it("keeps the shell visible while the search chunk loads", () => { | ||
| /* | ||
| When isDesktop flips true, the slot swaps to the dynamic component; | ||
| Next's DEFAULT loading state renders null, which would flash the slot | ||
| empty until the chunk arrives. The dynamic() options must therefore | ||
| provide the shell as the loading fallback (#1665 review). | ||
| */ | ||
| // Other modules in the import graph may register their own dynamic() | ||
| // components; find the one whose loading fallback renders the shell. | ||
| const fallbacks = captured.dynamicOptions | ||
| .filter((o) => typeof o.loading === "function") | ||
| .map((o) => renderToString((o.loading as () => JSX.Element)())); | ||
| const shellFallback = fallbacks.find((html) => html.includes("search-box")); | ||
| expect(shellFallback).toBeDefined(); | ||
| expect(shellFallback).toContain('placeholder="search.placeholder"'); | ||
| }); | ||
| }); | ||
|
|
||
| describe("NavbarSearchShell", () => { | ||
| it("server-renders the idle search input markup", () => { | ||
| // renderToString runs no effects, exactly like the server. | ||
| const html = renderToString(<NavbarSearchShell />); | ||
| expect(html).toContain("suggestion relative"); | ||
| expect(html).toContain("search-box"); | ||
| expect(html).toContain('placeholder="search.placeholder"'); | ||
| expect(html).toContain("<input"); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.