Open
Conversation
- Denormalized key_factor_votes_score on the comments table - Added `exclude_bots_only_project` -
Contributor
|
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a denormalized, indexed Changes
Sequence DiagramssequenceDiagram
participant User
participant Frontend
participant API
participant KeyFactorVoteService as KeyFactorVoteService
participant KeyFactor
participant Comment
participant Database
User->>Frontend: Click vote on key factor
Frontend->>API: POST /key-factor-vote
API->>KeyFactorVoteService: key_factor_vote(...)
KeyFactorVoteService->>Database: create/update Vote
KeyFactorVoteService->>KeyFactor: recalc votes_score
KeyFactor->>Database: save(update_fields=["votes_score"])
KeyFactorVoteService->>Comment: comment.update_key_factor_votes_score()
Comment->>Database: prefetch key_factors and votes
Comment->>Comment: compute sum(mean(abs(v.score)) per key_factor)
Comment->>Database: save(update_fields=["key_factor_votes_score"])
KeyFactorVoteService-->>API: result
API-->>Frontend: 200 OK
Frontend-->>User: UI updates
sequenceDiagram
participant User
participant Frontend
participant CommentsAPI
participant FeedService as get_comments_feed
participant Database
User->>Frontend: Open comments feed (empty search)
Frontend->>CommentsAPI: GET /comments?exclude_bots=...&exclude_bots_only_project=true&sort=-key_factor_votes_score
CommentsAPI->>FeedService: get_comments_feed(...)
FeedService->>Database: filter out projects with BotLeaderboardStatus.BOTS_ONLY
FeedService->>Database: annotate/return comments with annotated_key_factor_votes_score
Database-->>FeedService: Comment list
FeedService-->>CommentsAPI: serialized comments (include key_factor_votes_score)
CommentsAPI-->>Frontend: Response
Frontend->>Frontend: Render sorted feed by key_factor_votes_score
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
…nts' into feat/4510-comment-feed-improvements
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
comments/serializers/common.py (1)
85-86: Minor: Missing trailing comma for consistency.The tuple entries in
fieldstypically have trailing commas for cleaner diffs. Consider adding one after"key_factor_votes_score".✏️ Suggested fix
"is_pinned", - "key_factor_votes_score" + "key_factor_votes_score", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comments/serializers/common.py` around lines 85 - 86, The tuple of serializer fields ending with the string "key_factor_votes_score" in the fields definition should include a trailing comma for consistency and cleaner diffs; update the fields tuple (the variable/attribute named fields in the serializer/class) to add a comma after "key_factor_votes_score".comments/models.py (1)
3-3: Consider using Python's built-in for mean calculation instead of NumPy.Using
numpysolely for computing a simple mean adds a heavyweight dependency for minimal benefit. Python'sstatistics.meanor a manual calculation would be more appropriate here, especially since the list is small (votes per key factor).♻️ Proposed refactor to remove numpy dependency
Remove the numpy import at line 3:
-import numpy as npUpdate the method:
def update_key_factor_votes_score(self): score = 0.0 for kf in self.key_factors.prefetch_related("votes").all(): votes = [abs(v.score) for v in kf.votes.all()] if votes: - score += np.mean(votes) + score += sum(votes) / len(votes) self.key_factor_votes_score = score self.save(update_fields=["key_factor_votes_score"]) return scoreAlso applies to: 185-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comments/models.py` at line 3, The module currently imports NumPy solely for computing averages (import numpy as np) and uses np.mean in the votes-averaging logic; remove the heavy dependency by deleting the "import numpy as np" and replace all uses of np.mean (in the votes-per-key-factor routine) with Python's built-in calculation (either statistics.mean after adding "import statistics" or a simple sum(votes)/len(votes) with a guard for empty lists). Update the function(s) that compute the mean accordingly and ensure any related imports are adjusted and edge cases (empty lists) are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comments/migrations/0026_comment_key_factor_votes_score.py`:
- Around line 9-25: The migration keeps all matching Comment objects and related
votes in memory via the prefetch and the to_update list; instead process in
smaller chunks: iterate the Comment queryset in batches (e.g., using .iterator()
or by slicing with a limit/offset or by primary-key range), for each batch
compute score for each comment using comment.key_factors.all() / kf.votes.all()
as in the diff, collect updates into to_update for that batch, call
Comment.objects.bulk_update(to_update, ["key_factor_votes_score"],
batch_size=500) immediately for that batch, then clear to_update before
processing the next batch; reference the Comment model, key_factors, votes,
key_factor_votes_score, to_update and bulk_update when applying this change.
In `@front_end/src/components/comment_feed/comment_feed_content.tsx`:
- Around line 160-161: The current logic forces exclude_bots_only_project when
debouncedSearch is empty, which hides bots-only-project comments even if the
user set the visible filter to "Include bots"; update the filter construction so
exclude_bots_only_project is only applied when excludeBots is true (e.g. change
the spread from ...(!debouncedSearch && { exclude_bots_only_project: true }) to
guard with excludeBots) or alternatively expose exclude_bots_only_project as its
own UI state and use that flag instead; key symbols to update: debouncedSearch,
exclude_bots_only_project, and excludeBots within the object that builds the
query/filters.
---
Nitpick comments:
In `@comments/models.py`:
- Line 3: The module currently imports NumPy solely for computing averages
(import numpy as np) and uses np.mean in the votes-averaging logic; remove the
heavy dependency by deleting the "import numpy as np" and replace all uses of
np.mean (in the votes-per-key-factor routine) with Python's built-in calculation
(either statistics.mean after adding "import statistics" or a simple
sum(votes)/len(votes) with a guard for empty lists). Update the function(s) that
compute the mean accordingly and ensure any related imports are adjusted and
edge cases (empty lists) are handled.
In `@comments/serializers/common.py`:
- Around line 85-86: The tuple of serializer fields ending with the string
"key_factor_votes_score" in the fields definition should include a trailing
comma for consistency and cleaner diffs; update the fields tuple (the
variable/attribute named fields in the serializer/class) to add a comma after
"key_factor_votes_score".
🪄 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: e1f0e716-a48b-478b-a23f-2304b03a4697
📒 Files selected for processing (12)
comments/migrations/0026_comment_key_factor_votes_score.pycomments/models.pycomments/serializers/common.pycomments/services/common.pycomments/services/feed.pycomments/services/key_factors/common.pyfront_end/src/components/comment_feed/comment_feed_card.tsxfront_end/src/components/comment_feed/comment_feed_content.tsxfront_end/src/services/api/comments/comments.shared.tsfront_end/src/types/comment.tsprojects/migrations/0023_alter_project_bot_leaderboard_status.pyprojects/models.py
Comment on lines
+9
to
+25
| comments = ( | ||
| Comment.objects.filter(key_factors__votes__isnull=False) | ||
| .distinct() | ||
| .prefetch_related("key_factors__votes") | ||
| ) | ||
| to_update = [] | ||
| for comment in comments: | ||
| score = 0.0 | ||
| for kf in comment.key_factors.all(): | ||
| votes = [abs(v.score) for v in kf.votes.all()] | ||
| if votes: | ||
| score += np.mean(votes) | ||
| if score: | ||
| comment.key_factor_votes_score = score | ||
| to_update.append(comment) | ||
|
|
||
| Comment.objects.bulk_update(to_update, ["key_factor_votes_score"], batch_size=500) |
Contributor
There was a problem hiding this comment.
This still holds the full backfill set in memory.
batch_size=500 only chunks the SQL update; to_update is still populated once, after prefetching every matching comment and related vote. On production-sized tables that is a risky migration shape. Please process comments in smaller chunks and flush bulk_update per chunk, or move the aggregation into SQL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@comments/migrations/0026_comment_key_factor_votes_score.py` around lines 9 -
25, The migration keeps all matching Comment objects and related votes in memory
via the prefetch and the to_update list; instead process in smaller chunks:
iterate the Comment queryset in batches (e.g., using .iterator() or by slicing
with a limit/offset or by primary-key range), for each batch compute score for
each comment using comment.key_factors.all() / kf.votes.all() as in the diff,
collect updates into to_update for that batch, call
Comment.objects.bulk_update(to_update, ["key_factor_votes_score"],
batch_size=500) immediately for that batch, then clear to_update before
processing the next batch; reference the Comment model, key_factors, votes,
key_factor_votes_score, to_update and bulk_update when applying this change.
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit