refactor(renderer): isolate generated outputs from src tree#128
Conversation
Reviewer's GuideRefactors the renderer build pipeline so that all generated JS/CSS assets live in dist/renderer instead of src/renderer, updates dev/QA/release tooling to build and serve from the new location, keeps packaging working, and simplifies static-analysis exclusions; also hardens a QA screenshot flow by making checkbox toggling more robust. Sequence diagram for robust checkbox toggling in QA screenshot flowsequenceDiagram
actor QA_Job
participant Script as capture_ui_screenshot_js
participant Page
participant Checkbox as Secret_and_Suspicious_Toggles
QA_Job->>Script: npm run qa:screenshot
Script->>Page: open renderer UI
Script->>Page: click config tab
Script->>Page: waitForSelector(secretScanningToggle)
loop for each toggle
Script->>Page: setCheckboxState(selector, false)
activate Page
Page->>Checkbox: locator(selector).first()
Page-->>Script: waitFor visible
loop up to 3 attempts
Script->>Page: isChecked()
Page-->>Script: currentState
alt currentState != false
Script->>Page: uncheck()
Script->>Page: waitForTimeout(75ms)
else currentState == false
Script-->>Page: return
Script-->>Page: break
end
end
alt still not false after attempts
Page-->>Script: throw Error("Unable to set checkbox")
end
deactivate Page
end
Script->>Page: waitForFunction(both toggles unchecked in DOM)
Script->>Page: click Save config button
Page-->>Script: config persisted (localStorage)
Script-->>QA_Job: screenshots captured successfully
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoIsolate renderer build artifacts from source tree
WalkthroughsDescription• Move renderer build outputs from src/renderer to dist/renderer • Update webpack, tailwind, and build pipelines to consume assets from new location • Remove tracked generated artifact src/renderer/index.js.LICENSE.txt • Clean stale generated-file exclusions from Sonar and CodeQL configs • Improve screenshot capture script with robust checkbox state handling Diagramflowchart LR
A["Renderer Source<br/>src/renderer"] -->|"Webpack builds"| B["Renderer Build Output<br/>dist/renderer"]
C["Build Scripts<br/>webpack, tailwind, dev"] -->|"Updated paths"| B
D["Config Files<br/>Sonar, CodeQL"] -->|"Cleaned exclusions"| E["Simplified Config"]
F["HTML Entry Point<br/>index.html"] -->|"Updated links"| B
File Changes1. scripts/capture-ui-screenshot.js
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR reorganizes the build output structure by migrating compiled renderer assets from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Mehdi-Bl, 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 refactors the project's build output strategy by isolating all generated renderer assets into a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
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
|
![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:
- The renderer asset path
dist/rendereris now hard-coded in multiple places (scripts, package.json, webpack config); consider centralizing this in a single config/constant to avoid future drift if the path changes again. - In
src/renderer/index.html, the../../dist/renderer/...URLs are somewhat brittle; switching to root-relative paths like/dist/renderer/output.cssand/dist/renderer/bundle.jswould be easier to reason about and less sensitive to where the HTML is served from.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The renderer asset path `dist/renderer` is now hard-coded in multiple places (scripts, package.json, webpack config); consider centralizing this in a single config/constant to avoid future drift if the path changes again.
- In `src/renderer/index.html`, the `../../dist/renderer/...` URLs are somewhat brittle; switching to root-relative paths like `/dist/renderer/output.css` and `/dist/renderer/bundle.js` would be easier to reason about and less sensitive to where the HTML is served from.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
![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 effectively refactors the project to isolate generated renderer assets into the dist/renderer directory, which is a good step for improving the project structure and maintainability. All related build and development scripts have been updated accordingly. Additionally, the QA scripts have been improved for better reliability. My review includes one suggestion to simplify a cleanup script by removing redundant path definitions.
| path.join(ROOT_DIR, 'dist', 'renderer', 'bundle.js'), | ||
| path.join(ROOT_DIR, 'dist', 'renderer', 'bundle.js.map'), | ||
| path.join(ROOT_DIR, 'build', 'ts'), | ||
| path.join(ROOT_DIR, 'src', 'renderer', 'output.css'), | ||
| path.join(ROOT_DIR, 'dist', 'renderer', 'output.css'), |
There was a problem hiding this comment.
The pathsToRemove array contains redundant paths. Since path.join(ROOT_DIR, 'dist') is already being removed recursively on line 107, explicitly listing files within the dist/renderer directory is not necessary. You can simplify this array to make the code cleaner and easier to maintain.
| path.join(ROOT_DIR, 'dist', 'renderer', 'bundle.js'), | |
| path.join(ROOT_DIR, 'dist', 'renderer', 'bundle.js.map'), | |
| path.join(ROOT_DIR, 'build', 'ts'), | |
| path.join(ROOT_DIR, 'src', 'renderer', 'output.css'), | |
| path.join(ROOT_DIR, 'dist', 'renderer', 'output.css'), | |
| path.join(ROOT_DIR, 'build', 'ts'), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
50-50:⚠️ Potential issue | 🟠 Major
predocs:screenshotsis missingbuild:css, unlikepreqa:screenshotandpree2e:playwright.CSS output is built to
dist/renderer/output.css, and without explicitly building it, doc screenshots will render without styles. Bothpreqa:screenshot(line 44) andpree2e:playwright(line 46) includebuild:cssin their pre-scripts, butpredocs:screenshots(line 50) does not.🐛 Proposed fix
- "predocs:screenshots": "npm run build:ts && npm run build:webpack", + "predocs:screenshots": "npm run build:ts && npm run build:css && npm run build:webpack",
🤖 Fix all issues with AI agents
In `@scripts/lib/dev.js`:
- Around line 32-45: The tailwind execSync call can fail if the dist/renderer
directory is missing; before running execSync (the code that references execSync
and cssFile), ensure the output directory exists by calling
utils.ensureDir(path.join(utils.ROOT_DIR, 'dist', 'renderer')) or, preferably,
replace the direct execSync invocation with utils.runNpmScript('build:css') so
the prebuild hook that creates build dirs runs; update the branch that logs
"Running tailwindcss directly..." and use the chosen approach (ensureDir or
runNpmScript) to guarantee the directory exists before invoking tailwind.
🧹 Nitpick comments (1)
scripts/lib/utils.js (1)
106-112: Redundant entries: individual files underdist/are already removed by thedistdirectory deletion.Line 107 removes the entire
distdirectory recursively. Lines 108–109 and 111 then attempt to delete individual files withindist/renderer/, but they will never exist at that point. Consider removing the redundant entries for clarity.♻️ Suggested cleanup
const pathsToRemove = [ path.join(ROOT_DIR, 'dist'), - path.join(ROOT_DIR, 'dist', 'renderer', 'bundle.js'), - path.join(ROOT_DIR, 'dist', 'renderer', 'bundle.js.map'), path.join(ROOT_DIR, 'build', 'ts'), - path.join(ROOT_DIR, 'dist', 'renderer', 'output.css'), ];
Code Review by Qodo
1. dev.js missing dist directory
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| const cssFile = path.join(utils.ROOT_DIR, 'dist', 'renderer', 'output.css'); | ||
| if (!fs.existsSync(cssFile)) { | ||
| log('CSS not found, building...', colors.yellow); | ||
| try { | ||
| // Try direct command execution first | ||
| const { execSync } = require('child_process'); | ||
| log('Running tailwindcss directly...', colors.blue); | ||
| execSync('npx tailwindcss -i ./src/renderer/styles.css -o ./src/renderer/output.css', { | ||
| stdio: 'inherit', | ||
| cwd: utils.ROOT_DIR, | ||
| }); | ||
| execSync( | ||
| 'npx @tailwindcss/cli -i ./src/renderer/styles.css -o ./dist/renderer/output.css', | ||
| { | ||
| stdio: 'inherit', | ||
| cwd: utils.ROOT_DIR, | ||
| } | ||
| ); |
There was a problem hiding this comment.
1. dev.js missing dist directory 📘 Rule violation ⛯ Reliability
scripts/lib/dev.js now writes Tailwind output to dist/renderer/output.css but does not ensure dist/renderer exists first, which can cause dev startup to fail on a clean checkout. This is an unhandled dependency/edge case for a key dev workflow.
Agent Prompt
## Issue description
`scripts/lib/dev.js` builds Tailwind output into `dist/renderer/output.css` via a direct CLI call, but it does not ensure `dist/renderer` exists first. Since `dist` is gitignored and may not exist on clean checkouts, the Tailwind write can fail and prevent `npm run dev` from starting.
## Issue Context
The repo already has `scripts/ensure-build-dirs.js` and wiring via `prebuild:css`/`prewatch:css`, but `dev.js` bypasses that by calling Tailwind directly.
## Fix Focus Areas
- scripts/lib/dev.js[32-45]
- package.json[37-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d4b28ce46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| execSync( | ||
| 'npx @tailwindcss/cli -i ./src/renderer/styles.css -o ./dist/renderer/output.css', |
There was a problem hiding this comment.
The dev bootstrap now executes Tailwind directly to ./dist/renderer/output.css, but this bypasses the new prebuild:css setup that creates dist/renderer first. In a clean workspace (or after removing dist), npm run dev can fail before watchers start because this write target may not exist yet; using npm run build:css here (or explicitly ensuring the directory) would keep startup consistent with the rest of the build pipeline.
Useful? React with 👍 / 👎.
|
1 participant
Summary
bundle.js, sourcemap, license output,output.css) fromsrc/renderertodist/rendererdist/renderersrc/renderer/index.js.LICENSE.txtdist/renderer/**/*in electron-builder filesBacklog
P0 Renderer cleanup 1/4: isolate build artifacts from src treePVTI_lADODAMERs4A0Spmzgld4wkValidation
npm run lintnpm test -- --runInBandnpm run qa:screenshotmake sonarAll passed locally.
Summary by Sourcery
Isolate renderer build artifacts under dist/renderer and update tooling, packaging, and analysis configs to consume assets from the new location.
Enhancements:
Summary by CodeRabbit