App router migration #1295
App router migration #1295
Conversation
- Create lib/openapi with cached loader, types, and helpers - Create app/(api-reference) route group with layout and providers - Create server components for resource sections, methods, schemas - Create client component islands for expandable properties and code examples - Create catch-all routes for /api-reference and /mapi-reference - Update next.config.js to remove rewrites for API reference pages Co-authored-by: chris <chris@knock.app>
- Remove old Pages Router files (/pages/api-reference, /pages/mapi-reference) - Add App Router compatible client components for resource sections, methods, and schemas - Refactor page-shell and loading components to use plain HTML/Tailwind (avoid @telegraph SSR issues) - Add App Router compatible API sections, rate limit, section heading, and content actions components - Add 'use client' directives to hooks that need them (useClipboard, useIsMounted) - Add 'use client' directive to CodeBlock component - Simplify pre-content to static HTML rendering - Simplify providers to avoid NextRouter dependency issues Co-authored-by: chris <chris@knock.app>
Co-authored-by: chris <chris@knock.app>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 11 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (path) { | ||
| const url = new URL(window.location.href); | ||
| url.pathname = path; | ||
| window.history.pushState({}, "", url.toString()); |
There was a problem hiding this comment.
Section heading click handler sets incorrect URL path
Medium Severity
The handleClick function in SectionHeadingAppRouter sets url.pathname = path where path is a relative resource path like /workflows. This doesn't include the base path (e.g., /api-reference), causing navigation to incorrect URLs like example.com/workflows instead of example.com/api-reference/workflows. The path prop passed to this component from Section originates from resourcePath in resource-section.tsx, which is constructed as /${resourceName} without the full route prefix.
| // Navigate to the section | ||
| const element = document.querySelector( | ||
| `[data-resource-path="${item.slug.replace(/^\//, "")}"]`, | ||
| ); |
There was a problem hiding this comment.
Sidebar item click selectors don't match DOM attribute values
Medium Severity
The handleClick function in SidebarItem queries for elements using [data-resource-path="${item.slug.replace(/^\//, "")}"] which produces selectors like [data-resource-path="create"]. However, the actual data-resource-path attributes in the DOM are set to values like /workflows/create (from method.path in resource-section-client.tsx). This mismatch means the query selector will never find the target element, causing the scroll-to-section functionality to silently fail.
| tooltip: "1 update / second / entity", | ||
| color: "bg-red-100 text-red-800", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Incomplete batchConfig causes crash for batch tier values
Low Severity
The batchConfig object only defines tier 1, but the component's Props type accepts tier: 1 | 2 | 3 | 4 | 5 with isBatch?: boolean. If isBatch is true and tier is any value other than 1, accessing batchConfig[tier].tooltip or batchConfig[tier].color will crash with "Cannot read properties of undefined" since batchConfig[2], batchConfig[3], etc. are all undefined.
Additional Locations (1)
| timeoutRef.current = setTimeout(() => { | ||
| setCopied(false); | ||
| }, 2000); | ||
| }; |
There was a problem hiding this comment.
Missing timeout cleanup causes memory leak on unmount
Low Severity
The ContentActionsAppRouter component creates a timeout via timeoutRef but lacks a cleanup effect to clear it on unmount. If the component unmounts while a timeout is pending (e.g., user navigates away within 2 seconds of copying), the timeout will still fire and attempt to call setCopied(false) on an unmounted component. This causes a memory leak and React's "Can't perform a state update on an unmounted component" warning. The existing useClipboard hook in hooks/useClipboard.ts correctly implements this cleanup pattern with a useEffect return function.
| return `${items.title || items.type || "unknown"}[]`; | ||
| } | ||
| return schema.type || "unknown"; | ||
| } |
There was a problem hiding this comment.
Duplicated getTypeString utility across five files
Medium Severity
The getTypeString function is duplicated 5 times across different component files with nearly identical implementations. This should be extracted to a shared utility in lib/openapi/helpers.ts to avoid redundancy and ensure consistent bug fixes.
| } from "../../../../lib/openapi/types"; | ||
| import { ResourceSectionClient } from "./resource-section-client"; | ||
| import { MethodContentClient } from "./method-content-client"; | ||
| import { SchemaSectionClient } from "./schema-section-client"; |
There was a problem hiding this comment.
Unused imports MethodContentClient and SchemaSectionClient
Low Severity
MethodContentClient and SchemaSectionClient are imported but never used in this file. The component only renders ResourceSectionClient. These imports should be removed.
| } | ||
|
|
||
| export function ContentActionsAppRouter({ | ||
| mdPath, |
There was a problem hiding this comment.
Unused mdPath prop in ContentActionsAppRouter
Low Severity
The mdPath prop is declared in the interface and destructured in the component but never used in the function body. Either remove it or implement the intended functionality (e.g., edit on GitHub link).
| // Simple sidebar context for same-page routing | ||
| const SidebarContext = createContext<{ samePageRouting: boolean }>({ | ||
| samePageRouting: true, | ||
| }); |
There was a problem hiding this comment.
Unused SidebarContext is created but never consumed
Low Severity
SidebarContext is created with createContext and wrapped with Provider, but useContext(SidebarContext) is never called anywhere in the codebase. This context has no consumers and should be removed.
| </ExampleColumn> | ||
| </Section> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Entire MethodContent server component is unused
Medium Severity
The MethodContent async server component (258 lines) is defined but never imported or used anywhere. The app uses MethodContentClient instead. This entire file appears to be dead code.
| return { modelName, schema }; | ||
| }) | ||
| .filter((s): s is SchemaData => s !== null); | ||
| } |
There was a problem hiding this comment.
Unused helper functions getResourceMethods and getResourceSchemas
Low Severity
getResourceMethods and getResourceSchemas are exported but never called anywhere in the codebase. They only appear in their definitions and the migration plan markdown. Remove these unused functions.
Description
This PR migrates the
/api-referenceand/mapi-referencedocumentation to the Next.js App Router.Why: This migration leverages React Server Components for improved performance, better aligns with the latest Next.js architecture, and provides a more robust and scalable foundation for our API documentation.
How:
app/(api-reference)route group with[[...slug]]catch-all routes for both API and Management API references.lib/openapi/) for efficient spec loading, processing, and schema referencing./api-referenceand/mapi-referencewith the new App Router structure."use client"directives, resolvingnext/routerdependencies, and adapting@telegraphcomponent usage for SSR contexts.next.config.jsto remove outdated rewrites and redirects, allowing the App Router to manage these routes.The migration ensures that all build, type-check, and linting checks pass, and the API reference pages are now fully served by the App Router.
Todos
Tasks
This PR addresses the migration detailed in
api-reference-migration-plan.md.