Refresh container management pages and resource views - #1260
Merged
Conversation
Split the container page into four tabs (Containers, Images, Prune, Settings) using a TabBar/TabBarView layout like the settings page. Fine-tune each tab's display: show live count summaries in list headers, turn prune/settings into header cards, show FAB only on the containers tab. Add dockerPruneTip l10n for all languages.
Contributor
📝 WalkthroughWalkthroughThe container page now separates containers, images, and settings into tabs. Refreshes select a resource target and execute only its required commands. Image and system pruning support selectable scopes and command previews. Image usage matching supports IDs, tags, digests, aliases, and unknown counts. Podman parsing retains detailed status text and network-statistic fallbacks. New views display grouped containers, image metadata, metrics, logs, and responsive layouts. Tests and localization strings cover the updated flows. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (7)
lib/view/page/container/resource_views.dart (4)
304-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd selection semantics to the prune scope tiles.
_PruneScopeTilerenders a radio appearance with a plainIcon. Screen readers do not receive the selected state. Setselected: selectedon theListTile, or wrap the tile inSemantics(inMutuallyExclusiveGroup: true, selected: selected), so assistive technology announces which prune scope is active.♿ Proposed change
return ListTile( contentPadding: EdgeInsets.zero, + selected: selected, leading: Icon(🤖 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 `@lib/view/page/container/resource_views.dart` around lines 304 - 332, Update _PruneScopeTile to expose its selected state to assistive technology by setting selected: selected on the ListTile, while preserving the existing radio icon and tap behavior.
168-181: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider lazy building for large image lists.
The
Columnbuilds every_ContainerImageRoweagerly, and each row creates its ownLayoutBuilder. On hosts with hundreds of images this increases build and layout cost on every rebuild. If large image counts are expected, split the rows intoListViewslivers or wrap the rows in aSliverListso only visible rows build.🤖 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 `@lib/view/page/container/resource_views.dart` around lines 168 - 181, Update the image list rendering around the Column and _ContainerImageRow to use a lazy list structure such as ListView or SliverList, ensuring only visible rows are built while preserving trailingBuilder output and divider placement between items.
1158-1257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the util helpers into an
extension onblock.The file keeps the presentation utils as top-level private functions (
_runtimeIcon,_statusLabel,_imageReference,_imageCreatedLabel). The repository pattern separates Widget build, Actions, and Utils withextension on. Group these helpers in an extension on the owning view or model type to match the pattern.As per coding guidelines: "Split UI into Widget build, Actions, Utils using
extension onto achieve this pattern".🤖 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 `@lib/view/page/container/resource_views.dart` around lines 1158 - 1257, Move the presentation utility helpers from top-level private functions into an extension on the owning view or model type, grouping _runtimeIcon, _statusLabel, _imageReference, _imageCreatedLabel, and the related parsing helpers together. Preserve their existing behavior and update any call sites as needed to use the extension-based organization.Source: Coding guidelines
1259-1268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a locale-aware date format.
_formatUnixDatehardcodesyyyy-MM-dd. The app supports many locales, so the image creation date does not follow the user's locale conventions. Useintl'sDateFormat.yMd(context.locale...)(or the project's existing date helper) to format the date.🤖 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 `@lib/view/page/container/resource_views.dart` around lines 1259 - 1268, Update _formatUnixDate to format the converted local DateTime with the project’s locale-aware date helper or intl DateFormat.yMd using the active context locale, instead of manually constructing yyyy-MM-dd. Preserve the existing null and non-positive timestamp handling and return type.test/container_test.dart (1)
384-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a mixed known-and-unknown case.
The tests cover an all-known set (result
1) and an all-unknown set (resultnull). They do not cover a set that mixes a confirmed unused image with an unresolvable image. That case defines whethercountUnusedTaggedImagesreturnsnullor a partial count, which drives the "Unused tagged: Unknown" label inContainerImagePruneOptionsView. Add a test that pins this behavior.🤖 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 `@test/container_test.dart` around lines 384 - 395, Add a test near the existing countUnusedTaggedImages cases for a mixed input containing one confirmed unused image and one unresolvable image. Assert the intended return value for this combination, including the behavior consumed by ContainerImagePruneOptionsView’s “Unused tagged: Unknown” label.lib/data/model/container/ps.dart (1)
132-149: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider tolerating a single malformed line.
parsePodmanPsOutputdecodes each line insidemap. One malformed line throws and discards the whole container list. The Docker path inlib/data/provider/container.dart(Lines 364-370) catchesFormatExceptionper row and skips only that row. Align the Podman path with that behavior.♻️ Proposed per-line tolerance
List<PodmanPs> parsePodmanPsOutput(String raw) { - return raw - .split('\n') - .where((line) => line.trim().isNotEmpty) - .map((line) { - final separator = line.lastIndexOf('\t'); - final jsonPart = separator < 0 ? line : line.substring(0, separator); - final data = json.decode(jsonPart) as Map<String, dynamic>; - if (separator >= 0) { - final detailedStatus = line.substring(separator + 1).trim(); - if (detailedStatus.isNotEmpty) { - data['ServerBoxStatus'] = detailedStatus; - } - } - return PodmanPs.fromJson(data); - }) - .toList(growable: false); + final items = <PodmanPs>[]; + for (final line in raw.split('\n')) { + if (line.trim().isEmpty) continue; + final separator = line.lastIndexOf('\t'); + final jsonPart = separator < 0 ? line : line.substring(0, separator); + try { + final data = json.decode(jsonPart) as Map<String, dynamic>; + if (separator >= 0) { + final detailedStatus = line.substring(separator + 1).trim(); + if (detailedStatus.isNotEmpty) { + data['ServerBoxStatus'] = detailedStatus; + } + } + items.add(PodmanPs.fromJson(data)); + } on FormatException { + continue; + } + } + return List.unmodifiable(items); }🤖 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 `@lib/data/model/container/ps.dart` around lines 132 - 149, Update parsePodmanPsOutput so each line’s JSON decoding and PodmanPs.fromJson conversion is isolated per row, catching FormatException and skipping only malformed lines. Preserve filtering of blank lines, ServerBoxStatus enrichment, and the existing fixed-size result list for successfully parsed containers, matching the Docker path’s per-row tolerance.lib/view/page/container/actions.dart (1)
127-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit cancel action to both prune dialogs. Both dialogs are destructive and build their actions with
Btn.ok(...).toList, so the only visible action confirms the prune. The coding guidelines requireBtnx.cancelOkfor this pattern.
lib/view/page/container/actions.dart#L127-L136: replaceBtn.ok(...).toListin_showImagePruneDialogwithBtnx.cancelOkand keep the red confirm styling.lib/view/page/container/actions.dart#L164-L176: replaceBtn.ok(...).toListin_showSystemPruneDialogwithBtnx.cancelOkand keep the red confirm styling.As per coding guidelines: "Use widgets and utilities from
fl_libpackage for common functionalities such asCustomAppBar,context.showRoundDialog,Input,Btnx.cancelOk, etc."🤖 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 `@lib/view/page/container/actions.dart` around lines 127 - 136, Update _showImagePruneDialog at lib/view/page/container/actions.dart:127-136 and _showSystemPruneDialog at lib/view/page/container/actions.dart:164-176 to use Btnx.cancelOk instead of Btn.ok(...).toList, preserving the existing asynchronous prune callbacks and red confirm styling while providing an explicit cancel action.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 `@lib/data/provider/container.dart`:
- Around line 531-540: Update pruneSystem to refresh both the container and
image lists after the system prune completes, replacing the null refreshTarget
with the established refresh mechanism and ensuring the second target is queued
or coalesced as supported by refresh.
- Line 92: Update the container provider switching flow around setType so
clearing the resource lists is followed by a _refreshContainerTab call for the
selected resource tab. Ensure the refresh starts immediately after the settings
provider changes, without requiring a later tab change.
In `@lib/l10n/app_fr.arb`:
- Around line 107-109: Update the French localization value for
pruneUnusedImagesTip to use grammatically correct ne … aucun negation, while
preserving the intended meaning that tagged images are not used by any
container.
In `@lib/l10n/app_tr.arb`:
- Around line 108-109: Update the Turkish translations for pruneUnusedImagesTip
and includeUnusedVolumesTip to replace “kapsayıcı” with “konteyner”, matching
the terminology used elsewhere in the localization file.
In `@lib/l10n/app_zh.arb`:
- Around line 119-127: Use the existing Chinese verb “清理” consistently for prune
actions: update pruneImages, pruneVolumes, and pruneUnusedData in
lib/l10n/app_zh.arb (lines 119-127) and lib/l10n/app_zh_tw.arb (lines 119-127),
replacing “修剪” without changing other translations.
In `@lib/view/page/container/actions.dart`:
- Around line 102-107: Update the unused-tagged-image count calculation around
countUnusedTaggedImages so a null _containerState.items remains null instead of
being replaced with an empty list. Preserve the mapped image references when
items are available, allowing the view to show unknown usage while containers
are still loading.
In `@lib/view/page/container/resource_views.dart`:
- Around line 704-710: Update both row key constructions in
lib/view/page/container/resource_views.dart at lines 704-710 and 1003-1008 to
include each row’s index in the KeyedSubtree ValueKey, alongside the existing
wide/compact mode and identifier values. Ensure the enclosing builders expose
the correct row index, so fallback identifiers like 'unknown' and '<none>'
remain unique among sibling rows.
---
Nitpick comments:
In `@lib/data/model/container/ps.dart`:
- Around line 132-149: Update parsePodmanPsOutput so each line’s JSON decoding
and PodmanPs.fromJson conversion is isolated per row, catching FormatException
and skipping only malformed lines. Preserve filtering of blank lines,
ServerBoxStatus enrichment, and the existing fixed-size result list for
successfully parsed containers, matching the Docker path’s per-row tolerance.
In `@lib/view/page/container/actions.dart`:
- Around line 127-136: Update _showImagePruneDialog at
lib/view/page/container/actions.dart:127-136 and _showSystemPruneDialog at
lib/view/page/container/actions.dart:164-176 to use Btnx.cancelOk instead of
Btn.ok(...).toList, preserving the existing asynchronous prune callbacks and red
confirm styling while providing an explicit cancel action.
In `@lib/view/page/container/resource_views.dart`:
- Around line 304-332: Update _PruneScopeTile to expose its selected state to
assistive technology by setting selected: selected on the ListTile, while
preserving the existing radio icon and tap behavior.
- Around line 168-181: Update the image list rendering around the Column and
_ContainerImageRow to use a lazy list structure such as ListView or SliverList,
ensuring only visible rows are built while preserving trailingBuilder output and
divider placement between items.
- Around line 1158-1257: Move the presentation utility helpers from top-level
private functions into an extension on the owning view or model type, grouping
_runtimeIcon, _statusLabel, _imageReference, _imageCreatedLabel, and the related
parsing helpers together. Preserve their existing behavior and update any call
sites as needed to use the extension-based organization.
- Around line 1259-1268: Update _formatUnixDate to format the converted local
DateTime with the project’s locale-aware date helper or intl DateFormat.yMd
using the active context locale, instead of manually constructing yyyy-MM-dd.
Preserve the existing null and non-positive timestamp handling and return type.
In `@test/container_test.dart`:
- Around line 384-395: Add a test near the existing countUnusedTaggedImages
cases for a mixed input containing one confirmed unused image and one
unresolvable image. Assert the intended return value for this combination,
including the behavior consumed by ContainerImagePruneOptionsView’s “Unused
tagged: Unknown” label.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 19a4d017-fc01-4e9d-b92e-d3c94e106ae3
⛔ Files ignored due to path filters (16)
lib/generated/l10n/l10n.dartis excluded by!**/generated/**lib/generated/l10n/l10n_de.dartis excluded by!**/generated/**lib/generated/l10n/l10n_en.dartis excluded by!**/generated/**lib/generated/l10n/l10n_es.dartis excluded by!**/generated/**lib/generated/l10n/l10n_fr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_id.dartis excluded by!**/generated/**lib/generated/l10n/l10n_it.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ja.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ko.dartis excluded by!**/generated/**lib/generated/l10n/l10n_nl.dartis excluded by!**/generated/**lib/generated/l10n/l10n_pt.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ru.dartis excluded by!**/generated/**lib/generated/l10n/l10n_tr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_uk.dartis excluded by!**/generated/**lib/generated/l10n/l10n_zh.dartis excluded by!**/generated/**pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
lib/data/model/container/image.dartlib/data/model/container/ps.dartlib/data/provider/container.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_id.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_ko.arblib/l10n/app_nl.arblib/l10n/app_pt.arblib/l10n/app_ru.arblib/l10n/app_tr.arblib/l10n/app_uk.arblib/l10n/app_zh.arblib/l10n/app_zh_tw.arblib/view/page/container/actions.dartlib/view/page/container/container.dartlib/view/page/container/resource_views.dartlib/view/page/container/types.dartlib/view/widget/percent_circle.darttest/container_resource_views_test.darttest/container_test.dart
![winnowl[bot]](https://avatars.githubusercontent.com/in/4312382?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
🔎 Confirmed findings (9)
- 🟡 Medium A single malformed Podman JSON process row aborts parsing of the entire container listing.
parsePodmanPsOutputcallsjson.decodeinside an unguardedmap, andContainerNotifier.refreshinvokes it without per-row recovery; for example, one truncated row after valid rows throws, leaving the refresh with no parseditemsinstead of retaining valid containers. (inline) - 🟡 Medium One malformed image JSON row prevents all valid images from being published. Both the JSON-array and line-delimited branches map
ContainerImg.fromRawJsonwithout isolating exceptions, so a malformed or non-object row causes the whole mapping to throw and the state remains without an image list; this violates the requirement that malformed rows cannot corrupt the listing. (inline) - 🟡 Medium Stats are associated by
contains(id.substring(0, 5))rather than by an exact parsed container ID, so two containers sharing the same first five characters can receive the first container's stats; this violates the intended container-ID association even though the remote stats command returns all containers. (inline) - 🟡 Medium The resource tab load/error rendering is shared across tabs and does not distinguish an error from a successful empty result, but more importantly provider parse failures can leave the prior resource list intact while setting
error: the page checksitems != null/images != nullfirst and renders the populated view even whencontainerState.erroris set. For example, after a successful image load followed by malformed image JSON,refresh()catches the parse exception and setserrorwithout clearingimages; the Images tab then shows stale populated data instead of the required error state. This would be false only if every parse failure is guaranteed to clear the corresponding list before state update, which the catch path does not do. (inline) - 🟡 Medium Destructive/resource actions are not gated while the container notifier is busy.
_buildPruneActionalways supplies a non-nullonPressed, and per-container/image popup menus are always built, while_buildResourceActionsonly disables refresh. During an in-flight command (whenisBusy/runLogis active), the user can confirm prune or start another action, causing overlapping commands and refreshes against the same provider instead of the required disabled/gated behavior. This would be false only if the dialog/menu framework globally rejects taps whilerun()orrefresh()is active, but these callbacks are directly exposed and no such guard exists here. (inline) - 🟡 Medium The new resource labels are not localized at runtime: every locale renders the metric headings as the English literals
CPU,MEM,NET, andDISK, so the localized explanatory/prune strings are mixed with untranslated labels. (inline) - 🟡 Medium The test named
missing or unparseable container stats are omitteddoes not exercise malformed non-empty network/disk values._extractMetricValuereturns any non-empty text when it cannot find a byte-size token, so values such asnet = 'not available'ordisk = 'garbage'are rendered as resource modules instead of omitted; the current fixture leaves both fields null and therefore cannot catch that regression. This is false only if arbitrary non-empty runtime stats are intentionally valid display values. (inline) - 🟡 Medium The unknown-usage matching tests do not cover reference boundaries and the implementation adds unqualified short-repository markers. An image
registry-a.example/team/api:latestcan be considered used by a running container referenceregistry-b.example/api:latest, because both produce the markerapi:latest; this makes the unused count incorrectly zero. The claim is false only if short repository names are guaranteed globally unique across all registries in the supported runtime output. (inline) - 🟡 Medium Unknown image usage can be falsely marked as confirmed when two registries share the same short repository name.
_imageMarkersadds only the final path component (api), and_addRuntimeImageReferenceadds the same short marker; consequently an unknown-count imageregistry.one/team/api:stableis considered used by a container referenceregistry.two/other/api:stable, causingcountUnusedTaggedImagesto return 0 instead of null/unknown (or count the image as unused). The current tests cover a matching short reference and an unrelated different name, but not this collision boundary. This is disproven if container image references are guaranteed never to come from distinct full repositories with the same final component. (inline)
📋 Additional findings from this change (not shown inline) (14)
- 🟠 High A remote execution exception can leave the notifier permanently busy and can leave an action log stuck instead of producing a ContainerErr. (lib/data/provider/container.dart) — anchor-outside-diff
- 🟠 High The add-container flow interpolates the user-provided name directly into the shell command instead of shell-quoting it. A name such as
safe; touch /tmp/pwned; #becomes command syntax in the preview/run path, so confirming the dialog can execute attacker-controlled host-shell commands (and a name containing spaces is also parsed incorrectly). This violates the quoting requirement for add-container actions; it would be false only if_buildAddCmdwere guaranteed to receive a pre-tokenized/safely escaped name, but it receivesnameCtrl.text.trim()directly. (lib/view/page/container/container.dart) — anchor-outside-diff - 🟠 High The runtime/host scope is preserved for merged Compose logs but not for per-container logs and terminal actions.
_openMergedLogswraps the command with the configured host environment, whereas_onTapMoreBtnconstructsdocker/podman logsandexecdirectly and passes it to SSH without_wrapContainerHost. With a configured remote Docker/Podman socket, clicking Logs or Terminal acts on the SSH host's default runtime instead of the selected configured host, violating configured runtime/host scope. This would be false only when no custom container host is configured. (lib/view/page/container/actions.dart) — anchor-outside-diff - 🟡 Medium Podman image listings produced by the runtime are parsed as dangling because
PodmanImg.fromJsononly reads lowercaserepositoryandtag, while the selectedpodman image ls --format "{{json .}}"output uses the template's capitalizedRepositoryandTagfields. A normal tagged image therefore gets null repository/tag,isDangling == true, and is omitted/misclassified by cleanup and usage UI. (lib/data/model/container/image.dart) — anchor-outside-diff - 🟡 Medium Podman lifecycle classification treats every container with
Exited == falseas running and never consults the Podman state/status text. A paused, restarting, or otherwise non-running Podman container can therefore be shown as running and be included in stop/restart actions (and excluded from stopped counts), violating the lifecycle mapping promised byContainerStatus. This is atPodmanPs.status, which delegates only toContainerStatus.fromPodmanExited(exited); the same object already retainsrawStatusfrom.Status/.State. The claim would be false if Podman guaranteed thatExited == falseoccurs only for actively running containers and never for paused/restarting/created states. (lib/data/model/container/ps.dart) — anchor-outside-diff - 🟡 Medium Unknown container states are classified and summarized as stopped. Both the page summary and Compose header compute stopped as
items.length - running/!e.status.isRunning, whileContainerStatus.unknown.isRunningis false. A Podman row with a missing/non-booleanExitedvalue (or an unrecognized Docker state) is therefore displayed in the stopped count and offered to group stop/restart/start logic as a stopped container, rather than preserving an unknown status. This would be false only if runtime rows can never have unknown state values, but the model explicitly supportsunknownfor null/unrecognized input. (lib/view/page/container/resource_views.dart) — anchor-unreliable - 🟡 Medium The Podman image fallback test does not verify the required
Namesfallback: it supplies onlyNames: ['docker.io/library/nginx:latest']and expectsimg.isDangling == true. Consequently, the test passes whilePodmanImg.fromJsonignoresNamesand leaves repository/tag null, so a regression that fails to parse the actual image reference is undetected. This is false only if Podman’s image JSON contract guarantees thatNamesis never the only image-reference field (or if dangling is the intended result for that fixture). (test/container_test.dart) — anchor-outside-diff - 🟡 Medium The volume cleanup dialog does not show the command that will execute, and therefore cannot keep a runtime-qualified preview synchronized with the selected runtime as required by the cleanup contract.
_buildPruneCardroutes volumes to_showPruneDialog, whose body is only a generic confirmation message, while image/system dialogs displaydocker/podmancommand previews; selecting Podman consequently gives no evidence thatpodman volume prune -f(rather than Docker or another command) will run. This is false only if volume cleanup is intentionally excluded from the command-preview requirement. (lib/view/page/container/container.dart) — per-file-budget - 🟡 Medium
execSelecteduses the literalSrvBoxSepas an unescaped record separator andrefreshblindly callsraw.split(ScriptConstants.separator). A Docker/Podman image name, container name, or status containing that string can add an extra split segment, causing the refresh to reportsegmentsNotMatchand discard the otherwise valid refresh. The current tests only assert that selected commands omit/include listings; they do not exercise separator-containing output. This would be disproven if the remote command/output contract guarantees thatSrvBoxSepcannot occur in any command output (including user-controlled image/container metadata). (lib/data/provider/container.dart) — inline-budget - 🟡 Medium System prune is reported as successful without refreshing either the container or image resource state. Because
pruneSystempassesrefreshTarget: null, removing containers/images (especially with-a) leaves both tabs displaying resources that no longer exist until the user manually refreshes them. (lib/data/provider/container.dart) — inline-budget - 🟡 Medium Podman lifecycle status is inferred solely from the
Exitedboolean, so a runtime row withExited: falseand raw statusPaused,Restarting,Created, orDeadis classified asContainerStatus.running. The model now exposes the raw status but does not use it for status mapping, causing non-running containers to be shown as running and potentially enabling running-only actions. (lib/data/model/container/status.dart) — inline-budget - 🟡 Medium A malformed stats payload for one container aborts stats parsing for every later container. The provider wraps the entire item loop in one try/catch, and
parseStatsperforms unchecked JSON/type operations (for example Dockerstats['NetIO'] as String?); if one row has a non-string or invalid payload, subsequent valid rows are never updated and the refresh reports a global parse error. (lib/data/provider/container.dart) — anchor-unreliable - 🔵 Low Usage matching treats any bare 12–64 hex string as an image ID before treating it as a repository reference. A legitimate runtime image reference whose repository/name is hex-only (for example
0123456789ab) can therefore match an unrelated image whose ID has that prefix, making an unknown-count tagged image appear used and understating the cleanup count. The normalization needs to distinguish an explicitly ID-shaped runtime value from ambiguous repository references (or require stronger evidence). (lib/data/model/container/image.dart) — inline-budget - 🔵 Low The volume cleanup dialog does not display the command it will execute (nor the selected runtime), while image and system cleanup dialogs display runtime-qualified previews. Selecting the Podman runtime therefore shows only a generic 'prune volumes' confirmation even though confirmation executes
podman volume prune -f; this leaves the displayed confirmation/preview unsynchronized with the actual Docker/Podman command and is not covered by the widget tests, which never exercise the volume card/dialog. This would be disproven if the product intentionally requires no command preview for volume cleanup or the generic confirmation is documented as sufficient for that action. (lib/view/page/container/actions.dart) — anchor-unreliable
❓ Low-evidence leads (not confirmed — verify before acting) (2)
- Successful system and volume prune operations never refresh any resource list because both pass refreshTarget: null, so the UI continues to show pre-prune images/containers (and the system-prune option can remove both) until an unrelated manual refresh. (lib/data/provider/container.dart)
- A malformed stats row aborts parsing stats for all remaining containers. The refresh loop calls
item.parseStats(statsLine, state.version)without isolating exceptions per matched item; malformed JSON or an unexpected stats shape for one container exits the loop into the single outer catch, so subsequent containers with valid stats are left unparsed. This would be false only if every matched stats line is guaranteed valid and shape-compatible. (lib/data/provider/container.dart)
🤖 Prompt for AI agents — all findings (23)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (9)
In lib/data/model/container/ps.dart around line 139, address this finding:
A single malformed Podman JSON process row aborts parsing of the entire container listing. `parsePodmanPsOutput` calls `json.decode` inside an unguarded `map`, and `ContainerNotifier.refresh` invokes it without per-row recovery; for example, one truncated row after valid rows throws, leaving the refresh with no parsed `items` instead of retaining valid containers.
In lib/data/provider/container.dart around line 432, address this finding:
One malformed image JSON row prevents all valid images from being published. Both the JSON-array and line-delimited branches map `ContainerImg.fromRawJson` without isolating exceptions, so a malformed or non-object row causes the whole mapping to throw and the state remains without an image list; this violates the requirement that malformed rows cannot corrupt the listing.
In lib/data/provider/container.dart around line 402, address this finding:
Stats are associated by `contains(id.substring(0, 5))` rather than by an exact parsed container ID, so two containers sharing the same first five characters can receive the first container's stats; this violates the intended container-ID association even though the remote stats command returns all containers.
In lib/view/page/container/container.dart around line 166, address this finding:
The resource tab load/error rendering is shared across tabs and does not distinguish an error from a successful empty result, but more importantly provider parse failures can leave the prior resource list intact while setting `error`: the page checks `items != null`/`images != null` first and renders the populated view even when `containerState.error` is set. For example, after a successful image load followed by malformed image JSON, `refresh()` catches the parse exception and sets `error` without clearing `images`; the Images tab then shows stale populated data instead of the required error state. This would be false only if every parse failure is guaranteed to clear the corresponding list before state update, which the catch path does not do.
In lib/view/page/container/container.dart around line 247, address this finding:
Destructive/resource actions are not gated while the container notifier is busy. `_buildPruneAction` always supplies a non-null `onPressed`, and per-container/image popup menus are always built, while `_buildResourceActions` only disables refresh. During an in-flight command (when `isBusy`/`runLog` is active), the user can confirm prune or start another action, causing overlapping commands and refreshes against the same provider instead of the required disabled/gated behavior. This would be false only if the dialog/menu framework globally rejects taps while `run()` or `refresh()` is active, but these callbacks are directly exposed and no such guard exists here.
In lib/view/page/container/resource_views.dart around line 808, address this finding:
The new resource labels are not localized at runtime: every locale renders the metric headings as the English literals `CPU`, `MEM`, `NET`, and `DISK`, so the localized explanatory/prune strings are mixed with untranslated labels.
In lib/view/page/container/resource_views.dart around line 1230, address this finding:
The test named `missing or unparseable container stats are omitted` does not exercise malformed non-empty network/disk values. `_extractMetricValue` returns any non-empty text when it cannot find a byte-size token, so values such as `net = 'not available'` or `disk = 'garbage'` are rendered as resource modules instead of omitted; the current fixture leaves both fields null and therefore cannot catch that regression. This is false only if arbitrary non-empty runtime stats are intentionally valid display values.
In lib/data/model/container/image.dart around line 73, address this finding:
The unknown-usage matching tests do not cover reference boundaries and the implementation adds unqualified short-repository markers. An image `registry-a.example/team/api:latest` can be considered used by a running container reference `registry-b.example/api:latest`, because both produce the marker `api:latest`; this makes the unused count incorrectly zero. The claim is false only if short repository names are guaranteed globally unique across all registries in the supported runtime output.
In lib/data/model/container/image.dart around line 68, address this finding:
Unknown image usage can be falsely marked as confirmed when two registries share the same short repository name. `_imageMarkers` adds only the final path component (`api`), and `_addRuntimeImageReference` adds the same short marker; consequently an unknown-count image `registry.one/team/api:stable` is considered used by a container reference `registry.two/other/api:stable`, causing `countUnusedTaggedImages` to return 0 instead of null/unknown (or count the image as unused). The current tests cover a matching short reference and an unrelated different name, but not this collision boundary. This is disproven if container image references are guaranteed never to come from distinct full repositories with the same final component.
## Additional findings on this change (not posted inline) (14)
In lib/data/provider/container.dart around line 244, address this finding:
A remote execution exception can leave the notifier permanently busy and can leave an action log stuck instead of producing a ContainerErr.
In lib/view/page/container/container.dart around line 383, address this finding:
The add-container flow interpolates the user-provided name directly into the shell command instead of shell-quoting it. A name such as `safe; touch /tmp/pwned; #` becomes command syntax in the preview/run path, so confirming the dialog can execute attacker-controlled host-shell commands (and a name containing spaces is also parsed incorrectly). This violates the quoting requirement for add-container actions; it would be false only if `_buildAddCmd` were guaranteed to receive a pre-tokenized/safely escaped name, but it receives `nameCtrl.text.trim()` directly.
In lib/view/page/container/actions.dart around line 347, address this finding:
The runtime/host scope is preserved for merged Compose logs but not for per-container logs and terminal actions. `_openMergedLogs` wraps the command with the configured host environment, whereas `_onTapMoreBtn` constructs `docker/podman logs` and `exec` directly and passes it to SSH without `_wrapContainerHost`. With a configured remote Docker/Podman socket, clicking Logs or Terminal acts on the SSH host's default runtime instead of the selected configured host, violating configured runtime/host scope. This would be false only when no custom container host is configured.
In lib/data/model/container/image.dart around line 162, address this finding:
Podman image listings produced by the runtime are parsed as dangling because `PodmanImg.fromJson` only reads lowercase `repository` and `tag`, while the selected `podman image ls --format "{{json .}}"` output uses the template's capitalized `Repository` and `Tag` fields. A normal tagged image therefore gets null repository/tag, `isDangling == true`, and is omitted/misclassified by cleanup and usage UI.
In lib/data/model/container/ps.dart around line 66, address this finding:
Podman lifecycle classification treats every container with `Exited == false` as running and never consults the Podman state/status text. A paused, restarting, or otherwise non-running Podman container can therefore be shown as running and be included in stop/restart actions (and excluded from stopped counts), violating the lifecycle mapping promised by `ContainerStatus`. This is at `PodmanPs.status`, which delegates only to `ContainerStatus.fromPodmanExited(exited)`; the same object already retains `rawStatus` from `.Status`/`.State`. The claim would be false if Podman guaranteed that `Exited == false` occurs only for actively running containers and never for paused/restarting/created states.
In lib/view/page/container/resource_views.dart, address this finding:
Unknown container states are classified and summarized as stopped. Both the page summary and Compose header compute stopped as `items.length - running` / `!e.status.isRunning`, while `ContainerStatus.unknown.isRunning` is false. A Podman row with a missing/non-boolean `Exited` value (or an unrecognized Docker state) is therefore displayed in the stopped count and offered to group stop/restart/start logic as a stopped container, rather than preserving an unknown status. This would be false only if runtime rows can never have unknown state values, but the model explicitly supports `unknown` for null/unrecognized input.
In test/container_test.dart around line 474, address this finding:
The Podman image fallback test does not verify the required `Names` fallback: it supplies only `Names: ['docker.io/library/nginx:latest']` and expects `img.isDangling == true`. Consequently, the test passes while `PodmanImg.fromJson` ignores `Names` and leaves repository/tag null, so a regression that fails to parse the actual image reference is undetected. This is false only if Podman’s image JSON contract guarantees that `Names` is never the only image-reference field (or if dangling is the intended result for that fixture).
In lib/view/page/container/container.dart around line 398, address this finding:
The volume cleanup dialog does not show the command that will execute, and therefore cannot keep a runtime-qualified preview synchronized with the selected runtime as required by the cleanup contract. `_buildPruneCard` routes volumes to `_showPruneDialog`, whose body is only a generic confirmation message, while image/system dialogs display `docker`/`podman` command previews; selecting Podman consequently gives no evidence that `podman volume prune -f` (rather than Docker or another command) will run. This is false only if volume cleanup is intentionally excluded from the command-preview requirement.
In lib/data/provider/container.dart around line 315, address this finding:
`execSelected` uses the literal `SrvBoxSep` as an unescaped record separator and `refresh` blindly calls `raw.split(ScriptConstants.separator)`. A Docker/Podman image name, container name, or status containing that string can add an extra split segment, causing the refresh to report `segmentsNotMatch` and discard the otherwise valid refresh. The current tests only assert that selected commands omit/include listings; they do not exercise separator-containing output. This would be disproven if the remote command/output contract guarantees that `SrvBoxSep` cannot occur in any command output (including user-controlled image/container metadata).
In lib/data/provider/container.dart around line 539, address this finding:
System prune is reported as successful without refreshing either the container or image resource state. Because `pruneSystem` passes `refreshTarget: null`, removing containers/images (especially with `-a`) leaves both tabs displaying resources that no longer exist until the user manually refreshes them.
In lib/data/model/container/status.dart around line 36, address this finding:
Podman lifecycle status is inferred solely from the `Exited` boolean, so a runtime row with `Exited: false` and raw status `Paused`, `Restarting`, `Created`, or `Dead` is classified as `ContainerStatus.running`. The model now exposes the raw status but does not use it for status mapping, causing non-running containers to be shown as running and potentially enabling running-only actions.
In lib/data/provider/container.dart, address this finding:
A malformed stats payload for one container aborts stats parsing for every later container. The provider wraps the entire item loop in one try/catch, and `parseStats` performs unchecked JSON/type operations (for example Docker `stats['NetIO'] as String?`); if one row has a non-string or invalid payload, subsequent valid rows are never updated and the refresh reports a global parse error.
In lib/data/model/container/image.dart around line 91, address this finding:
Usage matching treats any bare 12–64 hex string as an image ID before treating it as a repository reference. A legitimate runtime image reference whose repository/name is hex-only (for example `0123456789ab`) can therefore match an unrelated image whose ID has that prefix, making an unknown-count tagged image appear used and understating the cleanup count. The normalization needs to distinguish an explicitly ID-shaped runtime value from ambiguous repository references (or require stronger evidence).
In lib/view/page/container/actions.dart, address this finding:
The volume cleanup dialog does not display the command it will execute (nor the selected runtime), while image and system cleanup dialogs display runtime-qualified previews. Selecting the Podman runtime therefore shows only a generic 'prune volumes' confirmation even though confirmation executes `podman volume prune -f`; this leaves the displayed confirmation/preview unsynchronized with the actual Docker/Podman command and is not covered by the widget tests, which never exercise the volume card/dialog. This would be disproven if the product intentionally requires no command preview for volume cleanup or the generic confirmation is documented as sufficient for that action.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 5 of 5 areas reviewed
| .map((line) { | ||
| final separator = line.lastIndexOf('\t'); | ||
| final jsonPart = separator < 0 ? line : line.substring(0, separator); | ||
| final data = json.decode(jsonPart) as Map<String, dynamic>; |
There was a problem hiding this comment.
🔍 Error Handling | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/container/ps.dart, address this finding:
A single malformed Podman JSON process row aborts parsing of the entire container listing. `parsePodmanPsOutput` calls `json.decode` inside an unguarded `map`, and `ContainerNotifier.refresh` invokes it without per-row recovery; for example, one truncated row after valid rows throws, leaving the refresh with no parsed `items` instead of retaining valid containers.
| Text(containerState.runLog!), | ||
| ], | ||
| Widget _buildImagesTab(ContainerState containerState) { | ||
| if (containerState.images == null) { |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/container.dart, address this finding:
The resource tab load/error rendering is shared across tabs and does not distinguish an error from a successful empty result, but more importantly provider parse failures can leave the prior resource list intact while setting `error`: the page checks `items != null`/`images != null` first and renders the populated view even when `containerState.error` is set. For example, after a successful image load followed by malformed image JSON, `refresh()` catches the parse exception and sets `error` without clearing `images`; the Images tab then shows stale populated data instead of the required error state. This would be false only if every parse failure is guaranteed to clear the corresponding list before state update, which the catch path does not do.
| return IconButton( | ||
| key: key, | ||
| tooltip: label, | ||
| onPressed: onPressed, |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/container.dart, address this finding:
Destructive/resource actions are not gated while the container notifier is busy. `_buildPruneAction` always supplies a non-null `onPressed`, and per-container/image popup menus are always built, while `_buildResourceActions` only disables refresh. During an in-flight command (when `isBusy`/`runLog` is active), the user can confirm prune or start another action, causing overlapping commands and refreshes against the same provider instead of the required disabled/gated behavior. This would be false only if the dialog/menu framework globally rejects taps while `run()` or `refresh()` is active, but these callbacks are directly exposed and no such guard exists here.
| ? null | ||
| : _ResourceMetricSlot( | ||
| key: ValueKey('container-resource-circle-$id-cpu'), | ||
| label: 'CPU', |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/resource_views.dart, address this finding:
The new resource labels are not localized at runtime: every locale renders the metric headings as the English literals `CPU`, `MEM`, `NET`, and `DISK`, so the localized explanatory/prune strings are mixed with untranslated labels.
Deploying sbmd with
|
| Latest commit: |
00a3c39
|
| Status: | 🚫 Build failed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/provider/container.dart (1)
642-666: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
runcan wait forever for the sudo probe.Line 655 awaits
sudoCompleter.future. Only_requiresSudocompletes that completer, andrefreshis the only caller of_requiresSudo.buildno longer starts a refresh, and bothsetTypeand_resetSudoProbereplacesudoCompleterwith a new uncompleted completer. If the user runs an action while the current completer is uncompleted,runnever returns.Line 648 already set
runLogto'', so_containerActionsBusyinlib/view/page/container/actions.dartstaystrueand the container UI blocks with no error and no recovery.Start the probe from
runwhen the completer is not completed.🐛 Proposed fix
- final needSudo = await sudoCompleter.future; + final sudo = sudoCompleter; + if (!sudo.isCompleted) { + unawaited( + _requiresSudo(sudo, state.type, ContainerRefreshTarget.containers), + ); + } + final needSudo = await sudo.future;🤖 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 `@lib/data/provider/container.dart` around lines 642 - 666, Update run to ensure the current sudo probe is started before awaiting sudoCompleter.future, invoking _requiresSudo when that completer has not completed. Preserve the existing runLog reset, password handling, and error flow while preventing run from waiting indefinitely after setType or _resetSudoProbe replaces the completer.
🧹 Nitpick comments (1)
lib/view/page/container/actions.dart (1)
338-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the local
idinstead ofdItem.id!.Line 283 already assigns
dItem.idtoidand returns early when it isnull. Lines 339 and 348 repeat the access with!. Useidto remove the null assertion.♻️ Proposed refactor
- final cmd = - '${_containerState.type.name} logs -f --tail 100 ${shellSingleQuote(dItem.id!)}'; + final cmd = + '${_containerState.type.name} logs -f --tail 100 ${shellSingleQuote(id)}';- final cmd = - '${_containerState.type.name} exec -it ${shellSingleQuote(dItem.id!)} sh -c "command -v bash && exec bash || command -v ash && exec ash || exec sh"'; + final cmd = + '${_containerState.type.name} exec -it ${shellSingleQuote(id)} sh -c "command -v bash && exec bash || command -v ash && exec ash || exec sh"';🤖 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 `@lib/view/page/container/actions.dart` around lines 338 - 352, Update the container logs and terminal command construction in the menu action switch to use the local non-null `id` variable established earlier in the handler instead of accessing `dItem.id!`; preserve the existing command behavior while removing the redundant null assertions.
🤖 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 `@lib/data/model/app/menu/container.dart`:
- Around line 23-24: Update the menu branching around status.isStopped so paused
and unknown containers retain the start action, while preserving the existing
rm/logs actions for stopped containers and the running-container path.
In `@test/container_test.dart`:
- Around line 32-42: Update buildContainerRunCmd and its caller to treat
extraArgs as untrusted input: parse argsCtrl.text.trim() into individual
arguments and shell-quote each one, or reject shell operators before
constructing the command. Preserve valid flags such as -p 8080:80 while
preventing injected commands from reaching remote shell execution.
---
Outside diff comments:
In `@lib/data/provider/container.dart`:
- Around line 642-666: Update run to ensure the current sudo probe is started
before awaiting sudoCompleter.future, invoking _requiresSudo when that completer
has not completed. Preserve the existing runLog reset, password handling, and
error flow while preventing run from waiting indefinitely after setType or
_resetSudoProbe replaces the completer.
---
Nitpick comments:
In `@lib/view/page/container/actions.dart`:
- Around line 338-352: Update the container logs and terminal command
construction in the menu action switch to use the local non-null `id` variable
established earlier in the handler instead of accessing `dItem.id!`; preserve
the existing command behavior while removing the redundant null assertions.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e5ea5eb-1330-4936-9f8e-fc8133c42ea8
⛔ Files ignored due to path filters (3)
lib/generated/l10n/l10n_fr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_tr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_zh.dartis excluded by!**/generated/**
📒 Files selected for processing (14)
lib/data/model/app/menu/container.dartlib/data/model/container/image.dartlib/data/model/container/ps.dartlib/data/model/container/status.dartlib/data/provider/container.dartlib/l10n/app_fr.arblib/l10n/app_tr.arblib/l10n/app_zh.arblib/l10n/app_zh_tw.arblib/view/page/container/actions.dartlib/view/page/container/container.dartlib/view/page/container/resource_views.darttest/container_resource_views_test.darttest/container_test.dart
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/l10n/app_zh.arb
- lib/l10n/app_zh_tw.arb
- lib/l10n/app_fr.arb
- lib/l10n/app_tr.arb
- lib/view/page/container/resource_views.dart
- lib/data/model/container/ps.dart
- lib/view/page/container/container.dart
There was a problem hiding this comment.
Actionable comments posted: 8
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
🔎 Confirmed findings (8)
- 🟠 High The container creation path concatenates the user-controlled
extraArgsstring directly into the remote shell command, so shell metacharacters in the UI's Extra Args field can execute arbitrary commands on the host. (inline) - 🟡 Medium Docker statuses such as
Up 5 minutes (Paused)are classified as running because thestartsWith('up')branch runs before the paused check. This exposes stop/restart/terminal actions for a paused container and displays it as running instead of paused; the claim would be false if Docker never emits the human-readable(Paused)suffix in the value passed to this parser. (inline) - 🟡 Medium When a container image reference is digest-qualified,
_addRuntimeImageReferenceremoves the digest and then adds the repository with an implicitlatesttag. Thusregistry.example/app@sha256:<digest>can match an image taggedregistry.example/app:latesteven though the digest may identify a different tag/image, producing a false in-use match and undercounting unused images; this would be false only if digest-qualified references are guaranteed to correspond to the latest tag in the image listing. (inline) - 🟡 Medium
parseContainerImagesOutputdecodes a bracketed JSON array before entering its per-row try/catch. A malformed array or one element that cannot be encoded therefore throws out of the function and discards every otherwise valid image, unlike newline-delimited output where malformed rows are isolated. This would be false only if the runtime's array output is guaranteed to be valid and never contains malformed elements. (inline) - 🟡 Medium The container resource list eagerly builds every row instead of virtualizing large lists.
_ResourceListreceives a fully materializedchildrenlist, and_ContainerGroupCardloops through every item and invokes each trailing builder during build; a large standalone list or expanded compose project therefore constructs all action widgets and resource panels before scrolling, unlike the image list'sListView.builder. This violates the large-list performance contract and can make the container page expensive or janky. (inline) - 🟡 Medium Container lists are not virtualized: every row in every expanded group is constructed eagerly as a child of a regular ListView. With a host returning a large container set (for example, thousands of standalone containers, or a user expanding a large compose project),
_ContainerGroupCard.buildcreates all row subtrees and invokes every row's trailing builder before scrolling, causing high build/layout memory cost and potentially jank/OOM. This is inconsistent with the image tab, which usesListView.builderfor large lists. (inline) - 🟡 Medium Container labels are not safe/synchronized with the chart for over-limit usage. A Docker stats payload such as
MemUsage: "2 GiB / 1 GiB"yieldsmemoryPercent == 200, andCPUperc: "150%"yields 150;_ContainerResourcePanelpassescenterText: '${...}%'using that raw value, while PercentCircle clamps its chart progress to 99.9. The UI therefore displays200.0%/150.0%inside a nearly-full 99.9% chart, violating the expected bounded percentage label/chart contract. (inline) - 🟡 Medium The
pruneDanglingImagesTiptext says that dangling pruning removes only “untagged layers,” but it does not state Docker's other required condition: the image must also be unused by containers. In the image-prune dialog this is presented as the explanation for the default command (image prune), so a user can reasonably infer that every untagged layer will be deleted even when a container references it; this is especially misleading because the view separately exposes a count derived from image metadata and offers a broader-ascope. The wording should describe dangling images as untagged images not referenced by any container (or otherwise accurately describe the runtime's semantics). This is atlib/l10n/app_en.arb, keypruneDanglingImagesTip(and the corresponding generated/locale strings). The claim would be false if the supported Docker/Podman implementations actually define their dangling-prune operation as deleting all untagged layers regardless of container references. (inline)
📋 Additional findings from this change (not shown inline) (20)
- 🟠 High A refresh can run concurrently with a mutation because
refresh()only treatsisBusyas occupied, while mutations signal occupancy withrunLog. The auto-refresh timer (and the refresh buttons, which are disabled only forisBusy) can therefore start a second remote command while a delete/pull/prune/start/stop command is still executing; the mutation then also schedules its own refresh, producing overlapping operations and stale/interleaved state. (lib/data/provider/container.dart) — anchor-outside-diff - 🟡 Medium Dangling images are unconditionally reported as unused, even when their runtime container count is positive or a container references the dangling image by ID. The UI uses
isUnusedfor usage badges and counts, so an in-use<none>:<none>image is presented as safely unused; this would be false only if dangling images are guaranteed never to have container references in all supported runtime outputs. (lib/data/model/container/image.dart) — anchor-unreliable - 🟡 Medium Podman network stats schema selection depends on the locally reported version, and a missing/unparseable version takes the legacy top-level-field branch. For a Podman 5 runtime (or a remote CONTAINER_HOST whose server version differs) emitting nested
Network.{iface}.RxBytes/TxBytes,parseStatsleaves traffic at zero instead of parsing the available counters; this would be false only if the runtime always supplies a matching parseable version and Podman 5 never uses nested network fields. (lib/data/model/container/ps.dart) — anchor-outside-diff - 🟡 Medium Podman PS parsing assumes
Namesis always an iterable list: any runtime/template variant returning a scalar name (for example"Names":"worker") reachesjson['Names']!.map(...)and throwsNoSuchMethodError, which is not caught byparsePodmanPsOutput(it catches only FormatException and TypeError). The entire refresh then fails instead of preserving the valid record; this would be false only if all supported Podman JSON versions guaranteeNamesis always a list. (lib/data/model/container/ps.dart) — anchor-outside-diff - 🟡 Medium
isUnusedtreats every dangling image as unused without consulting its container reference count, so a dangling image that is still referenced by a container is displayed as unused. Docker/Podman can report an untagged (<none>:<none>) image withContainersgreater than zero; in that case cleanup semantics must not classify it as unused (andimage prunewill retain it), but both implementations return true immediately fromif (isDangling) return true. This affects the image summary and per-image usage labels, whose callers count/filter solely viaisUnused. The claim would be false only if the supported runtimes guarantee that dangling images can never have a positive container reference count. (lib/data/model/container/image.dart) — anchor-unreliable - 🟡 Medium A malformed JSON array causes the entire image refresh to fail instead of preserving valid image rows. When image output is bracketed (for example a runtime or wrapper returns
[valid-row, malformed-row, valid-row]), thejson.decode(... ) as Listexpression runs before the per-rowtryblock, so one bad element (or malformed array syntax) throws out of the parser; the provider then sets the image parse error and discards the whole image list. This violates the parser's existing malformed-row convention demonstrated by the newline test, which keeps both valid images. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Podman network-stat format is selected from the client version even though stats may be served by a different remote server version, so remote Podman connections can silently show zero network traffic. The refresh command exports CONTAINER_HOST and the version cache is populated from
Client.Version; for a Podman 4 client talking to a Podman 5 server,parseStatstakes the <=4 branch and reads absent top-level NetInput/NetOutput instead of the server's nested Network counters (and the inverse mismatch loses old counters as well). This is false only if the configured Podman client and server are guaranteed to have identical versions. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Docker containers whose Compose working directory or project label contains a tab cannot be parsed correctly: the command emits raw label values into a tab-delimited record, while
DockerPs.parsetreats every tab as a field boundary and only uses positions 4/5. A valid row such as a working directory containing a tab is split into extra fields and the stored workingDir is truncated, so the UI displays incorrect project metadata. This is false only if Docker label values are guaranteed never to contain tab characters in the supported environments. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Every remote operation with exit code 2 is reported as
sudoPasswordIncorrect, even when sudo is not being used or the runtime itself returned 2 for a command/argument error. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Container summary badges omit recognized non-running states such as paused, restarting, removing, and dead.
_ContainerGroupHeaderhas the same omission, because it counts onlyisRunning,isStopped, and exactunknown; a list containing a paused container therefore shows0 Runningwith no stopped/unknown count even though the row is neither running nor unknown. This loses status information in the new summary presentation. (lib/view/page/container/resource_views.dart) — anchor-unreliable - 🟡 Medium PercentCircle does not sanitize non-finite values, and the container view's explicit center label is generated from the unclamped parsed value. A
NaN/infinite percent reaching another existing PercentCircle call site remains invalid because the switch comparisons do not match NaN; additionally, a valid container metric such as150%sends 99.9 to the chart but displays150.0%, contradicting the rendered gauge. Zero similarly displays0.0%while the chart is forced to 0.01. The metric contract requires bounded chart input and faithful, non-misleading labels. (lib/view/widget/percent_circle.dart) — anchor-outside-diff - 🟡 Medium The container summary and compose-group badges silently omit
paused,restarting, andremovingcontainers:runningcounts onlyisRunning, whilestoppedcounts onlyexited,created, anddead. Thus a list containing only a paused container renders neither1 Stoppednor1 Unknown(and a mixed group reports a total smaller than its item count), despite the row showing a non-running status. This would be false only if those lifecycle states are deliberately intended to be excluded from all summary counts rather than represented as non-running resources. (lib/view/page/container/resource_views.dart) — anchor-unreliable - 🟡 Medium The Add Container dialog treats the free-form
extraArgsfield as trusted shell syntax and sends it directly to the remote shell. For example, entering--label x; touch /tmp/pwned; #producesrun -itd --label x; touch /tmp/pwned; # 'image'; pressing Run executes the injected command becauseContainerNotifier.runonly prefixes the string with the runtime and passes it through_wrap/SSH execution. The preview makes this exact unsafe command look executable rather than restricting or safely tokenizing the arguments. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium An in-flight command can update notifier state after the container page/provider has been disposed. In the normal completion path
run()unconditionally assignsstate = state.copyWith(runLog: null)and then may callrefresh(), even though navigation/provider disposal can occur whileexecWithPwdis awaiting; only the exception callback and stdout callback checkref.mounted. This can leave the operation throwing after the user leaves the page instead of being safely cancelled/ignored. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Known non-running states are silently omitted from container counts, so the summary can report fewer containers than are actually listed. For example, a list containing one paused container produces no
running,stopped, orunknowncount (and a running+paused list displays only1 Running). The same omission occurs in compose-group summaries. (lib/view/page/container/resource_views.dart) — anchor-unreliable - 🟡 Medium The image UI labels every untagged image as dangling/prunable without considering whether containers still reference it. If Docker reports an untagged (
Repository/Tag<none>) image withContainersgreater than zero,isDanglingand the prune dialog's dangling count still include it, and the row gets a Dangling badge, although image pruning must retain an image referenced by a container. This makes the displayed candidate count and badge overstate what the shownimage prune -foperation can remove. (lib/data/model/container/image.dart) — anchor-unreliable - 🟡 Medium Rendering a Podman image with a positive Unix
Createdvalue can throwLocaleDataExceptionfor a non-English locale because_formatUnixDatecallsDateFormat.yMd(locale.toLanguageTag())without repository-wide date-symbol initialization. The widget fixture always setsLocale('en')and its image rows are Docker rows whose timestamps are displayed verbatim, leaving the deployed non-English Podman path untested. This is disproven if the app or dependency initializes all supported intl locale data before this view is reachable on every target. (lib/view/page/container/resource_views.dart) — per-file-budget - 🟡 Medium The image-use count consumes
ContainerPs.imagereferences from both Docker and Podman, but the tests only exercisecountUnusedTaggedImageswith DockerImg and raw strings. Podman image parsing now normalizesNamesinto repository/tag, yet there is no combined test covering a Podman image with unknownContainersmatched against a Podman container's image string (includingdocker.io/library/...aliasing or digest references). A mismatch in either producer's normalization would silently showUnknown/wrong unused counts and affect the-aprune decision. (test/container_test.dart) — anchor-outside-diff - 🟡 Medium A Podman row with valid JSON but a scalar
Namesfield can throw fromList<String>.from(json['Names']!.map(...)), andparsePodmanPsOutputcatches onlyFormatExceptionandTypeError. Depending on the scalar type, the resultingNoSuchMethodErroris not caught, aborting the entire listing instead of skipping only that malformed row. The tests exercise malformed JSON and missing labels but no malformedNamesshape. This is disproven if all supported Podman versions guaranteeNamesis always a list, including partially corrupted output. (lib/data/model/container/ps.dart) — anchor-unreliable - 🔵 Low A legitimate zero container metric produces a chart/label mismatch:
cpu == "0%"(or usage0 B / positive) is parsed as 0 and passed withcenterText: "0.0%", but PercentCircle changes only the chart input to 0.01. Thus the visible label says 0.0% while the chart is nonzero, and this same mismatch applies to any negative value if one is ever supplied via the caller. (lib/view/page/container/resource_views.dart) — per-file-budget
🤖 Prompt for AI agents — all findings (28)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (8)
In lib/data/provider/container.dart around line 45, address this finding:
The container creation path concatenates the user-controlled `extraArgs` string directly into the remote shell command, so shell metacharacters in the UI's Extra Args field can execute arbitrary commands on the host.
In lib/data/model/container/status.dart around line 24, address this finding:
Docker statuses such as `Up 5 minutes (Paused)` are classified as running because the `startsWith('up')` branch runs before the paused check. This exposes stop/restart/terminal actions for a paused container and displays it as running instead of paused; the claim would be false if Docker never emits the human-readable `(Paused)` suffix in the value passed to this parser.
In lib/data/model/container/image.dart around line 77, address this finding:
When a container image reference is digest-qualified, `_addRuntimeImageReference` removes the digest and then adds the repository with an implicit `latest` tag. Thus `registry.example/app@sha256:<digest>` can match an image tagged `registry.example/app:latest` even though the digest may identify a different tag/image, producing a false in-use match and undercounting unused images; this would be false only if digest-qualified references are guaranteed to correspond to the latest tag in the image listing.
In lib/data/provider/container.dart around line 56, address this finding:
`parseContainerImagesOutput` decodes a bracketed JSON array before entering its per-row try/catch. A malformed array or one element that cannot be encoded therefore throws out of the function and discards every otherwise valid image, unlike newline-delimited output where malformed rows are isolated. This would be false only if the runtime's array output is guaranteed to be valid and never contains malformed elements.
In lib/view/page/container/resource_views.dart around line 698, address this finding:
The container resource list eagerly builds every row instead of virtualizing large lists. `_ResourceList` receives a fully materialized `children` list, and `_ContainerGroupCard` loops through every item and invokes each trailing builder during build; a large standalone list or expanded compose project therefore constructs all action widgets and resource panels before scrolling, unlike the image list's `ListView.builder`. This violates the large-list performance contract and can make the container page expensive or janky.
In lib/view/page/container/resource_views.dart around line 697, address this finding:
Container lists are not virtualized: every row in every expanded group is constructed eagerly as a child of a regular ListView. With a host returning a large container set (for example, thousands of standalone containers, or a user expanding a large compose project), `_ContainerGroupCard.build` creates all row subtrees and invokes every row's trailing builder before scrolling, causing high build/layout memory cost and potentially jank/OOM. This is inconsistent with the image tab, which uses `ListView.builder` for large lists.
In lib/view/page/container/resource_views.dart around line 907, address this finding:
Container labels are not safe/synchronized with the chart for over-limit usage. A Docker stats payload such as `MemUsage: "2 GiB / 1 GiB"` yields `memoryPercent == 200`, and `CPUperc: "150%"` yields 150; `_ContainerResourcePanel` passes `centerText: '${...}%'` using that raw value, while PercentCircle clamps its chart progress to 99.9. The UI therefore displays `200.0%`/`150.0%` inside a nearly-full 99.9% chart, violating the expected bounded percentage label/chart contract.
In lib/l10n/app_en.arb around line 123, address this finding:
The `pruneDanglingImagesTip` text says that dangling pruning removes only “untagged layers,” but it does not state Docker's other required condition: the image must also be unused by containers. In the image-prune dialog this is presented as the explanation for the default command (`image prune`), so a user can reasonably infer that every untagged layer will be deleted even when a container references it; this is especially misleading because the view separately exposes a count derived from image metadata and offers a broader `-a` scope. The wording should describe dangling images as untagged images not referenced by any container (or otherwise accurately describe the runtime's semantics). This is at `lib/l10n/app_en.arb`, key `pruneDanglingImagesTip` (and the corresponding generated/locale strings). The claim would be false if the supported Docker/Podman implementations actually define their dangling-prune operation as deleting all untagged layers regardless of container references.
## Additional findings on this change (not posted inline) (20)
In lib/data/provider/container.dart around line 248, address this finding:
A refresh can run concurrently with a mutation because `refresh()` only treats `isBusy` as occupied, while mutations signal occupancy with `runLog`. The auto-refresh timer (and the refresh buttons, which are disabled only for `isBusy`) can therefore start a second remote command while a delete/pull/prune/start/stop command is still executing; the mutation then also schedules its own refresh, producing overlapping operations and stale/interleaved state.
In lib/data/model/container/image.dart, address this finding:
Dangling images are unconditionally reported as unused, even when their runtime container count is positive or a container references the dangling image by ID. The UI uses `isUnused` for usage badges and counts, so an in-use `<none>:<none>` image is presented as safely unused; this would be false only if dangling images are guaranteed never to have container references in all supported runtime outputs.
In lib/data/model/container/ps.dart around line 88, address this finding:
Podman network stats schema selection depends on the locally reported version, and a missing/unparseable version takes the legacy top-level-field branch. For a Podman 5 runtime (or a remote CONTAINER_HOST whose server version differs) emitting nested `Network.{iface}.RxBytes/TxBytes`, `parseStats` leaves traffic at zero instead of parsing the available counters; this would be false only if the runtime always supplies a matching parseable version and Podman 5 never uses nested network fields.
In lib/data/model/container/ps.dart around line 116, address this finding:
Podman PS parsing assumes `Names` is always an iterable list: any runtime/template variant returning a scalar name (for example `"Names":"worker"`) reaches `json['Names']!.map(...)` and throws `NoSuchMethodError`, which is not caught by `parsePodmanPsOutput` (it catches only FormatException and TypeError). The entire refresh then fails instead of preserving the valid record; this would be false only if all supported Podman JSON versions guarantee `Names` is always a list.
In lib/data/model/container/image.dart, address this finding:
`isUnused` treats every dangling image as unused without consulting its container reference count, so a dangling image that is still referenced by a container is displayed as unused. Docker/Podman can report an untagged (`<none>:<none>`) image with `Containers` greater than zero; in that case cleanup semantics must not classify it as unused (and `image prune` will retain it), but both implementations return true immediately from `if (isDangling) return true`. This affects the image summary and per-image usage labels, whose callers count/filter solely via `isUnused`. The claim would be false only if the supported runtimes guarantee that dangling images can never have a positive container reference count.
In lib/data/provider/container.dart around line 55, address this finding:
A malformed JSON array causes the entire image refresh to fail instead of preserving valid image rows. When image output is bracketed (for example a runtime or wrapper returns `[valid-row, malformed-row, valid-row]`), the `json.decode(... ) as List` expression runs before the per-row `try` block, so one bad element (or malformed array syntax) throws out of the parser; the provider then sets the image parse error and discards the whole image list. This violates the parser's existing malformed-row convention demonstrated by the newline test, which keeps both valid images.
In lib/data/provider/container.dart around line 423, address this finding:
Podman network-stat format is selected from the client version even though stats may be served by a different remote server version, so remote Podman connections can silently show zero network traffic. The refresh command exports CONTAINER_HOST and the version cache is populated from `Client.Version`; for a Podman 4 client talking to a Podman 5 server, `parseStats` takes the <=4 branch and reads absent top-level NetInput/NetOutput instead of the server's nested Network counters (and the inverse mismatch loses old counters as well). This is false only if the configured Podman client and server are guaranteed to have identical versions.
In lib/data/provider/container.dart around line 743, address this finding:
Docker containers whose Compose working directory or project label contains a tab cannot be parsed correctly: the command emits raw label values into a tab-delimited record, while `DockerPs.parse` treats every tab as a field boundary and only uses positions 4/5. A valid row such as a working directory containing a tab is split into extra fields and the stored workingDir is truncated, so the UI displays incorrect project metadata. This is false only if Docker label values are guaranteed never to contain tab characters in the supported environments.
In lib/data/provider/container.dart around line 692, address this finding:
Every remote operation with exit code 2 is reported as `sudoPasswordIncorrect`, even when sudo is not being used or the runtime itself returned 2 for a command/argument error.
In lib/view/page/container/resource_views.dart, address this finding:
Container summary badges omit recognized non-running states such as paused, restarting, removing, and dead. `_ContainerGroupHeader` has the same omission, because it counts only `isRunning`, `isStopped`, and exact `unknown`; a list containing a paused container therefore shows `0 Running` with no stopped/unknown count even though the row is neither running nor unknown. This loses status information in the new summary presentation.
In lib/view/widget/percent_circle.dart around line 17, address this finding:
PercentCircle does not sanitize non-finite values, and the container view's explicit center label is generated from the unclamped parsed value. A `NaN`/infinite percent reaching another existing PercentCircle call site remains invalid because the switch comparisons do not match NaN; additionally, a valid container metric such as `150%` sends 99.9 to the chart but displays `150.0%`, contradicting the rendered gauge. Zero similarly displays `0.0%` while the chart is forced to 0.01. The metric contract requires bounded chart input and faithful, non-misleading labels.
In lib/view/page/container/resource_views.dart, address this finding:
The container summary and compose-group badges silently omit `paused`, `restarting`, and `removing` containers: `running` counts only `isRunning`, while `stopped` counts only `exited`, `created`, and `dead`. Thus a list containing only a paused container renders neither `1 Stopped` nor `1 Unknown` (and a mixed group reports a total smaller than its item count), despite the row showing a non-running status. This would be false only if those lifecycle states are deliberately intended to be excluded from all summary counts rather than represented as non-running resources.
In lib/data/provider/container.dart around line 39, address this finding:
The Add Container dialog treats the free-form `extraArgs` field as trusted shell syntax and sends it directly to the remote shell. For example, entering `--label x; touch /tmp/pwned; #` produces `run -itd --label x; touch /tmp/pwned; # 'image'`; pressing Run executes the injected command because `ContainerNotifier.run` only prefixes the string with the runtime and passes it through `_wrap`/SSH execution. The preview makes this exact unsafe command look executable rather than restricting or safely tokenizing the arguments.
In lib/data/provider/container.dart around line 690, address this finding:
An in-flight command can update notifier state after the container page/provider has been disposed. In the normal completion path `run()` unconditionally assigns `state = state.copyWith(runLog: null)` and then may call `refresh()`, even though navigation/provider disposal can occur while `execWithPwd` is awaiting; only the exception callback and stdout callback check `ref.mounted`. This can leave the operation throwing after the user leaves the page instead of being safely cancelled/ignored.
In lib/view/page/container/resource_views.dart, address this finding:
Known non-running states are silently omitted from container counts, so the summary can report fewer containers than are actually listed. For example, a list containing one paused container produces no `running`, `stopped`, or `unknown` count (and a running+paused list displays only `1 Running`). The same omission occurs in compose-group summaries.
In lib/data/model/container/image.dart, address this finding:
The image UI labels every untagged image as dangling/prunable without considering whether containers still reference it. If Docker reports an untagged (`Repository`/`Tag` `<none>`) image with `Containers` greater than zero, `isDangling` and the prune dialog's dangling count still include it, and the row gets a Dangling badge, although image pruning must retain an image referenced by a container. This makes the displayed candidate count and badge overstate what the shown `image prune -f` operation can remove.
In lib/view/page/container/resource_views.dart around line 1365, address this finding:
Rendering a Podman image with a positive Unix `Created` value can throw `LocaleDataException` for a non-English locale because `_formatUnixDate` calls `DateFormat.yMd(locale.toLanguageTag())` without repository-wide date-symbol initialization. The widget fixture always sets `Locale('en')` and its image rows are Docker rows whose timestamps are displayed verbatim, leaving the deployed non-English Podman path untested. This is disproven if the app or dependency initializes all supported intl locale data before this view is reachable on every target.
In test/container_test.dart around line 563, address this finding:
The image-use count consumes `ContainerPs.image` references from both Docker and Podman, but the tests only exercise `countUnusedTaggedImages` with DockerImg and raw strings. Podman image parsing now normalizes `Names` into repository/tag, yet there is no combined test covering a Podman image with unknown `Containers` matched against a Podman container's image string (including `docker.io/library/...` aliasing or digest references). A mismatch in either producer's normalization would silently show `Unknown`/wrong unused counts and affect the `-a` prune decision.
In lib/data/model/container/ps.dart, address this finding:
A Podman row with valid JSON but a scalar `Names` field can throw from `List<String>.from(json['Names']!.map(...))`, and `parsePodmanPsOutput` catches only `FormatException` and `TypeError`. Depending on the scalar type, the resulting `NoSuchMethodError` is not caught, aborting the entire listing instead of skipping only that malformed row. The tests exercise malformed JSON and missing labels but no malformed `Names` shape. This is disproven if all supported Podman versions guarantee `Names` is always a list, including partially corrupted output.
In lib/view/page/container/resource_views.dart around line 897, address this finding:
A legitimate zero container metric produces a chart/label mismatch: `cpu == "0%"` (or usage `0 B / positive`) is parsed as 0 and passed with `centerText: "0.0%"`, but PercentCircle changes only the chart input to 0.01. Thus the visible label says 0.0% while the chart is nonzero, and this same mismatch applies to any negative value if one is ever supplied via the caller.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 6 of 6 areas reviewed
| } | ||
| final reference = _splitImageReference(withoutDigest); | ||
| _addRepositoryMarkers(markers, reference.repository, reference.tag); | ||
| } |
There was a problem hiding this comment.
🔍 Data Integrity | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/container/image.dart, address this finding:
When a container image reference is digest-qualified, `_addRuntimeImageReference` removes the digest and then adds the repository with an implicit `latest` tag. Thus `registry.example/app@sha256:<digest>` can match an image tagged `registry.example/app:latest` even though the digest may identify a different tag/image, producing a false in-use match and undercounting unused images; this would be false only if digest-qualified references are guaranteed to correspond to the latest tag in the image listing.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| } | |
| void _addRuntimeImageReference(Set<String> markers, String? raw) { | |
| final value = raw?.trim(); | |
| if (value == null || value.isEmpty) return; | |
| final digestSeparator = value.indexOf('@'); | |
| if (digestSeparator >= 0) { | |
| _addImageId(markers, value.substring(digestSeparator + 1).trim()); | |
| return; | |
| } | |
| final reference = _splitImageReference(value); | |
| _addRepositoryMarkers(markers, reference.repository, reference.tag); | |
| } |
| ) { | ||
| final trimmed = raw.trim(); | ||
| final encodedRows = trimmed.startsWith('[') && trimmed.endsWith(']') | ||
| ? (json.decode(trimmed) as List).map(json.encode) |
There was a problem hiding this comment.
🔍 Compatibility | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/provider/container.dart, address this finding:
`parseContainerImagesOutput` decodes a bracketed JSON array before entering its per-row try/catch. A malformed array or one element that cannot be encoded therefore throws out of the function and discards every otherwise valid image, unlike newline-delimited output where malformed rows are isolated. This would be false only if the runtime's array output is guaranteed to be valid and never contains malformed elements.
| if (showItems) const Divider(height: 1), | ||
| ], | ||
| if (showItems) | ||
| for (var index = 0; index < group.items.length; index++) ...[ |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/resource_views.dart, address this finding:
The container resource list eagerly builds every row instead of virtualizing large lists. `_ResourceList` receives a fully materialized `children` list, and `_ContainerGroupCard` loops through every item and invokes each trailing builder during build; a large standalone list or expanded compose project therefore constructs all action widgets and resource panels before scrolling, unlike the image list's `ListView.builder`. This violates the large-list performance contract and can make the container page expensive or janky.
| ), | ||
| if (showItems) const Divider(height: 1), | ||
| ], | ||
| if (showItems) |
There was a problem hiding this comment.
⚡ Performance | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/resource_views.dart, address this finding:
Container lists are not virtualized: every row in every expanded group is constructed eagerly as a child of a regular ListView. With a host returning a large container set (for example, thousands of standalone containers, or a user expanding a large compose project), `_ContainerGroupCard.build` creates all row subtrees and invokes every row's trailing builder before scrolling, causing high build/layout memory cost and potentially jank/OOM. This is inconsistent with the image tab, which uses `ListView.builder` for large lists.
| label: 'MEM', | ||
| child: PercentCircle( | ||
| percent: data.memoryPercent!, | ||
| centerText: '${data.memoryPercent!.toStringAsFixed(1)}%', |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/resource_views.dart, address this finding:
Container labels are not safe/synchronized with the chart for over-limit usage. A Docker stats payload such as `MemUsage: "2 GiB / 1 GiB"` yields `memoryPercent == 200`, and `CPUperc: "150%"` yields 150; `_ContainerResourcePanel` passes `centerText: '${...}%'` using that raw value, while PercentCircle clamps its chart progress to 99.9. The UI therefore displays `200.0%`/`150.0%` inside a nearly-full 99.9% chart, violating the expected bounded percentage label/chart contract.
| "pruneDanglingImages": "Prune dangling images", | ||
| "pruneImages": "Prune images", | ||
| "unusedTaggedImages": "Unused tagged", | ||
| "pruneDanglingImagesTip": "Only remove dangling images (untagged layers).", |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/l10n/app_en.arb, address this finding:
The `pruneDanglingImagesTip` text says that dangling pruning removes only “untagged layers,” but it does not state Docker's other required condition: the image must also be unused by containers. In the image-prune dialog this is presented as the explanation for the default command (`image prune`), so a user can reasonably infer that every untagged layer will be deleted even when a container references it; this is especially misleading because the view separately exposes a count derived from image metadata and offers a broader `-a` scope. The wording should describe dangling images as untagged images not referenced by any container (or otherwise accurately describe the runtime's semantics). This is at `lib/l10n/app_en.arb`, key `pruneDanglingImagesTip` (and the corresponding generated/locale strings). The claim would be false if the supported Docker/Podman implementations actually define their dangling-prune operation as deleting all untagged layers regardless of container references.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| "pruneDanglingImagesTip": "Only remove dangling images (untagged layers).", | |
| "pruneDanglingImagesTip": "Only remove dangling images (untagged images not referenced by any container).", |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/provider/container.dart (1)
227-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock runtime changes while a container action is running.
runsetsrunLogbefore it capturesstate.typeat Line 704.setTypecan then clearrunLogand change the runtime while that action awaits sudo or command execution. The command can run against the old runtime, but its refresh uses the new runtime.Reject or defer
setTypewhilestate.runLog != null.Proposed fix
void setType(ContainerType type) { + if (state.runLog != null) return; _resetSudoProbe();🤖 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 `@lib/data/provider/container.dart` around lines 227 - 239, Update setType in the container state controller to reject or defer type changes whenever state.runLog is non-null, leaving the current runtime and action state unchanged while run is awaiting sudo or command execution. Preserve the existing reset and state-copy behavior for idle containers, and only call Stores.container.setType for accepted changes.
🤖 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.
Outside diff comments:
In `@lib/data/provider/container.dart`:
- Around line 227-239: Update setType in the container state controller to
reject or defer type changes whenever state.runLog is non-null, leaving the
current runtime and action state unchanged while run is awaiting sudo or command
execution. Preserve the existing reset and state-copy behavior for idle
containers, and only call Stores.container.setType for accepted changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 457553e8-bd29-4191-9b8f-5985472cc436
📒 Files selected for processing (3)
lib/data/provider/container.dartlib/view/page/container/actions.darttest/container_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/container_test.dart
- lib/view/page/container/actions.dart
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/data/provider/container.dart (2)
461-464: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalize
ContainerErr.messageliterals.The messages at Lines 463 and 701 are hard-coded English. The segment error is rendered by
lib/view/page/container/container.dartin Lines 194-220, and the command error can reach action feedback. Add localized keys and pass the dynamic segment count as a parameter.As per coding guidelines,
lib/**/*.dartmust uselibL10nandl10nfor localization strings, prioritizinglibL10nfromfl_libto avoid duplication.Also applies to: 698-702
🤖 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 `@lib/data/provider/container.dart` around lines 461 - 464, Replace the hard-coded ContainerErr messages at the segmentsNotMatch and corresponding command-error sites with localized libL10n/l10n keys, using the existing localization conventions and passing the dynamic segment count as a parameter. Add the required localization entries and update the container rendering/action-feedback paths to resolve these keys without duplicating English literals.Source: Coding guidelines
299-307: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent refresh and command overlap.
If
state.runLog != null,refresh()must queue the target instead of starting. Use_containerActionsBusyfor the resource refresh and error-retry controls.🤖 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 `@lib/data/provider/container.dart` around lines 299 - 307, Update refresh() to queue the target and return whenever state.runLog is non-null, preventing refresh from overlapping commands. Use _containerActionsBusy for the resource-refresh and error-retry busy controls while preserving the existing _pendingRefreshTarget behavior.lib/view/page/container/container.dart (1)
64-133: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit the new page sections into
extension onblocks.
_ContainerPageStatenow contains the Widget build, Actions, and Utils methods from Lines 64-454. Move these methods into separateextension on _ContainerPageStatesections. This follows the required page structure and reduces the size of the state class.As per coding guidelines,
lib/view/**/*.dartmust split UI into Widget build, Actions, and Utils usingextension on.Also applies to: 135-253, 255-309, 313-367, 369-415, 418-460
🤖 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 `@lib/view/page/container/container.dart` around lines 64 - 133, Split the methods currently defined directly on _ContainerPageState into separate extension on _ContainerPageState blocks for Widget build, Actions, and Utils, covering the page sections through the referenced ranges. Keep each method’s implementation unchanged while relocating it into the appropriate extension, leaving the state class focused on state, initialization, and lifecycle members.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.
Outside diff comments:
In `@lib/data/provider/container.dart`:
- Around line 461-464: Replace the hard-coded ContainerErr messages at the
segmentsNotMatch and corresponding command-error sites with localized
libL10n/l10n keys, using the existing localization conventions and passing the
dynamic segment count as a parameter. Add the required localization entries and
update the container rendering/action-feedback paths to resolve these keys
without duplicating English literals.
- Around line 299-307: Update refresh() to queue the target and return whenever
state.runLog is non-null, preventing refresh from overlapping commands. Use
_containerActionsBusy for the resource-refresh and error-retry busy controls
while preserving the existing _pendingRefreshTarget behavior.
In `@lib/view/page/container/container.dart`:
- Around line 64-133: Split the methods currently defined directly on
_ContainerPageState into separate extension on _ContainerPageState blocks for
Widget build, Actions, and Utils, covering the page sections through the
referenced ranges. Keep each method’s implementation unchanged while relocating
it into the appropriate extension, leaving the state class focused on state,
initialization, and lifecycle members.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 671e9c08-4f3c-4dc5-b6e7-0301baa18d71
📒 Files selected for processing (2)
lib/data/provider/container.dartlib/view/page/container/container.dart
There was a problem hiding this comment.
Actionable comments posted: 10
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
🔎 Confirmed findings (10)
- 🟡 Medium Container actions do not participate in the refresh busy gate.
run()sets onlyrunLog, whilerefresh()rejects work only whenstate.isBusyis true; therefore an auto-refresh (or tab refresh) can start while a start/stop/rm/prune command is still executing. The refresh can read partially changing runtime state and, more importantly, action completion then starts another refresh after clearingrunLog, allowing overlapping remote commands and inconsistent state/error updates. This is false only if every caller guarantees that no refresh can occur duringrun()(including the periodic auto-refresh), which the provider's public API and timer caller do not enforce. (inline) - 🟡 Medium PercentCircle can render a chart clamped to 99.9 while displaying an unclamped value above 100%, so the visual percentage and text disagree for large/malformed-but-parseable CPU or memory values. (inline)
- 🟡 Medium The widget suite does not assert the empty-state obligation for either resource list.
ContainerItemsViewandContainerImagesVieweach render a distinct_EmptyResourceCardwhen their input is empty (and the container page can additionally provide a customemptyState), yet the only empty-list test issummary refresh actions follow prune and are tappable, which checks actions and never checks that the empty card/message is present or that rows are absent. A regression that drops the empty card, shows the wrong icon/message, or accidentally builds a row would pass this suite. This is disproven if another test outside this file instantiates both views with empty data and asserts their empty card/message and no rows. (inline) - 🟡 Medium The image prune dialog reports
Unused tagged: Unknownwhenever the container list has not been loaded, even when every image has a knownContainerscount._showImagePruneDialogpassesnulldirectly in that case instead of callingcountUnusedTaggedImages(images, const []), whose documented behavior can count all known zero-reference tagged images without container rows. (inline) - 🟡 Medium The added widget coverage does not exercise the production prune dialogs or notifier wiring: both test helpers construct the option views directly and synthesize the preview from a test
typeargument. Consequently, a regression in_runtimePruneCommand, the page'sallUnused/volume callback capture, or the actual Docker-vs-PodmanContainerState.typeselection would leave all current tests green. The tests also omit the Podman image-prune UI and toggling either option back to its default, so they do not cover every UI combination requested by the feature. (inline) - 🟡 Medium The Simplified Chinese label for
unusedTaggedImagesis未使用标记(unused tags), which omits the image/resource subject and can mislead users about what the prune count represents. This label is used directly for the unused-tagged-image count badge in the image prune UI, so users may interpret the count as tags rather than images. (inline) - 🟡 Medium
ContainerStatus.fromDockerStatechecksstartsWith('up')before lifecycle keywords, but Podman.Statuscommonly contains strings such asUp 5 seconds (healthy)and is correctly running; more importantly, statuses such asUp ... (paused)or other compound status text are classified running because the broadstartsWith('up')branch wins. The UI then exposes stop/restart instead of the appropriate stopped/unknown action state, and the group summary/action bulk lists use the same misclassification. (inline) - 🟡 Medium Resource unit parsing is not comprehensively covered: no test asserts decimal SI units, lowercase/case variants, bare bytes, or malformed units. In particular,
_parseByteSizeuses an optional unit and an unanchored match, so a value such as1 XBis accepted as1byte instead of being omitted; a regression or fix in this behavior would pass the current suite because the only invalid fixtures are words such asnot availableandgarbage. This can make an invalid runtime stat render a misleading memory percentage. (inline) - 🟡 Medium
run()has no generation or mounted checks after its awaited sudo probe/password dialog or command execution. If the container page/provider is disposed while the dialog or SSH command is pending, the continuation still executesstate = state.copyWith(...)(including the password-cancel path and the post-commandrunLogclear), which can update a disposed Riverpod notifier or throw instead of completing cleanly. The refresh path explicitly checks_isStaleRefresh/ref.mounted, but the action path does not. This is false only if provider disposal is guaranteed never to occur during any action or password prompt. (inline) - 🟡 Medium
sudoCompleteris reused for both container and image refreshes, but_requiresSudoprobes only the first requested target (psorimages). If that target succeeds without sudo while the other target requires sudo (or the probe is against a target-specific permission boundary), subsequent refreshes/actions reuse the wrongfalseresult and execute without sudo, leaving that tab empty/erroring. The cache must be keyed by runtime/target or invalidated when the target changes. (inline)
⛔ Unresolved from previous review (1) — not approved until fixed
- lib/data/provider/container.dart: A refresh can run concurrently with a mutation because
refresh()only treatsisBusyas occupied, while mutations signal occupancy withrunLog. The auto-refresh timer (and the refresh buttons, which are disabled only forisBusy) can therefore start a second remote command while a delete/pull/prune/start/stop command is still executing; the mutation then also schedules its own refresh, producing overlapping operations and stale/interleaved state. — Still present: refresh() continues to gate only on state.isBusy. run() rejects competing operations using state.isBusy or state.runLog, then marks a mutation active with state.copyWith(runLog: ''). Since it does not set isBusy, an auto-refresh or button-triggered refresh can pass this check and execute concurrently; the mutation's post-command refresh can then overlap as well.
📋 Additional findings from this change (not shown inline) (9)
- 🟡 Medium The extracted resource-view widgets are only instantiated with
type: ContainerType.docker; no widget test renders either view with Podman items/type. Thus Podman-specific image/container presentation and runtime summary/icon paths are untested, even though the scope requires coverage of both runtime models. A Podman model incompatibility in the views would pass the suite. (test/container_resource_views_test.dart) — anchor-unreliable - 🟡 Medium Supported locales can silently show English in otherwise localized runtime UI: the German ARB has no
invalidUrlentry, while the generated German class implementsinvalidUrlasInvalid URL. Any German screen usingcontext.l10n.invalidUrltherefore displays English rather than German, and the generated interface gives no compile-time indication that the translation is missing. The same fallback pattern is present in other supported locales (for exampleconfiguredandinvalidUrlin several generated classes). (lib/generated/l10n/l10n_de.dart) — anchor-outside-diff - 🟡 Medium There is no Docker missing-field test, and
DockerImg.fromJsonwill fail on a valid row that omitsCreatedAtorTag: both are passed directly to non-nullDockerImgconstructor fields.parseContainerImagesOutputcatches that exception and silently drops the image, so a runtime response with an omitted optional field loses an otherwise valid image. The existing malformed-image test only proves that a wholly invalid JSON line is skipped, and the missing numeric-field test covers Podman only. (lib/data/model/container/image.dart) — anchor-outside-diff - 🟡 Medium Container status labels are not localized for five enum states:
created,paused,restarting,removing, anddeadalways display English text. When a non-English locale is selected and a container is in one of these states, the container page mixes English status labels into the localized UI, violating the localization contract. (lib/data/model/container/status.dart) — anchor-outside-diff - 🟡 Medium A Docker image row with a missing/non-string
CreatedAtfield is not treated as a malformed row that can be skipped while preserving other rows;DockerImg.fromJsonpassesjson['CreatedAt']directly into the non-nullableString createdAtconstructor parameter. In legacy/custom Docker formats whereCreatedAtis absent or numeric, this throws andparseContainerImagesOutputskips that row (or a whole JSON-array parse can fail), so otherwise usable image metadata disappears. This is false only if all supported Docker image JSON formats always provide a stringCreatedAt. (lib/data/model/container/image.dart) — anchor-outside-diff - 🔵 Low The prune-option widget tests do not cover all required system-prune combinations: they never assert the volumes-only state/preview (
docker system prune --volumes -for Podman equivalent). A regression that drops--volumeswhen-ais false would pass the added tests, despite the obligation requiring exact previews for all combinations. (test/container_resource_views_test.dart) — inline-budget - 🔵 Low The system-prune widget test covers default,
-a, and-a --volumes, but never selects--volumesalone, so the UI state transition and exact preview for that fourth combination are unverified. The provider builder has a distinct branch for this case (system prune --volumes -f), and the production dialog forwardsincludeVolumesindependently; a callback/state wiring regression could therefore pass the current widget test while running the wrong command. (test/container_resource_views_test.dart) — inline-budget - 🔵 Low The French
includeUnusedVolumesTipuses awkward/reversed wording (volumes utilisés par aucun conteneur), which can be read as volumes that are used by no container only with effort and is materially less clear than the source's explicit unused-volume condition. This is shown directly beneath the--volumesprune switch, where ambiguity affects the user's understanding of which volumes will be deleted. (lib/l10n/app_fr.arb) — inline-budget - 🔵 Low Unknown image-usage and Podman selection are tested separately but not together: the unknown-count test uses the default Docker helper, while the only Podman prune preview test covers system prune. There is no assertion for a Podman image-prune UI showing
Unused tagged: Unknownand previewspodman image prune -f/podman image prune -a -f; a runtime-specific wiring regression in that required combination would pass. (test/container_resource_views_test.dart) — anchor-unreliable
♻️ Previously reported (still present) (1)
- 🟡 Medium Digest-qualified container image references are incorrectly treated as unrelated to the image row when the runtime reports an unknown container count.
_addRuntimeImageReferenceremoves everything after@before checking for asha256:ID, so a reference such asregistry.example/app@sha256:<image-id>contributes only a repository marker and cannot match the image's ID marker.countUnusedTaggedImagesthen returnsnull(unknown) or may classify the tagged image as unresolved instead of confirming it is in use. This is false if the runtime never supplies digest-qualifiedImagereferences to this helper or always supplies a numeric Containers count. (lib/data/model/container/image.dart) — previously-reported
🤖 Prompt for AI agents — all findings (21)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (1)
In lib/data/provider/container.dart, address this finding:
A refresh can run concurrently with a mutation because `refresh()` only treats `isBusy` as occupied, while mutations signal occupancy with `runLog`. The auto-refresh timer (and the refresh buttons, which are disabled only for `isBusy`) can therefore start a second remote command while a delete/pull/prune/start/stop command is still executing; the mutation then also schedules its own refresh, producing overlapping operations and stale/interleaved state.
## Findings on this change (also posted as inline comments) (10)
In lib/data/provider/container.dart around line 704, address this finding:
Container actions do not participate in the refresh busy gate. `run()` sets only `runLog`, while `refresh()` rejects work only when `state.isBusy` is true; therefore an auto-refresh (or tab refresh) can start while a start/stop/rm/prune command is still executing. The refresh can read partially changing runtime state and, more importantly, action completion then starts another refresh after clearing `runLog`, allowing overlapping remote commands and inconsistent state/error updates. This is false only if every caller guarantees that no refresh can occur during `run()` (including the periodic auto-refresh), which the provider's public API and timer caller do not enforce.
In lib/view/page/container/resource_views.dart around line 897, address this finding:
PercentCircle can render a chart clamped to 99.9 while displaying an unclamped value above 100%, so the visual percentage and text disagree for large/malformed-but-parseable CPU or memory values.
In test/container_resource_views_test.dart around line 561, address this finding:
The widget suite does not assert the empty-state obligation for either resource list. `ContainerItemsView` and `ContainerImagesView` each render a distinct `_EmptyResourceCard` when their input is empty (and the container page can additionally provide a custom `emptyState`), yet the only empty-list test is `summary refresh actions follow prune and are tappable`, which checks actions and never checks that the empty card/message is present or that rows are absent. A regression that drops the empty card, shows the wrong icon/message, or accidentally builds a row would pass this suite. This is disproven if another test outside this file instantiates both views with empty data and asserts their empty card/message and no rows.
In lib/view/page/container/actions.dart around line 108, address this finding:
The image prune dialog reports `Unused tagged: Unknown` whenever the container list has not been loaded, even when every image has a known `Containers` count. `_showImagePruneDialog` passes `null` directly in that case instead of calling `countUnusedTaggedImages(images, const [])`, whose documented behavior can count all known zero-reference tagged images without container rows.
In test/container_resource_views_test.dart around line 86, address this finding:
The added widget coverage does not exercise the production prune dialogs or notifier wiring: both test helpers construct the option views directly and synthesize the preview from a test `type` argument. Consequently, a regression in `_runtimePruneCommand`, the page's `allUnused`/volume callback capture, or the actual Docker-vs-Podman `ContainerState.type` selection would leave all current tests green. The tests also omit the Podman image-prune UI and toggling either option back to its default, so they do not cover every UI combination requested by the feature.
In lib/l10n/app_zh.arb around line 120, address this finding:
The Simplified Chinese label for `unusedTaggedImages` is `未使用标记` (unused tags), which omits the image/resource subject and can mislead users about what the prune count represents. This label is used directly for the unused-tagged-image count badge in the image prune UI, so users may interpret the count as tags rather than images.
In lib/data/model/container/status.dart around line 24, address this finding:
`ContainerStatus.fromDockerState` checks `startsWith('up')` before lifecycle keywords, but Podman `.Status` commonly contains strings such as `Up 5 seconds (healthy)` and is correctly running; more importantly, statuses such as `Up ... (paused)` or other compound status text are classified running because the broad `startsWith('up')` branch wins. The UI then exposes stop/restart instead of the appropriate stopped/unknown action state, and the group summary/action bulk lists use the same misclassification.
In lib/view/page/container/resource_views.dart around line 1289, address this finding:
Resource unit parsing is not comprehensively covered: no test asserts decimal SI units, lowercase/case variants, bare bytes, or malformed units. In particular, `_parseByteSize` uses an optional unit and an unanchored match, so a value such as `1 XB` is accepted as `1` byte instead of being omitted; a regression or fix in this behavior would pass the current suite because the only invalid fixtures are words such as `not available` and `garbage`. This can make an invalid runtime stat render a misleading memory percentage.
In lib/data/provider/container.dart around line 727, address this finding:
`run()` has no generation or mounted checks after its awaited sudo probe/password dialog or command execution. If the container page/provider is disposed while the dialog or SSH command is pending, the continuation still executes `state = state.copyWith(...)` (including the password-cancel path and the post-command `runLog` clear), which can update a disposed Riverpod notifier or throw instead of completing cleanly. The refresh path explicitly checks `_isStaleRefresh`/`ref.mounted`, but the action path does not. This is false only if provider disposal is guaranteed never to occur during any action or password prompt.
In lib/data/provider/container.dart around line 712, address this finding:
`sudoCompleter` is reused for both container and image refreshes, but `_requiresSudo` probes only the first requested target (`ps` or `images`). If that target succeeds without sudo while the other target requires sudo (or the probe is against a target-specific permission boundary), subsequent refreshes/actions reuse the wrong `false` result and execute without sudo, leaving that tab empty/erroring. The cache must be keyed by runtime/target or invalidated when the target changes.
## Additional findings on this change (not posted inline) (9)
In test/container_resource_views_test.dart, address this finding:
The extracted resource-view widgets are only instantiated with `type: ContainerType.docker`; no widget test renders either view with Podman items/type. Thus Podman-specific image/container presentation and runtime summary/icon paths are untested, even though the scope requires coverage of both runtime models. A Podman model incompatibility in the views would pass the suite.
In lib/generated/l10n/l10n_de.dart around line 453, address this finding:
Supported locales can silently show English in otherwise localized runtime UI: the German ARB has no `invalidUrl` entry, while the generated German class implements `invalidUrl` as `Invalid URL`. Any German screen using `context.l10n.invalidUrl` therefore displays English rather than German, and the generated interface gives no compile-time indication that the translation is missing. The same fallback pattern is present in other supported locales (for example `configured` and `invalidUrl` in several generated classes).
In lib/data/model/container/image.dart around line 289, address this finding:
There is no Docker missing-field test, and `DockerImg.fromJson` will fail on a valid row that omits `CreatedAt` or `Tag`: both are passed directly to non-null `DockerImg` constructor fields. `parseContainerImagesOutput` catches that exception and silently drops the image, so a runtime response with an omitted optional field loses an otherwise valid image. The existing malformed-image test only proves that a wholly invalid JSON line is skipped, and the missing numeric-field test covers Podman only.
In lib/data/model/container/status.dart around line 63, address this finding:
Container status labels are not localized for five enum states: `created`, `paused`, `restarting`, `removing`, and `dead` always display English text. When a non-English locale is selected and a container is in one of these states, the container page mixes English status labels into the localized UI, violating the localization contract.
In lib/data/model/container/image.dart around line 279, address this finding:
A Docker image row with a missing/non-string `CreatedAt` field is not treated as a malformed row that can be skipped while preserving other rows; `DockerImg.fromJson` passes `json['CreatedAt']` directly into the non-nullable `String createdAt` constructor parameter. In legacy/custom Docker formats where `CreatedAt` is absent or numeric, this throws and `parseContainerImagesOutput` skips that row (or a whole JSON-array parse can fail), so otherwise usable image metadata disappears. This is false only if all supported Docker image JSON formats always provide a string `CreatedAt`.
In test/container_resource_views_test.dart around line 711, address this finding:
The prune-option widget tests do not cover all required system-prune combinations: they never assert the volumes-only state/preview (`docker system prune --volumes -f` or Podman equivalent). A regression that drops `--volumes` when `-a` is false would pass the added tests, despite the obligation requiring exact previews for all combinations.
In test/container_resource_views_test.dart around line 724, address this finding:
The system-prune widget test covers default, `-a`, and `-a --volumes`, but never selects `--volumes` alone, so the UI state transition and exact preview for that fourth combination are unverified. The provider builder has a distinct branch for this case (`system prune --volumes -f`), and the production dialog forwards `includeVolumes` independently; a callback/state wiring regression could therefore pass the current widget test while running the wrong command.
In lib/l10n/app_fr.arb around line 109, address this finding:
The French `includeUnusedVolumesTip` uses awkward/reversed wording (`volumes utilisés par aucun conteneur`), which can be read as volumes that are used by no container only with effort and is materially less clear than the source's explicit unused-volume condition. This is shown directly beneath the `--volumes` prune switch, where ambiguity affects the user's understanding of which volumes will be deleted.
In test/container_resource_views_test.dart, address this finding:
Unknown image-usage and Podman selection are tested separately but not together: the unknown-count test uses the default Docker helper, while the only Podman prune preview test covers system prune. There is no assertion for a Podman image-prune UI showing `Unused tagged: Unknown` and previews `podman image prune -f`/`podman image prune -a -f`; a runtime-specific wiring regression in that required combination would pass.
## Previously reported and still present (1)
In lib/data/model/container/image.dart around line 77, address this finding:
Digest-qualified container image references are incorrectly treated as unrelated to the image row when the runtime reports an unknown container count. `_addRuntimeImageReference` removes everything after `@` before checking for a `sha256:` ID, so a reference such as `registry.example/app@sha256:<image-id>` contributes only a repository marker and cannot match the image's ID marker. `countUnusedTaggedImages` then returns `null` (unknown) or may classify the tagged image as unresolved instead of confirming it is in use. This is false if the runtime never supplies digest-qualified `Image` references to this helper or always supplies a numeric Containers count.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 4 of 4 areas reviewed
| message: 'Another container operation is already running', | ||
| ); | ||
| } | ||
| state = state.copyWith(runLog: ''); |
There was a problem hiding this comment.
🔀 Concurrency | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/provider/container.dart, address this finding:
Container actions do not participate in the refresh busy gate. `run()` sets only `runLog`, while `refresh()` rejects work only when `state.isBusy` is true; therefore an auto-refresh (or tab refresh) can start while a start/stop/rm/prune command is still executing. The refresh can read partially changing runtime state and, more importantly, action completion then starts another refresh after clearing `runLog`, allowing overlapping remote commands and inconsistent state/error updates. This is false only if every caller guarantees that no refresh can occur during `run()` (including the periodic auto-refresh), which the provider's public API and timer caller do not enforce.
| label: 'CPU', | ||
| child: PercentCircle( | ||
| percent: data.cpuPercent!, | ||
| centerText: '${data.cpuPercent!.toStringAsFixed(1)}%', |
There was a problem hiding this comment.
🔍 Data Integrity | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/resource_views.dart, address this finding:
PercentCircle can render a chart clamped to 99.9 while displaying an unclamped value above 100%, so the visual percentage and text disagree for large/malformed-but-parseable CPU or memory values.
| expect(tester.takeException(), isNull); | ||
| }); | ||
|
|
||
| testWidgets('summary refresh actions follow prune and are tappable', ( |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In test/container_resource_views_test.dart, address this finding:
The widget suite does not assert the empty-state obligation for either resource list. `ContainerItemsView` and `ContainerImagesView` each render a distinct `_EmptyResourceCard` when their input is empty (and the container page can additionally provide a custom `emptyState`), yet the only empty-list test is `summary refresh actions follow prune and are tappable`, which checks actions and never checks that the empty card/message is present or that rows are absent. A regression that drops the empty card, shows the wrong icon/message, or accidentally builds a row would pass this suite. This is disproven if another test outside this file instantiates both views with empty data and asserts their empty card/message and no rows.
| final containerImages = _containerState.items?.map((item) => item.image); | ||
| final unusedTaggedCount = containerImages == null | ||
| ? null | ||
| : countUnusedTaggedImages(images, containerImages); |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/actions.dart, address this finding:
The image prune dialog reports `Unused tagged: Unknown` whenever the container list has not been loaded, even when every image has a known `Containers` count. `_showImagePruneDialog` passes `null` directly in that case instead of calling `countUnusedTaggedImages(images, const [])`, whose documented behavior can count all known zero-reference tagged images without container rows.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| : countUnusedTaggedImages(images, containerImages); | |
| final unusedTaggedCount = countUnusedTaggedImages( | |
| images, | |
| containerImages ?? const [], | |
| ); |
| ); | ||
| } | ||
|
|
||
| Widget imagePruneOptions({ |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In test/container_resource_views_test.dart, address this finding:
The added widget coverage does not exercise the production prune dialogs or notifier wiring: both test helpers construct the option views directly and synthesize the preview from a test `type` argument. Consequently, a regression in `_runtimePruneCommand`, the page's `allUnused`/volume callback capture, or the actual Docker-vs-Podman `ContainerState.type` selection would leave all current tests green. The tests also omit the Podman image-prune UI and toggling either option back to its default, so they do not cover every UI combination requested by the feature.
| "pruneUnusedImages": "清理未使用镜像", | ||
| "pruneDanglingImages": "清理悬空镜像", | ||
| "pruneImages": "清理镜像", | ||
| "unusedTaggedImages": "未使用标记", |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/l10n/app_zh.arb, address this finding:
The Simplified Chinese label for `unusedTaggedImages` is `未使用标记` (unused tags), which omits the image/resource subject and can mislead users about what the prune count represents. This label is used directly for the unused-tagged-image count badge in the image prune UI, so users may interpret the count as tags rather than images.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| "unusedTaggedImages": "未使用标记", | |
| "unusedTaggedImages": "未使用的已标记镜像", |
| }; | ||
|
|
||
| final needSudo = await sudoCompleter.future; | ||
| final sudo = sudoCompleter; |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: unknown
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/provider/container.dart, address this finding:
`sudoCompleter` is reused for both container and image refreshes, but `_requiresSudo` probes only the first requested target (`ps` or `images`). If that target succeeds without sudo while the other target requires sudo (or the probe is against a target-specific permission boundary), subsequent refreshes/actions reuse the wrong `false` result and execute without sudo, leaving that tab empty/erroring. The cache must be keyed by runtime/target or invalidated when the target changes.
There was a problem hiding this comment.
Actionable comments posted: 6
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
🔎 Confirmed findings (6)
- 🟡 Medium A refresh deferred while an operation is running is stored only as a target, and its auto/manual mode is discarded; when the operation finishes,
run()callsrefresh(refreshTarget)without draining the pending target, while normal refresh completion drains it with the defaultisAuto: false. Thus an auto-refresh queued behind a busy operation can either remain permanently stale (if the operation has no successful refresh) or later prompt for sudo as if it were manual. (inline) - 🟡 Medium Negative and out-of-range resource statistics are displayed as valid percentages instead of being rejected or safely represented. The regexes search for an unsigned numeric substring, so
-5%parses as5%; similarly-1 MiB / 2 GiBparses as a positive value. A valid ratio above 100% is also passed toPercentCircle, which clamps only the chart while the center text still shows the unclamped value (for example, a 150% memory label on a 99.9% chart). (inline) - 🟡 Medium The newly added container messages are not present in the Korean (and likewise nl/pt/ru/tr/uk) ARB catalogs, so their generated getters silently return English instead of the selected locale's language. For example, Korean users will see English for both a malformed container-response error and the operation-lock error. (inline)
- 🟡 Medium The newly added container-operation messages are not actually localized for the requested non-English locales: the generated German implementation returns the English source text for both getters, and the corresponding app_de.arb has no entries for these keys. The same missing-ARB/fallback pattern is present in es, fr, id, it, and ja. Consequently, when a container response has the wrong segment count or an operation is already running, users in those locales see English instead of the selected locale. (inline)
- 🟡 Medium Docker paused containers are classified as running whenever Docker emits its normal human-readable paused status, such as
Up 5 minutes (Paused).fromDockerStatechecksstartsWith('up')before checkingcontains('paused'), so the paused state never reaches the paused branch; this exposes stop/restart/terminal actions and marks it active instead of preserving the intended paused status. The claim would be false if Docker never includes(Paused)in the STATUS field supplied by the configured ps format. (inline) - 🟡 Medium A digest-pinned container image reference is normalized to
repository:latestand loses its digest identity._addRuntimeImageReferencestrips everything after@before matching, soregistry.example/team/api@sha256:<digest>marks only the repository's implicitlatestmarker; it neither matches an image whose tag isstablenor the corresponding image ID. With an unknownContainerscount this can report a genuinely used tagged image as unresolved/unused instead of confirming usage. The claim would be false if container.Imageis never emitted with digest-pinned references. (inline)
📋 Additional findings from this change (not shown inline) (14)
- 🟠 High When a configured remote Docker/Podman host requires sudo, the operation can execute against the wrong runtime endpoint because the host variable is exported outside the sudo boundary. (lib/data/provider/container.dart) — anchor-outside-diff
- 🟠 High A configured remote Docker host can be silently ignored whenever the Docker command needs sudo, causing the privileged operation to target the local daemon instead of the configured host.
_wrapemitsexport DOCKER_HOST=... && ..., but_buildSudoCmdthen runs the command throughsudo -S; on normal sudo configurationsenv_resetdrops DOCKER_HOST (and CONTAINER_HOST), and the variable is not passed withsudo -E/sudo envor exported inside the privileged shell. Thus a refresh/action against a remote socket can show or mutate the wrong Docker installation, potentially including destructive prune/remove operations. (lib/data/provider/container.dart) — anchor-outside-diff - 🟡 Medium Volume-only prune and failed operations do not drain a refresh target that was queued while the operation was busy.
pruneVolumes()intentionally passesrefreshTarget: null, andrun()returns on execution failure or nonzero exit without calling the pending-refresh drain, so a user-triggered refresh during the operation can be lost and the displayed collection remains stale indefinitely. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium
run()unconditionally publishesstate = state.copyWith(runLog: null)after awaiting SSH execution, unlike its guarded stdout callback. If the provider is disposed while the command is in flight, this late completion writes to the disposed notifier and can then invoke a refresh, violating the disposal/stale-result guard. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Exit code 2 is always reported as an incorrect sudo password, even when the operation did not use sudo. Docker/Podman commands can legitimately return status 2 for invalid usage or unsupported options; those failures are therefore misclassified, invalidate a cached password unnecessarily, and expose the wrong user-facing error. (lib/data/provider/container.dart) — per-file-budget
- 🟡 Medium An automatic refresh that arrives while another refresh/operation is busy is replayed as a manual refresh.
_refreshPendingIfNeededcallsrefresh(target)withoutisAuto: true, so if the replayed target requires sudo, therefreshpath can open the sudo password dialog even though auto-refresh is explicitly supposed to skip sudo-required refreshes. For example, enable auto-refresh on a sudo-only Docker host, start a manual refresh, and let the timer fire before it completes; the queued refresh is then treated as manual and prompts after the first request finishes. This is false if queued auto-refreshes are intentionally allowed to prompt for sudo or if the pending request can never be set by the timer while a refresh/operation is active. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Refresh requests queued during a container operation can be stranded indefinitely after that operation finishes without invoking a matching refresh.
refresh()stores_pendingRefreshTargetwheneverrunLogis non-null, butrun()returns directly on exec exceptions, nonzero exit codes, sudo-password cancellation/failure, and successfulrefreshTarget: nulloperations without clearing or replaying the pending target. Thus, for example, an auto-refresh tick duringvolume prune(or during a failed prune) sets a pending target, and after the operation ends the UI remains stale until a later timer tick/manual refresh; with auto-refresh disabled it may remain stale forever. This is false only if callers guarantee no refresh can arrive during these operations or another independent drain always runs afterward. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium An automatic refresh queued while another refresh/operation is busy is replayed as a manual refresh and can unexpectedly prompt for sudo.
refresh()records only_pendingRefreshTargetwhen busy, discardingisAuto;_refreshPendingIfNeeded()then callsrefresh(target, generation: generation)with the defaultisAuto: false. On a Docker host requiring sudo, an auto-refresh tick during a busy operation therefore triggers_getSudoPassword()as soon as the operation finishes, despite the explicit auto-refresh policy that says sudo-required auto refreshes must be skipped. This is disproven if automatic refreshes can never occur whilestate.isBusyorrunLogis non-null. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Exit code 2 is treated as a bad sudo password even when sudo was not used. Both
refresh()andrun()branch solely oncode == 2, without checkingneedSudo; consequently an ordinary Docker/Podman failure that exits 2 (for example an invalid command/argument or runtime-level CLI error while sudo probing says false) is surfaced ascontainerSudoPasswordIncorrect, clears a potentially valid cached password, and makes the user re-enter credentials. This is disproven only if the supported Docker and Podman CLIs guarantee that every exit-2 result from all commands issued here can only be produced by sudo authentication. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium The edit-host dialog allocates a
TextEditingControllerand never disposes it. Every opening of the settings action creates a new controller (final ctrl = TextEditingController(text: host)), but unlike_showAddFAB, there is noctrl.dispose()aftershowRoundDialogcompletes. Repeated navigation/opening of this dialog leaks controller resources; this would be disproven only if the dialog helper itself takes ownership and disposes supplied controllers, which is not indicated by this call site or the explicit disposal pattern in the adjacent add dialog. (lib/view/page/container/actions.dart) — anchor-outside-diff - 🟡 Medium The new
containerSegmentsMismatchandcontainerOperationInProgressmessages are present in the generated localization contract and every generated locale class, but are absent from the ARB source catalogs for the non-English locales (including German). Consequently, those generated implementations return the English fallback strings rather than translations whenever the container provider reports a malformed response or rejects a concurrent operation. This violates the source/generated/locale compatibility obligation and makes newly introduced user-facing container errors unexpectedly English in supported locales. (lib/l10n/app_de.arb) — anchor-outside-diff - 🟡 Medium Podman 5 stats with top-level
NetInput/NetOutputcounters are reported as zero. For version 5+,parseStatsignores those valid fields and only sumsNetwork.{interface}.RxBytes/TxBytes; when Podman 5 output uses the top-level counters (or a mixed/compatibility output), the displayed network values silently become0 B / 0 B. The claim would be false if all supported Podman 5 stats output is guaranteed to contain only the nested Network map and never top-level counters. (lib/data/model/container/ps.dart) — anchor-outside-diff - 🟡 Medium The logs, terminal, and compose-log actions bypass the provider's sudo/password handling. On a Docker host where the daemon is only accessible via sudo (a supported path detected by _requiresSudo), these UI actions run plain
docker ...over SSH and fail with permission denied even though start/stop/delete work. Route these commands through the same privilege-aware execution path or apply the detected sudo wrapper. (lib/view/page/container/actions.dart) — anchor-outside-diff - 🔵 Low The image Pull action accepts dangling/empty image identities and constructs a remote pull command from them.
_onTapImageMenuonly rejectsrepo == null; for a dangling image whose repository is<none>(or an empty string), it sendspull '<none>:latest'(orpull ':latest') rather than disabling/rejecting the action. This can invoke a different runtime resolution than the displayed dangling identity and makes the action invalid for rows explicitly marked unusable. (lib/view/page/container/actions.dart) — anchor-outside-diff
🤖 Prompt for AI agents — all findings (20)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (6)
In lib/data/provider/container.dart around line 304, address this finding:
A refresh deferred while an operation is running is stored only as a target, and its auto/manual mode is discarded; when the operation finishes, `run()` calls `refresh(refreshTarget)` without draining the pending target, while normal refresh completion drains it with the default `isAuto: false`. Thus an auto-refresh queued behind a busy operation can either remain permanently stale (if the operation has no successful refresh) or later prompt for sudo as if it were manual.
In lib/view/page/container/resource_views.dart around line 1269, address this finding:
Negative and out-of-range resource statistics are displayed as valid percentages instead of being rejected or safely represented. The regexes search for an unsigned numeric substring, so `-5%` parses as `5%`; similarly `-1 MiB / 2 GiB` parses as a positive value. A valid ratio above 100% is also passed to `PercentCircle`, which clamps only the chart while the center text still shows the unclamped value (for example, a 150% memory label on a 99.9% chart).
In lib/generated/l10n/l10n_ko.dart around line 1067, address this finding:
The newly added container messages are not present in the Korean (and likewise nl/pt/ru/tr/uk) ARB catalogs, so their generated getters silently return English instead of the selected locale's language. For example, Korean users will see English for both a malformed container-response error and the operation-lock error.
In lib/generated/l10n/l10n_de.dart around line 1115, address this finding:
The newly added container-operation messages are not actually localized for the requested non-English locales: the generated German implementation returns the English source text for both getters, and the corresponding app_de.arb has no entries for these keys. The same missing-ARB/fallback pattern is present in es, fr, id, it, and ja. Consequently, when a container response has the wrong segment count or an operation is already running, users in those locales see English instead of the selected locale.
In lib/data/model/container/status.dart around line 24, address this finding:
Docker paused containers are classified as running whenever Docker emits its normal human-readable paused status, such as `Up 5 minutes (Paused)`. `fromDockerState` checks `startsWith('up')` before checking `contains('paused')`, so the paused state never reaches the paused branch; this exposes stop/restart/terminal actions and marks it active instead of preserving the intended paused status. The claim would be false if Docker never includes `(Paused)` in the STATUS field supplied by the configured ps format.
In lib/data/model/container/image.dart around line 77, address this finding:
A digest-pinned container image reference is normalized to `repository:latest` and loses its digest identity. `_addRuntimeImageReference` strips everything after `@` before matching, so `registry.example/team/api@sha256:<digest>` marks only the repository's implicit `latest` marker; it neither matches an image whose tag is `stable` nor the corresponding image ID. With an unknown `Containers` count this can report a genuinely used tagged image as unresolved/unused instead of confirming usage. The claim would be false if container `.Image` is never emitted with digest-pinned references.
## Additional findings on this change (not posted inline) (14)
In lib/data/provider/container.dart around line 784, address this finding:
When a configured remote Docker/Podman host requires sudo, the operation can execute against the wrong runtime endpoint because the host variable is exported outside the sudo boundary.
In lib/data/provider/container.dart around line 736, address this finding:
A configured remote Docker host can be silently ignored whenever the Docker command needs sudo, causing the privileged operation to target the local daemon instead of the configured host. `_wrap` emits `export DOCKER_HOST=... && ...`, but `_buildSudoCmd` then runs the command through `sudo -S`; on normal sudo configurations `env_reset` drops DOCKER_HOST (and CONTAINER_HOST), and the variable is not passed with `sudo -E`/`sudo env` or exported inside the privileged shell. Thus a refresh/action against a remote socket can show or mutate the wrong Docker installation, potentially including destructive prune/remove operations.
In lib/data/provider/container.dart around line 766, address this finding:
Volume-only prune and failed operations do not drain a refresh target that was queued while the operation was busy. `pruneVolumes()` intentionally passes `refreshTarget: null`, and `run()` returns on execution failure or nonzero exit without calling the pending-refresh drain, so a user-triggered refresh during the operation can be lost and the displayed collection remains stale indefinitely.
In lib/data/provider/container.dart around line 757, address this finding:
`run()` unconditionally publishes `state = state.copyWith(runLog: null)` after awaiting SSH execution, unlike its guarded stdout callback. If the provider is disposed while the command is in flight, this late completion writes to the disposed notifier and can then invoke a refresh, violating the disposal/stale-result guard.
In lib/data/provider/container.dart around line 759, address this finding:
Exit code 2 is always reported as an incorrect sudo password, even when the operation did not use sudo. Docker/Podman commands can legitimately return status 2 for invalid usage or unsupported options; those failures are therefore misclassified, invalidate a cached password unnecessarily, and expose the wrong user-facing error.
In lib/data/provider/container.dart around line 605, address this finding:
An automatic refresh that arrives while another refresh/operation is busy is replayed as a manual refresh. `_refreshPendingIfNeeded` calls `refresh(target)` without `isAuto: true`, so if the replayed target requires sudo, the `refresh` path can open the sudo password dialog even though auto-refresh is explicitly supposed to skip sudo-required refreshes. For example, enable auto-refresh on a sudo-only Docker host, start a manual refresh, and let the timer fire before it completes; the queued refresh is then treated as manual and prompts after the first request finishes. This is false if queued auto-refreshes are intentionally allowed to prompt for sudo or if the pending request can never be set by the timer while a refresh/operation is active.
In lib/data/provider/container.dart around line 772, address this finding:
Refresh requests queued during a container operation can be stranded indefinitely after that operation finishes without invoking a matching refresh. `refresh()` stores `_pendingRefreshTarget` whenever `runLog` is non-null, but `run()` returns directly on exec exceptions, nonzero exit codes, sudo-password cancellation/failure, and successful `refreshTarget: null` operations without clearing or replaying the pending target. Thus, for example, an auto-refresh tick during `volume prune` (or during a failed prune) sets a pending target, and after the operation ends the UI remains stale until a later timer tick/manual refresh; with auto-refresh disabled it may remain stale forever. This is false only if callers guarantee no refresh can arrive during these operations or another independent drain always runs afterward.
In lib/data/provider/container.dart around line 602, address this finding:
An automatic refresh queued while another refresh/operation is busy is replayed as a manual refresh and can unexpectedly prompt for sudo. `refresh()` records only `_pendingRefreshTarget` when busy, discarding `isAuto`; `_refreshPendingIfNeeded()` then calls `refresh(target, generation: generation)` with the default `isAuto: false`. On a Docker host requiring sudo, an auto-refresh tick during a busy operation therefore triggers `_getSudoPassword()` as soon as the operation finishes, despite the explicit auto-refresh policy that says sudo-required auto refreshes must be skipped. This is disproven if automatic refreshes can never occur while `state.isBusy` or `runLog` is non-null.
In lib/data/provider/container.dart around line 419, address this finding:
Exit code 2 is treated as a bad sudo password even when sudo was not used. Both `refresh()` and `run()` branch solely on `code == 2`, without checking `needSudo`; consequently an ordinary Docker/Podman failure that exits 2 (for example an invalid command/argument or runtime-level CLI error while sudo probing says false) is surfaced as `containerSudoPasswordIncorrect`, clears a potentially valid cached password, and makes the user re-enter credentials. This is disproven only if the supported Docker and Podman CLIs guarantee that every exit-2 result from all commands issued here can only be produced by sudo authentication.
In lib/view/page/container/actions.dart around line 203, address this finding:
The edit-host dialog allocates a `TextEditingController` and never disposes it. Every opening of the settings action creates a new controller (`final ctrl = TextEditingController(text: host)`), but unlike `_showAddFAB`, there is no `ctrl.dispose()` after `showRoundDialog` completes. Repeated navigation/opening of this dialog leaks controller resources; this would be disproven only if the dialog helper itself takes ownership and disposes supplied controllers, which is not indicated by this call site or the explicit disposal pattern in the adjacent add dialog.
In lib/l10n/app_de.arb around line 237, address this finding:
The new `containerSegmentsMismatch` and `containerOperationInProgress` messages are present in the generated localization contract and every generated locale class, but are absent from the ARB source catalogs for the non-English locales (including German). Consequently, those generated implementations return the English fallback strings rather than translations whenever the container provider reports a malformed response or rejects a concurrent operation. This violates the source/generated/locale compatibility obligation and makes newly introduced user-facing container errors unexpectedly English in supported locales.
In lib/data/model/container/ps.dart around line 91, address this finding:
Podman 5 stats with top-level `NetInput`/`NetOutput` counters are reported as zero. For version 5+, `parseStats` ignores those valid fields and only sums `Network.{interface}.RxBytes/TxBytes`; when Podman 5 output uses the top-level counters (or a mixed/compatibility output), the displayed network values silently become `0 B / 0 B`. The claim would be false if all supported Podman 5 stats output is guaranteed to contain only the nested Network map and never top-level counters.
In lib/view/page/container/actions.dart around line 363, address this finding:
The logs, terminal, and compose-log actions bypass the provider's sudo/password handling. On a Docker host where the daemon is only accessible via sudo (a supported path detected by _requiresSudo), these UI actions run plain `docker ...` over SSH and fail with permission denied even though start/stop/delete work. Route these commands through the same privilege-aware execution path or apply the detected sudo wrapper.
In lib/view/page/container/actions.dart around line 256, address this finding:
The image Pull action accepts dangling/empty image identities and constructs a remote pull command from them. `_onTapImageMenu` only rejects `repo == null`; for a dangling image whose repository is `<none>` (or an empty string), it sends `pull '<none>:latest'` (or `pull ':latest'`) rather than disabling/rejecting the action. This can invoke a different runtime resolution than the displayed dangling identity and makes the action invalid for rows explicitly marked unusable.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 4 of 4 areas reviewed
| bool isAuto = false, | ||
| int? generation, | ||
| }) async { | ||
| if (state.isBusy || state.runLog != null) { |
There was a problem hiding this comment.
🔀 Concurrency | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/provider/container.dart, address this finding:
A refresh deferred while an operation is running is stored only as a target, and its auto/manual mode is discarded; when the operation finishes, `run()` calls `refresh(refreshTarget)` without draining the pending target, while normal refresh completion drains it with the default `isAuto: false`. Thus an auto-refresh queued behind a busy operation can either remain permanently stale (if the operation has no successful refresh) or later prompt for sudo as if it were manual.
| return raw == null || raw.isEmpty ? item.status.displayName : raw; | ||
| } | ||
|
|
||
| double? _parsePercent(String? raw) { |
There was a problem hiding this comment.
🔍 Data Integrity | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/resource_views.dart, address this finding:
Negative and out-of-range resource statistics are displayed as valid percentages instead of being rejected or safely represented. The regexes search for an unsigned numeric substring, so `-5%` parses as `5%`; similarly `-1 MiB / 2 GiB` parses as a positive value. A valid ratio above 100% is also passed to `PercentCircle`, which clamps only the chart while the center text still shows the unclamped value (for example, a 150% memory label on a 99.9% chart).
| String get tmuxNotAvailable => 'tmux is not available'; | ||
|
|
||
| @override | ||
| String containerSegmentsMismatch(int count) { |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/generated/l10n/l10n_ko.dart, address this finding:
The newly added container messages are not present in the Korean (and likewise nl/pt/ru/tr/uk) ARB catalogs, so their generated getters silently return English instead of the selected locale's language. For example, Korean users will see English for both a malformed container-response error and the operation-lock error.
| String get tmuxNotAvailable => 'tmux is not available'; | ||
|
|
||
| @override | ||
| String containerSegmentsMismatch(int count) { |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/generated/l10n/l10n_de.dart, address this finding:
The newly added container-operation messages are not actually localized for the requested non-English locales: the generated German implementation returns the English source text for both getters, and the corresponding app_de.arb has no entries for these keys. The same missing-ARB/fallback pattern is present in es, fr, id, it, and ja. Consequently, when a container response has the wrong segment count or an operation is already running, users in those locales see English instead of the selected locale.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/container_resource_views_test.dart (1)
347-357: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse parser-valid, isolated fixtures for negative statistics.
The valid network and disk fixtures use
value / valuestrings. This test uses↓ -1 MiB / ↑ 2 MiBandRead -1 MiB / Write 2 MiB. IfDockerPsorPodmanPsparsing does not support these prefixes, the test covers unparseable input instead of negative-value rejection. Confirm the raw output contract and test each statistic with the other statistics valid.🤖 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 `@test/container_resource_views_test.dart` around lines 347 - 357, Update the negative-statistics fixture in the test named “negative container stats are omitted” to use parser-valid raw formats, matching the established value/value contract for network and disk statistics. Isolate each negative CPU, memory, network, and disk case by keeping the other statistics valid, so the test verifies rejection of negative values rather than unparseable prefixes.
🤖 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.
Nitpick comments:
In `@test/container_resource_views_test.dart`:
- Around line 347-357: Update the negative-statistics fixture in the test named
“negative container stats are omitted” to use parser-valid raw formats, matching
the established value/value contract for network and disk statistics. Isolate
each negative CPU, memory, network, and disk case by keeping the other
statistics valid, so the test verifies rejection of negative values rather than
unparseable prefixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8355daef-ec90-419a-8506-86ed08ced170
⛔ Files ignored due to path filters (12)
lib/generated/l10n/l10n_de.dartis excluded by!**/generated/**lib/generated/l10n/l10n_es.dartis excluded by!**/generated/**lib/generated/l10n/l10n_fr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_id.dartis excluded by!**/generated/**lib/generated/l10n/l10n_it.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ja.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ko.dartis excluded by!**/generated/**lib/generated/l10n/l10n_nl.dartis excluded by!**/generated/**lib/generated/l10n/l10n_pt.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ru.dartis excluded by!**/generated/**lib/generated/l10n/l10n_tr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_uk.dartis excluded by!**/generated/**
📒 Files selected for processing (22)
lib/data/model/app/menu/container.dartlib/data/model/container/image.dartlib/data/model/container/ps.dartlib/data/model/container/status.dartlib/data/provider/container.dartlib/l10n/app_de.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_id.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_ko.arblib/l10n/app_nl.arblib/l10n/app_pt.arblib/l10n/app_ru.arblib/l10n/app_tr.arblib/l10n/app_uk.arblib/view/page/container/actions.dartlib/view/page/container/container.dartlib/view/page/container/resource_views.darttest/container_resource_views_test.darttest/container_test.dart
🚧 Files skipped from review as they are similar to previous changes (19)
- lib/l10n/app_tr.arb
- lib/l10n/app_es.arb
- lib/l10n/app_ko.arb
- lib/l10n/app_ru.arb
- lib/l10n/app_nl.arb
- lib/l10n/app_uk.arb
- lib/l10n/app_it.arb
- lib/l10n/app_de.arb
- lib/l10n/app_pt.arb
- lib/l10n/app_fr.arb
- lib/data/model/app/menu/container.dart
- lib/l10n/app_id.arb
- lib/data/model/container/ps.dart
- lib/data/model/container/status.dart
- test/container_test.dart
- lib/data/model/container/image.dart
- lib/view/page/container/actions.dart
- lib/data/provider/container.dart
- lib/view/page/container/resource_views.dart
There was a problem hiding this comment.
Actionable comments posted: 5
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
🔎 Confirmed findings (5)
- 🟡 Medium
parseContainerImagesOutputdoes not honor its row-level malformed-row recovery for JSON-array output: it decodes and casts the entire array before entering thetryloop. A truncated or otherwise malformed array therefore throws out of the function and discards/aborts parsing instead of skipping the bad row while retaining valid rows, unlike newline-delimited output. (inline) - 🟡 Medium Paused containers are parsed as
paused, but the menu maps paused to the stopped branch and offersstart. Docker/Podman start is not the operation that resumes a paused container (unpause is required), so the displayed lifecycle action cannot transition a paused container and may produce an operation error; this violates the intended action-menu contract for paused state. (inline) - 🟡 Medium Container ordering is not deterministic within a compose or standalone group:
_groupContainerssorts only the group keys and appends each item in the input order. Since provider refreshes can return rows in a different order, the same model set can render different row ordering (and the index-based row keys change), undermining the scope's deterministic-ordering invariant. (inline) - 🟡 Medium A malformed metric pair with a valid first value and invalid/negative second value is rendered as a valid one-sided metric instead of being omitted.
_parseMetricPaironly rejects whenfirstis null and silently storessecondas null; for examplenet = '12 MB / garbage'produces a NET module showing only download, anddisk = '1 GB / -2 MB'shows only read. This violates the stated omission of invalid metrics and can present incomplete resource data as authoritative. (inline) - 🟡 Medium Byte parsing accepts trailing garbage because
_parseByteSize's regex is not anchored at the end. Values such asmem = '640 MiB junk / 2 GiB'are parsed as 640 MiB and displayed as a percentage, even though the metric is malformed; the same applies to negative/invalid suffix cases where a valid prefix is followed by non-metric text. This conflicts with omitting invalid metrics. (inline)
📋 Additional findings from this change (not shown inline) (3)
- 🟡 Medium Docker's human-readable removal status is commonly
Removal In Progress, butfromDockerStateonly recognizes strings containingremoving. Such a container is classified asunknown, andContainerMenu.itemsthen exposesstart, offering an invalid lifecycle action rather than treating it as removing/non-startable. (lib/data/model/container/status.dart) — anchor-outside-diff - 🟡 Medium The selected working directory for merged compose logs is not deterministic when multiple directories tie.
_mostCommonWorkingDirreduces over the input-order map and keeps the first entry on equal counts (a.value >= b.value), while the provider does not establish a stable container ordering. A refresh/reordering can therefore make the same compose group launch logs from different directories. (lib/view/page/container/container.dart) — anchor-outside-diff - 🟡 Medium CPU percentage parsing also accepts malformed trailing text:
_parsePercentmatches a valid prefix such as12.5% garbagebecause its regexp is only anchored at the start. The resource UI then renders a PercentCircle for a value that is not a valid provider metric, rather than omitting the invalid metric as required. (lib/view/page/container/resource_views.dart) — anchor-unreliable
♻️ Previously reported (still present) (1)
- 🟡 Medium The locale ARB catalogs do not retain the base catalog's key set:
app_zh_tw.arbhas noinvalidUrlentry even thoughapp_en.arbdefines it and the generated Traditional Chinese implementation exposesinvalidUrlas the English fallback. Consequently, users selecting zh-TW see English for this source message rather than a catalog translation, and the source catalogs fail the scope's same-key-set contract (the generated API remains compilable only because Flutter permits fallback). This is proven false ifinvalidUrlis intentionally excluded from the supported source contract or is added toapp_zh_tw.arband regenerated. (lib/l10n/app_zh_tw.arb) — previously-reported
🤖 Prompt for AI agents — all findings (9)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (5)
In lib/data/provider/container.dart around line 109, address this finding:
`parseContainerImagesOutput` does not honor its row-level malformed-row recovery for JSON-array output: it decodes and casts the entire array before entering the `try` loop. A truncated or otherwise malformed array therefore throws out of the function and discards/aborts parsing instead of skipping the bad row while retaining valid rows, unlike newline-delimited output.
In lib/data/model/app/menu/container.dart around line 23, address this finding:
Paused containers are parsed as `paused`, but the menu maps paused to the stopped branch and offers `start`. Docker/Podman start is not the operation that resumes a paused container (unpause is required), so the displayed lifecycle action cannot transition a paused container and may produce an operation error; this violates the intended action-menu contract for paused state.
In lib/view/page/container/resource_views.dart around line 641, address this finding:
Container ordering is not deterministic within a compose or standalone group: `_groupContainers` sorts only the group keys and appends each item in the input order. Since provider refreshes can return rows in a different order, the same model set can render different row ordering (and the index-based row keys change), undermining the scope's deterministic-ordering invariant.
In lib/view/page/container/resource_views.dart around line 1320, address this finding:
A malformed metric pair with a valid first value and invalid/negative second value is rendered as a valid one-sided metric instead of being omitted. `_parseMetricPair` only rejects when `first` is null and silently stores `second` as null; for example `net = '12 MB / garbage'` produces a NET module showing only download, and `disk = '1 GB / -2 MB'` shows only read. This violates the stated omission of invalid metrics and can present incomplete resource data as authoritative.
In lib/view/page/container/resource_views.dart around line 1290, address this finding:
Byte parsing accepts trailing garbage because `_parseByteSize`'s regex is not anchored at the end. Values such as `mem = '640 MiB junk / 2 GiB'` are parsed as 640 MiB and displayed as a percentage, even though the metric is malformed; the same applies to negative/invalid suffix cases where a valid prefix is followed by non-metric text. This conflicts with omitting invalid metrics.
## Additional findings on this change (not posted inline) (3)
In lib/data/model/container/status.dart around line 28, address this finding:
Docker's human-readable removal status is commonly `Removal In Progress`, but `fromDockerState` only recognizes strings containing `removing`. Such a container is classified as `unknown`, and `ContainerMenu.items` then exposes `start`, offering an invalid lifecycle action rather than treating it as removing/non-startable.
In lib/view/page/container/container.dart around line 466, address this finding:
The selected working directory for merged compose logs is not deterministic when multiple directories tie. `_mostCommonWorkingDir` reduces over the input-order map and keeps the first entry on equal counts (`a.value >= b.value`), while the provider does not establish a stable container ordering. A refresh/reordering can therefore make the same compose group launch logs from different directories.
In lib/view/page/container/resource_views.dart, address this finding:
CPU percentage parsing also accepts malformed trailing text: `_parsePercent` matches a valid prefix such as `12.5% garbage` because its regexp is only anchored at the start. The resource UI then renders a PercentCircle for a value that is not a valid provider metric, rather than omitting the invalid metric as required.
## Previously reported and still present (1)
In lib/l10n/app_zh_tw.arb around line 131, address this finding:
The locale ARB catalogs do not retain the base catalog's key set: `app_zh_tw.arb` has no `invalidUrl` entry even though `app_en.arb` defines it and the generated Traditional Chinese implementation exposes `invalidUrl` as the English fallback. Consequently, users selecting zh-TW see English for this source message rather than a catalog translation, and the source catalogs fail the scope's same-key-set contract (the generated API remains compilable only because Flutter permits fallback). This is proven false if `invalidUrl` is intentionally excluded from the supported source contract or is added to `app_zh_tw.arb` and regenerated.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 4 of 4 areas reviewed
| List<_ContainerGroup> _groupContainers(List<ContainerPs> items) { | ||
| final grouped = <String?, List<ContainerPs>>{}; | ||
| for (final item in items) { | ||
| grouped.putIfAbsent(item.project, () => []).add(item); |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/resource_views.dart, address this finding:
Container ordering is not deterministic within a compose or standalone group: `_groupContainers` sorts only the group keys and appends each item in the input order. Since provider refreshes can return rows in a different order, the same model set can render different row ordering (and the index-based row keys change), undermining the scope's deterministic-ordering invariant.
GT-610
dismissed stale reviews from winnowl[bot] and coderabbitai[bot]
via
August 8, 2026 16:16
00a3c39
There was a problem hiding this comment.
Actionable comments posted: 8
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
🔎 Confirmed findings (8)
- 🟡 Medium Podman status text is not authoritative when it is present but unrecognized:
fromPodmanfalls back to the legacyExitedflag for anyrawStatusthat parses asunknown. For example, a row withrawStatus: 'Paused (health: starting)'or a newer Podman lifecycle string andExited: falseis classified as running, exposing stop/restart/terminal actions instead of the safe unknown-state policy. The fallback should be limited to absent/empty status text (or otherwise preserve unknown). This is disproven if all supported non-empty Podman status strings are guaranteed to be among the Docker parser's exact substring patterns. (inline) - 🟡 Medium Unknown containers are startable in the per-container menu but are omitted from group start eligibility and execution.
ContainerMenu.items(unknown)returns[start, rm, logs], while the group menu is shown only whenanyStoppedis true and_onTapGroupMenucollects onlyisStoppedIDs; sinceunknown.isStoppedis false, an unknown-only group has no Start action and mixed groups silently exclude its unknown IDs. This is disproven if unknown containers are intentionally excluded from bulk start despite being individually startable; in that case the per-container policy must also remove Start. (inline) - 🟡 Medium The parser accepts rows with no usable container ID (
Idmissing, null, or empty) and still createsPodmanPsentries. These phantom entries can appear in the UI with status-derived actions, while bulk action extraction drops their null IDs, producing inconsistent menus and counts. A malformed Podman row should be skipped, just like malformed JSON rows. This is disproven if every runtime output is guaranteed to contain a non-emptyIdand the parser is not expected to defend against valid JSON rows missing required fields. (inline) - 🟡 Medium The stats matcher can assign the same stats row to multiple containers when IDs share the first 12 characters.
findContainerStatsRowaccepts any pair of IDs of length at least 12 when either starts with the other, so if two containers have IDs0123456789abcdef...Aand0123456789abcdef...B, both lookups can match the first row's 12-character prefix only when the supplied ID itself is shortened; more importantly, the function does not reject ambiguous prefixes and returns the first match. A short runtime/container ID of exactly 12 characters can therefore select an arbitrary row among multiple full IDs with that prefix. The claim would be false only if callers always provide full IDs and runtime output never contains ambiguous prefixes. (inline) - 🟡 Medium Overlapping refresh requests are not queued losslessly:
_pendingRefreshstores only one target, and a request for the other target overwrites the earlier pending request. For example, while a container refresh is busy, callrefreshImages()and thenrefreshContainers(); the image refresh is discarded and only the latter container refresh runs. This violates the obligation to serialize or queue overlapping refreshes correctly and can leave the image view stale after operations. The claim would be false only if callers guarantee that overlapping requests always have the same target or independently refresh every discarded target. (inline) - 🟡 Medium Two containers whose IDs share the first 12 characters can receive the same statistics.
docker psis deliberately requested with{{.ID}}, which is normally a 12-character short ID, whiledocker statscan return longer IDs;findContainerStatsRowtreats either value as a match whenever both are at least 12 characters and returns the first match. If two live containers share that 12-character prefix, the loop applies the first stats row to both containers, so CPU/memory/network/disk values are displayed for the wrong container. (inline) - 🟡 Medium Image rows are not sorted before being rendered, so the order (and index-based row keys) follows the runtime's raw
image lsoutput. Docker/Podman can return the same images in different orders across refreshes, causing visible row movement and unstableimage-row-*-$index-$ididentity despite the scope requiring stable sorting and collision-resistant presentation. (inline) - 🟡 Medium Unknown image usage matching does not normalize Docker Hub short names on the container-reference side. An image row for
docker.io/library/alpine:latestgets markers for both the fully qualified and short aliases, but a running container reported asalpine:latestis processed only asref:alpine:latest; this particular pair matches, while digest/repository forms with adocker.ioprefix or equivalent short-name variants can fail to match and causeunusedTaggedCountto become Unknown or misclassify usage. (inline)
📋 Additional findings from this change (not shown inline) (26)
- 🟡 Medium
PodmanPs.fromJsonassumesNamesis either null or an iterable of strings and callsList<String>.from(json['Names']!.map((x) => x)). A malformed-but-JSON-valid row such as{"Names":"worker"}or{"Names":[null]}throws duringPodmanPs.fromJson;parsePodmanPsOutputcatches onlyFormatExceptionandTypeError, so this can escape and abort parsing the entire listing rather than skipping that malformed row while retaining valid rows. This is disproven if the Podman command contract guaranteesNamesis always a non-null list of strings for every row, including older/compatibility output. (lib/data/model/container/ps.dart) — anchor-outside-diff - 🟡 Medium Unknown-status containers can be started individually but cannot be started through their compose/group menu.
ContainerMenu.itemsexplicitly includesstartforContainerStatus.unknown, while_buildGroupMoreBtnsetsanyStoppedonly fromstatus.isStoppedand_onTapGroupMenubuildsstoppedIdsonly fromstatus.isStopped;isStoppedintentionally excludesunknown. Therefore a group containing only unknown containers has no Start menu item, and a mixed group silently omits those IDs from the bulk start command. This is false only if unknown containers are intentionally forbidden from group actions despite being offered Start individually. (lib/view/page/container/container.dart) — anchor-outside-diff - 🟡 Medium Malformed container rows with no usable ID are accepted as real containers instead of being skipped. For example, Docker output
\tExited (0)\tname\timagepasses theparts.length >= 4check and creates aDockerPswhoseidis empty; similarly, Podman output{}is accepted byPodmanPs.fromJson. These phantom rows can appear in the container list and cannot be targeted by action or matched to stats, violating the parser's malformed-row handling contract. This would be false only if every runtime is guaranteed to emit a non-empty ID and callers independently filter empty/null IDs before displaying or acting on parsed items. (lib/data/model/container/ps.dart) — anchor-outside-diff - 🟡 Medium Malformed Podman numeric fields are converted to zero rather than treated as invalid/unknown.
_asIntusesint.tryParse(...) ?? 0, so a stats row such as{"MemUsage":"not-a-number", "NetInput":"bad"}produces apparently valid0 Bmetrics (and similarly corrupts individual counters) instead of isolating the malformed metric/row. This violates the requirement that numeric variation and malformed rows not poison parsed statistics. The claim would be false only if Podman guarantees these fields are always numeric or the zero display is explicitly the required representation for invalid values. (lib/data/model/container/ps.dart) — per-file-budget - 🟡 Medium Podman 5 network parsing discards valid legacy counters when the nested Network map contains any counter key, even if that interface's value is malformed or only one direction is present.
hasNestedNetworkCountersbecomes true forRxBytes/TxByteskey presence, then_asIntturns missing/invalid values into zero and the code never falls back to top-levelNetInput/NetOutput. For example, a Podman 5 row withNetwork: {eth0: {RxBytes: "bad"}}and valid top-level counters displays0 B / 0 Bor partial data instead of the valid counters. The claim would be false only if Podman guarantees nested counters are always complete and valid whenever either key appears. (lib/data/model/container/ps.dart) — per-file-budget - 🟡 Medium Docker stats parsing mutates the container metrics before validating later fields, so a malformed row can leave partially updated statistics visible.
cpuandmemare assigned beforeNetIO/BlockIOare cast asString; if either field is a numeric JSON value or otherwise malformed, a TypeError is caught by the caller, but the new CPU/memory values remain on the object while net/disk retain old values. This violates malformed-row isolation. The claim would be false only if Docker always emits all four fields as strings and the parser is never invoked with malformed or version-variant rows. (lib/data/model/container/ps.dart) — per-file-budget - 🟡 Medium Digest-pinned container references using the canonical Docker Hub name do not match images listed under Docker's usual short repository name.
_addDigestMarkersonly adds the unqualified alias when the image repository starts withdocker.io/library/; for an image parsed as repositorynginxwith digestsha256:..., a running container reported asdocker.io/library/nginx@sha256:...produces markerdigest:docker.io/library/nginx@..., while the image produces onlydigest:nginx@.... ConsequentlycountUnusedTaggedImagesreturnsnull(unknown) even though the container reference definitively proves the image is used, making the unused-image count incorrect. This would be disproven if the runtime always emits the same repository spelling in both image-list and ps output, or if Docker image-list output is guaranteed to canonicalizenginxtodocker.io/library/nginx. (lib/data/model/container/image.dart) — anchor-unreliable - 🟡 Medium Malformed or missing Podman numeric fields are converted to plausible zero values instead of isolating the malformed metric/row. For example, a stats row with
{"CPU":"garbage","AvgCPU":"garbage","NetInput":"garbage","NetOutput":"garbage"}is accepted byparseStats:_asDouble/_asIntreturn 0, producing0.0% / Avg 0.0%and↓ 0 B / ↑ 0 B. The provider catches exceptions per row, but these invalid values do not throw, so the UI reports zero usage rather than omitting the bad metric or row, masking a malformed Docker/Podman response. (lib/data/model/container/ps.dart) — per-file-budget - 🟡 Medium
resetSudoProbeis not a complete cancellation barrier for an in-flight refresh: it replaces the sudo completer and increments the generation, but an already-runningclient.execWithPwdis left alive. Because it also setsisBusyfalse (via callers such as host editing), another refresh may begin and execute concurrently with the old remote command. The old command's output is only discarded after it returns, so generation prevents stale state publication but does not preserve the single-command-at-a-time invariant or actually cancel work. This is disproven only if the notifier can guarantee that reset is never called whilerefreshhas passed the probe and is executing; the provider API currently has no such guard. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium The queue stores only one
(target, isAuto)entry, and_queueRefreshreplaces it whenever a later request has a different target. Thus a refresh request for the other resource is silently dropped. For example, while a container refresh is busy, an images refresh queuesimages; a subsequent containers refresh changes_pendingRefreshtocontainers, so_refreshPendingIfNeededruns only containers and images are never refreshed. This is false only if the intended contract is explicitly “latest refresh request wins” and callers never rely on both resource targets being refreshed; the provider exposes independentrefreshContainers/refreshImagesAPIs and UI can generate both requests. (lib/data/provider/container.dart) — per-file-budget - 🟡 Medium Negative image reference counts are treated as valid data and can be rendered as non-unused. Docker's
containersCountusesint.tryParsewithout rejecting values below zero; forContainers: '-1',isUnusedis false and the image is not marked unknown, so prune summaries can assert a misleading resource state. (lib/data/model/container/image.dart) — anchor-outside-diff - 🟡 Medium Negative image sizes are accepted and rendered directly.
DockerImg.sizeMBreturns the raw Docker size string without validating sign, so a provider row such asSize: '-2 MB'produces a visible negative size rather than omitting malformed data. (lib/data/model/container/image.dart) — anchor-outside-diff - 🟡 Medium Container summary badges omit valid lifecycle states such as paused, restarting, removing, created, and dead. The summary computes only running, stopped (which itself excludes paused/restarting/removing), and unknown, so a page containing only paused containers displays
0 Runningwith no badge for the actual state and the aggregate does not account for all items. (lib/view/page/container/resource_views.dart) — anchor-unreliable - 🟡 Medium Negative container-reference counts are accepted as a known value and therefore suppress the unknown/unused indication. A Docker or Podman image row with
Containers: "-1"(or-1) is parsed successfully;isUnusedreturns false andcountUnusedTaggedImagesneither counts it nor returns null. This can make the prune dialog report a definite lower count even though the usage count is malformed and cannot establish whether the tagged image is in use. The claim would be false if runtime image output is guaranteed to constrainContainersto non-negative values before these models receive it, or if negative counts are intentionally a valid “in use” sentinel. (lib/data/model/container/image.dart) — per-file-budget - 🟡 Medium
PercentCircledoes not sanitize NaN before passing it toCircleChart. Forpercent = double.nan, both switch comparisons are false, so NaN is forwarded asprogressNumberand also formatted asNaN%; the chart can assert or render invalid geometry instead of receiving a bounded percentage. This is reachable from existing callers such as PVE's(item.cpu / item.maxcpu) * 100whenmaxcpu == 0, and from any malformed/partial metric calculation. The claim would be false if every caller is proven to exclude NaN and thePercentCircleAPI is never instantiated with non-finite values. (lib/view/widget/percent_circle.dart) — anchor-outside-diff - 🟡 Medium Container status localization remains incomplete:
ContainerStatus.displayNamereturns hard-coded English for created, paused, restarting, removing, and dead, while the scope's container status contract requires localized status distinctions across supported locales. In any non-English locale, these statuses render English labels even though running/exited/unknown use localization. This would be false only if these enum values are guaranteed never to reach the container UI. (lib/data/model/container/status.dart) — inline-budget - 🟡 Medium A container row with an empty
idis treated as actionable._onTapMoreBtnreturns only forid == null, so its start/stop/restart/delete paths generate commands such asstart '', and logs/terminal generate interactive commands targeting''; the operation can then be reported as successful even though no container was selected. This is reachable with a model/UI item whose ID is empty (the row code already supports missing/empty identity fallbacks), and the group path similarly collects empty strings viawhereType<String>(). The invariant should be that an action is not offered/executed unless the identifier is non-empty after trimming. (lib/view/page/container/actions.dart) — inline-budget - 🟡 Medium The helper does not initialize
UIs.primaryColor/the app theme, soPercentCirclerendering depends on mutable process-global state left by whatever test ran before it.PercentCirclepassesUIs.primaryColordirectly toCircleChart, while_pumpAtcreates a bareMaterialAppwith nothemeand never assigns the color. A preceding test or a repeated test invocation that changesUIs.primaryColorcan therefore produce a different resource-card appearance (and any future color/golden assertion can fail) without changing the widget input. (test/container_resource_views_test.dart) — inline-budget - 🟡 Medium Paused, restarting, and removing containers are classified as non-running but not stopped, so
ContainerMenu.itemsfalls through to only delete/logs andContainerGroupMenu.itemsoffers no start/stop/restart action for them. In particular a paused container has no exposed unpause/restore transition, and a restarting/removing container is treated as an inert row despite the newly parsed states. (lib/data/model/app/menu/container.dart) — inline-budget - 🟡 Medium When Docker reports
Containers: "N/A",DockerImg.isUnusedis false, so the image row has no unused badge and the image summary reports no unused count; however the prune dialog'scountUnusedTaggedImagescorrectly reports an unknown count for the same image. The same screen therefore presents an image as neither unused nor unknown while warning that the unused count cannot be determined. (lib/view/page/container/resource_views.dart) — inline-budget - 🟡 Medium The Podman ps command emits
{{json .}}\t{{.Status}}, but the stats command remains JSON-per-line andparseContainerStatsRowsonly acceptsID,Id, orContainerID. Podman stats commonly identifies the container withCID/ContainerIDvariants depending on version; if it emits onlyCID, the new Podman resource panels silently receive no stats even though the ps rows and version parse successfully. (lib/data/provider/container.dart) — inline-budget - 🟡 Medium Changing the runtime while a refresh is in flight can create overlapping remote refreshes:
setTypeis allowed whilestate.isBusyis true,_resetSudoProbeincrements the generation and immediately marks the state non-busy, but it does not cancel or await the old SSH operation. The old refresh continues throughclient.execWithPwd; a new refresh can start concurrently, and only the old result is suppressed when it eventually observes the stale generation. This breaks the single-flight invariant and can produce competing remote commands (and transiently inconsistent loading state). This would be false only ifsetType/resetSudoProbewere guaranteed never to be called during an in-flight refresh outside the UI guards, but the notifier method itself does not enforce that andsetTypeexplicitly guards onlyrunLog. (lib/data/provider/container.dart) — inline-budget - 🟡 Medium Non-running lifecycle states are silently omitted from both the overall and per-project counts:
paused,restarting,removing, anddeadare neitherisRunningnorisStopped, and they are not counted asunknown. A host with only a paused (or dead) container therefore renders a summary saying0 Runningwith no state badge, even though the row visibly contains a non-running container. (lib/view/page/container/resource_views.dart) — anchor-unreliable - 🟡 Medium A malformed/extreme Podman
Createdtimestamp can throw during image rendering instead of being omitted safely._formatUnixDatemultiplies arbitrary positive seconds by 1000 and passes the result to DateTime.fromMillisecondsSinceEpoch; values outside DateTime's supported range throw, and_ContainerImageRowinvokes this synchronously in build. The new widget tests cover only DockercreatedAtstrings and never verify malformed Podman image dates, so a partial Podman image response can produce a render exception. (lib/view/page/container/resource_views.dart) — anchor-unreliable - 🔵 Low Negative image sizes are rendered as if they were valid sizes.
PodmanImg.fromJsonaccepts a negativeSizethrough_asInt, andsizeMBdirectly formats it;DockerImg.fromJsonlikewise preserves a"-10 MB"size string. Consequently malformed runtime output can show a negative image size in the image list instead of the existing placeholder/omission behavior used for invalid metrics. The claim would be false if both supported runtimes guarantee non-negative image sizes at the command boundary and no mocked/older runtime can emit a negative value. (lib/data/model/container/image.dart) — inline-budget - 🔵 Low These tests cannot detect localization-induced layout/lookup failures:
_pumpAtalways supplieslocale: const Locale('en'), and every assertion searches English strings such asCPU,Dangling,Unused, and1 Running · 1 Stopped. For example, a generated German/Chinese resource label could be missing, overflow, or use the wrong delegate while this entire suite remains green. ThesupportedLocaleslist is present but never exercised with any non-English locale. (test/container_resource_views_test.dart) — inline-budget
♻️ Previously reported (still present) (1)
- 🟡 Medium Podman containers in unsupported but non-running lifecycle states are incorrectly classified as running when
Exitedis false.fromPodmancallsfromDockerState(rawStatus), then falls back for any unknown text; thus{Exited:false, Status:'stopping'}(and Podman states such asconfigured/initialized/unknown) becomesContainerStatus.running, exposing stop/restart/terminal actions for a container that is not running. The claim is false only if Podman guarantees that every non-running state is rendered as one of the recognizedExited,Created,Paused,Restarting,Removing, orDeadstrings, and never emits these lifecycle states in.Status/.State. (lib/data/model/container/status.dart) — previously-reported
❓ Low-evidence leads (not confirmed — verify before acting) (3)
- Docker image deserialization converts missing optional fields into literal strings, losing null/data semantics. In
DockerImg.fromJson, theContainersswitch'sObject?arm also matches null and callsa.toString(), yielding"null"; the same pattern forSizeyields"null", whileCreatedAt/Tagare passed through dynamically. Consequently a valid runtime row omitting optional fields serializes asContainers: "null"/Size: "null"rather than preserving missing values, and re-parsing cannot distinguish missing from a literal value. The claim would be false only if Docker always emits these fields and persisted JSON is never produced from incomplete rows. (lib/data/model/container/image.dart) - Podman stats with nested
Networkcounters are silently shown as zero when the Podman version is unavailable.parseStatstakes themajorVersionNum == nullbranch and reads only top-levelNetInput/NetOutput; a Podman 5/netavark stats row such as{"Network":{"eth0":{"RxBytes":1024,"TxBytes":2048}}}therefore producesnet = '↓ 0 B / ↑ 0 B'despite valid counters. This is a concrete uncovered variant of the version-missing parsing obligation: version lookup can fail or be omitted, while the stats payload still identifies the counters. The claim would be false if the application guarantees that every Podman stats refresh always has a valid version before parsing, or if Podman never emits nested network data without that version. (lib/data/model/container/ps.dart) - A malformed/out-of-range Podman
createdvalue can throw during page rendering instead of being omitted._imageCreatedLabelcalls_formatUnixDate, which directly constructsDateTime.fromMillisecondsSinceEpoch(seconds * 1000)with no range/exception guard; Podman JSON is remote/untrusted and a sufficiently large positive timestamp is outside DartDateTime's supported range and raises aRangeError. Thus an otherwise valid image list can make the images tab fail to build, violating the no-exception rendering behavior tested for malformed resource data. This is false only if the provider/parser guaranteescreatedis always a bounded epoch-second value before anyPodmanImgreaches this view. (lib/view/page/container/resource_views.dart)
🤖 Prompt for AI agents — all findings (35)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (8)
In lib/data/model/container/status.dart around line 48, address this finding:
Podman status text is not authoritative when it is present but unrecognized: `fromPodman` falls back to the legacy `Exited` flag for any `rawStatus` that parses as `unknown`. For example, a row with `rawStatus: 'Paused (health: starting)'` or a newer Podman lifecycle string and `Exited: false` is classified as running, exposing stop/restart/terminal actions instead of the safe unknown-state policy. The fallback should be limited to absent/empty status text (or otherwise preserve unknown). This is disproven if all supported non-empty Podman status strings are guaranteed to be among the Docker parser's exact substring patterns.
In lib/view/page/container/container.dart around line 311, address this finding:
Unknown containers are startable in the per-container menu but are omitted from group start eligibility and execution. `ContainerMenu.items(unknown)` returns `[start, rm, logs]`, while the group menu is shown only when `anyStopped` is true and `_onTapGroupMenu` collects only `isStopped` IDs; since `unknown.isStopped` is false, an unknown-only group has no Start action and mixed groups silently exclude its unknown IDs. This is disproven if unknown containers are intentionally excluded from bulk start despite being individually startable; in that case the per-container policy must also remove Start.
In lib/data/model/container/ps.dart around line 153, address this finding:
The parser accepts rows with no usable container ID (`Id` missing, null, or empty) and still creates `PodmanPs` entries. These phantom entries can appear in the UI with status-derived actions, while bulk action extraction drops their null IDs, producing inconsistent menus and counts. A malformed Podman row should be skipped, just like malformed JSON rows. This is disproven if every runtime output is guaranteed to contain a non-empty `Id` and the parser is not expected to defend against valid JSON rows missing required fields.
In lib/data/provider/container.dart around line 199, address this finding:
The stats matcher can assign the same stats row to multiple containers when IDs share the first 12 characters. `findContainerStatsRow` accepts any pair of IDs of length at least 12 when either starts with the other, so if two containers have IDs `0123456789abcdef...A` and `0123456789abcdef...B`, both lookups can match the first row's 12-character prefix only when the supplied ID itself is shortened; more importantly, the function does not reject ambiguous prefixes and returns the first match. A short runtime/container ID of exactly 12 characters can therefore select an arbitrary row among multiple full IDs with that prefix. The claim would be false only if callers always provide full IDs and runtime output never contains ambiguous prefixes.
In lib/data/provider/container.dart around line 304, address this finding:
Overlapping refresh requests are not queued losslessly: `_pendingRefresh` stores only one target, and a request for the other target overwrites the earlier pending request. For example, while a container refresh is busy, call `refreshImages()` and then `refreshContainers()`; the image refresh is discarded and only the latter container refresh runs. This violates the obligation to serialize or queue overlapping refreshes correctly and can leave the image view stale after operations. The claim would be false only if callers guarantee that overlapping requests always have the same target or independently refresh every discarded target.
In lib/data/provider/container.dart around line 202, address this finding:
Two containers whose IDs share the first 12 characters can receive the same statistics. `docker ps` is deliberately requested with `{{.ID}}`, which is normally a 12-character short ID, while `docker stats` can return longer IDs; `findContainerStatsRow` treats either value as a match whenever both are at least 12 characters and returns the first match. If two live containers share that 12-character prefix, the loop applies the first stats row to both containers, so CPU/memory/network/disk values are displayed for the wrong container.
In lib/view/page/container/resource_views.dart around line 186, address this finding:
Image rows are not sorted before being rendered, so the order (and index-based row keys) follows the runtime's raw `image ls` output. Docker/Podman can return the same images in different orders across refreshes, causing visible row movement and unstable `image-row-*-$index-$id` identity despite the scope requiring stable sorting and collision-resistant presentation.
In lib/data/model/container/image.dart around line 76, address this finding:
Unknown image usage matching does not normalize Docker Hub short names on the container-reference side. An image row for `docker.io/library/alpine:latest` gets markers for both the fully qualified and short aliases, but a running container reported as `alpine:latest` is processed only as `ref:alpine:latest`; this particular pair matches, while digest/repository forms with a `docker.io` prefix or equivalent short-name variants can fail to match and cause `unusedTaggedCount` to become Unknown or misclassify usage.
## Additional findings on this change (not posted inline) (26)
In lib/data/model/container/ps.dart around line 123, address this finding:
`PodmanPs.fromJson` assumes `Names` is either null or an iterable of strings and calls `List<String>.from(json['Names']!.map((x) => x))`. A malformed-but-JSON-valid row such as `{"Names":"worker"}` or `{"Names":[null]}` throws during `PodmanPs.fromJson`; `parsePodmanPsOutput` catches only `FormatException` and `TypeError`, so this can escape and abort parsing the entire listing rather than skipping that malformed row while retaining valid rows. This is disproven if the Podman command contract guarantees `Names` is always a non-null list of strings for every row, including older/compatibility output.
In lib/view/page/container/container.dart around line 421, address this finding:
Unknown-status containers can be started individually but cannot be started through their compose/group menu. `ContainerMenu.items` explicitly includes `start` for `ContainerStatus.unknown`, while `_buildGroupMoreBtn` sets `anyStopped` only from `status.isStopped` and `_onTapGroupMenu` builds `stoppedIds` only from `status.isStopped`; `isStopped` intentionally excludes `unknown`. Therefore a group containing only unknown containers has no Start menu item, and a mixed group silently omits those IDs from the bulk start command. This is false only if unknown containers are intentionally forbidden from group actions despite being offered Start individually.
In lib/data/model/container/ps.dart around line 222, address this finding:
Malformed container rows with no usable ID are accepted as real containers instead of being skipped. For example, Docker output `\tExited (0)\tname\timage` passes the `parts.length >= 4` check and creates a `DockerPs` whose `id` is empty; similarly, Podman output `{}` is accepted by `PodmanPs.fromJson`. These phantom rows can appear in the container list and cannot be targeted by action or matched to stats, violating the parser's malformed-row handling contract. This would be false only if every runtime is guaranteed to emit a non-empty ID and callers independently filter empty/null IDs before displaying or acting on parsed items.
In lib/data/model/container/ps.dart around line 247, address this finding:
Malformed Podman numeric fields are converted to zero rather than treated as invalid/unknown. `_asInt` uses `int.tryParse(...) ?? 0`, so a stats row such as `{"MemUsage":"not-a-number", "NetInput":"bad"}` produces apparently valid `0 B` metrics (and similarly corrupts individual counters) instead of isolating the malformed metric/row. This violates the requirement that numeric variation and malformed rows not poison parsed statistics. The claim would be false only if Podman guarantees these fields are always numeric or the zero display is explicitly the required representation for invalid values.
In lib/data/model/container/ps.dart around line 98, address this finding:
Podman 5 network parsing discards valid legacy counters when the nested Network map contains any counter key, even if that interface's value is malformed or only one direction is present. `hasNestedNetworkCounters` becomes true for `RxBytes`/`TxBytes` key presence, then `_asInt` turns missing/invalid values into zero and the code never falls back to top-level `NetInput`/`NetOutput`. For example, a Podman 5 row with `Network: {eth0: {RxBytes: "bad"}}` and valid top-level counters displays `0 B / 0 B` or partial data instead of the valid counters. The claim would be false only if Podman guarantees nested counters are always complete and valid whenever either key appears.
In lib/data/model/container/ps.dart around line 204, address this finding:
Docker stats parsing mutates the container metrics before validating later fields, so a malformed row can leave partially updated statistics visible. `cpu` and `mem` are assigned before `NetIO`/`BlockIO` are cast as `String`; if either field is a numeric JSON value or otherwise malformed, a TypeError is caught by the caller, but the new CPU/memory values remain on the object while net/disk retain old values. This violates malformed-row isolation. The claim would be false only if Docker always emits all four fields as strings and the parser is never invoked with malformed or version-variant rows.
In lib/data/model/container/image.dart, address this finding:
Digest-pinned container references using the canonical Docker Hub name do not match images listed under Docker's usual short repository name. `_addDigestMarkers` only adds the unqualified alias when the image repository starts with `docker.io/library/`; for an image parsed as repository `nginx` with digest `sha256:...`, a running container reported as `docker.io/library/nginx@sha256:...` produces marker `digest:docker.io/library/nginx@...`, while the image produces only `digest:nginx@...`. Consequently `countUnusedTaggedImages` returns `null` (unknown) even though the container reference definitively proves the image is used, making the unused-image count incorrect. This would be disproven if the runtime always emits the same repository spelling in both image-list and ps output, or if Docker image-list output is guaranteed to canonicalize `nginx` to `docker.io/library/nginx`.
In lib/data/model/container/ps.dart around line 244, address this finding:
Malformed or missing Podman numeric fields are converted to plausible zero values instead of isolating the malformed metric/row. For example, a stats row with `{"CPU":"garbage","AvgCPU":"garbage","NetInput":"garbage","NetOutput":"garbage"}` is accepted by `parseStats`: `_asDouble`/`_asInt` return 0, producing `0.0% / Avg 0.0%` and `↓ 0 B / ↑ 0 B`. The provider catches exceptions per row, but these invalid values do not throw, so the UI reports zero usage rather than omitting the bad metric or row, masking a malformed Docker/Podman response.
In lib/data/provider/container.dart around line 296, address this finding:
`resetSudoProbe` is not a complete cancellation barrier for an in-flight refresh: it replaces the sudo completer and increments the generation, but an already-running `client.execWithPwd` is left alive. Because it also sets `isBusy` false (via callers such as host editing), another refresh may begin and execute concurrently with the old remote command. The old command's output is only discarded after it returns, so generation prevents stale state publication but does not preserve the single-command-at-a-time invariant or actually cancel work. This is disproven only if the notifier can guarantee that reset is never called while `refresh` has passed the probe and is executing; the provider API currently has no such guard.
In lib/data/provider/container.dart around line 302, address this finding:
The queue stores only one `(target, isAuto)` entry, and `_queueRefresh` replaces it whenever a later request has a different target. Thus a refresh request for the other resource is silently dropped. For example, while a container refresh is busy, an images refresh queues `images`; a subsequent containers refresh changes `_pendingRefresh` to `containers`, so `_refreshPendingIfNeeded` runs only containers and images are never refreshed. This is false only if the intended contract is explicitly “latest refresh request wins” and callers never rely on both resource targets being refreshed; the provider exposes independent `refreshContainers`/`refreshImages` APIs and UI can generate both requests.
In lib/data/model/container/image.dart around line 267, address this finding:
Negative image reference counts are treated as valid data and can be rendered as non-unused. Docker's `containersCount` uses `int.tryParse` without rejecting values below zero; for `Containers: '-1'`, `isUnused` is false and the image is not marked unknown, so prune summaries can assert a misleading resource state.
In lib/data/model/container/image.dart around line 264, address this finding:
Negative image sizes are accepted and rendered directly. `DockerImg.sizeMB` returns the raw Docker size string without validating sign, so a provider row such as `Size: '-2 MB'` produces a visible negative size rather than omitting malformed data.
In lib/view/page/container/resource_views.dart, address this finding:
Container summary badges omit valid lifecycle states such as paused, restarting, removing, created, and dead. The summary computes only running, stopped (which itself excludes paused/restarting/removing), and unknown, so a page containing only paused containers displays `0 Running` with no badge for the actual state and the aggregate does not account for all items.
In lib/data/model/container/image.dart around line 42, address this finding:
Negative container-reference counts are accepted as a known value and therefore suppress the unknown/unused indication. A Docker or Podman image row with `Containers: "-1"` (or `-1`) is parsed successfully; `isUnused` returns false and `countUnusedTaggedImages` neither counts it nor returns null. This can make the prune dialog report a definite lower count even though the usage count is malformed and cannot establish whether the tagged image is in use. The claim would be false if runtime image output is guaranteed to constrain `Containers` to non-negative values before these models receive it, or if negative counts are intentionally a valid “in use” sentinel.
In lib/view/widget/percent_circle.dart around line 17, address this finding:
`PercentCircle` does not sanitize NaN before passing it to `CircleChart`. For `percent = double.nan`, both switch comparisons are false, so NaN is forwarded as `progressNumber` and also formatted as `NaN%`; the chart can assert or render invalid geometry instead of receiving a bounded percentage. This is reachable from existing callers such as PVE's `(item.cpu / item.maxcpu) * 100` when `maxcpu == 0`, and from any malformed/partial metric calculation. The claim would be false if every caller is proven to exclude NaN and the `PercentCircle` API is never instantiated with non-finite values.
In lib/data/model/container/status.dart around line 66, address this finding:
Container status localization remains incomplete: `ContainerStatus.displayName` returns hard-coded English for created, paused, restarting, removing, and dead, while the scope's container status contract requires localized status distinctions across supported locales. In any non-English locale, these statuses render English labels even though running/exited/unknown use localization. This would be false only if these enum values are guaranteed never to reach the container UI.
In lib/view/page/container/actions.dart around line 299, address this finding:
A container row with an empty `id` is treated as actionable. `_onTapMoreBtn` returns only for `id == null`, so its start/stop/restart/delete paths generate commands such as `start ''`, and logs/terminal generate interactive commands targeting `''`; the operation can then be reported as successful even though no container was selected. This is reachable with a model/UI item whose ID is empty (the row code already supports missing/empty identity fallbacks), and the group path similarly collects empty strings via `whereType<String>()`. The invariant should be that an action is not offered/executed unless the identifier is non-empty after trimming.
In test/container_resource_views_test.dart around line 925, address this finding:
The helper does not initialize `UIs.primaryColor`/the app theme, so `PercentCircle` rendering depends on mutable process-global state left by whatever test ran before it. `PercentCircle` passes `UIs.primaryColor` directly to `CircleChart`, while `_pumpAt` creates a bare `MaterialApp` with no `theme` and never assigns the color. A preceding test or a repeated test invocation that changes `UIs.primaryColor` can therefore produce a different resource-card appearance (and any future color/golden assertion can fail) without changing the widget input.
In lib/data/model/app/menu/container.dart around line 23, address this finding:
Paused, restarting, and removing containers are classified as non-running but not stopped, so `ContainerMenu.items` falls through to only delete/logs and `ContainerGroupMenu.items` offers no start/stop/restart action for them. In particular a paused container has no exposed unpause/restore transition, and a restarting/removing container is treated as an inert row despite the newly parsed states.
In lib/view/page/container/resource_views.dart around line 152, address this finding:
When Docker reports `Containers: "N/A"`, `DockerImg.isUnused` is false, so the image row has no unused badge and the image summary reports no unused count; however the prune dialog's `countUnusedTaggedImages` correctly reports an unknown count for the same image. The same screen therefore presents an image as neither unused nor unknown while warning that the unused count cannot be determined.
In lib/data/provider/container.dart around line 180, address this finding:
The Podman ps command emits `{{json .}}\t{{.Status}}`, but the stats command remains JSON-per-line and `parseContainerStatsRows` only accepts `ID`, `Id`, or `ContainerID`. Podman stats commonly identifies the container with `CID`/`ContainerID` variants depending on version; if it emits only `CID`, the new Podman resource panels silently receive no stats even though the ps rows and version parse successfully.
In lib/data/provider/container.dart around line 276, address this finding:
Changing the runtime while a refresh is in flight can create overlapping remote refreshes: `setType` is allowed while `state.isBusy` is true, `_resetSudoProbe` increments the generation and immediately marks the state non-busy, but it does not cancel or await the old SSH operation. The old refresh continues through `client.execWithPwd`; a new refresh can start concurrently, and only the old result is suppressed when it eventually observes the stale generation. This breaks the single-flight invariant and can produce competing remote commands (and transiently inconsistent loading state). This would be false only if `setType`/`resetSudoProbe` were guaranteed never to be called during an in-flight refresh outside the UI guards, but the notifier method itself does not enforce that and `setType` explicitly guards only `runLog`.
In lib/view/page/container/resource_views.dart, address this finding:
Non-running lifecycle states are silently omitted from both the overall and per-project counts: `paused`, `restarting`, `removing`, and `dead` are neither `isRunning` nor `isStopped`, and they are not counted as `unknown`. A host with only a paused (or dead) container therefore renders a summary saying `0 Running` with no state badge, even though the row visibly contains a non-running container.
In lib/view/page/container/resource_views.dart, address this finding:
A malformed/extreme Podman `Created` timestamp can throw during image rendering instead of being omitted safely. `_formatUnixDate` multiplies arbitrary positive seconds by 1000 and passes the result to DateTime.fromMillisecondsSinceEpoch; values outside DateTime's supported range throw, and `_ContainerImageRow` invokes this synchronously in build. The new widget tests cover only Docker `createdAt` strings and never verify malformed Podman image dates, so a partial Podman image response can produce a render exception.
In lib/data/model/container/image.dart around line 175, address this finding:
Negative image sizes are rendered as if they were valid sizes. `PodmanImg.fromJson` accepts a negative `Size` through `_asInt`, and `sizeMB` directly formats it; `DockerImg.fromJson` likewise preserves a `"-10 MB"` size string. Consequently malformed runtime output can show a negative image size in the image list instead of the existing placeholder/omission behavior used for invalid metrics. The claim would be false if both supported runtimes guarantee non-negative image sizes at the command boundary and no mocked/older runtime can emit a negative value.
In test/container_resource_views_test.dart around line 927, address this finding:
These tests cannot detect localization-induced layout/lookup failures: `_pumpAt` always supplies `locale: const Locale('en')`, and every assertion searches English strings such as `CPU`, `Dangling`, `Unused`, and `1 Running · 1 Stopped`. For example, a generated German/Chinese resource label could be missing, overflow, or use the wrong delegate while this entire suite remains green. The `supportedLocales` list is present but never exercised with any non-English locale.
## Previously reported and still present (1)
In lib/data/model/container/status.dart around line 50, address this finding:
Podman containers in unsupported but non-running lifecycle states are incorrectly classified as running when `Exited` is false. `fromPodman` calls `fromDockerState(rawStatus)`, then falls back for *any* unknown text; thus `{Exited:false, Status:'stopping'}` (and Podman states such as `configured`/`initialized`/`unknown`) becomes `ContainerStatus.running`, exposing stop/restart/terminal actions for a container that is not running. The claim is false only if Podman guarantees that every non-running state is rendered as one of the recognized `Exited`, `Created`, `Paused`, `Restarting`, `Removing`, or `Dead` strings, and never emits these lifecycle states in `.Status`/`.State`.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 5 of 5 areas reviewed
| static ContainerStatus fromPodman(bool? exited, String? rawStatus) { | ||
| final parsed = fromDockerState(rawStatus); | ||
| if (parsed != ContainerStatus.unknown) return parsed; | ||
| return fromPodmanExited(exited); |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/container/status.dart, address this finding:
Podman status text is not authoritative when it is present but unrecognized: `fromPodman` falls back to the legacy `Exited` flag for any `rawStatus` that parses as `unknown`. For example, a row with `rawStatus: 'Paused (health: starting)'` or a newer Podman lifecycle string and `Exited: false` is classified as running, exposing stop/restart/terminal actions instead of the safe unknown-state policy. The fallback should be limited to absent/empty status text (or otherwise preserve unknown). This is disproven if all supported non-empty Podman status strings are guaranteed to be among the Docker parser's exact substring patterns.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| return fromPodmanExited(exited); | |
| static ContainerStatus fromPodman(bool? exited, String? rawStatus) { | |
| if (rawStatus == null || rawStatus.trim().isEmpty) { | |
| return fromPodmanExited(exited); | |
| } | |
| return fromDockerState(rawStatus); |
| child: PopupMenu( | ||
| items: ContainerGroupMenu.items( | ||
| anyRunning: groupItems.any((e) => e.status.isRunning), | ||
| anyStopped: groupItems.any((e) => e.status.isStopped), |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/container.dart, address this finding:
Unknown containers are startable in the per-container menu but are omitted from group start eligibility and execution. `ContainerMenu.items(unknown)` returns `[start, rm, logs]`, while the group menu is shown only when `anyStopped` is true and `_onTapGroupMenu` collects only `isStopped` IDs; since `unknown.isStopped` is false, an unknown-only group has no Start action and mixed groups silently exclude its unknown IDs. This is disproven if unknown containers are intentionally excluded from bulk start despite being individually startable; in that case the per-container policy must also remove Start.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| anyStopped: groupItems.any((e) => e.status.isStopped), | |
| anyStopped: groupItems.any((e) => e.status.isStopped || e.status == ContainerStatus.unknown), |
| data['ServerBoxStatus'] = detailedStatus; | ||
| } | ||
| } | ||
| items.add(PodmanPs.fromJson(data)); |
There was a problem hiding this comment.
🔍 Error Handling | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/container/ps.dart, address this finding:
The parser accepts rows with no usable container ID (`Id` missing, null, or empty) and still creates `PodmanPs` entries. These phantom entries can appear in the UI with status-derived actions, while bulk action extraction drops their null IDs, producing inconsistent menus and counts. A malformed Podman row should be skipped, just like malformed JSON rows. This is disproven if every runtime output is guaranteed to contain a non-empty `Id` and the parser is not expected to defend against valid JSON rows missing required fields.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| items.add(PodmanPs.fromJson(data)); | |
| final item = PodmanPs.fromJson(data); | |
| if (item.id?.trim().isNotEmpty != true) continue; | |
| items.add(item); |
| row.id.length >= 12 && | ||
| (id.startsWith(row.id) || row.id.startsWith(id)); | ||
| if (id == row.id || prefixMatch) return row.raw; | ||
| } |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/provider/container.dart, address this finding:
The stats matcher can assign the same stats row to multiple containers when IDs share the first 12 characters. `findContainerStatsRow` accepts any pair of IDs of length at least 12 when either starts with the other, so if two containers have IDs `0123456789abcdef...A` and `0123456789abcdef...B`, both lookups can match the first row's 12-character prefix only when the supplied ID itself is shortened; more importantly, the function does not reject ambiguous prefixes and returns the first match. A short runtime/container ID of exactly 12 characters can therefore select an arbitrary row among multiple full IDs with that prefix. The claim would be false only if callers always provide full IDs and runtime output never contains ambiguous prefixes.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| } | |
| final matches = rows.where((row) { | |
| final prefixMatch = id.length >= 12 && | |
| row.id.length >= 12 && | |
| (id.startsWith(row.id) || row.id.startsWith(id)); | |
| return id == row.id || prefixMatch; | |
| }).toList(); | |
| return matches.length == 1 ? matches.single.raw : null; |
|
|
||
| void _queueRefresh(ContainerRefreshTarget target, bool isAuto) { | ||
| final pending = _pendingRefresh; | ||
| _pendingRefresh = ( |
There was a problem hiding this comment.
🔀 Concurrency | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/provider/container.dart, address this finding:
Overlapping refresh requests are not queued losslessly: `_pendingRefresh` stores only one target, and a request for the other target overwrites the earlier pending request. For example, while a container refresh is busy, call `refreshImages()` and then `refreshContainers()`; the image refresh is discarded and only the latter container refresh runs. This violates the obligation to serialize or queue overlapping refreshes correctly and can leave the image view stale after operations. The claim would be false only if callers guarantee that overlapping requests always have the same target or independently refresh every discarded target.
| final prefixMatch = id.length >= 12 && | ||
| row.id.length >= 12 && | ||
| (id.startsWith(row.id) || row.id.startsWith(id)); | ||
| if (id == row.id || prefixMatch) return row.raw; |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/provider/container.dart, address this finding:
Two containers whose IDs share the first 12 characters can receive the same statistics. `docker ps` is deliberately requested with `{{.ID}}`, which is normally a 12-character short ID, while `docker stats` can return longer IDs; `findContainerStatsRow` treats either value as a match whenever both are at least 12 characters and returns the first match. If two live containers share that 12-character prefix, the loop applies the first stats row to both containers, so CPU/memory/network/disk values are displayed for the wrong container.
| itemBuilder: (context, index) { | ||
| if (index == 0) return summary; | ||
| if (index == 1) return const SizedBox(height: 10); | ||
| final imageIndex = index - 2; |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/container/resource_views.dart, address this finding:
Image rows are not sorted before being rendered, so the order (and index-based row keys) follows the runtime's raw `image ls` output. Docker/Podman can return the same images in different orders across refreshes, causing visible row movement and unstable `image-row-*-$index-$id` identity despite the scope requiring stable sorting and collision-resistant presentation.
| return markers; | ||
| } | ||
|
|
||
| void _addRuntimeImageReference(Set<String> markers, String? raw) { |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/container/image.dart, address this finding:
Unknown image usage matching does not normalize Docker Hub short names on the container-reference side. An image row for `docker.io/library/alpine:latest` gets markers for both the fully qualified and short aliases, but a running container reported as `alpine:latest` is processed only as `ref:alpine:latest`; this particular pair matches, while digest/repository forms with a `docker.io` prefix or equivalent short-name variants can fail to match and cause `unusedTaggedCount` to become Unknown or misclassify usage.
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
1 participant
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
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Summary
Changes