Turn on noUncheckedIndexedAccess once #65 has typed the query results #101

New Issue

2026-08-21T16:02:53-05:00

bermudalamb commented

2026-08-21 16:02:53 -05:00

Both workspaces set strict: true, which is most of the value. The compiler flag that would catch the most remaining real defects here is not in strict, and has to be asked for.

The pattern it catches, which this codebase uses everywhere

rows[0].id — indexing an array and immediately reading a property. TypeScript types rows[0] as the element type regardless of whether the array has any elements, so the compiler is satisfied by code that throws at runtime. noUncheckedIndexedAccess types it as T | undefined instead and demands the check.

The backend does this after a rows.length guard in most places, which is correct and would satisfy the flag with a small change. In a handful it does not guard at all, and those are the ones worth finding — a query that returns nothing where the code assumed a row produces Cannot read properties of undefined, surfacing as a 500 with no useful message.

Why this is sequenced after #65, not before

Right now pool.query() returns any rows, so rows[0].id is already unchecked and the flag has nothing to bite on — it would report little and prove less. #65 stage 3 gives query results real row types. After that, noUncheckedIndexedAccess is what makes those types honest about emptiness.

Enabling it first would be measuring the wrong thing and would produce a violation count that changes completely once #65 lands. Doing it in the other order means each change is verifiable on its own.

What else was checked and is already fine

Worth recording so nobody re-investigates:

No enum anywhere. The TypeScript team steers away from them for exactly the reasons this codebase already avoids — union types of string literals are used throughout instead, as in ItemStatus.
Two non-null assertions in the whole codebase, both defensible: document.getElementById('root')! is the standard React entry idiom, and byEmail.get(row.email)! in server.ts follows a set on the same key.
strict: true on both sides, and no // @ts-ignore or @ts-expect-error anywhere.

The TypeScript hygiene here is good. This one flag is the gap.

Worth deciding

Whether to also take exactOptionalPropertyTypes and noImplicitOverride in the same pass. Both are smaller, and exactOptionalPropertyTypes in particular interacts with how optional props are spread into antd components, so it deserves its own measurement rather than being assumed cheap.

Verification

The violation count, measured before and after, and the full suite green. Any site the flag exposes as a genuinely unguarded index is a bug fix rather than an annotation, and should be called out as one rather than silenced with a !.

Severity

Low today, and only becomes worth doing after #65. Filed now so the sequencing is recorded rather than rediscovered.

Found during a TypeScript best-practices review.

Both workspaces set `strict: true`, which is most of the value. The compiler flag that would catch the most remaining real defects here is not in `strict`, and has to be asked for. ## The pattern it catches, which this codebase uses everywhere `rows[0].id` — indexing an array and immediately reading a property. TypeScript types `rows[0]` as the element type regardless of whether the array has any elements, so the compiler is satisfied by code that throws at runtime. `noUncheckedIndexedAccess` types it as `T | undefined` instead and demands the check. The backend does this after a `rows.length` guard in most places, which is correct and would satisfy the flag with a small change. In a handful it does not guard at all, and those are the ones worth finding — a query that returns nothing where the code assumed a row produces `Cannot read properties of undefined`, surfacing as a 500 with no useful message. ## Why this is sequenced after #65, not before Right now `pool.query()` returns `any` rows, so `rows[0].id` is already unchecked and the flag has nothing to bite on — it would report little and prove less. #65 stage 3 gives query results real row types. **After** that, `noUncheckedIndexedAccess` is what makes those types honest about emptiness. Enabling it first would be measuring the wrong thing and would produce a violation count that changes completely once #65 lands. Doing it in the other order means each change is verifiable on its own. ## What else was checked and is already fine Worth recording so nobody re-investigates: - **No `enum` anywhere.** The TypeScript team steers away from them for exactly the reasons this codebase already avoids — union types of string literals are used throughout instead, as in `ItemStatus`. - **Two non-null assertions in the whole codebase**, both defensible: `document.getElementById('root')!` is the standard React entry idiom, and `byEmail.get(row.email)!` in `server.ts` follows a `set` on the same key. - **`strict: true` on both sides**, and no `// @ts-ignore` or `@ts-expect-error` anywhere. The TypeScript hygiene here is good. This one flag is the gap. ## Worth deciding Whether to also take `exactOptionalPropertyTypes` and `noImplicitOverride` in the same pass. Both are smaller, and `exactOptionalPropertyTypes` in particular interacts with how optional props are spread into antd components, so it deserves its own measurement rather than being assumed cheap. ## Verification The violation count, measured before and after, and the full suite green. Any site the flag exposes as a genuinely unguarded index is a bug fix rather than an annotation, and should be called out as one rather than silenced with a `!`. ## Severity Low today, and only becomes worth doing after #65. Filed now so the sequencing is recorded rather than rediscovered. Found during a TypeScript best-practices review.

bermudalamb added this to the Code Quality and Hardening 2 project 2026-08-21 16:08:39 -05:00

bermudalamb self-assigned this 2026-08-21 16:09:07 -05:00

bermudalamb commented

2026-08-23 09:37:15 -05:00

Still relevant, but the dependency this issue is waiting on has gone stale and needs re-pointing before the work can start.

This issue holds itself back until "#65 stage 3 gives query results real row types". #65 is closed — but #65 turned out to be Import convention and avoidable any, which has no stages and never included a typing programme for query results. What it did deliver in this area was narrow: publicCustomer(c: any) became a CustomerRow, and the PoolClient annotations in cartCheckout.ts.

Measured on main just now, the precondition is not met:

pool.query<T>(...) with a row type: 1
pool.query(...) total: 89
rows[0] reads: 36

So 88 of 89 queries still return any rows, and this issue's own reasoning still applies unchanged — noUncheckedIndexedAccess would have almost nothing to bite on, would report a count that means little, and that count would change completely once the rows are actually typed.

The blocker is real; it is just no longer represented by an open issue. Typing the query results needs an issue of its own, and this one should depend on that instead of on #65. Filing it is the next step here — this issue should stay open and stay parked until then rather than being started.

Nothing else about the analysis has drifted. The enum and non-null-assertion findings recorded above still hold: no enum anywhere, and the same two defensible ! uses.

Still relevant, but the dependency this issue is waiting on has gone stale and needs re-pointing before the work can start. This issue holds itself back until "#65 stage 3 gives query results real row types". #65 is closed — but #65 turned out to be *Import convention and avoidable `any`*, which has no stages and never included a typing programme for query results. What it did deliver in this area was narrow: `publicCustomer(c: any)` became a `CustomerRow`, and the `PoolClient` annotations in `cartCheckout.ts`. Measured on `main` just now, the precondition is not met: - `pool.query<T>(...)` with a row type: **1** - `pool.query(...)` total: **89** - `rows[0]` reads: **36** So 88 of 89 queries still return `any` rows, and this issue's own reasoning still applies unchanged — `noUncheckedIndexedAccess` would have almost nothing to bite on, would report a count that means little, and that count would change completely once the rows are actually typed. The blocker is real; it is just no longer represented by an open issue. Typing the query results needs an issue of its own, and this one should depend on that instead of on #65. Filing it is the next step here — this issue should stay open and stay parked until then rather than being started. Nothing else about the analysis has drifted. The `enum` and non-null-assertion findings recorded above still hold: no `enum` anywhere, and the same two defensible `!` uses.

bermudalamb referenced this issue

2026-08-24 12:19:37 -05:00

Query results are untyped, so `strict: true` stops at the database boundary #159

bermudalamb commented

2026-08-24 12:19:59 -05:00

The missing dependency now exists: #159, Query results are untyped, so strict: true stops at the database boundary.

This issue depends on that one, not on #65. #65 is closed and was Import convention and avoidable any — it had no stages, and the two things it delivered in this area were CustomerRow and the PoolClient annotations in cartCheckout.ts. The "stage 3" this issue waits for was never part of it.

The sequencing argument in the body still holds exactly as written, and #159 records the measurement that backs it: 1 of roughly 184 query sites carries a row type. Until that changes, rows[0] is any, any indexes to any, and noUncheckedIndexedAccess would report a count that means little and that changes completely once the rows are typed.

One correction to this issue's own framing, repeated from an earlier comment because it affects how urgent this looks. The body says "in a handful it does not guard at all". Two candidates were sampled — adminCategories.ts:128 and admin.ts:380 — and both carry a proper if (!rows.length) return 404 a few lines above. The unguarded-index risk is therefore unproven rather than established, and finding out whether any genuinely exist is part of the work here rather than a premise of it.

The missing dependency now exists: **#159**, *Query results are untyped, so `strict: true` stops at the database boundary*. This issue depends on that one, not on #65. #65 is closed and was *Import convention and avoidable `any`* — it had no stages, and the two things it delivered in this area were `CustomerRow` and the `PoolClient` annotations in `cartCheckout.ts`. The "stage 3" this issue waits for was never part of it. The sequencing argument in the body still holds exactly as written, and #159 records the measurement that backs it: 1 of roughly 184 query sites carries a row type. Until that changes, `rows[0]` is `any`, `any` indexes to `any`, and `noUncheckedIndexedAccess` would report a count that means little and that changes completely once the rows are typed. One correction to this issue's own framing, repeated from an earlier comment because it affects how urgent this looks. The body says "in a handful it does not guard at all". Two candidates were sampled — `adminCategories.ts:128` and `admin.ts:380` — and both carry a proper `if (!rows.length) return 404` a few lines above. The unguarded-index risk is therefore unproven rather than established, and finding out whether any genuinely exist is part of the work here rather than a premise of it.

bermudalamb referenced this issue

2026-08-24 12:20:57 -05:00

Query results are untyped, so `strict: true` stops at the database boundary #159

bermudalamb referenced this issue from a commit

2026-08-24 15:25:15 -05:00

refactor: turn on noUncheckedIndexedAccess in both workspaces (#101)

bermudalamb referenced a pull request that will close this issue

2026-08-24 15:28:34 -05:00

refactor: turn on noUncheckedIndexedAccess in both workspaces (#101) #167

bermudalamb closed this issue

2026-08-24 15:30:29 -05:00

bermudalamb referenced this issue from a commit