Skip to content

refactor(rpc): deepen LinkService and thin links router - #698

Open
sundaram2021 wants to merge 7 commits into
databuddy-analytics:mainfrom
sundaram2021:feat/links-deep-link-service
Open

refactor(rpc): deepen LinkService and thin links router#698
sundaram2021 wants to merge 7 commits into
databuddy-analytics:mainfrom
sundaram2021:feat/links-deep-link-service

Conversation

@sundaram2021

@sundaram2021 sundaram2021 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Deepens the Link module (candidate 01 from architecture review). Moves all distributed invariants — slug generation, deep-link validation, folder/targetDomain normalization, Redis lease lifecycle, PG error classification & reconciliation, and agent-context invalidation — from the shallow links.ts router (1,042 LOC) into a single deep LinkService module. Router becomes a thin adapter (303 LOC) that only does auth → service call → return.

No behavior change — purely improvement / code optimization. Preserves existing error codes (CONFLICT, SERVICE_UNAVAILABLE, BAD_REQUEST), retry semantics, fire-and-forget cache publish, and invalidateAgentContext handling.

What changed

3 commits — one per file (code only):

  1. feat(rpc): add LinkService deep module for link cache and persistence invariantspackages/rpc/src/services/link-service.ts (809 LOC)

    • Encapsulates generateLinkSlug (nanoid 8), validateDeepLinkConfiguration, normalize*, validateFolderId, toCachedLink, hasPostgresSqlState
    • Co-locates all cache helpers: abandonLinkCacheMutations, finishLinkCacheMutation, backfillLinkCache, tombstoneLinkCacheMutations, beginLinkCacheMutations (sorted, token-checked) — directly calls @databuddy/redis Lua scripts, no adapter interface (removed per ponytail review)
    • Owns create / update / delete / getLinkOrThrow with full PG unique-violation vs definitive vs uncertain branching and isUniqueViolationFor handling
    • Exports normalizeTargetDomain / normalizeNullableText for reuse (deduplicated from router)
  2. refactor(rpc): thin links router to delegate to LinkServicepackages/rpc/src/routers/links.ts (1,042 → 303 LOC, -739)

    • Keeps only query-side helpers (buildLinkListConditions, buildLinkSearchCondition, pagination) and getLinkOrThrow for reads
    • create / update / delete now: requireLinkAccessnew LinkService({ db: context.db })svc.create/update/delete
    • Removes duplicated normalize*, hasPostgresSqlState, isUniqueViolationFor, and all 5 cache helpers from router
  3. test(rpc): add LinkService tests for helpers and cache lease invariantspackages/rpc/src/services/link-service.test.ts (315 LOC, 9 tests)

    • Pure helper tests for normalizeNullableText / normalizeTargetDomain
    • Mocked @databuddy/redis via mock.module + fake db (in-memory Map, handles folder check / insert / reconciliation)
    • Covers: deep-link mismatch (BAD_REQUEST before DB), unknown folder, custom slug lease acquired, busyCONFLICT, Redis throw → SERVICE_UNAVAILABLE, generated slug 23505 retry, and backfill setIfAbsent fire-and-forget path

Impact

  • Locality: slug / cache / PG / domain logic in one module — understanding “when is cache authoritative?” no longer bounces 4 files (links.ts + links-cache.ts + db/schema + redis)
  • Leverage: one tested path for 4 mutations; future basket / links consumers can reuse LinkService instead of copying ceremony
  • Testability: service testable without live PG/Redis via fake db + mocked redis (9 new tests); router no longer needs DB for lease logic
  • Depth: interface shrinks to create(input) → Link | throw / update(link, patch) / delete(link) — implementation absorbs 5 helpers + 3 error branches

Net: -130 lines removed after ponytail trim (deleted LinkCachePort, createProdLinkCache/createInMemoryLinkCache, generate* DI, duplicated helpers, setTimeout shim). No new dependencies.

Tests ran

All are unit tests — no live PG/ClickHouse/Redis required.

  • bun test --isolate packages/rpc/src/services/link-service.test.ts9 pass, 0 fail (26 expects)
  • bun test --isolate packages/rpc/src/routers/links.test.ts — pass (validates createLinkSchema / updateLinkSchema still correct)
  • bun test --isolate packages/rpc/src/routers/links.test.ts packages/redis/links-cache.test.ts packages/rpc/src/services/link-service.test.ts packages/rpc/src/services/insight-schedule.test.ts34 pass, 0 fail
  • bun test --isolate packages/rpc/src/services/link-service.test.ts packages/rpc/src/routers/links.test.ts packages/rpc/src/routers/link-folders.test.ts packages/rpc/src/utils/billing.test.ts packages/redis/links-cache.test.ts43 pass, 0 fail
  • bun tsc --noEmit --project packages/rpc/tsconfig.json — pass
  • bunx ultracite check — pass (no fixes)
  • bun run lint:policies — 14 pass

No functional change — existing links.test.ts / links-cache.test.ts still pass; error codes and retry semantics preserved.


Summary by cubic

Moves all link mutation logic (slug generation, cache lease handling, PG error classification, folder/domain normalization) from the links router into a new LinkService, leaving the router to handle auth and return responses. Error codes and retry semantics are preserved, with behavior changes listed below.

Behavior changes

  • New setCachedLinkIfAbsentOrNotFound Redis script lets backfill replace missing, null, or malformed cache entries, but never a pending mutation or tombstone.
  • Ambiguous create results keep the cache lease instead of abandoning it, so negative caching cannot outlive a late commit.
  • createdBy is resolved after deep-link and folder validation.
  • Failed create, update, or delete abandon the cache lease before throwing on definitive errors.
  • Adds unit tests using a mock Redis and in-memory fake DB.

Written for commit f96f300. Summary will update on new commits.

Review in cubic

@vercel

vercel Bot commented Sep 1, 2026

Copy link
Copy Markdown

@sundaram2021 is attempting to deploy a commit to the Databuddy OSS Team on Vercel.

A member of the Team first needs to authorize it.

@vercel

vercel Bot commented Sep 1, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated
documentation Skipped Skipped Sep 1, 2026 7:21pm UTC

@vercel
vercel Bot temporarily deployed to Preview – documentation September 1, 2026 19:21 Inactive
@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 29c4b792-1997-4aaf-8c6e-fb0eec78476a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 3 files

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread packages/rpc/src/routers/links.ts Outdated
Comment thread packages/rpc/src/services/link-service.ts
Comment thread packages/rpc/src/services/link-service.ts
Comment thread packages/rpc/src/services/link-service.test.ts
…cache on backfill and abandon lease on ambiguous create

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 4 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread packages/rpc/src/services/link-service.ts
Comment thread packages/rpc/src/services/link-service.ts Outdated
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 1 file (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/rpc/src/services/link-service.ts Outdated
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 1 file (changes from recent commits).

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would require human review. Refactors the links router into a LinkService, but adds a new Redis Lua script with conditional cache-overwrite semantics (null/tombstone/invalid entries) not shown in the prior router path, so the 'no behavior change' claim is unverified and needs human sign-off.

Re-trigger cubic

@sundaram2021
sundaram2021 marked this pull request as ready for review September 1, 2026 19:57
@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR moves link mutation, validation, persistence reconciliation, and Redis lease handling from the links router into LinkService, and adds a Redis backfill primitive plus focused service tests.

  • Thins create, update, and delete router handlers into authorization and service delegation.
  • Centralizes slug generation, normalization, folder validation, PostgreSQL error handling, and cache mutation lifecycle.
  • Adds lease and helper tests using mocked Redis and a fake database.

Confidence Score: 3/5

The PR is not safe to merge until uncertain creates retain their lease and delayed create backfills cannot overwrite newer deletion tombstones.

The changed cache lifecycle permits unresolved custom-slug creates to lose their coordination guard and permits an older generated-link backfill to restore a deleted link in Redis.

Files Needing Attention: packages/rpc/src/services/link-service.ts and packages/redis/links-cache.ts

Important Files Changed

Filename Overview
packages/redis/links-cache.ts Adds backfill capable of replacing not-found states, but unconditional tombstone replacement can overwrite a newer deletion.
packages/rpc/src/services/link-service.ts Centralizes mutation invariants, but releases custom-slug leases while create outcomes remain uncertain and introduces prohibited broad types.
packages/rpc/src/routers/links.ts Successfully reduces mutation handlers to authorization, argument adaptation, and LinkService delegation.
packages/rpc/src/services/link-service.test.ts Adds useful create and lease tests, though it does not exercise ambiguous persistence or tombstone/backfill ordering.

Sequence Diagram

sequenceDiagram
    participant Router as Links Router
    participant Service as LinkService
    participant Redis as Redis Link Cache
    participant DB as PostgreSQL
    Router->>Service: create/update/delete
    Service->>Redis: begin mutation lease
    Service->>DB: persist mutation
    alt confirmed success
        Service->>Redis: publish link or tombstone
    else definitive failure
        Service->>Redis: abandon lease
    else uncertain outcome
        Service->>DB: reconcile
        Service-->>Router: result or service unavailable
    end
Loading

Reviews (1): Last reviewed commit: "refactor: update indentation" | Re-trigger Greptile

Comment thread packages/rpc/src/services/link-service.ts Outdated
Comment thread packages/redis/links-cache.ts Outdated
Comment thread packages/rpc/src/services/link-service.ts
…l from overwriting tombstones

Keep pending lease for ambiguous inserts so late commits are not lost to negative caching or second writers; backfill now only replaces missing/null/corrupt entries, never pending or tombstone (prevents resurrecting deleted links).

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 2 files (changes from recent commits).

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would require human review. Refactor continues, but the delta removes abandonLinkCacheMutations calls on uncertain persistence paths and retains a Lua script that overwrites null/invalid cache entries, so the 'no behavior change' claim is not substantiated and cache-consistency semantics need a human owner.

Re-trigger cubic

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.

1 participant