Skip to content

fix: whitelist not returning username#65

Merged
dewabisma merged 5 commits intomainfrom
fix/twitter-aggregation
Mar 30, 2026
Merged

fix: whitelist not returning username#65
dewabisma merged 5 commits intomainfrom
fix/twitter-aggregation

Conversation

@dewabisma
Copy link
Copy Markdown
Contributor

@dewabisma dewabisma commented Mar 27, 2026

Summary

So, apparently it was fine to use ID before but now it's not, welp... X changed their API.

Copy link
Copy Markdown
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.

I think these findings are mostly nitpicks and that stuff was already there,

feel free to look at them / ignore

Approved!

PR #65 Review: "fix: whitelist not returning username"

CI Status: Passing

Summary

This PR has two logical changes:

  1. Fix get_whitelist to return usernames instead of IDs (the actual bug fix)
  2. Filter out duplicate since_id tweets before sending to Telegram (new feature)

Plus test seed script updates.


1. Whitelist fix (src/repositories/tweet_author.rs)

The one-liner changing SELECT id to SELECT username looks correct. The build_batched_whitelist_queries function in rusx clearly expects usernames (Twitter's from: search operator works with usernames, not numeric IDs), so this was indeed a bug.

Minor nit: The local variable is still called ids on line 111. Since this now returns usernames, it would be cleaner to rename it to something like usernames -- but this is cosmetic and non-blocking.


2. Duplicate since_id filtering (src/services/tweet_synchronizer_service.rs)

The logic at line 233-238 in the diff filters out the tweet whose ID matches last_id before sending raid targets. This prevents re-notifying Telegram about a tweet that was already processed in a previous sync cycle.

This looks good, but I have one concern:

The filter only applies to process_sending_raid_targets, not to the earlier process_relevant_tweets call. The duplicate tweet still gets upserted to the DB via process_relevant_tweets (line 224). That's fine because the DB upsert is idempotent (ON CONFLICT ... DO UPDATE), but it means track_tweets_pulled on line 231 will also count the duplicate in its metrics. If accurate metrics matter, consider filtering before that point -- or at least be aware this is intentional.

One more thing: last_id is fetched once at line 185 (before the for query in whitelist_queries loop), but the loop could process multiple batches. Each batch could produce new tweets, so last_id would only be accurate for the first iteration. That said, duplicate since_id echoes would only happen for the first batch anyway (since the same since_id is sent for all queries), so this is fine in practice.


3. New test: test_sync_excludes_last_id_from_telegram_when_raid_active

The test is well-structured:

  • Seeds a "last tweet" into the DB
  • Returns that same tweet from the mock Twitter API
  • Asserts Telegram receives 0 messages (.expect(0))

This directly verifies the fix. Nice.


4. Seed scripts

  • seed_test_relevant_tweets.sh -- new script for seeding relevant tweets, looks reasonable for manual testing.
  • seed_test_tweet_authors.sh -- adds a second author (nic_carter) and adds ON CONFLICT upsert handling so it's re-runnable. Also changes the verify query to select all authors instead of one. All fine.

One thing: the SQL_FILE variable in seed_test_relevant_tweets.sh is set to "seed_authors.sql" (line 8) -- this is a copy-paste issue. It should probably be "seed_relevant_tweets.sql" for clarity since it's seeding relevant tweets, not authors.


Verdict

The core fixes are solid and the test coverage is good. The PR is safe to merge. The items I'd suggest addressing (none are blocking):

Item Severity Description
Variable naming Nit ids -> usernames in get_whitelist
SQL_FILE naming Low seed_test_relevant_tweets.sh uses seed_authors.sql as the temp file name
Metrics counting Info track_tweets_pulled still counts the duplicate -- fine if intentional

@dewabisma dewabisma merged commit 4e3813c into main Mar 30, 2026
1 check passed
@dewabisma dewabisma deleted the fix/twitter-aggregation branch March 30, 2026 05:52
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.

2 participants