Skip to content

chore: enforce no-floating-promises in activate/#251

Open
0xMink wants to merge 1 commit into
mainfrom
chore/no-floating-promises-activate
Open

chore: enforce no-floating-promises in activate/#251
0xMink wants to merge 1 commit into
mainfrom
chore/no-floating-promises-activate

Conversation

@0xMink
Copy link
Copy Markdown
Contributor

@0xMink 0xMink commented May 22, 2026

Summary

First slice of a ratchet that re-enables @typescript-eslint/no-floating-promises directory by directory.

The repo's lint is zero-tolerance (eslint --max-warnings=0 + the only-warn plugin), so the rule can't be enabled globally without first clearing all ~194 floating promises across src/. Instead, this enables the rule scoped to activate/** via a files-block in eslint.config.mjs (with type-aware parsing for that scope). pnpm lint stays green; subsequent PRs widen the scope until it's global.

Fixes

activate/registerCommands.ts had 7 un-awaited postMessageToWebview(...) calls (fire-and-forget UI messages):

  • 5 marked void — in synchronous command handlers (settingsButtonClicked, historyButtonClicked, marketplaceButtonClicked, acceptInput); these are intentional fire-and-forget and the handler is not async.
  • 2 awaited — in handlers that are already async (focusInput, toggleAutoApprove). focusInput's call sits inside a try/catch, so awaiting routes a rejected post into the existing error handler.

No sync→async handler conversions; behavior is unchanged.

Test plan

  • eslint activate/ clean.
  • eslint . --ext=ts --max-warnings=0 (the repo lint command) passes.
  • tsc --noEmit clean.
  • activate/ test suite passes (12 tests).

Summary by CodeRabbit

  • Bug Fixes

    • Improved consistency and reliability of UI action handlers for settings, history, marketplace access, and input interactions.
  • Chores

    • Enhanced code quality standards by adding stricter linting rules to ensure robust handling of asynchronous operations throughout the codebase.

Review Change Stack

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.
Copilot AI review requested due to automatic review settings May 22, 2026 08:05
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ab731655-3e5a-41ff-a180-7f3b0c4e45a4

📥 Commits

Reviewing files that changed from the base of the PR and between 45b239c and 4c5b80d.

📒 Files selected for processing (2)
  • src/activate/registerCommands.ts
  • src/eslint.config.mjs

📝 Walkthrough

Walkthrough

Command handlers in the activate module are updated to explicitly handle promise returns from webview message posting: most use void to intentionally ignore promises, while toggleAutoApprove switches to await to block until completion. ESLint configuration is added to enforce the no-floating-promises rule across the module.

Changes

Promise handling updates

Layer / File(s) Summary
Command handler promise handling
src/activate/registerCommands.ts
settingsButtonClicked explicitly voids its promise and adds a follow-up didBecomeVisible message; historyButtonClicked, marketplaceButtonClicked, focusInput, and acceptInput use void on webview message calls; toggleAutoApprove changes from fire-and-forget to await.
ESLint no-floating-promises enforcement
src/eslint.config.mjs
Configuration block added for activate/**/*.ts to enforce @typescript-eslint/no-floating-promises rule with TypeScript project-based parsing (project: true and tsconfigRootDir).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🐰 Promises now handled with care,
Void or await, each has its pair,
ESLint watches from above,
No floating hopes, just honest love! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description covers implementation details, design rationale, and testing verification; however, it is missing the required 'Related GitHub Issue' section and pre-submission checklist. Add the 'Related GitHub Issue' section with the issue number and complete the pre-submission checklist items to match the repository template.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'chore: enforce no-floating-promises in activate/' clearly and concisely summarizes the main change: enabling ESLint's no-floating-promises rule in the activate directory.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/no-floating-promises-activate

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.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

src/activate/registerCommands.ts

ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.

src/eslint.config.mjs

ESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR re-enables @typescript-eslint/no-floating-promises in a scoped, incremental way by enforcing it only under src/activate/**, and fixes the newly-detected floating promises in activate/registerCommands.ts without broadly changing async behavior elsewhere in the repo.

Changes:

  • Scoped @typescript-eslint/no-floating-promises to activate/**/*.ts with type-aware parsing enabled for that scope.
  • Annotated intentional fire-and-forget webview messages with void in sync handlers.
  • Awaited two webview messages in existing async handlers to satisfy the rule.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/eslint.config.mjs Adds a flat-config files override to enforce no-floating-promises only for activate/** using type-aware parser options.
src/activate/registerCommands.ts Resolves floating promises by marking intentional fire-and-forget calls as void and awaiting calls in async handlers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/activate/registerCommands.ts 0.00% 7 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants