Browser/Plugin improvements: counting nodes, ordering and Nodes section at top/bottom

[rNoz]

What am I doing?

Providing some tweaks to Sabre\DAV\Browser\Plugin.php:

• showing the number of nodes
• ordering by last modified datetime and then by displayPath.
• Nodes section is displayed at the bottom if the number of nodes are greater than 20 (considering UX).

Example with this PR:

1. When accessing a collection with more than 20 nodes (e.g., calendar)

2. When traversing collections with <=20 nodes, so it is comfortable and quick to jump from collections as it is the most used action in these types of resources.

3. When there are no nodes, there is no "Nodes (0)" section generated. Zoomed out:

4. When accessing a collection with more than 20 nodes (e.g., addressbook). Zoomed out:

Why?

More context: issue

I have added a test to evaluate the new compareNodes protected function, as I didn't see a way to easily replace lastmodified values for a set of files and just parsing the table rows of the response body. So I am using reflection to verify all the possibilities when comparing.

Originally, I had another function (see below) to evaluate specifically the counting of nodes, but I think it will be fine just using what is provided by the parent::setUp() + setUp (3 nodes).

public function testCollectionNodes()
{
    $dir = $this->server->tree->getNodeForPath('dir2');
    $dir->createDirectory('subdir2');
    $dir->createDirectory('subdir1');
    $dir->createFile('file2');
    $dir->createFile('file1');

    $request = new HTTP\Request('GET', '/dir2');
    $this->server->httpRequest = ($request);
    $this->server->exec();

    $body = $this->response->getBodyAsString();
    self::assertTrue(false !== strpos($body, 'Nodes (4)'), $body);
    self::assertTrue(false !== strpos($body, '<a href="/dir2/subdir1/">'));
}

All passing:

> phpunit --configuration tests/phpunit.xml tests
PHPUnit 9.6.21 by Sebastian Bergmann and contributors.

....SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS   61 / 1646 (  3%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS............  122 / 1646 (  7%)
.............................................................  183 / 1646 ( 11%)
.............................................................  244 / 1646 ( 14%)
.............................................................  305 / 1646 ( 18%)
.............................................................  366 / 1646 ( 22%)
.............................................................  427 / 1646 ( 25%)
.............................................................  488 / 1646 ( 29%)
.............................................................  549 / 1646 ( 33%)
.............................................................  610 / 1646 ( 37%)
.............................................................  671 / 1646 ( 40%)
.........SSSSSSSSSSSSSSSSSSSSSSSSSSSS........................  732 / 1646 ( 44%)
.............................................................  793 / 1646 ( 48%)
.................................................SSSS........  854 / 1646 ( 51%)
.............................................................  915 / 1646 ( 55%)
.............................................................  976 / 1646 ( 59%)
........................................SSSSSSSSSSSSSSSS..... 1037 / 1646 ( 63%)
............................................................. 1098 / 1646 ( 66%)
..........................................SSSSSSSSSSSSSSSSSS. 1159 / 1646 ( 70%)
............................................................. 1220 / 1646 ( 74%)
............................................................. 1281 / 1646 ( 77%)
............................................................. 1342 / 1646 ( 81%)
............................................................. 1403 / 1646 ( 85%)
............................................................. 1464 / 1646 ( 88%)
............................................................. 1525 / 1646 ( 92%)
......SSSSSSSSSSSSSSSSSSSSSSSSSS............................. 1586 / 1646 ( 96%)
............................................................  1646 / 1646 (100%)

Time: 00:06.069, Memory: 48.00 MB

OK, but incomplete, skipped, or risky tests!
Tests: 1646, Assertions: 2915, Skipped: 198.

Let me know if something else is necessary ;)

[codecov]

Codecov Report

❌ Patch coverage is 98.18182% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.23%. Comparing base (ed901a9) to head (a1c5ea2).
⚠️ Report is 48 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/DAV/Browser/Plugin.php	98.18%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1575   +/-   ##
=========================================
  Coverage     97.23%   97.23%           
- Complexity     2836     2843    +7     
=========================================
  Files           175      175           
  Lines          9028     9043   +15     
=========================================
+ Hits           8778     8793   +15     
  Misses          250      250           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[phil-davis]

@rNoz php-cs-fixer has some minor formatting required.

https://github.com/sabre-io/dav/actions/runs/12096265416/job/34109861744?pr=1575

You can run it locally composer cs-fixer to see what it does, then commit the changes and push.

[phil-davis]

Can you add a test case for when there are more than 20 nodes?
That will exercise this code, and the test can check that the Nodes section comes down the bottom of the HTML.

[rNoz]

Sure. Created testCollectionWithManyNodesGetSubdir. I have also updated the normal case (testCollectionGetRoot) to check that is at the top.

[rNoz]

@phil-davis I needed to update the cs-fixer changes manually, as I have php 8.3 and it warns it is better to execute it with php <= 8.0, and installing that in my system is getting some troubles. I couldn't verify if all is fixed, as the Action/workflow didn't run automatically.

[phil-davis]

@DeepDiver1975 @staabm can either (or both) review this?
Any objections to this sort of rearrangement of the browser plugin rendering?

[staabm]

I have no strong opinion on such changes.
looks useful, but I can't judge how this plugin is used in the wild.

I won't have time to look into more details for a few days

[DeepDiver1975]

Code Review

Refactors generateDirectoryIndex() to extract node-table rendering into generateNodesSection(), add a node count to the heading, place the Nodes section at the top (≤20 children) or bottom (>20), and sort by last-modified descending then natural name order. Nicely done overall — clear extraction and good test coverage.

Strengths

• The extraction of generateNodesSection() is clean and removes the awkward inline block.
• It quietly fixes a latent bug: the original code appended $html .= '</section>'; outside the if ($node instanceof DAV\ICollection) block, emitting a stray closing </section> for non-collection nodes (and an empty <section><h1>Nodes</h1> for empty collections). The new code only emits the section when there are nodes.
• Comparator change is null-safe and uses the spaceship operator correctly for descending order.
• Good use of XPath to assert section placement rather than just substring presence.

Suggestions

• Behavior change — default sort order. Sorting by last-modified-desc instead of alphabetically is a visible change for every consumer of the Browser plugin, not just large collections. Defensible UX-wise, but it's a silent default change — worth a changelog note, and ideally opt-in/configurable since some integrators may rely on alphabetical ordering.
• Magic number 20. $maxNodesAtTopSection = 20; is a local variable. Since the plugin is subclassable and this is a tunable UX knob, a protected class property/constant would let subclasses override it without copying the whole method. It's also duplicated in testCollectionWithManyNodesGetSubdir ($maxNodes = 20) — promoting it would keep test and code in sync.
• Redundant $numSubNodes parameter. generateNodesSection($subNodes, $numSubNodes) could derive the count internally with count($subNodes); passing it in invites the two getting out of sync.
• $numSubNodes = 0; as a control flag (reset after rendering at the top to suppress the bottom render) works but reads as a side-effect. A separate boolean like $nodesRendered would be more self-documenting.

Tests

• testCollectionNodesOrder asserts the comparator returns exactly -1 / 1. The last-modified branch uses <=> (always ±1/0, fine), but the strnatcasecmp() fallback is not contractually guaranteed to return exactly ±1 — it's normalized on modern PHP, so it passes today, but asserting the sign (assertLessThan(0, ...) / assertGreaterThan(0, ...)) for the name-ordering cases would be more robust.
• Typo: $file1_clon → $file1_clone (cosmetic).

Security & performance

• No new injection surface: dynamic output still routes through escapeHTML(), the count is an integer, CSP header unchanged. Performance is equivalent to before.

Verdict: Solid, well-tested improvement that also fixes a pre-existing stray-</section> bug. Main thing to confirm before merge is the silent change to the default sort order (alphabetical → last-modified desc) — is that the intended default, or should it be opt-in?

🤖 Generated with Claude Code