Skip to content

[Refactor] Remove obsolete cloud account lifecycle listeners - #1353

Merged
edelauna merged 2 commits into
mainfrom
fix/extension-lifecycle-promises-0w723cyfrtfqr
Sep 1, 2026
Merged

[Refactor] Remove obsolete cloud account lifecycle listeners#1353
edelauna merged 2 commits into
mainfrom
fix/extension-lifecycle-promises-0w723cyfrtfqr

Conversation

@zoomote

@zoomote zoomote Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

​Created by Roomote. Follow up by mentioning @roomote, in the web UI, or in Discord.

What changed

  • Expanded the type-aware @typescript-eslint/no-floating-promises ratchet to extension.ts.
  • Removed obsolete auth-state-changed and user-info listener registration, state refresh, and cleanup left behind after interactive cloud accounts were removed.
  • Retained settings-updated for live organization-policy refreshes as contained fire-and-forget work with failure logging.
  • Kept model-cache initialization non-blocking with explicit failure ownership, removed the now-unused no-explicit-any suppression, and updated focused tests.

Why this change was made

The interactive cloud-account UI was already removed, and initial compatibility/static-token state is read after CloudService initialization. The auth and user-info listeners no longer provided required behavior, while the organization-settings listener remains live for policy changes.

Impact

Extension activation has fewer obsolete account lifecycle hooks. Organization policy changes still refresh visible state, model-cache startup still does not block activation, and future floating promises in extension.ts fail lint.

@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@edelauna
edelauna marked this pull request as ready for review August 23, 2026 12:43
@github-actions github-actions Bot added the awaiting-review PR changes are ready and waiting for maintainer re-review label Aug 23, 2026
@zoomote

zoomote Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Removed the obsolete auth-state and user-info extension listeners and their cleanup/test scaffolding. The live organization-settings listener remains as contained fire-and-forget work with failure logging; model-cache startup remains non-blocking. The stale no-explicit-any suppression was also removed.

Full lint, type-check, and test suites pass. Pushed in commit c86ffac0c.

@zoomote zoomote Bot changed the title [Fix] Cloud state updates can complete before webview refresh [Refactor] Remove obsolete cloud account lifecycle listeners Aug 23, 2026
@github-actions github-actions Bot added awaiting-review PR changes are ready and waiting for maintainer re-review and removed awaiting-review PR changes are ready and waiting for maintainer re-review labels Aug 23, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Review process

This PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging.

  1. Required CI checks pass.
  2. The workflow starts CodeRabbit automatically.
  3. For eligible human-authored PRs, CodeRabbit reviews and approves the latest commit.
  4. A human maintainer reviews and approves after CodeRabbit.

Current step: The required review sequence passed. Remaining merge requirements apply.

@github-actions github-actions Bot added coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit awaiting-review PR changes are ready and waiting for maintainer re-review awaiting-maintainer CodeRabbit approved; waiting for a human maintainer and removed awaiting-review PR changes are ready and waiting for maintainer re-review coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Aug 29, 2026
@edelauna
edelauna force-pushed the fix/extension-lifecycle-promises-0w723cyfrtfqr branch from c86ffac to 7b04018 Compare September 1, 2026 00:35
@github-actions github-actions Bot removed the awaiting-maintainer CodeRabbit approved; waiting for a human maintainer label Sep 1, 2026
@edelauna

edelauna commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: da96ec46-8f90-47cb-8be7-16597b6b924c

📥 Commits

Reviewing files that changed from the base of the PR and between dfed27d and 7b04018.

📒 Files selected for processing (4)
  • src/__tests__/extension.spec.ts
  • src/eslint-suppressions.json
  • src/eslint.config.mjs
  • src/extension.ts
💤 Files with no reviewable changes (1)
  • src/eslint-suppressions.json

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: platform-unit-test (windows-latest)
  • GitHub Check: platform-unit-test (ubuntu-latest)
  • GitHub Check: e2e-mock
🧰 Additional context used
📓 Path-based instructions (7)
Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases. Check cleanup and deterministic async behavior and prefer shared typed test helpe...

⚙️ CodeRabbit configuration file

Files:

  • src/__tests__/extension.spec.ts
Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths. Verify promises and errors are handled, existing helpers are reused, and new code introduces no `any`, unjustified dou...

⚙️ CodeRabbit configuration file

Files:

  • src/eslint.config.mjs
  • src/__tests__/extension.spec.ts
  • src/extension.ts
Verify extension/webview contracts, cancellation and error propagation, VS Code lifecycle correctness, and behavior under retries and partial failure. Check listeners, resources, and providers are disposed without stale state or duplicate w...

⚙️ CodeRabbit configuration file

Files:

  • src/eslint.config.mjs
  • src/__tests__/extension.spec.ts
  • src/extension.ts
Act as an adversarial second-opinion reviewer. Verify PR claims against implementation, contracts, and tests. Trace changed inputs through normal, boundary, error, cancellation, retry, and default paths and their consumers. Seek plausible c...

⚙️ CodeRabbit configuration file

Files:

  • src/eslint.config.mjs
  • src/__tests__/extension.spec.ts
  • src/extension.ts
Add focused tests for UI binding and save behavior, persistence or normalization, and the value returned by `getStateToPostToWebview()`, including true and false/unset cases when defaults could hide omissions.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • src/__tests__/extension.spec.ts
Fix lint violations in new TypeScript code instead of suppressing them.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • src/__tests__/extension.spec.ts
  • src/extension.ts
After editing a file, run ESLint with pruning and zero warnings for that relative file, and confirm its suppression count did not increase.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • src/__tests__/extension.spec.ts
  • src/extension.ts
🔇 Additional comments (1)
src/eslint.config.mjs (1)

61-61: LGTM!


📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Webview content now refreshes reliably when settings change.
    • Improved error handling and logging when settings updates or model refreshes fail.
    • Removed outdated authentication-state handling to prevent unnecessary updates and improve extension stability.
  • Quality Improvements

    • Enhanced validation helps catch unhandled background operations earlier.

Walkthrough

The extension now refreshes webview state after cloud settings updates and logs asynchronous failures. Model cache refresh initialization also logs background failures without blocking activation. Tests cover both failure paths, and ESLint now checks floating promises in extension.ts.

Changes

Extension event and async handling

Layer / File(s) Summary
Settings update lifecycle
src/extension.ts, src/__tests__/extension.spec.ts
Cloud settings updates refresh visible webview state. Refresh failures are logged. Removed authentication and user-info handlers are no longer registered or disposed. Tests cover the updated callback and failure logging.
Background model cache initialization
src/extension.ts, src/__tests__/extension.spec.ts
Model cache refresh initialization runs in the background. Activation continues when initialization fails, and the failure is logged.
Extension lint rule coverage
src/eslint.config.mjs, src/eslint-suppressions.json
no-floating-promises now covers extension.ts. Its obsolete no-explicit-any suppression is removed.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 7b040

This refactor removes obsolete account lifecycle hooks while retaining organization-policy refreshes and non-blocking model-cache startup; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: edelauna

Sequence Diagram(s)

sequenceDiagram
  participant CloudService
  participant Extension
  participant Webview
  participant OutputChannel
  CloudService->>Extension: Emit settings-updated
  Extension->>Webview: Refresh visible state
  Webview-->>Extension: Return refresh result
  Extension->>OutputChannel: Log refresh failure when rejected
Loading
🚥 Pre-merge checks | ✅ 5 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the implementation, rationale, and impact, but it omits the required linked GitHub issue, test procedure, pre-submission checklist, documentation updates, and reviewer contact… Add the required template sections. Provide an approved issue number, detailed test steps and environment, completed checklist items, documentation impact, and Discord contact information.
Regression Evidence ⚠️ Warning The PR removes the auth-state-changed and user-info registrations and their deactivation cleanup in src/extension.ts, but src/__tests__/extension.spec.ts does not assert either change. The clo… Add focused extension tests that capture the third argument to CloudService.createInstance and assert that it contains only settings-updated, with no auth-state-changed or user-info. Return a tracked off mock from the cloud-servic…
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: removal of obsolete cloud account lifecycle listeners.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files.
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.
Trust And Persistence Invariants ✅ Passed No changed path meets the failure conditions. The PR removes two CloudService webview listeners and retains the settings listener with an explicit rejection handler. CloudService still owns authentica…
Full details: Description check

Explanation

The description explains the implementation, rationale, and impact, but it omits the required linked GitHub issue, test procedure, pre-submission checklist, documentation updates, and reviewer contact sections.

Full details: Regression Evidence

Explanation

The PR removes the auth-state-changed and user-info registrations and their deactivation cleanup in src/extension.ts, but src/__tests__/extension.spec.ts does not assert either change. The cloud test captures only handlers["settings-updated"]; it does not inspect the handler map, so obsolete listeners could be reintroduced without failing the test. The suite also has no deactivation test for the remaining settings-updated cleanup. The new tests cover Error rejections for settings refresh and model-cache initialization, but not the explicit non-Error String(error) branches. The configuration changes and UI snapshot rule are not relevant.

Resolution

Add focused extension tests that capture the third argument to CloudService.createInstance and assert that it contains only settings-updated, with no auth-state-changed or user-info. Return a tracked off mock from the cloud-service fixture, call deactivate(), and assert cleanup for only settings-updated. Add rejection cases using a non-Error value for the new logging paths and assert the resulting String(error) text.

Full details: Trust And Persistence Invariants

Explanation

No changed path meets the failure conditions. The PR removes two CloudService webview listeners and retains the settings listener with an explicit rejection handler. CloudService still owns authentication, settings-cache persistence, and disposal; its internal auth listeners clear cached settings on logout, and extension activation registers the service for disposal. The changed model-cache call is fire-and-forget with failure logging, while model-cache writes remain atomic and unchanged. The state refresh only reads state and posts to the webview; it does not persist data. No new secret/PII flow, unvalidated execution, approval bypass, missing persistence await, or lifecycle leak is introduced.

  • Fix all pre-merge checks with AI
✨ 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 fix/extension-lifecycle-promises-0w723cyfrtfqr

Warning

Some tools did not complete. Review the errors below.

🔧 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/__tests__/extension.spec.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 matched ESLint configuration already failed (missing-dependency).

src/extension.ts

ESLint skipped: the matched ESLint configuration already failed (missing-dependency).


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.

@edelauna
edelauna enabled auto-merge September 1, 2026 00:42
@edelauna
edelauna added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit 9c9e68c Sep 1, 2026
18 checks passed
@edelauna
edelauna deleted the fix/extension-lifecycle-promises-0w723cyfrtfqr branch September 1, 2026 01:00
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