feat: Integrate native D-ATIS auto-loading#271
Open
leftos wants to merge 5 commits into
Open
Conversation
Fetch real-world D-ATIS data from the existing Digital ATIS API, process it, and populate airport conditions and NOTAMs fields automatically when a station has a preset named "D-ATIS" selected. Data refreshes every 5 minutes. This eliminates the need for the external vATISLoad tool. New "Automatically fetch D-ATIS" setting in User Settings controls the feature. When enabled, the "Automatically fetch ATIS letter" setting is forced on and cannot be unchecked. Per-station text replacement rules (literal or regex) are configurable via a new "D-ATIS" tab in the ATIS Configuration window and are stored in the profile JSON. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds D-ATIS fetching, parsing, and publishing: a timed DatisRepository that retrieves Digital ATIS, a DatisTextProcessor to produce structured DatisResult, configuration/UI for replacements and auto-fetch, event DatisReceived, and view-model integration for automatic updates. Changes
Sequence DiagramsequenceDiagram
participant Timer as Periodic Timer
participant DatisRepo as DatisRepository
participant Downloader as IDownloader
participant Processor as DatisTextProcessor
participant EventBus as Event Bus
participant VM as AtisStationViewModel
Timer->>DatisRepo: Tick -> UpdateAsync()
loop For each monitored station
DatisRepo->>Downloader: HTTP GET (station URL)
alt 200 OK
Downloader-->>DatisRepo: JSON payload
DatisRepo->>DatisRepo: Parse JSON, select entry by AtisType
DatisRepo->>Processor: Process(rawBody, station config)
Processor->>Processor: Strip envelope, normalize NOTAMs, apply replacements, expand contractions, split sections
Processor-->>DatisRepo: DatisResult
DatisRepo->>EventBus: Publish DatisReceived(DatisResult)
else Error / missing
Downloader-->>DatisRepo: Error / empty
DatisRepo->>EventBus: Publish DatisReceived(not-available result)
end
end
EventBus->>VM: Deliver DatisReceived
VM->>VM: Update airport conditions, NOTAMs, ATIS letter
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vATIS.Desktop/Atis/DatisRepository.cs`:
- Around line 147-150: The catch block only logs the exception and fails to
publish the "D-ATIS NOT AVBL." sentinel like the non-success/no-data branches;
update the catch for Exception ex in DatisRepository.cs to set the station's
D-ATIS sentinel and publish the unavailable state (the same code used in the
non-success/no-data paths) after logging, e.g. set station.DatisText to "D-ATIS
NOT AVBL." (or call the existing method that does this) and invoke the same
publish/update routine so stale D-ATIS text is cleared when an exception occurs
for station.Identifier.
- Around line 48-49: The Tick handler for _updateTimer is allowing overlapping
runs because UpdateAsync is effectively async void; add a reentrancy guard in
UpdateAsync (or the Tick delegate) to prevent concurrent executions: introduce a
private volatile/atomic flag (e.g. _isUpdating) or use Interlocked to
check-and-set at the start of UpdateAsync, return immediately if already
running, and clear the flag in a finally block (or alternatively Stop/Start
_updateTimer around the awaited work). Ensure the guard protects the call path
that invokes FetchForStationAsync and EventBus publishing so results cannot
interleave or publish out of order.
In `@vATIS.Desktop/Atis/DatisTextProcessor.cs`:
- Around line 77-100: Apply a timeout to user-provided regex replacements in
ApplyReplacements: when DatisTextReplacement.IsRegex is true, call the
Regex.Replace overload that accepts a timeout (e.g. Regex.Replace(input, pattern
+ @"[,.;]{0,1}", replacement, RegexOptions.None, TimeSpan.FromSeconds(1)))
instead of the current timeout-less call, and add a catch for
RegexMatchTimeoutException to log a warning similar to the existing
RegexParseException handling; ensure you keep the existing pattern concat and
replacement behavior but protect against ReDoS by imposing the timeout.
In `@vATIS.Desktop/Config/AppConfig.cs`:
- Around line 117-119: When loading persisted settings into AppConfig, enforce
the invariant that AutoFetchDatis implies AutoFetchAtisLetter: after assigning
AutoFetchDatis and AutoFetchAtisLetter (in the config load method or constructor
where AutoFetchAtisLetter = config.AutoFetchAtisLetter; AutoFetchDatis =
config.AutoFetchDatis; is done), add logic to set AutoFetchAtisLetter = true
whenever AutoFetchDatis is true so the in-memory config cannot enter the invalid
state; reference the AutoFetchDatis and AutoFetchAtisLetter fields/properties in
AppConfig and apply the correction immediately after the assignments.
In `@vATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axaml`:
- Line 11: The DataGrid named ReplacementsGrid enables multi-selection via
SelectionMode="Extended" but the delete logic only removes a single
SelectedItem; either change SelectionMode to "Single" on the ReplacementsGrid
declaration or update the delete handler to operate on
ReplacementsGrid.SelectedItems (and remove all selected entries from the view
model's Replacements collection) so bulk deletes are supported; ensure the
deletion code updates bindings/collection notifications and prevents modifying
the SelectedItems collection while iterating.
- Around line 11-20: The column bindings in ReplacementsGrid (DataGridTextColumn
bindings for Pattern and Replacement and the DataGridCheckBoxColumn binding for
IsRegex) should be made explicit by setting Mode=TwoWay and an
UpdateSourceTrigger (e.g., PropertyChanged) to ensure clear, deterministic
updates to the model; update the Binding expressions for Pattern, Replacement
and IsRegex accordingly while leaving the existing
ReplacementsGrid_OnCellEditEnding/SaveConfig logic intact.
In `@vATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs`:
- Line 22: The handlers in DatisReplacementsViewModel are mutating
Vatsim.Vatis.Profiles.Models.AtisStation but persist changes via
IAppConfig.SaveConfig(), which is incorrect; change the persistence to use the
profile repository save used by AtisStationViewModel.cs instead. Replace calls
that use _appConfig.SaveConfig() (and any direct app-config writes) with the
profile-save call/pattern from AtisStationViewModel (e.g., obtain the active
profile, update the AtisStation model, and call the ProfileRepository save
method used there such as SaveProfile/SaveActiveProfileAsync), and ensure all
mutation handlers referenced (lines around the private IAppConfig field and the
handlers at 30-32, 108-111, 123-135) follow that same flow so per-profile D-ATIS
replacement rules are persisted to the profile store not IAppConfig.
In `@vATIS.Desktop/Ui/ViewModels/AtisStationViewModel.cs`:
- Line 816: The AutoFetchDatis toggle currently only adds/removes the repository
in the selection/change and Dispose paths, so toggling AutoFetchDatis while the
current preset is a D-ATIS preset doesn't start/stop monitoring immediately;
update the AutoFetchDatis change handler (the property setter or method that
runs when AutoFetchDatis is toggled in AtisStationViewModel) to check if the
current preset type is D-ATIS and call the same repository management logic you
use elsewhere: when enabling, call the repository AddStation logic (the
counterpart to _datisRepository.RemoveStation(AtisStation.Id)) to start the
5-minute fetch loop, and when disabling call
_datisRepository.RemoveStation(AtisStation.Id) to stop it; ensure this mirrors
the behavior in Dispose() and the preset-change handlers so the monitoring
starts/stops immediately without needing a preset reselection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0bc53dc6-0854-4ed4-867e-83dca57f8d18
📒 Files selected for processing (22)
vATIS.Desktop/Atis/DatisRepository.csvATIS.Desktop/Atis/DatisResult.csvATIS.Desktop/Atis/DatisTextProcessor.csvATIS.Desktop/Atis/IDatisRepository.csvATIS.Desktop/Atis/IDatisTextProcessor.csvATIS.Desktop/Config/AppConfig.csvATIS.Desktop/Config/IAppConfig.csvATIS.Desktop/Container/Factory/ViewModelFactory.csvATIS.Desktop/Container/ServiceProvider.csvATIS.Desktop/Events/DatisReceived.csvATIS.Desktop/Profiles/Models/AtisStation.csvATIS.Desktop/Profiles/Models/DatisTextReplacement.csvATIS.Desktop/SourceGenerationContext.csvATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axamlvATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axaml.csvATIS.Desktop/Ui/Dialogs/SettingsDialog.axamlvATIS.Desktop/Ui/IViewModelFactory.csvATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.csvATIS.Desktop/Ui/ViewModels/AtisConfigurationWindowViewModel.csvATIS.Desktop/Ui/ViewModels/AtisStationViewModel.csvATIS.Desktop/Ui/ViewModels/SettingsDialogViewModel.csvATIS.Desktop/Ui/Windows/AtisConfigurationWindow.axaml
Comment on lines
+11
to
+20
| <DataGrid x:Name="ReplacementsGrid" GridLinesVisibility="All" CanUserReorderColumns="False" CanUserResizeColumns="False" CanUserSortColumns="False" BorderThickness="1" BorderBrush="#646464" Height="380" ItemsSource="{Binding Replacements, DataType=vm:DatisReplacementsViewModel}" SelectionMode="Extended" CellEditEnding="ReplacementsGrid_OnCellEditEnding"> | ||
| <DataGrid.Styles> | ||
| <Style Selector="DataGrid:focus DataGridCell:current /template/ Grid#FocusVisual"> | ||
| <Setter Property="IsVisible" Value="False" /> | ||
| </Style> | ||
| </DataGrid.Styles> | ||
| <DataGrid.Columns> | ||
| <DataGridTextColumn Header="Pattern" Width="2*" IsReadOnly="False" Binding="{Binding Pattern, DataType=models:DatisTextReplacement}"/> | ||
| <DataGridTextColumn Header="Replacement" Width="2*" IsReadOnly="False" Binding="{Binding Replacement, DataType=models:DatisTextReplacement}"/> | ||
| <DataGridCheckBoxColumn Header="Regex" Width="80" IsReadOnly="False" Binding="{Binding IsRegex, DataType=models:DatisTextReplacement}"/> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify DataGrid edit event wiring and save timing path.
rg -n "CellEditEnding|CellEditEndingCommand|SaveConfig|DataGridTextColumn|DataGridCheckBoxColumn|Mode=TwoWay|UpdateSourceTrigger" -C2Repository: vatis-project/vatis
Length of output: 39997
🏁 Script executed:
# Check Avalonia DataGrid binding defaults and look for other DataGrid patterns
rg -n "DataGridTextColumn.*Binding.*Mode|DataGridCheckBoxColumn.*Binding.*Mode" --max-count=20
# Verify if DatisTextReplacement model has INotifyPropertyChanged
rg -n "class DatisTextReplacement|INotifyPropertyChanged" -A 5 vATIS.Desktop/Repository: vatis-project/vatis
Length of output: 1116
🏁 Script executed:
# Look for FormattingView pattern to compare binding approaches
grep -A 5 "DataGridCellEndEditBehavior" vATIS.Desktop/Ui/AtisConfiguration/FormattingView.axaml | head -20Repository: vatis-project/vatis
Length of output: 1227
🏁 Script executed:
# Check if DatisTextReplacement implements property change notifications
find . -name "*DatisTextReplacement*" -type fRepository: vatis-project/vatis
Length of output: 118
🏁 Script executed:
# Read the complete DatisTextReplacement model
cat -n vATIS.Desktop/Profiles/Models/DatisTextReplacement.csRepository: vatis-project/vatis
Length of output: 1744
🏁 Script executed:
# Check if there is any property change notification infrastructure for models in this project
rg -n "INotifyPropertyChanged" vATIS.Desktop/Profiles/Models/ -A 2 -B 2Repository: vatis-project/vatis
Length of output: 45
🏁 Script executed:
# Check Avalonia's DataGrid binding documentation or similar patterns - see how other view models handle POCO binding
rg -n "DataGridTextColumn.*Binding" vATIS.Desktop/Ui/AtisConfiguration/ -A 1 | head -40Repository: vatis-project/vatis
Length of output: 3948
🏁 Script executed:
# Verify how the Replacements collection is updated - check if it's a reactive collection or plain list
rg -n "Replacements.*=" vATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs -B 2 -A 2Repository: vatis-project/vatis
Length of output: 829
🏁 Script executed:
# Check if there are any Avalonia DataGrid tests or documentation comments in the codebase
rg -n "DataGrid.*edit|binding.*DataGrid" -i vATIS.Desktop/ --type cs -A 3 | head -50Repository: vatis-project/vatis
Length of output: 5150
🏁 Script executed:
# Look for any Avalonia version or binding configuration in project files
find . -name "*.csproj" -o -name ".csproj" | xargs grep -l "Avalonia" | head -1 | xargs cat | grep -i "avalonia\|version" | head -20Repository: vatis-project/vatis
Length of output: 1538
🏁 Script executed:
# Check Avalonia DataGrid source or documentation patterns - look for binding mode defaults
rg -n "TwoWay|OneWay|BindingMode" vATIS.Desktop/Ui/AtisConfiguration/ -i | head -20Repository: vatis-project/vatis
Length of output: 592
🏁 Script executed:
# Verify the actual execution flow - check if bindings are committed before CellEditEnding fires
cat -n vATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axaml.csRepository: vatis-project/vatis
Length of output: 1348
Add explicit binding modes for clarity; Commit guard already mitigates persistence timing concerns.
The CellEditEnding handler includes a safeguard at line 27: it only executes SaveConfig() when e.EditAction == DataGridEditAction.Commit, meaning the binding has already updated the model before persistence. However, the column bindings lack explicit Mode=TwoWay and UpdateSourceTrigger declarations, making the behavior implicit and harder to trace. For defensive clarity and consistency with XAML best practices, add explicit binding modes:
Proposed hardening
- <DataGridTextColumn Header="Pattern" Width="2*" IsReadOnly="False" Binding="{Binding Pattern, DataType=models:DatisTextReplacement}"/>
- <DataGridTextColumn Header="Replacement" Width="2*" IsReadOnly="False" Binding="{Binding Replacement, DataType=models:DatisTextReplacement}"/>
- <DataGridCheckBoxColumn Header="Regex" Width="80" IsReadOnly="False" Binding="{Binding IsRegex, DataType=models:DatisTextReplacement}"/>
+ <DataGridTextColumn Header="Pattern" Width="2*" IsReadOnly="False" Binding="{Binding Pattern, DataType=models:DatisTextReplacement, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>
+ <DataGridTextColumn Header="Replacement" Width="2*" IsReadOnly="False" Binding="{Binding Replacement, DataType=models:DatisTextReplacement, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>
+ <DataGridCheckBoxColumn Header="Regex" Width="80" IsReadOnly="False" Binding="{Binding IsRegex, DataType=models:DatisTextReplacement, Mode=TwoWay}"/>📝 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.
Suggested change
| <DataGrid x:Name="ReplacementsGrid" GridLinesVisibility="All" CanUserReorderColumns="False" CanUserResizeColumns="False" CanUserSortColumns="False" BorderThickness="1" BorderBrush="#646464" Height="380" ItemsSource="{Binding Replacements, DataType=vm:DatisReplacementsViewModel}" SelectionMode="Extended" CellEditEnding="ReplacementsGrid_OnCellEditEnding"> | |
| <DataGrid.Styles> | |
| <Style Selector="DataGrid:focus DataGridCell:current /template/ Grid#FocusVisual"> | |
| <Setter Property="IsVisible" Value="False" /> | |
| </Style> | |
| </DataGrid.Styles> | |
| <DataGrid.Columns> | |
| <DataGridTextColumn Header="Pattern" Width="2*" IsReadOnly="False" Binding="{Binding Pattern, DataType=models:DatisTextReplacement}"/> | |
| <DataGridTextColumn Header="Replacement" Width="2*" IsReadOnly="False" Binding="{Binding Replacement, DataType=models:DatisTextReplacement}"/> | |
| <DataGridCheckBoxColumn Header="Regex" Width="80" IsReadOnly="False" Binding="{Binding IsRegex, DataType=models:DatisTextReplacement}"/> | |
| <DataGrid x:Name="ReplacementsGrid" GridLinesVisibility="All" CanUserReorderColumns="False" CanUserResizeColumns="False" CanUserSortColumns="False" BorderThickness="1" BorderBrush="#646464" Height="380" ItemsSource="{Binding Replacements, DataType=vm:DatisReplacementsViewModel}" SelectionMode="Extended" CellEditEnding="ReplacementsGrid_OnCellEditEnding"> | |
| <DataGrid.Styles> | |
| <Style Selector="DataGrid:focus DataGridCell:current /template/ Grid#FocusVisual"> | |
| <Setter Property="IsVisible" Value="False" /> | |
| </Style> | |
| </DataGrid.Styles> | |
| <DataGrid.Columns> | |
| <DataGridTextColumn Header="Pattern" Width="2*" IsReadOnly="False" Binding="{Binding Pattern, DataType=models:DatisTextReplacement, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/> | |
| <DataGridTextColumn Header="Replacement" Width="2*" IsReadOnly="False" Binding="{Binding Replacement, DataType=models:DatisTextReplacement, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/> | |
| <DataGridCheckBoxColumn Header="Regex" Width="80" IsReadOnly="False" Binding="{Binding IsRegex, DataType=models:DatisTextReplacement, Mode=TwoWay}"/> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axaml` around lines
11 - 20, The column bindings in ReplacementsGrid (DataGridTextColumn bindings
for Pattern and Replacement and the DataGridCheckBoxColumn binding for IsRegex)
should be made explicit by setting Mode=TwoWay and an UpdateSourceTrigger (e.g.,
PropertyChanged) to ensure clear, deterministic updates to the model; update the
Binding expressions for Pattern, Replacement and IsRegex accordingly while
leaving the existing ReplacementsGrid_OnCellEditEnding/SaveConfig logic intact.
| /// <inheritdoc /> | ||
| public void Dispose() | ||
| { | ||
| _datisRepository.RemoveStation(AtisStation.Id); |
Contributor
There was a problem hiding this comment.
React to AutoFetchDatis changes after a D-ATIS preset is already selected.
The repository is only added/removed here and in Dispose(). If the user toggles the setting while already on a D-ATIS preset, enabling never starts monitoring until the preset is reselected, and disabling leaves the 5-minute fetch loop running until a preset change or disposal.
Also applies to: 1675-1696
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vATIS.Desktop/Ui/ViewModels/AtisStationViewModel.cs` at line 816, The
AutoFetchDatis toggle currently only adds/removes the repository in the
selection/change and Dispose paths, so toggling AutoFetchDatis while the current
preset is a D-ATIS preset doesn't start/stop monitoring immediately; update the
AutoFetchDatis change handler (the property setter or method that runs when
AutoFetchDatis is toggled in AtisStationViewModel) to check if the current
preset type is D-ATIS and call the same repository management logic you use
elsewhere: when enabling, call the repository AddStation logic (the counterpart
to _datisRepository.RemoveStation(AtisStation.Id)) to start the 5-minute fetch
loop, and when disabling call _datisRepository.RemoveStation(AtisStation.Id) to
stop it; ensure this mirrors the behavior in Dispose() and the preset-change
handlers so the monitoring starts/stops immediately without needing a preset
reselection.
- Add reentrancy guard to DatisRepository timer tick handler to prevent overlapping UpdateAsync runs when a cycle exceeds the timer interval - Publish "D-ATIS NOT AVBL." sentinel on exceptions so stale text is cleared instead of persisting until the next successful refresh - Add 1-second timeout to user-provided regex replacements in DatisTextProcessor to prevent ReDoS from pathological patterns - Enforce AutoFetchDatis → AutoFetchAtisLetter invariant during config load so persisted config cannot produce an invalid state at startup - Change DataGrid SelectionMode from Extended to Single to match the single-item delete behavior - Persist D-ATIS replacement edits via IProfileRepository instead of IAppConfig.SaveConfig(), matching all other config tab ViewModels Skipped two findings: - Explicit Mode=TwoWay / UpdateSourceTrigger=PropertyChanged on AXAML bindings: Avalonia DataGrid already defaults to TwoWay for editable columns, and UpdateSourceTrigger is a WPF-only concept that doesn't exist in Avalonia's binding system. - AutoFetchDatis toggle reactivity while a D-ATIS preset is already selected: this requires the user to open Settings, toggle the flag, and return — an uncommon flow. The existing preset-change handlers already start/stop monitoring on preset selection, which covers the normal usage. Adding a cross-VM subscription for this edge case adds coupling disproportionate to the benefit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@leftos does this allow for the manual removal/addition bit into a D-ATIS? |
…d NOTAMs Allow users to prepend and append custom text to D-ATIS airport conditions and NOTAMs sections independently, similar to vATISLoad's vatisloaduserconfig.json functionality. Text supports contraction expansion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Now it does |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
vATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs (1)
94-157: Move persistence out of text-bound setters to prevent repeated disk writes on keystroke.These four properties bind to
TextBox.Textwith Avalonia's defaultPropertyChangedtrigger, meaningSaveProfile()executes on every keystroke and performs synchronous disk I/O. This creates unnecessary repeated writes during editing. Keep the in-memory station updates in the setters, but consolidate persistence to a debounced, focus-loss, or explicit-apply path instead.Minimal change for text properties
public string PrependAirportConditions { get => _prependAirportConditions; set { this.RaiseAndSetIfChanged(ref _prependAirportConditions, value); if (SelectedStation != null) { SelectedStation.DatisPrependAirportConditions = value; - SaveProfile(); } } }Apply the same change to
AppendAirportConditions,PrependNotams, andAppendNotams.Consider consolidating all four saves into a single persistence point (e.g., on TextBox focus loss or a dedicated Save button) to reduce I/O contention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs` around lines 94 - 157, The setters for PrependAirportConditions, AppendAirportConditions, PrependNotams, and AppendNotams currently call SaveProfile() on every change; remove the SaveProfile() calls from each property's setter so they only update SelectedStation (SelectedStation.DatisPrependAirportConditions, DatisAppendAirportConditions, DatisPrependNotams, DatisAppendNotams) and keep RaiseAndSetIfChanged, and implement a single persistence trigger elsewhere (e.g., a debounced save, on TextBox focus loss, or an explicit Apply/Save command) that calls SaveProfile() once to persist the updated SelectedStation; ensure the SaveProfile() invocation is centralized rather than inside those property setters.vATIS.Desktop/Atis/DatisRepository.cs (1)
73-75: This fault-only continuation is currently dead.
FetchForStationAsynccatchesExceptioninternally, so the returned task never faults and theOnlyOnFaultedcontinuation never runs._ = FetchForStationAsync(station);is clearer unless you want to move failure handling out ofFetchForStationAsync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vATIS.Desktop/Atis/DatisRepository.cs` around lines 73 - 75, The continuation using TaskContinuationOptions.OnlyOnFaulted on FetchForStationAsync(station) is dead because FetchForStationAsync catches exceptions internally; remove the ContinueWith and simply call _ = FetchForStationAsync(station) (or alternatively rework FetchForStationAsync to let exceptions propagate and then keep the fault-only continuation), updating references around FetchForStationAsync and the Log.Error continuation accordingly.vATIS.Desktop/Atis/IDatisTextProcessor.cs (1)
29-38: Prefer a request/options type for this new public contract.Nine positional parameters here—especially the four adjacent
stringvalues—make call sites easy to misorder and harder to evolve. A small request/options record would make the API safer while this surface is still new.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vATIS.Desktop/Atis/IDatisTextProcessor.cs` around lines 29 - 38, The public method Process on IDatisTextProcessor exposes nine positional parameters which are error-prone; replace them with a single request/options type (e.g., DatisTextProcessRequest or DatisTextOptions) that contains properties for rawBody, stationId, atisLetter, contractions, replacements, prependAirportConditions, appendAirportConditions, prependNotams and appendNotams, update the IDatisTextProcessor.Process signature to accept that single request type and return DatisResult, and then update all implementations and callers to construct and pass the new request object (preserve null/optional semantics of char? and lists and add validation in the implementation as needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vATIS.Desktop/Atis/DatisRepository.cs`:
- Around line 70-82: The in-flight fetch started by MonitorStation captures the
old station and can republish results after RemoveStation has removed or changed
the monitored entry; fix FetchForStationAsync to check the current monitored
station in _monitoredStations before publishing or calling PublishNotAvailable
so stale results are dropped: after any await or just before any call that
publishes (including all PublishNotAvailable branches), look up
_monitoredStations[station.Id] (or TryGetValue) and ensure the referenced
station instance/Identifier matches the original/curr ent expected one (or bail
out if missing/changed), and apply this same guard in every PublishNotAvailable
branch noted in the diff so removed/updated stations aren’t repopulated.
In `@vATIS.Desktop/Atis/DatisTextProcessor.cs`:
- Around line 95-118: The try/catch around Regex.Replace and text.Replace
currently catches RegexParseException and RegexMatchTimeoutException but misses
ArgumentException thrown when the user-provided replacement string is malformed;
update the error handling in the block that checks replacement.IsRegex and calls
Regex.Replace (and text.Replace) to also catch ArgumentException (e.g., add a
catch for ArgumentException ex) and log a warning similar to the other catches
referencing replacement.Pattern and/or replacement.Replacement so a single bad
replacement rule is skipped instead of crashing the station update.
---
Nitpick comments:
In `@vATIS.Desktop/Atis/DatisRepository.cs`:
- Around line 73-75: The continuation using
TaskContinuationOptions.OnlyOnFaulted on FetchForStationAsync(station) is dead
because FetchForStationAsync catches exceptions internally; remove the
ContinueWith and simply call _ = FetchForStationAsync(station) (or alternatively
rework FetchForStationAsync to let exceptions propagate and then keep the
fault-only continuation), updating references around FetchForStationAsync and
the Log.Error continuation accordingly.
In `@vATIS.Desktop/Atis/IDatisTextProcessor.cs`:
- Around line 29-38: The public method Process on IDatisTextProcessor exposes
nine positional parameters which are error-prone; replace them with a single
request/options type (e.g., DatisTextProcessRequest or DatisTextOptions) that
contains properties for rawBody, stationId, atisLetter, contractions,
replacements, prependAirportConditions, appendAirportConditions, prependNotams
and appendNotams, update the IDatisTextProcessor.Process signature to accept
that single request type and return DatisResult, and then update all
implementations and callers to construct and pass the new request object
(preserve null/optional semantics of char? and lists and add validation in the
implementation as needed).
In `@vATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs`:
- Around line 94-157: The setters for PrependAirportConditions,
AppendAirportConditions, PrependNotams, and AppendNotams currently call
SaveProfile() on every change; remove the SaveProfile() calls from each
property's setter so they only update SelectedStation
(SelectedStation.DatisPrependAirportConditions, DatisAppendAirportConditions,
DatisPrependNotams, DatisAppendNotams) and keep RaiseAndSetIfChanged, and
implement a single persistence trigger elsewhere (e.g., a debounced save, on
TextBox focus loss, or an explicit Apply/Save command) that calls SaveProfile()
once to persist the updated SelectedStation; ensure the SaveProfile() invocation
is centralized rather than inside those property setters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eaa3e8b4-8ef3-414f-a9c9-b3aef0605834
📒 Files selected for processing (8)
vATIS.Desktop/Atis/DatisRepository.csvATIS.Desktop/Atis/DatisTextProcessor.csvATIS.Desktop/Atis/IDatisTextProcessor.csvATIS.Desktop/Config/AppConfig.csvATIS.Desktop/Container/Factory/ViewModelFactory.csvATIS.Desktop/Profiles/Models/AtisStation.csvATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axamlvATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- vATIS.Desktop/Container/Factory/ViewModelFactory.cs
Comment on lines
+70
to
+82
| public void MonitorStation(AtisStation station) | ||
| { | ||
| _monitoredStations[station.Id] = station; | ||
| FetchForStationAsync(station).ContinueWith( | ||
| t => Log.Error(t.Exception, "Error during initial D-ATIS fetch for {StationId}", station.Identifier), | ||
| TaskContinuationOptions.OnlyOnFaulted); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public void RemoveStation(string stationId) | ||
| { | ||
| _monitoredStations.Remove(stationId); | ||
| } |
Contributor
There was a problem hiding this comment.
Drop stale results from in-flight fetches.
MonitorStation kicks off a fetch immediately, RemoveStation only drops the dictionary entry, and FetchForStationAsync publishes from the captured station without checking whether that station is still the current monitored one. A request that started before a remove/update can therefore repopulate D-ATIS after auto-fetch was disabled or the preset changed.
Possible fix
+ private bool IsCurrentMonitoredStation(AtisStation station) =>
+ _monitoredStations.TryGetValue(station.Id, out var current) && ReferenceEquals(current, station);
+
private async Task FetchForStationAsync(AtisStation station)
{
try
{
@@
- PublishNotAvailable(station);
+ if (IsCurrentMonitoredStation(station))
+ {
+ PublishNotAvailable(station);
+ }
return;
}
@@
- EventBus.Instance.Publish(new DatisReceived(result));
+ if (IsCurrentMonitoredStation(station))
+ {
+ EventBus.Instance.Publish(new DatisReceived(result));
+ }
}
catch (Exception ex)
{
Log.Error(ex, "Error fetching D-ATIS for {Station}", station.Identifier);
- PublishNotAvailable(station);
+ if (IsCurrentMonitoredStation(station))
+ {
+ PublishNotAvailable(station);
+ }
}
}Apply the same guard to the other PublishNotAvailable branches too.
Also applies to: 112-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vATIS.Desktop/Atis/DatisRepository.cs` around lines 70 - 82, The in-flight
fetch started by MonitorStation captures the old station and can republish
results after RemoveStation has removed or changed the monitored entry; fix
FetchForStationAsync to check the current monitored station in
_monitoredStations before publishing or calling PublishNotAvailable so stale
results are dropped: after any await or just before any call that publishes
(including all PublishNotAvailable branches), look up
_monitoredStations[station.Id] (or TryGetValue) and ensure the referenced
station instance/Identifier matches the original/curr ent expected one (or bail
out if missing/changed), and apply this same guard in every PublishNotAvailable
branch noted in the diff so removed/updated stations aren’t repopulated.
Catch ArgumentException from malformed $ tokens in regex replacement strings to prevent a single bad rule from crashing the station update. Persist prepend/append text on focus loss instead of every keystroke to reduce unnecessary disk I/O. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
vATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs (1)
101-112: Consider saving on station change to prevent edge-case data loss.The property setters update
SelectedStationimmediately, but persistence only occurs on text box focus loss. If a user edits a field then directly clicks a different station (without tabbing out), the edit is written to the model but never persisted. This is a minor UX edge case.💡 Optional: Save before loading new station
private void HandleAtisStationChanged(AtisStation? station) { if (station == null) { return; } + // Persist any pending edits from the previous station + if (SelectedStation != null) + { + SaveProfile(); + } + SelectedStation = station;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs` around lines 101 - 112, The PrependAirportConditions setter updates SelectedStation but persistence only happens on textbox focus loss, causing edits to be lost when switching stations; fix by invoking the persistence routine before changing stations (e.g., call the existing save method such as SaveSelectedStation() or PersistSelectedStationChanges() from the SelectedStation property setter just before assigning a new station), ensure the call is null-safe and await it if it's asynchronous so any in-flight edits are persisted before the new station is loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vATIS.Desktop/Atis/DatisTextProcessor.cs`:
- Around line 86-88: Apply a defensive null/empty check in ApplyReplacements so
it does not iterate when replacements is null or empty: if replacements is null
(or has no elements) simply return the original text unchanged; also defensively
skip any null entries inside the list when iterating over DatisTextReplacement
items. Update the ApplyReplacements(string text, List<DatisTextReplacement>
replacements) method to perform these checks before the foreach so callers
(e.g., where station.DatisTextReplacements is passed) cannot trigger a
NullReferenceException.
- Around line 165-171: In ExpandContractions, guard against a null contractions
parameter before running the LINQ chain: check if contractions is null (same
pattern used for replacements) and treat it as empty (or return the original
text) so the .Where/.OrderByDescending call cannot throw; update the method that
uses the local variable sorted (ExpandContractions) to handle the empty result
accordingly.
- Around line 97-105: The Regex.Replace call can throw ArgumentNullException if
replacement.Replacement is null; update the logic in DatisTextProcessor (the
branch using replacement.IsRegex, replacement.Pattern and Regex.Replace) to
guard against null by checking replacement.Replacement before calling
Regex.Replace and either skip the replacement, use a safe fallback (e.g.
String.Empty) or log and continue; ensure the null check is applied only for the
regex branch so Regex.Replace is never invoked with a null replacement argument.
---
Nitpick comments:
In `@vATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs`:
- Around line 101-112: The PrependAirportConditions setter updates
SelectedStation but persistence only happens on textbox focus loss, causing
edits to be lost when switching stations; fix by invoking the persistence
routine before changing stations (e.g., call the existing save method such as
SaveSelectedStation() or PersistSelectedStationChanges() from the
SelectedStation property setter just before assigning a new station), ensure the
call is null-safe and await it if it's asynchronous so any in-flight edits are
persisted before the new station is loaded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b22cf02c-6032-4938-a2de-666a299b9199
📒 Files selected for processing (4)
vATIS.Desktop/Atis/DatisTextProcessor.csvATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axamlvATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axaml.csvATIS.Desktop/Ui/ViewModels/AtisConfiguration/DatisReplacementsViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- vATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axaml.cs
- vATIS.Desktop/Ui/AtisConfiguration/DatisReplacementsView.axaml
Comment on lines
+86
to
+88
| private static string ApplyReplacements(string text, List<DatisTextReplacement> replacements) | ||
| { | ||
| foreach (var replacement in replacements) |
Contributor
There was a problem hiding this comment.
Add defensive null checks for list parameters.
If replacements is null, the foreach loop throws NullReferenceException. The caller (see DatisRepository.cs:155-164) passes station.DatisTextReplacements directly without null-checking.
🛡️ Proposed fix
private static string ApplyReplacements(string text, List<DatisTextReplacement> replacements)
{
+ if (replacements == null)
+ {
+ return text;
+ }
+
foreach (var replacement in replacements)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vATIS.Desktop/Atis/DatisTextProcessor.cs` around lines 86 - 88, Apply a
defensive null/empty check in ApplyReplacements so it does not iterate when
replacements is null or empty: if replacements is null (or has no elements)
simply return the original text unchanged; also defensively skip any null
entries inside the list when iterating over DatisTextReplacement items. Update
the ApplyReplacements(string text, List<DatisTextReplacement> replacements)
method to perform these checks before the foreach so callers (e.g., where
station.DatisTextReplacements is passed) cannot trigger a
NullReferenceException.
Comment on lines
+97
to
+105
| if (replacement.IsRegex) | ||
| { | ||
| text = Regex.Replace( | ||
| text, | ||
| replacement.Pattern + @"[,.;]{0,1}", | ||
| replacement.Replacement, | ||
| RegexOptions.None, | ||
| TimeSpan.FromSeconds(1)); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against null replacement.Replacement to prevent ArgumentNullException.
Regex.Replace throws ArgumentNullException if the replacement parameter is null. If DatisTextReplacement.Replacement is nullable or uninitialized, this will crash the processing loop.
🛡️ Proposed fix
if (replacement.IsRegex)
{
text = Regex.Replace(
text,
replacement.Pattern + @"[,.;]{0,1}",
- replacement.Replacement,
+ replacement.Replacement ?? string.Empty,
RegexOptions.None,
TimeSpan.FromSeconds(1));
}
else
{
- text = text.Replace(replacement.Pattern, replacement.Replacement);
+ text = text.Replace(replacement.Pattern, replacement.Replacement ?? string.Empty);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vATIS.Desktop/Atis/DatisTextProcessor.cs` around lines 97 - 105, The
Regex.Replace call can throw ArgumentNullException if replacement.Replacement is
null; update the logic in DatisTextProcessor (the branch using
replacement.IsRegex, replacement.Pattern and Regex.Replace) to guard against
null by checking replacement.Replacement before calling Regex.Replace and either
skip the replacement, use a safe fallback (e.g. String.Empty) or log and
continue; ensure the null check is applied only for the regex branch so
Regex.Replace is never invoked with a null replacement argument.
Comment on lines
+165
to
+171
| private static string ExpandContractions(string text, List<ContractionMeta> contractions) | ||
| { | ||
| var sorted = contractions | ||
| .Where(c => !string.IsNullOrEmpty(c.Text) && !string.IsNullOrEmpty(c.VariableName)) | ||
| .Where(c => !c.Text!.All(char.IsDigit)) | ||
| .OrderByDescending(c => c.Text!.Length) | ||
| .ToList(); |
Contributor
There was a problem hiding this comment.
Add null guard for contractions parameter.
Similar to replacements, if contractions is null, the LINQ query throws NullReferenceException.
🛡️ Proposed fix
private static string ExpandContractions(string text, List<ContractionMeta> contractions)
{
+ if (contractions == null)
+ {
+ return text;
+ }
+
var sorted = contractions📝 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.
Suggested change
| private static string ExpandContractions(string text, List<ContractionMeta> contractions) | |
| { | |
| var sorted = contractions | |
| .Where(c => !string.IsNullOrEmpty(c.Text) && !string.IsNullOrEmpty(c.VariableName)) | |
| .Where(c => !c.Text!.All(char.IsDigit)) | |
| .OrderByDescending(c => c.Text!.Length) | |
| .ToList(); | |
| private static string ExpandContractions(string text, List<ContractionMeta> contractions) | |
| { | |
| if (contractions == null) | |
| { | |
| return text; | |
| } | |
| var sorted = contractions | |
| .Where(c => !string.IsNullOrEmpty(c.Text) && !string.IsNullOrEmpty(c.VariableName)) | |
| .Where(c => !c.Text!.All(char.IsDigit)) | |
| .OrderByDescending(c => c.Text!.Length) | |
| .ToList(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vATIS.Desktop/Atis/DatisTextProcessor.cs` around lines 165 - 171, In
ExpandContractions, guard against a null contractions parameter before running
the LINQ chain: check if contractions is null (same pattern used for
replacements) and treat it as empty (or return the original text) so the
.Where/.OrderByDescending call cannot throw; update the method that uses the
local variable sorted (ExpandContractions) to handle the empty result
accordingly.
Prevents data loss when a user edits a prepend/append field then clicks a different station without tabbing out first. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
New files
Atis/DatisResult.csAtis/IDatisTextProcessor.cs/DatisTextProcessor.csAtis/IDatisRepository.cs/DatisRepository.csDispatcherTimer), publishesDatisReceivedeventsEvents/DatisReceived.csProfiles/Models/DatisTextReplacement.csUi/ViewModels/AtisConfiguration/DatisReplacementsViewModel.csUi/AtisConfiguration/DatisReplacementsView.axaml(.cs)Modified files
AtisStation.csDatisTextReplacementsproperty + CloneSourceGenerationContext.csDatisTextReplacementtypesServiceProvider.csIDatisTextProcessor,IDatisRepository,DatisReplacementsViewModelViewModelFactory.cs/IViewModelFactory.csAtisStationViewModel.csDatisReceived, manages D-ATIS monitoring lifecycleAtisConfigurationWindowViewModel.csDatisReplacementsViewModelAtisConfigurationWindow.axamlIAppConfig.cs/AppConfig.csAutoFetchDatissettingSettingsDialogViewModel.csAutoFetchDatiswith coupling toAutoFetchAtisLetterSettingsDialog.axamlTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI
Settings