[weather] refactor: migrate to server-side providers with centralized HTTPFetcher#35
Open
KristjanESPERANTO wants to merge 107 commits intodevelopfrom
Open
[weather] refactor: migrate to server-side providers with centralized HTTPFetcher#35KristjanESPERANTO wants to merge 107 commits intodevelopfrom
KristjanESPERANTO wants to merge 107 commits intodevelopfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
KristjanESPERANTO
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@defaultmodules/weather/node_helper.js`:
- Around line 38-42: The code uses the user-derived identifier directly to build
providerPath and require(providerPath), which allows path traversal;
validate/sanitize identifier before use by enforcing a strict allowlist or
whitelist-regex (e.g., only alphanumeric, dash, underscore) and/or map known
provider names to filenames, then resolve the final path and verify it is inside
the providers directory (use path.resolve on providersDir and providerPath and
assert the resolved providerPath startsWith the resolved providersDir) before
calling require(providerPath); update references in node_helper.js (identifier,
providerPath, require(providerPath)) to perform these checks and fall back to a
safe default or throw a controlled error on invalid input.
In `@defaultmodules/weather/providers/openmeteo.js`:
- Around line 205-231: The switch over this.config.type can leave weatherData
undefined for unknown values; update the switch in the try block (the switch
that calls `#generateWeatherDayFromCurrentWeather`,
`#generateWeatherObjectsFromForecast`, `#generateWeatherObjectsFromHourly`) to
include a default case that either sets weatherData to a sensible fallback
(e.g., empty object/array) or throws a descriptive Error; if you choose to
throw, catch will log via Log.error and invoke this.onErrorCallback({ message:
error.message, translationKey: "MODULE_ERROR_UNSPECIFIED" }); ensure you do not
call this.onDataCallback with undefined by validating weatherData before calling
this.onDataCallback.
- Around line 411-434: The function `#generateWeatherDayFromCurrentWeather` uses
new Date().getHours() which is the server local hour and can mismatch the API
timezone; instead derive the correct hourly index from
parsedData.current_weather.time by finding that timestamp in
parsedData.hourly.time (e.g., use
parsedData.hourly.time.indexOf(parsedData.current_weather.time) or a findIndex
that normalizes formats) and use that index for parsedData.hourly[...] lookups
(ensure you fall back safely if index is -1); update references to h accordingly
so humidity, feelsLikeTemp, rain, snow, precipitationAmount,
precipitationProbability and uvIndex use the location-correct hourly entry.
In `@defaultmodules/weather/providers/openweathermap.js`:
- Around line 30-43: The initialize() method currently assumes onErrorCallback
exists and may silently swallow the error if setCallbacks() hasn't run; change
initialize() to validate that onErrorCallback is a function (e.g., typeof
this.onErrorCallback === 'function') before invoking it and, if it's missing,
throw an Error (or return a rejected Promise) with the same message so the
caller is notified; update the error handling block in initialize() (and any
helper like `#initializeFetcher`() if it relies on callbacks) to either call the
callback when present or throw/reject when it's absent, and document in comments
that setCallbacks() must be called before initialize() if you prefer keeping
callback-driven behavior.
- Around line 100-127: The switch over this.config.type in the try block (using
this.#generateWeatherObjectsFromOnecall and assigning weatherData) lacks a
default branch, so unknown types can leave weatherData undefined and pass that
to this.onDataCallback; add a default case that assigns a safe fallback (e.g.,
an empty object/array or onecallData.current) and/or logs an error before
invoking this.onDataCallback, and ensure the same behavior routes through
this.onErrorCallback if you prefer to treat unknown types as errors.
In `@defaultmodules/weather/providers/weatherflow.js`:
- Around line 13-16: The constructor currently assigns the incoming config
directly (constructor, this.config) which can leave keys like type and
updateInterval undefined; update the constructor to merge the provided config
with a sensible defaults object (e.g., defaults for type, updateInterval,
apiKey/endpoint as appropriate) and assign the merged result to this.config, and
ensure any dependent initialization (like this.fetcher creation) uses the merged
this.config values so missing fields get defaulted rather than causing undefined
behavior.
- Line 2: The import in weatherflow.js uses a relative path for HTTPFetcher;
replace the require("../../../js/http_fetcher") with the module alias
require("#http_fetcher") so HTTPFetcher is imported consistently with other
providers (locate the const HTTPFetcher declaration in weatherflow.js and update
its require to "#http_fetcher").
- Around line 258-268: The provider exposes two separate setters
setOnDataCallback and setOnErrorCallback which is inconsistent with other
providers that implement a single setCallbacks(onData, onError) API expected by
the node_helper; change the WeatherFlow provider to implement a single method
named setCallbacks(onData, onError) that assigns both this.onDataCallback and
this.onErrorCallback and keep or alias the existing
setOnDataCallback/setOnErrorCallback to call the new setCallbacks to preserve
backward compatibility, ensuring callers (and node_helper) can use the uniform
setCallbacks interface.
- Around line 62-64: The provider currently attaches fetcher.on("data", ...)
which never fires; change it to listen for the "response" event from HTTPFetcher
(fetcher.on("response", ...)), await response.json() and then pass the parsed
JSON into this.processData and this.onDataCallback, i.e. replace the "data"
handler with an async "response" handler that calls response.json() before
invoking this.processData(...) and this.onDataCallback(...).
In `@defaultmodules/weather/providers/weathergov.js`:
- Around line 303-309: The forecast.windDirection is a cardinal string (e.g.,
"NW") but weather.windFromDirection is expected to be numeric degrees; add a
private helper (e.g., `#convertWindDirection`(direction)) that maps N, NNE, NE,
... NNW to their degree values (N=0, NNE=22.5, NE=45, …, NNW=337.5) and call it
where you set weather.windFromDirection instead of assigning
forecast.windDirection directly; if the mapping returns undefined/unknown, set
windFromDirection to null (or leave as original only if you intentionally need
the string) and ensure callers of this field handle numeric or null values.
In `@defaultmodules/weather/providers/yr.js`:
- Around line 440-444: The getSunriseUrl function currently hardcodes
offset=+01:00 which breaks sunrise times outside CET; update getSunriseUrl to
use a configurable or computed timezone offset instead of the literal.
Specifically, read an offset value from this.config.offset (if present) and
otherwise compute the local offset for the device (e.g., use the local
Date.getTimezoneOffset for today and convert to the ±HH:MM format) and include
that formatted value in the URL (or omit the offset parameter when you prefer
UTC). Modify the `#getSunriseUrl` method to use this.config.offsetFallback (or
computed value) in place of the hardcoded "+01:00" so different timezones get
correct sunrise/sunset times.
In `@defaultmodules/weather/weather.js`:
- Around line 195-205: In createWeatherObject, avoid calling moment() with
undefined/null for sunrise and sunset (which yields current time); update
createWeatherObject (and the WeatherObject assignment) to only convert
date/sunrise/sunset with moment when the source values are non-null/defined
(e.g., date: data.date ? moment(data.date) : null, sunrise: data.sunrise ?
moment(data.sunrise) : null, sunset: data.sunset ? moment(data.sunset) : null)
so missing values remain null/undefined instead of becoming the current time.
🧹 Nitpick comments (13)
tests/unit/modules/default/weather/weather_providers_spec.js (1)
9-13: Consider restoring the global fetch mock between tests.The global fetch mock is set once and never restored. While this may work for these smoke tests, it could leak into other test files if they run in the same process context.
♻️ Suggested improvement
+import { afterAll } from "vitest"; + +const originalFetch = global.fetch; + // Mock global fetch for location lookup global.fetch = vi.fn(() => Promise.resolve({ ok: true, json: () => Promise.resolve({ city: "Munich", locality: "Munich" }) })); + +afterAll(() => { + global.fetch = originalFetch; +});tests/electron/helpers/weather-setup.js (2)
118-129: ReplacewaitForTimeoutwith proper wait conditions.Static analysis correctly flags
page.waitForTimeout()as an anti-pattern. These fixed delays are flaky and slow. Consider waiting for specific DOM elements or module states instead.♻️ Suggested improvement
exports.startApp = async (configFileName, systemDate, mockDataFile = "weather_onecall_current.json") => { await helpers.startApplication(configFileName, systemDate); - // Wait for modules to initialize - await global.page.waitForTimeout(1000); + // Wait for weather module to be present in DOM + await global.page.waitForSelector(".module.weather", { state: "attached" }); // Inject mock weather data await injectMockWeatherData(mockDataFile); - // Wait for rendering - await global.page.waitForTimeout(500); + // Wait for weather data to render + await global.page.waitForSelector(".weather .normal.medium", { state: "visible" }); };
14-42: Consider extracting shared test utilities.The
convertWeatherTypeandapplyOffsethelpers are duplicated between this file andtests/e2e/helpers/weather-functions.js. Extracting these to a shared module would reduce maintenance burden.tests/e2e/helpers/weather-functions.js (2)
117-124: ReplacewaitForTimeoutwith proper wait conditions.Static analysis correctly flags
page.waitForTimeout()as an anti-pattern. These fixed delays make tests flaky and slower than necessary.♻️ Suggested improvement
// If mock data file is provided, inject it if (mockDataFile) { const page = helpers.getPage(); - // Wait for modules to initialize - await page.waitForTimeout(1000); + // Wait for weather module to be present + await page.waitForSelector(".module.weather", { state: "attached" }); await injectMockWeatherData(page, mockDataFile); - // Wait for rendering - await page.waitForTimeout(500); + // Wait for weather content to render + await page.waitForSelector(".weather .normal.medium", { state: "visible" }); }
92-110: Consider adding error handling when weather module is not found.If
weatherModuleisnull, the injection silently does nothing. This could mask test failures. Consider throwing an error or returning a status.♻️ Suggested improvement
// Inject weather data by evaluating code in the browser context - await page.evaluate(({ type, data }) => { + const injected = await page.evaluate(({ type, data }) => { // Find the weather module instance const weatherModule = MM.getModules().find((m) => m.name === "weather"); if (weatherModule) { // Send INITIALIZED first weatherModule.socketNotificationReceived("WEATHER_INITIALIZED", { instanceId: weatherModule.instanceId, locationName: "Munich" }); // Then send the actual data weatherModule.socketNotificationReceived("WEATHER_DATA", { instanceId: weatherModule.instanceId, type: type, data: data }); + return true; } + return false; }, { type, data }); + + if (!injected) { + throw new Error("Weather module not found - cannot inject mock data"); + }defaultmodules/weather/providers/envcanada.js (2)
29-29: Consider usingnullinstead of magic number999for temperature cache.Using
999as a sentinel value could be problematic if displayed to users when no valid temperature has been cached yet. Anullvalue would be more explicit and easier to handle in downstream code.Suggested fix
- this.cacheCurrentTemp = 999; + this.cacheCurrentTemp = null;Then update the usage in
#generateCurrentWeather:if (temp && temp !== "") { current.temperature = parseFloat(temp); this.cacheCurrentTemp = current.temperature; - } else { + } else if (this.cacheCurrentTemp !== null) { current.temperature = this.cacheCurrentTemp; }
340-351: Silent fallback to current date may mask data issues.When
timeStris invalid or too short, returningnew Date()silently could lead to incorrect weather data timestamps. Consider returningnulland handling it in the caller, or at least logging a warning.Suggested improvement
`#parseECTime` (timeStr) { - if (!timeStr || timeStr.length < 14) return new Date(); + if (!timeStr || timeStr.length < 14) { + Log.warn("[weatherprovider.envcanada] Invalid time string:", timeStr); + return null; + }defaultmodules/weather/providers/yr.js (1)
199-202: Consider awaiting stellar data refresh or documenting the async behavior.When using cached weather data,
#fetchStellarData()is called withoutawait. This fire-and-forget approach means the current response will use potentially stale stellar data. If immediate accuracy is needed, consider awaiting the refresh.- if (fromCache) { - this.#fetchStellarData(); - } + if (fromCache) { + await this.#fetchStellarData(); + }defaultmodules/weather/providers/weathergov.js (1)
291-291: Unconditionally skipping the first day may lose valid forecast data.The comment says "Skip first incomplete day," but depending on when the forecast is fetched, the first day might contain complete data. Consider checking if the first day is actually incomplete before skipping.
defaultmodules/weather/providers/smhi.js (1)
64-70: Potential locale issue withIntl.NumberFormatparsing.Using
Intl.NumberFormatwithen-USlocale ensures decimal points, butparseFloatmay still have issues if the formatted string contains grouping separators. For coordinate precision, consider usingtoFixed()instead:Alternative implementation
`#limitDecimals` (value, decimals) { - const formatter = new Intl.NumberFormat("en-US", { - minimumFractionDigits: decimals, - maximumFractionDigits: decimals - }); - return parseFloat(formatter.format(value)); + return parseFloat(parseFloat(value).toFixed(decimals)); }defaultmodules/weather/providers/ukmetofficedatahub.js (1)
53-105: Consider making internal methods private for encapsulation.Several methods that are implementation details are exposed as public:
initializeFetcher,getForecastType,getUrl,handleResponse,generateCurrentWeather,generateForecast,generateHourly, andconvertWeatherType. Other providers in this PR use private methods (prefixed with#) for these.Example refactor for key methods
- initializeFetcher () { + `#initializeFetcher` () { - getForecastType () { + `#getForecastType` () { - getUrl (forecastType) { + `#getUrl` (forecastType) { - handleResponse (data) { + `#handleResponse` (data) {Then update
initialize():- this.initializeFetcher(); + this.#initializeFetcher();defaultmodules/weather/providers/openmeteo.js (2)
234-244: RedundanthasOwnPropertycheck.Since
maxNumberOfDaysis always present inthis.configfrom the constructor defaults (line 99), thehasOwnPropertycheck is unnecessary.♻️ Simplified version
- if (this.config.hasOwnProperty("maxNumberOfDays") && !isNaN(parseFloat(this.config.maxNumberOfDays))) { + if (!isNaN(parseFloat(this.config.maxNumberOfDays))) { const daysFactor = ["daily", "forecast"].includes(this.config.type) ? 1 : this.config.type === "hourly" ? 24 : 0;
370-370: Consider usinginoperator for object key check.
Object.keys(weatherConditions).includes(\${weathercode}`)creates an array on every call. Using thein` operator is more efficient.♻️ Proposed fix
- if (!Object.keys(weatherConditions).includes(`${weathercode}`)) return null; + if (!(weathercode in weatherConditions)) return null;
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/functions/server_functions_spec.js (1)
3-19:⚠️ Potential issue | 🟡 MinorRestore
global.configto avoid test leakage.
Mutating global state without cleanup can spill into other tests.🛠️ Suggested fix
describe("server_functions tests", () => { describe("getUserAgent", () => { it("Gets User-Agent from configuration", async () => { - global.config = {}; + const previousConfig = global.config; + global.config = {}; let userAgent; userAgent = getUserAgent(); expect(userAgent).toContain("Mozilla/5.0 (Node.js "); @@ global.config.userAgent = () => "Mozilla/5.0 (Bar)"; userAgent = getUserAgent(); expect(userAgent).toBe("Mozilla/5.0 (Bar)"); + + global.config = previousConfig; }); }); });
🤖 Fix all issues with AI agents
In `@defaultmodules/weather/providers/envcanada.js`:
- Around line 296-301: The code is constructing weather.date by calling
this.#parseECTime(dateTimeUTC) which yields a local-time Date and then adds
utcOffset, causing a double shift; change parsing so dateTimeUTC is interpreted
as UTC before applying utcOffset (e.g., adjust this.#parseECTime to return a
Date in UTC or normalize dateTimeUTC to include a 'Z'/use Date.UTC/Date.parse
with UTC components), then compute weather.date = new Date(utcTime.getTime() +
utcOffset * 60 * 60 * 1000) so the offset is applied only once; update
references in the loop over hourlyMatches (dateTimeUTC, this.#parseECTime,
utcOffset, weather.date) accordingly.
In `@defaultmodules/weather/providers/openmeteo.js`:
- Around line 425-441: parsedData.hourly can be either an object with arrays or
an array of hourly objects (after transpose), so the current block skips because
parsedData.hourly.time is undefined and the time comparison may be by reference;
update the enrichment in the openmeteo provider to handle both shapes: if
parsedData.hourly.time exists use it as before, otherwise if
Array.isArray(parsedData.hourly) find the index with
parsedData.hourly.findIndex(h => String(h.time) ===
String(parsedData.current_weather.time)); use that index (or 0 if not found,
logging the warning) and then read values either from
parsedData.hourly.<field>[index] (object-with-arrays case) or
parsedData.hourly[index]?.<field> (array-of-objects case) for fields like
relativehumidity_2m, apparent_temperature, rain, snowfall, precipitation,
precipitation_probability, uv_index and set
current.humidity/current.feelsLikeTemp/etc. accordingly.
- Around line 284-286: The current Object.keys(params).filter((key) =>
!!params[key])... removes valid numeric zeros (latitude/longitude); update the
filter to preserve 0 by checking for null/undefined/empty-string instead, e.g.
use .filter((key) => params[key] !== undefined && params[key] !== null &&
params[key] !== '') so keys with value 0 remain included when building the query
(locate the Object.keys(params).filter(...).map(...) block in openmeteo.js).
In `@defaultmodules/weather/providers/smhi.js`:
- Around line 54-62: The validateConfig() method currently treats 0 as falsy and
rejects valid coordinates; update the check to only reject missing coordinates
by testing for null/undefined (e.g., use this.config.lat == null ||
this.config.lon == null or typeof checks) instead of a falsy check, then
continue to apply limitDecimals(this.config.lat, 6) and
limitDecimals(this.config.lon, 6) as before so zero coordinates are accepted but
absent values still throw the Error("Latitude and longitude are required").
- Around line 278-296: The gap-filling currently clones data[i] and generates
hourly points starting at the previous timestamp, which shifts values and never
re-adds the real entries; update `#fillInGaps`(data) to start by pushing the first
original point (data[0]), then for each interval between data[i-1] and data[i]
clone data[i-1] (not data[i]) to generate hourly entries from the previous
timestamp up to but not including the next timestamp (set validTime via new
Date(from).setHours(from.getHours()+j) and toISOString()), and finally push the
actual data[i] after filling the hourly gaps so original points are preserved.
Ensure types and ISO formatting remain consistent.
In `@defaultmodules/weather/providers/ukmetofficedatahub.js`:
- Around line 149-254: The code uses the key precipitation but the templates
expect precipitationAmount; update all objects to use precipitationAmount
instead of precipitation: change the current object built in the current-weather
logic (the first current with precipitation: (hour.totalPrecipAmount || 0) +
(hour.totalSnowAmount || 0)), the fallback current (firstHour case), each day
object returned in `#generateForecast` (set precipitationAmount appropriately,
e.g. combine rain/snow or use provided probability/amount fields), and each hour
object in `#generateHourly` (replace precipitation with precipitationAmount and
assign (hour.totalPrecipAmount || 0) + (hour.totalSnowAmount || 0)). Keep the
original fallback/default numeric logic (|| 0) and do not change other field
names or calculations.
In `@defaultmodules/weather/providers/weathergov.js`:
- Around line 66-120: The two native fetch calls inside the private method
`#fetchWeatherGovURLs` lack timeouts; add an AbortController with a configurable
timeout (e.g., this.config.fetchTimeout) for each fetch, pass controller.signal
to fetch(this.observationStationsURL, { ..., signal }) and the pointsUrl fetch,
set a timer to call controller.abort() after the timeout, clear the timer on
success, and catch/translate AbortError into a clear error (e.g., throw new
Error("Weather.gov request timed out while fetching grid point" or "observation
stations")). Ensure the AbortController is created per request (or reused
carefully) and that the signal is included in both fetch calls so initialization
cannot hang indefinitely.
- Around line 342-345: The code incorrectly converts observation wind speeds
because Weather.gov observations are already in m/s but the parser calls the
private helper `#convertWindToMs`; remove the call to `#convertWindToMs` when
processing observation data (only use it for forecast data returned in km/h when
?units=si), or alternatively make `#convertWindToMs` unit-aware by accepting a
units flag (e.g., 'm/s' vs 'km/h') and only dividing by 3.6 for km/h; update the
observation parsing path to pass the correct units or skip conversion so
observed values remain in m/s.
In `@defaultmodules/weather/providers/yr.js`:
- Around line 194-207: The switch over this.config.type can fall through and
leave weatherData undefined for unknown values; add a default branch to the
switch that handles unexpected types (e.g. in the switch in yr.js add a default:
case that throws or returns an Error/Promise rejection with a clear message like
"Unknown weather type: <value>" or calls the existing callback with an Error) so
that `#generateCurrentWeather`, `#generateForecast` and `#generateHourly` are only
used for known types and callers get an explicit error when this.config.type is
invalid.
- Around line 72-79: The validation in the private method `#validateConfig`
rejects valid 0 coordinates because it uses a falsy check; change the check to
explicitly validate presence and numericness (e.g., ensure lat/lon are not
null/undefined and are finite numbers) before calling
limitDecimals(this.config.lat, 4) and limitDecimals(this.config.lon, 4), and
throw the same error message if validation fails; reference the `#validateConfig`
method and the use of limitDecimals to locate where to update the condition.
In `@tests/e2e/helpers/weather-functions.js`:
- Around line 15-59: The helper currently leaves data null when rawData lacks
current, daily, or hourly, causing silent invalid payloads; add a guard in the
weather mock (use rawData, type, data) that immediately throws a clear Error (or
asserts) when none of rawData.current, rawData.daily, or rawData.hourly are
present—include the offending rawData keys or a brief description in the error
message so tests fail fast and show which fixture format is unsupported.
In `@tests/unit/modules/default/weather/weather_providers_spec.js`:
- Around line 1-8: Replace the ESM import line with a CommonJS require call so
the Vitest symbols are loaded via const destructuring; specifically change the
top-level `import { describe, it, expect, vi, beforeEach, beforeAll, afterAll }
from "vitest";` to use `require()` and keep the same symbol names (`describe`,
`it`, `expect`, `vi`, `beforeEach`, `beforeAll`, `afterAll`) so the rest of the
test file continues to work unchanged.
🧹 Nitpick comments (4)
defaultmodules/weather/provider-utils.js (2)
70-77: Consider handling polar edge cases.The
suncalclibrary returnsInvalid Datefor polar regions during perpetual day/night. Callers likeisDayTimehandle null values but notInvalid Date. Consider adding validation:♻️ Optional: Handle invalid dates from polar regions
function getSunTimes (date, lat, lon) { const SunCalc = require("suncalc"); const sunTimes = SunCalc.getTimes(date, lat, lon); + const isValidDate = (d) => d instanceof Date && !isNaN(d.getTime()); return { - sunrise: sunTimes.sunrise, - sunset: sunTimes.sunset + sunrise: isValidDate(sunTimes.sunrise) ? sunTimes.sunrise : null, + sunset: isValidDate(sunTimes.sunset) ? sunTimes.sunset : null }; }
110-112: Clarify UTC behavior in documentation.
toISOString()converts to UTC before formatting, which may return a different date than the local date near midnight. If this is intentional, consider updating the JSDoc to note the UTC behavior.📝 Optional: Clarify UTC behavior or use local date
If UTC is intentional:
/** - * Get date string in YYYY-MM-DD format + * Get date string in YYYY-MM-DD format (UTC) * `@param` {Date} date - The date to format * `@returns` {string} Date string in YYYY-MM-DD format */If local date is preferred:
function getDateString (date) { - return date.toISOString().split("T")[0]; + const year = date.getFullYear(); + const month = String(date.getMonth() + 1).padStart(2, "0"); + const day = String(date.getDate()).padStart(2, "0"); + return `${year}-${month}-${day}`; }tests/electron/helpers/weather-setup.js (1)
88-99: Hardcoded timeouts may cause test flakiness.Using
waitForTimeoutwith fixed delays (1000ms, 500ms) can lead to flaky tests on slower CI environments. Consider usingwaitForSelectororwaitForFunctionto wait for specific conditions instead.♻️ Suggested improvement: Use condition-based waiting
exports.startApp = async (configFileName, systemDate, mockDataFile = "weather_onecall_current.json") => { await helpers.startApplication(configFileName, systemDate); // Wait for modules to initialize - await global.page.waitForTimeout(1000); + await global.page.waitForFunction(() => { + return typeof MM !== 'undefined' && MM.getModules().some((m) => m.name === "weather"); + }, { timeout: 5000 }); // Inject mock weather data await injectMockWeatherData(mockDataFile); // Wait for rendering - await global.page.waitForTimeout(500); + await global.page.waitForSelector('.weather', { timeout: 5000 }); };defaultmodules/weather/providers/weatherbit.js (1)
146-159: Edge case: Sunrise/sunset date assumption.The sunrise/sunset parsing uses
new Date()(today) as the base, which could be incorrect near midnight if the API returns yesterday's sunset time. This is a minor edge case but worth noting.📝 Optional: Consider using data timestamp for date base
// Parse sunrise/sunset from HH:mm format (already in local time) if (current.sunrise) { const [hours, minutes] = current.sunrise.split(":"); - const sunrise = new Date(); + const sunrise = new Date(current.ts * 1000); sunrise.setHours(parseInt(hours), parseInt(minutes), 0, 0); weather.sunrise = sunrise; } if (current.sunset) { const [hours, minutes] = current.sunset.split(":"); - const sunset = new Date(); + const sunset = new Date(current.ts * 1000); sunset.setHours(parseInt(hours), parseInt(minutes), 0, 0); weather.sunset = sunset; }
KristjanESPERANTO
force-pushed
the
weather
branch
from
776d7db to
57a0b3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@defaultmodules/weather/providers/envcanada.js`:
- Around line 139-151: The switch in `#parseWeatherData` currently falls through
to returning null for unknown this.config.type and that null propagates into
onDataCallback silently; update the default branch to either throw a descriptive
Error or emit a warning (e.g., this.logger.warn or console.warn) that includes
the invalid type, and ensure callers (onDataCallback) never receive null — for
example, have the default return a well-formed error object or throw so upstream
code can handle it; reference `#parseWeatherData`, `#generateCurrentWeather`,
`#generateForecast`, `#generateHourly` and onDataCallback when making the change.
In `@defaultmodules/weather/providers/pirateweather.js`:
- Around line 80-91: The switch on this.config.type in the weather provider
lacks a default branch to handle unknown types; add a default case after the
existing cases that handles unexpected values (e.g., call a fallback, set
weatherData to null/empty, and/or log an error) and include the config type in
the log message; update the switch around generateCurrentWeather,
generateForecast, and generateHourly to ensure unknown this.config.type values
are safely handled and surfaced.
- Around line 17-30: In initialize(), ensure the missing API-key path propagates
errors even when setCallbacks() hasn't been called: when !this.config.apiKey
prepare the error object and if this.onErrorCallback is defined call it,
otherwise throw (or return a rejected Promise) with that error so callers see
the failure; update the same block that currently calls Log.error and
this.onErrorCallback to always either call the callback or throw/reject, keeping
initializeFetcher() untouched.
In `@defaultmodules/weather/providers/smhi.js`:
- Around line 94-105: The switch on this.config.type in the method that sets
weatherData (using `#generateCurrentWeather`, `#generateForecast`, `#generateHourly`)
lacks a default branch, so unknown types leave weatherData undefined and still
get passed to onDataCallback; add a default case that handles unexpected values
(e.g., log an error or call onDataCallback with an error/empty payload) and/or
throw a clear error indicating the invalid config.type so you never call
onDataCallback with undefined weatherData.
In `@defaultmodules/weather/providers/ukmetofficedatahub.js`:
- Around line 38-51: The initialize() method currently skips validating that
required callbacks are set before starting; add a guard that checks
this.onErrorCallback (and any other required callbacks e.g. data/update
callbacks used later) are functions, log an error via Log.error and invoke
this.onErrorCallback with the same error payload used for missing API key, then
return early; only call this.#initializeFetcher() after the callback validation
passes. Reference initialize(), this.onErrorCallback, and `#initializeFetcher`()
when making the change.
- Around line 121-132: Add a default branch to the switch on this.config.type to
handle unknown types so we never pass null into onDataCallback; inside the
default case set weatherData to an empty object (e.g. {}) and log the unexpected
type using the module's logger (e.g. this.logger.error or console.error) with
the offending this.config.type and a short message, then break—this ensures
`#generateCurrentWeather`, `#generateForecast`, `#generateHourly` are unchanged and
onDataCallback always receives a valid object.
In `@defaultmodules/weather/providers/weatherbit.js`:
- Around line 110-121: The switch on this.config.type can leave weatherData null
for unknown types; add a default case in the switch (in the same block that
calls generateCurrentWeather, generateForecast, generateHourly) that logs or
throws a clear error including the invalid this.config.type and the provider
name, or sets weatherData to a safe fallback, e.g. call a validation/error
handler; ensure the default references this.config.type and affects the same
weatherData variable so unknown types are surfaced instead of silently passing
through.
- Around line 30-43: The initialize() method in WeatherbitProvider uses
this.onErrorCallback but doesn't validate callbacks were set by setCallbacks(),
so errors may not propagate; add a guard at the start of initialize() that
verifies required callbacks (at least this.onErrorCallback and any success
callback your provider expects) are present, and if not log an error and return
(or invoke a fallback error path) before proceeding to initializeFetcher();
mirror the validation approach used in OpenWeatherMapProvider (check callbacks
set in initialize(), reference initialize(), setCallbacks(), and
onErrorCallback).
In `@defaultmodules/weather/providers/weatherflow.js`:
- Around line 121-124: In generateCurrentWeather, add defensive null/shape
checks before reading nested API properties: verify data is defined and
data.current_conditions exists before using it (the current variable), and
verify data.forecast && Array.isArray(data.forecast.daily) &&
data.forecast.daily.length > 0 before accessing data.forecast.daily[0] (the
daily variable); on missing values return a safe fallback (null or an object
with default values) or throw a clear error so callers of generateCurrentWeather
handle incomplete API responses.
- Around line 187-212: The generateHourly method assumes data.forecast.hourly
exists; add a defensive check at the start of generateHourly to return an empty
array if data, data.forecast, or data.forecast.hourly is missing or not an array
before iterating; keep the rest of the logic unchanged and reference
generateHourly and data.forecast.hourly so the early-return guard prevents
runtime errors when hourly data is absent.
- Around line 146-180: The generateForecast function lacks null checks and uses
an unsafe date comparison; first guard that data and data.forecast and both
data.forecast.daily and data.forecast.hourly are arrays and return [] if not;
then replace the per-hour matching logic in generateForecast so you compute a
precise time window for each forecast day (const startMs =
forecast.day_start_local * 1000; const endMs = startMs + 24*60*60*1000) and
include hours where const hourMs = hour.time * 1000 and hourMs >= startMs &&
hourMs < endMs (use this to update uvIndex and precipitationAmount), and remove
the current getDate() check and the faulty break based on hourDate >
forecastDate. This uses the unique symbol generateForecast and the
forecast.daily/hourly properties to locate and fix the code.
In `@defaultmodules/weather/providers/weathergov.js`:
- Around line 34-47: The initialize() method of WeathergovProvider doesn't
validate that callbacks were provided before starting async setup; before
calling this.#fetchWeatherGovURLs() and this.#initializeFetcher() add a check
(similar to OpenWeatherMapProvider) to ensure this.onErrorCallback (and any
other required callbacks set by setCallbacks()) are present and if not, invoke
Log.error and throw/propagate an Error (or call the onErrorCallback if present)
so initialization stops and errors are surfaced consistently; modify
initialize() to validate callbacks up-front, reference the existing
setCallbacks(), onErrorCallback, `#fetchWeatherGovURLs`, and `#initializeFetcher`
symbols when implementing the guard.
- Around line 176-196: The switch over this.config.type in the data handling
block (cases "current", "forecast"/"daily", "hourly") lacks a default branch, so
unknown types can result in undefined passed to onDataCallback; add a default
case that either throws a descriptive Error (e.g., `Unknown weather provider
type: ${this.config.type}`) or calls onDataCallback with a clear error,
referencing the switch that populates weatherData and the methods
`#generateWeatherObjectFromCurrentWeather`, `#generateWeatherObjectsFromForecast`,
and `#generateWeatherObjectsFromHourly` so you ensure unknown config.type values
are caught and handled rather than falling through.
🧹 Nitpick comments (17)
defaultmodules/weather/weatherobject.js (1)
64-77: Update JSDoc to reflect nullable return type.The JSDoc on line 69 states
@returns {string} "sunset" or "sunrise", but the function now returnsnullwhen sunrise/sunset data is unavailable (lines 72-75). Callers need to be aware of this change.📝 Proposed documentation update
/** * Determines if the sun sets or rises next. Uses the current time and not * the date from the weather-forecast. * `@param` {Moment} date an optional date where you want to get the next * action for. Useful only in tests, defaults to the current time. - * `@returns` {string} "sunset" or "sunrise" + * `@returns` {string|null} "sunset", "sunrise", or null if sun data unavailable */tests/configs/modules/weather/hourlyweather_default.js (1)
15-15: Minor formatting inconsistency.The closing brace is on the same line as
apiKey, unlike other test config files which place it on a separate line. This is a minor consistency issue.🔧 Suggested formatting fix
- apiKey: "test-api-key" } + apiKey: "test-api-key" + }tests/unit/modules/default/weather/weather_providers_spec.js (1)
10-20: Consider scoping the fetch mock to avoid test pollution.The global fetch mock is set at module load time and restored in
afterAll. If other test files run in the same process before this file'safterAll, they may unexpectedly use this mock. Consider usingbeforeAll/afterAllwithin the describe block orvi.spyOnfor tighter scoping.♻️ Proposed refactor using beforeAll/afterAll
-// Mock global fetch for location lookup -const originalFetch = global.fetch; - -global.fetch = vi.fn(() => Promise.resolve({ - ok: true, - json: () => Promise.resolve({ city: "Munich", locality: "Munich" }) -})); - -// Restore original fetch after all tests -afterAll(() => { - global.fetch = originalFetch; -}); - describe("Weather Provider Smoke Tests", () => { + let originalFetch; + + beforeAll(() => { + originalFetch = global.fetch; + global.fetch = vi.fn(() => Promise.resolve({ + ok: true, + json: () => Promise.resolve({ city: "Munich", locality: "Munich" }) + })); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + describe("OpenMeteoProvider", () => {tests/electron/helpers/weather-setup.js (2)
11-73: Consider extracting shared mock data transformation logic.The
injectMockWeatherDatafunction duplicates the transformation logic found intest/e2e/helpers/weather-functions.js(lines 10-56). Both files transform raw mock data identically for current, forecast, and hourly types.Consider extracting the shared transformation logic into a common utility to reduce maintenance burden and ensure consistency.
88-99: Hardcoded timeouts may cause flaky tests.Using fixed
waitForTimeout(1000)andwaitForTimeout(500)can lead to flaky tests depending on system load. Consider using condition-based waiting instead.♻️ Suggested improvement
exports.startApp = async (configFileName, systemDate, mockDataFile = "weather_onecall_current.json") => { await helpers.startApplication(configFileName, systemDate); - // Wait for modules to initialize - await global.page.waitForTimeout(1000); + // Wait for weather module to be ready + await global.page.waitForFunction(() => { + const weatherModule = MM?.getModules()?.find((m) => m.name === "weather"); + return weatherModule && weatherModule.instanceId; + }, { timeout: 5000 }); // Inject mock weather data await injectMockWeatherData(mockDataFile); - // Wait for rendering - await global.page.waitForTimeout(500); + // Wait for weather content to render + await global.page.waitForSelector(".weather .normal.medium", { timeout: 5000 }); };defaultmodules/weather/providers/pirateweather.js (1)
4-10: Constructor doesn't merge default configuration values.Unlike other providers (e.g.,
WeatherbitProvider,OpenWeatherMapProvider) that merge defaults with the provided config, this constructor assigns config directly. This means missing config properties won't have fallback values.♻️ Suggested fix for consistency
class PirateweatherProvider { constructor (config) { - this.config = config; + this.config = { + apiBase: "https://api.pirateweather.net", + weatherEndpoint: "/forecast", + apiKey: "", + lat: 0, + lon: 0, + type: "current", + lang: "en", + updateInterval: 10 * 60 * 1000, + ...config + }; this.fetcher = null;defaultmodules/weather/provider-utils.js (3)
70-77: Consider hoistingrequire("suncalc")to module level.Requiring
suncalcinside the function on every call adds overhead. Since this is a server-side module, hoisting the require to module level improves performance without any downsides.♻️ Suggested improvement
+const SunCalc = require("suncalc"); + /** * Shared utility functions for weather providers */ // ... other functions ... function getSunTimes (date, lat, lon) { - const SunCalc = require("suncalc"); const sunTimes = SunCalc.getTimes(date, lat, lon);
132-140:validateCoordinatesmutates the input config object.This function has a hidden side effect - it modifies
config.latandconfig.lonin place. This could surprise callers who don't expect their config to be modified. Consider either documenting this clearly or returning new values instead.♻️ Alternative: return validated values without mutation
-function validateCoordinates (config, maxDecimals = 4) { +function validateCoordinates (config, maxDecimals = 4) { if (config.lat == null || config.lon == null || !Number.isFinite(config.lat) || !Number.isFinite(config.lon)) { throw new Error("Latitude and longitude are required"); } - config.lat = limitDecimals(config.lat, maxDecimals); - config.lon = limitDecimals(config.lon, maxDecimals); + return { + lat: limitDecimals(config.lat, maxDecimals), + lon: limitDecimals(config.lon, maxDecimals) + }; }
52-61:limitDecimalsmay not handle scientific notation correctly.For very small or large numbers that JavaScript represents in scientific notation (e.g.,
1e-10), the string-based approach won't find a decimal point where expected.🛡️ More robust alternative
function limitDecimals (value, decimals) { + // Handle edge cases + if (!Number.isFinite(value)) return value; + const str = value.toString(); + // Handle scientific notation + if (str.includes("e") || str.includes("E")) { + const factor = Math.pow(10, decimals); + return Math.trunc(value * factor) / factor; + } if (str.includes(".")) {defaultmodules/weather/providers/yr.js (1)
83-104: Stellar data fetch lacks timeout protection.The
fetchcall for stellar data has no timeout, which could causeinitialize()to hang indefinitely if the Yr.no sunrise API is unresponsive. Consider adding an AbortController with a reasonable timeout.🛡️ Proposed fix
async `#fetchStellarData` () { const today = getDateString(new Date()); // Check if we already have today's data if (this.stellarDataDate === today && this.stellarData) { return; } const url = this.#getSunriseUrl(); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); try { const response = await fetch(url, { headers: { "User-Agent": "MagicMirror", Accept: "application/json" - } + }, + signal: controller.signal }); + clearTimeout(timeoutId);defaultmodules/weather/weather.js (3)
98-107: Instance ID generation uses Date.now() which may collide in rapid initialization.If multiple weather modules are initialized in the same millisecond (unlikely but possible during rapid page loads),
Date.now()could produce duplicate IDs. Consider adding a counter or random component.🔧 Proposed fix
- this.instanceId = `${this.identifier}_${Date.now()}`; + this.instanceId = `${this.identifier}_${Date.now()}_${Math.random().toString(36).substring(2, 9)}`;
144-151: CURRENT_WEATHER_OVERRIDE silently no-ops if currentWeatherObject is null.If an override notification arrives before the first weather data is received, it will be silently ignored. Consider logging a warning or queuing the override.
171-193: Missing default case in handleWeatherData switch.If
typeis not one of the expected values, the function silently returns without error. Consider adding a warning log for unexpected types.🛡️ Proposed fix
case "hourly": this.weatherHourlyArray = data.map((d) => this.createWeatherObject(d)); break; + default: + Log.warn(`[weather] Unknown weather data type: ${type}`); + return; }defaultmodules/weather/providers/smhi.js (1)
296-302: Coordinate resolution assumes specific array structure.The code assumes
data.geometry.coordinates[0]is an array with[lon, lat]format. If the API response structure changes or is malformed, this could throw. Consider adding defensive checks.🛡️ Proposed fix
`#resolveCoordinates` (data) { // SMHI returns coordinates in [lon, lat] format + if (!data?.geometry?.coordinates?.[0]?.[0] || !data?.geometry?.coordinates?.[0]?.[1]) { + Log.warn("[weatherprovider.smhi] Invalid coordinate structure, using config values"); + return { lat: this.config.lat, lon: this.config.lon }; + } return { lat: data.geometry.coordinates[0][1], lon: data.geometry.coordinates[0][0] }; }defaultmodules/weather/providers/openmeteo.js (2)
145-160: Geocoding fetch lacks timeout and error handling for non-JSON responses.The
#fetchLocationmethod doesn't handle network timeouts or non-JSON error responses from BigDataCloud. A malformed response would causeresponse.json()to throw.🛡️ Proposed fix
async `#fetchLocation` () { const url = `${GEOCODE_BASE}?latitude=${this.config.lat}&longitude=${this.config.lon}&localityLanguage=${this.config.lang || "en"}`; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); try { - const response = await fetch(url); + const response = await fetch(url, { signal: controller.signal }); + clearTimeout(timeoutId); if (!response.ok) { throw new Error(`HTTP ${response.status}`); }
518-519: Hourly weather filtering logic may skip valid entries.The condition
hours.length === 0 && weather.time <= nowwill skip all entries until the first future entry is found, which is correct. However, using<=means an entry exactly at the current time is also skipped. Consider using<if you want to include the current hour.defaultmodules/weather/providers/envcanada.js (1)
319-327: Multiple regex patterns used for XML parsing are fragile to schema changes.The provider extensively uses regex to extract data from Environment Canada XML responses (forecast blocks, temperature, precipitation, icon codes, hourly forecasts, etc.). While functional, this approach is prone to breaking with whitespace changes, attribute reordering, or XML structure modifications. Consider adding an XML parser library (e.g.,
fast-xml-parser) to handle these edge cases more robustly if the API schema proves unstable.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@defaultmodules/weather/providers/openmeteo.js`:
- Around line 474-495: The code assumes parsedData.daily is an object of arrays
but after `#transposeDataMatrix` parsedData.daily is an array of daily objects;
update the accesses to read the first day's properties (e.g.
parsedData.daily[0].sunrise, parsedData.daily[0].sunset,
parsedData.daily[0].temperature_2m_min, parsedData.daily[0].temperature_2m_max)
when populating
current.sunrise/current.sunset/current.minTemperature/current.maxTemperature,
and then call this.#convertWeatherType(parsedData.current_weather.weathercode,
this.#isDayTime(parsedData.current_weather.time, current.sunrise,
current.sunset)) once sunrise and sunset are set.
In `@defaultmodules/weather/providers/openweathermap.js`:
- Around line 182-183: The code currently uses a truthy check that turns 0 into
undefined (e.g., weather.precipitationProbability = hour.pop ? hour.pop * 100 :
undefined); change this to a nullish or explicit undefined check so 0% is
preserved — e.g. set weather.precipitationProbability = (hour.pop ?? undefined)
!== undefined ? hour.pop * 100 : undefined or equivalently
weather.precipitationProbability = hour.pop !== undefined ? hour.pop * 100 :
undefined; make the same change for the other occurrence that assigns
precipitationProbability from day.pop (replace the truthy check with a
nullish/explicit undefined check).
In `@defaultmodules/weather/providers/pirateweather.js`:
- Around line 227-231: getUrl currently hardcodes "units=si" and can emit
"lang=undefined"; update it to use the module config defaults instead: read
units via this.config.units (fallback to "us") and lang via this.config.lang
(fallback to "en") when building the query string, and also add default entries
for units and lang in the module's defaults object (the defaults defined near
the top of the file) or ensure the getUrl fallbacks are used; specifically
modify getUrl to compute const units = this.config.units || "us" and const lang
= this.config.lang || "en" and replace the hardcoded units=si and lang usages
with those variables so URLs reflect the config.
In `@defaultmodules/weather/providers/smhi.js`:
- Around line 104-118: The switch's default branch logs an error for unknown
this.config.type but then allows onDataCallback(weatherData) to be called with
weatherData === undefined; change the default branch in the switch so it does
not fall through: either return early after Log.error(`[smhi] Unknown weather
type: ${this.config.type}`) or set weatherData to a valid error/result object
and call onDataCallback with an explicit error payload; ensure onDataCallback is
only invoked when weatherData is defined (refer to weatherData, onDataCallback,
this.config.type and Log.error).
In `@defaultmodules/weather/providers/weatherbit.js`:
- Around line 167-197: The precipitation field names in generateForecast and
generateHourly are incorrect: change the forecast/hourly output objects to use
precipitationAmount instead of precipitation so they match the WeatherObject
schema and templates; update both generateForecast (the object pushed in the
loop for daily entries) and generateHourly (the object pushed in the loop for
hourly entries) to replace the precipitation property with precipitationAmount
while keeping the same parsed value (parseFloat(forecast.precip) or default 0).
In `@defaultmodules/weather/providers/yr.js`:
- Around line 73-104: The `#fetchStellarData` method currently never parses or
saves the fetched JSON, so this.stellarData remains unset; fix it by, after
confirming response.ok, calling await response.json(), assigning the parsed
object to this.stellarData and setting this.stellarDataDate = today (and
optionally return the data); ensure this happens inside the try block (before
clearTimeout or immediately after) and preserve the existing error/timeout
handling and Log.warn behavior in the non-ok and catch branches so
`#getStellarInfoForDate` can read the stored stellar data.
In `@tests/electron/helpers/weather-setup.js`:
- Around line 19-56: The helper in tests/electron/helpers/weather-setup.js
currently leaves data null when rawData lacks current/daily/hourly; add an
explicit validation guard after reading rawData (e.g., check rawData.current ||
rawData.daily || rawData.hourly) and if none are present throw a clear Error (or
return a rejected/invalid marker) so invalid fixtures fail fast; update the
logic around the variables type and data to only proceed when that validation
passes (referencing rawData, type, data and the mapping branches for
current/daily/hourly).
🧹 Nitpick comments (5)
tests/electron/helpers/weather-setup.js (1)
91-98: Fixed timeouts may cause flaky tests.The hardcoded
waitForTimeout(1000)andwaitForTimeout(500)are less reliable than selector-based waits. Consider usingwaitForSelectorsimilar to the E2E helper, which waits for.weatherand.weather .weathericonelements with explicit timeouts.♻️ Suggested improvement
exports.startApp = async (configFileName, systemDate, mockDataFile = "weather_onecall_current.json") => { await helpers.startApplication(configFileName, systemDate); - // Wait for modules to initialize - await global.page.waitForTimeout(1000); + // Wait for weather module to initialize + await global.page.waitForSelector(".weather", { timeout: 5000 }); // Inject mock weather data await injectMockWeatherData(mockDataFile); - // Wait for rendering - await global.page.waitForTimeout(500); + // Wait for data to be rendered + await global.page.waitForSelector(".weather .weathericon", { timeout: 2000 }); };tests/configs/modules/weather/hourlyweather_default.js (1)
15-15: Minor formatting inconsistency.The closing brace is on the same line as
apiKey, unlike other weather config files where each property has its own line. Consider moving it to a separate line for consistency.🔧 Suggested fix
- apiKey: "test-api-key" } + apiKey: "test-api-key" + }defaultmodules/weather/providers/pirateweather.js (1)
101-104: Default case logs error but doesn't notify via callback.The default case now exists (addressing the past review comment), but unlike other error paths in this file, it doesn't invoke
onErrorCallbackto notify the caller.🛡️ Proposed fix to invoke error callback
default: Log.error(`[pirateweather] Unknown weather type: ${this.config.type}`); + if (this.onErrorCallback) { + this.onErrorCallback({ + message: `Unknown weather type: ${this.config.type}`, + translationKey: "MODULE_ERROR_UNSPECIFIED" + }); + } break;defaultmodules/weather/weather.js (1)
166-168: Consider displaying errors to the user.When
WEATHER_ERRORis received, the error is only logged. The user won't see any indication that weather data failed to load. Consider updating the DOM with an error state or usingthis.updateDom()to show a loading/error message.defaultmodules/weather/providers/openmeteo.js (1)
436-443: Time comparison usingString()is fragile.Both
hour.timeandcurrentTimeare Date objects. UsingString()comparison relies on identical string representations, which may vary. A more robust approach would compare timestamps directly.🔧 Proposed fix
if (Array.isArray(parsedData.hourly)) { // Array of objects (after transpose) - const hourlyIndex = parsedData.hourly.findIndex((hour) => String(hour.time) === String(currentTime)); + const hourlyIndex = parsedData.hourly.findIndex((hour) => hour.time.getTime() === currentTime.getTime()); h = hourlyIndex !== -1 ? hourlyIndex : 0;
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@defaultmodules/weather/providers/weatherflow.js`:
- Line 226: The hourly windSpeed assignment uses convertKmhToMs(hour.wind_avg)
without null-checking hour.wind_avg; update the expression in the hourly mapping
(where windSpeed is set) to guard against missing values by checking
hour.wind_avg for null/undefined and only calling convertKmhToMs when present,
otherwise return null/undefined (match the same null-check pattern used
elsewhere in this module).
- Line 150: The windSpeed assignment can produce NaN when current.wind_avg is
undefined; change the expression to only call convertKmhToMs when
current.wind_avg is present and otherwise return null (e.g., replace
convertKmhToMs(current.wind_avg) with a conditional like current.wind_avg !=
null ? convertKmhToMs(current.wind_avg) : null) so that convertKmhToMs is not
called with undefined and windSpeed follows the same fallback pattern as other
fields.
🧹 Nitpick comments (8)
tests/configs/modules/weather/hourlyweather_default.js (1)
10-16: Minor formatting inconsistency.The closing brace is on the same line as
apiKey. For consistency withforecastweather_default.js, consider placing it on a separate line.🔧 Suggested formatting fix
config: { lat: 48.14, lon: 11.58, type: "hourly", weatherProvider: "openweathermap", - apiKey: "test-api-key" } + apiKey: "test-api-key" + } }js/http_fetcher.js (1)
272-275: Consider wrapping URL parsing in try-catch for robustness.While the URL is likely valid if
fetch()was attempted, malformed URLs could causenew URL()to throw, potentially masking the original network error. Consider a defensive wrapper.🛡️ Suggested defensive improvement
// Truncate URL for cleaner logs - const urlObj = new URL(this.url); - const shortUrl = `${urlObj.origin}${urlObj.pathname}${urlObj.search.length > 50 ? "?..." : urlObj.search}`; + let shortUrl = this.url; + try { + const urlObj = new URL(this.url); + shortUrl = `${urlObj.origin}${urlObj.pathname}${urlObj.search.length > 50 ? "?..." : urlObj.search}`; + } catch { + // Use original URL if parsing fails + } Log.error(`${this.logContext}${shortUrl} - ${message}`);defaultmodules/weather/provider-utils.js (4)
35-35: Consider using direct property access instead ofhasOwnProperty.Using
weatherTypes[weatherType] ?? nullorObject.hasOwn(weatherTypes, weatherType)is more concise and equally safe for object literals.Suggested simplification
- return weatherTypes.hasOwnProperty(weatherType) ? weatherTypes[weatherType] : null; + return weatherTypes[weatherType] ?? null;
55-64: Edge case: scientific notation not handled.Values like
1.23e-10would pass theincludes(".")check but the string split would produce unexpected results. For coordinate truncation, this is unlikely but worth noting.More robust alternative using Math
function limitDecimals (value, decimals) { - const str = value.toString(); - if (str.includes(".")) { - const parts = str.split("."); - if (parts[1].length > decimals) { - return parseFloat(`${parts[0]}.${parts[1].substring(0, decimals)}`); - } - } - return value; + if (!Number.isFinite(value)) return value; + const factor = Math.pow(10, decimals); + return Math.trunc(value * factor) / factor; }
73-79: Potential issue with polar regions.SunCalc returns
Invalid Datefor locations where the sun doesn't rise or set (polar day/night). Callers likeisDayTimehandle null but notInvalid Date. Consider adding validation.Suggested defensive check
function getSunTimes (date, lat, lon) { const sunTimes = SunCalc.getTimes(date, lat, lon); + const sunrise = isNaN(sunTimes.sunrise?.getTime()) ? null : sunTimes.sunrise; + const sunset = isNaN(sunTimes.sunset?.getTime()) ? null : sunTimes.sunset; return { - sunrise: sunTimes.sunrise, - sunset: sunTimes.sunset + sunrise, + sunset }; }
134-142: Function mutates input config, which may be unexpected.The function name suggests validation only, but it also truncates
config.latandconfig.lonin-place. Consider either returning the modified values or renaming tovalidateAndTruncateCoordinatesto make the side effect explicit.defaultmodules/weather/providers/ukmetofficedatahub.js (1)
132-139: Default case only logs; consider calling error callback.The default case logs the unknown type but doesn't call
onErrorCallback. While the null check at line 137 prevents passing null toonDataCallback, the caller won't know why no data arrived.Suggested improvement
default: Log.error(`[ukmetofficedatahub] Unknown weather type: ${this.config.type}`); + if (this.onErrorCallback) { + this.onErrorCallback({ + message: `Unknown weather type: ${this.config.type}`, + translationKey: "MODULE_ERROR_UNSPECIFIED" + }); + } + return; - break; }defaultmodules/weather/providers/envcanada.js (1)
71-81: Consider potential infinite loop on persistent hour change.If
#initializeFetcheris called and the hour changes during fetch processing, this could cause repeated reinitialization. This is likely rare in practice, but adding a guard (e.g., a flag to prevent re-entry during reinitialization) could improve robustness.♻️ Optional: Add re-entry guard
`#initializeFetcher` () { + if (this.reinitializing) return; this.currentHour = new Date().toISOString().substring(11, 13); const indexURL = this.#getIndexUrl(); @@ -73,6 +74,7 @@ const newHour = new Date().toISOString().substring(11, 13); if (newHour !== this.currentHour) { Log.info("[envcanada] Hour changed, reinitializing fetcher"); + this.reinitializing = true; this.stop(); this.#initializeFetcher(); this.start(); + this.reinitializing = false; return; }
KristjanESPERANTO
force-pushed
the
weather
branch
from
b173ff6 to
59e4348
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@defaultmodules/weather/providers/openmeteo.js`:
- Line 529: The computed daily index h = Math.ceil((i + 1) / 24) - 1 can be out
of range for parsedData.daily; add a defensive bounds check before using
parsedData.daily[h] in the hourly loop (the block around the h calculation and
the accesses at lines ~534-541). Specifically, ensure parsedData.daily exists
and use a safe index (e.g., if h < 0 set h = 0, if h >= parsedData.daily.length
set h = parsedData.daily.length - 1) or skip setting day-specific fields when no
matching daily entry exists, then read sunrise/sunset/other fields from
parsedData.daily[safeH] instead of parsedData.daily[h]. Ensure any early-return
or default fallback prevents accessing properties on undefined.
- Around line 524-527: The hourly filtering in parsedData.hourly.forEach
currently uses (hours.length === 0 && weather.time <= now) which incorrectly
skips the first valid future entry; change the logic in that loop so you skip
any entry where weather.time <= now (i.e., only process entries with
weather.time > now) and push entries until hours.length reaches
this.config.maxEntries, ensuring the first future hour is included; update the
condition that references parsedData.hourly, hours, now, and
this.config.maxEntries accordingly.
In `@defaultmodules/weather/providers/pirateweather.js`:
- Around line 124-167: The code incorrectly treats numeric zeros as missing by
using truthy checks (e.g., data.currently.humidity ? ...,
data.currently.windSpeed ? ..., forecast.precipProbability ? ...), so update
generateCurrent (the current object construction) and generateForecast to check
for undefined/null instead of falsiness: use explicit checks like
data.currently.humidity !== undefined && data.currently.humidity !== null (or
Object.prototype.hasOwnProperty) before parsing/multiplying, and similarly for
windSpeed, apparentTemperature, forecast.precipProbability, temperatureMin/Max
etc., so 0 values are preserved as 0 rather than converted to null; keep
conversions (parseFloat, *100) the same when the value is present.
In `@js/http_fetcher.js`:
- Around line 272-275: The current logging creates a URL object from this.url
(new URL(this.url)) which can throw and hide the original error; wrap the URL
parsing in a try/catch inside the same block that builds shortUrl (used by
Log.error) and on failure fall back to a safe sanitized representation (e.g.,
the raw this.url or a trimmed "invalid-url" string), then call
Log.error(`${this.logContext}${shortUrl} - ${message}`); ensure the change is
applied around the existing shortUrl construction so references to this.url,
shortUrl and Log.error remain and no exceptions are thrown while logging.
In `@js/server_functions.js`:
- Line 94: Remove the dead getVersion function and its export: delete the
getVersion function definition from server_functions.js and update the
module.exports object (the exported names list that currently includes
getVersion) to no longer reference getVersion so only existing used functions
(getHtml, getEnvVars, getEnvVarsAsObj, getUserAgent, getConfigFilePath) are
exported. Ensure there are no remaining references to getVersion after this
change.
In `@tests/e2e/helpers/weather-functions.js`:
- Around line 54-65: The precipitationProbability calculation in the hourly
branch treats 0 as falsy and drops valid 0% values; update the ternary in the
hourly map (inside tests/e2e/helpers/weather-functions.js where rawData.hourly
is mapped) to use a nullish check (or explicit null/undefined check) against
hour.pop so that 0 is preserved (e.g., check hour.pop != null or hour.pop ??
...) and only produce undefined when pop is null/undefined.
In `@tests/electron/helpers/weather-setup.js`:
- Around line 51-54: The precipitationProbability check uses a truthy test that
drops 0 (hour.pop) causing 0% to become undefined; update the conditional in
weather-setup.js for precipitationProbability to use a nullish/explicit null
check (e.g., check hour.pop != null) so that hour.pop == 0 is preserved and you
multiply by 100 only when hour.pop is not null/undefined; locate the
precipitationProbability assignment in the diff and replace the truthy ternary
with a null-checking ternary or nullish-aware logic.
In `@tests/unit/modules/default/weather/providers/envcanada_spec.js`:
- Around line 143-146: The comment above the assertion for result.feelsLikeTemp
is incorrect: update the comment that currently reads "XML has windChill of -12"
to match the XML mock value of -31 so it correctly documents the expectation for
the test that checks result.feelsLikeTemp equals -31; locate the test/assertion
referencing result.feelsLikeTemp in the envcanada_spec.js block and change only
the comment text to "XML has windChill of -31".
- Around line 286-307: The callbacks are wired incorrectly in the test: when
calling provider.setCallbacks(...) the first callback should set a data flag and
the second should set an error flag, but the current test sets errorCalled
inside the data callback and resolves errorPromise inside the error callback,
causing inverted logic. Fix by passing two named inline callbacks to
setCallbacks where the first sets a dataCalled boolean (or noop) and the second
sets errorCalled = true and resolves errorPromise; then either await
errorPromise with a timeout to assert it is not resolved (use Promise.race with
a short timeout) or simply wait a short delay and assert errorCalled is false
after calling provider.initialize() and provider.start(); ensure you reference
EnvCanadaProvider, setCallbacks, initialize, and start when locating the test
code to update.
In `@tests/unit/modules/default/weather/providers/openmeteo_spec.js`:
- Around line 93-104: The test contains debug console.log statements inside the
Promise that wraps provider.setCallbacks (the two callbacks passed to
provider.setCallbacks and the variable dataPromise); remove the two console.log
calls ("[TEST] onDataCallback called" and "[TEST] onErrorCallback called:") so
the callbacks only call resolve(data) and reject(error) without printing to
stdout.
🧹 Nitpick comments (5)
tests/mocks/weather_ukmetoffice.json (1)
1020-1035: Last timeSeries entry is missing fields present in other entries.The final hourly entry (2026-02-09T12:00Z) is missing several fields that are present in all other entries:
maxScreenAirTemp,minScreenAirTemp,totalPrecipAmount, andtotalSnowAmount.If this is intentional to test the provider's handling of incomplete data from the API, consider adding a brief comment in a companion test file documenting this. Otherwise, add the missing fields for consistency.
js/server_functions.js (1)
5-5: Dead code:startUpvariable is no longer used.The
startUpvariable was previously used by the removedgetStartupfunction. It can be safely deleted.🧹 Proposed fix to remove dead code
const Log = require("logger"); -const startUp = new Date(); - /** * Gets the HTML to display the magic mirror.tests/unit/modules/default/weather/providers/openweathermap_spec.js (1)
88-101: Consider adding timeout protection for callback-based tests.The
dataPromisepattern is clean, but if the callback is never invoked (e.g., due to a bug in the provider), the test will hang indefinitely rather than failing with a clear message.💡 Optional: Add timeout wrapper
- const dataPromise = new Promise((resolve) => { - provider.setCallbacks(resolve, vi.fn()); - }); + const dataPromise = Promise.race([ + new Promise((resolve) => { + provider.setCallbacks(resolve, vi.fn()); + }), + new Promise((_, reject) => + setTimeout(() => reject(new Error("Test timeout")), 5000) + ) + ]);Alternatively, Vitest's built-in test timeout should handle this, but an explicit timeout provides clearer failure messages.
tests/unit/modules/default/weather/providers/smhi_spec.js (1)
13-13: Unused constantSMHI_URL.This constant is declared but never used in the tests. Consider removing it or using it in the HTTP mock setup for consistency.
🧹 Proposed fix
-const SMHI_URL = "https://opendata-download-metfcst.smhi.se/*";Or use it in the
http.get()calls instead of the inline string.defaultmodules/weather/providers/weathergov.js (1)
341-342: Both min and max temperature arrays receive the same value.Both
minTempandmaxTemparrays pushforecast.temperature, which is the same value. While the intent is to track temperature values across a day and then compute min/max, this works correctly. However, it would be clearer to use a single array:♻️ Optional simplification
let minTemp = []; - let maxTemp = []; + let temps = []; let date = ""; let weather = {}; for (const forecast of forecasts) { // ... if (date !== dateStr) { if (date !== "") { - weather.minTemperature = Math.min(...minTemp); - weather.maxTemperature = Math.max(...maxTemp); + weather.minTemperature = Math.min(...temps); + weather.maxTemperature = Math.max(...temps); days.push(weather); } weather = {}; - minTemp = []; - maxTemp = []; + temps = []; // ... } // ... - minTemp.push(forecast.temperature); - maxTemp.push(forecast.temperature); + temps.push(forecast.temperature); }
There was a problem hiding this comment.
Pull request overview
Migrates the Weather module to server-side provider execution coordinated by node_helper, leveraging centralized HTTPFetcher behavior and adding comprehensive provider unit tests with MSW fixtures.
Changes:
- Replaced client-side weather provider fetching (and CORS proxy dependency) with server-side providers wired via
node_helper. - Enhanced
HTTPFetcherlogging withlogContextand improved error messaging/URL truncation. - Added/updated extensive unit and E2E/Electron test fixtures and helper utilities for injecting weather data via sockets.
Reviewed changes
Copilot reviewed 68 out of 78 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/modules/default/weather/providers/smhi_spec.js | Adds SMHI provider unit coverage using MSW. |
| tests/unit/modules/default/weather/providers/pirateweather_spec.js | Adds Pirate Weather provider unit coverage using MSW. |
| tests/unit/modules/default/weather/providers/openweathermap_spec.js | Adds OpenWeatherMap provider unit coverage using MSW. |
| tests/unit/modules/default/weather/providers/openmeteo_spec.js | Adds OpenMeteo provider unit coverage including geocoding mocking. |
| tests/unit/modules/default/weather/providers/envcanada_spec.js | Adds EnvCanada provider unit coverage for the two-step fetch and XML parsing. |
| tests/unit/modules/default/weather/provider_utils_spec.js | Adds tests for new shared provider utility functions. |
| tests/unit/modules/default/utils_spec.js | Removes tests for deleted performWebRequest (CORS path). |
| tests/unit/functions/server_functions_spec.js | Removes CORS tests; keeps getUserAgent coverage and restores global config handling. |
| tests/mocks/weather_weathergov_points.json | Adds WeatherGov points fixture. |
| tests/mocks/weather_weathergov_forecast.json | Adds WeatherGov forecast fixture. |
| tests/mocks/weather_weathergov_current.json | Adds WeatherGov current conditions fixture. |
| tests/mocks/weather_weatherbit_hourly.json | Adds Weatherbit “no access” hourly error fixture. |
| tests/mocks/weather_weatherbit_forecast.json | Adds Weatherbit forecast fixture. |
| tests/mocks/weather_weatherbit.json | Adds Weatherbit current fixture. |
| tests/mocks/weather_openmeteo_current_weather.json | Adds OpenMeteo combined current+forecast fixture. |
| tests/mocks/weather_onecall_hourly.json | Updates OneCall hourly fixture (timezone metadata + timestamps). |
| tests/mocks/weather_onecall_forecast.json | Adds OneCall forecast fixture. |
| tests/mocks/weather_onecall_current.json | Adds OneCall current fixture. |
| tests/electron/modules/weather_spec.js | Updates Electron weather tests to use socket-injected fixtures (no weather_mocker). |
| tests/electron/helpers/weather-setup.js | Adds socket-based mock injector for Electron weather rendering tests. |
| tests/e2e/modules/weather_hourly_spec.js | Updates E2E tests to inject hourly fixture via helper. |
| tests/e2e/modules/weather_forecast_spec.js | Updates E2E tests to inject forecast fixture via helper. |
| tests/e2e/modules/weather_current_spec.js | Updates E2E tests to inject current fixture via helper. |
| tests/e2e/helpers/weather-functions.js | Adds socket-based weather fixture injection in Playwright helper. |
| tests/configs/modules/weather/hourlyweather_showPrecipitation.js | Updates weather test configs to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/hourlyweather_options.js | Updates weather test configs to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/hourlyweather_default.js | Updates weather test configs to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/forecastweather_units.js | Updates forecast config to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/forecastweather_options.js | Updates forecast config to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/forecastweather_default.js | Updates forecast config to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/forecastweather_absolute.js | Updates forecast config to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/currentweather_units.js | Updates current config to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/currentweather_options.js | Updates current config to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/currentweather_default.js | Updates current config to lat/lon + apiKey (no mockData). |
| tests/configs/modules/weather/currentweather_compliments.js | Updates current config to lat/lon + apiKey (no mockData). |
| js/server_functions.js | Removes CORS proxy endpoints/helpers and related exports. |
| js/server.js | Removes /cors and /version routes; keeps /startup via a local closure. |
| js/http_fetcher.js | Adds logContext and improves error logging and URL display. |
| eslint.config.mjs | Relaxes eqeqeq for null checks and disables import rules for new weather provider unit tests. |
| defaultmodules/weather/weatherutils.js | Guards precipitation unit conversion against null/undefined/NaN. |
| defaultmodules/weather/weatherobject.js | Makes sun-based calculations resilient when sunrise/sunset are unavailable. |
| defaultmodules/weather/weather.js | Migrates to server-side coordination via node_helper, removes client-side provider loading/scheduling. |
| defaultmodules/weather/providers/weatherflow.js | Refactors WeatherFlow provider to server-side HTTPFetcher pattern. |
| defaultmodules/weather/providers/weatherbit.js | Refactors Weatherbit provider to server-side HTTPFetcher pattern. |
| defaultmodules/weather/providers/ukmetofficedatahub.js | Refactors UK Met Office provider to server-side HTTPFetcher pattern. |
| defaultmodules/weather/providers/pirateweather.js | Refactors Pirate Weather provider to server-side HTTPFetcher pattern. |
| defaultmodules/weather/providers/README.md | Updates provider dev documentation link. |
| defaultmodules/weather/provider-utils.js | Adds shared provider utilities (timezone, sun times, coordinate validation, etc.). |
| defaultmodules/weather/node_helper.js | Adds server-side provider lifecycle and socket communication for weather. |
| defaultmodules/weather/current.njk | Avoids rendering sun icon when sun data is unavailable. |
| defaultmodules/utils.js | Removes CORS-related performWebRequest, leaving formatTime. |
| defaultmodules/weather/weatherprovider.js | Removes legacy client-side WeatherProvider registry/loader. |
| defaultmodules/weather/providers/overrideWrapper.js | Removes legacy override wrapper (override logic moved into module). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 78 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 78 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/unit/modules/default/weather/providers/ukmetofficedatahub_spec.js
Outdated
Show resolved
Hide resolved
KristjanESPERANTO
force-pushed
the
weather
branch
2 times, most recently
from
2e18b98 to
310a5ac
Compare
…PFetcher Migrate the OpenMeteo weather provider from client-side (performWebRequest + CORS proxy) to server-side architecture using HTTPFetcher for consistency with Calendar and Newsfeed modules. Changes: - Add node_helper.js for server-side weather provider management - Add openmeteo_server.js using HTTPFetcher with periodic auto-fetch - Modify weather.js with hybrid client/server-side provider support - Remove client-side openmeteo.js (now obsolete) Architecture: - weather.js sends INIT_WEATHER → node_helper loads provider - Provider uses HTTPFetcher for automatic polling (reloadInterval) - Data flows via callbacks → socket notifications → weather.js Benefits: - No CORS proxy needed - API keys stay server-side - Unified architecture with Calendar/Newsfeed - HTTPFetcher retry strategies (429/5xx) built-in
Add server-side OpenWeatherMap provider using HTTPFetcher, following the same pattern as OpenMeteo migration. Changes: - Add openweathermap_server.js with OnceCall API 3.0 support - Update weather.js to recognize openweathermap as server-side provider - Remove client-side openweathermap.js (now obsolete) Features: - Supports current, forecast/daily, and hourly weather types - Timezone offset handling for accurate timestamps - UV index, precipitation, and feels-like temperature - HTTPFetcher automatic polling and retry strategies
Add server-side WeatherGov (US National Weather Service) provider using HTTPFetcher. Changes: - Add weathergov_server.js with 2-step initialization (grid-point → station) - Update weather.js to recognize weathergov as server-side provider - Remove client-side weathergov.js (now obsolete) Implementation notes: - SunCalc integration for sunrise/sunset (API doesn't provide this) - User-Agent header required by API - US locations only, no API key required
Add server-side Yr.no (Norwegian Meteorological Institute) provider with proper HTTP caching support. Changes: - Add yr_server.js using HTTPFetcher for periodic weather fetching - Implement If-Modified-Since header support per API recommendations - Handle 304 Not Modified responses to reduce API calls - Cache Last-Modified and Expires headers for efficient updates - Fetch stellar data (sunrise/sunset) separately with daily caching - Update weather.js to recognize yr as server-side provider - Remove client-side yr.js (now obsolete) Implementation notes: - Enforce 10-minute minimum update interval per API terms - Coordinate precision limited to 4 decimals per API requirements - Weather data cached in-memory with Last-Modified tracking - Stellar data refetched daily or when using cached weather data - Sunrise API v3.0 with timezone offset parameter
Add server-side SMHI (Swedish Meteorological and Hydrological Institute) provider with precipitation category handling. Changes: - Add smhi_server.js using HTTPFetcher for periodic weather fetching - Implement gap filling for hourly data interpolation - Calculate apparent temperature using heat index formula - Handle precipitation categories (snow, rain, mixed, drizzle, freezing) - Support configurable precipitation values (pmin, pmean, pmedian, pmax) - Update weather.js to recognize smhi as server-side provider - Remove client-side smhi.js (now obsolete) Implementation notes: - Sweden only, metric system required - Coordinate precision limited to 6 decimals per API requirements - Data gaps filled by duplicating previous hour's data - Weather type selected from median of daytime hours for forecasts - Uses SunCalc for sunrise/sunset times
- Updated to new Environment Canada MSC Datamart API structure
* Changed base URL from /citypage_weather/ to /today/citypage_weather/
* Updated filename pattern to timestamped format: {timestamp}_MSC_CitypageWeather_{siteCode}_en.xml
- Implemented regex-based XML parsing (no external dependencies)
- Two-step data fetch: index page → city XML file
- Supports current conditions, forecast (12 periods), and hourly (24 hours)
- Features:
* Wind chill and humidex for feels-like temperature
* Today/Tonight forecast logic
* Sunrise/sunset times from XML
* Weather alerts/warnings support
- All three types tested and working (Toronto)
- Migrated from WeatherProvider.register() to HTTPFetcher-based provider - Supports current conditions, forecast (daily), and hourly forecasts - Features: * API key validation with helpful error messages * Precipitation type detection (rain/snow) * Apparent temperature (feels-like) support * Sunrise/sunset from daily data - Tested and working with valid API key
- Migrated from WeatherProvider.register() to HTTPFetcher-based provider - Supports current conditions, forecast (daily), and hourly (3-hourly) forecasts - Features: * API key validation with helpful error messages * SunCalc integration for sunrise/sunset times * All 31 Met Office significant weather codes mapped * Handles different field names for hourly vs 3-hourly data * Temperature averaging for 3-hourly data (max/min avg) * Precipitation probabilities for rain, snow, hail * Feels-like temperature support - Tested and working with valid API key (London)
All 10 weather providers now use the same import pattern.
- OpenMeteo: Fix hourly logic that skipped first valid future hour - OpenMeteo: Add bounds check to prevent array out-of-bounds crash - Pirateweather: Fix null-safe checks to preserve 0 values (humidity, feelsLike, windSpeed, tempMin/Max, precipProb)
- http_fetcher: Wrap URL parsing in try-catch to prevent crash during error logging
- E2E weather-functions: Fix precipitationProbability to use null-safe check - Electron weather-setup: Fix precipitationProbability to use null-safe check - Ensures 0% precipitation is preserved instead of becoming undefined
- openmeteo_spec: Remove debug console.log statements - envcanada_spec: Fix comment (windChill is -31, not -12) - envcanada_spec: Fix inverted callbacks in error handling test - smhi_spec: Remove unused SMHI_URL constant
- Split precipitationProbability/uvIndex assignments - Split precip initialization and if-statement - Improves readability and follows coding standards
- Fixes inconsistent field names across providers - Templates expect windFromDirection (current.njk uses it) - Affected providers: Pirateweather, UkMetOffice, Weatherbit, WeatherFlow - Ensures wind direction displays correctly in UI
- provider-utils.js: Remove unused Log import - openmeteo_spec.js: Remove unused beforeEach import and OPEN_METEO_URL constant
- Remove unused errorPromise variable - Simplify callback setup without unused resolve/reject - Makes test intent clearer
- Adds back getVersion() function and /version route - Was incorrectly removed - /version endpoint is unrelated to weather module - Restores public API endpoint
- Increase timeout from 5s to 10s for geocoding API - Change error log level from ERROR to DEBUG - Geocoding is optional (only for location name display)
Restore original day/night-specific icons and missing codes 41-48 (tornado, windy, smoke, sandstorm, thunderstorm variants) that were lost during migration.
Rename windDirection → windFromDirection in generateHourly() and precipitation → precipitationAmount in generateForecast() and generateHourly() to match WeatherObject properties.
Compare year, month, and day when matching hourly data to daily forecasts to prevent incorrect matches across month boundaries (e.g., Jan 31 vs Feb 31).
Use waitForSelector and waitForFunction instead of fixed 1000ms/500ms delays in weather-setup.js for more robust and faster test execution.
Move describe() and it() opening to separate lines for better readability.
Ensure current.temperature is always set (either to a value, cached value, or null) to prevent undefined temperature in weather display.
Environment Canada's XML feed now returns <currentConditions/> as an empty element. Adapt by extracting current weather from the first forecast period instead. Additional fixes: - Add missing return when city page URL not found - Restore sunrise/sunset data (from riseSet element) - Accept both "high" and "low" temperature classes
…urrentConditions Wind speed, bearing, temperature, and humidity now correctly read from currentConditions element instead of forecast, with fallback logic.
More robust property existence check in PirateWeather provider instead of value-based undefined check.
…dates in daily mock response
KristjanESPERANTO
force-pushed
the
weather
branch
from
e13d17e to
3ab5f8a
Compare
…tUrl method Aligns with existing pattern in openweathermap, weatherapi providers.
…ackoff Network errors now use exponential backoff strategy instead of fixed reloadInterval delay, enabling faster recovery from transient issues. Changes: - Add networkErrorCount tracking alongside serverErrorCount - Network errors retry at: 15s → 30s → 60s → cap at reloadInterval - Gradual log-level escalation: WARN for first 2 attempts, ERROR after - Extract retry calculation to static HTTPFetcher.calculateBackoffDelay() - Apply same backoff strategy to WeatherGov initialization retries - Reset both counters on successful response Benefits: - Faster recovery from transient network glitches (15s vs 10min) - Less log spam for temporary issues (WARN vs ERROR initially) - Consistent retry behavior across HTTPFetcher and provider init - Reusable backoff calculation for future providers Example: SMHI "fetch failed" now retries after 15s instead of 10min.
…d hourly weather data
KristjanESPERANTO
force-pushed
the
weather
branch
from
e17fbf6 to
be9c24a
Compare
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.
This migrates the Weather module from client-side fetching to use the server-side centralized HTTPFetcher (introduced in MagicMirrorOrg#4016), following the same pattern as the Calendar and Newsfeed modules.
Motivation
This brings consistent error handling and better maintainability and completes the refactoring effort to centralize HTTP error handling across all default modules.
Migrating to server-side providers with HTTPFetcher brings:
Changes
Breaking Changes
None. Existing configurations continue to work.
Testing
To ensure proper functionality, I obtained API keys and credentials for all providers that require them. I configured all 10 providers in a carousel setup and tested each one individually. Screenshots for each provider are attached below demonstrating their working state.
I even requested developer access from the Tempest/WeatherFlow team to properly test their provider.
Comprehensive test coverage: A major advantage of the server-side architecture is the ability to thoroughly test providers with unit tests using real API response snapshots. Don't be alarmed by the many lines added in this PR - they are primarily test files and real-data mocks that ensure provider reliability.
Related
Part of the HTTPFetcher migration MagicMirrorOrg#4016.