Open
Conversation
…logic for cases we need
…eded to support it
darrenpicard25
requested review from
maiahneo,
neo-luca and
tjamescouch
as code owners
maiahneo
reviewed
Feb 23, 2026
maiahneo
reviewed
Feb 24, 2026
maiahneo
previously approved these changes
Feb 24, 2026
tjamescouch
reviewed
Feb 26, 2026
tjamescouch
reviewed
Feb 26, 2026
Collaborator
tjamescouch
left a comment
tjamescouch
left a comment
There was a problem hiding this comment.
Just the one comment - looks good otherwise.
📝 WalkthroughWalkthroughIntroduces a plugin system with lifecycle hooks to Chrono, adds plugin context and types, updates MongoDB datastore to support uninitialized behavior, updates tooling/dependencies and test config, rewrites documentation, and adds comprehensive tests for plugin and datastore behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Chrono
participant Plugin
participant PluginCtx as PluginContext
participant DataStore
App->>Chrono: use(plugin)
Chrono->>Plugin: register(PluginRegistrationContext)
Plugin->>PluginCtx: hooks.onStart(handler)
Plugin->>PluginCtx: hooks.onStop(handler)
Plugin-->>Chrono: return PluginAPI
Chrono-->>App: return PluginAPI
App->>Chrono: start()
Chrono->>PluginCtx: executeStartHooks()
PluginCtx->>PluginCtx: create LifecycleContext
PluginCtx->>Plugin: onStart(LifecycleContext) [FIFO order]
Plugin->>DataStore: getDatastore()
Plugin-->>PluginCtx: hook complete
PluginCtx-->>Chrono: hooks executed
Chrono->>Chrono: start processors
App->>Chrono: stop()
Chrono->>PluginCtx: executeStopHooks()
PluginCtx->>PluginCtx: create LifecycleContext
PluginCtx->>Plugin: onStop(LifecycleContext) [LIFO order]
Plugin->>DataStore: getDatastore()
Plugin-->>PluginCtx: hook complete
PluginCtx-->>Chrono: hooks executed
Chrono->>Chrono: stop processors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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/chrono-core/src/chrono.ts (1)
99-110:⚠️ Potential issue | 🔴 CriticalEnsure processor shutdown still runs when a stop hook fails.
If any plugin stop hook throws at Line 100,
stop()exits before stopping processors, which can leave workers running.💡 Proposed fix
public async stop(): Promise<void> { - // Execute plugin stop hooks first (in reverse order - LIFO) - for (const context of [...this.pluginContexts].reverse()) { - await context.executeStopHooks(); - } - - const stopPromises = Array.from(this.processors.values()).map((processor) => processor.stop()); - - try { - await promiseWithTimeout(Promise.all(stopPromises), this.exitTimeoutMs); - } catch (error) { - this.emit(ChronoEvents.STOP_ABORTED, { error, timestamp: new Date() }); + try { + // Execute plugin stop hooks first (in reverse order - LIFO) + for (const context of [...this.pluginContexts].reverse()) { + await context.executeStopHooks(); + } + } finally { + const stopPromises = Array.from(this.processors.values()).map((processor) => processor.stop()); + + try { + await promiseWithTimeout(Promise.all(stopPromises), this.exitTimeoutMs); + } catch (error) { + this.emit(ChronoEvents.STOP_ABORTED, { error, timestamp: new Date() }); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chrono-core/src/chrono.ts` around lines 99 - 110, A plugin stop hook thrown inside the loop over this.pluginContexts (calling context.executeStopHooks()) aborts the whole stop sequence and prevents processors from being stopped; surround each context.executeStopHooks() call (or the whole plugin stop hook phase) with try/catch so failures are caught and logged/emitted (e.g., emit ChronoEvents.STOP_ABORTED with the hook error and timestamp) but do not return/throw, then proceed to collect and await the processors' stop() promises via promiseWithTimeout with this.exitTimeoutMs; ensure processor.stop() is always invoked even if plugin stop hooks fail and surface both hook and processor errors appropriately.
🧹 Nitpick comments (2)
packages/chrono-core/src/plugins/lifecycle-context.ts (1)
8-8: Match the “read-only” contract in the type signature.Line 8 describes read-only access, but Line 15 exposes a mutable array type. Prefer a readonly array return type to enforce the contract.
♻️ Suggested API tweak
- getRegisteredTaskKinds(): (keyof TaskMapping)[]; + getRegisteredTaskKinds(): readonly (keyof TaskMapping)[];Also applies to: 15-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chrono-core/src/plugins/lifecycle-context.ts` at line 8, The API claims "read-only" access but the LifecycleContext interface exposes a mutable array type; update the array-returning member on LifecycleContext (e.g., the getPlugins / plugins method) to return a readonly array type (use ReadonlyArray<Plugin> or readonly Plugin[]) and adjust the implementation to return an immutable view (return a copy or Object.freeze/as const) so callers cannot mutate internal state.packages/chrono-core/test/unit/chrono.test.ts (1)
165-200: Add a regression test for failed plugin startup hooks.A case where
onStartthrows would lock in expected state-recovery behavior and guard against lifecycle regressions.💡 Suggested test case
describe('use', () => { + test('allows retry after start() fails in a plugin onStart hook', async () => { + const failingPlugin: ChronoPlugin<TaskMapping, DatastoreOptions> = { + name: 'failing-plugin', + register(context) { + context.hooks.onStart(() => { + throw new Error('boom'); + }); + }, + }; + + chrono.use(failingPlugin); + await expect(chrono.start()).rejects.toThrow('boom'); + + const plugin: ChronoPlugin<TaskMapping, DatastoreOptions> = { + name: 'late-plugin', + register: vitest.fn(), + }; + + expect(() => chrono.use(plugin)).not.toThrow(); + }); + test('throws when called after start()', async () => { await chrono.start();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chrono-core/test/unit/chrono.test.ts` around lines 165 - 200, Add a regression test that registers a plugin whose onStart hook throws, then assert that calling chrono.start() rejects with that error (or propagates it) and that Chrono recovers to a pre-start state by allowing subsequent plugin registrations; specifically, in chrono.test.ts create a plugin object with name like 'failing-start-plugin' and an onStart that throws, call chrono.use(plugin), await expect(chrono.start()).rejects.toThrow(/* error */), and then verify you can call chrono.use(anotherPlugin) successfully (or check any available pre-start flag/state) to ensure the failed onStart doesn't leave Chrono permanently started/locked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/chrono-core/README.md`:
- Line 58: The README's quick-start uses an undeclared type DatastoreOptions (in
the sample instantiation of Chrono<TaskMapping, DatastoreOptions> and nearby
examples); fix by either declaring and showing the DatastoreOptions
interface/type in the snippet or replacing DatastoreOptions with the actual
options type exported by the library (or a clear inline example like
Record<string, unknown>), and ensure any referenced types (TaskMapping, Chrono)
include import or declaration notes so the examples type-check.
In `@packages/chrono-core/src/chrono.ts`:
- Around line 85-89: In Chrono.start, currently this.started is set to true
before awaiting startup work (looping pluginContexts and
executingExecuteStartHooks), so if any hook throws the method leaves
started=true; change the start flow in the start method (the block that sets
this.started and iterates this.pluginContexts executing executeStartHooks) to
set this.started only after all startup work completes successfully, or wrap the
startup sequence in try/catch and reset this.started = false in the catch before
rethrowing; ensure any startup steps that include executeStartHooks() and
processor startup are covered so subsequent start()/use() behavior is correct.
In `@packages/chrono-mongo-datastore/src/chrono-mongo-datastore.ts`:
- Around line 76-84: The constructor currently uses
rest.uninitializedDatastoreBehavior to decide whether to include maxQueueSize,
which is undefined when the caller omits uninitializedDatastoreBehavior even
though the code sets the default to 'queue'; update the logic to compute the
effective behavior first (e.g., const behavior =
rest.uninitializedDatastoreBehavior || 'queue') and then set
uninitializedDatastoreBehavior: behavior and include maxQueueSize when behavior
=== 'queue' (e.g., ...(behavior === 'queue' ? { maxQueueSize: rest.maxQueueSize
} : undefined)); reference the constructor, ChronoMongoDatastoreConfig, config,
uninitializedDatastoreBehavior, and maxQueueSize when making this change.
In `@README.md`:
- Line 62: The README's Node prerequisite ("Node.js >= 22.14.0") conflicts with
package.json's engines.node (">= 20.18.3"); pick the intended minimum Node
version and make both sources consistent by updating either README.md to match
the package.json engines.node value or updating package.json.engines.node to the
README value—ensure you edit the exact string "Node.js >= 22.14.0" in README.md
or the "engines.node" entry in package.json so they match exactly.
---
Outside diff comments:
In `@packages/chrono-core/src/chrono.ts`:
- Around line 99-110: A plugin stop hook thrown inside the loop over
this.pluginContexts (calling context.executeStopHooks()) aborts the whole stop
sequence and prevents processors from being stopped; surround each
context.executeStopHooks() call (or the whole plugin stop hook phase) with
try/catch so failures are caught and logged/emitted (e.g., emit
ChronoEvents.STOP_ABORTED with the hook error and timestamp) but do not
return/throw, then proceed to collect and await the processors' stop() promises
via promiseWithTimeout with this.exitTimeoutMs; ensure processor.stop() is
always invoked even if plugin stop hooks fail and surface both hook and
processor errors appropriately.
---
Nitpick comments:
In `@packages/chrono-core/src/plugins/lifecycle-context.ts`:
- Line 8: The API claims "read-only" access but the LifecycleContext interface
exposes a mutable array type; update the array-returning member on
LifecycleContext (e.g., the getPlugins / plugins method) to return a readonly
array type (use ReadonlyArray<Plugin> or readonly Plugin[]) and adjust the
implementation to return an immutable view (return a copy or Object.freeze/as
const) so callers cannot mutate internal state.
In `@packages/chrono-core/test/unit/chrono.test.ts`:
- Around line 165-200: Add a regression test that registers a plugin whose
onStart hook throws, then assert that calling chrono.start() rejects with that
error (or propagates it) and that Chrono recovers to a pre-start state by
allowing subsequent plugin registrations; specifically, in chrono.test.ts create
a plugin object with name like 'failing-start-plugin' and an onStart that
throws, call chrono.use(plugin), await expect(chrono.start()).rejects.toThrow(/*
error */), and then verify you can call chrono.use(anotherPlugin) successfully
(or check any available pre-start flag/state) to ensure the failed onStart
doesn't leave Chrono permanently started/locked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cae4f299-c0ac-403b-9bcb-e5290b3efa0c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.gitignoreREADME.mdbiome.jsonpackage.jsonpackages/chrono-core/README.mdpackages/chrono-core/package.jsonpackages/chrono-core/src/chrono.tspackages/chrono-core/src/index.tspackages/chrono-core/src/plugins/chrono-plugin-context.tspackages/chrono-core/src/plugins/index.tspackages/chrono-core/src/plugins/lifecycle-context.tspackages/chrono-core/src/plugins/registration-context.tspackages/chrono-core/test/integration/plugins.test.tspackages/chrono-core/test/unit/chrono.test.tspackages/chrono-core/test/unit/plugins/chrono-plugin-context.test.tspackages/chrono-core/vitest.config.tspackages/chrono-mongo-datastore/README.mdpackages/chrono-mongo-datastore/package.jsonpackages/chrono-mongo-datastore/src/chrono-mongo-datastore.tspackages/chrono-mongo-datastore/test/unit/chrono-mongo-datastore.test.ts
Comment on lines
76
to
84
| constructor(config?: Partial<ChronoMongoDatastoreConfig>) { | ||
| const { completedDocumentTTLSeconds, collectionName, ...rest } = config || {}; | ||
|
|
||
| this.config = { | ||
| completedDocumentTTLSeconds: config?.completedDocumentTTLSeconds, | ||
| collectionName: config?.collectionName || DEFAULT_COLLECTION_NAME, | ||
| uninitializedDatastoreBehavior: rest.uninitializedDatastoreBehavior || 'queue', | ||
| ...(rest.uninitializedDatastoreBehavior === 'queue' ? { maxQueueSize: rest.maxQueueSize } : undefined), | ||
| }; |
There was a problem hiding this comment.
maxQueueSize is silently ignored when uninitializedDatastoreBehavior is not explicitly provided.
When a user provides { maxQueueSize: 100 } without explicitly setting uninitializedDatastoreBehavior, the default behavior correctly becomes 'queue' (line 82). However, line 83 checks rest.uninitializedDatastoreBehavior === 'queue', which is false when the property is undefined. This causes maxQueueSize to be dropped from the config.
Proposed fix
constructor(config?: Partial<ChronoMongoDatastoreConfig>) {
const { completedDocumentTTLSeconds, collectionName, ...rest } = config || {};
+ const behavior = rest.uninitializedDatastoreBehavior || 'queue';
this.config = {
completedDocumentTTLSeconds: config?.completedDocumentTTLSeconds,
collectionName: config?.collectionName || DEFAULT_COLLECTION_NAME,
- uninitializedDatastoreBehavior: rest.uninitializedDatastoreBehavior || 'queue',
- ...(rest.uninitializedDatastoreBehavior === 'queue' ? { maxQueueSize: rest.maxQueueSize } : undefined),
+ uninitializedDatastoreBehavior: behavior,
+ ...(behavior === 'queue' ? { maxQueueSize: rest.maxQueueSize } : undefined),
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| constructor(config?: Partial<ChronoMongoDatastoreConfig>) { | |
| const { completedDocumentTTLSeconds, collectionName, ...rest } = config || {}; | |
| this.config = { | |
| completedDocumentTTLSeconds: config?.completedDocumentTTLSeconds, | |
| collectionName: config?.collectionName || DEFAULT_COLLECTION_NAME, | |
| uninitializedDatastoreBehavior: rest.uninitializedDatastoreBehavior || 'queue', | |
| ...(rest.uninitializedDatastoreBehavior === 'queue' ? { maxQueueSize: rest.maxQueueSize } : undefined), | |
| }; | |
| constructor(config?: Partial<ChronoMongoDatastoreConfig>) { | |
| const { completedDocumentTTLSeconds, collectionName, ...rest } = config || {}; | |
| const behavior = rest.uninitializedDatastoreBehavior || 'queue'; | |
| this.config = { | |
| completedDocumentTTLSeconds: config?.completedDocumentTTLSeconds, | |
| collectionName: config?.collectionName || DEFAULT_COLLECTION_NAME, | |
| uninitializedDatastoreBehavior: behavior, | |
| ...(behavior === 'queue' ? { maxQueueSize: rest.maxQueueSize } : undefined), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/chrono-mongo-datastore/src/chrono-mongo-datastore.ts` around lines
76 - 84, The constructor currently uses rest.uninitializedDatastoreBehavior to
decide whether to include maxQueueSize, which is undefined when the caller omits
uninitializedDatastoreBehavior even though the code sets the default to 'queue';
update the logic to compute the effective behavior first (e.g., const behavior =
rest.uninitializedDatastoreBehavior || 'queue') and then set
uninitializedDatastoreBehavior: behavior and include maxQueueSize when behavior
=== 'queue' (e.g., ...(behavior === 'queue' ? { maxQueueSize: rest.maxQueueSize
} : undefined)); reference the constructor, ChronoMongoDatastoreConfig, config,
uninitializedDatastoreBehavior, and maxQueueSize when making this change.
Collaborator
There was a problem hiding this comment.
@darrenpicard25 this seems valid - since uninitializedDatastoreBehavior defaults to queue, shouldn't the maxQueueSize take effect in this case?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Collaborator
Author
There was a problem hiding this comment.
possible code wise. But not possible typescript wise as you can only pass in a maxQueueSize if uninitializedDatastoreBehavior = 'queue'
Collaborator
Author
There was a problem hiding this comment.
input is a union
tjamescouch
reviewed
Mar 16, 2026
Collaborator
tjamescouch
left a comment
tjamescouch
left a comment
There was a problem hiding this comment.
I think this code rabbit comment should be addressed.
Comment on lines
76
to
84
| constructor(config?: Partial<ChronoMongoDatastoreConfig>) { | ||
| const { completedDocumentTTLSeconds, collectionName, ...rest } = config || {}; | ||
|
|
||
| this.config = { | ||
| completedDocumentTTLSeconds: config?.completedDocumentTTLSeconds, | ||
| collectionName: config?.collectionName || DEFAULT_COLLECTION_NAME, | ||
| uninitializedDatastoreBehavior: rest.uninitializedDatastoreBehavior || 'queue', | ||
| ...(rest.uninitializedDatastoreBehavior === 'queue' ? { maxQueueSize: rest.maxQueueSize } : undefined), | ||
| }; |
Collaborator
There was a problem hiding this comment.
@darrenpicard25 this seems valid - since uninitializedDatastoreBehavior defaults to queue, shouldn't the maxQueueSize take effect in this case?
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
25-52: Consider adding shutdown to the quick example.After
await chrono.start(), the sample never demonstrates teardown. Addingawait chrono.stop()(or equivalent) makes copy-paste usage safer for scripts/tests.Suggested README snippet tweak
await chrono.scheduleTask({ kind: "send-email", when: new Date(), data: { to: "user@example.com", subject: "Hello!", body: "Welcome!" }, }); + +await chrono.stop();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 25 - 52, The quick example starts the scheduler with await chrono.start() but never shows teardown; update the snippet to demonstrate clean shutdown by calling await chrono.stop() after scheduling/processing tasks (or at the end of the example), ensuring you reference the same Chrono instance (chrono) created from ChronoMemoryDatastore and keep the registerTaskHandler/scheduleTask usage intact so users can copy-paste safe scripts/tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Around line 25-52: The quick example starts the scheduler with await
chrono.start() but never shows teardown; update the snippet to demonstrate clean
shutdown by calling await chrono.stop() after scheduling/processing tasks (or at
the end of the example), ensuring you reference the same Chrono instance
(chrono) created from ChronoMemoryDatastore and keep the
registerTaskHandler/scheduleTask usage intact so users can copy-paste safe
scripts/tests.
tjamescouch
approved these changes
Mar 17, 2026
maiahneo
reviewed
Mar 19, 2026
| ) { | ||
| this.chrono = { | ||
| registerTaskHandler: chrono.registerTaskHandler.bind(chrono), | ||
| use: chrono.use.bind(chrono), |
Collaborator
There was a problem hiding this comment.
A plugin can compose other plugins as well, nice!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introducing new Plugin support for Chrono
Plugin contract is extremely flexible allowing plugins almost complete access to defined contracts of chrono internals
Summary by CodeRabbit
New Features
Documentation
Tests
Chores