[workspace]Avoid getting unnecessary configs in ui setting client #11040
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughGet now supports an explicit scope and, when none is provided, derives and prioritizes scopes from the key defaults to return the first user-provided value (highest priority) or fall back to defaults; tests and a changelog fragment were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
opensearch-changeset-bot bot
added a commit
to Qxisylolo/OpenSearch-Dashboards
that referenced
this pull request
Dec 8, 2025
Qxisylolo
force-pushed
the
feat/enhance_ui_setting_read
branch
3 times, most recently
from
6f0e238 to
f972bb3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11040 +/- ##
=======================================
Coverage 60.78% 60.78%
=======================================
Files 4538 4538
Lines 122603 122617 +14
Branches 20597 20601 +4
=======================================
+ Hits 74522 74533 +11
- Misses 42805 42807 +2
- Partials 5276 5277 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Qxisylolo
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
ashwin-pc,
bandinib-amzn,
curq,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ruanyl,
sejli,
virajsanghvi,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
Qxisylolo
requested review from
Maosaic,
TackAdam,
angle943,
d-buckner,
huyaboo,
joshuali925,
paulstn,
ps48,
raintygao,
ruchidh,
sumukhswamy and
wanglam
as code owners
opensearch-changeset-bot bot
added a commit
to Qxisylolo/OpenSearch-Dashboards
that referenced
this pull request
Dec 8, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/core/server/ui_settings/ui_settings_client.ts (2)
155-159: Minor: Scope extraction could use a simpler ternary.The nested ternary is a bit hard to read. Consider simplifying:
- const scopes = Array.isArray(this.defaults[key]?.scope) - ? this.defaults[key].scope - : this.defaults[key]?.scope - ? [this.defaults[key].scope] - : []; + const defaultScope = this.defaults[key]?.scope; + const scopes = defaultScope + ? ([] as UiSettingScope[]).concat(defaultScope) + : [];
161-180: Redundant truthiness check and sequential awaits are intentional.Line 162:
scopes &&is redundant sincescopes?.length > 1already handles nullish values. Minor nit.The sequential
awaitin thefor...ofloop is appropriate here since it enables short-circuiting when a value is found in a higher-priority scope.- if (scopes && scopes?.length > 1) { + if (scopes?.length > 1) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/server/ui_settings/ui_settings_client.test.ts(1 hunks)src/core/server/ui_settings/ui_settings_client.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/core/server/ui_settings/ui_settings_client.ts (2)
src/core/server/index.ts (1)
UiSettingScope(363-363)src/core/server/ui_settings/index.ts (1)
UiSettingScope(51-51)
src/core/server/ui_settings/ui_settings_client.test.ts (3)
src/core/server/index.ts (1)
CURRENT_WORKSPACE_PLACEHOLDER(364-364)src/core/server/ui_settings/utils.ts (1)
CURRENT_WORKSPACE_PLACEHOLDER(9-9)src/core/server/ui_settings/index.ts (1)
CURRENT_WORKSPACE_PLACEHOLDER(54-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)
- GitHub Check: update-changelog
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: bundle-analyzer
🔇 Additional comments (4)
src/core/server/ui_settings/ui_settings_client.ts (1)
148-153: LGTM - Direct scope handling is clean.The early return when an explicit scope is provided avoids unnecessary lookups and aligns with the PR objective.
src/core/server/ui_settings/ui_settings_client.test.ts (3)
1093-1110: Good test coverage for scope-first retrieval optimization.This test correctly validates the core PR objective - avoiding unnecessary config fetches when the key's scope is known.
1112-1147: Good coverage for multi-scope priority resolution.The test correctly validates that workspace-scoped values take priority over global when both are present in a multi-scope setting.
1184-1206: Good coverage for default value fallback.This test validates the expected behavior when no user-provided values exist in any scope.
Comment on lines
1149
to
1182
| it('should return config following scope priority', async () => { | ||
| const defaults = { | ||
| multiSetting: { value: 'default', scope: ['global', 'workspace'] }, | ||
| }; | ||
|
|
||
| const { uiSettings, savedObjectsClient } = setup({ defaults }); | ||
|
|
||
| savedObjectsClient.get.mockImplementation((_type, id, _options) => { | ||
| if (id === `${CURRENT_WORKSPACE_PLACEHOLDER}_${ID}`) { | ||
| return Promise.resolve({ | ||
| attributes: {}, | ||
| } as any); | ||
| } else { | ||
| return Promise.resolve({ | ||
| attributes: { | ||
| multiSetting: 'global-value', | ||
| }, | ||
| } as any); | ||
| } | ||
| }); | ||
|
|
||
| const result = await uiSettings.get('multiSetting'); | ||
|
|
||
| expect(result).toBe('global-value'); | ||
|
|
||
| // Should fetch workspace config first | ||
| expect(savedObjectsClient.get).toHaveBeenCalledWith( | ||
| TYPE, | ||
| `${CURRENT_WORKSPACE_PLACEHOLDER}_${ID}` | ||
| ); | ||
|
|
||
| // Should fetch global config since workspace scope doesn't has this setting | ||
| expect(savedObjectsClient.get).toHaveBeenCalledWith(TYPE, ID); | ||
| }); |
There was a problem hiding this comment.
Minor grammar issue in comment.
Line 1180: "doesn't has" should be "doesn't have".
- // Should fetch global config since workspace scope doesn't has this setting
+ // Should fetch global config since workspace scope doesn't have this setting📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should return config following scope priority', async () => { | |
| const defaults = { | |
| multiSetting: { value: 'default', scope: ['global', 'workspace'] }, | |
| }; | |
| const { uiSettings, savedObjectsClient } = setup({ defaults }); | |
| savedObjectsClient.get.mockImplementation((_type, id, _options) => { | |
| if (id === `${CURRENT_WORKSPACE_PLACEHOLDER}_${ID}`) { | |
| return Promise.resolve({ | |
| attributes: {}, | |
| } as any); | |
| } else { | |
| return Promise.resolve({ | |
| attributes: { | |
| multiSetting: 'global-value', | |
| }, | |
| } as any); | |
| } | |
| }); | |
| const result = await uiSettings.get('multiSetting'); | |
| expect(result).toBe('global-value'); | |
| // Should fetch workspace config first | |
| expect(savedObjectsClient.get).toHaveBeenCalledWith( | |
| TYPE, | |
| `${CURRENT_WORKSPACE_PLACEHOLDER}_${ID}` | |
| ); | |
| // Should fetch global config since workspace scope doesn't has this setting | |
| expect(savedObjectsClient.get).toHaveBeenCalledWith(TYPE, ID); | |
| }); | |
| it('should return config following scope priority', async () => { | |
| const defaults = { | |
| multiSetting: { value: 'default', scope: ['global', 'workspace'] }, | |
| }; | |
| const { uiSettings, savedObjectsClient } = setup({ defaults }); | |
| savedObjectsClient.get.mockImplementation((_type, id, _options) => { | |
| if (id === `${CURRENT_WORKSPACE_PLACEHOLDER}_${ID}`) { | |
| return Promise.resolve({ | |
| attributes: {}, | |
| } as any); | |
| } else { | |
| return Promise.resolve({ | |
| attributes: { | |
| multiSetting: 'global-value', | |
| }, | |
| } as any); | |
| } | |
| }); | |
| const result = await uiSettings.get('multiSetting'); | |
| expect(result).toBe('global-value'); | |
| // Should fetch workspace config first | |
| expect(savedObjectsClient.get).toHaveBeenCalledWith( | |
| TYPE, | |
| `${CURRENT_WORKSPACE_PLACEHOLDER}_${ID}` | |
| ); | |
| // Should fetch global config since workspace scope doesn't have this setting | |
| expect(savedObjectsClient.get).toHaveBeenCalledWith(TYPE, ID); | |
| }); |
🤖 Prompt for AI Agents
In src/core/server/ui_settings/ui_settings_client.test.ts around lines 1149 to
1182, fix the minor grammar in the test comment on line 1180 by replacing
"doesn't has this setting" with "doesn't have this setting" so the comment reads
correctly; no behavior changes required.
Signed-off-by: Qxisylolo <[email protected]>
Qxisylolo
force-pushed
the
feat/enhance_ui_setting_read
branch
from
f25c853 to
0c5e5e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/server/ui_settings/ui_settings_client.test.ts (1)
1180-1180: Grammar correction needed in comment.The comment should read "doesn't have" instead of "doesn't has".
- // Should fetch global config since workspace scope doesn't has this setting + // Should fetch global config since workspace scope doesn't have this setting
🧹 Nitpick comments (1)
src/core/server/ui_settings/ui_settings_client.test.ts (1)
1184-1206: LGTM! Completes test coverage for default fallback.This test correctly validates the final fallback behavior when no user-provided values exist in any applicable scope, ensuring defaults are returned as expected.
Optional: The test description could be slightly more idiomatic:
- it('should return default config if there are not user provided values', async () => { + it('should return default config if there are no user-provided values', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelogs/fragments/11040.yml(1 hunks)src/core/server/ui_settings/ui_settings_client.test.ts(1 hunks)src/core/server/ui_settings/ui_settings_client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- changelogs/fragments/11040.yml
- src/core/server/ui_settings/ui_settings_client.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/server/ui_settings/ui_settings_client.test.ts (4)
src/core/server/ui_settings/ui_settings_service.ts (1)
setup(84-119)src/core/public/ui_settings/ui_settings_service.ts (1)
setup(55-85)src/plugins/workspace/server/ui_settings.ts (1)
uiSettings(17-59)src/core/server/ui_settings/utils.ts (1)
CURRENT_WORKSPACE_PLACEHOLDER(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (67)
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Lint and validate
🔇 Additional comments (3)
src/core/server/ui_settings/ui_settings_client.test.ts (3)
1093-1110: LGTM! Well-structured test for scope-aware retrieval.This test correctly validates that when fetching a workspace-scoped setting, the implementation only retrieves from the workspace scope and avoids unnecessary global config fetches, achieving the PR's optimization goal.
1112-1147: LGTM! Correctly tests scope priority and short-circuit optimization.This test properly validates that when multiple scopes define a setting, the implementation respects the priority order (workspace > global) and short-circuits once a value is found, avoiding unnecessary fetches.
1149-1179: LGTM! Validates scope fallback behavior correctly.This test properly confirms that when a higher-priority scope (workspace) doesn't contain the setting, the implementation falls back to the next applicable scope (global), ensuring comprehensive coverage of the priority-based retrieval logic.
Also applies to: 1181-1182
wanglam
reviewed
Dec 8, 2025
| UiSettingScope.GLOBAL, | ||
| ]; | ||
|
|
||
| const validScopes = scopePriority.filter((s) => scopes.includes(s)); |
Collaborator
There was a problem hiding this comment.
Can we just sort the scopes instead of filtering out not valid scopes? The main thought is when we add a new scope, we need to add a new scope priority items above. Otherwise this scope can't be retrieved.
FriedhelmWS
reviewed
Dec 8, 2025
| ? this.defaults[key].scope | ||
| : this.defaults[key]?.scope | ||
| ? [this.defaults[key].scope] | ||
| : []; |
Contributor
There was a problem hiding this comment.
Nit: could you please simplify these nested tertiary operands
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/server/ui_settings/ui_settings_client.ts (2)
155-159: Consider simplifying the nested ternary operators.As noted in a previous review, the nested ternary operators reduce readability. While the logic is correct, extracting the scope value into an intermediate variable would make the intent clearer.
Apply this diff to improve readability:
- const scopes = Array.isArray(this.defaults[key]?.scope) - ? this.defaults[key].scope - : this.defaults[key]?.scope - ? [this.defaults[key].scope] - : []; + const scopeValue = this.defaults[key]?.scope; + const scopes = scopeValue + ? (Array.isArray(scopeValue) ? scopeValue : [scopeValue]) + : [];
169-181: Minor optimization opportunity: consider caching getUserProvided results.When no user-provided value is found in the loop (lines 169-176), the fallback at line 181 calls
getAll(scopes[0]), which internally callsgetUserProvided(scopes[0])again—resulting in a duplicate read from the saved objects for the highest-priority scope. While this doesn't affect correctness and the overall optimization goal is still achieved, caching thegetUserProvidedresults from the loop could eliminate this redundant call.Example approach:
if (scopes && scopes?.length > 1) { const validScopes = [...scopes].sort((a, b) => { const indexA = UiSettingScopeReadOptions.findIndex((opt) => opt.scope === a); const indexB = UiSettingScopeReadOptions.findIndex((opt) => opt.scope === b); return indexB - indexA; }); + // Cache to avoid duplicate getUserProvided calls + const userProvidedCache = new Map<UiSettingScope, UserProvided>(); + for (const s of validScopes) { - const userProvided = await this.getUserProvided(s); + const userProvided = await this.getUserProvided(s); + userProvidedCache.set(s, userProvided); if (userProvided && Object.prototype.hasOwnProperty.call(userProvided, key)) { // align with getUserProvided, higher scope takes priority return userProvided[key].userValue as T; } } + + // Use cached result for fallback if available + if (userProvidedCache.has(scopes[0])) { + const userProvided = userProvidedCache.get(scopes[0])!; + const raw = defaultsDeep(userProvided, this.defaults); + const item = raw[key]; + return ('userValue' in item ? item.userValue : item.value) as T; + } }Note: This is a minor optimization and can be deferred if the added complexity isn't justified by performance profiles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelogs/fragments/11040.yml(1 hunks)src/core/server/ui_settings/ui_settings_client.test.ts(1 hunks)src/core/server/ui_settings/ui_settings_client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- changelogs/fragments/11040.yml
- src/core/server/ui_settings/ui_settings_client.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (68)
- GitHub Check: lighthouse
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: linkchecker
- GitHub Check: bundle-analyzer
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Lint and validate
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Run plugin functional tests on Linux
🔇 Additional comments (2)
src/core/server/ui_settings/ui_settings_client.ts (2)
148-153: LGTM!The explicit scope handling is clear and efficient. When a scope is provided, the method correctly delegates to
getAll(scope)and returns the value for the requested key.
162-177: Excellent optimization - multi-scope priority logic is correct.The scope prioritization correctly ensures that higher-priority scopes (those with higher indices in
UiSettingScopeReadOptions) are checked first, aligning with the merge order ingetUserProvided(). The early-return pattern efficiently avoids reading from lower-priority scopes once a user-provided value is found.
Signed-off-by: Qxisylolo <[email protected]>
Qxisylolo
force-pushed
the
feat/enhance_ui_setting_read
branch
from
7156638 to
4813f16
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/server/ui_settings/ui_settings_client.ts (1)
159-160: Minor: simplify the multi‑scope guard
scopesis always an array due to its initialization:const scopes = scopeValue ? (Array.isArray(scopeValue) ? scopeValue : [scopeValue]) : [];So the extra null/optional checks are redundant. You can simplify:
- // If no scope is provided and the key has multiple scopes - if (scopes && scopes?.length > 1) { + // If no scope is provided and the key has multiple scopes + if (scopes.length > 1) {This keeps behavior identical and makes the condition clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/server/ui_settings/ui_settings_client.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/server/ui_settings/ui_settings_client.ts (2)
src/core/server/index.ts (1)
UiSettingScope(363-363)src/core/server/ui_settings/index.ts (1)
UiSettingScope(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (68)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: lighthouse
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: linkchecker
- GitHub Check: bundle-analyzer
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Lint and validate
🔇 Additional comments (1)
src/core/server/ui_settings/ui_settings_client.ts (1)
149-153: Explicitscopeshort‑circuit looks correct and efficientUsing
getAll(scope)when an explicitscopeis passed cleanly avoids the multi‑scope resolution, keeps I/O limited to the requested scope, and preserves existing override/default behavior viagetAll. No issues here.
Comment on lines
+159
to
+174
| if (scopes && scopes?.length > 1) { | ||
| const validScopes = [...scopes].sort((a, b) => { | ||
| const indexA = UiSettingScopeReadOptions.findIndex((opt) => opt.scope === a); | ||
| const indexB = UiSettingScopeReadOptions.findIndex((opt) => opt.scope === b); | ||
| return indexB - indexA; | ||
| }); | ||
|
|
||
| for (const s of validScopes) { | ||
| const userProvided = await this.getUserProvided(s); | ||
|
|
||
| if (userProvided && Object.prototype.hasOwnProperty.call(userProvided, key)) { | ||
| // align with getUserProvided, higher scope takes priority | ||
| return userProvided[key].userValue as T; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix null‑override handling in multi‑scope get to avoid returning undefined
In the multi‑scope loop, getUserProvided(s) includes overrides, and a null override is represented as { isOverridden: true } without a userValue. The current check:
if (userProvided && Object.prototype.hasOwnProperty.call(userProvided, key)) {
// align with getUserProvided, higher scope takes priority
return userProvided[key].userValue as T;
}will treat such an entry as a hit and return undefined, whereas getAll/getRaw would fall back to the default value for null overrides. For multi‑scope keys with a null override, get(key) now returns undefined instead of the default, which is a behavioral bug.
You can align behavior with getAll by only treating entries that actually have a userValue as hits:
for (const s of validScopes) {
const userProvided = await this.getUserProvided(s);
- if (userProvided && Object.prototype.hasOwnProperty.call(userProvided, key)) {
- // align with getUserProvided, higher scope takes priority
- return userProvided[key].userValue as T;
- }
+ if (userProvided && Object.prototype.hasOwnProperty.call(userProvided, key)) {
+ const entry = userProvided[key];
+ if (entry && 'userValue' in entry) {
+ // align with getUserProvided, higher scope takes priority
+ return entry.userValue as T;
+ }
+ }
}This keeps non‑null overrides and user values working as before while letting null overrides correctly fall through to the default via the existing fallback.
🤖 Prompt for AI Agents
In src/core/server/ui_settings/ui_settings_client.ts around lines 159 to 174,
the multi‑scope loop treats any presence of a key in userProvided as a hit and
returns userProvided[key].userValue which can be undefined for null overrides;
change the condition to only treat an entry as a hit when the entry actually
contains a userValue (e.g. check that
Object.prototype.hasOwnProperty.call(userProvided, key) && 'userValue' in
userProvided[key] or userProvided[key].userValue !== undefined), and only then
return userProvided[key].userValue as T so null overrides fall through to the
default value as in getAll/getRaw.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pr enhances get method in the UI settings client to search only the scope that the key belongs to
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
Tests
Refactor
Chore
✏️ Tip: You can customize this high-level summary in your review settings.