-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Join session milestone point #43
Changes from all commits
bb9ef5e
0a28c00
6f5423f
02278df
f413887
d4ab474
2821e62
bdc92ec
c6bd7ff
766bd8e
4889b1e
27239a9
9f5d37b
bc2ff8e
d93108a
96a4d59
1607fbd
c4206d2
fd0b4ea
68f4ed5
ad4c6a4
2bdae0f
7f7011a
6e898e6
5efcf61
f9e79df
1ad2df8
b8afc27
dc32d21
6cc4734
4354aa0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
"use client"; | ||
|
||
import { useEffect, useState } from "react"; | ||
import { useEffect, useState, useCallback } from "react"; | ||
import { ActionsList } from "@/components/ui/coaching-sessions/actions-list"; | ||
import { ItemStatus, Id } from "@/types/general"; | ||
import { Action } from "@/types/action"; | ||
|
@@ -119,37 +119,40 @@ const OverarchingGoalContainer: React.FC<{ | |
}); | ||
}; | ||
|
||
useEffect(() => { | ||
async function fetchOverarchingGoal() { | ||
if (!coachingSession.id) { | ||
console.error( | ||
"Failed to fetch Overarching Goal since coachingSession.id is not set." | ||
const fetchOverarchingGoal = useCallback(async () => { | ||
if (!coachingSession.id) { | ||
console.error( | ||
"Failed to fetch Overarching Goal since coachingSession.id is not set." | ||
); | ||
return; | ||
} | ||
|
||
try { | ||
const goals = await fetchOverarchingGoalsByCoachingSessionId( | ||
coachingSession.id | ||
); | ||
if (goals.length > 0) { | ||
const goal = goals[0]; | ||
console.trace("Overarching Goal: " + overarchingGoalToString(goal)); | ||
if (goal.id !== goalId) { | ||
setGoalId(goal.id); | ||
setGoal(goal); | ||
} | ||
} else { | ||
console.trace( | ||
"No Overarching Goals associated with this coachingSession.id" | ||
); | ||
return; | ||
} | ||
|
||
await fetchOverarchingGoalsByCoachingSessionId(coachingSession.id) | ||
.then((goals) => { | ||
const goal = goals[0]; | ||
if (goals.length > 0) { | ||
console.trace("Overarching Goal: " + overarchingGoalToString(goal)); | ||
setGoalId(goal.id); | ||
setGoal(goal); | ||
} else { | ||
console.trace( | ||
"No Overarching Goals associated with this coachingSession.id" | ||
); | ||
} | ||
}) | ||
.catch((err) => { | ||
console.error( | ||
"Failed to fetch Overarching Goal for current coaching session: " + | ||
err | ||
); | ||
}); | ||
} catch (err) { | ||
console.error( | ||
"Failed to fetch Overarching Goal for current coaching session: " + err | ||
); | ||
} | ||
}, [coachingSession.id, goalId, setGoalId, setGoal]); | ||
|
||
useEffect(() => { | ||
fetchOverarchingGoal(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @qafui this is necessary, right now if you go to a coaching session page, if you've set an OverarchingGoal, it isn't getting loaded when the page loads. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! I should have re-tested that bit. I noticed it was re-rendering in the console and thought to fix it with memoization but the thought somehow trailed off. I was too tired 😸 |
||
}, [coachingSession.id, goalId]); | ||
}, [fetchOverarchingGoal]); | ||
|
||
const handleGoalChange = async (newGoal: OverarchingGoal) => { | ||
console.trace("handleGoalChange (goal to set/update): " + newGoal.title); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok if it fixes the fetch loop (not sure if it was the cause or not). We'll convert to de-duplicating storing these Ids separately from their object instances after we land this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the cause but a small optimization after observing that we weren't using any other properties of the object in the component.
I could be wrong but the way I understand it, this is a case of whether we want to pass around the string value or the reference to the object (while operating on its mutable property).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm open to your thoughts here on what's best long term for convenience and efficiency.