Conversation
|
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 |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughUpdated the hono dependency to version 4.11.9 across multiple packages and refactored the Router implementation to use explicit type casting for the innerRouter invocation, ensuring compatibility with the updated dependency. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
coderabbitai
bot
added
the
stat: ready to merge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/http-router/src/Router.ts (1)
190-195: The type cast is a pragmatic workaround for Hono 4.11.x's stricter type-system; narrowing it further may be impractical.Dynamic method dispatch via
this.innerRouter[method.toLowerCase()]requires a cast because TypeScript cannot infer which route-method overload to use at compile time. The cast specifically describes what's being passed—middlewares plus a handler that returnsPromise<ResponseSchema<TypedOptions>>—and is appropriately scoped. While narrower alternatives using Hono's exported types would be ideal, they would likely require either individualif/elseblocks for each HTTP method or Hono exposing a more generic handler interface, both of which have trade-offs.Also note: Line 190 uses
replace('//', '/')(first occurrence only) while Line 110 usesreplaceAll('//', '/'). This inconsistency is safe since/${subpath}can only have one leading//from the template literal.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
apps/meteor/package.jsonpackages/http-router/package.jsonpackages/http-router/src/Router.ts
🧰 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:
packages/http-router/src/Router.ts
🧠 Learnings (2)
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
packages/http-router/src/Router.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
packages/http-router/src/Router.ts
🧬 Code graph analysis (1)
packages/http-router/src/Router.ts (2)
packages/rest-typings/src/index.ts (1)
Method(118-118)apps/meteor/app/api/server/definition.ts (1)
TypedOptions(286-295)
⏰ 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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/package.json (1)
213-213: LGTM! Version bump to address CVEs, consistent with thepackages/http-router/package.jsonupdate.packages/http-router/package.json (1)
25-25: LGTM! Consistent version bump across the monorepo.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38727 +/- ##
===========================================
- Coverage 70.51% 70.51% -0.01%
===========================================
Files 3176 3176
Lines 111139 111140 +1
Branches 20050 20076 +26
===========================================
- Hits 78367 78365 -2
- Misses 30721 30728 +7
+ Partials 2051 2047 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/12 22:57", "02/13 22:38", "02/16 14:04", "02/16 17:14 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 16 Feb 2026 17:14:06 GMT |
1 participant
Proposed changes (including videos or screenshots)
Bump
honolibrary to address recent CVEs.After the upgrade, a TypeScript error appeared in Router.ts when registering routes using this.innerRouter[method]. Newer versions of Hono use stricter types for route methods, and because of this the path parameter was incorrectly treated as a handler, causing the build to fail.
To fix this, the router method is now assigned to a typed variable before being called. This makes the expected function signature explicit and allows TypeScript to resolve the types correctly.
Issue(s)
https://github.com/RocketChat/Rocket.Chat/security/dependabot/482
https://github.com/RocketChat/Rocket.Chat/security/dependabot/479
https://github.com/RocketChat/Rocket.Chat/security/dependabot/480
https://github.com/RocketChat/Rocket.Chat/security/dependabot/481
https://github.com/RocketChat/Rocket.Chat/security/dependabot/464
https://github.com/RocketChat/Rocket.Chat/security/dependabot/465
Steps to test or reproduce
N/A
Further comments
N/A
Summary by CodeRabbit
Chores
Refactor