Skip to content

Commit 9d022d4

Browse files
0xMink0xMink
authored
chore: enforce no-floating-promises in activate/ (#251)
* chore: enforce no-floating-promises in activate/ First slice of a ratchet that re-enables @typescript-eslint/no-floating-promises directory by directory. The rule and type-aware linting are scoped to activate/** via a files-block in eslint.config.mjs, so pnpm lint (eslint --max-warnings=0) stays green; later PRs widen the scope. registerCommands.ts had 7 un-awaited postMessageToWebview calls: the 5 in synchronous command handlers are marked void (intentional fire-and-forget); the 2 in async handlers are awaited — one sits inside a try/catch, so awaiting routes a rejected post into the existing error handling. * test: cover changed lines in registerCommands.ts for codecov Per edelauna's review on #251: add specs for the void/await fixes in registerCommands.ts so codecov/patch passes. Ten new tests covering the six handlers touched (settingsButtonClicked, historyButtonClicked, marketplaceButtonClicked, focusInput, acceptInput, toggleAutoApprove) hitting all seven previously-uncovered lines. * fix(activate): wire .catch on void-prefixed postMessageToWebview sites The five bare `void` prefixes in registerCommands.ts satisfy no-floating-promises but rely on ClineProvider.postMessageToWebview having its own try/catch — an implicit contract that a future change could break without notice. Matching #253's pattern, each void site now also installs a .catch arm that logs to outputChannel. Added a parameterized test asserting the .catch arm runs and logs when postMessageToWebview rejects, so a future regression in the implicit-swallow contract is caught at the test boundary. * test(registerCommands): pin await semantics with deferred-promise pattern CodeRabbit nit on the test additions: the two await-asserting tests for focusInput and toggleAutoApprove only verified the call payload, so they would still pass if `await` were replaced with `void` in the handler. Switched both to a deferred-promise pattern that observes the handler's pending state before the underlying postMessageToWebview resolves — now a regression that drops the `await` would be caught at the test boundary. * fix(registerCommands): defensive error handling on toggleAutoApprove + log context Addresses two CodeRabbit review items: - toggleAutoApprove now wraps the await in try/catch logging to outputChannel, matching the defensive posture used on the other handlers (settingsButtonClicked, historyButtonClicked, etc.). An unhandled rejection from postMessageToWebview would otherwise bubble to VS Code's command dispatcher rather than being logged consistently with the rest of the file. - All postMessageToWebview failure log messages now carry a [<command-name>] prefix so multi-failure logs are unambiguous. * docs(test): match comment to setImmediate microtask flush CodeRabbit nit on the test additions: the explanatory comment said the test awaits Promise.resolve() to flush the .catch microtask but the code uses setImmediate. Updated the comment to match the actual implementation. --------- Co-authored-by: 0xMink <260166390+0xMink@users.noreply.github.com>
1 parent ba845d6 commit 9d022d4

3 files changed

Lines changed: 313 additions & 11 deletions

File tree

‎src/activate/__tests__/registerCommands.spec.ts‎

Lines changed: 273 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type { Mock } from "vitest"
22
import * as vscode from "vscode"
33
import { ClineProvider } from "../../core/webview/ClineProvider"
44

5-
import { getVisibleProviderOrLog } from "../registerCommands"
5+
import { getVisibleProviderOrLog, registerCommands, setPanel } from "../registerCommands"
66

77
vi.mock("execa", () => ({
88
execa: vi.fn(),
@@ -25,10 +25,62 @@ vi.mock("vscode", () => ({
2525
},
2626
],
2727
},
28+
commands: {
29+
registerCommand: vi.fn(),
30+
executeCommand: vi.fn(),
31+
},
2832
}))
2933

3034
vi.mock("../../core/webview/ClineProvider")
3135

36+
vi.mock("../../shared/package", () => ({
37+
Package: {
38+
name: "zoo-code",
39+
},
40+
}))
41+
42+
vi.mock("@roo-code/telemetry", () => ({
43+
TelemetryService: {
44+
instance: {
45+
captureTitleButtonClicked: vi.fn(),
46+
},
47+
},
48+
}))
49+
50+
vi.mock("../../utils/focusPanel", () => ({
51+
focusPanel: vi.fn().mockResolvedValue(undefined),
52+
}))
53+
54+
vi.mock("../handleTask", () => ({
55+
handleNewTask: vi.fn(),
56+
}))
57+
58+
vi.mock("../../core/config/importExport", () => ({
59+
importSettingsWithFeedback: vi.fn(),
60+
}))
61+
62+
vi.mock("../../services/code-index/manager", () => ({
63+
CodeIndexManager: {
64+
getInstance: vi.fn(),
65+
},
66+
}))
67+
68+
vi.mock("../../services/mdm/MdmService", () => ({
69+
MdmService: {
70+
getInstance: vi.fn(),
71+
},
72+
}))
73+
74+
vi.mock("../../core/config/ContextProxy", () => ({
75+
ContextProxy: {
76+
getInstance: vi.fn(),
77+
},
78+
}))
79+
80+
vi.mock("../../i18n", () => ({
81+
t: (key: string) => key,
82+
}))
83+
3284
describe("getVisibleProviderOrLog", () => {
3385
let mockOutputChannel: vscode.OutputChannel
3486

@@ -65,3 +117,223 @@ describe("getVisibleProviderOrLog", () => {
65117
expect(mockOutputChannel.appendLine).toHaveBeenCalledWith("Cannot find any visible Roo Code instances.")
66118
})
67119
})
120+
121+
describe("registerCommands handlers", () => {
122+
let mockOutputChannel: vscode.OutputChannel
123+
let mockContext: vscode.ExtensionContext
124+
let mockVisibleProvider: { postMessageToWebview: Mock }
125+
let mockProvider: { postMessageToWebview: Mock }
126+
let handlers: Record<string, (...args: unknown[]) => unknown>
127+
128+
beforeEach(() => {
129+
vi.clearAllMocks()
130+
handlers = {}
131+
132+
mockOutputChannel = {
133+
appendLine: vi.fn(),
134+
append: vi.fn(),
135+
clear: vi.fn(),
136+
hide: vi.fn(),
137+
name: "mock",
138+
replace: vi.fn(),
139+
show: vi.fn(),
140+
dispose: vi.fn(),
141+
}
142+
143+
mockContext = {
144+
subscriptions: [],
145+
} as unknown as vscode.ExtensionContext
146+
147+
mockVisibleProvider = {
148+
postMessageToWebview: vi.fn().mockResolvedValue(undefined),
149+
}
150+
151+
mockProvider = {
152+
postMessageToWebview: vi.fn().mockResolvedValue(undefined),
153+
}
154+
;(ClineProvider.getVisibleInstance as Mock).mockReturnValue(mockVisibleProvider)
155+
;(vscode.commands.registerCommand as Mock).mockImplementation(
156+
(id: string, cb: (...args: unknown[]) => unknown) => {
157+
handlers[id] = cb
158+
return { dispose: vi.fn() }
159+
},
160+
)
161+
162+
registerCommands({
163+
context: mockContext,
164+
outputChannel: mockOutputChannel,
165+
provider: mockProvider as unknown as ClineProvider,
166+
})
167+
})
168+
169+
afterEach(() => {
170+
// Reset module-level panel state to prevent leakage between tests.
171+
setPanel(undefined, "sidebar")
172+
setPanel(undefined, "tab")
173+
})
174+
175+
it("settingsButtonClicked posts both settingsButtonClicked and didBecomeVisible actions", () => {
176+
handlers["zoo-code.settingsButtonClicked"]()
177+
178+
expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({
179+
type: "action",
180+
action: "settingsButtonClicked",
181+
})
182+
expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({
183+
type: "action",
184+
action: "didBecomeVisible",
185+
})
186+
expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledTimes(2)
187+
})
188+
189+
it("settingsButtonClicked is a no-op when no visible provider", () => {
190+
;(ClineProvider.getVisibleInstance as Mock).mockReturnValue(undefined)
191+
192+
handlers["zoo-code.settingsButtonClicked"]()
193+
194+
expect(mockVisibleProvider.postMessageToWebview).not.toHaveBeenCalled()
195+
})
196+
197+
it("historyButtonClicked posts historyButtonClicked action", () => {
198+
handlers["zoo-code.historyButtonClicked"]()
199+
200+
expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({
201+
type: "action",
202+
action: "historyButtonClicked",
203+
})
204+
})
205+
206+
it("marketplaceButtonClicked posts marketplaceButtonClicked action", () => {
207+
handlers["zoo-code.marketplaceButtonClicked"]()
208+
209+
expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({
210+
type: "action",
211+
action: "marketplaceButtonClicked",
212+
})
213+
})
214+
215+
it("acceptInput posts acceptInput message", () => {
216+
handlers["zoo-code.acceptInput"]()
217+
218+
expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({
219+
type: "acceptInput",
220+
})
221+
})
222+
223+
it("toggleAutoApprove awaits postMessage with toggleAutoApprove action", async () => {
224+
// Deferred-promise pattern: pin that the handler actually awaits
225+
// postMessageToWebview rather than fire-and-forgetting it. If `await`
226+
// were dropped in the handler, handlerPromise would resolve before
227+
// resolvePost() is called and `settled` would flip true at the
228+
// microtask flush below, failing the pending-state assertion.
229+
let resolvePost!: () => void
230+
const postPromise = new Promise<void>((resolve) => {
231+
resolvePost = resolve
232+
})
233+
mockVisibleProvider.postMessageToWebview.mockReturnValueOnce(postPromise)
234+
235+
const handlerPromise = handlers["zoo-code.toggleAutoApprove"]() as Promise<unknown>
236+
let settled = false
237+
void handlerPromise.then(() => {
238+
settled = true
239+
})
240+
await Promise.resolve()
241+
expect(settled).toBe(false)
242+
243+
resolvePost()
244+
await handlerPromise
245+
246+
expect(mockVisibleProvider.postMessageToWebview).toHaveBeenCalledWith({
247+
type: "action",
248+
action: "toggleAutoApprove",
249+
})
250+
})
251+
252+
it("focusInput awaits postMessage on the registered provider when a sidebar panel is active", async () => {
253+
const fakeSidebar = {} as vscode.WebviewView
254+
setPanel(fakeSidebar, "sidebar")
255+
256+
// Same deferred-promise pattern as above. focusInput first awaits
257+
// focusPanel() (mocked to resolve sync) and then awaits
258+
// provider.postMessageToWebview — so we flush two microtasks before
259+
// asserting the pending state, to let the handler advance past the
260+
// focusPanel await and suspend on the deferred postPromise.
261+
let resolvePost!: () => void
262+
const postPromise = new Promise<void>((resolve) => {
263+
resolvePost = resolve
264+
})
265+
mockProvider.postMessageToWebview.mockReturnValueOnce(postPromise)
266+
267+
const handlerPromise = handlers["zoo-code.focusInput"]() as Promise<unknown>
268+
let settled = false
269+
void handlerPromise.then(() => {
270+
settled = true
271+
})
272+
await Promise.resolve()
273+
await Promise.resolve()
274+
expect(settled).toBe(false)
275+
276+
resolvePost()
277+
await handlerPromise
278+
279+
expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({
280+
type: "action",
281+
action: "focusInput",
282+
})
283+
})
284+
285+
it("focusInput does not post when no sidebar panel is active", async () => {
286+
await handlers["zoo-code.focusInput"]()
287+
288+
expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled()
289+
})
290+
291+
// Representative coverage for the .catch arm on all five void-prefixed
292+
// postMessageToWebview sites in registerCommands.ts (settingsButtonClicked
293+
// posts twice, plus historyButtonClicked, marketplaceButtonClicked, and
294+
// acceptInput). Each handler is synchronous, so the .catch arm runs on a
295+
// microtask; setImmediate ensures all microtasks are flushed before we assert. The
296+
// log messages carry a `[<handlerName>]` prefix so multi-failure logs
297+
// remain unambiguous; the prefix is per-handler, not per-call (both of
298+
// settingsButtonClicked's posts share the same prefix).
299+
it.each([
300+
{ command: "zoo-code.settingsButtonClicked", prefix: "settingsButtonClicked", expectedCalls: 2 },
301+
{ command: "zoo-code.historyButtonClicked", prefix: "historyButtonClicked", expectedCalls: 1 },
302+
{ command: "zoo-code.marketplaceButtonClicked", prefix: "marketplaceButtonClicked", expectedCalls: 1 },
303+
{ command: "zoo-code.acceptInput", prefix: "acceptInput", expectedCalls: 1 },
304+
])(
305+
"$command logs to outputChannel when postMessageToWebview rejects",
306+
async ({ command, prefix, expectedCalls }) => {
307+
const boom = new Error("boom")
308+
mockVisibleProvider.postMessageToWebview.mockReset()
309+
mockVisibleProvider.postMessageToWebview.mockRejectedValue(boom)
310+
311+
handlers[command]()
312+
313+
// Flush microtasks so the chained .catch arm runs.
314+
await new Promise((resolve) => setImmediate(resolve))
315+
316+
expect(mockOutputChannel.appendLine).toHaveBeenCalledTimes(expectedCalls)
317+
expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
318+
`[${prefix}] postMessageToWebview failed: ${boom}`,
319+
)
320+
},
321+
)
322+
323+
it("toggleAutoApprove logs to outputChannel when postMessageToWebview rejects", async () => {
324+
// toggleAutoApprove is `async` and awaits postMessageToWebview inside a
325+
// try/catch (rather than relying on a `.catch` microtask like the
326+
// void-prefixed sites), so awaiting the handler itself is sufficient to
327+
// observe the appendLine call.
328+
const boom = new Error("boom")
329+
mockVisibleProvider.postMessageToWebview.mockReset()
330+
mockVisibleProvider.postMessageToWebview.mockRejectedValue(boom)
331+
332+
await handlers["zoo-code.toggleAutoApprove"]()
333+
334+
expect(mockOutputChannel.appendLine).toHaveBeenCalledTimes(1)
335+
expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
336+
`[toggleAutoApprove] postMessageToWebview failed: ${boom}`,
337+
)
338+
})
339+
})

‎src/activate/registerCommands.ts‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,13 @@ const getCommandsMap = ({ context, outputChannel, provider }: RegisterCommandOpt
103103

104104
TelemetryService.instance.captureTitleButtonClicked("settings")
105105

106-
visibleProvider.postMessageToWebview({ type: "action", action: "settingsButtonClicked" })
106+
void visibleProvider
107+
.postMessageToWebview({ type: "action", action: "settingsButtonClicked" })
108+
.catch((error) => outputChannel.appendLine(`[settingsButtonClicked] postMessageToWebview failed: ${error}`))
107109
// Also explicitly post the visibility message to trigger scroll reliably
108-
visibleProvider.postMessageToWebview({ type: "action", action: "didBecomeVisible" })
110+
void visibleProvider
111+
.postMessageToWebview({ type: "action", action: "didBecomeVisible" })
112+
.catch((error) => outputChannel.appendLine(`[settingsButtonClicked] postMessageToWebview failed: ${error}`))
109113
},
110114
historyButtonClicked: () => {
111115
const visibleProvider = getVisibleProviderOrLog(outputChannel)
@@ -116,12 +120,18 @@ const getCommandsMap = ({ context, outputChannel, provider }: RegisterCommandOpt
116120

117121
TelemetryService.instance.captureTitleButtonClicked("history")
118122

119-
visibleProvider.postMessageToWebview({ type: "action", action: "historyButtonClicked" })
123+
void visibleProvider
124+
.postMessageToWebview({ type: "action", action: "historyButtonClicked" })
125+
.catch((error) => outputChannel.appendLine(`[historyButtonClicked] postMessageToWebview failed: ${error}`))
120126
},
121127
marketplaceButtonClicked: () => {
122128
const visibleProvider = getVisibleProviderOrLog(outputChannel)
123129
if (!visibleProvider) return
124-
visibleProvider.postMessageToWebview({ type: "action", action: "marketplaceButtonClicked" })
130+
void visibleProvider
131+
.postMessageToWebview({ type: "action", action: "marketplaceButtonClicked" })
132+
.catch((error) =>
133+
outputChannel.appendLine(`[marketplaceButtonClicked] postMessageToWebview failed: ${error}`),
134+
)
125135
},
126136
newTask: handleNewTask,
127137
setCustomStoragePath: async () => {
@@ -150,7 +160,7 @@ const getCommandsMap = ({ context, outputChannel, provider }: RegisterCommandOpt
150160

151161
// Send focus input message only for sidebar panels
152162
if (sidebarPanel && getPanel() === sidebarPanel) {
153-
provider.postMessageToWebview({ type: "action", action: "focusInput" })
163+
await provider.postMessageToWebview({ type: "action", action: "focusInput" })
154164
}
155165
} catch (error) {
156166
outputChannel.appendLine(`Error focusing input: ${error}`)
@@ -170,7 +180,9 @@ const getCommandsMap = ({ context, outputChannel, provider }: RegisterCommandOpt
170180
return
171181
}
172182

173-
visibleProvider.postMessageToWebview({ type: "acceptInput" })
183+
void visibleProvider
184+
.postMessageToWebview({ type: "acceptInput" })
185+
.catch((error) => outputChannel.appendLine(`[acceptInput] postMessageToWebview failed: ${error}`))
174186
},
175187
toggleAutoApprove: async () => {
176188
const visibleProvider = getVisibleProviderOrLog(outputChannel)
@@ -179,10 +191,14 @@ const getCommandsMap = ({ context, outputChannel, provider }: RegisterCommandOpt
179191
return
180192
}
181193

182-
visibleProvider.postMessageToWebview({
183-
type: "action",
184-
action: "toggleAutoApprove",
185-
})
194+
try {
195+
await visibleProvider.postMessageToWebview({
196+
type: "action",
197+
action: "toggleAutoApprove",
198+
})
199+
} catch (error) {
200+
outputChannel.appendLine(`[toggleAutoApprove] postMessageToWebview failed: ${error}`)
201+
}
186202
},
187203
})
188204

‎src/eslint.config.mjs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,20 @@ export default [
2828
"no-undef": "off",
2929
},
3030
},
31+
{
32+
// Ratchet: enforce no-floating-promises directory by directory. Each
33+
// directory is added here once its floating promises are resolved.
34+
files: ["activate/**/*.ts"],
35+
languageOptions: {
36+
parserOptions: {
37+
project: true,
38+
tsconfigRootDir: import.meta.dirname,
39+
},
40+
},
41+
rules: {
42+
"@typescript-eslint/no-floating-promises": "error",
43+
},
44+
},
3145
{
3246
ignores: ["webview-ui", "out"],
3347
},

0 commit comments

Comments
 (0)