Skip to content

Conversation

@AlexKarpov98
Copy link
Contributor

Testing: see ticket.

Preview:

Screen.Recording.2026-01-05.at.14.13.16.mov

@AlexKarpov98 AlexKarpov98 self-assigned this Jan 5, 2026
@bugclerk bugclerk changed the title NAS-139053: Boot environments dialogue- keep/unkeep flag switched NAS-139053 / 26.04 / Boot environments dialogue- keep/unkeep flag switched Jan 5, 2026
@bugclerk
Copy link
Contributor

bugclerk commented Jan 5, 2026

@claude
Copy link

claude bot commented Jan 5, 2026

Claude finished @AlexKarpov98's task —— View job


Review Complete! ✅

Great fix! The keep/unkeep icons are now correctly assigned, and the test coverage is comprehensive.

What Changed

The PR swaps the bookmark icons that were incorrectly assigned:

  • Keep action (when keep=false): Now shows bookmark_border (outline) ✓
  • Unkeep action (when keep=true): Now shows bookmark (filled) ✓

This matches the expected UX pattern where the outline bookmark indicates "add to bookmarks" and the filled bookmark indicates "remove from bookmarks."

Code Quality 👍

The implementation is clean and straightforward:

  • The logic in toggleKeep() was already correct (checking !bootenv.keep for the keep action)
  • Only the icon assignments needed swapping
  • The fix is minimal and focused on the actual bug

Test Coverage 🧪

Excellent test additions! The new tests verify:

  • Correct icon display based on keep state (lines 118-126)
  • Keep action triggers correct API call with value: true (lines 128-141)
  • Unkeep action triggers correct API call with value: false (lines 143-156)
  • Uses harnesses properly (IxIconHarness, IxTableHarness)
  • Minimal mock data with proper typing

The tests follow the repository's testing guidelines perfectly - using harnesses over direct DOM queries and providing minimally sufficient mock data.

Minor Observation

The test data includes a third boot environment with keep: true to test the unkeep scenario. This is good practice for comprehensive coverage of both states.

Excellent work - this is ready to merge! 🚀

@codecov
Copy link

codecov bot commented Jan 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.73%. Comparing base (297a642) to head (999b391).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13062      +/-   ##
==========================================
+ Coverage   86.71%   86.73%   +0.02%     
==========================================
  Files        1847     1847              
  Lines       69415    69415              
  Branches     8585     8585              
==========================================
+ Hits        60193    60208      +15     
+ Misses       9222     9207      -15     

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AlexKarpov98 AlexKarpov98 marked this pull request as ready for review January 5, 2026 16:00
@AlexKarpov98 AlexKarpov98 requested a review from a team as a code owner January 5, 2026 16:00
@AlexKarpov98 AlexKarpov98 requested review from william-gr and removed request for a team January 5, 2026 16:00
@AlexKarpov98 AlexKarpov98 merged commit adf3306 into master Jan 6, 2026
15 checks passed
@AlexKarpov98 AlexKarpov98 deleted the NAS-139053 branch January 6, 2026 09:12
@bugclerk
Copy link
Contributor

bugclerk commented Jan 6, 2026

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Jan 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants