mikebridge commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3690781013
########## superset-frontend/src/features/versionHistory/grouping.ts: ########## @@ -0,0 +1,270 @@ +/** + * 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 type { + ActivityRecord, + RelatedEntry, + SaveGroup, + TimelineEntry, +} from './types'; + +/** + * Stable identity for one activity record, used to deduplicate rows + * when merging pages (offset pagination can re-serve rows if a new + * save lands between page fetches). + */ +export function recordKey(record: ActivityRecord): string { + return [ + record.transaction_id, + record.entity_kind, + record.entity_uuid ?? record.entity_name, + record.source, + record.kind, + record.operation, + JSON.stringify(record.path), + ].join('|'); +} + +/** Merge a newly fetched page into already loaded records, deduplicated. */ +export function mergeActivityPages( + existing: ActivityRecord[], + incoming: ActivityRecord[], +): ActivityRecord[] { + const seen = new Set(existing.map(recordKey)); + const merged = [...existing]; + incoming.forEach(record => { + const key = recordKey(record); + if (!seen.has(key)) { + seen.add(key); + merged.push(record); + } + }); + return merged; +} + +/** One related row per save of a related entity, regardless of how many + * change records that save produced. */ +export function relatedEntryKey(record: ActivityRecord): string { + return [ + record.transaction_id, + record.entity_kind, + record.entity_uuid ?? record.entity_name, + ].join('|'); +} + +/** Identity of the entity a related record belongs to, ignoring the + * transaction. */ +function relatedEntityKey(record: ActivityRecord): string { + return [record.entity_kind, record.entity_uuid ?? record.entity_name].join( + '|', + ); +} + +const RELATED_MERGE_WINDOW_MS = 60_000; + +/** + * The backend can split one logical save of a related entity into + * several transactions issued at (nearly) the same instant, which + * would render as duplicate-looking rows. Collapse adjacent related + * entries — no self save between them — for the same entity issued + * within a short window into the newer entry: its representative + * record is kept and the older save's records are absorbed so search + * over collapsed records keeps working. + */ +// TODO(version-history): backend workaround — remove when upstream emits +// one transaction per logical save. +function mergeAdjacentRelatedEntries( + entries: TimelineEntry[], +): TimelineEntry[] { + const merged: TimelineEntry[] = []; + entries.forEach(entry => { + const previous = merged[merged.length - 1]; + if ( + entry.type === 'related' && + previous?.type === 'related' && + relatedEntityKey(previous.record) === relatedEntityKey(entry.record) && + Math.abs( + Date.parse(previous.record.issued_at) - + Date.parse(entry.record.issued_at), + ) <= RELATED_MERGE_WINDOW_MS + ) { Review Comment: Acknowledged as a real trade-off, deliberately taken — not fixing client-side. The backend can split one logical save of a related entity into several transactions issued at nearly the same instant; without the merge window those render as duplicate-looking rows for a single user action, which misleads in the opposite direction. The code marks both this and the per-chart cascade rollup as `TODO(version-history): backend workaround`, and the 10-lens review posted above reached the same verdict (its "60-second wall-clock heuristic" finding): the correct fix is upstream — one transaction per logical save, and a dataset-level impact record instead of per-chart cascades — at which point both client heuristics get deleted rather than refined. Two genuine same-entity saves inside the window colliding is the known cost; the absorbed records remain searchable, so nothing is lost from the record, only from the row boundary. _Triaged by Claude (AI) on behalf of @mikebridge._ ########## superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx: ########## @@ -703,6 +829,44 @@ const DashboardBuilder = () => { className="dashboard-content" editMode={editMode} marginLeft={dashboardContentMarginLeft} + data-test="dashboard-grid-gate" + aria-disabled={isVersionPreviewActive} + previewGated={isVersionPreviewActive} + onKeyDownCapture={event => { + if (!isVersionPreviewActive) { + return; + } + // pointer-events: none leaves the subtree tab-focusable, so + // focus-navigation keys must pass through — swallowing Tab + // would trap keyboard focus inside the gated grid + // (WCAG 2.1.2). Activation/typing keys stay blocked outside + // the tab navs, which remain interactive during preview. + if (event.key === 'Tab' || event.key === 'Escape') { + return; + } + // Scrolling is not interaction. A mouse user can still scroll a + // gated preview with the wheel, so blocking the keyboard + // equivalent would leave a long dashboard unreadable by + // keyboard alone (WCAG 2.1.1). + if (SCROLL_KEYS.has(event.key)) { + return; + } + // Space scrolls too, but only where it would not also press + // something: the gate exists to stop activation, and + // pointer-events does not cover the keyboard. + if ( + event.key === ' ' && + !(event.target as HTMLElement).closest( + 'a, button, input, select, textarea, [role="button"], [tabindex]:not([tabindex="-1"])', + ) + ) { + return; + } + if (!(event.target as HTMLElement).closest('.ant-tabs-nav')) { + event.preventDefault(); + event.stopPropagation(); + } + }} Review Comment: Acknowledged — this is the known limitation of the grid gate's design, and the constraint that shaped it is worth stating: the tab strip must stay interactive during preview (tabbed dashboards are unnavigable otherwise), which rules out `inert` on the whole subtree — the approach the two filter-bar gates correctly use, since they have no carve-out. The keydown gate is the compromise: Tab/Escape pass (WCAG 2.1.2, no focus trap), scroll keys pass (WCAG 2.1.1, fixed earlier on this branch), activation is blocked. Focusable-but-inert controls during a preview are the residue. The structural fix — one shared `<PreviewGate>` component applying `inert` to the chart holders while leaving the tab strip live — is on the known-remaining list in the PR description alongside the gate-wrapper consolidation, planned for the post-merge cleanup batch rather than this PR. The affected surface only exists behind the default-off `VERSION_HISTORY` flag, and only while a preview is actively applied. _Triaged by Claude (AI) on behalf of @mikebridge._ -- 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]
