[Feat] Dialog Refactor#1140
Open
RichardAnderson wants to merge 8 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughImplements a typed dialog registry, a Zustand dialog store, a DialogHost and useDialog hook; adds multiple registered dialog components, integrates the host into the app layout, converts many inline dialogs/actions to registry-driven open calls, and introduces DeploymentTable with controller wiring and small UI/table tweaks. ChangesCentralized dialog system and deployments table
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant Hook as useDialog()
participant Store as useDialogStore
participant Host as DialogHost
participant Dialog as RegisteredDialog
UI->>Hook: dialog.<key>.open(typedProps)
Hook->>Store: open(key, props)
Store-->>Host: active={key, props}
Host->>Dialog: render(open=true, props)
Dialog-->>Store: onOpenChange(false) -> close()
Store-->>Host: clear active, restore focus
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
resources/js/pages/backups/components/edit-backup.tsx (1)
35-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse form-native submit for “Save” in the edit backup dialog
- The form already calls
onSubmit={submit}(which doese.preventDefault()), but the “Save” button bypasses it by usingtype="button"withonClick={submit}.Suggested fix
- <Button form="edit-backup-form" type="button" onClick={submit} disabled={form.processing}> + <Button form="edit-backup-form" type="submit" disabled={form.processing}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/backups/components/edit-backup.tsx` around lines 35 - 95, The Save button is bypassing the form-native submit by using type="button" with onClick={submit}; change the Button (the one rendering Save that references form="edit-backup-form") to use type="submit" and remove its onClick handler so the Form's onSubmit={submit} (and its e.preventDefault() logic) is used natively; ensure the Form id "edit-backup-form" and the submit function remain unchanged so validation/processing (form.processing) still controls the disabled state.resources/js/pages/cronjobs/components/columns.tsx (1)
28-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix keyboard activation for Cron job dropdown action
DropdownMenuItemcurrently callsonSelect={(e) => e.preventDefault()}and triggers submit viaonClick, so keyboard selection won’t runsubmit. Put the action ononSelect.♿ Suggested fix
- <DropdownMenuItem onSelect={(e) => e.preventDefault()} onClick={submit} disabled={form.processing}> + <DropdownMenuItem onSelect={submit} disabled={form.processing}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/cronjobs/components/columns.tsx` around lines 28 - 33, The DropdownMenuItem currently prevents default in onSelect and triggers submit on onClick which breaks keyboard activation; move the submit invocation into the onSelect handler (e.g. call submit inside onSelect), remove or adjust the existing onClick usage, and ensure you still call e.preventDefault() if needed before calling submit so the form.post(route(routeName, routeParams)) in submit runs for both mouse and keyboard while respecting form.processing and showing LoaderCircleIcon.resources/js/pages/site-features/components/feature-action.tsx (1)
28-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear dynamic form state between action opens.
This controlled dialog now closes on success but no longer resets form data. Values can persist into the next action open and submit unintended fields.
Proposed fix
const submit = (e: FormEvent) => { e.preventDefault(); form.post( ... { - onSuccess: () => onOpenChange(false), + onSuccess: () => { + onOpenChange(false); + form.reset(); + form.clearErrors(); + }, }, ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/site-features/components/feature-action.tsx` around lines 28 - 43, The form state persists between dialog opens because the useForm instance (form) is never reset; update the submit flow and dialog open handling in feature-action.tsx so the form is cleared between uses: call form.reset() after a successful post (inside submit's onSuccess before/after calling onOpenChange(false)) and also clear form when the dialog is opened (add an effect or handler that calls form.reset() when the dialog's open state becomes true) to ensure no stale values remain; reference the useForm hook, the submit function, form.post, and onOpenChange to locate where to add the reset calls.resources/js/pages/scripts/components/form.tsx (1)
16-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRehydrate ScriptForm state when reopening or switching scripts
useFormis initialized fromscript?.name/contentonly on mount, and the only existinguseEffectupdates focus—so reopening or switching thescriptprop can retain stale field values/errors. The MonacoEditoralso usesdefaultValue, so content won’t reliably update after callingform.setData.Proposed fix
useEffect(() => { setFocused(open); return () => setFocused(false); }, [open, setFocused]); + + useEffect(() => { + if (!open) return; + form.setData({ + name: script?.name ?? '', + content: script?.content ?? '', + }); + form.clearErrors(); + }, [open, script?.id]);Also resync Monaco content by switching the
Editorto a controlledvalue={form.data.content}(or remounting it with akeytied toscript?.id/ reopen).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/scripts/components/form.tsx` around lines 16 - 26, ScriptForm's useForm is only initialized on mount so reopening or switching script leaves stale fields and errors; add a useEffect in the ScriptForm component that watches [script, open] and calls form.setData({ name: script?.name ?? '', content: script?.content ?? '' }) (and clear errors/state if needed) to rehydrate the form when the modal is opened or the script prop changes, and make the Monaco Editor controlled by switching from defaultValue to value={form.data.content} (or remount it by giving it a key tied to script?.id) so the editor content stays in sync with form.data.content.resources/js/pages/server-features/components/feature-action.tsx (1)
68-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove click-bound submit to avoid duplicate requests.
This button already targets the form; keeping
onClick={submit}can execute submit logic twice. Use explicittype="submit"and let the formonSubmithandle it.Suggested fix
- <Button form="action-form" disabled={form.processing} onClick={submit}> + <Button form="action-form" type="submit" disabled={form.processing}>As per coding guidelines, form submit buttons in dialogs must use
form="<form-id>" type="submit"and not rely on buttononClick.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/server-features/components/feature-action.tsx` at line 68, The Button currently binds submit via onClick which can trigger duplicate submissions; remove the onClick={submit} from the Button that references form="action-form" and instead set type="submit" so the form's onSubmit handles submission; ensure the existing submit handler (submit) is wired to the form's onSubmit (not the Button) and keep disabled={form.processing} intact to prevent double requests.resources/js/pages/hosted-domains/components/edit-hosted-domain.tsx (1)
44-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEdited domain state may leak between dialog sessions.
useFormis seeded fromhostedDomainonce, and success only closes. If this dialog is reused, stale values can appear for a different domain.Suggested fix
-import { FormEvent } from 'react'; +import { FormEvent, useEffect } from 'react'; ... + useEffect(() => { + if (!open) return; + form.setData({ + domain: hostedDomain.domain, + type: hostedDomain.type, + ssl_method: hostedDomain.ssl_method, + ssl_id: hostedDomain.ssl_id ? String(hostedDomain.ssl_id) : '', + }); + form.clearErrors(); + }, [open, hostedDomain.id]);Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/hosted-domains/components/edit-hosted-domain.tsx` around lines 44 - 49, The form created with useForm<EditForm> is only seeded once from hostedDomain so if the dialog is reused for another hostedDomain it can show stale values; update the component to reset or reinitialize the form whenever hostedDomain changes (e.g., in a useEffect that calls the form reset method or re-creates form state) so fields (domain, type, ssl_method, ssl_id) are updated for the new hostedDomain; apply the same approach for the other instance referenced around lines 73-74 to ensure no state leaks between dialog sessions.resources/js/pages/firewall/components/form.tsx (1)
43-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFirewall form can submit stale values after switching rows.
The form is initialized once from
firewallRule, and success now only closes the dialog. In a reused controlled dialog, opening another rule can carry over prior state.Suggested fix
+import { FormEvent, useEffect } from 'react'; ... + useEffect(() => { + if (!open) return; + form.setData({ + name: firewallRule?.name || '', + type: firewallRule?.type || '', + protocol: firewallRule?.protocol || '', + port: firewallRule?.port?.toString() || '', + source_any: !firewallRule?.source, + source: firewallRule?.source || '', + mask: firewallRule?.mask?.toString() || '', + }); + form.clearErrors(); + }, [open, firewallRule?.id]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/firewall/components/form.tsx` around lines 43 - 54, The submit handler currently reuses the form instance initialized once from firewallRule which allows stale values when the dialog is reused; update the component to reset or reinitialize the form whenever the active firewallRule or the dialog open state changes and also after a successful submit. Concretely, add logic (e.g. useEffect or the dialog open handler) that calls form.reset(...) or form.setData(...) with the current firewallRule fields (or empty defaults when creating) when firewallRule changes or when opening the dialog, and ensure the onSuccess callbacks for both form.put and form.post also reset the form (and then call onOpenChange(false)) so the next open starts with fresh data; reference the submit function, form.put, form.post, firewallRule and onOpenChange to locate where to hook the reset.
🧹 Nitpick comments (5)
resources/js/components/dialogs/log-viewer-dialog.tsx (1)
32-34: ⚡ Quick winReplace raw internal anchor with Inertia
Link.This is an internal route and should use
Link/router APIs rather than a raw<a>.Proposed fix
+import { Link } from '`@inertiajs/react`'; @@ - <a href={route('logs.download', { server: serverId, log: logId })} target="_blank" rel="noopener noreferrer"> - <Button variant="outline">Download</Button> - </a> + <Button asChild variant="outline"> + <Link href={route('logs.download', { server: serverId, log: logId })} target="_blank" rel="noopener noreferrer"> + Download + </Link> + </Button>As per coding guidelines: "
**/*.{ts,tsx}: Use<Link>orrouter.visit()for internal navigation — never raw<a>tags for internal routes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/components/dialogs/log-viewer-dialog.tsx` around lines 32 - 34, Replace the raw anchor used for internal navigation with Inertia's Link: change the <a href={route('logs.download', { server: serverId, log: logId })} ...> wrapper around the Button to a Link from Inertia (e.g. import { Link } from '`@inertiajs/react`' or '`@inertiajs/inertia-react`' depending on the project), keeping the same href (route('logs.download', { server: serverId, log: logId })) and attributes (target="_blank" rel="noopener noreferrer") and ensure the Link wraps the Button component so the internal route uses the router API rather than a raw <a>.resources/js/pages/domains/components/columns.tsx (1)
7-7: 💤 Low valueUnused import after refactor.
The
routerimport from@inertiajs/reactappears to be unused after the dialog refactor removed the inline form submission logic.🧹 Proposed fix
-import { router } from '`@inertiajs/react`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/domains/components/columns.tsx` at line 7, Remove the now-unused router import from `@inertiajs/react` in resources/js/pages/domains/components/columns.tsx: locate the import line that includes "router" and delete "router" (or remove the entire import if nothing else is imported) so there are no unused imports after the dialog refactor.resources/js/pages/php/components/default-cli.tsx (1)
12-19: ⚡ Quick winInclude an explicit confirm
variantin this dialog payload.This keeps confirm dialog behavior consistent with the project’s standardized confirm contract.
Suggested patch
dialog.confirm.open({ title: 'Make default cli', description: `Are you sure you want to make PHP ${service.version} the default cli?`, + variant: 'default', confirmLabel: 'Save', method: 'post', url: route('php.default-cli', { server: service.server_id, service: service.id }), data: { version: service.version }, })As per coding guidelines
For simple confirm or destructive actions, use dialog.confirm.open() with title, description, variant, confirmLabel, method, and url.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/php/components/default-cli.tsx` around lines 12 - 19, Add the missing variant key to the dialog.confirm.open payload to comply with the confirm contract: update the call to dialog.confirm.open (in default-cli.tsx) to include a variant property (e.g., variant: 'confirm' or 'destructive' as appropriate for this action) alongside title, description, confirmLabel, method, url and data so the dialog behavior matches the project's standardized confirm contract.resources/js/pages/plugins/components/install.tsx (1)
11-18: ⚡ Quick winInclude
variantindialog.confirm.openpayload for consistency.Add explicit
variantto match the standardized confirm contract.🛠️ Suggested fix
dialog.confirm.open({ title: 'Install plugin', description: `Are you sure you want to install the plugin located at ${plugin.folder}?`, + variant: 'default', confirmLabel: 'Install', method: 'patch', url: route('plugins.install'), data: { id: plugin.id }, })As per coding guidelines:
For simple confirm or destructive actions, use dialog.confirm.open() with title, description, variant, confirmLabel, method, and url.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/plugins/components/install.tsx` around lines 11 - 18, Add the missing variant property to the dialog.confirm.open call to match the standardized confirm contract: update the payload passed to dialog.confirm.open (the call that currently uses title, description, confirmLabel, method, url, data with plugin.folder and plugin.id) to include variant (e.g., 'default' or the appropriate variant used elsewhere) alongside the other fields so the confirm dialog follows the required contract.resources/js/pages/projects/components/invitations.tsx (1)
23-26: ⚡ Quick winUse Inertia navigation for the internal accept route.
Replace
window.location.hrefwithrouter.visit()to keep navigation consistent with app routing behavior.🔁 Suggested fix
+import { router } from '`@inertiajs/react`'; @@ <DropdownMenuItem onSelect={(e) => { e.preventDefault(); - window.location.href = `/settings/projects/${invitation.project_id}/invitations/accept`; + router.visit(`/settings/projects/${invitation.project_id}/invitations/accept`); }} >As per coding guidelines:
Use <Link> or router.visit() for internal navigation — never raw <a> tags for internal routes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/pages/projects/components/invitations.tsx` around lines 23 - 26, Replace the direct window.location.href navigation in the onSelect handler with Inertia's router.visit to preserve SPA routing: change the handler to call router.visit(`/settings/projects/${invitation.project_id}/invitations/accept`) and ensure you import the router (e.g. import { router } from '`@inertiajs/react`' or use the useRouter hook) at the top of the file so internal navigation for the onSelect uses Inertia routing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Tables/DeploymentTable.php`:
- Around line 14-27: Add PHPDoc blocks for the new DeploymentTable class and its
methods/properties: document the class with a brief description and tags (e.g.,
`@package` or `@author` if used), add method PHPDoc for query() and columns()
describing purpose, parameters (none) and return types (void/array), and
document the protected properties $tableSettings, $defaultSort and $perPage with
`@var` annotations; update DeploymentTable, query(), columns(), and the properties
in the class to include these PHPDoc blocks so the file conforms to the
repository PHP documentation rule.
- Line 35: The Column::data accessor for 'server_id' dereferences the nullable
site relation on the Deployment model and can throw when site is null; update
the accessor (the Column::data callback that takes Deployment $deployment) to
guard the nullable relationship (e.g., check $deployment->site or use PHP
null-safe access) and return null (or a safe default) when site is missing so
you don't call ->server_id on null.
In `@resources/js/components/dialogs/confirmation-dialog.tsx`:
- Around line 38-57: The current submit function is invoked via Button onClick
and lacks a FormEvent, so change the pattern to a real form submit: convert
submit into an onSubmit handler that accepts (e: FormEvent), call
e.preventDefault(), then run the same logic that calls
form.delete/post/patch/put and onOpenChange/onSuccess; attach this handler to
the form element as onSubmit and make the dialog action button a plain
type="submit" (remove its onClick). Update references to the existing submit
function in ConfirmationDialog and ensure the handler still uses the same
visitOptions and onOpenChange/onSuccess calls.
In `@resources/js/components/dialogs/php-extensions-dialog.tsx`:
- Around line 30-31: The current destructuring const [, php] =
Object.entries(configs.service.services).filter(([key]) => key === 'php')[0] ||
null can throw when no php entry exists; replace this with a safe lookup (use
.find or check the [0] result) to get the php entry before destructuring — e.g.
obtain phpEntry = Object.entries(configs.service.services).find(([key]) => key
=== 'php') and set php = phpEntry ? phpEntry[1] : null — then keep the existing
if (!php) return null; guard; update references to php accordingly in
php-extensions-dialog.tsx to avoid destructuring from null.
In `@resources/js/components/dialogs/php-ini-dialog.tsx`:
- Around line 62-64: The Monaco Editor's value is bound to query.data.ini which
causes user edits to be overwritten on rerenders; change the Editor's value prop
to use form.data.ini so edits persist (reference Editor and
form.setData/form.data.ini). Also adjust the hydration gate in the response
handling: replace the truthy check (if (response.data?.ini)) with a string-type
check (e.g., typeof response.data?.ini === 'string') so empty-string INI is
correctly hydrated into form.setData('ini', response.data.ini). Ensure these two
changes are applied where the Editor component and the response handling logic
are implemented.
In `@resources/js/components/dialogs/plugin-logs-dialog.tsx`:
- Around line 18-29: expandedItems currently tracks expansion by array index
(setExpandedItems, toggleExpanded) and Card components use index as key, which
breaks when the errors array reorders; change to use a stable identifier from
each error (e.g., error.id or a concatenation of unique fields like
error.timestamp + error.type) instead of index: update toggleExpanded to accept
an id (string | number), store that id in expandedItems Set, update any callers
to pass error.id, and change Card key from index to that same stable id so
expansion state follows the correct error item even after reordering or refresh.
In `@resources/js/pages/backups/components/columns.tsx`:
- Around line 20-29: The confirmation message can render "undefined" when
backup.database is null; update the target computation used by DropdownMenuItem
and dialog.confirm.open so it never yields undefined (e.g. in the expression
that defines target which currently uses backup.type === 'database' ?
backup.database?.name : backup.path). Replace that ternary with a guarded
fallback (use nullish coalescing) so for database backups you use
backup.database?.name ?? '(unknown database)' and for file backups use
backup.path ?? '(unknown path)', and keep dialog.confirm.open and the
title/description templates unchanged to consume this safe target.
In `@resources/js/pages/dns-providers/components/edit-dialog.tsx`:
- Around line 29-32: The form is initialized once with useForm({ name:
dnsProvider.name, global: dnsProvider.global }) so its state can drift when
dnsProvider changes; update the logic to reset the form whenever dnsProvider
changes by calling the form reset/update API (e.g., form.setValues or
form.reset) inside an effect that watches dnsProvider (or re-create the form by
keying the component on dnsProvider.id); locate the useForm call and ensure you
update/reset the form values from dnsProvider in an useEffect that runs when
dnsProvider changes.
In `@resources/js/pages/domains/components/record-form.tsx`:
- Around line 67-74: The onSuccess handlers for form.patch and form.post
currently only close the dialog (onOpenChange(false)) and can leave stale form
state; update both handlers to also reset or re-sync the form state (e.g., call
form.reset() and/or set form data to the current record/default values) before
or immediately after calling onOpenChange(false) so the next open starts with a
clean/accurate form (referencing the existing form.patch, form.post, and
onSuccess handlers to locate where to change).
In `@resources/js/pages/hosted-domains/components/create-hosted-domain.tsx`:
- Around line 30-36: The form's ssl_method default uses
site.webserver_default_ssl_method even when that value was filtered out by
allowedSslMethods, so change the initialization to pick a valid default from
sslMethodOptions: compute sslMethodOptions first (as you already do) and set
form's ssl_method to site.webserver_default_ssl_method only if sslMethodOptions
contains that value, otherwise fall back to a safe option such as 'none' or the
first sslMethodOptions[0].value; update the form initialization
(useForm<CreateForm>) to reference sslMethodOptions when determining the
ssl_method default to ensure the select starts with a permitted value.
In `@resources/js/pages/plugins/components/disable.tsx`:
- Line 14: The confirmation description currently uses plugin.name which may be
undefined; update the description in the disable dialog to use a null-safe label
(e.g. use plugin?.name ?? plugin?.id ?? a fallback like "this plugin") so the
copy never renders empty or "undefined"; change the string interpolation at the
description assignment (where plugin.name is used) to reference the safe
fallback expression.
In `@resources/js/pages/plugins/components/enable.tsx`:
- Around line 11-18: Update the dialog.confirm.open call to include an explicit
variant and make the description null-safe: add the variant property (e.g.,
variant: 'destructive' or 'default' per intent) and guard the plugin label by
using a fallback when plugin.name is missing (e.g., use plugin.name ?? 'this
plugin' or similar) in the description string; keep the existing confirmLabel,
method, url (route('plugins.enable')) and data ({ id: plugin.id }) intact so
dialog.confirm.open has title, description, variant, confirmLabel, method, url,
and data.
In `@resources/js/pages/plugins/components/update.tsx`:
- Line 13: The confirmation description uses plugin.name which can be undefined;
change the template in update.tsx (the description field that references
plugin.name) to reuse the uninstall action's fallback pattern by substituting
plugin.name with a safe fallback (e.g. plugin.name ?? plugin.slug ?? 'this
plugin') so the dialog never renders "undefined" and matches the uninstall
wording.
In `@resources/js/pages/server-providers/components/edit-dialog.tsx`:
- Around line 23-26: The form created by useForm only captures serverProvider at
mount, causing stale values when the dialog is reused; update the form whenever
serverProvider changes by adding an effect that resets or re-initializes the
form values (e.g., useEffect(() => form.setValues({ name: serverProvider.name,
global: serverProvider.global }), [serverProvider]) ) or recreate the form when
serverProvider changes (useMemo or recreate useForm with serverProvider as
dependency). Target the useForm call and the form instance (form.setValues /
form.reset / recreating useForm) to ensure the dialog always reflects the
current serverProvider.
In `@resources/js/pages/servers/components/actions.tsx`:
- Around line 65-72: The confirm dialog payloads for server actions are missing
the required variant field; update both dialog.confirm.open(...) calls (the
reboot/restart dialog and the other confirm at the second occurrence) to include
the project's standard variant (e.g., add variant: 'danger') alongside title,
description, confirmLabel, method, and url so they conform to the confirm
contract.
In `@resources/js/pages/services/components/config-file-dialog.tsx`:
- Around line 58-60: The Monaco Editor is bound to query.data.content causing
edits to be overwritten and the form is only hydrated when response.data.content
is truthy so empty config strings are ignored; change the Editor to use the form
state (use form.data.content as the value and call form.setData('content',
newValue) in onChange) so typing updates and persists in the form, and change
the hydration to set form.setData('content', response.data.content) when
response.data.content is defined (check for !== undefined/null rather than
truthiness) so empty-string configs are accepted; update references to Editor
value prop, onChange handler, and the hydration call that currently checks
response.data?.content.
In `@resources/js/stores/dialog-store.ts`:
- Around line 24-32: The close() function unconditionally focuses the previous
trigger on the next animation frame, which can steal focus if another dialog
opened in the meantime; modify close() (which uses triggerElement and set({
active: null })) to remember the dialog identity (or current active value)
before calling set, then inside requestAnimationFrame re-check that the dialog
store's active value is still the expected value (or null) and that
triggerElement still matches the captured trigger and isConnected before calling
focus(); this ensures focus is only restored when no other dialog became active
in the interim.
---
Outside diff comments:
In `@resources/js/pages/backups/components/edit-backup.tsx`:
- Around line 35-95: The Save button is bypassing the form-native submit by
using type="button" with onClick={submit}; change the Button (the one rendering
Save that references form="edit-backup-form") to use type="submit" and remove
its onClick handler so the Form's onSubmit={submit} (and its e.preventDefault()
logic) is used natively; ensure the Form id "edit-backup-form" and the submit
function remain unchanged so validation/processing (form.processing) still
controls the disabled state.
In `@resources/js/pages/cronjobs/components/columns.tsx`:
- Around line 28-33: The DropdownMenuItem currently prevents default in onSelect
and triggers submit on onClick which breaks keyboard activation; move the submit
invocation into the onSelect handler (e.g. call submit inside onSelect), remove
or adjust the existing onClick usage, and ensure you still call
e.preventDefault() if needed before calling submit so the
form.post(route(routeName, routeParams)) in submit runs for both mouse and
keyboard while respecting form.processing and showing LoaderCircleIcon.
In `@resources/js/pages/firewall/components/form.tsx`:
- Around line 43-54: The submit handler currently reuses the form instance
initialized once from firewallRule which allows stale values when the dialog is
reused; update the component to reset or reinitialize the form whenever the
active firewallRule or the dialog open state changes and also after a successful
submit. Concretely, add logic (e.g. useEffect or the dialog open handler) that
calls form.reset(...) or form.setData(...) with the current firewallRule fields
(or empty defaults when creating) when firewallRule changes or when opening the
dialog, and ensure the onSuccess callbacks for both form.put and form.post also
reset the form (and then call onOpenChange(false)) so the next open starts with
fresh data; reference the submit function, form.put, form.post, firewallRule and
onOpenChange to locate where to hook the reset.
In `@resources/js/pages/hosted-domains/components/edit-hosted-domain.tsx`:
- Around line 44-49: The form created with useForm<EditForm> is only seeded once
from hostedDomain so if the dialog is reused for another hostedDomain it can
show stale values; update the component to reset or reinitialize the form
whenever hostedDomain changes (e.g., in a useEffect that calls the form reset
method or re-creates form state) so fields (domain, type, ssl_method, ssl_id)
are updated for the new hostedDomain; apply the same approach for the other
instance referenced around lines 73-74 to ensure no state leaks between dialog
sessions.
In `@resources/js/pages/scripts/components/form.tsx`:
- Around line 16-26: ScriptForm's useForm is only initialized on mount so
reopening or switching script leaves stale fields and errors; add a useEffect in
the ScriptForm component that watches [script, open] and calls form.setData({
name: script?.name ?? '', content: script?.content ?? '' }) (and clear
errors/state if needed) to rehydrate the form when the modal is opened or the
script prop changes, and make the Monaco Editor controlled by switching from
defaultValue to value={form.data.content} (or remount it by giving it a key tied
to script?.id) so the editor content stays in sync with form.data.content.
In `@resources/js/pages/server-features/components/feature-action.tsx`:
- Line 68: The Button currently binds submit via onClick which can trigger
duplicate submissions; remove the onClick={submit} from the Button that
references form="action-form" and instead set type="submit" so the form's
onSubmit handles submission; ensure the existing submit handler (submit) is
wired to the form's onSubmit (not the Button) and keep
disabled={form.processing} intact to prevent double requests.
In `@resources/js/pages/site-features/components/feature-action.tsx`:
- Around line 28-43: The form state persists between dialog opens because the
useForm instance (form) is never reset; update the submit flow and dialog open
handling in feature-action.tsx so the form is cleared between uses: call
form.reset() after a successful post (inside submit's onSuccess before/after
calling onOpenChange(false)) and also clear form when the dialog is opened (add
an effect or handler that calls form.reset() when the dialog's open state
becomes true) to ensure no stale values remain; reference the useForm hook, the
submit function, form.post, and onOpenChange to locate where to add the reset
calls.
---
Nitpick comments:
In `@resources/js/components/dialogs/log-viewer-dialog.tsx`:
- Around line 32-34: Replace the raw anchor used for internal navigation with
Inertia's Link: change the <a href={route('logs.download', { server: serverId,
log: logId })} ...> wrapper around the Button to a Link from Inertia (e.g.
import { Link } from '`@inertiajs/react`' or '`@inertiajs/inertia-react`' depending
on the project), keeping the same href (route('logs.download', { server:
serverId, log: logId })) and attributes (target="_blank" rel="noopener
noreferrer") and ensure the Link wraps the Button component so the internal
route uses the router API rather than a raw <a>.
In `@resources/js/pages/domains/components/columns.tsx`:
- Line 7: Remove the now-unused router import from `@inertiajs/react` in
resources/js/pages/domains/components/columns.tsx: locate the import line that
includes "router" and delete "router" (or remove the entire import if nothing
else is imported) so there are no unused imports after the dialog refactor.
In `@resources/js/pages/php/components/default-cli.tsx`:
- Around line 12-19: Add the missing variant key to the dialog.confirm.open
payload to comply with the confirm contract: update the call to
dialog.confirm.open (in default-cli.tsx) to include a variant property (e.g.,
variant: 'confirm' or 'destructive' as appropriate for this action) alongside
title, description, confirmLabel, method, url and data so the dialog behavior
matches the project's standardized confirm contract.
In `@resources/js/pages/plugins/components/install.tsx`:
- Around line 11-18: Add the missing variant property to the dialog.confirm.open
call to match the standardized confirm contract: update the payload passed to
dialog.confirm.open (the call that currently uses title, description,
confirmLabel, method, url, data with plugin.folder and plugin.id) to include
variant (e.g., 'default' or the appropriate variant used elsewhere) alongside
the other fields so the confirm dialog follows the required contract.
In `@resources/js/pages/projects/components/invitations.tsx`:
- Around line 23-26: Replace the direct window.location.href navigation in the
onSelect handler with Inertia's router.visit to preserve SPA routing: change the
handler to call
router.visit(`/settings/projects/${invitation.project_id}/invitations/accept`)
and ensure you import the router (e.g. import { router } from '`@inertiajs/react`'
or use the useRouter hook) at the top of the file so internal navigation for the
onSelect uses Inertia routing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d26bfa6-e8f2-4c2a-bb4e-fc8d53538b15
📒 Files selected for processing (101)
.github/instructions/frontend.instructions.mdCLAUDE.mdapp/Enums/DeploymentStatus.phpapp/Http/Controllers/ApplicationController.phpapp/Tables/DeploymentTable.phpapp/Tables/Servers/FirewallRuleTable.phpresources/js/components/dialogs/activate-server-ssl-dialog.tsxresources/js/components/dialogs/confirmation-dialog.tsxresources/js/components/dialogs/dialog-host.tsxresources/js/components/dialogs/log-viewer-dialog.tsxresources/js/components/dialogs/php-extensions-dialog.tsxresources/js/components/dialogs/php-ini-dialog.tsxresources/js/components/dialogs/plugin-logs-dialog.tsxresources/js/components/dialogs/registry.tsresources/js/components/dialogs/worker-logs-dialog.tsxresources/js/components/server-banners.tsxresources/js/hooks/use-dialog.tsresources/js/layouts/app/layout.tsxresources/js/pages/api-keys/components/columns.tsxresources/js/pages/application/components/app-with-deployment.tsxresources/js/pages/application/components/delete-deployment.tsxresources/js/pages/application/components/deployment-columns.tsxresources/js/pages/application/components/rollback.tsxresources/js/pages/backups/components/columns.tsxresources/js/pages/backups/components/edit-backup.tsxresources/js/pages/backups/components/file-columns.tsxresources/js/pages/backups/components/restore-backup.tsxresources/js/pages/commands/components/columns.tsxresources/js/pages/commands/components/edit-command.tsxresources/js/pages/cronjobs/components/columns.tsxresources/js/pages/cronjobs/components/form.tsxresources/js/pages/cronjobs/index.tsxresources/js/pages/database-users/components/columns.tsxresources/js/pages/database-users/components/edit-database-user.tsxresources/js/pages/database-users/components/link-dialog.tsxresources/js/pages/databases/components/delete.tsxresources/js/pages/dns-providers/components/delete.tsxresources/js/pages/dns-providers/components/edit-dialog.tsxresources/js/pages/dns-providers/components/edit.tsxresources/js/pages/domains/components/columns.tsxresources/js/pages/domains/components/record-columns.tsxresources/js/pages/domains/components/record-form.tsxresources/js/pages/domains/show.tsxresources/js/pages/firewall/components/delete.tsxresources/js/pages/firewall/components/form.tsxresources/js/pages/firewall/index.tsxresources/js/pages/hosted-domains/components/create-hosted-domain.tsxresources/js/pages/hosted-domains/components/edit-hosted-domain.tsxresources/js/pages/hosted-domains/index.tsxresources/js/pages/monitoring/components/actions.tsxresources/js/pages/monitoring/components/data-retention-dialog.tsxresources/js/pages/notification-channels/components/delete.tsxresources/js/pages/notification-channels/components/edit-dialog.tsxresources/js/pages/notification-channels/components/edit.tsxresources/js/pages/php/components/default-cli.tsxresources/js/pages/php/components/extensions.tsxresources/js/pages/php/components/ini.tsxresources/js/pages/plugins/components/delete-logs.tsxresources/js/pages/plugins/components/disable.tsxresources/js/pages/plugins/components/enable.tsxresources/js/pages/plugins/components/install.tsxresources/js/pages/plugins/components/installed.tsxresources/js/pages/plugins/components/uninstall.tsxresources/js/pages/plugins/components/update.tsxresources/js/pages/plugins/components/view-logs.tsxresources/js/pages/projects/components/invitations.tsxresources/js/pages/redirects/components/columns.tsxresources/js/pages/scripts/components/columns.tsxresources/js/pages/scripts/components/form.tsxresources/js/pages/scripts/index.tsxresources/js/pages/server-features/components/feature-action.tsxresources/js/pages/server-features/index.tsxresources/js/pages/server-logs/components/columns.tsxresources/js/pages/server-providers/components/delete.tsxresources/js/pages/server-providers/components/edit-dialog.tsxresources/js/pages/server-providers/components/edit.tsxresources/js/pages/server-ssh-keys/components/delete.tsxresources/js/pages/server-ssls/components/activate-server-ssl.tsxresources/js/pages/server-ssls/components/columns.tsxresources/js/pages/servers/components/actions.tsxresources/js/pages/servers/components/reboot-server.tsxresources/js/pages/servers/components/update-server.tsxresources/js/pages/services/components/action.tsxresources/js/pages/services/components/config-file-dialog.tsxresources/js/pages/services/components/config-file.tsxresources/js/pages/services/components/installation-log.tsxresources/js/pages/services/components/uninstall.tsxresources/js/pages/site-features/components/feature-action.tsxresources/js/pages/site-features/index.tsxresources/js/pages/sites/stats.tsxresources/js/pages/source-controls/components/columns.tsxresources/js/pages/source-controls/components/edit-dialog.tsxresources/js/pages/ssh-keys/components/delete.tsxresources/js/pages/storage-providers/components/delete.tsxresources/js/pages/storage-providers/components/edit-dialog.tsxresources/js/pages/storage-providers/components/edit.tsxresources/js/pages/workers/components/columns.tsxresources/js/pages/workers/components/form.tsxresources/js/pages/workers/components/worker-row-actions.tsxresources/js/pages/workers/index.tsxresources/js/stores/dialog-store.ts
💤 Files with no reviewable changes (5)
- resources/js/pages/application/components/delete-deployment.tsx
- resources/js/pages/servers/components/reboot-server.tsx
- resources/js/pages/application/components/rollback.tsx
- resources/js/pages/servers/components/update-server.tsx
- resources/js/pages/application/components/deployment-columns.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactors dialogs to ensure they render outside of the dropdown, resolves problems such as live updates changing tables and rebinding data, updating the wrong row, or streaming the wrong log file.
Summary by CodeRabbit
New Features
Improvements
Documentation