Let local bindings override global ones and sort the menu by key - #67
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The specified uppercase-before-lowercase ordering lacks acceptance-test coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Separates binding conflict resolution from menu ordering.
Changes:
- Local bindings override conflicting global bindings.
- Duplicate keys collapse to the first reachable entry.
- Rows sort recursively using fixed-locale natural key ordering.
File summaries
| File | Description |
|---|---|
src/run.ts |
Adds merging, deduplication, and recursive sorting. |
e2e/tests/04_config.bats |
Tests local override behavior. |
e2e/tests/07_render.bats |
Tests deduplication and sorting. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
844196
added a commit
that referenced
this pull request
Sep 3, 2026
Copilot's review of #67 flagged that `caseFirst: 'upper'` (src/run.ts's keyCollator) had no e2e coverage: nothing would catch a regression that dropped it or changed the tie-break direction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not cover preservation and ordering of bindings across nonempty global and local sources.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
e2e/tests/04_config.bats:177
- No global-only key is exercised while the local file is nonempty: every global key is shadowed, so an implementation that drops the global list entirely would still pass. Add a unique global binding and assert that both sources contribute.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
844196
added a commit
that referenced
this pull request
Sep 3, 2026
Copilot's review of #67 flagged that "local bindings override global ones with the same key" only used keys that were either shadowed by local or local-only: an implementation that dropped the global list entirely (rather than merging it) would still have passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Collation-equivalent distinct keys can still retain declaration-dependent ordering.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
844196
added a commit
that referenced
this pull request
Sep 3, 2026
…stic Copilot's review of #67 caught that `Intl.Collator`'s `numeric` option compares distinct keys like `f2` and `f02` as equal, and `toSorted` is stable — so two such keys kept whatever relative order they happened to arrive in (merge order, declaration order), contradicting the promise that display order doesn't depend on origin. Also covers a second gap the same review round flagged: the "regardless of declaration order" render test only ever used a single (global) source, so it couldn't catch an implementation that sorted each source separately before concatenating them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
ICU numeric collation cannot satisfy the stated unlimited digit-width ordering guarantee.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
844196
added a commit
that referenced
this pull request
Sep 3, 2026
Copilot's review of #67 pointed out ICU chunks numeric collation past 254 significant digits, so "no width limit" overclaimed what `Intl.Collator` actually guarantees. A key name that long is not a real scenario for a keybinding menu, so this narrows the comment rather than hand-rolling an arbitrary-precision comparator. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`globalBindings.concat(localBindings)` conflated two unrelated concerns: which definition won when global and local shared a key (main.ts's `find` always resolved to the first match, so global won), and what order the menu drew rows in (declaration order, since nothing reordered the array). Local bindings now shadow global ones sharing a key, whole-entry — a group and a command never partially merge, and a group's own nested `bindings` never cross the global/local boundary. Menu rows sort by key with a fixed locale (natural for digit runs, so f2 comes before f10; uppercase before lowercase; an exact-string tie-break where the collation itself calls two distinct keys equal, e.g. f2 vs f02), independent of declaration order or global/local origin, at every nesting level. A duplicate key now collapses to a single row before it reaches the menu, matching what a keypress already resolved to. BREAKING CHANGE: a local binding used to lose to a global one sharing the same key; it now wins. Menu rows used to draw in declaration order; they now sort by key. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
844196
force-pushed
the
844196/main/local-bindings-precedence-and-sort
branch
from
September 3, 2026 16:43
14f4dc1 to
27018bd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
globalBindings.concat(localBindings)conflated two unrelated concerns: which definition won when global and local bindings shared a key, and what order the menu drew rows in.bindingsnever cross the global/local boundary.enso it doesn't shift with the user'sLANG; numeric sof2sorts beforef10regardless of digit width; uppercase before lowercase), independent of declaration order or global/local origin, at every nesting level — not just the merged top level.main.ts'sfindalways resolved to the first match; previously the menu still showed the unreachable second row).Breaking changes