fix(sqllab): preserve database state on SET_DATABASES#41281
fix(sqllab): preserve database state on SET_DATABASES#41281GagandeepSingh20 wants to merge 1 commit into
Conversation
Code Review Agent Run #531c26Actionable 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 |
✅ 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 action = actions.setDatabases([ | ||
| { | ||
| ...incomingDb, | ||
| extra: '{}', | ||
| }, | ||
| ] as any); |
There was a problem hiding this comment.
Suggestion: Remove the any cast on the setDatabases payload and type it using the action creator's expected argument type (or a specific database DTO type) so the test validates real compile-time contracts. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is TypeScript code in a modified test file, and it uses as any on the setDatabases payload. The custom rule explicitly forbids new or modified TypeScript/TSX code from using any, so this is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/reducers/sqlLab.test.ts
**Line:** 50:55
**Comment:**
*Custom Rule: Remove the `any` cast on the `setDatabases` payload and type it using the action creator's expected argument type (or a specific database DTO type) so the test validates real compile-time contracts.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| }, | ||
| ] as any); | ||
|
|
||
| const newState = sqlLabReducer(state as any, action); |
There was a problem hiding this comment.
Suggestion: Avoid casting state to any; declare state with the proper reducer state type so the reducer call remains fully type-checked. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This modified TypeScript test line casts state to any, which directly violates the rule against using any in new or changed TS/TSX code. The suggestion is therefore valid.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/reducers/sqlLab.test.ts
**Line:** 57:57
**Comment:**
*Custom Rule: Avoid casting `state` to `any`; declare `state` with the proper reducer state type so the reducer call remains fully type-checked.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
1 participant
SUMMARY
Fixes an issue where SQL Lab could lose previously loaded database entries when additional database pages were fetched.
The SET_DATABASES reducer currently replaces the entire databases map in Redux state with the latest API response. Since DatabaseSelector loads databases through paginated requests, loading another page can remove database entries that were already present in state.
This change updates the reducer to merge incoming database entries into the existing state instead of replacing the entire map. A regression test has also been added to verify that previously loaded databases are preserved when additional database results are received.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (no UI changes).
TESTING INSTRUCTIONS
Run:
npm run test -- src/SqlLab/reducers/sqlLab.test.tsVerify the reducer test suite passes.
Verify the new regression test confirms that existing database entries remain available after dispatching SET_DATABASES with additional database results.
ADDITIONAL INFORMATION