-
Notifications
You must be signed in to change notification settings - Fork 1k
Refactor video effective date and optimize DB indexes #1361
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.
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
Conversation
WalkthroughThis PR introduces a computed column Changes
Sequence DiagramsequenceDiagram
participant Query as Web Query
participant DB as Database
participant Videos as videos table
Query->>DB: SELECT effectiveCreatedAt FROM videos ORDER BY effectiveCreatedAt DESC
DB->>Videos: Compute effectiveCreatedAt = COALESCE(metadata→customCreatedAt, createdAt)
Videos->>DB: Return computed date value (precomputed in DB)
DB->>Query: Return ordered results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx (2)
213-221: Consider addingvideos.effectiveCreatedAtto thegroupByclause.While
effectiveCreatedAtis a generated column deterministic onmetadataandcreatedAt(both present ingroupBy), explicitly includingvideos.effectiveCreatedAtin thegroupByclause would improve clarity and prevent potential MySQL version-specific issues when ordering by a selected column.Apply this diff:
.groupBy( videos.id, videos.ownerId, videos.name, videos.createdAt, videos.metadata, users.name, + videos.effectiveCreatedAt, )
307-316: Consider addingvideos.effectiveCreatedAtto thegroupByclause.Similar to the
fetchSpaceVideosfunction, explicitly includingvideos.effectiveCreatedAtin thegroupByclause would improve clarity and prevent potential MySQL version-specific issues when ordering by a selected column.Apply this diff:
.groupBy( videos.id, videos.ownerId, videos.name, videos.createdAt, videos.metadata, users.name, videos.duration, + videos.effectiveCreatedAt, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/actions/spaces/get-user-videos.ts(3 hunks)apps/web/app/(org)/dashboard/caps/page.tsx(2 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx(4 hunks)apps/web/lib/folder.ts(2 hunks)packages/database/migrations/0007_public_toxin.sql(1 hunks)packages/database/migrations/0008_fat_ender_wiggin.sql(1 hunks)packages/database/migrations/0008_secondary_index_cleanup.sql(1 hunks)packages/database/migrations/meta/0007_snapshot.json(1 hunks)packages/database/migrations/meta/0008_snapshot.json(1 hunks)packages/database/migrations/meta/_journal.json(1 hunks)packages/database/schema.ts(18 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/web/app/(org)/dashboard/caps/page.tsxapps/web/actions/spaces/get-user-videos.tsapps/web/lib/folder.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsxpackages/database/schema.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/dashboard/caps/page.tsxapps/web/actions/spaces/get-user-videos.tsapps/web/lib/folder.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsxpackages/database/schema.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/dashboard/caps/page.tsxapps/web/actions/spaces/get-user-videos.tsapps/web/lib/folder.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to **/queries.ts : Do not edit auto-generated files named `queries.ts`.
Applied to files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsxpackages/database/schema.ts
🧬 Code graph analysis (5)
apps/web/app/(org)/dashboard/caps/page.tsx (1)
packages/database/schema.ts (1)
videos(291-360)
apps/web/actions/spaces/get-user-videos.ts (1)
packages/database/schema.ts (1)
videos(291-360)
apps/web/lib/folder.ts (1)
packages/database/schema.ts (1)
videos(291-360)
apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx (1)
packages/database/schema.ts (1)
videos(291-360)
packages/database/schema.ts (5)
packages/database/helpers.ts (1)
nanoId(6-9)packages/web-domain/src/User.ts (2)
UserId(10-10)UserId(11-11)packages/web-domain/src/Folder.ts (3)
Folder(37-45)FolderId(11-11)FolderId(12-12)packages/web-domain/src/Video.ts (3)
Video(16-59)VideoId(12-12)VideoId(13-13)packages/web-domain/src/S3Bucket.ts (3)
S3Bucket(7-15)S3BucketId(4-4)S3BucketId(5-5)
🪛 GitHub Actions: Validate Migrations
packages/database/migrations/meta/_journal.json
[error] 2-2: Migration journal version cannot be changed (was: <BASE_VERSION>, now: <CURRENT_VERSION>)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
packages/database/migrations/0008_fat_ender_wiggin.sql (2)
15-16: LGTM! Well-designed composite indexes.The new composite indexes are well-structured for their intended query patterns:
video_type_created_idxefficiently supports filtering comments by video/type and ordering by creation timespace_id_folder_id_idxoptimizes folder-based queries within spaces
9-14: Verify that queries filtering byspaceIdalone on space_members won't suffer performance degradation.The migration drops
space_id_idxandspace_id_user_id_idxfrom space_members without replacement. Multiple queries in the codebase filter byspaceIdalone on this table (e.g., delete operations in delete-space.ts and select operations in [spaceId]/page.tsx), which would have benefited from the dropped single-column index. Composite indexes may not be optimal for these lookups if they're not created to cover them.For notifications, dropping
recipient_id_idxis acceptable sincerecipient_read_idxandrecipient_created_idxcomposite indexes can serve queries filtering byrecipientId.packages/database/migrations/meta/0008_snapshot.json (1)
1-1847: Migration snapshot is correctly generated.This auto-generated snapshot file accurately reflects the schema state after migrations 0007 and 0008, including:
- The
effectiveCreatedAtgenerated column on videos- New composite indexes (
video_type_created_idx,space_id_folder_id_idx)- Removal of redundant unique constraints on primary key columns
packages/database/migrations/0007_public_toxin.sql (2)
19-21: LGTM! Indexes are well-designed for the query patterns.The new indexes effectively support:
- Tombstone-aware organization queries (
owner_id_tombstone_idx)- Folder-based video filtering within organizations (
org_owner_folder_idx)- Efficient ordering by the computed effective creation date (
org_effective_created_idx)These indexes align well with the query patterns observed in the web application.
14-18: Timestamp-to-datetime conversion is a real type mismatch that requires a fix.The
videos.createdAtcolumn is defined astimestamp(UTC-stored, session-timezone-converted), whileeffectiveCreatedAtisdatetime(literal storage, no conversion). WhencreatedAtserves as the fallback in the COALESCE expression, any subsequent changes to the MySQL session timezone will cause sorting and display inconsistencies—older rows won't update their stored datetime values, but new rows will reflect the new timezone interpretation.Since
effectiveCreatedAtis actively used for sorting, display, and indexing across the application, this type mismatch should be corrected:
- Change
effectiveCreatedAttotimestampto ensure consistent timezone behavior, OR- Implement timezone-aware handling in the application layer to guarantee consistency
packages/database/migrations/meta/0007_snapshot.json (1)
1-1906: Migration snapshot correctly represents state after 0007.This auto-generated snapshot accurately reflects the intermediate schema state after migration 0007, including:
- The new
effectiveCreatedAtgenerated column- New indexes:
owner_id_tombstone_idx,org_owner_folder_idx,org_effective_created_idx- Indexes and constraints that will be modified in migration 0008 are still present
packages/database/migrations/meta/_journal.json (1)
2-2: The version field was not modified—it remains "5" before and after this change.The git diff confirms that the
versionfield at line 2 of_journal.jsonis unchanged (still"5"). The two new migration entries (idx 7 and 8) also correctly include"version": "5", consistent with all existing entries. No drizzle-orm version changes were detected inpackage.json. The migration entries follow the proper format with sequential indexing and valid timestamps. The pipeline failure is unrelated to this file's changes.
Introduces a new computed column,
effectiveCreatedAt, to thevideostable and refactors the codebase to use this column for sorting and querying video creation dates. Additionally, it removes redundant unique indexes from several tables, adds new composite indexes to optimize queries, and updates the schema definitions to match these changes.Summary by CodeRabbit