Introducing familyActivitySelectionId to reduce load on userDefaults … #1
Conversation
…and streamline structure
|
Warning Rate limit exceeded@robertherber has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces modifications to the device activity management system, focusing on replacing references to activity names with unique identifiers across multiple files. Key changes include renaming methods, updating type signatures, and adjusting how family activity selections are stored and retrieved. The updates are reflected in React Native components, Swift files, and TypeScript type definitions, emphasizing a streamlined approach to managing family activity selections through identifiers. Changes
Sequence DiagramsequenceDiagram
participant App
participant DeviceActivity
participant ShieldConfiguration
App->>DeviceActivity: setFamilyActivitySelectionId(id)
DeviceActivity-->>App: Store selection ID
App->>ShieldConfiguration: updateShield(configuration)
ShieldConfiguration->>DeviceActivity: Apply shield with selection ID
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: 6
🔭 Outside diff range comments (1)
example/screens/ShieldTab.tsx (1)
Missing Required Parameter in updateShield Call
The current implementation is missing the required
shieldActionsparameter. Based on the type definitions,updateShieldrequires two parameters:
shieldConfiguration: Your current configuration appears valid with the correct properties (title, colors, etc.)shieldActions: This mandatory parameter is missing and must include at least aprimaryactionYou need to add the missing
shieldActionsparameter with at least:shieldActions: { primary: ShieldAction }🔗 Analysis chain
Line range hint
70-92: Ensure Correct Usage of Updated MethodThe function
ReactNativeDeviceActivity.updateShieldreplaces the deprecatedupdateShieldConfiguration. Ensure that all parameters and their structures align with the new method's expectations.Please confirm that
updateShieldaccepts the same parameters or adjust them accordingly. Run the following script to check the method definition:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the definition of updateShield rg -A 5 "function updateShield\(" src/Length of output: 342
Script:
#!/bin/bash # Let's check the ShieldConfiguration type definition ast-grep --pattern 'type ShieldConfiguration = { $$$ }' # Also search for any usage examples rg -B 5 -A 10 "updateShield\(" --type typescriptLength of output: 4738
Script:
#!/bin/bash # Let's check the ShieldActions type and search for usage examples with proper file type ast-grep --pattern 'type ShieldActions = { $$$ }' rg -B 5 -A 10 'updateShield\(' --type ts --type tsxLength of output: 393
🧹 Nitpick comments (4)
ios/Shared.swift (2)
261-263: Add Documentation toFamilyActivitySelectionWithIdStructConsider adding documentation comments to the
FamilyActivitySelectionWithIdstruct to clarify its purpose and usage within the codebase. This improves maintainability and helps other developers understand its role.Apply this diff to add documentation:
+ /// A struct that pairs a `FamilyActivitySelection` with its unique identifier. struct FamilyActivitySelectionWithId { var selection: FamilyActivitySelection var id: String }
Line range hint
378-409: Simplify Optional Unwrapping ingetPossibleFamilyActivitySelectionIdThe return statement
return foundIt??.iduses double optional chaining, which can be confusing. Adjust the unwrapping to improve readability and ensure safe optional handling.Apply this diff to simplify the return statement:
- return foundIt??.id + if let mapping = foundIt, let id = mapping?.id { + return id + } + return niltargets/ShieldConfiguration/ShieldConfigurationExtension.swift (1)
Line range hint
107-113: Avoid Code Duplication in Placeholder DictionariesThe
placeholdersdictionary is constructed similarly in multiple methods. Consider extracting it into a shared function to reduce code duplication and improve maintainability.Apply this diff to create a shared function:
func createPlaceholders(token: Token?, tokenType: String, activityToken: ActivityCategoryToken?) -> [String: String?] { return [ "applicationOrDomainDisplayName": token?.localizedDisplayName, "token": "\(token?.hashValue ?? 0)", "tokenType": tokenType, "familyActivitySelectionId": getPossibleFamilyActivitySelectionId( applicationToken: token as? ApplicationToken, webDomainToken: token as? WebDomainToken, categoryToken: activityToken ) ] }Then update the methods to use this function:
override func configuration(shielding application: Application) -> ShieldConfiguration { let placeholders = createPlaceholders(token: application.token, tokenType: "application", activityToken: nil) // Rest of the method remains the same }example/components/CreateActivity.tsx (1)
21-23: Remove Unnecessary Commented CodeThe commented-out code for
ReactNativeDeviceActivity.setFamilyActivitySelectionIdmay cause confusion. If it's no longer needed, consider removing it to keep the code clean.Apply this diff to remove the commented code:
- // ReactNativeDeviceActivity.setFamilyActivitySelectionId({ - // activityName, - // familyActivitySelection: activitySelection, - // });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
example/components/CreateActivity.tsx(1 hunks)example/screens/AllTheThings.tsx(4 hunks)example/screens/ShieldTab.tsx(1 hunks)ios/Shared.swift(6 hunks)src/ReactNativeDeviceActivity.types.ts(1 hunks)src/index.ts(1 hunks)targets/ShieldConfiguration/ShieldConfigurationExtension.swift(4 hunks)
🔇 Additional comments (7)
targets/ShieldConfiguration/ShieldConfigurationExtension.swift (3)
Line range hint 130-136: See Previous Comment Regarding Code Duplication
As with the previous method, consider using the shared function for placeholder creation to avoid duplication.
Line range hint 151-157: See Previous Comment Regarding Code Duplication
Again, consider refactoring to use the shared function for placeholder creation.
Line range hint 175-181: See Previous Comment Regarding Code Duplication
Refactor to use the shared placeholder creation function to enhance code maintainability.
src/index.ts (1)
161-176: Implementation looks good and aligns with PR objectives.
The function properly maintains existing mappings while adding new ones. The approach of using a single userDefaults key for all mappings helps reduce the load on userDefaults storage.
Let's verify the usage of this new function across the codebase:
✅ Verification successful
Implementation verified and properly integrated across the codebase
The verification shows:
- The new function is properly integrated with a commented example usage in
example/components/CreateActivity.tsx - The
familyActivitySelectionIdskey is consistently used across both TypeScript and Swift code - No legacy storage patterns were found, indicating a clean migration to the new approach
- The implementation is properly consumed by the Swift code which reads the stored mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of the old familyActivitySelection storage pattern
rg "familyActivitySelectionToActivityNameMap"
# Verify all calls to the new function match the expected signature
ast-grep --pattern 'setFamilyActivitySelectionId({ id: $_, familyActivitySelection: $_ })'
Length of output: 138
Script:
#!/bin/bash
# Search for any usage of the new function name
rg "setFamilyActivitySelectionId" -A 2
# Search for any usage of the familyActivitySelectionIds key in userDefaults
rg "familyActivitySelectionIds"
# Look for any other potential related storage patterns
rg "ActivitySelection.*userDefaults" -i
Length of output: 1438
src/ReactNativeDeviceActivity.types.ts (1)
153-153:
Breaking change: Verify all consumers of the Action type.
The property rename from familyActivitySelection to familyActivitySelectionId is a breaking change that requires updates in all code that creates actions of type "blockSelection".
Let's verify all usages of this action type:
example/screens/AllTheThings.tsx (2)
Line range hint 37-44: LGTM: Cleanup of debug logging.
Good cleanup of development-only debug logging.
Line range hint 266-309: LGTM: Proper usage of updated shield API.
The change to use updateShield is consistent with the API updates and maintains proper typing of shield configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ios/Shared.swift (1)
71-71: 🛠️ Refactor suggestionConsider using async/await or completion handlers instead of sleep.
Using
sleepfor timing control can lead to poor user experience and potential race conditions. Consider using more modern approaches:
- For shield config changes: Use notification observers or completion handlers
- For HTTP requests: Use async/await or completion handlers
Also applies to: 85-85
🧹 Nitpick comments (1)
ios/Shared.swift (1)
Line range hint
375-406: Consider simplifying the token matching logic.The nested conditions in
getPossibleFamilyActivitySelectionIdcould be simplified for better readability.Consider this refactor:
func getPossibleFamilyActivitySelectionId( applicationToken: ApplicationToken?, webDomainToken: WebDomainToken?, categoryToken: ActivityCategoryToken? ) -> String? { let familyActivitySelectionIds = getFamilyActivitySelectionIds() - let foundIt = familyActivitySelectionIds.first(where: { (mapping) in - if let mapping = mapping { - if let applicationToken = applicationToken { - if mapping.selection.applicationTokens.contains(applicationToken) { - return true - } - } - - if let webDomainToken = webDomainToken { - if mapping.selection.webDomainTokens.contains(webDomainToken) { - return true - } - } - - if let categoryToken = categoryToken { - if mapping.selection.categoryTokens.contains(categoryToken) { - return true - } - } - } - return false - }) + let foundIt = familyActivitySelectionIds.first { mapping in + guard let mapping = mapping else { return false } + return (applicationToken.map { mapping.selection.applicationTokens.contains($0) } ?? false) || + (webDomainToken.map { mapping.selection.webDomainTokens.contains($0) } ?? false) || + (categoryToken.map { mapping.selection.categoryTokens.contains($0) } ?? false) + } return foundIt??.id }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
example/components/CreateActivity.tsx(1 hunks)ios/Shared.swift(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- example/components/CreateActivity.tsx
🔇 Additional comments (3)
ios/Shared.swift (3)
50-59: LGTM! Error handling looks good.
The changes properly handle the case where activity selection is not found and include appropriate error logging.
264-266: LGTM! Clean struct implementation.
The struct effectively combines the activity selection with its identifier, supporting the new identifier-based approach.
345-356: LGTM! Efficient implementation of activity selection retrieval.
The functions effectively implement the new identifier-based approach with proper error handling and efficient dictionary access.
Also applies to: 362-372
…and streamline structure
And testing out pkg.pkg config :)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores