Skip to content

Conversation

@dewabisma
Copy link
Contributor

@dewabisma dewabisma commented Dec 22, 2025

Summary

Now user need only to input one data. More check to ensure valid owner of reply tweet.

Changes

  • make target_id optional in table column
  • check reply is actually owned by raider
  • update related tests

- make target_id optional in table column
- check reply is actually owned by raider
- update related tests
@dewabisma dewabisma requested review from illuzen and n13 December 22, 2025 09:39
Copy link
Contributor

@n13 n13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Feat - Simplify Raid Submission

The implementation looks solid and adheres to the project's structure. The use of existing repositories and helper functions like parse_x_status_url keeps the code DRY.

I have a few suggestions to improve performance and robustness, specifically within handle_create_raid_submission.

1. Optimization: Check "Active Raid" First

In src/handlers/raid_quest.rs, the handler currently parses the target link and queries the relevant_tweets table before checking if there is an active raid.

If no raid is active, we should fail immediately to avoid unnecessary parsing and database lookups.

Recommendation: Move the active raid check to the top of the handler.

// Move this check to the start of handle_create_raid_submission
let Some(current_active_raid) = state.db.raid_quests.find_active().await? else {
    return Err(AppError::Database(DbError::RecordNotFound(format!(
        "No active raid is found"
    ))));
};

2. Check User Association Early

Similar to the active raid check, verifying the user's X association can be done earlier. If the user isn't linked, we don't need to parse URLs or look up the target tweet.

Proposed Order of Operations:

  1. Check Active Raid (System availability)
  2. Check X Association (User eligibility)
  3. Parse & Validate Links (Input validation)
  4. Check Target Tweet (Business logic)
  5. Create Submission

3. Idempotency Handling

The creation of a raid submission uses the tweet ID as the primary key. If a user submits the same reply twice (e.g., due to a network retry), this will likely throw a generic database error (Unique Constraint Violation).

Suggestion: Consider handling the unique violation explicitly. Returning a 409 Conflict would be more descriptive for the frontend than a generic 500 or database error.

@dewabisma dewabisma requested a review from n13 January 2, 2026 09:54
@n13
Copy link
Contributor

n13 commented Jan 4, 2026

Note

I let it fix the DRY violoation

PR 45: Simplify Raid Submission - Code Review

Summary

The changes successfully implement the simplification of raid submissions by decoupling the submission from a specific target tweet. The target_id in raid_submissions is now nullable, and the creation flow no longer requires it.

Changes Analysis

Database Schema

  • Migration 007_raid_submissions_target_id_nullable.sql: Correctly alters raid_submissions table to make target_id nullable. This allows submissions to be created without a pre-existing link to a relevant_tweet.

Backend Logic (src/handlers/raid_quest.rs)

  • handle_create_raid_submission:
    • Validates that an active raid exists.
    • Validates that the user has an X association.
    • Parses the tweet reply link to extract the reply_id (submission ID) and reply_username.
    • Security Check: Verifies that the reply_username matches the authenticated user's X username.
    • Creates the submission using state.db.raid_submissions.create without a target_id.
    • Correctness: The logic aligns with the goal. The submission ID is correctly derived from the tweet URL.

Data Model (src/models/raid_submission.rs)

  • CreateRaidSubmission: Struct correctly excludes target_id.
  • RaidSubmission: Field target_id is correctly typed as Option<String>.

Repository (src/repositories/raid_submission.rs)

  • create: The SQL query correctly inserts only id, raid_id, and raider_id. It relies on the default nullability of target_id.

Test Coverage

  • test_create_raid_submission_success:
    • Successfully tests the creation flow.
    • Verification: Explicitly asserts assert!(sub.target_id.is_none());, confirming the decoupling.
    • Note: The test setup calls seed_target_tweet, which is technically redundant now that target_id is not used, but harmless.

Improvements Made (Refactoring)

DRY Principle Adherence

A DRY violation was identified in src/handlers/raid_quest.rs where the logic to fetch the active raid and verify the user's X association was duplicated in handle_get_active_raid_raider_submissions and handle_create_raid_submission.

Action Taken: This has been resolved by extracting the common validation logic into a new helper function:

async fn get_active_raid_and_x_association(
    state: &AppState,
    user: &Address,
) -> Result<(RaidQuest, XAssociation), AppError>

Conclusion

The PR meets the functional requirements. The "simplify" goal is achieved. The code has been refactored to adhere to strict DRY principles.

Copy link
Contributor

@n13 n13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG now

@dewabisma dewabisma merged commit 589bf08 into main Jan 4, 2026
1 check passed
@dewabisma dewabisma deleted the feat/simplify-raid-submission branch January 4, 2026 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants