Full-text search (D1 FTS5)#93
Conversation
Extend the videos_fts virtual table with a tags column (label + slug) and recreate the sync triggers, plus add triggers on video_tags and tags so the index stays current. The /api/videos/search MATCH already spans all FTS columns, so tag-keyed queries now hit too. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds tags to the ChangesFull-Text Search Index for Tags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a database migration to update the videos_fts virtual table to include tags, allowing for more comprehensive search functionality. The reviewer identified a critical issue where the migration references a non-existent user table and name column, which conflicts with the actual schema. Additionally, the reviewer noted that using UNINDEXED for video_id in the FTS5 table will cause performance issues due to full table scans during trigger operations as the dataset grows.
| COALESCE(u.name, ''), | ||
| COALESCE( | ||
| (SELECT GROUP_CONCAT(t.label || ' ' || t.slug, ' ') | ||
| FROM video_tags vt | ||
| JOIN tags t ON t.slug = vt.tag_slug | ||
| WHERE vt.video_id = v.id), | ||
| '' | ||
| ) | ||
| FROM videos v | ||
| LEFT JOIN user u ON u.id = v.user_id | ||
| WHERE v.deleted_at IS NULL; | ||
|
|
||
| CREATE TRIGGER videos_fts_ai AFTER INSERT ON videos | ||
| WHEN new.deleted_at IS NULL | ||
| BEGIN | ||
| INSERT INTO videos_fts (video_id, title, description, channel_name, tags) | ||
| VALUES ( | ||
| new.id, | ||
| new.title, | ||
| new.description, | ||
| COALESCE((SELECT name FROM user WHERE id = new.user_id), ''), | ||
| COALESCE( | ||
| (SELECT GROUP_CONCAT(t.label || ' ' || t.slug, ' ') | ||
| FROM video_tags vt | ||
| JOIN tags t ON t.slug = vt.tag_slug | ||
| WHERE vt.video_id = new.id), | ||
| '' | ||
| ) | ||
| ); | ||
| END; | ||
|
|
||
| CREATE TRIGGER videos_fts_au | ||
| AFTER UPDATE OF title, description, user_id, deleted_at ON videos | ||
| BEGIN | ||
| DELETE FROM videos_fts WHERE video_id = old.id; | ||
| INSERT INTO videos_fts (video_id, title, description, channel_name, tags) | ||
| SELECT new.id, | ||
| new.title, | ||
| new.description, | ||
| COALESCE(u.name, ''), | ||
| COALESCE( | ||
| (SELECT GROUP_CONCAT(t.label || ' ' || t.slug, ' ') | ||
| FROM video_tags vt | ||
| JOIN tags t ON t.slug = vt.tag_slug | ||
| WHERE vt.video_id = new.id), | ||
| '' | ||
| ) | ||
| FROM (SELECT 1) AS s LEFT JOIN user u ON u.id = new.user_id | ||
| WHERE new.deleted_at IS NULL; | ||
| END; | ||
|
|
||
| CREATE TRIGGER videos_fts_ad AFTER DELETE ON videos BEGIN | ||
| DELETE FROM videos_fts WHERE video_id = old.id; | ||
| END; | ||
|
|
||
| CREATE TRIGGER user_name_videos_fts | ||
| AFTER UPDATE OF name ON user | ||
| BEGIN | ||
| DELETE FROM videos_fts WHERE video_id IN (SELECT id FROM videos WHERE user_id = new.id); | ||
| INSERT INTO videos_fts (video_id, title, description, channel_name, tags) | ||
| SELECT v.id, | ||
| v.title, | ||
| v.description, | ||
| new.name, | ||
| COALESCE( | ||
| (SELECT GROUP_CONCAT(t.label || ' ' || t.slug, ' ') | ||
| FROM video_tags vt | ||
| JOIN tags t ON t.slug = vt.tag_slug | ||
| WHERE vt.video_id = v.id), | ||
| '' | ||
| ) | ||
| FROM videos v | ||
| WHERE v.user_id = new.id AND v.deleted_at IS NULL; |
There was a problem hiding this comment.
The migration references a table named user and a column named name (e.g., lines 28, 48, 75, 84, 91). However, src/db/schema.sql defines the table as users and includes columns username and display_name, but no name column. This discrepancy will cause the migration to fail if the database follows the provided schema. Please verify the correct table and column names and ensure consistency with src/db/schema.sql.
| DROP TABLE IF EXISTS videos_fts; | ||
|
|
||
| CREATE VIRTUAL TABLE videos_fts USING fts5( | ||
| video_id UNINDEXED, |
There was a problem hiding this comment.
The video_id column is marked as UNINDEXED. In SQLite FTS5, performing DELETE or UPDATE operations using a non-rowid column in the WHERE clause (as seen in the triggers on lines 62, 80, 86, 114, 127, and 142) results in a full table scan of the virtual table. While acceptable for small datasets, this will lead to performance degradation as the number of videos increases. If performance becomes an issue, consider mapping the TEXT video ID to an integer rowid or using an external content table if the primary key can be adapted.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 872589c5d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| END; | ||
|
|
||
| -- Keep tags column in sync when video_tags rows are inserted/deleted. | ||
| CREATE TRIGGER video_tags_ai_fts AFTER INSERT ON video_tags |
There was a problem hiding this comment.
This migration creates video_tags_ai_fts (and later video_tags_ad_fts / tags_au_label_fts) without IF NOT EXISTS, but the preamble only drops the legacy videos_fts_* and user_name_videos_fts triggers. If an initial apply fails after one of these new triggers is created (for example, a later statement errors), rerunning 0019 will stop with "trigger already exists", which can block deploys until manual DB cleanup.
Useful? React with 👍 / 👎.
…150) Pin the FTS5-includes-tags contract so a future drop/recreate or trigger removal would fail the migration suite — search relevance silently regressing was the failure mode that hid the original gap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/db/migrations.test.ts (1)
74-85: ⚡ Quick winConsider asserting the recreated video/user triggers to close the regression-prevention gap.
The test verifies the three new tag-related triggers but does not assert that the four recreated triggers (
videos_fts_ai,videos_fts_au,videos_fts_ad,user_name_videos_fts) are present. Since the migration drops and recreates all of them, accidentally omitting aCREATE TRIGGERstatement for any of them would go undetected.♻️ Proposed additions to close the gap
expect(sql).toMatch(/AFTER INSERT ON video_tags/); expect(sql).toMatch(/AFTER DELETE ON video_tags/); expect(sql).toMatch(/AFTER UPDATE OF label ON tags/); + // Recreated video + user triggers must also be present. + expect(sql).toMatch(/CREATE TRIGGER videos_fts_ai/); + expect(sql).toMatch(/CREATE TRIGGER videos_fts_au/); + expect(sql).toMatch(/CREATE TRIGGER videos_fts_ad/); + expect(sql).toMatch(/CREATE TRIGGER user_name_videos_fts/); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations.test.ts` around lines 74 - 85, The test for 0019_videos_fts_tags currently checks only tag-related triggers but must also assert the recreated FTS triggers are present; update the test in migrations.test.ts to add expect assertions that the SQL contains CREATE TRIGGER statements for videos_fts_ai, videos_fts_au, videos_fts_ad and user_name_videos_fts (e.g. expect(sql).toMatch(/CREATE TRIGGER\s+videos_fts_ai/), etc.), so any missing recreated trigger will fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/db/migrations.test.ts`:
- Around line 74-85: The test for 0019_videos_fts_tags currently checks only
tag-related triggers but must also assert the recreated FTS triggers are
present; update the test in migrations.test.ts to add expect assertions that the
SQL contains CREATE TRIGGER statements for videos_fts_ai, videos_fts_au,
videos_fts_ad and user_name_videos_fts (e.g. expect(sql).toMatch(/CREATE
TRIGGER\s+videos_fts_ai/), etc.), so any missing recreated trigger will fail the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10b19483-a592-4a0c-90b3-fa5145a3dcf9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/db/migrations.test.tssrc/db/migrations/0019_videos_fts_tags.sql
The 0019 migration drops and recreates videos_fts_ai/au/ad and user_name_videos_fts. Without explicit assertions, accidentally omitting any of those CREATE TRIGGER statements would slip through. Per CodeRabbit feedback on PR #93.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ecc-tools comment is an informational no-op — its body says ECC bundle files are already tracked in this repository and it's skipping generation of another bundle PR. No change requested, no code change needed. Latest commit (623a0cb) already addressed the CodeRabbit nitpick about asserting the recreated FTS triggers (videos_fts_ai/au/ad, user_name_videos_fts), and CI is green on that commit. |
Closes ALO-150.
Summary
videos_ftsFTS5 virtual table to include atagscolumn (tag labels + slugs concatenated), so search now spans title / description / channel / tags as the ticket spec requires.ALTER, so the migration drops and recreates the virtual table plus the keep-in-sync triggers, then adds new triggers onvideo_tags(insert/delete) andtags(label updates) so the tag column stays current./api/videos/search?q=and the FE header search box were already in place from earlier work; the MATCH spans all indexed columns so no query change was needed.Test plan
npm test— 491/491 passnpm run lint— clean (incl. AI Gateway guard)npm run db:migrateto apply migration 0019 in prodSummary by CodeRabbit
Release Notes