⚡ perf: Cache synchronous file reads in step iteration loop - #3
groupthinking wants to merge 1 commit into
Conversation
Introduced a `fileCache` Map in `cli.ts` to prevent redundant synchronous disk I/O when generating markdown for multiple sections that reference the same source files. The cache is passed to `generateRichSectionMarkdown` and used in both loops where `fs.readFileSync` was previously called directly. Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
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 significantly enhances the performance of the Highlights
Changelog
Activity
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
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
| return null; | ||
| } | ||
| function generateRichSectionMarkdown(section, projectRoot, sectionWorkingDir, walkthroughTargets) { |
There was a problem hiding this comment.
Bug: The compiled file dist/src/cli.js is stale and does not include the caching logic from the TypeScript source, potentially causing the optimization to be missed in production.
Severity: MEDIUM
Suggested Fix
Ensure the build process regenerates the dist directory from the TypeScript source before publishing. Consider removing the dist directory from version control and adding it to .gitignore to prevent stale compiled files from being committed in the future.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/walkthroughgen/dist/src/cli.js#L192
Potential issue: The compiled JavaScript file `dist/src/cli.js` is out of sync with its
TypeScript source file `src/cli.ts`. The call to `generateRichSectionMarkdown` is
missing the `fileCache` parameter that was added in the source. While the test suite
validates the TypeScript source directly, this discrepancy poses a risk. If the
project's build and deployment process uses the committed `dist` files directly without
regenerating them, the intended caching optimization will be missing from the production
code, silently negating the performance improvement of this change.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the walkthrough generator CLI by caching repeated synchronous reads of step source files during markdown/section README generation to reduce redundant disk I/O.
Changes:
- Add a
fileCache: Map<string, string>incli()and use it to reuse source file contents across steps/sections. - Thread
fileCacheintogenerateRichSectionMarkdown()and apply cached reads in both section-README and main markdown generation paths. - Add several generated
.jsartifacts undertest/anddist/, and update gitignore entries.
Reviewed changes
Copilot reviewed 5 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/walkthroughgen/src/cli.ts | Introduces fileCache and uses it to avoid repeated readFileSync calls. |
| packages/walkthroughgen/test/e2e/test-e2e.js | Added JS copy of e2e tests (duplicates TS suite). |
| packages/walkthroughgen/test/utils/console-mock.js | Added JS copy of console mocking helper (duplicates TS helper). |
| packages/walkthroughgen/test/utils/temp-dir.js | Added JS copy of temp dir helper (duplicates TS helper). |
| packages/walkthroughgen/dist/src/cli.js | Added compiled JS CLI artifact (currently not reflecting the new caching changes). |
| packages/walkthroughgen/dist/src/index.js | Added compiled JS entrypoint artifact. |
| packages/walkthroughgen/dist/test/e2e/test-e2e.js | Added compiled JS test artifact. |
| packages/walkthroughgen/dist/test/utils/console-mock.js | Added compiled JS test utility artifact. |
| packages/walkthroughgen/dist/test/utils/temp-dir.js | Added compiled JS test utility artifact. |
| packages/walkthroughgen/.gitignore | Updates ignore rules (currently malformed in this PR). |
| .gitignore | Adds ignore rule for packages/walkthroughgen/node_modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "use strict"; | ||
| var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) { | ||
| if (k2 === undefined) k2 = k; | ||
| var desc = Object.getOwnPropertyDescriptor(m, k); | ||
| if (!desc || ("get" in desc ? !m.__esModule : desc.writable || desc.configurable)) { | ||
| desc = { enumerable: true, get: function() { return m[k]; } }; | ||
| } | ||
| Object.defineProperty(o, k2, desc); | ||
| }) : (function(o, m, k, k2) { | ||
| if (k2 === undefined) k2 = k; |
There was a problem hiding this comment.
This .js e2e test file duplicates the .ts test suite, but Jest is configured to only match **/test/**/*.ts (see jest.config.js). That means this file won’t run in CI and is likely an accidental compiled output; consider removing it (and the related JS utils) to avoid confusion and repo bloat, or update the test configuration if you intend to run JS tests too.
| * A utility function to mock console.log and console.error and capture their output | ||
| * @param callback The function to execute while console is mocked | ||
| * @returns The captured console output (both log and error messages) | ||
| */ | ||
| const withMockedConsole = (callback) => { | ||
| const originalConsoleLog = console.log; | ||
| const originalConsoleError = console.error; | ||
| let capturedOutput = []; | ||
| console.log = (...args) => { | ||
| capturedOutput.push(args.join(" ")); | ||
| }; | ||
| console.error = (...args) => { | ||
| capturedOutput.push(args.join(" ")); | ||
| }; | ||
| try { | ||
| callback(); | ||
| } | ||
| finally { | ||
| console.log = originalConsoleLog; | ||
| console.error = originalConsoleError; | ||
| } | ||
| return capturedOutput.join("\n"); | ||
| }; | ||
| exports.withMockedConsole = withMockedConsole; |
There was a problem hiding this comment.
This JS helper appears to be a compiled duplicate of test/utils/console-mock.ts. Since the Jest config only runs .ts tests, the .js utilities are likely dead code and add maintenance overhead (risk of drifting behavior). Consider deleting this file (and other compiled JS under test/utils) or adjusting the build/test setup so there’s a single source of truth.
| * A utility function to mock console.log and console.error and capture their output | |
| * @param callback The function to execute while console is mocked | |
| * @returns The captured console output (both log and error messages) | |
| */ | |
| const withMockedConsole = (callback) => { | |
| const originalConsoleLog = console.log; | |
| const originalConsoleError = console.error; | |
| let capturedOutput = []; | |
| console.log = (...args) => { | |
| capturedOutput.push(args.join(" ")); | |
| }; | |
| console.error = (...args) => { | |
| capturedOutput.push(args.join(" ")); | |
| }; | |
| try { | |
| callback(); | |
| } | |
| finally { | |
| console.log = originalConsoleLog; | |
| console.error = originalConsoleError; | |
| } | |
| return capturedOutput.join("\n"); | |
| }; | |
| exports.withMockedConsole = withMockedConsole; | |
| * This file previously contained a compiled duplicate of the TypeScript helper | |
| * in `console-mock.ts`. To avoid drift and keep a single source of truth, it now | |
| * re-exports the implementation from the TypeScript module. | |
| */ | |
| // eslint-disable-next-line @typescript-eslint/no-var-requires | |
| const tsHelper = require("./console-mock.ts"); | |
| exports.withMockedConsole = tsHelper.withMockedConsole; |
| exports.withTmpDir = withTmpDir; | ||
| const fs_1 = require("fs"); | ||
| const path_1 = require("path"); | ||
| /** | ||
| * Creates a temporary directory, executes a function with that directory, then removes it | ||
| */ | ||
| function withTmpDir(fn) { | ||
| const dir = (0, fs_1.mkdtempSync)((0, path_1.join)(__dirname, '.tmptest')); | ||
| try { | ||
| return fn(dir); | ||
| } | ||
| finally { | ||
| (0, fs_1.rmSync)(dir, { recursive: true, force: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This JS helper appears to be a compiled duplicate of test/utils/temp-dir.ts. Given the Jest config only runs .ts tests, keeping both .ts and .js versions in-repo increases the chance they diverge and is likely unintentional—consider removing the compiled .js copy (or updating tooling so only one version is checked in).
| exports.withTmpDir = withTmpDir; | |
| const fs_1 = require("fs"); | |
| const path_1 = require("path"); | |
| /** | |
| * Creates a temporary directory, executes a function with that directory, then removes it | |
| */ | |
| function withTmpDir(fn) { | |
| const dir = (0, fs_1.mkdtempSync)((0, path_1.join)(__dirname, '.tmptest')); | |
| try { | |
| return fn(dir); | |
| } | |
| finally { | |
| (0, fs_1.rmSync)(dir, { recursive: true, force: true }); | |
| } | |
| } | |
| /** | |
| * NOTE: | |
| * This file is a thin wrapper around the TypeScript implementation in `temp-dir.ts` | |
| * to avoid maintaining two divergent copies of the same helper. | |
| */ | |
| const tempDirTs = require("./temp-dir.ts"); | |
| function withTmpDir(fn) { | |
| return tempDirTs.withTmpDir(fn); | |
| } | |
| exports.withTmpDir = withTmpDir; |
| if (fileCache.has(srcAbsolutePath)) { | ||
| newContent = fileCache.get(srcAbsolutePath)!; | ||
| } else { | ||
| newContent = fs.readFileSync(srcAbsolutePath, 'utf8'); | ||
| fileCache.set(srcAbsolutePath, newContent); | ||
| } |
There was a problem hiding this comment.
The cached file-read logic is duplicated in multiple places (here and in generateRichSectionMarkdown). To reduce the chance of future drift, consider extracting a small helper like readFileCached(fileCache, absPath) and reusing it for both loops.
| @@ -1 +1 @@ | |||
| .tmptest* No newline at end of file | |||
| .tmptest*node_modules/ | |||
There was a problem hiding this comment.
The .gitignore entry looks malformed: .tmptest*node_modules/ combines two patterns into one, so it won’t ignore either temporary test dirs or node_modules as intended. Split this into separate lines (e.g., one for .tmptest* and one for node_modules/).
| .tmptest*node_modules/ | |
| .tmptest* | |
| node_modules/ |
![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 file caching mechanism to the walkthroughgen CLI tool, specifically within the generateRichSectionMarkdown function and the main cli function, to optimize file reading performance by avoiding redundant disk I/O. However, the review comments highlight several issues: a critical security vulnerability where untrusted YAML input is directly embedded into generated markdown without sanitization, potentially leading to Cross-Site Scripting (XSS) or content injection; a violation of the DRY principle due to duplicated file caching logic that should be refactored into a helper function; and incorrect .gitignore entries, as node_modules/ was improperly appended to an existing pattern, and the dist/ directory containing compiled output was committed and not ignored.
|
|
||
| // Generate and write section README | ||
| const sectionMarkdown = generateRichSectionMarkdown(section, projectRoot, sectionPath, data.targets); | ||
| const sectionMarkdown = generateRichSectionMarkdown(section, projectRoot, sectionPath, data.targets, fileCache); |
There was a problem hiding this comment.
The generateRichSectionMarkdown function constructs markdown content by directly embedding values from the untrusted walkthrough.yaml file (e.g., section.title, section.text, step.text, step.command, result.code). If a malicious YAML file is processed, an attacker can inject arbitrary HTML or Markdown. When this generated markdown is rendered in a web browser or a markdown viewer that doesn't properly sanitize the content, it could lead to Cross-Site Scripting (XSS) attacks, allowing arbitrary JavaScript execution, or other content injection vulnerabilities.
Remediation: All untrusted input that is embedded into the markdown output should be properly escaped or sanitized. For HTML contexts, this means HTML-escaping characters like <, >, &, ", '. For markdown contexts, this means escaping markdown-specific characters (e.g., *, _, [, ], (, )). Consider using a robust markdown sanitization library before writing the final markdown to the file.
| @@ -1 +1 @@ | |||
| .tmptest* No newline at end of file | |||
| .tmptest*node_modules/ | |||
There was a problem hiding this comment.
The change from .tmptest* to .tmptest*node_modules/ seems incorrect. This new pattern will only match a string that contains both parts, like .tmptest-foo-node_modules/, rather than ignoring both .tmptest* files and the node_modules directory separately. Each pattern should be on its own line.
Additionally, this pull request includes compiled files from the dist/ directory. It's a best practice to ignore build output directories in version control. I've included dist/ in the suggestion to prevent this.
.tmptest*
node_modules/
dist/
| try { | ||
| newContent = fs.readFileSync(srcAbsolutePath, 'utf8'); | ||
| if (fileCache.has(srcAbsolutePath)) { | ||
| newContent = fileCache.get(srcAbsolutePath)!; | ||
| } else { | ||
| newContent = fs.readFileSync(srcAbsolutePath, 'utf8'); | ||
| fileCache.set(srcAbsolutePath, newContent); | ||
| } | ||
| } catch (error: any) { |
There was a problem hiding this comment.
This file-reading logic with caching is duplicated here and again in the cli function (lines 497-504). To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, it's best to extract this logic into a dedicated helper function.
For example, you could define a function like this elsewhere in the file:
function readFileWithCache(filePath: string, cache: Map<string, string>): string {
if (cache.has(filePath)) {
return cache.get(filePath)!;
}
const content = fs.readFileSync(filePath, 'utf8');
cache.set(filePath, content);
return content;
}And then you could simplify this block and the other one significantly.
💡 What:
Introduced a
fileCache(Map<string, string>) in theclifunction and passed it togenerateRichSectionMarkdown. It caches the contents of source files read during the walkthrough generation process, avoiding redundant synchronous disk reads for the same file.🎯 Why:
When generating markdown for multiple sections that might reference the same source files across many steps, the CLI previously read the same files synchronously from disk repeatedly in a loop. This caused inefficient CPU and I/O usage. Caching the file contents prevents these redundant disk I/O operations and speeds up the CLI significantly.
📊 Measured Improvement:
Measured using a benchmark where 50 sections with 500 steps each read the same dummy source file (
100lines long).generate.generate.PR created automatically by Jules for task 6921878041145294563 started by @groupthinking