fix: preserve event duration when dragging ranged calendar events#8869
Open
hkarmoush wants to merge 1 commit into
Open
fix: preserve event duration when dragging ranged calendar events#8869hkarmoush wants to merge 1 commit into
hkarmoush wants to merge 1 commit into
Conversation
Dragging a calendar card with an End Date toggled silently did nothing. DateTypeOption::apply_changeset() rejects a changeset for an is_range date cell unless timestamp and end_timestamp are both present or both absent (added in AppFlowy-IO#6582 to catch accidental partial updates from the date picker). MoveCalendarEventPB only ever set timestamp, so the whole update was dropped for ranged events. Add DatabaseEditor::move_calendar_event(), which loads the existing cell first and, when it's a ranged date, shifts end_timestamp by the same delta as the new start timestamp so the event keeps its original duration instead of collapsing to a single day. Fixes AppFlowy-IO#8859 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Reviewer's GuideImplements a dedicated DatabaseEditor::move_calendar_event API so that dragging calendar events updates both start and end timestamps for ranged date cells while keeping single-date behavior unchanged, and wires the calendar event handler plus tests to validate the new behavior. Sequence diagram for moving a ranged calendar eventsequenceDiagram
actor User
participant UI as CalendarUI
participant Handler as move_calendar_event_handler
participant Editor as DatabaseEditor
participant DB as Database
User->>UI: Drag calendar event to new date
UI->>Handler: MoveCalendarEventPB(timestamp, cell_path)
Handler->>Editor: get_database_editor_with_view_id(view_id)
Handler->>Editor: move_calendar_event(view_id, row_id, field_id, new_timestamp)
Editor->>DB: get_cell(field_id, row_id)
DB-->>Editor: Cell
Editor->>Editor: DateCellData::from(Cell)
Editor->>Editor: [if is_range] compute end_timestamp
Editor->>DB: update_cell_with_changeset(view_id, row_id, field_id, DateCellChangeset)
DB-->>Editor: Ok
Editor-->>Handler: Ok
Handler-->>UI: Ok
UI-->>User: Event moved (duration preserved)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
hkarmoush
marked this pull request as ready for review
July 20, 2026 15:21
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
DatabaseEditor::move_calendar_event, theget_cellresult is only mapped and then used viafilter/and_then, which both operate onResultrather thanOption; consider usinglet old = DateCellData::from(&self.get_cell(field_id, row_id).await?);and then deriving anOption<i64>forend_timestampfromoldto both fix compilation and make the error propagation behavior explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DatabaseEditor::move_calendar_event`, the `get_cell` result is only mapped and then used via `filter`/`and_then`, which both operate on `Result` rather than `Option`; consider using `let old = DateCellData::from(&self.get_cell(field_id, row_id).await?);` and then deriving an `Option<i64>` for `end_timestamp` from `old` to both fix compilation and make the error propagation behavior explicit.
## Individual Comments
### Comment 1
<location path="frontend/rust-lib/flowy-database2/src/services/database/database_editor.rs" line_range="1405-1408" />
<code_context>
+ /// Moves a calendar event to `new_timestamp`. If the event's date cell spans a range
+ /// (start + end date), the end date is shifted by the same amount as the start date so the
+ /// event keeps its original duration, instead of collapsing to a single day.
+ #[tracing::instrument(level = "trace", skip_all)]
+ pub async fn move_calendar_event(
+ &self,
+ view_id: &str,
+ row_id: &RowId,
+ field_id: &str,
+ new_timestamp: i64,
+ ) -> FlowyResult<()> {
+ let old_cell_data = self
+ .get_cell(field_id, row_id)
+ .await
+ .map(|cell| DateCellData::from(&cell));
+ let end_timestamp = old_cell_data
</code_context>
<issue_to_address>
**suggestion:** Including `err` in the tracing instrumentation for `move_calendar_event` would make failures easier to diagnose.
`move_calendar_event` returns a `FlowyResult<()>` and can fail (e.g., during cell access or update). Adding `err` to the `#[tracing::instrument]` attribute, as in `notify_did_insert_database_field`, would auto-log these errors and keep diagnostics consistent for calendar move operations.
```suggestion
/// Moves a calendar event to `new_timestamp`. If the event's date cell spans a range
/// (start + end date), the end date is shifted by the same amount as the start date so the
/// event keeps its original duration, instead of collapsing to a single day.
#[tracing::instrument(level = "trace", skip_all, err)]
```
</issue_to_address>
### Comment 2
<location path="frontend/rust-lib/flowy-database2/tests/database/cell_test/test.rs" line_range="208-209" />
<code_context>
}
}
+
+#[tokio::test]
+async fn move_calendar_event_shifts_ranged_end_date_test() {
+ let test = DatabaseCellTest::new().await;
+ let date_field = test.get_first_field(FieldType::DateTime).await;
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for moving ranged events backward in time to cover negative deltas
Since `move_calendar_event` derives `end_timestamp` from `new_timestamp - old_timestamp`, please also add a test that moves a ranged event backward (e.g., `new_start = start - 86400`) and verifies both `timestamp` and `end_timestamp` shift back by one day. This will cover negative deltas and better protect against regressions in the delta application logic.
</issue_to_address>
### Comment 3
<location path="frontend/rust-lib/flowy-database2/tests/database/cell_test/test.rs" line_range="239-243" />
<code_context>
+ .await
+ .unwrap();
+
+ let cell = test.editor.get_cell(&date_field.id, &row_id).await.unwrap();
+ let cell_data = DateCellData::from(&cell);
+ assert_eq!(cell_data.timestamp, Some(new_start));
+ assert_eq!(cell_data.end_timestamp, Some(end + 86400));
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also asserting the `is_range` flag to fully validate the ranged behavior
Also assert that `cell_data.is_range` remains `true` after the move to verify the event is still treated as a ranged date cell and not collapsed into a single-date value.
```suggestion
let cell = test.editor.get_cell(&date_field.id, &row_id).await.unwrap();
let cell_data = DateCellData::from(&cell);
assert_eq!(cell_data.timestamp, Some(new_start));
assert_eq!(cell_data.end_timestamp, Some(end + 86400));
assert!(cell_data.is_range, "Moved calendar event should remain a ranged date cell");
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1405
to
+1408
| /// Moves a calendar event to `new_timestamp`. If the event's date cell spans a range | ||
| /// (start + end date), the end date is shifted by the same amount as the start date so the | ||
| /// event keeps its original duration, instead of collapsing to a single day. | ||
| #[tracing::instrument(level = "trace", skip_all)] |
Contributor
There was a problem hiding this comment.
suggestion: Including err in the tracing instrumentation for move_calendar_event would make failures easier to diagnose.
move_calendar_event returns a FlowyResult<()> and can fail (e.g., during cell access or update). Adding err to the #[tracing::instrument] attribute, as in notify_did_insert_database_field, would auto-log these errors and keep diagnostics consistent for calendar move operations.
Suggested change
| /// Moves a calendar event to `new_timestamp`. If the event's date cell spans a range | |
| /// (start + end date), the end date is shifted by the same amount as the start date so the | |
| /// event keeps its original duration, instead of collapsing to a single day. | |
| #[tracing::instrument(level = "trace", skip_all)] | |
| /// Moves a calendar event to `new_timestamp`. If the event's date cell spans a range | |
| /// (start + end date), the end date is shifted by the same amount as the start date so the | |
| /// event keeps its original duration, instead of collapsing to a single day. | |
| #[tracing::instrument(level = "trace", skip_all, err)] |
Comment on lines
+208
to
+209
| #[tokio::test] | ||
| async fn move_calendar_event_shifts_ranged_end_date_test() { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test for moving ranged events backward in time to cover negative deltas
Since move_calendar_event derives end_timestamp from new_timestamp - old_timestamp, please also add a test that moves a ranged event backward (e.g., new_start = start - 86400) and verifies both timestamp and end_timestamp shift back by one day. This will cover negative deltas and better protect against regressions in the delta application logic.
Comment on lines
+239
to
+243
| let cell = test.editor.get_cell(&date_field.id, &row_id).await.unwrap(); | ||
| let cell_data = DateCellData::from(&cell); | ||
| assert_eq!(cell_data.timestamp, Some(new_start)); | ||
| assert_eq!(cell_data.end_timestamp, Some(end + 86400)); | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider also asserting the is_range flag to fully validate the ranged behavior
Also assert that cell_data.is_range remains true after the move to verify the event is still treated as a ranged date cell and not collapsed into a single-date value.
Suggested change
| let cell = test.editor.get_cell(&date_field.id, &row_id).await.unwrap(); | |
| let cell_data = DateCellData::from(&cell); | |
| assert_eq!(cell_data.timestamp, Some(new_start)); | |
| assert_eq!(cell_data.end_timestamp, Some(end + 86400)); | |
| } | |
| let cell = test.editor.get_cell(&date_field.id, &row_id).await.unwrap(); | |
| let cell_data = DateCellData::from(&cell); | |
| assert_eq!(cell_data.timestamp, Some(new_start)); | |
| assert_eq!(cell_data.end_timestamp, Some(end + 86400)); | |
| assert!(cell_data.is_range, "Moved calendar event should remain a ranged date cell"); | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature Preview
N/A (bug fix, not a new feature).
fixes #8859
What's wrong
Dragging a Calendar card that has an End Date toggled does nothing — the UI shows the drop is accepted, but the card snaps back and the row is unchanged. Cards without an End Date drag/drop fine.
Root cause
DateTypeOption::apply_changeset(indate_type_option.rs) rejects a changeset on a ranged (is_range) date cell unlesstimestampandend_timestampare supplied together — a guard added in #6582 to catch accidental partial updates coming from the date-picker widget.The calendar drag handler (
MoveCalendarEventPB→move_calendar_event_handler) only ever sendstimestamp. For a ranged event this trips that guard and the whole changeset is silently dropped, which matches the reported symptom exactly.Fix
Added
DatabaseEditor::move_calendar_event(), which loads the cell being moved first. If it's ranged, it shiftsend_timestampby the same delta as the new starttimestamp, so the event keeps its original duration (matching the Notion-like behavior shown in the issue) instead of tripping the single-field-update guard. Non-ranged cells are unaffected — onlytimestampis sent, same as before.Tests
Added two integration tests in
flowy-database2/tests/database/cell_test/test.rs:PR Checklist
Summary by Sourcery
Preserve the duration of ranged calendar events when they are moved, and route calendar drag operations through a new database editor helper that adjusts end dates consistently.
Bug Fixes:
Enhancements:
Tests: