Skip to content

fix(appkit): allow undefined values in IGenieConfig.spaces#322

Open
jamesbroadhead wants to merge 1 commit intomainfrom
genie-spaces-allow-undefined
Open

fix(appkit): allow undefined values in IGenieConfig.spaces#322
jamesbroadhead wants to merge 1 commit intomainfrom
genie-spaces-allow-undefined

Conversation

@jamesbroadhead
Copy link
Copy Markdown
Contributor

@jamesbroadhead jamesbroadhead commented Apr 28, 2026

Why

IGenieConfig.spaces is currently typed as Record<string, string>, but the plugin's runtime contract is already permissive of undefined values. defaultSpaces() constructs the same map and freely returns {} when the env var is unset:

private defaultSpaces(): Record<string, string> {
  const spaceId = process.env.DATABRICKS_GENIE_SPACE_ID;
  return spaceId ? { default: spaceId } : {};
}

private resolveSpaceId(alias: string): string | null {
  return this.config.spaces?.[alias] ?? null;
}

When env is unset, spaces.default is undefined at runtime; ?? null coalesces it; routes 404 with "Unknown space alias". That's the intended, working behaviour. The type just doesn't admit it.

The mismatch surfaces the moment a caller wants more than one named alias and reaches for the env var directly:

genie({ spaces: { wanderbricks: process.env.DATABRICKS_GENIE_SPACE_ID } })
error TS2322: Type 'string | undefined' is not assignable to type 'string'.
  Type 'undefined' is not assignable to type 'string'.

The workarounds (?? '', as string, hoist-and-narrow with a runtime throw) all paper over a type that's stricter than the code it describes.

Change

spaces?: Record<string, string>spaces?: Record<string, string | undefined>. defaultSpaces() return type updated to match. resolveSpaceId is unchanged — ?? null already handles undefined values identically to missing keys.

Alternatives considered

  • Require callers to hoist + assert. Three lines per usage to fight a type that doesn't reflect the runtime. Rejected — perpetuates the lie.
  • Document as string in the README. Type assertions silently mask config bugs at the callsite without changing runtime behaviour. Rejected — same hazard, less honest.
  • Make spaces keys string and require non-empty values via Zod. Real validation, but a strictly bigger change and arguably the wrong layer — the plugin already returns 404 on miss, which is the validation step.

Compatibility

Non-breaking. Function parameter types of Record<K, T> are contravariant for callers: existing callers passing Record<string, string> (a stricter map) still satisfy Record<string, string | undefined> (a looser map). resolveSpaceId's string | null return is unchanged.

Test plan

  • pnpm -r typecheck passes
  • npx vitest run packages/appkit/src/plugins/genie/tests/genie.test.ts — 30 passed, includes a new test asserting an explicitly-undefined alias 404s the same as a missing key
  • npx biome check on touched files — clean
  • pnpm build — appkit + appkit-ui rebuild green

This pull request and its description were written by Isaac.

The literal `genie({ spaces: { wanderbricks: process.env.X } })` does not
typecheck under tsc strict: `process.env.X` is `string | undefined`, but
`spaces` was typed `Record<string, string>`. Callers had to hoist into a
local with a runtime check, or use a non-null assertion, even though the
plugin's own resolveSpaceId already coalesces missing entries to null.

Loosen the type to `Record<string, string | undefined>` so env-driven
configs work without ceremony. Resolution behaviour is unchanged: aliases
mapped to undefined values fall through to "Unknown space alias" the same
way as missing keys, since `?? null` already handled both cases. New test
locks this in.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
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.

1 participant