[no-ticket] Catch nullable values laundered through TypeScript assertions - #49
Open
bockets wants to merge 1 commit into
Open
[no-ticket] Catch nullable values laundered through TypeScript assertions#49bockets wants to merge 1 commit into
bockets wants to merge 1 commit into
Conversation
Codex/Claude approved biggerpockets PRs #30877/#30879/#30880/#30881/#30883 (a React port of /notifications) that crashed on first load in a review app: TypeError: Cannot read properties of null (reading 'charAt'). The colleague_requests.status and referenceships.status columns are nullable in db/schema.rb, the serializer passed the column straight through, and the TypeScript layer declared status: string and laundered the null through an unchecked `as raw.status as string` assertion, which the component then called .charAt(0) on. The throw hit React Router's error boundary and blanked the whole page. Test factories always set a status, so no spec caught it. Adds prompts/_shared/nullability-boundary-rules.md, wired into both review stages, to check nullability at the schema/serializer source rather than trusting a declared TypeScript type, and to weigh blast radius when a new throw sits inside a subtree covered by an error boundary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BiggiePockets reviewed and approved PRs #30877, #30879, #30880, #30881, and
#30883 in BiggerPockets/biggerpockets (a React port of the /notifications
page). The page then crashed on first load in a review app:
The root cause:
colleague_requests.statusandreferenceships.statusarenullable columns in
db/schema.rb(nonull: false), and real rows carryNULL.
app/serializers/api/v3/notification_serializer.rbpassed the columnstraight through (
status: parent.status).frontend/api/notifications/index.tsdeclared
status: string— notstring | null— and laundered the nullthrough an unchecked assertion,
status: raw.status as string. A componentthen called
status.charAt(0), threw, and the throw hit React Router's errorboundary, blanking the entire page rather than just the one notification
card. Test factories always set a status, so no spec caught it, and the
review missed it too.
This adds
prompts/_shared/nullability-boundary-rules.mdand wires it intoboth review stages (
codex-first-pass.md,claude-synthesize.md,claude-synthesize-thesis-first.md):db/schema.rb) whenit crosses a DB column → serializer → TypeScript API type → component
boundary, rather than trusting a declared type further down the chain.
asassertion that narrows a nullable value to anon-null TypeScript type as a finding unless it's justified against the
real schema/serializer.
boundary (React Router's route boundary, or an explicit
ErrorBoundary)takes down everything the boundary covers, not just the failing element.
Also adds the file to the
prompts/tree listing inREADME.md.