Skip to content

Integration status page connecting #2734

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

Merged
merged 38 commits into from
Dec 20, 2024

Conversation

emlimlf
Copy link
Collaborator

@emlimlf emlimlf commented Dec 19, 2024

Changes proposed ✍️

What

  • Modified the config integration components actions to accept custom labels
  • Changed integration status page to display the action buttons and drawer directly from the admin panels
  • Changed the behavior of integrations that don't have drawers and uses redirects to work within the admin panels list

Why

To make it easier for the user to add integrations to projects that aren't yet connected by making the integration drawer accessible on the admin > integrations page

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screenshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

Summary by CodeRabbit

  • New Features

    • Enhanced button components across various integrations to allow dynamic content insertion via <slot>.
    • Introduced new props (segmentId and grandparentId) in the Slack integration for improved URL construction.
    • Added a dropdown for managing integration actions in the integration status page.
  • Bug Fixes

    • Improved handling of integration disconnections with a new function to track events.
  • Refactor

    • Updated the integration management interface for better user interaction and responsiveness.

@emlimlf emlimlf requested a review from gaspergrom December 19, 2024 08:33
Copy link

coderabbitai bot commented Dec 19, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces a consistent enhancement across multiple integration components in the frontend. The primary modification involves replacing hardcoded "Connect" button text with a slot-based approach, enabling more flexible content insertion. Most components receive this template change, allowing parent components to dynamically define button labels. Additionally, the Discord and Slack components introduce new props like segmentId and grandparentId to enhance connection context management.

Changes

File Path Change Summary
frontend/src/config/integrations/*/components/*-connect.vue Replaced static "Connect" text with <slot> in button components for Confluence, DevTo, Discord, Discourse, Gerrit, Git, GitHub, GitLab, GroupsIO, HackerNews, Jira, LinkedIn, Reddit, Slack, StackOverflow, and Twitter
frontend/src/config/integrations/discord/components/discord-connect.vue Added segmentId and grandparentId props with local storage context management
frontend/src/config/integrations/slack/components/slack-connect.vue Added segmentId and grandparentId props, updated URL construction logic
frontend/src/modules/admin/modules/integration/pages/integration-status.page.vue Enhanced integration management interface with dropdown actions and disconnectIntegration method

Sequence Diagram

sequenceDiagram
    participant Parent as Parent Component
    participant ConnectComponent as Connect Component
    participant Integration as Integration Service

    Parent->>ConnectComponent: Provide custom button content via slot
    Parent->>ConnectComponent: Trigger connection
    ConnectComponent->>Integration: Initiate connection process
    Integration-->>ConnectComponent: Return connection status
    ConnectComponent-->>Parent: Update connection state
Loading

Possibly related PRs

Suggested labels

Improvement

Suggested reviewers

  • themarolt

Poem

🐰 Connecting buttons, flexible and bright,
Slots dancing with content, pure delight!
From Discord to Slack, components now sing,
A rabbit's code dance, on integration's wing! 🌈
Flexibility blooms, connections take flight! 🚀


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🔭 Outside diff range comments (2)
frontend/src/config/integrations/github/components/github-connect.vue (1)

GitHub integration is temporarily disabled by hardcoded UI state

The GitHub integration is currently disabled through hardcoded UI elements, not through configuration flags. This is evident in multiple components:

  • github-settings-drawer.vue: Button disabled with tooltip message
  • github-connect.vue: Connect button disabled with same tooltip
  • github-settings-drawer.vue: Settings update button disabled with same tooltip

The only configuration flag found (CROWD_GITHUB_IS_COMMIT_DATA_ENABLED) is unrelated to this issue as it only controls commit data processing.

Recommendations:

  • Consider implementing a feature flag to control this state instead of hardcoding
  • Add tracking issue reference in the tooltip message
  • Document the temporary nature of this state in the codebase
🔗 Analysis chain

Line range hint 9-18: Verify GitHub integration status

The button is disabled with a tooltip indicating GitHub integration issues. Please verify:

  1. Is this a temporary or permanent state?
  2. Should this be controlled by a configuration flag?
  3. Is there a tracking issue for the GitHub integration problems?

Let me search for any configuration flags or feature toggles that might control this GitHub integration state.


Let me check the environment variables and configuration files specifically for any GitHub integration flags.


Let me check for any recent changes or commits related to the GitHub integration status.


Let me check the specific content of these files to understand the GitHub integration disabled state implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for related GitHub integration issues or configuration
rg -g '!node_modules' -A 5 'GitHub.*disabled|GitHub.*issues'

Length of output: 9986


Script:

#!/bin/bash
# Search for GitHub-related configuration flags or feature toggles
rg -g '!node_modules' -A 5 'github.*enabled|github.*feature|github.*flag|github.*config'

# Also check environment variables or configuration files
fd -e env -e config -e yml -e yaml -e json | xargs rg -l 'github'

Length of output: 23280


Script:

#!/bin/bash
# Check environment and configuration files for GitHub-related settings
cat backend/config/custom-environment-variables.json backend/config/default.json services/apps/webhook_api/config/custom-environment-variables.json 2>/dev/null | grep -A 5 -B 5 "github"

# Also check for any recent commits or issues related to GitHub integration status
rg -g '!node_modules' "GitHub.*onboarding.*disabled" -B 2 -A 2

Length of output: 1893


Script:

#!/bin/bash
# Search for recent changes in GitHub integration components
fd -e vue -e ts -e js github | xargs rg -l "disabled.*GitHub|GitHub.*disabled" -B 3 -A 3

# Check git history for recent changes to GitHub integration files
fd "github.*\.vue|github.*\.ts|github.*\.js" | xargs git log -n 5 --oneline --

Length of output: 325


Script:

#!/bin/bash
# Check the content of the identified files
cat frontend/src/config/integrations/github/components/settings/github-settings-drawer.vue frontend/src/config/integrations/github/components/github-connect.vue frontend/src/config/integrations/github/components/connect/github-connect.vue

# Also check for any feature flags or configuration in the GitHub integration
rg -g '!node_modules' "isGithub.*Enabled|github.*enabled|GITHUB.*ENABLED" -B 2 -A 2

Length of output: 18968

frontend/src/config/integrations/gitlab/components/gitlab-connect.vue (1)

Line range hint 67-70: Improve error handling

Replace console.error with proper user feedback.

   } catch (error) {
-    console.error('Error connecting to GitLab:', error);
-    // Handle error (e.g., show an error message to the user)
+    Message.error(`Failed to connect to GitLab: ${error.message}. Please try again or contact support.`);
   }
🧹 Nitpick comments (15)
frontend/src/config/integrations/devto/components/devto-connect.vue (1)

Line range hint 3-7: Consider cleaning up or documenting commented code

There's commented-out code for a "Details" button. If this is for a future feature, consider adding a TODO comment explaining its purpose, or remove it if no longer needed.

frontend/src/config/integrations/reddit/components/reddit-connect.vue (1)

Line range hint 1-40: Consider creating a base integration connect component

There's significant duplication across the integration connect components (reddit-connect.vue, devto-connect.vue, discourse-connect.vue). Consider creating a base component to reduce duplication and improve maintainability.

Example implementation:

<!-- base-integration-connect.vue -->
<template>
  <div class="flex items-center gap-4">
    <lf-button type="secondary" @click="onConnect">
      <lf-icon name="link-simple" />
      <slot>Connect</slot>
    </lf-button>
  </div>
  <component
    :is="drawerComponent"
    v-if="isDrawerOpen"
    v-model="isDrawerOpen"
    :integration="integration"
  />
</template>

<script setup>
const props = defineProps({
  integration: {
    type: Object,
    required: true
  },
  drawerComponent: {
    type: Object,
    required: true
  }
});
</script>
frontend/src/config/integrations/discourse/components/discourse-connect.vue (1)

9-9: LGTM! Completes consistent pattern across integration components

The slot implementation completes the pattern of allowing custom labels across all integration components, aligning with the PR objectives to improve integration management flexibility.

The consistent pattern across all integration components (devto, reddit, discourse) strongly suggests an opportunity for component abstraction. Consider implementing the base component suggestion from the previous comment to improve maintainability and reduce duplication.

frontend/src/config/integrations/git/components/git-connect.vue (1)

Line range hint 3-7: Remove commented out "Details" button code.

Consider removing the commented-out code for better maintainability. If this feature is planned for future implementation, it should be tracked in an issue instead.

frontend/src/config/integrations/hackernews/components/hackernews-connect.vue (1)

Line range hint 13-17: Consider consistent drawer rendering approach across components.

While git-connect.vue always renders the drawer, this component uses v-if. Consider adopting a consistent approach across all integration components to avoid potential UX differences.

-  <lf-hackernews-settings-drawer
-    v-if="isHackernewsSettingsDrawerOpen"
-    v-model="isHackernewsSettingsDrawerOpen"
-    :integration="props.integration"
-  />
+  <lf-hackernews-settings-drawer
+    v-model="isHackernewsSettingsDrawerOpen"
+    :integration="props.integration"
+  />
frontend/src/config/integrations/stackoverflow/components/stackoverflow-connect.vue (1)

Line range hint 3-6: Clean up commented code

Consider removing the commented "Details" button code if it's no longer needed. If it's for future implementation, create a tracking issue instead.

frontend/src/config/integrations/linkedin/components/linkedin-connect.vue (1)

Line range hint 31-35: Enhance error handling in connect function

The current error handling only logs to console. Consider adding user feedback for connection failures.

 const connect = async () => {
   const nango = new Nango({ host: config.nangoUrl });
 
   try {
     await nango.auth(
       'linkedin',
       `${AuthService.getTenantId()}-linkedin`,
     );
     await callOnboard();
   } catch (e) {
-    console.error(e);
+    console.error('LinkedIn connection failed:', e);
+    // Add user notification
+    useNotify({
+      type: 'error',
+      message: 'Failed to connect to LinkedIn. Please try again.',
+    });
   }
 };
frontend/src/config/integrations/github/components/github-connect.vue (1)

Line range hint 1-43: Add type safety improvements

Consider these TypeScript improvements:

  1. Add proper typing for the slot content
  2. Define an interface for the component props
interface GitHubConnectProps {
  hideDetails?: boolean;
}

// Add to <script setup>
defineProps<GitHubConnectProps>();
frontend/src/config/integrations/discord/components/discord-connect.vue (2)

Line range hint 3-7: Remove commented-out code

The commented-out "Details" button code should be removed if it's no longer needed. Keeping commented code can lead to confusion and maintenance issues.

-    <!--      <lf-button type="secondary-ghost" @click="isDetailsModalOpen = true">-->
-    <!--        <lf-icon name="circle-info" type="regular" />-->
-    <!--        Details-->
-    <!--      </lf-button>-->

25-36: Add JSDoc comments for the new props

The new props would benefit from JSDoc documentation explaining their purpose and usage.

 const props = defineProps({
+  /** ID of the segment for integration context */
   segmentId: {
     type: String,
     required: false,
     default: null,
   },
+  /** ID of the grandparent element for integration context */
   grandparentId: {
     type: String,
     required: false,
     default: null,
   },
 });
frontend/src/config/integrations/twitter/components/twitter-connect.vue (2)

Line range hint 39-47: Refactor URL construction logic

The URL construction logic could be simplified and made more maintainable.

 const connectUrl = computed(() => {
+  const params = new URLSearchParams({
+    redirectUrl: `${window.location.protocol}//${window.location.host}${window.location.pathname}?success=true`,
+    crowdToken: AuthService.getToken(),
+    'segments[]': route.params.id
+  });
+
+  return `${config.backendUrl}/twitter/${tenant.value.id}/connect?${params.toString()}`;
 });

Line range hint 71-73: Improve error message specificity

The error message could be more specific about what went wrong during the OAuth process.

-    Message.error('Something went wrong during X/Twitter OAuth. Please try again later.');
+    Message.error(`Twitter OAuth failed: ${twitterError}. Please try again or contact support if the issue persists.`);
frontend/src/config/integrations/gitlab/components/gitlab-connect.vue (1)

Line range hint 39-47: Maintain consistency in URL construction

For consistency with other integration components, consider refactoring the URL construction using URLSearchParams.

 const connectUrl = computed(() => {
   const authStore = useAuthStore();
   const { tenant } = storeToRefs(authStore);
 
+  const params = new URLSearchParams({
+    crowdToken: AuthService.getToken(),
+    'segments[]': route.params.id
+  });
 
-  return `${config.backendUrl}/gitlab/${
-    tenant.value.id
-  }/connect?crowdToken=${AuthService.getToken()}&segments[]=${route.params.id}`;
+  return `${config.backendUrl}/gitlab/${tenant.value.id}/connect?${params.toString()}`;
 });
frontend/src/config/integrations/slack/components/slack-connect.vue (2)

31-42: Add JSDoc comments to document the props usage.

While the props are well-defined, their purpose and relationship aren't immediately clear. Please add JSDoc comments explaining:

  • When to use segmentId vs route.params.id
  • The purpose of grandparentId
  • The relationship between these props

Example:

 const props = defineProps({
+  /** 
+   * The segment ID for the integration. Used as fallback when route.params.id 
+   * is not available. 
+   */
   segmentId: {
     type: String,
     required: false,
     default: null,
   },
+  /** 
+   * ID of the parent integration section. Used for constructing the redirect URL
+   * when viewing from admin panels.
+   */
   grandparentId: {
     type: String,
     required: false,
     default: null,
   },
 });

Line range hint 3-6: Remove commented-out code.

If the details modal is no longer needed, the commented-out code should be removed. If it's for future use, consider tracking it in a separate issue.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 062368c and b9d225b.

📒 Files selected for processing (17)
  • frontend/src/config/integrations/confluence/components/confluence-connect.vue (1 hunks)
  • frontend/src/config/integrations/devto/components/devto-connect.vue (1 hunks)
  • frontend/src/config/integrations/discord/components/discord-connect.vue (2 hunks)
  • frontend/src/config/integrations/discourse/components/discourse-connect.vue (1 hunks)
  • frontend/src/config/integrations/gerrit/components/gerrit-connect.vue (1 hunks)
  • frontend/src/config/integrations/git/components/git-connect.vue (1 hunks)
  • frontend/src/config/integrations/github/components/github-connect.vue (3 hunks)
  • frontend/src/config/integrations/gitlab/components/gitlab-connect.vue (1 hunks)
  • frontend/src/config/integrations/groupsio/components/groupsio-connect.vue (1 hunks)
  • frontend/src/config/integrations/hackernews/components/hackernews-connect.vue (1 hunks)
  • frontend/src/config/integrations/jira/components/jira-connect.vue (1 hunks)
  • frontend/src/config/integrations/linkedin/components/linkedin-connect.vue (1 hunks)
  • frontend/src/config/integrations/reddit/components/reddit-connect.vue (1 hunks)
  • frontend/src/config/integrations/slack/components/slack-connect.vue (3 hunks)
  • frontend/src/config/integrations/stackoverflow/components/stackoverflow-connect.vue (1 hunks)
  • frontend/src/config/integrations/twitter/components/twitter-connect.vue (1 hunks)
  • frontend/src/modules/admin/modules/integration/pages/integration-status.page.vue (5 hunks)
🔇 Additional comments (18)
frontend/src/modules/admin/modules/integration/pages/integration-status.page.vue (1)

22-22: Padding Bottom Assigned to Table

This addition of “pb-16” increases the bottom padding within the table, which can improve spacing for scrollable content. No functional or correctness concerns identified.

frontend/src/config/integrations/groupsio/components/groupsio-connect.vue (1)

9-9: Good Use of Slot for Flexible Label

Replacing the hardcoded "Connect" text with a slot aids reusability and customization. This aligns with the slot-based approach taken in other integration components.

frontend/src/config/integrations/jira/components/jira-connect.vue (1)

9-9: Improved Reusability with Slot

Using a slot to render the button label is a solid enhancement, enabling parent components to customize the button text universally without altering this component’s internal implementation.

frontend/src/config/integrations/gerrit/components/gerrit-connect.vue (1)

9-9: Slot Integration for Button Text

Changing the button text to a slot is consistent with the approach in other integration files. This ensures uniformity and extensibility across integrations.

frontend/src/config/integrations/devto/components/devto-connect.vue (1)

9-9: LGTM! Slot implementation enhances component reusability

The change to use slots for button content follows Vue.js best practices and aligns with the PR objective of supporting custom labels while maintaining a default "Connect" text.

frontend/src/config/integrations/reddit/components/reddit-connect.vue (1)

9-9: LGTM! Consistent implementation across integration components

The slot implementation matches the pattern used in other integration components, maintaining consistency throughout the codebase.

frontend/src/config/integrations/discourse/components/discourse-connect.vue (1)

Line range hint 1-1: Verify consistent implementation across all integration components

Let's verify that all integration components follow the same pattern and identify any potential parent components using the new slot feature.

✅ Verification successful

The previous commands didn't provide clear output. Let's examine the actual content of the connect components and their usage patterns to verify consistency in implementation and slot usage.


Based on the output, I can now generate the final response as we have sufficient information about the implementation across integration components.

Implementation is consistent across integration components

All integration connect components follow the same pattern:

  • They use a consistent template structure with a flex container and gap
  • They all implement the <slot>Connect</slot> feature in the same way within an lf-button component
  • They maintain consistent button styling with type="secondary" and the link icon
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check implementation consistency across integration components and usage of slots

# Check all integration connect components for consistent implementation
echo "Checking integration connect components implementation..."
ast-grep --pattern $'<template>
  $$$
  <lf-button $$$>
    $$$
    <slot>Connect</slot>
    $$$
  </lf-button>
  $$$
</template>' frontend/src/config/integrations/*/components/*-connect.vue

# Find potential parent components using the new slot feature
echo "Checking for parent components using slots..."
rg -l "connect.*>.*</.*connect" frontend/src/

Length of output: 1559


Script:

#!/bin/bash
# Let's try a different approach to verify the implementation

# First, let's check the structure of all connect components
echo "Checking all connect components structure..."
fd -e vue "connect" frontend/src/config/integrations/ --exec cat {}

# Additionally, let's check for any direct usage of these components
echo "Checking usage of connect components..."
rg -A 2 "<\w+-connect" frontend/src/

Length of output: 42031

frontend/src/config/integrations/git/components/git-connect.vue (1)

10-10: LGTM! Slot implementation enhances component flexibility.

The slot implementation with default content maintains backward compatibility while enabling custom labels, aligning with the PR objectives.

frontend/src/config/integrations/hackernews/components/hackernews-connect.vue (1)

9-9: LGTM! Slot implementation matches the pattern.

The slot implementation is consistent with other integration components, maintaining a unified approach.

frontend/src/config/integrations/confluence/components/confluence-connect.vue (2)

9-9: LGTM! Slot implementation follows the established pattern.

The slot implementation maintains consistency with other integration components.


Line range hint 34-36: Standardize component naming convention.

The component name uses 'LfConfluenceConnect' while other integration components use 'App' prefix (e.g., 'AppGitConnect', 'AppHackerNewsConnect'). Consider standardizing the naming convention across all integration components.

 export default {
-  name: 'LfConfluenceConnect',
+  name: 'AppConfluenceConnect',
 };

Let's verify the naming convention across other integration components:

frontend/src/config/integrations/stackoverflow/components/stackoverflow-connect.vue (1)

9-9: LGTM: Slot implementation for flexible button labels

The slot implementation aligns with the PR objectives and maintains consistency with other integration components.

frontend/src/config/integrations/linkedin/components/linkedin-connect.vue (1)

9-9: LGTM: Consistent slot implementation

The slot implementation aligns with other integration components and supports custom labels.

frontend/src/config/integrations/github/components/github-connect.vue (1)

16-16: LGTM: Slot implementation matches pattern

The slot implementation is consistent with other integration components.

frontend/src/config/integrations/twitter/components/twitter-connect.vue (1)

Line range hint 3-7: Remove commented-out code

Similar to other components, remove the commented-out "Details" button code.

frontend/src/config/integrations/gitlab/components/gitlab-connect.vue (2)

Line range hint 3-7: Remove commented-out code

Similar to other components, remove the commented-out "Details" button code.


9-9: Consistent slot implementation across integration components

The slot implementation for the connect button is consistent across all integration components, which improves maintainability and follows the PR objectives.

Also applies to: 9-9

frontend/src/config/integrations/slack/components/slack-connect.vue (1)

9-9: LGTM! Slot implementation enhances component flexibility.

The replacement of hardcoded text with a slot allows for custom button labels while maintaining backward compatibility through the default "Connect" text.

Comment on lines 88 to 122
<div class="flex justify-end gap-3">
<component
:is="lfIntegrations[integration.platform].connectComponent"
v-if="status === 'notConnected' && lfIntegrations[integration.platform].connectComponent"
:integration="integration"
:hide-details="true"
:segment-id="integration.segmentId"
:grandparent-id="integration.grandparentId"
>
Connect {{ lfIntegrations[integration.platform].name }}
</component>
<component
:is="lfIntegrations[integration.platform].actionComponent"
v-if="status === 'waitingForAction' && lfIntegrations[integration.platform].actionComponent"
:integration="integration"
/>
<template v-if="status !== 'notConnected'">
<lf-dropdown placement="bottom-end" width="14.5rem">
<template #trigger>
<lf-button type="secondary-ghost" icon-only>
<lf-icon name="ellipsis" />
</lf-button>
</template>
<component
:is="lfIntegrations[integration.platform].dropdownComponent"
v-if="status === 'done' && lfIntegrations[integration.platform].dropdownComponent"
:integration="integration"
/>
<lf-dropdown-item type="danger" @click="disconnectIntegration(integration)">
<lf-icon name="link-simple-slash" type="regular" />
Disconnect integration
</lf-dropdown-item>
</lf-dropdown>
</template>
</div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Check Error Handling & Concurrency on Disconnect

The disconnectIntegration method (lines 262-272) awaits doDestroy without any try...catch or error handling. If doDestroy fails, it may not prompt any user feedback or rollback. Consider adding a try/catch or promise rejection handling to maintain a robust user experience.

Possible approach:

- await doDestroy(integration.id);
- fetchGlobalIntegrations();
+ try {
+   await doDestroy(integration.id);
+   fetchGlobalIntegrations();
+ } catch (error) {
+   // Provide user notification or log the error
+ }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 31 to 33
// const props = defineProps<{
// hideDetails: boolean;
// }>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Clarify commented prop definition

The hideDetails prop is commented out but the template references it. Either:

  1. Implement the prop if needed
  2. Remove the commented code if not needed

Comment on lines +43 to +46
if (props.grandparentId && props.segmentId) {
localStorage.setItem('segmentId', props.segmentId);
localStorage.setItem('segmentGrandparentId', props.grandparentId);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider security implications of storing IDs in localStorage

Storing sensitive IDs in localStorage could pose security risks. Consider:

  1. Using more secure storage methods like sessionStorage
  2. Encrypting the data before storage
  3. Adding expiration mechanism

Comment on lines +48 to +50
const redirectUrl = props.grandparentId && props.segmentId
? `${window.location.protocol}//${window.location.host}/integrations/${props.grandparentId}/${props.segmentId}?slack-success=true`
: `${window.location.protocol}//${window.location.host}${window.location.pathname}?slack-success=true`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

🛠️ Refactor suggestion

Based on the gathered evidence, I can now provide the final response:

Some concerns about URL construction can be addressed, others are not applicable

The codebase appears to be a client-side Vue application (evidenced by vite.config.js and frontend structure), so SSR is not a concern. However, there are still valid points to address:

  1. The redirectUrl should be properly encoded as other parts of the codebase handle sensitive URL parameters:

    • Backend expects base64 encoded state in automationSlackCallback.ts
    • Similar Twitter integration uses proper parameter handling
  2. The segments[] array notation is consistent with other integrations (Twitter, Slack) in the codebase, so this is actually the expected format.

 const redirectUrl = props.grandparentId && props.segmentId
-  ? `${window.location.protocol}//${window.location.host}/integrations/${props.grandparentId}/${props.segmentId}?slack-success=true`
-  : `${window.location.protocol}//${window.location.host}${window.location.pathname}?slack-success=true`;
+  ? `/integrations/${props.grandparentId}/${props.segmentId}?slack-success=true`
+  : `${window.location.pathname}?slack-success=true`;

 return `${config.backendUrl}/slack/${
   tenant.value.id
-}/connect?redirectUrl=${redirectUrl}&crowdToken=${AuthService.getToken()}&segments[]=${route.params.id || props.segmentId}`;
+}/connect?redirectUrl=${encodeURIComponent(window.location.origin + redirectUrl)}&crowdToken=${AuthService.getToken()}&segments[]=${route.params.id || props.segmentId}`;
🔗 Analysis chain

Review URL construction for potential issues.

Several concerns in the URL construction logic need attention:

  1. Using window.location might cause issues during SSR
  2. The redirectUrl parameter isn't properly encoded
  3. No validation for tenant.value
  4. The segments[] parameter suggests an array but accepts only one value

Let's verify the tenant validation:

Consider applying these improvements:

 const redirectUrl = props.grandparentId && props.segmentId
-  ? `${window.location.protocol}//${window.location.host}/integrations/${props.grandparentId}/${props.segmentId}?slack-success=true`
-  : `${window.location.protocol}//${window.location.host}${window.location.pathname}?slack-success=true`;
+  ? `/integrations/${props.grandparentId}/${props.segmentId}?slack-success=true`
+  : `${window.location.pathname}?slack-success=true`;

+if (!tenant.value?.id) {
+  throw new Error('Tenant information is required');
+}

 return `${config.backendUrl}/slack/${
   tenant.value.id
-}/connect?redirectUrl=${redirectUrl}&crowdToken=${AuthService.getToken()}&segments[]=${route.params.id || props.segmentId}`;
+}/connect?redirectUrl=${encodeURIComponent(window.location.origin + redirectUrl)}&crowdToken=${AuthService.getToken()}&segments[]=${route.params.id || props.segmentId}`;

Also applies to: 60-60


The previous search might have failed due to specific pattern matching. Let's try a broader search to understand tenant validation patterns and check if SSR is actually a concern in this project.


Let's verify if this is a client-side only application and how redirectUrl is handled elsewhere to determine if SSR and URL encoding are actual concerns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if tenant validation exists in other components
ast-grep --pattern 'const { tenant } = storeToRefs($_)
$$$
if (!tenant.value) {
  $$$
}'

Length of output: 90


Script:

#!/bin/bash
# Let's try a broader search for tenant validation patterns
ast-grep --pattern 'if (!tenant.value) {
  $$$
}'

# Also search for tenant usage patterns
rg -A 2 "tenant\.value" frontend/src/config/integrations/slack/components/

# Check if SSR is used in the project
fd -g "nuxt.config.*"

Length of output: 507


Script:

#!/bin/bash
# Check if this is a Vue/React SPA by looking for main configuration files
fd -e js -e ts -g "*main.*"
fd -g "vite.config.*"
fd -g "vue.config.*"

# Look for any SSR-related configurations or dependencies
rg -l "ssr|serverside|server-side" package.json

# Check how redirectUrl is handled in other parts
rg -A 2 "redirectUrl.*=" 

Length of output: 6730

gaspergrom and others added 10 commits December 19, 2024 12:59
…:CrowdDotDev/crowd.dev into improvement/integration-overview-connect
…:CrowdDotDev/crowd.dev into improvement/integration-overview-connect
Signed-off-by: Efren Lim <[email protected]>
…:CrowdDotDev/crowd.dev into improvement/integration-overview-connect
…:CrowdDotDev/crowd.dev into improvement/integration-overview-connect
@gaspergrom gaspergrom changed the title Improvement/integration overview connect Integration status page connecting Dec 20, 2024
@gaspergrom gaspergrom requested review from joanagmaia and removed request for gaspergrom December 20, 2024 10:19
@gaspergrom gaspergrom merged commit beb117e into main Dec 20, 2024
6 checks passed
@gaspergrom gaspergrom deleted the improvement/integration-overview-connect branch December 20, 2024 12:20
This was referenced Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants