[PM-26061] Consolidate Debug Menu to BitwardenKit #2177
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2177 +/- ##
==========================================
- Coverage 85.42% 84.13% -1.29%
==========================================
Files 1733 1974 +241
Lines 146246 161551 +15305
==========================================
+ Hits 124926 135918 +10992
- Misses 21320 25633 +4313 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KatherineInCode
requested review from
a team and
matt-livefront
as code owners
|
Claude finished @KatherineInCode's task —— View job Code Review: PR 2177 - Consolidate Debug Menu to BitwardenKit
Overall Assessment: APPROVE with Required FixThis PR successfully consolidates the Debug Menu from AuthenticatorShared to BitwardenKit, making it available for broader reuse. The architecture changes are sound, code follows established patterns, and tests are comprehensive. Critical SDK Hex Concern Analysis✅ SDK Hex Pattern - NOT A BREAKING CONCERNAddresses @fedemkr's question about The commit func zeroingUnwantedHexStrings() -> String {
let sdkAddressPattern = "_[0-9a-fA-F]{12,16}_" // Matches 12 to 16 hex digits
let sdkAddressReplacement = "_0000000000000000_"
// applies regex replacement
}Why this works:
Verdict: Tests will NOT break with SDK updates. The implementation dynamically zeros out hex addresses regardless of their value. Must Fix Before Merge❌ Missing Localization StringFile: The view references Required addition: Impact: Button label will display raw key instead of localized text. Should Address
|
fedemkr
left a comment
fedemkr
left a comment
There was a problem hiding this comment.
Looks good, just one small question.
| AuthenticatorBridgeKitMocks: 0x0000000000000000 | ||
| BitwardenKit: 0x0000000000000000 | ||
| BitwardenKitMocks: 0x0000000000000000 | ||
| BitwardenSdk_464161978EAC5FCE_PackageProduct: 0x0000000000000000 |
There was a problem hiding this comment.
🤔 I'm wondering if the 464161978EAC5FCE changes with new releases of the SDK. If so, we should make this more flexible or the test will fail every time the SDK gets updated.
There was a problem hiding this comment.
That's a very good question, I don't actually know 🤔 I'll dig into it, see where that's coming from and how I can update the snapshot test to handle it better.
There was a problem hiding this comment.
@fedemkr As near as I could tell, the hex string didn't change with a new release. However, just to be safe, I still set it up to zero out that hex string (like we're already doing with the addresses).
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-26061
📔 Objective
This consolidates the Debug Menu and related objects to
BitwardenKit, as part of a broader effort to consolidate things toBitwardenKit.I had to also bring in our
safeIndexarray extension as part of this, but I also think we should expand usage of it (particularly in tests).I also split out the "Generate SDK Error" from "Generate Error", to provide more granular control. As well, I changed which kind of error is generated.
The close button of the Debug menu has also been updated to match the rest of the app.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes