feat: add better shop

[mja00]

No description provided.

[mja00]

@codex review this

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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".

[chatgpt-codex-connector]

Treat 36-char slugs as UUIDs

In getShopItemByIdOrSlug (src/services/shop.service.ts) any identifier whose length is 36 and contains a hyphen is automatically treated as a shop UUID (lines 113‑117). Slugs are generated from arbitrary item names, so it is easy to end up with a 36-character slug with hyphens (e.g., mythic-obsidian-deep-sea-fishing-rod). Such a slug will be sent down the /buy command and via the shop buttons, but this method will wrongly call getShopItemById instead of the slug lookup, returning null and causing “Shop item not found” even though the item exists. Please use a stricter UUID check (e.g., regex for the 8-4-4-4-12 pattern) or try the slug lookup first so legitimate slugs can always be purchased.

Useful? React with 👍 / 👎.

[claude]

Pull Request Review: Better Shop Feature

Overview

This PR implements an enhanced shop system with pagination, slug-based item identification, modal-based purchasing, and improved UX. The changes are substantial (~2100 additions) and introduce several new features including:

• Migration of slug field from shop table to items table
• Paginated shop display with navigation buttons
• Modal-based quantity input for purchases
• Autocomplete for the buy command
• Transaction-based purchase logic with race condition protection

Code Quality & Best Practices

✅ Strengths

1. Transaction Safety (src/services/shop.service.ts:167-250): Excellent use of database transactions with race condition protection. The gte(users.money, totalCost) guard in the WHERE clause prevents concurrent purchase issues.

2. Proper Error Handling: Consistent error handling throughout with appropriate logging and user-friendly error messages.

3. Type Safety: Good use of TypeScript types and proper null checking (e.g., buy-command.ts:66-69).

4. Separation of Concerns: Well-organized code with dedicated handlers for buttons and modals.

5. User Experience: Modal for quantity input is a nice touch, and autocomplete makes the buy command more discoverable.

⚠️ Issues & Suggestions

Bug: Duplicate Purchase Logic (CRITICAL)

Both buy-command.ts (lines 91-196) and modal-handler.ts (lines 42-145) contain nearly identical purchase logic. This violates DRY principles and creates maintenance burden.

Impact: Changes to purchase flow must be duplicated in two places, increasing bug risk.

Recommendation: Extract the common purchase logic into a shared method in ShopService or a separate utility.

// In ShopService
public async executePurchase(
    userId: string,
    userTag: string,
    identifier: string,
    quantity: number
): Promise<PurchaseResult> {
    // Common logic from both handlers
}

Potential Race Condition in Button Handler (MEDIUM)

File: src/buttons/shop-button.ts:49-51

The dynamic import of ShopService creates a new instance each time, which is fine, but the lack of deferral before the async operation could cause issues if users spam the button.

Recommendation: Consider moving the ShopService instantiation to the constructor like in other commands.

Missing Input Validation (MEDIUM)

File: src/scripts/populate-slugs.ts:19-21

The generateSlug() function doesn't handle edge cases:

• Empty strings
• Special characters that aren't spaces
• Potential duplicate slugs (e.g., "Item-1" and "Item 1" both become "item-1")

Recommendation:

const generateSlug = (name: string): string => {
    if (!name?.trim()) {
        throw new Error('Cannot generate slug from empty name');
    }
    return name
        .toLowerCase()
        .replace(/[^a-z0-9\s-]/g, '') // Remove special chars
        .replace(/\s+/g, '-')
        .replace(/-+/g, '-') // Collapse multiple hyphens
        .trim();
};

Also, add uniqueness checking or handle the unique constraint violation gracefully.

Magic Numbers (LOW)

File: src/events/modal-handler.ts:65

The max quantity of 1000 is hardcoded. Consider moving to a constant or configuration:

const MAX_PURCHASE_QUANTITY = 1000;

File: src/commands/chat/shop-command.ts:21

const ITEMS_PER_PAGE = 4;

Already properly extracted - good practice! Consider moving to a shared config if other parts of the app need it.

Inconsistent Error Messages (LOW)

Some error messages include balance info, others don't. Standardize for better UX.

Database & Migration Concerns

⚠️ Schema Migration Issue (HIGH PRIORITY)

File: drizzle/0006_silly_richard_fisk.sql

The migration moves slug from shop to items, but doesn't migrate existing data. This could result in:

1. Lost slug data if any existed in the shop table
2. Items without slugs won't appear in autocomplete (buy-command.ts:47-48)

Recommendation: Add a data migration step:

-- Before dropping the shop.slug column
UPDATE items 
SET slug = shop.slug 
FROM shop 
WHERE items.id = shop.item_id AND shop.slug IS NOT NULL;

Alternatively, document that the populate-slugs.ts script must be run after migration.

Missing Index (MEDIUM)

File: src/db/schema.ts:48

The items.slug field has a unique constraint but queries in shop.service.ts:94 would benefit from an index for performance. However, since it's already unique, PostgreSQL automatically creates an index. ✓

Security Considerations

✅ Good Practices

1. SQL Injection Protected: Using Drizzle ORM's parameterized queries throughout.
2. Rate Limiting: Modal handler properly implements rate limiting (modal-handler.ts:16-32).
3. Input Validation: Quantity validation prevents negative or zero purchases.

⚠️ Potential Issues

No Maximum Total Cost Check (LOW)

While individual quantity is limited to 1000, there's no check for maximum total cost. A 1000x purchase of an expensive item could cause integer overflow issues in the database (PostgreSQL's INTEGER max is 2,147,483,647).

Recommendation: Add a sanity check:

const MAX_TOTAL_COST = 1_000_000; // Adjust as needed
if (totalCost > MAX_TOTAL_COST) {
    throw new Error(`Total cost exceeds maximum (${MAX_TOTAL_COST} coins)`);
}

Performance Considerations

✅ Efficient Queries

• Proper use of joins instead of N+1 queries
• Limit clauses on autocomplete (buy-command.ts:78)
• Indexed lookups for slugs

⚠️ Potential Issues

Unoptimized Page Navigation (MEDIUM)

File: src/commands/chat/shop-command.ts:199-230

Every page navigation fetches all shop items from the database, then slices in memory. For large shops, this is wasteful.

Recommendation: Implement database-level pagination:

.limit(ITEMS_PER_PAGE)
.offset((page - 1) * ITEMS_PER_PAGE)

However, this requires a separate count query for total pages. Consider caching shop items count.

Multiple Service Instantiations (LOW)

In modal-handler.ts:76 and :88, new service instances are created. While not necessarily wrong, consider dependency injection or singleton patterns for consistency with the rest of the codebase.

Test Coverage

❌ Missing Tests (HIGH PRIORITY)

No test files were added in this PR. Given the complexity of the feature, especially the transaction logic and race condition protection, automated tests are crucial.

Recommendations:

1. Unit tests for ShopService.purchaseItem() - especially edge cases:
   • Insufficient funds
   • Race conditions
   • Invalid quantities
   • Nonexistent items
2. Integration tests for the purchase flow
3. Tests for slug generation and uniqueness

Additional Observations

Missing Files

• No update to README or documentation about the new shop feature
• Language files updated (lang/lang.en-US.json) but changes are minimal

Good Practices Noted

• Proper logging throughout (using Logger service)
• Consistent code style
• Good commit message ("feat: add better shop")
• Proper use of TypeScript features
• Ephemeral messages for user-facing interactions

Summary & Recommendations

Must Fix Before Merge

1. ❗ Deduplicate purchase logic between buy-command and modal-handler
2. ❗ Add data migration or document the need to run populate-slugs script
3. ❗ Add tests for critical purchase transaction logic

Should Fix

4. Improve generateSlug() function with better validation and duplicate handling
5. Optimize page navigation to use database pagination
6. Add maximum total cost validation

Nice to Have

7. Extract magic numbers to constants
8. Standardize error messages
9. Add JSDoc comments for public API methods (some are already present, ensure consistency)

Overall Assessment

This is a solid feature addition with good architecture and transaction safety. The main concerns are:

• Code duplication that should be addressed
• Missing test coverage
• Potential data migration issues

With the critical issues addressed, this will be a great improvement to the shop system. The UX improvements (modals, autocomplete, pagination) are well-executed.

Recommendation: Request changes for the critical issues, then approve once addressed.

[claude]

Pull Request Review: Better Shop Implementation

Summary

This PR implements a comprehensive shop overhaul with improved UX through pagination, modals for quantity input, and slug-based item identification. The implementation is solid with good architectural patterns, though there are some areas for improvement.

---

Code Quality & Best Practices

✅ Strengths

1. Transaction Safety (src/services/shop.service.ts:167-250)

   • Excellent use of database transactions for atomic operations
   • Race condition protection with balance guard in the WHERE clause (line 178-180)
   • Proper error handling with rollback on failure

2. Code Organization

   • Well-structured separation of concerns (commands, buttons, modals, services)
   • Consistent error handling patterns across all files
   • Good use of TypeScript types and interfaces

3. User Experience

   • Pagination implementation is clean and functional
   • Modal-based quantity input is intuitive
   • Autocomplete for item selection enhances usability

4. Migration Handling

   • Moving slug from shop to items table is the correct architectural decision
   • Utility script (populate-slugs.ts) provided for data migration

⚠️ Areas for Improvement

1. Security & Validation

Database Schema Migration (drizzle/0006_silly_richard_fisk.sql:4)

ALTER TABLE "items" ADD CONSTRAINT "items_slug_unique" UNIQUE("slug");

• Issue: Missing newline at end of file
• Impact: Minor - may cause issues with some SQL tools
• Recommendation: Add newline at EOF

UUID Detection (src/services/shop.service.ts:114)

const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;

• Issue: Doesn't validate UUID version or variant bits
• Improvement: Consider using a UUID validation library or validating against database constraints
• Low Priority: Current implementation is acceptable for this use case

Quantity Limits (src/events/modal-handler.ts:65-70)

if (quantity > 1000) {
    await InteractionUtils.send(intr, { content: 'Maximum quantity is 1000.' }, true);
    return;
}

• Good: Max quantity validation exists
• Missing: Similar validation in buy-command.ts (only validates >= 1)
• Recommendation: Add max quantity check in buy-command.ts for consistency

2. Potential Bugs

Autocomplete Filter Logic (src/commands/chat/buy-command.ts:63-77)

const choices: ApplicationCommandOptionChoiceData[] = filteredItems
    .map((shopItem) => {
        const slug = shopItem.item.slug;
        if (!slug) {
            return null;  // Filtered out earlier, but defensive check
        }
        // ...
    })
    .filter((choice): choice is ApplicationCommandOptionChoiceData => choice !== null)

• Issue: Redundant null check (already filtered on line 48)
• Impact: Minor performance overhead
• Recommendation: Remove redundant check or add a comment explaining defensive programming

Button Defer Handling (src/buttons/shop-button.ts:10)

public deferType = ButtonDeferType.NONE; // We'll handle defer manually based on button type

• Good: Manual defer handling for modal compatibility
• Risk: If error occurs before defer (lines 20-35), user gets no feedback
• Current State: Acceptable, error handling exists (lines 83-94)

3. Error Handling

Duplicate Purchase Logic (modal-handler.ts:99-137 vs buy-command.ts:147-185)

• Issue: Nearly identical purchase logic duplicated in two places
• Impact: Maintenance burden, potential for inconsistencies
• Recommendation: Extract common purchase logic to a shared helper function
• Example refactor:

// In a shared utility or service
private async handlePurchaseResult(
    intr: CommandInteraction | ModalSubmitInteraction,
    shopService: ShopService,
    userService: UserService,
    userId: string,
    slug: string,
    quantity: number,
    shopItem: ShopItem
): Promise<void> {
    // Common purchase logic here
}

Error Message Consistency (src/services/shop.service.ts:163)

throw new Error(`Insufficient funds. Need ${totalCost} coins, have ${user.money} coins`);

vs (line 198)

throw new Error(`Insufficient funds. Need ${totalCost} coins, have ${currentUser[0].money} coins`);

• Good: Consistent error message format
• Note: Second check handles race conditions correctly

4. Performance Considerations

Repeated Shop Queries (src/commands/chat/shop-command.ts:199-201)

public async handlePageNavigation(intr: ButtonInteraction, page: number): Promise<void> {
    const shopItems = await this.shopService.getShopItems();  // Fetches all items on every page turn

• Issue: Full shop query on every page navigation
• Impact: Low if shop is small; could be significant with large catalogs
• Recommendation: Consider caching shop items with TTL or passing items via button state
• Trade-off: Current approach ensures data freshness; acceptable for current scale

N Purchase Records (src/services/shop.service.ts:204-210)

for (let i = 0; i < quantity; i++) {
    purchaseRecords.push({
        userId: userId,
        itemId: shopItem.item.id,
        shopId: shopItem.shop.id,
    });
}

• Question: Why create N individual purchase records instead of one with quantity?
• Impact: Database bloat if users buy items in large quantities
• Recommendation: Consider adding a quantity field to purchases table
• Note: Current approach may be intentional for audit trail - document if so

---

Test Coverage

Missing Tests

• No tests found for new functionality
• Critical paths needing tests:
  • Shop pagination logic
  • Purchase transaction race conditions
  • UUID vs slug detection
  • Quantity validation
  • Modal submission handling

Recommendation: Add integration tests for purchase flow and unit tests for service methods

---

Security Concerns

✅ Good Security Practices

1. SQL Injection Protection: Using Drizzle ORM with parameterized queries
2. Rate Limiting: Applied to buttons and modals (modal-handler.ts:16-19)
3. Transaction Atomicity: Prevents double-spending
4. Input Validation: Quantity bounds checking

⚠️ Minor Concerns

1. Integer Overflow: No check for totalCost overflow with large quantities

   • Example: quantity = 2147483647, cost = 2 → overflow
   • Mitigation: Max quantity of 1000 provides protection, but only enforced in modal handler
   • Recommendation: Add overflow check or document max safe values

2. Slug Uniqueness: Migration adds unique constraint, but populate-slugs.ts doesn't handle conflicts

   • Line 50: const slug = generateSlug(item.name);
   • If two items have same name, migration will fail
   • Recommendation: Add conflict resolution (append ID, counter, etc.)

---

Documentation & Maintenance

Missing Documentation

1. Migration Script (populate-slugs.ts)

   • No documentation on when/how to run
   • Should be added to README or migration guide

2. API Documentation

   • Service methods lack JSDoc for parameters/returns
   • Good: Some methods have JSDoc (e.g., line 18-21)
   • Missing: Complex methods like purchaseItem could use more detail

Code Comments

Well-commented sections:

• Transaction logic in shop.service.ts
• Modal defer handling in shop-button.ts

Could use comments:

• Why individual purchase records vs aggregated (if intentional)
• Pagination algorithm rationale

---

Performance Metrics

Metric	Assessment	Notes
Database Queries	⚠️ Good	Full shop scan on pagination could be optimized
Transaction Efficiency	✅ Excellent	Single transaction for purchase
Code Duplication	⚠️ Moderate	Purchase logic duplicated
Memory Usage	✅ Good	Pagination limits data loaded

---

Recommendations Priority

High Priority

1. ✅ Add max quantity validation to buy-command.ts (consistency)
2. ✅ Extract duplicate purchase logic to shared function
3. ⚠️ Add slug conflict resolution to populate-slugs.ts
4. ⚠️ Add tests for critical purchase flow

Medium Priority

5. Consider caching shop items to reduce database queries
6. Add comprehensive JSDoc documentation
7. Document purchase record design decision
8. Add migration instructions to docs

Low Priority

9. Fix missing newline in SQL migration file
10. Remove redundant null check in autocomplete
11. Consider UUID validation library

---

Conclusion

This is a well-implemented feature with solid architectural decisions and good security practices. The transaction handling is particularly well done. Main areas for improvement are:

1. Reducing code duplication between modal and command handlers
2. Adding test coverage
3. Handling edge cases in the slug population script
4. Minor validation consistency issues

Recommendation: ✅ Approve with minor changes

The code is production-ready, but addressing the high-priority items will improve maintainability and robustness.

Great work on the comprehensive shop system! 🎉

[claude]

Pull Request Review: Better Shop Implementation

I've completed a comprehensive review of this PR. Overall, this is a well-structured feature addition with good database practices and clean architecture.

✅ Strengths

1. Excellent Database Transaction Handling (shop.service.ts:170-253)

   • Atomic transactions prevent race conditions
   • Balance guard in WHERE clause prevents concurrent purchase issues
   • Proper rollback on failure with detailed error messages

2. Clean Schema Migration

   • Moving slug from shop to items table is the right architectural decision
   • Slugs belong with items, not shop entries
   • Includes migration script to populate existing items

3. Good UX Features

   • Paginated shop display prevents overwhelming users
   • Modal-based quantity input is user-friendly
   • Autocomplete for buy command improves discoverability

4. Code Organization

   • Service layer properly separated from command layer
   • executePurchaseWithResponse centralizes purchase logic
   • Button and modal handlers properly integrated

🐛 Potential Bugs

1. UUID Detection Issue (shop.service.ts:117-118)

   • The UUID regex only matches UUIDv4 format but PostgreSQL gen_random_uuid() can generate UUIDs with various version fields
   • Impact: Minor - works for most cases but may fail edge cases
   • Recommendation: Consider using a UUID library for validation

2. Missing Null Checks (shop-command.ts:88-93)

   • Falls back to shop ID when slug is missing, but the buy command won't accept shop IDs after the migration
   • Impact: Medium - confusing UX when items lack slugs
   • Recommendation: Ensure all items have slugs or support both in buy command

3. Race Condition in Slug Generation (populate-slugs.ts:69-92)

   • Check-then-act pattern without transaction - concurrent runs could generate duplicate slugs
   • Impact: Low - script likely runs once manually

⚡ Performance Considerations

1. N+1 Query Pattern (shop-command.ts:152-153, 201)

   • Every page navigation re-queries all shop items
   • Impact: Acceptable for small shops, but scales poorly with hundreds of items
   • Recommendation: Consider caching shop items

2. Autocomplete Query (buy-command.ts:44)

   • Fetches all shop items on every keystroke
   • Recommendation: Add caching layer or optimize with database LIKE queries

3. Multiple Sequential Queries (shop.service.ts:515-534)

   • executePurchaseWithResponse fetches shop item, then user, then executes purchase, then fetches user again
   • Recommendation: Combine queries where possible

🔒 Security Concerns

1. Input Validation

   • Good: Slug generation properly sanitizes input
   • Issue: No validation that item names aren't empty after sanitization
   • Recommendation: Add validation before slug generation

2. Rate Limiting Present ✅ Good use of rate limiting on modal submissions

3. SQL Injection Protection ✅ All queries use parameterized queries via Drizzle ORM

🧪 Test Coverage

Critical Issue: No tests found for new functionality

• No unit tests for ShopService
• No integration tests for purchase transactions
• No tests for edge cases

Recommendation: Add tests for:

• Transaction rollback scenarios
• Concurrent purchase attempts
• Invalid input handling
• Slug generation edge cases

📝 Code Quality & Best Practices

1. Inconsistent Error Handling - Some methods rethrow, others wrap in new Error
2. Magic Numbers ✅ Good use of constants
3. Type Safety ✅ Excellent use of TypeScript throughout
4. Logging ✅ Comprehensive logging for debugging and audit trails
5. Missing JSDoc - Button execute method lacks documentation

🎯 Recommendations

High Priority:

1. Add test coverage for critical purchase transaction logic
2. Ensure all items have slugs or handle missing slugs gracefully
3. Add database index on items.slug for autocomplete performance

Medium Priority:
4. Implement caching for shop items to reduce database load
5. Standardize error handling patterns across service methods
6. Add validation for edge cases in slug generation

Low Priority:
7. Consider paginating autocomplete results in database query
8. Add JSDoc comments to public methods in buttons/handlers
9. Consider adding ON DELETE CASCADE to foreign keys for data cleanup

📊 Summary

Code Quality: ⭐⭐⭐⭐ (4/5)
Architecture: ⭐⭐⭐⭐⭐ (5/5)
Security: ⭐⭐⭐⭐ (4/5)
Test Coverage: ⭐ (1/5)

Overall Assessment: This is a solid feature implementation with excellent database practices and clean architecture. The main concern is the lack of test coverage for critical transaction logic. The code is production-ready but would benefit from tests before merging.

Recommendation: ✅ Approve with suggestions - Address test coverage and slug handling in follow-up work.

Great work on the transaction handling and architectural decisions! 🎉