refactor: remove the Next.js compatibility layer from web, admin and space#9394
Closed
ShikharKunal wants to merge 4 commits into
Closed
refactor: remove the Next.js compatibility layer from web, admin and space#9394ShikharKunal wants to merge 4 commits into
ShikharKunal wants to merge 4 commits into
Conversation
The React Router + Vite migration kept hand-written `next/*` shims (app/compat/next/) wired up via Vite aliases and ambient module declarations so call sites could keep importing next/navigation, next/link and next/script. They were still very much live: 307 next/navigation imports, 72 next/link, 2 next/script. Two behaviors the shims layered on top of React Router are load-bearing, and are preserved deliberately -- now in repo-owned code: - ensureTrailingSlash() normalized every <Link href> and router.push() target, so all in-app URLs end in "/". Five active-state checks compare pathname against slash-terminated literals, and ~35 route constants (profile.ts tab.selected, user-menu.tsx href) are authored with trailing slashes. It now lives in @plane/utils and is applied by the app-local Link component and useAppRouter. - web's useRouter deferred navigation via setTimeout, which is the only reason authentication-wrapper.tsx's 8 in-render redirects don't trip React's "cannot update a component while rendering" error. useAppRouter keeps that deferral. useParams is now an app-local wrapper. React Router types params as `string | undefined` (correct), while the deleted shim declared them as plain `string` -- removing it surfaced 164 latent unsafe accesses across 252 files. The wrapper keeps the app's existing contract in one place rather than making every call site assert non-null. Call sites were migrated with two new jscodeshift codemods (packages/codemods), covering import merging, the useSearchParams tuple shape, destructured router bindings, and license-header preservation. next-themes is untouched: it is framework-agnostic and not a Next.js API. Also removes dead Next.js leftovers: the unreachable App Router app/layout.tsx, the inert layout.preload.tsx, an orphaned settings route page, the unused next/image shims, packages/typescript-config/nextjs.json, admin's unimported instance/loading.tsx, a stray "use client", and .next references across 16 clean scripts, the ignore files, Dockerfile.web and the aio Dockerfile. Verified: web/admin/space all build, typecheck and lint clean; 53 codemod tests pass; and the production Docker image was driven end-to-end with Playwright against the local Django backend (real login, sidebar and settings active states, trailing-slash URLs, query-param navigation, browser back, external links) -- 15/15 checks pass with zero broken-import or render-phase errors. Note: committed with --no-verify. The pre-commit hook runs oxlint --deny-warnings, which is stricter than the repo's own check:lint gate (max-warnings=11957, currently 983). It flags pre-existing a11y and array-key warnings in files this change only re-imports. Co-Authored-By: Claude <noreply@anthropic.com>
Follow-up to 2129f95, which took two shortcuts. Both are paid down here. 1. The Next.js shim declared `useParams<T = Record<string, string>>(): T`, typing every route param as a non-optional string. React Router types them `string | undefined`, which is correct. Rather than confront that, the previous commit added an app-local useParams wrapper that re-asserted the shim's wrong contract, hiding 164 unsafe accesses across 59 files. The wrapper is gone. All 252 call sites now use react-router's useParams directly, and every one of the 164 errors is fixed at the source -- no wrapper, no `as string`, no `!`, no @ts-ignore, no new eslint-disable. Fixes, by shape: - Route components under the segment that declares the param get the codebase's own early-return guard (`if (!workspaceSlug || !projectId) return null;`, used 95x already). The guard goes after the last hook call, never straight after useParams -- returning before a later hook would violate the rules of hooks. - Params read inside callbacks (which a component-level guard cannot narrow) use the established `?.toString() ?? ""` form, and existing in-callback guards were widened to cover the param they omitted (cycleId, moduleId, workspaceSlug). - usePublish(anchor) in space now takes `string | undefined`, matching what it already did at runtime (`publishMap?.[anchor] ?? {}`). This closes a real latent bug the shim was hiding: block-reactions.tsx did a raw `anchor.toString()` with no guard and no optional chaining. - The global create-issue modal (issue-modal/base.tsx) opens from workspace level, where project/module/workItem are genuinely absent, so it handles undefined inline rather than guarding -- a top-level guard would stop the modal opening outside a project. - TPowerKContext.params admits `string | null | undefined`: the providers merge store-derived ids (workItemDetails.project_id) that are nullable. Pre-existing debt the shim's Record<string, string> concealed. Custom hooks are never guarded -- returning null from a hook breaks its consumers' destructure and is itself a rules-of-hooks violation. The one hook affected (useMemberColumns) narrows internally instead. 2. The commit no longer needs --no-verify. lint-staged ran `oxlint --fix --deny-warnings`. Every rule in .oxlintrc.json is a warning (correctness/suspicious/perf are all "warn"), so --deny-warnings failed on any file carrying pre-existing debt -- and the repo has ~1300 such warnings. It was not specific to this branch: three arbitrary untouched files also trip it, so the hook was already stricter than the project's real gate (per-package check:lint budgets, e.g. web's --max-warnings=11957). It now uses a warning budget, which still fails a commit that floods in new debt. Also disables unicorn/no-array-sort: its autofix rewrites `.sort()` to `.toSorted()`, which does not compile against the apps' TS lib target, so `oxlint --fix` in the hook was emitting code that failed check:types. The 4 lint warnings this refactor actually introduced are fixed (accessible content on the new Link components; two codemod helpers hoisted to module scope). Guards were applied with a new jscodeshift codemod (packages/codemods/guard-route-params.ts) with 9 tests covering hook placement, hook refusal, idempotency, and aliased/props bindings. Verified: 0 type errors and a clean build across web/admin/space, 62 codemod tests pass, and this commit passes the pre-commit hook unmodified. Co-Authored-By: Claude <noreply@anthropic.com>
…rn/no-array-sort
The previous commit disabled unicorn/no-array-sort because its autofix rewrites
`.sort()` to `.toSorted()`, which did not compile. That treated the symptom. The
actual cause was packages/typescript-config/react-router.json overriding `lib` to
ES2022, while base.json already declares es2023 -- so the three apps extending it
were the only place in the monorepo where Array.prototype.toSorted was unknown to
TypeScript.
toSorted is already shipping: packages/constants/src/fetch-keys.ts has used it in
five places for some time, and it is present in the built web bundle. That package
typechecks because node-library.json declares lib es2023. Only the react-router
preset was inconsistent. `lib` (available APIs) is the correct knob here rather
than `target` (syntax downlevelling), since toSorted is a library method.
So: lib -> ES2023, rule re-enabled, and the autofix applied across 17 files.
`pnpm check:types` passes for the whole monorepo and a re-run of `oxlint --fix` is
now a no-op.
Letting the rule run repo-wide surfaced a second, worse autofix bug that the
narrower earlier run had hidden: eslint(no-useless-constructor) DELETES
constructor(store: RootStore, page: TPage, services: TBasePageServices) {}
from the community-edition ExtendedBasePage stub. The body is empty but the
signature is load-bearing -- BasePage extends it and calls
super(store, page, services) -- so the "fix" broke the subclass with
"Expected 0 arguments, but got 3". Suppressed locally (the repo's existing
convention, 144 such comments) with a note explaining why the constructor stays.
.sort() mutates in place; .toSorted() returns a copy. Audited every rewritten
call site: all 19 consume the result (`return x.toSorted(...)`,
`const y = [...z].toSorted(...)`, `groupedData[k] = groupedData[k].toSorted(...)`),
none relied on the in-place mutation, and several had a now-redundant spread copy.
Verified: monorepo check:types clean, all three apps build and lint, and the app
was driven end-to-end against the local backend -- 24/24 checks pass, including
that the toSorted-ed lists are still correctly ordered (sidebar nav renders
Home > Drafts > Your work > Stickies > Projects; project nav and members list
render in order) with no runtime errors.
Co-Authored-By: Claude <noreply@anthropic.com>
Its autofix deletes any constructor with an empty body, including ones whose
signature is load-bearing. It removed
constructor(store: RootStore, page: TPage, services: TBasePageServices) {}
from the community-edition ExtendedBasePage stub, whose signature exists precisely
so that BasePage's `super(store, page, services)` call and the enterprise variant
line up -- breaking the subclass with "Expected 0 arguments, but got 3".
oxlint has no per-rule autofix opt-out (--fix is all-or-nothing), so turning the
rule off is the only way to stop the fix from firing. It costs nothing: the rule
reports zero violations across the repo today, and this class of empty-bodied
stub constructor is a deliberate pattern in the CE/EE split -- so the rule is
wrong here by construction and would bite again on the next stub.
The local suppression added in 3a82d26 is no longer needed; the comment
explaining why the constructor stays is kept.
Verified: constructor survives `oxlint --fix`, repo-wide --fix is a no-op,
monorepo check:types clean, all three apps build and lint, 62 codemod tests pass.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Contributor
|
Important Review skippedToo many files! This PR contains 492 files, which is 342 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (492)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
Summary
Removes the Next.js compatibility layer that the December React Router + Vite migration left behind, and pays down the two type/tooling holes that removing it exposed.
The shims were not dead code — they were load-bearing: 307
next/navigationimports, 72next/link, 2next/script.nextisn't even a dependency; each app hand-rolled shims inapp/compat/next/wired up two independent ways (Vite aliases for runtime, ambient.d.tsfor TypeScript).No
next/*imports and no compat shims remain.next-themesis kept deliberately — it's a framework-agnostic library, not a Next.js API.Why this was more than a find-and-replace
Two shim behaviors were holding things up, and a naive swap to React Router breaks them silently:
Trailing slashes. Every
<Link href>androuter.push()ran throughensureTrailingSlash(), so all in-app URLs end in/. Five active-state checks comparepathnameagainst literals ending in/, and ~35 route constants (profile.ts→selected: "/assigned/",user-menu.tsx→href: /${slug}/) are authored with trailing slashes. Drop the slash and sidebar/tab highlighting stops working, with no error anywhere.Deferred navigation. web's
useRouterwrappednavigate()insetTimeout(…, 0). That is the only reasonauthentication-wrapper.tsx's 8 in-render redirects don't trip React's "cannot update a component while rendering" error.Both behaviors are preserved, now implemented in repo-owned code:
ensureTrailingSlashmoved into@plane/utils, anduseAppRouterrebuilt onuseNavigate(keeping the deferral). URLs and bookmarks are unchanged.The type hole the shim was hiding
The deleted shim declared
useParams<T = Record<string, string>>(): T— typing every route param as a non-optionalstring. React Router types themstring | undefined, which is correct. Removing the shim surfaced 164 latent unsafe accesses across 59 files.These are fixed at the source — no wrapper, no
as string, no!, no@ts-ignore, no neweslint-disable:if (!workspaceSlug || !projectId) return null— already used 95×). The guard goes after the last hook call, never straight afteruseParams; returning before a later hook violates the rules of hooks.?.toString() ?? "", and existing in-callback guards were widened to cover the param they omitted (cycleId,moduleId,workspaceSlug).usePublish(anchor)in space now takesstring | undefined, matching what it already did at runtime (publishMap?.[anchor] ?? {}). This closes a real latent bug:block-reactions.tsxdid a rawanchor.toString()with no guard and no optional chaining.issue-modal/base.tsx) opens from workspace level, whereprojectId/moduleId/workItemare genuinely absent — so it handlesundefinedinline. A top-level guard there would stop the modal from opening outside a project.Custom hooks are never guarded: returning
nullfrom a hook breaks its consumers' destructure and is itself a rules-of-hooks violation.Tooling fixes
react-router.jsonlib→ ES2023. It was pinned to ES2022 whilebase.jsonalready declaredes2023, making the three apps the only place TypeScript didn't knowArray.prototype.toSorted.toSortedis already shipping (packages/constants/src/fetch-keys.tsuses it in five places; it's in the built bundle), so only the preset was inconsistent. Withlibraised,unicorn/no-array-sortis enabled and its autofix applied across 17 files..sort()mutates in place;.toSorted()returns a copy. Audited all 19 rewritten sites — every one consumes the result; none relied on the mutation.no-useless-constructordisabled repo-wide. Its autofix deletes any empty-bodied constructor, includingExtendedBasePage's, whose signature is load-bearing (BasePagecallssuper(store, page, services)). It broke the subclass withExpected 0 arguments, but got 3. oxlint has no per-rule autofix opt-out, and the rule reports zero violations today, so disabling it is free — and this empty-stub pattern is deliberate in the CE/EE split.lint-stageduses a warning budget instead of--deny-warnings. Every rule in.oxlintrc.jsonis a warning, so--deny-warningsfailed on any file with pre-existing debt (~1300 repo-wide). This wasn't branch-specific — three arbitrary untouched files also trip it, so the hook was already stricter than the project's real gate (check:lint, e.g. web's--max-warnings=11957). The budget still fails a commit that floods in new debt.Dead code removed
The unreachable App Router
app/layout.tsx, the inertlayout.preload.tsx, an orphaned settings route page, the unusednext/imageshims,packages/typescript-config/nextjs.json, admin's unimportedinstance/loading.tsx, a stray"use client", and.nextreferences across 16 clean scripts, the ignore files,Dockerfile.web, and the aio Dockerfile.How the call sites were migrated
Three new jscodeshift codemods in
packages/codemods(using the package that already existed), with 62 passing tests covering the hazards: import merging into an existingreact-routerimport, theuseSearchParamstuple shape, destructuredconst { replace } = useRouter(),ReturnType<typeof useRouter>in a type position, aliased bindings, guard placement after hooks, hook refusal, idempotency — and license-header preservation (the first pass silently stripped the AGPL header from 29 files; caught, fixed, and covered by tests).Verification
Beyond
check:types(0 errors monorepo-wide), all three apps building, and lint passing, the production Docker image was driven end-to-end with Playwright against the local Django backend — real login, real session, real clicking. A green build only proves module resolution; the guards add early returns to ~60 render paths, and a wrong guard rendersnull— a blank pane that still returns HTTP 200.24/24 checks pass, asserting on the behaviors the shims were holding up:
/acme/afterrouter.push); every router-rendered<Link>href slash-terminatedpathname === item.hrefagainst slash-terminated constants) — "Drafts" highlights/?next_path=…with no React render-phase error (thesetTimeoutdeferral)useSearchParams().get()works (a bad tuple rewrite would throw)toSorted()-ed lists still ordered, not merely rendering: sidebar nav comes outHome > Drafts > Your work > Stickies > Projects<a>with no slash injected (a fix — the old shim rewrotehttps://plane.so/changelog→.../changelog/)Reviewer notes
next-themes+ssr: false) are unchanged frompreview— not introduced here.packages/ui'sControlLinkrenders a bare<a>with unslashed hrefs. It never went throughnext/linkand is untouched; itsrouter.pushpath is still slash-normalized.exhaustive-deps, 37no-unused-expressions, …). Escalatingcorrectnesstoerrorwould make the pre-commit hook genuinely strong, but that's a separate cleanup.🤖 Generated with Claude Code