fix(docker): guard primary container name access#1977
Merged
Conversation
- Purpose: prevent Docker template sync from crashing when Docker reports a container without a primary name. - Before: the scanner assumed container.names[0] always existed and called .replace() on it during sync and template matching. - Why that was a problem: stale or malformed Docker metadata could make container.names[0] undefined, which crashed the scanner and bubbled the failure into mothership-backed API flows. - What changed: nameless containers are now skipped for name-based mapping, repository-only matching still works, and the regression is covered with scanner tests. - How it works: normalizeContainerName now returns null for missing names, scan/sync use a shared helper to guard primary-name access, and tests cover empty-name containers end-to-end.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
Disabled knowledge base sources:
WalkthroughCentralized Docker container primary-name extraction was introduced and propagated across backend services and web UI. Logic now returns null for missing names; nameless containers are skipped in scanning/syncing and fallback matching by repository is preserved. Tests expanded to cover null/empty-name scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- Purpose: centralize Docker primary-name lookup so nameless containers no longer crash template scanning or related flows. - Before: multiple API and web call sites read names[0] directly or duplicated slash-stripping logic with inconsistent fallback behavior. - Problem: containers without a first Docker name could break scans, resolver paths, and UI interactions, and the duplicated logic made regressions easy to miss. - Now: API and web Docker code route name lookup through shared helpers, preserving organizer ID semantics where the leading slash is expected while safely handling missing names elsewhere. - How: added a reusable Docker name helper, updated the Docker scanner/service/resolver/autostart/port and organizer code plus affected web components/composables, and refreshed tests/fixtures to cover nameless containers safely.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts (1)
295-297: Consider reducing warning noise for persistent nameless containersLine 295 logs a warning every time this helper is hit; on systems with stale nameless containers this can flood logs during periodic sync/scan. Consider
debuglevel or dedupe-by-container-id logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts` around lines 295 - 297, The repeated warning comes from the docker-template-scanner.service logging "Skipping container ${container.id}... no primary Docker name" on every scan; change this to reduce noise by either lowering it to logger.debug or implementing a one-time warning per container id: add a Set (e.g., loggedNamelessContainerIds) as a private field on the DockerTemplateScannerService class and, in the helper that currently calls logger.warn, check/set that Set and only log once per container.id (or use logger.debug instead if dedupe is not desired). Ensure you update references to container.id and logger.warn in the helper to use the new Set/check or debug call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts`:
- Around line 295-297: The repeated warning comes from the
docker-template-scanner.service logging "Skipping container ${container.id}...
no primary Docker name" on every scan; change this to reduce noise by either
lowering it to logger.debug or implementing a one-time warning per container id:
add a Set (e.g., loggedNamelessContainerIds) as a private field on the
DockerTemplateScannerService class and, in the helper that currently calls
logger.warn, check/set that Set and only log once per container.id (or use
logger.debug instead if dedupe is not desired). Ensure you update references to
container.id and logger.warn in the helper to use the new Set/check or debug
call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 45958d2f-87d1-48b6-974c-571d45b37f26
📒 Files selected for processing (2)
api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts
- Purpose: unblock the failing PR checks after the shared Docker name helper landed. - Before: the logic changes were correct, but a few API imports/line wraps and one Vue file were not formatted the way CI expects. - Why that was a problem: the Test API lint step and Build Web App lint step both failed, so the branch looked broken even though the behavior and tests were fine. - What changed: applied the exact Prettier formatting CI wanted in the Docker resolver/helper/organizer files and the Docker containers table. - How it works: no runtime behavior changed; this commit only normalizes formatting so the existing lint/build pipeline passes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1977 +/- ##
==========================================
+ Coverage 52.08% 52.10% +0.02%
==========================================
Files 1031 1032 +1
Lines 71564 71616 +52
Branches 8090 8105 +15
==========================================
+ Hits 37275 37318 +43
- Misses 34164 34173 +9
Partials 125 125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
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
names[0]directlyRoot cause
Docker can report stale or malformed container metadata with an empty or missing primary name. Several code paths assumed
names[0]always existed, which made the template scan brittle and left a few related API/UI flows exposed to the same failure mode.Verification
cd api && pnpm run lintcd api && pnpm run testcd web && pnpm run buildcd web && pnpm run lintSummary by CodeRabbit
Bug Fixes
Tests
Refactor