Night Shift: fix broken Convex filter in toggleLineLearned#18
Night Shift: fix broken Convex filter in toggleLineLearned#18
Conversation
The filter used JavaScript && between Convex query expressions:
q.eq(q.field("songId"), ...) && q.eq(q.field("lineNumber"), ...)
Since && returns the right operand when the left is truthy (any object),
this silently dropped the songId filter. The mutation could toggle the
wrong line's learned state when a user has multiple songs with matching
line numbers.
Fixed by using q.and() to properly combine both filter conditions.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨No code suggestions found for the PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/songProgress.ts (2)
233-244:⚠️ Potential issue | 🟠 MajorAdd composite index on
(userId, songId)tolineProgresstable to avoid full per-user scans.The current query uses
withIndex("by_user")which fetches all of a user'slineProgressrows regardless of song, then filterssongIdandlineNumberin memory. As a user learns more lines across more songs, this scan grows unboundedly. The schema has composite indexes forvisitorIdcombinations but not foruserId. Adding an index on(userId, songId)— or(userId, songId, lineNumber)for a point-lookup — would reduce this to O(lines in song) or O(1).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/songProgress.ts` around lines 233 - 244, The query on lineProgress using withIndex("by_user") performs a full per-user scan and then filters songId and lineNumber in memory; add a composite index on (userId, songId) or (userId, songId, lineNumber) to the lineProgress table schema so the query becomes index-backed (update the schema/index definitions for lineProgress to include an index keyed by userId+songId or userId+songId+lineNumber) and then switch the query to use that index (replace withIndex("by_user") with the new index name) so lookups by userId/songId/(lineNumber) are efficient.
225-303:⚠️ Potential issue | 🟡 MinorAdd a regression test for the
toggleLineLearnedmutation to verify multi-songlineNumbercollision is prevented.The mutation's fix correctly filters by both
songIdandlineNumbercombined, but this critical logic lacks test coverage. Without a targeted test, the bug—where toggling a line in one song could affect another song with the same line number—could silently regress. Add a Convex integration test that verifies toggling line N in song A does not affect song B's line N.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/songProgress.ts` around lines 225 - 303, Add a Convex integration regression test for the toggleLineLearned mutation that ensures toggling a line for one song does not affect another song with the same lineNumber: set up two distinct songs (songA, songB), call toggleLineLearned with songA and lineNumber N, then query the lineProgress and userSongProgress collections to assert a lineProgress/userSongProgress was created/updated for songA but no lineProgress or song progress exists or was modified for songB; reference the mutation name toggleLineLearned and the collections/records "lineProgress" and "userSongProgress" when locating where to add the assertions.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
convex/songProgress.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TanStack Start framework with Bun runtime for the application
Tests must pass locally via
bun run testbefore committing code, as Husky pre-commit hooks will block commits with failing tests
Files:
convex/songProgress.ts
convex/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Convex for database queries, mutations, and authentication configuration
Files:
convex/songProgress.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All new helpers and utilities MUST have corresponding test files
Files:
convex/songProgress.ts
convex/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
convex/**/*.ts: NEVER create .js files in the convex/ folder - only .ts files belong there
Before starting Convex dev server, always runrm -f convex/*.jsto clean compiled JavaScript files
Use Convex schema.ts for database schema definitions and songs.ts for queries and mutations
Files:
convex/songProgress.ts
🧠 Learnings (3)
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to convex/**/*.ts : Use Convex schema.ts for database schema definitions and songs.ts for queries and mutations
Applied to files:
convex/songProgress.ts
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*{score,progress,leaderboard}*.{ts,tsx,js} : Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)
Applied to files:
convex/songProgress.ts
📚 Learning: 2026-01-23T18:12:38.519Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:38.519Z
Learning: Applies to convex/*.ts : Use Convex for database queries, mutations, and authentication configuration
Applied to files:
convex/songProgress.ts
🔇 Additional comments (1)
convex/songProgress.ts (1)
238-243: Fix is correct —q.and()is the right Convex API here.The old
&&silently discarded thesongIdpredicate because Convex filter expressions are objects (always truthy), soobjA && objBreturnsobjB. Replacing it withq.and()correctly passes both predicates to the Convex query engine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@convex/songProgress.ts`:
- Around line 233-244: The query on lineProgress using withIndex("by_user")
performs a full per-user scan and then filters songId and lineNumber in memory;
add a composite index on (userId, songId) or (userId, songId, lineNumber) to the
lineProgress table schema so the query becomes index-backed (update the
schema/index definitions for lineProgress to include an index keyed by
userId+songId or userId+songId+lineNumber) and then switch the query to use that
index (replace withIndex("by_user") with the new index name) so lookups by
userId/songId/(lineNumber) are efficient.
- Around line 225-303: Add a Convex integration regression test for the
toggleLineLearned mutation that ensures toggling a line for one song does not
affect another song with the same lineNumber: set up two distinct songs (songA,
songB), call toggleLineLearned with songA and lineNumber N, then query the
lineProgress and userSongProgress collections to assert a
lineProgress/userSongProgress was created/updated for songA but no lineProgress
or song progress exists or was modified for songB; reference the mutation name
toggleLineLearned and the collections/records "lineProgress" and
"userSongProgress" when locating where to add the assertions.
1 participant
User description
Automated improvement by Golems Night Shift.
fix broken Convex filter in toggleLineLearned
PR Type
Bug fix
Description
Fixed broken Convex filter using q.and() instead of JavaScript &&
Prevents toggling wrong line's learned state across multiple songs
Ensures both songId and lineNumber conditions are properly combined
Diagram Walkthrough
File Walkthrough
songProgress.ts
Replace && with q.and() in filter logicconvex/songProgress.ts
filter
Note
Low Risk
Small query predicate change in a single Convex mutation; low blast radius but could affect whether the correct
lineProgressrecord is found/toggled.Overview
Fixes
toggleLineLearnedinconvex/songProgress.tsby updating thelineProgresslookup filter to useq.and(...)to combinesongIdandlineNumberpredicates.This prevents the mutation from using an invalid/broken filter expression, ensuring the correct per-line progress record is found and toggled.
Written by Cursor Bugbot for commit 1a46a1d. This will update automatically on new commits. Configure here.
Summary by CodeRabbit