Add channel tui option - #9
Conversation
e6ddcb9 to
f3723d7
Compare
rominf
left a comment
There was a problem hiding this comment.
Review
Thanks for the PR — the toggle semantics, Release default, and correct threading of the channel value through to install sdk --channel are all sound. Tests are well updated and replacing handle_key(Up) navigation with direct select_onboarding_choice() calls is a clear improvement.
A few issues need addressing before merge.
Critical: channel label always shows "Release"
Both install_sdk_choice_label and onboarding_menu_label hardcode the string "Release" as the left-side label regardless of the selected channel:
InstallSdkChoice::Channel => format!(
"{:<14} {}",
"Release", // ← always "Release"
if matches!(channel, InstallSdkChannel::Release) {
"Stable (default)"
} else {
"Nightly (preview)" // when nightly: displays "Release Nightly (preview)"
}
),When the user switches to Nightly the row reads Release Nightly (preview) — contradictory. Compare with the Folder row: Install folder: /path/. The channel row label should be a neutral descriptor like "Channel:" so it stays coherent in both states: Channel: Nightly (preview).
toggle_install_sdk_channel updates status unconditionally
fn toggle_install_sdk_channel(&mut self) {
if let Some(state) = self.install_manager.as_mut()
&& let InstallManagerScreen::Sdk { channel, .. } = &mut state.screen
{
*channel = channel.toggle();
state.message = None;
state.detail_scroll = 0;
}
self.status = "ROCm channel changed. ...".to_owned(); // ← runs even when the if let fails
}If the install manager isn't on the SDK screen, the toggle silently does nothing but the status still claims the channel changed. Move self.status = inside the if let block.
Duplicate Left/Right arms in handle_onboarding_key
The Left and Right handlers for OnboardingMenuChoice::Channel are identical — same guard conditions, same two-line body. Either collapse them or extract a helper (consistent with how toggle_install_sdk_channel already abstracts the install manager side).
Minor
- Status messages use lowercase
"release is stable, nightly is preview."but UI labels use title-case"Stable (default)"/"Nightly (preview)". Align the casing. - Detail text says
"On the Release row, Enter or Left/Right switches channel."— will mislead users who have Nightly selected. Should be"On the Channel row, ...". - The
Char('2')test block is testing pre-existing number-key-jump behavior — a short comment explaining what'2'does and why the assertion follows would help future readers. - It's worth confirming the terminal height change from 32 to 36 rows in the install-failed render test reflects real additional content rather than masking an overflow.
|
All review comments have been addressed: Critical: channel label always shows "Release"
Duplicate Left/Right arms in Minor items:
|
…omments and test names - #9 bug: guard ttft_ms/tpot_ms with is_finite() in instances.rs (the shared Observe renderer), matching tokens_per_watt; NaN/Inf now render the em-dash, never 'NaNms'/'infms'. New render test proves it. - #9 test: observe table test now also asserts the TPOT value ('22ms'), not just TTFT ('150ms'). - #9 test: add efficiency::instance_power_w unit tests (match-sum, no-match -> None, empty -> None). - #8: reword Hardware/Instances/Bench 'tab' doc comments to 'Observe sub-panel'; 'Doctor overlay' -> 'examine overlay' (rocm examine job); initial_tab doc says Home (not removed Overview) for rocm dash. - #8: rename action_tab_renders_key_labels -> rocm_and_serving_tabs_render_verb_labels and slash_gpu_switches_to_hardware -> slash_gpu_switches_to_observe to match their assertions; update the referencing parity-map/checklist rows. Signed-off-by: Michael Roy <michael.roy@amd.com>
This pull request adds support for selecting the ROCm SDK installation channel (release or nightly) in both the onboarding and install manager flows of the TUI. Users can now switch between stable (release) and preview (nightly) channels via the UI, and the selected channel is used when installing the SDK. The UI, keyboard handling, and test code have all been updated to support this new capability.