Conversation
📝 WalkthroughWalkthroughUpdates the Design-Patterns repository documentation with new Content Gap Audit, adds anti-patterns (N+1 Query, Dual Writes) and system patterns (Event Sourcing, Transactional Outbox), introduces a new migration guide for Transactional Outbox, adds framework version baselines, and improves test fixture setup standardization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@VoDaiLocz I've opened a new pull request, #6, to work on those changes. Once the pull request is ready, I'll request review from you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4763f15e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await database.users.find_one({"email": email}) | ||
|
|
||
| async def save(self, data: dict) -> User: | ||
| return await database.users.insert_one(data) |
There was a problem hiding this comment.
UserRepository.save is annotated to return User, but database.users.insert_one(data) returns an insert result object in common Mongo drivers (e.g., Motor/PyMongo), not the created document. In this snippet, create_user also declares response_model=User, so copying this code will produce response-validation failures or missing user fields after create; the example should return a user document (e.g., fetch by inserted id) instead of the raw insert result.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
skills/design-patterns/references/migration-playbook.md (1)
410-414: Strengthen verification step with explicit outbox-state checks.The step says “simulate broker outage,” but the command only runs
npm test. Add a concrete DB assertion (pending rows remain, none marked sent) so verification is actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/design-patterns/references/migration-playbook.md` around lines 410 - 414, Update the verification step that currently only runs "npm test" to include explicit database assertions: extend the test suite (or add a new test invoked by npm test) to query the outbox table and assert that rows created during the simulated broker outage remain in a pending state (e.g., status != 'sent' or sent_at IS NULL) and that no rows are marked sent; reference the outbox table/columns used in your codebase (outbox, status, sent_at or equivalent) and ensure the test fails if any pending message was incorrectly marked as sent.skills/design-patterns/references/system-patterns.md (1)
316-323: Add idempotency note to the outbox “Minimal Flow.”The flow is solid, but it should explicitly state at-least-once delivery and idempotent consumers to avoid duplicate-processing blind spots.
Suggested doc patch
### Minimal Flow
- Begin transaction
- Write business entity (e.g., order)
- Write outbox event row (e.g., order.created)
- Commit
- Outbox worker publishes event and marks row as sent
+6) Consumers handle events idempotently (dedupe by event ID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/design-patterns/references/system-patterns.md` around lines 316 - 323, Add an explicit idempotency and at-least-once delivery note to the "Minimal Flow" section: after the outbox worker step (the list ending with "Outbox worker publishes event and marks row as sent" / the outbox worker behavior), append a step or sentence stating that delivery is at-least-once and consumers must handle events idempotently (e.g., dedupe by event ID) to prevent duplicate processing; reference the "Minimal Flow" heading and the outbox worker description when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTENT-GAP-AUDIT.md`:
- Line 16: Update the capitalization of the word "markdown" to "Markdown" in
CONTENT-GAP-AUDIT.md where the sentence currently reads "Ran a markdown
link/path scan (no actionable broken local links detected)."—locate the phrase
and change it to "Ran a Markdown link/path scan (no actionable broken local
links detected)." to maintain consistent use of "Markdown".
In `@skills/design-patterns/references/architecture-patterns.md`:
- Around line 112-115: The snippet is missing the type imports for CreateUserDTO
and User; add explicit imports for the CreateUserDTO type and the User
model/class so the example is self-contained (e.g., import CreateUserDTO and
User from your DTO/schema and model modules respectively), and ensure the
top-of-file import block that currently contains Depends, HTTPException,
UserRepository, and hash_password also includes CreateUserDTO and User.
- Around line 134-135: The save() function currently returns the InsertOneResult
from database.users.insert_one(data) but is declared to return a User; update
save to return a populated User instance: call database.users.insert_one(data),
get result.inserted_id, then fetch the created document (e.g.,
database.users.find_one({"_id": inserted_id})) or construct a User from the
original data plus the inserted_id, and return that User object; also update the
function's internal variable names and typing if needed so save() actually
returns a User rather than InsertOneResult (referencing save and
database.users.insert_one).
In `@skills/design-patterns/references/migration-playbook.md`:
- Around line 387-393: The outbox event is using orderData.id which may be
undefined for DB-generated IDs; inside the prisma.$transaction callback, capture
the created order returned by tx.order.create (e.g., assign its result to a
variable) and use that persisted order's id when calling tx.outboxEvent.create
for aggregateId instead of orderData.id; update references in the transaction
block (tx.order.create and tx.outboxEvent.create) to use the created order's id.
In `@skills/design-patterns/references/testing-patterns.md`:
- Line 238: The two docs present conflicting Repository APIs: the
testing-patterns example instantiates UserRepository with "new
UserRepository(prisma)" while the architecture-patterns reference describes
static repository methods; pick one canonical style and make both docs
consistent. Either (A) convert the testing-patterns example to the static API by
replacing instance usage with the documented static methods (adjust any calls to
UserRepository.find..., UserRepository.create... accordingly), or (B) update the
architecture-patterns reference to document the instance + DI approach by
replacing static examples with constructor-based usage and showing dependency
injection for UserRepository and its methods; ensure all examples and code
snippets consistently reference the chosen API shape (UserRepository constructor
+ instance methods or UserRepository static methods).
- Around line 292-297: The test fails to arrange readDb.query, so inside the
beforeEach that constructs handler = new GetOrderSummaryHandler(readDb) set a
deterministic mock return for readDb.query (e.g., mockResolvedValue /
mockReturnValue depending on whether GetOrderSummaryHandler expects a promise)
that returns the denormalized fields the assertions expect; update the mock on
the readDb object (readDb.query) so the example is runnable and returns the
exact shape asserted by the test.
---
Nitpick comments:
In `@skills/design-patterns/references/migration-playbook.md`:
- Around line 410-414: Update the verification step that currently only runs
"npm test" to include explicit database assertions: extend the test suite (or
add a new test invoked by npm test) to query the outbox table and assert that
rows created during the simulated broker outage remain in a pending state (e.g.,
status != 'sent' or sent_at IS NULL) and that no rows are marked sent; reference
the outbox table/columns used in your codebase (outbox, status, sent_at or
equivalent) and ensure the test fails if any pending message was incorrectly
marked as sent.
In `@skills/design-patterns/references/system-patterns.md`:
- Around line 316-323: Add an explicit idempotency and at-least-once delivery
note to the "Minimal Flow" section: after the outbox worker step (the list
ending with "Outbox worker publishes event and marks row as sent" / the outbox
worker behavior), append a step or sentence stating that delivery is
at-least-once and consumers must handle events idempotently (e.g., dedupe by
event ID) to prevent duplicate processing; reference the "Minimal Flow" heading
and the outbox worker description when making the change.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed6bf9e9-5225-49df-bf68-ea25b064e07f
📒 Files selected for processing (10)
CONTENT-GAP-AUDIT.mdREADME.mdskills/design-patterns/SKILL.mdskills/design-patterns/references/anti-patterns.mdskills/design-patterns/references/architecture-patterns.mdskills/design-patterns/references/code-patterns.mdskills/design-patterns/references/framework-catalog.mdskills/design-patterns/references/migration-playbook.mdskills/design-patterns/references/system-patterns.mdskills/design-patterns/references/testing-patterns.md
| - Verified all references listed in `skills/design-patterns/SKILL.md` exist in repository. | ||
|
|
||
| ## Low | ||
| - Ran a markdown link/path scan (no actionable broken local links detected). |
There was a problem hiding this comment.
Use “Markdown” capitalization for consistency.
Minor docs polish: “markdown” should be “Markdown”.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...d` exist in repository. ## Low - Ran a markdown link/path scan (no actionable broken lo...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTENT-GAP-AUDIT.md` at line 16, Update the capitalization of the word
"markdown" to "Markdown" in CONTENT-GAP-AUDIT.md where the sentence currently
reads "Ran a markdown link/path scan (no actionable broken local links
detected)."—locate the phrase and change it to "Ran a Markdown link/path scan
(no actionable broken local links detected)." to maintain consistent use of
"Markdown".
| from fastapi import Depends, HTTPException | ||
| from repositories.user_repository import UserRepository | ||
| from utils.security import hash_password | ||
|
|
There was a problem hiding this comment.
Missing type imports in the service snippet.
CreateUserDTO and User are used later in this snippet but not imported here, so the example is not self-contained.
Proposed doc fix
from fastapi import Depends, HTTPException
+from schemas.user import CreateUserDTO, User
from repositories.user_repository import UserRepository
from utils.security import hash_password🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/design-patterns/references/architecture-patterns.md` around lines 112
- 115, The snippet is missing the type imports for CreateUserDTO and User; add
explicit imports for the CreateUserDTO type and the User model/class so the
example is self-contained (e.g., import CreateUserDTO and User from your
DTO/schema and model modules respectively), and ensure the top-of-file import
block that currently contains Depends, HTTPException, UserRepository, and
hash_password also includes CreateUserDTO and User.
| async def save(self, data: dict) -> User: | ||
| return await database.users.insert_one(data) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PyMongo insert_one return type what does it return
💡 Result:
Collection.insert_one(document, ...) returns a pymongo.results.InsertOneResult object. [1]
From that result you typically use:
result.inserted_id→ the inserted document’s_id[1]result.acknowledged→ whether the write was acknowledged (e.g.,FalsewithWriteConcern(w=0)) [1]
Example:
result = collection.insert_one({"x": 1})
_id = result.inserted_id
``` [2]
**Sources:** [1] PyMongo `InsertOneResult` API docs, [2] PyMongo `Collection.insert_one` API docs
---
🌐 Web query:
`Motor async MongoDB driver insert_one return type`
💡 Result:
In Motor (e.g., `AsyncIOMotorCollection.insert_one()`), `insert_one` is an async coroutine, so you do:
```py
result = await collection.insert_one(doc)and it returns a pymongo.results.InsertOneResult (type annotated as InsertOneResult). You typically use result.inserted_id (and result.acknowledged). [1]
Source(s): [1] (motor.readthedocs.io)
Citations:
- 1: https://motor.readthedocs.io/en/stable/api-asyncio/asyncio_motor_collection.html?utm_source=openai
Fix the return type contract: save() must return a User, not InsertOneResult.
The code declares async def save(self, data: dict) -> User: but insert_one() returns a pymongo.results.InsertOneResult object containing only metadata (inserted_id, acknowledged). This breaks the return contract expected by the service layer and controller.
Proposed doc fix
class UserRepository:
async def save(self, data: dict) -> User:
- return await database.users.insert_one(data)
+ result = await database.users.insert_one(data)
+ return await database.users.find_one({"_id": result.inserted_id})📝 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.
| async def save(self, data: dict) -> User: | |
| return await database.users.insert_one(data) | |
| async def save(self, data: dict) -> User: | |
| result = await database.users.insert_one(data) | |
| return await database.users.find_one({"_id": result.inserted_id}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/design-patterns/references/architecture-patterns.md` around lines 134
- 135, The save() function currently returns the InsertOneResult from
database.users.insert_one(data) but is declared to return a User; update save to
return a populated User instance: call database.users.insert_one(data), get
result.inserted_id, then fetch the created document (e.g.,
database.users.find_one({"_id": inserted_id})) or construct a User from the
original data plus the inserted_id, and return that User object; also update the
function's internal variable names and typing if needed so save() actually
returns a User rather than InsertOneResult (referencing save and
database.users.insert_one).
| await prisma.$transaction(async (tx) => { | ||
| await tx.order.create({ data: orderData }); | ||
| await tx.outboxEvent.create({ | ||
| data: { | ||
| aggregateType: 'order', | ||
| aggregateId: orderData.id, | ||
| eventType: 'order.created', |
There was a problem hiding this comment.
Use persisted order ID, not orderData.id, in outbox event.
orderData.id may not exist when IDs are DB-generated, causing invalid aggregateId in the outbox record.
Suggested fix
await prisma.$transaction(async (tx) => {
- await tx.order.create({ data: orderData });
+ const createdOrder = await tx.order.create({ data: orderData });
await tx.outboxEvent.create({
data: {
aggregateType: 'order',
- aggregateId: orderData.id,
+ aggregateId: createdOrder.id,
eventType: 'order.created',
- payload: orderData,
+ payload: createdOrder,
},
});
});📝 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.
| await prisma.$transaction(async (tx) => { | |
| await tx.order.create({ data: orderData }); | |
| await tx.outboxEvent.create({ | |
| data: { | |
| aggregateType: 'order', | |
| aggregateId: orderData.id, | |
| eventType: 'order.created', | |
| await prisma.$transaction(async (tx) => { | |
| const createdOrder = await tx.order.create({ data: orderData }); | |
| await tx.outboxEvent.create({ | |
| data: { | |
| aggregateType: 'order', | |
| aggregateId: createdOrder.id, | |
| eventType: 'order.created', | |
| payload: createdOrder, | |
| }, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/design-patterns/references/migration-playbook.md` around lines 387 -
393, The outbox event is using orderData.id which may be undefined for
DB-generated IDs; inside the prisma.$transaction callback, capture the created
order returned by tx.order.create (e.g., assign its result to a variable) and
use that persisted order's id when calling tx.outboxEvent.create for aggregateId
instead of orderData.id; update references in the transaction block
(tx.order.create and tx.outboxEvent.create) to use the created order's id.
| beforeAll(async () => { | ||
| // Use test database | ||
| await prisma.$connect(); | ||
| repo = new UserRepository(prisma); |
There was a problem hiding this comment.
Repository API shape now conflicts with architecture reference.
This example uses new UserRepository(prisma) (instance API), while skills/design-patterns/references/architecture-patterns.md (Line 87-Line 94) still documents static repository methods. These two docs currently teach incompatible contracts.
Please align both references to one canonical style (instance + DI or static utility).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/design-patterns/references/testing-patterns.md` at line 238, The two
docs present conflicting Repository APIs: the testing-patterns example
instantiates UserRepository with "new UserRepository(prisma)" while the
architecture-patterns reference describes static repository methods; pick one
canonical style and make both docs consistent. Either (A) convert the
testing-patterns example to the static API by replacing instance usage with the
documented static methods (adjust any calls to UserRepository.find...,
UserRepository.create... accordingly), or (B) update the architecture-patterns
reference to document the instance + DI approach by replacing static examples
with constructor-based usage and showing dependency injection for UserRepository
and its methods; ensure all examples and code snippets consistently reference
the chosen API shape (UserRepository constructor + instance methods or
UserRepository static methods).
| let handler: GetOrderSummaryHandler; | ||
| const readDb = { query: vi.fn() }; | ||
|
|
||
| beforeEach(() => { | ||
| handler = new GetOrderSummaryHandler(readDb); | ||
| }); |
There was a problem hiding this comment.
Query handler setup is incomplete for the asserted result shape.
readDb.query is never arranged, but the test later asserts denormalized fields. Add a deterministic mock return in beforeEach (or in the test case) so the example is runnable.
Proposed doc fix
beforeEach(() => {
handler = new GetOrderSummaryHandler(readDb);
+ readDb.query.mockResolvedValue({
+ orderId: "ord-1",
+ userName: "Test User",
+ itemCount: 2,
+ });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/design-patterns/references/testing-patterns.md` around lines 292 -
297, The test fails to arrange readDb.query, so inside the beforeEach that
constructs handler = new GetOrderSummaryHandler(readDb) set a deterministic mock
return for readDb.query (e.g., mockResolvedValue / mockReturnValue depending on
whether GetOrderSummaryHandler expects a promise) that returns the denormalized
fields the assertions expect; update the mock on the readDb object
(readDb.query) so the example is runnable and returns the exact shape asserted
by the test.
Motivation
Description
CONTENT-GAP-AUDIT.mdsummarizing the content additions and verification results.skills/design-patterns/references/anti-patterns.md) withN+1 Query ProblemandDual Writes (DB + Event Bus without Outbox), updated severity and detection/cure guidance, and re-numbered subsequent items.Event SourcingandTransactional Outboxpattern documentation and entries in the pattern decision matrix (skills/design-patterns/references/system-patterns.md).Migration 6: Direct Publish -> Transactional Outboxwith schema, transactional write example, worker guidance, and verification steps (skills/design-patterns/references/migration-playbook.md).Version Baselinetable (skills/design-patterns/references/framework-catalog.md) and bumped README counts/links for anti-patterns and migration guides inREADME.mdandskills/design-patterns/SKILL.md.references/architecture-patterns.md,references/code-patterns.md), and testing examples updated forvitestmocks and CQRS handler tests inreferences/testing-patterns.md.Testing
CONTENT-GAP-AUDIT.md, which found no actionable broken local links.npm testas part of the migration verification steps and observed the test suite completing successfully.Codex Task
Summary by CodeRabbit
New Features
Documentation