Repository navigation
[codex] warn before relay toggle restarts host service - #3845
Conversation
Greptile SummaryThis PR routes both the enable and disable relay-toggle actions through the Confidence Score: 5/5Safe to merge — logic is correct, state is properly reset between dialog openings, and the security gate for enabling is preserved. Both changed files are small, focused, and follow existing patterns. The No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/settings/security/components/SecuritySettings/SecuritySettings.tsx | Routes both enable and disable toggle actions through the confirmation dialog by adding confirmTargetEnabled state; captures value in closure for runToggle |
| apps/desktop/src/renderer/routes/_authenticated/settings/security/components/SecuritySettings/components/ExposeViaRelayConfirmDialog/ExposeViaRelayConfirmDialog.tsx | Adds targetEnabled prop to branch dialog title, body, typed-confirmation requirement, button variant, and button label between enable and disable flows |
Sequence Diagram
sequenceDiagram
participant U as User
participant SW as Switch
participant SS as SecuritySettings
participant D as ExposeViaRelayConfirmDialog
participant API as tRPC Mutation
U->>SW: Toggle (on or off)
SW->>SS: handleChange(next)
SS->>SS: setConfirmTargetEnabled(next)
SS->>SS: setConfirmOpen(true)
SS->>D: open=true, targetEnabled=next
alt targetEnabled = true (Enable)
D-->>U: Show security risk warning + "I understand" input
U->>D: Types "I understand"
U->>D: Click "Enable"
D->>SS: onConfirm()
SS->>SS: setConfirmOpen(false)
SS->>API: runToggle(true)
else targetEnabled = false (Disable)
D-->>U: Show restart/disconnect warning (no input required)
U->>D: Click "Disable"
D->>SS: onConfirm()
SS->>SS: setConfirmOpen(false)
SS->>API: runToggle(false)
end
API-->>SS: toast.promise (loading -> success/error)
API-->>SS: optimistic update + invalidate
Reviews (1): Last reviewed commit: "warn before relay toggle restarts host s..." | Re-trigger Greptile
📝 WalkthroughWalkthroughBoth enabling and disabling the "Expose via Relay" setting now open a confirmation dialog; the intended target state is stored in component state and passed to the dialog so confirmation executes the correct enable or disable action. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SecuritySettings
participant ConfirmDialog
participant ToggleService
User->>SecuritySettings: click relay toggle
SecuritySettings->>SecuritySettings: set confirmTargetEnabled, open ConfirmDialog
SecuritySettings->>ConfirmDialog: show(targetEnabled)
alt targetEnabled == true
User->>ConfirmDialog: type CONFIRM_PHRASE and click confirm
else targetEnabled == false
User->>ConfirmDialog: click confirm
end
ConfirmDialog->>SecuritySettings: onConfirm(targetEnabled)
SecuritySettings->>ToggleService: runToggle(targetEnabled)
ToggleService-->>SecuritySettings: success/failure
SecuritySettings-->>User: update UI / show result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/security/components/SecuritySettings/components/ExposeViaRelayConfirmDialog/ExposeViaRelayConfirmDialog.tsx (1)
33-37:⚠️ Potential issue | 🟡 MinorReset typed confirmation when mode flips while open.
On Line 37,
canConfirmcan staytrueiftargetEnabledchanges in-place while the dialog remains open andtypedstill matches. Consider clearingtypedontargetEnabledtransitions as well, not only on close.Suggested patch
-useEffect(() => { - if (!open) setTyped(""); -}, [open]); +useEffect(() => { + if (!open) { + setTyped(""); + return; + } + if (targetEnabled) { + setTyped(""); + } +}, [open, targetEnabled]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/security/components/SecuritySettings/components/ExposeViaRelayConfirmDialog/ExposeViaRelayConfirmDialog.tsx` around lines 33 - 37, The dialog leaves typed matching CONFIRM_PHRASE when targetEnabled flips while open, so add logic to reset typed on targetEnabled transitions (not just on open): in ExposeViaRelayConfirmDialog add a useEffect that watches [targetEnabled, open] (or include targetEnabled in the existing useEffect dependency) and calls setTyped("") whenever open is true and targetEnabled changes, ensuring canConfirm (which uses targetEnabled and typed) is recalculated correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/security/components/SecuritySettings/components/ExposeViaRelayConfirmDialog/ExposeViaRelayConfirmDialog.tsx`:
- Around line 33-37: The dialog leaves typed matching CONFIRM_PHRASE when
targetEnabled flips while open, so add logic to reset typed on targetEnabled
transitions (not just on open): in ExposeViaRelayConfirmDialog add a useEffect
that watches [targetEnabled, open] (or include targetEnabled in the existing
useEffect dependency) and calls setTyped("") whenever open is true and
targetEnabled changes, ensuring canConfirm (which uses targetEnabled and typed)
is recalculated correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c53c8fcf-985a-40f6-bea5-a6fadfec9b89
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/settings/security/components/SecuritySettings/components/ExposeViaRelayConfirmDialog/ExposeViaRelayConfirmDialog.tsx
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
* warn before relay toggle restarts host service * trim relay toggle dialog copy * clarify relay toggle kills terminals
Summary
Warn before changing the relay exposure setting in either direction, because the setting mutation restarts active host-service instances and stops running terminals.
Changes
Enable and restartorDisable and restart.Impact
Users now see that changing Relay access restarts the host service, stops running terminals, and interrupts file watches or other host-backed work before the setting is applied.
Validation
bunx @biomejs/biome@2.4.2 check --write --unsafe apps/desktop/src/renderer/routes/_authenticated/settings/security/components/SecuritySettings/SecuritySettings.tsx apps/desktop/src/renderer/routes/_authenticated/settings/security/components/SecuritySettings/components/ExposeViaRelayConfirmDialog/ExposeViaRelayConfirmDialog.tsxbun run --cwd apps/desktop typecheckSummary by cubic
Show a confirmation before enabling or disabling Superset Relay access, warning that the change restarts the host service, kills running terminals, and interrupts file watches and other host-backed work. Enabling still requires typing "I understand"; disabling only needs a simple confirm.
Written for commit 635f1bc. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes