codeant-ai-for-open-source[bot] commented on code in PR #41550:
URL: https://github.com/apache/superset/pull/41550#discussion_r3691512293
##########
superset/models/helpers.py:
##########
@@ -692,6 +692,31 @@ def _user(user: User) -> str:
return escape(user)
+def format_time_humanized(timestamp: datetime) -> str:
+ """Humanize *timestamp* against the server's naive-local clock.
+
+ Module-level rather than a mixin method so values projected outside an
+ entity (the archive list reads ``deleted_at`` in a bare column query) can
+ be humanized identically. The subtraction uses ``datetime.now()`` because
+ the audit columns and ``deleted_at`` are stamped with it; humanizing here,
+ on the clock that did the stamping, is what spares every client from
+ guessing the server's timezone.
+ """
+ locale = str(get_locale())
+ time_diff = datetime.now() - timestamp
+ # Skip activation for 'en' locale as it's humanize's default locale
+ if locale == "en":
+ return humanize.naturaltime(time_diff)
+ try:
+ humanize.i18n.activate(locale)
+ result = humanize.naturaltime(time_diff)
+ humanize.i18n.deactivate()
+ return result
Review Comment:
**Suggestion:** `humanize.i18n.activate` and `deactivate` modify
process-global locale state, so concurrent requests can interleave between
these calls and format a timestamp using another request's locale, or
deactivate a locale activated by another request. Protect the operation with
synchronization or use a locale-scoped API that does not mutate shared global
state. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Archived timestamps can appear in another user's language.
- ⚠️ Audit timestamp responses can receive incorrect translations.
- ⚠️ Concurrent localized requests can interfere with one another.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2c34f3fb7c23463e893a8d76e61417b6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2c34f3fb7c23463e893a8d76e61417b6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 711:714
**Comment:**
*Race Condition: `humanize.i18n.activate` and `deactivate` modify
process-global locale state, so concurrent requests can interleave between
these calls and format a timestamp using another request's locale, or
deactivate a locale activated by another request. Protect the operation with
synchronization or use a locale-scoped API that does not mutate shared global
state.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41550&comment_hash=580d93bda20c667ddb9fbf75a7910e92309cde6e1f7fd4143feb93d06df8d396&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41550&comment_hash=580d93bda20c667ddb9fbf75a7910e92309cde6e1f7fd4143feb93d06df8d396&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/ArchivedList/index.tsx:
##########
@@ -0,0 +1,558 @@
+/**
+ * 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 { useCallback, useMemo, useRef, useState } from 'react';
+import { useHistory } from 'react-router-dom';
+import { useAppSelector } from 'src/views/store';
+import { getClientErrorObject, SupersetClient } from '@superset-ui/core';
+import { t } from '@apache-superset/core/translation';
+import { styled } from '@apache-superset/core/theme';
+import {
+ ActionButton,
+ ConfirmStatusChange,
+ Select,
+ Tooltip,
+} from '@superset-ui/core/components';
+import { Icons } from '@superset-ui/core/components/Icons';
+import { useListViewResource } from 'src/views/CRUD/hooks';
+import {
+ ListView,
+ ListViewFilterOperator as FilterOperator,
+ type ListViewProps,
+ type ListViewFilters,
+} from 'src/components';
+import SubMenu from 'src/features/home/SubMenu';
+import withToasts from 'src/components/MessageToasts/withToasts';
+import { recoveredToast } from 'src/utils/softDeleteCopy';
+import { findPermission } from 'src/utils/findPermission';
+import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
+import {
+ ARCHIVED_TYPES,
+ ARCHIVED_TYPE_CONFIG,
+ type ArchivedItem,
+ type ArchivedType,
+} from './types';
+
+const PAGE_SIZE = 25;
+
+const TypeSelectRow = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.sizeUnit * 2}px ${theme.sizeUnit * 4}px;
+ width: 240px;
+ `}
+`;
+
+const StyledActions = styled.div`
+ ${({ theme }) => `
+ color: ${theme.colorIcon};
+
+ /* TableCollection hides .actions with opacity and reveals them on row
+ hover. Without a focus companion, tabbing lands on fully transparent
+ controls — and on this page recovering and permanently deleting are
+ the only actions there are. Scoped here rather than in the shared
+ component, which has the same gap on every list view. */
+ &:focus-within {
+ opacity: 1;
+ }
+ `}
+`;
+
+const EmptyStateRow = styled.div`
+ ${({ theme }) => `
+ padding: ${theme.sizeUnit * 6}px;
+ color: ${theme.colorTextSecondary};
+ `}
+`;
+
+const TYPE_LABELS: Record<ArchivedType, string> = {
+ chart: t('Chart'),
+ dashboard: t('Dashboard'),
+ dataset: t('Dataset'),
+};
+
+interface ToastProps {
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string, options?: { allowHtml?: boolean }) => void;
+}
+
+/** The per-row Recover + Delete-permanently actions. */
+function ArchivedRowActions({
+ item,
+ name,
+ onRestore,
+ onPurge,
+ busy = false,
+}: {
+ item: ArchivedItem;
+ name: string;
+ onRestore: (item: ArchivedItem) => void;
+ onPurge: (item: ArchivedItem) => void;
+ /** A request for this row is in flight; both actions stand down. */
+ busy?: boolean;
+}) {
+ return (
+ <StyledActions className="actions">
+ <ActionButton
+ label={t('Recover')}
+ tooltip={t('Recover this item')}
+ placement="bottom"
+ icon={<Icons.RollbackOutlined iconSize="l" />}
+ dataTest="archived-row-restore"
+ disabled={busy}
+ onClick={() => onRestore(item)}
+ />
+ <ConfirmStatusChange
+ title={t('Delete permanently %(name)s?', { name })}
+ description={t(
+ "If you delete this item, you won't be able to recover it.",
+ )}
+ onConfirm={() => onPurge(item)}
+ >
+ {confirmDelete => (
+ <ActionButton
+ label={t('Delete permanently')}
+ tooltip={t('Delete permanently')}
+ placement="bottom"
+ icon={<Icons.DeleteOutlined iconSize="l" />}
+ dataTest="archived-row-purge"
+ disabled={busy}
+ onClick={confirmDelete}
+ />
+ )}
+ </ConfirmStatusChange>
+ </StyledActions>
+ );
+}
+
+/**
+ * The per-type table body. Mounted with `key={type}` by the parent so the
+ * `useListViewResource` state and derived columns reset cleanly on a type
+ * switch. Sourced from the selected type's existing list endpoint with the
+ * soft-delete `<type>_deleted_state:only` baseline filter.
+ */
+function ArchivedListBody({
+ type,
+ addDangerToast,
+ addSuccessToast,
+}: ToastProps & { type: ArchivedType }) {
+ const config = ARCHIVED_TYPE_CONFIG[type];
+
+ const baseFilters = useMemo(
+ () => [{ id: 'id', operator: config.deletedStateOperator, value: 'only' }],
+ [config.deletedStateOperator],
+ );
+
+ const {
+ state: { loading, resourceCount, resourceCollection },
+ fetchData,
+ refreshData,
+ } = useListViewResource<ArchivedItem>(
+ config.resource,
+ TYPE_LABELS[type],
+ addDangerToast,
+ true,
+ [],
+ baseFilters,
+ );
+
+ // Restore is immediate (no confirm dialog). On success, refetch the full
page
+ // so the server-side count/pagination stays consistent and the row drops
out;
+ // on any error surface a danger toast and leave the row in place. The list
+ // read is already owner-scoped, so every visible row is restorable.
+ // A second activation while a request is in flight races the first: by the
+ // time the retry lands the row is already restored (or purged), so the
+ // server answers 404 and the user is shown a failure after a success. The
+ // ref is the guard rather than the state, because state updates are async
+ // and two quick clicks could both pass a state check; the state mirrors it
+ // so the buttons can render disabled meanwhile.
+ const inFlightRef = useRef<Set<string>>(new Set());
+ const [inFlight, setInFlight] = useState<readonly string[]>([]);
+
+ const beginAction = useCallback((uuid: string): boolean => {
+ if (inFlightRef.current.has(uuid)) {
+ return false;
+ }
+ inFlightRef.current.add(uuid);
+ setInFlight([...inFlightRef.current]);
+ return true;
+ }, []);
+
+ const endAction = useCallback((uuid: string) => {
+ inFlightRef.current.delete(uuid);
+ setInFlight([...inFlightRef.current]);
+ }, []);
+
+ const handleRestore = useCallback(
+ async (item: ArchivedItem) => {
+ const name = String(item[config.nameField] ?? '');
+ if (!beginAction(item.uuid)) {
+ return;
+ }
+ try {
+ await SupersetClient.post({
+ endpoint: `/api/v1/${config.resource}/${item.uuid}/restore`,
+ });
+ const { text, options } = recoveredToast(
+ name,
+ TYPE_LABELS[type],
+ item.url ?? item.explore_url,
+ );
+ addSuccessToast(text, options);
+ // Awaited so the finally's endAction does not re-enable this row's
+ // buttons while the stale, already-restored row is still rendered --
+ // a keyboard user could re-activate it and get a 404 after success.
+ await refreshData();
Review Comment:
**Suggestion:** The list hook handles fetch failures internally and resolves
its promise, so awaiting `refreshData()` does not guarantee that the row was
actually refreshed. If the refetch fails after a successful restore, this code
still clears the in-flight guard while leaving the restored row visible; a
subsequent Recover click sends a second request and produces a misleading 404
failure. Keep the row pending or explicitly handle refresh failure before
re-enabling its actions. [error handling]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Archived rows remain visible after successful recovery.
- ⚠️ Retry produces misleading restore failure toasts.
- ⚠️ Users may incorrectly believe recovery failed.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=371d184ad88c427fa0e39af2cbcc356e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=371d184ad88c427fa0e39af2cbcc356e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/ArchivedList/index.tsx
**Line:** 219:219
**Comment:**
*Error Handling: The list hook handles fetch failures internally and
resolves its promise, so awaiting `refreshData()` does not guarantee that the
row was actually refreshed. If the refetch fails after a successful restore,
this code still clears the in-flight guard while leaving the restored row
visible; a subsequent Recover click sends a second request and produces a
misleading 404 failure. Keep the row pending or explicitly handle refresh
failure before re-enabling its actions.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41550&comment_hash=60370169f3547adbf5455675d3d6f45c04f77fd342303ceb17065bf3b05f695b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41550&comment_hash=60370169f3547adbf5455675d3d6f45c04f77fd342303ceb17065bf3b05f695b&reaction=dislike'>👎</a>
--
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]
