Fix flashcard order race condition

[tpaulshippy]

Summary

• Use order_by('-order').first() instead of aggregate for max_order calculation
• Removed select_for_update() calls (no-op on SQLite, not needed for this approach)

The race condition is resolved using Django ORM's order_by('-order').first() to get the last card, which is atomic and doesn't require explicit locking on SQLite.

Summary by CodeRabbit

• Bug Fixes
  • Improved flashcard order calculation to use idiomatic Django ORM pattern

[coderabbitai]

📝 Walkthrough

Walkthrough

The create_flashcard tool's transaction now acquires a row-level lock on Flashcard records when computing the maximum order value via select_for_update(), ensuring atomic determination of the next order sequence number while maintaining existing logic flow.

Changes

Cohort / File(s)	Summary
Flashcard Creation Locking back/bots/models/chat.py	Added select_for_update() to the queryset in aggregate(Max('order')) call to apply row-level locks during max order computation in the create_flashcard tool transaction.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A lock upon the flashcard's door,
No race conditions anymore,
With select_for_update() bright,
The order flows, precise and right! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: adding a row-level lock to fix a race condition in flashcard ordering logic.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/flashcard-order-race-condition

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

back/bots/models/chat.py (1)

147-147: Remove unnecessary select_for_update() from Flashcard aggregate query for clarity.

Line 147 uses select_for_update() on a Flashcard aggregate query. On SQLite (your database backend), select_for_update() is silently ignored by Django and provides no locking. Additionally, the Deck is already locked earlier in this transaction, which serializes all per-deck writes. Use order_by('-order').first() instead for clearer, more idiomatic code.

Proposed refactor

-                    max_order = Flashcard.objects.select_for_update().filter(deck=deck).aggregate(models.Max('order'))['order__max'] or -1
+                    last_flashcard = (
+                        Flashcard.objects
+                        .filter(deck=deck)
+                        .order_by('-order')
+                        .first()
+                    )
+                    max_order = last_flashcard.order if last_flashcard else -1

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@back/bots/models/chat.py` at line 147, The aggregate query uses
select_for_update() unnecessarily on Flashcard: remove select_for_update() and
replace the aggregate approach with an idiomatic lookup using
order_by('-order').first() to compute max_order; specifically, in the Flashcard
query that currently assigns max_order (the line referencing
Flashcard.objects.select_for_update().filter(deck=deck).aggregate(...)), drop
select_for_update() and instead fetch the highest-order Flashcard via
Flashcard.objects.filter(deck=deck).order_by('-order').first() and derive
max_order from that result (defaulting to -1 if None); keep the surrounding
transaction/Deck locking logic unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@back/bots/models/chat.py`:
- Line 147: The aggregate query uses select_for_update() unnecessarily on
Flashcard: remove select_for_update() and replace the aggregate approach with an
idiomatic lookup using order_by('-order').first() to compute max_order;
specifically, in the Flashcard query that currently assigns max_order (the line
referencing
Flashcard.objects.select_for_update().filter(deck=deck).aggregate(...)), drop
select_for_update() and instead fetch the highest-order Flashcard via
Flashcard.objects.filter(deck=deck).order_by('-order').first() and derive
max_order from that result (defaulting to -1 if None); keep the surrounding
transaction/Deck locking logic unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0fb8e024-50a5-4031-8c13-eade220084c5

📥 Commits

Reviewing files that changed from the base of the PR and between 7b6d850 and bebec56.

📒 Files selected for processing (1)

• back/bots/models/chat.py