chore: Introduce middleware support for function patching #37560
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 |
|
WalkthroughThe changes replace an internal patch-tracking mechanism with a middleware-based approach for the patch injection system. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PatchedFn as Patched Function
participant Middleware as Middleware Chain
participant OriginalFn as Original Function
Client->>PatchedFn: Call patched function(args)
PatchedFn->>Middleware: Execute middleware runner
loop For each middleware
Middleware->>Middleware: Transform context
end
Middleware->>OriginalFn: Invoke with final context
OriginalFn-->>Middleware: Return result
Middleware-->>PatchedFn: Return result
PatchedFn-->>Client: Return result
Client->>PatchedFn: patch(fn, condition?)
PatchedFn->>Middleware: Register middleware via use()
Middleware-->>PatchedFn: Return disposer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
📦 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/18 22:53", "11/19 23:02", "11/21 16:49", "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", "01/28 14:42 (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.00]
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.00]
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.00]
line "omnichannel-transcript-service" [0.14, 0.14, 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.00]
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.00]
line "queue-worker-service" [0.14, 0.14, 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.00]
line "rocketchat" [0.35, 0.35, 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.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 28 Jan 2026 14:42:26 GMT |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37560 +/- ##
===========================================
- Coverage 70.79% 70.76% -0.04%
===========================================
Files 3158 3156 -2
Lines 109390 109370 -20
Branches 19682 20069 +387
===========================================
- Hits 77445 77398 -47
- Misses 29919 29944 +25
- Partials 2026 2028 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/middleware
branch
from
baa0b03 to
a74e100
Compare
ggazzo
force-pushed
the
chore/middleware
branch
from
a74e100 to
3f5c94d
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🤖 Fix all issues with AI agents
In `@packages/patch-injection/src/midleware.ts`:
- Around line 18-25: The disposer currently uses middlewares.indexOf(middleware)
which will remove the first matching function when the same middleware is
registered multiple times; change use so each registration gets a unique entry
(e.g., wrap the provided middleware in a per-registration object/wrapper or
generate a token) and push that unique entry into middlewares (use the unique
wrapper/token when calling buildRunner); have the returned disposer remove that
exact entry (indexOf(wrapper/token)) so removals target the correct registration
and then rebuild runner with buildRunner().
🧹 Nitpick comments (2)
packages/patch-injection/src/index.ts (1)
1-4: Remove commented-out exports and confirm the API change.Line 1 and Line 4 leave commented exports in production code. If
definitiontypes are still public, re-export them explicitly; otherwise delete the commented lines and document the breaking change for consumers.♻️ Suggested cleanup
-// export * from './definition'; export * from './makeFunction'; export * from './midleware'; -// export * from './addPatch';As per coding guidelines Avoid code comments in the implementation.
packages/patch-injection/src/midleware.ts (1)
7-13: Confirm middleware execution order (currently last-registered runs first).Line 8 composes with
reduce, which makes the most recently added middleware execute first. If you want registration order to run first (common middleware behavior), switch toreduceRightor reverse the array.♻️ Option to preserve registration order
- return middlewares.reduce( + return middlewares.reduceRight(
| const use = (middleware: Middleware<F>) => { | ||
| middlewares.push(middleware); | ||
|
|
||
| runner = buildRunner(); | ||
| return () => { | ||
| const index = middlewares.indexOf(middleware); | ||
| if (index > -1) { | ||
| middlewares.splice(index, 1); |
There was a problem hiding this comment.
Disposer can remove the wrong middleware when the same function is registered twice.
Line 23 uses indexOf(middleware). If the same middleware is registered multiple times, the disposer for the later registration removes the first entry. Wrap the middleware per registration (or store an entry token) so disposal removes the correct instance.
🐛 Ensure disposer targets the exact registration
const use = (middleware: Middleware<F>) => {
- middlewares.push(middleware);
+ const entry: Middleware<F> = (ctx, next) => middleware(ctx, next);
+ middlewares.push(entry);
runner = buildRunner();
return () => {
- const index = middlewares.indexOf(middleware);
+ const index = middlewares.indexOf(entry);
if (index > -1) {
middlewares.splice(index, 1);
}📝 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.
| const use = (middleware: Middleware<F>) => { | |
| middlewares.push(middleware); | |
| runner = buildRunner(); | |
| return () => { | |
| const index = middlewares.indexOf(middleware); | |
| if (index > -1) { | |
| middlewares.splice(index, 1); | |
| const use = (middleware: Middleware<F>) => { | |
| const entry: Middleware<F> = (ctx, next) => middleware(ctx, next); | |
| middlewares.push(entry); | |
| runner = buildRunner(); | |
| return () => { | |
| const index = middlewares.indexOf(entry); | |
| if (index > -1) { | |
| middlewares.splice(index, 1); |
🤖 Prompt for AI Agents
In `@packages/patch-injection/src/midleware.ts` around lines 18 - 25, The disposer
currently uses middlewares.indexOf(middleware) which will remove the first
matching function when the same middleware is registered multiple times; change
use so each registration gets a unique entry (e.g., wrap the provided middleware
in a per-registration object/wrapper or generate a token) and push that unique
entry into middlewares (use the unique wrapper/token when calling buildRunner);
have the returned disposer remove that exact entry (indexOf(wrapper/token)) so
removals target the correct registration and then rebuild runner with
buildRunner().
1 participant
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.