feat: Execute scripts via right-click on info panel header column #3068
Conversation
|
🚀 Thanks for opening this pull request! |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Warning Rate limit exceeded@mtrezza has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes script validation and execution into src/lib/ScriptUtils.js, adds a ScriptConfirmationModal component, and updates BrowserCell and DataBrowser to use these utilities and a modal-driven confirmation/context-menu flow for running Parse Cloud scripts. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DataBrowser
participant BrowserCell
participant ScriptUtils
participant ScriptConfirmationModal
participant ParseCloud as Parse Cloud
User->>BrowserCell: Right-click panel header (or DataBrowser trigger)
BrowserCell->>DataBrowser: handlePanelHeaderContextMenu(event, objectId)
DataBrowser->>ScriptUtils: getValidScripts(scripts, className, field)
ScriptUtils-->>DataBrowser: { validScripts, validator }
alt validator or script requires confirmation
DataBrowser->>ScriptConfirmationModal: show(selectedScript)
User->>ScriptConfirmationModal: Confirm
ScriptConfirmationModal->>DataBrowser: onConfirm
else no confirmation
DataBrowser->>ScriptUtils: executeScript(script, className, objectId, showNote, onRefresh)
end
ScriptUtils->>ParseCloud: Parse.Cloud.run(cloudFunction, { object: pointer }, { useMasterKey: true })
ParseCloud-->>ScriptUtils: result / error
ScriptUtils->>DataBrowser: showNote(result/error) & onRefresh()
DataBrowser->>User: update UI / display note
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 2
🧹 Nitpick comments (2)
src/lib/ScriptUtils.js (1)
70-73: Useconsole.errorfor error logging.Error messages should use
console.errorinstead ofconsole.logfor proper log level categorization and filtering.} catch (e) { showNote(e.message, true); - console.log(`Could not run ${script.title}: ${e}`); + console.error(`Could not run ${script.title}:`, e); }src/dashboard/Data/Browser/DataBrowser.react.js (1)
1809-1824: Consider clearingselectedScriptstate after execution.The implementation correctly shows the confirmation modal and executes the script. However,
selectedScriptremains in state after the dialog closes. While functionally benign (the modal won't render whenshowScriptConfirmationDialogis false), it's good practice to clear it.onConfirm={() => { executeScript( this.state.selectedScript, this.state.selectedScript.className, this.state.selectedScript.objectId, this.props.showNote, this.props.onRefresh ); - this.setState({ showScriptConfirmationDialog: false }); + this.setState({ showScriptConfirmationDialog: false, selectedScript: null }); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/BrowserCell/BrowserCell.react.js(4 hunks)src/components/ScriptConfirmationModal/ScriptConfirmationModal.react.js(1 hunks)src/dashboard/Data/Browser/DataBrowser.react.js(7 hunks)src/lib/ScriptUtils.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the `confirmExecuteScriptRows` method in `src/dashboard/Data/Browser/Browser.react.js`), individual `setState` calls to update `processedScripts` counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
📚 Learning: 2025-05-27T12:09:47.644Z
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the `confirmExecuteScriptRows` method in `src/dashboard/Data/Browser/Browser.react.js`), individual `setState` calls to update `processedScripts` counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Applied to files:
src/components/ScriptConfirmationModal/ScriptConfirmationModal.react.jssrc/components/BrowserCell/BrowserCell.react.jssrc/dashboard/Data/Browser/DataBrowser.react.js
🧬 Code graph analysis (2)
src/components/BrowserCell/BrowserCell.react.js (2)
src/lib/ScriptUtils.js (2)
getValidScripts(17-48)executeScript(58-74)src/components/ScriptConfirmationModal/ScriptConfirmationModal.react.js (1)
ScriptConfirmationModal(16-36)
src/dashboard/Data/Browser/DataBrowser.react.js (3)
src/context/currentApp.js (2)
CurrentApp(3-3)CurrentApp(3-3)src/lib/ScriptUtils.js (4)
getValidScripts(17-48)validScripts(19-45)validator(18-18)executeScript(58-74)src/components/ScriptConfirmationModal/ScriptConfirmationModal.react.js (1)
ScriptConfirmationModal(16-36)
🪛 Biome (2.1.2)
src/lib/ScriptUtils.js
[error] 35-35: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
⏰ 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). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (8)
src/components/ScriptConfirmationModal/ScriptConfirmationModal.react.js (1)
16-35: LGTM! Clean and reusable confirmation modal component.The component is well-structured with proper null-guarding for the
scriptprop and appropriate modal type mapping based onconfirmationDialogStyle.src/components/BrowserCell/BrowserCell.react.js (4)
12-16: LGTM! Clean import structure for centralized utilities.The imports properly reference the new shared utilities and confirmation modal component, aligning with the refactoring goals.
351-373: LGTM! Good refactoring to use centralized script validation.The context menu options now properly delegate to
getValidScriptsfor filtering valid scripts and obtaining the validator, reducing code duplication with DataBrowser.
378-386: LGTM! Clean delegation to shared executeScript utility.The method now simply delegates to the centralized
executeScriptfunction with all required parameters.
556-566: LGTM! Proper usage of ScriptConfirmationModal.The modal component receives the correct props (
script,onCancel,onConfirm) and the callbacks appropriately handle dialog dismissal and script execution.src/dashboard/Data/Browser/DataBrowser.react.js (3)
13-20: LGTM! Appropriate imports for script execution feature.The imports correctly bring in the context, utilities, and modal component needed for the panel header script execution functionality.
82-83: LGTM! Proper context type declaration.Setting
static contextType = CurrentAppcorrectly enables access to the app context viathis.context.
1704-1707: LGTM! Context menu handler properly wired to panel header.The
onContextMenuhandler correctly prevents default browser behavior and delegates tohandlePanelHeaderContextMenuwith the event and object ID.
|
Thanks for the review @coderabbitai! I've addressed both actionable comments:
Regarding the |
|
Tip For best results, initiate chat on the files or code changes. @mtrezza, thank you for addressing both comments! 🎉 The changes look great:
Your explanation about the Great work on this feature enhancement! 🚀 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/ScriptUtils.js (1)
34-38: Past review comment still applies.The existing review comment about documenting the
eval()usage in the trusted admin context remains valid. Since this has already been flagged, consider adding the suggested JSDoc comment as described in the previous review.
🧹 Nitpick comments (1)
src/lib/ScriptUtils.js (1)
58-74: Consider defensive checks for callback parameters.The function assumes
showNoteandonRefreshcallbacks are always provided. Consider adding null checks before invoking these callbacks to prevent runtime errors if they're undefined.Apply this diff:
export async function executeScript(script, className, objectId, showNote, onRefresh) { try { const object = Parse.Object.extend(className).createWithoutData(objectId); const response = await Parse.Cloud.run( script.cloudCodeFunction, { object: object.toPointer() }, { useMasterKey: true } ); - showNote( - response || `Ran script "${script.title}" on "${className}" object "${object.id}".` - ); - onRefresh(); + showNote?.( + response || `Ran script "${script.title}" on "${className}" object "${object.id}".` + ); + onRefresh?.(); } catch (e) { - showNote(e.message, true); + showNote?.(e.message, true); console.error(`Could not run ${script.title}:`, e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dashboard/Data/Browser/DataBrowser.react.js(7 hunks)src/lib/ScriptUtils.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the `confirmExecuteScriptRows` method in `src/dashboard/Data/Browser/Browser.react.js`), individual `setState` calls to update `processedScripts` counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
📚 Learning: 2025-05-27T12:09:47.644Z
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the `confirmExecuteScriptRows` method in `src/dashboard/Data/Browser/Browser.react.js`), individual `setState` calls to update `processedScripts` counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Applied to files:
src/dashboard/Data/Browser/DataBrowser.react.js
🧬 Code graph analysis (1)
src/dashboard/Data/Browser/DataBrowser.react.js (3)
src/context/currentApp.js (2)
CurrentApp(3-3)CurrentApp(3-3)src/lib/ScriptUtils.js (4)
getValidScripts(17-48)validScripts(19-45)validator(18-18)executeScript(58-74)src/components/ScriptConfirmationModal/ScriptConfirmationModal.react.js (1)
ScriptConfirmationModal(16-36)
🪛 Biome (2.1.2)
src/lib/ScriptUtils.js
[error] 35-35: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (4)
src/dashboard/Data/Browser/DataBrowser.react.js (4)
979-979: Verify hardcoded 'objectId' field is intentional.The field is hardcoded to
'objectId'on line 979, meaning script filtering always validates against the objectId field. For panel header context menus this makes sense since they represent entire objects. Confirm this is the intended behavior and that field-specific script execution (if needed) is handled elsewhere.
986-986: Clarify dependency ononEditSelectedRowprop.The Scripts menu is only added when
this.props.onEditSelectedRowexists (line 986). This dependency seems unrelated to script execution capabilities. Verify whether this check is necessary or if it's a leftover from copy-pasted code.
1704-1707: Context menu integration looks good.The onContextMenu handler correctly prevents default browser behavior and triggers the custom script menu flow with the appropriate objectId.
1809-1824: Modal rendering and state management are correct.The ScriptConfirmationModal is properly integrated with appropriate callbacks for both cancel and confirm actions. State cleanup is handled correctly in both paths.
| handlePanelHeaderContextMenu(event, objectId) { | ||
| const { scripts = [] } = this.context || {}; | ||
| const className = this.props.className; | ||
| const field = 'objectId'; | ||
|
|
||
| const { validScripts, validator } = getValidScripts(scripts, className, field); | ||
|
|
||
| const menuItems = []; | ||
|
|
||
| // Add Scripts menu if there are valid scripts | ||
| if (validScripts.length && this.props.onEditSelectedRow) { | ||
| menuItems.push({ | ||
| text: 'Scripts', | ||
| items: validScripts.map(script => { | ||
| return { | ||
| text: script.title, | ||
| disabled: validator?.(objectId, field) === false, | ||
| callback: () => { | ||
| const selectedScript = { ...script, className, objectId }; | ||
| if (script.showConfirmationDialog) { | ||
| this.setState({ | ||
| showScriptConfirmationDialog: true, | ||
| selectedScript | ||
| }); | ||
| } else { | ||
| executeScript( | ||
| script, | ||
| className, | ||
| objectId, | ||
| this.props.showNote, | ||
| this.props.onRefresh | ||
| ); | ||
| } | ||
| }, | ||
| }; | ||
| }), | ||
| }); | ||
| } | ||
|
|
||
| const { pageX, pageY } = event; | ||
| if (menuItems.length) { | ||
| this.setContextMenu(pageX, pageY, menuItems); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add defensive check for context access.
Line 977 destructures scripts from this.context without verifying that this.context is not null. If the component is used outside a CurrentApp provider, this will throw an error.
Apply this diff:
handlePanelHeaderContextMenu(event, objectId) {
- const { scripts = [] } = this.context || {};
+ const { scripts = [] } = this.context ?? {};
const className = this.props.className;
const field = 'objectId';Note: The ?? operator provides the same protection but is more idiomatic for null/undefined checks.
🤖 Prompt for AI Agents
In src/dashboard/Data/Browser/DataBrowser.react.js around lines 976 to 1019, the
code destructures scripts from this.context without guarding against
this.context being null/undefined; update the destructuring to safely handle a
missing context (e.g. use the nullish coalescing operator to default to an empty
object before destructuring, so scripts defaults to an empty array) and ensure
downstream logic uses that safe value.
|
Thanks for the follow-up review @coderabbitai! Nitpick addressed: Regarding the questions:
|
|
Tip For best results, initiate chat on the files or code changes. @mtrezza, thank you for the thorough clarifications! 🎉 Nitpick: Your explanations make perfect sense:
Excellent work on both addressing the feedback and providing clear rationale for the design decisions! 🚀 🐰✨ ✏️ Learnings added
🧠 Learnings used |
|
All review feedback has been addressed! Summary of changes:
The implementation successfully:
Ready for review! 🚀 |
|
Added comprehensive comment documenting the // SAFETY: eval() is used here on validator strings from trusted admin-controlled
// dashboard configuration only (not user input). These validators are used solely
// for UI validation logic to enable/disable script menu items. This is an accepted
// tradeoff in this trusted admin context. If requirements change, consider replacing
// with Function constructor or a safer expression parser.
validator = eval(currentField.validator);The comment clearly explains:
This addresses the Biome lint warning and provides context for future reviewers. |
# [8.2.0-alpha.6](8.2.0-alpha.5...8.2.0-alpha.6) (2025-12-13) ### Features * Execute scripts via right-click on info panel header column ([#3068](#3068)) ([2983741](2983741))
|
🎉 This change has been released in version 8.2.0-alpha.6 |
2 participants
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.