feat(type-checking): Add type-checking pre-commit hooks #32261
Conversation
Added the script as a pre-commit hook to ensure type-checking before commits.
alveifbklsiu259
requested review from
sadpandajoe,
geido,
eschutho,
rusackas and
betodealmeida
as code owners
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Blocking File System Operations ▹ view | ✅ |
 |
Incorrect Log Levels for stdout/stderr ▹ view | ✅ |
 |
Missing Error Context In Catch Block ▹ view | ✅ |
Files scanned
| File Path | Reviewed |
|---|---|
| scripts/check-type.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… add error context - Updated log levels for stdout and stderr to reflect appropriate severity. - Refactored fs operations to use asynchronous method. - Enhanced error handling by adding context information.
|
@mistercrunch don't we already have some sort of type checking? Adding you since you did a lot of work to clean up our pre-commits |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #32261 +/- ##
===========================================
+ Coverage 60.48% 83.45% +22.96%
===========================================
Files 1931 546 -1385
Lines 76236 39097 -37139
Branches 8568 0 -8568
===========================================
- Hits 46114 32629 -13485
+ Misses 28017 6468 -21549
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@sadpandajoe yes we do in CI, but this add the checks to Wondering if it would make sense to only do it on superset-frontend/**/js-and-ts-files to keep pre-commit.yml a bit simpler. @alveifbklsiu259 is it all super fast? |
- Removed type-checking-websocket hook - Removed type-checking-embedded-sdk hook - Removed type-checking-cypress hook
…n CI pipeline Disable it as it requires npm ci, plus type-check is handled by other job.
alveifbklsiu259
requested review from
villebro,
nytai,
craig-rueda,
kgabryje and
dpgaspar
as code owners
github-actions
bot
added
the
github_actions
|
/korbit-review |
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
|
Missing TSConfig Existence Check ▹ view | ✅ |
|
Missing Dependency Check ▹ view | ✅ |
 |
Incomplete Process Output Handling ▹ view | ✅ |
Files scanned
| File Path | Reviewed |
|---|---|
| scripts/check-type.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
- Add check for module installation and provide clear error message if missing - Ensure both stdout and stderr are logged if they contain content - Add check for existence of tsconfig.json and provide clear error message if missing
|
/korbit-review |
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Command Injection Vulnerability in TypeScript Compiler Command ▹ view | |
|
Inefficient Array Concatenation ▹ view | ✅ |
|
Sequential Directory Traversal ▹ view | ✅ |
|
Missing Directory Read Error Handling ▹ view | ✅ |
Files scanned
| File Path | Reviewed |
|---|---|
| scripts/check-type.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
|
|
||
| const packageRootDirAbsolute = join(SUPERSET_ROOT, packageRootDir); | ||
| const tsConfig = getTsConfig(packageRootDirAbsolute); | ||
| const command = `--noEmit --allowJs --composite false --project ${tsConfig} ${argsStr} ${declarationFilesStr}`; |
There was a problem hiding this comment.
Command Injection Vulnerability in TypeScript Compiler Command 
Tell me more
What is the issue?
Command string is constructed using direct string interpolation with user-provided input (argsStr and declarationFilesStr), which could lead to command injection if the input is malicious.
Why this matters
An attacker could potentially inject malicious commands by providing specially crafted arguments that escape the intended TypeScript compiler context and execute arbitrary system commands.
Suggested change ∙ Feature Preview
Validate and sanitize argsStr and declarationFilesStr to ensure they only contain safe TypeScript compiler arguments. Consider using a whitelist of allowed arguments or a safer command construction method:
const safeArgs = validateTypeScriptArgs(argsStr);
const safeDeclarationFiles = validateDeclarationFiles(declarationFilesStr);
const command = ['--noEmit', '--allowJs', '--composite', 'false', '--project', tsConfig, ...safeArgs, ...safeDeclarationFiles].join(' ');
💬 Chat with Korbit by mentioning @korbit-ai.
There was a problem hiding this comment.
I understand the concerns about command injection, but I'd like to provide some context on why sanitization might not be strictly necessary in this case.
-
Controlled Environment: The script is executed as part of a pre-commit hook in our version control. This controlled environment means that the inputs to the script are coming from a trusted source (i.e., the pre-commit configuration). These inputs include only the changed files and predefined arguments.
-
Trusted Source: Since the arguments are provided by the pre-commit hook configuration, the risk of malicious input is significantly minimized. The inputs are controlled by the developers working within our repository, which reduces the likelihood of encountering harmful characters.
-
Practical Implications: Adding sanitization to the script could impose unnecessary restrictions on file naming conventions. This might lead to confusion and hinder developer productivity, as it would require developers to avoid certain characters in file names to ensure the type-checking script works correctly.
Additionally, we have similar validation for arguments. For instance, declaration files are ensured to be files that end with .d.ts, and only arguments predefined in pre-commit-config.yaml or files matching ^superset-frontend\/.*\.(js|jsx|ts|tsx)$ are passed to the script.
There was a problem hiding this comment.
Thank you @alveifbklsiu259 for your detailed explaination, you have valid points and this context helps. Given the controlled environment and trusted source of the inputs, the risk is indeed minimized. However, I still recommend being cautious about potential command injections, even in controlled environments -- better safe than sorry. But, as you clearly laid out, the practical implications might outweigh the theoretical security risk in this case. Thanks for your thoughtful work on this.
- getFilesRecursively: use `promise.all`, handle error. - getPackage: handle error.
|
/korbit-review |
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
|
Incorrect Directory Exclusion Logic ▹ view | ✅ |
Suppressed issues based on your team's Korbit activity
| This issue | Is similar to | Because |
|---|---|---|
|
Command string is constructed using direct string interpolation with unsanitized user inputs (argsStr and declarationFilesStr), which could lead to command injection. |
Command Injection Vulnerability in TypeScript Compiler Command |
Ignored |
When you react to issues (for example, an upvote or downvote) or you fix them, Korbit will tune future reviews based on these signals.
Files scanned
| File Path | Reviewed |
|---|---|
| scripts/check-type.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
|
/korbit-review |
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
|
Unbounded concurrent directory traversal ▹ view | |
|
Directory exclusion logic false positives ▹ view | ✅ |
Files scanned
| File Path | Reviewed |
|---|---|
| scripts/check-type.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
scripts/check-type.js
Outdated
| const recursivePromises = []; | ||
| const result = []; | ||
|
|
||
| for (const file of files) { | ||
| const fullPath = join(dir, file.name); | ||
| const shouldExclude = excludedDirs.some((excludedDir) => | ||
| normalize(fullPath).includes(normalize(excludedDir)) | ||
| ); | ||
|
|
||
| if (file.isDirectory() && !shouldExclude) { | ||
| recursivePromises.push( | ||
| getFilesRecursively(fullPath, regex, excludedDirs) | ||
| ); | ||
| } else if (regex.test(file.name)) { | ||
| result.push(fullPath); | ||
| } | ||
| } | ||
|
|
||
| const recursiveResults = await Promise.all(recursivePromises); |
There was a problem hiding this comment.
Unbounded concurrent directory traversal 
Tell me more
What is the issue?
The recursive directory traversal creates and awaits all promises simultaneously, which can lead to memory spikes when processing large directory trees.
Why this matters
In large codebases with deep directory structures, this approach will create many promises in memory before any can complete, potentially causing out-of-memory issues or degraded performance.
Suggested change ∙ Feature Preview
Implement a semaphore or concurrency limit to control the number of concurrent directory reads. Consider using a library like p-limit or implement a simple queue:
const pLimit = require('p-limit');
const limit = pLimit(10); // limit concurrent operations
async function getFilesRecursively(dir, regex, excludedDirs) {
const files = await readdir(dir, { withFileTypes: true });
const recursivePromises = [];
const result = [];
for (const file of files) {
// ... existing checks ...
if (file.isDirectory() && !shouldExclude) {
recursivePromises.push(
limit(() => getFilesRecursively(fullPath, regex, excludedDirs))
);
}
// ... rest of the code ...
}
}💬 Chat with Korbit by mentioning @korbit-ai.
There was a problem hiding this comment.
After analyzing the memory impact, I found that the increase is not significant enough to warrant the added complexity of introducing a semaphore or concurrency limit. Implementing such a solution would complicate the codebase.
I believe the current implementation is sufficient for our use case and does not pose any immediate performance concerns. Maybe we will consider revisiting this approach if we encounter any memory-related issues in the future.
const initial = process.memoryUsage();
let declarationFiles = await getFilesRecursively(
packageRootDir,
DECLARATION_FILE_REGEX,
excludedDeclarationDirs
);
const final = process.memoryUsage();
const memoryDiff = {
rss: `${Math.round((final.rss - initial.rss) / (1024 * 1024))} MB`,
heapTotal: `${Math.round(
(final.heapTotal - initial.heapTotal) / (1024 * 1024)
)} MB`,
heapUsed: `${Math.round(
(final.heapUsed - initial.heapUsed) / (1024 * 1024)
)} MB`,
external: `${Math.round(
(final.external - initial.external) / (1024 * 1024)
)} MB`,
};
console.log("Memory Difference:", memoryDiff);
// Memory Difference: { rss: '16 MB', heapTotal: '9 MB', heapUsed: '3 MB', external: '0 MB' }
feat(type-checking): Add type-checking pre-commit hooks
SUMMARY
This PR introduces some pre-commit hooks to check TypeScript files using
tscw-config. This enhancement aims to streamline the development process by running type checks only on changed files.NOTES
superset-frontend,superset-websocket...) use their owntsconfig.jsonand may require different declaration files,multiple type-checking hooks are used. (edit: after discussion, only one hook (type-check-frontend) is used, if future requirement changes, one can always reference commits).checkJsis not enabled by default. To enable type-checking for JS/JSX files, either include//@ts-checkat the top of each JS/JSX file or enable it in the package'stsconfig.json.superset-frontend,superset-websocket... will include all files ending with.d.tsin their respective directories (recursively). However, if one needs to include third-party declaration files, one can either import them directly in the files that need them or declare them in thetypefield of the package'stsconfig.json. For exampleimport "@testing-library/jest-dom"tsconfig.jsonfrom the current package (superset-frontend/tsconfig.jsonforsuperset-frontend), remove theincludefiled, and include all the files passed to this hook (and some declaration files) in thefilesfield. This means that ANY files passed to the hook will be type-cheked even if they are not included in the originaltsconfig.json'sincludefield. For example, at the time of writing,superset-frontend/tsconfig.jsondoes not include files from./*/storybook/**/*, but if one modifiessuperset-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-color/RenderPalettes.tsx, this file will still be type-checked. IMHO type-checking the files even though they will not be compiled to javascript is not a bad idea, but if one really wants to disable this behavior, one can:// @ts-nocheck.exclude: ^(superset-frontend/cypress-base/|superset-frontend/.*/storybook/)inpre-commit-config.yaml.DISCLOSURE
I am the author of
tscw-config. I genuinely believe that this library offers an effective solution to the issue discussed in Issue #27379.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION