Skip to content

Conversation

@miguelg719
Copy link
Collaborator

@miguelg719 miguelg719 commented Dec 3, 2025

why

These dev dependencies don't belong here. Some are no longer used, some should go into their respective packages

what changed

Moved dev dependencies to respective packages and removed unused ones

test plan


Summary by cubic

Moved dev dependencies from the workspace root into the packages that use them and removed unused ones to cut install bloat.

  • Dependencies
    • Removed unused devDependencies from the root; moved required ones into packages/core and packages/evals.
    • Added missing dev deps to packages/core (@types/adm-zip, @types/node, @types/ws, adm-zip, chalk, esbuild) and packages/evals (braintrust, chalk, string-comparison).
    • Cleaned pnpm-lock.yaml (large reduction in entries).

Written for commit e3b0277. Summary will update automatically on new commits.

@changeset-bot
Copy link

changeset-bot bot commented Dec 3, 2025

⚠️ No Changeset found

Latest commit: e3b0277

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@miguelg719 miguelg719 marked this pull request as ready for review December 3, 2025 05:40
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 3, 2025

Greptile Overview

Greptile Summary

Cleaned up root workspace devDependencies by moving 7 dependencies to their appropriate packages (packages/core and packages/evals) and removing 19 unused dependencies.

  • Moved @types/adm-zip, @types/node, adm-zip, chalk, and esbuild to packages/core
  • Moved braintrust and string-comparison to packages/evals
  • Removed unused dependencies including @langchain/community, @langchain/langgraph, autoevals, cheerio, express, multer, chromium-bidi, and various type definitions

Confidence Score: 5/5

  • This PR is safe to merge - it only reorganizes dev dependencies without changing runtime behavior
  • The changes are straightforward dependency housekeeping that properly moves dev dependencies to their appropriate packages and removes unused ones. The lock file confirms all dependencies are correctly resolved.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
package.json 5/5 Removed 26 dev dependencies from root workspace that were either unused or belonged in specific packages
packages/core/package.json 5/5 Added 5 dev dependencies moved from root: @types/adm-zip, @types/node, adm-zip, chalk, and esbuild
packages/evals/package.json 5/5 Added 2 dev dependencies moved from root: braintrust and string-comparison
pnpm-lock.yaml 5/5 Lock file regenerated to reflect dependency reorganization across packages

Sequence Diagram

sequenceDiagram
    participant Root as Root Workspace
    participant Core as packages/core
    participant Evals as packages/evals

    Note over Root: Before: 26 devDependencies
    
    Root->>Core: Move @types/adm-zip, @types/node, adm-zip, chalk, esbuild
    Root->>Evals: Move braintrust, string-comparison
    
    Note over Root: Remove 19 unused deps<br/>(langchain, cheerio, express, etc.)
    
    Note over Root: After: 9 devDependencies
    Note over Core: +5 devDependencies
    Note over Evals: +2 devDependencies
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 4 files

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

3 issues found across 4 files

Prompt for AI agents (all 3 issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/evals/package.json">

<violation number="1" location="packages/evals/package.json:24">
P1: `braintrust` is required at runtime (imported by the eval runner and AISDK wrapper) but was added as a devDependency, so consumers will miss it and the CLI will fail to start.</violation>

<violation number="2" location="packages/evals/package.json:25">
P1: `chalk` is used by the runtime CLI but was added under devDependencies, so running the CLI outside the workspace will fail due to the missing module.</violation>

<violation number="3" location="packages/evals/package.json:26">
P1: `string-comparison` powers runtime scoring utilities but is declared as a devDependency, so downstream installs will miss it and runtime imports will throw.</violation>
</file>

Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR

"devDependencies": {
"braintrust": "^0.4.7",
"chalk": "^5.4.1",
"string-comparison": "^1.3.0",
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Dec 3, 2025

Choose a reason for hiding this comment

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

P1: string-comparison powers runtime scoring utilities but is declared as a devDependency, so downstream installs will miss it and runtime imports will throw.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/evals/package.json, line 26:

<comment>`string-comparison` powers runtime scoring utilities but is declared as a devDependency, so downstream installs will miss it and runtime imports will throw.</comment>

<file context>
@@ -21,6 +21,9 @@
   &quot;devDependencies&quot;: {
+    &quot;braintrust&quot;: &quot;^0.4.7&quot;,
+    &quot;chalk&quot;: &quot;^5.4.1&quot;,
+    &quot;string-comparison&quot;: &quot;^1.3.0&quot;,
     &quot;tsx&quot;: &quot;^4.10.5&quot;
   }
</file context>
Fix with Cubic

},
"devDependencies": {
"braintrust": "^0.4.7",
"chalk": "^5.4.1",
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Dec 3, 2025

Choose a reason for hiding this comment

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

P1: chalk is used by the runtime CLI but was added under devDependencies, so running the CLI outside the workspace will fail due to the missing module.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/evals/package.json, line 25:

<comment>`chalk` is used by the runtime CLI but was added under devDependencies, so running the CLI outside the workspace will fail due to the missing module.</comment>

<file context>
@@ -21,6 +21,9 @@
   },
   &quot;devDependencies&quot;: {
+    &quot;braintrust&quot;: &quot;^0.4.7&quot;,
+    &quot;chalk&quot;: &quot;^5.4.1&quot;,
+    &quot;string-comparison&quot;: &quot;^1.3.0&quot;,
     &quot;tsx&quot;: &quot;^4.10.5&quot;
</file context>
Fix with Cubic

"zod": "^4.1.8"
},
"devDependencies": {
"braintrust": "^0.4.7",
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Dec 3, 2025

Choose a reason for hiding this comment

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

P1: braintrust is required at runtime (imported by the eval runner and AISDK wrapper) but was added as a devDependency, so consumers will miss it and the CLI will fail to start.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/evals/package.json, line 24:

<comment>`braintrust` is required at runtime (imported by the eval runner and AISDK wrapper) but was added as a devDependency, so consumers will miss it and the CLI will fail to start.</comment>

<file context>
@@ -21,6 +21,9 @@
     &quot;zod&quot;: &quot;^4.1.8&quot;
   },
   &quot;devDependencies&quot;: {
+    &quot;braintrust&quot;: &quot;^0.4.7&quot;,
+    &quot;chalk&quot;: &quot;^5.4.1&quot;,
+    &quot;string-comparison&quot;: &quot;^1.3.0&quot;,
</file context>
Fix with Cubic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this package is not published so install will still hit dev

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the feedback! I've saved this as a new learning to improve future reviews.

@miguelg719 miguelg719 changed the base branch from main to miguelgonzalez/stg-992-update-suggested-models-in-docs December 4, 2025 20:21
@miguelg719 miguelg719 changed the base branch from miguelgonzalez/stg-992-update-suggested-models-in-docs to main December 4, 2025 20:21
@miguelg719 miguelg719 force-pushed the miguelgonzalez/stg-1031-clean-workspace-devdependencies branch from 78b9fc0 to e3b0277 Compare December 4, 2025 20:28
@browserbase browserbase deleted a comment from claude bot Dec 4, 2025
@claude
Copy link

claude bot commented Dec 4, 2025

PR Review: Clean workspace dev dependencies

Summary

This PR successfully moves development dependencies from the workspace root to their respective packages (packages/core and packages/evals) and removes unused dependencies. This is a positive change that improves the monorepo structure and reduces installation bloat.

✅ Code Quality & Best Practices

Excellent work on:

  • Proper monorepo organization: Dependencies are now correctly scoped to the packages that use them
  • Clean separation of concerns: Dev dependencies are in the packages where they are actually needed
  • Significant lockfile reduction: 2,096 deletions in pnpm-lock.yaml indicates substantial cleanup

✅ Verification of Changes

I verified that all moved dependencies are actually used in their target packages:

packages/core/package.json - Added dependencies are used:

  • @types/adm-zip: Used in lib/v3/tests/downloads.spec.ts:4
  • @types/node: Required for TypeScript type checking
  • @types/ws: Required for WebSocket types
  • adm-zip: Used in lib/v3/tests/downloads.spec.ts:4
  • chalk: Used in 7+ example files (examples/agent_stream_example.ts:3, examples/cua-example.ts:10, etc.)
  • esbuild: Used in build scripts (lib/v3/dom/genDomScripts.ts:7, lib/v3/dom/genLocatorScripts.ts:4)

packages/evals/package.json - Added dependencies are used:

  • braintrust: Used in index.eval.ts:25, types/evals.ts:5, lib/AISdkClientWrapped.ts:13
  • chalk: Used for console output formatting
  • string-comparison: Used in utils.ts:12

✅ Build Scripts

Verified that build tools remain accessible:

  • tsup is correctly in devDependencies for both core and evals packages (used in build scripts)
  • tsx is correctly in devDependencies for both packages (used for TypeScript execution)

✅ Removed Dependencies

The following dependencies were removed from the root and appear to be truly unused:

  • @langchain/community, @langchain/langgraph, @langchain/openai (LangChain deps - likely replaced by direct AI SDK usage)
  • @playwright/test (moved to core package where tests live)
  • express, multer (server dependencies - not needed)
  • puppeteer-core (browser automation - already in core's optionalDependencies)
  • chromium-bidi (CDP protocol - not directly used)
  • autoevals (evaluation library - unused)
  • cheerio (HTML parsing - not needed at root)
  • Various type packages (@types/cheerio, @types/express, etc.)

✅ Security & Performance

  • Security: No security concerns. All changes are dependency reorganization
  • Performance: This change actually improves performance by:
    • Reducing root-level installation bloat
    • Allowing more precise dependency resolution
    • Faster installs for consumers who only need certain packages

🎯 Potential Considerations

  1. Build verification: Recommend running a full CI build to ensure all packages still build correctly
  2. pnpm-lock.yaml size: The -2,096 deletions is excellent and indicates proper cleanup
  3. Workspace hoisting: This change might affect how pnpm hoists dependencies, but this is generally a good thing for correctness

📋 Test Coverage

The PR description mentions "test plan" but it is empty. Consider adding:

  • Run pnpm install successfully
  • Build all packages: pnpm build
  • Run tests in core: pnpm --filter @browserbasehq/stagehand test
  • Run evals: pnpm --filter @browserbasehq/stagehand-evals evals
  • Verify examples still work

✅ Final Verdict

LGTM! 🚀 This is a well-executed cleanup that improves the project structure. The changes are:

  • Correctly scoped
  • Properly verified
  • Low risk
  • High value

Recommendation: Approve and merge after CI passes.


Review generated by Claude Code

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.

3 participants