typegen web ui #78
typegen web ui #78
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a TypeGen web UI and Hono-based API, fmdapi/fmodata support (env handling, metadata download/parse, OData type generation), many new web UI components/hooks, registry and fmodata enhancements (webhooks), workspace/package manifest updates, and dependency bumps (zod v4, TypeScript, Vitest). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant UI as TypeGen Web UI
participant API as TypeGen API (Hono)
participant FS as Filesystem
participant FM as FileMaker Server
User->>Browser: navigate to UI
Browser->>UI: load assets (vite dev or static)
UI->>API: GET /api/config
API->>FS: read configPath
FS-->>API: file content / not-found
API->>API: parse JSONC & validate (zod)
API-->>UI: 200 { exists, path, config } / 500 error
User->>UI: Save config
UI->>API: POST /api/config { config }
API->>API: validate payload (zod)
alt valid
API->>FS: write formatted JSON
FS-->>API: OK
API-->>UI: { success: true }
else invalid
API-->>UI: { success: false, issues }
end
User->>UI: Test connection (choose config)
UI->>API: POST /api/test-connection { config? }
API->>API: resolve envs & create client
API->>FM: request layouts / test endpoints
alt FM success
FM-->>API: layouts / ok
API-->>UI: success + layouts
else FM error
FM-->>API: error
API-->>UI: error details (kind/code)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Potential hotspots to focus review on:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/registry/package.json (1)
42-48: Version mismatch:@vitest/coverage-v8should matchvitestmajor version.
vitestis upgraded to^4.0.7but@vitest/coverage-v8remains at^2.1.8. These packages should have matching major versions to ensure compatibility."devDependencies": { - "@vitest/coverage-v8": "^2.1.8", + "@vitest/coverage-v8": "^4.0.7", "chokidar-cli": "^3.0.0",
🟡 Minor comments (26)
packages/typegen/web/src/components/ui/accordion.tsx-86-91 (1)
86-91: Inconsistentindicatortype — missing'none'option.The
AccordionContextType(line 77) andaccordionTriggerVariants(line 48) both support'none'as an indicator value, but theAccordioncomponent's props type only allows'arrow' | 'plus'. This prevents consumers from explicitly disabling the indicator.Apply this diff to include the
'none'option:function Accordion( props: React.ComponentProps<typeof AccordionPrimitive.Root> & VariantProps<typeof accordionRootVariants> & { - indicator?: 'arrow' | 'plus'; + indicator?: 'arrow' | 'plus' | 'none'; }, ) {packages/typegen/web/src/components/ui/accordion.tsx-143-156 (1)
143-156:classNameapplied to both wrapper and inner div.The
classNameprop is merged into both theAccordionPrimitive.Content(line 150) and the inner<div>(line 153). This could cause unexpected behavior if a consumer passes layout or spacing classes, as they'll be applied twice.Apply this diff to apply
classNameonly to the outer wrapper:return ( <AccordionPrimitive.Content data-slot="accordion-content" className={cn(accordionContentVariants({ variant }), className)} {...rest} > - <div className={cn('pb-5 pt-0', className)}>{children}</div> + <div className="pb-5 pt-0">{children}</div> </AccordionPrimitive.Content> );packages/typegen/web/src/components/tree/default.tsx-85-88 (1)
85-88: Potential runtime error ingetChildrenif itemId is missing.If
getChildrenis called with anitemIdthat doesn't exist initems, accessingitems[itemId].childrenwill throw aTypeErrorbecauseitems[itemId]isundefined.Apply this diff to add a safe access pattern consistent with
getItem:dataLoader: { getItem: (itemId) => items[itemId], - getChildren: (itemId) => items[itemId].children ?? [], + getChildren: (itemId) => items[itemId]?.children ?? [], },packages/typegen/web/src/components/ui/tree.tsx-124-134 (1)
124-134: Missing defensive checks in TreeItemLabel.
TreeItemLabelcallsitem.isFolder(),item.isExpanded(), anditem.getItemName()directly without the defensivetypeofchecks used inTreeItem. For consistency and robustness, consider applying the same pattern here.- {item.isFolder() && + {typeof item.isFolder === 'function' && item.isFolder() && (toggleIconType === 'plus-minus' ? ( - item.isExpanded() ? ( + typeof item.isExpanded === 'function' && item.isExpanded() ? ( <SquareMinus className="text-muted-foreground size-3.5" stroke="currentColor" strokeWidth="1" /> ) : ( <SquarePlus className="text-muted-foreground size-3.5" stroke="currentColor" strokeWidth="1" /> ) ) : ( <ChevronDownIcon className="text-muted-foreground size-4 in-aria-[expanded=false]:-rotate-90" /> ))} - {children || (typeof item.getItemName === 'function' ? item.getItemName() : null)} + {children || (typeof item.getItemName === 'function' ? item.getItemName() : null)}packages/typegen/web/src/components/ui/tree.tsx-88-94 (1)
88-94: Inconsistent defensive checks for item methods.Lines 88-92 defensively check if methods like
isFocused,isFolder,isSelectedexist before calling them, but line 93 callsitem.isExpanded()directly without the same check. This inconsistency could cause a runtime error if the item instance doesn't haveisExpanded.Apply this diff for consistency:
data-search-match={typeof item.isMatchingSearch === 'function' ? item.isMatchingSearch() || false : undefined} - aria-expanded={item.isExpanded()} + aria-expanded={typeof item.isExpanded === 'function' ? item.isExpanded() : undefined}packages/typegen/web/src/components/button/loading.tsx-11-14 (1)
11-14: Update comment to match actual interval.The comment states "every 4 seconds" but the interval is set to 1000ms (1 second).
Apply this diff:
- // Automatically toggle button state every 4 seconds + // Automatically toggle button state every 1 second const interval = setInterval(() => {packages/typegen/web/src/components/ui/alert.tsx-71-75 (1)
71-75: Typo in data-slot selector for mono solid variant.The selector
*:data-slot-[alert=close]on line 74 appears malformed. Based on the pattern used elsewhere (e.g.,[&_[data-slot=alert-close]]), this should likely be[&_[data-slot=alert-close]].- className: 'bg-zinc-950 text-white dark:bg-zinc-300 dark:text-black *:data-slot-[alert=close]:text-white', + className: 'bg-zinc-950 text-white dark:bg-zinc-300 dark:text-black [&_[data-slot=alert-close]]:text-white',packages/typegen/web/src/components/ui/alert.tsx-31-34 (1)
31-34: Inconsistent data attribute selector in size variants.Line 33 uses
*:data-alert-iconwhich appears to be a typo. Other size variants use*:data-slot=alert-icon(line 31-32). This inconsistency will cause the small size styling to not apply correctly to the alert icon.- sm: 'rounded-md px-3 py-2.5 gap-2 text-xs [&>[data-slot=alert-icon]>svg]:size-4 *:data-alert-icon:mt-0.5 [&_[data-slot=alert-close]]:mt-0.25 [&_[data-slot=alert-close]_svg]:size-3.5', + sm: 'rounded-md px-3 py-2.5 gap-2 text-xs [&>[data-slot=alert-icon]>svg]:size-4 *:data-slot=alert-icon:mt-0.5 [&_[data-slot=alert-close]]:mt-0.25 [&_[data-slot=alert-close]_svg]:size-3.5',packages/typegen/web/src/components/badge/circle.tsx-1-59 (1)
1-59: Rename demo component to be consistent with other demos.The default export should be named
BadgeCircleDemoto match the naming convention used by other demo components in the codebase (ButtonDemo,DialogDemo,InputDemo,SwitchDemo,SelectDemo,ComboboxDemo). The genericComponentname is inconsistent and lacks clarity about the component's purpose.Additionally, consider whether this and similar unused demo files should be relocated to a dedicated
examples/or__demos__/directory to avoid bundling them into the production build.packages/typegen/web/package.json-36-36 (1)
36-36: Fix Zod 4 incompatibility: replace.check()method in form schemaThe
.check()method doesn't exist in Zod 4 and should be replaced with.email(). Atpackages/typegen/web/src/components/form/default.tsxline 22, change:email: z.string().check(z.email('Please enter a valid email address.')),to:
email: z.string().email('Please enter a valid email address.'),The rest of the codebase's Zod patterns (z.record with both key and value schemas, z.default usage, etc.) are already Zod 4 compatible.
packages/typegen/web/src/components/input/addon.tsx-1-29 (1)
1-29: This demo component is unused and should be removed or relocated.
InputDemois not imported anywhere in the codebase. While Vite's tree-shaking will exclude it from the bundle, keeping example/demo components in the production source directory alongside UI components is poor organization. Relocate to a dedicated examples directory or remove entirely—similar to other demo files likebutton/loading.tsx,dialog/default.tsx, etc.packages/typegen/web/src/hooks/useConfig.ts-35-43 (1)
35-43: Remove debug logging.The
console.logstatement appears to be a debug artifact that should be removed before merging.const saveMutation = useMutation({ mutationFn: async (configsToSave: SingleConfig[]) => { - console.log("configsToSave", configsToSave); return await saveConfig(configsToSave); },packages/typegen/web/src/App.tsx-54-61 (1)
54-61: Missingformin useEffect dependency array.The
form.reset()call depends on theformobject, which should be included in the dependency array. WhileformfromuseFormis typically stable, the linter will flag this as a missing dependency.useEffect(() => { console.log("configData from useEffect", configDataResponse); if (configDataResponse) { const configData = configDataResponse?.config; const serverConfigs = normalizeConfig(configData); form.reset({ config: serverConfigs }); } - }, [configDataResponse]); + }, [configDataResponse, form]);packages/typegen/web/src/components/ConfigList.tsx-46-57 (1)
46-57: Add keyboard accessibility to clickable list items.The clickable
divelements are not accessible via keyboard. Users navigating with keyboard or screen readers cannot activate these items. Consider using abuttonelement or adding proper ARIA attributes and keyboard handlers.- <div + <button key={index} - className="config-list-item" + type="button" + className="config-list-item w-full text-left" onClick={() => onSelectConfig(index)} > <div className="config-list-item-content"> <span className="config-list-item-label"> {getConfigLabel(config, index)} </span> <span className="config-list-item-arrow">→</span> </div> - </div> + </button>packages/typegen/web/src/App.tsx-188-194 (1)
188-194: Guard against undefined config access.
configs[index]could beundefinedifconfigsandfieldsare temporarily out of sync during re-renders. Add a guard to prevent passingundefinedtoConfigSummary.{fields.map((field, index) => { const config = configs[index]; + if (!config) return null; return ( <AccordionItem key={field.id} value={index.toString()}> <AccordionTrigger> <ConfigSummary config={config} />packages/typegen/web/src/components/ui/button.tsx-310-319 (1)
310-319: Typo in CSS selectors - extra opening bracket.Lines 313 and 318 have malformed Tailwind selectors with an extra
[at the start, which will prevent the SVG sizing from being applied correctly.{ size: 'xs', mode: 'icon', - className: 'w-7 h-7 p-0 [[&_svg:not([class*=size-])]:size-3.5', + className: 'w-7 h-7 p-0 [&_svg:not([class*=size-])]:size-3.5', }, { size: 'sm', mode: 'icon', - className: 'w-8 h-8 p-0 [[&_svg:not([class*=size-])]:size-3.5', + className: 'w-8 h-8 p-0 [&_svg:not([class*=size-])]:size-3.5', },packages/typegen/web/vite.config.ts-44-56 (1)
44-56: Proxy error handler may leave requests hanging.When
ECONNREFUSEDis caught and silently returned, no response is sent to the client. This can cause the request to hang until timeout. Consider sending an error response.proxy.on("error", (err, _req, res) => { // Suppress ECONNREFUSED errors during startup if ( err && (err as any).code === "ECONNREFUSED" && res && !res.headersSent ) { - // Silently handle connection refused - the retry logic will handle it - return; + // Send 503 so client-side retry logic can handle it + res.writeHead(503, { "Content-Type": "text/plain" }); + res.end("Server starting up, please retry"); } });packages/typegen/web/src/components/EnvVarDialog.tsx-347-377 (1)
347-377: The warning indicator placement appears incorrect.The
⚠️emoji is placed inside parentheses which results in output like"Server (⚠️)"whensuspectedField === "server", or"Server ()"otherwise - leaving empty parentheses. Consider moving the indicator outside or removing the parentheses:{errorDetails.details.missing.server && ( <li> - Server ( - {errorDetails.suspectedField === "server" && - "⚠️"} - ) + Server + {errorDetails.suspectedField === "server" && + " ⚠️"} </li> )}Apply similar changes to Database, Authentication, and Password list items.
packages/typegen/src/server/api.ts-1-41 (1)
1-41: Remove duplicate fileapi.ts.This module is never imported or used anywhere in the codebase. The API endpoints are instead implemented in
app.tsviacreateApiApp, which is the only implementation referenced byindex.ts. This file is dead code and should be deleted.packages/typegen/src/server/api.ts-61-73 (1)
61-73: Missing schema validation after parsing JSONC.The JSONC is parsed but not validated against
typegenConfigschema before returning. Compare toapp.tswhich doestypegenConfig.parse(rawJson). This could return malformed config data.const raw = fs.readFileSync(fullPath, "utf8"); const parsed = parse(raw); + const validated = typegenConfig.parse(parsed); return { status: 200, headers: { "Content-Type": "application/json" }, body: JSON.stringify({ exists: true, path: configPath, - config: parsed, + config: validated.config, }), };packages/typegen/web/src/components/ui/badge.tsx-6-12 (1)
6-12: BadgeProps interface has unuseddotClassNameproperty.The
dotClassNameproperty is defined inBadgePropsbut is never used in theBadgecomponent implementation (lines 186-209). Either remove it or implement the functionality.export interface BadgeProps extends React.HTMLAttributes<HTMLDivElement>, VariantProps<typeof badgeVariants> { asChild?: boolean; - dotClassName?: string; disabled?: boolean; }packages/typegen/web/src/components/ui/badge.tsx-14-18 (1)
14-18: BadgeButtonProps extends HTMLDivElement but component renders as span/button.
BadgeButtonPropsextendsReact.ButtonHTMLAttributes<HTMLDivElement>which is semantically incorrect—button attributes on a div type. The component renders as aspanwithrole="button"(line 223).Consider aligning the type with the actual element:
export interface BadgeButtonProps - extends React.ButtonHTMLAttributes<HTMLDivElement>, + extends React.HTMLAttributes<HTMLSpanElement>, VariantProps<typeof badgeButtonVariants> { asChild?: boolean; }packages/typegen/src/server/createDataApiClient.ts-71-120 (1)
71-120: Consider logging sensitive information carefully.The error logging on lines 73-78 outputs the values of
server,db,apiKey, andusernameto the console. While this is helpful for debugging, logging credentials (even partial) could be a security concern in production environments.Consider masking or omitting the
apiKeyvalue in logs:console.error("Missing required environment variables", { server, db, - apiKey, + apiKey: apiKey ? "[REDACTED]" : undefined, username, });packages/typegen/web/src/components/ui/switch.tsx-9-19 (1)
9-19: Context check is ineffective due to default value.The context is initialized with a default object
{ permanent: false }, soReact.useContext(SwitchContext)will never return a falsy value. The check on line 15 will never trigger the error, defeating the purpose of validating thatSwitchIndicatoris used within aSwitchcomponent.To properly detect missing provider, use
nullas default:-const SwitchContext = React.createContext<{ permanent: boolean }>({ - permanent: false, -}); +const SwitchContext = React.createContext<{ permanent: boolean } | null>(null); const useSwitchContext = () => { const context = React.useContext(SwitchContext); if (!context) { throw new Error("SwitchIndicator must be used within a Switch component"); } return context; };packages/typegen/web/src/components/ui/form.tsx-34-55 (1)
34-55: Context validation occurs after context usage.The check
if (!fieldContext)on line 41 happens afterfieldContext.nameis already accessed on line 39. Additionally, with the default empty object{},fieldContextwill always be truthy, making this check ineffective.Consider restructuring to validate contexts before usage:
const useFormField = () => { const fieldContext = React.useContext(FormFieldContext); const itemContext = React.useContext(FormItemContext); + + if (!fieldContext.name) { + throw new Error('useFormField should be used within <FormField>'); + } + + if (!itemContext.id) { + throw new Error('useFormField should be used within <FormItem>'); + } + const { getFieldState, formState } = useFormContext(); const fieldState = getFieldState(fieldContext.name, formState); - if (!fieldContext) { - throw new Error('useFormField should be used within <FormField>'); - } - const { id } = itemContext; // ... };packages/typegen/web/src/components/ui/select.tsx-171-185 (1)
171-185: Custom indicator lacks selection-aware visibility.When a custom
indicatoris provided (line 173), it's rendered directly without being wrapped inSelectPrimitive.ItemIndicator. This means custom indicators will always be visible, while the default Check icon (lines 181-183) only shows when the item is selected.If custom indicators should also only show when selected, wrap them in
ItemIndicator:{indicatorVisibility && (indicator && isValidElement(indicator) ? ( - indicator + <span + className={cn( + 'absolute flex h-3.5 w-3.5 items-center justify-center', + indicatorPosition === 'left' ? 'start-2' : 'end-2', + )} + > + <SelectPrimitive.ItemIndicator>{indicator}</SelectPrimitive.ItemIndicator> + </span> ) : (If intentional (custom indicators always visible), consider documenting this behavior difference.
🧹 Nitpick comments (49)
packages/typegen/web/src/App.css (2)
21-27: Verify color contrast for secondary text.The
.config-infouses#a1a1aawhich is a light gray on a dark background context. While likely readable, ensure this meets WCAG AA contrast requirements, especially for users with low vision. Consider testing with an accessibility checker.Consider extracting key colors to CSS variables (e.g.,
--color-text-secondary: #a1a1aa) to enable theme customization and easier future adjustments.:root { --color-text-primary: #e4e4e7; --color-text-secondary: #a1a1aa; --color-bg-dark: #18181b; --color-status-success: #16a34a; --color-status-error: #dc2626; /* ... other theme tokens ... */ } .config-info { color: var(--color-text-secondary); }
107-127: Verify error message color contrast.The error and validation messages use
#fca5a5text on#7f1d1dbackground. While likely readable, the contrast ratio should be verified to ensure it meets WCAG AA requirements (4.5:1 for normal text). Both colors are in the red spectrum, which may reduce perceptual contrast.As suggested earlier, consider using CSS variables for error states:
:root { --color-error-bg: #7f1d1d; --color-error-text: #fca5a5; --color-error-border: #991b1b; } .error-message, .validation-errors { background: var(--color-error-bg); border-color: var(--color-error-border); color: var(--color-error-text); }This enables easier theme adjustments and ensures consistency across all error UI.
packages/typegen/web/src/components/tree/default.tsx (1)
76-76: Consider a more descriptive component name.The default export is named
Component, which is generic and doesn't convey the component's purpose. Consider renaming to something likeCrmTreeExampleorDefaultTreeDemofor better discoverability and documentation.packages/typegen/web/src/components/ui/tree.tsx (1)
11-16: UseTreeInstance<T>from@headless-tree/corefor proper typing of thetreeprop.The
treeproperty is currently typed asanyin bothTreeContextValueandTreeProps, losing type safety and IDE support. ImportTreeInstancefrom@headless-tree/core(which is already a dependency) and type the property asTreeInstance<T>to align with the existing generic typing pattern used withItemInstance<T>.packages/typegen/src/formatting.ts (2)
15-23: Behavioral nuance whengetFileInfois unavailableUsing
prettier.getFileInfo?.(filePath) ?? { ignored: false }preserves ignore behavior whengetFileInfoexists, and falls back to “never ignored” when it doesn’t. That’s reasonable for robustness, but note that in environments wheregetFileInfois missing you’ll silently stop honoring.prettierignorefor these files.If that trade‑off isn’t desired, you might instead:
- Fail fast when
getFileInfois undefined, or- Log a warning once so it’s obvious ignores aren’t applied.
The
prettier.formatusage withfilepathlooks correct and keeps parser inference wired through the file path.
9-27: Avoid silently swallowing formatting errorsThe empty
catch (error) {}means any Prettier/IO error will be completely silent, butawait project.save();will still run, potentially leaving partially formatted files with no signal to the caller.Consider at least logging, or rethrowing after logging, e.g.:
- } catch (error) {} - await project.save(); + } catch (error) { + // Consider routing this through your logger instead of console + console.error("Failed to format and save source files", error); + } + await project.save();Or, if you want per‑file resilience, move the
try/catchinside the loop so one bad file doesn’t block formatting the rest.packages/typegen/scripts/build.ts (1)
1-5: Track removal of this placeholder file.This file exists only for git worktree compatibility, with the actual build logic moved to
build-copy.js. Consider tracking its removal once the worktree is updated to avoid maintaining dead code.Do you want me to open a new issue to track the removal of this placeholder file after the worktree update?
packages/typegen/web/src/components/switch/button.tsx (1)
5-5: Remove unused variable.The
idvariable is generated but never used. TheSwitchcomponent on line 10 doesn't reference this id.Apply this diff:
export default function SwitchDemo() { - const id = useId(); - return (And remove the id prop from the Switch:
- <Switch id={id} size="xl" className="w-full rounded-md h-9" thumbClassName="rounded-md" /> + <Switch size="xl" className="w-full rounded-md h-9" thumbClassName="rounded-md" />packages/typegen/web/src/components/ui/textarea.tsx (1)
29-35: Consider adding ref forwarding for form library compatibility.The Textarea component doesn't forward refs, which may cause issues with form libraries like react-hook-form that need to attach refs to form elements. Most UI primitives that wrap native form elements should use
React.forwardRef.Apply this diff to add ref forwarding:
-function Textarea({ +const Textarea = React.forwardRef< + HTMLTextAreaElement, + React.ComponentProps<'textarea'> & VariantProps<typeof textareaVariants> +>(function Textarea({ className, variant, ...props -}: React.ComponentProps<'textarea'> & VariantProps<typeof textareaVariants>) { - return <textarea data-slot="textarea" className={cn(textareaVariants({ variant }), className)} {...props} />; -} +}, ref) { + return <textarea ref={ref} data-slot="textarea" className={cn(textareaVariants({ variant }), className)} {...props} />; +});packages/typegen/web/src/components/LayoutSelector.tsx (2)
66-67: Type the error object instead of usingany.Casting to
anybypasses TypeScript's type safety. Define a proper error type for better type safety and maintainability.Define an error type:
type LayoutError = Error & { details?: { missing?: { server?: boolean; db?: boolean; auth?: boolean; password?: boolean; }; fmErrorCode?: string | number; suspectedField?: 'server' | 'db' | 'auth'; }; };Then use it:
- // Extract error details from the error object - const errorDetails = error && (error as any).details; + const errorDetails = error && (error as LayoutError).details;
205-219: Avoid string replacement for deriving form paths.Using string replacement to derive the schema name path from the layout name path is fragile and error-prone. If the field naming convention changes, this will break silently.
Consider a more explicit approach:
// If schema name is undefined or empty, set it to the layout name if (newValue) { - const schemaNamePath = path.replace( - ".layoutName", - ".schemaName", - ) as Path<FormData>; + const schemaNamePath = `config.${configIndex}.schemaName` as Path<FormData>; const currentSchemaName = getValues(schemaNamePath);Or pass the schema name path as a prop alongside the layout name path for explicitness.
packages/typegen/web/src/components/ui/alert.tsx (3)
218-220: Type annotation mismatch: usesHTMLHeadingElementbut renders adiv.
AlertTitledeclares its props asReact.HTMLAttributes<HTMLHeadingElement>, but the component renders a<div>. Consider usingHTMLDivElementfor consistency, or render an actual heading element (e.g.,<h4>) for semantic correctness.-function AlertTitle({ className, ...props }: React.HTMLAttributes<HTMLHeadingElement>) { - return <div data-slot="alert-title" className={cn('grow tracking-tight', className)} {...props} />; +function AlertTitle({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) { + return <div data-slot="alert-title" className={cn('grow tracking-tight', className)} {...props} />; }
238-246: Type annotation mismatch: usesHTMLParagraphElementbut renders adiv.Both
AlertDescriptionandAlertContentdeclare props asReact.HTMLAttributes<HTMLParagraphElement>, but render<div>elements. Update the type annotations for consistency.-function AlertDescription({ className, ...props }: React.HTMLAttributes<HTMLParagraphElement>) { +function AlertDescription({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) { return ( <div data-slot="alert-description" ... -function AlertContent({ className, ...props }: React.HTMLAttributes<HTMLParagraphElement>) { +function AlertContent({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) { return ( <div data-slot="alert-content"Also applies to: 248-256
230-236:AlertToolbarusesAlertIconPropsbut doesn't utilize variant props.
AlertToolbaracceptsAlertIconProps(which includesvariant,icon,appearance,sizefromVariantProps) but doesn't use any of these props. Consider using a simpler type likeReact.HTMLAttributes<HTMLDivElement>to avoid confusion.-function AlertToolbar({ children, className, ...props }: AlertIconProps) { +function AlertToolbar({ children, className, ...props }: React.HTMLAttributes<HTMLDivElement>) { return ( <div data-slot="alert-toolbar" className={cn(className)} {...props}> {children} </div> ); }packages/typegen/web/src/components/ui/command.tsx (1)
113-126: Misleading interface nameButtonArrowPropsforCommandCheck.The interface
ButtonArrowPropsis used forCommandCheckbut the name suggests it's for a button arrow component. Consider renaming for clarity.-interface ButtonArrowProps extends React.SVGProps<SVGSVGElement> { +interface CommandCheckProps extends React.SVGProps<SVGSVGElement> { icon?: LucideIcon; // Allows passing any Lucide icon } -function CommandCheck({ icon: Icon = Check, className, ...props }: ButtonArrowProps) { +function CommandCheck({ icon: Icon = Check, className, ...props }: CommandCheckProps) { return (packages/typegen/web/index.html (1)
8-29: Dark mode handling is functional; consider adding user preference persistence.The inline script correctly applies dark mode based on system preferences before the page renders, preventing visual flashing. The implementation properly:
- Detects system preference using
matchMedia- Applies the
darkclass todocumentElement- Listens for changes to system preference
Consider enhancing the dark mode logic to respect user preferences stored in localStorage:
<script> // Apply dark mode class based on system preference before page renders (function () { + // Check for stored user preference first + const stored = localStorage.getItem('theme'); + if (stored === 'dark') { + document.documentElement.classList.add("dark"); + return; + } else if (stored === 'light') { + return; + } + + // Fall back to system preference const prefersDark = window.matchMedia( "(prefers-color-scheme: dark)", ).matches; if (prefersDark) { document.documentElement.classList.add("dark"); } // Listen for changes to system preference window .matchMedia("(prefers-color-scheme: dark)") .addEventListener("change", (e) => { + // Only respond if user hasn't set a preference + if (localStorage.getItem('theme')) return; + if (e.matches) { document.documentElement.classList.add("dark"); } else { document.documentElement.classList.remove("dark"); } }); })(); </script>packages/typegen/web/src/lib/utils.ts (1)
10-12: LGTM! Consider standardizing thecnutility across the codebase.The implementation correctly combines
clsxfor conditional class normalization withtwMergefor Tailwind conflict resolution. This is the recommended pattern for a class name utility.However, there's an inconsistency in the codebase:
apps/docs/src/lib/cn.tsexportstwMergedirectly without theclsxwrapper:// apps/docs/src/lib/cn.ts twMerge as cnYour implementation here is more robust as it handles conditional classes properly. Consider updating the docs version to match this pattern for consistency.
#!/bin/bash # Search for other cn utility implementations across the codebase echo "=== Finding all cn utility definitions ===" rg -n --type=ts --type=tsx -A3 'export (function|const) cn' echo -e "\n=== Finding direct twMerge exports as cn ===" rg -n --type=ts 'twMerge as cn' echo -e "\n=== Finding cn utility imports ===" rg -n --type=ts --type=tsx "import.*cn.*from"packages/typegen/web/src/main.tsx (1)
7-15: Review aggressive retry configuration.The retry configuration attempts 5 retries with exponential backoff up to 30 seconds. For a local development UI server, this seems excessive and could lead to poor UX if the server is down.
Consider reducing retries for local development:
const queryClient = new QueryClient({ defaultOptions: { queries: { - retry: 5, + retry: 2, retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), refetchOnWindowFocus: false, }, }, });packages/typegen/web/src/lib/envValues.ts (2)
7-9: Clarify return behavior when envName is undefined.When
envNameis undefined, the function returnsundefinedinside thequeryFn, but the query still succeeds withdata: undefined. This could be confusing—consider whether the query should be disabled entirely or if this is the intended behavior.If the intention is to always have the query disabled when
envNameis undefined, the currentenabled: !!envNameon line 18 already handles this. However, the early return on line 8 is redundant. Consider removing it:queryFn: async () => { - if (!envName) return undefined; const res = await client.api["env-names"].$get({ query: { envName }, });Or if you want to keep the guard for type safety, make it explicit:
queryFn: async () => { - if (!envName) return undefined; + // This should never happen due to enabled: !!envName, but guard for type safety + if (!envName) throw new Error("envName is required"); const res = await client.api["env-names"].$get({
9-11: Rename endpoint to reflect its singular nature.The endpoint
/api/env-namesuses a plural name but fetches a single environment variable value for the providedenvName. This is inconsistent with the client-side naming (useEnvValue, query keyenvValue) and the response structure ({ value }). Rename to/api/env-valueor/api/env-namefor clarity.packages/typegen/web/src/components/ServerEnvField.tsx (1)
42-42: Review fallback logic with||operator.The expression
envName || defaultValuewill usedefaultValuewhenenvNameis an empty string (""), which may not be the intended behavior. If an empty string is a valid env name that should be queried, use nullish coalescing instead.If empty strings should be treated as "no env name", the current code is correct. However, if empty strings are valid env names, apply this diff:
- const { data: envValue, isLoading } = useEnvValue(envName || defaultValue); + const { data: envValue, isLoading } = useEnvValue(envName ?? defaultValue);And update line 54:
- {(envName || defaultValue) && ( + {(envName ?? defaultValue) && (packages/typegen/web/src/components/ConfigList.css (1)
80-81: Remove trailing blank lines.There are extra blank lines at the end of the file.
packages/typegen/web/src/hooks/useConfig.ts (1)
53-55: SimplifyisRetryinglogic.The expression
(isLoadingConfig || isFetching) && isFetchingsimplifies to justisFetching. If the intent is to detect retries specifically, consider using thefailureCountfrom useQuery instead.return { // ... isLoading: isLoadingConfig || isFetching, - isRetrying: (isLoadingConfig || isFetching) && isFetching, + isRetrying: isFetching && !isLoadingConfig, // fetching but not initial load };Alternatively, extract
failureCountfromuseQueryto accurately detect retry attempts.packages/typegen/web/src/components/ConfigSummary.tsx (2)
4-19: Unnecessary optional chaining on required prop.
configis a required prop, so the optional chaining inconfig?.typeon line 18 is unnecessary. Consider usingconfig.typedirectly for consistency.- {config?.type.toUpperCase()} + {config.type.toUpperCase()}
30-32: Consider using a single export pattern.The component has both a named export and a default export. Consider using only one pattern for consistency across the codebase.
packages/typegen/src/typegen.ts (1)
201-202: Redundant type check.The condition
config?.type === "fmdapi"is always true here because the function parameter is already typed asExtract<z.infer<typeof typegenConfigSingle>, { type: "fmdapi" }>. This check can be simplified.webviewerScriptName: - config?.type === "fmdapi" ? config.webviewerScriptName : undefined, + config.webviewerScriptName,packages/typegen/web/src/components/ConfigList.tsx (1)
3-4: Consider using a typed interface instead ofunknown[].The
configs: unknown[]type forces unsafe type assertions ingetConfigLabel. Since this component is part of the config management UI, consider importing and usingSingleConfigfromconfig-utils.tsfor better type safety.+import { SingleConfig } from "../lib/config-utils"; + interface ConfigListProps { - configs: unknown[]; + configs: SingleConfig[]; onSelectConfig: (index: number) => void; onAddConfig: () => void; }packages/typegen/web/src/App.tsx (2)
55-55: Remove console.log before merging.Debug logging should be removed from production code.
useEffect(() => { - console.log("configData from useEffect", configDataResponse); if (configDataResponse) {
88-89: Remove console.error or use proper error handling.Consider using a toast notification or UI feedback for save failures instead of just logging to console, consistent with how success feedback might be shown elsewhere in the app.
packages/typegen/web/src/components/dialog/default.tsx (1)
28-30: Move schema definition outside the component.
FormSchemais recreated on every render. Moving it outside the component avoids unnecessary object recreation and improves performance.+const FormSchema = z.object({ + feedback: z.string().min(1, 'Feedback is required').max(200, 'Feedback cannot exceed 200 characters'), +}); + export default function DialogDemo() { const [open, setOpen] = useState(false); const direction = useDirection(); - const FormSchema = z.object({ - feedback: z.string().min(1, 'Feedback is required').max(200, 'Feedback cannot exceed 200 characters'), - }); - const form = useForm<z.infer<typeof FormSchema>>({packages/typegen/web/src/components/EnvVarField.tsx (1)
46-61: Consider extracting debounce logic into a reusable hook.The manual timer management works correctly, but this pattern could be extracted into a
useDebouncedValuehook for reuse across components. This is a minor refactor suggestion for maintainability.// Example: useDebouncedValue.ts function useDebouncedValue<T>(value: T, delay: number): T { const [debouncedValue, setDebouncedValue] = useState(value); useEffect(() => { const timer = setTimeout(() => setDebouncedValue(value), delay); return () => clearTimeout(timer); }, [value, delay]); return debouncedValue; }packages/typegen/web/src/components/useEnvVarIndicator.ts (2)
6-7: Consider using a proper type instead ofany[]for better type safety.Using
any[]loses the benefits of TypeScript type checking. Consider importing and usingSingleConfigor the appropriate form type to maintain type safety throughout the hook.+import type { SingleConfig } from "../lib/config-utils"; + export function useEnvVarIndicator(index: number) { - const { control } = useFormContext<{ config: any[] }>(); + const { control } = useFormContext<{ config: SingleConfig[] }>();
57-70: ThehasCustomValueslogic duplicates the auth checking from lines 31-47.Consider simplifying by reusing the already-computed
authEnvName:const hasCustomValues = (envNamesServer && envNamesServer.trim() !== "") || - (envNamesDb && envNamesDb.trim() !== "") || - (envNamesAuth && - typeof envNamesAuth === "object" && - (("apiKey" in envNamesAuth && - envNamesAuth.apiKey && - envNamesAuth.apiKey.trim() !== "") || - ("username" in envNamesAuth && - envNamesAuth.username && - envNamesAuth.username.trim() !== "") || - ("password" in envNamesAuth && - envNamesAuth.password && - envNamesAuth.password.trim() !== ""))); + (envNamesDb && envNamesDb.trim() !== "") || + !!authEnvName;packages/typegen/web/src/components/EnvVarDialog.tsx (2)
30-32: Consider using a proper type instead ofany[].Same as in
useEnvVarIndicator, usingany[]loses type safety. Import and useSingleConfigtype for consistency.+import type { SingleConfig } from "../lib/config-utils"; + const { control, setValue, getValues } = useFormContext<{ - config: any[]; + config: SingleConfig[]; }>();
92-122: RemoveauthTypeSelectorfrom the dependency array to avoid potential update loops.Including
authTypeSelectorin dependencies while also callingsetAuthTypeSelectorinside the effect is a code smell. The guard at line 119 prevents infinite loops, but the effect should only run whenenvNamesAuthchanges.- }, [envNamesAuth, setValue, getValues, index, authTypeSelector]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [envNamesAuth, setValue, getValues, index]);Alternatively, use a ref to track the previous auth type if you need to compare values without triggering re-runs.
packages/typegen/src/server/index.ts (1)
88-96: Simplify return statement: remove unnecessaryPromise.resolve.The function is already
async, so returning an object directly will automatically wrap it in a resolved promise.- // The serve function from @hono/node-server already starts listening - // We just need to return the server with a close method - return Promise.resolve({ + return { port: actualPort, close: () => { server.close(); }, - }); + };packages/typegen/src/server/api.ts (2)
89-97: Consider returning 400 for malformed JSON input.If
JSON.parse(body)throws, the outer catch returns a 500 error. For client-side JSON syntax errors, a 400 (Bad Request) would be more appropriate.try { const body = await readRequestBody(req); - const data = JSON.parse(body); + let data; + try { + data = JSON.parse(body); + } catch { + return { + status: 400, + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + success: false, + error: "Invalid JSON in request body", + }), + }; + }
140-153: Consider adding a body size limit to prevent memory exhaustion.The
readRequestBodyfunction accumulates all chunks without a size limit. For a local dev tool this is low-risk, but adding a reasonable limit (e.g., 1MB) would be safer.+const MAX_BODY_SIZE = 1024 * 1024; // 1MB function readRequestBody(req: IncomingMessage): Promise<string> { return new Promise((resolve, reject) => { let body = ""; req.on("data", (chunk) => { body += chunk.toString(); + if (body.length > MAX_BODY_SIZE) { + req.destroy(); + reject(new Error("Request body too large")); + } });packages/typegen/web/src/components/ui/tooltip.tsx (1)
7-17: Note: EachTooltipcreates its ownTooltipProvider.This pattern means each tooltip instance has its own provider context, which isolates
delayDurationand other settings per-tooltip rather than sharing app-wide defaults. This is a valid design choice for component isolation but differs from the typical pattern of a single app-level provider.Also, the
data-slotattribute onTooltipProvider(line 8) won't appear in the DOM since Provider doesn't render a DOM element.Consider removing the unused
data-slotfrom the Provider:function TooltipProvider({ delayDuration = 0, ...props }: React.ComponentProps<typeof TooltipPrimitive.Provider>) { - return <TooltipPrimitive.Provider data-slot="tooltip-provider" delayDuration={delayDuration} {...props} />; + return <TooltipPrimitive.Provider delayDuration={delayDuration} {...props} />; }packages/typegen/web/src/hooks/useTestConnection.ts (1)
82-100: Consider typing the error attachment more explicitly.Using
(error as any).detailsis a common pattern but loses type safety. Consider creating a typed error class or using a discriminated union for the result.// Alternative: Create a typed error class class TestConnectionError extends Error { constructor(message: string, public details: TestConnectionResult) { super(message); } }Also, the type assertion on line 99 (
result as TestConnectionResult) relies on the server response matching the interface. The hc client should already provide proper typing based on the API contract.packages/typegen/src/server/app.ts (2)
246-256: Type assertion on client.layouts() due to inference issues.The comment explains the type assertion is needed because "TypeScript has inference issues with DataApi return type." While this works, consider adding a more specific type guard or improving the DataApi types upstream to avoid
as anycasts.
100-184: Redundant Zod schema definitions in POST /api/config response handling.The response schema is defined inline multiple times (lines 121-138, 149-162, 165-177). Consider extracting this to a reusable constant.
+const configResponseSchema = z.object({ + success: z.boolean(), + error: z.string().optional(), + issues: z + .array( + z.object({ + path: z.array(z.union([z.string(), z.number()])), + message: z.string(), + }), + ) + .optional(), +}); // Then use it: -const response = z.object({ ... }).parse({ success: false, ... }); +const response = configResponseSchema.parse({ success: false, ... });packages/typegen/web/src/components/ui/badge.tsx (2)
186-209: Badge component type mismatch with BadgeProps interface.The
Badgefunction signature (lines 195-196) defines its own props type inline rather than using the definedBadgePropsinterface. This causes thedotClassNameproperty fromBadgePropsto be unused.function Badge({ className, variant, size, appearance, shape, asChild = false, disabled, ...props -}: React.ComponentProps<"span"> & - VariantProps<typeof badgeVariants> & { asChild?: boolean }) { +}: Omit<BadgeProps, 'dotClassName'>) {
211-227: BadgeButton uses span with role="button" instead of actual button element.The component renders as
spanwithrole="button"but doesn't include keyboard event handlers (onKeyDownfor Enter/Space). For full accessibility, interactive elements withrole="button"should be keyboard accessible.Consider using an actual
buttonelement whenasChildis false, or add keyboard handlers:- const Comp = asChild ? SlotPrimitive.Slot : "span"; + const Comp = asChild ? SlotPrimitive.Slot : "button"; return ( <Comp data-slot="badge-button" className={cn(badgeButtonVariants({ variant, className }))} - role="button" + type="button" {...props} /> );packages/typegen/web/src/components/ui/switch.tsx (1)
56-84: Size variant inconsistency between switch and thumb.
switchThumbVariantsdefinesxssize (line 65), butswitchVariants(lines 36-41) only definessm,md,lg,xl. Ifsize="xs"is passed, the thumb will apply the compound variant but the switch itself won't have corresponding dimensions.Consider either adding
xstoswitchVariantsor removing it fromswitchThumbVariantsto maintain consistency.packages/typegen/src/server/contract.ts (1)
1-7: Remove unused import.
typegenConfigis imported on line 2 but never used in this file.-import { typegenConfigSingle, typegenConfig } from "../types"; +import { typegenConfigSingle } from "../types";packages/typegen/web/src/components/ui/form.tsx (2)
101-116: Type mismatch between props and rendered element.The props are typed as
React.HTMLAttributes<HTMLParagraphElement>but the component renders a<div>. Consider aligning the type with the rendered element.-function FormDescription({ className, ...props }: React.HTMLAttributes<HTMLParagraphElement>) { +function FormDescription({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) {
118-136: Type mismatch between props and rendered element.Same issue as
FormDescription- props typed asHTMLParagraphElementbut renders a<div>.-function FormMessage({ className, children, ...props }: React.HTMLAttributes<HTMLParagraphElement>) { +function FormMessage({ className, children, ...props }: React.HTMLAttributes<HTMLDivElement>) {packages/typegen/src/types.ts (1)
84-88: Consider the order of.default()and.optional().The combination
.default("schema").optional()may not behave as expected. With.default()first, undefined inputs become"schema", so the.optional()mainly affects type inference. If the intent is for the field to be optional with a default when omitted, the current order works but may confuse readers.If the field should always have a value (defaulting to
"schema"), consider removing.optional():const path = z .string() .default("schema") - .optional() .meta({ description: "The folder path to output the generated files" });Alternatively, if
undefinedshould be a valid explicit value,.optional().default("schema")would only apply the default when the key is missing, not when explicitlyundefined.
| type?: "text" | "password"; | ||
| } | ||
|
|
||
| export function EnvVarField({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix naming inconsistency.
The file is named ServerEnvField.tsx but exports EnvVarField. This inconsistency can cause confusion when importing or searching for the component.
Either rename the file to match the export:
git mv packages/typegen/web/src/components/ServerEnvField.tsx packages/typegen/web/src/components/EnvVarField.tsxOr update the export to match the filename:
-export function EnvVarField({
+export function ServerEnvField({🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ServerEnvField.tsx around line 25 the
component is exported as EnvVarField which mismatches the filename
ServerEnvField.tsx; fix by making names consistent — either rename the file to
EnvVarField.tsx (git mv ...) so the filename matches the exported component, or
change the exported component name to ServerEnvField to match the file and
update any imports accordingly.
| @theme inline { | ||
| --radius-sm: calc(var(--radius) - 4px); | ||
| --radius-md: calc(var(--radius) - 2px); | ||
| --radius-lg: var(--radius); | ||
| --radius-xl: calc(var(--radius) + 4px); | ||
| --radius-2xl: calc(var(--radius) + 8px); | ||
| --radius-3xl: calc(var(--radius) + 12px); | ||
| --radius-4xl: calc(var(--radius) + 16px); | ||
| --color-background: var(--background); | ||
| --color-foreground: var(--foreground); | ||
| --color-card: var(--card); | ||
| --color-card-foreground: var(--card-foreground); | ||
| --color-popover: var(--popover); | ||
| --color-popover-foreground: var(--popover-foreground); | ||
| --color-primary: var(--primary); | ||
| --color-primary-foreground: var(--primary-foreground); | ||
| --color-secondary: var(--secondary); | ||
| --color-secondary-foreground: var(--secondary-foreground); | ||
| --color-muted: var(--muted); | ||
| --color-muted-foreground: var(--muted-foreground); | ||
| --color-accent: var(--accent); | ||
| --color-accent-foreground: var(--accent-foreground); | ||
| --color-destructive: var(--destructive); | ||
| --color-border: var(--border); | ||
| --color-input: var(--input); | ||
| --color-ring: var(--ring); | ||
| --color-chart-1: var(--chart-1); | ||
| --color-chart-2: var(--chart-2); | ||
| --color-chart-3: var(--chart-3); | ||
| --color-chart-4: var(--chart-4); | ||
| --color-chart-5: var(--chart-5); | ||
| --color-sidebar: var(--sidebar); | ||
| --color-sidebar-foreground: var(--sidebar-foreground); | ||
| --color-sidebar-primary: var(--sidebar-primary); | ||
| --color-sidebar-primary-foreground: var(--sidebar-primary-foreground); | ||
| --color-sidebar-accent: var(--sidebar-accent); | ||
| --color-sidebar-accent-foreground: var(--sidebar-accent-foreground); | ||
| --color-sidebar-border: var(--sidebar-border); | ||
| --color-sidebar-ring: var(--sidebar-ring); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent CSS custom property naming between theme definition and usage.
The @theme inline block defines tokens with the --color- prefix (e.g., --color-background, --color-primary), but the :root and .dark sections use unprefixed names (e.g., --background, --primary). This mismatch means the theme tokens reference variables that don't exist at parse time. Either:
- Rename the variables in
@themeto match the unprefixed names used in:rootand.dark, or - Update
:rootand.darkto use the--color-prefixed names.
Option 1 is recommended for consistency with standard CSS naming conventions.
@theme inline {
- --color-background: var(--background);
+ --background: var(--background);
- --color-foreground: var(--foreground);
+ --foreground: var(--foreground);
// ... and so on for all --color-* properties
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/typegen/web/src/index.css around lines 6-45, the @theme inline block
defines variables with a --color- prefix while :root and .dark use unprefixed
names (e.g., --background, --primary), causing mismatched references; fix by
renaming the tokens in the @theme block to the unprefixed names used elsewhere
(replace --color-background → --background, --color-primary → --primary, etc.)
so all variable names match, or alternatively update :root/.dark to use the
--color- prefixed names—prefer renaming the @theme variables to the unprefixed
form for consistency.
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/registry/tsdown.config.ts (1)
21-41: Consider logging a warning if expected declaration files are not found.The code silently continues if
dtsFileordtsMapFileare not found. Sincedts: trueis explicitly enabled, the absence of these files might indicate a build problem. Adding a warning message would help diagnose issues.Apply this diff to add warning logs:
if (dtsFile) { fs.renameSync( path.join(distLib, dtsFile), path.join(distLib, "index.d.ts"), ); console.log(`✅ Renamed ${dtsFile} to index.d.ts`); + } else { + console.warn("⚠️ No hashed .d.ts file found to rename"); } if (dtsMapFile) { fs.renameSync( path.join(distLib, dtsMapFile), path.join(distLib, "index.d.ts.map"), ); console.log(`✅ Renamed ${dtsMapFile} to index.d.ts.map`); + } else { + console.warn("⚠️ No hashed .d.ts.map file found to rename"); }packages/registry/package.json (2)
27-29: Build/dev script pipeline looks coherent; consider dev-ergonomics later
prebuild→build→devwiring is consistent, and running validation beforetsdown && publint --strictmakes sense. Thedevscript does a full build and then re-runstsdown+tsc --emitDeclarationOnlyon every change, which is correct but may be heavier than necessary on large templates; if it becomes a pain, consider switching to dedicated--watchflows instead of rebuilding from scratch each time.
55-55: Zod v4 bump: verify continued use ofzod/v3and consider consolidating on v4 APIYou’ve upgraded this package to
zod@^4.2.0whilepackages/registry/lib/types.tsstill imports from"zod/v3". If that subpath is intentionally used as a v3‑compat entrypoint with Zod 4, that’s fine, but it’s worth double‑checking that:
"zod/v3"is still a supported and stable path in the version you’re targeting.- You’re not unintentionally mixing v3 semantics with v4 expectations elsewhere.
Longer term, you may want to migrate the registry types to the main Zod 4 API (
import { z } from "zod") to avoid relying on compatibility shims.packages/registry/scripts/validate.ts (1)
1-26: Prebuild validator wiring is solid; jiti usage is reasonableThe script cleanly resolves the validator path relative to itself, uses jiti to execute the TS source before build, and exits non‑zero on failure, which is exactly what you want for a
prebuildgate. You might eventually simplify this by importingvalidateRegistrydirectly (lettingtsxhandle TS) instead of layering jiti, but the current approach is consistent and explicit.packages/registry/lib/types.ts (1)
192-205:RegistryIndexschema and type are consistent; consider deriving the type from the schemaThe
RegistryIndexstructural type matches whatregistryIndexSchemaproduces ({ name, title, category, description? }[]derived fromtemplateMetadataSchema), which is good for keeping the index view in sync with template metadata.To reduce duplication and avoid future drift, you could instead define the type from the schema:
export const registryIndexSchema = templateMetadataSchema .pick({ title: true, category: true, description: true }) .extend({ name: z.string() }) .array(); export type RegistryIndex = z.infer<typeof registryIndexSchema>;Not required, but it keeps the contract DRY and guarantees the TS type always matches the Zod schema.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/registry/lib/types.ts(2 hunks)packages/registry/lib/validator.ts(1 hunks)packages/registry/package.json(2 hunks)packages/registry/scripts/validate.ts(1 hunks)packages/registry/tsconfig.json(1 hunks)packages/registry/tsdown.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/registry/tsdown.config.ts (1)
apps/docs/scripts/bundle-registry-templates.js (2)
fs(3-3)path(4-4)
packages/registry/scripts/validate.ts (1)
packages/registry/lib/validator.ts (1)
validateRegistry(107-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
packages/registry/tsconfig.json (1)
23-24: Verify test file exclusion pattern covers all test files in lib.The exclude pattern
*.test.tsonly matches test files at the root level. If test files exist within lib (e.g.,lib/utils.test.ts), they won't be excluded and will be included by theinclude: ["lib/**/*"]directive.Confirm whether:
- Test files should be excluded from type-checking in this package
- Test files exist within
lib/and, if so, update the pattern to**/*.test.tsorlib/**/*.test.tsIf test files are intentionally included, this comment can be disregarded.
packages/registry/tsdown.config.ts (2)
15-16: LGTM!Dynamic imports for Node.js built-in modules are appropriate in this async context.
8-8: The code correctly implements the expected behavior—tsdown 0.14.2 does append content hashes to declaration filenames by default (via the rolldown-plugin-dts plugin). The renaming logic to convertindex-<hash>.d.tstoindex.d.tsis necessary and properly handles the hashed output.packages/registry/package.json (1)
46-50: Tooling version bumps look fine; align with repo-wide constraintsThe publint/tsdown/tsx/TypeScript/vitest bumps look reasonable and consistent with a modern toolchain. Just make sure these versions match any central constraints (root
package.json/pnpm workspace) so you don’t end up with divergent tooling behavior between packages.packages/registry/lib/validator.ts (1)
5-5:./types.jsimport: confirm jiti can resolve this before buildSwitching the import to
./types.jsis correct for the compiled ESM output indist. However,scripts/validate.tsnow loadslib/validator.tsvia jiti before the build runs, when onlytypes.tsexists. Please confirm that jiti’s resolver will successfully handle an import of./types.jsfrom a TS source file (i.e., that it will map this to the TS file rather than requiring an actualtypes.jsnext to it). If not, you may want to revert to an extensionless import ("./types") or adjust the prebuild script to point at the built output instead.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/typegen/web/src/components/ConfigEditor.tsx (1)
150-189: Consider wiring the label to the actual switch element for better a11yRight now
FormLabelends up targeting theFormControl’s first child (the wrapping<div>viaformItemId), while the switch itself has a separateid={generateClientSwitchId}. For assistive tech it’s usually preferable for the label’shtmlForto reference the focusable control.One option is to make the
Switchthe direct child ofFormControl(or anotherSlot), so theformItemIdends up on the switch instead of an intermediate<div>.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/typegen/package.json(3 hunks)packages/typegen/web/src/App.tsx(1 hunks)packages/typegen/web/src/components/ConfigEditor.tsx(1 hunks)packages/typegen/web/src/components/ui/form.tsx(1 hunks)packages/typegen/web/src/lib/api.ts(1 hunks)packages/typegen/web/src/main.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/typegen/web/src/App.tsx
- packages/typegen/web/src/main.tsx
- packages/typegen/web/src/lib/api.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/typegen/web/src/components/ConfigEditor.tsx (8)
packages/typegen/web/src/lib/config-utils.ts (1)
SingleConfig(4-4)packages/typegen/web/src/components/EnvVarDialog.tsx (1)
EnvVarDialog(29-417)packages/typegen/web/src/components/ui/form.tsx (5)
FormField(184-184)FormItem(185-185)FormLabel(186-186)FormControl(182-182)FormMessage(187-187)packages/typegen/web/src/components/InfoTooltip.tsx (1)
InfoTooltip(4-15)packages/typegen/web/src/components/ui/input.tsx (1)
Input(163-163)packages/typegen/web/src/components/ui/select.tsx (5)
Select(224-224)SelectTrigger(233-233)SelectValue(234-234)SelectContent(225-225)SelectItem(228-228)packages/typegen/web/src/components/ui/switch.tsx (3)
SwitchWrapper(191-191)Switch(191-191)SwitchIndicator(191-191)packages/typegen/web/src/components/LayoutEditor.tsx (1)
LayoutEditor(12-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (8)
packages/typegen/web/src/components/ConfigEditor.tsx (1)
36-38: Hook placement issue resolved; id now computed at component top levelMoving
useId()to the top level (baseId/generateClientSwitchId) fixes the earlier Rules of Hooks violation in theGenerateswitch. The switch is now getting a stable id and there’s no hook call inside theFormFieldrender callback.Also applies to: 150-189
packages/typegen/web/src/components/ui/form.tsx (1)
74-96: Previous circular dependency issue betweenFormItemanduseFormFieldresolved
FormItemnow deriveserrordirectly fromuseFormContextandFormFieldContextinstead of callinguseFormField, so it no longer depends onFormItemContextbeing established beforeuseFormFieldruns. That removes the earlier circular context issue noted in past reviews.packages/typegen/package.json (6)
31-42: Exports look correct.The
./apiand./api-appexports are properly differentiated, with each pointing to the appropriate module. This addresses the previous review concern about duplicate export paths.
9-10: The--no-openflag is properly implemented in the CLI atpackages/typegen/src/cli.ts. The flag is defined via commander.js and correctly suppresses automatic browser opening when set (if (options.open !== false)). No changes needed.
12-12: No issues found. Thescripts/build-copy.jsfile exists, is tracked in version control, and is correctly implemented. It safely copies built web assets fromweb/disttodist/webwith proper existence checks and error handling.
84-84: No action needed. The codebase already imports explicitly from"zod/v4"(packages/typegen/src/types.ts line 1) and uses modern Zod v4 patterns throughout (z.union, z.discriminatedUnion, z.preprocess, .default(), etc.). Zod v4 compatibility has already been addressed during implementation.
93-94: Vitest v4 upgrade is properly implemented and compatible.The upgrade from Vitest 3.2.4 to 4.0.7 has been correctly applied:
vitest.config.tsuses the correct v4 API withdefineConfigfrom "vitest/config"- Test file imports and assertions (including
toMatchFileSnapshot()) are v4-compatible- Setup file for environment variables is compatible
- No deprecated config options (poolMatchGlobs, environmentMatchGlobs, minWorkers, etc.) are present
- No breaking change patterns detected (mock names, restoreAllMocks behavior, custom reporters/environments)
No action required.
68-69: Hono packages and related dependencies are compatible with each other and the project's Node.js >= 18 requirement:
- @hono/node-server 1.19.7 is the official Node.js adapter for Hono v4.9.0 ✓
- @hono/zod-validator 0.7.5 requires Zod (already present at v4.1.13) and runs on Node.js ✓
- nodemon 3.1.11 and concurrently 8.2.2 both support Node.js 18+ ✓
- open 10.1.0 is native ESM compatible with the project's "type": "module" configuration ✓
No version conflicts detected.
| import { Label } from "@/components/ui/label"; | ||
| import { Slot } from "@radix-ui/react-slot"; | ||
| import { Label as LabelPrimitive } from "radix-ui"; | ||
| import { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file to see the imports and usage
cat -n packages/typegen/web/src/components/ui/form.tsx | head -120Repository: proofgeist/proofkit
Length of output: 3845
🌐 Web query:
Radix UI react-label import pattern official documentation
💡 Result:
Official Radix docs show two equivalent import patterns:
-
Named export via the Radix meta package:
import { Label } from "radix-ui"; then use <Label.Root />. [1] -
Direct package (common in examples):
import * as Label from "@radix-ui/react-label"; then use <Label.Root />. [2]
Sources: Radix Label docs and package page. [1][2]
References
[1] Radix Primitives — Label docs. https://www.radix-ui.com/primitives/docs/components/label
[2] @radix-ui/react-label (npm / package.json). https://www.npmjs.com/package/%40radix-ui/react-label
Fix type signature mismatch in FormLabel
The import pattern import { Label as LabelPrimitive } from "radix-ui" is valid per official Radix UI documentation (equivalent to import * as LabelPrimitive from "@radix-ui/react-label"). However, there's a type/implementation mismatch: the function signature declares props as React.ComponentProps<typeof LabelPrimitive.Root> but actually renders the custom <Label> component (from @/components/ui/label), not LabelPrimitive.Root.
Align the type signature with the actual implementation—either use LabelPrimitive.Root for rendering, or adjust the prop type to match the custom Label component's accepted props.
Also applies to: 98-113
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/form.tsx around lines 5-8 and 98-113,
the FormLabel component types are declared as React.ComponentProps<typeof
LabelPrimitive.Root> while the implementation actually renders the custom Label
imported from "@/components/ui/label"; update the type signature to match the
rendered component by replacing React.ComponentProps<typeof LabelPrimitive.Root>
with React.ComponentProps<typeof Label> (or alternatively change the rendered
element to LabelPrimitive.Root if you intend to use Radix's primitive), and
ensure any forwarded props and ref types align with the chosen component.
| const FormFieldContext = React.createContext<FormFieldContextValue>( | ||
| {} as FormFieldContextValue, | ||
| ); |
There was a problem hiding this comment.
Guard useFormField against missing context; avoid using context values before validating
useFormField currently:
- Creates
FormFieldContextandFormItemContextwith{} as ...defaults. - Immediately calls
getFieldState(fieldContext.name, formState)and later checksif (!fieldContext), which will never trigger because{}is truthy. - Unconditionally destructures
const { id } = itemContext, so when used outside aFormItemyou’ll getid === undefinedandundefined-form-itemIDs.
This combination can lead to confusing runtime behavior (or errors) when FormLabel/FormControl/FormMessage are ever rendered outside the expected <FormField><FormItem>... nesting.
Consider tightening this:
-const FormFieldContext = React.createContext<FormFieldContextValue>(
- {} as FormFieldContextValue,
-);
+const FormFieldContext = React.createContext<FormFieldContextValue | undefined>(
+ undefined,
+);
+const FormItemContext = React.createContext<FormItemContextValue | undefined>(
+ undefined,
+);
const useFormField = () => {
const fieldContext = React.useContext(FormFieldContext);
const itemContext = React.useContext(FormItemContext);
const { getFieldState, formState } = useFormContext();
- const fieldState = getFieldState(fieldContext.name, formState);
-
- if (!fieldContext) {
- throw new Error("useFormField should be used within <FormField>");
- }
-
- const { id } = itemContext;
+ if (!fieldContext) {
+ throw new Error("useFormField should be used within <FormField>");
+ }
+ if (!itemContext) {
+ throw new Error("useFormField should be used within <FormItem>");
+ }
+
+ const fieldState = getFieldState(fieldContext.name, formState);
+ const { id } = itemContext;
return {
id,
name: fieldContext.name,
formItemId: `${id}-form-item`,
formDescriptionId: `${id}-form-item-description`,
formMessageId: `${id}-form-item-message`,
...fieldState,
};
};This makes mis‑nesting fail fast with clear errors rather than undefined names/IDs leaking through.
Also applies to: 43-63, 66-72
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/form.tsx around lines 26-28 (and
similarly for the contexts/usages at lines 43-63 and 66-72): the code creates
FormFieldContext/FormItemContext with "{} as ..." defaults and then uses their
properties before verifying they exist, causing silent incorrect values or
undefined IDs when components are mis-nested. Change the contexts to be created
with undefined defaults (createContext<...|undefined>(undefined)); in
useFormField first read the contexts and immediately throw a clear Error if
fieldContext or itemContext are missing (e.g. "useFormField must be used within
<FormField>") before calling getFieldState or destructuring itemContext.id; only
call getFieldState and destructure id after the existence checks so mis-nesting
fails fast with a helpful error.
eluce2
force-pushed
the
12-13-typegen_web_ui
branch
from
5d7353a to
940dd98
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/typegen/src/cli.ts (1)
99-136: NormalizeconfigParsed.data.configbefore passing it intogenerateTypedClients.With
typegenConfignow allowing either a single config or an array:export const typegenConfig = z.object({ config: z.union([z.array(typegenConfigSingle), typegenConfigSingle]), });
configParsed.data.configcan beSingleConfig | SingleConfig[], butrunCodegenstill passes it straight togenerateTypedClients:const result = await generateTypedClients(configParsed.data.config, { resetOverrides, });To avoid surprises (and keep
generateTypedClients’s signature simple), consider normalizing here:- const result = await generateTypedClients(configParsed.data.config, { + const configs = Array.isArray(configParsed.data.config) + ? configParsed.data.config + : [configParsed.data.config]; + const result = await generateTypedClients(configs, { resetOverrides, }).catch((err: unknown) => {or explicitly update
generateTypedClients’s API to accept the union and handle normalization internally.
♻️ Duplicate comments (6)
packages/registry/lib/types.ts (1)
150-160: Re-alignTemplateMetadatawithtemplateMetadataSchemato avoid unsound typing
TemplateMetadataextendsz.infer<typeof registryItemSchema>(bringing in fields likename,type,docs, and the originalfilestype), buttemplateMetadataSchemaomitsname,type,files, anddocsand only re-addsfiles. Annotating the schema asz.ZodType<TemplateMetadata>meanssafeParseclaims to return aTemplateMetadatathat includes those omitted fields, even though they’re not validated or produced, and thefilestype is an intersection of two potentially different shapes. This is the same mismatch a prior review already flagged.To keep the runtime shape and TypeScript type in sync, consider deriving the type from the schema instead of asserting it, e.g.:
-export type TemplateMetadata = z.infer<typeof registryItemSchema> & { - title: string; - description?: string; - category: z.infer<typeof categorySchema>; - files: TemplateFile[]; - registryType: z.infer<typeof registryTypeSchema>; - postInstall?: PostInstallStep[]; - minimumProofKitVersion?: string; - allowedFrameworks?: z.infer<typeof frameworkSchema>[]; - schemaRequired?: boolean; -}; - -// Defines the metadata for a single template (_meta.ts) -export const templateMetadataSchema: z.ZodType<TemplateMetadata> = registryItemSchema - .omit({ name: true, type: true, files: true, docs: true }) - .extend({ +// Defines the metadata for a single template (_meta.ts) +export const templateMetadataSchema = registryItemSchema + .omit({ name: true, type: true, files: true, docs: true }) + .extend({ title: z.string(), description: z.string().optional(), category: categorySchema, files: z.array(templateFileSchema), registryType: registryTypeSchema, postInstall: z .array(postInstallStepsSchema) .optional() .describe( "Steps that should be run by the ProofKit CLI after shadcn CLI is done", ), minimumProofKitVersion: z .string() .describe("The minimum version of ProofKit required to use this template") .optional(), allowedFrameworks: z.array(frameworkSchema).optional(), schemaRequired: z .boolean() .optional() .describe( "Whether this template requires a database schema to be selected", ), }); + +export type TemplateMetadata = z.infer<typeof templateMetadataSchema>;If you do need a richer “resolved” template type that includes
name/docs, it may be cleaner to introduce a separate type for that rather than asserting it on the_meta.tsschema.Also applies to: 163-188
packages/typegen/src/server/index.ts (1)
32-55: Block directory traversal when serving static assets
pathnameis joined directly withWEB_DIRand then read from disk. A request like/../../../../../etc/passwdcould escape the intended web root and serve arbitrary files readable by the process. This was already flagged in an earlier review and remains unresolved.Apply a resolved-path guard and use the resolved path for I/O, for example:
- const pathname = - url.pathname === "/" ? "index.html" : url.pathname.slice(1); - const filePath = join(WEB_DIR, pathname); + const pathname = + url.pathname === "/" ? "index.html" : url.pathname.slice(1); + const unsafePath = join(WEB_DIR, pathname); + const webRoot = resolve(WEB_DIR); + const filePath = resolve(unsafePath); + + // Prevent path traversal outside WEB_DIR + if ( + !filePath.startsWith(webRoot + require("path").sep) && + filePath !== webRoot + ) { + return c.text("Forbidden", 403); + }Then keep using
filePathforexistsSync/readFileSync.packages/typegen/src/server/app.ts (2)
186-202: POST/api/runstill doesn’t send a response after running typegenThe
/runhandler awaitsgenerateTypedClients(config)and then callsawait next()but never returns anything. If there’s no downstream middleware to handle this route, clients will see a 404/empty response after a potentially long-running operation. This was previously flagged and remains unresolved.Consider making the handler self-contained and returning a simple JSON status (optionally including summary counts from
generateTypedClients):- .post( - "/run", - zValidator( - "json", - z.object({ - config: z.union([z.array(typegenConfigSingle), typegenConfigSingle]), - }), - ), - async (c, next) => { - const data = c.req.valid("json"); - const config = data.config; - - await generateTypedClients(config); - await next(); - }, - ) + .post( + "/run", + zValidator( + "json", + z.object({ + config: z.union([z.array(typegenConfigSingle), typegenConfigSingle]), + }), + ), + async (c) => { + const { config } = c.req.valid("json"); + const summary = await generateTypedClients(config); + + return c.json( + summary ?? { + successCount: 0, + errorCount: 0, + totalCount: 0, + }, + ); + }, + )Adjust the response shape to match whatever contract you’ve defined for the run endpoint.
298-308: Limit/api/env-namesto an allowlist of safe keysThis endpoint currently exposes arbitrary
process.enventries based solely on the user-controlledenvNamequery param, which can leak secrets (API keys, tokens, DB credentials, etc.) if the server is reachable.To tighten this up, restrict lookups to a known allowlist and optionally reject unknown names:
.get( "/env-names", zValidator("query", z.object({ envName: z.string() })), async (c) => { const input = c.req.valid("query"); - - const value = process.env[input.envName]; - - return c.json({ value }); + const allowedEnvNames = [ + "FM_SERVER", + "FM_DATABASE", + "OTTO_API_KEY", + "FM_USERNAME", + "FM_PASSWORD", + ]; + + if (!allowedEnvNames.includes(input.envName)) { + return c.json( + { error: "Environment variable not allowed" }, + 400 as ContentfulStatusCode, + ); + } + + const value = process.env[input.envName]; + return c.json({ value }); }, )Align the allowlist with the actual keys you support in
defaultEnvNamesand related config.packages/typegen/web/src/components/ui/form.tsx (2)
98-115: AlignFormLabelprop types with the renderedLabelcomponent
FormLabelis typed asReact.ComponentProps<typeof LabelPrimitive.Root>but actually renders the customLabelfrom"@/components/ui/label". That mismatch can let callers pass props accepted by the Radix primitive but not by your wrapper (or vice versa), andLabelPrimitiveis never used at runtime.Recommend typing against the component you render:
-import { Label } from "@/components/ui/label"; +import { Label } from "@/components/ui/label"; @@ function FormLabel({ className, ...props -}: React.ComponentProps<typeof LabelPrimitive.Root>) { +}: React.ComponentPropsWithoutRef<typeof Label>) {(or, alternatively, render
LabelPrimitive.Rootdirectly if that’s what you intend).
19-28: Guard form contexts and avoid using them before validating
FormFieldContext/FormItemContextdefault to{}anduseFormFieldcallsgetFieldState(fieldContext.name, formState)and destructuresitemContext.idbefore any null/undefined checks. Mis-nesting (or forgetting aFormItem/FormField) will quietly passundefinedintogetFieldStateand produceundefined-*IDs instead of failing fast.Consider tightening the contexts and guards like this:
-const FormFieldContext = React.createContext<FormFieldContextValue>( - {} as FormFieldContextValue, -); +const FormFieldContext = React.createContext<FormFieldContextValue | undefined>( + undefined, +); @@ -const useFormField = () => { - const fieldContext = React.useContext(FormFieldContext); - const itemContext = React.useContext(FormItemContext); - const { getFieldState, formState } = useFormContext(); - - const fieldState = getFieldState(fieldContext.name, formState); - - if (!fieldContext) { - throw new Error("useFormField should be used within <FormField>"); - } - - const { id } = itemContext; - - return { - id, - name: fieldContext.name, - formItemId: `${id}-form-item`, - formDescriptionId: `${id}-form-item-description`, - formMessageId: `${id}-form-item-message`, - ...fieldState, - }; -}; +const useFormField = () => { + const fieldContext = React.useContext(FormFieldContext); + const itemContext = React.useContext(FormItemContext); + const { getFieldState, formState } = useFormContext(); + + if (!fieldContext) { + throw new Error("useFormField must be used within <FormField>"); + } + if (!itemContext) { + throw new Error("useFormField must be used within <FormItem>"); + } + + const fieldState = getFieldState(fieldContext.name, formState); + const { id } = itemContext; + + return { + id, + name: fieldContext.name, + formItemId: `${id}-form-item`, + formDescriptionId: `${id}-form-item-description`, + formMessageId: `${id}-form-item-message`, + ...fieldState, + }; +}; @@ -type FormItemContextValue = { - id: string; -}; - -const FormItemContext = React.createContext<FormItemContextValue>( - {} as FormItemContextValue, -); +type FormItemContextValue = { + id: string; +}; + +const FormItemContext = React.createContext<FormItemContextValue | undefined>( + undefined, +); @@ function FormItem({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) { const id = React.useId(); const fieldContext = React.useContext(FormFieldContext); const { getFieldState, formState } = useFormContext(); - const fieldState = fieldContext.name - ? getFieldState(fieldContext.name, formState) - : null; + const fieldState = + fieldContext?.name != null + ? getFieldState(fieldContext.name, formState) + : null; const error = fieldState?.error;This makes mis-nesting fail loudly and avoids ever passing an undefined field name into
react-hook-form.Also applies to: 43-63, 66-72, 79-83
🧹 Nitpick comments (24)
packages/typegen/web/src/components/ui/badge.tsx (1)
172-184: Consider simplifyingbadgeButtonVariantsor adding meaningful variants.The
badgeButtonVariantsCVA configuration only includes a"default"variant with no conditional styling, which provides no value.Option 1 (if no variants are needed): Remove the variant system and apply the classes directly:
-const badgeButtonVariants = cva( - "cursor-pointer transition-all inline-flex items-center justify-center leading-none size-3.5 [&>svg]:opacity-100! [&>svg]:size-3.5! p-0 rounded-md -me-0.5 opacity-60 hover:opacity-100", - { - variants: { - variant: { - default: "", - }, - }, - defaultVariants: { - variant: "default", - }, - }, -); +const badgeButtonClasses = "cursor-pointer transition-all inline-flex items-center justify-center leading-none size-3.5 [&>svg]:opacity-100! [&>svg]:size-3.5! p-0 rounded-md -me-0.5 opacity-60 hover:opacity-100";Option 2 (if variants are planned): Add meaningful variants in a future iteration.
packages/typegen/web/src/components/ui/tree.tsx (1)
11-16: Consider improving type safety for thetreeproperty.The
treeproperty is currently typed asany, which removes type safety. If the exact type from@headless-tree/reactis available, consider using it. Otherwise,unknownwould be safer thanany.Example refactor:
interface TreeContextValue<T = any> { indent: number; currentItem?: ItemInstance<T>; - tree?: any; + tree?: unknown; // or import the proper type from @headless-tree/react toggleIconType?: ToggleIconType; }interface TreeProps extends React.HTMLAttributes<HTMLDivElement> { indent?: number; - tree?: any; + tree?: unknown; // or import the proper type from @headless-tree/react toggleIconType?: ToggleIconType; }Also applies to: 29-33
packages/registry/lib/types.ts (1)
192-204: DeriveRegistryIndexfromregistryIndexSchema(or a shared item schema) to prevent future drift
RegistryIndexmanually re-specifies the item shape whileregistryIndexSchemais annotated asz.ZodType<RegistryIndex>. They’re consistent today, but this repeats the pattern that caused theTemplateMetadatamismatch and could drift over time.Consider inverting the relationship so the type is inferred from the schema, e.g.:
const registryIndexItemSchema = templateMetadataSchema .pick({ title: true, category: true, description: true }) .extend({ name: z.string() }); export const registryIndexSchema = registryIndexItemSchema.array(); export type RegistryIndex = z.infer<typeof registryIndexSchema>;This keeps the runtime validation and TypeScript surface locked together with one source of truth.
packages/typegen/web/src/components/ui/input.tsx (4)
51-76:inputGroupVariantstyping could be simplified until you add real variantsRight now
inputGroupVariantsis defined withvariants: {}/defaultVariants: {}, soVariantProps<typeof inputGroupVariants>doesn’t add anything to theInputGroupprop type and may confuse readers into thinking there are configurable variants that don’t exist yet. You could simplify the signature to just the nativedivprops until you actually introduce group variants.
78-161: Confirm intent of composinginputVariantsintoInputWrapperas well asInput
InputWrapperappliesinputVariants({ variant })to the wrapper itself, while the childInputalso appliesinputVariants({ variant }), and the wrapper then overrides a bunch of child styles via[&_[data-slot=input]]:*utilities. This is a clever way to share size/border/radius tokens between standalone inputs and wrapped/grouped inputs, but it does mean wrapper and child share a lot of base styling with overrides relying on Tailwind’s ordering.If that’s intentional, it’s worth sanity‑checking a few combinations (standalone
<Input>,<InputWrapper><Input /></InputWrapper>, with addons/buttons/datefield) to ensure there are no unexpected double borders or rings in your actual Tailwind build.
127-143: Consider adding ref forwarding toInput/InputAddonSince these are low‑level primitives, callers will often want to attach refs (for focusing, form libraries, etc.). Right now they’re plain functions, so the ref has to go on a wrapper instead of the underlying DOM node. Converting these to
forwardRefwrappers would improve ergonomics without changing the public surface otherwise.
163-163: Export set feels slightly asymmetric vs. defined variantsYou export
inputVariantsandinputAddonVariantsbut keepinputGroupVariants/inputWrapperVariantsinternal. If consumers will ever need to style‑compose around groups/wrappers the way they can around raw inputs and addons, consider exporting those variant factories too; otherwise, this asymmetry is fine but might merit a short comment that group/wrapper variants are intentionally internal.packages/typegen/web/src/components/ui/select.tsx (2)
191-209: Avoid leaking RadixItemIndicatorprops onto the wrapper<span>
SelectIndicatoris typed asReact.ComponentProps<typeof SelectPrimitive.ItemIndicator>, but those props (e.g.forceMount,asChild) are currently spread onto the outer<span>, not the underlyingItemIndicator. That can both leak unknown attributes into the DOM and make Radix-specific props ineffective.You can keep the same public API but forward props to
ItemIndicatorinstead:-function SelectIndicator({ - children, - className, - ...props -}: React.ComponentProps<typeof SelectPrimitive.ItemIndicator>) { - const { indicatorPosition } = React.useContext(SelectContext); - - return ( - <span - data-slot="select-indicator" - className={cn( - 'absolute flex top-1/2 -translate-y-1/2 items-center justify-center', - indicatorPosition === 'left' ? 'start-2' : 'end-2', - className, - )} - {...props} - > - <SelectPrimitive.ItemIndicator>{children}</SelectPrimitive.ItemIndicator> - </span> - ); -} +function SelectIndicator({ + children, + className, + ...props +}: React.ComponentPropsWithoutRef<typeof SelectPrimitive.ItemIndicator>) { + const { indicatorPosition } = React.useContext(SelectContext); + + return ( + <span + data-slot="select-indicator" + className={cn( + 'absolute flex top-1/2 -translate-y-1/2 items-center justify-center', + indicatorPosition === 'left' ? 'start-2' : 'end-2', + )} + > + <SelectPrimitive.ItemIndicator className={className} {...props}> + {children} + </SelectPrimitive.ItemIndicator> + </span> + ); +}This way all Radix
ItemIndicatorprops behave as expected, and only semantic HTML attributes land on the<span>.
35-221: Consider forwarding refs for all Radix wrappersAll wrappers (
SelectGroup,SelectValue,SelectTrigger,SelectContent,SelectItem, etc.) are typed withReact.ComponentProps<typeof ...>, which exposes arefprop in TS, but since they’re plain function components, that ref can’t actually be used. To avoid surprising consumers (and to enable focus/measurement hooks), consider adoptingReact.forwardReffor these wrappers.For example, for
SelectTrigger:-export interface SelectTriggerProps - extends React.ComponentProps<typeof SelectPrimitive.Trigger>, - VariantProps<typeof selectTriggerVariants> {} - -function SelectTrigger({ className, children, size, ...props }: SelectTriggerProps) { - return ( - <SelectPrimitive.Trigger - data-slot="select-trigger" - className={cn(selectTriggerVariants({ size }), className)} - {...props} - > - {children} - <SelectPrimitive.Icon asChild> - <ChevronDown className="h-4 w-4 opacity-60 -me-0.5" /> - </SelectPrimitive.Icon> - </SelectPrimitive.Trigger> - ); -} +export interface SelectTriggerProps + extends React.ComponentPropsWithoutRef<typeof SelectPrimitive.Trigger>, + VariantProps<typeof selectTriggerVariants> {} + +const SelectTrigger = React.forwardRef< + React.ElementRef<typeof SelectPrimitive.Trigger>, + SelectTriggerProps +>(({ className, children, size, ...props }, ref) => ( + <SelectPrimitive.Trigger + ref={ref} + data-slot="select-trigger" + className={cn(selectTriggerVariants({ size }), className)} + {...props} + > + {children} + <SelectPrimitive.Icon asChild> + <ChevronDown className="h-4 w-4 opacity-60 -me-0.5" /> + </SelectPrimitive.Icon> + </SelectPrimitive.Trigger> +)); + +SelectTrigger.displayName = 'SelectTrigger';You can mirror this pattern for the other wrappers as needed.
packages/typegen/src/typegen.ts (1)
201-202: Redundant type check—config.typeis always"fmdapi"here.Since the function signature on line 60 constrains
configtoExtract<..., { type: "fmdapi" }>, the conditionconfig?.type === "fmdapi"is always true. This check is unnecessary but harmless—it may have been added for defensive coding or future flexibility.If the intent is purely defensive, consider simplifying:
- webviewerScriptName: - config?.type === "fmdapi" ? config.webviewerScriptName : undefined, + webviewerScriptName: config.webviewerScriptName,packages/typegen/web/src/components/ui/textarea.tsx (1)
8-14: Duplicatebg-backgroundclass in base styles.The base styles contain
bg-backgroundtwice on line 10, which is redundant.- w-full bg-background border border-input bg-background text-foreground shadow-xs shadow-black/5 transition-[color,box-shadow] + w-full bg-background border border-input text-foreground shadow-xs shadow-black/5 transition-[color,box-shadow]packages/typegen/web/src/components/dialog/default.tsx (3)
28-30: Consider movingFormSchemaoutside the component.Defining the schema inside the component causes it to be recreated on every render. While acceptable for a demo, moving it outside improves performance and follows common patterns.
+const FormSchema = z.object({ + feedback: z.string().min(1, 'Feedback is required').max(200, 'Feedback cannot exceed 200 characters'), +}); + export default function DialogDemo() { const [open, setOpen] = useState(false); const direction = useDirection(); - const FormSchema = z.object({ - feedback: z.string().min(1, 'Feedback is required').max(200, 'Feedback cannot exceed 200 characters'), - }); - const form = useForm<z.infer<typeof FormSchema>>({
38-50:onSubmitignores form data parameter.The handler receives validated form data but doesn't use it. If the feedback content should be submitted (e.g., to an API), the parameter should be utilized. If this is intentional for the demo, consider adding an underscore prefix or a comment.
- function onSubmit() { + function onSubmit(_data: z.infer<typeof FormSchema>) { + // TODO: Send _data.feedback to API
22-22: Multiple web components use inconsistent zod imports—consider updating tozod/v4for consistency.This file imports from
'zod'while core typegen files (typegen.ts, types.ts, server files) import from'zod/v4'. Other component files like form/default.tsx, ServerEnvField.tsx, and EnvVarField.tsx have the same inconsistency. Consider updating the import to'zod/v4'to align with the package's convention.-import { z } from 'zod'; +import { z } from 'zod/v4';packages/typegen/web/src/components/ui/accordion.tsx (1)
143-156:classNameapplied to bothAccordionPrimitive.Contentand innerdiv.The
classNameprop is applied twice—to the Content wrapper (line 150) and the inner div (line 153). This may cause unintended style duplication or conflicts.If the intent is to allow custom styling on the content container, apply
classNameonly to the inner div:<AccordionPrimitive.Content data-slot="accordion-content" - className={cn(accordionContentVariants({ variant }), className)} + className={cn(accordionContentVariants({ variant }))} {...rest} > <div className={cn('pb-5 pt-0', className)}>{children}</div>packages/typegen/web/src/components/ui/alert.tsx (1)
193-193:AlertIconPropsreused for unrelated components.
AlertToolbarandAlertContentuseAlertIconPropsas their prop type, but these components don't semantically relate to icons and don't use the variant props. Consider defining separate prop types or using a more generic base type.-function AlertToolbar({ children, className, ...props }: AlertIconProps) { +function AlertToolbar({ children, className, ...props }: React.HTMLAttributes<HTMLDivElement>) {-function AlertContent({ className, ...props }: React.HTMLAttributes<HTMLParagraphElement>) { +function AlertContent({ className, children, ...props }: React.HTMLAttributes<HTMLDivElement>) {Note:
AlertContentalready usesHTMLParagraphElementbut renders adiv—this is a semantic type mismatch.Also applies to: 230-236, 248-256
packages/typegen/web/src/components/ui/command.tsx (1)
24-35: Consider adding an accessible dialog title instead of an empty hiddenDialogTitle.Right now
DialogTitleis rendered withclassName="hidden"and no content, so the dialog has no accessible name. If you intended a screen-reader-only title, consider rendering text with a visually-hidden class (e.g.sr-only) or wiring anaria-labelonDialogContent; otherwise you can safely dropDialogTitlealtogether.packages/typegen/src/types.ts (1)
84-139: Minor schema cleanups for defaults and single-config support.Two small improvements you might want to consider:
path:const path = z .string() .default("schema") .optional();The
.optional()is redundant because.default("schema")already handlesundefined. You can simplify to:const path = z .string() .default("schema") .meta({ description: "The folder path to output the generated files" });
typegenConfig.config:export const typegenConfig = z.object({ config: z.union([z.array(typegenConfigSingle), typegenConfigSingle]), });This means callers now see
configasSingleConfig | SingleConfig[]. Ensure downstream code (e.g.generateTypedClientsintypegen.tsandrunCodegenincli.ts) normalizes this to an array or accepts both shapes; otherwise single-config files will parse but still break at runtime.packages/typegen/web/src/App.tsx (1)
53-69: Minor cleanup: avoid logging noise and guardconfigsagainstundefined.Two small polish tweaks:
- The
console.log("configData from useEffect", configDataResponse);inuseEffectis useful during development but will be noisy in production; consider removing or gating behind a debug flag.const configs = form.watch("config");can produceundefinedbriefly; usingconst configs = form.watch("config") ?? [];makes downstream access (e.g.configs[index]when mappingfields) safer.Neither is blocking but they’ll tighten up runtime behavior a bit.
packages/typegen/web/src/lib/api.ts (1)
1-43: Config client error handling looks solid; consider tightening response typing later.Both
getConfigandsaveConfigcorrectly checkres.okand surface meaningful error messages (including per-path issues for saves), which should make UI failures much easier to debug. Longer-term you might want to type thejson()responses using the server contract types (e.g.GetConfigResponse,PostConfigResponse) instead ofany, but that’s purely a refinement.packages/typegen/src/cli.ts (1)
252-274: Silent env parse is intentional but note that syntax errors in.envare now only surfaced later.
parseEnvsnow ignoresenvRes.errorentirely, relying on later code to complain about missing env vars instead of failing fast when.envparsing fails. This matches the “should fail silently” comment, but just be aware that invalid.envsyntax won’t cause the CLI to exit anymore; only downstream checks will catch it.packages/typegen/web/src/components/EnvVarDialog.tsx (1)
164-208: Clean up unreachable"none"branch in auth type select
authTypeSelectorand theonValueChangehandler support a"none"mode that clearsenvNames.auth, but the<Select>only exposes"apiKey"and"username"options. That makes the"none"branch effectively dead code.If “no auth” isn’t a real state, you can simplify by dropping
"none"from the union and the correspondingelsebranch. If you do plan to support it, consider adding an explicit “No auth” option so this path is actually reachable.packages/typegen/web/src/components/ui/switch.tsx (1)
8-19: MakeSwitchContextguard effective so misuse is caught in development
SwitchContextis created with a non-null default{ permanent: false }, souseSwitchContext’sif (!context)check will never fire. That meansSwitch/SwitchIndicatorcan be rendered outsideSwitchWrapperwithout any warning, even though the error message implies otherwise.You can make the guard actually work and clarify the message like this:
-const SwitchContext = React.createContext<{ permanent: boolean }>({ - permanent: false, -}); +const SwitchContext = React.createContext<{ permanent: boolean } | undefined>( + undefined, +); const useSwitchContext = () => { const context = React.useContext(SwitchContext); if (!context) { - throw new Error("SwitchIndicator must be used within a Switch component"); + throw new Error( + "Switch and SwitchIndicator must be used within a SwitchWrapper", + ); } return context; };This keeps the permanent-flag plumbing intact while ensuring misuse is surfaced early.
Also applies to: 130-147, 149-189
packages/typegen/web/src/components/ui/form.tsx (1)
117-134: Tighten ARIA wiring and DOM typings for description/messageTwo small refinements here:
FormDescription/FormMessageare typed asHTMLParagraphElementprops but render<div>elements, which can confuse refs and stricter typing.- When
erroris present,FormDescriptionreturnsnull, butFormControlstill includesformDescriptionIdinaria-describedby, so assistive tech can reference a non-existent node.You can make both more consistent like this:
function FormControl({ ...props }: React.ComponentProps<typeof Slot>) { const { error, formItemId, formDescriptionId, formMessageId } = useFormField(); @@ - <Slot + <Slot data-slot="form-control" id={formItemId} - aria-describedby={ - !error - ? `${formDescriptionId}` - : `${formDescriptionId} ${formMessageId}` - } + aria-describedby={error ? formMessageId : formDescriptionId} aria-invalid={!!error} {...props} /> @@ function FormDescription({ className, ...props -}: React.HTMLAttributes<HTMLParagraphElement>) { +}: React.HTMLAttributes<HTMLDivElement>) { @@ function FormMessage({ className, children, ...props -}: React.HTMLAttributes<HTMLParagraphElement>) { +}: React.HTMLAttributes<HTMLDivElement>) {That keeps
aria-describedbypointing only at elements that are actually rendered and aligns the TS types with the actual DOM nodes.Also applies to: 136-178
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (88)
apps/demo/package.json(2 hunks)apps/docs/package.json(2 hunks)package.json(1 hunks)packages/better-auth/package.json(1 hunks)packages/cli/package.json(4 hunks)packages/cli/src/utils/formatting.ts(2 hunks)packages/fmdapi/package.json(2 hunks)packages/fmodata/package.json(1 hunks)packages/fmodata/src/client/error-parser.ts(1 hunks)packages/fmodata/tests/tsconfig.build.json(1 hunks)packages/registry/lib/types.ts(2 hunks)packages/registry/lib/validator.ts(1 hunks)packages/registry/package.json(2 hunks)packages/registry/scripts/validate.ts(1 hunks)packages/registry/tsconfig.json(1 hunks)packages/registry/tsdown.config.ts(1 hunks)packages/typegen/package.json(3 hunks)packages/typegen/proofkit-typegen.config.jsonc(1 hunks)packages/typegen/scripts/build-copy.js(1 hunks)packages/typegen/scripts/build.ts(1 hunks)packages/typegen/src/buildSchema.ts(2 hunks)packages/typegen/src/cli.ts(5 hunks)packages/typegen/src/formatting.ts(2 hunks)packages/typegen/src/server/api.ts(1 hunks)packages/typegen/src/server/app.ts(1 hunks)packages/typegen/src/server/contract.ts(1 hunks)packages/typegen/src/server/createDataApiClient.ts(1 hunks)packages/typegen/src/server/index.ts(1 hunks)packages/typegen/src/typegen.ts(2 hunks)packages/typegen/src/types.ts(1 hunks)packages/typegen/vite.config.ts(1 hunks)packages/typegen/web/components.json(1 hunks)packages/typegen/web/index.html(1 hunks)packages/typegen/web/package.json(1 hunks)packages/typegen/web/src/App.tsx(1 hunks)packages/typegen/web/src/components/ConfigEditor.tsx(1 hunks)packages/typegen/web/src/components/ConfigList.css(1 hunks)packages/typegen/web/src/components/ConfigList.tsx(1 hunks)packages/typegen/web/src/components/ConfigSummary.tsx(1 hunks)packages/typegen/web/src/components/EnvVarDialog.tsx(1 hunks)packages/typegen/web/src/components/EnvVarField.tsx(1 hunks)packages/typegen/web/src/components/InfoTooltip.tsx(1 hunks)packages/typegen/web/src/components/LayoutEditor.tsx(1 hunks)packages/typegen/web/src/components/LayoutItemEditor.tsx(1 hunks)packages/typegen/web/src/components/LayoutSelector.tsx(1 hunks)packages/typegen/web/src/components/ServerEnvField.tsx(1 hunks)packages/typegen/web/src/components/badge/circle.tsx(1 hunks)packages/typegen/web/src/components/button/loading.tsx(1 hunks)packages/typegen/web/src/components/combobox/default.tsx(1 hunks)packages/typegen/web/src/components/dialog/default.tsx(1 hunks)packages/typegen/web/src/components/form/default.tsx(1 hunks)packages/typegen/web/src/components/input/addon.tsx(1 hunks)packages/typegen/web/src/components/select/default.tsx(1 hunks)packages/typegen/web/src/components/switch/button.tsx(1 hunks)packages/typegen/web/src/components/switch/default.tsx(1 hunks)packages/typegen/web/src/components/tree/default.tsx(1 hunks)packages/typegen/web/src/components/ui/accordion.tsx(1 hunks)packages/typegen/web/src/components/ui/alert.tsx(1 hunks)packages/typegen/web/src/components/ui/badge.tsx(1 hunks)packages/typegen/web/src/components/ui/button.tsx(1 hunks)packages/typegen/web/src/components/ui/command.tsx(1 hunks)packages/typegen/web/src/components/ui/dialog.tsx(1 hunks)packages/typegen/web/src/components/ui/form.tsx(1 hunks)packages/typegen/web/src/components/ui/input.tsx(1 hunks)packages/typegen/web/src/components/ui/label.tsx(1 hunks)packages/typegen/web/src/components/ui/popover.tsx(1 hunks)packages/typegen/web/src/components/ui/select.tsx(1 hunks)packages/typegen/web/src/components/ui/switch-field.tsx(1 hunks)packages/typegen/web/src/components/ui/switch.tsx(1 hunks)packages/typegen/web/src/components/ui/textarea.tsx(1 hunks)packages/typegen/web/src/components/ui/tooltip.tsx(1 hunks)packages/typegen/web/src/components/ui/tree.tsx(1 hunks)packages/typegen/web/src/components/useEnvVarIndicator.ts(1 hunks)packages/typegen/web/src/hooks/useConfig.ts(1 hunks)packages/typegen/web/src/hooks/useTestConnection.ts(1 hunks)packages/typegen/web/src/index.css(1 hunks)packages/typegen/web/src/lib/api.ts(1 hunks)packages/typegen/web/src/lib/config-utils.ts(1 hunks)packages/typegen/web/src/lib/envValues.ts(1 hunks)packages/typegen/web/src/lib/schema.ts(1 hunks)packages/typegen/web/src/lib/utils.ts(1 hunks)packages/typegen/web/src/main.tsx(1 hunks)packages/typegen/web/tsconfig.app.json(1 hunks)packages/typegen/web/tsconfig.json(1 hunks)packages/typegen/web/tsconfig.node.json(1 hunks)packages/typegen/web/vite.config.ts(1 hunks)packages/webviewer/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (51)
- packages/typegen/scripts/build.ts
- packages/typegen/web/package.json
- packages/typegen/web/tsconfig.app.json
- packages/typegen/web/src/main.tsx
- packages/typegen/src/formatting.ts
- packages/typegen/web/src/components/switch/button.tsx
- packages/typegen/web/src/components/ui/button.tsx
- packages/typegen/web/tsconfig.node.json
- packages/typegen/web/src/components/ServerEnvField.tsx
- packages/typegen/web/src/components/badge/circle.tsx
- packages/typegen/web/src/components/LayoutEditor.tsx
- packages/typegen/web/src/lib/envValues.ts
- apps/demo/package.json
- packages/typegen/web/components.json
- packages/typegen/web/src/components/combobox/default.tsx
- packages/fmodata/src/client/error-parser.ts
- pnpm-workspace.yaml
- packages/fmodata/package.json
- packages/typegen/web/src/components/ConfigSummary.tsx
- packages/typegen/web/src/components/EnvVarField.tsx
- packages/typegen/web/src/components/input/addon.tsx
- packages/registry/tsconfig.json
- packages/fmodata/tests/tsconfig.build.json
- packages/typegen/web/src/hooks/useConfig.ts
- packages/typegen/web/src/components/ConfigList.tsx
- packages/typegen/vite.config.ts
- packages/typegen/web/src/components/ui/label.tsx
- packages/typegen/web/src/components/ui/popover.tsx
- packages/webviewer/package.json
- packages/typegen/web/src/index.css
- packages/typegen/web/vite.config.ts
- packages/typegen/web/src/lib/utils.ts
- packages/typegen/web/src/components/InfoTooltip.tsx
- package.json
- packages/typegen/scripts/build-copy.js
- packages/typegen/web/src/components/ConfigList.css
- packages/registry/tsdown.config.ts
- packages/cli/src/utils/formatting.ts
- packages/typegen/web/index.html
- packages/typegen/web/src/components/form/default.tsx
- packages/typegen/proofkit-typegen.config.jsonc
- packages/typegen/web/src/components/ui/tooltip.tsx
- packages/typegen/src/buildSchema.ts
- packages/typegen/package.json
- packages/typegen/web/src/components/useEnvVarIndicator.ts
- packages/registry/scripts/validate.ts
- packages/typegen/src/server/api.ts
- packages/typegen/web/src/hooks/useTestConnection.ts
- packages/typegen/web/src/components/select/default.tsx
- packages/typegen/web/src/components/ui/dialog.tsx
- packages/typegen/web/src/lib/config-utils.ts
🧰 Additional context used
🧬 Code graph analysis (17)
packages/typegen/web/src/components/switch/default.tsx (3)
packages/typegen/web/src/components/switch/button.tsx (1)
SwitchDemo(4-20)packages/typegen/web/src/components/ui/switch.tsx (1)
Switch(191-191)packages/typegen/web/src/components/ui/label.tsx (1)
Label(31-31)
packages/typegen/src/cli.ts (1)
packages/typegen/src/server/index.ts (1)
startServer(23-96)
packages/typegen/web/src/components/button/loading.tsx (1)
packages/typegen/web/src/components/ui/button.tsx (1)
Button(423-423)
packages/typegen/web/src/components/ui/textarea.tsx (1)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)
packages/typegen/src/server/app.ts (5)
packages/typegen/src/server/api.ts (1)
ApiContext(8-11)packages/fmdapi/src/client-types.ts (1)
LayoutOrFolder(262-262)packages/typegen/src/types.ts (2)
typegenConfig(137-139)typegenConfigSingle(130-135)packages/typegen/src/typegen.ts (1)
generateTypedClients(28-57)packages/typegen/src/server/createDataApiClient.ts (2)
createDataApiClient(192-251)createClientFromConfig(41-184)
packages/typegen/src/server/index.ts (1)
packages/typegen/src/server/app.ts (1)
createApiApp(58-428)
packages/typegen/web/src/components/ui/accordion.tsx (1)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)
packages/typegen/src/server/createDataApiClient.ts (2)
packages/typegen/src/types.ts (2)
typegenConfigSingle(130-135)typegenConfig(137-139)packages/typegen/src/server/api.ts (1)
ApiContext(8-11)
packages/typegen/web/src/components/ui/select.tsx (1)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)
packages/typegen/src/server/contract.ts (3)
packages/typegen/web/src/lib/config-utils.ts (2)
SingleConfig(4-4)ConfigsArray(7-7)packages/typegen/src/types.ts (1)
typegenConfigSingle(130-135)packages/typegen/web/src/lib/schema.ts (1)
typegenConfigSingle(3-3)
packages/typegen/web/src/components/ui/tree.tsx (1)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)
packages/typegen/web/src/components/ui/alert.tsx (1)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)
packages/typegen/web/src/components/ui/switch.tsx (1)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)
packages/typegen/web/src/components/ui/form.tsx (2)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)packages/typegen/web/src/components/ui/label.tsx (1)
Label(31-31)
packages/typegen/src/typegen.ts (2)
packages/typegen/src/types.ts (1)
typegenConfigSingle(130-135)packages/typegen/web/src/lib/schema.ts (1)
typegenConfigSingle(3-3)
packages/typegen/web/src/components/ui/command.tsx (2)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)packages/typegen/web/src/components/ui/dialog.tsx (3)
Dialog(128-128)DialogContent(131-131)DialogTitle(137-137)
packages/typegen/src/types.ts (2)
apps/docs/scripts/bundle-registry-templates.js (1)
path(4-4)packages/typegen/web/src/lib/schema.ts (1)
typegenConfigSingle(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (40)
packages/typegen/web/tsconfig.json (3)
26-27: Include and references configured appropriately.The inclusion of both
srcand../srcenables the path aliases to resolve correctly, and the project reference totsconfig.node.jsonfollows standard conventions for monorepo tooling configuration.
9-9: No concerns. TheallowImportingTsExtensionssetting is correctly configured for this Vite setup. WithnoEmit: truein tsconfig.json, TypeScript is used only for type checking, and Vite handles all transpilation. The final production bundle contains .js files, not .ts files, so direct .ts imports in the final output are not a concern.
2-24: Path aliases and React JSX configuration are correct.The path mappings all point to valid files and directories. The web subproject uses React 19.1.1, which fully supports the
react-jsxJSX transform (introduced in React 17), so the compiler option is appropriately configured. The overall TypeScript configuration for the web UI is sound.packages/typegen/web/src/components/button/loading.tsx (1)
7-28: Component behavior and cleanup look goodThe client component setup, state toggling logic, and interval cleanup in
useEffectare all idiomatic and safe. The loading/disabled rendering on theButtonalso reads clearly.packages/typegen/web/src/components/ui/badge.tsx (2)
22-170: LGTM! Comprehensive variant configuration.The
badgeVariantsCVA configuration is well-structured with comprehensive variant options, compound variants for different appearance modes, and good use of CSS custom properties with fallbacks.
229-240: LGTM! Clean implementation.The
BadgeDotcomponent is simple, well-implemented, and correctly usescurrentColorto inherit the parent badge's color.apps/docs/package.json (1)
43-43: Verify Zod v4 migration in source code; Vitest v4 compatibility in tests.The dependency updates include Zod v4 (major version bump with breaking changes) and Vitest v4. Per the library documentation, Zod v4 introduces:
- Unified
errorparameter for error customization (replacing fragmented error handling)- Changed default value behavior (short-circuits on
undefined)- Object schema improvements affecting optional field defaults
- Number validation changes (rejects infinite values, only safe integers)
Ensure all source code using Zod has been migrated to v4 API patterns, and that tests pass with Vitest v4.
Also applies to: 58-59
packages/better-auth/package.json (1)
61-61: Verify Zod v4 usage in better-auth source code.This is a core dependency that other packages rely on. Ensure that zod v4 API patterns have been applied throughout the authentication module.
Also applies to: 69-70
packages/fmdapi/package.json (1)
61-61: Verify Zod v4 migration in fmdapi Data API client.As a public dependency (exported as
.,./utils/*,./tokenStore/*,./adapters/*), ensure all schema validations and error handling use Zod v4 API.Also applies to: 75-76
packages/registry/package.json (1)
55-55: Zod v4.2.0 in registry requires migration verification.Registry's validation script (
scripts/validate.ts) must use Zod v4 API. Additionally, any schema definitions in the registry lib that are exported to downstream packages must be compatible with Zod v4.packages/cli/package.json (1)
70-70: Multiple major version bumps in CLI — verify end-to-end functionality.Several significant dependency updates beyond the standard Zod v4/TypeScript/Vitest bumps:
- es-toolkit:
1.15.1→1.38.0— verify API compatibility in CLI utilities- @vitest/coverage-v8:
1.4.0→2.1.8(major version) — confirm test coverage works- @tanstack/react-query:
5.49.2→5.76.1— minor bump but used in templates- Vitest:
3.2.4→4.0.7— major version jumpSince the CLI generates project templates and runs code generation, incompatibilities could break user project scaffolding. Recommend thorough CLI functional testing, especially around:
- Template generation and TypeScript codegen
- Test command execution
- Plugin initialization
Also applies to: 99-99, 111-111, 127-128
packages/typegen/web/src/components/ui/tree.tsx (5)
35-53: LGTM!The Tree component implementation is well-structured with proper defensive checks, prop merging, and CSS variable setup.
61-100: LGTM!The TreeItem component demonstrates good practices with defensive method checks, proper prop merging, accessibility attributes, and flexible rendering via the Slot pattern.
106-137: LGTM!TreeItemLabel has good fallback logic, proper warning for missing items, and clear icon rendering based on toggle type and folder state.
139-158: LGTM!TreeDragLine component properly validates the tree context and getDragLineStyle method availability with appropriate warnings.
7-7: The import is correct.radix-ui(v1.4.3) is a legitimate monorepo entrypoint package that includes Radix UI Slot. The web search confirms that Slot can be imported from either@radix-ui/react-slotor theradix-uientrypoint, and the code correctly usesSlotPrimitive.Slotsyntax.packages/typegen/web/src/components/tree/default.tsx (3)
1-11: LGTM!Clean imports and straightforward interface definition for the tree data structure.
13-74: LGTM!The static CRM data structure is well-organized and provides a comprehensive example of a hierarchical tree.
92-105: LGTM!The rendering logic is clean and properly uses keys for the list of tree items.
packages/registry/lib/validator.ts (1)
5-5: Explicit.jsextension on the schema import looks correct for ESM buildsSwitching the import to
./types.jsmatches the emitted filename for TS→ESM output and aligns with the rest of this module’s ESM usage (import.meta.url,jiti). No further changes needed here.packages/typegen/web/src/components/ui/input.tsx (1)
5-49: Base input and addon variants look consistent and robustThe sizing, disabled/read‑only, file input, and
aria-invalidhandling ininputVariantsandinputAddonVariantsare coherent and should give you good standalone ergonomics for both raw inputs and add‑ons. I don’t see functional issues here.packages/typegen/web/src/components/ui/select.tsx (5)
3-8: Verify Radix Select import alias
import { Select as SelectPrimitive } from 'radix-ui';assumes a local barrel or alias that re-exports the Radix Select namespace. If you’re using the upstream package directly, this would typically look like:-import { Select as SelectPrimitive } from 'radix-ui'; +import * as SelectPrimitive from '@radix-ui/react-select';Please confirm the module/alias setup so this doesn’t turn into a “module not found” or incorrect-type issue at runtime.
10-33: Context-driven root wrapper looks solidThe
SelectContext+Selectroot wrapper cleanly centralizeindicatorPosition,indicatorVisibility, andindicatorwhile keeping all descendants in a single provider scope. Default values are sane and the root still just delegates toSelectPrimitive.Rootvia props.
43-84: Trigger variants and API look goodThe
selectTriggerVariantssetup andSelectTriggerwrapper give a compact, size-based API with good focus/invalid state coverage, and theSelectTriggerPropstyping matches the intended variant + Radix props surface. Nothing blocking here.
86-146: Content/portal composition is well-structured
SelectContentcorrectly wraps in a portal, applies animated state/side classes, and reuses the scroll buttons with consistent slot data. Theposition === 'popper'branch aligning viewport size with trigger height/width is a nice touch.
158-188: Double-checkoutline-hiddenutilityIn
SelectItem’s class list,outline-hiddenis not a standard Tailwind utility out of the box (vsoutline-none). If you don’t have a custom plugin for it, it will be a no-op:className={cn( 'relative flex w-full cursor-default select-none items-center rounded-sm py-1.5 text-sm outline-hidden text-foreground ...', ... )}If this isn’t intentional, consider swapping it for
outline-noneor removing it.packages/typegen/src/typegen.ts (2)
44-56: LGTM! Clean array normalization and type filtering.The config normalization and iteration logic is sound. Processing only
"fmdapi"type with a warning for unsupported types provides good forward compatibility for when other config types are implemented.
60-60: Good type narrowing usingExtract.The type constraint ensures
generateTypedClientsSingleonly accepts"fmdapi"configurations, providing compile-time safety that aligns with the runtime check ingenerateTypedClients.packages/typegen/web/src/components/ui/textarea.tsx (1)
29-35: LGTM! Clean component implementation.The Textarea component follows the established pattern with proper prop forwarding, variant support, and className composition via
cn. Thedata-slotattribute enables slot-based styling.packages/typegen/web/src/components/ui/accordion.tsx (2)
86-91: Type mismatch:indicatorprop excludes'none'but context type includes it.The
Accordioncomponent'sindicatorprop (line 89) only allows'arrow' | 'plus', butAccordionContextType(line 77) includes'none'. This means'none'can only come from the default context value, which may be intentional for internal use. If'none'should be externally settable, update the prop type.Is the
'none'indicator variant intended to be user-facing? If so:VariantProps<typeof accordionRootVariants> & { - indicator?: 'arrow' | 'plus'; + indicator?: 'arrow' | 'plus' | 'none'; },
122-141: LGTM! Well-structured trigger with conditional indicator rendering.The trigger correctly consumes context for variant and indicator, applies appropriate styles, and conditionally renders the icon based on the indicator type. The CSS transforms for open/closed states are properly handled.
packages/typegen/web/src/components/ui/alert.tsx (1)
195-216: LGTM! Well-implemented Alert component with close functionality.The Alert component correctly composes variants, handles the close button with proper accessibility (
aria-label), and follows the slot-based pattern consistently. Therole="alert"ensures screen reader accessibility.packages/typegen/web/src/lib/schema.ts (1)
1-3: Scoped re-export for browser usage looks good.Re-exporting only
typegenConfigSingleasconfigSchemafrom the types entrypoint cleanly avoids bundling the node-only generator into the web app. Nice separation of concerns.packages/typegen/web/src/components/switch/default.tsx (1)
1-11: LGTM on the Switch demo component.Simple, accessible wiring between
SwitchandLabelwithid/htmlFor; nothing concerning here.packages/typegen/web/src/components/ui/switch-field.tsx (1)
1-71: Well-structured, accessible switch field.Nice composition of
Switch,Label,InfoTooltip, and description witharia-describedby; props cover the common needs cleanly and the generated id fallback looks solid.packages/typegen/src/cli.ts (2)
138-173: Generate command env handling and deprecation behavior look consistent.The generate command now always calls
parseEnvsbut logs a deprecation notice when--skip-env-checkis used, andparseEnvsitself fails silently if it can’t load env files. That’s a reasonable balance between backward compatibility and a smoother DX for misconfigured environments.
186-248:uicommand wiring and shutdown handling are solid.The
uicommand correctly:
- Resolves the config path (with fallback),
- Loads env vars before starting,
- Validates the optional
--port,- Starts the server via
startServer,- Optionally opens the browser, and
- Handles SIGINT/SIGTERM for graceful shutdown.
This looks like a clean integration of the new UI server into the CLI.
packages/typegen/web/src/components/ConfigEditor.tsx (1)
147-183: VerifygenerateClientsemantics between top-level switch and per-layout selectThe top-level “Generate” switch maps
checkedto “Types Only” and unchecked to “Full Client”, whileLayoutItemEditormapsgenerateClient === trueto “Generate Client” andfalseto “Types Only”. Please confirm that the stored boolean meaning is consistent across config-level and layout-level (e.g.,truealways means “full client” or always “types only”) and adjust labels/mapping as needed to avoid inverted behavior.packages/typegen/web/src/components/LayoutItemEditor.tsx (1)
30-205: LayoutItemEditor form wiring and tri-state config look solidThe per-layout editor correctly wires into the
configarray, keeps header labels in sync viawatch, and handlesvalueLists,strictNumbers, andgenerateClient(default/true/false) in a predictable way. I don’t see correctness or UX blockers here.packages/typegen/src/server/contract.ts (1)
1-75: API contract schemas and inferred types look coherentThe Zod contracts here (single vs array config unions, config/run/layout/env-names responses, and the re-exported
SingleConfig/ConfigsArrayaliases) are internally consistent and match the shapes implied by the existingtypegenConfigSingleschema. I don’t see any blocking issues in this file.
| const envNames = z | ||
| .object({ | ||
| server: z.string(), | ||
| db: z.string(), | ||
| server: z | ||
| .string() | ||
| .optional() | ||
| .transform((val) => (val === "" ? undefined : val)), | ||
| db: z | ||
| .string() | ||
| .optional() | ||
| .transform((val) => (val === "" ? undefined : val)), | ||
| auth: z.union([ | ||
| z | ||
| .object({ | ||
| apiKey: z.string(), | ||
| apiKey: z | ||
| .string() | ||
| .optional() | ||
| .transform((val) => (val === "" ? undefined : val)), | ||
| }) | ||
| .partial(), | ||
| .optional() | ||
| .transform((val) => { | ||
| if (val && Object.values(val).every((v) => v === undefined)) { | ||
| return undefined; | ||
| } | ||
| return val ?? undefined; | ||
| }), | ||
| z | ||
| .object({ | ||
| username: z.string(), | ||
| password: z.string(), | ||
| username: z | ||
| .string() | ||
| .optional() | ||
| .transform((val) => (val === "" ? undefined : val)), | ||
| password: z | ||
| .string() | ||
| .optional() | ||
| .transform((val) => (val === "" ? undefined : val)), | ||
| }) | ||
| .partial(), | ||
| .optional() | ||
| .transform((val) => { | ||
| if (val && Object.values(val).every((v) => v === undefined)) { | ||
| return undefined; | ||
| } | ||
| return val ?? undefined; | ||
| }), | ||
| ]), | ||
| }) | ||
| .partial() | ||
| .optional() | ||
| .transform((val) => { | ||
| if (val && Object.values(val).every((v) => v === undefined)) { | ||
| return undefined; | ||
| } | ||
| return val ?? undefined; | ||
| }) | ||
| .meta({ | ||
| description: | ||
| "If you're using other environment variables than the default, custom the NAMES of them here for the typegen to lookup their values when it runs.", | ||
| }); |
There was a problem hiding this comment.
envNames.auth is effectively required now; this may break existing configs.
envNames’s shape makes auth a required key:
const envNames = z
.object({
server: ...,
db: ...,
auth: z.union([...]), // no .optional() here
})
.optional()
.transform(...);Even though each auth variant is optional().transform(...), the property itself is required by the object schema, so configs like:
{
"envNames": {
"server": "FM_HOST",
"db": "FM_DB"
// no "auth"
}
}will now fail validation. That’s a behavior change compared to the more permissive interpretation where auth could be omitted entirely.
If you want auth to remain optional while still collapsing empty auth objects to undefined, consider:
auth: z
.union([
// existing variants
])
.optional()or wrapping the existing union in an outer .optional() so both the property and the inner objects can be omitted/emptied without errors.
🤖 Prompt for AI Agents
In packages/typegen/src/types.ts around lines 27 to 82, the object schema
defines auth as a required key because the z.union(...) is not marked optional;
make the auth property optional so configs that omit auth still validate. Change
the schema so auth is either the existing union wrapped with .optional() (i.e.
z.union([...]).optional()) or wrap the entire union in an outer .optional(), and
keep the current per-variant .optional().transform(...) logic so empty objects
still collapse to undefined; update any downstream typing if necessary.
| // Run typegen mutation | ||
| const runTypegenMutation = useMutation({ | ||
| mutationFn: async () => { | ||
| await client.api.run.$post({ | ||
| json: { config: configs }, | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/typegen/web/src/App.tsx | head -100 | tail -40Repository: proofgeist/proofkit
Length of output: 1514
🏁 Script executed:
# Find the saveConfig implementation to compare error handling
rg "saveConfig" packages/typegen/web/src/ -B 3 -A 10Repository: proofgeist/proofkit
Length of output: 2835
🏁 Script executed:
# Check how client API calls are structured elsewhere
rg "client\.api\." packages/typegen/web/src/App.tsx -B 2 -A 5Repository: proofgeist/proofkit
Length of output: 229
🏁 Script executed:
# Search for Response handling patterns in the codebase
rg "\.ok\b" packages/typegen/web/src/ -B 2 -A 2Repository: proofgeist/proofkit
Length of output: 3540
🏁 Script executed:
# Check the client configuration to understand the API client type
fd "client\." packages/typegen/web/src -o | head -20
rg "import.*client" packages/typegen/web/src/App.tsxRepository: proofgeist/proofkit
Length of output: 232
🏁 Script executed:
# Check how runTypegenMutation errors are handled in the UI
rg "runTypegenMutation" packages/typegen/web/src/App.tsx -A 5 -B 2Repository: proofgeist/proofkit
Length of output: 1320
🏁 Script executed:
# See the full handleRunTypegen function
sed -n '92,110p' packages/typegen/web/src/App.tsxRepository: proofgeist/proofkit
Length of output: 570
🏁 Script executed:
# Check if there's any error display for runTypegenMutation in the component
rg "runTypegenMutation\.(error|isError)" packages/typegen/web/src/App.tsxRepository: proofgeist/proofkit
Length of output: 45
Missing error check on runTypegenMutation allows silent failure if the server returns an error status.
client.api.run.$post returns a Response object, but the mutation doesn't check res.ok, treating all responses (including 4xx/5xx errors) as success:
const runTypegenMutation = useMutation({
mutationFn: async () => {
await client.api.run.$post({
json: { config: configs },
});
},
});This is inconsistent with other API calls in the codebase—saveConfig, getConfig, and useTestConnection all check res.ok and throw errors on failure. Without this check, users receive no feedback if the typegen run fails on the server.
Add an error check matching the saveConfig pattern:
const runTypegenMutation = useMutation({
- mutationFn: async () => {
- await client.api.run.$post({
- json: { config: configs },
- });
- },
+ mutationFn: async () => {
+ const res = await client.api.run.$post({
+ json: { config: configs },
+ });
+
+ if (!res.ok) {
+ const errorData = (await res.json().catch(() => ({}))) as {
+ error?: string;
+ };
+ throw new Error(
+ errorData.error ||
+ `Failed to run typegen: ${res.status} ${res.statusText}`,
+ );
+ }
+
+ return res.json().catch(() => null);
+ },
});🤖 Prompt for AI Agents
In packages/typegen/web/src/App.tsx around lines 70 to 77, the mutation calls
client.api.run.$post but does not inspect the Response, allowing 4xx/5xx
responses to be treated as success; change the mutationFn to capture the
Response, check res.ok, and if not ok throw an Error that includes the status
and response body (similar to saveConfig), otherwise return the parsed JSON on
success so callers get consistent error handling and feedback.
| { | ||
| variant: 'mono', | ||
| appearance: 'solid', | ||
| className: 'bg-zinc-950 text-white dark:bg-zinc-300 dark:text-black *:data-slot-[alert=close]:text-white', | ||
| }, |
There was a problem hiding this comment.
Likely typo in CSS selector for mono variant.
The selector *:data-slot-[alert=close] on line 74 appears malformed. Based on the pattern used elsewhere (e.g., [&_[data-slot=alert-close]]), this should likely be:
- className: 'bg-zinc-950 text-white dark:bg-zinc-300 dark:text-black *:data-slot-[alert=close]:text-white',
+ className: 'bg-zinc-950 text-white dark:bg-zinc-300 dark:text-black [&_[data-slot=alert-close]]:text-white',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| variant: 'mono', | |
| appearance: 'solid', | |
| className: 'bg-zinc-950 text-white dark:bg-zinc-300 dark:text-black *:data-slot-[alert=close]:text-white', | |
| }, | |
| { | |
| variant: 'mono', | |
| appearance: 'solid', | |
| className: 'bg-zinc-950 text-white dark:bg-zinc-300 dark:text-black [&_[data-slot=alert-close]]:text-white', | |
| }, |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/alert.tsx around lines 71 to 75, the
CSS selector `*:data-slot-[alert=close]` is malformed; replace it with the
correct pattern used elsewhere (e.g. `[&_[data-slot=alert-close]]`) so the mono
variant targets the alert close slot properly—update the className string to use
`[&_[data-slot=alert-close]]:text-white` (or the equivalent selector used
project-wide) preserving other classes like bg-zinc-950 text-white
dark:bg-zinc-300 dark:text-black.
| extends React.HTMLAttributes<HTMLDivElement>, | ||
| VariantProps<typeof badgeVariants> { | ||
| asChild?: boolean; | ||
| dotClassName?: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused dotClassName prop.
The dotClassName prop is declared in BadgeProps but is never used in the Badge component implementation (lines 186-209).
If this prop is not needed, apply this diff:
export interface BadgeProps
extends React.HTMLAttributes<HTMLDivElement>,
VariantProps<typeof badgeVariants> {
asChild?: boolean;
- dotClassName?: string;
disabled?: boolean;
}If it's intended for future use with BadgeDot, please add the implementation or document its intended usage.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dotClassName?: string; | |
| export interface BadgeProps | |
| extends React.HTMLAttributes<HTMLDivElement>, | |
| VariantProps<typeof badgeVariants> { | |
| asChild?: boolean; | |
| disabled?: boolean; | |
| } |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/badge.tsx around line 10, the
BadgeProps type declares an unused prop dotClassName; remove dotClassName from
the BadgeProps declaration (and any exported types/interfaces) and any related
references so props match the component implementation (lines ~186-209), or if
the prop is intended to style the internal BadgeDot, wire it through: add a
dotClassName prop to the Badge component signature and pass it into the BadgeDot
element as its className (or merge it with existing classes) and update the type
and any usages accordingly.
| export interface BadgeButtonProps | ||
| extends React.ButtonHTMLAttributes<HTMLDivElement>, | ||
| VariantProps<typeof badgeButtonVariants> { | ||
| asChild?: boolean; | ||
| } |
There was a problem hiding this comment.
Fix the generic type parameter for ButtonHTMLAttributes.
BadgeButtonProps extends ButtonHTMLAttributes<HTMLDivElement> but should extend ButtonHTMLAttributes<HTMLButtonElement> since it represents button attributes.
Apply this diff:
export interface BadgeButtonProps
- extends React.ButtonHTMLAttributes<HTMLDivElement>,
+ extends React.ButtonHTMLAttributes<HTMLButtonElement>,
VariantProps<typeof badgeButtonVariants> {
asChild?: boolean;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface BadgeButtonProps | |
| extends React.ButtonHTMLAttributes<HTMLDivElement>, | |
| VariantProps<typeof badgeButtonVariants> { | |
| asChild?: boolean; | |
| } | |
| export interface BadgeButtonProps | |
| extends React.ButtonHTMLAttributes<HTMLButtonElement>, | |
| VariantProps<typeof badgeButtonVariants> { | |
| asChild?: boolean; | |
| } |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/badge.tsx around lines 14 to 18, the
BadgeButtonProps incorrectly extends React.ButtonHTMLAttributes<HTMLDivElement>;
change the generic type parameter to
React.ButtonHTMLAttributes<HTMLButtonElement> so the props match a button
element. Update the extends clause to use HTMLButtonElement and confirm any
usages/consumers still compile.
| function Badge({ | ||
| className, | ||
| variant, | ||
| size, | ||
| appearance, | ||
| shape, | ||
| asChild = false, | ||
| disabled, | ||
| ...props | ||
| }: React.ComponentProps<"span"> & | ||
| VariantProps<typeof badgeVariants> & { asChild?: boolean }) { | ||
| const Comp = asChild ? SlotPrimitive.Slot : "span"; | ||
|
|
||
| return ( | ||
| <Comp | ||
| data-slot="badge" | ||
| className={cn( | ||
| badgeVariants({ variant, size, appearance, shape, disabled }), | ||
| className, | ||
| )} | ||
| {...props} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the exported BadgeProps interface for type consistency.
The Badge component defines its props inline (lines 195-196) instead of using the exported BadgeProps interface. This creates potential for the interface and implementation to drift apart.
Apply this diff:
function Badge({
className,
variant,
size,
appearance,
shape,
asChild = false,
disabled,
...props
-}: React.ComponentProps<"span"> &
- VariantProps<typeof badgeVariants> & { asChild?: boolean }) {
+}: BadgeProps) {
const Comp = asChild ? SlotPrimitive.Slot : "span";🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/badge.tsx around lines 186 to 209, the
Badge component currently declares its props inline instead of using the
exported BadgeProps interface; update the Badge function signature to accept
BadgeProps (imported if necessary) and remove the inline prop type so the
component's props use the shared exported interface, ensuring types remain
consistent and preventing drift between the implementation and the exported
type.
| function BadgeButton({ | ||
| className, | ||
| variant, | ||
| asChild = false, | ||
| ...props | ||
| }: React.ComponentProps<"button"> & | ||
| VariantProps<typeof badgeButtonVariants> & { asChild?: boolean }) { | ||
| const Comp = asChild ? SlotPrimitive.Slot : "span"; | ||
| return ( | ||
| <Comp | ||
| data-slot="badge-button" | ||
| className={cn(badgeButtonVariants({ variant, className }))} | ||
| role="button" | ||
| {...props} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix element type and semantic mismatch in BadgeButton.
The component has semantic and type inconsistencies:
- Props are typed as
React.ComponentProps<"button">(line 216) but the component renders as"span"(line 218) - Using
role="button"on a span doesn't provide proper button semantics (missing keyboard interaction, focus management, etc.)
Recommended fix: Render as an actual button element:
function BadgeButton({
className,
variant,
asChild = false,
...props
}: React.ComponentProps<"button"> &
VariantProps<typeof badgeButtonVariants> & { asChild?: boolean }) {
- const Comp = asChild ? SlotPrimitive.Slot : "span";
+ const Comp = asChild ? SlotPrimitive.Slot : "button";
return (
<Comp
data-slot="badge-button"
className={cn(badgeButtonVariants({ variant, className }))}
- role="button"
{...props}
/>
);
}If it must remain a span for styling reasons, update the type signature to React.ComponentProps<"span"> and ensure proper keyboard event handlers are added for accessibility.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function BadgeButton({ | |
| className, | |
| variant, | |
| asChild = false, | |
| ...props | |
| }: React.ComponentProps<"button"> & | |
| VariantProps<typeof badgeButtonVariants> & { asChild?: boolean }) { | |
| const Comp = asChild ? SlotPrimitive.Slot : "span"; | |
| return ( | |
| <Comp | |
| data-slot="badge-button" | |
| className={cn(badgeButtonVariants({ variant, className }))} | |
| role="button" | |
| {...props} | |
| /> | |
| ); | |
| } | |
| function BadgeButton({ | |
| className, | |
| variant, | |
| asChild = false, | |
| ...props | |
| }: React.ComponentProps<"button"> & | |
| VariantProps<typeof badgeButtonVariants> & { asChild?: boolean }) { | |
| const Comp = asChild ? SlotPrimitive.Slot : "button"; | |
| return ( | |
| <Comp | |
| data-slot="badge-button" | |
| className={cn(badgeButtonVariants({ variant, className }))} | |
| {...props} | |
| /> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/badge.tsx around lines 211 to 227, the
component types and rendered element disagree: props are typed as
React.ComponentProps<"button"> but the component defaults to rendering a "span"
with role="button", which breaks semantics and accessibility. Fix by rendering
an actual <button> by changing the default Comp from "span" to "button" (or if
you must keep a span for styling, change the prop type to
React.ComponentProps<"span"> and add proper keyboard handlers, focusable
tabIndex, and aria attributes to emulate button behavior); update the
VariantProps usage and any callers if their expectations change to ensure types
and runtime element match.
- Upgraded React and React-DOM to version 19.2.3 across various packages for improved performance and features. - Added support for OData configuration in the typegen package, allowing users to create OData clients from configuration. - Enhanced error handling in the API client creation process to provide more informative responses. - Introduced a confirmation dialog for removing configurations in the ConfigEditor component. - Improved UI components with additional icons and better state management for loading and error states. - Updated TypeScript configurations to include ES2022 features for better error handling support.
- Added a background class to AccordionItem in App.tsx for improved styling. - Cleaned up TypeScript configuration files by removing unnecessary whitespace. - Ensured consistency in component files by removing trailing whitespace in various components.
- Removed unnecessary whitespace in error-parser.ts and TypeScript configuration files. - Added error logging for unsupported API key authentication in listDatabaseNames() method. - Improved error handling in createApiApp function to provide more detailed connection error messages. - Updated ConfigEditor component to conditionally render fields based on config type, enhancing user experience. - Added console logs for debugging connection parameters in createDataApiClient function.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| await generateTypedClients(config); | ||
| await next(); | ||
| }, |
There was a problem hiding this comment.
Bug: API run endpoint missing response return
The /api/run POST endpoint calls await next() after executing generateTypedClients but doesn't return a response. In Hono, routes need to return a response to the client. Since there's no subsequent handler and no return c.json(...) statement, the HTTP request will fail or hang. All other endpoints in this file properly return responses via c.json().
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (6)
packages/typegen/web/src/App.tsx (2)
76-82: Missing error check onrunTypegenMutationresponse.The mutation doesn't check
res.ok, treating all HTTP responses (including 4xx/5xx errors) as success. This is inconsistent with other API calls in the codebase that properly check response status.Add response validation:
const runTypegenMutation = useMutation({ mutationFn: async () => { - await client.api.run.$post({ + const res = await client.api.run.$post({ json: { config: configs }, }); + + if (!res.ok) { + const errorData = (await res.json().catch(() => ({}))) as { + error?: string; + }; + throw new Error( + errorData.error || + `Failed to run typegen: ${res.status} ${res.statusText}`, + ); + } + + return res.json().catch(() => null); }, });
84-96:handleSaveAllswallows errors, allowinghandleRunTypegento proceed with stale config.The try-catch in
handleSaveAllcatches and logs errors but doesn't propagate them. This meanshandleRunTypegen(line 102) will always proceed even when saving fails, potentially running typegen with invalid or stale configuration.Remove the try-catch to allow errors to propagate:
const handleSaveAll = form.handleSubmit(async (data) => { - try { - await saveMutation.mutateAsync(data.config); - const currentConfigs = form.getValues("config"); - form.reset({ config: currentConfigs }); - } catch (err) { - // Error is handled by the mutation - console.error("Failed to save configs:", err); - } + await saveMutation.mutateAsync(data.config); + const currentConfigs = form.getValues("config"); + form.reset({ config: currentConfigs }); });packages/typegen/src/server/app.ts (2)
189-203: POST /api/run endpoint callsnext()but doesn't return a response.After calling
generateTypedClients, the handler invokesawait next()without returning a response. Since there's no subsequent middleware, this will cause the request to hang or return an empty response.Replace the
next()call with a proper response:.post( "/run", zValidator( "json", z.object({ config: z.union([z.array(typegenConfigSingle), typegenConfigSingle]), }), ), - async (c, next) => { + async (c) => { const data = c.req.valid("json"); const config = data.config; await generateTypedClients(config); - await next(); + return c.json({ success: true }); }, )
301-310: GET /api/env-names exposes arbitrary environment variables.The endpoint allows querying any environment variable by name without restriction, potentially exposing sensitive information like secrets, API keys, or internal configuration.
Implement an allowlist to restrict which environment variables can be queried:
.get( "/env-names", zValidator("query", z.object({ envName: z.string() })), async (c) => { const input = c.req.valid("query"); + + // Only allow querying known env var names + const allowedEnvNames = [ + "FM_SERVER", "FM_DATABASE", "OTTO_API_KEY", + "FM_USERNAME", "FM_PASSWORD" + ]; + if (!allowedEnvNames.includes(input.envName)) { + return c.json({ error: "Environment variable not allowed" }, 400); + } const value = process.env[input.envName]; return c.json({ value }); }, )packages/typegen/src/server/createDataApiClient.ts (1)
75-83: Remove credential logging to prevent security leak.As flagged in the previous review, console.error logs raw environment values including apiKey and username, which leaks sensitive credentials into stderr and log aggregators. The structured error object returned on lines 104-123 already provides safe boolean flags for missing variables.
Remove the console.error statement:
- if (!server || !db || (!apiKey && !username)) { - console.error("Missing required environment variables", { - server, - db, - apiKey, - username, - }); - + if (!server || !db || (!apiKey && !username)) { // Build missing details objectpackages/typegen/src/types.ts (1)
27-82: Auth field is still effectively required, breaking existing configs.As flagged in the previous review, the
authproperty on line 37 is not marked.optional(), making it a required field even though each variant uses.optional(). This means configs that omit theauthfield entirely will fail validation, which is a breaking change for existing configurations.Make the auth property optional:
.object({ server: z .string() .optional() .transform((val) => (val === "" ? undefined : val)), db: z .string() .optional() .transform((val) => (val === "" ? undefined : val)), - auth: z.union([ + auth: z.union([ z .object({ apiKey: z .string() .optional() .transform((val) => (val === "" ? undefined : val)), }) .optional() .transform((val) => { if (val && Object.values(val).every((v) => v === undefined)) { return undefined; } return val ?? undefined; }), z .object({ username: z .string() .optional() .transform((val) => (val === "" ? undefined : val)), password: z .string() .optional() .transform((val) => (val === "" ? undefined : val)), }) .optional() .transform((val) => { if (val && Object.values(val).every((v) => v === undefined)) { return undefined; } return val ?? undefined; }), - ]), + ]).optional(), })
🧹 Nitpick comments (9)
packages/typegen/web/src/components/theme-provider.tsx (2)
33-49: Add listener for system theme preference changes.The current implementation only checks the system theme preference once. If a user changes their system theme while the app is running with "system" selected, the UI won't update to reflect the change.
Apply this diff to add a listener:
useEffect(() => { const root = window.document.documentElement; root.classList.remove("light", "dark"); if (theme === "system") { const systemTheme = window.matchMedia("(prefers-color-scheme: dark)") .matches ? "dark" : "light"; root.classList.add(systemTheme); + + const mediaQuery = window.matchMedia("(prefers-color-scheme: dark)"); + const handleChange = (e: MediaQueryListEvent) => { + root.classList.remove("light", "dark"); + root.classList.add(e.matches ? "dark" : "light"); + }; + + mediaQuery.addEventListener("change", handleChange); + return () => mediaQuery.removeEventListener("change", handleChange); - return; } root.classList.add(theme); }, [theme]);
66-73: Context guard is technically unnecessary.The check
context === undefinedwill never be true sinceThemeProviderContextis always initialized withinitialState. However, this is a common defensive pattern and the error message is still useful for debugging, so it can remain as-is.packages/typegen/web/src/components/ui/switch-field.tsx (1)
57-68: Consider InfoTooltip placement for accessibility.The label association and description implementation are correct. However, the InfoTooltip is currently rendered inside the Label element (line 61), which could potentially cause accessibility issues if InfoTooltip contains interactive elements (nested interactive elements reduce accessibility).
Consider restructuring to place the InfoTooltip adjacent to the label rather than inside it:
- <Label htmlFor={switchId}> - {label} {infoTooltip && <InfoTooltip label={infoTooltip} />} - </Label> + <div className="flex items-center gap-1"> + <Label htmlFor={switchId}>{label}</Label> + {infoTooltip && <InfoTooltip label={infoTooltip} />} + </div>This change would only be necessary if InfoTooltip contains interactive elements like buttons or links.
packages/typegen/web/src/components/ConfigEditor.tsx (1)
50-56: Consider usinguseWatchconsistently for reactive form field subscriptions.The component mixes
watch(line 50) anduseWatch(lines 53-56) for reading form values. While both work, usinguseWatchconsistently provides better performance through granular subscriptions and avoids potential stale closure issues.Apply this diff for consistency:
const { control, formState: { errors }, setValue, - watch, } = useFormContext<{ config: SingleConfig[] }>(); const baseId = useId(); const generateClientSwitchId = `${baseId}-generate-client`; - const configType = watch(`config.${index}.type` as const); + const configType = useWatch({ + control, + name: `config.${index}.type` as const, + });packages/typegen/src/fmodata/downloadMetadata.ts (1)
54-54: Remove redundant return statement.The explicit
return;is unnecessary in aPromise<void>function.Apply this diff:
await mkdir(outputDir, { recursive: true }); await writeFile(resolvedPath, fullMetadata, "utf-8"); - - return; }packages/typegen/web/src/components/EnvVarDialog.tsx (1)
185-219: Consider extracting auth value preservation logic.The auth type switching logic (lines 189-212) contains repetitive nested conditionals to preserve existing values. This could be extracted into a helper function to improve readability and reduce duplication.
Example refactor:
const getAuthValue = (key: 'apiKey' | 'username' | 'password') => { return currentAuth && typeof currentAuth === "object" && key in currentAuth ? currentAuth[key] : ""; }; // Then use: if (value === "apiKey") { setValue(`config.${index}.envNames.auth` as const, { apiKey: getAuthValue('apiKey'), }); } else if (value === "username") { setValue(`config.${index}.envNames.auth` as const, { username: getAuthValue('username'), password: getAuthValue('password'), }); }packages/typegen/web/src/hooks/useTestConnection.ts (2)
59-65: Use structured query key instead of JSON.stringify.JSON.stringify is used to create a stable key for envNames, but this can be unreliable due to key ordering issues. React Query recommends using structured arrays for query keys, which are compared by deep equality.
Apply this refactor:
- // Create a stable key for envNames to use in queryKey - const envNamesKey = envNames - ? JSON.stringify({ - server: envNames.server, - db: envNames.db, - auth: envNames.auth, - }) - : ""; + // Use structured key for better React Query caching + const envNamesKey = envNames + ? { + server: envNames.server, + db: envNames.db, + auth: envNames.auth, + } + : null;Then update the query key:
- queryKey: ["testConnection", configIndex, envNamesKey], + queryKey: ["testConnection", configIndex, envNamesKey],React Query will handle deep equality comparison automatically.
86-95: Consider using a custom error class for type safety.The code attaches detailed error information to the error object using (error as any).details, which bypasses type safety. While functional, this could be improved with a custom error class.
Example custom error class:
class TestConnectionError extends Error { constructor( message: string, public details: TestConnectionResult ) { super(message); this.name = 'TestConnectionError'; } } // Then in queryFn: if (!res.ok) { const errorData = await res.json().catch(() => ({})) as TestConnectionResult; throw new TestConnectionError( errorData.error || errorData.message || "Test connection failed", errorData ); } // And in the return: const errorDetails: TestConnectionResult | undefined = isError && error instanceof TestConnectionError ? error.details : data && !data.ok ? data : undefined;packages/typegen/src/server/createDataApiClient.ts (1)
47-150: Eliminate code duplication with getEnvValues.ts.The
getEnvVarsFromConfigfunction duplicates logic frompackages/typegen/src/getEnvValues.ts, which providesgetEnvValuesandvalidateEnvValuesfunctions. This duplication increases maintenance burden and risk of inconsistencies.Refactor to use the existing utilities:
import { getEnvValues, validateEnvValues } from "../getEnvValues"; function getEnvVarsFromConfig( envNames: SingleConfig["envNames"], ): EnvVarsResult { const envValues = getEnvValues(envNames); const validation = validateEnvValues(envValues, envNames); if (!validation.success) { // Map the validation error to CreateClientError format return { error: validation.errorMessage, statusCode: 400, kind: "missing_env" as const, // ... map other fields from validation result }; } return { server: validation.server, db: validation.db, authType: "apiKey" in validation.auth ? "apiKey" : "username", auth: validation.auth, }; }This eliminates ~100 lines of duplicated logic and ensures consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/typegen/web/public/proofkit-horiz.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
packages/fmodata/src/client/filemaker-odata.ts(1 hunks)packages/fmodata/tests/tsconfig.build.json(1 hunks)packages/typegen/package.json(3 hunks)packages/typegen/proofkit-typegen.config.jsonc(1 hunks)packages/typegen/src/fmodata/downloadMetadata.ts(1 hunks)packages/typegen/src/fmodata/index.ts(1 hunks)packages/typegen/src/fmodata/parseMetadata.ts(1 hunks)packages/typegen/src/getEnvValues.ts(1 hunks)packages/typegen/src/server/app.ts(1 hunks)packages/typegen/src/server/createDataApiClient.ts(1 hunks)packages/typegen/src/typegen.ts(4 hunks)packages/typegen/src/types.ts(1 hunks)packages/typegen/tsconfig.json(1 hunks)packages/typegen/web/package.json(1 hunks)packages/typegen/web/src/App.tsx(1 hunks)packages/typegen/web/src/components/ConfigEditor.tsx(1 hunks)packages/typegen/web/src/components/ConfigList.css(1 hunks)packages/typegen/web/src/components/ConfigList.tsx(1 hunks)packages/typegen/web/src/components/ConfigSummary.tsx(1 hunks)packages/typegen/web/src/components/EnvVarDialog.tsx(1 hunks)packages/typegen/web/src/components/LayoutEditor.tsx(1 hunks)packages/typegen/web/src/components/dropdown-menu/default.tsx(1 hunks)packages/typegen/web/src/components/mode-toggle.tsx(1 hunks)packages/typegen/web/src/components/theme-provider.tsx(1 hunks)packages/typegen/web/src/components/ui/dropdown-menu.tsx(1 hunks)packages/typegen/web/src/components/ui/switch-field.tsx(1 hunks)packages/typegen/web/src/components/ui/tooltip.tsx(1 hunks)packages/typegen/web/src/components/useEnvVarIndicator.ts(1 hunks)packages/typegen/web/src/hooks/useConfig.ts(1 hunks)packages/typegen/web/src/hooks/useTestConnection.ts(1 hunks)packages/typegen/web/src/main.tsx(1 hunks)packages/typegen/web/tsconfig.app.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/typegen/web/src/components/ConfigSummary.tsx
- packages/typegen/web/tsconfig.app.json
- packages/typegen/web/package.json
- packages/typegen/web/src/components/ConfigList.css
- packages/typegen/web/src/components/ui/tooltip.tsx
- packages/typegen/proofkit-typegen.config.jsonc
- packages/typegen/web/src/main.tsx
- packages/typegen/web/src/components/useEnvVarIndicator.ts
- packages/typegen/web/src/hooks/useConfig.ts
- packages/typegen/web/src/components/LayoutEditor.tsx
- packages/typegen/web/src/components/ConfigList.tsx
- packages/fmodata/tests/tsconfig.build.json
🧰 Additional context used
🧬 Code graph analysis (10)
packages/typegen/src/fmodata/downloadMetadata.ts (2)
packages/typegen/src/types.ts (1)
typegenConfigSingle(137-142)packages/typegen/src/getEnvValues.ts (2)
getEnvValues(35-63)validateEnvValues(73-125)
packages/typegen/web/src/components/mode-toggle.tsx (3)
packages/typegen/web/src/components/theme-provider.tsx (1)
useTheme(66-73)packages/typegen/web/src/components/ui/dropdown-menu.tsx (4)
DropdownMenu(209-209)DropdownMenuTrigger(223-223)DropdownMenuContent(211-211)DropdownMenuItem(213-213)packages/registry/templates/components/mode-toggle/mode-toggle.tsx (1)
ModeToggle(14-39)
packages/typegen/src/typegen.ts (3)
packages/typegen/src/types.ts (1)
typegenConfigSingle(137-142)packages/typegen/web/src/lib/schema.ts (1)
typegenConfigSingle(3-3)packages/typegen/src/getEnvValues.ts (2)
getEnvValues(35-63)validateAndLogEnvValues(135-179)
packages/typegen/src/getEnvValues.ts (2)
packages/typegen/src/types.ts (1)
typegenConfigSingle(137-142)packages/fmodata/tests/e2e/setup.ts (3)
apiKey(21-21)username(22-22)password(23-23)
packages/typegen/src/server/createDataApiClient.ts (4)
packages/typegen/src/types.ts (2)
typegenConfigSingle(137-142)typegenConfig(144-146)packages/typegen/src/server/contract.ts (1)
SingleConfig(6-6)packages/fmodata/src/client/filemaker-odata.ts (1)
FMServerConnection(22-304)packages/typegen/src/server/app.ts (1)
ApiContext(19-22)
packages/typegen/web/src/hooks/useTestConnection.ts (3)
packages/typegen/src/server/contract.ts (1)
SingleConfig(6-6)packages/typegen/web/src/lib/config-utils.ts (1)
SingleConfig(4-4)packages/typegen/web/src/lib/api.ts (1)
client(7-7)
packages/typegen/src/server/app.ts (4)
packages/fmdapi/src/client-types.ts (2)
LayoutOrFolder(262-262)FileMakerError(289-296)packages/typegen/src/types.ts (2)
typegenConfig(144-146)typegenConfigSingle(137-142)packages/typegen/src/typegen.ts (1)
generateTypedClients(29-58)packages/typegen/src/server/createDataApiClient.ts (3)
createDataApiClient(251-310)createClientFromConfig(195-243)createOdataClientFromConfig(167-188)
packages/typegen/src/types.ts (2)
apps/docs/scripts/bundle-registry-templates.js (1)
path(4-4)packages/typegen/web/src/lib/schema.ts (1)
typegenConfigSingle(3-3)
packages/typegen/web/src/App.tsx (5)
packages/typegen/web/src/lib/config-utils.ts (1)
SingleConfig(4-4)packages/typegen/web/src/hooks/useConfig.ts (1)
useConfig(5-56)packages/typegen/web/src/lib/api.ts (1)
client(7-7)packages/typegen/web/src/components/ConfigSummary.tsx (1)
ConfigSummary(5-36)packages/typegen/web/src/components/ConfigEditor.tsx (1)
ConfigEditor(40-328)
packages/typegen/web/src/components/EnvVarDialog.tsx (8)
packages/typegen/web/src/hooks/useTestConnection.ts (2)
setDialogOpen(11-17)useTestConnection(44-140)packages/typegen/web/src/components/useEnvVarIndicator.ts (1)
useEnvVarIndicator(6-81)packages/typegen/web/src/lib/envValues.ts (1)
useEnvValue(4-20)packages/typegen/web/src/components/ui/dialog.tsx (4)
Dialog(128-128)DialogTrigger(138-138)DialogContent(131-131)DialogTitle(137-137)packages/typegen/web/src/components/ui/button.tsx (1)
Button(423-423)packages/typegen/web/src/components/ServerEnvField.tsx (1)
EnvVarField(25-70)packages/typegen/web/src/components/EnvVarField.tsx (1)
EnvVarField(115-139)packages/typegen/web/src/components/ui/select.tsx (5)
Select(224-224)SelectTrigger(233-233)SelectValue(234-234)SelectContent(225-225)SelectItem(228-228)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Cursor Bugbot
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (16)
packages/typegen/web/src/components/dropdown-menu/default.tsx (1)
35-158: LGTM! Comprehensive dropdown demo.This demo effectively showcases the dropdown menu component capabilities including nested submenus, keyboard shortcuts, badges, and various menu item states. The structure is clear and serves as a good reference for component usage.
packages/typegen/web/src/components/mode-toggle.tsx (1)
12-37: LGTM! Clean theme toggle implementation.The component correctly integrates with the theme provider and follows the standard pattern for theme toggles with smooth icon transitions. The implementation aligns with the registry template.
packages/typegen/web/src/components/ui/switch-field.tsx (5)
1-7: LGTM: Clean import structure.The imports are well-organized and appropriate for a client-side React component with UI primitives and utilities.
9-19: LGTM: Well-defined props interface.The props interface is comprehensive with appropriate types. The size prop uses string literals for type safety, and optional props are clearly marked.
21-34: Excellent ID generation pattern.The use of
useId()with a fallback to a provided ID is a React best practice for generating stable, unique identifiers. The deriveddescriptionIdproperly supports the accessibility implementation below.
36-45: LGTM: Well-structured container with conditional styling.The container appropriately adjusts layout based on description presence and provides good visual feedback with the border color change when checked.
46-56: Switch component props verification complete.The
sizeprop is properly supported by the Switch component via its CVA variants (sm,md,lg,xl). The accessibility implementation witharia-describedbyis correctly implemented.The
className="order-1"on the Switch is ineffective since it's the only child of the flex container—theinline-flexin SwitchWrapper only applies ordering rules when there are multiple flex siblings. This can optionally be removed.packages/typegen/web/src/components/ConfigEditor.tsx (1)
48-50: LGTM: Hook violation from previous review has been correctly resolved.The
useIdhook is now called at the top level of the component, and IDs are derived from the base ID, properly addressing the Rules of Hooks violation flagged in the previous review.packages/typegen/package.json (1)
31-42: LGTM: Export paths correctly distinguish API contract from app implementation.The
./apiand./api-appexports now correctly point to separate implementations (contract.js vs app.js), properly addressing the duplicate export concern from the previous review.packages/typegen/src/typegen.ts (2)
49-57: LGTM: Proper type narrowing and graceful handling of unsupported config types.The code correctly normalizes config to an array and uses type guards to process only
fmdapiconfigurations while warning about unsupported types. This aligns well with the narrowed function signature at line 61.
101-112: LGTM: Clean environment validation flow with proper error handling.The refactored validation flow using
getEnvValuesandvalidateAndLogEnvValuesprovides clear separation of concerns and proper early-exit on validation failure.packages/typegen/src/fmodata/index.ts (1)
1-11: LGTM: Clean barrel export pattern for fmodata module.The file provides a consolidated public API surface for metadata handling functionality, properly re-exporting functions and types.
packages/typegen/src/fmodata/parseMetadata.ts (1)
60-66: LGTM: Well-configured XML parser for OData metadata.The parser configuration appropriately handles attributes, auto-parses values, and uses a prefix to avoid naming collisions. The boolean parsing throughout the file correctly handles both string and boolean representations.
packages/typegen/src/getEnvValues.ts (2)
35-63: LGTM!The environment variable resolution logic correctly handles custom env names with fallback to defaults, supporting both API key and username/password authentication patterns.
73-125: LGTM!The validation logic correctly identifies missing environment variables and constructs clear error messages. The defensive
?? ""fallback on lines 117 is acceptable even though validation ensures these values exist, as it provides additional type safety.packages/typegen/src/types.ts (1)
90-142: LGTM!The z.preprocess wrapper adds a default type: "fmdapi" for backward compatibility, which properly handles existing configs that don't specify a type field. The discriminated union structure is well-designed.
| /* Include ES2022 lib for ErrorOptions and Error.cause support */ | ||
| "lib": ["DOM", "DOM.Iterable", "ES2021", "ES2022"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if packages/typegen exists and what files it contains
echo "=== Checking packages/typegen directory ==="
if [ -d "packages/typegen" ]; then
echo "Directory exists. Listing contents:"
ls -la packages/typegen/ | head -20
# Check package.json
echo -e "\n=== packages/typegen/package.json ==="
if [ -f "packages/typegen/package.json" ]; then
cat packages/typegen/package.json
fi
# Check tsconfig.json
echo -e "\n=== packages/typegen/tsconfig.json ==="
if [ -f "packages/typegen/tsconfig.json" ]; then
cat packages/typegen/tsconfig.json
fi
# Check if src directory exists
echo -e "\n=== Checking packages/typegen/src ==="
if [ -d "packages/typegen/src" ]; then
echo "Source directory exists. Files:"
find packages/typegen/src -type f -name "*.ts" -o -name "*.tsx" | head -20
fi
else
echo "packages/typegen directory not found"
echo "Available packages:"
ls -d packages/*/ 2>/dev/null | head -20
fiRepository: proofgeist/proofkit
Length of output: 5377
🏁 Script executed:
#!/bin/bash
# Search for DOM API usage in src directory
echo "=== Searching for DOM API usage in packages/typegen/src ==="
rg -n --type=ts '(document\.|window\.|HTMLElement|navigator\.|localStorage|sessionStorage|fetch\(|addEventListener|DOM|document\.querySelector|document\.getElementById)' packages/typegen/src/
echo -e "\n=== Checking if web/ has its own tsconfig.json ==="
if [ -f "packages/typegen/web/tsconfig.json" ]; then
echo "web/tsconfig.json exists:"
cat packages/typegen/web/tsconfig.json
else
echo "web/tsconfig.json does not exist"
ls -la packages/typegen/web/ 2>/dev/null || echo "web/ directory not found"
fi
echo -e "\n=== Checking if vite.config.ts uses DOM or browser APIs ==="
head -50 packages/typegen/vite.config.tsRepository: proofgeist/proofkit
Length of output: 1467
Remove unnecessary DOM types from server-side TypeScript configuration.
The main packages/typegen/tsconfig.json is used for server-side code and contains no DOM API usage. DOM types should only be included in the web UI's separate tsconfig.json (which they already are). The ES2022 addition for Error.cause support is appropriate, but DOM and DOM.Iterable should be removed:
Change:
"lib": ["DOM", "DOM.Iterable", "ES2021", "ES2022"],
To:
"lib": ["ES2022"],
🤖 Prompt for AI Agents
In packages/typegen/tsconfig.json around lines 9 to 10, the tsconfig
unnecessarily includes DOM and DOM.Iterable which are client-side types; remove
those and simplify the lib entry to only include the needed ES2022 lib (for
Error.cause) by replacing the current lib array with a single "ES2022" entry so
server-side compilation no longer pulls in DOM types.
| const { control, setValue, getValues } = useFormContext<{ | ||
| config: any[]; | ||
| }>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any[] with proper type.
The form context uses any[] for the config array, which bypasses type safety. Use the proper SingleConfig[] type imported from config-utils (line 4) to ensure type safety throughout the component.
Apply this diff:
- const { control, setValue, getValues } = useFormContext<{
- config: any[];
- }>();
+ const { control, setValue, getValues } = useFormContext<{
+ config: SingleConfig[];
+ }>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { control, setValue, getValues } = useFormContext<{ | |
| config: any[]; | |
| }>(); | |
| const { control, setValue, getValues } = useFormContext<{ | |
| config: SingleConfig[]; | |
| }>(); |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/EnvVarDialog.tsx around lines 37 to 39,
the useFormContext generic currently uses `any[]` for `config`; replace it with
the proper `SingleConfig[]` type (imported from `config-utils` on line 4) so the
line becomes useFormContext<{ config: SingleConfig[] }>(), and update any
related local typings if necessary to match `SingleConfig` fields to restore
full type safety.
| useEffect(() => { | ||
| let authSelector: "none" | "apiKey" | "username" = "apiKey"; | ||
|
|
||
| if (envNamesAuth) { | ||
| if (typeof envNamesAuth === "object") { | ||
| // Check for username first (since it has two fields, it's more specific) | ||
| if ("username" in envNamesAuth || "password" in envNamesAuth) { | ||
| authSelector = "username"; | ||
| } else if ("apiKey" in envNamesAuth) { | ||
| authSelector = "apiKey"; | ||
| } | ||
| // If it's an empty object {}, don't change the selector or reset values | ||
| // This preserves the current state when the server returns {} | ||
| } | ||
| } else { | ||
| // Only initialize if auth is truly undefined/null | ||
| // Check current form value to avoid overwriting | ||
| const currentAuth = getValues(`config.${index}.envNames.auth` as any); | ||
| if (!currentAuth) { | ||
| setValue(`config.${index}.envNames.auth` as const, { | ||
| apiKey: "", | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Only update selector if it's different to avoid unnecessary re-renders | ||
| if (authSelector !== authTypeSelector) { | ||
| setAuthTypeSelector(authSelector); | ||
| } | ||
| }, [envNamesAuth, setValue, getValues, index, authTypeSelector]); |
There was a problem hiding this comment.
Risk of infinite loop due to state in effect dependencies.
The effect depends on authTypeSelector (line 129) and conditionally updates it via setAuthTypeSelector (line 127). While the guard on line 126 prevents updates when unchanged, this pattern is fragile and can lead to infinite loops if the comparison logic changes or if React's batching behavior changes.
Apply this refactor to remove the circular dependency:
- }, [envNamesAuth, setValue, getValues, index, authTypeSelector]);
+ }, [envNamesAuth, setValue, getValues, index]);The effect will still work correctly because it derives authSelector from envNamesAuth and only updates the state when needed. Removing authTypeSelector from dependencies eliminates the circular dependency risk.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| let authSelector: "none" | "apiKey" | "username" = "apiKey"; | |
| if (envNamesAuth) { | |
| if (typeof envNamesAuth === "object") { | |
| // Check for username first (since it has two fields, it's more specific) | |
| if ("username" in envNamesAuth || "password" in envNamesAuth) { | |
| authSelector = "username"; | |
| } else if ("apiKey" in envNamesAuth) { | |
| authSelector = "apiKey"; | |
| } | |
| // If it's an empty object {}, don't change the selector or reset values | |
| // This preserves the current state when the server returns {} | |
| } | |
| } else { | |
| // Only initialize if auth is truly undefined/null | |
| // Check current form value to avoid overwriting | |
| const currentAuth = getValues(`config.${index}.envNames.auth` as any); | |
| if (!currentAuth) { | |
| setValue(`config.${index}.envNames.auth` as const, { | |
| apiKey: "", | |
| }); | |
| } | |
| } | |
| // Only update selector if it's different to avoid unnecessary re-renders | |
| if (authSelector !== authTypeSelector) { | |
| setAuthTypeSelector(authSelector); | |
| } | |
| }, [envNamesAuth, setValue, getValues, index, authTypeSelector]); | |
| useEffect(() => { | |
| let authSelector: "none" | "apiKey" | "username" = "apiKey"; | |
| if (envNamesAuth) { | |
| if (typeof envNamesAuth === "object") { | |
| // Check for username first (since it has two fields, it's more specific) | |
| if ("username" in envNamesAuth || "password" in envNamesAuth) { | |
| authSelector = "username"; | |
| } else if ("apiKey" in envNamesAuth) { | |
| authSelector = "apiKey"; | |
| } | |
| // If it's an empty object {}, don't change the selector or reset values | |
| // This preserves the current state when the server returns {} | |
| } | |
| } else { | |
| // Only initialize if auth is truly undefined/null | |
| // Check current form value to avoid overwriting | |
| const currentAuth = getValues(`config.${index}.envNames.auth` as any); | |
| if (!currentAuth) { | |
| setValue(`config.${index}.envNames.auth` as const, { | |
| apiKey: "", | |
| }); | |
| } | |
| } | |
| // Only update selector if it's different to avoid unnecessary re-renders | |
| if (authSelector !== authTypeSelector) { | |
| setAuthTypeSelector(authSelector); | |
| } | |
| }, [envNamesAuth, setValue, getValues, index]); |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/EnvVarDialog.tsx around lines 100 to 129,
the useEffect currently lists authTypeSelector in its dependency array and
conditionally calls setAuthTypeSelector inside the effect, creating a fragile
circular dependency; remove authTypeSelector from the dependency array so the
effect only depends on envNamesAuth, setValue, getValues, and index, and keep
the existing guard that only calls setAuthTypeSelector when authSelector !==
authTypeSelector to avoid unnecessary updates; ensure setAuthTypeSelector is the
same stable setter (no further changes needed) and run tests to confirm no
regressions.
| const [theme, setTheme] = useState<Theme>( | ||
| () => (localStorage.getItem(storageKey) as Theme) || defaultTheme | ||
| ); |
There was a problem hiding this comment.
Validate localStorage value before using.
The localStorage value is cast directly to Theme without validation. If the stored value is corrupted or invalid, this could result in unexpected behavior.
Apply this diff to add validation:
const [theme, setTheme] = useState<Theme>(
- () => (localStorage.getItem(storageKey) as Theme) || defaultTheme
+ () => {
+ const stored = localStorage.getItem(storageKey) as Theme;
+ if (stored === "dark" || stored === "light" || stored === "system") {
+ return stored;
+ }
+ return defaultTheme;
+ }
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [theme, setTheme] = useState<Theme>( | |
| () => (localStorage.getItem(storageKey) as Theme) || defaultTheme | |
| ); | |
| const [theme, setTheme] = useState<Theme>( | |
| () => { | |
| const stored = localStorage.getItem(storageKey) as Theme; | |
| if (stored === "dark" || stored === "light" || stored === "system") { | |
| return stored; | |
| } | |
| return defaultTheme; | |
| } | |
| ); |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/theme-provider.tsx around lines 29-31,
the useState initializer casts localStorage.getItem(storageKey) to Theme without
validation; replace this with code that reads the raw string, checks it against
the allowed Theme values (e.g., Object.values(Theme) or a small whitelist), and
returns that value only if valid, otherwise return defaultTheme; also guard
against exceptions (wrap read in try/catch) and avoid direct casting so
corrupted or unexpected values fall back to default.
| setTheme: (theme: Theme) => { | ||
| localStorage.setItem(storageKey, theme); | ||
| setTheme(theme); | ||
| }, |
There was a problem hiding this comment.
Add error handling for localStorage operations.
The localStorage.setItem call can throw errors in environments where localStorage is unavailable (private browsing mode), full, or restricted. This could crash the theme toggle functionality.
Apply this diff to add error handling:
setTheme: (theme: Theme) => {
- localStorage.setItem(storageKey, theme);
+ try {
+ localStorage.setItem(storageKey, theme);
+ } catch (error) {
+ console.warn("Failed to save theme preference:", error);
+ }
setTheme(theme);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setTheme: (theme: Theme) => { | |
| localStorage.setItem(storageKey, theme); | |
| setTheme(theme); | |
| }, | |
| setTheme: (theme: Theme) => { | |
| try { | |
| localStorage.setItem(storageKey, theme); | |
| } catch (error) { | |
| console.warn("Failed to save theme preference:", error); | |
| } | |
| setTheme(theme); | |
| }, |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/theme-provider.tsx around lines 53 to 56,
the direct call to localStorage.setItem may throw and crash the theme toggle;
wrap the storage operation in a try/catch (or guard with typeof window !==
'undefined' && window.localStorage) so failures don’t propagate, log or report
the error in the catch, and always call the React setTheme(state updater) so the
UI still updates even if persisting to localStorage fails; keep behavior
minimal—don’t rethrow, optionally fall back to an in-memory flag if persistence
is required.
- Updated typegen configuration to support OData clients, allowing for better integration and client generation. - Improved the ConfigEditor component with new fields for metadata path and auto-download options. - Refactored layout and switch components for better usability and added tooltips for clarity. - Enhanced error handling and logging in the type generation process. - Updated UI components to improve user experience, including better state management and additional icons.
- Added support for downloading and parsing metadata in the ConfigEditor component, allowing users to manage OData configurations more effectively. - Updated typegen configuration to ensure metadata paths point to .xml files, improving validation and user feedback. - Introduced new API endpoints for downloading and parsing metadata, enhancing the overall functionality of the typegen server. - Improved UI components with additional state management for download actions and better user experience through tooltips and loading indicators. - Updated dependencies for @dnd-kit and @TanStack libraries to the latest versions for improved performance and features.
…tion testing - Introduced a new state to control loading of tables based on configuration. - Added connection testing logic to display appropriate messages for connection errors. - Updated useListTables hook to accept an enabled parameter for conditional querying. - Improved loading and error handling UI to provide better user feedback.
…ction - Added support for `reduceMetadata` option in the typegen configuration for `fmdapi_test` table, allowing for field-specific overrides. - Updated the `ConfigEditor` component to include a toggle for reducing metadata annotations. - Improved `MetadataFieldsDialog` to manage variable name and reduce metadata settings for individual tables. - Refactored `generateODataTablesSingle` to utilize the new `reduceMetadata` configuration.
- Deleted unused OData schema files: contacts, customer, related, user_customer, users, fmdapi_test, isolated_contacts, and their corresponding index entries. - Updated typegen configuration to include `alwaysOverrideFieldNames` option, allowing for more flexible field name management during schema generation. - Improved handling of existing fields in `generateODataTypes` to preserve user customizations and manage field name updates based on metadata.
This commit refactors the logic for preserving user-defined chained calls in generated types. It also updates dependencies in pnpm-lock.yaml and adds a new test file for preserving user customizations. Co-authored-by: eric.luce <eric.luce@proofgeist.com>
...luce/Documents/Code/work/proofkit/apps/demo/tests/typegen-output/without-zod/client/index.ts
Show resolved
Hide resolved
| validator = z.number(); | ||
| break; | ||
| default: | ||
| validator = z.unknown(); |
There was a problem hiding this comment.
Bug: Schema only stores explicit validators, breaking expand default select
The refactored code in fmTableOccurrence now only populates outputSchema with entries for fields that have explicit outputValidator set. Previously, default validators were auto-generated for every field. This breaks getDefaultSelectFields() which uses Object.keys(baseTableConfig.schema) to enumerate field names when defaultSelect === "schema". For tables without explicit validators, this returns an empty array, causing expand operations to have empty $select clauses. While the main query path in entity-set.ts uses getTableColumns() (which still works), the expand-builder.ts relies on the now-broken getDefaultSelectFields().
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Adds
packages/typegen/webtopnpm-workspace.yamlpackage list.Written by Cursor Bugbot for commit bd77c9a. This will update automatically on new commits. Configure here.