Download export should show loading indicator #3913
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExport dialog gains loading state and error signal; template shows a progress bar and disables Download/Cancel while loading. download() now sets loading, delays briefly, selects filtered or all entities, triggers download, closes on success, and logs/sets a localized error on failure. QueryService.toEntities rejects falsy ids. ChangesExport Dialog: loading, error, wiring
Query Service: id validation
Sequence DiagramsequenceDiagram
participant User
participant ExportDialog
participant QueryService
participant DownloadService
participant Logging
User->>ExportDialog: Click Download
ExportDialog->>ExportDialog: set isLoading = true
ExportDialog->>ExportDialog: await short delay
ExportDialog->>QueryService: toEntities(filter ids)
QueryService-->>ExportDialog: return resolved entities
ExportDialog->>DownloadService: triggerDownload(data)
alt success
DownloadService-->>ExportDialog: success
ExportDialog->>ExportDialog: close dialog
else error
DownloadService-->>ExportDialog: throw error
ExportDialog->>Logging: log warning
ExportDialog-->>ExportDialog: set downloadError(msg)
end
ExportDialog->>ExportDialog: set isLoading = false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Deployed to https://pr-3913.aam-digital.net/ |
Abhinegi2
added
the
Final Review
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
github-actions
Bot
removed
the
Final Review
![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)
src/app/core/export/export-dialog/export-dialog.component.html (1)
43-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDownload and Cancel buttons correctly disabled during loading — note: dialog-close
Xin the title header is unguarded.The
[disabled]="isLoading()"bindings on both action buttons are correct: disabled native<button>elements suppress click events, soMatDialogCloseon the Cancel button will not fire while loading.However, the
<app-dialog-close mat-dialog-close>in the<h1>title remains fully interactive whileisLoading()istrue. Clicking it mid-download closes the dialog (the background download still completes via the browser), but the subsequentthis.dialogRef.close()/alertService.addWarning(...)calls indownload()will execute against an already-closed dialog. This is a minor UX inconsistency relative to the stated goal of "disable buttons while the export is being processed."🤖 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 `@src/app/core/export/export-dialog/export-dialog.component.html` around lines 43 - 61, The title's close control (<app-dialog-close mat-dialog-close>) remains interactive during export; prevent closing while loading by wiring the same loading guard used on the action buttons: add a disabled binding or a click guard to the title close control (e.g., change <app-dialog-close mat-dialog-close> to include [disabled]="isLoading()" or replace it with a button that checks isLoading() in (click) and only calls dialogRef.close() when false); keep the existing download() and alertService/dialogRef calls unchanged but ensure the title control cannot close the dialog while isLoading() is true.
🧹 Nitpick comments (2)
src/app/core/export/export-dialog/export-dialog.component.ts (1)
78-82: ⚡ Quick winReplace
console.errorwith theLoggingservice.
console.errorbypasses the application's centralized logging infrastructure. The codebase already usesLogging.error(...)/Logging.warn(...)(seedownload.service.ts). As per coding guidelines for*.component.ts, useLoggingfrom#src/app/core/logging/logging.serviceinstead of anyconsole.*call.♻️ Proposed fix
+import { Logging } from "#src/app/core/logging/logging.service"; ... } catch (e) { - console.error("Export download failed:", e); + Logging.error("Export download failed:", e); this.alertService.addWarning(As per coding guidelines: "Use
Loggingservice from#src/app/core/logging/logging.serviceinstead ofconsole.log" (for**/*.component.ts).🤖 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 `@src/app/core/export/export-dialog/export-dialog.component.ts` around lines 78 - 82, In ExportDialogComponent's catch block replace the direct console.error call with the centralized Logging service: import Logging from `#src/app/core/logging/logging.service` and call Logging.error("Export download failed:", e) (or equivalent signature used elsewhere) then keep the existing this.alertService.addWarning(...) behavior; remove the console.error call so all errors go through Logging.error in the export download handler.src/app/core/export/export-dialog/export-dialog.component.html (1)
6-8: ⚡ Quick winProgress bar placement causes a layout shift on toggle.
Placing the
mat-progress-barconditionally between the title and the dialog content means the content area shifts down ~4 px when loading starts and snaps back up when it ends. Usingvisibilityinstead of@ifkeeps space reserved and avoids the reflow:💡 Proposed alternative to avoid layout shift
-@if (isLoading()) { - <mat-progress-bar mode="indeterminate"></mat-progress-bar> -} +<mat-progress-bar + [style.visibility]="isLoading() ? 'visible' : 'hidden'" + mode="indeterminate" +></mat-progress-bar>🤖 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 `@src/app/core/export/export-dialog/export-dialog.component.html` around lines 6 - 8, The progress bar is conditionally rendered with "@if (isLoading())" causing a layout shift; instead always render the <mat-progress-bar> in export-dialog.component.html and toggle its visibility (e.g., bind [style.visibility] or [hidden] to isLoading()) so space is reserved while it's invisible—locate the mat-progress-bar element near the dialog title and replace the conditional rendering that uses isLoading() with a visibility binding tied to the same isLoading() method.
🤖 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.
Outside diff comments:
In `@src/app/core/export/export-dialog/export-dialog.component.html`:
- Around line 43-61: The title's close control (<app-dialog-close
mat-dialog-close>) remains interactive during export; prevent closing while
loading by wiring the same loading guard used on the action buttons: add a
disabled binding or a click guard to the title close control (e.g., change
<app-dialog-close mat-dialog-close> to include [disabled]="isLoading()" or
replace it with a button that checks isLoading() in (click) and only calls
dialogRef.close() when false); keep the existing download() and
alertService/dialogRef calls unchanged but ensure the title control cannot close
the dialog while isLoading() is true.
---
Nitpick comments:
In `@src/app/core/export/export-dialog/export-dialog.component.html`:
- Around line 6-8: The progress bar is conditionally rendered with "@if
(isLoading())" causing a layout shift; instead always render the
<mat-progress-bar> in export-dialog.component.html and toggle its visibility
(e.g., bind [style.visibility] or [hidden] to isLoading()) so space is reserved
while it's invisible—locate the mat-progress-bar element near the dialog title
and replace the conditional rendering that uses isLoading() with a visibility
binding tied to the same isLoading() method.
In `@src/app/core/export/export-dialog/export-dialog.component.ts`:
- Around line 78-82: In ExportDialogComponent's catch block replace the direct
console.error call with the centralized Logging service: import Logging from
`#src/app/core/logging/logging.service` and call Logging.error("Export download
failed:", e) (or equivalent signature used elsewhere) then keep the existing
this.alertService.addWarning(...) behavior; remove the console.error call so all
errors go through Logging.error in the export download handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 988bae4f-c8e6-4282-b682-9ee5faaa9d70
📒 Files selected for processing (3)
src/app/core/export/export-dialog/export-dialog.component.htmlsrc/app/core/export/export-dialog/export-dialog.component.tssrc/app/core/export/query.service.ts
Abhinegi2
added
the
Final Review
github-actions
Bot
removed
the
Final Review
Abhinegi2
added
the
Final Review
github-actions
Bot
removed
the
Final Review
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/core/export/export-dialog/export-dialog.component.html (1)
54-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message is still displayed after (below) the buttons, not above them
The
downloadErrorblock at line 63 is rendered after the Cancel button insidemat-dialog-actions, meaning the error appears below the buttons. The previous review specifically requested the error be shown above the buttons for better visibility.Move the error display to just before the
mat-dialog-actionsblock (or at the bottom ofmat-dialog-content) so it's visible without scrolling past the action bar.🤖 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 `@src/app/core/export/export-dialog/export-dialog.component.html` around lines 54 - 65, The downloadError() mat-error block is currently placed inside mat-dialog-actions after the Cancel button, so move the error display out of the actions and place it immediately before the <mat-dialog-actions> block (or at the bottom of <mat-dialog-content>) so the error appears above the action buttons; update the template to render downloadError() (the mat-error element) above the Cancel button (which uses mat-dialog-close and [disabled]="isLoading()") and ensure binding remains {{ downloadError() }}.
🧹 Nitpick comments (1)
src/app/core/export/export-dialog/export-dialog.component.ts (1)
13-13: ⚡ Quick win
MatFormFieldModuleis only needed ifmat-erroris used correctly inside amat-form-fieldThe sole reason this module is imported is to support
<mat-error>in the template, but<mat-error>is designed exclusively to be a child of<mat-form-field>. Once the template-side issue (see below) is fixed by replacing<mat-error>with an inline error span, this import becomes unused and should be removed.🤖 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 `@src/app/core/export/export-dialog/export-dialog.component.ts` at line 13, The MatFormFieldModule import is now unused because <mat-error> will be replaced with an inline error span; remove the import of MatFormFieldModule and any references to it in this file (e.g., any NgModule or component-level imports that list MatFormFieldModule) so there are no unused Angular Material module imports left (target the MatFormFieldModule symbol and any occurrences in the file).
🤖 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 `@src/app/core/export/export-dialog/export-dialog.component.html`:
- Around line 63-65: The download error display should be moved out of the
dialog actions and avoid using <mat-error> outside a <mat-form-field>: relocate
the conditional error UI from inside mat-dialog-actions to the top of
mat-dialog-content (above buttons), replace the current `@if`(downloadError())
block with an Angular structural conditional (e.g. *ngIf="downloadError()") and
render a plain element (e.g. <div> or <p>) with role="alert" and a class that
applies the Material error color (or color="warn" via component styles) instead
of <mat-error>; update the template where downloadError() is referenced and add
a CSS rule in the component stylesheet to style the error text for accessibility
and visual parity.
In `@src/app/core/export/export-dialog/export-dialog.component.ts`:
- Around line 83-85: The template literal passes an expression with type unknown
(caught error `e`) which violates strict template-expression rules; update the
call to this.downloadError.set(...) to convert `e` to a string safely (e.g., use
the existing `e instanceof Error ? e.message : String(e)` or check typeof e ===
'string' ? e : String(e)) so the value inside the template literal is strictly a
string; locate the `downloadError.set` invocation in export-dialog.component.ts
and replace the ternary expression with a string-coercing alternative to satisfy
type-checking.
---
Duplicate comments:
In `@src/app/core/export/export-dialog/export-dialog.component.html`:
- Around line 54-65: The downloadError() mat-error block is currently placed
inside mat-dialog-actions after the Cancel button, so move the error display out
of the actions and place it immediately before the <mat-dialog-actions> block
(or at the bottom of <mat-dialog-content>) so the error appears above the action
buttons; update the template to render downloadError() (the mat-error element)
above the Cancel button (which uses mat-dialog-close and
[disabled]="isLoading()") and ensure binding remains {{ downloadError() }}.
---
Nitpick comments:
In `@src/app/core/export/export-dialog/export-dialog.component.ts`:
- Line 13: The MatFormFieldModule import is now unused because <mat-error> will
be replaced with an inline error span; remove the import of MatFormFieldModule
and any references to it in this file (e.g., any NgModule or component-level
imports that list MatFormFieldModule) so there are no unused Angular Material
module imports left (target the MatFormFieldModule symbol and any occurrences in
the file).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebcdd9ab-05bb-444b-8334-9f79658a3d52
📒 Files selected for processing (2)
src/app/core/export/export-dialog/export-dialog.component.htmlsrc/app/core/export/export-dialog/export-dialog.component.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Abhinegi2
added
the
Final Review
github-actions
Bot
removed
the
Final Review
sleidig
left a comment
sleidig
left a comment
There was a problem hiding this comment.
Not very pretty error message 😊 but otherwise good. Thanks!
|
🎉 This PR is included in version 3.79.1-master.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
4 participants
PR Checklist
Before you request a manual PR review from someone, please go through all of this list, check the points and mark them checked here.
This helps us reduce the number of review cycles and use the time of our core reviewers more effectively.
ng lint) passesng test) passes⏪ if issues are commented from testing or code review, make changes. Please then re-check every item of the checklist here again.
Remaining TODOs:
Summary by CodeRabbit
New Features
Bug Fixes