Merge pull request #2428 from omnivore-app/fix/save-page

fix: failed to save tweet

packages/api/src/resolvers/article/index.ts

@@ -5,7 +5,6 @@
 /* eslint-disable @typescript-eslint/no-floating-promises */
 import { Readability } from '@omnivore/readability'
 import graphqlFields from 'graphql-fields'
-import normalizeUrl from 'normalize-url'
 import { searchHighlights } from '../../elastic/highlights'
 import {
   createPage,
@@ -83,7 +82,7 @@ import {
   createLabels,
   getLabelsByIds,
 } from '../../services/labels'
-import { parsedContentToPage } from '../../services/save_page'
+import { cleanUrl, parsedContentToPage } from '../../services/save_page'
 import { traceAs } from '../../tracing'
 import { Merge } from '../../util'
 import { analytics } from '../../utils/analytics'
@@ -228,10 +227,7 @@ export const createArticleResolver = authorized<
           pageType: PageType.Unknown,
           contentReader: ContentReader.Web,
           author: '',
-          url: normalizeUrl(canonicalUrl || url, {
-            stripHash: true,
-            stripWWW: false,
-          }),
+          url: cleanUrl(canonicalUrl || url),
           hash: '',
           isArchived: false,
         },

packages/api/src/resolvers/article_saving_request/index.ts

@@ -1,5 +1,4 @@
 /* eslint-disable prefer-const */
-import normalizeUrl from 'normalize-url'
 import { getPageByParam } from '../../elastic/pages'
 import { User } from '../../entity/user'
 import { getRepository } from '../../entity/utils'
@@ -16,6 +15,7 @@ import {
   QueryArticleSavingRequestArgs,
 } from '../../generated/graphql'
 import { createPageSaveRequest } from '../../services/create_page_save_request'
+import { cleanUrl } from '../../services/save_page'
 import { analytics } from '../../utils/analytics'
 import {
   authorized,
@@ -75,12 +75,7 @@ export const articleSavingRequestResolver = authorized<
     return { errorCodes: [ArticleSavingRequestErrorCode.Unauthorized] }
   }
 
-  const normalizedUrl = url
-    ? normalizeUrl(url, {
-        stripHash: true,
-        stripWWW: false,
-      })
-    : undefined
+  const normalizedUrl = url ? cleanUrl(url) : undefined
 
   const params = {
     _id: id || undefined,

packages/api/src/services/create_page_save_request.ts

@@ -1,4 +1,3 @@
-import normalizeUrl from 'normalize-url'
 import * as privateIpLib from 'private-ip'
 import { v4 as uuidv4 } from 'uuid'
 import { createPubSubClient, PubsubClient } from '../datalayer/pubsub'
@@ -17,6 +16,7 @@ import {
 } from '../generated/graphql'
 import { enqueueParseRequest } from '../utils/createTask'
 import { generateSlug, pageToArticleSavingRequest } from '../utils/helpers'
+import { cleanUrl } from './save_page'
 
 interface PageSaveRequest {
   userId: string
@@ -104,10 +104,7 @@ export const createPageSaveRequest = async ({
   priority = priority || (await getPriorityByRateLimit(userId))
 
   // look for existing page
-  const normalizedUrl = normalizeUrl(url, {
-    stripHash: true,
-    stripWWW: false,
-  })
+  const normalizedUrl = cleanUrl(url)
 
   const ctx = {
     pubsub,
@@ -164,7 +161,7 @@ export const createPageSaveRequest = async ({
   }))
   // enqueue task to parse page
   await enqueueParseRequest({
-    url,
+    url: normalizedUrl,
     userId,
     saveRequestId: page.id,
     priority,

packages/api/src/services/save_email.ts

@@ -1,4 +1,3 @@
-import normalizeUrl from 'normalize-url'
 import { PubsubClient } from '../datalayer/pubsub'
 import { createPage, getPageByParam, updatePage } from '../elastic/pages'
 import { ArticleSavingRequestStatus, Page } from '../elastic/types'
@@ -14,6 +13,7 @@ import {
   parsePreparedContent,
   parseUrlMetadata,
 } from '../utils/parser'
+import { cleanUrl } from './save_page'
 
 export type SaveContext = {
   pubsub: PubsubClient
@@ -63,10 +63,7 @@ export const saveEmail = async (
     description: metadata?.description || parseResult.parsedContent?.excerpt,
     title: input.title,
     author: input.author,
-    url: normalizeUrl(parseResult.canonicalUrl || url, {
-      stripHash: true,
-      stripWWW: false,
-    }),
+    url: cleanUrl(parseResult.canonicalUrl || url),
     pageType: parseResult.pageType,
     hash: stringToHash(content),
     image:

packages/api/src/services/save_page.ts

@@ -36,13 +36,30 @@ type SaverUserData = {
   username: string
 }
 
+const TWEET_URL_REGEX =
+  /twitter\.com\/(?:#!\/)?(\w+)\/status(?:es)?\/(\d+)(?:\/.*)?/
+
 // where we can use APIs to fetch their underlying content.
 const FORCE_PUPPETEER_URLS = [
-  // twitter status url regex
-  /twitter\.com\/(?:#!\/)?(\w+)\/status(?:es)?\/(\d+)(?:\/.*)?/,
+  TWEET_URL_REGEX,
   /^((?:https?:)?\/\/)?((?:www|m)\.)?((?:youtube\.com|youtu.be))(\/(?:[\w-]+\?v=|embed\/|v\/)?)([\w-]+)(\S+)?$/,
 ]
 
+export const cleanUrl = (url: string) => {
+  const trackingParams: (RegExp | string)[] = [/^utm_\w+/i] // remove utm tracking parameters
+  if (TWEET_URL_REGEX.test(url)) {
+    console.debug('cleaning tweet url', url)
+    // remove tracking parameters from tweet links:
+    // https://twitter.com/omnivore/status/1673218959624093698?s=12&t=R91quPajs0E53Yds-fhv2g
+    trackingParams.push('s', 't')
+  }
+  return normalizeUrl(url, {
+    stripHash: true,
+    stripWWW: false,
+    removeQueryParameters: trackingParams,
+  })
+}
+
 const createSlug = (url: string, title?: Maybe<string> | undefined) => {
   const { pathname } = new URL(url)
   const croppedPathname = decodeURIComponent(
@@ -104,11 +121,7 @@ export const savePage = async (
     originalHtml: parseResult.domContent,
     canonicalUrl: parseResult.canonicalUrl,
   })
-  // check if the page already exists
-  const existingPage = await getPageByParam({
-    userId: saver.userId,
-    url: articleToSave.url,
-  })
+
   // save state
   articleToSave.archivedAt =
     input.state === ArticleSavingRequestStatus.Archived ? new Date() : null
@@ -117,28 +130,8 @@ export const savePage = async (
     ? await createLabels(ctx, input.labels)
     : undefined
 
-  if (existingPage) {
-    pageId = existingPage.id
-    slug = existingPage.slug
-    if (
-      !(await updatePage(
-        existingPage.id,
-        {
-          // update the page with the new content
-          ...articleToSave,
-          id: pageId, // we don't want to update the id
-          slug, // we don't want to update the slug
-          createdAt: existingPage.createdAt, // we don't want to update the createdAt
-        },
-        ctx
-      ))
-    ) {
-      return {
-        errorCodes: [SaveErrorCode.Unknown],
-        message: 'Failed to update existing page',
-      }
-    }
-  } else if (shouldParseInBackend(input)) {
+  // always parse in backend if the url is in the force puppeteer list
+  if (shouldParseInBackend(input)) {
     try {
       await createPageSaveRequest({
         userId: saver.userId,
@@ -155,14 +148,42 @@ export const savePage = async (
       }
     }
   } else {
-    const newPageId = await createPage(articleToSave, ctx)
-    if (!newPageId) {
-      return {
-        errorCodes: [SaveErrorCode.Unknown],
-        message: 'Failed to create new page',
+    // check if the page already exists
+    const existingPage = await getPageByParam({
+      userId: saver.userId,
+      url: articleToSave.url,
+    })
+    if (existingPage) {
+      pageId = existingPage.id
+      slug = existingPage.slug
+      if (
+        !(await updatePage(
+          existingPage.id,
+          {
+            // update the page with the new content
+            ...articleToSave,
+            id: pageId, // we don't want to update the id
+            slug, // we don't want to update the slug
+            createdAt: existingPage.createdAt, // we don't want to update the createdAt
+          },
+          ctx
+        ))
+      ) {
+        return {
+          errorCodes: [SaveErrorCode.Unknown],
+          message: 'Failed to update existing page',
+        }
       }
+    } else {
+      const newPageId = await createPage(articleToSave, ctx)
+      if (!newPageId) {
+        return {
+          errorCodes: [SaveErrorCode.Unknown],
+          message: 'Failed to create new page',
+        }
+      }
+      pageId = newPageId
     }
-    pageId = newPageId
   }
 
   // create a task to update thumbnail and pre-cache all images
@@ -248,10 +269,7 @@ export const parsedContentToPage = ({
       parsedContent?.siteName ||
       url,
     author: parsedContent?.byline ?? undefined,
-    url: normalizeUrl(canonicalUrl || url, {
-      stripHash: true,
-      stripWWW: false,
-    }),
+    url: cleanUrl(canonicalUrl || url),
     pageType,
     hash: uploadFileHash || stringToHash(parsedContent?.content || url),
     image: parsedContent?.previewImage ?? undefined,