Conversation
WalkthroughThe PR refactors the AI moderation system from EventEmitter-based event emission to an internal Awaiter pattern, removes EventEmitter inheritance from AIModeration, comments out AI moderation initialization in AutoModerationStack, and adds comprehensive test coverage for the Awaiter utility. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
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)
src/middlewares/auto-moderation-stack/ai-moderation.ts (1)
103-114:⚠️ Potential issue | 🟠 MajorRace condition: awaiter reference is captured before replacement.
The logic here has a race condition that will cause all moderation checks to return
null:
- Line 104 awaits
this.responseAwaiter(already resolved from constructor or previous batch)- Line 113 returns a promise from the same
this.responseAwaiterreference- Since this awaiter is already resolved (with
[]or previous results),.then()fires immediately with stale data, returningnull- When
triggerCheckruns 10 seconds later, it creates a new awaiter—but no one is waiting on itThe fundamental issue is that candidates capture a reference to the awaiter before
triggerCheckreplaces it with a new one.Since AI moderation is currently disabled in
index.ts, this won't manifest, but must be fixed before re-enabling.Possible fix approach
One approach: create the new awaiter when the first item of a batch arrives, not in
triggerCheck:private async addToCheckQueue(candidate: ModerationCandidate): Promise<ModerationResult | null> { await this.responseAwaiter // wait for the previous batch to be processed + + // Start a new batch if this is the first item + if (this.checkQueue.length === 0) { + this.responseAwaiter = new Awaiter() + } + const batchAwaiter = this.responseAwaiter // capture reference for this batch + const index = this.checkQueue.push(candidate) - 1 if (this.timeout === null) { this.timeout = setTimeout(() => { this.triggerCheck() this.timeout = null }, 10 * 1000) } - return this.responseAwaiter.then((results) => results[index] ?? null) + return batchAwaiter.then((results) => results[index] ?? null) }And remove line 84 from
triggerCheck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middlewares/auto-moderation-stack/ai-moderation.ts` around lines 103 - 114, The current addToCheckQueue captures the already-resolved this.responseAwaiter causing callers to receive stale results; fix by creating a new awaiter when starting a new batch (i.e., when checkQueue is empty) inside addToCheckQueue, assign it to this.responseAwaiter and also capture it into a local variable (e.g., localAwaiter) before pushing the candidate, then use localAwaiter.then(...) to return that caller's result; update the timeout logic as-is, and remove the code that creates/replaces the awaiter inside triggerCheck so triggerCheck only consumes the current checkQueue and resolves the existing awaiter.
🧹 Nitpick comments (2)
src/middlewares/auto-moderation-stack/index.ts (1)
10-10: Consider deleting commented-out code instead of keeping it.The PR title indicates "remove AI" but the code is commented out rather than deleted. Commented-out code adds maintenance burden and clutters the codebase. Since Git preserves history, the code can be recovered if needed later.
If this is intentional for quick re-enablement, consider creating a feature flag instead, or at minimum adding a comment explaining why the code is preserved.
Also applies to: 17-17, 52-52, 78-78, 171-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middlewares/auto-moderation-stack/index.ts` at line 10, Remove the commented-out code fragments (e.g., the commented import "defer" and the commented blocks at the other locations) rather than leaving them in place; either fully delete those commented lines from the auto-moderation-stack module or, if they must be re-enabled conditionally, convert them into a proper feature flag/conditional (not commented code) or add a concise comment explaining why they are preserved for quick re-enablement. Specifically target the commented import of defer and the commented blocks around the ranges you were called out (including the block that mentions defer and the larger commented region), ensuring no leftover commented implementation remains in functions/classes in this module so history in VCS can be used to restore if needed.tests/awaiter.test.ts (1)
51-57: Timing assertions may be flaky in CI environments.The
expect(elapsed).toBeLessThan(2)assertions (also at line 147) could fail intermittently on slow CI runners or under load. Consider either increasing the threshold or using a mock timer approach withvi.useFakeTimers().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/awaiter.test.ts` around lines 51 - 57, The timing assertion around awaiting the promise (the expect(elapsed).toBeLessThan(2) check for the local variable elapsed after awaiting awaiter) is too strict and can flake on CI; either relax the threshold (e.g., to toBeLessThan(20) or another reasonable ms) or switch the test to use mocked timers: call vi.useFakeTimers() before creating/awaiting awaiter, advance timers appropriately (vi.advanceTimersToNextTimer() / vi.runAllTimers()) so the await resolves deterministically, then call vi.useRealTimers() afterward; update both occurrences of expect(elapsed).toBeLessThan(2) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/middlewares/auto-moderation-stack/ai-moderation.ts`:
- Around line 65-69: The constructor currently pre-resolves the Awaiter instance
(responseAwaiter.resolve([])), causing immediate resolution and a race where the
first batch sees an already-resolved awaiter; remove that pre-resolution and
instead initialize responseAwaiter as a fresh unresolved Awaiter (or null) in
the constructor, then create/assign a new Awaiter when batching/queuing
candidates (the code paths that call await this.responseAwaiter should ensure an
Awaiter exists before awaiting), and only call responseAwaiter.resolve(results)
when the moderation results are actually available (update any methods that
consume or reset responseAwaiter accordingly) so the first batch waits
correctly.
---
Outside diff comments:
In `@src/middlewares/auto-moderation-stack/ai-moderation.ts`:
- Around line 103-114: The current addToCheckQueue captures the already-resolved
this.responseAwaiter causing callers to receive stale results; fix by creating a
new awaiter when starting a new batch (i.e., when checkQueue is empty) inside
addToCheckQueue, assign it to this.responseAwaiter and also capture it into a
local variable (e.g., localAwaiter) before pushing the candidate, then use
localAwaiter.then(...) to return that caller's result; update the timeout logic
as-is, and remove the code that creates/replaces the awaiter inside triggerCheck
so triggerCheck only consumes the current checkQueue and resolves the existing
awaiter.
---
Nitpick comments:
In `@src/middlewares/auto-moderation-stack/index.ts`:
- Line 10: Remove the commented-out code fragments (e.g., the commented import
"defer" and the commented blocks at the other locations) rather than leaving
them in place; either fully delete those commented lines from the
auto-moderation-stack module or, if they must be re-enabled conditionally,
convert them into a proper feature flag/conditional (not commented code) or add
a concise comment explaining why they are preserved for quick re-enablement.
Specifically target the commented import of defer and the commented blocks
around the ranges you were called out (including the block that mentions defer
and the larger commented region), ensuring no leftover commented implementation
remains in functions/classes in this module so history in VCS can be used to
restore if needed.
In `@tests/awaiter.test.ts`:
- Around line 51-57: The timing assertion around awaiting the promise (the
expect(elapsed).toBeLessThan(2) check for the local variable elapsed after
awaiting awaiter) is too strict and can flake on CI; either relax the threshold
(e.g., to toBeLessThan(20) or another reasonable ms) or switch the test to use
mocked timers: call vi.useFakeTimers() before creating/awaiting awaiter, advance
timers appropriately (vi.advanceTimersToNextTimer() / vi.runAllTimers()) so the
await resolves deterministically, then call vi.useRealTimers() afterward; update
both occurrences of expect(elapsed).toBeLessThan(2) accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edb51963-6b6e-408c-a3bb-f2a9252f63b7
📒 Files selected for processing (3)
src/middlewares/auto-moderation-stack/ai-moderation.tssrc/middlewares/auto-moderation-stack/index.tstests/awaiter.test.ts
No description provided.