Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR removes unnecessary blank lines from two Android source files and adds a new iOS test case to verify delay update cancellation behavior with killed conditions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
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
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ios/Sources/CapacitorUpdaterPlugin/DelayUpdateUtils.swift (1)
153-163:do/catcharound non-throwing code —catchblock is unreachable.
UserDefaults.standard.set(...)andsynchronize()don't throw. Thecatchblock on Line 159 will never execute. This same pattern repeats insetBackgroundTimestamp,unsetBackgroundTimestamp,getBackgroundTimestamp, andcancelDelay.Not blocking since this is pre-existing, but worth cleaning up to avoid misleading readers into thinking these operations can fail with a thrown error.
There was a problem hiding this comment.
Pull request overview
This PR aligns multi-delay date parsing behavior across Android and iOS (supporting more ISO-8601 variants), removes the unintended setDelay exposure from iOS/web wiring, and expands native test coverage around multi-delay persistence and filtering.
Changes:
- Add dedicated date parsing helpers on iOS/Android to accept ISO-8601 strings with/without fractional seconds and with/without timezone.
- Remove
setDelayfrom iOS bridged method registration and the web implementation stub. - Add iOS unit tests for
setMultiDelaypersistence and kill-condition filtering; add Android test coverage for ISO dates without milliseconds.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/web.ts | Removes the web setDelay stub and associated type import. |
| ios/Tests/CapacitorUpdaterPluginTests/CapacitorUpdaterPluginTests.swift | Adds iOS unit tests for multi-delay persistence and kill filtering (and imports Version). |
| ios/Sources/CapacitorUpdaterPlugin/DelayUpdateUtils.swift | Introduces a helper to parse additional ISO-8601 date variants for delay conditions. |
| ios/Sources/CapacitorUpdaterPlugin/CapacitorUpdaterPlugin.swift | Removes unintended setDelay exposure from plugin method registration. |
| android/src/test/java/ee/forgr/capacitor_updater/DelayUpdateUtilsTest.java | Adds a regression test for ISO-8601 dates without milliseconds. |
| android/src/main/java/ee/forgr/capacitor_updater/DelayUpdateUtils.java | Adds parseDateCondition to support ISO-8601 variants across timezones and fractional seconds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,6 @@ | |||
| import XCTest | |||
| @testable import CapacitorUpdaterPlugin | |||
| import Version | |||
There was a problem hiding this comment.
import Version in this test target will likely fail to compile under SwiftPM because CapacitorUpdaterPluginTests only declares a dependency on CapacitorUpdaterPlugin in Package.swift (SwiftPM targets can only import direct dependencies, not transitive ones). Add the Version product to the test target dependencies (or avoid importing it by constructing the needed value via the plugin module).
| import Version |
| // Legacy fallback for strings without timezone. | ||
| for format in ["yyyy-MM-dd'T'HH:mm:ss.SSS", "yyyy-MM-dd'T'HH:mm:ss"] { | ||
| let formatter = DateFormatter() | ||
| formatter.locale = Locale(identifier: "en_US_POSIX") | ||
| formatter.dateFormat = format | ||
| if let date = formatter.date(from: value) { | ||
| return date | ||
| } |
There was a problem hiding this comment.
In the legacy timezone-less fallback, DateFormatter is created with a fixed dateFormat but no explicit calendar/timeZone. For users with a non-Gregorian current calendar (or unexpected timezone defaults), this can parse into an unintended absolute Date, causing delay conditions to be kept/removed incorrectly. Set formatter.calendar = Calendar(identifier: .gregorian) and formatter.timeZone = .current (or whatever “legacy” behavior is intended) to make parsing deterministic.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1291cc8bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sdf.setTimeZone(TimeZone.getDefault()); | ||
| } | ||
|
|
||
| Date parsed = sdf.parse(value); |
There was a problem hiding this comment.
parseDateCondition falls back from timezone-aware patterns to timezone-less ones, but SimpleDateFormat.parse(String) here accepts trailing characters, so values that miss the XXX format (for example 2099-12-31T23:59:59+0000) can be parsed by the local-time fallback while silently discarding the offset suffix. That means date delays are evaluated in device local time instead of the requested offset on non-UTC devices, which can shift update timing by hours. Please require full-string consumption (e.g., via ParsePosition) or handle +HHMM/Z offset patterns before timezone-less fallbacks.
Useful? React with 👍 / 👎.
riderx
force-pushed
the
riderx/create-pr
branch
from
f1291cc to
ccba3f7
Compare
|
|
Rebased this branch onto the latest Resolution summary:
The branch is pushed and no longer behind |
What
setDelayexposure from iOS plugin method registration and the web stub.setMultiDelaypersistence and kill-condition filtering, and add Android coverage for ISO dates without milliseconds.Why
How
Testing
bun run test:iosbun run verify:androidbun run verify:iosbun run buildNot Tested
Summary by CodeRabbit
Style
Tests