Prevent retriggering of event actions #47
Conversation
WalkthroughThis pull request introduces several updates across various components of the project. A new rule is added to the SwiftLint configuration to disable Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.swiftlint.yml(1 hunks)example/ios/Tests/SharedTests.swift(1 hunks)ios/Shared.swift(1 hunks)src/ReactNativeDeviceActivity.types.ts(2 hunks)src/index.ts(1 hunks)targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Swift Test (example project)
- GitHub Check: Expo Bundle (example project)
🔇 Additional comments (15)
targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (7)
85-89: Looks solid.No particular issues in this newly added call; the usage of
self.executeActionsForEventis clear.
103-107: Usage is consistent.No issues with this event-based invocation; the method call aligns well with the overall flow.
138-142: Parameter extraction is clear.The skip-related parameters are well-defined and checked before being passed. Looks good.
144-183: Conditional logic seems appropriate.All skip conditions are funneled through
shouldExecuteAction, which simplifies the main loop.
185-189: No issues in this change.Invocation is consistent with other event callbacks.
203-208: Flows consistently.The approach mirrors the other event callback calls. No concerns.
223-228: Same pattern used effectively.All event triggers now use the new logic, ensuring consistent behavior.
example/ios/Tests/SharedTests.swift (3)
80-89: Well-structured test.This test covers boundary conditions (lower, equal, higher). Looks great.
91-99: Tests string-based event names thoroughly.Good coverage of prefixed string scenarios.
101-120: Exercises prefix replace logic clearly.The test ensures key variations are handled properly. Nicely done.
src/index.ts (1)
151-162: Verify date conversions for reliability.While converting from
Dateto timestamp is correct, confirm that these fields are always validDateobjects. Otherwise,.getTime()may fail at runtime if a non-Date value is passed.You could run a quick type check before calling
.getTime():skipIfLargerEventRecordedAfter: - action.skipIfLargerEventRecordedAfter - ? action.skipIfLargerEventRecordedAfter.getTime() - : undefined, + action.skipIfLargerEventRecordedAfter instanceof Date + ? action.skipIfLargerEventRecordedAfter.getTime() + : undefined,src/ReactNativeDeviceActivity.types.ts (1)
179-182: LGTM! Well-structured timing control properties.The new optional properties provide granular control over action execution:
skipIfAlreadyTriggeredAfter: Skip if already triggered after a specific dateskipIfLargerEventRecordedAfter: Skip if a larger event was recorded after a specific dateskipIfAlreadyTriggeredWithinMS: Skip if triggered within a specific time window in millisecondsskipIfLargerEventRecordedWithinMS: Skip if a larger event was recorded within a specific time window in millisecondsAlso applies to: 188-191, 197-200, 207-210, 217-220, 227-230, 241-244
ios/Shared.swift (2)
544-553: LGTM! Good extraction of key generation logic.The function follows the single responsibility principle and maintains consistent key formatting.
555-567: LGTM! Good synchronization of user defaults.The function correctly uses the extracted key generation logic and ensures changes are persisted immediately through
CFPreferencesAppSynchronize..swiftlint.yml (1)
11-11: LGTM! Necessary rule adjustment.Disabling
function_parameter_countis appropriate to accommodate the new functions with multiple parameters, such ashasHigherTriggeredEventandgetLastTriggeredTimeFromUserDefaultsinios/Shared.swift.
targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
CONTRIBUTING.md(1 hunks)config-plugin/getAppGroupFromExpoConfig.js(1 hunks)config-plugin/withCopyTargetFolder.js(1 hunks)example/ios/Tests/SharedTests.swift(1 hunks)example/package.json(1 hunks)ios/Shared.swift(1 hunks)package.json(1 hunks)src/index.ts(2 hunks)targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift(3 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[typographical] ~7-~7: Consider adding two commas here.
Context: ... - The entitlements and info.plist files however duplicated - to not mess with the examp...
(HOWEVER_COMMA)
[style] ~8-~8: Consider an alternative to strengthen your wording.
Context: ...to each target. I prefer this to making more changes in @bacons/xcode package which only sup...
(CHANGES_ADJUSTMENTS)
[typographical] ~9-~9: Consider adding a comma after this introductory phrase.
Context: ...ot level of each target directory. - In addition the example project contains an XCode t...
(IN_GENERAL_COMMA)
[uncategorized] ~13-~13: You might be missing the article “the” here.
Context: ... as SwiftLint. ## Prebuild To try out prebuild functionality (i.e. the config plugin) ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~13-~13: Did you mean to use the possessive pronoun “its”?
Context: ... expected, but the result should not in it's full state be commited to the example p...
(IT_S_ITS)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Swift Test (example project)
- GitHub Check: Expo Bundle (example project)
🔇 Additional comments (15)
config-plugin/getAppGroupFromExpoConfig.js (1)
12-12: LGTM!The environment variable has been updated to use
INTERNALLY_TEST_EXAMPLE_PROJECTwhich aligns with the changes in other configuration files.config-plugin/withCopyTargetFolder.js (1)
5-5: LGTM!The environment variable has been renamed to
COPY_TO_TARGET_FOLDERwhich better describes its purpose.targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (3)
54-95: Consider modularizing or clarifying responsibilities.
executeActionsForEventhandles multiple tasks—fetching, filtering, validating, and executing actions. For readability and testability, consider splitting out the user defaults retrieval, the filtering logic, and the action execution into smaller helper methods.
22-26: LGTM!The function calls to
executeActionsForEventhave been reformatted for better readability and consistency.Also applies to: 40-44, 103-107, 122-126, 140-144, 160-164
75-80:❓ Verification inconclusive
Verify milliseconds to seconds conversion in timing checks.
The code extracts timing parameters in milliseconds but may need to convert them to seconds when used with
Dateoperations. Please verify the conversion in theshouldExecuteActionfunction.
🏁 Script executed:
#!/bin/bash # Description: Search for time interval operations in the codebase rg -A 5 'addingTimeInterval|timeIntervalSince1970'Length of output: 2291
Action: Verify Timing Conversion Consistency in the
shouldExecuteActionFunctionThe extracted timing parameters appear to be in milliseconds, and our search confirms that elsewhere in the code (e.g., in
ios/Shared.swift) time values are converted by multiplyingDate().timeIntervalSince1970by 1000 for millisecond comparisons. Please double-check that theshouldExecuteActionfunction consistently applies this conversion—ensuring that comparisons against thresholds likeskipIfAlreadyTriggeredAfter,skipIfLargerEventRecordedAfter,skipIfAlreadyTriggeredWithinMS, andskipIfLargerEventRecordedWithinMSare made with all time values represented in milliseconds.src/index.ts (2)
136-138: LGTM!The function provides a clean wrapper around the module's method.
155-166: LGTM! Proper handling of Date objects.The function correctly transforms Date objects to timestamps before storing in user defaults, which is essential for proper serialization and comparison of dates.
ios/Shared.swift (6)
544-553: LGTM! Clean key generation logic.The function effectively generates user defaults keys with proper handling of optional event names.
555-567: LGTM! Proper timestamp handling and synchronization.The function correctly:
- Converts current time to milliseconds
- Uses the extracted key generation logic
- Ensures preferences are synchronized
569-575: Consider adding input validation.The function correctly handles both numeric and string comparisons, but could benefit from additional error handling.
577-583: Consider adding input validation.The function correctly handles prefix removal but could benefit from input validation.
585-616: Consider adding logging for better debugging.The function correctly handles event timing and retrieval but could benefit from additional logging.
693-706: LGTM! Clean implementation.The function effectively retrieves and type-casts the stored timestamp.
example/package.json (1)
6-11: LGTM! Proper environment variable configuration.The scripts correctly set the required environment variables for testing the example project.
package.json (1)
3-3: Version Bump Verified
The version field has been updated to"0.4.0", which aligns with the new event handling features introduced in this PR. Please ensure this version bump is also reflected in the changelog and release documentation.
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (1)
54-99: 🧹 Nitpick (assertive)Consider extracting config-key generation.
The logic building thekeystring is elegantly handled, but you might further unify or test that logic in a separate helper, especially if used elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
example/ios/Tests/SharedTests.swift(1 hunks)ios/ReactNativeDeviceActivityModule.swift(0 hunks)ios/Shared.swift(1 hunks)package.json(1 hunks)src/ReactNativeDeviceActivity.types.ts(2 hunks)targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift(3 hunks)
💤 Files with no reviewable changes (1)
- ios/ReactNativeDeviceActivityModule.swift
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Swift Test (example project)
- GitHub Check: Expo Bundle (example project)
🔇 Additional comments (15)
package.json (1)
3-3: Version Bump Reflects New Feature ChangesThe version has been updated from "0.3.1" to "0.4.1" to align with the new event retrigger prevention mechanisms introduced in this PR. This bump is appropriate to signal backwards-incompatible changes or new functionality to package consumers.
src/ReactNativeDeviceActivity.types.ts (6)
189-193: Repeat recommendation to provide doc comments.
These newly added properties would benefit from clear explanations to guide downstream usage.
199-203: Repeat recommendation to provide doc comments.
Remember to detail usage scenarios and valid ranges for the skip conditions.
210-214: Repeat recommendation to provide doc comments.
Including references or examples on how these properties interact with each other might be helpful.
221-225: Repeat recommendation to provide doc comments.
Ensure the structure and expected data types are clarified.
231-235: Repeat recommendation to provide doc comments.
Clear usage guidelines reduce the risk of misuse or confusion.
247-251: Repeat recommendation to provide doc comments.
Accurate inline descriptions help keep documentation in sync with the code.ios/Shared.swift (2)
555-565: Confirm user defaults’ suite availability at runtime.
IfuserDefaultsisnil(e.g., missingappGroup), this call will silently fail. Safely handlenilto avoid data loss.
618-712: Double-check time interval calculations in milliseconds.
The code correctly multipliestimeIntervalSince1970by 1000, but ensure all interplay between millisecond logic and second-based APIs (e.g.,Date) is consistent to avoid off-by-thousand errors.targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (6)
22-27: Well-structured call toexecuteActionsForEvent.
The new parametereventNameis set tonil, aligning with theintervalDidStartlogic. Looks consistent.
40-45: Consistent usage ofexecuteActionsForEvent.
Likewise, passingnilforeventNamematches theintervalDidEndbehavior.
107-112: Proper invocation with event-based key.
This ensures the correct action set is retrieved for threshold-based events.
126-131: Good consistency for warnings.
Again, ensuringeventNameisnilwhen no specific event is triggered.
144-149: Uniform approach to action execution.
Maintains clarity: separate calls for each of the “WillEndWarning” states.
164-169: Aligned with event-based logic.
The passing ofevent.rawValueensures no mismatch in user defaults or code references.
robertherber
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/index.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/index.ts
[error] 302-305: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 312-315: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 319-319: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Swift Test (example project)
- GitHub Check: Expo Bundle (example project)
🔇 Additional comments (4)
src/index.ts (3)
155-169: LGTM! Proper handling of timing-related properties.The implementation correctly converts Date objects to timestamps while safely handling undefined values.
399-400: LGTM! Good use of constants for string literals.Centralizing these string literals improves maintainability and reduces the risk of typos.
402-417: LGTM! Good tracking of shield updates.The changes properly track shield updates with timestamps and source information.
package.json (1)
3-3: LGTM! Version bump reflects new features.The version update to 0.4.6 aligns with the new features for preventing event retriggering.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
ios/Shared.swift (1)
62-123: 🧹 Nitpick (assertive)Consider refactoring large aggregator function
TheexecuteActionfunction handles multiple distinct responsibilities (sleep intervals, user defaults, optional app blocking, etc.) and relies on synchronous waits. Blocking the main thread can degrade user experience, especially on iOS. Consider splitting out sub-operations or using async calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
example/ios/Tests/SharedTests.swift(1 hunks)ios/Shared.swift(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Swift Test (example project)
- GitHub Check: Expo Bundle (example project)
🔇 Additional comments (14)
ios/Shared.swift (7)
15-16: Use constants for user defaults keys
Defining these constants is a good practice that improves maintainability and clarity when referencing user defaults throughout the code.
28-51: Verify logic of storing shield data
This function retrieves the dictionary from a key that includes theshieldId(e.g."shieldConfiguration_\(shieldId)"), but then stores it under a general"SHIELD_CONFIGURATION_KEY". Ensure this isn’t inadvertently overwriting configurations for other shields. If unintentional, consider persisting the updated dictionary under the same key.
584-593: Validate or sanitize keys
If unexpected characters appear inactivityName,callbackName, oreventName, they could collide with other user defaults keys. This was flagged previously as a potential concern.
609-615: Check string comparison edge cases
When numeric parsing fails, the fallback tolocalizedComparemight produce counterintuitive ordering for certain prefixed strings (e.g., “prefix_5” vs. “prefix_10”). If that’s the intended behavior, document it.
617-623: Minimal prefix remover
removePrefixIfPresentis concise. If performance in repeated calls is a concern, you might store or reuse the result, but otherwise looks good.
625-656: Potential performance overhead
hasHigherTriggeredEventiterates over the entire user defaults dictionary. In large-scale usage, consider caching or indexing to avoid repeated full scans.
762-775: Nil handling in last triggered time
ReturningnilfromgetLastTriggeredTimeFromUserDefaultscan risk repeated triggers if the calling code doesn’t handle it. As previously suggested, returning a sentinel (like 0 or -1) might make the logic safer.example/ios/Tests/SharedTests.swift (7)
117-155: Correct skip logic test
testShouldSkipIfAlreadyTriggeredAfterstraightforwardly confirms skip behavior when the last trigger time is beyond a threshold. Good coverage.
199-240: Verifies larger event handling
testShouldSkipIfLargerTriggeredAfterensures that if a higher event is logged after a certain time, the code properly skips. Logic looks correct.
242-285: Time-based skip
testSkipIfLargerTriggeredWithinMSis a good test of restricting a lower event. Adding a test for exactly the edge time (e.g., 100ms) may clarify boundary conditions.
287-337: Interval-based logic
testSkipIfLargerTriggeredAfterIntervalStartedchecks whether a larger event blocks triggers after an interval’s start. Great scenario coverage.
339-347: Numeric comparisons
testIsHigherEventNumconfirms numeric ordering. Looks correct and matches the intended logic for numeric event names.
349-357: String comparison vs. numeric
testIsHigherEventStringshows “prefix_5” is higher than “prefix_10” lexicographically. If you genuinely intend this difference from numeric ordering, add a comment clarifying.
359-378: Checks prefix removal
testReplaceTextnicely verifiesremovePrefixIfPresentwith multiple inputs, including empty. This coverage is strong.
Also adds more traceability:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
blockAppsWithSelectionIdmethod for enhanced app blocking functionality.Bug Fixes
Chores