Repository navigation
Conversation
1 task done
|
React Doctor found 17 new issues in 10 files · 17 warnings · score 61 / 100 (Needs work) · 2 fixed · vs 17 warnings
Reviewed by React Doctor for commit |
Callers currently have to know which of two packages an icon lives in: 170 `*Icon` components come from `@pace/propel/icons` and 737 `*Outline`/`*Filled` ones from `@makeplane/propel/icons`, and 52 files in apps/web alone import from both. Re-exporting the external set here collapses that into one import path without drawing a single new SVG — @makeplane/propel stays the icon vendor, it just stops being something call sites have to name. The two name sets are disjoint, which is the property that makes a single barrel safe: comparing the exported symbols of both modules with the TypeScript checker gives 170 and 737 names with an empty intersection, so no name is ambiguous and the local exports below shadow nothing. The merged module resolves to 900 runtime values (the 170 local exports include 7 type-only ones).
The sweep this drives touches 516 files, which no one can review hunk by hunk — so the reviewable artefact has to be the transform and its tests, and the transform has to be provably incapable of changing anything but the module specifier. It uses the AST only to locate the specifier and then splices the replacement into the original text, rather than returning `root.toSource()`. That is not caution for its own sake: the first pass over these 531 files did go through recast, and recast rewrote unrelated JSX in 7 of them — wrapping elements in parentheses, folding a self-closing `<span />` onto the text node beside it, and stripping blank lines between siblings. recast reprints a whole subtree whenever it cannot reconcile comment attachment, and several of these files carry `// oxlint-disable-next-line` comments inside their JSX. Splicing keeps the property the sweep needs: every hunk is the single `from` line. The spec pins that by asserting full output equality both on a multi-line specifier list and on the exact JSX shape that recast mangled. Locating with the AST rather than a regex is what rules out matching the path inside a string or a comment, and it covers `export ... from` re-export sources. The other cases the spec pins are the ones easy to get wrong: type-only imports, and the sibling `@makeplane/propel/components/*` and `/hooks` subpaths, which must survive untouched because 55 form screens are expected to keep exactly one component import from the external package. It deliberately does not merge into an existing `@pace/propel/icons` import. Merging would reflow the specifier lists of 52 files for no behavioural gain, and two imports from one source is something the codebase already does — `issue-layouts/utils.tsx` splits its type and value icon imports that way, and both oxlint and oxfmt accept it. The script's target list names apps/web, apps/space, packages/editor and packages/ui explicitly instead of globbing `apps/*` and `packages/*`, because three scopes must keep the vendor path: packages/propel owns the re-export and must not import its own public subpath, and apps/admin and packages/utils declare no `@pace/propel` dependency at all. jscodeshift's `--ignore-pattern` does not filter these out when the targets are reached through `../..`, so an explicit list is both the working mechanism and the honest documentation of the exclusions.
Generated by `pnpm --filter @pace/codemods run icons-to-pace-propel`; re-running it reproduces this commit byte for byte. 516 files, one changed line each — the module specifier and nothing else. No icon is renamed, added or removed, and because `@pace/propel/icons` re-exports the external set the rewrite is an identity at runtime. What it buys is that the choice of icon package stops being a decision made 516 times. 419 of the 689 files importing `@makeplane/propel` imported nothing from it but icons, so those files now name one component library instead of two, and the population of files juggling three or four of them drops from 159 to 82. Three hunks are not one line: `workspace-invitations/page.tsx`, `settings/workspace/sidebar/item-icon.tsx` and `workspace-notifications/.../menu-option/root.tsx` had multi-line specifier lists that fit within the print width once the specifier lost six characters, so oxfmt collapses them onto one line. Three scopes deliberately keep the vendor path. packages/propel's own 20 files are where the re-export lives, and a package importing its own public subpath would make its source depend on its build output and pull the whole 900-name barrel into every component that wants one icon — @makeplane/propel is propel's declared dependency, so importing it directly is the honest edge. apps/admin (23 files) and packages/utils (1) declare no `@pace/propel` dependency; admin was deliberately moved off it and carries shims that say so, and @pace/propel depends on @pace/utils, so pointing utils back at propel would close a package cycle. Moving either needs a package.json change that is a decision of its own, not a side effect of an icon sweep. 15 further files in apps/web still import the old path; every one is deleted by the dead-code removal in #252, so rewriting them would only produce modify/delete conflicts. Re-running the codemod after that lands finishes them if it does not.
Now that apps/space imports every icon through @pace/propel/icons, an oxlint rule stops the old path coming back. It is an error rather than a warning deliberately: apps/space's script allows 676 warnings, so a warning there is invisible. Verified by introducing a violation and watching `oxlint --max-warnings=676` exit 1, then removing it and watching it exit 0. The scope is apps/space only, because that is the only app the sweep left clean. apps/web still has 15 users of the old path and every one of them is a file the dead-code branch deletes, so it joins this rule when that lands rather than being carved out file by file now. apps/admin cannot join at all while it declares no @pace/propel dependency.
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.
houko
added a commit
that referenced
this pull request
Sep 19, 2026
…r a skeptic pass rescued 62 of the candidates (#252) Rebased onto main after the other seven branches in this batch landed. The deletion list was re-verified against the current tree rather than carried over, because #257 and #258 moved a great deal of it. Eighteen files that main had edited (icon-path and router-shim rewrites) were deleted by this branch. Each was re-checked for consumers in the post-merge tree before the deletion was accepted; none had any. The same check was run over all 256 assets, again finding no references — #258 moved them into `core/assets` and updated their importers, so an asset that had a consumer would have shown one. `root.store.ts` needed both sides: main's kebab-case rename of `cycle_filter.store`, and this branch's removal of `DashboardStore`. `apps/web/package.json` had each side dropping a different orphaned dependency — main dropped `react-is`, this branch dropped `react-markdown` — so the merge drops both. Neither is imported anywhere. `pnpm-lock.yaml` was regenerated from the resolved manifests rather than merged by hand. Verified: typecheck, lint, format and build across all 56 workspace tasks, plus `go build` and the full Go test suite.
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
Routes every icon import in
apps/webandapps/spacethrough@pace/propel/icons. 516 files, one line each.@makeplane/propelstays as the icon vendor —@pace/propel/iconsre-exports it. That was the decision this step needed, and it is the cheap one: bringing 737 icons in-house means 155 new SVGs plus a semantic rename map, and buys nothing a user can see. The two name sets are disjoint (every local export is suffixed*Icon), so nothing is shadowed and no name is ambiguous.This matters because the "four competing UI libraries" problem is 90% an icon-import-path problem: 559 files reached for
@makeplane/propel/iconswhile@pace/propel/iconsalready existed with ~70 local icon modules of its own. A call site now never has to know which set an icon came from.Type of Change
Screenshots and Media (if applicable)
None. The rewrite is an identity — no specifier renamed, added or removed.
Test Scenarios
pnpm turbo run build check:types check:lint check:format --force— 59/59, no cache, exit 0. This is the strong evidence: if even one icon name failed to resolve across 516 files,check:typescatches it.apps/web's client bundle grows 4 KB on 35 MB (0.01%).packages/codemodswith a spec, so it is repeatable for the remaining migration steps.The plan's codemod design was wrong, and it was measured rather than argued. The plan specified a jscodeshift transform that mutates the AST and returns
root.toSource(), following the existingheadlessui-v2-default-tags.ts. Built exactly that and ran it: 531 files changed, 567 insertions against 574 deletions — recast reprinted unrelated JSX in 7 files, wrapping elements in parentheses, folding a self-closing<span />onto the text node beside it, leaving a stray</div>)indent and stripping blank lines between siblings. recast reprints a whole subtree when it cannot reconcile comment attachment, and those files carry// oxlint-disable-next-linecomments inside their JSX.Reverted, and rewritten to use the AST only to locate the specifier's character offsets and splice the replacement into the original text. Result: 516 files, 516 changed lines, zero collateral. The spec now pins that property against the exact JSX shape recast mangled.
References
61 files deliberately keep
@makeplane/propel/icons, each for a stated reason:apps/webare files chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252 deletes. Rewriting them would only create conflicts on files about to disappear. Verified individually against chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252's deletion list — all 15 match, reproducing the plan's figure exactly.apps/admincannot be rewritten:apps/admindeclares no@pace/propeldependency and pnpm links no such package, so this would not compile without a manifest and lockfile change. Admin also carries purpose-written shims (core/components/common/skeleton.tsx:11,core/providers/toast.tsx:14) that say in as many words that they exist to avoid@pace/propel. Migrating admin is a policy reversal needing an owner decision, not a codemod.packages/propelare the package's own runtime components. A package importing its own subpath risks a cycle.packages/utilsis blocked by dependency direction, not policy:@pace/propeldepends on@pace/utils, so utils cannot depend back. The fix, if the one-path goal matters for it, is to movegetIconForLinkto a layer above propel.packages/codemodsare the codemod and its test fixture.Also fixed, because the config was describing a package that does not exist:
tsdown.config.tslisted an entry forsrc/emoji-reaction-picker/index.ts— a directory that is not there — andsrc/separatorwas in neither the entry list nor the exports map, so nothing could import it. (The brief claimed./iconsalso needed adding; it was already in both. Andsrc/separator/index.tshad to be created, not merely listed.)52 files now hold two import statements from
@pace/propel/iconsinstead of one from each package. Deliberate: merging them would reflow 52 specifier lists and destroy the one-line-per-file property that makes a 516-file diff reviewable. The premise that this is disallowed is false —issue-layouts/utils.tsx:32-33already splits its type and value icon imports exactly that way, and both oxlint and oxfmt accept it.After #252 merges, re-run
pnpm --filter @pace/codemods run icons-to-pace-propeland confirm it reports 0 files changed. That is the proof the 15 skipped files are gone rather than merely unswept.Merge order: this branch and #257 (layering) both rewrite import lines across hundreds of the same files and will conflict heavily. One should merge, then the other rebase. #252 should go before both.