fix: match autostart membership to what the UI renders - #54
Conversation
syncContainerOrder decided folder membership differently from docker.js, so
the autostart file could disagree with the screen.
Docker labels were missing entirely. docker.js assigns a container to a folder
when its `folder.view3` label matches the folder name, and treats that claim as
beating another folder's regex. The PHP had no label handling, so a container
assigned purely by label rendered inside its folder but was invisible here — it
was ordered as an unassigned container instead of with its group. Labels are
read from getDockerJSON("/containers/json?all=1"), the same endpoint readInfo
already uses, because getDockerContainers() carries no Labels key.
The regex gate diverged in both directions. empty("0") is true in PHP, so a
regex of "0" was dropped while docker.js applied it; conversely a whitespace
regex was applied here while docker.js skips it via trim(). Gate on
trim($regex) !== '' to mirror the JS test.
Verified on Unraid 7.3.2: a container labelled for "Utilities" moves from line 1
(unrecognised, prepended) to directly after that folder's block. Output is
byte-identical for a config using neither labels nor regex.
|
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)
📝 WalkthroughWalkthrough
ChangesContainer membership synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/folder.view3/usr/local/emhttp/plugins/folder.view3/server/lib.php`:
- Around line 337-343: Update the folder-assignment setup around
$explicitAssigned to maintain a separate set of containers explicitly listed in
$folder['containers']; while collecting label claims from $ctLabels, skip any
container present in that explicit-members set. Keep $explicitAssigned available
for the later filtering, so explicit folder membership always takes precedence
over label-based membership regardless of $folders order.
- Around line 317-327: Update the label-loading block around $ctLabels and
$rawCts to abort before order calculation or persistence when the Docker
response is unavailable, any entry lacks a valid Names[0] or folder.view3 label,
or the mapped names do not fully cover $allContainerNames. Log the failure and
return immediately on these validation errors; only continue to the
autostart-order computation when every required container has a validated label.
- Around line 348-351: Update the folder regex handling around the
regex-building loop to require folder['regex'] to be a string before trimming or
compiling it. Capture preg_match() results and treat false as a compilation
error, then abort synchronization before any autostart-file rewrite; only
continue membership checks when the match result is valid.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a50c1edd-044c-486e-aa85-4692eaddff7d
📒 Files selected for processing (1)
src/folder.view3/usr/local/emhttp/plugins/folder.view3/server/lib.php
| $explicitAssigned = []; | ||
| foreach ($folders as $folder) { | ||
| $explicitAssigned = array_merge($explicitAssigned, $folder['containers'] ?? []); | ||
| } | ||
| foreach ($ctLabels as $ctName => $ctLabel) { | ||
| if (isset($folderNameSet[$ctLabel])) { $explicitAssigned[] = $ctName; } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve explicit membership over label membership.
$explicitAssigned contains both explicit members and label claims. The label loop at Lines 356-358 does not distinguish them. The later $assignedContainers filter therefore makes the first folder in $folders win.
If container c is explicitly assigned to folder A but its label names folder B, the label wins when B appears before A. Keep a separate explicit-members set and skip label assignment for names in that set.
Proposed fix
- $explicitAssigned = [];
+ $explicitMembers = [];
foreach ($folders as $folder) {
- $explicitAssigned = array_merge($explicitAssigned, $folder['containers'] ?? []);
+ $explicitMembers = array_merge($explicitMembers, $folder['containers'] ?? []);
}
+ $explicitAssigned = $explicitMembers;
foreach ($ctLabels as $ctName => $ctLabel) {
- if (isset($folderNameSet[$ctLabel])) { $explicitAssigned[] = $ctName; }
+ if (isset($folderNameSet[$ctLabel]) && !in_array($ctName, $explicitMembers, true)) {
+ $explicitAssigned[] = $ctName;
+ }
}
- if ($ctLabel === ($folder['name'] ?? null) && !in_array($ctName, $members)) {
+ if ($ctLabel === ($folder['name'] ?? null)
+ && !in_array($ctName, $members, true)
+ && !in_array($ctName, $explicitMembers, true)) {Also applies to: 356-358
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/folder.view3/usr/local/emhttp/plugins/folder.view3/server/lib.php` around
lines 337 - 343, Update the folder-assignment setup around $explicitAssigned to
maintain a separate set of containers explicitly listed in
$folder['containers']; while collecting label claims from $ctLabels, skip any
container present in that explicit-members set. Keep $explicitAssigned available
for the later filtering, so explicit folder membership always takes precedence
over label-based membership regardless of $folders order.
There was a problem hiding this comment.
Declining this one for the scope of this PR, and tracking it separately.
The conflict is real and reachable — folder.js:271-276 keeps containers labelled for other folders in choose, where they render as plain enabled checkboxes with nothing indicating they are already claimed, so ticking one into a second folder takes two clicks. Containers labelled for the folder being edited are correctly checked disabled and excluded from the save, but that protection does not extend across folders.
The reason not to fix it here: docker.js is order-dependent in exactly the same way. Label matching at docker.js:305 only tests !combinedContainers.includes(el), so whichever folder renders first takes the container — there is no explicit-wins rule on the client to mirror. This PR exists to remove divergence between the two membership calculations, and making the PHP deterministic while the JS stays arbitrary would introduce a new one.
Worth noting the two also iterate differently — PHP walks $folders in docker.json key order, the client renders in display order — so for any contested container they can already disagree, regardless of this change.
Explicit membership beating a label is the better rule; it just needs applying to both sides in one change. Filed separately.
There was a problem hiding this comment.
@chodeus, acknowledged. The finding remains valid, but the proposed PHP-only change would break the current client/server parity that this PR restores.
The follow-up should define the same explicit-membership precedence in both docker.js and syncContainerOrder(). It should also align the folder iteration order or make contested membership deterministic.
🐇
✏️ Learnings added
Learnt from: chodeus
Repo: chodeus/folder.view3 PR: 54
File: src/folder.view3/usr/local/emhttp/plugins/folder.view3/server/lib.php:346-352
Timestamp: 2026-08-09T13:15:00.281Z
Learning: In Folder View 3 container folder membership, `src/folder.view3/usr/local/emhttp/plugins/folder.view3/docker.js` currently resolves conflicting label and explicit assignments by folder iteration order through `!combinedContainers.includes(el)`. `src/folder.view3/usr/local/emhttp/plugins/folder.view3/server/lib.php` `syncContainerOrder()` also resolves these conflicts by folder iteration order, but PHP uses `docker.json` key order and the client uses display order. A future change must implement explicit-membership precedence and deterministic matching on both client and server together.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Two defects in the previous commit, both raised in review. The label read failed open: a failed or partial Docker response left $ctLabels empty and the order was still computed and written, silently emitting label-assigned containers as unassigned. Abort before any write instead, matching the $ctListComplete guard directly above it (#214). The regex gate called trim() on an unvalidated value. empty() tolerated any type, trim() raises a TypeError on PHP 8, so a folder whose regex is not a string (a hand-edited or imported config) killed the sync outright — verified as exit 255 against a folder with an array regex. Check is_string() first, which also matches docker.js's `typeof folder.regex === 'string'`. Verified on Unraid 7.3.2: the guard does not false-trigger (raw and container counts both 45) and the sync still reaches the write; an array regex now completes instead of fataling; a container labelled for Utilities is still placed with that folder's block.
|
@coderabbitai review |
✅ Action performedReview finished.
|
The remove block ran `removepkg folder.view3-<version>`, but since 2026.07.07.1 the package is named folder.view3-<version>-x86_64-1. removepkg aborted with "No such package ... Can't remove" while the following rm -rf lines still deleted the plugin dir and flash config, orphaning the /var/log/packages entry. upgradepkg --install-new then reported "Skipping package ... (already installed)" on reinstall, so files were never extracted and the plugin came back dead while plugin-manager reported success. - Remove now globs /var/log/packages/folder.view3-* and removepkg's each basename: version-agnostic, and it also clears legacy pre-2026.07.07.1 entries the old exact-name call could never match. Siblings folder.view and folder.view2 do not match the glob. - Install gained --reinstall so a stale entry can never suppress extraction. - The package name now has a single definition (&pkgname;) referenced by the pre-install prune, the install FILE/URL and the post-install keep-check, so the four-way hand-reconstruction that allowed this drift is gone. Builds beta 2026.08.14.1, bundling the two fixes already on beta: folder.view2 exports accepted by Import Everything (#53), and autostart membership matching what the Docker page renders (#54).
Merges beta into main and builds the stable package. - Uninstall removes the plugin cleanly: the remove block matched the package DB entry without its -x86_64-1 suffix, so removepkg aborted while the following rm -rf lines still deleted the plugin dir and flash config. The orphaned entry then made upgradepkg skip extraction on reinstall, leaving the plugin installed-but-dead. - Import Everything accepts a folder.view2 backup file (#53). - Container autostart order matches the folder membership the Docker page renders, including label-assigned containers (#54). CHANGES consolidated to a single ###2026.08.14 heading; main keeps its stable changelog history rather than beta's per-build entries.
1 participant
Problem
syncContainerOrder()decides folder membership differently fromdocker.js, so the autostart file can disagree with what the Docker tab renders.Docker labels were missing entirely.
docker.js:305assigns a container to a folder when itsfolder.view3label matches the folder name, anddocker.js:35treats that claim as beating another folder's regex. The PHP had no label handling at all — no reference to the label anywhere underserver/. A container assigned to a folder purely by label therefore renders inside that folder on screen but is invisible to the autostart calculation, and gets ordered as an unassigned container instead of with its group.The labels were not simply overlooked:
getDockerContainers()returns noLabelskey (verified across all 45 containers on a live server), so they were not reachable from the data source this function already had.The regex gate diverged in both directions:
docker.jssyncContainerOrder(before)0empty("0")istruein PHP" ".trim()/ /Change
folder.view3label claims fromgetDockerJSON("/containers/json?all=1")— the same endpointreadInfo()already uses — and assign them to the matching folder, with claims joining$explicitAssignedso precedence matches the client (explicit > label > regex).trim($folder['regex']) !== ''to mirror the JS test exactly, fixing both directions.Cost is one extra Docker API call per sync, measured at 5ms. If that call fails the label map is empty and behaviour falls back to exactly what it is today.
Testing
On Unraid 7.3.2 with plugin 2026.08.01, calling
syncContainerOrder("docker")directly against a live 45-container, 10-folder config:No change for configs using neither labels nor regex — the function ran (returned in 199ms, autostart file mtime advanced, confirming it really executed and rewrote) and produced a byte-identical file.
Label membership now honoured. A container labelled
folder.view3=Utilities, seeded into the autostart file, with everything else held constant:Note for reviewers
Membership is now computed in two places —
docker.jsandlib.php— and nothing enforces that they agree. This PR makes them agree; it does not stop them drifting again, which is how the label gap arose. A single source of truth (the server exposing computed membership, or the client consuming it) would be the durable fix and is deliberately out of scope here.Summary by CodeRabbit
New Features
folder.view3labels.Bug Fixes
"0"being incorrectly treated as empty.