From f372636d6e9ebc369f60ac28ae0524f7886b9cb8 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 27 Jun 2023 16:47:36 +0800 Subject: [PATCH 1/3] fix: tweet not saved correctly when using share button on iOS --- packages/api/src/services/save_page.ts | 35 +++++++++++++------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/packages/api/src/services/save_page.ts b/packages/api/src/services/save_page.ts index c58ded776..e45fba331 100644 --- a/packages/api/src/services/save_page.ts +++ b/packages/api/src/services/save_page.ts @@ -117,7 +117,24 @@ export const savePage = async ( ? await createLabels(ctx, input.labels) : undefined - if (existingPage) { + // always parse in backend if the url is in the force puppeteer list + if (shouldParseInBackend(input)) { + try { + await createPageSaveRequest({ + userId: saver.userId, + url: articleToSave.url, + pubsub: ctx.pubsub, + articleSavingRequestId: input.clientRequestId, + archivedAt: articleToSave.archivedAt, + labels: articleToSave.labels, + }) + } catch (e) { + return { + errorCodes: [SaveErrorCode.Unknown], + message: 'Failed to create page save request', + } + } + } else if (existingPage) { pageId = existingPage.id slug = existingPage.slug if ( @@ -138,22 +155,6 @@ export const savePage = async ( message: 'Failed to update existing page', } } - } else if (shouldParseInBackend(input)) { - try { - await createPageSaveRequest({ - userId: saver.userId, - url: articleToSave.url, - pubsub: ctx.pubsub, - articleSavingRequestId: input.clientRequestId, - archivedAt: articleToSave.archivedAt, - labels: articleToSave.labels, - }) - } catch (e) { - return { - errorCodes: [SaveErrorCode.Unknown], - message: 'Failed to create page save request', - } - } } else { const newPageId = await createPage(articleToSave, ctx) if (!newPageId) { From bb359d97478b8b0ea932b9c3a56f9b9ce837509e Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 27 Jun 2023 16:58:57 +0800 Subject: [PATCH 2/3] save normalized url --- .../src/services/create_page_save_request.ts | 2 +- packages/api/src/services/save_page.ts | 67 ++++++++++--------- 2 files changed, 36 insertions(+), 33 deletions(-) diff --git a/packages/api/src/services/create_page_save_request.ts b/packages/api/src/services/create_page_save_request.ts index 2734c50e1..9bc040f0d 100644 --- a/packages/api/src/services/create_page_save_request.ts +++ b/packages/api/src/services/create_page_save_request.ts @@ -164,7 +164,7 @@ export const createPageSaveRequest = async ({ })) // enqueue task to parse page await enqueueParseRequest({ - url, + url: normalizedUrl, userId, saveRequestId: page.id, priority, diff --git a/packages/api/src/services/save_page.ts b/packages/api/src/services/save_page.ts index e45fba331..adb3039ad 100644 --- a/packages/api/src/services/save_page.ts +++ b/packages/api/src/services/save_page.ts @@ -104,11 +104,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 @@ -134,36 +130,43 @@ export const savePage = async ( message: 'Failed to create page save request', } } - } else 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', + // 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 From ed8287df1924782078a829dd2d8b8fe180de62e6 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 27 Jun 2023 17:49:31 +0800 Subject: [PATCH 3/3] remove tracking params from tweet url --- packages/api/src/resolvers/article/index.ts | 8 ++---- .../resolvers/article_saving_request/index.ts | 9 ++----- .../src/services/create_page_save_request.ts | 7 ++--- packages/api/src/services/save_email.ts | 7 ++--- packages/api/src/services/save_page.ts | 26 ++++++++++++++----- 5 files changed, 28 insertions(+), 29 deletions(-) diff --git a/packages/api/src/resolvers/article/index.ts b/packages/api/src/resolvers/article/index.ts index 4e4eeb374..417d83146 100644 --- a/packages/api/src/resolvers/article/index.ts +++ b/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, }, diff --git a/packages/api/src/resolvers/article_saving_request/index.ts b/packages/api/src/resolvers/article_saving_request/index.ts index 9f6e40125..197f970dc 100644 --- a/packages/api/src/resolvers/article_saving_request/index.ts +++ b/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, diff --git a/packages/api/src/services/create_page_save_request.ts b/packages/api/src/services/create_page_save_request.ts index 9bc040f0d..20f067f52 100644 --- a/packages/api/src/services/create_page_save_request.ts +++ b/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, diff --git a/packages/api/src/services/save_email.ts b/packages/api/src/services/save_email.ts index 7e49494b0..04999329a 100644 --- a/packages/api/src/services/save_email.ts +++ b/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: diff --git a/packages/api/src/services/save_page.ts b/packages/api/src/services/save_page.ts index adb3039ad..a65a48d5e 100644 --- a/packages/api/src/services/save_page.ts +++ b/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 | undefined) => { const { pathname } = new URL(url) const croppedPathname = decodeURIComponent( @@ -252,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,