Conversation
WalkthroughThis pull request introduces several SQL changes to enhance database functionality. Key updates include SQL migration scripts to update the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SnippetDB as Snippets Database
participant CommentsDB as Comments Database
participant LikesDB as Likes Database
User->>LikesDB: Like/Unlike Snippet
LikesDB->>SnippetDB: Trigger update_like_count
SnippetDB-->>SnippetDB: Update like_count
User->>CommentsDB: Add/Delete Comment
CommentsDB->>SnippetDB: Trigger update_snippet_comment_count
SnippetDB-->>SnippetDB: Update comment_count
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: 13
🧹 Outside diff range and nitpick comments (11)
supabase/database/sql/comment_count_migration.sql (1)
1-7: Consider performance optimization for large tables.For large tables, consider batching the updates to prevent long-running transactions:
CREATE OR REPLACE PROCEDURE batch_update_comment_counts( batch_size INTEGER DEFAULT 1000 ) LANGUAGE plpgsql AS $$ DECLARE total_rows INTEGER; processed_rows INTEGER := 0; BEGIN SELECT COUNT(*) INTO total_rows FROM snippets; WHILE processed_rows < total_rows LOOP WITH batch AS ( SELECT id FROM snippets ORDER BY id LIMIT batch_size OFFSET processed_rows ) UPDATE snippets s SET comment_count = ( SELECT COUNT(*) FROM comments c WHERE c.room_id = s.id AND deleted_at is null ) WHERE s.id IN (SELECT id FROM batch); processed_rows := processed_rows + batch_size; RAISE NOTICE 'Processed % of % rows', LEAST(processed_rows, total_rows), total_rows; COMMIT; END LOOP; END; $$;You can then execute this as:
CALL batch_update_comment_counts(1000);supabase/database/sql/like_count_trigger.sql (1)
1-4: Consider monitoring trigger performance impact.Since this trigger will execute on every like operation, consider monitoring its performance impact on high-traffic deployments.
Recommendations:
- Ensure proper indexing exists on the
user_like_snippetstable columns used in the count calculation- Consider implementing a batch update strategy if performance becomes a concern
- Monitor trigger execution time in production
supabase/database/sql/like_count_migration.sql (1)
1-7: Consider batching the UPDATE operation for large datasetsFor large tables, updating all rows at once can cause long-running transactions and table locks. Consider batching the updates to reduce the impact on database performance.
Here's a suggested approach using batching:
DO $$ DECLARE batch_size INTEGER := 1000; max_id INTEGER; current_id INTEGER := 0; BEGIN SELECT MAX(id) INTO max_id FROM snippets; WHILE current_id <= max_id LOOP UPDATE snippets s SET like_count = ( SELECT COUNT(*) FROM user_like_snippets uls WHERE uls.snippet = s.id AND uls.value = 1 ) WHERE s.id > current_id AND s.id <= current_id + batch_size; current_id := current_id + batch_size; COMMIT; END LOOP; END $$;supabase/database/sql/update_snippet_like_count.sql (1)
5-12: Verify concurrent update handling.The current implementation might be susceptible to race conditions during concurrent updates. Consider adding a row-level lock to ensure consistency.
Consider this modification:
UPDATE snippets + SET like_count = ( + SELECT COUNT(*) + FROM user_like_snippets + WHERE snippet = COALESCE(NEW.snippet, OLD.snippet) + AND value = 1 + ) - WHERE id = COALESCE(NEW.snippet, OLD.snippet); + WHERE id = COALESCE(NEW.snippet, OLD.snippet) + FOR UPDATE;supabase/database/sql/update_snippet_comment_count.sql (1)
1-19: Consider soft delete implicationsIf comments implement soft delete, this trigger might need adjustment to handle both soft and hard deletes appropriately.
Consider:
- Adding a condition to check for soft deleted status
- Creating a separate trigger for soft delete operations
- Maintaining separate counters for active and deleted comments
Would you like me to provide an implementation that handles soft deletes?
supabase/database/sql/update_snippet_upvote_count.sql (1)
13-21: Add concurrency control mechanismThe update operation could be subject to race conditions in a concurrent environment. Consider adding explicit locking or using atomic operations.
IF snippet_id IS NOT NULL THEN + -- Lock the snippets row to prevent concurrent updates + PERFORM 1 FROM snippets WHERE id = snippet_id FOR UPDATE; 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 ) WHERE id = snippet_id; END IF;supabase/database/sql/get_filtering_options_function.sql (3)
79-83: Consider handling NULL radio_station_code case.While the label construction logic is good, it might be worth handling the edge case where
radio_station_codeis NULL despite the WHERE clause filter.Consider updating the CASE statement:
- 'label', CASE - WHEN radio_station_name IS NOT NULL - THEN radio_station_name || ' - ' || radio_station_code - ELSE radio_station_code - END, + 'label', CASE + WHEN radio_station_name IS NOT NULL AND radio_station_code IS NOT NULL + THEN radio_station_name || ' - ' || radio_station_code + WHEN radio_station_code IS NOT NULL + THEN radio_station_code + ELSE 'Unknown' + END,
71-75: Consider adding ORDER BY clause for consistent results.The sources query lacks explicit ordering, which might lead to inconsistent ordering of radio stations in the UI. Consider adding an ORDER BY clause for a consistent user experience.
SELECT DISTINCT radio_station_code, radio_station_name FROM public.audio_files - WHERE radio_station_code IS NOT NULL + WHERE radio_station_code IS NOT NULL + ORDER BY COALESCE(radio_station_name, radio_station_code)
71-83: Consider performance optimization for large datasets.For large datasets, the DISTINCT operation might impact performance. Consider creating an index if this becomes a bottleneck.
CREATE INDEX IF NOT EXISTS idx_audio_files_radio_station ON public.audio_files (radio_station_code, radio_station_name) WHERE radio_station_code IS NOT NULL;supabase/database/sql/get_snippets_function.sql (2)
83-88: Consider optimizing the admin visibility filterThe nested EXISTS clause could be simplified for better readability and potentially better performance.
Consider this alternative:
- user_is_admin OR - NOT EXISTS ( - SELECT 1 - FROM user_hide_snippets uhs - WHERE uhs.snippet = s.id - ) + user_is_admin OR uhs.snippet IS NULLThis leverages the already existing LEFT JOIN to user_hide_snippets instead of using a subquery.
255-273: Simplify ordering logicThe current implementation uses multiple CASE statements with NULL handling which makes it harder to maintain.
Consider this more concise approach:
- ORDER BY - CASE - WHEN p_order_by = 'upvotes' THEN (s.upvote_count + s.like_count) - ELSE NULL - END DESC NULLS LAST, - CASE - WHEN p_order_by = 'comments' THEN s.comment_count - ELSE NULL - END DESC NULLS LAST, - CASE - WHEN p_order_by = 'activities' THEN s.updated_at - ELSE NULL - END DESC NULLS LAST, - CASE - WHEN p_order_by IS NULL OR p_order_by = 'latest' OR p_order_by = '' THEN s.recorded_at - ELSE NULL - END DESC NULLS LAST, - -- Default fallback to recorded_at if no other conditions match - s.recorded_at DESC NULLS LAST; + ORDER BY + CASE p_order_by + WHEN 'upvotes' THEN (s.upvote_count + s.like_count) + WHEN 'comments' THEN s.comment_count + WHEN 'activities' THEN extract(epoch from s.updated_at) + ELSE extract(epoch from s.recorded_at) + END DESC;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
supabase/database/sql/comment_count_migration.sql(1 hunks)supabase/database/sql/get_filtering_options_function.sql(1 hunks)supabase/database/sql/get_snippets_function.sql(5 hunks)supabase/database/sql/like_count_migration.sql(1 hunks)supabase/database/sql/like_count_trigger.sql(1 hunks)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 (9)
supabase/database/sql/like_count_trigger.sql (2)
1-4: LGTM! The trigger implementation looks correct.
The trigger timing (AFTER) and row-level execution are appropriate for maintaining like counts.
Let's verify the existence of the dependent function and related objects:
✅ Verification successful
Trigger implementation is well-designed and dependencies are in place
The verification confirms:
- The
update_snippet_like_count()function exists and is properly implemented to handle like count updates - An index
idx_snippets_like_countexists on the targetsnippets.like_countcolumn for efficient querying - The function implementation includes proper handling of both NEW and OLD record references, ensuring concurrency safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of dependent function and proper indexing
# Check for the function definition
rg -A 10 "CREATE.*FUNCTION.*update_snippet_like_count"
# Check for indexes on the tables involved
rg -A 5 "CREATE.*INDEX.*user_like_snippets"
Length of output: 997
Script:
#!/bin/bash
# Let's check for any indexes on user_like_snippets table using a broader search
rg -A 5 "CREATE.*INDEX.*like"
# Also check table definition to understand the structure
rg -A 10 "CREATE TABLE.*user_like_snippets"
# Check if there are any migrations related to indexes
fd -e sql | xargs rg "INDEX.*user_like_snippets"
Length of output: 242
1-4: Verify concurrent update handling.
Ensure that the update_snippet_like_count() function handles concurrent updates safely to prevent race conditions.
Let's check the function implementation for proper locking:
✅ Verification successful
Let me gather more information about the update_snippet_like_count function implementation:
The like count update implementation is safe for concurrent operations
The current implementation is safe from race conditions because:
- The trigger function uses a single atomic UPDATE operation
- The COUNT aggregation is performed within the same transaction as the UPDATE
- PostgreSQL's MVCC (Multi-Version Concurrency Control) ensures transaction isolation
The implementation correctly maintains consistency even under concurrent updates, inserts, or deletes of likes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper concurrency handling in the function
# Look for locking mechanisms or FOR UPDATE clauses
rg -A 10 "FOR UPDATE|LOCK TABLE|SELECT.*FROM.*snippets.*WHERE"
Length of output: 6134
Script:
#!/bin/bash
# Search for the update_snippet_like_count function implementation
ast-grep --pattern 'CREATE OR REPLACE FUNCTION update_snippet_like_count()'
# Also try with rg to ensure we don't miss it
rg -A 15 "CREATE.*FUNCTION update_snippet_like_count"
Length of output: 1387
supabase/database/sql/like_count_migration.sql (1)
9-9: 🛠️ Refactor suggestion
Consider using CREATE INDEX CONCURRENTLY
Creating an index can block writes to the table. Using CONCURRENTLY allows the index to be built without preventing concurrent inserts, updates, or deletes.
-CREATE INDEX idx_snippets_like_count ON snippets(like_count DESC);
+CREATE INDEX CONCURRENTLY idx_snippets_like_count ON snippets(like_count DESC);Note: The CONCURRENTLY option takes longer but is safer for production environments.
Let's check the table size to assess the impact:
supabase/database/sql/update_snippet_like_count.sql (3)
1-2: LGTM! Function signature is correctly defined.
The function is properly declared as a trigger function with the correct return type.
14-14: LGTM! Correct trigger function return value.
Returning NULL is the correct pattern for AFTER triggers.
9-10: Verify edge case handling for NULL values.
While COALESCE handles the basic NULL case, we should verify the behavior when:
- Both NEW and OLD are NULL
- The value column contains NULL instead of 0 or 1
Let's check the schema and constraints:
supabase/database/sql/get_filtering_options_function.sql (1)
71-74: LGTM! Enhanced radio station information retrieval.
The addition of radio_station_name to the DISTINCT selection provides more descriptive information about radio stations, improving the user experience.
supabase/database/sql/get_snippets_function.sql (2)
73-78: LGTM! Efficient like count calculation
The use of LATERAL join with filtered COUNT is an efficient way to calculate likes and dislikes in a single pass.
294-299: LGTM! Clear and consistent return structure
The return structure is well-organized and includes all necessary pagination information.
| UPDATE snippets s | ||
| SET comment_count = ( | ||
| SELECT COUNT(*) | ||
| FROM comments c | ||
| WHERE c.room_id = s.id | ||
| and deleted_at is null | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The SQL logic looks correct but consider adding safety measures.
While the UPDATE query is logically correct, consider these improvements for safer execution:
+ BEGIN;
+
+ -- Log start of migration
+ DO $$
+ BEGIN
+ RAISE NOTICE 'Starting comment count migration...';
+ END $$;
+
UPDATE snippets s
SET comment_count = (
SELECT COUNT(*)
FROM comments c
WHERE c.room_id = s.id
and deleted_at is null
);
+
+ -- Validate the update
+ DO $$
+ DECLARE
+ invalid_count INTEGER;
+ BEGIN
+ SELECT COUNT(*) INTO invalid_count
+ FROM snippets s
+ WHERE s.comment_count != (
+ SELECT COUNT(*)
+ FROM comments c
+ WHERE c.room_id = s.id
+ AND deleted_at is null
+ );
+
+ IF invalid_count > 0 THEN
+ RAISE EXCEPTION 'Validation failed: % snippets have incorrect comment counts', invalid_count;
+ END IF;
+
+ RAISE NOTICE 'Migration completed successfully';
+ END $$;
+
+ COMMIT;📝 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.
| UPDATE snippets s | |
| SET comment_count = ( | |
| SELECT COUNT(*) | |
| FROM comments c | |
| WHERE c.room_id = s.id | |
| and deleted_at is null | |
| ); | |
| BEGIN; | |
| -- Log start of migration | |
| DO $$ | |
| BEGIN | |
| RAISE NOTICE 'Starting comment count migration...'; | |
| END $$; | |
| UPDATE snippets s | |
| SET comment_count = ( | |
| SELECT COUNT(*) | |
| FROM comments c | |
| WHERE c.room_id = s.id | |
| and deleted_at is null | |
| ); | |
| -- Validate the update | |
| DO $$ | |
| DECLARE | |
| invalid_count INTEGER; | |
| BEGIN | |
| SELECT COUNT(*) INTO invalid_count | |
| FROM snippets s | |
| WHERE s.comment_count != ( | |
| SELECT COUNT(*) | |
| FROM comments c | |
| WHERE c.room_id = s.id | |
| AND deleted_at is null | |
| ); | |
| IF invalid_count > 0 THEN | |
| RAISE EXCEPTION 'Validation failed: % snippets have incorrect comment counts', invalid_count; | |
| END IF; | |
| RAISE NOTICE 'Migration completed successfully'; | |
| END $$; | |
| COMMIT; |
💡 Codebase verification
Migration needs concurrent update handling
The trigger function update_snippet_comment_count uses simple increment/decrement operations which could lead to race conditions during the migration. The migration's UPDATE query should use a transaction-safe approach to count comments. Consider using:
UPDATE snippets s
SET comment_count = (
SELECT COUNT(*)
FROM comments c
WHERE c.room_id = s.id
AND deleted_at is null
FOR UPDATE
);- The trigger function in
update_snippet_comment_count.sqluses non-atomiccomment_count + 1andcomment_count - 1operations - The migration query doesn't use any concurrency control mechanisms like
FOR UPDATE - Missing trigger creation statement that would bind this function to the comments table
🔗 Analysis chain
Verify concurrent update handling.
Ensure that the trigger function update_snippet_comment_count mentioned in the summary is properly handling concurrent updates during this migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's proper concurrent update handling in the trigger function
# Look for the trigger function definition and its concurrency controls
ast-grep --pattern 'CREATE OR REPLACE FUNCTION update_snippet_comment_count'
# Check if there are any other triggers or constraints on the comment_count column
rg -A 10 'CREATE TRIGGER.*comment_count'
rg -A 5 'ADD CONSTRAINT.*comment_count'
Length of output: 163
Script:
#!/bin/bash
# Let's try a broader search to find any trigger-related code
rg -i "trigger" supabase/database/sql/
# Search for any function that updates comment_count
rg -i "comment_count" supabase/database/sql/
# Look for any migration files that might contain trigger definitions
fd . supabase/database/sql/ -t f
Length of output: 2462
Script:
#!/bin/bash
# Let's examine the content of the trigger function
cat supabase/database/sql/update_snippet_comment_count.sql
# Also check if there's a trigger creation for this function
rg -A 5 "CREATE TRIGGER.*comment" supabase/database/sql/
Length of output: 575
| SET like_count = ( | ||
| SELECT COUNT(*) | ||
| FROM user_like_snippets | ||
| WHERE snippet = COALESCE(NEW.snippet, OLD.snippet) | ||
| AND value = 1 | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Performance optimization needed for user_like_snippets table
Based on the verification attempts, we couldn't find the table definition or any existing indexes for the user_like_snippets table in the codebase. This suggests that the recommended index on (snippet, value) is indeed missing, which could impact query performance for the COUNT operation.
- Add an index on (snippet, value) columns in the user_like_snippets table to optimize the COUNT query
- Consider implementing a materialized count if the table grows significantly
🔗 Analysis chain
Consider optimizing the COUNT query performance.
For large tables, counting all matching rows can become a performance bottleneck. Consider these optimizations:
- Add an index on (snippet, value) in the user_like_snippets table
- Consider maintaining a materialized count if performance becomes an issue
Let's verify if the required index exists:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the index exists on user_like_snippets table
rg -A 5 "CREATE.*INDEX.*user_like_snippets.*snippet.*value"
Length of output: 61
Script:
#!/bin/bash
# Let's check for any index definitions in the schema files
fd -e sql | xargs rg -i "CREATE.*INDEX|CREATE TABLE.*user_like_snippets"
Length of output: 3456
Script:
#!/bin/bash
# Let's check for the user_like_snippets table definition and any related indexes
rg -i "CREATE TABLE.*user_like_snippets" -A 10
Length of output: 48
Script:
#!/bin/bash
# Let's search for any mention of user_like_snippets table in migration files
rg -i "user_like_snippets" supabase/migrations/
Length of output: 47
| UPDATE snippets | ||
| SET comment_count = comment_count - 1 | ||
| WHERE id = OLD.room_id; |
There was a problem hiding this comment.
Prevent negative comment counts
The decrement operation should include a safety check to prevent negative counts.
- UPDATE snippets
- SET comment_count = comment_count - 1
- WHERE id = OLD.room_id;
+ UPDATE snippets
+ SET comment_count = GREATEST(0, comment_count - 1)
+ WHERE id = OLD.room_id;📝 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.
| UPDATE snippets | |
| SET comment_count = comment_count - 1 | |
| WHERE id = OLD.room_id; | |
| UPDATE snippets | |
| SET comment_count = GREATEST(0, comment_count - 1) | |
| WHERE id = OLD.room_id; |
| UPDATE snippets | ||
| SET comment_count = comment_count + 1 | ||
| WHERE id = NEW.room_id; |
There was a problem hiding this comment.
Prevent race conditions in counter updates
The direct increment operation could lead to race conditions in concurrent scenarios. Consider using atomic updates with proper locking.
- UPDATE snippets
- SET comment_count = comment_count + 1
- WHERE id = NEW.room_id;
+ UPDATE snippets
+ SET comment_count = (
+ SELECT COUNT(*)
+ FROM comments
+ WHERE room_id = NEW.room_id
+ )
+ WHERE id = NEW.room_id;📝 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.
| UPDATE snippets | |
| SET comment_count = comment_count + 1 | |
| WHERE id = NEW.room_id; | |
| UPDATE snippets | |
| SET comment_count = ( | |
| SELECT COUNT(*) | |
| FROM comments | |
| WHERE room_id = NEW.room_id | |
| ) | |
| WHERE id = NEW.room_id; |
| CREATE OR REPLACE FUNCTION update_snippet_comment_count() | ||
| RETURNS TRIGGER AS $$ | ||
| BEGIN | ||
| -- For INSERT | ||
| IF (TG_OP = 'INSERT') THEN | ||
| UPDATE snippets | ||
| SET comment_count = comment_count + 1 | ||
| WHERE id = NEW.room_id; | ||
|
|
||
| -- For DELETE | ||
| ELSIF (TG_OP = 'DELETE') THEN | ||
| UPDATE snippets | ||
| SET comment_count = comment_count - 1 | ||
| WHERE id = OLD.room_id; | ||
| END IF; | ||
|
|
||
| RETURN NULL; | ||
| END; | ||
| $$ LANGUAGE plpgsql; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and validation
The function should handle several edge cases:
- Non-existent snippets
- NULL values in NEW/OLD records
- Failed updates
CREATE OR REPLACE FUNCTION update_snippet_comment_count()
RETURNS TRIGGER AS $$
BEGIN
+ -- Validate input
+ IF (TG_OP = 'INSERT' AND NEW.room_id IS NULL) OR
+ (TG_OP = 'DELETE' AND OLD.room_id IS NULL) THEN
+ RAISE EXCEPTION 'room_id cannot be null';
+ END IF;
+
-- For INSERT
IF (TG_OP = 'INSERT') THEN
+ -- Verify snippet exists
+ IF NOT EXISTS (SELECT 1 FROM snippets WHERE id = NEW.room_id) THEN
+ RAISE EXCEPTION 'Snippet with id % does not exist', NEW.room_id;
+ END IF;
+
UPDATE snippets
- SET comment_count = comment_count + 1
+ SET comment_count = COALESCE(comment_count, 0) + 1
WHERE id = NEW.room_id;
+
+ IF NOT FOUND THEN
+ RAISE EXCEPTION 'Failed to update comment count for snippet %', NEW.room_id;
+ END IF;
-- For DELETE
ELSIF (TG_OP = 'DELETE') THEN
+ -- Verify snippet exists
+ IF NOT EXISTS (SELECT 1 FROM snippets WHERE id = OLD.room_id) THEN
+ RAISE EXCEPTION 'Snippet with id % does not exist', OLD.room_id;
+ END IF;
+
UPDATE snippets
- SET comment_count = comment_count - 1
+ SET comment_count = GREATEST(0, COALESCE(comment_count, 1) - 1)
WHERE id = OLD.room_id;
+
+ IF NOT FOUND THEN
+ RAISE EXCEPTION 'Failed to update comment count for snippet %', OLD.room_id;
+ END IF;
END IF;
RETURN NULL;
END;
$$ LANGUAGE plpgsql;📝 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.
| CREATE OR REPLACE FUNCTION update_snippet_comment_count() | |
| RETURNS TRIGGER AS $$ | |
| BEGIN | |
| -- For INSERT | |
| IF (TG_OP = 'INSERT') THEN | |
| UPDATE snippets | |
| SET comment_count = comment_count + 1 | |
| WHERE id = NEW.room_id; | |
| -- For DELETE | |
| ELSIF (TG_OP = 'DELETE') THEN | |
| UPDATE snippets | |
| SET comment_count = comment_count - 1 | |
| WHERE id = OLD.room_id; | |
| END IF; | |
| RETURN NULL; | |
| END; | |
| $$ LANGUAGE plpgsql; | |
| CREATE OR REPLACE FUNCTION update_snippet_comment_count() | |
| RETURNS TRIGGER AS $$ | |
| BEGIN | |
| -- Validate input | |
| IF (TG_OP = 'INSERT' AND NEW.room_id IS NULL) OR | |
| (TG_OP = 'DELETE' AND OLD.room_id IS NULL) THEN | |
| RAISE EXCEPTION 'room_id cannot be null'; | |
| END IF; | |
| -- For INSERT | |
| IF (TG_OP = 'INSERT') THEN | |
| -- Verify snippet exists | |
| IF NOT EXISTS (SELECT 1 FROM snippets WHERE id = NEW.room_id) THEN | |
| RAISE EXCEPTION 'Snippet with id % does not exist', NEW.room_id; | |
| END IF; | |
| UPDATE snippets | |
| SET comment_count = COALESCE(comment_count, 0) + 1 | |
| WHERE id = NEW.room_id; | |
| IF NOT FOUND THEN | |
| RAISE EXCEPTION 'Failed to update comment count for snippet %', NEW.room_id; | |
| END IF; | |
| -- For DELETE | |
| ELSIF (TG_OP = 'DELETE') THEN | |
| -- Verify snippet exists | |
| IF NOT EXISTS (SELECT 1 FROM snippets WHERE id = OLD.room_id) THEN | |
| RAISE EXCEPTION 'Snippet with id % does not exist', OLD.room_id; | |
| END IF; | |
| UPDATE snippets | |
| SET comment_count = GREATEST(0, COALESCE(comment_count, 1) - 1) | |
| WHERE id = OLD.room_id; | |
| IF NOT FOUND THEN | |
| RAISE EXCEPTION 'Failed to update comment count for snippet %', OLD.room_id; | |
| END IF; | |
| END IF; | |
| RETURN NULL; | |
| END; | |
| $$ LANGUAGE plpgsql; |
💡 Codebase verification
Missing trigger setup for comment count updates
The function update_snippet_comment_count() is defined but the corresponding trigger to execute this function is missing. While we found:
- A migration file that initializes comment counts
- Several other triggers in the system
- A similar pattern with the like count trigger
The trigger to execute update_snippet_comment_count() after INSERT or DELETE on the comments table needs to be created.
Add the following trigger definition:
CREATE TRIGGER update_comment_count
AFTER INSERT OR DELETE ON comments
FOR EACH ROW
EXECUTE FUNCTION update_snippet_comment_count();🔗 Analysis chain
Verify trigger setup and initialization
Let's verify the trigger setup and initial comment counts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify trigger setup and initialization
# Check if trigger exists and is properly configured
echo "Checking trigger configuration..."
rg -A 10 "CREATE TRIGGER.*update_snippet_comment_count"
# Look for comment_count column definition
echo "Checking comment_count column..."
rg -A 5 "comment_count.*snippets"
# Check for initialization/migration scripts
echo "Checking initialization scripts..."
fd -e sql | rg -i "migration|init.*comment.*count"
Length of output: 527
Script:
#!/bin/bash
# Let's check the content of these migration files and any trigger setup
echo "Checking comment count migration content..."
cat supabase/database/sql/comment_count_migration.sql
echo -e "\nChecking remote schema migration..."
cat supabase/migrations/20241029135348_remote_schema.sql | grep -A 10 -B 10 "comment_count\|update_snippet_comment_count"
echo -e "\nChecking snippets table definition..."
rg "CREATE TABLE.*snippets" -A 15
Length of output: 627
Script:
#!/bin/bash
# Let's try a different approach to find the trigger setup and table definition
# Search for trigger creation
echo "Searching for trigger creation..."
rg "CREATE TRIGGER" -A 5
# Look for snippets table with different patterns
echo -e "\nSearching for snippets table..."
rg "CREATE TABLE.*snippets|ALTER TABLE.*snippets" -A 15
# Check for comment_count column addition
echo -e "\nSearching for comment_count column addition..."
rg "ALTER TABLE.*ADD.*comment_count|comment_count.*INTEGER"
Length of output: 4025
| CREATE OR REPLACE FUNCTION update_snippet_upvote_count() | ||
| RETURNS TRIGGER AS $$ | ||
| DECLARE | ||
| snippet_id UUID; | ||
| BEGIN |
There was a problem hiding this comment.
Add trigger creation statement
The trigger function is defined but the corresponding trigger creation statement (CREATE TRIGGER) is missing. This should specify when the trigger executes (BEFORE/AFTER) and for which operations (INSERT/UPDATE/DELETE).
Add the trigger creation statement. Example:
CREATE TRIGGER update_snippet_upvote_count_trigger
AFTER INSERT OR DELETE OR UPDATE ON label_upvotes
FOR EACH ROW
EXECUTE FUNCTION update_snippet_upvote_count();| SELECT snippet INTO snippet_id | ||
| FROM snippet_labels | ||
| WHERE id = COALESCE(NEW.snippet_label, OLD.snippet_label); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid snippet_label
The query silently continues if the snippet_label is invalid. Consider adding explicit error handling to maintain data integrity.
SELECT snippet INTO snippet_id
FROM snippet_labels
WHERE id = COALESCE(NEW.snippet_label, OLD.snippet_label);
+ IF NOT FOUND THEN
+ RAISE EXCEPTION 'Invalid snippet_label: %', COALESCE(NEW.snippet_label, OLD.snippet_label);
+ END IF;📝 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.
| SELECT snippet INTO snippet_id | |
| FROM snippet_labels | |
| WHERE id = COALESCE(NEW.snippet_label, OLD.snippet_label); | |
| SELECT snippet INTO snippet_id | |
| FROM snippet_labels | |
| WHERE id = COALESCE(NEW.snippet_label, OLD.snippet_label); | |
| IF NOT FOUND THEN | |
| RAISE EXCEPTION 'Invalid snippet_label: %', COALESCE(NEW.snippet_label, OLD.snippet_label); | |
| END IF; |
| 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 | ||
| ) | ||
| WHERE id = snippet_id; |
There was a problem hiding this comment.
💡 Codebase verification
Add indexes to optimize label upvote queries
Based on the codebase analysis, the tables are frequently joined and queried, but only one index (idx_snippets_like_count) exists. The following indexes should be added to optimize the upvote count calculation and related queries:
snippet_labels(snippet): Used in multiple JOIN operations and WHERE clauseslabel_upvotes(snippet_label): Used in JOIN operations and COUNT querieslabel_upvotes(upvoted_by, snippet_label): Used in unique constraint and filtering
CREATE INDEX IF NOT EXISTS idx_snippet_labels_snippet ON snippet_labels(snippet);
CREATE INDEX IF NOT EXISTS idx_label_upvotes_snippet_label ON label_upvotes(snippet_label);
CREATE UNIQUE INDEX IF NOT EXISTS idx_label_upvotes_user_label ON label_upvotes(upvoted_by, snippet_label);🔗 Analysis chain
Optimize query performance with appropriate indexes
The COUNT subquery with JOIN could be performance-intensive. Ensure proper indexes exist on the join columns and frequently queried fields.
Let's verify the existing indexes:
Consider adding these indexes if not present:
CREATE INDEX IF NOT EXISTS idx_snippet_labels_snippet ON snippet_labels(snippet);
CREATE INDEX IF NOT EXISTS idx_label_upvotes_snippet_label ON label_upvotes(snippet_label);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing indexes on relevant columns
# Search for index definitions
rg -A 3 'CREATE.*INDEX' supabase/database/sql/
# Search for table definitions to verify primary keys
rg -A 10 'CREATE TABLE (snippet_labels|label_upvotes|snippets)' supabase/database/sql/
Length of output: 251
Script:
#!/bin/bash
# Let's search for table definitions in all SQL files
rg -A 10 'CREATE TABLE' supabase/database/sql/
# Also search for any foreign key constraints that might imply indexes
rg -B 2 -A 2 'FOREIGN KEY' supabase/database/sql/
# Search specifically for label_upvotes and snippet_labels related definitions
rg -A 10 '(label_upvotes|snippet_labels)' supabase/database/sql/
Length of output: 22235
| CREATE OR REPLACE FUNCTION update_snippet_upvote_count() | ||
| 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 | ||
| ) | ||
| WHERE id = snippet_id; | ||
| END IF; | ||
|
|
||
| RETURN NULL; | ||
| END; | ||
| $$ LANGUAGE plpgsql; |
There was a problem hiding this comment.
💡 Codebase verification
Missing trigger definition for upvote count updates
The function update_snippet_upvote_count() is defined but there's no corresponding trigger to execute it. The function needs to be triggered after INSERT, UPDATE, or DELETE operations on the label_upvotes table to maintain accurate upvote counts.
- Add a trigger definition similar to the like count trigger:
CREATE TRIGGER update_upvote_count
AFTER INSERT OR UPDATE OR DELETE ON label_upvotes
FOR EACH ROW
EXECUTE FUNCTION update_snippet_upvote_count();🔗 Analysis chain
Verify trigger execution order with related triggers
This trigger function is part of a set of related triggers (comment_count, like_count). Ensure proper trigger execution order to maintain data consistency.
Let me gather more information about the trigger definitions for all these functions.
Let me try one more search to find the trigger definition for the upvote count function and table definitions using a different pattern.
Let me try one final search with a different approach to find all SQL files and their content.
Let me check the content of the relevant files for trigger definitions and table relationships.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related triggers and their execution order
# Search for other trigger definitions
rg -A 5 'CREATE TRIGGER' supabase/database/sql/
# Search for related trigger functions
rg -A 5 'update_snippet.*_count\(\)' supabase/database/sql/
Length of output: 2003
Script:
#!/bin/bash
# Search for trigger definitions for all count-related functions
rg -A 3 'CREATE TRIGGER.*update_.*_count' supabase/database/sql/
# Search for table definitions to understand the relationships
rg -A 10 'CREATE TABLE (snippets|snippet_labels|label_upvotes)' supabase/database/sql/
Length of output: 482
Script:
#!/bin/bash
# Search for trigger definition for upvote count
rg -A 3 'CREATE TRIGGER' supabase/database/sql/update_snippet_upvote_count.sql
# Search for table definitions with a broader pattern
rg -A 10 'CREATE TABLE' supabase/database/sql/schema.sql
Length of output: 212
Script:
#!/bin/bash
# Find all SQL files in the database directory
fd -e sql . supabase/database/
# Search for trigger definition in all SQL files
rg -A 3 'CREATE TRIGGER.*upvote' supabase/database/
# Search for table definitions in all SQL files
rg -A 5 'CREATE TABLE' supabase/database/
Length of output: 1670
Script:
#!/bin/bash
# Check content of upvote-related files
cat supabase/database/sql/update_snippet_upvote_count.sql
cat supabase/database/sql/toggle_upvote_label_function.sql
# Check content of table definitions in migration files
cat supabase/database/sql/comment_count_migration.sql
cat supabase/database/sql/like_count_migration.sql
Length of output: 3621
| AND ( | ||
| p_filter IS NULL OR | ||
| NOT p_filter ? 'upvotedBy' OR | ||
| ( | ||
| CASE | ||
| WHEN jsonb_array_length(p_filter->'upvotedBy') = 0 THEN TRUE | ||
| ELSE ( | ||
| CASE | ||
| WHEN ( | ||
| p_filter->'upvotedBy' ? 'by_me' AND | ||
| p_filter->'upvotedBy' ? 'by_others' | ||
| ) THEN | ||
| EXISTS ( | ||
| SELECT 1 | ||
| FROM label_upvotes lu | ||
| WHERE lu.snippet_label IN ( | ||
| SELECT id FROM snippet_labels WHERE snippet = s.id | ||
| ) | ||
| ) | ||
| WHEN p_filter->'upvotedBy' ? 'by_me' THEN | ||
| EXISTS ( | ||
| SELECT 1 | ||
| FROM label_upvotes lu | ||
| WHERE lu.snippet_label IN ( | ||
| SELECT id FROM snippet_labels WHERE snippet = s.id | ||
| ) | ||
| AND lu.upvoted_by = current_user_id | ||
| ) | ||
| WHEN p_filter->'upvotedBy' ? 'by_others' THEN | ||
| EXISTS ( | ||
| SELECT 1 | ||
| FROM label_upvotes lu | ||
| WHERE lu.snippet_label IN ( | ||
| SELECT id FROM snippet_labels WHERE snippet = s.id | ||
| ) | ||
| AND lu.upvoted_by != current_user_id | ||
| ) | ||
| ELSE FALSE | ||
| END | ||
| ) | ||
| END | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize upvotedBy filter performance
The current implementation uses multiple similar subqueries which could impact performance with large datasets.
Consider using a CTE to improve performance:
+ WITH label_upvotes_info AS (
+ SELECT DISTINCT
+ sl.snippet,
+ bool_or(lu.upvoted_by = current_user_id) as upvoted_by_me,
+ bool_or(lu.upvoted_by != current_user_id) as upvoted_by_others
+ FROM snippet_labels sl
+ LEFT JOIN label_upvotes lu ON lu.snippet_label = sl.id
+ GROUP BY sl.snippet
+ )
-- Replace the entire upvotedBy filter with:
AND (
p_filter IS NULL OR
NOT p_filter ? 'upvotedBy' OR
EXISTS (
SELECT 1
FROM label_upvotes_info lui
WHERE lui.snippet = s.id
AND (
(p_filter->'upvotedBy' ? 'by_me' AND lui.upvoted_by_me) OR
(p_filter->'upvotedBy' ? 'by_others' AND lui.upvoted_by_others)
)
)
)Committable suggestion skipped: line range outside the PR's diff.
| AND uls.value = 1 | ||
| ); | ||
|
|
||
| CREATE INDEX idx_snippets_like_count ON snippets(like_count DESC); |
There was a problem hiding this comment.
Perhaps creating index for comment_count and upvote_count as well? 🤔
| -- For INSERT | ||
| IF (TG_OP = 'INSERT') THEN | ||
| UPDATE snippets | ||
| SET comment_count = comment_count + 1 |
There was a problem hiding this comment.
It's better to recalculate this (instead of incrementing by "1")
| ) | ||
| ORDER BY | ||
| CASE | ||
| WHEN p_order_by = 'upvotes' THEN (s.upvote_count + s.like_count) |
There was a problem hiding this comment.
Let's add some indentation here to make the case-when statement here more readable
|
Overall, this PR looks good to me. 💯 |
# Conflicts: # supabase/database/sql/get_snippets_function.sql
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)
83-88: Improve code formatting for better readabilityThe EXISTS clause indentation is inconsistent with the surrounding code.
AND ( -- If user is admin, show all snippets (including hidden ones) -- If user is not admin, only show non-hidden snippets - user_is_admin OR - NOT EXISTS ( - SELECT 1 - FROM user_hide_snippets uhs - WHERE uhs.snippet = s.id - ) + user_is_admin OR + NOT EXISTS ( + SELECT 1 + FROM user_hide_snippets uhs + WHERE uhs.snippet = s.id + ) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
supabase/database/sql/get_snippets_function.sql(6 hunks)
🔇 Additional comments (4)
supabase/database/sql/get_snippets_function.sql (4)
73-76: LGTM! Efficient implementation of like counts
The use of FILTER clause for counting likes/dislikes is an efficient approach.
213-254: LGTM with previous optimization suggestion
The upvotedBy filter implementation is functionally correct.
Referring to previous optimization suggestion using CTEs for better performance.
267-285: LGTM with previous validation suggestion
The ordering logic implementation is correct.
Referring to previous suggestion about adding input validation for p_order_by parameter.
306-311: LGTM! Improved return structure
The simplified return structure with direct use of total_count is more accurate and maintainable.
| CREATE | ||
| OR REPLACE FUNCTION get_snippets ( | ||
| page INTEGER DEFAULT 0, | ||
| page_size INTEGER DEFAULT 10, | ||
| p_language TEXT DEFAULT 'english', | ||
| p_filter JSONB DEFAULT '{}'::jsonb | ||
| ) RETURNS jsonb SECURITY DEFINER AS $$ | ||
| OR REPLACE FUNCTION get_snippets_order_by (p_language text,p_filter jsonb,page INTEGER,page_size INTEGER, p_order_by text) RETURNS jsonb SECURITY DEFINER AS $$ |
There was a problem hiding this comment.
Breaking changes in function signature require attention
The function signature changes will break existing callers:
- Removal of default values for parameters
- Changed parameter order
- Function renamed from
get_snippetstoget_snippets_order_by
Consider:
- Creating a new function instead of replacing the existing one
- Or maintaining backward compatibility by keeping default values:
-CREATE OR REPLACE FUNCTION get_snippets_order_by (p_language text,p_filter jsonb,page INTEGER,page_size INTEGER, p_order_by text)
+CREATE OR REPLACE FUNCTION get_snippets_order_by (
+ p_language text DEFAULT 'english',
+ p_filter jsonb DEFAULT '{}'::jsonb,
+ page INTEGER DEFAULT 0,
+ page_size INTEGER DEFAULT 10,
+ p_order_by text DEFAULT 'latest'
+)📝 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.
| CREATE | |
| OR REPLACE FUNCTION get_snippets ( | |
| page INTEGER DEFAULT 0, | |
| page_size INTEGER DEFAULT 10, | |
| p_language TEXT DEFAULT 'english', | |
| p_filter JSONB DEFAULT '{}'::jsonb | |
| ) RETURNS jsonb SECURITY DEFINER AS $$ | |
| OR REPLACE FUNCTION get_snippets_order_by (p_language text,p_filter jsonb,page INTEGER,page_size INTEGER, p_order_by text) RETURNS jsonb SECURITY DEFINER AS $$ | |
| CREATE | |
| OR REPLACE FUNCTION get_snippets_order_by ( | |
| p_language text DEFAULT 'english', | |
| p_filter jsonb DEFAULT '{}'::jsonb, | |
| page INTEGER DEFAULT 0, | |
| page_size INTEGER DEFAULT 10, | |
| p_order_by text DEFAULT 'latest' | |
| ) RETURNS jsonb SECURITY DEFINER AS $$ |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
supabase/database/sql/upvote_count_migration.sql (1)
1-13: Ensure transaction safety for the migrationThis migration performs a bulk update of the
snippetstable. While most migration systems automatically wrap migrations in transactions, it's worth verifying that this is the case in your setup to ensure atomic updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
supabase/database/sql/comment_count_migration.sql(1 hunks)supabase/database/sql/update_snippet_comment_count.sql(1 hunks)supabase/database/sql/upvote_count_migration.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/database/sql/comment_count_migration.sql
🔇 Additional comments (2)
supabase/database/sql/upvote_count_migration.sql (2)
13-13: Verify index usage and existing indexes
The descending index will optimize sorting by upvote_count, but we should verify:
- That there are no overlapping indexes
- That the queries will actually use this index
✅ Verification successful
Index creation is appropriate and will be utilized
The index creation is verified to be beneficial because:
- No overlapping indexes exist on the
upvote_countcolumn - The index will be used by the
get_snippets_function.sqlwhich sorts byupvote_countwhenp_order_by = 'upvotes' - The column is actively maintained by a trigger function
update_snippet_upvote_count()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing indexes on snippets table
rg -A 5 "CREATE.*INDEX.*ON snippets"
# Check if there are ORDER BY upvote_count queries that will use this index
rg -A 5 "ORDER BY.*upvote_count"
Length of output: 436
Script:
#!/bin/bash
# Let's check SQL files and views for queries that might use upvote_count in WHERE or ORDER BY
rg -t sql "upvote_count" -B 2 -A 2
# Also check for any functions or stored procedures that might use upvote_count
ast-grep --pattern 'CREATE FUNCTION $$$upvote_count$$$'
# Let's also check application code for potential query usage
rg "upvote_count" -g "!*.sql" -B 2 -A 2
Length of output: 1995
1-7: Verify handling of soft-deleted records
The CTE logic for counting upvotes is correct, but we should verify if there's a need to handle soft-deleted records in either snippet_labels or label_upvotes tables.
✅ Verification successful
Based on the schema information from the output, I can now provide a definitive response about the soft deletion concern:
No need to handle soft-deleted records
The schema shows that neither snippet_labels nor label_upvotes tables have soft delete columns (like deleted_at or is_deleted). The only mention of deleted_at in the codebase is in an unrelated comment_count_migration.sql file. Therefore, the current upvote counting logic in the CTE is correct as is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these tables have soft delete columns
rg -A 5 "CREATE TABLE (snippet_labels|label_upvotes)"
Length of output: 55
Script:
#!/bin/bash
# Let's try a broader search for table definitions
fd -e sql | xargs rg -l "CREATE TABLE"
Length of output: 40
Script:
#!/bin/bash
# Search for any mentions of deleted/active status in SQL files
fd -e sql | xargs rg -i "(deleted_at|is_deleted|active|status)"
Length of output: 3659
Script:
#!/bin/bash
# Look for table definitions in schema files
fd "schema|table" -e sql | xargs cat
Length of output: 61650
| 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) | ||
| ) | ||
| WHERE id = COALESCE(NEW.room_id, OLD.room_id); | ||
|
|
||
| RETURN NULL; | ||
| END; | ||
| $$ LANGUAGE plpgsql; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling
While the recalculation approach is good, the function needs additional safeguards:
CREATE OR REPLACE FUNCTION update_snippet_comment_count()
RETURNS TRIGGER AS $$
BEGIN
+ -- Validate input
+ IF (TG_OP = 'INSERT' AND NEW.room_id IS NULL) OR
+ (TG_OP = 'DELETE' AND OLD.room_id IS NULL) THEN
+ RAISE EXCEPTION 'room_id cannot be null';
+ END IF;
+
+ -- Verify snippet exists
+ IF NOT EXISTS (
+ SELECT 1
+ FROM snippets
+ WHERE id = COALESCE(NEW.room_id, OLD.room_id)
+ ) THEN
+ RAISE EXCEPTION 'Snippet with id % does not exist',
+ COALESCE(NEW.room_id, OLD.room_id);
+ END IF;
+
UPDATE snippets
SET comment_count = (
SELECT COUNT(*)
FROM comments
- WHERE room_id = COALESCE(NEW.room_id, OLD.room_id)
+ WHERE room_id = COALESCE(NEW.room_id, OLD.room_id)
+ AND deleted_at IS NULL -- Only count non-deleted comments
)
WHERE id = COALESCE(NEW.room_id, OLD.room_id);
+
+ IF NOT FOUND THEN
+ RAISE EXCEPTION 'Failed to update comment count for snippet %',
+ COALESCE(NEW.room_id, OLD.room_id);
+ END IF;
RETURN NULL;
END;
$$ LANGUAGE plpgsql;📝 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.
| 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) | |
| ) | |
| WHERE id = COALESCE(NEW.room_id, OLD.room_id); | |
| RETURN NULL; | |
| END; | |
| $$ LANGUAGE plpgsql; | |
| CREATE OR REPLACE FUNCTION update_snippet_comment_count() | |
| RETURNS TRIGGER AS $$ | |
| BEGIN | |
| -- Validate input | |
| IF (TG_OP = 'INSERT' AND NEW.room_id IS NULL) OR | |
| (TG_OP = 'DELETE' AND OLD.room_id IS NULL) THEN | |
| RAISE EXCEPTION 'room_id cannot be null'; | |
| END IF; | |
| -- Verify snippet exists | |
| IF NOT EXISTS ( | |
| SELECT 1 | |
| FROM snippets | |
| WHERE id = COALESCE(NEW.room_id, OLD.room_id) | |
| ) THEN | |
| RAISE EXCEPTION 'Snippet with id % does not exist', | |
| COALESCE(NEW.room_id, OLD.room_id); | |
| END IF; | |
| UPDATE snippets | |
| SET comment_count = ( | |
| SELECT COUNT(*) | |
| FROM comments | |
| WHERE room_id = COALESCE(NEW.room_id, OLD.room_id) | |
| AND deleted_at IS NULL -- Only count non-deleted comments | |
| ) | |
| WHERE id = COALESCE(NEW.room_id, OLD.room_id); | |
| IF NOT FOUND THEN | |
| RAISE EXCEPTION 'Failed to update comment count for snippet %', | |
| COALESCE(NEW.room_id, OLD.room_id); | |
| END IF; | |
| RETURN NULL; | |
| END; | |
| $$ LANGUAGE plpgsql; |
💡 Codebase verification
Create trigger to execute the update_snippet_comment_count function
Add the following trigger definition to the same SQL file:
CREATE TRIGGER update_comment_count
AFTER INSERT OR DELETE ON comments
FOR EACH ROW
EXECUTE FUNCTION update_snippet_comment_count();Based on the verification:
- The trigger is indeed missing from the codebase
- Similar trigger patterns exist (e.g.,
update_like_counttrigger inlike_count_trigger.sql) - The function exists but is not being executed by any trigger
🔗 Analysis chain
Create trigger to execute the function
The trigger function is defined but the corresponding trigger is missing.
Add the following trigger definition:
CREATE TRIGGER update_comment_count
AFTER INSERT OR DELETE ON comments
FOR EACH ROW
EXECUTE FUNCTION update_snippet_comment_count();🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if trigger exists
rg "CREATE TRIGGER.*update_snippet_comment_count"
Length of output: 51
Script:
#!/bin/bash
# Let's search for any trigger definitions in SQL files
fd -e sql -x cat {} | rg -i "CREATE\s+TRIGGER"
# Also search for any files that might contain trigger definitions
fd -e sql | grep -i "trigger"
Length of output: 1997
Script:
#!/bin/bash
# Let's check the content of like_count_trigger.sql as it might have a similar pattern
cat supabase/database/sql/like_count_trigger.sql
# Also let's check for any other files that might contain our function name
fd -e sql -x cat {} | rg -A 5 "update_snippet_comment_count"
Length of output: 417
| UPDATE snippets s | ||
| SET upvote_count = COALESCE(uc.total_upvotes, 0) | ||
| FROM upvote_counts uc | ||
| WHERE s.id = uc.snippet_id; |
There was a problem hiding this comment.
Consider RIGHT JOIN to update all snippets
The current UPDATE might miss snippets that have no entries in snippet_labels. Consider modifying the CTE to start from the snippets table to ensure all snippets get their upvote_count updated.
WITH upvote_counts AS (
- SELECT sl.snippet AS snippet_id,
+ SELECT s.id AS snippet_id,
COUNT(lu.id) AS total_upvotes
- FROM snippet_labels sl
- LEFT JOIN label_upvotes lu ON lu.snippet_label = sl.id
- GROUP BY sl.snippet
+ FROM snippets s
+ LEFT JOIN snippet_labels sl ON sl.snippet = s.id
+ LEFT JOIN label_upvotes lu ON lu.snippet_label = sl.id
+ GROUP BY s.id
)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
supabase/database/sql/get_snippets_function.sql (2)
216-252: Improve readability with consistent indentationThe nested CASE statements would be more readable with consistent indentation:
- CASE - WHEN jsonb_array_length(p_filter->'upvotedBy') = 0 THEN TRUE - ELSE ( - CASE - WHEN ( - p_filter->'upvotedBy' ? 'by_me' AND - p_filter->'upvotedBy' ? 'by_others' - ) THEN + CASE + WHEN jsonb_array_length(p_filter->'upvotedBy') = 0 THEN TRUE + ELSE ( + CASE + WHEN (p_filter->'upvotedBy' ? 'by_me' AND + p_filter->'upvotedBy' ? 'by_others') THEN
268-293: Consider performance optimization for sorting logicThe current implementation has several areas for improvement:
- Consider adding index hints for commonly used sorting columns
- Document the sorting priority and fallback behavior
Consider refactoring to:
+ -- Add index hints for commonly used columns + /*+ + INDEX(s snippets_recorded_at_idx) + INDEX(s snippets_updated_at_idx) + INDEX(s snippets_engagement_idx) + */ ORDER BY ( CASE WHEN p_order_by = 'upvotes' THEN s.upvote_count + s.like_count END DESC,Also, consider creating a composite index for engagement sorting:
CREATE INDEX snippets_engagement_idx ON snippets (upvote_count + like_count DESC, recorded_at DESC);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
supabase/database/sql/get_snippets_function.sql(4 hunks)
🔇 Additional comments (2)
supabase/database/sql/get_snippets_function.sql (2)
314-319: LGTM! Return structure improvements
The simplified return structure with direct use of total_count improves clarity and correctness.
2-2: Verify security implications of SECURITY DEFINER
The function uses SECURITY DEFINER which executes with the privileges of the function owner. Ensure:
- Proper access controls are in place
- SQL injection vectors are properly handled
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check for proper input validation and sanitization.
Based on the analysis of the results, I can now provide a final response regarding the security implications:
Security concerns with SECURITY DEFINER are properly addressed
The function implementation shows proper security controls:
-
Input validation is handled through:
- Language parameter is validated via explicit CASE statements
- Order by parameter is restricted to specific values ('upvotes', 'comments', 'activities', 'latest')
- Filter parameter is type-safe through JSONB type and accessed via -> operator
- No dynamic SQL execution (no EXECUTE statements)
-
Access control is enforced:
- User authentication check via
auth.uid() - Admin role verification through
user_roles - All database access is done through proper SQL constructs without string concatenation
- User authentication check via
-
SQL injection prevention:
- Uses parameterized values
- Employs COALESCE for null safety
- All string comparisons use exact matching
- No user input is used in dynamic SQL
The SECURITY DEFINER usage here follows security best practices with proper input validation and access controls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in function definition and usage
# Check for other SECURITY DEFINER functions that might be chained
rg -l "SECURITY\s+DEFINER" --type sql
# Check for proper input validation
rg -A 5 "p_(language|filter|order_by)" --type sql
# Check for usage of dynamic SQL (potential SQL injection vector)
rg "EXECUTE|EXECUTE\s+IMMEDIATE|EXEC" --type sql
Length of output: 26442
Script:
#!/bin/bash
# Check for input validation and sanitization in the get_snippets function
# Check how p_language is validated
rg -A 10 "p_language\s*:=|p_language\s*=|p_language\s+text" --type sql
# Check how p_filter is validated/sanitized
rg -A 10 "p_filter\s*:=|p_filter\s*=|p_filter\s+jsonb" --type sql
# Check how p_order_by is validated
rg -A 10 "p_order_by\s*:=|p_order_by\s*=|p_order_by\s+text" --type sql
# Check for any SQL injection prevention patterns
rg "quote_literal|quote_ident|COALESCE|NULLIF" --type sql
Length of output: 13394
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores