Copilot commented on code in PR #64185:
URL: https://github.com/apache/airflow/pull/64185#discussion_r3025333266
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -1791,6 +1791,14 @@ api:
type: boolean
example: ~
default: "False"
+ default_ui_log_source:
+ description: |
+ Default setting for log_source toggle in task logs web UI.
+ If not set, it defaults to All Sources. Optionally, you can set it to
any standard log source e.g., task, operator, task.stderr, etc.
+ version_added: 3.2.0
+ type: string
+ example: "task"
+ default: ""
Review Comment:
The description suggests users can set this to "All Sources", but the UI
logic appears to use the token `all` for that option. To avoid
confusion/misconfiguration, consider documenting the exact accepted values
(including `all` if supported) and clarifying that an empty value means “All
Sources”.
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/TaskLogHeader.tsx:
##########
@@ -123,15 +125,24 @@ export const TaskLogHeader = ({
if (((val === undefined || val === "all") && rest.length === 0) ||
rest.includes("all")) {
searchParams.delete(SearchParamsKeys.SOURCE);
+ searchParams.append(SearchParamsKeys.SOURCE, "all");
} else {
Review Comment:
`Select.Trigger` is marked `clearable`, but when the selection is cleared
(`value` becomes empty) this handler appends `SOURCE=all`. That prevents the
intended “no SOURCE param => fall back to api.default_ui_log_source” behavior,
and makes it impossible to return to the configured default via the UI.
Consider treating the empty selection case as “unset”: delete SOURCE keys and
do not append `all` unless the user explicitly picked the “All Sources” option.
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/Logs.tsx:
##########
@@ -82,6 +82,17 @@ export const Logs = () => {
const [fullscreen, setFullscreen] = useState(false);
const [expanded, setExpanded] = useState(false);
+ const defaultLogSource = useConfig("default_ui_log_source") as string |
undefined;
+
+ const sourceFilters =
+ sourceFiltersParam.length > 0
+ ? sourceFiltersParam.includes("all")
+ ? []
+ : sourceFiltersParam
+ : defaultLogSource !== undefined && defaultLogSource !== "" &&
defaultLogSource !== "All Sources"
+ ? [defaultLogSource]
Review Comment:
The fallback logic treats the string "All Sources" specially, but the UI’s
canonical token for “all sources” appears to be `"all"` (used in search params
and in the select collection). If an admin sets `api.default_ui_log_source =
all`, this will currently be treated as a concrete source filter and may break
filtering. Normalize the config value (e.g., treat `"all"`/"All Sources"/case
variants as “no filter”) before building `sourceFilters`.
```suggestion
const normalizedDefaultLogSource =
typeof defaultLogSource === "string" ?
defaultLogSource.trim().toLowerCase() : undefined;
const sourceFilters =
sourceFiltersParam.length > 0
? sourceFiltersParam.includes("all")
? []
: sourceFiltersParam
: normalizedDefaultLogSource &&
normalizedDefaultLogSource !== "all" &&
normalizedDefaultLogSource !== "all sources"
? [defaultLogSource as string]
```
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/Logs.tsx:
##########
@@ -82,6 +82,17 @@ export const Logs = () => {
const [fullscreen, setFullscreen] = useState(false);
const [expanded, setExpanded] = useState(false);
+ const defaultLogSource = useConfig("default_ui_log_source") as string |
undefined;
+
+ const sourceFilters =
+ sourceFiltersParam.length > 0
+ ? sourceFiltersParam.includes("all")
+ ? []
+ : sourceFiltersParam
+ : defaultLogSource !== undefined && defaultLogSource !== "" &&
defaultLogSource !== "All Sources"
+ ? [defaultLogSource]
+ : [];
Review Comment:
This introduces new behavior (derive `sourceFilters` from URL params *or*
`api.default_ui_log_source`, plus the special `SOURCE=all` override), but
`Logs.test.tsx` doesn’t currently cover any of these cases. Please add/extend
UI tests to verify: (1) no `SOURCE` param falls back to the configured default,
(2) `SOURCE=all` forces all sources even when a default is configured, and (3)
clearing the source selection reverts to the configured default (once the
handler logic is fixed).
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/TaskLogHeader.tsx:
##########
@@ -123,15 +125,24 @@ export const TaskLogHeader = ({
if (((val === undefined || val === "all") && rest.length === 0) ||
rest.includes("all")) {
searchParams.delete(SearchParamsKeys.SOURCE);
+ searchParams.append(SearchParamsKeys.SOURCE, "all");
} else {
searchParams.delete(SearchParamsKeys.SOURCE);
value
- .filter((state) => state !== "all")
- .map((state) => searchParams.append(SearchParamsKeys.SOURCE, state));
+ .filter((state: string) => state !== "all")
+ .forEach((state: string) =>
searchParams.append(SearchParamsKeys.SOURCE, state));
}
setSearchParams(searchParams);
};
+ const defaultLogSource = useConfig("default_ui_log_source") as string |
undefined;
+ const sourcesToSelect =
+ sources.length > 0
+ ? sources
+ : defaultLogSource !== undefined && defaultLogSource !== "" &&
defaultLogSource !== "All Sources"
+ ? [defaultLogSource]
Review Comment:
`useConfig("default_ui_log_source")` is typed as `string | null | undefined`
(generated OpenAPI type), but this code casts it to `string | undefined` and
doesn’t handle `null`. If the API returns `null`, the current check passes and
you can end up with `[null]` as a selected source/filter. Handle `null`
explicitly (treat it like unset) and avoid the unsafe cast.
```suggestion
const defaultLogSource = useConfig("default_ui_log_source");
const normalizedDefaultLogSource =
defaultLogSource === null ||
defaultLogSource === undefined ||
defaultLogSource === "" ||
defaultLogSource === "All Sources"
? undefined
: defaultLogSource;
const sourcesToSelect =
sources.length > 0
? sources
: normalizedDefaultLogSource !== undefined
? [normalizedDefaultLogSource]
```
--
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]
