feat: resume QWC setup#1766
feat: resume QWC setup#1766JustARatherRidiculouslyLongUsername wants to merge 1 commit intoqwc-regen-existing-pathfrom
Conversation
If the user leaves mid-setup, redirect them back to the same step once they return. - we check onboarding state and load the appropriate section to resume where the flow was left off - however, the same onboarding states are used mid-onboarding also - check if advanced settings exists to decide whether to redirect to onboarding to qwc regen
WalkthroughThis pull request introduces a QWC flow state persistence mechanism enabling users to resume from their last visited regeneration step. It adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-qwc-last-visited-flow.service.ts (1)
14-20: Consider adding aclear()method for cleanup.When the QWC regeneration flow completes successfully, you may want to clear the stored last-visited flow to avoid stale state on subsequent regeneration attempts.
♻️ Suggested addition
set(flow: QwcRegenerationFlowType): void { this.storageService.set('qwc-last-visited-flow', flow); } + + clear(): void { + this.storageService.remove('qwc-last-visited-flow'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-qwc-last-visited-flow.service.ts` around lines 14 - 20, The service currently exposes get() and set(flow: QwcRegenerationFlowType) but lacks a clear() method to remove stale state; add a public clear(): void method on the same service (the class containing get and set) that calls this.storageService.remove('qwc-last-visited-flow') (or the appropriate delete/remove API on storageService) and use clear() after a successful QWC regeneration to avoid persisting the last-visited flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-qwc-file/qbd-direct-regenerate-qwc-file/qbd-direct-regenerate-qwc-file.component.ts`:
- Around line 327-333: The workspace fetch in ngOnInit sets this.isLoading =
true and calls this.workspaceService.getWorkspace(user.org_id).subscribe(...)
but has no error handling, so if the API fails isLoading stays true; update the
call to handle errors by adding an error path (either a second subscribe
callback or pipe + catchError) on workspaceService.getWorkspace(user.org_id) to
set this.isLoading = false, surface an appropriate user-facing message or toast,
and avoid calling this.navigate when the fetch fails; reference the symbols
this.isLoading, storageService.get('user'), workspaceService.getWorkspace, and
this.navigate to locate and update the code.
- Around line 317-322: Destructuring goToPrerequisites from history.state with a
direct `as QwcRouteState` can be misleading and risky if history.state is
null/undefined; update the logic to defensively read history.state (e.g., guard
or default to an empty object) before extracting goToPrerequisites, remove the
unsafe assertion, and then keep the existing check that calls
this.state.set(QwcFlowState.PREREQUISITES) when goToPrerequisites is truthy so
the code safely handles direct navigation or refresh cases.
In `@src/app/integrations/qbd-direct/qbd-direct.component.ts`:
- Around line 83-96: The onboardingStateComponentMap lookup can return undefined
for this.workspace.onboarding_state and then pass that to
this.router.navigateByUrl; add a defensive guard or fallback before calling
navigateByUrl: compute the target via
onboardingStateComponentMap[this.workspace.onboarding_state], verify it is a
non-empty string (or select a default route like
'/integrations/qbd_direct/onboarding/landing' or
'/integrations/qbd_direct/main'), and only call
this.router.navigateByUrl(target) when target is valid (otherwise handle the
unexpected state via a log/warning and navigate to the safe default).
- Around line 66-79: The error handler for
qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe is treating
every failure as a 404; change the error callback to inspect the error status
(e.g., HttpErrorResponse.status) and only navigate to
'/integrations/qbd_direct/onboarding/connector' when status === 404, otherwise
handle non-404 errors separately (log or surface a user error state) and ensure
isLoading is set to false in all cases; keep references to
qbdDirectAdvancedSettingsService.getQbdAdvancedSettings(),
qbdDirectQwcLastVisitedFlowService.get(), router.navigateByUrl(...) and
isLoading when implementing the change.
---
Nitpick comments:
In
`@src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-qwc-last-visited-flow.service.ts`:
- Around line 14-20: The service currently exposes get() and set(flow:
QwcRegenerationFlowType) but lacks a clear() method to remove stale state; add a
public clear(): void method on the same service (the class containing get and
set) that calls this.storageService.remove('qwc-last-visited-flow') (or the
appropriate delete/remove API on storageService) and use clear() after a
successful QWC regeneration to avoid persisting the last-visited flow.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-qwc-file.model.tssrc/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-qwc-last-visited-flow.service.tssrc/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-qwc-file/qbd-direct-qwc-file-landing/qbd-direct-qwc-file-landing.component.tssrc/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-qwc-file/qbd-direct-regenerate-qwc-file/qbd-direct-regenerate-qwc-file.component.htmlsrc/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-qwc-file/qbd-direct-regenerate-qwc-file/qbd-direct-regenerate-qwc-file.component.tssrc/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.htmlsrc/app/integrations/qbd-direct/qbd-direct.component.ts
|
|
||
| const { goToPrerequisites } = history.state as QwcRouteState; | ||
| if (goToPrerequisites) { | ||
| this.state.set(QwcFlowState.PREREQUISITES); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Potential runtime error when history.state is null or undefined.
Destructuring goToPrerequisites from history.state assumes the state object exists. If the user navigates directly to this route (e.g., via URL or refresh), history.state may be null or an empty object, causing goToPrerequisites to be undefined.
The current logic handles undefined correctly (falsy check), but the type assertion as QwcRouteState is misleading. Consider adding a null check for clarity.
🛡️ Proposed defensive handling
- const { goToPrerequisites } = history.state as QwcRouteState;
- if (goToPrerequisites) {
+ const routeState = history.state as QwcRouteState | null;
+ if (routeState?.goToPrerequisites) {
this.state.set(QwcFlowState.PREREQUISITES);
return;
}📝 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 { goToPrerequisites } = history.state as QwcRouteState; | |
| if (goToPrerequisites) { | |
| this.state.set(QwcFlowState.PREREQUISITES); | |
| return; | |
| } | |
| const routeState = history.state as QwcRouteState | null; | |
| if (routeState?.goToPrerequisites) { | |
| this.state.set(QwcFlowState.PREREQUISITES); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-qwc-file/qbd-direct-regenerate-qwc-file/qbd-direct-regenerate-qwc-file.component.ts`
around lines 317 - 322, Destructuring goToPrerequisites from history.state with
a direct `as QwcRouteState` can be misleading and risky if history.state is
null/undefined; update the logic to defensively read history.state (e.g., guard
or default to an empty object) before extracting goToPrerequisites, remove the
unsafe assertion, and then keep the existing check that calls
this.state.set(QwcFlowState.PREREQUISITES) when goToPrerequisites is truthy so
the code safely handles direct navigation or refresh cases.
| this.isLoading = true; | ||
| const user: MinimalUser = this.storageService.get('user'); | ||
| this.workspaceService.getWorkspace(user.org_id).subscribe((workspaces: QbdDirectWorkspace[]) => { | ||
| this.navigate(workspaces[0].onboarding_state).subscribe(() => { | ||
| this.isLoading = false; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing error handling for workspace fetch during resume flow.
The getWorkspace call in ngOnInit does not handle errors. If the API fails, isLoading will remain true indefinitely, leaving the user stuck on the loading screen.
🛡️ Proposed error handling
this.isLoading = true;
const user: MinimalUser = this.storageService.get('user');
this.workspaceService.getWorkspace(user.org_id).subscribe((workspaces: QbdDirectWorkspace[]) => {
this.navigate(workspaces[0].onboarding_state).subscribe(() => {
this.isLoading = false;
});
+ }, (error) => {
+ console.error('Error fetching workspace:', error);
+ this.toastService.displayToastMessage(ToastSeverity.ERROR, this.translocoService.translate('qbdDirectRegenerateQwcFile.somethingWentWrong'));
+ this.isLoading = false;
});📝 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.
| this.isLoading = true; | |
| const user: MinimalUser = this.storageService.get('user'); | |
| this.workspaceService.getWorkspace(user.org_id).subscribe((workspaces: QbdDirectWorkspace[]) => { | |
| this.navigate(workspaces[0].onboarding_state).subscribe(() => { | |
| this.isLoading = false; | |
| }); | |
| }); | |
| this.isLoading = true; | |
| const user: MinimalUser = this.storageService.get('user'); | |
| this.workspaceService.getWorkspace(user.org_id).subscribe((workspaces: QbdDirectWorkspace[]) => { | |
| this.navigate(workspaces[0].onboarding_state).subscribe(() => { | |
| this.isLoading = false; | |
| }); | |
| }, (error) => { | |
| console.error('Error fetching workspace:', error); | |
| this.toastService.displayToastMessage(ToastSeverity.ERROR, this.translocoService.translate('qbdDirectRegenerateQwcFile.somethingWentWrong')); | |
| this.isLoading = false; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-qwc-file/qbd-direct-regenerate-qwc-file/qbd-direct-regenerate-qwc-file.component.ts`
around lines 327 - 333, The workspace fetch in ngOnInit sets this.isLoading =
true and calls this.workspaceService.getWorkspace(user.org_id).subscribe(...)
but has no error handling, so if the API fails isLoading stays true; update the
call to handle errors by adding an error path (either a second subscribe
callback or pipe + catchError) on workspaceService.getWorkspace(user.org_id) to
set this.isLoading = false, surface an appropriate user-facing message or toast,
and avoid calling this.navigate when the fetch fails; reference the symbols
this.isLoading, storageService.get('user'), workspaceService.getWorkspace, and
this.navigate to locate and update the code.
| this.qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe({ | ||
| next: () => { | ||
| // 200: advanced settings exists - we are regenerating the QWC file | ||
| const lastVisitedFlow = this.qbdDirectQwcLastVisitedFlowService.get(); | ||
| const flowRoute = lastVisitedFlow === QwcRegenerationFlowType.EXISTING ? 'existing' : 'new'; | ||
| this.router.navigateByUrl(`/integrations/qbd_direct/main/configuration/qwc_file/${flowRoute}`); | ||
| this.isLoading = false; | ||
| }, | ||
| error: () => { | ||
| // 404: advanced settings does not exist - we are onboarding | ||
| this.router.navigateByUrl('/integrations/qbd_direct/onboarding/connector'); | ||
| this.isLoading = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Error handling assumes all errors indicate missing advanced settings.
The error callback treats any error as a 404 (advanced settings not found), but network errors, server errors, or other issues would also redirect to onboarding. Consider checking the error status code explicitly.
🛡️ Proposed fix to check error status
error: () => {
- // 404: advanced settings does not exist - we are onboarding
- this.router.navigateByUrl('/integrations/qbd_direct/onboarding/connector');
- this.isLoading = false;
+ error: (error) => {
+ if (error?.status === 404) {
+ // 404: advanced settings does not exist - we are onboarding
+ this.router.navigateByUrl('/integrations/qbd_direct/onboarding/connector');
+ } else {
+ // Unexpected error - fallback to onboarding but log the issue
+ console.error('Error fetching advanced settings:', error);
+ this.router.navigateByUrl('/integrations/qbd_direct/onboarding/connector');
+ }
+ this.isLoading = false;
+ }
}📝 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.
| this.qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe({ | |
| next: () => { | |
| // 200: advanced settings exists - we are regenerating the QWC file | |
| const lastVisitedFlow = this.qbdDirectQwcLastVisitedFlowService.get(); | |
| const flowRoute = lastVisitedFlow === QwcRegenerationFlowType.EXISTING ? 'existing' : 'new'; | |
| this.router.navigateByUrl(`/integrations/qbd_direct/main/configuration/qwc_file/${flowRoute}`); | |
| this.isLoading = false; | |
| }, | |
| error: () => { | |
| // 404: advanced settings does not exist - we are onboarding | |
| this.router.navigateByUrl('/integrations/qbd_direct/onboarding/connector'); | |
| this.isLoading = false; | |
| } | |
| }); | |
| this.qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe({ | |
| next: () => { | |
| // 200: advanced settings exists - we are regenerating the QWC file | |
| const lastVisitedFlow = this.qbdDirectQwcLastVisitedFlowService.get(); | |
| const flowRoute = lastVisitedFlow === QwcRegenerationFlowType.EXISTING ? 'existing' : 'new'; | |
| this.router.navigateByUrl(`/integrations/qbd_direct/main/configuration/qwc_file/${flowRoute}`); | |
| this.isLoading = false; | |
| }, | |
| error: (error) => { | |
| if (error?.status === 404) { | |
| // 404: advanced settings does not exist - we are onboarding | |
| this.router.navigateByUrl('/integrations/qbd_direct/onboarding/connector'); | |
| } else { | |
| // Unexpected error - fallback to onboarding but log the issue | |
| console.error('Error fetching advanced settings:', error); | |
| this.router.navigateByUrl('/integrations/qbd_direct/onboarding/connector'); | |
| } | |
| this.isLoading = false; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/integrations/qbd-direct/qbd-direct.component.ts` around lines 66 -
79, The error handler for
qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe is treating
every failure as a 404; change the error callback to inspect the error status
(e.g., HttpErrorResponse.status) and only navigate to
'/integrations/qbd_direct/onboarding/connector' when status === 404, otherwise
handle non-404 errors separately (log or surface a user error state) and ensure
isLoading is set to false in all cases; keep references to
qbdDirectAdvancedSettingsService.getQbdAdvancedSettings(),
qbdDirectQwcLastVisitedFlowService.get(), router.navigateByUrl(...) and
isLoading when implementing the change.
| if (pathName === '/integrations/qbd_direct') { | ||
| const onboardingStateComponentMap: Record<QbdDirectOnboardingState, string> = { | ||
| const onboardingStateComponentMap: Partial<Record<QbdDirectOnboardingState, string>> = { | ||
| [QbdDirectOnboardingState.YET_TO_START]: '/integrations/qbd_direct/onboarding/landing', | ||
| [QbdDirectOnboardingState.CONFIRM_PRE_REQUISITES]: '/integrations/qbd_direct/onboarding/pre_requisite', | ||
| [QbdDirectOnboardingState.CONNECTION]: '/integrations/qbd_direct/onboarding/connector', | ||
| [QbdDirectOnboardingState.PENDING_QWC_UPLOAD]: '/integrations/qbd_direct/onboarding/connector', | ||
| [QbdDirectOnboardingState.INCORRECT_COMPANY_PATH]: '/integrations/qbd_direct/onboarding/connector', | ||
| [QbdDirectOnboardingState.INCORRECT_PASSWORD]: '/integrations/qbd_direct/onboarding/connector', | ||
| [QbdDirectOnboardingState.DESTINATION_SYNC_IN_PROGRESS]: '/integrations/qbd_direct/onboarding/connector', | ||
| [QbdDirectOnboardingState.DESTINATION_SYNC_COMPLETE]: '/integrations/qbd_direct/onboarding/connector', | ||
| [QbdDirectOnboardingState.EXPORT_SETTINGS]: '/integrations/qbd_direct/onboarding/export_settings', | ||
| [QbdDirectOnboardingState.IMPORT_SETTINGS]: '/integrations/qbd_direct/onboarding/import_settings', | ||
| [QbdDirectOnboardingState.ADVANCED_SETTINGS]: '/integrations/qbd_direct/onboarding/advanced_settings', | ||
| [QbdDirectOnboardingState.COMPANY_NAME_MISMATCH]: '/integrations/qbd_direct/main', | ||
| [QbdDirectOnboardingState.COMPLETE]: '/integrations/qbd_direct/main' | ||
| }; | ||
| this.router.navigateByUrl(onboardingStateComponentMap[this.workspace.onboarding_state]); | ||
| this.router.navigateByUrl( | ||
| onboardingStateComponentMap[this.workspace.onboarding_state] as string | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential undefined route when state is not in the map.
The onboardingStateComponentMap is now a Partial<Record>, so if this.workspace.onboarding_state is not in the map (e.g., one of the connection states that should have been handled earlier), the cast as string will result in undefined as string, causing navigateByUrl to receive undefined.
While the early return at line 80 should prevent this, consider adding a fallback or guard for robustness.
🛡️ Proposed defensive check
- this.router.navigateByUrl(
- onboardingStateComponentMap[this.workspace.onboarding_state] as string
- );
+ const route = onboardingStateComponentMap[this.workspace.onboarding_state];
+ if (route) {
+ this.router.navigateByUrl(route);
+ } else {
+ console.warn('Unexpected onboarding state:', this.workspace.onboarding_state);
+ this.router.navigateByUrl('/integrations/qbd_direct/onboarding/landing');
+ }📝 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.
| if (pathName === '/integrations/qbd_direct') { | |
| const onboardingStateComponentMap: Record<QbdDirectOnboardingState, string> = { | |
| const onboardingStateComponentMap: Partial<Record<QbdDirectOnboardingState, string>> = { | |
| [QbdDirectOnboardingState.YET_TO_START]: '/integrations/qbd_direct/onboarding/landing', | |
| [QbdDirectOnboardingState.CONFIRM_PRE_REQUISITES]: '/integrations/qbd_direct/onboarding/pre_requisite', | |
| [QbdDirectOnboardingState.CONNECTION]: '/integrations/qbd_direct/onboarding/connector', | |
| [QbdDirectOnboardingState.PENDING_QWC_UPLOAD]: '/integrations/qbd_direct/onboarding/connector', | |
| [QbdDirectOnboardingState.INCORRECT_COMPANY_PATH]: '/integrations/qbd_direct/onboarding/connector', | |
| [QbdDirectOnboardingState.INCORRECT_PASSWORD]: '/integrations/qbd_direct/onboarding/connector', | |
| [QbdDirectOnboardingState.DESTINATION_SYNC_IN_PROGRESS]: '/integrations/qbd_direct/onboarding/connector', | |
| [QbdDirectOnboardingState.DESTINATION_SYNC_COMPLETE]: '/integrations/qbd_direct/onboarding/connector', | |
| [QbdDirectOnboardingState.EXPORT_SETTINGS]: '/integrations/qbd_direct/onboarding/export_settings', | |
| [QbdDirectOnboardingState.IMPORT_SETTINGS]: '/integrations/qbd_direct/onboarding/import_settings', | |
| [QbdDirectOnboardingState.ADVANCED_SETTINGS]: '/integrations/qbd_direct/onboarding/advanced_settings', | |
| [QbdDirectOnboardingState.COMPANY_NAME_MISMATCH]: '/integrations/qbd_direct/main', | |
| [QbdDirectOnboardingState.COMPLETE]: '/integrations/qbd_direct/main' | |
| }; | |
| this.router.navigateByUrl(onboardingStateComponentMap[this.workspace.onboarding_state]); | |
| this.router.navigateByUrl( | |
| onboardingStateComponentMap[this.workspace.onboarding_state] as string | |
| ); | |
| } | |
| if (pathName === '/integrations/qbd_direct') { | |
| const onboardingStateComponentMap: Partial<Record<QbdDirectOnboardingState, string>> = { | |
| [QbdDirectOnboardingState.YET_TO_START]: '/integrations/qbd_direct/onboarding/landing', | |
| [QbdDirectOnboardingState.CONFIRM_PRE_REQUISITES]: '/integrations/qbd_direct/onboarding/pre_requisite', | |
| [QbdDirectOnboardingState.CONNECTION]: '/integrations/qbd_direct/onboarding/connector', | |
| [QbdDirectOnboardingState.EXPORT_SETTINGS]: '/integrations/qbd_direct/onboarding/export_settings', | |
| [QbdDirectOnboardingState.IMPORT_SETTINGS]: '/integrations/qbd_direct/onboarding/import_settings', | |
| [QbdDirectOnboardingState.ADVANCED_SETTINGS]: '/integrations/qbd_direct/onboarding/advanced_settings', | |
| [QbdDirectOnboardingState.COMPLETE]: '/integrations/qbd_direct/main' | |
| }; | |
| const route = onboardingStateComponentMap[this.workspace.onboarding_state]; | |
| if (route) { | |
| this.router.navigateByUrl(route); | |
| } else { | |
| console.warn('Unexpected onboarding state:', this.workspace.onboarding_state); | |
| this.router.navigateByUrl('/integrations/qbd_direct/onboarding/landing'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/integrations/qbd-direct/qbd-direct.component.ts` around lines 83 -
96, The onboardingStateComponentMap lookup can return undefined for
this.workspace.onboarding_state and then pass that to this.router.navigateByUrl;
add a defensive guard or fallback before calling navigateByUrl: compute the
target via onboardingStateComponentMap[this.workspace.onboarding_state], verify
it is a non-empty string (or select a default route like
'/integrations/qbd_direct/onboarding/landing' or
'/integrations/qbd_direct/main'), and only call
this.router.navigateByUrl(target) when target is valid (otherwise handle the
unexpected state via a log/warning and navigate to the safe default).
Description
If the user leaves mid-setup, redirect them back to the same step once they return.
Clickup
https://app.clickup.com/t/86d1xnfta
https://app.clickup.com/t/86d1xnftt
Summary by CodeRabbit
New Features
Improvements