chore(lint): add core/webview to no-floating-promises ratchet#950
Open
morgan-coded wants to merge 1 commit into
Open
chore(lint): add core/webview to no-floating-promises ratchet#950morgan-coded wants to merge 1 commit into
morgan-coded wants to merge 1 commit into
Conversation
morgan-coded
requested review from
edelauna,
hannesrudolph and
navedmerchant
as code owners
July 19, 2026 16:55
Contributor
📝 WalkthroughWalkthroughWebview lifecycle, message handlers, file access, indexing, marketplace operations, and supporting handlers now explicitly await required asynchronous work or mark intentional fire-and-forget calls. ESLint enforcement now covers ChangesWebview asynchronous control
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/core/webview/ClineProvider.ts (1)
845-871: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer destructuring with
awaitover.then().Mixing
awaitwith.then()is less idiomatic than using standard async/await destructuring. Consider refactoring this initialization block to use standard destructuring for better readability.♻️ Proposed refactor
- await this.getState().then( - ({ - terminalShellIntegrationTimeout = Terminal.defaultShellIntegrationTimeout, - terminalShellIntegrationDisabled = false, - terminalCommandDelay = 0, - terminalZshClearEolMark = true, - terminalZshOhMy = false, - terminalZshP10k = false, - terminalPowershellCounter = false, - terminalZdotdir = false, - terminalProfile, - ttsEnabled, - ttsSpeed, - }) => { - Terminal.setShellIntegrationTimeout(terminalShellIntegrationTimeout) - Terminal.setShellIntegrationDisabled(terminalShellIntegrationDisabled) - Terminal.setCommandDelay(terminalCommandDelay) - Terminal.setTerminalZshClearEolMark(terminalZshClearEolMark) - Terminal.setTerminalZshOhMy(terminalZshOhMy) - Terminal.setTerminalZshP10k(terminalZshP10k) - Terminal.setPowershellCounter(terminalPowershellCounter) - Terminal.setTerminalZdotdir(terminalZdotdir) - Terminal.setTerminalProfile(terminalProfile) - setTtsEnabled(ttsEnabled ?? false) - setTtsSpeed(ttsSpeed ?? 1) - }, - ) + const { + terminalShellIntegrationTimeout = Terminal.defaultShellIntegrationTimeout, + terminalShellIntegrationDisabled = false, + terminalCommandDelay = 0, + terminalZshClearEolMark = true, + terminalZshOhMy = false, + terminalZshP10k = false, + terminalPowershellCounter = false, + terminalZdotdir = false, + terminalProfile, + ttsEnabled, + ttsSpeed, + } = await this.getState() + + Terminal.setShellIntegrationTimeout(terminalShellIntegrationTimeout) + Terminal.setShellIntegrationDisabled(terminalShellIntegrationDisabled) + Terminal.setCommandDelay(terminalCommandDelay) + Terminal.setTerminalZshClearEolMark(terminalZshClearEolMark) + Terminal.setTerminalZshOhMy(terminalZshOhMy) + Terminal.setTerminalZshP10k(terminalZshP10k) + Terminal.setPowershellCounter(terminalPowershellCounter) + Terminal.setTerminalZdotdir(terminalZdotdir) + Terminal.setTerminalProfile(terminalProfile) + setTtsEnabled(ttsEnabled ?? false) + setTtsSpeed(ttsSpeed ?? 1)🤖 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 `@src/core/webview/ClineProvider.ts` around lines 845 - 871, Refactor the initialization block in ClineProvider to await getState() directly and destructure the returned state in the surrounding async method, then keep the existing Terminal setters and TTS defaults unchanged.src/core/webview/checkpointRestoreHandler.ts (1)
34-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant polling loop.
Now that
currentCline.abortTask()is explicitly awaited, the task is guaranteed to have completed its abort sequence (andthis.abortis synchronously set totruewithin it). The subsequentpWaitForpolling loop is therefore redundant.♻️ Proposed refactor
await currentCline.abortTask() - // Wait a bit for the abort to complete - await pWaitFor(() => currentCline.abort === true, { - timeout: 1000, - interval: 50, - }).catch(() => { - // Continue even if timeout - the abort flag should be set - }) }🤖 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 `@src/core/webview/checkpointRestoreHandler.ts` around lines 34 - 41, Remove the redundant pWaitFor polling and its timeout catch after awaiting currentCline.abortTask() in the checkpoint restore flow; rely on the awaited abortTask completion and synchronous abort flag update before continuing.src/core/webview/webviewMessageHandler.ts (2)
1298-1302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the switch case in a block.
To prevent potential lexical scope pollution and resolve the
noSwitchDeclarationsstatic analysis warning, wrap the case logic in a block.♻️ Proposed refactor
- case "requestVsCodeLmModels": + case "requestVsCodeLmModels": { const vsCodeLmModels = await getVsCodeLmModels() // TODO: Cache like we do for OpenRouter, etc? await provider.postMessageToWebview({ type: "vsCodeLmModels", vsCodeLmModels }) break + }🤖 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 `@src/core/webview/webviewMessageHandler.ts` around lines 1298 - 1302, Wrap the "requestVsCodeLmModels" switch case body in a block so the declarations of vsCodeLmModels remain scoped to that case and satisfy noSwitchDeclarations, preserving the existing model retrieval and provider.postMessageToWebview behavior.Source: Linters/SAST tools
3006-3016: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant initialization block.
Since
await manager.initialize(provider.contextProxy)is already called successfully a few lines above (on line 3004) before reaching this block, checking!manager.isInitializedand attempting to initialize and start indexing again is redundant.♻️ Proposed refactor
const currentState = manager.state if (currentState === "Standby" || currentState === "Error") { void manager.startIndexing() - - if (!manager.isInitialized) { - await manager.initialize(provider.contextProxy) - if (manager.state === "Standby" || manager.state === "Error") { - void manager.startIndexing() - } - } }🤖 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 `@src/core/webview/webviewMessageHandler.ts` around lines 3006 - 3016, Remove the redundant !manager.isInitialized check and its nested manager.initialize(provider.contextProxy) and second manager.startIndexing() call from the currentState handling block. Keep the initial manager.startIndexing() behavior for Standby and Error states unchanged, relying on the earlier successful initialization.
🤖 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.
Nitpick comments:
In `@src/core/webview/checkpointRestoreHandler.ts`:
- Around line 34-41: Remove the redundant pWaitFor polling and its timeout catch
after awaiting currentCline.abortTask() in the checkpoint restore flow; rely on
the awaited abortTask completion and synchronous abort flag update before
continuing.
In `@src/core/webview/ClineProvider.ts`:
- Around line 845-871: Refactor the initialization block in ClineProvider to
await getState() directly and destructure the returned state in the surrounding
async method, then keep the existing Terminal setters and TTS defaults
unchanged.
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 1298-1302: Wrap the "requestVsCodeLmModels" switch case body in a
block so the declarations of vsCodeLmModels remain scoped to that case and
satisfy noSwitchDeclarations, preserving the existing model retrieval and
provider.postMessageToWebview behavior.
- Around line 3006-3016: Remove the redundant !manager.isInitialized check and
its nested manager.initialize(provider.contextProxy) and second
manager.startIndexing() call from the currentState handling block. Keep the
initial manager.startIndexing() behavior for Standby and Error states unchanged,
relying on the earlier successful initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 14b8e4b5-aae8-4335-ae15-adc5c60d0fe8
📒 Files selected for processing (7)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/core/webview/checkpointRestoreHandler.tssrc/core/webview/rulesMessageHandler.tssrc/core/webview/skillsMessageHandler.tssrc/core/webview/webviewMessageHandler.tssrc/eslint.config.mjs
github-actions
Bot
added
the
awaiting-review
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.
Picked up #949 from the #912 review-thread invitation and triaged the live
core/webviewset: 75 calls are awaited and 14 are explicitly voided. Two of the voids are flagged for your review as genuinely ambiguous: the constructorupdateGlobalState("codebaseIndexModels", ...)inClineProvider.ts(constructors cannot await) andworkspaceTracker?.initializeFilePaths()in thewebviewDidLaunchcase, where the existing "Don't await" comment is preserved because a workspace scan would sit on the launch path. Both keep baseline timing and rejection behavior exactly.Added
core/webview/**/*.tsto the existing ratchet. I ran the issue repro with zero warnings across all three ratcheted directories, the full suite with 6,785 passing, plus lint and typecheck.Fixes #949
Summary by CodeRabbit