fix: hide view, download and filter buttons when there are no records - #108
fix: hide view, download and filter buttons when there are no records#108maikschneider wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Classes/Controller/AbstractBackendController.php (1)
677-701: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winPrevent unnecessary queries on error paths and improve type safety.
There are two minor improvements to make here:
- Unnecessary queries: When the user has no accessible storage (
getAccessiblePids() === []) but the request specifies a single scope (e.g.,?scope=single&id=123),getRequestedPids()will return that ID. BecauserenderNoStorageResponse()eventually triggers this method to populate view variables, it results in an unnecessary (and technically unauthorized) database query. Short-circuiting ifgetAccessiblePids()is empty prevents this.- Type safety: Depending on the database driver, the count value returned by the query could be a string. Casting the result to
(int)guarantees strict type safety when assigning it to the?int $fullRecordCountproperty. Additionally, usingfetchOne()is cleaner and more idiomatic in Doctrine DBAL for fetching a single scalar value likeCOUNT(*).💡 Proposed refactor
protected function getFullRecordCount(): int { if ($this->fullRecordCount !== null) { return $this->fullRecordCount; } - if ($this->getRequestedPids() === []) { + $pids = $this->getRequestedPids(); + if ($this->getAccessiblePids() === [] || $pids === []) { return $this->fullRecordCount = 0; } $tableName = $this->getTableName(); $qb = $this->connectionPool->getQueryBuilderForTable($tableName); $qb->getRestrictions()->add(GeneralUtility::makeInstance(WorkspaceRestriction::class, $this::WORKSPACE_ID)); $qb->getRestrictions()->removeByType(HiddenRestriction::class); $count = $qb->count('*') ->from($tableName) ->where( - $qb->expr()->in('pid', $qb->quoteArrayBasedValueListToIntegerList($this->getRequestedPids())) + $qb->expr()->in('pid', $qb->quoteArrayBasedValueListToIntegerList($pids)) ) ->executeQuery() - ->fetchNumeric(); + ->fetchOne(); - return $this->fullRecordCount = ($count ? $count[0] : 0); + return $this->fullRecordCount = (int)$count; }🤖 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 `@Classes/Controller/AbstractBackendController.php` around lines 677 - 701, Update getFullRecordCount() to return and cache 0 before querying when getAccessiblePids() is empty, including the existing empty-requested-PIDs case. Fetch the COUNT result with fetchOne() and cast it to int before assigning it to the fullRecordCount property.
🤖 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.
Outside diff comments:
In `@Classes/Controller/AbstractBackendController.php`:
- Around line 677-701: Update getFullRecordCount() to return and cache 0 before
querying when getAccessiblePids() is empty, including the existing
empty-requested-PIDs case. Fetch the COUNT result with fetchOne() and cast it to
int before assigning it to the fullRecordCount property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da313ade-b613-4125-ac43-67ddedbeca87
📒 Files selected for processing (1)
Classes/Controller/AbstractBackendController.php
When a storage contains no records at all, only the empty-state infobox is shown. The view, download and filter doc-header buttons had nothing to act on and were dead controls, so they are now skipped when fullRecordCount is 0. The count is memoized to avoid a second query. The guard uses fullRecordCount, not recordCount, so the buttons still show when records exist but a search returns no matches. Resolves #107
70698d1 to
a97b352
Compare
1 participant
Resolves #107.
Problem
When a storage contains no records at all, the list module shows only the empty-state infobox — but the doc-header still rendered the view, download and filter buttons. With zero records these are dead controls (nothing to filter, no columns to configure, an empty export).
Change
In
AbstractBackendController::configureModuleTemplateDocHeader(), the view dropdown, download and filter-toggle buttons are now only registered whengetFullRecordCount() > 0.getFullRecordCount()is memoized (?int $fullRecordCount) so the guard reuses the count already fetched inassignViewVariables()instead of firing a second query.Why
fullRecordCount, notrecordCountThe guard checks the total record count, not the filtered result count. When records exist but a search/filter returns zero matches, the buttons still show so the user can reset the filter — that path is unchanged.
Verification
ddev php -lpasses.configureModuleTemplateDocHeader()orgetFullRecordCount(), so the guard applies to all list modules.Summary by CodeRabbit