Conversation
🦋 Changeset detectedLatest commit: f520abb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an asynchronous BullMQ queue and worker for Alchemy webhook subscriber updates, routes webhook subscription work through the queue from credential creation, introduces WebhookNotReadyError with 503 handling in auth routes, expands tests and mocks, and integrates worker lifecycle into server startup/shutdown. Changes
Sequence DiagramsequenceDiagram
actor User
participant Auth as Auth Handler
participant Cred as createCredential
participant Queue as Alchemy Queue
participant Worker as Queue Worker
participant Alchemy as Alchemy API
User->>Auth: register/login request
Auth->>Cred: createCredential()
alt webhookId available
Cred->>Queue: enqueue ADD_SUBSCRIBER (account, webhookId)
Queue-->>Cred: job queued
Cred-->>Auth: success
Auth-->>User: 200 OK
Note over Queue,Worker: background processing
Queue->>Worker: process ADD_SUBSCRIBER
Worker->>Alchemy: PATCH /addresses (headers + payload)
alt Alchemy 2xx
Alchemy-->>Worker: 200 OK
Worker-->>Queue: job completed
else non-2xx
Alchemy-->>Worker: error
Worker-->>Sentry: log error
Worker-->>Queue: job failed
end
else webhookId missing
Cred-->>Cred: throw WebhookNotReadyError
Auth->>Auth: catch WebhookNotReadyError
Auth-->>User: 503 Service Unavailable (retriable)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested Reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @aguxez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust queueing system for handling Alchemy webhook subscription updates. By integrating BullMQ, the application now processes these updates asynchronously, moving the task from the main request-response cycle to a background worker. This change enhances the system's reliability and performance by preventing blocking operations and providing built-in retry capabilities for external API calls, particularly when a new user credential is created. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
2 times, most recently
from
32c8a33 to
0dc7771
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
0dc7771 to
63b777f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #619 +/- ##
==========================================
+ Coverage 71.13% 71.24% +0.11%
==========================================
Files 211 211
Lines 8349 8427 +78
Branches 2727 2750 +23
==========================================
+ Hits 5939 6004 +65
- Misses 2132 2140 +8
- Partials 278 283 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@cursor review |
PR SummaryCursor Bugbot is generating a summary for commit 63b777f. Configure here. |
PR SummaryIntroduces background processing for Alchemy webhook address updates and wires it into credential creation.
Written by Cursor Bugbot for commit 63b777f. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
63b777f to
03cbfe4
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
03cbfe4 to
54b79be
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @server/queues/alchemyQueue.ts:
- Line 9: The file directly reads process.env.DISABLE_WORKERS into the const
disableWorkers which violates the rule to not access process.env in application
code; change the module to accept configuration via dependency injection (e.g.,
export a factory like createAlchemyQueue(config) or accept a Config parameter)
and replace direct usage of disableWorkers with config.disableWorkers (or
equivalent) throughout the file so configuration is loaded once at startup and
passed in rather than read from process.env inside this module.
- Around line 21-26: The code directly reads process.env to build the Redis
`connection` object and uses Number() for port parsing; change this by
introducing a factory function (e.g., export function
createRedisConnection(redisConfig: { url?: string; host?: string; port?: string
| number })) that takes the preloaded config instead of accessing process.env
inside this module, and inside that function build the connection: if
redisConfig.url use new Redis(redisConfig.url, { maxRetriesPerRequest: null }),
otherwise use { host: redisConfig.host ?? "localhost", port:
Number.parseInt(String(redisConfig.port ?? "6379"), 10) }; update call sites to
pass the startup-loaded config into createRedisConnection instead of relying on
environment access here.
- Around line 41-68: The span.setStatus calls in processor (inside the
AlchemyJob.ADD_SUBSCRIBER case and the default case) use the magic number 2;
replace that literal with a named constant or the Sentry enum (e.g., import
SpanStatusCode from "@sentry/node" and use SpanStatusCode.Error) or define a
local constant like SPAN_STATUS_ERROR and use it in both span.setStatus calls to
improve readability and consistency.
- Around line 72-100: The Worker instance created for QueueName.ALCHEMY is not
stored, preventing graceful shutdown; assign it to a module-scoped variable
(e.g., let alchemyWorker: Worker | undefined = new Worker(...) or create it then
set alchemyWorker = worker) so you can call alchemyWorker?.close() during
application termination/shutdown handling; ensure the variable is exported or
accessible by your existing shutdown routine and keep existing event handlers
(.on("failed"/"completed"/"active"/"error")) attached to the stored worker.
In @server/test/queues/alchemyQueue.test.ts:
- Line 54: The test assertion includes an unnecessary type assertion on the
headers matcher; remove the trailing "as Record<string, string>" from the
headers line so the matcher uses expect.objectContaining({ "X-Alchemy-Token":
"mock-token" }) directly (update the headers property in the assertion where
expect.objectContaining is used).
In @server/test/utils/createCredential.test.ts:
- Line 67: Remove the redundant type assertion on the account matcher: locate
the object assigning account: expect.stringMatching(/^0x/) as string and simply
use account: expect.stringMatching(/^0x/); the expect.stringMatching call
already returns the correct matcher type so drop the "as string" cast to clean
up the test.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.changeset/short-cars-return.mdserver/queues/alchemyQueue.tsserver/queues/constants.tsserver/script/openapi.tsserver/test/queues/alchemyQueue.test.tsserver/test/utils/createCredential.test.tsserver/utils/createCredential.ts
🧰 Additional context used
📓 Path-based instructions (7)
server/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/server.mdc)
server/**/*.ts: Usec.varobject to pass strongly-typed data between Hono middleware and route handlers; do not usec.set
All request validation (headers, body, params) must be handled by@hono/valibot-validatormiddleware; do not perform manual validation inside route handlers
Use Hono's built-in error handling by throwingnew HTTPException()for expected errors; unhandled errors will be caught and logged automatically
Enforce Node.js best practices using ESLintplugin:n/recommendedconfiguration
Enforce Drizzle ORM best practices using ESLintplugin:drizzle/allconfiguration, including requiringwhereclauses forupdateanddeleteoperations
Use Drizzle ORM query builder for all database interactions; do not write raw SQL queries unless absolutely unavoidable
All authentication and authorization logic must be implemented in Hono middleware
Do not accessprocess.envdirectly in application code; load all configuration and secrets once at startup and pass them through dependency injection or context
Avoid long-running, synchronous operations; useasync/awaitcorrectly and be mindful of CPU-intensive tasks to prevent blocking the event loop
Files:
server/test/utils/createCredential.test.tsserver/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.ts
**/*.{js,ts,tsx,jsx,sol}
📄 CodeRabbit inference engine (AGENTS.md)
Follow linter/formatter (eslint, prettier, solhint) strictly with high strictness level. No
anytype.
Files:
server/test/utils/createCredential.test.tsserver/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Omit redundant type names in variable declarations - let the type system explain itself
**/*.{ts,tsx}: Use PascalCase for TypeScript types and interfaces
Use valibot for all runtime validation of API inputs, environment variables, and other data; define schemas once and reuse them
Infer TypeScript types from valibot schemas usingtype User = v.Input<typeof UserSchema>instead of manually defining interfaces
Files:
server/test/utils/createCredential.test.tsserver/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Omit contextual names - don't repeat class/module names in members
Omit meaningless words like 'data', 'state', 'manager', 'engine', 'value' from variable and function names unless they add disambiguation
**/*.{ts,tsx,js,jsx}: Prefer function declarations for all multi-line functions; use function expressions or arrow functions only for single-line implementations
Preferconstfor all variable declarations by default; only useletif the variable's value will be reassigned
Declare each variable on its own line with its ownconstorletkeyword, not multiple declarations on one line
Use camelCase for TypeScript variables and functions
Always useimport type { ... }for type imports
Use relative paths for all imports within the project; avoid tsconfig path aliases
Follow eslint-plugin-import order: react, external libraries, then relative paths
Use object and array destructuring to access and use properties
Use object method shorthand syntax when a function is a property of an object
Prefer optional chaining (?.), nullish coalescing (??), object and array spreading (...), andfor...ofloops over traditional syntax
Do not use abbreviations or cryptic names; write out full words likeerror,parameters,requestinstead oferr,params,req
UseNumber.parseInt()instead of the globalparseInt()function when parsing numbers
All classes called withnewmust use PascalCase
UseBuffer.from(),Buffer.alloc(), orBuffer.allocUnsafe()instead of the deprecatednew Buffer()
Use@ts-expect-errorinstead of@ts-ignore; follow it immediately with a single-line lowercase comment explaining why the error is expected, without separators like-or:
Do not include the type in a variable's name; let the static type system do its job (e.g., useconst user: Usernotconst userObject: User)
Do not repeat the name of a class or module within its members; omit contextual names (e.g., use `class User { getProfil...
Files:
server/test/utils/createCredential.test.tsserver/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.ts
server/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
server/**/*.{ts,tsx}: Server API: implement schema-first approach using OpenAPI via hono with validation via valibot middleware
Server database: drizzle schema is source of truth. Migrations required. No direct database access in handlers - usec.var.db
Files:
server/test/utils/createCredential.test.tsserver/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
For files with a single
defaultexport, name the file identically to the export; for files with multiple exports, use camelCase with a strong preference for a single word
Files:
server/test/utils/createCredential.test.tsserver/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.ts
**/.changeset/*.md
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Use a lowercase sentence in the imperative present tense for changeset summaries
Files:
.changeset/short-cars-return.md
🧠 Learnings (6)
📚 Learning: 2025-12-23T19:58:16.574Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T19:58:16.574Z
Learning: Zero config local dev environment: no `.env` files, mock all external services
Applied to files:
server/script/openapi.ts
📚 Learning: 2025-12-23T19:58:16.574Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T19:58:16.574Z
Learning: Server stack: use hono, node.js, drizzle orm, postgres
Applied to files:
server/script/openapi.ts
📚 Learning: 2025-12-23T19:57:35.503Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/server.mdc:0-0
Timestamp: 2025-12-23T19:57:35.503Z
Learning: Applies to server/**/*.ts : Do not access `process.env` directly in application code; load all configuration and secrets once at startup and pass them through dependency injection or context
Applied to files:
server/script/openapi.tsserver/queues/alchemyQueue.ts
📚 Learning: 2025-12-31T00:23:55.034Z
Learnt from: cruzdanilo
Repo: exactly/exa PR: 610
File: .changeset/ready-experts-fly.md:1-2
Timestamp: 2025-12-31T00:23:55.034Z
Learning: In the exactly/exa repository, allow and require empty changeset files (containing only --- separators) when changes are not user-facing and do not warrant a version bump. This is needed because CI runs changeset status --since origin/main and requires a changeset file to exist. Ensure such empty changesets are used only for non-user-facing changes and document the rationale in the commit or changelog notes.
Applied to files:
.changeset/short-cars-return.md
📚 Learning: 2025-12-23T19:57:35.503Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/server.mdc:0-0
Timestamp: 2025-12-23T19:57:35.503Z
Learning: Applies to server/**/*.ts : Avoid long-running, synchronous operations; use `async/await` correctly and be mindful of CPU-intensive tasks to prevent blocking the event loop
Applied to files:
server/queues/alchemyQueue.ts
📚 Learning: 2025-12-30T15:03:28.449Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/style.mdc:0-0
Timestamp: 2025-12-30T15:03:28.449Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `ts-expect-error` instead of `ts-ignore`; follow it immediately with a single-line lowercase comment explaining why the error is expected, without separators like `-` or `:`
Applied to files:
server/utils/createCredential.ts
🧬 Code graph analysis (3)
server/test/queues/alchemyQueue.test.ts (1)
server/queues/alchemyQueue.ts (2)
AlchemyJobData(14-19)processor(41-68)
server/queues/alchemyQueue.ts (1)
server/queues/constants.ts (2)
QueueName(1-3)AlchemyJob(7-9)
server/utils/createCredential.ts (2)
server/queues/alchemyQueue.ts (1)
alchemyQueue(33-33)server/queues/constants.ts (1)
AlchemyJob(7-9)
🔇 Additional comments (12)
server/queues/constants.ts (1)
1-11: LGTM! Clean constants definition.The use of
as constwith derived enum types is idiomatic TypeScript, providing strong type safety for queue and job names.server/script/openapi.ts (1)
25-26: LGTM! Appropriate environment configuration for OpenAPI generation.The explicit Redis URL and
DISABLE_WORKERSflag correctly prevent worker initialization during spec generation while maintaining the zero-config local dev environment..changeset/short-cars-return.md (1)
1-5: LGTM! Changeset follows the required format.The summary correctly uses lowercase imperative present tense as per coding guidelines.
server/test/utils/createCredential.test.ts (1)
1-58: LGTM! Comprehensive test coverage with proper mocking.The test suite correctly validates both the happy path (job enqueuing) and error handling (exception capture), with well-isolated mocks.
Also applies to: 72-82
server/test/queues/alchemyQueue.test.ts (1)
1-53: LGTM! Comprehensive test coverage for queue processor.The test suite validates all critical paths: successful webhook updates, unknown job handling, and API failure scenarios. The tests correctly expect the processor to throw errors for edge cases.
Also applies to: 55-90
server/utils/createCredential.ts (4)
6-7: LGTM!The Sentry import changes appropriately separate
setUserfrom@sentry/coreandcaptureExceptionfrom@sentry/node, following the modular structure of the Sentry SDK.
14-14: LGTM!The database import addition is necessary for the database operations and follows the project's import conventions.
21-22: LGTM!The queue-related imports are correctly added to support the new asynchronous job enqueueing approach, replacing the previous synchronous HTTP call.
54-54: Consider consistent error capture options.The
captureExceptioncall at line 54 doesn't include aleveloption, while the similar sardine error capture at line 56 specifies{ level: "error" }. For consistency, consider adding the same error level to both non-critical async operations.🔎 Suggested consistency improvement
- alchemyQueue.add(AlchemyJob.ADD_SUBSCRIBER, { account, webhookId }).catch(captureException), + alchemyQueue.add(AlchemyJob.ADD_SUBSCRIBER, { account, webhookId }).catch((error: unknown) => + captureException(error, { level: "error" }), + ),Likely an incorrect or invalid review comment.
server/queues/alchemyQueue.ts (3)
1-6: LGTM!The imports are correctly structured with proper type imports and necessary dependencies for the queue implementation.
11-19: LGTM!The
AlchemyJobDatainterface is well-documented with clear JSDoc comments explaining each field's purpose.
28-33: LGTM!The queue is correctly instantiated with proper documentation explaining its purpose for offloading webhook subscription updates and enabling retries.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
54b79be to
61cb4bb
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @server/test/queues/alchemyQueue.test.ts:
- Line 54: The test contains an unnecessary type assertion on the headers
matcher: remove the trailing "as Record<string, string>" from the
expect.objectContaining call (the matcher returned by expect.objectContaining({
"X-Alchemy-Token": "mock-token" }) is already correctly typed); update the line
using the headers matcher (the expression starting with
expect.objectContaining({ "X-Alchemy-Token": "mock-token" })) to omit the cast
and run tests to confirm no type errors remain.
In @server/test/utils/createCredential.test.ts:
- Line 67: Remove the redundant type assertion on the account matcher: replace
the property value "expect.stringMatching(/^0x/) as string" with just
"expect.stringMatching(/^0x/)" in the createCredential.test.ts test (the object
property named account), since expect.stringMatching already returns the correct
matcher type.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.changeset/short-cars-return.mdserver/queues/alchemyQueue.tsserver/queues/constants.tsserver/script/openapi.tsserver/test/queues/alchemyQueue.test.tsserver/test/utils/createCredential.test.tsserver/utils/createCredential.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/.changeset/*.md
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Use a lowercase sentence in the imperative present tense for changeset summaries
Files:
.changeset/short-cars-return.md
server/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/server.mdc)
server/**/*.ts: Usec.varobject to pass strongly-typed data between Hono middleware and route handlers; do not usec.set
All request validation (headers, body, params) must be handled by@hono/valibot-validatormiddleware; do not perform manual validation inside route handlers
Use Hono's built-in error handling by throwingnew HTTPException()for expected errors; unhandled errors will be caught and logged automatically
Enforce Node.js best practices using ESLintplugin:n/recommendedconfiguration
Enforce Drizzle ORM best practices using ESLintplugin:drizzle/allconfiguration, including requiringwhereclauses forupdateanddeleteoperations
Use Drizzle ORM query builder for all database interactions; do not write raw SQL queries unless absolutely unavoidable
All authentication and authorization logic must be implemented in Hono middleware
Do not accessprocess.envdirectly in application code; load all configuration and secrets once at startup and pass them through dependency injection or context
Avoid long-running, synchronous operations; useasync/awaitcorrectly and be mindful of CPU-intensive tasks to prevent blocking the event loop
Files:
server/queues/alchemyQueue.tsserver/script/openapi.tsserver/utils/createCredential.tsserver/test/queues/alchemyQueue.test.tsserver/queues/constants.tsserver/test/utils/createCredential.test.ts
**/*.{js,ts,tsx,jsx,sol}
📄 CodeRabbit inference engine (AGENTS.md)
Follow linter/formatter (eslint, prettier, solhint) strictly with high strictness level. No
anytype.
Files:
server/queues/alchemyQueue.tsserver/script/openapi.tsserver/utils/createCredential.tsserver/test/queues/alchemyQueue.test.tsserver/queues/constants.tsserver/test/utils/createCredential.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Omit redundant type names in variable declarations - let the type system explain itself
**/*.{ts,tsx}: Use PascalCase for TypeScript types and interfaces
Use valibot for all runtime validation of API inputs, environment variables, and other data; define schemas once and reuse them
Infer TypeScript types from valibot schemas usingtype User = v.Input<typeof UserSchema>instead of manually defining interfaces
Files:
server/queues/alchemyQueue.tsserver/script/openapi.tsserver/utils/createCredential.tsserver/test/queues/alchemyQueue.test.tsserver/queues/constants.tsserver/test/utils/createCredential.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Omit contextual names - don't repeat class/module names in members
Omit meaningless words like 'data', 'state', 'manager', 'engine', 'value' from variable and function names unless they add disambiguation
**/*.{ts,tsx,js,jsx}: Prefer function declarations for all multi-line functions; use function expressions or arrow functions only for single-line implementations
Preferconstfor all variable declarations by default; only useletif the variable's value will be reassigned
Declare each variable on its own line with its ownconstorletkeyword, not multiple declarations on one line
Use camelCase for TypeScript variables and functions
Always useimport type { ... }for type imports
Use relative paths for all imports within the project; avoid tsconfig path aliases
Follow eslint-plugin-import order: react, external libraries, then relative paths
Use object and array destructuring to access and use properties
Use object method shorthand syntax when a function is a property of an object
Prefer optional chaining (?.), nullish coalescing (??), object and array spreading (...), andfor...ofloops over traditional syntax
Do not use abbreviations or cryptic names; write out full words likeerror,parameters,requestinstead oferr,params,req
UseNumber.parseInt()instead of the globalparseInt()function when parsing numbers
All classes called withnewmust use PascalCase
UseBuffer.from(),Buffer.alloc(), orBuffer.allocUnsafe()instead of the deprecatednew Buffer()
Use@ts-expect-errorinstead of@ts-ignore; follow it immediately with a single-line lowercase comment explaining why the error is expected, without separators like-or:
Do not include the type in a variable's name; let the static type system do its job (e.g., useconst user: Usernotconst userObject: User)
Do not repeat the name of a class or module within its members; omit contextual names (e.g., use `class User { getProfil...
Files:
server/queues/alchemyQueue.tsserver/script/openapi.tsserver/utils/createCredential.tsserver/test/queues/alchemyQueue.test.tsserver/queues/constants.tsserver/test/utils/createCredential.test.ts
server/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
server/**/*.{ts,tsx}: Server API: implement schema-first approach using OpenAPI via hono with validation via valibot middleware
Server database: drizzle schema is source of truth. Migrations required. No direct database access in handlers - usec.var.db
Files:
server/queues/alchemyQueue.tsserver/script/openapi.tsserver/utils/createCredential.tsserver/test/queues/alchemyQueue.test.tsserver/queues/constants.tsserver/test/utils/createCredential.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
For files with a single
defaultexport, name the file identically to the export; for files with multiple exports, use camelCase with a strong preference for a single word
Files:
server/queues/alchemyQueue.tsserver/script/openapi.tsserver/utils/createCredential.tsserver/test/queues/alchemyQueue.test.tsserver/queues/constants.tsserver/test/utils/createCredential.test.ts
🧠 Learnings (6)
📚 Learning: 2025-12-31T00:23:55.034Z
Learnt from: cruzdanilo
Repo: exactly/exa PR: 610
File: .changeset/ready-experts-fly.md:1-2
Timestamp: 2025-12-31T00:23:55.034Z
Learning: In the exactly/exa repository, allow and require empty changeset files (containing only --- separators) when changes are not user-facing and do not warrant a version bump. This is needed because CI runs changeset status --since origin/main and requires a changeset file to exist. Ensure such empty changesets are used only for non-user-facing changes and document the rationale in the commit or changelog notes.
Applied to files:
.changeset/short-cars-return.md
📚 Learning: 2025-12-23T19:57:35.503Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/server.mdc:0-0
Timestamp: 2025-12-23T19:57:35.503Z
Learning: Applies to server/**/*.ts : Do not access `process.env` directly in application code; load all configuration and secrets once at startup and pass them through dependency injection or context
Applied to files:
server/queues/alchemyQueue.tsserver/script/openapi.ts
📚 Learning: 2025-12-23T19:57:35.503Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/server.mdc:0-0
Timestamp: 2025-12-23T19:57:35.503Z
Learning: Applies to server/**/*.ts : Avoid long-running, synchronous operations; use `async/await` correctly and be mindful of CPU-intensive tasks to prevent blocking the event loop
Applied to files:
server/queues/alchemyQueue.ts
📚 Learning: 2025-12-23T19:58:16.574Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T19:58:16.574Z
Learning: Zero config local dev environment: no `.env` files, mock all external services
Applied to files:
server/script/openapi.ts
📚 Learning: 2025-12-23T19:58:16.574Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T19:58:16.574Z
Learning: Server stack: use hono, node.js, drizzle orm, postgres
Applied to files:
server/script/openapi.ts
📚 Learning: 2025-12-30T15:03:28.449Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/style.mdc:0-0
Timestamp: 2025-12-30T15:03:28.449Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `ts-expect-error` instead of `ts-ignore`; follow it immediately with a single-line lowercase comment explaining why the error is expected, without separators like `-` or `:`
Applied to files:
server/utils/createCredential.ts
🧬 Code graph analysis (4)
server/queues/alchemyQueue.ts (1)
server/queues/constants.ts (2)
QueueName(1-3)AlchemyJob(7-9)
server/utils/createCredential.ts (2)
server/queues/alchemyQueue.ts (1)
alchemyQueue(33-33)server/queues/constants.ts (1)
AlchemyJob(7-9)
server/test/queues/alchemyQueue.test.ts (3)
.maestro/maestro.d.ts (1)
Response(19-19)server/queues/constants.ts (1)
AlchemyJob(7-9)server/queues/alchemyQueue.ts (2)
AlchemyJobData(14-19)processor(41-68)
server/test/utils/createCredential.test.ts (3)
server/utils/createCredential.ts (1)
createCredential(24-61)server/queues/alchemyQueue.ts (1)
alchemyQueue(33-33)server/queues/constants.ts (1)
AlchemyJob(7-9)
🔇 Additional comments (9)
server/script/openapi.ts (1)
25-26: LGTM! Environment configuration aligns with new queue infrastructure.The Redis URL now provides a proper connection string for local development, and the DISABLE_WORKERS flag enables control over worker initialization during spec generation.
.changeset/short-cars-return.md (1)
1-5: LGTM! Changeset follows guidelines.The description uses lowercase imperative present tense as required by the coding guidelines.
server/test/utils/createCredential.test.ts (1)
1-82: LGTM! Test coverage is comprehensive.The tests appropriately cover both the success path (job enqueuing) and error path (exception capture), with thorough mocking of dependencies.
server/queues/constants.ts (1)
1-11: LGTM! Clean constants definition.The constants follow TypeScript best practices with
as constassertions and proper type derivation. Naming conventions are correct throughout.server/test/queues/alchemyQueue.test.ts (1)
1-90: LGTM! Comprehensive test coverage for the queue processor.The tests cover all critical paths: successful processing, unknown job handling, and API failure scenarios. The mocking strategy is thorough and appropriate.
server/utils/createCredential.ts (1)
46-59: Queue-based approach for webhook subscription looks good.The switch from synchronous HTTP PATCH to asynchronous queue processing improves reliability by enabling retries on API failures and avoids blocking the main request flow. The
Promise.allcorrectly handles the concurrent operations with appropriate error capture.server/queues/alchemyQueue.ts (3)
60-64: Unknown job handling now correctly throws - good fix.The default case properly sets the span status and throws an error, ensuring unknown jobs fail fast rather than silently succeeding. This addresses the previous feedback about silent failures.
98-99: Error handler addition prevents process crashes - good fix.The
.on("error")handler correctly captures Redis connection failures and internal errors, preventing uncaught exceptions from crashing the process. This follows the pattern used elsewhere in the codebase (e.g.,analytics.on("error", ...)insegment.ts).
41-68: Processor implementation is well-structured.The processor correctly:
- Uses Sentry tracing with appropriate span attributes
- Handles the ADD_SUBSCRIBER job type with proper API call
- Throws on non-ok responses with status details
- Throws on unknown job names for fail-fast behavior
aguxez
force-pushed
the
feature/webhook-queue
branch
3 times, most recently
from
f9b7be5 to
5e59af2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.changeset/short-cars-return.md:
- Line 5: Update the changelog summary in .changeset/short-cars-return.md by
removing the redundant "server:" prefix so the summary reads "add queue on user
creation" (i.e., replace "server: add queue on user creation" with "add queue on
user creation").
In @server/test/queues/alchemyQueue.test.ts:
- Line 54: Remove the redundant type assertion by deleting "as Record<string,
string>" from the headers matcher so it reads headers: expect.objectContaining({
"X-Alchemy-Token": "mock-token" }); leave the expect.objectContaining() usage
intact (in the test that sets headers) since Vitest will infer the matcher types
correctly.
In @server/test/utils/createCredential.test.ts:
- Line 67: The test includes a redundant type assertion on the account property:
remove the unnecessary "as string" cast and rely on expect.stringMatching(/^0x/)
directly (i.e., replace "account: expect.stringMatching(/^0x/) as string" with
"account: expect.stringMatching(/^0x/)" in the test using
expect.stringMatching).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.changeset/short-cars-return.mdserver/queues/alchemyQueue.tsserver/queues/constants.tsserver/script/openapi.tsserver/test/queues/alchemyQueue.test.tsserver/test/utils/createCredential.test.tsserver/utils/createCredential.ts
🧰 Additional context used
📓 Path-based instructions (7)
server/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/server.mdc)
server/**/*.ts: Usec.varobject to pass strongly-typed data between Hono middleware and route handlers; do not usec.set
All request validation (headers, body, params) must be handled by@hono/valibot-validatormiddleware; do not perform manual validation inside route handlers
Use Hono's built-in error handling by throwingnew HTTPException()for expected errors; unhandled errors will be caught and logged automatically
Enforce Node.js best practices using ESLintplugin:n/recommendedconfiguration
Enforce Drizzle ORM best practices using ESLintplugin:drizzle/allconfiguration, including requiringwhereclauses forupdateanddeleteoperations
Use Drizzle ORM query builder for all database interactions; do not write raw SQL queries unless absolutely unavoidable
All authentication and authorization logic must be implemented in Hono middleware
Do not accessprocess.envdirectly in application code; load all configuration and secrets once at startup and pass them through dependency injection or context
Avoid long-running, synchronous operations; useasync/awaitcorrectly and be mindful of CPU-intensive tasks to prevent blocking the event loop
Files:
server/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.tsserver/test/utils/createCredential.test.ts
**/*.{js,ts,tsx,jsx,sol}
📄 CodeRabbit inference engine (AGENTS.md)
Follow linter/formatter (eslint, prettier, solhint) strictly with high strictness level. No
anytype.
Files:
server/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.tsserver/test/utils/createCredential.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Omit redundant type names in variable declarations - let the type system explain itself
**/*.{ts,tsx}: Use PascalCase for TypeScript types and interfaces
Use valibot for all runtime validation of API inputs, environment variables, and other data; define schemas once and reuse them
Infer TypeScript types from valibot schemas usingtype User = v.Input<typeof UserSchema>instead of manually defining interfaces
Files:
server/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.tsserver/test/utils/createCredential.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Omit contextual names - don't repeat class/module names in members
Omit meaningless words like 'data', 'state', 'manager', 'engine', 'value' from variable and function names unless they add disambiguation
**/*.{ts,tsx,js,jsx}: Prefer function declarations for all multi-line functions; use function expressions or arrow functions only for single-line implementations
Preferconstfor all variable declarations by default; only useletif the variable's value will be reassigned
Declare each variable on its own line with its ownconstorletkeyword, not multiple declarations on one line
Use camelCase for TypeScript variables and functions
Always useimport type { ... }for type imports
Use relative paths for all imports within the project; avoid tsconfig path aliases
Follow eslint-plugin-import order: react, external libraries, then relative paths
Use object and array destructuring to access and use properties
Use object method shorthand syntax when a function is a property of an object
Prefer optional chaining (?.), nullish coalescing (??), object and array spreading (...), andfor...ofloops over traditional syntax
Do not use abbreviations or cryptic names; write out full words likeerror,parameters,requestinstead oferr,params,req
UseNumber.parseInt()instead of the globalparseInt()function when parsing numbers
All classes called withnewmust use PascalCase
UseBuffer.from(),Buffer.alloc(), orBuffer.allocUnsafe()instead of the deprecatednew Buffer()
Use@ts-expect-errorinstead of@ts-ignore; follow it immediately with a single-line lowercase comment explaining why the error is expected, without separators like-or:
Do not include the type in a variable's name; let the static type system do its job (e.g., useconst user: Usernotconst userObject: User)
Do not repeat the name of a class or module within its members; omit contextual names (e.g., use `class User { getProfil...
Files:
server/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.tsserver/test/utils/createCredential.test.ts
server/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
server/**/*.{ts,tsx}: Server API: implement schema-first approach using OpenAPI via hono with validation via valibot middleware
Server database: drizzle schema is source of truth. Migrations required. No direct database access in handlers - usec.var.db
Files:
server/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.tsserver/test/utils/createCredential.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
For files with a single
defaultexport, name the file identically to the export; for files with multiple exports, use camelCase with a strong preference for a single word
Files:
server/script/openapi.tsserver/queues/constants.tsserver/test/queues/alchemyQueue.test.tsserver/queues/alchemyQueue.tsserver/utils/createCredential.tsserver/test/utils/createCredential.test.ts
**/.changeset/*.md
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Use a lowercase sentence in the imperative present tense for changeset summaries
Files:
.changeset/short-cars-return.md
🧠 Learnings (6)
📚 Learning: 2025-12-23T19:58:16.574Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T19:58:16.574Z
Learning: Zero config local dev environment: no `.env` files, mock all external services
Applied to files:
server/script/openapi.ts
📚 Learning: 2025-12-23T19:58:16.574Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T19:58:16.574Z
Learning: Server stack: use hono, node.js, drizzle orm, postgres
Applied to files:
server/script/openapi.ts
📚 Learning: 2025-12-23T19:57:35.503Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/server.mdc:0-0
Timestamp: 2025-12-23T19:57:35.503Z
Learning: Applies to server/**/*.ts : Do not access `process.env` directly in application code; load all configuration and secrets once at startup and pass them through dependency injection or context
Applied to files:
server/script/openapi.tsserver/queues/alchemyQueue.ts
📚 Learning: 2025-12-23T19:57:35.503Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/server.mdc:0-0
Timestamp: 2025-12-23T19:57:35.503Z
Learning: Applies to server/**/*.ts : Avoid long-running, synchronous operations; use `async/await` correctly and be mindful of CPU-intensive tasks to prevent blocking the event loop
Applied to files:
server/queues/alchemyQueue.ts
📚 Learning: 2025-12-31T00:23:55.034Z
Learnt from: cruzdanilo
Repo: exactly/exa PR: 610
File: .changeset/ready-experts-fly.md:1-2
Timestamp: 2025-12-31T00:23:55.034Z
Learning: In the exactly/exa repository, allow and require empty changeset files (containing only --- separators) when changes are not user-facing and do not warrant a version bump. This is needed because CI runs changeset status --since origin/main and requires a changeset file to exist. Ensure such empty changesets are used only for non-user-facing changes and document the rationale in the commit or changelog notes.
Applied to files:
.changeset/short-cars-return.md
📚 Learning: 2025-12-30T15:03:28.449Z
Learnt from: CR
Repo: exactly/exa PR: 0
File: .cursor/rules/style.mdc:0-0
Timestamp: 2025-12-30T15:03:28.449Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `ts-expect-error` instead of `ts-ignore`; follow it immediately with a single-line lowercase comment explaining why the error is expected, without separators like `-` or `:`
Applied to files:
server/utils/createCredential.ts
🧬 Code graph analysis (4)
server/test/queues/alchemyQueue.test.ts (3)
.maestro/maestro.d.ts (1)
Response(19-19)server/queues/constants.ts (1)
AlchemyJob(7-9)server/queues/alchemyQueue.ts (2)
AlchemyJobData(15-20)processor(42-69)
server/queues/alchemyQueue.ts (1)
server/queues/constants.ts (2)
QueueName(1-3)AlchemyJob(7-9)
server/utils/createCredential.ts (2)
server/queues/alchemyQueue.ts (1)
alchemyQueue(34-34)server/queues/constants.ts (1)
AlchemyJob(7-9)
server/test/utils/createCredential.test.ts (4)
.maestro/maestro.d.ts (1)
Response(19-19)server/utils/createCredential.ts (1)
createCredential(24-61)server/queues/alchemyQueue.ts (1)
alchemyQueue(34-34)server/queues/constants.ts (1)
AlchemyJob(7-9)
🔇 Additional comments (5)
server/script/openapi.ts (1)
25-26: LGTM!The REDIS_URL change to a full URL format is more explicit, and adding DISABLE_WORKERS prevents the queue worker from starting during OpenAPI spec generation, which is appropriate for this tooling script.
server/test/utils/createCredential.test.ts (1)
73-81: LGTM!The exception handling test correctly verifies that queue failures are captured via Sentry's
captureException, ensuring observability for background job enqueue failures.server/queues/constants.ts (1)
1-11: LGTM!Clean constants definition following TypeScript best practices. The use of
as constfor literal types and indexed access types for enums ensures type safety throughout the queue system.server/test/queues/alchemyQueue.test.ts (2)
68-71: LGTM!The test correctly validates that the processor throws an error for unknown job names, aligning with the processor's default case behavior.
73-89: LGTM!Excellent addition of the API failure test case. This ensures the processor correctly handles and reports non-ok responses from the Alchemy API, improving resilience and observability.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
5e59af2 to
fdc90ad
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Additional Comments (3)
On retry the client calls the same endpoint again, which tries to insert the same credential ID a second time. Because the row is already in the database that second insert will hit a unique-constraint violation, returning a 500 instead of the expected success — the user is permanently stuck. The fix is to validate if (!webhookId) throw new WebhookNotReadyError();
await database.insert(credentials).values([
{
account,
id: credentialId,
publicKey,
factory: exaAccountFactoryAddress,
transports: options?.webauthn?.transports,
counter: options?.webauthn?.counter,
source: options?.source,
},
]);
If Consider either:
} catch (error) {
captureException(error, { level: "error", tags: { queue: QUEUE_NAME, phase: "initialization" } });
await _alchemyQueue?.close();
_alchemyQueue = undefined;
return;
}
BullMQ internally requires one connection for pub/sub (blocking commands) and another for regular commands, and it duplicates the provided connection to achieve this. When both the The safest approach (as also mentioned in BullMQ docs) is to pass an import { Redis as IORedis } from "ioredis";
if (!process.env.REDIS_URL) throw new Error("missing redis url");
const bullmqRedis = new IORedis(process.env.REDIS_URL, { maxRetriesPerRequest: null });
// pass bullmqRedis to Queue / Worker, never call .quit() on it from outside BullMQ |
Additional Comments (2)
The
The user can never successfully retry because the second attempt always 500s, defeating the entire purpose of the 503 retriable pattern. The fix is to move the webhook guard above the database insert: if (!webhookId) throw new WebhookNotReadyError();
await database.insert(credentials).values([
{
account,
id: credentialId,
publicKey,
factory: exaAccountFactoryAddress,
transports: options?.webauthn?.transports,
counter: options?.webauthn?.counter,
source: options?.source,
},
]);
The test correctly asserts that Adding an assertion like the one below would have caught the ordering issue: it("should throw WebhookNotReadyError when webhookId is undefined", async () => {
mocks.webhookId.value = undefined;
const credentialId = "0x1234567890123456789012345678901234567890";
await expect(createCredential(mockContext, credentialId)).rejects.toThrow(WebhookNotReadyError);
// DB must NOT have been written — a real retry would hit a unique-constraint error otherwise
expect(vi.mocked(database.insert)).not.toHaveBeenCalled();
}); |
Additional Comments (2)
The On retry, the same The fix is to check // guard before any side effects
if (!webhookId) throw new WebhookNotReadyError();
await database.insert(credentials).values([
{
account,
id: credentialId,
...
},
]);
In In practice this may pass due to V8's microtask ordering, but it is fragile. Consider wrapping the assertion in await createCredential(mockContext, credentialId);
await vi.waitFor(() => {
expect(vi.mocked(captureException)).toHaveBeenCalledWith(
error,
expect.objectContaining({ ... }),
);
}); |
There was a problem hiding this comment.
💡 Codex Review
exa/server/utils/createCredential.ts
Line 44 in bf7e914
Check webhook readiness before inserting credentials
createCredential persists the credential row before verifying webhookId, so when webhook initialization is still pending the function throws WebhookNotReadyError after writing to credentials. In the registration handlers this returns 503 on the first attempt, but a retry with the same credential now hits a unique-key insert error and falls through the generic 500 path instead of the intended retriable response. Move the readiness guard ahead of the insert (or make the insert idempotent) so retries remain valid while the webhook is unavailable.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Additional Comments (2)
The queue is mocked here, but if (!webhookId) throw new WebhookNotReadyError();where
vi.mock("../../hooks/activity", () => ({
get webhookId() { return mocks.webhookId.value; },
}));The same pattern should be applied here so these tests don't depend on an environment variable being present:
The This is a design trade-off, and the choice to not block credential creation is reasonable. It's worth being explicit in a comment that this is intentional and that ops must monitor the Sentry // Fire-and-forget: credential creation is not blocked on queue availability.
// Monitor Sentry for `extra.job = "add-subscriber"` errors — a failure here
// means the user's address was never subscribed to the Alchemy webhook.
getAlchemyQueue()
.add(AlchemyJob.ADD_SUBSCRIBER, { account, webhookId })
.catch((error: unknown) =>
captureException(error, {
level: "error",
extra: { job: AlchemyJob.ADD_SUBSCRIBER, account, webhookId, credentialId },
}),
); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Additional Comments (2)
If either Using
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
server/utils/createCredential.ts
Outdated
| getAlchemyQueue() | ||
| .add(AlchemyJob.ADD_SUBSCRIBER, { account, webhookId }) | ||
| .catch((error: unknown) => | ||
| captureException(error, { | ||
| level: "error", | ||
| extra: { job: AlchemyJob.ADD_SUBSCRIBER, account, webhookId, credentialId }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🚩 Credential is inserted into DB before webhook subscription is queued
The credential is persisted to the database at line 109 before the webhook subscription job is enqueued at line 138. If the server crashes between these two operations, the credential exists in the DB but no webhook subscription job was ever queued. This means the account won't receive activity notifications until manually remediated. The old code had the same gap (fire-and-forget .catch()), so this isn't a regression, but with a job queue it could be improved by making the job enqueue part of the main flow rather than fire-and-forget.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5344ea539d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| credentialId: C, | ||
| options?: { source?: string; webauthn?: WebAuthnCredential }, | ||
| ) { | ||
| if (!webhookId) throw new WebhookNotReadyError(); |
There was a problem hiding this comment.
This unconditional WebhookNotReadyError turns a transient startup failure into a persistent outage for new credential creation: webhookId is initialized once in hooks/activity via a single findWebhook(...).then(...).catch(...) chain, and the catch path only logs/captures without retrying or re-initializing. If that initial Alchemy call fails once (e.g., brief network error), webhookId remains undefined for the life of the process, so every new registration/SIWE-first-login keeps returning 503 until a restart.
Useful? React with 👍 / 👎.
| .then(async (results) => { | ||
| await closeRedis(); | ||
| if (error) reject(error); | ||
| else if (results.some((result) => result.status === "rejected")) reject(new Error("closing services failed")); | ||
| else resolve(null); |
There was a problem hiding this comment.
🟡 closeRedis() failure in shutdown masks other close errors and server error
In the close() function, await closeRedis() at line 326 is called inside the .then() callback before the error-checking logic at lines 327-329. If closeRedis() throws, the rejection propagates to .catch(reject), completely bypassing the checks for the server.close error and allSettled results. This means: (1) if the server had a close error, it is swallowed; (2) if any service in allSettled (Sentry, Segment, DB, credential queue) failed, those failures are silently masked by the Redis error. closeRedis() should be wrapped in try/catch or included in allSettled to avoid masking other shutdown failures.
(Refers to lines 325-330)
Prompt for agents
In server/index.ts, lines 325-330, the `await closeRedis()` call should be wrapped in a try/catch so that its failure does not bypass the checks for the server.close error and allSettled results. One approach:
.then(async (results) => {
const redisResult = await closeRedis().then(() => ({ status: 'fulfilled' as const, value: null })).catch((e) => ({ status: 'rejected' as const, reason: e }));
if (error) reject(error);
else if (results.some((result) => result.status === 'rejected') || redisResult.status === 'rejected') reject(new Error('closing services failed'));
else resolve(null);
})
Alternatively, restructure so that closeRedis() is part of a second allSettled round after closeCredentialQueue completes, or wrap it in try/catch and merge the result with the allSettled results.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 633264c817
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| credentialId: C, | ||
| options?: { source?: string; webauthn?: WebAuthnCredential }, | ||
| ) { | ||
| if (!webhookId) throw new WebhookNotReadyError(); |
There was a problem hiding this comment.
createCredential now hard-fails with WebhookNotReadyError whenever webhookId is unset, but webhookId is only assigned by the one-time startup promise in hooks/activity.ts (which just logs/captures on failure and does not retry). If Alchemy is temporarily unavailable during boot, this guard makes all new SIWE/WebAuthn credential creations return 503 for the lifetime of the process, so the advertised “please retry” path never recovers without a restart.
Useful? React with 👍 / 👎.
| }); | ||
|
|
||
| export async function closeQueue() { | ||
| await Promise.all([worker.close(), queue.close()]); |
There was a problem hiding this comment.
Using Promise.all here short-circuits on the first rejection, so if either worker.close() or queue.close() fails, closeQueue() rejects before awaiting the other close path. In server/index.ts, shutdown then proceeds to closeRedis(), which can race with the still-closing BullMQ resource and produce flaky shutdown errors/leftover handles when one close operation is already failing.
Useful? React with 👍 / 👎.
| export const CredentialJob = { CREATE: "create" } as const; | ||
|
|
||
| export type CredentialJobData = { account: string; webhookId: string }; | ||
|
|
||
| const queue = new Queue(queueName, { connection: redisConnection }); | ||
|
|
||
| export async function processor(job: Job<CredentialJobData>) { | ||
| return startSpan( | ||
| { name: "credential.processor", op: "queue.process", attributes: { job: job.name, ...job.data } }, | ||
| async (span: Span) => { | ||
| switch (job.name) { | ||
| case CredentialJob.CREATE: { | ||
| const { account, webhookId: webhook } = job.data; | ||
| const response = await fetch("https://dashboard.alchemy.com/api/update-webhook-addresses", { | ||
| method: "PATCH", | ||
| headers, | ||
| body: JSON.stringify({ webhook_id: webhook, addresses_to_add: [account], addresses_to_remove: [] }), | ||
| }); | ||
| if (!response.ok) { | ||
| const text = await response.text(); | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: text }); | ||
| throw new Error(`${response.status} ${text}`); | ||
| } | ||
| break; | ||
| } | ||
| default: { | ||
| const message = `Unknown job name: ${job.name}`; | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message }); | ||
| throw new Error(message); | ||
| } | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| const worker = new Worker(queueName, processor, { | ||
| connection: redisConnection, | ||
| limiter: { max: 10, duration: 1000 }, | ||
| }); | ||
|
|
||
| worker | ||
| .on("failed", (job: Job<CredentialJobData> | undefined, error: Error) => { | ||
| captureException(error, { level: "error", extra: { job: job?.data } }); | ||
| }) | ||
| .on("completed", (job: Job<CredentialJobData>) => { | ||
| addBreadcrumb({ category: "queue", message: `Job ${job.id} completed`, level: "info", data: { job: job.data } }); | ||
| }) | ||
| .on("active", (job: Job<CredentialJobData>) => { | ||
| addBreadcrumb({ category: "queue", message: `Job ${job.id} active`, level: "info", data: { job: job.data } }); | ||
| }) | ||
| .on("error", (error: Error) => { | ||
| captureException(error, { level: "error", tags: { queue: queueName } }); | ||
| }); | ||
|
|
||
| export async function closeQueue() { | ||
| await Promise.all([worker.close(), queue.close()]); | ||
| } | ||
|
|
||
| export class WebhookNotReadyError extends Error { | ||
| constructor() { | ||
| super("alchemy webhook not initialized yet, retry credential creation"); | ||
| this.name = "WebhookNotReadyError"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Default export placed after ~60 lines of supporting code, violating AGENTS.md file ordering rule
AGENTS.md mandates: "the default export — the thing the file exists for — goes first. standalone function declarations only exist because they were extracted for reuse or genuine complexity — they are supporting details and belong at the bottom alongside internal constants and types." In server/utils/createCredential.ts, the default export createCredential is at line 94, but ~60 lines of queue infrastructure (queueName, CredentialJob, CredentialJobData, queue, processor, worker event handlers, closeQueue, WebhookNotReadyError) are placed before it (lines 27–92). Since createCredential is a function declaration (hoisted), it can safely be placed first — its body only executes at call time, when all module-level const/class bindings are initialized.
Prompt for agents
In server/utils/createCredential.ts, move the default export function `createCredential` (currently at lines 94–147) to immediately after the imports (around line 27). Then place all the supporting declarations below it in order of relevance: `queueName` const, `CredentialJob` const, `CredentialJobData` type, `queue` const, `processor` function, `worker` const with event handlers, `closeQueue` function, and `WebhookNotReadyError` class. This follows the AGENTS.md rule that the default export goes first and supporting details go at the bottom.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f520abb1fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| credentialId: C, | ||
| options?: { source?: string; webauthn?: WebAuthnCredential }, | ||
| ) { | ||
| if (!webhookId) throw new WebhookNotReadyError(); |
There was a problem hiding this comment.
createCredential now hard-fails with WebhookNotReadyError whenever webhookId is unset, but webhookId is initialized by a one-shot async call in hooks/activity.ts that only logs on failure and does not retry. If Alchemy webhook discovery/creation fails once during startup (for example, transient network/API failure), webhookId remains undefined for the life of the process and every new registration/SIWE-first-login returns 503 until a restart, effectively causing a prolonged signup outage from a transient boot error.
Useful? React with 👍 / 👎.
| }).catch((error: unknown) => captureException(error, { level: "error" })), | ||
| ]); | ||
|
|
||
| queue.add(CredentialJob.CREATE, { account, webhookId }).catch((error: unknown) => |
There was a problem hiding this comment.
Jobs are added without any cleanup options (removeOnComplete / removeOnFail) and the queue has no default job options, so completed and failed credential jobs are retained in Redis indefinitely. Since this path runs on each new credential, Redis keyspace and memory usage will grow monotonically in production, eventually hurting queue performance and operational stability.
Useful? React with 👍 / 👎.
3 participants
closes #177
Summary by CodeRabbit
New Features
Bug Fixes
Tests
This is part 1 of 2 in a stack made with GitButler:
Greptile Summary
This PR migrates the Alchemy webhook subscriber registration from a synchronous inline
fetchcall to an async BullMQ job queue, and introduces aWebhookNotReadyError(503) guard so that credential creation fails gracefully when the webhook hasn't been initialized yet. Redis is also split into two connections — one withmaxRetriesPerRequest: nullfor BullMQ compatibility and one withmaxRetriesPerRequest: 3for regular request handling.Key changes:
server/queues/alchemyQueue.ts: defines the BullMQ queue, worker, processor, and lifecycle helpers (initializeWorker,close).createCredential.ts: guards onwebhookIdbefore DB insert, removes the directupdateWebhookAddressescall, and fire-and-forgets a BullMQADD_SUBSCRIBERjob after the credential is committed.authentication.tsandregistration.ts: catchWebhookNotReadyErrorand return 503 with a retriable tag.redis.ts: splits into a BullMQ-safe default connection (maxRetriesPerRequest: null) and a request-scopedrequestRedis(maxRetriesPerRequest: 3).close()function inalchemyQueue.tsusesPromise.allrather thanPromise.allSettled, meaning if either the worker or queue close call rejects, thealchemyWorker = undefined/_alchemyQueue = undefinedcleanup never runs and leaves stale module-level references.Confidence Score: 3/5
close()should be addressed before this reaches production.close()function inalchemyQueue.tsusesPromise.allwhich means cleanup state is never reset if either close call fails — this can cause subtle bugs on repeated close calls or in tests. Combined with the previously noted shutdown race condition (queue and Redis closing concurrently) and lack of retry/backoff on failed jobs, the queue infrastructure has three reliability gaps that could affect production.server/queues/alchemyQueue.ts—close()cleanup bug, missing retry config, and inconsistent Sentry imports.server/index.ts— shutdown ordering of queue vs Redis.Important Files Changed
close()usesPromise.allwhich skips cleanup if either close call fails; should usePromise.allSettled. Mixing@sentry/coreand@sentry/nodeimports is inconsistent. No retry/backoff configured on the queue (noted in prior review). Both Queue and Worker share the same redis connection (BullMQ will duplicate it internally, so this is safe).webhookIdbefore inserting credentials (returns 503 viaWebhookNotReadyErrorif not ready), then enqueues a fire-and-forget BullMQ job after DB commit. The fire-and-forget pattern is intentional but means a silent Redis failure at enqueue time leaves the user unsubscribed from the webhook with only a Sentry event as signal.maxRetriesPerRequest: nullfor BullMQ compatibility;requestRedisusesmaxRetriesPerRequest: 3for API request handlers.closeRediscloses both connections.closeAlchemyQueue()andcloseRedis()run concurrently inPromise.allSettled, creating a shutdown race condition (noted in prior review).WebhookNotReadyErrorhandler in the siwe credential creation path (mirrors the registration.ts approach) and switches torequestRedis. Addresses the gap noted in prior review.WebhookNotReadyError503 handling and switches torequestRedis. Straightforward and correct.initializeWorkeror the event handler callbacks, but core processing logic is tested.WebhookNotReadyErrorwhen webhookId is absent, and exception capture on queue failure. All three cases are well-structured and use proper hoisted mocks.Sequence Diagram
sequenceDiagram participant Client participant Auth as Auth Endpoint participant CC as createCredential participant DB as Database participant AQ as AlchemyQueue (BullMQ) participant Redis participant W as AlchemyWorker participant Alchemy as Alchemy API Client->>Auth: POST register or SIWE Auth->>CC: createCredential(ctx, credentialId) CC->>CC: check webhookId is set alt webhookId not initialized CC-->>Auth: throw WebhookNotReadyError Auth-->>Client: 503 retriable else webhookId ready CC->>DB: INSERT credential row CC->>CC: setSignedCookie plus sardine customer CC-)AQ: queue.add(ADD_SUBSCRIBER, data) fire-and-forget CC-->>Auth: return credential info Auth-->>Client: 200 OK Note over AQ,W: Background processing AQ->>Redis: persist job W->>Redis: dequeue job W->>Alchemy: PATCH update-webhook-addresses alt success Alchemy-->>W: 200 else failure Alchemy-->>W: error W-->>W: mark failed, no retry configured end endLast reviewed commit: 65ebdb9