chore: add strictNullChecks allowlist tsconfig and CI gate#1009
Conversation
Add apps/web/tsconfig.strictNullChecks.json, an overlay extending the main tsconfig that flips on only strictNullChecks over an explicit file allowlist, seeded with src/app/types.ts. Add a typecheck:strict script and a required strict-compile step to the check-typescript CI job so allowlisted files that regress fail the PR. The loose main tsconfig.json is left unchanged.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduces a strict-null-checks-only TypeScript config for the web app, wires it into package scripts, and adds a CI gate so an allowlisted subset of files is compiled with strictNullChecks on every PR. Flow diagram for new strictNullChecks CI typecheck gateflowchart LR
CI[GitHub_Actions_typecheck_job]
TSC[Step tsc]
STRICT[Step tsc_strictNullChecks]
TSBASE[tsconfig_json]
TSSTRICT[tsconfig_strictNullChecks_json]
CI --> TSC
CI --> STRICT
TSC --> TSBASE
STRICT --> TSSTRICT
Dev[Developer]
Dev --> PKG[apps_web_package_json_scripts]
PKG --> LOCALSTRICT[typecheck_strict_script]
LOCALSTRICT --> TSSTRICT
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughThe PR introduces strict null checking for TypeScript in the web application. It adds a dedicated TypeScript configuration file that extends the base config and enables strict null checks scoped to ChangesStrict Null Checking
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 left some high level feedback:
- In the CI workflow, consider invoking the new
typecheck:strictscript (e.g.pnpm --filter @virtool/web typecheck:strict) instead of directly callingtsc -p tsconfig.strictNullChecks.jsonso the command definition lives in one place and stays in sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the CI workflow, consider invoking the new `typecheck:strict` script (e.g. `pnpm --filter @virtool/web typecheck:strict`) instead of directly calling `tsc -p tsconfig.strictNullChecks.json` so the command definition lives in one place and stays in sync.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a strict null-checking script (typecheck:strict) to package.json and introduces a new tsconfig.strictNullChecks.json configuration file targeting src/app/types.ts. Feedback on the configuration points out that overriding the include property completely replaces the base configuration's include paths, which will prevent ambient type definitions (such as .d.ts files) from being loaded. It is recommended to update the include array to also include "src/**/*.d.ts" to ensure these global declarations remain available.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "compilerOptions": { | ||
| "strictNullChecks": true | ||
| }, | ||
| "include": ["src/app/types.ts"] |
There was a problem hiding this comment.
When overriding the include property in an extending tsconfig, the base include (which is ["src/**/*"] in tsconfig.json) is completely replaced. This means any ambient type definitions (such as vite-env.d.ts or other .d.ts files in src/) will not be loaded.
As more files are added to this allowlist, any reference to global variables, Vite environment variables (import.meta.env), or other ambient types will fail to compile. Including "src/**/*.d.ts" ensures these ambient declarations are always available.
"include": [
"src/**/*.d.ts",
"src/app/types.ts"
]![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/tsconfig.strictNullChecks.json (1)
3-6: ⚡ Quick winDon’t assume the strict gate can emit—inheritance already disables it
apps/web/tsconfig.strictNullChecks.jsonextendsapps/web/tsconfig.json, which already setscompilerOptions.noEmit: true, so the strict-null gate is currently non-emitting. AddingnoEmit: trueto the strict config is only a defensive, self-contained improvement if you want to guard against future base-config changes.Suggested change
{ "extends": "./tsconfig.json", "compilerOptions": { - "strictNullChecks": true + "strictNullChecks": true, + "noEmit": true }, "include": ["src/app/types.ts"] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/tsconfig.strictNullChecks.json` around lines 3 - 6, The strict-null gate file tsconfig.strictNullChecks.json currently enables "strictNullChecks" but inherits a noEmit setting from the base tsconfig.json, which can change; add "noEmit": true to the "compilerOptions" object in tsconfig.strictNullChecks.json (alongside "strictNullChecks") so this strict config remains non-emitting even if the base tsconfig.json is modified, keeping the "include": ["src/app/types.ts"] unchanged..github/workflows/ci.yaml (1)
81-82: ⚡ Quick winReuse
@virtool/web’stypecheck:strictscript in CI
ci.yamlhardcodestsc -p tsconfig.strictNullChecks.json, butapps/web/package.jsonalready defines the same command astypecheck:strict(and theapps/web/tsconfig.strictNullChecks.jsonfile exists there). Reuse the script to avoid drift.Suggested change
- name: tsc (strictNullChecks) - run: pnpm --filter `@virtool/web` exec tsc -p tsconfig.strictNullChecks.json + run: pnpm --filter `@virtool/web` run typecheck:strict🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 81 - 82, Replace the hardcoded tsc invocation in the CI step named "tsc (strictNullChecks)" with the package script `typecheck:strict` from the `@virtool/web` package to avoid drift; specifically, change the step's run command to use `pnpm --filter `@virtool/web` run typecheck:strict` so it executes the existing script (which already points to `tsconfig.strictNullChecks.json`) instead of calling `tsc -p tsconfig.strictNullChecks.json` directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 81-82: Replace the hardcoded tsc invocation in the CI step named
"tsc (strictNullChecks)" with the package script `typecheck:strict` from the
`@virtool/web` package to avoid drift; specifically, change the step's run
command to use `pnpm --filter `@virtool/web` run typecheck:strict` so it executes
the existing script (which already points to `tsconfig.strictNullChecks.json`)
instead of calling `tsc -p tsconfig.strictNullChecks.json` directly.
In `@apps/web/tsconfig.strictNullChecks.json`:
- Around line 3-6: The strict-null gate file tsconfig.strictNullChecks.json
currently enables "strictNullChecks" but inherits a noEmit setting from the base
tsconfig.json, which can change; add "noEmit": true to the "compilerOptions"
object in tsconfig.strictNullChecks.json (alongside "strictNullChecks") so this
strict config remains non-emitting even if the base tsconfig.json is modified,
keeping the "include": ["src/app/types.ts"] unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 542fcb8e-86b6-48d9-9949-4bcc80431fe6
📒 Files selected for processing (3)
.github/workflows/ci.yamlapps/web/package.jsonapps/web/tsconfig.strictNullChecks.json
- Include src/**/*.d.ts so ambient declarations are not dropped by the include override as the allowlist grows - Set noEmit explicitly on the strict overlay - Run the typecheck:strict script in CI instead of duplicating the tsc call
igboyes
deleted the
igboyes/vir-2398-set-up-dual-tsconfig-strictnullchecks-allowlist-and-ci-gate
branch
1 participant
Summary
apps/web/tsconfig.strictNullChecks.json, a secondary tsconfig that extends the base config and enablesstrictNullChecks, with an explicitincludeallowlist starting withsrc/app/types.tstypecheck:strictscript inapps/web/package.jsonfor running the allowlist typecheck locallytsc (strictNullChecks)step to the CItypecheckjob so the allowlist is enforced in every PRTest plan
tsc (strictNullChecks)steppnpm --filter @virtool/web exec tsc -p tsconfig.strictNullChecks.jsonlocally exits cleanlyincludethat has null-check violations causes the CI step to failSummary by Sourcery
Introduce a strict null-checking TypeScript configuration for the web app and enforce it via local scripts and CI.
Enhancements:
CI: