feat: generate a summary of meeting summaries #10017
Merged
Conversation
This file contains 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
WalkthroughThe changes introduce a new GraphQL mutation for generating insights based on team performance across specified date ranges. This feature encompasses robust error handling, data retrieval from multiple sources, and structured response types. Additionally, it strengthens the permissions model by limiting access to superusers. New helper functions and types were created to facilitate effective insight processing, thereby enhancing the overall functionality of the application. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Database
participant OpenAI
Client->>Server: Call generateInsight(teamId, startDate, endDate)
Server->>Database: Validate and retrieve data
Database-->>Server: Return meeting data
Server->>OpenAI: Generate insights from data
OpenAI-->>Server: Return insights
Server-->>Client: Return GenerateInsightSuccess
Assessment against linked issues
Possibly related issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review as they are similar to previous changes (3)
Additional context usedBiome
Additional comments not posted (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| return null | ||
| } | ||
|
|
||
| return data |
There was a problem hiding this comment.
Log the error before returning null.
Logging the error can help in debugging.
+ console.error('Error in generateInsight:', e)Committable suggestion was skipped due to low confidence.
Comment on lines
348
to
352
| async generateInsight( | ||
| yamlData: string, | ||
| useSummaries: boolean, | ||
| userPrompt?: string | ||
| ): Promise<InsightResponse | null> { |
There was a problem hiding this comment.
Consider making the meeting URL a configurable parameter.
Hardcoding the URL might limit flexibility. Consider passing it as a parameter or using a configuration file.
- const meetingURL = 'https://action.parabol.co/meet/'
+ const meetingURL = process.env.MEETING_URL || 'https://action.parabol.co/meet/'Committable suggestion was skipped due to low confidence.
Comment on lines
+394
to
+410
| try { | ||
| const response = await this.openAIApi.chat.completions.create({ | ||
| model: 'gpt-4o', | ||
| messages: [ | ||
| { | ||
| role: 'user', | ||
| content: `${prompt}\n\n${yamlData}` | ||
| } | ||
| ], | ||
| response_format: { | ||
| type: 'json_object' | ||
| }, | ||
| temperature: 0.7, | ||
| top_p: 1, | ||
| frequency_penalty: 0, | ||
| presence_penalty: 0 | ||
| }) |
There was a problem hiding this comment.
Enhance error handling by providing more context in error messages.
Providing more context in error messages can help in debugging.
- const error = e instanceof Error ? e : new Error('Error in generateInsight')
+ const error = e instanceof Error ? e : new Error(`Error in generateInsight: ${e}`)Committable suggestion was skipped due to low confidence.
| } | ||
|
|
||
| if (!res.comments || !res.comments.length) { | ||
| delete (res as any).comments |
There was a problem hiding this comment.
Replace delete operator with assignment to undefined.
The delete operator can impact performance. Replace it with an assignment to undefined.
- delete (res as any).comments
+ (res as any).comments = undefinedCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| delete (res as any).comments | |
| (res as any).comments = undefined |
Tools
Biome
[error] 178-178: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Comment on lines
7
to
45
| const generateInsight: MutationResolvers['generateInsight'] = async ( | ||
| _source, | ||
| {teamId, startDate, endDate, useSummaries = true, prompt}, | ||
| {dataLoader} | ||
| ) => { | ||
| if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) { | ||
| return standardError( | ||
| new Error('Invalid date format. Please use ISO 8601 format (e.g., 2024-01-01T00:00:00Z).') | ||
| ) | ||
| } | ||
| const oneWeekInMs = 7 * 24 * 60 * 60 * 1000 | ||
| if (endDate.getTime() - startDate.getTime() < oneWeekInMs) { | ||
| return standardError(new Error('The end date must be at least one week after the start date.')) | ||
| } | ||
|
|
||
| const response = useSummaries | ||
| ? await getSummaries(teamId, startDate, endDate, prompt as string) | ||
| : await getTopics(teamId, startDate, endDate, dataLoader, prompt as string) | ||
|
|
||
| if ('error' in response) { | ||
| return response | ||
| } | ||
| const {wins, challenges} = response | ||
| const pg = getKysely() | ||
|
|
||
| await pg | ||
| .insertInto('Insight') | ||
| .values({ | ||
| teamId, | ||
| wins, | ||
| challenges, | ||
| startDateTime: startDate, | ||
| endDateTime: endDate | ||
| }) | ||
| .execute() | ||
|
|
||
| return response | ||
| } | ||
|
|
There was a problem hiding this comment.
Use Number.isNaN instead of isNaN.
isNaN is unsafe as it attempts type coercion. Use Number.isNaN for better type safety.
- if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) {
+ if (Number.isNaN(startDate.getTime()) || Number.isNaN(endDate.getTime())) {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const generateInsight: MutationResolvers['generateInsight'] = async ( | |
| _source, | |
| {teamId, startDate, endDate, useSummaries = true, prompt}, | |
| {dataLoader} | |
| ) => { | |
| if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) { | |
| return standardError( | |
| new Error('Invalid date format. Please use ISO 8601 format (e.g., 2024-01-01T00:00:00Z).') | |
| ) | |
| } | |
| const oneWeekInMs = 7 * 24 * 60 * 60 * 1000 | |
| if (endDate.getTime() - startDate.getTime() < oneWeekInMs) { | |
| return standardError(new Error('The end date must be at least one week after the start date.')) | |
| } | |
| const response = useSummaries | |
| ? await getSummaries(teamId, startDate, endDate, prompt as string) | |
| : await getTopics(teamId, startDate, endDate, dataLoader, prompt as string) | |
| if ('error' in response) { | |
| return response | |
| } | |
| const {wins, challenges} = response | |
| const pg = getKysely() | |
| await pg | |
| .insertInto('Insight') | |
| .values({ | |
| teamId, | |
| wins, | |
| challenges, | |
| startDateTime: startDate, | |
| endDateTime: endDate | |
| }) | |
| .execute() | |
| return response | |
| } | |
| const generateInsight: MutationResolvers['generateInsight'] = async ( | |
| _source, | |
| {teamId, startDate, endDate, useSummaries = true, prompt}, | |
| {dataLoader} | |
| ) => { | |
| if (Number.isNaN(startDate.getTime()) || Number.isNaN(endDate.getTime())) { | |
| return standardError( | |
| new Error('Invalid date format. Please use ISO 8601 format (e.g., 2024-01-01T00:00:00Z).') | |
| ) | |
| } | |
| const oneWeekInMs = 7 * 24 * 60 * 60 * 1000 | |
| if (endDate.getTime() - startDate.getTime() < oneWeekInMs) { | |
| return standardError(new Error('The end date must be at least one week after the start date.')) | |
| } | |
| const response = useSummaries | |
| ? await getSummaries(teamId, startDate, endDate, prompt as string) | |
| : await getTopics(teamId, startDate, endDate, dataLoader, prompt as string) | |
| if ('error' in response) { | |
| return response | |
| } | |
| const {wins, challenges} = response | |
| const pg = getKysely() | |
| await pg | |
| .insertInto('Insight') | |
| .values({ | |
| teamId, | |
| wins, | |
| challenges, | |
| startDateTime: startDate, | |
| endDateTime: endDate | |
| }) | |
| .execute() | |
| return response | |
| } |
Tools
Biome
[error] 12-12: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 12-12: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
This was referenced Jul 26, 2024
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
packages/server/graphql/public/mutations/helpers/getTopics.ts (1)
97-104: Improve readability inprocessSectionThe function is straightforward but can be improved for readability by using descriptive variable names.
- return section - .flatMap((item) => { - const lines = item.split('\n') - return processLines(lines) - }) - .filter((processedItem) => processedItem.trim() !== '') + return section + .flatMap((paragraph) => processLines(paragraph.split('\n'))) + .filter((processedLine) => processedLine.trim() !== '')
Comment on lines
+106
to
+229
| const meetings = await Promise.all( | ||
| rawMeetings.map(async (meeting) => { | ||
| const { | ||
| id: meetingId, | ||
| disableAnonymity, | ||
| name: meetingName, | ||
| createdAt: meetingDate | ||
| } = meeting as MeetingRetrospective | ||
| const rawReflectionGroups = await dataLoader | ||
| .get('retroReflectionGroupsByMeetingId') | ||
| .load(meetingId) | ||
| const reflectionGroups = Promise.all( | ||
| rawReflectionGroups | ||
| .filter((g) => g.voterIds.length > 0) | ||
| .map(async (group) => { | ||
| const {id: reflectionGroupId, voterIds, title} = group | ||
| const [comments, rawReflections] = await Promise.all([ | ||
| getComments(reflectionGroupId, dataLoader), | ||
| dataLoader.get('retroReflectionsByGroupId').load(group.id) | ||
| ]) | ||
| const reflections = await Promise.all( | ||
| rawReflections.map(async (reflection) => { | ||
| const {promptId, creatorId, plaintextContent} = reflection | ||
| const [prompt, creator] = await Promise.all([ | ||
| dataLoader.get('reflectPrompts').load(promptId), | ||
| creatorId ? dataLoader.get('users').loadNonNull(creatorId) : null | ||
| ]) | ||
| const {question} = prompt | ||
| const creatorName = | ||
| disableAnonymity && creator ? creator.preferredName : 'Anonymous' | ||
| return { | ||
| prompt: question, | ||
| author: creatorName, | ||
| text: plaintextContent | ||
| } | ||
| }) | ||
| ) | ||
| const res = { | ||
| voteCount: voterIds.length, | ||
| title: title, | ||
| comments, | ||
| reflections, | ||
| meetingName, | ||
| date: meetingDate, | ||
| meetingId | ||
| } | ||
|
|
||
| if (!res.comments || !res.comments.length) { | ||
| delete (res as any).comments | ||
| } | ||
| return res | ||
| }) | ||
| ) | ||
| return reflectionGroups | ||
| }) | ||
| ) | ||
|
|
||
| const hotTopics = meetings | ||
| .flat() | ||
| .filter((t) => t.voteCount > 2) | ||
| .sort((a, b) => (a.voteCount > b.voteCount ? -1 : 1)) | ||
|
|
||
| const idGenerator = { | ||
| meeting: 1 | ||
| } | ||
|
|
||
| const shortTokenedTopics = hotTopics | ||
| .map((t) => { | ||
| const {date, meetingId} = t | ||
| const shortMeetingId = `m${idGenerator.meeting++}` | ||
| const shortMeetingDate = new Date(date).toISOString().split('T')[0] | ||
| meetingLookup[shortMeetingId] = meetingId | ||
| return { | ||
| ...t, | ||
| date: shortMeetingDate, | ||
| meetingId: shortMeetingId | ||
| } | ||
| }) | ||
| .filter((t) => t) | ||
|
|
||
| if (shortTokenedTopics.length === 0) { | ||
| return standardError(new Error('No meeting content found for the specified date range.')) | ||
| } | ||
|
|
||
| const yamlData = yaml.dump(shortTokenedTopics, { | ||
| noCompatMode: true | ||
| }) | ||
|
|
||
| const openAI = new OpenAIServerManager() | ||
| const rawInsight = await openAI.generateInsight(yamlData, false, prompt) | ||
| if (!rawInsight) { | ||
| return standardError(new Error('Unable to generate insight.')) | ||
| } | ||
|
|
||
| const wins = processSection(rawInsight.wins) | ||
| const challenges = processSection(rawInsight.challenges) | ||
| const meetingIds = rawMeetings.map((meeting) => meeting.id) | ||
|
|
||
| return {wins, challenges, meetingIds} | ||
| } |
There was a problem hiding this comment.
Avoid using the delete operator
The delete operator can impact performance. Replace it with an assignment to undefined.
- delete (res as any).comments
+ (res as any).comments = undefinedCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const getTopics = async ( | |
| teamId: string, | |
| startDate: Date, | |
| endDate: Date, | |
| dataLoader: DataLoaderWorker, | |
| prompt?: string | null | |
| ) => { | |
| const r = await getRethink() | |
| const MIN_REFLECTION_COUNT = 3 | |
| const MIN_MILLISECONDS = 60 * 1000 // 1 minute | |
| const rawMeetings = await r | |
| .table('NewMeeting') | |
| .getAll(teamId, {index: 'teamId'}) | |
| .filter((row: any) => | |
| row('meetingType') | |
| .eq('retrospective') | |
| .and(row('createdAt').ge(startDate)) | |
| .and(row('createdAt').le(endDate)) | |
| .and(row('reflectionCount').gt(MIN_REFLECTION_COUNT)) | |
| .and(r.table('MeetingMember').getAll(row('id'), {index: 'meetingId'}).count().gt(1)) | |
| .and(row('endedAt').sub(row('createdAt')).gt(MIN_MILLISECONDS)) | |
| ) | |
| .run() | |
| const meetings = await Promise.all( | |
| rawMeetings.map(async (meeting) => { | |
| const { | |
| id: meetingId, | |
| disableAnonymity, | |
| name: meetingName, | |
| createdAt: meetingDate | |
| } = meeting as MeetingRetrospective | |
| const rawReflectionGroups = await dataLoader | |
| .get('retroReflectionGroupsByMeetingId') | |
| .load(meetingId) | |
| const reflectionGroups = Promise.all( | |
| rawReflectionGroups | |
| .filter((g) => g.voterIds.length > 0) | |
| .map(async (group) => { | |
| const {id: reflectionGroupId, voterIds, title} = group | |
| const [comments, rawReflections] = await Promise.all([ | |
| getComments(reflectionGroupId, dataLoader), | |
| dataLoader.get('retroReflectionsByGroupId').load(group.id) | |
| ]) | |
| const reflections = await Promise.all( | |
| rawReflections.map(async (reflection) => { | |
| const {promptId, creatorId, plaintextContent} = reflection | |
| const [prompt, creator] = await Promise.all([ | |
| dataLoader.get('reflectPrompts').load(promptId), | |
| creatorId ? dataLoader.get('users').loadNonNull(creatorId) : null | |
| ]) | |
| const {question} = prompt | |
| const creatorName = | |
| disableAnonymity && creator ? creator.preferredName : 'Anonymous' | |
| return { | |
| prompt: question, | |
| author: creatorName, | |
| text: plaintextContent | |
| } | |
| }) | |
| ) | |
| const res = { | |
| voteCount: voterIds.length, | |
| title: title, | |
| comments, | |
| reflections, | |
| meetingName, | |
| date: meetingDate, | |
| meetingId | |
| } | |
| if (!res.comments || !res.comments.length) { | |
| delete (res as any).comments | |
| } | |
| return res | |
| }) | |
| ) | |
| return reflectionGroups | |
| }) | |
| ) | |
| const hotTopics = meetings | |
| .flat() | |
| .filter((t) => t.voteCount > 2) | |
| .sort((a, b) => (a.voteCount > b.voteCount ? -1 : 1)) | |
| const idGenerator = { | |
| meeting: 1 | |
| } | |
| const shortTokenedTopics = hotTopics | |
| .map((t) => { | |
| const {date, meetingId} = t | |
| const shortMeetingId = `m${idGenerator.meeting++}` | |
| const shortMeetingDate = new Date(date).toISOString().split('T')[0] | |
| meetingLookup[shortMeetingId] = meetingId | |
| return { | |
| ...t, | |
| date: shortMeetingDate, | |
| meetingId: shortMeetingId | |
| } | |
| }) | |
| .filter((t) => t) | |
| if (shortTokenedTopics.length === 0) { | |
| return standardError(new Error('No meeting content found for the specified date range.')) | |
| } | |
| const yamlData = yaml.dump(shortTokenedTopics, { | |
| noCompatMode: true | |
| }) | |
| const openAI = new OpenAIServerManager() | |
| const rawInsight = await openAI.generateInsight(yamlData, false, prompt) | |
| if (!rawInsight) { | |
| return standardError(new Error('Unable to generate insight.')) | |
| } | |
| const wins = processSection(rawInsight.wins) | |
| const challenges = processSection(rawInsight.challenges) | |
| const meetingIds = rawMeetings.map((meeting) => meeting.id) | |
| return {wins, challenges, meetingIds} | |
| } | |
| if (!res.comments || !res.comments.length) { | |
| (res as any).comments = undefined | |
| } |
Tools
Biome
[error] 178-178: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Comment on lines
+66
to
+95
| const processLines = (lines: string[]): string[] => { | ||
| const meetingURL = 'https://action.parabol.co/meet/' | ||
| return lines | ||
| .map((line) => { | ||
| if (line.includes(meetingURL)) { | ||
| let processedLine = line | ||
| const regex = new RegExp(`${meetingURL}\\S+`, 'g') | ||
| const matches = processedLine.match(regex) || [] | ||
|
|
||
| let isValid = true | ||
| matches.forEach((match) => { | ||
| const shortMeetingId = match.split(meetingURL)[1]?.split(/[),\s]/)[0] // Split by closing parenthesis, comma, or space | ||
| const actualMeetingId = shortMeetingId && (meetingLookup[shortMeetingId] as string) | ||
|
|
||
| if (shortMeetingId && actualMeetingId) { | ||
| processedLine = processedLine.replace(shortMeetingId, actualMeetingId) | ||
| } else { | ||
| const error = new Error( | ||
| `AI hallucinated. Unable to find meetingId for ${shortMeetingId}. Line: ${line}` | ||
| ) | ||
| sendToSentry(error) | ||
| isValid = false | ||
| } | ||
| }) | ||
| return isValid ? processedLine : '' | ||
| } | ||
| return line | ||
| }) | ||
| .filter((line) => line.trim() !== '') | ||
| } |
There was a problem hiding this comment.
Improve error handling in processLines
The function is well-structured but can be improved by handling errors more gracefully.
- matches.forEach((match) => {
- const shortMeetingId = match.split(meetingURL)[1]?.split(/[),\s]/)[0] // Split by closing parenthesis, comma, or space
- const actualMeetingId = shortMeetingId && (meetingLookup[shortMeetingId] as string)
- if (shortMeetingId && actualMeetingId) {
- processedLine = processedLine.replace(shortMeetingId, actualMeetingId)
- } else {
- const error = new Error(
- `AI hallucinated. Unable to find meetingId for ${shortMeetingId}. Line: ${line}`
- )
- sendToSentry(error)
- isValid = false
- }
- })
+ for (const match of matches) {
+ const shortMeetingId = match.split(meetingURL)[1]?.split(/[),\s]/)[0]
+ const actualMeetingId = shortMeetingId && (meetingLookup[shortMeetingId] as string)
+ if (shortMeetingId && actualMeetingId) {
+ processedLine = processedLine.replace(shortMeetingId, actualMeetingId)
+ } else {
+ sendToSentry(new Error(
+ `AI hallucinated. Unable to find meetingId for ${shortMeetingId}. Line: ${line}`
+ ))
+ isValid = false
+ }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const processLines = (lines: string[]): string[] => { | |
| const meetingURL = 'https://action.parabol.co/meet/' | |
| return lines | |
| .map((line) => { | |
| if (line.includes(meetingURL)) { | |
| let processedLine = line | |
| const regex = new RegExp(`${meetingURL}\\S+`, 'g') | |
| const matches = processedLine.match(regex) || [] | |
| let isValid = true | |
| matches.forEach((match) => { | |
| const shortMeetingId = match.split(meetingURL)[1]?.split(/[),\s]/)[0] // Split by closing parenthesis, comma, or space | |
| const actualMeetingId = shortMeetingId && (meetingLookup[shortMeetingId] as string) | |
| if (shortMeetingId && actualMeetingId) { | |
| processedLine = processedLine.replace(shortMeetingId, actualMeetingId) | |
| } else { | |
| const error = new Error( | |
| `AI hallucinated. Unable to find meetingId for ${shortMeetingId}. Line: ${line}` | |
| ) | |
| sendToSentry(error) | |
| isValid = false | |
| } | |
| }) | |
| return isValid ? processedLine : '' | |
| } | |
| return line | |
| }) | |
| .filter((line) => line.trim() !== '') | |
| } | |
| const processLines = (lines: string[]): string[] => { | |
| const meetingURL = 'https://action.parabol.co/meet/' | |
| return lines | |
| .map((line) => { | |
| if (line.includes(meetingURL)) { | |
| let processedLine = line | |
| const regex = new RegExp(`${meetingURL}\\S+`, 'g') | |
| const matches = processedLine.match(regex) || [] | |
| let isValid = true | |
| for (const match of matches) { | |
| const shortMeetingId = match.split(meetingURL)[1]?.split(/[),\s]/)[0] // Split by closing parenthesis, comma, or space | |
| const actualMeetingId = shortMeetingId && (meetingLookup[shortMeetingId] as string) | |
| if (shortMeetingId && actualMeetingId) { | |
| processedLine = processedLine.replace(shortMeetingId, actualMeetingId) | |
| } else { | |
| sendToSentry(new Error( | |
| `AI hallucinated. Unable to find meetingId for ${shortMeetingId}. Line: ${line}` | |
| )) | |
| isValid = false | |
| } | |
| } | |
| return isValid ? processedLine : '' | |
| } | |
| return line | |
| }) | |
| .filter((line) => line.trim() !== '') | |
| } |
Comment on lines
+10
to
+61
| const getComments = async (reflectionGroupId: string, dataLoader: DataLoaderWorker) => { | ||
| const pg = getKysely() | ||
| const IGNORE_COMMENT_USER_IDS = ['parabolAIUser'] | ||
| const discussion = await pg | ||
| .selectFrom('Discussion') | ||
| .selectAll() | ||
| .where('discussionTopicId', '=', reflectionGroupId) | ||
| .limit(1) | ||
| .executeTakeFirst() | ||
| if (!discussion) return null | ||
| const {id: discussionId} = discussion | ||
| const rawComments = await dataLoader.get('commentsByDiscussionId').load(discussionId) | ||
| const humanComments = rawComments.filter((c) => !IGNORE_COMMENT_USER_IDS.includes(c.createdBy)) | ||
| const rootComments = humanComments.filter((c) => !c.threadParentId) | ||
| rootComments.sort((a, b) => { | ||
| return a.createdAt.getTime() < b.createdAt.getTime() ? -1 : 1 | ||
| }) | ||
| const comments = await Promise.all( | ||
| rootComments.map(async (comment) => { | ||
| const {createdBy, isAnonymous, plaintextContent} = comment | ||
| const creator = await dataLoader.get('users').loadNonNull(createdBy) | ||
| const commentAuthor = isAnonymous ? 'Anonymous' : creator.preferredName | ||
| const commentReplies = await Promise.all( | ||
| humanComments | ||
| .filter((c) => c.threadParentId === comment.id) | ||
| .sort((a, b) => { | ||
| return a.createdAt.getTime() < b.createdAt.getTime() ? -1 : 1 | ||
| }) | ||
| .map(async (reply) => { | ||
| const {createdBy, isAnonymous, plaintextContent} = reply | ||
| const creator = await dataLoader.get('users').loadNonNull(createdBy) | ||
| const replyAuthor = isAnonymous ? 'Anonymous' : creator.preferredName | ||
| return { | ||
| text: plaintextContent, | ||
| author: replyAuthor | ||
| } | ||
| }) | ||
| ) | ||
| return commentReplies.length === 0 | ||
| ? { | ||
| text: plaintextContent, | ||
| author: commentAuthor | ||
| } | ||
| : { | ||
| text: plaintextContent, | ||
| author: commentAuthor, | ||
| replies: commentReplies | ||
| } | ||
| }) | ||
| ) | ||
| return comments | ||
| } |
There was a problem hiding this comment.
Optimise comment retrieval
The function is well-structured but can be optimised by reducing redundant data loading and improving readability.
- const rootComments = humanComments.filter((c) => !c.threadParentId)
- rootComments.sort((a, b) => {
- return a.createdAt.getTime() < b.createdAt.getTime() ? -1 : 1
- })
+ const rootComments = humanComments
+ .filter((c) => !c.threadParentId)
+ .sort((a, b) => a.createdAt.getTime() - b.createdAt.getTime())
- const commentReplies = await Promise.all(
- humanComments
- .filter((c) => c.threadParentId === comment.id)
- .sort((a, b) => {
- return a.createdAt.getTime() < b.createdAt.getTime() ? -1 : 1
- })
- .map(async (reply) => {
- const {createdBy, isAnonymous, plaintextContent} = reply
- const creator = await dataLoader.get('users').loadNonNull(createdBy)
- const replyAuthor = isAnonymous ? 'Anonymous' : creator.preferredName
- return {
- text: plaintextContent,
- author: replyAuthor
- }
- })
- )
+ const commentReplies = await Promise.all(
+ humanComments
+ .filter((c) => c.threadParentId === comment.id)
+ .sort((a, b) => a.createdAt.getTime() - b.createdAt.getTime())
+ .map(async (reply) => {
+ const {createdBy, isAnonymous, plaintextContent} = reply
+ const creator = await dataLoader.get('users').loadNonNull(createdBy)
+ const replyAuthor = isAnonymous ? 'Anonymous' : creator.preferredName
+ return {
+ text: plaintextContent,
+ author: replyAuthor
+ }
+ })
+ )Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const getComments = async (reflectionGroupId: string, dataLoader: DataLoaderWorker) => { | |
| const pg = getKysely() | |
| const IGNORE_COMMENT_USER_IDS = ['parabolAIUser'] | |
| const discussion = await pg | |
| .selectFrom('Discussion') | |
| .selectAll() | |
| .where('discussionTopicId', '=', reflectionGroupId) | |
| .limit(1) | |
| .executeTakeFirst() | |
| if (!discussion) return null | |
| const {id: discussionId} = discussion | |
| const rawComments = await dataLoader.get('commentsByDiscussionId').load(discussionId) | |
| const humanComments = rawComments.filter((c) => !IGNORE_COMMENT_USER_IDS.includes(c.createdBy)) | |
| const rootComments = humanComments.filter((c) => !c.threadParentId) | |
| rootComments.sort((a, b) => { | |
| return a.createdAt.getTime() < b.createdAt.getTime() ? -1 : 1 | |
| }) | |
| const comments = await Promise.all( | |
| rootComments.map(async (comment) => { | |
| const {createdBy, isAnonymous, plaintextContent} = comment | |
| const creator = await dataLoader.get('users').loadNonNull(createdBy) | |
| const commentAuthor = isAnonymous ? 'Anonymous' : creator.preferredName | |
| const commentReplies = await Promise.all( | |
| humanComments | |
| .filter((c) => c.threadParentId === comment.id) | |
| .sort((a, b) => { | |
| return a.createdAt.getTime() < b.createdAt.getTime() ? -1 : 1 | |
| }) | |
| .map(async (reply) => { | |
| const {createdBy, isAnonymous, plaintextContent} = reply | |
| const creator = await dataLoader.get('users').loadNonNull(createdBy) | |
| const replyAuthor = isAnonymous ? 'Anonymous' : creator.preferredName | |
| return { | |
| text: plaintextContent, | |
| author: replyAuthor | |
| } | |
| }) | |
| ) | |
| return commentReplies.length === 0 | |
| ? { | |
| text: plaintextContent, | |
| author: commentAuthor | |
| } | |
| : { | |
| text: plaintextContent, | |
| author: commentAuthor, | |
| replies: commentReplies | |
| } | |
| }) | |
| ) | |
| return comments | |
| } | |
| const getComments = async (reflectionGroupId: string, dataLoader: DataLoaderWorker) => { | |
| const pg = getKysely() | |
| const IGNORE_COMMENT_USER_IDS = ['parabolAIUser'] | |
| const discussion = await pg | |
| .selectFrom('Discussion') | |
| .selectAll() | |
| .where('discussionTopicId', '=', reflectionGroupId) | |
| .limit(1) | |
| .executeTakeFirst() | |
| if (!discussion) return null | |
| const {id: discussionId} = discussion | |
| const rawComments = await dataLoader.get('commentsByDiscussionId').load(discussionId) | |
| const humanComments = rawComments.filter((c) => !IGNORE_COMMENT_USER_IDS.includes(c.createdBy)) | |
| const rootComments = humanComments | |
| .filter((c) => !c.threadParentId) | |
| .sort((a, b) => a.createdAt.getTime() - b.createdAt.getTime()) | |
| const comments = await Promise.all( | |
| rootComments.map(async (comment) => { | |
| const {createdBy, isAnonymous, plaintextContent} = comment | |
| const creator = await dataLoader.get('users').loadNonNull(createdBy) | |
| const commentAuthor = isAnonymous ? 'Anonymous' : creator.preferredName | |
| const commentReplies = await Promise.all( | |
| humanComments | |
| .filter((c) => c.threadParentId === comment.id) | |
| .sort((a, b) => a.createdAt.getTime() - b.createdAt.getTime()) | |
| .map(async (reply) => { | |
| const {createdBy, isAnonymous, plaintextContent} = reply | |
| const creator = await dataLoader.get('users').loadNonNull(createdBy) | |
| const replyAuthor = isAnonymous ? 'Anonymous' : creator.preferredName | |
| return { | |
| text: plaintextContent, | |
| author: replyAuthor | |
| } | |
| }) | |
| ) | |
| return commentReplies.length === 0 | |
| ? { | |
| text: plaintextContent, | |
| author: commentAuthor | |
| } | |
| : { | |
| text: plaintextContent, | |
| author: commentAuthor, | |
| replies: commentReplies | |
| } | |
| }) | |
| ) | |
| return comments | |
| } |
24 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
View more in depth update in Slack with example output here.
Fix: #10037
This PR generates an insight for a team. I'm comparing two approaches that you can test by toggling the
useSummariesargument in the mutation:The second option is significantly faster: one insight took 32 seconds with the first approach and 5 seconds with the second (see here).
There has been some feedback that the quality of the insight isn't quite as good with the second approach - see Drew & Jordan's comments here.
I prefer the second approach. The speed means that users can generate Insights for the time periods they want (e.g. since I became the boss). I think the quality is similar, and a better prompt will satisfy the concerns.
Rather than just me playing with the prompt, I'd like to open it up to anyone at Parabol, so I've added
promptas an optional argument.I've added a Notion doc here as a guide to generate insights.
Next Steps
summarys for each retro meeting for a team. We need to do this as the old summaries don't include links or quotes.To test
cloud-sql-proxy --address 0.0.0.0 --port 5433 parabol-saas-staging:us-central1:saas-staging-postgresql-clone-for-upgradegenerateInsightmutation, e.g.generateInsight(teamId: [teamId], startDate: "2024-01-01T00:00:00.000Z", endDate: "2024-04-01T00:00:00.000Z", useSummaries: true) {Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores