Copilot commented on code in PR #64185:
URL: https://github.com/apache/airflow/pull/64185#discussion_r3066484334
##########
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:
New behavior is introduced here (falling back to `api.default_ui_log_source`
when the `log_source` search param is absent, plus special handling for the
"all" sentinel), but there are existing UI tests for this page
(`pages/TaskInstance/Logs/Logs.test.tsx`) and none cover the new fallback
behavior. Please add a regression test that asserts the configured default
source is applied when `log_source` is missing, and that setting the config to
represent "All Sources" does not filter out logs.
##########
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);
Review Comment:
`handleSourceChange` treats a cleared selection (`value` empty -> `val` is
`undefined`) the same as explicitly selecting the "All Sources" option (`val
=== "all"`). This forces `log_source=all` into the URL even when the user
clears the filter, which makes it impossible to fall back to the configured
default when the search param is empty. Consider handling `val === undefined`
separately (delete the param and do not append), and only append `all` when the
user explicitly selects the "All Sources" option.
```suggestion
searchParams.delete(SearchParamsKeys.SOURCE);
if (val === undefined && rest.length === 0) {
setSearchParams(searchParams);
return;
}
if ((val === "all" && rest.length === 0) || rest.includes("all")) {
searchParams.append(SearchParamsKeys.SOURCE, "all");
} else {
```
##########
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]
+ : ["all"];
Review Comment:
`defaultLogSource` is cast to `string | undefined`, but `useConfig()`
returns `ConfigResponse[key]` and the generated type for
`default_ui_log_source` includes `null`. If the API returns `null`, the current
checks will treat it as a valid value and end up passing `[null]` to the
Select, causing runtime/type issues. Avoid the cast and explicitly normalize
`null`/`""`/`"All Sources"` (and likely `"all"`) to the same "no override"
behavior.
```suggestion
const defaultLogSource = useConfig("default_ui_log_source");
const normalizedDefaultLogSource =
typeof defaultLogSource === "string" &&
defaultLogSource !== "" &&
defaultLogSource !== "All Sources" &&
defaultLogSource !== "all"
? defaultLogSource
: undefined;
const sourcesToSelect = sources.length > 0 ? sources :
normalizedDefaultLogSource !== undefined ? [normalizedDefaultLogSource] :
["all"];
```
--
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]
