Repository navigation
Conversation
Settings registered through setConfig win over the window globals uicore
has always read; unset keys keep the window fallback, so existing
consumers are unaffected. A key set to undefined or an empty string is
unset; null, false and 0 are values (oauth2UseRefreshToken: false turns
the refresh flow off).
The state lives on globalThis under
Symbol.for('openstack-uicore-foundation.config'), so every copy of the
module shares it: bundles that inline it, nested installs, and symlinked
dev checkouts.
getServerTime fetched whatever getTimeServiceUrl returned, so an
unconfigured host produced a garbage request ('' fetches the current
page, undefined fetches the literal string) before landing on the same
local-clock fallback a rejected fetch reaches directly.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Legacy global compatibility and falsy group configuration handling need fixes.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds opt-in configuration so hosts can supply uicore settings without populating window globals.
Changes:
- Introduces shared
setConfigstate and configuration getters. - Migrates consumers while retaining existing getter exports.
- Adds configuration tests and skips clock requests when no endpoint exists.
| File | Description |
|---|---|
| webpack.common.js | Exposes the config entry point. |
| src/utils/query-actions.js | Uses centralized API configuration. |
| src/utils/methods.js | Re-exports configuration getters. |
| src/utils/config.js | Adds shared settings and fallback readers. |
| src/utils/__tests__/config.test.js | Tests overrides, resets and shared state. |
| src/utils/__tests__/config-reads.test.js | Tests legacy defaults and exports. |
| src/utils/__tests__/config-no-window.test.js | Tests server-side configuration reads. |
| src/components/security/reducers.js | Uses centralized OAuth settings. |
| src/components/security/methods.js | Imports and re-exports OAuth getters. |
| src/components/security/actions.js | Uses centralized API and group settings. |
| src/components/security/abstract-auth-callback-route.js | Reads OAuth flow from config. |
| src/components/security/abstract-auth-callback-route-v2.js | Reads OAuth flow from config. |
| src/components/security/__tests__/get-user-info.test.js | Updates configuration mocks. |
| src/components/exclusive-wrapper.js | Reads configured exclusive sections. |
| src/components/clock.js | Uses config and handles missing endpoints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const readConfig = () => globalThis[CONFIG_KEY] || {}; | ||
|
|
||
| export const setConfig = (next) => { | ||
| const source = next && typeof next === 'object' && !Array.isArray(next) ? next : {}; | ||
| globalThis[CONFIG_KEY] = Object.fromEntries( |
There was a problem hiding this comment.
Fixed in 2ef8771 — config reads and writes now use the same guarded global selection as methods.js (globalThis → window → {}), so runtimes without globalThis keep the window-globals flow.
| (hasWindow() ? window.TIMEINTERVALSINCE1970_API_URL || process.env.TIMEINTERVALSINCE1970_API_URL : null)); | ||
|
|
||
| export const getAllowedUserGroups = () => | ||
| configuredOr('allowedUserGroups', () => (hasWindow() ? window.ALLOWED_USER_GROUPS || '' : null)); |
There was a problem hiding this comment.
Fixed in 0800356 — the allowed-user-groups string is parsed through parseUserGroups, which returns an empty list for any falsy value (including the null the no-window fallback returns), so getUserInfo no longer calls .split on a non-string. Covered by new tests in config.test.js.
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
@gcutrini I think we need to discuss the approach
| */ | ||
| const CONFIG_KEY = Symbol.for('openstack-uicore-foundation.config'); | ||
|
|
||
| const readConfig = () => globalThis[CONFIG_KEY] || {}; |
There was a problem hiding this comment.
I don't get it, why not use localStorage for example to overdrive ENV ? we would only need one method that "gets" the config and use that instead of the env var
There was a problem hiding this comment.
A getter is the right idea, that's what getConfig is. The question is where it reads from.
It's one mechanism for everything the host hands uicore: the config, the token resolver (#329), and the auth handlers (#350). The resolver and the handlers are functions — functions can't go in localStorage or ENV. So the store has to be an in-memory registry, and the config rides the same one instead of a second mechanism just for config.
On ENV: env values are inlined at build time, and only into your own repo's files. uicore ships already compiled and your build never recompiles it, so an env read inside uicore never gets your value — and the browser where uicore runs has no process.env anyway. Feeding it ENV would mean swapping uicore's config module at build time, the webpack-alias hack this PR removes.
The store is globalThis under a Symbol.for key: in-memory, lives with the page, works in any runtime, and the consumer never touches it — they call setConfig/getConfig.
readConfig and setConfig used bare globalThis. They now use the same
_global fallback (globalThis → window → {}) as the security methods, so
the window-globals fallback still works on a runtime without globalThis.
getUserInfo split the allowed-user-groups string whenever it was not '', so the null value getAllowedUserGroups returns with no window threw. parseUserGroups splits a truthy string and returns an empty list for any falsy value; getUserInfo calls it.
ref: https://app.clickup.com/t/86bcdq027
uicore reads its OAuth/app settings (
apiBaseUrl,idpBaseUrl,oauth2ClientId,timeApiUrl) from window globals by default. A host that drives uicore through ports needs to hand uicore those settings directly instead of via globals it never populates.setConfiglets a consumer pass uicore its settings; the config readers (clock, exclusive-wrapper, auth-callback routes, security actions/methods) delegate to it when a config is registered, otherwise the built-in window-globals flow is unchanged. Same opt-in shape assetAccessTokenResolver(#324).