Add close function to popup module
#286
Conversation
WalkthroughA new asynchronous Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Popup as popup/mod.ts
participant Denops as Denops Host
Test->>Popup: open(denops, options)
Popup->>Denops: Create popup window
Denops-->>Popup: Return winid
Popup-->>Test: Return winid
Test->>Popup: close(denops, winid)
Popup->>Denops: Determine host (Vim/Neovim)
Popup->>Denops: Call appropriate close function
Denops-->>Popup: Popup closed
Popup->>Denops: Redraw UI
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
popup/mod.ts (1)
178-189: Consider adding anoRedrawoption for consistency.The standalone
closefunction always callsdenops.redraw(), while the instance method'sclose()respects thenoRedrawoption from the originalopen()call. This inconsistency could lead to unexpected behavior when users want to batch multiple operations without triggering redraws.Consider adding an optional parameter or making the redraw behavior consistent with other functions in the module.
/** - * Close popup window + * Close popup window by window ID in Vim/Neovim compatible way. + * + * ```typescript + * import type { Entrypoint } from "jsr:@denops/std"; + * import * as popup from "jsr:@denops/std/popup"; + * + * export const main: Entrypoint = async (denops) => { + * // Open a popup window + * const popupWindow = await popup.open(denops, { + * relative: "editor", + * width: 20, + * height: 20, + * row: 1, + * col: 1, + * }); + * + * // Close using the standalone close function + * await popup.close(denops, popupWindow.winid); + * } + * ``` + * + * Note that this function does NOT work in `batch.collect()`. */ export async function close( denops: Denops, winid: number, + options: { noRedraw?: boolean } = {}, ): Promise<void> { const close = denops.meta.host === "vim" ? closePopupVim : closePopupNvim; await close(denops, winid); - await denops.redraw(); + if (!options.noRedraw) { + await denops.redraw(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
popup/mod.ts(1 hunks)popup/mod_test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (ubuntu-latest, 1.45.0, v9.1.0448, v0.10.0)
- GitHub Check: test (ubuntu-latest, 1.x, v9.1.0448, v0.10.0)
- GitHub Check: test (macos-latest, 1.x, v9.1.0448, v0.10.0)
- GitHub Check: test (macos-latest, 1.45.0, v9.1.0448, v0.10.0)
- GitHub Check: test (windows-latest, 1.x, v9.1.0448, v0.10.0)
- GitHub Check: test (windows-latest, 1.45.0, v9.1.0448, v0.10.0)
🔇 Additional comments (1)
popup/mod_test.ts (1)
109-130: Well-structured test for the new close function.The test properly verifies the standalone close functionality by:
- Opening a popup window and confirming its type
- Using the new
popup.close()function with the window ID- Verifying the popup is properly closed afterward
The test follows the existing patterns in the file and provides good coverage for the new functionality.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
==========================================
+ Coverage 85.80% 85.84% +0.03%
==========================================
Files 65 65
Lines 3459 3468 +9
Branches 301 302 +1
==========================================
+ Hits 2968 2977 +9
Misses 489 489
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Tests