fix: resolve full code-review findings (fiscal correctness, parsers, generators, web)#183
Merged
Conversation
…tors, web Fiscal-correctness and robustness fixes from a whole-repo review. Engine: - Add year-parameterized savings-bracket module (tax-brackets.ts) as the single source of truth; fixes stale 28% top rate in web charts (2025+ is 30% per Ley 7/2024) and removes the divergent table previously duplicated in two places. - double-taxation: thread tax year, add per-country treaty-rate cap (default 15%) so excess foreign withholding is not over-credited (Art. 80 LIRPF). - wash-sale: clamp calendar-month arithmetic (no day overflow), apply the 1-year anti-churning window to all unlisted assets (no-ISIN + crypto), not crypto only. - ecb: validate CSV header / look up columns by name, guard against rate=0. - fifo: option exercise uses strike (not market price) for the share basis. - Skip crypto-denominated income (e.g. Kraken staking, paid in the coin) that has no ECB rate; warn the user to value it manually instead of crashing the report. Parsers: - csv-utils parseNumber: keep European comma-decimal semantics; only treat multi-comma values as thousands (avoids 1000x errors on broker prices). - trade-republic: carry money as Decimal-from-string, not lossy parseFloat. - scalable: detect ETFs as FUND via the instrument-type column. - kraken/coinbase: reclassify staking rewards out of the dividend pool. - binance: crypto carries empty ISIN (avoids false wash-sale homogeneity keys). Generators: - modelo720: round (half-up) instead of truncating cents; write the filer name in the holder field (positions 36-75), keeping the 500-byte record layout. - section-guide: interest box shows Casilla 0027 (was a stale 0033). Web: - charts: derive the tax-bracket card from the shared year-keyed bands. - profile: load from localStorage via a whitelist (prototype-pollution safe). - main: reject oversized uploads before parsing (DoS guard). Infra/docs: - nginx: add Content-Security-Policy and related security headers. - CLAUDE.md: correct the supported-broker count. - Add tsconfig.test.json + typecheck:tests so tests are type-checked in CI. All 1145 tests pass; src and test typechecks clean.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR consolidates Spanish tax-year-aware dividend/interest handling across the report and parsing pipeline. It introduces a reusable tax-brackets module, refactors double-taxation to cap treaty withholding, reclassifies staking rewards as interest in multiple brokers, hardens number/ECB parsing, and improves FIFO/wash-sale logic with month-aware windows. Updates translations and UI to reference the correct tax form field (casilla 0027 instead of 0033) and enforces file upload size limits on the web frontend. ChangesCore Tax & Report Engine
Parser Improvements
FIFO & Wash-Sale Engine
Modelo 720 & Output Precision
Web Frontend & Profile
Translations & Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/web/main.ts (1)
294-324:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the oversized-file warning when valid files already exist.
If the user already has pending files and then adds only oversized ones, Line 322 still calls
updateDetectionStatus(), which immediately overwrites the warning from Lines 294-303. Gate this onaccepted.length > 0, or keep the oversized warning in separate state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/web/main.ts` around lines 294 - 324, The oversized-file warning is being overwritten because updateDetectionStatus() runs even when the current drop contained only oversized files; change the final guard so detection is refreshed only when there were accepted files in this add or when there are pending files and no oversized files to display. Concretely, in the block after renderFileList() adjust the conditional that calls updateDetectionStatus() to require accepted.length > 0 OR (pendingFiles.length > 0 && oversized.length === 0) so the message created using oversized and the "detection-status" element is preserved; reference oversized, accepted, pendingFiles, and updateDetectionStatus() when locating the code to modify.src/web/charts.ts (1)
269-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep tax-bracket estimate math in
Decimal(avoidNumberdrift)In
src/web/charts.ts(displayBrackets +renderTaxBracketCard), the code convertsb.ratetonumberand performs the full bracket allocation,totalTax/netTax, andeffectiveRateusing JSNumber. This violates the monetary-math rule and can desync the chart from the engine. ChangerenderTaxBracketCardto acceptDecimalinputs (and/or usecalculateSavingsTax(income: Decimal, year)fromsrc/engine/tax-brackets.ts), keep all intermediate tax/rate/amount computations inDecimal, and only convert at the final formatting step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/web/charts.ts` around lines 269 - 316, The tax-bracket math currently converts b.rate to Number in displayBrackets and runs all bracket allocation and totals as JS Numbers in renderTaxBracketCard (variables rows, totalTax, netTax, effectiveRate), which risks precision drift; keep all rates, amounts and tax arithmetic in Decimal by returning Decimal rates from displayBrackets (or stop converting b.rate.toNumber()), perform bracket allocation using Decimal math inside renderTaxBracketCard (use Decimal arithmetic for remaining, width, amount, tax, totalTax, netTax, effectiveRate) and only convert to Number/string when formatting for output; alternatively call the engine helper calculateSavingsTax(income: Decimal, year) to compute per-bracket Decimals and derive rows for rendering so the chart stays in sync with the engine.src/parsers/trade-republic.ts (1)
87-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep sign/zero checks on Decimal, not Number.
src/parsers/trade-republic.tsnum()usesparseFloat(...)and returnsnumber, and the parser then branches on thoseNumbers:
if (shares === 0) continue;if (tax !== 0)withtax > 0 ? ... : ...if (amount > 0)- Refactor to derive
DecimalfromnumStr()and useisZero()/isPositive()for these control-flow checks.♻️ Proposed fix
-/** Numeric value of a column, for sign/zero comparisons only (never money). */ -function num(fields: string[], col: number): number { - const v = field(fields, col); - if (!v) return 0; - return parseFloat(parseNumber(v)) || 0; +/** Decimal value of a column for sign/zero comparisons. */ +function dec(fields: string[], col: number): Decimal { + return new Decimal(numStr(fields, col)); } @@ - const amount = num(fields, cols.amount); - const shares = num(fields, cols.shares); - const tax = num(fields, cols.tax); + const amount = dec(fields, cols.amount); + const shares = dec(fields, cols.shares); + const tax = dec(fields, cols.tax); @@ - if (shares === 0) continue; + if (shares.isZero()) continue; @@ - const absShares = new Decimal(sharesStr).abs().toString(); - const absAmount = new Decimal(amountStr).abs().toString(); + const absShares = shares.abs().toString(); + const absAmount = amount.abs().toString(); const absFee = new Decimal(feeStr).abs(); - const absTax = new Decimal(taxStr).abs(); + const absTax = tax.abs(); @@ - if (tax !== 0) { + if (!tax.isZero()) { @@ - amount: tax > 0 ? new Decimal(taxStr).abs().neg().toString() : taxStr, + amount: tax.isPositive() ? tax.abs().neg().toString() : taxStr, @@ - if (amount > 0) { + if (amount.isPositive()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parsers/trade-republic.ts` around lines 87 - 92, The num() helper currently returns a JavaScript number via parseFloat, which breaks the requirement to do sign/zero checks using Decimal; change num() to return a Decimal instead: inside num(fields, col) call the existing numStr(fields, col) (or extract the string via field/parseNumber), construct a new Decimal(...) from that string (return Decimal(0) when empty/invalid), and then update downstream control-flow to use Decimal methods (isZero(), isPositive(), and decimal.comparedTo(0) where needed) instead of numeric comparisons like === 0, > 0 or !== 0; keep function name num() and usages intact so callers get a Decimal object for sign/zero checks.src/parsers/scalable.ts (1)
11-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep Scalable monetary fields on
Decimalend-to-end.
src/parsers/scalable.tsstill converts parsedshares/fee(and also withholdingtax) throughparseFloat/Math.abs, then interpolates JSnumbervalues into emittedTrade.quantity/Trade.commission/withholdingcashTransactions.amount, reintroducing binary rounding. Keep these values asDecimaluntil you emit strings.Proposed fix
+import Decimal from "decimal.js"; import type { BrokerParser, Statement } from "../types/broker.js"; import type { Trade, CashTransaction } from "../types/ibkr.js"; import { parseCsvLine, parseNumber, @@ - const sharesNum = parseFloat(parseNumber(shares)); - if (sharesNum === 0) continue; + const sharesQty = new Decimal(shares); + if (sharesQty.isZero()) continue; @@ - const absShares = Math.abs(sharesNum); - const feeNum = parseFloat(parseNumber(fee)); + const absShares = sharesQty.abs(); + const feeAmount = new Decimal(fee); @@ - quantity: isSell ? `-${absShares}` : `${absShares}`, + quantity: isSell ? absShares.neg().toString() : absShares.toString(), @@ - commission: feeNum !== 0 ? `-${Math.abs(feeNum)}` : "0", + commission: feeAmount.isZero() ? "0" : feeAmount.abs().neg().toString(),Also applies to: 140-155 (withholding
taxNumblock), in addition to 159-197 (trade block).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parsers/scalable.ts` around lines 11 - 19, The parser currently converts Scalable monetary fields (shares, fee, withholding tax / taxNum) to native JS numbers via parseFloat/Math.abs before populating Trade.quantity, Trade.commission and cashTransactions.amount, which reintroduces binary rounding; change the logic in the taxNum block and the trade block so you keep parsed values as Decimal instances (use Decimal.abs/Decimal methods instead of Math.abs/parseFloat) throughout processing and only serialize to strings when assigning to the output fields (e.g., set Trade.quantity = decimalQuantity.toString(), Trade.commission = decimalFee.toString(), cashTransactions.amount = decimalTax.toString()); update any intermediate variables referenced in these blocks (shares, fee, tax, taxNum, and where Trade is constructed) to be Decimal types and remove parseFloat/Math.abs usage.
🧹 Nitpick comments (4)
tests/engine/double-taxation.test.ts (1)
23-154: ⚡ Quick winAdd one test for the
totalSavingsBasebranch.Every case here calls
calculateDoubleTaxation(entries, YEAR)without the third arg, so the effective-average-rate path used fromsrc/generators/report.tsis still untested. A regression there would change Casilla 0588 while this suite stays green.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/engine/double-taxation.test.ts` around lines 23 - 154, Tests never exercise the "totalSavingsBase" / effective-average-rate branch because calculateDoubleTaxation is always called with two args; add one test that calls calculateDoubleTaxation(entries, YEAR, someTotalSavingsBase) to trigger the alternate code path (use the existing makeEntry helper to construct entries and reuse YEAR), assert expected totals and per-country deductionAllowed/taxPaid for that scenario, and place it alongside the other "it" cases to ensure the branch in calculateDoubleTaxation (and the logic in src/generators/report.ts that reads totalSavingsBase) is covered.AGENTS.md (2)
104-111: 💤 Low valueOptional: Add language specification to checklist code block.
Same as SKILL.md: the session closing checklist code block should specify a language for consistent rendering.
📝 Optional fix
-``` +```text [ ] 1. git add + git commit [ ] 2. git push [ ] 3. gh pr checks <PR> --watch 2>&1 (IMPORTANT: WAIT for final summary, do NOT tell user it is done until you confirm it passes CI!)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 104 - 111, The session closing checklist code block in AGENTS.md lacks a language specifier causing inconsistent rendering; update the fenced code block surrounding the checklist (the "session closing checklist code block") to include a language tag such as "text" (matching SKILL.md) so the fence starts with ```text and the closing fence remains ```, ensuring consistent formatting across docs.
255-272: ⚖️ Poor tradeoffOptional: Fill in project-specific sections.
The placeholder sections (Build & Test, Architecture Overview, Conventions & Patterns) are clearly marked but could be populated with DeclaRenta-specific details if needed for agent context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 255 - 272, Populate the placeholder sections in AGENTS.md: replace the "Build & Test" block with DeclaRenta's actual build and test commands (install, build, test, CI steps), fill "Architecture Overview" with a concise diagram/description of key components and data flow for DeclaRenta (services, databases, agents), and document "Conventions & Patterns" with your repo's coding standards, branching/PR rules, and naming/typing patterns so agents can rely on them; update the exact headings "Build & Test", "Architecture Overview", and "Conventions & Patterns" in AGENTS.md accordingly..claude/skills/tbd/SKILL.md (1)
111-118: 💤 Low valueOptional: Add language specification to checklist code block.
The fenced code block containing the session closing checklist has no language specified. Adding a language identifier (e.g.,
textormarkdown) would improve rendering consistency.📝 Optional fix
-``` +```text [ ] 1. git add + git commit [ ] 2. git push [ ] 3. gh pr checks <PR> --watch 2>&1 (IMPORTANT: WAIT for final summary, do NOT tell user it is done until you confirm it passes CI!)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/tbd/SKILL.md around lines 111 - 118, The fenced checklist code block in SKILL.md lacks a language identifier; update the triple-backtick fence that encloses the session closing checklist to include a language specifier like `text` or `markdown` (e.g., change ``` to ```text) so the block renders consistently—locate the checklist block in .claude/skills/tbd/SKILL.md and add the language tag to the opening fence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/hooks/tbd-closing-reminder.sh:
- Around line 5-12: The hook currently uses jq to set command from input
(command=$(echo "$input" | jq -r '.tool_input.command // empty')), but jq may
not be installed; either ensure jq is provisioned in the session bootstrap or
make the hook robust: replace the jq usage with a fallback JSON extractor (e.g.,
try jq first and if missing use Python: command=$(echo "$input" | python3 -c
'import sys,json;
print(json.load(sys.stdin).get("tool_input",{}).get("command",""))') or
similar), and keep the subsequent git push detection logic (the checks against
"$command" and the tbd closing invocation) unchanged so the hook still triggers
when jq is absent.
In @.claude/scripts/tbd-session.sh:
- Around line 28-46: The bootstrap installs a floating get-tbd version; change
.claude/scripts/tbd-session.sh to read the pinned version from .tbd/config.yml
(tbd_version) into a shell variable (e.g., TBD_VERSION) and use that when
installing: append @${TBD_VERSION} to every package install invocation (npm
install -g get-tbd@${TBD_VERSION}, npm install --prefix ~/.local
get-tbd@${TBD_VERSION}, pnpm add -g get-tbd@${TBD_VERSION}, yarn global add
get-tbd@${TBD_VERSION}) and keep the existing local-install symlink logic
pointing to ~/.local/node_modules/.bin/tbd; ensure the script fails with a clear
error if tbd_version is missing or cannot be parsed.
In `@src/engine/ecb.ts`:
- Around line 102-103: The expression that computes hasDataRows uses unnecessary
optional chaining on l (lines.slice(1).some((l) => l?.trim())), but lines come
from csv.split("\n") so l is always a string; replace the predicate to call
trim() directly (e.g., (l) => l.trim() or Boolean(l.trim())) to satisfy the
no-unnecessary-condition lint rule and keep the same semantics; update the
hasDataRows calculation accordingly in the code that defines hasDataRows.
In `@src/generators/modelo720.ts`:
- Around line 220-225: After rounding the absolute Decimal `dec` to `decLen`
with Decimal.ROUND_HALF_UP, detect if the integer portion overflows its fixed
width: compute the integer string (e.g., from `dec.floor().toString()` or
`intPart` before padding) and if its length exceeds `intLen`, fail fast (throw
an Error or return a validation failure) instead of silently truncating/padding;
do this check prior to creating `intPart`/`fracPart` and ensure the thrown
message includes the offending `value`, `intLen`, and `decLen` for debugging.
In `@src/generators/report.ts`:
- Around line 176-185: The warning about crypto-denominated unresolvableInterest
is only being pushed into allMessages, so callers that read summary.warnings
won't see it; update the block that checks unresolvableInterest > 0 to also push
the same warning object into summary.warnings (or the warnings array that
populates summary.warnings) in addition to allMessages, referencing the existing
symbols unresolvableInterest, allMessages and summary.warnings (or the local
warnings accumulator) so both message lists are kept in sync.
In `@src/i18n/locales/es.ts`:
- Line 470: Summary: The coding guideline text still lists interest income as
casilla 0033 but should be updated to 0027. Fix: find the guideline string
"Casilla values: 0327-0328 (capital gains), 0029 (dividends), 0033 (interest
income), ..." in the documentation/generator content and change "0033 (interest
income)" to "0027 (interest income)"; ensure any nearby references or examples
that mention 0033 are updated consistently and add a short note or bead entry
indicating the guideline correction for future tracking.
In `@src/web/profile.ts`:
- Line 53: Replace the current non-integer-safe check for the year field by
validating that parsed.year is an integer: change the condition that sets
profile.year (currently "typeof parsed.year === 'number' &&
Number.isFinite(parsed.year)") to use Number.isInteger(parsed.year) so only
whole-year values are accepted when assigning profile.year.
In `@tests/engine/ecb-fetch.test.ts`:
- Around line 204-215: The test converts ECB rate strings with parseFloat and
compares with a hardcoded float which is precision-sensitive; update the
assertion to use Decimal for both the parsed rate and the expected reciprocal
(e.g., use Decimal to wrap rates.get("2025-01-02")!.get("USD")! and compute
Decimal(1).div(Decimal("1.035"))) and compare using Decimal's comparison or
toNumber with toFixed as appropriate. Modify the test in ec b-fetch.test.ts
around the fetchEcbRates(...) call and the rate assertion to import and use the
project's Decimal class (or decimal.js) instead of parseFloat and numeric
literals so the equality uses high-precision Decimal operations. Ensure you
reference fetchEcbRates and the rates Map key lookup when making the
replacement.
---
Outside diff comments:
In `@src/parsers/scalable.ts`:
- Around line 11-19: The parser currently converts Scalable monetary fields
(shares, fee, withholding tax / taxNum) to native JS numbers via
parseFloat/Math.abs before populating Trade.quantity, Trade.commission and
cashTransactions.amount, which reintroduces binary rounding; change the logic in
the taxNum block and the trade block so you keep parsed values as Decimal
instances (use Decimal.abs/Decimal methods instead of Math.abs/parseFloat)
throughout processing and only serialize to strings when assigning to the output
fields (e.g., set Trade.quantity = decimalQuantity.toString(), Trade.commission
= decimalFee.toString(), cashTransactions.amount = decimalTax.toString());
update any intermediate variables referenced in these blocks (shares, fee, tax,
taxNum, and where Trade is constructed) to be Decimal types and remove
parseFloat/Math.abs usage.
In `@src/parsers/trade-republic.ts`:
- Around line 87-92: The num() helper currently returns a JavaScript number via
parseFloat, which breaks the requirement to do sign/zero checks using Decimal;
change num() to return a Decimal instead: inside num(fields, col) call the
existing numStr(fields, col) (or extract the string via field/parseNumber),
construct a new Decimal(...) from that string (return Decimal(0) when
empty/invalid), and then update downstream control-flow to use Decimal methods
(isZero(), isPositive(), and decimal.comparedTo(0) where needed) instead of
numeric comparisons like === 0, > 0 or !== 0; keep function name num() and
usages intact so callers get a Decimal object for sign/zero checks.
In `@src/web/charts.ts`:
- Around line 269-316: The tax-bracket math currently converts b.rate to Number
in displayBrackets and runs all bracket allocation and totals as JS Numbers in
renderTaxBracketCard (variables rows, totalTax, netTax, effectiveRate), which
risks precision drift; keep all rates, amounts and tax arithmetic in Decimal by
returning Decimal rates from displayBrackets (or stop converting
b.rate.toNumber()), perform bracket allocation using Decimal math inside
renderTaxBracketCard (use Decimal arithmetic for remaining, width, amount, tax,
totalTax, netTax, effectiveRate) and only convert to Number/string when
formatting for output; alternatively call the engine helper
calculateSavingsTax(income: Decimal, year) to compute per-bracket Decimals and
derive rows for rendering so the chart stays in sync with the engine.
In `@src/web/main.ts`:
- Around line 294-324: The oversized-file warning is being overwritten because
updateDetectionStatus() runs even when the current drop contained only oversized
files; change the final guard so detection is refreshed only when there were
accepted files in this add or when there are pending files and no oversized
files to display. Concretely, in the block after renderFileList() adjust the
conditional that calls updateDetectionStatus() to require accepted.length > 0 OR
(pendingFiles.length > 0 && oversized.length === 0) so the message created using
oversized and the "detection-status" element is preserved; reference oversized,
accepted, pendingFiles, and updateDetectionStatus() when locating the code to
modify.
---
Nitpick comments:
In @.claude/skills/tbd/SKILL.md:
- Around line 111-118: The fenced checklist code block in SKILL.md lacks a
language identifier; update the triple-backtick fence that encloses the session
closing checklist to include a language specifier like `text` or `markdown`
(e.g., change ``` to ```text) so the block renders consistently—locate the
checklist block in .claude/skills/tbd/SKILL.md and add the language tag to the
opening fence.
In `@AGENTS.md`:
- Around line 104-111: The session closing checklist code block in AGENTS.md
lacks a language specifier causing inconsistent rendering; update the fenced
code block surrounding the checklist (the "session closing checklist code
block") to include a language tag such as "text" (matching SKILL.md) so the
fence starts with ```text and the closing fence remains ```, ensuring consistent
formatting across docs.
- Around line 255-272: Populate the placeholder sections in AGENTS.md: replace
the "Build & Test" block with DeclaRenta's actual build and test commands
(install, build, test, CI steps), fill "Architecture Overview" with a concise
diagram/description of key components and data flow for DeclaRenta (services,
databases, agents), and document "Conventions & Patterns" with your repo's
coding standards, branching/PR rules, and naming/typing patterns so agents can
rely on them; update the exact headings "Build & Test", "Architecture Overview",
and "Conventions & Patterns" in AGENTS.md accordingly.
In `@tests/engine/double-taxation.test.ts`:
- Around line 23-154: Tests never exercise the "totalSavingsBase" /
effective-average-rate branch because calculateDoubleTaxation is always called
with two args; add one test that calls calculateDoubleTaxation(entries, YEAR,
someTotalSavingsBase) to trigger the alternate code path (use the existing
makeEntry helper to construct entries and reuse YEAR), assert expected totals
and per-country deductionAllowed/taxPaid for that scenario, and place it
alongside the other "it" cases to ensure the branch in calculateDoubleTaxation
(and the logic in src/generators/report.ts that reads totalSavingsBase) is
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69c8a2f7-1feb-4035-9077-b4203dffe703
📒 Files selected for processing (54)
.claude/.gitignore.claude/hooks/tbd-closing-reminder.sh.claude/scripts/ensure-gh-cli.sh.claude/scripts/tbd-session.sh.claude/settings.json.claude/skills/tbd/SKILL.md.tbd/.gitattributes.tbd/.gitignore.tbd/config.ymlAGENTS.mdCLAUDE.mdnginx.confpackage.jsonsrc/engine/double-taxation.tssrc/engine/ecb.tssrc/engine/fifo.tssrc/engine/fx-fifo.tssrc/engine/tax-brackets.tssrc/engine/wash-sale.tssrc/generators/modelo720.tssrc/generators/report.tssrc/i18n/locales/ca.tssrc/i18n/locales/en.tssrc/i18n/locales/es.tssrc/i18n/locales/eu.tssrc/i18n/locales/gl.tssrc/parsers/binance.tssrc/parsers/coinbase.tssrc/parsers/csv-utils.tssrc/parsers/kraken.tssrc/parsers/scalable.tssrc/parsers/trade-republic.tssrc/web/charts.tssrc/web/main.tssrc/web/profile.tssrc/web/section-guide.tstests/engine/double-taxation.test.tstests/engine/ecb-fetch.test.tstests/engine/fifo.test.tstests/engine/options-v0137-23.test.tstests/engine/tax-brackets.test.tstests/engine/wash-sale.test.tstests/generators/modelo720.test.tstests/generators/report.test.tstests/parsers/binance.test.tstests/parsers/coinbase-branches.test.tstests/parsers/coinbase.test.tstests/parsers/csv-utils.test.tstests/parsers/etoro-xlsx.test.tstests/parsers/fixtures.test.tstests/parsers/kraken-branches.test.tstests/parsers/kraken.test.tstests/parsers/scalable.test.tstsconfig.test.json
- ecb.ts: drop unnecessary optional chain (CI eslint failure) - modelo720.ts: throw on integer-field overflow instead of silently widening the 500-byte record - report.ts: surface crypto-income warning on legacy warnings[] too - profile.ts: validate stored year is an integer, not just finite - tests: use Decimal instead of parseFloat for ECB rate assertions; cover the modelo720 overflow guard - remove local agent tooling (.claude/, .tbd/, AGENTS.md) wrongly committed; gitignore them - CLAUDE.md: interest income casilla 0033 -> 0027
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
==========================================
+ Coverage 97.57% 97.60% +0.03%
==========================================
Files 39 40 +1
Lines 8408 8519 +111
Branches 1719 1750 +31
==========================================
+ Hits 8204 8315 +111
Misses 204 204
🚀 New features to boost your workflow:
|
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
1 participant
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
Implements every finding from the whole-repo deep review (critical → nit). Focus was fiscal/legal correctness for Spanish IRPF, with parser robustness, AEAT format fixes, web hardening, and infra. Decomposed and executed in parallel, then verified by independent code-review / test / architecture passes.
Fiscal correctness (engine)
src/engine/tax-brackets.tsis the single source of truth. Fixes the stale 28% top rate in the web tax-estimate card (2025+ is 30% per Ley 7/2024). Removes the table that was duplicated and divergent acrosscharts.tsanddouble-taxation.ts.Parsers
parseNumberkeeps European comma-decimal semantics; only multi-comma values are treated as thousands (avoids 1000× errors on broker prices like2,082).Decimal-from-string (no lossyparseFloat).FUND; Binance crypto carries empty ISIN (no false wash-sale keys); Kraken/Coinbase staking moved out of the dividend pool.AEAT generators
numPadrounds half-up instead of truncating cents; the holder field (pos 36-75) now carries the filer name, not the security description. 500-byte ISO-8859-15 layout preserved.Web / security
localStoragevia a key whitelist (prototype-pollution safe).Infra / docs
Content-Security-Policy+ related security headers.CLAUDE.md: corrected supported-broker count.tsconfig.test.json+typecheck:testsso tests are type-checked in CI.Test plan
npx vitest run— 1145 passing (55 files), incl. new regression tests for every fixnpx tsc --noEmit -p tsconfig.json— cleannpx tsc --noEmit -p tsconfig.test.json— clean (tests now type-checked)npm run build— web bundle buildsSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests & Tooling