chore(deps): bump vite, webpack, underscore, google-storage and is-svg#39400
chore(deps): bump vite, webpack, underscore, google-storage and is-svg#39400julio-rocketchat wants to merge 8 commits intodevelopfrom
vite, webpack, underscore, google-storage and is-svg#39400Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMultiple dependency version bumps across the monorepo (Storybook, webpack, underscore, vite, tsx, esbuild, terser, etc.), plus WebDAV client adapter rewritten for lazy dynamic import and streaming, and image service updated to use dynamic imports and async SVG detection with minor resize signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WebdavAdapter as WebdavClientAdapter
participant WebdavLib as webdav (dynamic)
participant Remote as WebDAV Server
Caller->>WebdavAdapter: request createReadStream(path)
WebdavAdapter->>WebdavAdapter: if !_clientPromise -> import('webdav') -> createClient()
WebdavAdapter->>WebdavLib: await _clientPromise (client)
WebdavAdapter->>Remote: client.createReadStream(path)
Remote-->>WebdavAdapter: remote stream / data
WebdavAdapter-->>Caller: PassThrough stream piping remote stream (errors forwarded)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39400 +/- ##
========================================
Coverage 70.94% 70.94%
========================================
Files 3207 3207
Lines 113958 113962 +4
Branches 20694 20699 +5
========================================
+ Hits 80842 80856 +14
+ Misses 31059 31047 -12
- Partials 2057 2059 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
cardoso
left a comment
cardoso
left a comment
There was a problem hiding this comment.
Can we get rid of some of the duplicates?
Skimming through the yarn.lock (there may be more):
terser-webpack-plugin 5.3.14 & 5.3.16
tapable 2.2.1 & 2.3.0
schema-utils 4.3.2 & 4.3.3
esbuild 0.25.5 & 0.27.3
es-module-lexer 1.5.4 & 2.0.0
enhanced-resolve 5.17.1 & 5.20.0
@types/estree 1.0.6 & 1.0.8
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps-engine/package.json`:
- Line 85: The esbuild version bump to ~0.27.3 may be incompatible with our
legacy bundler; either revert the dep to a secure 0.25.x release (e.g.
"~0.25.12") in package.json, or add a focused integration test that exercises
the legacy bundler (bundleLegacyApp in
packages/apps-engine/src/server/runtime/deno/bundler.ts) calling the esbuild JS
API (build(), onResolve(), onLoad(), outputFiles) to ensure the current config
(including target: ['node10'] and base64 loader behavior) still produces the
expected output; implement the test to fail the PR if the bundle output or build
invocation changes so we validate compatibility before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8751dc17-4b0a-434c-abd9-3413f87b2a5c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
apps/meteor/package.jsonee/packages/pdf-worker/package.jsonpackages/apps-engine/package.jsonpackages/fuselage-ui-kit/package.jsonpackages/gazzodown/package.jsonpackages/livechat/package.jsonpackages/mock-providers/package.jsonpackages/storybook-config/package.jsonpackages/ui-client/package.jsonpackages/ui-composer/package.jsonpackages/ui-video-conf/package.jsonpackages/ui-voip/package.jsonpackages/web-ui-registration/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/gazzodown/package.json
- packages/livechat/package.json
- packages/storybook-config/package.json
- packages/ui-composer/package.json
📜 Review details
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
ee/packages/pdf-worker/package.jsonpackages/ui-client/package.jsonpackages/mock-providers/package.jsonpackages/ui-video-conf/package.jsonapps/meteor/package.jsonpackages/web-ui-registration/package.jsonpackages/fuselage-ui-kit/package.jsonpackages/ui-voip/package.json
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
ee/packages/pdf-worker/package.jsonpackages/ui-client/package.jsonpackages/ui-video-conf/package.jsonapps/meteor/package.json
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
ee/packages/pdf-worker/package.jsonpackages/ui-client/package.jsonpackages/ui-video-conf/package.jsonpackages/web-ui-registration/package.jsonpackages/ui-voip/package.json
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
ee/packages/pdf-worker/package.json
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/mock-providers/package.json
🔇 Additional comments (12)
packages/fuselage-ui-kit/package.json (2)
61-67: Storybook package alignment looks good.These bumps keep the Storybook packages on the same patch version, which is the right shape for avoiding tooling skew in this workspace.
86-89: Tooling bumps are consistent with the rest of the manifest.
storybookstays aligned with the related addons/framework packages, and thewebpackupdate fits the chore's dependency-refresh scope.packages/web-ui-registration/package.json (1)
37-43: LGTM!Storybook patch version bump (^8.6.17 → ^8.6.18) is consistent across all related packages, and the caret ranges are appropriate for devDependencies. This aligns well with the broader dependency deduplication effort across the monorepo.
Also applies to: 56-56
ee/packages/pdf-worker/package.json (1)
34-37: LGTM!All Storybook-related devDependencies are consistently bumped to ^8.6.18. This patch version update aligns with the PR's objective of updating dependencies and maintains version consistency across the monorepo. Based on learnings, isolating dependency bumps into a dedicated chore PR (as done here) is the preferred approach.
Also applies to: 49-49
packages/mock-providers/package.json (1)
20-20: Version alignment looks good.Keeping
@storybook/reactandstorybookon the same patch here is the right move and helps avoid mixed Storybook installs in this workspace.Also applies to: 40-40
packages/ui-voip/package.json (1)
46-54: LGTM - Storybook packages consistently updated to 8.6.18.All Storybook-related devDependencies are aligned to the same patch version, maintaining ecosystem consistency.
Also applies to: 71-71
packages/ui-client/package.json (1)
43-44: LGTM - Storybook core packages updated to 8.6.18.Updates are consistent with other packages in this PR.
Also applies to: 61-61
packages/ui-video-conf/package.json (2)
34-41: LGTM - Storybook packages updated to 8.6.18.Consistent with other packages in this PR.
Also applies to: 52-52
54-54: Webpack 5.104.0 is available and safe to use with no breaking changes.apps/meteor/package.json (3)
323-329: LGTM - Storybook packages updated to 8.6.18.Consistent with all other packages in the monorepo.
Also applies to: 439-439
448-448: Both npm package versions exist and are available.tsx version 4.21.0 and webpack version 5.104.0 are valid releases on the npm registry.
300-300: underscore 1.13.8 properly addresses CVE-2026-27601.Verified: underscore 1.13.8 (released Feb 19, 2026) fixes CVE-2026-27601, which addresses unbounded recursion vulnerabilities in
_.flattenand_.isEqualthat could cause denial-of-service attacks on deeply nested data structures. The version bump achieves the PR's CVE prevention objective.
cardoso
left a comment
cardoso
left a comment
There was a problem hiding this comment.
Removed as much duplicate dependencies as possible without touching code.
storybook packages and url-loader in livechat still pull in older versions of some packages.
julio-rocketchat
changed the title
vite, webpack, underscorevite, webpack, underscore, google-storage and is-svg
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/services/image/service.ts">
<violation number="1" location="apps/meteor/server/services/image/service.ts:8">
P1: The `file-type` import in `determineFileType.js` is incompatible with file-type v16+ API changes</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| import ExifTransformer from 'exif-be-gone'; | ||
| import ft from 'file-type'; | ||
| import isSvg from 'is-svg'; | ||
| import sharp from 'sharp'; |
There was a problem hiding this comment.
P1: The file-type import in determineFileType.js is incompatible with file-type v16+ API changes
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/services/image/service.ts, line 8:
<comment>The `file-type` import in `determineFileType.js` is incompatible with file-type v16+ API changes</comment>
<file context>
@@ -5,8 +5,6 @@ import { ServiceClassInternal } from '@rocket.chat/core-services';
import ExifTransformer from 'exif-be-gone';
-import ft from 'file-type';
-import isSvg from 'is-svg';
import sharp from 'sharp';
export class MediaService extends ServiceClassInternal implements IMediaService {
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/meteor/app/webdav/server/methods/getWebdavFilePreview.ts (1)
48-48: Drop the inline implementation comment.The code is already clear, and implementation files here should stay comment-free.
As per coding guidelines,
**/*.{ts,tsx,js}: Avoid code comments in the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/webdav/server/methods/getWebdavFilePreview.ts` at line 48, Remove the inline implementation comment "// ✅ Use the Fetch API method to extract the array buffer" from the getWebdavFilePreview implementation; locate the comment inside the getWebdavFilePreview function in getWebdavFilePreview.ts and delete that single-line comment so the file contains no inline implementation comments per the coding guidelines.apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts (1)
4-4: Remove the new implementation comments.They restate what the code already shows and violate the TS/JS guideline for implementation files.
As per coding guidelines,
**/*.{ts,tsx,js}: Avoid code comments in the implementation.Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts` at line 4, Remove the implementation comments that restate code behavior — specifically delete the line containing "// Type imports are completely safe, they disappear when compiled." and the other similar comment at the second occurrence (around the referenced location) in webdavClientAdapter.ts so the file contains no explanatory implementation comments per TS/JS guideline; leave only necessary code and type imports.apps/meteor/server/services/image/service.ts (1)
81-85: Remove the inline migration notes from the implementation.These comments add noise to a straightforward code path and violate the repository rule for TS implementation files.
✂️ Suggested cleanup
async isImage(buff: Buffer): Promise<boolean> { - // ✅ Dynamically import the ESM module const { fromBuffer } = await import('file-type'); - - // ✅ Safely handle the v17+ method rename (fromBuffer -> fileTypeFromBuffer) const data = await fromBuffer(buff);As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/image/service.ts` around lines 81 - 85, Remove the inline migration/annotation comments around the dynamic import and usage of file-type in service.ts; locate the block that dynamically imports 'file-type' and calls fromBuffer(buff) (symbols: fromBuffer, fileType, buff, data) and delete the explanatory comments so the code contains only the import and the data = await fromBuffer(buff) call, keeping behavior unchanged.
🤖 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/meteor/app/webdav/server/lib/webdavClientAdapter.ts`:
- Around line 103-123: createWriteStream currently returns a PassThrough (ws)
whose finish is independent of the remote HTTP request and which buffers writes
before getClient() resolves; change it so the returned Writable's completion and
errors are driven by the actual client.customRequest() lifecycle: create a local
PassThrough source (or temporary Writable) but do NOT treat its 'finish' as
completion—once getClient() resolves, call client.customRequest(...) to obtain
the request/response stream, pipe the PassThrough into that request using
stream.pipeline (or equivalent) and forward request/response 'finish'/'end' to
the returned stream, and forward any client or request errors to the returned
stream; ensure errors from getClient() are emitted on the returned stream and
that backpressure is applied by piping the returned Writable into the real
request stream so writes before getClient() do not unboundedly buffer. Tie all
handlers to createWriteStream, getClient, customRequest, ws and use
stream.pipeline to manage closure and error propagation.
- Around line 89-100: The createReadStream implementation can emit errors on the
PassThrough (pt) before callers attach listeners, causing unhandled exceptions;
update the error emissions from both the getClient() rejection and the
client.createReadStream error handler to defer emission to the next tick so
listeners can be attached first. Specifically, in createReadStream, replace
direct pt.emit('error', err) in the .catch(...) and in rs.on('error', ...) with
process.nextTick(() => pt.emit('error', err)) (or setImmediate if preferred) so
errors are emitted asynchronously; keep the existing getClient(),
client.createReadStream(...) and rs.pipe(pt) logic and return pt as before.
---
Nitpick comments:
In `@apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts`:
- Line 4: Remove the implementation comments that restate code behavior —
specifically delete the line containing "// Type imports are completely safe,
they disappear when compiled." and the other similar comment at the second
occurrence (around the referenced location) in webdavClientAdapter.ts so the
file contains no explanatory implementation comments per TS/JS guideline; leave
only necessary code and type imports.
In `@apps/meteor/app/webdav/server/methods/getWebdavFilePreview.ts`:
- Line 48: Remove the inline implementation comment "// ✅ Use the Fetch API
method to extract the array buffer" from the getWebdavFilePreview
implementation; locate the comment inside the getWebdavFilePreview function in
getWebdavFilePreview.ts and delete that single-line comment so the file contains
no inline implementation comments per the coding guidelines.
In `@apps/meteor/server/services/image/service.ts`:
- Around line 81-85: Remove the inline migration/annotation comments around the
dynamic import and usage of file-type in service.ts; locate the block that
dynamically imports 'file-type' and calls fromBuffer(buff) (symbols: fromBuffer,
fileType, buff, data) and delete the explanatory comments so the code contains
only the import and the data = await fromBuffer(buff) call, keeping behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bda99dce-1d80-46f1-b27e-8dea1ebd5caf
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
apps/meteor/app/webdav/server/lib/webdavClientAdapter.tsapps/meteor/app/webdav/server/methods/getWebdavFilePreview.tsapps/meteor/package.jsonapps/meteor/server/services/image/service.ts
📜 Review details
⏰ 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). (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/services/image/service.tsapps/meteor/app/webdav/server/methods/getWebdavFilePreview.tsapps/meteor/app/webdav/server/lib/webdavClientAdapter.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/services/image/service.tsapps/meteor/app/webdav/server/methods/getWebdavFilePreview.tsapps/meteor/app/webdav/server/lib/webdavClientAdapter.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/services/image/service.tsapps/meteor/app/webdav/server/methods/getWebdavFilePreview.tsapps/meteor/app/webdav/server/lib/webdavClientAdapter.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/webdav/server/methods/getWebdavFilePreview.tsapps/meteor/app/webdav/server/lib/webdavClientAdapter.ts
📚 Learning: 2026-02-10T16:32:49.806Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:49.806Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON which automatically deserializes Date objects. Stream handlers (e.g., sdk.stream()) receive Date fields as Date objects directly and do not require manual conversion using `new Date()`. Only REST API responses require manual date conversion because they return plain JSON where dates are serialized as strings.
Applied to files:
apps/meteor/app/webdav/server/methods/getWebdavFilePreview.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/package.json
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/package.json
🔇 Additional comments (1)
apps/meteor/package.json (1)
302-302: No breaking changes detected for webdav v5 upgrade. All APIs used in the codebase—createClient,stat,createDirectory,deleteFile,getFileContents,getDirectoryContents,putFileContents,createReadStream,createWriteStream, andcustomRequest—are either standard WebDAV operations or fully documented in v5 with no breaking changes. The code uses dynamic ESM import (compatible with Node.js), contains no Axios-specific options, and only uses stream methods server-side. The bump is safe to merge.
| createReadStream(path: string, options?: Record<string, any>): Readable { | ||
| return this._client.createReadStream(path, options); | ||
| const pt = new stream.PassThrough(); | ||
|
|
||
| this.getClient() | ||
| .then((client) => { | ||
| const rs = client.createReadStream(path, options); | ||
| rs.pipe(pt); | ||
| rs.on('error', (err) => pt.emit('error', err)); | ||
| }) | ||
| .catch((err) => pt.emit('error', err)); | ||
|
|
||
| return pt; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
sed -n '85,105p' apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts
printf '\n---\n'
head -n 30 apps/meteor/app/file-upload/server/config/Webdav.tsRepository: RocketChat/Rocket.Chat
Length of output: 1542
🏁 Script executed:
# Check if there are other callers of createReadStream and how they handle errors
rg -A 5 -B 2 "createReadStream" --type ts apps/meteor/app/file-upload/Repository: RocketChat/Rocket.Chat
Length of output: 1746
🏁 Script executed:
sed -n '1,80p' apps/meteor/app/file-upload/ufs/Webdav/server.tsRepository: RocketChat/Rocket.Chat
Length of output: 2228
🏁 Script executed:
sed -n '80,130p' apps/meteor/app/file-upload/ufs/Webdav/server.tsRepository: RocketChat/Rocket.Chat
Length of output: 1129
🏁 Script executed:
sed -n '1,50p' apps/meteor/app/file-upload/server/config/Webdav.tsRepository: RocketChat/Rocket.Chat
Length of output: 1437
🏁 Script executed:
# Check if getReadStream is always awaited by callers
rg -B 3 "getReadStream" apps/meteor/app/file-upload/server/config/ -A 1Repository: RocketChat/Rocket.Chat
Length of output: 3529
🏁 Script executed:
# Check if PassThrough error handlers are pre-attached elsewhere
rg -B 5 -A 5 "pt.*on.*error" apps/meteor/app/webdav/Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Understand the stream event emission timing - check if there's any setup
sed -n '89,105p' apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts | cat -n
printf '\n---\n'
# Check getClient implementation
rg -A 10 "getClient()" apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1321
🏁 Script executed:
# Check if webdav module is cached or if it's lazy-loaded
rg "import.*webdav" apps/meteor/ | head -20Repository: RocketChat/Rocket.Chat
Length of output: 2812
🏁 Script executed:
# Let's trace the exact timing - check if getClient rejection can happen synchronously
sed -n '40,65p' apps/meteor/app/webdav/server/lib/webdavClientAdapter.tsRepository: RocketChat/Rocket.Chat
Length of output: 951
🏁 Script executed:
# Verify if error events on PassThrough with no listeners cause unhandled errors
cat > /tmp/test_stream.js << 'EOF'
const stream = require('stream');
const pt = new stream.PassThrough();
// Simulate what happens if error is emitted before listener
setTimeout(() => {
console.log('Emitting error on PassThrough with no listeners...');
try {
pt.emit('error', new Error('Test error'));
} catch (e) {
console.log('Error caught:', e.message);
}
}, 0);
// Simulating caller would attach listener after
setTimeout(() => {
console.log('Attaching error listener');
pt.on('error', (err) => {
console.log('Listener caught error:', err.message);
});
}, 100);
// Show if unhandled rejection warning occurs
process.on('uncaughtException', (err) => {
console.log('UNCAUGHT:', err.message);
});
EOF
node /tmp/test_stream.jsRepository: RocketChat/Rocket.Chat
Length of output: 166
🏁 Script executed:
# Test the exact race condition scenario
cat > /tmp/test_race.js << 'EOF'
const stream = require('stream');
async function testRaceCondition() {
const pt = new stream.PassThrough();
// Simulate getClient() that rejects
Promise.resolve()
.then(() => {
throw new Error('getClient failed');
})
.catch((err) => {
console.log('About to emit error on stream with listeners:', pt.listenerCount('error'));
pt.emit('error', err);
});
// This is what the caller does - they get the stream and THEN attach listeners
const stream_returned = pt;
// Small delay to simulate real code execution
await new Promise(resolve => setImmediate(resolve));
console.log('Attaching error listener');
stream_returned.on('error', (err) => {
console.log('Listener caught error:', err.message);
});
}
process.on('error', (err) => {
console.log('PROCESS ERROR:', err.message);
});
testRaceCondition().catch(e => console.log('Uncaught:', e.message));
// Wait to see if unhandled error occurs
setTimeout(() => process.exit(0), 500);
EOF
node /tmp/test_race.js 2>&1Repository: RocketChat/Rocket.Chat
Length of output: 343
🏁 Script executed:
# Test the proposed fix with setImmediate and destroy
cat > /tmp/test_fix.js << 'EOF'
const stream = require('stream');
async function testFixedVersion() {
const pt = new stream.PassThrough();
// Proposed fix: use setImmediate to defer error emission
setImmediate(() => {
Promise.resolve()
.then(() => {
throw new Error('getClient failed');
})
.catch((err) => {
console.log('About to call destroy with listeners:', pt.listenerCount('error'));
pt.destroy(err);
});
});
const stream_returned = pt;
// Caller attaches listener - now with setImmediate, this happens first
console.log('Attaching error listener immediately');
stream_returned.on('error', (err) => {
console.log('Listener caught error:', err.message);
});
// Wait for async operations
await new Promise(resolve => setTimeout(resolve, 100));
console.log('Test completed successfully');
}
testFixedVersion().catch(e => console.log('Error:', e.message));
EOF
node /tmp/test_fix.js 2>&1Repository: RocketChat/Rocket.Chat
Length of output: 210
🏁 Script executed:
# Verify that in the actual code flow, callers DO attach listeners after getting stream
sed -n '8,20p' apps/meteor/app/file-upload/server/config/Webdav.tsRepository: RocketChat/Rocket.Chat
Length of output: 430
Don't emit read-stream errors before callers can subscribe.
createReadStream returns the PassThrough stream immediately while the promise chain executes asynchronously. If getClient() rejects before the caller attaches .on('error'), the error is emitted with zero listeners, causing an unhandled stream error that crashes the process.
Suggested fix
createReadStream(path: string, options?: Record<string, any>): Readable {
const pt = new stream.PassThrough();
- this.getClient()
- .then((client) => {
- const rs = client.createReadStream(path, options);
- rs.pipe(pt);
- rs.on('error', (err) => pt.emit('error', err));
- })
- .catch((err) => pt.emit('error', err));
+ setImmediate(() => {
+ void this.getClient()
+ .then((client) => {
+ const rs = client.createReadStream(path, options);
+ rs.on('error', (err) => pt.destroy(err));
+ rs.pipe(pt);
+ })
+ .catch((err) => pt.destroy(err));
+ });
return pt;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| createReadStream(path: string, options?: Record<string, any>): Readable { | |
| return this._client.createReadStream(path, options); | |
| const pt = new stream.PassThrough(); | |
| this.getClient() | |
| .then((client) => { | |
| const rs = client.createReadStream(path, options); | |
| rs.pipe(pt); | |
| rs.on('error', (err) => pt.emit('error', err)); | |
| }) | |
| .catch((err) => pt.emit('error', err)); | |
| return pt; | |
| createReadStream(path: string, options?: Record<string, any>): Readable { | |
| const pt = new stream.PassThrough(); | |
| setImmediate(() => { | |
| void this.getClient() | |
| .then((client) => { | |
| const rs = client.createReadStream(path, options); | |
| rs.on('error', (err) => pt.destroy(err)); | |
| rs.pipe(pt); | |
| }) | |
| .catch((err) => pt.destroy(err)); | |
| }); | |
| return pt; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts` around lines 89 -
100, The createReadStream implementation can emit errors on the PassThrough (pt)
before callers attach listeners, causing unhandled exceptions; update the error
emissions from both the getClient() rejection and the client.createReadStream
error handler to defer emission to the next tick so listeners can be attached
first. Specifically, in createReadStream, replace direct pt.emit('error', err)
in the .catch(...) and in rs.on('error', ...) with process.nextTick(() =>
pt.emit('error', err)) (or setImmediate if preferred) so errors are emitted
asynchronously; keep the existing getClient(), client.createReadStream(...) and
rs.pipe(pt) logic and return pt as before.
| createWriteStream(path: string, fileSize: number): Writable { | ||
| const ws = new stream.PassThrough(); | ||
|
|
||
| this._client | ||
| .customRequest(path, { | ||
| method: 'PUT', | ||
| headers: { | ||
| ...(fileSize ? { 'Content-Length': String(fileSize) } : {}), | ||
| }, | ||
| data: ws, | ||
| maxRedirects: 0, | ||
| this.getClient() | ||
| .then((client) => { | ||
| client | ||
| .customRequest(path, { | ||
| method: 'PUT', | ||
| headers: { | ||
| ...(fileSize ? { 'Content-Length': String(fileSize) } : {}), | ||
| }, | ||
| data: ws, | ||
| maxRedirects: 0, | ||
| }) | ||
| .catch((err) => { | ||
| ws.emit('error', err); | ||
| }); | ||
| }) | ||
| .catch((err) => { | ||
| ws.emit('error', err); | ||
| }); | ||
| .catch((err) => ws.emit('error', err)); | ||
|
|
||
| return ws; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual implementation in webdavClientAdapter.ts
cat -n apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts | sed -n '100,130p'Repository: RocketChat/Rocket.Chat
Length of output: 739
🏁 Script executed:
# Check how the stream is used in Webdav/server.ts
cat -n apps/meteor/app/file-upload/ufs/Webdav/server.ts | sed -n '125,155p'Repository: RocketChat/Rocket.Chat
Length of output: 1169
🏁 Script executed:
# Also check the complete createWriteStream method to understand context
cat -n apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts | sed -n '103,130p'Repository: RocketChat/Rocket.Chat
Length of output: 701
Fix the stream lifecycle mismatch in createWriteStream.
The method returns a PassThrough before the underlying customRequest() is initialized, causing two problems:
-
The
finishevent fires when input stops, not when the HTTP request completes. This is evidenced by the 500ms timeout hack inapps/meteor/app/file-upload/ufs/Webdav/server.ts:135-145with the TODO comment "remove timeout when UploadFS bug resolved." -
Data written before
getClient()resolves gets buffered in memory instead of being backpressured to the request.
Return a Writable whose completion and error lifecycle is driven by the actual customRequest(), not a local PassThrough.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/webdav/server/lib/webdavClientAdapter.ts` around lines 103 -
123, createWriteStream currently returns a PassThrough (ws) whose finish is
independent of the remote HTTP request and which buffers writes before
getClient() resolves; change it so the returned Writable's completion and errors
are driven by the actual client.customRequest() lifecycle: create a local
PassThrough source (or temporary Writable) but do NOT treat its 'finish' as
completion—once getClient() resolves, call client.customRequest(...) to obtain
the request/response stream, pipe the PassThrough into that request using
stream.pipeline (or equivalent) and forward request/response 'finish'/'end' to
the returned stream, and forward any client or request errors to the returned
stream; ensure errors from getClient() are emitted on the returned stream and
that backpressure is applied by piping the returned Writable into the real
request stream so writes before getClient() do not unboundedly buffer. Tie all
handlers to createWriteStream, getClient, customRequest, ws and use
stream.pipeline to manage closure and error propagation.
| async isImage(buff: Buffer): Promise<boolean> { | ||
| const data = await ft.fromBuffer(buff); | ||
| // ✅ Dynamically import the ESM module | ||
| const { fromBuffer } = await import('file-type'); |
There was a problem hiding this comment.
why are we doing dynamic imports?
There was a problem hiding this comment.
Cardoso was trying to do it this way to be able to upgrade webdav. If this is a pattern we wouln't like to have, I can remove it from the PR
d-gubert
left a comment
d-gubert
left a comment
There was a problem hiding this comment.
All good from apps-engine side
Proposed changes (including videos or screenshots)
Bump
vite,webpack,underscore,google-storageandis-svgto ensure they're running more up-to-date versions and prevent CVEs.Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit