Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 52 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded OpenTelemetry tracing to the registrar-actions API handler and the registrar-actions business layer. Introduced module-level tracers and wrapped key operations in named spans, attaching span attributes. Adjusted the count query by removing an inner join to the domain/subgraph table. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Handler\n(registrar-actions-api)"
participant Tracer as "OpenTelemetry\nTracer"
participant BL as "Business Logic\n(find-registrar-actions)"
participant DB
Client->>API: Request fetchRegistrarActions(params)
API->>Tracer: withActiveSpanAsync("fetchRegistrarActions")
Tracer->>API: span(active)
API->>BL: call fetchRegistrarActions (within span)
BL->>Tracer: withSpanAsync("registrarActions.count") / "registrarActions.find"
Tracer->>BL: span(s) active
BL->>DB: execute count/find SQL (without domain innerJoin for count)
DB-->>BL: rows / count
BL-->>API: registrarActions + pageContext
API-->>Tracer: end span("fetchRegistrarActions")
API-->>Client: response (registrarActions, pageContext)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
| eq(ensIndexerSchema.registrarActions.node, ensIndexerSchema.registrationLifecycles.node), | ||
| ) | ||
| // join Domains associated with Registration Lifecycles | ||
| .innerJoin( |
There was a problem hiding this comment.
removes the unnecessary join on domains, not needed for count
apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts
Outdated
Show resolved
Hide resolved
Greptile SummaryThis PR is a hotfix targeting two related production issues in the registrar actions API: (1) excessive query latency caused by an unnecessary Key changes:
The performance motivation is clear and the structural change is minimal. The join asymmetry between count and find (count omits Confidence Score: 4/5Safe to merge for the performance win, but count/find join asymmetry flagged in prior threads remains structurally present and unaddressed. The core performance fix (removing the subgraph_domain INNER JOIN from the count query) is correct and addresses the stated production latency problem. The tracing additions are additive and do not change query behavior. However, _countRegistrarActions now joins three tables while _findRegistrarActions joins four — the count omits the subgraph_domain INNER JOIN that the find query uses to hydrate domain.name. Under normal data integrity this is fine, but if any registrationLifecycle/subregistries row lacks a corresponding subgraph_domain entry, totalRecords will exceed the actual rows returned, producing phantom pagination pages. This concern flagged in prior threads has not been structurally resolved in this revision, warranting a 4/5 rather than 5/5. apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts — specifically the join difference between _countRegistrarActions and _findRegistrarActions. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Handler as registrar-actions-api.ts
participant Fetcher as fetchRegistrarActions
participant FA as findRegistrarActions
participant Count as _countRegistrarActions
participant Find as _findRegistrarActions
participant DB as PostgreSQL
Client->>Handler: GET /api/registrar-actions[/:parentNode]
Handler->>Fetcher: fetchRegistrarActions(parentNode, query)
Note over Fetcher: OTel active span: fetchRegistrarActions
Fetcher->>FA: findRegistrarActions(options)
FA->>Count: _countRegistrarActions(filters)
Note over Count: OTel span: registrarActions.count
Count->>DB: SELECT count(*) 3-table JOIN + WHERE
DB-->>Count: count N
Count-->>FA: N
FA->>Find: _findRegistrarActions(options)
Note over Find: OTel span: registrarActions.find
Find->>DB: SELECT rows 4-table JOIN + WHERE + LIMIT/OFFSET
DB-->>Find: rows[]
Find-->>FA: rows[]
FA-->>Fetcher: registrarActions + totalRecords
Fetcher-->>Handler: registrarActions + pageContext
Handler-->>Client: JSON response
Reviews (2): Last reviewed commit: "fix: actually keep the join for correctn..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Hotfix aimed at reducing production slowness in the Registrar Actions explore endpoints by cutting down expensive query patterns (notably unnecessary joins/full table scan behavior) and adding tracing around the critical paths to aid diagnosis.
Changes:
- Optimized
_countRegistrarActionsto avoid joining extra tables unless a subregistry-node filter is present. - Added OpenTelemetry spans around registrar-actions DB queries and API fetch logic for better observability.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts | Adds OTel spans and refactors the count query to conditionally join tables only when needed. |
| apps/ensapi/src/handlers/api/explore/registrar-actions-api.ts | Wraps shared fetch logic in an active OTel span and attaches request attributes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts
Outdated
Show resolved
Hide resolved
apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts
Outdated
Show resolved
Hide resolved
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts`:
- Around line 25-27: The spans created in _countRegistrarActions and
_findRegistrarActions use withSpanAsync (which calls tracer.startSpan) causing
them to be siblings of the outer span; change those calls to withActiveSpanAsync
so the spans become children of the active parent span. Specifically, replace
withSpanAsync with withActiveSpanAsync around the registrarActions.count and
registrarActions.find calls in the _countRegistrarActions and
_findRegistrarActions functions (keeping the same span names and attributes) so
the tracing hierarchy is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c86489a-2bdb-42a7-b753-68ea4d564e19
📒 Files selected for processing (2)
apps/ensapi/src/handlers/api/explore/registrar-actions-api.tsapps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts
|
@greptile re-review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts`:
- Around line 104-123: The count query in _countRegistrarActions (the
withSpanAsync "registrarActions.count" block) is missing the same join to the
subgraph_domain table that _findRegistrarActions uses, causing mismatched
totals; update the count builder to include the same join(s) as the fetch
path—i.e., add the innerJoin to ensIndexerSchema.subgraph_domains (the same join
condition used in _findRegistrarActions between ensIndexerSchema.subregistries
and ensIndexerSchema.subgraph_domains) so the .select({ count: count()
}).from(...).innerJoin(...).innerJoin(...).innerJoin(...) chain mirrors
_findRegistrarActions exactly and uses the identical buildWhereClause(filters)
to compute totals that match returned rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e868c11-e8f7-4884-9c7f-e7135210360a
📒 Files selected for processing (1)
apps/ensapi/src/lib/registrar-actions/find-registrar-actions.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.