Skip to content

Improve Integration Cards #643

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 3 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/resources/data/forms/integrations.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"icon": "heroicons:envelope-20-solid",
"section_name": "Notifications",
"file_name": "EmailIntegration",
"actions_file_name": "EmailIntegrationActions",
"is_pro": false,
"crisp_help_page_slug": "can-i-receive-notifications-on-form-submissions-134svqv"
},
Expand All @@ -12,20 +13,23 @@
"icon": "mdi:slack",
"section_name": "Notifications",
"file_name": "SlackIntegration",
"actions_file_name": "SlackIntegrationActions",
"is_pro": true
},
"discord": {
"name": "Discord Notification",
"icon": "ic:baseline-discord",
"section_name": "Notifications",
"file_name": "DiscordIntegration",
"actions_file_name": "DiscordIntegrationActions",
"is_pro": true
},
"webhook": {
"name": "Webhook Notification",
"icon": "material-symbols:webhook",
"section_name": "Automation",
"file_name": "WebhookIntegration",
"actions_file_name": "WebhookIntegrationActions",
"is_pro": false
},
"zapier": {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<template>
<div class="flex flex-1 items-center">
<div
v-if="integration"
class="hidden md:block space-y-1"
>
<UBadge
:label="mentionAsText(integration.data.message)"
color="gray"
size="xs"
class="max-w-[300px] truncate"
/>
</div>
</div>
</template>

<script setup>
import { mentionAsText } from '~/lib/utils.js'

const props = defineProps({
integration: {
type: Object,
required: true,
},
form: {
type: Object,
required: true,
}
})

const formIntegrationsStore = useFormIntegrationsStore()
let interval = null

onMounted(() => {
if (!props.integration.data || props.integration.data.length === 0) {
interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
setTimeout(() => { clearInterval(interval) }, 30000)
}
})

onBeforeUnmount(() => {
if (interval) {
clearInterval(interval)
}
})
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<template>
<div class="flex flex-1 items-center">
<div
v-if="integration"
class="hidden md:block space-y-1"
>
<UBadge
:label="mentionAsText(integration.data.subject)"
color="gray"
size="xs"
class="max-w-[300px] block truncate"
/>
<div class="flex items-center gap-1">
<UBadge
:label="firstEmail"
color="white"
size="xs"
class="max-w-[300px] block truncate"
/>
<UBadge
v-if="additionalEmailsCount > 0"
:label="`+${additionalEmailsCount}`"
color="white"
size="xs"
/>
</div>
</div>
</div>
</template>

<script setup>
import { mentionAsText } from '~/lib/utils.js'

const props = defineProps({
integration: {
type: Object,
required: true,
},
form: {
type: Object,
required: true,
}
})

const formIntegrationsStore = useFormIntegrationsStore()
let interval = null

onMounted(() => {
if (!props.integration.data || props.integration.data.length === 0) {
interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
setTimeout(() => { clearInterval(interval) }, 30000)
}
})

onBeforeUnmount(() => {
if (interval) {
clearInterval(interval)
}
})
Comment on lines +45 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve polling mechanism robustness

The current polling implementation has several potential issues:

  1. No error handling for failed requests
  2. Hard-coded timing values
  3. No loading state indication to users

Consider these improvements:

+const POLL_INTERVAL = 3000
+const POLL_TIMEOUT = 30000
+const loading = ref(false)
+
 const formIntegrationsStore = useFormIntegrationsStore()
 let interval = null
 
 onMounted(() => {
   if (!props.integration.data || props.integration.data.length === 0) {
-    interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
-    setTimeout(() => { clearInterval(interval) }, 30000)
+    loading.value = true
+    interval = setInterval(async () => {
+      try {
+        await formIntegrationsStore.fetchFormIntegrations(props.form.id)
+      } catch (error) {
+        console.error('Failed to fetch integrations:', error)
+        clearInterval(interval)
+        loading.value = false
+      }
+    }, POLL_INTERVAL)
+    setTimeout(() => {
+      clearInterval(interval)
+      loading.value = false
+    }, POLL_TIMEOUT)
   }
 })
📝 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 formIntegrationsStore = useFormIntegrationsStore()
let interval = null
onMounted(() => {
if (!props.integration.data || props.integration.data.length === 0) {
interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
setTimeout(() => { clearInterval(interval) }, 30000)
}
})
onBeforeUnmount(() => {
if (interval) {
clearInterval(interval)
}
})
const POLL_INTERVAL = 3000
const POLL_TIMEOUT = 30000
const loading = ref(false)
const formIntegrationsStore = useFormIntegrationsStore()
let interval = null
onMounted(() => {
if (!props.integration.data || props.integration.data.length === 0) {
loading.value = true
interval = setInterval(async () => {
try {
await formIntegrationsStore.fetchFormIntegrations(props.form.id)
} catch (error) {
console.error('Failed to fetch integrations:', error)
clearInterval(interval)
loading.value = false
}
}, POLL_INTERVAL)
setTimeout(() => {
clearInterval(interval)
loading.value = false
}, POLL_TIMEOUT)
}
})
onBeforeUnmount(() => {
if (interval) {
clearInterval(interval)
}
})


const firstEmail = computed(() => {
const emails = mentionAsText(props.integration.data.send_to).split('\n').filter(Boolean)
return emails[0] || ''
})

const additionalEmailsCount = computed(() => {
const emails = mentionAsText(props.integration.data.send_to).split('\n').filter(Boolean)
return Math.max(0, emails.length - 1)
})
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@
v-if="integration.provider"
class="hidden md:block space-y-1"
>
<div
class="font-medium mr-2"
>
{{ integration.provider.name }}
</div>
<div class="text-sm">
{{ integration.provider.email }}
</div>
<UBadge
:label="mentionAsText(integration.provider.email)"
color="gray"
size="xs"
class="max-w-[300px] truncate"
/>
</div>

<div
Expand Down Expand Up @@ -39,6 +37,8 @@
</template>

<script setup>
import { mentionAsText } from '~/lib/utils.js'

const props = defineProps({
integration: {
type: Object,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
class="text-gray-500 border shadow rounded-md p-5 mt-4 relative flex items-center"
>
<div
class="flex items-center"
class="flex items-center w-full md:max-w-[240px]"
:class="{'flex-grow': !actionsComponent}"
>
<div
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<template>
<div class="flex flex-1 items-center">
<div
v-if="integration"
class="hidden md:block space-y-1"
>
<UBadge
:label="mentionAsText(integration.data.message)"
color="gray"
size="xs"
class="max-w-[300px] block truncate"
/>
</div>
</div>
</template>

<script setup>
import { mentionAsText } from '~/lib/utils.js'

const props = defineProps({
integration: {
type: Object,
required: true,
},
form: {
type: Object,
required: true,
}
})
Comment on lines +20 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance prop definitions with validation and documentation.

The current prop definitions lack detailed type validation and documentation about the expected data structure.

Consider enhancing the props:

 const props = defineProps({
   integration: {
     type: Object,
     required: true,
+    validator(value) {
+      return value && typeof value.data === 'object'
+    },
+    default: () => ({}),
   },
   form: {
     type: Object,
     required: true,
+    validator(value) {
+      return value && typeof value.id === 'string'
+    },
+    default: () => ({}),
   }
 })

Also, consider adding JSDoc comments to document the expected prop structure:

/**
 * @typedef {Object} Integration
 * @property {Object} data - Integration data containing message
 * @property {string} data.message - The message to display
 */

/**
 * @typedef {Object} Form
 * @property {string} id - The form identifier
 */


const formIntegrationsStore = useFormIntegrationsStore()
let interval = null

onMounted(() => {
if (!props.integration.data || props.integration.data.length === 0) {
interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
setTimeout(() => { clearInterval(interval) }, 30000)
}
})

onBeforeUnmount(() => {
if (interval) {
clearInterval(interval)
}
})
Comment on lines +31 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve polling mechanism and error handling.

The current implementation has several potential issues:

  1. 3-second polling interval might be too aggressive and could cause unnecessary server load.
  2. No error handling for failed API calls.
  3. No loading state management.
  4. Potential race conditions with the interval.

Consider these improvements:

 const formIntegrationsStore = useFormIntegrationsStore()
 let interval = null
+const POLL_INTERVAL = 10000 // 10 seconds
+const MAX_RETRIES = 6 // 1 minute total
+let retryCount = 0
+const isLoading = ref(false)
 
 onMounted(() => {
   if (!props.integration.data || props.integration.data.length === 0) {
-    interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
-    setTimeout(() => { clearInterval(interval) }, 30000)
+    interval = setInterval(async () => {
+      try {
+        isLoading.value = true
+        await formIntegrationsStore.fetchFormIntegrations(props.form.id)
+        retryCount++
+        if (retryCount >= MAX_RETRIES) {
+          clearInterval(interval)
+          interval = null
+        }
+      } catch (error) {
+        console.error('Failed to fetch integrations:', error)
+        clearInterval(interval)
+        interval = null
+      } finally {
+        isLoading.value = false
+      }
+    }, POLL_INTERVAL)
   }
 })

Also add a loading indicator to the template:

 <UBadge
   :label="mentionAsText(integration.data.message)"
   color="gray"
   size="xs"
   class="max-w-[300px] block truncate"
+  :loading="isLoading"
 />

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

</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<template>
<div class="flex flex-1 items-center">
<div
v-if="integration"
class="hidden md:block space-y-1"
>
<UBadge
:label="integration.data.webhook_url"
color="gray"
size="xs"
class="max-w-[300px] block truncate"
/>
</div>
</div>
</template>

<script setup>
const props = defineProps({
integration: {
type: Object,
required: true,
},
form: {
type: Object,
required: true,
}
})

const formIntegrationsStore = useFormIntegrationsStore()
let interval = null

onMounted(() => {
if (!props.integration.data || props.integration.data.length === 0) {
interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
setTimeout(() => { clearInterval(interval) }, 30000)
}
})

Comment on lines +32 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve polling mechanism robustness

The current polling implementation has several potential issues:

  1. No maximum retry count
  2. No exponential backoff
  3. No error handling for failed requests
  4. Hardcoded timeout values

Consider implementing a more robust polling mechanism:

-let interval = null
+const MAX_RETRIES = 10
+const INITIAL_INTERVAL = 3000
+const MAX_TIMEOUT = 30000
+let retryCount = 0
+let pollingInterval = null

 onMounted(() => {
   if (!props.integration.data || props.integration.data.length === 0) {
-    interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
-    setTimeout(() => { clearInterval(interval) }, 30000)
+    const startPolling = async () => {
+      try {
+        await formIntegrationsStore.fetchFormIntegrations(props.form.id)
+        if (formIntegrationsStore.hasData) {
+          clearInterval(pollingInterval)
+          return
+        }
+        retryCount++
+        if (retryCount >= MAX_RETRIES) {
+          clearInterval(pollingInterval)
+        }
+      } catch (error) {
+        console.error('Failed to fetch integrations:', error)
+        clearInterval(pollingInterval)
+      }
+    }
+    pollingInterval = setInterval(startPolling, INITIAL_INTERVAL)
+    setTimeout(() => clearInterval(pollingInterval), MAX_TIMEOUT)
   }
 })
📝 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
onMounted(() => {
if (!props.integration.data || props.integration.data.length === 0) {
interval = setInterval(() => formIntegrationsStore.fetchFormIntegrations(props.form.id), 3000)
setTimeout(() => { clearInterval(interval) }, 30000)
}
})
const MAX_RETRIES = 10
const INITIAL_INTERVAL = 3000
const MAX_TIMEOUT = 30000
let retryCount = 0
let pollingInterval = null
onMounted(() => {
if (!props.integration.data || props.integration.data.length === 0) {
const startPolling = async () => {
try {
await formIntegrationsStore.fetchFormIntegrations(props.form.id)
if (formIntegrationsStore.hasData) {
clearInterval(pollingInterval)
return
}
retryCount++
if (retryCount >= MAX_RETRIES) {
clearInterval(pollingInterval)
}
} catch (error) {
console.error('Failed to fetch integrations:', error)
clearInterval(pollingInterval)
}
}
pollingInterval = setInterval(startPolling, INITIAL_INTERVAL)
setTimeout(() => clearInterval(pollingInterval), MAX_TIMEOUT)
}
})

onBeforeUnmount(() => {
if (interval) {
clearInterval(interval)
}
})
</script>
4 changes: 4 additions & 0 deletions client/data/forms/integrations.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"icon": "heroicons:envelope-20-solid",
"section_name": "Notifications",
"file_name": "EmailIntegration",
"actions_file_name": "EmailIntegrationActions",
"is_pro": false,
"crisp_help_page_slug": "can-i-receive-notifications-on-form-submissions-134svqv"
},
Expand All @@ -12,20 +13,23 @@
"icon": "mdi:slack",
"section_name": "Notifications",
"file_name": "SlackIntegration",
"actions_file_name": "SlackIntegrationActions",
"is_pro": true
},
"discord": {
"name": "Discord Notification",
"icon": "ic:baseline-discord",
"section_name": "Notifications",
"file_name": "DiscordIntegration",
"actions_file_name": "DiscordIntegrationActions",
"is_pro": true
},
"webhook": {
"name": "Webhook Notification",
"icon": "material-symbols:webhook",
"section_name": "Automation",
"file_name": "WebhookIntegration",
"actions_file_name": "WebhookIntegrationActions",
"is_pro": false
},
"zapier": {
Expand Down
12 changes: 12 additions & 0 deletions client/lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,15 @@ export const customDomainUsed = function () {

return host !== appDomain && getDomain(host) !== appDomain
}

export const mentionAsText = (content) => {
if (!content) return ''

// Parse the content and style mentions
return content.replace(
/<span\s+mention-field-id="([^"]+)"\s+mention-field-name="([^"]+)"[^>]*>([^<]+)<\/span>/g,
(match, fieldId, fieldName, text) => {
return `${text}`
}
)
}
Loading