fix: DH-20243: improved notebook settings merging

[jnumainville]

It is expected that getSettings substitutes custom settings on top of default settings when the settings are pulled.
The problem is this substitution was only happening properly if top level settings were being replaced.
This meant that the nested linter settings were not getting properly replaced for notebookSettings because notebookSettings is never itself undefined, only the keys config and isEnabled are.

[dgodinez-dh]

Looks like an eslint failure? But it looks like you've got a comment for the error, so I'm not sure why it's failing.

[mattrunyon]

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

[jnumainville]

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.

[mattrunyon]

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?

[jnumainville]

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.

[jnumainville]

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.

[dgodinez-dh]

@jnumainville I see this PR is still failing some checks. I am out until next year, so you might want to re-assign this review to someone else.

[jnumainville]

Yeah I'm still actively cleaning this up... I think Matt's review should be sufficient so I'll only rerequest from him (I know he's off next week too but hopefully we can get this merged tomorrow worst case).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 45.48%. Comparing base (d199942) to head (d8e8376).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2593   +/-   ##
=======================================
  Coverage   45.48%   45.48%           
=======================================
  Files         771      771           
  Lines       43591    43591           
  Branches    11027    11027           
=======================================
  Hits        19827    19827           
  Misses      23748    23748           
  Partials       16       16           

Flag	Coverage Δ
unit	45.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mattrunyon]

Does this change alone fix the default for Ruff in DHE or will we also need a DHE change to create the settings properly on login?

[jnumainville]

This should fix it on DHE. This was where I traced the issue back to on DHE.