Plan: finding misplaced nullability in site/src/pages/AgentsPage

Status: brainstorm plus feasibility spike. No repo changes made.

Decisions

• The analysis tool lives outside the repo, in the workspace (/home/coder/nullflow/). It is never committed.
• Scope is site/src/pages/AgentsPage only. Slots are only reported if declared there. Callers anywhere in site/src still count as inflows, so a slot used from outside AgentsPage isn't narrowed by mistake.
• Test and story call sites don't justify nullability. If only tests pass undefined or null, the slot is dead: narrow it and update the tests.
• This is a one-off cleanup: no CI ratchet, no new lint rule, no guideline doc.

1. Problem framing

A null | undefined value goes through three roles:

A check at the origin is healthy. The smells are about carriers:

1. Dead nullability. A slot is declared nullable, but nothing nullable ever flows into it. Its guard is dead code.
2. Long carry distance. A value is nullable at the origin, crosses N carriers, and is only narrowed deep inside. It should be narrowed at or near the origin, for example by gating render on data being loaded and then passing the non-null value down.
3. Re-widening. A value that is already narrowed (non-null) goes into a nullable slot and gets checked again. This is dead nullability seen from the caller's side.
4. Guard fan-out. One origin produces many guards downstream (the same if (!chatID) repeated in many helpers).
5. Side finding: falsy checks on nullable values. !x on number | undefined or string | undefined also treats 0 and "" as absent. This is sometimes a real bug.

Grep baseline (non-test, non-story files): 377 nullable-typed annotations, 911 optional ?: members, 118 if (!x) return null/undefined guards, 399 ??, 640 ?.. Grep can't tell origins from carriers. For example, most if (!match) return undefined hits in thinkingTitle.ts are legitimate origins. So a syntactic approach alone will mostly produce noise.

2. Approaches

A. Syntactic heuristics (grep, a Biome rule, an agent that "just looks")

• Pros: zero setup, fast, easy to explain.
• Cons: no types and no cross-function view. It can't tell origin from carrier, and it can't see whether callers ever pass null. It has a high false-positive rate and depends on the agent's judgment, which is the thing we don't trust.
• Verdict: fine for a baseline count, not a method.

B. Bottom-up (start at sinks and carriers, walk toward callers)

Start from every nullable slot (param, prop, destructured prop, hook option field) and use the type checker to answer one question: "Does any call site actually pass a nullable value?"

• Pros:
  • Every finding is local and mechanically fixable: narrow the signature, delete the guard, and let tsc confirm.
  • Low risk, small diffs, easy to review. Agents do this well when given an exact target.
  • It converges iteratively. Narrowing a leaf can make its caller's slot dead too, so rerun until nothing changes and chains collapse upward.
  • Cheap to compute (see spike: about 13s over all of site/src).
• Cons:
  • When the inflow really is nullable, bottom-up only says "push the guard up one level." It doesn't point at the root cause or the right place to narrow.
  • Can produce churn: the same chain gets touched once per iteration.
  • Sensitive to test call sites (tests often pass literals or undefined). Resolved by the decision above: count only production inflows.

C. Top-down (start at origins, taint forward)

List origins (query data, route params, optional API fields, Map.get/find/match, context values) and follow each one forward to find how many hops it travels before it's narrowed, and how many guards it creates.

• Pros:
  • Finds root causes and the architectural fix ("narrow chat once in AgentChatPage, and pass a non-null Chat to the whole subtree").
  • Groups findings per origin, so one decision removes many guards. This matches the goal of handling absence earlier.
  • Surfaces guard fan-out and carry distance directly.
• Cons:
  • Forward dataflow through React is hard. It goes through useState, useMemo, context, hook return objects, spreads ({...props}), and callbacks, and precision drops at each of these.
  • Large fan-out, and many origins are legitimately optional end to end (for example, an optional API field shown as "n/a"). This needs an allowlist of "OK to stay optional" facts, which is domain knowledge.
  • Fixes are bigger (restructuring component boundaries, loading gates) and need human judgment. They're less suited to autonomous agents.

D. Hybrid: a nullability flow graph, queried both ways (recommended)

Build one graph and run both queries on it.

• Nodes: nullable slots (params, prop signatures, destructured props, hook option and return fields; later also context values and useState).
• Node facts: declared nullability, count of nullable vs non-null inflows (production vs test counted separately), whether the body narrows it (guard, ?., ??), and whether it's an origin.
• Edges: value flow at call arguments, object-literal arguments (hook options), and JSX attributes. Later also return values to caller variables.
• Queries:
  • Bottom-up: deadNullability means a slot with zero nullable inflows from production code.
  • Top-down: chains means for each origin, the longest path to its first narrowing, plus the number of guards along the way.
  • rewiden means a non-null argument flowing into a nullable slot, reported at the caller.
• Pros: reuses one analysis. Bottom-up gives safe mechanical wins now, and top-down gives a ranked list of architectural refactors. The graph can also be rendered (DOT or Mermaid) for a chosen origin, which helps reviewers.
• Cons: the most engineering effort, and the tool needs maintenance. Precision depends on how many React flow patterns get modeled.

3. Feasibility spike (done)

Script: /tmp/nullspike/spike.ts (about 150 lines, TypeScript compiler API, uses the existing [email protected] dependency). It covers call args, object-literal args, JSX attributes, and destructured props, and tracks pass-through edges between slots.

• Builds the program for all of site/src in about 3.5s; about 13s total.
• 308 nullable slots in AgentsPage have at least one observed inflow.
• 81 never receive a nullable value (dead nullability candidates).
• 91 receive nullable only by pass-through from another nullable slot (carrier chains).
• Deepest chains are 3 to 4 hops, for example modelOptions.ts ref <- storedRef <- storedRef <- modelDisplayName.lastModelConfigID, and chatQueueReconciliation.ts promotedHeadID/queuedTail.

Manually verified: useChatStore({ chatID: string | undefined }) has one production caller (AgentChatPage.tsx:371), which passes agentId, typed string (AgentChatPage.tsx:103). The other 65 inflows are tests. This is a real finding, but it shows the tool must separate production inflows from test inflows.

Known gaps in the spike:

• Tests and stories are mixed into the counts.
• Return values aren't tracked (functions returning T | undefined whose callers immediately use ??).
• Callbacks passed by reference, {...spread} props, useState, context, and generics aren't modeled.
• Optional "options bag" params (options?: {...}) are idiomatic and should be allowlisted.
• Falsy-check detection isn't implemented yet.

4. Methodology

1. Harden the tool (approach D) in /home/coder/nullflow/ (outside the repo). It loads typescript from site/node_modules via createRequire, so nothing is copied into site/. Changes from the spike:
   • Count production inflows and test/story inflows separately. dead means zero nullable production inflows. Also list the test files that pass nullable values, since those need updating.
   • Track return values: a function returning T | undefined where every production caller immediately uses ??, ?., or a guard, or where the function never actually returns absent.
   • Track guards in the slot's body, so each finding names the dead guard to delete.
   • Add a falsy section: !x or x && on number | undefined or string | undefined slots.
   • Exclusion list: optional options?: bags, callbacks passed by reference that the tool can't follow, and slots that receive {...spread} props (report these as "unknown", never as dead).
   • Output: JSON graph plus a Markdown report (dead, chains, rewiden, falsy), each item with file:line, inflow counts, and the upstream origin. --focus <slot> prints a Mermaid subgraph for one chain.
2. Validate precision. Hand-check about 30 findings across sections. Target at least 80% true positives for dead before using it to drive fixes. Tune the exclusions as needed.
3. Bottom-up fix loop. For each dead slot:
   • Narrow the type and delete the now-dead guard.
   • Update tests that pass undefined or null so they match production usage.
   • Run pnpm lint:types (tsc), pnpm check (Biome), and the affected Vitest tests (pnpm test <path>), then rerun the tool. Callers that just became dead show up in the next pass.
   • Repeat until nothing changes. Start with the chatID cluster (useChatStore, rightPanelTabStorage, sidebarTabStorage, draftStorage, useChatDraftAttachments, useGitWatcher, useDesktopConnection).
4. Top-down pass. Once the fixpoint is reached, the remaining chains are real nullable origins carried too far. Group them by origin, rank by number of guards downstream, and decide per origin where to narrow: a loading gate, a parent component, or a converter at the API boundary. These need judgment, so I'll report each proposed narrowing point before changing it.
5. Delivery. If the total cleanup is small (roughly up to 30 files or about 600 changed lines), ship one PR. Otherwise, ship one PR per cluster or origin. Every PR touches only AgentsPage and its tests, verified with pnpm lint:types, pnpm check, pnpm lint:oxlint, pnpm lint:compiler, and the relevant Vitest files. The final tool report (before and after counts) goes in each PR description. The tool itself isn't committed.

5. Results so far

Tool: /home/coder/nullflow/nullflow.mts, report at /home/coder/nullflow/out/report.md (about 25s per run).

Fixes made after validation:

• Story/test references (component: Foo) no longer mark props as "unknown". This had hidden 296 slots.
• Unchecked index detection now follows local consts, ?., ??/||, ternaries, destructuring, and dot access on Record types.
• Guards now include destructuring/param defaults, right operands in a && x && ... chains, and object-literal implementations of interface methods.
• Returns check reuses the unchecked-index logic and ignores ReactNode.
• Inflows that are non-null only via an as cast are flagged (e.g. useParams() as { agentId: string }, which is safe because KeyedAgentChatPage renders not-found when agentId is empty).

Precision (hand-verified by two read-only agents on the pre-fix report):

• data: 63 TP, 9 FP, 3 skip (87.5%). 8 of 9 FPs were unchecked-index cases now caught. The remaining one (ParsedMessageEntry.mergedFrom, omitted in an untyped literal) is caught by tsc when narrowing.
• callbacks: 20/20 TP. Flags: 28 TP, 3 skip (idiomatic leaf props).
• Returns: 0 real findings (section now empty).
• Falsy-risk: 15/15 sampled were intentional (empty string means absent). Not acted on.

Current dead slots, excluding idiomatic (options bags, signal, param defaults) and the skip list (chatFamilyAllowsArchive/chatStatusAllowsArchive documented fail-open, fileContentOptions, aiGatewayDisabled, CompactIntegerField.error/disabled, mergedFrom): 254 slots in 69 files (about 110 data, 73 callback, 70 flag). Well over the one-PR threshold, so one PR per cluster.

6. Clusters (one PR each, separate branches from main)

1. Chat ID and chat store: useChatStore, chatStore, rightPanelTabStorage, sidebarTabStorage, useChatDraftAttachments, chatDraftAttachmentStorage, useConversationEditingState, useChatToolInvalidations, useWorkspaceWatch, useGitWatcher, chatQueueReconciliation. Reference PR, done by me.
2. Conversation timeline and tools: ConversationTimeline, MessageBlocks, UserMessageContent, AttachmentBlocks, AssistantOutput, messageHelpers, ChatStatusCallout, ChatElements/tools/*.
3. Chat input and page shell: AgentChatInput, ChatMessageInput, SkillsTriggerMenu, AttachmentPreview, ChatPageContent, AgentChatPageView.
4. Sidebar: ChatsSidebar/*, ChatSummary, ChatActionsMenuItems (flags only).
5. Right panel: DiffViewer/*, GitPanel, TerminalPanel, DesktopPanel, RightPanelAddTabControl, DebugPanel/*.
6. Settings and misc: settings page views, UserCompactionThresholdSettings, WebPushButton, ChimeButton, AgentSetupNotice, CompactOrgSelector, UsageIndicator, modelOptions, diffColors, useAgentsPageKeybindings.

Rules for every cluster:

• Narrow the type, delete the guards/defaults the report lists, update tests and stories that pass or omit the value. tsc is the source of truth for what to update.
• Flags: narrow on feature components; skip generic leaf-control props such as disabled/error.
• A prop whose every production caller passes the same constant can be deleted instead of narrowed.
• No behavior change. Anything that turns out to be reachable with null/undefined stays as is and is reported.
• Top-down pass (step 4) happens after these land, with proposals first.

7. Open questions

None.