Skip to content

chore: 🤖 remove leaked radix ui types#848

Merged
punkbit merged 1 commit intomainfrom
chore/remove-leaked-radix-ui-types
Mar 6, 2026
Merged

chore: 🤖 remove leaked radix ui types#848
punkbit merged 1 commit intomainfrom
chore/remove-leaked-radix-ui-types

Conversation

@punkbit
Copy link
Collaborator

@punkbit punkbit commented Feb 25, 2026

Why?

Introduces click-ui's own DialogProps and DialogTriggerProps types, replacing direct Radix UI type re-exports (leaks). This decouples the public API from internal implementation details.

⚠️ WARNING: Depends on #845 which should be merged first
🤖 On #845 merge, change base branch to main

How?

  • Create custom click ui types
  • Export old leaks as deprecated

Preview?

N/A

@changeset-bot
Copy link

changeset-bot bot commented Feb 25, 2026

🦋 Changeset detected

Latest commit: d47a834

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 2m 8s —— View job


Code Review

Nice work decoupling the public API from Radix UI internals! The approach is solid. There's one build-breaking bug to fix before merge.


🚨 Must Fix

  • src/index.ts — Duplicate exports: linkStyles and StyledLinkProps are exported twice (lines 300–307 and again 352–359). This will cause a TypeScript compilation error (Duplicate identifier). The second "Deprecated Exports" block at the bottom of the file is a copy-paste artifact and should be removed. Fix this →

⚠️ Potential Type Mismatch

  • FlyoutContentPropsonPointerDownOutside / onFocusOutside typed as CustomEvent: These are typed as (event: CustomEvent) => void in Flyout.types.ts, but they get spread via ...props into FlyoutContent (a styled(RadixDialog.Content)), which expects Radix-specific event types (PointerDownOutsideEvent, FocusOutsideEvent). With strictFunctionTypes, this may produce a TypeScript error. Consider either using any / Radix's types directly, or handling these in the component (like onInteractOutside) rather than forwarding them.

💡 Suggestions (non-blocking)

  • FlyoutHeaderProps / FlyoutFooterProps type discrepancy: Flyout.tsx defines and exports richer versions of these types (the union type TitleHeaderProps | ChildrenHeaderProps for FlyoutHeaderProps, and an Omit<ContainerProps<'div'>, ...> extension for FlyoutFooterProps), but Flyout/index.ts re-exports the simpler versions from Flyout.types.ts. Consumers end up with incomplete types. This is pre-existing, but this PR would be a good opportunity to consolidate — move the richer definitions to Flyout.types.ts. Fix this →

  • DialogContentProps still extends Radix's type: DialogContentProps in Dialog.types.ts still extends RadixDialog.DialogContentProps. Keeping it in scope for a follow-up is fine, just worth tracking as the next step in this cleanup.


The ThemeName narrowing improvement ((typeof THEMES)[keyof typeof THEMES] instead of a hardcoded union) is a nice bonus. 🙌

Base automatically changed from chore/public-api-as-exports-generator-source-of-truth to main March 6, 2026 13:58
@punkbit punkbit force-pushed the chore/remove-leaked-radix-ui-types branch from ad26e7b to 47af4f2 Compare March 6, 2026 16:28
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 22s —— View job


Code Review

Good direction — decoupling the public API from Radix UI internals is the right move. A few real issues to address before merge.


🚨 Must Fix

  • FlyoutHeaderProps regression (Flyout.types.ts): The new type reduces FlyoutHeaderProps to { title: string } & ContainerProps<'div'>, making title required and removing the children-based variant. The actual Header component in Flyout.tsx supports either a title prop or children (the TitleHeaderProps | ChildrenHeaderProps union). Consumers who pass children without a title will now get a TypeScript error even though the component handles it correctly. The rich union type from Flyout.tsx needs to be the one exported publicly. Fix this →

  • FlyoutFooterProps regression (Flyout.types.ts): Reduced to { children: ReactNode }, but Flyout.tsx uses and exports FlyoutFooterProps as Omit<ContainerProps<'div'>, 'orientaion' | 'justifyContent' | 'component' | 'padding' | 'gap'> & { type?: FlyoutType }. Since Flyout/index.ts re-exports from Flyout.types.ts, consumers lose all ContainerProps passthrough and type prop support at the type level. Fix this →


⚠️ Potential Type Mismatch

  • onPointerDownOutside / onFocusOutside typed as CustomEvent (Flyout.types.ts lines 29–30): These end up in ...props and get spread directly into FlyoutContent (styled(RadixDialog.Content)), which expects Radix's PointerDownOutsideEvent / FocusOutsideEvent. With strictFunctionTypes, this will produce a type error. Consider destructuring them from props explicitly and handling/forwarding them in the component (similar to how onInteractOutside is handled), or typing them to match Radix's expectations.

💡 Suggestions (non-blocking)

  • DialogContentProps still extends RadixDialog.DialogContentProps (Dialog.types.ts line 17): This still leaks Radix types for DialogContent. Noted as a follow-up in the PR description — just flagging for tracking.

@punkbit punkbit force-pushed the chore/remove-leaked-radix-ui-types branch from 47af4f2 to da0a129 Compare March 6, 2026 16:35
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 39s —— View job


Code Review

Good progress — the FlyoutHeaderProps/FlyoutFooterProps regressions from previous rounds are now fixed, and the deprecated re-export approach is clean. One real breaking change to address before merge.


🚨 Must Fix

  • DialogTriggerProps is too narrow (Dialog.types.ts line 12–15): The new type only exposes children and asChild, but the DialogTrigger component spreads ...props into a styled(RadixDialog.Trigger). Consumers who were passing className, onClick, id, data-testid, or any HTML attribute will now get TypeScript errors even though the component handles those at runtime. The previous Radix type included all of these. At minimum, extend React.ComponentPropsWithoutRef<'button'> or React.HTMLAttributes<HTMLButtonElement>. Fix this →

⚠️ Potential Type Mismatch

  • onPointerDownOutside / onFocusOutside typed as CustomEvent (Flyout.types.ts lines 29–30): Both props still end up in ...props and get spread into FlyoutContent (styled(RadixDialog.Content)), which expects Radix's PointerDownOutsideEvent / FocusOutsideEvent. With strictFunctionTypes, this is a compile-time error. Destructure them explicitly in the Content component (like onInteractOutside) or type them to match Radix's expectations.

💡 Suggestions (non-blocking)

  • DialogContentProps still extends RadixDialog.DialogContentProps (Dialog.types.ts line 17): Still leaks Radix types — noted as an acknowledged follow-up, just flagging for tracking.

  • FlyoutSizeType, FlyoutStrategy, FlyoutType, FlyoutAlignmentType are unexported (Flyout.types.ts lines 12–15): These are now private types. If consumers need them (e.g. to type their own wrapper props), they won't be able to. Consider exporting them.


@punkbit punkbit force-pushed the chore/remove-leaked-radix-ui-types branch from da0a129 to f8e1094 Compare March 6, 2026 16:39
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

  • FlyoutSizeType, FlyoutStrategy, FlyoutType, FlyoutAlignmentType unexported (Flyout.types.ts lines 12–15): Used in FlyoutContentProps but not exported. Consumers who want to type wrapper props around Flyout.Content can't reference these by name. Consider exporting.

These types were never publicly exported, so making them private (type instead of export type) is not a breaking change. No fix needed.

@punkbit punkbit force-pushed the chore/remove-leaked-radix-ui-types branch from f8e1094 to c5ebabf Compare March 6, 2026 16:46
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 43s —— View job


Code Review

  • Read changed files
  • Analyze types and check for regressions
  • Post review

All previous must-fix items have been addressed. The PR is in good shape. ✅

Previously resolved ✅

  • Duplicate linkStyles/StyledLinkProps exports — fixed
  • FlyoutHeaderProps regression (children-variant missing) — fixed, union type now in Flyout.types.ts
  • FlyoutFooterProps regression — fixed, correctly extends Omit<ContainerProps<'div'>, ...>
  • DialogTriggerProps too narrow — fixed, now extends React.ComponentPropsWithoutRef<'button'>

💡 Suggestions (non-blocking)

  • FlyoutSizeType, FlyoutStrategy, FlyoutType, FlyoutAlignmentType not re-exported (Flyout/index.ts): These are defined with export in Flyout.types.ts but not re-exported from Flyout/index.ts or src/index.ts. They're used in FlyoutContentProps props like size?: FlyoutSizeType, so consumers who want to build typed wrappers around Flyout.Content can't reference them by name. Consider adding them to the Flyout/index.ts and src/index.ts exports. Fix this →

  • onPointerDownOutside / onFocusOutside typed as CustomEvent (Flyout.types.ts lines 29–30): Due to contravariance this is type-safe and compiles correctly. The only loss is that consumers can't access event.detail.originalEvent with full typing. Low priority, but worth noting for a future cleanup.

  • DialogContentProps still extends RadixDialog.DialogContentProps (Dialog.types.ts line 17): Noted as an acknowledged follow-up — just tracking for completeness.


The deprecated alias approach (RadixDialogProps, RadixDialogTriggerProps) is a clean migration path. The Flyout.Trigger props-forwarding fix (moving ...props to the inner <div> instead of DialogTrigger) is functionally correct since Radix's asChild/Slot merges parent props onto the child. Nice work overall. 🙌

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 7s —— View job


Code Review

  • Read changed files and conventions
  • Analyze types and check for regressions
  • Post review

All previous must-fix items have been addressed. The PR is ready to merge. ✅

Previously resolved ✅

  • Duplicate linkStyles/StyledLinkProps exports — fixed
  • FlyoutHeaderProps regression (children-variant missing) — fixed, full union type in Flyout.types.ts
  • FlyoutFooterProps regression — fixed, correctly extends Omit<ContainerProps<'div'>, ...>
  • DialogTriggerProps too narrow — fixed, now extends React.ComponentPropsWithoutRef<'button'>
  • FlyoutSizeType, FlyoutStrategy, FlyoutType, FlyoutAlignmentType not re-exported — fixed, all four are now exported from Flyout/index.ts and src/index.ts

💡 Suggestions (non-blocking)

  • onPointerDownOutside / onFocusOutside typed as CustomEvent (Flyout.types.ts lines 29–30): These are spread via ...props into FlyoutContent (styled(RadixDialog.Content)). As noted in a previous round, contravariance makes this type-safe and it compiles. The only loss is that consumers can't access event.detail.originalEvent with full typing — low priority follow-up.

  • DialogContentProps still extends RadixDialog.DialogContentProps (Dialog.types.ts line 17): Acknowledged follow-up in the PR description — just tracking for completeness.

@punkbit punkbit force-pushed the chore/remove-leaked-radix-ui-types branch from 38a839d to d47a834 Compare March 6, 2026 16:57
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 23s —— View job


Code Review

  • Read changed files and conventions
  • Analyze types and check for regressions
  • Post review

All previously flagged must-fix items are resolved. The PR is in good shape. ✅

Previously resolved ✅

  • Duplicate linkStyles/StyledLinkProps exports — fixed
  • FlyoutHeaderProps regression (children-variant missing) — fixed, full union type in Flyout.types.ts
  • FlyoutFooterProps regression — fixed, correctly extends Omit<ContainerProps<'div'>, ...>
  • DialogTriggerProps too narrow — fixed, now extends React.ComponentPropsWithoutRef<'button'>
  • FlyoutSizeType, FlyoutStrategy, FlyoutType, FlyoutAlignmentType not re-exported — fixed

💡 Suggestions (non-blocking)

  • onInteractOutside on FlyoutContentProps typed as (event: CustomEvent) => void (Flyout.types.ts line 27): The component wraps this prop internally (extracts it from ...props and calls onInteractOutside(e) where e is actually the Radix event object). The type lie is harmless but the consumer never gets the full Radix event shape. Low priority.

  • onPointerDownOutside / onFocusOutside typed as CustomEvent (Flyout.types.ts lines 29–30): These remain in ...props and reach FlyoutContent (styled(RadixDialog.Content)). Previous review confirmed this compiles due to contravariance — the only loss is consumers can't access event.detail.originalEvent with full types. Fine as a follow-up.

  • DialogContentProps still extends RadixDialog.DialogContentProps (Dialog.types.ts line 17): Acknowledged as a follow-up in the PR description — just tracking for completeness.

  • ContextMenuProps, HoverCardProps, PopoverProps deprecated without aliases: Unlike DialogProps/DialogTriggerProps which get renamed aliases (RadixDialogProps/RadixDialogTriggerProps), these three are still re-exported under their original names with a @deprecated JSDoc. The asymmetry is minor but worth noting — future cleanup could rename these to RadixContextMenuProps etc. for consistency.


The migration path is clean, the changeset is well-written, and the deprecated alias approach for RadixDialogProps/RadixDialogTriggerProps is a nice non-breaking bridge. 🙌

@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-g68v43op3-clickhouse.vercel.app

Built from commit: b69ab8eca40ca2949d110d331921292e745bb6e3

@punkbit punkbit merged commit 71fb216 into main Mar 6, 2026
8 checks passed
@punkbit punkbit deleted the chore/remove-leaked-radix-ui-types branch March 6, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants