Add nullability annotations and simplify time zone logic#17
Conversation
…etBrowserTimeZone logic
WalkthroughAdds member-nullability attributes to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Consumer
participant S as ILocalTimeService
C->>S: IsTimeZoneInfoAvailable()
alt true
Note over S,C #DFF2E1: MemberNotNullWhen(true) -> TimeZoneInfo non-null
C->>S: GetBrowserTimeZone()
S-->>C: return TimeZoneInfo
else false
C->>S: GetBrowserTimeZone()
S-->>C: throw (unavailable)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/BlazorLocalTime/LocalTimeService.cs (1)
104-111: Typo/mismatch in the exception message: component is BlazorLocalTimeProvider, not BrowserLocalTimeProvider.This is user-facing and can mislead consumers following the guidance.
- 1) The `<BrowserLocalTimeProvider />` component has not been added. - In this case, please add `<BrowserLocalTimeProvider />` to a root component such as `Routes.razor`. + 1) The `<BlazorLocalTimeProvider />` component has not been added. + In this case, please add `<BlazorLocalTimeProvider />` to a root component such as `Routes.razor`.
🧹 Nitpick comments (2)
src/BlazorLocalTime/LocalTimeService.cs (2)
38-39: Good use of MemberNotNullWhen to establish the nullability contract.Minor style nit: prefer pattern matching for readability.
- public bool IsTimeZoneInfoAvailable => TimeZoneInfo != null; + public bool IsTimeZoneInfoAvailable => TimeZoneInfo is not null;
100-115: Guard simplification is sound; optionally capture the value to avoid double-evaluation/race window.Today you compute TimeZoneInfo twice (in the property and again when returning). Capturing once is a tiny optimization and closes a theoretical race between check and use if the value changes mid-call.
- if (!IsTimeZoneInfoAvailable) + var tz = TimeZoneInfo; + if (tz is null) { throw new InvalidOperationException( """ Failed to obtain the browser's time zone information. Possible causes: - 1) The `<BrowserLocalTimeProvider />` component has not been added. + 1) The `<BlazorLocalTimeProvider />` component has not been added. In this case, please add `<BrowserLocalTimeProvider />` 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 and perform processing after the time zone information has been set. """ ); } - return TimeZoneInfo; + return tz;
📜 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 (1)
src/BlazorLocalTime/LocalTimeService.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/BlazorLocalTime/LocalTimeService.cs (4)
src/BlazorLocalTime/Components/LocalTimeText.razor.cs (1)
LocalTimeText(8-59)tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs (2)
LocalTimeServiceTest(6-42)Fact(8-15)src/BlazorLocalTime/Components/LocalTime.razor.cs (1)
LocalTime(8-55)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (1)
BlazorLocalTimeProviderCodeTest(10-66)
🔇 Additional comments (1)
src/BlazorLocalTime/LocalTimeService.cs (1)
1-1: Supported TFMs and LangVersion Confirmed – No Action NeededAll projects target .NET 6.0 and above with
LangVersionset tolatest, which include the built-inMemberNotNullWhenAttribute. No polyfill or additional package is required.•
src/BlazorLocalTime/BlazorLocalTime.csproj: net6.0;net7.0;net8.0;net9.0
•tests/BlazorLocalTimeTest/BlazorLocalTimeTest.csproj: net8.0;net9.0
•example/BlazorLocalTimeSample/BlazorLocalTimeSample.csproj: net9.0
• CentralDirectory.Build.props:<LangVersion>latest</LangVersion>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net9.approved.txt (1)
15-16: Is duplicating the attribute on both property and getter required?Some analyzers/consumers only honor the accessor-level attribute, but others pick up the property-level one. If you don’t have a concrete need for both, consider simplifying to just the getter target to reduce API noise. If you’ve observed analyzer gaps, keep both as-is.
Proposed change in src/BlazorLocalTime/LocalTimeService.cs:
- [MemberNotNullWhen(true, nameof(TimeZoneInfo))] - [get: MemberNotNullWhen(true, nameof(TimeZoneInfo))] + [get: MemberNotNullWhen(true, nameof(TimeZoneInfo))] bool IsTimeZoneInfoAvailable { get; }tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net8.approved.txt (1)
15-16: Consider trimming to only the accessor attribute unless both are neededIf analyzer/tooling coverage is satisfied with just the getter attribute, you can remove the property-level attribute to minimize public API churn. Keep both if you rely on property-level consumption in any downstream tooling.
Suggested change in src/BlazorLocalTime/LocalTimeService.cs:
- [MemberNotNullWhen(true, nameof(TimeZoneInfo))] - [get: MemberNotNullWhen(true, nameof(TimeZoneInfo))] + [get: MemberNotNullWhen(true, nameof(TimeZoneInfo))] bool IsTimeZoneInfoAvailable { get; }
📜 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 (3)
src/BlazorLocalTime/LocalTimeService.cs(3 hunks)tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net8.approved.txt(1 hunks)tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net9.approved.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/BlazorLocalTime/LocalTimeService.cs
🧰 Additional context used
🧬 Code graph analysis (2)
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net8.approved.txt (3)
tests/BlazorLocalTimeTest/PublicApiCheckTest.cs (2)
PublicApiCheckTest(8-30)Fact(10-22)tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs (1)
Fact(8-15)tests/BlazorLocalTimeTest/BlazorLocalTimeProviderCodeTest.cs (1)
Fact(57-65)
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net9.approved.txt (2)
tests/BlazorLocalTimeTest/PublicApiCheckTest.cs (2)
PublicApiCheckTest(8-30)Fact(10-22)tests/BlazorLocalTimeTest/LocalTimeServiceTest.cs (1)
Fact(8-15)
🪛 LanguageTool
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net9.approved.txt
[grammar] ~1-~1: There might be a mistake here.
Context: .../github.com/arika0093/BlazorLocalTime")] [assembly: System.Runtime.CompilerServic...
(QB_NEW_EN)
🔇 Additional comments (4)
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net9.approved.txt (2)
15-16: Good addition: precise nullability contract on IsTimeZoneInfoAvailableAnnotating IsTimeZoneInfoAvailable with MemberNotNullWhen(true, "TimeZoneInfo") (and mirroring it on the getter) clearly communicates the flow contract to the compiler and consumers. This is a source- and binary-compatible public API enhancement.
15-16: Don’t extendMemberNotNullWhento BrowserTimeZoneInfoThe current implementation of
IsTimeZoneInfoAvailableguarantees thatTimeZoneInfois non-null when true, but does not guarantee the same forBrowserTimeZoneInfo. In particular:
TimeZoneInforeturnsOverrideTimeZoneInfoif set, or elseBrowserTimeZoneInfo.- When a user override is applied (
OverrideTimeZoneInfo != null),IsTimeZoneInfoAvailablebecomes true even thoughBrowserTimeZoneInfomay still be null.- Applying
[MemberNotNullWhen(true, nameof(BrowserTimeZoneInfo))]would be incorrect and could lead to false assumptions.Consumers should continue to rely on the non-nullable
TimeZoneInfoproperty whenIsTimeZoneInfoAvailableis true.Likely an incorrect or invalid review comment.
tests/BlazorLocalTimeTest/Approvals/PublicApiCheckTest.Run.net8.approved.txt (2)
15-16: Nullability attribute on IsTimeZoneInfoAvailable looks correct and is compat-safeCarrying the MemberNotNullWhen(true, "TimeZoneInfo") contract into the net8 public API is consistent with the implementation intent and remains a non-breaking, compiler-aided improvement for consumers on .NET 8.
15-16: Ignore BrowserTimeZoneInfo nullability suggestionThe contract of IsTimeZoneInfoAvailable only guarantees that TimeZoneInfo (defined as OverrideTimeZoneInfo ?? BrowserTimeZoneInfo) is non-null. When an override is set, IsTimeZoneInfoAvailable can be true while BrowserTimeZoneInfo remains null, so adding MemberNotNullWhen(true, nameof(BrowserTimeZoneInfo)) would be incorrect.
Likely an incorrect or invalid review comment.
1 participant
Introduce nullability annotations for
IsTimeZoneInfoAvailableand streamline the logic inGetBrowserTimeZoneto enhance code clarity and safety.Summary by CodeRabbit
Refactor
Chores
Bug Fixes / UX