Skip to content
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

Github archive revert #2757

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

gaspergrom
Copy link
Contributor

@gaspergrom gaspergrom commented Jan 10, 2025

Changes proposed ✍️

What

copilot:summary

copilot:poem

Why

How

copilot:walkthrough

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

Release Notes

  • New Features

    • Added GitHub integration components for connecting, configuring, and managing GitHub repositories
    • Introduced new modals and drawers for GitHub integration setup
    • Implemented repository mapping and connection functionality
  • Improvements

    • Restructured GitHub integration components and services
    • Enhanced user interface for GitHub integration settings
    • Added more detailed onboarding and connection workflows
  • Changes

    • Reorganized GitHub integration file structure
    • Updated import paths for GitHub-related components and services

@gaspergrom gaspergrom requested a review from joanagmaia January 10, 2025 11:13
@gaspergrom gaspergrom self-assigned this Jan 10, 2025
Copy link

coderabbitai bot commented Jan 10, 2025

Walkthrough

This pull request introduces a comprehensive refactoring of the GitHub integration components, primarily focusing on restructuring the project's file organization from a github to a github-archive directory. The changes include creating new components for GitHub connection, settings, and status, updating import paths, and modifying the configuration to support these new components. The refactoring aims to improve the modularity and organization of the GitHub integration functionality.

Changes

File Change Summary
frontend/src/config/integrations/github-archive/components/ Multiple new components added: github-connect.vue, github-details-modal.vue, github-dropdown.vue, github-params.vue, settings/github-settings-*
frontend/src/config/integrations/github-archive/config.ts New configuration file for GitHub integration added
frontend/src/config/integrations/github-archive/services/github.api.service.ts Import paths updated for GitHub types
frontend/src/config/integrations/github/components/ New components added: connect/github-connect-modal.vue, connect/github-connect-finishing-modal.vue, github-action.vue, github-status.vue, settings/github-settings-bulk-select.vue
Import Paths Systematically updated from @/config/integrations/github/... to @/config/integrations/github-archive/...

Sequence Diagram

sequenceDiagram
    participant User
    participant GithubConnectComponent
    participant GithubConnectModal
    participant GithubApiService
    participant IntegrationService

    User->>GithubConnectComponent: Click "Connect GitHub"
    GithubConnectComponent->>GithubConnectModal: Open Modal
    User->>GithubConnectModal: Select GitHub Installation
    GithubConnectModal->>GithubApiService: Fetch Installation Details
    GithubApiService-->>GithubConnectModal: Return Installation Info
    User->>GithubConnectModal: Confirm Connection
    GithubConnectModal->>IntegrationService: Initiate GitHub Connection
    IntegrationService-->>GithubConnectModal: Connection Status
    GithubConnectModal->>User: Display Connection Result
Loading

Possibly related PRs

Suggested Labels

improvement, refactoring, github-integration

Suggested Reviewers

  • themarolt

Poem

🐰 Hopping through code with glee,
GitHub integration, now set free!
Archiving paths with rabbit's might,
Components dancing, oh so bright!
A refactor's tale of pure delight! 🚀

Finishing Touches

  • 📝 Generate Docstrings (Beta)

🪧 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: 11

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

Line range hint 44-57: Add validation for OAuth query parameters.

The GitHub OAuth flow handling lacks validation for required query parameters.

 const finallizeGithubConnect = () => {
   const {
     code, source, state,
   } = route.query;
   const setupAction = route.query.setup_action;
   const installId = route.query.installation_id;

+  // Validate required parameters
+  if (!code) {
+    console.error('Missing required OAuth code parameter');
+    return;
+  }
+
   if (code && !source && state !== 'noconnect') {
     isFinishingModalOpen.value = true;
     doGithubConnect({
🧹 Nitpick comments (12)
frontend/src/config/integrations/github/components/github-status.vue (1)

3-3: Remove redundant props prefix in template

The props prefix is unnecessary in the template when using script setup.

-    v-if="props.integration?.status === 'mapping'"
+    v-if="integration?.status === 'mapping'"
frontend/src/config/integrations/github/components/github-action.vue (1)

21-23: Potential reactivity issue with drawer state

The initial state of isSettingsDrawerOpen is set only once based on the integration status. If the status changes, the drawer state won't update automatically.

-const isSettingsDrawerOpen = ref(props.integration.status === 'mapping');
+const isSettingsDrawerOpen = ref(false);
+
+watch(
+  () => props.integration.status,
+  (newStatus) => {
+    isSettingsDrawerOpen.value = newStatus === 'mapping';
+  },
+  { immediate: true }
+);
frontend/src/config/integrations/github/components/connect/github-connect-finishing-modal.vue (1)

2-14: Add accessibility attributes to improve user experience

The modal should include proper ARIA attributes for better accessibility.

-  <lf-modal v-model="isModalOpen" width="25rem">
+  <lf-modal 
+    v-model="isModalOpen" 
+    width="25rem"
+    role="dialog"
+    aria-labelledby="setup-title"
+    aria-describedby="setup-description"
+  >
     <div class="p-6">
       <div class="flex items-center gap-2">
-        <h5>Finishing the setup</h5>
+        <h5 id="setup-title">Finishing the setup</h5>
         <lf-spinner size="1.25rem" />
       </div>
-      <p class="text-small pt-3 text-gray-500">
+      <p id="setup-description" class="text-small pt-3 text-gray-500">
frontend/src/config/integrations/github/components/settings/github-settings-bulk-select.vue (1)

128-140: Add confirmation dialog for bulk actions

The mapRepos function should show a confirmation dialog before applying bulk changes, especially when "All repositories" is selected.

 const mapRepos = () => {
+  const repoCount = isAll.value ? props.repositories.length : form.repositories.length;
+  if (repoCount > 10) {  // Show confirmation for large changes
+    if (!confirm(`Are you sure you want to map ${repoCount} repositories to "${props.subprojects.find(s => s.id === form.subproject)?.name}"?`)) {
+      return;
+    }
+  }
+
   let repos = form.repositories;
   if (isAll.value) {
     repos = props.repositories.map((r) => r.url);
   }
   const data = repos.reduce((mapping, url) => ({
     ...mapping,
     [url]: form.subproject,
   }), {});
   emit('apply', data);
   reset();
   isModalVisible.value = false;
 };
frontend/src/config/integrations/github-archive/components/github-details-modal.vue (1)

64-66: Replace placeholder text with actual content.

The component contains Lorem ipsum placeholder text that should be replaced with actual descriptive content about GitHub repository selection and mapping process.

Also applies to: 77-79

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

181-190: Add clipboard API fallback mechanism.

The clipboard operation lacks a fallback mechanism for browsers where navigator.clipboard is not available.

 const copy = () => {
-  if (navigator.clipboard) {
+  if (navigator.clipboard) {
     const urlWithState = `${config.gitHubInstallationUrl}?state=noconnect`;
     navigator.clipboard.writeText(urlWithState);
     copied.value = true;
     setTimeout(() => {
       copied.value = false;
     }, 1000);
+  } else {
+    // Fallback using textarea
+    const textarea = document.createElement('textarea');
+    const urlWithState = `${config.gitHubInstallationUrl}?state=noconnect`;
+    textarea.value = urlWithState;
+    document.body.appendChild(textarea);
+    textarea.select();
+    try {
+      document.execCommand('copy');
+      copied.value = true;
+      setTimeout(() => {
+        copied.value = false;
+      }, 1000);
+    } catch (err) {
+      console.error('Fallback clipboard copy failed:', err);
+    }
+    document.body.removeChild(textarea);
+  }
 };
frontend/src/config/integrations/github/components/connect/github-connect.vue (1)

Line range hint 3-7: Remove or document commented code.

The file contains multiple blocks of commented-out code without explanation. Either remove this code if it's no longer needed or document why it's temporarily disabled.

Also applies to: 14-19, 34-34

frontend/src/config/integrations/github/components/settings/github-settings-drawer.vue (5)

12-16: Add aria-label to improve accessibility.

The GitHub logo image should have a more descriptive aria-label for better accessibility.

 <img
   :src="githubDetails.image"
   class="w-6 h-6 mr-2"
   alt="GitHub logo"
+  aria-label="GitHub Integration Logo"
 />

53-59: Enhance warning message accessibility.

The warning message about irreversible repository mapping should be marked with appropriate ARIA roles.

-<div class="border border-yellow-100 rounded-md bg-yellow-50 p-2 flex">
+<div class="border border-yellow-100 rounded-md bg-yellow-50 p-2 flex" role="alert" aria-live="polite">
   <div class="w-4 h-4 flex items-center ri-alert-fill text-yellow-500" />
   <div class="flex-grow text-yellow-900 text-2xs leading-4.5 pl-2">
     Repository mapping is not reversible. Once GitHub is connected,
     you won't be able to update these settings and reconnecting a different organization or repositories won't override past activities.
   </div>
 </div>

236-239: Add null checks and default values for form initialization.

The form initialization assumes repos array exists and has values. Add proper null checks and default values.

-const form = ref<Record<string, string>>(repos.value.reduce((a: Record<string, any>, b: any) => ({
+const form = ref<Record<string, string>>((repos.value || []).reduce((a: Record<string, any>, b: any) => ({
   ...a,
   [b.url]: props.integration.segmentId,
-}), {}));
+}), {} as Record<string, string>));

290-294: Improve error handling with specific error messages.

The error handling is too generic. Consider catching specific error types and providing more detailed error messages.

-.catch(() => {
+.catch((error) => {
+  const errorMessage = error.response?.data?.message 
+    || 'There was an error mapping GitHub repos. Please try again or contact support if the issue persists.';
   Message.error(
-    'There was an error mapping github repos',
+    errorMessage,
   );
 });

251-289: Handle loading state consistently.

The sending ref is defined but not used consistently throughout the connection flow.

 const connect = () => {
+  if (sending.value) return;
   const data = { ...form.value };
   ConfirmDialog({
     // ... dialog config
   } as any)
     .then(() => {
+      sending.value = true;
       IntegrationService.githubMapRepos(props.integration.id, data, [props.integration.segmentId])
         .then(() => {
           isDrawerVisible.value = false;
           // ... success handling
         })
         .catch(() => {
           // ... error handling
         })
+        .finally(() => {
+          sending.value = false;
+        });
     });
 };
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 191f50b and 0218ca7.

📒 Files selected for processing (20)
  • frontend/src/config/integrations/github-archive/components/connect/github-connect.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/components/github-connect.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/components/github-details-modal.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/components/github-dropdown.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/components/github-params.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-add-repository-modal.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-drawer.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-mapping.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-org-item.vue (2 hunks)
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-repo-item.vue (1 hunks)
  • frontend/src/config/integrations/github-archive/config.ts (1 hunks)
  • frontend/src/config/integrations/github-archive/services/github.api.service.ts (1 hunks)
  • frontend/src/config/integrations/github/components/connect/github-connect-finishing-modal.vue (1 hunks)
  • frontend/src/config/integrations/github/components/connect/github-connect-modal.vue (1 hunks)
  • frontend/src/config/integrations/github/components/connect/github-connect.vue (2 hunks)
  • frontend/src/config/integrations/github/components/github-action.vue (1 hunks)
  • frontend/src/config/integrations/github/components/github-status.vue (1 hunks)
  • frontend/src/config/integrations/github/components/settings/github-settings-bulk-select.vue (1 hunks)
  • frontend/src/config/integrations/github/components/settings/github-settings-drawer.vue (2 hunks)
  • frontend/src/config/integrations/github/config.ts (1 hunks)
✅ Files skipped from review due to trivial changes (7)
  • frontend/src/config/integrations/github-archive/services/github.api.service.ts
  • frontend/src/config/integrations/github-archive/components/github-connect.vue
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-repo-item.vue
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-org-item.vue
  • frontend/src/config/integrations/github-archive/components/github-dropdown.vue
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-add-repository-modal.vue
  • frontend/src/config/integrations/github-archive/components/settings/github-settings-mapping.vue
🔇 Additional comments (4)
frontend/src/config/integrations/github/config.ts (1)

13-14: Verify impact of dropdown component removal

The removal of dropdownComponent might be a breaking change. Please ensure all references to this component have been updated accordingly.

✅ Verification successful

Dropdown removal appears safe

The only reference to the dropdown component exists in an unused archive file (frontend/src/config/integrations/github-archive/config.ts). This file appears to be deprecated as it's not imported anywhere in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to GitHub dropdown component
rg -l "GithubDropdown|dropdownComponent.*github" 

Length of output: 108


Script:

#!/bin/bash
# Check content of the archive file
echo "=== Archive file content ==="
cat frontend/src/config/integrations/github-archive/config.ts

echo -e "\n=== Usage of archive config ==="
# Search for imports of the archive config
rg -l "github-archive/config"

Length of output: 854

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

22-31: Consider adding error handling and timeout

The component should handle cases where the setup process takes too long or fails.

Consider implementing:

  1. A timeout mechanism to prevent indefinite loading
  2. Error state handling to show appropriate messages
  3. A way to retry or cancel the operation

Would you like me to provide an implementation for these improvements?

frontend/src/config/integrations/github-archive/components/github-details-modal.vue (1)

1-112: Well-structured component with clear separation of concerns.

The component follows Vue 3 best practices with proper TypeScript integration, semantic HTML, and clear organization of code.

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

1-220: Well-implemented connection modal with comprehensive user flows.

The component effectively handles different user roles and provides clear guidance for both admin and non-admin users.

Comment on lines +14 to +16
const props = defineProps<{
integration: any;
}>();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety for the integration prop

Instead of using any, define a proper interface for the integration object to improve type safety and maintainability.

+interface GitHubIntegration {
+  status: 'mapping' | 'connected' | 'disconnected';
+  // add other required properties
+}

 const props = defineProps<{
-  integration: any;
+  integration: GitHubIntegration;
 }>();

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

Comment on lines +1 to +16
import { IntegrationConfig } from '@/config/integrations';
import GithubConnect from './components/github-connect.vue';
import GithubParams from './components/github-params.vue';
import GithubDropdown from './components/github-dropdown.vue';

const github: IntegrationConfig = {
key: 'github',
name: 'GitHub',
image: '/images/integrations/github.png',
description: 'Connect GitHub to sync profile information, stars, forks, pull requests, issues, and discussions.',
connectComponent: GithubConnect,
dropdownComponent: GithubDropdown,
connectedParamsComponent: GithubParams,
};

export default github;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove duplicate GitHub configuration from archive

According to the PR objectives to revert GitHub archive changes, this configuration file should be removed as it duplicates the configuration in github/config.ts.

The components should be imported and configured only in frontend/src/config/integrations/github/config.ts.

Comment on lines +17 to +19
const props = defineProps<{
integration: any;
}>();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety and error handling

The component has the following issues:

  1. Using any type reduces type safety
  2. Missing null check for integration prop
+interface GitHubIntegration {
+  status: 'mapping' | 'connected' | 'disconnected';
+  // add other required properties
+}

 const props = defineProps<{
-  integration: any;
+  integration: GitHubIntegration;
 }>();

+if (!props.integration) {
+  throw new Error('Integration prop is required');
+}

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

Comment on lines +59 to +69
const mappings = ref([]);

const repoNameFromUrl = (url) => url.split('/').at(-1);

onMounted(() => {
if (props.integration.status !== 'mapping') {
IntegrationService.fetchGitHubMappings(props.integration).then((res) => {
mappings.value = res;
});
}
});
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and loading state

The component lacks error handling for the API call and doesn't show a loading state.

+const isLoading = ref(false);
+const error = ref<Error | null>(null);
 const mappings = ref([]);

 onMounted(() => {
   if (props.integration.status !== 'mapping') {
+    isLoading.value = true;
     IntegrationService.fetchGitHubMappings(props.integration).then((res) => {
       mappings.value = res;
+    }).catch((err) => {
+      error.value = err;
+      console.error('Failed to fetch GitHub mappings:', err);
+    }).finally(() => {
+      isLoading.value = 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.

Suggested change
const mappings = ref([]);
const repoNameFromUrl = (url) => url.split('/').at(-1);
onMounted(() => {
if (props.integration.status !== 'mapping') {
IntegrationService.fetchGitHubMappings(props.integration).then((res) => {
mappings.value = res;
});
}
});
const isLoading = ref(false);
const error = ref<Error | null>(null);
const mappings = ref([]);
const repoNameFromUrl = (url) => url.split('/').at(-1);
onMounted(() => {
if (props.integration.status !== 'mapping') {
isLoading.value = true;
IntegrationService.fetchGitHubMappings(props.integration).then((res) => {
mappings.value = res;
}).catch((err) => {
error.value = err;
console.error('Failed to fetch GitHub mappings:', err);
}).finally(() => {
isLoading.value = false;
});
}
});

Comment on lines +55 to +57
const props = defineProps<{
integration: any;
}>();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace any type with proper interface

Using any type reduces type safety. Define a proper interface for the integration prop.

+interface GitHubIntegration {
+  status: string;
+  id: string;
+  // Add other required properties
+}

 const props = defineProps<{
-  integration: any;
+  integration: GitHubIntegration;
 }>();

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

Comment on lines +48 to +65
const finallizeGithubConnect = () => {
const {
code, source, state,
} = route.query;
const setupAction = route.query.setup_action;
const installId = route.query.installation_id;

if (code && !source && state !== 'noconnect') {
isFinishingModalOpen.value = true;
doGithubConnect({
code,
installId,
setupAction,
}).then(() => {
isFinishingModalOpen.value = false;
});
}
};
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation and error handling

The finallizeGithubConnect function lacks proper input validation and error handling.

 const finallizeGithubConnect = () => {
   const {
     code, source, state,
   } = route.query;
   const setupAction = route.query.setup_action;
   const installId = route.query.installation_id;

+  // Validate query parameters
+  if (!code || typeof code !== 'string') {
+    console.error('Invalid or missing code parameter');
+    return;
+  }

   if (code && !source && state !== 'noconnect') {
     isFinishingModalOpen.value = true;
     doGithubConnect({
       code,
       installId,
       setupAction,
-    }).then(() => {
+    }).then((response) => {
+      // Handle successful connection
       isFinishingModalOpen.value = false;
+    }).catch((error) => {
+      console.error('GitHub connection failed:', error);
+      // Show error message to user
+      isFinishingModalOpen.value = 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.

Suggested change
const finallizeGithubConnect = () => {
const {
code, source, state,
} = route.query;
const setupAction = route.query.setup_action;
const installId = route.query.installation_id;
if (code && !source && state !== 'noconnect') {
isFinishingModalOpen.value = true;
doGithubConnect({
code,
installId,
setupAction,
}).then(() => {
isFinishingModalOpen.value = false;
});
}
};
const finallizeGithubConnect = () => {
const {
code, source, state,
} = route.query;
const setupAction = route.query.setup_action;
const installId = route.query.installation_id;
// Validate query parameters
if (!code || typeof code !== 'string') {
console.error('Invalid or missing code parameter');
return;
}
if (code && !source && state !== 'noconnect') {
isFinishingModalOpen.value = true;
doGithubConnect({
code,
installId,
setupAction,
}).then((response) => {
// Handle successful connection
isFinishingModalOpen.value = false;
}).catch((error) => {
console.error('GitHub connection failed:', error);
// Show error message to user
isFinishingModalOpen.value = false;
});
}
};

Comment on lines +83 to +87
const props = defineProps<{
modelValue: boolean,
repositories: any[],
subprojects: any[],
}>();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Define proper interfaces for props

Replace any[] with proper TypeScript interfaces for better type safety.

+interface Repository {
+  url: string;
+  name: string;
+}

+interface Subproject {
+  id: string;
+  name: string;
+}

 const props = defineProps<{
   modelValue: boolean,
-  repositories: any[],
-  subprojects: any[],
+  repositories: Repository[],
+  subprojects: Subproject[],
 }>();

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

Comment on lines +223 to +229
.catch(() => {
Message.error(
props.integration?.id
? 'There was error updating settings'
: 'There was error connecting GitHub',
);
});
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling with specific error messages.

The error handling uses generic messages that don't provide users with actionable information about what went wrong.

-    .catch(() => {
+    .catch((error) => {
+      const errorMessage = error.response?.data?.message 
+        || error.message 
+        || (props.integration?.id
+          ? 'There was error updating settings'
+          : 'There was error connecting GitHub');
       Message.error(
-        props.integration?.id
-          ? 'There was error updating settings'
-          : 'There was error connecting GitHub',
+        errorMessage
       );
     });
📝 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.

Suggested change
.catch(() => {
Message.error(
props.integration?.id
? 'There was error updating settings'
: 'There was error connecting GitHub',
);
});
.catch((error) => {
const errorMessage = error.response?.data?.message
|| error.message
|| (props.integration?.id
? 'There was error updating settings'
: 'There was error connecting GitHub');
Message.error(
errorMessage
);
});

Comment on lines +148 to +157
const allOrganizations = computed<any[]>(() => {
const owners = new Set();
return repositories.value.reduce((acc: any[], r) => {
if (!owners.has(r.org!.name)) {
owners.add(r.org!.name);
acc.push(r.org!);
}
return acc;
}, []);
});
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix type safety in allOrganizations computed property.

The computed property uses any[] type and unsafe property access with ! operator.

-const allOrganizations = computed<any[]>(() => {
+const allOrganizations = computed<GitHubOrganization[]>(() => {
   const owners = new Set<string>();
-  return repositories.value.reduce((acc: any[], r) => {
-    if (!owners.has(r.org!.name)) {
-      owners.add(r.org!.name);
-      acc.push(r.org!);
+  return repositories.value.reduce((acc: GitHubOrganization[], r) => {
+    if (r.org && !owners.has(r.org.name)) {
+      owners.add(r.org.name);
+      acc.push(r.org);
     }
     return acc;
   }, []);
 });
📝 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.

Suggested change
const allOrganizations = computed<any[]>(() => {
const owners = new Set();
return repositories.value.reduce((acc: any[], r) => {
if (!owners.has(r.org!.name)) {
owners.add(r.org!.name);
acc.push(r.org!);
}
return acc;
}, []);
});
const allOrganizations = computed<GitHubOrganization[]>(() => {
const owners = new Set<string>();
return repositories.value.reduce((acc: GitHubOrganization[], r) => {
if (r.org && !owners.has(r.org.name)) {
owners.add(r.org.name);
acc.push(r.org);
}
return acc;
}, []);
});

Comment on lines 184 to 187
const props = defineProps<{
modelValue: boolean;
integration?: Integration<GitHubSettings>;
segmentId: string | null;
grandparentId: string | null;
modelValue: boolean,
integration: any
}>();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add proper TypeScript interface for integration prop.

Using any type reduces type safety. Consider creating a proper interface for the integration prop.

+interface GitHubIntegration {
+  id: string;
+  segmentId: string;
+  settings?: {
+    repos: Array<{
+      url: string;
+      name: string;
+      owner: string;
+    }>;
+    orgAvatar?: string;
+  };
+}

 const props = defineProps<{
   modelValue: boolean,
-  integration: any
+  integration: GitHubIntegration
 }>();
📝 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.

Suggested change
const props = defineProps<{
modelValue: boolean;
integration?: Integration<GitHubSettings>;
segmentId: string | null;
grandparentId: string | null;
modelValue: boolean,
integration: any
}>();
interface GitHubIntegration {
id: string;
segmentId: string;
settings?: {
repos: Array<{
url: string;
name: string;
owner: string;
}>;
orgAvatar?: string;
};
}
const props = defineProps<{
modelValue: boolean,
integration: GitHubIntegration
}>();

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.

1 participant