feat: lock navigation while regenerating QWC file#1767
Open
JustARatherRidiculouslyLongUsername wants to merge 2 commits intoqwc-regen-handle-redirectionfrom
Open
feat: lock navigation while regenerating QWC file#1767JustARatherRidiculouslyLongUsername wants to merge 2 commits intoqwc-regen-handle-redirectionfrom
JustARatherRidiculouslyLongUsername wants to merge 2 commits intoqwc-regen-handle-redirectionfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an allowQwcRegeneration flag to branding configs and FeatureConfiguration, introduces a root-scoped NavigationLockService, and integrates navigation locking and gating for QWC regeneration across QBD Direct components and menu templates. Changes
Sequence DiagramsequenceDiagram
participant User
participant Menu as Main Menu
participant QBD as QBD Direct Component
participant Regen as Regenerate QWC Component
participant Nav as Navigation Lock Service
User->>Menu: open menu / click QBD
Menu->>QBD: check allowQwcRegeneration
QBD->>Nav: lock()
QBD->>Regen: navigate to regeneration flow
Regen->>Regen: perform prerequisites / download / setup
Regen->>Nav: lock() / unlock() around critical transitions
Regen->>Nav: unlock() when CONNECTION_DONE or terminal state
Nav->>Menu: isLocked signal updates
Menu->>User: tabs enabled/disabled based on isLocked
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-qwc-file/qbd-direct-regenerate-qwc-file/qbd-direct-regenerate-qwc-file.component.ts (1)
282-290:⚠️ Potential issue | 🟠 MajorUnlock navigation on the
DESTINATION_SYNC_COMPLETEerror path too.At Line 286,
catchErrorreturns without callingunlockNavigation(). If this update fails, the user can remain permanently locked from menu navigation.🔧 Proposed fix
-import { catchError, interval, map, Observable, of, switchMap, takeWhile, tap } from 'rxjs'; +import { catchError, finalize, interval, map, Observable, of, switchMap, takeWhile, tap } from 'rxjs'; @@ } else if (state === QbdDirectOnboardingState.DESTINATION_SYNC_COMPLETE) { return this.workspaceService.updateWorkspaceOnboardingState({ onboarding_state: QbdDirectOnboardingState.COMPLETE }).pipe( tap(() => { this.state.set(QwcFlowState.CONNECTION_DONE); - this.unlockNavigation(); }), catchError((error) => { console.error('Error updating workspace onboarding state:', error); this.toastService.displayToastMessage(ToastSeverity.ERROR, this.translocoService.translate('qbdDirectRegenerateQwcFile.somethingWentWrong')); return of(null); }), + finalize(() => this.unlockNavigation()), map(() => null) ); }🤖 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 282 - 290, The catchError inside the observable leaves navigation locked when updating the workspace onboarding state fails; modify the catchError handler in qbd-direct-regenerate-qwc-file.component (the block handling state.set(QwcFlowState.CONNECTION_DONE)) to call this.unlockNavigation() before returning of(null) and after showing the toast (ensure unlockNavigation() is invoked regardless of success or error), so the unlockNavigation() function is executed on the DESTINATION_SYNC_COMPLETE/error path as well.src/app/integrations/qbd-direct/qbd-direct.component.ts (1)
68-85:⚠️ Potential issue | 🟠 MajorNavigation lock can leak across flows without an explicit reset path.
This method locks navigation on the regeneration branch (Line 75), but there is no corresponding unlock in the non-regeneration branches of
navigate(). A stale locked state can keep menus disabled outside the regen flow.Suggested fix
private navigate(): void { + // Reset lock when entering root routing decision tree. + this.navigationLockService.unlock(); + const pathName = this.windowReference.location.pathname; const connectionStates = [ @@ if (brandingFeatureConfig.qbdDirect.allowQwcRegeneration && connectionStates.includes(this.workspace.onboarding_state)) { @@ this.router.navigateByUrl(`/integrations/qbd_direct/main/configuration/qwc_file/${flowRoute}`); this.navigationLockService.lock();🤖 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 68 - 85, The navigationLockService.lock() call in the regeneration branch can leak because other branches (the onboarding navigation and early returns) never call unlock; update navigate() so every execution path that previously returned or navigated without unlocking now calls navigationLockService.unlock() where appropriate (e.g., in the error callback of qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe and before any return when the lock might have been set), or refactor the async subscribe call to use a finally-like handler (or RxJS finalize) to ensure navigationLockService.unlock() runs after navigation logic; reference qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe, navigationLockService.lock(), navigationLockService.unlock(), and the navigate() method to locate and implement the change.
🧹 Nitpick comments (1)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-configuration/qbd-direct-configuration.component.ts (1)
33-36: Use guideline-compliant i18n key naming for the new QWC tab label.Line 35 adds
qbd_direct.configuration.QWCFile.stepName, which doesn't match the component i18n schema in the current guidelines. Please align this new key with the component-scoped camelCase format.🔧 Proposed key update (example)
- {label: this.translocoService.translate('qbd_direct.configuration.QWCFile.stepName'), routerLink: '/integrations/qbd_direct/main/configuration/qwc_file', value: 'qwc_file'} + {label: this.translocoService.translate('qbdDirectConfiguration.qwcFileStepName'), routerLink: '/integrations/qbd_direct/main/configuration/qwc_file', value: 'qwc_file'}As per coding guidelines:
**/*.component.{ts,html}top-level i18n object must be component/feature folder name in camelCase, and keys should be meaningful/semantic.🤖 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-main/qbd-direct-configuration/qbd-direct-configuration.component.ts` around lines 33 - 36, The i18n key used in the translocoService.translate call when pushing the QWC tab is not following the component-scoped camelCase schema; update the key passed to translocoService.translate inside the modules.push call in qbd-direct-configuration.component (the line using this.translocoService.translate('qbd_direct.configuration.QWCFile.stepName')) to a component/feature folder camelCase root (e.g., qwcFile.stepName or qbdDirectQwcFile.stepName) and add the corresponding entry to the component’s i18n object so the label follows the required component-scoped camelCase naming convention.
🤖 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 120-127: The component sets a global navigation lock in
lockNavigation() but never releases it; add an Angular lifecycle teardown to
call unlockNavigation() when the component is destroyed (implement ngOnDestroy()
in the qbd-direct-regenerate-qwc-file.component class and invoke
this.unlockNavigation()), ensuring the global lock (navigationLockService) is
always unlocked on component destruction; also consider clearing the
qbdDirectQwcLastVisitedFlowService value if appropriate inside the same
ngOnDestroy to avoid stale state.
In `@src/app/integrations/qbd-direct/qbd-direct.component.ts`:
- Line 103: The navigation call using
onboardingStateComponentMap[this.workspace.onboarding_state] can produce
undefined and break routing; update the code around router.navigateByUrl in
qbd-direct.component (the line calling this.router.navigateByUrl(...)) to
resolve a safe fallback: compute a target =
onboardingStateComponentMap[this.workspace.onboarding_state] || '<defaultRoute>'
(choose the appropriate default route string used by your app) and call
this.router.navigateByUrl(target) instead, ensuring you reference
onboardingStateComponentMap and this.workspace.onboarding_state when
implementing the guard so unexpected backend values route to the default.
In `@src/app/shared/components/menu/main-menu/main-menu.component.ts`:
- Line 80: The dropdown visibility was changed to rely solely on
brandingFeatureConfig.showMoreDropdownInMainMenu, removing the iframe-origin
guard; restore that guard so the “More” menu remains hidden in embedded/iframe
contexts by combining the two checks (e.g. set showMoreDropdown =
brandingFeatureConfig.showMoreDropdownInMainMenu && !isEmbeddedIframe() or &&
window.self === window.top), or call your existing iframe-detection helper if
present; update the assignment of showMoreDropdown in main-menu.component.ts
accordingly so both the branding flag and iframe context are respected.
---
Outside diff 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 282-290: The catchError inside the observable leaves navigation
locked when updating the workspace onboarding state fails; modify the catchError
handler in qbd-direct-regenerate-qwc-file.component (the block handling
state.set(QwcFlowState.CONNECTION_DONE)) to call this.unlockNavigation() before
returning of(null) and after showing the toast (ensure unlockNavigation() is
invoked regardless of success or error), so the unlockNavigation() function is
executed on the DESTINATION_SYNC_COMPLETE/error path as well.
In `@src/app/integrations/qbd-direct/qbd-direct.component.ts`:
- Around line 68-85: The navigationLockService.lock() call in the regeneration
branch can leak because other branches (the onboarding navigation and early
returns) never call unlock; update navigate() so every execution path that
previously returned or navigated without unlocking now calls
navigationLockService.unlock() where appropriate (e.g., in the error callback of
qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe and before
any return when the lock might have been set), or refactor the async subscribe
call to use a finally-like handler (or RxJS finalize) to ensure
navigationLockService.unlock() runs after navigation logic; reference
qbdDirectAdvancedSettingsService.getQbdAdvancedSettings().subscribe,
navigationLockService.lock(), navigationLockService.unlock(), and the navigate()
method to locate and implement the change.
---
Nitpick comments:
In
`@src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-configuration/qbd-direct-configuration.component.ts`:
- Around line 33-36: The i18n key used in the translocoService.translate call
when pushing the QWC tab is not following the component-scoped camelCase schema;
update the key passed to translocoService.translate inside the modules.push call
in qbd-direct-configuration.component (the line using
this.translocoService.translate('qbd_direct.configuration.QWCFile.stepName')) to
a component/feature folder camelCase root (e.g., qwcFile.stepName or
qbdDirectQwcFile.stepName) and add the corresponding entry to the component’s
i18n object so the label follows the required component-scoped camelCase naming
convention.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/app/branding/c1/branding-config.tssrc/app/branding/fyle/branding-config.tssrc/app/core/models/branding/feature-configuration.model.tssrc/app/core/services/common/navigation-lock.service.tssrc/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-configuration/qbd-direct-configuration.component.tssrc/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.component.tssrc/app/shared/components/menu/main-menu/main-menu.component.htmlsrc/app/shared/components/menu/main-menu/main-menu.component.tssrc/app/shared/components/menu/sub-menu/sub-menu.component.htmlsrc/app/shared/components/menu/sub-menu/sub-menu.component.ts
Comment on lines
+120
to
+127
| private lockNavigation(): void { | ||
| this.qbdDirectQwcLastVisitedFlowService.set(this.flowType); | ||
| this.navigationLockService.lock(); | ||
| } | ||
|
|
||
| private unlockNavigation(): void { | ||
| this.navigationLockService.unlock(); | ||
| } |
Contributor
There was a problem hiding this comment.
Release the global navigation lock when this component is destroyed.
Line 120 introduces a global lock path, but there is no teardown unlock. If users leave via browser back/direct URL, isLocked can remain true and keep menus disabled app-wide.
🔧 Proposed fix
-import { Component, computed, OnInit, signal } from '@angular/core';
+import { Component, computed, OnDestroy, OnInit, signal } from '@angular/core';
@@
-export class QbdDirectRegenerateQwcFileComponent implements OnInit {
+export class QbdDirectRegenerateQwcFileComponent implements OnInit, OnDestroy {
@@
ngOnInit(): void {
@@
}
+
+ ngOnDestroy(): void {
+ this.unlockNavigation();
+ }
}🤖 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 120 - 127, The component sets a global navigation lock in
lockNavigation() but never releases it; add an Angular lifecycle teardown to
call unlockNavigation() when the component is destroyed (implement ngOnDestroy()
in the qbd-direct-regenerate-qwc-file.component class and invoke
this.unlockNavigation()), ensuring the global lock (navigationLockService) is
always unlocked on component destruction; also consider clearing the
qbdDirectQwcLastVisitedFlowService value if appropriate inside the same
ngOnDestroy to avoid stale state.
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
1 participant
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.
Description
Clickup
https://app.clickup.com/t/86d1xy7uw
Summary by CodeRabbit