fix: ParseInstallation saves timeZone field as empty string#1141
Open
AdrianCurtin wants to merge 4 commits into
Open
fix: ParseInstallation saves timeZone field as empty string#1141AdrianCurtin wants to merge 4 commits into
ParseInstallation saves timeZone field as empty string#1141AdrianCurtin wants to merge 4 commits into
Conversation
Improve local timezone name resolution by capturing DateTime.now() once to avoid DST race, preferring the OS-reported IANA zone when available, and falling back to matching a location whose current zone offset equals the local offset. This also fixes a bug where the old code compared incompatible types (int vs Duration) and could produce an empty string. Tests updated to initialize Parse/timezone data in setUpAll and to assert the installation's timeZone field exists, is non-empty, is either an IANA name or the OS-reported name, and (when matched via offset) has an offset equal to the system offset.
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR rewrites local timezone resolution to initialize timezone data once, capture the current moment once, prefer OS timezone names that match IANA locations, match zones by normalized offset when needed, and fall back to the OS name; tests are refactored and expanded to validate presence, non-emptiness, IANA/fallback validity, and offset correctness. ChangesTimezone Name Resolution and Test Suite
Sequence DiagramsequenceDiagram
participant ParseInstallation
participant TimezoneDB
participant OSClock
ParseInstallation->>OSClock: capture now = DateTime.now()
ParseInstallation->>TimezoneDB: ensure tz.initializeTimeZones() (once, guard _timeZonesInitialized)
ParseInstallation->>TimezoneDB: lookup OS timeZoneName as IANA location
alt found IANA location
TimezoneDB-->>ParseInstallation: return location (may include offset)
else not found
TimezoneDB-->>ParseInstallation: search zones by current offset (use _zoneOffsetMs)
end
ParseInstallation-->>OSClock: return resolved timeZoneName (IANA or OS name)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves how ParseInstallation derives and persists the local timeZone, and adds regression tests to prevent it from being stored as an empty string due to mismatched offset types in older timezone versions.
Changes:
- Refactors
_getNameLocalTimeZone()to prefer OS-reported IANA zone names and fixes offset matching acrosstimezoneversions. - Adds tests asserting the
timeZonefield exists, is non-empty, and is either an IANA zone or the OS-reported name. - Adds a regression test verifying offset-based matches correspond to the local offset.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/dart/lib/src/objects/parse_installation.dart | Fixes and hardens local timezone name detection (IANA preference + offset fallback). |
| packages/dart/test/parse_installation_test.dart | Adds regression/behavior tests validating non-empty and valid timezone persistence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+97
to
+100
| tz.initializeTimeZones(); | ||
| var locations = tz.timeZoneDatabase.locations; | ||
|
|
||
| Duration offset = DateTime.now().timeZoneOffset; | ||
| String name = ""; | ||
| // Capture once to avoid a DST-transition race between the two reads. | ||
| final DateTime now = DateTime.now(); |
Comment on lines
+61
to
+76
| tz.initializeTimeZones(); | ||
| final installation = await ParseInstallation.currentInstallation(); | ||
| final tzValue = installation.get<String>(keyTimeZone)!; | ||
|
|
||
| // act | ||
| final ParseInstallation installation = | ||
| await ParseInstallation.currentInstallation(); | ||
| final location = tz.timeZoneDatabase.locations[tzValue]; | ||
| if (location == null) { | ||
| // OS-reported, non-IANA fallback (Windows/Web). Nothing to verify. | ||
| return; | ||
| } | ||
|
|
||
| dynamic actualHasTimeZoneResult = installation.containsKey(keyTimeZone); | ||
| final dynamic zoneOffset = location.currentTimeZone.offset; | ||
| final int zoneOffsetMs = zoneOffset is Duration | ||
| ? zoneOffset.inMilliseconds | ||
| : zoneOffset as int; | ||
|
|
||
| // assert | ||
| expect(actualHasTimeZoneResult, true); | ||
| expect(zoneOffsetMs, equals(DateTime.now().timeZoneOffset.inMilliseconds)); |
|
|
||
| test('installation timeZone is an IANA name or the OS-reported name', | ||
| () async { | ||
| tz.initializeTimeZones(); |
|
|
||
| test('when timeZone is matched via offset, its offset equals the local offset', | ||
| () async { | ||
| tz.initializeTimeZones(); |
Comment on lines
+116
to
+119
| final dynamic zoneOffset = location.currentTimeZone.offset; | ||
| final int zoneOffsetMs = zoneOffset is Duration | ||
| ? zoneOffset.inMilliseconds | ||
| : zoneOffset as int; |
AdrianCurtin
changed the title
ParseInstallation saves timeZone field as empty string
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dart/test/parse_installation_test.dart`:
- Around line 41-76: The tests read a persisted ParseInstallation (via
ParseInstallation.currentInstallation) and compare its keyTimeZone against
DateTime.now(), which can flake across DST changes; before each test recreate or
clear the persisted installation by removing the stored key
(keyParseStoreInstallation) or resetting the in-memory singleton/state (e.g.
_currentInstallationId) so currentInstallation() returns a fresh installation,
then set/get keyTimeZone for the new instance and run the assertions; update
test setup/tearDown to clear persistence and reset the internal current
installation state so comparisons to DateTime.now() are always against a
freshly-created installation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e016ac40-6fc7-4153-b599-077d22f68c56
📒 Files selected for processing (2)
packages/dart/lib/src/objects/parse_installation.dartpackages/dart/test/parse_installation_test.dart
Avoid repeatedly loading the timezone database by adding a process-wide _timeZonesInitialized flag and initializing tz data at most once in ParseInstallation. Add a helper _zoneOffsetMs to normalize TimeZone.offset (handles both int milliseconds on older timezone package versions and Duration on newer versions). Update tests to initialize timezones in setUpAll, clear persisted installation between tests to avoid stale data across DST boundaries, and capture DateTime.now() once per test to prevent racey comparisons.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dart/lib/src/objects/parse_installation.dart`:
- Around line 119-123: The fallback in _getNameLocalTimeZone currently returns
the first tz.timeZoneDatabase location whose
_zoneOffsetMs(location.currentTimeZone.offset) equals localOffsetMs, which can
pick an arbitrary IANA name when multiple zones share the same offset; change it
to collect all matching locations (using tz.timeZoneDatabase.locations.values
and _zoneOffsetMs) and only return the matching location.name if there is
exactly one match, otherwise return the original OS-reported system name
(now.timeZoneName) so we don't persist an arbitrary IANA zone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2281b152-1e9f-444f-828c-e1f2f8b9c389
📒 Files selected for processing (2)
packages/dart/lib/src/objects/parse_installation.dartpackages/dart/test/parse_installation_test.dart
Avoid overwriting a caller-provided timeZone on ParseInstallation. The change checks the existing value (via super.get) and only fills in the offset-matched local IANA name when the field is null or empty. Added comments explaining the offset-match fallback (which may pick an alphabetical IANA zone rather than a regionally accurate one), recommends apps set a real IANA name when available, and documents a first-launch caveat where the fallback can be persisted before caller code runs.
Change timezone fallback logic to only return an IANA zone if exactly one location matches the local offset. Many zones share the same instant offset (e.g. America/Los_Angeles and America/Vancouver), so returning the first match could fabricate a wrong location. The code now records a single match and clears it if a second match is found, falling back to the OS string when ambiguous. Also centralizes the local offset calculation and fixes comparisons against historical zone offsets.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Issue
N/A — bug surfaced during local testing; no existing issue.
Approach
ParseInstallation._getNameLocalTimeZone()returned""instead of an IANA timezone name (e.g.America/New_York), so installations were persisted with an emptytimeZonefield. This affects any server-side logic that targets users by locale, including push scheduling.Root cause:
DateTime.now().timeZoneOffset(aDuration) againstTimeZone.offsetfrom thetimezonepackage. Ontimezone <0.11.0that field is anint(ms), so the equality check is always false andnamestayed"". Ontimezone >=0.11.0both sides areDuration, but the loop scanned every historical zone for each location (LMT, pre-DST, etc.) and the outerforEachnever broke out, so whichever location came last in map iteration won.Fix:
DateTime.now()once to avoid a DST-transition race between the two reads.DateTime.now().timeZoneNamewhen it's a valid IANA location key (macOS, Linux, iOS, Android typically report IANA names directly).location.currentTimeZone(the current zone, not the full historical list) against the local offset. Handles bothintandDurationoffset shapes so the SDK stays compatible across the fulltimezone: ">=0.9.4 <0.12.0"constraint."". On Windows/Web this may be non-IANA ("Pacific Standard Time","EDT"), but it's strictly better than empty.Tests:
Expanded
parse_installation_test.dartfrom one weakcontainsKeyassertion (which passed even with the""bug) to four cases:timeZonefield existstimeZoneis non-emptytimeZoneis either an IANA name or the OS-reported fallbackTasks
Summary by CodeRabbit