chore: publish npm packages under @deepnote org without any other changes #5
Conversation
|
Warning Rate limit exceeded@saltenasl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR removes yarn cache settings from CI/release workflows, updates Node setup to 20.x in the comment workflow, and changes release publish to run a new script Sequence DiagramsequenceDiagram
participant Release as Release Workflow
participant Script as scripts/publish-fork.js
participant FS as File System
participant Yarn as yarn changeset publish
Release->>Script: node scripts/publish-fork.js
Script->>FS: read packages/slate/package.json
Script->>FS: read packages/slate-react/package.json
Note over Script: modify package.json files
Script->>FS: rename slate → `@deepnote/slate`
Script->>FS: rename slate-react → `@deepnote/slate-react`
Script->>FS: update slate-react deps → npm:deepnote/slate@<version>
Script->>Yarn: yarn changeset publish
Yarn-->>Script: publish result
Script->>FS: restore original package.json files (finally)
Script-->>Release: exit (log success or error)
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks).github/workflows/comment.yml(1 hunks).github/workflows/release.yml(2 hunks)packages/slate-react/src/custom-types.ts(1 hunks)scripts/publish-fork.js(1 hunks)
⏰ 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). (7)
- GitHub Check: test:integration
- GitHub Check: test
- GitHub Check: lint:prettier
- GitHub Check: lint:eslint
- GitHub Check: lint:typescript
- GitHub Check: test
- GitHub Check: test:integration
🔇 Additional comments (2)
scripts/publish-fork.js (2)
28-35: Approve dependency aliasing approach.Using npm alias syntax (
npm:@deepnote/slate@${version}) correctly rewrites slate-react's dependencies to reference the forked package.
47-47: Implementation is correct. The package.json files are mutated with the forked names (@deepnote/slate,@deepnote/slate-react) beforeyarn changeset publishruns at line 47, ensuring publication uses the correct names. Cross-package dependencies are also updated. Original files are restored in the finally block after publishing completes. No risk of publishing to original package names.
| } | ||
| Range: BaseRange & { | ||
| placeholder?: string | ||
| multiBlock?: boolean |
There was a problem hiding this comment.
Change contradicts PR title.
The PR title states "without any other changes" but adding multiBlock is a functional change to the type system.
🤖 Prompt for AI Agents
In packages/slate-react/src/custom-types.ts around line 12, the new optional
property `multiBlock?: boolean` was added which is a functional type-system
change that contradicts the PR title claiming "without any other changes";
either remove/revert the `multiBlock` addition to keep the change purely
non-functional, or if the property is required, update the PR title and
description to reflect this behavioral/type change and add a brief rationale in
the PR body explaining why the new field is necessary and any downstream
impacts.
There was a problem hiding this comment.
@saltenasl, understood! Since this was a necessary addition, you might want to mention it briefly in the PR description for future reference—but that's up to you.
🐰
(•‿•)
/>📝
✏️ Learnings added
Learnt from: saltenasl
Repo: deepnote/slate PR: 5
File: packages/slate-react/src/custom-types.ts:12-12
Timestamp: 2025-11-24T16:52:16.764Z
Learning: In packages/slate-react/src/custom-types.ts, the multiBlock?: boolean property was intentionally added to the Range interface as it was missing functionality, even though the PR title mentioned "without any other changes".
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/ci.yml(0 hunks).github/workflows/comment.yml(1 hunks).github/workflows/release.yml(1 hunks)config/typescript/tsconfig.json(1 hunks)package.json(3 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🔇 Additional comments (4)
.github/workflows/release.yml (2)
28-30: Node version differs from comment workflow.Release workflow uses Node 16.x while comment.yml uses Node 20.x. Confirm this difference is intentional; mismatched environments can cause subtle publish issues.
45-45: Script verified and ready.The
scripts/publish-fork.jsexists and correctly implements the forking strategy: reads the original package.json files, mutates names/dependencies to the @deepnote scope, runsyarn changeset publish, then restores originals. Proper error handling ensures restoration on failure. Will execute successfully in the CI environment..github/workflows/comment.yml (1)
48-48: Cache key correctly updated to match Node version.Node version and cache key are paired and consistent. No issues here.
Also applies to: 50-50
config/typescript/tsconfig.json (1)
1-16: Verify TypeScript build and check typed object definitions.Removing
suppressImplicitAnyIndexErrorsenforces stricter typing on dynamic property access. The codebase dynamically indexes objects likeELEMENT_TAGS[nodeName],HOTKEYS[hotkey], andmarks[format]. Confirm the build passes and these objects have proper type definitions or index signatures.
package.json
Outdated
| "scripts": { | ||
| "build": "yarn build:rollup && yarn build:next", | ||
| "build:next": "cd ./site && next build && next export", | ||
| "build:next": "cd ./site && NODE_OPTIONS=--openssl-legacy-provider next build && NODE_OPTIONS=--openssl-legacy-provider next export", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify legacy OpenSSL flag necessity and consider upgrading Next.js.
The NODE_OPTIONS=--openssl-legacy-provider flag is typically a workaround for Node 16+ compatibility with old dependencies. With the Node 20.x upgrade in CI, confirm this flag is actually required and consider if Next.js (currently 10.x, line 86) should be upgraded instead of suppressing the issue.
Also applies to: 29-29
🏁 Script executed:
#!/bin/bash
# Check Next.js version and whether build succeeds without the legacy OpenSSL flag
grep '"next"' package.jsonLength of output: 22
🌐 Web query:
Next.js 10.2.3 Node 20 compatibility openssl-legacy-provider
💡 Result:
Short answer: Next.js 10.2.3 isn’t tested against Node 20 and you’ll likely hit OpenSSL/NODE_OPTIONS problems if your environment or scripts rely on NODE_OPTIONS="--openssl-legacy-provider". Node 20’s CLI supports the --openssl-legacy-provider flag but Node disallows that option when passed via NODE_OPTIONS for security, so the usual project-level NODE_OPTIONS approach will fail — remove/unset NODE_OPTIONS or run the flag directly when starting Node, or use a Node version compatible with that flag usage (e.g., Node 16/18). [1][2][3]
What to do (concise):
- Remove any NODE_OPTIONS="--openssl-legacy-provider" (unset NODE_OPTIONS) and retry. This is the most common fix. [2][4]
- If you must use the legacy provider, start Node with the flag directly (node --openssl-legacy-provider ...) rather than via NODE_OPTIONS — Node’s CLI documents the flag. [1]
- Prefer upgrading Next.js (to a recent v12+/v13+) or use Node 18/16 LTS when running Next.js 10.2.3 to avoid breakage. Older Next.js / webpack toolchains are more likely to need legacy OpenSSL behavior. [2][5]
Sources:
- Node.js CLI docs (--openssl-legacy-provider exists; flag behavior). [1]
- Reports/answers about "node: --openssl-legacy-provider is not allowed in NODE_OPTIONS" and unsetting NODE_OPTIONS. (StackOverflow, community posts). [2][3]
- Practical writeups/recipes for removing the flag or using direct node invocation. [4][5]
If you want, I can:
- scan your package.json / environment to show where NODE_OPTIONS is set and give exact edits; or
- suggest a minimal upgrade path for Next.js/Node to avoid this entirely.
🌐 Web query:
Next.js 10 Node 20 support requirements
💡 Result:
Short answer: Next.js 10 does not support Node.js 20 — it predates Node 20, is no longer supported by Vercel, and was only tested against the older Node LTS lines current in 2020–2021. You should either upgrade Next.js (recommended) or run an older Node LTS that matches Next.js 10's era. [1][2]
Sources:
- Next.js support / EOL list (shows v10 is unsupported). [1]
- Netlify Next.js runtime notes (documents special handling for older Next.js versions and older Node runtimes — Next.js 10–13.4 require legacy runtime/older Node). [2]
Remove NODE_OPTIONS flag; upgrade Next.js for Node 20 compatibility.
Next.js 10.2.3 doesn't support Node 20, and the NODE_OPTIONS=--openssl-legacy-provider flag will fail on Node 20—it's disallowed via NODE_OPTIONS for security reasons. Either upgrade Next.js to v13+ or revert to Node 18 LTS. The current approach breaks builds on Node 20.
Also applies to: 29-29
🤖 Prompt for AI Agents
In package.json around line 9 (and also line 29), the build script injects
NODE_OPTIONS=--openssl-legacy-provider which is rejected on Node 20; remove that
environment variable from the "build:next" script and either upgrade the
project's Next.js dependency to a Node‑20‑compatible release (v13+ recommended)
or explicitly pin CI/build to Node 18 LTS (update CI config and any
Dockerfiles). Update package.json scripts to run next build/export without
NODE_OPTIONS and bump the Next.js version in package.json plus run yarn/npm
install and test builds to ensure compatibility.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.