-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove hoisting of dependencies to prevent global scope packages #310
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
😱 Found 1 issue. Time to roll up your sleeves! 😱 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to cccefc0 in 2 minutes and 21 seconds
More details
- Looked at
232
lines of code in16
files - Skipped
1
files when reviewing. - Skipped posting
36
drafted comments based on config settings.
1. .npmrc:1
- Draft comment:
Removal of public hoist patterns is intentional; please ensure dependencies that were hoisted do not break resolution. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. apps/app-config/constants.ts:9
- Draft comment:
Good enhancement: adding VERCEL_URL and localhost fallback improves robustness in determining the server URL. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. apps/app-config/package.json:75
- Draft comment:
Updated Zod version to ^3.24.2 for consistency; verify that any breaking changes are handled. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. connectors/cnext/package.json:11
- Draft comment:
Added Zod dependency update; consistency maintained. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. connectors/connector-postgres/package.json:16
- Draft comment:
Dependency changes: Addition of 'remeda' and Zod update; confirm no unintended side effects. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. kits/connect/package.json:13
- Draft comment:
Zod version update to ^3.24.2 looks good; consistent dependency versioning across packages. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. packages/api/authRouter.ts:14
- Draft comment:
Switching the import of TRPCError to '@openint/trpc' aligns with internal customizations; ensure all error handling is consistent. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment suggests ensuring consistency in error handling, which is a form of asking the author to double-check their work. This violates the rule against asking the author to ensure behavior is intended or tested.
8. packages/api/createRouterHandler.ts:23
- Draft comment:
Using fetchRequestHandler from '@openint/trpc' instead of '@trpc/server/adapters/fetch' centralizes the abstraction; ensure the new adapter is compatible. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure compatibility, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or ask for a specific test to be written.
9. packages/db/package.json:20
- Draft comment:
Zod version updated to ^3.24.2; maintain consistency across packages. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
10. packages/engine-backend/router/pipelineRouter.ts:130
- Draft comment:
Changed omit from removing both 'id' and 'settings' to only 'id'. Verify that sensitive settings are not inadvertently exposed; deletion later in parseConnection mitigates this. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
11. packages/engine-frontend/package.json:16
- Draft comment:
Explicitly adding 'next' version (15.2.1) ensures a consistent runtime environment for front-end code. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
12. packages/meta-service-airbyte/package.json:18
- Draft comment:
Zod version updated to ^3.24.2 here as well; great for version consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
13. packages/trpc/index.ts:6
- Draft comment:
No issues; maintaining exports from '@trpc/server' is fine and supports existing consumers. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
14. packages/ui/package.json:43
- Draft comment:
Ensure the explicit next version is replicated in both frontend packages for consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
15. packages/util/package.json:41
- Draft comment:
Zod update to ^3.24.2 supports consistent validations across utilities. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
16. unified/unified-file-storage/adapters/sharepoint-adapter/index.ts:2
- Draft comment:
Importing TRPCError from '@openint/vdk' instead of '@trpc/server' fits with internal patterns; double-check that error handling and messaging are compatible. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment suggests checking compatibility of error handling and messaging, which violates the rule against asking the PR author to double-check things. It does not provide a specific code suggestion or ask for a specific test to be written.
17. .npmrc:1
- Draft comment:
Deletion of the .npmrc file removes custom hoisting. Ensure that this change doesn't impact dependency resolution or module layout in your build system. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. apps/app-config/constants.ts:10
- Draft comment:
Switching to bracket notation for accessing the 'host' header improves consistency. Verify that the header is always available and consider a fallback if it’s missing. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code already has multiple layers of fallbacks if the req.headers['host'] part fails - it will try environment variables and eventually default to localhost. The change from .host to ['host'] notation is just a style change. The type definition shows headers is Record<string, string> so TypeScript ensures it's safe.
Maybe there's a case where having req but missing the host header could cause different behavior than having no req at all?
Even in that case, the existing fallback chain would handle it gracefully by moving to the environment variables and localhost fallbacks.
The comment should be deleted because the code already has robust fallback handling, and the suggestion would actually make the fallback logic less clear.
19. apps/app-config/package.json:75
- Draft comment:
Zod version has been updated to ^3.24.2. Confirm that all schema validations across dependent packages remain compatible. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to confirm compatibility across dependent packages, which violates the rule against asking for confirmation or ensuring compatibility. It does not provide a specific code suggestion or point out a specific issue.
20. connectors/connector-postgres/package.json:14
- Draft comment:
A new dependency 'remeda' has been added and zod updated to ^3.24.2. Ensure this version of remeda aligns with the rest of the monorepo to prevent potential version conflicts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is about dependencies, specifically mentioning a new dependency 'remeda' and an update to 'zod'. It advises ensuring version alignment within the monorepo. This falls under the rule of not commenting on dependency changes or library versions that are not recognized. The comment does not provide a specific code suggestion or ask for a test to be written, which would make it useful.
21. kits/connect/package.json:13
- Draft comment:
Zod version updated to ^3.24.2. Verify that consumers of this package are not affected by any breaking changes in the new version. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify that consumers are not affected by breaking changes, which falls under the rule of not asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue with the code.
22. packages/api/authRouter.ts:14
- Draft comment:
TRPCError import has been moved from '@trpc/server' to '@openint/trpc'. Ensure that the new re-export or wrapper maintains the same behavior for error handling. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is about a change in the import path for TRPCError. It suggests ensuring that the new import maintains the same behavior, which is a request for confirmation of behavior. This violates the rule against asking the PR author to ensure behavior is intended.
23. packages/api/createRouterHandler.ts:20
- Draft comment:
The fetchRequestHandler import has been switched to use '@openint/trpc'. Confirm that its API and error handling remain fully compatible with previous usage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm compatibility, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code change.
24. packages/db/package.json:23
- Draft comment:
Zod version bumped to ^3.24.2. Ensure that all database schema validations and related logic continue to function correctly with the new version. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that the database schema validations and related logic continue to function correctly with the new version of Zod. This falls under the category of asking the author to double-check things, which is against the rules. The comment does not provide a specific code suggestion or point out a specific issue with the code.
25. packages/engine-backend/router/pipelineRouter.ts:130
- Draft comment:
The omission of the 'settings' property from the connection schema has been removed. Ensure that exposing settings does not leak sensitive configuration details. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code already handles settings security by explicitly deleting them at line 178 before returning the data. The .omit() change doesn't create a security risk because settings are handled elsewhere. The comment's suggestion is also technically incorrect - .omit() takes an object of keys to omit, not boolean values.
I could be wrong about the relationship between the .omit() call and the later deletion of settings. Maybe there's a reason both were needed originally.
The explicit deletion with a clear comment about security at line 178 shows this is the intended place to handle sensitive data. Having redundant security measures could make the code more confusing.
Delete this comment. The security concern is already properly handled elsewhere in the code, and the suggested fix uses incorrect syntax.
26. packages/engine-frontend/package.json:16
- Draft comment:
An explicit dependency on 'next' version 15.2.1 has been added. Confirm that this version is compatible with the rest of your frontend setup. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to confirm compatibility of a dependency version, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
27. packages/meta-service-airbyte/package.json:15
- Draft comment:
Zod has been updated to ^3.24.2. Verify that schema validations in the airbyte service remain correct after this upgrade. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify the correctness of schema validations after a dependency update. This falls under the rule of not asking the author to ensure behavior is intended or tested, especially related to dependencies.
28. packages/trpc/index.ts:7
- Draft comment:
The export for fetchRequestHandler has been added. This centralizes its usage; ensure that all consumers are updated to import from this consolidated module. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is purely informative and asks the author to ensure that all consumers are updated, which violates the rules. It doesn't provide a specific code suggestion or ask for a specific test to be written.
29. packages/ui/package.json:42
- Draft comment:
A dependency on 'next' version 15.2.1 has been added. Make sure this version is consistent with other packages in the monorepo. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is about a dependency change, specifically mentioning a version of a package. The rules state not to comment on pure dependency changes or library versions that are not recognized. This comment is asking to ensure consistency with other packages, which is similar to asking for confirmation or verification, which is not allowed.
30. packages/util/package.json:41
- Draft comment:
Zod has been upgraded to ^3.24.2. Verify that there are no breaking changes affecting utility functions or type validations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify that there are no breaking changes, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
31. unified/unified-file-storage/adapters/sharepoint-adapter/index.ts:2
- Draft comment:
TRPCError is now imported from '@openint/vdk'. Ensure that the error handling behavior is consistent and that the new source correctly maps error codes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure consistency in error handling behavior and correct mapping of error codes. This falls under asking the author to double-check things, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue.
32. apps/app-config/constants.ts:25
- Draft comment:
Consider renaming 'getGraphqlEndpoint' to 'getGraphQLEndpoint' for clarity and consistency with common GraphQL naming conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
33. apps/app-config/package.json:25
- Draft comment:
Typographical error: The package '@openint/connector-foreceipt' appears to have a possible misspelling. Please check if 'foreceipt' is intended or if it should be corrected to something like 'connector-for-receipt' or another proper spelling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
34. packages/api/authRouter.ts:114
- Draft comment:
Typographical Error: On line 114, the code mistakenly uses 'usr.publicMetadata' when parsing unsafeMetadata. Please change it to 'usr.unsafeMetadata' to correctly reflect the intended data. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
35. packages/api/createRouterHandler.ts:111
- Draft comment:
Typographical error: 'ignoroing' should be corrected to 'ignoring' in the commented-out console.warn statement. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
36. unified/unified-file-storage/adapters/sharepoint-adapter/index.ts:397
- Draft comment:
Typographical error: Change 'Sharepoint' to 'SharePoint' in the error message of exportFile for correct product naming. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_efhKM2ljjDpfupcL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Remove hoisting configurations and update
zod
and other dependencies across multiple packages, with minor code refactoring..npmrc
file, eliminating all hoisting configurations.zod
dependency to^3.24.2
inapps/app-config/package.json
,connectors/cnext/package.json
,connectors/connector-postgres/package.json
,kits/connect/package.json
,packages/db/package.json
,packages/meta-service-airbyte/package.json
,packages/util/package.json
.remeda
dependency inconnectors/connector-postgres/package.json
.next
dependency inpackages/engine-frontend/package.json
andpackages/ui/package.json
.TRPCError
import from@trpc/server
to@openint/trpc
inpackages/api/authRouter.ts
andpackages/api/createRouterHandler.ts
.zStandard.connection.omit
call inpipelineRouter.ts
to removesettings
omission.TRPCError
import inunified/unified-file-storage/adapters/sharepoint-adapter/index.ts
to@openint/vdk
.This description was created by
for cccefc0. It will automatically update as commits are pushed.