Repository navigation
Conversation
Native Time Range filters currently display evaluated ranges using raw ISO-style values (e.g. 2019-01-14), with no way to configure how they're shown. This adds an optional 'Display format' setting to the Time filter's Filter Settings tab, reusing the existing D3 time-format infrastructure (D3_TIME_FORMAT_OPTIONS/getTimeFormatter) that already powers chart-level Timestamp Format controls. The format is presentation-only: time_range serialization, query semantics, and DAYJS_FORMAT-based CustomFrame parsing are untouched. When unset, display behavior is byte-for-byte identical to today. Fixes apache#33724
Code Review Agent Run #f0663dActionable Suggestions - 0Additional Suggestions - 3
Review 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #44237 +/- ##
==========================================
- Coverage 80.25% 75.92% -4.33%
==========================================
Files 2927 2926 -1
Lines 173273 173048 -225
Branches 40173 40154 -19
==========================================
- Hits 139057 131394 -7663
- Misses 31621 38975 +7354
- Partials 2595 2679 +84
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:
|
…d extension contract Address review feedback on apache#44237: - DateFilterLabel.tsx: both fetchTimeRange() call sites lacked protection against out-of-order resolution. If value/displayFormat changed again before a prior request resolved, the older result could overwrite the current pill/tooltip/validation state. Add a shared monotonic request id (useRef) and ignore any response whose request id is no longer the latest. - ui-overrides/types.ts: the 'filter.dateFilterControl' extension point's DateFilterControlProps (a separate, independently maintained contract from the real component's props) did not declare displayFormat, so a custom registered DateFilterControl couldn't type or discover it. Add the field.
Code Review Agent Run #e6003fActionable 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.
Verified at 99bf4a7: the shared request-id guard regresses the pill on an ordinary Apply (reproduced; correct pre-PR). The rest is additive — no existing fetchTimeRange caller passes a 4th positional arg, and the formatDateEndpoint rewrite is behaviour-preserving.
… DateFilterLabel The stale-fetch guard added a single shared latestRequestId ref for both the value-driven fetch and the debounced draft-preview fetch. Editing the draft and clicking Apply before the debounce fired let the leftover draft fetch bump the shared counter, causing the Apply-triggered fetch's result to be discarded and the pill to keep showing the stale value. Split the guard into latestValueRequestId and latestDraftRequestId so each effect only guards against staleness within itself.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ate guard
Existing displayFormat tests only covered human-readable values ('Last
week', 'No filter') that short-circuit before reaching the date-format
branch, so swapping the isValid() check for an unconditional true left
every test green while unparseable endpoints rendered as 'Invalid Date'.
Add a case with an unparseable colon-separated range to exercise the
isValid() guard directly.
Neither TimeFilterPlugin nor getControlItemsMap had a test guarding the
displayFormat prop's path: deleting displayFormat={props.formData.displayFormat}
in TimeFilterPlugin, or dropping its exclusion from the generic
renderTrigger checkbox loop in getControlItemsMap, left every existing
test green.
Add a TimeFilterPlugin test asserting the prop reaches the underlying
date filter control, and a getControlItemsMap test asserting displayFormat
is excluded from the generic checkbox control items (it needs its own
custom control, not an auto-generated checkbox).
…Data The displayFormat wiring review only covered formData -> control (the TimeFilterPlugin prop pass-through); the first hop, controlValues -> formData via getFormData's generic spread, had no test. Dropping or excluding displayFormat from that spread would have passed every existing test silently.
|
The E2E jobs (cypress-matrix, playwright-tests) are failing fast (~90s) during the This PR touches zero migration files, and |
Code Review Agent Run #a27094Actionable Suggestions - 0Additional Suggestions - 1
Review 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.
The 8 red E2E legs are a genuine double alembic head (88a01c781622, c7f53d184ea2) inherited from master — 4dc0b9fe0 had one. Not yours to fix and a re-run won't clear it, though enforce-single-migration-head passes anyway.
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
LGTM, thanks for this feature Ujjwal, and for the quick, thorough follow ups on every review thread.
I checked out bbb537c and verified it locally rather than just reading the diff:
- All six touched jest suites pass (
fetchTimeRange, the fullDateFilterControl/testsdirectory,TimeFilterPlugin,getControlItemsMap,nativeFilters/utils). - The new coverage is non vacuous. I mutation tested it: swapping
extendedDayjs.utc()for local time parsing, dropping thedisplayFormatprop inTimeFilterPlugin, re sharing the request id counter between the two fetch effects, and relaxing theisValid()guard each make exactly one of the new tests fail. - This stays presentation only:
onSaveemits the rawtimeRangeValueand the data mask carries onlytime_range, soextra_form_dataand query semantics are untouched. - Backward compat and export/import are safe: with
displayFormatunset the calls collapse to the pre PR defaults, andcontrolValuesround trips throughnative_filter_configuration(fields.List(fields.Dict())insuperset/dashboards/schemas.py) with no schema change needed.
The red E2E legs are the master inherited double alembic head Amin already diagnosed, not this change.
SUMMARY
Native Time Range filters currently display evaluated ranges using raw ISO-style values (e.g.
2019-01-14), with no way to configure how they're shown. Chart-levelTimestamp Formatdoes not flow into native-filter rendering — a native filter is not owned by any single chart, so inheriting one chart's format isn't architecturally sound.This adds an optional, filter-level Display format setting to the Time filter's Filter Settings tab. It reuses the existing D3 time-format infrastructure (
D3_TIME_FORMAT_OPTIONS/getTimeFormatter) that already powers chart-levelTimestamp Formatcontrols (e.g. the Table viz'stable_timestamp_format) — no new formatting system is introduced.Design notes:
Last weekorprevious calendar monthare never passed through the formatter.time_rangeserialization, query semantics, and theDAYJS_FORMAT-basedCustomFramedatetime picker are untouched — this is a presentation-only change.extendedDayjs.utc()(not local-time parsing) before formatting, so the formatted output preserves the original wall-clock digits regardless of the browser's timezone.Fixes #33724
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not included in this PR — happy to add one if a reviewer would like to see the new "Display format" field in the Filter Settings tab.
TESTING INSTRUCTIONS
%d-%m-%Y %H:%M:%S.Automated coverage:
superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts— custom format applied to both range endpoints,-∞/∞placeholders preserved, human-readable values left untouched, end-to-endfetchTimeRange()with a format, and a UTC-safety regression test (verified non-vacuous: reverting the UTC-safe parsing to local-time parsing makes this test fail with a reproducible timezone-shifted value under the suite's fixedAmerica/New_Yorktest timezone).superset-frontend/src/explore/components/controls/DateFilterControl/tests/DateFilterLabel.test.tsx— the configured format reaches the evaluated-range tooltip while the human-readable pill stays untouched.All 83 tests pass.
npx oxlint/npx oxfmtpass with no new warnings on the changed files.npm run typesurfaces pre-existingTS6305"stale project-reference build output" errors across the whole repo (reproducible on unrelated files on a clean checkout without this change) — unrelated to this PR.ADDITIONAL INFORMATION