spec: configurable word delimiters for word-deletion shortcuts (#10348)#10459
spec: configurable word delimiters for word-deletion shortcuts (#10348)#10459lonexreb wants to merge 4 commits intowarpdotdev:masterfrom
Conversation
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec defines configurable word delimiter behavior for word deletion and word navigation across terminal, agent, and editor input contexts. The scope is appropriate for the linked feature and does not introduce a meaningful high-level security surface.
Concerns
- The core deletion behavior contract contradicts the later examples and acceptance criteria, so implementers could build incompatible behavior for delimiter runs.
- The delimiter setting representation is underspecified for escaped whitespace in config and the single-line settings UI.
- The settings schema guidance conflicts with the fallback behavior for missing, empty, and whitespace-only values.
- The test plan should explicitly cover Delete Word Right, not only left deletion plus navigation parity.
Verdict
Found: 1 critical, 2 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| ### B2. Word-deletion semantics | ||
|
|
||
| For Delete Word Left and Delete Word Right, characters in the delimiter set are word boundaries. Delete Word Left removes characters from the cursor leftward until (and including) the next delimiter run, OR start-of-line, whichever comes first. Delete Word Right is the mirror: rightward until a delimiter run or end-of-line. |
There was a problem hiding this comment.
🚨 [CRITICAL] B2 contradicts B7/A1/A2: until (and including) the next delimiter run would make Delete Word Left from foo//bar| remove //bar, but B7 requires removing bar first and the delimiter run on the next press. Rewrite the deletion contract to distinguish deleting a non-delimiter run from deleting an adjacent delimiter run.
| A new setting `editor.word_delimiters` (string of characters) defines the global word-boundary set. Default value: | ||
|
|
||
| ``` | ||
| "/.-_:= \t\n\r\x0b\x0c" |
There was a problem hiding this comment.
\t, \n, etc. are decoded escape sequences, raw backslash characters, or display-only notation, and how users enter/reset whitespace delimiters.
| - **Resolution helper.** Add `resolve_word_delimiters(context: InputContext) -> String` in the same module, implementing the B4 fallback chain. `InputContext` enum: `Terminal`, `Agent`, `Editor`. | ||
| - **Key handler dispatch.** Update `app/src/editor/key_handler.rs` so the four shortcut handlers (Delete Word Left, Delete Word Right, Move Cursor Word Left, Move Cursor Word Right) call into `WordBoundaryClassifier` instead of the existing whitespace-only check. | ||
| - **Whitespace floor.** Inside `WordBoundaryClassifier`, OR the user's set with the fixed whitespace set so B5 holds without callsite branching. | ||
| - **Settings schema.** Add the three keys to `app/src/settings/editor.rs` (and matching terminal/agent settings modules). All three are optional `Option<String>` except `editor.word_delimiters`, which has the literal default. |
There was a problem hiding this comment.
editor.word_delimiters is described as defaulted here, but the behavior contract still requires missing/empty/whitespace-only editor values to fall back to the built-in default. Specify the loaded type and normalization path for each key so implementations do not diverge.
|
|
||
| ## Tests | ||
|
|
||
| - **T1.** Default behavior on `/var/www/example.com`: Delete Word Left from end pops segments per A1. |
There was a problem hiding this comment.
💡 [SUGGESTION] Add explicit tests for Delete Word Right, including delimiter-run collapse and default path behavior. The feature covers both directions, but T1/T2/T6 only assert left deletion and T7 only covers navigation parity.
|
Round-1 fixes for oz-for-oss review: (1) CRITICAL fixed — wrote ONE canonical Delete Word Left/Right algorithm (B2) with worked example for |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
Reviewed the spec-only PR for configurable word delimiters across deletion and navigation shortcuts.
Concerns
- The Delete Word Right acceptance criteria contradict the canonical algorithm and would lead to conflicting implementation/tests.
- The settings documentation has invalid TOML escape guidance and conflicting UI behavior for empty values.
- The left-delete test sequence omits a required intermediate state.
Verdict
Found: 1 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| All criteria below match the canonical algorithm in B2. | ||
|
|
||
| - **A1.** Delete Word Left on `/var/www/example.com|` (cursor at end) with the default `D` produces, on successive presses: `/var/www/example.|` → `/var/www/|` → `/var/|` → `/|` → `|`. (5 presses to fully clear; matches B2 worked example.) | ||
| - **A2.** Delete Word Right is the mirror. On `|/var/www/example.com` (cursor at start) with the default `D`, successive presses produce: `|/var/www/example.com` (Rule 3 — char right=`/` ∈ D — consume `/` then `var`) → `|/www/example.com` → `|/example.com` → `|.com` → `|com` → `|`. |
There was a problem hiding this comment.
🚨 [CRITICAL] A2 contradicts B2's Delete Word Right rule: starting before / should consume /var, and |.com should go directly to |, not |com. Resolve the canonical behavior and update A2/T8a/T7a to match.
|
|
||
| | Key | Type | Default | Notes | | ||
| | -------------------------- | -------- | ---------------------------- | ------------------------------------------------------ | | ||
| | `editor.word_delimiters` | `string` | `"/.-_:= \t\n\r\x0b\x0c"` | Global default. See B6 for value-string semantics, escape sequences, validation, and missing/empty/whitespace-only/invalid handling. Whitespace floor (B5) always applies. | |
There was a problem hiding this comment.
\x0b/\x0c, but B6.2 says users configure a TOML basic string and TOML has no \xNN escape. Document a TOML-safe representation, such as \u000B/\f, and make the examples/tests use the same syntax.
| | `terminal.word_delimiters` | `string` | unset (falls back to editor) | Override for terminal input. Per B4 + B6.5, missing/empty/invalid falls through to `editor.word_delimiters` (NOT directly to the built-in default). | | ||
| | `agent.word_delimiters` | `string` | unset (falls back to editor) | Override for agent prompt input. Same fallthrough as terminal. | | ||
|
|
||
| UI placement: **Settings → Editor → "Word Boundary Characters"**. Single-line text input with a "Reset to default" button and a "Show escapes" toggle (per B6.3). Validation per B6.4 — disallowed control characters trigger an inline error and disable save. Whitespace-only or empty inline error per B6.5. Below the input, a live preview line: |
There was a problem hiding this comment.
"" as equivalent to missing with an empty field and default placeholder. Make this line say only whitespace-only/invalid values error, while empty clears to default.
|
|
||
| ### Delete Word Left | ||
|
|
||
| - **T1.** Default behavior on `/var/www/example.com|`: 5 successive Delete Word Left presses produce `/var/www/example.|`, `/var/www/|`, `/var/|`, `/|`, `|` (matches A1). |
There was a problem hiding this comment.
/| state before the final |. Add /| so the test covers the delimiter-only final step.
|
Round-2 fixes: (1) CRITICAL resolved — every Delete Word Right acceptance criterion now matches the canonical mirror algorithm (cursor-mid-non-delim consumes to end-of-run; otherwise consumes delim+non-delim); two worked examples added; (2) TOML escape guidance corrected — only standard escapes ( |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines configurable word delimiters across deletion and navigation shortcuts, including context overrides, fallback behavior, UI expectations, and tests.
Concerns
- The TOML escape guidance for VT/FF needs to be unambiguous and copy-pasteable before implementation.
- The acceptance criteria currently flatten whitespace-only fallback to the built-in default, contradicting the per-context fallback rule.
- The implementation pointers make
editor.word_delimitersa literal-default schema value, which conflicts with the spec's absent/empty-as-default behavior and UI state.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| **Non-standard `\xXX` is NOT a valid TOML escape** and will fail TOML parsing if used. Earlier drafts of this spec referenced `\x0b` and `\x0c` for vertical tab and form feed; those references were incorrect. The valid encodings are: | ||
|
|
||
| - Vertical tab (U+000B) — encode as ` `. (TOML has no single-character escape for VT.) |
There was a problem hiding this comment.
\u000B and \f/\u000C so the default can be implemented and documented consistently.
| - **A3.** With default set, on `/path/to/file|` the first Delete Word Left removes only `file`, leaving `/path/to/|`. (Rule 3: char left=`e` ∉ D, consume just the non-delimiter run `file`.) | ||
| - **A4.** Per-context override applies only in that input context; other contexts use the editor default or their own override (per B4). | ||
| - **A5.** Missing/empty setting value falls back to the built-in default from B1 (per B6.5). | ||
| - **A6.** Whitespace-only setting value is rejected as invalid; runtime falls back to the built-in default and logs a warning; Settings UI shows inline error (per B6.5). |
There was a problem hiding this comment.
terminal.word_delimiters/agent.word_delimiters fall through to editor.word_delimiters instead of directly to the built-in default; scope A6 to the editor setting or split the acceptance criterion by context.
| - **Resolution helper.** Add `resolve_word_delimiters(context: InputContext) -> String` in the same module, implementing the B4 fallback chain. `InputContext` enum: `Terminal`, `Agent`, `Editor`. | ||
| - **Key handler dispatch.** Update `app/src/editor/key_handler.rs` so the four shortcut handlers (Delete Word Left, Delete Word Right, Move Cursor Word Left, Move Cursor Word Right) call into `WordBoundaryClassifier` instead of the existing whitespace-only check. | ||
| - **Whitespace floor.** Inside `WordBoundaryClassifier`, OR the user's set with the fixed whitespace set so B5 holds without callsite branching. | ||
| - **Settings schema.** Add the three keys to `app/src/settings/editor.rs` (and matching terminal/agent settings modules). All three are optional `Option<String>` except `editor.word_delimiters`, which has the literal default. |
There was a problem hiding this comment.
editor.word_delimiters a schema-level literal default conflicts with B6.5's absent/empty behavior and the UI requirement that the stored value remains empty/absent; specify an optional stored value with the B1 default applied only by the resolver.
|
Addressed all 3 round-N concerns: 1. TOML escape for VT/FF is now unambiguous and copy-pasteable. Replaced the placeholder backticks with explicit text: VT (U+000B) is encoded as the 4-hex-digit Unicode escape 2. A6 no longer flattens whitespace-only fallback to the built-in default. Split A6 into A6.a (global 3. Implementation pointers no longer make Commit: 6b439d5 |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
The spec defines configurable word delimiters for deletion/navigation shortcuts, settings resolution, UI behavior, and test coverage. The overall structure covers the feature, but several internal contradictions would send implementation and tests in different directions.
Concerns
- The B1 TOML example still uses invalid
\xescapes despite later sections declaring them invalid and providing a corrected form. - The missing/empty fallback rule is stated as direct built-in-default fallback, which conflicts with the per-context fallback chain to
editor.word_delimiters. - The Move Cursor Word Right test contradicts the canonical Delete Word Right parity rule and would validate the wrong cursor stops.
- The Settings UI text-field behavior should clarify how literal newlines work in a single-line input.
Verdict
Found: 1 critical, 2 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| Expressed as a valid TOML basic string (see B6.2 for the full escape rules — note that **`\xXX` is NOT a valid TOML escape**; vertical tab and form feed must be encoded with the Unicode `\u00XX` form): | ||
|
|
||
| ```toml | ||
| editor.word_delimiters = "/.-_:= \t\n\r " |
There was a problem hiding this comment.
\x0b/\x0c while the surrounding prose says those escapes are invalid TOML; replace it with the B6.2 form (\u000B\f) or remove this duplicate snippet.
|
|
||
| The Settings text field accepts characters either literally (e.g., typing `/`) or as escape sequences (e.g., typing `\t`). Two display modes: | ||
|
|
||
| - **Literal mode (default).** Whitespace characters render as themselves. Tabs and newlines may be hard to see but are still present in the value. |
There was a problem hiding this comment.
💡 [SUGGESTION] A single-line field cannot literally render or hold newline characters as visible line breaks; specify that newlines are entered/displayed via escapes in the UI while stored as codepoints, or define multiline behavior.
|
|
||
| **Empty value behavior (single source of truth — no error on empty):** | ||
|
|
||
| The "missing" and "empty string" cases are treated **identically** as "use the built-in default from B1". This applies whether the value is absent from the TOML config OR the user clears the Settings UI text field. Specifically: |
There was a problem hiding this comment.
editor.word_delimiters, not directly to B1. Scope this sentence to editor.word_delimiters or restate the per-context behavior here.
| ### Move Cursor Word Left / Right | ||
|
|
||
| - **T7.** Move Cursor Word Left / Right land at the same boundaries Delete Word Left / Right would operate on, for identical input and setting (per A9 / B3). Verify on the A1 and A2 examples. | ||
| - **T7a.** Move Cursor Word Right on `foo|/bar/baz` with default set lands at `foo/|bar/baz` after the first press, `foo/bar/|baz` after the second, `foo/bar/baz|` after the third. |
There was a problem hiding this comment.
🚨 [CRITICAL] T7a contradicts B3/A9: using the Delete Word Right parity rules from a cursor before /, the first move should land after bar (foo/bar|/baz), not after only the delimiter (foo/|bar/baz). Update these expected states or explicitly define navigation differently.
1 participant
Summary
Spec for #10348. Makes the set of characters that act as word boundaries configurable for all four word-cursor shortcuts: Delete Word Left, Delete Word Right, Move Cursor Word Left, Move Cursor Word Right. Today these shortcuts treat path-like strings (e.g.
/var/www/example.com/logs) as a single word; with a configurable delimiter set including/,.,-,_,:,=, the shortcuts split per segment, matching VS Code, JetBrains, and Sublime conventions.What's specified
editor.word_delimiterssetting (string of literal characters) with a sensible default:"/.-_:= \t\n\r\x0b\x0c".terminal.word_delimiters,agent.word_delimiters(both fall back toeditor.word_delimiters, then to the built-in default).This spec affects 4 shortcuts (Delete Word Left/Right, Move Cursor Word Left/Right) so they share one boundary definition rather than diverging.
Test plan
Spec-only PR. Tests T1–T10 will be implemented alongside the code change.
foo//bar/label ready-to-spec