Feature/my awesome feature#3984
Conversation
|
@SipheDilima247 is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR introduces a Country Profile feature: a modal overlay showing aggregated per-country intelligence (military, economic, cyber, news), a
Confidence Score: 3/5The core country-selection flow is broken before a user can view any profile data — merging as-is ships a non-functional feature. Two independent defects both block the primary user interaction: the selector modal is rendered into a detached DOM node (invisible to the user) and
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant App
participant CPManager as CountryProfileManager
participant CSelector as CountrySelector
participant CPView as CountryProfileView
participant CPPanel as CountryProfilePanel
User->>App: openCountrySelector()
App->>CPManager: openCountrySelector()
CPManager->>CSelector: "new CountrySelector({ container: selectorContainer })"
Note over CSelector: ⚠️ selectorContainer is detached — UI never visible
CSelector-->>CPManager: (selector ready, but invisible)
User->>CSelector: selects a country
CSelector->>CPManager: onCountrySelected(code, name)
CPManager->>CPManager: closeCountrySelector()
CPManager->>CSelector: destroy()
Note over CSelector: ❌ destroy() does not exist → TypeError
CPManager->>CPView: new CountryProfileView(...)
CPView->>CPView: render() + injectStyles()
Note over CPView: keydown listener added, never removed
CPView-->>CPManager: (profile view displayed)
CPManager->>CPManager: loadCountryData(code, name)
CPManager->>CPPanel: loadCountry(code, name)
CPPanel->>CPPanel: aggregateCountryData()
Note over CPPanel: reads window.__COUNTRY_PROFILE_NEWS
CPPanel-->>CPView: renderCountryProfile(data)
Reviews (1): Last reviewed commit: "feat: update city filter with city-focus..." | Re-trigger Greptile |
| */ | ||
| public closeCountrySelector(): void { | ||
| if (this.countrySelector) { | ||
| this.countrySelector.destroy(); | ||
| this.countrySelector = null; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
CountrySelector.destroy() is missing — closes country selector unconditionally
closeCountrySelector() calls this.countrySelector.destroy(), but CountrySelector has no destroy() method defined. Every time a country is selected, selectCountry() calls closeCountrySelector(), which will immediately throw TypeError: this.countrySelector.destroy is not a function before any profile can load. This makes the country selection flow completely non-functional.
| private selectorContainer: HTMLElement; | ||
|
|
||
| constructor(appContext: AppContext) { | ||
| this.appContext = appContext; | ||
| this.selectorContainer = document.createElement('div'); |
There was a problem hiding this comment.
selectorContainer never attached to the document — selector is invisible
this.selectorContainer is created with document.createElement('div') but never appended to document.body or appContext.container. When it is passed as options.container to CountrySelector, that class uses it directly without inserting it into the live DOM tree (it only appends to document.body when no container is provided). Since the element is detached, CSS position: fixed has no effect — the rendered selector UI never appears on screen.
| data: m, | ||
| }))); |
There was a problem hiding this comment.
Duplicate city source registration
registerSource('city', getCitySearchItems()) is called twice: once in the init() flow (around line 210) and again here at the end of buildSearchItems(). Depending on SearchModal.registerSource's implementation, this either double-populates city results (showing every city twice) or silently overwrites the earlier registration with a second full copy of the same data. The second call should be removed.
| if (indicator) indicator.remove(); | ||
| this.onClose?.(); | ||
| } | ||
|
|
||
| public destroy(): void { | ||
| this.close(); |
There was a problem hiding this comment.
Keyboard event listener is never removed — accumulates on repeated open/close
keyHandler is a local variable whose reference is not stored on the instance. destroy() → close() removes the DOM container but never calls document.removeEventListener('keydown', keyHandler). Each time a CountryProfileView is opened and closed, one more stale listener remains on document. After several cycles, every Escape keypress triggers multiple close() calls on discarded instances, each dispatching this.onClose?.() unnecessarily.
| return title.includes(countryCode.toLowerCase()) || | ||
| title.includes(countryCode.toUpperCase()) || | ||
| countries.includes(countryCode.toLowerCase()); | ||
| }); | ||
|
|
||
| // Store filtered news for use in panels |
There was a problem hiding this comment.
Global
window properties used for cross-component state instead of AppContext
window.__SELECTED_COUNTRY_CODE and window.__COUNTRY_PROFILE_NEWS (also read in CountryProfilePanel as window.__COUNTRY_RISK_SCORES) bypass the established AppContext state tree. This pattern risks stale reads if components initialize before the values are set, and the globals persist in any frame/tab that shares the same window, making it impossible to safely open two country profiles across different contexts. The existing AppContext has appropriate places for this state.
| } else { | ||
| // Chrome / Firefox: lazy-load hls.js only when needed | ||
| const { default: Hls } = await import('hls.js'); | ||
| const { default: Hls } = await import('hls.js' as any); |
There was a problem hiding this comment.
The
as any cast on the module specifier suppresses TypeScript's module resolution for hls.js. With the version bump to ^1.8.0 the types should be present; if there is a genuine type error, it should be addressed in the type definitions rather than silenced here.
| const { default: Hls } = await import('hls.js' as any); | |
| const { default: Hls } = await import('hls.js'); |
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!
|
did you see the comments greptile wrote on your PR @SipheDilima247 ?? |
Yes, currently working on getting the feature working properly and fixing these bugs collectively. Will keep pushing throughout the day |
Summary
Type of change
Affected areas
/api/*)Checklist
api/rss-proxy.jsallowlist (if adding feeds)npm run typecheck)Screenshots