add getUserRelations (PR #59 continuation) #82
Conversation
Summary of ChangesHello @raybell-md, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature that allows the system to retrieve and filter an authenticated user's personal relations, such as manager, spouse, or assistant. It leverages the Google People API to fetch this data and exposes it as a new tool, providing a flexible way to access relationship information within the application. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
raybell-md
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new getUserRelations tool for the People service, allowing retrieval of a user's relations from the Google People API, with an option to filter by relation type. The changes include the core service logic, registration of the new tool, and a comprehensive set of unit tests. My review focuses on a potential runtime error in the service implementation and suggests a refactoring to improve code structure and maintainability. The tests are well-written and cover the necessary scenarios effectively.
| const relations = res.data.relations || []; | ||
|
|
||
| // If a specific relation type is requested, filter the results | ||
| if (relationType) { | ||
| const filteredRelations = relations.filter( | ||
| (relation) => relation.type?.toLowerCase() === relationType.toLowerCase() | ||
| ); | ||
| logToFile(`[PeopleService] Filtered to ${filteredRelations.length} relations of type: ${relationType}`); | ||
| return { | ||
| content: [{ | ||
| type: "text" as const, | ||
| text: JSON.stringify({ | ||
| relationType, | ||
| relations: filteredRelations | ||
| }) | ||
| }] | ||
| }; | ||
| } | ||
|
|
||
| // Return all relations if no specific type is requested | ||
| logToFile(`[PeopleService] Returning all ${relations.length} relations`); | ||
| return { | ||
| content: [{ | ||
| type: "text" as const, | ||
| text: JSON.stringify({ | ||
| relations | ||
| }) | ||
| }] | ||
| }; |
There was a problem hiding this comment.
There are a couple of improvements we can make in this section.
-
Potential Null Pointer Exception: The API response
res.datacan benullaccording to thegaxioslibrary'sGaxiosResponsetype. The current coderes.data.relationswould throw aTypeErrorin that case. Using optional chaining (res.data?.relations) will prevent this crash and make the code more robust. -
Code Duplication (Not DRY): The two successful return paths (with and without
relationType) construct very similar response objects. We can refactor this to determine the payload first and then wrap it in the finalcontentstructure once. This makes the code more DRY (Don't Repeat Yourself) and easier to maintain.
Here's a suggested change that addresses both points by first preparing the data and then constructing the final return object once.
const relations = res.data?.relations || [];
let responseData;
if (relationType) {
const filteredRelations = relations.filter(
(relation) => relation.type?.toLowerCase() === relationType.toLowerCase()
);
logToFile(`[PeopleService] Filtered to ${filteredRelations.length} relations of type: ${relationType}`);
responseData = {
relationType,
relations: filteredRelations,
};
} else {
logToFile(`[PeopleService] Returning all ${relations.length} relations`);
responseData = {
relations,
};
}
return {
content: [{
type: "text" as const,
text: JSON.stringify(responseData),
}],
};
|
Hi @raybell-md, Thanks for this PR! The I had a thought that this tool would be even more useful if it could retrieve relations for any user (that the authenticated user has access to), rather than being restricted to just the logged-in user ( This would make the tool more flexible and aligned with how other "get by ID" tools often work. Here are the suggested changes: 1. /**
* Gets a user's relations (e.g., manager, spouse, assistant).
* Defaults to the authenticated user if no userId is provided.
* Optionally filters by a specific relation type.
*/
public getUserRelations = async ({ userId, relationType }: { userId?: string, relationType?: string }) => {
const targetUser = userId ? (userId.startsWith('people/') ? userId : `people/${userId}`) : 'people/me';
logToFile(`[PeopleService] Starting getUserRelations for ${targetUser} with relationType=${relationType}`);
try {
const people = await this.getPeopleClient();
const res = await people.people.get({
resourceName: targetUser,
personFields: 'relations',
});
logToFile(`[PeopleService] Finished getUserRelations API call`);
const relations = res.data?.relations || [];
let responseData;
if (relationType) {
const filteredRelations = relations.filter(
(relation) => relation.type?.toLowerCase() === relationType.toLowerCase()
);
logToFile(`[PeopleService] Filtered to ${filteredRelations.length} relations of type: ${relationType}`);
responseData = {
resourceName: targetUser,
relationType,
relations: filteredRelations,
};
} else {
logToFile(`[PeopleService] Returning all ${relations.length} relations`);
responseData = {
resourceName: targetUser,
relations,
};
}
return {
content: [{
type: "text" as const,
text: JSON.stringify(responseData),
}],
};
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error);
logToFile(`[PeopleService] Error during people.getUserRelations: ${errorMessage}`);
return {
content: [{
type: "text" as const,
text: JSON.stringify({ error: errorMessage })
}]
};
}
}2. server.registerTool(
"people.getUserRelations",
{
description: 'Gets a user\'s relations (e.g., manager, spouse, assistant, etc.). Common relation types include: manager, assistant, spouse, partner, relative, mother, father, parent, sibling, child, friend, domesticPartner, referredBy. Defaults to the authenticated user if no userId is provided.',
inputSchema: {
userId: z.string().optional().describe('The ID of the user to get relations for (e.g., "110001608645105799644" or "people/110001608645105799644"). Defaults to the authenticated user if not provided.'),
relationType: z.string().optional().describe('The type of relation to filter by (e.g., "manager", "spouse", "assistant"). If not provided, returns all relations.'),
}
},
peopleService.getUserRelations
);Let me know what you think!' |
This change updates the tests for PeopleService to correctly handle the userId parameter in the getUserRelations method. A new test is added to ensure that the correct `resourceName` is passed to the Google People API when a `userId` is provided. Existing tests for `getUserRelations` are updated to reflect the new response format, which now includes the `resourceName`.
…5036747166406265 feat(people): Update PeopleService tests to handle userId
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new getUserRelations function to the PeopleService, allowing retrieval of a user's relations from the Google People API. The function is exposed as a new tool, people.getUserRelations, and is accompanied by a comprehensive set of unit tests covering various scenarios, including filtering, case-insensitivity, and error handling. The implementation is solid and well-tested. I have one suggestion to refactor a small part of the getUserRelations function to improve code clarity and reduce duplication.
| let responseData; | ||
|
|
||
| if (relationType) { | ||
| const filteredRelations = relations.filter( | ||
| (relation) => relation.type?.toLowerCase() === relationType.toLowerCase() | ||
| ); | ||
| logToFile(`[PeopleService] Filtered to ${filteredRelations.length} relations of type: ${relationType}`); | ||
| responseData = { | ||
| resourceName: targetUser, | ||
| relationType, | ||
| relations: filteredRelations, | ||
| }; | ||
| } else { | ||
| logToFile(`[PeopleService] Returning all ${relations.length} relations`); | ||
| responseData = { | ||
| resourceName: targetUser, | ||
| relations, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The construction of responseData can be made more concise to improve readability and reduce code duplication. You can determine the filteredRelations first, and then construct the responseData object in a single step, conditionally including the relationType property when it's provided.
const filteredRelations = relationType
? relations.filter(
(relation) => relation.type?.toLowerCase() === relationType.toLowerCase()
)
: relations;
if (relationType) {
logToFile(`[PeopleService] Filtered to ${filteredRelations.length} relations of type: ${relationType}`);
} else {
logToFile(`[PeopleService] Returning all ${filteredRelations.length} relations`);
}
const responseData = {
resourceName: targetUser,
...(relationType && { relationType }),
relations: filteredRelations,
};
There was a problem hiding this comment.
It would be good to implement this fix.
|
Overall looks good. It would be good to implement the change that gemini recommends above, otherwise I think this will be ready to go in. |
|
Tested and created #115 for testing notes. Works as expected. Prompt: Tools called:
|
2 participants
Sorry continuation of #59 and related to #56
My fault for working across work and personal accounts.
Thought it was easier to start a new PR.
Appreciate the option of me coding or you. I'm indifferent. I would say don't feel bad about taking over here and getting any of my PRs across the finish line. I'm not sure when/if I have time to work of stuff here