Add setting to adjust temperature number display style #549
Conversation
📝 WalkthroughWalkthroughAdds a new integer configuration property TemperatureDecimalDigits across backend interfaces and models, Web API adapters and DTOs, frontend API types and the clock UI; test resources and tests updated to include and validate the new property. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Server as Server (GeneralSettings)
participant Adapter as WebAPI Adapter
participant API as Web API / DTO
participant Client as Frontend (Config Store)
participant UI as Clock Component
Note over Server,Adapter: New property added: TemperatureDecimalDigits
Server->>Adapter: expose TemperatureDecimalDigits
Adapter->>API: include TemperatureDecimalDigits in ClientSettingsDto
API->>Client: send client settings (temperatureDecimalDigits)
Client->>UI: provide $configStore.temperatureDecimalDigits
UI->>UI: render (weather.temperature ?? 0).toFixed(temperatureDecimalDigits ?? 1)
alt invalid value (outside 0..2)
Adapter-->>Server: throws ArgumentOutOfRangeException during Validate()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @immichFrame.Web/src/lib/components/elements/clock.svelte:
- Line 82: The current JSX renders undefined temperatures inconsistently: when
$configStore.useWholeNumberTemperatures is true it falls back to 0 via "?? 0",
but when false it uses "?.toFixed(1)" which yields nothing for undefined; change
the expression so both branches operate on the same normalized value (e.g.,
const temp = weather.temperature ?? 0 or use a shared fallback like '-' for
missing data) and then apply either Math.round(temp) or temp.toFixed(1) based on
$configStore.useWholeNumberTemperatures; update the div with class
"weather-temperature" to use that normalized value so undefined handling is
consistent across both branches.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Models/ServerSettings.csimmichFrame.Web/src/lib/components/elements/clock.svelteimmichFrame.Web/src/lib/immichFrameApi.ts
🧰 Additional context used
🧬 Code graph analysis (3)
immichFrame.Web/src/lib/immichFrameApi.ts (1)
immichFrame.Web/src/lib/stores/config.store.ts (1)
ps(7-9)
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)
ImmichFrame.WebApi/Controllers/ConfigController.cs (1)
ApiController(7-33)
ImmichFrame.WebApi/Models/ServerSettings.cs (1)
ImmichFrame.WebApi/Helpers/SettingsExtensonMethods.cs (1)
IConfigSettable(6-9)
🔇 Additional comments (7)
immichFrame.Web/src/lib/immichFrameApi.ts (1)
209-209: LGTM!The optional boolean property follows the established pattern for client settings and maintains backward compatibility.
ImmichFrame.WebApi/Models/ServerSettings.cs (1)
60-60: LGTM!The property follows the established pattern with an appropriate default value of
falsethat maintains backward compatibility.ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
59-59: LGTM!The property is appropriately defined in the interface with logical placement among other weather-related settings.
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
123-123: LGTM!Hardcoding to
falseis appropriate for the V1 adapter, as the original V1 settings format did not support this property. This ensures backward compatibility for users with legacy configuration files.immichFrame.Web/src/lib/components/elements/clock.svelte (1)
82-82: No action needed—the UseWholeNumberTemperatures setting is properly mapped from GeneralSettings to ClientSettingsDto.cs and will be received by the frontend.ImmichFrame.WebApi/Models/ClientSettingsDto.cs (2)
27-27: LGTM! Property declaration is clean and well-positioned.The new
UseWholeNumberTemperaturesboolean property is correctly declared and logically positioned among other weather-related settings.
58-58: LGTM! Assignment follows the established pattern.The assignment correctly maps the property from
IGeneralSettingsto the DTO and maintains consistency with the surrounding code.
|
I would be good just always using a whole number. Feel like this has gone back and forth. This is fine too just would prefer less settings. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
120-128: Consider adding test coverage forUseWholeNumberTemperatures = falsein V2 configs.The special-case logic correctly handles the property for V1 configs (defaults to false) and V2 configs with the value set to true. However, there's no test case verifying V2 configs with
UseWholeNumberTemperatures = false.For comprehensive coverage, consider adding a test case (e.g., TestV2_UseWholeNumberTemperaturesFalse.json) to verify that V2 configs explicitly setting this property to false are parsed correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.csImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlimmichFrame.Web/src/lib/components/elements/clock.svelte
🧰 Additional context used
🧬 Code graph analysis (2)
ImmichFrame.WebApi.Tests/Resources/TestV2.json (2)
ImmichFrame.Core/Models/Weather.cs (2)
Weather(5-13)ImmichFrame(3-14)ImmichFrame.WebApi/Controllers/WeatherController.cs (1)
ImmichFrame(5-29)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
ImmichFrame.WebApi/Models/ServerSettings.cs (1)
GeneralSettings(38-76)
🔇 Additional comments (4)
ImmichFrame.WebApi.Tests/Resources/TestV2.json (1)
33-33: LGTM!The test data correctly includes the new
UseWholeNumberTemperaturessetting with a value oftrue, which appropriately tests the feature when enabled.ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
73-73: LGTM!Making the prefix parameter explicit improves code clarity without changing functionality.
ImmichFrame.WebApi.Tests/Resources/TestV2.yml (1)
32-32: LGTM!The YAML test data correctly mirrors the JSON test configuration, maintaining consistency across test resources.
immichFrame.Web/src/lib/components/elements/clock.svelte (1)
82-82: Excellent fix for the undefined temperature handling!The updated code correctly addresses the past review feedback by ensuring both branches handle undefined temperatures consistently using the
?? 0fallback. The logic now properly displays temperatures as whole numbers when the setting is enabled, and with one decimal place otherwise.
|
Some people would like one digit, you like none, some like two or more... Did you check for a possibility to hand over a format instead of a bool? I'd prefer this more. I'm not sure how precise openweathermap delivers the data, might be worth checking. |
|
I’d also like to remove the C/F and just use degree symbol. |
… int (0-2) for flexible temperature precision
nopoz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @ImmichFrame.Core/Interfaces/IServerSettings.cs:
- Line 59: GeneralSettings.Validate() must enforce that TemperatureDecimalDigits
is between 0 and 2; update the GeneralSettings.Validate() implementation to
check the TemperatureDecimalDigits property and throw an
ArgumentOutOfRangeException (or return/collect a validation error) if the value
is < 0 or > 2, including a clear message referencing TemperatureDecimalDigits so
callers know the allowed range.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.csImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Models/ServerSettings.csimmichFrame.Web/src/lib/components/elements/clock.svelteimmichFrame.Web/src/lib/immichFrameApi.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
- immichFrame.Web/src/lib/immichFrameApi.ts
- immichFrame.Web/src/lib/components/elements/clock.svelte
- ImmichFrame.WebApi/Models/ServerSettings.cs
🧰 Additional context used
🧬 Code graph analysis (1)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
ImmichFrame.WebApi/Models/ServerSettings.cs (1)
GeneralSettings(38-76)
🔇 Additional comments (4)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
123-123: LGTM!The hardcoded default of
1for V1 configurations provides sensible backwards compatibility, matching the default inServerSettings.cs.ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)
27-27: LGTM!The property addition and mapping follow the established pattern for other settings in the DTO. The implementation correctly propagates the
TemperatureDecimalDigitsvalue fromIGeneralSettingsto the client-facing DTO.Also applies to: 58-58
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (2)
73-73: VerifyProperties signature change is correctly implemented and consistent.The method signature now includes a
prefixparameter with a default value, and both calls in the file properly use the updated signature: the first passes an empty string explicitly, and the second passes a conditional prefix expression. The refactoring is consistent across all usages.
122-131: Test is correct as written.The test value of 7 is consistent with the V2 test resource files (TestV2.yml and TestV2.json both specify
TemperatureDecimalDigits: 7). The test logic correctly validates that V1 config defaults to 1 while V2 config uses the loaded value from the test data. No range validation for 0-2 exists in the codebase, and the property accepts the value without constraints.Likely an incorrect or invalid review comment.
I assume you're playing devil's advocate and don't actually want 2 digits of temp displayed yourself? 😅 Anyways, it's a fair point so I've changed the setting to openweathermap API provides two decimal places in their temp values |
See #553 |
|
I'm good with this. |
3rob3
left a comment
3rob3
left a comment
There was a problem hiding this comment.
Can you update the Settings.example.json, Settings.example.yml, and example.env with this new setting please?
3 participants
Adds boolean setting "UseWholeNumberTemperatures" to display temperature as integer (whole numbers) in order to simplify display.
When set to false or not set, the default is to display Temperature as decimal.
Summary by CodeRabbit
New Features
Validation
Tests
Documentation / Examples
✏️ Tip: You can customize this high-level summary in your review settings.