Repository navigation
refactor: retire the duplicate @pace/ui components onto @pace/propel - #259
Merged
Merged
Conversation
…ce/propel @pace/propel is the library the repo standardises on, and Card is the one place where the two copies were a byte-level duplicate rather than a redesign: packages/propel/src/card/helper.tsx and packages/ui/src/card/helper.tsx were identical, and the two card.tsx files differed on exactly one line — whether `cn` comes from `../utils/classname` or `../utils`. Both are clsx + tailwind-merge, so there is no behaviour to preserve and nothing to decide. Two of the six consumers (profile/overview/workload.tsx and state-distribution.tsx) imported nothing else from @pace/ui and now import no @pace/ui at all. The rest keep the symbols that have no propel twin, so the specifier is split rather than the whole statement moved. packages/ui survives as the app-composites layer; only the components that have a propel equivalent leave it.
The sweep this drives touches 80 files, so the reviewable artefact is the transform and its tests rather than the hunks: the transform has to be provably incapable of changing anything but the import lines and the identifier at its use sites. It keys off the import source and never off the name. `Loader` is a name app code also defines for itself, and @pace/propel's own emoji picker re-exports a lucide `Loader`; a transform matching `<Loader>` in JSX would rewrite first-party components in files that import nothing from @pace/ui. The headlessui-v2-default-tags transform already makes that guard and its comment explains why. It uses the AST only to locate character offsets and splices the replacement into the original text instead of returning `root.toSource()`. An earlier sweep in this effort did go through recast, and recast rewrote unrelated JSX in 7 files — it reprints a whole subtree whenever it cannot reconcile comment attachment, and several of these files carry `// oxlint-disable-next-line` comments inside their JSX. The spec pins that by asserting full output equality on exactly that shape. Two things were easy to get wrong and are pinned by tests. JSXIdentifier is declared as a subtype of Identifier in ast-types, so searching both types splices every JSX element name twice. And the scope hangs off Program, not off the File node the root path points at — `root.get().scope` is null, so comparing a lookup against it matches every unresolved identifier rather than none, which is the difference between leaving a function-scoped `const Loader` alone and renaming it.
…ss 80 files Loader and Skeleton are the same component under two names: both wrap their children in `animate-pulse` with `role="status"`, both expose an `.Item` taking `height`/`width`/`className` defaulting to `"auto"`, and the class expressions are identical. Skeleton is the one that lives in the library everything standardises on, and it had zero consumers until now. Skeleton additionally emits `data-slot` attributes and an `aria-label="Loading content"`, so the rendered markup gains an accessible name it did not have. Nothing styles the old markup — `plane-ui-loader` appeared only on the deleted component's own `displayName`, and the call sites pass only `height`, `width` and `className`, all of which Skeleton supports. 63 of the 80 files imported nothing else from @pace/ui and now import no @pace/ui at all. The rest keep the composites that have no propel twin, so the specifier is split out rather than the statement moved. Produced by `pnpm --filter @pace/codemods run ui-loader-to-propel-skeleton`; every hunk is an import line or a bare identifier.
Both packages exported a `Spinner` with the same name, the same `ISpinner { height = "32px", width = "32px", className }` and the same two SVG paths verbatim. They differed in two places, so propel's copy is corrected first and the 33 call sites move second — that ordering is what makes the swap a no-op rather than a visual change.
The accent fill moves from the arc `<path>` onto the `<svg>`, and the arc gets `fill="currentFill"` back. Those two halves are one mechanism: `currentFill` is not a valid paint value, so the attribute is dropped and the arc inherits the svg's computed fill, while the track keeps `fill="currentColor"` from `text-secondary`. Rendered output is the same either way, but only the @pace/ui arrangement lets a caller's `fill-*` class reach the arc, and it is the arrangement 33 live call sites were drawn against.
`clsx` becomes the package's own `cn`, which is the difference that can actually be seen today: `apps/web/core/components/analytics/work-items/modal/content.tsx` renders `<Spinner className="text-blue-500" />`, and only tailwind-merge drops the component's `text-secondary` in favour of it. clsx would emit both classes and let stylesheet order decide. @pace/propel's `cn` and @pace/ui's (re-exported from @pace/utils) are the same extended tailwind-merge, so nothing else about the class strings changes.
Confirmed by diffing the two files after the edit: they are byte-identical apart from `cn` coming from `../utils/classname` rather than `../utils`.
packages/propel/src/input/input.tsx and packages/ui/src/form-fields/input.tsx are the same component: the same `mode`/`inputSize`/`hasError` props with the same defaults and a character-for-character identical class expression. propel's renders Base UI's `Input` instead of a bare `<input>` and adds `aria-invalid` when `hasError` is set, which is the only difference the three auth forms can see — none of them passes `hasError`, and Base UI's input is a `Field.Control` that forwards every native prop and composes its own change/focus handlers with the caller's. @pace/ui stops re-exporting its copy, so no app can reach it any more, but the file itself stays: input-color-picker still composes it and that composite has no propel counterpart. Deleting the file would mean migrating a screen that is not part of this work.
`showTooltip` was already declared in propel Avatar's prop type and never destructured, so passing it did nothing — the same class of dead prop as the ones in propel's menu. It now does what @pace/ui's Avatar did: wraps the avatar in propel's Tooltip labelled `fallbackText ?? name ?? "?"`, and passes `disabled` rather than dropping the wrapper, so the DOM is the same shape whether the tooltip is on or off.
The default is `true` because that is what the call sites being migrated were written against. apps/space's group-by column headers (`issue-layouts/utils.tsx`) render `<Avatar name={...} size="md" />` with no `showTooltip` and rely on the name appearing on hover; `properties/member.tsx` and `navbar/user-avatar.tsx` pass it explicitly and are unaffected by the choice. The one pre-existing propel Avatar call site, apps/web's profile activity feed, therefore gains a tooltip carrying the actor's display name where it had none.
AvatarGroup moves across as-is (`max`, `showTooltip`, `size`, the `+N` overflow chip) with only its import paths rewritten; it was already reaching into @pace/propel for the Tooltip, so the move shortens that edge rather than creating one. @pace/ui's avatar/helper.tsx duplicated propel's `getSizeInfo`/`getBorderRadius`/`isAValidNumber` verbatim and goes with it.
Two visible consequences for the three apps/space sites, both wanted: the fallback initial's background stops being the hardcoded `#028375`/`#ffffff` and reads `--background-color-accent-primary`/`--text-color-on-color`, which is what makes it correct in dark mode; and the avatar image regains its `alt`, which Base UI's Avatar.Image does not set on its own. Neither is verifiable without a browser — nothing here renders one.
Five files reached for `cn` through the component library. @pace/ui's is a one-line re-export of @pace/utils' `cn`, which is where the extended tailwind-merge configuration actually lives and where 358 other app files already import it from, so this points them at the owner instead of at a hop through a UI package they otherwise may not need. The plan suggested @pace/propel/utils. That would have been the wrong target: propel keeps its own copy of the same function, only two files in the repo import from there, and `cn` is not a component, so the decision to standardise components on @pace/propel does not apply to it. @pace/ui keeps re-exporting `cn` for its own internal use, which costs nothing now that no app goes through it.
`position` defaulted to `"top"`, and the placement branch reads `if (position)`, so the branch was taken for every caller and `side`/`align` were props that could be typed, documented and passed with no effect whatsoever. Nothing in the repo notices today — all 26 in-repo call sites pass either `position` or nothing — but it is the trap waiting for the apps/web Tooltip migration, where 25 call sites pass `side` and 13 pass `align` and would have type-checked into silently top-centred tooltips.
The defaults move onto `side` and `align` instead, and `"top"`/`"center"` is precisely what `convertPlacementToSideAndAlign("top")` returned, so the 19 call sites that pass neither prop are placed exactly where they were and the 7 that pass `position` still go through the conversion.
|
React Doctor found 13 new issues in 12 files · 13 warnings · score 76 / 100 (Needs work) · 14 fixed · vs 13 warnings
Reviewed by React Doctor for commit |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ) : ( | ||
| <Loader className="px-6"> | ||
| <Loader.Item height="30px" /> | ||
| <Skeleton className="px-6"> |
There was a problem hiding this comment.
React Doctor · react-doctor/duplicate-jsx-subtree (warning)
2 copies of this 6-node JSX tree appear across 2 files, repeating about 8 lines. Composition path: FullScreenPeekView > div > div > Skeleton.
Fix → Consider extracting a shared component if these trees represent the same UI concept. Keep them separate when the resemblance is incidental or the variants are likely to evolve independently.
| if (issueLoader) { | ||
| return ( | ||
| <Loader className="flex h-full gap-5 p-5"> | ||
| <Skeleton className="flex h-full gap-5 p-5"> |
There was a problem hiding this comment.
React Doctor · react-doctor/duplicate-jsx-subtree (warning)
2 copies of this 11-node JSX tree appear across 2 files, repeating about 14 lines. Composition path: IssueDetailsPage > Skeleton.
Fix → Consider extracting a shared component if these trees represent the same UI concept. Keep them separate when the resemblance is incidental or the variants are likely to evolve independently.
| @@ -140,18 +141,18 @@ export const DescriptionVersionsModal = observer(function DescriptionVersionsMod | |||
| /> | |||
| ) : ( | |||
| <div className="space-y-1"> | |||
There was a problem hiding this comment.
React Doctor · react-doctor/duplicate-jsx-subtree (warning)
2 copies of this 11-node JSX tree appear across 2 files, repeating about 14 lines. Composition path: DescriptionVersionsModal > ModalCore > div > div > div.
Fix → Consider extracting a shared component if these trees represent the same UI concept. Keep them separate when the resemblance is incidental or the variants are likely to evolve independently.
| <div className="size-full py-5"> | ||
| <Loader className="relative space-y-4"> | ||
| <Loader.Item width="50%" height="36px" /> | ||
| <Skeleton className="relative space-y-4"> |
There was a problem hiding this comment.
React Doctor · react-doctor/duplicate-jsx-subtree (warning)
2 copies of this 26-node JSX tree appear across 2 files, repeating about 36 lines. Composition path: PageContentLoader > div > div > div > Skeleton.
Fix → Consider extracting a shared component if these trees represent the same UI concept. Keep them separate when the resemblance is incidental or the variants are likely to evolve independently.
| <Loader key={i} className="relative flex items-center gap-2 border-b border-subtle p-3 py-4"> | ||
| <Loader.Item width={`${250 + 10 * Math.floor(Math.random() * 10)}px`} height="22px" /> | ||
| <Skeleton key={i} className="relative flex items-center gap-2 border-b border-subtle p-3 py-4"> | ||
| <Skeleton.Item width={`${250 + 10 * Math.floor(Math.random() * 10)}px`} height="22px" /> |
There was a problem hiding this comment.
React Doctor · react-doctor/rendering-hydration-mismatch-time (warning)
This can cause a hydration mismatch because Math.random() reached from JSX gives a different value on the server than in the browser. Move it into useEffect+useState to run only in the browser, or add suppressHydrationWarning to the parent if it's on purpose.
Fix → Move time or random values into useEffect+useState so they only run in the browser, or add suppressHydrationWarning to the parent if it's intentional
houko
added a commit
that referenced
this pull request
Sep 19, 2026
…259) * refactor(ui): drop the duplicate Card and point its call sites at @pace/propel @pace/propel is the library the repo standardises on, and Card is the one place where the two copies were a byte-level duplicate rather than a redesign: packages/propel/src/card/helper.tsx and packages/ui/src/card/helper.tsx were identical, and the two card.tsx files differed on exactly one line — whether `cn` comes from `../utils/classname` or `../utils`. Both are clsx + tailwind-merge, so there is no behaviour to preserve and nothing to decide. Two of the six consumers (profile/overview/workload.tsx and state-distribution.tsx) imported nothing else from @pace/ui and now import no @pace/ui at all. The rest keep the symbols that have no propel twin, so the specifier is split rather than the whole statement moved. packages/ui survives as the app-composites layer; only the components that have a propel equivalent leave it. * chore(codemods): add the @pace/ui Loader to @pace/propel Skeleton rename The sweep this drives touches 80 files, so the reviewable artefact is the transform and its tests rather than the hunks: the transform has to be provably incapable of changing anything but the import lines and the identifier at its use sites. It keys off the import source and never off the name. `Loader` is a name app code also defines for itself, and @pace/propel's own emoji picker re-exports a lucide `Loader`; a transform matching `<Loader>` in JSX would rewrite first-party components in files that import nothing from @pace/ui. The headlessui-v2-default-tags transform already makes that guard and its comment explains why. It uses the AST only to locate character offsets and splices the replacement into the original text instead of returning `root.toSource()`. An earlier sweep in this effort did go through recast, and recast rewrote unrelated JSX in 7 files — it reprints a whole subtree whenever it cannot reconcile comment attachment, and several of these files carry `// oxlint-disable-next-line` comments inside their JSX. The spec pins that by asserting full output equality on exactly that shape. Two things were easy to get wrong and are pinned by tests. JSXIdentifier is declared as a subtype of Identifier in ast-types, so searching both types splices every JSX element name twice. And the scope hangs off Program, not off the File node the root path points at — `root.get().scope` is null, so comparing a lookup against it matches every unresolved identifier rather than none, which is the difference between leaving a function-scoped `const Loader` alone and renaming it. * refactor(ui): retire Loader in favour of @pace/propel's Skeleton across 80 files Loader and Skeleton are the same component under two names: both wrap their children in `animate-pulse` with `role="status"`, both expose an `.Item` taking `height`/`width`/`className` defaulting to `"auto"`, and the class expressions are identical. Skeleton is the one that lives in the library everything standardises on, and it had zero consumers until now. Skeleton additionally emits `data-slot` attributes and an `aria-label="Loading content"`, so the rendered markup gains an accessible name it did not have. Nothing styles the old markup — `plane-ui-loader` appeared only on the deleted component's own `displayName`, and the call sites pass only `height`, `width` and `className`, all of which Skeleton supports. 63 of the 80 files imported nothing else from @pace/ui and now import no @pace/ui at all. The rest keep the composites that have no propel twin, so the specifier is split out rather than the statement moved. Produced by `pnpm --filter @pace/codemods run ui-loader-to-propel-skeleton`; every hunk is an import line or a bare identifier. * refactor(ui): retire the duplicate Spinner onto @pace/propel/spinners Both packages exported a `Spinner` with the same name, the same `ISpinner { height = "32px", width = "32px", className }` and the same two SVG paths verbatim. They differed in two places, so propel's copy is corrected first and the 33 call sites move second — that ordering is what makes the swap a no-op rather than a visual change. The accent fill moves from the arc `<path>` onto the `<svg>`, and the arc gets `fill="currentFill"` back. Those two halves are one mechanism: `currentFill` is not a valid paint value, so the attribute is dropped and the arc inherits the svg's computed fill, while the track keeps `fill="currentColor"` from `text-secondary`. Rendered output is the same either way, but only the @pace/ui arrangement lets a caller's `fill-*` class reach the arc, and it is the arrangement 33 live call sites were drawn against. `clsx` becomes the package's own `cn`, which is the difference that can actually be seen today: `apps/web/core/components/analytics/work-items/modal/content.tsx` renders `<Spinner className="text-blue-500" />`, and only tailwind-merge drops the component's `text-secondary` in favour of it. clsx would emit both classes and let stylesheet order decide. @pace/propel's `cn` and @pace/ui's (re-exported from @pace/utils) are the same extended tailwind-merge, so nothing else about the class strings changes. Confirmed by diffing the two files after the edit: they are byte-identical apart from `cn` coming from `../utils/classname` rather than `../utils`. * refactor(space): point the auth forms at @pace/propel's Input packages/propel/src/input/input.tsx and packages/ui/src/form-fields/input.tsx are the same component: the same `mode`/`inputSize`/`hasError` props with the same defaults and a character-for-character identical class expression. propel's renders Base UI's `Input` instead of a bare `<input>` and adds `aria-invalid` when `hasError` is set, which is the only difference the three auth forms can see — none of them passes `hasError`, and Base UI's input is a `Field.Control` that forwards every native prop and composes its own change/focus handlers with the caller's. @pace/ui stops re-exporting its copy, so no app can reach it any more, but the file itself stays: input-color-picker still composes it and that composite has no propel counterpart. Deleting the file would mean migrating a screen that is not part of this work. * feat(propel): honour showTooltip on Avatar and take over AvatarGroup `showTooltip` was already declared in propel Avatar's prop type and never destructured, so passing it did nothing — the same class of dead prop as the ones in propel's menu. It now does what @pace/ui's Avatar did: wraps the avatar in propel's Tooltip labelled `fallbackText ?? name ?? "?"`, and passes `disabled` rather than dropping the wrapper, so the DOM is the same shape whether the tooltip is on or off. The default is `true` because that is what the call sites being migrated were written against. apps/space's group-by column headers (`issue-layouts/utils.tsx`) render `<Avatar name={...} size="md" />` with no `showTooltip` and rely on the name appearing on hover; `properties/member.tsx` and `navbar/user-avatar.tsx` pass it explicitly and are unaffected by the choice. The one pre-existing propel Avatar call site, apps/web's profile activity feed, therefore gains a tooltip carrying the actor's display name where it had none. AvatarGroup moves across as-is (`max`, `showTooltip`, `size`, the `+N` overflow chip) with only its import paths rewritten; it was already reaching into @pace/propel for the Tooltip, so the move shortens that edge rather than creating one. @pace/ui's avatar/helper.tsx duplicated propel's `getSizeInfo`/`getBorderRadius`/`isAValidNumber` verbatim and goes with it. Two visible consequences for the three apps/space sites, both wanted: the fallback initial's background stops being the hardcoded `#028375`/`#ffffff` and reads `--background-color-accent-primary`/`--text-color-on-color`, which is what makes it correct in dark mode; and the avatar image regains its `alt`, which Base UI's Avatar.Image does not set on its own. Neither is verifiable without a browser — nothing here renders one. * refactor(web): import cn from @pace/utils rather than through @pace/ui Five files reached for `cn` through the component library. @pace/ui's is a one-line re-export of @pace/utils' `cn`, which is where the extended tailwind-merge configuration actually lives and where 358 other app files already import it from, so this points them at the owner instead of at a hop through a UI package they otherwise may not need. The plan suggested @pace/propel/utils. That would have been the wrong target: propel keeps its own copy of the same function, only two files in the repo import from there, and `cn` is not a component, so the decision to standardise components on @pace/propel does not apply to it. @pace/ui keeps re-exporting `cn` for its own internal use, which costs nothing now that no app goes through it. * fix(propel): let Tooltip's side and align props reach the positioner `position` defaulted to `"top"`, and the placement branch reads `if (position)`, so the branch was taken for every caller and `side`/`align` were props that could be typed, documented and passed with no effect whatsoever. Nothing in the repo notices today — all 26 in-repo call sites pass either `position` or nothing — but it is the trap waiting for the apps/web Tooltip migration, where 25 call sites pass `side` and 13 pass `align` and would have type-checked into silently top-centred tooltips. The defaults move onto `side` and `align` instead, and `"top"`/`"center"` is precisely what `convertPlacementToSideAndAlign("top")` returned, so the 19 call sites that pass neither prop are placed exactly where they were and the 7 that pass `position` still go through the conversion. * chore: bump the workspace version, which check-version requires of every PR
houko
added a commit
that referenced
this pull request
Sep 19, 2026
…top shipping two copies (#256) * refactor(propel): move off the deprecated Base UI beta onto @base-ui/react 1.7.0 packages/propel was built on `@base-ui-components/react@1.0.0-beta.3`, which pnpm-lock.yaml itself records as `deprecated: Package was renamed to @base-ui/react`. @makeplane/propel 0.3.0 already depends on `@base-ui/react@^1.6.0` and resolves to 1.7.0, so every bundle shipped two Base UI runtimes. After this change `pnpm why -r` reports one version of @base-ui/react, resolved to the same peer set as the one the external propel pulls, so the duplicate is gone from apps/web, apps/space and apps/admin alike. Doing the substrate move now rather than later matters because the migration onto @pace/propel is about to add hundreds of call sites on top of it. The rename is not an identity mapping — 1.0 shipped between beta.3 and 1.7.0 — so each of the sixteen importing components was checked against the installed package rather than sed-ed. Four APIs actually moved. `Accordion.Root`'s `openMultiple` is now `multiple` (renamed in 1.0.0-beta.4). `Tooltip`'s `delay`/`closeDelay` and `Menu`'s `openOnHover` moved from the root onto the trigger in 1.0.0-beta.5, where detached triggers can each carry their own; the menu has three mutually exclusive trigger branches, so the prop is applied to each. `Tabs.Tab` renamed its `[data-selected]` attribute to `[data-active]` and its render-state key from `selected` to `active`, which the trigger's variant classes and the emoji picker's class callback both read. `Combobox.Root` now reports a cleared selection as `null`, which the public `onValueChange` signature does not carry, so the root maps it back to the empty value for the active mode. `Popover` is the one place where an upstream change would have altered a @pace/propel prop contract: `openOnHover`, `delay` and `closeDelay` left `Popover.Root`, but three apps/web call sites pass them to @pace/propel's `Popover` (the calendar block, the gantt block and the editor user mention, all hover preview cards). Rather than edit apps in a dependency PR, the root keeps accepting them and forwards them to `Popover.Button` through a small context, so the component's public shape is unchanged. Everything else survived the jump unchanged, verified attribute by attribute: the `--accordion-panel-height`, `--collapsible-panel-height`, `--active-tab-width`/`--active-tab-left` and `--toast-*` custom properties, `data-panel-open`, `data-side`/`data-align`, `data-starting-style`/`data-ending-style`, and the toast manager surface behind `setToast`/`updateToast`/`setPromiseToast`/`dismissToast`. * fix(propel): ship the separator subpath and mark the package side-effect free The build config no longer described the package. `src/separator` existed with a working component and a Storybook story but had no `index.ts`, no tsdown entry and no `exports` key, so `@pace/propel/separator` could not be imported at all — which matters now, because the twelve zero-consumer subpaths are the landing zones the component consolidation is about to use, not dead code. Meanwhile the entry list still named `src/emoji-reaction-picker/index.ts` for a directory that does not exist; tsdown silently ignored it, so the stale entry only served to mislead. `sideEffects: false` matches the sibling `@pace/ui` manifest and is safe here: nothing under `packages/propel/src` imports CSS or touches module-scope globals, and the one stylesheet the package publishes reaches apps/web and apps/space through a CSS `@import` in their `globals.css`, which bundler side-effect pruning does not see. Without the flag every consumer has to keep all forty-odd subpath modules it touches. * chore: bump the workspace version, which check-version requires of every PR * refactor: retire the duplicate @pace/ui components onto @pace/propel (#259) * refactor(ui): drop the duplicate Card and point its call sites at @pace/propel @pace/propel is the library the repo standardises on, and Card is the one place where the two copies were a byte-level duplicate rather than a redesign: packages/propel/src/card/helper.tsx and packages/ui/src/card/helper.tsx were identical, and the two card.tsx files differed on exactly one line — whether `cn` comes from `../utils/classname` or `../utils`. Both are clsx + tailwind-merge, so there is no behaviour to preserve and nothing to decide. Two of the six consumers (profile/overview/workload.tsx and state-distribution.tsx) imported nothing else from @pace/ui and now import no @pace/ui at all. The rest keep the symbols that have no propel twin, so the specifier is split rather than the whole statement moved. packages/ui survives as the app-composites layer; only the components that have a propel equivalent leave it. * chore(codemods): add the @pace/ui Loader to @pace/propel Skeleton rename The sweep this drives touches 80 files, so the reviewable artefact is the transform and its tests rather than the hunks: the transform has to be provably incapable of changing anything but the import lines and the identifier at its use sites. It keys off the import source and never off the name. `Loader` is a name app code also defines for itself, and @pace/propel's own emoji picker re-exports a lucide `Loader`; a transform matching `<Loader>` in JSX would rewrite first-party components in files that import nothing from @pace/ui. The headlessui-v2-default-tags transform already makes that guard and its comment explains why. It uses the AST only to locate character offsets and splices the replacement into the original text instead of returning `root.toSource()`. An earlier sweep in this effort did go through recast, and recast rewrote unrelated JSX in 7 files — it reprints a whole subtree whenever it cannot reconcile comment attachment, and several of these files carry `// oxlint-disable-next-line` comments inside their JSX. The spec pins that by asserting full output equality on exactly that shape. Two things were easy to get wrong and are pinned by tests. JSXIdentifier is declared as a subtype of Identifier in ast-types, so searching both types splices every JSX element name twice. And the scope hangs off Program, not off the File node the root path points at — `root.get().scope` is null, so comparing a lookup against it matches every unresolved identifier rather than none, which is the difference between leaving a function-scoped `const Loader` alone and renaming it. * refactor(ui): retire Loader in favour of @pace/propel's Skeleton across 80 files Loader and Skeleton are the same component under two names: both wrap their children in `animate-pulse` with `role="status"`, both expose an `.Item` taking `height`/`width`/`className` defaulting to `"auto"`, and the class expressions are identical. Skeleton is the one that lives in the library everything standardises on, and it had zero consumers until now. Skeleton additionally emits `data-slot` attributes and an `aria-label="Loading content"`, so the rendered markup gains an accessible name it did not have. Nothing styles the old markup — `plane-ui-loader` appeared only on the deleted component's own `displayName`, and the call sites pass only `height`, `width` and `className`, all of which Skeleton supports. 63 of the 80 files imported nothing else from @pace/ui and now import no @pace/ui at all. The rest keep the composites that have no propel twin, so the specifier is split out rather than the statement moved. Produced by `pnpm --filter @pace/codemods run ui-loader-to-propel-skeleton`; every hunk is an import line or a bare identifier. * refactor(ui): retire the duplicate Spinner onto @pace/propel/spinners Both packages exported a `Spinner` with the same name, the same `ISpinner { height = "32px", width = "32px", className }` and the same two SVG paths verbatim. They differed in two places, so propel's copy is corrected first and the 33 call sites move second — that ordering is what makes the swap a no-op rather than a visual change. The accent fill moves from the arc `<path>` onto the `<svg>`, and the arc gets `fill="currentFill"` back. Those two halves are one mechanism: `currentFill` is not a valid paint value, so the attribute is dropped and the arc inherits the svg's computed fill, while the track keeps `fill="currentColor"` from `text-secondary`. Rendered output is the same either way, but only the @pace/ui arrangement lets a caller's `fill-*` class reach the arc, and it is the arrangement 33 live call sites were drawn against. `clsx` becomes the package's own `cn`, which is the difference that can actually be seen today: `apps/web/core/components/analytics/work-items/modal/content.tsx` renders `<Spinner className="text-blue-500" />`, and only tailwind-merge drops the component's `text-secondary` in favour of it. clsx would emit both classes and let stylesheet order decide. @pace/propel's `cn` and @pace/ui's (re-exported from @pace/utils) are the same extended tailwind-merge, so nothing else about the class strings changes. Confirmed by diffing the two files after the edit: they are byte-identical apart from `cn` coming from `../utils/classname` rather than `../utils`. * refactor(space): point the auth forms at @pace/propel's Input packages/propel/src/input/input.tsx and packages/ui/src/form-fields/input.tsx are the same component: the same `mode`/`inputSize`/`hasError` props with the same defaults and a character-for-character identical class expression. propel's renders Base UI's `Input` instead of a bare `<input>` and adds `aria-invalid` when `hasError` is set, which is the only difference the three auth forms can see — none of them passes `hasError`, and Base UI's input is a `Field.Control` that forwards every native prop and composes its own change/focus handlers with the caller's. @pace/ui stops re-exporting its copy, so no app can reach it any more, but the file itself stays: input-color-picker still composes it and that composite has no propel counterpart. Deleting the file would mean migrating a screen that is not part of this work. * feat(propel): honour showTooltip on Avatar and take over AvatarGroup `showTooltip` was already declared in propel Avatar's prop type and never destructured, so passing it did nothing — the same class of dead prop as the ones in propel's menu. It now does what @pace/ui's Avatar did: wraps the avatar in propel's Tooltip labelled `fallbackText ?? name ?? "?"`, and passes `disabled` rather than dropping the wrapper, so the DOM is the same shape whether the tooltip is on or off. The default is `true` because that is what the call sites being migrated were written against. apps/space's group-by column headers (`issue-layouts/utils.tsx`) render `<Avatar name={...} size="md" />` with no `showTooltip` and rely on the name appearing on hover; `properties/member.tsx` and `navbar/user-avatar.tsx` pass it explicitly and are unaffected by the choice. The one pre-existing propel Avatar call site, apps/web's profile activity feed, therefore gains a tooltip carrying the actor's display name where it had none. AvatarGroup moves across as-is (`max`, `showTooltip`, `size`, the `+N` overflow chip) with only its import paths rewritten; it was already reaching into @pace/propel for the Tooltip, so the move shortens that edge rather than creating one. @pace/ui's avatar/helper.tsx duplicated propel's `getSizeInfo`/`getBorderRadius`/`isAValidNumber` verbatim and goes with it. Two visible consequences for the three apps/space sites, both wanted: the fallback initial's background stops being the hardcoded `#028375`/`#ffffff` and reads `--background-color-accent-primary`/`--text-color-on-color`, which is what makes it correct in dark mode; and the avatar image regains its `alt`, which Base UI's Avatar.Image does not set on its own. Neither is verifiable without a browser — nothing here renders one. * refactor(web): import cn from @pace/utils rather than through @pace/ui Five files reached for `cn` through the component library. @pace/ui's is a one-line re-export of @pace/utils' `cn`, which is where the extended tailwind-merge configuration actually lives and where 358 other app files already import it from, so this points them at the owner instead of at a hop through a UI package they otherwise may not need. The plan suggested @pace/propel/utils. That would have been the wrong target: propel keeps its own copy of the same function, only two files in the repo import from there, and `cn` is not a component, so the decision to standardise components on @pace/propel does not apply to it. @pace/ui keeps re-exporting `cn` for its own internal use, which costs nothing now that no app goes through it. * fix(propel): let Tooltip's side and align props reach the positioner `position` defaulted to `"top"`, and the placement branch reads `if (position)`, so the branch was taken for every caller and `side`/`align` were props that could be typed, documented and passed with no effect whatsoever. Nothing in the repo notices today — all 26 in-repo call sites pass either `position` or nothing — but it is the trap waiting for the apps/web Tooltip migration, where 25 call sites pass `side` and 13 pass `align` and would have type-checked into silently top-centred tooltips. The defaults move onto `side` and `align` instead, and `"top"`/`"center"` is precisely what `convertPlacementToSideAndAlign("top")` returned, so the 19 call sites that pass neither prop are placed exactly where they were and the 7 that pass `position` still go through the conversion. * chore: bump the workspace version, which check-version requires of every PR * refactor(web): take the last @pace/ui Loader onto propel's Skeleton The lazy-loading pass that landed on main after this branch was cut introduced a fresh `Loader` Suspense fallback in the sidebar progress chart, and this branch retires `Loader` from @pace/ui. Neither side conflicts textually, so the rebase produced a tree that does not compile. Converted by the branch's own `ui-loader-to-propel-skeleton` codemod, the same way every other call site was.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Stacked on
p3/base-ui(#256) — review that first; this branch needs its Base UI 1.7.0 APIs.Retires the
@pace/uicomponents that@pace/propelalready has: Card (6 call sites), Loader → Skeleton (80 files), Spinner (33), Avatar/AvatarGroup, andapps/space's auth-form Input.packages/uisurvives as a thin app-composites layer — only the specific duplicates go.Two of the eight commits are bug fixes that had to land first, because without them the migrations would have been silently wrong:
@pace/propel's Tooltip ignored its ownsideandalignprops.positiondefaulted to"top"and theif (position)branch always won, so the positioner never saw them. Fixed at exactly the values the old conversion produced, so all 26 existing call sites are unchanged.@pace/propel's Avatar declaredshowTooltip?: booleanand never destructured it. It did nothing. This is the same dead-prop pattern asrenderToolTipByDefaultin perf: compress the assets, cut 20% off the entry payload, and stop the per-row AMQP and per-member query fan-out #250 andopenOnHoverin propel'smenu.tsx— three instances now, so it is a pattern in that package worth a sweep, not three coincidences.Step 6 fixed propel's Spinner to be byte-equivalent to
@pace/ui's before moving the 33 call sites, in the same commit, so the swap is a genuine no-op rather than a silent restyle.Type of Change
Screenshots and Media (if applicable)
None, and three changes below are visual. Nothing here runs a browser, so every claim about how a spinner, tooltip, avatar or skeleton looks is reasoning from the class strings and the Base UI source.
Test Scenarios
pnpm turbo run build check:types check:lint check:format --force— 59/59, no cache (every task loggedcache bypass, force executing), exit 0. That covers real vite/tsdown builds of web, space, admin and every package, which is what answers the subpath-export questionstsccannot (@pace/propel/{skeleton,spinners,input,card,avatar}).packages/codemodsvitest: 58 tests pass, including 11 new cases. The sweep reported80 ok / 1459 unmodified / 0 errors / 0 skipped.oxlinton changed files: 0 errors; only pre-existing a11y/promise warnings on lines not written here, and every per-package--max-warningsbaseline passes unchanged.oxfmt --checkclean (two rounds of fixes were needed —packages/codemodsformats at a narrower print width than the apps).git diff -M --find-renames=90%records the AvatarGroup move as a rename with 6 changed lines, all import paths.The new codemod follows the offset-splice pattern rather than
root.toSource(): it uses the AST only to locate character offsets and splices into the original text. That is not a style preference — a recast-based transform reprints whole JSX subtrees in files carrying// oxlint-disable-next-lineinside their JSX, which damaged 7 files on an earlier track.References
Behaviour changes:
profile/overview/activity.tsxgains a tooltip. It is the one pre-existing propel Avatar call site and passes noshowTooltip, so each activity-feed avatar now shows the actor's name on hover. The default had to betrueto preserveapps/space's group-by column headers, which render<Avatar name={...} />and rely on the name appearing on hover.apps/spaceAvatar sites change fallback colours, from a hardcoded#028375/#fffffftovar(--background-color-accent-primary)/var(--text-color-on-color). A hardcoded pair cannot be right in both themes, so this is a fix — but it is visible in light mode too and was not looked at.alt. Base UI'sAvatar.Imagesets none and@pace/ui's raw<img>passedalt={name}; a literal port would have read image URLs to a screen reader.Two steps deliberately not done, and the reasons are the most useful thing in this PR:
The 110-file Tooltip migration is a design decision wearing a migration's clothes. The two Tooltips are not the same component with different prop names — they are different designs. External's cva is
border-sm border-subtle-1 bg-layer-2 px-2 py-1.5 shadow-overlay-200with the label as a bare text child; propel hangsmax-w-xs rounded-lg break-wordson the positioner and wraps content in<p class="text-caption-sm-regular text-secondary">. Absorbinglayout(63 sites) faithfully means giving propel's Tooltip a second visual identity switched by a prop, which needs Figma. Also measured: the open delay would change from 600ms to 200ms on all 149 sites,sideOffsetfrom 8 to 10, andshortcut(3 sites) andalignOffset(1) have no propel prop at all.Retiring
@headlessui's Popover and Tab is not a rename either.usePopper+ two refs +style={styles.popper}), which propel replaces with a portal and floating-ui props — the hook and its wiring have to be re-expressed per file. Worse, headlessui's Popover root renders a DOM element and acceptsclassName/ref/tabIndex/onKeyDown(one file passes all four) while Base UI's renders nothing, and 6 files use{({ open }) => …}render props that have no Base UI equivalent.Tab.Group as={Fragment}, all wrap panels inTab.Panels(no counterpart, and it often carries classes), selection is index-based against Base UI's value-based model, and propel'sTabsTriggercarries opinionateddata-[active]:classes that would stack on styling those sites drive from local state.Both are popup and tab-strip behaviour that is only judgeable in a browser.
@headlessui/reactstays; no app reaches for it where propel already has the component.Corrections to the plan: step 5 is 80 files not 79, step 6 is 33 not 31.
cnwas pointed at@pace/utils, not@pace/propel/utilsas the plan said —@pace/ui'scnis a one-line re-export of@pace/utils', 358 app files already import it from there, andcnis not a component so the standardise-on-propel decision does not reach it.packages/ui/src/form-fields/input.tsxcould not be deleted as the plan implied (input-color-picker.tsxcomposes it); the barrel re-export was removed instead so no app can reach it. Every--filter=@pace/webin the plan's verification lines fails — the workspace names areweb,space,admin.