fix: datasource name not reflecting in schema pane #37131
Conversation
WalkthroughThe changes introduce a new selector, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Bug
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11589444561. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (1)
101-104: Consider adding loading state for datasource name
The current implementation might show an empty string briefly while the datasource is being fetched. Consider showing a loading state.
-datasourceName={datasource?.name || ""}
+datasourceName={datasource?.name || datasource === undefined ? "Loading..." : ""}Also applies to: 260-260
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (3 hunks)
🔇 Additional comments (3)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (3)
24-24: LGTM: Clean import addition
The new selector import is properly grouped with related entity selectors.
101-104: LGTM: Proper selector implementation
The selector usage correctly handles the optional chaining for datasource ID, maintaining null safety.
260-260: Verify error handling for undefined datasource name
The fallback to empty string is good, but we should verify the UI behavior when datasource name is undefined.
✅ Verification successful
Fallback handling for undefined datasource name is consistent across codebase
The current implementation using datasource?.name || "" is consistent with other similar usages across the codebase, appearing in multiple components including DataSourceEditor and PluginActionResponse. This pattern represents a standard approach for handling undefined datasource names in the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Schema component usage to verify consistent handling of undefined names
ast-grep --pattern 'Schema $$$datasourceName={$_}$$$'
Length of output: 55
Script:
#!/bin/bash
# Let's check the component usage and error handling patterns
rg -U "datasourceName.*undefined|datasourceName.*\|\|" -A 2
# Also check the component definition to understand expected props
ast-grep --pattern 'interface $_ {
$$$
datasourceName$_
$$$
}'
# Look for similar patterns of handling undefined datasource names
rg "datasource\?\.name.*\|\|" -A 2
Length of output: 1713
|
Deploy-Preview-URL: https://ce-37131.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (1)
Line range hint 101-260: Consider memoizing the datasource selector
The datasource selector result could be memoized to prevent unnecessary re-renders.
+ const memoizedGetDatasource = useCallback(
+ (state) => getDatasource(state, currentActionConfig?.datasource?.id ?? ""),
+ [currentActionConfig?.datasource?.id]
+ );
- const datasource = useSelector((state) =>
- getDatasource(state, currentActionConfig?.datasource?.id ?? ""),
- );
+ const datasource = useSelector(memoizedGetDatasource);📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (3 hunks)
🔇 Additional comments (3)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (3)
24-24: LGTM: Clean import addition
The addition of getDatasource selector import aligns with the PR's objective to fix the datasource name update issue.
101-103: LGTM: Proper selector implementation
Good use of useSelector hook to fetch datasource information reactively. The optional chaining on the datasource ID provides proper null safety.
260-260: Verify error handling for undefined datasource name
While the optional chaining provides null safety, consider the UX when datasource name is undefined.
✅ Verification successful
The previous regex had issues. I've split the search pattern and added proper escaping. This will help us understand how datasource names are handled across the codebase and if there are any established patterns for handling undefined cases.
Let's try without file type restrictions since tsx isn't recognized. This should help us understand how datasource names are handled across the codebase.
I'll search for how the datasource name is used in the component and look for any error handling patterns around it.
Fallback handling for datasource name is appropriate
The code already implements proper error handling for undefined datasource name by using the nullish coalescing operator || to provide an empty string as fallback. This is a standard and safe approach, especially for display purposes in a debugger tab where an empty string is an acceptable fallback for missing datasource names.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's any specific handling or display for undefined datasource names
rg -A 3 "datasourceName={|datasource.*name" --type tsx
Length of output: 188
Script:
#!/bin/bash
# Let's fix the regex and search for datasource name usage patterns
rg -A 3 "datasourceName=\{|datasource.*name" --type tsx
# Also search for any error handling or fallback patterns related to datasource
rg -B 2 -A 2 "datasource\?" --type tsx
Length of output: 154
Script:
#!/bin/bash
# Search for datasource name patterns in all files
rg -A 3 "datasourceName=\{|datasource.*name"
# Look for error handling patterns related to datasource
rg -B 2 -A 2 "datasource\?"
# Find the specific component file to understand the context better
fd "QueryDebuggerTabs.tsx"
Length of output: 547021
Script:
#!/bin/bash
# Let's check how datasource name is used in the component and if there's any error handling
ast-grep --pattern 'datasourceName={$_}'
# Also check for any error handling patterns around datasource name
rg -B 2 -A 2 'datasourceName.*\|\|.*""' --type ts
Length of output: 1292
…o fix/datasource-name-schema
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (2)
Line range hint 48-91: Add test coverage for datasource name display.
The current test only verifies text selection. Consider adding tests to verify the datasource name display functionality, which is the core issue being fixed in this PR.
it("should display the correct datasource name", () => {
const mockDatasource = { id: "ds1", name: "TestDB" };
const storeWithDatasource = {
...storeState,
entities: {
...storeState.entities,
datasources: {
...storeState.entities.datasources,
list: [mockDatasource],
},
},
};
const store = mockStore(storeWithDatasource);
const { getByText } = render(
<Provider store={store}>
<ThemeProvider theme={lightTheme}>
<Router>
<QueryDebuggerTabs
actionName="Query1"
actionSource={{
id: "ID1",
name: "Query1",
type: ENTITY_TYPE.ACTION,
datasource: mockDatasource.id,
}}
isRunning={false}
onRunClick={() => {}}
/>
</Router>
</ThemeProvider>
</Provider>
);
expect(getByText(mockDatasource.name)).toBeInTheDocument();
});Line range hint 54-55: Clean up TODO comment and eslint disable.
Since we're modifying the file, we should address the TODO comment and properly type the store instead of using any.
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- let store: any;
+ let store: ReturnType<typeof mockStore>;📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (1 hunks)
🔇 Additional comments (1)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (1)
27-27: LGTM: Mock store structure updated correctly.
The addition of list: [] to datasources mock state aligns with the new selector requirements.
Description
Renaming the datasource was not getting updated in schema tab. This is caused because of the use of unpublished action object.
Fixes #37118
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11591002654
Commit: 2f3b111
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 30 Oct 2024 17:09:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements
Schemacomponent.