Merged
Conversation
📝 WalkthroughWalkthroughStandardizes form submission handling across Material dialogs by wrapping content in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes Angular Material dialog markup so dialogs containing form-like inputs submit via <form (ngSubmit)> with a type="submit" primary action, improving keyboard (Enter key) submission behavior and consistency across the frontend.
Changes:
- Wrapped multiple
mat-dialog-content/mat-dialog-actionsblocks in<form (ngSubmit)=...>and converted primary action buttons totype="submit". - Added missing
nameattributes tongModel-bound inputs for template-driven forms. - Documented the required dialog form pattern in
frontend/CLAUDE.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/components/user-settings/account-delete-dialog/account-delete-dialog.component.html | Wraps dialog content/actions in a form and submits via ngSubmit. |
| frontend/src/app/components/secrets/master-password-dialog/master-password-dialog.component.html | Adds form submission flow and ensures ngModel has a name. |
| frontend/src/app/components/hosted-databases/hosted-database-rename-dialog/hosted-database-rename-dialog.component.html | Converts rename dialog to standard form submit pattern. |
| frontend/src/app/components/dashboard/db-tables-list/db-folder-edit-dialog/db-folder-edit-dialog.component.html | Enables Enter-to-save via form submit and fixes button types inside form. |
| frontend/src/app/components/dashboard/db-table-view/db-table-export-dialog/db-table-export-dialog.component.html | Makes Export a real form submit and prevents Cancel from submitting. |
| frontend/src/app/components/connections-list/hosted-database-rename-dialog/hosted-database-rename-dialog.component.html | Converts save action to submit and adds name for ngModel. |
| frontend/src/app/components/connect-db/db-connection-delete-dialog/db-connection-delete-dialog.component.html | Wraps dialog in form and submits delete via ngSubmit. |
| frontend/src/app/components/company/invite-member-dialog/invite-member-dialog.component.html | Makes primary action explicitly type="submit" for the existing form. |
| frontend/CLAUDE.md | Adds a documented convention for mat-dialog forms and submit buttons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> | ||
| <button mat-flat-button color="warn" | ||
| (click)="openDeleteConfirmation()"> | ||
| <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> |
There was a problem hiding this comment.
mat-dialog-close is specified twice on this button (mat-dialog-close mat-dialog-close="cancel"). Duplicate attributes are invalid HTML and can lead to confusing behavior; keep a single close binding (e.g., only mat-dialog-close="cancel" or a single [mat-dialog-close] binding).
Suggested change
| <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> | |
| <button type="button" mat-flat-button mat-dialog-close="cancel">Abort</button> |
| <button mat-flat-button color="warn" | ||
| [disabled]="submitting" | ||
| (click)="deleteConnection()"> | ||
| <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> |
There was a problem hiding this comment.
mat-dialog-close is duplicated on this button (mat-dialog-close mat-dialog-close="cancel"). Please remove the redundant attribute and keep a single close value/binding to avoid invalid markup.
Suggested change
| <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> | |
| <button type="button" mat-flat-button mat-dialog-close="cancel">Abort</button> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/src/app/components/user-settings/account-delete-dialog/account-delete-dialog.component.html (1)
12-20: Use built-in control flow instead of structural directives.The changed lines use
*ngForand*ngIf, but the coding guidelines require using built-in control flow (@for,@if) in all new code.♻️ Suggested refactor using built-in control flow
- <div *ngFor="let reasonItem of reasonsList"> - <mat-radio-button [value]="reasonItem.id" class="radio-button"> - {{ reasonItem.caption }} - </mat-radio-button> - <p *ngIf="reasonItem.message && reason === reasonItem.id" - [innerHtml]="reasonItem.message" - class="mat-body-1 radio-group__message"> - </p> - </div> + `@for` (reasonItem of reasonsList; track reasonItem.id) { + <div> + <mat-radio-button [value]="reasonItem.id" class="radio-button"> + {{ reasonItem.caption }} + </mat-radio-button> + `@if` (reasonItem.message && reason === reasonItem.id) { + <p [innerHtml]="reasonItem.message" + class="mat-body-1 radio-group__message"> + </p> + } + </div> + }As per coding guidelines: "Use built-in control flow (
@if,@for,@switch) instead of structural directives (*ngIf,*ngFor,*ngSwitch) in all new code"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/user-settings/account-delete-dialog/account-delete-dialog.component.html` around lines 12 - 20, Replace the structural directives with Angular built-in control flow syntax: change the loop over reasonsList from "*ngFor" to "@for" (e.g., `@for`="let reasonItem of reasonsList") and change the conditional "*ngIf" on the message paragraph to "@if" (e.g., `@if`="reasonItem.message && reason === reasonItem.id"), keeping the existing bindings ([value]="reasonItem.id", {{ reasonItem.caption }}, [innerHtml]="reasonItem.message") and CSS classes (radio-button, radio-group__message) on the same elements (mat-radio-button and the paragraph) so the markup and behavior of reasonsList, reasonItem and reason remain identical while using `@for/`@if.frontend/src/app/components/connect-db/db-connection-delete-dialog/db-connection-delete-dialog.component.html (1)
9-12: Consider enforcing form validation on submit.The radio group has
requiredbut the submit button is only disabled by[disabled]="submitting". Therequiredconstraint won't prevent submission since the form's validity state isn't checked. If therequiredis intentional, consider adding form validation:Optional: Disable submit when form is invalid
<button type="submit" mat-flat-button color="warn" - [disabled]="submitting"> + [disabled]="submitting || !deleteConnectionForm.valid"> Proceed </button>Also applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/connect-db/db-connection-delete-dialog/db-connection-delete-dialog.component.html` around lines 9 - 12, The radio group uses required but the submit button only checks [disabled]="submitting", so the form can submit with no selection; update the template to use an Angular form (e.g., add `#deleteForm`="ngForm" on the form element and bind (ngSubmit) to the existing submit handler) and change the submit button's disabled binding to also check form validity (e.g., [disabled]="submitting || deleteForm.invalid"). Also update the component's submit method (onDelete/onSubmit) to guard on the form or the reason model (reason) and return early if the form is invalid to enforce server-side safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/app/components/connect-db/db-connection-delete-dialog/db-connection-delete-dialog.component.html`:
- Line 34: In the DbConnectionDeleteDialogComponent template, remove the
duplicate mat-dialog-close attribute on the Abort button so only the
value-bearing attribute remains; specifically, delete the bare mat-dialog-close
and keep mat-dialog-close="cancel" on the <button> element to avoid duplicate
attributes.
In
`@frontend/src/app/components/user-settings/account-delete-dialog/account-delete-dialog.component.html`:
- Line 30: The Abort button element in account-delete-dialog.component.html
incorrectly includes the mat-dialog-close attribute twice; remove the duplicate
plain mat-dialog-close attribute and keep the value form
mat-dialog-close="cancel" on the <button> so the dialog close payload is
explicit (locate the Abort button element in
account-delete-dialog.component.html and delete the redundant mat-dialog-close
occurrence).
---
Nitpick comments:
In
`@frontend/src/app/components/connect-db/db-connection-delete-dialog/db-connection-delete-dialog.component.html`:
- Around line 9-12: The radio group uses required but the submit button only
checks [disabled]="submitting", so the form can submit with no selection; update
the template to use an Angular form (e.g., add `#deleteForm`="ngForm" on the form
element and bind (ngSubmit) to the existing submit handler) and change the
submit button's disabled binding to also check form validity (e.g.,
[disabled]="submitting || deleteForm.invalid"). Also update the component's
submit method (onDelete/onSubmit) to guard on the form or the reason model
(reason) and return early if the form is invalid to enforce server-side safety.
In
`@frontend/src/app/components/user-settings/account-delete-dialog/account-delete-dialog.component.html`:
- Around line 12-20: Replace the structural directives with Angular built-in
control flow syntax: change the loop over reasonsList from "*ngFor" to "@for"
(e.g., `@for`="let reasonItem of reasonsList") and change the conditional "*ngIf"
on the message paragraph to "@if" (e.g., `@if`="reasonItem.message && reason ===
reasonItem.id"), keeping the existing bindings ([value]="reasonItem.id", {{
reasonItem.caption }}, [innerHtml]="reasonItem.message") and CSS classes
(radio-button, radio-group__message) on the same elements (mat-radio-button and
the paragraph) so the markup and behavior of reasonsList, reasonItem and reason
remain identical while using `@for/`@if.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e3eb7b0-3397-4261-ba77-137ae65fca58
📒 Files selected for processing (9)
frontend/CLAUDE.mdfrontend/src/app/components/company/invite-member-dialog/invite-member-dialog.component.htmlfrontend/src/app/components/connect-db/db-connection-delete-dialog/db-connection-delete-dialog.component.htmlfrontend/src/app/components/connections-list/hosted-database-rename-dialog/hosted-database-rename-dialog.component.htmlfrontend/src/app/components/dashboard/db-table-view/db-table-export-dialog/db-table-export-dialog.component.htmlfrontend/src/app/components/dashboard/db-tables-list/db-folder-edit-dialog/db-folder-edit-dialog.component.htmlfrontend/src/app/components/hosted-databases/hosted-database-rename-dialog/hosted-database-rename-dialog.component.htmlfrontend/src/app/components/secrets/master-password-dialog/master-password-dialog.component.htmlfrontend/src/app/components/user-settings/account-delete-dialog/account-delete-dialog.component.html
| <button mat-flat-button color="warn" | ||
| [disabled]="submitting" | ||
| (click)="deleteConnection()"> | ||
| <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> |
There was a problem hiding this comment.
Duplicate mat-dialog-close attributes.
The button has both mat-dialog-close (without value) and mat-dialog-close="cancel". Only the second one is needed.
Proposed fix
- <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button>
+ <button type="button" mat-flat-button mat-dialog-close="cancel">Abort</button>📝 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.
Suggested change
| <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> | |
| <button type="button" mat-flat-button mat-dialog-close="cancel">Abort</button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/connect-db/db-connection-delete-dialog/db-connection-delete-dialog.component.html`
at line 34, In the DbConnectionDeleteDialogComponent template, remove the
duplicate mat-dialog-close attribute on the Abort button so only the
value-bearing attribute remains; specifically, delete the bare mat-dialog-close
and keep mat-dialog-close="cancel" on the <button> element to avoid duplicate
attributes.
| <button mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> | ||
| <button mat-flat-button color="warn" | ||
| (click)="openDeleteConfirmation()"> | ||
| <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> |
There was a problem hiding this comment.
Duplicate mat-dialog-close attribute.
The Abort button has mat-dialog-close specified twice. The second instance with the value will override the first, but this is likely unintentional and should be cleaned up.
🔧 Proposed fix
- <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button>
+ <button type="button" mat-flat-button mat-dialog-close="cancel">Abort</button>📝 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.
Suggested change
| <button type="button" mat-flat-button mat-dialog-close mat-dialog-close="cancel">Abort</button> | |
| <button type="button" mat-flat-button mat-dialog-close="cancel">Abort</button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/user-settings/account-delete-dialog/account-delete-dialog.component.html`
at line 30, The Abort button element in account-delete-dialog.component.html
incorrectly includes the mat-dialog-close attribute twice; remove the duplicate
plain mat-dialog-close attribute and keep the value form
mat-dialog-close="cancel" on the <button> so the dialog close payload is
explicit (locate the Abort button element in
account-delete-dialog.component.html and delete the redundant mat-dialog-close
occurrence).
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.
Summary by CodeRabbit
Documentation
Refactor