fix(mapping): UI updates after match, persist character name - #25
Merged
Conversation
…perfluous WriteHeader
- Associations API now returns `charId`/`charName` (was `characterId` only),
fixing the "unknown" name and the matched character still appearing in
the unassociated list until refresh.
- `AssociateCharacter` persists `charName`; handler accepts it from POST body.
- POST/DELETE responses include `{success, message}` so the frontend's
optimistic update branch actually runs (was returning 204/no body).
- Remove duplicate `w.WriteHeader` in CreateAssociation that was logging
"superfluous response.WriteHeader call".
- Migrate 5 MUI Dialogs from deprecated `PaperProps` to `slotProps.paper`
to silence React DOM unknown-attribute warning under MUI v9.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAssociation flows now accept and persist a new ChangesCharacter Association Enhancement
MUI Dialog Styling Migration
sequenceDiagram
participant Client as Browser UI
participant API as HTTP Handler (/api/associations)
participant Service as AccountManagementService
participant DB as Database
Client->>API: POST /api/associations { userId, characterId, charName }
API->>Service: AssociateCharacter(userId, characterId, charName)
alt association exists
Service->>DB: update association (UserId, CharName if non-empty)
else new association
Service->>DB: insert association (UserId, CharId, CharName)
end
Service-->>API: result { success, message, userId, charId, charName }
API-->>Client: 200 JSON { success, message, userId, charId, charName }
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/services/account/account_management_service.go (1)
349-374:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
AssociateCharacterwith the service mutex to prevent lost updates.This method mutates shared persisted state via a read-modify-write cycle without locking. Concurrent association requests can overwrite each other.
🔧 Proposed fix
func (s *AccountManagementService) AssociateCharacter(userId, charId, charName string) error { + s.mu.Lock() + defer s.mu.Unlock() + accountData, err := s.storage.LoadAccountData() if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/services/account/account_management_service.go` around lines 349 - 374, AssociateCharacter performs a read-modify-write on persisted account data without synchronization, causing lost updates under concurrency; guard the entire operation by adding/using a mutex on AccountManagementService (e.g., a sync.Mutex or sync.RWMutex field like mu) and call s.mu.Lock() before loading/modifying/saving (with defer s.mu.Unlock()) so the sequence around storage.LoadAccountData and storage.SaveAccountData is atomic; if the service lacks a mutex field, add one to the struct and use it in AssociateCharacter to serialize concurrent calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/services/account/account_management_service.go`:
- Around line 349-374: AssociateCharacter performs a read-modify-write on
persisted account data without synchronization, causing lost updates under
concurrency; guard the entire operation by adding/using a mutex on
AccountManagementService (e.g., a sync.Mutex or sync.RWMutex field like mu) and
call s.mu.Lock() before loading/modifying/saving (with defer s.mu.Unlock()) so
the sequence around storage.LoadAccountData and storage.SaveAccountData is
atomic; if the service lacks a mutex field, add one to the struct and use it in
AssociateCharacter to serialize concurrent calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 353606cf-24aa-4b00-94ea-694d8db174b0
📒 Files selected for processing (10)
internal/handlers/associations.gointernal/services/account/account_management_service.gointernal/services/interfaces/account_management.gointernal/testutil/mock_interfaces.gorenderer/src/api/accountsApi.jsrenderer/src/components/common/AccountPromptModal.jsxrenderer/src/components/common/CharacterDetailModal.jsxrenderer/src/components/common/CustomConfirmDialog.jsxrenderer/src/components/setup/FirstRunDialog.jsxrenderer/src/components/skillplan/AddSkillPlanModal.jsx
…update test - Take s.mu in AssociateCharacter and UnassociateCharacter to prevent lost updates from concurrent read-modify-write on persisted account data (CodeRabbit review). - Update accountsApi test to assert charName in the POST body now that the client sends it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/services/account/account_management_service.go (1)
353-356: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd contextual wrapping and error logs in association persistence paths.
These storage failures are returned raw, which loses operation context and reduces observability in production incidents.
Proposed patch
func (s *AccountManagementService) AssociateCharacter(userId, charId, charName string) error { s.mu.Lock() defer s.mu.Unlock() accountData, err := s.storage.LoadAccountData() if err != nil { - return err + s.logger.Errorf("AssociateCharacter: failed to load account data: %v", err) + return fmt.Errorf("associate character: load account data: %w", err) } @@ - return s.storage.SaveAccountData(accountData) + if err := s.storage.SaveAccountData(accountData); err != nil { + s.logger.Errorf("AssociateCharacter: failed to save updated association for charId=%s userId=%s: %v", charId, userId, err) + return fmt.Errorf("associate character: save updated association: %w", err) + } + return nil } } @@ - return s.storage.SaveAccountData(accountData) + if err := s.storage.SaveAccountData(accountData); err != nil { + s.logger.Errorf("AssociateCharacter: failed to save new association for charId=%s userId=%s: %v", charId, userId, err) + return fmt.Errorf("associate character: save new association: %w", err) + } + return nil } func (s *AccountManagementService) UnassociateCharacter(userId, charId string) error { s.mu.Lock() defer s.mu.Unlock() accountData, err := s.storage.LoadAccountData() if err != nil { - return err + s.logger.Errorf("UnassociateCharacter: failed to load account data: %v", err) + return fmt.Errorf("unassociate character: load account data: %w", err) } @@ - return s.storage.SaveAccountData(accountData) + if err := s.storage.SaveAccountData(accountData); err != nil { + s.logger.Errorf("UnassociateCharacter: failed to save account data for charId=%s userId=%s: %v", charId, userId, err) + return fmt.Errorf("unassociate character: save account data: %w", err) + } + return nil }As per coding guidelines, "Use structured errors with proper logging in Go backend code" and "Use logrus with appropriate log levels for logging in Go backend code".
Also applies to: 365-366, 376-377, 383-386, 396-397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/services/account/account_management_service.go` around lines 353 - 356, The storage calls like s.storage.LoadAccountData() (and the other storage calls in the association persistence paths) return raw errors; update each call to log a structured message with logrus including context and the error (e.g., logrus.WithError(err).WithFields(...).Error("failed to load account data") ), and return a wrapped error using fmt.Errorf("failed to load account data: %w", err) (or errors.Wrapf) so callers retain context; apply the same pattern to the other storage calls referenced (the similar s.storage.* calls in the association persistence code paths).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/services/account/account_management_service.go`:
- Around line 353-356: The storage calls like s.storage.LoadAccountData() (and
the other storage calls in the association persistence paths) return raw errors;
update each call to log a structured message with logrus including context and
the error (e.g., logrus.WithError(err).WithFields(...).Error("failed to load
account data") ), and return a wrapped error using fmt.Errorf("failed to load
account data: %w", err) (or errors.Wrapf) so callers retain context; apply the
same pattern to the other storage calls referenced (the similar s.storage.*
calls in the association persistence code paths).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02ace0cc-b7ca-4c21-8ebe-d845f3952734
📒 Files selected for processing (3)
.claude/settings.local.jsoninternal/services/account/account_management_service.gorenderer/src/api/accountsApi.test.js
Per CodeRabbit review: storage calls in AssociateCharacter,
UnassociateCharacter, and GetAssociations now wrap errors with
fmt.Errorf("...: %w", err) so the handler's existing log line includes
which storage operation failed. Skipped the suggestion to add structured
service-level logging since the handlers already log with context — adding
service logs would just duplicate them.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
charId/charName(frontend was reading undefinedcharId), fixing the "unknown" name and matched characters still showing as selectable until manual refresh.AssociateCharacternow persistscharName; POST/DELETE responses include{success, message}so the frontend's optimistic update path actually fires — no more refresh-to-see-the-match.w.WriteHeaderinCreateAssociation(was loggingsuperfluous response.WriteHeader call).PaperPropstoslotProps.paper(MUI v9) — silences the React DOM unknown-attribute warning.Test plan
superfluous response.WriteHeaderlog line on associate.PaperPropsReact warning in devtools.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests