refactor(trading-signals): share signal machinery via object-result base and fix BBANDS warm-up#1182
Open
bennycode wants to merge 1 commit into
Open
refactor(trading-signals): share signal machinery via object-result base and fix BBANDS warm-up#1182bennycode wants to merge 1 commit into
bennycode wants to merge 1 commit into
Conversation
…ase and fix BBANDS warm-up
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors the trading-signals package to share common “object-shaped result” signal machinery across indicators, while also correcting a Bollinger Bands (BBANDS) warm-up off-by-one so the first computed bands align with the interval’s first full window.
Changes:
- Fix BBANDS warm-up to emit the first bands result at
intervalinputs (instead ofinterval + 1) and update BBANDS/BBW tests to assert the correct warm-up offset. - Introduce
TrendTechnicalIndicator<Result, Input, SignalState>as a generic base for composite/object results, providing previous-result caching and{state, hasChanged}signal reporting. - Migrate
MACDandBollingerBandsonto the new base, removing duplicated per-indicator previous-result + signal-change bookkeeping.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/trading-signals/src/types/Indicator.ts | Adds TrendTechnicalIndicator base class for object-result indicators with shared signal-change tracking. |
| packages/trading-signals/src/volatility/BBANDS/BollingerBands.ts | Fixes warm-up logic (compute at prices.length === interval) and adopts TrendTechnicalIndicator for signal machinery. |
| packages/trading-signals/src/momentum/MACD/MACD.ts | Refactors MACD to extend TrendTechnicalIndicator, removing local previous-result/signal bookkeeping. |
| packages/trading-signals/src/volatility/BBANDS/BollingerBands.test.ts | Strengthens tests around BBANDS warm-up offset, SMA alignment, replace behavior, and signal semantics. |
| packages/trading-signals/src/volatility/BBW/BollingerBandsWidth.test.ts | Adjusts BBW expectations/offset to reflect BBANDS producing one earlier result. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
BBANDS warm-up off-by-one: confirmed and fixed
BollingerBandsonly computed a result whenpushUpdatereturned adropOut, which first happens at the (interval+1)-th input — whilegetRequiredInputs()returnsintervaland the SMA that defines the middle band stabilizes at exactlyintervalinputs. Verification against reference data:src/fixtures/BB/data.json, interval 20): the first non-null reference value sits at index 19 (the 20th input) —middle[19] = 74.3, which equals the mean ofprices[0..19]. The old implementation emitted its first result at index 20.BollingerBands.test.ts, interval 5): reference values start at index 4 — lower80.53/ middle82.43/ upper84.32, which match the bands computed from the first five inputs (81.59, 81.06, 82.87, 83.0, 83.61; mean82.426).if (bb.isStable), so the reference values at the first index were silently skipped.BollingerBandsnow computes from the full window onceprices.length === interval, makinggetRequiredInputs()accurate. The tests assert the exact warm-up offset (getRequiredInputs() - 1), and a new test proves the middle band stabilizes in lockstep with anSMAof the same interval.BollingerBandsWidth(built on BBANDS) gains one earlier result accordingly; its new first value at index 19 (0.19, windowcloses[0..19]) is consistent with the TradingView-verified series.Generic object-result indicator base
New
TrendTechnicalIndicator<Result, Input, SignalState>insrc/types/Indicator.tsmirrorsTrendIndicatorSeriesbut is generic over the result type:previousResultcaching viasetResult(), abstractcalculateSignalState(), andgetSignal()returning{state, hasChanged}. It is exported through the existingtypesbarrel.MACDandBollingerBandsnow extend it; their hand-rolled#previousResult/#twoPreviousResultbookkeeping and duplicatedcalculateSignal/getSignalmethods are deleted.MACDResult/BandsResultshapes, and thegetSignal()return shape{state, hasChanged}are all stable.replace()behavior is preserved;BollingerBandskeeps a two-level price history so signal states are re-derived from the correct (result, price) pairing on replace. A strict bidirectional replace test for BBANDS results was added.Behavior note: BBANDS signal semantics
BollingerBands.getSignal()previously compared the latest price against the bands from before that price entered the window. It now compares the latest price against the current bands (standard %B semantics, and the same comparison downstream code likeMultiIndicatorConfluenceStrategyperforms manually). All existing signal test scenarios produce identical states.Follow-ups (out of scope)
StochasticOscillatorandAccelerationBandsstill hand-roll the same signal machinery (calculateSignal+getSignal); ABANDS also computes only ondropOutand likely has the same warm-up off-by-one. Both are candidates for a follow-up migration ontoTrendTechnicalIndicator.Test plan
npm testinpackages/trading-signals: 63 files / 455 tests pass, coverage 100% on all metrics (statements 1259/1259, branches 690/690, functions 232/232, lines 1235/1235)npm run lintinpackages/trading-signalspassesnpx lerna run dist --scope trading-strategies --include-dependenciessucceeds; additionally rebuilt all workspace packages via npm workspaces and ran thetrading-strategiessuite: 22 files / 248 tests pass