Feat: bypass google account picker on sign in #261

2026-05-18T20:07:06Z

cirex-web commented

2026-05-18 20:07:06 +00:00

(Migrated from github.com)

Also adds report data on both the primary v2/locations endpoint and a new endpoint for fetching the reports of a single location

Also adds report data on both the primary `v2/locations` endpoint and a new endpoint for fetching the reports of a single location

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2026-05-18 20:10:17 +00:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull request overview

The PR's stated goal is to bypass the Google account picker on sign-in by removing the prompt: "select_account" parameter and adding an hd: "andrew.cmu.edu" hosted-domain hint to the OIDC authorization URL. However, it also includes a sizable set of unrelated changes: a new QueryUtils.getReportsAfter query helper, a refactor of the /v2/locations/:locationId/reports endpoint to use it with a 1-day window, a new reportCount field on the locations API response, a new pnpm-workspace.yaml, and pure formatting churn in auth.ts (trailing commas).

Changes:

Replace prompt: "select_account" with hd: "andrew.cmu.edu" in the OIDC auth URL.
Add QueryUtils.getReportsAfter, wire it into the reports endpoint and into getAllLocationsFromDB to expose a new reportCount per location.
Add a pnpm-workspace.yaml workspace config and apply formatting tweaks across auth.ts.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
src/endpoints/auth.ts	Core PR change: swap `prompt: "select_account"` for `hd: "andrew.cmu.edu"`; otherwise formatting-only edits.
src/endpoints/reviews.ts	Refactor `/v2/locations/:locationId/reports` to use `QueryUtils.getReportsAfter` with a 1-day window; leaves several unused imports.
src/db/dbQueryUtils.ts	New `getReportsAfter(start_time, for_location_id?)` helper using `gt` + optional `eq` filter on `reportsTable`.
src/db/getLocations.ts	Fetch reports since `timeSearchCutoff`, aggregate by location, attach `reportCount` to each location object.
pnpm-workspace.yaml	New pnpm workspace file declaring `packages: [.]` and `strictDepBuilds: false`.

Comments suppressed due to low confidence (1)

src/endpoints/reviews.ts:154

The endpoint's response schema (lines 145–154) declares createdAt: t.Date(), but getReportsAfter returns rows from reportsTable directly, which typically yields a JS Date from drizzle but serializes over HTTP as an ISO string or number depending on the schema column type. Please double‑check that the reportsTable.createdAt column type produces a value compatible with t.Date() (Elysia validates outgoing payloads), otherwise responses may fail validation at runtime. Note the other endpoint above uses createdAt: t.Number() for review timestamps — confirm the inconsistency is intentional.

      response: t.Array(
        t.Object({
          id: t.Number(),
          userId: t.Nullable(t.Number()),
          createdAt: t.Date(),
          locationId: t.String(),
          message: t.String()
        })
      )
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview The PR's stated goal is to bypass the Google account picker on sign-in by removing the `prompt: "select_account"` parameter and adding an `hd: "andrew.cmu.edu"` hosted-domain hint to the OIDC authorization URL. However, it also includes a sizable set of unrelated changes: a new `QueryUtils.getReportsAfter` query helper, a refactor of the `/v2/locations/:locationId/reports` endpoint to use it with a 1-day window, a new `reportCount` field on the locations API response, a new `pnpm-workspace.yaml`, and pure formatting churn in `auth.ts` (trailing commas). **Changes:** - Replace `prompt: "select_account"` with `hd: "andrew.cmu.edu"` in the OIDC auth URL. - Add `QueryUtils.getReportsAfter`, wire it into the reports endpoint and into `getAllLocationsFromDB` to expose a new `reportCount` per location. - Add a `pnpm-workspace.yaml` workspace config and apply formatting tweaks across `auth.ts`. ### Reviewed changes Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | src/endpoints/auth.ts | Core PR change: swap `prompt: "select_account"` for `hd: "andrew.cmu.edu"`; otherwise formatting-only edits. | | src/endpoints/reviews.ts | Refactor `/v2/locations/:locationId/reports` to use `QueryUtils.getReportsAfter` with a 1-day window; leaves several unused imports. | | src/db/dbQueryUtils.ts | New `getReportsAfter(start_time, for_location_id?)` helper using `gt` + optional `eq` filter on `reportsTable`. | | src/db/getLocations.ts | Fetch reports since `timeSearchCutoff`, aggregate by location, attach `reportCount` to each location object. | | pnpm-workspace.yaml | New pnpm workspace file declaring `packages: [.]` and `strictDepBuilds: false`. | </details> <details> <summary>Comments suppressed due to low confidence (1)</summary> **src/endpoints/reviews.ts:154** * The endpoint's response schema (lines 145–154) declares `createdAt: t.Date()`, but `getReportsAfter` returns rows from `reportsTable` directly, which typically yields a JS `Date` from drizzle but serializes over HTTP as an ISO string or number depending on the schema column type. Please double‑check that the `reportsTable.createdAt` column type produces a value compatible with `t.Date()` (Elysia validates outgoing payloads), otherwise responses may fail validation at runtime. Note the other endpoint above uses `createdAt: t.Number()` for review timestamps — confirm the inconsistency is intentional. ``` response: t.Array( t.Object({ id: t.Number(), userId: t.Nullable(t.Number()), createdAt: t.Date(), locationId: t.String(), message: t.String() }) ) } ``` </details> --- 💡 <a href="/ScottyLabs/dining-api/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

src/db/dbQueryUtils.ts

					
				@ -1,4 +1,4 @@

				import { avg, count, sql } from "drizzle-orm";

				import { avg, count, gt, sql } from "drizzle-orm";

				import {

getReportsAfter uses and and eq from drizzle-orm, but only gt was added to the import on line 1. This works only because and and eq happen to be imported on line 17 via a second drizzle-orm import statement. For consistency and to avoid two import statements from the same module, consolidate the drizzle-orm imports into a single line (and add gt to that line) instead of maintaining two.

`getReportsAfter` uses `and` and `eq` from drizzle-orm, but only `gt` was added to the import on line 1. This works only because `and` and `eq` happen to be imported on line 17 via a second drizzle-orm import statement. For consistency and to avoid two import statements from the same module, consolidate the drizzle-orm imports into a single line (and add `gt` to that line) instead of maintaining two.

src/db/dbQueryUtils.ts

The new public method uses snake_case parameter names (start_time, for_location_id), which is inconsistent with the rest of the codebase (and the rest of this class) which uses camelCase — e.g. todayAsSQLString, timeSearchCutoffStr, locationId. Please rename to startTime and forLocationId (or simply locationId).

The new public method uses snake_case parameter names (`start_time`, `for_location_id`), which is inconsistent with the rest of the codebase (and the rest of this class) which uses camelCase — e.g. `todayAsSQLString`, `timeSearchCutoffStr`, `locationId`. Please rename to `startTime` and `forLocationId` (or simply `locationId`).

for_location_id is typed as string | undefined, but an empty string "" is also falsy in the ternary for_location_id ? eq(...) : undefined. If a caller ever passes an empty string (e.g. from a route param), the filter will be silently dropped and all reports across all locations will be returned. Prefer an explicit for_location_id !== undefined check.

`for_location_id` is typed as `string | undefined`, but an empty string `""` is also falsy in the ternary `for_location_id ? eq(...) : undefined`. If a caller ever passes an empty string (e.g. from a route param), the filter will be silently dropped and all reports across all locations will be returned. Prefer an explicit `for_location_id !== undefined` check.

Indentation in this method is 6 spaces / inconsistent with the rest of the class which uses 2-space indentation (see getSpecials, getLocationIdToDataMap below). Please reformat to match.

Indentation in this method is 6 spaces / inconsistent with the rest of the class which uses 2-space indentation (see `getSpecials`, `getLocationIdToDataMap` below). Please reformat to match.

The new /v2/locations/:locationId/reports behavior and the new reportCount field added to getAllLocationsFromDB are not covered by any tests in this PR. Given the time-window logic (1‑day cutoff using DateTime.now() vs. a today parameter in getLocations.ts), this is exactly the kind of code that benefits from at least one unit test.

The new `/v2/locations/:locationId/reports` behavior and the new `reportCount` field added to `getAllLocationsFromDB` are not covered by any tests in this PR. Given the time-window logic (1‑day cutoff using `DateTime.now()` vs. a `today` parameter in `getLocations.ts`), this is exactly the kind of code that benefits from at least one unit test.

src/db/getLocations.ts

					
				@ -21,4 +21,4 @@

				  const generalOverrides = await DB.getGeneralOverrides();

				  const { idToPointOverrides, idToWeeklyOverrides } = await DB.getTimeOverrides(

				    timeSearchCutoff.toSQLDate(),

				  );

let reportCounts is never reassigned and should be const, matching the rest of this file which uses const exclusively (e.g. timeSearchCutoff, DB, locationIdToData, reports, finalLocationData). The same applies to let amt inside the reducer — it is mutated only via the subsequent acc[...] assignment, not the variable itself, so it should also be const.

`let reportCounts` is never reassigned and should be `const`, matching the rest of this file which uses `const` exclusively (e.g. `timeSearchCutoff`, `DB`, `locationIdToData`, `reports`, `finalLocationData`). The same applies to `let amt` inside the reducer — it is mutated only via the subsequent `acc[...]` assignment, not the variable itself, so it should also be `const`.

timeSearchCutoff is today.minus({ days: 1 }) — i.e. 1 day before "today". Passing this to getReportsAfter will count every report created in the last day plus everything older than that as well from "today minus 1 day" forward. Given that the comment on line 14 says "1 day's worth of data before today" (intended for time slot lookback), this same cutoff is probably not what you want for reportCount. If the intent is "reports filed in the last 24 hours", pass today.minus({ days: 1 }).toJSDate() explicitly, but be aware that today is a DateTime in some local zone, so the JS Date boundary may not match the report-window semantics shown in reviews.ts (which uses DateTime.now()). Please confirm intended window.

`timeSearchCutoff` is `today.minus({ days: 1 })` — i.e. 1 day before "today". Passing this to `getReportsAfter` will count every report created in the last day _plus_ everything older than that as well from "today minus 1 day" forward. Given that the comment on line 14 says "1 day's worth of data before today" (intended for time slot lookback), this same cutoff is probably not what you want for `reportCount`. If the intent is "reports filed in the last 24 hours", pass `today.minus({ days: 1 }).toJSDate()` explicitly, but be aware that today is a `DateTime` in some local zone, so the JS `Date` boundary may not match the report-window semantics shown in `reviews.ts` (which uses `DateTime.now()`). Please confirm intended window.

src/db/getLocations.ts

					
				@ -49,3 +58,4 @@

				        todaysSoups: specials[id]?.soups ?? [],

				        todaysSpecials: specials[id]?.specials ?? [],

				        reportCount: reportCounts[id] ?? 0,

				      };

Missing trailing semicolons and a trailing comma on the new reportCount: reportCounts[id] ?? 0 property compared to the surrounding object entries (other lines all terminate with a comma). The block also has formatting drift (no semicolons on lines 35, 38). Please run the project's formatter to keep the file consistent.

Missing trailing semicolons and a trailing comma on the new `reportCount: reportCounts[id] ?? 0` property compared to the surrounding object entries (other lines all terminate with a comma). The block also has formatting drift (no semicolons on lines 35, 38). Please run the project's formatter to keep the file consistent.

Adding reportCount to the location object is a public API change to the locations response. Any downstream consumer / TypeScript client / Elysia response schema for this endpoint should be updated to include the new field; otherwise consumers either miss the field or strict response validation may reject the payload. Please verify the corresponding response schema for the locations endpoint is updated.

Adding `reportCount` to the location object is a public API change to the locations response. Any downstream consumer / TypeScript client / Elysia response schema for this endpoint should be updated to include the new field; otherwise consumers either miss the field or strict response validation may reject the payload. Please verify the corresponding response schema for the locations endpoint is updated.

src/endpoints/auth.ts

					
				@ -35,7 +36,7 @@ authEndpoints.get(

				  {

Rather than leaving prompt: "select_account" commented out, the line should be deleted. Dead/commented-out code accumulates and obscures intent — version control already preserves the prior behavior.

Rather than leaving `prompt: "select_account"` commented out, the line should be deleted. Dead/commented-out code accumulates and obscures intent — version control already preserves the prior behavior.

src/endpoints/auth.ts

					
				@ -35,7 +36,7 @@ authEndpoints.get(

				  {

				    query: t.Object({ redirectURL: t.Nullable(t.String()) }),

Setting hd: "andrew.cmu.edu" is a Google-specific authorization parameter that restricts sign-in to users in that hosted domain. The inline comment itself acknowledges uncertainty ("idk if this excludes cmu.edu emails..."). Per Google's documentation, hd restricts to a single domain; users with @cmu.edu (faculty/staff) or alumni addresses will be blocked from logging in. Please verify the actual domain(s) issued by the upstream IdP (login.cmu.edu) before merging, and resolve the uncertainty in the comment rather than shipping it. If the OIDC provider is login.cmu.edu rather than Google directly, hd may also be silently ignored.

Setting `hd: "andrew.cmu.edu"` is a Google-specific authorization parameter that restricts sign-in to users in that hosted domain. The inline comment itself acknowledges uncertainty ("idk if this excludes cmu.edu emails..."). Per Google's documentation, `hd` restricts to a single domain; users with `@cmu.edu` (faculty/staff) or alumni addresses will be blocked from logging in. Please verify the actual domain(s) issued by the upstream IdP (login.cmu.edu) before merging, and resolve the uncertainty in the comment rather than shipping it. If the OIDC provider is login.cmu.edu rather than Google directly, `hd` may also be silently ignored.

src/endpoints/reviews.ts

					
				@ -10,7 +9,8 @@ import {

				  updateTagReview,

				} from "db/reviews";

				import { db } from "db/db";

and and gt are imported here but never used in this file (the previous direct eq(reportsTable.locationId, ...) query has been moved into QueryUtils.getReportsAfter). Additionally, reportsTable (line 13) is no longer referenced anywhere in this file after the refactor. Please drop these unused imports.

`and` and `gt` are imported here but never used in this file (the previous direct `eq(reportsTable.locationId, ...)` query has been moved into `QueryUtils.getReportsAfter`). Additionally, `reportsTable` (line 13) is no longer referenced anywhere in this file after the refactor. Please drop these unused imports.

src/endpoints/reviews.ts

					
				@ -132,4 +132,4 @@

				  )

				  .get(

				    "/v2/locations/:locationId/reports",

				    async ({ params: { locationId } }) => {

The PR title and description indicate this PR is about bypassing the Google account picker on sign-in, but it also contains substantial unrelated changes: a new getReportsAfter query utility, refactoring of the /v2/locations/:locationId/reports endpoint to filter by a 1‑day window, and a new reportCount field added to the locations API response. These should likely be split into a separate PR (or the description updated) so the change set is reviewable and revertable independently.

The PR title and description indicate this PR is about bypassing the Google account picker on sign-in, but it also contains substantial unrelated changes: a new `getReportsAfter` query utility, refactoring of the `/v2/locations/:locationId/reports` endpoint to filter by a 1‑day window, and a new `reportCount` field added to the locations API response. These should likely be split into a separate PR (or the description updated) so the change set is reviewable and revertable independently.

Several stylistic deviations from the surrounding file: let is used for variables that are never reassigned (yesterday, ret) — const is the convention elsewhere in this file. The endpoint also lacks trailing semicolons on lines 138 and 140 and uses an awkward (new QueryUtils(db)).getReportsAfter(...) rather than just calling a helper or constructing it on a separate line. Consider const reports = await new QueryUtils(db).getReportsAfter(yesterday.toJSDate(), locationId); and returning reports directly.

Several stylistic deviations from the surrounding file: `let` is used for variables that are never reassigned (`yesterday`, `ret`) — `const` is the convention elsewhere in this file. The endpoint also lacks trailing semicolons on lines 138 and 140 and uses an awkward `(new QueryUtils(db)).getReportsAfter(...)` rather than just calling a helper or constructing it on a separate line. Consider `const reports = await new QueryUtils(db).getReportsAfter(yesterday.toJSDate(), locationId);` and returning `reports` directly.

github-actions[bot] commented

2026-05-18 20:25:32 +00:00

(Migrated from github.com)

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	82.69%	478 / 578
🔵	Statements	81.9%	498 / 608
🔵	Functions	80%	128 / 160
🔵	Branches	72.42%	260 / 359

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/db/dbQueryUtils.ts	83.72%	88.88%	78.94%	85.36%	167-170, 187-190, 234-245
src/db/getLocations.ts	94.11%	90.9%	87.5%	93.75%	32-33

Generated in workflow #517 for commit 348f5e9 by the Vitest Coverage Report Action

<h2>Coverage Report</h2> <table> <thead> <tr> <th align="center">Status</th> <th align="left">Category</th> <th align="right">Percentage</th> <th align="right">Covered / Total</th> </tr> </thead> <tbody> <tr> <td align="center">🔵</td> <td align="left">Lines</td> <td align="right">82.69%</td> <td align="right">478 / 578</td> </tr> <tr> <td align="center">🔵</td> <td align="left">Statements</td> <td align="right">81.9%</td> <td align="right">498 / 608</td> </tr> <tr> <td align="center">🔵</td> <td align="left">Functions</td> <td align="right">80%</td> <td align="right">128 / 160</td> </tr> <tr> <td align="center">🔵</td> <td align="left">Branches</td> <td align="right">72.42%</td> <td align="right">260 / 359</td> </tr> </tbody> </table><details><summary>File Coverage</summary><table> <thead> <tr> <th align="left">File</th> <th align="right">Stmts</th> <th align="right">Branches</th> <th align="right">Functions</th> <th align="right">Lines</th> <th align="left">Uncovered Lines</th> </tr> </thead> <tbody> <tr> <td align="left" colspan="6"><b>Changed Files</b></td> </tr> <tr> <td align="left"><a href="https://github.com/ScottyLabs/dining-api/blob/348f5e9bfab6687044ec1da22de99ff27d51e771/src/db/dbQueryUtils.ts">src/db/dbQueryUtils.ts</a></td> <td align="right">83.72%</td> <td align="right">88.88%</td> <td align="right">78.94%</td> <td align="right">85.36%</td> <td align="left"><a href="https://github.com/ScottyLabs/dining-api/blob/348f5e9bfab6687044ec1da22de99ff27d51e771/src/db/dbQueryUtils.ts#L167-L170" class="text-red">167-170</a>, <a href="https://github.com/ScottyLabs/dining-api/blob/348f5e9bfab6687044ec1da22de99ff27d51e771/src/db/dbQueryUtils.ts#L187-L190" class="text-red">187-190</a>, <a href="https://github.com/ScottyLabs/dining-api/blob/348f5e9bfab6687044ec1da22de99ff27d51e771/src/db/dbQueryUtils.ts#L234-L245" class="text-red">234-245</a></td> </tr> <tr> <td align="left"><a href="https://github.com/ScottyLabs/dining-api/blob/348f5e9bfab6687044ec1da22de99ff27d51e771/src/db/getLocations.ts">src/db/getLocations.ts</a></td> <td align="right">94.11%</td> <td align="right">90.9%</td> <td align="right">87.5%</td> <td align="right">93.75%</td> <td align="left"><a href="https://github.com/ScottyLabs/dining-api/blob/348f5e9bfab6687044ec1da22de99ff27d51e771/src/db/getLocations.ts#L32-L33" class="text-red">32-33</a></td> </tr> </tbody> </table></details> <em>Generated in workflow <a href=https://github.com/ScottyLabs/dining-api/actions/runs/26058362044>#517</a> for commit <a href="https://github.com/ScottyLabs/dining-api/commit/348f5e9bfab6687044ec1da22de99ff27d51e771">348f5e9</a> by the <a href="https://github.com/davelosert/vitest-coverage-report-action">Vitest Coverage Report Action</a></em>

Sign in to join this conversation.

No reviewers