feat: add reloadIndexer() & async support for indexer constructor & record reorgs #188
Conversation
📝 WalkthroughWalkthroughImplements async indexer constructor support and a runtime reload mechanism. Adds ReloadIndexerRequest error and reloadIndexer() helper for hot-reloading indexers during execution. Propagates AbortSignal through indexer hooks for lifecycle management. Extends drizzle plugin to record chain reorganizations. Updates CLI runtime and example indexers accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Start as start.ts
participant Indexer as Indexer Instance
participant ReloadUtil as reloadIfNeeded()
User->>Start: run start command
loop Reload Loop
Start->>Start: resolve config (env or standalone)
Start->>Start: await createIndexer()
Start->>Start: establish client & logger
Start->>Indexer: run with runWithReconnect
Indexer->>Indexer: execute indexer logic
Indexer->>ReloadUtil: call reloadIfNeeded()
alt Reload Requested
ReloadUtil->>ReloadUtil: check context._reload flag
ReloadUtil->>Indexer: throw ReloadIndexerRequest
Indexer->>Start: propagate ReloadIndexerRequest
Start->>Start: log "reloaded"
Start->>Start: continue loop (reinit)
else No Reload
Indexer->>Start: complete normally
Start->>User: exit
end
end
sequenceDiagram
participant Run as run() in indexer.ts
participant Middleware as handler:middleware hook
participant ReloadUtil as reloadIfNeeded()
participant AbortCtrl as AbortController
AbortCtrl->>Run: create signal
Run->>Run: invoke plugins:init({ abortSignal })
Run->>Run: invoke run:before({ abortSignal })
Run->>Run: connect with abortSignal
Run->>Run: stream data messages
loop Per Message
Run->>ReloadUtil: reloadIfNeeded() before middleware
Run->>Middleware: invoke with { use, abortSignal }
Middleware->>Middleware: register middleware hooks
Run->>ReloadUtil: reloadIfNeeded() after middleware
alt Reload Triggered
ReloadUtil->>Run: throw ReloadIndexerRequest
Run->>AbortCtrl: abort()
else Continue
Run->>Run: process message
end
end
Run->>Run: invoke run:after({ abortSignal })
AbortCtrl->>Run: signal aborted on completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/runtime/internal/app.ts (1)
18-32: Await async createIndexer in testing modules
- In packages/indexer/src/testing/index.ts, change
toconst indexer = createIndexer(indexerConfig);and mark the enclosing function as async.const indexer = await createIndexer(indexerConfig);- In packages/indexer/src/internal/testing.ts, change
toexport function getMockIndexer(…) { … return createIndexer(…); }export async function getMockIndexer(…) { … return await createIndexer(…); }
🧹 Nitpick comments (15)
packages/cli/src/rolldown/config.ts (1)
22-22: Add a short note explaining why fsevents is externalized (macOS-only, optional dep).Helps future maintainers understand the rationale.
Apply this diff:
"bufferutil", - "fsevents", + // macOS-only optional dependency used by some watchers; keep external to avoid bundling resolution noise. + "fsevents",packages/plugin-drizzle/src/index.ts (1)
409-412: Preserve DrizzleStorageError contract & normalize thrown values
Rethrowing arbitrary errors here changes the storage error contract; callers catching onlyDrizzleStorageErrormay miss others.
- Throw
errorif it’s already aDrizzleStorageError.- Wrap all other values into
DrizzleStorageErrorand normalize non-Errorthrows.packages/plugin-drizzle/src/index.ts:409-412
} catch (error) { await removeTriggers(db, tableNames, indexerId); - throw error; + if (error instanceof DrizzleStorageError) throw error; + if (!(error instanceof Error)) { + throw new DrizzleStorageError('Failed to run handler: middleware', { cause: error }); + } + throw error; }examples/cli-drizzle/indexers/2-starknet.indexer.ts (1)
17-23: Prefer the framework logger over console.log for consistent output and formatting.Replace console.log with useLogger().
Apply this diff:
- console.log("[2-starknet] Starknet indexer waiting....."); + const logger = useLogger(); + logger.info("[2-starknet] Starknet indexer waiting....."); // Simulating a API call that takes 5 seconds await new Promise((resolve) => setTimeout(resolve, 5000)); - console.log("[2-starknet] Starknet indexer done waiting....."); + logger.info("[2-starknet] Starknet indexer done waiting.....");If tests run this example, ensure the test timeout accommodates the 5s wait (e.g., in Vitest/Jest):
// vitest import { test } from "vitest"; test.setTimeout(15000); // jest jest.setTimeout(15000);packages/cli/src/core/utils.ts (2)
1-3: Type the helper as never and allow an optional messageSmall DX improvement: make the control-flow explicit and let callers pass context.
-export function reloadIndexer() { - throw new ReloadIndexerRequest(); -} +export function reloadIndexer(message?: string): never { + throw new ReloadIndexerRequest(message); +}
5-10: Harden the custom error (stack capture + cross-realm tag)Mark the error for instanceof-safe detection across multiple package copies/realms and capture stack cleanly.
export class ReloadIndexerRequest extends Error { - constructor(message?: string) { - super(message); - this.name = "ReloadIndexerRequest"; - } + static readonly tag: symbol = Symbol.for("apibara.reloadIndexer"); + constructor(message?: string) { + super(message); + this.name = "ReloadIndexerRequest"; + // Mark for cross-realm / multi-copy checks + (this as any)[ReloadIndexerRequest.tag] = true; + Error.captureStackTrace?.(this, ReloadIndexerRequest); + } }You can also expose a type guard for consumers:
export function isReloadIndexerRequest(err: unknown): err is ReloadIndexerRequest { return ( err instanceof ReloadIndexerRequest || (typeof err === "object" && err !== null && (err as any).name === "ReloadIndexerRequest") || (typeof err === "object" && err !== null && ReloadIndexerRequest.tag in (err as any)) ); }packages/cli/src/core/index.ts (1)
5-5: Re-export the type guard to simplify consumer importsIf you add
isReloadIndexerRequest, surface it here too.-export { ReloadIndexerRequest, reloadIndexer } from "./utils"; +export { ReloadIndexerRequest, reloadIndexer, isReloadIndexerRequest } from "./utils";examples/cli-drizzle/test/starknet.test.ts (1)
13-13: Bump test timeout to account for async indexer initGiven the indexer’s async startup, avoid flakiness by increasing the test timeout.
// Change the test signature (line 12) it("should work", { timeout: 30000 }, async () => {Please confirm the example indexer’s artificial delay (if any) won’t exceed the configured timeout.
packages/cli/src/runtime/project-info.ts (1)
51-55: Minor readability: avoid destructuring from a possibly undefined expressionThis keeps types simple and reads clearer.
- const { indexer: indexerInstance } = - (await createIndexer({ - indexerName: indexer, - processedRuntimeConfig, - preset, - })) ?? {}; + const created = await createIndexer({ + indexerName: indexer, + processedRuntimeConfig, + preset, + }); + const indexerInstance = created?.indexer;packages/cli/src/runtime/internal/app.ts (1)
18-32: Add an explicit return type for the new async APIHelps callers and prevents accidental type widening as usage evolves.
-export async function createIndexer({ +export async function createIndexer({ indexerName, processedRuntimeConfig, preset, }: { indexerName: string; @@ -}) { +}): Promise< + | { + indexer: ReturnType<typeof _createIndexer>; + logger: ReturnType<typeof consola.create>; + } + | undefined +> {packages/cli/src/runtime/start.ts (2)
93-101: Register instrumentation only once.register() is invoked on every reload; if it’s not idempotent, this can duplicate reporters/handlers.
- let _logger: ConsolaInstance | undefined; + let _logger: ConsolaInstance | undefined; + let _registered = false; @@ - if (register) { + if (register && !_registered) { consola.start("Registering from instrumentation"); await register(); consola.success("Registered from instrumentation"); + _registered = true; }
65-69: Preset arg is ignored in non-standalone mode.When running with the CLI, a provided --preset should override env. Use argPreset as highest priority.
- preset = envResult.preset; + preset = argPreset ?? envResult.preset;Would this align with how presets are passed by the outer CLI wrapper?
packages/cli/src/runtime/dev.ts (4)
25-27: Surface missing-indexer error to the user.Silent return makes debugging confusing in dev. Log an explicit error.
- if (!indexerInstance) { - return; - } + if (!indexerInstance) { + _logger?.error?.(`Specified indexer "${indexer}" but it was not defined`); + return; + }
67-69: Trim indexer names parsed from CLI.Handles “a, b” and trailing spaces.
- if (indexersArgs) { - selectedIndexers = indexersArgs.split(","); - } + if (indexersArgs) { + selectedIndexers = indexersArgs.split(",").map((s) => s.trim()).filter(Boolean); + }
79-79: Isolate failures between indexers during dev.One crashing indexer currently rejects Promise.all and may bring down the dev runner. Consider allSettled or per-indexer error handling.
- await Promise.all(selectedIndexers.map((indexer) => startIndexer(indexer))); + const results = await Promise.allSettled( + selectedIndexers.map((indexer) => startIndexer(indexer)), + ); + const rejected = results.filter((r) => r.status === "rejected"); + if (rejected.length) { + throw new Error(`${rejected.length} indexer(s) failed to start`); + }
63-70: Validate unknown args in dev runner for parity with start.Add checkForUnknownArgs to detect typos.
-import { getRuntimeDataFromEnv } from "apibara/common"; +import { getRuntimeDataFromEnv, checkForUnknownArgs } from "apibara/common"; @@ - async run({ args }) { + async run({ args, cmd }) { @@ - const { indexers: indexersArgs } = args; + const { indexers: indexersArgs } = args; + await checkForUnknownArgs(args, cmd);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
change/@apibara-plugin-drizzle-112cac96-eb12-4cd7-9ab9-382e1e000b43.json(1 hunks)change/apibara-1cf36caf-ddc8-48dc-9226-974c69dd9ed9.json(1 hunks)examples/cli-drizzle/indexers/2-starknet.indexer.ts(1 hunks)examples/cli-drizzle/test/starknet.test.ts(1 hunks)packages/cli/src/core/index.ts(1 hunks)packages/cli/src/core/utils.ts(1 hunks)packages/cli/src/rolldown/config.ts(1 hunks)packages/cli/src/runtime/dev.ts(2 hunks)packages/cli/src/runtime/internal/app.ts(2 hunks)packages/cli/src/runtime/project-info.ts(1 hunks)packages/cli/src/runtime/start.ts(2 hunks)packages/plugin-drizzle/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/cli/src/runtime/project-info.ts (1)
packages/cli/src/runtime/internal/app.ts (1)
createIndexer(18-91)
packages/cli/src/runtime/internal/app.ts (1)
packages/indexer/src/indexer.ts (1)
createIndexer(132-153)
packages/cli/src/runtime/dev.ts (5)
packages/cli/src/common/helper.ts (1)
getRuntimeDataFromEnv(10-32)packages/cli/src/runtime/internal/app.ts (1)
createIndexer(18-91)packages/indexer/src/indexer.ts (2)
createIndexer(132-153)runWithReconnect(161-215)packages/protocol/src/client.ts (1)
createAuthenticatedClient(89-114)packages/cli/src/core/utils.ts (1)
ReloadIndexerRequest(5-10)
packages/cli/src/core/utils.ts (1)
packages/cli/src/core/index.ts (2)
reloadIndexer(5-5)ReloadIndexerRequest(5-5)
packages/cli/src/runtime/start.ts (5)
packages/cli/src/common/helper.ts (2)
getProcessedRuntimeConfig(41-86)getRuntimeDataFromEnv(10-32)packages/cli/src/runtime/internal/app.ts (1)
createIndexer(18-91)packages/indexer/src/indexer.ts (2)
createIndexer(132-153)runWithReconnect(161-215)packages/protocol/src/client.ts (1)
createAuthenticatedClient(89-114)packages/cli/src/core/utils.ts (1)
ReloadIndexerRequest(5-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
change/@apibara-plugin-drizzle-112cac96-eb12-4cd7-9ab9-382e1e000b43.json (1)
1-7: No external DrizzleStorageError consumers – patch bump is correct
Search found only the internalDrizzleStorageErrorcheck in packages/plugin-drizzle/src/index.ts; no external code relies on it, so forwarding the original error is non-breaking.examples/cli-drizzle/indexers/2-starknet.indexer.ts (1)
12-12: Async default export aligns with the new runtime; LGTM.packages/cli/src/runtime/internal/app.ts (1)
49-52: LGTM on awaiting the indexer factoryAwaiting function exports removes a class of race conditions during initialization.
packages/cli/src/runtime/start.ts (1)
71-79: LGTM: re-create indexer and logger on each reload.Re-initializing both the indexer and its logger per cycle matches the reload design.
packages/cli/src/runtime/dev.ts (1)
10-49: LGTM: per-indexer reload loop matches requirements.Handling ReloadIndexerRequest per indexer ensures only the requesting indexer restarts in dev mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/plugin-drizzle/tests/persistence.test.ts (1)
49-51: Prefer regex to reduce brittleness in error-message assertion.Use a regex so minor punctuation/whitespace changes don’t break the test.
- await expect(run(mockClient, indexer)).rejects.toThrow( - "Intentional error to test persistence", - ); + await expect(run(mockClient, indexer)).rejects.toThrow(/Intentional error/);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/plugin-drizzle/tests/persistence.test.ts(1 hunks)
🔇 Additional comments (2)
packages/plugin-drizzle/tests/persistence.test.ts (2)
49-51: Expectation updated to original error — aligns with rethrow behavior.Good change. Now the test asserts the underlying error message after the plugin stops wrapping it.
49-51: Confirm no remaining wrapped error message references
I ran a global search across all code and snapshots and found no instances of “Failed to run handler:middleware.” Please manually verify that no tests or snapshots reference this old message.
|
It looks like for some reason this is not working as expected if calling Add the following to the test indexer Then, while the indexer is running, run I think the easiest solution is to have I quickly tested it with the following and it works well. Obviously in the real implementation hooks: {
"run:before": async () => {
const ctx = useIndexerContext();
watchFile("/tmp/test", (curr, prev) => {
ctx._reload = true;
});
},
"handler:middleware": async ({ use }) => {
use(async (ctx, next) => {
if (ctx._reload) {
reloadIndexer();
}
await next();
if (ctx._reload) {
reloadIndexer();
}
});
},
}, |
fracek
left a comment
fracek
left a comment
There was a problem hiding this comment.
We need to support the watchFile use case.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/indexer/src/indexer.ts (1)
404-407: Deduplicate with the same helper.Use the same checkReload helper after middleware to keep behavior uniform and reduce drift.
- if (context._reload) { - context._reload = false; - throw new ReloadIndexerRequest(); - } + checkReload(context);packages/indexer/src/utils/index.ts (2)
3-6: Provide an ergonomic escape hatch for callback contexts.To cover callbacks where AsyncLocalStorage might not propagate, allow passing a context explicitly; fallback to useIndexerContext() otherwise.
-import { useIndexerContext } from "../context"; +import { useIndexerContext, type IndexerContext } from "../context"; -export function reloadIndexer() { - const context = useIndexerContext(); - context._reload = true; -} +export function reloadIndexer(ctx?: IndexerContext) { + const context = ctx ?? useIndexerContext(); + context._reload = true; +}
8-13: Custom error is fine; consider optional reason for better logs.Class definition is correct. If you adopt reason plumbing, you can pass it from the helper to improve observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
change/@apibara-indexer-d23b48af-f33a-40c0-afc3-44ae8d4459d6.json(1 hunks)examples/cli-drizzle/indexers/2-starknet.indexer.ts(1 hunks)packages/cli/src/runtime/dev.ts(2 hunks)packages/cli/src/runtime/start.ts(2 hunks)packages/indexer/package.json(1 hunks)packages/indexer/src/index.ts(1 hunks)packages/indexer/src/indexer.ts(3 hunks)packages/indexer/src/utils/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- change/@apibara-indexer-d23b48af-f33a-40c0-afc3-44ae8d4459d6.json
- packages/indexer/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/src/runtime/start.ts
- packages/cli/src/runtime/dev.ts
- examples/cli-drizzle/indexers/2-starknet.indexer.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/indexer/src/indexer.ts (1)
packages/indexer/src/utils/index.ts (1)
ReloadIndexerRequest(8-13)
packages/indexer/src/utils/index.ts (1)
packages/indexer/src/index.ts (1)
useIndexerContext(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
packages/indexer/src/index.ts (1)
3-3: Barrel re-export looks good.Exporting utils from the index barrel is appropriate and avoids deep imports. No cycle concerns here.
packages/indexer/src/indexer.ts (1)
36-36: Import aligns with the new reload flow.Local import keeps coupling minimal; public re-export in index.ts also enables external consumers.
|
The code looks good but it's still not working as expected. Try to add the hook I showed in the previous comment. Then update the Need to make sure it works in this case too. |
jaipaljadeja
force-pushed
the
feat/qol-improvements-fixes
branch
from
ce63f14 to
0b03fb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
examples/cli-drizzle/indexers/4-async.indexer.ts (2)
12-13: Consider making the delay configurableThe hardcoded 500ms delay simulates an API call but could be made configurable through the runtime configuration for testing different scenarios.
- // Simulating a API call that takes 500 ms - await new Promise((resolve) => setTimeout(resolve, 500)); + // Simulating an API call with configurable delay + const startupDelay = runtimeConfig?.starknet?.startupDelay ?? 500; + await new Promise((resolve) => setTimeout(resolve, startupDelay));
38-49: Consider reducing the transform delay for better performanceThe 3-second delay in the transform function significantly slows down indexing. Consider reducing it or making it configurable unless it's specifically needed for testing purposes.
- await new Promise((resolve) => setTimeout(resolve, 3000)); + // Configurable delay for testing/throttling + const transformDelay = runtimeConfig?.starknet?.transformDelay ?? 100; + if (transformDelay > 0) { + await new Promise((resolve) => setTimeout(resolve, transformDelay)); + }packages/cli/src/runtime/dev.ts (1)
24-26: Log when indexer is missing instead of silently returning.Improves DX parity with
start.ts.- if (!indexerInstance) { - return; - } + if (!indexerInstance) { + _logger?.warn(`Specified indexer "${indexer}" but it was not defined`); + return; + }packages/indexer/src/indexer.ts (2)
473-477: Pass abortSignal to finalize hook for consistency.Keeps all message hooks uniformly cancellable.
- await tracer.startActiveSpan("message finalize", async (span) => { - await indexer.hooks.callHook("message:finalize", { - message: message.finalize, - }); + await tracer.startActiveSpan("message finalize", async (span) => { + await indexer.hooks.callHook("message:finalize", { + message: message.finalize, + abortSignal, + }); span.end(); });
268-269: Thread abortSignal into handler:middleware registration.Plugins may need cancellation during middleware setup. Propagate the signal.
- const middleware = await registerMiddleware(indexer); + const middleware = await registerMiddleware(indexer, abortSignal);-async function registerMiddleware<TFilter, TBlock>( - indexer: Indexer<TFilter, TBlock>, -): Promise<MiddlewareFunction<IndexerContext>> { +async function registerMiddleware<TFilter, TBlock>( + indexer: Indexer<TFilter, TBlock>, + abortSignal?: AbortSignal, +): Promise<MiddlewareFunction<IndexerContext>> { @@ - await indexer.hooks.callHook("handler:middleware", { use }); + await indexer.hooks.callHook("handler:middleware", { use, abortSignal });Also applies to: 529-549, 537-538
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
change/@apibara-indexer-d23b48af-f33a-40c0-afc3-44ae8d4459d6.json(1 hunks)change/@apibara-plugin-drizzle-112cac96-eb12-4cd7-9ab9-382e1e000b43.json(1 hunks)change/apibara-1cf36caf-ddc8-48dc-9226-974c69dd9ed9.json(1 hunks)examples/cli-drizzle/indexers/2-starknet.indexer.ts(1 hunks)examples/cli-drizzle/indexers/4-async.indexer.ts(1 hunks)examples/cli-drizzle/test/starknet.test.ts(1 hunks)packages/cli/src/rolldown/config.ts(1 hunks)packages/cli/src/runtime/dev.ts(2 hunks)packages/cli/src/runtime/internal/app.ts(2 hunks)packages/cli/src/runtime/project-info.ts(1 hunks)packages/cli/src/runtime/start.ts(2 hunks)packages/indexer/package.json(1 hunks)packages/indexer/src/index.ts(1 hunks)packages/indexer/src/indexer.ts(16 hunks)packages/indexer/src/utils/index.ts(1 hunks)packages/plugin-drizzle/src/index.ts(1 hunks)packages/plugin-drizzle/tests/persistence.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/indexer/package.json
- change/@apibara-plugin-drizzle-112cac96-eb12-4cd7-9ab9-382e1e000b43.json
🚧 Files skipped from review as they are similar to previous changes (10)
- change/apibara-1cf36caf-ddc8-48dc-9226-974c69dd9ed9.json
- packages/indexer/src/utils/index.ts
- packages/plugin-drizzle/tests/persistence.test.ts
- packages/indexer/src/index.ts
- examples/cli-drizzle/test/starknet.test.ts
- packages/cli/src/runtime/internal/app.ts
- change/@apibara-indexer-d23b48af-f33a-40c0-afc3-44ae8d4459d6.json
- examples/cli-drizzle/indexers/2-starknet.indexer.ts
- packages/cli/src/runtime/project-info.ts
- packages/plugin-drizzle/src/index.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: fracek
PR: apibara/typescript-sdk#188
File: packages/indexer/src/indexer.ts:321-324
Timestamp: 2025-09-10T13:23:40.640Z
Learning: In packages/indexer/src/indexer.ts, the reload mechanism should use a helper function called `reloadIfNeeded(context)` and should only be called on `data` and `heartbeat` messages, not on other message types like invalidate/finalize/systemMessage.
📚 Learning: 2025-09-10T13:23:40.640Z
Learnt from: fracek
PR: apibara/typescript-sdk#188
File: packages/indexer/src/indexer.ts:321-324
Timestamp: 2025-09-10T13:23:40.640Z
Learning: In packages/indexer/src/indexer.ts, the reload mechanism should use a helper function called `reloadIfNeeded(context)` and should only be called on `data` and `heartbeat` messages, not on other message types like invalidate/finalize/systemMessage.
Applied to files:
packages/cli/src/runtime/start.tspackages/indexer/src/indexer.ts
🧬 Code graph analysis (4)
examples/cli-drizzle/indexers/4-async.indexer.ts (3)
packages/cli/src/types/runtime.ts (1)
ApibaraRuntimeConfig(1-1)packages/indexer/src/indexer.ts (1)
defineIndexer(140-149)packages/starknet/src/index.ts (1)
StarknetStream(26-31)
packages/cli/src/runtime/start.ts (5)
packages/cli/src/common/helper.ts (2)
getProcessedRuntimeConfig(41-86)getRuntimeDataFromEnv(10-32)packages/indexer/src/indexer.ts (2)
createIndexer(157-178)runWithReconnect(186-245)packages/cli/src/runtime/internal/app.ts (1)
createIndexer(18-91)packages/protocol/src/client.ts (1)
createAuthenticatedClient(89-114)packages/indexer/src/utils/index.ts (1)
ReloadIndexerRequest(16-21)
packages/cli/src/runtime/dev.ts (4)
packages/cli/src/common/helper.ts (1)
getRuntimeDataFromEnv(10-32)packages/cli/src/runtime/internal/app.ts (1)
createIndexer(18-91)packages/protocol/src/client.ts (1)
createAuthenticatedClient(89-114)packages/indexer/src/utils/index.ts (1)
ReloadIndexerRequest(16-21)
packages/indexer/src/indexer.ts (5)
packages/indexer/src/compose.ts (1)
MiddlewareFunction(28-31)packages/protocol/src/stream.ts (10)
StreamDataRequest(86-92)StreamDataRequest(94-96)StreamDataResponse(160-167)StreamDataResponse(178-180)Invalidate(98-100)Invalidate(102-102)Finalize(104-106)Finalize(108-108)SystemMessage(123-128)SystemMessage(130-130)packages/protocol/src/client.ts (1)
StreamDataOptions(38-43)packages/protocol/src/common.ts (2)
Cursor(35-50)Cursor(53-53)packages/indexer/src/utils/index.ts (1)
reloadIfNeeded(8-14)
🔇 Additional comments (5)
packages/cli/src/rolldown/config.ts (1)
22-22: Externalizing fsevents is fine; add rationale comment and verify cross‑OS.Good call to silence unresolved import warnings for optional macOS watcher deps. Add context so future edits don’t regress, and please sanity‑check builds/run on Linux/Windows where fsevents isn’t installed.
Apply this small doc tweak:
"bufferutil", - "fsevents", + // macOS-only optional native dep (e.g., chokidar). Externalize to avoid bundling/warnings. + "fsevents",examples/cli-drizzle/indexers/4-async.indexer.ts (1)
29-36: Good preparation for the reload featureThe commented code shows proper integration with the reload mechanism using
abortSignalfor cleanup. When uncommenting, ensure to import the required modules (watchfrom 'node:fs/promises' andreloadIndexerfrom 'apibara/indexer').packages/cli/src/runtime/dev.ts (1)
28-39: Always dispose the client on reload/exit to prevent leaked connections.Wrap
runWithReconnectin try/finally and close/destroy the client.const client = createAuthenticatedClient( indexerInstance.streamConfig, indexerInstance.options.streamUrl, indexerInstance.options.clientOptions, ); - if (logger) { - logger.info(`Indexer ${blueBright(indexer)} started`); - } - - await runWithReconnect(client, indexerInstance); + try { + if (logger) { + logger.info(`Indexer ${blueBright(indexer)} started`); + } + await runWithReconnect(client, indexerInstance); + } finally { + if (client && typeof (client as any).close === "function") { + await (client as any).close(); + } else if (client && typeof (client as any).destroy === "function") { + (client as any).destroy(); + } + } return;packages/cli/src/runtime/start.ts (2)
52-52: Type mismatch: processedRuntimeConfig must be non-optional.Prevents TS compile error when calling
createIndexer.- let processedRuntimeConfig: Record<string, unknown> | undefined; + let processedRuntimeConfig: Record<string, unknown> = {};
86-103: Dispose the client on reload/exit to avoid connection leaks.Mirror the fix suggested for dev.ts.
- const client = createAuthenticatedClient( + const client = createAuthenticatedClient( indexerInstance.streamConfig, indexerInstance.options.streamUrl, indexerInstance.options.clientOptions, ); @@ - if (register) { - consola.start("Registering from instrumentation"); - await register(); - consola.success("Registered from instrumentation"); - } - - if (logger) { - logger.info(`Indexer ${blueBright(indexer)} started`); - } - - await runWithReconnect(client, indexerInstance); + try { + if (register) { + consola.start("Registering from instrumentation"); + await register(); + consola.success("Registered from instrumentation"); + } + if (logger) { + logger.info(`Indexer ${blueBright(indexer)} started`); + } + await runWithReconnect(client, indexerInstance); + } finally { + if (client && typeof (client as any).close === "function") { + await (client as any).close(); + } else if (client && typeof (client as any).destroy === "function") { + (client as any).destroy(); + } + }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/indexer/src/indexer.ts (2)
399-416: Don’t assume the next message is data after reconfiguring the stream.
assert(message._tag === "data")can fail if the server emits a heartbeat/system message first. Read until a data message (or exit) instead.- const { value: message } = await stream.next(); - - assert(message._tag === "data"); - - const [_factoryBlock, _block] = message.data.data; - block = _block; + // Drain non-data messages until first data message arrives. + for (;;) { + const { value: nextMessage, done } = await stream.next(); + if (done) break; + if (nextMessage._tag === "heartbeat") { + reloadIfNeeded(); + continue; + } + if (nextMessage._tag !== "data") { + // Surface to generic message hook for consistency. + await indexer.hooks.callHook("message", { + message: nextMessage, + abortSignal, + }); + continue; + } + const [_factoryBlock, _block] = nextMessage.data.data; + block = _block; + break; + }
302-320: Hook AbortSignal into client.streamData (use options.signal)ClientCallOptions defines signal?: AbortSignal in packages/protocol/src/client.ts — set options.signal = abortSignal (not options.abortSignal) before calling client.streamData so the stream can be cancelled.
File: packages/indexer/src/indexer.ts (around lines 302–320).
🧹 Nitpick comments (2)
packages/indexer/src/indexer.ts (2)
351-358: Avoid recording NaN in metrics when endCursor is undefined.
Number(endCursor?.orderKey)can beNaN. Guard before recording.- indexerMetrics.currentBlockGauge.record( - Number(endCursor?.orderKey), - { - indexer_id: indexerId, - }, - ); + if (endCursor?.orderKey !== undefined) { + indexerMetrics.currentBlockGauge.record(Number(endCursor.orderKey), { + indexer_id: indexerId, + }); + }
345-356: Keep context fields consistent: set/clear production alongside finality.You set
context.finalitybut notcontext.production, and later clear finality/endCursor/cursor but not production. Align initialization/cleanup.const { cursor, endCursor, finality, production } = message.data; context.cursor = cursor; context.endCursor = endCursor; context.finality = finality; + context.production = production; ... context.cursor = undefined; context.endCursor = undefined; context.finality = undefined; + context.production = undefined as any;Also applies to: 452-456
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/indexer/src/indexer.ts(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T13:23:40.640Z
Learnt from: fracek
PR: apibara/typescript-sdk#188
File: packages/indexer/src/indexer.ts:321-324
Timestamp: 2025-09-10T13:23:40.640Z
Learning: In packages/indexer/src/indexer.ts, the reload mechanism should use a helper function called `reloadIfNeeded(context)` and should only be called on `data` and `heartbeat` messages, not on other message types like invalidate/finalize/systemMessage.
Applied to files:
packages/indexer/src/indexer.ts
🧬 Code graph analysis (1)
packages/indexer/src/indexer.ts (6)
packages/indexer/src/compose.ts (1)
MiddlewareFunction(28-31)packages/indexer/src/context.ts (1)
IndexerContext(6-6)packages/protocol/src/client.ts (1)
StreamDataOptions(38-43)packages/cli/src/runtime/start.ts (1)
run(43-113)packages/indexer/src/testing/index.ts (1)
run(23-80)packages/indexer/src/utils/index.ts (1)
reloadIfNeeded(8-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
packages/indexer/src/indexer.ts (3)
43-93: AbortSignal plumbed through hooks and handler args — LGTM.Optional abort propagation across hooks/middleware/transform is consistent and non‑breaking.
If any public plugins rely on strict payload typing, consider a minor release note. Do you want a quick scan of the repo for external plugin packages to double‑check?
Also applies to: 116-117, 247-250, 264-274, 304-309, 321-321, 339-340, 381-381, 403-403, 435-435, 467-467, 477-477, 487-487, 513-513, 533-533, 540-540
36-36: reloadIfNeeded placement matches the agreed behavior.Checks occur only for data and heartbeat messages, before/after processing, aligning with the retrieved learning from fracek. Good.
Also applies to: 359-360, 442-444, 484-489
197-205: Reconnects reuse an already-aborted signal — create a fresh AbortController per attempt.After the first abort, subsequent retries reuse an aborted signal, breaking reconnects and possibly the “second reload” flow. Move the controller inside the loop.
- const abortController = new AbortController(); - - const runOptions: RunOptions = { - onConnect() { - retryCount = 0; - }, - abortSignal: abortController.signal, - }; - - while (true) { - try { - await run(client, indexer, runOptions); - abortController.abort(); + while (true) { + const abortController = new AbortController(); + const runOptions: RunOptions = { + onConnect() { + retryCount = 0; + }, + abortSignal: abortController.signal, + }; + try { + await run(client, indexer, runOptions); + abortController.abort(); return; } catch (error) { retryCount++; abortController.abort(); ... } }Also applies to: 206-245
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/indexer/src/indexer.ts (2)
345-346: Fix: stale cursor/finality/production after switching to new stream (factory mode).After fetching the first message from the rebuilt stream, you set
block = _blockbut keep using the oldcursor/endCursor/finality/production(andcontext.*) from the previous message. This misattributes the processed block and can corrupt downstream state/metrics.Apply:
- const { cursor, endCursor, finality, production } = message.data; + let { cursor, endCursor, finality, production } = message.data; ... - const { value: message } = await stream.next(); - - assert(message._tag === "data"); - const [_factoryBlock, _block] = message.data.data; + const { value: nextMessage } = await stream.next(); + assert(nextMessage._tag === "data"); + const [_factoryBlock, _block] = nextMessage.data.data; + // Refresh metadata for the new message + ({ cursor, endCursor, finality, production } = nextMessage.data); + context.cursor = cursor; + context.endCursor = endCursor; + context.finality = finality;Also applies to: 410-417
343-446: Ensure spans end even when reload is requested.
reloadIfNeeded()throws; when it does,span.end()is skipped, breaking tracing. Wrap span body intry/finally.- await tracer.startActiveSpan("message data", async (span) => { + await tracer.startActiveSpan("message data", async (span) => { + try { ... - span.end(); + } finally { + span.end(); + } });Apply the same try/finally pattern around the heartbeat span (see below). Optionally apply to invalidate/finalize/systemMessage for consistency.
🧹 Nitpick comments (2)
packages/indexer/src/indexer.ts (2)
232-236: Nit: stray blankconsole.log()during reconnect.Tiny polish: drop it or replace with
consola.log()with context.- consola.start("Reconnecting..."); - console.log(); + consola.start("Reconnecting...");
43-93: API: hooks payloads now includeabortSignal— update docs, release notes, and plugin hooks.
- Update docs/examples to show the optional
abortSignalon hook payloads.- Call out the change in release notes / semver (minor) so integrators can test.
- Update & test internal plugins that destructure hook args; check these files: packages/plugin-mongo/src/index.ts, packages/plugin-drizzle/src/index.ts, packages/plugin-sqlite/src/index.ts, packages/indexer/src/plugins/persistence.ts, packages/indexer/src/internal/testing.ts, packages/indexer/src/vcr/record.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/indexer/src/indexer.ts(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T13:23:40.640Z
Learnt from: fracek
PR: apibara/typescript-sdk#188
File: packages/indexer/src/indexer.ts:321-324
Timestamp: 2025-09-10T13:23:40.640Z
Learning: In packages/indexer/src/indexer.ts, the reload mechanism should use a helper function called `reloadIfNeeded(context)` and should only be called on `data` and `heartbeat` messages, not on other message types like invalidate/finalize/systemMessage.
Applied to files:
packages/indexer/src/indexer.ts
🔇 Additional comments (3)
packages/indexer/src/indexer.ts (3)
116-117: PassingabortSignalthrough handler paths looks correct.Factory and transform receive the signal; matches the new HandlerArgs type.
Also applies to: 380-381, 434-435
526-527: run:after moved out of per‑message loop — correct semantics.This prevents running “after” logic on every message.
359-360: reloadIfNeeded is zero-arg — no change requiredVerified: packages/indexer/src/utils/index.ts exports
function reloadIfNeeded()(no parameters); the calls in packages/indexer/src/indexer.ts (around lines 359, 442, 484–488) match this signature — do not passcontext.Likely an incorrect or invalid review comment.
jaipaljadeja
changed the title
2 participants
Resolves #187 & #186
Also adds a feature to drizzle plugin to record reorgs and fixes a invalidation bug