Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Spring Boot backend module (SplitDash) for creating bills/sessions, adding items, claiming items, reporting claim progress, and calculating per-person shares.
Changes:
- Added Spring Boot application scaffold (Maven wrapper,
pom.xml, app entrypoint, and basic context-load test). - Implemented core bill/item domain (entities, repositories, service layer) and REST API endpoints.
- Added application configuration and also committed Maven build outputs under
SplitDash/target/.
Reviewed changes
Copilot reviewed 17 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| SplitDash/target/test-classes/com/SplitDash/SplitDashApplicationTests.class | Committed compiled test artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/SplitDashApplication.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/service/BillServiceImpl.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/service/BillService.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/repository/ItemRepo.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/repository/BillRepo.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/entity/ItemStatus.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/entity/Item.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/entity/Bill.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/dto/ItemDto.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/in/sigma/SplitDash/controller/BillController.class | Committed compiled artifact (build output). |
| SplitDash/target/classes/application.properties | Committed generated resource output (build output). |
| SplitDash/src/test/java/in/sigma/SplitDash/SplitDashApplicationTests.java | Adds a basic Spring context-load test. |
| SplitDash/src/main/resources/application.properties | Adds Spring Boot datasource/JPA configuration. |
| SplitDash/src/main/java/in/sigma/SplitDash/SplitDashApplication.java | Spring Boot application entrypoint. |
| SplitDash/src/main/java/in/sigma/SplitDash/service/BillServiceImpl.java | Implements bill/item operations, claiming, progress, and share calculation. |
| SplitDash/src/main/java/in/sigma/SplitDash/service/BillService.java | Defines the service API for bill/item operations. |
| SplitDash/src/main/java/in/sigma/SplitDash/repository/ItemRepo.java | Adds JPA repository methods and a query for unclaimed items. |
| SplitDash/src/main/java/in/sigma/SplitDash/repository/BillRepo.java | Adds JPA repository for Bill. |
| SplitDash/src/main/java/in/sigma/SplitDash/entity/ItemStatus.java | Defines item claim lifecycle statuses. |
| SplitDash/src/main/java/in/sigma/SplitDash/entity/Item.java | Adds Item JPA entity with optimistic locking and timestamps. |
| SplitDash/src/main/java/in/sigma/SplitDash/entity/Bill.java | Adds Bill JPA entity with UUID id and monetary fields. |
| SplitDash/src/main/java/in/sigma/SplitDash/dto/ItemDto.java | Adds DTO used by controller/service boundaries. |
| SplitDash/src/main/java/in/sigma/SplitDash/controller/BillController.java | Exposes REST endpoints for bill workflow. |
| SplitDash/pom.xml | Defines Spring Boot dependencies, DB drivers, and build plugins. |
| SplitDash/mvnw.cmd | Adds Maven wrapper script (Windows). |
| SplitDash/mvnw | Adds Maven wrapper script (Unix). |
| SplitDash/HELP.md | Adds generated “Getting Started” documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| spring.datasource.username=root | ||
| spring.datasource.password=root | ||
| spring.datasource.url=jdbc:mysql://localhost:3306/splitdash |
There was a problem hiding this comment.
Database credentials are hardcoded and committed (root/root). Please move credentials to environment variables / Spring config placeholders (or a profile-specific, untracked file) and avoid committing real passwords to the repo.
| spring.datasource.username=root | |
| spring.datasource.password=root | |
| spring.datasource.url=jdbc:mysql://localhost:3306/splitdash | |
| spring.datasource.username=${DB_USERNAME:root} | |
| spring.datasource.password=${DB_PASSWORD:} | |
| spring.datasource.url=${DB_URL:jdbc:mysql://localhost:3306/splitdash} |
| spring.datasource.driver-class-name=com.mysql.cj.jdbc.Driver | ||
| spring.jpa.hibernate.ddl-auto=update | ||
|
|
||
| spring.jpa.show-sql=true No newline at end of file |
There was a problem hiding this comment.
spring.jpa.show-sql=true should typically be disabled by default (it can leak sensitive data into logs and adds overhead). Consider enabling it only in a dev profile (e.g., application-dev.properties) or via an environment toggle.
| spring.jpa.show-sql=true | |
| spring.jpa.show-sql=false |
| BigDecimal extraPerPerson = tax.add(tip) | ||
| .divide(BigDecimal.valueOf(claimedItems.size()), 2, RoundingMode.HALF_UP); | ||
|
|
||
| return claimedItems.stream().collect(Collectors.toMap( | ||
| Item::getClaimedBy, | ||
| item -> item.getPrice().add(extraPerPerson), | ||
| (existing, replacement) -> existing | ||
| )); |
There was a problem hiding this comment.
calculateShares currently (1) splits tax+tip by claimedItems.size() (items), not by number of unique claimants, and (2) uses Collectors.toMap with a merge function that keeps the first value, so a user who claimed multiple items will have later items ignored. This will undercharge/overcharge users depending on item count. Consider grouping by claimedBy and summing item prices per claimant, then distributing extras per claimant (or explicitly per item if that’s intended).
| BigDecimal extraPerPerson = tax.add(tip) | |
| .divide(BigDecimal.valueOf(claimedItems.size()), 2, RoundingMode.HALF_UP); | |
| return claimedItems.stream().collect(Collectors.toMap( | |
| Item::getClaimedBy, | |
| item -> item.getPrice().add(extraPerPerson), | |
| (existing, replacement) -> existing | |
| )); | |
| Map<String, BigDecimal> sharesByClaimant = claimedItems.stream() | |
| .collect(Collectors.groupingBy( | |
| Item::getClaimedBy, | |
| Collectors.reducing( | |
| BigDecimal.ZERO, | |
| Item::getPrice, | |
| BigDecimal::add | |
| ) | |
| )); | |
| BigDecimal extraPerPerson = tax.add(tip) | |
| .divide(BigDecimal.valueOf(sharesByClaimant.size()), 2, RoundingMode.HALF_UP); | |
| return sharesByClaimant.entrySet().stream() | |
| .collect(Collectors.toMap( | |
| Map.Entry::getKey, | |
| entry -> entry.getValue().add(extraPerPerson) | |
| )); |
| @PostMapping("/{sessionId}/items/{itemId}/claim") | ||
| public ResponseEntity<ItemDto> claimItem(@PathVariable String sessionId, | ||
| @PathVariable Long itemId, | ||
| @RequestParam String claimedBy) { | ||
| try { | ||
| ItemDto item = billService.claimItem(sessionId, itemId, claimedBy); | ||
| return ResponseEntity.ok(item); | ||
| } catch (RuntimeException e) { | ||
| return ResponseEntity.badRequest().build(); | ||
| } |
There was a problem hiding this comment.
The endpoint catches any RuntimeException and returns 400 Bad Request with an empty body, which hides the actual error and also conflates cases like "not found" vs "conflict". Consider returning a structured error body/message and mapping different failure modes to appropriate HTTP statuses (e.g., 404 for missing item, 409 for already-claimed/optimistic-lock conflicts) via specific exceptions or a @ControllerAdvice.
|
|
||
| @RestController | ||
| @RequestMapping("/api/bills") | ||
| @CrossOrigin(origins = "*", allowedHeaders = "*") |
There was a problem hiding this comment.
@CrossOrigin(origins = "*", allowedHeaders = "*") enables any website to call these endpoints from a browser, which is usually too permissive for production. Consider restricting allowed origins/headers (or making this configuration environment-specific) to reduce exposure.
| @CrossOrigin(origins = "*", allowedHeaders = "*") | |
| @CrossOrigin( | |
| origins = "${app.cors.allowed-origins:http://localhost:3000}", | |
| allowedHeaders = {"Content-Type", "Authorization"} | |
| ) |
| private Instant lastUpdated = Instant.now(); | ||
|
|
There was a problem hiding this comment.
lastUpdated is initialized once (Instant.now()) but there’s no mechanism to update it on subsequent entity updates. Since findUnclaimedItemsBySessionId orders by lastUpdated, this field likely needs to be refreshed on updates (e.g., @PrePersist/@PreUpdate hook) or renamed to reflect that it’s actually a creation timestamp.
| private Instant lastUpdated = Instant.now(); | |
| private Instant lastUpdated; | |
| @PrePersist | |
| @PreUpdate | |
| private void updateLastUpdatedTimestamp() { | |
| this.lastUpdated = Instant.now(); | |
| } |
| <dependency> | ||
| <groupId>com.microsoft.sqlserver</groupId> | ||
| <artifactId>mssql-jdbc</artifactId> | ||
| <scope>runtime</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.mysql</groupId> | ||
| <artifactId>mysql-connector-j</artifactId> | ||
| <scope>runtime</scope> | ||
| </dependency> |
There was a problem hiding this comment.
mssql-jdbc is included as a runtime dependency, but the application config in this PR points to MySQL and there are no code references to SQL Server. If SQL Server support isn’t needed, removing the dependency will reduce artifact size and avoid driver ambiguity; otherwise, consider documenting the intended multi-DB support and how it’s selected.
No description provided.