Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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. 📝 WalkthroughWalkthroughAdds 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, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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.
![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.
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (4)
packages/typegen/src/server/app.ts (2)
190-196: POST /api/run endpoint callsnext()but doesn't return a response.The
/runhandler completesgenerateTypedClientsthen callsawait next()without returning anything. Since there's no subsequent middleware, this will cause the request to hang or return an empty response.- 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 }); },
293-304: GET /api/env-names exposes arbitrary environment variables.This endpoint allows querying any environment variable by name, which could leak secrets, API keys, or sensitive configuration if an attacker discovers the endpoint.
Consider restricting to an allowlist of known env var names:
.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)
77-82: Avoid logging raw credentials in the missing-env error path.The
console.errorcall logs actual values forserver,db,apiKey, andusername, which leaks sensitive credentials to stderr and log aggregators.Remove or redact the sensitive values:
- console.error("Missing required environment variables", { - server, - db, - apiKey, - username, - }); + console.error("Missing required environment variables", { + server: !!server, + db: !!db, + apiKey: !!apiKey, + username: !!username, + });packages/typegen/src/types.ts (1)
37-70:envNames.authis effectively required, breaking configs without auth key.The
authproperty itself is not optional in the object schema. While each auth variant has.optional(), the property key is required, so configs like:{ "envNames": { "server": "FM_HOST", "db": "FM_DB" // no "auth" key } }will fail validation.
Wrap the union in
.optional():- auth: z.union([ + auth: z.union([ // ... existing variants - ]), + ]).optional(),
🟡 Minor comments (11)
packages/typegen/web/src/components/ui/sheet.tsx-38-40 (1)
38-40: Fix typo:items-strech→items-stretch.The Tailwind class
items-strechis misspelled and won't be applied.const sheetVariants = cva( - 'flex flex-col items-strech fixed z-50 gap-4 bg-background p-6 shadow-lg transition ease-in-out data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:duration-300 data-[state=open]:duration-400', + 'flex flex-col items-stretch fixed z-50 gap-4 bg-background p-6 shadow-lg transition ease-in-out data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:duration-300 data-[state=open]:duration-400', {packages/typegen/web/src/components/input/icon.tsx-23-23 (1)
23-23: Fix typo in placeholder text."clickble" should be "clickable".
- <Input type="email" placeholder="End clickble icon" /> + <Input type="email" placeholder="End clickable icon" />packages/typegen/web/src/components/input/icon.tsx-20-20 (1)
20-20: Fix typo in placeholder text."clickble" should be "clickable".
- <Input type="email" placeholder="Start clickble icon" /> + <Input type="email" placeholder="Start clickable icon" />packages/typegen/web/src/components/ui/button.tsx-8-8 (1)
8-8: Remove duplicate class name.The
whitespace-nowrapclass appears twice in the base classes string.Apply this diff:
- "cursor-pointer group whitespace-nowrap focus-visible:outline-hidden inline-flex items-center justify-center has-data-[arrow=true]:justify-between whitespace-nowrap text-sm font-medium ring-offset-background transition-[color,box-shadow] disabled:pointer-events-none disabled:opacity-60 [&_svg]:shrink-0", + "cursor-pointer group whitespace-nowrap focus-visible:outline-hidden inline-flex items-center justify-center has-data-[arrow=true]:justify-between text-sm font-medium ring-offset-background transition-[color,box-shadow] disabled:pointer-events-none disabled:opacity-60 [&_svg]:shrink-0",packages/typegen/web/src/components/ui/data-grid-table-dnd.tsx-115-115 (1)
115-115: Remove debugconsole.logstatement.This appears to be a development artifact that should be removed before merging.
{table.getHeaderGroups().map((headerGroup: HeaderGroup<TData>, index) => { - console.log('table.getState().columnOrder:', table.getState().columnOrder); - return (packages/typegen/web/src/components/ui/data-grid-table.tsx-278-291 (1)
278-291: Typo in component name:DataGridTableBodyRowExpandded.The component name has a typo with double 'd' - should be
DataGridTableBodyRowExpanded. This affects the component definition (line 278), usage (line 493), and export (line 511).-function DataGridTableBodyRowExpandded<TData>({ row }: { row: Row<TData> }) { +function DataGridTableBodyRowExpanded<TData>({ row }: { row: Row<TData> }) {And update the export:
- DataGridTableBodyRowExpandded, + DataGridTableBodyRowExpanded,Also applies to: 493-493, 505-523
packages/typegen/web/src/components/ui/data-grid-table.tsx-229-231 (1)
229-231: Stray quote at end of className string.Line 231 has a trailing quote that will be included in the className, potentially causing CSS issues:
data-pinned:bg-background/90 data-pinned:backdrop-blur-xs"'props.tableLayout?.columnsPinnable && column.getCanPin() && - '[&[data-pinned=left][data-last-col=left]]:border-e! [&[data-pinned=right][data-last-col=right]]:border-s! [&[data-pinned][data-last-col]]:border-border data-pinned:bg-background/90 data-pinned:backdrop-blur-xs"', + '[&[data-pinned=left][data-last-col=left]]:border-e! [&[data-pinned=right][data-last-col=right]]:border-s! [&[data-pinned][data-last-col]]:border-border data-pinned:bg-background/90 data-pinned:backdrop-blur-xs',packages/typegen/web/src/components/ui/data-grid.tsx-59-65 (1)
59-65: Typo:strippedshould bestriped.The property name
strippedat line 64 should likely bestriped, which is the standard term for alternating row colors in tables.This typo appears in multiple places across related files (data-grid-table.tsx uses the same spelling).
tableLayout?: { dense?: boolean; cellBorder?: boolean; rowBorder?: boolean; rowRounded?: boolean; - stripped?: boolean; + striped?: boolean; headerBackground?: boolean;packages/typegen/web/src/components/ui/data-grid-table.tsx-331-333 (1)
331-333: Same stray quote issue in DataGridTableBodyRowCell.The same trailing quote issue exists here at line 333.
props.tableLayout?.columnsPinnable && column.getCanPin() && - '[&[data-pinned=left][data-last-col=left]]:border-e! [&[data-pinned=right][data-last-col=right]]:border-s! [&[data-pinned][data-last-col]]:border-border data-pinned:bg-background/90 data-pinned:backdrop-blur-xs"', + '[&[data-pinned=left][data-last-col=left]]:border-e! [&[data-pinned=right][data-last-col=right]]:border-s! [&[data-pinned][data-last-col]]:border-border data-pinned:bg-background/90 data-pinned:backdrop-blur-xs',packages/typegen/web/src/components/ui/data-grid-table.tsx-314-322 (1)
314-322: Unusual prop spreading ontdelement.Line 318 spreads
{ cell }onto thetdelement whencolumnsDraggableis enabled. Sincecellis not a valid HTML attribute, this will result in React warnings about unknown DOM properties.If this is intended for drag-and-drop functionality via data attributes, consider using a data attribute instead:
- {...(props.tableLayout?.columnsDraggable && !isPinned ? { cell } : {})} + data-cell={props.tableLayout?.columnsDraggable && !isPinned ? cell.id : undefined}Or if the cell reference is needed for DnD library integration, pass it through a
data-*attribute or handle it differently to avoid DOM warnings.Committable suggestion skipped: line range outside the PR's diff.
packages/typegen/src/typegen.ts-166-167 (1)
166-167: Theconfig?.type === "fmdapi"check is indeed redundant and should be removed.Since
generateTypedClientsSingleis typed withExtract<z.infer<typeof typegenConfigSingle>, { type: "fmdapi" }>, TypeScript guarantees thatconfig.typeis always"fmdapi". The condition is always true, so the code can be simplified to:webviewerScriptName: config.webviewerScriptName,
🧹 Nitpick comments (30)
.gitignore (1)
1-75: Clean up pre‑existing duplicate ignore rules.The
.gitignorefile contains significant duplication across lines 58–75 (e.g.,.DS_Store,.turbo,*.tsbuildinfo,dist,node_modules,.next,.envappear multiple times). While this doesn't block the current PR, consolidating these would improve maintainability. Consider removing the duplicate section and keeping only the primary rules.packages/typegen/web/src/components/ui/sheet.tsx (1)
64-71: Simplify redundant type intersection.
SheetContentPropsalready extendsReact.ComponentProps<typeof SheetPrimitive.Content>, so the intersection on the function parameter is redundant.function SheetContent({ side = 'right', overlay = true, close = true, className, children, ...props -}: React.ComponentProps<typeof SheetPrimitive.Content> & SheetContentProps) { +}: SheetContentProps) {packages/typegen/web/src/components/ui/checkbox.tsx (1)
10-17: Consider removing the trailing comma after the template literal.Line 15 has a trailing comma immediately after the template literal string, which is syntactically unusual. While valid JavaScript, this appears to be a formatting artifact.
Apply this diff to clean up the syntax:
aria-invalid:border-destructive/60 aria-invalid:ring-destructive/10 dark:aria-invalid:border-destructive dark:aria-invalid:ring-destructive/20 [[data-invalid=true]_&]:border-destructive/60 [[data-invalid=true]_&]:ring-destructive/10 dark:[[data-invalid=true]_&]:border-destructive dark:[[data-invalid=true]_&]:ring-destructive/20, data-[state=checked]:bg-primary data-[state=checked]:border-primary data-[state=checked]:text-primary-foreground data-[state=indeterminate]:bg-primary data-[state=indeterminate]:border-primary data-[state=indeterminate]:text-primary-foreground - `, + `,packages/typegen/web/src/components/input/icon.tsx (1)
17-19: Add accessible labels to icon-only buttons.Icon-only buttons should include
aria-labelattributes to ensure screen reader users understand their purpose.- <Button size="sm" variant="dim" mode="icon" className="size-5 -ms-0.5"> + <Button size="sm" variant="dim" mode="icon" className="size-5 -ms-0.5" aria-label="User icon"> <User /> </Button>- <Button size="sm" variant="dim" mode="icon" className="size-5 -me-0.5"> + <Button size="sm" variant="dim" mode="icon" className="size-5 -me-0.5" aria-label="Clear"> <X /> </Button>Also applies to: 24-26
packages/typegen/web/src/components/ui/button.tsx (2)
415-457: Consider defaultingtypeto"button"to prevent unintended form submissions.The Button component doesn't set a default
type="button". When rendered as a<button>inside a form, it defaults totype="submit", which can cause unexpected form submissions.Apply this diff to add a safe default:
function Button({ className, selected, variant, radius, appearance, mode, size, autoHeight, underlined, underline, asChild = false, placeholder = false, + type = "button", ...props }: React.ComponentProps<"button"> & VariantProps<typeof buttonVariants> & { selected?: boolean; asChild?: boolean; }) { const Comp = asChild ? SlotPrimitive.Slot : "button"; return ( <Comp data-slot="button" className={cn( buttonVariants({ variant, size, radius, appearance, mode, autoHeight, placeholder, underlined, underline, className, }), asChild && props.disabled && "pointer-events-none opacity-50", )} {...(selected && { "data-state": "open" })} + {...(!asChild && { type })} {...props} /> ); }Note: Only apply
typewhen not usingasChild, as the slotted element may not be a button.
453-453: Document theselectedprop behavior.The
selectedprop manually setsdata-state="open", which may conflict with Radix UI components (Popover, DropdownMenu, etc.) that manage this attribute internally. Ensure this prop is only used for standalone buttons or document the precedence behavior.packages/typegen/web/src/components/ui/data-grid-table-dnd-rows.tsx (1)
61-67: Consider usingcell.idas the key instead ofcolIndex.Using array index as React key can cause reconciliation issues if columns are reordered or filtered. The cell already has a stable
idproperty.- <DataGridTableBodyRowCell cell={cell} key={colIndex}> + <DataGridTableBodyRowCell cell={cell} key={cell.id}>packages/typegen/web/src/components/ui/data-grid-table-dnd.tsx (1)
147-163: InefficientSortableContextusage and inconsistent keys.Two issues here:
SortableContext per cell: Creating a new
SortableContextfor each cell is unnecessary overhead. Since all cells in a row share the same column order, wrap the cells once at the row level.Inconsistent keys: The
Fragmentusesrow.idas key, but the innerDataGridTableBodyRowusesindex. Remove the redundant key on the inner element.<Fragment key={row.id}> - <DataGridTableBodyRow row={row} key={index}> - {row.getVisibleCells().map((cell: Cell<TData, unknown>) => { - return ( - <SortableContext - key={cell.id} - items={table.getState().columnOrder} - strategy={horizontalListSortingStrategy} - > - <DataGridTableDndCell cell={cell} /> - </SortableContext> - ); - })} - </DataGridTableBodyRow> + <DataGridTableBodyRow row={row}> + <SortableContext + items={table.getState().columnOrder} + strategy={horizontalListSortingStrategy} + > + {row.getVisibleCells().map((cell: Cell<TData, unknown>) => ( + <DataGridTableDndCell cell={cell} key={cell.id} /> + ))} + </SortableContext> + </DataGridTableBodyRow> {row.getIsExpanded() && <DataGridTableBodyRowExpandded row={row} />} </Fragment>packages/typegen/web/src/components/ui/data-grid-pagination.tsx (1)
50-60: Edge case: pagination info shows "1 - 0 of 0" when data is empty.When
recordCountis 0,fromcalculates to 1 andtocalculates to 0, resulting in "1 - 0 of 0". Consider handling this edge case for better UX.const from = pageIndex * pageSize + 1; const to = Math.min((pageIndex + 1) * pageSize, recordCount); + const displayFrom = recordCount === 0 ? 0 : from; const pageCount = table.getPageCount(); // Replace placeholders in paginationInfo const paginationInfo = mergedProps?.info ? mergedProps.info - .replace('{from}', from.toString()) + .replace('{from}', displayFrom.toString()) .replace('{to}', to.toString()) .replace('{count}', recordCount.toString()) - : `${from} - ${to} of ${recordCount}`; + : `${displayFrom} - ${to} of ${recordCount}`;packages/typegen/web/src/components/ui/data-grid-column-header.tsx (1)
32-51: Unusedpinnableprop in interface.The
pinnableprop is defined in the interface at line 37 but is never destructured or used in the component. The pinning behavior instead relies onprops.tableLayout?.columnsPinnable && column.getCanPin().Consider removing it if not needed, or implement its intended behavior.
interface DataGridColumnHeaderProps<TData, TValue> extends HTMLAttributes<HTMLDivElement> { column: Column<TData, TValue>; title?: string; icon?: ReactNode; - pinnable?: boolean; filter?: ReactNode; visibility?: boolean; customActions?: ReactNode; }packages/typegen/web/src/components/ui/data-grid.tsx (1)
49-51: Consider makingtableprop required in the type definition.The
tableprop is marked optional (table?: Table<TData>) inDataGridProps, but the component throws an error if it's not provided (lines 166-168). This creates a runtime check for what could be a compile-time guarantee.export interface DataGridProps<TData extends object> { className?: string; - table?: Table<TData>; + table: Table<TData>; recordCount: number;This would provide better TypeScript enforcement and eliminate the need for the runtime check.
Also applies to: 165-168
packages/typegen/web/src/components/ui/data-grid-table.tsx (2)
357-380: Consider extracting the spinner SVG to a shared component.The spinner SVG is duplicated in
DataGridTableLoader(lines 363-375) and the inline spinner loading state (lines 462-474). Extracting it would reduce duplication and ensure consistent styling.function Spinner({ className }: { className?: string }) { return ( <svg className={cn("animate-spin h-5 w-5 text-muted-foreground", className)} xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" > <circle className="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" strokeWidth="3" /> <path className="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z" /> </svg> ); }Also applies to: 457-478
361-363: Minor: Double space in className.Line 362 has a double space between
bg-cardandflex.- <div className="text-muted-foreground bg-card flex items-center gap-2 px-4 py-2 font-medium leading-none text-sm border shadow-xs rounded-md"> + <div className="text-muted-foreground bg-card flex items-center gap-2 px-4 py-2 font-medium leading-none text-sm border shadow-xs rounded-md">packages/typegen/web/src/components/ui/data-grid-column-filter.tsx (2)
75-83: Consider using an immutable pattern for filter value updates.The current implementation mutates the
selectedValuesSet directly (lines 76-80), then converts it to an array for the filter update. While this pattern works because the Set is a local variable recreated on each render, it's not idiomatic React and could be confusing to maintainers.Apply this diff for a clearer, immutable approach:
<CommandItem key={option.value} onSelect={() => { - if (isSelected) { - selectedValues.delete(option.value); - } else { - selectedValues.add(option.value); - } - const filterValues = Array.from(selectedValues); + const filterValues = isSelected + ? Array.from(selectedValues).filter((v) => v !== option.value) + : [...Array.from(selectedValues), option.value]; column?.setFilterValue(filterValues.length ? filterValues : undefined); }} >This approach avoids mutation entirely and makes the logic more explicit.
36-62: Consider adding accessibility attributes.The trigger button and filter UI would benefit from ARIA attributes to improve screen reader support, such as
aria-labelfor the button,aria-expandedfor the popover state, androleattributes for the filter controls.Example enhancement:
<Button variant="outline" size="sm" aria-label={`Filter by ${title}`} aria-expanded={/* popover open state */} >packages/typegen/src/server/app.ts (1)
116-134: Duplicated response schema definitions.The same Zod object schema for the response (
{ success, error?, issues? }) is defined three times. Extract it to reduce duplication.+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({...}); +const response = configResponseSchema.parse({...});Also applies to: 144-158, 160-177
packages/typegen/src/fmodata/generateODataTypes.ts (3)
72-73: Consider adding a comment explaining the Edm.Boolean mapping.
Edm.Booleanis mapped tonumberField().outputValidator(z.coerce.boolean())rather than a native boolean field. This appears intentional for FileMaker compatibility, but a brief comment would help future maintainers understand this design decision.
226-236: String-based field builder tracking is fragile.The tracking logic uses
fieldBuilder.includes("textField()")which could break if the field builder output format changes. Consider extracting the base field builder name before the.outputValidator()chain, or track alongside themapODataTypeToFieldBuilderreturn.- // Track which field builders are used - if (fieldBuilder.includes("textField()")) { - usedFieldBuilders.add("textField"); - } else if (fieldBuilder.includes("numberField()")) { - usedFieldBuilders.add("numberField"); - } else if (fieldBuilder.includes("dateField()")) { - usedFieldBuilders.add("dateField"); - } else if (fieldBuilder.includes("timestampField()")) { - usedFieldBuilders.add("timestampField"); - } else if (fieldBuilder.includes("containerField()")) { - usedFieldBuilders.add("containerField"); - } + // Track which field builders are used + if (fieldBuilder.startsWith("textField")) { + usedFieldBuilders.add("textField"); + } else if (fieldBuilder.startsWith("numberField")) { + usedFieldBuilders.add("numberField"); + } else if (fieldBuilder.startsWith("dateField")) { + usedFieldBuilders.add("dateField"); + } else if (fieldBuilder.startsWith("timestampField")) { + usedFieldBuilders.add("timestampField"); + } else if (fieldBuilder.startsWith("containerField")) { + usedFieldBuilders.add("containerField"); + }
417-420: Consider using asyncfs.emptyDirfor consistency.The function uses async operations throughout (
mkdir,writeFile) but switches to syncemptyDirSynchere. For consistency and to avoid blocking the event loop with large directories, consider usingfs.emptyDir(async version).if (clearOldFiles) { // Clear the directory if requested (but keep the directory itself) - fs.emptyDirSync(resolvedOutputPath); + await fs.emptyDir(resolvedOutputPath); }packages/typegen/web/src/hooks/useParseMetadata.ts (2)
65-72: Consider switching to POST for large configs.As the comment acknowledges, passing the entire config as a JSON-stringified query parameter could hit URL length limits (~2048 chars in some browsers). Consider using POST for reliability with complex configurations.
81-82: Type assertion without validation.The response is cast to
ParsedMetadataResponsewithout runtime validation. Consider using Zod to validate the response matches the expected shape, especially since Zod 4 is already a dependency.packages/typegen/src/typegen.ts (1)
50-58: Missingcontinueafter error logging for invalid config type.When an invalid config type is encountered, the error is logged but the loop continues to the next iteration implicitly. Adding an explicit
continuewould make the control flow clearer, and potentially tracking this as an error for the return value would improve observability.} else if (singleConfig.type === "fmodata") { await generateODataTablesSingle(singleConfig); } else { console.log(chalk.red("ERROR: Invalid config type")); + continue; }packages/typegen/web/src/components/MetadataFieldsDialog.tsx (5)
35-40: Good performance optimization for row models.Memoizing row model functions outside the component prevents unnecessary re-renders. However,
EMPTY_FIELDS_CONFIGtyped asany[]loses type safety.-// Stable empty array to prevent infinite re-renders -const EMPTY_FIELDS_CONFIG: any[] = []; +// Stable empty array to prevent infinite re-renders +const EMPTY_FIELDS_CONFIG: Array<{ fieldName: string; exclude?: boolean; typeOverride?: string }> = [];
169-277: Significant code duplication betweentoggleFieldExcludeandsetFieldTypeOverride.These two functions share ~80% of their logic (finding tables/fields, updating arrays, cleaning up empty entries). Consider extracting a shared helper function that handles the common table/field lookup and update pattern.
// Example helper structure function updateFieldConfig( params: { configType: string; tableName: string | null; allTablesConfig: any[]; configIndex: number; setValue: any; fieldName: string; update: (existing: any) => any; // returns updated field or null to remove } ) { // Shared table/field lookup and update logic }Also applies to: 280-391
416-486: Duplicate logic for Map vs object property handling.The
fieldsDatamemo has nearly identical logic for handlingMapand plain object formats (lines 416-450 vs 451-486). Consider extracting a helper or normalizing the data format earlier.// Example: normalize to entries first const entries = properties instanceof Map ? Array.from(properties.entries()) : Object.entries(properties); // Then use single loop for entries
229-230: Shadowed variable name_.The underscore
_is used twice in the destructuring on line 230 (const { exclude: _, ...rest }), which shadows the loop variable from line 234. While this works, it's potentially confusing.- const { exclude: _, ...rest } = fieldConfig; + const { exclude: _excluded, ...rest } = fieldConfig;
46-67: Extract shared OData type mapping to prevent inconsistenciesBoth
mapODataTypeToReadableLabelandmapODataTypeToFieldBuilderduplicate the same EDM type categorization logic. While the outputs differ (readable labels vs. field builder calls), the underlying type groupings are identical. Consider extracting the type mapping into a shared utility (e.g.,packages/typegen/src/utils/oDataTypeMapping.ts) that both frontend and backend can import. This ensures future type additions or changes are reflected consistently across the UI and code generation.packages/typegen/web/src/components/MetadataTablesEditor.tsx (3)
27-27: Improve type safety for the empty array constant.The
any[]type reduces type safety. Consider using a more specific type based on the tables config structure.Apply this diff to improve type safety:
-const EMPTY_TABLES_CONFIG: any[] = []; +const EMPTY_TABLES_CONFIG: NonNullable<Extract<SingleConfig, { type: 'fmodata' }>['tables']> = [];Or simply:
-const EMPTY_TABLES_CONFIG: any[] = []; +const EMPTY_TABLES_CONFIG = [] as const;
91-91: Type assertions with react-hook-form dynamic paths.Multiple
as anytype assertions are used forsetValuepaths due to react-hook-form's limitations with dynamically constructed path strings. While this is a known limitation, consider using type-safe alternatives if available in your react-hook-form version.If using react-hook-form v7.43.0+, you can potentially improve type safety:
setValue(`config.${configIndex}.tables`, newTables, { shouldDirty: true, });However, if type inference still fails with nested dynamic indices, the
as anyapproach is acceptable as a pragmatic workaround.Also applies to: 97-97, 112-112, 120-120, 158-158, 187-187
215-217: Redundant Array.isArray check.The
Array.isArray(tablesConfig)check is redundant sincetablesConfigis already guaranteed to be an array based on lines 65-68. However, this doesn't harm functionality and may serve as defensive programming.Consider simplifying:
-const tableConfig = Array.isArray(tablesConfig) - ? tablesConfig.find((t) => t?.tableName === entitySet.Name) - : undefined; +const tableConfig = tablesConfig.find((t) => t?.tableName === entitySet.Name);
📜 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 (33)
.gitignore(1 hunks)packages/typegen/proofkit-typegen.config.jsonc(1 hunks)packages/typegen/src/fmodata/generateODataTypes.ts(1 hunks)packages/typegen/src/fmodata/index.ts(1 hunks)packages/typegen/src/fmodata/typegen.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/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/LayoutItemEditor.tsx(1 hunks)packages/typegen/web/src/components/MetadataFieldsDialog.tsx(1 hunks)packages/typegen/web/src/components/MetadataTablesEditor.tsx(1 hunks)packages/typegen/web/src/components/input/icon.tsx(1 hunks)packages/typegen/web/src/components/ui/button.tsx(1 hunks)packages/typegen/web/src/components/ui/checkbox.tsx(1 hunks)packages/typegen/web/src/components/ui/data-grid-column-filter.tsx(1 hunks)packages/typegen/web/src/components/ui/data-grid-column-header.tsx(1 hunks)packages/typegen/web/src/components/ui/data-grid-column-visibility.tsx(1 hunks)packages/typegen/web/src/components/ui/data-grid-pagination.tsx(1 hunks)packages/typegen/web/src/components/ui/data-grid-table-dnd-rows.tsx(1 hunks)packages/typegen/web/src/components/ui/data-grid-table-dnd.tsx(1 hunks)packages/typegen/web/src/components/ui/data-grid-table.tsx(1 hunks)packages/typegen/web/src/components/ui/data-grid.tsx(1 hunks)packages/typegen/web/src/components/ui/separator.tsx(1 hunks)packages/typegen/web/src/components/ui/sheet.tsx(1 hunks)packages/typegen/web/src/components/ui/skeleton.tsx(1 hunks)packages/typegen/web/src/components/ui/switch-field.tsx(1 hunks)packages/typegen/web/src/hooks/useFileExists.ts(1 hunks)packages/typegen/web/src/hooks/useParseMetadata.ts(1 hunks)packages/typegen/web/src/hooks/useRunTypegen.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/typegen/proofkit-typegen.config.jsonc
- packages/typegen/web/src/App.tsx
- packages/typegen/web/src/components/ConfigEditor.tsx
- packages/typegen/web/src/components/ui/switch-field.tsx
- packages/typegen/src/fmodata/index.ts
🧰 Additional context used
🧬 Code graph analysis (9)
packages/typegen/web/src/components/ui/skeleton.tsx (1)
apps/docs/src/components/ui/skeleton.tsx (1)
Skeleton(4-6)
packages/typegen/web/src/components/ui/button.tsx (1)
packages/typegen/web/src/lib/utils.ts (1)
cn(10-12)
packages/typegen/web/src/hooks/useParseMetadata.ts (1)
packages/typegen/web/src/hooks/useFileExists.ts (1)
useFileExists(4-26)
packages/typegen/web/src/components/LayoutItemEditor.tsx (4)
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/components/LayoutSelector.tsx (1)
LayoutSelector(33-241)packages/typegen/web/src/components/InfoTooltip.tsx (1)
InfoTooltip(4-15)
packages/typegen/web/src/components/input/icon.tsx (3)
packages/typegen/web/src/components/ui/input.tsx (2)
InputWrapper(163-163)Input(163-163)packages/typegen/web/src/components/ui/button.tsx (1)
Button(477-477)packages/cli/template/extras/fmaddon-auth/server/auth/utils/user.ts (1)
User(5-11)
packages/typegen/web/src/components/ui/data-grid-column-header.tsx (4)
packages/typegen/web/src/components/ui/data-grid.tsx (1)
useDataGrid(193-193)apps/docs/src/lib/source.ts (1)
icon(12-47)packages/typegen/web/src/components/ui/button.tsx (1)
Button(477-477)packages/typegen/web/src/components/ui/dropdown-menu.tsx (11)
DropdownMenu(209-209)DropdownMenuTrigger(223-223)DropdownMenuContent(211-211)DropdownMenuLabel(214-214)DropdownMenuSeparator(218-218)DropdownMenuItem(213-213)DropdownMenuSub(220-220)DropdownMenuSubTrigger(222-222)DropdownMenuPortal(215-215)DropdownMenuSubContent(221-221)DropdownMenuCheckboxItem(210-210)
packages/typegen/web/src/components/ui/data-grid-pagination.tsx (4)
packages/typegen/web/src/components/ui/data-grid.tsx (1)
useDataGrid(193-193)packages/typegen/web/src/components/ui/skeleton.tsx (1)
Skeleton(8-8)packages/typegen/web/src/components/ui/button.tsx (1)
Button(477-477)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/src/fmodata/generateODataTypes.ts (1)
packages/typegen/src/types.ts (1)
FmodataConfig(217-220)
packages/typegen/src/server/app.ts (10)
packages/typegen/src/server/api.ts (1)
ApiContext(8-11)packages/fmdapi/src/client-types.ts (2)
LayoutOrFolder(262-262)FileMakerError(289-296)apps/docs/scripts/bundle-registry-templates.js (2)
path(4-4)fs(3-3)packages/typegen/src/types.ts (2)
typegenConfig(211-213)typegenConfigSingle(204-209)packages/typegen/web/src/lib/schema.ts (1)
typegenConfigSingle(3-3)packages/typegen/src/typegen.ts (1)
generateTypedClients(30-59)packages/typegen/src/server/createDataApiClient.ts (3)
createDataApiClient(247-306)createClientFromConfig(191-239)createOdataClientFromConfig(167-184)packages/typegen/web/src/lib/api.ts (1)
client(7-7)packages/typegen/src/fmodata/index.ts (2)
downloadMetadata(1-1)parseMetadata(3-3)packages/typegen/src/server/contract.ts (1)
ApiApp(75-75)
⏰ 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: Cursor Bugbot
- 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 (57)
.gitignore (1)
75-75: Entry correctly ignores generated metadata schema artifact.The addition of
packages/typegen/schema/metadata.xmlis appropriate and aligns with the PR's introduction of typegen schema generation. The entry follows existing patterns (e.g.,packages/fmdapi/test/typegen/*on line 74) for ignoring generated artifacts in the typegen workflow.packages/typegen/web/src/components/ui/sheet.tsx (1)
7-7: No issues found. The import from'radix-ui'is the modern consolidated approach and is correct.packages/typegen/web/src/components/ui/checkbox.tsx (1)
32-45: Component logic is well-structured.The Checkbox component implementation correctly:
- Integrates variant props with component props
- Renders the appropriate icon (Check or Minus) based on the indeterminate state
- Applies proper className merging
- Maintains type safety with VariantProps
Once the import path on line 7 is corrected, this component should function as intended.
packages/typegen/web/src/components/input/icon.tsx (2)
1-3: LGTM!All imports are correctly referenced and follow the project's import patterns.
5-5: No issues found. Both files exportInputDemo, but they are isolated demo components never imported together. In TypeScript modules, two different modules will never contribute names to the same scope, so no naming collision or import conflict occurs in practice.packages/typegen/web/src/components/LayoutItemEditor.tsx (6)
1-28: LGTM! Clean imports and interface definition.The imports are well-organized, and the component props are clearly typed. The interface appropriately defines the required data and callbacks.
35-41: LGTM! Standard React Hook Form pattern.The form context setup and field watching are implemented correctly using react-hook-form best practices.
43-96: LGTM! Well-structured header and input fields.The header provides clear visual feedback with appropriate fallbacks, and the form fields are properly integrated with React Hook Form. The responsive grid layout and InfoTooltip components enhance the user experience.
98-131: LGTM! Appropriate default value handling pattern.The use of
"__default__"as a sentinel value to representundefinedin the Select component is a reasonable pattern for handling optional configuration values. The bidirectional mapping between the sentinel andundefinedis implemented consistently.
133-151: LGTM! Appropriate boolean field handling.The switch field correctly defaults to
falsewhen the value is undefined or null, which is appropriate behavior for a boolean configuration option. The descriptive tooltip helps users understand the implications of this setting.
153-205: LGTM! Excellent tri-state select implementation.The Generate Client field elegantly handles three distinct states (undefined, true, false) with clear visual feedback. The conditional styling using Tailwind's arbitrary variant syntax correctly distinguishes the default state from explicit choices. The bidirectional mapping between the tri-state boolean and string values is implemented correctly.
packages/typegen/web/package.json (3)
1-10: Package metadata and scripts look good.The package is properly configured as a private, ESM module with standard Vite scripts. The setup is clean and follows monorepo conventions.
11-33: Dependencies are well-structured and versions align with the upgrade.The package includes appropriate dependencies for a modern React 19 web application with Tailwind 4 and Zod 4. The UI libraries (@dnd-kit, @radix-ui/, @tanstack/), form handling, styling utilities, and build tooling are all correctly versioned and compatible.
Also applies to: 35-42, 43-51
32-32: Verify Hono dependency placement.Hono is a server-side framework, which is unusual in a frontend web package. The PR mentions "Hono-based API," so this may be intentional if the web package includes both UI and API code, or uses Hono for development/mocking purposes.
Confirm this dependency is correctly placed and verify its usage:
#!/bin/bash # Search for Hono usage in the web package rg -n --type=ts --type=tsx 'from ["\']hono["\']|import.*hono' packages/typegen/web/packages/typegen/web/src/components/ui/button.tsx (1)
459-475: LGTM!The ButtonArrow component is well-structured with appropriate defaults and type safety. The
ms-autoclass correctly positions the icon at the end of flex containers.packages/typegen/web/src/components/ui/skeleton.tsx (1)
1-8: LGTM!Clean implementation that matches the established pattern in the codebase (consistent with
apps/docs/src/components/ui/skeleton.tsx). The component correctly uses thecnutility for className composition and follows thedata-slotconvention.packages/typegen/web/src/components/ui/data-grid-column-visibility.tsx (1)
1-38: LGTM!Well-structured column visibility toggle component. The filtering logic correctly identifies columns with data accessors that are hideable, and the
onSelectpreventDefault pattern appropriately keeps the dropdown open for multi-column selection.packages/typegen/web/src/components/ui/data-grid-table-dnd-rows.tsx (2)
35-45: LGTM!The drag handle component correctly uses
useSortableand spreads bothattributesandlistenersto enable keyboard and pointer drag interactions.
72-144: LGTM!The DnD rows implementation correctly sets up the drag context with appropriate sensors, collision detection, and axis constraints. The skeleton loading fallback and empty state handling are well implemented.
packages/typegen/web/src/components/ui/data-grid-table-dnd.tsx (2)
35-74: LGTM!The header and cell DnD components correctly implement the sortable pattern with appropriate styling during drag operations. The drag handle with accessible aria-label is a good touch.
9-9: FunctionDataGridTableBodyRowExpanddedis misspelled throughout the codebase and should beDataGridTableBodyRowExpanded.The typo exists in the function definition in
data-grid-table.tsx(line 278), its export (line 511), and subsequently in all imports and usages includingdata-grid-table-dnd.tsx. The import statement is correct—it matches the export—but the underlying component name itself is misspelled with a double 'd'.Likely an incorrect or invalid review comment.
packages/typegen/web/src/components/ui/data-grid-pagination.tsx (2)
26-93: LGTM!Well-designed pagination component with good configurability through props, proper default values, and clean group-based page button rendering logic. The RTL support and responsive layout are nice touches.
131-211: LGTM!The render logic correctly handles loading states with skeleton placeholders, properly disables navigation buttons at boundaries, and provides accessible labels via
sr-onlyspans. The conditional rendering forpageCount > 1avoids showing unnecessary pagination controls for single-page data.packages/typegen/web/src/components/ui/data-grid-column-header.tsx (1)
43-135: LGTM!The column header component is well-structured with clear separation between label, button, pin, and controls rendering modes. The sorting toggle logic correctly cycles through asc → desc → clear states, and the icon rendering appropriately reflects the current sort state.
packages/typegen/web/src/components/ui/data-grid.tsx (1)
101-118: LGTM!The provider pattern with context is well-implemented. The separation between
DataGridProvider(internal) andDataGrid(public API with defaults) provides a clean abstraction, and the deep merge oftableLayoutandtableClassNamesensures user props override defaults correctly.packages/typegen/web/src/components/ui/data-grid-table.tsx (1)
414-503: LGTM!The main
DataGridTablecomponent has well-structured conditional rendering for loading states (skeleton vs spinner), data rows, and empty state. The use ofFragmentfor expanded row rendering is appropriate, and the skeleton loading mode correctly usespagination.pageSizeto render placeholder rows.packages/typegen/web/src/components/ui/separator.tsx (2)
7-22: Component implementation looks good.The Separator component logic is well-implemented with appropriate prop defaults, clean conditional styling, and correct Tailwind utilities. The
data-slotattribute maintains consistency with other UI primitives in the project.Note: The
'use client'directive on Line 1 is unnecessary in a Vite app (mentioned in PR summary) but harmless, as Vite will simply ignore it. If these components may be shared with Next.js parts of the codebase, keeping it is acceptable for consistency.
5-5: The import statement is correct. The project uses the officialradix-uibarrel export package (version 1.4.3), which is installed in package.json. The patternimport { Separator } from "radix-ui"followed by accessing.Rootis the recommended approach for using this package. The current code follows this correctly, so no changes are needed.packages/typegen/web/src/components/ui/data-grid-column-filter.tsx (3)
1-17: LGTM!Imports are well-organized and all dependencies are used appropriately in the component.
19-27: LGTM!The interface is well-defined with appropriate generics and optional properties that align with the component's flexible usage patterns.
124-124: LGTM!The component and type exports follow best practices for reusable React component libraries.
packages/typegen/web/src/hooks/useRunTypegen.ts (1)
5-19: LGTM!The hook correctly wraps the API call with
useMutationand exposes the essential states (runTypegen,isRunning,error). The fire-and-forget pattern is appropriate for triggering typegen runs.packages/typegen/web/src/hooks/useFileExists.ts (1)
4-26: LGTM!The hook properly handles edge cases (empty/undefined path), uses appropriate query configuration for file existence checks, and correctly enables the query only when a valid path is provided.
packages/typegen/src/server/app.ts (2)
27-52: LGTM!The
flattenLayoutshelper correctly recursively flattens nested layout/folder structures into a flat array with full paths.
367-624: Test connection logic is comprehensive but verbose.The error handling for both fmdapi and fmodata connections is thorough, mapping FileMaker error codes to user-friendly messages with suspected fields. The duplication in error inference logic (lines 482-517 and 552-587) could be extracted, but given the contextual differences this is acceptable.
packages/typegen/src/server/createDataApiClient.ts (2)
191-239: LGTM!The
createClientFromConfigfunction cleanly handles both API key and username/password authentication, with proper error handling for adapter construction failures.
247-306: LGTM!The
createDataApiClientfunction properly handles config file loading, parsing, index-based selection, and type validation with appropriate error responses.packages/typegen/src/types.ts (3)
84-88:.default("schema").optional()ordering may cause unexpected behavior.In Zod 4,
.default()before.optional()means: if the value isundefined, use"schema", but also allow explicitundefined. This is likely intentional for backward compatibility, but verify this is the desired behavior.
146-209: Well-structured discriminated union with backward compatibility.The
typegenConfigSingleBasediscriminated union cleanly separates fmdapi and fmodata configurations. Thez.preprocesswrapper intypegenConfigSinglecorrectly defaultstypeto"fmdapi"for backward compatibility with existing configs.
90-144: LGTM!The
fieldOverrideandtableOverrideschemas provide flexible configuration for fmodata type generation with clear metadata descriptions.packages/typegen/src/fmodata/generateODataTypes.ts (6)
1-5: LGTM on imports.The imports are well-organized, combining Node.js built-in modules with external dependencies. The mixed use of
node:fs/promisesfor async operations andfs-extrafor sync operations (likeemptyDirSync) is a reasonable approach.
15-43: LGTM!The function correctly maps all type override values to their corresponding field builders. TypeScript's exhaustiveness checking ensures all cases are handled.
89-101: LGTM!The regex-based extraction logic is correct. The function handles both
Collection(...)wrapped types and plain dotted type strings appropriately.
308-346: LGTM!The import generation correctly uses
"zod/v4"which aligns with the Zod 4 upgrade mentioned in the PR objectives.
348-353: LGTM!Simple and effective filename sanitization.
362-368: Destructive default:clearOldFilesdefaults totrue.The default of
clearOldFiles = truemeans existing generated files are deleted by default. This is probably intentional for clean regeneration, but ensure consumers are aware of this behavior.packages/typegen/src/fmodata/typegen.ts (1)
7-23: LGTM!The orchestration flow is clean and logical: optionally download metadata, validate file existence, parse, and generate types. The error message is clear when the metadata file is missing.
packages/typegen/src/typegen.ts (2)
61-62: Good type narrowing for fmdapi config.The
Extract<z.infer<typeof typegenConfigSingle>, { type: "fmdapi" }>type ensures type safety within the function, preventing accidental access to fmodata-specific properties.
102-107: LGTM on environment validation flow.The early return pattern when validation fails is clean and prevents further execution with invalid configuration.
packages/typegen/web/src/components/MetadataFieldsDialog.tsx (2)
104-127: LGTM on conditional rendering pattern.The wrapper component pattern that only mounts the content when
openis true is a good optimization to avoid expensive hook computations when the dialog is closed.
788-800: LGTM on table configuration.Good use of memoized row models and proper filter state management with
globalFilterFn: "includesString".packages/typegen/web/src/hooks/useParseMetadata.ts (1)
8-23: This suggestion is not applicable. TheParsedMetadataResponseinterface inuseParseMetadata.tsis not present in the server contract (packages/typegen/src/server/contract.ts). The server contract defines API response schemas for config, layouts, and environment operations, but does not define anyParsedMetadatatypes. TheParsedMetadataResponseinterface is a specialized web client type used specifically for parsing OData metadata in the hook and has no counterpart to share.Likely an incorrect or invalid review comment.
packages/typegen/web/src/components/MetadataTablesEditor.tsx (5)
130-190: LGTM: Bulk table inclusion/exclusion logic is correct.The bulk operations correctly handle:
- Guard clauses for config type and metadata presence
- Efficient lookups using Map in
excludeAllTables- Cleanup of entries that only contain
tableNameinincludeAllTables- Setting
undefinedwhen the resulting array is empty
203-213: LGTM: Defensive handling of Properties format.The code correctly handles both
Mapand plain object formats forentityType.Properties, which is good defensive programming in case the metadata parsing produces different structures.
251-350: LGTM: Column definitions are well-structured.The column definitions correctly implement:
- Include/exclude toggle with bulk actions
- Proper event propagation handling (
stopPropagation())- Visual feedback (strikethrough, muted text) for excluded items
- Conditional field count display showing excluded/total when applicable
- Appropriate memoization dependencies
352-426: LGTM: Table instance and state handling are correct.The component properly:
- Uses memoized model functions for stable references
- Handles all metadata states (loading, error, missing file, no data)
- Provides clear user feedback with appropriate icons and messages
- Connects the row click handler to open the fields dialog
428-463: LGTM: Main render structure is clean and correct.The component properly integrates:
- Search input with icon and state binding
- DataGrid with filtering, loading state, and row interaction
- MetadataFieldsDialog for per-table field configuration
- Appropriate spacing and layout structure
| .get( | ||
| "/file-exists", | ||
| zValidator("query", z.object({ path: z.string() })), | ||
| async (c) => { | ||
| const input = c.req.valid("query"); | ||
| const path = input.path; | ||
| const exists = await fs.pathExists(path); | ||
| return c.json({ exists }); | ||
| }, | ||
| ) |
There was a problem hiding this comment.
GET /api/file-exists allows arbitrary path traversal.
This endpoint checks file existence for any path without validation, potentially allowing attackers to probe the filesystem structure (e.g., /etc/passwd, ../../sensitive-file).
Consider restricting to paths within the project directory:
.get(
"/file-exists",
zValidator("query", z.object({ path: z.string() })),
async (c) => {
const input = c.req.valid("query");
- const path = input.path;
- const exists = await fs.pathExists(path);
+ // Resolve to absolute and ensure it's within project root
+ const resolvedPath = path.resolve(context.cwd, input.path);
+ if (!resolvedPath.startsWith(context.cwd)) {
+ return c.json({ error: "Path outside project directory" }, 400);
+ }
+ const exists = await fs.pathExists(resolvedPath);
return c.json({ exists });
},
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/typegen/src/server/app.ts around lines 305 to 314, the GET
/file-exists handler accepts any arbitrary path which allows path traversal and
filesystem probing; change it to only accept paths under the project base
directory by validating and sanitizing the input: reject absolute paths or those
containing null bytes, resolve the provided path against a configured baseDir
(e.g. project root) using path.resolve(baseDir, input.path), then ensure the
resolved path startsWith the baseDir (or is equal) before calling fs.pathExists;
if the check fails return a 400/403 error. Ensure you log/return a safe error
message and keep the zod schema but add a stricter constraint (no leading slash,
optional regex to disallow ..) or perform the validation programmatically prior
to filesystem access.
| import * as React from "react"; | ||
| import { cva, type VariantProps } from "class-variance-authority"; | ||
| import { ChevronDown, LucideIcon } from "lucide-react"; | ||
| import { Slot as SlotPrimitive } from "radix-ui"; |
There was a problem hiding this comment.
Fix the incorrect import path.
The import path "radix-ui" is incorrect. Radix UI's Slot component should be imported from "@radix-ui/react-slot".
Apply this diff:
-import { Slot as SlotPrimitive } from "radix-ui";
+import { Slot as SlotPrimitive } from "@radix-ui/react-slot";📝 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.
| import { Slot as SlotPrimitive } from "radix-ui"; | |
| import { Slot as SlotPrimitive } from "@radix-ui/react-slot"; |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/button.tsx around line 4 the import
path is wrong: it imports Slot from "radix-ui". Replace that import with the
correct Radix Slot import from "@radix-ui/react-slot", updating the import
statement to import Slot (or Slot as SlotPrimitive) from "@radix-ui/react-slot"
so the component resolves correctly.
| underline: { | ||
| solid: "", | ||
| dashed: "", | ||
| }, | ||
| underlined: { | ||
| solid: "", | ||
| dashed: "", | ||
| }, |
There was a problem hiding this comment.
Clarify the distinction between underline and underlined variants.
Both underline and underlined variant keys exist with identical options (solid, dashed). This creates API confusion—it's unclear when to use which variant.
Consider consolidating to a single variant or documenting the semantic difference (e.g., if underline means "underline on hover" and underlined means "always underlined").
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/button.tsx around lines 35 to 42, the
variant keys `underline` and `underlined` are identical which causes API
confusion; pick one semantic meaning and apply it consistently: either
consolidate to a single variant (remove one key, migrate all occurrences and
types to the chosen name) or define distinct behavior (e.g., `underline` =
underline on hover, `underlined` = always underlined) and implement the
CSS/classes accordingly, then update the component's prop types, unit/visual
tests, and any usages/docs to reflect the chosen name and behavior.
| mode: "link", | ||
| underlined: "dashed", | ||
| className: | ||
| "font-medium text-primary hover:text-primary/90 [&_svg]:opacity-60 underline underline-offset-4 decoration-dashed decoration-1", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use consistent selector pattern.
Line 267 uses the shorter [&_svg]:opacity-60 selector while similar rules use the more specific [&_svg:not([role=img]):not([class*=text-])]:opacity-60. This inconsistency may cause unintended opacity on icon elements with role="img" or text color classes.
Apply this diff for consistency:
- "font-medium text-primary hover:text-primary/90 [&_svg]:opacity-60 underline underline-offset-4 decoration-dashed decoration-1",
+ "font-medium text-primary hover:text-primary/90 [&_svg:not([role=img]):not([class*=text-])]:opacity-60 underline underline-offset-4 decoration-dashed decoration-1",📝 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.
| "font-medium text-primary hover:text-primary/90 [&_svg]:opacity-60 underline underline-offset-4 decoration-dashed decoration-1", | |
| "font-medium text-primary hover:text-primary/90 [&_svg:not([role=img]):not([class*=text-])]:opacity-60 underline underline-offset-4 decoration-dashed decoration-1", |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/button.tsx around line 267, the class
string uses the shorter selector "[&_svg]:opacity-60" which is inconsistent with
other rules and may affect svg elements with role="img" or text color classes;
replace that selector with the more specific
"[&_svg:not([role=img]):not([class*=text-])]:opacity-60" so only non-decorative
SVGs get opacity applied, keeping the rest of the class string unchanged.
| { | ||
| size: "xs", | ||
| mode: "icon", | ||
| className: "w-7 h-7 p-0 [[&_svg:not([class*=size-])]:size-3.5", |
There was a problem hiding this comment.
Fix syntax error: extra opening bracket.
The selector has a double opening bracket [[&_svg instead of [&_svg.
Apply this diff:
- 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",📝 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.
| 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", |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/button.tsx around line 357, the
className string contains a syntax error with a double opening bracket in the
selector ("[[&_svg..."); remove the extra "[" so the selector reads
"[&_svg:not([class*=size-])]:size-3.5" to fix the malformed Tailwind/selector
string and restore correct className syntax.
| { | ||
| size: "sm", | ||
| mode: "icon", | ||
| className: "w-8 h-8 p-0 [[&_svg:not([class*=size-])]:size-3.5", |
There was a problem hiding this comment.
Fix syntax error: extra opening bracket.
The selector has a double opening bracket [[&_svg instead of [&_svg.
Apply this diff:
- 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",📝 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.
| 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", |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/button.tsx around line 362, the
className string contains a syntax error with a double opening bracket before
the sibling selector (`[[&_svg`); change the selector to a single opening
bracket so it reads `[&_svg:not([class*=size-])]` (preserve the rest of the
class string, quotes, and trailing comma).
| @@ -0,0 +1,47 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
Remove the unnecessary 'use client' directive.
The 'use client' directive is specific to Next.js and React Server Components. According to the PR summary, this is a React/Vite application where this directive has no effect and may cause confusion.
Apply this diff to remove the directive:
-'use client';
-
import * as React from 'react';📝 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.
| 'use client'; | |
| import * as React from 'react'; |
🤖 Prompt for AI Agents
In packages/typegen/web/src/components/ui/checkbox.tsx around line 1, remove the
unnecessary 'use client' directive by deleting the first line containing 'use
client'; ensure no other code depends on it and that the component continues to
export/compile correctly in the React/Vite app (run TypeScript/ESLint if needed
to catch any side effects).
| function DataGridColumnFilter<TData, TValue>({ column, title, options }: DataGridColumnFilterProps<TData, TValue>) { | ||
| const facets = column?.getFacetedUniqueValues(); | ||
| const selectedValues = new Set(column?.getFilterValue() as string[]); |
There was a problem hiding this comment.
Unsafe type assertion should be replaced with runtime type checking.
Line 31 uses as string[] to assert the filter value type, but TanStack Table's getFilterValue() returns unknown type. The actual filter value depends on the column's filterFn configuration and could be any type. This unsafe assertion may cause runtime errors if the filter value isn't actually a string array.
Add runtime type checking:
- const selectedValues = new Set(column?.getFilterValue() as string[]);
+ const filterValue = column?.getFilterValue();
+ const selectedValues = new Set(
+ Array.isArray(filterValue) && filterValue.every((v) => typeof v === 'string')
+ ? filterValue
+ : []
+ );Alternatively, if the component type is properly constrained in props (e.g., TValue extends string), ensure the column definition is typed accordingly to make the assertion safe.
📝 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 DataGridColumnFilter<TData, TValue>({ column, title, options }: DataGridColumnFilterProps<TData, TValue>) { | |
| const facets = column?.getFacetedUniqueValues(); | |
| const selectedValues = new Set(column?.getFilterValue() as string[]); | |
| function DataGridColumnFilter<TData, TValue>({ column, title, options }: DataGridColumnFilterProps<TData, TValue>) { | |
| const facets = column?.getFacetedUniqueValues(); | |
| const filterValue = column?.getFilterValue(); | |
| const selectedValues = new Set( | |
| Array.isArray(filterValue) && filterValue.every((v) => typeof v === 'string') | |
| ? filterValue | |
| : [] | |
| ); |
🤖 Prompt for AI Agents
packages/typegen/web/src/components/ui/data-grid-column-filter.tsx lines 29-31:
the code unsafely asserts column?.getFilterValue() as string[]; replace this
with a runtime-safe approach — either constrain the component generic so TValue
extends string and ensure the column is typed accordingly, or perform runtime
checks: read const raw = column?.getFilterValue(); if (Array.isArray(raw) &&
raw.every(v => typeof v === 'string')) then create selectedValues = new Set(raw)
else use an empty Set (or handle other expected shapes); update
types/refs/comments to reflect the chosen approach.
|
|
||
| await generateTypedClients(config); | ||
| await next(); | ||
| }, |
There was a problem hiding this comment.
Bug: Run endpoint calls next() but returns no response
The POST /api/run endpoint handler calls await generateTypedClients(config) and then await next(), but never returns a response to the client. In Hono, calling next() without a subsequent handler that returns a response will leave the client without proper feedback. The frontend's runTypegenMutation in App.tsx calls this endpoint and expects a response. The handler needs to return something like return c.json({ success: true }) after generating the typed clients.
| return c.body(content, 200, { | ||
| "Content-Type": contentType, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Static file serving lacks path traversal protection
The static file serving handler uses path.join(WEB_DIR, pathname) without validating that the resolved path remains within WEB_DIR. A request with path components like /../../../etc/passwd would allow reading files outside the web directory. The code should resolve the full path and verify it starts with WEB_DIR before serving the file. While this is a localhost-only development server (reducing practical impact), it still violates the principle of least privilege.
|
|
||
| await generateTypedClients(config); | ||
| await next(); | ||
| }, |
There was a problem hiding this comment.
Bug: API run endpoint never returns a response
The /api/run POST endpoint handler calls await next() after generateTypedClients completes but never returns a response. All other POST handlers in this file return responses via c.json(...). Since there's no subsequent middleware for this route, the client will hang indefinitely waiting for a response. The handler needs to return something like return c.json({ success: true }) instead of calling next().
| moduleSpecifier: type, | ||
| moduleSpecifier: "zod/v4", | ||
| namedImports: ["z"], | ||
| }); |
There was a problem hiding this comment.
Bug: Schema file always imports from zod/v4 regardless of type
The buildSchema function hardcodes moduleSpecifier: "zod/v4" for the schema file import regardless of the type parameter value. When a user specifies "zod/v3" as the validator, the generated schema file will incorrectly import from "zod/v4". Additionally, there's an inconsistency: the override file (in buildOverrideFile) uses different logic, mapping "zod/v4" to "zod" while leaving "zod/v3" as-is, creating mismatched imports between generated files.
Additional Locations (1)
| // Apply variableName override if provided, otherwise generate from entitySetName | ||
| const varName = tableOverride?.variableName | ||
| ? tableOverride.variableName.replace(/[^a-zA-Z0-9_]/g, "_") | ||
| : entitySetName.replace(/[^a-zA-Z0-9_]/g, "_"); |
There was a problem hiding this comment.
Bug: Generated variable names may start with numbers
The varName generation and sanitizeFileName function only remove non-alphanumeric characters using the regex [^a-zA-Z0-9_] but don't handle names that start with numbers. If an entity set name like "123Users" or "2ndTable" is in the metadata, the generated code would be export const 123Users = fmTableOccurrence(...), which is invalid JavaScript syntax. The sanitization needs to also prefix an underscore or similar character when the name starts with a digit.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/registry/lib/validator.test.ts (1)
217-236: Inconsistent test name and implementation.The test name at line 217 says "should throw error for invalid proofkitDependencies" but the test data at line 225 uses the field name
registryDependencies. Additionally, the earlier test at lines 196-215 titled "should validate proofkitDependencies exist" still usesproofkitDependenciesin the test data (line 204).This inconsistency suggests an incomplete refactoring. Update test names to match the field names being tested, or ensure all tests use the same field name consistently.
Apply this diff to align the test name with the implementation:
- it("should throw error for invalid proofkitDependencies", () => { + it("should throw error for invalid registryDependencies", () => { const metaWithInvalidDeps = { type: "static", title: "Test Template",Also consider updating the test at line 196 and its test data at line 204 to use
registryDependenciesfor consistency, or clarify if both field names are intentionally supported.packages/fmodata/tests/fixtures/responses.ts (1)
294-308: Unsanitized real server URL in location headers.The
locationheaders at lines 296 and 308 containacme-dev.ottomatic.cloudinstead of the sanitizedapi.example.comdomain used elsewhere. This appears to be a real server URL that bypassed sanitization.The capture script's
sanitizeUrlfunction sanitizes theurlfield but thelocationheader extraction at lines 664 and 689-695 doesn't appear to sanitize the location value.- "https://acme-dev.ottomatic.cloud/fmi/odata/v4/fmdapi_test.fmp12/contacts(ROWID=11167)", + "https://api.example.com/fmi/odata/v4/fmdapi_test.fmp12/contacts(ROWID=11167)",And at line 308:
- "https://acme-dev.ottomatic.cloud/fmi/odata/v4/fmdapi_test.fmp12/contacts('A51EAF8A-68DF-426D-9683-AFF5AAB3CD6D')", + "https://api.example.com/fmi/odata/v4/fmdapi_test.fmp12/contacts('A51EAF8A-68DF-426D-9683-AFF5AAB3CD6D')",Consider also updating the capture script to sanitize the
locationheader value.
🧹 Nitpick comments (6)
apps/docs/tests/utils.manifest.test.ts (1)
16-18: Consider adding negative assertions to match the documented shape.The comment states that
RegistryIndexItemdoes not havetypeorfilesproperties, but the test only verifies the presence oftitleanddescription. Adding explicit negative assertions would strengthen the test and catch regressions if these properties are accidentally reintroduced.Apply this diff to add negative assertions:
expect(index[0]).toHaveProperty("title"); expect(index[0]).toHaveProperty("description"); + expect(index[0]).not.toHaveProperty("type"); + expect(index[0]).not.toHaveProperty("files"); // RegistryIndexItem has name, category, title, description - not type or filespackages/cli/vitest.config.ts (1)
14-14: LGTM! Reasonable timeout for CLI tests.The 60-second timeout is appropriate for CLI tests that may involve process spawning and I/O operations. The comment clearly explains the rationale.
Optional: If only a subset of tests are slow, consider using
test.setTimeout(60000)within specific test files instead of a global default. This allows faster tests to fail quickly while giving slow tests the time they need.packages/fmodata/tests/webhooks.test.ts (1)
252-274: Skipped invoke() tests.The
invoke()tests are skipped. Consider adding a TODO comment explaining why (e.g., missing mock responses, incomplete implementation) so future maintainers know the status.packages/fmodata/scripts/test-webhooks.ts (1)
108-121: Defensive webhook ID extraction handles multiple response shapes.This handles potential variations in the API response structure. Once the response format is confirmed stable, this could be simplified to just check
webHookResult.webHookID.packages/fmodata/src/client/webhook-builder.ts (2)
3-3: Remove unused import.
FFetchOptionsis imported but never used in this file.Apply this diff to remove the unused import:
-import type { FFetchOptions } from "@fetchkit/ffetch";
265-274: Consider simplifying body construction logic.The current implementation creates an empty body object and checks
Object.keys(body).length > 0to decide whether to include it. This could be more directly expressed by checkingoptions?.rowIDs !== undefined.Apply this diff for a more direct approach:
- const body: { rowIDs?: number[] } = {}; - if (options?.rowIDs !== undefined) { - body.rowIDs = options.rowIDs; - } - const result = await this.context._makeRequest<unknown>( `/${this.databaseName}/Webhook.Invoke(${webhookId})`, { method: "POST", - body: Object.keys(body).length > 0 ? JSON.stringify(body) : undefined, + body: options?.rowIDs !== undefined ? JSON.stringify({ rowIDs: options.rowIDs }) : undefined, ...executeOptions, }, );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
Users/ericluce/Documents/Code/work/proofkit/apps/demo/tests/typegen-output/without-zod/client/index.ts(1 hunks)apps/docs/tests/utils.manifest.test.ts(1 hunks)packages/cli/vitest.config.ts(1 hunks)packages/fmodata/README.md(1 hunks)packages/fmodata/scripts/capture-responses.ts(4 hunks)packages/fmodata/scripts/test-webhooks.ts(1 hunks)packages/fmodata/src/client/database.ts(2 hunks)packages/fmodata/src/client/error-parser.ts(1 hunks)packages/fmodata/src/client/webhook-builder.ts(1 hunks)packages/fmodata/src/index.ts(1 hunks)packages/fmodata/tests/fixtures/responses.ts(4 hunks)packages/fmodata/tests/tsconfig.build.json(1 hunks)packages/fmodata/tests/webhooks.test.ts(1 hunks)packages/registry/lib/validator.test.ts(5 hunks)packages/registry/vitest.config.ts(1 hunks)packages/typegen/tests/__snapshots__/zod-layout-client.snap.ts(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/theme-provider.tsx(1 hunks)packages/typegen/web/src/components/ui/tooltip.tsx(1 hunks)packages/typegen/web/src/hooks/useRunTypegen.ts(1 hunks)packages/typegen/web/tsconfig.app.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/fmodata/src/client/error-parser.ts
- packages/typegen/web/src/components/ConfigList.css
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/typegen/web/src/hooks/useRunTypegen.ts
- packages/typegen/web/src/components/ConfigList.tsx
- packages/typegen/web/src/components/theme-provider.tsx
- packages/fmodata/tests/tsconfig.build.json
- packages/typegen/web/src/components/ui/tooltip.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/fmodata/scripts/test-webhooks.ts (4)
packages/typegen/scripts/build-copy.js (2)
__filename(5-5)__dirname(6-6)apps/docs/scripts/bundle-registry-templates.js (1)
path(4-4)packages/fmodata/tests/e2e/setup.ts (4)
serverUrl(20-20)apiKey(21-21)username(22-22)password(23-23)packages/fmodata/src/index.ts (3)
fmTableOccurrence(18-18)textField(9-9)FMServerConnection(4-4)
⏰ 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: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Cursor Bugbot
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (23)
packages/fmodata/README.md (3)
781-947: Inconsistency between AI summary and provided code.The AI-generated summary claims "duplication of the Webhooks documentation section: two identical blocks," but the provided code shows only one comprehensive Webhooks section. Please verify whether this duplication actually exists (perhaps in a different file section or outside the visible range), or if the summary is flagging a false positive.
781-947: Webhooks documentation is well-structured and comprehensive.The added Webhooks section provides clear subsections (Add, Configure, List, Get, Remove, Invoke, Example), practical examples with column references and filter expressions, and explanatory notes on webhook behavior. The documentation follows the same high quality and consistency as the rest of the README.
34-34: This import syntax is correct for Zod v4.The examples using
import { z } from "zod/v4";follow Zod's official versioning strategy. The subpath import is the recommended approach for Zod v4, not a non-standard pattern. No changes needed.Likely an incorrect or invalid review comment.
packages/cli/vitest.config.ts (1)
18-18: LGTM! Good coverage expansion.Expanding coverage to all TypeScript source files in
src/ensures comprehensive tracking of test coverage across the CLI package.packages/registry/lib/validator.test.ts (3)
113-114: LGTM! Comment clarification improves test readability.The updated comment clearly explains which mock is responsible for the file existence check.
141-144: LGTM! Mock setup properly isolates the test case.Adding
"other-file.ts"to thereaddirSyncmock ensures the template directory passes the "has content" validation, allowing the test to specifically verify the declared file existence check. This is good test isolation.Also applies to: 167-170
238-281: LGTM! Tests correctly validate proofkit registry dependencies.These tests properly validate both valid and invalid
registryDependenciesreferences with the{proofkit}/r/prefix format, and correctly ignore non-proofkit registry dependencies.Users/ericluce/Documents/Code/work/proofkit/apps/demo/tests/typegen-output/without-zod/client/index.ts (1)
1-2: This is generated code in the typegen-output directory. The export names reflect the standard Proofkit typegen configuration ("clientSuffix": "Layout"), and a codebase search confirms there are no references to the old export names.Likely an incorrect or invalid review comment.
packages/typegen/tests/__snapshots__/zod-layout-client.snap.ts (1)
6-6: No issues found. The"zod/v4"import path is valid and intentional—the project uses zod's subpath exports feature to support multiple validator versions ("zod", "zod/v4", "zod/v3") with "zod/v4" as the default.Likely an incorrect or invalid review comment.
packages/fmodata/src/client/database.ts (1)
7-7: LGTM!The
WebhookManagerwiring follows the same pattern asSchemaManager— imported, declared as a public readonly field, and instantiated in the constructor with the same parameters.Also applies to: 12-12, 28-28
packages/fmodata/scripts/capture-responses.ts (2)
477-493: Potential failure if no webhooks exist.
webhook-getandwebhook-deletequeries (lines 477-525) depend on webhooks already existing in the database. If the list is empty, both will throw"No webhook ID found". Consider:
- Running
webhook-addbefore these queries execute and using the created ID- Or accepting that these queries will fail gracefully if no webhooks exist
Since this is a capture script for test fixtures, this may be acceptable if you ensure webhooks exist when running the capture.
789-795: Good addition of prettier formatting for generated fixtures.Using
prettier.formatwith thefilepathoption ensures consistent formatting and correct parser selection.packages/fmodata/src/index.ts (1)
70-75: LGTM!Type-only exports for webhook-related types follow the established pattern and correctly expose the public API surface.
packages/fmodata/tests/webhooks.test.ts (3)
1-81: Well-structured webhook test suite.Good coverage of the WebhookManager API with proper type assertions and mock response verification. The defensive null checks (lines 51-56) ensure type safety during assertions.
122-158: Nice use of before hook to verify request transformation.This test effectively verifies that the filter and select expressions are correctly transformed to OData syntax before being sent in the request body.
276-314: Integration test uses different IDs across operations.The integration test acknowledges in comments (lines 296-298) that it uses different webhook IDs for add/get/delete due to mock constraints. This is acceptable for mock-based testing but worth noting that it doesn't fully simulate a real add→get→delete flow with a single webhook.
packages/fmodata/scripts/test-webhooks.ts (2)
1-59: Good manual test script for webhook exploration.Clear setup with environment variable loading, flexible auth support (API key or username/password), and proper validation. The table occurrence definition aligns with other test files.
60-231: Well-isolated test cases with informative output.Each test has its own try/catch block ensuring one failure doesn't prevent subsequent tests from running. The verbose console output is helpful for understanding API behavior.
packages/fmodata/src/client/webhook-builder.ts (5)
8-46: LGTM on type definitions!The webhook types are well-structured and properly document the API surface. Using
unknown[]forpendingOperationsis appropriate when the API shape is not yet fully known.
179-243: LGTM on remove, get, and list methods!These methods follow a consistent pattern with proper error handling. The implementation is clean and well-documented.
48-52: LGTM on class structure!The constructor is clean, and the overall WebhookManager class is well-organized with consistent patterns across all methods and excellent documentation.
117-121: No action needed. The code is correct.
formatSelectFieldsdoes acceptFMTable<any, any>as its second parameter, andwebhook.tableNameis indeed anFMTableinstance (from theWebhook<FMTable>generic type). The current implementation properly passes anFMTableto a function that expects it.Likely an incorrect or invalid review comment.
110-115: The code correctly handles the type safety of the select field.The
selecttypestring | Column<any, any, any>[]correctly specifies that when an array is provided, it contains only Column instances. TheString(item)fallback on line 114 is defensive programming that gracefully handles any unexpected non-Column values at runtime, even though the type system guarantees the array contains only Columns. The implementation properly transforms Column instances to strings before passing them toformatSelectFields, which expectsstring[]. This is type-safe and follows established patterns.
| "webhook-add": { | ||
| url: "https://api.example.com/otto/fmi/odata/v4/fmdapi_test.fmp12/Webhook.Add", | ||
| method: "GET", | ||
| status: 200, | ||
| headers: { | ||
| "content-type": "application/json;charset=utf-8", | ||
| }, | ||
| response: { | ||
| webHookResult: { | ||
| webHookID: 5, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Incorrect HTTP method captured for webhook-add and webhook-delete.
The method field is "GET" for webhook-add (line 760) and webhook-delete (line 827), but these operations use POST requests in the capture script. This is caused by line 687 in capture-responses.ts which hardcodes method: "GET" regardless of the actual request method.
If tests rely on matching the HTTP method, this could cause issues.
🤖 Prompt for AI Agents
In packages/fmodata/tests/fixtures/responses.ts around lines 758-770 (and
similarly for the webhook-delete entry near line 827) the HTTP method is set to
"GET" but the capture script hardcodes this at capture-responses.ts line ~687;
update the capture logic to assign the actual request method (e.g., use
req.method or equivalent) instead of the literal "GET", then regenerate or edit
the fixtures so the webhook-add and webhook-delete entries use "POST" to match
the real requests.
| { | ||
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "baseUrl": ".", | ||
| "paths": { | ||
| "@/*": ["./src/*"] | ||
| } | ||
| }, | ||
| "include": ["src", "../src"] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check import patterns from parent package in web UI source
echo "=== Checking for relative imports from parent directory ==="
rg -n --type=ts --type=tsx "from ['\"]\.\./" packages/typegen/web/src/ 2>/dev/null || echo "No results found"
echo ""
echo "=== Checking for package imports ==="
rg -n --type=ts --type=tsx "from ['\"]@" packages/typegen/web/src/ 2>/dev/null | head -20 || echo "No results found"
echo ""
echo "=== Listing TypeScript files in web/src ==="
find packages/typegen/web/src -name "*.ts" -o -name "*.tsx" 2>/dev/null | head -20Repository: proofgeist/proofkit
Length of output: 1299
Remove the unused "../src" entry from the include array.
The "../src" entry includes the parent package's source in compilation but the web UI doesn't import from it. Keep the include array as ["src"] to maintain clean module boundaries. If the web UI needs to access parent package code, imports should go through the package's public API.
🤖 Prompt for AI Agents
In packages/typegen/web/tsconfig.app.json lines 1-10, remove the unused "../src"
entry from the "include" array so the web package only compiles its own src;
update the include to just ["src"] to keep module boundaries clean and ensure
any access to parent package code is done via its public API imports.
|
Cursor Agent can help with this pull request. Just |
Co-authored-by: eric.luce <eric.luce@proofgeist.com>
- Updated the handling of ROWID and ROWMODID to use destructured values when available. - Maintained original response inclusion for other fields not in the schema.
…s; enhance validation logic by including special columns in validation functions and record builders.
…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>
eluce2
force-pushed
the
12-13-typegen_web_ui
branch
from
bd77c9a to
f0867d5
Compare
| const baseTableConfig = getBaseTableConfig(table); | ||
| const allFields = Object.keys(baseTableConfig.schema); | ||
| // Filter out container fields | ||
| return [...new Set(allFields.filter((f) => !containerFields.includes(f)))]; |
There was a problem hiding this comment.
Schema partial causes expand queries to miss fields without validators
The refactoring changed baseTableConfig.schema from containing validators for ALL fields (with auto-generated defaults) to only containing fields with explicit outputValidator calls. However, getDefaultSelectFields still uses Object.keys(baseTableConfig.schema) to determine which fields to select when defaultSelect === "schema". This means expanded relations will now only select fields that have explicit validators, silently omitting all other fields from the query results. For a table where most fields lack validators (the common case), expand operations would return incomplete data.
Additional Locations (1)
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Enables a UI-driven workflow and expands OData capabilities while updating docs and tooling.
npx @proofkit/typegen uicommand to launch a local web UI for configuring and running typegen; docs added for UI and command optionsadd/get/list/invoke/remove), optional special columns (ROWID,ROWMODID) across query/record/insert/update with typed results, richergetMetadataoptions (table-scoped, reduced annotations), improved select/expand/validation and schema handling; updated response-capture fixtures and scripts; removed experimental batch scriptWritten by Cursor Bugbot for commit f0867d5. This will update automatically on new commits. Configure here.