feat: add keyboard navigation to Breadcrumbs widget#2002
Conversation
github-actions
Bot
added
type:feature
|
Hi @jainiksha 👋 ⭐ Star this repo before your PR merges. Why? GSSoC 2026 contributors who star get priority review and points credit. After you star, push any commit (or re-run this check). The Thanks for your contribution to TermUI. |
📝 WalkthroughWalkthroughBreadcrumbs widget gains keyboard-driven selection with an onSelect callback. TextInput's key handling and rendering pipeline are substantially refactored. A new Workspace module provides named layout save/load/switch/persist operations, exported publicly alongside a narrowed SplitPane type export. ChangesBreadcrumb Keyboard Navigation
Estimated code review effort: 2 (Simple) | ~12 minutes TextInput Refactor
Estimated code review effort: 4 (Complex) | ~55 minutes Workspace Layout Persistence Module
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Breadcrumbs
participant OnSelectCallback
User->>Breadcrumbs: handleKey(left/right)
Breadcrumbs->>Breadcrumbs: update _selectedIndex
User->>Breadcrumbs: handleKey(enter)
Breadcrumbs->>OnSelectCallback: onSelect(index, label)
sequenceDiagram
participant Caller
participant Workspace
participant WorkspaceStorage
Caller->>Workspace: save(storage)
Workspace->>WorkspaceStorage: save(name, JSON.stringify(layouts))
Caller->>Workspace: restore(storage)
Workspace->>WorkspaceStorage: load(name)
WorkspaceStorage-->>Workspace: data or null
Workspace->>Workspace: parse and replace layouts map
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.1)packages/widgets/src/input/TextInput.tsFile contains syntax errors that prevent linting: Line 33: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '; Line 44: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '; Line 48: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '; Line 48: expected a semicolon to end the class property, but found none; Line 48: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '; Line 119: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '; Line 122: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '; Line 134: Expected an identifier, a string literal, a number literal, a private field nam ... [truncated 1178 characters] ... cted a statement but instead found '<<<<<<< HEAD ======='.; Line 412: Expected a statement but instead found '>>>>>>> upstream/main'.; Line 419: Expected a case but instead found '<<<<<<< HEAD =======
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/widgets/src/input/TextInput.ts (1)
524-579: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the unresolved merge conflict in
_renderSelf. The method still contains<<<<<<< HEAD/=======/>>>>>>> upstream/mainmarkers, nestedif (this.isFocused)blocks, and duplicate cursor/suggestion rendering. Keep one rendering path and delete the conflict remnants so the file compiles cleanly.🤖 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/widgets/src/input/TextInput.ts` around lines 524 - 579, The `_renderSelf` method still has unresolved merge conflict markers and duplicated focused-rendering logic. Remove the `<<<<<<< HEAD`/`=======`/`>>>>>>> upstream/main` remnants, collapse the nested `if (this.isFocused)` blocks into one path, and keep a single consistent implementation for cursor, mode indicator/counter, and inline suggestions rendering in `TextInput`.packages/widgets/src/display/Breadcrumbs.ts (1)
41-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_selectedIndexisn't clamped when segments shrink.
setSegmentsdoesn't reset or clamp_selectedIndex. If a caller replaces segments with a shorter array,_selectedIndexcan point past the new array's end, sothis._segments[this._selectedIndex]in theenterbranch (line 65) would beundefined, and_onSelectwould fire with anundefinedlabel; also no segment would render as active sinceoriginalIndexnever matches.🛡️ Proposed fix
setSegments(segments: string[]): void { if ( this._segments.length === segments.length && this._segments.every((segment, index) => segment === segments[index]) ) { return; } this._segments = segments; + this._selectedIndex = Math.min(this._selectedIndex, Math.max(0, segments.length - 1)); this.markDirty(); }Also applies to: 62-67
🤖 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/widgets/src/display/Breadcrumbs.ts` around lines 41 - 50, `Breadcrumbs.setSegments` leaves `_selectedIndex` untouched when the segment list changes, so a shorter array can make the current selection point past the end. Update `setSegments` to clamp or reset `_selectedIndex` whenever `this._segments` is replaced, and keep `enter`/selection rendering in `Breadcrumbs` safe by ensuring `this._segments[this._selectedIndex]` always resolves to a valid segment before calling `_onSelect`.
🧹 Nitpick comments (5)
packages/widgets/src/input/TextInput.ts (1)
73-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate assignment of
_suggestions.
this._suggestions = options.suggestions ?? []is set on line 73 and again on line 76 (under the "Auto-complete data" comment). Remove one.♻️ Proposed cleanup
this._onChange = options.onChange; this._onSubmit = options.onSubmit; - this._suggestions = options.suggestions ?? []; - - // Auto-complete data this._suggestions = options.suggestions ?? [];🤖 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/widgets/src/input/TextInput.ts` around lines 73 - 76, The TextInput constructor is assigning `_suggestions` twice with the same `options.suggestions ?? []` value, creating redundant duplicate initialization. Remove the extra assignment in `TextInput` and keep a single `_suggestions` initialization near the auto-complete setup so the constructor only sets it once.packages/widgets/src/display/Breadcrumbs.ts (1)
116-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
isLast— it no longer reflects its meaning.The variable now indicates the selected segment, not the last one; the stale name hurts readability.
♻️ Proposed rename
- const isLast = originalIndex === this._selectedIndex; + const isSelected = originalIndex === this._selectedIndex; renderList.push({ - type: isLast ? 'active' : 'normal', + type: isSelected ? 'active' : 'normal', text: visibleSegments[i] });🤖 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/widgets/src/display/Breadcrumbs.ts` at line 116, The variable name in Breadcrumbs no longer matches its purpose: the boolean computed from originalIndex and this._selectedIndex identifies the selected segment, not the last segment. Rename the local in Breadcrumbs to something like selected/ isSelected and update its usages in the same rendering logic so the name reflects the actual meaning.packages/widgets/src/layout/Workspace.ts (2)
36-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
switchWorkspacefails silently for unknown names.No error, return value, or log indicates whether the switch succeeded, making it easy for callers to assume they're on a different workspace when they're not.
♻️ Suggested tweak
- switchWorkspace(name: string): void { + switchWorkspace(name: string): boolean { if (this.layouts.has(name)) { this.activeWorkspace = name; + return true; } + return false; }🤖 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/widgets/src/layout/Workspace.ts` around lines 36 - 40, switchWorkspace currently does nothing when the requested workspace name is unknown, so callers cannot tell whether the active workspace changed. Update Workspace.switchWorkspace to surface success or failure by either returning a boolean, throwing on unknown names, or logging a clear failure, and make the behavior explicit when this.layouts does not contain the requested name. Use the existing switchWorkspace method and activeWorkspace/layouts members to locate the change.
58-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for
save()/restore().The provided
Workspace.test.tssnippet covers only the in-memory CRUD operations;save()/restore()— the code paths most likely to break due to serialization or adapter mismatches — have no test coverage. Given the key-mismatch bug flagged above, tests exercising a mockWorkspaceStorageacrosssave→restoreround-trips (including with a switched workspace) would have caught this.🤖 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/widgets/src/layout/Workspace.ts` around lines 58 - 73, Add test coverage for the Workspace save/restore flow in Workspace.test.ts by exercising the Workspace.save and Workspace.restore methods with a mocked WorkspaceStorage. Create a round-trip test that saves the current layouts, restores them into a new Workspace instance, and verifies the layouts are preserved, then add a second case that switches activeWorkspace before restore to catch key mismatches between active workspace selection and stored data. Use the Workspace and WorkspaceStorage symbols to locate the affected paths.packages/widgets/src/layout/Workspace.test.ts (1)
1-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for
save()/restore()persistence viaWorkspaceStorage.The suite covers
saveLayout/loadLayout,switchWorkspace,getWorkspaceNames,removeWorkspace, andclear, but not thesave(storage)/restore(storage)methods that serialize/deserialize layouts through aWorkspaceStorageadapter — the core feature of this cohort ("Persistence via WorkspaceStorage"). A round-trip test with a simple in-memory mock storage would catch serialization regressions (e.g., Map↔JSON conversion issues).✅ Suggested additional test
+ it('persists and restores layouts via storage', () => { + const store = new Map<string, string>(); + const storage = { + save: (key: string, data: string) => store.set(key, data), + load: (key: string) => store.get(key), + }; + + const workspace = new Workspace(); + workspace.saveLayout('dashboard', { id: 'dashboard', panels: [] }); + workspace.save(storage); + + const restored = new Workspace(); + restored.save(storage); // no-op, just to align activeWorkspace key if needed + restored.restore(storage); + + expect(restored.loadLayout('dashboard')).toEqual({ id: 'dashboard', panels: [] }); + });🤖 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/widgets/src/layout/Workspace.test.ts` around lines 1 - 85, The Workspace test suite is missing coverage for the persistence API exposed by Workspace.save and Workspace.restore. Add a round-trip test using a simple in-memory WorkspaceStorage mock to verify that layouts are serialized, saved, restored, and reloaded correctly, and that the current workspace and workspace names survive the save/restore flow. Use the existing Workspace class methods and a mocked storage adapter to catch Map-to-JSON or deserialization regressions.
🤖 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/widgets/src/index.ts`:
- Around line 131-138: The public barrel in index.ts is missing the
SplitDirection export, while the Workspace-related type exports are already
correct. Add SplitDirection back to the existing export list alongside
SplitPaneOptions in the widgets index so consumers can import it from the
package root, and keep the current Workspace re-exports unchanged.
In `@packages/widgets/src/input/TextInput.ts`:
- Around line 33-48: Resolve all Git merge conflict markers in TextInput by
manually choosing the correct implementation for each duplicated hunk so the
file is valid TypeScript again. Keep a single consistent definition for the
_vimMode, _suggestions, and _suggestionIndex fields in TextInput, and ensure
methods like getSuggestions, insertChar, handleKey, and _renderSelf are present
only once with coherent control flow. Remove every <<<<<<<, =======, and >>>>>>>
marker and verify the resolved behavior does not duplicate _onChange calls or
suggestion-index resets.
In `@packages/widgets/src/layout/Workspace.ts`:
- Around line 65-72: The restore path in Workspace.restore currently feeds
JSON.parse(data) straight into this.layouts without validation or error
handling. Wrap the parse in try/catch, validate that the parsed value is a
Map-compatible iterable of WorkspaceLayout entries before assigning, and fall
back safely if storage is corrupted or from an incompatible schema. Keep the fix
localized to Workspace.restore and the this.layouts assignment so malformed
payloads cannot become active state.
- Around line 58-73: `Workspace.save()` and `Workspace.restore()` currently use
the mutable `activeWorkspace` as the storage key, which breaks round-tripping
when `switchWorkspace` changes the active workspace. Update both methods in
`Workspace` to read and write the serialized `layouts` map under a fixed, stable
storage identifier for the whole workspace collection instead of
`this.activeWorkspace`, so `restore()` can always find the saved data regardless
of the current active workspace.
---
Outside diff comments:
In `@packages/widgets/src/display/Breadcrumbs.ts`:
- Around line 41-50: `Breadcrumbs.setSegments` leaves `_selectedIndex` untouched
when the segment list changes, so a shorter array can make the current selection
point past the end. Update `setSegments` to clamp or reset `_selectedIndex`
whenever `this._segments` is replaced, and keep `enter`/selection rendering in
`Breadcrumbs` safe by ensuring `this._segments[this._selectedIndex]` always
resolves to a valid segment before calling `_onSelect`.
In `@packages/widgets/src/input/TextInput.ts`:
- Around line 524-579: The `_renderSelf` method still has unresolved merge
conflict markers and duplicated focused-rendering logic. Remove the `<<<<<<<
HEAD`/`=======`/`>>>>>>> upstream/main` remnants, collapse the nested `if
(this.isFocused)` blocks into one path, and keep a single consistent
implementation for cursor, mode indicator/counter, and inline suggestions
rendering in `TextInput`.
---
Nitpick comments:
In `@packages/widgets/src/display/Breadcrumbs.ts`:
- Line 116: The variable name in Breadcrumbs no longer matches its purpose: the
boolean computed from originalIndex and this._selectedIndex identifies the
selected segment, not the last segment. Rename the local in Breadcrumbs to
something like selected/ isSelected and update its usages in the same rendering
logic so the name reflects the actual meaning.
In `@packages/widgets/src/input/TextInput.ts`:
- Around line 73-76: The TextInput constructor is assigning `_suggestions` twice
with the same `options.suggestions ?? []` value, creating redundant duplicate
initialization. Remove the extra assignment in `TextInput` and keep a single
`_suggestions` initialization near the auto-complete setup so the constructor
only sets it once.
In `@packages/widgets/src/layout/Workspace.test.ts`:
- Around line 1-85: The Workspace test suite is missing coverage for the
persistence API exposed by Workspace.save and Workspace.restore. Add a
round-trip test using a simple in-memory WorkspaceStorage mock to verify that
layouts are serialized, saved, restored, and reloaded correctly, and that the
current workspace and workspace names survive the save/restore flow. Use the
existing Workspace class methods and a mocked storage adapter to catch
Map-to-JSON or deserialization regressions.
In `@packages/widgets/src/layout/Workspace.ts`:
- Around line 36-40: switchWorkspace currently does nothing when the requested
workspace name is unknown, so callers cannot tell whether the active workspace
changed. Update Workspace.switchWorkspace to surface success or failure by
either returning a boolean, throwing on unknown names, or logging a clear
failure, and make the behavior explicit when this.layouts does not contain the
requested name. Use the existing switchWorkspace method and
activeWorkspace/layouts members to locate the change.
- Around line 58-73: Add test coverage for the Workspace save/restore flow in
Workspace.test.ts by exercising the Workspace.save and Workspace.restore methods
with a mocked WorkspaceStorage. Create a round-trip test that saves the current
layouts, restores them into a new Workspace instance, and verifies the layouts
are preserved, then add a second case that switches activeWorkspace before
restore to catch key mismatches between active workspace selection and stored
data. Use the Workspace and WorkspaceStorage symbols to locate the affected
paths.
🪄 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: fa4c76ec-d2ce-4c8f-a733-eaebb2ecb2e5
📒 Files selected for processing (5)
packages/widgets/src/display/Breadcrumbs.tspackages/widgets/src/index.tspackages/widgets/src/input/TextInput.tspackages/widgets/src/layout/Workspace.test.tspackages/widgets/src/layout/Workspace.ts
| export type { SplitPaneOptions } from './layout/SplitPane.js'; | ||
| export { Workspace } from './layout/Workspace.js'; | ||
|
|
||
| export type { | ||
| WorkspaceLayout, | ||
| WorkspaceOptions, | ||
| WorkspaceStorage, | ||
| } from './layout/Workspace.js'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any remaining usages of SplitDirection that would break after removal from the public index
rg -n 'SplitDirection' --type=ts -C2
# Confirm Workspace.ts actually exports WorkspaceLayout, WorkspaceOptions, WorkspaceStorage as types
fd Workspace.ts packages/widgets/src/layout --exec cat -n {}
# Check widgets tsconfig for compile scope (is `include` limited to non-test files?)
fd tsconfig.json packages/widgets --exec cat -n {}Repository: Karanjot786/TermUI
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== index excerpt ==\n'
cat -n packages/widgets/src/index.ts | sed -n '120,150p'
printf '\n== SplitPane symbols ==\n'
rg -n --context 2 'SplitDirection|SplitPaneOptions|export .*SplitPane' packages/widgets/src/layout/SplitPane.ts packages/widgets/src/index.ts
printf '\n== Workspace symbols ==\n'
rg -n --context 2 'WorkspaceLayout|WorkspaceOptions|WorkspaceStorage|export .*Workspace' packages/widgets/src/layout/Workspace.ts packages/widgets/src/index.ts
printf '\n== SplitDirection usages ==\n'
rg -n --context 2 'SplitDirection' packages/widgets/srcRepository: Karanjot786/TermUI
Length of output: 8699
Restore the SplitDirection export from packages/widgets/src/index.ts. WorkspaceLayout, WorkspaceOptions, and WorkspaceStorage are already re-exported correctly.
🤖 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/widgets/src/index.ts` around lines 131 - 138, The public barrel in
index.ts is missing the SplitDirection export, while the Workspace-related type
exports are already correct. Add SplitDirection back to the existing export list
alongside SplitPaneOptions in the widgets index so consumers can import it from
the package root, and keep the current Workspace re-exports unchanged.
Source: Pipeline failures
| <<<<<<< HEAD | ||
|
|
||
| // Auto-complete | ||
| private _suggestions: string[] = []; | ||
| private _suggestionIndex = 0; | ||
|
|
||
| // Vim mode support | ||
| private _vimMode: VimMode = | ||
| process.env.TERMUI_KEYBINDINGS === 'vim' | ||
| ? 'normal' | ||
| : 'insert'; | ||
| ======= | ||
| private _vimMode: VimMode = process.env.TERMUI_KEYBINDINGS === 'vim' ? 'normal' : 'insert'; | ||
| private _suggestions: string[] = []; | ||
| private _suggestionIndex = 0; | ||
| >>>>>>> upstream/main |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Blocker: unresolved Git merge conflict markers throughout the file.
The file still contains <<<<<<< HEAD, =======, and >>>>>>> upstream/main markers at multiple locations (lines 33/44/48, 119/122/134, 147/170/172, 193/224/250, 312/325, 395/412, 419/421/422, 528/540/559). This is not valid TypeScript and will fail to compile/lint — Biome reports parse errors on each of these lines.
The conflict must be resolved by hand-picking the correct side for each hunk before this can merge. A few resolutions to watch out for while doing so:
- The
_vimMode/_suggestions/_suggestionIndexfield block (33-48) is duplicated across both sides — keep one definition. insertChar(147-172): both sides set_suggestionIndex = 0and call_onChange; ensure the resolved version does each exactly once.getSuggestionsappears on both sides (123-124 vs 197-203) — keep a single definition.handleKey's non-vim block and_renderSelf's cursor block (528-559) are split across the markers; verify the resolved control flow is coherent.
Also applies to: 119-134, 147-172, 193-250, 312-325, 395-412, 419-422, 528-559
🧰 Tools
🪛 Biome (2.5.1)
[error] 33-33: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '<<'.
(parse)
[error] 44-44: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '==='.
(parse)
[error] 48-48: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '>'.
(parse)
[error] 48-48: expected a semicolon to end the class property, but found none
(parse)
[error] 48-48: Expected an identifier, a string literal, a number literal, a private field name, or a computed name but instead found '/'.
(parse)
🪛 GitHub Actions: CI / 0_build-and-test.txt
[error] 33-33: esbuild: Expected identifier but found "<<" at line 33. Build failed.
[error] 33-33: TypeScript (DTS build): Merge conflict marker encountered. (TS1185)
[error] 44-44: TypeScript (DTS build): Merge conflict marker encountered. (TS1185)
[error] 48-48: TypeScript (DTS build): Merge conflict marker encountered. (TS1185)
🪛 GitHub Actions: CI / build-and-test
[error] 33-33: esbuild: Build failed. Expected identifier but found "<<" (merge conflict marker).
[error] 44-44: TypeScript: TS1185 Merge conflict marker encountered.
[error] 48-48: TypeScript: TS1185 Merge conflict marker encountered.
🤖 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/widgets/src/input/TextInput.ts` around lines 33 - 48, Resolve all
Git merge conflict markers in TextInput by manually choosing the correct
implementation for each duplicated hunk so the file is valid TypeScript again.
Keep a single consistent definition for the _vimMode, _suggestions, and
_suggestionIndex fields in TextInput, and ensure methods like getSuggestions,
insertChar, handleKey, and _renderSelf are present only once with coherent
control flow. Remove every <<<<<<<, =======, and >>>>>>> marker and verify the
resolved behavior does not duplicate _onChange calls or suggestion-index resets.
Source: Linters/SAST tools
| save(storage: WorkspaceStorage): void { | ||
| storage.save( | ||
| this.activeWorkspace, | ||
| JSON.stringify([...this.layouts]) | ||
| ); | ||
| } | ||
|
|
||
| restore(storage: WorkspaceStorage): void { | ||
| const data = storage.load(this.activeWorkspace); | ||
|
|
||
| if (!data) return; | ||
|
|
||
| this.layouts = new Map( | ||
| JSON.parse(data) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
save()/restore() key on the mutable activeWorkspace, breaking round-trips.
save() serializes the entire layouts map but stores it under the key this.activeWorkspace, and restore() looks it up the same way. If the active workspace changes between calls (which is the whole point of switchWorkspace), restore() will look under the new name and silently find nothing (line 68 swallows this), or save() will scatter duplicate copies of the full state across multiple storage keys named after whichever workspace happened to be active. The storage key should be a fixed identifier for "all workspaces", decoupled from activeWorkspace.
🐛 Proposed fix
+const WORKSPACE_STORAGE_KEY = "termui:workspaces";
+
export class Workspace {
private layouts = new Map<string, WorkspaceLayout>();
private activeWorkspace: string;
@@
save(storage: WorkspaceStorage): void {
storage.save(
- this.activeWorkspace,
- JSON.stringify([...this.layouts])
+ WORKSPACE_STORAGE_KEY,
+ JSON.stringify({
+ activeWorkspace: this.activeWorkspace,
+ layouts: [...this.layouts],
+ })
);
}
restore(storage: WorkspaceStorage): void {
- const data = storage.load(this.activeWorkspace);
+ const data = storage.load(WORKSPACE_STORAGE_KEY);
if (!data) return;
- this.layouts = new Map(
- JSON.parse(data)
- );
+ const parsed = JSON.parse(data) as {
+ activeWorkspace: string;
+ layouts: [string, WorkspaceLayout][];
+ };
+ this.layouts = new Map(parsed.layouts);
+ this.activeWorkspace = parsed.activeWorkspace;
}
}🤖 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/widgets/src/layout/Workspace.ts` around lines 58 - 73,
`Workspace.save()` and `Workspace.restore()` currently use the mutable
`activeWorkspace` as the storage key, which breaks round-tripping when
`switchWorkspace` changes the active workspace. Update both methods in
`Workspace` to read and write the serialized `layouts` map under a fixed, stable
storage identifier for the whole workspace collection instead of
`this.activeWorkspace`, so `restore()` can always find the saved data regardless
of the current active workspace.
| restore(storage: WorkspaceStorage): void { | ||
| const data = storage.load(this.activeWorkspace); | ||
|
|
||
| if (!data) return; | ||
|
|
||
| this.layouts = new Map( | ||
| JSON.parse(data) | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unvalidated JSON.parse result flows straight into layouts.
JSON.parse(data) returns any and is passed directly into new Map(...) with no shape validation or try/catch. Corrupted or incompatible storage payloads (e.g. from an older schema version) will either throw uncaught or silently populate layouts with malformed entries typed as WorkspaceLayout.
🤖 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/widgets/src/layout/Workspace.ts` around lines 65 - 72, The restore
path in Workspace.restore currently feeds JSON.parse(data) straight into
this.layouts without validation or error handling. Wrap the parse in try/catch,
validate that the parsed value is a Map-compatible iterable of WorkspaceLayout
entries before assigning, and fall back safely if storage is corrupted or from
an incompatible schema. Keep the fix localized to Workspace.restore and the
this.layouts assignment so malformed payloads cannot become active state.
1 participant
Description
Summary
Adds keyboard navigation support to the Breadcrumbs widget, allowing users to navigate between breadcrumb items using the keyboard and trigger selection callbacks.
Changes Made
onSelectcallback support.Why
This improves accessibility and provides a more interactive navigation experience for terminal applications.
Closes #1989
Summary by CodeRabbit