Skip to content

fix: DH-20243: improved notebook settings merging #2593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jnumainville merged 9 commits into from
Dec 19, 2025
Merged

fix: DH-20243: improved notebook settings merging #2593

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8832253
improving notebook merging
jnumainville Dec 17, 2025
ed643b6
remove sneaky mystery packages
jnumainville Dec 17, 2025
c38c974
mergeWith
jnumainville Dec 18, 2025
026e77a
refine mergewith usage
jnumainville Dec 18, 2025
c763a44
types for mergewith
jnumainville Dec 18, 2025
7229918
unknown?
jnumainville Dec 18, 2025
1004b00
I wasn't using that ignore anyways
jnumainville Dec 18, 2025
0236144
comment
jnumainville Dec 19, 2025
d8e8376
removing special config logic
jnumainville Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions packages/redux/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,38 @@
return workspace;
};

/**
* Helpter function to replace settings values
* Any nested objects under workspace settings need to be handled here
* @param key the key of the setting to replace
* @param settings current settings object
* @param customizedSettings customized settings to apply
*/
const replaceSettings = (
key: keyof WorkspaceSettings,
settings: WorkspaceSettings,
customizedSettings: UndoPartial<WorkspaceSettings>
): void => {
let newSettings = customizedSettings[key];
if (key === 'notebookSettings') {
const customizedLinter =
customizedSettings.notebookSettings?.python?.linter;
const settingsLinter = settings.notebookSettings?.python?.linter;
newSettings = {
...settings.notebookSettings,
...customizedSettings.notebookSettings,
python: {
linter: {
isEnabled: customizedLinter?.isEnabled ?? settingsLinter?.isEnabled,
config: customizedLinter?.config ?? settingsLinter?.config,
},
},
};
}
// @ts-expect-error assign non-undefined customized settings to settings
settings[key] = newSettings;

Check failure on line 87 in packages/redux/src/selectors.ts

View workflow job for this annotation

GitHub Actions / unit 

Assignment to property of function parameter 'settings'
};

// Settings
export const getSettings = memoize(
<State extends RootState>(
Expand All @@ -67,8 +99,11 @@
for (let i = 0; i < keys.length; i += 1) {
const key = keys[i];
if (customizedSettings[key] !== undefined) {
// @ts-expect-error assign non-undefined customized settings to settings
settings[key] = customizedSettings[key];
replaceSettings(
key,
settings,
customizedSettings as UndoPartial<WorkspaceSettings>
);
Copy link
Collaborator

@mattrunyon mattrunyon Dec 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use lodash mergeWith here to deep merge keys, but don't merge arrays (the default behavior of lodash merge). Then this should be more resilient to future changes

const newSettings = mergeWith({ ......getDefaultWorkspaceSettings(store) }, customizedSettings, (objValue, srcValue) => Array.isArray(objValue) && Array.isArray(srcValue) ? srcValue : undefined)

Plain merge will merge array keys. I don't know if we have any array state stored here, but merging it is probably wrong if we do?

This is the default behavior

const a = [1,2,3,4,5,6]
const b = [4,5,6]
merge(a, b) // [4,5,6,4,5,6]
mergeWith(...) // [4,5,6] using above code

Copy link
Contributor Author

@jnumainville jnumainville Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and replaced the whole function here with the mergeWith.
The only weird thing is the linter config. We wouldn't want that merged, correct?
I just took the approach of pulling off that config, manually determining which one we want, then putting it on top of whatever is merged. Not the most elegant and might be wasteful, but simple, and I'd need that special case either way. With the new implementation we're otherwise resilient unless we have more objects that should be replaced instead of merged.

Copy link
Collaborator

@mattrunyon mattrunyon Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if merging the linter config would be a big deal. AFAIK you can turn all the rules/settings off, so merging shouldn't be an issue since you would just explicitly turn something off instead of remove it. And in some cases I think we might put an explicit default (even if it matches the omitted default in ruff) just so it's shown

Also, by not merging arrays, this would still work for the linter config as I think that's just an array of enabled rules.

Can you add a comment that says this is deep merging, but not merging items in arrays?

Copy link
Contributor Author

@jnumainville jnumainville Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right this wouldn't really matter for the rules themselves but it does look like there are some settings that aren't within arrays that could be set like line-length as a random example? Although maybe if it was set on the server then removed on the frontend we actually would want to revert to the server default instead of the ruff default. I guess I'm trying to understand if there are any settings where you'd say "if this setting is removed we should revert to the ruff default instead of the server default" but seems like that answer is no? Which is reasonable.
I've added the comment.

Copy link
Contributor Author

@jnumainville jnumainville Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the special config logic as well. Seems like we should be reverting to a server default not a ruff default if a setting is removed, which is what would happen.

}
}
return settings as UndoPartial<State['workspace']['data']['settings']>;
Expand Down
Loading