Added 6 new actions to support advanced cell formatting, data validation, and protection #18995
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ion, and protection
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Collaborator
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
Collaborator
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
Contributor
WalkthroughAdds six new Google Sheets action modules for conditional formatting, protected ranges, data validation, and cell merging. Each module provides metadata, configuration properties, and a run method that constructs and executes a corresponding Google Sheets API batchUpdate request. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
components/google_sheets/actions/add-conditional-format-rule/add-conditional-format-rule.mjs(1 hunks)components/google_sheets/actions/add-protected-range/add-protected-range.mjs(1 hunks)components/google_sheets/actions/delete-conditional-format-rule/delete-conditional-format-rule.mjs(1 hunks)components/google_sheets/actions/merge-cells/merge-cells.mjs(1 hunks)components/google_sheets/actions/set-data-validation/set-data-validation.mjs(1 hunks)components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
components/google_sheets/actions/set-data-validation/set-data-validation.mjs (3)
components/google_sheets/actions/add-protected-range/add-protected-range.mjs (1)
request(89-115)components/google_sheets/actions/merge-cells/merge-cells.mjs (1)
request(67-85)components/google_sheets/google_sheets.app.mjs (4)
startRow(142-142)endRow(143-145)startCol(138-138)endCol(139-141)
components/google_sheets/actions/delete-conditional-format-rule/delete-conditional-format-rule.mjs (2)
components/google_sheets/actions/add-conditional-format-rule/add-conditional-format-rule.mjs (1)
request(477-489)components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs (1)
request(481-495)
components/google_sheets/actions/add-conditional-format-rule/add-conditional-format-rule.mjs (2)
components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs (2)
rule(312-320)protoToCssColor(321-347)components/google_sheets/google_sheets.app.mjs (4)
startRow(142-142)endRow(143-145)startCol(138-138)endCol(139-141)
components/google_sheets/actions/merge-cells/merge-cells.mjs (3)
components/google_sheets/actions/add-protected-range/add-protected-range.mjs (1)
request(89-115)components/google_sheets/actions/set-data-validation/set-data-validation.mjs (1)
request(94-121)components/google_sheets/google_sheets.app.mjs (4)
startRow(142-142)endRow(143-145)startCol(138-138)endCol(139-141)
components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs (3)
components/google_sheets/actions/add-conditional-format-rule/add-conditional-format-rule.mjs (3)
rule(307-315)protoToCssColor(317-343)request(477-489)components/google_sheets/google_sheets.app.mjs (4)
startRow(142-142)endRow(143-145)startCol(138-138)endCol(139-141)components/google_sheets/actions/delete-conditional-format-rule/delete-conditional-format-rule.mjs (1)
request(47-59)
components/google_sheets/actions/add-protected-range/add-protected-range.mjs (1)
components/google_sheets/google_sheets.app.mjs (4)
startRow(142-142)endRow(143-145)startCol(138-138)endCol(139-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (2)
components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs (2)
3-15: LGTM!The RGB to CSS hex color conversion logic is correct.
481-497: LGTM!The API request structure correctly follows the Google Sheets
updateConditionalFormatRuleRequestformat.
Comment on lines
+300
to
+488
| const { | ||
| startCol, | ||
| endCol, | ||
| startRow, | ||
| endRow, | ||
| } = this.props.googleSheets._parseRangeString(`${this.props.worksheetId}!${this.props.range}`); | ||
|
|
||
| const rule = { | ||
| range: { | ||
| sheetId: this.props.worksheetId, | ||
| startRowIndex: startRow, | ||
| endRowIndex: endRow, | ||
| startColumnIndex: startCol.charCodeAt(0) - 65, | ||
| endColumnIndex: endCol.charCodeAt(0) - 64, | ||
| }, | ||
| }; | ||
|
|
||
| const protoToCssColor = (rgbColor) => { | ||
| var redFrac = rgbColor.red || 0.0; | ||
| var greenFrac = rgbColor.green || 0.0; | ||
| var blueFrac = rgbColor.blue || 0.0; | ||
| var red = Math.floor(redFrac * 255); | ||
| var green = Math.floor(greenFrac * 255); | ||
| var blue = Math.floor(blueFrac * 255); | ||
|
|
||
| if (!("alpha" in rgbColor)) { | ||
| return rgbToCssColor(red, green, blue); | ||
| } | ||
|
|
||
| var alphaFrac = rgbColor.alpha.value || 0.0; | ||
| var rgbParams = [ | ||
| red, | ||
| green, | ||
| blue, | ||
| ].join(","); | ||
| return [ | ||
| "rgba(", | ||
| rgbParams, | ||
| ",", | ||
| alphaFrac, | ||
| ")", | ||
| ] | ||
| .join(""); | ||
| }; | ||
|
|
||
| this.props.formattingType === "GRADIENT_RULE" ? | ||
| rule.gradientRule = { | ||
| minpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MIN", | ||
| }, | ||
| }, | ||
| midpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MID", | ||
| }, | ||
| }, | ||
| maxpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MAX", | ||
| }, | ||
| }, | ||
| } : | ||
| rule.booleanRule = { | ||
| condition: { | ||
| type: this.props.conditionType, | ||
| values: this.props.conditionValues?.map((v) => ({ | ||
| userEnteredValue: v, | ||
| })) || [], | ||
| }, | ||
| format: { | ||
| cellFormat: { | ||
| numberFormat: { | ||
| type: this.props.numberFormatType, | ||
| }, | ||
| backgroundColorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| borders: { | ||
| top: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| bottom: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| left: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| right: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| padding: { | ||
| top: this.props.topPadding, | ||
| right: this.props.rightPadding, | ||
| bottom: this.props.bottomPadding, | ||
| left: this.props.leftPadding, | ||
| }, | ||
| horizontalAlignment: this.props.horizontalAlign, | ||
| verticalAlignment: this.props.verticalAlign, | ||
| wrapStrategy: this.props.wrapStrategy, | ||
| textDirection: this.props.textDirection, | ||
| textFormat: { | ||
| ...this.props.textFormat, | ||
| foregroundColorStyle: { | ||
| rgbColor: this.props.rgbColor, | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| fontFamily: this.props.fontFamily, | ||
| fontSize: this.props.fontSize, | ||
| bold: this.props.bold, | ||
| italic: this.props.italic, | ||
| underline: this.props.underline, | ||
| strikethrough: this.props.strikethrough, | ||
| link: { | ||
| url: this.props.url, | ||
| }, | ||
| }, | ||
| hyperlinkDisplayType: this.props.hyperlinkDisplayType, | ||
| textRotation: { | ||
| angle: 0, | ||
| vertical: false, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| const request = { | ||
| spreadsheetId: this.props.sheetId, | ||
| requestBody: { | ||
| requests: [ | ||
| { | ||
| addConditionalFormatRuleRequest: { | ||
| rule, | ||
| index: this.props.index, | ||
| }, | ||
| }, | ||
| ], | ||
| }, |
Contributor
There was a problem hiding this comment.
Request payload will 400 due to range math and invalid color payloads.
Several blockers here:
- The GridRange indices reuse
_parseRangeStringoutput, so multi-letter columns are reduced to the first letter and single-cell ranges sendendRowIndex === startRowIndex, creating an empty range.(developers.google.com) protoToCssColor()returns CSS strings, butColorStyle.rgbColormust be a{red, green, blue[, alpha]}object; giving it a string violates the schema.(developers.google.com)- Gradient rules should set
minpoint,midpoint,maxpointdirectly toInterpolationPointobjects. Nesting aninterpolationPointproperty and feeding strings intocolorStyle.rgbColordoes not match the API.(developers.google.com) protoToCssColor(this.props.rgbColor)will throw whenrgbColorisn’t provided (it’s optional), becausergbColoris dereferenced inside the helper.
Please (a) fix the column/row conversions so the GridRange matches the Sheets contract, (b) hand the API proper Color/ColorStyle objects with numeric channels, (c) build the gradient rule using the documented InterpolationPoint shape, and (d) guard against absent rgbColor before converting.(developers.google.com)
Comment on lines
+81
to
+117
| async run() { | ||
| const { | ||
| startCol, | ||
| endCol, | ||
| startRow, | ||
| endRow, | ||
| } = this.props.googleSheets._parseRangeString(`${this.props.worksheetId}!${this.props.range}`); | ||
|
|
||
| const request = { | ||
| spreadsheetId: this.props.sheetId, | ||
| requestBody: { | ||
| requests: [ | ||
| { | ||
| addProtectedRangeRequest: { | ||
| protectedRange: { | ||
| protectedRangeId: this.props.protectedRangeId, | ||
| range: { | ||
| sheetId: this.props.worksheetId, | ||
| startRowIndex: startRow, | ||
| endRowIndex: endRow, | ||
| startColumnIndex: startCol.charCodeAt(0) - 65, | ||
| endColumnIndex: endCol.charCodeAt(0) - 64, | ||
| }, | ||
| description: this.props.description, | ||
| warningOnly: this.props.warningOnly, | ||
| requestingUserCanEdit: this.props.requestingUserCanEdit, | ||
| editors: { | ||
| users: this.props.protectors || [], | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| return await this.props.googleSheets.batchUpdate(request); | ||
| }, |
Contributor
There was a problem hiding this comment.
Range math is off for multi-column and single-cell inputs.
The code still assumes one-letter columns and copies _parseRangeString’s zero-based end row unchanged. Any protected range beyond column Z resolves to column A, and protecting a single cell sends endRowIndex === startRowIndex, which Sheets treats as an empty range.(developers.google.com)
Please:
- Convert the full column string (e.g. “AA”) to its numeric index instead of using
charCodeAt(0). - Increment
endRowIndex/endColumnIndexwhen the parsed range has no explicit lower/right boundary so the exclusive end index matches the API contract.(developers.google.com)
🤖 Prompt for AI Agents
components/google_sheets/actions/add-protected-range/add-protected-range.mjs
lines 81-117: the current code uses charCodeAt(0) (one-letter only) for columns
and leaves end indices non-exclusive, causing multi-letter columns to wrap and
single-cell ranges to become empty. Replace the single-char logic with a
function that converts full column strings (e.g. "A", "Z", "AA") to a zero-based
column index (A->0, B->1, Z->25, AA->26), use that for startColumnIndex, and set
endColumnIndex to columnIndex(endCol) + 1 (exclusive). Also make endRowIndex
exclusive by using endRow + 1 (and keep startRowIndex as startRow); ensure you
handle missing endCol/endRow consistently so a single-cell range becomes
start..start+1.
Comment on lines
+59
to
+86
| async run() { | ||
| const { | ||
| startCol, | ||
| endCol, | ||
| startRow, | ||
| endRow, | ||
| } = this.props.googleSheets._parseRangeString(`${this.props.worksheetId}!${this.props.range}`); | ||
|
|
||
| const request = { | ||
| spreadsheetId: this.props.sheetId, | ||
| requestBody: { | ||
| requests: [ | ||
| { | ||
| mergeCellsRequest: { | ||
| range: { | ||
| sheetId: this.props.worksheetId, | ||
| startRowIndex: startRow, | ||
| endRowIndex: endRow, | ||
| startColumnIndex: startCol.charCodeAt(0) - 65, | ||
| endColumnIndex: endCol.charCodeAt(0) - 64, | ||
| }, | ||
| mergeType: this.props.mergeType, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| return await this.props.googleSheets.batchUpdate(request); |
Contributor
There was a problem hiding this comment.
Correct the computed GridRange before merging.
charCodeAt(0) - 65 only works for columns A–Z; “AA5:AB10” collapses to columns A–B. Also, for single cells (B2), endRowIndex and endColumnIndex equal the start indexes, producing an empty range, so the merge request fails. The Sheets API expects zero-based inclusive start and exclusive end indexes for GridRange.(developers.google.com)
Please convert full column labels (base‑26) and ensure end*Index is one past the last row/column when the user supplies only the top-left coordinate.(developers.google.com)
🤖 Prompt for AI Agents
In components/google_sheets/actions/merge-cells/merge-cells.mjs around lines
59–86, the GridRange calculation is incorrect: convert full column labels from
base‑26 (e.g., "AA" -> 26) to a zero‑based index rather than using
charCodeAt(0)-65, and ensure the endRowIndex and endColumnIndex are exclusive
(one past the last index). Fix by implementing a columnLabelToIndex(label) that
computes zero‑based index for multi‑letter labels, set startRowIndex = startRow
- 1, endRowIndex = (endRow || startRow) - 1 + 1 (or simpler: (endRow ||
startRow)), and set startColumnIndex = columnLabelToIndex(startCol),
endColumnIndex = columnLabelToIndex(endCol) + 1; plug those values into the
mergeCellsRequest so the API receives zero‑based inclusive start and exclusive
end indexes.
Comment on lines
+86
to
+121
| async run() { | ||
| const { | ||
| startCol, | ||
| endCol, | ||
| startRow, | ||
| endRow, | ||
| } = this.props.googleSheets._parseRangeString(`${this.props.worksheetId}!${this.props.range}`); | ||
|
|
||
| const request = { | ||
| spreadsheetId: this.props.sheetId, | ||
| requestBody: { | ||
| requests: [ | ||
| { | ||
| setDataValidation: { | ||
| range: { | ||
| sheetId: this.props.worksheetId, | ||
| startRowIndex: startRow, | ||
| endRowIndex: endRow, | ||
| startColumnIndex: startCol.charCodeAt(0) - 65, | ||
| endColumnIndex: endCol.charCodeAt(0) - 64, | ||
| }, | ||
| rule: { | ||
| condition: { | ||
| type: this.props.validationType, | ||
| values: this.props.validationValues?.map((v) => ({ | ||
| userEnteredValue: v, | ||
| })) || [], | ||
| }, | ||
| showCustomUi: true, | ||
| strict: true, | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
Fix GridRange indexes and column conversion.
_parseRangeString returns column labels like “AA” and zero-based row indices, but this request truncates the column to the first letter and reuses the raw end row index. As soon as a user targets a multi-letter column (e.g. “AA10”), charCodeAt(0) - 65 will point to column 0, and single-cell ranges end up with endRowIndex === startRowIndex, producing an empty range. Grid ranges must keep the full column label and set end*Index one past the last row/column.(developers.google.com)
Please convert A1 columns to numbers (e.g. base‑26) and add 1 when the A1 range omits the bottom/right bounds before calling batchUpdate.(developers.google.com)
🤖 Prompt for AI Agents
In components/google_sheets/actions/set-data-validation/set-data-validation.mjs
around lines 86 to 121, the GridRange construction currently uses only the first
character of column labels and treats end indices as inclusive; change it to
convert full A1 column labels (e.g., "AA") to zero-based column numbers using
base-26 conversion for startColumnIndex and endColumnIndex, and ensure
endColumnIndex and endRowIndex are set to one past the last index (end
exclusive). Also handle omitted bottom/right bounds by adding +1 to the end
indices so single-cell ranges produce a non-empty exclusive end, and keep
startRowIndex/startColumnIndex zero-based as returned by _parseRangeString.
Ensure the column conversion replaces charCodeAt(0)-65 logic and that end
indices are computed from the converted column/row numbers +1 before sending the
batchUpdate request.
| googleSheets, | ||
| "range", | ||
| ], | ||
| description: "The range of cells to protect (e.g., `A1:A10`)", |
Contributor
There was a problem hiding this comment.
Fix the description to match the action.
The description mentions "protect" which is incorrect for a conditional formatting action.
Apply this diff:
- description: "The range of cells to protect (e.g., `A1:A10`)",
+ description: "The range of cells to apply conditional formatting (e.g., `A1:A10`)",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| description: "The range of cells to protect (e.g., `A1:A10`)", | |
| description: "The range of cells to apply conditional formatting (e.g., `A1:A10`)", |
🤖 Prompt for AI Agents
In
components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs
around line 59, the parameter description incorrectly references "protect"
instead of the conditional formatting action; update the description to say
something like: "The range of cells the conditional formatting rule applies to
(e.g., A1:A10)" so it correctly describes the range affected by the conditional
formatting rule.
Comment on lines
+349
to
+479
| this.props.formattingType === "GRADIENT_RULE" ? | ||
| rule.gradientRule = { | ||
| minpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MIN", | ||
| }, | ||
| }, | ||
| midpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MID", | ||
| }, | ||
| }, | ||
| maxpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MAX", | ||
| }, | ||
| }, | ||
| } : | ||
| rule.booleanRule = { | ||
| condition: { | ||
| type: this.props.conditionType, | ||
| values: this.props.conditionValues?.map((v) => ({ | ||
| userEnteredValue: v, | ||
| })) || [], | ||
| }, | ||
| format: { | ||
| cellFormat: { | ||
| numberFormat: { | ||
| type: this.props.numberFormatType, | ||
| }, | ||
| backgroundColorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| borders: { | ||
| top: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| bottom: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| left: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| right: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| padding: { | ||
| top: this.props.topPadding, | ||
| right: this.props.rightPadding, | ||
| bottom: this.props.bottomPadding, | ||
| left: this.props.leftPadding, | ||
| }, | ||
| horizontalAlignment: this.props.horizontalAlign, | ||
| verticalAlignment: this.props.verticalAlign, | ||
| wrapStrategy: this.props.wrapStrategy, | ||
| textDirection: this.props.textDirection, | ||
| textFormat: { | ||
| ...this.textFormat, | ||
| foregroundColorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| fontFamily: this.props.fontFamily, | ||
| fontSize: this.props.fontSize, | ||
| bold: this.props.bold, | ||
| italic: this.props.italic, | ||
| underline: this.props.underline, | ||
| strikethrough: this.props.strikethrough, | ||
| link: { | ||
| url: this.props.url, | ||
| }, | ||
| }, | ||
| hyperlinkDisplayType: this.props.hyperlinkDisplayType, | ||
| textRotation: { | ||
| angle: 0, | ||
| vertical: false, | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor the 130-line ternary operator for readability.
The ternary operator spanning lines 349-479 is extremely difficult to read and maintain. Consider refactoring to use if-else statements or separate helper functions.
Apply this pattern:
- this.props.formattingType === "GRADIENT_RULE" ?
- rule.gradientRule = {
- ...
- } :
- rule.booleanRule = {
- ...
- };
+ if (this.props.formattingType === "GRADIENT_RULE") {
+ rule.gradientRule = {
+ ...
+ };
+ } else {
+ rule.booleanRule = {
+ ...
+ };
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs
lines 349-479: the giant ternary assigning either rule.gradientRule or
rule.booleanRule is hard to read—replace it with a clear if/else (or two small
helper functions) that construct the gradient rule and boolean rule objects
separately and then assign them to the rule variable to preserve existing
behavior; specifically, extract the repeated color/format/border/textFormat
building into helper functions buildGradientRule(props) and
buildBooleanRule(props) (or inline an if (this.props.formattingType ===
"GRADIENT_RULE") { rule.gradientRule = buildGradientRule(this.props); } else {
rule.booleanRule = buildBooleanRule(this.props); }), ensure all property names
and nested structures remain identical, and run tests to confirm no behavior
change.
Comment on lines
+349
to
+387
| this.props.formattingType === "GRADIENT_RULE" ? | ||
| rule.gradientRule = { | ||
| minpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MIN", | ||
| }, | ||
| }, | ||
| midpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MID", | ||
| }, | ||
| }, | ||
| maxpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MAX", | ||
| }, | ||
| }, | ||
| } : |
Contributor
There was a problem hiding this comment.
Critical: Handle undefined rgbColor to prevent runtime errors.
protoToCssColor is called with this.props.rgbColor at lines 354, 366, and 378, but rgbColor is optional. If undefined, this will cause a runtime error since protoToCssColor expects an object with red, green, and blue properties.
Additionally, using the same rgbColor for all three interpolation points (min, mid, max) defeats the purpose of a gradient. Each point should have its own color configuration.
Consider these changes:
- Add separate color props for min/mid/max points
- Add null checks before calling
protoToCssColor:
colorStyle: {
- rgbColor: protoToCssColor(this.props.rgbColor),
+ rgbColor: this.props.minRgbColor ? protoToCssColor(this.props.minRgbColor) : undefined,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs
around lines 349 to 387, the code calls protoToCssColor(this.props.rgbColor) for
min/mid/max but rgbColor is optional and using the same color defeats gradient
semantics; add null/undefined checks before calling protoToCssColor and use
separate props for each interpolation point (e.g. this.props.minRgbColor /
midRgbColor / maxRgbColor) or fall back to a safe default color object when a
specific prop is missing, then pass the validated/fallback color to
protoToCssColor for each interpolationPoint so no runtime error occurs and the
gradient can use distinct colors.
Comment on lines
+359
to
+384
| type: this.props.InterpolationPointType, | ||
| value: "MIN", | ||
| }, | ||
| }, | ||
| midpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MID", | ||
| }, | ||
| }, | ||
| maxpoint: { | ||
| interpolationPoint: { | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| type: this.props.InterpolationPointType, | ||
| value: "MAX", |
Contributor
There was a problem hiding this comment.
Design issue: Gradient points should have distinct configurations.
All three interpolation points (min, mid, max) use the same interpolationPointType prop and hardcoded string values ("MIN", "MID", "MAX"). For a proper gradient:
- Each point should have its own interpolation type configuration
- The
valuefield should be user-configurable or dynamically calculated, not hardcoded strings
Consider adding separate props for each interpolation point's type and value, or make the gradient configuration accept an array of point configurations.
🤖 Prompt for AI Agents
In
components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs
around lines 359 to 384, the min/mid/max gradient blocks all reuse
this.props.InterpolationPointType and hardcoded "MIN"/"MID"/"MAX" values;
replace this with distinct, configurable interpolation types and values for each
point. Add new props (or accept a gradientPoints array) such as
minInterpolationType, midInterpolationType, maxInterpolationType and
minValue/midValue/maxValue (or an array of {type,value,colorStyle}) and wire the
blocks to use those props instead of the shared prop and hardcoded strings;
ensure colorStyle construction remains the same but reads from each point's
config so each gradient point can be independently typed and have a
runtime-configurable value.
Comment on lines
+388
to
+479
| rule.booleanRule = { | ||
| condition: { | ||
| type: this.props.conditionType, | ||
| values: this.props.conditionValues?.map((v) => ({ | ||
| userEnteredValue: v, | ||
| })) || [], | ||
| }, | ||
| format: { | ||
| cellFormat: { | ||
| numberFormat: { | ||
| type: this.props.numberFormatType, | ||
| }, | ||
| backgroundColorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| borders: { | ||
| top: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| bottom: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| left: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| right: { | ||
| style: this.props.borderStyle, | ||
| colorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| padding: { | ||
| top: this.props.topPadding, | ||
| right: this.props.rightPadding, | ||
| bottom: this.props.bottomPadding, | ||
| left: this.props.leftPadding, | ||
| }, | ||
| horizontalAlignment: this.props.horizontalAlign, | ||
| verticalAlignment: this.props.verticalAlign, | ||
| wrapStrategy: this.props.wrapStrategy, | ||
| textDirection: this.props.textDirection, | ||
| textFormat: { | ||
| ...this.textFormat, | ||
| foregroundColorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| fontFamily: this.props.fontFamily, | ||
| fontSize: this.props.fontSize, | ||
| bold: this.props.bold, | ||
| italic: this.props.italic, | ||
| underline: this.props.underline, | ||
| strikethrough: this.props.strikethrough, | ||
| link: { | ||
| url: this.props.url, | ||
| }, | ||
| }, | ||
| hyperlinkDisplayType: this.props.hyperlinkDisplayType, | ||
| textRotation: { | ||
| angle: 0, | ||
| vertical: false, | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
Critical: Handle undefined rgbColor in boolean rule.
protoToCssColor is called with this.props.rgbColor at lines 401, 410, 419, 428, 437, and 457, but rgbColor is optional. This will cause runtime errors when rgbColor is undefined.
Add null checks before calling protoToCssColor:
backgroundColorStyle: {
- rgbColor: protoToCssColor(this.props.rgbColor),
+ rgbColor: this.props.rgbColor ? protoToCssColor(this.props.rgbColor) : undefined,
themeColor: {Apply similar changes at lines 410, 419, 428, 437, and 457.
Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+454
to
+471
| textFormat: { | ||
| ...this.textFormat, | ||
| foregroundColorStyle: { | ||
| rgbColor: protoToCssColor(this.props.rgbColor), | ||
| themeColor: { | ||
| type: this.props.themeColorType, | ||
| }, | ||
| }, | ||
| fontFamily: this.props.fontFamily, | ||
| fontSize: this.props.fontSize, | ||
| bold: this.props.bold, | ||
| italic: this.props.italic, | ||
| underline: this.props.underline, | ||
| strikethrough: this.props.strikethrough, | ||
| link: { | ||
| url: this.props.url, | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
Critical: Fix reference to textFormat prop.
Line 455 references this.textFormat but it should be this.props.textFormat. This will cause a reference error.
Apply this diff:
textFormat: {
- ...this.textFormat,
+ ...this.props.textFormat,
foregroundColorStyle: {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| textFormat: { | |
| ...this.textFormat, | |
| foregroundColorStyle: { | |
| rgbColor: protoToCssColor(this.props.rgbColor), | |
| themeColor: { | |
| type: this.props.themeColorType, | |
| }, | |
| }, | |
| fontFamily: this.props.fontFamily, | |
| fontSize: this.props.fontSize, | |
| bold: this.props.bold, | |
| italic: this.props.italic, | |
| underline: this.props.underline, | |
| strikethrough: this.props.strikethrough, | |
| link: { | |
| url: this.props.url, | |
| }, | |
| }, | |
| textFormat: { | |
| ...this.props.textFormat, | |
| foregroundColorStyle: { | |
| rgbColor: protoToCssColor(this.props.rgbColor), | |
| themeColor: { | |
| type: this.props.themeColorType, | |
| }, | |
| }, | |
| fontFamily: this.props.fontFamily, | |
| fontSize: this.props.fontSize, | |
| bold: this.props.bold, | |
| italic: this.props.italic, | |
| underline: this.props.underline, | |
| strikethrough: this.props.strikethrough, | |
| link: { | |
| url: this.props.url, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In
components/google_sheets/actions/update-conditional-format-rule/update-conditional-format-rule.mjs
around lines 454 to 471, the spread is using this.textFormat which is undefined;
change the reference to this.props.textFormat so the existing textFormat object
comes from props (i.e., replace this.textFormat with this.props.textFormat) and
keep the rest of the merged properties intact.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHY
Summary by CodeRabbit