Fix coaching relationship sync when navigating between sessions #231
Conversation
… sessions When navigating between sessions with different relationships, the page displayed wrong coachee names while showing correct notes. Fixed by: - Updated auto-sync to trigger when relationship ID differs (not just when empty) - Added shouldSyncRelationship helper for readability - Call refresh() after setting new relationship ID to fetch correct data - Added comprehensive test coverage for all sync scenarios Fixes #228
- Extract shouldSyncRelationship to separate module with unit tests - Add explicit return type interfaces for hooks - Create type-safe test factory for CoachingSession (removes 'as any') - Add const assertions to test mock parameters for immutability - Rename coaching_relationship.ts to coaching-relationship.ts for consistency - Update all imports to use hyphenated naming convention - Fix type signatures to allow null coaching relationship IDs - Add 9 comprehensive unit tests for shouldSyncRelationship helper These changes improve type safety, maintainability, and follow TypeScript best practices without changing functionality.
These documentation files were committed by mistake and should not be part of this PR.
calebbourg
left a comment
calebbourg
left a comment
There was a problem hiding this comment.
Few minor things but looks good!
| push: vi.fn(), | ||
| replace: vi.fn(), | ||
| } | ||
| } as const |
There was a problem hiding this comment.
This seems like a potential typing red flag. Do we know that these are absolutely necessary?
| /> | ||
| <CoachingSessionSelector | ||
| relationshipId={currentCoachingRelationshipId} | ||
| relationshipId={currentCoachingRelationshipId || ""} |
There was a problem hiding this comment.
Is setCurrentCoachingRelationshipId sometimes null or undefined?
This feels somewhat strange (but maybe is idiomatic?)
I would expect a boolean check inside the CoachingSessionSelector to determine if the relationshipId is a valid value.
This feels like it's converting from one falsey value to another.
If we do in fact need to ensure that the non-present case is an empty string, can we do that conversion inside CoachingSessionSelector since that is where the requirement exists?
Let me know if that seems reasonable or feasible
| /** | ||
| * Return type for the useCurrentCoachingRelationship hook. | ||
| */ | ||
| export interface UseCurrentCoachingRelationshipReturn { |
There was a problem hiding this comment.
Thank you for formalizing the type here!!
I wonder if Result would be more consistent with the rest of the application, than Return in the name here?
| @@ -0,0 +1,36 @@ | |||
| /** | |||
There was a problem hiding this comment.
I might suggest defining this inline in the Coaching Session page file since it's only used there and the logic is simple.
We can always extract it later if it's used elsewhere, I tend to favor the "Law of Three".
Not a blocker if you feel it should be here.
Description
Fixes the bug where navigating between coaching sessions with different relationships (e.g. from Today's Sessions) displays the wrong coachee name in the session title while showing the correct notes.
GitHub Issue: Fixes #228
Changes
shouldSyncRelationshiphelper function for improved readability and maintainabilityrefresh()call after setting new relationship ID to force immediate fetch of correct relationship dataTesting Strategy
Automated:
Manual verification needed:
Concerns
None - the fix aligns with the codebase's URL-first architecture and preserves all existing functionality.