Skip to content

fix: 🐛 resolve circular dependencies (first pass)#841

Open
punkbit wants to merge 245 commits intomainfrom
fix/circular-dependencies-first-pass
Open

fix: 🐛 resolve circular dependencies (first pass)#841
punkbit wants to merge 245 commits intomainfrom
fix/circular-dependencies-first-pass

Conversation

@punkbit
Copy link
Collaborator

@punkbit punkbit commented Feb 23, 2026

Why?

The Click-UI source code has several circular dependencies that must be resolved.

During the resolution of component path redundancies and public API encapsulation in #798, several circular dependencies were exposed. There, some quick basic fixes were applied to allow to progress, but it was found that a separate PR was needed to resolve them.

⚠️ WARNING: Depends on #798, which should be merged firstly
⚠️ WARNING: Depends on #832, which should be merged firstly

🤖 Once #832 is merged, change base to main.

How?

  • Reconciles types
  • Refactor main export,s which now exports types
  • Removes the root common types
  • Make common components, e.g. the simple styled-components into their own src/components/[Components] ns to help prevent circular imports

Preview?

❌ Before

Screenshot 2026-02-23 at 14 52 41

 👍 After

Screenshot 2026-02-23 at 17 42 50

Copy link
Collaborator

@ariser ariser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked through commits 1 by 1, w/o reading too deep into them. Looks good to me. Appreciate your approach to atomic commits

tbh I don't think PRs like this require thorough reviewing. And they can't be thoroughly reviewed, for that matter. As long as there are no functional changes, if it builds — ship it.

@punkbit
Copy link
Collaborator Author

punkbit commented Mar 6, 2026

I looked through commits 1 by 1, w/o reading too deep into them. Looks good to me. Appreciate your approach to atomic commits

tbh I don't think PRs like this require thorough reviewing. And they can't be thoroughly reviewed, for that matter. As long as there are no functional changes, if it builds — ship it.

@ariser Thanks for your time and support!

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

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


Code Review: fix/circular-dependencies-first-pass

  • Gather context
  • Read CLAUDE.md and conventions
  • Analyze PR diff and changed files
  • Review for correctness, circular dependency resolution, and API integrity
  • Post final review

Great cleanup work here — the approach of making src/index.ts the single, explicit barrel (instead of chained export * re-exports) is exactly the right pattern for a component library. The bulk of the Icon/Flag/component import path changes are mechanical and correct. A few issues do need attention before merging:


🚨 Must Fix

  • Missing public exports — InitCUIThemeScript / InitCUIThemeScriptProps: The base branch exported these via export * from './theme'src/theme/index.ts. The new src/index.ts has no equivalent export. Consumers using SSR theme injection (<InitCUIThemeScript>) will get a compile error after this lands. Add:

    export { InitCUIThemeScript } from './theme/InitCUIThemeScript';
    export type { InitCUIThemeScriptProps } from './theme/InitCUIThemeScript';

    Fix this →

  • Missing public exports — useInitialTheme / useUpdateEffect: These hooks were exported via export * from './hooks' in the base branch. The new src/index.ts doesn't export them. Both are consumed by apps relying on theme persistence. Add:

    export { useInitialTheme, useUpdateEffect } from './hooks';
  • Missing public export — CursorOptions: Was exported as export type { CursorOptions } from './Common' in the base src/components/types.ts. Now inlined into Panel.types.ts but not re-exported from src/index.ts. Add to the global types section:

    export type { CursorOptions } from './components/Panel/Panel.types';
  • as unknown as Theme cast in src/theme/theme.core.ts:11-12: This cast silences a legitimate TypeScript error — the token files' inferred types don't match the declared Theme. This could hide real type mismatches at runtime if tokens diverge. Prefer fixing the type mismatch (e.g. making Theme typeof lightTheme directly, which it already is in theme.types.ts) rather than casting.

  • Changeset bump is Minor, but public API removals are Major: The three missing exports above are breaking changes. The changeset at .changeset/fast-drinks-argue.md should be major with a migration guide, or the exports need to be restored.


💡 Suggestions (non-blocking)

  • ThemeName is now a literal string union ('dark' | 'light') instead of being derived from THEMES in src/theme/theme.types.ts:8. If a new theme is added to THEMES, ThemeName won't automatically pick it up. The previous pattern (typeof THEMES)[keyof typeof THEMES] was more robust. This is minor but worth tracking.

  • ToastIconWrapper in Toast.tsx:12-14: The wrapping function is an unusual pattern — it works, but a comment explaining why (avoiding circular styled-component extension) would help future maintainers understand the intent at a glance.

  • TODO: ClickUIProvider vs ThemeProvider — the TODO comment in src/providers/index.ts is valid. Since it's part of the public API surface, consider opening a follow-up issue to track the consolidation rather than leaving it as a code comment.


Nice systematic approach to this — the before/after dependency graph screenshots make the improvement very clear. Once the missing exports are addressed this will be solid. 🙌

Base automatically changed from refactor/modular-file-architecture to main March 6, 2026 11:29
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

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


Code Review: fix/circular-dependencies-first-pass

  • Gather context
  • Read CLAUDE.md and conventions
  • Analyze PR diff and changed files
  • Review for correctness, circular dependency resolution, and API integrity
  • Post final review

Good systematic work here — the move from a three-line wildcard barrel (export * from './components') to an explicit, curated src/index.ts is the right long-term pattern for a component library. The before/after dependency graph makes the improvement clear. There are a few issues that need to be fixed before merging:


🚨 Must Fix

  • Common/index.ts exports from a deleted fileCommon.types.ts was removed in this PR, but src/components/Common/index.ts:9 still has:

    export type { TextSize, TextWeight, CursorOptions } from './Common.types';

    This is a broken module reference — tsc --noEmit will fail on it (the Storybook/Vite build skips type checking so it passes there). The fix is to remove that line from Common/index.ts (the consumers have already been updated to import CursorOptions from Panel.types and TextSize/TextWeight from Typography). Fix this →

  • Missing public export — InitCUIThemeScript / InitCUIThemeScriptProps — The base branch exported these via export * from './theme'src/theme/index.ts. The new src/index.ts has no equivalent. Consumers using SSR theme injection will get a compile error. Add:

    export { InitCUIThemeScript } from './theme/InitCUIThemeScript';
    export type { InitCUIThemeScriptProps } from './theme/InitCUIThemeScript';

    Fix this →

  • Missing public exports — useInitialTheme / useUpdateEffect — Both hooks were exported via export * from './hooks' on the base branch. The new src/index.ts doesn't export them. Add:

    export { useInitialTheme, useUpdateEffect } from './hooks';
  • Missing public export — CursorOptions — Was exported from src/components/types.tsCommon. Now inlined in Panel.types.ts but not re-exported from src/index.ts. Add to the global types section:

    export type { CursorOptions } from './components/Panel/Panel.types';
  • as unknown as Theme cast in src/theme/theme.core.ts:11darkTheme as unknown as Theme silences a legitimate TS error. Since Theme = typeof lightTheme, the dark token file should match the same shape. Prefer aligning the types rather than casting, or use a type assertion with a comment explaining why the shapes diverge.

  • Changeset is minor but three public exports are removedInitCUIThemeScript, useInitialTheme, useUpdateEffect, and CursorOptions are all breaking removals. The changeset in .changeset/fast-drinks-argue.md should be major (or the exports need to be restored, which is the real fix).


💡 Suggestions (non-blocking)

  • ThemeName is now a literal union ('dark' | 'light') rather than derived from THEMES ((typeof THEMES)[keyof typeof THEMES]). If a new theme is ever added to THEMES, ThemeName won't automatically pick it up. Minor but worth tracking.

  • TODO in src/providers/index.ts — the ClickUIProvider vs ThemeProvider consolidation note is valid; consider opening a follow-up issue to track it rather than leaving it as a code comment.


Nice approach overall — the atomic commits and before/after dependency graph make this easy to reason about. Once the missing exports and the broken Common/index.ts reference are addressed, this will be solid. 🙌

@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

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

Built from commit: 2c99dfdab263b649fd55940a5562941d902b4708

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