Repository navigation
refactor(web): finish the component reorganisation — theme, skeletons, shell, modals - #260
Merged
Merged
Conversation
…appearance Theme lived in two directories: core/components/appearance held the public entry point (theme-switcher.tsx, the only file the barrel re-exports and the only one settings reaches) while the six pieces it composes sat under the unnamed core/components/core bucket. A reader arriving at the switcher had to leave the feature directory to find the selector it renders. appearance/ is the home rather than a new top-level theme/ because it is feature-named and already owns the entry point. The four files reached only through relative ./ imports from custom-theme-selector.tsx move with it untouched, so the entire cross-directory cost is the two specifiers in theme-switcher.tsx, which become sibling-relative in line with how the rest of this directory already refers to its own files.
core/components/ui holds 24 files, 22 of which are skeleton loaders under loader/. The two exceptions are a label-count chip and an empty-state layout, and both have an obvious owning directory already: IssueLabelsList belongs with the other label components and EmptySpace/EmptySpaceItem with the other empty states. Moving them first means the directory that gets renamed next contains nothing but loaders, so the new name needs no qualification. Neither file is added to core/components/labels/index.ts: that barrel is hand-maintained and already omits several of its neighbours, so the single consumer keeps importing by path.
The path lied twice over. apps/web imports the @pace/ui package on nearly every screen, so "@/components/ui" reads as a local copy of the design system when in fact every file under it is a loading skeleton. Naming the directory for what it contains removes the collision and tells a reader where skeletons live without opening the folder. Four files stay behind in core/components/ui because #252 (chore/remove-dead-code-and-assets) deletes them: markdown-to-component.tsx, profile-empty-state.tsx, loader/notification-loader.tsx and loader/pages-loader.tsx. All four are unreferenced anywhere in the repo, and none of them imports a sibling, so leaving them in place breaks nothing and keeps this rename from colliding with that deletion. core/components/ui disappears when #252 lands.
…s/shell PageTitle, ContentWrapper and AppHeader are the three highest-traffic residents of core/components/core, and unlike the rest of that bucket they share an obvious identity: they are the chrome every route renders around its content, consumed almost entirely by app/ layouts and pages. Ninety-eight of the directory's import lines point at these three files, so naming them is where the bucket's cost actually is. shell rather than app-shell keeps the import short, and rather than core/layouts because that directory already exists with a different meaning: it holds three route-level layout components, not primitives those layouts compose. The remaining 33 files in core/components/core are deliberately untouched. They have no single destination between them and each one wants a judgement call about its owning feature; emptying the directory is not what this change is for.
…eatures Every one of the eight files in this directory has an unambiguous owner, so the grouping by "is a modal" bought nothing: it only guaranteed that anyone working on work items, the workspace logo or the profile avatar had to look somewhere other than the feature directory. Fifteen import lines total, and the two relative imports inside the group (bulk-delete-issues-modal to its item, existing-issues-list-modal to its empty state) survive untouched because both pairs land in issues/. The two destinations that needed a judgement call: gpt-assistant-popover.tsx goes to editor/ rather than issues/ because it is a generic AI-completion popover over a RichTextEditor with no work-item concept in it, and its sole consumer imports it as an editor affordance. user-image-upload-modal.tsx goes to profile/ rather than account/ because it edits the profile avatar; account/ in this codebase holds authentication and account-lifecycle concerns, which is also why change-email-modal.tsx goes there. core/modals/ (the unrelated top-level directory) is not involved, and the remaining 25 files of core/components/core are left alone.
|
React Doctor found 1 new issue in 1 file · 1 warning · score 79 / 100 (Needs work) · 1 fixed · vs 1 warning
Reviewed by React Doctor for commit |
houko
added a commit
that referenced
this pull request
Sep 19, 2026
…, shell, modals (#260) * refactor(web): merge core/components/core/theme into core/components/appearance Theme lived in two directories: core/components/appearance held the public entry point (theme-switcher.tsx, the only file the barrel re-exports and the only one settings reaches) while the six pieces it composes sat under the unnamed core/components/core bucket. A reader arriving at the switcher had to leave the feature directory to find the selector it renders. appearance/ is the home rather than a new top-level theme/ because it is feature-named and already owns the entry point. The four files reached only through relative ./ imports from custom-theme-selector.tsx move with it untouched, so the entire cross-directory cost is the two specifiers in theme-switcher.tsx, which become sibling-relative in line with how the rest of this directory already refers to its own files. * refactor(web): move the two non-loaders out of core/components/ui core/components/ui holds 24 files, 22 of which are skeleton loaders under loader/. The two exceptions are a label-count chip and an empty-state layout, and both have an obvious owning directory already: IssueLabelsList belongs with the other label components and EmptySpace/EmptySpaceItem with the other empty states. Moving them first means the directory that gets renamed next contains nothing but loaders, so the new name needs no qualification. Neither file is added to core/components/labels/index.ts: that barrel is hand-maintained and already omits several of its neighbours, so the single consumer keeps importing by path. * refactor(web): rename core/components/ui to core/components/skeletons The path lied twice over. apps/web imports the @pace/ui package on nearly every screen, so "@/components/ui" reads as a local copy of the design system when in fact every file under it is a loading skeleton. Naming the directory for what it contains removes the collision and tells a reader where skeletons live without opening the folder. Four files stay behind in core/components/ui because #252 (chore/remove-dead-code-and-assets) deletes them: markdown-to-component.tsx, profile-empty-state.tsx, loader/notification-loader.tsx and loader/pages-loader.tsx. All four are unreferenced anywhere in the repo, and none of them imports a sibling, so leaving them in place breaks nothing and keeps this rename from colliding with that deletion. core/components/ui disappears when #252 lands. * refactor(web): extract the route-shell primitives into core/components/shell PageTitle, ContentWrapper and AppHeader are the three highest-traffic residents of core/components/core, and unlike the rest of that bucket they share an obvious identity: they are the chrome every route renders around its content, consumed almost entirely by app/ layouts and pages. Ninety-eight of the directory's import lines point at these three files, so naming them is where the bucket's cost actually is. shell rather than app-shell keeps the import short, and rather than core/layouts because that directory already exists with a different meaning: it holds three route-level layout components, not primitives those layouts compose. The remaining 33 files in core/components/core are deliberately untouched. They have no single destination between them and each one wants a judgement call about its owning feature; emptying the directory is not what this change is for. * refactor(web): disperse core/components/core/modals into the owning features Every one of the eight files in this directory has an unambiguous owner, so the grouping by "is a modal" bought nothing: it only guaranteed that anyone working on work items, the workspace logo or the profile avatar had to look somewhere other than the feature directory. Fifteen import lines total, and the two relative imports inside the group (bulk-delete-issues-modal to its item, existing-issues-list-modal to its empty state) survive untouched because both pairs land in issues/. The two destinations that needed a judgement call: gpt-assistant-popover.tsx goes to editor/ rather than issues/ because it is a generic AI-completion popover over a RichTextEditor with no work-item concept in it, and its sole consumer imports it as an editor affordance. user-image-upload-modal.tsx goes to profile/ rather than account/ because it edits the profile avatar; account/ in this codebase holds authentication and account-lifecycle concerns, which is also why change-email-modal.tsx goes there. core/modals/ (the unrelated top-level directory) is not involved, and the remaining 25 files of core/components/core are left alone. * chore: bump the workspace version, which check-version requires of every PR
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/layering(#258) — review that first.Four relocations inside
apps/web/core/components, finishing the reorganisation #258 started. 156 changed lines across 120 files, and git records 39 renames, so this reads as a move rather than a rewrite.core/components/core/theme/(6 files) →core/components/appearance/, which is feature-named and already holds the public entry point.core/components/ui/→core/components/skeletons/, and the two files in it that are not skeletons evicted tolabels/andempty-state/.page-title,content-wrapper,app-header) →core/components/shell/. 98 import lines, most of them fromapp/.core/components/core/modals/dispersed into the eight owning features.core/components/coregoes from 42 files to 25. The other 25 are deliberately untouched — emptying that directory is not this PR's job.Type of Change
Screenshots and Media (if applicable)
None. Every file moved byte-identical; only import specifiers changed.
Test Scenarios
pnpm turbo run build check:types check:lint check:format --force— 59/59, no cache, exit 0.git diff -M --find-renames=90% --stat→ 159 files, 156 insertions / 156 deletions, 39 renames. A move-only diff: no line is both added and removed within a file.unicorn/filename-caseon as an error, so every moved path had to satisfy it;check:lintis the proof.References
Two deliberate interactions with #252 (
chore/remove-dead-code-and-assets), both of which a reviewer needs to know about:ui/→skeletons/rename moves 20 of 24 files, not all 24. The other 4 are files chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252 deletes. Moving them would create 4 rename-vs-delete conflicts whose wrong resolution resurrects dead code — the exact risk worth avoiding. Verified before choosing this: all 4 have zero references anywhere inappsorpackages, and none imports a sibling insideui/, so nothing dangles. The consequence is thatcore/components/uidoes not disappear until chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252 lands.issues/issue-detail/relation-select.tsx,onboarding/profile-setup.tsx). They were rewritten rather than skipped, because skipping would leave this branch failingtsc. That produces 2 modify/delete conflicts when chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252 merges, both resolved by taking the deletion. Unlike case 1, there was no option that avoided a conflict.Corrections to the plan, all measured:
core/components/coreholds 42 files, step 6 already removes 6 of them, so 33 remain after step 9 and 25 after step 10 — not 39. This was caught after it had been written into two commit messages, and those two commits were rebuilt with the real numbers (git diff --statbetween old and new heads is empty, so only the messages changed).comm -12and the overlap is empty, which is why the counts coincide there.core/components/uiholds files whose skeleton-ness is arguable. It does not. The audit's guess that the 4 files chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252 removes "made the name arguable" is half right:markdown-to-component.tsxandprofile-empty-state.tsxgenuinely were not skeletons, butnotification-loader.tsxandpages-loader.tsxare ordinary skeletons that happen to be dead.Not done: the remaining 25 files in
core/components/corestill need homes, andapps/web/helpers/is still a fourth root alongsideapp/andcore/— the only structural oddity left in the path map.