fix: update dropdown option structure and improve Google Sheets range…#65
Conversation
📝 WalkthroughWalkthroughThis PR refactors Google Sheets integration identifiers across the stack. Tab identification shifts from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/nodes/src/google-sheets/google-sheets.executor.ts (1)
203-208:⚠️ Potential issue | 🟡 MinorAdd validation for
sheetNamebefore constructing the range.If
context.config.sheetNameis undefined or empty, the range will becomeundefined!${range}or!${range}, causing the Google Sheets API to fail with a cryptic error. Consider validating thatsheetNameexists before constructing the range string.🛡️ Proposed fix
async executeReadRows(sheetsService: GoogleSheetsService, context: NodeExecutionContext): Promise<NodeExecutionResult> { try{ + if (!context.config.sheetName) { + return { + success: false, + error: 'Sheet name is required for reading rows' + }; + } const rows = await sheetsService.readRows({ spreadsheetId: context.config.spreadsheetId, range: `${context.config.sheetName}!${context.config.range}` });apps/web/app/lib/nodeConfigs/googleSheet.action.ts (1)
40-48:⚠️ Potential issue | 🟠 MajorOperations
append_rowandupdate_roware not implemented in the executor.The dropdown offers three operations, but the executor (in
google-sheets.executor.tslines 143-152) only handlesread_rows. Selecting "Append Row" or "Update Row" will result in an "unknown operation" error at runtime.Consider either removing these options until implemented, or marking them as disabled/coming soon.
🐛 Option 1: Remove unimplemented options for now
options: [ { label: "Read Rows", id: "read_rows" }, - { label: "Append Row", id: "append_row" }, - { label: "Update Row", id: "update_row" } ],💡 Option 2: Mark as coming soon in labels
options: [ { label: "Read Rows", id: "read_rows" }, - { label: "Append Row", id: "append_row" }, - { label: "Update Row", id: "update_row" } + { label: "Append Row (Coming Soon)", id: "append_row" }, + { label: "Update Row (Coming Soon)", id: "update_row" } ],apps/web/app/workflows/[id]/components/ConfigModal.tsx (1)
175-179:⚠️ Potential issue | 🟡 MinorOperator precedence issue in option value logic.
The expression
opt.value || opt.id !== undefined ? opt.id : opthas unexpected behavior due to operator precedence. The ternary operator? :has lower precedence than||, so it evaluates as:(opt.value || opt.id !== undefined) ? opt.id : optThis means even when
opt.valueis truthy, the result isopt.id, notopt.value. If the intent is to preferopt.valueoveropt.id, add parentheses:🐛 Proposed fix
- {options.map((opt: any) => ( - <option key={opt.value || opt.id || opt} value={opt.value || opt.id !== undefined ? opt.id : opt }> + {options.map((opt: any) => ( + <option key={opt.id ?? opt.value ?? opt} value={opt.id ?? opt.value ?? opt}> {opt.label || opt.name || opt} </option> ))}
🤖 Fix all issues with AI agents
In `@apps/web/app/workflows/`[id]/components/ConfigModal.tsx:
- Around line 167-173: Remove the two debug console.log calls introduced in the
ConfigModal component: delete the console.log("log for options: ", fieldValue)
inside the select onChange handler (where handleFieldChange is invoked) and
remove the JSX expression {console.log(options)} that runs during render; ensure
no other stray console.log statements remain in the component (particularly
around handleFieldChange, fieldValue, and options usage).
| console.log("log for options: ",fieldValue) | ||
| await handleFieldChange(field.name, e.target.value, nodeConfig); | ||
| }} | ||
| className="w-full p-3 border border-gray-900 bg-black text-white rounded-md" | ||
| required={field.required} | ||
| > | ||
| {console.log(options)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug console.log statements before merging.
These debug logging statements should be removed:
- Line 167: logs
fieldValueon every dropdown change - Line 173:
{console.log(options)}inside JSX renders on every component render and returnsundefined
The console.log inside JSX (line 173) is particularly problematic as it executes on every render cycle, not just during development interaction.
🧹 Remove debug logs
<select
value={fieldValue}
onChange={async(e) => {
- console.log("log for options: ",fieldValue)
await handleFieldChange(field.name, e.target.value, nodeConfig);
}}
className="w-full p-3 border border-gray-900 bg-black text-white rounded-md"
required={field.required}
>
- {console.log(options)}
<option value="">Select {field.label.toLowerCase()}</option>📝 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.
| console.log("log for options: ",fieldValue) | |
| await handleFieldChange(field.name, e.target.value, nodeConfig); | |
| }} | |
| className="w-full p-3 border border-gray-900 bg-black text-white rounded-md" | |
| required={field.required} | |
| > | |
| {console.log(options)} | |
| await handleFieldChange(field.name, e.target.value, nodeConfig); | |
| }} | |
| className="w-full p-3 border border-gray-900 bg-black text-white rounded-md" | |
| required={field.required} | |
| > |
🤖 Prompt for AI Agents
In `@apps/web/app/workflows/`[id]/components/ConfigModal.tsx around lines 167 -
173, Remove the two debug console.log calls introduced in the ConfigModal
component: delete the console.log("log for options: ", fieldValue) inside the
select onChange handler (where handleFieldChange is invoked) and remove the JSX
expression {console.log(options)} that runs during render; ensure no other stray
console.log statements remain in the component (particularly around
handleFieldChange, fieldValue, and options usage).
2 participants
… handling
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.