fix(sqllab): preserve database state on SET_DATABASES#41281
fix(sqllab): preserve database state on SET_DATABASES#41281GagandeepSingh20 wants to merge 6 commits 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}
|
Approving the CI run 🤞 @betodealmeida might be best to take a look here. |
|
| Language | Fuzzy before | Fuzzy after | New |
|---|---|---|---|
ar |
1130 | 1133 | +3 |
it |
1660 | 1662 | +2 |
ko |
1514 | 1516 | +2 |
pt |
1835 | 1837 | +2 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (ar, it, ko, pt):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41281 +/- ##
=======================================
Coverage 64.70% 64.70%
=======================================
Files 2686 2686
Lines 148625 148625
Branches 34298 34298
=======================================
+ Hits 96164 96173 +9
+ Misses 50696 50690 -6
+ Partials 1765 1762 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@rusackas Hi! Just following up on this PR since it's been a few days. I wanted to check if there's anything else I should address on my side, or if there are any remaining concerns about the proposed fix. I'm happy to make any changes if needed. Thanks for taking a look! |
|
@GagandeepSingh20 I think the approach looks OK... I'll take another review pass at it when CI is green, but right now, it needs a rebase to fix the flagged translation problem, and you'll need to get pre-commit to pass. Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
|
@sadpandajoe Thanks for the review! I flattened the regression test by removing the extra I pushed the update to the PR branch, and CI should run again now. |
Code Review Agent Run #2e511dActionable 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 |
|
@sadpandajoe Thanks for the review and approval! I’ll wait for the workflows to be approved/run, and can update the branch if needed. |
GagandeepSingh20
force-pushed
the
fix-sql-lab-database-state
branch
from
a7c6352 to
b0c570b
Compare
| const state = { | ||
| ...initialState, | ||
| databases: { | ||
| [existingDbId]: existingDb, | ||
| }, | ||
| } as any; |
There was a problem hiding this comment.
Suggestion: Replace the any cast on the reducer state with a concrete state type (for example, a properly typed partial or test-specific state interface) to preserve type safety. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The rule forbids new or changed TypeScript code from using any. This test setup explicitly casts the reducer state to any, so the violation is real and present in the final code.
(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:** 46:51
**Comment:**
*Custom Rule: Replace the `any` cast on the reducer state with a concrete state type (for example, a properly typed partial or test-specific state interface) to preserve type safety.
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
Code Review Agent Run #f0887cActionable 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 |
3 participants
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