Conversation
Summary of ChangesHello, 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 significant new feature that allows users to save their progress on surveys as drafts. This enhances the user experience by providing flexibility and preventing data loss for long or complex surveys. The changes span across the database, backend API, and frontend application, establishing a complete system for managing survey response drafts from creation to submission or deletion. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
![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 the functionality for survey response drafts, a significant feature addition. The implementation spans the backend and frontend, including database changes, new API endpoints, and UI components for managing drafts. The code is generally well-structured. My feedback focuses on improving database performance by addressing N+1 query patterns, removing leftover debugging code, and enhancing data type consistency in the database schema.
| const enrichedDrafts = await Promise.all( | ||
| drafts.map(async draft => { | ||
| const survey = draft.survey_id | ||
| ? await models.survey.findById(draft.survey_id as string) | ||
| : null; | ||
|
|
||
| const entity = draft.entity_id | ||
| ? await models.entity.findById(draft.entity_id as string) | ||
| : null; | ||
|
|
||
| return { | ||
| id: draft.id, | ||
| surveyId: draft.survey_id, | ||
| surveyCode: survey?.code ?? null, | ||
| surveyName: survey?.name ?? null, | ||
| countryCode: (draft.country_code as string) ?? entity?.country_code ?? null, | ||
| entityId: draft.entity_id ?? null, | ||
| entityName: entity?.name ?? null, | ||
| startTime: draft.start_time ?? null, | ||
| formData: draft.form_data, | ||
| screenNumber: draft.screen_number, | ||
| updatedAt: draft.updated_at, | ||
| }; | ||
| }), | ||
| ); |
There was a problem hiding this comment.
The current implementation fetches details for each draft individually within a Promise.all, which can lead to an N+1 query problem and cause performance issues if a user has many drafts. To optimize this, you can collect all survey_ids and entity_ids and fetch them in two batch queries. This will significantly reduce the number of database calls.
const surveyIds = drafts.map(draft => draft.survey_id).filter(Boolean);
const entityIds = drafts.map(draft => draft.entity_id).filter(Boolean);
const [surveys, entities] = await Promise.all([
models.survey.findManyById(surveyIds as string[]),
models.entity.findManyById(entityIds as string[]),
]);
const surveysById = new Map(surveys.map(s => [s.id, s]));
const entitiesById = new Map(entities.map(e => [e.id, e]));
const enrichedDrafts = drafts.map(draft => {
const survey = draft.survey_id ? surveysById.get(draft.survey_id as string) : null;
const entity = draft.entity_id ? entitiesById.get(draft.entity_id as string) : null;
return {
id: draft.id,
surveyId: draft.survey_id,
surveyCode: survey?.code ?? null,
surveyName: survey?.name ?? null,
countryCode: (draft.country_code as string) ?? entity?.country_code ?? null,
entityId: draft.entity_id ?? null,
entityName: entity?.name ?? null,
startTime: draft.start_time ?? null,
formData: draft.form_data,
screenNumber: draft.screen_number,
updatedAt: draft.updated_at,
};
});| return Promise.all( | ||
| drafts.map(async draft => { | ||
| const survey = draft.survey_id ? await models.survey.findById(draft.survey_id) : null; | ||
| const entity = draft.entity_id ? await models.entity.findById(draft.entity_id) : null; | ||
|
|
||
| return { | ||
| id: draft.id, | ||
| surveyId: draft.survey_id, | ||
| surveyCode: survey?.code ?? null, | ||
| surveyName: survey?.name ?? null, | ||
| countryCode: draft.country_code ?? entity?.country_code ?? null, | ||
| entityId: draft.entity_id ?? null, | ||
| entityName: entity?.name ?? null, | ||
| startTime: draft.start_time ?? null, | ||
| formData: draft.form_data, | ||
| screenNumber: draft.screen_number, | ||
| updatedAt: draft.updated_at, | ||
| }; | ||
| }), | ||
| ); |
There was a problem hiding this comment.
This logic for fetching local drafts introduces an N+1 query problem by fetching survey and entity details for each draft individually. This can be inefficient, especially in an offline-first scenario on devices with limited resources. It's better to batch these queries by first collecting all necessary IDs and then fetching them in a minimal number of queries.
const surveyIds = drafts.map(draft => draft.survey_id).filter(Boolean);
const entityIds = drafts.map(draft => draft.entity_id).filter(Boolean);
const [surveys, entities] = await Promise.all([
models.survey.findManyById(surveyIds),
models.entity.findManyById(entityIds),
]);
const surveysById = new Map(surveys.map(s => [s.id, s]));
const entitiesById = new Map(entities.map(e => [e.id, e]));
return drafts.map(draft => {
const survey = draft.survey_id ? surveysById.get(draft.survey_id) : null;
const entity = draft.entity_id ? entitiesById.get(draft.entity_id) : null;
return {
id: draft.id,
surveyId: draft.survey_id,
surveyCode: survey?.code ?? null,
surveyName: survey?.name ?? null,
countryCode: draft.country_code ?? entity?.country_code ?? null,
entityId: draft.entity_id ?? null,
entityName: entity?.name ?? null,
startTime: draft.start_time ?? null,
formData: draft.form_data,
screenNumber: draft.screen_number,
updatedAt: draft.updated_at,
};
});| user_id TEXT NOT NULL, | ||
| country_code TEXT, | ||
| entity_id TEXT, | ||
| start_time TEXT, |
There was a problem hiding this comment.
The start_time column is defined as TEXT. For consistency with the updated_at column and for better data integrity, it would be more appropriate to use a TIMESTAMP or TIMESTAMPTZ data type for storing date/time values.
| start_time TEXT, | |
| start_time TIMESTAMP, |
|
Superseded by a stack of smaller PRs for easier review:
|
1 participant
Issue #:
Changes:
Screenshots: