fix(slug): never slugify to an empty string [UPL-53] - #517
Merged
Merged
Conversation
…nly names public.slugify() and its generateSlug/toSlug mirrors collapsed names with no ASCII alphanumerics to "", which the dedupe triggers treat as "no slug supplied" and the slug-based lookups/unique constraints can't handle. Fall back to a deterministic md5-derived slug (UPL-53). Also drops the CSV-import guard that rejected such names outright, since the empty-slug case it was guarding against no longer exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016w93iLcniKNQm3TfiBZp7S
…ed md5 The fallback test compared generateSlug's output to its own md5 helper, which can't catch a divergence from Postgres' md5() output. Hardcode the expected hex digests instead, verified independently against Node's crypto md5 (same RFC 1321 algorithm). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016w93iLcniKNQm3TfiBZp7S
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Deploy →
|
Owner
Author
|
CI: "Require staging label for migrations" is failing, not fixable from this PR. This PR ships a new migration ( Generated by Claude Code |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Existing malformed slugs remain uncorrected, and clearing editable slug fields now produces an unintended hash.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds deterministic hash fallbacks so non-ASCII and punctuation-only names always receive valid slugs.
Changes:
- Adds matching SQL, browser, and Deno slug fallbacks.
- Removes CSV rejection of non-sluggable names.
- Adds MD5 and slug behavior tests.
| File | Description |
|---|---|
supabase/migrations/20261002050800_slugify_non_empty_fallback.sql |
Adds the database fallback. |
supabase/functions/diff-schedule/helpers.ts |
Mirrors fallback in Deno. |
supabase/functions/diff-schedule/helpers.test.ts |
Tests Deno fallback behavior. |
src/services/scheduleImport/parseCsv.ts |
Removes obsolete import guard. |
src/services/scheduleImport/parseCsv.test.ts |
Covers newly accepted names. |
src/lib/slug.ts |
Adds browser-side fallback. |
src/lib/slug.test.ts |
Tests deterministic fallback slugs. |
src/lib/md5.ts |
Implements synchronous MD5. |
src/lib/md5.test.ts |
Validates MD5 vectors. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Playwright test results
Details
|
…e hash fallback Addresses Copilot review on #517: - Rows written before this fix could have slug = '' (or, for artists/sets, a '-2'/'-3' collision suffix on an empty base). Recompute those with the fixed public.slugify() and re-resolve any new collision by id, the same way the existing artists/sets unique-constraint migrations already do. - sanitizeSlug() delegated to generateSlug(), so clearing a manually- controlled slug field (festival/edition dialogs) silently filled it with a hash of the empty string and bypassed the "slug is required" validation. sanitizeSlug() now preserves blank input; only generateSlug() (deriving a slug from a name) applies the non-empty fallback. Verified the migration against a local Postgres 16 instance (seeded with legacy '', '-2', '-3' rows across artists/sets/groups/stages, including a same-name collision and a cross-edition case) before relying on it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016w93iLcniKNQm3TfiBZp7S
chiptus
commented
Oct 4, 2026
Addresses review on #517: replaces the duplicated ~90-line hand-rolled MD5 in src/lib/md5.ts and the private copy in the diff-schedule edge function with the blueimp-md5 package (zero deps, works in both the browser bundle and Deno via an npm: specifier) — same output, far less custom crypto-ish code to maintain. Also applies a review suggestion: generateSlug() now calls sanitizeSlug() instead of duplicating its stripping logic inline. Verified the Deno side against the real edge runtime (Deno 2.9.7, matching CI's deno-version: v2.x) rather than assuming the npm: import resolves. Adds explicit Hebrew/Chinese fallback test cases per review discussion — Hebrew is the actual name that surfaced this bug (see UPL-53's Linear discussion). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016w93iLcniKNQm3TfiBZp7S
Addresses review on #517: neither migration had reached staging/prod yet (every deploy run so far was blocked on the "Require staging label" check), so there's no reason to ship the slugify() redefinition and its backfill as two separate migrations. Folded 20261002051500_backfill_legacy_empty_slugs.sql into 20261002050800_slugify_non_empty_fallback.sql, keeping the function redefinition first so the backfill's UPDATEs call the fixed slugify(). Re-verified end-to-end as a single transaction against a local Postgres 16 instance (same legacy-row fixtures as before, including the same-name collision and cross-edition case) before pushing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016w93iLcniKNQm3TfiBZp7S
chiptus
commented
Oct 4, 2026
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The legacy-slug backfill can violate existing unique constraints before collision repair runs, aborting the migration.
Review effort: Balanced
Findings: 1
Open (3)
Resolved since last review (2)
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
…ce trim Addresses Copilot review on #517, three confirmed bugs: - The two-pass backfill (recompute, then separately re-suffix collisions) aborted the whole migration whenever two legacy rows shared a name: e.g. two artists both named "!!!" (slugs '' and '-2' from the old buggy trigger) both recompute to the same hash in one UPDATE, which violates artists_slug_unique before the re-suffix pass ever runs. Replaced with a per-row PL/pgSQL loop that resolves each row's collision (against existing rows and already-repaired siblings) before writing it, mirroring the dedupe triggers' own algorithm. Reproduced the abort and verified the fix against a local Postgres 16 instance seeded with this exact shape, plus a hash colliding with an unrelated existing group's slug. - public.slugify() used plain TRIM(), which only strips ASCII spaces, while the JS/Deno mirrors' .trim() also strips tabs and newlines. A tab-padded name hashed differently in the DB than in resolveArtists() during CSV import, which would miss the existing row and create a duplicate. Switched to a \s-based regex trim in both places the function uses TRIM(); verified the SQL and JS outputs now agree on a tab-padded Hebrew name. - resolveStage() treated two non-Latin stage names as a "close match" whenever both stripped to "" under the ASCII-only filter, now that such names can reach this function (the CSV-import guard that used to block them is gone). Added an explicit check: an empty stripped string never matches another empty stripped string. Note: no automated migration-level regression test accompanies the backfill fix — this sandbox has no Docker, so the repo's integration-test path (`pnpm run test:integration`, which needs a local Supabase/Postgres via Docker) isn't runnable here. Verified by hand instead with a close approximation: a plain Postgres 16 instance with the same tables/ constraints, seeded with each adversarial case from the review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016w93iLcniKNQm3TfiBZp7S
Playwright test results
Details
Flaky testswebkit › groups-flow.spec.ts › group lifecycle: create, invite, join, isolate, leave |
chiptus
changed the title
This branch was successfully deployed
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
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.
UPL-53:
public.slugify()and its JS/Deno mirrors collapsed names with no ASCII alphanumerics (non-Latin, punctuation-only) to"", which broke the dedupe triggers and slug-based lookups. All three now fall back to a deterministicn-<md5 prefix>slug, and the CSV-import guard that rejected such names outright (a stopgap for this exact bug) is removed.Verification
SELECT public.slugify('サカナクション')returns a stablen-xxxxxxxxslug, not''.!!!orכנסיית השכל; it gets a non-empty, stable slug.pnpm run typecheck,pnpm run lint,pnpm exec vitest run, andpnpm run buildall pass.Hello World→hello-world).🤖 Generated with Claude Code
https://claude.ai/code/session_016w93iLcniKNQm3TfiBZp7S
Generated by Claude Code