Fix: PHP Warnings & Share/Device prev/next buttons on narrow screens#2373
Conversation
WalkthroughAdd conditional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant S as Server (PHP)
participant B as Browser JS
participant D as DOM
U->>S: Request Device/Share page
S-->>U: HTML (nav span includes `menuNavButtonsTabbed` if $tabbed)
U->>B: load scripts
B->>B: evaluate sheet count / $tabbed flag
alt tabbed / multi-sheet
B->>D: append(prevNextCtrl) to .tabs-container
Note right of D #add8e6: nav element has class `menuNavButtonsTabbed`
else not tabbed
B->>D: append(prevNextCtrl) to .tabs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
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 |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
emhttp/plugins/dynamix/styles/default-base.css (1)
232-241: Reserve space and ensure stacking for absolute nav; consider adding top and z-index.Absolute positioning without reserving space can overlap the last tab; also, without an explicit stacking context it may sit under other elements.
Apply this diff to improve robustness:
.menuNavButtonsTabbed { position: absolute; right: 0; + top: 0; + z-index: 1; /* keep buttons above tabs */ } @media (max-width: 1024px) { .menuNavButtonsTabbed { position: relative; right: auto; } }Additionally, add right padding to the tabs container when the buttons are present so last tabs remain clickable:
/* outside the changed hunk */ .tabs-container.hasMenuNavButtons { padding-right: 64px; } /* ~2 icon buttons + gap */Note: If you prefer avoiding a new class, we could use :has, but adding a class is safer for older engines.
Please confirm the last tab remains fully clickable when many tabs overflow on desktop and that no overlap occurs at widths just above 1024px.
emhttp/plugins/dynamix/Share.page (1)
61-64: Also mark the container to reserve space for the absolute buttons.Chaining a class onto the container lets CSS add padding-right to avoid covering the last tab.
Apply this diff:
- const target = <?= $tabbed ? '".tabs-container"' : '".title:first .right"' ?>; - $(target).append(prevNextButtons); + const target = <?= $tabbed ? '".tabs-container"' : '".title:first .right"' ?>; + $(target).append(prevNextButtons)<?= $tabbed ? '.addClass("hasMenuNavButtons")' : '' ?>;Please verify on shares with many tabs that the last tab isn’t occluded at desktop widths.
emhttp/plugins/dynamix/DeviceInfo.page (2)
1935-1938: Class injection LGTM; minor a11y nit (optional).The conditional menuNavButtonsTabbed is correct. Optional: add aria-labels on the chevron buttons to complement title for screen readers.
Example:
- var ctrl = "<span class='inline-flex flex-row items-center gap-2<?= $tabbed ? ' menuNavButtonsTabbed' : ''?>'><span class='waitmsg fa fa-circle-o-notch fa-span fa-fw' style='display:none;'></span><a href='/<?=$path?>?name=<?=$prev?>' title='_(previous device)_'>"; - ctrl += "<button type='button' onclick='this.disabled=true;$(\".waitmsg\").show();'><i class='fa fa-chevron-left fa-fw'></i></button></a>"; - ctrl += "<a href='/<?=$path?>?name=<?=$next?>' title='_(next device)_'><button type='button' onclick='this.disabled=true;$(\".waitmsg\").show();'><i class='fa fa-chevron-right fa-fw'></i></button></a></span>"; + var ctrl = "<span class='inline-flex flex-row items-center gap-2<?= $tabbed ? ' menuNavButtonsTabbed' : ''?>'><span class='waitmsg fa fa-circle-o-notch fa-span fa-fw' style='display:none;'></span><a href='/<?=$path?>?name=<?=$prev?>' title='_(previous device)_' aria-label='_(previous device)_'>"; + ctrl += "<button type='button' aria-hidden='true' onclick='this.disabled=true;$(\".waitmsg\").show();'><i class='fa fa-chevron-left fa-fw'></i></button></a>"; + ctrl += "<a href='/<?=$path?>?name=<?=$next?>' title='_(next device)_' aria-label='_(next device)_'><button type='button' aria-hidden='true' onclick='this.disabled=true;$(\".waitmsg\").show();'><i class='fa fa-chevron-right fa-fw'></i></button></a></span>";
1939-1943: Append target switch LGTM; add container class to prevent tab overlap.Adds consistency with Share.page and enables CSS padding-right.
Apply this diff:
- $('.tabs-container').append(ctrl); + $('.tabs-container').append(ctrl).addClass('hasMenuNavButtons');Please confirm device tabs remain fully clickable on desktop when overflowing and that behavior matches Share.page.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/DeviceInfo.page(1 hunks)emhttp/plugins/dynamix/Share.page(2 hunks)emhttp/plugins/dynamix/styles/default-base.css(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#0
File: :0-0
Timestamp: 2025-05-31T05:10:13.120Z
Learning: Squidly271 prefers not to receive CodeRabbit reviews or feedback while PRs are in draft status. Only provide reviews after PRs are moved out of draft.
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
PR: unraid/webgui#2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.
Applied to files:
emhttp/plugins/dynamix/Share.page
🔇 Additional comments (1)
emhttp/plugins/dynamix/Share.page (1)
49-58: Class injection LGTM.
The conditional addition of menuNavButtonsTabbed is correct and localized to tabbed mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
1097-1098: Avoid undefined offset and string-offset warnings when line 2 is missing.
($conf[1][0] ?? '')still indexes$conf[1]before??, which can emit warnings if the file has fewer than 2 lines or the second line is empty. Guard withisset()and usestrncmp()(orstr_starts_with()on PHP 8+) to avoid string-offset access.Apply this diff:
- $conf = is_file("/etc/wireguard/$network.conf") ? file("/etc/wireguard/$network.conf") : []; - if ( ($conf[1][0]??'')=='#') $name .= ' -- '.compress(trim(substr($conf[1],1))); + $conf = is_file("/etc/wireguard/$network.conf") ? file("/etc/wireguard/$network.conf") : []; + if (isset($conf[1]) && strncmp($conf[1], '#', 1) === 0) { + $name .= ' -- ' . compress(trim(substr($conf[1], 1))); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#0
File: :0-0
Timestamp: 2025-05-31T05:10:13.120Z
Learning: Squidly271 prefers not to receive CodeRabbit reviews or feedback while PRs are in draft status. Only provide reviews after PRs are moved out of draft.
🧬 Code graph analysis (1)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
emhttp/plugins/dynamix/include/Helpers.php (1)
compress(235-237)
Squidly271
changed the title
2 participants
Summary by CodeRabbit
Style
Bug Fixes