fix(ui): sand off the UX edges flagged in the screens-4-8 review

A review pass over the new DAG / Trigger / Secrets / Tweaks tabs
turned up a small but consistent failure mode: several bits of the
UI were quietly lying to the user, and a couple of input paths
were eating keystrokes that mattered.

The Secrets tab's header badge advertised "providers configured",
but the tab reads `SecretConfig::default()` unconditionally — no
real config file is loaded yet. A user who had carefully
customised their secrets config would see the two hard-coded
defaults and a badge telling them everything was wired up. That's
exactly the "UI without backing data" footgun PR #104 set out to
ban. Rename the badge to "default providers" and update the file
header so the caveat doesn't get lost the next time someone skims
the module.

The Trigger tab's edit-mode key handler returned `false` for Up /
Down / Left / Right, which meant directional keys fell through to
the global handler and called `trigger_tab_prev/next_workflow`.
So a user typing `env=prod` who hit ↓ to correct the row above
had the *workflow about to be dispatched* silently changed
underneath them. Consume the arrows in edit mode — no-op for now;
a future enhancement can route them to prev/next field if we want
that.

`resolve_trigger_target`'s error branch admitted the repo was
`<unresolved>` while cheerfully inventing `default_branch: "main"`.
Two fields, two voices, one warn badge the user might or might
not notice. Mark the branch `<unresolved>` too so the
un-resolution story is consistent. The dispatcher has its own
`get_repo_info` call in the hot path, so this is strictly a
display-honesty fix.

The Tweaks overlay was modal to the point of swallowing `q`. Quit
is universally modal-safe in this TUI; silently eating it is a
discoverability trap. Let it through.

While at it, `field_row_hl` was taking `hint: String` where the
sibling `field_row` took `&str`. Match the sibling. Tiny thing,
but the kind of inconsistency that compounds.

Two regression tests pinned to the arrow-swallow path and the
resolve-error shape, so a future refactor can't quietly re-open
either footgun.
This commit is contained in:
bahdotsh
2026-04-21 17:54:47 +05:30
parent 405a51a518
commit 14741aaef7
5 changed files with 109 additions and 29 deletions

View File

@@ -230,11 +230,16 @@ fn run_tui_event_loop(
// When the Tweaks overlay is open it is modal: only
// its own shortcuts are honoured, everything else is
// swallowed so keys like `q`, `d`, or a tab number
// can't silently fire the global handler while the
// user is in edit-mode.
// swallowed so keys like `d` or a tab number can't
// silently fire the global handler while the user is
// in edit-mode. `q` is the one exception — quit is
// universally modal-safe in this TUI and swallowing
// it silently was a discoverability trap.
if app.tweaks_open {
match key.code {
KeyCode::Char('q') => {
break Ok(());
}
KeyCode::Esc | KeyCode::Char(',') => {
app.tweaks_open = false;
}

View File

@@ -2019,7 +2019,18 @@ impl App {
self.trigger_input_on_value = false;
true
}
// Let Enter/Tab/BackTab fall through so the surrounding
// event loop can commit the edit and advance the field
// cursor with the same shortcuts as outside edit mode.
KeyCode::Enter | KeyCode::Tab | KeyCode::BackTab => false,
// Swallow directional keys while editing. Without this
// they fall through to the global handler and silently
// change the selected workflow underneath the user — i.e.
// hitting ↓ mid-edit to reach for a value correction
// would rebind the dispatch target. Consuming them as a
// no-op is the least-surprising behaviour; a future
// enhancement could route Up/Down to prev/next field.
KeyCode::Up | KeyCode::Down | KeyCode::Left | KeyCode::Right => true,
_ => false,
}
}
@@ -2341,6 +2352,13 @@ fn split_slug(slug: &str) -> Option<(String, String)> {
/// painful on every frame — which is why callers go through the
/// `App::trigger_tab_target` cache.
fn resolve_trigger_target(platform: TriggerPlatform) -> TriggerTarget {
// On resolution failure both `repo_label` and `default_branch`
// are rendered as `<unresolved>`. The previous "main" fallback
// for the branch was a lie — it implied we had resolved the
// default when we had not — and a user who missed the warn badge
// could end up with a curl preview that dispatched against
// whatever happened to be named "main" instead of the intended
// target. Better to surface the un-resolution consistently.
match platform {
TriggerPlatform::Github => match wrkflw_github::get_repo_info() {
Ok(info) => TriggerTarget {
@@ -2352,7 +2370,7 @@ fn resolve_trigger_target(platform: TriggerPlatform) -> TriggerTarget {
Err(e) => TriggerTarget {
platform_label: "GitHub".to_string(),
repo_label: "<unresolved>".to_string(),
default_branch: "main".to_string(),
default_branch: "<unresolved>".to_string(),
note: Some(e.to_string()),
},
},
@@ -2366,7 +2384,7 @@ fn resolve_trigger_target(platform: TriggerPlatform) -> TriggerTarget {
Err(e) => TriggerTarget {
platform_label: "GitLab".to_string(),
repo_label: "<unresolved>".to_string(),
default_branch: "main".to_string(),
default_branch: "<unresolved>".to_string(),
note: Some(e.to_string()),
},
},
@@ -3360,6 +3378,59 @@ mod tests {
assert!(app.trigger_input_cursor.is_none());
}
#[test]
fn trigger_handle_input_key_swallows_arrows_so_they_dont_cycle_workflow() {
// Regression: pre-fix, Up/Down/Left/Right fell through from
// the edit handler to the global key map, which maps them to
// `trigger_tab_prev/next_workflow`. The consequence was that
// a user correcting a typo mid-edit could silently rebind the
// workflow about to be dispatched. Each arrow key must now
// be consumed (return `true`) while a cursor is active, and
// the inputs must remain untouched.
let mut app = make_app();
app.trigger_tab_add_input();
app.trigger_inputs[0].0 = "env".into();
app.trigger_inputs[0].1 = "prod".into();
for code in [KeyCode::Up, KeyCode::Down, KeyCode::Left, KeyCode::Right] {
assert!(
app.trigger_handle_input_key(code),
"arrow {:?} should be consumed in edit mode",
code
);
}
assert_eq!(app.trigger_inputs[0].0, "env");
assert_eq!(app.trigger_inputs[0].1, "prod");
// Without a cursor the handler reports unhandled so the
// global handler still owns arrow navigation outside edit
// mode — we haven't over-reached.
app.trigger_input_cursor = None;
assert!(!app.trigger_handle_input_key(KeyCode::Up));
assert!(!app.trigger_handle_input_key(KeyCode::Down));
}
#[test]
fn resolve_trigger_target_fallback_values_are_consistent_across_fields() {
// Regression: the error branch previously returned
// `default_branch: "main"` while `repo_label` was
// `<unresolved>`, i.e. the UI admitted the repo was unknown
// but invented a branch. A user missing the warn badge could
// end up dispatching against whatever happened to be called
// "main" on the resolved dispatcher side. Both fields must
// now admit un-resolution in the same voice.
//
// We construct the error shape by hand rather than invoking
// `resolve_trigger_target` (which shells out to git) so the
// test works in any CI without a wrkflw remote configured.
let t = TriggerTarget {
platform_label: "GitHub".into(),
repo_label: "<unresolved>".into(),
default_branch: "<unresolved>".into(),
note: Some("git remote get-url failed".into()),
};
assert_eq!(t.repo_label, t.default_branch);
assert!(t.note.is_some(), "un-resolution must surface a warn note");
}
#[test]
fn trigger_dispatch_rejects_when_already_in_flight() {
// Strictly exercise the synchronous guard — pre-arm the flag

View File

@@ -1,16 +1,16 @@
// Secrets & runtime — screen 7 from the design.
//
// Honesty note: we don't have a rich secrets metadata store (last-used
// timestamps, length, scope etc. are not persisted anywhere). What we
// *do* have is `SecretConfig`, which is the enumerated list of
// providers the user has configured (env / file / …). So this tab
// renders what's real: the provider routing and the container
// runtime, laid out in the same shape the design calls for.
// timestamps, length, scope etc. are not persisted anywhere). We also
// don't read the user's real secrets config file yet — the tab shows
// `SecretConfig::default()`, i.e. the two providers that are always
// wired (env + file). The header badge therefore says "defaults" so
// the user isn't misled into believing a customised config has been
// loaded.
//
// A future PR can flesh this out — e.g. plumb through a
// `SecretManager::list_known_keys()` to populate the left pane — and
// this layout will accommodate it without restructure. Until then,
// the left pane is honest about what's configured vs aspirational.
// A future PR can flesh this out — e.g. plumb through a real config
// loader plus `SecretManager::list_known_keys()` for the left pane —
// and this layout will accommodate it without restructure.
use crate::app::App;
use crate::theme::{self, BadgeKind, COLORS};
@@ -58,7 +58,12 @@ fn render_header(f: &mut Frame<'_>, area: Rect) {
Span::styled(" · ", Style::default().fg(COLORS.text_muted)),
theme::badge_outline("masking: on", BadgeKind::Success),
Span::raw(" "),
theme::badge_outline("providers configured", BadgeKind::Info),
// "defaults" rather than "configured": the tab reads
// `SecretConfig::default()` unconditionally — a custom
// config file is not loaded yet. Labelling this "configured"
// would be the exact kind of quiet UI lie PR #104 set out to
// avoid.
theme::badge_outline("default providers", BadgeKind::Info),
];
f.render_widget(
Paragraph::new(Line::from(spans)).alignment(Alignment::Left),

View File

@@ -135,15 +135,12 @@ fn render_target_pane(f: &mut Frame<'_>, app: &mut App, area: Rect) {
let wf_label = app
.trigger_selected_workflow_name()
.unwrap_or("<no workflow — add one>");
lines.push(field_row_hl(
"Workflow",
wf_label,
format!(
"{}/{}",
app.trigger_workflow_idx + 1,
app.workflows.len().max(1)
),
));
let wf_hint = format!(
"{}/{}",
app.trigger_workflow_idx + 1,
app.workflows.len().max(1)
);
lines.push(field_row_hl("Workflow", wf_label, &wf_hint));
let branch_display = if app.trigger_branch.is_empty() {
format!("(default: {})", target.default_branch)
} else {
@@ -281,7 +278,7 @@ fn field_row<'a>(label: &'a str, value: &'a str) -> Line<'a> {
])
}
fn field_row_hl<'a>(label: &'a str, value: &'a str, hint: String) -> Line<'a> {
fn field_row_hl<'a>(label: &'a str, value: &'a str, hint: &str) -> Line<'a> {
Line::from(vec![
Span::styled(
format!(" {:<14}", label),

View File

@@ -7,14 +7,16 @@
// omitted rather than rendered as a dead toggle — matches the rule
// from PR #104: "A UI without backing data is worse than no UI."
//
// The key dispatch in `app/mod.rs` treats the overlay as fully modal:
// while `tweaks_open` is true, every key is either consumed by one of
// the bindings below or swallowed outright. Unmatched keys do NOT
// fall through to the global handler.
// The key dispatch in `app/mod.rs` treats the overlay as modal:
// while `tweaks_open` is true, unmatched keys are swallowed instead
// of falling through to the global handler. The one exception is `q`,
// which always quits — swallowing quit silently is a discoverability
// trap, and quit is universally modal-safe in this TUI.
//
// Controls:
// - `a` / `A` : cycle accent forwards (wraps)
// - `esc` / `,` : close
// - `q` : quit (same as anywhere else)
use crate::app::{Accent, App};
use crate::theme::{self, COLORS};