Skip to content

Commit 7426d3d

Browse files
committed
fix(connectors): address review findings on listing and hashing
- microsoft-excel: `fetchWorksheets` read only the first Graph page and never followed `@odata.nextLink`. A workbook with more sheets than fit in one page dropped the remainder from the listing without setting `listingCapped`, so the sync engine reconciled those documents away as deleted. The walk now pages, bounded by MAX_WORKSHEETS, and only follows a nextLink that stays on the Graph origin, since the link is server-supplied and carries the token. - google-slides: the listing `contentHash` covered only the file id and modified time, so toggling the speaker-notes option left every stored hash matching and no presentation was ever re-hydrated with the new scope. The setting is now part of the hash, in the single shared stub builder so the list and hydrate paths stay identical. - mintlify: `pathPrefix` filtered with a bare `startsWith`, so a prefix of `/guides` also matched a sibling like `/guides-archive`. It now shares the `/`-boundary rule `withinBasePath` already used, extracted as `isUnderPath`.
1 parent 903c94e commit 7426d3d

3 files changed

Lines changed: 63 additions & 23 deletions

File tree

apps/sim/connectors/google-slides/google-slides.ts

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -218,15 +218,20 @@ function shouldIncludeSpeakerNotes(sourceConfig: Record<string, unknown>): boole
218218
* Creates a lightweight stub from a Drive file entry. Content is deferred
219219
* and only fetched via getDocument for new or changed documents.
220220
*/
221-
function fileToStub(file: DriveFile): ExternalDocument {
221+
function fileToStub(file: DriveFile, includeSpeakerNotes: boolean): ExternalDocument {
222222
return {
223223
externalId: file.id,
224224
title: file.name || 'Untitled',
225225
content: '',
226226
contentDeferred: true,
227227
mimeType: 'text/plain',
228228
sourceUrl: file.webViewLink || `https://docs.google.com/presentation/d/${file.id}/edit`,
229-
contentHash: `gslides:${file.id}:${file.modifiedTime ?? ''}`,
229+
/**
230+
* The speaker-notes setting selects what the rendered content contains, so it
231+
* belongs in the hash. Without it, toggling the option leaves every stored
232+
* hash matching and no presentation is ever re-hydrated with the new scope.
233+
*/
234+
contentHash: `gslides:${file.id}:${file.modifiedTime ?? ''}:${includeSpeakerNotes ? 'n1' : 'n0'}`,
230235
metadata: {
231236
modifiedTime: file.modifiedTime,
232237
createdTime: file.createdTime,
@@ -314,7 +319,8 @@ export const googleSlidesConnector: ConnectorConfig = {
314319
const maxDocs = sourceConfig.maxDocs ? Number(sourceConfig.maxDocs) : 0
315320
const previouslyFetched = (syncContext?.totalDocsFetched as number) ?? 0
316321

317-
let documents = files.map(fileToStub)
322+
const includeSpeakerNotes = shouldIncludeSpeakerNotes(sourceConfig)
323+
let documents = files.map((file) => fileToStub(file, includeSpeakerNotes))
318324
let slicedSome = false
319325
if (maxDocs > 0) {
320326
const remaining = maxDocs - previouslyFetched
@@ -376,11 +382,8 @@ export const googleSlidesConnector: ConnectorConfig = {
376382
if (file.trashed) return null
377383
if (file.mimeType !== PRESENTATION_MIME_TYPE) return null
378384

379-
const content = await fetchPresentationContent(
380-
accessToken,
381-
file.id,
382-
shouldIncludeSpeakerNotes(sourceConfig)
383-
)
385+
const includeSpeakerNotes = shouldIncludeSpeakerNotes(sourceConfig)
386+
const content = await fetchPresentationContent(accessToken, file.id, includeSpeakerNotes)
384387

385388
/**
386389
* An image-only deck carries no extractable text. Surfacing it as a skipped
@@ -389,10 +392,15 @@ export const googleSlidesConnector: ConnectorConfig = {
389392
* presentation would simply be missing and re-fetched on every sync.
390393
*/
391394
if (!content.trim()) {
392-
return { ...fileToStub(file), content: '', contentDeferred: false, skippedReason: NO_TEXT }
395+
return {
396+
...fileToStub(file, includeSpeakerNotes),
397+
content: '',
398+
contentDeferred: false,
399+
skippedReason: NO_TEXT,
400+
}
393401
}
394402

395-
return { ...fileToStub(file), content, contentDeferred: false }
403+
return { ...fileToStub(file, includeSpeakerNotes), content, contentDeferred: false }
396404
},
397405

398406
validateConfig: async (

apps/sim/connectors/microsoft-excel/microsoft-excel.ts

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,13 @@ const CONTENT_FORMAT_VERSION = 'v2'
3939
*/
4040
const MAX_WORKSHEETS = 500
4141

42+
/**
43+
* Origin every Graph response must stay on. `@odata.nextLink` is server-supplied
44+
* and carries the bearer token when followed, so a link pointing anywhere else is
45+
* dropped rather than requested.
46+
*/
47+
const GRAPH_API_BASE = 'https://graph.microsoft.com/'
48+
4249
/** Maximum rows read from a single worksheet's used range. */
4350
const MAX_ROWS = 5000
4451

@@ -82,6 +89,7 @@ interface Worksheet {
8289

8390
interface WorksheetListResponse {
8491
value?: Worksheet[]
92+
'@odata.nextLink'?: string
8593
}
8694

8795
interface WorkbookItem {
@@ -243,18 +251,33 @@ async function fetchWorkbookItem(
243251

244252
/** Lists the workbook's worksheets in tab order. */
245253
async function fetchWorksheets(accessToken: string, basePath: string): Promise<Worksheet[]> {
246-
const response = await fetchWithRetry(
247-
`${basePath}/workbook/worksheets?$select=id,name,position,visibility&$orderby=position`,
248-
{
254+
const worksheets: Worksheet[] = []
255+
let url: string | undefined =
256+
`${basePath}/workbook/worksheets?$select=id,name,position,visibility&$orderby=position`
257+
258+
/**
259+
* Graph paginates collection responses, so a workbook with more sheets than fit
260+
* in one page must follow `@odata.nextLink`. Reading only the first page would
261+
* drop the remainder from the listing without setting `listingCapped`, and the
262+
* sync engine would then reconcile those documents away as deleted. The walk is
263+
* bounded by `MAX_WORKSHEETS`, whose truncation the caller does flag.
264+
*/
265+
while (url && worksheets.length <= MAX_WORKSHEETS) {
266+
const response = await fetchWithRetry(url, {
249267
method: 'GET',
250268
headers: { Authorization: `Bearer ${accessToken}`, Accept: 'application/json' },
251-
}
252-
)
269+
})
270+
271+
if (!response.ok) await graphError(response, 'Failed to list worksheets')
253272

254-
if (!response.ok) await graphError(response, 'Failed to list worksheets')
273+
const data = (await response.json()) as WorksheetListResponse
274+
worksheets.push(...(data.value ?? []))
275+
276+
const next = data['@odata.nextLink']
277+
url = next && next.startsWith(GRAPH_API_BASE) ? next : undefined
278+
}
255279

256-
const data = (await response.json()) as WorksheetListResponse
257-
return data.value ?? []
280+
return worksheets
258281
}
259282

260283
/**

apps/sim/connectors/mintlify/mintlify.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -323,9 +323,18 @@ async function discoverFromSitemap(
323323
*/
324324
function withinBasePath(pages: MintlifyPageLink[], site: MintlifySite): MintlifyPageLink[] {
325325
if (!site.basePath) return pages
326-
return pages.filter(
327-
(page) => page.path === site.basePath || page.path.startsWith(`${site.basePath}/`)
328-
)
326+
return pages.filter((page) => isUnderPath(page.path, site.basePath))
327+
}
328+
329+
/**
330+
* Whether `path` is `prefix` itself or sits beneath it.
331+
*
332+
* A bare `startsWith` would also match a sibling whose name merely begins with
333+
* the prefix — `/guides` would capture `/guides-archive` — so the boundary `/`
334+
* is required.
335+
*/
336+
function isUnderPath(path: string, prefix: string): boolean {
337+
return path === prefix || path.startsWith(`${prefix}/`)
329338
}
330339

331340
/**
@@ -469,7 +478,7 @@ export const mintlifyConnector: ConnectorConfig = {
469478
}
470479

471480
const filtered = pathPrefix
472-
? discovered.filter((page) => page.path.startsWith(pathPrefix))
481+
? discovered.filter((page) => isUnderPath(page.path, pathPrefix))
473482
: discovered
474483

475484
if (filtered.length > maxPages && syncContext) {
@@ -591,7 +600,7 @@ export const mintlifyConnector: ConnectorConfig = {
591600
}
592601

593602
const pathPrefix = resolvePathPrefix(sourceConfig.pathPrefix)
594-
if (pathPrefix && !pages.some((page) => page.path.startsWith(pathPrefix))) {
603+
if (pathPrefix && !pages.some((page) => isUnderPath(page.path, pathPrefix))) {
595604
return { valid: false, error: `No pages match the path prefix "${pathPrefix}"` }
596605
}
597606

0 commit comments

Comments
 (0)