Skip to content

[f] 241 - Update the last activity user only for user action #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
giahung68 merged 3 commits into from
Dec 10, 2024
Merged

[f] 241 - Update the last activity user only for user action #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51c02a1
[f] 241 - Update the last activity user only for user action
Dec 9, 2024
cb42594
[f] 241 - Update the initial last_user_activity
Dec 9, 2024
15b88c0
[f] 241 - Update get_snippets
Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion supabase/database/sql/get_snippets_function.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ BEGIN
SELECT
s.id,
s.recorded_at,
s.user_last_activity,
s.duration,
s.start_time,
s.end_time,
Expand Down Expand Up @@ -271,7 +272,11 @@ BEGIN
CASE
WHEN p_order_by = 'upvotes' THEN s.upvote_count + s.like_count
WHEN p_order_by = 'comments' THEN s.comment_count
WHEN p_order_by = 'activities' THEN EXTRACT(EPOCH FROM s.updated_at)
WHEN p_order_by = 'activities' THEN
CASE
WHEN s.user_last_activity IS NULL THEN 0
ELSE EXTRACT(EPOCH FROM s.user_last_activity)
END
WHEN p_order_by IS NULL OR p_order_by = 'latest' OR p_order_by = '' THEN EXTRACT(EPOCH FROM s.recorded_at)
ELSE EXTRACT(EPOCH FROM s.recorded_at)
END DESC,
Expand Down
30 changes: 30 additions & 0 deletions supabase/database/sql/update_last_user_activity.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
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
);
Comment on lines +1 to +30
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Dec 9, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

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.sql calculates MAX(activity_time) from historical records
  • Three triggers (update_snippet_comment_count, update_snippet_like_count, update_snippet_upvote_count) set user_last_activity = NOW()
  • The aggregate update could overwrite a more recent NOW() timestamp from triggers

Recommendations:

  • Add WHERE user_last_activity < calculated_max condition 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:

  1. Adding transaction isolation level controls
  2. 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

13 changes: 8 additions & 5 deletions supabase/database/sql/update_snippet_comment_count.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ CREATE OR REPLACE FUNCTION update_snippet_comment_count()
RETURNS TRIGGER AS $$
BEGIN
UPDATE snippets
SET comment_count = (
SELECT COUNT(*)
FROM comments
WHERE room_id = COALESCE(NEW.room_id, OLD.room_id)
)
SET
comment_count = (
SELECT COUNT(*)
FROM comments
WHERE room_id = COALESCE(NEW.room_id, OLD.room_id)
),
user_last_activity = NOW()
WHERE id = COALESCE(NEW.room_id, OLD.room_id);

RETURN NULL;
END;

$$ LANGUAGE plpgsql;
15 changes: 8 additions & 7 deletions supabase/database/sql/update_snippet_like_count.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
CREATE OR REPLACE FUNCTION update_snippet_like_count()
RETURNS TRIGGER AS $$
BEGIN
-- Update the like count for the affected snippet
UPDATE snippets
SET like_count = (
SELECT COUNT(*)
FROM user_like_snippets
WHERE snippet = COALESCE(NEW.snippet, OLD.snippet)
AND value = 1
)
SET
like_count = (
SELECT COUNT(*)
FROM user_like_snippets
WHERE snippet = COALESCE(NEW.snippet, OLD.snippet)
AND value = 1
),
user_last_activity = NOW()
Comment on lines +5 to +12
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Dec 9, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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

WHERE id = COALESCE(NEW.snippet, OLD.snippet);

RETURN NULL;
Expand Down
16 changes: 8 additions & 8 deletions supabase/database/sql/update_snippet_upvote_count.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,20 @@ RETURNS TRIGGER AS $$
DECLARE
snippet_id UUID;
BEGIN
-- Get the snippet_id from snippet_labels
SELECT snippet INTO snippet_id
FROM snippet_labels
WHERE id = COALESCE(NEW.snippet_label, OLD.snippet_label);

-- Update the upvote count for the affected snippet
IF snippet_id IS NOT NULL THEN
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
)
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()
Comment on lines +12 to +19
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Dec 9, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

WHERE id = snippet_id;
END IF;

Expand Down