Develop to main#102
Merged
Merged
Conversation
Contributor
|
🤖 Hi @omatheusmesmo, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @omatheusmesmo, but I was unable to process your request. Please see the logs for more details. |
Contributor
|
🤖 Hi @omatheusmesmo, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @omatheusmesmo, but I was unable to process your request. Please see the logs for more details. |
Contributor
|
🤖 Hi @omatheusmesmo, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @omatheusmesmo, but I was unable to process your request. Please see the logs for more details. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors backend from field to constructor injection; introduces Snowflake ID generation and BIGINT migrations; adds RSA key configuration and CSRF filter for JWT; persists unitPrice/totalPrice and computes shopping list totals; integrates TestContainers for integration tests; migrates frontend components to Angular Signals; removes Gemini CI automations; adds .coderabbit.yaml. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
Contributor
|
🤖 Hi @omatheusmesmo, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @omatheusmesmo, but I was unable to process your request. Please see the logs for more details. |
Contributor
|
🤖 Hi @omatheusmesmo, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @omatheusmesmo, but I was unable to process your request. Please see the logs for more details. |
Contributor
|
🤖 Hi @coderabbitai[bot], I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
|
Contributor
|
🤖 I'm sorry @coderabbitai[bot], but I was unable to process your request. Please see the logs for more details. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
A few possible reasons for this type of failure, given the context of this PR (which modifies the Gemini CLI command files):
Could you please check the workflow run logs and share any relevant error messages? That would help narrow down the root cause. Alternatively, if the Gemini CLI workflows are being intentionally removed/replaced by the new |
Contributor
|
🤖 Hi @omatheusmesmo, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @omatheusmesmo, but I was unable to process your request. Please see the logs for more details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
frontend/src/app/shared/components/list-item/list-item.component.ts (2)
187-193:⚠️ Potential issue | 🟡 MinorMissing
unitPricereset inresetForm.The form reset doesn't include
unitPrice, so after canceling an edit, the unit price field retains its previous value instead of resetting to the default (0).🔧 Proposed fix
resetForm(): void { this.listItemForm.reset({ quantity: 1, + unitPrice: 0, purchased: false, }); this.editingListItemId.set(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/list-item/list-item.component.ts` around lines 187 - 193, The resetForm method currently resets listItemForm fields but omits unitPrice, causing the field to keep previous values; update resetForm to include unitPrice: 0 in the object passed to this.listItemForm.reset(...) and leave the existing this.editingListItemId.set(null) call unchanged so the form and editing state are both fully reset (refer to resetForm, listItemForm, unitPrice, and editingListItemId).
165-185:⚠️ Potential issue | 🔴 CriticalThe
togglePurchasedmethod doesn't send the purchased state, andListItemRequestDTOis missing thepurchasedfield.The method creates an
updatedListItemrequest but never includes thepurchasedvalue. Additionally, theListItemRequestDTOinterface is missing thepurchasedfield entirely, which is needed to support toggling. TheonSubmitmethod also doesn't sendpurchasedwhen editing, indicating a broader issue.To fix this:
- Add
purchased?: boolean;toListItemRequestDTOinfrontend/src/app/shared/interfaces/list-item.interface.ts- Include
purchased: !listItem.purchasedin thetogglePurchasedmethod's request object- Consider adding
purchasedto theonSubmitmethod as well if editing should preserve the purchased state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/list-item/list-item.component.ts` around lines 165 - 185, Add the missing purchased flag to the DTO and include it when toggling: update the ListItemRequestDTO interface (ListItemRequestDTO) to include purchased?: boolean; then modify togglePurchased(listItem: ListItemResponseDTO) to set purchased: !listItem.purchased on the updatedListItem before calling listItemService.updateListItem; also audit the onSubmit method to include the current purchased value when editing so the purchased state is preserved.backend/docker-compose.yml (1)
5-14:⚠️ Potential issue | 🟠 MajorAdd an explicit postgres 17 → 18 migration or reset step.
The docker image switch to
postgres:18introduces a breaking change in the data directory structure. PostgreSQL 18's official Docker image usesPGDATA=/var/lib/postgresql/18/docker(versioned by major version), whereas postgres:17 used a flat structure. Mounting to/var/lib/postgresqlis correct per the image's declaredVOLUME, but any existing postgres:17 cluster in./postgres-datawill not be recognized by postgres:18 without an explicit migration.Postgres 17 → 18 is a major-version upgrade requiring either:
pg_upgrade(in-place migration), ordumpfrom 17 +restoreto 18Simply starting the postgres:18 image against old postgres:17 data will fail to initialize. Add either migration instructions to the backend README or a reset procedure (and document it) so developers upgrading from a local postgres:17 cluster have a clear path forward.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/docker-compose.yml` around lines 5 - 14, The docker-compose change to image: postgres:18 with volumes: - ./postgres-data:/var/lib/postgresql requires an explicit migration or reset path for existing postgres:17 data; update the backend README (or add a scripts/upgrade-postgres.md) to document two supported workflows: (1) in-place migration using pg_upgrade (include commands to run a temporary postgres:17 container, run pg_upgrade to the postgres:18 data layout, and then start image: postgres:18 with PGDATA adjusted), or (2) dump-and-restore (pg_dumpall from a postgres:17 container and restore into a fresh postgres:18 instance), and alternatively document a safe reset procedure that moves or removes ./postgres-data and notes the new PGDATA layout; reference image: postgres:18, the mounted path ./postgres-data:/var/lib/postgresql and PGDATA in the docs so developers know exactly what to run before switching versions.docker-compose.yml (1)
5-14:⚠️ Potential issue | 🟠 MajorDocument the
postgres-datavolume migration path when upgrading from Postgres 17 to 18.Postgres 18 restructures the data directory layout—data now goes under
/var/lib/postgresql/18/dockerinstead of the old flat structure. Reusing the same named volumepostgres-dataacross this upgrade means existing developer volumes will be incompatible: Postgres 18 will either fail to start or reinitialize the cluster, causing data loss.Add to the README or setup guide one of: (1) migration instructions to upgrade the existing volume in place, (2) a new volume name (
postgres-data-v18), or (3) clear documentation that developers must manually drop and recreate the volume on upgrade. Otherwise, developers running the project after this change will encounter silent failures or data loss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 5 - 14, The docker-compose change uses Postgres 18 (image: postgres:18-alpine) with a named volume postgres-data which is incompatible with Postgres 17 layout; update documentation and/or the compose file to avoid silent data loss by either (a) adding clear README setup/migration steps that explain how to migrate an existing postgres-data volume to the Postgres 18 layout (including backing up the old volume, running pg_upgrade or exporting/importing data), (b) changing the compose volume name to a new one like postgres-data-v18 in docker-compose.yml (service shoppmate-db) to force a fresh volume and avoid reusing the old layout, or (c) documenting that developers must drop and recreate the postgres-data volume before starting the Postgres 18 container—pick one approach and add the chosen instructions to the README/setup guide and a short note in docker-compose.yml comments referencing container_name shoppmate-db and the postgres-data volume.frontend/src/app/list/components/list-details/list-details.component.ts (1)
73-85:⚠️ Potential issue | 🟡 MinorLoading state may not reset if
list$fails independently.The
loadingsignal is set tofalseonly viafinalize()onlistItems$. Iflist$fails (e.g., network error) beforelistItems$completes, the loading spinner could remain visible sincelist$errors aren't handled and don't trigger the finalize onlistItems$.Consider adding error handling for
list$or restructuring to useforkJointo coordinate both requests:🛠️ Suggested improvement using forkJoin
loadData(): void { this.loading.set(true); - - // Get the list details - this.list$ = this.shoppingListService - .getAllShoppingLists() - .pipe(map((lists) => lists.find((list) => list.idList === this.listId)!)); - - // Get list items - this.listItems$ = this.listItemService - .getAllListItemsByListId(this.listId) - .pipe(finalize(() => this.loading.set(false))); + + forkJoin({ + lists: this.shoppingListService.getAllShoppingLists(), + items: this.listItemService.getAllListItemsByListId(this.listId) + }).pipe( + finalize(() => this.loading.set(false)) + ).subscribe({ + next: ({ lists, items }) => { + // Store results in signals or subjects + }, + error: () => { + this.snackBar.open('Erro ao carregar dados', 'Fechar', { duration: 3000 }); + } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/list-details/list-details.component.ts` around lines 73 - 85, The loading signal is only cleared via finalize on listItems$, so if list$ (set in loadData using shoppingListService.getAllShoppingLists) errors the spinner can hang; update loadData to coordinate both calls and ensure loading.set(false) always runs by handling errors for list$ or combining the observables (e.g., forkJoin or combineLatest) for shoppingListService.getAllShoppingLists and listItemService.getAllListItemsByListId, and move the finalize/loading.set(false) to that combined stream; reference list$, listItems$, loadData(), shoppingListService.getAllShoppingLists, listItemService.getAllListItemsByListId, and finalize when making the change.backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java (1)
153-164:⚠️ Potential issue | 🟡 MinorAssert the new
unitPricebehavior, not just DTO construction.This test now passes a price, but it never verifies that
editListactually copies that value onto the savedListItem. A regression that silently ignoresunitPricewould still pass here.Suggested assertion
assertNotNull(result); assertEquals(3, result.getQuantity()); assertTrue(result.getPurchased()); + assertEquals(0, BigDecimal.valueOf(10.0).compareTo(result.getUnitPrice())); verify(auditService, times(1)).setAuditData(listItem, false); verify(ListItemRepository, times(1)).save(listItem);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java` around lines 153 - 164, The test editList_Ok builds a ListItemUpdateRequestDTO with a unitPrice but never asserts that service.editList copied it to the entity; update the test (in ListItemServiceTest.editList_Ok) to verify the saved ListItem has the expected unitPrice (use ListItem.getUnitPrice and compare to BigDecimal.valueOf(10.0) or use compareTo == 0), and keep existing verifications for auditService.setAuditData(listItem, false) and ListItemRepository.save(listItem).backend/src/main/java/com/omatheusmesmo/shoppmate/auth/controller/AuthController.java (1)
40-42:⚠️ Potential issue | 🟠 MajorDon't expose the persistence entity from
/auth/sign.Returning
Userhere makes the entity shape part of the public API and risks leaking internal or security-sensitive fields as the model evolves. Map the result toUserResponseDTObefore returning it.Safer response shape
- public ResponseEntity<User> registerUser(`@Valid` `@RequestBody` RegisterUserDTO dto) { + public ResponseEntity<UserResponseDTO> registerUser(`@Valid` `@RequestBody` RegisterUserDTO dto) { var registeredUser = userService.addUser(dto); - return ResponseEntity.ok(registeredUser); + var response = new UserResponseDTO( + registeredUser.getId(), + registeredUser.getFullName(), + registeredUser.getEmail()); + return ResponseEntity.ok(response); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/controller/AuthController.java` around lines 40 - 42, The registerUser method currently returns the persistence entity User from AuthController.registerUser (which calls userService.addUser) — change it to map the returned User to a UserResponseDTO before returning so the persistence model is not exposed; replace the ResponseEntity<User> return type with ResponseEntity<UserResponseDTO>, convert the User result from userService.addUser into a UserResponseDTO (via an existing mapper, a static factory on UserResponseDTO, or a small conversion helper) and return that DTO in ResponseEntity.ok(...).backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.java (1)
21-81:⚠️ Potential issue | 🟠 MajorEnforce the parent
{listId}on every nested item endpoint.The controller declares
/lists/{listId}/items, but onlygetAllListItemsByListId()actually consumeslistId. The read/create/update/delete handlers operate on raw item ids or request bodies only, so/lists/1/items/42can target an item from another list unless a deeper layer rechecks membership. PasslistIdthrough every handler and validate that the item belongs to that list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.java` around lines 21 - 81, The endpoints under /lists/{listId}/items must accept and enforce the listId path variable: update method signatures for getListItemById, addListItem, deleteListItem, and updateListItem to include `@PathVariable` Long listId and pass that listId into service calls (or validate fetched entities) so the controller enforces membership; e.g., change uses of service.findListItemById(id) to a method that accepts (listId, id) or fetch the ListItem and verify listItem.getShoppingList().getId().equals(listId) before returning/changing it, similarly pass listId when calling addShoppItemList, removeList, and editList and build the created location URI with both listId and addedListItem.getId().
🟡 Minor comments (7)
frontend/src/app/shared/components/unit/unit.component.html-69-69 (1)
69-69:⚠️ Potential issue | 🟡 MinorMinor:
unit.idis optional but used for tracking and deletion.The
Unitinterface declaresid?: numberas optional, yet Line 69 tracks byunit.idand Line 79 usesunit.id!for deletion. While units retrieved from the backend should always have an id, consider adding a null guard or ensuring the type reflects that loaded units always have an id.🛡️ Option: Add defensive guard for deleteUnit
- <button mat-icon-button color="warn" (click)="deleteUnit(unit.id!)"> + <button mat-icon-button color="warn" (click)="unit.id && deleteUnit(unit.id)" [disabled]="!unit.id">Alternatively, consider creating a separate interface like
PersistedUnitwith a requiredidfor units returned from the API.Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/unit/unit.component.html` at line 69, The template is tracking and deleting using unit.id even though the Unit type declares id?: number; update the code to avoid runtime nulls by either (a) adding a null-guard in the template and delete handler — e.g., only track/render when unit.id is defined and ensure deleteUnit checks for id before calling the API — or (b) introduce a PersistedUnit interface with id: number and use that type for the units() data source so unit.id is non-optional; modify usages of units(), the `@for` track expression, and deleteUnit to match the chosen approach (references: units(), unit.id, deleteUnit, Unit/PersistedUnit).backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java-22-24 (1)
22-24:⚠️ Potential issue | 🟡 MinorMake
workerIdconfigurable to support distributed deployments.While the
workerIdis currently hardcoded to1L, this is a design issue that should be addressed before deploying multiple application instances. In a single-instance deployment, this poses no immediate risk, but it prevents the application from scaling horizontally without ID collision.Additionally, the fallback path in
SnowflakeIdentifierGenerator.generate()at line 24 creates a newSnowflakeIdGenerator()instance when the Spring context is unavailable, which would also use the same hardcoded worker ID.Consider making the
workerIdconfigurable via environment variables or Spring properties to support future multi-instance deployments:Suggested approach
+import org.springframework.beans.factory.annotation.Value; + `@Component` public class SnowflakeIdGenerator { // ... private final long workerId; - public SnowflakeIdGenerator() { - this.workerId = 1L; + public SnowflakeIdGenerator(`@Value`("${app.snowflake.worker-id:1}") long workerId) { + if (workerId < 0 || workerId >= (1L << WORKER_ID_BITS)) { + throw new IllegalArgumentException("Worker ID must be between 0 and " + ((1L << WORKER_ID_BITS) - 1)); + } + this.workerId = workerId; }Then configure unique worker IDs per instance via environment variables in deployment configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java` around lines 22 - 24, The SnowflakeIdGenerator currently hardcodes workerId to 1L (constructor SnowflakeIdGenerator()) causing ID collisions in multi-instance deployments; make workerId configurable by adding a constructor or setter that accepts a long (or reading from a Spring property/@Value or System.getenv) and use that in SnowflakeIdGenerator instead of the hardcoded 1L, then update the fallback in SnowflakeIdentifierGenerator.generate() to construct SnowflakeIdGenerator with the configured workerId (or read the same env/property there) so instances can supply unique worker IDs via environment variables or Spring properties at startup.frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.html-5-10 (1)
5-10:⚠️ Potential issue | 🟡 MinorTrim the name before enabling save.
[disabled]="!listName()"treats' 'as valid, so this dialog can still submit a blank list name. Use a trimmed value for the disabled state and for the saved payload.Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.html` around lines 5 - 10, The dialog treats whitespace-only names as valid; update the template and save handler to use a trimmed value: change the disabled binding to check trimmed listName (e.g., [disabled]="!(listName() || '').trim()") and ensure the payload or method that persists the name (the save/submit action that reads listName()) sends listName().trim() (or calls listName.set(listName().trim()) before persisting). Reference the listName() getter/call and listName.set(...) usage to locate and fix both the disabled check and the saved payload.frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.ts-70-75 (1)
70-75:⚠️ Potential issue | 🟡 Minor
onSaveallows saving with nullitemId.The
onSave()method doesn't validate thatselectedItemIdis set before closing the dialog. If the user clicks save without selecting an item, it will return{ itemId: null, quantity: 1 }, which could cause issues in the calling component.🛠️ Suggested fix with validation
onSave(): void { + if (this.selectedItemId() === null) { + return; // Or show validation error + } this.dialogRef.close({ itemId: this.selectedItemId(), quantity: this.quantity(), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.ts` around lines 70 - 75, onSave currently closes the dialog even if selectedItemId() is null; update onSave (and/or the Save button state) to validate selectedItemId() before calling this.dialogRef.close({ itemId: this.selectedItemId(), quantity: this.quantity() }); if selectedItemId() is null/undefined, prevent closing and surface an error or return early (e.g. show a validation message or disable the save action) so only a non-null itemId is emitted to the caller.frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.ts-60-64 (1)
60-64:⚠️ Potential issue | 🟡 MinorMissing error handling for item loading.
The
loadItems()method lacks error handling. If the API call fails, the user won't see any feedback and the dialog will appear empty without explanation.🛠️ Suggested fix with error handling
loadItems(): void { - this.itemService.getAllItems().subscribe((items) => { - this.items.set(items); + this.itemService.getAllItems().subscribe({ + next: (items) => this.items.set(items), + error: () => { + // Consider injecting MatSnackBar to show error message + console.error('Failed to load items'); + } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.ts` around lines 60 - 64, The loadItems() method lacks error handling for the itemService.getAllItems() observable; update loadItems() to handle API errors by subscribing with an error handler (or using pipe + catchError) to log the error, set a user-visible error state (e.g., this.loadError = true or set an error message property) and/or show a user notification (e.g., MatSnackBar or dialog message) instead of leaving the dialog blank, while keeping successful behavior where this.items.set(items) is called on next; reference loadItems, itemService.getAllItems, this.items.set and the component's UI/notification mechanism to implement the fix.backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ListItem.java-31-31 (1)
31-31:⚠️ Potential issue | 🟡 MinorAdd
@Columnannotation to document the database contract.The Flyway migration
V8__ADD_PRICE_TO_LIST_ITEMS.sqlcorrectly definesunit_price DECIMAL(19, 2). Add the matching annotation to the entity field for consistency and to prevent schema drift:Suggested change
+ `@Column`(precision = 19, scale = 2) private BigDecimal unitPrice;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ListItem.java` at line 31, Add a JPA `@Column` annotation to the ListItem.entity field unitPrice to match the DB migration: annotate the private BigDecimal unitPrice field in class ListItem with `@Column`(name = "unit_price", precision = 19, scale = 2) so the entity reflects the DECIMAL(19,2) column definition and prevents schema drift.backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java-20-37 (1)
20-37:⚠️ Potential issue | 🟡 MinorFix field naming convention violation.
The field
ListItemRepositoryuses PascalCase instead of camelCase, violating Java naming conventions. This also leads to confusing assignment in the constructor (ListItemRepository = listItemRepository).🐛 Proposed fix
`@Service` public class ListItemService { - private final ListItemRepository ListItemRepository; + private final ListItemRepository listItemRepository; private final ShoppingListService shoppingListService; private final ItemService itemService; private final AuditService auditService; private final ListItemMapper listItemMapper; public ListItemService(ListItemRepository listItemRepository, ShoppingListService shoppingListService, ItemService itemService, AuditService auditService, ListItemMapper listItemMapper) { - ListItemRepository = listItemRepository; + this.listItemRepository = listItemRepository; this.shoppingListService = shoppingListService; this.itemService = itemService; this.auditService = auditService; this.listItemMapper = listItemMapper; }Also update all usages of
ListItemRepositorytolistItemRepositorythroughout the class (lines 47, 65, 72, 83, 88).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java` around lines 20 - 37, The field name 'ListItemRepository' in ListItemService violates Java naming conventions and causes confusing assignments in the constructor; rename the field to 'listItemRepository', update the constructor parameter assignment to this.listItemRepository = listItemRepository, and update every usage of the field in ListItemService (e.g., calls currently referencing ListItemRepository in methods like save, findById, delete, etc.) to use 'listItemRepository' so the class consistently uses camelCase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 20-57: The .coderabbit.yaml uses an unsupported top-level
"instructions" key; remove that key and replace it with a "path_instructions"
array of objects (each with "path" and "instructions") so CodeRabbit will apply
the guidance per-path; specifically, locate the "instructions" block in the file
and convert its contents into one or more entries under "path_instructions" (or
move global guidance into a repository "Code Guidelines" doc) ensuring the YAML
structure matches CodeRabbit's expected schema.
In `@backend/pom.xml`:
- Around line 103-107: The pom dependency declares org.testcontainers:artifactId
"testcontainers-postgresql" which does not exist; update the dependency in the
backend POM by changing the artifactId from "testcontainers-postgresql" to the
correct published artifactId "postgresql" (keep groupId org.testcontainers and
scope test) so the Testcontainers PostgreSQL dependency resolves from Maven
Central.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java`:
- Around line 18-22: RsaKeyConfig currently loads keys from classpath Resources
(privateKeyResource/publicKeyResource) which commits key material; change
configuration to expect external file paths via environment-managed properties
(e.g., require jwt.private-key and jwt.public-key to be file:/ or env-resolved
paths) and update RsaKeyConfig to load Keys from those file-based Resources or
Paths instead of classpath resources; also remove the committed test private key
files from the repository and add their filenames/patterns to .gitignore so key
material is not tracked.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemRequestDTO.java`:
- Around line 8-9: Add validation annotations to the unitPrice field on both
DTOs: in ListItemRequestDTO annotate unitPrice with `@NotNull`(message = "Unit
price cannot be null") and numeric constraints like `@DecimalMin`(value = "0.00",
inclusive = true, message = "Unit price must be non-negative") and
`@Digits`(integer = 10, fraction = 2) to enforce scale/precision; in
ListItemUpdateRequestDTO annotate unitPrice (but do NOT add `@NotNull`) with
`@DecimalMin`(value = "0.00", inclusive = true, message = "Unit price must be
non-negative") and `@Digits`(integer = 10, fraction = 2) so updates validate when
a value is provided. Ensure imports for javax.validation.constraints.* (or
jakarta.validation.constraints.*) are added and match existing DTO annotations.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemUpdateRequestDTO.java`:
- Around line 8-10: Add validation to reject negative unitPrice values on the
DTO: update the ListItemUpdateRequestDTO record to annotate the unitPrice
parameter with an appropriate constraint (e.g., `@DecimalMin`(value = "0.00",
inclusive = true) or `@PositiveOrZero`) so that negative prices are rejected at
request validation; ensure the validation import is added and keep the
constraint consistent with monetary precision expected by
ListItemMapper.calculateTotalPrice() so downstream calculations never receive
negative unitPrice.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListItemMapper.java`:
- Around line 33-49: The mapper currently collapses a missing unitPrice to
BigDecimal.ZERO causing unknown prices to appear free; modify ListItemMapper so
getSafeUnitPrice no longer defaults null to BigDecimal.ZERO (return
listItem.getUnitPrice() as-is) and change calculateTotalPrice to return null
when unitPrice or quantity is null (instead of BigDecimal.ZERO); update the
place that builds ListItemResponseDTO to pass the raw unitPrice (which may be
null) and the possibly-null totalPrice so the response preserves unknown prices.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdentifierGenerator.java`:
- Around line 21-27: The fallback in SnowflakeIdentifierGenerator.generate
creates a new SnowflakeIdGenerator per call (risking duplicate IDs); change it
to use a single shared, thread-safe fallback instance instead of newing one each
time—e.g., add a static, lazily-initialized/thread-safe SnowflakeIdGenerator
singleton in SnowflakeIdentifierGenerator and return singleton.nextId() when
context == null; ensure the singleton preserves sequence/lastTimestamp state and
is safe for concurrent calls, and consider wiring SnowflakeIdGenerator
configuration (workerId) from env/config rather than relying on hardcoded values
in SnowflakeIdGenerator.
In `@backend/src/main/resources/application.properties`:
- Around line 24-28: Remove the classpath defaults from the shared config by
deleting or emptying the jwt.private-key and jwt.public-key entries in
application.properties and move the development defaults into a local-only
profile file (e.g., application-dev.properties) that sets
jwt.private-key=classpath:certs/private_key.pem and
jwt.public-key=classpath:certs/public_key.pem; ensure production uses
environment/profile-specific overrides (absolute paths or env variables) so the
shared config no longer bundles a private key fallback and that components
reading 'jwt.private-key' and 'jwt.public-key' will pick up the profile or
env-provided values at runtime.
In `@frontend/scripts/check-signals.js`:
- Around line 39-58: The mutable-state check falsely flags readonly reactive
fields and initializers like computed()/effect()/input()/input.required();
update the regexes and check order: extend hasUiStateProperty and
mutableUiStatePattern to allow an optional readonly modifier (add "readonly" to
the modifier group alongside public|private|protected), and expand the
mutableUiStatePattern's negative lookahead to exempt initializers matching
signal(...), computed(...), effect(...), input(...) and input.required(...)
(allow optional generic args and whitespace). Also ensure the
usesAsyncPipe/usesSignals check (usesAsyncPipe and usesSignals) runs before or
short-circuits the hard mutable-state error so components using the async pipe
aren't falsely flagged (reference hasUiStateProperty, mutableUiStatePattern,
usesSignals, usesAsyncPipe, computed, effect, input, signal, readonly, filePath,
hasErrors).
In
`@frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.ts`:
- Around line 35-36: The dialog initializes listUserId as signal(0) so in create
mode it emits idUser: 0; update the initialization and onSave path to use the
caller-provided user context instead of the zero default: when the injected data
contains a user id (e.g., data.userId or similar) seed the listUserId signal
from that value (replace readonly listUserId = signal(0) with the provided user
id) and ensure onSave() builds the payload using that signal; alternatively, in
the create branch of onSave() omit idUser from the payload entirely so new-list
submissions do not send idUser: 0. Reference the listUserId signal, the optional
data.list usage, and onSave() to locate the affected code.
In
`@frontend/src/app/list/components/units-management/units-management.component.html`:
- Around line 17-18: The template treats an empty units() array the same whether
data failed to load or there truly are no units; update the template to branch
on the component's error flag from units-management.component.ts: if error is
truthy render a "load failed" UI (with retry action), else if units().length > 0
render the units mat-card, else render the "Nenhuma unidade cadastrada" message.
Locate uses of units() and the error property in the template and replace the
single `@if` (units().length) check with a three-way conditional (error -> show
error, units().length -> show list, otherwise -> show empty-state).
- Around line 37-49: The icon-only buttons invoking editUnit(element) and
deleteUnit(element) lack accessible names; add explicit accessible labels (e.g.,
aria-label attributes like "Edit unit" and "Delete unit" or dynamic labels using
element properties such as aria-label="Edit {{element.name}}") to the <button
mat-icon-button> elements so screen readers announce the action; you may also
optionally keep a matTooltip for visual users but ensure aria-label is present
on both the edit and delete buttons.
In
`@frontend/src/app/list/components/units-management/units-management.component.ts`:
- Around line 59-61: The catchError in loadUnits() currently calls
this.error.set(true) and this.units.set([]), which wipes existing rows on
transient failures; remove the this.units.set([]) call so the previous units
value is preserved and only set this.error.set(true) (and optionally trigger the
snackbar) on failure, ensuring the catchError still returns an appropriate
Observable (e.g., EMPTY or throwError) to satisfy the stream; locate the
catchError inside the loadUnits() method and update it to not clear units but
still surface the error via this.error.set(true)/snackbar.
In `@frontend/src/app/shared/components/list-item/list-item.component.html`:
- Around line 92-101: The template is accessing the signal itself instead of its
value; update the usage of the signal symbol listItems in the
list-item.component.html so that you call the signal to get its array value (use
listItems() instead of listItems) before indexing (e.g., replace listItems[0]
with listItems()[0]); ensure anywhere else in this template that indexes or
reads from listItems uses listItems() and consider guarding against empty arrays
(e.g., check listItems()?.length) if needed.
---
Outside diff comments:
In `@backend/docker-compose.yml`:
- Around line 5-14: The docker-compose change to image: postgres:18 with
volumes: - ./postgres-data:/var/lib/postgresql requires an explicit migration or
reset path for existing postgres:17 data; update the backend README (or add a
scripts/upgrade-postgres.md) to document two supported workflows: (1) in-place
migration using pg_upgrade (include commands to run a temporary postgres:17
container, run pg_upgrade to the postgres:18 data layout, and then start image:
postgres:18 with PGDATA adjusted), or (2) dump-and-restore (pg_dumpall from a
postgres:17 container and restore into a fresh postgres:18 instance), and
alternatively document a safe reset procedure that moves or removes
./postgres-data and notes the new PGDATA layout; reference image: postgres:18,
the mounted path ./postgres-data:/var/lib/postgresql and PGDATA in the docs so
developers know exactly what to run before switching versions.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/controller/AuthController.java`:
- Around line 40-42: The registerUser method currently returns the persistence
entity User from AuthController.registerUser (which calls userService.addUser) —
change it to map the returned User to a UserResponseDTO before returning so the
persistence model is not exposed; replace the ResponseEntity<User> return type
with ResponseEntity<UserResponseDTO>, convert the User result from
userService.addUser into a UserResponseDTO (via an existing mapper, a static
factory on UserResponseDTO, or a small conversion helper) and return that DTO in
ResponseEntity.ok(...).
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.java`:
- Around line 21-81: The endpoints under /lists/{listId}/items must accept and
enforce the listId path variable: update method signatures for getListItemById,
addListItem, deleteListItem, and updateListItem to include `@PathVariable` Long
listId and pass that listId into service calls (or validate fetched entities) so
the controller enforces membership; e.g., change uses of
service.findListItemById(id) to a method that accepts (listId, id) or fetch the
ListItem and verify listItem.getShoppingList().getId().equals(listId) before
returning/changing it, similarly pass listId when calling addShoppItemList,
removeList, and editList and build the created location URI with both listId and
addedListItem.getId().
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java`:
- Around line 153-164: The test editList_Ok builds a ListItemUpdateRequestDTO
with a unitPrice but never asserts that service.editList copied it to the
entity; update the test (in ListItemServiceTest.editList_Ok) to verify the saved
ListItem has the expected unitPrice (use ListItem.getUnitPrice and compare to
BigDecimal.valueOf(10.0) or use compareTo == 0), and keep existing verifications
for auditService.setAuditData(listItem, false) and
ListItemRepository.save(listItem).
In `@docker-compose.yml`:
- Around line 5-14: The docker-compose change uses Postgres 18 (image:
postgres:18-alpine) with a named volume postgres-data which is incompatible with
Postgres 17 layout; update documentation and/or the compose file to avoid silent
data loss by either (a) adding clear README setup/migration steps that explain
how to migrate an existing postgres-data volume to the Postgres 18 layout
(including backing up the old volume, running pg_upgrade or exporting/importing
data), (b) changing the compose volume name to a new one like postgres-data-v18
in docker-compose.yml (service shoppmate-db) to force a fresh volume and avoid
reusing the old layout, or (c) documenting that developers must drop and
recreate the postgres-data volume before starting the Postgres 18 container—pick
one approach and add the chosen instructions to the README/setup guide and a
short note in docker-compose.yml comments referencing container_name
shoppmate-db and the postgres-data volume.
In `@frontend/src/app/list/components/list-details/list-details.component.ts`:
- Around line 73-85: The loading signal is only cleared via finalize on
listItems$, so if list$ (set in loadData using
shoppingListService.getAllShoppingLists) errors the spinner can hang; update
loadData to coordinate both calls and ensure loading.set(false) always runs by
handling errors for list$ or combining the observables (e.g., forkJoin or
combineLatest) for shoppingListService.getAllShoppingLists and
listItemService.getAllListItemsByListId, and move the
finalize/loading.set(false) to that combined stream; reference list$,
listItems$, loadData(), shoppingListService.getAllShoppingLists,
listItemService.getAllListItemsByListId, and finalize when making the change.
In `@frontend/src/app/shared/components/list-item/list-item.component.ts`:
- Around line 187-193: The resetForm method currently resets listItemForm fields
but omits unitPrice, causing the field to keep previous values; update resetForm
to include unitPrice: 0 in the object passed to this.listItemForm.reset(...) and
leave the existing this.editingListItemId.set(null) call unchanged so the form
and editing state are both fully reset (refer to resetForm, listItemForm,
unitPrice, and editingListItemId).
- Around line 165-185: Add the missing purchased flag to the DTO and include it
when toggling: update the ListItemRequestDTO interface (ListItemRequestDTO) to
include purchased?: boolean; then modify togglePurchased(listItem:
ListItemResponseDTO) to set purchased: !listItem.purchased on the
updatedListItem before calling listItemService.updateListItem; also audit the
onSubmit method to include the current purchased value when editing so the
purchased state is preserved.
---
Minor comments:
In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ListItem.java`:
- Line 31: Add a JPA `@Column` annotation to the ListItem.entity field unitPrice
to match the DB migration: annotate the private BigDecimal unitPrice field in
class ListItem with `@Column`(name = "unit_price", precision = 19, scale = 2) so
the entity reflects the DECIMAL(19,2) column definition and prevents schema
drift.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java`:
- Around line 20-37: The field name 'ListItemRepository' in ListItemService
violates Java naming conventions and causes confusing assignments in the
constructor; rename the field to 'listItemRepository', update the constructor
parameter assignment to this.listItemRepository = listItemRepository, and update
every usage of the field in ListItemService (e.g., calls currently referencing
ListItemRepository in methods like save, findById, delete, etc.) to use
'listItemRepository' so the class consistently uses camelCase.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java`:
- Around line 22-24: The SnowflakeIdGenerator currently hardcodes workerId to 1L
(constructor SnowflakeIdGenerator()) causing ID collisions in multi-instance
deployments; make workerId configurable by adding a constructor or setter that
accepts a long (or reading from a Spring property/@Value or System.getenv) and
use that in SnowflakeIdGenerator instead of the hardcoded 1L, then update the
fallback in SnowflakeIdentifierGenerator.generate() to construct
SnowflakeIdGenerator with the configured workerId (or read the same env/property
there) so instances can supply unique worker IDs via environment variables or
Spring properties at startup.
In
`@frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.ts`:
- Around line 70-75: onSave currently closes the dialog even if selectedItemId()
is null; update onSave (and/or the Save button state) to validate
selectedItemId() before calling this.dialogRef.close({ itemId:
this.selectedItemId(), quantity: this.quantity() }); if selectedItemId() is
null/undefined, prevent closing and surface an error or return early (e.g. show
a validation message or disable the save action) so only a non-null itemId is
emitted to the caller.
- Around line 60-64: The loadItems() method lacks error handling for the
itemService.getAllItems() observable; update loadItems() to handle API errors by
subscribing with an error handler (or using pipe + catchError) to log the error,
set a user-visible error state (e.g., this.loadError = true or set an error
message property) and/or show a user notification (e.g., MatSnackBar or dialog
message) instead of leaving the dialog blank, while keeping successful behavior
where this.items.set(items) is called on next; reference loadItems,
itemService.getAllItems, this.items.set and the component's UI/notification
mechanism to implement the fix.
In
`@frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.html`:
- Around line 5-10: The dialog treats whitespace-only names as valid; update the
template and save handler to use a trimmed value: change the disabled binding to
check trimmed listName (e.g., [disabled]="!(listName() || '').trim()") and
ensure the payload or method that persists the name (the save/submit action that
reads listName()) sends listName().trim() (or calls
listName.set(listName().trim()) before persisting). Reference the listName()
getter/call and listName.set(...) usage to locate and fix both the disabled
check and the saved payload.
In `@frontend/src/app/shared/components/unit/unit.component.html`:
- Line 69: The template is tracking and deleting using unit.id even though the
Unit type declares id?: number; update the code to avoid runtime nulls by either
(a) adding a null-guard in the template and delete handler — e.g., only
track/render when unit.id is defined and ensure deleteUnit checks for id before
calling the API — or (b) introduce a PersistedUnit interface with id: number and
use that type for the units() data source so unit.id is non-optional; modify
usages of units(), the `@for` track expression, and deleteUnit to match the chosen
approach (references: units(), unit.id, deleteUnit, Unit/PersistedUnit).
---
Nitpick comments:
In `@backend/.gitignore`:
- Around line 41-42: Replace the broad "*.pem" ignore entry with a path-scoped
pattern so only PEMs under the runtime cert dir are ignored: remove or comment
out the global "*.pem" line and add (or ensure) "src/main/resources/certs/*.pem"
alongside the existing "src/main/resources/certs/" entry; reference the existing
.gitignore patterns "*.pem" and "src/main/resources/certs/" when making the
change.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemResponseDTO.java`:
- Around line 4-5: The imports jakarta.validation.constraints.Min and
jakarta.validation.constraints.NotNull are unused in the record
ListItemResponseDTO; remove these two import statements from the top of the file
to clean up unused imports and avoid warnings, leaving only the necessary
imports (e.g., any remaining imports used by ListItemResponseDTO).
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ShoppingList.java`:
- Around line 24-25: Initialize the items collection in the ShoppingList entity
(replace the current null-initialized List<ListItem> items with a new
ArrayList<>() instance) and add bidirectional helper methods on ShoppingList
named addItem(ListItem item) and removeItem(ListItem item) that both update the
ShoppingList.items collection and set/unset the owning side via
ListItem.setShoppList(this) / ListItem.setShoppList(null); ensure these helpers
are the central place for mutation so callers don't modify items directly and
update equals/hashCode usage if necessary for collection behavior.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListMapper.java`:
- Around line 47-57: The calculateTotalValue(ShoppingList) method is triggering
N+1 queries because it lazily accesses ShoppingList.getItems() during
toResponseDTO() mapping invoked by getAllShoppingLists(); fix by eager-loading
items in the repository or moving total computation to the service: add an
`@EntityGraph` (or a join fetch) to the repository method used by
getAllShoppingLists so items are fetched with the lists, or change the service
to load items for all lists up-front and compute totalValue there (e.g., iterate
lists, call calculateItemTotal per item, then pass the precomputed BigDecimal
into toResponseDTO/ShoppingListResponseDTO construction) so calculateTotalValue
no longer causes extra queries.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/BaseAuditableEntity.java`:
- Around line 18-19: The annotations on BaseAuditableEntity duplicate the
fully-qualified GenericGenerator FQN used in DomainEntity; replace the FQN with
an import for
com.omatheusmesmo.shoppmate.shared.utils.SnowflakeIdentifierGenerator and import
org.hibernate.annotations.GenericGenerator to improve readability, and if both
BaseAuditableEntity and DomainEntity always use the same generator, consolidate
by extracting a shared annotation strategy (e.g., a meta-annotation or a single
annotated mapped superclass) so the `@GeneratedValue`(generator = "snowflake") and
`@GenericGenerator`(name = "snowflake", type = SnowflakeIdentifierGenerator.class)
are defined once and reused.
- Line 5: Remove the unused import of jakarta.persistence.GenerationType from
the BaseAuditableEntity.java file; open the BaseAuditableEntity class, delete
the GenerationType import line (since you now use the custom Snowflake
generator), and either run your IDE/organizer to optimize imports or verify
there are no remaining references to GenerationType in methods/annotations.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/DomainEntity.java`:
- Around line 15-16: Replace the fully qualified annotation on DomainEntity with
an import for org.hibernate.annotations.GenericGenerator and use
`@GenericGenerator`(name = "snowflake", type =
com.omatheusmesmo.shoppmate.shared.utils.SnowflakeIdentifierGenerator.class) to
improve readability; then remove the duplicated configuration in
BaseAuditableEntity by either moving the annotation into a shared superclass or
creating a reusable composed annotation so both DomainEntity and
BaseAuditableEntity (or their common parent) use the same
SnowflakeIdentifierGenerator without repeating the annotation.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java`:
- Around line 29-31: Replace the generic RuntimeException thrown in
SnowflakeIdGenerator when detecting clock drift with a specific custom
exception: create a ClockDriftException (extend RuntimeException or a more
specific unchecked/checked base as your error-handling strategy requires) that
accepts a message and the offending timestamps, then update the check in
SnowflakeIdGenerator (the if (currentTimestamp < lastTimestamp) branch) to throw
new ClockDriftException(...) including contextual data (e.g., currentTimestamp
and lastTimestamp) so callers can catch and handle clock-drift errors
explicitly.
- Around line 48-54: The waitNextMillis method in SnowflakeIdGenerator currently
busy-waits (while loop calling System.currentTimeMillis()), which is
CPU-intensive; modify the loop inside waitNextMillis(long lastTimestamp) to
yield or sleep briefly (e.g., call Thread.yield() or Thread.sleep(1) inside the
loop) so the thread relinquishes CPU while waiting, then return timestamp once
it advances; ensure InterruptedException is handled if you choose
Thread.sleep().
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/user/controller/UserController.java`:
- Around line 19-26: UserController injects UserDetailsService via the
constructor but never uses the userDetailsService field; remove the unused
dependency to clean up the class: delete the userDetailsService field and its
constructor parameter, and adjust the constructor to only accept UserService
(leave the existing userService field and constructor assignment intact) so
UserController no longer stores or requires UserDetailsService.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/auth/service/JwtServiceTest.java`:
- Around line 77-83: The test should avoid Thread.sleep by producing a token
that is already expired or by using an injectable clock; update JwtServiceTest
to call createServiceWithExpiration with a past expiration (e.g., -1L or 0L) or
modify createServiceWithExpiration to accept a Clock so you can supply a fixed
clock set after the token expiry, then generate the token via
JwtService.generateToken(userDetails) and assertFalse on
JwtService.validateToken(token) deterministically (referencing JwtServiceTest,
createServiceWithExpiration, JwtService.generateToken, and
JwtService.validateToken).
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/category/controller/CategoryControllerWithIntegrationTest.java`:
- Line 94: The variable name categoryResponseDTOToUpdated in
CategoryControllerWithIntegrationTest is misleading because it's an instance of
CategoryRequestDTO; rename it to something accurate like
categoryRequestDTOToUpdate or categoryRequestToUpdate wherever declared and
referenced (in the test method within CategoryControllerWithIntegrationTest) so
the type and intent match the identifier and update any subsequent uses to the
new name.
- Around line 134-149: The test methods use inconsistent naming (e.g.,
IntegrationTestPostAddCategory_BadRequest and
IntegrationTestPutEditCategory_NotFound) while other tests use camelCase (e.g.,
testPostAddCategory); rename these methods to follow the project's camelCase
convention (for example testPostAddCategory_BadRequest and
testPutEditCategory_NotFound or simply testPostAddCategoryBadRequest and
testPutEditCategoryNotFound) and update any references to these methods (IDE run
configurations or other tests) so usages of
IntegrationTestPostAddCategory_BadRequest and
IntegrationTestPutEditCategory_NotFound are replaced with the new method names
in CategoryControllerWithIntegrationTest and similar test classes.
- Around line 50-51: Remove the unused static fields categoryResponseDTOCreated
and categoryResponseDTOUpdated from CategoryControllerWithIntegrationTest and
delete any test assignments that set these fields (they are assigned but never
read; `@BeforeEach` resets state so they don't share state between tests); search
for references to categoryResponseDTOCreated and categoryResponseDTOUpdated in
the test class and remove those assignment lines and any related unused imports
or comments.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/item/controller/ItemControllerWithIntegrationTest.java`:
- Around line 145-150: Rename the misnamed test method testDeleteRemoveCategory
in ItemControllerWithIntegrationTest to a name that reflects the endpoint under
test (e.g., testDeleteItemById or testDeleteItem) so it clearly documents that
it exercises DELETE /item/{id}; update the method declaration and any internal
references to the method name to keep consistency (method:
testDeleteRemoveCategory, class: ItemControllerWithIntegrationTest, behavior:
given(specification)...delete("{id}")...statusCode(204)).
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListServiceTest.java`:
- Around line 111-115: In testEditListSuccess update the test to use two
distinct objects: create an existingList instance (the entity returned by
shoppingListRepository.findById(1L)) and a separate updatedList payload with the
new name/data, stub
when(shoppingListRepository.findById(1L)).thenReturn(Optional.of(existingList)),
call shoppingListService.editList(updatedList), and assert that the returned
ShoppingList reflects updatedList values (and optionally verify
shoppingListRepository.save was called with the merged/updated entity). This
ensures editList actually applies the incoming update rather than returning the
fetched instance unchanged.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/shared/testcontainers/AbstractIntegrationTest.java`:
- Line 21: The field declaration uses the raw PostgreSQLContainer type which
causes compiler warnings; update the declaration of postgreSQLContainer (symbol:
PostgreSQLContainer) to use a parameterized type (e.g., PostgreSQLContainer<?> )
and instantiate with the diamond-form constructor (new
PostgreSQLContainer<>("postgres:18")) so the container is not a raw type and the
warning is eliminated.
In `@frontend/src/app/layout/navbar/navbar.component.ts`:
- Around line 40-45: Extract the magic number 768 into a named constant (e.g.,
LARGE_SCREEN_BREAKPOINT or BREAKPOINT_MD) and use that constant in the
isLargeScreen signal initialization and the onResize method; update the readonly
isLargeScreen initialization (currently using window.innerWidth >= 768) and the
onResize setter (currently using (event.target as Window).innerWidth >= 768) to
reference the new constant so the breakpoint is defined once and easy to change.
In
`@frontend/src/app/list/components/items-management/items-management.component.ts`:
- Around line 149-153: The error callback in items-management.component.ts
currently drops the error object; update the subscribe error handler (the arrow
function where this.snackBar.open(...) is called) to accept an error parameter
(e.g., err) and log it (e.g., console.error(err) or forward to your monitoring
service) before showing the snackbar so the user message remains but the
underlying error is captured for debugging; ensure the handler is in the same
method where the delete subscription is made (the subscription that calls
this.snackBar.open on error).
- Around line 84-107: The openAddItemDialog method currently nests a
subscription to itemService.addItem inside dialogRef.afterClosed().subscribe;
refactor it to a single reactive pipeline by using
dialogRef.afterClosed().pipe(filter(result => !!result), switchMap(result =>
this.itemService.addItem(result))).subscribe(...) so you remove the inner
subscribe and still call loadData() and snackBar.open(...) on next/error; keep
references to dialogRef, afterClosed, itemService.addItem, loadData, and
snackBar in the updated implementation.
- Around line 69-82: The loading signal in loadData currently only tracks items$
via finalize on itemService.getAllItems(), so categories and units can still be
loading when loading.set(false) runs; change loadData to combine all three
observables (itemService.getAllItems(), categoryService.getAllCategories(),
unitService.getAllUnits()) using forkJoin (or combineLatest if you need
streaming) and move the finalize(() => this.loading.set(false)) to the combined
observable, then inside the subscription assign this.items$,
this.categories.set(...), and this.units.set(...) from the combined result;
alternatively, introduce separate loading signals for categories/units and clear
them when their respective subscriptions complete if you prefer parallel
independent tracking.
- Line 57: The component injects FormBuilder into the private field fb but never
uses it; remove the unused injection by deleting the private fb =
inject(FormBuilder) declaration and any related unused imports (FormBuilder)
from the component so form logic remains solely in ItemDialogComponent; ensure
compilation by running a quick build or linter to catch any remaining references
to fb.
In `@frontend/src/app/list/components/list-details/list-details.component.ts`:
- Line 60: Remove the unused FormBuilder injection: delete the private fb =
inject(FormBuilder) property and remove the corresponding FormBuilder import
from the top of the file; if there are any leftover references to fb in methods
or templates (e.g., list-details component class or its template), either remove
them or replace with the appropriate implementation, but if none exist simply
remove the injection and import to clean up unused code.
In
`@frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.scss`:
- Around line 1-4: The .w-100 rule mixes width and spacing; split it into two
clear classes (e.g., .full-width { width: 100%; } and .mb-1 { margin-bottom:
1rem; }) or rename to a semantic dialog-specific class (e.g.,
.dialog-full-width) in list-item-dialog.component.scss, then update all usages
of "w-100" in the list-item-dialog component template
(list-item-dialog.component.html) and any tests or styles to use the new class
names (compose .full-width + .mb-1 if you split, or replace with the semantic
name if you renamed) so behavior remains the same but the utility is no longer
overloaded.
In `@frontend/src/app/shared/components/list-item/list-item.component.scss`:
- Around line 33-55: Replace the hardcoded colors in .list-summary-card,
.summary-label, and .summary-value with your Angular Material theme variables
(e.g., use the Material primary palette for .summary-value and the theme
foreground/neutral for background and label) so the component follows the app
theme; update the SCSS for .list-summary-card, .summary-row, .summary-label, and
.summary-value to reference the project’s theme tokens (either
mat-color($your-theme-primary, ...) or the global CSS custom properties used for
Material theming) instead of `#f5f5f5`, rgba(0,0,0,0.6), and `#3f51b5`, and ensure
the component SCSS imports/has access to the theme variables used by the
application.
In `@frontend/src/app/shared/components/unit/unit.component.ts`:
- Around line 84-96: Cache the repeated signal read by calling editingUnitId()
once into a local constant (e.g., const editingId = this.editingUnitId()) at the
start of the method, then use editingId when setting unitData.id, choosing the
operation (unitService.updateUnit / unitService.addUnit), and when calling
snackBar.open; this avoids multiple signal invocations and improves readability
while keeping the existing logic and usages of unitData, operation,
unitService.updateUnit, unitService.addUnit, and snackBar.open unchanged.
In `@frontend/src/app/shared/services/auth.service.ts`:
- Around line 60-68: In the register error handler inside AuthService (the
register method where the current catchError swallows the error), inspect the
incoming error object in the catchError callback and map common HTTP statuses to
distinct user-facing messages (e.g., 409 -> "Email já cadastrado", 400 -> "Dados
inválidos, verifique os campos", otherwise -> generic server error like "Erro ao
registrar usuário. Tente novamente mais tarde."); return throwError(() => new
Error(mappedMessage)) while avoiding logging sensitive details; update the
catchError closure to accept the error parameter and branch on error.status to
choose the appropriate message.
Contributor
|
🤖 Hi @coderabbitai[bot], I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
|
Contributor
|
🤖 I'm sorry @coderabbitai[bot], but I was unable to process your request. Please see the logs for more details. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3552
Root Cause: The
This is happening because:
Resolution Options:
Since the intent of this PR is clearly to replace Gemini CLI with CodeRabbit (evidenced by the |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/app/shared/services/auth.service.ts (1)
61-68:⚠️ Potential issue | 🟡 MinorError information is discarded, hindering debuggability.
The
catchErrorcallback ignores the error parameter entirely, discarding valuable diagnostic information. This is also inconsistent with thelogin()method (line 41) which logs errors viaconsole.error.Consider capturing the error for logging to maintain debuggability:
Proposed fix
- catchError(() => { + catchError((error: HttpErrorResponse) => { + console.error('Registration error:', error); return throwError( () => new Error( 'Erro ao registrar usuário. Tente novamente mais tarde.', ), ); }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/services/auth.service.ts` around lines 61 - 68, In the register() catchError block, stop discarding the caught error: change the catchError callback to accept the error parameter (e.g., err), log it the same way as login() (e.g., console.error('register error', err) or use the app logger) and then rethrow the user-facing Error with throwError(() => new Error('Erro ao registrar usuário. Tente novamente mais tarde.')); update the catchError surrounding function name register and ensure you only expose the friendly message to callers while preserving the original error in logs for debugging.frontend/src/app/list/components/list-share-dialog/list-share-dialog.component.ts (1)
127-131:⚠️ Potential issue | 🟡 MinorGuard against undefined
user.idbefore using non-null assertion.The
Userinterface declaresidas optional (id?: number). The non-null assertionuser.id!could passundefinedto the API if a user exists without an ID.🛡️ Proposed fix to add validation
const request = { idList: this.data.listId, - idUser: user.id!, + idUser: user.id ?? 0, permission: this.shareForm.value.permission, }; + + if (!user.id) { + this.snackBar.open('User ID not found', 'Close', { duration: 3000 }); + return; + }Or check before constructing the request:
+ if (!user.id) { + this.snackBar.open('User ID not found', 'Close', { duration: 3000 }); + return; + } + const request = { idList: this.data.listId, - idUser: user.id!, + idUser: user.id, permission: this.shareForm.value.permission, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/list-share-dialog/list-share-dialog.component.ts` around lines 127 - 131, Guard against a possibly undefined user.id before using the non-null assertion when building the request: in list-share-dialog.component.ts, validate user.id (the User.id property) before constructing the request object (which currently sets idUser: user.id!), and handle the missing id (e.g., abort/return, show an error to the user, or disable submission) so you never send undefined to the API; ensure shareForm.value.permission and this.data.listId remain unchanged and only build/send the request when user.id is a defined number.frontend/src/app/shared/components/list-item/list-item.component.ts (1)
187-193:⚠️ Potential issue | 🟡 Minor
resetFormdoesn't resetunitPriceto its default value.The form reset specifies
quantity: 1andpurchased: falsebut omitsunitPrice. This could leave a stale unit price value when switching from edit mode to add mode.🐛 Proposed fix
resetForm(): void { this.listItemForm.reset({ quantity: 1, + unitPrice: 0, purchased: false, }); this.editingListItemId.set(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/list-item/list-item.component.ts` around lines 187 - 193, The resetForm method currently resets listItemForm with quantity and purchased but omits unitPrice, leaving a stale value; update resetForm (and the call to this.listItemForm.reset) to include unitPrice set to its default (e.g., 0 or whatever the form’s initial default is) and then clear editingListItemId as before so new adds start with the correct unitPrice.docker-compose.yml (1)
5-14:⚠️ Potential issue | 🟠 MajorHandle the PostgreSQL 17 → 18 volume migration explicitly.
Upgrading from
postgres:17-alpinetopostgres:18-alpinechanges the volume mount from/var/lib/postgresql/datato/var/lib/postgresql. The new mount point is correct for PostgreSQL 18, but reusing the same named volume (postgres-data) will render existing local data inaccessible. PostgreSQL 18+ changedPGDATAto/var/lib/postgresql/18/docker, so the old layout stored atpostgres-data:/var/lib/postgresql/datawon't be found at the new mount location without explicit migration. Either document a one-time migration step forpostgres-databefore applying this change, or revert topostgres:17-alpineuntil the upgrade path is integrated into the dev setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 5 - 14, The docker-compose change updates image to postgres:18-alpine and the volume postgres-data mount to /var/lib/postgresql which will make existing data stored under /var/lib/postgresql/data inaccessible; either revert image back to postgres:17-alpine and keep the current volume mapping until a migration is planned, or add an explicit one-time migration step to the repo docs and orchestration (referencing the image tag postgres:18-alpine, the named volume postgres-data, and the new mount /var/lib/postgresql) that instructs how to copy or relocate data from the old path (/var/lib/postgresql/data or legacy PGDATA) into the new PGDATA layout before bringing up the service.
♻️ Duplicate comments (8)
frontend/src/app/list/components/units-management/units-management.component.html (2)
37-50:⚠️ Potential issue | 🟠 MajorLabel the icon-only action buttons for accessibility.
These buttons have no accessible name, so assistive technology will announce generic "button" controls instead of the intended edit/delete actions.
,
♿ Proposed fix to add aria-labels
<button mat-icon-button color="primary" + [attr.aria-label]="'Editar unidade ' + element.name" (click)="editUnit(element)" > <mat-icon>edit</mat-icon> </button> <button mat-icon-button color="warn" + [attr.aria-label]="'Excluir unidade ' + element.name" (click)="deleteUnit(element)" > <mat-icon>delete</mat-icon> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/units-management/units-management.component.html` around lines 37 - 50, The icon-only mat-icon-button elements that call editUnit(element) and deleteUnit(element) lack accessible names; add descriptive aria-label attributes (e.g., aria-label="Edit unit" and aria-label="Delete unit") to those buttons so screen readers announce the action; ensure the labels describe the action and, if needed, include contextual info from element (or use i18n keys) while keeping the (click) handlers editUnit and deleteUnit unchanged.
17-20:⚠️ Potential issue | 🟠 MajorDifferentiate "load failed" from "no units yet."
The component has an
errorsignal (set in the TS file on fetch failure), but this template routes every empty array to "Nenhuma unidade cadastrada." When the initial load fails, that message is misleading and hides the actual error from the user.,
🐛 Proposed fix to handle error state
- `@if` (units().length) { + `@if` (error()) { + <mat-card appearance="outlined" class="empty-state"> + <mat-card-content> + <p>Não foi possível carregar as unidades.</p> + <button + mat-raised-button + color="primary" + (click)="loadUnits()" + > + Tentar novamente + </button> + </mat-card-content> + </mat-card> + } `@else` if (units().length) { <mat-card appearance="outlined">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/units-management/units-management.component.html` around lines 17 - 20, The template currently treats any empty units() array as "Nenhuma unidade cadastrada." instead of surfacing fetch errors; update the template logic to check the component's error signal (the error reactive signal set in the TS) before deciding what to show: if error is set render a user-facing error message (or localized string) indicating load failed, else if units().length render the table, else render "Nenhuma unidade cadastrada."; locate the relevant template around units(), the error signal in the component TS (error) and adjust the *ngIf/*ngIf-else structure so the error branch is evaluated first.frontend/src/app/shared/components/list-item/list-item.component.html (1)
92-101:⚠️ Potential issue | 🔴 CriticalBug: Missing signal accessor parentheses on
listItems.Line 97 uses
listItems[0]butlistItemsis a signal. This should belistItems()[0]to access the array. Without the parentheses, this accesses a property on the signal function itself, which will beundefinedand cause a runtime error.Additionally, accessing
listItems()[0]without a length guard could cause an error if the array is somehow empty at this point (though the@elseblock guards against this in the current flow).,
🐛 Proposed fix
<mat-card-content> <div class="summary-row"> <span class="summary-label">Total List Value:</span> <span class="summary-value">{{ - listItems[0].shoppingList.totalValue | currency + listItems()[0].shoppingList.totalValue | currency }}</span> </div> </mat-card-content>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/list-item/list-item.component.html` around lines 92 - 101, The template is accessing the signal directly — change the usage of the signal `listItems` in the Total List Value binding to call the signal (`listItems()`), i.e. read the array via `listItems()` and then access the first element's `shoppingList.totalValue`; also add a defensive guard (either optional chaining or a length check on `listItems()` before accessing index 0) to avoid runtime errors when the array is empty, targeting the binding that references `listItems[0].shoppingList.totalValue`.frontend/src/app/list/components/units-management/units-management.component.ts (1)
59-70:⚠️ Potential issue | 🟠 MajorPreserve the last successful list when reloads fail.
loadUnits()is called on initial load and after CRUD operations (lines 88, 107, 131). If a follow-up request fails,this.units.set([])wipes out already loaded rows, causing the table to disappear due to a transient backend error. Keep the previousunitsvalue and surface the failure througherror/snackbar instead.,
🐛 Proposed fix
catchError(() => { this.error.set(true); - this.units.set([]); this.snackBar.open( 'Erro ao carregar unidades. Tente novamente mais tarde.', 'Fechar',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/units-management/units-management.component.ts` around lines 59 - 70, The catchError in loadUnits() currently clears the existing list by calling this.units.set([]) on error; remove that line so we preserve the last successful units state, still set this.error to true and keep the snackBar.open call to surface the failure; leave the returned of([]) (or return a safe fallback) so the observable chain stays valid but do not mutate this.units in the error handler.backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListItemMapper.java (1)
33-49:⚠️ Potential issue | 🟠 MajorPreserve unknown prices instead of coercing them to zero.
Lines 33-49 still collapse
nulland0into the same value, so an item with no recorded price is serialized as free and gets atotalPriceof0. PasslistItem.getUnitPrice()through unchanged and returnnullfromcalculateTotalPrice()when price or quantity is missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListItemMapper.java` around lines 33 - 49, The mapper is currently coercing null prices to BigDecimal.ZERO; update ListItemMapper so getSafeUnitPrice no longer converts null to ZERO (pass listItem.getUnitPrice() through unchanged) and change calculateTotalPrice(BigDecimal unitPrice, Integer quantity) to return null when unitPrice or quantity is null instead of BigDecimal.ZERO; adjust the creation of ListItemResponseDTO to pass the nullable unitPrice and the possibly-null totalPrice so unknown prices are preserved in serialization (ensure any downstream code or DTO constructors accept nullable values).backend/src/main/resources/application.properties (1)
24-28:⚠️ Potential issue | 🟠 MajorShared config still bakes in a default RSA keypair path.
This is the same issue raised earlier: keeping
jwt.private-keyandjwt.public-keypointed at bundled classpath resources makes it too easy to ship the same keypair across environments. Please move these defaults to a local-only profile and keep shared config env/profile-driven.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/resources/application.properties` around lines 24 - 28, The shared application.properties currently hardcodes jwt.private-key and jwt.public-key to classpath certs; move these two properties out of the shared config into a local-only profile (e.g., application-local.properties) so production/shared config no longer contains bundled key paths, and make jwt.private-key and jwt.public-key environment/profile-driven (e.g., via SPRING_PROFILES or externalized config) with comments/documentation instructing teams to supply their own secure paths or secrets; update any startup docs/tests that assume the classpath keys and ensure code that reads jwt.private-key/jwt.public-key still works when values are provided via environment or profile..coderabbit.yaml (1)
20-57:⚠️ Potential issue | 🟠 Major
reviews.instructionswon't be applied.Under the schema attached to this review,
reviewsacceptspath_instructionsfor review guidance, not a free-forminstructionskey. This block will be ignored, so the backend/frontend guidance will not affect reviews until it is moved underreviews.path_instructionsor into a repository code-guidelines document.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 20 - 57, The YAML uses a top-level reviews.instructions key which is ignored by the schema; update the file so the guidance is placed under the schema-expected key (move the current reviews.instructions block into reviews.path_instructions) or relocate it into your repository code-guidelines document, ensuring the policy text for Backend/Frontend is nested under reviews.path_instructions and remove the duplicate/incorrect reviews.instructions entry; reference the reviews.instructions and reviews.path_instructions keys in your change so reviewers can verify the fix.backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java (1)
18-22:⚠️ Potential issue | 🔴 CriticalDon't let signing keys come from classpath-managed resources.
This still allows the app to boot with a repository-shipped PEM, which turns the JWT signing key into deployable application content. For private keys, require externally managed file/env-backed locations and reject classpath values at startup.
Also applies to: 25-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java` around lines 18 - 22, RsaKeyConfig currently accepts classpath-managed Resources for privateKeyResource and publicKeyResource which allows repository-shipped PEMs; change the config to require externally-managed file/env-backed key locations and fail fast if classpath/embedded resources are provided: update RsaKeyConfig to use file-based properties (e.g., jwt.private-key-file / jwt.public-key-file) or accept Resource but validate at startup that Resource.getURL().getProtocol() is not "classpath" or "jar" (or that it is a readable java.io.File/Path), and throw an IllegalStateException with a clear message if the keys are not filesystem/env-backed; adjust any key-loading methods in RsaKeyConfig (e.g., the methods that load the private/public key bytes or build the KeyPair) to use the validated file paths and to check readability/permissions before proceeding.
🧹 Nitpick comments (15)
frontend/src/app/shared/services/auth.service.ts (1)
86-90: Hardcoded placeholder for user ID may cause incorrect behavior.
getCurrentUserId()always returns1, which could lead to authorization issues or data being associated with the wrong user. The TODO indicates this needs proper JWT token parsing.Would you like me to help implement JWT token decoding to extract the actual user ID? I can open an issue to track this task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/services/auth.service.ts` around lines 86 - 90, getCurrentUserId() currently returns a hardcoded 1; replace this placeholder with real JWT parsing: read the auth token used by the app (e.g., from localStorage/session or the AuthService method that stores tokens), base64-decode the JWT payload, safely parse the JSON and extract the user identifier claim (commonly "sub", "userId" or "id"), convert it to a number, and return it; handle missing/invalid tokens and parsing errors by returning null or throwing a controlled error and update getCurrentUserId's signature/consumers accordingly (refer to the getCurrentUserId function and any AuthService token getters to locate where to implement this).backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/BaseAuditableEntity.java (2)
18-19: Consider using imports for cleaner annotation declarations.Same suggestion as
DomainEntity.java- using imports would improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/BaseAuditableEntity.java` around lines 18 - 19, The annotation uses fully-qualified names which reduces readability; add imports for org.hibernate.annotations.GenericGenerator and com.omatheusmesmo.shoppmate.shared.utils.SnowflakeIdentifierGenerator and then replace the fully-qualified annotation on BaseAuditableEntity so it reads `@GeneratedValue`(generator = "snowflake") and `@GenericGenerator`(name = "snowflake", type = SnowflakeIdentifierGenerator.class) to match the style used in DomainEntity.
5-5: Unused import.
GenerationTypeis no longer used after switching to the Snowflake generator.♻️ Remove unused import
-import jakarta.persistence.GenerationType;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/BaseAuditableEntity.java` at line 5, Remove the unused import GenerationType from BaseAuditableEntity.java: locate the import statement "import jakarta.persistence.GenerationType;" in the BaseAuditableEntity class and delete it (the Snowflake generator is now used instead, so GenerationType is not referenced anywhere); ensure no other code in BaseAuditableEntity references GenerationType after removal and run a quick compile to verify.backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java (2)
48-54: Busy-wait loop consumes CPU unnecessarily.The
waitNextMillismethod spins continuously, consuming CPU cycles. For high-throughput scenarios where sequence exhaustion is rare, this is acceptable, butThread.sleep(1)would be more CPU-friendly.♻️ Optional: Add brief sleep to reduce CPU usage
private long waitNextMillis(long lastTimestamp) { long timestamp = System.currentTimeMillis(); while (timestamp <= lastTimestamp) { + try { + Thread.sleep(1); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while waiting for next millisecond", e); + } timestamp = System.currentTimeMillis(); } return timestamp; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java` around lines 48 - 54, The busy-wait in SnowflakeIdGenerator.waitNextMillis continuously spins; modify waitNextMillis to sleep briefly inside the loop (e.g., call Thread.sleep(1) or Thread.yield()) to reduce CPU usage, and catch InterruptedException to restore the thread interrupt status (Thread.currentThread().interrupt()) before returning or continuing so interruption semantics are preserved.
29-31: Consider using a more specific exception type.
RuntimeExceptionis generic and may be caught unintentionally by broad catch blocks. A custom exception orIllegalStateExceptionwould be more appropriate for this clock-skew scenario.♻️ Use IllegalStateException
if (currentTimestamp < lastTimestamp) { - throw new RuntimeException("Clock moved backwards. Refusing to generate id."); + throw new IllegalStateException("Clock moved backwards. Refusing to generate id."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java` around lines 29 - 31, Replace the generic RuntimeException thrown in SnowflakeIdGenerator when clock moves backwards with a more specific exception type (e.g., IllegalStateException or a custom ClockSkewException) so callers can handle this distinct error; update the throw in the method that compares currentTimestamp and lastTimestamp (the block that currently does throw new RuntimeException("Clock moved backwards. Refusing to generate id.");) to throw the chosen specific exception and adjust any surrounding javadoc/comments or tests to expect the new exception type.backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/DomainEntity.java (1)
15-16: Consider using imports for cleaner annotation declarations.The fully qualified class names work but reduce readability. Consider importing the annotation and generator class.
♻️ Suggested refactor
Add imports at the top of the file:
import org.hibernate.annotations.GenericGenerator; import com.omatheusmesmo.shoppmate.shared.utils.SnowflakeIdentifierGenerator;Then simplify the annotations:
`@Id` `@GeneratedValue`(generator = "snowflake") - `@org.hibernate.annotations.GenericGenerator`(name = "snowflake", type = com.omatheusmesmo.shoppmate.shared.utils.SnowflakeIdentifierGenerator.class) + `@GenericGenerator`(name = "snowflake", type = SnowflakeIdentifierGenerator.class) `@Column`(name = "id") private Long id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/DomainEntity.java` around lines 15 - 16, Replace the fully-qualified Hibernate annotation usage in DomainEntity (the `@org.hibernate.annotations.GenericGenerator` reference) by adding imports for org.hibernate.annotations.GenericGenerator and com.omatheusmesmo.shoppmate.shared.utils.SnowflakeIdentifierGenerator at the top of the file, then update the annotation on the identifier to use `@GenericGenerator`(name = "snowflake", type = SnowflakeIdentifierGenerator.class) alongside the existing `@GeneratedValue`(generator = "snowflake") to improve readability.frontend/src/app/shared/components/unit/unit.component.html (1)
69-69: Consider handling optionalidin track expression.The
Unitinterface definesidas optional (id?: number). While units loaded from the backend should always have anid, usingtrack unit.id(line 69) with a potentially undefined value could cause tracking issues. Similarly,deleteUnit(unit.id!)(line 79) relies on a non-null assertion.If the backend always provides
idfor persisted units, this is fine in practice. Otherwise, consider using a fallback track expression:- `@for` (unit of units(); track unit.id) { + `@for` (unit of units(); track unit.id ?? $index) {Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/unit/unit.component.html` at line 69, Unit.id is optional but the template uses track unit.id and calls deleteUnit(unit.id!), which can break if id is undefined; update the tracking expression to use a safe fallback (e.g., track unit.id ?? index/temporaryId) and avoid non-null assertions in deleteUnit calls by either passing the whole unit (deleteUnit(unit)) or guarding the call (only call deleteUnit when unit.id != null) and updating the deleteUnit signature to accept undefined or a Unit to handle unpersisted items; locate usages around units(), track unit.id and deleteUnit(unit.id!) to apply these changes.frontend/src/app/list/components/list-details/list-details.component.ts (1)
73-85: Loading state may not sync correctly withAsyncPipesubscription.The
finalizeoperator onlistItems$only executes when the observable is subscribed (likely viaAsyncPipein the template). If the template subscribes lazily or re-subscribes, theloadingsignal may not accurately reflect the actual loading state.Consider whether this pattern works correctly with your template's usage of
list$andlistItems$viaAsyncPipe, or consider fully migrating to signals for data as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/list-details/list-details.component.ts` around lines 73 - 85, The loading state set by loading.set(true) in loadData() may not be cleared reliably because finalize() on listItems$ only runs on subscription completion; update loadData() to control loading explicitly (e.g., subscribe once or use operators that emit on first response) — for example, call listItemService.getAllListItemsByListId(this.listId).pipe(take(1), tap(() => this.loading.set(false)), shareReplay(1)) and assign that to listItems$, or alternatively convert list$ and listItems$ to signals and set loading.set(false) when the first values arrive; ensure you reference loadData, listItems$, list$, listId, loading, shoppingListService and listItemService when making the change.backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java (2)
28-29: Naming convention: mock field should use camelCase.The mock field
ListItemRepositorystarts with uppercase, violating Java naming conventions. This should belistItemRepository.Suggested fix
`@Mock` - private ListItemRepository ListItemRepository; + private ListItemRepository listItemRepository;Then update all references in the test methods accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java` around lines 28 - 29, In ListItemServiceTest rename the mock field from ListItemRepository to listItemRepository to follow Java camelCase conventions; update the field declaration annotated with `@Mock` and change all usages/references within the test class (e.g., in setup and test methods that call ListItemRepository) to the new symbol listItemRepository so compilation and Mockito wiring remain correct.
152-165: Test doesn't verifyunitPriceis applied during edit.The test creates an update DTO with a price value but doesn't assert that
listItem.getUnitPrice()is set correctly after the edit operation. Consider adding verification for the new price field.Suggested addition
assertNotNull(result); assertEquals(3, result.getQuantity()); assertTrue(result.getPurchased()); + assertEquals(BigDecimal.valueOf(10.0), result.getUnitPrice());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java` around lines 152 - 165, The test editList_Ok in ListItemServiceTest currently omits asserting the updated unitPrice; update the test that constructs ListItemUpdateRequestDTO(...) to also assert that after calling service.editList(1L, updateDTO) the returned ListItem (result or listItem) has its unitPrice equal to the DTO value (e.g., BigDecimal.valueOf(10.0)), and ensure any mocks still verify auditService.setAuditData(listItem, false) and ListItemRepository.save(listItem) as before; reference the editList method behavior and the ListItemUpdateRequestDTO.unitPrice field when adding the new assertion.frontend/src/app/list/components/items-management/items-management.component.ts (1)
69-81: Split item refresh from reference-data loading.
loadData()is called after add/update/delete, so each item mutation now also re-fetches categories and units even though those lists did not change. That adds avoidable network traffic and can overlap stale reference-data requests.♻️ Suggested refactor
ngOnInit(): void { - this.loadData(); + this.loadReferenceData(); + this.loadItems(); } - loadData(): void { + private loadItems(): void { this.loading.set(true); this.items$ = this.itemService .getAllItems() .pipe(finalize(() => this.loading.set(false))); + } + private loadReferenceData(): void { this.categoryService.getAllCategories().subscribe((categories) => { this.categories.set(categories); }); this.unitService.getAllUnits().subscribe((units) => { this.units.set(units); }); }Then call
this.loadItems()after add/update/delete instead ofthis.loadData().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/items-management/items-management.component.ts` around lines 69 - 81, loadData() currently refreshes items and reference data together, causing unnecessary category/unit fetches after every add/update/delete; extract the items-only logic into a new loadItems() that sets this.loading, assigns this.items$ from this.itemService.getAllItems().pipe(finalize(...)) and leaves category/unit fetches in loadData() (which should call loadItems() initially). Update post-mutation callers to invoke loadItems() instead of loadData(), and keep categoryService.getAllCategories() and unitService.getAllUnits() only in loadData() so reference-data reloads happen only when needed.frontend/src/app/shared/services/item.service.ts (1)
14-16: This constructor is redundant if the base service already self-injects.Once the
BaseServicewiring is confirmed, prefer removing thissuper()boilerplate instead of keeping an empty subclass constructor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/services/item.service.ts` around lines 14 - 16, The ItemService currently declares an empty constructor that only calls super(); remove this redundant constructor from the ItemService class so the class inherits BaseService's injection/initialization automatically (confirm no subclass-specific initialization exists in the constructor before removal); ensure the class still compiles and unit/integration tests pass after deleting the constructor to verify BaseService wiring covers the needs.frontend/src/app/shared/services/user.service.ts (1)
11-13: Remove this constructor ifBaseServiceis now zero-arg.If the base class already handles its own dependency injection, this constructor is just noise and can be omitted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/services/user.service.ts` around lines 11 - 13, The explicit empty constructor in the UserService class is redundant if BaseService already has a zero-arg constructor and handles DI; remove the constructor() { super(); } from UserService so it simply inherits BaseService's behavior (look for the UserService class and its constructor and delete that empty constructor), keeping the class declaration and imports unchanged.frontend/src/app/shared/services/shopping-list.service.ts (1)
21-23: Drop the no-op constructor after the BaseService check.If
BaseServicenow self-injectsHttpClient, this constructor adds no behavior and can be deleted to keep the subclass lean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/services/shopping-list.service.ts` around lines 21 - 23, The no-op constructor in ShoppingListService should be removed because BaseService now self-injects HttpClient; delete the empty constructor method (the constructor() { super(); } block) from the ShoppingListService class so the subclass relies on BaseService's injection and remains lean, then run tests/build to verify nothing breaks.backend/src/test/java/com/omatheusmesmo/shoppmate/auth/service/JwtServiceTest.java (1)
77-83: Make the expiry test deterministic.This assertion depends on
Thread.sleep(10), which makes the test timing-sensitive and prone to CI flakes. Prefer generating an already-expired token instead of waiting.Minimal change
- JwtService shortLivedService = createServiceWithExpiration(1L); + JwtService shortLivedService = createServiceWithExpiration(-1L); String token = shortLivedService.generateToken(userDetails); - - Thread.sleep(10); assertFalse(shortLivedService.validateToken(token));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/com/omatheusmesmo/shoppmate/auth/service/JwtServiceTest.java` around lines 77 - 83, The test shouldNotValidateExpiredToken is flaky because it relies on Thread.sleep; instead produce an already-expired token deterministically. Update the test to create a JwtService (via createServiceWithExpiration or by constructing JwtService) and generate a token whose expiration is set in the past (e.g., use createServiceWithExpiration with a negative expiry or call the token generator with an explicit past expiration) then assertFalse on validateToken(token). Reference: JwtServiceTest, shouldNotValidateExpiredToken, createServiceWithExpiration, generateToken, validateToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 10-13: The base_branches entry uses a glob ("feature/*") but the
auto_review.base_branches expects regex patterns; replace the glob with a regex
(e.g., change "feature/*" to "feature/.*") so branches like feature/foo are
correctly matched by the base_branches setting.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/JwtService.java`:
- Around line 32-40: The file JwtService has formatting issues; run the
project's configured formatter (e.g., mvn spotless:apply or IDE formatter
configured for the repo) and reformat the JwtService constructor and surrounding
lines so they match repository style. Focus on the JwtService(...) constructor
block where encrypter, decrypter, tokenExpiration are assigned and the
logger.info call is made (and also the nearby line 52 referenced), then commit
the formatted changes.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListMapper.java`:
- Around line 52-63: The mapper currently treats null unitPrice as zero in
calculateItemTotal and thus silently underreports totals in calculateTotalValue;
update calculateTotalValue/calculateItemTotal so that if any ListItem.unitPrice
is null the method propagates an "unknown" total (e.g., return null or
Optional<BigDecimal>) instead of summing with BigDecimal.ZERO, or validate/throw
early; locate calculateTotalValue(ShoppingList) and calculateItemTotal(ListItem)
and change the null-handling path to detect item.getUnitPrice() == null and
return the chosen sentinel (null/Optional/exception) so callers can handle
unknown totals rather than getting an incorrect numeric sum.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java`:
- Around line 22-24: The SnowflakeIdGenerator constructor currently hardcodes
workerId to 1L, which will cause ID collisions across instances; change the
constructor of SnowflakeIdGenerator to accept a configurable workerId (or read
it from an environment variable / application property) and assign that value to
the workerId field instead of the literal 1L; alternatively provide an
overloaded constructor or a static factory that derives workerId from instance
metadata (pod ordinal, container ID hash, or Spring property) so that the class
(SnowflakeIdGenerator, its constructor, and the workerId field) use a unique,
configurable identifier per instance.
In `@backend/src/main/resources/db/migration/V6__CREATE_TABLE_LIST_ITEMS.sql`:
- Around line 2-4: The V6 migration was edited in-place to change column types;
instead create a new Flyway migration (e.g., V7__) that alters the existing
table instead of modifying V6: add an ALTER TABLE statement(s) to change
list_items.id, list_items.id_list, and list_items.id_item to BIGINT (and include
any necessary CAST/USING clauses and index/constraint adjustments), ensure the
new migration file name follows Flyway versioning and is added to migrations so
already-migrated environments apply the type changes without rewriting
V6__CREATE_TABLE_LIST_ITEMS.sql.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/category/controller/CategoryControllerWithIntegrationTest.java`:
- Around line 70-74: The shared RequestSpecBuilder stores sensitive auth/Csrf
data and currently attaches RequestLoggingFilter/ResponseLoggingFilter with
LogDetail.ALL (see variable specification and classes RequestSpecBuilder,
RequestLoggingFilter, ResponseLoggingFilter, LogDetail.ALL); remove those
addFilter(...) calls from the shared spec, or replace them with a
redacting/debug-only approach that strips "Authorization" and cookie headers
before logging (e.g., use a logging filter that omits or masks Authorization and
XSRF-TOKEN), and apply any remaining verbose logging only in local/debug tests;
repeat the same change in the ItemControllerWithIntegrationTest setup.
In
`@frontend/src/app/list/components/categories-management/category-dialog/category-dialog.component.ts`:
- Around line 50-54: onSave() currently closes the dialog with
this.dialogRef.close({...this.data.category, name: this.categoryName()}) which
can emit whitespace-only names; update onSave() to normalize the name by calling
this.categoryName().trim(), guard against an empty string (e.g. don't close or
set name to a default/throw validation error) and then pass the trimmed value
into dialogRef.close (preserve Category shape). Also ensure the save button's
disabled logic that references categoryName() uses .trim() so whitespace-only
names disable the button as well.
In
`@frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.html`:
- Around line 33-40: The Save button's disabled binding allows NaN to bypass
validation because NaN <= 0 is false; update the template expression for the
button (where selectedItemId() and quantity() are referenced) to explicitly
guard against falsy/invalid quantities — e.g. disable when !selectedItemId() ||
!quantity() || isNaN(quantity()) (or use
Number.isFinite(quantity())/Boolean(quantity()) as appropriate) so empty, null,
undefined or NaN quantities keep the button disabled.
- Around line 13-15: The template iterates via items() and directly accesses
item.unit.symbol which can throw if unit is missing; update the template to use
safe navigation on item.unit (e.g., item.unit?.symbol with a fallback like '-')
or ensure the service validates/populates unit before assigning to the signal
used by items(); reference the items() iterable and the item.unit access in
list-item-dialog.component.html and apply one of these fixes so the template
never dereferences a null/undefined unit.
In
`@frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.html`:
- Line 19: The Save button's disabled guard uses listName() which treats
whitespace-only strings as truthy; update the template binding so it rejects
names that are only whitespace by checking the trimmed value (e.g., use
listName() trimmed or a new helper like isListNameValid() that returns false for
empty/whitespace-only names) and bind [disabled] to that validation helper or
the trimmed-check instead of just !listName().
In `@README.md`:
- Around line 37-49: The README's setup instructs creating backend/.env but the
root docker-compose.yml doesn't load it, so update the project either by adding
an env_file entry in the root docker-compose.yml to reference backend/.env (so
compose picks up those vars) or by moving/duplicating .env to the repository
root and updating README accordingly; reference the docker-compose.yml file and
the backend/.env path when making the change and update the README steps to
reflect which .env is used by docker compose.
---
Outside diff comments:
In `@docker-compose.yml`:
- Around line 5-14: The docker-compose change updates image to
postgres:18-alpine and the volume postgres-data mount to /var/lib/postgresql
which will make existing data stored under /var/lib/postgresql/data
inaccessible; either revert image back to postgres:17-alpine and keep the
current volume mapping until a migration is planned, or add an explicit one-time
migration step to the repo docs and orchestration (referencing the image tag
postgres:18-alpine, the named volume postgres-data, and the new mount
/var/lib/postgresql) that instructs how to copy or relocate data from the old
path (/var/lib/postgresql/data or legacy PGDATA) into the new PGDATA layout
before bringing up the service.
In
`@frontend/src/app/list/components/list-share-dialog/list-share-dialog.component.ts`:
- Around line 127-131: Guard against a possibly undefined user.id before using
the non-null assertion when building the request: in
list-share-dialog.component.ts, validate user.id (the User.id property) before
constructing the request object (which currently sets idUser: user.id!), and
handle the missing id (e.g., abort/return, show an error to the user, or disable
submission) so you never send undefined to the API; ensure
shareForm.value.permission and this.data.listId remain unchanged and only
build/send the request when user.id is a defined number.
In `@frontend/src/app/shared/components/list-item/list-item.component.ts`:
- Around line 187-193: The resetForm method currently resets listItemForm with
quantity and purchased but omits unitPrice, leaving a stale value; update
resetForm (and the call to this.listItemForm.reset) to include unitPrice set to
its default (e.g., 0 or whatever the form’s initial default is) and then clear
editingListItemId as before so new adds start with the correct unitPrice.
In `@frontend/src/app/shared/services/auth.service.ts`:
- Around line 61-68: In the register() catchError block, stop discarding the
caught error: change the catchError callback to accept the error parameter
(e.g., err), log it the same way as login() (e.g., console.error('register
error', err) or use the app logger) and then rethrow the user-facing Error with
throwError(() => new Error('Erro ao registrar usuário. Tente novamente mais
tarde.')); update the catchError surrounding function name register and ensure
you only expose the friendly message to callers while preserving the original
error in logs for debugging.
---
Duplicate comments:
In @.coderabbit.yaml:
- Around line 20-57: The YAML uses a top-level reviews.instructions key which is
ignored by the schema; update the file so the guidance is placed under the
schema-expected key (move the current reviews.instructions block into
reviews.path_instructions) or relocate it into your repository code-guidelines
document, ensuring the policy text for Backend/Frontend is nested under
reviews.path_instructions and remove the duplicate/incorrect
reviews.instructions entry; reference the reviews.instructions and
reviews.path_instructions keys in your change so reviewers can verify the fix.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java`:
- Around line 18-22: RsaKeyConfig currently accepts classpath-managed Resources
for privateKeyResource and publicKeyResource which allows repository-shipped
PEMs; change the config to require externally-managed file/env-backed key
locations and fail fast if classpath/embedded resources are provided: update
RsaKeyConfig to use file-based properties (e.g., jwt.private-key-file /
jwt.public-key-file) or accept Resource but validate at startup that
Resource.getURL().getProtocol() is not "classpath" or "jar" (or that it is a
readable java.io.File/Path), and throw an IllegalStateException with a clear
message if the keys are not filesystem/env-backed; adjust any key-loading
methods in RsaKeyConfig (e.g., the methods that load the private/public key
bytes or build the KeyPair) to use the validated file paths and to check
readability/permissions before proceeding.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListItemMapper.java`:
- Around line 33-49: The mapper is currently coercing null prices to
BigDecimal.ZERO; update ListItemMapper so getSafeUnitPrice no longer converts
null to ZERO (pass listItem.getUnitPrice() through unchanged) and change
calculateTotalPrice(BigDecimal unitPrice, Integer quantity) to return null when
unitPrice or quantity is null instead of BigDecimal.ZERO; adjust the creation of
ListItemResponseDTO to pass the nullable unitPrice and the possibly-null
totalPrice so unknown prices are preserved in serialization (ensure any
downstream code or DTO constructors accept nullable values).
In `@backend/src/main/resources/application.properties`:
- Around line 24-28: The shared application.properties currently hardcodes
jwt.private-key and jwt.public-key to classpath certs; move these two properties
out of the shared config into a local-only profile (e.g.,
application-local.properties) so production/shared config no longer contains
bundled key paths, and make jwt.private-key and jwt.public-key
environment/profile-driven (e.g., via SPRING_PROFILES or externalized config)
with comments/documentation instructing teams to supply their own secure paths
or secrets; update any startup docs/tests that assume the classpath keys and
ensure code that reads jwt.private-key/jwt.public-key still works when values
are provided via environment or profile.
In
`@frontend/src/app/list/components/units-management/units-management.component.html`:
- Around line 37-50: The icon-only mat-icon-button elements that call
editUnit(element) and deleteUnit(element) lack accessible names; add descriptive
aria-label attributes (e.g., aria-label="Edit unit" and aria-label="Delete
unit") to those buttons so screen readers announce the action; ensure the labels
describe the action and, if needed, include contextual info from element (or use
i18n keys) while keeping the (click) handlers editUnit and deleteUnit unchanged.
- Around line 17-20: The template currently treats any empty units() array as
"Nenhuma unidade cadastrada." instead of surfacing fetch errors; update the
template logic to check the component's error signal (the error reactive signal
set in the TS) before deciding what to show: if error is set render a
user-facing error message (or localized string) indicating load failed, else if
units().length render the table, else render "Nenhuma unidade cadastrada.";
locate the relevant template around units(), the error signal in the component
TS (error) and adjust the *ngIf/*ngIf-else structure so the error branch is
evaluated first.
In
`@frontend/src/app/list/components/units-management/units-management.component.ts`:
- Around line 59-70: The catchError in loadUnits() currently clears the existing
list by calling this.units.set([]) on error; remove that line so we preserve the
last successful units state, still set this.error to true and keep the
snackBar.open call to surface the failure; leave the returned of([]) (or return
a safe fallback) so the observable chain stays valid but do not mutate
this.units in the error handler.
In `@frontend/src/app/shared/components/list-item/list-item.component.html`:
- Around line 92-101: The template is accessing the signal directly — change the
usage of the signal `listItems` in the Total List Value binding to call the
signal (`listItems()`), i.e. read the array via `listItems()` and then access
the first element's `shoppingList.totalValue`; also add a defensive guard
(either optional chaining or a length check on `listItems()` before accessing
index 0) to avoid runtime errors when the array is empty, targeting the binding
that references `listItems[0].shoppingList.totalValue`.
---
Nitpick comments:
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/BaseAuditableEntity.java`:
- Around line 18-19: The annotation uses fully-qualified names which reduces
readability; add imports for org.hibernate.annotations.GenericGenerator and
com.omatheusmesmo.shoppmate.shared.utils.SnowflakeIdentifierGenerator and then
replace the fully-qualified annotation on BaseAuditableEntity so it reads
`@GeneratedValue`(generator = "snowflake") and `@GenericGenerator`(name =
"snowflake", type = SnowflakeIdentifierGenerator.class) to match the style used
in DomainEntity.
- Line 5: Remove the unused import GenerationType from BaseAuditableEntity.java:
locate the import statement "import jakarta.persistence.GenerationType;" in the
BaseAuditableEntity class and delete it (the Snowflake generator is now used
instead, so GenerationType is not referenced anywhere); ensure no other code in
BaseAuditableEntity references GenerationType after removal and run a quick
compile to verify.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/DomainEntity.java`:
- Around line 15-16: Replace the fully-qualified Hibernate annotation usage in
DomainEntity (the `@org.hibernate.annotations.GenericGenerator` reference) by
adding imports for org.hibernate.annotations.GenericGenerator and
com.omatheusmesmo.shoppmate.shared.utils.SnowflakeIdentifierGenerator at the top
of the file, then update the annotation on the identifier to use
`@GenericGenerator`(name = "snowflake", type = SnowflakeIdentifierGenerator.class)
alongside the existing `@GeneratedValue`(generator = "snowflake") to improve
readability.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java`:
- Around line 48-54: The busy-wait in SnowflakeIdGenerator.waitNextMillis
continuously spins; modify waitNextMillis to sleep briefly inside the loop
(e.g., call Thread.sleep(1) or Thread.yield()) to reduce CPU usage, and catch
InterruptedException to restore the thread interrupt status
(Thread.currentThread().interrupt()) before returning or continuing so
interruption semantics are preserved.
- Around line 29-31: Replace the generic RuntimeException thrown in
SnowflakeIdGenerator when clock moves backwards with a more specific exception
type (e.g., IllegalStateException or a custom ClockSkewException) so callers can
handle this distinct error; update the throw in the method that compares
currentTimestamp and lastTimestamp (the block that currently does throw new
RuntimeException("Clock moved backwards. Refusing to generate id.");) to throw
the chosen specific exception and adjust any surrounding javadoc/comments or
tests to expect the new exception type.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/auth/service/JwtServiceTest.java`:
- Around line 77-83: The test shouldNotValidateExpiredToken is flaky because it
relies on Thread.sleep; instead produce an already-expired token
deterministically. Update the test to create a JwtService (via
createServiceWithExpiration or by constructing JwtService) and generate a token
whose expiration is set in the past (e.g., use createServiceWithExpiration with
a negative expiry or call the token generator with an explicit past expiration)
then assertFalse on validateToken(token). Reference: JwtServiceTest,
shouldNotValidateExpiredToken, createServiceWithExpiration, generateToken,
validateToken.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.java`:
- Around line 28-29: In ListItemServiceTest rename the mock field from
ListItemRepository to listItemRepository to follow Java camelCase conventions;
update the field declaration annotated with `@Mock` and change all
usages/references within the test class (e.g., in setup and test methods that
call ListItemRepository) to the new symbol listItemRepository so compilation and
Mockito wiring remain correct.
- Around line 152-165: The test editList_Ok in ListItemServiceTest currently
omits asserting the updated unitPrice; update the test that constructs
ListItemUpdateRequestDTO(...) to also assert that after calling
service.editList(1L, updateDTO) the returned ListItem (result or listItem) has
its unitPrice equal to the DTO value (e.g., BigDecimal.valueOf(10.0)), and
ensure any mocks still verify auditService.setAuditData(listItem, false) and
ListItemRepository.save(listItem) as before; reference the editList method
behavior and the ListItemUpdateRequestDTO.unitPrice field when adding the new
assertion.
In
`@frontend/src/app/list/components/items-management/items-management.component.ts`:
- Around line 69-81: loadData() currently refreshes items and reference data
together, causing unnecessary category/unit fetches after every
add/update/delete; extract the items-only logic into a new loadItems() that sets
this.loading, assigns this.items$ from
this.itemService.getAllItems().pipe(finalize(...)) and leaves category/unit
fetches in loadData() (which should call loadItems() initially). Update
post-mutation callers to invoke loadItems() instead of loadData(), and keep
categoryService.getAllCategories() and unitService.getAllUnits() only in
loadData() so reference-data reloads happen only when needed.
In `@frontend/src/app/list/components/list-details/list-details.component.ts`:
- Around line 73-85: The loading state set by loading.set(true) in loadData()
may not be cleared reliably because finalize() on listItems$ only runs on
subscription completion; update loadData() to control loading explicitly (e.g.,
subscribe once or use operators that emit on first response) — for example, call
listItemService.getAllListItemsByListId(this.listId).pipe(take(1), tap(() =>
this.loading.set(false)), shareReplay(1)) and assign that to listItems$, or
alternatively convert list$ and listItems$ to signals and set loading.set(false)
when the first values arrive; ensure you reference loadData, listItems$, list$,
listId, loading, shoppingListService and listItemService when making the change.
In `@frontend/src/app/shared/components/unit/unit.component.html`:
- Line 69: Unit.id is optional but the template uses track unit.id and calls
deleteUnit(unit.id!), which can break if id is undefined; update the tracking
expression to use a safe fallback (e.g., track unit.id ?? index/temporaryId) and
avoid non-null assertions in deleteUnit calls by either passing the whole unit
(deleteUnit(unit)) or guarding the call (only call deleteUnit when unit.id !=
null) and updating the deleteUnit signature to accept undefined or a Unit to
handle unpersisted items; locate usages around units(), track unit.id and
deleteUnit(unit.id!) to apply these changes.
In `@frontend/src/app/shared/services/auth.service.ts`:
- Around line 86-90: getCurrentUserId() currently returns a hardcoded 1; replace
this placeholder with real JWT parsing: read the auth token used by the app
(e.g., from localStorage/session or the AuthService method that stores tokens),
base64-decode the JWT payload, safely parse the JSON and extract the user
identifier claim (commonly "sub", "userId" or "id"), convert it to a number, and
return it; handle missing/invalid tokens and parsing errors by returning null or
throwing a controlled error and update getCurrentUserId's signature/consumers
accordingly (refer to the getCurrentUserId function and any AuthService token
getters to locate where to implement this).
In `@frontend/src/app/shared/services/item.service.ts`:
- Around line 14-16: The ItemService currently declares an empty constructor
that only calls super(); remove this redundant constructor from the ItemService
class so the class inherits BaseService's injection/initialization automatically
(confirm no subclass-specific initialization exists in the constructor before
removal); ensure the class still compiles and unit/integration tests pass after
deleting the constructor to verify BaseService wiring covers the needs.
In `@frontend/src/app/shared/services/shopping-list.service.ts`:
- Around line 21-23: The no-op constructor in ShoppingListService should be
removed because BaseService now self-injects HttpClient; delete the empty
constructor method (the constructor() { super(); } block) from the
ShoppingListService class so the subclass relies on BaseService's injection and
remains lean, then run tests/build to verify nothing breaks.
In `@frontend/src/app/shared/services/user.service.ts`:
- Around line 11-13: The explicit empty constructor in the UserService class is
redundant if BaseService already has a zero-arg constructor and handles DI;
remove the constructor() { super(); } from UserService so it simply inherits
BaseService's behavior (look for the UserService class and its constructor and
delete that empty constructor), keeping the class declaration and imports
unchanged.
…e logs, configure test keys)
…gular best practices
Contributor
|
🤖 Hi @omatheusmesmo, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @omatheusmesmo, but I was unable to process your request. Please see the logs for more details. |
Contributor
|
🤖 Hi @coderabbitai[bot], I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @coderabbitai[bot], but I was unable to process your request. Please see the logs for more details. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3577
The
This is the expected and consistent outcome — since the Gemini workflows still live on The resolution path remains the same:
No further investigation needed on my end — the root cause is confirmed. 🐇 |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
No further investigation is needed. The fix is either to:
|
Contributor
|
🤖 Hi @omatheusmesmo, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
🤖 I'm sorry @omatheusmesmo, but I was unable to process your request. Please see the logs for more details. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/shared/components/item/item.component.html (1)
90-104:⚠️ Potential issue | 🟠 MajorAdd accessible names to icon-only action buttons.
The edit/delete icon buttons need
aria-labelto pass AXE/WCAG for screen-reader users.♿ Suggested fix
- <button mat-icon-button color="primary" (click)="startEdit(item)"> + <button + mat-icon-button + color="primary" + (click)="startEdit(item)" + [attr.aria-label]="'Edit item ' + item.name" + > <mat-icon>edit</mat-icon> </button> - <button mat-icon-button color="warn" (click)="deleteItem(item.id)"> + <button + mat-icon-button + color="warn" + (click)="deleteItem(item.id)" + [attr.aria-label]="'Delete item ' + item.name" + > <mat-icon>delete</mat-icon> </button>As per coding guidelines, "It MUST pass all AXE checks" and "It MUST follow all WCAG AA minimums, including focus management, color contrast, and ARIA attributes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/item/item.component.html` around lines 90 - 104, The icon-only action buttons that call startEdit(item) and deleteItem(item.id) lack accessible names; add aria-label attributes to both <button mat-icon-button> elements so screen readers announce their purpose (e.g., aria-label="Edit {item.name}" for the button that invokes startEdit(item) and aria-label="Delete {item.name}" for the button that invokes deleteItem(item.id)), ensuring the labels include the item context and are properly bound to the item data so each button has a unique, descriptive accessible name.
♻️ Duplicate comments (4)
frontend/src/app/list/components/units-management/units-management.component.html (1)
49-65:⚠️ Potential issue | 🟡 MinorUse dynamic aria-labels to identify specific units.
The static aria-labels (
"Editar unidade","Excluir unidade") don't distinguish which unit the button acts on. When screen reader users navigate through multiple table rows, every edit/delete button will announce identically, making it difficult to know which unit they're about to modify.,
♿ Suggested fix for dynamic aria-labels
<button mat-icon-button color="primary" (click)="editUnit(element)" - aria-label="Editar unidade" + [attr.aria-label]="'Editar unidade ' + element.name" > <mat-icon>edit</mat-icon> </button> <button mat-icon-button color="warn" (click)="deleteUnit(element)" - aria-label="Excluir unidade" + [attr.aria-label]="'Excluir unidade ' + element.name" > <mat-icon>delete</mat-icon> </button>As per coding guidelines: "It MUST follow all WCAG AA minimums, including focus management, color contrast, and ARIA attributes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/units-management/units-management.component.html` around lines 49 - 65, The edit/delete buttons use static aria-labels; update them to include the specific unit identifier so screen readers can distinguish rows—use the row context (the template variable element) to build dynamic aria-label values like `"Editar unidade {{element.name}}"` or include an id (`element.id`) and bind them in the template on the buttons that call editUnit(element) and deleteUnit(element); ensure you update both the edit and delete button aria-label attributes to use Angular property binding (e.g., [attr.aria-label]) so they reflect each unit's name or id.backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdentifierGenerator.java (1)
23-25:⚠️ Potential issue | 🔴 CriticalFallback path can generate duplicate IDs before Spring context is ready.
At Line 24, creating
new SnowflakeIdGenerator(1L)on every call resets sequence state each time. Under rapid inserts in the same millisecond, this can collide.🛠️ Suggested fix
public class SnowflakeIdentifierGenerator implements IdentifierGenerator, ApplicationContextAware { - private static ApplicationContext context; + private static volatile ApplicationContext context; + private static final SnowflakeIdGenerator FALLBACK_GENERATOR = + new SnowflakeIdGenerator(Long.getLong("snowflake.worker-id", 1L)); @@ public Serializable generate(SharedSessionContractImplementor session, Object object) { if (context == null) { - return new SnowflakeIdGenerator(1L).nextId(); + return FALLBACK_GENERATOR.nextId(); } return context.getBean(SnowflakeIdGenerator.class).nextId(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdentifierGenerator.java` around lines 23 - 25, The fallback path in SnowflakeIdentifierGenerator creates a new SnowflakeIdGenerator(1L) on every call which resets sequence state and can produce duplicate IDs; change this to reuse a single thread-safe fallback instance instead (e.g., a private static final or lazily initialized synchronized/atomic SnowflakeIdGenerator) so that method that currently calls new SnowflakeIdGenerator(1L).nextId() uses the shared instance; ensure the shared fallback is safely published and used across threads to avoid duplicate IDs before Spring context is ready.frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.ts (1)
35-36:⚠️ Potential issue | 🟠 MajorCreate flow can still submit
idUser: 0.When
data.listis absent,listUserIdremains0, andonSave()emits that value. This can create invalid ownership on new-list submissions.Suggested fix
- readonly listUserId = signal(0); + readonly listUserId = signal<number | null>(null); - `@Inject`(MAT_DIALOG_DATA) public data: { list?: ShoppingListRequestDTO }, + `@Inject`(MAT_DIALOG_DATA) + public data: { list?: ShoppingListRequestDTO; userId?: number }, ) { - if (data.list) { - this.listName.set(data.list.name); - this.listUserId.set(data.list.idUser); - } + if (data.list) this.listName.set(data.list.name); + this.listUserId.set(data.list?.idUser ?? data.userId ?? null); } onSave(): void { const trimmedName = this.listName().trim(); - if (!trimmedName) return; + if (!trimmedName || this.listUserId() == null) return; this.dialogRef.close({ name: trimmedName, - idUser: this.listUserId(), + idUser: this.listUserId()!, } satisfies ShoppingListRequestDTO); }Also applies to: 56-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.ts` around lines 35 - 36, The create flow emits a default listUserId of 0 when data.list is absent; update the component so listUserId is not initialized to 0 (use null/undefined or a nullable signal) and ensure onSave() validates/sets listUserId before emitting: if data.list is undefined require a real user id (reject/disable save or prompt) and do not emit idUser: 0. Adjust the same logic used around listName handling (see listUserId signal and onSave method) and apply the same validation/fix referenced in the other block (lines 56-59) so new-list submissions never send idUser: 0.backend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemRequestDTO.java (1)
8-10:⚠️ Potential issue | 🟠 MajorValidate
unitPricein the record constructor before accepting input.
unitPriceis now client-controlled, but there’s no guard against negative values.As per coding guidelines, "Records: Use record for DTOs and data carriers. Use Compact Constructors for validation and normalization."Suggested fix
public record ListItemRequestDTO(`@NotNull`(message = "List ID cannot be null") Long listId, - `@NotNull`(message = "Item ID cannot be null") Long itemId, `@Min`(1) Integer quantity, BigDecimal unitPrice) { + `@NotNull`(message = "Item ID cannot be null") Long itemId, `@Min`(1) Integer quantity, BigDecimal unitPrice) { + public ListItemRequestDTO { + if (unitPrice != null && unitPrice.signum() < 0) { + throw new IllegalArgumentException("Unit price cannot be negative"); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemRequestDTO.java` around lines 8 - 10, Add a compact constructor to the record ListItemRequestDTO to validate and normalize unitPrice: inside the compact constructor for ListItemRequestDTO check if unitPrice is non-null and unitPrice.signum() >= 0 (reject negative values) and throw an IllegalArgumentException with a clear message when invalid; if you want to allow null keep that behavior but still guard against negative; implement this validation inside the record's compact constructor so the DTO cannot be instantiated with a negative unitPrice.
🧹 Nitpick comments (11)
frontend/src/app/auth/signup/signup.component.html (2)
51-62: Signal migration looks correct; addaria-labelfor accessibility.The signal-based bindings (
hidePassword(),.set()) follow Angular best practices. However, the password visibility toggle button lacks anaria-labelto describe its function to screen readers, which is required for WCAG AA compliance.♿ Proposed accessibility fix
<button type="button" mat-icon-button matSuffix (click)="hidePassword.set(!hidePassword())" + [attr.aria-label]="hidePassword() ? 'Mostrar senha' : 'Ocultar senha'" >As per coding guidelines: "It MUST follow all WCAG AA minimums, including focus management, color contrast, and ARIA attributes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/auth/signup/signup.component.html` around lines 51 - 62, The password visibility toggle button is missing an aria-label; update the button that uses the signal hidePassword() and its .set(...) toggle (the element containing mat-icon and matSuffix) to include a dynamic aria-label that reflects the action (e.g., "Show password" when hidePassword() is true, "Hide password" when false), ensuring screen readers get the correct description for the toggle on the input with formControlName="password".
79-90: Signal usage is correct; consider improving loading state accessibility.The
isLoading()signal bindings are properly implemented. For better screen reader support, consider addingaria-busyto indicate the loading state.♿ Optional accessibility enhancement
<button mat-raised-button color="primary" type="submit" [disabled]="signupForm.invalid || isLoading()" + [attr.aria-busy]="isLoading()" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/auth/signup/signup.component.html` around lines 79 - 90, Add an accessibility attribute to expose the loading state by binding aria-busy to the isLoading() signal on the submit button (e.g., set attr.aria-busy or [attr.aria-busy] to isLoading()), so the existing button that references signupForm and isLoading() communicates to assistive tech when the spinner is active; ensure the binding updates with the signal and keep the existing [disabled] behavior.frontend/src/app/layout/navbar/navbar.component.ts (1)
43-46: Replace@HostListenerwithhostobject in decorator.The
@HostListenerdecorator should not be used per coding guidelines. Move the window resize listener to thehostobject inside the@Componentdecorator.♻️ Proposed refactor using host object
import { ChangeDetectionStrategy, Component, - HostListener, inject, signal, } from '@angular/core';`@Component`({ selector: 'app-navbar', standalone: true, imports: [ MatButtonModule, MatIconModule, MatToolbarModule, MatSidenavModule, MatListModule, MatMenuModule, RouterLink, RouterLinkActive, AsyncPipe, ], templateUrl: './navbar.component.html', styleUrls: ['./navbar.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush, + host: { + '(window:resize)': 'onResize($event)', + }, }) export class NavbarComponent { private authService = inject(AuthService); private router = inject(Router); readonly isLargeScreen = signal(window.innerWidth >= 768); isLoggedIn$ = this.authService.isLoggedIn$; - `@HostListener`('window:resize', ['$event']) onResize(event: UIEvent) { this.isLargeScreen.set((event.target as Window).innerWidth >= 768); }As per coding guidelines: "Do NOT use the
@HostBindingand@HostListenerdecorators. Put host bindings inside thehostobject of the@Componentor@Directivedecorator instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/layout/navbar/navbar.component.ts` around lines 43 - 46, Remove the `@HostListener` usage and move the window resize listener into the `@Component` decorator's host object: delete the `@HostListener`(...) line above the onResize(event: UIEvent) method and add an entry in the component's host: { '(window:resize)': 'onResize($event)' } so the existing onResize(event: UIEvent) method continues to call this.isLargeScreen.set((event.target as Window).innerWidth >= 768); locate the `@Component` decorator in this file and add the host mapping there, keeping the onResize method name unchanged.frontend/src/app/list/components/list-share-dialog/list-share-dialog.component.ts (2)
38-57: Removestandalone: truefrom the decorator.In Angular v20+, standalone is the default for components, so explicitly setting it is unnecessary.
♻️ Suggested fix
`@Component`({ selector: 'app-list-share-dialog', - standalone: true, imports: [ CommonModule,As per coding guidelines: "Must NOT set
standalone: trueinside Angular decorators. It's the default in Angular v20+."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/list-share-dialog/list-share-dialog.component.ts` around lines 38 - 57, Remove the explicit "standalone: true" property from the `@Component` decorator in the ListShareDialogComponent (list-share-dialog.component.ts); locate the decorator for the class named ListShareDialogComponent and delete the standalone: true line while leaving the imports, templateUrl, styleUrls, and changeDetection properties intact so the component uses the Angular v20+ default standalone behavior.
71-79: Replace constructor injection withinject()function.The component uses constructor injection for
MatDialogRefandMAT_DIALOG_DATA, but other dependencies already use theinject()pattern. Migrate these for consistency.♻️ Suggested fix
+ public dialogRef = inject(MatDialogRef<ListShareDialogComponent>); + public data = inject<{ listId: number; listName: string }>(MAT_DIALOG_DATA); + shareForm: FormGroup; readonly permissions = signal<ListPermissionSummaryDTO[]>([]); readonly users = signal<User[]>([]); displayedColumns: string[] = ['user', 'permission', 'actions']; permissionTypes = Object.values(Permission); readonly isLoading = signal(false); - constructor( - public dialogRef: MatDialogRef<ListShareDialogComponent>, - `@Inject`(MAT_DIALOG_DATA) public data: { listId: number; listName: string }, - ) { + constructor() { this.shareForm = this.fb.group({ email: ['', [Validators.required, Validators.email]], permission: [Permission.READ, Validators.required], }); }After this change, you can also remove the
Injectimport from@angular/core(line 4).As per coding guidelines: "Use the
inject()function instead of constructor injection."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/list-share-dialog/list-share-dialog.component.ts` around lines 71 - 79, Replace the constructor-based injection in ListShareDialogComponent by using Angular's inject() function: remove the constructor signature that takes MatDialogRef and `@Inject`(MAT_DIALOG_DATA) and instead add class-level injections such as private dialogRef = inject(MatDialogRef<ListShareDialogComponent>>), public data = inject(MAT_DIALOG_DATA) as { listId: number; listName: string }, and ensure FormBuilder (fb) is injected via inject() or already available; then move the shareForm creation (this.fb.group({...})) out of the constructor into a field initializer or ngOnInit so shareForm is created using the injected fb; finally remove the Inject import from `@angular/core`..coderabbit.yaml (1)
54-54: Minor: Angular version reference could be more accurate.Standalone components became the default in Angular 19 (November 2024), not v20+. The guidance itself is correct—just the version reference is slightly off.
📝 Suggested clarification
- - Must NOT set `standalone: true` inside Angular decorators. It's the default in Angular v20+. + - Must NOT set `standalone: true` inside Angular decorators. It's the default in Angular v19+.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml at line 54, Update the guidance text that currently reads "Must NOT set `standalone: true` inside Angular decorators. It's the default in Angular v20+." to reference the correct version: change "v20+" to "v19 (November 2024)" or simply "Angular 19", keeping the rest of the sentence and the `standalone: true` identifier unchanged.backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java (1)
26-27: Prefer Java 17.formatted(...)overString.format(...).Line 26 and Line 36 still use
String.format(...); switch to.formatted(...)for consistency with the Java 17 standard used in this repo.♻️ Suggested refactor
- throw new IllegalArgumentException(String.format("workerId must be between 0 and %d", MAX_WORKER_ID)); + throw new IllegalArgumentException("workerId must be between 0 and %d".formatted(MAX_WORKER_ID)); ... - String.format("Clock moved backwards. Refusing to generate id for %d milliseconds", - lastTimestamp - currentTimestamp)); + "Clock moved backwards. Refusing to generate id for %d milliseconds" + .formatted(lastTimestamp - currentTimestamp));As per coding guidelines, "Use
str.formatted(...)instead ofString.format()."Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java` around lines 26 - 27, The exception messages in SnowflakeIdGenerator use String.format(...) instead of Java 17's instance method; locate the validation checks that throw IllegalArgumentException (the checks referencing MAX_WORKER_ID and MAX_DATACENTER_ID inside the SnowflakeIdGenerator constructor/initialization) and replace String.format("workerId must be between 0 and %d", MAX_WORKER_ID) and the analogous String.format(...) for datacenter with the string instance-formatted form: "workerId must be between 0 and %d".formatted(MAX_WORKER_ID) and the datacenter message using .formatted(MAX_DATACENTER_ID) so the code follows the repo's Java 17 guideline.frontend/src/app/shared/components/category/category.component.ts (1)
84-97: Capture edit mode once before the async callback.Lines 84/89/96 re-read
editingCategoryId()multiple times. If state changes beforenext, message/branch logic can drift. Snapshot once and reuse.Suggested refactor
- if (this.editingCategoryId() !== null) { - categoryData.id = this.editingCategoryId()!; - } + const editingId = this.editingCategoryId(); + const isEditing = editingId !== null; + if (isEditing) { + categoryData.id = editingId; + } - const operation = - this.editingCategoryId() !== null + const operation = + isEditing ? this.categoryService.updateCategory(categoryData) : this.categoryService.addCategory(categoryData); operation.subscribe({ next: () => { this.snackBar.open( - this.editingCategoryId() !== null + isEditing ? 'Category updated successfully' : 'Category created successfully',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/category/category.component.ts` around lines 84 - 97, The code repeatedly calls editingCategoryId() before and inside the async subscription causing potential drift; capture the edit mode once into a local const (e.g., const isEdit = this.editingCategoryId() !== null) before constructing categoryData and selecting the operation, use that const to set categoryData.id when needed, to choose between this.categoryService.updateCategory(...) and this.categoryService.addCategory(...), and to decide the snackBar.open message inside operation.subscribe so the branch and message remain consistent even if state changes later.frontend/src/app/shared/components/list-item/list-item.component.ts (1)
198-205: Use acomputed()signal for derived available items.
getAvailableItems()is derived from signals and called from the template, so converting it tocomputed()will keep derivation declarative and avoid repeated ad-hoc filtering work during checks.♻️ Proposed refactor
-import { - ChangeDetectionStrategy, - Component, - OnInit, - signal, -} from '@angular/core'; +import { + ChangeDetectionStrategy, + Component, + OnInit, + computed, + signal, +} from '@angular/core'; @@ - getAvailableItems(): ItemResponseDTO[] { - if (!this.editingListItemId()) { - return this.availableItems().filter( - (item) => - !this.listItems().some((listItem) => listItem.item.id === item.id), - ); - } - return this.availableItems(); - } + readonly filteredAvailableItems = computed(() => { + if (this.editingListItemId()) return this.availableItems(); + return this.availableItems().filter( + (item) => !this.listItems().some((listItem) => listItem.item.id === item.id), + ); + });And in template usage:
-@for (item of getAvailableItems(); track item.id) { +@for (item of filteredAvailableItems(); track item.id) {As per coding guidelines, "Use
computed()for derived state."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/components/list-item/list-item.component.ts` around lines 198 - 205, Replace the ad-hoc getAvailableItems() function with a computed signal so the derived list is memoized: create a computed (e.g., availableItemsComputed = computed(() => { ... })) that implements the same logic using existing signals editingListItemId(), availableItems(), and listItems() (filtering out items present in listItems when not editing), then update the template and any call sites to use availableItemsComputed() instead of getAvailableItems(); ensure you export or expose the computed signal where the component template can access it.frontend/src/app/list/components/categories-management/category-dialog/category-dialog.component.html (1)
5-10: Prefer reactive form control binding here.This migrated to signals, but it still relies on
ngModel. Consider switching this field to aFormControl([formControl]) and drive the signal from control value changes for consistency with the project form standard.As per coding guidelines, "Prefer Reactive forms instead of Template-driven ones."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/list/components/categories-management/category-dialog/category-dialog.component.html` around lines 5 - 10, Replace the template-driven binding on the input (currently using [ngModel]="categoryName()" and (ngModelChange)="categoryName.set($event)") with a Reactive FormControl: add a FormControl (e.g., categoryNameControl) in the component class, bind the input with [formControl]="categoryNameControl", apply the required validator on that control, and drive the existing signal by subscribing to categoryNameControl.valueChanges (or use a computed signal) to call categoryName.set(value); remove the ngModel and ngModelChange usage and ensure the control is initialized with the current categoryName() value so behavior is preserved.backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ListItem.java (1)
30-31: Consider specifying precision and scale for the monetary field.
BigDecimalis the correct choice for monetary values, but without an explicit@Columnannotation definingprecisionandscale, the database schema may vary across implementations. For currency fields, explicit precision ensures consistent behavior.Suggested improvement
+ `@Column`(precision = 19, scale = 4) private BigDecimal unitPrice;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ListItem.java` around lines 30 - 31, The unitPrice BigDecimal field in class ListItem lacks an explicit column definition; add a JPA `@Column` on the unitPrice field (ListItem.unitPrice) specifying appropriate precision and scale for your currency (e.g., precision and scale values that cover total digits and decimal places you need) so the generated schema is consistent across DBs; ensure the annotation is applied to the unitPrice field and adjust nullable/columnDefinition if required by your domain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/docker-compose.yml`:
- Line 14: The docker-compose volume mount currently maps ./postgres-data to
/var/lib/postgresql which is incorrect for PostgreSQL; update the volume mapping
string (the entry currently './postgres-data:/var/lib/postgresql') to target the
official PGDATA directory '/var/lib/postgresql/data' instead so the service (the
postgres container defined in the compose file) persists DB files correctly and
avoids permission/initialization issues.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java`:
- Around line 19-23: The RsaKeyConfig class uses mutable field injection for
privateKeyResource and publicKeyResource; change these fields to be final and
remove `@Value` on fields, add a constructor annotated (or unannotated for a
single constructor) that accepts Resource privateKeyResource and Resource
publicKeyResource and assigns them to the final fields so the bean is immutable
and fails fast; update any references to the fields in methods like
getPrivateKey() / getPublicKey() if present to use the final fields.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/JwtService.java`:
- Around line 32-37: The constructor in JwtService currently accepts
tokenExpiration without validation so non-positive values silently break token
issuance; update the JwtService(RSAPublicKey publicKey, RSAPrivateKey
privateKey, long tokenExpiration) constructor to validate that tokenExpiration >
0 and if not throw an IllegalArgumentException (and log an error) with a clear
message so the application fails fast; reference the constructor and the
tokenExpiration field when making the change.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListMapper.java`:
- Around line 47-49: calculateTotalValue(entity) can return null and that null
is passed into the ShoppingListResponseDTO (via the toResponseDTO/mapper code),
violating the frontend contract expecting a numeric totalValue; fix by making
the mapper defensive: before constructing ShoppingListResponseDTO, check the
result of calculateTotalValue(entity) and either (A) fail fast by throwing a
clear IllegalStateException (e.g., "Missing unitPrice on items, cannot compute
total") so the API does not return null, or (B) normalize the value to
BigDecimal.ZERO if you want to preserve the response shape; implement the chosen
behavior in the method that builds the DTO (where you call calculateTotalValue)
and reference calculateTotalValue(entity) and the ShoppingListResponseDTO
constructor when making the change.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java`:
- Line 80: In ListItemService, avoid overwriting an existing unit price when the
update payload omits it: change the logic around
existingListItem.setUnitPrice(...) so you only call setUnitPrice when
listItemUpdateRequestDTO.unitPrice() is non-null (e.g., check if
listItemUpdateRequestDTO.unitPrice() != null before invoking
existingListItem.setUnitPrice). Leave existingListItem.unitPrice unchanged when
the DTO's unitPrice is null, and apply this null-check pattern wherever partial
updates are performed for unitPrice in the update flow.
In `@backend/src/main/resources/db/migration/V2__CREATE_TABLE_UNITS.sql`:
- Line 2: Revert the change in V2__CREATE_TABLE_UNITS.sql so the original column
definition (e.g., SERIAL or previous type) is preserved, then add a new forward
Flyway migration (e.g., V9__ALTER_UNITS_ID_TYPE.sql) that alters the units table
id column to BIGINT; target the table name "units" and column "id" in the new
migration and use the proper ALTER TABLE statement for our DB so Flyway
checksums remain unchanged for V2 and environments that already applied V2 are
not broken.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/auth/service/JwtServiceTest.java`:
- Around line 93-96: The test in JwtServiceTest (method
shouldNotValidateMalformedJweToken) uses a JWT-shaped literal in variable
malformedJwe which triggers gitleaks; replace that JWT-looking fixture with a
clearly non-JWT token (e.g., "not-a-jwt-token" or another random string without
dot-separated JWT structure) and keep the assertion calling
jwtService.validateToken(malformedJwe) unchanged so the test still asserts false
while avoiding JWT-like literal exposure.
In
`@backend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListServiceTest.java`:
- Around line 110-121: The testEditListSuccess misses stubbing
userService.findUser which is called via shoppingListService.editList ->
isListValid; add a when(...) stub for userService.findUser(...) to return the
expected user (e.g., Optional.of(testUser) or testUser) before invoking
shoppingListService.editList(testList) so the validation passes; keep existing
verifications for auditService.setAuditData(testList, false) and
shoppingListRepository.save(testList).
- Around line 52-61: The test ShoppingListServiceTest::testSaveListSuccess
currently doesn't stub userService.findUser(), making it brittle because
ShoppingListService.saveList -> isListValid -> getOwnerId ->
userService.findUser() actually may throw; update the test to explicitly stub
userService.findUser(...) to return an Optional<User> (or the expected User) for
testList's owner before calling shoppingListService.saveList(testList) so the
service validation path uses a real user; ensure you reference the mock
userService and the test fixture object testList (or its owner id) when
stubbing.
In
`@frontend/src/app/list/components/items-management/items-management.component.ts`:
- Around line 70-75: The loadItems() function creates a deadlock because calling
this.loading.set(true) hides the template branch that subscribes to items$,
preventing itemService.getAllItems() from being subscribed and finalized; fix by
ensuring the async subscription is always active or by driving loading from the
observable: change loadItems() to assign this.items$ =
this.itemService.getAllItems().pipe(tap(() => {/*optional side effects*/}),
finalize(() => this.loading.set(false))); remove the initial
this.loading.set(true) or set loading before assigning items$ and ensure the
template places (items$ | async) outside the loading conditional so the
observable can subscribe while the spinner shows. Ensure you update references
to loadItems(), items$, itemService.getAllItems(), tap/finalize and
loading.set(...) accordingly.
- Around line 77-84: loadReferenceData() currently subscribes to
categoryService.getAllCategories() and unitService.getAllUnits() without error
handlers, so failures are silent; update loadReferenceData() to add error
callbacks to both subscriptions (for categoryService.getAllCategories() and
unitService.getAllUnits()) that log the error (or call the component
notification/toast service) and set a sensible fallback for
this.categories.set(...) and this.units.set(...) (e.g., empty array or
previously cached values) so the dialog doesn't remain empty and the user gets
feedback; ensure you reference the same methods (getAllCategories, getAllUnits)
and state setters (this.categories.set, this.units.set) when implementing the
handlers.
In `@frontend/src/app/shared/components/category/category.component.html`:
- Line 58: The template uses "track category.id" but Category.id is optional so
missing ids collapse; update the ngFor tracking expression to use a stable
fallback like "track category.id ?? category.name" (or another unique field) in
category.component.html, and apply the same change in the other templates that
iterate getAllCategories() (item.component.html, item-dialog.component.html,
categories-management.component.html) so each category uses a unique stable
tracking key even when id is undefined.
In `@frontend/src/app/shared/components/category/category.component.ts`:
- Around line 113-115: In startEdit(Category) add an explicit guard that
verifies category.id is present before switching into edit mode: if category.id
is falsy, do not call this.editingCategoryId.set(...) or patch the form; instead
notify the user (e.g., via the existing notification/toast service or an alert)
and return early. Update the logic referencing this.editingCategoryId,
startEdit, and categoryForm.patchValue so only a valid id is accepted and null
is no longer used to silently convert an edit into a create.
In `@frontend/src/app/shared/components/item/item.component.html`:
- Line 5: The icon-only action buttons for edit/delete lack accessible names;
update the edit and delete mat-icon-button elements that call startEdit(item)
and deleteItem(item.id) to include aria-label (e.g., "Edit {{ item.name }}" and
"Delete {{ item.name }}") or a title attribute so screen readers can announce
the action; additionally, if the template repeatedly calls the signal-returning
function editingItemId(), capture it once with an Angular template binding
(using `@let` or a local template variable) and reuse that variable instead of
calling editingItemId() multiple times to improve clarity and performance.
In `@frontend/src/app/shared/components/list-item/list-item.component.html`:
- Around line 45-47: The Unit Price numeric input (template element with
formControlName="unitPrice" inside list-item.component.html) currently lacks a
step attribute and thus defaults to step="1", preventing decimals; update the
<input matInput ... formControlName="unitPrice"> element to include an
appropriate step (e.g., step="0.01") so users can enter cents/decimal values
while keeping min="0" intact.
In `@frontend/src/app/shared/components/list-item/list-item.component.ts`:
- Around line 109-123: Capture the current edit-mode before making the async
call so the success message uses the submit-time state instead of re-reading
editingListItemId() later; e.g., compute a boolean or id like const isEditing =
this.editingListItemId() (or const editingId = this.editingListItemId()) and use
that to choose between calling listItemService.updateListItem(...) vs
listItemService.addListItem(...), and then inside operation.subscribe next()
call snackBar.open(...) using the saved isEditing/editingId value rather than
calling editingListItemId() again.
In `@frontend/src/app/shared/services/auth.service.ts`:
- Around line 61-62: The catchError block in AuthService (the RxJS catchError
handling HttpErrorResponse in the register flow) currently logs the full
HttpErrorResponse which may leak server payloads/PII; change the logging to only
emit safe metadata (e.g., err.status, err.url or the request endpoint, and
err.message) instead of the whole error object, and keep the existing error
propagation (rethrow/return throwError) behavior; locate the catchError in the
register method and replace the console.error(err) usage with a concise log of
status+endpoint+message.
- Around line 87-100: getCurrentUserId currently assumes a standard base64
payload and may produce NaN; update it to first validate the token has three
parts, then normalize the base64url payload (replace '-' with '+', '_' with '/',
add '=' padding to length %4), atob-decode and JSON.parse into a typed
Record<string, unknown> (not any), then look up claims (decoded.userId,
decoded.id, decoded.sub) but only accept them if typeof is 'number' or a numeric
string (use a safe numeric check before converting), returning a Number value or
null; ensure all parsing is wrapped in try/catch and return null on failure.
---
Outside diff comments:
In `@frontend/src/app/shared/components/item/item.component.html`:
- Around line 90-104: The icon-only action buttons that call startEdit(item) and
deleteItem(item.id) lack accessible names; add aria-label attributes to both
<button mat-icon-button> elements so screen readers announce their purpose
(e.g., aria-label="Edit {item.name}" for the button that invokes startEdit(item)
and aria-label="Delete {item.name}" for the button that invokes
deleteItem(item.id)), ensuring the labels include the item context and are
properly bound to the item data so each button has a unique, descriptive
accessible name.
---
Duplicate comments:
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemRequestDTO.java`:
- Around line 8-10: Add a compact constructor to the record ListItemRequestDTO
to validate and normalize unitPrice: inside the compact constructor for
ListItemRequestDTO check if unitPrice is non-null and unitPrice.signum() >= 0
(reject negative values) and throw an IllegalArgumentException with a clear
message when invalid; if you want to allow null keep that behavior but still
guard against negative; implement this validation inside the record's compact
constructor so the DTO cannot be instantiated with a negative unitPrice.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdentifierGenerator.java`:
- Around line 23-25: The fallback path in SnowflakeIdentifierGenerator creates a
new SnowflakeIdGenerator(1L) on every call which resets sequence state and can
produce duplicate IDs; change this to reuse a single thread-safe fallback
instance instead (e.g., a private static final or lazily initialized
synchronized/atomic SnowflakeIdGenerator) so that method that currently calls
new SnowflakeIdGenerator(1L).nextId() uses the shared instance; ensure the
shared fallback is safely published and used across threads to avoid duplicate
IDs before Spring context is ready.
In
`@frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.ts`:
- Around line 35-36: The create flow emits a default listUserId of 0 when
data.list is absent; update the component so listUserId is not initialized to 0
(use null/undefined or a nullable signal) and ensure onSave() validates/sets
listUserId before emitting: if data.list is undefined require a real user id
(reject/disable save or prompt) and do not emit idUser: 0. Adjust the same logic
used around listName handling (see listUserId signal and onSave method) and
apply the same validation/fix referenced in the other block (lines 56-59) so
new-list submissions never send idUser: 0.
In
`@frontend/src/app/list/components/units-management/units-management.component.html`:
- Around line 49-65: The edit/delete buttons use static aria-labels; update them
to include the specific unit identifier so screen readers can distinguish
rows—use the row context (the template variable element) to build dynamic
aria-label values like `"Editar unidade {{element.name}}"` or include an id
(`element.id`) and bind them in the template on the buttons that call
editUnit(element) and deleteUnit(element); ensure you update both the edit and
delete button aria-label attributes to use Angular property binding (e.g.,
[attr.aria-label]) so they reflect each unit's name or id.
---
Nitpick comments:
In @.coderabbit.yaml:
- Line 54: Update the guidance text that currently reads "Must NOT set
`standalone: true` inside Angular decorators. It's the default in Angular v20+."
to reference the correct version: change "v20+" to "v19 (November 2024)" or
simply "Angular 19", keeping the rest of the sentence and the `standalone: true`
identifier unchanged.
In `@backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ListItem.java`:
- Around line 30-31: The unitPrice BigDecimal field in class ListItem lacks an
explicit column definition; add a JPA `@Column` on the unitPrice field
(ListItem.unitPrice) specifying appropriate precision and scale for your
currency (e.g., precision and scale values that cover total digits and decimal
places you need) so the generated schema is consistent across DBs; ensure the
annotation is applied to the unitPrice field and adjust
nullable/columnDefinition if required by your domain.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.java`:
- Around line 26-27: The exception messages in SnowflakeIdGenerator use
String.format(...) instead of Java 17's instance method; locate the validation
checks that throw IllegalArgumentException (the checks referencing MAX_WORKER_ID
and MAX_DATACENTER_ID inside the SnowflakeIdGenerator
constructor/initialization) and replace String.format("workerId must be between
0 and %d", MAX_WORKER_ID) and the analogous String.format(...) for datacenter
with the string instance-formatted form: "workerId must be between 0 and
%d".formatted(MAX_WORKER_ID) and the datacenter message using
.formatted(MAX_DATACENTER_ID) so the code follows the repo's Java 17 guideline.
In `@frontend/src/app/auth/signup/signup.component.html`:
- Around line 51-62: The password visibility toggle button is missing an
aria-label; update the button that uses the signal hidePassword() and its
.set(...) toggle (the element containing mat-icon and matSuffix) to include a
dynamic aria-label that reflects the action (e.g., "Show password" when
hidePassword() is true, "Hide password" when false), ensuring screen readers get
the correct description for the toggle on the input with
formControlName="password".
- Around line 79-90: Add an accessibility attribute to expose the loading state
by binding aria-busy to the isLoading() signal on the submit button (e.g., set
attr.aria-busy or [attr.aria-busy] to isLoading()), so the existing button that
references signupForm and isLoading() communicates to assistive tech when the
spinner is active; ensure the binding updates with the signal and keep the
existing [disabled] behavior.
In `@frontend/src/app/layout/navbar/navbar.component.ts`:
- Around line 43-46: Remove the `@HostListener` usage and move the window resize
listener into the `@Component` decorator's host object: delete the
`@HostListener`(...) line above the onResize(event: UIEvent) method and add an
entry in the component's host: { '(window:resize)': 'onResize($event)' } so the
existing onResize(event: UIEvent) method continues to call
this.isLargeScreen.set((event.target as Window).innerWidth >= 768); locate the
`@Component` decorator in this file and add the host mapping there, keeping the
onResize method name unchanged.
In
`@frontend/src/app/list/components/categories-management/category-dialog/category-dialog.component.html`:
- Around line 5-10: Replace the template-driven binding on the input (currently
using [ngModel]="categoryName()" and (ngModelChange)="categoryName.set($event)")
with a Reactive FormControl: add a FormControl (e.g., categoryNameControl) in
the component class, bind the input with [formControl]="categoryNameControl",
apply the required validator on that control, and drive the existing signal by
subscribing to categoryNameControl.valueChanges (or use a computed signal) to
call categoryName.set(value); remove the ngModel and ngModelChange usage and
ensure the control is initialized with the current categoryName() value so
behavior is preserved.
In
`@frontend/src/app/list/components/list-share-dialog/list-share-dialog.component.ts`:
- Around line 38-57: Remove the explicit "standalone: true" property from the
`@Component` decorator in the ListShareDialogComponent
(list-share-dialog.component.ts); locate the decorator for the class named
ListShareDialogComponent and delete the standalone: true line while leaving the
imports, templateUrl, styleUrls, and changeDetection properties intact so the
component uses the Angular v20+ default standalone behavior.
- Around line 71-79: Replace the constructor-based injection in
ListShareDialogComponent by using Angular's inject() function: remove the
constructor signature that takes MatDialogRef and `@Inject`(MAT_DIALOG_DATA) and
instead add class-level injections such as private dialogRef =
inject(MatDialogRef<ListShareDialogComponent>>), public data =
inject(MAT_DIALOG_DATA) as { listId: number; listName: string }, and ensure
FormBuilder (fb) is injected via inject() or already available; then move the
shareForm creation (this.fb.group({...})) out of the constructor into a field
initializer or ngOnInit so shareForm is created using the injected fb; finally
remove the Inject import from `@angular/core`.
In `@frontend/src/app/shared/components/category/category.component.ts`:
- Around line 84-97: The code repeatedly calls editingCategoryId() before and
inside the async subscription causing potential drift; capture the edit mode
once into a local const (e.g., const isEdit = this.editingCategoryId() !== null)
before constructing categoryData and selecting the operation, use that const to
set categoryData.id when needed, to choose between
this.categoryService.updateCategory(...) and
this.categoryService.addCategory(...), and to decide the snackBar.open message
inside operation.subscribe so the branch and message remain consistent even if
state changes later.
In `@frontend/src/app/shared/components/list-item/list-item.component.ts`:
- Around line 198-205: Replace the ad-hoc getAvailableItems() function with a
computed signal so the derived list is memoized: create a computed (e.g.,
availableItemsComputed = computed(() => { ... })) that implements the same logic
using existing signals editingListItemId(), availableItems(), and listItems()
(filtering out items present in listItems when not editing), then update the
template and any call sites to use availableItemsComputed() instead of
getAvailableItems(); ensure you export or expose the computed signal where the
component template can access it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31be9d85-71e6-430d-885d-82f638faf8ee
⛔ Files ignored due to path filters (2)
backend/src/test/resources/certs/private_key.pemis excluded by!**/*.pembackend/src/test/resources/certs/public_key.pemis excluded by!**/*.pem
📒 Files selected for processing (110)
.coderabbit.yaml.github/commands/gemini-invoke.toml.github/commands/gemini-review.toml.github/commands/gemini-scheduled-triage.toml.github/commands/gemini-triage.toml.github/workflows/gemini-dispatch.yml.github/workflows/gemini-invoke.yml.github/workflows/gemini-review.yml.github/workflows/gemini-scheduled-triage.yml.github/workflows/gemini-triage.ymlREADME.mdbackend/.gitignorebackend/docker-compose.ymlbackend/mvnwbackend/pom.xmlbackend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/CsrfCookieFilter.javabackend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.javabackend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/SecurityConfig.javabackend/src/main/java/com/omatheusmesmo/shoppmate/auth/controller/AuthController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/CustomUserDetailsService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/JwtService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/category/controller/CategoryController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/category/service/CategoryService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/item/controller/ItemController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/item/service/ItemService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ShoppingListController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemRequestDTO.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemResponseDTO.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemUpdateRequestDTO.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ShoppingListResponseDTO.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ListItem.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ShoppingList.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListItemMapper.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListMapper.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/BaseAuditableEntity.javabackend/src/main/java/com/omatheusmesmo/shoppmate/shared/domain/DomainEntity.javabackend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdGenerator.javabackend/src/main/java/com/omatheusmesmo/shoppmate/shared/utils/SnowflakeIdentifierGenerator.javabackend/src/main/java/com/omatheusmesmo/shoppmate/unit/controller/UnitController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/unit/service/UnitService.javabackend/src/main/java/com/omatheusmesmo/shoppmate/user/controller/UserController.javabackend/src/main/java/com/omatheusmesmo/shoppmate/user/dtos/RegisterUserDTO.javabackend/src/main/java/com/omatheusmesmo/shoppmate/user/service/UserService.javabackend/src/main/resources/application.propertiesbackend/src/main/resources/db/migration/V1__CREATE_TABLE_USERS.sqlbackend/src/main/resources/db/migration/V2__CREATE_TABLE_UNITS.sqlbackend/src/main/resources/db/migration/V3__CREATE_TABLE_CATEGORIES.sqlbackend/src/main/resources/db/migration/V4__CREATE_TABLE_ITEMS.sqlbackend/src/main/resources/db/migration/V5__CREATE_TABLE_LISTS.sqlbackend/src/main/resources/db/migration/V6__CREATE_TABLE_LIST_ITEMS.sqlbackend/src/main/resources/db/migration/V7__CREATE_TABLE_LIST_USER_PERMISSIONS.sqlbackend/src/main/resources/db/migration/V8__ADD_PRICE_TO_LIST_ITEMS.sqlbackend/src/test/java/com/omatheusmesmo/shoppmate/auth/service/JwtServiceTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/category/controller/CategoryControllerTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/category/controller/CategoryControllerWithIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/item/controller/ItemControllerWithIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ListItemServiceTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/list/service/ShoppingListServiceTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/service/JwtServiceTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/shared/testcontainers/AbstractIntegrationTest.javabackend/src/test/java/com/omatheusmesmo/shoppmate/shared/testcontainers/utils/TestUserFactory.javadocker-compose.ymlfrontend/eslint.config.jsfrontend/scripts/check-signals.jsfrontend/src/app/auth/login/login.component.htmlfrontend/src/app/auth/login/login.component.tsfrontend/src/app/auth/signup/signup.component.htmlfrontend/src/app/auth/signup/signup.component.tsfrontend/src/app/layout/navbar/navbar.component.htmlfrontend/src/app/layout/navbar/navbar.component.tsfrontend/src/app/list/components/categories-management/categories-management.component.htmlfrontend/src/app/list/components/categories-management/categories-management.component.tsfrontend/src/app/list/components/categories-management/category-dialog/category-dialog.component.htmlfrontend/src/app/list/components/categories-management/category-dialog/category-dialog.component.tsfrontend/src/app/list/components/items-management/items-management.component.htmlfrontend/src/app/list/components/items-management/items-management.component.tsfrontend/src/app/list/components/list-details/list-details.component.htmlfrontend/src/app/list/components/list-details/list-details.component.tsfrontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.htmlfrontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.tsfrontend/src/app/list/components/list-share-dialog/list-share-dialog.component.htmlfrontend/src/app/list/components/list-share-dialog/list-share-dialog.component.tsfrontend/src/app/list/components/shopping-list-dialog/shopping-list-dialog.component.htmlfrontend/src/app/list/components/shopping-list-dialog/shopping-list-dialog.component.tsfrontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.htmlfrontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.tsfrontend/src/app/list/components/shopping-list/shopping-list.component.htmlfrontend/src/app/list/components/shopping-list/shopping-list.component.tsfrontend/src/app/list/components/units-management/units-management.component.htmlfrontend/src/app/list/components/units-management/units-management.component.tsfrontend/src/app/shared/components/category/category.component.htmlfrontend/src/app/shared/components/category/category.component.tsfrontend/src/app/shared/components/item/item.component.htmlfrontend/src/app/shared/components/item/item.component.tsfrontend/src/app/shared/components/list-item/list-item.component.htmlfrontend/src/app/shared/components/list-item/list-item.component.scssfrontend/src/app/shared/components/list-item/list-item.component.tsfrontend/src/app/shared/components/unit/unit.component.htmlfrontend/src/app/shared/components/unit/unit.component.tsfrontend/src/app/shared/interfaces/list-item.interface.tsfrontend/src/app/shared/interfaces/shopping-list.interface.tsfrontend/src/app/shared/services/auth.service.tsfrontend/src/app/shared/services/item.service.tsfrontend/src/app/shared/services/shopping-list.service.tsfrontend/src/app/shared/services/user.service.ts
💤 Files with no reviewable changes (12)
- backend/src/test/java/com/omatheusmesmo/shoppmate/service/JwtServiceTest.java
- .github/workflows/gemini-scheduled-triage.yml
- frontend/src/app/shared/services/user.service.ts
- .github/commands/gemini-scheduled-triage.toml
- .github/workflows/gemini-dispatch.yml
- .github/commands/gemini-invoke.toml
- .github/commands/gemini-triage.toml
- .github/workflows/gemini-triage.yml
- frontend/src/app/shared/services/item.service.ts
- .github/commands/gemini-review.toml
- .github/workflows/gemini-invoke.yml
- .github/workflows/gemini-review.yml
🚧 Files skipped from review as they are similar to previous changes (44)
- frontend/src/app/list/components/items-management/items-management.component.html
- backend/src/main/java/com/omatheusmesmo/shoppmate/unit/service/UnitService.java
- backend/src/test/java/com/omatheusmesmo/shoppmate/shared/testcontainers/utils/TestUserFactory.java
- frontend/src/app/shared/components/unit/unit.component.html
- frontend/src/app/list/components/shopping-list-dialog/shopping-list-dialog.component.ts
- backend/src/test/java/com/omatheusmesmo/shoppmate/shared/testcontainers/AbstractIntegrationTest.java
- frontend/src/app/list/components/list-details/list-details.component.ts
- backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListItemMapper.java
- backend/src/main/resources/db/migration/V1__CREATE_TABLE_USERS.sql
- frontend/src/app/list/components/categories-management/category-dialog/category-dialog.component.ts
- backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListItemController.java
- frontend/scripts/check-signals.js
- README.md
- frontend/src/app/shared/interfaces/shopping-list.interface.ts
- frontend/src/app/auth/login/login.component.html
- backend/src/main/resources/db/migration/V5__CREATE_TABLE_LISTS.sql
- backend/src/main/resources/db/migration/V8__ADD_PRICE_TO_LIST_ITEMS.sql
- frontend/src/app/shared/interfaces/list-item.interface.ts
- backend/src/main/resources/application.properties
- frontend/src/app/list/components/list-share-dialog/list-share-dialog.component.html
- frontend/src/app/auth/signup/signup.component.ts
- frontend/src/app/shared/components/unit/unit.component.ts
- backend/src/test/java/com/omatheusmesmo/shoppmate/category/controller/CategoryControllerTest.java
- backend/src/main/java/com/omatheusmesmo/shoppmate/list/entity/ShoppingList.java
- backend/src/main/resources/db/migration/V6__CREATE_TABLE_LIST_ITEMS.sql
- frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.ts
- backend/pom.xml
- frontend/src/app/list/components/units-management/units-management.component.ts
- frontend/src/app/shared/components/item/item.component.ts
- docker-compose.yml
- backend/src/main/java/com/omatheusmesmo/shoppmate/list/controller/ListPermissionController.java
- frontend/eslint.config.js
- frontend/src/app/list/components/shopping-list/list-dialog/list-dialog.component.html
- backend/src/main/java/com/omatheusmesmo/shoppmate/unit/controller/UnitController.java
- frontend/src/app/list/components/list-details/list-item-dialog/list-item-dialog.component.html
- backend/src/main/java/com/omatheusmesmo/shoppmate/list/dtos/ListItemResponseDTO.java
- frontend/src/app/shared/components/list-item/list-item.component.scss
- backend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/CustomUserDetailsService.java
- backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/SecurityConfig.java
- frontend/src/app/list/components/list-details/list-details.component.html
- backend/src/main/java/com/omatheusmesmo/shoppmate/user/service/UserService.java
- backend/src/main/resources/db/migration/V3__CREATE_TABLE_CATEGORIES.sql
- backend/src/main/resources/db/migration/V7__CREATE_TABLE_LIST_USER_PERMISSIONS.sql
- backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListPermissionService.java
| - POSTGRES_DB=${POSTGRES_DB:-SHOPPMATE} | ||
| volumes: | ||
| - ./postgres-data:/var/lib/postgresql/data No newline at end of file | ||
| - ./postgres-data:/var/lib/postgresql No newline at end of file |
Contributor
There was a problem hiding this comment.
Incorrect volume mount path will cause data persistence issues.
The volume mount should target /var/lib/postgresql/data (the PGDATA directory), not /var/lib/postgresql. The official PostgreSQL Docker image stores database files in /var/lib/postgresql/data. Mounting the parent directory can cause:
- Data not persisting correctly between container restarts
- Permission issues with the postgres user's home directory
- Container initialization failures
🐛 Proposed fix
volumes:
- - ./postgres-data:/var/lib/postgresql
+ - ./postgres-data:/var/lib/postgresql/data📝 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
| - ./postgres-data:/var/lib/postgresql | |
| - ./postgres-data:/var/lib/postgresql/data |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/docker-compose.yml` at line 14, The docker-compose volume mount
currently maps ./postgres-data to /var/lib/postgresql which is incorrect for
PostgreSQL; update the volume mapping string (the entry currently
'./postgres-data:/var/lib/postgresql') to target the official PGDATA directory
'/var/lib/postgresql/data' instead so the service (the postgres container
defined in the compose file) persists DB files correctly and avoids
permission/initialization issues.
Comment on lines
+19
to
+23
| @Value("${jwt.private-key}") | ||
| private Resource privateKeyResource; | ||
|
|
||
| @Value("${jwt.public-key}") | ||
| private Resource publicKeyResource; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use constructor injection for the key resources.
This config reintroduces mutable field injection. Make the two Resource dependencies final and inject them through the constructor so the bean stays immutable and fails fast during creation.
♻️ Proposed refactor
`@Configuration`
public class RsaKeyConfig {
-
- `@Value`("${jwt.private-key}")
- private Resource privateKeyResource;
-
- `@Value`("${jwt.public-key}")
- private Resource publicKeyResource;
+ private final Resource privateKeyResource;
+ private final Resource publicKeyResource;
+
+ public RsaKeyConfig(`@Value`("${jwt.private-key}") Resource privateKeyResource,
+ `@Value`("${jwt.public-key}") Resource publicKeyResource) {
+ this.privateKeyResource = privateKeyResource;
+ this.publicKeyResource = publicKeyResource;
+ }📝 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
| @Value("${jwt.private-key}") | |
| private Resource privateKeyResource; | |
| @Value("${jwt.public-key}") | |
| private Resource publicKeyResource; | |
| `@Configuration` | |
| public class RsaKeyConfig { | |
| private final Resource privateKeyResource; | |
| private final Resource publicKeyResource; | |
| public RsaKeyConfig(`@Value`("${jwt.private-key}") Resource privateKeyResource, | |
| `@Value`("${jwt.public-key}") Resource publicKeyResource) { | |
| this.privateKeyResource = privateKeyResource; | |
| this.publicKeyResource = publicKeyResource; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/configs/RsaKeyConfig.java`
around lines 19 - 23, The RsaKeyConfig class uses mutable field injection for
privateKeyResource and publicKeyResource; change these fields to be final and
remove `@Value` on fields, add a constructor annotated (or unannotated for a
single constructor) that accepts Resource privateKeyResource and Resource
publicKeyResource and assigns them to the final fields so the bean is immutable
and fails fast; update any references to the fields in methods like
getPrivateKey() / getPublicKey() if present to use the final fields.
Comment on lines
+32
to
+37
| public JwtService(RSAPublicKey publicKey, RSAPrivateKey privateKey, | ||
| @Value("${jwt.token.expiration}") long tokenExpiration) { | ||
| this.encrypter = new RSAEncrypter(publicKey); | ||
| this.decrypter = new RSADecrypter(privateKey); | ||
| this.tokenExpiration = tokenExpiration; | ||
| logger.info("RSA keys loaded successfully for JWE token operations"); |
Contributor
There was a problem hiding this comment.
Reject invalid expiration config at startup.
If jwt.token.expiration is 0 or negative, the app still boots but every token issued by generateToken() is immediately unusable. Validate the value here so auth breaks fast and loudly.
🐛 Proposed fix
public JwtService(RSAPublicKey publicKey, RSAPrivateKey privateKey,
`@Value`("${jwt.token.expiration}") long tokenExpiration) {
+ if (tokenExpiration <= 0) {
+ throw new IllegalArgumentException("jwt.token.expiration must be greater than zero");
+ }
this.encrypter = new RSAEncrypter(publicKey);
this.decrypter = new RSADecrypter(privateKey);
this.tokenExpiration = tokenExpiration;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/auth/service/JwtService.java`
around lines 32 - 37, The constructor in JwtService currently accepts
tokenExpiration without validation so non-positive values silently break token
issuance; update the JwtService(RSAPublicKey publicKey, RSAPrivateKey
privateKey, long tokenExpiration) constructor to validate that tokenExpiration >
0 and if not throw an IllegalArgumentException (and log an error) with a clear
message so the application fails fast; reference the constructor and the
tokenExpiration field when making the change.
Comment on lines
+47
to
+49
| BigDecimal totalValue = calculateTotalValue(entity); | ||
|
|
||
| return new ShoppingListResponseDTO(entity.getId(), entity.getName(), ownerDTO); | ||
| return new ShoppingListResponseDTO(entity.getId(), entity.getName(), ownerDTO, totalValue); |
Contributor
There was a problem hiding this comment.
Don't return null into a numeric totalValue API.
calculateTotalValue() now returns null when any item is missing unitPrice, and toResponseDTO() forwards that straight into ShoppingListResponseDTO. The frontend type in frontend/src/app/shared/interfaces/shopping-list.interface.ts still declares totalValue: number, so a partially priced list will break the contract. Either make totalValue nullable on both sides or fail fast before building the response.
Also applies to: 61-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/mapper/ListMapper.java`
around lines 47 - 49, calculateTotalValue(entity) can return null and that null
is passed into the ShoppingListResponseDTO (via the toResponseDTO/mapper code),
violating the frontend contract expecting a numeric totalValue; fix by making
the mapper defensive: before constructing ShoppingListResponseDTO, check the
result of calculateTotalValue(entity) and either (A) fail fast by throwing a
clear IllegalStateException (e.g., "Missing unitPrice on items, cannot compute
total") so the API does not return null, or (B) normalize the value to
BigDecimal.ZERO if you want to preserve the response shape; implement the chosen
behavior in the method that builds the DTO (where you call calculateTotalValue)
and reference calculateTotalValue(entity) and the ShoppingListResponseDTO
constructor when making the change.
|
|
||
| existingListItem.setQuantity(listItemUpdateRequestDTO.quantity()); | ||
| existingListItem.setPurchased(listItemUpdateRequestDTO.purchased()); | ||
| existingListItem.setUnitPrice(listItemUpdateRequestDTO.unitPrice()); |
Contributor
There was a problem hiding this comment.
Do not overwrite unitPrice when update payload omits it.
Line 80 currently allows null to replace an existing stored price, which can silently lose pricing data.
Suggested fix
- existingListItem.setUnitPrice(listItemUpdateRequestDTO.unitPrice());
+ if (listItemUpdateRequestDTO.unitPrice() != null) {
+ if (listItemUpdateRequestDTO.unitPrice().signum() < 0) {
+ throw new IllegalArgumentException("Unit price cannot be negative");
+ }
+ existingListItem.setUnitPrice(listItemUpdateRequestDTO.unitPrice());
+ }📝 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
| existingListItem.setUnitPrice(listItemUpdateRequestDTO.unitPrice()); | |
| if (listItemUpdateRequestDTO.unitPrice() != null) { | |
| if (listItemUpdateRequestDTO.unitPrice().signum() < 0) { | |
| throw new IllegalArgumentException("Unit price cannot be negative"); | |
| } | |
| existingListItem.setUnitPrice(listItemUpdateRequestDTO.unitPrice()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/omatheusmesmo/shoppmate/list/service/ListItemService.java`
at line 80, In ListItemService, avoid overwriting an existing unit price when
the update payload omits it: change the logic around
existingListItem.setUnitPrice(...) so you only call setUnitPrice when
listItemUpdateRequestDTO.unitPrice() is non-null (e.g., check if
listItemUpdateRequestDTO.unitPrice() != null before invoking
existingListItem.setUnitPrice). Leave existingListItem.unitPrice unchanged when
the DTO's unitPrice is null, and apply this null-check pattern wherever partial
updates are performed for unitPrice in the update flow.
| <mat-card-header> | ||
| <mat-card-title>{{ | ||
| editingItemId ? "Edit Item" : "New Item" | ||
| editingItemId() ? "Edit Item" : "New Item" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd -t f 'item.component.ts$' | head -n1)"
echo "Inspecting: $FILE"
# Verify these template-accessed members are declared as signal/computed-backed reads
rg -n -C3 '\b(editingItemId|isLoading|categories|units|items)\b' "$FILE"
rg -n -C2 'signal\(|computed\(' "$FILE"Repository: omatheusmesmo/ShoppMate
Length of output: 2252
🏁 Script executed:
cat -n frontend/src/app/shared/components/item/item.component.htmlRepository: omatheusmesmo/ShoppMate
Length of output: 4398
Add aria-label attributes to icon-only action buttons and consider binding repeated signal reads with @let to improve clarity.
Icon-only buttons at lines 99–104 (edit and delete actions) lack accessible names. Per WCAG 2.1 AA requirements, add aria-label or title attributes:
<button mat-icon-button color="primary" (click)="startEdit(item)" aria-label="Edit {{ item.name }}">
<mat-icon>edit</mat-icon>
</button>
<button mat-icon-button color="warn" (click)="deleteItem(item.id)" aria-label="Delete {{ item.name }}">
<mat-icon>delete</mat-icon>
</button>Additionally, reduce repeated signal calls by binding them once:
♻️ Template optimization
+ `@let` isEditing = editingItemId();
<mat-card-title>{{
- editingItemId() ? "Edit Item" : "New Item"
+ isEditing ? "Edit Item" : "New Item"
}}</mat-card-title>
...
- {{ editingItemId() ? "Update" : "Create" }}
+ {{ isEditing ? "Update" : "Create" }}
...
- `@if` (editingItemId()) {
+ `@if` (isEditing) {
...
- } `@else` if (items().length === 0) {
+ } `@else` {
+ `@let` currentItems = items();
+ `@if` (currentItems.length === 0) {
<mat-card class="empty-state">
<p>No items found</p>
</mat-card>
- } `@else` {
<div class="items-grid">
- `@for` (item of items(); track item.id) {
+ `@for` (item of currentItems; track item.id) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/shared/components/item/item.component.html` at line 5, The
icon-only action buttons for edit/delete lack accessible names; update the edit
and delete mat-icon-button elements that call startEdit(item) and
deleteItem(item.id) to include aria-label (e.g., "Edit {{ item.name }}" and
"Delete {{ item.name }}") or a title attribute so screen readers can announce
the action; additionally, if the template repeatedly calls the signal-returning
function editingItemId(), capture it once with an Angular template binding
(using `@let` or a local template variable) and reuse that variable instead of
calling editingItemId() multiple times to improve clarity and performance.
Comment on lines
+45
to
+47
| <mat-form-field appearance="outline"> | ||
| <mat-label>Unit Price</mat-label> | ||
| <input matInput type="number" formControlName="unitPrice" min="0" /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant lines
cat -n frontend/src/app/shared/components/list-item/list-item.component.html | sed -n '40,65p'Repository: omatheusmesmo/ShoppMate
Length of output: 1109
🏁 Script executed:
# Let's also check the component TypeScript file to understand the form structure
fd list-item.component.ts frontend/Repository: omatheusmesmo/ShoppMate
Length of output: 134
🏁 Script executed:
# Search for all numeric inputs in the template to see if there's a pattern
rg 'type="number"' frontend/src/app/shared/components/list-item/Repository: omatheusmesmo/ShoppMate
Length of output: 363
🏁 Script executed:
# Check the component TypeScript to understand form validation
cat -n frontend/src/app/shared/components/list-item/list-item.component.ts | head -100Repository: omatheusmesmo/ShoppMate
Length of output: 3864
🏁 Script executed:
# Look for any validators on unitPrice field
rg 'unitPrice' frontend/src/app/shared/components/list-item/list-item.component.ts -A 2 -B 2Repository: omatheusmesmo/ShoppMate
Length of output: 786
Add decimal step to the Unit Price input field.
Line 47: <input type="number"> without a step attribute defaults to step="1", which prevents users from entering decimal values. For a currency field like unit prices, this blocks valid entries (e.g., $1.99, $5.50).
Proposed fix
- <input matInput type="number" formControlName="unitPrice" min="0" />
+ <input
+ matInput
+ type="number"
+ formControlName="unitPrice"
+ min="0"
+ step="0.01"
+ />📝 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
| <mat-form-field appearance="outline"> | |
| <mat-label>Unit Price</mat-label> | |
| <input matInput type="number" formControlName="unitPrice" min="0" /> | |
| <mat-form-field appearance="outline"> | |
| <mat-label>Unit Price</mat-label> | |
| <input | |
| matInput | |
| type="number" | |
| formControlName="unitPrice" | |
| min="0" | |
| step="0.01" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/shared/components/list-item/list-item.component.html` around
lines 45 - 47, The Unit Price numeric input (template element with
formControlName="unitPrice" inside list-item.component.html) currently lacks a
step attribute and thus defaults to step="1", preventing decimals; update the
<input matInput ... formControlName="unitPrice"> element to include an
appropriate step (e.g., step="0.01") so users can enter cents/decimal values
while keeping min="0" intact.
Comment on lines
+109
to
123
| const operation = this.editingListItemId() | ||
| ? this.listItemService.updateListItem( | ||
| this.listId, | ||
| this.editingListItemId, | ||
| this.editingListItemId()!, | ||
| listItemData, | ||
| ) | ||
| : this.listItemService.addListItem(listItemData); | ||
| : this.listItemService.addListItem(this.listId, listItemData); | ||
|
|
||
| operation.subscribe({ | ||
| next: () => { | ||
| this.snackBar.open( | ||
| this.editingListItemId | ||
| this.editingListItemId() | ||
| ? 'Item updated successfully' | ||
| : 'Item added successfully', | ||
| 'Close', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -150 frontend/src/app/shared/components/list-item/list-item.component.ts | tail -60Repository: omatheusmesmo/ShoppMate
Length of output: 1749
🏁 Script executed:
sed -n '40,80p' frontend/src/app/shared/components/list-item/list-item.component.tsRepository: omatheusmesmo/ShoppMate
Length of output: 1215
🏁 Script executed:
sed -n '100,130p' frontend/src/app/shared/components/list-item/list-item.component.tsRepository: omatheusmesmo/ShoppMate
Length of output: 1000
Capture edit mode once before the async request to prevent message mismatch.
editingListItemId() is re-read in the success callback, which can diverge from submit-time state if user interactions occur before the request completes, resulting in incorrect success messages.
Proposed fix
- const operation = this.editingListItemId()
+ const editingId = this.editingListItemId();
+ const operation = editingId
? this.listItemService.updateListItem(
this.listId,
- this.editingListItemId()!,
+ editingId,
listItemData,
)
: this.listItemService.addListItem(this.listId, listItemData);
operation.subscribe({
next: () => {
this.snackBar.open(
- this.editingListItemId()
+ editingId
? 'Item updated successfully'
: 'Item added successfully',📝 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
| const operation = this.editingListItemId() | |
| ? this.listItemService.updateListItem( | |
| this.listId, | |
| this.editingListItemId, | |
| this.editingListItemId()!, | |
| listItemData, | |
| ) | |
| : this.listItemService.addListItem(listItemData); | |
| : this.listItemService.addListItem(this.listId, listItemData); | |
| operation.subscribe({ | |
| next: () => { | |
| this.snackBar.open( | |
| this.editingListItemId | |
| this.editingListItemId() | |
| ? 'Item updated successfully' | |
| : 'Item added successfully', | |
| 'Close', | |
| const editingId = this.editingListItemId(); | |
| const operation = editingId | |
| ? this.listItemService.updateListItem( | |
| this.listId, | |
| editingId, | |
| listItemData, | |
| ) | |
| : this.listItemService.addListItem(this.listId, listItemData); | |
| operation.subscribe({ | |
| next: () => { | |
| this.snackBar.open( | |
| editingId | |
| ? 'Item updated successfully' | |
| : 'Item added successfully', | |
| 'Close', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/shared/components/list-item/list-item.component.ts` around
lines 109 - 123, Capture the current edit-mode before making the async call so
the success message uses the submit-time state instead of re-reading
editingListItemId() later; e.g., compute a boolean or id like const isEditing =
this.editingListItemId() (or const editingId = this.editingListItemId()) and use
that to choose between calling listItemService.updateListItem(...) vs
listItemService.addListItem(...), and then inside operation.subscribe next()
call snackBar.open(...) using the saved isEditing/editingId value rather than
calling editingListItemId() again.
Comment on lines
+61
to
+62
| catchError((err: HttpErrorResponse) => { | ||
| console.error('Register error:', err); |
Contributor
There was a problem hiding this comment.
Avoid logging full HttpErrorResponse in client logs.
Logging the full error object may expose server payloads/PII in browser logs. Log only safe metadata (e.g., status + endpoint) instead.
Suggested fix
- catchError((err: HttpErrorResponse) => {
- console.error('Register error:', err);
+ catchError((err: HttpErrorResponse) => {
+ console.error('Register error:', {
+ status: err.status,
+ url: err.url ?? null,
+ });
return throwError(
() =>
new Error(📝 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
| catchError((err: HttpErrorResponse) => { | |
| console.error('Register error:', err); | |
| catchError((err: HttpErrorResponse) => { | |
| console.error('Register error:', { | |
| status: err.status, | |
| url: err.url ?? null, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/shared/services/auth.service.ts` around lines 61 - 62, The
catchError block in AuthService (the RxJS catchError handling HttpErrorResponse
in the register flow) currently logs the full HttpErrorResponse which may leak
server payloads/PII; change the logging to only emit safe metadata (e.g.,
err.status, err.url or the request endpoint, and err.message) instead of the
whole error object, and keep the existing error propagation (rethrow/return
throwError) behavior; locate the catchError in the register method and replace
the console.error(err) usage with a concise log of status+endpoint+message.
Comment on lines
+87
to
+100
| getCurrentUserId(): number | null { | ||
| const token = this.getToken(); | ||
| if (!token) return null; | ||
|
|
||
| try { | ||
| const payload = token.split('.')[1]; | ||
| const decoded = JSON.parse(atob(payload)); | ||
| // Try to find ID in common claims: sub, userId, id | ||
| const userId = decoded.userId || decoded.id || decoded.sub; | ||
| return userId ? Number(userId) : null; | ||
| } catch (e) { | ||
| console.error('Error decoding token:', e); | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: find JWT decode callsites and check for base64url normalization.
rg -nP --type=ts -C3 'getCurrentUserId\s*\(|atob\s*\(|split\(\x27\.\x27\)\[1\]|replace\(/-/g,\s*\x27\+\x27\)' frontendRepository: omatheusmesmo/ShoppMate
Length of output: 2044
Harden JWT payload parsing and validate numeric ID extraction.
The current implementation lacks base64url normalization, so it fails on valid JWTs with - or _ characters. It can also return NaN when claims are non-numeric strings. Normalize the base64url payload, validate token structure with a parts length check, explicitly type the decoded object as Record<string, unknown> (per guidelines, avoid any), and guard claim types before converting to number.
Suggested fix
getCurrentUserId(): number | null {
const token = this.getToken();
if (!token) return null;
try {
- const payload = token.split('.')[1];
- const decoded = JSON.parse(atob(payload));
- // Try to find ID in common claims: sub, userId, id
- const userId = decoded.userId || decoded.id || decoded.sub;
- return userId ? Number(userId) : null;
+ const parts = token.split('.');
+ if (parts.length < 2 || !parts[1]) return null;
+
+ const base64Url = parts[1];
+ const base64 = base64Url.replace(/-/g, '+').replace(/_/g, '/');
+ const padded = base64.padEnd(Math.ceil(base64.length / 4) * 4, '=');
+
+ const decoded: Record<string, unknown> = JSON.parse(atob(padded));
+ const rawUserId = decoded['userId'] ?? decoded['id'] ?? decoded['sub'];
+
+ const numericUserId =
+ typeof rawUserId === 'number'
+ ? rawUserId
+ : typeof rawUserId === 'string'
+ ? Number(rawUserId)
+ : NaN;
+
+ return Number.isFinite(numericUserId) ? numericUserId : null;
} catch (e) {
console.error('Error decoding token:', e);
return null;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/shared/services/auth.service.ts` around lines 87 - 100,
getCurrentUserId currently assumes a standard base64 payload and may produce
NaN; update it to first validate the token has three parts, then normalize the
base64url payload (replace '-' with '+', '_' with '/', add '=' padding to length
%4), atob-decode and JSON.parse into a typed Record<string, unknown> (not any),
then look up claims (decoded.userId, decoded.id, decoded.sub) but only accept
them if typeof is 'number' or a numeric string (use a safe numeric check before
converting), returning a Number value or null; ensure all parsing is wrapped in
try/catch and return null on failure.
This was referenced Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request primarily removes detailed prompt and operational instructions from three Gemini CLI-related GitHub Actions command files and introduces a new
.coderabbit.yamlconfiguration file with project-specific review and workflow settings. The main changes are grouped below:1. Removal of Gemini CLI Prompt Instructions:
.github/commands/gemini-invoke.toml,.github/commands/gemini-review.toml, and.github/commands/gemini-scheduled-triage.toml, leaving only the file headers. This significantly reduces the verbosity and complexity of these workflow command files. [1] [2] [3]2. Addition of Project-Specific Review Configuration:
.coderabbit.yamlfile that defines language, review workflow settings, path filters, and detailed instructions for backend (Java 17/Spring Boot) and frontend (Angular 18+/TypeScript) development best practices. The configuration also enables auto-review and chat auto-reply features.Summary by CodeRabbit
New Features
Improvements
Tests
Documentation