Skip to content

fix(db): preserve legacy digest subscriptions#1693

Open
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-legacy-digest-subscription-lookup
Open

fix(db): preserve legacy digest subscriptions#1693
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-legacy-digest-subscription-lookup

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Recent changes normalized newly-inserted digest subscription login/email to lowercase but did not make legacy mixed-case rows discoverable, which caused duplicate rows and lookup/visibility regressions for existing subscriptions.
  • The intent is to preserve existing subscriptions and avoid introducing duplicate case-variant rows while keeping new-case normalization in place.

Description

  • Add migration migrations/0083_normalize_digest_subscription_logins.sql that canonicalizes existing digest_subscriptions by collapsing case-variant duplicates using lower(login), lower(email) and keeping the newest row per logical subscription.
  • Make lookup case-insensitive by changing listDigestSubscriptionsForLogin to compare lower(login) against the lowercased lookup key so legacy mixed-case rows remain visible prior to/after migration.
  • Add unit/regression tests in test/unit/product-usage.test.ts that exercise migration normalization and that a legacy mixed-case row is visible via the lookup API.

Testing

  • Ran the focused unit test group with npx vitest run test/unit/product-usage.test.ts -t "digest subscription", which executed the new tests and passed.
  • Ran migration guard npm run db:migrations:check, which reported the migration set is contiguous and OK.
  • Ran typecheck npm run typecheck, which passed with no TypeScript errors.
  • Attempted coverage npm run test:coverage -- test/unit/product-usage.test.ts: the tests executed but coverage remapping failed with TypeError: jsTokens is not a function during V8 coverage processing.
  • Attempted full gate npm run test:ci: it stopped early in actionlint due to network resolution failure (fallback flagged a custom self-hosted runner label), and npm audit --audit-level=moderate failed to reach the audit endpoint (HTTP 403), so those CI-level steps did not complete in this environment.

Codex Task

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 28, 2026
@JSONbored JSONbored self-assigned this Jun 28, 2026
@gittensory-orb

gittensory-orb Bot commented Jun 28, 2026

Copy link
Copy Markdown

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-06-29 16:19:45 UTC

3 files · 1 AI reviewer · no blockers · readiness 68/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The change adds a one-time migration that lowercases and collapses digest subscription case variants, and switches login lookup to `lower(login)` so legacy mixed-case rows remain visible. The repository path is consistent with existing upsert normalization, and the new tests cover both the migration and lookup regression. No must-fix defects are visible in this diff; the main tradeoff is that lookup now uses a functional predicate instead of the existing indexed column predicate.

Nits — 6 non-blocking
  • nit: `src/db/repositories.ts:1366` applies `lower()` to the column, which will not use `digest_subscriptions_login_idx`; add an expression index or retain an indexed equality fast path if this table can grow.
  • nit: `migrations/0083_normalize_digest_subscription_logins.sql:1` rewrites the whole table without an explicit transaction in the migration text; confirm the migration runner wraps files atomically or add explicit transaction boundaries.
  • nit: `migrations/0083_normalize_digest_subscription_logins.sql:12` relies on `updated_at`, `created_at`, then `id` to choose the surviving conflicting row, and a short comment would make that retention policy auditable.
  • nit: `test/unit/product-usage.test.ts:346` opens a `DatabaseSync` handle without closing it; wrap the migration test in `try/finally` with `db.close()` to avoid leaking handles as this test grows.
  • Add a follow-up migration/schema update for a case-folded lookup or uniqueness index, such as an index over `lower(login)` or `lower(login), lower(email)`, if case-insensitive access is now part of the DB contract.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Review load ❌ 8/20 Readiness component derived from cached public PR metadata and labels; size label size:M.
Validation evidence ✅ 25/25 PR body includes validation/test evidence.
Open PR queue ❌ 3/10 25 open PR(s), 12 likely reviewable, 13 unlinked.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 74 PR(s), 280 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 74 PR(s), 280 issue(s).
  • Related work: Titles/paths share 6 meaningful terms. (PR #1716)
  • Related work: Titles/paths share 8 meaningful terms. (PR #1679, PR #1690)
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Review top overlaps.
  • Add scope summary.
  • Expect slower review.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added gittensor Gittensor contributor context gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. labels Jun 28, 2026
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.58%. Comparing base (8652e6c) to head (0199388).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1693   +/-   ##
=======================================
  Coverage   95.58%   95.58%           
=======================================
  Files         204      204           
  Lines       22295    22295           
  Branches     8053     8053           
=======================================
  Hits        21310    21310           
  Misses        408      408           
  Partials      577      577           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.16% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. gittensor Gittensor contributor context size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant