Skip to content

Improve photo tagging #1075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lodewiges wants to merge 2 commits into
base: staging
Choose a base branch
from
Open

Improve photo tagging #1075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e433977
intial commit
lodewiges Feb 25, 2026
2145171
fix lint
lodewiges Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/components/photo-tags/photo-tags.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,24 @@

{{#if this.newTagStyle }}
<div class="photo-tag photo-tag--new" style={{ this.newTagStyle }}>
<div class="photo-tag-controls mb-2">
<label class="form-check form-check-inline">
<Input
@type="checkbox"
@checked={{this.includeOldMembers}}
{{on 'change' this.toggleIncludeOldMembers}}
class="form-check-input"
/>
<span class="form-check-label">Oude leden meenemen</span>
</label>
</div>
<PowerSelect
@options={{this.users}}
@onChange={{this.storeTag}}
@searchEnabled={{true}}
@searchField='fullName'
@registerAPI={{this.openUserSelect}}
@onSearchChange={{this.onSearchChange}}
as |user|
>
{{user.fullName}}
Expand Down
33 changes: 31 additions & 2 deletions app/components/photo-tags/photo-tags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,54 @@
export default class PhotoTags extends Component {
@service store;
@service flashNotice;
@service router;
@tracked newTagX;
@tracked newTagY;
@tracked selectApi;
@tracked showTags = false;
@tracked users = [];

get users() {
return this.store.findAll('user');
get includeOldMembers() {
return this.router.currentRoute.queryParams.includeOldMembers === 'true';
}

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);
}
Comment on lines +22 to +28
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

@action
toggleShowTags() {
this.showTags = !this.showTags;
}

@action
toggleIncludeOldMembers() {
const newValue = !this.includeOldMembers;
this.router.transitionTo({ queryParams: { includeOldMembers: newValue ? 'true' : 'false' } });

Check failure on line 39 in app/components/photo-tags/photo-tags.js

View workflow job for this annotation

GitHub Actions / Lint 

Replace `·queryParams:·{·includeOldMembers:·newValue·?·'true'·:·'false'·}` with `⏎······queryParams:·{·includeOldMembers:·newValue·?·'true'·:·'false'·},⏎···`
this.fetchUsers();
Comment on lines +37 to +40
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
Comment on lines +36 to +41
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


@action
onSearchChange(searchResults) {
// Auto-select the user if there's only one search result
if (searchResults && searchResults.length === 1 && this.selectApi) {
next(this, () => {
this.selectApi.actions.choose(searchResults[0]);
});
}
}

@action
addTag(e) {
if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY)
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if (e.target.tagName.toLowerCase() != 'img' || this.newTagX || this.newTagY)
if (e.target.tagName.toLowerCase() !== 'img' || this.newTagX || this.newTagY)

Copilot uses AI. Check for mistakes.
return;
e.stopPropagation();
this.fetchUsers();
Comment on lines 54 to +58
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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();

Copilot uses AI. Check for mistakes.
let x = (e.offsetX / e.target.width) * 100;
let y = (e.offsetY / e.target.height) * 100;
this.newTagX = x;
Expand Down
6 changes: 5 additions & 1 deletion app/routes/photo-albums/photo-album/photos/photo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { ApplicationRoute } from 'amber-ui/routes/application/application';

export default class PhotoRoute extends ApplicationRoute {
queryParams = {};
queryParams = {
includeOldMembers: {
replace: true

Check failure on line 6 in app/routes/photo-albums/photo-album/photos/photo.js

View workflow job for this annotation

GitHub Actions / Lint 

Insert `,`
}

Check failure on line 7 in app/routes/photo-albums/photo-album/photos/photo.js

View workflow job for this annotation

GitHub Actions / Lint 

Insert `,`
};
Comment on lines +4 to +8
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.


model(params) {
return this.store.findRecord('photo', params.photo_id, params);
Expand Down
26 changes: 26 additions & 0 deletions app/styles/components/photo-tags.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,29 @@
right: 8px;
color: #fff;
}

.photo-tag-controls {
border-bottom: 1px solid rgb(255 255 255 / 20%);
padding: 8px 0;

.form-check {
margin-bottom: 0;
}

.form-check-label {
color: #fff;
margin-bottom: 0;

Check failure on line 59 in app/styles/components/photo-tags.scss

View workflow job for this annotation

GitHub Actions / Lint

Stylelint problem 

Expected "margin-bottom" to come before "color" (order/properties-order)
margin-left: 4px;
font-size: 12px;
}

.form-check-input {
border-color: rgb(255 255 255 / 50%);
background-color: rgb(255 255 255 / 20%);

Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line contains trailing whitespace. Consider removing it for code cleanliness.

Suggested change

Copilot uses AI. Check for mistakes.
&:checked {
border-color: #007bff;
background-color: #007bff;
}
}
}
Loading