feat: navigate between previously connected integrations #1184
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request refactors integration handling across the application. The primary service ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (case-insensitive) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/app/shared/components/menu/main-menu/main-menu.component.ts (2)
79-129: Consider refactoringaddDropdownOptionsfor readability.
While the approach is clear, this large method could be split into smaller helpers to make it even more maintainable (e.g., extracting “Add more integrations” item creation to its own function).
132-136: Handle potential subscription cleanup or errors.
InngOnInit, make sure the subscription is properly handled (e.g., unsubscribing inOnDestroyif relevant) and consider error-handling for thegetIntegrations()call.src/app/core/services/common/integrations.service.ts (1)
21-32: Map definition is clear but consider externalizing configuration.
Placing these static mappings in a config file or environment-based constants could reduce code duplication and improve maintainability as the integration list grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/core/services/common/integrations.service.ts(1 hunks)src/app/integrations/landing-v2/landing-v2.component.ts(2 hunks)src/app/shared/components/menu/main-menu/main-menu.component.html(1 hunks)src/app/shared/components/menu/main-menu/main-menu.component.ts(5 hunks)
🧰 Additional context used
🪛 ESLint
src/app/shared/components/menu/main-menu/main-menu.component.ts
[error] 52-52: Unexpected trailing comma.
(comma-dangle)
🪛 GitHub Check: lint
src/app/shared/components/menu/main-menu/main-menu.component.ts
[failure] 52-52:
Unexpected trailing comma
🪛 GitHub Actions: TypeScript Lint Check
src/app/shared/components/menu/main-menu/main-menu.component.ts
[error] 52-52: Unexpected trailing comma comma-dangle
🪛 Biome (1.9.4)
src/app/core/services/common/integrations.service.ts
[error] 48-48: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (7)
src/app/shared/components/menu/main-menu/main-menu.component.ts (4)
6-7: New imports look good.
These additions are consistent with the new integration handling approach.
9-9: Dependency injection is appropriate.
InjectingIntegrationsServiceis a clear approach to centralizing integration logic.
42-42: Dynamic dropdown options property.
Switching from a static array to a dynamically populated property is an excellent improvement for scalability.
73-78: Validate the route matching logic forisCurrentIntegration.
Consider verifying that partial route matches won't cause false positives if there are similarly named routes or query parameters.src/app/core/services/common/integrations.service.ts (1)
2-8: New imports are consistent with the service’s expanded functionality.
Thank you for keeping dependencies cohesive and relevant.src/app/integrations/landing-v2/landing-v2.component.ts (2)
164-164: Navigation to integrations through service usage looks good.
Relying onthis.integrationService.inAppIntegrationUrlMapensures consistent routing across the application.
218-224: Verify behavior of null keys when storing connected apps.
You are ignoring unknown TPA names by filtering outnull. Confirm that this silent discard is intended or if logging/warning would be beneficial.
| private router: Router, | ||
| private integrationsService: IntegrationsService, |
There was a problem hiding this comment.
Remove the trailing comma to fix lint errors.
The trailing comma on line 52 triggers the lint rule “comma-dangle”. Below is the suggested fix:
- private integrationsService: IntegrationsService,
+ private integrationsService: IntegrationsService📝 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.
| private router: Router, | |
| private integrationsService: IntegrationsService, | |
| private router: Router, | |
| private integrationsService: IntegrationsService |
🧰 Tools
🪛 ESLint
[error] 52-52: Unexpected trailing comma.
(comma-dangle)
🪛 GitHub Check: lint
[failure] 52-52:
Unexpected trailing comma
🪛 GitHub Actions: TypeScript Lint Check
[error] 52-52: Unexpected trailing comma comma-dangle
| getIntegrationKey(tpaName: string): IntegrationAppKey | null { | ||
| if (this.tpaNameToIntegrationKeyMap.hasOwnProperty(tpaName)) { | ||
| return this.tpaNameToIntegrationKeyMap[tpaName as keyof typeof this.tpaNameToIntegrationKeyMap]; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Object.hasOwn() instead of hasOwnProperty.
Accessing hasOwnProperty directly from objects is discouraged. Here's a suggested fix:
- if (this.tpaNameToIntegrationKeyMap.hasOwnProperty(tpaName)) {
+ if (Object.hasOwn(this.tpaNameToIntegrationKeyMap, tpaName)) {📝 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.
| getIntegrationKey(tpaName: string): IntegrationAppKey | null { | |
| if (this.tpaNameToIntegrationKeyMap.hasOwnProperty(tpaName)) { | |
| return this.tpaNameToIntegrationKeyMap[tpaName as keyof typeof this.tpaNameToIntegrationKeyMap]; | |
| } | |
| return null; | |
| } | |
| getIntegrationKey(tpaName: string): IntegrationAppKey | null { | |
| - if (this.tpaNameToIntegrationKeyMap.hasOwnProperty(tpaName)) { | |
| + if (Object.hasOwn(this.tpaNameToIntegrationKeyMap, tpaName)) { | |
| return this.tpaNameToIntegrationKeyMap[tpaName as keyof typeof this.tpaNameToIntegrationKeyMap]; | |
| } | |
| return null; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
| <span [class]="{ | ||
| 'tw-text-text-danger': option.label === 'Disconnect', | ||
| 'tw-text-mandatory-field-color': isCurrentIntegration(option.label), | ||
| }"> | ||
| {{option.label}} | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use [ngClass] for Conditional Class Binding
The new changes correctly introduce conditional styling for dropdown items. However, instead of binding an object to the plain [class] attribute, consider using Angular’s [ngClass] directive. This is the standard approach for conditionally applying multiple classes and ensures better readability and maintainability.
For example, update the code as follows:
- <span [class]="{
- 'tw-text-text-danger': option.label === 'Disconnect',
- 'tw-text-mandatory-field-color': isCurrentIntegration(option.label),
- }">
- {{option.label}}
- </span>
+ <span [ngClass]="{
+ 'tw-text-text-danger': option.label === 'Disconnect',
+ 'tw-text-mandatory-field-color': isCurrentIntegration(option.label)
+ }">
+ {{option.label}}
+ </span>📝 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.
| <span [class]="{ | |
| 'tw-text-text-danger': option.label === 'Disconnect', | |
| 'tw-text-mandatory-field-color': isCurrentIntegration(option.label), | |
| }"> | |
| {{option.label}} | |
| </span> | |
| <span [ngClass]="{ | |
| 'tw-text-text-danger': option.label === 'Disconnect', | |
| 'tw-text-mandatory-field-color': isCurrentIntegration(option.label) | |
| }"> | |
| {{option.label}} | |
| </span> |
| } | ||
|
|
||
| navigateToIntegration(inAppIntegration: InAppIntegration) { | ||
| this.router.navigate([this.inAppIntegrationUrlMap[inAppIntegration]]); |
There was a problem hiding this comment.
for apps with different tpa I think we'll have to write few more logic - we can discuss over a call, little complex to type it here - can discuss in our afternoon call
* fix: hide dropdown changes for c1 * fix: add a feature flag for the 'More' dropdown
|
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/shared/components/menu/main-menu/main-menu.component.ts (1)
51-52:⚠️ Potential issueRemove the trailing comma to fix lint errors.
The trailing comma after
integrationsServiceis causing linting errors in the CI pipeline.- private integrationsService: IntegrationsService, + private integrationsService: IntegrationsService🧰 Tools
🪛 ESLint
[error] 52-52: Unexpected trailing comma.
(comma-dangle)
🪛 GitHub Check: lint
[failure] 52-52:
Unexpected trailing comma
🧹 Nitpick comments (3)
src/app/shared/components/menu/main-menu/main-menu.component.ts (3)
73-77: Consider makingisCurrentIntegrationmethod private.This method appears to be used only internally within the component for highlighting the current integration. Since it's not called from the template or exposed to other components, it should be declared as private for better encapsulation.
- isCurrentIntegration(integrationName: InAppIntegration) { + private isCurrentIntegration(integrationName: InAppIntegration) {
99-111: Apply conditional styling to highlight the current integration.The code fetches and displays integrations but doesn't highlight which one is currently active. Consider using the
isCurrentIntegrationmethod to style the current integration in the dropdown.options[0].items.unshift({ label: integrationName, + styleClass: this.isCurrentIntegration(integrationName) ? 'current-integration' : '', handler: () => { this.integrationsService.navigateToIntegration(integrationName); } });Don't forget to add the corresponding CSS class in your component's stylesheet.
120-125: Apply text-danger class to the Disconnect option.According to the PR objectives, the "Disconnect" option should be styled with a text-danger class for better visibility.
{ label: 'Disconnect', + styleClass: 'text-danger', handler: () => { this.disconnect(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/branding/c1-branding-config.ts(1 hunks)src/app/branding/fyle-branding-config.ts(1 hunks)src/app/core/models/branding/feature-configuration.model.ts(1 hunks)src/app/shared/components/menu/main-menu/main-menu.component.html(3 hunks)src/app/shared/components/menu/main-menu/main-menu.component.ts(5 hunks)
🧰 Additional context used
🪛 ESLint
src/app/shared/components/menu/main-menu/main-menu.component.ts
[error] 52-52: Unexpected trailing comma.
(comma-dangle)
🪛 GitHub Check: lint
src/app/shared/components/menu/main-menu/main-menu.component.ts
[failure] 52-52:
Unexpected trailing comma
🪛 GitHub Actions: TypeScript Lint Check
src/app/shared/components/menu/main-menu/main-menu.component.ts
[error] 52-53: Lint error: Unexpected trailing comma (comma-dangle)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unit-test
🔇 Additional comments (8)
src/app/branding/c1-branding-config.ts (1)
17-17: LGTM: Feature flag added for C1 branding configuration.The
showMoreDropdownInMainMenuproperty has been correctly added to thec1FeatureConfigobject and set tofalse, which means C1-branded applications won't show the "More" dropdown in the main menu.src/app/core/models/branding/feature-configuration.model.ts (1)
10-10: LGTM: Model updated to support new feature configuration.The
showMoreDropdownInMainMenuproperty has been properly added to theFeatureConfigurationtype definition, enabling configurable behavior across different branding profiles.src/app/branding/fyle-branding-config.ts (1)
17-17: LGTM: Feature flag added for Fyle branding configuration.The
showMoreDropdownInMainMenuproperty has been correctly added to thefyleFeatureConfigobject and set totrue, which means Fyle-branded applications will show the "More" dropdown in the main menu, consistent with the overall architecture.src/app/shared/components/menu/main-menu/main-menu.component.html (3)
5-9: LGTM: Conditional disconnect button implemented.The disconnect button is correctly implemented to appear only when
isDisconnectRequiredis true and the branding configuration doesn't show the "More" dropdown, providing an alternative access to the disconnect feature.
28-28: LGTM: Dropdown conditionally displayed based on branding configuration.The dropdown menu is now properly controlled by the
brandingFeatureConfig.showMoreDropdownInMainMenuflag, which aligns with the branding-specific configurations added in this PR.
47-52: Use[ngClass]for conditional styling.The implementation correctly applies conditional styling, but using Angular's
[ngClass]directive would be more idiomatic than binding to the plain[class]attribute.- <span [class]="{ - 'tw-text-text-danger': option.label === 'Disconnect', - 'tw-text-mandatory-field-color': isCurrentIntegration(option.label), - }"> - {{option.label}} - </span> + <span [ngClass]="{ + 'tw-text-text-danger': option.label === 'Disconnect', + 'tw-text-mandatory-field-color': isCurrentIntegration(option.label) + }"> + {{option.label}} + </span>src/app/shared/components/menu/main-menu/main-menu.component.ts (2)
79-130: Well-structured implementation of dynamic dropdown options.The
addDropdownOptionsmethod effectively constructs the dropdown menu with proper navigation capabilities. The code is well-commented, particularly explaining the backwards iteration logic.
132-138: Consider handling dropdown options for non-Fyle brand IDs.The dropdown options are only initialized for the 'fyle' brand ID. If other brand IDs need dropdown options, this implementation may be limiting.
Is there a specific reason why dropdown options are only initialized for the 'fyle' brand? If other brands need similar functionality, consider adding a fallback or alternative initialization strategy.
| ngOnInit(): void { | ||
| if (brandingConfig.brandId === 'fyle') { | ||
| this.integrationsService.getIntegrations().subscribe(integrations => { | ||
| this.addDropdownOptions(integrations); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory leak due to unmanaged subscription.
The subscription to getIntegrations() is not being stored or unsubscribed from when the component is destroyed, which could lead to memory leaks.
Consider implementing OnDestroy and unsubscribing:
-export class MainMenuComponent implements OnInit {
+export class MainMenuComponent implements OnInit, OnDestroy {
+ private subscription: Subscription;
ngOnInit(): void {
if (brandingConfig.brandId === 'fyle') {
- this.integrationsService.getIntegrations().subscribe(integrations => {
+ this.subscription = this.integrationsService.getIntegrations().subscribe(integrations => {
this.addDropdownOptions(integrations);
});
}
}
+
+ ngOnDestroy(): void {
+ if (this.subscription) {
+ this.subscription.unsubscribe();
+ }
+ }Don't forget to import OnDestroy and Subscription:
import { Component, EventEmitter, Input, OnDestroy, OnInit, Output, viewChild } from '@angular/core';
import { Subscription } from 'rxjs';
* fix: login before navigating to an app from the dropdown (wip) * fix: directly navigate to apps that have been logged in to * fix: set the correct base api before connecting to travelperk * fix: listen for postEvents globally * fix: hide duplicate options in dropdown (#1196) * fix: use latest `workspaceId` from localstorage everywhere (#1197) * feat: branding based conditions to feature flag (#1179) * feat: branding based conditions to feature flag * fix: casing bug in field initialization making the field display 'null' (#1187) * feat: hide error section conditionally skip export v2 (#1186) * fix: branding condition for import settings page (#1188) * fix: vulnerability fixes (#1189) * fix:esbult vulnerbility fix * remove storybook workflow * fix:browserTarget to buildTraget conversion (#1190) * fix: payload for fetching account types in qbd direct export settings (#1191) * fix: payload for fetching account types in qbd direct export settings (wip) * refactor: remove console.log * fix: update account types based on the field we are searching options for * refactor: remove console.log, repeated code * fix: Article links (#1195) * fix: Article links * fix test * fix test * fix test * test * asd * asd * few more * asd * fix * fix * few more links * fix: use latest `workspaceId` from localstorage everywhere * refactor: lint * test: disable flaky tests --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]> --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]>
|
PR description must contain a link to a ClickUp (case-insensitive) |
JustARatherRidiculouslyLongUsername
merged commit 7f7171b
into
add-main-menu-dropdown
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
…ection (#1183) * feat: add a dropdown menu to the main menu for navigation and disconnection * feat: navigate between previously connected integrations (#1184) * feat: navigate between previously connected integrations * fix: hide dropdown changes for c1 (#1185) * fix: hide dropdown changes for c1 * fix: add a feature flag for the 'More' dropdown * fix: login before navigating to an app from the dropdown (wip) (#1193) * fix: login before navigating to an app from the dropdown (wip) * fix: directly navigate to apps that have been logged in to * fix: set the correct base api before connecting to travelperk * fix: listen for postEvents globally * fix: hide duplicate options in dropdown (#1196) * fix: use latest `workspaceId` from localstorage everywhere (#1197) * feat: branding based conditions to feature flag (#1179) * feat: branding based conditions to feature flag * fix: casing bug in field initialization making the field display 'null' (#1187) * feat: hide error section conditionally skip export v2 (#1186) * fix: branding condition for import settings page (#1188) * fix: vulnerability fixes (#1189) * fix:esbult vulnerbility fix * remove storybook workflow * fix:browserTarget to buildTraget conversion (#1190) * fix: payload for fetching account types in qbd direct export settings (#1191) * fix: payload for fetching account types in qbd direct export settings (wip) * refactor: remove console.log * fix: update account types based on the field we are searching options for * refactor: remove console.log, repeated code * fix: Article links (#1195) * fix: Article links * fix test * fix test * fix test * test * asd * asd * few more * asd * fix * fix * few more links * fix: use latest `workspaceId` from localstorage everywhere * refactor: lint * test: disable flaky tests --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]> --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]> --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]> --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]>
…ection (#1183) * feat: add a dropdown menu to the main menu for navigation and disconnection * feat: navigate between previously connected integrations (#1184) * feat: navigate between previously connected integrations * fix: hide dropdown changes for c1 (#1185) * fix: hide dropdown changes for c1 * fix: add a feature flag for the 'More' dropdown * fix: login before navigating to an app from the dropdown (wip) (#1193) * fix: login before navigating to an app from the dropdown (wip) * fix: directly navigate to apps that have been logged in to * fix: set the correct base api before connecting to travelperk * fix: listen for postEvents globally * fix: hide duplicate options in dropdown (#1196) * fix: use latest `workspaceId` from localstorage everywhere (#1197) * feat: branding based conditions to feature flag (#1179) * feat: branding based conditions to feature flag * fix: casing bug in field initialization making the field display 'null' (#1187) * feat: hide error section conditionally skip export v2 (#1186) * fix: branding condition for import settings page (#1188) * fix: vulnerability fixes (#1189) * fix:esbult vulnerbility fix * remove storybook workflow * fix:browserTarget to buildTraget conversion (#1190) * fix: payload for fetching account types in qbd direct export settings (#1191) * fix: payload for fetching account types in qbd direct export settings (wip) * refactor: remove console.log * fix: update account types based on the field we are searching options for * refactor: remove console.log, repeated code * fix: Article links (#1195) * fix: Article links * fix test * fix test * fix test * test * asd * asd * few more * asd * fix * fix * few more links * fix: use latest `workspaceId` from localstorage everywhere * refactor: lint * test: disable flaky tests --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]> --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]> --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]> --------- Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> Co-authored-by: Ashwin Thanaraj <[email protected]>
2 participants
Description
GET /integrationscalltext-dangerClickup
Summary by CodeRabbit
New Features
Style