fix(archived-list): use the semantic-layers-aware label for the dataset type - #43465
Conversation
…et type The Recently Archived view hardcoded "Dataset" in its type map, so the type-filter option and the Type column ignored the SEMANTIC_LAYERS flag that renames the concept to "Datasource" everywhere else in the app (sc-117448, QA-reported). The map now holds label getters and the dataset entry delegates to the shared naming module (src/features/semanticLayers/label.ts), so the flag is read at render time and a future rename stays consistent automatically. Only display text changes: the ArchivedType identifiers, filter values, and requests are identical under both flag states. Docs: the Recently Archived page notes the flag-aware name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
change:frontend
Code Review Agent Run #65b0b6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The review comment correctly identifies an issue: the To resolve this, you should pass a dedicated plural label to the hook for error handling while continuing to use the singular label for UI display. You can update the Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/src/pages/ArchivedList/index.tsx |
Review-panel follow-ups: pin the Type column cell with a row-scoped assertion (the previous check was satisfiable by the Select's own rendered value), reset the flag mock in afterEach so a test that dies mid-body cannot leak SEMANTIC_LAYERS into later tests, and compare against FeatureFlag.SemanticLayers instead of a string literal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43465 +/- ##
=======================================
Coverage 78.85% 78.86%
=======================================
Files 2876 2876
Lines 164601 164603 +2
Branches 38015 38015
=======================================
+ Hits 129799 129806 +7
+ Misses 32355 32350 -5
Partials 2447 2447
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #c525d3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Thanks for this, @mikebridge — this is a clean, well-reasoned fix. I reviewed the two substantive commits (5208775 fix, c024fca tests); the head bf28feb8 is just an "Update branch" merge. Verdict: looks ready for a committer's attention. Notes below, none blocking.
The core question: did every call site get converted?
Yes — all four, including the one the diff context hides. TYPE_LABELS is read in exactly four places, and every one now invokes the getter:
index.tsx:172—useListViewResourcesingular label →TYPE_LABELS[type]()index.tsx:253—recoveredToast(...)→TYPE_LABELS[type]()index.tsx:312—Cell: () => TYPE_LABELS[type]()index.tsx:545— the Type-filter dropdownoptionsmap →TYPE_LABELS[option]()
That fourth one (availableTypes.map) is the easy one to miss — a Record<…, () => string> read without () renders as nothing / [object Function] and TS won't always catch it once it flows into ReactNode. You caught it. No fifth read exists.
The other checks
-
datasetLabelis a plain function, not a hook.src/features/semanticLayers/label.ts—export const datasetLabel = () => sl(t('Dataset'), t('Datasource')), whereslcallsisFeatureEnabled('SEMANTIC_LAYERS')at call time. So it's safe inside theuseMemo/column config (no rules-of-hooks violation) and it is not a module-load no-op. ✅ -
Incidental i18n win — real, and worth calling out. The old map evaluated
t('Chart')/t('Dashboard')/t('Dataset')at module scope, before the locale is initialized, so those three could render untranslated. Deferring all three to render time fixes that for chart and dashboard too, not just the dataset relabel. Nice bonus. -
No staleness risk on the hook label.
TYPE_LABELS[type]()at:172is evaluated at render time;SEMANTIC_LAYERSis a boot-time flag, so it cannot go stale within a session. Fine. -
Docs match the behavior.
recently-archived.mdxnow reads "Dataset (shown as Datasource when semantic layers are enabled)" — flag-aware, not hardcoded. ✅
Test adequacy (one LOW note)
The two new tests are genuinely good: they exercise both flag states, and each asserts both the dropdown option label (:545) and the Type-column cell (:312) — including the nice touch of scoping the cell assertion to the row (within(datasetRow)) so it can't accidentally match the Select's own rendered value. The afterEach flag-mock restore is the right defensive move for shared module state.
LOW — two of the four call sites aren't covered. The recoveredToast label (:253) and the useListViewResource error-label (:172) are only reachable via a restore-success toast and a fetch-error path, neither of which these tests drive. Both are correctly converted here, so there's no bug — but if a future edit dropped the () on either line, the suite would stay green. A one-line assertion on the restore toast text (there's already a restore test at :188) would close that gap. Not required for this PR.
CI (re-derived at review time via statusCheckRollup)
Your own suite is green on CI: PASS src/pages/ArchivedList/ArchivedList.test.tsx (19.3 s) on sharded-jest-tests (3). The one red leg on that shard is SqlEditor › enables the save dataset button when the latest query succeeded in SqlLab/.../SqlEditor.test.tsx — a file this PR doesn't touch; it's an unrelated/flaky failure, not yours. A re-run should clear it. Seven legs (cypress, playwright, docker, storybook) were still running when I looked.
(Locally I INSPECTED rather than RAN — superset-frontend/node_modules isn't present in this worktree — so the "passing" claim rests on the CI shard log above, not a local run.)
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
On the Recently Archived (soft-delete recovery) view, the archived-object type was labeled "Dataset" — in the type-filter dropdown and the Type column — even on workspaces where the
SEMANTIC_LAYERSfeature flag renames the concept to "Datasource" everywhere else (the top nav reads "Datasources" while the archived row says "Dataset"). QA-reported cosmetic inconsistency (sc-117448).Root cause: the view's
TYPE_LABELSmap was a module-level constant withdataset: t('Dataset')hardcoded, bypassing the app's flag-aware naming module. The map now holds label getters (Record<ArchivedType, () => string>) and the dataset entry delegates to the existing shared helper (datasetLabelfromsrc/features/semanticLayers/label.ts), so the flag is read at render time and any future rename stays consistent automatically.Only display text changes: the
ArchivedTypeidentifiers, filter values, and API requests are byte-identical under both flag states (asserted in the tests — selecting the renamed option still drives the dataset endpoints). With the flag off, every string renders exactly as before.Docs: the Recently Archived page's Type-selector sentence now notes the flag-aware name.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (from the ticket, flag ON — nav says "Datasources", archived type says "Dataset"): see sc-117448's screenshot. After: the same two spots read "Datasource"; with the flag off, "Dataset" (unchanged). Text-only change; RTL tests pin both states.
TESTING INSTRUCTIONS
SOFT_DELETEandSEMANTIC_LAYERS(top nav shows Datasources); archive a dataset.SEMANTIC_LAYERS: both spots read Dataset, unchanged from before this PR.npm run test -- src/pages/ArchivedList— 23 tests including the two new flag-state tests (the flag-on assertion fails against the previous hardcoded map).ADDITIONAL INFORMATION
SEMANTIC_LAYERSfor the renamed label (SOFT_DELETEto reach the view); flag-off rendering unchanged🤖 Authored with the assistance of Claude (AI), directed and reviewed by @mikebridge.