feat: Read settings from props/server config when available by Zhou-Ziheng · Pull Request #1558 · deephaven/web-client-ui

Zhou-Ziheng · 2023-10-04T04:16:59Z

Updated the follow values to use ServerConfig when possible:

defaultDateTimeFormat
timeZone
defaultDecimalFormatOptions
defaultIntegerFormatOptions
truncateNumbersWithPound
defaultNotebookSettings.isMinimapEnabled

To test, first create a config file (see https://deephaven.io/core/docs/how-to-guides/configuration/config-file/), then add
- ./deephaven.prop:/opt/deephaven/config/deephaven.prop
as a volumn for services.deephaven.columns

BREAKING CHANGE: saveSettings action has been replaced with updateSettings, which now just takes partial settings instead

codecov · 2023-10-04T04:24:17Z

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (4875d2e) 46.66% compared to head (4e06b30) 46.63%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1558      +/-   ##
==========================================
- Coverage   46.66%   46.63%   -0.03%     
==========================================
  Files         591      591              
  Lines       36394    36406      +12     
  Branches     9108     9113       +5     
==========================================
- Hits        16984    16979       -5     
- Misses      19358    19375      +17     
  Partials       52       52

Flag	Coverage Δ
unit	`46.63% <31.52%> (-0.03%)`	⬇️

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

Files	Coverage Δ
packages/code-studio/src/main/AppMainContainer.tsx	`31.15% <ø> (ø)`
...ages/code-studio/src/styleguide/StyleGuideInit.tsx	`0.00% <ø> (ø)`
...ashboard-core-plugins/src/panels/IrisGridPanel.tsx	`43.05% <ø> (ø)`
packages/code-studio/src/main/AppInit.tsx	`0.00% <0.00%> (ø)`
...udio/src/settings/ColumnSpecificSectionContent.tsx	`97.25% <85.71%> (-0.06%)`	⬇️
...de-studio/src/settings/ShortcutsSectionContent.tsx	`6.97% <33.33%> (ø)`
...ackages/dashboard-core-plugins/src/ChartPlugin.tsx	`4.25% <0.00%> (-0.19%)`	⬇️
...ashboard-core-plugins/src/panels/NotebookPanel.tsx	`2.23% <0.00%> (+<0.01%)`	⬆️
...e-studio/src/settings/FormattingSectionContent.tsx	`64.46% <48.88%> (+1.76%)`	⬆️
...s/code-studio/src/storage/LocalWorkspaceStorage.ts	`9.30% <0.00%> (-12.92%)`	⬇️

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

mofojed · 2023-10-11T14:49:06Z

+          LocalWorkspaceStorage.getServerConfigValueOrUseDefault(
+            serverConfigValues,
+            'dateTimeFormat',
+            DateTimeColumnFormatter.DEFAULT_DATETIME_FORMAT_STRING
+          ),


Instead of setting the value, we should leave it undefined; that way if the server value is updated again, user gets the new value unless they've explicitly updated the option before.

That's how we handle it on Enterprise, anyway: https://github.com/deephaven-ent/iris/blob/b41e56506498118f27fed79ccac41f3dc2af4617/web/client-ui/src/dashboard/WorkspaceStorage.ts#L174
https://github.com/deephaven-ent/iris/blob/b41e56506498118f27fed79ccac41f3dc2af4617/web/client-ui/src/settings/FormattingSectionContent.tsx#L857

From a previous commit I did: https://github.com/deephaven-ent/iris/commit/28a75ae75b27bf97fa1046418af89aa16986e412

mofojed

Can you include the steps you took to configure the server/test different default values being read?

mofojed · 2023-11-01T13:38:02Z

 export const saveSettings =
  (
-    settings: WorkspaceSettings
+    settings: Partial<WorkspaceSettings>
  ): ThunkAction<
-    Promise<Workspace>,
+    Promise<CustomizableWorkspace>,
    RootState,
    never,
    PayloadAction<unknown>
  > =>
  dispatch =>
    dispatch(updateWorkspaceData({ settings }));


I'd rather you make this a new action updateSettings rather than saveSettings. Otherwise there's no way to delete an old setting.

old settings can be deleted by setting it's value to undefined. Although I do agree updateSettings make more sense semantically

mofojed · 2023-11-01T13:46:52Z

+    // this will only be null on initial render
+    return null as never;


We should just throw in this case ...

Suggested change

// this will only be null on initial render

return null as never;

throw new Error('getWorkspace called before workspace was set')

Then in AppInit we should catch that error in the mapStateToProps and set workspace to null there. That will achieve two things:

A more descriptive error about how to fix the issue rather than just getting a possibly inexplicable NPE

More accurate depiction in AppInit of what's going on with the workspace prop. Right now it's defined as just Workspace but we're still expecting/checking if it's null to know if isLoading is still ongoing, which is kind of ignoring the type checking.

mofojed · 2023-11-01T13:51:07Z

+  const defaultInjectedWorkspace: Workspace = {
+    data: {
+      ...workspace.data,
+      settings: getDefaultWorkspaceSettings(store) ?? {},
+    },
+  };


Right idea, but I think we're injecting at the wrong spot. We should inject just the settings in getSettings instead of injecting whenever the workspace is fetched.
Also, we should memoize the result.

I wonder if we are fetching the entire workspace then manually destructure the settings object from it somewhere. My original intention was so that the Workspace object would look identical to before the change, to avoid creating unintended changes elsewhere

mofojed · 2023-11-01T13:53:51Z

+  const keys = Object.keys(
+    customizedWorkspaceSettings
+  ) as (keyof WorkspaceSettings)[];
+
+  for (let i = 0; i < keys.length; i += 1) {
+    const key = keys[i];
+    const value = customizedWorkspaceSettings[key];
+    // only set pass in customized defaults if defined
+    if (value !== undefined) {
+      defaultInjectedWorkspace.data.settings[key] =
+        value as WorkspaceSettings[typeof key] as never;
+    }
+  }


Unsure why you wrote it this way - pretty sure when injecting in getSettings, you could just do something like:

export const getSettings = <State extends RootState>( store: State ): WorkspaceSettings => { const defaultSettings = getDefaultWorkspaceSettings(store) ?? EMPTY_OBJECT; const userSettings = getWorkspace(store).data.settings; return { ...defaultSettings, ...userSettings, }; };

This was because sometimes the userSettings have values intentionally set to undefined (not just missing) to let user reset values to default etc. setting a value to undefined means the value is still keyed in userSettings so doing object spreading would make those values undefined as opposed to the default.

mofojed · 2023-11-01T13:55:56Z

+  defaultDateTimeFormat?: string;
+  showTimeZone?: boolean;
+  showTSeparator?: boolean;
+  timeZone?: string;
+  truncateNumbersWithPound?: boolean;
+  saveSettings: (settings: Partial<WorkspaceSettings>) => void;
+  defaultDecimalFormatOptions?: FormatOption;
+  defaultIntegerFormatOptions?: FormatOption;


None of these should be optional, the value we get from redux should always be defined.

This was mostly wrestling with TypeScript. I had to change the Redux state type for workspace.data.settings to be optional (because it really is optional now), so the getWorkspace's return value is now typed as CustomizableWorkspace instead of Workspace, marking these parameters optional. I'll investigate for a workaround

mofojed · 2023-11-01T13:57:31Z

+  saveSettings: (settings: Partial<WorkspaceSettings>) => void;
+  defaultDecimalFormatOptions?: FormatOption;
+  defaultIntegerFormatOptions?: FormatOption;
  defaults: {


defaults we should be able to just map to getDefaultWorkspaceSettings instead of redefining defaults here.

mofojed · 2023-11-01T13:57:55Z

+    assertNotNull(showTimeZone);
+    assertNotNull(showTSeparator);
+    assertNotNull(timeZone);
+    assertNotNull(truncateNumbersWithPound);
+    assertNotNull(defaultDateTimeFormat);
+    assertNotNull(defaultDecimalFormatOptions);
+    assertNotNull(defaultIntegerFormatOptions);


None of these should be optional/you shouldn't need to assert against null

mofojed · 2023-11-01T14:08:35Z

+        IntegerColumnFormatter.makeCustomFormat(event.target.value)
+      )
+    ) {
+      this.commitChanges(update);


We should still debounce these changes, as they could have implications for the entire UI (e.g. if you have a table open and you're updating the formatting, it'll update every cell of every table open with every keystroke).
See my other comment in this file about debouncing for a suggestion.

Zhou-Ziheng · 2023-11-08T16:53:54Z

This is the deephaven.prop I tested with:


internal.webClient.appInit.canCopy=false
internal.webClient.appInit.canDownloadCsv=false
integerFormat=##123

client.configuration.list=java.version,deephaven.version,barrage.version,internal.webClient.appInit.canCopy,internal.webClient.appInit.canDownloadCsv,integerFormat,

the only thing of importance is the integerFormat

mofojed

Still a few issues with functionality, I think I've pointed out all the problematic areas but here is a test case that I think should be comprehensive to illustrate what the issue is. This assumes starting from an incognito tab, so no previous workspace data

Set integerFormat=##000 in props
Login as user and check in the Settings menu

Integer format option should show ##000 and not have the "Reset Integer Format" button visible, since we are using the default value

Shut down the server, set integerFormat=##111 in props
From the same browser as step 2 (so workspace data is intact), login and check the settings tab

Integer format option should show ##111 and not have the "Reset Integer Format" visible

Change the option to ##999. Reset button should become visible
Shut down the server, set integerFormat=##222 in props
Login and check settings tab

Integer format should show ##999 as you set in step 5. Reset button should be visible

Click the Reset button - the integer format should change to ##222 and reset button should disappear.

You can also inspect what you have set in Redux using the Redux dev tools, and what's stored in LocalStorage using the browser dev tools. You want to make sure the default settings in redux are populated with the settings read from the server (anything not defined by the server should fallback to whatever previous defaults we have set right now), and workspace storage settings should only have the options the user has changed.

mofojed

Some minor cleanup, but looks great! Thanks

init

20602fa

Zhou-Ziheng changed the title ~~Read settings from props/server config when available~~ feat: Read settings from props/server config when available Oct 4, 2023

mofojed requested changes Oct 11, 2023

View reviewed changes

Zhou-Ziheng self-assigned this Oct 11, 2023

Zhou-Ziheng added 2 commits October 11, 2023 14:45

interesting

f058868

Merge remote-tracking branch 'origin/main' into use_server_config

e5c9794

Zhou-Ziheng marked this pull request as ready for review October 11, 2023 18:50

Zhou-Ziheng requested a review from mofojed October 11, 2023 18:50

mofojed reviewed Oct 12, 2023

View reviewed changes

Zhou-Ziheng added 8 commits October 28, 2023 22:39

sus

6616fc2

Merge remote-tracking branch 'origin/main' into use_server_config

0985974

sus

c4ba548

fixed types

6dc09d8

tests

c1ecd97

types

f17a4b7

sus3

6fa97f4

types

b54cbbc

Zhou-Ziheng requested a review from mofojed October 31, 2023 15:40

mofojed requested changes Nov 1, 2023

View reviewed changes

mofojed linked an issue Nov 6, 2023 that may be closed by this pull request

Read settings from props/server config when available #1446

Closed

Zhou-Ziheng added 6 commits November 8, 2023 11:08

only inject when getSettings

e9aa8bc

update optional

dfdba6d

debounce commit changes

9c28f30

debounce

532ca85

Merge remote-tracking branch 'origin/main' into use_server_config

bad34f1

assertions

4149f4f

Zhou-Ziheng requested a review from mofojed November 8, 2023 16:50

weird redux issue

c7baa09

Zhou-Ziheng added 6 commits November 8, 2023 14:57

weird redux issue

8335a0b

deep copy

8c615ac

why did I add this

2f1008c

yikes

6e8044d

ts

83b0925

more ts

69f6579

mofojed requested changes Nov 8, 2023

View reviewed changes

Comment thread packages/code-studio/src/settings/FormattingSectionContent.tsx Outdated

Comment thread packages/code-studio/src/storage/LocalWorkspaceStorage.ts Outdated

Comment thread packages/code-studio/src/storage/LocalWorkspaceStorage.ts

clean code

99f8a4b

Zhou-Ziheng requested a review from mofojed November 8, 2023 21:41

mofojed previously approved these changes Nov 9, 2023

View reviewed changes

requested changes

7bb79c4

Zhou-Ziheng dismissed mofojed’s stale review via 7bb79c4 November 9, 2023 20:11

Merge remote-tracking branch 'origin/main' into use_server_config

4e06b30

Zhou-Ziheng enabled auto-merge (squash) November 9, 2023 21:01

Zhou-Ziheng requested a review from mofojed November 9, 2023 21:01

mofojed approved these changes Nov 9, 2023

View reviewed changes

Zhou-Ziheng merged commit 52ba2cd into deephaven:main Nov 9, 2023

github-actions Bot locked and limited conversation to collaborators Nov 9, 2023

		// this will only be null on initial render
		return null as never;

	// this will only be null on initial render
	return null as never;
	throw new Error('getWorkspace called before workspace was set')

Conversation

Zhou-Ziheng commented Oct 4, 2023 • edited by mofojed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mofojed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Zhou-Ziheng commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mofojed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mofojed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Zhou-Ziheng commented Oct 4, 2023 •

edited by mofojed

Loading

codecov Bot commented Oct 4, 2023 •

edited

Loading

Zhou-Ziheng commented Nov 8, 2023 •

edited

Loading