fix(DatabaseModal): prevent errors when pasting text into supported database select#35916
Conversation
…d DB values without errors
Code Review Agent Run #af3f5dActionable 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 |
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/features/databases/DatabaseModal/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
Interaction Diagram by BitosequenceDiagram
participant User as User/Component
participant Modal as DatabaseModal<br/>🔄 Updated | ●●○ Medium
participant Select as Select Component
participant SetDb as setDatabaseModel<br/>🔄 Updated | ●●○ Medium
participant Reducer as dbReducer
participant State as Database State
User->>Modal: Render DatabaseModal
User->>Select: Select database from dropdown
Select->>SetDb: onChange(database_name)
SetDb->>SetDb: Filter availableDbs for match
Note over SetDb: Added null check:<br/>if (!selectedDbModel) return
SetDb->>Reducer: setDB(DbSelected action)
Reducer->>State: Update database state
State-->>Modal: Re-render with config
Critical path: User/Component -> DatabaseModal -> Select Component -> setDatabaseModel (MODIFIED) -> dbReducer -> Database State
|
|
🎪 Showtime deployed environment on GHA for d987d1f • Environment: http://35.161.248.34:8080 (admin/admin) |
msyavuz
left a comment
There was a problem hiding this comment.
Looks good! Can we have some tests?
|
🎪 Showtime deployed environment on GHA for f00802a • Environment: http://34.221.22.254:8080 (admin/admin) |
| fireEvent.paste(selectInput, { | ||
| clipboardData: { | ||
| getData: () => 'post', | ||
| }, | ||
| }); | ||
|
|
||
| fireEvent.change(selectInput, { target: { value: 'post' } }); |
There was a problem hiding this comment.
Is this correct? Do we not just trigger onChange with a setup like this?
| fireEvent.paste(selectInput, { | |
| clipboardData: { | |
| getData: () => 'post', | |
| }, | |
| }); | |
| fireEvent.change(selectInput, { target: { value: 'post' } }); | |
| fireEvent.paste(selectInput, { | |
| clipboardData: { | |
| getData: () => 'post', | |
| }, | |
| }); |
There was a problem hiding this comment.
I simplified the test, when text is pasted into the select, it verifies that no exceptions are thrown
SUMMARY
Problem
When creating a new database connection, users must select the database engine/type from a dropdown.
Currently, pasting a value into this dropdown causes an error.
Solution
This PR fixes the issue by filtering pasted text against the supported database values, preventing errors and
adds a test to ensure that pasting text into the supported database select field correctly filters the available options, preventing the regression of the previous paste-related error.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
https://github.com/user-attachments/assets/12822da7-fdde-4577-997e-db3dc248c23e
After
https://github.com/user-attachments/assets/d6f75a6c-7ba5-42e0-956f-6c6bb56b2a77
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION