fix(weather overrides): ensure json format for features

[SkrubbySkrubInAShrub]

This PR changes the loading and saving logic to be consistent with the json format initially defined, with feature overriders being nested in "featureSettings".

Summary by CodeRabbit

• Refactor

  • Improved weather settings storage to use a nested feature structure with stronger validation.
  • Ensures safe read/write behavior and preserves caching while reducing risk of corrupted or missing data.
  • Cleans up unused per-weather settings files when no features remain.

• Bug Fixes

  • Prevents errors when feature settings are absent or malformed.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Per-weather load/save logic in src/WeatherManager.cpp was changed to nest feature settings under weatherData["featureSettings"]; loading iterates that object with guards, saving ensures/creates featureSettings, updates/removes entries there, and deletes the weather file if no features remain.

Changes

Cohort / File(s)	Summary
Weather settings read/write update src/WeatherManager.cpp	LoadPerWeatherSettingsFromDisk now expects and iterates weatherData["featureSettings"] (with guards for missing/non-object). SaveSettingsToWeather ensures weatherData and weatherData["featureSettings"] exist, writes/removes per-feature entries under featureSettings, removes the weather file when featureSettings becomes empty, preserves cache behaviour, and logs removals/saves.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• davo0411
• doodlum

Poem

🐰 I hopped through JSON, neat and bright,
Nested features tucked away just right,
I moved the keys and swept the slate,
Now weather files rest tidy and straight 🌦️

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: restructuring JSON format for weather feature overrides to use nested featureSettings.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Using provided base ref: 245a433
Using base ref: 245a433
Base commit date: 2026-01-27T12:19:11+10:00 (Tuesday, January 27, 2026 12:19 PM)
No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/WeatherManager.cpp (1)

151-179: Legacy entries can be dropped on save.

When a legacy flat file is saved, only the edited feature gets written into featureSettings while the other legacy keys remain at the top level. Subsequent loads prefer featureSettings and ignore those remaining keys, effectively losing settings and not removing the legacy format. Consider migrating legacy keys into featureSettings (and stripping them) before updating.

🛠️ Suggested fix (migrate legacy keys before update)

-	// Ensure weatherData has featureSettings object
-	if (!weatherData.contains("featureSettings") || !weatherData["featureSettings"].is_object()) {
-		weatherData["featureSettings"] = json::object();
-	}
+	// Normalize legacy flat format into featureSettings
+	if (!weatherData.is_object()) {
+		weatherData = json::object();
+	}
+	if (!weatherData.contains("featureSettings")) {
+		json legacy = std::move(weatherData);
+		weatherData = json::object();
+		weatherData["featureSettings"] = std::move(legacy);
+	} else if (!weatherData["featureSettings"].is_object()) {
+		logger::warn("Invalid featureSettings in weather file ({}), resetting to empty object", filePath);
+		weatherData["featureSettings"] = json::object();
+	}
+	// Merge any leftover legacy keys into featureSettings, then drop them
+	for (auto it = weatherData.begin(); it != weatherData.end();) {
+		if (it.key() == "featureSettings") {
+			++it;
+			continue;
+		}
+		if (it.value().is_object()) {
+			weatherData["featureSettings"][it.key()] = it.value();
+			it = weatherData.erase(it);
+		} else {
+			++it;
+		}
+	}

[github-actions]

✅ A pre-release build is available for this PR:
Download

[SkrubbySkrubInAShrub]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)

Edgecase is very small for the amount of code, I do not expect enough people to have done enough edits for that amount of code to be worthwhile. the legacy support should be removed around 1.5.1 or later anyway.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/WeatherManager.cpp (2)

50-57: Legacy-format files are no longer loaded.

Line 52 only reads featureSettings, so legacy top‑level feature entries are ignored. This contradicts the PR description (“legacy-format files remain readable”) and the commit message hinting at removal. Please clarify the intended scope; if legacy support is still required, add a fallback to read the legacy root. Based on learnings, please explicitly confirm scope alignment.

🛠️ Suggested fallback (legacy-compatible)

-			// The structure is expected to be: { "featureSettings": { "FeatureName": { settings }, ... }
-			if (weatherData.is_object() && weatherData.contains("featureSettings") && weatherData["featureSettings"].is_object()) {
-				for (auto& [featureName, featureSettings] : weatherData["featureSettings"].items()) {
-					perWeatherSettingsCache[weatherKey][featureName] = featureSettings;
-				}
-				logger::info("Loaded settings for weather: {}", weatherKey);
-			}
+			// Expected: { "featureSettings": { "FeatureName": { settings }, ... } }
+			// Legacy:   { "FeatureName": { settings }, ... }
+			const json* featureRoot = nullptr;
+			if (weatherData.is_object()) {
+				if (weatherData.contains("featureSettings") && weatherData["featureSettings"].is_object()) {
+					featureRoot = &weatherData["featureSettings"];
+				} else {
+					featureRoot = &weatherData;  // legacy fallback
+				}
+			}
+			if (featureRoot && featureRoot->is_object()) {
+				for (auto& [featureName, featureSettings] : featureRoot->items()) {
+					perWeatherSettingsCache[weatherKey][featureName] = featureSettings;
+				}
+				logger::info("Loaded settings for weather: {}", weatherKey);
+			}

---

160-187: Guard non-object JSON and migrate legacy keys on save.

If an existing file parses to a non-object root, weatherData["featureSettings"] can throw a type_error (Line 162). Also, resaving doesn’t purge legacy top‑level feature keys, so the file remains in the incorrect format. Consider coercing to an object and migrating/cleaning legacy keys before updating. As per coding guidelines, favor robust error handling and graceful degradation.

🛠️ Suggested fix (robust + legacy cleanup)

-	// Ensure weatherData has featureSettings object
-	if (!weatherData.contains("featureSettings") || !weatherData["featureSettings"].is_object()) {
-		weatherData["featureSettings"] = json::object();
-	}
+	// Ensure weatherData is an object and has featureSettings
+	if (!weatherData.is_object()) {
+		weatherData = json::object();
+	}
+	if (!weatherData.contains("featureSettings") || !weatherData["featureSettings"].is_object()) {
+		weatherData["featureSettings"] = json::object();
+	}
+	auto& featureSettings = weatherData["featureSettings"];
+	// Migrate any legacy top-level entries into featureSettings and remove them
+	for (auto it = weatherData.begin(); it != weatherData.end(); ) {
+		if (it.key() == "featureSettings") {
+			++it;
+			continue;
+		}
+		if (!featureSettings.contains(it.key())) {
+			featureSettings[it.key()] = it.value();
+		}
+		it = weatherData.erase(it);
+	}
 
 	// Update with new feature settings or remove feature entry if settings empty
 	if (settings.is_object() && settings.empty()) {
 		// Remove feature entry from loaded JSON
-		if (weatherData["featureSettings"].is_object()) {
-			weatherData["featureSettings"].erase(featureName);
-		}
+		featureSettings.erase(featureName);
 	} else {
-		weatherData["featureSettings"][featureName] = settings;
+		featureSettings[featureName] = settings;
 	}
 
 	// Write back to disk
-	if (weatherData["featureSettings"].is_object() && weatherData["featureSettings"].empty()) {
+	if (featureSettings.empty()) {

🧹 Nitpick comments (1)

src/WeatherManager.cpp (1)

160-163: PR metadata: consider a shorter conventional title + issue reference.

Suggested conventional title (≤50 chars): fix(weather): normalize per-weather json format.
If this PR fixes a tracked bug, please add an issue reference like “Fixes #123” or “Closes #123” in the description.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/WeatherManager.cpp (1)

51-57: Legacy format not loaded despite PR description claiming support.

The PR description states "The legacy format is still parsed on load," but this implementation only loads settings when weatherData["featureSettings"] exists. Files in the legacy flat format (features at top level) will be silently ignored, not loaded.

If legacy support is intentionally omitted (as suggested in your comment about removing legacy support around v1.5.1), consider updating the PR description to reflect this. Alternatively, add legacy loading:

🔧 Suggested legacy support (if desired)

 			// Store the entire weather settings in cache
 			// The structure is expected to be: { "featureSettings": { "FeatureName": { settings }, ... }
 			if (weatherData.is_object() && weatherData.contains("featureSettings") && weatherData["featureSettings"].is_object()) {
 				for (auto& [featureName, featureSettings] : weatherData["featureSettings"].items()) {
 					perWeatherSettingsCache[weatherKey][featureName] = featureSettings;
 				}
 				logger::info("Loaded settings for weather: {}", weatherKey);
+			} else if (weatherData.is_object()) {
+				// Legacy format: features at top level
+				for (auto& [featureName, featureSettings] : weatherData.items()) {
+					perWeatherSettingsCache[weatherKey][featureName] = featureSettings;
+				}
+				logger::info("Loaded settings for weather (legacy format): {}", weatherKey);
 			}

🤖 Fix all issues with AI agents

In `@src/WeatherManager.cpp`:
- Around line 160-176: The file leaves legacy top-level feature entries in
weatherData instead of migrating them into the new featureSettings map; update
WeatherManager.cpp to, after ensuring weatherData and featureSettings exist but
before assigning featureSettings[featureName] or erasing it, iterate the
top-level keys of weatherData and for each key != "featureSettings" (and not
other known top-level keys if any) where the value is an object, move that value
into featureSettings[key] and erase the original top-level entry; then proceed
with the existing logic using featureSettings, so legacy entries get migrated
(or removed) and you don't produce a hybrid file.

🧹 Nitpick comments (1)

src/WeatherManager.cpp (1)

178-188: Consider error-code-based file removal to avoid TOCTOU.

The exists() check followed by remove() has a time-of-check to time-of-use race. Using the error-code overload of std::filesystem::remove is both safer and cleaner. Based on learnings, this pattern is preferred for file deletion.

♻️ Suggested refactor

 	// Write back to disk
 	if (featureSettings.empty()) {
 		// No features left for this weather — remove file if it exists
-		if (std::filesystem::exists(filePath)) {
-			try {
-				std::filesystem::remove(filePath);
+		std::error_code ec;
+		if (std::filesystem::remove(filePath, ec)) {
 				logger::info("Removed weather settings file (no features remain): {}", filePath);
-			} catch (const std::filesystem::filesystem_error& e) {
-				logger::warn("Failed to remove empty weather settings file ({}): {}", filePath, e.what());
-			}
+		} else if (ec) {
+			logger::warn("Failed to remove empty weather settings file ({}): {}", filePath, ec.message());
 		}
 		return;
 	}

[doodlum]

check ai

[SkrubbySkrubInAShrub]

check ai

I did, added guarding but adding code to clean up legacy jsons seems a bit out of scope if Davo also tells me to not read the legacy jsons. If we don't want code bloat for supporting an invalid format, then why should we have code bloat for fixing up those invalid formats? current implementation will still read the file just fine, but ignore the improperly saved objects.

[SkrubbySkrubInAShrub]

gonna recheck some saving behaviour

[SkrubbySkrubInAShrub]

ok no, I can't break the saving system. While there may be some jank left over from the original feature, I do not feel confident in removing that yet with my knowledge of the codebase. In either case, manually breaking the jsons doesn't cause unexpected behaviour.

[davo0411]

remove any legacy code, don't add tech debt by meaning we need to redo it in future. this feature is very new so we don't need to uphold compatibility.

[davo0411]

just make sure no legacy support otherwise gud

[SkrubbySkrubInAShrub]

yeah no legacy support at all.