Skip to content

feat(narratives): add config contract, static assets and lockfile - #439

Open
ddebasmita-lab wants to merge 2 commits into
datacommonsorg:mainfrom
ddebasmita-lab:narratives-config-assets
Open

feat(narratives): add config contract, static assets and lockfile#439
ddebasmita-lab wants to merge 2 commits into
datacommonsorg:mainfrom
ddebasmita-lab:narratives-config-assets

Conversation

@ddebasmita-lab

Copy link
Copy Markdown
Contributor

Makes the narratives app deployable from a clean clone. Three related gaps:

  • package-lock.json was never committed, so npm ci — used by the image build — failed outright. All 25 dependencies float on ^ ranges, so pinning the tree is what makes a clone reproduce the tested build.

  • public/ was missing entirely while four code paths referenced it: /send.svg (view_initial, data_agent), /loader.png (block_reasoning) and /logo.png (header, as a fallback). Those requests 404'd.

  • config/ documents the per-instance contract — the branding and agent schemas plus fillable examples — which the agent and UI read from the instance's config bucket at runtime. Instance-specific values stay out of the repo: copy the .example.json files and edit them.

template_vars is kept but now says plainly that placeholder rendering is not implemented; the previous wording claimed the agent already substituted {{instance.*}}, which it does not.


@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces configuration schemas, examples, and documentation for customizing Data Commons instances, including support for branding, metrics, and agent settings. It also updates the useBranding hook and its tests to support the new logo configuration key alongside its legacy alias. The review feedback suggests tightening the regex validation for fiscal_year_start in the agent configuration schema and adding a test case to cover the public-bucket fetching path in the branding hook tests.

Comment on lines +161 to +165
"fiscal_year_start": {
"description": "Month/day of fiscal-year start (e.g. '04-01' for India, '01-01' for calendar-year states).",
"type": "string",
"pattern": "^[0-9]{2}-[0-9]{2}$"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current regex pattern ^[0-9]{2}-[0-9]{2}$ only validates that the value consists of two digits, a hyphen, and two digits. This allows invalid dates like 99-99 or 00-00 to pass schema validation. We can make this pattern more robust by strictly validating that the month is between 01 and 12 and the day is between 01 and 31.

Suggested change
"fiscal_year_start": {
"description": "Month/day of fiscal-year start (e.g. '04-01' for India, '01-01' for calendar-year states).",
"type": "string",
"pattern": "^[0-9]{2}-[0-9]{2}$"
}
"fiscal_year_start": {
"description": "Month/day of fiscal-year start (e.g. '04-01' for India, '01-01' for calendar-year states).",
"type": "string",
"pattern": "^(0[1-9]|1[0-2])-(0[1-9]|[12][0-9]|3[01])$"
}

Comment on lines +73 to +74
});
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test suite currently only covers the private-bucket path (where /agent/brand returns the branding object directly). The public-bucket path (where /agent/brand only returns brand_config_url and the browser must fetch branding.json directly from the GCS bucket) is currently untested. Adding a test case for this path would improve test coverage and ensure that the public-bucket fetching logic works as expected.

  });

  it("fetches branding.json from the bucket in the public-bucket path", async () => {
    vi.stubGlobal(
      "fetch",
      vi.fn()
        .mockResolvedValueOnce({
          ok: true,
          json: async () => ({ brand_config_url: BUCKET_URL }),
        })
        .mockResolvedValueOnce({
          ok: true,
          json: async () => ({ instance_name: "Public Example", logo: "assets/logo.png" }),
        }),
    );

    const { result } = renderHook(() => useBranding());

    await waitFor(() => expect(result.current.loaded).toBe(true));
    expect(result.current.branding.logoUrl).toBe(BUCKET_URL + "/assets/logo.png");
    expect(result.current.branding.instanceName).toBe("Public Example");
  });
});

Makes the narratives app deployable from a clean clone. Three related gaps:

- `package-lock.json` was never committed, so `npm ci` — used by the image
  build — failed outright. All 25 dependencies float on `^` ranges, so pinning
  the tree is what makes a clone reproduce the tested build.

- `public/` was missing entirely while four code paths referenced it:
  `/send.svg` (view_initial, data_agent), `/loader.png` (block_reasoning) and
  `/logo.png` (header, as a fallback). Those requests 404'd.

- `config/` documents the per-instance contract — the branding and agent
  schemas plus fillable examples — which the agent and UI read from the
  instance's config bucket at runtime. Instance-specific values stay out of the
  repo: copy the `.example.json` files and edit them.

Also fixes the branding logo, which never loaded from config. The schema
publishes `logo`, but the UI read only `logo_url`, so `logoUrl` stayed empty and
the header always took the bundled fallback. `mapRawToBranding` now reads `logo`
and keeps `logo_url` as a legacy alias, matching how the same file already
accepts legacy aliases for colors, fonts and `suggestion_chips`. Covered by
tests that fail without the fix.

Overriding the logo stays optional: no logo ships in the config example, so a
fresh instance renders the bundled `public/logo.png`. An instance that wants its
own adds a `logo` key and uploads the image to its config bucket, which
`config/README.md` documents.

The branding schema previously declared eight keys with no consumer anywhere in
the UI — `favicon`, `logo_alt`, `extra_css`, `footer`, `splash_assets`,
`instance_id`, `domain_hint` and `analytics`. They are omitted here rather than
carried over, so every key in the schema is one the UI actually reads.

`template_vars` is kept but now says plainly that placeholder rendering is not
implemented; the previous wording claimed the agent already substituted
`{{instance.*}}`, which it does not.
@ddebasmita-lab
ddebasmita-lab force-pushed the narratives-config-assets branch from 67b1632 to bef8ea0 Compare August 6, 2026 19:56

@juliawu juliawu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the cleanup! Just a few comments. I also think the gemini review comments are worth looking at.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be changed later, but @miss-o-soup should we have a different "default" logo in the main repo?

"additionalProperties": false,
"properties": {
"api_keys": {
"description": "Key pool for rotation. SHAPE only \u2014 real values live in Secret Manager (GEMINI_API_KEYS_SECRET env var) in prod mode. The agent's prod-mode loader rejects inline keys with a warning.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's require Secret Manager instead of inline keys throughout. This makes it safe to check in configs in repos, and defends against inadvertent exposed keys.

…tests

Review follow-ups on the config contract:

- API keys are no longer part of the config contract at all. `gemini.api_keys`,
  `demo_api_keys` and the legacy scalar `api_key` are removed from the schema and
  the example. `gemini` is `additionalProperties: false`, so adding them back is
  a validation error — which is the point: the file stays safe to commit and to
  serve from a config bucket, and a key cannot leak by being pasted into it.
  A deployed instance resolves keys from Secret Manager via
  `GEMINI_API_KEYS_SECRET` / `GEMINI_DEMO_API_KEYS_SECRET`.

  The agent keeps reading `gemini.api_keys` from a local, uncommitted
  `config.json` so local development still works, but that path now logs a
  warning naming the environment variable to set. Previously it succeeded
  silently, so a deployment reading keys from config looked healthy.
  `get_api_keys`'s docstring also claimed the scalar `gemini.api_key` was
  "rejected outright" when the code accepted it; it now describes what the code
  does.

- `template_vars.fiscal_year_start` had pattern `^[0-9]{2}-[0-9]{2}$`, which
  accepted `99-99` and `00-00`. Now bounds the month to 01-12 and the day to
  01-31.

- Two tests cover the public-bucket path, which had none: `/agent/brand`
  returning only `brand_config_url` and the browser fetching
  `<bucket>/branding.json` itself, plus the 404 case falling back to defaults.

- The header's logo alt text hardcoded one instance's name as its fallback.
  It now uses the configured instance name, or a neutral string when unset.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants