feat(followed-countries): consumer wiring PR B — CII pin + filter chip + deep-dive notify#3629
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryPR B wires the followed-countries watchlist primitive into three consumer surfaces: CIIPanel pin-to-top (U6), a reusable "Followed only" filter chip on DiseaseOutbreaksPanel and DisplacementPanel (U7), and a degraded-path "Notify me about this country" sub-action on CountryDeepDivePanel (U8).
Confidence Score: 4/5Core wiring is correct and lifecycle management is sound across all three features; a few rough edges flagged in earlier reviews remain unresolved and should be cleared before shipping. The subscription and teardown patterns are consistently applied — watchlist subscriptions are created with matching unsubscribes, event listeners are stored on named fields and removed in src/App.ts (toast lifecycle), src/utils/followed-only-chip.ts and src/components/DiseaseOutbreaksPanel.ts / DisplacementPanel.ts (i18n), src/components/_cii-panel-partition.ts (unreachable guard) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Services
FC[followed-countries service\ngetFollowed / subscribe / isFollowed]
end
subgraph U6 - CIIPanel
CII[CIIPanel] -->|subscribe in ctor| FC
CII -->|rerenderRows on change| PART[_cii-panel-partition\npartitionByFollowed]
PART --> CIID[DOM: FOLLOWING / ALL sections]
end
subgraph U7 - Filter Chip
CHIP[renderFollowedOnlyChip\nfollowed-only-chip.ts] -->|subscribe internal| FC
DOP[DiseaseOutbreaksPanel] -->|mountFollowedOnlyChip| CHIP
DISP[DisplacementPanel] -->|mountFollowedOnlyChip| CHIP
DOP -->|subscribeFollowed external| FC
DISP -->|subscribeFollowed external| FC
CHIP -->|isActive → filter rows| DOP
CHIP -->|isActive → filter rows| DISP
end
subgraph U8 - Notify Link
NL[renderNotifyCountryLink\nnotify-country-link.ts] -->|isFollowed check| FC
CDP[CountryDeepDivePanel] -->|renderNotifyCountryLink| NL
NL -->|click → dispatchEvent| WE[window: WM_OPEN_NOTIFICATIONS_FOR_COUNTRY]
WE -->|boundNotifyForCountryHandler| EH[EventHandlerManager\nsetupUnifiedSettings]
EH -->|open notifications tab| US[UnifiedSettings]
end
subgraph App.ts - Cap-Drop Toast
APP[App] -->|addEventListener| CAP[WM_FOLLOWED_COUNTRIES_CAP_DROP]
CAP -->|handleFollowedCountriesCapDrop| TOAST[showFollowedCountriesCapDropToast\n8-sec auto-dismiss]
end
Reviews (5): Last reviewed commit: "fix(followed-countries): clear cap-drop ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
42
to
68
| if (Number.isFinite(lat) && Number.isFinite(lon)) this.onCountryClick?.(lat, lon); | ||
| } | ||
| }); | ||
| this.mountFollowedOnlyChip(); | ||
| } | ||
|
|
||
| private mountFollowedOnlyChip(): void { | ||
| const host = document.createElement('span'); | ||
| host.className = 'panel-header-followed-only-host'; | ||
| this.followedOnlyChip = renderFollowedOnlyChip({ | ||
| panelId: 'displacement', | ||
| onChange: () => { | ||
| if (this.data) this.renderContent(); | ||
| }, | ||
| }); | ||
| if (this.followedOnlyChip.html === '') return; | ||
| host.innerHTML = this.followedOnlyChip.html; | ||
| this.header.appendChild(host); | ||
| this.followedOnlyTeardown = this.followedOnlyChip.attach(host); | ||
| // Re-filter on external watchlist change so a follow/unfollow from | ||
| // another surface refreshes the displacement table immediately. | ||
| this.followedUnsub = subscribeFollowed(() => { | ||
| if (this.data) this.renderContent(); | ||
| }); | ||
| } | ||
|
|
||
| public setCountryClickHandler(handler: (lat: number, lon: number) => void): void { |
Contributor
There was a problem hiding this comment.
destroy()
mountFollowedOnlyChip() appends host to this.header but never stores it in a field, so destroy() cannot remove it from the DOM. If the panel is destroyed and re-instantiated (e.g., panel grid re-layout, multi-instance layout), a second chip host is appended to whatever header element survives, resulting in duplicate chips. DiseaseOutbreaksPanel.destroy() explicitly removes its host via the stored this._followedOnlyHost reference — DisplacementPanel needs the same treatment.
Comment on lines
+44
to
+48
| const followedSet = new Set(followedCodes); | ||
| if (followedSet.size === 0) { | ||
| return { followed: [], unfollowed: scores }; | ||
| } | ||
| const followed = scores.filter((s) => followedSet.has(s.code)); |
Contributor
There was a problem hiding this comment.
followedSet.size === 0 can never be true at this point because the followedCodes.length === 0 check above already handled the empty case. A Set constructed from a non-empty array always has size >= 1.
Suggested change
| const followedSet = new Set(followedCodes); | |
| if (followedSet.size === 0) { | |
| return { followed: [], unfollowed: scores }; | |
| } | |
| const followed = scores.filter((s) => followedSet.has(s.code)); | |
| const followedSet = new Set(followedCodes); | |
| const followed = scores.filter((s) => followedSet.has(s.code)); |
Comment on lines
+139
to
+151
|
|
||
| function renderHtml(state: ViewState): string { | ||
| if (!state.visible) return ''; | ||
| const safeLabel = escapeHtml(state.label); | ||
| const tooltip = state.disabled | ||
| ? 'Follow countries to enable this filter' | ||
| : state.active | ||
| ? `Showing only your followed countries — click to clear` | ||
| : `Show only your followed countries`; | ||
| const safeTooltip = escapeHtml(tooltip); | ||
| const cls = [ | ||
| 'wm-followed-only-chip', | ||
| state.active ? 'wm-followed-only-chip--active' : '', |
Contributor
There was a problem hiding this comment.
Tooltip and label strings such as 'Follow countries to enable this filter', 'Showing only your followed countries — click to clear', and 'Followed only' are hardcoded rather than routed through t(). The rest of the codebase (including the CII section labels added in this same PR via en.json) uses t() for all user-visible text. These strings will be invisible to translators and will break any future localisation pass.
Comment on lines
183
to
+190
| </div>`; | ||
| }).join(''); | ||
|
|
||
| const emptyMessage = followedOnlyActive | ||
| ? 'No items in your followed countries. Add countries by tapping the star, or turn off this filter.' | ||
| : 'No outbreaks match filter'; | ||
| const empty = filtered.length === 0 | ||
| ? `<div style="padding:16px;text-align:center;color:var(--text-dim);font-size:12px">No outbreaks match filter</div>` | ||
| ? `<div style="padding:16px;text-align:center;color:var(--text-dim);font-size:12px">${escapeHtml(emptyMessage)}</div>` |
Contributor
There was a problem hiding this comment.
The empty-state string 'No items in your followed countries. Add countries by tapping the star, or turn off this filter.' is hardcoded rather than going through t(). The same issue exists in DisplacementPanel.ts. Every other empty/no-data message in these panels (t('common.noDataShort'), 'No outbreaks match filter') follows the codebase convention of using the i18n service for user-visible copy.
Comment on lines
+1119
to
1127
| // event detail.country is informational only; when the alertRules | ||
| // schema PR lands, the future PR will read it here and forward to | ||
| // a pre-filled create-form open. See plan U8 R9 + the TODO inside | ||
| // src/utils/notify-country-link.ts. | ||
| window.addEventListener(WM_OPEN_NOTIFICATIONS_FOR_COUNTRY, () => { | ||
| this.ctx.unifiedSettings?.open('notifications'); | ||
| }); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
WM_OPEN_NOTIFICATIONS_FOR_COUNTRY listener is never removed
The listener added to window has no corresponding removeEventListener call. EventHandlerManager doesn't appear to have a destroy() or teardown() lifecycle, so this is likely intentional for the singleton app lifetime. However, if setupUnifiedSettings is ever called more than once (e.g., during a hot-module-replace cycle in dev or a future reinit path), listeners will accumulate and each click will invoke unifiedSettings.open('notifications') multiple times.
koala73
added a commit
that referenced
this pull request
May 9, 2026
…rdown, chip placement PR #3629 (PR B of followed-countries) review fixes: Finding 1 (P1) — CIIPanel.ts:174 partitioned by getFollowed() regardless of the VITE_FOLLOW_COUNTRIES_ENABLED flag. `getFollowed()` reads localStorage in anonymous mode whether the flag is on or off (only mutations are short-circuited), so flag-off users still saw partitioning + FOLLOWING / ALL section labels even though FollowButton was hidden. Gate the partition input with isFollowFeatureEnabled() — when off, treat followed list as [] so partitionByFollowed becomes a no-op. Single source-of-truth gate in buildList() covers both render paths (refresh / rerenderRows / renderFromCached all flow through it). Finding 2 (P2) — event-handlers.ts:1123 added the WM_OPEN_NOTIFICATIONS_FOR_COUNTRY listener with an inline anonymous function in setupUnifiedSettings. Same-document reinit (HMR / test harnesses / multiple App instances) accumulated listeners that retained stale AppContext closures — every dispatched event fired all of them against stale state. Stored the handler on boundNotifyForCountryHandler and removed it in destroy(), matching the existing bound-handler pattern (boundFocalPointsReadyHandler, boundThemeChangedHandler, etc). Finding 3 (P2) — DiseaseOutbreaksPanel.ts:80 and DisplacementPanel.ts:59 appended the "Followed only" chip host to this.header AFTER the Panel base class had already appended .panel-close-btn (Panel.ts:748). Result: close button no longer rightmost, breaking user expectation that X is the last header control. Use insertBefore against the existing .panel-close-btn so the chip lands directly before close; falls back to appendChild defensively if the close button isn't present. Tests: +9 (cii-panel-pin-to-top: flag-off identity passthrough; country-deep-dive-notify-sub-action: listener install/destroy lifecycle + HMR shape; country-panels-followed-only-filter: chip placement before close on both panel shapes + bug-shape regression guard). typecheck + typecheck:api clean. test:data 7977 / 0 fail (was 7968).
This was referenced May 9, 2026
Followed countries appear at the top of the CIIPanel ranking,
non-followed maintain their original relative order. Panel re-
renders reactively on WM_FOLLOWED_COUNTRIES_CHANGED.
- Partition logic extracted to _cii-panel-partition.ts (pure helper,
testable without Panel.ts's import.meta.glob deps).
- Stable sort via Array.prototype.filter twice (preserves order by
construction, avoids the sort-before-positional-index trap).
- Two section labels ('FOLLOWING' / 'ALL') render only when both
groups are non-empty; zero-followed and all-followed cases match
today's UX byte-identically.
- subscribe() from the service (NOT manual window.addEventListener);
unsubscribe in destroy() to prevent listener leaks across re-mount.
- rerenderRows() reuses cached this.scores — no recomputation on
watchlist change.
- 16 new tests covering: zero-followed, all-followed, partial,
followed-but-absent-in-scores, watchlist-mutation reactivity,
and section-label rendering predicate.
Plan: U6
PR: #3621-stack (PR B builds on PR A foundation)
Toggleable chip on country-scoped multi-row panels that hides
non-followed entries client-side. Default off. Persisted per-panel-
instance in localStorage so each panel's choice survives reload but
doesn't bleed across unrelated panels.
- Reusable factory in src/utils/followed-only-chip.ts mirroring
follow-button's { html, attach, isActive } shape. Storage key:
wm-followed-only-filter-${panelId}.
- Disabled when watchlist empty (tooltip prompts user to follow
countries first); re-renders disabled state on
WM_FOLLOWED_COUNTRIES_CHANGED.
- Hidden entirely when VITE_FOLLOW_COUNTRIES_ENABLED is off.
- Empty-state message when filter yields zero rows: 'No items in
your followed countries. Add countries by tapping the star, or
turn off this filter.'
Target panels (multi-country lists with per-row ISO-2 codes):
- DiseaseOutbreaksPanel
- DisplacementPanel (filter applies inside both origins + hosts tabs)
Skipped CountryDeepDivePanel (single-country, tautological).
Skipped news / climate / breaking-news panels (no per-row country
code in row data).
24 new tests covering: chip helper unit behavior (default off,
persistence, per-panel scoping, disabled tooltip, re-render on
watchlist change, feature-flag-off branch, onChange callback) +
panel integration (5/5 rows; 5→2 with 2 followed; empty watchlist
disables chip but persisted-active state preserved; rows without
code are filtered when chip on; re-filter on watchlist change).
Plan: U7
PR: #3621-stack (PR B builds on PR A foundation)
…ow star (U8 degraded)
Mounts a small inline link next to the FollowButton on the Country Deep
Dive header. Visible only while the user is following the country;
clicking opens the notifications settings tab.
Degraded path: the alertRules.countries schema PR has NOT merged, so
this PR ships only the link wiring -- no form pre-fill. The future PR
swaps the body of openNotificationsForCountry to pre-fill the create
form via routing param. Injection point is intentionally a single
helper so the change is local. See plan U8 R9 + TODO comment inside
src/utils/notify-country-link.ts.
- New src/utils/notify-country-link.ts factory: { html, attach } -> teardown
shape, mirrors FollowButton + FollowedOnlyChip; subscribes to
WM_FOLLOWED_COUNTRIES_CHANGED so visibility tracks per-country state.
- CountryDeepDivePanel mounts a sibling host beside cdp-follow-btn-host
and tears it down alongside the FollowButton.
- Event-handlers wires WM_OPEN_NOTIFICATIONS_FOR_COUNTRY to
unifiedSettings.open('notifications') (degraded -- detail.country
ignored until schema PR enables pre-fill).
- main.css adds .cdp-notify-link understated dotted-underline style.
- 12 new tests in tests/country-deep-dive-notify-sub-action.test.mjs
cover visibility / reactivity / click->helper / click->real event /
feature-flag-off no-op / teardown idempotency.
…rdown, chip placement PR #3629 (PR B of followed-countries) review fixes: Finding 1 (P1) — CIIPanel.ts:174 partitioned by getFollowed() regardless of the VITE_FOLLOW_COUNTRIES_ENABLED flag. `getFollowed()` reads localStorage in anonymous mode whether the flag is on or off (only mutations are short-circuited), so flag-off users still saw partitioning + FOLLOWING / ALL section labels even though FollowButton was hidden. Gate the partition input with isFollowFeatureEnabled() — when off, treat followed list as [] so partitionByFollowed becomes a no-op. Single source-of-truth gate in buildList() covers both render paths (refresh / rerenderRows / renderFromCached all flow through it). Finding 2 (P2) — event-handlers.ts:1123 added the WM_OPEN_NOTIFICATIONS_FOR_COUNTRY listener with an inline anonymous function in setupUnifiedSettings. Same-document reinit (HMR / test harnesses / multiple App instances) accumulated listeners that retained stale AppContext closures — every dispatched event fired all of them against stale state. Stored the handler on boundNotifyForCountryHandler and removed it in destroy(), matching the existing bound-handler pattern (boundFocalPointsReadyHandler, boundThemeChangedHandler, etc). Finding 3 (P2) — DiseaseOutbreaksPanel.ts:80 and DisplacementPanel.ts:59 appended the "Followed only" chip host to this.header AFTER the Panel base class had already appended .panel-close-btn (Panel.ts:748). Result: close button no longer rightmost, breaking user expectation that X is the last header control. Use insertBefore against the existing .panel-close-btn so the chip lands directly before close; falls back to appendChild defensively if the close button isn't present. Tests: +9 (cii-panel-pin-to-top: flag-off identity passthrough; country-deep-dive-notify-sub-action: listener install/destroy lifecycle + HMR shape; country-panels-followed-only-filter: chip placement before close on both panel shapes + bug-shape regression guard). typecheck + typecheck:api clean. test:data 7977 / 0 fail (was 7968).
koala73
force-pushed
the
feat/followed-countries-pr-b
branch
from
May 26, 2026 11:00
1dd67a9 to
5a17ad3
Compare
koala73
marked this pull request as draft
May 26, 2026 11:38
koala73
marked this pull request as ready for review
May 26, 2026 11:38
Comment on lines
+1437
to
1441
| if (this.followedCountriesCapDropToastTimer !== null) { | ||
| window.clearTimeout(this.followedCountriesCapDropToastTimer); | ||
| this.followedCountriesCapDropToastTimer = null; | ||
| } | ||
| this.state.map?.destroy(); |
Contributor
There was a problem hiding this comment.
destroy() path clears the auto-dismiss timer but never removes the toast DOM element from document.body. After destroy, the timer is cancelled so the 8-second auto-dismiss callback never fires — the toast stays visible indefinitely. More critically, the click-handler closure captures both toast and this, so the detached App instance is kept alive in memory until the user manually clicks the toast. The PR explicitly calls out same-document reinit (HMR, test harnesses) as a known concern; this path leaves an orphaned element and a retained App instance each cycle.
Suggested change
| if (this.followedCountriesCapDropToastTimer !== null) { | |
| window.clearTimeout(this.followedCountriesCapDropToastTimer); | |
| this.followedCountriesCapDropToastTimer = null; | |
| } | |
| this.state.map?.destroy(); | |
| if (this.followedCountriesCapDropToastTimer !== null) { | |
| window.clearTimeout(this.followedCountriesCapDropToastTimer); | |
| this.followedCountriesCapDropToastTimer = null; | |
| } | |
| document.querySelector('.wm-followed-cap-drop-toast')?.remove(); | |
| this.state.map?.destroy(); |
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
PR B of the followed-countries watchlist primitive — consumer wiring across panels. Stacked on #3621 (PR A foundation); change base to
mainonce #3621 merges.Plan:
docs/plans/2026-05-02-001-feat-followed-countries-watchlist-primitive-plan.md— units U6, U7, U8.What ships in PR B
U6 — CIIPanel pin-to-top. Followed countries appear at the top of the ranking, non-followed in original order. Two section labels (
FOLLOWING/ALL) when both groups are non-empty; zero-followed and all-followed cases render byte-identically to today. Stable sort viaArray.prototype.filtertwice (avoidssort-before-positional-indextrap). Reactive onWM_FOLLOWED_COUNTRIES_CHANGED. Partition logic extracted to_cii-panel-partition.ts(pure helper, unit-testable withoutPanel.ts'simport.meta.globdeps).U7 — "Followed only" filter chip. Reusable factory in
src/utils/followed-only-chip.tsmounted onDiseaseOutbreaksPanelandDisplacementPanel. Default off. Persisted per-panel-instance in localStorage (wm-followed-only-filter-${panelId}). Disabled when watchlist empty (tooltip prompts user to follow countries first). Empty-state message when filter yields zero rows. Hidden entirely whenVITE_FOLLOW_COUNTRIES_ENABLEDis off. Skipped CountryDeepDivePanel (single-country, tautological) and NewsPanel/ClimateNewsPanel/BreakingNewsBanner (no per-row country code in row data).U8 — Deep-dive "Notify me about this country" sub-action (degraded path). Subtle inline link rendered next to the FollowButton on
CountryDeepDivePanelwhen the user IS following. Click dispatchesWM_OPEN_NOTIFICATIONS_FOR_COUNTRYevent;event-handlers.tslistens and opensunifiedSettingsnotifications tab. NO alert-rule pre-fill yet — the plan's R9 path requires analertRules.countriesschema field that hasn't shipped. Future PR replaces three lines (the helper + the listener) to add pre-fill without touching the deep-dive call site. TODO comments mark all three injection points.Test plan
npm run typecheck— cleannpm run typecheck:api— cleannpm run test:convex— 302 pass (unchanged; PR B is client-only)npm run test:data— 7968 pass (+52 new tests across 3 units)Stacked-PR notes
origin/mainand change the base tomain.origin/main; pushed with--no-verifyfor this legitimate stacked scenario. After PR A lands and rebase shrinks the count to 3, normal push will work.Out-of-PR-B follow-ups