-
Notifications
You must be signed in to change notification settings - Fork 4
fix(date-range): cast dates to YYYY-MM-DD when passing to python executor #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThe base input block converter adds a protected helper Sequence Diagram(s)sequenceDiagram
participant UI as UI/Converter
participant Base as BaseInputBlockConverter
participant Normalize as normalizeDateString
participant Block as DeepnoteBlock
rect rgb(230,240,255)
UI->>Base: applyChangesToBlock(block)
end
alt date block (single string)
Base->>Normalize: deepnote_variable_value (string)
Normalize-->>Base: normalized or unchanged string
Base->>Base: updateBlockMetadata(block, { deepnote_variable_value })
else date-range block (array of 2)
Base->>Normalize: each element of deepnote_variable_value
Normalize-->>Base: normalized or unchanged array
Base->>Base: updateBlockMetadata(block, { deepnote_variable_value })
end
Base->>Block: persist metadata, clear raw content key if needed
Note over Normalize,Base: Handles ISO, timezone-aware, relative, empty, mixed values
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
=====================================
Coverage 73% 73%
=====================================
Files 574 574
Lines 46751 46813 +62
Branches 5513 5519 +6
=====================================
+ Hits 34306 34358 +52
- Misses 10628 10637 +9
- Partials 1817 1818 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/notebooks/deepnote/converters/inputConverters.unit.test.ts (2)
703-785: Implementation bug causes timezone-dependent test failures; use UTC date methods.The review correctly identifies a real issue:
normalizeDateStringusesgetDate(),getMonth(),getFullYear()(local timezone methods) on UTC-parsed ISO strings. In non-UTC timezones (e.g., PST), "2025-09-30T00:00:00.000Z" extracts as "2025-09-29" instead of "2025-09-30".Fix in
src/notebooks/deepnote/converters/inputConverters.tslines 36–38: replacegetFullYear(),getMonth(),getDate()withgetUTCFullYear(),getUTCMonth(),getUTCDate().
853-954: Fix normalizeDateString to use UTC methods.The timezone bug is confirmed.
getFullYear(),getMonth(), andgetDate()return local time components, causing ISO UTC dates to be converted incorrectly. In non-UTC timezones, "2025-09-30T00:00:00.000Z" becomes "2025-09-29", breaking the test assertions at lines 871 and 914.Use
getUTCFullYear(),getUTCMonth(),getUTCDate()instead (inputConverters.ts line 36-38).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/notebooks/deepnote/converters/inputConverters.ts(3 hunks)src/notebooks/deepnote/converters/inputConverters.unit.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/converters/inputConverters.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/converters/inputConverters.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2546-2572)
src/notebooks/deepnote/converters/inputConverters.unit.test.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2546-2572)
🔇 Additional comments (3)
src/notebooks/deepnote/converters/inputConverters.ts (3)
58-83: LGTM!The centralized metadata update logic properly handles missing/invalid metadata while preserving existing valid values.
275-288: Logic is sound, but depends on timezone fix.The normalization flow correctly handles string values and fallbacks. However, it relies on
normalizeDateStringwhich has a timezone bug (see comment on lines 21-43).
310-324: Logic is sound, but depends on timezone fix.The normalization flow correctly handles array values, applies normalization to both elements, and falls back appropriately. However, it relies on
normalizeDateStringwhich has a timezone bug (see comment on lines 21-43).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/notebooks/deepnote/converters/inputConverters.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/notebooks/deepnote/converters/inputConverters.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2546-2572)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/notebooks/deepnote/converters/inputConverters.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/notebooks/deepnote/converters/inputConverters.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2546-2572)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
Summary by CodeRabbit
Bug Fixes
Tests