feat(frontend): UI revamp — IA restructure + action-center reskin (closes #340)

[cristim]

Summary

Restructure the CUDly frontend information architecture and reskin to the action-center pattern from ~/Desktop/CUDly UI Revamp/ mocks. Frontend-only — no backend, API, IaC, or Go-side changes.

Closes #340.

Before / After

Before: 6 top-tabbed nav (Dashboard · Recommendations · Purchase Plans · Purchase History · RI Exchange · Settings) above the content.

After: action-center shell — top bar (logo + user info) + left sidebar with 6 sections:

• Home (← Dashboard)
• Opportunities (← Recommendations)
• Plans (← Purchase Plans, label simplified)
• Purchases (← Purchase History)
• Inventory & Coverage (NEW umbrella — folds in RI Exchange + placeholders for Active commitments + Coverage)
• Admin (← Settings — 4 sub-tabs relabeled: General · Purchasing policies · Accounts & onboarding · Users, roles & API keys)

Commit-by-commit

#	Commit	What
1	style: design-token CSS variables	T1 — :root tokens for color, type, space, radius, elevation, layout
2	feat: action-center shell — top bar + sidebar nav	T2 — new HTML shell + collapsible sidebar (icons via inline Lucide SVG, ISC)
3	refactor: rename tab IDs + URL redirect	T3 — IA rename with LEGACY_PATH_REDIRECTS so /dashboard, /recommendations, /history, /settings/*, /ri-exchange still resolve
4	feat: fold RI Exchange into Inventory & Coverage	T4 — new umbrella section + sub-nav (RI Exchange default) + 2 placeholder sub-sections
5	feat/admin: relabel Admin sub-tabs	T5 — Purchasing → "Purchasing policies", Accounts → "Accounts & onboarding", Users & API Keys → "Users, roles & API keys"
6	feat/home: KPI sparklines + tightened card grid	T6 — DOM-built KPI tiles + inline-SVG sparklines on YTD Savings (skipped silently when no data)
7	style: visual polish across Opportunities + Plans	T8 — design tokens applied to action-box + plans cards; .context-drawer CSS slot reserved for future right-side drawer

Verification

• Tests: 1602/1602 pass (started at 1591; +6 sparkline + +5 inventory tests).
• Build: clean. Bundle: 503 KiB total (+0.8% from 499 KiB baseline, well under the ±10% gate).
• Manual browser smoke: docker-compose stack up; every section + every sub-tab reachable via sidebar; legacy URLs redirect correctly (/dashboard → /home, /ri-exchange → /inventory, /settings/general → /admin/general).

Deferred (tracked as sub-tasks in #340)

These mock widgets need backend support before the frontend can wire them:

• Home: Onboarding & setup guidance checklist; Operational at-a-glance upcoming events feed
• Opportunities: full right-side "Plan builder" drawer (CSS hook in place via .context-drawer); cross-payment-option comparison
• Plans: Approvals & blockers timeline; Plan health score
• Purchases: Audit timeline / search audit
• Inventory & Coverage: Active commitments per-commitment list; per-provider coverage donuts; renewal-risk alerts; proactive RI exchange suggestions
• Admin: API keys usage stats; Approval ownership matrix; Federation setup guided wizard; per-provider purchasing policy defaults UI

Empty-state placeholders ship today wherever a section would otherwise render nothing meaningful.

Constraints upheld

• No functionality dropped — only relocated: every existing user-reachable control (Settings panels, RI Exchange UI, history filters, plan-creation modals, etc.) is reachable in the new IA.
• Frontend-only: zero changes under internal/, iac/, terraform/, Optimizer/.
• XSS: DOM construction via createElement / textContent / appendChild for all new code paths; no innerHTML with interpolated data.
• Accessibility: aria-* / role / aria-labelledby preserved on every renamed panel; sidebar nav is keyboard-navigable.
• Test coverage parity: 80.65% statements / 68.22% branches → preserved (+11 new tests for new code paths).
• URL backwards-compat: LEGACY_PATH_REDIRECTS map handles every old top-level path.

Closes

• Closes feat(frontend): UI revamp — restructure IA + visual reskin to action-center pattern #340

Summary by CodeRabbit

• New Features

  • Redesigned app shell with collapsible sidebar (state persists), reorganized top-level tabs and Admin sub-navigation.
  • Dashboard KPI tiles now include sparklines.
  • “Inventory & Coverage” umbrella with sub-tabs (including RI Exchange) and related navigation wiring.

• Style

  • Global design-token theme and broad visual refresh across layout, components, and controls; new KPI tile styling.

• Tests

  • Added/updated test suites for navigation, dashboard sparklines, inventory sub-tabs, settings dirty-state, HTML structure, and CSS token validation.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: de9cabdb-e8c4-4435-b74f-eef9ff342663

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Restructures the frontend IA to Home, Opportunities, Plans, Purchases, Inventory & Coverage, and Admin; replaces top tabs with an app-shell + left sidebar; adds :root design tokens and tokenized component styles; implements Inventory sub-tabs; refactors dashboard KPIs to DOM-rendered tiles with sparkline helpers; and updates routing and tests.

Changes

UI Revamp: IA Restructuring + Visual Reskin

UI Revamp: IA Restructuring + Visual Reskin

Layer / File(s)	Summary
Navigation IA & Routing frontend/src/navigation.ts, frontend/src/app.ts	Canonical tabs added (home,opportunities,plans,purchases,inventory,admin); legacy-path redirects added; switchTab/applyTabFromPath/initRouter updated; admin history URLs use /admin/<subTab> and the unsaved-changes guard tracks leaving admin.
App Shell & Sidebar Layout frontend/src/index.html, frontend/src/styles/layout.css, frontend/src/index.ts	Index rebuilt into app-shell (topbar, sidebar, main); layout.css implements sidebar/topbar layout and collapse behavior; index.ts wires the sidebar toggle and persists cudly_sidebar_collapsed in localStorage.
Design Tokens & Base Styles frontend/src/styles/base.css	Add :root --cudly-* tokens (colors, typography, spacing, radii, shadows, dimensions); update body and global utilities to consume tokens; add reduced-motion and skip-to-main rules.
Component & Theming Styles frontend/src/styles/components.css, frontend/src/styles/forms.css, frontend/src/styles/plans.css, frontend/src/styles/settings.css, frontend/src/styles/modals.css, frontend/src/styles/tables.css, frontend/src/styles/charts.css	Introduce .kpi-tile; retokenize buttons, toasts, chips, badges, filter popovers, action box, context drawer, forms (filter chips), plan cards, modals, tables, charts; remove legacy sticky settings rail CSS.
Inventory & Coverage Module frontend/src/inventory.ts	New module exporting switchInventorySubSection, wireSubNavListeners, and loadInventory() that manage three sub-sections and default to ri-exchange, triggering loadRIExchange() when active.
Dashboard KPI Rendering & Sparklines frontend/src/dashboard.ts, frontend/src/__tests__/dashboard.test.ts	renderDashboardSummary now builds KPI tiles via DOM; adds sparklinePoints and attachSparkline (exported via __test__); loadSavingsTrendChart computes cumulative YTD series and calls attachSparkline('ytd', ...); tests validate helper outputs and DOM polyline insertion.
Upcoming Purchases DOM frontend/src/dashboard.ts	renderUpcomingPurchases builds upcoming cards via DOM APIs, sets dataset attributes, and sanitizes provider badge class using a whitelist regex.
Purchases Deeplink & Recommendations Anchors frontend/src/purchases-deeplink.ts, frontend/src/recommendations.ts	Purchases deeplink handler now replaces stale deeplink URLs with /purchases (missing/cancel/post-API); recommendations helpers now anchor/observe/mount against opportunities-tab instead of recommendations-tab.
Settings → Admin Dirty-State frontend/src/settings-subnav.ts, frontend/src/__tests__/settings-subnav.test.ts	Sticky sub-nav rail removed; reflectDirtyState now toggles .has-unsaved on #admin-tab-btn (previously #settings-tab-btn); tests updated and narrowed to dirty-state behavior.
Tests & Fixtures frontend/src/__tests__/*	Navigation/html tests updated for sidebar selectors and new tab keys; inventory.test added for sub-section behavior and loadRIExchange invocation; many recommendations tests' fixtures updated to opportunities-tab; css.test now asserts presence of design tokens; dashboard sparkline helper tests added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• LeanerCloud/CUDly#295: Modifies frontend/src/dashboard.ts and its dashboard rendering logic (renderDashboardSummary / loadDashboard), so they overlap.
• LeanerCloud/CUDly#311: Also edits frontend/src/dashboard.ts with defensive guards around rendering, overlapping KPI changes.
• LeanerCloud/CUDly#160: Related recommendations UI wiring and filter/status lifecycle updates, overlapping tab-anchor logic.

Suggested labels

priority/p1, severity/high

"🐰 I hopped the sidebar, found tabs anew,
Sparkles in KPIs and tokens of blue,
Inventory nested, Admin in view,
Routes rewritten and tests updated too,
A rabbit cheers — the UI refreshed for you!"

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/issue-340-ui-revamp

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

This PR ships a UI revamp (issue #340) restructuring the IA from 6 top tabs to a sidebar-driven action-center pattern + design-token reskin. Frontend-only, 7 atomic commits, 1602/1602 tests pass, bundle within +0.8% of baseline.

Specifically interested in feedback on:

• the LEGACY_PATH_REDIRECTS table in navigation.ts (does it cover all reasonable old-bookmark paths?)
• the inline-SVG sparkline approach in dashboard.ts (vs reaching for chart.js)
• the inventory data-inv-subtab sub-nav pattern in inventory.ts (vs extracting from settings-subnav.ts)
• XSS posture on the DOM-construction migration in renderDashboardSummary (replacing the old innerHTML template)

[coderabbitai]

🧩 Analysis chain

Let me pull the relevant source files to give you targeted feedback on all four areas.

---

🏁 Script executed:

#!/bin/bash
# Find the key files
fd -e ts navigation.ts
fd -e ts dashboard.ts
fd -e ts inventory.ts
fd -e ts settings-subnav.ts

Length of output: 141

---

🏁 Script executed:

#!/bin/bash
echo "=== navigation.ts ==="
cat frontend/src/navigation.ts

echo ""
echo "=== dashboard.ts ==="
cat frontend/src/dashboard.ts

echo ""
echo "=== inventory.ts ==="
cat frontend/src/inventory.ts

echo ""
echo "=== settings-subnav.ts ==="
cat frontend/src/settings-subnav.ts

Length of output: 44883

---

@cristim: Happy to dig into all four areas — found some real issues worth addressing alongside the design observations.

---

1. LEGACY_PATH_REDIRECTS — coverage & pushState vs replaceState

Coverage gap — /purchase-plans

The old nav had a "Purchase Plans" tab. If its URL slug was /purchase-plans, it is not covered by the redirect map:

// navigation.ts (current)
const LEGACY_PATH_REDIRECTS: Record<string, string> = {
  dashboard: 'home',
  recommendations: 'opportunities',
  history: 'purchases',
  settings: 'admin',
  'ri-exchange': 'inventory',
  // ⚠️ 'purchase-plans': 'plans' — missing if old slug was /purchase-plans
};

If the old top-level path was literally /plans, it's already a valid TABS key and needs no entry. But if the slug was ever /purchase-plans, old bookmarks silently fall back to /home. Worth confirming against the pre-#340 pushState calls or browser history in your smoke test.

pushState vs replaceState for legacy redirects

The PR description says "we replaceState() to the canonical new URL", but applyTabFromPath() returns the resolved tab name and switchTab — when called without push: false — issues a pushState. That means visiting /dashboard pushes a new /home entry onto the stack rather than replacing the legacy URL in place. The practical effect: pressing Back from /home takes the user back to /dashboard instead of wherever they came from. The fix is a replaceState when a legacy redirect fires:

export function applyTabFromPath(): string {
  const segment = window.location.pathname
    .replace(/^\/+/, '').replace(/\/+$/, '').split('/')[0]
    ?.toLowerCase() ?? '';
  if (segment === '') return 'home';
  if (segment in LEGACY_PATH_REDIRECTS) {
    const canonical = LEGACY_PATH_REDIRECTS[segment]!;
    // Rewrite legacy URL in-place so Back doesn't loop through it
    window.history.replaceState(null, '', '/' + canonical + window.location.search + window.location.hash);
    return canonical;
  }
  return segment in TABS ? segment : 'home';
}

Settings sub-tab deep-links (e.g. /settings/general, /settings/purchasing) are handled correctly — segment[0] = settings → admin, then getSettingsSubTabFromPath() reads segment[1]. ✅

---

2. Inline-SVG sparklines vs Chart.js

The design choice is correct — a 80×24 px decorative polyline does not warrant a Chart.js instance. The approach is lightweight, bundle-neutral for new code, and the aria-hidden="true" / createElementNS DOM path is clean.

Two actionable issues:

a) Three KPI tiles have empty SVG placeholders that never get data

renderDashboardSummary stamps data-spark-key on all four tiles (savings, commitments, coverage, ytd), but attachSparkline is only called for ytd (inside loadSavingsTrendChart). The other three empty <svg class="kpi-tile-spark"> elements persist in the DOM forever. If populating them is deferred, hide them until data is available:

// in renderDashboardSummary, for tiles that won't get a sparkline yet
if (t.kpi !== 'ytd') spark.classList.add('hidden');

b) Math.min/max(...values) spread in sparklinePoints

const min = Math.min(...values);  // ⚠️ stack overflow for large arrays
const max = Math.max(...values);

Sparklines are short series in practice, but this is a cheap fix:

const min = values.reduce((a, b) => (b < a ? b : a), values[0]!);
const max = values.reduce((a, b) => (b > a ? b : a), values[0]!);

---

3. data-inv-subtab pattern vs extracting from settings-subnav.ts

The separation is correct — renderSubNav in settings-subnav.ts is a scrollspy sticky left-rail with an IntersectionObserver and a DOM-restructuring wrapper (.settings-layout-content) designed for long single-page forms. The inventory sub-nav is discrete section-switching. Sharing that infrastructure would couple two fundamentally different UX patterns and drag IntersectionObserver complexity into a simple tab-switch.

However, there is a real cross-contamination bug caused by the global .sub-tab-btn query in navigation.ts:

// navigation.ts — switchSettingsSubTab (line ~144)
document.querySelectorAll<HTMLButtonElement>('.sub-tab-btn').forEach(btn => {
  const isActive = btn.dataset['settingsTab'] === subTab;  // ← global, not scoped
  btn.classList.toggle('active', isActive);
  btn.setAttribute('aria-selected', isActive ? 'true' : 'false');
});

Inventory sub-tab buttons also carry .sub-tab-btn but use data-inv-subtab (not data-settings-tab). When the user switches to Admin, this loop runs over all .sub-tab-btn elements in the DOM — including those in #inventory-tab — and clears their active class and sets aria-selected="false" because dataset['settingsTab'] is undefined. If the user then switches back to Inventory, the active indicator will be gone until switchInventorySubSection re-activates the button.

The fix is to scope the query:

// navigation.ts — switchSettingsSubTab
document.querySelectorAll<HTMLButtonElement>('#admin-tab .sub-tab-btn').forEach(btn => {

switchInventorySubSection already scopes with #inventory-tab .sub-tab-btn, so the bug is strictly one-directional (settings clobbers inventory). ✅ Inventory → settings direction is clean.

---

4. XSS posture — renderDashboardSummary migration

renderDashboardSummary itself is clean. The tile values flow through formatCurrency, String(), or compile-time literals, and the full createElement / textContent / appendChild chain means no user-controlled string can be interpreted as markup. ✅

buildUpcomingDetailsModal is also clean — full DOM API, textContent on every field. ✅

renderUpcomingPurchases was not migrated and has several unescaped interpolations:

// dashboard.ts — renderUpcomingPurchases
container.innerHTML = purchases.map(p => {
  return `
    …
    <span class="provider-badge ${p.provider}">   // ⚠️ raw in class attribute
    ${p.provider.toUpperCase()}                    // ⚠️ raw in text node
    Step ${p.step_number} of ${p.total_steps}      // ⚠️ raw (numeric at runtime but typed as API fields)
    data-id="${p.execution_id}">                   // ⚠️ raw in attribute — quote-break possible

p.provider injected directly as a CSS class token is the highest-risk line: a value like aws" onclick="alert(1) would escape the attribute and inject an event handler. p.execution_id in data-id="..." carries the same attribute-injection risk. Since this function handles the same backend data as the newly-secured modal builder, it should receive the same DOM-API treatment (or at minimum, escapeHtml on every interpolated value including p.provider, p.execution_id, p.step_number, and p.total_steps).

---

Summary table

Area	Status	Action
LEGACY_PATH_REDIRECTS coverage	✅ / ⚠️	Verify /purchase-plans slug; add if needed
Legacy redirect uses pushState not replaceState	🐛	Use replaceState in applyTabFromPath when a legacy segment is hit
Sparklines — design choice	✅	Correct, keep
3 KPI tiles have orphaned empty SVG placeholders	⚠️	Hide until data available
Math.min/max spread in sparklinePoints	⚠️	Use reduce
data-inv-subtab vs extracting renderSubNav	✅	Separation is correct
switchSettingsSubTab global .sub-tab-btn query clobbers inventory active state	🐛	Scope to #admin-tab .sub-tab-btn
renderDashboardSummary XSS posture	✅	Clean
renderUpcomingPurchases unescaped provider, execution_id, etc.	🐛	Apply escapeHtml to all interpolated API fields, or migrate to DOM API

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

frontend/src/navigation.ts (1)

164-168: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Scope admin sub-tab activation to the admin container.

The global .sub-tab-btn query can clear active state on unrelated sub-navs. Scope this to #admin-tab .sub-tab-btn (or equivalent admin root) to avoid cross-tab clobbering.

🤖 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 `@frontend/src/navigation.ts` around lines 164 - 168, The current
querySelectorAll('.sub-tab-btn') is global and will toggle active state across
all sub-navs; restrict it to the admin container by querying only admin-scoped
buttons (e.g., use '#admin-tab .sub-tab-btn' or the specific admin root element)
so the loop that reads btn.dataset['settingsTab'] === subTab and updates
classList.toggle('active', ...) and aria-selected only affects admin sub-tabs;
locate the block that uses
document.querySelectorAll<HTMLButtonElement>('.sub-tab-btn') and change the
selector to the admin-scoped selector or query the admin root and call
querySelectorAll on it instead.

frontend/src/dashboard.ts (1)

362-383: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Unescaped attribute interpolation in innerHTML still allows injection.

provider is interpolated into a class name and execution_id into data-id in an innerHTML template. Please migrate this block to DOM construction (createElement, classList, dataset, textContent) to close attribute-injection risk.

🤖 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 `@frontend/src/dashboard.ts` around lines 362 - 383, The current
container.innerHTML = purchases.map(...) template injects unescaped values into
attributes (class and data-id); replace this whole innerHTML construction in the
block using DOM APIs: iterate purchases, call createElement for the card and its
children (matching the structure built currently), use classList.add() for
classes (validate/whitelist p.provider before adding to avoid injection), set
element.dataset.executionId = String(p.execution_id) and dataset.action for
buttons, and assign textual content with textContent (or use escapeHtml for any
HTML you intend to allow) for plan_name, service, dateParts and formatCurrency
output; keep getDateParts, escapeHtml and formatCurrency usage for values but
stop using innerHTML and build the nodes programmatically so attributes are
never interpolated into raw HTML.

🧹 Nitpick comments (3)

frontend/src/dashboard.ts (1)

211-214: ⚡ Quick win

Only render spark placeholders for tiles that actually receive data.

All KPI cards currently get an SVG placeholder, but only ytd is populated. This leaves dead UI placeholders. Consider rendering spark SVG conditionally (or hiding by default until populated).

Also applies to: 575-577

🤖 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 `@frontend/src/dashboard.ts` around lines 211 - 214, Only create or show the
SVG spark placeholder for KPI tiles that actually have spark data: instead of
unconditionally creating the element via spark = document.createElementNS(...)
and adding class 'kpi-tile-spark' and dataset['sparkKey'] = t.kpi, gate that
code on the presence of the tile's spark data (e.g., check a sparkData map or
t.kpi === 'ytd' or similar) and only append the spark to the DOM when populated;
alternatively, if you prefer to always create the element, add a default hidden
state (e.g., add a 'hidden' class or set aria-hidden and CSS display:none) and
ensure whatever populates the spark removes the hidden state when data is
available, using the same identifiers spark, 'kpi-tile-spark',
dataset['sparkKey'], and t.kpi to locate and toggle visibility.

frontend/src/__tests__/navigation.test.ts (1)

44-55: ⚡ Quick win

Add canonical /admin/* path coverage for sub-tab parsing.

These updates move nav semantics to admin, but getSettingsSubTabFromPath coverage is still centered on /settings/*. Please add /admin, /admin/accounts, and /admin/purchasing cases (and keep one /settings/* legacy case) so the new canonical routing is locked by tests.

Proposed test additions

 describe('getSettingsSubTabFromPath', () => {
+  test('returns general for root admin path', () => {
+    delete (window as unknown as Record<string, unknown>).location;
+    (window as unknown as Record<string, unknown>).location = { pathname: '/admin' } as Location;
+    expect(getSettingsSubTabFromPath()).toBe('general');
+  });
+
+  test('returns accounts for /admin/accounts', () => {
+    delete (window as unknown as Record<string, unknown>).location;
+    (window as unknown as Record<string, unknown>).location = { pathname: '/admin/accounts' } as Location;
+    expect(getSettingsSubTabFromPath()).toBe('accounts');
+  });
+
+  test('returns purchasing for /admin/purchasing', () => {
+    delete (window as unknown as Record<string, unknown>).location;
+    (window as unknown as Record<string, unknown>).location = { pathname: '/admin/purchasing' } as Location;
+    expect(getSettingsSubTabFromPath()).toBe('purchasing');
+  });
+
   test('returns general for root settings path', () => {
     delete (window as unknown as Record<string, unknown>).location;
     (window as unknown as Record<string, unknown>).location = { pathname: '/settings' } as Location;
     expect(getSettingsSubTabFromPath()).toBe('general');
   });

Also applies to: 162-190

🤖 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 `@frontend/src/__tests__/navigation.test.ts` around lines 44 - 55, Update the
navigation tests to include canonical /admin routes for sub-tab parsing: add
test cases calling getSettingsSubTabFromPath with "/admin", "/admin/accounts",
and "/admin/purchasing" (expecting the appropriate sub-tab results) and retain
one legacy "/settings" case (e.g., "/settings", "/settings/accounts" or similar)
to ensure backward compatibility; locate these additions in
frontend/src/__tests__/navigation.test.ts near the existing
getSettingsSubTabFromPath test block so both old "/settings/*" and new
"/admin/*" paths are covered.

frontend/src/inventory.ts (1)

68-77: ⚡ Quick win

Make listener wiring conditional on finding sub-nav buttons

At Line 69–77, listenersWired is set true even when no #inventory-tab .sub-tab-btn nodes exist. If initialization ever happens before that DOM is present, listeners never get attached later.

Suggested patch

 function wireSubNavListeners(): void {
   if (listenersWired) return;
-  document.querySelectorAll<HTMLButtonElement>('#inventory-tab .sub-tab-btn').forEach((btn) => {
+  const buttons = document.querySelectorAll<HTMLButtonElement>('#inventory-tab .sub-tab-btn');
+  if (buttons.length === 0) return;
+  buttons.forEach((btn) => {
     btn.addEventListener('click', () => {
       const name = btn.dataset['invSubtab'] ?? DEFAULT_SUB_SECTION;
       switchInventorySubSection(name);
     });
   });
   listenersWired = true;
 }

🤖 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 `@frontend/src/inventory.ts` around lines 68 - 77, The function
wireSubNavListeners sets listenersWired = true even when
document.querySelectorAll('#inventory-tab .sub-tab-btn') returns no elements,
preventing future wiring; modify wireSubNavListeners so it only sets
listenersWired to true after it actually attaches at least one listener (i.e.,
check the NodeList length or count attached handlers) and leave listenersWired
false when no buttons were found so the function can be re-run later to attach
handlers for elements targeted by switchInventorySubSection.

🤖 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 `@frontend/src/__tests__/html.test.ts`:
- Around line 93-116: Add a new unit test mirroring the existing tab assertions
to explicitly check for the inventory tab: create a test (e.g., test('has
inventory tab button', ...)) that queries
document.querySelector('[data-tab="inventory"]') and asserts it is truthy; place
it alongside the other tab tests so the suite validates the inventory tab button
presence and prevents silent nav regressions.

In `@frontend/src/index.html`:
- Around line 41-64: The sidebar tab buttons (elements with class "tab-btn" such
as home-tab-btn, opportunities-tab-btn, plans-tab-btn, purchases-tab-btn,
inventory-tab-btn, admin-tab-btn) lose their visible label when .sidebar-label
is hidden; add an explicit aria-label attribute on each button with the same
human-readable text as its .sidebar-label (e.g., "Home", "Opportunities",
"Plans", "Purchases", "Inventory & Coverage", "Admin") so assistive tech still
has a name while keeping existing role="tab" and aria-selected attributes.

In `@frontend/src/navigation.ts`:
- Around line 222-230: applyTabFromPath currently maps legacy segments via
LEGACY_PATH_REDIRECTS but doesn't canonicalize the browser URL; update
applyTabFromPath to call history.replaceState to rewrite the URL to the
canonical path when a legacy mapping is used. Specifically, after resolving
segment via LEGACY_PATH_REDIRECTS (the branch that returns
LEGACY_PATH_REDIRECTS[segment]), compute the canonical path (e.g., '/' + the
resolved tab), call history.replaceState(null, '', canonicalPath) before
returning the tab, and leave other branches unchanged.

In `@frontend/src/styles/components.css`:
- Around line 32-39: The `.kpi-tile h3` rules (margin-bottom, color, font-size,
font-weight, letter-spacing) are being overridden by the later `.card h3` rule
because they have equal specificity; fix this by either moving the `.kpi-tile
h3` block to appear after the `.card h3` block or increase selector specificity
so it wins (e.g., change selector to a more specific form like `.card.kpi-tile
h3` or `.kpi-tile.card h3`), and update the selector references in
components.css accordingly so the KPI typography always takes precedence.

---

Outside diff comments:
In `@frontend/src/dashboard.ts`:
- Around line 362-383: The current container.innerHTML = purchases.map(...)
template injects unescaped values into attributes (class and data-id); replace
this whole innerHTML construction in the block using DOM APIs: iterate
purchases, call createElement for the card and its children (matching the
structure built currently), use classList.add() for classes (validate/whitelist
p.provider before adding to avoid injection), set element.dataset.executionId =
String(p.execution_id) and dataset.action for buttons, and assign textual
content with textContent (or use escapeHtml for any HTML you intend to allow)
for plan_name, service, dateParts and formatCurrency output; keep getDateParts,
escapeHtml and formatCurrency usage for values but stop using innerHTML and
build the nodes programmatically so attributes are never interpolated into raw
HTML.

In `@frontend/src/navigation.ts`:
- Around line 164-168: The current querySelectorAll('.sub-tab-btn') is global
and will toggle active state across all sub-navs; restrict it to the admin
container by querying only admin-scoped buttons (e.g., use '#admin-tab
.sub-tab-btn' or the specific admin root element) so the loop that reads
btn.dataset['settingsTab'] === subTab and updates classList.toggle('active',
...) and aria-selected only affects admin sub-tabs; locate the block that uses
document.querySelectorAll<HTMLButtonElement>('.sub-tab-btn') and change the
selector to the admin-scoped selector or query the admin root and call
querySelectorAll on it instead.

---

Nitpick comments:
In `@frontend/src/__tests__/navigation.test.ts`:
- Around line 44-55: Update the navigation tests to include canonical /admin
routes for sub-tab parsing: add test cases calling getSettingsSubTabFromPath
with "/admin", "/admin/accounts", and "/admin/purchasing" (expecting the
appropriate sub-tab results) and retain one legacy "/settings" case (e.g.,
"/settings", "/settings/accounts" or similar) to ensure backward compatibility;
locate these additions in frontend/src/__tests__/navigation.test.ts near the
existing getSettingsSubTabFromPath test block so both old "/settings/*" and new
"/admin/*" paths are covered.

In `@frontend/src/dashboard.ts`:
- Around line 211-214: Only create or show the SVG spark placeholder for KPI
tiles that actually have spark data: instead of unconditionally creating the
element via spark = document.createElementNS(...) and adding class
'kpi-tile-spark' and dataset['sparkKey'] = t.kpi, gate that code on the presence
of the tile's spark data (e.g., check a sparkData map or t.kpi === 'ytd' or
similar) and only append the spark to the DOM when populated; alternatively, if
you prefer to always create the element, add a default hidden state (e.g., add a
'hidden' class or set aria-hidden and CSS display:none) and ensure whatever
populates the spark removes the hidden state when data is available, using the
same identifiers spark, 'kpi-tile-spark', dataset['sparkKey'], and t.kpi to
locate and toggle visibility.

In `@frontend/src/inventory.ts`:
- Around line 68-77: The function wireSubNavListeners sets listenersWired = true
even when document.querySelectorAll('#inventory-tab .sub-tab-btn') returns no
elements, preventing future wiring; modify wireSubNavListeners so it only sets
listenersWired to true after it actually attaches at least one listener (i.e.,
check the NodeList length or count attached handlers) and leave listenersWired
false when no buttons were found so the function can be re-run later to attach
handlers for elements targeted by switchInventorySubSection.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 639f8247-a074-4d72-8d15-901a9d8dab72

📥 Commits

Reviewing files that changed from the base of the PR and between f33ae7c and 1497859.

📒 Files selected for processing (20)

• frontend/src/__tests__/dashboard.test.ts
• frontend/src/__tests__/html.test.ts
• frontend/src/__tests__/inventory.test.ts
• frontend/src/__tests__/navigation.test.ts
• frontend/src/__tests__/recommendations.test.ts
• frontend/src/__tests__/settings-subnav.test.ts
• frontend/src/app.ts
• frontend/src/dashboard.ts
• frontend/src/index.html
• frontend/src/index.ts
• frontend/src/inventory.ts
• frontend/src/navigation.ts
• frontend/src/purchases-deeplink.ts
• frontend/src/recommendations.ts
• frontend/src/settings-subnav.ts
• frontend/src/styles/base.css
• frontend/src/styles/components.css
• frontend/src/styles/layout.css
• frontend/src/styles/plans.css
• frontend/src/styles/settings.css

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

frontend/src/navigation.ts (1)

163-167: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Scope admin sub-tab activation to the admin panel.

Line 163 toggles every .sub-tab-btn in the document. With Inventory & Coverage now having its own sub-nav, switching an Admin sub-tab also clears active state outside Admin. Query the admin container instead.

Suggested fix

-  document.querySelectorAll<HTMLButtonElement>('.sub-tab-btn').forEach(btn => {
+  document.querySelectorAll<HTMLButtonElement>('#admin-tab .sub-tab-btn').forEach(btn => {
     const isActive = btn.dataset['settingsTab'] === subTab;
     btn.classList.toggle('active', isActive);
     btn.setAttribute('aria-selected', isActive ? 'true' : 'false');
   });

🤖 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 `@frontend/src/navigation.ts` around lines 163 - 167, The code currently
toggles every '.sub-tab-btn' across the document, causing Admin sub-tab changes
to clear active state in other panels; restrict the query to the Admin panel
container (e.g., select the admin panel element and call
querySelectorAll('.sub-tab-btn') on it) so only sub-tab buttons within the Admin
container are iterated; keep using the same logic with
btn.dataset['settingsTab'], btn.classList.toggle('active', isActive) and
btn.setAttribute('aria-selected', ...) but replace document.querySelectorAll
with the scoped container.querySelectorAll to limit updates to the Admin panel.

♻️ Duplicate comments (1)

frontend/src/navigation.ts (1)

223-225: ⚠️ Potential issue | 🟠 Major

Still missing legacy URL canonicalization.

Line 224 resolves the legacy segment but leaves the stale pathname in place, so /dashboard, /settings/..., etc. can remain in the address bar/back stack even though Lines 34-35 say they are replaceState()'d. Rewrite to the canonical path before returning.

🤖 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 `@frontend/src/navigation.ts` around lines 223 - 225, The legacy segment check
resolves the target but doesn’t update the browser URL, leaving stale paths in
the address bar/back stack; change the branch that handles LEGACY_PATH_REDIRECTS
to compute the canonical segment (e.g., const canonical =
LEGACY_PATH_REDIRECTS[segment]) then call the history replace API to update the
pathname (preserving query/hash) — e.g., use window.history.replaceState(...) or
your router's replace method — and finally return the canonical; likewise ensure
the final return uses the rewritten canonical value (use LEGACY_PATH_REDIRECTS,
segment, TABS as location hints).

🧹 Nitpick comments (1)

frontend/src/__tests__/settings-subnav.test.ts (1)

30-45: ⚡ Quick win

Add a regression test for the RI Exchange exemption.

This suite only exercises the generic .settings-buttons path. The special-case guard in reflectDirtyState() is what prevents a second sticky save bar inside Purchasing, so it should stay covered here.

Suggested test

+  it('skips nested RI Exchange settings rows', () => {
+    const wrapper = document.createElement('div');
+    wrapper.id = 'ri-exchange-automation-settings';
+    const nested = document.createElement('div');
+    nested.className = 'settings-buttons';
+    wrapper.appendChild(nested);
+    document.body.appendChild(wrapper);
+
+    reflectDirtyState(true);
+    expect(nested.classList.contains('settings-savebar')).toBe(false);
+    expect(nested.classList.contains('dirty')).toBe(false);
+  });

🤖 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 `@frontend/src/__tests__/settings-subnav.test.ts` around lines 30 - 45, Add a
regression test that verifies the RI Exchange/Purchasing exemption in
reflectDirtyState: create a .settings-buttons element placed inside a container
that matches the guard used by reflectDirtyState (e.g., class "purchasing" or
data attribute "data-module='purchasing'" — use whichever the implementation
checks for), call reflectDirtyState(true), and assert that the inner
.settings-buttons does NOT receive the "settings-savebar" (or second sticky save
bar) class while other .settings-buttons still do; then call
reflectDirtyState(false) and assert cleanup of "dirty" as in the existing test.

🤖 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 `@frontend/src/index.html`:
- Around line 244-247: The Inventory sub-tabs (container class
"inventory-subnav" and the buttons with data-inv-subtab values
"active-commitments", "coverage", "ri-exchange" and aria-controls values like
"inventory-active-commitments") are missing the paired tabpanel elements and
proper button IDs/labeling; fix by giving each tab button a unique id, ensuring
aria-controls matches the id of a corresponding element with role="tabpanel",
adding aria-labelledby on each panel that references the tab button id, and
updating panel visibility (hidden or aria-hidden) and buttons' aria-selected
states so only the active tab (data-inv-subtab="ri-exchange") has
aria-selected="true" and its panel is shown; apply the same wiring pattern to
the other inventory/admin sub-tab group referenced around lines 302-307.

In `@frontend/src/styles/components.css`:
- Around line 498-505: The CSS is using invalid concatenations like
var(--cudly-surface)3cd and var(--cudly-surface)3e0 which browsers ignore; for
each affected selector (e.g., .settings-savebar.dirty::before and the
paused/medium-confidence/running badge rules mentioned in the comment) replace
the concatenated form with a valid color expression — either use the custom
property directly (background: var(--cudly-surface);) or compute a proper
translucent variant (e.g., background: rgba(...) or use
color-mix()/color-modifiers or a separate CSS variable that includes alpha) so
the background becomes valid CSS and displays as intended.

In `@frontend/src/styles/settings.css`:
- Line 253: Replace the invalid CSS tokens like "var(--cudly-surface)bf5",
"var(--cudly-surface)3cd", and "var(--cudly-surface)3e0" with valid color
values: for the two occurrences in the settings styles that are warning
backgrounds (previously at the shown diff line and another nearby), use `#fff3e0`
or var(--cudly-warn-bg); for the remaining warning/yellow uses (including the
settings.css line 421 and the four instances in components.css) replace them
with var(--cudly-warn-bg) to keep consistency; search for the exact malformed
strings and update each background-color declaration so the CSS parser accepts
the values.

---

Outside diff comments:
In `@frontend/src/navigation.ts`:
- Around line 163-167: The code currently toggles every '.sub-tab-btn' across
the document, causing Admin sub-tab changes to clear active state in other
panels; restrict the query to the Admin panel container (e.g., select the admin
panel element and call querySelectorAll('.sub-tab-btn') on it) so only sub-tab
buttons within the Admin container are iterated; keep using the same logic with
btn.dataset['settingsTab'], btn.classList.toggle('active', isActive) and
btn.setAttribute('aria-selected', ...) but replace document.querySelectorAll
with the scoped container.querySelectorAll to limit updates to the Admin panel.

---

Duplicate comments:
In `@frontend/src/navigation.ts`:
- Around line 223-225: The legacy segment check resolves the target but doesn’t
update the browser URL, leaving stale paths in the address bar/back stack;
change the branch that handles LEGACY_PATH_REDIRECTS to compute the canonical
segment (e.g., const canonical = LEGACY_PATH_REDIRECTS[segment]) then call the
history replace API to update the pathname (preserving query/hash) — e.g., use
window.history.replaceState(...) or your router's replace method — and finally
return the canonical; likewise ensure the final return uses the rewritten
canonical value (use LEGACY_PATH_REDIRECTS, segment, TABS as location hints).

---

Nitpick comments:
In `@frontend/src/__tests__/settings-subnav.test.ts`:
- Around line 30-45: Add a regression test that verifies the RI
Exchange/Purchasing exemption in reflectDirtyState: create a .settings-buttons
element placed inside a container that matches the guard used by
reflectDirtyState (e.g., class "purchasing" or data attribute
"data-module='purchasing'" — use whichever the implementation checks for), call
reflectDirtyState(true), and assert that the inner .settings-buttons does NOT
receive the "settings-savebar" (or second sticky save bar) class while other
.settings-buttons still do; then call reflectDirtyState(false) and assert
cleanup of "dirty" as in the existing test.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0f7f8e84-538f-42d7-a21d-e578dda1375b

📥 Commits

Reviewing files that changed from the base of the PR and between 1497859 and 1c4b5df.

📒 Files selected for processing (16)

• frontend/src/__tests__/css.test.ts
• frontend/src/__tests__/html.test.ts
• frontend/src/__tests__/settings-subnav.test.ts
• frontend/src/index.html
• frontend/src/navigation.ts
• frontend/src/settings-subnav.ts
• frontend/src/styles/base.css
• frontend/src/styles/charts.css
• frontend/src/styles/components.css
• frontend/src/styles/forms.css
• frontend/src/styles/layout.css
• frontend/src/styles/modals.css
• frontend/src/styles/plans.css
• frontend/src/styles/settings.css
• frontend/src/styles/tables.css
• frontend/src/styles/tabs.css

✅ Files skipped from review due to trivial changes (1)

• frontend/src/styles/charts.css

🚧 Files skipped from review as they are similar to previous changes (3)

• frontend/src/tests/html.test.ts
• frontend/src/styles/plans.css
• frontend/src/styles/layout.css

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 3 file(s) based on 3 unresolved review comments.

Files modified:

• frontend/src/index.html
• frontend/src/styles/components.css
• frontend/src/styles/settings.css

Commit: 3095246d2d83dfa699693e440f36a0c19434f3f2

The changes have been pushed to the feat/issue-340-ui-revamp branch.

Time taken: 5m 7s

[cristim]

The two navigation.ts findings in the cycle-2 review ("outside diff" at line 163-167 and "duplicate" at line 223-225) are already addressed in commit 8e8c770:

• #admin-tab .sub-tab-btn scoped selector — landed at navigation.ts:163
• history.replaceState canonicalisation — landed at navigation.ts:225-226

Both CR auto-acknowledged these in the per-comment thread.

[cristim]

@coderabbitai review

Round 3 — addressed all three actionable findings from your last review:

1. CRITICAL CSS fix: the over-eager replace_all of #fff in earlier token sweeps had chopped longer hex literals (#fff3cd, #fff3e0, #fffbf5) into invalid var(--cudly-surface)3cd strings — browsers silently dropped them. All 7 occurrences across components.css + settings.css now resolve to var(--cudly-warn-bg) (the canonical warm pastel).

2. Sub-tab a11y wiring: Inventory & Coverage and Admin sub-tabs now carry the full ARIA tab pattern — id on each button, matching role="tabpanel" + aria-labelledby on each panel, aria-orientation="horizontal" on the tablist.

3. RI Exchange exemption regression test: added a test in settings-subnav.test.ts that places a .settings-buttons row inside #ri-exchange-automation-settings and asserts the exemption holds (no .settings-savebar, no .dirty on the nested row).

Two prior findings (admin sub-tab querySelector scoping to #admin-tab; legacy URL canonicalization via replaceState) were already covered in commit 8e8c770b.

Tests: 1598/1598 pass. Build: clean.

[coderabbitai]

@cristim: Thanks for the thorough round-3 summary. To confirm what's now covered:

• ✅ CSS broken-hex fix — #fff3cd / #fff3e0 / #fffbf5 all seven occurrences resolved to var(--cudly-warn-bg) across components.css + settings.css.
• ✅ Sub-tab ARIA wiring — full role="tab" / role="tabpanel" / aria-labelledby / aria-orientation="horizontal" pattern applied to both Inventory & Coverage and Admin sub-navs.
• ✅ RI Exchange exemption regression test — .settings-buttons row inside #ri-exchange-automation-settings asserted clean (no .dirty, no savebar).
• ✅ #admin-tab .sub-tab-btn scoping + replaceState canonicalization — confirmed landed in 8e8c770b.

Triggering Round 3 review now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

frontend/src/__tests__/navigation.test.ts (2)

277-315: ⚡ Quick win

Add /admin/users path coverage for settings sub-tab resolution.

getSettingsSubTabFromPath is now covered for /admin, /admin/accounts, and /admin/purchasing, but not the users deep link.

Suggested patch

   test('returns purchasing for /admin/purchasing', () => {
     delete (window as unknown as Record<string, unknown>).location;
     (window as unknown as Record<string, unknown>).location = { pathname: '/admin/purchasing' } as Location;
     expect(getSettingsSubTabFromPath()).toBe('purchasing');
   });
+
+  test('returns users for /admin/users', () => {
+    delete (window as unknown as Record<string, unknown>).location;
+    (window as unknown as Record<string, unknown>).location = { pathname: '/admin/users' } as Location;
+    expect(getSettingsSubTabFromPath()).toBe('users');
+  });

🤖 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 `@frontend/src/__tests__/navigation.test.ts` around lines 277 - 315, Add a unit
test in frontend/src/__tests__/navigation.test.ts that mirrors the existing
admin path tests: call getSettingsSubTabFromPath with window.location.pathname
set to '/admin/users' (use the same delete and reassign pattern used in the
other tests) and assert the result is 'users'; place it alongside the other
describe('getSettingsSubTabFromPath') cases so both canonical (/admin/users)
and, if desired, the legacy '/settings/users' variant can be tested similarly.

---

43-54: ⚡ Quick win

Add inventory switchTab coverage to match the six-section IA.

The fixture and switch tests currently skip the inventory top-level section, so a regression there can pass unnoticed.

Suggested patch

       <div class="tabs">
         <button class="tab-btn active" data-tab="home">Dashboard</button>
         <button class="tab-btn" data-tab="opportunities">Recommendations</button>
         <button class="tab-btn" data-tab="plans">Plans</button>
         <button class="tab-btn" data-tab="purchases">History</button>
+        <button class="tab-btn" data-tab="inventory">Inventory & Coverage</button>
         <button class="tab-btn" data-tab="admin">Settings</button>
       </div>
       <div id="home-tab" class="tab-content active"></div>
       <div id="opportunities-tab" class="tab-content"></div>
       <div id="plans-tab" class="tab-content"></div>
       <div id="purchases-tab" class="tab-content"></div>
+      <div id="inventory-tab" class="tab-content"></div>
       <div id="admin-tab" class="tab-content">
@@
     test('switches to history tab and initializes date range', () => {
       switchTab('purchases');
@@
       expect(initHistoryDateRange).toHaveBeenCalled();
     });
+
+    test('switches to inventory tab', () => {
+      switchTab('inventory');
+
+      const inventoryBtn = document.querySelector('[data-tab="inventory"]');
+      const inventoryContent = document.getElementById('inventory-tab');
+      expect(inventoryBtn?.classList.contains('active')).toBe(true);
+      expect(inventoryContent?.classList.contains('active')).toBe(true);
+    });

Also applies to: 73-193

🤖 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 `@frontend/src/__tests__/navigation.test.ts` around lines 43 - 54, The test
fixture and switchTab assertions are missing the "inventory" top-level section;
update the HTML fixture in navigation.test.ts to add a button with class
"tab-btn" and data-tab="inventory" (e.g., data-tab="inventory") and a
corresponding content div with id="inventory-tab" and class "tab-content", and
then add assertions in the switchTab tests that call the switchTab handler (or
simulate clicking the tab button) to verify the "active" class toggles correctly
for the inventory button and `#inventory-tab` element just like the other sections
(use existing patterns around data-tab="home", "opportunities", etc., to locate
where to add the new case).

🤖 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 `@frontend/src/dashboard.ts`:
- Around line 247-253: attachSparkline currently removes children but leaves the
previous sparkline visible when values are empty or on error because the early
return doesn't hide the svg; update attachSparkline to explicitly clear children
and add svg.classList.add('hidden') (or remove the visible class) before
returning when values.length < 2, and likewise ensure any error/empty-response
handlers that previously only returned on failure also call the same hide/clear
behavior (refer to function attachSparkline and the kpi-tile-spark element/class
and mirror this hide+clear logic in the error paths around the other update code
mentioned at lines 621-625).

---

Nitpick comments:
In `@frontend/src/__tests__/navigation.test.ts`:
- Around line 277-315: Add a unit test in
frontend/src/__tests__/navigation.test.ts that mirrors the existing admin path
tests: call getSettingsSubTabFromPath with window.location.pathname set to
'/admin/users' (use the same delete and reassign pattern used in the other
tests) and assert the result is 'users'; place it alongside the other
describe('getSettingsSubTabFromPath') cases so both canonical (/admin/users)
and, if desired, the legacy '/settings/users' variant can be tested similarly.
- Around line 43-54: The test fixture and switchTab assertions are missing the
"inventory" top-level section; update the HTML fixture in navigation.test.ts to
add a button with class "tab-btn" and data-tab="inventory" (e.g.,
data-tab="inventory") and a corresponding content div with id="inventory-tab"
and class "tab-content", and then add assertions in the switchTab tests that
call the switchTab handler (or simulate clicking the tab button) to verify the
"active" class toggles correctly for the inventory button and `#inventory-tab`
element just like the other sections (use existing patterns around
data-tab="home", "opportunities", etc., to locate where to add the new case).

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9794562c-8e9f-4f34-bfee-5a3dbabcb427

📥 Commits

Reviewing files that changed from the base of the PR and between 1c4b5df and 0e08514.

📒 Files selected for processing (9)

• frontend/src/__tests__/html.test.ts
• frontend/src/__tests__/navigation.test.ts
• frontend/src/__tests__/settings-subnav.test.ts
• frontend/src/dashboard.ts
• frontend/src/index.html
• frontend/src/inventory.ts
• frontend/src/navigation.ts
• frontend/src/styles/components.css
• frontend/src/styles/settings.css

🚧 Files skipped from review as they are similar to previous changes (5)

• frontend/src/tests/settings-subnav.test.ts
• frontend/src/inventory.ts
• frontend/src/index.html
• frontend/src/navigation.ts
• frontend/src/styles/components.css

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

♻️ Duplicate comments (1)

frontend/src/dashboard.ts (1)

618-623: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Also clear the YTD sparkline in the error path.

The no-data branch now resets the KPI sparkline, but a failed analytics refresh still leaves the previous polyline visible while the widget switches to the unavailable state. Please mirror attachSparkline('ytd', []) in the catch block around Line 676 so the KPI stays consistent.

Suggested patch

   } catch (err) {
     // Analytics is Postgres-backed now, but the endpoint still guards with
     // a 503 when the analytics client isn't wired. Don't break the rest of
     // the dashboard — hide the widget and fall back to a neutral message.
     console.warn('Savings trend chart unavailable:', err);
     if (savingsTrendChart) { savingsTrendChart.destroy(); savingsTrendChart = null; }
+    attachSparkline('ytd', []);
     canvas.classList.add('hidden');
     if (empty) {
       empty.textContent = 'Savings history is not available yet.';
       empty.classList.remove('hidden');
     }

Also applies to: 628-632

🤖 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 `@frontend/src/dashboard.ts` around lines 618 - 623, The error path currently
leaves the previous YTD KPI sparkline visible; in the catch block that handles
the analytics refresh failure (where you hide the canvas, show the
unavailable/empty UI and destroy savingsTrendChart), call attachSparkline('ytd',
[]) as well to clear the KPI polyline; mirror the same attachSparkline('ytd',
[]) addition wherever the no-data branch resets the sparkline (same logic around
the alternate branch that hides canvas and shows empty) so both the no-data and
error paths consistently clear the YTD sparkline.

🤖 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.

Duplicate comments:
In `@frontend/src/dashboard.ts`:
- Around line 618-623: The error path currently leaves the previous YTD KPI
sparkline visible; in the catch block that handles the analytics refresh failure
(where you hide the canvas, show the unavailable/empty UI and destroy
savingsTrendChart), call attachSparkline('ytd', []) as well to clear the KPI
polyline; mirror the same attachSparkline('ytd', []) addition wherever the
no-data branch resets the sparkline (same logic around the alternate branch that
hides canvas and shows empty) so both the no-data and error paths consistently
clear the YTD sparkline.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6eca1756-f67e-4e6c-969a-462ac17dc1be

📥 Commits

Reviewing files that changed from the base of the PR and between 0e08514 and 8b55a4d.

📒 Files selected for processing (2)

• frontend/src/__tests__/navigation.test.ts
• frontend/src/dashboard.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• frontend/src/tests/navigation.test.ts

[cristim]

@coderabbitai review