fix: fixed tui service status to now properly show container status instead of missing#2139
Conversation
* fix: add ppc64le build support for frontend Signed-off-by: Ethan Choe <ethanchoe@ibm.com> * fix: add ppc64le webpack fallback for make frontend dev server Signed-off-by: Ethan Choe <ethanchoe3@gmail.com> * fix: use path.resolve for ENV_FILE to support absolute paths Signed-off-by: Ethan Choe <ethanchoe3@gmail.com> * fix: escape $(hostname) as $$(hostname) in Makefile frontend target --------- Signed-off-by: Ethan Choe <ethanchoe@ibm.com> Signed-off-by: Ethan Choe <ethanchoe3@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughContainer status resolution now prefers compose service metadata for Docker and Podman containers, falling back to legacy container-name mappings. Tests cover prefix-independent resolution and unmatched-container behavior. ChangesContainer status resolution
Estimated code review effort: 3 (Moderate) | ~15–30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/test_container_manager_status.py (1)
28-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for Docker
Service-field resolution.The production change covers both Podman labels and Docker’s
Servicefield, but these tests only exercise the Podman path and direct fallback. Add a focused_process_service_jsontest so regressions in the Docker branch are detected.Suggested test
+def test_process_service_json_resolves_by_compose_service(): + manager = _make_manager() + services = {} + + manager._process_service_json( + { + "Name": "tui-opensearch", + "Service": "opensearch", + "State": "running", + }, + services, + ) + + assert services["opensearch"].status == ServiceStatus.RUNNING🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_container_manager_status.py` around lines 28 - 51, Add a focused test for _process_service_json that supplies a container record with Docker’s Service field and verifies it resolves to the expected service and status. Keep the test independent of label-based resolution and assert the Docker branch’s resulting service mapping so regressions are detected.
🤖 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 `@Makefile`:
- Around line 735-743: Extend the Power architecture fallback beyond the
Makefile frontend target so every frontend dev entrypoint uses Webpack on
ppc64le and ppc64. Update the frontend package-level dev command or the
Playwright startup configuration, using the existing architecture detection and
preserving plain next dev for other architectures; ensure both npm run dev and
Playwright startup select the fallback consistently.
- Around line 739-743: Update both Next.js dev command branches in the Makefile
to pass the configured bind-host variable instead of invoking the machine’s
hostname. Preserve the existing default and use the same host setting for both
the branch containing the preceding command and the npx next dev fallback.
---
Nitpick comments:
In `@tests/unit/test_container_manager_status.py`:
- Around line 28-51: Add a focused test for _process_service_json that supplies
a container record with Docker’s Service field and verifies it resolves to the
expected service and status. Keep the test independent of label-based resolution
and assert the Docker branch’s resulting service mapping so regressions are
detected.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e35f6e4a-ca04-4e13-b953-326edc4d2e5a
📒 Files selected for processing (6)
Makefilefrontend/build.jsfrontend/next.config.tsfrontend/package.jsonsrc/tui/managers/container_manager.pytests/unit/test_container_manager_status.py
lucaseduoli
left a comment
There was a problem hiding this comment.
LGTM but please merge to main instead of release-0.6.0. I will cherry pick the commit later
Summary
Fixes #2134 The TUI status screen was showing all container services as "Missing" on Podman because the container name resolution relied on reconstructing a {project_name}-{service} prefix, but Compose derives the project name from the compose file's directory (~/.openrag/tui/), producing tui-* containers instead of the assumed openrag-*. The fix introduces a _resolve_service_name helper that keys off the stable com.docker.compose.service label (Podman) and the Service field (Docker) before falling back to the existing name map, matching the pattern already used in welcome.py. This makes status resolution robust regardless of what project name Compose derives at runtime.
Changes
src/tui/managers/container_manager.py: add _resolve_service_name helper and wire it into both Docker and Podman status paths to resolve containers by compose label before falling back to the name maptests/unit/test_container_manager_status.py: add regression and fallback tests for the new resolution logicSummary by CodeRabbit