Conversation
|
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:
📝 WalkthroughWalkthroughMinor code cleanups and typing tweaks: optional chaining in AMQP consumer, Awilix devDependency bumped to v13 across packages, test DI containers given explicit generics, Docker Compose adds RabbitMQ healthcheck and amqp start script uses --wait, and several GCP Pub/Sub tests were reformatted. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/amqp/test/utils/testContext.ts (1)
114-123:⚠️ Potential issue | 🟠 MajorRestore
lifetime: Lifetime.SINGLETONtoamqpConnectionManager.The
amqpConnectionManagerregistration is missing an explicit lifetime specification. In Awilix, the default isTRANSIENT, meaning a new instance is created per resolution. This is problematic for a connection manager withasyncInit/asyncDisposelifecycle hooks:
- Multiple AMQP connections will be created if the dependency is resolved more than once
- Resource cleanup will be incomplete since only one instance's
asyncDisposewill be properly trackedAll other similar registrations in this file (
permissionConsumer,permissionPublisher,queuePublisherManager,topicPublisherManager) explicitly specifylifetime: Lifetime.SINGLETON. This registration should too.Fix
amqpConnectionManager: asFunction( ({ logger }: Dependencies) => { return new AmqpConnectionManager(config, logger) }, { + lifetime: Lifetime.SINGLETON, asyncInit: 'init', asyncDispose: 'close', asyncDisposePriority: 1, }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/amqp/test/utils/testContext.ts` around lines 114 - 123, The amqpConnectionManager registration is missing an explicit Lifetime.SINGLETON, causing Awilix to treat it as TRANSIENT and create multiple AmqpConnectionManager instances (breaking asyncInit/asyncDispose lifecycle tracking); update the asFunction registration for amqpConnectionManager to include lifetime: Lifetime.SINGLETON (matching other registrations like permissionConsumer/permissionPublisher/queuePublisherManager/topicPublisherManager) so a single AmqpConnectionManager instance is created and its asyncInit/asyncDispose hooks are managed correctly.
🤖 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/amqp/package.json`:
- Around line 40-53: Update the package TypeScript configuration to pin
TS6-breaking defaults so builds don’t change: add or update tsconfig.json's
"compilerOptions" to explicitly set "strict" (false or your intended value),
"target" (e.g., "es2022" or your prior target), "module" (e.g., "commonjs" or
"esnext" to match runtime), "types" (include needed libs instead of empty), and
"noUncheckedSideEffectImports" (set to false if you relied on previous
behavior), and include "ignoreDeprecations": "6.0" to silence TS6 deprecation
errors; ensure the package.json's "typescript" devDependency remains as intended
and run CI/tests to verify everything passes.
---
Outside diff comments:
In `@packages/amqp/test/utils/testContext.ts`:
- Around line 114-123: The amqpConnectionManager registration is missing an
explicit Lifetime.SINGLETON, causing Awilix to treat it as TRANSIENT and create
multiple AmqpConnectionManager instances (breaking asyncInit/asyncDispose
lifecycle tracking); update the asFunction registration for
amqpConnectionManager to include lifetime: Lifetime.SINGLETON (matching other
registrations like
permissionConsumer/permissionPublisher/queuePublisherManager/topicPublisherManager)
so a single AmqpConnectionManager instance is created and its
asyncInit/asyncDispose hooks are managed correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06ffdfa1-24b5-4577-bf63-7b6a55229e3d
📒 Files selected for processing (9)
packages/amqp/lib/AbstractAmqpConsumer.tspackages/amqp/package.jsonpackages/amqp/test/utils/testContext.tspackages/core/package.jsonpackages/gcp-pubsub/package.jsonpackages/kafka/lib/utils/KafkaMessageBatchStream.tspackages/kafka/package.jsonpackages/sns/package.jsonpackages/sqs/package.json
packages/amqp/package.json
Outdated
| "@biomejs/biome": "^2.3.15", | ||
| "@lokalise/biome-config": "^3.1.0", | ||
| "@lokalise/tsconfig": "^3.0.0", | ||
| "@message-queue-toolkit/core": "*", | ||
| "@types/amqplib": "0.10.8", | ||
| "@types/node": "^25.0.2", | ||
| "@vitest/coverage-v8": "^4.0.15", | ||
| "amqplib": "^0.10.8", | ||
| "awilix": "^12.0.5", | ||
| "awilix-manager": "^6.1.0", | ||
| "rimraf": "^6.0.1", | ||
| "typescript": "^5.9.3", | ||
| "vitest": "^4.0.15", | ||
| "zod": "^4.1.13" | ||
| "@types/node": "^25.5.0", | ||
| "@types/amqplib": "^0.10.8", | ||
| "@vitest/coverage-v8": "^4.0.18", | ||
| "amqplib": "^1.0.3", | ||
| "awilix": "^13.0.3", | ||
| "awilix-manager": "^6.4.0", | ||
| "rimraf": "^6.1.3", | ||
| "typescript": "^6.0.2", | ||
| "vitest": "^4.0.18", | ||
| "zod": "^4.3.6" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any amqplib-specific type imports or usages that might be affected
rg -n --type ts "from ['\"]amqplib['\"]"Repository: kibertoad/message-queue-toolkit
Length of output: 1826
🌐 Web query:
amqplib 1.0 breaking changes from 0.10
💡 Result:
The breaking changes in amqplib 1.0.0 from 0.10.x are primarily: - Minimum Node.js requirement raised to v18 (previously supported down to v10 in 0.10.x). Other changes include: - Introduction of Biome formatter for code formatting. - Automated formatting via pre-commit hooks using Lefthook. - Formatted entire codebase with standardised rules. - Added npm format script. - Enabled and fixed lint rules: noUnusedFunctionParameters and noUnusedVariables. No API or behavioral changes to the AMQP client functionality are mentioned; the update appears to be a maintenance release focused on tooling, linting, and Node.js version support. The library remains an AMQP 0-9-1 client compatible with RabbitMQ.
Citations:
- 1: https://github.com/amqp-node/amqplib/blob/main/CHANGELOG.md
- 2: https://www.npmjs.com/package/amqplib
🌐 Web query:
awilix 13.0 breaking changes from 12.x
💡 Result:
The breaking changes in Awilix 13.0.0 from 12.x are all TypeScript-only and do not affect runtime behavior. They are: - Default cradle type changed from any to {}: createContainer without an explicit generic now returns AwilixContainer<{}> instead of AwilixContainer. This enables better type accumulation via register chaining. - register returns a new container type that includes the registered resolvers. - asValue now preserves literal types and deep readonly for objects. No migration is required for JavaScript users or runtime code. Subsequent patch releases (13.0.1-13.0.3) fixed ESM/CJS interop issues but introduced no additional breaking changes.
Citations:
- 1: https://github.com/jeffijoe/awilix/blob/master/CHANGELOG.md
- 2: https://raw.githubusercontent.com/jeffijoe/awilix/master/CHANGELOG.md
🌐 Web query:
typescript 6.0 breaking changes
💡 Result:
TypeScript 6.0, released in March 2026, is a bridge release preparing for the native Go-based TypeScript 7.0. It introduces several breaking changes, deprecations, and default shifts. Deprecations can be temporarily ignored with "ignoreDeprecations": "6.0" in tsconfig.json, but will be removed in 7.0. Key breaking changes and deprecations: New Defaults (potentially breaking if relying on old defaults): - strict: true (was false) - target: es2025 (was ES3/ES5) - module: esnext - types: [] (was ["*"], now explicitly set e.g. ["node"]) - rootDir: config directory (was inferred) - noUncheckedSideEffectImports: true - alwaysStrict assumed everywhere (alwaysStrict: false deprecated) Deprecated/Removed Options: - target: es5 (minimum now es2015) - --outFile (removed; use bundlers) - baseUrl (deprecated; migrate paths) - moduleResolution: node10/node/classic (use nodenext/bundler) - module: amd/umd/system (removed) - downlevelIteration (deprecated) - esModuleInterop/allowSyntheticDefaultImports: false (now always true) - Legacy module Foo {} syntax (use namespace) - Import assertions: assert {} (use with {}; deprecated in static/dynamic imports) - Triple-slash references /// <reference ... /> - --stableTypeOrdering flag for diagnosing 7.0 differences Behavioral Changes: - Less context-sensitivity for this-less functions (tighter inference) - Type ordering in .d.ts may differ (use --stableTypeOrdering to preview) - dom lib now includes dom.iterable/asynciterable Migration Tips: - Use experimental ts5to6 codemod for baseUrl/rootDir. - Explicitly set types array. - Set rootDir if sources are in subdir. - Update imports for new interop. - See https://aka.ms/ts6 for official migration info. These align TypeScript with modern JS ecosystem (ESM, bundlers, evergreen browsers).
Citations:
- 1: https://devblogs.microsoft.com/typescript/announcing-typescript-6-0/
- 2: TypeScript 6.0 Iteration Plan microsoft/TypeScript#63085
- 3: https://gist.github.com/privatenumber/3d2e80da28f84ee30b77d53e1693378f
- 4: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-6-0.html
- 5: http://www.typescriptlang.org/docs/handbook/release-notes/typescript-6-0.html
- 6: 6.0 Deprecation List microsoft/TypeScript#54500
- 7: 6.0 Migration Guide microsoft/TypeScript#62508
Prioritize testing TypeScript 6.0 compatibility and configuration adjustments.
While amqplib 1.0.x has no API breaking changes (only Node.js v18+ requirement), TypeScript 6.0 introduces significant breaking changes including new compiler defaults: strict: true, target: es2025, module: esnext, types: [], and noUncheckedSideEffectImports: true. These defaults could affect the build and type checking without an explicit ignoreDeprecations: "6.0" configuration.
The awilix 13.x changes are TypeScript-only (type inference changes to cradle) and don't affect runtime behavior.
Verify that the tsconfig.json explicitly sets required configuration options and that CI passes with these new compiler defaults.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/amqp/package.json` around lines 40 - 53, Update the package
TypeScript configuration to pin TS6-breaking defaults so builds don’t change:
add or update tsconfig.json's "compilerOptions" to explicitly set "strict"
(false or your intended value), "target" (e.g., "es2022" or your prior target),
"module" (e.g., "commonjs" or "esnext" to match runtime), "types" (include
needed libs instead of empty), and "noUncheckedSideEffectImports" (set to false
if you relied on previous behavior), and include "ignoreDeprecations": "6.0" to
silence TS6 deprecation errors; ensure the package.json's "typescript"
devDependency remains as intended and run CI/tests to verify everything passes.
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/gcp-pubsub/test/consumers/PubSubPermissionConsumer.subscriptionRetry.spec.ts`:
- Around line 379-381: The delayed call using globalThis.setTimeout with an
async callback calling topic.createSubscription(SUBSCRIPTION_NAME) can produce
unhandled promise rejections; update the timeout callback to handle errors
(e.g., wrap the await topic.createSubscription(...) in a try/catch or attach a
.catch handler) and log or propagate the error appropriately so any rejection
from createSubscription is handled within the
PubSubPermissionConsumer.subscriptionRetry.spec.ts test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6b85c3a-5291-408a-9ce1-f96e2ece44f0
📒 Files selected for processing (5)
docker-compose.ymlpackages/amqp/package.jsonpackages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.deduplication.spec.tspackages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.payloadOffloading.spec.tspackages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.subscriptionRetry.spec.ts
✅ Files skipped from review due to trivial changes (2)
- packages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.payloadOffloading.spec.ts
- packages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.deduplication.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/amqp/package.json
packages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.subscriptionRetry.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.subscriptionRetry.spec.ts (1)
378-383:⚠️ Potential issue | 🟠 MajorDetach-free delayed subscription creation to avoid flaky async failures.
The delayed
globalThis.setTimeouttask is not part of the test lifecycle, so failures can surface out-of-band and make this test flaky. Prefer an awaited delayed promise so errors are deterministic and cleanup-safe.Proposed fix
- // Create subscription after a delay (simulating eventual consistency) - globalThis.setTimeout(() => { - topic.createSubscription(SUBSCRIPTION_NAME).catch((err) => { - expect.unreachable(`Failed to create subscription in delayed callback: ${err}`) - }) - }, 1500) + // Create subscription after a delay (simulating eventual consistency) + const delayedSubscriptionCreation = (async () => { + await setTimeout(1500) + await topic.createSubscription(SUBSCRIPTION_NAME) + })() try { // This should retry and eventually succeed when subscription is created await consumer.start() // `@ts-expect-error` - accessing private field for testing expect(consumer.isConsuming).toBe(true) + await delayedSubscriptionCreation } finally { + await delayedSubscriptionCreation.catch(() => undefined) await consumer.close() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.subscriptionRetry.spec.ts` around lines 378 - 383, The delayed globalThis.setTimeout callback that calls topic.createSubscription(SUBSCRIPTION_NAME) should be converted into an awaited delay inside the test so failures are caught by the test runner; replace the detached setTimeout with an await new Promise(resolve => setTimeout(resolve, 1500)) (or a test util delay) followed by await topic.createSubscription(SUBSCRIPTION_NAME) wrapped in try/catch and failing the test with expect.unreachable or rethrowing the error so the error is deterministic and cleanup-safe; update any references in this spec (PubSubPermissionConsumer.subscriptionRetry.spec.ts) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.subscriptionRetry.spec.ts`:
- Around line 378-383: The delayed globalThis.setTimeout callback that calls
topic.createSubscription(SUBSCRIPTION_NAME) should be converted into an awaited
delay inside the test so failures are caught by the test runner; replace the
detached setTimeout with an await new Promise(resolve => setTimeout(resolve,
1500)) (or a test util delay) followed by await
topic.createSubscription(SUBSCRIPTION_NAME) wrapped in try/catch and failing the
test with expect.unreachable or rethrowing the error so the error is
deterministic and cleanup-safe; update any references in this spec
(PubSubPermissionConsumer.subscriptionRetry.spec.ts) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7539c4af-6fbb-410e-bf13-1830c7a574c5
📒 Files selected for processing (1)
packages/gcp-pubsub/test/consumers/PubSubPermissionConsumer.subscriptionRetry.spec.ts
1 participant
Summary by CodeRabbit
Refactor
Chores
Tests