Refactor duplicate event registration error handling - #987
Merged
Conversation
Replace DuplicateEventRegistration ReScript exception with Js.Exn.raiseError so callers get a standard JS Error with a descriptive message like "Duplicate registration of event handlers not allowed for Gravatar.CustomSelection". Also: - Add smoke tests in EventHandlers.ts that exercise duplicate handler and contractRegister paths, exporting caught errors for assertion. - Add test case in EventHandler.test.ts verifying the error messages. - Rename _test.ts files to .test.ts so vitest discovers them. https://claude.ai/code/session_013sDgWWA3hVAoX6YVqJg1Ex
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes duplicate-registration handling in Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
When an event handler or contractRegister is registered multiple times with identical options (wildcard, eventFilters), the handlers are now composed to run sequentially instead of throwing. Mismatched options still error with a user-friendly message. https://claude.ai/code/session_013sDgWWA3hVAoX6YVqJg1Ex
DZakh
enabled auto-merge (squash)
February 24, 2026 18:41
DZakh
disabled auto-merge
February 24, 2026 18:42
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/envio/src/HandlerRegister.res`:
- Around line 191-195: The params passed into Logging.createChildFrom are
currently a record literal ({contractName, eventName}); change this to a JS
object payload instead so downstream JS consumers get an object. Update the call
in raiseDuplicateRegistration (the ~params argument passed to
Logging.createChildFrom / Logging.childError) to pass an object-style payload
mapping contractName and eventName to their values (i.e., a JS object with keys
"contractName" and "eventName") rather than the record literal.
- Around line 197-205: The current eventOptionsMatch function compares
eventFilters (of type Js.Json.t) with reference equality, which fails for
structurally equal JSON; update eventOptionsMatch (the function comparing
Internal.eventOptions) to perform a deep/stable comparison of a.eventFilters and
b.eventFilters instead of using == (for example by canonicalizing to a stable
string representation like JSON stringify or calling a dedicated deepEqual
helper) while keeping the wildcard equality check; ensure the deep comparison
handles None/Some cases the same way as now and applies only to the eventFilters
field of Internal.eventOptions.
In `@scenarios/test_codegen/test/EventHandler.test.ts`:
- Around line 627-649: The test is non-deterministic because
composedContractRegisterCalled may already be true from prior tests; import the
handlers module (the exported symbols composedContractRegisterCalled and
mismatchedHandlerOptionsError) before calling
MockDb.createMockDb().processEvents and explicitly reset
handlers.composedContractRegisterCalled = false (and clear
handlers.mismatchedHandlerOptionsError = undefined) so the subsequent call to
mockDbInitial.processEvents([event]) drives the composition and the assertion
reliably; ensure the import is done at the top of the test and the flags are
reset immediately before triggering event processing.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
scenarios/test_codegen/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/envio/src/HandlerRegister.resscenarios/test_codegen/src/handlers/EventHandlers.tsscenarios/test_codegen/test/CustomSelection.test.tsscenarios/test_codegen/test/EventHandler.test.tsscenarios/test_codegen/test/Logging.test.tsscenarios/test_codegen/test/S.test.tsscenarios/test_codegen/test/fixtures/LogTesting.res
Comment on lines
+191
to
+195
| let raiseDuplicateRegistration = (~contractName, ~eventName, ~msg, ~logger) => { | ||
| let fullMsg = msg ++ " for " ++ contractName ++ "." ++ eventName | ||
| Logging.createChildFrom(~logger, ~params={contractName, eventName})->Logging.childError(fullMsg) | ||
| Js.Exn.raiseError(fullMsg) | ||
| } |
Contributor
There was a problem hiding this comment.
Use an object literal for logger params payload.
~params={contractName, eventName} is a record literal; prefer a JS object for payloads passed between functions.
🛠️ Suggested change
- Logging.createChildFrom(~logger, ~params={contractName, eventName})->Logging.childError(fullMsg)
+ Logging.createChildFrom(
+ ~logger,
+ ~params={"contractName": contractName, "eventName": eventName},
+ )->Logging.childError(fullMsg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/envio/src/HandlerRegister.res` around lines 191 - 195, The params
passed into Logging.createChildFrom are currently a record literal
({contractName, eventName}); change this to a JS object payload instead so
downstream JS consumers get an object. Update the call in
raiseDuplicateRegistration (the ~params argument passed to
Logging.createChildFrom / Logging.childError) to pass an object-style payload
mapping contractName and eventName to their values (i.e., a JS object with keys
"contractName" and "eventName") rather than the record literal.
Comment on lines
+197
to
+205
| let eventOptionsMatch = ( | ||
| existing: option<Internal.eventOptions<Js.Json.t>>, | ||
| incoming: option<Internal.eventOptions<Js.Json.t>>, | ||
| ) => { | ||
| switch (existing, incoming) { | ||
| | (None, None) => true | ||
| | (Some(a), Some(b)) => a.wildcard == b.wildcard && a.eventFilters == b.eventFilters | ||
| | _ => false | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find eventOptions type definition and eventFilters references
rg -n "type eventOptions" -g '*.res' -A 5Repository: enviodev/hyperindex
Length of output: 378
🏁 Script executed:
#!/bin/bash
# Search for eventFilters usage and definition
rg -n "eventFilters" -g '*.res' -B 2 -A 2Repository: enviodev/hyperindex
Length of output: 11204
🏁 Script executed:
#!/bin/bash
# Check the Internal module to find the actual type definition
fd -e res -e resi | head -20 | xargs grep -l "eventOptions\|eventFilters" 2>/dev/nullRepository: enviodev/hyperindex
Length of output: 98
Use deep equality or stable comparison for eventFilters matching.
The eventFilters field is cast from a typed variant (Static with arrays or Dynamic with functions) to opaque Js.Json.t before comparison. Comparing with == on Js.Json.t will reject structurally identical filters if they are different object references. Since the error message directs users to use "identical options," implement deep-equality comparison or document that eventFilters must be the exact same reference across registrations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/envio/src/HandlerRegister.res` around lines 197 - 205, The current
eventOptionsMatch function compares eventFilters (of type Js.Json.t) with
reference equality, which fails for structurally equal JSON; update
eventOptionsMatch (the function comparing Internal.eventOptions) to perform a
deep/stable comparison of a.eventFilters and b.eventFilters instead of using ==
(for example by canonicalizing to a stable string representation like JSON
stringify or calling a dedicated deepEqual helper) while keeping the wildcard
equality check; ensure the deep comparison handles None/Some cases the same way
as now and applies only to the eventFilters field of Internal.eventOptions.
Comment on lines
+627
to
+649
| it("composes duplicate handlers with same options and rejects mismatched options", async () => { | ||
| const mockDbInitial = MockDb.createMockDb(); | ||
|
|
||
| const event = Gravatar.FactoryEvent.createMockEvent({ | ||
| contract: "0x1234567890123456789012345678901234567890", | ||
| testCase: "syncRegistration", | ||
| }); | ||
|
|
||
| // Trigger module load via autoLoadFromSrcHandlers | ||
| await mockDbInitial.processEvents([event]); | ||
|
|
||
| // Dynamic-import EventHandlers.js to access exported error values | ||
| const handlers = await import("../src/handlers/EventHandlers"); | ||
|
|
||
| // Same options → composed without error | ||
| // contractRegister ran during factory event processing, proving compose works | ||
| assert.strictEqual(handlers.composedContractRegisterCalled, true); | ||
|
|
||
| // Different options → throws a user-friendly error | ||
| assert.strictEqual( | ||
| handlers.mismatchedHandlerOptionsError?.message, | ||
| "Cannot register a second handler with different options. Make sure all handlers for the same event use identical options (wildcard, eventFilters) for Gravatar.CustomSelection" | ||
| ); |
Contributor
There was a problem hiding this comment.
Make the composition test deterministic.
composedContractRegisterCalled may already be true from earlier tests that processed FactoryEvent. Import the handlers first and reset the flag before processing so this test actually drives the transition.
🧪 Suggested adjustment
- // Trigger module load via autoLoadFromSrcHandlers
- await mockDbInitial.processEvents([event]);
-
- // Dynamic-import EventHandlers.js to access exported error values
- const handlers = await import("../src/handlers/EventHandlers");
+ // Import handlers early so we can reset flags for deterministic assertions
+ const handlers = await import("../src/handlers/EventHandlers");
+ handlers.composedContractRegisterCalled = false;
+
+ await mockDbInitial.processEvents([event]);📝 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
| it("composes duplicate handlers with same options and rejects mismatched options", async () => { | |
| const mockDbInitial = MockDb.createMockDb(); | |
| const event = Gravatar.FactoryEvent.createMockEvent({ | |
| contract: "0x1234567890123456789012345678901234567890", | |
| testCase: "syncRegistration", | |
| }); | |
| // Trigger module load via autoLoadFromSrcHandlers | |
| await mockDbInitial.processEvents([event]); | |
| // Dynamic-import EventHandlers.js to access exported error values | |
| const handlers = await import("../src/handlers/EventHandlers"); | |
| // Same options → composed without error | |
| // contractRegister ran during factory event processing, proving compose works | |
| assert.strictEqual(handlers.composedContractRegisterCalled, true); | |
| // Different options → throws a user-friendly error | |
| assert.strictEqual( | |
| handlers.mismatchedHandlerOptionsError?.message, | |
| "Cannot register a second handler with different options. Make sure all handlers for the same event use identical options (wildcard, eventFilters) for Gravatar.CustomSelection" | |
| ); | |
| it("composes duplicate handlers with same options and rejects mismatched options", async () => { | |
| const mockDbInitial = MockDb.createMockDb(); | |
| const event = Gravatar.FactoryEvent.createMockEvent({ | |
| contract: "0x1234567890123456789012345678901234567890", | |
| testCase: "syncRegistration", | |
| }); | |
| // Import handlers early so we can reset flags for deterministic assertions | |
| const handlers = await import("../src/handlers/EventHandlers"); | |
| handlers.composedContractRegisterCalled = false; | |
| await mockDbInitial.processEvents([event]); | |
| // Same options → composed without error | |
| // contractRegister ran during factory event processing, proving compose works | |
| assert.strictEqual(handlers.composedContractRegisterCalled, true); | |
| // Different options → throws a user-friendly error | |
| assert.strictEqual( | |
| handlers.mismatchedHandlerOptionsError?.message, | |
| "Cannot register a second handler with different options. Make sure all handlers for the same event use identical options (wildcard, eventFilters) for Gravatar.CustomSelection" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scenarios/test_codegen/test/EventHandler.test.ts` around lines 627 - 649, The
test is non-deterministic because composedContractRegisterCalled may already be
true from prior tests; import the handlers module (the exported symbols
composedContractRegisterCalled and mismatchedHandlerOptionsError) before calling
MockDb.createMockDb().processEvents and explicitly reset
handlers.composedContractRegisterCalled = false (and clear
handlers.mismatchedHandlerOptionsError = undefined) so the subsequent call to
mockDbInitial.processEvents([event]) drives the composition and the assertion
reliably; ensure the import is done at the top of the test and the flags are
reset immediately before triggering event processing.
…d (bool) eventFilters is typed as Js.Json.t so structural equality is safe and needed for deep comparison. wildcard is bool so === suffices. https://claude.ai/code/session_013sDgWWA3hVAoX6YVqJg1Ex
Pattern match on Static/Dynamic variants to use == (structural) for Static data and === (referential) for Dynamic functions, avoiding caml_equal throw on functional values. https://claude.ai/code/session_013sDgWWA3hVAoX6YVqJg1Ex
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/envio/src/HandlerRegister.res (2)
191-195:~paramspayload is still a record literal, not a JS object.The
{contractName, eventName}at line 193 is a ReScript record. The past review comment requesting it be changed to{"contractName": contractName, "eventName": eventName}has not been addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 191 - 195, The params argument passed to Logging.createChildFrom in raiseDuplicateRegistration is still a ReScript record literal; change it to a JS object literal so the logger receives a plain JS object (replace the {contractName, eventName} record with an object literal like {"contractName": contractName, "eventName": eventName}) while keeping the call site Logging.createChildFrom(~logger, ~params=...) and then calling Logging.childError(fullMsg).
197-206:eventFiltersstructural equality concern is still unresolved.The
a.eventFilters == b.eventFilterscomparison onJs.Json.tat line 203 was previously flagged. The past review comment requesting deep/stable equality foreventFiltershas not been addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 197 - 206, In eventOptionsMatch, replace the structural JS equality check a.eventFilters == b.eventFilters with a proper deep JSON equality: call a JSON deep-compare helper (e.g. deepEqualJson(a.eventFilters, b.eventFilters)) or compare canonicalized strings (e.g. Js.Json.stringify(a.eventFilters) === Js.Json.stringify(b.eventFilters)) to ensure stable/deep equality; update or add the helper (deepEqualJson) and use it in the (Some(a), Some(b)) branch inside eventOptionsMatch to compare a.eventFilters and b.eventFilters.
🧹 Nitpick comments (1)
packages/envio/src/HandlerRegister.res (1)
189-189: Remove unusedeventNamespacetype.The type is declared at line 189 but never referenced anywhere in the codebase. It's safe to remove.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` at line 189, The declared type eventNamespace ({contractName: string, eventName: string}) is unused and should be removed; delete the type alias declaration for eventNamespace from HandlerRegister.res to clean up dead code and ensure nothing else references it (search for eventNamespace to confirm no usages before removing).
🤖 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/envio/src/HandlerRegister.res`:
- Around line 236-241: The code writes the handler into eventRegistrations
before validating eventOptions, causing a partial-write if setEventOptions
throws; change the order in the None branch of the event registration flow so
you call setEventOptions(~contractName, ~eventName, ~eventOptions, ~logger)
first, then re-fetch the registration record (t) and only then call
set(~contractName, ~eventName, {...t, handler: Some(newHandler)}). Apply the
same change to the analogous block in setContractRegister so options are
validated first and the record is re-read before persisting the handler to avoid
overwriting freshly-set eventOptions; references: set, setEventOptions,
setContractRegister, eventRegistrations, hasRegistration, getHandler, t,
newHandler.
---
Duplicate comments:
In `@packages/envio/src/HandlerRegister.res`:
- Around line 191-195: The params argument passed to Logging.createChildFrom in
raiseDuplicateRegistration is still a ReScript record literal; change it to a JS
object literal so the logger receives a plain JS object (replace the
{contractName, eventName} record with an object literal like {"contractName":
contractName, "eventName": eventName}) while keeping the call site
Logging.createChildFrom(~logger, ~params=...) and then calling
Logging.childError(fullMsg).
- Around line 197-206: In eventOptionsMatch, replace the structural JS equality
check a.eventFilters == b.eventFilters with a proper deep JSON equality: call a
JSON deep-compare helper (e.g. deepEqualJson(a.eventFilters, b.eventFilters)) or
compare canonicalized strings (e.g. Js.Json.stringify(a.eventFilters) ===
Js.Json.stringify(b.eventFilters)) to ensure stable/deep equality; update or add
the helper (deepEqualJson) and use it in the (Some(a), Some(b)) branch inside
eventOptionsMatch to compare a.eventFilters and b.eventFilters.
---
Nitpick comments:
In `@packages/envio/src/HandlerRegister.res`:
- Line 189: The declared type eventNamespace ({contractName: string, eventName:
string}) is unused and should be removed; delete the type alias declaration for
eventNamespace from HandlerRegister.res to clean up dead code and ensure nothing
else references it (search for eventNamespace to confirm no usages before
removing).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/envio/src/HandlerRegister.res (1)
175-177:⚠️ Potential issue | 🔴 CriticalFix
.resiinterface:getEventFiltersreturn type must be updated tooption<Internal.eventFilters>.The pipeline confirms the build failure: the
.resiinterface still declaresgetEventFiltersas returningoption<Js.Json.t>, but after theeventOptionsfield type change (line 4) the implementation now returnsoption<Internal.eventFilters>. Update the interface file accordingly.- let getEventFilters: (~contractName: string, ~eventName: string) => option<Js.Json.t> + let getEventFilters: (~contractName: string, ~eventName: string) => option<Internal.eventFilters>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 175 - 177, The .resi interface declaration for getEventFilters is outdated: update the return type of getEventFilters from option<Js.Json.t> to option<Internal.eventFilters> to match the implementation (which now returns the eventOptions -> eventFilters chain of type Internal.eventFilters); edit the getEventFilters signature in the .resi to use option<Internal.eventFilters> so the interface aligns with the changed eventOptions field type and the implementation in get(~contractName, ~eventName).eventOptions -> Belt.Option.flatMap(...).
♻️ Duplicate comments (3)
packages/envio/src/HandlerRegister.res (3)
285-291: Same partial-write bug insetContractRegister'sNonebranch.
contractRegisteris committed (line 287–290) beforesetEventOptionsvalidates for conflicts (line 291). Apply the same validate-first, re-fetch-then-write fix here.🐛 Proposed fix
| None => - set(~contractName, ~eventName, { - ...t, - contractRegister: Some(newContractRegister), - }) - setEventOptions(~contractName, ~eventName, ~eventOptions, ~logger) + setEventOptions(~contractName, ~eventName, ~eventOptions, ~logger) + let t = get(~contractName, ~eventName) + set(~contractName, ~eventName, { + ...t, + contractRegister: Some(newContractRegister), + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 285 - 291, In setContractRegister's None branch avoid the partial-write by validating event options before committing: call setEventOptions(~contractName, ~eventName, ~eventOptions, ~logger) first (so conflicts are detected), then re-fetch the current register state (the same t/contractRegister) to ensure no race, and only after successful validation and re-fetch assign contractRegister = Some(newContractRegister) via set(~contractName, ~eventName, { ... }) to perform the write; ensure you reference the existing symbols setEventOptions and the branch updating contractRegister when applying the change.
244-250: Partial-write bug: handler is stored beforesetEventOptionsvalidates for conflicts.In the
Nonebranch,set(...)commits the new handler toeventRegistrations(line 246–249) beforesetEventOptionsruns (line 250). IfsetEventOptionsraises (e.g., a concurrentsetContractRegisteralready stored conflicting options), the handler is permanently written while the registration is considered rejected — leaving the registry corrupt. Validate options first, then re-fetchtbefore writing:🐛 Proposed fix
| None => - set(~contractName, ~eventName, { - ...t, - handler: Some(newHandler), - }) - setEventOptions(~contractName, ~eventName, ~eventOptions, ~logger) + setEventOptions(~contractName, ~eventName, ~eventOptions, ~logger) + let t = get(~contractName, ~eventName) + set(~contractName, ~eventName, { + ...t, + handler: Some(newHandler), + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 244 - 250, The handler is being written to eventRegistrations before setEventOptions runs, causing a partial-write if setEventOptions fails; change the None branch so you first call setEventOptions(~contractName, ~eventName, ~eventOptions, ~logger) to validate/raise, then re-fetch t from eventRegistrations (to detect concurrent changes), and only after reloading/confirming t is still None call set(~contractName, ~eventName, {...t, handler: Some(newHandler)}) to store the handler; use the same identifiers (set, setEventOptions, eventRegistrations, handler) so the update validates options before mutating state and avoids leaving a stale handler on errors.
191-195: Use a JS object literal for the~paramspayload inraiseDuplicateRegistration.
~params={contractName, eventName}is a record literal; the coding guidelines require objects for data passed between functions as payloads.🛠️ Suggested change
- Logging.createChildFrom(~logger, ~params={contractName, eventName})->Logging.childError(fullMsg) + Logging.createChildFrom( + ~logger, + ~params={"contractName": contractName, "eventName": eventName}, + )->Logging.childError(fullMsg)As per coding guidelines, "Use records when working with structured data, and objects to conveniently pass payload data between functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/HandlerRegister.res` around lines 191 - 195, The params argument to Logging.createChildFrom in raiseDuplicateRegistration is a record literal; change it to a JS object literal so payloads use objects per guidelines. Replace ~params={contractName, eventName} with an object-style payload (e.g. ~params={"contractName": contractName, "eventName": eventName}) in the raiseDuplicateRegistration function before calling Logging.createChildFrom, and ensure the resulting value still type-checks with Logging.createChildFrom and Logging.childError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/envio/src/HandlerRegister.res`:
- Around line 175-177: The .resi interface declaration for getEventFilters is
outdated: update the return type of getEventFilters from option<Js.Json.t> to
option<Internal.eventFilters> to match the implementation (which now returns the
eventOptions -> eventFilters chain of type Internal.eventFilters); edit the
getEventFilters signature in the .resi to use option<Internal.eventFilters> so
the interface aligns with the changed eventOptions field type and the
implementation in get(~contractName, ~eventName).eventOptions ->
Belt.Option.flatMap(...).
---
Duplicate comments:
In `@packages/envio/src/HandlerRegister.res`:
- Around line 285-291: In setContractRegister's None branch avoid the
partial-write by validating event options before committing: call
setEventOptions(~contractName, ~eventName, ~eventOptions, ~logger) first (so
conflicts are detected), then re-fetch the current register state (the same
t/contractRegister) to ensure no race, and only after successful validation and
re-fetch assign contractRegister = Some(newContractRegister) via
set(~contractName, ~eventName, { ... }) to perform the write; ensure you
reference the existing symbols setEventOptions and the branch updating
contractRegister when applying the change.
- Around line 244-250: The handler is being written to eventRegistrations before
setEventOptions runs, causing a partial-write if setEventOptions fails; change
the None branch so you first call setEventOptions(~contractName, ~eventName,
~eventOptions, ~logger) to validate/raise, then re-fetch t from
eventRegistrations (to detect concurrent changes), and only after
reloading/confirming t is still None call set(~contractName, ~eventName, {...t,
handler: Some(newHandler)}) to store the handler; use the same identifiers (set,
setEventOptions, eventRegistrations, handler) so the update validates options
before mutating state and avoids leaving a stale handler on errors.
- Around line 191-195: The params argument to Logging.createChildFrom in
raiseDuplicateRegistration is a record literal; change it to a JS object literal
so payloads use objects per guidelines. Replace ~params={contractName,
eventName} with an object-style payload (e.g. ~params={"contractName":
contractName, "eventName": eventName}) in the raiseDuplicateRegistration
function before calling Logging.createChildFrom, and ensure the resulting value
still type-checks with Logging.createChildFrom and Logging.childError.
Call setEventOptions before set in the None branches of setHandler and setContractRegister. Re-fetch the record after setEventOptions so the handler write includes any freshly-set eventOptions. https://claude.ai/code/session_013sDgWWA3hVAoX6YVqJg1Ex
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
2 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.
Summary
Refactored the duplicate event registration error handling in
HandlerRegister.resto use a dedicated helper function instead of relying on exception types. This improves code maintainability and enables better error logging with structured context.Key Changes
DuplicateEventRegistrationexception type that was previously used for error handlingraiseDuplicateRegistrationfunction that:setEventOptions,setHandler, andsetContractRegisterwith calls to the new helper functionEventHandler.test.tsthat verifies duplicate handler and contractRegister registrations throw appropriate errors with correct messagesEventHandlers.tsto export caught errors for test assertionsLogTesting.resto referenceLogging.test.tsinstead ofLogging_test.tsImplementation Details
The new
raiseDuplicateRegistrationfunction consolidates error handling logic that was previously duplicated across three locations. It provides consistent error messaging and logging behavior while reducing code duplication. The function signature accepts the necessary context parameters (contractName, eventName, message, and logger) to construct meaningful error messages and logs.https://claude.ai/code/session_013sDgWWA3hVAoX6YVqJg1Ex
Summary by CodeRabbit
New Features
Tests
Bug Fixes