From 438b140ee698e8c4ddfc24c7c824bfe59ced0034 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 24 Oct 2026 00:31:19 +0000 Subject: [PATCH] fix(ladder): dispose the planned-purchase chart when leaving its section The Chart.js instance behind the ladder planned-purchase timeline, with its ResizeObserver and listeners, was only destroyed when the timeline was re-initialized, so it stayed alive after the user navigated away from Settings > Purchasing. Visiting the General sub-tab also rebuilt it inside the hidden Purchasing panel, because loadGlobalSettings runs the ladder init for both sub-tabs. navigation.ts now calls disposeLadderTimeline() when switching to any top-level tab other than Settings and to any settings sub-tab other than Purchasing. Dispose destroys the chart and drops in-flight loads. initLadderTimeline skips building while its section is hidden, and the existing loadGlobalSettings call re-initializes the timeline when the Purchasing sub-tab is shown again. The other items in the issue (409 reload, focus restore after save, no tab stop on the canvas) were shipped in #688. Refs #667 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/ladder-timeline.test.ts | 34 ++++++++- frontend/src/__tests__/navigation.test.ts | 38 ++++++++++ frontend/src/ladder-timeline.ts | 18 ++++- frontend/src/navigation.ts | 7 ++ .../ladder-timeline-teardown.spec.ts | 69 +++++++++++++++++++ 5 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 frontend/tests-e2e/ladder-timeline-teardown.spec.ts diff --git a/frontend/src/__tests__/ladder-timeline.test.ts b/frontend/src/__tests__/ladder-timeline.test.ts index 87a060557..50f178399 100644 --- a/frontend/src/__tests__/ladder-timeline.test.ts +++ b/frontend/src/__tests__/ladder-timeline.test.ts @@ -5,7 +5,7 @@ jest.mock('../permissions', () => ({ canAccess: jest.fn(() => true) })); import { listAccountsMinimal } from '../api/accounts'; import { getLadderEvents, getLadderRuns, amendLadderEvent, type LadderEvent, type LadderRun } from '../api/ladder'; -import { initLadderTimeline } from '../ladder-timeline'; +import { initLadderTimeline, disposeLadderTimeline } from '../ladder-timeline'; import { canAccess } from '../permissions'; import { Chart } from 'chart.js'; @@ -360,3 +360,35 @@ test('editor labels each row by its basis and keeps the native amount', async () expect(open('m2')).toBe('Planning budget (on-demand-equivalent $/hr)|2.000000'); expect(open('m3')).toBe('Planning budget (on-demand-equivalent $/hr)|1.000000'); }); + +describe('chart teardown on navigation (issue #667)', () => { + const chartInstance = (index: number): { destroy: jest.Mock } => jest.mocked(Chart).mock.results[index]!.value as { destroy: jest.Mock }; + + test('dispose destroys the chart, stops pending loads and init builds a fresh one', async () => { + await initLadderTimeline(document.getElementById('timeline')!, [], false); + expect(Chart).toHaveBeenCalledTimes(1); + let release!: (value: LadderEvent[]) => void; + jest.mocked(getLadderEvents).mockImplementationOnce(() => new Promise(resolve => { release = resolve; })); + document.querySelector('[data-reload]')!.click(); + disposeLadderTimeline(); + expect(chartInstance(0).destroy).toHaveBeenCalled(); + release([event]); + await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); + expect(Chart).toHaveBeenCalledTimes(1); + disposeLadderTimeline(); + await initLadderTimeline(document.getElementById('timeline')!, [], false); + expect(Chart).toHaveBeenCalledTimes(2); + }); + + test('a hidden section is not initialized and releases the previous chart', async () => { + await initLadderTimeline(document.getElementById('timeline')!, [], false); + document.body.classList.add('hidden'); + const calls = jest.mocked(getLadderEvents).mock.calls.length; + await initLadderTimeline(document.getElementById('timeline')!, [], false); + document.body.classList.remove('hidden'); + expect(chartInstance(0).destroy).toHaveBeenCalled(); + expect(Chart).toHaveBeenCalledTimes(1); + expect(listAccountsMinimal).toHaveBeenCalledTimes(1); + expect(getLadderEvents).toHaveBeenCalledTimes(calls); + }); +}); diff --git a/frontend/src/__tests__/navigation.test.ts b/frontend/src/__tests__/navigation.test.ts index acc672af4..0d780326a 100644 --- a/frontend/src/__tests__/navigation.test.ts +++ b/frontend/src/__tests__/navigation.test.ts @@ -26,6 +26,9 @@ jest.mock('../settings', () => ({ isUnsavedChanges: jest.fn().mockReturnValue(false), loadAccountsTab: jest.fn().mockResolvedValue(undefined), })); +jest.mock('../ladder-timeline', () => ({ + disposeLadderTimeline: jest.fn(), +})); jest.mock('../riexchange', () => ({ loadRIExchange: jest.fn().mockResolvedValue(undefined), loadAutomationSettings: jest.fn().mockResolvedValue(undefined), @@ -62,6 +65,7 @@ import { loadAutomationSettings } from '../riexchange'; import { canAccess } from '../permissions'; import { loadInventory } from '../inventory'; import { isAdmin } from '../auth'; +import { disposeLadderTimeline } from '../ladder-timeline'; function setPathname(pathname: string): void { const url = new URL(window.location.href); @@ -693,4 +697,38 @@ describe('Navigation Module', () => { expect(document.getElementById('home-tab')?.classList.contains('active')).toBe(true); }); }); + + // Issue #667: the ladder planned-purchase chart is released when its + // section is left; returning re-runs loadGlobalSettings, which re-inits it. + describe('ladder timeline teardown', () => { + test('leaving Settings disposes the ladder chart; the dirty guard keeps it until confirmed', async () => { + switchTab('admin'); + switchSettingsSubTab('purchasing'); + (disposeLadderTimeline as jest.Mock).mockClear(); + (isUnsavedChanges as jest.Mock).mockReturnValue(true); + switchTab('home'); + expect(disposeLadderTimeline).not.toHaveBeenCalled(); + (isUnsavedChanges as jest.Mock).mockReturnValue(false); + switchTab('plans'); + expect(disposeLadderTimeline).toHaveBeenCalledTimes(1); + document.querySelector('.modal-confirm-backdrop')?.remove(); + }); + + test('other settings sub-tabs dispose it and Purchasing reloads it', () => { + switchTab('admin'); + (disposeLadderTimeline as jest.Mock).mockClear(); + switchSettingsSubTab('purchasing'); + expect(disposeLadderTimeline).not.toHaveBeenCalled(); + for (const sub of ['general', 'accounts', 'users']) { + (disposeLadderTimeline as jest.Mock).mockClear(); + switchSettingsSubTab(sub); + expect(disposeLadderTimeline).toHaveBeenCalledTimes(1); + } + (loadGlobalSettings as jest.Mock).mockClear(); + (disposeLadderTimeline as jest.Mock).mockClear(); + switchSettingsSubTab('purchasing'); + expect(disposeLadderTimeline).not.toHaveBeenCalled(); + expect(loadGlobalSettings).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/frontend/src/ladder-timeline.ts b/frontend/src/ladder-timeline.ts index 85d6cb1d0..f71b1c315 100644 --- a/frontend/src/ladder-timeline.ts +++ b/frontend/src/ladder-timeline.ts @@ -10,8 +10,22 @@ import { ladderChartConfig, ladderChartModel, ladderChartSummary, LADDER_UNIT_LA Chart.register(...registerables); let disposeTimeline: (() => void) | undefined; -export async function initLadderTimeline(root: HTMLElement, configs: readonly LadderConfig[], globalEnabled: boolean): Promise { +/** + * Destroys the planned-purchase chart (Chart.js removes its resize and event + * listeners on destroy) and drops any in-flight timeline load. Navigation + * calls this when the Purchasing settings section is left; loadGlobalSettings + * re-runs initLadderTimeline when the section is shown again (issue #667). + */ +export function disposeLadderTimeline(): void { disposeTimeline?.(); + disposeTimeline = undefined; +} + +export async function initLadderTimeline(root: HTMLElement, configs: readonly LadderConfig[], globalEnabled: boolean): Promise { + disposeLadderTimeline(); + // Settings loads also run for the General sub-tab, where the Purchasing + // panel is hidden; building a chart there would leak it until the next init. + if (root.closest('.hidden')) return; let chart: Chart<'line'> | undefined; let rowTotals = new Map(); let generation = 0; @@ -22,7 +36,7 @@ export async function initLadderTimeline(root: HTMLElement, configs: readonly La let accountsLoaded = false; let filters: LadderFilters = { ...emptyLadderFilters(), status: 'scheduled', from: new Date().toISOString().slice(0, 19) }; let selectedID = ''; - disposeTimeline = () => { disposed = true; generation++; chart?.destroy(); }; + disposeTimeline = () => { disposed = true; generation++; chart?.destroy(); chart = undefined; }; const labels = { run_id: 'Run', layer_type: 'Commitment type', term: 'Commitment term', payment_option: 'Payment option', status: 'Status' }; root.innerHTML = `

Planned purchases

Inspect retained planning tranches and edit future scheduled budgets. Ladder purchases are not executed yet. Coverage and expiry forecasts are unavailable.

diff --git a/frontend/src/navigation.ts b/frontend/src/navigation.ts index dc71108a7..5537e65d6 100644 --- a/frontend/src/navigation.ts +++ b/frontend/src/navigation.ts @@ -7,6 +7,7 @@ import { loadRecommendations } from './recommendations'; import { loadPlans } from './plans'; import { initHistoryDateRange, loadHistory } from './history'; import { loadGlobalSettings, isUnsavedChanges, loadAccountsTab } from './settings'; +import { disposeLadderTimeline } from './ladder-timeline'; import { loadUsers } from './users'; import { loadApiKeys } from './apikeys'; import { loadSavingsHistory } from './modules/savings-history'; @@ -162,6 +163,9 @@ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { setActiveNav(tabName); + // Leaving Settings: release the ladder chart and its listeners (#667). + if (tabName !== 'admin') disposeLadderTimeline(); + document.querySelectorAll('.tab-content').forEach(content => { content.classList.toggle('active', content.id === `${tabName}-tab`); }); @@ -300,6 +304,9 @@ export function switchSettingsSubTab(subTab: string, opts: SwitchTabOptions = {} btn.setAttribute('aria-selected', isActive ? 'true' : 'false'); }); + // Only the Purchasing sub-tab shows the ladder timeline (#667). + if (subTab !== 'purchasing') disposeLadderTimeline(); + for (const [tab, ids] of Object.entries(SECTION_MAP)) { for (const id of ids) { const el = document.getElementById(id); diff --git a/frontend/tests-e2e/ladder-timeline-teardown.spec.ts b/frontend/tests-e2e/ladder-timeline-teardown.spec.ts new file mode 100644 index 000000000..e2e4f4921 --- /dev/null +++ b/frontend/tests-e2e/ladder-timeline-teardown.spec.ts @@ -0,0 +1,69 @@ +import { test, expect, type Page } from '@playwright/test'; +import { mockApi, seedAuth } from './fixtures/recs'; +import type { LadderEvent, LadderRun } from '../src/api/ladder'; + +// Issue #667: the planned-purchase Chart.js instance (and the ResizeObserver +// it attaches) is released when the user leaves the Purchasing settings +// section, and rebuilt when they come back. +async function liveLadderObservers(page: Page): Promise { + return page.evaluate(() => (window as unknown as { __ladderObservers: () => number }).__ladderObservers()); +} + +test('leaving Purchasing settings disposes the ladder chart and returning rebuilds it', async ({ page }) => { + const errors: string[] = []; + page.on('pageerror', error => errors.push(error.message)); + await page.addInitScript(() => { + // Track which ResizeObserver targets inside the ladder timeline are live. + const live = new Map>(); + const proto = ResizeObserver.prototype; + const { observe, unobserve, disconnect } = proto; + proto.observe = function (this: ResizeObserver, target: Element, options?: ResizeObserverOptions) { + if (target.closest('#ladder-purchase-timeline')) { + if (!live.has(this)) live.set(this, new Set()); + live.get(this)!.add(target); + } + return observe.call(this, target, options); + }; + proto.unobserve = function (this: ResizeObserver, target: Element) { live.get(this)?.delete(target); return unobserve.call(this, target); }; + proto.disconnect = function (this: ResizeObserver) { live.delete(this); return disconnect.call(this); }; + (window as unknown as { __ladderObservers: () => number }).__ladderObservers = () => Array.from(live.values()).reduce((sum, set) => sum + set.size, 0); + }); + await page.route('**/*', route => new URL(route.request().url()).hostname === '127.0.0.1' ? route.fallback() : route.abort()); + await seedAuth(page); await mockApi(page); + await page.route('**/api/commitment-options', route => route.fulfill({ json: { status: 'ok', aws: {} } })); + await page.route('**/api/ri-exchange/config', route => route.fulfill({ json: { mode: 'manual', auto_exchange_enabled: false, utilization_threshold: 80, max_payment_per_exchange_usd: 0, max_payment_daily_usd: 0, lookback_days: 30 } })); + await page.route('**/api/ladder/configs', route => route.fulfill({ json: { configs: [] } })); + const events: LadderEvent[] = [1, 2].map(day => ({ id: `event-${day}`, config_id: 'config', run_id: 'run', layer_type: 'compute-sp', term: '1yr', payment_option: 'no-upfront', status: 'scheduled', run_status: 'planned', revision: 0, amount_usd_hr: '1.000000', scheduled_date: `2030-01-0${day}T00:00:00Z`, created_at: '2026-01-01T00:00:00Z' })); + const run: LadderRun = { id: 'run', config_id: 'config', status: 'planned', started_at: '2026-01-01T00:00:00Z', created_at: '2026-01-01T00:00:00Z', baseline_usd_hr: null, existing_usd_hr: null, target_usd_hr: null, gap_usd_hr: null, total_hourly_commit: '2.000000', actions: [] }; + let trancheLoads = 0; + await page.route('**/api/ladder/runs?*', route => route.fulfill({ json: { total_count: 1, runs: [run] } })); + await page.route('**/api/ladder/tranches?*', route => { trancheLoads++; return route.fulfill({ json: { total_count: events.length, total_usd_hr: '2.000000', events } }); }); + + await page.goto('/admin/purchasing'); + const timeline = page.locator('#ladder-purchase-timeline'); + await expect(timeline.locator('[data-summary]')).toContainText('2 of 2'); + await expect.poll(() => liveLadderObservers(page)).toBeGreaterThan(0); + const loadsAfterFirstVisit = trancheLoads; + + await page.locator('#admin-subtab-general').click(); + await expect.poll(() => liveLadderObservers(page)).toBe(0); + await page.waitForLoadState('networkidle'); + // The General sub-tab reloads settings but must not rebuild the hidden chart. + expect(await liveLadderObservers(page)).toBe(0); + expect(trancheLoads).toBe(loadsAfterFirstVisit); + + await page.locator('#admin-subtab-purchasing').click(); + await expect(timeline.locator('canvas')).toBeVisible(); + await expect.poll(() => liveLadderObservers(page)).toBeGreaterThan(0); + expect(trancheLoads).toBeGreaterThan(loadsAfterFirstVisit); + + await page.locator('#plans-tab-btn').click(); + await expect(page).toHaveURL(/\/plans$/); + await expect.poll(() => liveLadderObservers(page)).toBe(0); + + await page.goBack(); + await expect(page).toHaveURL(/\/admin\/purchasing$/); + await expect(timeline.locator('[data-summary]')).toContainText('2 of 2'); + await expect.poll(() => liveLadderObservers(page)).toBeGreaterThan(0); + expect(errors).toEqual([]); +});