FlightLogging: improve date setting behaviour#221
FlightLogging: improve date setting behaviour#221Claudio-Chies wants to merge 2 commits intodbrgn:mainfrom
Conversation
|
I'm currently testing CodeRabbit reviews, so don't be surprised if there are automated reviews. We'll see how well it works 🙂 @coderabbitai please review |
|
@dbrgn: Thank you for trying out CodeRabbit! I'll review the changes in this PR and provide feedback on the improved date setting behavior for flight logging. Looking forward to helping with the review! 🐰 ✅ Actions performedReview triggered.
|
WalkthroughIntroduces a new time-helpers utility module containing six functions for converting between time formats, calculating flight durations, and comparing times. Adds comprehensive test coverage for these functions and integrates them into FlightForm for flight data processing and IGC metadata handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/lib/time-helpers.test.tsfrontend/src/lib/time-helpers.tsfrontend/src/routes/flights/FlightForm.svelte
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow TypeScript coding conventions
Files:
frontend/src/lib/time-helpers.test.tsfrontend/src/lib/time-helpers.ts
frontend/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Prettier code style using
npm run formatin the frontend
Files:
frontend/src/lib/time-helpers.test.tsfrontend/src/lib/time-helpers.tsfrontend/src/routes/flights/FlightForm.svelte
**/*.{rs,ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,ts,tsx,svelte}: Avoid deeply nested logic by using early returns for error cases
Write clean, high-quality code with concise comments and clear variable names
Files:
frontend/src/lib/time-helpers.test.tsfrontend/src/lib/time-helpers.tsfrontend/src/routes/flights/FlightForm.svelte
🧬 Code graph analysis (1)
frontend/src/lib/time-helpers.test.ts (1)
frontend/src/lib/time-helpers.ts (6)
timeToMinutes(10-13)minutesToTime(20-24)hmsToTime(31-39)calculateFlightDuration(47-64)ymdToDateString(71-76)isTimeBefore(84-86)
🔇 Additional comments (11)
frontend/src/lib/time-helpers.test.ts (5)
12-28: LGTM! Comprehensive edge case coverage.The parameterized tests cover all important time boundaries effectively.
30-46: LGTM! Good symmetric testing.These tests effectively validate the inverse operation of timeToMinutes.
78-126: LGTM! Excellent coverage of duration calculation.The tests comprehensively cover same-day flights, day rollover, empty inputs, and formatting edge cases.
128-142: LGTM! Good date conversion coverage.The tests include leap year handling, which is important for date validation.
144-173: LGTM! Clear documentation of same-day limitation.The test at lines 169-172 usefully documents that
isTimeBeforedoesn't handle day rollover, which is important context for its usage in FlightForm.frontend/src/routes/flights/FlightForm.svelte (4)
9-14: LGTM! Clean imports.All imported helpers are actively used throughout the form logic.
56-60: LGTM! Good UX improvement.Auto-populating date to today and times to '00:00' for new flights addresses the PR objective of reducing manual entry. The logic correctly preserves existing flight data when editing.
179-182: LGTM! Clean integration of helper function.Using
calculateFlightDurationconsolidates the duration logic in a testable utility.
265-287: LGTM! IGC metadata integration is well-implemented.The intentional overwrite behavior (as noted in comments and PR description) ensures IGC data takes precedence over manual entries, which is the correct approach for accuracy. The use of helpers is consistent and clean.
frontend/src/lib/time-helpers.ts (2)
47-64: LGTM! Robust duration calculation.The function correctly handles empty inputs, day rollover, and formats durations appropriately for display.
84-86: LGTM! Simple and correct for same-day comparison.The function is well-suited for its use case, with the same-day limitation documented in the test suite.
| it('handles hour overflow when rounding seconds at 23:59', () => { | ||
| expect(hmsToTime([23, 59, 30])).to.equal('24:00'); | ||
| }); |
There was a problem hiding this comment.
Consider handling 24:00 edge case.
The test expects hmsToTime([23, 59, 30]) to produce '24:00', which is technically outside the standard time range (00:00-23:59). This could cause issues if downstream code or UI components expect valid time strings.
Consider wrapping to '00:00' or capping at '23:59' to maintain standard time format compliance.
🔎 Suggested fix
If wrapping to next day is desired:
it('handles hour overflow when rounding seconds at 23:59', () => {
- expect(hmsToTime([23, 59, 30])).to.equal('24:00');
+ expect(hmsToTime([23, 59, 30])).to.equal('00:00');
});Corresponding implementation fix would be needed in time-helpers.ts.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/lib/time-helpers.test.ts around lines 65–67, the test asserts
hmsToTime([23, 59, 30]) returns '24:00' which is outside the standard
00:00–23:59 range; update the implementation in time-helpers.ts to handle second
rounding overflow by either wrapping to '00:00' (increment day rollover) or
capping to '23:59', then update this test to assert the chosen behavior (e.g.,
expect '00:00' if wrapping, or '23:59' if capping) and add a comment explaining
the chosen policy so downstream consumers know the format guarantee.
| export function hmsToTime(hms: [number, number, number]): string { | ||
| let hours = hms[0]; | ||
| let minutes = Math.round(hms[1] + hms[2] / 60); | ||
| if (minutes === 60) { | ||
| minutes = 0; | ||
| hours += 1; | ||
| } | ||
| return minutesToTime(hours * 60 + minutes); | ||
| } |
There was a problem hiding this comment.
Address hour overflow to prevent invalid time format.
The function can produce '24:00' or higher when rounding seconds causes hour overflow (e.g., [23, 59, 30] → '24:00'). Standard time format requires hours in the 0-23 range.
Consider wrapping hours to maintain valid time format:
🔎 Proposed fix
export function hmsToTime(hms: [number, number, number]): string {
let hours = hms[0];
let minutes = Math.round(hms[1] + hms[2] / 60);
if (minutes === 60) {
minutes = 0;
hours += 1;
}
+ // Wrap hours to 0-23 range to maintain valid time format
+ hours = hours % 24;
return minutesToTime(hours * 60 + minutes);
}Also update the test expectation in time-helpers.test.ts line 66 from '24:00' to '00:00'.
📝 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.
| export function hmsToTime(hms: [number, number, number]): string { | |
| let hours = hms[0]; | |
| let minutes = Math.round(hms[1] + hms[2] / 60); | |
| if (minutes === 60) { | |
| minutes = 0; | |
| hours += 1; | |
| } | |
| return minutesToTime(hours * 60 + minutes); | |
| } | |
| export function hmsToTime(hms: [number, number, number]): string { | |
| let hours = hms[0]; | |
| let minutes = Math.round(hms[1] + hms[2] / 60); | |
| if (minutes === 60) { | |
| minutes = 0; | |
| hours += 1; | |
| } | |
| // Wrap hours to 0-23 range to maintain valid time format | |
| hours = hours % 24; | |
| return minutesToTime(hours * 60 + minutes); | |
| } |
🤖 Prompt for AI Agents
In frontend/src/lib/time-helpers.ts around lines 31 to 39, hmsToTime can produce
hours >=24 when seconds round up (e.g., [23,59,30] -> "24:00"); clamp or wrap
the hour value into 0-23 after incrementing (hours = hours % 24) before calling
minutesToTime so the result stays a valid 00:00–23:59 time, and update the test
expectation in time-helpers.test.ts line 66 from "24:00" to "00:00".
| // When user changes launch time and landing is earlier, update landing to match launch | ||
| let previousLaunchTime = launchTime; | ||
| function adjustLandingTime(): void { | ||
| if (launchTime !== previousLaunchTime && launchTime !== '' && landingTime !== '') { | ||
| if (isTimeBefore(landingTime, launchTime)) { | ||
| landingTime = launchTime; | ||
| } | ||
| } | ||
| previousLaunchTime = launchTime; | ||
| } | ||
| $: reactive(adjustLandingTime, [launchTime]); |
There was a problem hiding this comment.
Day rollover not handled in landing time adjustment.
When launch time changes, if landing time appears earlier (e.g., launch 23:00, landing 01:00 for next-day landing), the function will incorrectly "fix" landing to match launch because isTimeBefore doesn't account for day rollover.
While calculateFlightDuration correctly handles rollover for display, users would need to re-enter the correct landing time after changing launch time in midnight-crossing scenarios.
🔎 Potential fix approach
Consider using calculateFlightDuration to detect if the current duration is valid/reasonable before auto-adjusting:
function adjustLandingTime(): void {
if (launchTime !== previousLaunchTime && launchTime !== '' && landingTime !== '') {
const duration = calculateFlightDuration(launchTime, landingTime);
// Only adjust if landing is truly before launch (not a rollover scenario)
// For example, if duration would be > 12 hours, it's likely an issue
if (duration && parseInt(duration.substring(1)) > 12 * 60) {
landingTime = launchTime;
}
}
previousLaunchTime = launchTime;
}Note: This is optional given that most flights don't cross midnight.
🤖 Prompt for AI Agents
frontend/src/routes/flights/FlightForm.svelte around lines 109-119: The
adjustLandingTime currently treats any earlier landing time as an error and
overwrites it without accounting for next-day rollovers; change
adjustLandingTime to call calculateFlightDuration(launchTime, landingTime) and
only auto-set landingTime = launchTime when the computed duration indicates an
unlikely/erroneous value (e.g., parsed duration in minutes > 12 * 60) or when
the duration is missing/invalid; otherwise do not adjust so valid next-day
landings are preserved; keep the previousLaunchTime update and reactive
invocation intact.
|
@Claudio-Chies note: Before handling the review comments, I would first want to revisit #74, I think that would be the better solution (instead of working around the time requirement). |
|
@dbrgn sounds good, i can also try to give that a shot. And then we could keep the part where if you set start-time the endtime is autopopulated to the same time, to then make it easyer to adjust. |
|
Before you do that, I'll give it a stab myself in the coming days. The change has various implications, so thinking it through and implementing it myself is almost easier than thoroughly reviewing a PR 🙂 |
2 participants
Issue
if logging a flight manually, the time fields were not auto-populated, meaning you had to enter a time even if you didnt care about the flight time because it was a quick afternoon glide.
also when logging a time it was a hassle because you had to set it twice, even if the flight was just a half a hour. (was more a problem on mobile with the scrolling time)
IMG_9504.mp4
Solution
ScreenRecording_10-14-2025.11-24-11_1.mp4
Additional
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.