codeant-ai-for-open-source[bot] commented on code in PR #36368: URL: https://github.com/apache/superset/pull/36368#discussion_r2729936260
########## superset/daos/tasks.py: ########## @@ -0,0 +1,416 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Task DAO for Global Task Framework (GTF)""" + +import logging +import uuid +from datetime import datetime +from typing import Any + +from superset_core.api.tasks import TaskScope, TaskStatus + +from superset.daos.base import BaseDAO +from superset.daos.exceptions import DAOCreateFailedError, DAODeleteFailedError +from superset.extensions import db +from superset.models.task_subscribers import TaskSubscriber +from superset.models.tasks import Task +from superset.tasks.filters import TaskFilter +from superset.tasks.types import TaskProperties +from superset.tasks.utils import get_active_dedup_key +from superset.utils.core import get_user_id +from superset.utils.decorators import transaction + +logger = logging.getLogger(__name__) + + +class TaskDAO(BaseDAO[Task]): + """ + Concrete TaskDAO for the Global Task Framework (GTF). + + Provides database access operations for async tasks including + creation, status management, filtering, and subscription management + for shared tasks. + """ + + base_filter = TaskFilter + + @classmethod + def find_by_task_key( + cls, + task_type: str, + task_key: str, + scope: TaskScope | str = TaskScope.PRIVATE, + user_id: int | None = None, + ) -> Task | None: + """ + Find active task by type, key, scope, and user. + + Uses dedup_key internally for efficient querying with a unique index. + Only returns tasks that are active (pending or in progress). + + Uniqueness logic by scope: + - private: scope + task_type + task_key + user_id + - shared/system: scope + task_type + task_key (user-agnostic) + + :param task_type: Task type to filter by + :param task_key: Task identifier for deduplication + :param scope: Task scope (private/shared/system) + :param user_id: User ID (required for private tasks) + :returns: Task instance or None if not found or not active + """ + dedup_key = get_active_dedup_key( + scope=scope, + task_type=task_type, + task_key=task_key, + user_id=user_id, + ) + + # Simple single-column query with unique index + return db.session.query(Task).filter(Task.dedup_key == dedup_key).one_or_none() + + @classmethod + @transaction() + def create_task( + cls, + task_type: str, + task_key: str | None = None, + scope: TaskScope | str = TaskScope.PRIVATE, + user_id: int | None = None, + payload: dict[str, Any] | None = None, + properties: TaskProperties | None = None, + **kwargs: Any, + ) -> Task: + """ + Create a new async task with scope-aware deduplication. + + For shared tasks, if a task with the same parameters already exists, + the current user is subscribed to it instead of creating a duplicate. + + :param task_type: Type of task to create + :param task_key: Optional task identifier for deduplication + :param scope: Task scope (private/shared/system), defaults to private + :param user_id: User ID creating the task (required for subscription) + :param payload: Optional user-defined context data (dict) + :param properties: Optional framework-managed runtime state (e.g., timeout) + :param kwargs: Additional task attributes + :returns: Created or existing Task instance + :raises DAOCreateFailedError: If duplicate private task exists + """ + from superset_core.api.tasks import TaskScope + + from superset.tasks.utils import get_active_dedup_key + + # Generate task_key if not provided + if task_key is None: + task_key = str(uuid.uuid4()) + + # Determine user_id early: use provided value or fall back to current user + effective_user_id = user_id if user_id is not None else get_user_id() + + # Build dedup_key for active task + dedup_key = get_active_dedup_key( + scope=scope, + task_type=task_type, + task_key=task_key, + user_id=effective_user_id, + ) + + # Check for existing active task using dedup_key + # Use effective_user_id consistently for both lookup and subscription + if existing := cls.find_by_task_key( + task_type, task_key, scope, effective_user_id + ): + # For shared tasks, subscribe user to existing task + if ( + scope == TaskScope.SHARED + and effective_user_id + and not existing.has_subscriber(effective_user_id) + ): + cls.add_subscriber(existing.id, effective_user_id) + logger.info( + "User %s subscribed to existing shared task: %s", + effective_user_id, + task_key, + ) + return existing + + # For private/system tasks or already subscribed, raise error + raise DAOCreateFailedError( + f"Task with key '{task_key}' already exists " + f"and is active (status: {existing.status})" + ) + + # Create new task with dedup_key + # Handle both TaskScope enum and string values + scope_value = scope.value if isinstance(scope, TaskScope) else scope + + # Note: properties is handled separately via update_properties() + # because it's a hybrid property with only a getter + task_data = { + "task_type": task_type, + "task_key": task_key, + "scope": scope_value, + "status": TaskStatus.PENDING.value, + "dedup_key": dedup_key, + **kwargs, + } + + # Handle payload - serialize to JSON if dict provided + if payload: + from superset.utils import json + + task_data["payload"] = json.dumps(payload) + + if effective_user_id is not None: + task_data["user_id"] = effective_user_id + + task = cls.create(attributes=task_data) + + # Set properties after creation (hybrid property with getter only) + if properties: + task.update_properties(properties) + + # Flush to get the task ID (auto-incremented primary key) + db.session.flush() + + # Auto-subscribe creator for all tasks (not just shared) + # This enables consistent subscriber display across all task types + if effective_user_id: + cls.add_subscriber(task.id, effective_user_id) + logger.info( + "Creator %s auto-subscribed to task: %s (scope: %s)", + effective_user_id, + task_key, + scope_value, + ) + + logger.info( + "Created new async task: %s (type: %s, scope: %s)", + task_key, + task_type, + scope_value, + ) + return task + + @classmethod + @transaction() + def abort_task(cls, task_uuid: str, skip_base_filter: bool = False) -> bool: + """ + Abort a task by UUID. + + Abort behavior by status: + - PENDING: Goes directly to ABORTED (always abortable) + - IN_PROGRESS with is_abortable=True: Goes to ABORTING + - IN_PROGRESS with is_abortable=False/None: Raises TaskNotAbortableError + - ABORTING: Returns True (idempotent) + - Finished statuses: Returns False + + For shared tasks, only aborts if: + - Admin is aborting (skip_base_filter=True), OR + - This is the last subscriber unsubscribing + + :param task_uuid: UUID of task to abort + :param skip_base_filter: If True, skip base filter (for admin abortions) + :returns: True if task was aborted/aborting, False if not found or finished + :raises TaskNotAbortableError: If in-progress task has no abort handler + """ + from superset.commands.tasks.exceptions import TaskNotAbortableError + + task = cls.find_one_or_none(skip_base_filter=skip_base_filter, uuid=task_uuid) + if not task: + return False + + # Already aborting - idempotent success + if task.status == TaskStatus.ABORTING.value: + logger.info("Task %s is already aborting", task_uuid) + return True + + # Already finished - cannot abort + if task.status not in [TaskStatus.PENDING.value, TaskStatus.IN_PROGRESS.value]: + return False + + # For shared tasks, check subscriber count + if task.is_shared and not skip_base_filter: + # Don't abort if there are still other subscribers + if task.subscriber_count > 0: + logger.info( + "Not aborting shared task %s - %d subscriber(s) remain", + task_uuid, + task.subscriber_count, + ) + return False + + # PENDING: Go directly to ABORTED + if task.status == TaskStatus.PENDING.value: + task.set_status(TaskStatus.ABORTED) + logger.info("Aborted pending task: %s (scope: %s)", task_uuid, task.scope) + return True + + # IN_PROGRESS: Check if abortable + if task.status == TaskStatus.IN_PROGRESS.value: + if task.properties.get("is_abortable") is not True: Review Comment: **Suggestion:** Accessing `task.properties.get(...)` without guarding against `task.properties` being None or a non-dict will raise an AttributeError at runtime when `properties` is missing or not a dict; ensure you handle None and non-dict property objects and treat `is_abortable` accordingly before calling `.get`. [null pointer] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ TaskDAO.abort_task raises unexpected AttributeError on abort. - ⚠️ UI/CLI abort action may surface HTTP 500 errors. - ⚠️ Admin abort workflows can be interrupted unexpectedly. ``` </details> ```suggestion props = task.properties or {} is_abortable = None if isinstance(props, dict): is_abortable = props.get("is_abortable") else: is_abortable = getattr(props, "is_abortable", None) if is_abortable is not True: ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create a task without properties by calling TaskDAO.create_task() in superset/daos/tasks.py (create_task is defined starting at the hunk around lines 90-110); do not pass the `properties` argument so Task.properties remains unset/None after creation. 2. Mark that task as in-progress (e.g., the worker sets task.status = TaskStatus.IN_PROGRESS in the DB, or update the DB row manually). This places the Task into the IN_PROGRESS branch. 3. Call TaskDAO.abort_task(task_uuid, skip_base_filter=False) from superset/daos/tasks.py (abort_task's IN_PROGRESS check executes at file location shown around lines 264-269). Execution reaches the code at superset/daos/tasks.py:265 that does `task.properties.get(...)`. 4. Observe an AttributeError: "'NoneType' object has no attribute 'get'" (or similar) because task.properties is None (or not a dict), causing the abort to raise an unexpected exception instead of raising TaskNotAbortableError or following the intended abort flow. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/daos/tasks.py **Line:** 265:265 **Comment:** *Null Pointer: Accessing `task.properties.get(...)` without guarding against `task.properties` being None or a non-dict will raise an AttributeError at runtime when `properties` is missing or not a dict; ensure you handle None and non-dict property objects and treat `is_abortable` accordingly before calling `.get`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## tests/unit_tests/tasks/test_utils.py: ########## @@ -330,3 +339,237 @@ def test_get_executor( ) assert executor_type == expected_executor_type assert executor == expected_executor + + [email protected]( + "scope,task_type,task_key,user_id,expected", + [ + # Private tasks with TaskScope enum + ( + TaskScope.PRIVATE, + "sql_execution", + "chart_123", + 42, + "private|sql_execution|chart_123|42", + ), + ( + TaskScope.PRIVATE, + "thumbnail_gen", + "dash_456", + 100, + "private|thumbnail_gen|dash_456|100", + ), + # Private tasks with string scope + ( + "private", + "api_call", + "endpoint_789", + 200, + "private|api_call|endpoint_789|200", + ), + # Shared tasks with TaskScope enum + ( + TaskScope.SHARED, + "report_gen", + "monthly_report", + None, + "shared|report_gen|monthly_report", + ), + ( + TaskScope.SHARED, + "export_csv", + "large_export", + 999, # user_id should be ignored for shared + "shared|export_csv|large_export", + ), + # Shared tasks with string scope + ( + "shared", + "batch_process", + "batch_001", + 123, # user_id should be ignored for shared + "shared|batch_process|batch_001", + ), + # System tasks with TaskScope enum + ( + TaskScope.SYSTEM, + "cleanup_task", + "daily_cleanup", + None, + "system|cleanup_task|daily_cleanup", + ), + ( + TaskScope.SYSTEM, + "db_migration", + "version_123", + 1, # user_id should be ignored for system + "system|db_migration|version_123", + ), + # System tasks with string scope + ( + "system", + "maintenance", + "nightly_job", + 2, # user_id should be ignored for system + "system|maintenance|nightly_job", + ), + ], +) +def test_get_active_dedup_key(scope, task_type, task_key, user_id, expected, mocker): + """Test get_active_dedup_key generates correct format for all scopes""" + # Mock get_user_id to return the specified user_id + mocker.patch("superset.utils.core.get_user_id", return_value=user_id) Review Comment: **Suggestion:** The test patches `superset.utils.core.get_user_id`, but the function under test likely imports `get_user_id` into `superset.tasks.utils`; patching the definition site won't replace the reference used by `get_active_dedup_key`. Patch the symbol where it's looked up (`superset.tasks.utils.get_user_id`) so the mock actually affects the function under test. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Dedup key tests fail in CI pipeline. - ⚠️ Test reliability and developer feedback impacted. ``` </details> ```suggestion mocker.patch("superset.tasks.utils.get_user_id", return_value=user_id) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open tests/unit_tests/tasks/test_utils.py and locate test_get_active_dedup_key (definition at lines 418-424). The test patches `superset.utils.core.get_user_id` at line 421. 2. Run pytest for that single test: pytest tests/unit_tests/tasks/test_utils.py::test_get_active_dedup_key. The test calls get_active_dedup_key at line 423. 3. If `get_active_dedup_key` (imported from superset.tasks.utils at line 29) resolves `get_user_id` from its own module (`superset.tasks.utils.get_user_id`), then patching `superset.utils.core.get_user_id` will not change the reference used by get_active_dedup_key. The test will therefore observe the real `get_user_id` behavior, not the mocked return_value, leading to assertion failures at tests/unit_tests/tasks/test_utils.py:424. 4. Reproduce reliably by replacing the patch target with `mocker.patch("superset.tasks.utils.get_user_id", return_value=user_id)` (the suggested improved_code) and re-running the test — the test will then observe the mocked user_id and pass. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/tasks/test_utils.py **Line:** 421:421 **Comment:** *Possible Bug: The test patches `superset.utils.core.get_user_id`, but the function under test likely imports `get_user_id` into `superset.tasks.utils`; patching the definition site won't replace the reference used by `get_active_dedup_key`. Patch the symbol where it's looked up (`superset.tasks.utils.get_user_id`) so the mock actually affects the function under test. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## tests/unit_tests/tasks/test_utils.py: ########## @@ -330,3 +339,237 @@ def test_get_executor( ) assert executor_type == expected_executor_type assert executor == expected_executor + + [email protected]( + "scope,task_type,task_key,user_id,expected", + [ + # Private tasks with TaskScope enum + ( + TaskScope.PRIVATE, + "sql_execution", + "chart_123", + 42, + "private|sql_execution|chart_123|42", + ), + ( + TaskScope.PRIVATE, + "thumbnail_gen", + "dash_456", + 100, + "private|thumbnail_gen|dash_456|100", + ), + # Private tasks with string scope + ( + "private", + "api_call", + "endpoint_789", + 200, + "private|api_call|endpoint_789|200", + ), + # Shared tasks with TaskScope enum + ( + TaskScope.SHARED, + "report_gen", + "monthly_report", + None, + "shared|report_gen|monthly_report", + ), + ( + TaskScope.SHARED, + "export_csv", + "large_export", + 999, # user_id should be ignored for shared + "shared|export_csv|large_export", + ), + # Shared tasks with string scope + ( + "shared", + "batch_process", + "batch_001", + 123, # user_id should be ignored for shared + "shared|batch_process|batch_001", + ), + # System tasks with TaskScope enum + ( + TaskScope.SYSTEM, + "cleanup_task", + "daily_cleanup", + None, + "system|cleanup_task|daily_cleanup", + ), + ( + TaskScope.SYSTEM, + "db_migration", + "version_123", + 1, # user_id should be ignored for system + "system|db_migration|version_123", + ), + # System tasks with string scope + ( + "system", + "maintenance", + "nightly_job", + 2, # user_id should be ignored for system + "system|maintenance|nightly_job", + ), + ], +) +def test_get_active_dedup_key(scope, task_type, task_key, user_id, expected, mocker): + """Test get_active_dedup_key generates correct format for all scopes""" + # Mock get_user_id to return the specified user_id + mocker.patch("superset.utils.core.get_user_id", return_value=user_id) + + result = get_active_dedup_key(scope, task_type, task_key) + assert result == expected + + +def test_get_active_dedup_key_private_requires_user_id(mocker): + """Test that private tasks require user_id from get_user_id()""" + # Mock get_user_id to return None + mocker.patch("superset.utils.core.get_user_id", return_value=None) Review Comment: **Suggestion:** Same issue as above: patching `superset.utils.core.get_user_id` in the private-requires test won't affect `get_active_dedup_key` if the function references `get_user_id` from `superset.tasks.utils`; patch the usage location instead. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Private-task user_id validation test fails. - ⚠️ CI test correctness and debugging time affected. ``` </details> ```suggestion mocker.patch("superset.tasks.utils.get_user_id", return_value=None) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open tests/unit_tests/tasks/test_utils.py and find test_get_active_dedup_key_private_requires_user_id (lines 427-433). The test intends to patch get_user_id to return None at line 430. 2. Run pytest for that test: pytest tests/unit_tests/tasks/test_utils.py::test_get_active_dedup_key_private_requires_user_id. The test expects get_active_dedup_key to raise ValueError at line 433. 3. If get_active_dedup_key uses `get_user_id` from superset.tasks.utils (imported into that module at import time), patching `superset.utils.core.get_user_id` will not affect the reference used by the function, so the test will not simulate a missing user and may fail (no ValueError raised). 4. Confirm by changing the patch target to `mocker.patch("superset.tasks.utils.get_user_id", return_value=None)` (the improved_code) and re-running the test — the test then correctly simulates no user_id and raises the expected ValueError. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/tasks/test_utils.py **Line:** 430:430 **Comment:** *Possible Bug: Same issue as above: patching `superset.utils.core.get_user_id` in the private-requires test won't affect `get_active_dedup_key` if the function references `get_user_id` from `superset.tasks.utils`; patch the usage location instead. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/pages/TaskList/index.tsx: ########## @@ -0,0 +1,605 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { SupersetClient } from '@superset-ui/core'; +import { t, useTheme } from '@apache-superset/core'; +import { useMemo, useCallback, useState } from 'react'; +import { Tooltip, Label, Modal, Checkbox } from '@superset-ui/core/components'; +import { + CreatedInfo, + ListView, + ListViewFilterOperator as FilterOperator, + type ListViewFilters, + FacePile, +} from 'src/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import withToasts from 'src/components/MessageToasts/withToasts'; +import SubMenu from 'src/features/home/SubMenu'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { createErrorHandler, createFetchRelated } from 'src/views/CRUD/utils'; +import TaskStatusIcon from 'src/features/tasks/TaskStatusIcon'; +import TaskPayloadPopover from 'src/features/tasks/TaskPayloadPopover'; +import TaskStackTracePopover from 'src/features/tasks/TaskStackTracePopover'; +import { formatDuration } from 'src/features/tasks/timeUtils'; +import { + Task, + TaskStatus, + TaskScope, + canAbortTask, + isTaskAborting, +} from 'src/features/tasks/types'; +import { isUserAdmin } from 'src/dashboard/util/permissionUtils'; +import getBootstrapData from 'src/utils/getBootstrapData'; + +const PAGE_SIZE = 25; + +interface TaskListProps { + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; + user: { + userId: string | number; + firstName: string; + lastName: string; + }; +} + +function TaskList({ addDangerToast, addSuccessToast, user }: TaskListProps) { + const { + state: { loading, resourceCount: tasksCount, resourceCollection: tasks }, + fetchData, + refreshData, + } = useListViewResource<Task>('task', t('task'), addDangerToast); + + // Get full user with roles to check admin status + const bootstrapData = getBootstrapData(); + const fullUser = bootstrapData?.user; + const isAdmin = useMemo(() => isUserAdmin(fullUser), [fullUser]); + + // State for cancel confirmation modal + const [cancelModalTask, setCancelModalTask] = useState<Task | null>(null); + const [forceCancel, setForceCancel] = useState(false); + + // Determine dialog message based on task context + const getCancelDialogMessage = useCallback((task: Task) => { + const isSharedTask = task.scope === TaskScope.Shared; + const subscriberCount = task.subscriber_count || 0; + const otherSubscribers = subscriberCount - 1; + + // If it's going to abort (private, system, or last subscriber) + if (!isSharedTask || subscriberCount <= 1) { + return t('This will cancel the task.'); + } + + // Shared task with multiple subscribers + return t( + "You'll be removed from this task. It will continue running for %s other subscriber(s).", + otherSubscribers, + ); + }, []); + + // Get force abort message for admin checkbox + const getForceAbortMessage = useCallback((task: Task) => { + const subscriberCount = task.subscriber_count || 0; + return t( + 'This will abort (stop) the task for all %s subscriber(s).', + subscriberCount, + ); + }, []); + + // Check if current user is subscribed to a task + const isUserSubscribed = useCallback( + (task: Task) => + task.subscribers?.some((sub: any) => sub.user_id === user.userId) ?? + false, + [user.userId], + ); + + // Check if force cancel option should be shown (for admins on shared tasks) + const showForceCancelOption = useCallback( + (task: Task) => { + const isSharedTask = task.scope === TaskScope.Shared; + // Show for admins on shared tasks: + // - If not subscribed: they can only abort (checkbox pre-checked, disabled) + // - If subscribed with multiple subscribers: they can choose + return isAdmin && isSharedTask; + }, + [isAdmin], + ); + + // Check if force cancel checkbox should be disabled (admin not subscribed) + const isForceCancelDisabled = useCallback( + (task: Task) => isAdmin && !isUserSubscribed(task), + [isAdmin, isUserSubscribed], + ); + + const handleTaskCancel = useCallback( + (task: Task, force: boolean = false) => { + SupersetClient.post({ + endpoint: `/api/v1/task/${task.uuid}/cancel`, + jsonPayload: force ? { force: true } : {}, + }).then( + ({ json }) => { + refreshData(); + const { action } = json as { action: string }; + if (action === 'aborted') { + addSuccessToast( + t('Task cancelled: %s', task.task_name || task.task_key), + ); + } else { + addSuccessToast( + t( + 'You have been removed from task: %s', + task.task_name || task.task_key, + ), + ); + } + }, + createErrorHandler(errMsg => + addDangerToast( + t('There was an issue cancelling the task: %s', errMsg), + ), + ), + ); + }, + [addDangerToast, addSuccessToast, refreshData], + ); + + // Handle opening the cancel modal - set initial forceCancel state + const openCancelModal = useCallback( + (task: Task) => { + // Pre-check force cancel if admin is not subscribed + const shouldPreCheck = isAdmin && !isUserSubscribed(task); + setForceCancel(shouldPreCheck); + setCancelModalTask(task); + }, + [isAdmin, isUserSubscribed], + ); + + // Handle modal confirmation + const handleCancelConfirm = useCallback(() => { + if (cancelModalTask) { + handleTaskCancel(cancelModalTask, forceCancel); + setCancelModalTask(null); + setForceCancel(false); + } + }, [cancelModalTask, forceCancel, handleTaskCancel]); + + // Handle modal close + const handleCancelModalClose = useCallback(() => { + setCancelModalTask(null); + setForceCancel(false); + }, []); + + const columns = useMemo( + () => [ + { + Cell: ({ + row: { + original: { task_name, task_key, uuid }, + }, + }: any) => { + // Display preference: task_name > task_key + const displayText = task_name || task_key; + const truncated = + displayText.length > 30 + ? `${displayText.slice(0, 30)}...` + : displayText; + + // Build tooltip with all identifiers + const tooltipLines = []; + if (task_name) tooltipLines.push(`Name: ${task_name}`); + tooltipLines.push(`Key: ${task_key}`); + tooltipLines.push(`UUID: ${uuid}`); + const tooltipText = tooltipLines.join('\n'); + + return ( + <Tooltip + title={ + <span style={{ whiteSpace: 'pre-line' }}>{tooltipText}</span> + } + placement="top" + > + <span>{truncated}</span> + </Tooltip> + ); + }, + accessor: 'task_name', + Header: t('Task'), + size: 'xl', + id: 'task', + }, + { + Cell: ({ + row: { + original: { status, properties, duration_seconds }, + }, + }: any) => ( + <TaskStatusIcon + status={status} + progressPercent={properties?.progress_percent} + progressCurrent={properties?.progress_current} + progressTotal={properties?.progress_total} + durationSeconds={duration_seconds} + errorMessage={properties?.error_message} + exceptionType={properties?.exception_type} + /> + ), + accessor: 'status', + Header: t('Status'), + size: 'xs', + id: 'status', + }, + { + accessor: 'task_type', + Header: t('Type'), + size: 'md', + id: 'task_type', + }, + { + Cell: ({ + row: { + original: { scope }, + }, + }: any) => { + const scopeConfig: Record< + TaskScope, + { label: string; type: 'default' | 'info' | 'warning' } + > = { + [TaskScope.Private]: { label: t('Private'), type: 'default' }, + [TaskScope.Shared]: { label: t('Shared'), type: 'info' }, + [TaskScope.System]: { label: t('System'), type: 'warning' }, + }; + + const config = scopeConfig[scope as TaskScope] || { + label: scope, + type: 'default' as const, + }; + + return <Label type={config.type}>{config.label}</Label>; + }, + accessor: 'scope', + Header: t('Scope'), + size: 'sm', + id: 'scope', + }, + { + Cell: ({ + row: { + original: { subscriber_count, subscribers }, + }, + }: any) => { + if (!subscribers || subscriber_count === 0) { + return '-'; + } + + // Convert subscribers to FacePile format + const users = subscribers.map((sub: any) => ({ + id: sub.user_id, + first_name: sub.first_name, + last_name: sub.last_name, + })); + + return <FacePile users={users} maxCount={3} />; + }, + accessor: 'subscriber_count', + Header: t('Subscribers'), + size: 'md', + id: 'subscribers', + disableSortBy: true, + }, + { + Cell: ({ + row: { + original: { + created_on_delta_humanized: createdOn, + created_by: createdBy, + }, + }, + }: any) => <CreatedInfo date={createdOn} user={createdBy} />, + Header: t('Created'), + accessor: 'created_on', + size: 'xl', + id: 'created_on', + }, + { + // Hidden column for filtering by created_by + accessor: 'created_by', + id: 'created_by', + hidden: true, + }, + { + Cell: ({ + row: { + original: { duration_seconds }, + }, + }: any) => formatDuration(duration_seconds) ?? '-', + accessor: 'duration_seconds', + Header: t('Duration'), + size: 'sm', + id: 'duration_seconds', + disableSortBy: true, + }, + { + Cell: ({ + row: { + original: { payload, properties, status }, + }, + }: any) => { + const theme = useTheme(); + const hasPayload = payload && Object.keys(payload).length > 0; + const hasStackTrace = !!properties?.stack_trace; + + // Show warning if timeout is set but no abort handler during execution + // Only show for IN_PROGRESS (abort handler registers at runtime, not during PENDING) + const hasTimeoutWithoutHandler = + status === TaskStatus.InProgress && + properties?.timeout && + !properties?.is_abortable; + + if (!hasPayload && !hasStackTrace && !hasTimeoutWithoutHandler) { + return null; + } + + return ( + <div style={{ display: 'flex', gap: '8px' }}> + {hasTimeoutWithoutHandler && ( Review Comment: **Suggestion:** A React hook (`useTheme()`) is being called inside a table cell render block rather than at the top level of the component; calling hooks inside nested render functions can violate the Rules of Hooks and lead to unpredictable behavior. Replace the cell's inline hook usage with a safe alternative (do not call `useTheme()` inside the Cell function). The minimal fix below removes the hook call from the Cell and falls back to the default icon rendering (no `iconColor`) so the component no longer relies on `useTheme` in that nested render. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Tasks list page fails to render when visited. - ❌ Console shows "Invalid hook call" error. - ⚠️ Task details tooltip icon color omitted. - ⚠️ Debugging time increases for frontend developers. ``` </details> ```suggestion const hasPayload = payload && Object.keys(payload).length > 0; const hasStackTrace = !!properties?.stack_trace; // Show warning if timeout is set but no abort handler during execution // Only show for IN_PROGRESS (abort handler registers at runtime, not during PENDING) const hasTimeoutWithoutHandler = status === TaskStatus.InProgress && properties?.timeout && !properties?.is_abortable; if (!hasPayload && !hasStackTrace && !hasTimeoutWithoutHandler) { return null; } return ( <div style={{ display: 'flex', gap: '8px' }}> {hasTimeoutWithoutHandler && ( <Tooltip title={t( 'Timeout configured (%s seconds) but no abort handler defined. ' + 'Task will continue running past the timeout.', properties.timeout, )} placement="top" > <span> <Icons.WarningOutlined iconSize="l" /> ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the frontend with this PR code and navigate to the Tasks page (component at `superset-frontend/src/pages/TaskList/index.tsx` — page mounted via `<SubMenu name={t('Tasks')} />` and `ListView` rendering the columns defined in this file). 2. During render, the columns array is iterated and the payload column's Cell renderer (defined in this file, see block starting at diff hunk line 338) is invoked by the ListView/table implementation. The Cell implementation calls `useTheme()` at the top of the function (`const theme = useTheme();` — added around diff line ~344). 3. React enforces the Rules of Hooks: hooks must be called from React function components or custom hooks at the top level. The Cell here is a plain render callback (not a React function component), so invoking `useTheme()` from it causes an invalid hook call. Reproduce: open Tasks page and observe console error "Invalid hook call. Hooks can only be called inside of the body of a function component." and broken/failed rendering of the Tasks list. 4. The improved approach is to move hook usage to the top-level TaskList component (e.g., call `const theme = useTheme();` near the top of `TaskList`), then reference `theme` inside the Cell, or convert the Cell renderer into a proper React component that can call hooks. The quick fix in the suggested improved_code avoids calling the hook inside the Cell (removing `useTheme()` usage), preventing the invalid hook call. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/pages/TaskList/index.tsx **Line:** 344:361 **Comment:** *Possible Bug: A React hook (`useTheme()`) is being called inside a table cell render block rather than at the top level of the component; calling hooks inside nested render functions can violate the Rules of Hooks and lead to unpredictable behavior. Replace the cell's inline hook usage with a safe alternative (do not call `useTheme()` inside the Cell function). The minimal fix below removes the hook call from the Cell and falls back to the default icon rendering (no `iconColor`) so the component no longer relies on `useTheme` in that nested render. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
