Skip to content

Conversation

@jaasen-livefront
Copy link
Collaborator

@jaasen-livefront jaasen-livefront commented Nov 25, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-20206
https://bitwarden.atlassian.net/browse/PM-22372

📔 Objective

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

@claude
Copy link
Contributor

claude bot commented Nov 25, 2025

Claude finished @jaasen-livefront's task —— View job


Code Review Complete

Tasks

  • Read changed file and understand modifications
  • Review git diff for exact changes
  • Search codebase for remaining references to removed flags
  • Evaluate PR title and description quality
  • Provide comprehensive review feedback

Summary of Changes

This PR removes two end-user notification feature flags from src/Core/Constants.cs:

  • EndUserNotifications (pm-10609-end-user-notifications)
  • PM19315EndUserActivationMvp (pm-19315-end-user-activation-mvp)

Findings

Finding 1: PR description lacks objective context (💭)

The PR template's ## 📔 Objective section is empty. For feature flag removals, it's helpful to document:

  • Whether these features are now permanently enabled or completely removed
  • Any migration or rollout considerations
  • Timeline or decision context (e.g., "Feature has been stable for X months")
Suggested improvement
## 📔 Objective

Remove the `EndUserNotifications` and `PM19315EndUserActivationMvp` feature flags as these features have been fully rolled out and are now permanently enabled in production. These flags are no longer needed for gradual rollout control.

Finding 2: Duplicate commits with identical messages (🎨)

The PR contains two commits with the same message "remove end user notifications feature flag" (81e0e44 and 6bd48b4). These could be squashed into a single, cleaner commit before merging.

Consider using:

git rebase -i origin/main

Good Practices Observed

  • Clean, straightforward removal with no remaining references in the codebase
  • Both Jira tickets properly referenced in PR title

Action Items

  • Add objective context to PR description explaining why these flags are being removed
  • (Optional) Consider squashing duplicate commits for cleaner git history

@jaasen-livefront jaasen-livefront changed the title [PM-20206] - remove end user notifications feature flag [PM-20206][PM-22372] - remove end user notifications feature flags Nov 25, 2025
@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailsd8b8951e-0516-4e48-bc8e-6e4046037b76

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.10%. Comparing base (1413dd7) to head (81e0e44).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6642   +/-   ##
=======================================
  Coverage   53.10%   53.10%           
=======================================
  Files        1903     1903           
  Lines       84877    84877           
  Branches     7633     7633           
=======================================
+ Hits        45071    45074    +3     
+ Misses      38055    38053    -2     
+ Partials     1751     1750    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants