Simplify removing a self-hosted site

[nbradbury]

Fixes CMM-180

We've a had a long-standing issue to make it easier to remove a self-hosted site from the app. Previously you could only do it by long-pressing the site in the site picker and confirming its removal. This PR changes this to use a very visible x icon at the end of the site row.

To test

Go to the site picker, tap the x icon on a self-hosted site, and verify that it works as expected.

[dangermattic]

	1 Warning
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

[nbradbury]

@claude

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr22487-e1de90f
	Commit	e1de90f
	Direct Download	jetpack-prototype-build-pr22487-e1de90f.apk

Note: Google Login is not supported on these builds.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr22487-e1de90f
	Commit	e1de90f
	Direct Download	wordpress-prototype-build-pr22487-e1de90f.apk

Note: Google Login is not supported on these builds.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 38.95%. Comparing base (04816ab) to head (e1de90f).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...n/java/org/wordpress/android/ui/main/SiteRecord.kt	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #22487   +/-   ##
=======================================
  Coverage   38.94%   38.95%           
=======================================
  Files        2189     2189           
  Lines      105364   105366    +2     
  Branches    14948    14948           
=======================================
+ Hits        41039    41043    +4     
+ Misses      60892    60890    -2     
  Partials     3433     3433           

☔ 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.

[nbradbury]

@claude

[claude]

Claude finished @nbradbury's task in 1m 13s —— View job

---

Code Review Complete ✅

I've reviewed the PR changes for simplifying the removal of self-hosted sites. Overall, the implementation is clean and well-tested. Here's my detailed feedback:

---

✅ Strengths

1. Good Test Coverage: The new ChooseSiteViewHolderTest.kt covers all four scenarios for the remove button visibility logic, which is excellent.

2. Clean Separation of Concerns: The logic for determining button visibility is extracted into a static companion function shouldShowRemoveButton(), making it testable and reusable.

3. Proper Layout Implementation: The use of a Barrier in the XML layout is a clean approach to handle the mutually exclusive visibility of the pin and remove buttons.

4. Analytics Integration: The existing code already has proper analytics tracking for site removal actions.

5. Consistent UX Pattern: The remove button follows the same pattern as the pin button (ImageView with clickable background).

---

🔍 Issues and Suggestions

1. Missing Test Coverage (Codecov Report)

The Codecov report shows 50% patch coverage with 1 line missing in SiteRecord.kt:30. Looking at the code:

isSelfHostedSite = !siteModel.isUsingWpComRestApi  // Line 30 - not covered

Recommendation: While this is initialization code in a data class, consider adding a simple test to verify the isSelfHostedSite property is correctly set based on isUsingWpComRestApi. This would bring coverage to 100%.

2. Potential Accessibility Improvement

In item_choose_site.xml:50, the remove button has a content description:

android:contentDescription="@string/remove_account"

This is good! However, consider whether the string resource remove_account is appropriate here. The context is removing a site from the app, not removing an account. The dialog title also uses remove_account (line 294 in ChooseSiteActivity.kt).

Recommendation: Consider if @string/remove_site or similar would be more semantically accurate. This could be a follow-up improvement.

3. Code Style: Companion Object Placement

According to CLAUDE.md, companion objects should be placed at the bottom of the class. In ChooseSiteViewHolder.kt, the companion object (lines 171-183) is correctly placed at the bottom. ✅

However, I notice the companion object is quite substantial with a helper function. This is fine, but consider if the function could be a top-level function in the file instead, since it doesn't need access to the class. This is a minor style preference, not a requirement.

4. Confirmation Dialog UX

The confirmation dialog in ChooseSiteActivity.kt:292-307 uses:

• Title: @string/remove_account
• Message: @string/sure_to_remove_account

Since this PR makes removal more prominent via a visible X button (instead of hidden in long-press), users might accidentally tap it. Consider:

• Making the dialog message more specific (e.g., show the site name being removed)
• Adding a "Don't show again" option (though this might be overkill)

Example improvement:

.setMessage(
    getString(R.string.sure_to_remove_site, site.name ?: site.url)
)

This is a UX enhancement suggestion, not a blocker.

5. Long Lines Check

According to CLAUDE.md, the line length limit is 120 characters. Let me check:

ChooseSiteViewHolder.kt:176-178 - The KDoc comment is formatted nicely with proper line breaks. ✅

All other lines appear to be within the 120 character limit. ✅

---

🔒 Security Considerations

• Input Validation: The code properly checks for null when retrieving the site: siteStore.getSiteByLocalId(siteRecord.localId) ?: return (line 288)
• Authorization: The removal only works for self-hosted sites, which is correct since WordPress.com sites should be managed through the WordPress.com account settings
• Confirmation Dialog: The modal confirmation prevents accidental deletion ✅

No security concerns identified.

---

⚡ Performance Considerations

• RecyclerView Updates: The code uses notifyDataSetChanged() in the adapter, which is acceptable for this use case since the entire list is being refreshed
• No Memory Leaks: The click listeners are properly scoped to the ViewHolder lifecycle
• Efficient Layout: The ConstraintLayout with Barrier is an efficient approach

No performance concerns identified.

---

📝 Documentation

• The KDoc for shouldShowRemoveButton() is clear and explains the business logic well
• The test method names are descriptive using the backtick naming convention

Minor suggestion: Consider adding a KDoc comment to handleRemoveButton() explaining when and why the button is shown, similar to the companion function.

---

🎯 Overall Assessment

This is a well-implemented PR that achieves its goal of making self-hosted site removal more discoverable. The code quality is high, and the addition of unit tests is commendable.

Recommended Actions Before Merge:

1. ✅ Optional: Add test coverage for SiteRecord.isSelfHostedSite to reach 100% coverage
2. ✅ Optional: Review string resources for semantic accuracy (remove_account vs remove_site)
3. ✅ Optional: Consider making the confirmation dialog more specific by showing the site name

Ready to merge once you've considered these optional improvements. Great work! 🚀

---

[nbradbury]

Since this PR makes removal more prominent via a visible X button (instead of hidden in long-press), users might accidentally tap it. Consider:

• Making the dialog message more specific (e.g., show the site name being removed)

I added this in e1de90f.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[adalpari]

This is a great UX improvement!!! 👏

Looks a bit odd to have the "X" only for self-hosted sites without any context.
But given that the PR is just moving the action to a more visual position, that's a different (design) issue.