Conversation
|
Codex seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis pull request introduces a "saved commands" feature for the AI panel. The implementation adds UI components to save, display, edit, and manage code blocks from AI responses as reusable commands. Users can save commands, insert them into prompts, run them in focused terminals, or delete them. The feature includes validation logic to determine which code blocks are saveable (based on language and content patterns), persistent storage via object metadata, and integration with the AI model's state management. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
|
@codex review and find possible issue |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return nextCommand.id; | ||
| } | ||
|
|
||
| updateSavedCommand(id: string, text: string) { |
There was a problem hiding this comment.
WARNING: Inconsistent text normalization - addSavedCommand normalizes text by removing trailing newlines (line 319), but updateSavedCommand does not. This can cause duplicate commands if a user updates an existing command with trailing whitespace/newlines.
Consider normalizing the text in updateSavedCommand as well:
updateSavedCommand(id: string, text: string) {
const normalizedText = text.replace(/\n$/, "");
// ...
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files)
Note: The inline comment was posted directly on the code. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/element/streamdown.tsx (1)
356-356:⚠️ Potential issue | 🟡 MinorMissing
onClickSaveCommandinuseMemodependency array.The
componentsmemo usesonClickSaveCommandin theprecomponent (line 276), but it's not included in the dependency array. This can cause stale closures where the save button uses an outdated callback reference.🐛 Proposed fix
}), - [onClickExecute, codeBlockMaxWidthAtom] + [onClickExecute, onClickSaveCommand, codeBlockMaxWidthAtom] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/element/streamdown.tsx` at line 356, The memoized "components" (created with useMemo) references onClickSaveCommand inside the pre component but omits it from the dependency array ([onClickExecute, codeBlockMaxWidthAtom]), which can lead to stale closures; update the dependency array for the useMemo that defines components to include onClickSaveCommand so the pre component always captures the latest callback (i.e., add onClickSaveCommand alongside onClickExecute and codeBlockMaxWidthAtom).
🧹 Nitpick comments (2)
frontend/app/aipanel/savedcommandspanel.tsx (1)
53-55: Consider debouncing command text updates.The
onChangehandler callsmodel.updateSavedCommandon every keystroke. The implementation directly callspersistSavedCommandswithout any debouncing, which means every character typed triggers a persistence operation. This could cause unnecessary storage writes or backend calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/aipanel/savedcommandspanel.tsx` around lines 53 - 55, The textarea's onChange directly calls model.updateSavedCommand each keystroke causing immediate persistence; debounce those updates instead. Implement a debounced wrapper (e.g., using lodash.debounce or a custom debounce) around model.updateSavedCommand inside the component (or create a new method like debouncedUpdateSavedCommand) and call that from the textarea onChange; ensure you cancel/flush the debounce on unmount. Alternatively modify model.updateSavedCommand to batch/debounce calls before calling persistSavedCommands so persistSavedCommands is not invoked on every keystroke.frontend/app/aipanel/waveai-model.tsx (1)
357-388: Inconsistent use of trimmed vs. original text.The method validates using
commandText(trimmed), but sends usingtext(original) at line 385. This means leading/trailing whitespace is stripped for validation but preserved when sending to the terminal. If intentional (to preserve original formatting), consider documenting this behavior. If not, usecommandTextconsistently.Additionally, the method is marked
asyncbut contains noawaitexpressions—thesendDataToControllercall is synchronous per the codebase.♻️ Proposed refactor for consistency
- async runSavedCommand(text: string) { + runSavedCommand(text: string) { const commandText = text.trim(); if (!commandText) { return; } if (this.inBuilder) { this.setError("Running saved commands in the terminal is not available from the builder."); return; } const focusedBlockId = getFocusedBlockId(); if (!focusedBlockId) { this.setError("Focus a terminal block, then run the saved command again."); return; } const blockAtom = WOS.getWaveObjectAtom<Block>(WOS.makeORef("block", focusedBlockId)); const blockData = globalStore.get(blockAtom); if (blockData?.meta?.view !== "term") { this.setError("Focus a terminal block, then run the saved command again."); return; } const blockComponentModel = getBlockComponentModel(focusedBlockId); const termViewModel = blockComponentModel?.viewModel as { sendDataToController?: (data: string) => void }; if (typeof termViewModel?.sendDataToController !== "function") { this.setError("The focused terminal is not ready to receive commands."); return; } - const commandWithNewline = text.endsWith("\n") ? text : `${text}\n`; + const commandWithNewline = commandText.endsWith("\n") ? commandText : `${commandText}\n`; termViewModel.sendDataToController(commandWithNewline); this.requestNodeFocus(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/aipanel/waveai-model.tsx` around lines 357 - 388, The method runSavedCommand uses a trimmed commandText for validation but sends the original text to termViewModel.sendDataToController, causing inconsistent whitespace handling; change the send to use commandText (append newline to commandText as needed) so validation and sent data match, and since there are no awaits remove the async modifier from runSavedCommand to avoid misleading async semantics; update references to commandWithNewline to be derived from commandText and ensure requestNodeFocus() remains called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/aipanel/waveai-model.tsx`:
- Around line 318-342: The addSavedCommand function currently allows empty or
whitespace-only text because the find predicate prevents matching but doesn’t
prevent creating a new empty command; add an early validation in addSavedCommand
to trim the input and if trimmed text length is 0 return an empty string (or
null per project convention) without persisting; use the normalizedText (or
trimmed variable) to check and bail out before accessing globalStore or calling
persistSavedCommands, referencing addSavedCommand, savedCommandsAtom, and
persistSavedCommands to locate the change.
In `@pkg/waveobj/objrtinfo.go`:
- Around line 6-11: setFieldValue currently ignores slice targets so
WaveAISavedCommands (type []WaveAISavedCommand) is never persisted; update
setFieldValue in pkg/wstore/wstore_rtinfo.go to handle reflect.Slice: detect
when dest.Kind() == reflect.Slice, and if src is a string or []byte JSON
payload, unmarshal that JSON into a new slice of the destination element type
(e.g., []WaveAISavedCommand) and use reflect.Value.Set to assign it; ensure the
code path integrates with existing cases and will be used by SetRTInfoCommand
when persisting WaveAISavedCommands.
---
Outside diff comments:
In `@frontend/app/element/streamdown.tsx`:
- Line 356: The memoized "components" (created with useMemo) references
onClickSaveCommand inside the pre component but omits it from the dependency
array ([onClickExecute, codeBlockMaxWidthAtom]), which can lead to stale
closures; update the dependency array for the useMemo that defines components to
include onClickSaveCommand so the pre component always captures the latest
callback (i.e., add onClickSaveCommand alongside onClickExecute and
codeBlockMaxWidthAtom).
---
Nitpick comments:
In `@frontend/app/aipanel/savedcommandspanel.tsx`:
- Around line 53-55: The textarea's onChange directly calls
model.updateSavedCommand each keystroke causing immediate persistence; debounce
those updates instead. Implement a debounced wrapper (e.g., using
lodash.debounce or a custom debounce) around model.updateSavedCommand inside the
component (or create a new method like debouncedUpdateSavedCommand) and call
that from the textarea onChange; ensure you cancel/flush the debounce on
unmount. Alternatively modify model.updateSavedCommand to batch/debounce calls
before calling persistSavedCommands so persistSavedCommands is not invoked on
every keystroke.
In `@frontend/app/aipanel/waveai-model.tsx`:
- Around line 357-388: The method runSavedCommand uses a trimmed commandText for
validation but sends the original text to termViewModel.sendDataToController,
causing inconsistent whitespace handling; change the send to use commandText
(append newline to commandText as needed) so validation and sent data match, and
since there are no awaits remove the async modifier from runSavedCommand to
avoid misleading async semantics; update references to commandWithNewline to be
derived from commandText and ensure requestNodeFocus() remains called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae5bd66e-4985-455e-8793-200fa48b89b2
📒 Files selected for processing (9)
frontend/app/aipanel/aimessage.tsxfrontend/app/aipanel/aipanel.tsxfrontend/app/aipanel/aitypes.tsfrontend/app/aipanel/savedcommandspanel.tsxfrontend/app/aipanel/waveai-model.tsxfrontend/app/element/streamdown.test.tsfrontend/app/element/streamdown.tsxfrontend/types/gotypes.d.tspkg/waveobj/objrtinfo.go
| addSavedCommand(text: string): string { | ||
| const normalizedText = text.replace(/\n$/, ""); | ||
| const now = Date.now(); | ||
| const currentCommands = globalStore.get(this.savedCommandsAtom); | ||
| const existing = currentCommands.find( | ||
| (command) => command.text.trim() === normalizedText.trim() && normalizedText.trim().length > 0 | ||
| ); | ||
| if (existing) { | ||
| this.persistSavedCommands( | ||
| currentCommands.map((command) => | ||
| command.id === existing.id ? { ...command, updatedts: now } : command | ||
| ) | ||
| ); | ||
| return existing.id; | ||
| } | ||
|
|
||
| const nextCommand: SavedCommand = { | ||
| id: crypto.randomUUID(), | ||
| text: normalizedText, | ||
| createdts: now, | ||
| updatedts: now, | ||
| }; | ||
| this.persistSavedCommands([nextCommand, ...currentCommands]); | ||
| return nextCommand.id; | ||
| } |
There was a problem hiding this comment.
Add validation for empty or whitespace-only input.
If text is empty or contains only whitespace, existing will never match (due to the normalizedText.trim().length > 0 check in the find predicate), and the code will proceed to create a new command with empty text. This could result in multiple empty commands being saved.
🛡️ Proposed fix to add early validation
addSavedCommand(text: string): string {
const normalizedText = text.replace(/\n$/, "");
+ if (!normalizedText.trim()) {
+ return "";
+ }
const now = Date.now();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addSavedCommand(text: string): string { | |
| const normalizedText = text.replace(/\n$/, ""); | |
| const now = Date.now(); | |
| const currentCommands = globalStore.get(this.savedCommandsAtom); | |
| const existing = currentCommands.find( | |
| (command) => command.text.trim() === normalizedText.trim() && normalizedText.trim().length > 0 | |
| ); | |
| if (existing) { | |
| this.persistSavedCommands( | |
| currentCommands.map((command) => | |
| command.id === existing.id ? { ...command, updatedts: now } : command | |
| ) | |
| ); | |
| return existing.id; | |
| } | |
| const nextCommand: SavedCommand = { | |
| id: crypto.randomUUID(), | |
| text: normalizedText, | |
| createdts: now, | |
| updatedts: now, | |
| }; | |
| this.persistSavedCommands([nextCommand, ...currentCommands]); | |
| return nextCommand.id; | |
| } | |
| addSavedCommand(text: string): string { | |
| const normalizedText = text.replace(/\n$/, ""); | |
| if (!normalizedText.trim()) { | |
| return ""; | |
| } | |
| const now = Date.now(); | |
| const currentCommands = globalStore.get(this.savedCommandsAtom); | |
| const existing = currentCommands.find( | |
| (command) => command.text.trim() === normalizedText.trim() && normalizedText.trim().length > 0 | |
| ); | |
| if (existing) { | |
| this.persistSavedCommands( | |
| currentCommands.map((command) => | |
| command.id === existing.id ? { ...command, updatedts: now } : command | |
| ) | |
| ); | |
| return existing.id; | |
| } | |
| const nextCommand: SavedCommand = { | |
| id: crypto.randomUUID(), | |
| text: normalizedText, | |
| createdts: now, | |
| updatedts: now, | |
| }; | |
| this.persistSavedCommands([nextCommand, ...currentCommands]); | |
| return nextCommand.id; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/aipanel/waveai-model.tsx` around lines 318 - 342, The
addSavedCommand function currently allows empty or whitespace-only text because
the find predicate prevents matching but doesn’t prevent creating a new empty
command; add an early validation in addSavedCommand to trim the input and if
trimmed text length is 0 return an empty string (or null per project convention)
without persisting; use the normalizedText (or trimmed variable) to check and
bail out before accessing globalStore or calling persistSavedCommands,
referencing addSavedCommand, savedCommandsAtom, and persistSavedCommands to
locate the change.
| type WaveAISavedCommand struct { | ||
| Id string `json:"id"` | ||
| Text string `json:"text"` | ||
| CreatedTs int64 `json:"createdts,omitempty"` | ||
| UpdatedTs int64 `json:"updatedts,omitempty"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that setFieldValue lacks slice handling
rg -n 'reflect\.Slice' pkg/wstore/wstore_rtinfo.go
# Show the setFieldValue function to confirm missing slice case
ast-grep --pattern $'func setFieldValue($_, $_) {
$$$
}'Repository: wavetermdev/waveterm
Length of output: 3466
Add reflect.Slice handling to setFieldValue function.
The WaveAISavedCommands field (added to ObjRTInfo) is a slice type, but setFieldValue in pkg/wstore/wstore_rtinfo.go (lines 19-79) only handles string, bool, int, map, and interface kinds. When SetRTInfoCommand attempts to persist saved commands via this function, the slice assignment is silently skipped due to the missing case, resulting in data loss across sessions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/waveobj/objrtinfo.go` around lines 6 - 11, setFieldValue currently
ignores slice targets so WaveAISavedCommands (type []WaveAISavedCommand) is
never persisted; update setFieldValue in pkg/wstore/wstore_rtinfo.go to handle
reflect.Slice: detect when dest.Kind() == reflect.Slice, and if src is a string
or []byte JSON payload, unmarshal that JSON into a new slice of the destination
element type (e.g., []WaveAISavedCommand) and use reflect.Value.Set to assign
it; ensure the code path integrates with existing cases and will be used by
SetRTInfoCommand when persisting WaveAISavedCommands.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd31239a37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WaveAIChatId string `json:"waveai:chatid,omitempty"` | ||
| WaveAIMode string `json:"waveai:mode,omitempty"` | ||
| WaveAIMaxOutputTokens int `json:"waveai:maxoutputtokens,omitempty"` | ||
| WaveAISavedCommands []WaveAISavedCommand `json:"waveai:savedcommands,omitempty"` |
There was a problem hiding this comment.
Adding waveai:savedcommands as []WaveAISavedCommand makes persistence silently fail because SetRTInfo currently routes updates through setFieldValue, which does not support reflect.Slice (it only handles nil/string/bool/int/map/interface in pkg/wstore/wstore_rtinfo.go). As a result, frontend calls to SetRTInfoCommand with non-empty saved command arrays are ignored, so commands appear in-memory but are lost after reload/restart.
Useful? React with 👍 / 👎.
2 participants
No description provided.