feat: complete Phases 2-4 — crypto parsers, i18n, charts, PWA, WCAG#18
Merged
Conversation
Add previousYearIsins parameter to generateD6Report() to generate cancellation entries for ISINs no longer held. The AFORIX guide now includes a CANCELACIONES section. CLI gains --previous-d6 option.
Add exported checkModelo720Thresholds() for per-category (values/ accounts/realEstate) threshold evaluation. STK positions now use Q4 average ECB rate; FUND/BOND keep Dec 31 spot rate per BOE rules.
New function averages daily ECB rates from Oct 1 to Dec 31 for Modelo 720 STK positions. Returns Decimal(1) for EUR. Throws if no Q4 rates found for the requested currency.
New validateModelo720Records() validates 500-char records: register type, model 720, NIF format, numeric fields, ISO country codes, ISIN Luhn checksum, and A/M/C declaration types. Wired into CLI output.
FIFO engine already handles cross-currency correctly (USD stock from GBP account). Added tests for: USD/GBP cross-currency, GBP-denominated stocks, and FX-only gain/loss isolation.
Export getQ4AverageRate, checkModelo720Thresholds, and validateModelo720Records from the main index.ts barrel file.
Phase 2 (Multi-broker): - Add Coinbase, Binance, Kraken CSV parsers with auto-detection - Register all 8 brokers in parser registry Phase 3 (Tax models): - 720 threshold detection, Q4 average FX rate, BOE validation - D-6 negative declaration support - Crypto parsers enable Modelo 721 Phase 4 (Web UI): - i18n system: 5 locales (es, en, ca, eu, gl) with language selector - SVG charts: asset distribution, monthly G/P, currency, withholdings - Legal disclaimer modal (native <dialog>) - PWA: Service Worker (cache-first static, network-first API) + manifest - WCAG 2.1 AA: skip-link, focus-visible, ARIA roles, keyboard nav - GitHub Pages deploy workflow 407 tests passing, lint clean, typecheck clean.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 16 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds i18n (5 locales), three crypto CSV parsers (Coinbase, Binance, Kraken), Modelo 720 validator and threshold logic (Q4 averaging), D-6 cancellation support, SVG charting and disclaimer UI, PWA/Service Worker and manifest, GitHub Pages deploy workflow, related tests and config updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
==========================================
- Coverage 75.82% 75.54% -0.29%
==========================================
Files 28 41 +13
Lines 3144 4678 +1534
Branches 630 863 +233
==========================================
+ Hits 2384 3534 +1150
- Misses 758 1142 +384
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/index.ts (1)
349-355:⚠️ Potential issue | 🔴 CriticalDon't suppress negative D-6 filings when only cancellations remain.
With
--previous-d6, a valid declaración negativa can havereport.positions.length === 0andreport.cancelled.length > 0. This branch exits early and tells the user no filing is needed, so the new cancellation workflow never produces output.Suggested fix
- if (report.positions.length === 0) { + if (report.positions.length === 0 && report.cancelled.length === 0) { console.error("No se encontraron posiciones extranjeras. No es necesario presentar D-6."); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/index.ts` around lines 349 - 355, The early-return currently checks only report.positions and skips output even when there are cancellations from a previous D-6; update the condition so we only skip when there are no positions and no cancellations relevant to the --previous-d6 flow. Replace the existing if (report.positions.length === 0) check with a combined condition that returns only when report.positions.length === 0 AND (opts.previousD6 is falsey OR report.cancelled.length === 0); reference fetchEcbRates, generateD6Report and the report.positions / report.cancelled fields so the cancellation workflow runs when opts.previousD6 is enabled and report.cancelled has entries.
🧹 Nitpick comments (3)
src/i18n/index.ts (1)
86-90: Low-risk ReDoS concern with dynamic regex.The static analysis tool flagged the dynamic
RegExpconstruction. Sincekcomes from param keys (developer-controlled, not user input), the risk is minimal in practice. However, for defense-in-depth, consider escaping or using string replacement.Option: Use string split/join instead of regex
if (params) { for (const [k, v] of Object.entries(params)) { - text = text.replace(new RegExp(`\\{\\{${k}\\}\\}`, "g"), v); + text = text.split(`{{${k}}}`).join(v); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/index.ts` around lines 86 - 90, The dynamic RegExp constructed in the params replacement loop (for (const [k, v] of Object.entries(params)) { text = text.replace(new RegExp(`\\{\\{${k}\\}\\}`, "g"), v); }) can pose a ReDoS risk; change the implementation to avoid building a regex from the key: either escape the key before passing to RegExp or replace the replace(RegExp...) call with a safe string-based approach such as text = text.split(`{{${k}}}`).join(String(v)); ensuring you still coerce v to string and keep the same behavior inside the same params handling block/function.src/parsers/kraken.ts (1)
229-229: Redundant ternary returns same value for both branches.fxRateToBase: quote === "EUR" ? "1" : "1",Both branches return
"1". Either simplify to just"1"or implement proper FX rate handling for non-EUR currencies if expected at parse time.Simplify
- fxRateToBase: quote === "EUR" ? "1" : "1", + fxRateToBase: "1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parsers/kraken.ts` at line 229, The ternary setting fxRateToBase currently returns the same value for both branches, so replace the redundant expression with a single constant value (fxRateToBase: "1") or, if non-EUR quotes must be supported, implement proper FX lookup logic using the quote variable (e.g., call a fx-rate helper or fetch from the FX service) and assign the computed rate to fxRateToBase; update the parser function in kraken.ts where fxRateToBase and quote are used to reflect the chosen approach.src/web/charts.ts (1)
213-213: Hardcoded Spanish month abbreviations bypass i18n.const MONTHS = ["Ene", "Feb", "Mar", "Abr", "May", "Jun", "Jul", "Ago", "Sep", "Oct", "Nov", "Dic"];Consider using translation keys for month labels to maintain consistency with the i18n system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/charts.ts` at line 213, The MONTHS constant currently hardcodes Spanish abbreviations and should be replaced with a locale-aware source; remove the hardcoded const MONTHS and generate month labels via the app i18n or Intl.DateTimeFormat instead (e.g., map 0..11 to i18n.t('months.short.jan').. or use new Intl.DateTimeFormat(locale, { month: 'short' }).format), and update any code referencing MONTHS to use that generated array so labels respect the active locale (search for MONTHS in src/web/charts.ts to update all usages).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Around line 250-252: The checklist item wording implies the ECB supplies
quoted-share valuations; update the text so ECB is only cited as the FX
conversion source and the "media del último trimestre" remains the price basis —
e.g., change "Acciones cotizadas: media del último trimestre (Q4 average via
ECB)" to something like "Acciones cotizadas: media del último trimestre (price
source); convertir a EUR usando tipos de cambio del BCE" so the ECB is clearly
referenced only for currency conversion, not for the share price itself.
In `@src/cli/index.ts`:
- Around line 288-301: The CLI currently logs BOE format errors but continues,
allowing a malformed Modelo 720 file to be written; after computing
validationResults/invalidRecords (from validateModelo720Records and output720),
change the flow so that when invalidRecords.length > 0 you exit with a non-zero
status (e.g., call process.exit(1) or return a failing code) immediately after
logging the errors and before any code that writes or returns output720,
ensuring the command fails and no invalid file is written.
In `@src/generators/modelo720.ts`:
- Around line 37-63: checkModelo720Thresholds uses year-end spot rates while
generateModelo720 uses a Q4-average for STK, causing inconsistent exceedance
decisions and the current catch swallows all errors; introduce a shared helper
(e.g., getValuationRate(rateMap, year, currency, assetCategory) or
getEcbValuationRate) that encapsulates the valuation logic used by
generateModelo720 (Q4-average for assetCategory "STK", year-end spot for
others), update checkModelo720Thresholds to call that helper instead of
getEcbRate so both paths use the same rates, and tighten error handling in that
helper to throw a specific MissingRateError (or similar) for missing rates so
callers only catch that specific error and rethrow any other exceptions (update
checkModelo720Thresholds and generateModelo720 to catch MissingRateError if
needed).
In `@src/parsers/binance.ts`:
- Around line 130-154: The header validation only checks
cols.date/cols.pair/cols.side/cols.price but not cols.executed or cols.amount,
which can cause zero-value trades; update the header guard to also assert
cols.executed >= 0 and cols.amount >= 0 (the same way the existing check is
done) and throw the same Error message if either is missing, and ensure the
parsing loop that uses fields[cols.executed] and fields[cols.amount] continues
to rely on those guaranteed indices (refer to the cols object and the loop that
creates executed and amount Decimal values, as well as parseCsvLine,
convertBinanceDate, and parsePair to locate the relevant code).
- Around line 60-85: parsePair currently guesses a 3-char quote when no known
quote matches, which mis-parses Binance pairs (e.g., BTCFDUSD); update behavior
to fail closed: remove the 3-character fallback and instead make parsePair (and
its callers) reject unknown quotes by throwing a clear error (or returning a
defined failure value) when no entry in KNOWN_QUOTES matches; additionally,
expand KNOWN_QUOTES to include any Binance-specific quote tokens you need to
support (update the KNOWN_QUOTES array) so legitimate pairs are recognized—refer
to the KNOWN_QUOTES constant and the parsePair function when making these
changes.
In `@src/parsers/coinbase.ts`:
- Around line 145-146: The code is using parseFloat in several places (e.g., the
local variables quantityNum/feeNum near where quantity and fees are parsed,
destination quantity/subtotal around lines handling destination parsing, and
later quantity/fees parsing) instead of the project-standard parseNumber from
csv-utils which wraps Decimal.js; replace all parseFloat(...) usages with
parseNumber(...) and ensure you preserve Math.abs(...) where used (e.g.,
quantityNum = Math.abs(parseNumber(quantity))). Update the destination parsing
and the later quantity/fees assignments to call parseNumber for every
string-to-number conversion so all monetary values use Decimal-backed parsing
consistently.
In `@src/parsers/kraken.ts`:
- Around line 209-211: volNum, costNum and feeNum are created with direct
parseFloat calls (lines around the volNum/costNum/feeNum declarations and again
at lines ~286-287), violating the guideline to route all string→number
conversions through Decimal.js; replace parseFloat usage by calling the existing
parseNumber utility (or using Decimal) to parse and normalize values, then
convert to number only when needed (or keep as Decimal for downstream
arithmetic). Update the locations referencing volNum/costNum/feeNum to accept
Decimal values or the normalized numeric result from parseNumber so all
financial calculations in functions like the Kraken parser use Decimal.js-backed
parsing consistently.
In `@src/web/charts.ts`:
- Around line 185-209: The three chart data builders (assetDistribution,
currencyComposition, withholdingsByCountry) are creating fake Decimal-like
objects instead of real Decimal instances; replace the fake objects with actual
Decimal.js values by constructing new Decimal(value) (or Decimal(value)) for
each mapped entry (use the same pattern for byAsset → assetDistribution,
byCurrency → currencyComposition, and byCountry → withholdingsByCountry), ensure
the numeric `value` passed is a plain number (e.g. Math.abs(...)/.toNumber()
results) and that Decimal is imported where these mappings occur so downstream
Decimal methods work correctly.
In `@src/web/index.html`:
- Around line 14-17: The hardcoded Spanish UI strings (the skip link text in the
<a class="skip-link" href="#upload">, the <nav class="top-bar"
aria-label="Ajustes"> label, the <select id="lang-select"> and the <button
id="theme-toggle" title="Cambiar tema" aria-label="Cambiar tema">) must be
replaced with localized values: wire these elements to your i18n/localization
lookup (or a messages object) so their textContent, title and aria-label
attributes are set from the current locale and updated when the user changes
<select id="lang-select">; use keys like "skip_to_content", "nav_settings", and
"toggle_theme" to fetch translations and ensure the anchor href and ARIA targets
remain unchanged while only the readable strings are swapped.
In `@src/web/main.ts`:
- Around line 373-384: renderResults() currently appends a new charts grid every
run, causing duplicates and leaving stale charts when a subsequent report has
none; before inserting the new chartsHtml, locate and remove any existing charts
container (the element with class "charts-grid") that was previously inserted
after casillasDiv so the DOM is cleaned up; then only insert the new grid if
chartsHtml is non-empty, updating the logic around
casillasDiv.insertAdjacentHTML and the chartsHtml variable in renderResults().
In `@src/web/manifest.json`:
- Line 5: The manifest uses absolute paths which break on GitHub Pages; update
the "start_url" value (currently "/") to a relative path ("./" or an appropriate
relative path) and change any icon paths in the manifest (the entries referenced
on lines 12-23) from absolute (leading "/") to relative paths (e.g.,
"./icons/..." or "icons/...") so the PWA resolves correctly when served from a
repository subpath; modify the "start_url" and all icon "src" entries in the
manifest.json accordingly.
In `@src/web/style.css`:
- Around line 540-552: The disclaimer link is too faint due to font-size:
0.75rem and opacity: 0.5 on .disclaimer-link; update the rules for
.disclaimer-link and .disclaimer-link a to increase legibility by raising the
font-size to at least 0.875rem (or 14px equivalent) and removing or increasing
opacity to ~0.85, and ensure the link color uses a higher-contrast token (e.g.,
use var(--text) or a stronger muted token) while keeping the hover state
distinct (leave .disclaimer-link a:hover as var(--text) or adjust similarly).
In `@src/web/sw.ts`:
- Around line 47-53: The fetch handler currently caches every network response
into API_CACHE (see fetch(event.request), response.clone(), and
caches.open(API_CACHE).then(cache => cache.put(...))). Change the logic so you
only clone and cache when the response is successful (use response.ok or check
status in 200–299) and skip caching for 4xx/5xx responses; still return the
original response to the caller. Ensure the conditional surrounds the clone +
cache.put call and leaves the catch fallback unchanged.
- Around line 1-76: The service worker file (symbols: self.addEventListener
handlers, CACHE_VERSION, STATIC_CACHE, API_CACHE) is currently excluded from
TypeScript and ESLint configs so it bypasses strict/ES2022 checks; update
tsconfig.json, eslint.config.js, and tsconfig.eslint.json to remove the
exclusion entry that omits src/web/sw.ts (or the glob that excludes it) so the
file is compiled and linted, and ensure the project config still has "target":
"ES2022" and "strict": true (or equivalent ESLint/TS settings) so the service
worker is validated under the required strict/ES2022 rules.
In `@tests/i18n/i18n.test.ts`:
- Around line 10-12: The tests currently seed locale state in beforeEach by
calling setLocale("es"), which prevents initLocale() from exercising its
detection path and makes tests stateful; remove or change that seeding so each
test starts from a neutral/no-persisted-locale state (e.g., clear the
persistence store or unset the locale key instead of calling setLocale in
beforeEach), and update the tests around lines 53-58 similarly so they
explicitly set up the environment they need (mock navigator.language or
localStorage as required) before calling initLocale(); target symbols:
beforeEach, setLocale, initLocale.
In `@tsconfig.json`:
- Line 28: The service worker source src/web/sw.ts is excluded from the main
tsconfig but not added as a build entry, so the registered /sw.js from main.ts
won't be emitted; update the Vite build configuration to include sw.ts as a
separate entry (use build.rollupOptions.input and add the sw.ts -> /sw.js
mapping) or add an explicit build step for sw.ts (e.g., a separate tsup/rollup
invocation) so that sw.ts is compiled and output as /sw.js for registration;
ensure the file path and output name match the registration in main.ts.
---
Outside diff comments:
In `@src/cli/index.ts`:
- Around line 349-355: The early-return currently checks only report.positions
and skips output even when there are cancellations from a previous D-6; update
the condition so we only skip when there are no positions and no cancellations
relevant to the --previous-d6 flow. Replace the existing if
(report.positions.length === 0) check with a combined condition that returns
only when report.positions.length === 0 AND (opts.previousD6 is falsey OR
report.cancelled.length === 0); reference fetchEcbRates, generateD6Report and
the report.positions / report.cancelled fields so the cancellation workflow runs
when opts.previousD6 is enabled and report.cancelled has entries.
---
Nitpick comments:
In `@src/i18n/index.ts`:
- Around line 86-90: The dynamic RegExp constructed in the params replacement
loop (for (const [k, v] of Object.entries(params)) { text = text.replace(new
RegExp(`\\{\\{${k}\\}\\}`, "g"), v); }) can pose a ReDoS risk; change the
implementation to avoid building a regex from the key: either escape the key
before passing to RegExp or replace the replace(RegExp...) call with a safe
string-based approach such as text = text.split(`{{${k}}}`).join(String(v));
ensuring you still coerce v to string and keep the same behavior inside the same
params handling block/function.
In `@src/parsers/kraken.ts`:
- Line 229: The ternary setting fxRateToBase currently returns the same value
for both branches, so replace the redundant expression with a single constant
value (fxRateToBase: "1") or, if non-EUR quotes must be supported, implement
proper FX lookup logic using the quote variable (e.g., call a fx-rate helper or
fetch from the FX service) and assign the computed rate to fxRateToBase; update
the parser function in kraken.ts where fxRateToBase and quote are used to
reflect the chosen approach.
In `@src/web/charts.ts`:
- Line 213: The MONTHS constant currently hardcodes Spanish abbreviations and
should be replaced with a locale-aware source; remove the hardcoded const MONTHS
and generate month labels via the app i18n or Intl.DateTimeFormat instead (e.g.,
map 0..11 to i18n.t('months.short.jan').. or use new Intl.DateTimeFormat(locale,
{ month: 'short' }).format), and update any code referencing MONTHS to use that
generated array so labels respect the active locale (search for MONTHS in
src/web/charts.ts to update all usages).
🪄 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: d7776822-f5f2-45b9-a929-2d59282976f9
📒 Files selected for processing (38)
.github/workflows/deploy.ymlROADMAP.mdeslint.config.jssrc/cli/index.tssrc/engine/ecb.tssrc/generators/d6.tssrc/generators/modelo720-validator.tssrc/generators/modelo720.tssrc/i18n/index.tssrc/i18n/locales/ca.tssrc/i18n/locales/en.tssrc/i18n/locales/es.tssrc/i18n/locales/eu.tssrc/i18n/locales/gl.tssrc/index.tssrc/parsers/binance.tssrc/parsers/coinbase.tssrc/parsers/index.tssrc/parsers/kraken.tssrc/types/ibkr.tssrc/web/charts.tssrc/web/disclaimer.tssrc/web/index.htmlsrc/web/main.tssrc/web/manifest.jsonsrc/web/style.csssrc/web/sw.tstests/engine/ecb.test.tstests/engine/fifo.test.tstests/generators/d6.test.tstests/generators/modelo720-validator.test.tstests/generators/modelo720.test.tstests/i18n/i18n.test.tstests/parsers/binance.test.tstests/parsers/coinbase.test.tstests/parsers/kraken.test.tstsconfig.eslint.jsontsconfig.json
Comment on lines
+540
to
+552
| .disclaimer-link { | ||
| margin-top: 0.25rem; | ||
| font-size: 0.75rem; | ||
| opacity: 0.5; | ||
| } | ||
|
|
||
| .disclaimer-link a { | ||
| color: var(--muted); | ||
| } | ||
|
|
||
| .disclaimer-link a:hover { | ||
| color: var(--text); | ||
| } |
Contributor
There was a problem hiding this comment.
Increase contrast on the disclaimer link.
font-size: 0.75rem plus opacity: 0.5 on muted text makes the legal notice too faint. That undermines the WCAG 2.1 AA claim in this PR.
🛠️ Proposed fix
.disclaimer-link {
margin-top: 0.25rem;
- font-size: 0.75rem;
- opacity: 0.5;
+ font-size: 0.875rem;
+ opacity: 1;
}
.disclaimer-link a {
- color: var(--muted);
+ color: var(--text);
}📝 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
| .disclaimer-link { | |
| margin-top: 0.25rem; | |
| font-size: 0.75rem; | |
| opacity: 0.5; | |
| } | |
| .disclaimer-link a { | |
| color: var(--muted); | |
| } | |
| .disclaimer-link a:hover { | |
| color: var(--text); | |
| } | |
| .disclaimer-link { | |
| margin-top: 0.25rem; | |
| font-size: 0.875rem; | |
| opacity: 1; | |
| } | |
| .disclaimer-link a { | |
| color: var(--text); | |
| } | |
| .disclaimer-link a:hover { | |
| color: var(--text); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/style.css` around lines 540 - 552, The disclaimer link is too faint
due to font-size: 0.75rem and opacity: 0.5 on .disclaimer-link; update the rules
for .disclaimer-link and .disclaimer-link a to increase legibility by raising
the font-size to at least 0.875rem (or 14px equivalent) and removing or
increasing opacity to ~0.85, and ensure the link color uses a higher-contrast
token (e.g., use var(--text) or a stronger muted token) while keeping the hover
state distinct (leave .disclaimer-link a:hover as var(--text) or adjust
similarly).
Comment on lines
+1
to
+76
| /** | ||
| * DeclaRenta Service Worker — offline-first caching. | ||
| * | ||
| * Strategy: | ||
| * - Static assets (HTML, CSS, JS): cache-first with network fallback | ||
| * - ECB API calls: network-first with cache fallback (stale rates are better than no rates) | ||
| */ | ||
|
|
||
| /// <reference lib="webworker" /> | ||
| declare const self: ServiceWorkerGlobalScope; | ||
|
|
||
| const CACHE_VERSION = "declarenta-v1"; | ||
| const STATIC_CACHE = `${CACHE_VERSION}-static`; | ||
| const API_CACHE = `${CACHE_VERSION}-api`; | ||
|
|
||
| const STATIC_URLS = [ | ||
| "/", | ||
| "/index.html", | ||
| ]; | ||
|
|
||
| self.addEventListener("install", (event) => { | ||
| event.waitUntil( | ||
| caches.open(STATIC_CACHE).then((cache) => cache.addAll(STATIC_URLS)) | ||
| ); | ||
| self.skipWaiting(); | ||
| }); | ||
|
|
||
| self.addEventListener("activate", (event) => { | ||
| event.waitUntil( | ||
| caches.keys().then((keys) => | ||
| Promise.all( | ||
| keys | ||
| .filter((key) => key !== STATIC_CACHE && key !== API_CACHE) | ||
| .map((key) => caches.delete(key)) | ||
| ) | ||
| ) | ||
| ); | ||
| self.clients.claim(); | ||
| }); | ||
|
|
||
| self.addEventListener("fetch", (event) => { | ||
| const url = new URL(event.request.url); | ||
|
|
||
| // ECB API: network-first with cache fallback | ||
| if (url.hostname.includes("ecb.europa.eu")) { | ||
| event.respondWith( | ||
| fetch(event.request) | ||
| .then((response) => { | ||
| const clone = response.clone(); | ||
| caches.open(API_CACHE).then((cache) => cache.put(event.request, clone)); | ||
| return response; | ||
| }) | ||
| .catch(() => caches.match(event.request).then((r) => r ?? new Response("", { status: 503 }))) | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // Static assets: cache-first | ||
| if (event.request.method === "GET" && url.origin === self.location.origin) { | ||
| event.respondWith( | ||
| caches.match(event.request).then((cached) => { | ||
| if (cached) return cached; | ||
| return fetch(event.request).then((response) => { | ||
| if (response.ok) { | ||
| const clone = response.clone(); | ||
| caches.open(STATIC_CACHE).then((cache) => cache.put(event.request, clone)); | ||
| } | ||
| return response; | ||
| }); | ||
| }) | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| event.respondWith(fetch(event.request)); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tsconfig files ==="
fd -i 'tsconfig*.json' . -x sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' sh {}
echo
echo "=== references to the service worker ==="
rg -n 'src/web/sw\.ts|sw\.js|serviceWorker' tsconfig*.json eslint.config.* src/web .github/workflowsRepository: GeiserX/DeclaRenta
Length of output: 1294
Include src/web/sw.ts in TypeScript compilation.
The service worker file is currently excluded from tsconfig.json (line 28), eslint.config.js, and tsconfig.eslint.json, which means it bypasses TypeScript strict mode and ES2022 type checking. The coding guideline requires all **/*.ts files to use TypeScript strict mode with ES2022 target. Remove the exclusion so this critical offline-first infrastructure code benefits from type safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/sw.ts` around lines 1 - 76, The service worker file (symbols:
self.addEventListener handlers, CACHE_VERSION, STATIC_CACHE, API_CACHE) is
currently excluded from TypeScript and ESLint configs so it bypasses
strict/ES2022 checks; update tsconfig.json, eslint.config.js, and
tsconfig.eslint.json to remove the exclusion entry that omits src/web/sw.ts (or
the glob that excludes it) so the file is compiled and linted, and ensure the
project config still has "target": "ES2022" and "strict": true (or equivalent
ESLint/TS settings) so the service worker is validated under the required
strict/ES2022 rules.
Comment on lines
+10
to
+12
| beforeEach(() => { | ||
| setLocale("es"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't seed locale state before asserting initLocale() detection.
beforeEach() forces "es" for every case. If setLocale() persists state, initLocale() here just reloads that value instead of exercising the detection path, so this test becomes stateful and environment-dependent.
Also applies to: 53-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/i18n/i18n.test.ts` around lines 10 - 12, The tests currently seed
locale state in beforeEach by calling setLocale("es"), which prevents
initLocale() from exercising its detection path and makes tests stateful; remove
or change that seeding so each test starts from a neutral/no-persisted-locale
state (e.g., clear the persistence store or unset the locale key instead of
calling setLocale in beforeEach), and update the tests around lines 53-58
similarly so they explicitly set up the environment they need (mock
navigator.language or localStorage as required) before calling initLocale();
target symbols: beforeEach, setLocale, initLocale.
| }, | ||
| "include": ["src/**/*.ts"], | ||
| "exclude": ["node_modules", "dist", "tests"] | ||
| "exclude": ["node_modules", "dist", "tests", "src/web/sw.ts"] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a separate tsconfig for the service worker or Vite config handling it
fd -t f "tsconfig" --exec cat {}
echo "---"
# Check Vite config for service worker handling
fd -t f "vite.config" --exec cat {}Repository: GeiserX/DeclaRenta
Length of output: 1310
🏁 Script executed:
# Find sw.ts file and check if it exists
fd -t f "sw.ts" -x cat {}
echo "---"
# Check for any build config that might handle the service worker
fd -t f "\.(json|ts|js)$" | xargs grep -l "service.?worker\|sw\.ts" 2>/dev/null | head -20
echo "---"
# Check Vite config more carefully for entry points or rollup options
fd -t f "vite.config" -x grep -A 20 "build\|entry\|worker" {}Repository: GeiserX/DeclaRenta
Length of output: 2346
🏁 Script executed:
# Look for service worker registration in the codebase
rg "sw\.ts|service.*worker|register.*worker" -t ts -t js -t html
echo "---"
# Check if there's a build script that might handle sw.ts separately
cat package.json 2>/dev/null | grep -A 20 "scripts"
echo "---"
# Look for index.html to see what's the main entry point
fd -t f "index.html" -x cat {}Repository: GeiserX/DeclaRenta
Length of output: 5618
🏁 Script executed:
# Check main.ts for service worker registration
fd -t f "main.ts" -path "*/src/web/*" -x cat {}Repository: GeiserX/DeclaRenta
Length of output: 230
🏁 Script executed:
# Find main.ts file
fd "main.ts" -x cat {}
echo "---"
# Also check if sw.ts is imported anywhere
rg "import.*sw\|register.*worker\|navigator.serviceWorker" -t ts -t jsRepository: GeiserX/DeclaRenta
Length of output: 21352
Ensure sw.ts is built as a separate entry point or bundle.
Excluding src/web/sw.ts from the main tsconfig.json is correct, but the current Vite config lacks explicit configuration to build it. The app registers /sw.js in main.ts, but with no entry point for sw.ts in Vite and no imports of it in the build chain, the service worker will not be compiled and registration will fail.
Configure either:
- Add
sw.tsas a separate Vite entry point viabuild.rollupOptions.input - Or explicitly handle it in the build script (e.g.,
tsupforsw.ts)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tsconfig.json` at line 28, The service worker source src/web/sw.ts is
excluded from the main tsconfig but not added as a build entry, so the
registered /sw.js from main.ts won't be emitted; update the Vite build
configuration to include sw.ts as a separate entry (use
build.rollupOptions.input and add the sw.ts -> /sw.js mapping) or add an
explicit build step for sw.ts (e.g., a separate tsup/rollup invocation) so that
sw.ts is compiled and output as /sw.js for registration; ensure the file path
and output name match the registration in main.ts.
- Modelo 720: extract shared getValuationRate() helper (DRY) - Binance: extend KNOWN_QUOTES, remove fragile 3-char fallback, validate columns - Coinbase/Kraken: replace parseFloat with Decimal.js for monetary precision - Charts: use proper Decimal instances instead of duck-typed objects - CLI: throw on BOE validation failure instead of silent warnings - SW: only cache response.ok ECB responses - PWA manifest: use relative paths for portability - Style: increase disclaimer link contrast for WCAG compliance - i18n: add a11y.* keys (skip-link, nav, theme, drop-zone) in all 5 locales - HTML: wire data-i18n-aria/data-i18n-title for dynamic ARIA translation - Main: prevent duplicate chart grids on locale re-render - Tests: fix i18n test setup ordering
top: -100% on position: absolute resolves to 0 when parent has no explicit height, leaving the link visible in the corner. Use the standard sr-only clip pattern that reliably hides the element until keyboard focus.
Remove the "Self-hosted. Tus datos no salen de tu equipo" header subtitle, its app.privacy i18n keys from all 5 locales, the .privacy CSS class, and the --privacy-bg/--privacy-border custom properties from all 4 theme blocks.
This was referenced Apr 17, 2026
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
Mega PR completing Phases 2, 3, and 4 of the DeclaRenta roadmap.
Phase 2 — Multi-broker (crypto)
Phase 3 — Tax models
Phase 4 — Web UI profesional
t()system, language selector dropdown,localechangeevent re-rendering<dialog>:focus-visibleoutlines, ARIA roles/labels, keyboard navigation for drop zoneStats
Test plan
npm test— 407 tests passnpm run lint— zero errorsnpm run typecheck— zero errorsSummary by CodeRabbit
New Features
Documentation
Tests