fix(config): only fetch the branding overlay when a deployment ships one - #523
Merged
Aman-Mittal merged 2 commits intoSep 6, 2026
Merged
Conversation
`branding/config.json` and `branding/i18n/{lang}.json` were requested on every
load. They are absent on every default install — which is the documented, correct
state — so a fresh deployment reported two 404s in the browser console on every
route.
The application already declined to *report* those failures: both fetches carry
`skipErrorToast`, and both resolve to `{}`. That is as far as this side can go. A
404 is a network-level event the browser writes regardless, so the only way for a
supported configuration to stop looking like a misconfiguration is not to make
the request.
`brandingOverlayEnabled` gates both. It cannot live in the overlay itself — a
file cannot announce its own absence — so it belongs in the layer below, and the
container entrypoint sets it by looking for the directory the deployer already
declared with `COPY branding/ ...`. Nothing new to remember, and the heredoc's own
rule is satisfied: this is a fact about the container's filesystem, which is
exactly what that file is the right source for.
The loader resolves `ConfigService` on use rather than as a field. Injecting it
at construction closes a cycle — `ConfigService` needs `HttpClient`, whose
`errorInterceptor` needs `I18N`, which needs the loader — and Angular reports
that as NG0200 during bootstrap with nothing rendered at all. The unit tests do
not catch it, because they provide `ConfigService` directly; it shows up the
moment the application is actually loaded.
Verified against a running dev server: `/branding/config.json` and
`/branding/i18n/en.json` are no longer requested, and with the flag on and an
overlay present both return 200 and the overlay's `appName` renders.
Contributor
E2E — mocked backend🎭 E2E Tests✅ All green — 343 passed · 0 failed · 0 skipped, across 28 spec files in 7m 21s. By spec file
All 343 tests — click to expand
Slowest 10 — what the shard counts should be tuned against
📼 Download the HTML report, videos and traces — see the Generated by run 34033212065 from |
Contributor
E2E — real Fineract🎭 E2E Tests✅ All green — 77 passed · 0 failed · 0 skipped, across 23 spec files in 5m 51s. By spec file
All 77 tests — click to expand
Slowest 10 — what the shard counts should be tuned against
📼 Download the HTML report, videos and traces — see the Generated by run 34033212065 from |
Both branding suites served `branding/config.json` while the layer beneath it said nothing, which is a state the container entrypoint cannot produce — it sets `brandingOverlayEnabled` from the directory the deployer copied in. Once the probe is gated on that flag the fixtures modelled a deployment that shipped `branding/` and never told the application, and 26 tests failed. `deployment-customization` derives the flag from whether the test asked for an overlay at all, so the "no overlay mounted" cases keep exercising the off path. `branded-deployment` always mounts one, so it is always on. Adds the assertion the fix is actually about, which only an e2e can make: with no overlay declared, nothing under `/branding/` is requested. A unit test cannot see this — it is about a request not being on the wire, and the console entry it used to produce was written by the browser, not by the application. 28 passed across both specs.
Member
Author
|
Pushed Both branding suites served
Added the assertion this change is actually about, which only an e2e can make: test('does not ask for an overlay this deployment has not declared', async ({ page }) => {
const asked: string[] = [];
page.on('request', (r) => { if (r.url().includes('/branding/')) asked.push(r.url()); });
await deployWith(page, null);
await expect(page.locator('.app-title')).toBeVisible();
expect(asked).toEqual([]);
});A unit test cannot see this. It is about a request not being on the wire, and the console entry it used to produce was written by the browser, not by us. One local-only failure, for the recordA full It passes on its own, and it is a local artifact: this config uses Not touched here. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #487.
What it does
branding/config.jsonandbranding/i18n/{lang}.jsonwere requested on every load. They are absent on every default install — the documented, correct state — so a fresh deployment reported two 404s in the browser console on every route.The application already declined to report those failures: both fetches carry
skipErrorToastand both resolve to{}. That is as far as this side can reach. A 404 is a network-level event the browser writes regardless, so the only way for a supported configuration to stop looking like a misconfiguration is not to make the request.The flag, and where it has to live
brandingOverlayEnabledgates both fetches. It cannot live in the overlay itself — a file cannot announce its own absence — so it belongs in the layer below.The container entrypoint sets it for you, by looking for the directory the deployer already declared with the
COPY branding/ ...line inDOCS/CUSTOMIZATION.md. There is nothing new to remember, and it satisfies that heredoc's own stated rule:Whether this image carries a branding overlay is a fact about the container's filesystem, which is exactly that. A deployment serving
dist/from its own web server sets it inconfig.jsonalongsidefineractApiUrl; that case is documented.The flag also makes a distinction the old code could not: "no overlay configured" versus "overlay configured but misdeployed". The second is a real error a deployer wants to see, and it still surfaces — a declared overlay that 404s is tolerated in the merge but is no longer indistinguishable from the ordinary case.
Verified against a running app
Console across
/dashboard,/clients,/loans,/notifications, capturing every line.Before, on
main:After, on this branch:
What remains is the microfrontend remote, which is simply not running in a plain
ng serve.And the positive path, with the flag on and a real overlay in place:
The overlay's
appNamerenders and the language overlay is merged, so the flag gates the probe without disabling the feature. The temporary overlay used for that check was removed;npm run check:branding-pathpasses.One trap worth flagging in review
The loader resolves
ConfigServiceon use, not as a field. Injecting it at construction closes a cycle —ConfigServiceneedsHttpClient, whoseerrorInterceptorneedsI18N, which needs the loader. Angular reports it as NG0200 during bootstrap and the app renders nothing at all.The unit tests do not catch that, because they provide
ConfigServicedirectly. I found it by loading the application;login.spec.tswould have caught it in CI. Worth knowing before anyone tidies thatinject(Injector)back into a field — the comment on it says so.Verification
npm run test:unitTZ=America/New_York npm run test:unitnpm run buildnpm run lintbash scripts/check-license.shnode scripts/check-branding-path.mjspublic/branding/is reserved and emptybash -n deploy/entrypoint.shNew coverage: four cases on
ConfigServicefor the probe (not declared, declared off, declared on, declared but missing) and a newdeployment-translate.loader.test.tswith four more, including that the overlay request is not issued when undeclared.Rebased on
f18d9592.