refactor(propel): move off the deprecated Base UI beta, so the apps stop shipping two copies - #256
Merged
Merged
Conversation
|
React Doctor found 14 new issues in 13 files · 14 warnings · score 76 / 100 (Needs work) · 15 fixed · vs 14 warnings
Reviewed by React Doctor for commit |
2 tasks done
…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`.
…ect 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.
…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
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.
![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
… lint it shut (#258) Rebased onto main after #256 and #257 landed. Two things beyond the mechanical conflict resolution are worth recording. The 33 import conflicts were all the same shape — main had already moved the icon path, this branch moves the router shims — and were resolved by taking main's side and reapplying this branch's two renames (`@/app/hooks/navigation` to `@/lib/navigation`, `@/app/hooks/link` to `@/lib/navigation/link`). `.oxlintrc.json` needed more care. Both branches add an `overrides` entry configuring `no-restricted-imports`: this one restricts `@/app/**` under `**/core/**`, and #257 restricts `@makeplane/propel/icons` under `**/space/**`. oxlint does not merge a rule's options across overrides — the last matching entry replaces the earlier one outright — and `apps/space/core` matches both, so the straightforward merge silently turned this branch's route-tree rule off for the whole of `apps/space/core`. Confirmed by linting a file there that breaks both rules and seeing only the icons one reported. A third override for `**/space/core/**` carrying both restrictions fixes it. Verified across all three regions: `apps/space/core` reports both, `apps/web/core` reports the route-tree rule only, `apps/space/app` reports the icons rule only.
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
@pace/propelwas built on@base-ui-components/react@1.0.0-beta.3, which pnpm-lock.yaml itself records asdeprecated: Package was renamed to @base-ui/react. Meanwhile@makeplane/propel@0.3.0depends on@base-ui/react@^1.6.0, so both copies were installed and both shipped to users. This moves propel onto 1.7.0 and leaves one.Nobody had named this, and it is the prerequisite for the whole
@pace/propelconsolidation: standardising on a library that sits on a deprecated beta while its sibling already runs the successor is not a foundation.A rename is not an identity mapping across beta.3 → 1.7.0. Each of the 16 importing files was checked against the installed source rather than sed'd. Four real breaks, all fixed:
Accordion:openMultiple→multipleTooltip/Menu/Popover: the hover props moved from Root to TriggerTabs:data-selected→data-active,selected→activeCombobox: a cleared single-select value now arrives asnullwhere it was a stringPopover's public props are literallyReact.ComponentProps<typeof BasePopover.Root>, so the upstream narrowing propagates straight into threeapps/webfiles. Keeping this change insidepackages/propelwas only possible by adding a compatibility shim — a plain rename would have broken the app build.Type of Change
Screenshots and Media (if applicable)
None. No app-visible component changed its prop surface. The DOM-level and keyboard-level differences are listed below; none has a live consumer.
Test Scenarios
pnpm turbo run build check:types check:lint check:format --force— 59/59, no cache, exit 0. (A cached run reportsFULL TURBOand proves nothing; this was forced.)ls node_modules/.pnpm | grep base-ui) gives a false negative: pnpm leaves the orphaned virtual-store directory behind after install. It is genuinely dead — zero symlinks point at it,pnpm why -rfinds nothing, the lockfile has no entry — proved by deleting it and rebuilding web and propel from scratch.pnpm why -ror the lockfile is the right assertion.turbo run build, so they would not have caught a break.References
Behaviour differences, none with a live consumer today:
Switchroot renders a<span>instead of a<button>(upstream beta.5, #3205). Prop surface and Tailwind classes unchanged; Base UI still appliesrole="switch", tabindex and keyboard handling.@pace/propel's Switch has zero importers.Tabs'activateOnFocusdefault flipped true → false upstream (#3176). Arrow keys now move focus without switching; Enter/Space activates. The upstream default was kept deliberately — both are valid APG patterns, upstream changed it on purpose, and the only live consumer is the emoji/icon picker's two click-driven tabs. One prop to revert if you'd rather pin the old behaviour.Comboboxclearing now yieldsnullinternally; the publiconValueChangecontract is unchanged (the root maps it to""in single mode,[]in multiple).TooltipandMenuhover props now sit on the trigger. Same numbers, same visible timing; only matters if several triggers ever share one root.packages/propelis nowsideEffects: false, so app bundlers can drop unused propel modules. Verified safe: nothing underpackages/propel/srcimports CSS or mutates module-scope globals, and the published stylesheet reaches the apps through a CSS@importin theirglobals.css, which JS side-effect pruning does not touch.@pace/propel/separatorbecomes importable for the first time — it was in neither the tsdown entry list nor the exports map and had noindex.tsat all, so the plan's "just add the entry" was slightly wrong; the barrel had to be created.Conflicts with #250, which must be resolved by hand: #250 changes
packages/propel/src/tooltip/root.tsx(removes the per-instanceProvider, deletes the deadrenderByDefaultprop) and this branch moves the delay props from Root to Trigger. Merging either version verbatim breaksweb:check:types— the resolution is #250's structure plus 1.7.0's prop placement.Bugs found in propel while checking APIs, not fixed here:
context-menu.tsxhas eight deaddata-[state=...]animation classes (andswitch.stories.tsx:211); Base UI emitsdata-[open]/data-[closed].tabs.tsx'sTabsIndicatornever rendersTabsPrimitive.Indicator, so--active-tab-width/--active-tab-leftdo not exist.collapsible.tsx:84setsdata-panel-open={isOpen}manually, sogroup-data-[panel-open]:matches while closed.