feat(core): Implement Scoped Keyboard Focus Trap & Modal Management Subsystem (#3346) - #3360
Conversation
github-actions
Bot
added
type:feature
📝 WalkthroughWalkthroughThis change adds ChangesFocus lock management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant KeyboardEvent
participant FocusLockManager
participant FocusManager
KeyboardEvent->>FocusLockManager: handleKeyDown
FocusLockManager->>FocusManager: move focus on Tab or Shift+Tab
FocusLockManager->>FocusManager: release trap on Escape
FocusLockManager-->>KeyboardEvent: return consumed status
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new core input utility intended to manage a scoped modal focus-trap stack (Tab/Shift+Tab cycling, Escape dismissal) and introduces a corresponding unit test file.
Changes:
- Added
FocusLockManagerwrapper inpackages/core/src/input/focusLock.ts. - Added a new test suite for the focus lock behavior in
packages/core/test/focusLock.test.ts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/core/src/input/focusLock.ts | Introduces FocusLockManager for trapping focus and handling Tab/Escape while a trap is active. |
| packages/core/test/focusLock.test.ts | Adds tests for trap push/pop and key handling behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pushTrap(options: TrapOptions, focusables: Focusable[] = []): void { | ||
| this._trapStack.push(options.containerId); | ||
| this._focusManager.trap(options.containerId, focusables); | ||
| } | ||
|
|
||
| popTrap(): string | null { | ||
| if (this._trapStack.length === 0) return null; | ||
|
|
||
| const popped = this._trapStack.pop()!; | ||
| this._focusManager.release(); | ||
|
|
||
| return popped; | ||
| } |
| export class FocusLockManager { | ||
| private _focusManager: FocusManager; | ||
| private _trapStack: string[] = []; | ||
|
|
||
| constructor(focusManager: FocusManager) { | ||
| this._focusManager = focusManager; | ||
| } |
| @@ -0,0 +1,53 @@ | |||
| import { describe, expect, test, beforeEach } from 'bun:test'; | |||
| test('handles tab navigation cycling inside trap', () => { | ||
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | ||
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | ||
| { id: 'modal-btn2', tabIndex: 2, focusable: true }, | ||
| ]); | ||
|
|
||
| const handled = focusLock.handleKeyDown('tab'); | ||
| expect(handled).toBe(true); | ||
| }); |
| test('pops trap on escape key', () => { | ||
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | ||
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | ||
| ]); | ||
|
|
||
| const handled = focusLock.handleKeyDown('escape'); | ||
| expect(handled).toBe(true); | ||
| expect(focusLock.isLocked).toBe(false); | ||
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/input/focusLock.ts`:
- Around line 33-40: Add a concise inline comment immediately before the
non-null assertion in popTrap, explaining that the preceding _trapStack.length
check guarantees pop() returns a value. Leave the existing stack-pop and
focus-release behavior unchanged.
In `@packages/core/test/focusLock.test.ts`:
- Around line 34-42: Strengthen the `handles tab navigation cycling inside trap`
test by asserting `focusManager.currentId` becomes `modal-btn2` after the first
`handleKeyDown('tab')`, then becomes `modal-btn1` after a second Tab to verify
wrapping. Also assert the background target `bg-btn` is never selected while the
trap remains active, while preserving the existing handled-result assertion.
- Around line 44-52: Extend the Escape test around focusLock.handleKeyDown to
assert focusManager.currentId is restored to the widget focused before pushTrap,
not only that isLocked becomes false. Add a dedicated test that activates a
trap, attempts Tab navigation toward the background widget 'bg-btn', and
verifies that widget remains unreachable while the trap is active.
- Line 1: Update focusLock.test.ts to use the project’s Vitest APIs by importing
describe, expect, test, and beforeEach from vitest instead of bun:test, while
preserving the existing test suite behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fc3349a-0136-4468-a1eb-ca17d551d1f8
📒 Files selected for processing (2)
packages/core/src/input/focusLock.tspackages/core/test/focusLock.test.ts
| popTrap(): string | null { | ||
| if (this._trapStack.length === 0) return null; | ||
|
|
||
| const popped = this._trapStack.pop()!; | ||
| this._focusManager.release(); | ||
|
|
||
| return popped; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a comment justifying the non-null assertion.
Line 36 uses this._trapStack.pop()!. This is a non-null assertion, which TypeScript tooling treats as a type assertion that bypasses the type checker. The coding guidelines require an inline comment explaining why any type assertion is safe.
Add a short comment noting that the preceding length check on line 34 guarantees pop() returns a value.
🔧 Proposed fix
popTrap(): string | null {
if (this._trapStack.length === 0) return null;
- const popped = this._trapStack.pop()!;
+ // Non-null: the length check above guarantees an element exists.
+ const popped = this._trapStack.pop()!;
this._focusManager.release();
return popped;
}As per coding guidelines, "No type assertions without an inline comment explaining why."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| popTrap(): string | null { | |
| if (this._trapStack.length === 0) return null; | |
| const popped = this._trapStack.pop()!; | |
| this._focusManager.release(); | |
| return popped; | |
| } | |
| popTrap(): string | null { | |
| if (this._trapStack.length === 0) return null; | |
| // Non-null: the length check above guarantees an element exists. | |
| const popped = this._trapStack.pop()!; | |
| this._focusManager.release(); | |
| return popped; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/input/focusLock.ts` around lines 33 - 40, Add a concise
inline comment immediately before the non-null assertion in popTrap, explaining
that the preceding _trapStack.length check guarantees pop() returns a value.
Leave the existing stack-pop and focus-release behavior unchanged.
Source: Coding guidelines
| @@ -0,0 +1,53 @@ | |||
| import { describe, expect, test, beforeEach } from 'bun:test'; | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine the project's test runner and compare import conventions across core test files.
set -euo pipefail
echo "--- package.json test scripts ---"
fd -H package.json -x cat -n {} \; 2>/dev/null | rg -n -B2 -A2 '"test"'
echo "--- import styles used across core test files ---"
rg -n "from 'bun:test'|from 'vitest'" packages/core/test packages/widgets 2>/dev/null
echo "--- vitest / bun config files ---"
fd -i 'vitest.config|bunfig' -HRepository: Karanjot786/TermUI
Length of output: 17315
🏁 Script executed:
#!/bin/bash
# Inspect the exact core package test script/config and any Bun/Vitest environment setup.
set -euo pipefail
echo "--- root package manager files ---"
fd -H 'package(-lock)?\.json|bun.lockb?|pnpm-lock.yaml|yarn.lock|vitest.config.ts|bunfig' . -t f | sed 's#^\./##' | sort
echo "--- root package.json relevant fields ---"
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('package.json')
data=json.loads(p.read_text())
print("type:", data.get("type"))
print("scripts:", data.get("scripts", {}))
print("devDependencies.test:", data.get("devDependencies",{}).get("vitest"))
print("dependencies.test:", data.get("dependencies",{}).get("vitest"))
PY
echo "--- packages/core/package.json relevant fields ---"
python3 - <<'PY'
import json
data=json.loads(open('packages/core/package.json').read())
print("scripts:", data.get("scripts"))
print("devDependencies:", data.get("devDependences"))
for k,v in (data.get("devDependencies",{}) or {}).items():
if k in ("vitest","bun","typescript"):
print(k, v)
PY
echo "--- vitest.config.ts ---"
cat -n vitest.config.ts
echo "--- bunfig.toml ---"
cat -n bunfig.toml
echo "--- any bun:test imports elsewhere ---"
rg -n "from 'bun:test'|from \"bun:test\"|import 'bun:test'|import \"bun:test\"" . --glob '!node_modules' --glob '!dist' --glob '!build'Repository: Karanjot786/TermUI
Length of output: 4255
🌐 Web query:
Vitest environment node global test aliases describe expect vi import from vitest official docs
💡 Result:
In Vitest, environment, globals, and API usage are configured as follows: Environment: The default environment in Vitest is 'node' [1][2]. You can change this to 'jsdom', 'happy-dom', or 'edge-runtime' via the 'test.environment' option in your configuration file [1][3]. For file-specific requirements, you can use the @vitest-environment control comment at the top of the file [1][2]: // @vitest-environment jsdom Globals: By default, Vitest does not inject globals for explicitness [4][5]. To use APIs like 'describe', 'test', 'expect', and 'vi' globally, you must enable them in your configuration [4][6]: // vitest.config.ts export default defineConfig({ test: { globals: true, }, }) If using TypeScript, you must also add 'vitest/globals' to your 'tsconfig.json' to ensure proper type resolution [4][5]: { "compilerOptions": { "types": ["vitest/globals"] } } Imports and Aliases: If 'globals' are disabled (the recommended default), you must import the necessary APIs directly from the 'vitest' package [7][8]: import { describe, expect, test, vi } from 'vitest' Regarding aliases, Vitest uses your Vite configuration [9][10]. You can define path aliases in your 'vite.config.ts' (or 'vitest.config.ts') using standard Vite 'resolve.alias' options [9][10]. Note that for 'vi.mock', Vitest performs static analysis; if you use 'vi' without importing it directly from 'vitest', it may not be able to hoist the mock correctly unless 'globals' is enabled [7][11].
Citations:
- 1: https://vitest.dev/config/environment
- 2: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/guide/environment.md
- 3: https://www.devshelfhub.com/cheatsheets/vitest/
- 4: https://vitest.dev/config/globals
- 5: https://github.com/vitest-dev/vitest/blob/413ec5e6fc0addb2216db6104228138f8027f392/docs/config/index.md
- 6: https://raw.githubusercontent.com/vitest-dev/vitest/refs/heads/v3.1/docs/config/index.md
- 7: https://main.vitest.dev/api/vi
- 8: https://vitest.dev/api/test
- 9: https://vitest.dev/config/
- 10: https://github.com/vitest-dev/vitest/blob/main/docs/config/index.md
- 11: https://github.com/vitest-dev/vitest/blob/v4.1.7/docs/api/vi.md
🌐 Web query:
Vitest supports Bun test imports from 'bun:test' environment node default config
💡 Result:
Vitest does not natively support or execute tests using the 'bun:test' module directly. 'bun:test' is a built-in test runner specific to the Bun runtime [1], whereas Vitest is a separate test runner that is designed primarily to run in Node.js environments [2]. While recent versions of Vitest (starting from 4.0.7+) have improved compatibility for running within the Bun runtime, this refers to executing Vitest's own test suite—using Vitest's APIs (e.g., vitest/dist/index.js)—inside the Bun environment [3]. It does not enable the use of Bun's native 'bun:test' API [1]. Attempting to import 'bun:test' while running Vitest can cause issues, such as build-time errors where Vite (Vitest's underlying bundler) attempts to process the import [4] or run-time errors where the Vitest module runner incorrectly modifies module specifiers [5]. Developers needing to run code that uses 'bun:test' must use the native 'bun test' command [1]. If you are using Vitest to test code meant for both environments, you must use Vitest's own mocking and assertion APIs rather than importing 'bun:test' directly [1]. If you encounter issues with static analysis, a common workaround is to use dynamic imports with a non-literal specifier (e.g., import('bun' + ':test')) to prevent the bundler from attempting to resolve the module [4].
Citations:
- 1: https://bun.sh/reference/bun/test
- 2: https://vitest.dev/guide/
- 3: Vitest module mocking does not work with the Bun runtime vitest-dev/vitest#9031
- 4: Cannot bundle built-in module "bun:test" since Vitest v4.0.0 nuxt/test-utils#1490
- 5: Non-node builtin modules get prefixed with node vitest-dev/vitest#8925
Use the project test runner’s APIs in packages/core/test/focusLock.test.ts.
The core package test script runs Vitest against packages/core/src/**/*.test.*, and Vitest uses import { ... } from 'vitest' when globals are configured per-project. This file only imports from bun:test, which Vitest does not run and blocks bun test from discovering the suite consistently. Move the tests/fixture into the configured core test path, or import describe, expect, test, beforeEach from vitest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/focusLock.test.ts` at line 1, Update focusLock.test.ts to
use the project’s Vitest APIs by importing describe, expect, test, and
beforeEach from vitest instead of bun:test, while preserving the existing test
suite behavior.
Source: Learnings
| test('handles tab navigation cycling inside trap', () => { | ||
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | ||
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | ||
| { id: 'modal-btn2', tabIndex: 2, focusable: true }, | ||
| ]); | ||
|
|
||
| const handled = focusLock.handleKeyDown('tab'); | ||
| expect(handled).toBe(true); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Strengthen the "tab navigation cycling" test to assert observable focus movement.
This test only checks handled === true. It does not verify that focus actually moved to 'modal-btn2', that a second Tab wraps back to 'modal-btn1', or that 'bg-btn' is never reachable while the trap is active. The test name promises "cycling inside trap," but nothing here confirms cycling or isolation actually occurred.
Add assertions on focusManager.currentId after each handleKeyDown('tab') call, and confirm 'bg-btn' is skipped.
✅ Proposed strengthening
test('handles tab navigation cycling inside trap', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
{ id: 'modal-btn2', tabIndex: 2, focusable: true },
]);
const handled = focusLock.handleKeyDown('tab');
expect(handled).toBe(true);
+ expect(focusManager.currentId).toBe('modal-btn2');
+
+ focusLock.handleKeyDown('tab');
+ expect(focusManager.currentId).toBe('modal-btn1');
});As per coding guidelines, "Tests must be real. ... Assert observable behavior or rendered output."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('handles tab navigation cycling inside trap', () => { | |
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | |
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | |
| { id: 'modal-btn2', tabIndex: 2, focusable: true }, | |
| ]); | |
| const handled = focusLock.handleKeyDown('tab'); | |
| expect(handled).toBe(true); | |
| }); | |
| test('handles tab navigation cycling inside trap', () => { | |
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | |
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | |
| { id: 'modal-btn2', tabIndex: 2, focusable: true }, | |
| ]); | |
| const handled = focusLock.handleKeyDown('tab'); | |
| expect(handled).toBe(true); | |
| expect(focusManager.currentId).toBe('modal-btn2'); | |
| focusLock.handleKeyDown('tab'); | |
| expect(focusManager.currentId).toBe('modal-btn1'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/focusLock.test.ts` around lines 34 - 42, Strengthen the
`handles tab navigation cycling inside trap` test by asserting
`focusManager.currentId` becomes `modal-btn2` after the first
`handleKeyDown('tab')`, then becomes `modal-btn1` after a second Tab to verify
wrapping. Also assert the background target `bg-btn` is never selected while the
trap remains active, while preserving the existing handled-result assertion.
Source: Coding guidelines
| test('pops trap on escape key', () => { | ||
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | ||
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | ||
| ]); | ||
|
|
||
| const handled = focusLock.handleKeyDown('escape'); | ||
| expect(handled).toBe(true); | ||
| expect(focusLock.isLocked).toBe(false); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert focus restoration after popTrap, and add a dedicated isolation test.
This test verifies isLocked becomes false after Escape, but does not verify that focus is restored to the widget that was focused before the trap (focusManager.currentId), which is an explicit PR objective ("Restore the previous focus highlight state").
Separately, no test in this file verifies that background widgets ('bg-btn') are unreachable via Tab while a trap is active — the core "isolate key event propagation" objective from the linked issue is untested.
✅ Proposed additions
test('pops trap on escape key', () => {
focusLock.pushTrap({ containerId: 'modal-1' }, [
{ id: 'modal-btn1', tabIndex: 1, focusable: true },
]);
const handled = focusLock.handleKeyDown('escape');
expect(handled).toBe(true);
expect(focusLock.isLocked).toBe(false);
+ expect(focusManager.currentId).toBe('bg-btn');
});
+
+ test('isolates focus from background widgets while trapped', () => {
+ focusLock.pushTrap({ containerId: 'modal-1' }, [
+ { id: 'modal-btn1', tabIndex: 1, focusable: true },
+ { id: 'modal-btn2', tabIndex: 2, focusable: true },
+ ]);
+
+ for (let i = 0; i < 5; i++) {
+ focusLock.handleKeyDown('tab');
+ expect(focusManager.currentId).not.toBe('bg-btn');
+ }
+ });As per PR objectives, "Restore the previous focus highlight state" and "Include unit tests covering focus isolation and related behavior."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('pops trap on escape key', () => { | |
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | |
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | |
| ]); | |
| const handled = focusLock.handleKeyDown('escape'); | |
| expect(handled).toBe(true); | |
| expect(focusLock.isLocked).toBe(false); | |
| }); | |
| test('pops trap on escape key', () => { | |
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | |
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | |
| ]); | |
| const handled = focusLock.handleKeyDown('escape'); | |
| expect(handled).toBe(true); | |
| expect(focusLock.isLocked).toBe(false); | |
| expect(focusManager.currentId).toBe('bg-btn'); | |
| }); | |
| test('isolates focus from background widgets while trapped', () => { | |
| focusLock.pushTrap({ containerId: 'modal-1' }, [ | |
| { id: 'modal-btn1', tabIndex: 1, focusable: true }, | |
| { id: 'modal-btn2', tabIndex: 2, focusable: true }, | |
| ]); | |
| for (let i = 0; i < 5; i++) { | |
| focusLock.handleKeyDown('tab'); | |
| expect(focusManager.currentId).not.toBe('bg-btn'); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/focusLock.test.ts` around lines 44 - 52, Extend the Escape
test around focusLock.handleKeyDown to assert focusManager.currentId is restored
to the widget focused before pushTrap, not only that isLocked becomes false. Add
a dedicated test that activates a trap, attempts Tab navigation toward the
background widget 'bg-btn', and verifies that widget remains unreachable while
the trap is active.
Summary
Implements a scoped keyboard focus trap stack and modal management subsystem to cycle focus strictly within active modal containers and restore focus upon dismissal.
Related Issue
Fixes #3346
Type of Change
Changes Implemented
FocusLockManagerinpackages/core/src/input/focusLock.tsfor managing active modal focus traps.packages/core/test/focusLock.test.ts.Testing
bun test packages/core/test/focusLock.test.ts(100% pass).Checklist
mainbase branch.bun.lockchanges).Summary by CodeRabbit
New Features
Tests