PM-11215 - Build Debug Menu #941
Conversation
|
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
==========================================
+ Coverage 88.72% 88.74% +0.02%
==========================================
Files 631 638 +7
Lines 39753 39993 +240
==========================================
+ Hits 35269 35491 +222
- Misses 4484 4502 +18 ☔ View full report in Codecov by Sentry. |
|
🤔 Having the gesture for the debug menu be something that can't be done on the simulator is weird to me. Wouldn't it be better to have something that doesn't require a physical device? Or is the expectation that if we need the debug menu locally we'll go and change the gesture? Also of note: Debug builds are only built for dev locally with Xcode. Any build in TestFlight is a Release build, so this will only be usable by devs and not QA. |
| /// - Returns: The value of the feature flag as the specified type `T`, or `nil` if the flag does not exist | ||
| /// or cannot be decoded. | ||
| /// | ||
| func featureFlag(name: String) -> Bool? |
There was a problem hiding this comment.
⛏️ The docs indicate that it's a generic type T, but all we have here is Bool. This also means we don't have overrides for Int or String flags.
There was a problem hiding this comment.
good catch, yeah for now I kept them as booleans but I can add capability for other types. I just didn't want to hold this up for testing the onboarding flow but I can certainly tackle that. Would another ticket be okay with you?
There was a problem hiding this comment.
Future tickets for the additional types is reasonable to keep this a bit smaller and focused.
| /// - value: The boolean value to assign to the feature flag. If `nil`, the feature flag will be removed | ||
| /// from the settings store. | ||
| /// | ||
| func setFeatureFlag(name: String, value: Bool?) |
There was a problem hiding this comment.
🤔 If we're gating using these functions behind an #if DEBUG, wouldn't it make sense to also gate these functions and attendant code?
(Other discussion about whether we want to #if DEBUG aside)
There was a problem hiding this comment.
yeah that a great call too, no need to have these functions available unless they are needed.
Yeah I agree, the ticket stated to use the gesture so I went with that but we could add another vessel to open it as well. Do you have any that would be useful? I've done a shake gesture before, which can be used in the simulator, or also triple tapping specific images, etc. |
BitwardenShared/UI/Platform/Application/AppProcessorTests.swift
Outdated
Show resolved
Hide resolved
BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift
Outdated
Show resolved
Hide resolved
| @@ -187,6 +203,10 @@ class DefaultConfigService: ConfigService { | |||
| return configuration?.featureStates[flag]?.stringValue ?? defaultValue | |||
| } | |||
|
|
|||
| func getRemoteFeatureFlags() async -> [FeatureFlag: AnyCodable] { | |||
There was a problem hiding this comment.
🤔 Does it make sense to also gate this behind the DEBUG_MENU flag?
There was a problem hiding this comment.
yeah, good call out. That would make the most sense to me to avoid any issues in release builds
I don't have any good ideas for it, unfortunately; the way apps I've worked on in the past handled debug screens and the like was through other means. The three-finger triple tap is probably reasonable enough, and if we need to access the debug menu in the simulator we could just manually update it to five two-finger taps or something like that. In practice, it's probably moot. |
| @@ -121,4 +131,21 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate { | |||
| private func showSplash() { | |||
| splashWindow?.alpha = 1 | |||
| } | |||
|
|
|||
| /// Handle the triple-tap gesture and launch the debug menu. | |||
There was a problem hiding this comment.
⛏️ Can probably gate this behind DEBUG_MENU as well
Configs/BitwardenShared.xcconfig
Outdated
| @@ -1,4 +1,6 @@ | |||
| #include "./Common.xcconfig" | |||
| #include? "./Local.xcconfig" | |||
|
|
|||
| BITWARDEN_FLAGS = DEBUG_MENU | |||
There was a problem hiding this comment.
🤔 If we have it in Shared, it doesn't need to be in both Debug and Release, and potentially defeats the point of gating it?
Configs/Bitwarden-Release.xcconfig
Outdated
| @@ -5,6 +5,8 @@ CODE_SIGN_IDENTITY = Apple Development | |||
| #include? "./Local.xcconfig" | |||
|
|
|||
| ASSETCATALOG_COMPILER_APPICON_NAME = $(APPICON_NAME) | |||
| BITWARDEN_FLAGS = DEBUG_MENU | |||
There was a problem hiding this comment.
🤔 Is the intention that this would be overridden with some other flags locally on some CI builds? How would we, ideally, know whether or not a build on CI should or should not have the debug menu at build time?
Right now the intention is to have builds from the Livefront CI/CD to update the builds with the flag so QA can begin testing the onboarding flow. I wanted to have the structure in place when the Bitwarden CI/CD wants to implement this. Since beta builds go out to users we haven't come to a conclusion on how to handle that just yet but getting the builds with the menu will allow QA to test the new features. Does that make sense or am i being confusing? |
We wouldn't want to modify that xcconfig file in the pipeline, though. We'd want to add something to The real issue is that if we don't want this shipping to users, then we can't put it in any CI/CD pipeline builds unless we know for sure that particular build isn't going to be promoted, since the builds that "are uploaded to TestFlight" are the same ones we'd pull from to decide what to release. In past apps I've worked on, we had a separate build variant (parallel to our prod variant) specifically for QA that had these sorts of debugging things turned on, because it's much easier to say "prod has debug menu off, qa has debug menu on". This solves the problem relatively nicely (though QA is then not necessarily actually testing the artifact that's going to be released to the public). However, because both our Prod variant and Beta variant go to users, we can't use them in this manner; we'd either have to stop releasing Beta to users or create a different variant that's only used internally by QA. |
🎟️ Tracking
PM-11215
📔 Objective
📸 Screenshots
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-17.at.09.26.17.mp4
⏰ 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