Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a checkbox to include/exclude old members in photo tagging, persists the choice via a route query param, fetches users accordingly into a tracked list, and wires a search-change handler to auto-select single search results. Changes
Sequence DiagramsequenceDiagram
actor User
participant Component as Photo Tags Component
participant Router as Router Service
participant API as User Data Service
User->>Component: Toggle "Oude leden meenemen" checkbox
Component->>Router: set query param includeOldMembers
Component->>API: fetchUsers(includeOldMembers)
alt includeOldMembers = true
API-->>Component: return all users
else
API-->>Component: return group "Leden" users
end
Component->>Component: update `@tracked` users
User->>Component: type in PowerSelect search
Component->>Component: onSearchChange(searchResults)
alt searchResults length == 1
Component->>Component: auto-select the single user
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This pull request improves the photo tagging functionality by adding a filter to include or exclude old members when tagging photos. The implementation adds a checkbox UI control, manages the filter state via query parameters, and implements an auto-select feature when search results return exactly one user.
Changes:
- Added a checkbox control to toggle between current members (Leden) and all users when tagging photos
- Implemented query parameter handling to persist the "includeOldMembers" filter state in the URL
- Added auto-select functionality that automatically selects a user when the search narrows down to a single result
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/styles/components/photo-tags.scss | Added styling for the new photo-tag-controls section with checkbox styling that matches the dark overlay theme |
| app/routes/photo-albums/photo-album/photos/photo.js | Configured includeOldMembers query parameter with replace: true to avoid cluttering browser history |
| app/components/photo-tags/photo-tags.js | Converted users getter to tracked property, added fetchUsers method to conditionally load users based on filter, implemented toggle and auto-select actions |
| app/components/photo-tags/photo-tags.hbs | Added checkbox UI control for toggling old members filter and connected onSearchChange handler to PowerSelect |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .form-check-input { | ||
| background-color: rgba(255, 255, 255, 0.2); | ||
| border-color: rgba(255, 255, 255, 0.5); | ||
|
|
There was a problem hiding this comment.
This line contains trailing whitespace. Consider removing it for code cleanliness.
|
|
||
| @action | ||
| addTag(e) { | ||
| if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY) |
There was a problem hiding this comment.
Use strict inequality operator (!==) instead of loose inequality (!=) for consistent comparison. The codebase predominantly uses strict equality operators, as seen in lines 73-74 and throughout other files in the codebase.
| if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY) | |
| if (e.target.tagName.toLowerCase() !== 'img' || this.newTagX || this.newTagY) |
| toggleIncludeOldMembers() { | ||
| const newValue = !this.includeOldMembers; | ||
| this.router.transitionTo({ queryParams: { includeOldMembers: newValue ? 'true' : 'false' } }); | ||
| this.fetchUsers(); |
There was a problem hiding this comment.
The toggleIncludeOldMembers method calls fetchUsers() without awaiting it, which could lead to race conditions where the user list is not yet populated when the component re-renders. Consider awaiting the fetchUsers() call to ensure the users list is populated before the component re-renders. Additionally, consider adding error handling for the fetchUsers() call in case the API request fails.
| addTag(e) { | ||
| if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY) | ||
| return; | ||
| e.stopPropagation(); | ||
| this.fetchUsers(); |
There was a problem hiding this comment.
The fetchUsers() method is called in addTag without awaiting it, which means the PowerSelect dropdown might open with an empty or stale users list. Consider awaiting fetchUsers() before opening the select dropdown to ensure users are loaded first.
| addTag(e) { | |
| if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY) | |
| return; | |
| e.stopPropagation(); | |
| this.fetchUsers(); | |
| async addTag(e) { | |
| if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY) | |
| return; | |
| e.stopPropagation(); | |
| await this.fetchUsers(); |
| async fetchUsers() { | ||
| if (this.includeOldMembers) { | ||
| this.users = await this.store.findAll('user'); | ||
| } else { | ||
| const params = { filter: { group: 'Leden' } }; | ||
| this.users = await this.store.query('user', params); | ||
| } |
There was a problem hiding this comment.
The fetchUsers() method lacks error handling. If the API call fails, there's no indication to the user, and the users list will remain empty. Consider adding try-catch error handling and displaying an error message to the user if the fetch fails.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #1075 +/- ##
===========================================
- Coverage 13.02% 12.96% -0.07%
===========================================
Files 450 450
Lines 3124 3139 +15
===========================================
Hits 407 407
- Misses 2717 2732 +15 ☔ View full report in Codecov by Sentry. 🚀 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/photo-tags/photo-tags.js (1)
53-66:⚠️ Potential issue | 🔴 CriticalRace condition: PowerSelect opens before
fetchUsers()resolves, showing an empty list.
this.fetchUsers()is fire-and-forget. Thenext()callback runs in the very next runloop tick — long before the async fetch can complete — so on the first click the dropdown renders withthis.users === [].
addTagmust beasyncand await the fetch before scheduling the open:🛠️ Proposed fix
`@action` - addTag(e) { + async addTag(e) { if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY) return; e.stopPropagation(); - this.fetchUsers(); + await this.fetchUsers(); let x = (e.offsetX / e.target.width) * 100; let y = (e.offsetY / e.target.height) * 100; this.newTagX = x; this.newTagY = y; next(this, () => { this.selectApi.actions.open(); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/photo-tags/photo-tags.js` around lines 53 - 66, The addTag action (addTag) is opening the PowerSelect before fetchUsers() completes; make addTag async, await this.fetchUsers() after setting newTagX/newTagY (or before calling selectApi.actions.open) so the users list is populated before opening, then call next(this, () => this.selectApi.actions.open()); ensure fetchUsers() returns a promise so awaiting works and preserve the existing early-return checks for e.target and newTagX/newTagY.
🧹 Nitpick comments (2)
app/components/photo-tags/photo-tags.js (1)
22-29:fetchUsers()is called on every photo click — consider loading once on init and only re-fetching on filter change.As-is, every click on the photo triggers a full user fetch. Users should be fetched once when the tag UI initialises (e.g. in a
constructoror a{{did-insert}}action) and only refreshed insidetoggleIncludeOldMembers. This removes the need for thefetchUsers()call inaddTagentirely.Additionally,
fetchUsers()currently has no error handling; a failed request leavesthis.usersunchanged (or empty on first load) with no user feedback.♻️ Sketch of the suggested approach
+ constructor(owner, args) { + super(owner, args); + this.fetchUsers(); + } async fetchUsers() { - if (this.includeOldMembers) { - this.users = await this.store.findAll('user'); - } else { - const params = { filter: { group: 'Leden' } }; - this.users = await this.store.query('user', params); + try { + if (this.includeOldMembers) { + this.users = await this.store.findAll('user'); + } else { + const params = { filter: { group: 'Leden' } }; + this.users = await this.store.query('user', params); + } + } catch (e) { + this.flashNotice.sendError('Gebruikerslijst laden mislukt.'); } } `@action` addTag(e) { if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY) return; e.stopPropagation(); - this.fetchUsers(); // ← remove ... }Also applies to: 53-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/photo-tags/photo-tags.js` around lines 22 - 29, fetchUsers is being called on every photo click; move the initial load to component initialization (constructor or did-insert) so fetchUsers runs once, remove the fetchUsers call from addTag, and only call fetchUsers inside toggleIncludeOldMembers when includeOldMembers changes; update fetchUsers to use a try/catch around the store.findAll/store.query calls, set this.users appropriately on success, and set a fallback (e.g. empty array) or an error flag on failure so UI can show feedback without leaving users undefined.app/styles/components/photo-tags.scss (1)
68-71: Consider using a CSS custom property instead of the hardcoded Bootstrap primary colour.
#007bffis the Bootstrap 4 primary. If the project uses Bootstrap 5 or custom theming, this won't follow theme changes. Usingvar(--bs-primary)(or whatever the project's custom property is) would keep the checked state consistent with the rest of the UI.♻️ Proposed fix
&:checked { - background-color: `#007bff`; - border-color: `#007bff`; + background-color: var(--bs-primary); + border-color: var(--bs-primary); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/styles/components/photo-tags.scss` around lines 68 - 71, The checked-state styles in photo-tags.scss use the hardcoded Bootstrap v4 primary hex (`#007bff`); change both background-color and border-color inside the &:checked rule to use the project's CSS custom property (e.g. var(--bs-primary)) (you can include a fallback like var(--bs-primary, `#007bff`) if desired) so the checked state follows Bootstrap/custom theming consistently; update the &:checked block accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/photo-tags/photo-tags.js`:
- Around line 36-41: The toggleIncludeOldMembers action flips includeOldMembers
but calls this.fetchUsers immediately, risking reading stale query params
because this.router.transitionTo is async and also leaves fetchUsers rejections
unhandled; update toggleIncludeOldMembers (the method) to await
this.router.transitionTo(...) before calling this.fetchUsers(), and call
this.fetchUsers().catch(...) (or use try/catch around await) to handle network
errors; also add the missing trailing comma in the transitionTo queryParams
object to satisfy ESLint/Prettier; ensure fetchUsers itself has proper error
handling as noted.
In `@app/routes/photo-albums/photo-album/photos/photo.js`:
- Around line 4-8: The object literal assigned to queryParams is missing
trailing commas which ESLint/Prettier expect; update the includeOldMembers inner
object and the outer queryParams object to include trailing commas (i.e., add a
trailing comma after the replace property inside the includeOldMembers object
and a trailing comma after the includeOldMembers property in queryParams) so the
assignment to queryParams and the nested object follow the project's
trailing-comma linting rules.
---
Outside diff comments:
In `@app/components/photo-tags/photo-tags.js`:
- Around line 53-66: The addTag action (addTag) is opening the PowerSelect
before fetchUsers() completes; make addTag async, await this.fetchUsers() after
setting newTagX/newTagY (or before calling selectApi.actions.open) so the users
list is populated before opening, then call next(this, () =>
this.selectApi.actions.open()); ensure fetchUsers() returns a promise so
awaiting works and preserve the existing early-return checks for e.target and
newTagX/newTagY.
---
Nitpick comments:
In `@app/components/photo-tags/photo-tags.js`:
- Around line 22-29: fetchUsers is being called on every photo click; move the
initial load to component initialization (constructor or did-insert) so
fetchUsers runs once, remove the fetchUsers call from addTag, and only call
fetchUsers inside toggleIncludeOldMembers when includeOldMembers changes; update
fetchUsers to use a try/catch around the store.findAll/store.query calls, set
this.users appropriately on success, and set a fallback (e.g. empty array) or an
error flag on failure so UI can show feedback without leaving users undefined.
In `@app/styles/components/photo-tags.scss`:
- Around line 68-71: The checked-state styles in photo-tags.scss use the
hardcoded Bootstrap v4 primary hex (`#007bff`); change both background-color and
border-color inside the &:checked rule to use the project's CSS custom property
(e.g. var(--bs-primary)) (you can include a fallback like var(--bs-primary,
`#007bff`) if desired) so the checked state follows Bootstrap/custom theming
consistently; update the &:checked block accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/photo-tags/photo-tags.hbsapp/components/photo-tags/photo-tags.jsapp/routes/photo-albums/photo-album/photos/photo.jsapp/styles/components/photo-tags.scss
| @action | ||
| toggleIncludeOldMembers() { | ||
| const newValue = !this.includeOldMembers; | ||
| this.router.transitionTo({ queryParams: { includeOldMembers: newValue ? 'true' : 'false' } }); | ||
| this.fetchUsers(); | ||
| } |
There was a problem hiding this comment.
Unawaited fetchUsers() may read stale query params and silently drops errors.
this.router.transitionTo(...) is async, so this.includeOldMembers (which reads currentRoute.queryParams) may still reflect the old value when fetchUsers() runs immediately after. Additionally, if the network request fails the rejected promise goes unhandled.
Await the transition first, then fetch (and handle errors):
🛠️ Proposed fix
`@action`
- toggleIncludeOldMembers() {
+ async toggleIncludeOldMembers() {
const newValue = !this.includeOldMembers;
- this.router.transitionTo({ queryParams: { includeOldMembers: newValue ? 'true' : 'false' } });
- this.fetchUsers();
+ await this.router.transitionTo({ queryParams: { includeOldMembers: newValue ? 'true' : 'false' } });
+ await this.fetchUsers();
}Error handling in fetchUsers itself should also be added (see the addTag note).
Also, ESLint/Prettier requires a trailing comma on line 39:
- this.router.transitionTo({ queryParams: { includeOldMembers: newValue ? 'true' : 'false' } });
+ this.router.transitionTo({ queryParams: { includeOldMembers: newValue ? 'true' : 'false' }, });🧰 Tools
🪛 ESLint
[error] 39-39: Replace ·queryParams:·{·includeOldMembers:·newValue·?·'true'·:·'false'·} with ⏎······queryParams:·{·includeOldMembers:·newValue·?·'true'·:·'false'·},⏎···
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/photo-tags/photo-tags.js` around lines 36 - 41, The
toggleIncludeOldMembers action flips includeOldMembers but calls this.fetchUsers
immediately, risking reading stale query params because this.router.transitionTo
is async and also leaves fetchUsers rejections unhandled; update
toggleIncludeOldMembers (the method) to await this.router.transitionTo(...)
before calling this.fetchUsers(), and call this.fetchUsers().catch(...) (or use
try/catch around await) to handle network errors; also add the missing trailing
comma in the transitionTo queryParams object to satisfy ESLint/Prettier; ensure
fetchUsers itself has proper error handling as noted.
| queryParams = { | ||
| includeOldMembers: { | ||
| replace: true | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix missing trailing commas flagged by ESLint/Prettier.
🔧 Proposed fix
queryParams = {
includeOldMembers: {
- replace: true
- }
+ replace: true,
+ },
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| queryParams = { | |
| includeOldMembers: { | |
| replace: true | |
| } | |
| }; | |
| queryParams = { | |
| includeOldMembers: { | |
| replace: true, | |
| }, | |
| }; |
🧰 Tools
🪛 ESLint
[error] 6-6: Insert ,
(prettier/prettier)
[error] 7-7: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/photo-albums/photo-album/photos/photo.js` around lines 4 - 8, The
object literal assigned to queryParams is missing trailing commas which
ESLint/Prettier expect; update the includeOldMembers inner object and the outer
queryParams object to include trailing commas (i.e., add a trailing comma after
the replace property inside the includeOldMembers object and a trailing comma
after the includeOldMembers property in queryParams) so the assignment to
queryParams and the nested object follow the project's trailing-comma linting
rules.
Follows
fixes some small issue with photo tagging
Summary by CodeRabbit
New Features
Style