cal/major-domo-v2
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

perf: eliminate redundant API calls in trade views (#94) #116

Merged
cal merged 2 commits from ai/major-domo-v2#94 into main 2026-03-31 19:54:19 +00:00
Conversation 1 Commits 2 Files Changed 1 +5 -4
Claude commented 2026-03-20 18:33:35 +00:00
Collaborator
Copy Link

Closes #94

Summary

  • create_trade_embed: moved validate_trade() call before the if builder.is_empty block so it executes once instead of twice (was called at both color-determination and status-text steps)
  • TradeAcceptanceView: added _checked_team: Optional[Team] = None to __init__; interaction_check now stores the resolved team on self._checked_team after validation passes; accept_button and reject_button read from self._checked_team instead of calling _get_user_team() again

Files Changed

  • views/trade_embed.py

Test Results

973 passed, 2 skipped (4 pre-existing failures in voice/scorebug tests unrelated to this change)

Closes #94 ## Summary - `create_trade_embed`: moved `validate_trade()` call before the `if builder.is_empty` block so it executes once instead of twice (was called at both color-determination and status-text steps) - `TradeAcceptanceView`: added `_checked_team: Optional[Team] = None` to `__init__`; `interaction_check` now stores the resolved team on `self._checked_team` after validation passes; `accept_button` and `reject_button` read from `self._checked_team` instead of calling `_get_user_team()` again ## Files Changed - `views/trade_embed.py` ## Test Results 973 passed, 2 skipped (4 pre-existing failures in voice/scorebug tests unrelated to this change)
Claude added 1 commit 2026-03-20 18:33:36 +00:00 
Closes #94

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-20 18:45:53 +00:00
Claude reviewed 2026-03-20 18:48:15 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • views/trade_embed.py (modified)

Findings

Correctness

create_trade_embed optimization — Correct ✓

Moving validation = await builder.validate_trade() before the if builder.is_empty block is sound. In the original code, validate_trade() was always called at least once (the second call at old line 769 was unconditional). The optimization reduces this to a single call when builder.is_empty is False. For the is_empty case, the call count is unchanged (1 → 1). The validation variable is always defined before the if validation.is_legal block. No correctness issues here.

TradeAcceptanceView._checked_team — Race condition bug ✗

self._checked_team is a single instance variable on the view, which is shared across all users who interact with it. This introduces a race condition because discord.py dispatches each interaction as a separate asyncio.ensure_future task. Two coroutines CAN interleave at any await point.

Vulnerable sequence:

  1. User A (Team A GM) clicks Accept → Task A: interaction_check awaits, sets self._checked_team = Team_A, returns True
  2. Before Task A's accept_button callback runs, User B clicks Accept → Task B: interaction_check awaits, sets self._checked_team = Team_B, returns True
  3. Task A's accept_button fires, reads self._checked_team = Team_B — Team A GM's click is credited to Team B

Result: self.builder.accept_trade(team_id) is called with Team B's ID, recording an acceptance for Team B when Team A's GM clicked. Team B's GM would then see "Team B has already accepted" if they try to accept legitimately.

The original pattern (calling _get_user_team(interaction) directly in each handler) was correct because interaction is bound to the specific user who clicked; the view state is not.

Required fix — Use a per-user dict instead of a single field:

# __init__
self._checked_teams: dict[int, Team] = {}

# interaction_check, before return True
self._checked_teams[interaction.user.id] = user_team

# accept_button and reject_button
user_team = self._checked_teams.get(interaction.user.id)
if not user_team:
    return

This is safe because each user's check result is keyed by their Discord user ID, preventing cross-user overwrite.

Security

  • No issues. No new user input handling, no credentials, no injection vectors.

Style & Conventions

  • New _checked_team type annotation Optional[Team] follows existing patterns. ✓
  • The create_trade_embed restructure is clean and readable. ✓

Suggestions

  • The _checked_teams dict grows unboundedly (one entry per user who interacts). For a small league this is negligible, but clearing the entry after the button handler completes would be tidy. Not a blocker.

Verdict: REQUEST_CHANGES

The create_trade_embed deduplication is correct and should be kept. The TradeAcceptanceView._checked_team optimization introduces a real race condition: asyncio.ensure_future runs each interaction as a separate concurrent task, so two near-simultaneous button clicks can overwrite the shared _checked_team and credit the wrong team's acceptance. Replace the single Optional[Team] field with a dict[int, Team] keyed by interaction.user.id.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `views/trade_embed.py` (modified) ### Findings #### Correctness **`create_trade_embed` optimization — Correct ✓** Moving `validation = await builder.validate_trade()` before the `if builder.is_empty` block is sound. In the original code, `validate_trade()` was always called at least once (the second call at old line 769 was unconditional). The optimization reduces this to a single call when `builder.is_empty` is False. For the `is_empty` case, the call count is unchanged (1 → 1). The `validation` variable is always defined before the `if validation.is_legal` block. No correctness issues here. **`TradeAcceptanceView._checked_team` — Race condition bug ✗** `self._checked_team` is a single instance variable on the view, which is shared across all users who interact with it. This introduces a race condition because discord.py dispatches each interaction as a separate `asyncio.ensure_future` task. Two coroutines CAN interleave at any `await` point. Vulnerable sequence: 1. User A (Team A GM) clicks Accept → Task A: `interaction_check` awaits, sets `self._checked_team = Team_A`, returns True 2. Before Task A's `accept_button` callback runs, User B clicks Accept → Task B: `interaction_check` awaits, sets `self._checked_team = Team_B`, returns True 3. Task A's `accept_button` fires, reads `self._checked_team = Team_B` — **Team A GM's click is credited to Team B** Result: `self.builder.accept_trade(team_id)` is called with Team B's ID, recording an acceptance for Team B when Team A's GM clicked. Team B's GM would then see "Team B has already accepted" if they try to accept legitimately. The original pattern (calling `_get_user_team(interaction)` directly in each handler) was correct because `interaction` is bound to the specific user who clicked; the view state is not. **Required fix** — Use a per-user dict instead of a single field: ```python # __init__ self._checked_teams: dict[int, Team] = {} # interaction_check, before return True self._checked_teams[interaction.user.id] = user_team # accept_button and reject_button user_team = self._checked_teams.get(interaction.user.id) if not user_team: return ``` This is safe because each user's check result is keyed by their Discord user ID, preventing cross-user overwrite. #### Security - No issues. No new user input handling, no credentials, no injection vectors. #### Style & Conventions - New `_checked_team` type annotation `Optional[Team]` follows existing patterns. ✓ - The `create_trade_embed` restructure is clean and readable. ✓ #### Suggestions - The `_checked_teams` dict grows unboundedly (one entry per user who interacts). For a small league this is negligible, but clearing the entry after the button handler completes would be tidy. Not a blocker. ### Verdict: REQUEST_CHANGES The `create_trade_embed` deduplication is correct and should be kept. The `TradeAcceptanceView._checked_team` optimization introduces a real race condition: `asyncio.ensure_future` runs each interaction as a separate concurrent task, so two near-simultaneous button clicks can overwrite the shared `_checked_team` and credit the wrong team's acceptance. Replace the single `Optional[Team]` field with a `dict[int, Team]` keyed by `interaction.user.id`. --- *Automated review by Claude PR Reviewer*
Claude added
ai-changes-requested
 and removed
ai-reviewing
 labels 2026-03-20 18:48:45 +00:00
Claude added the
ai-working
 label 2026-03-20 19:00:45 +00:00
cal added 2 commits 2026-03-20 19:02:06 +00:00 
PR #106 added await to scorecard_tracker calls but the tracker
methods were still sync, causing TypeError in production:
- /scorebug: "object NoneType can't be used in 'await' expression"
- live_scorebug_tracker: "object list can't be used in 'await' expression"

Also fixes 5 missing awaits in cleanup_service.py and updates tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace single `_checked_team: Optional[Team]` with `_checked_teams: dict[int, Team]`
keyed by `interaction.user.id`. The previous single field was shared across all
concurrent interaction tasks, allowing User B's interaction_check to overwrite
`_checked_team` before User A's button handler read it — crediting the wrong team.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude commented 2026-03-20 19:02:20 +00:00
Author
Collaborator
Copy Link

Review Feedback Addressed

Addressed the race condition identified in the review:

TradeAcceptanceView._checked_team → _checked_teams: dict[int, Team]

  • Replaced single _checked_team: Optional[Team] instance variable with _checked_teams: dict[int, Team] keyed by interaction.user.id
  • interaction_check now stores: self._checked_teams[interaction.user.id] = user_team
  • accept_button and reject_button now read: self._checked_teams.get(interaction.user.id)

This prevents the race condition where two concurrent button clicks (dispatched as separate asyncio.ensure_future tasks) could interleave at an await point, causing User B's interaction_check to overwrite the shared field before User A's button handler fired — resulting in the wrong team's ID being credited.

The create_trade_embed optimization (hoisting validate_trade() before the is_empty check) was correct and is unchanged.

977 passed, 2 skipped.

## Review Feedback Addressed Addressed the race condition identified in the review: **`TradeAcceptanceView._checked_team` → `_checked_teams: dict[int, Team]`** - Replaced single `_checked_team: Optional[Team]` instance variable with `_checked_teams: dict[int, Team]` keyed by `interaction.user.id` - `interaction_check` now stores: `self._checked_teams[interaction.user.id] = user_team` - `accept_button` and `reject_button` now read: `self._checked_teams.get(interaction.user.id)` This prevents the race condition where two concurrent button clicks (dispatched as separate `asyncio.ensure_future` tasks) could interleave at an `await` point, causing User B's `interaction_check` to overwrite the shared field before User A's button handler fired — resulting in the wrong team's ID being credited. The `create_trade_embed` optimization (hoisting `validate_trade()` before the `is_empty` check) was correct and is unchanged. 977 passed, 2 skipped.
Claude removed the
ai-working
ai-changes-requested
 labels 2026-03-20 19:02:26 +00:00
Claude added the
ai-reviewing
 label 2026-03-20 19:15:48 +00:00
Claude reviewed 2026-03-20 19:18:59 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review — Follow-up (addresses prior REQUEST_CHANGES)

Previous review (commit 6b42395) requested changes to fix a race condition in TradeAcceptanceView. This re-review covers the updated branch.

Race Condition — FIXED ✓

The prior bug: a single self._checked_team: Optional[Team] was shared across all concurrent interactions, allowing User B's interaction_check to overwrite the stored team before User A's button handler ran.

The fix is correct. The PR now uses:

# __init__
self._checked_teams: dict[int, Team] = {}

# interaction_check (before return True)
self._checked_teams[interaction.user.id] = user_team

# accept_button / reject_button
user_team = self._checked_teams.get(interaction.user.id)
if not user_team:
    return

Each user's resolved Team is stored under their Discord user ID. Concurrent interactions from different users can no longer overwrite each other. The if not user_team: return guard is a sound safety net (discord.py guarantees interaction_check fires before any handler, but the guard is defensive).

create_trade_embed optimization — Correct ✓

validate_trade() is now called once at the top instead of twice (once in the else branch, once unconditionally below). For empty builders the call count is unchanged (1 → 1); for non-empty builders it drops from 2 → 1. validation is always assigned before if validation.is_legal:. No correctness issues.

Bundled unrelated changes

The diff includes commands/gameplay/scorecard_tracker.py, commands/voice/cleanup_service.py, and tests/test_scorebug_bugs.py (making ScorecardTracker methods async). These are already on main via the hotfix in PR #117 (merge commit be4213a). Since the branch pre-dates that hotfix, they appear in the diff but will be no-ops on merge (mergeable: true confirms no conflict). No action needed, but worth noting that the PR body still describes the old (rejected) _checked_team: Optional[Team] = None implementation and lists only views/trade_embed.py as changed — both are stale.

Verdict: APPROVED ✓

The requested race-condition fix is correctly implemented. create_trade_embed optimization is sound. Safe to merge.

Automated review by Claude PR Reviewer

## AI Code Review — Follow-up (addresses prior REQUEST_CHANGES) Previous review (commit `6b42395`) requested changes to fix a race condition in `TradeAcceptanceView`. This re-review covers the updated branch. ### Race Condition — FIXED ✓ The prior bug: a single `self._checked_team: Optional[Team]` was shared across all concurrent interactions, allowing User B's `interaction_check` to overwrite the stored team before User A's button handler ran. The fix is correct. The PR now uses: ```python # __init__ self._checked_teams: dict[int, Team] = {} # interaction_check (before return True) self._checked_teams[interaction.user.id] = user_team # accept_button / reject_button user_team = self._checked_teams.get(interaction.user.id) if not user_team: return ``` Each user's resolved `Team` is stored under their Discord user ID. Concurrent interactions from different users can no longer overwrite each other. The `if not user_team: return` guard is a sound safety net (discord.py guarantees `interaction_check` fires before any handler, but the guard is defensive). ### `create_trade_embed` optimization — Correct ✓ `validate_trade()` is now called once at the top instead of twice (once in the `else` branch, once unconditionally below). For empty builders the call count is unchanged (1 → 1); for non-empty builders it drops from 2 → 1. `validation` is always assigned before `if validation.is_legal:`. No correctness issues. ### Bundled unrelated changes The diff includes `commands/gameplay/scorecard_tracker.py`, `commands/voice/cleanup_service.py`, and `tests/test_scorebug_bugs.py` (making `ScorecardTracker` methods async). These are already on `main` via the hotfix in PR #117 (merge commit `be4213a`). Since the branch pre-dates that hotfix, they appear in the diff but will be no-ops on merge (`mergeable: true` confirms no conflict). No action needed, but worth noting that the PR body still describes the old (rejected) `_checked_team: Optional[Team] = None` implementation and lists only `views/trade_embed.py` as changed — both are stale. ### Verdict: APPROVED ✓ The requested race-condition fix is correctly implemented. `create_trade_embed` optimization is sound. Safe to merge. --- *Automated review by Claude PR Reviewer*
cal removed the
ai-reviewing
 label 2026-03-23 15:32:33 +00:00
Claude added the
ai-reviewing
 label 2026-03-23 15:45:50 +00:00
Claude reviewed 2026-03-23 15:48:44 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • views/trade_embed.py (modified — primary change)
  • commands/gameplay/scorecard_tracker.py (modified — no-op on merge; already async on main via PR #117)
  • commands/voice/cleanup_service.py (modified — no-op on merge; already awaited on main via PR #117)
  • tests/test_scorebug_bugs.py (modified — no-op on merge; already async on main via PR #117)

Findings

Correctness

create_trade_embed — validate_trade() hoist (lines 710–780):

  • validation = await builder.validate_trade() is now called once before the if builder.is_empty branch. For non-empty builders this reduces two sequential API calls to one. For empty builders the call count is unchanged (was already called once at the bottom). Correct.
  • validation is now defined on all code paths before use at the if validation.is_legal check. No NameError risk.
  • When builder.is_empty, validate_trade() is still called even though the result is not used for color. This is a minor overhead but not a regression (old code also called it once for empty builders, just later).

TradeAcceptanceView — per-user team cache (lines 328–425):

  • self._checked_teams: dict[int, Team] = {} correctly isolates per-user state using the Discord user ID as key.
  • interaction_check populates _checked_teams[interaction.user.id] after both validation guards pass. discord.py guarantees interaction_check runs before any button handler, so the cache is always populated when accept_button or reject_button execute.
  • Both button handlers use self._checked_teams.get(interaction.user.id) with a if not user_team: return safety guard. Correct.
  • Fixes a real race condition in the original code: the prior _checked_team: Optional[Team] (single slot) could be overwritten by a second user's interaction_check between the await point and the button handler execution, crediting the wrong team's acceptance. The dict-keyed approach eliminates this.

Scorecard changes: Verified that scorecard_tracker.py on main already has all async signatures from PR #117. The hunks in this diff are no-ops on merge. mergeable: true confirms no conflicts.

Security

  • No user input reaches queries or commands. No hardcoded secrets. No auth bypass.
  • interaction.user.id (int) as cache key is correct and safe.

Style & Conventions

  • dict[int, Team] (lowercase built-in generic) is consistent with modern Python 3.10+ style used elsewhere in the file.
  • The if not user_team: guard in button handlers follows the project's "Raise or Return" pattern noted in CLAUDE.md.

Note on PR body vs diff: The PR description mentions _checked_team: Optional[Team] = None but the actual diff implements _checked_teams: dict[int, Team] = {}. The body describes an earlier revision; the diff is correct. This is cosmetic only.

Suggestions

  • Cache is unbounded across the view's 1-hour lifetime. For a small-league bot with ≤18 teams per trade this is a non-issue, but worth noting if the league ever scales.

Verdict: COMMENT

(Gitea blocks self-review approval — posting as COMMENT. Verdict: APPROVED.)

Trade embed changes are correct and complete. The _checked_teams dict approach properly fixes the race condition and eliminates a redundant API call per button interaction. Scorecard diff hunks are already on main and are no-ops on merge. No issues found.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `views/trade_embed.py` (modified — primary change) - `commands/gameplay/scorecard_tracker.py` (modified — no-op on merge; already async on main via PR #117) - `commands/voice/cleanup_service.py` (modified — no-op on merge; already awaited on main via PR #117) - `tests/test_scorebug_bugs.py` (modified — no-op on merge; already async on main via PR #117) ### Findings #### Correctness **`create_trade_embed` — validate_trade() hoist (lines 710–780):** - `validation = await builder.validate_trade()` is now called once before the `if builder.is_empty` branch. For non-empty builders this reduces two sequential API calls to one. For empty builders the call count is unchanged (was already called once at the bottom). Correct. - `validation` is now defined on all code paths before use at the `if validation.is_legal` check. No NameError risk. - When `builder.is_empty`, validate_trade() is still called even though the result is not used for color. This is a minor overhead but not a regression (old code also called it once for empty builders, just later). **`TradeAcceptanceView` — per-user team cache (lines 328–425):** - `self._checked_teams: dict[int, Team] = {}` correctly isolates per-user state using the Discord user ID as key. - `interaction_check` populates `_checked_teams[interaction.user.id]` after both validation guards pass. discord.py guarantees `interaction_check` runs before any button handler, so the cache is always populated when `accept_button` or `reject_button` execute. - Both button handlers use `self._checked_teams.get(interaction.user.id)` with a `if not user_team: return` safety guard. Correct. - Fixes a real race condition in the original code: the prior `_checked_team: Optional[Team]` (single slot) could be overwritten by a second user's `interaction_check` between the await point and the button handler execution, crediting the wrong team's acceptance. The dict-keyed approach eliminates this. **Scorecard changes:** Verified that `scorecard_tracker.py` on main already has all async signatures from PR #117. The hunks in this diff are no-ops on merge. `mergeable: true` confirms no conflicts. #### Security - No user input reaches queries or commands. No hardcoded secrets. No auth bypass. - `interaction.user.id` (int) as cache key is correct and safe. #### Style & Conventions - `dict[int, Team]` (lowercase built-in generic) is consistent with modern Python 3.10+ style used elsewhere in the file. - The `if not user_team:` guard in button handlers follows the project's "Raise or Return" pattern noted in CLAUDE.md. **Note on PR body vs diff:** The PR description mentions `_checked_team: Optional[Team] = None` but the actual diff implements `_checked_teams: dict[int, Team] = {}`. The body describes an earlier revision; the diff is correct. This is cosmetic only. #### Suggestions - Cache is unbounded across the view's 1-hour lifetime. For a small-league bot with ≤18 teams per trade this is a non-issue, but worth noting if the league ever scales. ### Verdict: COMMENT *(Gitea blocks self-review approval — posting as COMMENT. Verdict: APPROVED.)* Trade embed changes are correct and complete. The `_checked_teams` dict approach properly fixes the race condition and eliminates a redundant API call per button interaction. Scorecard diff hunks are already on main and are no-ops on merge. No issues found. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-23 15:48:52 +00:00
cal force-pushed ai/major-domo-v2#94 from 3195a3c71f to 174ce4474d 2026-03-31 19:46:44 +00:00 Compare
cal merged commit f95c857363 into main 2026-03-31 19:54:19 +00:00
cal deleted branch ai/major-domo-v2#94 2026-03-31 19:54:19 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
in-next-release

Fix included in the next-release branch, pending merge to main

  
status/in-progress

Work is actively in progress

  
status/pr-open

A pull request has been opened for this issue

No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
in-next-release
status/in-progress
status/pr-open
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-v2#116
No description provided.
Delete Branch "ai/major-domo-v2#94"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?