'use' > 'execute' and log sql query #1169
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughDatabase access was migrated from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Database as Database.use
participant CB as Callback (cb)
participant ORM as Drizzle Query
participant Effect as Effect.tryPromise + withSpan
Caller->>Database: use(cb)
activate Database
Database->>CB: invoke with db handle
activate CB
CB->>ORM: build/execute query
note right of CB: cb returns Promise<T>\noptional toSQL() -> Query
CB-->>Database: Promise<T> (& optional toSQL)
deactivate CB
Database->>Database: sql = promise.toSQL?.().sql || "Transaction"
Database->>Effect: tryPromise(promise) with DatabaseError
Effect-->>Caller: Effect<T> (traced span "Database.execute" with { sql })
deactivate Database
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-backend/src/Folders/index.ts (1)
25-72: Implement transactional folder deletion
The currentdeleteFolderissues separatedb.usecalls for selecting children, updating videos/spaceVideos, and deleting folders, without a surrounding transaction. If any step fails, you’ll end up with partial deletes and stale references. Wrap the entire recursive deletion logic in a single transaction (e.g. viadb.transactioninside onedb.usecallback) to guarantee atomicity.
🧹 Nitpick comments (2)
packages/web-backend/src/Folders/FoldersRepo.ts (1)
29-29: Consider renaming the callback parameter to avoid shadowing.The callback parameter
dbshadows the outerDatabaseservice variable from line 19. While this may be intentional, it could reduce clarity. Consider using a distinct name likedrizzle,conn, orclientfor the callback parameter to make the distinction explicit.Example for line 29:
- .use((db) => - db + .use((drizzle) => + drizzleAlso applies to: 44-44, 50-50, 66-66
packages/web-backend/src/Folders/index.ts (1)
31-31: Consider renaming the callback parameter to avoid shadowing.Similar to the changes in
FoldersRepo.ts, the callback parameterdbshadows the outerDatabaseservice variable from line 21. Consider using a distinct name likedrizzle,conn, orclientfor clarity.Example for line 31:
- const children = yield* db.use((db) => - db + const children = yield* db.use((drizzle) => + drizzleAlso applies to: 49-49, 61-61, 69-69, 121-121, 185-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/web-backend/src/Auth.ts(2 hunks)packages/web-backend/src/Database.ts(1 hunks)packages/web-backend/src/Folders/FoldersRepo.ts(3 hunks)packages/web-backend/src/Folders/index.ts(5 hunks)packages/web-backend/src/Organisations/OrganisationsRepo.ts(2 hunks)packages/web-backend/src/S3Buckets/S3BucketsRepo.ts(3 hunks)packages/web-backend/src/Spaces/SpacesRepo.ts(3 hunks)packages/web-backend/src/Spaces/index.ts(2 hunks)packages/web-backend/src/Videos/VideosRepo.ts(3 hunks)packages/web-backend/src/Videos/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
packages/web-backend/src/Spaces/SpacesRepo.tspackages/web-backend/src/Videos/VideosRepo.tspackages/web-backend/src/Videos/index.tspackages/web-backend/src/S3Buckets/S3BucketsRepo.tspackages/web-backend/src/Database.tspackages/web-backend/src/Folders/index.tspackages/web-backend/src/Spaces/index.tspackages/web-backend/src/Folders/FoldersRepo.tspackages/web-backend/src/Auth.tspackages/web-backend/src/Organisations/OrganisationsRepo.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
packages/web-backend/src/Spaces/SpacesRepo.tspackages/web-backend/src/Videos/VideosRepo.tspackages/web-backend/src/Videos/index.tspackages/web-backend/src/S3Buckets/S3BucketsRepo.tspackages/web-backend/src/Database.tspackages/web-backend/src/Folders/index.tspackages/web-backend/src/Spaces/index.tspackages/web-backend/src/Folders/FoldersRepo.tspackages/web-backend/src/Auth.tspackages/web-backend/src/Organisations/OrganisationsRepo.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
packages/web-backend/src/Spaces/SpacesRepo.tspackages/web-backend/src/Videos/VideosRepo.tspackages/web-backend/src/Videos/index.tspackages/web-backend/src/S3Buckets/S3BucketsRepo.tspackages/web-backend/src/Database.tspackages/web-backend/src/Folders/index.tspackages/web-backend/src/Spaces/index.tspackages/web-backend/src/Folders/FoldersRepo.tspackages/web-backend/src/Auth.tspackages/web-backend/src/Organisations/OrganisationsRepo.ts
🧬 Code graph analysis (8)
packages/web-backend/src/Spaces/SpacesRepo.ts (1)
packages/database/index.ts (1)
db(29-34)
packages/web-backend/src/Videos/index.ts (1)
packages/database/index.ts (1)
db(29-34)
packages/web-backend/src/S3Buckets/S3BucketsRepo.ts (1)
packages/database/index.ts (1)
db(29-34)
packages/web-backend/src/Database.ts (2)
packages/database/index.ts (1)
db(29-34)packages/web-domain/src/Database.ts (1)
DatabaseError(3-6)
packages/web-backend/src/Folders/index.ts (1)
packages/database/index.ts (1)
db(29-34)
packages/web-backend/src/Spaces/index.ts (1)
packages/database/index.ts (1)
db(29-34)
packages/web-backend/src/Folders/FoldersRepo.ts (2)
packages/database/index.ts (1)
db(29-34)packages/database/helpers.ts (1)
nanoId(6-9)
packages/web-backend/src/Organisations/OrganisationsRepo.ts (1)
packages/database/index.ts (1)
db(29-34)
⏰ 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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (18)
packages/web-backend/src/Videos/index.ts (1)
111-121: LGTM!The migration from
executetouseis correct, and the query structure remains intact.packages/web-backend/src/Spaces/index.ts (2)
22-33: LGTM!The migration from
executetouseis correct for the space query.
34-49: LGTM!The migration from
executetouseis correct for the organization query.packages/web-backend/src/S3Buckets/S3BucketsRepo.ts (3)
17-23: LGTM!The migration from
executetouseis correct ingetForVideo.
36-41: LGTM!The migration from
executetouseis correct ingetById.
54-59: LGTM!The migration from
executetouseis correct ingetForUser.packages/web-backend/src/Database.ts (1)
9-19: Nice addition of SQL tracing!The
toSQL()extraction enables query logging while gracefully handling transactions (which lacktoSQL()) by defaulting to "Transaction". The type signature correctly uses an intersection type to support both query builders and transactions.packages/web-backend/src/Auth.ts (2)
22-27: LGTM!The migration from
executetouseis correct ingetCurrentUser.
53-62: LGTM!The migration from
executetouseis correct inHttpAuthMiddlewareLive.packages/web-backend/src/Organisations/OrganisationsRepo.ts (2)
16-33: LGTM!The migration from
executetouseis correct inmembershipForVideo.
36-50: LGTM!The migration from
executetouseis correct inmembership.packages/web-backend/src/Videos/VideosRepo.ts (3)
26-28: LGTM!The migration from
executetouseis correct ingetById.
48-57: LGTM!The migration from
executetouseis correct indelete_. The transaction handling is preserved.
63-98: LGTM!The migration from
executetouseis correct increate. The transaction handling is preserved.packages/web-backend/src/Spaces/SpacesRepo.ts (3)
15-30: LGTM!The migration from
executetouseis correct inmembershipForVideo.
37-51: LGTM!The migration from
executetouseis correct inmembership.
55-58: LGTM!The migration from
executetouseis correct ingetById.packages/web-backend/src/Folders/FoldersRepo.ts (1)
29-80: Verify thatDatabase.usemaintains identical error propagation, transaction boundaries, and instrumentation as the removedexecuteAPI. Ensure callbacks run inside a transaction scope, errors map toDatabaseError, and spans still reflect the intended operation.
| try: () => promise, | ||
| catch: (cause) => new DatabaseError({ cause }), | ||
| }), | ||
| }).pipe(Effect.withSpan("Database.execute", { attributes: { sql } })); |
There was a problem hiding this comment.
Update the span name to match the new method name.
The span is still named "Database.execute" but the method has been renamed to use. This inconsistency could cause confusion when reviewing traces.
Apply this diff:
- }).pipe(Effect.withSpan("Database.execute", { attributes: { sql } }));
+ }).pipe(Effect.withSpan("Database.use", { attributes: { sql } }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }).pipe(Effect.withSpan("Database.execute", { attributes: { sql } })); | |
| }).pipe(Effect.withSpan("Database.use", { attributes: { sql } })); |
🤖 Prompt for AI Agents
In packages/web-backend/src/Database.ts around line 18, the tracing span is
named "Database.execute" but the method was renamed to use; update the span name
to "Database.use" so traces match the current method name by changing the string
passed to Effect.withSpan from "Database.execute" to "Database.use".
1 participant
Summary by CodeRabbit