fix(platform-links): prevent duplicate displayOrder via unique constraint and retry#2
Closed
Ridanshi wants to merge 3 commits into
Closed
fix(platform-links): prevent duplicate displayOrder via unique constraint and retry#2Ridanshi wants to merge 3 commits into
Ridanshi wants to merge 3 commits into
Conversation
…ard#506) * feat: add GitHub platform autodiscovery * fix: resolve lint and type issues in autodiscovery * fix: restore reply parameter in github autodiscovery route * fix: resolve unused reply lint issue
|
Hi @Ridanshi, Thanks for opening this pull request. This PR has been automatically classified based on the files modified. Applied Labels
Primary Review Area
Reviewer@Harxhit has been identified as the primary reviewer for this pull request. If you have any questions regarding the affected area or implementation details, feel free to reach out to the assigned reviewer. Thank you for your contribution! |
📝 WalkthroughWalkthroughThis PR implements per-user display-order uniqueness for platform links, adds retry-based conflict resolution and a two-phase reordering algorithm, integrates Redis cache invalidation across profile mutations, and provides comprehensive test coverage for both display-ordering and cache behaviors. ChangesPlatform Link Display-Order Enforcement and Cache Invalidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
CI — Checks FailedBackend — FAIL
Mobile — SKIP
Web — SKIP
Last updated: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/routes/cards.ts (1)
1-3:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix import order to comply with ESLint import-x/order rule.
Services imports must come before utils imports. Line 3 (
cardService) should be moved before Line 1 (error.util).This is a blocking CI failure per the pipeline log.
📦 Proposed fix for import order
-import { handleDbError } from '../utils/error.util.js'; -import { createCardSchema, updateCardSchema } from '../utils/validators.js'; import * as cardService from '../services/cardService' +import { handleDbError } from '../utils/error.util.js'; +import { createCardSchema, updateCardSchema } from '../utils/validators.js';🤖 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 `@apps/backend/src/routes/cards.ts` around lines 1 - 3, The import order violates ESLint import-x/order: move the service import to come before utility imports; specifically, place "import * as cardService from '../services/cardService'" above the imports for "handleDbError" and "createCardSchema, updateCardSchema" so cardService is imported before error.util.js and validators.js, preserving existing specifiers and paths.Source: Pipeline failures
🧹 Nitpick comments (3)
apps/backend/src/services/profileService.ts (1)
131-131: 💤 Low valueDead code: this line is unreachable.
The loop always exits via
returnon success (line 123) orthrowon error (line 128). On the final attempt, if a P2002 occurs,attempt < MAX_RETRIES - 1is false, so the original error is thrown rather than reaching this line.♻️ Remove dead code
throw err; } } - throw new Error('Failed to allocate display order after max retries'); + // Unreachable: loop always exits via return or throw + throw new Error('Unreachable'); }Or simply remove the line since TypeScript will infer the return type correctly without it.
🤖 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 `@apps/backend/src/services/profileService.ts` at line 131, Remove the unreachable final throw statement that reads "Failed to allocate display order after max retries" in the display-order allocation loop (the throw at the end of the function that contains the P2002 retry logic, e.g., in the allocateDisplayOrder or equivalent method in profileService.ts); the loop already returns on success and rethrows on the last retry, so delete that dead throw, ensure the function signature/type still compiles, and run TypeScript build/tests to confirm no regressions.apps/backend/src/__tests__/platform-link-ordering.test.ts (2)
196-207: 💤 Low valueConsider making the aggregate mock stateful for more realistic concurrency simulation.
The test mocks
aggregateto always returnmax=2, so both concurrent requests attemptdisplayOrder=3, and after retries both succeed with the same value. In reality, after the first successful insert, the second retry would read an updated max and insert at a different displayOrder.While the test successfully verifies that the retry mechanism works and both requests complete, a more realistic simulation would update the aggregate result after successful inserts to match actual database behavior.
♻️ Optional enhancement for test realism
describe('createPlatformLink — concurrent request simulation', () => { it('two simultaneous creates both succeed via retry when first conflicts', async () => { const p2002 = Object.assign(new Error('Unique constraint failed on (user_id, display_order)'), { code: 'P2002', }); let createCallCount = 0; + let currentMax = 2; (mockPrisma.platformLink.aggregate as any).mockImplementation(() => { - // Both reads see max=2 before either inserts - return Promise.resolve({ _max: { displayOrder: 2 } }); + return Promise.resolve({ _max: { displayOrder: currentMax } }); }); (mockPrisma.platformLink.create as any).mockImplementation(({ data }: any) => { createCallCount++; // Simulate: first two calls (both at order=3) conflict; retries succeed if (createCallCount <= 2) { return Promise.reject(p2002); } + currentMax = Math.max(currentMax, data.displayOrder); return Promise.resolve(baseLink(`link-${createCallCount}`, data.displayOrder)); });🤖 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 `@apps/backend/src/__tests__/platform-link-ordering.test.ts` around lines 196 - 207, The aggregate mock always returning {_max: {displayOrder: 2}} makes the test unrealistic; make it stateful by introducing a closure variable (e.g., currentMax = 2) and have mockPrisma.platformLink.aggregate return {_max: {displayOrder: currentMax}}; in the mockPrisma.platformLink.create implementation (which uses createCallCount, p2002 and baseLink), increment currentMax when a create resolves successfully so subsequent aggregate calls reflect the new max, and keep the initial rejects (<=2) to simulate conflict/retry.
227-233: 💤 Low valueConsider making the aggregate mock stateful for more realistic concurrency simulation.
Similar to the two-create test, this test uses a static aggregate result. While it successfully verifies that retries resolve conflicts, updating the mock to reflect successful inserts would more accurately simulate database behavior during concurrent operations.
🤖 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 `@apps/backend/src/__tests__/platform-link-ordering.test.ts` around lines 227 - 233, The aggregate mock is static but should reflect successful inserts so concurrency is simulated; update the test to make mockPrisma.platformLink.aggregate stateful by tying its returned count to the same counter used by (mockPrisma.platformLink.create as any).mockImplementation: keep callCount and p2002/baseLink logic, but when create resolves (i.e., not rejecting), increment a createdLinks counter (or derive from callCount) and have mockPrisma.platformLink.aggregate read that counter and return { _max: { displayOrder: ... } } / { _count: { id: createdLinks } } as appropriate so aggregate reflects successful inserts during retries; ensure the aggregate mock and create mock share the same closure/state so aggregate updates after each successful create.
🤖 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.
Inline comments:
In `@apps/backend/src/__tests__/profile-cache.test.ts`:
- Line 87: The two app builder functions are missing explicit TypeScript return
types; add explicit return type annotations for buildProfileApp and the other
app builder function referenced at the other location (line ~101) to satisfy
`@typescript-eslint/explicit-function-return-type`—e.g., annotate them as async
functions returning Promise<express.Application> (or the precise app type your
helpers return) so the signature reads like: async function
buildProfileApp(withRedis = true): Promise<express.Application> { ... } (apply
the same style to the second builder).
In `@apps/backend/src/routes/cards.ts`:
- Around line 118-119: The two single-line if statements checking error?.code
('NOT_FOUND' and 'LAST_CARD') in the cards route handler must be converted to
block form to satisfy the ESLint curly rule; update the if (error?.code ===
'NOT_FOUND') and if (error?.code === 'LAST_CARD') branches to use braces and
place the existing reply.status(...).send(...) calls inside their respective { }
blocks so behavior is unchanged but style complies.
---
Outside diff comments:
In `@apps/backend/src/routes/cards.ts`:
- Around line 1-3: The import order violates ESLint import-x/order: move the
service import to come before utility imports; specifically, place "import * as
cardService from '../services/cardService'" above the imports for
"handleDbError" and "createCardSchema, updateCardSchema" so cardService is
imported before error.util.js and validators.js, preserving existing specifiers
and paths.
---
Nitpick comments:
In `@apps/backend/src/__tests__/platform-link-ordering.test.ts`:
- Around line 196-207: The aggregate mock always returning {_max: {displayOrder:
2}} makes the test unrealistic; make it stateful by introducing a closure
variable (e.g., currentMax = 2) and have mockPrisma.platformLink.aggregate
return {_max: {displayOrder: currentMax}}; in the mockPrisma.platformLink.create
implementation (which uses createCallCount, p2002 and baseLink), increment
currentMax when a create resolves successfully so subsequent aggregate calls
reflect the new max, and keep the initial rejects (<=2) to simulate
conflict/retry.
- Around line 227-233: The aggregate mock is static but should reflect
successful inserts so concurrency is simulated; update the test to make
mockPrisma.platformLink.aggregate stateful by tying its returned count to the
same counter used by (mockPrisma.platformLink.create as any).mockImplementation:
keep callCount and p2002/baseLink logic, but when create resolves (i.e., not
rejecting), increment a createdLinks counter (or derive from callCount) and have
mockPrisma.platformLink.aggregate read that counter and return { _max: {
displayOrder: ... } } / { _count: { id: createdLinks } } as appropriate so
aggregate reflects successful inserts during retries; ensure the aggregate mock
and create mock share the same closure/state so aggregate updates after each
successful create.
In `@apps/backend/src/services/profileService.ts`:
- Line 131: Remove the unreachable final throw statement that reads "Failed to
allocate display order after max retries" in the display-order allocation loop
(the throw at the end of the function that contains the P2002 retry logic, e.g.,
in the allocateDisplayOrder or equivalent method in profileService.ts); the loop
already returns on success and rethrows on the last retry, so delete that dead
throw, ensure the function signature/type still compiles, and run TypeScript
build/tests to confirm no regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4ea01be-2f1b-49a7-841d-678aadde4408
📒 Files selected for processing (8)
apps/backend/prisma/migrations/20260612000000_platform_link_unique_display_order/migration.sqlapps/backend/prisma/schema.prismaapps/backend/src/__tests__/cards.test.tsapps/backend/src/__tests__/platform-link-ordering.test.tsapps/backend/src/__tests__/profile-cache.test.tsapps/backend/src/routes/cards.tsapps/backend/src/services/cardService.tsapps/backend/src/services/profileService.ts
|
|
||
| // ── App builders ────────────────────────────────────────────────────────────── | ||
|
|
||
| async function buildProfileApp(withRedis = true) { |
There was a problem hiding this comment.
Add explicit return type annotations.
The ESLint rule @typescript-eslint/explicit-function-return-type requires explicit return types on functions. Both app builder functions are missing return type annotations.
📝 Proposed fix
-async function buildProfileApp(withRedis = true) {
+async function buildProfileApp(withRedis = true): Promise<FastifyInstance> {
const app = Fastify({ logger: false });-async function buildPublicApp(withRedis = true) {
+async function buildPublicApp(withRedis = true): Promise<FastifyInstance> {
const app = Fastify({ logger: false });Also applies to: 101-101
🧰 Tools
🪛 GitHub Actions: CI / 1_backend-ci.txt
[warning] 87-87: @typescript-eslint/explicit-function-return-type: Missing return type on function
🪛 GitHub Actions: CI / backend-ci
[warning] 87-87: ESLint warning: Missing return type on function @typescript-eslint/explicit-function-return-type
🪛 GitHub Check: backend-ci
[warning] 87-87:
Missing return type on function
🤖 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 `@apps/backend/src/__tests__/profile-cache.test.ts` at line 87, The two app
builder functions are missing explicit TypeScript return types; add explicit
return type annotations for buildProfileApp and the other app builder function
referenced at the other location (line ~101) to satisfy
`@typescript-eslint/explicit-function-return-type`—e.g., annotate them as async
functions returning Promise<express.Application> (or the precise app type your
helpers return) so the signature reads like: async function
buildProfileApp(withRedis = true): Promise<express.Application> { ... } (apply
the same style to the second builder).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/prisma/schema.prisma">
<violation number="1" location="apps/backend/prisma/schema.prisma:54">
P1: Adding `@@unique([userId, displayOrder])` without first deduplicating existing duplicate rows in the migration will cause PostgreSQL to reject `CREATE UNIQUE INDEX` if duplicates exist. The migration must include a pre-step to remediate historical duplicates (e.g., assign sequential display orders per user) before creating the unique constraint.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| user User @relation(fields: [userId], references: [id], onDelete: Cascade) | ||
| cardLinks CardLink[] | ||
|
|
||
| @@unique([userId, displayOrder]) |
There was a problem hiding this comment.
P1: Adding @@unique([userId, displayOrder]) without first deduplicating existing duplicate rows in the migration will cause PostgreSQL to reject CREATE UNIQUE INDEX if duplicates exist. The migration must include a pre-step to remediate historical duplicates (e.g., assign sequential display orders per user) before creating the unique constraint.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/backend/prisma/schema.prisma, line 54:
<comment>Adding `@@unique([userId, displayOrder])` without first deduplicating existing duplicate rows in the migration will cause PostgreSQL to reject `CREATE UNIQUE INDEX` if duplicates exist. The migration must include a pre-step to remediate historical duplicates (e.g., assign sequential display orders per user) before creating the unique constraint.</comment>
<file context>
@@ -51,6 +51,7 @@ model PlatformLink {
user User @relation(fields: [userId], references: [id], onDelete: Cascade)
cardLinks CardLink[]
+ @@unique([userId, displayOrder])
@@map("platform_links")
}
</file context>
…ions Root cause: createPlatformLink, updatePlatformLink, deletePlatformLink, and reorderLinks all mutated the database but never called redis.del on the profile:<username> cache key, leaving stale data served to viewers until the 5-minute TTL expired naturally. Fix: Add a private invalidateProfileCacheForUser helper that resolves the username via a lightweight SELECT then calls redis.del. All four mutation functions now await this helper after a successful DB write so the cache is cleared immediately. Cache invalidation is skipped when Redis is absent and errors are caught and logged non-fatally so a Redis blip never fails a mutation request. Also fix the DELETE /api/cards/:id route handler which checked error codes as return values; the service throws errors, so the handler now catches them. Fix cards.test.ts duplicate buildApp declaration, and apply the PlatformLink type fix to cardService.ts (upstream/main has not yet merged that PR). Tests: 21 new tests in profile-cache.test.ts cover cache hit/miss lifecycle, all four mutation paths, failed mutations, non-existent links, Redis-absent mode, consecutive mutations, cache repopulation, and non-fatal Redis errors.
…aint and retry Concurrent createPlatformLink calls both read the same max(displayOrder) and insert the same value, corrupting link ordering for the user. - Add @@unique([userId, displayOrder]) to PlatformLink schema with migration - Wrap createPlatformLink in a retry loop (max 5 attempts) that re-reads max and retries on P2002 unique constraint violations - Reorder uses two-phase transaction (temp offset then final values) to avoid constraint conflicts when adjacent positions swap - Add platform-link-ordering.test.ts covering concurrency, retry, two-phase reorder, ordering integrity, and regression scenarios Closes Dev-Card#485
Ridanshi
force-pushed
the
fix/platform-link-display-order-race
branch
from
7f4104b to
b9591d0
Compare
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
2 participants
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
Fixes Dev-Card#485 — concurrent platform-link creation corrupts link ordering by assigning duplicate
displayOrdervalues.Root cause:
createPlatformLinkreadmax(displayOrder)and inserted in two separate operations with no transaction and no DB constraint. Two concurrent requests reading the same max would both attempt to insert with the samedisplayOrder.Changes:
schema.prisma: add@@unique([userId, displayOrder])toPlatformLink; migration in20260612000000_platform_link_unique_display_order/profileService.createPlatformLink: retry loop (max 5 attempts) re-reads max and retries onP2002unique constraint violations — the DB constraint catches races the application cannot prevent at READ COMMITTED isolationprofileService.reorderLinks: switch from batch array form to interactive$transactioncallback with two-phase updates (temp offset → final values) so swapping adjacent positions doesn't trigger the new unique constraint mid-transactionprofile-cache.test.ts: update$transactionmock to handle both array and callback formsplatform-link-ordering.test.ts: new test file covering display order assignment, P2002 retry (single, exhausted, non-P2002), concurrent request simulation, two-phase reorder verification, ordering integrity (sequential, delete-then-create), and regressionTest Plan
npx vitest run src/__tests__/platform-link-ordering.test.ts— 27 tests passnpx vitest run src/__tests__/profile-cache.test.ts— 18 tests pass (all existing cache tests preserved)npx vitest run src/__tests__/profiles.test.ts— 8 tests passnpx eslint src/services/profileService.ts src/__tests__/platform-link-ordering.test.ts— no errorsnpx tsc --noEmit— no type errorsSummary by cubic
Fixes duplicate platform-link displayOrder under concurrency and keeps public profiles fresh. Adds GitHub autodiscovery with cached suggestions for quick link setup.
Bug Fixes
(userId, displayOrder)and retrycreatePlatformLinkon P2002 (up to 5 attempts).$transaction(temp offset → final) to avoid unique conflicts.Migration
(user_id, display_order).Written for commit b9591d0. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes