Add more notification actions and multiple shield actions #53
Conversation
WalkthroughThis pull request introduces a new function, Changes
Sequence Diagram(s)sequenceDiagram
participant MonitorExt as DeviceActivityMonitorExtension
participant Shared as Shared.swift
MonitorExt->>Shared: executeGenericAction(action, placeholders, triggeredBy)
Note right of Shared: Processes sleepBefore, evaluates action types, then sleepAfter
Shared-->>MonitorExt: Action processed
sequenceDiagram
participant Shield as ShieldActionExtension
participant Shared as Shared.swift
Shield->>Shared: executeGenericAction(action, placeholders, triggeredBy)
Note right of Shared: Iterates through actions from configForSelectedAction
Shared-->>Shield: Action executed
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
🪧 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: 10
🔭 Outside diff range comments (2)
packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift (2)
121-125: 🛠️ Refactor suggestionUse placeholders in sendNotification
The sendNotification call uses an empty placeholders dictionary instead of the newly passed placeholders parameter, which seems inconsistent with the rest of the changes.
if type == "sendNotification" { if let payload = configForSelectedAction["payload"] as? [String: Any] { - sendNotification(contents: payload, placeholders: [:]) + sendNotification(contents: payload, placeholders: placeholders) } }
25-145: 🧹 Nitpick (assertive)Consider refactoring specific action types to use executeGenericAction
The function still contains logic for handling specific action types directly instead of leveraging the new generic action execution mechanism. This creates duplication and makes maintenance harder.
Consider refactoring each action type block to use
executeGenericActionfor consistency. For example:if type == "disableBlockAllMode" { - disableBlockAllMode(triggeredBy: "shieldAction") + executeGenericAction( + action: ["type": "disableBlockAllMode"], + placeholders: placeholders, + eventKey: "shieldAction" + ) }This would create a more consistent pattern throughout the codebase where all action execution goes through the same code path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
packages/react-native-device-activity/ios/Shared.swift(1 hunks)packages/react-native-device-activity/package.json(1 hunks)packages/react-native-device-activity/src/ReactNativeDeviceActivity.types.ts(2 hunks)packages/react-native-device-activity/targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift(2 hunks)packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/react-native-device-activity/targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (1)
packages/react-native-device-activity/ios/Shared.swift (1)
executeGenericAction(100-218)
packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift (1)
packages/react-native-device-activity/ios/Shared.swift (1)
executeGenericAction(100-218)
packages/react-native-device-activity/ios/Shared.swift (1)
packages/react-native-device-activity/src/index.ts (9)
updateShield(569-584)blockSelection(424-436)unblockSelection(450-462)addSelectionToWhitelistAndUpdateBlock(135-147)removeSelectionFromWhitelistAndUpdateBlock(149-161)clearWhitelist(105-107)resetBlocks(446-448)disableBlockAllMode(442-444)enableBlockAllMode(438-440)
🔇 Additional comments (11)
packages/react-native-device-activity/package.json (1)
3-3: Version bump confirms alignment with new features.
No concerns here; incrementing from 0.4.24 to 0.4.25 is appropriate for the additional functionality introduced elsewhere.packages/react-native-device-activity/src/ReactNativeDeviceActivity.types.ts (2)
182-182: Expanding ShieldAction to support multiple actions is a good improvement.
This addition promotes flexibility by allowing multipleActionitems. Ensure any top-level parsing handles both deprecated and new properties gracefully.
272-283:Details
❓ Verification inconclusive
Excellent extension of action types.
The new"setBadgeCount","removeAllPendingNotificationRequests", and"removePendingNotificationRequests"actions broaden functionality. Given thatsetBadgeCountrequires iOS 16, verify that any fallback behavior is clear for earlier iOS versions.
🏁 Script executed:
#!/bin/bash # Description: Check usage of "setBadgeCount" for iOS <16 fallback. rg --json "setBadgeCount"Length of output: 1938
Action Required: Validate Fallback Handling for iOS Versions <16
Excellent extension of the action types with the introduction of
"setBadgeCount","removeAllPendingNotificationRequests", and"removePendingNotificationRequests". However, since the new"setBadgeCount"action requires iOS 16, please ensure that there is explicit fallback behavior or clear documentation for devices running iOS versions lower than 16. For example, in the Swift implementation inpackages/react-native-device-activity/ios/Shared.swift(around lines 191 and 197), there is no visible conditional check (such as usingif #available(iOS 16, *)) to handle fallback scenarios. Kindly verify that either appropriate version checks or descriptive documentation is in place so that developers and users understand how the feature behaves on older iOS versions.packages/react-native-device-activity/ios/Shared.swift (3)
157-161: Clean approach to clearing the whitelist and updating.
This block looks solid and aligns with the existing pattern for block/whitelist modifications.
192-200:Details
❓ Verification inconclusive
Ensure fallback for devices below iOS 16.
UNUserNotificationCenter.setBadgeCountis iOS 16+ only, so confirm that older iOS versions still function as expected without error.
🏁 Script executed:
#!/bin/bash # Searching for setBadgeCount usage to confirm fallback handling. rg "setBadgeCount"Length of output: 304
Fallback Compatibility for iOS Versions Below 16
The current implementation only updates the badge count when running on iOS 16+ (inside the
#available(iOS 16.0, *)block). For devices running earlier iOS versions, there’s no explicit fallback or alternative handling. Please verify if the omission of badge count updates on older iOS versions is acceptable or if a fallback mechanism (such as logging or an alternative update method) should be implemented.
- Location:
packages/react-native-device-activity/ios/Shared.swift(lines 192–200)- Observation: No fallback branch exists for devices that do not meet the iOS 16 requirement despite the type being defined in
ReactNativeDeviceActivity.types.ts.
141-156:Details
❓ Verification inconclusive
Double-check parse behavior.
parseActivitySelectionInputmay return an empty selection if parsing fails. Ensure the calling logic or downstream code can handle that gracefully to avoid silent failures.
🏁 Script executed:
#!/bin/bash # Description: Check for usage sites of parseActivitySelectionInput and verify error handling. ast-grep --pattern $'parseActivitySelectionInput($_)'Length of output: 2898
Verify empty selection handling in
parseActivitySelectionInputThe code in
Shared.swift(lines 141–156) callsparseActivitySelectionInput(input:)without an explicit check for an empty result. Since this function may return an empty selection upon failure, please ensure that both the functions invoked here (namelyaddSelectionToWhitelistAndUpdateBlockandremoveSelectionFromWhitelistAndUpdateBlock) and other similar usage sites inReactNativeDeviceActivityModule.swifthandle an empty selection appropriately. This will help prevent silent failures and ensure that any parsing errors are either logged or otherwise managed.packages/react-native-device-activity/targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (2)
12-12: Import of NotificationCenter.
This allows removing or scheduling notification requests. Looks appropriate for the new actions introduced.
103-103: Adoption of the generic action executor is a good step.
Replacing any specialized implementation withexecuteGenericActionunifies the logic and reduces duplication.packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift (3)
12-14: LGTM: Function renamed for better semantics and added placeholders supportThe function has been appropriately renamed from
handleActiontohandleShieldActionto better reflect its purpose and the newplaceholdersparameter enables dynamic substitution in actions.
24-24: LGTM: Correct condition updateThe condition has been properly updated to use
else ifto account for the new actions array check.
194-197: LGTM: Correctly calling renamed function with placeholdersThe function call has been updated to use the renamed
handleShieldActionfunction and properly passes the placeholders parameter.
packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift
Show resolved
Hide resolved
packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
packages/react-native-device-activity/targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (1)
57-67: 🧹 Nitpick (assertive)Use descriptive
triggeredBynaming.
ThetriggeredBylabel reflects the originating event (e.g.,"actions_for_<activityName>_<callbackName>"). That’s useful, but consider reverting to the standard snake/camel-case naming if your logs or analytics systems have established naming conventions.packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift (1)
33-165: 🧹 Nitpick (assertive)Consider moving remaining action types to executeGenericAction
Now that you've implemented the
executeGenericActionfunction and moved some action types there (like "addCurrentToWhitelist"), consider moving the remaining action types in this function to the generic handler for better consistency and maintainability.This would make the code more consistent by having all action types handled in one place, making it easier to maintain and extend in the future.
♻️ Duplicate comments (4)
packages/react-native-device-activity/ios/Shared.swift (3)
100-112: 🧹 Nitpick (assertive)Caution with synchronous sleeps.
Usingsleep(ms:)on lines 110-112 (and elsewhere) can block the extension’s thread, potentially impacting performance or responsiveness. Consider an asynchronous approach or shorter waits, especially in an extension environment.- if let sleepBefore = action["sleepBefore"] as? Int { - sleep(ms: sleepBefore) - } + // Potential asynchronous approach if feasible: + // DispatchQueue.global().asyncAfter(deadline: .now() + .milliseconds(sleepBefore)) { ... }
210-217: 🧹 Nitpick (assertive)Consider removing delivered notifications as well.
The code removes only pending notification requests. If you also intend to remove delivered notifications, callremoveDeliveredNotificationssimilarly.+ // If you want to remove delivered notifications too: + UNUserNotificationCenter.current().removeDeliveredNotifications(withIdentifiers: identifiers)
231-239:⚠️ Potential issueAvoid blocking sleeps after sending HTTP requests.
Sleeping for 1 second immediately after sending a request may still leave asynchronous requests incomplete if the network is slow. Consider a callback-based or asynchronous approach.- task = sendHttpRequest(with: url, config: config, placeholders: placeholders) - sleep(ms: 1000) + task = sendHttpRequest(with: url, config: config, placeholders: placeholders) + // Possibly handle async completion or use OS background tasks.packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift (1)
192-201: Avoid force unwrapping optional tokensThe code force unwraps
applicationToken!andwebdomainToken!after checking if they're nil, which could potentially lead to runtime crashes if the token becomes unexpectedly nil after the check.let placeholders: [String: String?] = [ "action": actionButton, - "applicationName": applicationToken != nil - ? Application(token: applicationToken!).localizedDisplayName : nil, - "webDomain": webdomainToken != nil - ? WebDomain( - token: webdomainToken! - ).domain : nil, + "applicationName": applicationToken.map { Application(token: $0).localizedDisplayName }, + "webDomain": webdomainToken.map { WebDomain(token: $0).domain }, "familyActivitySelectionId": familyActivitySelectionId?.id ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (4)
packages/react-native-device-activity/ios/Shared.swift(1 hunks)packages/react-native-device-activity/src/ReactNativeDeviceActivity.types.ts(3 hunks)packages/react-native-device-activity/targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift(4 hunks)packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/react-native-device-activity/targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (1)
packages/react-native-device-activity/ios/Shared.swift (1)
executeGenericAction(100-244)
packages/react-native-device-activity/ios/Shared.swift (1)
packages/react-native-device-activity/src/index.ts (9)
updateShield(569-584)blockSelection(424-436)unblockSelection(450-462)addSelectionToWhitelistAndUpdateBlock(135-147)removeSelectionFromWhitelistAndUpdateBlock(149-161)clearWhitelist(105-107)resetBlocks(446-448)disableBlockAllMode(442-444)enableBlockAllMode(438-440)
packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift (1)
packages/react-native-device-activity/ios/Shared.swift (1)
executeGenericAction(100-244)
🔇 Additional comments (8)
packages/react-native-device-activity/src/ReactNativeDeviceActivity.types.ts (3)
160-163: Rename aligns with standardized action names.
Renaming"whitelistCurrent"to"addCurrentToWhitelist"is consistent with the new approach. No concerns here.
173-174: Confirm deprecation timeline.
Marking thetypefield as deprecated is appropriate, but consider scheduling removal or providing migration guidance to help consumers switch toactions.
182-182: Newactionsproperty is a clear improvement.
The addition ofactions?: Action[];allows more flexibility. Ensure that all internal references totypealone accommodate this array-based approach.packages/react-native-device-activity/ios/Shared.swift (1)
193-198: Opening the app with a fixed URL.
Using"device-activity://"as a placeholder is fine for initial testing, but consider making the URL configurable or at least documented if you anticipate future changes.Do you plan to externally configure or localize this URL? If so, I can help add that property.
packages/react-native-device-activity/targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (2)
12-12: New NotificationCenter import is appropriate.
Pulling inNotificationCenterusage is valid for handling or removing notifications. No concerns here.
70-70: Ensure robust null-checking ofactions.
The optional array from user defaults can be omitted or invalid, so the current check is correct. Possibly add logs if no array is found.packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift (2)
12-31: Greatly improved action handling architecture!The refactoring to support multiple actions through the
executeGenericActionfunction provides a more flexible and maintainable approach. This will make it easier to add new action types in the future while keeping the code organized.
136-136: Good name improvement for action typeRenaming from "whitelistCurrent" to "addCurrentToWhitelist" is more descriptive and follows the verb-noun convention, improving code readability.
packages/react-native-device-activity/src/ReactNativeDeviceActivity.types.ts
Show resolved
Hide resolved
...native-device-activity/targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift
Show resolved
Hide resolved
packages/react-native-device-activity/targets/ShieldAction/ShieldActionExtension.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
packages/react-native-device-activity/ios/Shared.swift (6)
100-113: Introduce safer handling for missing “type” and reduce reliance on synchronous sleeps.
Currently, if theaction["type"]is missing or malformed, the function silently skips. Consider throwing or logging an error. Also, callingsleep(ms:)blocks the thread, potentially impacting UI responsiveness and extension performance.
133-152: Consider removing the 50ms sleep to avoid brittle timing.
Usingsleep(ms: 50)to wait on shield updates is fragile and may fail under slower conditions. An asynchronous callback or completion block might be more reliable.
153-166: Synchronize shield actions upon unblocking to maintain consistency.
Removing only the shield configuration for the selection inunblockSelectionmay leave stale shield actions. Consider removing the associated shield actions as well if the intention is a full unblock.
194-209: Replace “todo” and reconsider extra sleeps for openApp & enableBlockAllMode.
• The// todo: replace with general stringcan be addressed now to ensure maintainability.
• The 1s and 50ms sleeps risk blocking the main thread, particularly in an extension environment.
233-241: Avoid blocking sleeps after sending HTTP requests.
Sleeping for 1s is no guarantee of network completion and may freeze the extension. Prefer async callbacks.
243-246: Synchronous sleep may stall extension shutdown.
sleepAfteragain blocks the thread. Consider asynchronous alternatives to improve performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (2)
packages/react-native-device-activity/ios/Shared.swift(1 hunks)packages/react-native-device-activity/src/ReactNativeDeviceActivity.types.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/react-native-device-activity/ios/Shared.swift (1)
packages/react-native-device-activity/src/index.ts (9)
updateShield(569-584)blockSelection(424-436)unblockSelection(450-462)addSelectionToWhitelistAndUpdateBlock(135-147)removeSelectionFromWhitelistAndUpdateBlock(149-161)clearWhitelist(105-107)resetBlocks(446-448)disableBlockAllMode(442-444)enableBlockAllMode(438-440)
🔇 Additional comments (9)
packages/react-native-device-activity/ios/Shared.swift (4)
114-131: Looks consistent with intended whitelist logic.
The approach of retrieving the current whitelist and inserting tokens aligns with the new “addCurrentToWhitelist” action. No immediate issues found here.
167-174: Clear, focused addition to the whitelist.
This logic seamlessly parses the selection and updates the block state. No issues noted.
175-182: Remove selection from whitelist is straightforward.
The flow properly calls the core helper to mutate the whitelist and reapply blocking. Looks good.
183-187: ClearWhitelistAndUpdateBlock is properly sequenced.
The actions here ensure the whitelist is cleared first, then the block is updated. No concerns.packages/react-native-device-activity/src/ReactNativeDeviceActivity.types.ts (5)
163-163: New “addCurrentToWhitelist” action type aligns with iOS code changes.
This syncs well with the new Swift logic.
182-182: Introducing a flexibleactionsarray fosters better extensibility.
No issues noted. This helps unify different action flows under one property.
272-284: Added action types enhance coverage of notification management.
The expanded actions—likeremovePendingNotificationRequests—align with iOS capabilities.
285-286: “addCurrentToWhitelist” action matches Swift implementation.
This keeps type definitions consistent with native code changes.
288-288: “removeAllDeliveredNotifications” rounds out notification clearing.
This addition complements the pending notification removal, providing full coverage.
packages/react-native-device-activity/src/ReactNativeDeviceActivity.types.ts
Show resolved
Hide resolved
robertherber
deleted the
add-more-notification-actions-and-multiple-shield-actions
branch
Summary by CodeRabbit
New Features
Refactor
Chores