Skip to content

Switch workspace to pnpm catalog specifiers - #3943

Merged
amitsingh-007 merged 1 commit into
nextfrom
f-pnpm-catalogs
Mar 17, 2026
Merged

amitsingh-007 merged 1 commit into
nextfrom
f-pnpm-catalogs

Conversation

@amitsingh-007

@amitsingh-007 amitsingh-007 commented Mar 17, 2026 •

Copy link
Copy Markdown
Owner

Replace explicit external package versions with pnpm 'catalog:' specifiers across package.json files to centralize dependency resolution. Add a catalog mapping to pnpm-workspace.yaml and update pnpm-lock.yaml accordingly. Also add AGENTS.md (symlink to CLAUDE.md). Files touched: apps/extension, apps/web, root package.json, packages/shared, packages/trpc, packages/ui, pnpm-workspace.yaml, pnpm-lock.yaml, and AGENTS.md.

Check if the Pull Request fulfils these requirements

  • Does the extension require a version change?

Greptile Summary

This PR centralizes all external dependency version management by migrating explicit version strings to pnpm catalog: specifiers across all 7 workspace packages, introducing a 63-entry catalog in pnpm-workspace.yaml. The migration is thorough and correct — every catalog: reference in every package.json has a corresponding entry in the workspace catalog, and the catalogMode: strict setting will enforce this going forward.

Key findings:

  • Catalog migration: The migration is complete and consistent. All external packages across apps/extension, apps/web, packages/shared, packages/trpc, packages/ui, packages/configs, and the root package.json now use catalog: specifiers, with every entry properly defined in pnpm-workspace.yaml.
  • onlyBuiltDependencies removal: The onlyBuiltDependencies allowlist (@firebase/util, @vercel/speed-insights, esbuild, protobufjs, sharp, spawn-sync, unrs-resolver) was silently removed from pnpm-workspace.yaml as part of this change. This setting controlled which packages were allowed to execute lifecycle scripts during install; its removal is unrelated to the catalog migration goal and may affect security posture or native package builds.
  • AGENTS.md: Noted as a symlink to CLAUDE.md per the PR description.

Confidence Score: 3/5

  • The catalog migration itself is safe to merge, but the silent removal of onlyBuiltDependencies warrants clarification before merging.
  • The catalog specifier migration is mechanically correct — all 63 packages in the catalog are referenced, and all references exist in the catalog. The drop in confidence comes from the unrelated removal of onlyBuiltDependencies in pnpm-workspace.yaml, which previously restricted lifecycle script execution to 7 known packages. This is a potential security regression or could break native package builds (e.g., sharp, esbuild) depending on pnpm 10's default behavior when the field is absent.
  • pnpm-workspace.yaml — the removal of onlyBuiltDependencies needs to be verified as intentional.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: pnpm-workspace.yaml
Line: 1-5

Comment:
**`onlyBuiltDependencies` removed without explanation**

The `onlyBuiltDependencies` allowlist was removed as part of this PR, even though the stated goal was only to add catalog specifiers. This seems unrelated to the catalog migration.

Previously, only these packages were permitted to run lifecycle scripts (postinstall, etc.):
- `@firebase/util`
- `@vercel/speed-insights`
- `esbuild`
- `protobufjs`
- `sharp`
- `spawn-sync`
- `unrs-resolver`

In pnpm, when `onlyBuiltDependencies` is set, only the listed packages can execute build scripts during `pnpm install`. Removing this setting means all packages in the dependency tree are now allowed to run lifecycle scripts, which broadens the attack surface (any compromised transitive dependency could execute arbitrary code at install time).

Additionally, some of these packages (`sharp`, `esbuild`, `unrs-resolver`) rely on native binaries that are compiled during `postinstall`. If pnpm 10's default behavior changed to block all build scripts when `onlyBuiltDependencies` is absent, these packages may fail to build their native bindings correctly.

Was this removal intentional? If so, it's worth documenting the reason (e.g., pnpm 10 handles this differently now, or these packages no longer need script execution).

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: c912ca7

Greptile also left 1 inline comment on this PR.

Replace explicit external package versions with pnpm 'catalog:' specifiers across package.json files to centralize dependency resolution. Add a catalog mapping to pnpm-workspace.yaml and update pnpm-lock.yaml accordingly. Also add AGENTS.md (symlink to CLAUDE.md). Files touched: apps/extension, apps/web, root package.json, packages/shared, packages/trpc, packages/ui, pnpm-workspace.yaml, pnpm-lock.yaml, and AGENTS.md.
@amitsingh-007 amitsingh-007 linked an issue Mar 17, 2026 that may be closed by this pull request

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively transitions the workspace to use pnpm's catalog: specifiers, centralizing dependency versions. This is a great improvement for dependency management and consistency across the monorepo. The changes in package.json files and the addition of the catalog to pnpm-workspace.yaml are well-executed. I have one point of feedback regarding the removal of the onlyBuiltDependencies configuration in pnpm-workspace.yaml, which could have unintended side effects on the build process for packages with native addons.

I am having trouble creating individual review comments. Click here to see my feedback.

pnpm-workspace.yaml (4-11)

medium

The onlyBuiltDependencies configuration has been removed without explanation. This setting was likely forcing specific packages with native addons, like sharp and esbuild, to be built from source. Removing this configuration will cause pnpm to fall back to its default behavior, which is typically to use pre-built binaries. While this can speed up installation, it may lead to runtime errors if the pre-built binaries are not compatible with the target environment, which might have been the original reason for this configuration. Please either restore this configuration or provide a justification for its removal in the pull request description.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment thread pnpm-workspace.yaml
Comment thread pnpm-workspace.yaml
@amitsingh-007
amitsingh-007 merged commit 2c9232b into next Mar 17, 2026
5 checks passed
@amitsingh-007
amitsingh-007 deleted the f-pnpm-catalogs branch March 17, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use pnpm catalogs

1 participant