Skip to content

fix(admin): Router role no longer reverts when selected in Remote Admin#3153

Merged
Yeraze merged 1 commit into
mainfrom
fix/admin-router-role-revert
May 23, 2026
Merged

fix(admin): Router role no longer reverts when selected in Remote Admin#3153
Yeraze merged 1 commit into
mainfrom
fix/admin-router-role-revert

Conversation

@Yeraze
Copy link
Copy Markdown
Owner

@Yeraze Yeraze commented May 23, 2026

Summary

User report: in the Remote Admin → Device Configuration role dropdown, every attempt to pick Router would immediately revert to the previous selection. Other roles (Sensor, Tracker, etc.) saved fine.

Root cause

src/components/AdminCommandsTab.tsx handleRoleChange had a special branch for newRole === 2 (ROUTER) that popped a native window.confirm() before applying the change:

if (newRole === 2) {
  const confirmed = window.confirm(t('admin_commands.router_mode_confirmation'));
  if (!confirmed) {
    setIsRoleDropdownOpen(false);
    return;   // <-- role never committed
  }
}
setDeviceConfig({ role: newRole });

If the user dismissed the dialog (Cancel, Escape, or accidental click — easy to do because the native modal feels disconnected from the custom in-page dropdown UX) the role state was never updated and the dropdown snapped back to the previous value. ROUTER is the only role with this gate, which is why Sensor/Tracker/etc. appeared to "work fine."

Fix

Replace the modal gate with a persistent inline warning banner:

  • AdminCommandsTab.tsx — drop the window.confirm() branch. handleRoleChange now always commits immediately, like every other role.
  • DeviceConfigurationSection.tsx — render a yellow warning banner directly under the role display whenever deviceRole === 2. It includes a ⚠️ icon and explains the routing implications. The banner stays visible until the user picks a different role, so the warning is reviewable before pressing "Save Device Config" (which is when the change actually goes to the node).
  • public/locales/en.json — new keys admin_commands.router_mode_warning_title and admin_commands.router_mode_warning. The old router_mode_confirmation key is left in place (unused) so non-English locale files aren't broken until translators update.

Why this UX is better

  • Selection ≠ commit: the rest of the form treats dropdown selection as staging local state; the actual write happens on Save. The old confirm violated that pattern.
  • Persistent warning is more discoverable than a one-shot dialog — a user re-opening the page can still see why Router is unusual.
  • Removes the dependency on a native dialog that some users were dismissing without realizing the role would silently drop.

Test plan

  • npx tsc --noEmit passes
  • Manually verify in dev container: pick Router → dropdown shows Router → warning banner appears below → press Save → device config sent to node
  • Pick Sensor (or any non-Router role) → no banner appears, behaves as before
  • Toggle back from Router to Client → banner disappears

🤖 Generated with Claude Code

The Remote Admin Device Configuration role dropdown popped a native
window.confirm() the moment a user picked ROUTER. If the user dismissed
the dialog — accidentally, via Escape, or because the native confirm
felt out of place inside the in-page dropdown UX — the role state was
never updated and the dropdown silently snapped back to the previous
value. Every other role (Sensor, Tracker, Client, etc.) committed
immediately, which made the symptom look ROUTER-specific.

Replace the confirm gate with a persistent inline warning banner under
the role display. Selection always commits to local form state now; the
warning explains the routing implications and stays visible until the
user picks a different role, so the user can review before pressing
"Save Device Config". New translation keys are added; the old
router_mode_confirmation key is retained (unused) so non-English
locales aren't broken until translators update.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 23, 2026

Claude finished @Yeraze's task —— View job


PR Review: Router Role Revert Fix

Todo List:

  • Read and analyze changed files
  • Review code quality and implementation approach
  • Check for potential bugs and security concerns
  • Evaluate UX improvements and best practices
  • Provide comprehensive feedback

Code Analysis

The PR successfully addresses the Router role reversion bug by replacing a blocking confirmation dialog with a persistent warning banner.

Key Changes:

  1. AdminCommandsTab.tsx - Simplified handleRoleChange by removing the Router-specific confirmation logic
  2. DeviceConfigurationSection.tsx - Added conditional warning banner that displays when deviceRole === 2 (Router)
  3. Localization - Added new warning text keys while preserving the old confirmation key

Code Quality & Implementation

✅ Excellent:

  • Clean removal of problematic code: The Router-specific confirmation logic was cleanly removed, making handleRoleChange consistent across all roles
  • Good separation of concerns: Warning display logic lives in the presentation component, not in the handler
  • Proper accessibility: Warning banner includes role="alert" and semantic structure
  • Consistent styling: Uses CSS custom properties and follows existing design patterns
  • Backward compatibility: Preserves old localization key to avoid breaking translations

✅ Good UX Design:

  • Persistent warning: Unlike the modal, the warning stays visible allowing users to review implications before saving
  • Clear visual hierarchy: Warning icon, title, and description are well-structured
  • Appropriate styling: Yellow warning color scheme and left border treatment follow design conventions

Security & Bug Analysis

✅ No Security Concerns:

  • No new attack vectors introduced
  • No sensitive data exposure
  • Role validation presumably happens server-side (as it should)

✅ No Apparent Bugs:

  • Logic is straightforward: show warning when deviceRole === 2, hide otherwise
  • No race conditions or edge cases identified
  • Proper conditional rendering with React patterns

UX Improvements & Best Practices

✅ Significant UX Enhancement:

  • Solves the core problem: Users can now successfully select Router without the role reverting
  • Better information architecture: Warning is contextual and persistent rather than interruptive
  • Consistent with platform patterns: Staging changes locally, then committing on "Save" is the expected pattern
  • Progressive disclosure: Warning only appears when relevant

✅ Follows React Best Practices:

  • Conditional rendering with proper JSX patterns
  • No unnecessary re-renders or performance issues
  • Clean component structure and prop passing

Performance Considerations

✅ Minimal Performance Impact:

  • Simple conditional rendering has negligible cost
  • No expensive operations or complex state changes
  • Warning text is pre-translated (no runtime translation overhead)

Test Coverage

⚠️ Areas for Consideration:
While no tests are included in this PR, the changes would benefit from:

  • Unit test verifying warning appears when deviceRole === 2
  • Unit test verifying warning disappears when role changes to non-Router
  • Integration test confirming role selection no longer reverts

Overall Assessment

🎉 Excellent Fix - This is a well-executed solution that:

  • Solves the user-reported problem completely
  • Improves the overall UX by removing a confusing modal
  • Maintains all the safety benefits of the original warning
  • Uses clean, maintainable code that follows project conventions
  • Includes proper accessibility considerations

Recommendation: Approve

The implementation is solid, the UX improvement is significant, and the code quality is high. This is exactly the kind of thoughtful user experience enhancement that makes software better to use.


@Yeraze Yeraze merged commit 6d0c15e into main May 23, 2026
19 checks passed
@Yeraze Yeraze mentioned this pull request May 23, 2026
4 tasks
Yeraze added a commit that referenced this pull request May 23, 2026
Patch bump covering recent fixes since 4.6.5:
- Router role no longer reverts on selection in Remote Admin (#3153)
- Auto-upgrade banner no longer persists after successful upgrade (#3155)
- Blog index full-width + post header (breadcrumbs, title, date, tags) (#3152)

Updated all five version-tracking files per CLAUDE.md:
- package.json
- package-lock.json (regenerated via npm install --package-lock-only --legacy-peer-deps)
- helm/meshmonitor/Chart.yaml (version + appVersion)
- desktop/src-tauri/tauri.conf.json
- desktop/package.json

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant