Conversation
|
Warning Rate limit exceeded@MrgSub has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (14)
WalkthroughThis set of changes refactors database access across several server modules, removing direct ORM interactions and context-based database injection in favor of encapsulated methods on a global database instance accessed via Cloudflare Workers' environment. It introduces new helper methods for user, connection, settings, and writing style matrix management, and updates service and router modules to use this new interface. Additionally, some helper functions are now exported, and logging is added to the Google subscription factory. Changes
Sequence Diagram(s)sequenceDiagram
participant Router
participant Env
participant ZeroDB
Router->>Env: Get global-db instance
Env-->>Router: Return ZeroDB instance
Router->>ZeroDB: Call helper method (e.g., updateUser, deleteConnection, etc.)
ZeroDB-->>Router: Perform DB operation and return result
sequenceDiagram
participant Service
participant Env
participant ZeroDB
Service->>Env: Get global-db instance
Env-->>Service: Return ZeroDB instance
Service->>ZeroDB: syncUserMatrix(connectionId, emailStyleMatrix)
ZeroDB-->>Service: Synchronize writing style matrix
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
apps/server/src/trpc/routes/shortcut.ts (1)
15-19: Consider caching the Durable-Object stub
env.ZERO_DB.get(env.ZERO_DB.idFromName('global-db'))is executed on every call path.
While cheap, repeating it in every resolver clutters code and adds avoidable overhead. Inject the stub once (e.g. increateContext) or wrap it in a helper.apps/server/src/trpc/routes/connections.ts (1)
46-50: Deduplicate DB-stub acquisitionSame observation as in
shortcut.ts– the stub retrieval is repeated in each resolver. Extractingconst db = getZeroDb()(utility) or attaching it toctxkeeps resolvers slimmer and centralises the binding logic.apps/server/src/trpc/routes/settings.ts (2)
17-20: Same DB-stub duplication comment as aboveFactor out stub retrieval to keep resolvers terse.
36-45: No error handling around DB writes
insertUserSettings/updateUserSettingsfailures will bubble up as 500s without context.
Wrap intry/catchand return typed tRPC errors so the client can act accordingly.apps/server/src/services/writing-style-service.ts (1)
192-194: Repeated stub acquisition inside retry loopInside
pRetrythe stub is fetched every retry. Minor, but caching the stub once before the retry would avoid unnecessary Durable-Object look-ups.apps/server/src/main.ts (2)
63-67: Consider surfacing the deleted-row count fromdeleteConnectionRight now the result of the
DELETEis discarded, so callers cannot distinguish “nothing deleted” (wrong IDs) from success.
Returning therowCount(or.returning({ id: connection.id })) would improve error handling.
172-175: Docstring warns about “dangerous” usage without explaining whyAdd a short rationale (e.g. “does not validate ownership – may leak data”) so future maintainers understand the risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/server/src/ctx.ts(0 hunks)apps/server/src/lib/factories/google-subscription.factory.ts(1 hunks)apps/server/src/main.ts(6 hunks)apps/server/src/services/writing-style-service.ts(2 hunks)apps/server/src/trpc/index.ts(0 hunks)apps/server/src/trpc/routes/connections.ts(1 hunks)apps/server/src/trpc/routes/settings.ts(3 hunks)apps/server/src/trpc/routes/shortcut.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/server/src/ctx.ts
- apps/server/src/trpc/index.ts
🔇 Additional comments (4)
apps/server/src/trpc/routes/connections.ts (1)
56-62: Race condition after deletion
activeConnectionis fetched afterawait db.deleteConnection(...).
IfgetActiveConnection()still relies on that deleted row (e.g. through caching), it may resolveundefined, making the subsequentupdateUsercheck always false.
Verify that behaviour is acceptable or fetchactiveConnectionbefore deletion.apps/server/src/trpc/routes/settings.ts (1)
26-26:ctx.cmay be undefined in tRPC context
ctx.c.executionCtx.waitUntil(...)assumes thatc(the original Hono context) was forwarded into the tRPC context under theckey.
If that contract changes, this line throws at runtime. Guard or surface a clearer error.apps/server/src/main.ts (2)
1-6: No issues detected with the new imports.Everything compiles and typings look correct.
98-102: Potential missing index onuserHotkeys.userId
findUserHotkeyswill be executed frequently and filters solely onuserId.
Verify thatuserIdis either the PK or has a dedicated index; otherwise look-ups will degrade as the table grows.
| console.log( | ||
| `[SUBSCRIPTION] Setting up Gmail watch for connection: ${connectionData.id} ${topicName} projects/${serviceAccount.project_id}/topics/${topicName}`, | ||
| ); | ||
| console.log(`[SUBSCRIPTION] Service Account: ${serviceAccount.client_email}`, serviceAccount); | ||
|
|
There was a problem hiding this comment.
Remove sensitive logging of the whole service-account object
serviceAccount.private_key (and other secrets) ends up in the worker logs, which are readable by anyone with log access. This is a direct leakage of credentials and violates least-privilege & PCI/GDPR guidance.
Keep the contextual info but mask / drop secrets.
- console.log(
- `[SUBSCRIPTION] Setting up Gmail watch for connection: ${connectionData.id} ${topicName} projects/${serviceAccount.project_id}/topics/${topicName}`,
- );
- console.log(`[SUBSCRIPTION] Service Account: ${serviceAccount.client_email}`, serviceAccount);
+ console.log(
+ `[SUBSCRIPTION] Setting up Gmail watch for connection ${connectionData.id} on topic ${topicName}`,
+ );
+ // Only log non-sensitive identifiers
+ console.log('[SUBSCRIPTION] Service Account:', {
+ client_email: serviceAccount.client_email,
+ project_id: serviceAccount.project_id,
+ });📝 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.
| console.log( | |
| `[SUBSCRIPTION] Setting up Gmail watch for connection: ${connectionData.id} ${topicName} projects/${serviceAccount.project_id}/topics/${topicName}`, | |
| ); | |
| console.log(`[SUBSCRIPTION] Service Account: ${serviceAccount.client_email}`, serviceAccount); | |
| console.log( | |
| `[SUBSCRIPTION] Setting up Gmail watch for connection ${connectionData.id} on topic ${topicName}`, | |
| ); | |
| // Only log non-sensitive identifiers | |
| console.log('[SUBSCRIPTION] Service Account:', { | |
| client_email: serviceAccount.client_email, | |
| project_id: serviceAccount.project_id, | |
| }); |
🤖 Prompt for AI Agents
In apps/server/src/lib/factories/google-subscription.factory.ts around lines 226
to 230, the code logs the entire serviceAccount object including sensitive
fields like private_key, which exposes credentials in logs. Modify the logging
to exclude or mask sensitive fields by only logging non-sensitive properties
such as client_email or project_id, and avoid printing the full serviceAccount
object to prevent credential leakage.
| async updateUserSettings(userId: string, settings: typeof defaultUserSettings) { | ||
| return await this.db | ||
| .update(userSettings) | ||
| .set({ | ||
| settings, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(userSettings.userId, userId)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updateUserSettings silently no-ops when the row is absent
If a new user calls this method before insertUserSettings, nothing is written.
Either:
- Change this into an upsert (
.onConflictDoUpdate) - Or fall back to
insertUserSettingswhenrowCount === 0.
This prevents hidden data loss.
🤖 Prompt for AI Agents
In apps/server/src/main.ts around lines 132 to 140, the updateUserSettings
method does not handle the case where the user settings row does not exist,
causing silent no-ops. To fix this, modify the method to perform an upsert using
.onConflictDoUpdate to insert or update the settings atomically, or
alternatively check the update result's rowCount and call insertUserSettings if
no rows were updated, ensuring data is not lost silently.
| async insertUserHotkeys(userId: string, shortcuts: (typeof userHotkeys.$inferInsert)[]) { | ||
| return await this.db | ||
| .insert(userHotkeys) | ||
| .values({ | ||
| userId, | ||
| shortcuts, | ||
| createdAt: new Date(), | ||
| updatedAt: new Date(), | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: userHotkeys.userId, | ||
| set: { | ||
| shortcuts, | ||
| updatedAt: new Date(), | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Upsert relies on userId being UNIQUE — is that constraint guaranteed?
onConflictDoUpdate({ target: userHotkeys.userId, … }) assumes a unique (or primary-key) constraint on userId.
If the table allows multiple rows per user the upsert will error at runtime.
Please confirm the schema or switch to a compound key (e.g. userId, shortcutId).
🤖 Prompt for AI Agents
In apps/server/src/main.ts around lines 104 to 120, the upsert operation uses
onConflictDoUpdate with userHotkeys.userId as the target, which requires userId
to have a unique or primary key constraint. Verify the database schema to ensure
userId is unique; if not, modify the conflict target to use a compound key such
as userId and shortcutId to avoid runtime errors during upsert.
| async updateUser(userId: string, data: Partial<typeof user.$inferInsert>) { | ||
| return await this.db.update(user).set(data).where(eq(user.id, userId)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updateUser should also bump updatedAt and optionally return the mutated row
Updating a user without touching the updatedAt column makes auditing harder and breaks the implicit contract most tables have.
Drizzle also lets you append .returning() if callers need the fresh row.
- return await this.db.update(user).set(data).where(eq(user.id, userId));
+ return await this.db
+ .update(user)
+ .set({ ...data, updatedAt: new Date() })
+ .where(eq(user.id, userId))
+ .returning(); # if callers need the updated row📝 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.
| async updateUser(userId: string, data: Partial<typeof user.$inferInsert>) { | |
| return await this.db.update(user).set(data).where(eq(user.id, userId)); | |
| } | |
| async updateUser(userId: string, data: Partial<typeof user.$inferInsert>) { | |
| return await this.db | |
| .update(user) | |
| .set({ ...data, updatedAt: new Date() }) | |
| .where(eq(user.id, userId)) | |
| .returning(); # if callers need the updated row | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/main.ts around lines 59 to 61, the updateUser method
currently updates user data without modifying the updatedAt timestamp and does
not return the updated row. Modify the updateUser method to also set the
updatedAt field to the current timestamp during the update and append a
.returning() call to return the updated row to the caller.
| async syncUserMatrix(connectionId: string, emailStyleMatrix: EmailMatrix) { | ||
| await this.db.transaction(async (tx) => { | ||
| const [existingMatrix] = await tx | ||
| .select({ | ||
| numMessages: writingStyleMatrix.numMessages, | ||
| style: writingStyleMatrix.style, | ||
| }) | ||
| .from(writingStyleMatrix) | ||
| .where(eq(writingStyleMatrix.connectionId, connectionId)) | ||
| .for('update'); | ||
|
|
||
| if (!existingMatrix) { | ||
| const newStyle = initializeStyleMatrixFromEmail(emailStyleMatrix); | ||
|
|
||
| await tx.insert(writingStyleMatrix).values({ | ||
| connectionId, | ||
| numMessages: 1, | ||
| style: newStyle, | ||
| }); | ||
| } else { | ||
| const newStyle = createUpdatedMatrixFromNewEmail( | ||
| existingMatrix.numMessages, | ||
| existingMatrix.style as WritingStyleMatrix, | ||
| emailStyleMatrix, | ||
| ); | ||
|
|
||
| await tx | ||
| .update(writingStyleMatrix) | ||
| .set({ | ||
| numMessages: existingMatrix.numMessages + 1, | ||
| style: newStyle, | ||
| }) | ||
| .where(eq(writingStyleMatrix.connectionId, connectionId)); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Race condition: two newcomers can still create duplicate matrix rows
Although the code locks the existing row with for update, if two concurrent transactions both find no row they’ll each attempt INSERT, and only one will win. The loser will throw a unique-constraint error.
Safer pattern:
- await tx.insert(writingStyleMatrix).values({ … });
+ await tx
+ .insert(writingStyleMatrix)
+ .values({ … })
+ .onConflictDoUpdate({
+ target: writingStyleMatrix.connectionId,
+ set: {
+ numMessages: 1,
+ style: initializeStyleMatrixFromEmail(emailStyleMatrix),
+ },
+ });This keeps the logic single-path and avoids aborted transactions.
📝 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.
| async syncUserMatrix(connectionId: string, emailStyleMatrix: EmailMatrix) { | |
| await this.db.transaction(async (tx) => { | |
| const [existingMatrix] = await tx | |
| .select({ | |
| numMessages: writingStyleMatrix.numMessages, | |
| style: writingStyleMatrix.style, | |
| }) | |
| .from(writingStyleMatrix) | |
| .where(eq(writingStyleMatrix.connectionId, connectionId)) | |
| .for('update'); | |
| if (!existingMatrix) { | |
| const newStyle = initializeStyleMatrixFromEmail(emailStyleMatrix); | |
| await tx.insert(writingStyleMatrix).values({ | |
| connectionId, | |
| numMessages: 1, | |
| style: newStyle, | |
| }); | |
| } else { | |
| const newStyle = createUpdatedMatrixFromNewEmail( | |
| existingMatrix.numMessages, | |
| existingMatrix.style as WritingStyleMatrix, | |
| emailStyleMatrix, | |
| ); | |
| await tx | |
| .update(writingStyleMatrix) | |
| .set({ | |
| numMessages: existingMatrix.numMessages + 1, | |
| style: newStyle, | |
| }) | |
| .where(eq(writingStyleMatrix.connectionId, connectionId)); | |
| } | |
| }); | |
| } | |
| async syncUserMatrix(connectionId: string, emailStyleMatrix: EmailMatrix) { | |
| await this.db.transaction(async (tx) => { | |
| const [existingMatrix] = await tx | |
| .select({ | |
| numMessages: writingStyleMatrix.numMessages, | |
| style: writingStyleMatrix.style, | |
| }) | |
| .from(writingStyleMatrix) | |
| .where(eq(writingStyleMatrix.connectionId, connectionId)) | |
| .for('update'); | |
| if (!existingMatrix) { | |
| const newStyle = initializeStyleMatrixFromEmail(emailStyleMatrix); | |
| await tx | |
| .insert(writingStyleMatrix) | |
| .values({ | |
| connectionId, | |
| numMessages: 1, | |
| style: newStyle, | |
| }) | |
| .onConflictDoUpdate({ | |
| target: writingStyleMatrix.connectionId, | |
| set: { | |
| numMessages: 1, | |
| style: initializeStyleMatrixFromEmail(emailStyleMatrix), | |
| }, | |
| }); | |
| } else { | |
| const newStyle = createUpdatedMatrixFromNewEmail( | |
| existingMatrix.numMessages, | |
| existingMatrix.style as WritingStyleMatrix, | |
| emailStyleMatrix, | |
| ); | |
| await tx | |
| .update(writingStyleMatrix) | |
| .set({ | |
| numMessages: existingMatrix.numMessages + 1, | |
| style: newStyle, | |
| }) | |
| .where(eq(writingStyleMatrix.connectionId, connectionId)); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/main.ts around lines 184 to 219, the current syncUserMatrix
method can cause a race condition where two concurrent transactions both find no
existing row and attempt to insert, leading to a unique-constraint error. To fix
this, refactor the method to attempt an update first and check the affected rows
count; if no rows were updated, then perform an insert. This single-path logic
avoids the race condition and aborted transactions by not relying on locking and
conditional inserts.
1 participant
READ CAREFULLY THEN REMOVE
Remove bullet points that are not relevant.
PLEASE REFRAIN FROM USING AI TO WRITE YOUR CODE AND PR DESCRIPTION. IF YOU DO USE AI TO WRITE YOUR CODE PLEASE PROVIDE A DESCRIPTION AND REVIEW IT CAREFULLY. MAKE SURE YOU UNDERSTAND THE CODE YOU ARE SUBMITTING USING AI.
Description
Please provide a clear description of your changes.
Type of Change
Please delete options that are not relevant.
Areas Affected
Please check all that apply:
Testing Done
Describe the tests you've done:
Security Considerations
For changes involving data or authentication:
Checklist
Additional Notes
Add any other context about the pull request here.
Screenshots/Recordings
Add screenshots or recordings here if applicable.
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
New Features
Refactor
Style
Documentation
Chores