[BUG] Allow user Theme Selection retain theme selection #7787

ananzh · 2024-08-21T17:25:25Z

Description

Add browserStoredSettings and getWithBrowserSettings method to allow the retrieval and resolution of values from the cache and browser settings separately. When user control is enabled, we update the browserStoredSettings without interrupting the cache (advance settings user values).

Issues Resolved

#7689

Screenshot

2024-08-21_10-09-24.mp4

Changelog

fix: [BUG] Allow user Theme Selection retain theme selection

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

github-actions · 2024-08-21T17:26:03Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

virajsanghvi · 2024-08-21T17:29:57Z

src/core/public/ui_settings/ui_settings_client.ts

@@ -62,7 +63,8 @@ export class UiSettingsClient implements IUiSettingsClient {
      this.cache['theme:enableUserControl']?.userValue ??
      this.cache['theme:enableUserControl']?.value
    ) {
-      this.cache = defaultsDeep(this.cache, this.getBrowserStoredSettings());
+      this.browserStoredSettings = this.getBrowserStoredSettings();
+      this.cache = defaultsDeep({}, this.defaults, this.cache, this.browserStoredSettings);


isn't the order of precedence left to right, so this doesn't really change any behavior, right?

it feels like we can just remove this if we're not utilizing it because there shouldn't be a case where there's a browser setting but not an advanced setting, right? Or if an admin doesn't explicitly save something, is there no userValue from advanced settings?

virajsanghvi · 2024-08-21T17:32:00Z

src/core/public/ui_settings/ui_settings_client.ts

+    }
+
+    const type = this.cache[key].type;
+    const userValue = this.cache[key].userValue;


i wouldn't name variable userValue as its really the value from advanced settings, not related to the user

virajsanghvi · 2024-08-21T17:34:23Z

src/core/public/ui_settings/ui_settings_client.ts

+    const defaultValue = defaultOverride !== undefined ? defaultOverride : this.cache[key].value;
+
+    let value;
+    if (this.cache['theme:enableUserControl']?.userValue ?? this.cache['theme:enableUserControl']?.value) {


know this is a wip, but seems like this should be consumer logic

virajsanghvi · 2024-08-21T17:40:51Z

src/core/public/ui_settings/ui_settings_client.ts

+      value = browserValue ?? userValue ?? defaultValue;
+    } else {
+      value = userValue ?? defaultValue;


can null be a valid value? if so, don't think we should use ??

codecov · 2024-08-21T17:54:27Z

Codecov Report

Attention: Patch coverage is 30.43478% with 32 lines in your changes missing coverage. Please review.

Project coverage is 63.83%. Comparing base (aed03fa) to head (4bf12eb).
Report is 17 commits behind head on main.

Files	Patch %	Lines
...dvanced_settings/public/header_user_theme_menu.tsx	0.00%	20 Missing ⚠️
src/core/public/ui_settings/ui_settings_client.ts	53.84%	7 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7787      +/-   ##
==========================================
+ Coverage   63.80%   63.83%   +0.03%     
==========================================
  Files        3656     3659       +3     
  Lines       81205    81326     +121     
  Branches    12950    12987      +37     
==========================================
+ Hits        51809    51915     +106     
- Misses      26215    26225      +10     
- Partials     3181     3186       +5

Flag	Coverage Δ
Linux_1	`?`
Linux_2	`55.89% <53.84%> (+0.02%)`	⬆️
Linux_3	`40.37% <0.00%> (-0.05%)`	⬇️
Linux_4	`31.29% <0.00%> (+<0.01%)`	⬆️
Windows_1	`30.17% <0.00%> (+0.07%)`	⬆️
Windows_2	`55.85% <53.84%> (+0.02%)`	⬆️
Windows_3	`40.38% <0.00%> (-0.04%)`	⬇️
Windows_4	`31.29% <0.00%> (+<0.01%)`	⬆️

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.

github-actions · 2024-08-23T04:59:10Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

ananzh · 2024-08-23T05:11:59Z

This can only be backported to 2.x if #5652, which is reverted in #6978, and #7776 are backported.

Issue Resolve: opensearch-project#7689 Signed-off-by: Anan Zhuang <ananzh@amazon.com>

virajsanghvi · 2024-08-23T18:17:31Z

src/core/public/ui_settings/types.ts

+  /**
+   * Gets the value for a specific uiSetting, considering browser-stored settings and advanced settings.
+   * This method returns an object containing the resolved value and individual setting values.
+   *
+   * @param key - The key of the uiSetting to retrieve
+   * @param defaultOverride - An optional default value to use if the setting is not declared
+   * @returns An object containing the resolved value and additional setting information
+   * @throws Error if the setting is not declared and no defaultOverride is provided
+   */
+  getWithBrowserSettings<T = any>(
+    key: string,
+    defaultOverride?: T
+  ): {
+    advancedSettingValue: T | undefined;
+    browserValue: T | undefined;
+    defaultValue: T;
+  };
+


callout: I think this is fine for now to avoid increasing scope - i think in the future, we should consider making get() aware of all contexts, and having experiences like advanced settings explicitly call another method or a variant to exclude browser settings in resolution

two suggestions:

should we call this getValues() or something more generic where this can provide all levels of values for this setting (not just browser as it also returns default value today)?

should this include a resolvedValue or a value that handles what the current active value is so consumers don't have to resolve this themselves?

virajsanghvi · 2024-08-23T18:23:26Z

src/core/public/ui_settings/ui_settings_client.ts

+    const resolveValue = (val: any): T => this.resolveValue(val, type);
+
+    const resolvedAdvancedValue =
+      advancedSettingValue !== undefined ? resolveValue(advancedSettingValue) : undefined;


nit: could create a helper function to handle this undefined case and inline these with return

virajsanghvi · 2024-08-23T18:25:09Z

src/core/public/ui_settings/ui_settings_client.ts

-
-    const oldVal = declared ? this.cache[key].userValue : undefined;
+    const userControl =
+      this.cache['theme:enableUserControl']?.userValue ??


ui_settings_client shouldn't be aware of a theme specific setting right?

also, could other settings be null? if so, this probably should be checking that these aren't defined.

virajsanghvi · 2024-08-23T18:29:03Z

src/plugins/advanced_settings/public/header_user_theme_menu.tsx

+    const currentDarkMode = enableUserControl
+      ? result.browserValue ?? result.advancedSettingValue ?? result.defaultValue
+      : result.advancedSettingValue ?? result.defaultValue;


i don't think consumers should be resolving values - think this should come from uisettingsclient

github-actions bot added the distinguished-contributor label Aug 21, 2024

github-actions bot added the failed changeset label Aug 21, 2024

virajsanghvi reviewed Aug 21, 2024

View reviewed changes

ananzh force-pushed the fix-browser-theme branch from 102d371 to 67cadf3 Compare August 23, 2024 04:58

ananzh changed the title ~~Fix Browser theme~~ [BUG] Allow user Theme Selection retain theme selection Aug 23, 2024

github-actions bot removed the failed changeset label Aug 23, 2024

opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Aug 23, 2024

Changeset file for PR opensearch-project#7787 created/updated

59183f0

ananzh marked this pull request as ready for review August 23, 2024 05:08

ananzh requested review from kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi and mengweieric as code owners August 23, 2024 05:08

ananzh requested review from LDrago27, sejli and joshuali925 as code owners August 23, 2024 05:08

ananzh added the bug Something isn't working label Aug 23, 2024

[BUG] Allow user Theme Selection retain theme selection

d2a5fc8

Issue Resolve: opensearch-project#7689 Signed-off-by: Anan Zhuang <ananzh@amazon.com>

ananzh force-pushed the fix-browser-theme branch from 59183f0 to d2a5fc8 Compare August 23, 2024 17:26

virajsanghvi reviewed Aug 23, 2024

View reviewed changes

Changeset file for PR opensearch-project#7787 created/updated

4bf12eb

joshuali925 mentioned this pull request Sep 13, 2024

[Rebase #6730 and fix tests] Fix/UI settings overrides #7776

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Allow user Theme Selection retain theme selection #7787

[BUG] Allow user Theme Selection retain theme selection #7787

ananzh commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024

virajsanghvi Aug 21, 2024

virajsanghvi Aug 21, 2024

virajsanghvi Aug 21, 2024

virajsanghvi Aug 21, 2024

codecov bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 23, 2024

ananzh commented Aug 23, 2024

virajsanghvi Aug 23, 2024

virajsanghvi Aug 23, 2024

virajsanghvi Aug 23, 2024

virajsanghvi Aug 23, 2024

[BUG] Allow user Theme Selection retain theme selection #7787

Are you sure you want to change the base?

[BUG] Allow user Theme Selection retain theme selection #7787

Conversation

ananzh commented Aug 21, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Changelog

Check List

github-actions bot commented Aug 21, 2024

❌ Empty Changelog Section

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 23, 2024

❌ Empty Changelog Section

ananzh commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananzh commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading