-
Notifications
You must be signed in to change notification settings - Fork 16.5k
feat: Added ability to run reports on demand #36987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: Added ability to run reports on demand #36987
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| original.owners.map((o: Owner) => o.id).includes(user.userId) || | ||
| isUserAdmin(user); | ||
|
|
||
| const isRunning = original.lastState === AlertState.Working; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Property-name mismatch: the cell code checks original.lastState but the rest of the code and API use last_state (snake_case). This causes the UI disabled state to be incorrect (the button may be enabled/disabled incorrectly). Use the consistent last_state property on original. [logic error]
Severity Level: Minor
| const isRunning = original.lastState === AlertState.Working; | |
| const isRunning = original.last_state === AlertState.Working; |
Why it matters? ⭐
The UI checks original.lastState but the rest of the component (and API data) uses snake_case last_state.
That mismatch will make isRunning always false and the Run Now button not reflect the real running state.
Changing to original.last_state fixes a real logic bug and aligns with the rest of the file where last_state is used.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/AlertReportList/index.tsx
**Line:** 395:395
**Comment:**
*Logic Error: Property-name mismatch: the cell code checks `original.lastState` but the rest of the code and API use `last_state` (snake_case). This causes the UI disabled state to be incorrect (the button may be enabled/disabled incorrectly). Use the consistent `last_state` property on `original`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| }, | ||
| ], | ||
| [canDelete, canEdit, isReportEnabled, toggleActive], | ||
| [canDelete, canEdit, isReportEnabled, toggleActive, refreshData], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Missing dependencies in the useMemo columns dependency array: columns uses runReportNow and user (closed over inside the column cell), but they are not listed in the memo dependencies which can lead to stale closures and UI not reflecting updated values; add them to the dependency array. [possible bug]
Severity Level: Critical 🚨
| [canDelete, canEdit, isReportEnabled, toggleActive, refreshData], | |
| [canDelete, canEdit, isReportEnabled, toggleActive, refreshData, runReportNow, user], |
Why it matters? ⭐
The columns use runReportNow and reference user inside cell renderers. Those values are closed over by useMemo.
If runReportNow or user change and the memo doesn't re-run, the column cell will keep stale references.
Adding them to the dependency array prevents subtle UI bugs. Ideally runReportNow should be stabilized with useCallback to avoid unnecessary recomputations.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/AlertReportList/index.tsx
**Line:** 455:455
**Comment:**
*Possible Bug: Missing dependencies in the `useMemo` columns dependency array: `columns` uses `runReportNow` and `user` (closed over inside the column cell), but they are not listed in the memo dependencies which can lead to stale closures and UI not reflecting updated values; add them to the dependency array.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
CodeAnt AI finished reviewing your PR. |
💡 Enhance Your PR ReviewsWe noticed that 3 feature(s) are not configured for this repository. Enabling these features can help improve your code quality and workflow: 🚦 Quality GatesStatus: Quality Gates are not enabled at the organization level 🎫 Jira Ticket ComplianceStatus: Jira credentials file not found. Please configure Jira integration in your settings ⚙️ Custom RulesStatus: No custom rules configured. Add rules via organization settings or .codeant/review.json in your repository Want to enable these features? Contact your organization admin or check our documentation for setup instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Agent Run #fc69a5
Actionable Suggestions - 3
-
superset-frontend/src/pages/AlertReportList/index.tsx - 1
- Property name mismatch in running check · Line 395-395
-
superset/reports/api.py - 2
- Use of datetime.utcnow() is deprecated · Line 493-493
- Blind exception catch should be specific · Line 495-498
Additional Suggestions - 5
-
superset/reports/api.py - 2
-
Deprecated datetime API · Line 493-493It appears datetime.utcnow() is deprecated as of Python 3.12, which this project supports. Consider switching to datetime.now(timezone.utc) for timezone-aware datetime and future compatibility.
-
Missing type hints · Line 471-471The new run_now method lacks type hints for its pk parameter, which the project's standards require for all new Python code.
-
-
superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx - 1
-
Icon list duplication risk · Line 105-105The addition of PlayCircleOutlined is correctly placed, but the code duplicates the icon list in the import and AntdIcons object. This requires updates in two places, risking inconsistency if one is missed.
-
-
superset-frontend/src/pages/AlertReportList/index.tsx - 2
-
Avoid any type in error handling · Line 258-258The error parameter in the catch block is typed as `any`, which violates the project's TypeScript standards that prohibit `any` types.
-
Inconsistent user messages · Line 397-397The tooltip message 'Currently running, please wait' differs slightly from the toast message 'Report is currently running, please wait', creating inconsistency in user-facing text.
-
Review Details
-
Files reviewed - 3 · Commit Range:
ca5f2bc..42b38f2- superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx
- superset-frontend/src/pages/AlertReportList/index.tsx
- superset/reports/api.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
| original.owners.map((o: Owner) => o.id).includes(user.userId) || | ||
| isUserAdmin(user); | ||
|
|
||
| const isRunning = original.lastState === AlertState.Working; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for isRunning uses original.lastState, but the property is last_state (as defined in AlertObject), causing the condition to always evaluate to false and failing to disable the run now button when a report is already running.
Code suggestion
Check the AI-generated fix before applying
| const isRunning = original.lastState === AlertState.Working; | |
| const isRunning = original.last_state === AlertState.Working; |
Code Review Run #fc69a5
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| # Use a random UUID for the execution id | ||
| execution_id = str(uuid4()) | ||
| # Use current time as scheduled_dttm | ||
| scheduled_dttm = datetime.utcnow() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace datetime.utcnow() with datetime.now(timezone.utc) to use timezone-aware datetime objects, as utcnow() is deprecated in Python 3.12+.
Code suggestion
Check the AI-generated fix before applying
from datetime import datetime
+from datetime import timezone
@@ -493,1 +494,1 @@
- scheduled_dttm = datetime.utcnow()
+ scheduled_dttm = datetime.now(timezone.utc)
Code Review Run #fc69a5
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| AsyncExecuteReportScheduleCommand(execution_id, pk, scheduled_dttm).run() | ||
| return self.response(200, message="Report execution started") | ||
| except Exception as ex: | ||
| return self.response(500, message=f"Failed to run report: {ex}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace the broad except Exception clause with specific exception types (e.g., ReportScheduleNotFoundError, SupersetException) to improve error handling and debugging.
Code suggestion
Check the AI-generated fix before applying
| AsyncExecuteReportScheduleCommand(execution_id, pk, scheduled_dttm).run() | |
| return self.response(200, message="Report execution started") | |
| except Exception as ex: | |
| return self.response(500, message=f"Failed to run report: {ex}") | |
| AsyncExecuteReportScheduleCommand(execution_id, pk, scheduled_dttm).run() | |
| return self.response(200, message="Report execution started") | |
| except (ReportScheduleNotFoundError, ReportScheduleInvalidError, SupersetException) as ex: | |
| return self.response(500, message=f"Failed to run report: {ex}") |
Code Review Run #fc69a5
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
Code Review Agent Run #dc708fActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Added an action button to the reports that allows the user to run the report immediately, regardless of schedule or enabled status.
Link to issue