Adds date / time data type#527
Conversation
project_bootstrap / desktop_index import ./all_css, which Rolldown emits as all_css.css; keep Flask asset naming consistent with mdv.css. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for mdv-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughDate-valued columns are ingested as UTC epoch-day doubles with ChangesDate column support
CSS asset mapping
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Source
participant MDVProject
participant DataStore
participant SelectionDialog
participant Chart
Source->>MDVProject: submit datetime columns
MDVProject->>DataStore: store epoch-day doubles and date metadata
DataStore->>SelectionDialog: provide date column and numeric range
SelectionDialog->>DataStore: apply parsed date bounds
DataStore->>Chart: provide date column values
Chart->>Chart: render ISO date ticks and labels
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/charts/SVGChart.js (1)
129-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
isDateColumnhelper to centralize date detection.All three files manually check
is_date || date_unit === "days". To prevent divergence, import and use the sharedisDateColumnhelper fromsrc/lib/dateFormat.ts.
src/charts/SVGChart.js#L129-L131: replace the manual check withif (isDateColumn(col)) { return col; }.src/charts/WGLScatterPlot.js#L274-L275: replace withconst xIsDate = isDateColumn(xCol);andconst yIsDate = isDateColumn(yCol);.src/utilities/Color.js#L243-L243: replace the ternary condition withisDateColumn(c).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/charts/SVGChart.js` around lines 129 - 131, Replace manual date detection with the shared isDateColumn helper and import it from src/lib/dateFormat.ts: update SVGChart.js at lines 129-131 to use isDateColumn(col), WGLScatterPlot.js at lines 274-275 to derive xIsDate and yIsDate through isDateColumn, and Color.js at line 243 to use isDateColumn(c) in the ternary condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/charts/WGLScatterPlot.js`:
- Around line 505-522: Remove the entire updateAxis() override from
WGLScatterPlot. Rely on the inherited SVGChart.js implementation for date tick
formatting and axis updates, preserving smooth transitions for non-date X and Y
axes.
- Around line 270-289: Extract the date-specific axis logic from the constructor
into a _applyDateAxisDefaults() helper, preserving the existing date detection,
log-scale disabling, and minimum axis sizes. Call this helper from both the
constructor and drawChart(), after the dynamic x/y parameters are assigned, so
Settings-driven column changes apply the defaults before rendering.
---
Nitpick comments:
In `@src/charts/SVGChart.js`:
- Around line 129-131: Replace manual date detection with the shared
isDateColumn helper and import it from src/lib/dateFormat.ts: update SVGChart.js
at lines 129-131 to use isDateColumn(col), WGLScatterPlot.js at lines 274-275 to
derive xIsDate and yIsDate through isDateColumn, and Color.js at line 243 to use
isDateColumn(c) in the ternary condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: beb45690-7222-42dd-9102-c2eb76a810c3
📒 Files selected for processing (30)
docs/TABLE_CHART_REACT.mddocs/extradocs/datasource.mddocs/jsdocs/extradocs/datasource.mdpython/mdvtools/mdvproject.pypython/mdvtools/tests/test_date_columns.pysrc/charts/BaseChart.tssrc/charts/SVGChart.jssrc/charts/WGLScatterPlot.jssrc/charts/charts.d.tssrc/charts/schemas/DataSourceSchema.tssrc/datastore/DataStore.jssrc/lib/dateFormat.tssrc/react/components/AxisComponent.tsxsrc/react/components/HistogramWidget.tsxsrc/react/components/SelectionDialogComponent.tsxsrc/react/components/legend/ColorLegend.tsxsrc/react/components/legend/LegendContinuousSvg.tsxsrc/react/legend/color_legend/buildColorLegendSpec.tssrc/react/legend/color_legend/types.tssrc/react/legend/shared/legendTypes.tssrc/react/legend/shared/legendUtils.tssrc/react/utils/valueReplacementUtil.tssrc/tests/dateAxisTicks.spec.tssrc/tests/dateColumnDisplay.spec.tssrc/tests/dateFormat.spec.tssrc/tests/histogramBrushRange.spec.tssrc/tests/react/legend/legendUtils.test.tssrc/tests/table_react/utils/valueReplacementUtils.test.tsxsrc/utilities/Color.jsvite.config.mts
| // Date tick labels are longer than day numbers — give axes a bit more room. | ||
| // Log scales are not meaningful for calendar days. | ||
| const xCol = this.dataStore.columnIndex[this.x]; | ||
| const yCol = this.dataStore.columnIndex[this.y]; | ||
| const xIsDate = xCol?.is_date || xCol?.date_unit === "days"; | ||
| const yIsDate = yCol?.is_date || yCol?.date_unit === "days"; | ||
| if (xIsDate && this.config.axis) { | ||
| this.config.axis.x_log_scale = false; | ||
| if (this.config.axis.x && (!this.config.axis.x.size || this.config.axis.x.size < 40)) { | ||
| this.config.axis.x.size = 40; | ||
| this.setAxisSize("x", 40); | ||
| } | ||
| } | ||
| if (yIsDate && this.config.axis) { | ||
| this.config.axis.y_log_scale = false; | ||
| if (this.config.axis.y && (!this.config.axis.y.size || this.config.axis.y.size < 45)) { | ||
| this.config.axis.y.size = 45; | ||
| this.setAxisSize("y", 45); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply date-specific axis adjustments dynamically.
Disabling the log scale and increasing the axis size for date columns is only performed in the constructor. If a user dynamically changes the X or Y parameter to a date column via the Settings dialog, drawChart() is called to rebuild the graph, but it does not execute these adjustments. As a result, the new date column may render with an incompatible log scale and clipped labels.
💡 Proposed fix to apply defaults dynamically
Extract this logic into a helper method and call it in both the constructor and drawChart(). Note: you can also import and use isDateColumn here to simplify the condition.
- // Date tick labels are longer than day numbers — give axes a bit more room.
- // Log scales are not meaningful for calendar days.
- const xCol = this.dataStore.columnIndex[this.x];
- const yCol = this.dataStore.columnIndex[this.y];
- const xIsDate = xCol?.is_date || xCol?.date_unit === "days";
- const yIsDate = yCol?.is_date || yCol?.date_unit === "days";
- if (xIsDate && this.config.axis) {
- this.config.axis.x_log_scale = false;
- if (this.config.axis.x && (!this.config.axis.x.size || this.config.axis.x.size < 40)) {
- this.config.axis.x.size = 40;
- this.setAxisSize("x", 40);
- }
- }
- if (yIsDate && this.config.axis) {
- this.config.axis.y_log_scale = false;
- if (this.config.axis.y && (!this.config.axis.y.size || this.config.axis.y.size < 45)) {
- this.config.axis.y.size = 45;
- this.setAxisSize("y", 45);
- }
- }Add the helper method:
_applyDateAxisDefaults() {
if (!this.config.axis) return;
const xCol = this.dataStore.columnIndex[this.x];
const yCol = this.dataStore.columnIndex[this.y];
if ((xCol?.is_date || xCol?.date_unit === "days")) {
this.config.axis.x_log_scale = false;
if (this.config.axis.x && (!this.config.axis.x.size || this.config.axis.x.size < 40)) {
this.config.axis.x.size = 40;
this.setAxisSize("x", 40);
}
}
if ((yCol?.is_date || yCol?.date_unit === "days")) {
this.config.axis.y_log_scale = false;
if (this.config.axis.y && (!this.config.axis.y.size || this.config.axis.y.size < 45)) {
this.config.axis.y.size = 45;
this.setAxisSize("y", 45);
}
}
}Then call this._applyDateAxisDefaults() inside both the constructor (where the old code was) and inside drawChart() (e.g. immediately after this.y = this.config.param[1];).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/charts/WGLScatterPlot.js` around lines 270 - 289, Extract the
date-specific axis logic from the constructor into a _applyDateAxisDefaults()
helper, preserving the existing date detection, log-scale disabling, and minimum
axis sizes. Call this helper from both the constructor and drawChart(), after
the dynamic x/y parameters are assigned, so Settings-driven column changes apply
the defaults before rendering.
| updateAxis() { | ||
| super.updateAxis(); | ||
| // Classic WGL scatters redraw axes often on pan/zoom; re-assert date tick | ||
| // labels so d3 does not fall back to numeric thousands separators. | ||
| if (this._isLinearScale(this.x_scale) && this.x_axis_call) { | ||
| this._applyDateTickFormat("x", this.x_axis_call); | ||
| this.x_axis_svg.call(this.x_axis_call); | ||
| } | ||
| if ( | ||
| this._isLinearScale(this.y_scale) && | ||
| this.y_axis_call && | ||
| this._getDateColumnForAxis("y") | ||
| ) { | ||
| this._applyDateTickFormat("y", this.y_axis_call); | ||
| this.y_axis_svg.call(this.y_axis_call); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the redundant and buggy updateAxis override.
This override appears to be an artifact intended to fix date labels falling back to thousands separators. However, the date formatting is already correctly and natively re-applied by SVGChart.js in its own updateAxis() on every update.
Furthermore, because the X-axis check misses the _getDateColumnForAxis("x") guard, this override unconditionally calls this.x_axis_svg.call(this.x_axis_call) for all non-date X axes. This unintentionally interrupts and cancels the smooth D3 pan/zoom transition that super.updateAxis() initiates, causing non-date X axes to snap instantly while non-date Y axes transition gracefully.
🗑️ Proposed fix
Since SVGChart.js already perfectly handles the date axis formatting for all subclasses, you can safely delete this entire method to restore correctness and consistent transition behavior.
- updateAxis() {
- super.updateAxis();
- // Classic WGL scatters redraw axes often on pan/zoom; re-assert date tick
- // labels so d3 does not fall back to numeric thousands separators.
- if (this._isLinearScale(this.x_scale) && this.x_axis_call) {
- this._applyDateTickFormat("x", this.x_axis_call);
- this.x_axis_svg.call(this.x_axis_call);
- }
- if (
- this._isLinearScale(this.y_scale) &&
- this.y_axis_call &&
- this._getDateColumnForAxis("y")
- ) {
- this._applyDateTickFormat("y", this.y_axis_call);
- this.y_axis_svg.call(this.y_axis_call);
- }
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updateAxis() { | |
| super.updateAxis(); | |
| // Classic WGL scatters redraw axes often on pan/zoom; re-assert date tick | |
| // labels so d3 does not fall back to numeric thousands separators. | |
| if (this._isLinearScale(this.x_scale) && this.x_axis_call) { | |
| this._applyDateTickFormat("x", this.x_axis_call); | |
| this.x_axis_svg.call(this.x_axis_call); | |
| } | |
| if ( | |
| this._isLinearScale(this.y_scale) && | |
| this.y_axis_call && | |
| this._getDateColumnForAxis("y") | |
| ) { | |
| this._applyDateTickFormat("y", this.y_axis_call); | |
| this.y_axis_svg.call(this.y_axis_call); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/charts/WGLScatterPlot.js` around lines 505 - 522, Remove the entire
updateAxis() override from WGLScatterPlot. Rely on the inherited SVGChart.js
implementation for date tick formatting and axis updates, preserving smooth
transitions for non-date X and Y axes.
2 participants
Adds real date-column support end to end. Datetimes ingested from pandas/polars are stored as numeric days since the Unix epoch, tagged with is_date, and shown as YYYY-MM-DD in tables, tooltips, axes, color legends, Selection Dialog filters, and Find/Replace — while charts and filters keep using the underlying day numbers so sorting and brushing stay chronological.
Summary by CodeRabbit
New Features
YYYY-MM-DDin tables, charts, axes, legends, filters, histograms, and value replacement.Documentation
Bug Fixes