fix: summary does not load indefinitely if there are no votes #10669
Conversation
| @@ -112,7 +112,8 @@ const safeEndRetrospective = async ({ | |||
| endedAt: sql`CURRENT_TIMESTAMP`, | |||
| phases: JSON.stringify(phases), | |||
| usedReactjis: JSON.stringify(insights.usedReactjis), | |||
| engagement: insights.engagement | |||
| engagement: insights.engagement, | |||
| summary: '' // empty string signals "loading" to the client | |||
There was a problem hiding this comment.
Other ideas are:
- Setting it to a specific string like "Loading" to make this clearer in the
WholeMeetingSummary - Adding a value to the meeting like
isSummaryLoading, but that feels like overkill - Checking in
WholeMeetingSummarywhether there are no votes in the Retro, but that wouldn't handle the situation where OpenAI fails.
| @@ -47,7 +47,7 @@ const WholeMeetingSummary = (props: Props) => { | |||
| const reflections = reflectionGroups?.flatMap((group) => group.reflections) // reflectionCount hasn't been calculated yet so check reflections length | |||
| const hasMoreThanOneReflection = reflections?.length && reflections.length > 1 | |||
| if (!hasMoreThanOneReflection || !organization.useAI || !hasAiApiKey) return null | |||
| if (!wholeMeetingSummary) return <WholeMeetingSummaryLoading /> | |||
| if (wholeMeetingSummary === '') return <WholeMeetingSummaryLoading /> // summary is undefined if it hasn't been generated yet. it's null if unsuccessful | |||
There was a problem hiding this comment.
-1 GraphQL does not distinguish between null and undefined like this, so while it does not change anything here, the comment is wrong.
I think because the summary is only used for the AI summary, it's ok to have a placeholder in the db indicating loading, but I would prefer if on GraphQL level we would add a field isSummaryLoading which checks the placeholder. We could also filter it out for the summary field. This way it's contained to the server only. Not sure '' is the best placeholder though, maybe something like <loading> would be more explicit?
| @@ -14,6 +14,7 @@ const RetrospectiveMeeting: RetrospectiveMeetingResolvers = { | |||
| const res = await dataLoader.get('meetingMembersByMeetingId').load(meetingId) | |||
| return res as RetroMeetingMember[] | |||
| }, | |||
| isLoadingSummary: ({summary}) => summary === '<loading>', | |||
There was a problem hiding this comment.
+1 to ensure we never accidentally render the placeholder on the client, we could add a resolver here
summary: ({summary}) => summary === '<loading>' ? null : summary,
Demo: https://www.loom.com/share/974d623e1e5f4864bc0fbd4947cac20b
Fix #10499
In prod, if the AI summary fails to generate because of a problem with ChatGPT or because the retro doesn't have any votes so
generateRetroSummarydoesn't have any valid reflection groups, the AI meeting summary will load indefinitely.In this PR, if we fail to generate the AI summary, we won't show the loading UI indefinitely.
AC