fix(map+panels): swallow deck.gl render race + viewport-gate tech-readiness refresh#3833
Conversation
Module-scoped installDeckInterleavedRaceFilter() adds a one-shot
capture-phase window error listener that suppresses
TypeError: Cannot read properties of null (reading 'id')
when the throwing file matches /deck-stack-…\.js/. Called from
initDeck() — idempotent across recreateWithFallback / HMR.
Trigger is the deck.gl 9.x + maplibre-gl 5.x interleaved-mode race:
setProps({layers}) finalizes a layer while maplibre's painter is
mid-render, so the painter callback iterates a list that just had
a slot nulled. MapboxOverlay.onError can't see this — maplibre, not
deck, owns the throwing callstack.
Sentry's beforeSend in main.ts:313-315 already drops this pattern
for telemetry, so impact is purely console-noise removal. First-
party .id crashes still surface because the filter requires BOTH
the message shape AND the deck-stack chunk filename.
…iants
Replace `if (SITE_VARIANT !== 'happy')` with `… && shouldLoad('tech-readiness')`,
matching the thermal-escalation gate one line below.
Why this fixes the energy/finance/commodity-variant 5s timeout:
App.ts:576-583 merges ALL_PANELS into panelSettings on every variant
for cross-variant pref carryover, so shouldCreatePanel('tech-readiness')
returns true everywhere — but the bootstrap seed key only exists on
full + tech variants, so the unconditional refresh() at
services/economic/index.ts:694 times out on every other variant's
data-loader cycle. Viewport-gating prevents the refresh fan-out from
firing on variants whose panel will never be visible.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryTwo targeted fixes: a capture-phase
Confidence Score: 4/5Both changes are surgical and low blast-radius; the recurring tech-readiness timeout storm is resolved and the deck.gl console noise is suppressed, but the initial forced-load path and the broad error-listener suppression warrant a quick look before merging. The data-loader fix correctly eliminates repeated timeouts during normal refresh cycles, but the initial loadAllData(true) call at startup bypasses the viewport gate — meaning one timeout can still appear in logs on first page load for affected variants. The error filter in DeckGLMap is well-scoped but calls stopImmediatePropagation beyond what is needed to suppress console noise, which could silently block future error handlers registered after initDeck(). Both changed files are worth a second read: data-loader.ts for the forceAll=true initial-load gap, and DeckGLMap.ts for the stopImmediatePropagation side-effect. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as App.ts
participant DL as DataLoaderManager
participant PL as PanelLayoutManager
participant TRP as TechReadinessPanel
participant DGM as DeckGLMap
participant Win as window (error events)
App->>PL: await panelLayout.init()
PL-->>TRP: lazyPanel() dynamic import (async, not awaited)
TRP-->>PL: ctx.panels set after import resolves
App->>DL: "loadAllData(forceAll=true)"
Note over DL: shouldLoad = forceAll || isPanelNearViewport
Note over DL: forceAll=true, task always queued
DL->>TRP: refresh() batch N after 300ms delays
App->>DL: loadAllData() subsequent refresh cycles
Note over DL: forceAll=false, shouldLoad gates on viewport
Note over DL: isPanelNearViewport returns false on energy/finance/commodity
DL-->>DL: task NOT queued, fix takes effect
App->>DGM: initDeck()
DGM->>Win: installDeckInterleavedRaceFilter() once
Win-->>Win: capture-phase listener registered
Note over Win: Matches msg and deck-stack filename
Win-->>Win: preventDefault plus stopImmediatePropagation
Reviews (1): Last reviewed commit: "fix(panels): gate tech-readiness refresh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (SITE_VARIANT !== 'happy' && shouldLoad('tech-readiness')) { | ||
| tasks.push({ name: 'techReadiness', task: runGuarded('techReadiness', () => (this.ctx.panels['tech-readiness'] as TechReadinessPanel)?.refresh()) }); |
There was a problem hiding this comment.
shouldLoad is defined as forceAll || isPanelNearViewport(id). On the initial startup call (App.ts:1227 calls loadAllData(true)), forceAll=true short-circuits the check and the tech-readiness refresh() is still dispatched. By the time the batch containing this task runs (after at least one 300 ms batch delay), the lazyPanel async import has already resolved and this.ctx.panels['tech-readiness'] is populated on energy/finance/commodity variants — so ?.refresh() is not a no-op. The behavior for the initial load is unchanged from before this fix; subsequent loadAllData() (forceAll=false) cycles are correctly gated. Worth noting if the PR test plan includes checking initial-load logs, as one timeout entry will still appear on first page load.
| ev.preventDefault(); | ||
| ev.stopImmediatePropagation(); |
There was a problem hiding this comment.
ev.preventDefault() is sufficient to suppress the browser's default console logging of uncaught errors. The additional ev.stopImmediatePropagation() call prevents any other window.addEventListener('error', {capture:true}) handler registered AFTER this one from seeing the event. Since installDeckInterleavedRaceFilter is called from initDeck() — which runs after app startup — any error monitoring tool or debug hook wired up later would be silently blocked for this error class. Dropping stopImmediatePropagation would still achieve the stated goal while leaving the event visible to later handlers.
Reviewer found that PR #3833's first cut was incomplete — the `shouldLoad('tech-readiness')` gate I added in data-loader was silently bypassed on initial boot, and a parallel auto-refresh path in panel-layout was never gated at all. P1 #1 — data-loader.ts:602 `shouldLoad(id)` returns `forceAll || isPanelNearViewport(id)` at data-loader.ts:444. App.ts:1226 calls `loadAllData(true)` on startup, forcing `shouldLoad` to true on every variant — so commodity/finance/ energy were still enqueueing `TechReadinessPanel.refresh()` at boot. P1 #2 — panel-layout.ts:1278 The `lazyPanel('tech-readiness', ...)` factory calls `void p.refresh()` unconditionally inside the dynamic import. Because App.ts:577-583 merges ALL_PANELS into panelSettings on every variant for cross-variant pref carryover, `shouldCreatePanel('tech-readiness')` (just a key- existence check at panel-layout.ts:854) is true everywhere — and the hide-disabled path at panel-layout.ts:2028-2030 runs AFTER the import's refresh has already fired the 5s `/api/bootstrap?keys=techReadiness` fetch. So the variant gate has to live inside the factory, not after. Fix — introduce `isPanelInVariantDefaults(key)` helper in src/config/panels.ts that returns `VARIANT_DEFAULTS[SITE_VARIANT].includes(key)`. Use it in both auto-refresh paths: data-loader.ts:602 if (isPanelInVariantDefaults('tech-readiness') && shouldLoad('tech-readiness')) panel-layout.ts:1278 (factory body) if (isPanelInVariantDefaults('tech-readiness')) { void p.refresh(); } The panel is still CREATED on all variants so users who opt-in via settings can still see and use it — but the eager fetch only fires where the bootstrap seeder actually populates the key (full + tech). Regression test — tests/tech-readiness-variant-gate.test.mts uses a line-walker (not a strict regex) to assert: 1. data-loader's techReadiness task is gated by isPanelInVariantDefaults 2. panel-layout's lazyPanel factory wraps p.refresh() in the same gate 3. the helper is exported from the @/config barrel Mutation-tested locally: reverting either gate makes the test fail with a clear diagnostic naming the bypassed location. Note — national-debt at panel-layout.ts:1286 has the same factory shape (eager `void p.refresh()` on a variant-restricted panel that's only in FULL_PANELS). Left for a follow-up so this PR stays scoped to the reviewer's findings.
|
Pushed P1 #1 — P1 #2 — Helper — added Regression test — Noted, not fixed — |
1 participant
Summary
Two independent fixes bundled into one PR — both are surgical, single-file, low-blast-radius.
#1 — deck.gl/maplibre interleaved-mode render race (
src/components/DeckGLMap.ts)Module-scoped
installDeckInterleavedRaceFilter()adds a one-shot capture-phasewindow.addEventListener('error', …)that suppressesTrigger: deck
setProps({layers})finalizes a layer between_resolveLayersandrenderLayers; the next maplibre repaint hits the freed slot.MapboxOverlay.onErrorcan't catch this — maplibre, not deck, owns the throwing callstack.initDeck()— idempotent onrecreateWithFallback/ HMR.deck-stack-*.jschunk filename so a first-party.idcrash still surfaces.beforeSendinmain.ts:313-315already drops this exact pattern for telemetry, so the change is pure console-noise removal.#5 — tech-readiness refresh storm on energy/finance/commodity variants (
src/app/data-loader.ts)Replaced
with
matching the
thermal-escalationgate one line below.Why this fixes the 5 s timeout:
App.ts:576-583mergesALL_PANELSintopanelSettingson every variant for cross-variant pref carryover, soshouldCreatePanel('tech-readiness')istrueeverywhere — but the bootstrap seed key only exists onfull+techvariants. The unconditionalrefresh()atservices/economic/index.ts:694therefore times out on every other variant's data-loader cycle. Viewport-gating prevents the fan-out from firing on variants whose panel will never be visible.Test plan
npm run typecheck:api, boundaries, premium-fetch parity — already green from push hooks)techReadinessin data-loader logsCannot read properties of nullin DevTools consoledeck-stack-*.idevent continues to be dropped client-side (no console error) AND continues to be filtered in telemetry