Repository navigation
fix(service): Make short finding URLs the default - #549
Conversation
Give each finding a permanent short URL and redirect existing UUID links. Keep repeated findings distinct so opening a short URL preserves the selected record, including after retention deletes older findings. Backfill existing records during deployment and keep the migration compatible with writes from the previous service version. Refs GH-546 Co-Authored-By: GPT-6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The migration and redirect flow look solid. Running the backfill, counter seeding, and trigger creation inside the migrator transaction (behind the ALTER TABLE lock) means old-version inserts can't slip through without a short_id. The legacy-schema integration test covers that path well.
One concurrency concern before this lands. The trigger's INSERT ... ON CONFLICT DO UPDATE on finding_short_id_counters takes a row lock that's held until the whole ingestRun transaction commits. insertFindings inserts rows one at a time in envelope order, and the ingest advisory lock is per clientRunId. So two runs ingesting at the same time can end up taking those locks in opposite orders, for example run A inserts ABC-DEF then XYZ-123 while run B inserts them the other way around. Then Postgres aborts one of them with a deadlock error. Reused codes across runs on the same PR are common because dedup carries the comment's finding ID forward. Nothing in the ingest path retries 40P01, so that run's ingest would just fail. The new concurrency test only uses one finding per envelope, so it won't catch this.
The smallest fix is to take the counter locks in a consistent order. You could sort envelope.findings by base code before inserting, or upsert the counter rows in sorted order at the top of insertFindings and let the trigger handle the rest. A two-finding, reversed-order concurrent ingest test would lock that in.
There was a problem hiding this comment.
Looks good to me. The rollout works because migrateDatabase runs everything in one transaction. The ALTER TABLE holds writes from the old deployment until the backfill, counter seeding, and trigger all commit, so no finding can be inserted without a short_id. The counter table also keeps retention deletes from reusing a URL. Ingest already locks the repository row and returns early on replays, so the counter upsert won't skip numbers on retries, and concurrent runs for the same repo get serialized.
Removing the collision check in getFindingDetail is correct now that (tenant_id, short_id) is unique. The scoped-token cases in the integration test cover what that check used to guard.
Make normal finding navigation use short URLs such as
/findings/ZWW-DCC. Existing UUID links redirect to the matching short URL and retain filters.Each finding keeps its URL. Repeated codes get a numeric suffix, and deleted findings' URLs are never reused. The deployment migration assigns IDs to existing records while keeping the current short link on the latest occurrence. It also accepts writes from the previous service version during rollout.
Verified with React navigation and PostgreSQL ingestion and migration integration tests. Lint, build, tests, and strict typechecking pass.
Follow-up to #546.