Microsoft Excel - Updates and New Components #16369
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis update introduces significant enhancements and refactoring to the Microsoft Excel integration. Several new action and source modules are added, including polling and webhook-based triggers for row additions, cell value changes, and workbook creation or updates. New actions enable adding rows, updating cells, retrieving table rows, and extracting worksheet data. The core app module is refactored for consistent naming and expanded API coverage, with new methods for range and worksheet operations. Existing actions are updated to align with these conventions. Supporting modules for polling and webhook sources are introduced, along with test event payloads for new triggers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action/Source
participant MicrosoftExcelApp
participant MicrosoftGraphAPI
participant DB
User->>Action/Source: Triggers action/source (e.g., add row, poll for new row)
Action/Source->>MicrosoftExcelApp: Calls method (e.g., addRow, getUsedRange)
MicrosoftExcelApp->>MicrosoftGraphAPI: Sends API request
MicrosoftGraphAPI-->>MicrosoftExcelApp: Returns data/response
MicrosoftExcelApp-->>Action/Source: Returns processed data
Action/Source->>DB: (If polling/source) Reads/writes state
Action/Source-->>User: Returns result or emits event
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
components/microsoft_excel/actions/add-row/add-row.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/microsoft_excel/actions/find-row/find-row.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
components/microsoft_excel/actions/update-worksheet-tablerow/update-worksheet-tablerow.mjs (1)
55-72:⚠️ Potential issueGuard against malformed JSON & reuse the shared
parseObjecthelper
JSON.parse(values)will throw and fail the whole step if the user supplies anything that is not valid JSON (e.g. a single‑quoted string, an empty input, accidental trailing comma, etc.).
All other Excel actions have already centralised this concern through the sharedparseObjectutility which returns the original value when parsing is not required and surfaces a friendlier error otherwise. Re‑using it here keeps behaviour consistent across the integration.-import microsoftExcel from "../../microsoft_excel.app.mjs"; +import microsoftExcel from "../../microsoft_excel.app.mjs"; +import { parseObject } from "../../common/utils.mjs"; @@ - data: { - values: JSON.parse(values), - }, + data: { + values: parseObject(values), + },
🧹 Nitpick comments (25)
components/microsoft_excel/microsoft_excel.app.mjs (3)
170-171: Inconsistent leading slash creates double “//” in URL
listWorksheets()builds its path with a leading/, while most other helpers omit it.
Because_makeRequest()prefixesthis._apiUrl() + "/", the final URL becomes
https://graph.microsoft.com/v1.0//me/drive/....Browsers tolerate the double slash, but it’s better to stay consistent and avoid accidental triple slashes when paths are concatenated elsewhere.
- path: `/me/drive/items/${sheetId}/workbook/worksheets`, + path: `me/drive/items/${sheetId}/workbook/worksheets`,
208-214: Handle missingtokenwhen callinggetDeltaIf
tokenis falsy the generated URL ends with?token=undefined, causing Graph to reject the request. Add a guard or default:- path: `${path}/delta?token=${token}`, + path: `${path}/delta${token ? `?token=${encodeURIComponent(token)}` : ""}`,
102-105: SetContent-Typefor non‑GET requests
_getHeaders()returns only theAuthorizationheader. ForPOST,PATCH, andDELETEcalls, Graph expectsContent-Type: application/json. Consider adding it conditionally (or always) to avoid 415 errors.return { "Authorization": `Bearer ${this.$auth.oauth_access_token}`, + "Content-Type": "application/json", };components/microsoft_excel/package.json (1)
15-18: Validate new dependency footprint
json-2-csv@^5.5.9adds ~400 KB installed size and pulls in transitive deps. Double‑check it’s really needed—Node hasJSON.stringifyand there are zero‑dep CSV helpers that may suffice. If it’s required, consider pinning to a patch version (5.5.9) to avoid unexpected breaking changes on minor bumps.components/microsoft_excel/actions/get-spreadsheet/get-spreadsheet.mjs (1)
36-41: Consider clarifying range format in descriptionThe range description is good, but it might be helpful to explicitly mention that the range format follows Excel's standard notation (e.g., column letter followed by row number).
range: { type: "string", label: "Range", - description: "The range within the worksheet to retrieve. E.g. `A1:C4`. If not specified, entire \"usedRange\" will be returned", + description: "The range within the worksheet to retrieve using Excel's standard notation. E.g. `A1:C4` where A and C are column letters and 1 and 4 are row numbers. If not specified, entire \"usedRange\" will be returned", optional: true, },components/microsoft_excel/actions/get-columns/get-columns.mjs (2)
35-39: Consider adding column format validationThe columns prop accepts any string array, but Excel columns follow a specific format (single letters A-Z or combinations like AA, AB, etc.). It might be helpful to add validation or clearer guidance on the expected format.
columns: { type: "string[]", label: "Columns", - description: "An array of column labels to retrieve. E.g. [\"A\", \"C\"]", + description: "An array of column labels to retrieve using Excel column notation (A-Z, AA, AB, etc.). E.g. [\"A\", \"C\"]", + validate: { + type: "array", + items: { + type: "string", + pattern: "^[A-Z]+$", + description: "Must be a valid Excel column reference (A-Z, AA, AB, etc.)" + } + } },
41-57: Consider optimizing for multiple column requestsFor efficiency, consider fetching multiple adjacent columns in a single request rather than making individual API calls for each column. This could significantly reduce API calls when users request many columns.
async run({ $ }) { const { rowCount } = await this.microsoftExcel.getUsedRange({ $, sheetId: this.sheetId, worksheet: this.worksheet, }); const values = {}; + // Group adjacent columns to minimize API calls + const sortedColumns = [...this.columns].sort(); + let currentGroup = []; + let lastColumn = null; + + // Process columns to identify adjacent groups + for (const column of sortedColumns) { + if (lastColumn && this.isAdjacent(lastColumn, column)) { + currentGroup.push(column); + } else { + if (currentGroup.length > 0) { + // Process previous group + await this.fetchColumnGroup($, values, currentGroup, rowCount); + } + currentGroup = [column]; + } + lastColumn = column; + } + + // Process final group + if (currentGroup.length > 0) { + await this.fetchColumnGroup($, values, currentGroup, rowCount); + } - for (const column of this.columns) { - const response = await this.microsoftExcel.getRange({ - $, - sheetId: this.sheetId, - worksheet: this.worksheet, - range: `${this.worksheet}!${column}1:${column}${rowCount}`, - }); - values[column] = response.values.map((v) => v[0]); - } $.export("$summary", "Successfully retrieved column values"); return values; }, + // Helper method to determine if columns are adjacent + isAdjacent(col1, col2) { + // Simple implementation for single-letter columns + if (col1.length === 1 && col2.length === 1) { + return col2.charCodeAt(0) - col1.charCodeAt(0) === 1; + } + // For multi-letter columns, more complex logic would be needed + return false; + }, + // Helper method to fetch a group of columns + async fetchColumnGroup($, values, columns, rowCount) { + if (columns.length === 1) { + // Single column fetch + const column = columns[0]; + const response = await this.microsoftExcel.getRange({ + $, + sheetId: this.sheetId, + worksheet: this.worksheet, + range: `${this.worksheet}!${column}1:${column}${rowCount}`, + }); + values[column] = response.values.map((v) => v[0]); + } else { + // Multi-column fetch + const firstCol = columns[0]; + const lastCol = columns[columns.length - 1]; + const response = await this.microsoftExcel.getRange({ + $, + sheetId: this.sheetId, + worksheet: this.worksheet, + range: `${this.worksheet}!${firstCol}1:${lastCol}${rowCount}`, + }); + // Process multi-column response + for (let i = 0; i < columns.length; i++) { + values[columns[i]] = response.values.map((row) => row[i]); + } + } + }Note: This optimization becomes more valuable when users request multiple columns, especially adjacent ones. The implementation above is a starting point and would need additional refinement for multi-letter columns. Consider adding this optimization in a future version if you observe users frequently requesting multiple columns.
components/microsoft_excel/actions/get-table-rows/get-table-rows.mjs (1)
36-54: Consider clarifying the pagination variable namingThe pagination implementation correctly fetches table rows in batches of 100, but the variable
totalon line 52 might be confusing as it represents the current batch size, not the total number of rows.- let total; + let currentBatchSize; do { const { value } = await this.microsoftExcel.listTableRows({ $, sheetId: this.sheetId, tableId: this.tableId, params, }); rows.push(...value); - total = value.length; + currentBatchSize = value.length; params["$skip"] += params["$top"]; - } while (total); + } while (currentBatchSize);components/microsoft_excel/sources/new-cell-value-changed/new-cell-value-changed.mjs (1)
39-43: Potential redundancy in range specificationThe
rangeparameter already includes the worksheet name (${this.worksheet}!${this.cell}:${this.cell}), while also passingworksheetas a separate parameter.Verify if both parameters are needed or if the range specification alone is sufficient according to the Microsoft Graph API requirements.
components/microsoft_excel/actions/update-cell/update-cell.mjs (1)
40-44: Consider supporting non-string cell valuesThe value prop is defined as a string type, which means numbers, booleans, and other data types will be coerced to strings when updating cells.
Consider supporting different data types for cell values to match Excel's native data handling capabilities:
value: { - type: "string", + type: "any", label: "Value", - description: "The value to enter in the cell", + description: "The value to enter in the cell. Can be text, number, boolean, etc.", },components/microsoft_excel/actions/add-row/add-row.mjs (3)
53-54: Remove unused eslint-disable commentThe eslint-disable comment is unnecessary since the variable is marked as unused via the comment. Consider removing it or using the more modern pattern for unused variables.
- /* eslint-disable no-unused-vars */ - rowStart, + _rowStart, // Prefix with underscore to indicate unused variable
35-39: Consider improved validation for values arrayThe current implementation accepts any array of strings for the row values, but doesn't validate that the input matches the expected format for Excel cells.
Consider adding validation and a more specific example:
values: { type: "string[]", label: "Values", - description: "An array of values for the new row. Each item in the array represents one cell. E.g. `[1, 2, 3]`", + description: "An array of values for the new row. Each item in the array represents one cell in order from left to right. E.g. `[\"John\", \"Doe\", \"[email protected]\"]`", + validate: (values) => { + if (!Array.isArray(values) || !values.length) { + return "Values must be a non-empty array"; + } + return true; + }, },
1-87: Add option to append row at the end instead of inserting with shiftThe current implementation always inserts a row by shifting existing rows down. In some cases, users might want to simply append a row at the end without shifting.
Consider adding an option to control this behavior:
props: { // ... existing props + insertMode: { + type: "string", + label: "Insert Mode", + description: "Choose whether to insert the row by shifting existing rows down, or simply append at the end", + options: [ + { label: "Insert (shift cells down)", value: "insert" }, + { label: "Append at end", value: "append" }, + ], + default: "insert", + }, }, async run({ $ }) { const { address } = await this.microsoftExcel.getUsedRange({ $, sheetId: this.sheetId, worksheet: this.worksheet, }); // get next row range const match = address.match(/^(.+!)?([A-Z]+)(\d+):([A-Z]+)(\d+)$/); if (!match) { throw new Error(`Unable to parse address format: ${address}`); } const [ , sheet = "", colStart, /* eslint-disable no-unused-vars */ rowStart, colEnd, ] = match; const nextRow = parseInt(match[5], 10) + 1; const range = `${sheet}${colStart}${nextRow}:${colEnd}${nextRow}`; + if (this.insertMode === "insert") { // insert range await this.microsoftExcel.insertRange({ $, sheetId: this.sheetId, worksheet: this.worksheet, range, data: { shift: "Down", }, }); + } // update range const response = await this.microsoftExcel.updateRange({ $, sheetId: this.sheetId, worksheet: this.worksheet, range, data: { values: [ this.values, ], }, }); - $.export("$summary", "Successfully added new row"); + $.export("$summary", `Successfully ${this.insertMode === "insert" ? "inserted" : "appended"} new row`); return response; },components/microsoft_excel/sources/new-row-added/new-row-added.mjs (2)
28-46: Improve robustness of row counting logicThe current implementation doesn't handle edge cases where the row count doesn't change but the content does, or when rows are both added and removed.
Consider storing more information about the worksheet state to detect changes more accurately:
async run() { const previousRowCount = this._getRowCount(); const response = await this.microsoftExcel.getUsedRange({ sheetId: this.sheetId, worksheet: this.worksheet, }); const { rowCount } = response; + // Initialize rowCount if this is the first run + if (previousRowCount === undefined) { + this._setRowCount(rowCount); + return; + } this._setRowCount(rowCount); if (!previousRowCount || rowCount <= previousRowCount) { return; } response.numRowsAdded = rowCount - previousRowCount; + + // Add more context to the event + response.previousRowCount = previousRowCount; + response.currentRowCount = rowCount; this.emitEvent(response); },
3-10: Fix typo in descriptionThere's a repeated word "when" in the description that should be fixed.
export default { ...common, key: "microsoft_excel-new-row-added", name: "New Row Added", - description: "Emit new event when when a new row is added to an Excel worksheet", + description: "Emit new event when a new row is added to an Excel worksheet", version: "0.0.1", type: "source", dedupe: "unique",components/microsoft_excel/sources/new-item-created/new-item-created.mjs (1)
40-46: Add error handling for date parsingThe current implementation doesn't handle potential date parsing failures.
generateMeta(item) { + let timestamp; + try { + timestamp = Date.parse(item.createdDateTime); + if (isNaN(timestamp)) { + console.warn(`Invalid date format: ${item.createdDateTime}`); + timestamp = Date.now(); + } + } catch (error) { + console.error(`Error parsing date: ${error.message}`); + timestamp = Date.now(); + } return { id: `${item.id}`, summary: `Item ${item.name} created`, - ts: Date.parse(item.createdDateTime), + ts: timestamp, }; },components/microsoft_excel/actions/find-row/find-row.mjs (1)
70-76: Add support for finding multiple matchesThe current implementation only returns the first match, but users might expect to find all matching rows.
Consider adding an option to return all matches:
+props: { + // ...existing props + findAllMatches: { + type: "boolean", + label: "Find All Matches", + description: "If true, finds all rows that match the value. If false, finds only the first match.", + default: false, + }, +}, async run({ $ }) { // ...existing code - const values = rangeValues.map((v) => v[0]); - const index = values.indexOf(this.value); + // Find matching indices + const values = rangeValues.map((v) => v[0]); + let indices = []; + for (let i = 0; i < values.length; i++) { + if (values[i] === this.value) { + indices.push(i); + if (!this.findAllMatches) break; + } + } - if (index === -1) { + if (indices.length === 0) { $.export("$summary", "No matching rows found"); return values; } - const row = index + 1; - const response = await this.microsoftExcel.getRange({ - $, - sheetId: this.sheetId, - worksheet: this.worksheet, - range: `${this.worksheet}!A${row}:${lastColumn}${row}`, - }); + const results = []; + for (const index of indices) { + const row = index + 1; + const response = await this.microsoftExcel.getRange({ + $, + sheetId: this.sheetId, + worksheet: this.worksheet, + range: `${this.worksheet}!A${row}:${lastColumn}${row}`, + }); + results.push({ row, data: response }); + } - $.export("$summary", `Found value in row ${row}`); - return response; + $.export("$summary", `Found value in ${results.length} row(s)`); + return this.findAllMatches ? results : results[0].data;components/microsoft_excel/actions/update-worksheet-tablerow/update-worksheet-tablerow.mjs (1)
74-75: Clarify success message wording
rowIdis an identifier, not necessarily an “index”. Consider rephrasing to avoid confusing users (especially when the ID is a GUID).-$.export("$summary", `The row with index: ${rowId} was successfully updated!`); +$.export("$summary", `Row ${rowId} was successfully updated.`);components/microsoft_excel/actions/add-a-worksheet-tablerow/add-a-worksheet-tablerow.mjs (2)
51-64: Return value fromadditionalPropswhensheetIdis not setWhen
this.sheetIdis falsy the method exits without returning anything.
The Pipedream runtime toleratesundefined, but for explicitness and to prevent future surprises it’s better to always return an object.if (this.sheetId) { ... return {}; } -} +return {}; }
52-58: Minor resilience improvementThe catch block silently swallows all errors from
listTables.
Consider logging the error (or at least its message) so users can diagnose authentication / permission problems instead of assuming the table lookup failed because the sheet doesn’t contain tables.-} catch { +} catch (err) { + this.$logger.debug(`listTables failed: ${err?.message ?? err}`); props.tableName.hidden = false; return {}; }components/microsoft_excel/sources/new-item-updated/new-item-updated.mjs (1)
14-20: Type‑safety: fallback whenlastModifiedDateTimeis missing
Date.parsereturnsNaNif the field is absent or not ISO‑8601.
That will propagate as thetsvalue and break Pipedream’s deduplication logic.
Safeguard with a fallback toDate.now().-const ts = Date.parse(item.lastModifiedDateTime); +const ts = Date.parse(item.lastModifiedDateTime) || Date.now();components/microsoft_excel/sources/common/base-webhook.mjs (4)
36-38: Unnecessary / misleading argument passed to_getHookId
_getHookIdis defined without parameters (see lines 43‑45), but here it’s invoked as_getHookId("hookId").
Although the extra argument is silently ignored, it’s confusing to readers and suggests a different API.- const id = this._getHookId("hookId"); + const id = this._getHookId();
85-92: Typo incontent-typeheader
text/plan→text/plain.- "content-type": "text/plan", + "content-type": "text/plain",
96-104: Missing guard when no stored delta token exists
this._getDeltaToken()may returnundefined, which could cause Graph to reject the request.
Consider defaulting to"latest"up‑front instead of relying on thecatch.- deltaValues = await this.getDeltaValues(this.path(), this._getDeltaToken()); + const storedToken = this._getDeltaToken() || "latest"; + deltaValues = await this.getDeltaValues(this.path(), storedToken);
109-116: Performance & readability: avoid double traversal and emit asynchronously
- The mime‑type string is repeated—extract to a constant.
forEachignores async completion fromemitEvent. UsingPromise.allconveys intent.- let spreadsheets = value.filter(({ file }) => file?.mimeType === "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"); - spreadsheets = this.filterRelevantSpreadsheets(spreadsheets); + const XLSX_MIME = "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"; + const spreadsheets = this + .filterRelevantSpreadsheets( + value.filter(({ file }) => file?.mimeType === XLSX_MIME), + );- spreadsheets.forEach((item) => this.emitEvent(item)); + await Promise.all( + spreadsheets.map((item) => this.emitEvent(item)), + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
components/microsoft_excel/actions/add-a-worksheet-tablerow/add-a-worksheet-tablerow.mjs(5 hunks)components/microsoft_excel/actions/add-row/add-row.mjs(1 hunks)components/microsoft_excel/actions/find-row/find-row.mjs(1 hunks)components/microsoft_excel/actions/get-columns/get-columns.mjs(1 hunks)components/microsoft_excel/actions/get-spreadsheet/get-spreadsheet.mjs(1 hunks)components/microsoft_excel/actions/get-table-rows/get-table-rows.mjs(1 hunks)components/microsoft_excel/actions/update-cell/update-cell.mjs(1 hunks)components/microsoft_excel/actions/update-worksheet-tablerow/update-worksheet-tablerow.mjs(4 hunks)components/microsoft_excel/microsoft_excel.app.mjs(7 hunks)components/microsoft_excel/package.json(2 hunks)components/microsoft_excel/sources/common/base-polling.mjs(1 hunks)components/microsoft_excel/sources/common/base-webhook.mjs(1 hunks)components/microsoft_excel/sources/common/base.mjs(1 hunks)components/microsoft_excel/sources/new-cell-value-changed/new-cell-value-changed.mjs(1 hunks)components/microsoft_excel/sources/new-item-created/new-item-created.mjs(1 hunks)components/microsoft_excel/sources/new-item-created/test-event.mjs(1 hunks)components/microsoft_excel/sources/new-item-updated/new-item-updated.mjs(1 hunks)components/microsoft_excel/sources/new-item-updated/test-event.mjs(1 hunks)components/microsoft_excel/sources/new-row-added/new-row-added.mjs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
🔇 Additional comments (14)
components/microsoft_excel/sources/common/base-polling.mjs (1)
1-39: Well-structured base polling module with proper dependency chainThis base polling module provides a solid foundation for Excel-related polling sources. The hierarchical props structure (folder → sheet → worksheet) creates a natural configuration flow for users, with each property dynamically dependent on previous selections.
The code follows best practices by:
- Extending the common base module
- Using platform constants for consistency
- Creating clear dependencies between props
components/microsoft_excel/sources/new-item-created/test-event.mjs (1)
1-46: Comprehensive test event with realistic Excel file metadataThe test event provides a well-structured example of a newly created Excel file event with all relevant metadata fields:
- Timestamps for creation and modification
- File details (name, size, MIME type)
- User/application information
- Parent folder references
This will be valuable for testing and demonstrating the expected event format to users.
components/microsoft_excel/actions/get-spreadsheet/get-spreadsheet.mjs (2)
43-55: LGTM! Clean conditional logic for range retrievalThe action correctly handles both cases - retrieving a specific range when provided or falling back to the entire used range. The code is clean and straightforward.
57-61: Helpful CSV conversion for better data usabilityConverting the data to CSV format enhances the action's utility by providing a standardized format that can be easily used in downstream steps.
components/microsoft_excel/actions/get-columns/get-columns.mjs (1)
58-61: LGTM! Clear return value structureThe action provides a well-structured response with column values organized by column label, making it easy for users to process the data in downstream steps.
components/microsoft_excel/actions/get-table-rows/get-table-rows.mjs (2)
1-35: Implementation follows best practices for component structureThe action is well-organized with clear imports, metadata, and prop definitions. The props follow a logical dependency chain (folderId → sheetId → tableId) which provides a good user experience when selecting Excel resources.
56-60: Well-formatted summary message with singular/plural handlingThe summary message is user-friendly and correctly handles singular/plural forms based on the result count.
components/microsoft_excel/sources/new-cell-value-changed/new-cell-value-changed.mjs (3)
1-18: Well-structured source component with appropriate propsThe source component extends the common base polling module and adds a specific cell prop with clear description and labeling. The dedupe setting ensures that only unique changes trigger workflows.
19-35: Effective state management with database methodsThe methods for getting and setting cell values use the database appropriately for state persistence between runs. The
generateMetamethod creates a unique ID for each event with timestamp inclusion.
51-57: Good handling of previous value inclusionThe code only includes the previous value in the event payload when it exists, which is appropriate for handling initial runs where there's no previous value stored.
components/microsoft_excel/sources/new-item-updated/test-event.mjs (1)
1-48: Comprehensive test event with detailed file metadataThe test event provides a complete representation of an Excel file update event with appropriate structure and field types. It includes all relevant metadata fields that would be returned by the Microsoft Graph API.
Note that the
lastModifiedDateTimeis set to April 2025, which aligns with the current date context provided.components/microsoft_excel/actions/update-cell/update-cell.mjs (2)
1-45: Well-structured action with appropriate propsThe action component has clear imports, metadata, and prop definitions. The props follow a logical dependency chain (folderId → sheetId → worksheet) and include well-described cell and value fields for user input.
46-62: Properly structured API call with nested values arrayThe action correctly formats the cell value in the nested array structure required by the Microsoft Graph API and provides a clear summary message after successful update.
components/microsoft_excel/sources/common/base-webhook.mjs (1)
42-54: Persist helpers silently swallow DB write failures
this.db.set/getreturn promises in some Pipedream components; here they’re treated as synchronous.
If that’s the case in this repo, wrap calls withawaitto ensure persistence before proceeding, or document why it’s safe to fire‑and‑forget.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/microsoft_excel/microsoft_excel.app.mjs (1)
56-74:⚠️ Potential issue
tableRowIdprop still does not matchupdateTableRow()signature
propDefinitions.tableRowIdexposes the value under the keytableRowId, but
updateTableRow({ …, rowId })expects a parameter namedrowId.
Unless every action renames the prop manually,undefinedwill be interpolated:…/rows/ItemAt(index=undefined)Consider aligning the names:
- updateTableRow({ - sheetId, tableId, rowId, ...args + updateTableRow({ + sheetId, tableId, tableRowId: rowId, ...args }) {
♻️ Duplicate comments (3)
components/microsoft_excel/microsoft_excel.app.mjs (1)
215-248: Worksheet name and range must be URI‑encoded (issue still unresolved)Earlier reviews already pointed out that worksheet names or ranges like
"My Sheet"or"A1:B2, D4"must be percent‑encoded to avoid400 Invalid Address.
None of the range methods have been updated yet.- path: `me/drive/items/${sheetId}/workbook/worksheets/${worksheet}/range(address='${range}')`, + const encodedSheet = encodeURIComponent(worksheet); + const encodedRange = encodeURIComponent(range); + path: `me/drive/items/${sheetId}/workbook/worksheets/${encodedSheet}/range(address='${encodedRange}')`,Apply the same fix to
getUsedRange,insertRange, andupdateRange.components/microsoft_excel/sources/common/base-webhook.mjs (2)
27-28: Expiration date must be an ISO‑8601 string
moment().add(30, "days")returns a Moment object; Graph expects a string like
"2025‑05‑26T15:04:59Z". Serialising the object will yield{}and the request
will fail withInvalid filter clause.- expirationDateTime: moment().add(30, "days"), + expirationDateTime: moment().add(30, "days").toISOString(),
78-86: Crash when no delta‑ or next‑link is returned
new URL(deltaLink)will throw whendeltaLinkisundefined(empty delta
response is a valid scenario). This issue was flagged before and is still
present.- token: new URL(deltaLink).searchParams.get("token"), + token: deltaLink ? new URL(deltaLink).searchParams.get("token") : null,
🧹 Nitpick comments (3)
components/microsoft_excel/common/utils.mjs (1)
20-28: Validate the argument to prevent silent mis‑behaviour
getColumnLetter()returns an empty string whenindex <= 0or a non‑integer is supplied.
Because the caller will probably concatenate the result into a range address, an empty
string can lead to puzzling400 Invalid Addresserrors from the Graph API.export function getColumnLetter(index) { + if (!Number.isInteger(index) || index <= 0) { + throw new Error("Column index must be a positive integer (1‑based)"); + } let letter = "";components/microsoft_excel/microsoft_excel.app.mjs (1)
149-173: Inconsistent leading slash causes double “//” in the final URL
listWorksheets()prepends/, whereas other methods don’t.
Because_makeRequest()already inserts a/, the final URL becomes:
https://graph.microsoft.com/v1.0//me/drive/items/…Graph tolerates this, but removing the extra slash keeps the codebase consistent.
- path: `/me/drive/items/${sheetId}/workbook/worksheets`, + path: `me/drive/items/${sheetId}/workbook/worksheets`,components/microsoft_excel/sources/common/base-webhook.mjs (1)
37-39:_getHookId()is called with an argument it does not acceptconst id = this._getHookId("hookId");The helper is defined without parameters, so the argument is ignored and the
line is misleading. Simply callthis._getHookId()(or change the helper to
accept a key name).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
components/microsoft_excel/actions/add-row/add-row.mjs(1 hunks)components/microsoft_excel/actions/find-row/find-row.mjs(1 hunks)components/microsoft_excel/actions/get-columns/get-columns.mjs(1 hunks)components/microsoft_excel/actions/get-spreadsheet/get-spreadsheet.mjs(1 hunks)components/microsoft_excel/actions/update-cell/update-cell.mjs(1 hunks)components/microsoft_excel/actions/update-worksheet-tablerow/update-worksheet-tablerow.mjs(4 hunks)components/microsoft_excel/common/utils.mjs(1 hunks)components/microsoft_excel/microsoft_excel.app.mjs(7 hunks)components/microsoft_excel/sources/common/base-webhook.mjs(1 hunks)components/microsoft_excel/sources/new-cell-value-changed/new-cell-value-changed.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/microsoft_excel/actions/find-row/find-row.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- components/microsoft_excel/actions/get-spreadsheet/get-spreadsheet.mjs
- components/microsoft_excel/actions/get-columns/get-columns.mjs
- components/microsoft_excel/actions/add-row/add-row.mjs
- components/microsoft_excel/actions/update-worksheet-tablerow/update-worksheet-tablerow.mjs
- components/microsoft_excel/sources/new-cell-value-changed/new-cell-value-changed.mjs
- components/microsoft_excel/actions/update-cell/update-cell.mjs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (1)
components/microsoft_excel/microsoft_excel.app.mjs (1)
117-125: Path interpolation may fail whentableNamecontains spaces or#
addTableRow()falls back totableNamewhentableIdis not provided, but the
raw table name is injected into the URL. Names with spaces must be wrapped in
single quotes and URI‑encoded per Graph docs:- path: `me/drive/items/${sheetId}/workbook/tables/${tableId || tableName}/rows/add`, + path: `me/drive/items/${sheetId}/workbook/tables/${tableId ?? `'${encodeURIComponent(tableName)}'`}/rows/add`,
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
components/microsoft_excel/sources/common/base-webhook.mjs (5)
27-27: Expiration date must be sent as an ISO string, not a Moment instance
moment().add(30, "days")returns a Moment object.
Microsoft Graph expects an ISO 8601 string (YYYY‑MM‑DDTHH:mm:ssZ). Passing the raw object will serialise to{}and the request will fail withInvalid filter clause.- expirationDateTime: moment().add(30, "days"), + expirationDateTime: moment().add(30, "days").toISOString(),
26-26:resourcepath in the subscription ignoresfolderIdThe subscription always targets
me/drive/rootbut thepath()method respectsfolderId. If the user selected a specific folder, the webhook will miss changes outside the root scope.- resource: "me/drive/root", + resource: this.path(),
67-67: Expiration date must be sent as an ISO string, not a Moment instanceSame issue as in the
activate()hook - the Moment object needs to be converted to an ISO string.- expirationDateTime: moment().add(30, "days"), + expirationDateTime: moment().add(30, "days").toISOString(),
66-66:resourcepath in the subscription ignoresfolderIdSame issue as in the
activate()hook - the resource should use thepath()method to respect the selected folder.- resource: "me/drive/root", + resource: this.path(),
82-86: Potential crash when neither@odata.deltaLinknor@odata.nextLinkis presentIf the first delta call returns only an empty array with no continuation link (which can happen),
deltaLinkwill beundefinedandnew URL(undefined)throws.- const deltaLink = delta["@odata.deltaLink"] || delta["@odata.nextLink"]; - return { - value: delta?.value, - token: new URL(deltaLink).searchParams.get("token"), - }; + const deltaLink = delta["@odata.deltaLink"] || delta["@odata.nextLink"]; + return { + value: delta?.value ?? [], + token: deltaLink ? new URL(deltaLink).searchParams.get("token") : null, + };
🧹 Nitpick comments (2)
components/microsoft_excel/sources/common/base-webhook.mjs (2)
37-37: Unnecessary parameter in_getHookId()callThe
_getHookId()method doesn't accept any parameters, but you're passing "hookId" here.- const id = this._getHookId("hookId"); + const id = this._getHookId();
57-57: Unnecessary parameter in_getHookId()callThe
_getHookId()method doesn't accept any parameters, but you're passing "hookId" here.- await this.microsoftExcel.deleteHook(this._getHookId("hookId")); + await this.microsoftExcel.deleteHook(this._getHookId());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/microsoft_excel/sources/common/base-webhook.mjs(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/microsoft_excel/sources/common/base-webhook.mjs
[error] 105-105: expected , but instead found let
Remove let
(parse)
[error] 105-105: expected , but instead found deltaValues
Remove deltaValues
(parse)
[error] 105-105: expected , but instead found ;
Remove ;
(parse)
[error] 122-122: Illegal return statement outside of a function
(parse)
[error] 126-127: Expected a statement but instead found '},
}'.
Expected a statement here.
(parse)
🪛 GitHub Actions: Pull Request Checks
components/microsoft_excel/sources/common/base-webhook.mjs
[error] 105-105: Parsing error: Unexpected token let
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (1)
components/microsoft_excel/sources/common/base-webhook.mjs (1)
102-103:⚠️ Potential issueSyntax error: Incorrect indentation causing parsing failure
The return statement is improperly indented, causing the parsing error reported in the pipeline failures. It should be aligned with the if statement, not inside its block.
if (query.validationToken) { this.http.respond({ status: 200, body: query.validationToken, headers: { "content-type": "text/plain", }, }); - } - return; + return; }Likely an incorrect or invalid review comment.
Resolves #16252
Summary by CodeRabbit
itemIdtosheetId).