fix: deep-merge range fields during alert update#297
Conversation
…min/max The shallow merge in the update handler replaced entire range objects (e.g. inflow_1h, usdValue), so updating only --inflow-1h-min would drop the existing max to null. Now all nested plain objects (ranges, inclusion, exclusion) are deep-merged, and null sub-keys from buildRange are preserved from the existing value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pr-reviewer Summary✅ No issues found The code review completed successfully with no findings. Review effort: 2/5 (Simple) SummaryThis PR fixes a real bug where partially updating a range field (e.g., The fix is correct and well-reasoned: after the initial shallow merge, it iterates only the keys that Two targeted tests are added covering the two alert types most likely to exercise range fields ( Token usage: 2,244 input, 4,779 output, 110,257 cache read, 17,509 cache write |
src/commands/alerts.js
Outdated
| if (m[subKey] === null && oldVal[subKey] != null) { | ||
| m[subKey] = oldVal[subKey]; | ||
| } |
There was a problem hiding this comment.
Just something worth noting: This does prevent accidentally dropping the sibling bound but it also makes it impossible to explicitly clear a range bound. The API seems to accept null as a value to not include it in the range bound so I'm thinking we need to change when to apply the options.data to after the deep-merge loop, so it lands directly into params.data without being touched by the null-preservation logic.
- buildRange now omits keys for unsupplied values instead of setting null, eliminating the ambiguous null sentinel that caused the original bug. - Replace one-level merge loop with recursive deepMergePlain(), which correctly handles ranges nested inside inclusion (marketCap, fdvUsd, tokenAge) — previously these were still silently dropped. - Add test cases for inclusion-nested marketCap and fdvUsd merges. - Update existing tests to expect sparse range objects.
b6ace4b to
e4b044e
Compare
The change in the original PR (f524acc) to always set subjects: [] broke the update path: updating without --subject now wipes existing subjects. Restore the conditional assignment (if (subjects) data.subjects = subjects;) from main branch. This preserves the deep-merge behavior while keeping the create path intact (TYPE_DEFAULTS still applies subjects: []). Also restore the omitted test 'should not wipe subjects when updating common-token-transfer without --subject' with updated assertion for sparse range format (usdValue: { min: 5000 } not { min: 5000, max: null }).
ee006b1 to
0dcba3b
Compare
Summary
Original issue: Updating an alert with only
--inflow-1h-minwould drop the siblingmaxbecause the update handler did a shallow merge that replaced entire range objects.Initial fix (commit f524acc): Special-case merge logic for
inclusionandexclusionobjects to preserve siblings.Improvement (commit 3721c0a): Generalized solution with two key changes:
buildRangenow omits unsupplied ends instead of setting them tonull. Eliminates the ambiguous null sentinel that caused the bug.deepMergePlain(), which recursively deep-merges nested plain objects at any depth. This fixes the bug for ranges nested insideinclusion(likemarketCap,fdvUsd,tokenAge), which the initial shallow merge still broke.Scope
inflow_*,outflow_*,netflow_*,usdValue,tokenAmountinclusion:marketCap,fdvUsd,tokenAgeTest Coverage