Draft#1928
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughIn Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
sagzy
force-pushed
the
improve-undo-handler
branch
2 times, most recently
from
e9bb063 to
0460e30
Compare
![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
🧹 Nitpick comments (2)
src/dispatchers.unit.test.ts (2)
1340-1343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an interface for the mock shape.
This inline object type defines a reusable object shape; moving it to an interface keeps the test aligned with the TypeScript guideline.
♻️ Proposed refactor
+interface MockGlobalDb { + get: ReturnType<typeof vi.fn>; + set: ReturnType<typeof vi.fn>; +} + describe('createUndoHandler', () => { @@ - let mockGlobalDb: { - get: ReturnType<typeof vi.fn>; - set: ReturnType<typeof vi.fn>; - }; + let mockGlobalDb: MockGlobalDb;As per coding guidelines,
**/*.{ts,tsx}: "Preferinterfacefor defining object shapes in TypeScript."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dispatchers.unit.test.ts` around lines 1340 - 1343, The mock shape for mockGlobalDb is currently defined inline as an object type, which should be moved to an interface to match the TypeScript style guideline. Define a reusable interface for this shape near the test helpers or alongside the relevant setup in dispatchers.unit.test.ts, then update mockGlobalDb to use that interface while keeping the same get and set vi.fn members.Source: Coding guidelines
1384-1391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid AP ID string comparisons in the mock implementation.
The mock branches on AP IDs with direct
===checks. Prefer ordered mock responses plus explicit call assertions, or the same AP-ID helper used by production code.♻️ Proposed refactor
- vi.mocked( - mockAccountService.getAccountByApId, - ).mockImplementation(async (apId: string) => { - if (apId === aliceUrl.href) { - return { id: 10 } as AccountType; - } - if (apId === ghostUserUrl.href) { - return { id: 1 } as AccountType; - } - return null; - }); + vi.mocked(mockAccountService.getAccountByApId) + .mockResolvedValueOnce({ id: 10 } as AccountType) + .mockResolvedValueOnce({ id: 1 } as AccountType);As per coding guidelines,
src/**/*.ts: "Never use direct string comparisons for ActivityPub IDs."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dispatchers.unit.test.ts` around lines 1384 - 1391, The mock in the test setup is using direct ActivityPub ID string comparisons inside the mocked lookup, which violates the AP ID guideline. Refactor the mock around the affected lookup helper (the mocked function in src/dispatchers.unit.test.ts) to avoid branching on apId with direct === checks; instead use ordered mock return values or assert the expected call sequence explicitly, and if you need to distinguish IDs, rely on the same AP-ID helper used by production code rather than raw string equality.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dispatchers.ts`:
- Line 511: The Undo actor validation in dispatchers should use URI
normalization instead of comparing .href directly. Update the actor checks in
the Undo handling logic around the undo.actorId and follow.actorId comparison to
use isEqual() from src/helpers/uri.ts, keeping the existing guard behavior but
making the URI comparison consistent with the rest of the codebase.
---
Nitpick comments:
In `@src/dispatchers.unit.test.ts`:
- Around line 1340-1343: The mock shape for mockGlobalDb is currently defined
inline as an object type, which should be moved to an interface to match the
TypeScript style guideline. Define a reusable interface for this shape near the
test helpers or alongside the relevant setup in dispatchers.unit.test.ts, then
update mockGlobalDb to use that interface while keeping the same get and set
vi.fn members.
- Around line 1384-1391: The mock in the test setup is using direct ActivityPub
ID string comparisons inside the mocked lookup, which violates the AP ID
guideline. Refactor the mock around the affected lookup helper (the mocked
function in src/dispatchers.unit.test.ts) to avoid branching on apId with direct
=== checks; instead use ordered mock return values or assert the expected call
sequence explicitly, and if you need to distinguish IDs, rely on the same AP-ID
helper used by production code rather than raw string equality.
🪄 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: d9d85f5f-02bc-4e52-9d8e-460c10db500e
📒 Files selected for processing (2)
src/dispatchers.tssrc/dispatchers.unit.test.ts
sagzy
force-pushed
the
improve-undo-handler
branch
from
0460e30 to
3a0bc62
Compare
sagzy
force-pushed
the
improve-undo-handler
branch
from
3a0bc62 to
d7f80ca
Compare
1 participant
No description provided.