mikebridge commented on PR #41551:
URL: https://github.com/apache/superset/pull/41551#issuecomment-5143606258
# Codex whole-system versioning capstone review
**Scope:** Current versioning working tree at `3b7a1fb944`, covering the
merged versioning backend and complete version-history frontend.
Soft-delete/purge was excluded because it was reviewed separately.
Ten review lenses completed: Clean Code, Continuous Delivery, CSS,
Domain-Driven Design, Preset, Python, React, SQLAlchemy, Superset Committer,
and Tidy First.
## Blockers
1. **Integer-ID reuse crosses entity identity boundaries**
`superset/versioning/queries.py:336`,
`superset/versioning/restore.py:138`
Version and restore queries match shadow rows using only reusable integer
IDs. A successor entity can inherit a hard-deleted predecessor's history and
restore its content. Pin every query to `(id, uuid)` and add ID-reuse
regression coverage.
**Sources:** Preset, Python, SQLAlchemy, DDD, Superset Committer
2. **Version-history UI and activity API remain outside an approved SIP**
`superset/config.py:728`
The committer review reports that SIP-210 remains unapproved, excludes
the UI, and does not cover `/activity/`. Amend it or create the follow-up SIP
and complete the Apache vote before upstream release.
**Source:** Superset Committer
3. **UI PR lacks required before/after evidence**
`.github/PULL_REQUEST_TEMPLATE.md:27`
Add screenshots or recordings covering the panel, preview, confirmation,
and restored result.
**Source:** Superset Committer
## High
1. **Entity-kind policy is scattered across parallel registries** —
`superset/versioning/activity/kinds.py:73`
Replace the parallel maps and dispatch tables with a typed
`EntityKindSpec` registry.
**Sources:** Clean Code, DDD
2. **Dashboard preview effect implements three interleaved state machines**
—
`superset-frontend/src/features/versionHistory/useDashboardVersionPreview.ts:220`
Extract explicit restore-reload, apply-preview, and exit-preview
operations with direct cancellation and cache ownership.
**Source:** Clean Code
3. **UI invents logical-save identity using a 60-second heuristic** —
`superset-frontend/src/features/versionHistory/grouping.ts:84`
Two genuine saves can be merged. Have the backend expose an authoritative
correlation identity.
**Source:** DDD
4. **Freshly captured baselines can immediately expire** —
`superset/versioning/baseline/insertion.py:120`
Historical `changed_on` is used as the retention timestamp, so old
entities can lose newly captured baselines during the next prune. It also mixes
local and UTC-naive clocks. Retain by capture time.
**Sources:** Preset, Python, SQLAlchemy
5. **Stable version resolution is discarded for an OFFSET lookup** —
`superset/versioning/queries.py:468`
A concurrent prune can shift the offset and return the wrong snapshot.
Fetch by the already-resolved transaction ID.
**Sources:** Preset, Python, SQLAlchemy
6. **Restore does not recheck dirty state when confirmation occurs** —
`superset-frontend/src/features/versionHistory/useVersionActions.tsx:142`
Work can become dirty after opening the modal and still be silently
discarded. Revalidate immediately before mutation.
**Source:** React
## Medium
1. Dashboard and Explore containers duplicate a lifecycle that has already
drifted — `DashboardVersionHistory.tsx:93`. **Sources:** Clean Code, Tidy First
2. Historical name and UUID resolution duplicate the same shadow-table
algorithm and query — `superset/versioning/activity/queries.py:620`.
**Source:** Clean Code
3. `describeRecord` is a growing conditional dispatch table —
`display.ts:206`. **Source:** Clean Code
4. Write-path capture has no latency metric despite relying on a
slowdown-triggered kill switch — `superset/versioning/changes/listener.py:430`.
**Source:** Continuous Delivery
5. Missing-schema failures silently discard semantic change records during
deployment — `superset/versioning/changes/listener.py:396`. **Source:**
Continuous Delivery
6. `height: 100vh` can make the history panel footer unreachable below
application chrome — `DashboardBuilder.tsx:159`. **Sources:** CSS, React
7. The fixed 320px dashboard panel has no narrow-viewport strategy —
`VersionHistoryPanel.tsx:45`. **Source:** CSS
8. Activity prose has separate server and client owners, leaving
related-event text unlocalizable —
`superset/versioning/activity/render.py:157`. **Source:** DDD
9. The activity API removes the natural record identity, forcing the UI to
hash mutable values — `superset/versioning/activity/render.py:196`. **Source:**
DDD
10. Restore events persist an unstable version ordinal as durable domain
text — `superset/commands/version_restore.py:119`. **Source:** DDD
11. Unchunked change-record queries can exceed bind limits and then silently
return empty changes — `superset/versioning/queries.py:268`. **Sources:**
Preset, Python, SQLAlchemy
12. `/versions/` exposes usernames contrary to the activity API and
established privacy posture — `superset/versioning/schemas.py:35`. **Sources:**
Preset, Python, Superset Committer
13. `/versions/` has no pagination or effective response ceiling —
`superset/versioning/api_helpers.py:222`. **Sources:** Preset, Superset
Committer
14. The change-record cap does not bound dashboard layout churn —
`superset/versioning/diff.py:256`. **Source:** Python
15. Dataset child-history tables lack parent-key composite indexes —
`2026-05-28_19-50_56cd24c07170_add_versioning_tables.py:439`. **Source:** Python
16. A fail-open comparison probe can poison the enclosing PostgreSQL
transaction because it lacks a savepoint —
`superset/versioning/factory.py:301`. **Source:** Python
17. Dashboard preview controls remain activatable through assistive or
synthetic interaction — `DashboardBuilder.tsx:325`. **Source:** React
18. The retention/activity integration test remains skipped even though the
implementation exists — `activity_view_tests.py:513`. **Source:** Tidy First
19. The restore/activity integration test is likewise obsoletely skipped —
`activity_view_tests.py:659`. **Source:** Tidy First
20. Banner and panel own independent mutation locks, permitting overlapping
operations — `PreviewBanner.tsx:58`. **Sources:** React, Tidy First
## Low
1. Some fail-open capture losses bypass the advertised error metrics —
`superset/versioning/changes/listener.py:215`. **Source:** Continuous Delivery
2. Timeline action styling is duplicated and has drifted —
`ActionRow.tsx:112`. **Source:** CSS
3. Icon-only history controls have undersized touch targets —
`VersionHistoryPanel.tsx:225`. **Source:** CSS
4. `column` means both dashboard-layout and dataset-schema columns —
`superset/versioning/diff.py:733`. **Source:** DDD
5. Partial-restore behavior depends on the prose sentinel `message !== 'OK'`
— `useVersionActions.tsx:169`. **Source:** Tidy First
6. Dashboard save-generation knowledge is duplicated —
`DashboardVersionHistory.tsx:138`. **Source:** Tidy First
**Summary:** 3 blockers, 6 high, 20 medium, 6 low. All ten reviewers
completed successfully. The strongest convergence was on the integer-ID reuse
defect, independently raised by five reviewers.
--
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]
