Skip to content

Improve container parsing code quality#1217

Merged
GT-610 merged 4 commits into
mainfrom
improve-container-code-quality
Jun 29, 2026
Merged

Improve container parsing code quality#1217
GT-610 merged 4 commits into
mainfrom
improve-container-code-quality

Conversation

@GT-610

@GT-610 GT-610 commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Bug Fixes
    • Improved container list/status handling by aligning persisted container details with the selected container type.
    • Safer Docker commands now shell-quote DOCKER_HOST to handle special characters correctly.
    • Sudo handling is more reliable: stale async refreshes no longer overwrite current state, and sudo probing resets when updating the Docker host.
  • Refactor
    • Streamlined container status models and updated parsing/serialization to match the simplified contract.
    • Container actions now always trigger a refresh after command execution.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 36391600-de10-42e7-9e0d-b7ab75abc3f5

📥 Commits

Reviewing files that changed from the base of the PR and between f86e6f7 and a9db5df.

📒 Files selected for processing (2)
  • lib/data/provider/container.dart
  • lib/view/page/container/actions.dart
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/data/provider/container.dart

📝 Walkthrough

Walkthrough

ContainerPs now exposes id, image, and name as abstract nullable getters, and PodmanPs keeps only exited, id, image, and names. DockerPs drops its cmd override. ContainerNotifier adds refresh-generation tracking and staleness checks, refactors sudo detection around ContainerType, passes sudo into execAll, and changes run() to always refresh after execution. Docker host wrapping now uses _shellQuote() for DOCKER_HOST, and saving Docker host resets sudo probe state before refresh.

Possibly related PRs

Suggested reviewers

  • lollipopkit
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and broadly matches the container parsing and related refactor work in this PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch improve-container-code-quality

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

72-85: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not clear isBusy while an old refresh may still be running.

Line 80 can bypass the refresh() guard after a type switch, allowing an older suspended refresh to resume and overwrite state for the new container type. Use a refresh generation/cancellation token, or otherwise ignore stale refresh completions after setType.

🤖 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 72 - 85, The setType flow in
ContainerProvider is clearing isBusy too early, which can let an older suspended
refresh resume after a type switch and overwrite the new state. Update setType
and refresh to use a generation/token or similar stale-refresh guard, and only
apply refresh results when they still match the latest ContainerType change.
Make sure refresh completions from an older request are ignored after setType
starts a new cycle.
🤖 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 72-85: The setType flow in ContainerProvider is clearing isBusy
too early, which can let an older suspended refresh resume after a type switch
and overwrite the new state. Update setType and refresh to use a
generation/token or similar stale-refresh guard, and only apply refresh results
when they still match the latest ContainerType change. Make sure refresh
completions from an older request are ignored after setType starts a new cycle.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5ee31879-dfba-45eb-a6f0-e9e8d9eae8d5

📥 Commits

Reviewing files that changed from the base of the PR and between 5484183 and 916466f.

📒 Files selected for processing (2)
  • lib/data/model/container/ps.dart
  • lib/data/provider/container.dart

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 29, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 121-128: The cached sudo probe is being reused across target
changes, so refreshes can keep a stale needSudo result after switching
endpoints. In the container provider’s refresh flow and the Docker-host save
path, make sure the cached sudo state is invalidated whenever the
target/endpoint changes before calling refresh(), and let _requiresSudo
recompute against the new target instead of reusing the old sudoCompleter
result. Use the existing refresh(), sudoCompleter, and _requiresSudo symbols to
locate the cache/reset logic and update it so a new endpoint always triggers a
fresh sudo probe.
- Around line 268-270: The Docker header filtering in the container listing
logic is too broad because `removeWhere((element) => element.contains('CONTAINER
ID'))` can حذف valid rows that merely contain that text. Update the
`ContainerType.docker` branch in the container parsing flow to remove only the
actual `ps` header row, using a precise header match or a positional/header
check instead of substring matching so real container entries are preserved.
- Around line 121-122: The refresh flow in ContainerProvider.refresh() is
dropping completion refreshes when state.isBusy is true, which can leave the
view stale after run() finishes. Update refresh() to queue a pending refresh
instead of returning immediately during an in-flight auto-refresh, and have
run() trigger that queued refresh once the busy operation completes. Use the
existing refresh() and run() methods in ContainerProvider to add the deferral
logic without changing the external API.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d7b5a0eb-9b2f-4836-a5b7-5e7e71720e28

📥 Commits

Reviewing files that changed from the base of the PR and between 916466f and f86e6f7.

📒 Files selected for processing (1)
  • lib/data/provider/container.dart

Comment thread lib/data/provider/container.dart Outdated
Comment thread lib/data/provider/container.dart
Comment thread lib/data/provider/container.dart Outdated
@coderabbitai coderabbitai Bot requested a review from lollipopkit June 29, 2026 04:34
@GT-610 GT-610 merged commit 1f6ee0b into main Jun 29, 2026
3 checks passed
@GT-610 GT-610 deleted the improve-container-code-quality branch June 29, 2026 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant