Add setting to adjust temperature number display style #549
Conversation
📝 WalkthroughWalkthroughIntroduces a new integer configuration 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 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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.
| public string Style { get; } | ||
| public string? BaseFontSize { get; } | ||
| public bool ShowWeatherDescription { get; } | ||
| public int TemperatureDecimalDigits { get; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for TemperatureDecimalDigits in the codebase to find validation logic
rg -n -A 10 -B 5 'TemperatureDecimalDigits' --type cs -g '!*Test*' | head -100Repository: immichFrame/ImmichFrame
Length of output: 7716
🌐 Web query:
OpenWeatherMap API temperature data precision decimal places
💡 Result:
OpenWeatherMap returns temperature as a numeric (floating‑point) value in the JSON (examples show two decimal places, e.g. 280.32, 298.48, 282.65). The documentation does not promise a fixed number of decimal digits—precision can vary by product/station and endpoint—so treat it as a float, not a guaranteed fixed‑decimal field. [1][2]
Sources:
- OpenWeatherMap API example responses showing temp values (e.g. 280.32, 298.48). [1]
- One Call / other API docs with temp as numeric (examples with two decimals) and units description. [2]
🏁 Script executed:
#!/bin/bash
# Search for Validate method implementation in ServerSettings classes
rg -n -A 15 'public void Validate()' --type cs -g '!*Test*' | head -80Repository: immichFrame/ImmichFrame
Length of output: 6297
Add validation for TemperatureDecimalDigits range (0-2).
The GeneralSettings.Validate() method is empty, allowing TemperatureDecimalDigits to accept any integer value despite the intended constraint of 0-2. OpenWeatherMap returns floating-point temperature data with variable precision; enforce this range to prevent invalid user input and ensure display logic matches the precision of temperature data available from the API.
🤖 Prompt for AI Agents
In @ImmichFrame.Core/Interfaces/IServerSettings.cs at 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.
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 |
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
✏️ Tip: You can customize this high-level summary in your review settings.