oli/rdbms-playground
1
0
Fork 0
Code Issues 4 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

ADR-0022 stage 8 follow-up r2: completion UX fixes from real testing

Browse Source 
Two concrete behaviour changes from the user's second testing
round:

1. **Single vs multi commit paths.** Previously every Tab,
   even single-candidate, created a memo so Esc/Backspace could
   undo. The downside: with one candidate, repeated Tab "cycled"
   through the same item invisibly — looked stuck. Now:
   - Single candidate → insert with trailing space, no memo.
     The user can keep typing or hit Tab again to fresh-complete
     at the new cursor. (Trade-off: Esc/Backspace no longer
     whole-span undo for unique completions; the user accepted
     this for the chained-Tab fluency.)
   - Multi candidate → insert WITHOUT trailing space, create
     memo for cycling. The natural commit gesture is space —
     pressing it clears the memo and inserts the space normally,
     producing "<chosen> " ready for the next position.
   The "stuck on unique" symptom goes away, and the missing
   trailing space on multi-Tab signals "you're picking; press
   space when you're done" without needing modal affordances.

2. **Keyword candidates in grammar order.** Dropped the
   alphabetical sort in `describe_expected` in favour of
   chumsky's native source-order traversal of `or_not`/`choice`
   chains — empirically this matches the canonical command
   shape. Result: `add column ` now offers `to` before
   `table` (as `add column [to] [table] <Table>:…` reads),
   not `table` before `to` which previously suggested the
   nonsensical `add column table to ...`. Identifiers still
   alphabetised within their group; entry-keyword fallback
   for the no-prefix case stays alphabetical (no source order
   when 10 separate command branches).

Tests: 750 passing, 0 failing, 1 ignored (747 baseline →
+3 net: replaced single-candidate Esc/Backspace tests with
new multi-candidate variants; added the unique-Tab-chains-
naturally case that drove the round-2 fix; kept the
keywords-in-grammar-order test updated to assert
`to`/`table`/identifiers ordering).
This commit is contained in:
claude@clouddev1
2026-05-11 22:27:54 +00:00
parent bd1cce672d
commit f94a999e66
3 changed files with 189 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/app.rs
+165 -95
View File
@@ -639,16 +639,22 @@ impl App {
}
/// Tab key handler — insert / cycle forward through the
/// candidates at the cursor (ADR-0022 stage 8). Three
/// shapes:
/// - memo present: cycle to next candidate, replace
/// inserted_range, advance memo.selection_idx;
/// - memo absent + multi-candidate: insert first
/// candidate (with trailing space) and create memo;
/// - memo absent + single candidate: insert it and
/// create a one-element memo so Esc/Backspace can
/// still undo;
/// - no candidates: no-op.
/// candidates at the cursor (ADR-0022 stage 8). Behaviour
/// is split between single and multi-candidate cases per
/// the user's stage-8 feedback round 2:
///
/// - **Single candidate**: insert with trailing space,
/// no memo. The space is the natural commit; subsequent
/// Tab fresh-computes at the new cursor. Esc/Backspace
/// are normal.
/// - **Multi candidate**: insert WITHOUT trailing space,
/// create memo for cycling. Tab again cycles; any other
/// key clears the memo. Pressing space (the natural
/// "I'm done picking" gesture) clears the memo and adds
/// the space, completing the chosen candidate.
/// - **Memo present**: cycle to next candidate, replacing
/// the inserted text in place (still no trailing space).
/// - **No candidates**: no-op.
fn completion_tab_forward(&mut self) -> Vec<Action> {
self.cancel_history_navigation();
if let Some(memo) = self.last_completion.take() {
@@ -656,17 +662,14 @@ impl App {
self.last_completion = Some(self.replace_inserted(memo, next));
return Vec::new();
}
if let Some(memo) = self.start_completion(0) {
self.last_completion = Some(memo);
}
self.start_or_complete_at(0);
Vec::new()
}
/// Shift-Tab key handler — symmetric to
/// `completion_tab_forward` but cycles backward, and
/// starts from the last candidate (alphabetically) if no
/// memo exists. Per the user's #2: wrap-around at both
/// ends.
/// Shift-Tab key handler — symmetric to forward; on a
/// fresh multi-candidate position starts from the last
/// candidate (per the user's #2 wrap-from-both-ends).
/// Single candidate behaves identically to Tab.
fn completion_tab_backward(&mut self) -> Vec<Action> {
self.cancel_history_navigation();
if let Some(memo) = self.last_completion.take() {
@@ -674,16 +677,17 @@ impl App {
self.last_completion = Some(self.replace_inserted(memo, prev));
return Vec::new();
}
if let Some(memo) = self.start_completion_last() {
self.last_completion = Some(memo);
}
self.start_or_complete_last();
Vec::new()
}
/// Esc / Backspace handler while a completion memo is
/// alive — restore the original text in
/// `inserted_range` and place the cursor where the user
/// was when they hit Tab. The memo is cleared.
/// alive — restore the original text in `inserted_range`
/// and place the cursor where the user was when they hit
/// Tab. The memo is cleared. Only fires on multi-candidate
/// completions (single-candidate paths don't create a
/// memo); the user accepts that single-candidate Tab
/// requires regular backspace to undo.
fn undo_last_completion(&mut self) {
let Some(memo) = self.last_completion.take() else {
return;
@@ -693,60 +697,87 @@ impl App {
self.input_cursor = start + memo.original_text.len();
}
/// Compute the candidates at the cursor and insert the
/// candidate at index `start_idx` (with a trailing space).
/// Returns the freshly-built memo. `None` if there are no
/// candidates to insert.
fn start_completion(&mut self, start_idx: usize) -> Option<crate::completion::LastCompletion> {
fn start_or_complete_at(&mut self, multi_start_idx: usize) {
let cursor = self.input_cursor.min(self.input.len());
let comp = crate::completion::candidates_at_cursor(
let Some(comp) = crate::completion::candidates_at_cursor(
&self.input,
cursor,
&self.schema_cache,
)?;
let idx = start_idx % comp.candidates.len();
let inserted = format!("{} ", comp.candidates[idx].text);
) else {
return;
};
if comp.candidates.len() == 1 {
self.commit_unique(&comp);
} else {
let idx = multi_start_idx % comp.candidates.len();
self.last_completion = Some(self.commit_multi(comp, idx));
}
}
fn start_or_complete_last(&mut self) {
let cursor = self.input_cursor.min(self.input.len());
let Some(comp) = crate::completion::candidates_at_cursor(
&self.input,
cursor,
&self.schema_cache,
) else {
return;
};
if comp.candidates.len() == 1 {
self.commit_unique(&comp);
} else {
let idx = comp.candidates.len() - 1;
self.last_completion = Some(self.commit_multi(comp, idx));
}
}
/// Single-candidate commit: insert "<text> " (with trailing
/// space) and DO NOT create a memo. The user can keep
/// typing or press Tab again to fresh-complete at the new
/// cursor.
fn commit_unique(&mut self, comp: &crate::completion::Completion) {
let inserted = format!("{} ", comp.candidates[0].text);
self.input
.replace_range(comp.replaced_range.0..comp.replaced_range.1, &inserted);
self.input_cursor = comp.replaced_range.0 + inserted.len();
}
/// Multi-candidate commit: insert just the candidate text
/// (no trailing space) and return the memo carrying the
/// full candidate list for cycling. The user presses space
/// (or any other non-Tab key) to commit the choice — that
/// clears the memo and inserts whatever they typed
/// normally, naturally producing "<chosen> " as the
/// completed text.
fn commit_multi(
&mut self,
comp: crate::completion::Completion,
idx: usize,
) -> crate::completion::LastCompletion {
let inserted = comp.candidates[idx].text.clone();
let original_text =
self.input[comp.replaced_range.0..comp.replaced_range.1].to_string();
self.input
.replace_range(comp.replaced_range.0..comp.replaced_range.1, &inserted);
let new_end = comp.replaced_range.0 + inserted.len();
self.input_cursor = new_end;
Some(crate::completion::LastCompletion {
crate::completion::LastCompletion {
inserted_range: (comp.replaced_range.0, new_end),
original_text,
candidates: comp.candidates,
selection_idx: idx,
})
}
}
/// Same as `start_completion` but starts from the last
/// candidate alphabetically (for Shift-Tab from a fresh
/// state).
fn start_completion_last(&mut self) -> Option<crate::completion::LastCompletion> {
// We need to know how many candidates there will be
// before we can pick "last". Compute the completion
// first, then call start_completion with that index.
let cursor = self.input_cursor.min(self.input.len());
let comp = crate::completion::candidates_at_cursor(
&self.input,
cursor,
&self.schema_cache,
)?;
let last = comp.candidates.len() - 1;
// Fall through to the same path so the inserted text
// and memo construction stay in one place.
self.start_completion(last)
}
/// Replace the inserted text with `candidates[idx]` and
/// Replace the inserted text with `candidates[idx]` (no
/// trailing space — same multi-candidate convention) and
/// return an updated memo. Used by Tab/Shift-Tab cycling.
fn replace_inserted(
&mut self,
memo: crate::completion::LastCompletion,
idx: usize,
) -> crate::completion::LastCompletion {
let new_inserted = format!("{} ", memo.candidates[idx].text);
let new_inserted = memo.candidates[idx].text.clone();
let (start, end) = memo.inserted_range;
self.input.replace_range(start..end, &new_inserted);
let new_end = start + new_inserted.len();
@@ -1776,25 +1807,30 @@ mod tests {
// ---- ADR-0022 stage 8: Tab completion + Esc/Backspace undo ----
#[test]
fn tab_inserts_unique_keyword_with_trailing_space() {
fn tab_with_unique_candidate_inserts_with_space_and_no_memo() {
// Single-candidate path: insert "<text> ", no memo.
// Stage-8 follow-up #2 (testing-round-2): no memo
// for unique completions so subsequent Tab fresh-
// computes at the new cursor.
let mut app = App::new();
type_str(&mut app, "cre");
let actions = app.update(key(KeyCode::Tab));
assert!(actions.is_empty());
assert_eq!(app.input, "create ");
assert_eq!(app.input_cursor, 7);
// Memo is alive even for a single-candidate completion
// so Esc/Backspace can still undo.
assert!(app.last_completion.is_some());
assert!(app.last_completion.is_none());
}
#[test]
fn tab_at_word_boundary_inserts_next_expected_keyword() {
// `create ` → expects only `table`. Single candidate;
// insert "table " with space, no memo.
let mut app = App::new();
type_str(&mut app, "create ");
let actions = app.update(key(KeyCode::Tab));
assert!(actions.is_empty());
assert_eq!(app.input, "create table ");
assert!(app.last_completion.is_none());
}
#[test]
@@ -1810,70 +1846,89 @@ mod tests {
}
#[test]
fn tab_cycles_forward_through_multi_candidate_set() {
// After `show ` two candidates: data, table.
fn tab_with_multi_candidates_inserts_without_space_and_creates_memo() {
// Multi-candidate path: insert WITHOUT trailing space
// so the user can press space to commit. Memo carries
// the candidate list for cycling.
let mut app = App::new();
type_str(&mut app, "show ");
app.update(key(KeyCode::Tab));
assert_eq!(app.input, "show data ");
assert_eq!(app.input, "show data");
assert!(app.last_completion.is_some());
}
#[test]
fn tab_cycles_forward_through_multi_candidate_set() {
let mut app = App::new();
type_str(&mut app, "show ");
app.update(key(KeyCode::Tab));
assert_eq!(app.input, "show table ");
// Wrap-around per the user's #2.
assert_eq!(app.input, "show data");
app.update(key(KeyCode::Tab));
assert_eq!(app.input, "show data ");
assert_eq!(app.input, "show table");
// Wrap-around.
app.update(key(KeyCode::Tab));
assert_eq!(app.input, "show data");
}
#[test]
fn shift_tab_cycles_backward_starting_from_last() {
// Fresh Shift-Tab on multi-candidate position starts
// at the last candidate alphabetically.
let mut app = App::new();
type_str(&mut app, "show ");
app.update(key(KeyCode::BackTab));
assert_eq!(app.input, "show table ");
assert_eq!(app.input, "show table");
app.update(key(KeyCode::BackTab));
assert_eq!(app.input, "show data ");
// Wrap.
assert_eq!(app.input, "show data");
app.update(key(KeyCode::BackTab));
assert_eq!(app.input, "show table ");
assert_eq!(app.input, "show table");
}
#[test]
fn esc_after_tab_restores_original_in_one_keystroke() {
fn space_after_multi_candidate_tab_commits_the_choice() {
// The natural commit gesture for multi-candidate Tab:
// press space. Memo clears (any non-Tab key clears),
// space is inserted normally → "show data " ready
// for the next position.
let mut app = App::new();
type_str(&mut app, "cre");
app.update(key(KeyCode::Tab));
assert_eq!(app.input, "create ");
app.update(key(KeyCode::Esc));
assert_eq!(app.input, "cre");
assert_eq!(app.input_cursor, 3);
type_str(&mut app, "show ");
app.update(key(KeyCode::Tab)); // → "show data" (no space)
type_str(&mut app, " ");
assert_eq!(app.input, "show data ");
assert!(app.last_completion.is_none());
}
#[test]
fn backspace_after_tab_restores_original_in_one_keystroke() {
// Symmetry: one keystroke to insert, one to remove.
fn esc_after_multi_tab_restores_original_in_one_keystroke() {
// Multi-candidate Tab leaves a memo; Esc undoes the
// whole insertion regardless of cycle depth.
let mut app = App::new();
type_str(&mut app, "cre");
type_str(&mut app, "show ");
app.update(key(KeyCode::Tab)); // → "show data"
app.update(key(KeyCode::Esc));
assert_eq!(app.input, "show ");
assert!(app.last_completion.is_none());
}
#[test]
fn backspace_after_multi_tab_restores_original_in_one_keystroke() {
let mut app = App::new();
type_str(&mut app, "show ");
app.update(key(KeyCode::Tab));
assert_eq!(app.input, "create ");
app.update(key(KeyCode::Backspace));
assert_eq!(app.input, "cre");
assert_eq!(app.input_cursor, 3);
assert_eq!(app.input, "show ");
assert!(app.last_completion.is_none());
}
#[test]
fn esc_after_multiple_tabs_restores_original_state_not_previous_cycle() {
// Tab Tab Tab cycled through three candidates; Esc
// restores the pre-completion state (the original
// partial prefix), not the previous cycle.
// restores the pre-completion state (no insertion at
// all), not the previous cycle.
let mut app = App::new();
type_str(&mut app, "drop ");
app.update(key(KeyCode::Tab)); // column
app.update(key(KeyCode::Tab)); // relationship
app.update(key(KeyCode::Tab)); // table
assert_eq!(app.input, "drop table ");
assert_eq!(app.input, "drop table");
app.update(key(KeyCode::Esc));
assert_eq!(app.input, "drop ");
}
@@ -1881,31 +1936,46 @@ mod tests {
#[test]
fn typing_a_letter_clears_the_completion_memo() {
let mut app = App::new();
type_str(&mut app, "cre");
type_str(&mut app, "show ");
app.update(key(KeyCode::Tab));
assert!(app.last_completion.is_some());
// Typing any non-Tab key clears the memo. The
// inserted text stays in the buffer.
type_str(&mut app, "x");
assert_eq!(app.input, "create x");
assert_eq!(app.input, "show datax");
assert!(app.last_completion.is_none());
// Backspace now does its normal job — delete one char.
app.update(key(KeyCode::Backspace));
assert_eq!(app.input, "create ");
assert_eq!(app.input, "show data");
}
#[test]
fn cursor_movement_clears_the_completion_memo() {
let mut app = App::new();
type_str(&mut app, "cre");
type_str(&mut app, "show ");
app.update(key(KeyCode::Tab));
// Per the user's #3 — cursor movement clears the
// memo. After this, Esc / Backspace behave normally
// and don't trigger the whole-span undo.
// Cursor movement clears the memo. After this,
// Esc / Backspace behave normally — no whole-span
// undo.
app.update(key(KeyCode::Home));
assert!(app.last_completion.is_none());
}
#[test]
fn unique_tab_then_another_unique_tab_chains_naturally() {
// Stage-8 follow-up #2 (testing-round-2): the
// single-candidate-no-memo design lets the user chain
// Tabs through unique completions without getting
// stuck. From "a", Tab → "add ", Tab → "add column ".
let mut app = App::new();
type_str(&mut app, "a");
app.update(key(KeyCode::Tab));
assert_eq!(app.input, "add ");
app.update(key(KeyCode::Tab));
assert_eq!(app.input, "add column ");
assert!(app.last_completion.is_none());
}
fn sample_description(name: &str) -> TableDescription {
TableDescription {
name: name.to_string(),
src/completion.rs
+16 -12
View File
@@ -126,15 +126,18 @@ pub fn candidates_at_cursor(
let matches_prefix = |s: &str| s.to_lowercase().starts_with(&lowered_prefix);
// Source 1: keyword candidates from the parser's
// expected-set.
// expected-set. Preserve `expected`'s order — it reflects
// chumsky's source-order traversal of `or_not` / `choice`
// chains, which matches the canonical command shape (e.g.
// `to` before `table` for `add column [to] [table] …`).
let mut keywords: Vec<String> = expected
.iter()
.filter_map(|item| strip_backticks(item))
.filter_map(|name| Keyword::from_word(name).map(|_| name.to_string()))
.filter(|name| matches_prefix(name))
.collect();
keywords.sort();
keywords.dedup();
let mut seen_kw = std::collections::HashSet::new();
keywords.retain(|k| seen_kw.insert(k.clone()));
// Source 2: schema identifiers — accumulated across every
// matching known-set slot. `NewName` slots return `&[]`.
@@ -444,24 +447,25 @@ mod tests {
}
#[test]
fn keywords_come_before_identifiers_in_candidate_order() {
// "add column " has both keyword candidates (`to`,
// `table`) and schema-identifier candidates. Per the
// user feedback after stage 8: keywords first
// (alphabetical within), identifiers second
// (alphabetical within).
fn keywords_come_before_identifiers_in_grammar_order() {
// "add column " has both keyword candidates and
// schema-identifier candidates. Per the user's stage-8
// feedback round 2: keywords first in *grammar order*
// (so `to` before `table` because the canonical shape
// is `add column [to] [table] <Table>:…`), identifiers
// after, alphabetised. The grammar order falls out of
// chumsky's source-order expected-set traversal — we
// preserve that order through `describe_expected`.
let cache = SchemaCache {
tables: vec!["Customers".to_string(), "Orders".to_string()],
..SchemaCache::default()
};
let kinds = cand_kinds_with("add column ", 11, &cache);
// Expect: ["table", "to"] keywords, then ["Customers",
// "Orders"] identifiers.
assert_eq!(
kinds,
vec![
("table".to_string(), CandidateKind::Keyword),
("to".to_string(), CandidateKind::Keyword),
("table".to_string(), CandidateKind::Keyword),
("Customers".to_string(), CandidateKind::Identifier),
("Orders".to_string(), CandidateKind::Identifier),
],
src/dsl/parser.rs
+8 -2
View File
@@ -765,8 +765,14 @@ fn describe_expected(expected: &[RichPattern<'_, Token>]) -> Vec<String> {
})
.map(describe_pattern)
.collect();
items.sort();
items.dedup();
// Dedup preserving first occurrence (which reflects
// chumsky's traversal order — typically source order for
// `or_not` / `choice` chains). Empirically this gives a
// grammar-natural ordering: `to` before `table` in
// `add column [to] [table] …`, which alphabetical
// (table, to) would invert.
let mut seen = std::collections::HashSet::new();
items.retain(|s| seen.insert(s.clone()));
items
}