feat(energy-atlas): seed-side countries[] denorm on disruptions + CountryDeepDive row (§R #5 = B)#3377
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
Greptile SummaryThis PR denormalises Confidence Score: 5/5Safe to merge — all findings are P2 style/robustness suggestions with no present defects. The denorm logic is correct, the asset-registry join is validated by validateRegistry with hard-failure semantics, both orphaned assetId fixes are confirmed present in their registries, the proto field is additive, and the server projection is consistent with sibling handlers. All three inline comments are cosmetic. scripts/_energy-disruption-registry.mjs — the gas+oil spread merge warrants a duplicate-key guard before future registry growth makes it risky. Important Files Changed
Sequence DiagramsequenceDiagram
participant Cron as Weekly Cron
participant Reg as _energy-disruption-registry.mjs
participant PipReg as pipelines-{gas,oil}.json
participant StoreReg as storage-facilities.json
participant Redis as Redis (energy:disruptions:v1)
participant Server as list-energy-disruptions.ts
participant Panel as CountryDeepDivePanel
Cron->>Reg: buildPayload()
Reg->>PipReg: loadAssetRegistries()
Reg->>StoreReg: loadAssetRegistries()
PipReg-->>Reg: { pipelines: {id->fromCountry,toCountry,...} }
StoreReg-->>Reg: { facilities: {id->country} }
Reg->>Reg: deriveCountriesForEvent(event, assets) per event
Reg->>Redis: write { events: { ...event, countries[] } }
Panel->>Server: listEnergyDisruptions({}) no filter
Server->>Redis: getCachedJson(ENERGY_DISRUPTIONS_KEY)
Redis-->>Server: raw registry
Server->>Server: projectDisruption() coerceStringArray(countries)
Server-->>Panel: { events: [...EnergyDisruptionEntry] }
Panel->>Panel: filter e.countries.includes(iso2)
Panel->>Panel: appendAtlasRow(Energy disruptions in iso2)
Reviews (1): Last reviewed commit: "chore(proto): regenerate openapi specs f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+63
to
+65
| pipelines: { ...(gas.pipelines ?? {}), ...(oil.pipelines ?? {}) }, | ||
| storage: storageRaw.facilities ?? {}, | ||
| }; |
Contributor
There was a problem hiding this comment.
The spread merge { ...(gas.pipelines ?? {}), ...(oil.pipelines ?? {}) } silently lets oil entries overwrite gas entries when the same ID key exists in both files. Today there is no overlap (verified), but if a curator ever adds a pipeline to both pipelines-gas.json and pipelines-oil.json with identical IDs, deriveCountriesForEvent will silently produce wrong country data for the overwritten entry — and no test or validation step will surface the collision.
Consider asserting uniqueness at load time:
const allPipelines = {};
for (const [id, p] of Object.entries(gas.pipelines ?? {})) allPipelines[id] = p;
for (const [id, p] of Object.entries(oil.pipelines ?? {})) {
if (allPipelines[id]) throw new Error(`Duplicate pipeline id in both gas+oil registries: ${id}`);
allPipelines[id] = p;
}
Comment on lines
+1270
to
+1283
| private async loadDisruptionsForCountry(iso2: string): Promise<void> { | ||
| try { | ||
| const { SupplyChainServiceClient } = await import( | ||
| '@/generated/client/worldmonitor/supply_chain/v1/service_client' | ||
| ); | ||
| const { getRpcBaseUrl } = await import('@/services/rpc-client'); | ||
| const client = new SupplyChainServiceClient(getRpcBaseUrl(), { | ||
| fetch: (...args: Parameters<typeof fetch>) => globalThis.fetch(...args), | ||
| }); | ||
| const res = await client.listEnergyDisruptions({ | ||
| assetId: '', | ||
| assetType: '', | ||
| ongoingOnly: false, | ||
| }); |
Contributor
There was a problem hiding this comment.
this.abortController
The class exposes this.abortController / this.signal for lifecycle teardown, but loadDisruptionsForCountry doesn't thread the signal into the RPC client's fetch wrapper. The in-flight request will continue even after the panel is hidden or a different country is loaded. The stale-closure guard (this.currentCode !== iso2) does discard stale results, so no wrong data is rendered — but the orphaned network request can't be cancelled.
Pass the signal through the client's fetch shim:
const client = new SupplyChainServiceClient(getRpcBaseUrl(), {
fetch: (...args: Parameters<typeof fetch>) =>
globalThis.fetch(args[0], { ...(args[1] ?? {}), signal: this.abortController.signal }),
});
Comment on lines
+1295
to
+1299
| summary, | ||
| events.map(e => ({ | ||
| id: e.id, | ||
| label: `${e.eventType} — ${e.shortDescription}`, | ||
| // Event type mirrors the existing asset-detail events (pipeline / |
Contributor
There was a problem hiding this comment.
shortDescription not truncated in row label
shortDescription values in the registry can be 100–200 characters (e.g. "OFAC designated PDVSA; US refiners cut Venezuelan heavy crude intake, reshaping Latam flows."). With font-size:11px and no overflow:hidden / text-overflow:ellipsis on the row, long descriptions will overflow or wrap awkwardly. Consider clamping the label:
label: `${e.eventType} — ${e.shortDescription.length > 80 ? e.shortDescription.slice(0, 77) + '…' : e.shortDescription}`,
6 tasks
koala73
added a commit
that referenced
this pull request
Apr 24, 2026
…review P2) Three Codex P2 findings on PR #3377: 1. `loadAssetRegistries()` spread-merged gas + oil pipelines, silently overwriting entries on id collision. No collision today, but a curator adding a pipeline under the same id to both files would cause `deriveCountriesForEvent` to return wrong-commodity country data with no test flagging it. Fix: explicit merge loop that throws on duplicate id. The next cron tick fails validation, seed-meta stays stale, health alarms fire — same loud-failure pattern the rest of the seeder uses. 2. `loadDisruptionsForCountry` didn't thread `this.signal` through the RPC fetch shim. The stale-closure guard (`currentCode !== iso2`) discarded stale RESULTS, but the in-flight request couldn't be cancelled when the user switched countries or closed the panel. Fix: wrap globalThis.fetch with { signal: this.signal } in the client factory, matching the signal lifecycle the rest of the panel already uses. 3. `shortDescription` values up to 200 chars rendered without ellipsis in the compact Atlas row, overflowing the row layout. Fix: new `truncateDisruptionLabel` helper clamps to 80 chars with ellipsis. Full text still accessible via click-through to the asset drawer. Typecheck clean, test:data 6698/6698 pass.
koala73
added a commit
that referenced
this pull request
Apr 24, 2026
Two events referenced pipeline ids that do not exist in scripts/data/pipelines-oil.json: - cpc-force-majeure-2022: assetId "cpc-pipeline" → "cpc" - pdvsa-designation-2019: assetId "ve-petrol-2026-q1" → "venezuela-anzoategui-puerto-la-cruz" Without this, clicking those rows in EnergyDisruptionsPanel dead-ends at "Pipeline detail unavailable", so the panel shipped with broken navigation on real data. Mirrors the same fix on PR #3377 (gap #5a registry); applying it on this branch as well so PR #3378 is independently correct regardless of merge order. The two changes will dedupe cleanly on rebase since the edits are byte-identical.
…w (§R #5 = B) Per plan §R/#5 decision B: denormalise countries[] at seed time on each disruption event so CountryDeepDivePanel can filter events per country without an asset-registry round trip. Schema join (pipeline/storage → event.assetId) happens once in the weekly cron, not on every panel render. The alternative (client-side join) was rejected because it couples UI logic to asset-registry internals and duplicates the join for every surface that wants a per-country filter. Changes: - `proto/.../list_energy_disruptions.proto`: add `repeated string countries = 15` to EnergyDisruptionEntry with doc comment tying it to the plan decision and the always-non-empty invariant. - `scripts/_energy-disruption-registry.mjs`: • Load pipeline-gas + pipeline-oil + storage-facilities registries once per seed cycle; index by id. • `deriveCountriesForEvent()` resolves assetId to {fromCountry, toCountry, transitCountries} (pipeline) or {country} (storage), deduped + alpha-sorted so byte-diff stability holds. • `buildPayload()` attaches the computed countries[] to every event before writing. • `validateRegistry()` now requires non-empty countries[] of ISO2 codes. Combined with the seeder's `emptyDataIsFailure: true`, this surfaces orphaned assetIds loudly — the next cron tick fails validation and seed-meta stays stale, tripping health alarms. - `scripts/data/energy-disruptions.json`: fix two orphaned assetIds that the new join caught: • `cpc-force-majeure-2022`: `cpc-pipeline` → `cpc` (matches the entry in pipelines-oil.json). • `pdvsa-designation-2019`: `ve-petrol-2026-q1` (non-existent) → `venezuela-anzoategui-puerto-la-cruz`. - `server/.../list-energy-disruptions.ts`: project countries[] into the RPC response via coerceStringArray. Legacy pre-denorm rows surface as empty array (always present on wire, length 0 => old). - `src/components/CountryDeepDivePanel.ts`: add 4th Atlas row — "Energy disruptions in {iso2}" — filtered by `iso2 ∈ countries[]`. Failure is silent; EnergyDisruptionsPanel (upcoming) is the primary disruption surface. - `tests/energy-disruptions-registry.test.mts`: switch to validating the buildPayload output (post-denorm), add §R #5 B invariant tests, plus a raw-JSON invariant ensuring curators don't hand-edit countries[] (it's derived, not declared). Proto regen note: `make generate` currently fails with a duplicate openapi plugin collision in buf.gen.yaml (unrelated bug — 3 plugin entries emit to the same out dir). Worked around by temporarily trimming buf.gen.yaml to just the TS plugins for this regen. Added only the `countries: string[]` wire field to both service_client and service_server; no other generated-file drift in this PR.
Runs `make generate` with the sebuf v0.11.1 plugin now correctly resolved via the PATH fix (cherry-picked from fix/makefile-generate-path-prefix). The new `countries` field on EnergyDisruptionEntry propagates into: - docs/api/SupplyChainService.openapi.yaml (primary per-service spec) - docs/api/SupplyChainService.openapi.json (machine-readable variant) - docs/api/worldmonitor.openapi.yaml (consolidated bundle) No TypeScript drift beyond the already-committed service_client.ts / service_server.ts updates in 80797e7.
Codex P2: loadDisruptionsForCountry dispatched `highlightEventId` but
neither PipelineStatusPanel nor StorageFacilityMapPanel consumes it
(the openDetailHandler reads only pipelineId / facilityId). The UI's
implicit promise (event-specific highlighting) wasn't delivered —
clickthrough was asset-generic, and the extra wire field was a
misleading API surface.
Fix: emit only {pipelineId, facilityId} in the dispatched detail.
Row click opens the asset drawer; user sees the full per-asset
disruption timeline and locates the event visually.
Symmetric fix for PR #3378's EnergyDisruptionsPanel — both emitters
now match the drawer contract exactly. Re-add `highlightEventId`
here when the drawer panels ship matching consumer code
(openDetailHandler accepts it, loadDetail stores it,
renderDisruptionTimeline scrolls + emphasises the matching event).
Typecheck clean, test:data 6698/6698 pass.
…review P2) Three Codex P2 findings on PR #3377: 1. `loadAssetRegistries()` spread-merged gas + oil pipelines, silently overwriting entries on id collision. No collision today, but a curator adding a pipeline under the same id to both files would cause `deriveCountriesForEvent` to return wrong-commodity country data with no test flagging it. Fix: explicit merge loop that throws on duplicate id. The next cron tick fails validation, seed-meta stays stale, health alarms fire — same loud-failure pattern the rest of the seeder uses. 2. `loadDisruptionsForCountry` didn't thread `this.signal` through the RPC fetch shim. The stale-closure guard (`currentCode !== iso2`) discarded stale RESULTS, but the in-flight request couldn't be cancelled when the user switched countries or closed the panel. Fix: wrap globalThis.fetch with { signal: this.signal } in the client factory, matching the signal lifecycle the rest of the panel already uses. 3. `shortDescription` values up to 200 chars rendered without ellipsis in the compact Atlas row, overflowing the row layout. Fix: new `truncateDisruptionLabel` helper clamps to 80 chars with ellipsis. Full text still accessible via click-through to the asset drawer. Typecheck clean, test:data 6698/6698 pass.
koala73
force-pushed
the
feat/atlas-disruption-countries-denorm
branch
from
April 24, 2026 15:04
6f7105d to
ee24089
Compare
koala73
added a commit
that referenced
this pull request
Apr 24, 2026
#3378) * feat(energy-atlas): EnergyDisruptionsPanel standalone timeline (§L #4) Closes gap #4 from docs/internal/energy-atlas-registry-expansion.md §L. Before this PR, the 52 disruption events in `energy:disruptions:v1` were only reachable by drilling into a specific pipeline or storage facility — PipelineStatusPanel and StorageFacilityMapPanel each render an asset-scoped slice of the log inside their drawers, but no surface listed the global event log. This panel makes the full log first-class. Shape: - Reverse-chronological table (newest first) of every event. - Filter chips: event type (sabotage, sanction, maintenance, mechanical, weather, war, commercial, other) + "ongoing only" toggle. - Row click dispatches the existing `energy:open-pipeline-detail` or `energy:open-storage-facility-detail` CustomEvent with `{assetId, highlightEventId}` — no new open-panel protocol introduced. Mirrors the CountryDeepDivePanel disruption row contract from PR #3377. - Uses `src/shared/disruption-timeline.ts` formatters (formatEventWindow, formatCapacityOffline, statusForEvent) that PipelineStatus/StorageFacilityMap already use — consistent UI across all three disruption surfaces. Wiring: - `src/components/EnergyDisruptionsPanel.ts` — new (~230 lines). - `src/components/index.ts` — export. - `src/app/panel-layout.ts` — `this.createPanel('energy-disruptions', () => new EnergyDisruptionsPanel())` alongside the other three atlas panels at :892. - `src/config/panels.ts` — add to `FULL_PANELS` (priority 2, next to fuel-shortages) + `ENERGY_PANELS` (priority 1, top tier) + `PANEL_CATEGORY_MAP.marketsFinance` list alongside the other atlas panels. - `src/config/commands.ts` — CMD+K entry `panel:energy-disruptions` with keywords matching the user vocabulary (sabotage, sanctions events, force majeure, drone strike, nord stream sabotage). Not done in this PR: - No new map pin layer — per plan §Q (Codex approved), disruptions stay a tabular/timeline surface; map assets (pipelines + storage) already show disruption markers on click. - No direct globe-mode or SVG-fallback rendering needs — panel is pure DOM, not a map layer. Test plan: - [x] npm run typecheck (clean) - [x] npm run test:data (6694/6694 pass) - [ ] Manual: CMD+K "disruption log" → panel opens with 52 events, newest first. Click "Sabotage" chip → narrows to sabotage events only. Click a Nord Stream row → PipelineStatusPanel opens with that event highlighted. * fix(energy-atlas): drop highlightEventId emission + respect empty-state (review P2) Two Codex P2 findings on this PR: 1. Row click dispatched `highlightEventId` but neither PipelineStatusPanel nor StorageFacilityMapPanel consumes it. The UI's implicit promise (event-specific highlighting) wasn't delivered — clickthrough was asset-generic, and the extra field on the wire was a misleading API surface. Fix: drop `highlightEventId` from the dispatched detail. Row click now opens the asset drawer with just {pipelineId, facilityId}, the fields the receivers actually consume. User sees the full disruption timeline for that asset and locates the event visually. A future PR can add real highlight support by: - drawers accept `highlightEventId` in their openDetailHandler - loadDetail stores it and renderDisruptionTimeline scrolls + emphasises the matching event - re-add `highlightEventId` to the dispatch here, symmetrically in CountryDeepDivePanel (which has the same wire emission) The internal `_eventId` parameter is kept as a plumb-through so that future work is a drawer-side change, not a re-plumb. 2. `events.length === 0` was conflated with `upstreamUnavailable` and triggered the error UI. The server contract (list-energy-disruptions handler) returns `upstreamUnavailable: false` with an empty events array when Redis is up but has no entries matching the filter — a legitimate empty state, not a fetch failure. Fix: gate `showError` on `upstreamUnavailable` alone. Empty results fall through to the normal render, where the table's `No events match the current filter` row already handles the case. Typecheck clean, test:data 6694/6694 pass. * fix(energy-atlas): event delegation on persistent content (review P1) Codex P1: Panel.setContent() debounces the DOM write by 150ms (see Panel.ts:1025), so attaching listeners in render() via `this.element.querySelector(...)` targets the STALE DOM — chips, rows, and the ongoing-toggle button are silently non-interactive. Visually the panel renders correctly after the debounce fires, but every click is permanently dead. Fix: register a single delegated click handler on `this.content` (persistent element) in the constructor. The handler uses `closest('[data-filter-type]')`, `closest('[data-toggle-ongoing]')`, and `closest('tr.ed-row')` to route by data-attribute. Works regardless of when setContent flushes or how many times render() re-rewrites the inner HTML. Also fixes Codex P2 on the same PR: filterEvents() was called twice per render (once for row HTML, again for filteredCount). Now computed once, reused. Trivial for 52 events but eliminates the redundant sort. Typecheck clean. * fix(energy-atlas): remap orphan disruption assetIds to real pipelines Two events referenced pipeline ids that do not exist in scripts/data/pipelines-oil.json: - cpc-force-majeure-2022: assetId "cpc-pipeline" → "cpc" - pdvsa-designation-2019: assetId "ve-petrol-2026-q1" → "venezuela-anzoategui-puerto-la-cruz" Without this, clicking those rows in EnergyDisruptionsPanel dead-ends at "Pipeline detail unavailable", so the panel shipped with broken navigation on real data. Mirrors the same fix on PR #3377 (gap #5a registry); applying it on this branch as well so PR #3378 is independently correct regardless of merge order. The two changes will dedupe cleanly on rebase since the edits are byte-identical.
3 tasks
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
Per the Codex-approved plan §R/#5 decision B: denormalise `countries[]` at seed time on each disruption event so `CountryDeepDivePanel` can filter per country without an asset-registry round trip. The schema join (pipeline/storage → event.assetId) happens once in the weekly cron, not on every panel render.
The alternative (client-side join) was rejected because it couples UI logic to asset-registry internals and duplicates the join for every surface that wants a per-country filter.
Changes
Proto + registry
Data cleanup (caught by the new join)
Server + UI
Tests
Build-infra dependency
This PR carries a cherry-picked commit from #3371 (`fix(build): use first GOPATH entry for plugin PATH prefix`) because the pre-push proto-freshness hook runs `make generate`, which previously failed on machines with a stale Homebrew-installed sebuf v0.7.0 in PATH. The cherry-pick will deduplicate cleanly on rebase once #3371 merges.
Test plan
Context
Part of `docs/internal/energy-atlas-registry-expansion.md` §L gap closure. Closes gap #5B. Unblocks gap #4 (`EnergyDisruptionsPanel`) which shares the `listEnergyDisruptions` client usage.
Related: #3366 (Atlas map layers on FULL variant, open), #3371 (build infra fix, open).