Copilot commented on code in PR #64552:
URL: https://github.com/apache/airflow/pull/64552#discussion_r3025352720
##########
airflow-core/src/airflow/ui/src/theme.ts:
##########
@@ -406,16 +406,20 @@ const defaultAirflowTheme = {
export const createTheme = (userTheme?: Theme) => {
const defaultAirflowConfig = defineConfig({ theme: defaultAirflowTheme });
- const userConfig = defineConfig(
- userTheme
- ? {
- theme: { tokens: userTheme.tokens },
+ const userConfig = userTheme
+ ? defineConfig({
+ ...(userTheme.tokens !== undefined && {
+ theme: { tokens: userTheme.tokens as Record<string, unknown> },
+ }),
+ ...(userTheme.globalCss !== undefined && {
globalCss: userTheme.globalCss as Record<string, SystemStyleObject>,
- }
- : {},
- );
+ }),
Review Comment:
The conditional spreading pattern here (`...(userTheme.tokens !== undefined
&& {...})`) is a bit hard to read and mixes type assertions (`as Record<string,
unknown>`) into the config shape.
Consider building `userConfig` in a more explicit way (e.g. start with an
empty object and conditionally assign `theme.tokens` / `globalCss`) so it’s
easier to extend later (e.g. if icons get incorporated into theme config) and
avoids repeated casts.
##########
airflow-core/src/airflow/ui/src/theme.ts:
##########
@@ -406,16 +406,20 @@ const defaultAirflowTheme = {
export const createTheme = (userTheme?: Theme) => {
const defaultAirflowConfig = defineConfig({ theme: defaultAirflowTheme });
- const userConfig = defineConfig(
- userTheme
- ? {
- theme: { tokens: userTheme.tokens },
+ const userConfig = userTheme
+ ? defineConfig({
+ ...(userTheme.tokens !== undefined && {
+ theme: { tokens: userTheme.tokens as Record<string, unknown> },
+ }),
+ ...(userTheme.globalCss !== undefined && {
globalCss: userTheme.globalCss as Record<string, SystemStyleObject>,
- }
- : {},
- );
+ }),
+ })
Review Comment:
`Theme` from `openapi-gen` is now an index-signature (`{ [key: string]:
unknown }`). Accessing `userTheme.tokens` / `userTheme.globalCss` will fail
type-checking under TS `strict` +
`noPropertyAccessFromIndexSignature`-compatible patterns (and in general
removes useful typing).
To keep this compiling and retain type safety, consider defining a local
UI-facing type (e.g. `type UITheme = { tokens?: Record<string, unknown>;
globalCss?: Record<string, SystemStyleObject> }`) and narrow `userTheme` to
that shape before property access, or (preferably) fix the OpenAPI schema
generation so `Theme` has explicit optional properties again.
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -216,7 +216,11 @@ def serialize_model(self, handler: Any) -> dict:
class Theme(BaseModel):
"""JSON to modify Chakra's theme."""
- tokens: dict[Literal["colors"], ThemeColors]
+ tokens: dict[Literal["colors"], ThemeColors] | None = None
globalCss: dict[str, dict] | None = None
icon: ThemeIconType = None
icon_dark_mode: ThemeIconType = None
+
+ @model_serializer(mode="wrap")
+ def serialize_model(self, handler: Any) -> dict:
+ return {k: v for k, v in handler(self).items() if v is not None}
Review Comment:
The new `@model_serializer(mode="wrap")` on `Theme` appears to be collapsing
the generated OpenAPI schema for `Theme` into an unstructured
`additionalProperties: true` object (see updated `_private_ui.yaml` /
`schemas.gen.ts` / `types.gen.ts` where `Theme` becomes `[key: string]:
unknown`). This loses the documented/typed fields (`tokens`, `globalCss`,
`icon`, `icon_dark_mode`) and weakens the UI client typings.
Consider avoiding a wrap-level model serializer here and instead excluding
`None` fields in a way that preserves JSON schema (e.g. per-field serializers
that skip serialization when the value is `None`, or handling
`exclude_none=True` specifically when serializing the `theme` field in the
config response). Then regenerate the OpenAPI artifacts so `Theme` remains
structurally described.
```suggestion
model_config = ConfigDict(ser_json_exclude_none=True)
tokens: dict[Literal["colors"], ThemeColors] | None = None
globalCss: dict[str, dict] | None = None
icon: ThemeIconType = None
icon_dark_mode: ThemeIconType = None
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
