fix(toolchain): mark external deps #2713
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This was referenced Jul 3, 2025
Closed
|
Claude finished @NathanFlurry's task —— View job Code Review CompleteI've analyzed the changes in this PR that focus on marking external dependencies in the toolchain. Here's my detailed feedback: Todo List:
🔍 Review FeedbackCritical Issues 🚨
Code Quality Issues
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| @@ -86,5 +91,5 @@ export const rivetPreset: Preset = { | |||
| //process: "@cloudflare/unenv-preset/runtime/node/process/index", | |||
| }, | |||
| polyfill: [], | |||
| external: nodeCompatModules.flatMap((p) => [p, `node:${p}`]), | |||
| externa: nodeCompatModules.flatMap((p) => [p, `node:${p}`]), | |||
There was a problem hiding this comment.
There appears to be a typo in the property name: externa should be external. This will prevent the external modules configuration from working correctly, as the system won't recognize the misspelled property.
Suggested change
| externa: nodeCompatModules.flatMap((p) => [p, `node:${p}`]), | |
| external: nodeCompatModules.flatMap((p) => [p, `node:${p}`]), |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| @@ -48,6 +50,9 @@ const hybridNodeCompatModules = [ | |||
| "util", | |||
| ]; | |||
|
|
|||
| const externalModules = nodeCompatModules.flatMap((p) => [p, `node:${p}`]) | |||
There was a problem hiding this comment.
The externalModules variable is defined but unused, while the same expression is duplicated in the externa property below. Consider either:
- Using the variable in the property:
externa: externalModules - Removing the unused variable definition
Also note that the property name appears to have a typo - externa should likely be external based on the context.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| @@ -54,6 +54,8 @@ export async function build(input: Input): Promise<Output> { | |||
| stdLibBrowser[ `node:${packageName}` ] = packagePath; | |||
| } | |||
|
|
|||
| console.log('foo') | |||
There was a problem hiding this comment.
This debug console.log('foo') statement should be removed before merging. It appears to be unintentional debugging code that made its way into the PR.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| @@ -48,6 +50,9 @@ const hybridNodeCompatModules = [ | |||
| "util", | |||
| ]; | |||
|
|
|||
| const externalModules = nodeCompatModules.flatMap((p) => [p, `node:${p}`]) | |||
| console.log('modules', externalModules); | |||
There was a problem hiding this comment.
This debug console.log statement should be removed before merging. It appears to be printing the external modules list, which is likely only needed during development.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
NathanFlurry
force-pushed
the
07-03-fix_toolchain_mark_external_deps
branch
from
363e596 to
9443c96
Compare
NathanFlurry
force-pushed
the
06-08-fix_toolchain_fix_compat_with_deploying_using_podman
branch
from
31794bf to
03bcfbc
Compare
Deploying rivet-studio with
|
| Latest commit: |
34d28a2
|
| Status: | ⚡️ Build in progress... |
|
Claude finished @NathanFlurry's task —— View job Code Review CompleteI've analyzed the changes in this PR that focus on marking external dependencies in the toolchain. Here's my detailed feedback: Todo List:
🔍 Review FeedbackCritical Issues 🚨
Code Quality Issues
|
|
Claude finished @NathanFlurry's task —— View job PR Code Review CompleteReviewed the changes in this PR that focus on marking external dependencies in the toolchain. Todo List:
🔍 Review FeedbackCritical Issues 🚨
Code Quality Issues
|
This was referenced Jul 4, 2025
Deploying rivet with
|
| Latest commit: |
ecc699b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e96f4b2e.rivet.pages.dev |
| Branch Preview URL: | https://07-03-fix-toolchain-mark-ext.rivet.pages.dev |
Deploying rivet-hub with
|
| Latest commit: |
ecc699b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4551e855.rivet-hub-7jb.pages.dev |
| Branch Preview URL: | https://07-03-fix-toolchain-mark-ext.rivet-hub-7jb.pages.dev |
This was referenced Jul 4, 2025
NathanFlurry
force-pushed
the
07-03-fix_toolchain_mark_external_deps
branch
from
9443c96 to
e81e23e
Compare
NathanFlurry
force-pushed
the
06-08-fix_toolchain_fix_compat_with_deploying_using_podman
branch
from
03bcfbc to
5db3aad
Compare
|
Claude finished @NathanFlurry's task —— View job Claude finished @NathanFlurry's task —— View job Code Review CompleteI've analyzed the changes in this PR that focus on marking external dependencies in the toolchain. Here's my detailed feedback: Todo List:
🔍 Review FeedbackCritical Issues 🚨
Code Quality Issues
|
|
Claude finished @NathanFlurry's task —— View job Claude is reviewing this PR Todo List:
🔍 Code Review FeedbackCritical Issues 🚨
Code Quality Issues
|
NathanFlurry
force-pushed
the
06-08-fix_toolchain_fix_compat_with_deploying_using_podman
branch
from
5db3aad to
c1e41fb
Compare
NathanFlurry
force-pushed
the
07-03-fix_toolchain_mark_external_deps
branch
from
e81e23e to
30e7e82
Compare
|
Claude finished @NathanFlurry's task —— View job Claude Code is reviewing this PR… Todo List:
🔍 Code Review FeedbackCritical Issues 🚨
Code Quality Issues
|
|
Claude finished @NathanFlurry's task —— View job Code Review CompleteI've analyzed the changes in this PR that focus on marking external dependencies in the toolchain. Here's my detailed feedback: Todo List:
🔍 Review FeedbackCritical Issues 🚨
Code Quality Issues
|
This was referenced Jul 11, 2025
|
Claude finished @NathanFlurry's task —— View job Claude Code is reviewing this PR… Todo List:
🔍 Code Review FeedbackCritical Issues 🚨
Code Quality Issues
|
|
Claude finished @NathanFlurry's task —— View job Claude Code is reviewing this PR… Todo List:
🔍 Code Review FeedbackCritical Issues 🚨
Code Quality Issues
|
NathanFlurry
force-pushed
the
07-03-fix_toolchain_mark_external_deps
branch
from
34d28a2 to
ecc699b
Compare
NathanFlurry
force-pushed
the
06-08-fix_toolchain_fix_compat_with_deploying_using_podman
branch
from
97a6df1 to
184d5a4
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review CompleteI've analyzed the changes in this PR that focus on marking external dependencies in the toolchain. Here's my detailed feedback: Todo List:
🔍 Review FeedbackCritical Issues 🚨
Code Quality Issues
|
|
Claude finished @NathanFlurry's task —— View job Claude Code is reviewing this PR… Todo List:
🔍 Code Review FeedbackCritical Issues 🚨
Code Quality Issues
|
MasterPtato
force-pushed
the
06-08-fix_toolchain_fix_compat_with_deploying_using_podman
branch
from
184d5a4 to
97a6df1
Compare
MasterPtato
force-pushed
the
07-03-fix_toolchain_mark_external_deps
branch
from
ecc699b to
34d28a2
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review CompleteI've analyzed the changes in this PR that focus on marking external dependencies in the toolchain. Here's my detailed feedback: Todo List:
🔍 Review FeedbackCritical Issues 🚨
Code Quality Issues
|
Merge activity
|
graphite-app bot
pushed a commit
that referenced
this pull request
Jul 12, 2025
<!-- Please make sure there is an issue that this PR is correlated to. --> ## Changes <!-- If there are frontend changes, please include screenshots. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes