Conversation
🦋 Changeset detectedLatest commit: ba0c532 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…ot causes and tasks
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a powerful new diagnostic tool, Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new script turbo-why to analyze and explain Turborepo cache misses, which is a great addition for debugging CI performance. The script is integrated into various CI jobs in package.json. The PR also includes optimizations in turbo.json to prevent cache invalidation from markdown file changes. My review focuses on the new script. I've found a logical issue in how it resolves script arguments which could lead to incorrect analysis, and a minor code cleanup. Overall, the script is well-structured and will be very useful once the issue is addressed.
| */ | ||
| function resolveScript(scriptName) { | ||
| const pkgPath = join(process.cwd(), "package.json"); | ||
| const pkg = JSON.parse(readFileSync(pkgPath, "utf-8")); | ||
| const scripts = pkg.scripts ?? {}; | ||
|
|
||
| const env = {}; | ||
| const seen = new Set(); | ||
|
|
||
| let current = scriptName; | ||
| while (true) { | ||
| if (seen.has(current)) { | ||
| console.error(red(`Circular script reference: ${current}`)); | ||
| process.exit(1); | ||
| } | ||
| seen.add(current); | ||
|
|
||
| const value = scripts[current]; | ||
| if (!value) { | ||
| console.error(red(`Script "${current}" not found in package.json`)); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Tokenize the script value, collecting leading KEY=value env vars | ||
| const tokens = value.split(/\s+/); | ||
| let i = 0; | ||
| while (i < tokens.length && /^\w+=\S+$/.test(tokens[i])) { | ||
| const [k, ...rest] = tokens[i].split("="); | ||
| env[k] = rest.join("="); | ||
| i++; | ||
| } | ||
| const rest = tokens.slice(i); | ||
|
|
||
| // Case 1: turbo run <args> | ||
| if (rest[0] === "turbo" && rest[1] === "run") { | ||
| return { turboArgs: rest.slice(2), env }; | ||
| } | ||
|
|
||
| // Case 2: pnpm <script> [-- extra-args] → follow the chain | ||
| if (rest[0] === "pnpm") { | ||
| const nextScript = rest[1]; | ||
| // Collect extra args after `--` (e.g. pnpm test -- --force) | ||
| // but ignore `--` separator and args like --output-logs since those | ||
| // will be on the turbo level eventually | ||
| current = nextScript; | ||
| continue; | ||
| } | ||
|
|
||
| // Case 3: npx turbo run <args> | ||
| if (rest[0] === "npx" && rest[1] === "turbo" && rest[2] === "run") { | ||
| return { turboArgs: rest.slice(3), env }; | ||
| } | ||
|
|
||
| console.error(red(`Script "${scriptName}" does not resolve to a turbo command.`)); | ||
| console.error(dim(`Resolved to: ${value}`)); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
The current implementation of resolveScript doesn't handle arguments passed to pnpm scripts via --. This can lead to turbo-why analyzing a different command than what would be run, giving misleading results. For example, a script like pnpm test -- --force would be analyzed as if it were just pnpm test, losing the --force argument which is significant for turbo's caching behavior.
To fix this, you should collect these extra arguments and append them to the final turboArgs.
function resolveScript(scriptName) {
const pkgPath = join(process.cwd(), 'package.json');
const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8'));
const scripts = pkg.scripts ?? {};
const env = {};
const seen = new Set();
const extraArgs = [];
let current = scriptName;
while (true) {
if (seen.has(current)) {
console.error(`${RED}Circular script reference: ${current}${RESET}`);
process.exit(1);
}
seen.add(current);
const value = scripts[current];
if (!value) {
console.error(`${RED}Script "${current}" not found in package.json${RESET}`);
process.exit(1);
}
// Tokenize the script value, collecting leading KEY=value env vars
const tokens = value.split(/\s+/);
let i = 0;
while (i < tokens.length && /^\w+=\S+$/.test(tokens[i])) {
const [k, ...rest] = tokens[i].split('=');
env[k] = rest.join('=');
i++;
}
const rest = tokens.slice(i);
// Case 1: turbo run <args>
if (rest[0] === 'turbo' && rest[1] === 'run') {
return { turboArgs: [...rest.slice(2), ...extraArgs], env };
}
// Case 2: pnpm <script> [-- extra-args] → follow the chain
if (rest[0] === 'pnpm') {
const nextScript = rest[1];
const doubleDashIndex = rest.indexOf('--');
if (doubleDashIndex !== -1) {
// pnpm appends arguments after '--'. We use 'unshift' to add args from the
// outer script to the end of the final command.
extraArgs.unshift(...rest.slice(doubleDashIndex + 1));
}
current = nextScript;
continue;
}
// Case 3: npx turbo run <args>
if (rest[0] === 'npx' && rest[1] === 'turbo' && rest[2] === 'run') {
return { turboArgs: [...rest.slice(3), ...extraArgs], env };
}
console.error(`${RED}Script "${scriptName}" does not resolve to a turbo command.${RESET}`);
console.error(`${DIM}Resolved to: ${value}${RESET}`);
process.exit(1);
}
}| // but dry-run reports them as MISS since there are no cached artifacts. | ||
| const realTasks = tasks.filter((t) => t.command !== "<NONEXISTENT>"); | ||
| const skipped = tasks.length - realTasks.length; | ||
|
|
There was a problem hiding this comment.
The variable byId is declared and initialized but is never used. It should be removed to improve code clarity.
3 participants
No description provided.