Re: [PR] Feat: add support for opening trigger modal via URL pre-populated [airflow]

["jscheffl (via GitHub)" <gi...@apache.org>]

jscheffl commented on code in PR #55277:
URL: https://github.com/apache/airflow/pull/55277#discussion_r2323505722


##########
airflow-core/src/airflow/ui/src/components/TriggerDag/TriggerDAGButton.tsx:
##########
@@ -31,9 +33,20 @@ type Props = {
   readonly withText?: boolean;
 };
 
+const TRIGGER_QUERY_KEY = "trigger";
+const isTruthy = (value: string | null) => value === "true" || value === "1";
+
 const TriggerDAGButton: React.FC<Props> = ({ dagDisplayName, dagId, isPaused, withText = true }) => {
   const { onClose, onOpen, open } = useDisclosure();
   const { t: translate } = useTranslation("components");
+  const { search } = useLocation();
+  const params = useMemo(() => new URLSearchParams(search), [search]);
+
+  useEffect(() => {

Review Comment:
   The usage of `useEffect()` is restricted and should only be used when there is no other alternative.



##########
airflow-core/src/airflow/ui/src/utils/query.ts:
##########
@@ -47,3 +47,45 @@ export const useAutoRefresh = ({ dagId, isPaused }: { dagId?: string; isPaused?:
   // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
   return (canRefresh ? autoRefreshInterval * 1000 : false) as number | false;
 };
+
+export const decodeParam = (params: URLSearchParams, key: string): string | null => {
+  const value = params.get(key);
+
+  if (value === null) {
+    return null;
+  }
+  try {
+    return decodeURIComponent(value);
+  } catch {
+    // Return original value if decoding fails
+    return value;
+  }
+};
+
+export const parseJsonSafe = (raw: string | null): string | null => {
+  if (raw === null) {
+    return null;
+  }
+  try {
+    const parsed: unknown = JSON.parse(raw);
+
+    return JSON.stringify(parsed, null, 2);
+  } catch {
+    // Return original string if parsing fails
+    return raw;

Review Comment:
   Why do you return the original if parsing fails? Then it would not be "safe" and some follow-up code will fail on this?



##########
airflow-core/src/airflow/ui/src/constants/reprocessBehaviourParams.ts:
##########
@@ -16,8 +16,23 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import type { ReprocessBehavior } from "openapi/requests/types.gen";
+
 export const reprocessBehaviors = [
   { label: "backfill.missingRuns", value: "none" },
   { label: "backfill.missingAndErroredRuns", value: "failed" },
   { label: "backfill.allRuns", value: "completed" },
 ];
+
+export const mapReprocessBehavior = (param: string): ReprocessBehavior => {

Review Comment:
   Why do you map the strings from the API to the user URL? Would it not be easier to use the same in the URL as well? Just probably needs some documentation so that users know about it.



##########
airflow-core/src/airflow/ui/src/constants/reprocessBehaviourParams.ts:
##########
@@ -16,8 +16,23 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import type { ReprocessBehavior } from "openapi/requests/types.gen";
+
 export const reprocessBehaviors = [
   { label: "backfill.missingRuns", value: "none" },
   { label: "backfill.missingAndErroredRuns", value: "failed" },
   { label: "backfill.allRuns", value: "completed" },
 ];
+
+export const mapReprocessBehavior = (param: string): ReprocessBehavior => {
+  switch (param) {
+    case "all_runs":
+      return "completed";
+    case "missing_and_errored_runs":
+      return "failed";
+    case "missing_runs":
+      return "none";
+    default:
+      return "none";

Review Comment:
   Default can be used for the missing as well
   ```suggestion
       default:
         return "none";
   ```



##########
airflow-core/src/airflow/ui/src/utils/query.ts:
##########
@@ -47,3 +47,45 @@ export const useAutoRefresh = ({ dagId, isPaused }: { dagId?: string; isPaused?:
   // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
   return (canRefresh ? autoRefreshInterval * 1000 : false) as number | false;
 };
+
+export const decodeParam = (params: URLSearchParams, key: string): string | null => {
+  const value = params.get(key);
+
+  if (value === null) {
+    return null;
+  }
+  try {
+    return decodeURIComponent(value);
+  } catch {
+    // Return original value if decoding fails
+    return value;

Review Comment:
   Same as below - why returning the original string if decoding fails? This would allow passing "bad" data further into later code and makes it very hard to understand bugs?



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org