Add ICAO flight rules option #100
Conversation
WalkthroughThis pull request introduces enhancements to the ATIS letter functionality, focusing on flight rules display and management. The changes include adding ICAO flight rules support, modifying the settings interface, updating the user interface from a checkbox to a dropdown selection, and implementing logic to calculate flight rules based on visibility and ceiling conditions. Additionally, the spell-checking word list has been updated to include relevant aviation terms. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Build for this pull request: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
com.neil-enns.vatis.sdPlugin/ui/atisLetter.html (1)
36-41: Implementation aligns well with PR objectives!The change from checkbox to dropdown effectively implements the new flight rules options while maintaining backward compatibility with "FAA" as the default value.
Consider enhancing accessibility.
Add an aria-label to provide more context for screen readers.
- <sdpi-select setting="showFlightRules" default="FAA"> + <sdpi-select setting="showFlightRules" default="FAA" aria-label="Select flight rules display type">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
com.neil-enns.vatis.sdPlugin/images/actions/atisLetter/template.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
.vscode/settings.json(1 hunks)com.neil-enns.vatis.sdPlugin/ui/atisLetter.html(1 hunks)src/actions/atisLetter.ts(1 hunks)src/controllers/atisLetter.ts(10 hunks)src/events/streamdeck/atisLetter/updateAtisLetterSettings.ts(1 hunks)
🔇 Additional comments (3)
src/controllers/atisLetter.ts (1)
425-425: Verify thresholds and units incalculateIcaoFlightRulesmethodPlease ensure that the thresholds and units used in the
calculateIcaoFlightRulesmethod accurately reflect the official ICAO standards for determining IMC and VMC conditions. Specifically:
- Visibility Thresholds: Confirm that using a visibility threshold of less than 5 statute miles is appropriate for distinguishing between IMC and VMC conditions under ICAO regulations.
- Ceiling Thresholds: Verify that comparing the ceiling (
cloudLevel) to 1500 feet correctly represents the minimum ceiling height for VMC conditions according to ICAO standards.Additionally, consider the following:
- Unit Consistency: Ensure that the units for visibility and ceiling are consistently used and converted if necessary. Clarify in the comments whether
cloudLevelis in feet or hundreds of feet to avoid confusion.- Code Duplication: Refactor common logic between
calculateIcaoFlightRulesandcalculateFaaFlightRulesmethods to reduce code duplication and enhance maintainability.Also applies to: 431-481
src/events/streamdeck/atisLetter/updateAtisLetterSettings.ts (1)
26-27: Appropriate refresh whenshowFlightRulessetting changesThe addition of
showFlightRulesto therequiresRefreshcondition ensures that the action properly refreshes when the flight rules display setting is updated. This change maintains consistency in the user interface behavior.src/actions/atisLetter.ts (1)
85-90: Enhance flight rules display options withFlightRuleDisplayenumTransitioning from the boolean
showFaaFlightRulesto theFlightRuleDisplayenum in theAtisLetterSettingsinterface is a beneficial change. It provides greater flexibility by allowing users to select between multiple flight rule display options:NONE,FAA, andICAO. This improvement enhances the extensibility and clarity of the settings.Also applies to: 97-97
|
Build for this pull request: |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.vscode/settings.json(1 hunks)com.neil-enns.vatis.sdPlugin/ui/atisLetter.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/settings.json
🔇 Additional comments (2)
com.neil-enns.vatis.sdPlugin/ui/atisLetter.html (2)
38-40: LGTM! The dropdown options align with requirements.The options correctly implement the requested flight rules display types, with:
- "NONE" for disabling the feature
- "FAA" maintaining backward compatibility as the default
- "ICAO" adding the new VMC/IMC states
36-36: Good accessibility practice witharia-label.The inclusion of an
aria-labelimproves accessibility for screen readers.
| <sdpi-select setting="showFlightRules" default="FAA" aria-label="Select flight rules display type"> | ||
| <sdpi-select setting="showFlightRules" default="FAA"> | ||
| <option value="NONE">None</option> | ||
| <option value="FAA">FAA</option> | ||
| <option value="ICAO">ICAO</option> | ||
| </sdpi-select> |
There was a problem hiding this comment.
Fix duplicate setting attribute and missing parent tag.
There are a few structural issues in this code segment:
- The
settingattribute is duplicated on lines 36 and 37 - The opening
<sdpi-item>tag with label is missing
Apply this diff to fix the issues:
+ <sdpi-item label="Flight rules">
- <sdpi-select setting="showFlightRules" default="FAA" aria-label="Select flight rules display type">
- <sdpi-select setting="showFlightRules" default="FAA">
+ <sdpi-select setting="showFlightRules" default="FAA" aria-label="Select flight rules display type">
<option value="NONE">None</option>
<option value="FAA">FAA</option>
<option value="ICAO">ICAO</option>
</sdpi-select>
</sdpi-item>📝 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.
| <sdpi-select setting="showFlightRules" default="FAA" aria-label="Select flight rules display type"> | |
| <sdpi-select setting="showFlightRules" default="FAA"> | |
| <option value="NONE">None</option> | |
| <option value="FAA">FAA</option> | |
| <option value="ICAO">ICAO</option> | |
| </sdpi-select> | |
| <sdpi-item label="Flight rules"> | |
| <sdpi-select setting="showFlightRules" default="FAA" aria-label="Select flight rules display type"> | |
| <option value="NONE">None</option> | |
| <option value="FAA">FAA</option> | |
| <option value="ICAO">ICAO</option> | |
| </sdpi-select> | |
| </sdpi-item> |
💡 Codebase verification
Create CHANGELOG.md and document the breaking change
The change from checkbox to dropdown for flight rules display is a breaking change. Please:
- Create a CHANGELOG.md file to track significant changes
- Document this UI change and its impact
- Update the external documentation at projects.neilenns.com/docs/streamdeck-vatis
🔗 Analysis chain
Document the breaking change in README/CHANGELOG.
This UI change from checkbox to dropdown is a breaking change that affects existing implementations. Please ensure that:
- The change is documented in the CHANGELOG
- Migration instructions are provided for users updating from previous versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if breaking change is documented
rg -i "breaking.*change|migration|upgrade" README.md CHANGELOG.md
Length of output: 120
Script:
#!/bin/bash
# Find documentation files
fd -e md
# Check the previous version of the file
git log -p -- "com.neil-enns.vatis.sdPlugin/ui/atisLetter.html" | head -n 50
Length of output: 20
Script:
#!/bin/bash
# Check README content
cat README.md
# Also check if there's any existing documentation about the showFlightRules setting
rg -i "showflightrules|flight.?rules" README.md
Length of output: 1088
|
Build for this pull request: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
com.neil-enns.vatis.sdPlugin/ui/atisLetter.html (1)
37-41: Consider enhancing accessibility.While the current implementation includes basic accessibility support, consider these improvements:
- Add
role="combobox"to enhance semantic meaning- Include a description of the flight rules options in the aria-label
<sdpi-select setting="showFlightRules" default="FAA" - aria-label="Select flight rules display type" + aria-label="Select flight rules display type: None, FAA, or ICAO" + role="combobox" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
com.neil-enns.vatis.sdPlugin/ui/atisLetter.html(1 hunks)
🔇 Additional comments (2)
com.neil-enns.vatis.sdPlugin/ui/atisLetter.html (2)
36-45: Well-structured dropdown implementation!The flight rules dropdown is properly implemented with clear labeling and good accessibility support.
36-45: Document the breaking change in README/CHANGELOG.This UI change from checkbox to dropdown is a breaking change that affects existing implementations. Please ensure that:
- The change is documented in the CHANGELOG
- Migration instructions are provided for users updating from previous versions
#!/bin/bash # Check if breaking change is documented rg -i "breaking.*change|migration|upgrade" README.md CHANGELOG.md # Find documentation files fd -e md
|
Build for this pull request: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/controllers/atisLetter.ts (2)
474-475: Fix incorrect comment about cloud level units.The comment states that cloud levels are in "100s of feet", but the code and ICAO standards use actual feet values.
- // Cloud levels are in 100s of feet. + // Cloud levels are in feet.
437-481: Consider simplifying the method using early returns.The method can be made more readable by using early returns for the VMC case.
private calculateIcaoFlightRules( ceiling?: Value, prevailingVisibility?: Value ) { // No visibility data means the flight rules can't be calculated. if (!prevailingVisibility) { this.icaoFlightRules = IcaoFlightRules.UNKNOWN; return; } // If the ceiling is null then set it to the default. if (!ceiling) { ceiling = { actualValue: DEFAULT_CEILING_LEVEL_FEET, actualUnit: Unit.Feet, }; } // Rules are only applicable to meters and feet if ( prevailingVisibility.actualUnit !== Unit.Meter || ceiling.actualUnit !== Unit.Feet ) { this.icaoFlightRules = IcaoFlightRules.UNKNOWN; return; } const cloudLevel = ceiling.actualValue; const visibility = prevailingVisibility.actualValue; if (visibility < 0 || cloudLevel < 0) { this.icaoFlightRules = IcaoFlightRules.UNKNOWN; return; } - // The checks are in this order to ensure the most restrictive, rather than least restrictive, - // is applied. - // Visibility is in meters. - // Cloud levels are in 100s of feet. - if (visibility < 5000 || cloudLevel < 1500) { - this.icaoFlightRules = IcaoFlightRules.IMC; - } else { - this.icaoFlightRules = IcaoFlightRules.VMC; - } + // VMC requires both visibility >= 5000m and ceiling >= 1500ft + if (visibility >= 5000 && cloudLevel >= 1500) { + this.icaoFlightRules = IcaoFlightRules.VMC; + return; + } + + // If not VMC, then it's IMC + this.icaoFlightRules = IcaoFlightRules.IMC; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/atisLetter.ts(10 hunks)
🔇 Additional comments (5)
src/controllers/atisLetter.ts (5)
36-43: LGTM! Well-documented enum for ICAO flight rules.The enum values and documentation accurately represent ICAO flight rules categories.
133-137: LGTM! Clean transition from boolean to enum-based flight rules display.The getter properly handles the default case by falling back to FAA display mode.
161-180: LGTM! Consistent implementation of ICAO flight rules property.The getter and setter follow the established patterns in the codebase, with proper change detection to avoid unnecessary updates.
425-425: LGTM! Clean integration of ICAO flight rules.The ICAO flight rules calculation and display are properly integrated into the existing update and refresh logic.
Also applies to: 583-584
431-481: Add unit tests for ICAO flight rules calculation.Please add unit tests to verify:
- VMC conditions (visibility >= 5000m, ceiling >= 1500ft)
- IMC conditions
- Edge cases (missing data, invalid units, negative values)
Would you like me to help generate the test cases?
|
Build for this pull request: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/controllers/atisLetter.ts (1)
431-481: Enhance the ICAO flight rules calculation logic.The implementation has a few areas for improvement:
- The visibility check at line 476 (
visibility < 5000) could be made more readable by using a named constant.- The cloud level check at line 476 (
cloudLevel < 150) could be made more readable by using a named constant.- Consider adding more detailed comments about the ICAO rules source/reference.
Consider applying this refactor:
+ // ICAO visibility threshold in meters + private static readonly ICAO_VMC_VISIBILITY_THRESHOLD = 5000; + // ICAO ceiling threshold in hundreds of feet + private static readonly ICAO_VMC_CEILING_THRESHOLD = 150; private calculateIcaoFlightRules( ceiling?: Value, prevailingVisibility?: Value ) { // ... existing validation code ... // The checks are in this order to ensure the most restrictive, rather than least restrictive, // is applied. // Reference: ICAO Annex 2 - Rules of the Air, Chapter 4 // Visibility is in meters. // Cloud levels are in 100s of feet. - if (visibility < 5000 || cloudLevel < 150) { + if (visibility < AtisLetterController.ICAO_VMC_VISIBILITY_THRESHOLD || + cloudLevel < AtisLetterController.ICAO_VMC_CEILING_THRESHOLD) { this.icaoFlightRules = IcaoFlightRules.IMC; } else { this.icaoFlightRules = IcaoFlightRules.VMC; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/atisLetter.ts(10 hunks)
🔇 Additional comments (3)
src/controllers/atisLetter.ts (3)
36-43: LGTM! Well-documented enum for ICAO flight rules.The enum is correctly defined with appropriate values and documentation. The mapping between ICAO and FAA rules is logical: IMC maps to IFR, and VMC maps to VFR.
583-584: LGTM! Correctly updated image replacements.The
refreshImagemethod has been properly updated to include both the ICAO flight rules and the display preference.
437-440: Add unit tests for ICAO flight rules calculation.The new
calculateIcaoFlightRulesmethod needs unit tests to verify:
- VMC/IMC threshold calculations
- Edge cases (visibility exactly 5000m, ceiling exactly 150)
- Error handling (invalid units, negative values)
- Default ceiling behavior
Let's check for existing test coverage:
Would you like me to help generate comprehensive unit tests for the ICAO flight rules functionality?
| * Gets the show flight rules setting. | ||
| * @returns {FlightRuleDisplay} The type of flight rules indicator to show. Defaults to FAA if no setting is provided. | ||
| */ | ||
| get showFaaFlightRules(): boolean { | ||
| return this.settings.showFaaFlightRules ?? true; | ||
| get showFlightRules(): FlightRuleDisplay { | ||
| return this.settings.showFlightRules ?? FlightRuleDisplay.FAA; |
There was a problem hiding this comment.
💡 Codebase verification
Add migration notes for the breaking change
The breaking change related to FlightRuleDisplay default value is not documented. While the code maintains backward compatibility by defaulting to FlightRuleDisplay.FAA, this change should be documented to help users understand:
- What changed in the flight rules display settings
- The new default behavior
- Steps required if they need to modify their implementation
Consider adding these details to README.md or creating a CHANGELOG.md file.
🔗 Analysis chain
Verify the default value in production code.
The getter's default value of FlightRuleDisplay.FAA maintains backward compatibility, which is good. However, since this is a breaking change as mentioned in the PR objectives, we should ensure this default value is documented in the migration guide.
Let's check if there's a migration guide or documentation update:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for migration guide or documentation updates
rg -g '!node_modules' -g '*.{md,txt}' -i 'migration|upgrade|breaking.?change' .
Length of output: 81
Script:
#!/bin/bash
# Look for documentation files and any mentions of flight rules
echo "=== Searching for documentation files ==="
fd -e md -e txt
echo -e "\n=== Searching for FlightRuleDisplay mentions in docs ==="
rg -g '*.{md,txt}' "FlightRuleDisplay|flight.?rules?"
echo -e "\n=== Checking for CHANGELOG ==="
fd -i changelog
Length of output: 400
|
Build for this pull request: |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.vscode/settings.json(1 hunks)src/controllers/atisLetter.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/settings.json
🔇 Additional comments (5)
src/controllers/atisLetter.ts (5)
Line range hint
21-48: Well-structured constants and enums!The new constants and
IcaoFlightRulesenum are well-documented and follow aviation standards.
64-64: LGTM - Consistent property handling!The
_icaoFlightRulesproperty follows the same initialization and reset patterns as other properties in the class.Also applies to: 110-110
138-142: Document the breaking change in flight rules display.While the code maintains backward compatibility by defaulting to
FlightRuleDisplay.FAA, this breaking change should be documented to help users understand the new behavior.
166-186: LGTM - Well-implemented getter/setter pair!The ICAO flight rules getter/setter implementation follows the class's established patterns with proper change detection and display updates.
590-591: LGTM - Consistent image refresh updates!The image refresh logic has been properly updated to include the new ICAO flight rules and display settings.
| /** | ||
| * Calculates ICAO flight rules based on ceiling and visibility values. | ||
| * @param {Value | undefined} ceiling - The ceiling height in hundreds of feet. | ||
| * @param {Value | undefined} prevailingVisibility - The visibility in meters. | ||
| * @private | ||
| */ | ||
| private calculateIcaoFlightRules( | ||
| ceiling?: Value, | ||
| prevailingVisibility?: Value | ||
| ) { | ||
| // No visibility data might mean CAVOK so assume unlimited visibility. | ||
| if (!prevailingVisibility) { | ||
| prevailingVisibility = { | ||
| actualValue: DEFAULT_VISIBILITY_METERS, | ||
| actualUnit: Unit.Meter, | ||
| }; | ||
| } | ||
|
|
||
| // If the ceiling is null then set it to the default. | ||
| if (!ceiling) { | ||
| ceiling = { | ||
| actualValue: DEFAULT_CEILING_LEVEL_FEET, | ||
| actualUnit: Unit.Feet, | ||
| }; | ||
| } | ||
|
|
||
| // Rules are only applicable to meters and feet. | ||
| if ( | ||
| prevailingVisibility.actualUnit !== Unit.Meter || | ||
| ceiling.actualUnit !== Unit.Feet | ||
| ) { | ||
| this.icaoFlightRules = IcaoFlightRules.UNKNOWN; | ||
| return; | ||
| } | ||
|
|
||
| const cloudLevel = ceiling.actualValue; | ||
| const visibility = prevailingVisibility.actualValue; | ||
|
|
||
| if (visibility < 0 || cloudLevel < 0) { | ||
| this.icaoFlightRules = IcaoFlightRules.UNKNOWN; | ||
| return; | ||
| } | ||
|
|
||
| // The checks are in this order to ensure the most restrictive, rather than least restrictive, | ||
| // is applied. | ||
| // Visibility is in meters. | ||
| // Cloud levels are in 100s of feet. | ||
| if (visibility < 5000 || cloudLevel < 15) { | ||
| this.icaoFlightRules = IcaoFlightRules.IMC; | ||
| } else { | ||
| this.icaoFlightRules = IcaoFlightRules.VMC; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Incorrect ICAO flight rules implementation
The implementation has several issues:
- The JSDoc comment incorrectly states "ceiling height in hundreds of feet" while the code uses actual feet
- The VMC/IMC threshold of 15 feet is incorrect (standard ICAO minimums are typically 1000-3000 feet)
- Assuming CAVOK for missing visibility is inconsistent with the FAA implementation and potentially unsafe
🔗 Analysis chain
Verify ICAO flight rules calculation logic.
Please verify the following aspects of the implementation:
- The assumption that missing visibility means CAVOK needs verification, as it could also indicate unavailable data.
- The comment states "Cloud levels are in 100s of feet" but the code treats them as actual feet.
- The VMC/IMC threshold of 15 feet ceiling seems unusually low for ICAO standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ICAO VMC/IMC definitions in documentation
echo "=== Searching for ICAO VMC/IMC documentation ==="
rg -i "vmc|imc|ceiling.*feet|visibility.*met(er|re)s?" .
echo -e "\n=== Searching for CAVOK handling ==="
rg -i "cavok|ceiling.and.visibility.ok" .
Length of output: 2499
Script:
#!/bin/bash
# Search for test files and ceiling/visibility handling
echo "=== Searching for test files ==="
fd -e test.ts -e spec.ts
echo -e "\n=== Searching for ceiling value handling ==="
rg -i "ceiling.*value|ceiling.*conversion" .
echo -e "\n=== Searching for visibility value handling ==="
rg -i "visibility.*value|visibility.*conversion" .
Length of output: 1737
Script:
#!/bin/bash
# Search for FAA implementation and Value interface
echo "=== Searching for FAA flight rules implementation ==="
rg -A 20 "calculateFaaFlightRules" .
echo -e "\n=== Searching for Value interface definition ==="
ast-grep --pattern 'interface Value'
Length of output: 3009
Fixes #99
Adds a VMC (reuses the VFR icon) and IMC (reuses the IFR icon) state that are passed to the template, and passes what type of display is requested. Changes the checkbox to a dropdown with three choices.
This is a breaking change.
Summary by CodeRabbit
New Features
Bug Fixes
Chores