chore: clear strictNullChecks errors in remaining feature modules#1010
Merged
igboyes merged 3 commits intoJun 3, 2026
Conversation
Clears strict-null errors across subtraction, hmm, jobs, uploads, users,
groups, labels, nav, and quality (plus index.tsx) and ratchets each green
file into the tsconfig.strictNullChecks.json allowlist (96 files). The main
tsconfig.json flag stays off until cutover.
The allowlist gate type-checks the full transitive program, so shared
foundation files that the feature components import had to be fixed first:
app/utils.ts, app/hooks.tsx, base/Alert.tsx, base/InputGroup.tsx,
indexes/queries.ts, and administration/{queries,utils,hooks}.ts.
Dominant fix is React Query narrowing (`if (isPending || !data) return
<LoadingPlaceholder/>`), preserving loading-on-error behaviour. Notable
root-cause fixes: server mutation handlers returned `T | undefined` because
their catch blocks ended in a bare `rethrowAsHttp(err)` expression (added
`return`); User.administrator_role was mistyped as non-null (the server type
is already `| null`) and now flows through a null-tolerant
hasSufficientAdminRole and the Nav/Sidebar/AdministrationTabs props.
routes/ files are intentionally left out of the allowlist: they
transitively pull in the samples, references, otus, and analyses feature
modules (owned by separate issues) plus app-wide createFileRoute loader
typing, none of which are null-clean yet. Listed escape hatches are
untouched and no new `as any`/`as never`/`@ts-expect-error` was introduced.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThis PR widens several prop and type definitions to allow null, tightens loading guards to require both pending flags and data presence, and adds defensive null checks in hooks, utilities, validators, and components. Server label handlers now return mapped HTTP rethrows. Tests and small UI behaviors were adjusted to match the nullable-safe contracts. The tsconfig for the web app was expanded to enable strictNullChecks across the listed sources. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
JobSteps, thestepsprop is now typed asJobStep[] | nullbut the component body still assumes an array; either keep the type non-nullable and normalize upstream, or add a null/empty guard before iterating or rendering. - In
UploadItem,useris nowUserNested | nullbut the component still appears to assume a non-null user (eg. accessinguser.handle); either keepuserrequired or add a fallback rendering path when it is null. - In
Groups, passingselectedGroupId ?? 0intouseFetchGroupmay trigger a fetch for an invalid group ID0; consider updating the hook to acceptundefined/nulland skip the query or handling this case explicitly instead of using a sentinel value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `JobSteps`, the `steps` prop is now typed as `JobStep[] | null` but the component body still assumes an array; either keep the type non-nullable and normalize upstream, or add a null/empty guard before iterating or rendering.
- In `UploadItem`, `user` is now `UserNested | null` but the component still appears to assume a non-null user (eg. accessing `user.handle`); either keep `user` required or add a fallback rendering path when it is null.
- In `Groups`, passing `selectedGroupId ?? 0` into `useFetchGroup` may trigger a fetch for an invalid group ID `0`; consider updating the hook to accept `undefined`/`null` and skip the query or handling this case explicitly instead of using a sentinel value.
## Individual Comments
### Comment 1
<location path="apps/web/src/base/Alert.tsx" line_range="38" />
<code_context>
className={cn(
"rounded-md shadow-sm font-medium mb-4 overflow-hidden",
- alertColorStyles[color] ?? defaultColorStyle,
+ (color && alertColorStyles[color]) ?? defaultColorStyle,
outerClassName,
)}
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `color && alertColorStyles[color]` check changes behavior for falsy `color` values like an empty string.
This means an empty string (or other falsy invalid value) will now evaluate as a valid result, giving `""` instead of `defaultColorStyle`, so no alert classes are applied. If `color` can ever be empty or otherwise invalid, prefer a nullish check:
```ts
alertColorStyles[color as keyof typeof alertColorStyles] ?? defaultColorStyle
```
or explicitly guard against `undefined`/invalid keys while still treating `""` as invalid.
</issue_to_address>
### Comment 2
<location path="apps/web/src/groups/components/Groups.tsx" line_range="38" />
<code_context>
}
- const { data: selectedGroup } = useFetchGroup(selectedGroupId);
+ const { data: selectedGroup } = useFetchGroup(selectedGroupId ?? 0);
- if (isPendingGroups || (groups.length && !selectedGroup)) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `0` as a fallback group id may trigger an unnecessary or invalid fetch when `selectedGroupId` is `null`.
Passing `0` means `useFetchGroup` will still fire when `selectedGroupId` is `null`, likely requesting a non-existent group and causing a failed/extra request. Since rendering is already guarded on `selectedGroup`, it’d be better to avoid calling `useFetchGroup` at all in this case (e.g., via an `enabled` flag or by passing `undefined` when `selectedGroupId` is `null`).
Suggested implementation:
```typescript
const { data: selectedGroup } = useFetchGroup(selectedGroupId ?? undefined, {
enabled: !!selectedGroupId,
});
```
This change assumes `useFetchGroup` accepts a second options argument with an `enabled` flag (common with React Query-style hooks). If `useFetchGroup` has a different signature, you should instead:
1. Update `useFetchGroup`’s implementation to accept an `options?: { enabled?: boolean }` parameter and wire that into the underlying query hook, or
2. If it already accepts an options object but with a different shape, adapt the second argument accordingly (e.g., `{ query: { enabled: !!selectedGroupId } }`).
</issue_to_address>
### Comment 3
<location path="apps/web/src/subtraction/components/__tests__/SubtractionItem.test.tsx" line_range="71" />
<code_context>
it("should correctly render subtractions where jobs=null", async () => {
- props.job = null;
+ props.job = undefined;
props.ready = false;
</code_context>
<issue_to_address>
**suggestion (testing):** Align test description and setup for `jobs=null` vs `undefined`, and consider covering both cases explicitly
The test name still refers to `jobs=null`, but the setup now uses `props.job = undefined`. If `null` and `undefined` are treated differently at runtime, this could hide an edge case. Please either rename the test to match `undefined` or add a separate test that sets `props.job = null` so both values are exercised.
```suggestion
it("should correctly render subtractions where jobs=undefined", async () => {
```
</issue_to_address>
### Comment 4
<location path="apps/web/src/uploads/components/__tests__/UploadItem.test.tsx" line_range="26-29" />
<code_context>
it("should render", () => {
- renderWithProviders(<UploadItem {...props} />);
+ const user = { id: 1, handle: "bill" };
+ renderWithProviders(<UploadItem {...props} user={user} />);
- expect(screen.getByText(new RegExp(props.user.handle))).toBeInTheDocument();
+ expect(screen.getByText(new RegExp(user.handle))).toBeInTheDocument();
expect(screen.getByText(new RegExp(props.name))).toBeInTheDocument();
expect(screen.getByText("10.0 B")).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for `UploadItem` when `user` is `null` to verify the new nullable type handling
Since `user` can now be `UserNested | null`, please add a test where `user={null}` (or the prop is omitted, if supported) and assert the expected UI in that case (e.g., no handle or a fallback label) to cover the nullable path in `UploadItem`.
Suggested implementation:
```typescript
});
it("should render", () => {
const user = { id: 1, handle: "bill" };
renderWithProviders(<UploadItem {...props} user={user} />);
expect(screen.getByText(new RegExp(user.handle))).toBeInTheDocument();
expect(screen.getByText(new RegExp(props.name))).toBeInTheDocument();
expect(screen.getByText("10.0 B")).toBeInTheDocument();
expect(screen.getByRole("button", { name: "remove" })).toBeInTheDocument();
});
it("should render when user is null", () => {
renderWithProviders(<UploadItem {...props} user={null} />);
// The user handle should not be rendered when user is null
expect(screen.queryByText(new RegExp(props.user.handle))).not.toBeInTheDocument();
// Other upload data should still be rendered
expect(screen.getByText(new RegExp(props.name))).toBeInTheDocument();
expect(screen.getByText("10.0 B")).toBeInTheDocument();
expect(screen.getByRole("button", { name: "remove" })).toBeInTheDocument();
});
```
If `UploadItem` renders a specific fallback label when `user` is null (for example, "Unknown user" or "You"), you should update the new test accordingly:
1. Replace the `queryByText`/`not.toBeInTheDocument` assertion with an expectation for that fallback, e.g.:
`expect(screen.getByText("Unknown user")).toBeInTheDocument();`
2. Ensure the `props` fixture used in this test includes a `user` with a `handle` so that `props.user.handle` is defined for the negative assertion (or inline the string if preferable).
</issue_to_address>
### Comment 5
<location path="apps/web/src/subtraction/components/Detail/__tests__/SubtractionDetail.test.tsx" line_range="93-94" />
<code_context>
it("should render file id when name not defined", async () => {
const subtraction = createFakeSubtraction({
- file: { id: 999, name: null },
+ file: { id: 999, name: "" },
});
const scope = mockApiGetSubtractionDetail(subtraction);
</code_context>
<issue_to_address>
**suggestion (testing):** Clarify and/or extend the test for missing file name now that `name` is an empty string instead of `null`
Since the fixture now uses `name: ""` instead of `null`, the current test name no longer matches precisely. If the code should treat `null` and empty string the same, either adjust the description to mention an empty name, or add a separate test (e.g. `file: { id: 999 }` or `name: null` via cast) so both cases are covered explicitly.
Suggested implementation:
```typescript
it("should render file id when file name is empty", async () => {
const subtraction = createFakeSubtraction({
file: { id: 999, name: "" },
});
const scope = mockApiGetSubtractionDetail(subtraction);
await renderRoute(formatSubtractionPath(subtraction));
await screen.findByText("999");
});
it("should render file id when file name is missing", async () => {
const subtraction = createFakeSubtraction({
file: { id: 999 } as any,
});
const scope = mockApiGetSubtractionDetail(subtraction);
await renderRoute(formatSubtractionPath(subtraction));
await screen.findByText("999");
```
1. Ensure `screen` is imported from `@testing-library/react` at the top of this test file if it is not already present.
2. If the existing test previously had a different assertion style (e.g. `getByText`, `findByText` with additional matchers, or snapshot testing), mirror that same assertion pattern in both tests instead of the `await screen.findByText("999");` shown here.
3. If `createFakeSubtraction` or the `file` type does not accept `name` being omitted, adjust the `as any` cast to the appropriate type or use a `name: null as unknown as string` pattern to represent a `null` name explicitly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/uploads/hooks.ts (1)
37-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
useEffectdependencies inapps/web/src/uploads/hooks.tsto track selected-ID changesThe
useEffectdepends onselected.lengthandselected.every;selected.everyis stable, so the effect won’t rerun when selected IDs swap but the length stays the same, which can leave validation using stale selection state.Suggested fix
}, [ data, - selected.length, + selected, hasNextPage, setSelected, - selected.every, isPending, fetchNextPage, ]);apps/web/src/app/utils.ts (1)
141-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle malformed sessionStorage payloads defensively.
JSON.parse(item)can throw and crash the read path if storage is corrupted or manually edited. Since this API now models absence withnull, parse failures should also returnnullsafely.Suggested fix
export function getSessionStorage(key: string): object | null { const item = window.sessionStorage.getItem(key); if (item === null) { return null; } - return JSON.parse(item); + try { + const parsed: unknown = JSON.parse(item); + return typeof parsed === "object" && parsed !== null ? (parsed as object) : null; + } catch { + return null; + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8584cd52-f2f1-4258-aa87-a8883126f695
📒 Files selected for processing (41)
apps/web/src/administration/components/AdministrationTabs.tsxapps/web/src/administration/queries.tsapps/web/src/administration/utils.tsapps/web/src/analyses/components/Pathoscope/PathoscopeOtuCoverage.tsxapps/web/src/app/hooks.tsxapps/web/src/app/utils.tsapps/web/src/base/Alert.tsxapps/web/src/base/InputGroup.tsxapps/web/src/groups/components/Groups.tsxapps/web/src/hmm/components/HmmInstall.tsxapps/web/src/hmm/components/HmmList.tsxapps/web/src/hmm/queries.tsapps/web/src/indexes/queries.tsapps/web/src/jobs/components/JobDetail.tsxapps/web/src/jobs/components/JobStep.tsxapps/web/src/jobs/components/JobSteps.tsxapps/web/src/jobs/components/__tests__/JobDetail.test.tsxapps/web/src/labels/components/Labels.tsxapps/web/src/nav/components/Nav.tsxapps/web/src/nav/components/Sidebar.tsxapps/web/src/quality/components/Sequences.tsapps/web/src/server/labels/functions.tsapps/web/src/subtraction/components/Detail/SubtractionDetail.tsxapps/web/src/subtraction/components/Detail/__tests__/SubtractionDetail.test.tsxapps/web/src/subtraction/components/SubtractionCreate.tsxapps/web/src/subtraction/components/SubtractionItem.tsxapps/web/src/subtraction/components/SubtractionList.tsxapps/web/src/subtraction/components/__tests__/SubtractionItem.test.tsxapps/web/src/uploads/components/FileManager.tsxapps/web/src/uploads/components/UploadBar.tsxapps/web/src/uploads/components/UploadItem.tsxapps/web/src/uploads/components/UploaderDialog.tsxapps/web/src/uploads/components/__tests__/UploadItem.test.tsxapps/web/src/uploads/hooks.tsapps/web/src/users/components/PrimaryGroup.tsxapps/web/src/users/components/UserDetail.tsxapps/web/src/users/components/UserGroups.tsxapps/web/src/users/components/UserItem.tsxapps/web/src/users/components/UsersList.tsxapps/web/src/users/types.tsapps/web/tsconfig.strictNullChecks.json
- narrow Alert color prop to the shared palette union and add gray/purple styles per the base-component color convention - guard getSessionStorage JSON.parse against corrupted storage - fix useValidateFiles effect deps (use selected, not selected.length / selected.every) - correct test names and assert the UploadItem null-user fallback
igboyes
deleted the
igboyes/vir-2404-fix-strictnullchecks-errors-in-remaining-modules-subtraction
branch
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
1 participant
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.
Summary
strictNullChecksTypeScript errors across administration, analyses, app, base, groups, hmm, indexes, jobs, labels, nav, quality, subtraction, uploads, and users modulesnullguards and narrows types throughout (e.g.AdministratorRoleName | null,GroupMinimal | null, proper!dataloading guards) to make existing runtime behaviour explicit under strict null checkingtsconfig.strictNullChecks.jsonto include all newly fixed files, enabling the CI gate to enforce strictNullChecks for these modules going forwardTest plan
pnpm typecheckpassespnpm checkpasses with no lint errorspnpm testpasses (subtraction, uploads, jobs, and users test suites in particular)