Skip to content

fix: Optimize campaign reach using batched school metrics queries and campaign reach queries#4740

Merged
vinaypanduga merged 2 commits into
mainfrom
main-ng-9
Jul 16, 2026
Merged

fix: Optimize campaign reach using batched school metrics queries and campaign reach queries#4740
vinaypanduga merged 2 commits into
mainfrom
main-ng-9

Conversation

@narayanangourav-sutara

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the campaign reach calculation to batch school ID queries and introduces a new API method, getCampaignParentsInGroupBySchoolIds, to fetch pre-aggregated parent metrics from the database. Feedback on these changes highlights a few critical issues: first, calling the optional API methods getParentWhatsappClassesBySchoolId and getCampaignParentsInGroupBySchoolIds in useCampaignReach.ts without non-null assertions will trigger TypeScript compilation errors; second, the SqliteApi class is missing an implementation for the newly added getCampaignParentsInGroupBySchoolIds method, which will lead to runtime failures.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +32 to +33
const schoolClasses =
await api.getParentWhatsappClassesBySchoolId(selectedSchoolIds);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Since getParentWhatsappClassesBySchoolId is an optional method on the API interface, calling it directly without the non-null assertion operator (!) will cause a TypeScript compilation error under strict null checks. Please restore the ! operator.

Suggested change
const schoolClasses =
await api.getParentWhatsappClassesBySchoolId(selectedSchoolIds);
const schoolClasses =
await api.getParentWhatsappClassesBySchoolId!(selectedSchoolIds);

Comment on lines +40 to +41
const memberCount =
await api.getCampaignParentsInGroupBySchoolIds(selectedSchoolIds);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Since getCampaignParentsInGroupBySchoolIds is an optional method on the API interface, calling it directly without the non-null assertion operator (!) will cause a TypeScript compilation error under strict null checks. Please add the ! operator.

Suggested change
const memberCount =
await api.getCampaignParentsInGroupBySchoolIds(selectedSchoolIds);
const memberCount =
await api.getCampaignParentsInGroupBySchoolIds!(selectedSchoolIds);

}
return await this._serverApi.getParentWhatsappClassesBySchoolId(schoolId);
return await this._serverApi.getParentWhatsappClassesBySchoolId(schoolIds);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The SqliteApi class implements ServiceApi, but it is missing the implementation for the new getCampaignParentsInGroupBySchoolIds method. If SqliteApi is active (e.g., in local or offline environments), calling this method via ApiHandler will throw an error because this.s.getCampaignParentsInGroupBySchoolIds is undefined.

Please implement getCampaignParentsInGroupBySchoolIds in SqliteApi to forward the call to _serverApi (similar to getParentWhatsappClassesBySchoolId).

  }

  async getCampaignParentsInGroupBySchoolIds(
    schoolIds: string[],
  ): Promise<number> {
    if (!this._serverApi || !this._serverApi.getCampaignParentsInGroupBySchoolIds) {
      throw new Error(
        'Campaign parents-in-group metrics lookup is not implemented in Supabase API.',
      );
    }
    return await this._serverApi.getCampaignParentsInGroupBySchoolIds(schoolIds);
  }

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 60f55674c3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +40 to +41
const memberCount =
await api.getCampaignParentsInGroupBySchoolIds(selectedSchoolIds);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve reach metrics in SQLite mode

When the app is initialized with APIMode.SQLITE, api is still an ApiHandler, so the earlier method-existence guard passes because ApiHandler defines getCampaignParentsInGroupBySchoolIds; however SqliteApi does not implement this optional delegate, unlike the existing class/parent-phone WhatsApp delegates, so this call throws Campaign parents-in-group metrics lookup is not implemented.... The previous implementation worked in SQLite mode via the existing delegates, but campaign setup/communication now logs an error and leaves the reach widget empty or stale in that environment; please add the SQLite-to-Supabase delegate or keep a fallback path.

Useful? React with 👍 / 👎.

@vinaypanduga
vinaypanduga merged commit 4616416 into main Jul 16, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants