builder secrets, builder config/data tab hooked up #2581
Conversation
WalkthroughAdds documentation for the OpenAIRequest.input JSON shapes and cleaning rules. Frontend: adds a new Builder "Config/Data" tab component and integrates it into Builder App Panel; refactors secrets handling to auto-save mappings, renames BuilderEnvTab → BuilderSecretTab, adds BuilderAppPanelModel.updateSecretBindings and extends TabType. Backend: exposes GetRole() on Anthropics and OpenAI message types and on GenAIMessage; adds ChatId and StepNum to AIMetrics and telemetry props and populates them in RunAIChat; sets X-Wave-ChatId/X-Wave-Version headers for OpenAI requests. Dev tooling: injects TSUNAMI_CORS in dev mode, adds DevModeCorsOrigins constant, and enables CORS + preflight handling in tsunami server handlers. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (7)
pkg/buildercontroller/buildercontroller.go (1)
327-329: Consider extracting CORS origins to a shared constant.The dev-mode CORS configuration is correct, but the same origin string
"http://localhost:5173,http://localhost:5174"appears in both this file andtsunamicontroller.go(lines 295-297), violating the DRY principle.Consider extracting to a shared constant in
pkg/wavebase/wavebase.goor a similar location:// In pkg/wavebase/wavebase.go const DevModeCORSOrigins = "http://localhost:5173,http://localhost:5174"Then use it in both files:
if wavebase.IsDevMode() { - cmd.Env = append(cmd.Env, "TSUNAMI_CORS=http://localhost:5173,http://localhost:5174") + cmd.Env = append(cmd.Env, "TSUNAMI_CORS="+wavebase.DevModeCORSOrigins) }pkg/blockcontroller/tsunamicontroller.go (1)
295-297: Consider extracting CORS origins to a shared constant.Same as the suggestion for
buildercontroller.go(lines 327-329): this hardcoded origin string"http://localhost:5173,http://localhost:5174"is duplicated. Extract it to a shared constant to maintain consistency and ease future updates.pkg/aiusechat/openai/openai-backend.go (1)
141-146: GetRole correctly exposes the chat message roleReturning
m.Message.Rolewhen present and""otherwise is fine, and works as expected for callers like CountUserMessages that only care about"user"messages. If you later need roles for pure tool-call messages, you may want to revisit the empty-string fallback.frontend/builder/builder-apppanel.tsx (1)
12-12: Config/Data tab is wired consistently with existing tabsThe new
BuilderConfigDataTabimport,TabButtonfor"configdata", and the associated content pane behindactiveTab === "configdata"mirror the existing tab patterns (Preview/Code/Files/Secrets) and are correctly wrapped inErrorBoundary. Functionally this looks solid.If you want to keep the file easier to scan later, you could optionally move the
"configdata"content block up near the Code pane so tab header order matches pane order.Also applies to: 314-320, 377-381
pkg/aiusechat/uctypes/usechat-types.go (1)
247-266: AIMetrics extensions and GenAIMessage.GetRole look consistentAdding
ChatId/StepNumtoAIMetricswith clear JSON tags is straightforward and should be backward compatible for existing JSON consumers. ExtendingGenAIMessagewithGetRole() stringaligns with the new usage inChatStore.CountUserMessagesand theOpenAIChatMessage.GetRoleimplementation.Just ensure all message types that are stored in
AIChat.NativeMessagesnow implementGetRole()so you don’t end up with compilation issues or panics from unexpected nil implementations.Also applies to: 277-281
frontend/builder/tabs/builder-configdatatab.tsx (1)
61-66: Tighten types forconfig/datainstead ofanyRight now
ConfigDataState.configand.dataare typed asanyand then stringified directly. If the backend shapes are reasonably stable, consider introducing explicit interfaces (or at leastunknown+ narrowing) for these fields to catch mismatches at compile time and improve editor support.Also applies to: 72-77, 198-201, 212-214
frontend/builder/tabs/builder-secrettab.tsx (1)
186-190: AvoidsetLocalBindingsin render and keep UI in sync on save failuresTwo related points around the new auto-save behavior:
State initialization in render (Lines [186-190])
CallingsetLocalBindingsduring render is an anti-pattern and easy to regress into a render loop later. It would be cleaner and safer to initialize/synclocalBindingsfromsecretBindingsin auseEffectthat runs whensecretBindingschanges, e.g. only whenlocalBindingsis still empty.Auto-save + error handling (Lines [198-214] and [220-239])
BothhandleMapDefaultandhandleSetAndMapoptimistically updatelocalBindingsbefore the RPC and never roll back on failure. Given the banner now promises “Changes are saved automatically” (Lines [245-252]), this can leave the UI showing a successful mapping even thoughWriteAppSecretBindingsCommandfailed. Consider capturing the previous bindings and restoring them in thecatchpath, or re-syncinglocalBindingsfrom the latestsecretBindingsafter an error so the display always reflects the persisted state.These are behavioral/maintainability improvements rather than blockers, but tightening them would make the auto-save semantics more robust.
Also applies to: 192-196, 198-214, 220-239, 245-252
📜 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 (15)
aiprompts/openai-request.md(1 hunks)frontend/builder/builder-apppanel.tsx(3 hunks)frontend/builder/store/builder-apppanel-model.ts(1 hunks)frontend/builder/tabs/builder-configdatatab.tsx(1 hunks)frontend/builder/tabs/builder-secrettab.tsx(3 hunks)pkg/aiusechat/anthropic/anthropic-backend.go(1 hunks)pkg/aiusechat/chatstore/chatstore.go(1 hunks)pkg/aiusechat/openai/openai-backend.go(1 hunks)pkg/aiusechat/openai/openai-convertmessage.go(1 hunks)pkg/aiusechat/uctypes/usechat-types.go(2 hunks)pkg/aiusechat/usechat.go(2 hunks)pkg/blockcontroller/tsunamicontroller.go(1 hunks)pkg/buildercontroller/buildercontroller.go(1 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(1 hunks)tsunami/engine/serverhandlers.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/builder/tabs/builder-configdatatab.tsx (1)
frontend/builder/store/builder-apppanel-model.ts (1)
BuilderAppPanelModel(21-317)
pkg/blockcontroller/tsunamicontroller.go (1)
pkg/wavebase/wavebase.go (1)
IsDevMode(113-115)
pkg/aiusechat/openai/openai-convertmessage.go (1)
cmd/server/main-server.go (1)
WaveVersion(50-50)
frontend/builder/tabs/builder-secrettab.tsx (4)
frontend/app/element/tooltip.tsx (1)
Tooltip(143-176)frontend/app/store/wshclientapi.ts (1)
RpcApi(682-682)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)
pkg/buildercontroller/buildercontroller.go (1)
pkg/wavebase/wavebase.go (1)
IsDevMode(113-115)
frontend/builder/builder-apppanel.tsx (1)
frontend/app/element/errorboundary.tsx (1)
ErrorBoundary(6-33)
pkg/aiusechat/usechat.go (2)
pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)pkg/aiusechat/uctypes/usechat-types.go (1)
AIMetrics(246-266)
🪛 LanguageTool
aiprompts/openai-request.md
[style] ~56-~56: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...l": "data:image/png;base64,..." } ``` - Can be a data URL or https:// URL - `filena...
(MISSING_IT_THERE)
⏰ 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). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (12)
pkg/aiusechat/openai/openai-convertmessage.go (1)
295-298: LGTM! Headers added correctly.The conditional
X-Wave-ChatIdheader and the unconditionalX-Wave-Versionheader are properly set, consistent with the existing header pattern in this function.pkg/telemetry/telemetrydata/telemetrydata.go (1)
130-131: LGTM! Telemetry fields added correctly.The new
WaveAIChatIdandWaveAIStepNumfields follow the existing naming convention and are positioned logically with other WaveAI telemetry properties.aiprompts/openai-request.md (1)
1-201: LGTM! Excellent documentation.This document provides clear and comprehensive documentation of the OpenAI request JSON structure, including object types, content block variants, and complete examples. The structure is well-organized and will be helpful for developers working with the OpenAI integration.
tsunami/engine/serverhandlers.go (2)
53-76: Verify CORS is only enabled in development mode and consider additional safeguards.While the CORS implementation is functional, there are several considerations:
Dev-mode enforcement: The code reads
TSUNAMI_CORSwithout verifying it's only set in development. While the builders/controllers conditionally set it (lines 327-329 in buildercontroller.go, lines 295-297 in tsunamicontroller.go), there's no enforcement here if the env var is set directly in production.Origin validation: The code trims spaces but doesn't validate that origins are well-formed URLs or reject dangerous patterns.
Credentials with dynamic origins: Setting
Access-Control-Allow-Credentials: truewith dynamically matched origins requires careful origin validation to prevent security issues.Consider these improvements:
func setCORSHeaders(w http.ResponseWriter, r *http.Request) bool { corsOriginsStr := os.Getenv("TSUNAMI_CORS") if corsOriginsStr == "" { return false } + // Safety check: only allow CORS in dev mode + if !wavebase.IsDevMode() { + log.Printf("WARNING: TSUNAMI_CORS is set but not in dev mode, ignoring") + return false + } origin := r.Header.Get("Origin") if origin == "" { return false } allowedOrigins := strings.Split(corsOriginsStr, ",") for _, allowedOrigin := range allowedOrigins { allowedOrigin = strings.TrimSpace(allowedOrigin) + if allowedOrigin == "" { + continue + } if allowedOrigin == origin { w.Header().Set("Access-Control-Allow-Origin", origin) w.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS") w.Header().Set("Access-Control-Allow-Headers", "Content-Type") w.Header().Set("Access-Control-Allow-Credentials", "true") return true } } return false }
228-234: LGTM! CORS and preflight handling applied consistently.The CORS headers and OPTIONS preflight handling are correctly applied to all four endpoints (
handleData,handleConfig,handleSchemas,handleManifest), with the pattern consistently placingsetCORSHeadersfirst, followed bysetNoCacheHeaders, then the preflight check.Also applies to: 258-264, 333-339, 552-558
pkg/aiusechat/anthropic/anthropic-backend.go (1)
49-51: LGTM! GetRole accessor added correctly.The new
GetRole()method follows the existing accessor pattern and is positioned logically alongsideGetMessageId()andGetUsage()methods. This aligns with theGenAIMessageinterface extension mentioned in the PR summary.pkg/aiusechat/usechat.go (2)
389-392: LGTM! Metrics fields initialized correctly.The
ChatIdandStepNumfields are properly initialized in the metrics object, withStepNumcalculated from the user message count, enabling per-chat and per-step correlation in telemetry.
578-579: LGTM! Telemetry integration is correct.The new
WaveAIChatIdandWaveAIStepNumfields are correctly included in the telemetry event, matching the field additions intelemetrydata.go(lines 130-131).pkg/aiusechat/chatstore/chatstore.go (1)
51-67: CountUserMessages implementation is simple and thread-safeLocking around the map read and in-place slice iteration is correct, and returning 0 for missing chats is a sensible default. Role filtering by
"user"aligns with the UI roles.frontend/builder/store/builder-apppanel-model.ts (1)
13-13: TabType union extension is consistent with new tabAdding
"configdata"toTabTypecleanly supports the new tab and remains compatible with existing usages (activeTabdefault,giveFocus, etc.).frontend/builder/tabs/builder-configdatatab.tsx (1)
61-77: Config/Data tab flow and fetch lifecycle look solidThe isRunning detection, auto-fetch on status change, and explicit NotRunning/Loading/Error states are wired cleanly, and the copy/refresh UX is straightforward and consistent with the rest of the builder UI. No blocking issues from a correctness perspective here.
Also applies to: 81-121, 146-173, 175-220
frontend/builder/tabs/builder-secrettab.tsx (1)
25-38: Secret status/tooltip logic reads clearlyThe addition of
isInvalidplus the updated tooltip text and distinct yellow/red/green icons gives a clear, low-friction signal for unmapped vs invalid vs valid bindings. The conditions on Lines [26-29] line up with the render logic on Lines [33-38], so the visual state should be reliable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/builder/store/builder-apppanel-model.ts (1)
124-132: Consider documenting silent no-op behavior.The method silently does nothing when
currentStatusis null. While this appears intentional and safe, adding a comment or debug log would improve maintainability and help future developers understand why this guard exists.updateSecretBindings(newBindings: { [key: string]: string }) { const currentStatus = globalStore.get(this.builderStatusAtom); if (currentStatus) { globalStore.set(this.builderStatusAtom, { ...currentStatus, secretbindings: newBindings, }); + } else { + console.debug("updateSecretBindings: no current status, skipping update"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/builder/store/builder-apppanel-model.ts(2 hunks)frontend/builder/tabs/builder-secrettab.tsx(3 hunks)pkg/blockcontroller/tsunamicontroller.go(1 hunks)pkg/buildercontroller/buildercontroller.go(2 hunks)pkg/tsunamiutil/tsunamiutil.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/blockcontroller/tsunamicontroller.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/buildercontroller/buildercontroller.go (2)
pkg/wavebase/wavebase.go (1)
IsDevMode(113-115)pkg/tsunamiutil/tsunamiutil.go (1)
DevModeCorsOrigins(14-14)
frontend/builder/tabs/builder-secrettab.tsx (5)
frontend/app/element/tooltip.tsx (1)
Tooltip(143-176)frontend/app/store/global.ts (1)
atoms(816-816)frontend/app/store/wshclientapi.ts (1)
RpcApi(682-682)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)
⏰ 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). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (8)
pkg/tsunamiutil/tsunamiutil.go (1)
14-15: LGTM! Well-chosen constant for development CORS origins.The constant appropriately centralizes the dev-mode CORS origins (Vite's default ports 5173-5174) for reuse across the codebase.
pkg/buildercontroller/buildercontroller.go (2)
20-20: LGTM! Import added for CORS origins constant.
328-330: CORS injection pattern verified as consistent across controllers.The verification confirms that the TSUNAMI_CORS environment variable injection follows a uniform pattern across all controllers:
- pkg/buildercontroller/buildercontroller.go:329 (under review) ✓
- pkg/blockcontroller/tsunamicontroller.go:296 ✓
Both locations use identical syntax, properly reference the
DevModeCorsOriginsconstant, and are guarded withif wavebase.IsDevMode()checks. The pattern is consumed correctly intsunami/engine/serverhandlers.go:54. No inconsistencies detected.frontend/builder/store/builder-apppanel-model.ts (1)
13-13: LGTM! New tab type added.The addition of "configdata" to the TabType union is straightforward and aligns with the new Config/Data tab introduced in the UI.
frontend/builder/tabs/builder-secrettab.tsx (4)
28-38: LGTM! Enhanced secret binding validation UI.The addition of
isInvalidstate and differentiated visual indicators (red AlertTriangle for invalid, green Check for valid, yellow AlertTriangle for unmapped) with context-aware tooltip messages provides clear feedback to users about secret binding status.
235-236: LGTM! Correctly validates required secrets.The
allRequiredBoundcomputation correctly filters for required secrets and validates that each has a non-empty trimmed binding fromsecretBindings. The logic is sound.
240-244: LGTM! UI messaging updated for auto-save.The heading and informational banner correctly reflect the new auto-save behavior, improving user understanding that changes persist automatically without manual save actions.
267-267: LGTM! Binding source correctly updated.Reading
currentBindingdirectly fromsecretBindingsinstead of local state correctly reflects the refactored auto-save architecture.
| const handleMapDefault = async (secretName: string) => { | ||
| const newBindings = { ...secretBindings, [secretName]: secretName }; | ||
|
|
||
| try { | ||
| const appId = globalStore.get(atoms.builderAppId); | ||
| await RpcApi.WriteAppSecretBindingsCommand(TabRpcClient, { | ||
| appid: appId, | ||
| bindings: localBindings, | ||
| bindings: newBindings, | ||
| }); | ||
| setIsDirty(false); | ||
| model.updateSecretBindings(newBindings); | ||
| globalStore.set(model.errorAtom, ""); | ||
| model.restartBuilder(); | ||
| } catch (err) { | ||
| console.error("Failed to save secret bindings:", err); | ||
| globalStore.set(model.errorAtom, `Failed to save secret bindings: ${err.message || "Unknown error"}`); | ||
| } finally { | ||
| setIsSaving(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Consider awaiting the builder restart.
The model.restartBuilder() call is not awaited (line 202). While the restart method has its own error handling, this creates a timing issue: the error is cleared on line 201, then if the restart fails moments later, the error suddenly appears. This can confuse users who see a brief success state before an error.
Consider awaiting the restart to provide clearer feedback:
model.updateSecretBindings(newBindings);
globalStore.set(model.errorAtom, "");
- model.restartBuilder();
+ await model.restartBuilder();
} catch (err) {Alternatively, if fire-and-forget is intentional, document why.
📝 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 handleMapDefault = async (secretName: string) => { | |
| const newBindings = { ...secretBindings, [secretName]: secretName }; | |
| try { | |
| const appId = globalStore.get(atoms.builderAppId); | |
| await RpcApi.WriteAppSecretBindingsCommand(TabRpcClient, { | |
| appid: appId, | |
| bindings: localBindings, | |
| bindings: newBindings, | |
| }); | |
| setIsDirty(false); | |
| model.updateSecretBindings(newBindings); | |
| globalStore.set(model.errorAtom, ""); | |
| model.restartBuilder(); | |
| } catch (err) { | |
| console.error("Failed to save secret bindings:", err); | |
| globalStore.set(model.errorAtom, `Failed to save secret bindings: ${err.message || "Unknown error"}`); | |
| } finally { | |
| setIsSaving(false); | |
| } | |
| }; | |
| const handleMapDefault = async (secretName: string) => { | |
| const newBindings = { ...secretBindings, [secretName]: secretName }; | |
| try { | |
| const appId = globalStore.get(atoms.builderAppId); | |
| await RpcApi.WriteAppSecretBindingsCommand(TabRpcClient, { | |
| appid: appId, | |
| bindings: newBindings, | |
| }); | |
| model.updateSecretBindings(newBindings); | |
| globalStore.set(model.errorAtom, ""); | |
| await model.restartBuilder(); | |
| } catch (err) { | |
| console.error("Failed to save secret bindings:", err); | |
| globalStore.set(model.errorAtom, `Failed to save secret bindings: ${err.message || "Unknown error"}`); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/builder/tabs/builder-secrettab.tsx around lines 191 to 207,
model.restartBuilder() is called without awaiting which can clear the error
prematurely; change the flow to await the restart and only clear the error after
a successful restart (or catch restart errors and set model.errorAtom
appropriately). Specifically, replace the fire-and-forget call with an awaited
call and wrap it in the try/catch so that after RpcApi and
model.updateSecretBindings succeed you await model.restartBuilder(); on success
clear the errorAtom, and if restart throws set the errorAtom with that error
message.
| const handleSetAndMap = async (secretName: string, secretValue: string) => { | ||
| await RpcApi.SetSecretsCommand(TabRpcClient, { [secretName]: secretValue }); | ||
| setAvailableSecrets((prev) => [...prev, secretName]); | ||
| setLocalBindings((prev) => ({ ...prev, [secretName]: secretName })); | ||
| setIsDirty(true); | ||
|
|
||
| const newBindings = { ...secretBindings, [secretName]: secretName }; | ||
|
|
||
| try { | ||
| const appId = globalStore.get(atoms.builderAppId); | ||
| await RpcApi.WriteAppSecretBindingsCommand(TabRpcClient, { | ||
| appid: appId, | ||
| bindings: newBindings, | ||
| }); | ||
| model.updateSecretBindings(newBindings); | ||
| globalStore.set(model.errorAtom, ""); | ||
| model.restartBuilder(); | ||
| } catch (err) { | ||
| console.error("Failed to save secret bindings:", err); | ||
| globalStore.set(model.errorAtom, `Failed to save secret bindings: ${err.message || "Unknown error"}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix UI state inconsistency on binding write failure.
If WriteAppSecretBindingsCommand fails (line 221), the UI is left in an inconsistent state: setAvailableSecrets (line 215) has already added the secret to the available list, but the binding write failed. Users will see the secret as available but the binding operation failed.
Move the optimistic UI update to after the binding write succeeds:
const handleSetAndMap = async (secretName: string, secretValue: string) => {
await RpcApi.SetSecretsCommand(TabRpcClient, { [secretName]: secretValue });
- setAvailableSecrets((prev) => [...prev, secretName]);
const newBindings = { ...secretBindings, [secretName]: secretName };
try {
const appId = globalStore.get(atoms.builderAppId);
await RpcApi.WriteAppSecretBindingsCommand(TabRpcClient, {
appid: appId,
bindings: newBindings,
});
+ setAvailableSecrets((prev) => [...prev, secretName]);
model.updateSecretBindings(newBindings);
globalStore.set(model.errorAtom, "");
- model.restartBuilder();
+ await model.restartBuilder();
} catch (err) {
console.error("Failed to save secret bindings:", err);
globalStore.set(model.errorAtom, `Failed to save secret bindings: ${err.message || "Unknown error"}`);
}
};This also addresses the non-awaited restartBuilder() call (same issue as in handleMapDefault).
🤖 Prompt for AI Agents
In frontend/builder/tabs/builder-secrettab.tsx around lines 213 to 232, the
handler currently performs an optimistic update to setAvailableSecrets before
persisting secret bindings, leaving UI inconsistent if
WriteAppSecretBindingsCommand fails; move the setAvailableSecrets update so it
only runs after the RPC call and model.updateSecretBindings succeed, and ensure
model.restartBuilder() is awaited (await model.restartBuilder()) so errors are
handled in the try/catch; keep clearing the error atom on success and set it in
the catch with the error message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/builder/tabs/builder-secrettab.tsx (1)
162-193: Secrets tab behavior matches the new auto‑save model.
- The manifest‑not‑available guard (
!builderStatus || !manifest) safely prevents null access and gives a clear “build first” message.- Using
builderStatus.secretbindings || {}as the single source of truth, and feeding it both intoallRequiredBoundandSecretRow.currentBinding, keeps the view consistent with backend status.handleMapDefaultandhandleSetAndMapboth constructnewBindings, persist viaWriteAppSecretBindingsCommand, then callmodel.updateSecretBindings(newBindings)and restart the builder, which aligns with the banner text that changes are saved automatically.- Computing
allRequiredBoundfromsortedSecretEntries.filter(!optional)ensures the warning only shows when required secrets truly lack non‑empty bindings.Overall, the new
BuilderSecretTabimplementation is cohesive withBuilderAppPanelModel.updateSecretBindingsand the rest of the builder panel flow.If you ever want more granular UX around restart failures (vs binding write failures), you could have
restartBuilderpropagate errors instead of swallowing them and then conditionally clearerrorAtomonly when both bind and restart succeed; current behavior is acceptable but lumps those cases together.Also applies to: 201-217, 223-246, 249-255, 272-278
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/builder/builder-apppanel.tsx(3 hunks)frontend/builder/store/builder-apppanel-model.ts(3 hunks)frontend/builder/tabs/builder-secrettab.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/builder/tabs/builder-secrettab.tsx (4)
frontend/app/element/tooltip.tsx (1)
Tooltip(143-176)frontend/app/store/wshclientapi.ts (1)
RpcApi(682-682)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)
frontend/builder/store/builder-apppanel-model.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(682-682)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
frontend/builder/builder-apppanel.tsx (3)
frontend/builder/tabs/builder-secrettab.tsx (1)
BuilderSecretTab(292-292)frontend/app/element/errorboundary.tsx (1)
ErrorBoundary(6-33)frontend/builder/tabs/builder-configdatatab.tsx (1)
BuilderConfigDataTab(225-225)
⏰ 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). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (4)
frontend/builder/store/builder-apppanel-model.ts (2)
13-25: Config/Data tab type wiring looks consistent.Extending
TabTypewith"configdata"and keepingactiveTabdefault"preview"aligns with the new tab inbuilder-apppanel.tsx; consumers ofTabTyperemain type‑safe and there’s no unexpected default to the new tab.
124-132: Secret binding updates and app switch flow look coherent.
updateSecretBindingspatchesbuilderStatusAtomwhile preserving all other fields, and safely no‑ops when status is still null; this matches how the tab consumessecretbindings. Switching toDeleteBuilderCommandinswitchBuilderAppkeeps the overall sequence (stop/delete, clearbuilder:appid, clear UI builder app id, then refresh) intact, assuming backend semantics ofDeleteBuilderCommandmatch the previous stop semantics.If there’s any ambiguity about
DeleteBuilderCommand’s contract vs the old stop command, double‑check its RPC signature and side‑effects against backend docs to ensureSetRTInfoCommandon the samebuilderIdis still valid immediately after delete.Also applies to: 225-241
frontend/builder/builder-apppanel.tsx (1)
12-16: New Config/Data tab and renamed Secrets tab are wired correctly.Importing
BuilderConfigDataTab/BuilderSecretTab, adding the"configdata"TabButton, and rendering both tabs insideErrorBoundarymirrors the existing pattern for other tabs and matches the extendedTabType; the tab switching logic and focus handling remain consistent.Also applies to: 314-320, 372-381
frontend/builder/tabs/builder-secrettab.tsx (1)
25-38: Secret status visualization is clear and state‑correct.Deriving
isInvalidfromisMapped && !isValid, adjusting tooltip copy, and using distinct icons/colors for unmapped (yellow), invalid (red), and valid (green) bindings makes the mapping state easy to reason about and matches the logic onavailableSecretsandcurrentBinding.
builder secrets, builder config/data tab hooked up, and tsunami cors config env var