Feature/container logs - #20
Conversation
- Implemented ContainerLogsDialog and ContainerLogsDialogViewModel for displaying logs of a selected container. - Added functionality to stream logs with options for following, timestamps, and limiting output. - Introduced ContainerLogsOptions to encapsulate log streaming parameters. - Updated IContainerCliProvider to support log streaming commands. - Enhanced existing ContainersViewModel to trigger the logs dialog. - Added tests for ContainerLogsDialogViewModel and command argument building for logs. - Updated .gitignore to exclude publish directories and added Inlines to VSCode settings.
…methods - Removed summary comments from EnvironmentVariableEntry, PortMappingEntry, RunContainerDialogViewModel, VolumeMountEntry, MainWindowViewModel, SidebarViewModel, ContainerItem, ImageItem, NavigationItem, Program, ServiceCollectionExtensions, IThemeService, ThemeService, ViewLocator, AppleContainerCliProvider, AppleContainerSystemService, ContainerCliProviderBase, AggregateCliCommandException, CliCommandException, IContainerCliProvider, CliOutputLine, Container, ContainerImage, ContainerLogsOptions, ContainerSystemStatus, PlatformInfo, RunContainerOptions, ServiceCollectionExtensions, WslcCliProvider, and SidebarViewModelTests.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds container log streaming, reusable dialog and refresh services, terminal output copying, and feature integration. It also updates C# conventions, removes or shortens XML documentation, reformats UI markup, changes logging configuration, and adds repository release documentation. ChangesContainer logs, dialogs, refresh, and UI integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ContainersViewModel
participant IDialogService
participant ContainerLogsDialogViewModel
participant IContainerCliProvider
Operator->>ContainersViewModel: Execute LogsCommand
ContainersViewModel->>IDialogService: Show ContainerLogsDialogViewModel
IDialogService->>ContainerLogsDialogViewModel: Resolve and configure
IDialogService-->>Operator: Display ContainerLogsDialog
ContainerLogsDialogViewModel->>IContainerCliProvider: StreamContainerLogsAsync
IContainerCliProvider-->>ContainerLogsDialogViewModel: Return CliOutputLine values
ContainerLogsDialogViewModel->>ContainerLogsDialogViewModel: Update OutputLines and TerminalState
Operator->>ContainerLogsDialogViewModel: Execute StopCommand or CloseCommand
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
src/Knarr.App/Features/Images/ImagesViewModel.cs (1)
242-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the comment added above private
LoadAsync.This PR adds a private-member comment, but repository guidance says to avoid comments/XML documentation on private or internal members. Remove it or encode the behavior through clearer naming/structure.
As per coding guidelines, avoid comments/XML docs on private or internal members.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.App/Features/Images/ImagesViewModel.cs` around lines 242 - 243, Remove the two-line comment immediately above the private LoadAsync method, leaving the method implementation and behavior unchanged.Source: Coding guidelines
src/Knarr.App/Controls/TerminalOutputView.axaml.cs (1)
209-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the private-method comment.
As per coding guidelines, “avoid comments/XML documentation on private or internal members.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.App/Controls/TerminalOutputView.axaml.cs` around lines 209 - 210, Remove the private-method comment shown in the diff, leaving the surrounding implementation unchanged.Source: Coding guidelines
src/Knarr.Service/AppleContainerCli/AppleContainerSystemService.cs (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the internal-member comment.
As per coding guidelines, “avoid comments/XML documentation on private or internal members.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.Service/AppleContainerCli/AppleContainerSystemService.cs` at line 56, Remove the internal-member comment in AppleContainerSystemService, leaving the surrounding implementation unchanged.Source: Coding guidelines
tests/Knarr.Service.Tests/ContainerLogsCommandTests.cs (1)
6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove XML documentation from the test class.
As per coding guidelines, “add /// XML documentation only to public interfaces and shared abstract base classes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Knarr.Service.Tests/ContainerLogsCommandTests.cs` around lines 6 - 10, Remove the XML documentation comment from the test class surrounding ContainerCliProviderBase.BuildLogsArgs and ContainerLogsOptions references, leaving the test implementation unchanged.Source: Coding guidelines
src/Knarr.Service/ContainerCliProviderBase.cs (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove comments from internal/private members.
ContainerCliProviderBaseis internal, andBuildRunArgsis private. As per coding guidelines, “avoid comments/XML documentation on private or internal members” and “add /// XML documentation only to public interfaces and shared abstract base classes.”Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.Service/ContainerCliProviderBase.cs` around lines 47 - 48, Remove the XML documentation comment from the internal ContainerCliProviderBase member LogsCommand and the private BuildRunArgs member, while leaving their declarations and behavior unchanged.Source: Coding guidelines
src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml (1)
8-9: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid fixed dialog dimensions.
Hard-coded dimensions do not adapt to text scaling or constrained displays; use content/star sizing and only necessary bounds.
As per coding guidelines,
**/*.axamlshould “avoid fixed pixel sizing.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml` around lines 8 - 9, Replace the fixed Width, Height, MinWidth, and MinHeight values in the ContainerLogsDialog layout with content- or star-based sizing, retaining only bounds that are necessary for usability and ensuring the dialog adapts to text scaling and constrained displays.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.editorconfig:
- Around line 14-18: Apply two-space AXAML formatting consistently: update
.editorconfig lines 14-18 to include axaml and xaml in the two-space override;
reindent the changed wrapper block in
src/Knarr.App/Features/Settings/SettingsView.axaml lines 11-16 and the changed
template block in src/Knarr.App/Features/Sidebar/Sidebar.axaml lines 170-198 to
two spaces.
In @.gitignore:
- Line 12: Update the Publish ignore pattern in .gitignore to use a fully
case-insensitive matching form, so every casing variant of the Publish directory
is ignored.
In `@src/Knarr.App/Controls/TerminalOutputView.axaml`:
- Around line 40-48: Update the icon-only PART_CopyButton by adding
AutomationProperties.Name with the accessible name “Copy all output”; leave its
existing visibility, tooltip, and icon configuration unchanged.
In `@src/Knarr.App/Controls/TerminalOutputView.axaml.cs`:
- Around line 211-247: Update TerminalOutputView’s RebuildInlines and streaming
output flow so OutputLines remains bounded to the selected tail limit, removing
oldest entries as new lines arrive. Reuse and append existing Inline elements
for newly added lines, removing only the corresponding leading inlines when the
tail evicts entries, instead of clearing and rebuilding the entire collection on
every CollectionChanged event.
In
`@src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml.cs`:
- Around line 9-34: Move dialog lifecycle and event orchestration out of
code-behind: in
src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml.cs#L9-L34,
replace OnDataContextChanged, OnCloseRequested, and OnClosed wiring with an
AXAML-bound behavior or framework-agnostic presentation abstraction, leaving
only InitializeComponent(). In
src/Knarr.App/Features/Containers/ContainersView.axaml.cs#L17-L60, replace the
logs-dialog event subscription and Show/ShowDialog handling with that same
presentation mechanism, leaving only InitializeComponent() in the feature
code-behind.
- Around line 27-33: Update ContainerLogsDialog.OnClosed to invoke the view
model’s existing close/cancellation behavior before unsubscribing CloseRequested
and clearing _viewModel. Ensure title-bar and command-driven closures both
cancel any active follow stream, then preserve the existing event-handler
cleanup.
In
`@src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cs`:
- Around line 119-121: Update the restart flow around RequestCancellation and
StreamAsync so a canceled stream cannot mutate shared output, TerminalState, or
IsStreaming after a replacement begins. Serialize stream restarts or associate
each run with its current cancellation source/generation, and guard every stream
mutation so only the active run can apply state changes.
- Around line 31-32: Add [NotifyPropertyChangedFor(nameof(ShortId))] to the
ContainerId observable property declaration so changing ContainerId also raises
the ShortId notification and refreshes the bound computed value during
Reset(...).
In `@src/Knarr.App/Features/Dashboard/DashboardView.axaml`:
- Around line 11-16: Reindent the new Panel block in DashboardView, including
its TextBlock and attributes, from four-space indentation to the
repository-standard two-space AXAML indentation without changing its content or
behavior.
In `@src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cs`:
- Line 32: Update the clipboard lookup in RunContainerDialog to call the fully
qualified Avalonia.Controls.TopLevel.GetTopLevel(this) method, preserving the
existing clipboard pattern and behavior.
In `@src/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cs`:
- Around line 177-179: Condense the cancellation rationale comment in the
RunContainerDialogViewModel cancellation flow to one concise line explaining
that CancellationTokenSource.Cancel can block the UI thread on Windows, so
cancellation is offloaded to a background thread.
In `@src/Knarr.Service/ContainerCliProviderBase.cs`:
- Around line 172-205: The shared BuildLogsArgs flow currently emits flags
unsupported by Apple’s container CLI. Update
src/Knarr.Service/ContainerCliProviderBase.cs lines 172-205 to support
provider-specific log arguments or reject unsupported options, update
src/Knarr.Service/AppleContainerCli/AppleContainerCliProvider.cs line 23 to map
TailLines to -n and expose unsupported timestamp/time-filter capabilities, and
update tests/Knarr.Service.Tests/ContainerLogsCommandTests.cs lines 95-102 to
verify Apple’s -n output and unsupported-option behavior.
- Around line 208-209: Update FormatTimestamp to use a round-trip timestamp
format that preserves fractional seconds while retaining UTC and
invariant-culture output for --since/--until values. Extend the existing
log-arguments test to verify fractional DateTimeOffset boundaries are emitted
with their fractional precision.
---
Nitpick comments:
In `@src/Knarr.App/Controls/TerminalOutputView.axaml.cs`:
- Around line 209-210: Remove the private-method comment shown in the diff,
leaving the surrounding implementation unchanged.
In `@src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml`:
- Around line 8-9: Replace the fixed Width, Height, MinWidth, and MinHeight
values in the ContainerLogsDialog layout with content- or star-based sizing,
retaining only bounds that are necessary for usability and ensuring the dialog
adapts to text scaling and constrained displays.
In `@src/Knarr.App/Features/Images/ImagesViewModel.cs`:
- Around line 242-243: Remove the two-line comment immediately above the private
LoadAsync method, leaving the method implementation and behavior unchanged.
In `@src/Knarr.Service/AppleContainerCli/AppleContainerSystemService.cs`:
- Line 56: Remove the internal-member comment in AppleContainerSystemService,
leaving the surrounding implementation unchanged.
In `@src/Knarr.Service/ContainerCliProviderBase.cs`:
- Around line 47-48: Remove the XML documentation comment from the internal
ContainerCliProviderBase member LogsCommand and the private BuildRunArgs member,
while leaving their declarations and behavior unchanged.
In `@tests/Knarr.Service.Tests/ContainerLogsCommandTests.cs`:
- Around line 6-10: Remove the XML documentation comment from the test class
surrounding ContainerCliProviderBase.BuildLogsArgs and ContainerLogsOptions
references, leaving the test implementation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99de30b6-4555-4416-95fd-c581a65f21c0
📒 Files selected for processing (63)
.editorconfig.github/copilot-instructions.md.github/skills/better-code/SKILL.md.gitignore.vscode/settings.jsonDirectory.Packages.propssrc/Knarr.App/Controls/Pill.axaml.cssrc/Knarr.App/Controls/TerminalOutputView.axamlsrc/Knarr.App/Controls/TerminalOutputView.axaml.cssrc/Knarr.App/Controls/Tile.axaml.cssrc/Knarr.App/Converters/CliOutputKindToBrushConverter.cssrc/Knarr.App/Converters/IconKeyToGeometryConverter.cssrc/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axamlsrc/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml.cssrc/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cssrc/Knarr.App/Features/Containers/ContainersView.axamlsrc/Knarr.App/Features/Containers/ContainersView.axaml.cssrc/Knarr.App/Features/Containers/ContainersViewModel.cssrc/Knarr.App/Features/Dashboard/DashboardView.axamlsrc/Knarr.App/Features/Images/ImagesView.axamlsrc/Knarr.App/Features/Images/ImagesViewModel.cssrc/Knarr.App/Features/Images/PullImageDialogViewModel.cssrc/Knarr.App/Features/RunContainer/EnvironmentVariableEntry.cssrc/Knarr.App/Features/RunContainer/PortMappingEntry.cssrc/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cssrc/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cssrc/Knarr.App/Features/RunContainer/VolumeMountEntry.cssrc/Knarr.App/Features/Settings/SettingsView.axamlsrc/Knarr.App/Features/Shell/MainWindowViewModel.cssrc/Knarr.App/Features/Sidebar/Sidebar.axamlsrc/Knarr.App/Features/Sidebar/SidebarViewModel.cssrc/Knarr.App/Knarr.App.csprojsrc/Knarr.App/Models/ContainerItem.cssrc/Knarr.App/Models/ImageItem.cssrc/Knarr.App/Models/NavigationItem.cssrc/Knarr.App/Program.cssrc/Knarr.App/ServiceCollectionExtensions.cssrc/Knarr.App/Services/IThemeService.cssrc/Knarr.App/Services/ThemeService.cssrc/Knarr.App/ViewLocator.cssrc/Knarr.Service/AppleContainerCli/AppleContainerCliProvider.cssrc/Knarr.Service/AppleContainerCli/AppleContainerSystemService.cssrc/Knarr.Service/ContainerCliProviderBase.cssrc/Knarr.Service/Exceptions/AggregateCliCommandException.cssrc/Knarr.Service/Exceptions/CliCommandException.cssrc/Knarr.Service/IContainerCliProvider.cssrc/Knarr.Service/Models/CliOutputLine.cssrc/Knarr.Service/Models/Container.cssrc/Knarr.Service/Models/ContainerImage.cssrc/Knarr.Service/Models/ContainerLogsOptions.cssrc/Knarr.Service/Models/ContainerSystemStatus.cssrc/Knarr.Service/Models/PlatformInfo.cssrc/Knarr.Service/Models/RunContainerOptions.cssrc/Knarr.Service/ServiceCollectionExtensions.cssrc/Knarr.Service/WslcCli/WslcCliProvider.cstests/Knarr.App.Tests/Features/Containers/ContainerLogs/ContainerLogsDialogViewModelTests.cstests/Knarr.App.Tests/Features/Containers/ContainersViewModelTests.cstests/Knarr.App.Tests/Features/Images/PullImageDialogViewModelTests.cstests/Knarr.App.Tests/Features/RunContainer/RunContainerDialogViewModelTests.cstests/Knarr.App.Tests/Features/Sidebar/SidebarViewModelTests.cstests/Knarr.Service.Tests/AppleContainerSystemServiceTests.cstests/Knarr.Service.Tests/ContainerLogsCommandTests.cstests/Knarr.Service.Tests/RunContainerCommandTests.cs
💤 Files with no reviewable changes (21)
- src/Knarr.Service/ServiceCollectionExtensions.cs
- src/Knarr.Service/Models/PlatformInfo.cs
- src/Knarr.App/Features/RunContainer/VolumeMountEntry.cs
- src/Knarr.App/Controls/Tile.axaml.cs
- src/Knarr.App/Knarr.App.csproj
- tests/Knarr.App.Tests/Features/Sidebar/SidebarViewModelTests.cs
- src/Knarr.Service/Models/ContainerImage.cs
- src/Knarr.App/Features/RunContainer/PortMappingEntry.cs
- src/Knarr.App/Models/NavigationItem.cs
- src/Knarr.App/Features/RunContainer/EnvironmentVariableEntry.cs
- src/Knarr.App/Services/IThemeService.cs
- src/Knarr.App/Features/Shell/MainWindowViewModel.cs
- src/Knarr.App/Services/ThemeService.cs
- src/Knarr.App/Models/ContainerItem.cs
- src/Knarr.App/Controls/Pill.axaml.cs
- src/Knarr.Service/Models/ContainerSystemStatus.cs
- src/Knarr.Service/Exceptions/AggregateCliCommandException.cs
- src/Knarr.Service/Models/Container.cs
- src/Knarr.Service/Models/CliOutputLine.cs
- src/Knarr.Service/Models/RunContainerOptions.cs
- Directory.Packages.props
| # JSON, XML, YAML and web-related overrides | ||
| [*.{json,xml,yml,yaml,html,css}] | ||
| indent_size = 2 | ||
| tab_width = 2 | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Apply the required two-space AXAML formatting consistently.
The .editorconfig override excludes AXAML, and both changed AXAML blocks remain four-space indented.
.editorconfig#L14-L18: includeaxamlandxamlin the two-space override.src/Knarr.App/Features/Settings/SettingsView.axaml#L11-L16: reindent the changed wrapper block to two spaces.src/Knarr.App/Features/Sidebar/Sidebar.axaml#L170-L198: reindent the changed template block to two spaces.
As per coding guidelines, AXAML files must use two-space indentation.
📍 Affects 3 files
.editorconfig#L14-L18(this comment)src/Knarr.App/Features/Settings/SettingsView.axaml#L11-L16src/Knarr.App/Features/Sidebar/Sidebar.axaml#L170-L198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.editorconfig around lines 14 - 18, Apply two-space AXAML formatting
consistently: update .editorconfig lines 14-18 to include axaml and xaml in the
two-space override; reindent the changed wrapper block in
src/Knarr.App/Features/Settings/SettingsView.axaml lines 11-16 and the changed
template block in src/Knarr.App/Features/Sidebar/Sidebar.axaml lines 170-198 to
two spaces.
Source: Coding guidelines
| *.suo | ||
| artifacts/ | ||
| /packages/ | ||
| [Pp]ublish/ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the Publish ignore pattern fully case-insensitive.
[Pp]ublish/ matches only Publish/ and publish/; it does not cover other casing variants despite the stated intent.
Proposed fix
-[Pp]ublish/
+[Pp][Uu][Bb][Ll][Ii][Ss][Hh]/📝 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.
| [Pp]ublish/ | |
| [Pp][Uu][Bb][Ll][Ii][Ss][Hh]/ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitignore at line 12, Update the Publish ignore pattern in .gitignore to
use a fully case-insensitive matching form, so every casing variant of the
Publish directory is ignored.
| <Button Name="PART_CopyButton" | ||
| Classes="icon" | ||
| HorizontalAlignment="Right" | ||
| VerticalAlignment="Top" | ||
| Margin="6" | ||
| IsVisible="{Binding HasOutput, RelativeSource={RelativeSource TemplatedParent}}" | ||
| ToolTip.Tip="Copy all"> | ||
| <PathIcon Data="{StaticResource CopyRegular}" Width="14" Height="14" /> | ||
| </Button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching TerminalOutputView.axaml:"
fd -a 'TerminalOutputView\.axaml$' . || true
file="$(fd 'TerminalOutputView\.axaml$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "---- $file ----"
wc -l "$file"
sed -n '1,120p' "$file"
fi
echo "---- Avalonia Button docs references / dependencies ----"
rg -n "Avalonia|Avalonia\.Controls|Button|AutomationProperties\.Name" -S --glob "*.csproj" --glob "*.axaml" --glob "*.props" --glob "*.targets" . | head -n 80 || trueRepository: hard-rox/knarr
Length of output: 11941
🌐 Web query:
Avalonia UI button accessibility AutomationProperties.Name icon-only button documentation
💡 Result:
In Avalonia UI, icon-only buttons are not automatically descriptive to assistive technologies because they lack text content [1][2]. To ensure accessibility for these controls, you must explicitly provide an accessible name using the AutomationProperties.Name attached property [1][2]. When a button contains only an icon, screen readers will not be able to identify its purpose without this metadata [1][2]. By setting AutomationProperties.Name, you provide the descriptive text that a screen reader announces when the button receives focus [1][2]. Implementation example: Key Accessibility Guidelines: - Use AutomationProperties.Name: This is the primary mechanism to label controls that do not have visible text or where the visible text is insufficient [1][2]. - When to set it explicitly: You should explicitly set the Name property when the button content is an icon or image, when the text is ambiguous, or when the control has no visible content [2][3]. - Do not affect visual UI: AutomationProperties are designed to supply metadata for assistive technologies and do not change the visual appearance of your controls [2][3]. - Consistency: Ensure that the string provided in AutomationProperties.Name is concise, meaningful, and clearly describes the action of the button [1][2].
Citations:
- 1: https://docs.avaloniaui.net/controls/input/buttons/button
- 2: https://docs.avaloniaui.net/docs/app-development/accessibility
- 3: https://docs.avaloniaui.net/docs/app-development/accessibility.md
Give the icon-only copy button an accessible name.
The button’s contents are only a PathIcon, and the tooltip is not exposed to assistive technologies. Set AutomationProperties.Name="Copy all output" to make it accessible.
Proposed fix
<Button Name="PART_CopyButton"
Classes="icon"
+ AutomationProperties.Name="Copy all output"
HorizontalAlignment="Right"📝 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.
| <Button Name="PART_CopyButton" | |
| Classes="icon" | |
| HorizontalAlignment="Right" | |
| VerticalAlignment="Top" | |
| Margin="6" | |
| IsVisible="{Binding HasOutput, RelativeSource={RelativeSource TemplatedParent}}" | |
| ToolTip.Tip="Copy all"> | |
| <PathIcon Data="{StaticResource CopyRegular}" Width="14" Height="14" /> | |
| </Button> | |
| <Button Name="PART_CopyButton" | |
| Classes="icon" | |
| AutomationProperties.Name="Copy all output" | |
| HorizontalAlignment="Right" | |
| VerticalAlignment="Top" | |
| Margin="6" | |
| IsVisible="{Binding HasOutput, RelativeSource={RelativeSource TemplatedParent}}" | |
| ToolTip.Tip="Copy all"> | |
| <PathIcon Data="{StaticResource CopyRegular}" Width="14" Height="14" /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/Controls/TerminalOutputView.axaml` around lines 40 - 48, Update
the icon-only PART_CopyButton by adding AutomationProperties.Name with the
accessible name “Copy all output”; leave its existing visibility, tooltip, and
icon configuration unchanged.
| private void RebuildInlines() | ||
| { | ||
| if (_outputText is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _outputText.Inlines ??= new InlineCollection(); | ||
| _outputText.Inlines.Clear(); | ||
|
|
||
| if (Lines is not IEnumerable lines) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| List<CliOutputLine> outputLines = []; | ||
| foreach (object? item in lines) | ||
| { | ||
| if (item is CliOutputLine line) | ||
| { | ||
| outputLines.Add(line); | ||
| } | ||
| } | ||
|
|
||
| for (int i = 0; i < outputLines.Count; i++) | ||
| { | ||
| CliOutputLine line = outputLines[i]; | ||
| IBrush? brush = CliOutputKindToBrushConverter.Instance.Convert( | ||
| line.Kind, typeof(IBrush), null, CultureInfo.InvariantCulture) as IBrush; | ||
|
|
||
| _outputText.Inlines.Add(new Run(line.Text) { Foreground = brush }); | ||
|
|
||
| if (i < outputLines.Count - 1) | ||
| { | ||
| _outputText.Inlines.Add(new LineBreak()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cs --items all
rg -n --type cs -C 4 'ObservableCollection<.*CliOutputLine>|Lines\s*\{|RemoveAt\(|Max.*Lines|Limit' \
src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.csRepository: hard-rox/knarr
Length of output: 2574
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ContainerLogsDialogViewModel relevant sections ==="
nl -ba src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cs | sed -n '55,190p'
echo
echo "=== Search CliOutputLine and collection usages ==="
rg -n --type cs -C 3 'CliOutputLine|OutputLines|Add\(|Remove|Capacity|Max.*Line|Keep|Trim|Limit|TailLines' src/Knarr.App/Features/Containers src/Knarr.App/Controls TerminalOutputView.axaml.cs 2>/dev/null || true
echo
echo "=== TerminalOutputView relevant section ==="
nl -ba src/Knarr.App/Controls/TerminalOutputView.axaml.cs | sed -n '1,280p'Repository: hard-rox/knarr
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ContainerLogsDialogViewModel relevant sections ==="
awk '{printf "%5d\t%s\n", NR, $0}' src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cs | sed -n '55,190p'
echo
echo "=== Search collection usages in ContainerLogs and TerminalOutputView ==="
rg -n -C 3 'CliOutputLine|OutputLines|\.Add\(|Remove|RemoveAt|Clear|Capacity|Max.*Line|Keep|Trim|Limit|TailLines' \
src/Knarr.App/Features/Containers \
src/Knarr.App/Controls/TerminalOutputView.axaml.cs \
|| true
echo
echo "=== TerminalOutputView relevant section ==="
awk '{printf "%5d\t%s\n", NR, $0}' src/Knarr.App/Controls/TerminalOutputView.axaml.cs | sed -n '1,280p'Repository: hard-rox/knarr
Length of output: 32609
Bound and append container log output incrementally.
StreamAsync() clears OutputLines once, then adds every streamed line forever while TerminalOutputView rebuilds the whole inlined text on each CollectionChanged. A continuous follow stream can become quadratic; keep the lines collection bounded by the selected tail limit and reuse existing inlines instead of rebuilding everything.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/Controls/TerminalOutputView.axaml.cs` around lines 211 - 247,
Update TerminalOutputView’s RebuildInlines and streaming output flow so
OutputLines remains bounded to the selected tail limit, removing oldest entries
as new lines arrive. Reuse and append existing Inline elements for newly added
lines, removing only the corresponding leading inlines when the tail evicts
entries, instead of clearing and rebuilding the entire collection on every
CollectionChanged event.
| { | ||
| if (_viewModel is { CommandPreview: { Length: > 0 } command } && | ||
| TopLevel.GetTopLevel(this)?.Clipboard is { } clipboard) | ||
| GetTopLevel(this)?.Clipboard is { } clipboard) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cs"
rg -n 'using static Avalonia\.Controls\.TopLevel|GetTopLevel\(' "$file"Repository: hard-rox/knarr
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cs"
echo "== file imports and relevant code =="
sed -n '1,80p' "$file"
echo "== repo usages of GetTopLevel =="
rg -n 'GetTopLevel\(' .Repository: hard-rox/knarr
Length of output: 2344
Use the fully qualified TopLevel.GetTopLevel(this) call here.
RunContainerDialog.axaml.cs only imports Avalonia.Controls; it does not declare using static Avalonia.Controls.TopLevel;, so the unqualified GetTopLevel(...) won’t compile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cs` at line 32,
Update the clipboard lookup in RunContainerDialog to call the fully qualified
Avalonia.Controls.TopLevel.GetTopLevel(this) method, preserving the existing
clipboard pattern and behavior.
| // CliWrap runs its graceful (Ctrl+C) cancellation handler synchronously inside | ||
| // CancellationTokenSource.Cancel(); on Windows that console signalling is blocking, so cancelling | ||
| // on the UI thread would freeze the app. Offload to a background thread instead. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Condense the cancellation rationale to one line.
The rationale is valid, but the repository’s comment discipline requires a single concise why-comment rather than a three-line explanation.
As per coding guidelines, use the better-code discipline for comments and keep exceptional rationale concise and one-line.
Proposed fix
- // CliWrap runs its graceful (Ctrl+C) cancellation handler synchronously inside
- // CancellationTokenSource.Cancel(); on Windows that console signalling is blocking, so cancelling
- // on the UI thread would freeze the app. Offload to a background thread instead.
+ // Cancel off the UI thread because CLI cancellation can block on Windows.📝 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.
| // CliWrap runs its graceful (Ctrl+C) cancellation handler synchronously inside | |
| // CancellationTokenSource.Cancel(); on Windows that console signalling is blocking, so cancelling | |
| // on the UI thread would freeze the app. Offload to a background thread instead. | |
| // Cancel off the UI thread because CLI cancellation can block on Windows. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cs` around
lines 177 - 179, Condense the cancellation rationale comment in the
RunContainerDialogViewModel cancellation flow to one concise line explaining
that CancellationTokenSource.Cancel can block the UI thread on Windows, so
cancellation is offloaded to a background thread.
Source: Coding guidelines
| private static string FormatTimestamp(DateTimeOffset timestamp) | ||
| => timestamp.ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ssZ", CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File outline =="
ast-grep outline src/Knarr.Service/ContainerCliProviderBase.cs --view expanded || true
echo "== Relevant lines =="
sed -n '1,260p' src/Knarr.Service/ContainerCliProviderBase.cs | cat -n
echo "== Search timestamp/usages =="
rg -n "FormatTimestamp|--since|--until|DateTimeOffset|Log" src/Knarr.Service src -g '*.cs' || true
echo "== Tests around timestamp/filtering =="
fd -i 'test|container|cli|log' . -t f -d 5 | rg '\.(cs)$' | head -50Repository: hard-rox/knarr
Length of output: 42529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ContainerLogsTests =="
sed -n '1,260p' tests/Knarr.Service.Tests/ContainerLogsCommandTests.cs | cat -n
echo "== Service model files =="
cat -n src/Knarr.Service/Models/ContainerLogsOptions.csRepository: hard-rox/knarr
Length of output: 4741
🌐 Web query:
Apple container logs --since --until timestamp format fractional seconds
💡 Result:
The support for fractional seconds and timestamp formats when filtering container logs depends on the container engine being used (e.g., Docker, Podman, or Kubernetes). Docker and Podman For both Docker and Podman, the --since and --until options accept several formats, including support for nanosecond precision [1][2][3]. Supported formats include: - RFC3339 and RFC3339Nano (e.g., 2014-09-16T06:17:46.000000000Z) [1][4][3] - Specific date formats: 2006-01-02T15:04:05, 2006-01-02T15:04:05.999999999, 2006-01-02Z07:00, and 2006-01-02 [1][2][3] - Unix timestamps in the format seconds[.nanoseconds], where the fractional part can have up to nine digits [1][5][3] - Go duration strings (e.g., 10m, 1h30s) [1][2][3] Kubernetes (kubectl) Kubernetes uses the --since-time flag for filtering logs by an exact timestamp [6][7]. This flag requires the timestamp to be in RFC3339 format [6][7]. While the underlying parser for RFC3339 typically supports fractional seconds, the documentation emphasizes the use of RFC3339 specifically [6][7]. Unlike Docker and Podman, kubectl does not natively support providing a raw Unix timestamp with fractional seconds for the --since-time flag [6][8][7]. Instead, for relative time windows, it uses the --since flag, which accepts duration strings (e.g., 5s, 2m, 3h) [6][8][7].
Citations:
- 1: https://docs.docker.com/reference/cli/docker/container/logs/
- 2: https://manpages.debian.org/unstable/podman-docker/docker-logs.1.en.html
- 3: https://docs.podman.io/en/stable/markdown/podman-logs.1.html
- 4: https://github.com/microsoft/docker/blob/85716f95/docs/reference/commandline/logs.md
- 5: https://github.com/microsoft/docker/blob/85716f95/man/docker-logs.1.md
- 6: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_logs/
- 7: https://komodor.com/learn/kubectl-logs/
- 8: https://man.archlinux.org/man/kubectl-logs.1.en
🌐 Web query:
Apple container logs since until RFC3339 format
💡 Result:
The native Apple macOS log command does not use RFC3339 for its --start and --end time filtering [1][2]. Instead, it supports the following specific date/time formats [1][3]: - YYYY-MM-DD - YYYY-MM-DD HH:MM:SS - YYYY-MM-DD HH:MM:SSZZZZZ You can use these formats with the log show command, for example: log show --start "2026-07-29 09:00:00" --end "2026-07-29 10:00:00" [1][2] If you are working with container runtimes on macOS (such as Podman or Kubernetes-based tools), those specific tools often support RFC3339 for their own log filtering flags (e.g., --since or --since-time) [4][5][6], but this is a feature of the container engine's CLI, not the underlying macOS system log utility [1][3].
Citations:
- 1: https://manp.gs/mac/1/log
- 2: https://ss64.com/mac/log.html
- 3: https://www.iru.com/blog/mac-logging-and-the-log-command-a-guide-for-apple-admins?hs_amp=true
- 4: https://docs.podman.io/en/stable/markdown/podman-logs.1.html
- 5: https://man.archlinux.org/man/podman-logs.1.en
- 6: Fix issue of kubectl logs with flag --since-time kubernetes/kubernetes#78071
Keep fractional seconds on container log timestamps.
ContainerLogsOptions.Since/Until are DateTimeOffset?, but FormatTimestamp drops the fractional ticks before passing values to --since/--until, so boundaries like 10:30:00.500Z become 10:30:00Z. Use a format that preserves fractional seconds and cover this in the log args test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.Service/ContainerCliProviderBase.cs` around lines 208 - 209, Update
FormatTimestamp to use a round-trip timestamp format that preserves fractional
seconds while retaining UTC and invariant-culture output for --since/--until
values. Extend the existing log-arguments test to verify fractional
DateTimeOffset boundaries are emitted with their fractional precision.
…ined refresh handling and reliability
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Knarr.App/Features/Sidebar/SidebarViewModel.cs`:
- Around line 15-38: Make SidebarViewModel implement IDisposable and add Dispose
to release _badgeSubscription. Ensure the subscription created by
InitializeAsync is disposed and cleared, matching the cleanup behavior of
ImagesViewModel and ContainersViewModel.
In `@src/Knarr.App/Services/AutoRefreshService.cs`:
- Line 43: Remove the leftover Console.WriteLine debug statements, including the
“Subscribing” call and the corresponding call near the second affected location,
from AutoRefreshService methods. Use the existing _logger only where structured
logging is needed, without adding replacement console output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec537c5a-47b9-42c7-b82b-498614847c5c
📒 Files selected for processing (12)
README.mddocs/RELEASING.mdsrc/Knarr.App/Features/Containers/ContainersViewModel.cssrc/Knarr.App/Features/Images/ImagesView.axamlsrc/Knarr.App/Features/Images/ImagesViewModel.cssrc/Knarr.App/Features/Images/PullImageDialog.axamlsrc/Knarr.App/Features/Shell/MainWindow.axaml.cssrc/Knarr.App/Features/Sidebar/SidebarViewModel.cssrc/Knarr.App/ServiceCollectionExtensions.cssrc/Knarr.App/Services/AutoRefreshService.cssrc/Knarr.App/Services/IAutoRefreshService.cstests/Knarr.App.Tests/Services/AutoRefreshServiceTests.cs
… adjust using directives for consistency
…ved separation of concerns
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Knarr.App/Common/DialogWindow.cs (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking
DialogWindowabstract.
DialogWindowis designed only to be inherited (constructor isprotected), matching the "shared abstract base class" pattern that justifies the///XML doc above it. Marking the classabstractmakes that intent explicit in the type signature, not only in the constructor accessibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.App/Common/DialogWindow.cs` at line 9, Mark the DialogWindow class abstract to explicitly enforce its role as an inheritance-only base class, while preserving its existing protected constructor and Window inheritance.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Knarr.App/Common/DialogWindow.cs`:
- Around line 33-39: Update DialogWindow.OnClosed to notify the associated view
model when the window closes through any path, not just VM-initiated closure.
Extend IDialogViewModel with an appropriate close/disposal contract or detect
IDisposable, invoke it before detaching handlers, and ensure
ContainerLogsDialogViewModel and other dialog view models with cancellable work
release resources by calling RequestCancellation() from Dispose().
In `@src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cs`:
- Around line 10-17: Move the copy behavior out of
RunContainerDialog.OnCopyCommand, leaving the code-behind limited to view
initialization. Add an ICommandProvider-based CopyCommand binding in the dialog
AXAML, and have the view-model command delegate clipboard writes to a
framework-agnostic clipboard service rather than GetTopLevel(this).Clipboard,
preserving the existing non-empty CommandPreview guard and unit-testability.
---
Nitpick comments:
In `@src/Knarr.App/Common/DialogWindow.cs`:
- Line 9: Mark the DialogWindow class abstract to explicitly enforce its role as
an inheritance-only base class, while preserving its existing protected
constructor and Window inheritance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a5a40e7c-cb00-4621-ab0a-b0c4fffc53b5
📒 Files selected for processing (20)
src/Knarr.App/Common/DialogWindow.cssrc/Knarr.App/Common/IDialogViewModel.cssrc/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axamlsrc/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml.cssrc/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cssrc/Knarr.App/Features/Containers/ContainersView.axaml.cssrc/Knarr.App/Features/Containers/ContainersViewModel.cssrc/Knarr.App/Features/Images/ImagesView.axaml.cssrc/Knarr.App/Features/Images/ImagesViewModel.cssrc/Knarr.App/Features/Images/PullImageDialog.axamlsrc/Knarr.App/Features/Images/PullImageDialog.axaml.cssrc/Knarr.App/Features/Images/PullImageDialogViewModel.cssrc/Knarr.App/Features/RunContainer/RunContainerDialog.axamlsrc/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cssrc/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cssrc/Knarr.App/ServiceCollectionExtensions.cssrc/Knarr.App/Services/DialogService.cssrc/Knarr.App/Services/IDialogService.cstests/Knarr.App.Tests/Features/Containers/ContainersViewModelTests.cstests/Knarr.App.Tests/Features/Images/ImagesViewModelTests.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/Knarr.App.Tests/Features/Containers/ContainersViewModelTests.cs
- src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml
- src/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cs
- src/Knarr.App/Features/Images/PullImageDialogViewModel.cs
- src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cs
| private void OnClosed(object? sender, EventArgs e) | ||
| { | ||
| DetachViewModel(); | ||
|
|
||
| DataContextChanged -= OnDataContextChanged; | ||
| Closed -= OnClosed; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Notify the view model on every close path, not only the VM-initiated one.
OnClosed runs for both the title-bar close and the VM's Close() command, because Avalonia raises the same Closed event either way. Only the VM-initiated path (Close() → RequestCancellation() → CloseRequested) performs cleanup. OnClosed here only detaches handlers; it never tells the view model that the window is gone.
For ContainerLogsDialogViewModel, closing the window through the title bar bypasses RequestCancellation(), so an active log-follow stream can keep running after the window disappears. This generalizes the earlier feedback on ContainerLogsDialog.axaml.cs's removed OnClosed override: the responsibility now lives here, and it affects every DialogWindow-derived dialog.
Extend IDialogViewModel (or check for IDisposable) so OnClosed can release view-model resources regardless of which path triggered the close.
🔧 Proposed fix
private void OnClosed(object? sender, EventArgs e)
{
+ (_viewModel as IDisposable)?.Dispose();
DetachViewModel();
DataContextChanged -= OnDataContextChanged;
Closed -= OnClosed;
} public interface IDialogViewModel
{
event EventHandler? CloseRequested;
}Have ContainerLogsDialogViewModel (and other dialog view models with cancellable work) implement IDisposable and call RequestCancellation() from Dispose().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/Common/DialogWindow.cs` around lines 33 - 39, Update
DialogWindow.OnClosed to notify the associated view model when the window closes
through any path, not just VM-initiated closure. Extend IDialogViewModel with an
appropriate close/disposal contract or detect IDisposable, invoke it before
detaching handlers, and ensure ContainerLogsDialogViewModel and other dialog
view models with cancellable work release resources by calling
RequestCancellation() from Dispose().
| private async void OnCopyCommand(object? sender, RoutedEventArgs e) | ||
| { | ||
| if (_viewModel is { CommandPreview: { Length: > 0 } command } && | ||
| TopLevel.GetTopLevel(this)?.Clipboard is { } clipboard) | ||
| if (DataContext is RunContainerDialogViewModel { CommandPreview: { Length: > 0 } command } && | ||
| GetTopLevel(this)?.Clipboard is { } clipboard) | ||
| { | ||
| await clipboard.SetTextAsync(command); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/Knarr.App --items all --type class,interface --match 'Clipboard|Dialog|Service'
rg -n -C 4 \
'Clipboard|SetTextAsync|OnCopyCommand|OnCopyImageName|CopyCommand' \
src/Knarr.AppRepository: hard-rox/knarr
Length of output: 7181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- RunContainer files ---\n'
for f in \
src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cs \
src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml \
src/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cs
do
if [ -f "$f" ]; then
echo "### $f"
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
printf '\n--- Service registrations ---\n'
sed -n '1,240p' src/Knarr.App/ServiceCollectionExtensions.cs
printf '\n--- Related services around UI clipboard candidates ---\n'
rg -n -C 3 'interface|abstract class|class .*Service|SetTextAs|Clipboard|GetTopLevel|TopLevel.Get' \
src/Knarr.App/Services src/Knarr.App/Common src/Knarr.App/Features src/Knarr.App/ControlsRepository: hard-rox/knarr
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RunContainer files ---'
for f in \
src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cs \
src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml \
src/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cs
do
if [ -f "$f" ]; then
echo "### $f"
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
printf '%s\n' ''
printf '%s\n' '--- Service registrations ---'
sed -n '1,240p' src/Knarr.App/ServiceCollectionExtensions.cs
printf '%s\n' ''
printf '%s\n' '--- Related services around UI clipboard candidates ---'
rg -n -C 3 'interface|abstract class|class .*Service|SetTextAs|Clipboard|GetTopLevel|TopLevel.Get' \
src/Knarr.App/Services src/Knarr.App/Common src/Knarr.App/Features src/Knarr.App/ControlsRepository: hard-rox/knarr
Length of output: 28065
Move clipboard handling out of code-behind.
RunContainerDialog.OnCopyCommand contains user-action logic instead of only InitializeComponent(). Add ICommandProvider/CopyCommand binding from AXAML and move the Avalonia clipboard call into a framework-agnostic clipboard service so the view model remains UI-frame work-agnostic and unit-testable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml.cs` around lines
10 - 17, Move the copy behavior out of RunContainerDialog.OnCopyCommand, leaving
the code-behind limited to view initialization. Add an ICommandProvider-based
CopyCommand binding in the dialog AXAML, and have the view-model command
delegate clipboard writes to a framework-agnostic clipboard service rather than
GetTopLevel(this).Clipboard, preserving the existing non-empty CommandPreview
guard and unit-testability.
Source: Coding guidelines
- Introduced `SupportsBootLogs`, `SupportsLogTimestamps`, and `SupportsLogTimeRange` to `IContainerCliProvider` for CLI feature compatibility. - Updated `ContainerLogsDialogViewModel` and dialog to include boot log option. - Adjusted command builders to conditionally include new flags. - Enhanced tests to verify CLI compatibility and command generation.
…nce `ContainerLogsDialogViewModel` - Refactored XAML resource dictionaries to improve readability and indentation consistency. - Simplified `App` class by removing `partial`. - Improved logging and property initialization in `ContainerLogsDialogViewModel`, consolidating constructors and refining error handling. - Combined similar CLI output cases in `CliOutputKindToBrushConverter`.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Knarr.App/Features/Containers/ContainersViewModel.cs (1)
151-152: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the comment on the private method.
Lines 151-152 document private
LoadAsync. The C# guideline prohibits comments and XML documentation on private or internal members.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.App/Features/Containers/ContainersViewModel.cs` around lines 151 - 152, Remove the two-line comment immediately preceding the private LoadAsync method in ContainersViewModel, leaving the method implementation unchanged.Source: Coding guidelines
🧹 Nitpick comments (2)
src/Knarr.App/App.axaml (1)
7-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse two-space indentation throughout the changed AXAML.
The same indentation violation appears in all six AXAML files. Reformat the changed ranges with two-space indentation.
src/Knarr.App/App.axaml#L7-L27: reindent the application resources and style includes.src/Knarr.App/Controls/Pill.axaml#L4-L45: reindent the preview, template, setters, and status styles.src/Knarr.App/Controls/Tile.axaml#L4-L37: reindent the preview, template, setters, and status styles.src/Knarr.App/Features/Shell/MainWindow.axaml#L20-L20: reindent the design data element.src/Knarr.App/Features/Shell/MainWindow.axaml#L42-L48: reindent the theme menu items.src/Knarr.App/Features/Shell/MainWindow.axaml#L68-L73: reindent the menu bar and sidebar elements.src/Knarr.App/Features/Shell/MainWindow.axaml#L84-L89: reindent the page title and transition elements.src/Knarr.App/Features/Shell/MainWindow.axaml#L98-L98: reindent the fallback visibility binding.src/Knarr.App/Themes/Styles.axaml#L2-L112: reindent the styles and selectors.src/Knarr.App/Themes/Glass.axaml#L4-L74: reindent the theme dictionaries and palette.src/Knarr.App/Themes/Glass.axaml#L86-L122: reindent the table resources and theme.As per coding guidelines,
**/*.axamlfiles must use 2-space indentation for AXAML/XML.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.App/App.axaml` around lines 7 - 27, Reindent all specified changed AXAML ranges using consistent two-space indentation, preserving the existing elements and attributes. Apply this to src/Knarr.App/App.axaml (7-27), src/Knarr.App/Controls/Pill.axaml (4-45), src/Knarr.App/Controls/Tile.axaml (4-37), the listed ranges in src/Knarr.App/Features/Shell/MainWindow.axaml (20, 42-48, 68-73, 84-89, 98), src/Knarr.App/Themes/Styles.axaml (2-112), and src/Knarr.App/Themes/Glass.axaml (4-74, 86-122).Source: Coding guidelines
src/Knarr.App/Features/Sidebar/SidebarViewModel.cs (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
services1to a meaningful name.Use
servicesdirectly or rename the local variable toserviceProvider.services1does not identify the dependency.Proposed rename
-IServiceProvider services1 = services; ... -() => services1.GetRequiredService<ContainersViewModel>() +() => services.GetRequiredService<ContainersViewModel>()Apply the same rename to the other navigation factories.
As per coding guidelines: “Keep names self-explanatory.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Knarr.App/Features/Sidebar/SidebarViewModel.cs` at line 34, Rename the local variable services1 in the SidebarViewModel navigation-factory setup to serviceProvider, or use the existing services parameter directly; apply the same meaningful naming consistently across the other navigation factories.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Knarr.App/App.axaml.cs`:
- Line 9: Restore the partial modifier on the App class declaration in
App.axaml.cs so it matches the XAML-generated companion class for
x:Class="Knarr.App.App" and allows Avalonia generated code to compile.
In `@src/Knarr.App/Features/Sidebar/SidebarViewModel.cs`:
- Around line 79-83: Apply the formatter’s attribute layout in both affected
files: in src/Knarr.App/Features/Sidebar/SidebarViewModel.cs lines 79-83, place
each attribute for _cliName and _cliVersion on its own four-space-indented line;
in src/Knarr.App/Models/NavigationItem.cs line 14, split the attributes for
_badge the same way.
- Around line 38-55: Fix the parameterless SidebarViewModel fallback so it no
longer passes a null service provider into the constructor or page factories.
Update SidebarViewModel() and the related MainWindowViewModel() fallback to
create or reuse a valid service graph, including the dependencies required by
InitializeAsync and each GetRequiredService-based NavigationItem factory;
alternatively remove or replace those parameterless constructors while
preserving page creation.
---
Outside diff comments:
In `@src/Knarr.App/Features/Containers/ContainersViewModel.cs`:
- Around line 151-152: Remove the two-line comment immediately preceding the
private LoadAsync method in ContainersViewModel, leaving the method
implementation unchanged.
---
Nitpick comments:
In `@src/Knarr.App/App.axaml`:
- Around line 7-27: Reindent all specified changed AXAML ranges using consistent
two-space indentation, preserving the existing elements and attributes. Apply
this to src/Knarr.App/App.axaml (7-27), src/Knarr.App/Controls/Pill.axaml
(4-45), src/Knarr.App/Controls/Tile.axaml (4-37), the listed ranges in
src/Knarr.App/Features/Shell/MainWindow.axaml (20, 42-48, 68-73, 84-89, 98),
src/Knarr.App/Themes/Styles.axaml (2-112), and src/Knarr.App/Themes/Glass.axaml
(4-74, 86-122).
In `@src/Knarr.App/Features/Sidebar/SidebarViewModel.cs`:
- Line 34: Rename the local variable services1 in the SidebarViewModel
navigation-factory setup to serviceProvider, or use the existing services
parameter directly; apply the same meaningful naming consistently across the
other navigation factories.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2376d047-f91c-447d-9698-a060f7a78e6c
📒 Files selected for processing (45)
.idea/.idea.Knarr/.idea/avalonia.xmlDirectory.Packages.propssrc/Knarr.App/App.axamlsrc/Knarr.App/App.axaml.cssrc/Knarr.App/Common/IDialogViewModel.cssrc/Knarr.App/Controls/Pill.axamlsrc/Knarr.App/Controls/TerminalOutputView.axamlsrc/Knarr.App/Controls/Tile.axamlsrc/Knarr.App/Converters/CliOutputKindToBrushConverter.cssrc/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axamlsrc/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cssrc/Knarr.App/Features/Containers/ContainersView.axamlsrc/Knarr.App/Features/Containers/ContainersViewModel.cssrc/Knarr.App/Features/Dashboard/DashboardView.axamlsrc/Knarr.App/Features/Dashboard/DashboardViewModel.cssrc/Knarr.App/Features/Images/ImagesViewModel.cssrc/Knarr.App/Features/Images/PullImageDialog.axamlsrc/Knarr.App/Features/Images/PullImageDialogViewModel.cssrc/Knarr.App/Features/RunContainer/EnvironmentVariableEntry.cssrc/Knarr.App/Features/RunContainer/PortMappingEntry.cssrc/Knarr.App/Features/RunContainer/RunContainerDialog.axamlsrc/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cssrc/Knarr.App/Features/RunContainer/VolumeMountEntry.cssrc/Knarr.App/Features/Settings/SettingsView.axamlsrc/Knarr.App/Features/Settings/SettingsViewModel.cssrc/Knarr.App/Features/Shell/MainWindow.axamlsrc/Knarr.App/Features/Shell/MainWindowViewModel.cssrc/Knarr.App/Features/Sidebar/Sidebar.axamlsrc/Knarr.App/Features/Sidebar/SidebarViewModel.cssrc/Knarr.App/Models/ContainerItem.cssrc/Knarr.App/Models/ImageItem.cssrc/Knarr.App/Models/NavigationItem.cssrc/Knarr.App/Program.cssrc/Knarr.App/Services/AutoRefreshService.cssrc/Knarr.App/Services/DialogService.cssrc/Knarr.App/Services/IDialogService.cssrc/Knarr.App/Themes/Glass.axamlsrc/Knarr.App/Themes/Icons.axamlsrc/Knarr.App/Themes/Styles.axamlsrc/Knarr.Service/AppleContainerCli/AppleContainerCliProvider.cssrc/Knarr.Service/ContainerCliProviderBase.cssrc/Knarr.Service/IContainerCliProvider.cssrc/Knarr.Service/Models/ContainerLogsOptions.cstests/Knarr.App.Tests/Features/RunContainer/RunContainerDialogViewModelTests.cstests/Knarr.Service.Tests/ContainerLogsCommandTests.cs
💤 Files with no reviewable changes (1)
- src/Knarr.App/Themes/Icons.axaml
🚧 Files skipped from review as they are similar to previous changes (29)
- Directory.Packages.props
- src/Knarr.Service/Models/ContainerLogsOptions.cs
- src/Knarr.App/Services/IDialogService.cs
- src/Knarr.App/Common/IDialogViewModel.cs
- src/Knarr.App/Converters/CliOutputKindToBrushConverter.cs
- src/Knarr.App/Features/RunContainer/EnvironmentVariableEntry.cs
- src/Knarr.App/Services/DialogService.cs
- tests/Knarr.App.Tests/Features/RunContainer/RunContainerDialogViewModelTests.cs
- src/Knarr.App/Features/Dashboard/DashboardView.axaml
- src/Knarr.App/Features/Images/PullImageDialogViewModel.cs
- src/Knarr.App/Features/RunContainer/VolumeMountEntry.cs
- src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialog.axaml
- src/Knarr.App/Features/Images/PullImageDialog.axaml
- src/Knarr.App/Features/RunContainer/PortMappingEntry.cs
- src/Knarr.App/Features/Settings/SettingsView.axaml
- src/Knarr.App/Models/ImageItem.cs
- src/Knarr.Service/AppleContainerCli/AppleContainerCliProvider.cs
- src/Knarr.App/Features/Shell/MainWindowViewModel.cs
- src/Knarr.App/Features/Containers/ContainersView.axaml
- src/Knarr.App/Program.cs
- src/Knarr.App/Features/RunContainer/RunContainerDialogViewModel.cs
- src/Knarr.App/Features/Containers/ContainerLogs/ContainerLogsDialogViewModel.cs
- src/Knarr.App/Models/ContainerItem.cs
- src/Knarr.App/Controls/TerminalOutputView.axaml
- src/Knarr.App/Services/AutoRefreshService.cs
- src/Knarr.App/Features/RunContainer/RunContainerDialog.axaml
- src/Knarr.App/Features/Images/ImagesViewModel.cs
- src/Knarr.Service/ContainerCliProviderBase.cs
- src/Knarr.App/Features/Sidebar/Sidebar.axaml
| namespace Knarr.App; | ||
|
|
||
| public partial class App : Application | ||
| public class App : Application |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if ! rg -q 'x:Class="Knarr\.App\.App"' src/Knarr.App/App.axaml; then
echo "Expected App.axaml x:Class declaration was not found." >&2
exit 1
fi
if ! rg -q 'public\s+partial\s+class\s+App\s*:\s*Application' src/Knarr.App/App.axaml.cs; then
echo "App.axaml.cs must declare App as partial." >&2
exit 1
fiRepository: hard-rox/knarr
Length of output: 194
Restore partial on App.
src/Knarr.App/App.axaml declares x:Class="Knarr.App.App". Avalonia requires that XAML code-target class to remain partial so generated companion code can compile with App.axaml.cs.
Proposed fix
-public class App : Application
+public partial class App : Application📝 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.
| public class App : Application | |
| public partial class App : Application |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/App.axaml.cs` at line 9, Restore the partial modifier on the
App class declaration in App.axaml.cs so it matches the XAML-generated companion
class for x:Class="Knarr.App.App" and allows Avalonia generated code to compile.
| _systemService = services.GetService<IContainerSystemService>(); | ||
|
|
||
| _containersItem = new NavigationItem( | ||
| "Containers", "CubeRegular", createPage: () => _services.GetRequiredService<ContainersViewModel>()); | ||
| "Containers", "CubeRegular", createPage: () => services1.GetRequiredService<ContainersViewModel>()); | ||
| _imagesItem = new NavigationItem( | ||
| "Images", "CloudRegular", createPage: () => _services.GetRequiredService<ImagesViewModel>()); | ||
| "Images", "CloudRegular", createPage: () => services1.GetRequiredService<ImagesViewModel>()); | ||
|
|
||
| NavigationItems = | ||
| [ | ||
| new NavigationItem("Dashboard", "BoardRegular", createPage: () => _services.GetRequiredService<DashboardViewModel>()), | ||
| new NavigationItem("Dashboard", "BoardRegular", | ||
| createPage: () => services1.GetRequiredService<DashboardViewModel>()), | ||
| _containersItem, | ||
| _imagesItem, | ||
| new NavigationItem("Networks", "GlobeRegular", "3"), | ||
| new NavigationItem("Volumes", "StorageRegular", "5"), | ||
| new NavigationItem("Registries", "LibraryRegular"), | ||
| new NavigationItem("Settings", "SettingsRegular", createPage: () => _services.GetRequiredService<SettingsViewModel>()), | ||
| new NavigationItem("Settings", "SettingsRegular", | ||
| createPage: () => services1.GetRequiredService<SettingsViewModel>()), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Fix the null service-provider path used by the fallback constructors.
SidebarViewModel() passes null! for services, but this constructor calls services.GetService<IContainerSystemService>() and creates factories that call services1.GetRequiredService(...). MainWindowViewModel() uses that fallback and invokes the selected page factory, so the fallback path throws NullReferenceException. InitializeAsync also has a null _cliProvider.
Provide a real service graph for the parameterless constructors, or remove or replace those constructors. A null check around _systemService alone will not fix the page factories.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/Features/Sidebar/SidebarViewModel.cs` around lines 38 - 55, Fix
the parameterless SidebarViewModel fallback so it no longer passes a null
service provider into the constructor or page factories. Update
SidebarViewModel() and the related MainWindowViewModel() fallback to create or
reuse a valid service graph, including the dependencies required by
InitializeAsync and each GetRequiredService-based NavigationItem factory;
alternatively remove or replace those parameterless constructors while
preserving page creation.
| [ObservableProperty] [NotifyPropertyChangedFor(nameof(CliDisplay))] | ||
| private string _cliName = "wslc"; | ||
|
|
||
| [ObservableProperty] | ||
| [NotifyPropertyChangedFor(nameof(CliDisplay))] | ||
| [ObservableProperty] [NotifyPropertyChangedFor(nameof(CliDisplay))] | ||
| private string _cliVersion = "detecting\u2026"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply the formatter’s attribute layout in both files.
The same inline attribute pattern triggers the CI whitespace check. Split each attribute into its own four-space-indented line.
src/Knarr.App/Features/Sidebar/SidebarViewModel.cs#L79-L83: split the attributes for_cliNameand_cliVersion.src/Knarr.App/Models/NavigationItem.cs#L14-L14: split the attributes for_badge.
🧰 Tools
🪛 GitHub Actions: CI / 0_Lint, Build & Test (ubuntu-latest).txt
[error] 79-79: dotnet format --verify-no-changes failed: whitespace formatting error. Replace 1 character with a CRLF newline followed by four spaces.
🪛 GitHub Actions: CI / Lint, Build & Test (ubuntu-latest)
[error] 79-79: dotnet format verification failed: WHITESPACE formatting issue. Replace 1 character with a newline and indentation.
🪛 GitHub Check: Lint, Build & Test (ubuntu-latest)
[failure] 82-82:
Fix whitespace formatting. Replace 1 characters with '\r\n\s\s\s\s'.
[failure] 79-79:
Fix whitespace formatting. Replace 1 characters with '\r\n\s\s\s\s'.
📍 Affects 2 files
src/Knarr.App/Features/Sidebar/SidebarViewModel.cs#L79-L83(this comment)src/Knarr.App/Models/NavigationItem.cs#L14-L14
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Knarr.App/Features/Sidebar/SidebarViewModel.cs` around lines 79 - 83,
Apply the formatter’s attribute layout in both affected files: in
src/Knarr.App/Features/Sidebar/SidebarViewModel.cs lines 79-83, place each
attribute for _cliName and _cliVersion on its own four-space-indented line; in
src/Knarr.App/Models/NavigationItem.cs line 14, split the attributes for _badge
the same way.
Source: Linters/SAST tools
…matting consistency - Changed `end_of_line` to `lf` in `.editorconfig`. - Adjusted attribute formatting in `SidebarViewModel`, `NavigationItem`, and other classes for clarity. - Renamed constant `Executable` to `_executable` in `AppleContainerSystemService` for naming consistency.
1 participant
Summary by CodeRabbit