Skip to content

Commit ef68402

Browse files
author
Vincent M.
committed
fix: address remaining code review findings (H2/H3/H4, M2-M6, L14/L18/N3)
- Prevent concurrent state mutation by processing file updates sequentially - Add error handling to command handlers and scanAndSendFileTree - Add keyboard navigation (arrow keys, Home/End) to overflow menu dropdown - Fix DashboardViewProvider disposal leak via onDidDispose cleanup - Normalize path separators before includes() checks for cross-platform reliability - Fix stale closure in story-detail-view useEffect dependencies - Add aria-selected and aria-expanded to file tree items - Cap error array at 50 entries in state-manager collectError - Use node:path import for consistency
1 parent 16ad2f9 commit ef68402

8 files changed

Lines changed: 118 additions & 26 deletions

File tree

src/extension/extension.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@ import { BmadDetector, FileWatcher, StateManager, WorkflowDiscoveryService } fro
33
import { DashboardViewProvider } from './providers/dashboard-view-provider';
44
import { EditorPanelProvider } from './providers/editor-panel-provider';
55

6+
function showCommandError(action: string, err: unknown): Thenable<string | undefined> {
7+
const msg = err instanceof Error ? err.message : String(err);
8+
return vscode.window.showErrorMessage(`BMAD ${action} failed: ${msg}`);
9+
}
10+
611
export async function activate(context: vscode.ExtensionContext): Promise<void> {
712
// Detect BMAD project in current workspace
813
const detector = new BmadDetector();
@@ -32,11 +37,19 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
3237

3338
context.subscriptions.push(
3439
vscode.window.registerWebviewViewProvider(DashboardViewProvider.viewType, dashboardProvider),
35-
vscode.commands.registerCommand('bmad.refresh', () => {
36-
void stateManager.refresh();
40+
vscode.commands.registerCommand('bmad.refresh', async () => {
41+
try {
42+
await stateManager.refresh();
43+
} catch (err) {
44+
void showCommandError('refresh', err);
45+
}
3746
}),
3847
vscode.commands.registerCommand('bmad.openEditorPanel', () => {
39-
EditorPanelProvider.createOrShow(context.extensionUri, stateManager);
48+
try {
49+
EditorPanelProvider.createOrShow(context.extensionUri, stateManager);
50+
} catch (err) {
51+
void showCommandError('open panel', err);
52+
}
4053
}),
4154
// Dispose editor panel on extension deactivation (singleton may be open)
4255
{ dispose: () => EditorPanelProvider.disposePanel() },

src/extension/parsers/epic-parser.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,7 @@ function parseStoryEntries(content: string): EpicStoryEntry[] {
8383
const current = matches[i];
8484
// Calculate end boundary for this story's content section.
8585
// For the last story, use end of content. For others, use the start of the next header.
86-
const nextIndex =
87-
i + 1 < matches.length ? matches[i + 1].startIndex : content.length;
86+
const nextIndex = i + 1 < matches.length ? matches[i + 1].startIndex : content.length;
8887

8988
// Get content between this story header and the next (or end)
9089
const storyContent = content.slice(current.index, nextIndex);

src/extension/providers/dashboard-view-provider.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,14 @@ export class DashboardViewProvider implements vscode.WebviewViewProvider {
7878
this.disposables
7979
);
8080
}
81+
82+
// Clean up disposables when the webview is disposed
83+
webviewView.onDidDispose(() => {
84+
for (const d of this.disposables) {
85+
d.dispose();
86+
}
87+
this.disposables.length = 0;
88+
});
8189
}
8290

8391
/**

src/extension/providers/editor-panel-provider.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as vscode from 'vscode';
2-
import * as path from 'path';
2+
import * as path from 'node:path';
33
import matter from 'gray-matter';
44
import type { StateManager } from '../services/state-manager';
55
import {
@@ -169,9 +169,14 @@ export class EditorPanelProvider implements vscode.Disposable {
169169
* Scan document library directories and send the file tree to the webview.
170170
*/
171171
private async scanAndSendFileTree(): Promise<void> {
172-
const scanner = new FileTreeScanner();
173-
const roots = await scanner.scan();
174-
void this.panel.webview.postMessage(createFileTreeMessage(roots));
172+
try {
173+
const scanner = new FileTreeScanner();
174+
const roots = await scanner.scan();
175+
void this.panel.webview.postMessage(createFileTreeMessage(roots));
176+
} catch (err) {
177+
// eslint-disable-next-line no-console
178+
console.debug('[BMAD] Failed to scan file tree:', err);
179+
}
175180
}
176181

177182
public dispose(): void {

src/extension/services/state-manager.ts

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { createInitialDashboardState, isStoryKey, isEpicKey } from '../../shared
2323
* 'readme.md' → false (doesn't match X-Y-name pattern)
2424
*/
2525
const STORY_FILE_REGEX = /^\d+-\d+[a-z]?-[\w-]+\.md$/;
26+
const MAX_STORED_ERRORS = 50;
2627

2728
/**
2829
* Service for managing aggregated BMAD dashboard state.
@@ -516,14 +517,15 @@ export class StateManager implements vscode.Disposable {
516517
this.removeFromState(filePath);
517518
}
518519

519-
// Process creates/changes in parallel
520+
// Process creates/changes sequentially to avoid concurrent state mutation
520521
const updateChanges = Array.from(event.changes.entries()).filter(
521522
([, changeType]) => changeType !== 'delete'
522523
);
523-
const updatePromises = updateChanges.map(async ([filePath]) =>
524-
this.handleFileUpdate(filePath, paths.outputRoot!)
525-
);
526-
await Promise.all(updatePromises);
524+
for (const [filePath] of updateChanges) {
525+
// Sequential processing is intentional — prevents concurrent state mutation
526+
// eslint-disable-next-line no-await-in-loop
527+
await this.handleFileUpdate(filePath, paths.outputRoot);
528+
}
527529

528530
this.determineCurrentStory();
529531
this.buildStorySummaries();
@@ -543,11 +545,13 @@ export class StateManager implements vscode.Disposable {
543545
* Handle a file update (create or change).
544546
*/
545547
private async handleFileUpdate(filePath: string, outputRoot: vscode.Uri): Promise<void> {
546-
const fileName = this.getFileName(filePath);
548+
// Normalize path separators for reliable matching on all platforms
549+
const normalizedPath = filePath.replace(/\\/g, '/');
550+
const fileName = normalizedPath.split('/').pop()!;
547551

548552
if (fileName === 'sprint-status.yaml') {
549553
await this.parseSprintStatus(outputRoot);
550-
} else if (filePath.includes('planning-artifacts') && fileName === 'epics.md') {
554+
} else if (normalizedPath.includes('planning-artifacts') && fileName === 'epics.md') {
551555
await this.parseEpics(outputRoot);
552556
this._state = {
553557
...this._state,
@@ -557,14 +561,14 @@ export class StateManager implements vscode.Disposable {
557561
},
558562
};
559563
} else if (
560-
filePath.includes('planning-artifacts') &&
564+
normalizedPath.includes('planning-artifacts') &&
561565
(fileName === 'prd.md' ||
562566
fileName === 'architecture.md' ||
563567
this.isProductBriefFile(fileName) ||
564568
this.isReadinessReportFile(fileName))
565569
) {
566570
await this.detectPlanningArtifacts(outputRoot);
567-
} else if (filePath.includes('implementation-artifacts') && this.isStoryFile(fileName)) {
571+
} else if (normalizedPath.includes('implementation-artifacts') && this.isStoryFile(fileName)) {
568572
await this.parseStoryFile(vscode.Uri.file(filePath));
569573
}
570574
// Other files are ignored (not relevant to BMAD dashboard)
@@ -670,8 +674,8 @@ export class StateManager implements vscode.Disposable {
670674
? this._state.errors.filter((e) => e.filePath !== error.filePath)
671675
: this._state.errors;
672676

673-
// Add the new error
674-
this._state = { ...this._state, errors: [...errors, error] };
677+
// Add the new error (cap to prevent unbounded growth)
678+
this._state = { ...this._state, errors: [...errors, error].slice(-MAX_STORED_ERRORS) };
675679
}
676680

677681
/**

src/webviews/dashboard/components/header-toolbar.tsx

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import React, { useState, useRef, useEffect, useCallback } from 'react';
1+
import React, { useState, useRef, useEffect, useCallback, type KeyboardEvent } from 'react';
22
import { HelpCircle, EllipsisVertical, RefreshCw, Play, PanelRight } from 'lucide-react';
33
import { useWorkflows } from '../store';
44
import { useVSCodeApi } from '../../shared/hooks';
@@ -56,13 +56,51 @@ export function HeaderToolbar(): React.ReactElement {
5656
// Escape key dismiss
5757
useEffect(() => {
5858
if (!isOpen) return;
59-
const handleEscape = (e: KeyboardEvent) => {
59+
const handleEscape = (e: globalThis.KeyboardEvent) => {
6060
if (e.key === 'Escape') setIsOpen(false);
6161
};
6262
document.addEventListener('keydown', handleEscape);
6363
return () => document.removeEventListener('keydown', handleEscape);
6464
}, [isOpen]);
6565

66+
// Focus first menu item when menu opens
67+
const menuItemsRef = useRef<(HTMLButtonElement | null)[]>([]);
68+
useEffect(() => {
69+
if (isOpen) {
70+
menuItemsRef.current[0]?.focus();
71+
}
72+
}, [isOpen]);
73+
74+
// Arrow key navigation for menu items
75+
const handleMenuKeyDown = useCallback((e: KeyboardEvent<HTMLDivElement>) => {
76+
const items = menuItemsRef.current.filter(Boolean) as HTMLButtonElement[];
77+
const currentIndex = items.indexOf(document.activeElement as HTMLButtonElement);
78+
let nextIndex = -1;
79+
80+
switch (e.key) {
81+
case 'ArrowDown':
82+
e.preventDefault();
83+
nextIndex = currentIndex + 1 >= items.length ? 0 : currentIndex + 1;
84+
break;
85+
case 'ArrowUp':
86+
e.preventDefault();
87+
nextIndex = currentIndex - 1 < 0 ? items.length - 1 : currentIndex - 1;
88+
break;
89+
case 'Home':
90+
e.preventDefault();
91+
nextIndex = 0;
92+
break;
93+
case 'End':
94+
e.preventDefault();
95+
nextIndex = items.length - 1;
96+
break;
97+
}
98+
99+
if (nextIndex >= 0) {
100+
items[nextIndex].focus();
101+
}
102+
}, []);
103+
66104
const iconButtonClass =
67105
'rounded p-1 text-[var(--vscode-descriptionForeground)] hover:text-[var(--vscode-foreground)] hover:bg-[var(--vscode-toolbar-hoverBackground)] focus:ring-1 focus:ring-[var(--vscode-focusBorder)] focus:outline-none';
68106

@@ -96,11 +134,16 @@ export function HeaderToolbar(): React.ReactElement {
96134
<div
97135
role="menu"
98136
data-testid="overflow-menu-dropdown"
137+
onKeyDown={handleMenuKeyDown}
99138
className="animate-expand-in absolute top-full right-0 z-10 mt-1 min-w-[160px] rounded border border-[var(--vscode-menu-border)] bg-[var(--vscode-menu-background)] py-1 shadow-md"
100139
>
101140
<button
141+
ref={(el) => {
142+
menuItemsRef.current[0] = el;
143+
}}
102144
role="menuitem"
103145
type="button"
146+
tabIndex={-1}
104147
data-testid="overflow-menu-open-panel"
105148
onClick={handleOpenPanel}
106149
className="flex w-full items-center gap-2 px-3 py-1.5 text-left text-xs text-[var(--vscode-menu-foreground)] hover:bg-[var(--vscode-menu-selectionBackground)]"
@@ -109,20 +152,28 @@ export function HeaderToolbar(): React.ReactElement {
109152
Open Tab View
110153
</button>
111154
<button
155+
ref={(el) => {
156+
menuItemsRef.current[1] = el;
157+
}}
112158
role="menuitem"
113159
type="button"
160+
tabIndex={-1}
114161
data-testid="overflow-menu-refresh"
115162
onClick={handleRefresh}
116163
className="flex w-full items-center gap-2 px-3 py-1.5 text-left text-xs text-[var(--vscode-menu-foreground)] hover:bg-[var(--vscode-menu-selectionBackground)]"
117164
>
118165
<RefreshCw size={12} />
119166
Refresh
120167
</button>
121-
{workflows.map((workflow) => (
168+
{workflows.map((workflow, index) => (
122169
<button
170+
ref={(el) => {
171+
menuItemsRef.current[index + 2] = el;
172+
}}
123173
key={workflow.id}
124174
role="menuitem"
125175
type="button"
176+
tabIndex={-1}
126177
data-testid={`overflow-menu-workflow-${workflow.id}`}
127178
onClick={() => handleWorkflowClick(workflow.command)}
128179
title={workflow.description}

src/webviews/editor-panel/components/file-tree.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,11 @@ function FileTreeItem({
125125
);
126126

127127
return (
128-
<div role="treeitem">
128+
<div
129+
role="treeitem"
130+
aria-selected={isSelected}
131+
aria-expanded={isDirectory ? isExpanded : undefined}
132+
>
129133
<button
130134
data-testid={`file-tree-item-${node.path}`}
131135
className="flex w-full items-center gap-1.5 rounded px-1.5 py-0.5 text-left hover:bg-[var(--vscode-list-hoverBackground)]"

src/webviews/editor-panel/views/story-detail-view.tsx

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,18 +55,26 @@ export function StoryDetailView(): React.ReactElement {
5555
);
5656

5757
// Load story detail on mount when needed
58+
const storyDetailKey = storyDetail?.key;
5859
useEffect(() => {
5960
if (!summary?.filePath) return;
6061
// If we already have the correct story loaded, skip
61-
if (storyDetail?.key === storyKey) return;
62+
if (storyDetailKey === storyKey) return;
6263

6364
setStoryDetailLoading(true);
6465
vscodeApi.postMessage(createRequestDocumentContentMessage(summary.filePath));
6566

6667
return () => {
6768
clearStoryDetail();
6869
};
69-
}, [storyKey, summary?.filePath]); // eslint-disable-line react-hooks/exhaustive-deps
70+
}, [
71+
storyKey,
72+
summary?.filePath,
73+
storyDetailKey,
74+
setStoryDetailLoading,
75+
vscodeApi,
76+
clearStoryDetail,
77+
]);
7078

7179
if (!storyKey) {
7280
return (

0 commit comments

Comments
 (0)