refactor: migrate from better-sqlite3 to libsql #2228
Conversation
WalkthroughThis PR migrates the database layer from better-sqlite3 (synchronous SQLite) to libsql ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
karakeep/packages/db/migrate.ts
Line 5 in a32127b
Await libsql migrations
The switch to drizzle-orm/libsql/migrator makes migrate(...) async (usage in upstream docs is await migrate(db, …)), but the migration entrypoint now fires it without awaiting or catching the promise. If the migration fails, the rejection is unhandled and pnpm db:migrate can report success while the schema remains unapplied; on some runtimes the process may even exit before the async work completes.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/routers/assets.ts (1)
21-27:ensureAssetOwnershipis querying by the wrong column/table
ensureAssetOwnershipis doing:const asset = await opts.ctx.db.query.assets.findFirst({ where: eq(bookmarks.id, opts.assetId), });This filters on
bookmarks.idwhile queryingassets, which should fail at runtime and never find any asset. It likely needs to compare againstassets.idinstead.Consider applying:
- const asset = await opts.ctx.db.query.assets.findFirst({ - where: eq(bookmarks.id, opts.assetId), - }); + const asset = await opts.ctx.db.query.assets.findFirst({ + where: eq(assets.id, opts.assetId), + });This will unblock
attachAsset,replaceAsset, anddetachAssetflows that rely on this helper.
🧹 Nitpick comments (12)
packages/benchmarks/docker-compose.yml (1)
20-20: Remove the commented-out line.The
# WORKERS_ENABLED_WORKERS: videocomment should be removed entirely rather than left as dead code.Apply this diff to clean up the commented-out line:
BROWSER_WEB_URL: http://chrome:9222 CRAWLER_NUM_WORKERS: 1 CRAWLER_ALLOWED_INTERNAL_HOSTNAMES: nginx DB_WAL_MODE: true - # WORKERS_ENABLED_WORKERS: video -packages/benchmarks/src/trpc.ts (2)
1-26: Retry predicate and attempts count may need tightening forretryLinkThe retry logic looks reasonable, but a couple of details are worth double‑checking:
- Relying on
opts.error.message.includes("fetch failed")is brittle: the message differs between Node and browsers (e.g."Failed to fetch"vs"fetch failed"), and may change across runtimes. Consider checking a more stable signal (e.g. error type or code) or at least handling both common message variants so you don’t silently skip retries in some environments.- Confirm whether
opts.attemptsis “number of retries so far” or “total attempts including the first one” for@trpc/clientv11.4.3. As written,opts.attempts <= 3could give 3 or 4 total tries depending on the API contract, so it’s worth verifying this matches your intended maximum.You could, for example, normalize the predicate and make the max‑retries intent explicit once you confirm the API:
retry({ attempts, error }) { const msg = error.message.toLowerCase(); const isNetworkFetchError = msg.includes("fetch failed") || msg.includes("failed to fetch"); if (!isNetworkFetchError) return false; const MAX_RETRIES = 3; return attempts < MAX_RETRIES; // or <=, depending on @trpc/client semantics }
35-44: Custom timeout overwrites upstreamsignal; consider composing abort signalsThe 60s timeout wrapper is good, but the override unconditionally replaces any existing
options.signal:fetch(url, { ...options, signal: controller.signal, } as RequestInit)If
httpBatchLink(now or in the future) passes its ownsignal, this drops upstream cancellation. You can safely preserve both by composing signals:fetch(url, options) { const timeoutController = new AbortController(); const timeoutId = setTimeout(() => timeoutController.abort(), 60_000); const originalSignal = options?.signal as AbortSignal | undefined; const controller = originalSignal == null ? timeoutController : (() => { const c = new AbortController(); const abort = () => c.abort(); originalSignal.addEventListener("abort", abort); timeoutController.signal.addEventListener("abort", abort); return c; })(); return fetch(url, { ...options, signal: controller.signal, } as RequestInit).finally(() => clearTimeout(timeoutId)); }This keeps your timeout behavior while still respecting any caller‑initiated aborts.
packages/trpc/models/backups.ts (1)
107-143: Use a localassetIdsnapshot indelete()to avoid drift and clarify intentThe new transactional delete block looks good overall (user‑scoped, single transaction, correct table/conditions). One small robustness tweak: you read
this.backup.assetIdin two places (before the transaction fordeleteAsset, and again inside the transaction for theassetsdelete). If the DB row’sassetIdcan ever change between loading thisBackupand callingdelete()(e.g., via another process or direct DB access), this method could end up deleting the “wrong” asset row or skipping the DB delete for the current one.Capturing the current
assetIdonce and using it consistently also makes the intent clearer:async delete(): Promise<void> { - if (this.backup.assetId) { - // Delete asset - await deleteAsset({ - userId: this.ctx.user.id, - assetId: this.backup.assetId, - }); - } + const assetId = this.backup.assetId; + + if (assetId) { + // Delete asset (storage / external system) + await deleteAsset({ + userId: this.ctx.user.id, + assetId, + }); + } - await this.ctx.db.transaction( - async (db) => { - // Delete asset first - if (this.backup.assetId) { + await this.ctx.db.transaction( + async (db) => { + // Delete asset row first + if (assetId) { await db .delete(assets) .where( and( - eq(assets.id, this.backup.assetId), + eq(assets.id, assetId), eq(assets.userId, this.ctx.user.id), ), ); } // Delete backup record await db .delete(backupsTable) .where( and( eq(backupsTable.id, this.backup.id), eq(backupsTable.userId, this.ctx.user.id), ), ); }, { behavior: "immediate", }, ); }Separately (non‑blocking), consider whether you want the external
deleteAssetcall to happen after a successful DB transaction to avoid the case where storage deletion succeeds but the DB transaction fails and leaves a dangling reference. If current behavior is intentional, a brief comment explaining the ordering choice could help future readers.apps/workers/workers/inference/tagging.ts (1)
323-353: Consider inlining the IIFE for improved readability.The async IIFE wrapping the tag matching logic adds nesting without clear benefit since the variables
matchedTagIdsandnotFoundTagNamescould be declared directly in the transaction scope. This is a minor style preference.- // Attempt to match exiting tags with the new ones - const { matchedTagIds, notFoundTagNames } = await (async () => { - const { normalizeTag, sql: normalizedTagSql } = tagNormalizer( - bookmarkTags.name, - ); - const normalizedInferredTags = inferredTags.map((t) => ({ - originalTag: t, - normalizedTag: normalizeTag(t), - })); - - const matchedTags = await tx.query.bookmarkTags.findMany({ - where: and( - eq(bookmarkTags.userId, userId), - inArray( - normalizedTagSql, - normalizedInferredTags.map((t) => t.normalizedTag), - ), - ), - }); - - const matchedTagIds = matchedTags.map((r) => r.id); - const notFoundTagNames = normalizedInferredTags - .filter( - (t) => - !matchedTags.some( - (mt) => normalizeTag(mt.name) === t.normalizedTag, - ), - ) - .map((t) => t.originalTag); - - return { matchedTagIds, notFoundTagNames }; - })(); + // Attempt to match existing tags with the new ones + const { normalizeTag, sql: normalizedTagSql } = tagNormalizer( + bookmarkTags.name, + ); + const normalizedInferredTags = inferredTags.map((t) => ({ + originalTag: t, + normalizedTag: normalizeTag(t), + })); + + const matchedTags = await tx.query.bookmarkTags.findMany({ + where: and( + eq(bookmarkTags.userId, userId), + inArray( + normalizedTagSql, + normalizedInferredTags.map((t) => t.normalizedTag), + ), + ), + }); + + const matchedTagIds = matchedTags.map((r) => r.id); + const notFoundTagNames = normalizedInferredTags + .filter( + (t) => + !matchedTags.some( + (mt) => normalizeTag(mt.name) === t.normalizedTag, + ), + ) + .map((t) => t.originalTag);Also note: Line 322 has a typo - "exiting" should be "existing".
packages/db/drizzle.config.ts (1)
7-18: Confirm turso dialect URL format; considerfile:prefix for local fallbackSwitching the dialect to
"turso"makes sense for libsql, butdbCredentials.urlis still a bare filesystem path. Drizzle/libsql examples typically use a URL with a scheme (e.g.libsql://...orfile:./db.db). If this config is used as a fallback for local dev/migrations, consider making it an explicit file URL:-const databaseURL = serverConfig.dataDir - ? `${serverConfig.dataDir}/db.db` - : "./db.db"; +const databaseURL = serverConfig.dataDir + ? `file:${serverConfig.dataDir}/db.db` + : "file:./db.db";Please double-check against your drizzle-kit/libsql setup to ensure this URL shape is what those tools expect in your environments.
packages/trpc/auth.ts (1)
36-47: Use strict equality for the rowsAffected check
rowsAffectedis numeric, so using strict equality keeps things type-safe and consistent with the rest of the codebase:- if (res.rowsAffected == 0) { + if (res.rowsAffected === 0) {packages/trpc/lib/__tests__/ruleEngine.test.ts (1)
77-82: Async DB initialization in tests is consistent; consider reusinggetTestDBhelperSwitching
beforeEachtoawait getInMemoryDB(true)matches the new async DB setup and looks correct. For consistency with other tests and to keep one source of truth for test DB configuration, you might consider usinggetTestDB()frompackages/trpc/testUtils.tshere instead of callinggetInMemoryDBdirectly.packages/trpc/models/users.ts (1)
7-8: LibSQL‑awarecreateRawtransaction looks correct; consider logging non‑constraint failuresWrapping
createRawindb.transaction(..., { behavior: "immediate" })correctly makes the “determine first user → assign role → insert” flow atomic, and usinggetLibsqlErrorto mapSQLITE_CONSTRAINT_UNIQUEto aBAD_REQUEST(“Email is already taken”) keeps the previous semantics under libsql.For all other DB errors you now throw a generic
INTERNAL_SERVER_ERRORwithout logging the underlying cause. For easier production debugging, you may want to loge(or at leastlibsqlError) before throwing the generic TRPCError, while still keeping the client‑facing message sanitized.Also applies to: 86-143
packages/trpc/models/tags.ts (1)
17-18: LibSQL error handling for tag create/update is sound; consider wrapping non‑constraint DB errorsUsing
getLibsqlErrorto detectSQLITE_CONSTRAINT_UNIQUEin bothTag.createandTag.updatecorrectly preserves the “duplicate tag name → BAD_REQUEST” behavior after moving to libsql. For non‑constraint cases, you currently rethrow the original error, which may surface DB details depending on your tRPC error formatting.If you want stricter separation between internal DB errors and client‑visible messages, consider logging
eand then throwing a genericTRPCErrorwithcode: "INTERNAL_SERVER_ERROR"for the fallback path, similar to the pattern inUser.createRaw.Also applies to: 59-83, 333-385
packages/trpc/models/lists.ts (1)
7-8: LibSQL primary‑key handling inaddBookmarkis correct; optional logging for other failures
ManualList.addBookmarknow usesgetLibsqlErrorand treatsSQLITE_CONSTRAINT_PRIMARYKEYas a no‑op, which is the right way to make “already in list” idempotent under the libsql driver. For any other DB error, you throw a genericINTERNAL_SERVER_ERROR, which is fine behaviorally.As in other modules, you might want to log
eorlibsqlErrorbefore throwing the generic TRPCError so that unexpected DB failures are easier to diagnose in production.Also applies to: 913-941
packages/db/drizzle.ts (1)
89-92: Consider addingconcurrencyoption for consistency with the main client.The test client is created without the
concurrency: 0option that's set on the main client (Line 22). While this may be intentional for test isolation, inconsistent settings could lead to different behavior between tests and production.const memClient = createClient({ url: `file:${tempFile}`, + concurrency: 0, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
apps/workers/package.json(1 hunks)apps/workers/tsdown.config.ts(1 hunks)apps/workers/workers/adminMaintenance/tasks/migrateLinkHtmlContent.ts(1 hunks)apps/workers/workers/crawlerWorker.ts(3 hunks)apps/workers/workers/inference/tagging.ts(1 hunks)apps/workers/workers/videoWorker.ts(1 hunks)docker/Dockerfile(2 hunks)docker/root/etc/s6-overlay/s6-rc.d/init-db-migration/run(1 hunks)packages/benchmarks/docker-compose.yml(1 hunks)packages/benchmarks/src/trpc.ts(3 hunks)packages/db/drizzle.config.ts(1 hunks)packages/db/drizzle.ts(1 hunks)packages/db/index.ts(2 hunks)packages/db/migrate.ts(1 hunks)packages/db/package.json(3 hunks)packages/db/tsdown.config.ts(1 hunks)packages/e2e_tests/docker-compose.yml(1 hunks)packages/trpc/auth.ts(1 hunks)packages/trpc/lib/__tests__/ruleEngine.test.ts(1 hunks)packages/trpc/lib/__tests__/search.test.ts(1 hunks)packages/trpc/models/backups.ts(1 hunks)packages/trpc/models/bookmarks.ts(1 hunks)packages/trpc/models/feeds.ts(1 hunks)packages/trpc/models/importSessions.ts(1 hunks)packages/trpc/models/listInvitations.ts(1 hunks)packages/trpc/models/lists.ts(8 hunks)packages/trpc/models/rules.ts(3 hunks)packages/trpc/models/tags.ts(6 hunks)packages/trpc/models/users.ts(4 hunks)packages/trpc/models/webhooks.ts(1 hunks)packages/trpc/routers/admin.ts(2 hunks)packages/trpc/routers/assets.ts(2 hunks)packages/trpc/routers/bookmarks.ts(4 hunks)packages/trpc/routers/prompts.ts(1 hunks)packages/trpc/testUtils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
packages/trpc/routers/prompts.tspackages/trpc/models/listInvitations.tsapps/workers/tsdown.config.tspackages/trpc/models/webhooks.tspackages/trpc/routers/admin.tspackages/trpc/models/bookmarks.tspackages/trpc/testUtils.tsapps/workers/workers/adminMaintenance/tasks/migrateLinkHtmlContent.tspackages/db/drizzle.config.tsapps/workers/workers/crawlerWorker.tspackages/db/tsdown.config.tspackages/trpc/models/feeds.tspackages/trpc/auth.tspackages/db/drizzle.tspackages/trpc/models/rules.tsapps/workers/workers/inference/tagging.tspackages/trpc/routers/assets.tspackages/trpc/models/importSessions.tspackages/trpc/routers/bookmarks.tspackages/db/index.tspackages/trpc/lib/__tests__/search.test.tspackages/trpc/models/tags.tspackages/trpc/models/backups.tspackages/trpc/lib/__tests__/ruleEngine.test.tsapps/workers/workers/videoWorker.tspackages/benchmarks/src/trpc.tspackages/trpc/models/lists.tspackages/db/migrate.tspackages/trpc/models/users.ts
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
packages/trpc/routers/prompts.tspackages/trpc/models/listInvitations.tsapps/workers/tsdown.config.tspackages/trpc/models/webhooks.tspackages/trpc/routers/admin.tspackages/trpc/models/bookmarks.tspackages/trpc/testUtils.tsapps/workers/workers/adminMaintenance/tasks/migrateLinkHtmlContent.tspackages/db/drizzle.config.tsapps/workers/workers/crawlerWorker.tspackages/db/tsdown.config.tspackages/trpc/models/feeds.tspackages/trpc/auth.tspackages/db/drizzle.tspackages/trpc/models/rules.tsapps/workers/workers/inference/tagging.tspackages/trpc/routers/assets.tspackages/trpc/models/importSessions.tspackages/trpc/routers/bookmarks.tspackages/db/index.tspackages/trpc/lib/__tests__/search.test.tspackages/trpc/models/tags.tspackages/trpc/models/backups.tspackages/trpc/lib/__tests__/ruleEngine.test.tsapps/workers/workers/videoWorker.tspackages/benchmarks/src/trpc.tspackages/db/package.jsonpackages/trpc/models/lists.tspackages/db/migrate.tsapps/workers/package.jsonpackages/trpc/models/users.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
packages/trpc/routers/prompts.tspackages/trpc/models/listInvitations.tsapps/workers/tsdown.config.tspackages/trpc/models/webhooks.tspackages/trpc/routers/admin.tspackages/trpc/models/bookmarks.tspackages/trpc/testUtils.tsapps/workers/workers/adminMaintenance/tasks/migrateLinkHtmlContent.tspackages/db/drizzle.config.tsapps/workers/workers/crawlerWorker.tspackages/db/tsdown.config.tspackages/trpc/models/feeds.tspackages/trpc/auth.tspackages/db/drizzle.tspackages/trpc/models/rules.tsapps/workers/workers/inference/tagging.tspackages/trpc/routers/assets.tspackages/trpc/models/importSessions.tspackages/trpc/routers/bookmarks.tspackages/db/index.tspackages/trpc/lib/__tests__/search.test.tspackages/trpc/models/tags.tspackages/trpc/models/backups.tspackages/trpc/lib/__tests__/ruleEngine.test.tsapps/workers/workers/videoWorker.tspackages/benchmarks/src/trpc.tspackages/trpc/models/lists.tspackages/db/migrate.tspackages/trpc/models/users.ts
packages/trpc/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Organize business logic in the tRPC router and procedures located in
packages/trpc
Files:
packages/trpc/routers/prompts.tspackages/trpc/models/listInvitations.tspackages/trpc/models/webhooks.tspackages/trpc/routers/admin.tspackages/trpc/models/bookmarks.tspackages/trpc/testUtils.tspackages/trpc/models/feeds.tspackages/trpc/auth.tspackages/trpc/models/rules.tspackages/trpc/routers/assets.tspackages/trpc/models/importSessions.tspackages/trpc/routers/bookmarks.tspackages/trpc/lib/__tests__/search.test.tspackages/trpc/models/tags.tspackages/trpc/models/backups.tspackages/trpc/lib/__tests__/ruleEngine.test.tspackages/trpc/models/lists.tspackages/trpc/models/users.ts
packages/db/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for database schema and migrations in the db package
Files:
packages/db/drizzle.config.tspackages/db/tsdown.config.tspackages/db/drizzle.tspackages/db/index.tspackages/db/migrate.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for writing and running tests
Files:
packages/trpc/lib/__tests__/search.test.tspackages/trpc/lib/__tests__/ruleEngine.test.ts
🧠 Learnings (10)
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/trpc/**/*.{ts,tsx} : Organize business logic in the tRPC router and procedures located in `packages/trpc`
Applied to files:
packages/trpc/routers/prompts.tspackages/trpc/routers/admin.tspackages/trpc/models/rules.tspackages/trpc/routers/assets.tspackages/trpc/models/tags.tspackages/benchmarks/src/trpc.ts
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Create database migrations using `pnpm db:generate --name description_of_schema_change` after schema updates
Applied to files:
docker/root/etc/s6-overlay/s6-rc.d/init-db-migration/rundocker/Dockerfilepackages/db/migrate.ts
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/db/**/*.{ts,tsx} : Use Drizzle ORM for database schema and migrations in the db package
Applied to files:
packages/trpc/testUtils.tspackages/db/drizzle.config.tspackages/db/tsdown.config.tspackages/db/drizzle.tspackages/db/index.tspackages/trpc/lib/__tests__/ruleEngine.test.tspackages/db/package.jsondocker/Dockerfilepackages/db/migrate.tspackages/trpc/models/users.ts
📚 Learning: 2025-11-16T13:02:08.919Z
Learnt from: MohamedBassem
Repo: karakeep-app/karakeep PR: 2141
File: packages/db/schema.ts:160-165
Timestamp: 2025-11-16T13:02:08.919Z
Learning: In this codebase using Drizzle ORM with SQLite, enum definitions (e.g., `text("status", { enum: ["value1", "value2"] })`) only provide TypeScript-level type safety and do not require database migrations when values are added or modified. SQLite stores these as TEXT columns without database-level CHECK constraints, so schema changes to enums don't need corresponding migration files.
Applied to files:
packages/db/drizzle.config.tspackages/db/drizzle.tspackages/db/index.tspackages/trpc/models/tags.ts
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/shared/**/*.{ts,tsx} : Organize shared code and types in the `packages/shared` directory for use across packages
Applied to files:
packages/db/tsdown.config.ts
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/api/**/*.{ts,tsx} : Use Hono and tRPC for building the API backend
Applied to files:
packages/benchmarks/src/trpc.ts
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to **/*.{ts,tsx} : Use TypeScript for type safety in all source files
Applied to files:
packages/db/package.json
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to **/*.{ts,tsx,js,jsx,json,css,md} : Format code using Prettier according to project standards
Applied to files:
packages/db/package.json
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Run `pnpm typecheck` to verify TypeScript type safety across the codebase
Applied to files:
packages/db/package.json
📚 Learning: 2025-11-27T16:02:47.440Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.440Z
Learning: Applies to packages/shared-server/**/*.{ts,tsx} : Place shared server-side logic in `packages/shared-server`
Applied to files:
apps/workers/package.json
🧬 Code graph analysis (11)
packages/trpc/models/listInvitations.ts (1)
packages/db/schema.ts (2)
listInvitations(488-519)listCollaborators(462-486)
apps/workers/workers/crawlerWorker.ts (3)
packages/db/drizzle.ts (1)
db(41-41)apps/workers/workerUtils.ts (1)
updateAsset(7-17)packages/db/schema.ts (4)
bookmarkAssets(339-351)bookmarks(154-213)bookmarkLinks(215-243)assets(259-295)
packages/db/drizzle.ts (1)
packages/db/index.ts (2)
db(8-8)DB(9-9)
packages/trpc/models/rules.ts (1)
packages/db/schema.ts (2)
ruleEngineRulesTable(648-684)ruleEngineActionsTable(686-720)
packages/trpc/routers/assets.ts (1)
packages/db/schema.ts (1)
assets(259-295)
packages/trpc/lib/__tests__/search.test.ts (1)
packages/db/drizzle.ts (2)
db(41-41)getInMemoryDB(81-104)
packages/trpc/models/tags.ts (1)
packages/db/index.ts (1)
getLibsqlError(22-33)
packages/trpc/lib/__tests__/ruleEngine.test.ts (1)
packages/db/drizzle.ts (2)
db(41-41)getInMemoryDB(81-104)
apps/workers/workers/videoWorker.ts (2)
packages/db/drizzle.ts (1)
db(41-41)apps/workers/workerUtils.ts (1)
updateAsset(7-17)
packages/trpc/models/lists.ts (2)
packages/db/index.ts (1)
getLibsqlError(22-33)packages/db/schema.ts (2)
bookmarksInLists(429-460)bookmarkLists(398-427)
packages/trpc/models/users.ts (1)
packages/db/index.ts (2)
db(8-8)getLibsqlError(22-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: typecheck
- GitHub Check: tests
- GitHub Check: format
🔇 Additional comments (43)
packages/benchmarks/docker-compose.yml (2)
17-17: Clarify the intent behind reducing CRAWLER_NUM_WORKERS from 6 to 1.In a benchmarks environment, the worker count significantly affects load profile and benchmark validity. Reducing from 6 to 1 is a major change.
Is this intentional to isolate database performance from crawler concurrency, or should the original value be retained to measure libsql performance under concurrent load?
19-19: DB_WAL_MODE addition is appropriate for libsql.Enabling WAL (Write-Ahead Logging) mode aligns with the libsql migration and follows modern SQLite best practices, improving concurrency and performance.
apps/workers/tsdown.config.ts (1)
13-17: [rewritten review comment]
[classification tag]packages/trpc/models/listInvitations.ts (1)
122-141: Transaction refactor withbehavior: "immediate"looks correct and preserves atomicityThe migration to
await this.ctx.db.transaction(async (tx) => { ... }, { behavior: "immediate" })keeps the accept flow logically identical: the invitation is deleted and the collaborator is inserted in a single atomic unit, andonConflictDoNothing()still makes this robust against repeats/races. The explicitbehavior: "immediate"is consistent with using libsql-style transactions and should give you predictable write locking semantics.I don’t see any new correctness or concurrency issues introduced by this change; assuming the types line up, this looks good.
Please double‑check that:
AuthedContext["db"]’stransactionAPI indeed expects(callback, { behavior })in your libsql/drizzle setup, and- Existing tests around invitation acceptance (and any new libsql-backed transaction tests) pass, to confirm there are no driver-specific quirks with
behavior: "immediate".apps/workers/workers/inference/tagging.ts (3)
356-370: Potential edge case: conflicting inserts may result in missing tag attachments.When using
onConflictDoNothing().returning(), only successfully inserted rows are returned. If another transaction creates the same tag after the initial query (line 332-340) but before this insert, those tag IDs won't appear in eithermatchedTagIdsornewTagIds, potentially leaving some tags unattached.The
immediatetransaction behavior mitigates this significantly, but for robustness, consider usingonConflictDoUpdateto return the existing row, or re-query to fetch IDs of all tags by name after insertion.This is likely a low-probability edge case given the immediate transaction lock, but verify if this could occur under high concurrency scenarios.
372-396: LGTM!The delete-then-attach pattern is correct. Using
Setfor deduplication andonConflictDoNothingontagsOnBookmarksproperly handles the case where a tag might already be attached by another source (e.g., manually by user). The returned values correctly capture only the actual changes for rule engine events.
398-412: MovetriggerRuleEngineOnEventoutside the transaction.
triggerRuleEngineOnEventenqueues a background job viaRuleEngineQueue.enqueue()inside the transaction. If the transaction rolls back after the enqueue succeeds, the queued job will process with stale event data. Collect the event data within the transaction and trigger the rule engine after the transaction completes to ensure consistency.packages/trpc/models/feeds.ts (1)
69-81: rowsAffected-based NOT_FOUND handling for delete looks correctUsing
res.rowsAffected === 0after the filtered delete is the right way to detect a missing feed and is consistent with the update path’s NOT_FOUND handling. No further changes needed here; just ensure your libsql/drizzle migration tests cover this branch.packages/trpc/models/importSessions.ts (1)
164-180: Import session delete now correctly checks rowsAffectedSwitching the not-found detection to
result.rowsAffected === 0after the scoped delete matches the expected libsql/drizzle behavior and aligns with other models’ NOT_FOUND semantics.apps/workers/package.json (1)
7-59: New @libsql/client dependency: keep version aligned across the workspaceAdding
"@libsql/client": "^0.15.15"to workers is fine if the workers use the client directly; just ensure this version stays in sync with the one used in@karakeep/dbso you don’t end up with mismatched libsql client versions in different packages.docker/root/etc/s6-overlay/s6-rc.d/init-db-migration/run (1)
4-6: Updated migration entrypoint matches compiled db buildPointing the init-db-migration script at
dist/migrate.jsunder/db_migrationsis consistent with the newpackages/dbbuild/deploy flow. Just keep this path in sync with any future changes to the db package’s build output layout.packages/trpc/models/webhooks.ts (1)
68-80: Webhook delete now correctly uses rowsAffected for NOT_FOUNDUsing
res.rowsAffected === 0after the scoped delete is the right way to detect that no webhook was deleted and keeps error semantics consistent with other models.docker/Dockerfile (1)
43-45: DB build/deploy flow and libsql bindings copy look consistent with the new migration setup
- Building the db package then deploying
@karakeep/dbto/db_migrationslines up with the init-db-migration script runningnode dist/migrate.jsfrom that directory.- Manually copying
/app/node_modules/@libsqlinto the final image ensures libsql’s native bindings are available despite Next.js standalone omitting optional dependencies.This looks coherent with the libsql migration; just verify that the deployed
/db_migrationstree indeed containsdist/migrate.jsand that@libsqlresolves correctly at runtime in the standalone app.Also applies to: 110-113
packages/db/package.json (2)
9-9: LGTM: Build script migration to tsdown.The switch to tsdown is appropriate for bundling the migration script with its dependencies (including workspace packages and native libsql bindings).
22-22: Version ^0.15.15 is current and has no known vulnerabilities.@libsql/client v0.15.15 is the latest stable version as of December 2025. No direct vulnerabilities have been reported for the npm @libsql/client package. While CVE-2025-47736 exists in the related libsql-sqlite3-parser Rust crate, it only affects versions ≤ 0.13.0 and does not impact this dependency.
packages/e2e_tests/docker-compose.yml (1)
19-19: LGTM: WAL mode enablement for e2e tests.Enabling WAL (Write-Ahead Logging) mode is appropriate for the LibSQL migration, as it supports better concurrency and aligns with LibSQL's operational characteristics.
packages/db/tsdown.config.ts (1)
1-22: LGTM: Well-structured tsdown configuration.The configuration properly:
- Externalizes native bindings (@libsql/client, libsql) that cannot be bundled
- Bundles workspace packages (/^@karakeep//) to create a self-contained migration script
- Uses modern ESM output targeting Node.js 22
- Includes sourcemaps for debugging
packages/db/migrate.ts (1)
1-5: LGTM: Correct migration to LibSQL async pattern.The changes properly:
- Update the import to use the libsql migrator
- Add await to handle the async migration (libsql operations are async unlike better-sqlite3)
packages/trpc/lib/__tests__/search.test.ts (1)
27-27: LGTM: Test updated for async DB initialization.The change correctly awaits the now-async getInMemoryDB() function, aligning with the LibSQL migration where database initialization requires asynchronous operations.
packages/trpc/models/bookmarks.ts (1)
820-820: LGTM: Updated to use standard rowsAffected property.The change from
changes(better-sqlite3 specific) torowsAffected(standard property used by libsql) correctly maintains the conditional logic for asset cleanup.apps/workers/workers/adminMaintenance/tasks/migrateLinkHtmlContent.ts (1)
117-117: LGTM: Updated to use standard rowsAffected property.The change from
changestorowsAffectedaligns with the LibSQL migration, maintaining the same error-throwing behavior when no rows are affected.packages/trpc/routers/prompts.ts (1)
110-110: LGTM: Updated to use standard rowsAffected property.The change from
changestorowsAffectedcorrectly adapts to the LibSQL client's API while preserving the NOT_FOUND error handling logic.packages/trpc/routers/admin.ts (1)
379-390:rowsAffected‑based NOT_FOUND checks align with libsql/drizzle semanticsUsing
result.rowsAffectedinstead ofchangesinupdateUserandresetPasswordpreserves the intended behavior (throwNOT_FOUNDwhen no row is updated) and matches the libsql/Drizzle result shape. No further issues from this change.Also applies to: 402-413
packages/trpc/routers/assets.ts (2)
166-180: Explicitbehavior: "immediate"on the replace transaction is appropriateAdding
{ behavior: "immediate" }to thectx.db.transactioncall keeps the existing atomic delete+update behavior while making the locking semantics explicit and consistent with the rest of the libsql migration. No issues from this change.
212-221:rowsAffectedcheck indetachAssetcorrectly drives NOT_FOUNDSwitching the delete result check to
result.rowsAffected == 0preserves the intended behavior (throwNOT_FOUNDwhen nothing was deleted) and matches other libsql/Drizzle usage in the PR. Note that with the fix toensureAssetOwnershipabove, this will correctly distinguish “asset exists but doesn’t match bookmark” from the happy path.apps/workers/workers/videoWorker.ts (1)
188-206: Immediate transaction aroundupdateAssetis consistent and safeWrapping
updateAssetindb.transaction(..., { behavior: "immediate" })maintains atomicity for deleting the old asset row and inserting the new one, and aligns this worker with the libsql transaction pattern used elsewhere. No additional issues introduced here.packages/trpc/testUtils.ts (1)
9-11: Async test DB helper and types are correctly updatedMaking
getTestDBasync, updatingTestDBtoAwaited<ReturnType<typeof getTestDB>>, and awaiting it inbuildTestContextkeeps the test utilities aligned with the asyncgetInMemoryDB(true)helper and preserves type safety for callers. This matches the Drizzle‑backed test DB pattern described for the@karakeep/dbpackage.Based on learnings, this stays consistent with the Drizzle ORM usage for the db package.
Also applies to: 13-14, 65-79
packages/trpc/models/users.ts (1)
218-229:rowsAffectedchecks in email verification and user deletion are correctSwitching from
changestoresult.rowsAffected/res.rowsAffectedinverifyEmailanddeleteInternalpreserves the intended behavior of throwingNOT_FOUNDwhen no user row is updated or deleted, and matches the libsql result shape used elsewhere in the PR. No issues detected here.Also applies to: 363-371
packages/trpc/models/tags.ts (1)
181-196:rowsAffectedusage and immediate merge transaction for tags are correct
deleteUnusednow returningres.rowsAffectedis the right adaptation fromchangesand gives a clear count of how many unused tags were removed.- The
mergeflow’sctx.db.transaction(..., { behavior: "immediate" })keeps the unlink → reinsert → delete‑tags sequence atomic and matches the new transaction pattern elsewhere.- In
delete, checkingres.rowsAffected === 0before throwingNOT_FOUNDcorrectly signals when the tag no longer exists or doesn’t belong to the current user.All of these changes look consistent with the libsql migration.
Also applies to: 246-283, 311-323
packages/trpc/models/lists.ts (2)
464-477:rowsAffected‑driven NOT_FOUND / BAD_REQUEST logic for lists and collaborators looks goodAcross
List.delete,removeCollaborator,leaveList,updateCollaboratorRole, andManualList.removeBookmark, switching fromresult.changestoresult.rowsAffectedpreserves the intended semantics:
rowsAffected === 0→NOT_FOUND(no such list/collaborator) orBAD_REQUEST(bookmark already not in list).rowsAffected > 0→ success.This matches the libsql result shape and keeps the API behavior unchanged.
Also applies to: 629-644, 651-675, 686-702, 943-961
979-1015: Merging lists inside an immediate transaction improves atomicityWrapping the bookmark move (insert with
onConflictDoNothing) and optional source list deletion in a singlectx.db.transaction(..., { behavior: "immediate" })ensures:
- Target list entries are created before the source list is removed.
- Deleting the source list correctly cascades its
bookmarksInListsrows.- The whole merge is atomic even under concurrent access.
This change is aligned with the broader transaction refactor.
packages/db/drizzle.ts (2)
25-41: LGTM!The pragma configuration is well-structured. The
busy_timeoutof 20 seconds provides adequate time for lock contention, and the conditional WAL mode based on config is a good pattern.
20-23: Setconcurrencytoundefinedif unlimited concurrent connections are intended.The
concurrency: 0setting does not enable unlimited concurrent connections. According to libsql documentation,concurrencysets the maximum number of simultaneous requests (default 20). To disable the concurrency limit entirely, setconcurrencytoundefinedinstead of0.Likely an incorrect or invalid review comment.
packages/trpc/models/rules.ts (2)
90-136: LGTM!The transaction with
{ behavior: "immediate" }is correct for the libsql migration. The rule creation logic properly handles event-based field mapping and action insertion within a single atomic transaction.
212-225: LGTM!The delete operation correctly uses
rowsAffectedfor the NOT_FOUND check. Related actions will be deleted via the foreign key cascade defined in the schema.apps/workers/workers/crawlerWorker.ts (3)
973-1006: LGTM!The transaction consolidation in
handleAsAssetBookmarkproperly groups the asset update, bookmark asset insertion, bookmark type change, and link deletion into a single atomic operation with immediate behavior. This ensures consistency when transforming a link bookmark to an asset bookmark.
1193-1263: LGTM!The consolidated transaction correctly handles all metadata and asset updates atomically. The pattern of collecting deletion tasks during the transaction and executing them after commit (Line 1266) is appropriate—file deletions are idempotent and don't need transactional guarantees.
1288-1307: LGTM!The archive update transaction follows the established pattern. The old asset cleanup after the transaction commit ensures data integrity.
packages/db/index.ts (2)
12-12: Good backward compatibility approach.Re-exporting
LibsqlErrorasSqliteErrorminimizes breaking changes for consumers of this package.
22-33: LGTM!The
getLibsqlErrorhelper correctly handles both directLibsqlErrorinstances and errors with a nested cause. This is useful for extracting the underlying database error from Drizzle ORM's error wrapping.packages/trpc/routers/bookmarks.ts (3)
147-279: LGTM!The
createBookmarkmutation correctly consolidates all bookmark creation logic (base bookmark, link/text/asset content, and asset ownership) into a single atomic transaction with immediate behavior. The switch statement cleanly handles the three bookmark types.
521-549: LGTM!The deprecated
updateBookmarkTextmutation correctly uses.returning()to verify the row exists, and properly updatesmodifiedAtwithin the same transaction.
729-850: LGTM!The
updateTagsmutation correctly handles the complex tag attach/detach logic within a single transaction. The use ofonConflictDoNothing()for both tag creation (Line 794) and relation insertion (Line 830) ensures idempotency without errors.
| "@tsconfig/node22": "^22.0.0", | ||
| "@types/better-sqlite3": "^7.6.11", | ||
| "@vercel/ncc": "^0.38.4" | ||
| "tsdown": "^0.12.9" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
curl -s https://registry.npmjs.org/tsdown | jq '.["dist-tags"].latest'Repository: karakeep-app/karakeep
Length of output: 73
🏁 Script executed:
cat -n packages/db/package.jsonRepository: karakeep-app/karakeep
Length of output: 1391
Update tsdown to the latest version.
Current version 0.12.9 is outdated; the latest available version is 0.17.2. Update the dependency to "tsdown": "^0.17.2" to benefit from recent improvements and bug fixes.
🤖 Prompt for AI Agents
In packages/db/package.json around line 33, the tsdown dependency is pinned to
"^0.12.9"; update it to the current recommended version by changing the
dependency entry to "^0.17.2". After updating package.json, run the package
manager (npm/yarn/pnpm) to install and regenerate lockfile, and run the
test/build scripts to verify no breaking changes.
| if (input.actions.length > 0) { | ||
| await tx | ||
| .delete(ruleEngineActionsTable) | ||
| .where(eq(ruleEngineActionsTable.ruleId, input.id)); | ||
| await tx.insert(ruleEngineActionsTable).values( | ||
| input.actions.map((action) => ({ | ||
| ruleId: input.id, | ||
| userId: this.ctx.user.id, | ||
| action: JSON.stringify(action), | ||
| listId: | ||
| action.type === "addToList" || action.type === "removeFromList" | ||
| ? action.listId | ||
| : null, | ||
| tagId: | ||
| action.type === "addTag" || action.type === "removeTag" | ||
| ? action.tagId | ||
| : null, | ||
| })), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Empty actions array leaves orphaned action records.
When input.actions.length > 0, existing actions are deleted and new ones inserted. However, if input.actions.length === 0, the old actions are never deleted. If the intent is to clear actions when an empty array is provided, the delete should happen unconditionally.
- if (input.actions.length > 0) {
- await tx
- .delete(ruleEngineActionsTable)
- .where(eq(ruleEngineActionsTable.ruleId, input.id));
+ await tx
+ .delete(ruleEngineActionsTable)
+ .where(eq(ruleEngineActionsTable.ruleId, input.id));
+ if (input.actions.length > 0) {
await tx.insert(ruleEngineActionsTable).values(🤖 Prompt for AI Agents
In packages/trpc/models/rules.ts around lines 181 to 200, currently the code
only deletes existing action records when input.actions.length > 0, leaving
orphaned actions when an empty array is provided; change the flow to always run
the delete within the transaction (delete by ruleId unconditionally), then only
run the insert when input.actions.length > 0 so that providing an empty actions
array clears existing actions but still avoids unnecessary inserts.
| if (result.rowsAffected == 0) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: | ||
| "Attempting to set link attributes for non-text type bookmark", | ||
| }); | ||
| } | ||
| somethingChanged = true; | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message for text bookmark update.
The error message says "Attempting to set link attributes" but this is for updating text content on a non-text bookmark.
if (result.rowsAffected == 0) {
throw new TRPCError({
code: "BAD_REQUEST",
message:
- "Attempting to set link attributes for non-text type bookmark",
+ "Attempting to set text content for non-text type bookmark",
});
}🤖 Prompt for AI Agents
In packages/trpc/routers/bookmarks.ts around lines 404 to 412, the thrown
TRPCError message incorrectly references "Attempting to set link attributes"
while the check is for updating text content on a non-text bookmark; change the
error message to accurately reflect the condition (for example: "Attempting to
set text content for non-text type bookmark" or similar clear wording) so the
error describes the actual operation that was attempted.
No description provided.