Sync dataset name with datasource in add remote view #8245
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
42-43: Consider adding JSDoc documentationWhile the parameter name is self-explanatory, adding JSDoc documentation would help explain the purpose and impact of this parameter to other developers.
export const syncDataSourceFields = ( form: FormInstance, syncTargetTabKey: "simple" | "advanced", + /** When true, synchronizes the dataset name between simple and advanced views */ syncDatasetName = false, ): void => {
59-69: LGTM: Advanced to simple sync implementationThe implementation includes proper type casting and null safety checks. For consistency with the rest of the codebase, consider using optional chaining syntax.
- if (syncDatasetName && dataSourceFromAdvancedTab?.id?.name) { + if (syncDatasetName && dataSourceFromAdvancedTab?.id?.name != null) {frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
Line range hint
503-503: Ensure consistent usage of syncDataSourceFieldsThe
syncDataSourceFieldsis called with different parameter counts in different places:
- Line 379:
syncDataSourceFields(form, activeEditMode, true)- Line 503:
syncDataSourceFields(form, dataSourceEditMode === "simple" ? "advanced" : "simple")- Line 592:
syncDataSourceFields(form, dataSourceEditMode === "simple" ? "advanced" : "simple")For consistent behavior, consider adding the
syncDatasetNameparameter to all calls:- syncDataSourceFields(form, dataSourceEditMode === "simple" ? "advanced" : "simple"); + syncDataSourceFields(form, dataSourceEditMode === "simple" ? "advanced" : "simple", true);Also applies to: 592-592
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(2 hunks)
🔇 Additional comments (3)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
36-36: LGTM: Type import addition
The ArbitraryObject type import enhances type safety for the data source object handling.
Line range hint 42-69: Verify bidirectional sync behavior
Please ensure that the bidirectional synchronization works as expected in the following scenarios:
- When switching from simple to advanced mode
- When switching from advanced to simple mode
- When the dataset name is empty
- When the datasource JSON is invalid
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
379-379: LGTM! Verify function signature in DatasetSettingsDataTab
The addition of true parameter to syncDataSourceFields correctly implements the dataset name synchronization between simple and advanced modes.
Let's verify the function signature update in the imported module:
✅ Verification successful
Function signature verified - Change is correct
The syncDataSourceFields function in dataset_settings_data_tab.tsx has the correct signature with an optional syncDatasetName parameter:
export const syncDataSourceFields = (
form: FormInstance,
syncTargetTabKey: "simple" | "advanced",
syncDatasetName = false,
): voidThe addition of true as the third argument in dataset_add_remote_view.tsx correctly enables dataset name synchronization, matching the function's signature and the PR's objective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the updated function signature of syncDataSourceFields
# Expected: Should find the updated function signature with syncDatasetName parameter
ast-grep --pattern 'export function syncDataSourceFields(form: FormInstance, syncTargetTabKey: "simple" | "advanced"$_)'
Length of output: 122
Script:
#!/bin/bash
# Let's try a broader search to locate the function definition
rg -l "syncDataSourceFields" --type ts --type tsx
# Then examine the content of any matching files
rg "syncDataSourceFields" -A 5 --type ts --type tsx
Length of output: 157
Script:
#!/bin/bash
# Search for the function without file type restrictions since tsx might not be recognized
rg "syncDataSourceFields" -A 5
Length of output: 6213
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
36-45: LGTM! Consider moving the comment closer to the parameter.The type import and optional parameter addition look good. The comments clearly explain the parameter's purpose and usage context.
Consider moving the comments closer to the parameter declaration for better readability:
export const syncDataSourceFields = ( form: FormInstance, syncTargetTabKey: "simple" | "advanced", - // Syncing the dataset name is optional as this is needed for the add remote view, but not for the edit view. - // In the edit view, the datasource.id fields should never be changed and the backend will automatically ignore all changes to the id field. - syncDatasetName = false, + syncDatasetName = false, // Optional: needed for add remote view, not for edit view where datasource.id changes are ignored by backend ): void => {
54-74: LGTM! Consider enhancing type safety.The implementation correctly handles bidirectional synchronization with proper null safety checks.
Consider using TypeScript's nullish coalescing operator for more concise null safety:
if (syncTargetTabKey === "advanced") { const dataSourceFromSimpleTab = form.getFieldValue("dataSource"); if (syncDatasetName) { - if (dataSourceFromSimpleTab.id == null) { - dataSourceFromSimpleTab.id = {}; - } + dataSourceFromSimpleTab.id ??= {}; dataSourceFromSimpleTab.id.name = form.getFieldValue(["dataset", "name"]); } form.setFieldsValue({ dataSourceJson: jsonStringify(dataSourceFromSimpleTab), }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(2 hunks)
🔇 Additional comments (1)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)
54-59: Previous null safety concern has been addressed.
The implementation now includes proper null safety checks for dataSourceFromSimpleTab.id, addressing the concern raised in the previous review.
There was a problem hiding this comment.
code looks alright, but testing didn't work for me. in my case I did not switch to the advanced tab. I think in that case the synchronization is not triggered? at least, I got this error: toast.tsx:215 The datasource config could not be stored. Error: The dataset name must not be empty.. only after switching to the tab, the import worked.
| form.validateFields(); | ||
| }; | ||
|
|
||
| async function handleStoreDataset() { | ||
| // Sync simple with advanced and get newest datasourceJson | ||
| syncDataSourceFields(form, dataSourceEditMode === "simple" ? "advanced" : "simple"); | ||
| syncDataSourceFields(form, dataSourceEditMode === "simple" ? "advanced" : "simple", true); |
There was a problem hiding this comment.
I missed a few occurrences of syncDataSourceFields 🙈, this one here being the most important.
| @@ -47,12 +51,25 @@ export const syncDataSourceFields = ( | |||
| if (syncTargetTabKey === "advanced") { | |||
| // Copy from simple to advanced: update json | |||
| const dataSourceFromSimpleTab = form.getFieldValue("dataSource"); | |||
| if (syncDatasetName && dataSourceFromSimpleTab) { | |||
There was a problem hiding this comment.
The && dataSourceFromSimpleTab was added as syncDataSourceFields is called before the first exploration request is sent to the backend. In this case dataSourceFromSimpleTab==undefined and thus I guarded this assignment below as else the code would silently crash.
URL of deployed dev instance (used for testing):
Steps to test:
id.nameshould be in sync with the dataset name input field of the simple settings viewIssues:
(Please delete unneeded items, merge only when none are left open)