fix(ratings): stop naming a user_id column production never had - #44
Merged
Merged
Conversation
Rating writes have been failing with 503 since 1c3a4c4 (2026-09-04), which collapsed the two insert shapes into one. The legacy arm then sent `user_id: null` even after its probe found the column missing, and PostgREST rejects an insert that names an unknown column rather than ignoring it. The route's own legacy fallback could never have worked. Production's style_ratings table never received migration 003's user_id half, so every rating lives under the `session_id = "user:<uuid>"` identity. The insert now branches again, with an explicit payload type so both arms stay assignable without the union that motivated the collapse. Migration 041 applies the missing column and folds the existing rows onto it. The client also stops discarding the route's already user-safe error message, which is what kept a silent 16-day outage looking like a generic failure.
Migration 034 left 003's user_favorites.user_id column unapplied on the grounds that no code referenced it. The code has referenced it since 2026-02-21 -- both the favorites API and the merge path try the user_id arm first and fall back to the session identity -- so production has served every signed-in write through the fallback. Migration 042 applies the column and folds the 370 legacy rows onto it, preserving their session_id so migration 034's RLS policies keep protecting them unchanged. Two details deliberately differ from 003 as written: the session_id NOT NULL drop is load-bearing, since the modern arm never names that column, and the unique index is not partial, because Postgres refuses to infer a partial index as the arbiter for the merge path's onConflict "user_id,style_slug" and fails it with 42P10.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe PR adds user bindings for style ratings and favorites, backfills eligible legacy records, preserves legacy rating inserts, adds migration indexes and duplicate cleanup, and improves client handling of failed rating submissions. ChangesUser-bound ratings and favorites
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The rating and favorites schema changes preserve their respective write paths without an actionable current-head issue. The PR is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
11 tasks
Link-Start
pushed a commit
to Link-Start/stylekit_AnxForever
that referenced
this pull request
Sep 21, 2026
stylekit-lint-example.yml sat in .github/workflows/, so it ran on every pull request that touched components/**/*.tsx -- which is how it first fired, on PR AnxForever#44. It could not pass there. It pointed at .github/actions/lint, whose two candidate linter paths (packages/core/dist/linter/index.cjs and lib/linter/index.ts) exist nowhere in this repository, and its glob src/**/*.tsx matches nothing in a checkout that has no src/ directory. Everything else about the file describes a consumer's repository: it installs @stylekit/core from npm and lints src/**. So it moves to docs/examples/ as documentation, and its uses: and input names now match the action the README actually documents, .github/actions/stylekit-lint -- which takes fail-on-error, not the fail-on that the workflow was passing to a different action entirely.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What changed:
Rating writes have returned 503 for every signed-in user since 2026-09-04. Production's
style_ratingstable never received migration 003'suser_idhalf — onlystyle_commentsandsubmissionsgot theirs — so every rating lives under the legacysession_id = "user:<uuid>"identity.1c3a4c42collapsed the route's two insert shapes into one to dodge a supabase-js union-typing error. The legacy arm then sentuser_id: nulleven after its own probe found the column missing. PostgREST rejects an insert that names a column it does not have rather than ignoring it, so the fallback the code was written around could never have worked.Two migrations apply the missing halves and fold the legacy rows onto the real identity:
041_style_ratings_user_binding.sql— 11 ratings, 9 users042_user_favorites_user_binding.sql— 370 favorites, 64 usersWhy:
Without the code change, the next database that reaches this state breaks identically. Without the backfill, adding the column would make
.eq("user_id", ...)— the read path the code prefers — return nothing, silently hiding every existing rating and favorite.Two details deliberately differ from migration 003 as written, both established by direct test rather than reading:
onConflict: "user_id,style_slug"and fails it with42P10, which neither error classifier in the favorites route treats as a missing column. Dropping the predicate is free: Postgres treats NULLuser_idvalues as distinct, so anonymous rows stay unconstrained.session_id NOT NULLdrop is load-bearing. The modern arm inserts{user_id, style_slug}and never namessession_id.RLS is left alone on purpose. Every favorites and ratings read/write goes through the service role, and migration 034's policies key off
session_id, which the backfill preserves. Rows the modern arm writes carry a NULLsession_id, which those policies deny to anon callers — fail-closed.Change Type
fix— bug fixScope
Validation
pnpm run security:secrets— no secrets detectedpnpm run lint— no errorsnpx tsc --noEmit— no type errorspnpm test— 264 files / 7797 tests passpnpm build— builds successfullyThe new route test was run against the unfixed code first and confirmed to fail on the
not.toHaveProperty("user_id")assertion.Security
.envfiles committedNEXT_PUBLIC_Breaking Changes
Notes for Reviewers
Key files:
app/api/styles/[slug]/rate/route.ts(the fix), the two migrations,components/styles/style-rating.tsx(the client stopped discarding the route's user-safe error, which is what kept a 16-day outage looking generic).Verification already performed against production, since the failure only occurs past authentication:
users_lost_or_changed = 0across all 64 favorites owners.{"success":true,...,"userRating":4}HTTP 200, re-rating updated in place without a second row, and the row landed withuser_idset andsession_idNULL. The account and its rating were deleted afterwards; row counts returned to 11 ratings / 371 favorites.Known risk: the migrations are already applied in production, so the schema leads this PR. Merging only brings the source of truth back in line.
Deploy note:
pnpm buildOOMs at node's default 2096 MB heap during the inlined TypeScript check (npx tsc --noEmitpasses independently).NODE_OPTIONS="--max-old-space-size=6144"is required locally.Summary by CodeRabbit
New Features
Bug Fixes