Skip to content

fix(slugs): disambiguate duplicate set and artist slugs - #289

Merged
chiptus merged 8 commits into
mainfrom
claude/issue-135-implementation-f0aal5
Aug 13, 2026
Merged

fix(slugs): disambiguate duplicate set and artist slugs#289
chiptus merged 8 commits into
mainfrom
claude/issue-135-implementation-f0aal5

Conversation

@chiptus

@chiptus chiptus commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Disambiguates a colliding set/artist slug with a numeric counter (-2, -3, ...) instead of inserting a duplicate. Generation and dedup now happen entirely at the DB layer — a BEFORE INSERT trigger derives the slug (via public.slugify) and resolves collisions under a transaction-scoped advisory lock, backed by a unique constraint (sets: per festival edition; artists: global, already existed) so concurrent creates can't slip through either. The frontend no longer computes slugs client-side.

Closes #135

Verification

  • Create a set named the same as an existing set in the same edition; new set gets slug -2 and its detail page loads.
  • Create a set with a name colliding with a set in a different edition; slug is unchanged (no suffix).
  • Create an artist named the same as an existing artist; new artist gets slug -2 and its detail page loads.
  • Run the edge function integration tests (supabase/functions/commit-schedule/commit-schedule.test.ts) — two sets with the same name get distinct slugs exercises the DB-level dedup via the commit_schedule RPC.

Generated by Claude Code

Sets and artists were inserted with a purely deterministic slug, so two
records with the same name collided and broke slug-based lookups
(useSetBySlug/useArtistBySlug use .single()). On create, retry with a
numeric counter suffix (-2, -3, ...) when the slug collides, backed by
a DB-level unique constraint so concurrent creates can't still collide:
sets are unique per festival edition (matching lookup scope), artists
already had a global unique constraint.

Closes #135

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBa5F78BNbcTrjeAbYt5J6
Copilot AI lite review requested due to automatic review settings August 4, 2026 09:06
@vercel

vercel Bot commented Aug 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
upline Ready Ready Preview Aug 13, 2026 7:57am

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Deploy → stagingworkflow run
Last updated: 2026-08-13 07:57:53 UTC

  • DB migrations succeeded
  • Edge functions succeeded

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

chiptus commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

The migrate / Push migrations (staging) failure isn't caused by this PR's migration — the CLI error is about 20260728000000_add_votes_user_id_profiles_fkey.sql (from #261, merged earlier) being out of order relative to what's already applied on staging, i.e. pre-existing migration history drift on the staging DB. This PR's own migration (20260804090308_add_sets_slug_unique.sql) is timestamped after everything else and isn't implicated.

Fixing this requires resetting/re-seeding the staging DB (supabase db reset --linked + resync), which is destructive to staging data — not something I'll run without explicit go-ahead. Flagging for a human to reset staging when convenient; I'll keep watching this PR in the meantime.


Generated by Claude Code

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Playwright test results

passed  50 passed

Details

stats  50 tests across 14 suites
duration  1 minute, 7 seconds
commit  0bf203f

Replace the app-level slug-counter logic with BEFORE INSERT triggers on
sets and artists, so every insert path is deduped automatically instead
of relying on each caller to pre-check or retry with a counter itself.

commit_schedule__upsert_artists relied on ON CONFLICT (slug) DO UPDATE
to reactivate archived artists, which would have raced the trigger (the
trigger could rewrite the slug before the conflict check runs, breaking
the match). Replaced it with an explicit "reactivate archived match,
else insert" — safe because artistsToCreate only ever contains slugs
absent among *active* artists, so a collision can only be archived.

App code (useCreateSet/useCreateArtist) now just retries the plain
insert a few times on 23505, as a backstop for the trigger racing a
concurrent insert to the same candidate slug.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBa5F78BNbcTrjeAbYt5J6

chiptus commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Expanded the fix per discussion: slug dedup now lives in BEFORE INSERT triggers on sets/artists, so it applies to every insert path (not just useCreateSet/useCreateArtist) instead of needing each caller to implement it.

This required touching commit_schedule__upsert_artists: it used ON CONFLICT (slug) DO UPDATE to reactivate archived artists on schedule import, which would race the new trigger (the trigger can rewrite NEW.slug before Postgres checks the conflict target, breaking the match). Replaced it with an explicit "find archived artist by slug and reactivate, else insert" — safe because diff-schedule only ever passes slugs absent among active artists, so a collision can only be against an archived one.

App code is now a thin retry-on-23505 wrapper (backstop for the trigger racing a concurrent insert to the same candidate), not the dedup logic itself.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

…ocks

Move slug generation itself into the insert triggers (public.slugify,
mirroring generateSlug/commit_schedule__slugify), not just dedup — the
frontend no longer computes or passes a meaningful slug on create, so
there's a single source of truth. commit_schedule__slugify now delegates
to public.slugify instead of duplicating the same regex.

Also close the dedup race outright: each trigger takes a transaction-
scoped advisory lock keyed on (scope, base slug) before checking for a
collision, so concurrent creates for the same name serialize instead of
racing to the same candidate. That removes the need for the app-level
retry-on-23505 loop from the previous commit — useCreateSet/
useCreateArtist are back to a single plain insert.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBa5F78BNbcTrjeAbYt5J6

chiptus commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Simplified further per discussion:

  • Slug generation moved into the DB too, not just dedup: public.slugify() mirrors generateSlug/commit_schedule__slugify, and the triggers now derive the slug from name themselves — commit_schedule__slugify delegates to public.slugify so there's one implementation. useCreateSet/useCreateArtist no longer call generateSlug at all; the slug field they submit is just an empty placeholder the trigger always overwrites (required by the NOT NULL column, no DB default).
  • Dedup race closed outright, not just retried around: each trigger takes a transaction-scoped pg_advisory_xact_lock keyed on (scope, base slug) before checking for a collision, so two concurrent creates for the same name now serialize instead of racing to the same candidate. That means the app-level retry-on-23505 loop is gone — both hooks are back to a single plain insert. The unique constraints stay as the hard backstop.

Typecheck/lint/unit tests all still green locally.


Generated by Claude Code

The dedupe triggers were unconditionally recomputing the slug from
`name`, ignoring whatever the caller passed in. That broke
commit_schedule and its tests: they insert artists with a precomputed
slug and then look that exact artist back up by slug later in the same
call (e.g. via artistSlugs) -- recomputing from name produced a
different value and the lookup found nothing ("Unknown artist slug(s)
in payload... resolved 0").

Only fall back to deriving from `name` when the caller didn't supply a
slug (the app create paths now pass an empty placeholder); otherwise
use the caller's slug as the dedupe base, same as before this PR's
last commit.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBa5F78BNbcTrjeAbYt5J6

chiptus commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

CI caught a real regression: Run Edge Function Tests failed because the trigger was unconditionally recomputing the slug from name, ignoring any slug the caller supplied. commit-schedule.test.ts (and commit_schedule itself, in production) inserts artists with a precomputed slug and then looks that exact artist back up by slug later in the same call via artistSlugs — recomputing silently produced a different value, so the lookup found nothing ("Unknown artist slug(s) in payload... resolved 0").

Fixed: the trigger now only derives from name when the caller didn't supply a slug (the app create paths pass an empty placeholder); if the caller did supply one, it's trusted as the dedupe base, same as before the previous commit. Pushed as 2bcfdab.


Generated by Claude Code

Comment thread src/api/artists/useCreateArtist.ts Outdated
Comment thread supabase/migrations/20260804103506_commit_schedule_slugify_delegates.sql Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

supabase/migrations/20260804101135_reactivate_archived_artists_on_upsert.sql:32

  • commit_schedule__upsert_artists can still INSERT a new row even when an active artist already exists for the incoming slug (e.g., manual RPC call, or a race where the diff step was computed before another transaction created the artist). With artists_dedupe_slug now rewriting slugs on insert, that can create unexpected extra artists ("-2", "-3", …) while downstream set syncing continues to reference the original payload slugs (potentially linking sets to the pre-existing artist instead of the newly inserted one). Consider taking the same advisory lock key as the trigger and skipping the INSERT when any artist already exists for that slug.
    UPDATE artists
    SET name = v_elem->>'name', archived = false
    WHERE slug = v_elem->>'slug' AND archived = true;

    IF NOT FOUND THEN
      INSERT INTO artists (name, slug, added_by)
      VALUES (v_elem->>'name', v_elem->>'slug', p_user_id);
    END IF;

…ments

- commit_schedule__upsert_stages and commit_schedule__create_sets now
  call public.slugify() directly instead of going through the
  commit_schedule__slugify wrapper, which is dropped since it's now
  unused (no reason for the extra indirection).
- Shorten the placeholder-slug comments in useCreateSet/useCreateArtist
  to one line.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBa5F78BNbcTrjeAbYt5J6

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (5)

supabase/migrations/20260804101202_add_slug_dedupe_triggers.sql:87

  • The artist slug dedupe trigger only runs on INSERT. If an artist is renamed to a name whose slug collides with another artist, the update path will currently hit the unique constraint instead of applying the same numeric disambiguation. Consider firing the trigger on UPDATE of name/slug too.
CREATE TRIGGER artists_dedupe_slug_trigger
  BEFORE INSERT ON public.artists
  FOR EACH ROW
  EXECUTE FUNCTION public.artists_dedupe_slug();

supabase/migrations/20260804101202_add_slug_dedupe_triggers.sql:54

  • The slug dedupe trigger only runs on INSERT. With the new (festival_edition_id, slug) unique constraint, renaming a set (or changing its slug) to a colliding value will now throw a constraint error instead of applying the same "-2" disambiguation behavior. Consider firing this trigger on UPDATE of name/slug as well to keep behavior consistent.

This issue also appears on line 84 of the same file.

CREATE TRIGGER sets_dedupe_slug_trigger
  BEFORE INSERT ON public.sets
  FOR EACH ROW
  EXECUTE FUNCTION public.sets_dedupe_slug();

supabase/migrations/20260804103506_commit_schedule_slugify_delegates.sql:62

  • Using only the first 8 chars of the UUID to disambiguate set slugs is not guaranteed unique; in the unlikely event of a prefix collision within an edition this UPDATE will violate the (festival_edition_id, slug) unique constraint and abort the import. Prefer appending the full UUID (as done in the sets slug dedupe migration) or a longer suffix with a uniqueness check.
    UPDATE sets
    SET slug = slug || '-' || SUBSTRING(v_new_set_id::text, 1, 8)
    WHERE id = v_new_set_id;

supabase/migrations/20260804101135_reactivate_archived_artists_on_upsert.sql:26

  • With artists_dedupe_slug in place, an INSERT here can silently get a suffixed slug (e.g. "foo-2") if the requested slug already exists, which can leave newly created artists unreferenced by the payload (since sets link artists by the original slug). Guard against inserting when any row already exists for the slug so commit_schedule stays slug-stable/idempotent.
    IF NOT FOUND THEN
      INSERT INTO artists (name, slug, added_by)
      VALUES (v_elem->>'name', v_elem->>'slug', p_user_id);
    END IF;

src/api/sets/useCreateSet.ts:31

  • PR description says pnpm test covers "slugCandidate" counter logic in src/lib/slug.test.ts, but the current tests only cover generateSlug/isValidSlug/sanitizeSlug and do not exercise collision disambiguation (which is now implemented in DB triggers). Please update the PR description or add a test that validates the new collision behavior.
    slug: "", // auto-generated by the sets_dedupe_slug DB trigger

@chiptus
chiptus merged commit b9425e2 into main Aug 13, 2026
14 checks passed
@chiptus
chiptus deleted the claude/issue-135-implementation-f0aal5 branch August 13, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sets with duplicate names get duplicate slugs

3 participants