Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (39)
💤 Files with no reviewable changes (5)
✅ Files skipped from review due to trivial changes (20)
🚧 Files skipped from review as they are similar to previous changes (11)
WalkthroughMonorepo-wide dependency and tooling upgrades, removal of Yarn-specific CI/cache settings, TypeScript target/module changes, and targeted migration from Koa v2/koa-router to Koa v3/@koa/router with corresponding route patterns and type updates. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts (2)
31-45:⚠️ Potential issue | 🟠 MajorAdd
cdto themethodsToOverridelist to serialize all FTP operations.The queue wraps certain FTP methods for exclusive execution, but
cdis missing from the list. This leaves directthis.client.cd('/')calls at lines 96, 199, 218, and 322 running outside the queue, allowing them to interleave with queued operations likeensureDirandremove. For example,ensureDir(queued) →cd(unqueued) →remove(queued) can race if another queued operation is waiting. This breaks the one-at-a-time guarantee.Suggested fix
const methodsToOverride: (keyof FTP.Client)[] = [ 'access', + 'cd', // 'close', // 'closed', 'downloadTo',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts` around lines 31 - 45, The FTPClient currently lists methods to wrap in methodsToOverride but omits 'cd', so direct calls to this.client.cd bypass the queue; add 'cd' to the methodsToOverride array (alongside 'downloadTo', 'ensureDir', etc.) so the wrapper/serialization logic in the FTPClient class will queue and serialize calls to this.client.cd (thereby ensuring calls like this.client.cd('/') used elsewhere are routed through the same exclusive execution mechanism).
52-54:⚠️ Potential issue | 🔴 CriticalUnsafe
JSON.stringify(args)breaks upload/download operations before execution.The queue wrapper serializes
argson line 53 for error context. SinceuploadFromanddownloadTomethods receive stream objects (Readable, PassThrough) as arguments,JSON.stringify()throws on circular references and crashes the queued operation beforeorgMethod.apply()runs. This prevents legitimate file transfers from executing.Suggested fix
- const orgError = new Error(`Error executing ${method}: ${JSON.stringify(args)}`) // Used later + let serializedArgs = '[unserializable args]' + try { + serializedArgs = JSON.stringify(args) + } catch { + // keep fallback + } + const orgError = new Error(`Error executing ${String(method)}: ${serializedArgs}`) // Used later🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts` around lines 52 - 54, The current queue wrapper creates orgError using JSON.stringify(args), which throws when args contain stream objects (e.g., for uploadFrom/downloadTo) and aborts the operation; replace that unsafe serialization with a safe representation: detect stream-like objects (Readable, PassThrough) or otherwise wrap JSON.stringify in a try/catch and fall back to a placeholder (e.g., "<stream>" or "<unserializable>") per argument so that creating orgError never throws; update the code around queue.add and the orgError creation (referencing method, args, and orgMethod.apply) to use this safe stringify logic before entering the try block.
🧹 Nitpick comments (3)
shared/packages/api/package.json (1)
20-20: Remove deprecated@types/winstonstub.Line 20 adds a legacy stub package while
winstonv3 already provides its own typings. Version mismatch between the@types/winstonv2.4.4 stubs andwinstonv3.19.0 runtime can cause type conflicts.♻️ Proposed cleanup
- "@types/winston": "^2.4.4",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/api/package.json` at line 20, Remove the legacy TypeScript stub "@types/winston" from package.json to avoid conflicts with winston v3's built-in typings; locate the entry "@types/winston": "^2.4.4" and delete it from dependencies/devDependencies, then run the package manager to update the lockfile (npm install / yarn install / pnpm install) so the change is reflected in package-lock.json/yarn.lock and rebuild types to confirm no remaining type errors.jest.config.base.js (1)
1-1: Consider usingnode:pathfor explicit built-in module identification.While
require('path')is standard and always valid, thenode:prefix makes it explicit that this is a built-in module. This is an optional stylistic convention in modern Node.js.Optional cleanup
-const path = require('path') +const path = require('node:path')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.config.base.js` at line 1, Update the built-in module import to use the explicit node: prefix by replacing the top-level require('path') with require('node:path') in jest.config.base.js (the symbol to change is the const path = require('path') declaration), keeping the rest of the file unchanged so it still uses the same `path` variable for path resolution.apps/quantel-http-transformer-proxy/packages/generic/package.json (1)
14-14: Duplicate@koa/corsin dependencies and devDependencies.
@koa/corsis listed in bothdependencies(line 14) anddevDependencies(line 29) with the same version. This is redundant—it only needs to be independenciessince it's used at runtime.🧹 Proposed fix to remove duplicate
"devDependencies": { - "@koa/cors": "^5.0.0", "@types/koa": "^3.0.1",Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/quantel-http-transformer-proxy/packages/generic/package.json` at line 14, The package.json currently lists the same runtime package "@koa/cors" in both dependencies and devDependencies; remove the duplicate entry from devDependencies so "@koa/cors" remains only in dependencies to avoid redundancy and ensure it's available at runtime; update the devDependencies block to delete the "@koa/cors" entry and ensure package.json remains valid JSON after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/http-server/app/package.json`:
- Line 15: The package.json currently lists "rimraf" in dependencies but it's
only used by the build script ("build": "yarn rimraf dist ..."); remove the
"rimraf" entry from the dependencies object and add the same version string to
devDependencies instead (mirroring packages/generic), then update the lockfile
by running your package manager install (yarn or npm) so the move is reflected;
also ensure you remove the duplicate occurrence noted elsewhere and run the
build script to verify nothing breaks.
In `@apps/http-server/packages/generic/src/lib.ts`:
- Around line 1-5: The exported CTX and CTXPost types currently alias
RouterContext<any, any>, which erases context/state typing; update their
definitions to use Koa's DefaultState and DefaultContext so compile-time types
are preserved: replace occurrences of RouterContext<any, any> for the CTX and
CTXPost exports with RouterContext<DefaultState, DefaultContext> (import
DefaultState and DefaultContext from 'koa') and keep the same export names (CTX,
CTXPost).
In `@apps/http-server/packages/generic/src/server.ts`:
- Around line 98-105: The route patterns using the older wildcard syntax (e.g.
'/package/*path' and '/uploadForm/*path') won't match under `@koa/router` v15+;
update the router calls in server.ts (the router.get/ head/ post handlers that
call handleStorage and storage.getPackage/headPackage, and the routes around the
uploadForm handlers) to use the new named wildcard syntax '/package/{/*path}'
and '/uploadForm/{/*path}' so Koa will correctly bind ctx.params.path for the
existing handleStorage, storage.getPackage, storage.headPackage and the upload
form handlers.
In `@apps/quantel-http-transformer-proxy/packages/generic/src/server.ts`:
- Line 123: Update the Koa route pattern: replace the incorrect catch-all string
"{/*path}" with the correct "/{/*path}" wherever it's used (e.g., the
this.router.get('{/*path}', async (ctx, next) => { ... }) route in server.ts);
also make the same replacement for the other occurrence in the http-server app
so the root "/" is properly matched.
---
Outside diff comments:
In
`@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts`:
- Around line 31-45: The FTPClient currently lists methods to wrap in
methodsToOverride but omits 'cd', so direct calls to this.client.cd bypass the
queue; add 'cd' to the methodsToOverride array (alongside 'downloadTo',
'ensureDir', etc.) so the wrapper/serialization logic in the FTPClient class
will queue and serialize calls to this.client.cd (thereby ensuring calls like
this.client.cd('/') used elsewhere are routed through the same exclusive
execution mechanism).
- Around line 52-54: The current queue wrapper creates orgError using
JSON.stringify(args), which throws when args contain stream objects (e.g., for
uploadFrom/downloadTo) and aborts the operation; replace that unsafe
serialization with a safe representation: detect stream-like objects (Readable,
PassThrough) or otherwise wrap JSON.stringify in a try/catch and fall back to a
placeholder (e.g., "<stream>" or "<unserializable>") per argument so that
creating orgError never throws; update the code around queue.add and the
orgError creation (referencing method, args, and orgMethod.apply) to use this
safe stringify logic before entering the try block.
---
Nitpick comments:
In `@apps/quantel-http-transformer-proxy/packages/generic/package.json`:
- Line 14: The package.json currently lists the same runtime package "@koa/cors"
in both dependencies and devDependencies; remove the duplicate entry from
devDependencies so "@koa/cors" remains only in dependencies to avoid redundancy
and ensure it's available at runtime; update the devDependencies block to delete
the "@koa/cors" entry and ensure package.json remains valid JSON after the
change.
In `@jest.config.base.js`:
- Line 1: Update the built-in module import to use the explicit node: prefix by
replacing the top-level require('path') with require('node:path') in
jest.config.base.js (the symbol to change is the const path = require('path')
declaration), keeping the rest of the file unchanged so it still uses the same
`path` variable for path resolution.
In `@shared/packages/api/package.json`:
- Line 20: Remove the legacy TypeScript stub "@types/winston" from package.json
to avoid conflicts with winston v3's built-in typings; locate the entry
"@types/winston": "^2.4.4" and delete it from dependencies/devDependencies, then
run the package manager to update the lockfile (npm install / yarn install /
pnpm install) so the change is reflected in package-lock.json/yarn.lock and
rebuild types to confirm no remaining type errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ef0909d-6fbc-4183-9de2-224b4afd99e5
⛔ Files ignored due to path filters (2)
.yarn/releases/yarn-4.1.1.cjsis excluded by!**/.yarn/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
.github/workflows/build-windows.yaml.github/workflows/lint-and-test.yaml.github/workflows/publish-prerelease.yaml.yarnrc.ymlapps/_boilerplate/app/package.jsonapps/_boilerplate/packages/generic/package.jsonapps/appcontainer-node/app/package.jsonapps/appcontainer-node/packages/generic/package.jsonapps/html-renderer/app/package.jsonapps/html-renderer/packages/generic/package.jsonapps/http-server/app/package.jsonapps/http-server/packages/generic/package.jsonapps/http-server/packages/generic/src/lib.tsapps/http-server/packages/generic/src/server.tsapps/package-manager/app/package.jsonapps/package-manager/packages/generic/package.jsonapps/quantel-http-transformer-proxy/app/package.jsonapps/quantel-http-transformer-proxy/packages/generic/package.jsonapps/quantel-http-transformer-proxy/packages/generic/src/server.tsapps/single-app/app/package.jsonapps/worker/app/package.jsonapps/worker/packages/generic/package.jsonapps/workforce/app/package.jsonapps/workforce/packages/generic/package.jsonjest.config.base.jslerna.jsonpackage.jsonshared/packages/api/package.jsonshared/packages/api/src/websocketServer.tsshared/packages/expectationManager/package.jsonshared/packages/worker/package.jsonshared/packages/worker/src/worker/accessorHandlers/__tests__/fileShare.spec.tsshared/packages/worker/src/worker/accessorHandlers/__tests__/localFolder.spec.tsshared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.tsshared/packages/worker/src/worker/accessorHandlers/lib/fetch.tsshared/packages/workforce/package.jsontests/internal-tests/package.jsontsconfig.build.jsontsconfig.jest.json
💤 Files with no reviewable changes (5)
- .yarnrc.yml
- .github/workflows/publish-prerelease.yaml
- .github/workflows/lint-and-test.yaml
- .github/workflows/build-windows.yaml
- shared/packages/worker/src/worker/accessorHandlers/lib/fetch.ts
Julusian
force-pushed
the
feat/update-libs
branch
from
025f340 to
2f1aa32
Compare
nytamin
approved these changes
Mar 18, 2026
Member
nytamin
left a comment
nytamin
left a comment
There was a problem hiding this comment.
It'll be great to get this updated, thanks for this!
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
About Me
Type of Contribution
This is a: Code improvement
Current Behavior
New Behavior
This updates a lot of the tooling and dependencies to be up to date. There are still some more bits that would be nice to look at, but I am unlikely to in this round.
A quick test with a demo rundown appears to be able to scan and generate previews as normal
Testing Instructions
Other Information
Status