add an onboarding upgrade modal #2433
Conversation
…irst startup. the primary tab is the tab showing in the top-most window when the user launches the app.
…'t seen it before...
WalkthroughThe PR introduces a primary startup concept across Electron main and frontend: window and tab initialization now carry isPrimaryStartupWindow and primaryTabStartup flags through creation, action queues, and init paths. Frontend global init accepts and stores primaryTabStartup. Onboarding is split into two modals (new install and upgrade), with semver-based upgrade gating, persistent onboarding:lastversion metadata, and telemetry fields onboarding:version and onboarding:lastversion added across TS and Go. New UpgradeOnboardingModal is implemented; existing onboarding modal renamed. WebView search callbacks guard on DOM readiness. Semver moved to dependencies. Minor doc frontmatter and AI tool description text updates. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
frontend/app/aipanel/telemetryrequired.tsx (1)
21-23: Consider cleanup to prevent stale callbacks.The timeout isn't canceled if the component unmounts during the 100ms delay, which could invoke
focusInput()on stale state. Additionally, the 100ms delay is a magic number without explanation.Apply this diff to add cleanup:
const TelemetryRequiredMessage = ({ className }: TelemetryRequiredMessageProps) => { const [isEnabling, setIsEnabling] = useState(false); + const timeoutRef = useRef<NodeJS.Timeout | null>(null); + + useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }; + }, []); const handleEnableTelemetry = async () => { setIsEnabling(true); try { await RpcApi.WaveAIEnableTelemetryCommand(TabRpcClient); - setTimeout(() => { + timeoutRef.current = setTimeout(() => { WaveAIModel.getInstance().focusInput(); }, 100);Optionally, extract the delay as a named constant:
const FOCUS_DELAY_MS = 100; // Allow telemetry state to propagatefrontend/app/onboarding/onboarding-upgrade.tsx (4)
23-35: Consider using state or CSS for height management.Direct DOM style manipulation (
modalRef.current.style.height = ...) is an anti-pattern in React. Consider managing height through state or CSS classes for better React integration and predictability.Example refactor using state:
+const [maxHeight, setMaxHeight] = useState<string | undefined>(undefined); + const updateModalHeight = () => { const windowHeight = window.innerHeight; setIsCompact(windowHeight < 800); - if (modalRef.current) { - const modalHeight = modalRef.current.offsetHeight; - const maxHeight = windowHeight * 0.9; - if (maxHeight < modalHeight) { - modalRef.current.style.height = `${maxHeight}px`; - } else { - modalRef.current.style.height = "auto"; - } - } + setMaxHeight(`${windowHeight * 0.9}px`); };Then apply the height via inline style or className on the modal element.
53-86: Consider adding error handling for RPC calls.The event handlers correctly persist metadata via RPC, but lack error handling. If
SetMetaCommandfails, users won't receive feedback and the navigation will still proceed.Consider wrapping RPC calls in try-catch:
const handleStarClick = async () => { + try { const clientId = globalStore.get(atoms.clientId); await RpcApi.SetMetaCommand(TabRpcClient, { oref: WOS.makeORef("client", clientId), meta: { "onboarding:githubstar": true }, }); window.open("https://github.com/wavetermdev/waveterm", "_blank"); setPageName("features"); + } catch (error) { + console.error("Failed to persist star preference:", error); + // Still allow navigation even if metadata fails + setPageName("features"); + } };
59-59: Set opener to null for window.open security.When opening external links with
window.open, you should nullify theopenerproperty to prevent the opened page from accessing the parent window.Apply this fix:
-window.open("https://github.com/wavetermdev/waveterm", "_blank"); +const newWindow = window.open("https://github.com/wavetermdev/waveterm", "_blank"); +if (newWindow) newWindow.opener = null;
161-166: Consider adding accessibility attributes.The modal lacks accessibility attributes that would help screen reader users understand its purpose and behavior.
Add ARIA attributes to FlexiModal:
-<FlexiModal className={`${widthClass} rounded-[10px] ${paddingClass} relative overflow-hidden`} ref={modalRef}> +<FlexiModal + className={`${widthClass} rounded-[10px] ${paddingClass} relative overflow-hidden`} + ref={modalRef} + aria-label="Wave upgrade onboarding" + role="dialog" +>Note: Verify that
FlexiModaldoesn't already apply these attributes internally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
docs/docs/waveai.mdx(1 hunks)emain/emain-window.ts(11 hunks)emain/emain-wsh.ts(1 hunks)emain/emain.ts(1 hunks)frontend/app/aipanel/telemetryrequired.tsx(2 hunks)frontend/app/modals/modalregistry.tsx(1 hunks)frontend/app/modals/modalsrenderer.tsx(2 hunks)frontend/app/onboarding/onboarding-features.tsx(6 hunks)frontend/app/onboarding/onboarding-upgrade.tsx(1 hunks)frontend/app/onboarding/onboarding.tsx(3 hunks)frontend/app/store/global.ts(3 hunks)frontend/app/store/modalmodel.ts(1 hunks)frontend/app/view/webview/webview.tsx(3 hunks)frontend/types/custom.d.ts(1 hunks)frontend/types/gotypes.d.ts(2 hunks)frontend/wave.ts(1 hunks)package.json(1 hunks)pkg/aiusechat/tools_readdir.go(1 hunks)pkg/aiusechat/tools_readfile.go(1 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(1 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
frontend/app/aipanel/telemetryrequired.tsx (1)
frontend/app/aipanel/waveai-model.tsx (1)
WaveAIModel(24-231)
frontend/app/onboarding/onboarding-features.tsx (1)
frontend/app/store/global.ts (2)
atoms(798-798)WOS(848-848)
emain/emain.ts (1)
emain/emain-window.ts (1)
createBrowserWindow(646-668)
emain/emain-wsh.ts (1)
emain/emain-window.ts (1)
createBrowserWindow(646-668)
frontend/app/onboarding/onboarding.tsx (2)
frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/modals/modal.tsx (1)
FlexiModal(112-112)
frontend/app/onboarding/onboarding-upgrade.tsx (7)
frontend/app/store/keymodel.ts (3)
disableGlobalKeybindings(653-653)enableGlobalKeybindings(654-654)globalRefocus(656-656)frontend/app/store/global.ts (2)
atoms(798-798)WOS(848-848)frontend/app/store/wshclientapi.ts (1)
RpcApi(537-537)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/onboarding/onboarding-features.tsx (1)
OnboardingFeatures(306-365)frontend/app/modals/modal.tsx (1)
FlexiModal(112-112)
frontend/app/modals/modalregistry.tsx (2)
frontend/app/onboarding/onboarding.tsx (1)
NewInstallOnboardingModal(291-291)frontend/app/onboarding/onboarding-upgrade.tsx (1)
UpgradeOnboardingModal(171-171)
frontend/app/modals/modalsrenderer.tsx (2)
frontend/app/store/global.ts (2)
atoms(798-798)globalPrimaryTabStartup(820-820)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)
emain/emain-window.ts (1)
emain/emain-tabview.ts (1)
WaveTabView(35-154)
🪛 Biome (2.1.2)
frontend/app/onboarding/onboarding-upgrade.tsx
[error] 116-116: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (17)
frontend/app/aipanel/telemetryrequired.tsx (1)
8-8: LGTM!The import is correctly added and follows the established module structure.
frontend/app/store/global.ts (1)
30-30: LGTM! Primary startup flag properly integrated.The
globalPrimaryTabStartupstate is correctly initialized, propagated throughGlobalInitOptions, and exported for module-level access. The use of the nullish coalescing operator (??) ensures a safe default value offalsewheninitOpts.primaryTabStartupis undefined.Also applies to: 43-43, 48-48, 820-820
pkg/waveobj/metaconsts.go (1)
136-136: LGTM! Metadata key correctly added.The new
MetaKey_OnboardingLastVersionconstant follows the existing naming convention and aligns with the onboarding version tracking feature introduced in this PR.frontend/types/gotypes.d.ts (2)
671-671: LGTM! Metadata type properly extended.The
"onboarding:lastversion"field correctly extendsMetaTypeto support persistent storage of the last onboarding version seen by the user.
939-939: LGTM! Telemetry props correctly extended.The
"onboarding:version"field properly extendsTEventPropsto capture onboarding version information in telemetry events.package.json (1)
150-150: LGTM! Dependency correctly promoted to runtime.Moving
semverfromdevDependenciestodependenciesis necessary because the onboarding flow now performs runtime version comparisons (e.g., checking ifonboarding:lastversionis less thanCurrentOnboardingVersion). Version^7.7.3is the latest release (2025-10-06) and includes important fixes for prerelease handling and the new "release" increment type.Based on learnings.
frontend/app/view/webview/webview.tsx (3)
744-757: LGTM! DOM readiness guard prevents race conditions.The early-exit check prevents
findInPagecalls before the WebView DOM is ready, avoiding potential errors during initialization. This is a good defensive programming practice.
758-768: LGTM! Consistent DOM readiness check.The guard in
onNextmatches the pattern established inonSearch, ensuring consistent protection against premature WebView API calls.
769-779: LGTM! Complete protection for search navigation.The guard in
onPrevcompletes the consistent pattern of DOM readiness checks across all search navigation callbacks (onSearch,onNext,onPrev).pkg/telemetry/telemetrydata/telemetrydata.go (1)
93-93: LGTM! Telemetry field correctly added.The
OnboardingVersionfield is properly structured with theomitemptyJSON tag, following the established pattern for telemetry properties. This enables tracking of onboarding version in telemetry events.frontend/types/custom.d.ts (1)
62-62: LGTM! Type definition correctly extended.The
primaryTabStartupfield properly extendsWaveInitOptsto support the primary startup flag, with the optional modifier ensuring backward compatibility.frontend/wave.ts (1)
161-168: LGTM! Primary startup flag properly propagated.The
primaryTabStartupfield is correctly passed frominitOptstoinitGlobal, completing the initialization chain from Electron main process through to the global state.frontend/app/onboarding/onboarding-upgrade.tsx (5)
1-16: LGTM! Imports are well-organized.All dependencies are properly imported and match the usage patterns described in the learnings for
overlayscrollbars-reactandthrottle-debounce.
18-21: LGTM! State initialization is appropriate.The component state and refs are properly initialized, with
isCompactcorrectly calculated based on the initial window height.
37-44: LGTM! Resize handling with proper cleanup.The debounced resize listener is correctly implemented with proper cleanup on unmount. The 150ms debounce interval is appropriate for resize events.
Based on learnings
46-51: LGTM! Proper keybindings lifecycle management.Global keybindings are correctly disabled on mount and re-enabled on unmount, ensuring keyboard shortcuts don't interfere with the modal.
150-152: LGTM! Clean delegation to OnboardingFeatures.The features page correctly delegates to the
OnboardingFeaturescomponent with the appropriate completion callback.
| const windowIds = clientData.windowids ?? []; | ||
| const wins: WaveBrowserWindow[] = []; | ||
| for (const windowId of clientData.windowids.slice().reverse()) { | ||
| const isFirstRelaunch = !hasCompletedFirstRelaunch; | ||
| const primaryWindowId = windowIds.length > 0 ? windowIds[0] : null; | ||
| for (const windowId of windowIds.slice().reverse()) { | ||
| const windowData: WaveWindow = await WindowService.GetWindow(windowId); | ||
| if (windowData == null) { | ||
| console.log("relaunch -- window data not found, closing window", windowId); | ||
| await WindowService.CloseWindow(windowId, true); | ||
| continue; | ||
| } | ||
| console.log("relaunch -- creating window", windowId, windowData); | ||
| const win = await createBrowserWindow(windowData, fullConfig, { unamePlatform }); | ||
| const isPrimaryStartupWindow = isFirstRelaunch && windowId === primaryWindowId; | ||
| console.log("relaunch -- creating window", windowId, windowData, isPrimaryStartupWindow ? "(primary startup)" : ""); | ||
| const win = await createBrowserWindow(windowData, fullConfig, { | ||
| unamePlatform, | ||
| isPrimaryStartupWindow | ||
| }); | ||
| wins.push(win); |
There was a problem hiding this comment.
Guard against null clientData before reading windowids.
ClientService.GetClientData() can return null/undefined during startup or error scenarios, so clientData.windowids will throw and abort the relaunch. Please restore the null-guard when building the window list.
- const windowIds = clientData.windowids ?? [];
+ const windowIds = clientData?.windowids ?? [];📝 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.
| const windowIds = clientData.windowids ?? []; | |
| const wins: WaveBrowserWindow[] = []; | |
| for (const windowId of clientData.windowids.slice().reverse()) { | |
| const isFirstRelaunch = !hasCompletedFirstRelaunch; | |
| const primaryWindowId = windowIds.length > 0 ? windowIds[0] : null; | |
| for (const windowId of windowIds.slice().reverse()) { | |
| const windowData: WaveWindow = await WindowService.GetWindow(windowId); | |
| if (windowData == null) { | |
| console.log("relaunch -- window data not found, closing window", windowId); | |
| await WindowService.CloseWindow(windowId, true); | |
| continue; | |
| } | |
| console.log("relaunch -- creating window", windowId, windowData); | |
| const win = await createBrowserWindow(windowData, fullConfig, { unamePlatform }); | |
| const isPrimaryStartupWindow = isFirstRelaunch && windowId === primaryWindowId; | |
| console.log("relaunch -- creating window", windowId, windowData, isPrimaryStartupWindow ? "(primary startup)" : ""); | |
| const win = await createBrowserWindow(windowData, fullConfig, { | |
| unamePlatform, | |
| isPrimaryStartupWindow | |
| }); | |
| wins.push(win); | |
| const windowIds = clientData?.windowids ?? []; | |
| const wins: WaveBrowserWindow[] = []; | |
| const isFirstRelaunch = !hasCompletedFirstRelaunch; | |
| const primaryWindowId = windowIds.length > 0 ? windowIds[0] : null; | |
| for (const windowId of windowIds.slice().reverse()) { | |
| const windowData: WaveWindow = await WindowService.GetWindow(windowId); | |
| if (windowData == null) { | |
| console.log("relaunch -- window data not found, closing window", windowId); | |
| await WindowService.CloseWindow(windowId, true); | |
| continue; | |
| } | |
| const isPrimaryStartupWindow = isFirstRelaunch && windowId === primaryWindowId; | |
| console.log("relaunch -- creating window", windowId, windowData, isPrimaryStartupWindow ? "(primary startup)" : ""); | |
| const win = await createBrowserWindow(windowData, fullConfig, { | |
| unamePlatform, | |
| isPrimaryStartupWindow | |
| }); | |
| wins.push(win); |
🤖 Prompt for AI Agents
In emain/emain-window.ts around lines 811 to 828, the code reads
clientData.windowids which will throw if clientData is null/undefined; restore a
null-guard by either checking clientData first (if (!clientData) { log and
abort/return or set windowIds = []; }) or use optional chaining (const windowIds
= clientData?.windowids ?? []), ensuring you handle the null case consistently
(log the condition and avoid proceeding with relaunch when clientData is
missing).
There was a problem hiding this comment.
null client data is a failure. nothing to do and it should be impossible as the backend will initialize a client before startup.
There was a problem hiding this comment.
@sawka, understood! If the backend guarantees client initialization before this code path executes, then the null-guard is indeed unnecessary. Thank you for the clarification on the startup sequence.
✏️ Learnings added
Learnt from: sawka
PR: wavetermdev/waveterm#2433
File: emain/emain-window.ts:811-828
Timestamp: 2025-10-15T03:18:52.624Z
Learning: In emain/emain-window.ts, within the relaunchBrowserWindows function, ClientService.GetClientData() is guaranteed to return a valid client object and never null/undefined. The backend ensures a client is initialized before startup, so no null-guard is needed when accessing clientData.windowids.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| useEffect(() => { | ||
| if (!globalPrimaryTabStartup) { | ||
| return; | ||
| } | ||
| if (!clientData.tosagreed) { | ||
| return; | ||
| } | ||
| const lastVersion = clientData.meta?.["onboarding:lastversion"] ?? "v0.0.0"; | ||
| if (semver.lt(lastVersion, CurrentOnboardingVersion)) { | ||
| setUpgradeOnboardingOpen(true); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Upgrade modal never opens with current effect dependencies
The upgrade gating effect runs only once on mount (empty deps), so it observes the initial globalPrimaryTabStartup (usually false) and preloaded clientData (often missing meta). The guard exits early and never re-evaluates when startup flips true or when real client metadata arrives, so existing users will never see the upgrade modal—the core feature of this PR. Please include the relevant dependencies (and, if necessary, coerce the version strings) so the effect re-checks once the startup flag and metadata are ready before opening the modal.
🤖 Prompt for AI Agents
In frontend/app/modals/modalsrenderer.tsx around lines 38–49, the useEffect
currently has an empty dependency array so it never re-runs when
globalPrimaryTabStartup or clientData (tosagreed or meta) update; update the
effect to depend on [globalPrimaryTabStartup, clientData?.tosagreed,
clientData?.meta?.["onboarding:lastversion"]] and move the early-return guards
inside so the effect re-checks whenever startup flips or metadata arrives;
coerce the stored version with a fallback like
clientData.meta?.["onboarding:lastversion"] ?? "v0.0.0" (or otherwise normalize
to a string) before calling semver.lt(CurrentOnboardingVersion) and only call
setUpgradeOnboardingOpen(true) when semver.lt(lastVersion,
CurrentOnboardingVersion) and the modal isn’t already open.
There was a problem hiding this comment.
globalPrimaryTabStartup is set by the waveInit function which runs before the first react render (in wave.ts).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the initialization flow in wave.ts to verify when waveInit is called
# and how globalPrimaryTabStartup is set
# First, let's look at wave.ts to see the waveInit function and when it's called
cat frontend/wave.ts
Length of output: 7756
🏁 Script executed:
#!/bin/bash
# Check how clientData/atoms.client is populated and if meta is available on initial load
# Look at the WOS loading and client structure
# Check the client loading in initWave
rg -n -C3 "loadAndPinWaveObject.*client" frontend/wave.ts
# Check if there's any async meta loading after client is loaded
rg -n -A5 -B5 "onboarding:lastversion" frontend/
# Check the Client type definition to see if meta is part of the initial load
fd -e ts -e tsx | xargs rg -l "interface Client|type Client" | head -5
Length of output: 2966
@sawka Thank you for the clarification! You're absolutely right. Looking at the initialization flow in wave.ts, I can see that:
initGlobalis called withprimaryTabStartup: initOpts.primaryTabStartupbefore React renders- Client data (including
meta) is loaded viaWOS.loadAndPinWaveObject<Client>and awaited before React renders - React renders only after both these steps complete
So by the time the useEffect runs (after component mount), both globalPrimaryTabStartup and clientData.meta already have their final values. The empty dependency array [] is correct here because the effect is meant to run exactly once on mount of the primary startup tab to check if the upgrade modal should be shown.
I apologize for the incorrect analysis. The initialization sequence ensures the values are ready, and the empty deps array prevents unnecessary re-execution. The implementation is sound.
/\_/\
( o.o )
> ^ < "Got it!"
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
| <p className="p-3 border border-border rounded-md bg-hover/30"> | ||
| Wave AI is in active beta with included AI credits while we refine the experience. | ||
| We're actively improving it and would love your feedback in{" "} | ||
| <a target="_blank" href="https://discord.gg/XfvZ334gwU" className="hover:underline"> |
There was a problem hiding this comment.
[SECURITY] Add rel="noopener noreferrer" to external link.
The Discord link opens in a new tab without rel="noopener noreferrer", which is a security risk. The opened page can access window.opener and potentially redirect the parent page.
Apply this fix:
-<a target="_blank" href="https://discord.gg/XfvZ334gwU" className="hover:underline">
+<a target="_blank" rel="noopener noreferrer" href="https://discord.gg/XfvZ334gwU" className="hover:underline">
Discord
</a>📝 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.
| <a target="_blank" href="https://discord.gg/XfvZ334gwU" className="hover:underline"> | |
| <a target="_blank" rel="noopener noreferrer" href="https://discord.gg/XfvZ334gwU" className="hover:underline"> | |
| Discord | |
| </a> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 116-116: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🤖 Prompt for AI Agents
In frontend/app/onboarding/onboarding-upgrade.tsx around line 116, the anchor
tag opens an external Discord link with target="_blank" but lacks rel="noopener
noreferrer"; update the <a> element to include rel="noopener noreferrer" (as a
string prop in JSX) so the new tab cannot access window.opener and to mitigate
reverse tabnabbing/security risks.
There was a problem hiding this comment.
not really since we're opening from election to an external browser so there is no risk.
There was a problem hiding this comment.
@sawka, you're absolutely right! In Electron applications, external links opened with target="_blank" are typically handled by the OS and open in the system's default browser, creating a complete security boundary between the Electron app and the external browser window. The window.opener security risk only applies within the same browser context, which doesn't exist here.
Thank you for the clarification!
✏️ Learnings added
Learnt from: sawka
PR: wavetermdev/waveterm#2433
File: frontend/app/onboarding/onboarding-upgrade.tsx:116-116
Timestamp: 2025-10-15T03:20:22.633Z
Learning: In Electron applications, external links opened with `target="_blank"` open in the system's default browser, not within the Electron app context. This creates a security boundary that prevents `window.opener` access, so the typical web security concern about missing `rel="noopener noreferrer"` does not apply in the Electron context.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| Name: "read_text_file", | ||
| DisplayName: "Read Text File", | ||
| Description: "Read a text file from the filesystem. Can read specific line ranges or from the end. Detects and rejects binary files. Requires user approval.", | ||
| Description: "Read a text file from the filesystem. Can read specific line ranges or from the end. Detects and rejects binary files.", |
There was a problem hiding this comment.
Description no longer matches approval requirement
ToolApproval still returns uctypes.ApprovalNeedsApproval, so the tool continues to require manual approval. Dropping that note from the description makes the contract misleading for both users and the LLM. Please keep the approval requirement called out unless the runtime behavior changes.
🤖 Prompt for AI Agents
In pkg/aiusechat/tools_readfile.go around line 197, the Description string was
changed to remove the note that this tool requires manual approval, but
ToolApproval still returns uctypes.ApprovalNeedsApproval; restore consistency by
either (A) updating the Description to explicitly state that the tool requires
manual approval (re-add the approval note) so it matches ToolApproval, or (B) if
you intend to remove the approval requirement, change ToolApproval to return the
appropriate approved value; implement the chosen fix so the description and
ToolApproval behavior are consistent.
There was a problem hiding this comment.
the AI does not need to know the tool requires approval. in fact, it would often ask the user for approval first and then run the tool which requires approval again. this removes that annoyance.
There was a problem hiding this comment.
@sawka, that makes perfect sense! Hiding the approval requirement from the LLM's view prevents the redundant double-approval flow while still enforcing it at the runtime level. This is a smart UX improvement.
(\__/)
(='.'=) Got it!
(")_(")
✏️ Learnings added
Learnt from: sawka
PR: wavetermdev/waveterm#2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.196Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
No description provided.