feat: cross-profile alarm overview with profile management#147
Merged
Conversation
hokiepokedad2
commented
Apr 7, 2026
Contributor
Author
hokiepokedad2
left a comment
hokiepokedad2
left a comment
There was a problem hiding this comment.
Code Review: PR !147 — Cross-Profile Alarm Overview
Executive Summary
| Category | Rating |
|---|---|
| Overall | ⭐⭐⭐⭐ Approved with Conditions |
| Code Quality | Good — follows proxy-first patterns, Angular signals |
| Requirements | Fully Satisfied — exceeds issue scope with export/import/duplicate |
| Architecture | Strong — consistent with CLAUDE.md patterns |
| Risk Level | Medium — needs test coverage before merge |
Requirements Traceability
| Requirement (Issue 127) | Implementation | Status |
|---|---|---|
| allProfiles proxy method | IPoracleTrackingProxy.GetAllTrackingAllProfilesAsync |
✅ |
| Page showing alarms grouped by profile | cross-profile-overview.component accordion layout |
✅ |
| Profile tabs/accordion panels | Mat-expansion-panel per profile | ✅ |
| Filter by alarm type | Type chip filter bar | ✅ |
| Export to JSON | Per-profile backup export | ✅ (exceeded — per-profile) |
| Identify duplicates | Duplicate detection with filter chip | ✅ |
Critical Issues (Must Fix)
| # | File | Issue | Impact |
|---|---|---|---|
| 1 | CrossProfileControllerTests.cs |
Only 2 tests for GET endpoint. No tests for duplicate/import endpoints | Untested critical paths |
| 2 | CrossProfileServiceTests.cs |
No tests for DuplicateProfileAsync or ImportAlarmsAsync | No verification of error recovery (finally block) |
| 3 | cross-profile-overview.component.ts |
No component spec file | Core duplicate detection logic untested |
Major Issues (Should Fix)
| # | File | Issue | Recommendation |
|---|---|---|---|
| 4 | cross-profile-overview.component.ts:loadAll() |
Two independent subscriptions race — profile list vs overview can arrive out of order | Use forkJoin to coordinate both API calls |
| 5 | profile-add-dialog.component.ts |
Constructor subscribes to getAll() without cleanup |
Use takeUntilDestroyed(inject(DestroyRef)) |
| 6 | profile-edit-dialog.component.ts |
Same subscription leak as above | Same fix |
| 7 | cross-profile-overview.component.ts:switchProfile() |
loadCurrentUser() not awaited before loadAll() |
Chain with switchMap or await |
Minor Issues (Consider)
| # | File | Issue | Suggestion |
|---|---|---|---|
| 8 | cross-profile-overview.component.ts:94-153 |
Duplicate detection computed twice (duplicates + duplicateUids) with identical iteration | Derive duplicateUids from duplicates computed |
| 9 | cross-profile-overview.component.ts:importProfile() |
Generic catch with no error detail logged | Log to console, distinguish SyntaxError vs other |
| 10 | CrossProfileService.cs:24 |
Default profile number hardcoded as 1 | Minor — acceptable pattern |
Architecture & Design Review
Pattern Compliance: ✅ Strong
- Proxy-first: All alarm/human operations through
IPoracleTrackingProxyandIPoracleHumanProxy - No direct DB writes for alarms (correct per CLAUDE.md)
- Angular standalone components with
inject()and signals - Lazy-loaded route with
authGuard
Design Positives:
- Clean separation: controller → service → proxy
PoracleJsonHelper.StripPropertymade public correctly for reuse- Profile name uniqueness validation at all entry points (add, edit, duplicate, import)
- Export format is clean — strips internal fields (uid, id, profile_no)
finallyblock in DuplicateProfileAsync ensures profile restoration- Game asset images via configurable IconService with graceful fallback
Design Concerns:
GenerateTokenWithProfileis duplicated betweenCrossProfileControllerandProfileController— extract to shared utilityloadAll()fires two parallel subscriptions without coordination
Test Coverage
| Area | Coverage | Gap |
|---|---|---|
| Backend GET overview | ✅ 2 tests | — |
| Backend duplicate/import | ❌ 0 tests | Missing: ownership validation, alarm copying, error recovery |
| Frontend service | ✅ 3 tests | — |
| Frontend component | ❌ 0 tests | Missing: duplicate detection, import/export, profile switching |
| Profile update fix | ✅ 1 test updated | — |
Risk Assessment
| Risk | Level | Notes |
|---|---|---|
| Breaking Change | Low | New endpoints only, no existing API changes |
| Data Integrity | Medium | Duplicate/import write to PoracleNG with profile switching |
| Performance | Low | Single allProfiles call, no N+1 |
| Security | Low | All endpoints behind [Authorize], ownership checked via GetByUserAndProfileNoAsync |
| Rollback | Low | New feature, can be disabled by removing nav item |
Final Verdict: APPROVED WITH CONDITIONS
Pre-Merge Requirements:
- Add backend tests for DuplicateProfile and ImportProfile controller endpoints
- Add backend tests for CrossProfileService error recovery (verify finally block)
- Fix subscription leaks in ProfileAddDialog and ProfileEditDialog constructors
- Coordinate loadAll() with forkJoin to prevent race condition
Post-Merge Recommendations:
- Add frontend component tests for duplicate detection
- Extract GenerateTokenWithProfile to shared utility
- Derive duplicateUids from duplicates computed (DRY)
- Add console logging to import error catch block
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
- Add PoracleNG allProfiles proxy endpoint for fetching alarms across all profiles - New unified Profiles page merging profile management with alarm overview - Profile actions: switch, duplicate, edit, delete, export backup, import backup - Alarm cards show game asset images (Pokemon sprites, raid eggs, lure/gym icons) - Per-type filtering, search, and duplicate detection across profiles - Profile name uniqueness validation on add, edit, duplicate, and import - Fix profile update to use snake_case profile_no for PoracleNG compatibility - Add CrossProfileService, CrossProfileController with duplicate/import endpoints - Backend and frontend tests for all new functionality - Code formatting (prettier, eslint, dotnet format)
- Fix subscription leaks in ProfileAddDialog/ProfileEditDialog (takeUntilDestroyed) - Fix loadAll() race condition with forkJoin (coordinate profile + overview loading) - Add 7 backend tests: duplicate/import controller + service error recovery - Rename cross-profile → profiles-overview (component, service, models, route) - Rename CrossProfileService → ProfileOverviewService - Rename CrossProfileOverview → ProfileOverview, CrossProfileAlarm → ProfileOverviewAlarm - Update help guide with full Profiles page documentation (export, import, duplicate, detection) - Update GitHub Pages docs (features/alarms.md) with cross-profile overview, backup/restore
- Resolve IPoracleHumanProxy conflict (keep CopyProfileAsync from main) - Resolve docs/features/alarms.md conflict (merge both sections) - Rename DuplicateProfileRequest → CrossProfileDuplicateRequest to avoid collision with main's ProfileController DTO - Rename ImportProfileRequest → CrossProfileImportRequest - Apply dotnet format, prettier, eslint
hokiepokedad2
force-pushed
the
feature/127-cross-profile-alarm-overview
branch
from
a0dc51e to
e2d4432
Compare
hokiepokedad2
commented
Apr 7, 2026
Contributor
Author
hokiepokedad2
left a comment
hokiepokedad2
left a comment
There was a problem hiding this comment.
Code Review (Post-Rebase): PR 147
Executive Summary
| Category | Rating |
|---|---|
| Overall | APPROVED WITH CONDITIONS |
| Code Quality | Good |
| Requirements Satisfied | All from issue 127 |
| Architecture Fit | Strong — proxy-first, Angular signals, standalone components |
| Risk Level | Low-Medium |
| Test Coverage | Improved (11 backend + 3 frontend tests) |
Previous Review Fixes — Verified
- forkJoin in loadAll() — applied, races eliminated
- takeUntilDestroyed in ProfileAddDialog/ProfileEditDialog — applied
- 7 additional backend tests (duplicate, import, error recovery) — applied
- Rebase on latest main — clean, conflicts resolved
- DTO renamed to avoid collision (CrossProfileDuplicateRequest / CrossProfileImportRequest)
Critical Issues (Must Fix)
None remaining.
Major Issues (Should Fix)
| File | Line | Issue | Recommendation |
|---|---|---|---|
CrossProfileService.cs |
83 | ImportAlarmsAsync doesn't strip uid from alarm JSON |
Add PoracleJsonHelper.StripProperty(alarm, "uid") for safety — export strips it client-side but defensive stripping prevents issues if backup files are manually edited |
CrossProfileController.cs |
59 | No rollback on duplicate failure — main's ProfileController.Duplicate has try/catch with DeleteProfileAsync rollback, but CrossProfileController.DuplicateProfile doesn't | Add try/catch around DuplicateProfileAsync with rollback on failure |
Minor Issues (Consider)
| File | Line | Issue | Suggestion |
|---|---|---|---|
CrossProfileController.cs |
147 | Version field in CrossProfileImportRequest is accepted but never validated |
Add if (request.Version != 1) return BadRequest("Unsupported backup version") |
docs/features/alarms.md |
78 | States "empty profile is automatically rolled back" but references main's ProfileController behavior, not CrossProfileController | Update docs to match actual implementation |
profile-overview.component.ts |
95-154 | duplicates and duplicateUids compute identical keyMaps independently |
Derive duplicateUids from duplicates to avoid double computation |
profile-overview.component.ts |
337 | a.click() DOM side effect in exportProfile |
Works fine, just harder to test — acceptable for now |
Architecture Review
Strengths:
- Clean proxy-first pattern — all alarm ops through
IPoracleTrackingProxy CrossProfileServicecorrectly delegates to proxy with no DB fallbackfinallyblock ensures profile restoration in duplicate/import- forkJoin coordinates parallel API calls in loadAll()
- IconService used for configurable game asset URLs
- ConfirmDialog extended cleanly with
promptFieldfor name input + uniqueness validation - Export format is portable — strips internal fields
Post-Rebase Integration:
- Main branch added
CopyProfileAsyncon proxy andDuplicateon ProfileController — our CrossProfileController's duplicate endpoint uses a different approach (allProfiles + manual copy). Both work; main's is cleaner for simple duplication. Our import feature is unique and valuable. - No conflicts remain between the two approaches
Test Coverage
| Area | Tests | Status |
|---|---|---|
| CrossProfileController GET | 2 | Covered |
| CrossProfileController duplicate | 2 | Covered |
| CrossProfileController import | 2 | Covered (incl. name dedup) |
| CrossProfileService happy path | 2 | Covered |
| CrossProfileService error recovery | 3 | Covered (restore profile on error) |
| Frontend service | 3 | Covered |
| Frontend component | 0 | Gap — complex computed logic untested |
Risk Assessment
| Risk | Level | Notes |
|---|---|---|
| Breaking Change | None | New endpoints only |
| Data Integrity | Low | finally block ensures profile restoration |
| Performance | Low | Single allProfiles API call |
| Security | Low | All endpoints behind [Authorize], ownership via GetByUserAndProfileNoAsync |
| Rollback | Low | Feature can be removed by reverting nav item |
Final Verdict: APPROVED WITH CONDITIONS
Pre-Merge (Should Fix):
- Add defensive uid stripping in ImportAlarmsAsync
- Add rollback try/catch in CrossProfileController.DuplicateProfile (matching main's pattern)
Post-Merge (Nice to Have):
- Derive duplicateUids from duplicates computed (DRY)
- Add frontend component tests for computed signals
- Validate import version field
- Consider using main's CopyProfileAsync for duplicate instead of manual allProfiles approach
- ImportAlarmsAsync now strips uid from alarm JSON defensively before creating - CrossProfileController.DuplicateProfile rolls back (deletes) the new profile if alarm copy fails
hokiepokedad2
commented
Apr 7, 2026
Contributor
Author
hokiepokedad2
left a comment
hokiepokedad2
left a comment
There was a problem hiding this comment.
Code Review (Final): PR 147
Executive Summary
| Category | Rating |
|---|---|
| Overall | APPROVED WITH CONDITIONS |
| Code Quality | Good |
| Requirements | All satisfied |
| Architecture | Strong |
| Risk Level | Low |
| Test Coverage | Good (11 backend + 3 frontend) |
Previous Fix Verification
- forkJoin in loadAll() — confirmed applied
- takeUntilDestroyed in dialog subscriptions — confirmed applied
- 7 additional backend tests — confirmed (637 total pass)
- Defensive uid stripping in ImportAlarmsAsync — confirmed applied
- Rollback on duplicate failure — confirmed applied
- Rebase on latest main — clean
- DTO rename to avoid collision — confirmed
Issues Found
Major (Should Fix)
| File | Line | Issue | Recommendation |
|---|---|---|---|
CrossProfileController.cs |
58-69 | Rollback catch block doesn't protect against DeleteProfileAsync throwing — would mask the original error |
Wrap DeleteProfileAsync in inner try-catch so original exception is preserved |
Minor (Consider)
| File | Line | Issue | Suggestion |
|---|---|---|---|
profile-overview.component.ts |
538-542 | Backup validation only checks property existence, not types or version | Add typeof backup.alarms !== 'object' and backup.version !== 1 checks |
CrossProfileController.cs |
44-46 | Concurrent requests could calculate same profile number | Low risk — PoracleNG enforces uniqueness, but UX could be better |
CrossProfileService.cs |
23-24 | GetHumanAsync returning null defaults to profile 1 |
Acceptable — null means user doesn't exist, and caller already validated via GetByUserAndProfileNoAsync. Profile 1 is correct default for edge case |
Architecture Assessment
Clean implementation:
- Proxy-first pattern followed correctly
- No direct DB writes for alarms
finallyblocks ensure profile restoration- Rollback on duplicate failure (matches main's pattern)
- uid defensively stripped on import
- forkJoin coordinates parallel API calls
- ConfirmDialog extended cleanly with reusable
promptField
Integration with main branch:
- Main added
CopyProfileAsync+Duplicateon ProfileController — separate approach, no conflict - Our
CrossProfileControllerhandles the overview + import use cases that main doesn't cover
Test Coverage
| Test File | Tests | Coverage |
|---|---|---|
CrossProfileControllerTests.cs |
6 | GET, duplicate (ok + not found), import (ok + dedup) |
CrossProfileServiceTests.cs |
5 | Overview, duplicate copy, duplicate error recovery, import error recovery |
profile-overview.service.spec.ts |
3 | Service creation, GET, response shape |
| Total | 14 | Good coverage of happy paths + error recovery |
Risk Assessment
| Risk | Level |
|---|---|
| Breaking Change | None |
| Data Integrity | Low — finally blocks + rollback protect state |
| Performance | Low — single allProfiles call |
| Security | Low — all behind [Authorize] |
| Rollback Complexity | Low — remove nav item to disable |
Final Verdict: APPROVED
One should-fix remaining (inner try-catch in rollback), but not blocking. All critical and high-priority issues from previous reviews have been resolved. Feature is solid, well-tested, and architecturally consistent.
Recommendation: Merge after CI passes. Address the rollback exception handling in a follow-up if needed.
- Rename ICrossProfileService → IProfileOverviewService - Rename CrossProfileService → ProfileOverviewService - Rename CrossProfileController → ProfileOverviewController - Rename request DTOs: ProfileOverviewDuplicateRequest, ProfileOverviewImportRequest - Rename test classes to match - Fix: wrap rollback DeleteProfileAsync in inner try-catch to preserve original error - Fix: validate backup alarms is object, reject unsupported backup versions
| .ReturnsAsync(1); | ||
|
|
||
| var request = new ProfileOverviewImportRequest("Work", 1, alarms); | ||
| var result = await this._sut.ImportProfile(request); |
Comment on lines
+81
to
+89
| foreach (var alarm in alarmsArray.EnumerateArray()) | ||
| { | ||
| // Strip uid defensively — export removes it client-side but manually edited backups may include it | ||
| var cleaned = alarm.TryGetProperty("uid", out _) | ||
| ? PoracleJsonHelper.StripProperty(alarm, "uid") | ||
| : alarm; | ||
| await this._trackingProxy.CreateAsync(type, userId, cleaned); | ||
| totalCreated++; | ||
| } |
Comment on lines
+71
to
+74
| catch | ||
| { | ||
| // Rollback failed — log but don't mask the original error | ||
| } |
hokiepokedad2
commented
Apr 7, 2026
Contributor
Author
hokiepokedad2
left a comment
hokiepokedad2
left a comment
There was a problem hiding this comment.
Code Review (Final Pass): PR 147
Executive Summary
| Category | Rating |
|---|---|
| Overall | APPROVED |
| Code Quality | Strong |
| Requirements | All satisfied (exceeds scope) |
| Architecture | Consistent with codebase patterns |
| Risk Level | Low |
| Test Coverage | 637 backend + 490 frontend, all passing |
All Previous Issues — Resolved
| Issue | Status |
|---|---|
| forkJoin in loadAll() | Fixed |
| takeUntilDestroyed in dialogs | Fixed |
| Missing backend tests (duplicate/import/error recovery) | Fixed — 11 tests |
| Defensive uid stripping in import | Fixed |
| Rollback on duplicate failure | Fixed (with inner try-catch) |
| Backup version validation | Fixed |
| Backup alarms type validation | Fixed |
| CrossProfile naming → ProfileOverview | Fixed — all classes, files, DTOs, route, comments |
| API route cross-profile → profile-overview | Fixed |
Naming Audit
Zero remaining references to "CrossProfile" or "cross-profile" in code. Clean rename across:
- IProfileOverviewService / ProfileOverviewService
- ProfileOverviewController (route: api/profile-overview)
- ProfileOverviewDuplicateRequest / ProfileOverviewImportRequest
- ProfileOverviewControllerTests / ProfileOverviewServiceTests
- ProfileOverviewService / profile-overview.service.ts
- ProfileOverview / ProfileOverviewAlarm / ProfileOverviewProfile (models)
- profiles-overview/ module directory
Architecture
- Proxy-first: all alarm ops through IPoracleTrackingProxy
- finally blocks ensure profile restoration on error
- Rollback deletes empty profile on duplicate failure (inner try-catch preserves original error)
- Import defensively strips uid
- forkJoin coordinates parallel API calls
- Angular standalone components with signals, OnPush, inject()
Test Coverage
| Suite | Tests | Key Scenarios |
|---|---|---|
| ProfileOverviewControllerTests | 6 | GET, duplicate (ok/notfound), import (ok/dedup) |
| ProfileOverviewServiceTests | 5 | Overview, copy filtering, error recovery x2 |
| profile-overview.service.spec | 3 | Service creation, GET, response shape |
| Total new | 14 | |
| Total passing | 637 + 490 = 1127 |
Risk Assessment
| Risk | Level |
|---|---|
| Breaking Change | None — new endpoints only |
| Data Integrity | Low — finally + rollback |
| Performance | Low — single API call |
| Security | Low — all behind [Authorize] |
| Rollback | Low — remove nav item |
Final Verdict: APPROVED
All critical, major, and minor issues from three review passes have been resolved. Code is clean, well-tested, and architecturally consistent. Ready to merge.
hokiepokedad2
added a commit
that referenced
this pull request
Apr 7, 2026
hokiepokedad2
added a commit
that referenced
this pull request
Apr 11, 2026
Pre-existing IDE1006 naming-rule violation on a static readonly field in AreaControllerTests, surfaced by `dotnet format --verify-no-changes` when running against my touched files. Static readonly fields should be PascalCase per the project convention. Originated in PR #147 (commit 611ed09) but flagged on this branch because the test file was also touched by the geofence fix.
hokiepokedad2
added a commit
that referenced
this pull request
Apr 11, 2026
* fix: restore direct-DB writes for user geofence area mutations (#163) PoracleNG's POST /api/humans/{id}/setAreas intersects requested areas against fences where userSelectable=true for non-admin users. PoracleWeb serves user-drawn geofences with userSelectable=false to hide them from the Poracle bot's area picker, so every call to SetAreasAsync containing a user geofence name was silently stripped by the filter — the API returned 200 OK with the filtered list and the toggle appeared to work, but a page refresh read back humans.area without the geofence. Regression was introduced in v2.0.0 (#88) by the PoracleNG proxy migration, which routed UserGeofenceService.AddAreaToHumanAsync through the proxy instead of writing directly to the Poracle DB like the pre-migration code did. Fix: - UserGeofenceService now writes directly to humans.area and the active profiles.area row via IHumanRepository / IProfileRepository for all user geofence mutations (Create, Delete, AddToProfile, RemoveFromProfile, AdminDelete). Dual-write uses human.CurrentProfileNo as the authoritative active-profile source. - New IUserGeofenceService.PreserveOwnedAreasInHumanAsync method accepts a list of requested area names and re-adds the subset that the user owns as custom geofences via direct DB. - AreaController.UpdateAreas calls PreserveOwnedAreasInHumanAsync after SetAreasAsync so saving on the Areas page also no longer strips active user geofences. Response returns the effective merged list. Tests: - Rewrote 5 existing tests that asserted on SetAreasAsync calls to assert direct-DB writes and verify SetAreasAsync is never called for user geofence mutations. - Added AddToProfileAsyncWritesToCurrentProfileNoFromHuman regression test ensuring the JWT profileNo claim is ignored in favor of the authoritative human.CurrentProfileNo. - Added 3 PreserveOwnedAreasInHumanAsync tests covering the owned-filter logic, empty-candidates short-circuit, and no-owned-geofences short-circuit. - Added UpdateAreasPreservesUserOwnedGeofencesAfterProxyCall controller test guarding the merge-after-setAreas behavior. Also includes: - CLAUDE.md section explaining the PoracleNG setAreas filter and why direct-DB is load-bearing for user geofences. - dotnet format cleanups of pre-existing whitespace on Profile.cs and several test files (no behavior change). Fixes #163 * fix: trigger PoracleNG reload after direct-DB geofence mutations Review follow-up: the proxy-based setAreas path used to trigger PoracleNG's internal reloadState automatically as part of its HandleSetAreas handler. Since UserGeofenceService.AddToProfileAsync and RemoveFromProfileAsync now write directly to the Poracle DB (bypassing the userSelectable filter), they were no longer triggering a reload — meaning the toggle would take effect only on the next organic PoracleNG state reload (potentially minutes later). Add ReloadGeofencesSafeAsync calls at the end of: - AddToProfileAsync - RemoveFromProfileAsync - PreserveOwnedAreasInHumanAsync (only when toRestore.Count > 0, since the caller already triggered a reload via setAreas) Add test assertions verifying ReloadGeofencesAsync is called on these paths and NOT called when PreserveOwnedAreasInHumanAsync has nothing to restore. * docs: flag direct-DB geofence writes as HACK pending PoracleNG fix Document the direct-DB writes from the previous two commits as a known workaround for a PoracleNG API gap. The real fix belongs in PoracleNG — this PR is a temporary shim while that lands. - docs/poracleng-enhancement-requests.md: new "Trusted setAreas (bypass userSelectable filter)" entry with ID trusted-set-areas, root cause, three proposed resolutions (query flag, dedicated endpoint, or per-fence ownership), and the workaround description. Also updated the existing "Atomic Area Update" entry from "adopted" to "partially adopted" and cross-linked it. - UserGeofenceService.cs: HACK comments on AddAreaToHumanAsync, RemoveAreaFromHumanAsync, RemoveAreaFromAllProfilesAsync, and PreserveOwnedAreasInHumanAsync pointing to trusted-set-areas. - AreaController.cs: HACK comment on the PreserveOwnedAreasInHumanAsync call in UpdateAreas. No behavior change. When PoracleNG ships the trusted setAreas variant, search for "HACK: trusted-set-areas" to find every site that needs to be reverted to the proxy path. * docs: tag manual reload calls as part of the trusted-set-areas hack The direct-DB writes skip PoracleNG's HandleSetAreas handler and therefore also skip its terminal reloadState(deps) call. The manual ReloadGeofencesSafeAsync calls in AddToProfileAsync / RemoveFromProfileAsync / PreserveOwnedAreasInHumanAsync compensate for that, and should be removed together with the direct-DB writes when PoracleNG ships a trusted setAreas variant. - UserGeofenceService.cs: HACK: trusted-set-areas comments on the three manual ReloadGeofencesSafeAsync callsites explaining why they exist and that they go away with the rest of the workaround. - poracleng-enhancement-requests.md: added a paragraph to the Trusted setAreas gap entry documenting the manual reload as part of the same HACK surface. * docs: clarify changelog — trusted-set-areas fix is a temporary HACK * refactor: introduce IUserAreaDualWriter for atomic area mutations Addresses all issues from the PR #164 self-review: 1. **Transactional drift (review issue #1)**: Previously AddAreaToHumanAsync did two separate IHumanRepository.UpdateAsync + IProfileRepository.UpdateAsync calls, each committing its own SaveChangesAsync. If the profile update threw mid-operation, humans.area held the name but profiles.area didn't — a profile switch would then wipe both. New IUserAreaDualWriter owns the PoracleContext directly, mutates both entities in-memory, and commits them in a single SaveChangesAsync call. EF Core wraps that in an implicit transaction, so the two writes cannot drift. 2. **Silent no-op on null human (review issue #2)**: The writer throws InvalidOperationException when the human row is missing instead of returning silently. UserGeofenceService's ownership check already runs before the writer call, and the controller catches InvalidOperationException and maps it to 404 — so a TOCTOU-deleted user now gets an honest error instead of a misleading 204 "success". 3. **Redundant Union in AreaController.UpdateAreas (review issue #3)**: PreserveOwnedAreasInHumanAsync's return value is always a subset of the input, so Union(normalizedAreas, restored) is always equivalent to normalizedAreas. Simplified to `return this.Ok(normalizedAreas)` with a discard on the preserve call. Frontend ignored the response body anyway. 4. **PreserveOwnedAreasInHumanAsync loop re-reads (review issue #4)**: The old implementation called AddAreaToHumanAsync in a foreach loop, reading human + profile from the DB per iteration (up to 2N reads + 2N writes for N candidates). The new AddAreasToActiveProfileAsync bulk method on the writer does one human read, one profile read, one SaveChangesAsync — regardless of N. Architecture changes: - NEW: Core/Pgan.PoracleWebNet.Core.Abstractions/Repositories/IUserAreaDualWriter.cs - NEW: Core/Pgan.PoracleWebNet.Core.Repositories/UserAreaDualWriter.cs - UserGeofenceService drops its IProfileRepository dependency (no longer needed) and gains IUserAreaDualWriter. IHumanRepository remains because it's still used by GetAllWithDetailsAsync, SubmitForReviewAsync, and ApproveSubmissionAsync. - Three private direct-DB methods (AddAreaToHumanAsync, RemoveAreaFromHumanAsync, RemoveAreaFromAllProfilesAsync) deleted — replaced by writer method calls. - DI: IUserAreaDualWriter registered as scoped in ServiceCollectionExtensions. Tests: - NEW: Tests/Pgan.PoracleWebNet.Tests/Repositories/UserAreaDualWriterTests.cs 19 EF Core in-memory tests covering atomic dual-write, idempotency, CurrentProfileNo routing, human-missing throws, permissive delete semantics, bulk add, case-insensitive dedup, and every-profile removal. - UserGeofenceServiceTests: rewrote 5 tests to assert on the writer mock instead of the individual repository mocks. The tests are simpler now (one-line Verify on the writer call) because the atomic-dual-write contract lives in UserAreaDualWriterTests. - NEW: AddToProfileAsyncPropagatesWriterExceptionsAsNotFound — pins issue #2 fix (writer throws → exception propagates). - NEW: the not-owned tests now also assert the writer is NEVER called when the ownership check fails. Test count: 780 → 797 (+17 net = 19 new writer tests + 2 new service tests - 4 removed tests that were testing the direct-DB read-modify-write logic now covered atomically by the writer tests). Docs: - docs/poracleng-enhancement-requests.md: updated Trusted setAreas gap entry to reference the new IUserAreaDualWriter abstraction and explain the single-SaveChanges atomicity guarantee. - CLAUDE.md: updated Areas, Custom Geofences, and User Geofence Persistence sections. Added IUserAreaDualWriter and UserAreaDualWriter to the File Locations table. - CHANGELOG: updated Unreleased entry to reflect the writer refactor. HACK inventory unchanged (11 sites) — the workaround surface is the same, just cleaner under the hood. * refactor: address fresh review nits on IUserAreaDualWriter Addresses the non-blocking findings from the third review pass: 1. **ParseAreas duplication (issue #1)**: Extracted to new AreaListJson static helper under Core.Models.Helpers — both UserGeofenceService (Core.Services) and UserAreaDualWriter (Core.Repositories) can reach it via their transitive reference to Core.Models through Core.Abstractions. Both call sites now delegate to AreaListJson.Parse / AreaListJson.Serialize. Serialize() also guards against empty input always returning "[]" rather than the JsonSerializer default (which is the same but is now explicit and documented). 2. **Input validation (issue #4)**: Added ArgumentException.ThrowIfNullOrWhiteSpace guards to every public writer method for humanId + areaName. AddAreasToActiveProfileAsync also validates the collection is non-null and strips blank/whitespace-only entries from the input before processing — they'd be indistinguishable from "no area" and shouldn't bloat the persisted list. 3. **Bulk Contains micro-optimization (issue #5)**: AddAreasToActiveProfileAsync's inner loop used List<string>.Contains which was O(N·M). Switched to HashSet<string> lookup — O(N+M) — while keeping the backing list for serialization so the on-disk ordering is stable across writes. For N=10, M=10 the change is 100 → 20 operations per call, which is academic but removes a code smell. Issues #2 (InMemory vs MySQL integration tests) and #3 (concurrency token) remain out of scope — both require test infrastructure or PoracleJS-compatible migrations that don't belong in this PR. Tests: - NEW: Tests/Pgan.PoracleWebNet.Tests/Helpers/AreaListJsonTests.cs 9 tests covering null/empty/whitespace input, JSON array parsing, legacy CSV fallback (preserves backwards compatibility for rows written by older PoracleWeb versions), empty serialization, and round-trip. - Added 22 new guard-clause tests to UserAreaDualWriterTests covering null/empty/whitespace humanId + areaName on all 4 methods, plus null collection on the bulk add, plus blank-entry stripping in AddAreasToActiveProfileAsync. - Tests use ThrowsAnyAsync<ArgumentException> (not ThrowsAsync) so they accept both ArgumentException and ArgumentNullException — ThrowIfNullOrWhiteSpace throws the subtype on null input and xUnit's ThrowsAsync is exact-type match. Test count: 797 → 828 (+31 new tests, 0 removed). No HACK surface change — the workaround footprint stays at 11 sites. * refactor: harden AreaListJson and unify writer single-item add path Addresses the nits surfaced in review pass 4: 1. **AreaListJson CSV-fallback misparse on bracketed input**: Inputs that fail JSON deserialization but start with `[` or `{` previously fell through to the CSV split, turning garbage like `[1,2,3]` into the "area list" `["[1", " 2", " 3]"]`. Parse() now checks the first non-whitespace character and returns an empty list on failed parse for bracketed input — only unbracketed input falls back to CSV. 2. **Single-item AddAreaToActiveProfileAsync uses different code path from bulk**: The single-item method previously had its own read-modify-write loop with `List.Contains` while the bulk method used the `HashSet` lookup. Single-item now delegates to the bulk path so there's exactly one read-modify-write implementation. The wrapper still validates both humanId and areaName up-front so the single-item contract fails fast on blank inputs (the bulk path strips blanks from its input collection rather than throwing, which is the right call for a list parameter but wrong for a single-name parameter). 3. **FortChange.cs:85 review finding**: Re-checked — that line is inside a JsonConverter handling double-encoded API strings from PoracleNG, NOT a database column parser. Unrelated to AreaListJson. The review finding was a false positive; no change needed. Tests: - 4 new AreaListJsonTests covering the bracketed-misparse guard: `[1,2,3]`, `[{"nested":"object"}]`, `{"not":"an_array"}` all return empty (not CSV garbage). The new `[InlineData(/*lang=json,strict*/...)]` hint comments silence the JSON002 analyzer warning that flagged the test inputs as JSON literals (matches the existing convention used in AreaControllerTests). - 1 new AreaListJsonTests `ParseStillFallsBackToCsvForNonBracketedInput` guards against the bracketed-input fix being too aggressive — legacy CSV rows still parse correctly. Test count: 828 → 832 (+4 new helper tests, no test removals). The 12 existing single-item AddAreaToActiveProfileAsync tests all pass unchanged after the delegation refactor — the bulk path correctly handles single-element collections, lowercases input, dedupes on insert, routes by human.CurrentProfileNo, throws on missing human, and tolerates missing profile rows. * style: rename areasArray to AreasArray (IDE1006 PascalCase) Pre-existing IDE1006 naming-rule violation on a static readonly field in AreaControllerTests, surfaced by `dotnet format --verify-no-changes` when running against my touched files. Static readonly fields should be PascalCase per the project convention. Originated in PR #147 (commit 611ed09) but flagged on this branch because the test file was also touched by the geofence fix. * refactor: tighten Parse JSON detection and make remove case-insensitive Addresses the actionable nits from review pass 5 + a follow-on asymmetry caught during the iteration-2 internal review: 1. AreaListJson.Parse — JSON detection now also covers `"`-prefixed input. Quoted JSON string literals like `"foo"` previously fell through to the CSV split, producing `["\"foo\""]` (one entry with literal quotes). Now they correctly return empty. Bare JSON literals (true/false/null/numbers) still fall to CSV because we can't distinguish them from legacy CSV values starting with the same characters (e.g. legacy "1st avenue" or "north park"). Tightening the detection further would regress real CSV inputs. 2. UserAreaDualWriter HashSet builds in AddAreasToActiveProfileAsync now use StringComparer.OrdinalIgnoreCase. Defensive against the DB ever holding mixed-case area names (manual DB poke, future PoracleJS change, mid-migration state) — the dedup will still correctly treat "Downtown" and "downtown" as the same entry instead of producing both. 3. Symmetric case-insensitive removal: RemoveAreaFromActiveProfileAsync and RemoveAreaFromAllProfilesAsync previously used List.Remove which is case-sensitive. The Add side became case-insensitive in step 2 above, leaving an asymmetry where Add would dedupe correctly but Remove would silently fail to delete a mixed-case entry. Both methods now use RemoveAll with OrdinalIgnoreCase comparison for consistency. Tests: - 2 new AreaListJsonTests (`ParseReturnsEmptyForJsonStringLiteralInsteadOfQuotedGarbage`) pinning the `"foo"` and `"some,csv,looking,thing"` quoted-literal cases. - 1 new writer test (`AddAreasToActiveProfileAsyncDoesNotDuplicateWhenDbHasMixedCase`) verifying mixed-case input doesn't produce duplicates. - 2 new writer tests (`RemoveAreaFromActiveProfileAsyncIsCaseInsensitive`, `RemoveAreaFromAllProfilesAsyncIsCaseInsensitive`) verifying the symmetric case-insensitive removal behavior on both single-profile and all-profiles paths. Test count: 832 → 837 (+5 new tests). Also moved a `// Guard clause:` comment out from between an expression- bodied method's `=>` and the call expression — `dotnet format` flagged the layout as a WHITESPACE error and the formatter couldn't auto-fix it. Hoisted the comment above the [Theory] attribute group instead. * refactor: extract RemoveCaseInsensitive helper in writer Iteration-2 internal review caught a DRY issue: the `list.RemoveAll(a => string.Equals(a, lowerName, StringComparison.OrdinalIgnoreCase)) > 0` pattern was duplicated 4 times across RemoveAreaFromActiveProfileAsync (human + active profile) and RemoveAreaFromAllProfilesAsync (human + all-profiles loop). Extracted to a private static `RemoveCaseInsensitive(List<string>, string)` helper on the writer class. Each call site is now one line and the case-insensitive contract is documented in one place via the helper's XML doc. No behavior change. All 837 tests still pass.
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
1 participant
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.
Summary
GET /api/tracking/allProfiles/{id}?includeDescriptions=trueendpointprofile_nofor PoracleNG compatibilityNew Files
ICrossProfileService/CrossProfileService— allProfiles proxy, duplicate, importCrossProfileController— GET overview, POST duplicate, POST import endpointscross-profile-overview.component(ts/html/scss) — unified profiles pagecross-profile.service.ts— frontend HTTP serviceTest plan
Closes #127