Implement Structs-Of-Arrays approach - #4652
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe change adds a cached ChangesAlbum Listing v3
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to This PR adds a new album-listing API with request validation, caching, and feature-default behavior. The current head can produce inconsistent created_at responses, silently accept malformed boolean inputs, and leaves a security-relevant descendant check unverified while marking a required analysis gate incomplete. These issues can affect API correctness and access handling, so the PR is not merge-ready until they are fixed or explicitly accepted. Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
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 |
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/specs/4-architecture/features/057-album-listing-v3/plan.md (1)
152-156: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required Markdown footer to every affected document.
docs/specs/4-architecture/features/057-album-listing-v3/plan.md#L152-L156: append the footer.docs/specs/4-architecture/features/057-album-listing-v3/spec.md#L247-L250: append the footer.docs/specs/4-architecture/features/057-album-listing-v3/tasks.md#L98-L101: append the footer.docs/specs/4-architecture/features/058-album-listing-v3-adoption/plan.md#L121-L125: append the footer.docs/specs/4-architecture/features/058-album-listing-v3-adoption/spec.md#L174-L180: append the footer.docs/specs/4-architecture/features/058-album-listing-v3-adoption/tasks.md#L69-L72: append the footer.docs/specs/4-architecture/open-questions.md#L9-L15: append the footer at end of file.docs/specs/_current-session.md#L38-L41: append the footer.Source: Coding guidelines
docs/specs/4-architecture/features/057-album-listing-v3/tasks.md (1)
98-101: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required Markdown footer.
At the end of this file, add
---followed by*Last updated: 2026-08-22*.Source: Coding guidelines
🧹 Nitpick comments (6)
app/Policies/AlbumQueryPolicy.php (2)
673-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the empty default for
$prefix.The docblock states
$prefixmust be non-empty. The signature still defaults it to''. A call that omits$prefixproduces abase_albumsalias that collides with the aliasapplyVisibilityFilter()already adds throughjoinBaseAlbumOwnerId(). Make the parameter required, or default it to'bulk_', so the documented constraint is enforced by the signature.♻️ Proposed change
public function joinBaseAlbumBulkEditFields( AlbumBuilder|FixedQueryBuilder|BaseBuilder $query, string $second = 'inner.id', - string $prefix = '', + string $prefix = 'bulk_', ): void {
678-696: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared
joinSubbody.
joinBaseAlbumOwnerId(),joinBaseAlbumBulkEditFields(), andjoinBaseAlbumSensitive()now contain the samejoinSubcall. Only the column list differs. Extract a private helper that takes the column list,$second, and$prefix. This removes the triplicated join wiring.app/Http/Controllers/Gallery/AlbumListController.php (2)
136-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
&on theCollectionparameters.PHP passes objects by handle.
Collection &$rowsadds no benefit here and none of the three methods reassigns$rows. Remove the reference to keep the signatures conventional.♻️ Proposed change
- private function toAlbumListResource(Collection &$rows, ?User $user): AlbumListResource + private function toAlbumListResource(Collection $rows, ?User $user): AlbumListResource- private function toParentIds(Collection &$rows): array + private function toParentIds(Collection $rows): array- private function toBulkEditResource(Collection &$rows): AlbumListBulkEditFieldsResource + private function toBulkEditResource(Collection $rows): AlbumListBulkEditFieldsResourceAlso applies to: 170-170, 187-187
65-114: 🚀 Performance & Scalability | 🔵 TrivialConfirm the unbounded listing is acceptable at scale.
queryAlbums()returns every visible album with no limit.for_bulk_edit=trueadds three joins plus a users join per row. For an installation with tens of thousands of albums, one uncached request builds the whole array set in memory. The cache reduces repeat cost, but every invalidation event evicts the wholealbum-listing-v3tag, so a write-heavy installation rebuilds the full listing often.Add a metric on the uncached build duration and result size so the cost is observable in production.
tests/Feature_v3/Album/AlbumListV3Test.php (1)
265-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the documented 422 response.
docs/specs/3-reference/api-design.mdline 239 documents 422 whenwith_parent_idorfor_bulk_editis present but not boolean-parseable. No test covers it. A test that sendswith_parent_id=maybeas an admin would pin the validation contract and would also detect a regression where the flag silently falls back tofalse.docs/specs/4-architecture/features/058-album-listing-v3-adoption/plan.md (1)
31-34: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument the existing server-side descendant check.
Move::do()delegates toAlbum::appendNode(), andlychee-org/nestedset11.0.1 prevents moving a node into its descendant. Update the plan, spec, and T-058-03 to record that the client-side exclusion is a UX safeguard, not the security boundary.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1549e254-f35a-4d21-aa4e-93fe8d3addec
📒 Files selected for processing (26)
Makefileapp/Http/Controllers/Gallery/AlbumListController.phpapp/Http/Requests/Gallery/AlbumListV3Request.phpapp/Http/Resources/V3/AlbumListBulkEditFieldsResource.phpapp/Http/Resources/V3/AlbumListResource.phpapp/Listeners/ManagedCacheAlbumListingInvalidator.phpapp/Policies/AlbumQueryPolicy.phpapp/Services/Cache/CacheKeyProvider.phpdocs/specs/3-reference/api-design.mddocs/specs/4-architecture/features/057-album-listing-v3/plan.mddocs/specs/4-architecture/features/057-album-listing-v3/spec.mddocs/specs/4-architecture/features/057-album-listing-v3/tasks.mddocs/specs/4-architecture/features/058-album-listing-v3-adoption/plan.mddocs/specs/4-architecture/features/058-album-listing-v3-adoption/spec.mddocs/specs/4-architecture/features/058-album-listing-v3-adoption/tasks.mddocs/specs/4-architecture/knowledge-map.mddocs/specs/4-architecture/open-questions.mddocs/specs/4-architecture/roadmap.mddocs/specs/_current-session.mdresources/js/lychee.d.tsroutes/api_v3.phptests/Feature_v3/Album/AlbumListV3Test.phptests/Feature_v3/Base/BaseApiWithDataTest.phptests/Unit/Listeners/ManagedCacheAlbumListingInvalidatorTest.phptests/Unit/Policies/AlbumQueryPolicyTest.phptests/Unit/Services/Cache/CacheKeyProviderTest.php
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97332ff4-96ec-497a-8fa2-922c9a9eafb8
📒 Files selected for processing (6)
app/Http/Requests/Gallery/AlbumListV3Request.phpapp/Http/Resources/GalleryConfigs/InitConfig.phpapp/Http/Resources/V3/AlbumListResource.phpapp/Rules/BooleanRule.phpconfig/features.phptests/Feature_v2/StructOfArrayInitTest.php
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Feature_v3/Album/AlbumListV3Test.php (2)
54-69: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore query logging when the callback throws.
$callback()can throw because the callers execute response assertions inside it. If that happens, the cleanup at Lines 68-69 is skipped. Later tests can inherit enabled query logging and accumulate queries. Usetry/finallyto flush and disable query logging on every path.
364-374: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert all public-permission fields covered by this scenario.
The test documents
grants_*behavior but checks onlyis_publicsandis_link_requireds. A regression ingrants_full_photo_accesses,grants_downloads, orgrants_uploadscould pass. Assert all three values from the publicaccess_permissionsrow.The
AlbumListBulkEditFieldsResourcecontract includes all three grant arrays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5380afbb-6837-466f-8d11-dff720d84c12
📒 Files selected for processing (1)
tests/Feature_v3/Album/AlbumListV3Test.php
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
|
@coderabbitai approve? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="LycheeOrg/Lychee"
pr="4652"
echo "== PR reviews =="
gh api "repos/${repo}/pulls/${pr}/reviews" --paginate \
--jq '.[] | {user: .user.login, state: .state, submitted_at: .submitted_at, body: .body}'
echo "== Unresolved review threads =="
gh api graphql \
-F owner="LycheeOrg" \
-F name="Lychee" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
comments(first: 20) {
nodes {
author { login }
body
path
line
originalLine
createdAt
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| .comments.nodes[]
| {author: .author.login, path: .path, line: (.line // .originalLine), body: .body, createdAt: .createdAt}'Length of output: 15751
[approve] You are interacting with an AI system. |
|
@coderabbitai please approve? |
|
✅ Action performedComments resolved and changes approved. |
1 participant
Summary by CodeRabbit