Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 8 Skipped Deployments
|
WalkthroughThis PR introduces a complete Next.js learning application under Changes
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![snyk-io[bot]](https://avatars.githubusercontent.com/in/372950?s=60&v=4){kind=small}
...earn/registry/default/platform/platform-kit-nextjs/app/api/supabase-proxy/[...path]/route.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@apps/learn/app/`(app)/[...slug]/page.tsx:
- Around line 49-59: The metadata construction can throw if
mainMetadata.openGraph is undefined; update the openGraph spread to guard
against that by defaulting to an empty object (e.g., use mainMetadata.openGraph
|| {} or destructure a default like openGraph: mainOpenGraph = {}) so that the
openGraph property in the metadata object always spreads from a defined object
before adding title, description, type, and url (used with
absoluteUrl(doc.slug)).
In `@apps/learn/app/`(app)/page.tsx:
- Around line 114-116: The placeholder href "/..." in the Button/Link render
will break routing; update the Link to use a real course URL derived from your
course data (e.g., use course.slug or course.id), ensure each course object in
the courses array includes a slug or href property, and replace Link href="/..."
with something like href={`/${course.slug}`} or href={course.href} in the
component where Button and Link are rendered so navigation points to the actual
course page.
In `@apps/learn/components/course-hero.tsx`:
- Around line 19-41: The CourseHero component accepts props (CourseHeroProps)
including subtitle, description, stats, and instructors but currently renders
hardcoded text; update CourseHero to use the passed-in subtitle and description
values (replace the hardcoded paragraph strings with {subtitle} and
{description}) and either render or remove stats/instructors from the props
(e.g., map over instructors to display names/avatar or remove stats/instructors
from CourseHeroProps) so the component matches the content layer requirements
and no unused props remain.
In `@apps/learn/components/sidebar.tsx`:
- Around line 9-11: getInternalContentPaths() returns a Set<string> which cannot
be serialized when passed from the server component Sidebar to the client
component SideNavigation; convert the Set to an array (e.g.
Array.from(getInternalContentPaths())) before passing it into <SideNavigation
...> and update SideNavigationProps (and any consumer typings) to expect
internalPaths: string[] instead of a Set so the client component receives a
serializable array.
In `@apps/learn/components/toc.tsx`:
- Line 1: Remove the top-line "// `@ts-nocheck`" and fix the TypeScript errors by
adding explicit types: annotate the Toc component props (e.g., define an
interface for incoming props), give the local state created with useState on
line 42 an explicit generic (for example useState<string[]>([]) or the correct
item type instead of implicit any), and type any event handlers/callbacks and
return values used inside the component (e.g., onClick handlers, map callbacks).
Update imports to include React types if needed (FC, ReactNode) and ensure all
referenced identifiers in the component (Toc, the state variable from useState,
and any handler names) have matching type annotations so the file can safely
remove `@ts-nocheck`.
In `@apps/learn/contentlayer.config.js`:
- Around line 194-196: The theme path passed into loadTheme in the
getHighlighter function is using path.join(process.cwd(),
'/lib/themes/supabase-2.json') which forces an absolute root path; update the
call in getHighlighter to remove the leading slash so it uses
path.join(process.cwd(), 'lib/themes/supabase-2.json') (i.e., change '/lib/...'
to 'lib/...') so the theme file resolves relative to process.cwd() before
calling loadTheme and then getHighlighter.
In `@apps/learn/scripts/sync-internal-content.mts`:
- Around line 28-29: The current use of repoUrl (const repoUrl =
`https://${token}@github.com/${repo}.git`) together with run (const run = (cmd:
string) => execSync(cmd, { stdio: 'inherit' })) risks leaking the token in CI
logs if git fails; change run to call execSync with stdio: 'pipe' and capture
stdout/stderr so you can sanitize any error before re-throwing or logging, and
avoid printing repoUrl directly — create a separate safeRepo string (without
token) for any logs and use the token-containing URL only as the command input
or via an environment-based auth mechanism (or Git credential helper) so the
token never appears in process output; update references to run and repoUrl
accordingly (e.g., where git clone is invoked) to ensure errors are sanitized
and token is not logged.
In `@apps/learn/styles/globals.css`:
- Around line 6-115: The Biome CSS parser is not configured to recognize
Tailwind at-rules like `@tailwind` and `@apply`, causing lint failures; open your
Biome config (biome.json) and under the css.parser object add
"tailwindDirectives": true (alongside any existing cssModules setting) so
css.parser becomes e.g. { "cssModules": true, "tailwindDirectives": true }; save
and re-run the linter to ensure `@tailwind/`@apply/@layer are accepted.
🟡 Minor comments (22)
apps/learn/tsconfig.scripts.json-1-13 (1)
1-13:⚠️ Potential issue | 🟡 MinorAdd
.mtsfiles to the TypeScript include patterns.The current
includepattern covers onlyscripts/**/*.ts, butsync-internal-content.mtsexists in the scripts directory and will not be type-checked. Include.mtsfiles (and.ctsfor future compatibility) in the TypeScript configuration.Suggested include expansion
- "include": [".contentlayer/generated", "scripts/**/*.ts"], + "include": [ + ".contentlayer/generated", + "scripts/**/*.ts", + "scripts/**/*.mts", + "scripts/**/*.cts" + ],apps/learn/content/platform/platform-kit.mdx-18-21 (1)
18-21:⚠️ Potential issue | 🟡 MinorGrammar: use “APIs” instead of “API's”.
✍️ Suggested edit
-The Platform Kit is a collection of customizable API's, hooks and components you can use to provide an embedded Supabase experience within your own platform. +The Platform Kit is a collection of customizable APIs, hooks and components you can use to provide an embedded Supabase experience within your own platform.apps/learn/components/what-will-i-learn.tsx-92-102 (1)
92-102:⚠️ Potential issue | 🟡 MinorMake the Smart Office layout responsive on small screens.
The grid uses
grid-cols-12withcol-span-6on all breakpoints, which keeps the image and list side-by-side on narrow screens. Stack them vertically on mobile for better readability.📱 Responsive fix
- <div className="grid grid-cols-12 gap-12 mt-6"> - <div className="col-span-6"> + <div className="grid grid-cols-1 md:grid-cols-12 gap-12 mt-6"> + <div className="col-span-12 md:col-span-6"> <Image src={smartOfficeImage} alt="Smart Office project dashboard screenshot" className="w-full h-auto" placeholder="blur" /> </div> - <div className="col-span-6"> + <div className="col-span-12 md:col-span-6"> <ul className="grid gap-6 text-foreground-light ">apps/learn/components/next-up.tsx-24-32 (1)
24-32:⚠️ Potential issue | 🟡 MinorAvoid nesting interactive elements — use Button's
asChildprop instead.The current structure (
<Link><Button>...</Button></Link>) nests interactive elements, creating invalid HTML and accessibility issues. TheButtoncomponent supportsasChildandiconRight, so restructure it as:- <Link href={href}> - <Button - type="default" - iconRight={<ArrowRight className="h-4 w-4" />} - className="flex items-center gap-2" - > - Start {chapterNumber ? `Chapter ${chapterNumber}` : 'Next'} - </Button> - </Link> + <Button + asChild + type="default" + iconRight={<ArrowRight className="h-4 w-4" />} + className="flex items-center gap-2" + > + <Link href={href}>Start {chapterNumber ? `Chapter ${chapterNumber}` : 'Next'}</Link> + </Button>apps/learn/content/foundations/architecture.mdx-16-18 (1)
16-18:⚠️ Potential issue | 🟡 MinorHyphenate compound adjective and fix typo.
Two minor text issues:
- Line 16: "open source" should be hyphenated as "open-source" when used as a compound adjective before a noun.
- Line 174: "from you database" should be "from your database".
📝 Proposed fixes
-Supabase is built on top of a powerful open source database called +Supabase is built on top of a powerful open-source database called-- **No separate backend required:** APIs are generated automatically from you database. +- **No separate backend required:** APIs are generated automatically from your database.apps/learn/content/foundations/authentication.mdx-25-31 (1)
25-31:⚠️ Potential issue | 🟡 MinorExample UUID is malformed.
The UUID
74d23f1b-1c94-3e7b-4d6a-9fa71e2a3c4ba987doesn't follow the standard UUID format (8-4-4-4-12 hexadecimal characters). The last segment has 16 characters instead of 12.📝 Proposed fix
-id: 74d23f1b-1c94-3e7b-4d6a-9fa71e2a3c4ba987 +id: 74d23f1b-1c94-3e7b-4d6a-9fa71e2a3c4bapps/learn/scripts/sync-internal-content.mts-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorSanitize shell arguments to prevent command injection.
The
branchvariable is interpolated directly into the shell command. While it originates from an environment variable (lower risk), unsanitized interpolation is a risky pattern. If the branch name contains shell metacharacters, it could lead to unexpected behavior.🛡️ Proposed fix using shell escaping
+import { quote } from 'shell-quote' + // Clone to temp directory -run(`git clone --depth 1 --branch ${branch} ${repoUrl} ${tmpDir}`) +run(`git clone --depth 1 --branch ${quote([branch])} ${quote([repoUrl])} ${quote([tmpDir])}`)Alternatively, use
execFileSyncwith an array of arguments to avoid shell interpolation entirely:import { execFileSync } from 'node:child_process' execFileSync('git', ['clone', '--depth', '1', '--branch', branch, repoUrl, tmpDir], { stdio: 'pipe' })apps/learn/styles/mdx.css-37-47 (1)
37-47:⚠️ Potential issue | 🟡 MinorDuplicate selector and inconsistent class naming.
There are two issues here:
- Lines 37-39 and 41-43 both target
.line--highlighted- these should be combined into one rule.- Line 45 uses
.line-highlighted(single dash) while lines 37 and 41 use.line--highlighted(double dash). This appears to be a typo.🔧 Proposed fix
[data-rehype-pretty-code-fragment] .line--highlighted { - `@apply` relative bg-surface-200 dark:bg-surface-100; -} - -[data-rehype-pretty-code-fragment] .line--highlighted { - `@apply` border-foreground-muted border-l-2; + `@apply` relative bg-surface-200 dark:bg-surface-100 border-foreground-muted border-l-2; } -[data-rehype-pretty-code-fragment] .line-highlighted span { +[data-rehype-pretty-code-fragment] .line--highlighted span { `@apply` relative; }apps/learn/components/site-footer.tsx-16-22 (1)
16-22:⚠️ Potential issue | 🟡 MinorGitHub link points to UI library, not Learn app
Line 17 links to
apps/ui-library, which doesn’t match the Learn app footer context. Consider linking to the Learn app directory or the repo root.🔧 Suggested update
- href="/supabase/supabase/tree/master/apps/ui-library" + href="/supabase/supabase/tree/master/apps/learn"apps/learn/content/foundations/edge-functions.mdx-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorReplace placeholder content before exposing this lesson
Line 7 is just “Soon...”. If this page is in navigation, users will land on an empty lesson. Consider adding at least a minimal outline or hiding it from navigation until content is ready.
apps/learn/README.md-52-54 (1)
52-54:⚠️ Potential issue | 🟡 MinorAdd a language to the fenced code block
Markdownlint (MD040) flags the fenced block at Line 52 for missing a language.
✏️ Suggested fix
-``` +```bash supabase gen types --local > registry/default/fixtures/database.types.ts</details> </blockquote></details> <details> <summary>apps/learn/components/theme-switcher-dropdown.tsx-16-16 (1)</summary><blockquote> `16-16`: _⚠️ Potential issue_ | _🟡 Minor_ **Unused import.** `RadioGroup_Shadcn_` is imported but only used in the unused `SingleThemeSelection` function. Remove this import when removing the dead code. </blockquote></details> <details> <summary>apps/learn/content/foundations/introduction.mdx-14-16 (1)</summary><blockquote> `14-16`: _⚠️ Potential issue_ | _🟡 Minor_ **Chapter count inconsistency.** The stats section shows "5 Chapters" but the course outline (line 51) states "7 chapters" and explicitly lists 7 chapters. Update this to match. <details> <summary>Suggested fix</summary> ```diff stats: - - label: 5 Chapters + - label: 7 Chapters icon: book-open accent: emeraldapps/learn/components/theme-switcher-dropdown.tsx-38-51 (1)
38-51:⚠️ Potential issue | 🟡 MinorRemove unused
SingleThemeSelectionfunction.This function is defined but never called anywhere in the component. The actual theme selection is handled by the
DropdownMenuRadioGroupbelow. This appears to be leftover code from development.Suggested removal
- function SingleThemeSelection() { - return ( - <form> - <RadioGroup_Shadcn_ - name="theme" - onValueChange={setTheme} - aria-label="Choose a theme" - defaultValue={theme} - value={theme} - className="flex flex-wrap gap-3" - ></RadioGroup_Shadcn_> - </form> - ) - } - const iconClasses = 'text-foreground-light group-data-[state=open]:text-foreground'apps/learn/context/framework-context.tsx-37-39 (1)
37-39:⚠️ Potential issue | 🟡 MinorError message references wrong provider name.
The error message says
CourseProviderbut the actual provider component is namedFrameworkProvider. This will confuse developers debugging the issue.🐛 Proposed fix
export function useCourse() { const context = useContext(CourseContext) if (context === undefined) { - throw new Error('useCourse must be used within a CourseProvider') + throw new Error('useCourse must be used within a FrameworkProvider') } return context }apps/learn/context/framework-context.tsx-17-23 (1)
17-23:⚠️ Potential issue | 🟡 MinorAdd try-catch for localStorage access.
localStorage.getItemcan throw in certain environments (private browsing mode, storage quota exceeded, or when cookies/storage are disabled). Wrap this in a try-catch to prevent runtime errors.🛡️ Proposed fix
useEffect(() => { - const storedCourse = localStorage.getItem('preferredCourse') - const validCourses: Course[] = ['foundations', 'smart-office', 'performance-scaling', 'debugging-operations'] - if (storedCourse && validCourses.includes(storedCourse as Course)) { - setCourseState(storedCourse as Course) + try { + const storedCourse = localStorage.getItem('preferredCourse') + const validCourses: Course[] = ['foundations', 'smart-office', 'performance-scaling', 'debugging-operations'] + if (storedCourse && validCourses.includes(storedCourse as Course)) { + setCourseState(storedCourse as Course) + } + } catch { + // localStorage may be unavailable (private browsing, storage disabled) } }, [])Similarly for
setCourse:const setCourse = (newCourse: Course) => { setCourseState(newCourse) - localStorage.setItem('preferredCourse', newCourse) + try { + localStorage.setItem('preferredCourse', newCourse) + } catch { + // localStorage may be unavailable + } }apps/learn/components/explore-more.tsx-71-84 (1)
71-84:⚠️ Potential issue | 🟡 MinorExternalLink icon displays for internal links.
The
ExternalLinkicon is rendered unconditionally insidecardContent, meaning it appears even for internal links. This is misleading UX - the icon should only appear for external links.🐛 Proposed fix
Move the
isExternalcheck inside the card content or pass it as a parameter:+ const cardContent = (isExternal: boolean) => ( <div className="relative bg-background border border-border rounded-lg shadow-sm p-6 h-full hover:shadow-md transition-shadow group"> - <div className="absolute top-4 right-4"> - <ExternalLink className="h-4 w-4 text-foreground-lighter opacity-50 group-hover:opacity-100 transition-opacity" /> - </div> + {isExternal && ( + <div className="absolute top-4 right-4"> + <ExternalLink className="h-4 w-4 text-foreground-lighter opacity-50 group-hover:opacity-100 transition-opacity" /> + </div> + )} <div className="flex flex-col gap-3"> <Icon className="h-6 w-6 text-foreground" /> <div> <h4 className="font-bold text-foreground mb-1">{item.title}</h4> <p className="text-sm text-foreground-light">{description}</p> </div> </div> </div> ) if (isExternal) { return ( <a key={index} href={item.link} target="_blank" rel="noopener noreferrer" className="block" > - {cardContent} + {cardContent(true)} </a> ) } return ( <Link key={index} href={item.link} className="block"> - {cardContent} + {cardContent(false)} </Link> )apps/learn/components/command.tsx-19-27 (1)
19-27:⚠️ Potential issue | 🟡 MinorGuard preview base URL when the branch URL is missing.
If
NEXT_PUBLIC_VERCEL_BRANCH_URLis undefined, the generated command becomeshttps://undefined/.... Add a fallback to avoid invalid commands.🐛 Suggested fix
if (process.env.NEXT_PUBLIC_VERCEL_TARGET_ENV === 'production') { // we have a special alias for the production environment, added in https://github.com/shadcn-ui/ui/pull/8161 return `@supabase` } else if (process.env.NEXT_PUBLIC_VERCEL_TARGET_ENV === 'preview') { - return `https://${process.env.NEXT_PUBLIC_VERCEL_BRANCH_URL}` + const branchUrl = process.env.NEXT_PUBLIC_VERCEL_BRANCH_URL + return branchUrl ? `https://${branchUrl}` : 'http://localhost:3004' } else { return 'http://localhost:3004' }apps/learn/components/side-navigation-item.tsx-89-110 (1)
89-110:⚠️ Potential issue | 🟡 MinorFix button attributes and remove the stray class.
buttondefaults totype="submit"and thezansclass looks accidental. Addingtype="button"plusaria-expandedimproves behavior and accessibility.🐛 Suggested fix
- <button - onClick={handleButtonClick} - className={cn( - 'w-full flex items-center justify-between gap-2 zans', - itemClasses - )} - > + <button + type="button" + aria-expanded={isOpen} + onClick={handleButtonClick} + className={cn('w-full flex items-center justify-between gap-2', itemClasses)} + >apps/learn/lib/merge-internal-content.ts-6-16 (1)
6-16:⚠️ Potential issue | 🟡 MinorRecursive href collection aligns with codebase patterns but not required for current nav structure.
The
getNavHrefsInSectionfunction collects hrefs from direct children only. While theSidebarNavItemtype supports arbitrary nesting (items?: NavItemWithChildren[]), the current nav config inapps/learn/config/docs.tsuses only two levels: sections (with href) and leaf items (with href, no nested items). The function works correctly for this structure.However, since
get-next-page.tsalready implements recursive traversal for the same type, and the type system allows deeper nesting, makinggetNavHrefsInSectionrecursive would improve consistency and future-proof the code against structural changes.♻️ Suggested fix
function getNavHrefsInSection(items?: SidebarNavItem[]): Set<string> { const hrefs = new Set<string>() - if (!items) return hrefs - - items.forEach((item) => { - if (item.href) { - hrefs.add(item.href) - } - }) + const visit = (nodes?: SidebarNavItem[]) => { + if (!nodes) return + nodes.forEach((item) => { + if (item.href) hrefs.add(item.href) + if (item.items?.length) visit(item.items) + }) + } + visit(items) return hrefs }apps/learn/components/command-menu.tsx-22-70 (1)
22-70:⚠️ Potential issue | 🟡 MinorFix CommandMenu prop typing—remove DialogProps and align with Button API.
CommandMenu accepts
DialogPropsbut spreads them onto<Button>, which expects Button-specific props. TheDialogProps(likeopen,onOpenChange) belong onCommandDialog(line 71), not the trigger Button.The
type="outline"is correct for the custom Supabase Button component used here—it's the CVA variant, not an HTML button type (which is set viahtmlTypeinternally). Remove the DialogProps import and type annotation, and either remove the spread or limit it to valid Button props.Suggested fix
-import { - Button, - CommandDialog, - CommandEmpty_Shadcn_, - CommandGroup_Shadcn_, - CommandInput_Shadcn_, - CommandItem_Shadcn_, - CommandList_Shadcn_, - CommandSeparator_Shadcn_, - DialogProps, -} from 'ui' +import { + Button, + CommandDialog, + CommandEmpty_Shadcn_, + CommandGroup_Shadcn_, + CommandInput_Shadcn_, + CommandItem_Shadcn_, + CommandList_Shadcn_, + CommandSeparator_Shadcn_, +} from 'ui' + +type CommandMenuProps = React.ComponentProps<typeof Button> -export function CommandMenu({ ...props }: DialogProps) { +export function CommandMenu({ ...buttonProps }: CommandMenuProps) {<Button type="outline" className={cn( `relative h-8 w-full justify-start rounded-[0.5rem] bg-background text-sm font-normal text-foreground-muted shadow-none sm:pr-12 hover:border-foreground-muted hover:bg-surface-100 hover:text-foreground-lighter ` )} onClick={() => setOpen(true)} - {...props} + {...buttonProps} >apps/learn/lib/get-next-page.ts-12-19 (1)
12-19:⚠️ Potential issue | 🟡 MinorInclude parent items with
hrefwhen flattening navigation.If section/group items (e.g.,
/foundations) have their own pages,flattenNavItemsdrops them because they also have children, sogetNextPagewill never find them and returnsnull. Consider adding the parent to the list before/after its children.✅ Suggested adjustment
for (const item of items) { - if (item.items && item.items.length > 0) { - // Recursively process children first - result.push(...flattenNavItems(item.items)) - } else if (item.href) { - // Only add items with href (leaf nodes) - result.push(item) - } + if (item.href) { + result.push(item) + } + if (item.items && item.items.length > 0) { + result.push(...flattenNavItems(item.items)) + } }
🧹 Nitpick comments (27)
apps/learn/lib/constants.ts (1)
1-1: Consider adding explicit validation for NEXT_PUBLIC_API_URL to improve error visibility.While the non-null assertion works within the build system's enforced environment variables (declared in turbo.json), it masks the issue if the variable is unexpectedly missing. For better error clarity at module load time, add an explicit check—particularly useful during local development. The
apps/docs/lib/constants.tsfile already demonstrates a more defensive approach with conditional fallback for dev environments.🔧 Suggested fix
-export const API_URL = process.env.NEXT_PUBLIC_API_URL! +const apiUrl = process.env.NEXT_PUBLIC_API_URL +if (!apiUrl) { + throw new Error('NEXT_PUBLIC_API_URL is required') +} +export const API_URL = apiUrlapps/learn/components/course-hero.tsx (1)
43-65: Course metadata is hardcoded; consider making it dynamic.The chapter count ("5 Chapters"), duration ("~1 hour"), and difficulty ("Beginner") are hardcoded. If this component is intended to be reusable across different courses, consider passing these values as props or deriving them from the
statsprop that's already defined in the interface.apps/learn/lib/get-current-chapter.ts (1)
16-16: Type assertion toanybypasses type safety.The
(doc as any)?.chapterNumberassertion suggestschapterNumberisn't properly typed in the generated contentlayerDoctype. Consider extending the Doc type or using a type guard to maintain type safety.💡 Suggested approach
If
chapterNumberis defined in contentlayer config, ensure the generated types include it. Otherwise, create a type guard:interface DocWithChapter { chapterNumber?: number } function hasChapterNumber(doc: unknown): doc is DocWithChapter { return typeof doc === 'object' && doc !== null && 'chapterNumber' in doc } // Usage const chapterNumber = hasChapterNumber(doc) ? doc.chapterNumber : undefinedapps/learn/components/use-local-storage.tsx (2)
27-44:setValuereference is unstable due tostoredValuedependency.Including
storedValuein the dependency array causessetValueto be recreated on every state change. This can lead to unnecessary re-renders in components that includesetValuein their effect dependencies.Use
setStoredValue's functional update form to access current state without the dependency:♻️ Proposed fix for stable setValue reference
const setValue = useCallback( (value: T | ((val: T) => T)) => { try { - // Allow value to be a function so we have same API as useState - const valueToStore = value instanceof Function ? value(storedValue) : value - // Save state - setStoredValue(valueToStore) - // Save to local storage - if (typeof window !== 'undefined') { - window.localStorage.setItem(key, JSON.stringify(valueToStore)) - } + setStoredValue((currentValue) => { + const valueToStore = value instanceof Function ? value(currentValue) : value + if (typeof window !== 'undefined') { + window.localStorage.setItem(key, JSON.stringify(valueToStore)) + } + return valueToStore + }) } catch (error) { - // A more advanced implementation would handle the error case - console.log(error) + console.error('useLocalStorage write error:', error) } }, - [key, storedValue] + [key] )
18-22: Useconsole.errorfor error logging.
console.logis used for error cases (Lines 20 and 40). Useconsole.errorto properly categorize these as errors in browser dev tools and logging systems.📝 Proposed fix
} catch (error) { - // If error also return initialValue - console.log(error) + console.error('useLocalStorage read error:', error) return initialValue }apps/learn/components/callout.tsx (1)
3-9: Consider extending base Alert props for type safety.The
...propsspread passes additional props toAlert_Shadcn_, butCalloutPropsdoesn't extend the Alert's prop type. This means TypeScript won't validate those props.💡 Suggested improvement
import { ComponentProps } from 'react' import { Alert_Shadcn_, AlertDescription_Shadcn_, AlertTitle_Shadcn_ } from 'ui' interface CalloutProps extends ComponentProps<typeof Alert_Shadcn_> { icon?: string title?: string children?: React.ReactNode }apps/learn/components/chapter-completion.tsx (1)
64-85: Consider memoizing the completion check to avoid unnecessary effect re-runs.The effect depends on
completedChapters, which may get a new array reference on each localStorage read/render. This could cause the effect to re-run more frequently than intended, potentially resetting the timer unnecessarily.♻️ Suggested improvement
Extract the completion check into a ref or memoized value:
+ const isChapterCompleted = completedChapters.includes(chapterNumber) + useEffect(() => { - if (isVisible && !isCompleted) { + if (isVisible && !isCompleted && !isChapterCompleted) { timerRef.current = setTimeout(() => { setIsCompleted(true) - if (!completedChapters.includes(chapterNumber)) { - setCompletedChapters([...completedChapters, chapterNumber]) - } + setCompletedChapters((prev) => + prev.includes(chapterNumber) ? prev : [...prev, chapterNumber] + ) }, 5000) } else if (!isVisible && timerRef.current) { clearTimeout(timerRef.current) timerRef.current = null } // ... - }, [isVisible, isCompleted, chapterNumber, completedChapters, setCompletedChapters]) + }, [isVisible, isCompleted, isChapterCompleted, chapterNumber, setCompletedChapters])This assumes
setCompletedChapterssupports functional updates. If it does, using a functional update avoids the stale closure issue.apps/learn/styles/mdx.css (1)
73-81: Consider removing commented-out code.If the preview grid background isn't needed, remove the commented block. If it's planned for future use, consider tracking it in an issue instead.
apps/learn/app/Providers.tsx (1)
12-25: Consider renaming to reflect composite responsibility.The component is named
ThemeProviderbut actually composes six different providers (Auth, Jotai, NextThemes, MobileMenu, Framework, Tooltip). A name likeAppProvidersorRootProviderswould better communicate its purpose and reduce confusion when developers look for theme-specific configuration.apps/learn/hooks/use-config.ts (1)
13-18: Consider namespacing the storage key.The storage key
'config'is generic and could potentially conflict with other applications or libraries using localStorage. Consider a namespaced key like'learn-config'or'supabase-learn-config'to avoid collisions.Suggested change
-const configAtom = atomWithStorage<Config>('config', { +const configAtom = atomWithStorage<Config>('learn-config', { style: 'default', radius: 0.5, sonnerPosition: 'bottom-right', sonnerExpand: false, })apps/learn/lib/telemetry.ts (1)
8-8: Inconsistent import path alias.This file uses
'lib/constants'whiletelemetry-wrapper.tsxuses'@/lib/constants'. For consistency across the codebase, consider using the same alias pattern.Suggested change
-import { API_URL } from 'lib/constants' +import { API_URL } from '@/lib/constants'apps/learn/components/theme-switcher-dropdown.tsx (1)
55-88: Unnecessary fragment wrapper.The outer
<>...</>fragment is not needed since there's only a single child element (DropdownMenu).Suggested simplification
return ( - <> - <DropdownMenu> + <DropdownMenu> ... - </DropdownMenu> - </> + </DropdownMenu> )apps/learn/lib/themes/supabase-2.json (1)
96-113: Duplicate scope definition.
entity.other.attribute-nameis defined in two separate token rules (lines 97 and 109). The second definition (lines 108-113) will override the first if both match. Consider consolidating these rules or removing the duplicate.Suggested consolidation
{ - "scope": ["entity.other.attribute-name"], - "settings": { - "foreground": "var(--code-token-property)" - } - }, - ... - { "scope": ["entity.other.attribute-name", "entity.other.inherited-class"], "settings": { "foreground": "var(--code-token-property)" } },apps/learn/context/mobile-menu-context.tsx (1)
28-35: Memoize the context value to avoid extra renders.
The comment says it’s memoized, but it isn’t. ConsideruseMemoso consumers don’t re-render on unrelated parent renders.Suggested diff
-import React, { ReactNode, useState, useCallback } from 'react' +import React, { ReactNode, useState, useCallback, useMemo } from 'react' @@ - // Memoize the context value to prevent unnecessary re-renders - const value: MobileMenuContextType = { - open, - setOpen: handleSetOpen, - toggle, - } + // Memoize the context value to prevent unnecessary re-renders + const value: MobileMenuContextType = useMemo( + () => ({ + open, + setOpen: handleSetOpen, + toggle, + }), + [open, handleSetOpen, toggle] + )apps/learn/app/(app)/layout.tsx (1)
16-16: Remove commented-out code.Line 16 contains commented-out
{children}which appears to be leftover from development. This is dead code that should be removed.🧹 Proposed fix
<main className="flex-1 max-w-site mx-auto w-full p-0"> - {/* {children} */} <div className="border-b">apps/learn/app/(app)/page.tsx (2)
6-7: Remove unused component.
HorizontalGridLineis defined but never used in this file.🧹 Proposed fix
-// Horizontal grid line component -const HorizontalGridLine = () => <div className="col-span-12 h-px bg-border/30" /> -
9-10: Redundant caching configuration.When
dynamic = 'force-dynamic'is set,revalidate = 0is redundant sinceforce-dynamicalready opts out of all caching. Consider removing one of these exports.🧹 Proposed fix
export const dynamic = 'force-dynamic' -export const revalidate = 0apps/learn/components/explore-more.tsx (1)
17-52: Consider using lookup maps for cleaner icon/description resolution.The
getIconandgetDescriptionfunctions use repetitive if-else chains with the same type checks. Using a map object would be more maintainable.♻️ Proposed refactor
const TYPE_CONFIG: Record<string, { icon: typeof BookOpen; description: string }> = { doc: { icon: BookA, description: 'Complete guides and references' }, documentation: { icon: BookA, description: 'Complete guides and references' }, reference: { icon: Code, description: 'Detailed API documentation' }, api: { icon: Code, description: 'Detailed API documentation' }, community: { icon: Users, description: 'Get help from the community' }, forum: { icon: Users, description: 'Get help from the community' }, video: { icon: IconYoutubeSolid, description: 'Watch video tutorials and guides' }, tutorial: { icon: IconYoutubeSolid, description: 'Watch video tutorials and guides' }, } const DEFAULT_CONFIG = { icon: BookA, description: 'Learn more about this topic' } function getTypeConfig(itemType?: string) { if (!itemType) return DEFAULT_CONFIG return TYPE_CONFIG[itemType.toLowerCase()] ?? DEFAULT_CONFIG }apps/learn/components/toc.tsx (1)
15-25: Redundant.flat()call afterflatMap.
flatMapalready flattens one level, so the subsequent.flat()on line 20 is redundant for single-level nesting.♻️ Proposed fix
const itemIds = React.useMemo( () => toc.items ? toc.items .flatMap((item) => [item.url, item?.items?.map((item) => item.url)]) - .flat() .filter(Boolean) .map((id) => id?.split('#')[1]) : [], [toc] )apps/learn/components/sidebar.tsx (1)
29-29: Conflictingfixedandmd:stickypositioning classes.The
fixedclass on line 29 is overridden bymd:stickyat medium breakpoints and above. However, belowmd, the element ishiddenanyway. Thefixedclass appears to be dead code.♻️ Proposed fix
- <aside className="fixed z-30 top-0 hidden h-screen w-full shrink-0 md:sticky md:block bg-200 border-r border-muted/50"> + <aside className="z-30 top-0 hidden h-screen w-full shrink-0 md:sticky md:block bg-200 border-r border-muted/50">apps/learn/content/foundations/quickstart.mdx (1)
12-67: Make the setup cards responsive on small screens.
grid-cols-2forces two columns even on narrow viewports. Consider a single-column default with a breakpoint for two columns.♻️ Suggested tweak
-<div className="grid grid-cols-2 gap-4 mt-8 sm:gap-6"> +<div className="grid grid-cols-1 gap-4 mt-8 sm:gap-6 md:grid-cols-2">apps/learn/components/command.tsx (1)
38-61: Coerce invalid stored package manager values.If localStorage holds an unexpected value, the Tabs component may render without an active tab. Normalizing prevents a blank state.
♻️ Suggested tweak
export function Command({ name, highlight, framework = 'react' }: CommandCopyProps) { const [value, setValue] = useLocalStorage(LOCAL_STORAGE_KEY, 'npm') + const isValidValue = (['npm', 'pnpm', 'yarn', 'bun'] as const).includes(value as PackageManager) + const selectedValue: PackageManager = isValidValue ? (value as PackageManager) : 'npm' const baseUrl = getBaseUrl() const componentPath = getComponentPath(name) @@ - return ( - <Tabs_Shadcn_ value={value} onValueChange={setValue} className="w-full"> + return ( + <Tabs_Shadcn_ value={selectedValue} onValueChange={setValue} className="w-full">apps/learn/scripts/build-llms-txt.ts (1)
15-17: Handle CRLF in frontmatter parsing.This regex fails on Windows line endings. Supporting
\r\nmakes the script more portable.♻️ Suggested tweak
- const frontmatterRegex = /---\n([\s\S]*?)\n---/ + const frontmatterRegex = /---\r?\n([\s\S]*?)\r?\n---/apps/learn/components/copy-button.tsx (1)
16-20: Avoid dangling timers inCopyButton.A timeout is scheduled on every state change (including when
hasCopiedisfalse) and isn’t cleared. Gate onhasCopiedand clean up the timer to avoid unnecessary work and potential setState-after-unmount.♻️ Proposed refactor
React.useEffect(() => { - setTimeout(() => { - setHasCopied(false) - }, 2000) + if (!hasCopied) return + const timeout = setTimeout(() => { + setHasCopied(false) + }, 2000) + return () => clearTimeout(timeout) }, [hasCopied])apps/learn/components/command-copy-button.tsx (1)
20-48: Keep telemetry parsing in sync with the shared implementation.
apps/ui-library/components/command-copy-button.tsxalready parses framework/title/package manager and supports additional frameworks (e.g., vue/nuxtjs) and patterns. Duplicating a narrower regex here can skew telemetry. Consider reusing the shared helper, or at least expand the supported framework list.🔧 Minimal alignment (if reusing the helper isn’t feasible)
- const frameworkMatch = cmd.match(/ui\/r\/.*?-(nextjs|react|react-router|tanstack)\.json/) + const frameworkMatch = cmd.match( + /ui\/r\/.*?-(nextjs|react|react-router|tanstack|vue|nuxtjs)\.json/ + ) ... - ? (frameworkMatch[1] as 'nextjs' | 'react-router' | 'tanstack' | 'react') + ? (frameworkMatch[1] as 'nextjs' | 'react-router' | 'tanstack' | 'react' | 'vue' | 'nuxtjs')apps/learn/lib/toc.ts (1)
1-2: Avoid blanket@ts-nocheckin the TOC utility.Disabling type checking for the whole module can hide regressions in the parsing pipeline. Prefer adding mdast/unist types or using targeted
@ts-expect-erroron known problem lines.If you want, I can help add the minimal typings to remove
@ts-nocheck.apps/learn/app/(app)/[...slug]/page.tsx (1)
78-149: Compute TOC only when it will be rendered.
getTableOfContentscan be avoided for docs without TOC to save work.♻️ Suggested tweak
- const toc = await getTableOfContents(doc.body.raw) + const toc = doc.toc ? await getTableOfContents(doc.body.raw) : null const nextPage = getNextPage(doc.slugAsParams) const currentChapter = getCurrentChapter(doc.slugAsParams) const slugSegments = doc.slugAsParams.split('/') const isIntroductionPage = slugSegments[slugSegments.length - 1] === 'introduction' @@ - {doc.toc && ( + {doc.toc && toc && ( <div className="hidden text-sm xl:block"> <div className="sticky top-16 -mt-10 pt-4"> <ScrollArea className="pb-10"> <div className="sticky top-16 -mt-10 h-[calc(100vh-3.5rem)] py-12"> <DashboardTableOfContents toc={toc} /> </div> </ScrollArea> </div> </div> )}
apps/learn/components/sidebar.tsx
Outdated
| export function Sidebar() { | ||
| // Get internal content paths on server side | ||
| const internalPaths = getInternalContentPaths() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if SideNavigation is a client component and what it expects
head -20 apps/learn/components/side-navigation.tsx
# Check what type SideNavigation expects for internalPaths
rg -n "internalPaths" apps/learn/components/side-navigation.tsx
# Also check getInternalContentPaths implementation to confirm return type
rg -n "function getInternalContentPaths\|const getInternalContentPaths" apps/learnRepository: supabase/supabase
Length of output: 1034
🏁 Script executed:
# Check sidebar.tsx to see full context and if it's a server component
cat -n apps/learn/components/sidebar.tsx | head -40
# Find getInternalContentPaths definition
rg -n "getInternalContentPaths" apps/learn --type ts --type tsx -A 5 | head -30Repository: supabase/supabase
Length of output: 1712
🏁 Script executed:
# Find getInternalContentPaths definition using different approach
find apps/learn -name "*get-internal-content*" -type f
# Look for the implementation
rg -n "getInternalContentPaths" apps/learn -A 10 | grep -E "^\s*(function|const|export)" -A 10Repository: supabase/supabase
Length of output: 99
🏁 Script executed:
# Read the get-internal-content.ts file to see the implementation
cat -n apps/learn/lib/get-internal-content.tsRepository: supabase/supabase
Length of output: 1700
Convert Set to array before passing to client component.
getInternalContentPaths() returns a Set<string>, but SideNavigation is a client component. Passing a Set from the server component directly to the client component will cause a serialization error because Sets are not serializable in React Server Components. Convert to an array: Array.from(getInternalContentPaths()) and update SideNavigationProps to expect internalPaths: string[].
🤖 Prompt for AI Agents
In `@apps/learn/components/sidebar.tsx` around lines 9 - 11,
getInternalContentPaths() returns a Set<string> which cannot be serialized when
passed from the server component Sidebar to the client component SideNavigation;
convert the Set to an array (e.g. Array.from(getInternalContentPaths())) before
passing it into <SideNavigation ...> and update SideNavigationProps (and any
consumer typings) to expect internalPaths: string[] instead of a Set so the
client component receives a serializable array.
There was a problem hiding this comment.
@saltcod this one needs fixing as it can break at runtime
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
🎭 Playwright Test Results
Details
Flaky testsFeatures › table-editor.spec.ts › table editor › add enums and show enums on table Skipped testsFeatures › sql-editor.spec.ts › SQL Editor › snippet favourite works as expected |
|
Lots of for the eslint(react/no-unescaped-entities) |
Regenerated lockfile to resolve missing entry for @supabase/postgrest-js@2.93.2 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Braintrust eval report
|
3 participants
wip
Summary by CodeRabbit
Release Notes