Builder MP port by mikaalnaik · Pull Request #23 · BuildCanada/TradingPost

mikaalnaik · 2026-06-19T15:44:37Z

No description provided.

- Remove unauthenticated /bills/api/auth/debug route that leaked NEXTAUTH_URL and secret-presence to anonymous callers - Require isAdmin on /bills/api/users POST so allowlisted users can no longer self-expand the allowlist (privilege escalation); add isAdmin field to the User model - Gate DEV_OPEN_ACCESS on an explicit BILLS_DEV_OPEN_ACCESS=true opt-in in addition to non-production NODE_ENV, so open access can never be enabled by accident; share the flag via env.ts and use it for sign-in auto-allow as well Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Clear all 42 ESLint errors flagged on the PR: - Replace no-explicit-any casts with proper types (auth, API routes, services, utils); add image field to the User model so auth no longer needs any-casts (also fixes the field being silently dropped on save) - Escape apostrophes in FAQ copy (react/no-unescaped-entities) - Satisfy React Compiler rules: move cache reset out of the Home component (react-hooks/globals), derive collapsed state from props instead of syncing via effect in FilterSection, and justify the remaining intentional setState-in-effect (BillShare) and Math.random skeleton width (sidebar) with scoped disables Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

mikaalnaik

Code review — Builder MP port

Reviewed at 0b69199. The three critical security issues flagged earlier are now resolved in this branch: the unauthenticated /bills/api/auth/debug leak is gone, /bills/api/users is admin-gated (isAdmin added to the User model), and DEV_OPEN_ACCESS now requires an explicit BILLS_DEV_OPEN_ACCESS=true opt-in so it can't be enabled by a misconfigured NODE_ENV. Lint is clean (0 errors). Nice.

What remains is 🟡 medium / cleanup — none blocking, but worth resolving before merge. Inline comments below.

Operational follow-up (no line to pin)

First-admin bootstrap. isAdmin now defaults to false for everyone, so in production /bills/api/users will 403 for all callers until an admin exists. You'll need a migration or one-off DB update to seed the first admin — otherwise the allowlist can never be managed via the API.
Unused dependencies. @google/genai and @google/generative-ai are both in package.json but have no imports anywhere in src/ (the AI services use OpenAI). Two Google AI SDKs plus OpenAI is a lot of dependency surface — drop them unless they're about to be used.
Test coverage. No tests accompany the auth guard, the admin gate, or the non-trivial body-parsing in api/[id]/route.ts (comma-split / question-grouping). Given the auth sensitivity, at minimum add a test asserting production mode denies non-allowlisted/non-admin users on each mutating route.

Quality (non-blocking)

The three ~70-line fallback tenet blocks in billApi.ts (no-key / parse-fail / error) are near-identical copy-paste — extract a FALLBACK_TENETS constant + makeFallback(reason) helper.
LLM output is parsed with JSON.parse + regex fallbacks; since you're on the OpenAI SDK, prefer a structured-output / response_format schema for robustness.

mikaalnaik · 2026-06-19T19:08:19Z

+  _request: NextRequest,
+  context: { params: Promise<{ email: string }> },
+) {
+  const session = { user: { email: null } };


Dead / broken endpoint. session is hardcoded to { user: { email: null } }, so the guard on the next line always returns 401 — this route can never do anything. It fails closed (safe), but it's misleading dead code.

Either delete the route, or implement it properly with getServerSession(authOptions) and an isAdmin check (granting allowlist access is the same privilege-escalation surface we just locked down on /api/users).

mikaalnaik · 2026-06-19T19:08:19Z

+
+export async function POST(_request: Request) {
+  // if (env.NODE_ENV !== "development") {
+  return NextResponse.json({ error: "Not found" }, { status: 404 });


This endpoint is stubbed to unconditionally return 404 with ~70 lines of commented-out implementation above/below. Please remove the dead code (and the file if the route isn't needed) before merge — commented-out blocks rot quickly and obscure the real behavior.

mikaalnaik · 2026-06-19T19:08:19Z

+export default function DevUsersPage() {
+  return (
+    <div>
+      howdy


Placeholder page renders literally howdy with the real form commented out. Remove it or finish it — it shouldn't ship as-is.

mikaalnaik · 2026-06-19T19:08:19Z

+  billId: string,
+): Promise<ApiBillDetail | null> {
+  const URL = `${env.CIVICS_PROJECT_BASE_URL}/canada/bills/${CANADIAN_PARLIAMENT_NUMBER}/${billId}`;
+  console.log({URL});


Leftover debug logging (console.log({URL})) — this runs on every Civics Project fetch and will spam production logs. Remove it (same for the console.log("Raw response:", responseText) further down, which also logs full AI output / bill text).

mikaalnaik · 2026-06-19T19:08:19Z

+    cached.promise = mongoose.connect(MONGO_URI, {
+      dbName: DATABASE_NAME,
+      serverSelectionTimeoutMS: 3000,
+      socketTimeoutMS: 3000,


socketTimeoutMS: 3000 is aggressive: combined with bufferCommands: false, any single DB operation taking >3s (cold Atlas connection, a slower aggregate, the reprocess path that writes after a long AI call) will have its socket killed mid-flight and fail. Keep serverSelectionTimeoutMS short for fast failover, but consider raising the socket timeout (e.g. 30s+).

xrendan

I think this is broadly fine, but I'd like to over the coming days coonsolidate on a single design system and shared components so that Build Canada has a consistent feel. Builder MP doesn't really feel like the rest of our work, and it'd be nice to ensure that we continue having high quality, consistent design across our properties.

xrendan · 2026-06-23T14:35:33Z

nit: We should use the proper build canada logo here.

xrendan · 2026-06-23T14:35:44Z

We shouldn't have this at all

xrendan · 2026-06-23T14:35:50Z

Where is this used?

xrendan · 2026-06-23T14:36:06Z

Where is this used?

xrendan · 2026-06-23T14:36:16Z

Where is this used?

xrendan · 2026-06-30T19:45:29Z

Would prefer to use the shared components instead of using these, but we can do that seperately.

Are all of these components actively used?

xrendan · 2026-06-30T19:45:56Z

We can kill this

xrendan · 2026-06-30T19:46:30Z

xrendan · 2026-06-30T19:46:43Z

Can we just kill this?

xrendan · 2026-06-30T19:46:51Z

Is this used?

…llback dedup - Delete the unauthenticated/dead allow-route, dev-users stub route, and dev-users placeholder page (all unreachable or no-op) - Delete the unused src/bills/lib/auth/ directory (real auth lives in the sibling auth.ts; allowed-users.ts/config.ts/types.d.ts had no references) - Remove leftover debug console.log calls in billApi.ts (one ran on every Civics Project fetch, the other logged full AI output/bill text) - Dedupe the three near-identical ~70-line fallback tenet blocks in billApi.ts into a FALLBACK_TENET_TITLES constant + makeFallbackTenets() helper - Raise mongoose socketTimeoutMS from 3s to 30s so slower operations don't get their socket killed mid-flight - Add scripts/seed-bills-admin.ts (pnpm seed:bills-admin <email>) so the first admin can be bootstrapped now that isAdmin defaults to false

- Delete bills-specific Footer/Nav/BillTimeline (never imported anywhere; the app uses the top-level Footer instead) and a stray extensionless duplicate of OpenGraph/BillOgCard.tsx - Delete SimpleAnalytics.tsx (never rendered) - Delete 35 unused shadcn ui/ primitives, keeping only the 8 actually used (button, card, badge, checkbox, input, label, select, separator) - Delete 4 unused Next.js boilerplate SVGs (next/globe/file/window) - Clean up dead code left in kept files: BillHeader's unfinished getParliament45Header (commented-out body, unused var), BillQuestions' unused _handleShareClick, an unused catch binding in social-issue-grader - Remove now-unused dependencies: @google/genai, @google/generative-ai (AI services use OpenAI), and 21 @radix-ui/* packages + cmdk, input-otp, react-resizable-panels, vaul that were only used by the deleted ui/ primitives (verified repo-wide, not just within bills)

- sitemap.ts used BASE_PATH || "/bills" for the bills index and bill detail URLs, unlike every other bills file (unauthorized/sign-in/SessionProvider), which correctly fall back to "/". Since BASE_PATH normalizes to "" when root-mounted, the old fallback would silently reintroduce "/bills" prefixes even when explicitly configured to mount at the root. Switched both call sites to the existing buildAbsoluteUrl() helper, which already handles this correctly and was otherwise unused. - getBillsFromCivicsProject() is used only by the sitemap, so its no-store fetch forced a full live re-fetch on every single sitemap request with no caching, unlike every other source in the file. Changed to a 1-hour revalidate. - Wrapped the departments.json/commitments.json tracker-API fetch in the same try/catch-and-fallback pattern the bills section already uses, so a transient outage in that one endpoint can't fail the entire production build/sitemap.

Replace the hand-recreated red badge in builder-mp-seo-image.png with the actual buildcanada-logo-square.png asset already in the repo, keeping the rest of the image (text, layout, colors) unchanged.

mikaalnaik and others added 5 commits June 19, 2026 11:44

initial port of builder mp into trading post

8a23fbd

sitemap

3515dac

font

9b8318f

mikaalnaik self-assigned this Jun 19, 2026

mikaalnaik commented Jun 19, 2026

View reviewed changes

mikaalnaik requested a review from xrendan June 22, 2026 13:33

xrendan reviewed Jun 30, 2026

View reviewed changes

mikaalnaik added 4 commits June 30, 2026 16:39

Use real Build Canada logo asset in the bills SEO image

96afd4e

Replace the hand-recreated red badge in builder-mp-seo-image.png with the actual buildcanada-logo-square.png asset already in the repo, keeping the rest of the image (text, layout, colors) unchanged.

Uh oh!

Conversation

mikaalnaik commented Jun 19, 2026

Uh oh!

mikaalnaik left a comment

Choose a reason for hiding this comment

Code review — Builder MP port

Operational follow-up (no line to pin)

Quality (non-blocking)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrendan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants