Refactor send prompt #33
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
| await fn.feedkeys(denops, '"qp'); | ||
| await denops.call("chansend", jobId, "\n"); | ||
| const promptLines = prompt.split("\n"); | ||
| const joined = promptLines.join("\x1b\x0d"); // use Esc + Ctrl-M instead of \n to avoid submit cf. https://github.com/Aider-AI/aider/issues/901 |
There was a problem hiding this comment.
issueの下の方のコメントでESC Ctrl-Mでaiderに送信せず改行できるって言ってるし私の手元環境でもうまく動いてるけどもしかしたら環境によって普通に送信されてしまったりするかもしれない
There was a problem hiding this comment.
\nだとVisualとかで複数行入力したときに一行ずつaiderに送られてしまうんじゃ
There was a problem hiding this comment.
@tsukimizake
await fn.feedkeys(denops, "G"); がないと、毎回ターミナルウインドウでGを押して下まで移動しないといけなくなるので、これは必要かもです。
There was a problem hiding this comment.
ただ、ここでfeedkeysをするのはなんか違うきがする。プロンプト送信に徹したい。( ；´。 ｀；)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
denops/aider/bufferOperation.ts (1)
Line range hint
88-101: LGTM! Consider adding JSDoc for the new parameter.The changes to the
sendPromptfunction improve its flexibility by allowing control over buffer opening. This is a good enhancement that maintains backward compatibility.Consider adding JSDoc for the new
optsparameter to improve code documentation:/** * Sends a prompt to the Aider buffer. * @param {Denops} denops - The Denops instance. * @param {string} input - The prompt to send. * @param {Object} [opts={ openBuf: true }] - Options for sending the prompt. * @param {boolean} [opts.openBuf=true] - Whether to open the buffer before sending the prompt. */ export async function sendPrompt(denops: Denops, input: string, opts = { openBuf: true }): Promise<void> { // ... (existing implementation) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- denops/aider/actualAiderCommand.ts (1 hunks)
- denops/aider/bufferOperation.ts (2 hunks)
- denops/aider/main.ts (5 hunks)
🧰 Additional context used
📓 Learnings (1)
denops/aider/actualAiderCommand.ts (2)
Learnt from: tsukimizake PR: nekowasabi/aider.vim#18 File: denops/aider/aiderCommand.ts:45-53 Timestamp: 2024-09-30T14:46:06.173Z Learning: In the `sendPrompt` function in `denops/aider/aiderCommand.ts`, replacing `fn.feedkeys` with `fn.append` did not work.Learnt from: tsukimizake PR: nekowasabi/aider.vim#23 File: denops/aider/actualAiderCommand.ts:76-79 Timestamp: 2024-10-05T15:24:04.950Z Learning: `denops/aider/actualAiderCommand.ts` の `sendPrompt` 関数で直接 `chansend` を使用すると、入力が分割されてしまう可能性がある。
🔇 Additional comments (4)
denops/aider/actualAiderCommand.ts (1)
43-45: Improved prompt handling, but consider potential input splitting.The new implementation of
sendPromptbrings several improvements:
- It now handles multi-line prompts effectively by splitting and rejoining them.
- The use of
\x1b\x0d(Esc + Ctrl-M) instead of\nis a clever way to avoid immediate submission, addressing the issue mentioned in the linked GitHub issue.- Direct use of
chansendaligns with the PR objective and allows the function to work regardless of the current buffer.However, there's a potential concern:
A previous learning (PR #23) mentioned that direct use of
chansendmight lead to input being split. Let's verify if this is still a concern:If the verification doesn't show any additional error handling, consider adding a safeguard against potential input splitting.
Would you like assistance in implementing additional error handling or input validation for the
chansendcall?denops/aider/main.ts (3)
85-90: LGTM: Improved flexibility inaddFileToAiderfunctionThe addition of the optional
optsparameter with a default value of{ openBuf: true }enhances the flexibility of theaddFileToAiderfunction. This change aligns well with the PR objectives of refactoring thesendPromptfunctionality and allows for more granular control over buffer operations.
150-150: LGTM: Consistent behavior for "silent" commandsThe
silentAddCurrentFileandsilentAddCurrentFileReadOnlycommands have been updated to use{ openBuf: false }when callingaddFileToAider. This change improves consistency between the command names and their behavior, as "silent" commands now don't open a buffer when adding files.Also applies to: 169-171
Line range hint
1-270: Overall assessment: Changes improve flexibility and consistencyThe modifications to
denops/aider/main.tssuccessfully refactor thesendPromptfunctionality as intended. The changes introduce more flexible buffer handling options and improve consistency in command behavior. These enhancements align well with the PR objectives and should contribute to a more robust and user-friendly Aider plugin.
denops/aider/actualAiderCommand.ts
Outdated
| @@ -41,8 +41,8 @@ async function run(denops: Denops): Promise<undefined> { | |||
| */ | |||
| async function sendPrompt(denops: Denops, jobId: number, prompt: string): Promise<undefined> { | |||
| const promptLines = prompt.split("\n"); | |||
| promptLines.push("\n"); | |||
| await denops.call("chansend", jobId, promptLines); | |||
| const joined = promptLines.join("\x1b\x0d"); // use Esc + Ctrl-M instead of \n to avoid sending cf. https://github.com/Aider-AI/aider/issues/901 | |||
| @@ -148,8 +148,6 @@ export async function main(denops: Denops): Promise<void> { | |||
|
|
|||
| await command("silentAddCurrentFile", "0", async () => { | |||
| await addFileToAider(denops, openBufferType, "add", { openBuf: false }); | |||
| await denops.cmd("fclose!"); | |||
| @@ -85,7 +85,7 @@ export async function prepareAiderBuffer(denops: Denops, openBufferType: BufferL | |||
| } | |||
| } | |||
|
|
|||
| export async function sendPrompt(denops: Denops, input: string, openBuf = true): Promise<void> { | |||
| export async function sendPrompt(denops: Denops, input: string, opts = { openBuf: true }): Promise<void> { | |||
There was a problem hiding this comment.
[Q] optsをjson的に渡せるようにしたのは、将来的な拡張性のためでしょうか
There was a problem hiding this comment.
まあyes 1個だけでもなんかboolの引数だけがあるよりわかりやすいし
|
@tsukimizake |
|
質問は本筋とは関係ないのでLGTM |
|
ちらつきが消えたのはいいのだけど本当にsilentになってしまったのでaider.sendPrompt内で下に/add ... みたいなmessage出すくらいはしてもいいかもしれねえ |
|
@tsukimizake |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation