-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-28177] Add feature flag for Send UI refresh #6708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Claude finished @mcamirault's task —— View job Review Complete ✓I've reviewed PR #6708 and the change looks good overall! Here's my assessment: Overall: APPROVE ✓This is a clean, straightforward addition of a feature flag constant for the Send UI refresh initiative. The implementation follows Bitwarden's established patterns and conventions correctly. Summary✓ Code Quality: The constant declaration is syntactically correct and follows C# naming conventions QuestionI have one clarification question about the ticket numbering - I've posted it as an inline comment on line 242 in the file diff. Detailed FindingsWhat's Good:
Observations:
|
| public const string UseSdkPasswordGenerators = "pm-19976-use-sdk-password-generators"; | ||
| public const string UseChromiumImporter = "pm-23982-chromium-importer"; | ||
| public const string ChromiumImporterWithABE = "pm-25855-chromium-importer-abe"; | ||
| public const string SendUIRefresh = "pm-28175-send-ui-refresh"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Clarification needed: Ticket number discrepancy
The PR title and description reference PM-28177, but the feature flag value uses pm-28175:
public const string SendUIRefresh = "pm-28175-send-ui-refresh";Is this intentional? Common scenarios:
- PM-28175 might be the epic/parent ticket for Send UI refresh
- PM-28177 is the specific task for adding this server-side flag
- The flag value should match PM-28175 for consistency with the epic
If PM-28175 is indeed the correct epic identifier, this is perfectly fine! Just want to confirm this wasn't a typo.
Note: Similar pattern exists with DesktopSendUIRefresh = "desktop-send-ui-refresh" on line 238, which suggests multiple flags for the same initiative across different scopes.
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6708 +/- ##
=======================================
Coverage 53.64% 53.64%
=======================================
Files 1926 1926
Lines 85720 85720
Branches 7687 7687
=======================================
Hits 45981 45981
Misses 37967 37967
Partials 1772 1772 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|

🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28177
📔 Objective
Add feature flag for the Send UI refresh epic
📸 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