fix: TimeZoneNotFoundException on old windows#20
Conversation
WalkthroughAdds a configuration object and DI overloads, makes BlazorLocalTimeProvider ICU-aware and optionally converts IANA→Windows IDs via a pluggable converter, internalizes/refactors local-time service/interface, adds NLS-specific tests and test project, updates solution structure and CI matrix, and adjusts test mocks and DI helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser JS
participant Provider as BlazorLocalTimeProvider
participant Config as BlazorLocalTimeConfiguration
participant TZ as TimeZoneInfo API
participant Service as LocalTimeService
Browser->>Provider: getBrowserTimeZone() -> IANA id
Provider->>Provider: IsIcuEnabled()?
alt ICU enabled
Provider->>TZ: FindSystemTimeZoneById(IANA)
else ICU not enabled
alt IanaToWindows provided
Provider->>Config: IanaToWindows(IANA)
Config-->>Provider: WindowsId
Provider->>TZ: FindSystemTimeZoneById(WindowsId)
else No converter
Provider-->>Provider: throw TimeZoneNotFoundException (guidance)
end
end
TZ-->>Provider: TimeZoneInfo
Provider->>Service: SetBrowserTimeZoneInfo(TimeZoneInfo)
Service-->>Service: Raise LocalTimeZoneChanged (if effective TZ changed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorLocalTime/LocalTimeService.cs (1)
16-16: Make BrowserTimeZoneInfo setter private and update tests
The setter is currently internal but tests in tests/BlazorLocalTimeTest/LocalTimeMockService.cs (lines 16, 25) assign it directly. Change the setter toprivateand modify those tests to callSetBrowserTimeZoneInfoinstead of assigning the property directly.
🧹 Nitpick comments (28)
tests/Directory.Build.props (1)
18-21: Add a global using for Bunit (tests convenience)Most test files will reference Bunit types; add a global using to reduce per-file imports.
<ItemGroup> <Using Include="Xunit" /> <Using Include="Shouldly" /> + <Using Include="Bunit" /> </ItemGroup>src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs (2)
6-6: Consider sealing the configuration typeSealing prevents unintended inheritance and makes the intent (a simple options bag) explicit.
-public class BlazorLocalTimeConfiguration +public sealed class BlazorLocalTimeConfiguration
8-19: Tighten XML docs: fix “Gets” vs “Gets or sets”, wording, and linksMinor doc polish for accuracy and readability.
- /// <summary> - /// Gets the <see cref="TimeProvider"/> used to supply the current time. - /// </summary> + /// <summary> + /// Gets or sets the <see cref="TimeProvider"/> used as the current time source. + /// </summary> @@ - /// <summary> - /// Specifies a function to convert IANA time zones to Windows time zones. - /// For example, `TZConvert.IanaToWindows` from TimeZoneConverter(https://github.com/mattjohnsonpint/TimeZoneConverter) can be used. - /// This is required on operating systems where ICU is unavailable(such as Windows Server 2016), but need not be specified on others. - /// For details, see https://github.com/arika0093/BlazorLocalTime/issues/19 - /// </summary> + /// <summary> + /// Specifies a function to convert IANA time zone IDs to Windows time zone IDs. + /// For example, you can pass <c>TZConvert.IanaToWindows</c> from the + /// <see href="https://github.com/mattjohnsonpint/TimeZoneConverter">TimeZoneConverter</see> package. + /// Required on operating systems where ICU is unavailable (such as Windows Server 2016); optional otherwise. + /// See <see href="https://github.com/arika0093/BlazorLocalTime/issues/19" /> for details. + /// </summary>tests/BlazorLocalTimeTest/LocalTimeTest.razor (1)
75-76: Minor naming nitRename tms to mock or service for readability in tests.
- var tms = new LocalTimeEmptyMockService(new() { TimeProvider = TimeProvider.System }); - Services.AddLocalTimeMockService(tms); + var mock = new LocalTimeEmptyMockService(new() { TimeProvider = TimeProvider.System }); + Services.AddLocalTimeMockService(mock);tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeTest.Nls.csproj (1)
15-17: Avoid test-to-test project reference; extract shared helpers instead.Referencing the base test project pulls all its tests as a dependency and couples test runs. Prefer a small shared test-utilities project referenced by both test projects.
- <ItemGroup> - <ProjectReference Include="..\..\src\BlazorLocalTime\BlazorLocalTime.csproj" /> - <ProjectReference Include="..\BlazorLocalTimeTest\BlazorLocalTimeTest.csproj" /> - </ItemGroup> + <ItemGroup> + <ProjectReference Include="..\..\src\BlazorLocalTime\BlazorLocalTime.csproj" /> + <!-- New shared helpers project: move TestInitializer, mocks, etc. here --> + <ProjectReference Include="..\BlazorLocalTimeTest.Common\BlazorLocalTimeTest.Common.csproj" /> + </ItemGroup>If you want, I can sketch the Common project layout and file moves.
tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs (1)
19-19: Reduce platform variance by avoiding IANA IDs in FindSystemTimeZoneById.On non-ICU Windows, “America/New_York” isn’t resolvable. Since this test doesn’t exercise ICU/NLS behavior, prefer a Windows-friendly ID (e.g., “Tokyo Standard Time”) or use the already-resolved Asia/Tokyo from elsewhere for consistency.
- var timeZone = TimeZoneInfo.FindSystemTimeZoneById("America/New_York"); + // Prefer a cross-platform-friendly ID to avoid ICU/NLS variance in this test + var timeZone = TimeZoneInfo.FindSystemTimeZoneById( + OperatingSystem.IsWindows() ? "Tokyo Standard Time" : "Asia/Tokyo");tests/BlazorLocalTimeTest/LocalTimeFormTest.razor (1)
152-153: Nit: fix “Blazil” → “Brazil”.Tiny spelling tweak in comments.
- // Change timezone to Blazil (UTC-3) + // Change timezone to Brazil (UTC-3)Also applies to: 178-179
tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeProviderTest.razor (2)
31-35: Nit: wording.Minor comment fix.
- // can convertable + // convertible
1-35: Optional: add a Windows-NLS job to CI for this test.To actually exercise the failure path, run tests on windows-latest with DOTNET_SYSTEM_GLOBALIZATION_USENLS=1; keep Linux/macOS to cover ICU.
src/BlazorLocalTime/BlazorLocalTimeExtension.cs (1)
55-61: Prefer TryAddSingleton and guard null configuration.Prevents accidental duplicate registrations and null delegates.
- services.AddSingleton<BlazorLocalTimeConfiguration>(_ => { - var config = new BlazorLocalTimeConfiguration(); - configuration(config); - return config; - }); + if (configuration is null) throw new ArgumentNullException(nameof(configuration)); + services.TryAddSingleton<BlazorLocalTimeConfiguration>(_ => + { + var cfg = new BlazorLocalTimeConfiguration(); + configuration(cfg); + return cfg; + });src/BlazorLocalTime/Components/BlazorLocalTimeProvider.razor.cs (2)
41-53: Improve message and resilience when converter is missing.
- Make the exception actionable (include the offending ID and setup hint).
- Minor grammar tweak in the comment.
- if(!ICUMode()) { - // On Windows with NLS mode, IANA time zone are must be converted to Windows time zone. + if (!ICUMode()) { + // On Windows with NLS mode, IANA time zones must be converted to Windows time zones. var converter = Configuration.IanaToWindows; - if(converter == null) { - var message = """ - In older Windows environments, IANA time zones (such as “Asia/Tokyo”) cannot be used directly. - For details, see https://github.com/arika0093/BlazorLocalTime/issues/19. - """; - throw new TimeZoneNotFoundException(message); + if (converter == null) { + var message = + $"IANA time zone '{timeZoneString}' cannot be used on this platform without a converter. " + + "Set BlazorLocalTimeConfiguration.IanaToWindows (e.g., TZConvert.IanaToWindows). " + + "See https://github.com/arika0093/BlazorLocalTime/issues/19."; + throw new TimeZoneNotFoundException(message); } - timeZoneString = converter(timeZoneString); + timeZoneString = converter(timeZoneString); } timeZone = TimeZoneInfo.FindSystemTimeZoneById(timeZoneString);Alternative (more robust across platforms): try resolving the IANA ID first, and only fall back to conversion on TimeZoneNotFoundException. This avoids relying on ICU detection.
- var timeZoneString = await module.InvokeAsync<string>("getBrowserTimeZone"); - if(!ICUMode()) { - ... - } - timeZone = TimeZoneInfo.FindSystemTimeZoneById(timeZoneString); + var timeZoneString = await module.InvokeAsync<string>("getBrowserTimeZone"); + try + { + timeZone = TimeZoneInfo.FindSystemTimeZoneById(timeZoneString); + } + catch (TimeZoneNotFoundException) + { + var converter = Configuration.IanaToWindows; + if (converter is null) + { + throw new TimeZoneNotFoundException( + $"IANA time zone '{timeZoneString}' cannot be used on this platform without a converter. " + + "Set BlazorLocalTimeConfiguration.IanaToWindows (e.g., TZConvert.IanaToWindows). " + + "See https://github.com/arika0093/BlazorLocalTime/issues/19."); + } + var windowsId = converter(timeZoneString); + timeZone = TimeZoneInfo.FindSystemTimeZoneById(windowsId); + }
79-86: Nit: cache ICU detection result.ICU mode won’t change at runtime; consider caching to a static readonly bool.
- private static bool ICUMode() + private static readonly bool s_isIcu = ComputeIcuMode(); + private static bool ICUMode() => s_isIcu; + private static bool ComputeIcuMode()tests/BlazorLocalTimeTest/LocalTimeTotalTest.razor (4)
9-10: Drop redundant TimeProvider registrationThe options-based AddBlazorLocalTimeService already conveys the TimeProvider; the extra singleton can cause ambiguity and is unused. Remove it.
- Services.AddBlazorLocalTimeService(option => option.TimeProvider = timeProvider); - Services.AddSingleton<TimeProvider>(timeProvider); + Services.AddBlazorLocalTimeService(option => option.TimeProvider = timeProvider);
22-22: Unnecessary DI entry for TimeProviderThis singleton isn’t required with the new configuration-based registration.
- Services.AddSingleton<TimeProvider>(TimeProvider.System);
36-37: Same here: remove the extra TimeProvider singletonKeep only the AddBlazorLocalTimeService configuration.
- Services.AddBlazorLocalTimeService(option => option.TimeProvider = timeProvider); - Services.AddSingleton<TimeProvider>(timeProvider); + Services.AddBlazorLocalTimeService(option => option.TimeProvider = timeProvider);
49-50: Remove redundant singleton registrationAvoid duplicate TimeProvider entries.
- Services.AddBlazorLocalTimeService(option => option.TimeProvider = timeProvider); - Services.AddSingleton<TimeProvider>(timeProvider); + Services.AddBlazorLocalTimeService(option => option.TimeProvider = timeProvider);tests/BlazorLocalTimeTest/LocalTimeMockService.cs (4)
9-17: Make Config immutableDeclare Config as readonly to prevent accidental reassignment during tests.
- internal BlazorLocalTimeConfiguration Config; + internal readonly BlazorLocalTimeConfiguration Config;
40-45: Use AddSingleton for instance-backed registrations and bind the exact configReturning the same instance from a scoped factory is unusual; register it as a singleton. Also prefer AddSingleton over TryAddSingleton for the config to avoid mismatches when another config was added earlier.
- services.AddScoped<ILocalTimeService>(_ => instance); - services.TryAddSingleton<BlazorLocalTimeConfiguration>(_ => instance.Config); + services.AddSingleton<ILocalTimeService>(instance); + services.AddSingleton<BlazorLocalTimeConfiguration>(_ => instance.Config);
47-54: Ensure deterministic config resolutionWhen passing a MockTimeProvider, prefer AddSingleton to guarantee this config is used even if prior registrations exist.
- services.AddScoped<ILocalTimeService, LocalTimeMockService>(); - services.TryAddSingleton<BlazorLocalTimeConfiguration>(_ => new() { + services.AddScoped<ILocalTimeService, LocalTimeMockService>(); + services.AddSingleton<BlazorLocalTimeConfiguration>(_ => new() { TimeProvider = instance });
40-45: Optional: add an overload that accepts ActionFor flexibility in tests (e.g., setting IanaToWindows), consider an overload mirroring the production extension to configure the mock service via an action.
I can draft it if helpful.
Also applies to: 47-54
src/BlazorLocalTime/LocalTimeService.cs (4)
1-1: Remove unused using
System.Diagnostics.CodeAnalysisisn’t used in this file.-using System.Diagnostics.CodeAnalysis; +
8-8: Guard against null configuration in primary constructorDefensive null-check avoids hard-to-trace NREs from DI misconfiguration.
-internal class LocalTimeService(BlazorLocalTimeConfiguration configuration) : ILocalTimeService +internal class LocalTimeService(BlazorLocalTimeConfiguration configuration) : ILocalTimeService +{ + public LocalTimeService : this(configuration ?? throw new ArgumentNullException(nameof(configuration))) {} +}Alternatively, switch to a traditional ctor and throw on null.
19-38: Avoid raising change event when the effective time zone didn’t changeIf override switches to a value equal to the current effective zone, we still fire an event. Short-circuit to reduce redundant renders.
var previousTimeZone = TimeZoneInfo; _overrideTimeZoneInfo = value; var currentTimeZone = TimeZoneInfo; - // Fire both events for backward compatibility - LocalTimeZoneChanged.Invoke(this, new(previousTimeZone, currentTimeZone)); + // Raise event only when the effective time zone actually changed + if (!(previousTimeZone is null ? currentTimeZone is null : previousTimeZone.Equals(currentTimeZone))) + { + LocalTimeZoneChanged.Invoke(this, new(previousTimeZone, currentTimeZone)); + }Also, the comment about “both events” is stale—only one event exists now.
52-64: Avoid event spam when there’s no effective change; handle null==null earlyWhen setting the same browser TZ or when both are null, we still raise. Also if an override is in effect, changing the browser TZ might not change the effective TZ.
public void SetBrowserTimeZoneInfo(TimeZoneInfo? timeZoneInfo) { - if (BrowserTimeZoneInfo != null && BrowserTimeZoneInfo.Equals(timeZoneInfo)) + if ((BrowserTimeZoneInfo is null && timeZoneInfo is null) || + (BrowserTimeZoneInfo is not null && BrowserTimeZoneInfo.Equals(timeZoneInfo))) { return; } var previousTimeZone = TimeZoneInfo; BrowserTimeZoneInfo = timeZoneInfo; var currentTimeZone = TimeZoneInfo; - // Fire both events for backward compatibility - LocalTimeZoneChanged.Invoke(this, new(previousTimeZone, currentTimeZone)); + // Raise event only when the effective time zone actually changed + if (!(previousTimeZone is null ? currentTimeZone is null : previousTimeZone.Equals(currentTimeZone))) + { + LocalTimeZoneChanged.Invoke(this, new(previousTimeZone, currentTimeZone)); + } }src/BlazorLocalTime/ILocalTimeService.cs (4)
48-53: Limit external mutation surfaceThese members are
internal, which is good. Consider documenting that consumers should not call them directly; all updates should flow via the provider.
79-92: Prefer BCL conversions to construct DateTimeOffsetUse
TimeZoneInfo.ConvertTimeoverloads to avoid manual pairing and edge cases around DST/ambiguous times.- public DateTimeOffset ToLocalTimeOffset(DateTime utcDateTime) - { - return new(ToLocalTime(utcDateTime), GetBrowserTimeZone().GetUtcOffset(utcDateTime)); - } + public DateTimeOffset ToLocalTimeOffset(DateTime utcDateTime) + { + var tz = GetBrowserTimeZone(); + return TimeZoneInfo.ConvertTime(new DateTimeOffset(utcDateTime, TimeSpan.Zero), tz); + } @@ - public DateTimeOffset ToLocalTimeOffset(DateTimeOffset dateTimeOffset) - { - return new(ToLocalTime(dateTimeOffset), GetBrowserTimeZone().GetUtcOffset(dateTimeOffset)); - } + public DateTimeOffset ToLocalTimeOffset(DateTimeOffset dateTimeOffset) + { + return TimeZoneInfo.ConvertTime(dateTimeOffset, GetBrowserTimeZone()); + }
95-116: Clarify method purpose (name vs behavior mismatch)
GetBrowserTimeZonereturns the effective TZ (override-aware), not strictly the browser TZ. Update docs to prevent misuse; consider a future alias for clarity.- /// Gets the browser's time zone information. + /// Gets the current effective time zone (override takes precedence over the browser zone). @@ - if (!IsTimeZoneInfoAvailable) + if (!IsTimeZoneInfoAvailable) { throw new InvalidOperationException( """ - Failed to obtain the browser's time zone information. + Failed to obtain local time zone information. Possible causes: 1) The `<BlazorLocalTimeProvider />` component has not been added. In this case, please add `<BlazorLocalTimeProvider />` to a root component such as `Routes.razor`. 2) You are trying to use `ILocalTimeService` in `OnInitialized(Async)`. - In this case, you need to subscribe to the `ILocalTimeService.OnLocalTimeZoneChanged` event + In this case, subscribe to the `ILocalTimeService.LocalTimeZoneChanged` event and perform processing after the time zone information has been set. """ ); } return TimeZoneInfo;Optionally, introduce a non-breaking alias:
- Add
TimeZoneInfo GetLocalTimeZone()that delegates toGetBrowserTimeZone(), and update docs to recommend it going forward.
46-47: Naming nit: IsSuccessLoadBrowserTimeZoneConsider
IsBrowserTimeZoneLoadedSuccessfullyorIsBrowserTimeZoneLoadedfor readability. Since it’sinternal, this is low-risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
BlazorLocalTime.sln(2 hunks)src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs(1 hunks)src/BlazorLocalTime/BlazorLocalTimeExtension.cs(1 hunks)src/BlazorLocalTime/Components/BlazorLocalTimeProvider.razor.cs(4 hunks)src/BlazorLocalTime/ILocalTimeService.cs(1 hunks)src/BlazorLocalTime/LocalTimeService.cs(2 hunks)src/Directory.Build.props(1 hunks)tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeProviderTest.razor(1 hunks)tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeTest.Nls.csproj(1 hunks)tests/BlazorLocalTimeTest.Nls/_Imports.razor(1 hunks)tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net8.approved.txt(1 hunks)tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net9.approved.txt(1 hunks)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderTest.razor(1 hunks)tests/BlazorLocalTimeTest/BlazorLocalTimeTest.csproj(0 hunks)tests/BlazorLocalTimeTest/LocalTimeChangeEventTest.razor(1 hunks)tests/BlazorLocalTimeTest/LocalTimeFormDefaultTest.razor(1 hunks)tests/BlazorLocalTimeTest/LocalTimeFormTest.razor(2 hunks)tests/BlazorLocalTimeTest/LocalTimeMockService.cs(2 hunks)tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs(2 hunks)tests/BlazorLocalTimeTest/LocalTimeTest.razor(1 hunks)tests/BlazorLocalTimeTest/LocalTimeTotalTest.razor(4 hunks)tests/BlazorLocalTimeTest/TestInitializer.cs(1 hunks)tests/Directory.Build.props(1 hunks)
💤 Files with no reviewable changes (1)
- tests/BlazorLocalTimeTest/BlazorLocalTimeTest.csproj
🧰 Additional context used
🧬 Code graph analysis (18)
tests/BlazorLocalTimeTest.Nls/_Imports.razor (4)
tests/BlazorLocalTimeTest/PublicApiCheckTest.cs (3)
PublicApiCheckTest(8-30)Fact(10-22)c(17-21)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (3)
BlazorLocalTimeProviderCodeTest(10-70)BlazorLocalTimeProviderCodeTest(12-16)Fact(18-29)src/BlazorLocalTime/LocalTimeZoneOverwrite.cs (1)
LocalTimeZoneOverwrite(18-125)src/BlazorLocalTime/Components/LocalTimeText.razor.cs (1)
LocalTimeText(8-59)
tests/Directory.Build.props (1)
tests/BlazorLocalTimeTest/PublicApiCheckTest.cs (3)
c(17-21)PublicApiCheckTest(8-30)Fact(10-22)
tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeProviderTest.razor (1)
tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (6)
BlazorLocalTimeProviderCodeTest(10-70)Fact(46-59)Fact(31-44)Fact(18-29)BlazorLocalTimeProviderCodeTest(12-16)Fact(61-69)
src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs (4)
src/BlazorLocalTime/Components/LocalTimeZone.razor.cs (1)
LocalTimeZone(8-47)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (2)
BlazorLocalTimeProviderCodeTest(12-16)BlazorLocalTimeProviderCodeTest(10-70)src/BlazorLocalTime/Components/LocalTimeText.razor.cs (1)
LocalTimeText(8-59)src/BlazorLocalTime/Components/LocalTime.razor.cs (1)
LocalTime(8-55)
tests/BlazorLocalTimeTest/LocalTimeFormDefaultTest.razor (2)
tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (1)
BlazorLocalTimeProviderCodeTest(12-16)tests/BlazorLocalTimeTest/MockTimeProvider.cs (1)
MockTimeProvider(3-6)
tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeTest.Nls.csproj (3)
tests/BlazorLocalTimeTest/PublicApiCheckTest.cs (3)
PublicApiCheckTest(8-30)c(17-21)Fact(10-22)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (2)
BlazorLocalTimeProviderCodeTest(10-70)BlazorLocalTimeProviderCodeTest(12-16)src/BlazorLocalTime/LocalTimeZoneOverwrite.cs (1)
LocalTimeZoneOverwrite(18-125)
tests/BlazorLocalTimeTest/TestInitializer.cs (1)
tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (4)
Fact(18-29)BlazorLocalTimeProviderCodeTest(10-70)Fact(31-44)Fact(46-59)
tests/BlazorLocalTimeTest/LocalTimeFormTest.razor (1)
tests/BlazorLocalTimeTest/MockTimeProvider.cs (2)
MockTimeProvider(3-6)DateTimeOffset(5-5)
tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs (1)
src/BlazorLocalTime/LocalTimeService.cs (1)
LocalTimeService(8-65)
src/BlazorLocalTime/Components/BlazorLocalTimeProvider.razor.cs (4)
src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs (1)
BlazorLocalTimeConfiguration(6-20)src/BlazorLocalTime/Components/LocalTimeForm.razor.cs (4)
Task(168-195)Task(197-216)Task(218-238)Task(240-260)src/BlazorLocalTime/ILocalTimeService.cs (1)
TimeZoneInfo(98-115)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (1)
BlazorLocalTimeProviderCodeTest(10-70)
src/BlazorLocalTime/ILocalTimeService.cs (4)
src/BlazorLocalTime/LocalTimeService.cs (2)
TimeZoneChangedEventArgs(75-87)SetBrowserTimeZoneInfo(52-64)src/BlazorLocalTime/Components/LocalTimeZone.razor.cs (1)
LocalTimeZone(8-47)src/BlazorLocalTime/Components/LocalTime.razor.cs (1)
LocalTime(8-55)src/BlazorLocalTime/Components/LocalTimeText.razor.cs (1)
LocalTimeText(8-59)
src/BlazorLocalTime/BlazorLocalTimeExtension.cs (3)
tests/BlazorLocalTimeTest/LocalTimeMockService.cs (3)
IServiceCollection(31-38)IServiceCollection(40-45)IServiceCollection(47-54)src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs (1)
BlazorLocalTimeConfiguration(6-20)src/BlazorLocalTime/LocalTimeService.cs (1)
LocalTimeService(8-65)
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net8.approved.txt (2)
tests/BlazorLocalTimeTest/PublicApiCheckTest.cs (2)
PublicApiCheckTest(8-30)Fact(10-22)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (2)
BlazorLocalTimeProviderCodeTest(10-70)BlazorLocalTimeProviderCodeTest(12-16)
tests/BlazorLocalTimeTest/LocalTimeTotalTest.razor (1)
tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (4)
BlazorLocalTimeProviderCodeTest(10-70)BlazorLocalTimeProviderCodeTest(12-16)Fact(61-69)Fact(18-29)
tests/BlazorLocalTimeTest/LocalTimeMockService.cs (4)
src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs (1)
BlazorLocalTimeConfiguration(6-20)src/BlazorLocalTime/ILocalTimeService.cs (1)
TimeZoneInfo(98-115)src/BlazorLocalTime/BlazorLocalTimeExtension.cs (3)
IServiceCollection(18-21)IServiceCollection(29-41)IServiceCollection(49-61)tests/BlazorLocalTimeTest/MockTimeProvider.cs (1)
MockTimeProvider(3-6)
tests/BlazorLocalTimeTest/BlazorLocalTimeProviderTest.razor (1)
tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (2)
BlazorLocalTimeProviderCodeTest(10-70)Fact(18-29)
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net9.approved.txt (2)
tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (2)
BlazorLocalTimeProviderCodeTest(10-70)BlazorLocalTimeProviderCodeTest(12-16)tests/BlazorLocalTimeTest/PublicApiCheckTest.cs (1)
PublicApiCheckTest(8-30)
src/BlazorLocalTime/LocalTimeService.cs (5)
src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs (1)
BlazorLocalTimeConfiguration(6-20)src/BlazorLocalTime/Components/LocalTimeZone.razor.cs (1)
LocalTimeZone(8-47)src/BlazorLocalTime/Components/LocalTime.razor.cs (1)
LocalTime(8-55)src/BlazorLocalTime/Components/LocalTimeForm.razor.cs (1)
OnLocalTimeZoneChangedDetailed(56-91)src/BlazorLocalTime/Components/LocalTimeText.razor.cs (1)
LocalTimeText(8-59)
🪛 GitHub Actions: .NET Build and Test
tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeProviderTest.razor
[error] 12-12: Shouldly.ShouldAssertException: '// cannot convert timezone from IANA to Windows Render( @ );' should throw System.TimeZoneNotFoundException but did not. Stack Trace: /home/runner/work/BlazorLocalTime/BlazorLocalTime/tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeProviderTest.razor(12,0)
🔇 Additional comments (18)
tests/Directory.Build.props (1)
11-11: Isolate bUnit package as test-only
- Apply:
- <PackageReference Include="bunit" Version="1.40.0" /> + <PackageReference Include="bunit" Version="1.40.0"> + <PrivateAssets>all</PrivateAssets> + </PackageReference>
- Compatibility verified: bUnit 1.40.0 targets net9.0 and works with .NET 9 and Microsoft.NET.Test.Sdk 17.14.1; xunit.runner.visualstudio 3.1.3 supports xUnit 2.x.
- Optionally pin exact versions in Directory.Packages.props for reproducible builds; mirror for other test-only dependencies.
tests/BlazorLocalTimeTest/TestInitializer.cs (1)
20-24: Making JavaScriptInitializer public is appropriate for cross-project reuseVisibility increase aligns with its usage from tests; no concerns.
src/Directory.Build.props (1)
39-41: InternalsVisibleTo entry verified; assembly name matches
- AssemblyName matches the
InternalsVisibleToentry.- For future strong-naming, include the public key token in this attribute.
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net9.approved.txt (1)
3-12: Approved baseline updates look correct
- InternalsVisibleTo includes Nls tests.
- New public BlazorLocalTimeConfiguration with expected properties is captured.
tests/BlazorLocalTimeTest/LocalTimeFormDefaultTest.razor (1)
12-12: Good move: centralize mock wiring via AddLocalTimeMockService.This keeps tests consistent with the new configuration-based DI and ensures the MockTimeProvider flows into BlazorLocalTimeConfiguration.
tests/BlazorLocalTimeTest.Nls/_Imports.razor (1)
1-8: Usings set looks right for bUnit + DI/JS interop.Nothing to change.
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net8.approved.txt (1)
3-3: Public API baselines updated for .NET 8 and .NET 9tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs (1)
11-11: Target-typednew()for configuration is clean and matches ctor expectations.Nice alignment with the new LocalTimeService(BlazorLocalTimeConfiguration) signature.
Also applies to: 19-19, 31-31
tests/BlazorLocalTimeTest/LocalTimeFormTest.razor (2)
97-99: Good: deterministic time with explicit MockTimeProvider.This makes the test stable and intention-revealing.
120-122: Good: consistent mock injection for time-only path.Keeps both date-only and time-only tests aligned.
BlazorLocalTime.sln (1)
12-19: LGTM: adds NLS test project and solution folders.No concerns from solution metadata changes.
tests/BlazorLocalTimeTest/LocalTimeChangeEventTest.razor (1)
10-10: Good migration to config-driven mocksUsing LocalTimeEmptyMockService with AddLocalTimeMockService aligns with the new API and keeps tests focused. LGTM.
Also applies to: 14-14
tests/BlazorLocalTimeTest/BlazorLocalTimeProviderTest.razor (1)
8-9: Config-based mock wiring looks correctConstructing LocalTimeEmptyMockService with TimeProvider.System and registering via AddLocalTimeMockService is consistent with the new DI surface. LGTM.
tests/BlazorLocalTimeTest/LocalTimeMockService.cs (3)
20-26: LGTMEmpty mock correctly nulls BrowserTimeZoneInfo, letting the provider populate it via JS.
34-36: Default mock registration OKTryAddSingleton with a default config is fine here since this helper is usually called first in tests.
9-17: Ignore redundant TimeProvider singleton suggestion: a search across the codebase shows no otherAddSingleton<TimeProvider>(…)registrations, so there are no duplicates to remove.Likely an incorrect or invalid review comment.
src/BlazorLocalTime/LocalTimeService.cs (1)
45-46: Now may throw before TZ is available
NowcallsToLocalTimeOffset(...)which throws until the time zone is set. Ensure call sites checkIsTimeZoneInfoAvailableor handle exceptions; consider documenting this explicitly in XML docs on the interface.src/BlazorLocalTime/ILocalTimeService.cs (1)
35-40: Good use of MemberNotNullWhen on availability flagNice usability improvement for callers and analyzers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeTest.Nls.csproj (1)
13-14: Gate UseNls to Windows to avoid false behavior on Linux/macOS.Apply the Windows-only condition so the NLS runtime switch is emitted only on Windows builds.
- <ItemGroup> - <RuntimeHostConfigurationOption Include="System.Globalization.UseNls" Value="true" /> - </ItemGroup> + <ItemGroup Condition="'$(OS)'=='Windows_NT'"> + <RuntimeHostConfigurationOption Include="System.Globalization.UseNls" Value="true" /> + </ItemGroup>Run to confirm Windows-only test gating or skips exist:
#!/bin/bash # Verify NLS tests are Windows-gated or skippable rg -nC2 -g 'tests/BlazorLocalTimeTest.Nls/**' -P '(SkippableFact|Skip\.If|OperatingSystem\.IsWindows|RuntimeInformation\.IsOSPlatform)'
🧹 Nitpick comments (4)
tests/BlazorLocalTimeTest/LocalTimeMockService.cs (4)
9-9: Make Config immutableThis is assigned once; mark it readonly to prevent accidental mutation in tests.
-internal BlazorLocalTimeConfiguration Config; +internal readonly BlazorLocalTimeConfiguration Config;
34-38: Deterministic override of test configurationTryAddSingleton won’t replace an already-registered config (e.g., if AddBlazorLocalTimeService was called earlier), leading to surprising behavior. Replace to guarantee the mock’s config wins in tests.
- services.TryAddSingleton<BlazorLocalTimeConfiguration>(_ => - new() { TimeProvider = TimeProvider.System } - ); + services.Replace( + ServiceDescriptor.Singleton<BlazorLocalTimeConfiguration>( + new BlazorLocalTimeConfiguration { TimeProvider = TimeProvider.System } + ) + );
50-58: Minor: clarify parameter name and consolidate registrationUse a descriptive name and consider delegating to the instance-based overload to reduce duplication.
- public static IServiceCollection AddLocalTimeMockService( - this IServiceCollection services, - MockTimeProvider instance - ) + public static IServiceCollection AddLocalTimeMockService( + this IServiceCollection services, + MockTimeProvider timeProvider + ) { - services.AddScoped<ILocalTimeService, LocalTimeMockService>(); - services.TryAddSingleton<BlazorLocalTimeConfiguration>(_ => - new() { TimeProvider = instance } - ); - return services; + return services.AddLocalTimeMockService( + new LocalTimeMockService( + new BlazorLocalTimeConfiguration { TimeProvider = timeProvider } + ) + ); }
40-48: Use Singleton registration for externally created instanceRegistering a prebuilt instance as Scoped can cause lifetime and disposal quirks; switch to Singleton:
- services.AddScoped<ILocalTimeService>(_ => instance); - services.TryAddSingleton<BlazorLocalTimeConfiguration>(_ => instance.Config); + services.AddSingleton<ILocalTimeService>(instance); + services.Replace( + ServiceDescriptor.Singleton<BlazorLocalTimeConfiguration>(instance.Config) + );LocalTimeService does not implement IDisposable or IAsyncDisposable, so Singleton disposal is safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.github/workflows/test.yaml(1 hunks)src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs(1 hunks)src/BlazorLocalTime/BlazorLocalTimeExtension.cs(1 hunks)src/BlazorLocalTime/Components/BlazorLocalTimeProvider.razor.cs(4 hunks)tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeProviderTest.razor(1 hunks)tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeTest.Nls.csproj(1 hunks)tests/BlazorLocalTimeTest/LocalTimeMockService.cs(2 hunks)tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeProviderTest.razor
- src/BlazorLocalTime/BlazorLocalTimeExtension.cs
- tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs
- src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs
🧰 Additional context used
🧬 Code graph analysis (3)
tests/BlazorLocalTimeTest.Nls/BlazorLocalTimeTest.Nls.csproj (3)
tests/BlazorLocalTimeTest/PublicApiCheckTest.cs (3)
PublicApiCheckTest(8-30)c(17-21)Fact(10-22)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (2)
BlazorLocalTimeProviderCodeTest(10-70)BlazorLocalTimeProviderCodeTest(12-16)src/BlazorLocalTime/LocalTimeZoneOverwrite.cs (1)
LocalTimeZoneOverwrite(18-125)
tests/BlazorLocalTimeTest/LocalTimeMockService.cs (4)
src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs (1)
BlazorLocalTimeConfiguration(6-20)src/BlazorLocalTime/BlazorLocalTimeExtension.cs (3)
IServiceCollection(18-21)IServiceCollection(29-41)IServiceCollection(49-62)tests/BlazorLocalTimeTest/MockTimeProvider.cs (1)
MockTimeProvider(3-6)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (2)
BlazorLocalTimeProviderCodeTest(10-70)BlazorLocalTimeProviderCodeTest(12-16)
src/BlazorLocalTime/Components/BlazorLocalTimeProvider.razor.cs (3)
src/BlazorLocalTime/BlazorLocalTimeConfiguration.cs (1)
BlazorLocalTimeConfiguration(6-20)src/BlazorLocalTime/LocalTimeService.cs (2)
ILocalTimeService(8-116)LocalTimeService(121-178)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (1)
BlazorLocalTimeProviderCodeTest(10-70)
🔇 Additional comments (3)
.github/workflows/test.yaml (1)
14-17: Matrix build LGTM.Multi-OS coverage is appropriate for ICU/NLS differences. No changes requested.
src/BlazorLocalTime/Components/BlazorLocalTimeProvider.razor.cs (1)
81-88: ICU detection approach LGTM.Matches Microsoft’s recommended detection pattern for ICU usage. (learn.microsoft.com)
tests/BlazorLocalTimeTest/LocalTimeMockService.cs (1)
20-27: LGTM: empty mock behavior is clearNulling BrowserTimeZoneInfo is appropriate for “no browser TZ” scenarios.
1 participant
This pull request introduces support for environments where ICU (International Components for Unicode) is unavailable, such as Windows Server 2016, by allowing conversion from IANA to Windows time zones through a configurable converter. It also refactors service registration to use a new configuration class, improves dependency injection, and adds targeted tests for NLS mode. The changes enhance flexibility and reliability of time zone detection and conversion in Blazor applications.
close #19
Summary by CodeRabbit
New Features
Tests
Chores