fix: address greptile-apps P0/P1 review comments on PR #2646#2663
fix: address greptile-apps P0/P1 review comments on PR #2646#2663fuleinist wants to merge 6 commits intokoala73:mainfrom
Conversation
- Create seed-climate-zone-normals.mjs to fetch 1991-2020 historical monthly means from Open-Meteo archive API per zone - Update seed-climate-anomalies.mjs to use WMO normals as baseline instead of climatologically meaningless 30-day rolling window - Add 7 new climate-specific zones: Arctic, Greenland, WestAntarctic, TibetanPlateau, CongoBasin, CoralTriangle, NorthAtlantic - Register climateZoneNormals cache key in cache-keys.ts - Add fallback to rolling baseline if normals not yet cached Fixes: koala73#2467
- seed-climate-zone-normals.mjs: Now fetches normals for ALL 22 zones (15 original geopolitical + 7 new climate zones) instead of just the 7 new climate zones. The 15 original zones were falling through to the broken rolling fallback. - seed-climate-anomalies.mjs: Fixed rolling fallback to fetch 30 days of data when WMO normals are not yet cached. Previously fetched only 7 days, causing baselineTemps slice to be empty and returning null for all zones. Now properly falls back to 30-day rolling baseline (last 7 days vs. prior 23 days) when normals seeder hasn't run. - cache-keys.ts: Removed climateZoneNormals from BOOTSTRAP_CACHE_KEYS. This is an internal seed-pipeline artifact (used by the anomaly seeder to read cached normals) and is not meant for the bootstrap endpoint. Only climate:anomalies:v1 (the final computed output) should be exposed to clients. Fixes greptile-apps P1 comments on PR koala73#2504.
…acement tiers Fixes algorithmic bias where China scores comparably to active conflict states due to Math.min(60, linear) compression in HAPI fallback. Changes: - HAPI fallback: Math.min(60, events * 3 * mult) → Math.min(60, log1p(events * mult) * 12) Preserves ordering: Iran (1549 events) now scores >> China (46 events) - Displacement tiers: 2 → 6 tiers (10K/100K/500K/1M/5M/10M thresholds) Adds signal for Syria's 5.65M outflow vs China's 332K Addresses koala73#2457 (point 1 and 3 per collaborator feedback)
- P1: seed-climate-zone-normals validate now requires >= ceil(22*2/3)=15 zones instead of >0. Partial seeding (e.g. 3/22) was passing validation and writing a 30-day TTL cache that would cause the anomalies seeder to throw on every run until cache expiry. - P2: Extract shared zone definitions (ZONES, CLIMATE_ZONES, ALL_ZONES, MIN_ZONES) into scripts/_climate-zones.mjs. Both seeders now import from the same source, eliminating the risk of silent divergence. - P2: seed-climate-anomalies currentMonth now uses getUTCMonth() instead of getMonth() to avoid off-by-one at month boundaries when the Railway container's local timezone differs from UTC. Reviewed-by: greptile-apps
…y feed Edge relay forwarded raw Railway relay response unchanged. If relay returns messages[] instead of items[], the browser panel reads response.items || [] = [] silently and shows zero items. The cache TTL check also misfires on messages[] payloads. Fix: always normalize to items[] before forwarding. Cache check now uses normalized relayItems array length instead of checking the potentially-absent items key. Addresses koala73#2593
P0 (deploy-time): Remove TypeScript type annotation from Edge Function .js file - api/telegram-feed.js: 'let normalizedBody: string' → 'let normalizedBody' (esbuild treats .js as JS, TS annotation would cause parse failure) P1 (silent data loss): seed-climate-anomalies.mjs now logs when a zone is dropped due to insufficient rolling-fallback data (baselineTemps < 7d) instead of silently returning null. P1 (CII scoring): country-instability.ts: move multiplier outside log scale so it provides linear amplification rather than sub-linear compression: Math.log1p(events * mult) * 12 → Math.log1p(events) * mult * 12 (Iran 1549 events still >> China 46 events) P2 (partial cache poisoning): seed-climate-zone-normals.mjs now fails early if fewer than MIN_ZONES (15) zones are fetched instead of only failing when ALL zones fail (length === 0). Prevents a partial write that would cause the anomalies seeder to throw on every run for 30 days. Addresses greptile-apps review comments on koala73#2646
|
@fuleinist is attempting to deploy a commit to the Elie Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR addresses the P0/P1/P2 review comments from #2646, covering four distinct fixes: removing a TypeScript type annotation from a All four stated fixes are correctly implemented and the architectural approach (shared zone definitions, Redis-cached normals, graceful fallback) fits the existing seeder patterns well. Three items worth noting:
Confidence Score: 4/5Safe to merge — all four stated fixes are correct; remaining notes are P2 style/observability concerns that don't affect correctness. The P0 deploy-blocker, two P1 data-quality bugs, and the P2 cache-poisoning guard are all properly addressed. The new zone-normals seeder is well-guarded with MIN_ZONES and follows established runSeed patterns. The only concerns are a doubled response payload in the relay normalization path, a misleading sourceVersion label in seed metadata, and undescribed displacement-boost tier changes — none of which cause runtime errors or data loss. api/telegram-feed.js (payload duplication when relay uses messages[]) and src/services/country-instability.ts (undescribed displacement boost tiers). Important Files Changed
Sequence DiagramsequenceDiagram
participant Cron as Railway Cron (monthly)
participant ZN as seed-climate-zone-normals.mjs
participant OMArchive as Open-Meteo Archive API
participant Redis as Upstash Redis
participant AnomalyCron as Railway Cron (3-hourly)
participant CA as seed-climate-anomalies.mjs
participant SPA as Frontend SPA
rect rgb(220, 240, 255)
Note over Cron,Redis: Monthly: Seed WMO 1991-2020 normals
Cron->>ZN: trigger
loop 22 zones x 30 years
ZN->>OMArchive: GET /v1/archive (year data)
OMArchive-->>ZN: daily temps + precip
end
ZN->>ZN: aggregate to monthly means
ZN->>Redis: SET climate:zone-normals:v1 (TTL 30d)
end
rect rgb(220, 255, 220)
Note over AnomalyCron,SPA: 3-hourly: Compute anomalies
AnomalyCron->>CA: trigger
CA->>Redis: GET climate:zone-normals:v1
alt normals available
Redis-->>CA: zones[] with monthly means
CA->>OMArchive: GET last 7 days per zone (22 zones)
OMArchive-->>CA: recent temps + precip
CA->>CA: delta = recent - WMO normal for current month
else normals missing (first run / expired)
Redis-->>CA: null
CA->>OMArchive: GET last 30 days per zone (22 zones)
OMArchive-->>CA: 30-day data
CA->>CA: delta = last 7d - prior 23d (rolling fallback)
end
CA->>CA: check anomalies.length >= MIN_ZONES (15)
CA->>Redis: SET climate:anomalies:v1 (TTL 3h)
SPA->>Redis: bootstrap / poll climate:anomalies:v1
Redis-->>SPA: anomalies[]
end
Reviews (1): Last reviewed commit: "fix: address greptile-apps P0/P1 review ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| : Array.isArray(parsed.items) ? parsed.items | ||
| : []; | ||
| isEmpty = parsed.count === 0 || relayItems.length === 0; | ||
| normalizedBody = JSON.stringify({ ...parsed, items: relayItems }); |
There was a problem hiding this comment.
messages key alongside new items key
When the relay returns { count: N, messages: [...] }, the spread { ...parsed, items: relayItems } will produce { count: N, messages: [...], items: [...] } — both messages and items containing the same data. This roughly doubles the payload for that code path.
Since the downstream SPA exclusively reads items, you can drop messages by cherry-picking only the fields you need, or explicitly deleting it:
| normalizedBody = JSON.stringify({ ...parsed, items: relayItems }); | |
| const { messages: _msg, ...rest } = parsed; | |
| normalizedBody = JSON.stringify({ ...rest, items: relayItems }); |
There was a problem hiding this comment.
Fixed: Refactored to cherry-pick only the needed fields and exclude the duplicate messages key. Changed from { ...parsed, items: relayItems } to:
This drops messages from the output payload when parsed.messages was the source, keeping only items — payload size is back to normal.
| runSeed('climate', 'anomalies', CANONICAL_KEY, fetchClimateAnomalies, { | ||
| validateFn: validate, | ||
| ttlSeconds: CACHE_TTL, | ||
| sourceVersion: 'open-meteo-archive-30d', | ||
| sourceVersion: 'open-meteo-archive-wmo-normals', | ||
| }).catch((err) => { |
There was a problem hiding this comment.
sourceVersion is always 'open-meteo-archive-wmo-normals' even when rolling fallback is used
The sourceVersion option is a hardcoded string, so seed metadata (written by runSeed → writeFreshnessMetadata) will always report open-meteo-archive-wmo-normals — even on the first run before the zone-normals seeder has populated Redis, when hasNormals is false and the actual baseline is the 30-day rolling window.
This misleads health-monitoring dashboards that read seed-meta:climate:anomalies. Consider computing sourceVersion dynamically:
runSeed('climate', 'anomalies', CANONICAL_KEY, fetchClimateAnomalies, {
validateFn: validate,
ttlSeconds: CACHE_TTL,
sourceVersion: hasNormals ? 'open-meteo-archive-wmo-normals' : 'open-meteo-archive-30d-rolling',Since hasNormals is determined inside fetchClimateAnomalies, you'll need to hoist it to module scope or restructure slightly, but it avoids the stale label.
There was a problem hiding this comment.
Fixed: sourceVersion is now computed dynamically based on whether WMO normals are available. The seed script now passes:
When hasNormals is false (Redis zone-normals not yet populated), the metadata correctly reports open-meteo-archive-30d-rolling.
| const displacementBoost = data.displacementOutflow >= 10_000_000 ? 12 | ||
| : data.displacementOutflow >= 5_000_000 ? 10 | ||
| : data.displacementOutflow >= 1_000_000 ? 8 | ||
| : data.displacementOutflow >= 500_000 ? 6 | ||
| : data.displacementOutflow >= 100_000 ? 4 | ||
| : data.displacementOutflow >= 10_000 ? 2 | ||
| : 0; |
There was a problem hiding this comment.
The PR description's summary table only lists the Math.log1p multiplier fix, but this diff also adds three new displacement-boost thresholds (>= 10_000_000 → 12, >= 500_000 → 6, >= 10_000 → 2) in both calculateCII (here) and getCountryScore (line 1068–1074). These are behavioral score changes that affect every country with displacement data.
This isn't a bug — the logic looks reasonable — but bundling undescribed score-calibration changes with P0/P1 fixes makes it harder to audit regressions later. Calling it out so reviewers can explicitly sign off.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Acknowledged. You're right that the expanded displacement boost tiers weren't called out in the original PR description. The extra tiers (≥500_000→6, ≥10_000→2, ≥10_000_000→12) were included in the same commit as the Math.log1p fix because they affect the same calculateCII and getCountryScore functions — they improve score calibration for mid-range displacement countries and mega-outflow events.
For transparency, the three new tiers are:
≥10,000,000displacement → 12 (new)≥5,000,000displacement → 10 (new)≥10,000displacement → 2 (new)
The existing tiers (≥1,000,000→8, ≥100,000→4) are unchanged. This is a behavioral change that should be reviewed before merging — noted for the PR reviewer.
1 participant
Summary
Addresses critical and high-priority review comments from greptile-apps on PR #2646 (Telegram feed normalization + bundled climate/CII changes).
Changes
let normalizedBody: stringwould cause a parse/deploy failureMath.log1p(events * mult) * 12→Math.log1p(events) * mult * 12Testing
node --check api/telegram-feed.js✅node --check scripts/seed-climate-anomalies.mjs✅node --check scripts/seed-climate-zone-normals.mjs✅Reviews
Supersedes / complements greptile-apps review on #2646.
Reviews: greptile-apps (P0, P1, P2)
Fixes: #2646