[f] 241 - Update the last activity user only for user action#12
[f] 241 - Update the last activity user only for user action#12
Conversation
|
Warning Rate limit exceeded@giahung68 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request modifies four SQL functions: Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
supabase/database/sql/update_snippet_comment_count.sql(1 hunks)supabase/database/sql/update_snippet_like_count.sql(1 hunks)supabase/database/sql/update_snippet_upvote_count.sql(1 hunks)
🔇 Additional comments (1)
supabase/database/sql/update_snippet_comment_count.sql (1)
5-11:
Consider differentiating between user and system actions
The current implementation updates user_last_activity for both comment creation and deletion. However, based on the PR title "[f] 241 - Update the last activity user only for user action", we should consider:
- Comment deletion might be performed by admins/system
- We might want to update
user_last_activityonly for comment creation
Let's check if there are admin-related comment deletions:
Consider modifying the trigger to only update user_last_activity for INSERT operations:
UPDATE snippets
SET
comment_count = (
SELECT COUNT(*)
FROM comments
WHERE room_id = COALESCE(NEW.room_id, OLD.room_id)
),
- user_last_activity = NOW()
+ user_last_activity = CASE
+ WHEN TG_OP = 'INSERT' THEN NOW()
+ ELSE user_last_activity
+ END
WHERE id = COALESCE(NEW.room_id, OLD.room_id);| SET | ||
| like_count = ( | ||
| SELECT COUNT(*) | ||
| FROM user_like_snippets | ||
| WHERE snippet = COALESCE(NEW.snippet, OLD.snippet) | ||
| AND value = 1 | ||
| ), | ||
| user_last_activity = NOW() |
There was a problem hiding this comment.
Consider handling unlike actions differently
Similar to the comment count update, we should consider differentiating between like and unlike actions for updating user_last_activity. This aligns with the PR objective of updating last activity only for user actions.
Consider modifying the trigger to only update user_last_activity for like actions (when value = 1):
UPDATE snippets
SET
like_count = (
SELECT COUNT(*)
FROM user_like_snippets
WHERE snippet = COALESCE(NEW.snippet, OLD.snippet)
AND value = 1
),
- user_last_activity = NOW()
+ user_last_activity = CASE
+ WHEN TG_OP = 'INSERT' AND NEW.value = 1 THEN NOW()
+ ELSE user_last_activity
+ END
WHERE id = COALESCE(NEW.snippet, OLD.snippet);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SET | |
| like_count = ( | |
| SELECT COUNT(*) | |
| FROM user_like_snippets | |
| WHERE snippet = COALESCE(NEW.snippet, OLD.snippet) | |
| AND value = 1 | |
| ), | |
| user_last_activity = NOW() | |
| SET | |
| like_count = ( | |
| SELECT COUNT(*) | |
| FROM user_like_snippets | |
| WHERE snippet = COALESCE(NEW.snippet, OLD.snippet) | |
| AND value = 1 | |
| ), | |
| user_last_activity = CASE | |
| WHEN TG_OP = 'INSERT' AND NEW.value = 1 THEN NOW() | |
| ELSE user_last_activity | |
| END |
| SET | ||
| upvote_count = ( | ||
| SELECT COUNT(*) | ||
| FROM label_upvotes lu | ||
| JOIN snippet_labels sl ON lu.snippet_label = sl.id | ||
| WHERE sl.snippet = snippet_id | ||
| ), | ||
| user_last_activity = NOW() |
There was a problem hiding this comment.
Consider handling upvote removal differently
Similar to the previous files, we should differentiate between upvote and upvote removal actions for updating user_last_activity. This maintains consistency with the PR's objective.
Consider modifying the trigger to only update user_last_activity for upvote actions:
UPDATE snippets
SET
upvote_count = (
SELECT COUNT(*)
FROM label_upvotes lu
JOIN snippet_labels sl ON lu.snippet_label = sl.id
WHERE sl.snippet = snippet_id
),
- user_last_activity = NOW()
+ user_last_activity = CASE
+ WHEN TG_OP = 'INSERT' THEN NOW()
+ ELSE user_last_activity
+ END
WHERE id = snippet_id;Consider extracting the user activity update logic into a separate function to maintain consistency across all three triggers and make future modifications easier.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
supabase/database/sql/update_last_user_activity.sql (3)
8-12: Consider adding an index for better performance.This subquery joins
snippet_labelswithlabel_upvotes. Consider adding an index onsnippet_labels(snippet, id)to optimize the lookup performance.
26-26: Inconsistent column naming:room_idvssnippet.The
room_idcolumn in thecommentstable is used to reference snippets, while other tables usesnippet. This inconsistency could lead to confusion. Consider renaming for consistency.-WHERE room_id = s.id +WHERE snippet = s.id
19-20: Simplify the value comparison condition.The condition
value = 1 OR value = -1can be simplified usingIN.-AND (value = 1 OR value = -1) +AND value IN (1, -1)
| UPDATE snippets s | ||
| SET user_last_activity = ( | ||
| SELECT MAX(activity_time) | ||
| FROM ( | ||
| -- Get label upvotes timestamps | ||
| SELECT created_at as activity_time | ||
| FROM label_upvotes | ||
| WHERE snippet_label IN ( | ||
| SELECT id | ||
| FROM snippet_labels | ||
| WHERE snippet = s.id | ||
| ) | ||
|
|
||
| UNION ALL | ||
|
|
||
| -- Get likes timestamps | ||
| SELECT created_at as activity_time | ||
| FROM user_like_snippets | ||
| WHERE snippet = s.id | ||
| AND (value = 1 OR value = -1) | ||
|
|
||
| UNION ALL | ||
|
|
||
| SELECT created_at as activity_time | ||
| FROM comments | ||
| WHERE room_id = s.id | ||
| AND deleted_at is null | ||
|
|
||
| ) all_activities | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Race condition confirmed between aggregate and trigger-based updates of user_last_activity
The verification confirms a critical race condition:
- The aggregate update in
update_last_user_activity.sqlcalculates MAX(activity_time) from historical records - Three triggers (
update_snippet_comment_count,update_snippet_like_count,update_snippet_upvote_count) setuser_last_activity = NOW() - The aggregate update could overwrite a more recent NOW() timestamp from triggers
Recommendations:
- Add
WHERE user_last_activity < calculated_maxcondition to the aggregate update to prevent overwriting newer timestamps - Consider using transaction isolation level SERIALIZABLE for the aggregate update
- Long term: consolidate the activity tracking logic to avoid multiple update paths
🔗 Analysis chain
Potential race condition with concurrent updates.
The AI summary indicates that other functions (update_snippet_comment_count, update_snippet_like_count, update_snippet_upvote_count) also update user_last_activity using NOW(). This could lead to race conditions where this aggregate update overwrites more recent activity timestamps.
Consider:
- Adding transaction isolation level controls
- Implementing a comparison to only update if the new timestamp is more recent
Let's verify the other function implementations:
The search results confirm the review comment's concern. Let me check the full context of these functions and their triggers to understand the complete picture:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other functions updating user_last_activity
rg -A 5 "user_last_activity.*NOW\(\)" --type sql
Length of output: 1379
Script:
#!/bin/bash
# Get full content of the files
echo "=== update_snippet_comment_count.sql ==="
cat supabase/database/sql/update_snippet_comment_count.sql
echo -e "\n=== update_snippet_like_count.sql ==="
cat supabase/database/sql/update_snippet_like_count.sql
echo -e "\n=== update_snippet_upvote_count.sql ==="
cat supabase/database/sql/update_snippet_upvote_count.sql
Length of output: 1960
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
supabase/database/sql/get_snippets_function.sql (1)
Line range hint
3-11: Consider performance optimization and security implicationsAs this function grows in complexity, consider the following architectural improvements:
Performance:
- Consider materializing commonly used subqueries
- Add appropriate indexes for the filtering conditions
- Monitor query execution plan for potential bottlenecks
Security:
- The function uses SECURITY DEFINER, ensure all user input is properly validated
- Consider implementing row-level security as an additional safety layer
- Add input validation for page and page_size parameters
Maintainability:
- Consider breaking down complex filtering logic into separate functions
- Add function documentation explaining the security model
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
supabase/database/sql/get_snippets_function.sql(3 hunks)
🔇 Additional comments (2)
supabase/database/sql/get_snippets_function.sql (2)
36-36: LGTM: Addition of user_last_activity field
The field is correctly added to the SELECT list and will be used for activity-based sorting.
275-279: Verify NULL handling behavior for activity-based sorting
The current implementation places snippets with no activity (NULL user_last_activity) at the bottom of the list when sorting by activities. Please confirm if this aligns with the business requirements.
Alternative approaches could be:
- Place NULL activities at the top (using
COALESCE(EXTRACT(EPOCH FROM s.user_last_activity), 9999999999)) - Use the
recorded_attimestamp as a fallback (usingCOALESCE(s.user_last_activity, s.recorded_at))
✅ Verification successful
Current NULL handling for activity-based sorting is correct
The codebase shows that user_last_activity is consistently updated to NOW() whenever there's user interaction (comments, likes, upvotes) with a snippet. The update_last_user_activity.sql maintains this field by tracking all activity timestamps. Having inactive snippets (NULL user_last_activity) appear at the bottom of activity-based sorting is the correct behavior because:
- NULL values represent snippets with no user interactions
- The field is automatically updated on any user activity
- The sorting puts most recently active snippets at the top, which aligns with typical activity feed patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the distribution of user_last_activity values
# This will help understand the impact of NULL handling
ast-grep --pattern 'UPDATE snippets SET user_last_activity = $_'
Length of output: 66
Script:
#!/bin/bash
# Let's search for files containing user_last_activity to understand its usage
rg -l "user_last_activity"
# Then look for SQL files that might define or modify this column
fd -e sql | xargs rg "user_last_activity" -B 2 -A 2
Length of output: 2907
| 'current_page', page, | ||
| 'page_size', page_size, | ||
| 'total_pages', total_pages | ||
| 'total_pages', total_pages, |
There was a problem hiding this comment.
Fix: Remove trailing comma in JSON object
The trailing comma after 'total_pages' will cause a PostgreSQL syntax error. JSON object construction in PostgreSQL doesn't support trailing commas.
- 'total_pages', total_pages,
+ 'total_pages', total_pages📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'total_pages', total_pages, | |
| 'total_pages', total_pages |
giahung68
force-pushed
the
features/ver-227-update-last-user-activity
branch
from
ce14dee to
15b88c0
Compare
2 participants
Summary by CodeRabbit
New Features
Bug Fixes