This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 6182c6c81d refactor(frontend): preserve config source in
GuiConfigService instead of one flat map (#5673)
6182c6c81d is described below
commit 6182c6c81df93fabf9ea22eb1350db1642ce9bb0
Author: Matthew B. <[email protected]>
AuthorDate: Tue Jun 23 15:33:03 2026 -0700
refactor(frontend): preserve config source in GuiConfigService instead of
one flat map (#5673)
> Stacked on #5545 (adds the `/config/amber` endpoint and the `amber`
config source this PR namespaces). Base diff includes #5545 until it
merges; review the last commit only, or after #5545 lands.
### What changes were proposed in this PR?
- Changed `GuiConfigService` to store each endpoint's payload under its
own key in a `configBySource` map (`preLogin`, `gui`, `amber`,
`userSystem`) instead of spreading them all into one flat object, so the
origin of every config value is preserved (addresses the review comment
on #5545).
- Derived the existing flat `env` getter from `configBySource` on read,
keeping its shape identical so the ~50 existing
`guiConfigService.env.<flag>` call sites are untouched.
- Memoized `env` so it returns a stable object reference (rebuilt only
when a source loads). This preserves the two-way `[(ngModel)]`
write-through at `menu.component.html`, which only persists when the
same object identity is held across change-detection reads.
- Added a `source(name)` accessor that returns one endpoint's payload in
isolation.
- Added unit tests for: source isolation (each source retrievable alone,
no key bleed across sources, empty object for an unloaded source), env
reference stability (stable across reads, rebuilt after a new source
loads, in-memory write discarded on reload), and merge precedence (later
source wins on a shared key).
### Any related issues, documentation, discussions?
Closes: #5669
### How was this PR tested?
- Run `cd frontend && npx ng test
--include="src/app/common/service/gui-config.service.spec.ts"` and
expect 15 passed (the 8 existing merge/orchestration tests plus 7 new
tests covering source isolation, env reference stability, and merge
precedence).
- Confirm the merged view is unchanged:
`service.env.defaultDataTransferBatchSize` still resolves after
post-login load, while `service.source("amber")` returns only the amber
payload and `service.source("gui")` does not contain
`defaultDataTransferBatchSize`.
### Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.8 in compliance with ASF
---
.../app/common/service/gui-config.service.spec.ts | 115 +++++++++++++++++++++
.../src/app/common/service/gui-config.service.ts | 56 ++++++++--
2 files changed, 160 insertions(+), 11 deletions(-)
diff --git a/frontend/src/app/common/service/gui-config.service.spec.ts
b/frontend/src/app/common/service/gui-config.service.spec.ts
index 92a0d5a615..9dad79e471 100644
--- a/frontend/src/app/common/service/gui-config.service.spec.ts
+++ b/frontend/src/app/common/service/gui-config.service.spec.ts
@@ -207,4 +207,119 @@ describe("GuiConfigService", () => {
http.expectNone(`${API}/config/user-system`);
await expect(pending).rejects.toThrow(/pre-login configuration/);
});
+
+ // ─── source() isolation ───────────────────────────────────────────────────
+
+ it("source() keeps each endpoint's payload retrievable in isolation", async
() => {
+ const preLoginPending = firstValueFrom(service.loadPreLogin());
+ http.expectOne(`${API}/config/pre-login`).flush(PRE_LOGIN_PAYLOAD);
+ await preLoginPending;
+
+ const postLoginPending = firstValueFrom(service.loadPostLogin());
+ http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD);
+ http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD);
+ http.expectOne(`${API}/config/user-system`).flush(USER_SYSTEM_PAYLOAD);
+ await postLoginPending;
+
+ expect(service.source("preLogin")).toEqual(PRE_LOGIN_PAYLOAD);
+ expect(service.source("gui")).toEqual(GUI_PAYLOAD);
+ expect(service.source("amber")).toEqual(AMBER_PAYLOAD);
+ expect(service.source("userSystem")).toEqual(USER_SYSTEM_PAYLOAD);
+ });
+
+ it("source() does not bleed keys across sources", async () => {
+ // A value from one endpoint must not appear under another.
+ const pending = firstValueFrom(service.loadPostLogin());
+ http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD);
+ http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD);
+ http.expectOne(`${API}/config/user-system`).flush(USER_SYSTEM_PAYLOAD);
+ await pending;
+
+
expect(service.source("amber")).toHaveProperty("defaultDataTransferBatchSize");
+
expect(service.source("gui")).not.toHaveProperty("defaultDataTransferBatchSize");
+ expect(service.source("gui")).not.toHaveProperty("inviteOnly");
+ // env still flattens them together for the common single-flag call sites.
+ expect(service.env.defaultDataTransferBatchSize).toBe(400);
+ });
+
+ it("source() returns an empty object for an endpoint that has not loaded",
() => {
+ expect(service.source("amber")).toEqual({});
+ });
+
+ // ─── env reference stability
───────────────────────────────────────────────
+
+ it("env returns a stable reference between loads so two-way bindings
persist", async () => {
+ const pending = firstValueFrom(service.loadPreLogin());
+ http.expectOne(`${API}/config/pre-login`).flush(PRE_LOGIN_PAYLOAD);
+ await pending;
+
+ // Repeated reads must return the same object (relied on by change
detection
+ // and by the [(ngModel)] write-through at menu.component.html).
+ expect(service.env).toBe(service.env);
+
+ // A write through env persists across subsequent reads.
+ (service.env as any).workflowEmailNotificationEnabled = true;
+ expect(service.env.workflowEmailNotificationEnabled).toBe(true);
+ });
+
+ it("env reference is rebuilt after a new source loads", async () => {
+ const preLoginPending = firstValueFrom(service.loadPreLogin());
+ http.expectOne(`${API}/config/pre-login`).flush(PRE_LOGIN_PAYLOAD);
+ await preLoginPending;
+ const before = service.env;
+
+ const postLoginPending = firstValueFrom(service.loadPostLogin());
+ http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD);
+ http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD);
+ http.expectOne(`${API}/config/user-system`).flush(USER_SYSTEM_PAYLOAD);
+ await postLoginPending;
+
+ expect(service.env).not.toBe(before);
+ expect(service.env.defaultDataTransferBatchSize).toBe(400);
+ });
+
+ it("a write through env is discarded when a source reloads", async () => {
+ // Documents the boundary of the memoized write-through: in-memory edits
made
+ // via [(ngModel)] land on the merged cache only, not on any source
payload.
+ // A subsequent source load invalidates the cache, so the edit does not
+ // survive a re-fetch (e.g. a fresh login re-running loadPostLogin). This
is
+ // intentional (config reloads are authoritative) and is locked in here so
a
+ // future "persist edits across reloads" change is a deliberate decision.
+ const preLoginPending = firstValueFrom(service.loadPreLogin());
+ http.expectOne(`${API}/config/pre-login`).flush(PRE_LOGIN_PAYLOAD);
+ await preLoginPending;
+
+ (service.env as any).workflowEmailNotificationEnabled = true;
+ expect(service.env.workflowEmailNotificationEnabled).toBe(true);
+
+ const postLoginPending = firstValueFrom(service.loadPostLogin());
+ http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD);
+ http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD);
+ http.expectOne(`${API}/config/user-system`).flush(USER_SYSTEM_PAYLOAD);
+ await postLoginPending;
+
+ expect(service.env.workflowEmailNotificationEnabled).toBeUndefined();
+ });
+
+ // ─── merge precedence
───────────────────────────────────────────────────────
+
+ it("env merges sources in MERGE_ORDER so a later source wins on a shared
key", async () => {
+ // The typed source payloads are disjoint, so precedence never fires
through
+ // the public API, but the merge reduce implements "later source wins" and
+ // nothing else covers it. Inject an overlapping key via the raw payloads
to
+ // lock the documented order (preLogin, gui, amber, userSystem).
+ const preLoginPending = firstValueFrom(service.loadPreLogin());
+ http.expectOne(`${API}/config/pre-login`).flush({ ...PRE_LOGIN_PAYLOAD,
inviteOnly: false });
+ await preLoginPending;
+ // userSystem comes after preLogin in MERGE_ORDER, so its value must win.
+ expect(service.env.inviteOnly).toBe(false);
+
+ const postLoginPending = firstValueFrom(service.loadPostLogin());
+ http.expectOne(`${API}/config/gui`).flush(GUI_PAYLOAD);
+ http.expectOne(`${API}/config/amber`).flush(AMBER_PAYLOAD);
+ http.expectOne(`${API}/config/user-system`).flush({ inviteOnly: true });
+ await postLoginPending;
+
+ expect(service.env.inviteOnly).toBe(true);
+ });
});
diff --git a/frontend/src/app/common/service/gui-config.service.ts
b/frontend/src/app/common/service/gui-config.service.ts
index 28695f7d06..7806f1a680 100644
--- a/frontend/src/app/common/service/gui-config.service.ts
+++ b/frontend/src/app/common/service/gui-config.service.ts
@@ -34,23 +34,36 @@ type AmberConfig = Pick<GuiConfig,
"defaultDataTransferBatchSize">;
type GuiOnlyConfig = Omit<GuiConfig, keyof PreLoginConfig | keyof AmberConfig
| "inviteOnly">;
type UserSystemConfig = Pick<GuiConfig, "inviteOnly">;
+// One entry per backend config endpoint.
+export type ConfigSource = "preLogin" | "gui" | "amber" | "userSystem";
+
@Injectable({ providedIn: "root" })
export class GuiConfigService {
- private config: Partial<GuiConfig> = {};
+ // Each endpoint's payload, kept under its own key.
+ private configBySource: Partial<Record<ConfigSource, Partial<GuiConfig>>> =
{};
+
+ // Memoized flat merge. Rebuilt only when a source is written so that env
+ // returns a stable reference: callers read env on every change-detection
+ // cycle, and one call site mutates env through a two-way [(ngModel)]
binding,
+ // which only persists when the object identity is held across reads.
+ private mergedCache: GuiConfig | null = null;
+
+ // Merge precedence when a key appears in multiple sources (later wins).
+ private static readonly MERGE_ORDER: ConfigSource[] = ["preLogin", "gui",
"amber", "userSystem"];
constructor(private http: HttpClient) {}
/**
* APP_INITIALIZER entry point. Always loads /config/pre-login (anonymous).
If
* a JWT is already in localStorage (browser reload while logged in), chains
- * /config/gui + /config/user-system in the same await so the full config is
- * available before any post-login component mounts.
+ * /config/gui + /config/amber + /config/user-system in the same await so the
+ * full config is available before any post-login component mounts.
*/
load(): Observable<Partial<GuiConfig>> {
return this.loadPreLogin().pipe(
switchMap(() => {
if (!GuiConfigService.hasStoredAccessToken()) {
- return of(this.config);
+ return of(this.env);
}
return this.loadPostLogin().pipe(
// Expired or malformed token → /config/gui returns 403. Continue
@@ -58,7 +71,7 @@ export class GuiConfigService {
// expiry on its own, so we shouldn't block bootstrap on it.
catchError((err: unknown) => {
console.warn("Failed to load authenticated config; continuing with
pre-login only.", err);
- return of(this.config);
+ return of(this.env);
})
);
})
@@ -68,9 +81,9 @@ export class GuiConfigService {
loadPreLogin(): Observable<Partial<GuiConfig>> {
return
this.http.get<PreLoginConfig>(`${AppSettings.getApiEndpoint()}/config/pre-login`).pipe(
tap(preLogin => {
- this.config = { ...this.config, ...preLogin };
+ this.setSource("preLogin", preLogin);
}),
- map(() => this.config),
+ map(() => this.env),
catchError((error: unknown) => {
console.error("Failed to load pre-login configuration:", error);
return throwError(() => new Error(`Failed to load pre-login
configuration from backend: ${error}`));
@@ -80,7 +93,7 @@ export class GuiConfigService {
/**
* Fetches the authenticated portion of the configuration. Runs after the
- * frontend has a valid JWT — called from APP_INITIALIZER on bootstrap when a
+ * frontend has a valid JWT, called from APP_INITIALIZER on bootstrap when a
* stored token exists, and from UserService.handleAccessToken on fresh
login.
*/
loadPostLogin(): Observable<Partial<GuiConfig>> {
@@ -89,14 +102,35 @@ export class GuiConfigService {
const userSystemConfig$ =
this.http.get<UserSystemConfig>(`${AppSettings.getApiEndpoint()}/config/user-system`);
return forkJoin([guiConfig$, amberConfig$, userSystemConfig$]).pipe(
tap(([guiConfig, amberConfig, userSystemConfig]) => {
- this.config = { ...this.config, ...guiConfig, ...amberConfig,
...userSystemConfig };
+ this.setSource("gui", guiConfig);
+ this.setSource("amber", amberConfig);
+ this.setSource("userSystem", userSystemConfig);
}),
- map(() => this.config)
+ map(() => this.env)
);
}
+ // Flat merge of all sources, memoized so reads return a stable reference.
get env(): GuiConfig {
- return this.config as GuiConfig;
+ if (this.mergedCache === null) {
+ this.mergedCache = GuiConfigService.MERGE_ORDER.reduce(
+ (merged, source) => ({ ...merged, ...this.configBySource[source] }),
+ {} as Partial<GuiConfig>
+ ) as GuiConfig;
+ }
+ return this.mergedCache;
+ }
+
+ // One endpoint's payload, in isolation. Returns a shallow copy so callers
+ // cannot mutate the stored source and desync it from the memoized env view.
+ source(name: ConfigSource): Partial<GuiConfig> {
+ return { ...(this.configBySource[name] ?? {}) };
+ }
+
+ // Store a source payload and invalidate the merged view.
+ private setSource(name: ConfigSource, payload: Partial<GuiConfig>): void {
+ this.configBySource[name] = payload;
+ this.mergedCache = null;
}
private static hasStoredAccessToken(): boolean {
