Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReorganizes notification settings: removes the question-subscription UI, adds "Site News" and "Keeping up" follow controls, introduces auto-follow-on-predict and per-user default follow preferences, extends user/profile types and migration, updates serializers/views/API, and adds i18n keys across multiple locales. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend
participant API as Backend API
participant Service as Subscription Service
participant DB as Database
User->>Frontend: Submit prediction (created=true)
Frontend->>API: POST /forecasts (prediction)
API->>DB: Load user (includes automatically_follow_on_predict)
DB-->>API: Return user
alt automatically_follow_on_predict = true
API->>Service: Query existing non-global subscriptions for post
Service->>DB: Fetch subscriptions
DB-->>Service: Return existing subscriptions
alt missing subscription types
API->>Service: Create CP-change, comment, milestone, status-change subscriptions
Service->>DB: Insert subscriptions
DB-->>Service: Confirm creation
end
end
API-->>Frontend: Return forecast created (and subscriptions)
Frontend-->>User: Show success and auto-follow result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
lsabor
force-pushed
the
issue/3350/feat/follow-reorganization
branch
from
87f684b to
86bc72b
Compare
…/feat/follow-reorganization
|
This pull request has been automatically marked as stale because it has not had recent activity. It will not be closed automatically, but please consider updating it or closing it if it is no longer relevant. |
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/app/(main)/accounts/settings/notifications/components/email_notifications.tsx (1)
97-123:⚠️ Potential issue | 🟠 MajorDon’t build mailing-tag updates from the original
userprop.These switches still render from, and diff against,
user.unsubscribed_mailing_tags. After the first toggle, a second change is computed from stale data and can silently undo the first choice. Keep a localunsubscribedMailingTagsstate here (or send per-tag deltas) and render/update from that instead.Also applies to: 231-247, 281-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx around lines 97 - 123, The toggle handler and render currently read and diff against the stale user.unsubscribed_mailing_tags prop (in handleEmailSubscriptionChange), causing racey undos; create a local state like unsubscribedMailingTags initialized from user.unsubscribed_mailing_tags, render switches from that state, and in handleEmailSubscriptionChange (and the places referenced at 231-247, 281-290) compute the new set from unsubscribedMailingTags, immediately update unsubscribedMailingTags optimistically and setLoadingTag, then call updateProfileAction (via updateMailingTag / useServerAction) and reconcile on success/failure — keep references to handleEmailSubscriptionChange, updateProfileAction, updateMailingTag, isMailingTagPending so the UI uses the local state and not the original user prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx:
- Around line 163-169: updateDefaultFollow currently fire-and-forgets
runDefaultFollowUpdate which allows overlapping writes; change it to serialize
the save by awaiting the server action and managing the pending state: call
setSavingDefaultField(field) before the await, await
runDefaultFollowUpdate(updates) inside a try/catch, clear
setSavingDefaultField(null) in finally and handle/log errors in catch; also
ensure the button groups use the same saving state (setSavingDefaultField) to
disable themselves while pending (same pattern applies to the similar handlers
runDefaultMentionUpdate/runDefaultMuteUpdate and their wrappers at the other
ranges).
- Around line 46-50: CP_BUTTONS currently uses hardcoded English labels
("small","medium","large") which prevents localization; replace the static
labels with translated strings obtained via useTranslations() (or the project's
i18n helper) and construct CP_BUTTONS using those translated values so
ButtonGroup renders localized labels; update the code where CP_BUTTONS is
defined (the GroupButton<CpKey> array) to call useTranslations() inside the
component or pass translated labels in from the parent and ensure keys (value:
"small"/"medium"/"large") remain unchanged while label uses t('...') lookups.
In `@questions/services/forecasts.py`:
- Around line 292-343: The auto-follow subscription creation guarded by created
and user.automatically_follow_on_predict (the block using post = question.post,
existing_subscription, PostSubscription.SubscriptionType.* and
create_subscription) is currently placed after early returns that prevent it
from running for normal forecasts; extract these checks into a helper (e.g.,
ensure_auto_follow_subscriptions(created, user, question)) or move the entire
block to execute immediately when created is True (before the early returns),
and use the same existing_subscription and create_subscription calls to add
CP_CHANGE, NEW_COMMENTS, MILESTONE, and STATUS_CHANGE subscriptions only if they
don't already exist.
- Around line 294-343: The subscription creation is racy because you check
existence then call create_subscription which uses direct .create(), risking
IntegrityError under concurrent requests; update the logic so
create_subscription (or the underlying factory) uses get_or_create (or
update_or_create) keyed on (type, user, post, is_global) or wraps the create in
a try/except that handles IntegrityError by returning the existing
PostSubscription; specifically modify create_subscription (and any factory it
calls) to call PostSubscription.objects.get_or_create(...) for
PostSubscription.SubscriptionType values (CP_CHANGE, NEW_COMMENTS, MILESTONE,
STATUS_CHANGE) instead of relying on the external exists() checks referenced by
existing_subscription.
In `@users/migrations/0019_user_automatically_follow_on_predict_and_more.py`:
- Around line 6-23: The migrate() function that sets existing users'
automatically_follow_on_predict=False is defined but not executed; add a
migrations.RunPython step to operations so the backfill runs before/after the
AddField. Update the Migration.operations list to include
migrations.RunPython(migrate, migrations.RunPython.noop) (referencing the
migrate function name) alongside the existing migrations.AddField for
model_name="user" name="automatically_follow_on_predict" to ensure existing rows
are set to False instead of being backfilled to True.
In `@users/serializers.py`:
- Around line 191-196: The serializer in users/serializers.py that exposes the
follow-default fields ("automatically_follow_on_predict",
"follow_notify_cp_change_threshold", "follow_notify_comments_frequency",
"follow_notify_milestone_step", "follow_notify_on_status_change",
"metaculus_news_subscription") must validate incoming values before save:
replace or augment the current writable field definitions with explicit DRF
validators (e.g. serializers.IntegerField with min_value and max_value or
serializers.ChoiceField for preset enums) or add model validators on the
corresponding model fields; ensure numeric fields reject 0 and non-integers
(e.g. use IntegerField to prevent 1.5), allow null only when intended, and
include clear error messages so invalid values are rejected by the
ProfileSerializer (or the serializer class in users/serializers.py) before
persisting.
---
Outside diff comments:
In
`@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx:
- Around line 97-123: The toggle handler and render currently read and diff
against the stale user.unsubscribed_mailing_tags prop (in
handleEmailSubscriptionChange), causing racey undos; create a local state like
unsubscribedMailingTags initialized from user.unsubscribed_mailing_tags, render
switches from that state, and in handleEmailSubscriptionChange (and the places
referenced at 231-247, 281-290) compute the new set from
unsubscribedMailingTags, immediately update unsubscribedMailingTags
optimistically and setLoadingTag, then call updateProfileAction (via
updateMailingTag / useServerAction) and reconcile on success/failure — keep
references to handleEmailSubscriptionChange, updateProfileAction,
updateMailingTag, isMailingTagPending so the UI uses the local state and not the
original user prop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5333c05b-70d1-4882-97c0-154dfd62a0cd
📒 Files selected for processing (20)
front_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonfront_end/src/app/(main)/accounts/profile/actions.tsxfront_end/src/app/(main)/accounts/settings/notifications/components/email_notifications.tsxfront_end/src/app/(main)/accounts/settings/notifications/components/question_notifications.tsxfront_end/src/app/(main)/accounts/settings/notifications/page.tsxfront_end/src/services/api/profile/profile.server.tsfront_end/src/types/notifications.tsfront_end/src/types/users.tsnotifications/constants.pyquestions/services/forecasts.pytests/unit/test_questions/test_views.pyusers/migrations/0019_user_automatically_follow_on_predict_and_more.pyusers/models.pyusers/serializers.pyusers/views.py
💤 Files with no reviewable changes (1)
- front_end/src/app/(main)/accounts/settings/notifications/components/question_notifications.tsx
front_end/src/app/(main)/accounts/settings/notifications/components/email_notifications.tsx
Outdated
Show resolved
Hide resolved
front_end/src/app/(main)/accounts/settings/notifications/components/email_notifications.tsx
Show resolved
Hide resolved
| if created and user.automatically_follow_on_predict: | ||
| post = question.post | ||
| existing_subscription = post.subscriptions.filter(user=user).exclude( | ||
| is_global=True | ||
| ) | ||
| if ( | ||
| user.follow_notify_cp_change_threshold | ||
| and not existing_subscription.filter( | ||
| type=PostSubscription.SubscriptionType.CP_CHANGE | ||
| ).exists() | ||
| ): | ||
| create_subscription( | ||
| subscription_type=PostSubscription.SubscriptionType.CP_CHANGE, | ||
| user=user, | ||
| post=post, | ||
| cp_change_threshold=user.follow_notify_cp_change_threshold, | ||
| ) | ||
| if ( | ||
| user.follow_notify_comments_frequency | ||
| and not existing_subscription.filter( | ||
| type=PostSubscription.SubscriptionType.NEW_COMMENTS | ||
| ).exists() | ||
| ): | ||
| create_subscription( | ||
| subscription_type=PostSubscription.SubscriptionType.NEW_COMMENTS, | ||
| user=user, | ||
| post=post, | ||
| comments_frequency=user.follow_notify_comments_frequency, | ||
| ) | ||
| if ( | ||
| user.follow_notify_milestone_step | ||
| and not existing_subscription.filter( | ||
| type=PostSubscription.SubscriptionType.MILESTONE | ||
| ).exists() | ||
| ): | ||
| create_subscription( | ||
| subscription_type=PostSubscription.SubscriptionType.MILESTONE, | ||
| user=user, | ||
| post=post, | ||
| milestone_step=user.follow_notify_milestone_step, | ||
| ) | ||
| if ( | ||
| user.follow_notify_on_status_change | ||
| and not existing_subscription.filter( | ||
| type=PostSubscription.SubscriptionType.STATUS_CHANGE | ||
| ).exists() | ||
| ): | ||
| create_subscription( | ||
| subscription_type=PostSubscription.SubscriptionType.STATUS_CHANGE, | ||
| user=user, | ||
| post=post, | ||
| ) |
There was a problem hiding this comment.
Don’t gate auto-follow on withdrawal-reminder eligibility.
This block is unreachable for the common “normal forecast” path. Because end_time falls back to start_time above, Line 273 returns for every forecast without an explicit end_time; Line 269 also exits when the forecast ends at or after scheduled_close_time. That means automatically_follow_on_predict only runs for a narrow subset of expiring forecasts instead of all new predictions. Please move the follow-subscription creation before those returns, or split it into a helper that runs unconditionally on created=True.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@questions/services/forecasts.py` around lines 292 - 343, The auto-follow
subscription creation guarded by created and
user.automatically_follow_on_predict (the block using post = question.post,
existing_subscription, PostSubscription.SubscriptionType.* and
create_subscription) is currently placed after early returns that prevent it
from running for normal forecasts; extract these checks into a helper (e.g.,
ensure_auto_follow_subscriptions(created, user, question)) or move the entire
block to execute immediately when created is True (before the early returns),
and use the same existing_subscription and create_subscription calls to add
CP_CHANGE, NEW_COMMENTS, MILESTONE, and STATUS_CHANGE subscriptions only if they
don't already exist.
There was a problem hiding this comment.
@lsabor can you please check that? It might not be for every forecast but still looked like it can block subscriptions for some cases.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| existing_subscription = post.subscriptions.filter(user=user).exclude( | ||
| is_global=True | ||
| ) | ||
| if ( | ||
| user.follow_notify_cp_change_threshold | ||
| and not existing_subscription.filter( | ||
| type=PostSubscription.SubscriptionType.CP_CHANGE | ||
| ).exists() | ||
| ): | ||
| create_subscription( | ||
| subscription_type=PostSubscription.SubscriptionType.CP_CHANGE, | ||
| user=user, | ||
| post=post, | ||
| cp_change_threshold=user.follow_notify_cp_change_threshold, | ||
| ) | ||
| if ( | ||
| user.follow_notify_comments_frequency | ||
| and not existing_subscription.filter( | ||
| type=PostSubscription.SubscriptionType.NEW_COMMENTS | ||
| ).exists() | ||
| ): | ||
| create_subscription( | ||
| subscription_type=PostSubscription.SubscriptionType.NEW_COMMENTS, | ||
| user=user, | ||
| post=post, | ||
| comments_frequency=user.follow_notify_comments_frequency, | ||
| ) | ||
| if ( | ||
| user.follow_notify_milestone_step | ||
| and not existing_subscription.filter( | ||
| type=PostSubscription.SubscriptionType.MILESTONE | ||
| ).exists() | ||
| ): | ||
| create_subscription( | ||
| subscription_type=PostSubscription.SubscriptionType.MILESTONE, | ||
| user=user, | ||
| post=post, | ||
| milestone_step=user.follow_notify_milestone_step, | ||
| ) | ||
| if ( | ||
| user.follow_notify_on_status_change | ||
| and not existing_subscription.filter( | ||
| type=PostSubscription.SubscriptionType.STATUS_CHANGE | ||
| ).exists() | ||
| ): | ||
| create_subscription( | ||
| subscription_type=PostSubscription.SubscriptionType.STATUS_CHANGE, | ||
| user=user, | ||
| post=post, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the PostSubscription model definition
find . -type f -name "*.py" | xargs grep -l "class PostSubscription" | head -5Repository: Metaculus/metaculus
Length of output: 80
🏁 Script executed:
# Find the posts/services/subscriptions.py file mentioned in the review
find . -type f -path "*/posts/services/subscriptions.py"Repository: Metaculus/metaculus
Length of output: 96
🏁 Script executed:
# Also check the questions/services/forecasts.py file structure
find . -type f -path "*/questions/services/forecasts.py" | head -1Repository: Metaculus/metaculus
Length of output: 96
🏁 Script executed:
# Check the PostSubscription model for unique constraints
cat -n ./posts/models.py | grep -A 30 "class PostSubscription"Repository: Metaculus/metaculus
Length of output: 1581
🏁 Script executed:
# Check the full create_subscription function in posts/services/subscriptions.py
cat -n ./posts/services/subscriptions.py | head -100Repository: Metaculus/metaculus
Length of output: 4471
🏁 Script executed:
# Get more of the PostSubscription model to see constraints
cat -n ./posts/models.py | sed -n '960,1010p'Repository: Metaculus/metaculus
Length of output: 2406
🏁 Script executed:
# Find the create_subscription function definition
grep -n "def create_subscription" ./posts/services/subscriptions.pyRepository: Metaculus/metaculus
Length of output: 321
🏁 Script executed:
# Check the main create_subscription function
cat -n ./posts/services/subscriptions.py | sed -n '539,600p'Repository: Metaculus/metaculus
Length of output: 1900
🏁 Script executed:
# Check the factory functions to see if they use create() or get_or_create()
cat -n ./posts/services/subscriptions.py | sed -n '468,490p'Repository: Metaculus/metaculus
Length of output: 704
🏁 Script executed:
# Also check create_subscription_new_comments
cat -n ./posts/services/subscriptions.py | sed -n '452,467p'Repository: Metaculus/metaculus
Length of output: 558
🏁 Script executed:
# And create_subscription_status_change
cat -n ./posts/services/subscriptions.py | sed -n '508,518p'Repository: Metaculus/metaculus
Length of output: 389
🏁 Script executed:
# Look at the context around lines 294-343 in questions/services/forecasts.py
cat -n ./questions/services/forecasts.py | sed -n '280,350p'Repository: Metaculus/metaculus
Length of output: 3097
🏁 Script executed:
# Find what function contains this subscription creation code
grep -B 30 "user.follow_notify_cp_change_threshold" ./questions/services/forecasts.py | head -50Repository: Metaculus/metaculus
Length of output: 1516
🏁 Script executed:
# Find the function containing this subscription creation code
grep -B 50 "if created and user.automatically_follow_on_predict" ./questions/services/forecasts.py | grep "^def " | tail -1Repository: Metaculus/metaculus
Length of output: 45
🏁 Script executed:
# Get the full function signature and check for transaction handling
cat -n ./questions/services/forecasts.py | sed -n '200,295p' | grep -A 1 "^.*def "</parameter>Repository: Metaculus/metaculus
Length of output: 250
🏁 Script executed:
# More precise: look for def around line 250-280
cat -n ./questions/services/forecasts.py | sed -n '200,300p'Repository: Metaculus/metaculus
Length of output: 4553
🏁 Script executed:
# Check if update_forecast_notification has transaction decorator
cat -n ./questions/services/forecasts.py | sed -n '230,245p'Repository: Metaculus/metaculus
Length of output: 838
🏁 Script executed:
# Find where update_forecast_notification is called from
grep -n "update_forecast_notification" ./questions/services/forecasts.pyRepository: Metaculus/metaculus
Length of output: 262
🏁 Script executed:
# Check the create_forecast function for transaction handling
grep -B 10 "update_forecast_notification" ./questions/services/forecasts.py | grep -E "(def |@transaction|atomic)" | head -20Repository: Metaculus/metaculus
Length of output: 174
🏁 Script executed:
# Check the create_forecast function around line 172 for transaction handling
cat -n ./questions/services/forecasts.py | sed -n '100,180p'Repository: Metaculus/metaculus
Length of output: 3409
🏁 Script executed:
# Search for transaction decorators in the file
grep -n "@transaction\|@atomic\|atomic(" ./questions/services/forecasts.pyRepository: Metaculus/metaculus
Length of output: 97
🏁 Script executed:
# Check what transaction.atomic() is being used for at line 494
cat -n ./questions/services/forecasts.py | sed -n '485,510p'Repository: Metaculus/metaculus
Length of output: 1275
🏁 Script executed:
# Check if create_forecast_bulk has any transaction handling
cat -n ./questions/services/forecasts.py | sed -n '160,175p'Repository: Metaculus/metaculus
Length of output: 676
🏁 Script executed:
# Look for imports of transaction at the top of the file
head -50 ./questions/services/forecasts.py | grep -i transactionRepository: Metaculus/metaculus
Length of output: 96
Use get_or_create to make subscription creation idempotent.
The code checks for existing subscriptions with .exists() then calls create_subscription() which invokes .objects.create() directly. PostSubscription has a unique constraint on (type, user_id, post_id, is_global). Two concurrent forecast submissions can both pass the existence check and race to insert, causing one to fail with IntegrityError after the forecast row has already been persisted.
Replace the existence checks and create_subscription() calls with get_or_create() logic in the subscription factory functions, or use ignore_conflicts=True in a bulk operation, to eliminate the race.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@questions/services/forecasts.py` around lines 294 - 343, The subscription
creation is racy because you check existence then call create_subscription which
uses direct .create(), risking IntegrityError under concurrent requests; update
the logic so create_subscription (or the underlying factory) uses get_or_create
(or update_or_create) keyed on (type, user, post, is_global) or wraps the create
in a try/except that handles IntegrityError by returning the existing
PostSubscription; specifically modify create_subscription (and any factory it
calls) to call PostSubscription.objects.get_or_create(...) for
PostSubscription.SubscriptionType values (CP_CHANGE, NEW_COMMENTS, MILESTONE,
STATUS_CHANGE) instead of relying on the external exists() checks referenced by
existing_subscription.
| def migrate(apps, schema_editor): | ||
| # automatically follow on predict should be set to false for existing users | ||
| User = apps.get_model("users", "User") | ||
| User.objects.all().update(automatically_follow_on_predict=False) | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("users", "0018_add_auth_revoked_at"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="user", | ||
| name="automatically_follow_on_predict", | ||
| field=models.BooleanField(default=True), | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the migration file
find . -name "0019_user_automatically_follow_on_predict_and_more.py" -type fRepository: Metaculus/metaculus
Length of output: 135
🏁 Script executed:
# Also search for the users migrations directory to understand context
fd "users.*migrations" -t d | head -5Repository: Metaculus/metaculus
Length of output: 45
🏁 Script executed:
# Look at the users migrations directory
ls -la users/migrations/ | grep -E "^-.*\.py$" | tail -20Repository: Metaculus/metaculus
Length of output: 1715
🏁 Script executed:
cat -n ./users/migrations/0019_user_automatically_follow_on_predict_and_more.pyRepository: Metaculus/metaculus
Length of output: 2255
🏁 Script executed:
# Search for related PRs or issues mentioning this migration or field
rg "automatically_follow_on_predict" --type py -l | head -20Repository: Metaculus/metaculus
Length of output: 202
🏁 Script executed:
# Check the User model to see the field definition
rg "automatically_follow_on_predict" -A 3 -B 1 users/models.pyRepository: Metaculus/metaculus
Length of output: 303
🏁 Script executed:
# Search for any follow-up PR numbers or related context
rg "4507|4499" --type py,md | head -20Repository: Metaculus/metaculus
Length of output: 92
🏁 Script executed:
# Search for any follow-up PR references or TODO comments related to this field
rg "automatically_follow_on_predict" -B 5 -A 5 --type py | grep -E "(TODO|FIXME|follow-up|PR|4[0-9]{3})"Repository: Metaculus/metaculus
Length of output: 45
🏁 Script executed:
# Check git log for this migration to see if there's a follow-up commit
cd . && git log --oneline --all -- users/migrations/0019_user_automatically_follow_on_predict_and_more.py | head -10Repository: Metaculus/metaculus
Length of output: 77
🏁 Script executed:
# Search for any references to the migration or field in PR/issue context
rg "RunPython" users/migrations/0019_user_automatically_follow_on_predict_and_more.pyRepository: Metaculus/metaculus
Length of output: 45
🏁 Script executed:
# Check how the field is used in the forecasts service
rg "automatically_follow_on_predict" -B 3 -A 3 questions/services/forecasts.pyRepository: Metaculus/metaculus
Length of output: 279
🏁 Script executed:
# Check the commit that touches this migration
git log --pretty=format:"%h %s" --all -- users/migrations/0019_user_automatically_follow_on_predict_and_more.py | head -5Repository: Metaculus/metaculus
Length of output: 77
🏁 Script executed:
# Check if there's a subsequent migration that might handle this
ls -la users/migrations/ | grep "^-" | tail -5Repository: Metaculus/metaculus
Length of output: 478
🏁 Script executed:
# Search for any RunPython usage in user migrations
rg "RunPython" users/migrations/ | head -10Repository: Metaculus/metaculus
Length of output: 577
🏁 Script executed:
# Check the git diff or any follow-up changes to understand intent
git log -p --all -S "automatically_follow_on_predict" -- users/migrations/ | head -100Repository: Metaculus/metaculus
Length of output: 45
🏁 Script executed:
# Check the commit details to see if there's any mention of follow-up
git show 7928048d --quiet || git show 7928048d | head -50Repository: Metaculus/metaculus
Length of output: 202
🏁 Script executed:
# Search for any TODO, FIXME, or comments about deferred backfill in this migration
cat -n ./users/migrations/0019_user_automatically_follow_on_predict_and_more.py | grep -E "(TODO|FIXME|defer|follow-up|PR)"Repository: Metaculus/metaculus
Length of output: 45
🏁 Script executed:
# Check if there's a follow-up migration file or any mention in recent PRs
fd "0020" users/migrations/Repository: Metaculus/metaculus
Length of output: 45
🏁 Script executed:
# Let me check one of the other migrations to see the exact pattern
cat -n users/migrations/0002_user_is_onboarding_complete.pyRepository: Metaculus/metaculus
Length of output: 1011
🏁 Script executed:
# Also check 0017 to see another example
cat -n users/migrations/0017_user_is_primary_bot_user_owner_and_more.py | head -50Repository: Metaculus/metaculus
Length of output: 2223
🏁 Script executed:
# Verify the RunPython pattern in 0017 fully
cat -n users/migrations/0017_user_is_primary_bot_user_owner_and_more.py | tail -30Repository: Metaculus/metaculus
Length of output: 1545
Add the missing RunPython step to wire the migration helper into operations.
The migrate() function is defined but never executed because it is not registered in operations. Without migrations.RunPython(migrate, migrations.RunPython.noop), the AddField(default=True) will backfill automatically_follow_on_predict=True for all existing users, inverting the intent stated in the helper comment ("automatically follow on predict should be set to false for existing users").
This pattern is established throughout the user migrations: 0002_user_is_onboarding_complete.py and 0017_user_is_primary_bot_user_owner_and_more.py both follow the same structure—define a migration function, then wire it with migrations.RunPython() in operations.
Suggested fix
operations = [
migrations.AddField(
model_name="user",
name="automatically_follow_on_predict",
field=models.BooleanField(default=True),
),
+ migrations.RunPython(migrate, migrations.RunPython.noop),
migrations.AddField(
model_name="user",
name="follow_notify_comments_frequency",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@users/migrations/0019_user_automatically_follow_on_predict_and_more.py`
around lines 6 - 23, The migrate() function that sets existing users'
automatically_follow_on_predict=False is defined but not executed; add a
migrations.RunPython step to operations so the backfill runs before/after the
AddField. Update the Migration.operations list to include
migrations.RunPython(migrate, migrations.RunPython.noop) (referencing the
migrate function name) alongside the existing migrations.AddField for
model_name="user" name="automatically_follow_on_predict" to ensure existing rows
are set to False instead of being backfilled to True.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| "automatically_follow_on_predict", | ||
| "follow_notify_cp_change_threshold", | ||
| "follow_notify_comments_frequency", | ||
| "follow_notify_milestone_step", | ||
| "follow_notify_on_status_change", | ||
| "metaculus_news_subscription", |
There was a problem hiding this comment.
Validate the new follow-default fields before saving them.
These settings are now writable through the profile API, but the serializer accepts any integer/float/null for them. That means clients can persist invalid defaults like follow_notify_comments_frequency=0 or follow_notify_milestone_step=1.5, and those bad values will later be copied into new subscriptions. Please add explicit serializer validation or model validators for the allowed ranges/presets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@users/serializers.py` around lines 191 - 196, The serializer in
users/serializers.py that exposes the follow-default fields
("automatically_follow_on_predict", "follow_notify_cp_change_threshold",
"follow_notify_comments_frequency", "follow_notify_milestone_step",
"follow_notify_on_status_change", "metaculus_news_subscription") must validate
incoming values before save: replace or augment the current writable field
definitions with explicit DRF validators (e.g. serializers.IntegerField with
min_value and max_value or serializers.ChoiceField for preset enums) or add
model validators on the corresponding model fields; ensure numeric fields reject
0 and non-integers (e.g. use IntegerField to prevent 1.5), allow null only when
intended, and include clear error messages so invalid values are rejected by the
ProfileSerializer (or the serializer class in users/serializers.py) before
persisting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
front_end/src/app/(main)/accounts/settings/notifications/components/email_notifications.tsx (1)
173-179:⚠️ Potential issue | 🟠 MajorRace condition:
savingDefaultFieldis never cleared after save completes.
updateDefaultFollowsetssavingDefaultFieldbut never resets it tonullafterrunDefaultFollowUpdatefinishes. This means:
- The pending indicator may stay visible indefinitely after
isDefaultFollowPendingbecomesfalse- Rapid clicks on different fields can issue overlapping writes where an earlier slow response could overwrite a later user selection
Consider either:
- Awaiting the action and clearing state in a finally block, or
- Disabling the controls while any default-follow save is pending
🔧 Suggested fix pattern
const updateDefaultFollow = useCallback( - (field: string, updates: Parameters<typeof updateProfileAction>[0]) => { + async (field: string, updates: Parameters<typeof updateProfileAction>[0]) => { setSavingDefaultField(field); - runDefaultFollowUpdate(updates); + try { + await runDefaultFollowUpdate(updates); + } finally { + setSavingDefaultField(null); + } }, [runDefaultFollowUpdate] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx around lines 173 - 179, The callback updateDefaultFollow sets savingDefaultField but never clears it, causing a stuck pending indicator and possible race overwrites; change updateDefaultFollow (and/or runDefaultFollowUpdate usage) to await the save action and clear setSavingDefaultField(null) in a finally block (or alternatively disable controls when isDefaultFollowPending is true) so savingDefaultField is reset after the save completes; reference the savingDefaultField state, updateDefaultFollow function, and runDefaultFollowUpdate action when making this change.
🧹 Nitpick comments (1)
front_end/src/app/(main)/accounts/settings/notifications/components/email_notifications.tsx (1)
101-104: Consider reverting optimistic state on error.
handleNewsChangeupdates local state optimistically before the server action completes. IfupdateProfileActionfails, the UI will show the new state but the server will retain the old value.💡 Pattern for reverting on error
const handleNewsChange = useCallback(async (checked: boolean) => { + const previousValue = newsSubscribed; setNewsSubscribed(checked); - await updateProfileAction({ metaculus_news_subscription: checked }); + try { + await updateProfileAction({ metaculus_news_subscription: checked }); + } catch { + setNewsSubscribed(previousValue); + } }, []);Note: A similar pattern could apply to
handleAutoFollowChangeat lines 139-142.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx around lines 101 - 104, handleNewsChange currently sets local state optimistically via setNewsSubscribed(checked) before awaiting updateProfileAction; change it to save the previous value, call updateProfileAction first (or await it) and only setNewsSubscribed(checked) on success, and if the action throws revert to the saved previous value (or set it in a catch block); apply the same pattern to handleAutoFollowChange (preserve previous auto-follow state, await updateProfileAction, then update local state or revert on error) so the UI reflects server state on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@front_end/messages/pt.json`:
- Around line 678-680: The CP-change size labels use masculine singular
Portuguese forms but must be feminine plural to agree with surrounding words
like "alterações"/"mudanças"; update the translation values for the keys
cpChangeSmall, cpChangeMedium, and cpChangeLarge to the feminine plural forms
("pequenas", "médias", "grandes") so the CP-change selector and sentence
templates render correctly.
---
Duplicate comments:
In
`@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx:
- Around line 173-179: The callback updateDefaultFollow sets savingDefaultField
but never clears it, causing a stuck pending indicator and possible race
overwrites; change updateDefaultFollow (and/or runDefaultFollowUpdate usage) to
await the save action and clear setSavingDefaultField(null) in a finally block
(or alternatively disable controls when isDefaultFollowPending is true) so
savingDefaultField is reset after the save completes; reference the
savingDefaultField state, updateDefaultFollow function, and
runDefaultFollowUpdate action when making this change.
---
Nitpick comments:
In
`@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx:
- Around line 101-104: handleNewsChange currently sets local state
optimistically via setNewsSubscribed(checked) before awaiting
updateProfileAction; change it to save the previous value, call
updateProfileAction first (or await it) and only setNewsSubscribed(checked) on
success, and if the action throws revert to the saved previous value (or set it
in a catch block); apply the same pattern to handleAutoFollowChange (preserve
previous auto-follow state, await updateProfileAction, then update local state
or revert on error) so the UI reflects server state on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b25e10d-2703-4272-b680-d9800ef53b03
📒 Files selected for processing (9)
front_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonfront_end/src/app/(main)/accounts/settings/notifications/components/email_notifications.tsxfront_end/src/components/post_subscribe/subscription_types_customisation/subscription_cp_change.tsxfront_end/src/components/ui/button_group.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- front_end/messages/es.json
- front_end/messages/zh-TW.json
- front_end/messages/zh.json
| const handleNewsChange = useCallback(async (checked: boolean) => { | ||
| setNewsSubscribed(checked); | ||
| await updateProfileAction({ metaculus_news_subscription: checked }); | ||
| }, []); |
There was a problem hiding this comment.
State is set optimistically before the server action completes, but there's no rollback on failure. If updateProfileAction throws, the switch shows the wrong state. Same applies to the default follow toggles. Either wrap in try/catch with rollback, or set state only after success.
| const handleDefaultFollowUpdate = useCallback( | ||
| async (updates: Parameters<typeof updateProfileAction>[0]) => { | ||
| await updateProfileAction(updates); | ||
| }, | ||
| [] | ||
| ); |
There was a problem hiding this comment.
handleDefaultFollowUpdate is just async (updates) => { await updateProfileAction(updates); } - it's an unnecessary wrapper. You could pass updateProfileAction directly to useServerAction.
3 participants
closes #3350
Summary by CodeRabbit
New Features
Improvements
Removed