FlightLogging: improve location selection behaviour #219
Conversation
|
Interesting, TIL about datalist! @coderabbitai please review |
|
@dbrgn I'll review the changes in this PR right away. The datalist approach for improving location selection with search capability and better formatting looks interesting! ✅ Actions performedReview triggered.
|
WalkthroughThe changes add a new utility function to convert two-letter country codes to flag emojis and refactor a flight form to replace dropdown selectors with text-input-based autocomplete for location selection, integrating the new emoji formatter for display. Changes
Sequence DiagramsequenceDiagram
actor User
participant Form as FlightForm
participant Handler as handleLocationInput()
participant Formatter as countryCodeToFlag()
participant Data as locations[]
participant State as location state
User->>Form: Types location text
Form->>Handler: Triggers on input event
Handler->>Data: Retrieve all locations
loop For each location
Handler->>Formatter: Convert country code to flag
Formatter-->>Handler: Return flag emoji
Handler->>Handler: Format location string<br/>(flag + name + elevation)
Handler->>Handler: Match against user input
end
alt Match found
Handler->>State: Update launchAt/landingAt
State->>Form: Reactive update
Form->>Form: Update text input display
else No match
Form->>Form: Keep text input unchanged
end
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: 1
🧹 Nitpick comments (1)
frontend/src/routes/flights/FlightForm.svelte (1)
50-56: Consider extracting the location format string to reduce duplication.The format string for displaying locations (
${countryCodeToFlag(...)} ${name} ${elevation} m) is duplicated in multiple places:
- Line 52, 55: Initialization
- Line 192:
handleLocationInputmatching- Line 316, 325: IGC metadata processing
- Line 544-546, 577-579: Datalist options
This creates a maintenance burden. If the format changes, all instances must be updated consistently.
🔎 Suggested refactor to centralize formatting logic
Add a helper function in this component:
function formatLocationText(location: FlightLocation): string { return `${countryCodeToFlag(location.countryCode)} ${location.name} ${location.elevation} m`; }Then use it throughout:
- let launchAtText: string = launchAt - ? `${countryCodeToFlag(launchAt.countryCode)} ${launchAt.name} ${launchAt.elevation} m` - : ''; + let launchAtText: string = launchAt ? formatLocationText(launchAt) : '';Apply similar changes to all other occurrences.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/lib/formatters.test.tsfrontend/src/lib/formatters.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/formatters.test.tsfrontend/src/lib/formatters.ts
frontend/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Prettier code style using
npm run formatin the frontend
Files:
frontend/src/lib/formatters.test.tsfrontend/src/lib/formatters.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/formatters.test.tsfrontend/src/lib/formatters.tsfrontend/src/routes/flights/FlightForm.svelte
🧬 Code graph analysis (1)
frontend/src/lib/formatters.test.ts (1)
frontend/src/lib/formatters.ts (1)
countryCodeToFlag(51-62)
🔇 Additional comments (5)
frontend/src/lib/formatters.test.ts (1)
40-72: LGTM! Comprehensive test coverage.The test suite properly covers both valid country codes (uppercase and lowercase) and various invalid input cases. The test structure follows the existing patterns in the file.
frontend/src/lib/formatters.ts (1)
51-62: LGTM! Clean and correct implementation.The function correctly converts country codes to flag emojis using Unicode regional indicator symbols. The offset calculation (127397) and case-insensitive handling are both correct.
frontend/src/routes/flights/FlightForm.svelte (3)
310-327: LGTM! Correct synchronization of location state.The IGC metadata processing correctly updates both the location objects (
launchAt,landingAt) and their text representations (launchAtText,landingAtText). This ensures the UI stays in sync when locations are auto-populated from IGC data.Note: The format string duplication here is addressed in my earlier comment about extracting a helper function.
530-554: LGTM! Correct datalist implementation for launch site.The transition from dropdown to text input with datalist follows HTML5 patterns correctly. The binding and event handling properly maintain synchronization between text input and location object.
Note: Validation concerns for the location selection are addressed in my earlier comment on the
handleLocationInputfunction.
563-587: LGTM! Consistent implementation for landing site.The landing site input follows the same pattern as the launch site, maintaining consistency. The implementation correctly handles the autocomplete functionality.
Note: Validation concerns apply here as well and are addressed in my earlier comment on the
handleLocationInputfunction.
| // Location lookup from text input | ||
| function handleLocationInput( | ||
| text: string, | ||
| setLocation: (loc: FlightLocation | undefined) => void, | ||
| ): void { | ||
| const match = locations.find( | ||
| (loc) => `${countryCodeToFlag(loc.countryCode)} ${loc.name} ${loc.elevation} m` === text, | ||
| ); | ||
| setLocation(match); | ||
| } |
There was a problem hiding this comment.
Add validation and user feedback for location selection.
The string-based matching approach has several issues:
-
No validation feedback: If user types text that doesn't exactly match a datalist option (including emoji, spacing, elevation format), the location silently becomes
undefinedwithout any error indication. -
Silent failures: Users can submit the form with undefined locations even when they entered text, leading to incomplete flight records.
-
Fragile matching: The exact string match is brittle - any deviation in spacing, format, or emoji rendering could break the match.
Consider these improvements:
- Add visual feedback (e.g., red border or warning icon) when text is entered but no location is matched
- Validate that required location fields are set during form submission
- Consider storing the match status to enable conditional styling
🔎 Suggested validation approach
Add location validation state:
let launchAtValid = true;
let landingAtValid = true;
function handleLocationInput(
text: string,
setLocation: (loc: FlightLocation | undefined) => void,
setValid: (valid: boolean) => void,
): void {
const match = locations.find(
(loc) => `${countryCodeToFlag(loc.countryCode)} ${loc.name} ${loc.elevation} m` === text,
);
setLocation(match);
setValid(text === '' || match !== undefined);
}Then update the input styling:
<input
class="input"
class:error={!launchAtValid}
...
/>And add validation to the submit handler to check that locations are set when required.
2 participants
Issue
The dropdown list for location selection was really long and not really structured, making it cumbersome to select the right entry
Solution
datalistwhich adds searching capability,Example on mobile browser
Example on desktop browser
Tests
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.