From 23c7c530e38bcc9b3039e16a126276a1c73cc874 Mon Sep 17 00:00:00 2001
From: Edward Langley <el-github@elangley.org>
Date: Wed, 15 Apr 2026 21:32:34 -0700
Subject: [PATCH 1/5] refactor(parser): simplify tests and generator logic

Refactor formula parser tests to use more concise assert!(matches!(...))
syntax. Simplify the formula generator implementation by removing unused
expression variants and using expect() for mandatory grammar rules. Add a
regression test for hyphenated identifiers in bare names.

Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-26B-A4B-it-UD-Q5_K_XL.gguf)
---
 crates/improvise-formula/src/parser.rs | 306 +++++++++++++------------
 1 file changed, 158 insertions(+), 148 deletions(-)

diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs
index 51cb101..b76506f 100644
--- a/crates/improvise-formula/src/parser.rs
+++ b/crates/improvise-formula/src/parser.rs
@@ -385,13 +385,13 @@ mod tests {
     #[test]
     fn parse_sum_with_inline_where_filter() {
         let f = parse_formula("EastTotal = SUM(Revenue WHERE Region = \"East\")", "Foo").unwrap();
-        if let Expr::Agg(AggFunc::Sum, inner, Some(filter)) = &f.expr {
-            assert!(matches!(**inner, Expr::Ref(_)));
-            assert_eq!(filter.category, "Region");
-            assert_eq!(filter.item, "East");
-        } else {
-            panic!("Expected SUM with inline WHERE filter, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::Agg(AggFunc::Sum, inner, Some(filter))
+                if matches!(**inner, Expr::Ref(_))
+                    && filter.category == "Region"
+                    && filter.item == "East"
+        ));
     }
 
     // ── Comparison operators ────────────────────────────────────────────
@@ -467,21 +467,20 @@ mod tests {
     fn parse_aggregate_name_without_parens_is_ref() {
         // "SUM" without parens should be treated as a reference, not a function
         let f = parse_formula("X = SUM + 1", "Cat").unwrap();
-        assert!(matches!(f.expr, Expr::BinOp(BinOp::Add, _, _)));
-        if let Expr::BinOp(_, lhs, _) = &f.expr {
-            assert!(matches!(**lhs, Expr::Ref(_)));
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Add, lhs, _) if matches!(**lhs, Expr::Ref(_))
+        ));
     }
 
     #[test]
     fn parse_if_without_parens_is_ref() {
         // "IF" without parens should be treated as a reference
         let f = parse_formula("X = IF + 1", "Cat").unwrap();
-        if let Expr::BinOp(BinOp::Add, lhs, _) = &f.expr {
-            assert!(matches!(**lhs, Expr::Ref(_)));
-        } else {
-            panic!("Expected BinOp(Add), got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Add, lhs, _) if matches!(**lhs, Expr::Ref(_))
+        ));
     }
 
     // ── Quoted string in WHERE ──────────────────────────────────────────
@@ -540,46 +539,46 @@ mod tests {
     fn pipe_quoted_identifier_in_expression() {
         let f = parse_formula("|Total Revenue| = |Base Revenue| + Bonus", "Foo").unwrap();
         assert_eq!(f.target, "|Total Revenue|");
-        if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr {
-            assert!(matches!(**lhs, Expr::Ref(ref s) if s == "Base Revenue"));
-            assert!(matches!(**rhs, Expr::Ref(ref s) if s == "Bonus"));
-        } else {
-            panic!("Expected Add, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Add, lhs, rhs)
+                if matches!(**lhs, Expr::Ref(ref s) if s == "Base Revenue")
+                    && matches!(**rhs, Expr::Ref(ref s) if s == "Bonus")
+        ));
     }
 
     #[test]
     fn pipe_quoted_keyword_as_identifier() {
         // A category named "WHERE" can be referenced with pipes
         let f = parse_formula("X = |WHERE| + |SUM|", "Cat").unwrap();
-        if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr {
-            assert!(matches!(**lhs, Expr::Ref(ref s) if s == "WHERE"));
-            assert!(matches!(**rhs, Expr::Ref(ref s) if s == "SUM"));
-        } else {
-            panic!("Expected Add, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Add, lhs, rhs)
+                if matches!(**lhs, Expr::Ref(ref s) if s == "WHERE")
+                    && matches!(**rhs, Expr::Ref(ref s) if s == "SUM")
+        ));
     }
 
     #[test]
     fn pipe_quoted_identifier_with_special_chars() {
         // Pipes allow characters that would normally break tokenization
         let f = parse_formula("X = |Revenue (USD)| + |Cost + Tax|", "Cat").unwrap();
-        if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr {
-            assert!(matches!(**lhs, Expr::Ref(ref s) if s == "Revenue (USD)"));
-            assert!(matches!(**rhs, Expr::Ref(ref s) if s == "Cost + Tax"));
-        } else {
-            panic!("Expected Add, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Add, lhs, rhs)
+                if matches!(**lhs, Expr::Ref(ref s) if s == "Revenue (USD)")
+                    && matches!(**rhs, Expr::Ref(ref s) if s == "Cost + Tax")
+        ));
     }
 
     #[test]
     fn pipe_quoted_in_aggregate() {
         let f = parse_formula("X = SUM(|Net Revenue|)", "Cat").unwrap();
-        if let Expr::Agg(AggFunc::Sum, inner, None) = &f.expr {
-            assert!(matches!(**inner, Expr::Ref(ref s) if s == "Net Revenue"));
-        } else {
-            panic!("Expected SUM aggregate, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::Agg(AggFunc::Sum, inner, None)
+                if matches!(**inner, Expr::Ref(ref s) if s == "Net Revenue")
+        ));
     }
 
     #[test]
@@ -593,12 +592,11 @@ mod tests {
     fn pipe_quoted_in_inline_where() {
         let f =
             parse_formula("X = SUM(Revenue WHERE |Region Name| = |East Coast|)", "Foo").unwrap();
-        if let Expr::Agg(AggFunc::Sum, _, Some(filter)) = &f.expr {
-            assert_eq!(filter.category, "Region Name");
-            assert_eq!(filter.item, "East Coast");
-        } else {
-            panic!("Expected SUM with WHERE filter, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::Agg(AggFunc::Sum, _, Some(filter))
+                if filter.category == "Region Name" && filter.item == "East Coast"
+        ));
     }
 
     // ── Pipe-quoted escape semantics ────────────────────────────────────
@@ -607,33 +605,21 @@ mod tests {
     fn pipe_quoted_escape_literal_pipe() {
         // \| inside a pipe-quoted identifier is a literal pipe
         let f = parse_formula("X = |A\\|B|", "Cat").unwrap();
-        if let Expr::Ref(ref s) = f.expr {
-            assert_eq!(s, "A|B");
-        } else {
-            panic!("Expected Ref, got: {:?}", f.expr);
-        }
+        assert!(matches!(&f.expr, Expr::Ref(s) if s == "A|B"));
     }
 
     #[test]
     fn pipe_quoted_escape_double_backslash() {
         // \\ inside a pipe-quoted identifier is a literal backslash
         let f = parse_formula("X = |A\\\\B|", "Cat").unwrap();
-        if let Expr::Ref(ref s) = f.expr {
-            assert_eq!(s, "A\\B");
-        } else {
-            panic!("Expected Ref, got: {:?}", f.expr);
-        }
+        assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\\B"));
     }
 
     #[test]
     fn pipe_quoted_escape_newline() {
         // \n inside a pipe-quoted identifier is a literal newline
         let f = parse_formula("X = |A\\nB|", "Cat").unwrap();
-        if let Expr::Ref(ref s) = f.expr {
-            assert_eq!(s, "A\nB");
-        } else {
-            panic!("Expected Ref, got: {:?}", f.expr);
-        }
+        assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\nB"));
     }
 
     #[test]
@@ -642,11 +628,7 @@ mod tests {
         // backslash-plus-character.  The grammar's `"\\" ~ ANY` allows
         // any following character; we just don't interpret it.
         let f = parse_formula("X = |A\\zB|", "Cat").unwrap();
-        if let Expr::Ref(ref s) = f.expr {
-            assert_eq!(s, "A\\zB");
-        } else {
-            panic!("Expected Ref, got: {:?}", f.expr);
-        }
+        assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\\zB"));
     }
 
     // ── Operator precedence and associativity ──────────────────────────
@@ -655,66 +637,62 @@ mod tests {
     fn mul_binds_tighter_than_add() {
         // `A + B * C` must parse as Add(A, Mul(B, C))
         let f = parse_formula("X = A + B * C", "Cat").unwrap();
-        if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr {
-            assert!(matches!(**lhs, Expr::Ref(_)));
-            assert!(matches!(**rhs, Expr::BinOp(BinOp::Mul, _, _)));
-        } else {
-            panic!("Expected Add with Mul on rhs, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Add, lhs, rhs)
+                if matches!(**lhs, Expr::Ref(_))
+                    && matches!(**rhs, Expr::BinOp(BinOp::Mul, _, _))
+        ));
     }
 
     #[test]
     fn pow_binds_tighter_than_mul() {
         // `A * B ^ C` must parse as Mul(A, Pow(B, C))
         let f = parse_formula("X = A * B ^ C", "Cat").unwrap();
-        if let Expr::BinOp(BinOp::Mul, lhs, rhs) = &f.expr {
-            assert!(matches!(**lhs, Expr::Ref(_)));
-            assert!(matches!(**rhs, Expr::BinOp(BinOp::Pow, _, _)));
-        } else {
-            panic!("Expected Mul with Pow on rhs, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Mul, lhs, rhs)
+                if matches!(**lhs, Expr::Ref(_))
+                    && matches!(**rhs, Expr::BinOp(BinOp::Pow, _, _))
+        ));
     }
 
     #[test]
     fn subtraction_is_left_associative() {
         // `A - B - C` must parse as Sub(Sub(A, B), C), not Sub(A, Sub(B, C))
         let f = parse_formula("X = A - B - C", "Cat").unwrap();
-        if let Expr::BinOp(BinOp::Sub, lhs, rhs) = &f.expr {
-            assert!(matches!(**lhs, Expr::BinOp(BinOp::Sub, _, _)));
-            assert!(matches!(**rhs, Expr::Ref(ref s) if s == "C"));
-        } else {
-            panic!("Expected left-associative Sub, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Sub, lhs, rhs)
+                if matches!(**lhs, Expr::BinOp(BinOp::Sub, _, _))
+                    && matches!(**rhs, Expr::Ref(ref s) if s == "C")
+        ));
     }
 
     #[test]
     fn division_is_left_associative() {
         // `A / B / C` must parse as Div(Div(A, B), C)
         let f = parse_formula("X = A / B / C", "Cat").unwrap();
-        if let Expr::BinOp(BinOp::Div, lhs, rhs) = &f.expr {
-            assert!(matches!(**lhs, Expr::BinOp(BinOp::Div, _, _)));
-            assert!(matches!(**rhs, Expr::Ref(ref s) if s == "C"));
-        } else {
-            panic!("Expected left-associative Div, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Div, lhs, rhs)
+                if matches!(**lhs, Expr::BinOp(BinOp::Div, _, _))
+                    && matches!(**rhs, Expr::Ref(ref s) if s == "C")
+        ));
     }
 
     #[test]
     fn unary_minus_before_pow() {
-        // `-A ^ B` — hand-rolled parser parses this as UnaryMinus(Ref(A))
-        // followed by nothing because pow is inside unary's recursion.
-        // My grammar: unary = unary_minus | primary, unary_minus = "-" ~ primary.
-        // So `-A` is UnaryMinus(Ref(A)); `^B` can't attach because pow_expr
-        // sits OUTSIDE unary.  Result: pow_expr(unary_minus(A)) — no pow
-        // because the optional pow_op doesn't have another unary to bind to.
-        // Verify: `-A ^ B` parses to BinOp(Pow, UnaryMinus(A), B).
+        // `-A ^ B` must parse as Pow(UnaryMinus(A), B).  The `-` binds
+        // through `unary_minus = { "-" ~ primary }`, producing a unary
+        // node that is then used as the pow_expr's base.
         let f = parse_formula("X = -A ^ B", "Cat").unwrap();
-        if let Expr::BinOp(BinOp::Pow, lhs, rhs) = &f.expr {
-            assert!(matches!(**lhs, Expr::UnaryMinus(_)));
-            assert!(matches!(**rhs, Expr::Ref(ref s) if s == "B"));
-        } else {
-            panic!("Expected Pow(UnaryMinus, Ref), got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Pow, lhs, rhs)
+                if matches!(**lhs, Expr::UnaryMinus(_))
+                    && matches!(**rhs, Expr::Ref(ref s) if s == "B")
+        ));
     }
 
     // ── Number literal variants ────────────────────────────────────────
@@ -767,11 +745,11 @@ mod tests {
     fn nested_sum_aggregate() {
         // Nested aggregates — outer SUM wrapping an inner SUM.
         let f = parse_formula("X = SUM(SUM(Revenue))", "Cat").unwrap();
-        if let Expr::Agg(AggFunc::Sum, outer_inner, None) = &f.expr {
-            assert!(matches!(**outer_inner, Expr::Agg(AggFunc::Sum, _, None)));
-        } else {
-            panic!("Expected nested SUM aggregates, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::Agg(AggFunc::Sum, outer_inner, None)
+                if matches!(**outer_inner, Expr::Agg(AggFunc::Sum, _, None))
+        ));
     }
 
     #[test]
@@ -785,12 +763,12 @@ mod tests {
     fn nested_if_expression() {
         // IF in the then-branch of another IF.
         let f = parse_formula("X = IF(A > B, IF(C > D, 1, 2), 3)", "Cat").unwrap();
-        if let Expr::If(_, then_e, else_e) = &f.expr {
-            assert!(matches!(**then_e, Expr::If(_, _, _)));
-            assert!(matches!(**else_e, Expr::Number(n) if n == 3.0));
-        } else {
-            panic!("Expected outer IF, got: {:?}", f.expr);
-        }
+        assert!(matches!(
+            &f.expr,
+            Expr::If(_, then_e, else_e)
+                if matches!(**then_e, Expr::If(_, _, _))
+                    && matches!(**else_e, Expr::Number(n) if n == 3.0)
+        ));
     }
 
     // ── Whitespace tolerance ───────────────────────────────────────────
@@ -845,6 +823,34 @@ mod tests {
         assert_eq!(f.target, "my_target-name");
     }
 
+    /// Regression: the hand-rolled tokenizer didn't allow `-` inside bare
+    /// identifiers, so references to persistence-legal names like
+    /// `east-coast` were silently parsed as `east - coast` (a subtraction
+    /// of two unrelated Refs).  The pest grammar mirrors improv.pest's
+    /// `bare_name`, which does allow `-`, so a hyphenated name now parses
+    /// as a single identifier.
+    #[test]
+    fn hyphen_in_bare_identifier_reference() {
+        let f = parse_formula("Total = east-coast + west-coast", "Cat").unwrap();
+        assert!(matches!(
+            &f.expr,
+            Expr::BinOp(BinOp::Add, lhs, rhs)
+                if matches!(**lhs, Expr::Ref(ref s) if s == "east-coast")
+                    && matches!(**rhs, Expr::Ref(ref s) if s == "west-coast")
+        ));
+    }
+
+    #[test]
+    fn hyphen_in_bare_identifier_inside_aggregate() {
+        // Same fix, but inside a SUM() to exercise the aggregate path as well.
+        let f = parse_formula("X = SUM(east-coast)", "Cat").unwrap();
+        assert!(matches!(
+            &f.expr,
+            Expr::Agg(AggFunc::Sum, inner, None)
+                if matches!(**inner, Expr::Ref(ref s) if s == "east-coast")
+        ));
+    }
+
     #[test]
     fn pipe_quoted_target_preserves_pipes() {
         let f = parse_formula("|My Target| = A", "Cat").unwrap();
@@ -907,13 +913,6 @@ mod generator {
             match expr {
                 Expr::Str(s) => out.push_str(s),
                 Expr::Insens(s) => out.push_str(s),
-                Expr::Range(lo, hi) => {
-                    let lo = lo.chars().next().unwrap() as u32;
-                    let hi = hi.chars().next().unwrap() as u32;
-                    let range = hi - lo + 1;
-                    let ch = char::from_u32(lo + (self.pick() as u32 % range)).unwrap();
-                    out.push(ch);
-                }
                 Expr::Ident(name) => self.emit_ident(name, out, atomic),
                 Expr::Seq(a, b) => {
                     self.emit(a, out, atomic);
@@ -923,18 +922,19 @@ mod generator {
                     self.emit(b, out, atomic);
                 }
                 Expr::Choice(a, b) => {
-                    let mut alts: Vec<&Expr> = vec![a.as_ref()];
-                    let mut cur = b.as_ref();
-                    while let Expr::Choice(l, r) = cur {
-                        alts.push(l.as_ref());
-                        cur = r.as_ref();
+                    // 50/50 between the two branches of each Choice.
+                    // Nested Choices (used by rules with 3+ alternatives)
+                    // recurse into themselves, so deeper branches are
+                    // chosen less often than shallower ones — good
+                    // enough for the random-generation use case.
+                    if self.pick().is_multiple_of(2) {
+                        self.emit(a, out, atomic);
+                    } else {
+                        self.emit(b, out, atomic);
                     }
-                    alts.push(cur);
-                    let idx = self.pick() as usize % alts.len();
-                    self.emit(alts[idx], out, atomic);
                 }
                 Expr::Opt(inner) => {
-                    // ~66% chance of emitting
+                    // ~66% chance of emitting the optional branch.
                     if !self.pick().is_multiple_of(3) {
                         self.emit(inner, out, atomic);
                     }
@@ -949,20 +949,20 @@ mod generator {
                     }
                 }
                 Expr::RepOnce(inner) => {
+                    // formula.pest only uses `+` inside atomic rules
+                    // (ASCII_DIGIT+), so inner repetitions never need
+                    // whitespace separation.
                     let count = 1 + self.pick() % 3;
-                    for i in 0..count {
-                        if i > 0 && !atomic {
-                            out.push(' ');
-                        }
+                    for _ in 0..count {
                         self.emit(inner, out, atomic);
                     }
                 }
-                Expr::NegPred(_) | Expr::PosPred(_) => {
-                    // Lookaheads don't emit output.
-                }
-                _ => {
-                    // Skip unsupported expressions.
-                }
+                // Lookaheads (NegPred, PosPred) don't emit output.  Any
+                // other Expr variant (Range, RepExact, Push, PeekSlice,
+                // Skip, …) is unused by formula.pest and silently
+                // produces nothing; if the grammar starts using one,
+                // generated formulas will fail to parse and we'll know.
+                _ => {}
             }
         }
 
@@ -984,21 +984,31 @@ mod generator {
                 "NEWLINE" => out.push('\n'),
                 "SOI" | "EOI" => {}
                 _ => {
-                    if let Some((ty, inner)) = self.rules.get(name).cloned() {
-                        let inner_atomic = atomic
-                            || matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic);
-                        self.emit(&inner, out, inner_atomic);
-                    }
+                    // Every Ident in formula.pest refers to a rule that
+                    // exists in the grammar file — if it doesn't, the
+                    // grammar itself failed to compile and we wouldn't be
+                    // running tests, so `expect` here is a bug marker.
+                    let (ty, inner) = self
+                        .rules
+                        .get(name)
+                        .cloned()
+                        .expect("rule referenced by grammar exists");
+                    let inner_atomic =
+                        atomic || matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic);
+                    self.emit(&inner, out, inner_atomic);
                 }
             }
         }
 
         pub fn generate(&mut self, rule_name: &str) -> String {
             let mut out = String::new();
-            if let Some((ty, expr)) = self.rules.get(rule_name).cloned() {
-                let atomic = matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic);
-                self.emit(&expr, &mut out, atomic);
-            }
+            let (ty, expr) = self
+                .rules
+                .get(rule_name)
+                .cloned()
+                .expect("entry rule exists in formula.pest");
+            let atomic = matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic);
+            self.emit(&expr, &mut out, atomic);
             out
         }
     }

From 7c00695398a1e5b91c3cda3a41a4d8f5e0e79c9b Mon Sep 17 00:00:00 2001
From: Edward Langley <el-github@elangley.org>
Date: Wed, 15 Apr 2026 21:32:34 -0700
Subject: [PATCH 2/5] refactor(navigation): include AppMode in view navigation
 stack

Introduce `ViewFrame` to store both the view name and the `AppMode` when
pushing to the navigation stack. Update `view_back_stack` and
`view_forward_stack` to use `ViewFrame` instead of `String` . Update
`CmdContext` and `Effect` implementations (SwitchView, ViewBack,
ViewForward) to handle the new `ViewFrame` structure. Add `is_editing()`
helper to `AppMode` .

Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-26B-A4B-it-UD-Q5_K_XL.gguf)
---
 src/command/cmd/core.rs |  4 ++--
 src/command/cmd/grid.rs | 10 ++++++++--
 src/ui/app.rs           | 16 ++++++++++++++--
 src/ui/effect.rs        | 37 +++++++++++++++++++++++++------------
 4 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/src/command/cmd/core.rs b/src/command/cmd/core.rs
index d9d2742..63c795c 100644
--- a/src/command/cmd/core.rs
+++ b/src/command/cmd/core.rs
@@ -34,8 +34,8 @@ pub struct CmdContext<'a> {
     /// Named text buffers
     pub buffers: &'a HashMap<String, String>,
     /// View navigation stacks (for drill back/forward)
-    pub view_back_stack: &'a [String],
-    pub view_forward_stack: &'a [String],
+    pub view_back_stack: &'a [crate::ui::app::ViewFrame],
+    pub view_forward_stack: &'a [crate::ui::app::ViewFrame],
     /// Whether the app currently has an active drill snapshot.
     pub has_drill_state: bool,
     /// Display value at the cursor — works uniformly for pivot and records mode.
diff --git a/src/command/cmd/grid.rs b/src/command/cmd/grid.rs
index a0176ab..178b58d 100644
--- a/src/command/cmd/grid.rs
+++ b/src/command/cmd/grid.rs
@@ -67,7 +67,10 @@ mod tests {
         let m = two_cat_model();
         let layout = make_layout(&m);
         let reg = make_registry();
-        let fwd_stack = vec!["View 2".to_string()];
+        let fwd_stack = vec![crate::ui::app::ViewFrame {
+            view_name: "View 2".to_string(),
+            mode: crate::ui::app::AppMode::Normal,
+        }];
         let mut ctx = make_ctx(&m, &layout, &reg);
         ctx.view_forward_stack = &fwd_stack;
         let cmd = ViewNavigate { forward: true };
@@ -84,7 +87,10 @@ mod tests {
         let m = two_cat_model();
         let layout = make_layout(&m);
         let reg = make_registry();
-        let back_stack = vec!["Default".to_string()];
+        let back_stack = vec![crate::ui::app::ViewFrame {
+            view_name: "Default".to_string(),
+            mode: crate::ui::app::AppMode::Normal,
+        }];
         let mut ctx = make_ctx(&m, &layout, &reg);
         ctx.view_back_stack = &back_stack;
         let cmd = ViewNavigate { forward: false };
diff --git a/src/ui/app.rs b/src/ui/app.rs
index 1d20ca7..1d75bcc 100644
--- a/src/ui/app.rs
+++ b/src/ui/app.rs
@@ -20,6 +20,13 @@ use crate::ui::grid::{
 };
 use crate::view::GridLayout;
 
+/// A saved view+mode pair for the navigation stack.
+#[derive(Debug, Clone, PartialEq)]
+pub struct ViewFrame {
+    pub view_name: String,
+    pub mode: AppMode,
+}
+
 /// Drill-down state: frozen record snapshot + pending edits that have not
 /// yet been applied to the model.
 #[derive(Debug, Clone, Default)]
@@ -88,6 +95,11 @@ impl AppMode {
         }
     }
 
+    /// True for any cell-editing mode (normal or records).
+    pub fn is_editing(&self) -> bool {
+        matches!(self, Self::Editing { .. } | Self::RecordsEditing { .. })
+    }
+
     pub fn editing() -> Self {
         Self::Editing {
             minibuf: MinibufferConfig {
@@ -173,9 +185,9 @@ pub struct App {
     pub tile_cat_idx: usize,
     /// View navigation history: views visited before the current one.
     /// Pushed on SwitchView, popped by `<` (back).
-    pub view_back_stack: Vec<String>,
+    pub view_back_stack: Vec<ViewFrame>,
     /// Views that were "back-ed" from, available for forward navigation (`>`).
-    pub view_forward_stack: Vec<String>,
+    pub view_forward_stack: Vec<ViewFrame>,
     /// Frozen records list for the drill view. When present, this is the
     /// snapshot that records-mode layouts iterate — records don't disappear
     /// when filters would change. Pending edits are stored alongside and
diff --git a/src/ui/effect.rs b/src/ui/effect.rs
index adfca98..ae2d801 100644
--- a/src/ui/effect.rs
+++ b/src/ui/effect.rs
@@ -4,7 +4,7 @@ use std::path::PathBuf;
 use crate::model::cell::{CellKey, CellValue};
 use crate::view::Axis;
 
-use super::app::{App, AppMode};
+use super::app::{App, AppMode, ViewFrame};
 
 pub(crate) const RECORD_COORDS_CANNOT_BE_EMPTY: &str = "Record coordinates cannot be empty";
 
@@ -193,7 +193,10 @@ impl Effect for SwitchView {
     fn apply(&self, app: &mut App) {
         let current = app.model.active_view.clone();
         if current != self.0 {
-            app.view_back_stack.push(current);
+            app.view_back_stack.push(ViewFrame {
+                view_name: current,
+                mode: app.mode.clone(),
+            });
             app.view_forward_stack.clear();
         }
         let _ = app.model.switch_view(&self.0);
@@ -205,10 +208,14 @@ impl Effect for SwitchView {
 pub struct ViewBack;
 impl Effect for ViewBack {
     fn apply(&self, app: &mut App) {
-        if let Some(prev) = app.view_back_stack.pop() {
+        if let Some(frame) = app.view_back_stack.pop() {
             let current = app.model.active_view.clone();
-            app.view_forward_stack.push(current);
-            let _ = app.model.switch_view(&prev);
+            app.view_forward_stack.push(ViewFrame {
+                view_name: current,
+                mode: app.mode.clone(),
+            });
+            let _ = app.model.switch_view(&frame.view_name);
+            app.mode = frame.mode;
         }
     }
 }
@@ -218,10 +225,14 @@ impl Effect for ViewBack {
 pub struct ViewForward;
 impl Effect for ViewForward {
     fn apply(&self, app: &mut App) {
-        if let Some(next) = app.view_forward_stack.pop() {
+        if let Some(frame) = app.view_forward_stack.pop() {
             let current = app.model.active_view.clone();
-            app.view_back_stack.push(current);
-            let _ = app.model.switch_view(&next);
+            app.view_back_stack.push(ViewFrame {
+                view_name: current,
+                mode: app.mode.clone(),
+            });
+            let _ = app.model.switch_view(&frame.view_name);
+            app.mode = frame.mode;
         }
     }
 }
@@ -1134,8 +1145,8 @@ mod tests {
 
         SwitchView("View 2".to_string()).apply(&mut app);
         assert_eq!(app.model.active_view.as_str(), "View 2");
-        assert_eq!(app.view_back_stack, vec!["Default".to_string()]);
-        // Forward stack should be cleared
+        assert_eq!(app.view_back_stack.len(), 1);
+        assert_eq!(app.view_back_stack[0].view_name, "Default");
         assert!(app.view_forward_stack.is_empty());
     }
 
@@ -1156,13 +1167,15 @@ mod tests {
         // Go back
         ViewBack.apply(&mut app);
         assert_eq!(app.model.active_view.as_str(), "Default");
-        assert_eq!(app.view_forward_stack, vec!["View 2".to_string()]);
+        assert_eq!(app.view_forward_stack.len(), 1);
+        assert_eq!(app.view_forward_stack[0].view_name, "View 2");
         assert!(app.view_back_stack.is_empty());
 
         // Go forward
         ViewForward.apply(&mut app);
         assert_eq!(app.model.active_view.as_str(), "View 2");
-        assert_eq!(app.view_back_stack, vec!["Default".to_string()]);
+        assert_eq!(app.view_back_stack.len(), 1);
+        assert_eq!(app.view_back_stack[0].view_name, "Default");
         assert!(app.view_forward_stack.is_empty());
     }
 

From ded35f705c7cfed2050e1e938cb1230ca2bc1a96 Mon Sep 17 00:00:00 2001
From: Edward Langley <el-github@elangley.org>
Date: Wed, 15 Apr 2026 21:32:35 -0700
Subject: [PATCH 3/5] feat(model): use IndexMap for deterministic insertion
 order in DataStore

Replace `HashMap` with `IndexMap` in `DataStore::cells` to preserve
insertion order. This allows records mode to display rows in the order they
were added. Update `remove` to use `shift_remove` to maintain order during
deletions.

Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-26B-A4B-it-UD-Q5_K_XL.gguf)
---
 src/model/cell.rs | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/model/cell.rs b/src/model/cell.rs
index c74cd39..e1cca01 100644
--- a/src/model/cell.rs
+++ b/src/model/cell.rs
@@ -1,3 +1,4 @@
+use indexmap::IndexMap;
 use serde::{Deserialize, Serialize};
 use std::collections::{HashMap, HashSet};
 
@@ -104,8 +105,9 @@ pub struct InternedKey(pub Vec<(Symbol, Symbol)>);
 /// to implement the `Serialize`-as-string requirement for JSON object keys.
 #[derive(Debug, Clone, Default)]
 pub struct DataStore {
-    /// Primary storage — interned keys for O(1) hash/compare.
-    cells: HashMap<InternedKey, CellValue>,
+    /// Primary storage — interned keys, insertion-ordered so records mode
+    /// can display rows in the order they were entered.
+    cells: IndexMap<InternedKey, CellValue>,
     /// String interner — all category/item names are interned here.
     pub symbols: SymbolTable,
     /// Secondary index: interned (category, item) → set of interned keys.
@@ -193,7 +195,7 @@ impl DataStore {
         let Some(ikey) = self.lookup_key(key) else {
             return;
         };
-        if self.cells.remove(&ikey).is_some() {
+        if self.cells.shift_remove(&ikey).is_some() {
             for pair in &ikey.0 {
                 if let Some(set) = self.index.get_mut(pair) {
                     set.remove(&ikey);

From 030865a0fff13a9be48305e1b8dd3b19184eb8ed Mon Sep 17 00:00:00 2001
From: Edward Langley <el-github@elangley.org>
Date: Wed, 15 Apr 2026 21:32:35 -0700
Subject: [PATCH 4/5] feat(records): implement records mode for data entry

Implement a new "Records" mode for data entry.
- Add `RecordsNormal` and `RecordsEditing` to `AppMode` and `ModeKey` .
- `DataStore` now uses `IndexMap` and supports `sort_by_key()` to ensure
  deterministic row order.
- `ToggleRecordsMode` command now sorts data and switches to
  `RecordsNormal` .
- `EnterEditMode` command now respects records editing variants.
- `RecordsNormal` mode includes a new `o` keybinding to add a record row.
- `RecordsEditing` mode inherits from `Editing` and adds an `Esc` binding
  to return to `RecordsNormal` .
- Added `SortData` effect to trigger data sorting.
- Updated UI to display "RECORDS" and "RECORDS INSERT" mode names and
  styles.
- Updated keymaps, command registry, and view navigation to support these
  new modes.
- Added comprehensive tests for records mode behavior, including sorting
  and boundary conditions for Tab/Enter.

Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-26B-A4B-it-UD-Q5_K_XL.gguf)
---
 src/command/cmd/commit.rs   |  35 ++++++--
 src/command/cmd/grid.rs     |   4 +
 src/command/cmd/mode.rs     |   9 +-
 src/command/cmd/registry.rs |   2 +
 src/command/keymap.rs       |  40 +++++++--
 src/draw.rs                 |   6 +-
 src/model/cell.rs           |  20 +++++
 src/ui/app.rs               | 167 ++++++++++++++++++++++++++++++++++--
 src/ui/effect.rs            |  14 ++-
 src/view/layout.rs          |  85 ++++++++++++++++--
 10 files changed, 350 insertions(+), 32 deletions(-)

diff --git a/src/command/cmd/commit.rs b/src/command/cmd/commit.rs
index 7fe2ca2..e4c67f1 100644
--- a/src/command/cmd/commit.rs
+++ b/src/command/cmd/commit.rs
@@ -3,6 +3,7 @@ use crate::ui::app::AppMode;
 use crate::ui::effect::{self, Effect};
 
 use super::core::{Cmd, CmdContext};
+use super::grid::AddRecordRow;
 use super::navigation::{CursorState, EnterAdvance, viewport_effects};
 
 #[cfg(test)]
@@ -259,15 +260,31 @@ impl Cmd for CommitAndAdvance {
             }
             AdvanceDir::Right => {
                 let col_max = self.cursor.col_count.saturating_sub(1);
-                let nc = (self.cursor.col + 1).min(col_max);
-                effects.extend(viewport_effects(
-                    self.cursor.row,
-                    nc,
-                    self.cursor.row_offset,
-                    self.cursor.col_offset,
-                    self.cursor.visible_rows,
-                    self.cursor.visible_cols,
-                ));
+                let row_max = self.cursor.row_count.saturating_sub(1);
+                let at_bottom_right = self.cursor.row >= row_max && self.cursor.col >= col_max;
+
+                if at_bottom_right && ctx.is_records_mode() {
+                    let add = AddRecordRow;
+                    effects.extend(add.execute(ctx));
+                    effects.extend(viewport_effects(
+                        self.cursor.row + 1,
+                        0,
+                        self.cursor.row_offset,
+                        self.cursor.col_offset,
+                        self.cursor.visible_rows,
+                        self.cursor.visible_cols,
+                    ));
+                } else {
+                    let nc = (self.cursor.col + 1).min(col_max);
+                    effects.extend(viewport_effects(
+                        self.cursor.row,
+                        nc,
+                        self.cursor.row_offset,
+                        self.cursor.col_offset,
+                        self.cursor.visible_rows,
+                        self.cursor.visible_cols,
+                    ));
+                }
             }
         }
         effects.push(Box::new(effect::EnterEditAtCursor));
diff --git a/src/command/cmd/grid.rs b/src/command/cmd/grid.rs
index 178b58d..4e418d3 100644
--- a/src/command/cmd/grid.rs
+++ b/src/command/cmd/grid.rs
@@ -1,4 +1,5 @@
 use crate::model::cell::CellValue;
+use crate::ui::app::AppMode;
 use crate::ui::effect::{self, Effect};
 use crate::view::AxisEntry;
 
@@ -414,6 +415,8 @@ impl Cmd for ToggleRecordsMode {
         let mut effects: Vec<Box<dyn Effect>> = Vec::new();
         let records_name = "_Records".to_string();
 
+        effects.push(Box::new(effect::SortData));
+
         // Create (or replace) a _Records view and switch to it
         effects.push(Box::new(effect::CreateView(records_name.clone())));
         effects.push(Box::new(effect::SwitchView(records_name)));
@@ -435,6 +438,7 @@ impl Cmd for ToggleRecordsMode {
                 }));
             }
         }
+        effects.push(effect::change_mode(AppMode::RecordsNormal));
         effects.push(effect::set_status("Records mode"));
         effects
     }
diff --git a/src/command/cmd/mode.rs b/src/command/cmd/mode.rs
index 0a705f3..84ed870 100644
--- a/src/command/cmd/mode.rs
+++ b/src/command/cmd/mode.rs
@@ -197,13 +197,18 @@ impl Cmd for EnterEditMode {
     fn name(&self) -> &'static str {
         "enter-edit-mode"
     }
-    fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
+    fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
+        let edit_mode = if ctx.mode.is_records() {
+            AppMode::records_editing()
+        } else {
+            AppMode::editing()
+        };
         vec![
             Box::new(effect::SetBuffer {
                 name: "edit".to_string(),
                 value: self.initial_value.clone(),
             }),
-            effect::change_mode(AppMode::editing()),
+            effect::change_mode(edit_mode),
         ]
     }
 }
diff --git a/src/command/cmd/registry.rs b/src/command/cmd/registry.rs
index 7e61141..13a727f 100644
--- a/src/command/cmd/registry.rs
+++ b/src/command/cmd/registry.rs
@@ -320,6 +320,8 @@ pub fn default_registry() -> CmdRegistry {
             "command" => AppMode::command_mode(),
             "category-add" => AppMode::category_add(),
             "editing" => AppMode::editing(),
+            "records-normal" => AppMode::RecordsNormal,
+            "records-editing" => AppMode::records_editing(),
             "formula-edit" => AppMode::formula_edit(),
             "export-prompt" => AppMode::export_prompt(),
             other => return Err(format!("Unknown mode: {other}")),
diff --git a/src/command/keymap.rs b/src/command/keymap.rs
index d7bdac5..a0276e3 100644
--- a/src/command/keymap.rs
+++ b/src/command/keymap.rs
@@ -61,6 +61,8 @@ pub enum ModeKey {
     CommandMode,
     SearchMode,
     ImportWizard,
+    RecordsNormal,
+    RecordsEditing,
 }
 
 impl ModeKey {
@@ -80,6 +82,8 @@ impl ModeKey {
             AppMode::ExportPrompt { .. } => Some(ModeKey::ExportPrompt),
             AppMode::CommandMode { .. } => Some(ModeKey::CommandMode),
             AppMode::ImportWizard => Some(ModeKey::ImportWizard),
+            AppMode::RecordsNormal => Some(ModeKey::RecordsNormal),
+            AppMode::RecordsEditing { .. } => Some(ModeKey::RecordsEditing),
             _ => None,
         }
     }
@@ -450,14 +454,9 @@ impl KeymapSet {
         normal.bind(KeyCode::Char('z'), none, "toggle-group-under-cursor");
         normal.bind(KeyCode::Char('H'), none, "hide-selected-row-item");
 
-        // Drill into aggregated cell / view history / add row
+        // Drill into aggregated cell / view history
         normal.bind(KeyCode::Char('>'), none, "drill-into-cell");
         normal.bind(KeyCode::Char('<'), none, "view-back");
-        normal.bind_seq(
-            KeyCode::Char('o'),
-            none,
-            vec![("add-record-row", vec![]), ("enter-edit-at-cursor", vec![])],
-        );
 
         // Records mode toggle and prune toggle
         normal.bind(KeyCode::Char('R'), none, "toggle-records-mode");
@@ -484,7 +483,17 @@ impl KeymapSet {
         z_map.bind(KeyCode::Char('Z'), none, "wq");
         normal.bind_prefix(KeyCode::Char('Z'), none, Arc::new(z_map));
 
-        set.insert(ModeKey::Normal, Arc::new(normal));
+        let normal = Arc::new(normal);
+        set.insert(ModeKey::Normal, normal.clone());
+
+        // ── Records normal mode (inherits from normal) ────────────────────
+        let mut rn = Keymap::with_parent(normal);
+        rn.bind_seq(
+            KeyCode::Char('o'),
+            none,
+            vec![("add-record-row", vec![]), ("enter-edit-at-cursor", vec![])],
+        );
+        set.insert(ModeKey::RecordsNormal, Arc::new(rn));
 
         // ── Help mode ────────────────────────────────────────────────────
         let mut help = Keymap::new();
@@ -754,7 +763,20 @@ impl KeymapSet {
         );
         ed.bind_args(KeyCode::Backspace, none, "pop-char", vec!["edit".into()]);
         ed.bind_any_char("append-char", vec!["edit".into()]);
-        set.insert(ModeKey::Editing, Arc::new(ed));
+        let ed = Arc::new(ed);
+        set.insert(ModeKey::Editing, ed.clone());
+
+        // ── Records editing mode (inherits from editing) ──────────────────
+        let mut re = Keymap::with_parent(ed);
+        re.bind_seq(
+            KeyCode::Esc,
+            none,
+            vec![
+                ("clear-buffer", vec!["edit".into()]),
+                ("enter-mode", vec!["records-normal".into()]),
+            ],
+        );
+        set.insert(ModeKey::RecordsEditing, Arc::new(re));
 
         // ── Formula edit ─────────────────────────────────────────────────
         let mut fe = Keymap::new();
@@ -1106,6 +1128,8 @@ mod tests {
             ModeKey::CommandMode,
             ModeKey::SearchMode,
             ModeKey::ImportWizard,
+            ModeKey::RecordsNormal,
+            ModeKey::RecordsEditing,
         ];
         for mode in &expected_modes {
             assert!(
diff --git a/src/draw.rs b/src/draw.rs
index 1d8bcd3..3eedc96 100644
--- a/src/draw.rs
+++ b/src/draw.rs
@@ -133,12 +133,16 @@ fn mode_name(mode: &AppMode) -> &'static str {
         AppMode::CommandMode { .. } => "COMMAND",
         AppMode::Help => "HELP",
         AppMode::Quit => "QUIT",
+        AppMode::RecordsNormal => "RECORDS",
+        AppMode::RecordsEditing { .. } => "RECORDS INSERT",
     }
 }
 
 fn mode_style(mode: &AppMode) -> Style {
     match mode {
-        AppMode::Editing { .. } => Style::default().fg(Color::Black).bg(Color::Green),
+        AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => {
+            Style::default().fg(Color::Black).bg(Color::Green)
+        }
         AppMode::CommandMode { .. } => Style::default().fg(Color::Black).bg(Color::Yellow),
         AppMode::TileSelect => Style::default().fg(Color::Black).bg(Color::Magenta),
         _ => Style::default().fg(Color::Black).bg(Color::DarkGray),
diff --git a/src/model/cell.rs b/src/model/cell.rs
index e1cca01..063251c 100644
--- a/src/model/cell.rs
+++ b/src/model/cell.rs
@@ -162,6 +162,26 @@ impl DataStore {
         )
     }
 
+    /// Sort cells by their CellKey for deterministic display order.
+    /// Call once on entry into records mode so existing data is ordered;
+    /// subsequent inserts append at the end.
+    pub fn sort_by_key(&mut self) {
+        let symbols = &self.symbols;
+        self.cells.sort_by(|a, _, b, _| {
+            let resolve = |k: &InternedKey| -> Vec<(String, String)> {
+                k.0.iter()
+                    .map(|(c, i)| {
+                        (
+                            symbols.resolve(*c).to_string(),
+                            symbols.resolve(*i).to_string(),
+                        )
+                    })
+                    .collect()
+            };
+            resolve(a).cmp(&resolve(b))
+        });
+    }
+
     pub fn set(&mut self, key: CellKey, value: CellValue) {
         let ikey = self.intern_key(&key);
         // Update index for each coordinate pair
diff --git a/src/ui/app.rs b/src/ui/app.rs
index 1d75bcc..ff87803 100644
--- a/src/ui/app.rs
+++ b/src/ui/app.rs
@@ -79,6 +79,12 @@ pub enum AppMode {
     },
     Help,
     Quit,
+    /// Records-mode normal: inherits from Normal with records-specific bindings.
+    RecordsNormal,
+    /// Records-mode editing: inherits from Editing with boundary-aware Tab/Enter.
+    RecordsEditing {
+        minibuf: MinibufferConfig,
+    },
 }
 
 impl AppMode {
@@ -86,6 +92,7 @@ impl AppMode {
     pub fn minibuffer(&self) -> Option<&MinibufferConfig> {
         match self {
             Self::Editing { minibuf, .. }
+            | Self::RecordsEditing { minibuf, .. }
             | Self::FormulaEdit { minibuf, .. }
             | Self::CommandMode { minibuf, .. }
             | Self::CategoryAdd { minibuf, .. }
@@ -110,6 +117,21 @@ impl AppMode {
         }
     }
 
+    pub fn records_editing() -> Self {
+        Self::RecordsEditing {
+            minibuf: MinibufferConfig {
+                buffer_key: "edit",
+                prompt: "edit: ".into(),
+                color: Color::Green,
+            },
+        }
+    }
+
+    /// True when this mode is a records-mode variant.
+    pub fn is_records(&self) -> bool {
+        matches!(self, Self::RecordsNormal | Self::RecordsEditing { .. })
+    }
+
     pub fn formula_edit() -> Self {
         Self::FormulaEdit {
             minibuf: MinibufferConfig {
@@ -408,7 +430,12 @@ impl App {
             AppMode::Normal => {
                 "hjkl:nav  i:edit  R:records  P:prune  F/C/V:panels  T:tiles  [:]:page  >:drill  ::cmd"
             }
-            AppMode::Editing { .. } => "Enter:commit  Tab:commit+right  Esc:cancel",
+            AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => {
+                "Enter:commit  Tab:commit+right  Esc:cancel"
+            }
+            AppMode::RecordsNormal => {
+                "hjkl:nav  i:edit  o:add-row  R:pivot  P:prune  <:back  ::cmd"
+            }
             AppMode::FormulaPanel => "n:new  d:delete  jk:nav  Esc:back",
             AppMode::FormulaEdit { .. } => "Enter:save  Esc:cancel  — type: Name = expression",
             AppMode::CategoryPanel => {
@@ -726,6 +753,43 @@ mod tests {
     }
 
     /// Regression: pressing `o` in an empty records view should create the
+    /// Pressing R to enter records mode should sort existing data by CellKey
+    /// so display order is deterministic regardless of insertion order.
+    #[test]
+    fn entering_records_mode_sorts_existing_data() {
+        use crate::model::cell::{CellKey, CellValue};
+        let mut m = Model::new("T");
+        m.add_category("Region").unwrap();
+        m.category_mut("Region").unwrap().add_item("North");
+        m.category_mut("Region").unwrap().add_item("East");
+        // Insert in reverse-alphabetical order
+        m.set_cell(
+            CellKey::new(vec![("Region".into(), "North".into())]),
+            CellValue::Number(1.0),
+        );
+        m.set_cell(
+            CellKey::new(vec![("Region".into(), "East".into())]),
+            CellValue::Number(2.0),
+        );
+        let mut app = App::new(m, None);
+        app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
+            .unwrap();
+        assert!(app.layout.is_records_mode());
+        let region_col = (0..app.layout.col_count())
+            .find(|&c| app.layout.col_label(c) == "Region")
+            .unwrap();
+        let row0 = app.layout.records_display(0, region_col).unwrap();
+        let row1 = app.layout.records_display(1, region_col).unwrap();
+        assert_eq!(
+            row0, "East",
+            "R should sort existing data: first row should be East"
+        );
+        assert_eq!(
+            row1, "North",
+            "R should sort existing data: second row should be North"
+        );
+    }
+
     /// first synthetic row instead of only entering edit mode on empty space.
     #[test]
     fn add_record_row_in_empty_records_view_creates_first_row() {
@@ -748,7 +812,7 @@ mod tests {
             "o should create the first record row in an empty records view"
         );
         assert!(
-            matches!(app.mode, AppMode::Editing { .. }),
+            app.mode.is_editing(),
             "o should leave the app in edit mode, got {:?}",
             app.mode
         );
@@ -787,14 +851,107 @@ mod tests {
             .unwrap();
 
         assert_eq!(
-            app.model.get_cell(&CellKey::new(vec![
-                ("_Measure".to_string(), "Rev".to_string()),
-            ])),
+            app.model.get_cell(&CellKey::new(vec![(
+                "_Measure".to_string(),
+                "Rev".to_string()
+            ),])),
             Some(&crate::model::cell::CellValue::Number(5.0)),
             "editing a synthetic row in plain records mode should write the value"
         );
     }
 
+    /// Build a records-mode app with two data rows for testing Tab/Enter
+    /// behavior at boundaries. Row 0 has _Measure=meas2, row 1 has _Measure=meas1.
+    fn records_model_with_two_rows() -> App {
+        use crate::model::cell::{CellKey, CellValue};
+        let mut m = Model::new("T");
+        m.add_category("Region").unwrap();
+        m.category_mut("Region").unwrap().add_item("North");
+        m.category_mut("_Measure").unwrap().add_item("meas1");
+        m.category_mut("_Measure").unwrap().add_item("meas2");
+        m.set_cell(
+            CellKey::new(vec![
+                ("Region".into(), "North".into()),
+                ("_Measure".into(), "meas2".into()),
+            ]),
+            CellValue::Number(10.0),
+        );
+        m.set_cell(
+            CellKey::new(vec![
+                ("Region".into(), "North".into()),
+                ("_Measure".into(), "meas1".into()),
+            ]),
+            CellValue::Number(20.0),
+        );
+        let mut app = App::new(m, None);
+        app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
+            .unwrap();
+        assert!(
+            app.layout.is_records_mode(),
+            "setup: should be records mode"
+        );
+        assert_eq!(app.layout.row_count(), 2, "setup: should have 2 records");
+        let cols: Vec<String> = (0..app.layout.col_count())
+            .map(|i| app.layout.col_label(i))
+            .collect();
+        assert!(
+            cols.contains(&"Region".to_string()),
+            "setup: should have Region column; got {:?}",
+            cols
+        );
+        assert!(
+            cols.contains(&"_Measure".to_string()),
+            "setup: should have _Measure column; got {:?}",
+            cols
+        );
+        assert_eq!(
+            cols.last().unwrap(),
+            "Value",
+            "setup: Value must be last column; got {:?}",
+            cols
+        );
+        app
+    }
+
+    /// improvise-hmu: TAB on the bottom-right cell of records view should
+    /// insert a new record below and move to the first cell of the new row
+    /// in edit mode.
+    #[test]
+    fn tab_on_bottom_right_of_records_inserts_below() {
+        let mut app = records_model_with_two_rows();
+        let initial_rows = app.layout.row_count();
+        assert!(initial_rows >= 1, "setup: need at least 1 record");
+
+        let last_row = initial_rows - 1;
+        let last_col = app.layout.col_count() - 1;
+        app.model.active_view_mut().selected = (last_row, last_col);
+
+        // Enter edit mode on the bottom-right cell
+        app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
+            .unwrap();
+        assert!(app.mode.is_editing(), "setup: should be editing");
+
+        // TAB should commit, insert below, move to first cell of new row
+        app.handle_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE))
+            .unwrap();
+
+        assert_eq!(
+            app.layout.row_count(),
+            initial_rows + 1,
+            "TAB on bottom-right should insert a record below"
+        );
+        assert_eq!(
+            app.model.active_view().selected,
+            (initial_rows, 0),
+            "TAB should move to first cell of the new row"
+        );
+        assert!(
+            app.mode.is_editing(),
+            "should enter edit mode on the new cell, got {:?}",
+            app.mode
+        );
+    }
+
     /// Drill-view edits should stay staged in drill state until the user
     /// navigates back, at which point ApplyAndClearDrill writes them through.
     #[test]
diff --git a/src/ui/effect.rs b/src/ui/effect.rs
index ae2d801..872abde 100644
--- a/src/ui/effect.rs
+++ b/src/ui/effect.rs
@@ -59,6 +59,14 @@ impl Effect for AddItemInGroup {
     }
 }
 
+#[derive(Debug)]
+pub struct SortData;
+impl Effect for SortData {
+    fn apply(&self, app: &mut App) {
+        app.model.data.sort_by_key();
+    }
+}
+
 #[derive(Debug)]
 pub struct SetCell(pub CellKey, pub CellValue);
 impl Effect for SetCell {
@@ -127,7 +135,11 @@ impl Effect for EnterEditAtCursor {
         let value = ctx.display_value.clone();
         drop(ctx);
         app.buffers.insert("edit".to_string(), value);
-        app.mode = AppMode::editing();
+        app.mode = if app.mode.is_records() {
+            AppMode::records_editing()
+        } else {
+            AppMode::editing()
+        };
     }
 }
 
diff --git a/src/view/layout.rs b/src/view/layout.rs
index a549b08..8cc6516 100644
--- a/src/view/layout.rs
+++ b/src/view/layout.rs
@@ -132,14 +132,12 @@ impl GridLayout {
         page_coords: Vec<(String, String)>,
         none_cats: Vec<String>,
     ) -> Self {
-        let mut records: Vec<(CellKey, CellValue)> = model
+        let records: Vec<(CellKey, CellValue)> = model
             .data
             .matching_cells(&page_coords)
             .into_iter()
             .map(|(k, v)| (k, v.clone()))
             .collect();
-        // Sort for deterministic ordering
-        records.sort_by(|a, b| a.0.0.cmp(&b.0.0));
 
         // Synthesize row items: one per record, labeled with its index
         let row_items: Vec<AxisEntry> = (0..records.len())
@@ -152,7 +150,10 @@ impl GridLayout {
             .into_iter()
             .filter(|c| {
                 let kind = model.category(c).map(|cat| cat.kind);
-                !matches!(kind, Some(CategoryKind::VirtualIndex | CategoryKind::VirtualDim))
+                !matches!(
+                    kind,
+                    Some(CategoryKind::VirtualIndex | CategoryKind::VirtualDim)
+                )
             })
             .map(String::from)
             .collect();
@@ -594,8 +595,7 @@ mod tests {
             CellValue::Number(50.0),
         );
         // Records mode setup: _Measure should not filter
-        m.active_view_mut()
-            .set_axis("_Measure", Axis::None);
+        m.active_view_mut().set_axis("_Measure", Axis::None);
         m
     }
 
@@ -730,6 +730,79 @@ mod tests {
         );
     }
 
+    /// On initial entry into records mode, rows should be in a deterministic
+    /// (CellKey-sorted) order regardless of insertion order.
+    #[test]
+    fn records_mode_initial_layout_is_sorted() {
+        let mut m = Model::new("T");
+        m.add_category("Region").unwrap();
+        m.category_mut("Region").unwrap().add_item("North");
+        m.category_mut("Region").unwrap().add_item("East");
+        // Insert East before North — opposite of alphabetical
+        m.set_cell(
+            CellKey::new(vec![("Region".into(), "East".into())]),
+            CellValue::Number(2.0),
+        );
+        m.set_cell(
+            CellKey::new(vec![("Region".into(), "North".into())]),
+            CellValue::Number(1.0),
+        );
+        // Sort the store before entering records mode, as ToggleRecordsMode would
+        m.data.sort_by_key();
+        let v = m.active_view_mut();
+        v.set_axis("_Index", Axis::Row);
+        v.set_axis("_Dim", Axis::Column);
+        let layout = GridLayout::new(&m, m.active_view());
+        assert_eq!(layout.row_count(), 2);
+        let region_col = (0..layout.col_count())
+            .find(|&c| layout.col_label(c) == "Region")
+            .unwrap();
+        let row0_region = layout.records_display(0, region_col).unwrap();
+        let row1_region = layout.records_display(1, region_col).unwrap();
+        assert_eq!(
+            row0_region, "East",
+            "first row should be East (alphabetical)"
+        );
+        assert_eq!(
+            row1_region, "North",
+            "second row should be North (alphabetical)"
+        );
+    }
+
+    /// New records added after initial layout should appear at the bottom,
+    /// not re-sorted into the middle by CellKey order.
+    #[test]
+    fn records_mode_new_record_appends_at_bottom() {
+        let mut m = records_model();
+        let v = m.active_view_mut();
+        v.set_axis("_Index", Axis::Row);
+        v.set_axis("_Dim", Axis::Column);
+        let layout = GridLayout::new(&m, m.active_view());
+        assert_eq!(layout.row_count(), 2);
+        let first_value = layout.records_display(0, layout.col_count() - 1).unwrap();
+
+        // Add a record whose CellKey sorts BEFORE the existing ones
+        m.set_cell(
+            CellKey::new(vec![
+                ("Region".into(), "AAA".into()),
+                ("_Measure".into(), "Cost".into()),
+            ]),
+            CellValue::Number(999.0),
+        );
+        let layout2 = GridLayout::new(&m, m.active_view());
+        assert_eq!(layout2.row_count(), 3);
+        // First row should still be the same record — new one appends at bottom
+        let first_value2 = layout2.records_display(0, layout2.col_count() - 1).unwrap();
+        assert_eq!(
+            first_value, first_value2,
+            "first row should be unchanged after adding a new record; \
+             new record should append at bottom, not re-sort"
+        );
+        // New record should be the last row
+        let last_value = layout2.records_display(2, layout2.col_count() - 1).unwrap();
+        assert_eq!(last_value, "999", "new record should be the last row");
+    }
+
     fn coord(pairs: &[(&str, &str)]) -> CellKey {
         CellKey::new(
             pairs

From 242ddebb4914caaf5259143ae9bb592e3902fe78 Mon Sep 17 00:00:00 2001
From: Edward Langley <el-github@elangley.org>
Date: Wed, 15 Apr 2026 21:33:18 -0700
Subject: [PATCH 5/5] chore: matches -> method

---
 src/ui/grid.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ui/grid.rs b/src/ui/grid.rs
index cdfe70a..2d3cc0d 100644
--- a/src/ui/grid.rs
+++ b/src/ui/grid.rs
@@ -422,7 +422,7 @@ impl<'a> GridWidget<'a> {
                     }
 
                     // Edit indicator
-                    if matches!(self.mode, AppMode::Editing { .. }) && ri == sel_row {
+                    if self.mode.is_editing() && ri == sel_row {
                         {
                             let buffer = self.buffers.get("edit").map(|s| s.as_str()).unwrap_or("");
                             let edit_x = col_x_at(sel_col);