fix(valuation): deterministic oldest-first canonical pick (chat#1889 row 10) - #794
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
ChangesCanonical artist lookup
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 2 files
Confidence score: 4/5
- In
lib/valuation/findCanonicalArtistBySpotifyId.ts, ties on equalupdated_atare left to Supabase’s unspecified row order, so canonical artist selection can vary across runs and cause inconsistent valuation results; add a deterministic secondary sort key (for exampleid) to make tie-breaking stable.
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="lib/valuation/findCanonicalArtistBySpotifyId.ts">
<violation number="1" location="lib/valuation/findCanonicalArtistBySpotifyId.ts:31">
P2: Equal `updated_at` values are not deterministically resolved: this comparator returns `0`, preserving the unspecified order returned by Supabase. Adding a stable secondary key (such as `id`) would prevent the canonical pick from flipping when socials share the same timestamp.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // pick can flip between two lookups in the same add (chat#1889 row 10). | ||
| // The oldest social is the stable, true canonical. | ||
| const oldestFirst = [...socials].sort( | ||
| (a, b) => new Date(a.updated_at ?? 0).getTime() - new Date(b.updated_at ?? 0).getTime(), |
There was a problem hiding this comment.
P2: Equal updated_at values are not deterministically resolved: this comparator returns 0, preserving the unspecified order returned by Supabase. Adding a stable secondary key (such as id) would prevent the canonical pick from flipping when socials share the same timestamp.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/valuation/findCanonicalArtistBySpotifyId.ts, line 31:
<comment>Equal `updated_at` values are not deterministically resolved: this comparator returns `0`, preserving the unspecified order returned by Supabase. Adding a stable secondary key (such as `id`) would prevent the canonical pick from flipping when socials share the same timestamp.</comment>
<file context>
@@ -24,7 +24,14 @@ export async function findCanonicalArtistBySpotifyId(
+ // pick can flip between two lookups in the same add (chat#1889 row 10).
+ // The oldest social is the stable, true canonical.
+ const oldestFirst = [...socials].sort(
+ (a, b) => new Date(a.updated_at ?? 0).getTime() - new Date(b.updated_at ?? 0).getTime(),
+ );
+
</file context>
| (a, b) => new Date(a.updated_at ?? 0).getTime() - new Date(b.updated_at ?? 0).getTime(), | |
| (a, b) => | |
| new Date(a.updated_at ?? 0).getTime() - new Date(b.updated_at ?? 0).getTime() || | |
| a.id.localeCompare(b.id), |
…row 10) findCanonicalArtistBySpotifyId iterated socials newest-first, and enrichSearchedArtistProfile bumps updated_at mid-flow -- so the pick could flip between the creation call and the valuation fallback within one add. Oldest-first is stable, and the oldest social is the true canonical (verified on prod: Ana Barbara's is the 2025-05-30 row that OneRPM org accounts + sweetmantech@gmail.com already share). No account context, per the 2026-07-29 decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
475ec6b to
c0f5efb
Compare
|
Rebased onto Status update after the #49/#50 data cleanups: prod currently has zero Spotify ids resolving to >1 artist, so this pick-order change has no behavioral effect on today's data. It stays worth merging as the code-level guarantee of that invariant: one live regrowth vector remains until chat#1900 lands (the onboarding form still client-mints a socialed row when adding an artist another account has), and if a duplicate reappears, newest-first picks the newest dupe — the exact failure mode that doubled rosters on 07-28 — while oldest-first picks the established canonical. One sort + one test; belt-and-suspenders by design. |
![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
🧹 Nitpick comments (1)
lib/valuation/findCanonicalArtistBySpotifyId.ts (1)
27-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the lookup function under 20 lines.
findCanonicalArtistBySpotifyIdnow spans 26 lines. Extract timestamp normalization and/or oldest-first ordering into a focused helper so the lookup remains small and single-purpose.As per coding guidelines: “Flag functions longer than 20 lines” and “Keep functions small and focused.”
🤖 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 `@lib/valuation/findCanonicalArtistBySpotifyId.ts` around lines 27 - 34, Reduce findCanonicalArtistBySpotifyId below 20 lines by extracting timestamp normalization and/or the oldest-first social ordering into a focused helper. Keep the lookup function responsible only for canonical artist lookup and preserve the existing stable oldest-first ordering behavior.Source: Coding guidelines
🤖 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 `@lib/valuation/findCanonicalArtistBySpotifyId.ts`:
- Around line 27-29: Update the ordering comment near
findCanonicalArtistBySpotifyId to describe the prior database-returned order as
unspecified, rather than claiming it was newest-first; retain the rationale that
selecting the oldest social record provides a stable canonical result.
- Around line 30-32: Update the oldestFirst sorting comparator in
findCanonicalArtistBySpotifyId to normalize malformed updated_at values to the
existing missing-timestamp fallback before subtraction. Ensure both compared
timestamps produce finite numeric values so sorting and oldest canonical social
selection remain deterministic.
---
Nitpick comments:
In `@lib/valuation/findCanonicalArtistBySpotifyId.ts`:
- Around line 27-34: Reduce findCanonicalArtistBySpotifyId below 20 lines by
extracting timestamp normalization and/or the oldest-first social ordering into
a focused helper. Keep the lookup function responsible only for canonical artist
lookup and preserve the existing stable oldest-first ordering behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 159026e1-bb30-49f6-bf53-1b5138dd85fa
⛔ Files ignored due to path filters (1)
lib/valuation/__tests__/findCanonicalArtistBySpotifyId.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (1)
lib/valuation/findCanonicalArtistBySpotifyId.ts
| // Oldest first: enrichment bumps updated_at mid-flow, so a newest-first | ||
| // pick can flip between two lookups in the same add (chat#1889 row 10). | ||
| // The oldest social is the stable, true canonical. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the ordering comment.
The previous implementation was not guaranteed to be newest-first; it used the database-returned order. Describe that order as unspecified rather than “newest-first” to keep the rationale accurate.
🤖 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 `@lib/valuation/findCanonicalArtistBySpotifyId.ts` around lines 27 - 29, Update
the ordering comment near findCanonicalArtistBySpotifyId to describe the prior
database-returned order as unspecified, rather than claiming it was
newest-first; retain the rationale that selecting the oldest social record
provides a stable canonical result.
| const oldestFirst = [...socials].sort( | ||
| (a, b) => new Date(a.updated_at ?? 0).getTime() - new Date(b.updated_at ?? 0).getTime(), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline lib/valuation/findCanonicalArtistBySpotifyId.ts --view expanded || true
echo "== file contents =="
cat -n lib/valuation/findCanonicalArtistBySpotifyId.ts
echo "== related test files =="
fd -a '.*canonical.*|.*findCanonical.*|spotify' . | sed 's#^\./##' | head -200
echo "== behavioral Date comparator probe =="
node - <<'JS'
function compare(a,b) {
return new Date(a.updated_at ?? 0).getTime() - new Date(b.updated_at ?? 0).getTime();
}
const cases = [
{updated_at: '2024-01-02T00:00:00Z'},
{updated_at: '2024-01-03T00:00:00Z'},
{updated_at: 'invalid'},
{updated_at: '2024-01-05T00:00:00Z'},
];
for (let i = 1; i <= 10; i++) {
const sorted = [...cases].sort(compare).map(c => c.updated_at);
console.log(JSON.stringify(sorted));
}
console.log(new Date('invalid').getTime());
console.log(new Date('2024-01-03T00:00:00Z').getTime() - new Date('invalid').getTime());
JSRepository: recoupable/api
Length of output: 3846
Normalize invalid timestamps before sorting.
new Date(value).getTime() returns NaN for malformed timestamps, and NaN - anything makes Array.sort’s comparator ineffective, so the oldest canonical social may not be selected reliably. Normalizing invalid values to the missing-timestamp fallback keeps canonical selection deterministic and consistent with the intended best-effort behavior.
🤖 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 `@lib/valuation/findCanonicalArtistBySpotifyId.ts` around lines 30 - 32, Update
the oldestFirst sorting comparator in findCanonicalArtistBySpotifyId to
normalize malformed updated_at values to the existing missing-timestamp fallback
before subtraction. Ensure both compared timestamps produce finite numeric
values so sorting and oldest canonical social selection remain deterministic.
Deployment testing — A/B against a controlled fixture proves the pick flipProd now holds zero multi-candidate Spotify ids (post database#49/#50), so the pick order is unobservable on live data. The honest test was a controlled fixture: one synthetic Spotify id ( Same
Both returned 200 with the artist linked; only the pick differed. That's the entire behavioral claim of this PR, demonstrated live. Fixture fully cleaned up — residue query returns 0 across accounts/socials/links, and the global invariant re-verified after cleanup: still 0 Spotify ids resolving to >1 artist. Two incidental findings while building the fixture, both good news: Ready to merge — closes the trilogy (rows 8–10 on chat#1889). |
1 participant
Row 10 of the canonical-artist trilogy on chat#1889.
findCanonicalArtistBySpotifyIditerated socials newest-first, andenrichSearchedArtistProfilebumpssocials.updated_atmid-flow — so the pick could flip between two lookups inside one add. Oldest-first is stable, and the oldest social is the true canonical (prod anchor: Ana Bárbara's 2025-05-30 row, shared by OneRPM org accounts + sweetmantech@gmail.com). No account context — per the 2026-07-29 decision that closed api#792.TDD red→green: the new test stages a newest social resolving to a fresh duplicate and an older one resolving to the true canonical, and asserts the old one wins. 145 valuation+artists tests pass;
tscat the 236 baseline; eslint clean.Sequencing: correctness is complete once database#49 (row 9) has collapsed existing duplicates — until then oldest-first is still a better pick than newest-first (stable + favors the true canonical) with no downside. Code-independent of api#793; no merge conflict (different files).
Tracked in chat#1889 (matrix row 10).
🤖 Generated with Claude Code
Summary by cubic
Make canonical artist selection deterministic by iterating socials oldest-first in
findCanonicalArtistBySpotifyIdto prevent flips whenenrichSearchedArtistProfileupdatessocials.updated_at. Aligns with chat#1889 (row 10); no account context required.updated_atascending and traverse oldest-first to select the stable canonical.Written for commit c0f5efb. Summary will update on new commits.
Summary by CodeRabbit