feat(sqllab): Improved query status indicator bar #36936
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 · |
Code Review Agent Run #ebadebActionable Suggestions - 0Additional Suggestions - 2
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 AI Code Review powered by |
justinpark
requested review from
michael-s-molina,
mistercrunch and
rusackas
and removed request for
rusackas
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| ); | ||
| const prevStepRef = useRef<number>(0); | ||
| const progress = | ||
| query.progress > 0 ? parseInt(query.progress.toFixed(0), 10) : undefined; |
There was a problem hiding this comment.
Suggestion: Calls toFixed on query.progress without ensuring it's a number; if query.progress is undefined this will throw. Guard the access and use a safe rounding method when query.progress is a number. [type error]
Severity Level: Minor
| query.progress > 0 ? parseInt(query.progress.toFixed(0), 10) : undefined; | |
| typeof query.progress === 'number' && query.progress > 0 | |
| ? Math.round(query.progress) | |
| : undefined; |
Why it matters? ⭐
If query.progress can be undefined (or a non-number), calling toFixed could throw or generate TypeScript errors. Guarding with a typeof check and using Math.round is clearer and safer for both runtime and type-checking. The current conditional (query.progress > 0) prevents the true-branch from executing when progress is undefined at runtime, but an explicit typeof check is more robust and explicit for maintainers and the type system.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/QueryStatusBar/index.tsx
**Line:** 157:157
**Comment:**
*Type Error: Calls `toFixed` on `query.progress` without ensuring it's a number; if `query.progress` is undefined this will throw. Guard the access and use a safe rounding method when `query.progress` is a number.
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.| const prevStepRef = useRef<number>(0); | ||
| const progress = | ||
| query.progress > 0 ? parseInt(query.progress.toFixed(0), 10) : undefined; | ||
| const { progress_text: progressText } = query.extra; |
There was a problem hiding this comment.
Suggestion: Destructuring query.extra without checking it can throw if extra is undefined; access progress_text via optional chaining and provide a safe default instead of directly destructuring from query.extra. [null pointer]
Severity Level: Minor
| const { progress_text: progressText } = query.extra; | |
| const progressText = query.extra?.progress_text ?? ''; |
Why it matters? ⭐
The current code destructures query.extra directly which will throw if query.extra is ever undefined at runtime.
Switching to optional chaining is a small, safe defensive change that prevents a potential crash and matches how the value is later used with a fallback (progressText ?? ''). This also avoids TypeScript complaints if extra is typed as optional.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/QueryStatusBar/index.tsx
**Line:** 158:158
**Comment:**
*Null Pointer: Destructuring `query.extra` without checking it can throw if `extra` is undefined; access `progress_text` via optional chaining and provide a safe default instead of directly destructuring from `query.extra`.
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.| const latestQuery = useSelector( | ||
| ({ sqlLab: { queries } }: SqlLabRootState) => queries[latestQueryId || ''], | ||
| const queries = useSelector( | ||
| ({ sqlLab: { queries } }: SqlLabRootState) => queries, |
There was a problem hiding this comment.
Suggestion: Indexing into queries can throw if queries is undefined (e.g. not yet loaded in state). Ensure the selector returns a default empty object or guard the lookup so queries[latestQueryId] is not evaluated on undefined. [null pointer]
Severity Level: Minor
| ({ sqlLab: { queries } }: SqlLabRootState) => queries, | |
| ({ sqlLab: { queries } }: SqlLabRootState) => queries ?? {}, |
Why it matters? ⭐
This is a valid defensive improvement. If the Redux slice guarantees queries is always an object, it's redundant — but that's an assumption. Returning queries ?? {} from the selector prevents a potential runtime TypeError when the state is temporarily undefined (e.g. during hydration). The proposed change is small, correct, and safe.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SouthPane/Results.tsx
**Line:** 57:57
**Comment:**
*Null Pointer: Indexing into `queries` can throw if `queries` is undefined (e.g. not yet loaded in state). Ensure the selector returns a default empty object or guard the lookup so `queries[latestQueryId]` is not evaluated on undefined.
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.| <ResultSet | ||
| search | ||
| queryId={latestQuery.id} | ||
| database={databases[latestQuery.dbId]} |
There was a problem hiding this comment.
Suggestion: Accessing databases[latestQuery.dbId] can throw if databases is undefined; guard the access so the prop passed to ResultSet is undefined when databases is missing, preventing a runtime exception. [null pointer]
Severity Level: Minor
| database={databases[latestQuery.dbId]} | |
| database={databases ? databases[latestQuery.dbId] : undefined} |
Why it matters? ⭐
Also a reasonable defensive change. If databases can be undefined transiently, indexing into it will throw. Guarding the access (or using optional chaining) is harmless and prevents a crash. If the app invariant guarantees databases exists, it's unnecessary but not harmful.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SouthPane/Results.tsx
**Line:** 97:97
**Comment:**
*Null Pointer: Accessing `databases[latestQuery.dbId]` can throw if `databases` is undefined; guard the access so the prop passed to `ResultSet` is undefined when `databases` is missing, preventing a runtime exception.
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. |
Code Review Agent Run #66dee5Actionable Suggestions - 0Additional Suggestions - 2
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 AI Code Review powered by |
Code Review Agent Run #7fcee6Actionable Suggestions - 0Review 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 AI Code Review powered by |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #51a851Actionable Suggestions - 0Review 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 AI Code Review powered by |
|
cc: @kasiazjc for design feedback |
1 participant
SUMMARY
The existing query status indicator makes it difficult to predict the progress, so this commit introduces a new progress status that shows which stage is currently in progress. This change will allow users to more accurately anticipate the query execution process and wait accordingly.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
before--query-state.mov
After:
improved_query_state.mov
progress_query_state.mov
failed_query.mov
TESTING INSTRUCTIONS
specs are added
ADDITIONAL INFORMATION