minor, fix double sub in webview giveFocus

[sawka]

No description provided.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c6d92c3e-749e-4daf-a604-f8e6f9583a83

📥 Commits

Reviewing files that changed from the base of the PR and between 697b959 and 8d5ef8f.

📒 Files selected for processing (1)

• frontend/app/view/webview/webview.tsx

---

Walkthrough

The WebViewModel class now stores a single unsubscribe callback in a new ctrlShiftUnsubFn field for the getSimpleControlShiftAtom() subscription. giveFocus() creates the subscription only when ctrlShiftState is truthy and no subscription exists, and the subscription callback clears and nulls the stored unsubscribe when the atom becomes falsy. A new dispose() method calls and clears ctrlShiftUnsubFn to ensure the subscription is cleaned up when the model is disposed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No description was provided by the author, which is a completely off-topic (or rather, absent) contribution to the changeset.	Add a pull request description explaining the fix: what the double subscription problem was, why it needed fixing, and how the solution addresses it.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'minor, fix double sub in webview giveFocus' directly describes the main change: fixing a double subscription issue in the webview's giveFocus method.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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 sawka/webview-double-sub

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)

• frontend/app/view/webview/webview.tsx

The changes correctly fix the double subscription issue by:

1. Adding a ctrlShiftUnsubFn field to track the subscription
2. Checking !this.ctrlShiftUnsubFn before subscribing to prevent duplicate subscriptions
3. Properly cleaning up the subscription when ctrlShiftState becomes false

Note: CodeRabbit posted a nitpick about making ctrlShiftUnsubFn private for better encapsulation - this is a minor style preference, not a functional issue.

No issues identified.

---

_{Reviewed by minimax-m2.5-20260211 · 238,532 tokens}

[cloudflare-workers-and-pages]

Deploying waveterm with    Cloudflare Pages

Latest commit:	8d5ef8f
Status:	✅  Deploy successful!
Preview URL:	https://de24b8c6.waveterm.pages.dev
Branch Preview URL:	https://sawka-webview-double-sub.waveterm.pages.dev

View logs

[coderabbitai]

🧹 Nitpick comments (1)

frontend/app/view/webview/webview.tsx (1)

77-77: Encapsulate the unsubscribe handle with deterministic cleanup.

Line 77 exposes ctrlShiftUnsubFn as a mutable field, and it is only cleared at lines 517–518 when getSimpleControlShiftAtom() flips to false. If the webview unmounts before that state change, the stored callback persists with a closure over the old WebViewModel, creating a potential memory leak. Hide this handle behind a cleanup method and call it from the existing unmount path (around line 950).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/webview/webview.tsx` at line 77, The public mutable field
ctrlShiftUnsubFn on WebViewModel can retain a closure and leak if the component
unmounts; make this handle private, add a deterministic cleanup method (e.g.,
clearCtrlShiftUnsub or disposeCtrlShiftUnsub) that, if the handle is non-null,
calls it and sets it to null, and update the code paths that assign
ctrlShiftUnsubFn (use a setter that clears any previous handle before storing
the new one). Finally call this cleanup method from the existing
unmount/teardown path (the WebViewModel/component unmount sequence) and also
when getSimpleControlShiftAtom() flips to false so the callback is always
removed and nulled.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@frontend/app/view/webview/webview.tsx`:
- Line 77: The public mutable field ctrlShiftUnsubFn on WebViewModel can retain
a closure and leak if the component unmounts; make this handle private, add a
deterministic cleanup method (e.g., clearCtrlShiftUnsub or
disposeCtrlShiftUnsub) that, if the handle is non-null, calls it and sets it to
null, and update the code paths that assign ctrlShiftUnsubFn (use a setter that
clears any previous handle before storing the new one). Finally call this
cleanup method from the existing unmount/teardown path (the
WebViewModel/component unmount sequence) and also when
getSimpleControlShiftAtom() flips to false so the callback is always removed and
nulled.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 452d57ec-994e-43ec-a664-ceb46280e529

📥 Commits

Reviewing files that changed from the base of the PR and between 645424a and 697b959.

📒 Files selected for processing (1)

• frontend/app/view/webview/webview.tsx