Skip to content

refactor(tui): extract shared setting edit overlay#1776

Merged
TaylorMutch merged 1 commit into
NVIDIA:mainfrom
ericcurtin:refactor/deduplicate-tui-overlays
Jun 5, 2026
Merged

refactor(tui): extract shared setting edit overlay#1776
TaylorMutch merged 1 commit into
NVIDIA:mainfrom
ericcurtin:refactor/deduplicate-tui-overlays

Conversation

@ericcurtin

Copy link
Copy Markdown
Contributor

Summary

The `draw_edit_overlay` function in `sandbox_settings.rs` and `global_settings.rs` was nearly identical — the only difference was which settings slice the entry was resolved from (`app.sandbox_settings` vs `app.global_settings`).

Extract the common rendering logic into a single `draw_setting_edit_overlay` function in `ui/mod.rs`, parameterized by `key`, `kind`, `edit`, `area`, and `theme`. Each pane now resolves its own entry and delegates to the shared implementation.

Related Issue

N/A — identified via codebase duplication audit.

Changes

  • Added `draw_setting_edit_overlay` to `ui/mod.rs` taking `(key, kind, edit, area, theme)` as parameters
  • Replaced the ~38-line duplicate bodies in `sandbox_settings.rs` and `global_settings.rs` with 4-line delegating wrappers
  • Added required `Clear`, `Padding` widget imports and `SettingEditState`, `Theme` type imports to `ui/mod.rs`

Net diff: -75 lines, +61 lines across the three files.

Testing

  • `cargo check -p openshell-tui` passes
  • `cargo clippy --workspace --all-targets -- -D warnings` passes (no new warnings)
  • `cargo test -p openshell-tui` passes (22 tests, 0 failures)
  • `cargo fmt --all -- --check` passes
  • License header check passes
  • E2E tests added/updated (not applicable — UI rendering only)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)

The draw_edit_overlay function in sandbox_settings and global_settings
was nearly identical — the only difference was which settings slice the
entry was resolved from (app.sandbox_settings vs app.global_settings).

Extract the common rendering logic into draw_setting_edit_overlay in
ui/mod.rs, taking key/kind/edit/area/theme as parameters. Each pane
now resolves its own entry and delegates to the single implementation.

Net: -75 lines, +61 lines across the three files.
@copy-pr-bot

copy-pr-bot Bot commented Jun 5, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@russellb

russellb commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

agent review:

No actionable findings.

I reviewed NVIDIA/OpenShell#1776 as a code review. The refactor preserves the existing behavior: both settings panes already aliased centered_popup as centered_rect, so the shared helper’s direct centered_popup(50, ...) call is equivalent. The parent-module private helper is accessible from child modules, and SettingValueKind is Copy, so the delegated calls compile cleanly.

I also tested it manually and both affected code paths still look fine in the TUI.

lgtm.

@TaylorMutch

Copy link
Copy Markdown
Collaborator

/ok to test baf8eb4

@TaylorMutch TaylorMutch merged commit 35afcf8 into NVIDIA:main Jun 5, 2026
28 checks passed
@ericcurtin ericcurtin deleted the refactor/deduplicate-tui-overlays branch June 5, 2026 20:33
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.

3 participants