feat: migrate to pnpm from yarn v1.22.22#7593
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRepository-wide migration from Yarn to pnpm: CI, Dockerfiles, setup scripts, and root configs updated; pnpm workspace and overrides added. Many package.json files switch to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Dev (local)
participant GH as GitHub Actions
participant Registry as Package Registry (pnpm)
participant BuildKit as Docker BuildKit
Dev->>GH: push PR (pnpm migration)
GH->>GH: checkout, setup Node
GH->>GH: corepack enable pnpm
GH->>Registry: pnpm install --frozen-lockfile
GH->>GH: pnpm run check:lint / format / build
GH->>BuildKit: docker build (multi-stage using pnpm)
BuildKit->>Registry: pnpm fetch --store-dir (uses cache)
BuildKit->>Registry: pnpm install --offline --frozen-lockfile --store-dir
BuildKit->>BuildKit: pnpm turbo run build --filter=<app>
BuildKit-->>GH: built image/artifacts
GH-->>Dev: CI results (lint/build)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
lifeiscontent
force-pushed
the
setup-pnpm-2
branch
from
ae4fb06 to
901409f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
apps/space/Dockerfile.space (1)
11-16: Use pnpm dlx for turbo prune to avoid PATH issues from global installsGlobal-installing turbo with pnpm doesn’t guarantee it’s on PATH in this Alpine base, and prune runs before node_modules exist. Prefer pnpm dlx to pin the version and avoid PATH pitfalls.
Apply this diff:
-ARG TURBO_VERSION=2.5.6 -RUN corepack enable pnpm && pnpm add -g turbo@${TURBO_VERSION} +ARG TURBO_VERSION=2.5.6 +RUN corepack enable pnpm @@ -RUN turbo prune --scope=space --docker +RUN pnpm dlx turbo@${TURBO_VERSION} prune --scope=space --docker
🧹 Nitpick comments (42)
CONTRIBUTING.md (1)
75-77: Clarifypnpm devusage in CONTRIBUTING.md
Thepnpm devsnippet at lines 75–77 should explicitly call out that it’s a root-level script (powered by Turborepo) and mention Corepack and version pinning. Suggested updates:• Update the code block to show it’s run from the repo root via Turbo:
@@ CONTRIBUTING.md:75-77 - ```bash - pnpm dev - ``` + ```bash + # From repo root – runs all app & package dev scripts via Turborepo + pnpm dev + ```• Add a note that Corepack must be enabled and that package.json pins
pnpm@10.12.1in thepackageManagerfield.
• Show per-app and recursive alternatives:# Single app pnpm --filter @plane/web dev # pnpm-native recursive run pnpm -r devapps/space/tsconfig.json (1)
20-26: Types configuration verified: no missing ambient types
No test-specific tsconfigs or test-framework dev-dependencies (jest/vitest/cypress) were found in apps/space. The added"types": ["node","react"]therefore safely limits ambient types to Node and React. If you introduce tests later, include their types (e.g.
jest,vitest) in a dedicated test tsconfig.Optional: consolidate duplicated
pluginsentries to avoid confusion
- In apps/space/tsconfig.json, remove the top-level
pluginsblock (lines 3–7) and keep only the one undercompilerOptions.plugins(lines 20–24):apps/space/tsconfig.json - "plugins": [ - { "name": "next" } - ], "compilerOptions": { … "plugins": [ { "name": "next" } ],packages/constants/tsconfig.json (1)
8-10: Avoid coupling a constants package to React ambient types unless strictly requiredIncluding
"react"here pulls in React typings for this package’s compilation and can mask unintended React/DOM usage in a pure constants library. If this package doesn’t contain JSX/React types, prefer keeping only Node (or notypesat all) to minimize ambient-type surface.Consider:
- "allowSyntheticDefaultImports": true, - "types": ["node", "react"] + "allowSyntheticDefaultImports": true + // Add types only if needed, e.g. "types": ["node"]If React types are truly needed by this package, ignore this comment.
apps/web/tsconfig.json (1)
13-14: Narrowing ambient types via "types" can hide needed globals (tests, Storybook, Cypress).Specifying "types" restricts auto-included @types packages to only Node and React. If this app relies on other globals (e.g., Jest, Cypress, Storybook, Google Maps), you’ll see missing-type errors until those are explicitly added here or in env-specific tsconfigs.
If the intent is not to restrict, consider removing the "types" override:
"plugins": [{ "name": "next" }], - "strictNullChecks": true, - "types": ["node", "react"] + "strictNullChecks": trueOtherwise, ensure all required env types are listed (e.g., "jest", "cypress") or split into per-env tsconfig extends.
apps/space/Dockerfile.dev (1)
8-9: Global turbo install appears unused; simplify and pin installs.The CMD runs pnpm scripts directly, not turbo. Installing turbo globally adds image weight without benefit. Also, consider lockfile enforcement for reproducibility.
Apply:
-RUN corepack enable pnpm && pnpm add -g turbo -RUN pnpm install +RUN corepack enable pnpm +RUN pnpm install --frozen-lockfileapps/admin/tsconfig.json (1)
17-19: Refine “types” in apps/admin/tsconfig.json to only include necessary entriesNext.js already wires types via next-env.d.ts, and restricting the
"types"array to["node","react"]can be both redundant and overly restrictive:
- apps/admin/next-env.d.ts is present and provides Next.js/React typings
@types/nodeand@types/reactare installed, and Node APIs (e.g.,process.env…) are used- No in-package tests (Jest/Cypress/Playwright) were detected, so limiting ambient types isn’t blocking test types here
Recommended optional refactor (either pick one):
--- a/apps/admin/tsconfig.json +++ b/apps/admin/tsconfig.json @@ lines 17–19 - "types": ["node", "react"] + "types": ["node"]Or remove
"types"entirely and rely on Next.js + automatic type inclusion (add separate test-specific tsconfigs if you introduce tests):--- a/apps/admin/tsconfig.json +++ b/apps/admin/tsconfig.json @@ lines 17–19 - "types": ["node", "react"] + // Rely on next-env.d.ts and automatic @types inclusionpackages/types/tsconfig.json (1)
7-8: Limit ambient types to React onlyNode ambient types aren’t used in this package (no
processorBufferreferences), while React types are heavily referenced. Update your tsconfig to remove"node"and keep only React:• File: packages/types/tsconfig.json
Lines 7–8"strictNullChecks": true, - "types": ["node", "react"] + "types": ["react"].gitignore (1)
32-33: Deduplicate pnpm debug log ignore patternsYou now have both a wildcard and a specific pnpm-debug.log entry. The wildcard already covers the specific filename; keep one for clarity.
Apply this diff to remove the redundant specific entry:
- pnpm-debug.logAlso applies to: 65-65
packages/ui/tsconfig.json (1)
5-7: Remove unnecessary Node types from UI tsconfigA scan of all
.ts/.tsxfiles inpackages/ui/srcfound no imports of Node core modules or use of Node globals, so it’s safe to drop the"node"ambient types here.Suggested change:
--- packages/ui/tsconfig.json @@ lines 5-7 - "types": ["node", "react"] + "types": ["react"]packages/utils/tsconfig.json (1)
5-7: Limit ambient Node types inpackages/utilsWe ran a search across
packages/utils/srcfor Node-specific APIs (fs,path,process, etc.) and found no usages. To prevent accidental Node globals from leaking into browser consumers, you can safely remove the"node"ambient types:• packages/utils/tsconfig.json
- "types": ["node", "react"] + "types": ["react"]If this package is ever consumed by a Node environment, you can re-add
"node"then.packages/hooks/tsconfig.json (1)
5-7: Be cautious: adding "types" narrows ambient types; verify if additional globals are needed (e.g., vite/client, jest).Setting "types" to ["node","react"] restricts ambient types to only these. If hooks import env globals (Vite, Next.js types) or rely on test types in the same config chain, they'll be missing unless explicitly listed. Since include targets only src, this might be fine. Still, please verify usage or drop "types" if not strictly required by this package.
Would you like me to scan for imports/usages that require additional ambient types and propose the exact additions?
apps/live/Dockerfile.dev (2)
7-9: Avoid global turbo install; pin pnpm via Corepack; enforce lockfile on install.
- Global turbo is not required if dev scripts invoke the workspace-local turbo via pnpm. Removing it reduces image size and avoids version drift.
- Consider pinning pnpm via Corepack (honor root packageManager) and use --frozen-lockfile for reproducible dev installs.
Suggested change:
-RUN corepack enable pnpm && pnpm add -g turbo -RUN pnpm install +RUN corepack enable pnpm +RUN pnpm install --frozen-lockfileIf dev scripts rely on a global turbo, I can adjust to pnpm dlx turbo or ensure turbo is a root devDependency instead. Confirm preference.
15-15: CMD looks good; consider adding -w to ensure root workspace context.If dev relies on workspace root scripts, using the workspace root explicitly can be safer:
-CMD ["pnpm","dev", "--filter=live"] +CMD ["pnpm","-w","dev","--filter=live"]Optional; keep as-is if the current command resolves correctly in your context.
apps/admin/Dockerfile.dev (2)
8-9: Same as live: remove global turbo, pin pnpm via Corepack, and use --frozen-lockfile.Suggested change:
-RUN corepack enable pnpm && pnpm add -g turbo -RUN pnpm install +RUN corepack enable pnpm +RUN pnpm install --frozen-lockfileIf admin's dev script depends on global turbo, I can switch it to use the workspace turbo or pnpm dlx turbo.
17-17: Nit: consider explicit workspace root for dev.-CMD ["pnpm", "dev", "--filter=admin"] +CMD ["pnpm", "-w", "dev", "--filter=admin"]apps/web/Dockerfile.dev (2)
8-9: Repeat of the above: drop global turbo, pin pnpm, enforce lockfile.Suggested change:
-RUN corepack enable pnpm && pnpm add -g turbo -RUN pnpm install +RUN corepack enable pnpm +RUN pnpm install --frozen-lockfile
13-13: Optional: run dev from workspace root explicitly.-CMD ["pnpm", "dev", "--filter=web"] +CMD ["pnpm", "-w", "dev", "--filter=web"]pnpm-workspace.yaml (1)
11-14: Catalog is good; recommend pinning turbo without a range to avoid surprise upgrades.Catalog is typically used to standardize exact versions across the workspace. Using a caret on turbo weakens that guarantee.
Suggested change:
catalog: sharp: "0.33.5" - turbo: "^2.5.6" + turbo: "2.5.6" typescript: "5.6.3"Do you want me to scan package.json files to normalize turbo versions to the catalog across the workspace?
packages/types/package.json (1)
34-40: Align root TypeScript version with workspace packagesRoot package.json declares
"typescript": "5.6.3"in its devDependencies (line 38), while every workspace package pins"typescript": "5.8.3". This mismatch can lead to confusion when running TS commands from the monorepo root.• package.json (root) @ line 38:
"typescript": "5.6.3"
• All workspaces:"typescript": "5.8.3"Consider bumping the root devDependency to match the workspaces:
--- package.json +++ package.json @@ devDependencies - "typescript": "5.6.3" + "typescript": "5.8.3"Alternatively, if you prefer to standardize on 5.6.x, downgrade each workspace to
"typescript": "5.6.3".apps/live/src/core/helpers/logger.ts (1)
40-40: Avoid double-casting through unknown; prefer a single, explicit cast or inferred typeThe unknown bridge hides real type incompatibilities. If the goal is to expose a pino Logger, a single cast is clearer. Alternatively, use typeof to preserve the exact type.
Apply one of the following:
- Prefer a single cast:
-export const manualLogger: Logger = logger.logger as unknown as Logger; +export const manualLogger: Logger = logger.logger as Logger;
- Or keep the exact type without any casts:
-export const manualLogger: Logger = logger.logger as unknown as Logger; +export const manualLogger = logger.logger; +export type ManualLogger = typeof manualLogger;turbo.json (2)
22-22: Consider adding package.json to globalDependencies for more reliable cache bustingYou already track pnpm-lock.yaml, pnpm-workspace.yaml, and .npmrc. Changes to root pnpm.overrides in package.json can also affect installs/builds. Tracking package.json helps avoid stale caches across the monorepo.
- "globalDependencies": ["pnpm-lock.yaml", "pnpm-workspace.yaml", ".npmrc"], + "globalDependencies": ["pnpm-lock.yaml", "pnpm-workspace.yaml", ".npmrc", "package.json"],
38-39: Be explicit about excluding node_modules and .turbo in lint inputsTurbo typically ignores node_modules, but being explicit avoids accidental regressions and reduces watch/load. Also exclude .turbo.
- "inputs": ["**/*", "!**/.next/**", "!**/dist/**"] + "inputs": ["**/*", "!**/.next/**", "!**/dist/**", "!**/node_modules/**", "!**/.turbo/**"]package.json (1)
24-26: Avoid using "latest" for Prettier; pin to a stable major/minor for reproducibility"latest" can suddenly change formatting and break CI. Prefer a pinned version (exact or at least a major range). Also consider aligning the devDependency version of turbo with the override to avoid mixed signals.
- "prettier": "latest", - "prettier-plugin-tailwindcss": "^0.6.14", - "turbo": "^2.5.6" + "prettier": "^3.0.0", + "prettier-plugin-tailwindcss": "^0.6.14", + "turbo": "2.5.6"If you want to keep Prettier strictly pinned, replace ^3.0.0 with the exact version resolved in your CI after the first pnpm install.
.dockerignore (1)
19-64: Nice expansion; consider ignoring PNPM store and lint caches tooGreat coverage. To keep Docker build contexts even leaner, add PNPM local store (if ever configured inside the repo) and ESLint cache.
# Misc *.pem *.key +.eslintcache +.pnpm-store/ +**/.pnpm-store/If your .npmrc sets a custom local store-dir within the repo, the above will significantly reduce context size.
apps/admin/Dockerfile.admin (1)
11-16: Avoid global turbo install; use pnpm dlx and pin pnpm via CorepackGlobal installs add state and can be flaky in CI. Use pnpm dlx for turbo (no global state) and let Corepack activate the repo’s declared pnpm version from package.json.
Apply this diff:
ARG TURBO_VERSION=2.5.6 -RUN corepack enable pnpm && pnpm add -g turbo@${TURBO_VERSION} +RUN corepack enable && pnpm dlx turbo@${TURBO_VERSION} --version COPY . . -RUN turbo prune --scope=admin --docker +RUN pnpm dlx turbo@${TURBO_VERSION} prune --scope=admin --dockerapps/web/Dockerfile.web (1)
12-16: Prefer pnpm dlx for turbo; avoid global installSame rationale as admin: ephemeral, pinned execution without global state.
Apply this diff:
ARG TURBO_VERSION=2.5.6 -RUN corepack enable pnpm && pnpm add -g turbo@${TURBO_VERSION} +RUN corepack enable && pnpm dlx turbo@${TURBO_VERSION} --version COPY . . -RUN turbo prune --scope=web --docker +RUN pnpm dlx turbo@${TURBO_VERSION} prune --scope=web --dockerapps/live/Dockerfile.live (3)
9-11: Remove apk update; it’s unnecessary with --no-cache and adds extra layersapk add --no-cache already fetches fresh indexes. Dropping apk update avoids extra layers and reduces image size.
Apply this diff:
-RUN apk update -RUN apk add --no-cache libc6-compat +RUN apk add --no-cache libc6-compat ... -FROM base AS installer -RUN apk update -RUN apk add --no-cache libc6-compat +FROM base AS installer +RUN apk add --no-cache libc6-compatAlso applies to: 23-25
13-17: Use pnpm dlx for turbo instead of global installAvoid global state and rely on a pinned version per build.
Apply this diff:
ARG TURBO_VERSION=2.5.6 -RUN corepack enable pnpm && pnpm add -g turbo@${TURBO_VERSION} +RUN corepack enable && pnpm dlx turbo@${TURBO_VERSION} --version COPY . . -RUN turbo prune --scope=live --docker +RUN pnpm dlx turbo@${TURBO_VERSION} prune --scope=live --docker
27-37: Build needs devDeps, but runtime doesn’t; prune devDeps after build to slim the imageInstall all deps to build, then prune to production before copying node_modules into the runner. This reduces image size and attack surface without breaking the build.
Apply this diff:
COPY turbo.json turbo.json -RUN --mount=type=cache,id=pnpm-store,target=/pnpm/store pnpm install --offline --frozen-lockfile --store-dir=/pnpm/store +RUN --mount=type=cache,id=pnpm-store,target=/pnpm/store pnpm install --offline --frozen-lockfile --store-dir=/pnpm/storeThen after the build step (below), add a prune:
-RUN pnpm turbo run build --filter=live +RUN pnpm turbo run build --filter=live +RUN pnpm prune --prodOptional hardening: set NODE_ENV=production in runner.
FROM base AS runner WORKDIR /app +ENV NODE_ENV=production.npmrc (2)
9-12: Peer dep settings may mask mismatchesauto-install-peers=true and strict-peer-dependencies=false can hide real peer issues across the monorepo. If you hit subtle runtime problems, consider reverting to defaults and fixing peer ranges explicitly.
29-31: Caution with resolution-mode=highestThis can increase churn and unexpected resolutions when adding packages. The lockfile mitigates it, but if you see frequent diffs or duplication elsewhere, consider the default resolution mode.
.github/workflows/pull-request-build-lint-web-apps.yml (2)
41-43: Pin pnpm version via Corepack to ensure consistencyEnable shims and activate the repo-declared pnpm version (package.json: packageManager). This avoids environment drift.
Apply this diff:
- - name: Enable Corepack and pnpm - run: corepack enable pnpm + - name: Enable Corepack and use repo pnpm version + run: | + corepack enable + node -p "require('./package.json').packageManager" | xargs corepack use
44-55: Add PNPM cache and broaden path triggers for PNPM/Turbo config changes
- Cache PNPM in setup-node to speed installs.
- Include pnpm-lock.yaml, pnpm-workspace.yaml, .npmrc, and turbo.json in workflow paths so builds run when tooling/config changes.
Suggested YAML changes (non-diff):
In “Set up Node.js” step:
with:
node-version-file: ".nvmrc"
cache: "pnpm"In on.pull_request.paths:
- "pnpm-lock.yaml"
- "pnpm-workspace.yaml"
- ".npmrc"
- "turbo.json"
apps/space/Dockerfile.space (2)
56-56: Ensure turbo is available for build or use dlx consistentlyThis relies on turbo being present in the pruned workspace’s devDependencies. To avoid coupling to that, invoke via pnpm dlx with the pinned version, consistent with the prune step.
Optional change:
-RUN pnpm turbo run build --filter=space +RUN pnpm dlx turbo@${TURBO_VERSION} run build --filter=spaceIf you prefer keeping pnpm turbo, please confirm turbo is included in the pruned root package.json produced by turbo prune.
35-52: Duplicate ARG/ENV in installer stage may be unnecessaryNEXT_PUBLIC_* vars are also set in the runner stage. Unless you need them at build time for Next’s compile-time replacement, consider removing from installer to reduce noise.
apps/web/package.json (1)
45-45: New dependency: assess bundle impact of emoji-picker-react.emoji-picker-react can add noticeable client bundle weight. Consider lazy-loading it where used to minimize initial page load.
packages/propel/package.json (1)
35-37: Dev configs to workspace: — consistent change. Also consider peerDependencies for library React packages.*Optional: as a UI library, declaring react and react-dom as peerDependencies (with matching devDependencies for local dev) can prevent duplicate Reacts in consumers. Not required for this PR; just a heads-up for packaging best practices.
apps/live/package.json (1)
27-28: Workspace: linking approved; Node ≥ 20 requirement verified*
- Both
apps/live/Dockerfile.devandapps/live/Dockerfile.liveusenode:22-alpine.- The
devandstartscripts inapps/live/package.jsoncorrectly include--env-file.Optional: add
"engines": { "node": ">=20" }to
apps/live/package.jsonto enforce the minimum Node version in local setups.packages/utils/package.json (2)
24-25: Good switch to pnpm workspace protocol; consider using workspace:^ for semver-aligned bumps
workspace:*is valid and will link local packages. If you want downstream bump automation to follow normal semver ranges (and to better reflect intended compatibility), preferworkspace:^. For private-only packages,workspace:*is fine; this is an optional consistency tweak.Apply if desired:
- "@plane/constants": "workspace:*", - "@plane/types": "workspace:*", + "@plane/constants": "workspace:^", + "@plane/types": "workspace:^",
35-36: Align devDependencies to workspace:^ for consistencySame note as dependencies: using
workspace:^can make version intent clearer across the monorepo. Optional.- "@plane/eslint-config": "workspace:*", - "@plane/typescript-config": "workspace:*", + "@plane/eslint-config": "workspace:^", + "@plane/typescript-config": "workspace:^",packages/ui/package.json (2)
37-40: Workspace migration looks correct; consider workspace:^ to encode compatibility intentThese internal deps will link properly under pnpm. If you prefer semver-aware intent and smoother publish workflows (for any future non-private packages),
workspace:^is a conventional choice. Optional.- "@plane/constants": "workspace:*", - "@plane/hooks": "workspace:*", - "@plane/utils": "workspace:*", - "@plane/types": "workspace:*", + "@plane/constants": "workspace:^", + "@plane/hooks": "workspace:^", + "@plane/utils": "workspace:^", + "@plane/types": "workspace:^",
56-58: Dev config packages: optional switch to workspace:^ for consistency with dependenciesNot required, but it keeps a uniform pattern across the repo.
- "@plane/eslint-config": "workspace:*", - "@plane/tailwind-config": "workspace:*", - "@plane/typescript-config": "workspace:*", + "@plane/eslint-config": "workspace:^", + "@plane/tailwind-config": "workspace:^", + "@plane/typescript-config": "workspace:^",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (49)
.dockerignore(1 hunks).github/workflows/pull-request-build-lint-web-apps.yml(1 hunks).gitignore(3 hunks).npmrc(1 hunks).yarnrc.yml(0 hunks)CONTRIBUTING.md(1 hunks)apps/admin/Dockerfile.admin(5 hunks)apps/admin/Dockerfile.dev(1 hunks)apps/admin/package.json(2 hunks)apps/admin/tsconfig.json(1 hunks)apps/live/Dockerfile.dev(1 hunks)apps/live/Dockerfile.live(4 hunks)apps/live/package.json(2 hunks)apps/live/src/core/helpers/logger.ts(1 hunks)apps/live/tsconfig.json(1 hunks)apps/space/.gitignore(0 hunks)apps/space/Dockerfile.dev(1 hunks)apps/space/Dockerfile.space(5 hunks)apps/space/package.json(2 hunks)apps/space/tsconfig.json(1 hunks)apps/web/Dockerfile.dev(1 hunks)apps/web/Dockerfile.web(5 hunks)apps/web/package.json(3 hunks)apps/web/tsconfig.json(1 hunks)package.json(1 hunks)packages/constants/package.json(1 hunks)packages/constants/tsconfig.json(1 hunks)packages/decorators/package.json(1 hunks)packages/decorators/tsconfig.json(1 hunks)packages/editor/package.json(2 hunks)packages/hooks/package.json(1 hunks)packages/hooks/tsconfig.json(1 hunks)packages/i18n/package.json(1 hunks)packages/logger/package.json(1 hunks)packages/logger/tsconfig.json(1 hunks)packages/propel/package.json(2 hunks)packages/services/package.json(1 hunks)packages/services/tsconfig.json(1 hunks)packages/shared-state/package.json(1 hunks)packages/types/package.json(1 hunks)packages/types/tsconfig.json(1 hunks)packages/ui/.storybook/main.ts(1 hunks)packages/ui/package.json(2 hunks)packages/ui/tsconfig.json(1 hunks)packages/utils/package.json(2 hunks)packages/utils/tsconfig.json(1 hunks)pnpm-workspace.yaml(1 hunks)setup.sh(1 hunks)turbo.json(2 hunks)
💤 Files with no reviewable changes (2)
- apps/space/.gitignore
- .yarnrc.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-04T16:22:44.344Z
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
Applied to files:
packages/ui/.storybook/main.ts
🧬 Code Graph Analysis (1)
apps/live/src/core/helpers/logger.ts (1)
packages/logger/src/config.ts (1)
logger(61-66)
🔇 Additional comments (39)
packages/shared-state/package.json (1)
21-21: Good move switching toworkspace:*for the internal eslint configThis aligns with pnpm workspaces and ensures local resolution without version skew. No issues from me.
packages/decorators/tsconfig.json (1)
9-9: Adding"types": ["node"]is appropriate for a Node-targeted libraryThis should stabilize ambient Node typings without pulling in browser/React types. Looks good.
packages/logger/package.json (1)
37-38: Switch to workspace: for internal devDeps looks good.*Aligns with PNPM workspaces and will resolve locally without publishing. No issues spotted.
packages/ui/.storybook/main.ts (1)
7-7: Comment update to generic PnP is appropriate.No functional changes. This keeps the helper aligned with monorepo/PnP setups. Also acknowledges prior decision to retain the CSS loader override in this file until the upstream Storybook issue is resolved.
packages/hooks/package.json (1)
27-27: workspace: for internal eslint config is consistent with the PNPM migration.*Looks good; no runtime impact.
packages/logger/tsconfig.json (1)
15-16: Confirmed: @types/node is declared as a devDependency
Thepackages/logger/package.jsonalready lists"@types/node": "^22.5.4"underdevDependencies, so TypeScript will correctly resolve the ambient Node types with pnpm’s strict node_modules.apps/live/tsconfig.json (1)
22-22: LGTM – Node types are correctly configuredConfirmed that
@types/node@^20.14.9is listed underapps/live/package.jsondevDependencies. Approving these changes.setup.sh (1)
80-83: Version Pinning VerifiedThe root package.json has
"packageManager": "pnpm@10.12.1", and running Corepack confirms pnpm 10.12.1 is used across environments—no updates needed..gitignore (1)
79-83: pnpm lockfile confirmed and trackedpnpm-lock.yaml is present at the repository root and committed, ensuring reproducible installs.
packages/decorators/package.json (1)
28-29: LGTM: move internal devDependencies to workspace rangesSwitching to "workspace:*" is consistent with pnpm workspaces and keeps local packages aligned.
pnpm-workspace.yaml (2)
16-20: onlyBuiltDependencies is valid in pnpm-workspace.yamlThe
onlyBuiltDependenciessetting is officially supported inpnpm-workspace.yamlfor workspace-wide control (pnpm v10+). There is no.npmrckey namedonly-built-dependencies—use:onlyBuiltDependencies: - turbo - sharpin
pnpm-workspace.yaml, or for per-package control:// package.json { "pnpm": { "onlyBuiltDependencies": ["turbo", "sharp"] } }No changes are needed to move this to
.npmrc.Likely an incorrect or invalid review comment.
21-22: Retain useNodeVersion for pnpm-managed Node pinning
pnpm natively supports auseNodeVersionkey in the rootpnpm-workspace.yaml(oruse-node-versionin.npmrc) to auto-install and use the specified Node.js version forpnpm runandpnpm node. You do not need to remove it.If you’d like broader enforcement across editors, shells, and CI, you can layer on one or more of the following:
- Add in
package.jsonand enable PNPM’s engine checks ("engines": { "node": "22.18.0" }nodeVersion+engineStrict) in your workspace config.- Commit a
.nvmrcor.node-versionfile for local tooling.- Pin via Volta (or asdf/nvm) in
package.jsonto ensure every environment—including CI—uses the same Node.Likely an incorrect or invalid review comment.
packages/types/package.json (1)
34-35: Switching internal configs to workspace: looks good*Using workspace:* for internal packages is appropriate for PNPM workspaces.
package.json (1)
27-41: esbuild override confirmed compatible with tsup 8.4.0tsup 8.4.0 ships with (and depends on) esbuild ^0.25.0, so pinning esbuild to 0.25.0 in your pnpm.overrides aligns exactly and won’t introduce incompatibilities.
• File: package.json → pnpm.overrides →
"esbuild": "0.25.0"
• (Optional) To double-check that only one copy is installed, run:pnpm ls esbuildapps/admin/Dockerfile.admin (3)
56-56: Build invocation looks goodpnpm turbo run build --filter=admin aligns with the workspace build pattern and the prune step.
98-98: Verify server entry path is correct at runtimeDepending on Next.js standalone output, the server entry can be at ./server.js or nested (e.g., apps/admin/server.js). Confirm this path matches what .next/standalone emits for admin.
If needed, prefer the standalone’s root entry:
- CMD ["node", "server.js"]
27-34: LGTM: PNPM fetch/install pattern and version locking verified
- Dockerfile.admin (apps/admin/Dockerfile.admin, lines 27–34): fetch + offline + cache pattern is correct.
- Root package.json contains
packageManager: "pnpm@10.12.1", ensuring Corepack uses a consistent pnpm version in Docker and CI.apps/web/Dockerfile.web (3)
30-37: Solid PNPM offline install and cachingThe fetch + offline install with a mounted store will keep layers stable and builds faster.
66-66: Build step LGTMpnpm turbo run build --filter=web is consistent with prune scope.
115-115: Confirm server start pathAs with admin, verify that the entry point exists as apps/web/server.js in the standalone output. If not, consider using node server.js.
apps/live/Dockerfile.live (2)
41-41: Build command aligns with prune scopeNo issues spotted.
58-58: CMD looks correct for copied layoutYou’re copying dist to /app/live and starting live/server.js. This matches the COPY paths.
.npmrc (1)
17-22: Workspace and public hoist settings look goodThese should reduce duplication for common tooling and keep workspace links intact.
apps/space/Dockerfile.space (1)
98-98: Confirm server.js path for Next standalone outputCMD points to apps/space/server.js. With Next 14 standalone output, paths can differ based on config. Please confirm this file exists in the copied standalone root.
Run inside the built image: ls -l apps/space/server.js (or adjust CMD to the correct path).
apps/admin/package.json (2)
21-27: Switch to workspace: looks good*Moving internal @plane deps to workspace:* aligns with PNPM workspaces and improves consistency.
42-42: Adding sharp: ensure Docker builds don’t use offline installs or provide libvipssharp requires downloading prebuilt binaries/libvips at install. If your Dockerfiles use pnpm install --offline (as in apps/space), the install will likely fail. Either drop --offline for images building this app or install system libvips/toolchain.
Would you like me to propose Dockerfile adjustments for the admin app similar to space?
packages/i18n/package.json (1)
18-23: Workspace migration acknowledgedSwitching to workspace:* for internal packages is correct and consistent with the PNPM migration.
apps/space/package.json (2)
24-31: Workspace: specifiers LGTM*Internal dependencies moved to workspace:* are consistent and correct.
50-51: Adding sharp: align Docker install flags to avoid failuresAs noted for admin, ensure your Docker build for space doesn’t use pnpm install --offline or, if you prefer offline installs, add system libvips and toolchain.
I can open a follow-up to make the Dockerfile.space change if you confirm the preferred approach (online install vs. system libvips).
packages/editor/package.json (1)
43-48: Workspace migration LGTMUsing workspace:* for internal @plane deps in both dependencies and devDependencies is correct. No further issues spotted.
packages/constants/package.json (1)
30-34: Workspace protocol switch looks good.Using workspace:* for internal packages aligns with pnpm workspaces and prevents accidental external resolution. Given this package is private, this is safe.
apps/web/package.json (3)
24-32: Good move to workspace: for internal packages.*This will ensure local linking and consistent versions across the monorepo with pnpm.
72-74: Dev config packages switched to workspace: — aligned with the migration.*
64-64: Environment support in place for sharp 0.33
All Node-based Dockerfiles already target node:22-alpine and installlibc6-compat. Sharp 0.33+ ships prebuilt musl binaries, so no extra vips or glibc packages are required in Alpine containers. If you have any non-Docker runtimes, just verify they run Node ≥ 16 LTS and glibc ≥ 2.28.packages/services/package.json (2)
18-18: Internal dependency moved to workspace: — good.*Ensures local linking under pnpm.
22-23: Dev config packages switched to workspace: — consistent with the rest of the repo.*packages/propel/package.json (1)
25-25: Switching @plane/utils to workspace: looks correct.*Monorepo-local resolution will avoid version drift.
apps/live/package.json (1)
49-50: Dev config packages to workspace: — consistent with the pnpm migration.*packages/utils/package.json (1)
24-36: ✅ All internal @plane/ dependencies useworkspace:protocol*
Ran a repo-wide scan and confirmed:
- No
@plane/*entries remain on non-workspace:specifiers.pnpm-workspace.yamlis present at the root and includespackages/*.No further changes required.
| # Enforce Node version for consistent installs | ||
| use-node-version=22.18.0 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align Node version across .npmrc and .nvmrc to avoid install-time mismatches
use-node-version=22.18.0 is strict. If .nvmrc differs, pnpm installs can fail. Ensure both match and CI uses the same Node.
Run to compare:
🏁 Script executed:
#!/bin/bash
echo "nvmrc: $(cat .nvmrc 2>/dev/null || echo missing)"
echo "npmrc use-node-version: $(rg -n '^use-node-version=' .npmrc | sed 's/.*=//')"Length of output: 179
Align Node versions across .nvmrc and .npmrc
Our verification shows:
- .nvmrc: lts/jod
- .npmrc (use-node-version): 22.18.0
Since pnpm’s use-node-version check is strict, mismatched values will cause install failures in CI or on developers’ machines. Please update one of the following so both files specify the same version:
• Change .nvmrc → 22.18.0
• Or set use-node-version=lts/jod in .npmrc (if you intend to rely on the LTS alias)
After updating, ensure your CI pipeline uses this same Node version.
🤖 Prompt for AI Agents
In .npmrc around lines 23 to 25, the Node version set (use-node-version=22.18.0)
conflicts with .nvmrc (lts/jod); update one of them so both match — either
change .nvmrc to "22.18.0" or change .npmrc to "use-node-version=lts/jod" — then
verify and align the CI/node runtime configuration to use the same version
string you choose.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (6)
apps/web/core/components/modules/analytics-sidebar/issue-progress.tsx (1)
66-74: Fix progress percentage logic (wrong variable check can cause divide-by-zero)In the “issues” branch, the condition checks
completedIssuestwice and never validatestotalIssues. This can computeInfinity/NaNiftotalIssuesis 0.Apply this diff:
- : completedIssues != 0 && completedIssues != 0 - ? Math.round((completedIssues / totalIssues) * 100) + : completedIssues != 0 && totalIssues != 0 + ? Math.round((completedIssues / totalIssues) * 100) : 0apps/web/core/components/workspace-notifications/sidebar/notification-card/item.tsx (1)
39-41: Potential runtime crash: unsafe access tonotificationand missing optional chainingLine 40 dereferences
notification.triggered_by_detailswithout null checks. IfuseNotificationhasn’t loaded yet, render will throw before the early return guard. Also, Line 39 accessesissue_activity.fieldwithout optional chaining onissue_activity, which can break whenissue_activityis absent.Apply safer optional chaining:
- const notificationField = notification?.data?.issue_activity.field || undefined; - const notificationTriggeredBy = notification.triggered_by_details || undefined; + const notificationField = notification?.data?.issue_activity?.field || undefined; + const notificationTriggeredBy = notification?.triggered_by_details || undefined;packages/propel/src/charts/area-chart/root.tsx (1)
84-95: Avoid NaN from divide-by-zero when data has a single pointWith a single datum, data.length - 1 is 0, making ratio NaN. Recharts may silently drop the series. Short-circuit for fewer than 2 points.
- if (!data || data.length === 0) return []; + if (!data || data.length < 2) return [];apps/web/core/components/dropdowns/estimate.tsx (1)
22-31: Align onChange/value/option types to include nullOptions include a "no estimate" entry with value null, and Props.value allows null, but Props.onChange and dropdownOnChange exclude null. This can cause type unsoundness and consumer bugs when "no estimate" is selected.
Apply this diff to make types consistent:
type Props = TDropdownProps & { button?: ReactNode; dropdownArrow?: boolean; dropdownArrowClassName?: string; - onChange: (val: string | undefined) => void; + onChange: (val: string | null | undefined) => void; onClose?: () => void; projectId: string | undefined; value: string | undefined | null; renderByDefault?: boolean; };- const dropdownOnChange = (val: string | undefined) => { + const dropdownOnChange = (val: string | null | undefined) => { onChange(val); handleClose(); };Also applies to: 150-153
packages/propel/src/menu/types.ts (1)
1-1: Import React types to avoid “Cannot find name 'React'” in type-only fileThis file references React.ReactNode but doesn’t import React types. In TS projects using the automatic JSX runtime, the React namespace isn’t globally available in type space; this can break builds.
Add a type-only import at the top:
import type React from "react";packages/propel/src/menu/menu.tsx (1)
53-57: Bug:close()here calls the global window.close(), not the menu API
close()is not defined in this scope, so this will call the browser’swindow.close()(and often no-op due to browser restrictions). It won’t close the menu and may confuse behavior.Remove it and rely on BaseMenu’s default item behavior (menus usually close on selection) or explicitly wire a proper close from context if needed.
- onClick={(e) => { - close(); - onClick?.(e); - submenuContext?.closeSubmenu(); - }} + onClick={(e) => { + onClick?.(e); + submenuContext?.closeSubmenu(); + }}
🧹 Nitpick comments (32)
packages/ui/src/button/button.tsx (1)
4-4: Nit: use type-only imports for TS types to improve clarity and bundling.Separating value imports from type imports helps with TS 4.5+ verbatimModuleSyntax/isolatedDeclarations setups and avoids accidental runtime named import lookups.
-import { getIconStyling, getButtonStyling, TButtonVariant, TButtonSizes } from "./helper"; +import { getIconStyling, getButtonStyling } from "./helper"; +import type { TButtonVariant, TButtonSizes } from "./helper";.gitignore (4)
27-27: Prefer wildcard ignore for TypeScript build info filesCovers all incremental build artifacts across packages, not just the default filename.
-tsconfig.tsbuildinfo +*.tsbuildinfo
33-35: Deduplicate pnpm debug log ignore patterns
pnpm-debug.log*at Line 33 already coverspnpm-debug.logat Line 65. Keeping the glob once avoids drift and keeps the file lean.- pnpm-debug.logAlso consider removing duplicate npm/yarn log patterns later in the file (e.g.,
npm-debug.log,yarn-error.log) since they’re already covered in the Debug section. I can propose a full dedupe pass if helpful.Also applies to: 65-65
73-73: Consolidate lockfile ignores; consider ignoring yarn.lock during pnpm migration
package-lock.jsonappears twice (Lines 73 and 80). Keep a single entry under the "lock files" section.- If the repo is standardizing on pnpm, ignoring
yarn.lockcan prevent accidental reintroduction.- package-lock.json# lock files package-lock.json +yarn.lockPlease confirm that:
- pnpm is the sole package manager (so ignoring
yarn.lockis desired).pnpm-lock.yamlis tracked at the repo root (expected per this migration).Also applies to: 79-81
82-83: Remove stray blank lines (nit)Not functional, just tidies the ignore list.
- -packages/propel/src/charts/pie-chart/active-shape.tsx (1)
3-3: Prefer a type-only import (and verify the import path’s stability).Use
import typeto avoid emitting a runtime import for a type, which is especially important with TS configs using verbatimModuleSyntax/isolatedModules. Also, therecharts/types/...path is semi-internal; ifPieSectorDataItemis re-exported fromrecharts, prefer that. If not, keep the current path but with a type-only import.-import { PieSectorDataItem } from "recharts/types/polar/Pie"; +import type { PieSectorDataItem } from "recharts/types/polar/Pie";If
PieSectorDataItemis exported from the root package, even better:import type { PieSectorDataItem } from "recharts";apps/web/core/components/onboarding/invitations.tsx (1)
48-48: Clarify/simplify early-return guardThe current condition returns only when both no selection and missing role. If the intent is simply to prevent submission with no selection (button is already disabled for this), simplifying improves readability and avoids role-coupling.
Apply this minimal change:
- if (invitationsRespond.length <= 0 && !invitation?.role) return; + if (invitationsRespond.length === 0) return;apps/web/core/components/modules/analytics-sidebar/issue-progress.tsx (1)
101-117: Ensure loader is reset reliably; use try/catch/finally
setLoader(false)is duplicated in both try and catch. Usefinallyto guarantee cleanup and reduce duplication.const onChange = async (value: TModulePlotType) => { setPlotType(moduleId, value); if (!workspaceSlug || !projectId || !moduleId) return; - try { - setLoader(true); + setLoader(true); + try { if (isArchived) { await fetchArchivedModuleDetails(workspaceSlug, projectId, moduleId); } else { await fetchModuleDetails(workspaceSlug, projectId, moduleId); } - setLoader(false); } catch (error) { - setLoader(false); setPlotType(moduleId, plotType); + } finally { + setLoader(false); } };packages/ui/src/popovers/popover-menu.tsx (1)
3-3: Minor: keep cn import grouped under helpers/utils for consistencycn is a utility helper, not a component. In this file it’s placed under the “components” section, while in modal-core it’s under “constants”. Consider standardizing its placement (e.g., a “helpers” or “utils” section) across the UI package to reduce churn and improve readability. Optional, non-blocking.
apps/web/core/components/dropdowns/member/member-options.tsx (3)
79-96: Skip undefined users and avoid “undefined” in search indexgetUserDetails can return undefined; currently those entries still produce option objects and the query string can contain literal “undefined” fragments. Filter early and build the query from present fields. Also removes the now-redundant final filter.
Apply this diff:
- const options = memberIds - ?.map((userId) => { - const userDetails = getUserDetails(userId); - return { - value: userId, - query: `${userDetails?.display_name} ${userDetails?.first_name} ${userDetails?.last_name}`, - content: ( - <div className="flex items-center gap-2"> - <Avatar name={userDetails?.display_name} src={getFileURL(userDetails?.avatar_url ?? "")} /> - <span className="flex-grow truncate"> - {currentUser?.id === userId ? t("you") : userDetails?.display_name} - </span> - </div> - ), - }; - }) - .filter((o) => !!o); + const options = memberIds + ?.map((userId) => { + const userDetails = getUserDetails(userId); + if (!userDetails) return null; + const { display_name = "", first_name = "", last_name = "", avatar_url } = userDetails; + return { + value: userId, + query: [display_name, first_name, last_name].filter(Boolean).join(" "), + content: ( + <div className="flex items-center gap-2"> + <Avatar name={display_name} src={getFileURL(avatar_url ?? "")} /> + <span className="flex-grow truncate"> + {currentUser?.id === userId ? t("you") : display_name} + </span> + </div> + ), + }; + }) + .filter(Boolean);
19-28: Prefer optional prop syntax over union with undefinedIdiomatic TS for optional props is
placement?: Placementinstead ofPlacement | undefined.- placement: Placement | undefined; + placement?: Placement;
63-71: Include onDropdownOpen in effect deps (or memoize it)Prevents a stale reference and satisfies exhaustive-deps linting. If onDropdownOpen is stable (e.g., from store), this is a no-op.
- }, [isOpen, isMobile]); + }, [isOpen, isMobile, onDropdownOpen]);packages/editor/package.json (3)
43-47: Switch workspace ranges toworkspace:^for smoother semver alignment.
workspace:*works, but it publishes as an exact version pin, which makes dependents churn on every internal release. Usingworkspace:^gives you the same local linking benefits while publishing as a caret range, easing coordinated bumps across the monorepo.Apply this diff:
- "@plane/constants": "workspace:*", - "@plane/hooks": "workspace:*", - "@plane/types": "workspace:*", - "@plane/ui": "workspace:*", - "@plane/utils": "workspace:*", + "@plane/constants": "workspace:^", + "@plane/hooks": "workspace:^", + "@plane/types": "workspace:^", + "@plane/ui": "workspace:^", + "@plane/utils": "workspace:^",
84-86: Useworkspace:^for devConfigs to reduce noisy cross-package pinning.Same rationale as runtime deps:
workspace:^eases coordinated updates of shared configs without forcing exact-version churn.- "@plane/eslint-config": "workspace:*", - "@plane/tailwind-config": "workspace:*", - "@plane/typescript-config": "workspace:*", + "@plane/eslint-config": "workspace:^", + "@plane/tailwind-config": "workspace:^", + "@plane/typescript-config": "workspace:^",
70-71: SSR Safety Confirmed; Optional: Hoist Regex Instance for Performance
- Verified that
isEmojiSupported()is only invoked inside a function (packages/editor/src/core/extensions/emoji/emoji.ts – line 158), not at module load, so SSR/Node won’t break.- Verified that
emojiRegex()is called within the text-matching function (…/emoji.ts – line 405), also inside runtime code, so no top-level execution during import.Optional refactor to avoid reallocating the regex on every match:
// At top of emoji.ts const emojiPattern = emojiRegex(); // Later inside your function: const matches = [...node.text.matchAll(emojiPattern)];packages/propel/src/charts/area-chart/root.tsx (2)
88-88: Coerce lastYValue to number to prevent accidental NaNIf the last point’s y value is a string (e.g., "10"), implicit coercion works, but any non-numeric string yields NaN. Be explicit.
- const lastYValue = lastPoint[yAxis.key] ?? 0; + const lastYValue = Number(lastPoint[yAxis.key] ?? 0);
93-99: Optional: base interpolation on x-values (handles irregular x spacing)Index-based ratios assume evenly spaced x values. If x is time or irregular, compute ratio by x distance for a true straight line from (x0, 0) to (xN, yN).
- const ratio = index / (data.length - 1); - const interpolatedValue = ratio * lastYValue; + // compute ratio by x-distance; supports number or Date; falls back to index if conversion fails + const toNum = (v: unknown) => + v instanceof Date ? v.getTime() : (typeof v === "number" ? v : Number(v)); + const x0Raw = data[0]?.[xAxis.key]; + const xNRaw = lastPoint[xAxis.key]; + const xiRaw = item[xAxis.key]; + const x0 = toNum(x0Raw); + const xN = toNum(xNRaw); + const xi = toNum(xiRaw); + const denom = Number.isFinite(xN - x0) && xN !== x0 ? xN - x0 : (data.length - 1); + const numer = Number.isFinite(xi - x0) ? xi - x0 : index; + const ratio = denom ? numer / denom : 0; + const interpolatedValue = ratio * lastYValue;apps/web/core/components/issues/workspace-draft/draft-issue-properties.tsx (1)
256-268: Nit: Redundant project_id guard (already guaranteed by early return)You return null at Line 119 if
!issue.project_id, so the extraissue.project_id &&check here is redundant. This also allows dropping optional chaining ontoString().- {issue.project_id && areEstimateEnabledByProjectId(issue.project_id?.toString()) && ( + {areEstimateEnabledByProjectId(issue.project_id.toString()) && (apps/web/core/components/labels/delete-label-modal.tsx (1)
35-70: Avoid mixing await with .then/.catch; use try/catch for clarityThis reads cleaner and prevents double-promise handling. Behavior remains the same.
const handleDeletion = async () => { if (!workspaceSlug || !projectId || !data) return; setIsDeleteLoading(true); - await deleteLabel(workspaceSlug.toString(), projectId.toString(), data.id) - .then(() => { - captureSuccess({ - eventName: PROJECT_SETTINGS_TRACKER_EVENTS.label_deleted, - payload: { - name: data.name, - project_id: projectId, - }, - }); - handleClose(); - }) - .catch((err) => { - setIsDeleteLoading(false); - - captureError({ - eventName: PROJECT_SETTINGS_TRACKER_EVENTS.label_deleted, - payload: { - name: data.name, - project_id: projectId, - }, - error: err, - }); - - const error = err?.error || "Label could not be deleted. Please try again."; - setToast({ - type: TOAST_TYPE.ERROR, - title: "Error!", - message: error, - }); - }); + try { + await deleteLabel(workspaceSlug.toString(), projectId.toString(), data.id); + captureSuccess({ + eventName: PROJECT_SETTINGS_TRACKER_EVENTS.label_deleted, + payload: { + name: data.name, + project_id: projectId, + }, + }); + handleClose(); + } catch (err: any) { + setIsDeleteLoading(false); + captureError({ + eventName: PROJECT_SETTINGS_TRACKER_EVENTS.label_deleted, + payload: { + name: data.name, + project_id: projectId, + }, + error: err, + }); + const error = err?.error || "Label could not be deleted. Please try again."; + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: error, + }); + } };apps/web/core/components/dropdowns/estimate.tsx (4)
97-117: Remove dead branch and tighten the options buildThe map callback has an
else undefined;that does nothing; you’re also using optional chaining on an already defaulted array. This refactor improves readability and removes the need for a custom undefined filter.- const options: DropdownOptions = (estimatePointIds ?? []) - ?.map((estimatePoint) => { - const currentEstimatePoint = estimatePointById(estimatePoint); - if (currentEstimatePoint) - return { - value: currentEstimatePoint.id, - query: `${currentEstimatePoint?.value}`, - content: ( - <div className="flex items-center gap-2"> - <Triangle className="h-3 w-3 flex-shrink-0" /> - <span className="flex-grow truncate"> - {currentActiveEstimate?.type === EEstimateSystem.TIME - ? convertMinutesToHoursMinutesString(Number(currentEstimatePoint.value)) - : currentEstimatePoint.value} - </span> - </div> - ), - }; - else undefined; - }) - .filter((estimatePointDropdownOption) => estimatePointDropdownOption != undefined) as DropdownOptions; + const options: DropdownOptions = (estimatePointIds ?? []) + .map((estimatePoint) => { + const currentEstimatePoint = estimatePointById(estimatePoint); + return ( + currentEstimatePoint && { + value: currentEstimatePoint.id, + query: `${currentEstimatePoint.value}`, + content: ( + <div className="flex items-center gap-2"> + <Triangle className="h-3 w-3 flex-shrink-0" /> + <span className="flex-grow truncate"> + {currentActiveEstimate?.type === EEstimateSystem.TIME + ? convertMinutesToHoursMinutesString(Number(currentEstimatePoint.value)) + : currentEstimatePoint.value} + </span> + </div> + ), + } + ); + }) + .filter(Boolean) as DropdownOptions;
260-260: Normalize React key to a stringMinor cleanup to avoid non-string keys and make uniqueness explicit.
- <Combobox.Option key={option.value} value={option.value}> + <Combobox.Option key={String(option.value)} value={option.value}>
224-224: Drop “static” on Options (it’s already conditionally rendered)You’re gating rendering with
isOpen &&, sostaticis redundant.- <Combobox.Options className="fixed z-10" static> + <Combobox.Options className="fixed z-10">
97-131: Optional: memoize options/filtering to reduce re-rendersIf this dropdown is used frequently, consider wrapping options and filteredOptions with useMemo keyed on [estimatePointIds, currentActiveEstimate?.type, query, t] to avoid recomputation.
packages/propel/tsconfig.json (1)
5-6: Ambient types confirmed—no extras needed.Our scan of
packages/propelfound no usage ofimport.meta,process.,Buffer, testing globals (test,vi.,describe,it,jest.), or triple-slash references. That means no additional ambient type packages are required.• You can safely keep the
"types": ["node", "react"]filter as is.
• To fall back on the base config’s default ambient types, simply remove the entire"types"entry.
• If this library is truly browser-only, you may trim out Node types:- "types": ["node", "react"] + "types": ["react"]packages/i18n/tsconfig.json (1)
6-8: Align i18n tsconfig.json with repo conventions
- There are no TS/TSX files under
packages/i18n/src, so the current"jsx"and"types"options have no effect today.- Optional JSX runtime update: if you plan to add React/TSX here, switch to the automatic runtime:
- "jsx": "react", + "jsx": "react-jsx",- Ambient types: by specifying
"types": ["node", "react"], you’re preventing other ambient types from the base config (e.g.vite/client,jest,vitest) from being included.
- Remove the
typesfield to inherit everything from@plane/typescript-config/react-library.json, or- Explicitly add any additional type packages you need.
packages/shared-state/tsconfig.json (1)
8-9: Remove unnecessary Node ambient types from shared-state tsconfigNo references to
process,Buffer,__dirname,fs,pathor other Node-only APIs were found underpackages/shared-state/src. You can safely drop the explicit Node types to avoid pulling in Node globals that might mask browser-only issues.
- packages/shared-state/tsconfig.json
{ … - "types": ["node"] }- If in the future you introduce Node-bound scripts or tests, create a separate
tsconfig.node.json(or extend the base config) that adds"types": ["node"]only where needed.- Optionally, once you adopt the automatic JSX runtime repo-wide, update
"jsx"to"react-jsx"in your base tsconfig for consistency.packages/propel/src/menu/types.ts (1)
24-24: Good: widen handler to variadic args for future-proofingThe change to
menuButtonOnClick?: (..._args: unknown[]) => void;is a sensible loosening. Consider actually forwarding the event from the trigger so downstream consumers can use it:In menu.tsx, pass the event:
- if (menuButtonOnClick) menuButtonOnClick(); + menuButtonOnClick?.(e);packages/propel/src/avatar/avatar.tsx (1)
107-107: Add alt text to improve accessibility for the avatar imageThe image lacks an alt attribute, which is important for screen readers.
- <AvatarPrimitive.Image src={src} width="48" height="48" /> + <AvatarPrimitive.Image src={src} width="48" height="48" alt={name ?? fallbackLetter} />packages/propel/src/menu/menu.tsx (3)
114-123: Avoid preventDefault; forward the click event tomenuButtonOnClickCalling
e.preventDefault()on the trigger can interfere with the headless menu’s internal open logic (many libs bail if default is prevented). Also, now thatmenuButtonOnClickaccepts variadic args, forward the event.- const handleMenuButtonClick = (e: React.MouseEvent<HTMLButtonElement, MouseEvent>) => { - e.stopPropagation(); - e.preventDefault(); + const handleMenuButtonClick = (e: React.MouseEvent<HTMLButtonElement, MouseEvent>) => { + e.stopPropagation(); if (isOpen) { closeDropdown(); } else { openDropdown(); } - if (menuButtonOnClick) menuButtonOnClick(); + menuButtonOnClick?.(e); };
126-126: Sync visual “open” styling with actual menu open state
isOpenis only toggled via the button click handler, and not updated when the menu opens/closes via other interactions (e.g., hover or outside click). WireonOpenChangeto keepisOpenin sync and still call the externalhandleOpenChange.- return ( - <BaseMenu.Root openOnHover={openOnHover} onOpenChange={handleOpenChange}> + const onBaseOpenChange = React.useCallback( + (open: boolean) => { + setIsOpen(open); + handleOpenChange?.(open); + }, + [handleOpenChange] + ); + + return ( + <BaseMenu.Root openOnHover={openOnHover} onOpenChange={onBaseOpenChange}>
22-33: Submenu content class name: confirm removal or support it
TSubMenuPropsstill includescontentClassName, but SubMenu no longer destructures or uses it. If the prop is intentionally removed from the API, drop it from the type. If it’s still desired, wire it into the Popup.Example to keep support:
- const { children, trigger, disabled = false, className = "" } = props; + const { children, trigger, disabled = false, className = "", contentClassName } = props; ... - <BaseMenu.Popup className={className}>{children} </BaseMenu.Popup> + <BaseMenu.Popup className={contentClassName ?? className}>{children}</BaseMenu.Popup>packages/propel/src/table/core.tsx (1)
5-11: Solid forwardRef typing; consider ElementRef for intrinsic elementsThe forwardRef conversions look good and remove unsafe casts. For intrinsic elements (e.g., "table", "thead"), idiomatic typing uses React.ElementRef instead of React.ComponentRef.
Minimal changes:
-const Table = React.forwardRef<React.ComponentRef<"table">, React.ComponentPropsWithoutRef<"table">>( +const Table = React.forwardRef<React.ElementRef<"table">, React.ComponentPropsWithoutRef<"table">>( ... -const TableHeader = React.forwardRef<React.ComponentRef<"thead">, React.ComponentPropsWithoutRef<"thead">>( +const TableHeader = React.forwardRef<React.ElementRef<"thead">, React.ComponentPropsWithoutRef<"thead">>( ... -const TableBody = React.forwardRef<React.ComponentRef<"tbody">, React.ComponentPropsWithoutRef<"tbody">>( +const TableBody = React.forwardRef<React.ElementRef<"tbody">, React.ComponentPropsWithoutRef<"tbody">>( ... -const TableFooter = React.forwardRef<React.ComponentRef<"tfoot">, React.ComponentPropsWithoutRef<"tfoot">>( +const TableFooter = React.forwardRef<React.ElementRef<"tfoot">, React.ComponentPropsWithoutRef<"tfoot">>( ... -const TableRow = React.forwardRef<React.ComponentRef<"tr">, React.ComponentPropsWithoutRef<"tr">>( +const TableRow = React.forwardRef<React.ElementRef<"tr">, React.ComponentPropsWithoutRef<"tr">>( ... -const TableHead = React.forwardRef<React.ComponentRef<"th">, React.ComponentPropsWithoutRef<"th">>( +const TableHead = React.forwardRef<React.ElementRef<"th">, React.ComponentPropsWithoutRef<"th">>( ... -const TableCell = React.forwardRef<React.ComponentRef<"td">, React.ComponentPropsWithoutRef<"td">>( +const TableCell = React.forwardRef<React.ElementRef<"td">, React.ComponentPropsWithoutRef<"td">>( ... -const TableCaption = React.forwardRef<React.ComponentRef<"caption">, React.ComponentPropsWithoutRef<"caption">>( +const TableCaption = React.forwardRef<React.ElementRef<"caption">, React.ComponentPropsWithoutRef<"caption">>(Also applies to: 14-23, 25-29, 30-36, 37-47, 48-60, 62-71, 73-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (107)
.gitignore(3 hunks)apps/space/core/components/editor/rich-text-editor.tsx(1 hunks)apps/space/package.json(3 hunks)apps/web/ce/components/workspace/edition-badge.tsx(1 hunks)apps/web/ce/components/workspace/sidebar/extended-sidebar-item.tsx(1 hunks)apps/web/core/components/analytics/analytics-filter-actions.tsx(1 hunks)apps/web/core/components/analytics/work-items/priority-chart.tsx(1 hunks)apps/web/core/components/command-palette/command-palette.tsx(1 hunks)apps/web/core/components/cycles/analytics-sidebar/issue-progress.tsx(1 hunks)apps/web/core/components/cycles/analytics-sidebar/sidebar-details.tsx(1 hunks)apps/web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx(1 hunks)apps/web/core/components/dropdowns/estimate.tsx(1 hunks)apps/web/core/components/dropdowns/member/member-options.tsx(1 hunks)apps/web/core/components/estimates/delete/modal.tsx(1 hunks)apps/web/core/components/estimates/estimate-disable-switch.tsx(1 hunks)apps/web/core/components/estimates/estimate-list-item.tsx(1 hunks)apps/web/core/components/estimates/root.tsx(1 hunks)apps/web/core/components/gantt-chart/chart/root.tsx(1 hunks)apps/web/core/components/home/home-dashboard-widgets.tsx(1 hunks)apps/web/core/components/home/root.tsx(1 hunks)apps/web/core/components/home/widgets/empty-states/no-projects.tsx(1 hunks)apps/web/core/components/inbox/content/inbox-issue-header.tsx(1 hunks)apps/web/core/components/inbox/inbox-filter/filters/filter-selection.tsx(1 hunks)apps/web/core/components/issues/archive-issue-modal.tsx(1 hunks)apps/web/core/components/issues/filters.tsx(1 hunks)apps/web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx(1 hunks)apps/web/core/components/issues/issue-layouts/filters/applied-filters/roots/project-root.tsx(1 hunks)apps/web/core/components/issues/issue-layouts/gantt/base-gantt-root.tsx(1 hunks)apps/web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx(1 hunks)apps/web/core/components/issues/issue-layouts/list/blocks-list.tsx(1 hunks)apps/web/core/components/issues/issue-modal/context/issue-modal-context.tsx(1 hunks)apps/web/core/components/issues/workspace-draft/draft-issue-properties.tsx(1 hunks)apps/web/core/components/labels/delete-label-modal.tsx(1 hunks)apps/web/core/components/labels/project-setting-label-group.tsx(1 hunks)apps/web/core/components/modules/analytics-sidebar/issue-progress.tsx(1 hunks)apps/web/core/components/modules/analytics-sidebar/root.tsx(1 hunks)apps/web/core/components/onboarding/create-workspace.tsx(1 hunks)apps/web/core/components/onboarding/invitations.tsx(1 hunks)apps/web/core/components/onboarding/root.tsx(1 hunks)apps/web/core/components/onboarding/steps/team/root.tsx(1 hunks)apps/web/core/components/onboarding/steps/workspace/create.tsx(1 hunks)apps/web/core/components/onboarding/steps/workspace/join-invites.tsx(1 hunks)apps/web/core/components/project/filters.tsx(1 hunks)apps/web/core/components/settings/header.tsx(1 hunks)apps/web/core/components/workspace-notifications/root.tsx(1 hunks)apps/web/core/components/workspace-notifications/sidebar/notification-card/item.tsx(1 hunks)apps/web/core/components/workspace-notifications/sidebar/root.tsx(1 hunks)apps/web/core/components/workspace/delete-workspace-form.tsx(1 hunks)apps/web/core/components/workspace/settings/members-list-item.tsx(1 hunks)apps/web/core/components/workspace/settings/workspace-details.tsx(1 hunks)apps/web/core/components/workspace/sidebar/sidebar-item.tsx(1 hunks)apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx(1 hunks)apps/web/core/hooks/use-favorite-item-details.tsx(1 hunks)apps/web/core/hooks/use-project-issue-properties.ts(1 hunks)apps/web/core/hooks/use-workspace-issue-properties.ts(1 hunks)apps/web/core/layouts/auth-layout/project-wrapper.tsx(1 hunks)apps/web/core/layouts/auth-layout/workspace-wrapper.tsx(1 hunks)apps/web/core/lib/intercom-provider.tsx(1 hunks)apps/web/core/lib/posthog-provider.tsx(1 hunks)apps/web/core/lib/wrappers/authentication-wrapper.tsx(1 hunks)apps/web/core/store/timeline/modules-timeline.store.ts(1 hunks)packages/decorators/.prettierignore(1 hunks)packages/editor/package.json(3 hunks)packages/hooks/package.json(1 hunks)packages/i18n/package.json(1 hunks)packages/i18n/tsconfig.json(1 hunks)packages/propel/package.json(2 hunks)packages/propel/src/avatar/avatar.tsx(1 hunks)packages/propel/src/charts/area-chart/root.tsx(1 hunks)packages/propel/src/charts/pie-chart/active-shape.tsx(2 hunks)packages/propel/src/menu/menu.tsx(1 hunks)packages/propel/src/menu/types.ts(2 hunks)packages/propel/src/table/core.tsx(4 hunks)packages/propel/tsconfig.json(1 hunks)packages/services/package.json(1 hunks)packages/shared-state/package.json(1 hunks)packages/shared-state/tsconfig.json(1 hunks)packages/ui/src/auth-form/auth-form.tsx(1 hunks)packages/ui/src/auth-form/auth-input.tsx(1 hunks)packages/ui/src/badge/badge.tsx(1 hunks)packages/ui/src/breadcrumbs/breadcrumbs.tsx(1 hunks)packages/ui/src/breadcrumbs/navigation-dropdown.tsx(1 hunks)packages/ui/src/breadcrumbs/navigation-search-dropdown.tsx(1 hunks)packages/ui/src/button/button.tsx(1 hunks)packages/ui/src/button/toggle-switch.tsx(1 hunks)packages/ui/src/collapsible/collapsible-button.tsx(1 hunks)packages/ui/src/collapsible/collapsible.tsx(1 hunks)packages/ui/src/content-wrapper/content-wrapper.tsx(1 hunks)packages/ui/src/dropdown/multi-select.tsx(1 hunks)packages/ui/src/dropdown/single-select.tsx(1 hunks)packages/ui/src/dropdowns/custom-menu.tsx(1 hunks)packages/ui/src/dropdowns/custom-select.tsx(1 hunks)packages/ui/src/emoji/emoji-icon-picker-new.tsx(1 hunks)packages/ui/src/emoji/emoji-icon-picker.tsx(1 hunks)packages/ui/src/emoji/icons-list.tsx(1 hunks)packages/ui/src/emoji/lucide-icons-list.tsx(1 hunks)packages/ui/src/form-fields/input-color-picker.tsx(1 hunks)packages/ui/src/form-fields/textarea.tsx(1 hunks)packages/ui/src/header/header.tsx(1 hunks)packages/ui/src/icons/cycle/cycle-group-icon.tsx(1 hunks)packages/ui/src/icons/priority-icon.tsx(1 hunks)packages/ui/src/modals/alert-modal.tsx(1 hunks)packages/ui/src/modals/modal-core.tsx(1 hunks)packages/ui/src/popovers/popover-menu.stories.tsx(1 hunks)packages/ui/src/popovers/popover-menu.tsx(1 hunks)packages/ui/src/popovers/popover.tsx(1 hunks)packages/ui/src/popovers/types.ts(1 hunks)
⛔ Files not processed due to max files limit (3)
- packages/ui/src/sortable/draggable.tsx
- packages/ui/src/toast/index.tsx
- packages/utils/package.json
✅ Files skipped from review due to trivial changes (73)
- packages/ui/src/auth-form/auth-input.tsx
- packages/ui/src/emoji/icons-list.tsx
- packages/ui/src/breadcrumbs/breadcrumbs.tsx
- apps/web/core/components/analytics/work-items/priority-chart.tsx
- packages/decorators/.prettierignore
- apps/web/core/components/home/widgets/empty-states/no-projects.tsx
- apps/web/core/lib/wrappers/authentication-wrapper.tsx
- apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx
- apps/web/core/components/issues/issue-layouts/gantt/base-gantt-root.tsx
- apps/web/core/components/home/home-dashboard-widgets.tsx
- packages/ui/src/breadcrumbs/navigation-dropdown.tsx
- apps/web/core/hooks/use-favorite-item-details.tsx
- packages/ui/src/form-fields/input-color-picker.tsx
- packages/ui/src/breadcrumbs/navigation-search-dropdown.tsx
- apps/web/core/hooks/use-workspace-issue-properties.ts
- packages/ui/src/header/header.tsx
- apps/web/core/components/modules/analytics-sidebar/root.tsx
- apps/web/core/components/gantt-chart/chart/root.tsx
- apps/web/core/components/workspace-notifications/sidebar/root.tsx
- packages/ui/src/form-fields/textarea.tsx
- apps/web/core/components/settings/header.tsx
- packages/ui/src/dropdown/multi-select.tsx
- apps/web/core/components/workspace/delete-workspace-form.tsx
- apps/web/core/store/timeline/modules-timeline.store.ts
- packages/ui/src/collapsible/collapsible.tsx
- packages/ui/src/content-wrapper/content-wrapper.tsx
- apps/web/core/components/onboarding/root.tsx
- apps/web/core/lib/intercom-provider.tsx
- apps/web/core/components/cycles/analytics-sidebar/issue-progress.tsx
- apps/web/core/components/onboarding/steps/workspace/create.tsx
- packages/ui/src/dropdowns/custom-menu.tsx
- apps/web/core/components/onboarding/steps/workspace/join-invites.tsx
- packages/ui/src/popovers/popover-menu.stories.tsx
- apps/web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx
- apps/space/core/components/editor/rich-text-editor.tsx
- apps/web/core/components/cycles/analytics-sidebar/sidebar-details.tsx
- apps/web/core/components/workspace-notifications/root.tsx
- apps/web/core/components/issues/issue-layouts/list/blocks-list.tsx
- packages/ui/src/dropdowns/custom-select.tsx
- apps/web/core/components/estimates/estimate-list-item.tsx
- apps/web/core/layouts/auth-layout/project-wrapper.tsx
- apps/web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx
- packages/ui/src/collapsible/collapsible-button.tsx
- apps/web/core/components/onboarding/steps/team/root.tsx
- packages/ui/src/popovers/popover.tsx
- apps/web/core/components/command-palette/command-palette.tsx
- packages/ui/src/dropdown/single-select.tsx
- apps/web/core/components/issues/issue-modal/context/issue-modal-context.tsx
- apps/web/core/components/estimates/delete/modal.tsx
- packages/ui/src/emoji/emoji-icon-picker-new.tsx
- apps/web/core/components/project/filters.tsx
- apps/web/core/components/workspace/settings/workspace-details.tsx
- apps/web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx
- apps/web/core/components/issues/issue-layouts/filters/applied-filters/roots/project-root.tsx
- apps/web/ce/components/workspace/edition-badge.tsx
- apps/web/core/lib/posthog-provider.tsx
- apps/web/core/components/estimates/root.tsx
- apps/web/core/components/onboarding/create-workspace.tsx
- apps/web/core/components/issues/archive-issue-modal.tsx
- apps/web/core/hooks/use-project-issue-properties.ts
- packages/ui/src/button/toggle-switch.tsx
- packages/ui/src/auth-form/auth-form.tsx
- apps/web/core/components/workspace/sidebar/sidebar-item.tsx
- apps/web/core/components/issues/filters.tsx
- apps/web/core/components/workspace/settings/members-list-item.tsx
- packages/ui/src/icons/priority-icon.tsx
- packages/ui/src/badge/badge.tsx
- packages/ui/src/emoji/emoji-icon-picker.tsx
- packages/ui/src/popovers/types.ts
- apps/web/core/components/home/root.tsx
- packages/ui/src/modals/alert-modal.tsx
- apps/web/core/components/estimates/estimate-disable-switch.tsx
- packages/ui/src/icons/cycle/cycle-group-icon.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/hooks/package.json
- packages/shared-state/package.json
- packages/propel/package.json
- packages/services/package.json
- apps/space/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-14T13:16:23.323Z
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
Applied to files:
apps/web/core/components/analytics/analytics-filter-actions.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (28)
apps/web/core/components/inbox/content/inbox-issue-header.tsx (1)
29-30: LGTM: Import reordering only; no runtime impactReordering these hook imports is fine and keeps the hooks grouped logically. No changes to bindings or behavior.
packages/ui/src/button/button.tsx (1)
4-4: LGTM: import reordering is safe and non-functional.This is a straightforward import-order tweak with no runtime impact. Good to keep imports organized consistently.
packages/propel/src/charts/pie-chart/active-shape.tsx (1)
5-5: Nice: explicit props typing improves safety and DX.Typing the props as
PieSectorDataItemremovesanyand aligns with Recharts’ expected shape.apps/web/core/components/onboarding/invitations.tsx (1)
17-17: Import reordering LGTMReordering the hook imports keeps the hooks grouped and preserves behavior. No functional impact.
apps/web/core/components/analytics/analytics-filter-actions.tsx (1)
5-5: Import reordering only; no runtime impact — LGTMGrouping hook imports is consistent and does not affect behavior. No further action needed.
apps/web/core/components/modules/analytics-sidebar/issue-progress.tsx (1)
20-20: Import reordering improves readability — LGTMMoving
useProjectEstimatesinto the hooks group keeps imports organized. No behavior change.apps/web/core/components/inbox/inbox-filter/filters/filter-selection.tsx (1)
6-6: Import reordering aligns with hooks grouping — LGTMNo functional change; keeps store hooks together for readability.
packages/ui/src/modals/modal-core.tsx (1)
2-2: LGTM: import reorg only; no runtime or type changesReordering React and moving cn to the top block doesn’t affect behavior. The paths remain correct and ModalCore usage is unchanged.
Also applies to: 4-4
packages/i18n/package.json (3)
18-18: Switch to workspace: for internal utils looks good*Using workspace:* ensures local resolution under pnpm and prevents accidental registry fetches. Good alignment with the monorepo migration.
24-25: Workspace-scoped tooling dependencies LGTMPointing @plane/eslint-config and @plane/typescript-config to workspace:* is correct for pnpm workspaces and keeps toolchain versions consistent.
18-21: MobX dependencies are actively used in i18n packageThe verification confirms that both
mobxandmobx-reactare legitimately used in the i18n package:• packages/i18n/src/store/index.ts: Uses
makeAutoObservableandrunInActionfrom mobx
• packages/i18n/src/context/index.tsx: Usesobserverfrom mobx-react to wrap the TranslationProvider componentThe i18n package implements a MobX-based translation store with reactive components, making these dependencies necessary for its functionality. The dependencies should remain as regular dependencies rather than being moved to peerDependencies, as this is a utility package that needs to manage its own state store.
apps/web/core/components/workspace-notifications/sidebar/notification-card/item.tsx (1)
10-13: Import re-order reads clean and preserves behaviorHooks are grouped under the "hooks" section and there’s no duplicate import anymore. No runtime changes introduced.
apps/web/core/components/dropdowns/member/member-options.tsx (1)
12-12: Consolidated IUserLite import — LGTMImporting IUserLite alongside other Plane workspace imports keeps things tidy and avoids duplicate imports. No runtime impact; types are correctly scoped.
packages/propel/src/charts/area-chart/root.tsx (1)
101-101: Good catch: add yAxis.key to useMemo depsIncluding yAxis.key ensures recomputation when the metric changes. This prevents stale comparison lines when toggling Y metrics.
packages/ui/src/emoji/lucide-icons-list.tsx (2)
7-7: Import reorder looks good; no functional impact.Moving the
cnimport has no runtime effect and aligns with the broader import tidy-up implied by the PR. Safe change.
90-93: No changes needed:cnsupports object syntax
cnis defined inpackages/ui/src/utils/classname.tsxas:export const cn = (...inputs: ClassValue[]) => twMerge(clsx(inputs));Since
clsxnatively handles object maps for conditional classes and its output is passed totwMerge, the{ "border-custom-border-400": !showHexInput }syntax is fully supported.apps/web/core/components/issues/workspace-draft/draft-issue-properties.tsx (2)
24-24: Import reordering within the hooks block is fineThis keeps hooks grouped and has no runtime impact.
21-21: Import alias resolution validated: theuseProjectEstimateshook is correctly exported and the@/*alias maps tocore/*inapps/web/tsconfig.json. Consistent imports of@/hooks/store/estimatesare already present across the codebase, so no further changes are needed.apps/web/core/components/labels/delete-label-modal.tsx (2)
7-7: Consolidated constants import looks goodBringing both PROJECT_SETTINGS_TRACKER_ELEMENTS and PROJECT_SETTINGS_TRACKER_EVENTS into a single top-level import cleans up duplication and keeps imports consistent.
13-13: Import organization LGTMKeeping useLabel grouped with the other hooks imports improves readability.
apps/web/core/components/labels/project-setting-label-group.tsx (1)
8-12: Import path and ordering cleanup looks correctMoving PROJECT_SETTINGS_TRACKER_ELEMENTS to import from @plane/constants and captureClick to the top import block keeps dependencies organized and removes bottom-file duplication.
apps/web/core/components/dropdowns/estimate.tsx (1)
13-15: Import re-order looks goodPure import shuffle; no behavioral impact. Matches the broader stylistic cleanups in the PR.
packages/propel/tsconfig.json (1)
4-4: Verified React and TypeScript versions – safe to usereact-jsxAll scanned
package.jsonfiles show React ≥ 17 (v18.3.1) and TypeScript ≥ 4.1 (v5.8.3), so the"jsx": "react-jsx"setting is fully supported.apps/web/ce/components/workspace/sidebar/extended-sidebar-item.tsx (1)
19-19: Import re-order is a safe no-op.Moving the user hooks import below
useWorkspaceis stylistic and has no runtime side effects. Looks good.apps/web/core/layouts/auth-layout/workspace-wrapper.tsx (2)
19-19: NewuseFavoriteimport aligns with existing usage.This import matches the
fetchFavoriteusage below and the SWR gate by permissions. All good.
24-24: ✅ pnpm Migration VerifiedVerified that the import reorder in
apps/web/core/layouts/auth-layout/workspace-wrapper.tsxis a no-op and the pnpm migration is complete:• pnpm-workspace.yaml present at repo root
• package.json’spackageManagerset to “pnpm@10.12.1”
• Noyarnornpm install/ci/runinvocations outside pnpm context
• pnpm commands used consistently in workflows, Dockerfiles, and scriptsNo further Yarn/NPM references detected—good to merge.
packages/propel/src/menu/types.ts (1)
46-46: Good: safer click signature for menu itemsUsing
onClick?: (_args?: unknown) => void;avoids over-constraining consumer event payloads and aligns with the internal call sites that pass the native event.packages/propel/src/avatar/avatar.tsx (1)
81-81: Robust number guard and correct NaN checkType-narrowing with
(value: unknown): value is numberandNumber.isNaNis the right approach here. Nice improvement overisNaN.
lifeiscontent
force-pushed
the
setup-pnpm-2
branch
from
2c08aef to
e0aea3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
packages/editor/tsconfig.json (2)
6-7: moduleResolution value casing likely invalid ("bundler" → "Bundler")TypeScript expects the enum-cased value "Bundler". Using "bundler" can cause TS to error with an unknown moduleResolution.
Apply this diff to fix:
- "moduleResolution": "bundler", + "moduleResolution": "Bundler",
3-11: Detected NodeBufferusage in browser‐only package – fix requiredThe editor package still references
Buffer.from(…)in
packages/editor/src/core/helpers/yjs-utils.ts (lines 42 & 49). Without Node ambient types, TS will error here.• File & lines to address
– packages/editor/src/core/helpers/yjs-utils.ts:42
– packages/editor/src/core/helpers/yjs-utils.ts:49Recommended fixes
- Re-introduce Node types in your tsconfig:
// packages/editor/tsconfig.json { "compilerOptions": { + "types": ["node", "vite/client"], … } }
- Or refactor these helpers to browser APIs (e.g. use atob()/btoa() on strings or TextEncoder/TextDecoder for ArrayBuffers) and remove
Buffer.from.- Alternatively, declare a local ambient for Buffer if you have a bundler polyfill:
// packages/editor/src/global.d.ts declare const Buffer: { from(input: string | Uint8Array, encoding?: string): Uint8Array | string; };
🧹 Nitpick comments (6)
apps/live/package.json (1)
49-50: Dotenv import detected—can’t remove until you migrate to--env-fileWe found that apps/live is still importing and using
dotenv, so you must keep it as a dependency unless you refactor startup to rely solely on Node’s--env-fileflag. Also, there’s no@types/pino-httpin this package, so nothing to remove there.• apps/live/src/core/services/api.service.ts –
import { config } from "dotenv";
• apps/live/src/core/helpers/logger.ts –import { pinoHttp } from "pino-http";(no@types/pino-httpentry in package.json)If you do want to drop
dotenv:
- Remove the
configimport and any calls toconfig()in your code.- Update your start scripts (e.g. in
package.json) to includenode --env-file .env ….- (Optional) Add an
"engines": { "node": ">=20.6" }to apps/live/package.json to document the required Node version.packages/utils/package.json (1)
30-30: Consider making UI libs peer deps or move icon helpers out of utils.Adding lucide-react as a runtime dependency in a "utils" package couples it to React UI concerns and can cause duplicate installs in consumers. Options:
- Prefer placing icon/React-specific helpers in @plane/ui.
- Or declare react and lucide-react as peerDependencies here (and keep @types/react in devDependencies for typing).
Apply this minimal change to avoid bundling lucide-react from utils:
"dependencies": { "@plane/constants": "workspace:*", "@plane/types": "workspace:*", "clsx": "^2.1.1", "date-fns": "^4.1.0", "isomorphic-dompurify": "^2.16.0", "lodash": "^4.17.21", - "lucide-react": "^0.469.0", "react": "^18.3.1", "tailwind-merge": "^2.5.5", "uuid": "^10.0.0" }, + "peerDependencies": { + "react": "^18.0.0", + "lucide-react": "^0.469.0" + }, + "peerDependenciesMeta": { + "lucide-react": { + "optional": true + } + },If utils must export React/icon helpers, this approach prevents version conflicts and duplicate bundles in apps.
apps/space/package.json (1)
11-11: Raising max lint warnings masks issues.Prefer fixing or disabling non-actionable rules rather than increasing the warning budget. If this is temporary to unblock the pnpm migration, add a TODO with a target to revert.
packages/editor/tsconfig.json (1)
2-20: Consider extending the shared tsconfig to reduce driftThis tsconfig doesn’t extend the monorepo’s shared configs. To stay aligned and reduce maintenance, consider:
+ "extends": "../typescript-config/react-library.json", "compilerOptions": { - "jsx": "react-jsx", - "lib": ["ES2022", "DOM"], - "module": "ESNext", - "moduleResolution": "Bundler", - "noEmit": true, - "skipLibCheck": true, - "sourceMap": true, - "target": "ESNext", + "jsx": "react-jsx", + "noEmit": true, + "sourceMap": true, "baseUrl": ".", "paths": { "@/*": ["./src/core/*"], "@/styles/*": ["./src/styles/*"], "@/plane-editor/*": ["./src/ce/*"] }, - "strictNullChecks": true, - "allowSyntheticDefaultImports": true + "strictNullChecks": true },Adjust as needed if you intentionally diverge (e.g., different target or libs).
packages/typescript-config/base.json (1)
7-8: Consider enabling incremental builds for faster TS rebuilds"incremental": false disables TS’s build info caching. In a monorepo with Turbo/pnpm, incremental true often reduces CI and local re-check times significantly. If you don’t rely on tsc -b artifacts, feel free to skip.
- "incremental": false, + "incremental": true,packages/typescript-config/nextjs.json (1)
8-11: allowJs: true may widen the typecheck surface in Next appsallowJs includes .js files in typechecking. If the app has many JS files, this can slow checks. Keep if intentional (mixed TS/JS), otherwise consider disabling or scoping via "include".
- "allowJs": true, + // "allowJs": true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.npmrc(1 hunks)apps/admin/package.json(2 hunks)apps/live/package.json(2 hunks)apps/live/tsconfig.json(1 hunks)apps/space/package.json(3 hunks)apps/space/tsconfig.json(1 hunks)apps/web/package.json(3 hunks)packages/constants/package.json(1 hunks)packages/decorators/package.json(1 hunks)packages/decorators/tsconfig.json(1 hunks)packages/editor/tsconfig.json(1 hunks)packages/logger/package.json(1 hunks)packages/propel/src/charts/pie-chart/active-shape.tsx(2 hunks)packages/propel/tsconfig.json(1 hunks)packages/typescript-config/base.json(1 hunks)packages/typescript-config/nextjs.json(1 hunks)packages/typescript-config/node-library.json(1 hunks)packages/typescript-config/react-library.json(1 hunks)packages/utils/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/decorators/tsconfig.json
- apps/web/package.json
- apps/space/tsconfig.json
- packages/decorators/package.json
- packages/logger/package.json
- packages/propel/src/charts/pie-chart/active-shape.tsx
- apps/live/tsconfig.json
- packages/constants/package.json
- packages/propel/tsconfig.json
- .npmrc
🔇 Additional comments (10)
apps/admin/package.json (2)
21-27: Workspace refs look good.Switching internal @plane deps to "workspace:*" aligns with pnpm workspaces and will prevent accidental version drift. No concerns.
46-49: Dev deps migration to workspace: and moving @types/lodash to dev is correct.*This cleans up install surfaces and keeps types out of runtime. Looks good.
apps/live/package.json (1)
27-29: Workspace refs migration is consistent.@plane/editor and @plane/types to "workspace:*" is aligned with the pnpm move. No action needed.
packages/utils/package.json (2)
24-25: Workspace refs LGTM.Using "workspace:*" for @plane/constants and @plane/types is correct for pnpm.
36-37: Dev tooling to workspace: and adding @types/lodash is fine.*No issues here. If you adopt peerDependencies for React above, keeping @types/react here is still appropriate for local TS builds.
Also applies to: 41-41
apps/space/package.json (4)
24-31: Workspace refs migration is consistent and comprehensive.All internal packages moved to "workspace:*" including the newly added @plane/utils. Looks good.
50-50: @popperjs/core version aligns with react-popper v2.react-popper@^2 expects @popperjs/core@^2, so this addition is correct.
52-52: sharp requires native binaries—double-check build images for this app too.Same caution as admin: ensure Docker/CI environments are compatible with sharp’s prebuilt binaries or install required libs (avoid Alpine or install libc-compat/libvips).
Run the same sharp/Docker verification script from the admin comment, replacing the path filter with apps/space if you maintain per-app Dockerfiles.
58-61: Dev deps to workspace: and updated @types/lodash are fine.*No concerns.
packages/typescript-config/nextjs.json (1)
6-7: LGTM on ESNext/Bundler for Next.js"module": "ESNext" and "moduleResolution": "Bundler" align with TS ≥ 5.3 and Next 14+ recommendations. Verified all workspace package.json files list TypeScript 5.6.3 or 5.8.3—no workspaces below 5.3, so no further action is needed.
…e and catalog; pnpm install to update lockfile
lifeiscontent
force-pushed
the
setup-pnpm-2
branch
from
460028e to
688a74f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
package.json (1)
27-41: TypeScript version alignment looks good nowThe root pnpm override pins typescript to 5.8.3, which aligns with packages (e.g., packages/propel). This resolves the earlier mismatch noted in prior reviews.
🧹 Nitpick comments (3)
package.json (2)
43-43: Add engines to enforce Node/pnpm versions across contributors and CITo avoid environment drift (especially with sharp/esbuild/turbo requirements), add an engines block. Also consider adding "engineStrict": true if you want installs to fail on mismatches.
Proposed addition near the top-level of package.json:
{ "name": "plane", "description": "Open-source project management that unlocks customer value", "repository": "https://github.com/makeplane/plane.git", "version": "0.28.0", "license": "AGPL-3.0", "private": true, + "engines": { + "node": ">=18.18.0", + "pnpm": ">=10.12.1" + }, "workspaces": [ "apps/*", "packages/*" ],
23-25: Pin Prettier to a fixed v3 version for deterministic installsPinning Prettier avoids non-reproducible CI/local installs. Align the root version with the one used in apps/web (v3.2.5) while preserving plugin compatibility:
• In package.json (root)
- "prettier": "latest", + "prettier": "^3.2.5", "prettier-plugin-tailwindcss": "^0.6.14", "turbo": "^2.5.6"• Compatibility verified:
– root prettier-plugin-tailwindcss ^0.6.14 → requires Prettier >=3 (✅)
– packages/tailwind-config prettier-plugin-tailwindcss ^0.3.0 → uses Prettier ^2.8.8 (unchanged)packages/propel/package.json (1)
25-29: Good move to workspace protocol for internal depsSwitching to "workspace:*" ensures local linking and consistent versions across the monorepo.
If you want semver protection on intra-repo bumps, consider using "workspace:^" instead of "*" so consumers don’t inadvertently accept breaking changes before coordinated releases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/space/package.json(3 hunks)apps/web/package.json(3 hunks)package.json(1 hunks)packages/editor/package.json(3 hunks)packages/propel/package.json(2 hunks)packages/ui/package.json(2 hunks)packages/utils/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/utils/package.json
- packages/editor/package.json
- apps/web/package.json
- packages/ui/package.json
- pnpm-workspace.yaml
- apps/space/package.json
🔇 Additional comments (2)
package.json (1)
27-41: esbuild 0.25.0 override is safe
Onlytsup@8.4.0(the sole esbuild consumer in this monorepo) declares"esbuild": "^0.25.0", so the override to0.25.0satisfies its dependency. No other build tools—Vite, Rollup, unplugin-*, next-transpile-modules, etc.—are present.• Root package.json (lines 27–41):
"esbuild": "0.25.0"
• All workspaces and apps usetsup@8.4.0, which depends onesbuild:^0.25.0packages/propel/package.json (1)
39-41: Dev toolchains now aligned to workspace packagesAll referenced
@plane/*workspace dependencies have been verified and are resolvable. Using"workspace:*"for internal config packages (eslint, tailwind, tsconfig) keeps the toolchain consistent across packages. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/space/Dockerfile.dev (1)
19-19: Fix pnpm filter flag position (likely running the wrong package).
--filter=spaceappears after the command, which pnpm treats as an argument to the script rather than a pnpm flag. This can execute the root script or the wrong package. Place the filter before the command or target the workspace directory with-C.Apply one of these:
Option A (directory-scoped, most robust):
-CMD ["pnpm", "dev", "--filter=space"] +CMD ["pnpm", "-C", "apps/space", "dev"]Option B (keep filter, ensure correct flag position; adjust pattern to actual package if needed):
-CMD ["pnpm", "dev", "--filter=space"] +CMD ["pnpm", "--filter=./apps/space", "dev"]
🧹 Nitpick comments (6)
packages/constants/src/workspace.ts (3)
276-281: Avoid non-null assertions on indexed access; tighten the map types insteadUsing ! masks missing-key bugs at runtime. These keys exist, so prefer stronger typing over assertions.
Apply this diff to remove the non-null assertions:
export const WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS_LINKS: IWorkspaceSidebarNavigationItem[] = [ - WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS["views"]!, - WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS["analytics"]!, - WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS["drafts"]!, - WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS["archives"]!, + WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS["views"], + WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS["analytics"], + WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS["drafts"], + WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS["archives"], ];If
noUncheckedIndexedAccessis enabled, removing!may surface type errors due toRecord<string, ...>. In that case, also refine the record’s type to exact keys so the index is non-optional:type DynamicNavKey = "views" | "analytics" | "drafts" | "archives"; // Option A (explicit Record) export const WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS: Record<DynamicNavKey, IWorkspaceSidebarNavigationItem> = { /* existing object */ }; // Option B (preferred if TS >= 4.9) export const WORKSPACE_SIDEBAR_DYNAMIC_NAVIGATION_ITEMS = { /* existing object */ } satisfies Record<DynamicNavKey, IWorkspaceSidebarNavigationItem>;
311-314: Same: drop non-null assertions and refine the static map typeStay consistent and keep type-safety without
!.Apply this diff:
export const WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS_LINKS: IWorkspaceSidebarNavigationItem[] = [ - WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS["home"]!, - WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS["inbox"]!, - WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS["your-work"]!, + WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS["home"], + WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS["inbox"], + WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS["your-work"], ];If needed for
noUncheckedIndexedAccess, narrow the map keys:type StaticNavKey = "home" | "inbox" | "your-work" | "projects"; export const WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS: Record<StaticNavKey, IWorkspaceSidebarNavigationItem> = { /* existing object */ }; // or with `satisfies` (TS >= 4.9) export const WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS = { /* existing object */ } satisfies Record<StaticNavKey, IWorkspaceSidebarNavigationItem>;
316-318: Also remove the non-null assertion hereConsistent with the above, avoid
!for pinned items.Apply this diff:
export const WORKSPACE_SIDEBAR_STATIC_PINNED_NAVIGATION_ITEMS_LINKS: IWorkspaceSidebarNavigationItem[] = [ - WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS["projects"]!, + WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS["projects"], ];apps/space/Dockerfile.dev (1)
10-11: Pin installs and improve dev-layer caching (optional).
- Use a locked install to avoid drift when the lockfile is present.
- Consider cache-friendly layering to speed up rebuilds (copy only lockfiles and manifests before
pnpm install). If you’ve addedpackageManagerin package.json (pnpm@x),corepack enablewill honor it.Minimal hardening:
-RUN pnpm install +RUN pnpm install --frozen-lockfileOptional cache-friendly restructuring (illustrative; adjust paths as needed):
- COPY . . - RUN corepack enable pnpm && pnpm add -g turbo - RUN pnpm install + COPY package.json pnpm-lock.yaml pnpm-workspace.yaml ./ + COPY apps/space/package.json apps/space/ + RUN corepack enable pnpm && pnpm install --frozen-lockfile + COPY . .If you don’t actually need Turbo globally here, you can avoid the global install and use dlx:
-RUN corepack enable pnpm && pnpm add -g turbo +RUN corepack enable pnpm +# when needed: pnpm dlx turbo <cmd>apps/space/helpers/file.helper.ts (1)
45-49: Current fallback is effectively a no-op; consider robust asset-id parsing (trim slash, strip query/hash, decode).
src.split("/")always yields at least one element, soassetUrlis neverundefined—the?? ""won’t trigger. For common cases like trailing slashes or signed URLs with query params, this can return an empty string or include the query string. Suggest normalizing: strip query/hash, trim trailing slashes, take the last path segment, and decode it.Apply this focused refactor:
export const getAssetIdFromUrl = (src: string): string => { - const sourcePaths = src.split("/"); - const assetUrl = sourcePaths[sourcePaths.length - 1]; - return assetUrl ?? ""; + if (!src) return ""; + // Strip query/hash and trailing slashes, then take last path segment + const [pathWithoutQuery] = src.split(/[?#]/); + const trimmed = pathWithoutQuery.replace(/\/+$/, ""); + const lastSegment = trimmed.split("/").pop() ?? ""; + try { + return decodeURIComponent(lastSegment); + } catch { + return lastSegment; // if not a valid URI component + } }Verification suggestion:
- Ensure all call sites treating a falsy id are resilient to empty-string returns (or consider returning
undefinedand updating types if that models “no id” better).Optional follow-up:
- This helper exists in multiple packages. Consider centralizing it in a shared util (e.g., a common package) to avoid drift. I can help consolidate if you want.
packages/services/src/file/helper.ts (1)
32-36: Consolidate and harden getAssetIdFromUrl across the repoYou’ve got three separate implementations of
getAssetIdFromUrl(in packages/utils, packages/services, and apps/space), each with the same simplistic logic and a never-used?? ""fallback. Let’s centralize to a single, robust helper and remove the duplicates.Suggested steps:
- Extract the improved version into
packages/utils/src/file.ts(or a new shared module).- Update all imports in:
•packages/services/src/file/helper.ts
•apps/space/helpers/file.helper.ts
• any other local copies
to import from@plane/utilsinstead of redefining.- Enhance the implementation to:
• guard against empty/invalid inputs
• strip query (?) or hash (#) parts
• trim trailing slashes
• safely decode URI segments
• fall back to""if nothing’s leftExample shared helper in packages/utils/src/file.ts:
export function getAssetIdFromUrl(src: string): string { if (!src) return ""; const [path] = src.split(/[?#]/); const trimmed = path.replace(/\/+$/, ""); const segment = trimmed.split("/").pop() ?? ""; try { return decodeURIComponent(segment); } catch { return segment; } }Then in your services and app code:
- import { getAssetIdFromUrl } from "./helper"; + import { getAssetIdFromUrl } from "@plane/utils";This DRYs up our code and ensures every consumer handles edge cases consistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/space/Dockerfile.dev(1 hunks)apps/space/core/components/editor/rich-text-editor.tsx(1 hunks)apps/space/helpers/file.helper.ts(1 hunks)packages/constants/src/workspace.ts(2 hunks)packages/services/src/file/helper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/space/core/components/editor/rich-text-editor.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (2)
apps/space/Dockerfile.dev (2)
17-17: Good catch on the node_modules volume path.The workspace-specific node_modules path is now correct:
/app/apps/space/node_modules.
13-13: Verify Docker Compose port mapping for space serviceI’ve confirmed that:
apps/space/package.jsondev script usesnext dev -p 3002apps/space/Dockerfile.devexposes port 3002However, neither of your
docker-compose.ymlfiles (indeployments/cli/communityor the repo root) defines aports:mapping for thespaceservice. Please confirm one of the following:
- If you need to publish the container port, add under
space:services: space: ports: - "3002:3002"- If you rely on an external proxy or network routing, verify it’s configured to route traffic to container port 3002.
sriramveeraghanta
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/typescript-config/base.json (2)
9-9: Do not leak DOM types from base config; keep base neutralIncluding "DOM" and "DOM.Iterable" in base pollutes Node libraries with browser globals (e.g., window, fetch), causing type conflicts. Push DOM libs down into browser-facing presets (react/next) instead and keep base with just ES libs. Also align casing to "ES2022".
Apply this diff:
- "lib": ["es2022", "DOM", "DOM.Iterable"], + "lib": ["ES2022"],If not already, set these in packages/typescript-config/react-library.json and nextjs.json:
- react-library.json: compilerOptions.lib ["ES2022", "DOM", "DOM.Iterable"]
- nextjs.json: compilerOptions.lib ["ES2022", "DOM", "DOM.Iterable"]
10-12: Avoid Node-centric module defaults in base; move to per-preset configsSetting "module": "NodeNext" and "moduleResolution": "NodeNext" (and forcing module detection) in base makes browser packages inherit Node behavior unless they override. Keep base minimal and move these to node-only presets; use ESNext/Bundler in browser presets.
Apply this diff to keep base neutral:
- "module": "NodeNext", - "moduleDetection": "force", - "moduleResolution": "NodeNext",Then ensure:
- packages/typescript-config/node-library.json sets:
"module": "NodeNext", "moduleResolution": "NodeNext", "moduleDetection": "force" (if you want strict ESM)- packages/typescript-config/react-library.json and nextjs.json set:
"module": "ESNext", "moduleResolution": "Bundler"To verify consumers are correctly configured, run:
#!/bin/bash # List tsconfig.json files extending base and check for browser-safe overrides echo "Scanning package tsconfig.json consumers..." fd -a tsconfig.json packages | grep -v 'packages/typescript-config/' | while read -r f; do ext=$(jq -r '.extends // ""' "$f" 2>/dev/null) if [[ "$ext" == *"typescript-config/base.json"* ]]; then mod=$(jq -r '.compilerOptions.module // ""' "$f" 2>/dev/null) res=$(jq -r '.compilerOptions.moduleResolution // ""' "$f" 2>/dev/null) libs=$(jq -r '.compilerOptions.lib // [] | join(",")' "$f" 2>/dev/null) echo "• $f extends base.json | module=$mod | moduleResolution=$res | lib=[$libs]" fi done echo echo "React/Next presets should show ESNext/Bundler and DOM libs; Node presets should show NodeNext/NodeNext and no DOM libs."
🧹 Nitpick comments (4)
apps/live/src/core/helpers/logger.ts (1)
39-39: LGTM for pnpm migration; optional: drop redundant type annotationUsing
typeof logger.loggeravoids importing thepinotype directly, which is a good move under pnpm’s stricter transitive dependency rules. As a minor cleanup, you can rely on type inference and omit the explicit annotation.Apply this diff if you prefer the leaner version:
-export const manualLogger: typeof logger.logger = logger.logger; +export const manualLogger = logger.logger;packages/typescript-config/base.json (1)
7-7: Don’t globally disable incremental builds in baseSetting "incremental": false at the base can slow down library rebuilds across the monorepo. Prefer leaving it unset here and enabling it in library-centric presets (with composite: true) to leverage TS build info caching.
Apply this diff:
- "incremental": false,And in packages/typescript-config/node-library.json (and similar for react-library.json if they build with tsc):
{ "compilerOptions": { "composite": true, "incremental": true } }apps/live/package.json (2)
49-50: Optional: Align dev tooling toworkspace:^for consistencyUsing
workspace:*for internal configs is fine, but adoptingworkspace:^here too keeps version compatibility checks consistent across the repo.Possible diff:
- "@plane/eslint-config": "workspace:*", - "@plane/typescript-config": "workspace:*", + "@plane/eslint-config": "workspace:^", + "@plane/typescript-config": "workspace:^",
27-28: Preferworkspace:^overworkspace:*for runtime dependenciesUsing
workspace:^ensures that your runtime packages follow semver ranges and will break if incompatible versions are introduced, whereasworkspace:*always links regardless of version mismatches.File: apps/live/package.json
Lines: 27–28- "@plane/editor": "workspace:*", - "@plane/types": "workspace:*", + "@plane/editor": "workspace:^", + "@plane/types": "workspace:^",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/live/package.json(2 hunks)apps/live/src/core/helpers/logger.ts(1 hunks)package.json(1 hunks)packages/i18n/package.json(1 hunks)packages/typescript-config/base.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/i18n/package.json
- package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/typescript-config/base.json (1)
13-17: LGTM on strictness and common interop flagsresolveJsonModule, skipLibCheck, strict, and target look reasonable for a shared baseline. No changes requested here.
3 participants
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores