Updating file metadata and download actions for sharepoint#19820
Updating file metadata and download actions for sharepoint#19820dannyroosevelt merged 27 commits intomasterfrom
Conversation
Prototypes — not ready to ship
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Microsoft Graph and SharePoint REST integrations, shared file-picker utilities, new SharePoint actions/sources (download-files, retrieve-file-metadata, updated-file-instant), ConfigureFilePicker UI enhancements, constants/utilities, package bumps, and many minor version increments and trailing-newline fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Picker as ConfigureFilePicker
participant SharePoint as SharePoint App
participant Graph as Microsoft Graph
User->>Picker: Open picker, search/select items
Picker->>SharePoint: resolveWrappedArrayValues / resolveWrappedValue
SharePoint->>Graph: graphRequest (get drive/item metadata)
Graph-->>SharePoint: metadata (incl. lastModifiedDateTime, childCount)
SharePoint-->>Picker: enriched items
User->>Picker: Trigger "Download Files" action
Picker->>SharePoint: filePickerMethods.fetchFileMetadata(files, includeDownloadUrl=true)
SharePoint->>Graph: graphRequest (get download URLs)
Graph-->>SharePoint: files with downloadUrl
SharePoint-->>Picker: processResults(response)
Picker-->>User: return formatted result
sequenceDiagram
participant Source as updated-file-instant
participant Graph as Microsoft Graph
participant Store as Datastore
participant User
Source->>Graph: createSubscription(resource, notificationUrl, clientState)
Graph-->>Source: subscriptionId + expirationDateTime
Source->>Store: save subscription metadata + monitoredFileIds + deltaLink
Webhook->>Source: validation request (validationToken)
Source-->>Webhook: echo validationToken
Webhook->>Source: notification (clientState + resource data)
Source->>Store: validate clientState
Source->>Graph: getDriveDelta / getDriveItem for changed files
Graph-->>Source: changed file metadata (ensure downloadUrl)
Source->>Store: update deltaLink
Source-->>User: emit events for updated files
loop periodic
Source->>Graph: updateSubscription (renew)
Graph-->>Source: success or error (handle 404/401/403)
end
Source->>Graph: deleteSubscription(subscriptionId)
Source->>Store: clear subscription state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
search, select / deselect all, surfacing additional metadata
Resolved conflicts by: - Keeping master's version bumps across all action/source files - Merging both HEAD's new methods (ACLs, groups, delta tracking) and master's methods in sharepoint.app.mjs - Combining constants from both branches in constants.mjs - Merging utility functions from both branches in utils.mjs - Accepting master's pnpm-lock.yaml - Resolving file rename/modification conflicts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/sharepoint/actions/update-item/update-item.mjs (1)
44-59: 🧹 Nitpick | 🔵 TrivialOptional: Consider mapping SharePoint column types to appropriate prop types.
The
additionalPropsmethod currently types all dynamically generated column properties as"string"(line 53), regardless of the actual SharePoint column type. SharePoint supports various column types (number, boolean, date, choice, lookup, etc.) that may benefit from more specific type annotations for better validation and developer experience.Note: This is existing code and not directly related to the version bump in this PR.
🤖 Fix all issues with AI agents
In `@components/sharepoint/actions/get-file-permissions/get-file-permissions.mjs`:
- Around line 487-492: The loop currently assigns the same usersArray (from
usersWithAccess) and expansionDebug to every result item, which is misleading;
instead, remove per-item assignment of usersWithAccess and _expansionDebug in
the for..of loop (referencing usersWithAccess, usersArray, results,
expansionDebug) and attach usersWithAccess (as an array) and _expansionDebug to
the top-level response object returned by the function; alternatively, if you
must keep per-item fields, add a clear comment above the assignment stating this
is a global list for all files, but preferred fix is to delete the per-item
assignments and include usersWithAccess: Array.from(usersWithAccess.values())
and _expansionDebug in the final return payload.
In `@components/sharepoint/actions/list-site-members/list-site-members.mjs`:
- Around line 470-547: The loop over sharePointSiteGroups in
list-site-members.mjs (see sharePointSiteGroups, getSharePointSiteGroupMembers,
groupsFromPermissions, usersFromPermissions) can call the API once per group and
cause latency/throttling; modify the code to make expansion optional via a new
flag/prop (e.g., expandSharePointGroups) and add a configurable cap (e.g.,
maxSharePointGroupsToExpand) so you only process the first N groups, and replace
the sequential awaits with a bounded-parallel approach (Promise.all with a
concurrency limiter or a simple worker pool) to batch calls to
getSharePointSiteGroupMembers and still collect results into
groupsFromPermissions and usersFromPermissions while preserving error handling
per-group.
- Around line 344-401: The helper expandSharePointGroup defined inside run()
mutates outer-scope usersFromPermissions and the group object, so extract it
into a standalone function (e.g., exportable async function
expandSharePointGroup) that takes explicit parameters (group, site,
sharePointSiteGroups, $ or context) and returns a result object containing
{usersToAdd: [...], updatedGroup: {...}, success: boolean, memberCount?,
error?}; in run() call this function, merge the returned usersToAdd into
usersFromPermissions and replace or assign group properties from updatedGroup
instead of letting the helper mutate closure variables, and handle errors by
setting group.type/expandError based on the returned error flag.
In `@components/sharepoint/actions/list-users/list-users.mjs`:
- Around line 215-217: The OData filter construction in the ListUsers logic uses
this.searchQuery directly (the code pushing into filters with
startswith(displayName,'...')), which breaks on single quotes and risks OData
injection; fix by sanitizing/escaping single quotes in this.searchQuery before
interpolation (e.g., replace each ' with '' per OData escaping) or introduce a
small helper like escapeODataString and use that when building the filter so
filters.push(`startswith(displayName,'${escapeODataString(this.searchQuery)}')`)
is used instead of interpolating raw input.
In `@components/sharepoint/common/constants.mjs`:
- Around line 6-17: The WEBHOOK_SUBSCRIPTION_EXPIRATION_TIME_MILLISECONDS
constant is set to 30 days which exceeds Microsoft Graph's max (42,300 minutes);
change WEBHOOK_SUBSCRIPTION_EXPIRATION_TIME_MILLISECONDS to use 29 days in
milliseconds instead (29 * 24 * 60 * 60 * 1000) and keep
WEBHOOK_SUBSCRIPTION_RENEWAL_SECONDS computed from that constant
((WEBHOOK_SUBSCRIPTION_EXPIRATION_TIME_MILLISECONDS * 0.95) / 1000) so renewal
stays at 95% of the new safe expiration.
In `@components/sharepoint/common/file-picker-base.mjs`:
- Around line 193-214: The code currently mutates the wrong object and leaves
duplicate URL properties; after building result (in the map that creates result
from file), if includeDownloadUrl is true assign result.downloadUrl =
file["@microsoft.graph.downloadUrl"] and then delete
result["@microsoft.graph.downloadUrl"], otherwise delete
result["@microsoft.graph.downloadUrl"] and ensure result.downloadUrl is not
present; update the logic around the includeDownloadUrl check (referencing
includeDownloadUrl, result, file and the "@microsoft.graph.downloadUrl"
property) so the final returned object only contains downloadUrl when requested
and never retains the original `@microsoft.graph.downloadUrl` key.
In `@components/sharepoint/common/utils.mjs`:
- Around line 80-85: The getAccessLevel utility currently calls
roles.includes(...) which will throw if roles is null/undefined; update
getAccessLevel to defensively handle missing or non-array input by normalizing
roles at the top (e.g., if (!Array.isArray(roles)) roles = [] or roles = roles
|| []), then run the existing includes checks and return "read" by default;
ensure you only change the getAccessLevel function and preserve its existing
return order ("owner", "write", "read").
In `@components/sharepoint/sharepoint.app.mjs`:
- Around line 470-481: The retry bail list currently checks status via "const
status = error.statusCode || error.response?.status" and bails by calling
"bail(error)" for 401, 403, 404; update that check to also include 400 so Bad
Request is treated as a client error that should not be retried (i.e., add 400
to the array passed to the includes(...) call that triggers bail(error)).
- Around line 426-437: The client() method currently creates a new Microsoft
Graph Client on every call which is wasteful; modify client() to memoize the
Client instance on the class (e.g., this._graphClient) and return the cached
instance when present, while keeping the authProvider.getAccessToken
implementation that calls this._getAccessToken() so token refresh continues to
work; additionally add a clear/reset (or recreate) hook to invalidate
this._graphClient when authentication state changes (logout/token revocation) so
the cached client can be recreated with fresh state.
- Around line 597-604: The getSharePointSiteGroupMembers method currently
interpolates groupId directly into the REST path; validate groupId (e.g.,
confirm it's a finite integer or numeric string convertible to an integer using
Number.isInteger or Number.isFinite after coercion) before building the path,
and if invalid throw a clear error or return a rejected Promise; update
getSharePointSiteGroupMembers to perform this check and only interpolate a
sanitized/parsed integer into the path (refer to the
getSharePointSiteGroupMembers function and the path
`/web/sitegroups/getbyid(${groupId})/users`), ensuring no path injection can
occur.
In `@components/sharepoint/sources/updated-file-instant/updated-file-instant.mjs`:
- Around line 263-267: The code directly invokes the lifecycle hook via
this.hooks.activate.call(this), which can cause double initialization; extract
the subscription creation logic into a new helper method (e.g.,
_createSubscription or createSubscription) that contains the current
subscription setup logic, update activate() to call that new method, and replace
the direct this.hooks.activate.call(this) call in the renewal path with await
this._createSubscription() so both paths reuse the same implementation without
invoking lifecycle hooks directly.
- Around line 344-352: The delta response may omit
file["@microsoft.graph.downloadUrl"], so update the loop that emits changedFiles
to handle missing download URLs: inside the for (const file of changedFiles)
loop (where you call this.$emit and this.generateMeta), check if
file["@microsoft.graph.downloadUrl"] is present and if not, fetch fresh metadata
for that file (e.g., call the Graph files API or the existing metadata helper
used elsewhere) to obtain the download URL before emitting; alternatively
explicitly allow undefined by emitting downloadUrl: undefined and document that
behavior—ensure the fix references the same symbols (changedFiles,
file["@microsoft.graph.downloadUrl"], this.$emit, generateMeta) so the emit
always has a reliable value or a documented undefined.
- Around line 286-297: The current loop over body.value rejects the whole batch
on the first clientState mismatch by calling this.http.respond and returning;
instead, filter out invalid notifications and only reject if no notifications
remain. Replace the for-loop that checks notification.clientState with logic
that builds a validNotifications array (e.g., const validNotifications =
body.value.filter(n => n.clientState === clientState)), log or count invalid
ones for diagnostics, and continue processing validNotifications; only call
this.http.respond(401, ...) if validNotifications.length === 0, otherwise
proceed with normal handling of validNotifications.
In `@packages/connect-react/CHANGELOG.md`:
- Around line 9-14: Update the changelog entry for ConfigureFilePicker to remove
the redundant phrase "SVG graphics" and replace it with a tighter term such as
"SVGs" or "SVG icons" so the bullet reads e.g. "Improved default icons for
folders and files with SVG icons"; keep the rest of the ConfigureFilePicker
bullets unchanged.
In `@packages/connect-react/src/components/ConfigureFilePicker/index.tsx`:
- Around line 62-73: The formatFileSize function can index past the sizes array
for huge byte values; update the sizes array to include larger units (e.g.,
"TB", "PB", "EB") and clamp the computed index i to Math.min(i, sizes.length -
1) before using sizes[i] so it never becomes undefined; modify the logic in
formatFileSize to compute i as Math.floor(Math.log(bytes) / Math.log(k)) then
clamp it and use the clamped index when computing the scaled value and formatted
string.
- Around line 1076-1103: The list item currently uses <li role="button"> with
click/keyboard/mouse handlers which weakens semantics; change the markup so the
<li> is static and contains a real <button> for the interactive element, move
onClick/onKeyDown/onMouseEnter/onMouseLeave/tabIndex and the dynamic background
style to that <button>, keep the style spread (styles.item) and selected logic
(theme.colors.primary25, theme.colors.neutral5) applied to the button, and keep
handleItemClick as the click handler; ensure the <li> no longer has
role="button" or tabIndex so accessibility and screen‑reader semantics are
correct.
- Around line 434-463: The selectedItems initialization (state created by
selectedItems / setSelectedItems in ConfigureFilePicker) currently omits
childCount and lastModifiedDateTime when reconstructing FilePickerItem from
initialConfiguredProps[fileOrFolderProp]; update the parsing logic that builds
items.push in the initializer to include childCount (as number | undefined) and
lastModifiedDateTime (as string | undefined) pulled from parsed (e.g., cast
parsed to Record<string, unknown> and read childCount and lastModifiedDateTime),
so restored FilePickerItem objects contain those fields before being stored in
selectedItems and later returned on confirm.
- Around line 46-56: The FolderIcon and FileIcon SVG components are missing
accessible text; update both components (FolderIcon and FileIcon) to include a
<title> element with a unique, descriptive label (e.g., "Folder" and "File") and
ensure the SVGs have aria-labelledby pointing to that title (or alternatively
add an explicit aria-label) so screen readers can announce them; make sure IDs
used for aria-labelledby are unique if multiple icons may appear on the page.
- Around line 650-657: The unconditional console.log calls in
ConfigureFilePicker that print allItems[0].raw and parsed fields can leak
metadata; replace them by gating behind the component's debug/logging mechanism
(use the existing log helper or a debug flag) so the prints only run when debug
is enabled—locate the block in ConfigureFilePicker where allItems is inspected
(references: allItems, allItems[0].raw, size, childCount, lastModifiedDateTime)
and change the console.log invocations to call the log helper (or wrap them in
if (debug) { ... }) so production builds no longer emit these messages.
components/sharepoint/actions/get-file-permissions/get-file-permissions.mjs
Outdated
Show resolved
Hide resolved
components/sharepoint/actions/list-site-members/list-site-members.mjs
Outdated
Show resolved
Hide resolved
components/sharepoint/actions/list-site-members/list-site-members.mjs
Outdated
Show resolved
Hide resolved
packages/connect-react/src/components/ConfigureFilePicker/index.tsx
Outdated
Show resolved
Hide resolved
This commit addresses all legitimate CodeRabbit review comments and fixes several critical issues identified during code review. Backend fixes (SharePoint components): - Fix critical downloadUrl bug in file-picker-base where property was deleted from wrong object - Fix webhook expiration exceeding Graph API limit (reduced from 30 to 29 days) - Add OData injection prevention in list-users search query - Improve clientState validation to filter invalid notifications instead of rejecting batch - Handle missing downloadUrl in delta API responses by fetching fresh metadata - Add null safety to getAccessLevel utility function - Add Graph client caching to prevent duplicate client instances - Add 400 status to retry bail list for better error handling Frontend fixes (connect-react ConfigureFilePicker): - Add accessibility: SVG title elements and semantic button with keyboard support - Fix formatFileSize overflow by capping max unit index - Preserve metadata (size, childCount, lastModifiedDateTime) when parsing items - Gate debug logging to avoid unnecessary processing - Fix CHANGELOG wording: change "SVG graphics" to "SVGs" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@components/sharepoint/common/file-picker-base.mjs`:
- Around line 119-147: The constructSharePointViewUrl function currently only
recognizes /sites/... URLs and returns null for /teams/...; update the regexes
used to extract libraryUrlPart and siteUrl (the two matches where /sites/ is
hardcoded) to accept either "sites" or "teams" (e.g., use a group like
(?:sites|teams)) so team-site URLs are handled; keep the rest of the logic the
same (use libraryUrlPart, siteUrl, decodeURIComponent(url.pathname), parentPath,
and build the AllItems.aspx URL as before).
In `@packages/connect-react/src/components/ConfigureFilePicker/index.tsx`:
- Around line 985-1000: The search input in ConfigureFilePicker (the <input>
using value={searchQuery} and onChange={(e) => setSearchQuery(e.target.value)})
lacks an accessible name; add an aria-label (for example aria-label="Search
files") to that input element so screen readers can announce its purpose even
after the placeholder disappears.
- Around line 86-94: The formatDate function currently hardcodes "en-US", which
breaks i18n; update the formatDate function to accept an optional locale
parameter (e.g., locale?: string) and pass that to toLocaleDateString (or
default to undefined or navigator.language to use the user's browser locale),
and update any callers of formatDate in this component to pass through a locale
if available; reference the formatDate function in ConfigureFilePicker and
ensure the default behavior preserves existing output when no locale is
provided.
Resolved pnpm-lock.yaml conflict by regenerating lockfile
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/logfire/logfire.app.mjs (1)
1-11: 🧹 Nitpick | 🔵 TrivialRemove unrelated file from SharePoint-focused PR.
This Logfire component is unrelated to the PR's stated objectives (SharePoint file metadata and download actions). The only change is adding a trailing newline, which has no functional impact. Including unrelated files creates noise and makes the PR harder to review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/logfire/logfire.app.mjs` around lines 1 - 11, This file exports a Logfire app object (export default { type: "app", app: "logfire", methods: { authKeys() { ... }}}) which is unrelated to the SharePoint metadata/download PR; remove the file from the change set (undo the addition or exclude it from the commit/branch) so only SharePoint-related changes remain, ensuring no unrelated artifacts like the authKeys method or trailing newline are included in this PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/dataiku/dataiku.app.mjs`:
- Line 11: Remove the accidental trailing newline addition in the Dataiku
component change: revert the unrelated whitespace edit so the file ends exactly
as before (no extra blank line after the closing "};"), ensuring the only
changes in this PR remain the SharePoint-related edits and the Dataiku file is
restored to its original state.
In `@components/voluum_session_auth/voluum_session_auth.app.mjs`:
- Line 11: A trailing newline was introduced in the voluum_session_auth module
(the file ending with the token "};") that is unrelated to this PR; revert that
cosmetic change by restoring the file to its previous state (remove the
incidental formatting-only edit around the final "};") and, if you want to apply
formatter changes, put them in a separate formatting-only PR so this feature PR
stays focused.
In `@components/voluum/voluum.app.mjs`:
- Line 11: This change only adds a trailing newline (the lone diff shows the
closing token "};") in components/voluum/voluum.app.mjs which appears unrelated
to the SharePoint metadata/download work; either remove this file change from
the PR (revert the whitespace-only edit around the "};") or explicitly
justify/describe why the components/voluum/voluum.app.mjs newline was
intentionally included in the PR description and commit message so reviewers
know it’s deliberate.
---
Outside diff comments:
In `@components/logfire/logfire.app.mjs`:
- Around line 1-11: This file exports a Logfire app object (export default {
type: "app", app: "logfire", methods: { authKeys() { ... }}}) which is unrelated
to the SharePoint metadata/download PR; remove the file from the change set
(undo the addition or exclude it from the commit/branch) so only
SharePoint-related changes remain, ensuring no unrelated artifacts like the
authKeys method or trailing newline are included in this PR.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/sharepoint/sources/updated-file-instant/updated-file-instant.mjs`:
- Around line 282-303: The code silently drops all notifications when
this._getSubscription() returns null because clientState becomes undefined; add
a defensive check right after calling this._getSubscription() that if
subscription is null/undefined you log a warning (include function/class context
and that subscription is missing) and immediately respond via this.http.respond
with an appropriate error status (e.g., 400 or 500) and short body, instead of
proceeding to compute validNotifications; reference _getSubscription(),
clientState, validNotifications and this.http.respond so the fix is placed
before the validNotifications filter.
- Around line 235-242: generateMeta currently uses
Date.parse(file.lastModifiedDateTime) which can be NaN for missing/malformed
values; update generateMeta to validate the parsed value (const ts =
Date.parse(file.lastModifiedDateTime)) and if Number.isNaN(ts) replace it with a
safe fallback (e.g., Date.now() or 0) before building the id and ts fields so
you never produce "fileId-NaN" or a NaN timestamp; keep the id format
`${file.id}-${ts}` and optionally add a debug/processLogger.warn mentioning the
malformed lastModifiedDateTime for easier tracing.
---
Duplicate comments:
In `@components/sharepoint/sources/updated-file-instant/updated-file-instant.mjs`:
- Around line 263-267: The code directly invokes the lifecycle hook
this.hooks.activate.call(this), which risks double initialization; refactor the
subscription creation logic out of the activate hook into a new reusable method
(e.g., createOrRecreateSubscription or ensureSubscription) and call that method
here instead, updating the activate implementation to call the new method as
well so both paths reuse the same logic and avoid calling
this.hooks.activate.call(this) directly.
components/sharepoint/sources/updated-file-instant/updated-file-instant.mjs
Show resolved
Hide resolved
components/sharepoint/sources/updated-file-instant/updated-file-instant.mjs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/connect-react/src/components/ConfigureFilePicker/index.tsx`:
- Line 511: Remove the stray empty line causing the ESLint
no-multiple-empty-lines error in the ConfigureFilePicker component file: open
the ConfigureFilePicker component (the React component or its export, e.g.,
ConfigureFilePicker / export default ConfigureFilePicker) and delete the extra
blank line at the reported location so there are no consecutive empty lines;
ensure surrounding code retains a single separating newline where appropriate.
- Around line 401-404: The no-arg useCallback named log causes TypeScript errors
because callers pass arguments; update the log callback returned by useCallback
in ConfigureFilePicker to accept rest parameters (e.g., (...args: any[]) or
(...args: unknown[])) and forward them to the real logger or console when
debug/logging is enabled; ensure the signature change is applied where log is
declared so all call sites (calls at lines referenced in the review) match the
function type and the implementation conditionally no-ops when debug is false.
---
Duplicate comments:
In `@packages/connect-react/src/components/ConfigureFilePicker/index.tsx`:
- Around line 946-961: The search input in ConfigureFilePicker uses only a
placeholder which is not accessible once typing starts; add an aria-label (e.g.,
aria-label="Search files") to the input element to provide a persistent
accessible name for screen readers—locate the JSX input that binds
value={searchQuery} and onChange={(e) => setSearchQuery(e.target.value)} and add
the aria-label attribute to it.
WHY
Summary by CodeRabbit
New Features
Bug Fixes
Chores