Repository navigation
refactor: stop core/ importing the route tree, in all three apps, and lint it shut - #258
Merged
Merged
Conversation
|
React Doctor found 3 new issues in 3 files · 3 warnings · score 66 / 100 (Needs work) · 2 fixed · vs 3 warnings
Reviewed by React Doctor for commit |
1 task done
These two files were part of the next/* compatibility layer added when the app moved from Next.js to React Router, and unlike the navigation and link shims next to them nothing ever imported them: `rg 'hooks/(image|script)'` over the whole tree returns nothing, there is no dynamic `import(` of any `@/app` path, and no tsconfig path maps `next/image` or `next/script` onto them. Removing them now means the router-shim move that follows relocates two files instead of four.
This was the only import in core/ that reached into app/ by URL segment: core/components/navigation/top-navigation-root.tsx imported `@/app/(all)/[workspaceSlug]/(projects)/star-us-link`, so renaming the `(projects)` route group would have broken a top-level navigation component for reasons a reader of either file could not see. The component is not a route — nothing in app/routes/ registers it — it is a link in the top nav, which is exactly what core/components/navigation/ holds, and the consumer now reaches it as a sibling `./star-us-link`.
core/ is the reusable layer and app/ is the React Router route tree, but 328 of core/'s import lines pointed upward at `@/app/hooks/navigation` and `@/app/hooks/link` — the two next/navigation and next/link compatibility shims left over from the Next.js port (370 lines once app/'s own imports and helpers/authentication.helper.tsx are counted, because the shims themselves move). Nothing about either file is route-specific: between them they import only react, react-router and `ensureTrailingSlash`, and they were parked under the route tree purely by accident of where the port started. They now live next to the rest of the framework glue in core/lib (store-context, wrappers, polyfills, idle-task), which means the route tree can be reorganised without touching a single file in core/. No tsconfig or vite change is needed: `@/*` already maps to `./core/*`, so `@/lib/navigation` resolves the moment the files land there. Every one of the rewritten references was a static `from "…"` import declaration — there is no dynamic `import()` of any `@/app` path and no string registry — so this is a pure prefix substitution. `git diff -U0 -- '*.ts' '*.tsx' | rg '^[+-]' | rg -v '^(\+\+\+|---)' | rg -v '@/(app/hooks|lib/navigation)'` prints nothing, which is the mechanical proof that no line outside the substitution moved. The shim keeps its `setTimeout(…, 0)` deferral on push/replace/back/forward. apps/space's and apps/admin's copies navigate directly instead, so the two are not interchangeable and each app keeps its own.
478 static files — 434 in web, 27 in space, 17 in admin — sat inside the React Router route directory while being imported 214 times, 166 of those from core/ or helpers/. Nothing about a cover image, an empty-state illustration or a favicon is route-specific, and keeping them under app/ meant the reusable layer could not render a picture without importing upward from the layer above it. `@/*` already maps to `./core/*` in all three tsconfigs, so `@/assets/…` resolves with no config change; the substitution is prefix-only, which is why the `?url` query suffixes come through untouched. packages/brand/generate.sh writes the favicons, the apple-touch icon, the two PWA icons and the two auth gradients directly into these directories, so it is repointed in the same commit — otherwise the next brand regeneration would silently recreate the old app/assets tree and leave the committed rasters unreachable. Verified with a real build, not just tsc: `*.png?url` resolves through vite/client's ambient module declaration, so tsc cannot tell a moved asset from a missing one. `turbo run build check:types check:lint check:format` is green across all 59 tasks, and web's build output lists the moved rasters (favicon-16x16, icon-512x512, og-image, maintenance-mode-*) as emitted assets.
…r out of the route tree
The same layering inversion apps/web had: space's and admin's next/navigation and next/link shims lived under app/hooks/, so 42 import lines — 25 of them in the reusable layers — pointed upward into the route tree, and admin's AuthHeader, rendered by core/components/instance/failure.tsx and setup-form.tsx as well as by the sign-in form, was addressed as `@/app/(all)/(home)/auth-header`, which encoded two route group names in an import path. AuthHeader now sits in core/components/common next to pace-lockup and page-wrapper, the pieces it is made of. After this commit no file in any of the three apps imports `@/app/…`.
Each app keeps its own copy of the shim rather than sharing one. web's defers push/replace/back/forward through `setTimeout(…, 0)` ("Defer navigation to avoid state updates during render") while space's and admin's navigate directly; those two are byte-identical to each other, but unifying all three would change navigation timing in one app, and the repo has no JavaScript test that would catch the regression. That is a behaviour decision, not part of a move.
The 480-odd upward imports the previous commits removed grew one file at a time, and nothing in the repo would have objected to the next one. This rule makes the layering a build error rather than a convention: any `@/app/**` import from a file under core/ fails check:lint. Three details are load-bearing and were each confirmed by making the rule fire and then removing the violation. The group must be `@/app/**`, because oxlint's single `*` matches one path segment and `@/app/*` matches nothing we care about. The override glob must be `**/core/**` and not `apps/*/core/**`, because oxlint matches paths relative to the working directory and each app's check:lint runs with cwd set to the app — which also means the rule necessarily covers space and admin, so they had to be cleaned up first. And the severity must be `error`: apps/web's script is `oxlint --max-warnings=11957`, which would swallow a warning without a trace.
`EmptyState` lived in core/components/common while the three empty-state roots it sits beside conceptually — SimpleEmptyState, DetailedEmptyState, SectionEmptyState — live in core/components/empty-state, so a reader looking for "the empty state component" found three of four. This is colocation, not deduplication: the moved component takes a raw `image` plus button props and is not a variant of the other three, so it keeps its own file and its own name.
…tion 1596 of apps/web/core's 1620 files are kebab-case; the exceptions were 10 snake_case MobX stores, 3 snake_case components, 6 `-HOC.tsx` files, two `useXColumns.tsx` hooks, `workItem-detail.tsx` and one camelCase directory (gantt-chart/helpers/blockResizables), plus 3 files in apps/space/core, 3 in packages/editor and 15 in packages/types. Nothing chose those names on purpose — they are the residue of several import waves — and the cost of leaving them is that every new file is a coin flip. With the renames done the convention becomes machine-checkable, which is what the next commit does. The renames are path-scoped, not string-scoped, and that distinction is the whole risk here: across apps and packages `rg -F start_date` hits 744 lines, `target_date` 572 and `state_group` 251, essentially all of them API payload keys and group-by values — `state_group` is a documented view property in packages/types. The actual module references are 1, 1 and 4. Every one of the 93 rewritten lines matched a quoted specifier whose final path segment was the old basename (a `/` immediately before it), so no payload key or translation key could be caught by it. apps/web/core/components/workspace/ConfirmWorkspaceMemberRemove.tsx is deleted rather than renamed: confirm-workspace-member-remove.tsx already exists, is the version both call sites import, and exports the `Props` type that the PascalCase copy imports back from it. There is no name left for it to take.
`unicorn/filename-case` was explicitly `"off"`, which is why the 44 files the previous commit renamed accumulated in the first place. It is on at `error` now, so the convention is checked rather than remembered; apps/web's `--max-warnings=11957` would have swallowed a warning. apps/api/tools is exempt. Its ten `generate_*.mjs` files are Go-side generators named to match the testdata they emit, and apps/api/README.md documents `tools/generate_ydoc_schema.mjs` by that name, so renaming them would break a documented interface to buy nothing.
…, 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
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.
houko
added a commit
that referenced
this pull request
Sep 19, 2026
`TestAChangeOnOneServerReachesTheOther` fails on CI roughly one run in eight, always as `0 sync frame(s) arrived in ten seconds`. It hit #254 and #258 during the P3 batch; neither touches `internal/live`, and #258 does not touch Go at all. `Relay.Subscribe` records a subscription in `r.subscriptions` and only afterwards waits for Redis to confirm it, because `client.Subscribe` merely queues the command — `Receive` is what sends it. `waitForSubscription` polled that map, so it returned during the window between those two statements. Pub/sub keeps no backlog, so the change published in that window is not delayed, it is dropped, and the test waits out its full ten seconds for a frame that no longer exists. The relay itself is unchanged: its own ordering is correct, it confirms before it publishes. The test now asks Redis with `PUBSUB NUMSUB`, which only counts a subscriber once the `SUBSCRIBE` has landed. Both servers share one Redis, so one question covers the pair. `0 frames` is what points here rather than at a slow machine: every server publishes SyncStep1 and QueryAwareness from `Subscribe` itself, so a server that were subscribed would have received those too. Receiving nothing means it was not subscribed when the other side published. Verified: the `internal/live` suite passes, including under CPU contention and `-race`. Not verified: the original failure did not reproduce locally in 60 runs under load, on either the old or the new code. This closes a window that is certain from reading the code, not one demonstrated by a local repro. If the test flakes again after this, the root cause is elsewhere and this reasoning should be discarded rather than patched over.
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
core/no longer imports upward into the route tree, in all three apps, and a lint rule keeps it that way.The audit reported 441 upward imports as if they were 441 problems. They are four targets, and 95% of them are one 55-line Next.js router shim plus a static-asset directory that simply live in the wrong folder. So this is a handful of moves plus a large mechanical import rewrite — 629 import lines across ~620 files — not a redesign.
rg 'from "@/app/' apps/web/corenow returns 0.What moved:
app/hooks/{navigation,link}→core/lib/navigation, in all three apps.app/assets/→core/assets/, in all three apps.app/hooks/{image,script}.tsxdeleted — zero importers anywhere in the repo, so they were never moves.StarUsOnGitHubLinkand admin's auth header out of the route tree.common/empty-state.tsxcolocated with the other empty states.unicorn/filename-caseturned on.Two guard rails, both verified to fire rather than assumed:
no-restricted-importson@/app/**from anything undercore/, andunicorn/filename-case. Both are errors, not warnings, becauseapps/web's script allows 11,957 warnings and a warning there is invisible.Type of Change
Screenshots and Media (if applicable)
None, and one check is genuinely owed here — see below.
Test Scenarios
pnpm turbo run build check:types check:lint check:format --force— 59/59, no cache, exit 0.buildis the load-bearing check, notcheck:types.tsccannot verify asset paths:*.webp?urlresolves throughvite/client's ambient declaration, so a broken asset import type-checks fine and only a real build catches it. The build was run.core/,oxlintreportedeslint(no-restricted-imports)as an error, and the probe was removed.rg 'from "@/app/' apps/web/core→ 0 lines.Still owed, and not done: a browser smoke check on the asset move — load the app once and confirm the favicon, the logo spinner and one empty-state illustration render. The build emits them and
index.htmllinks the hashed favicons, but no renderer has run.References
The plan missed a real dependency and it would have silently broken things.
packages/brand/generate.shwrites the favicons, apple-touch icon, PWA icons and the two auth gradients directly intoapps/$app/app/assets/for web, space and admin in one loop. Moving onlyapps/webas the plan said would have left the brand generator recreatingapps/web/app/assetsand orphaning the committed rasters. All three apps moved in one commit, withgenerate.shrepointed in it. Anyone regenerating brand rasters must be on this branch or later.Other corrections, all measured:
ConfirmWorkspaceMemberRemove.tsx/confirm-workspace-member-remove.tsxare a case-duplicate pair is wrong on the premise — this is APFS, case-insensitive. The pair coexists because the names differ by hyphens, not only case. It is just a dead PascalCase copy that imports itsPropstype back from the live kebab file, and it is deleted here. The six-HOC→-hocrenames are case-only;git mvrecorded them R100, but a reviewer applying the patch on a case-sensitive box should confirm.rg -F start_dateis 744 lines, not 395;target_date572, not 257;state_group251, not 37. The plan's conclusion — rename by path, never by basename — is right and matters more than the numbers.push/replace/back/forwardthroughsetTimeout(..., 0)and space's and admin's callnavigate()directly (admin's turned out byte-identical to space's). Extraction would be a behaviour decision, so each app keeps its own copy.commentkey inside anoverrides[]entry, so the rationale for exemptingapps/api/toolslives in the commit message rather than beside the override.packages/types/src/issues/issue_subscription.tsis exported by no barrel and referenced nowhere. Renamed rather than deleted — deleting dead modules is chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252's job — but worth folding into a dead-code pass.Not done, as scoped: steps 6, 7, 9 and 10 of the plan — merge
core/components/core/themeintoappearance/, renamecore/components/uitoskeletons, extract the three app-shell primitives intocore/components/shell, dispersecore/components/core/modals. Three of those want #252 merged first.Also left:
apps/web/helpers/is a fourth root alongsideapp/andcore/, mapped as@/helpers/*, and it imports assets and the link shim. It is the only structural oddity remaining in the path map.Merge order: this branch and #257 (icons) both rewrite import lines across hundreds of the same files and will conflict heavily — one merges, the other rebases. #252 deletes 57
apps/webmodules and 256app/assetsfiles, so it should go before both; this branch was built pre-#252 and its counts are correspondingly larger than the plan's post-#252 estimates.