-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: incorrect URL for /integrations
call and rendering errors
#1149
fix: incorrect URL for /integrations
call and rendering errors
#1149
Conversation
WalkthroughThe pull request introduces two modifications in different service and component files. In the Changes
Suggested Reviewers
Poem
Finishing Touches
🪧 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 (
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/integrations/landing-v2/landing-v2.component.ts (2)
169-169
: LGTM! Consider handling undefined state in UI.The optional chaining operator prevents runtime errors when
connectedApps
is undefined. However, the UI should handle this undefined state appropriately.Consider explicitly handling the undefined state in the template:
isAppConnected(appKey: IntegrationAppKey): boolean { return this.connectedApps?.includes(appKey) ?? false; }This ensures a consistent boolean return value, making it easier to handle in templates.
Line range hint
251-254
: Consider adding loading state handling.The integrations data is fetched asynchronously in
storeConnectedApps
, but there's no loading state management. This could lead to UI flickering or incorrect states during the initial load.Consider adding a loading state:
private loading: boolean = true; private storeConnectedApps() { this.loading = true; this.integrationService.getIntegrations().subscribe({ next: (integrations) => { const tpaNames = integrations.map(integration => integration.tpa_name); const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]); this.connectedApps = connectedApps; }, error: (error) => { console.error('Failed to fetch integrations:', error); }, complete: () => { this.loading = false; } }); }Then use this in your template to show appropriate loading states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/core/services/common/integrations.service.ts
(1 hunks)src/app/integrations/landing-v2/landing-v2.component.ts
(1 hunks)
🔇 Additional comments (1)
src/app/core/services/common/integrations.service.ts (1)
19-21
: LGTM! Consider caching the base URL setup.Moving
setBaseApiURL
here ensures correct URL configuration before each API call, fixing the incorrect URL issue. However, ifgetIntegrations
is called frequently, consider caching the URL setup to avoid unnecessary resets.Let's verify if multiple calls to
getIntegrations
are common:
0facabc
into
landing-v2-fix-comments
* fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: create new landing page and add header and tab switcher * feat: new app tile structure + remove extra content (#1140) * feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: create new landing page and add header and tab switcher * feat: new app tile structure + remove extra content (#1140) * feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
Clickup
https://app.clickup.com/t/86cxhent0
Summary by CodeRabbit
isAppConnected
method to prevent potential errors whenconnectedApps
is undefined