#3 made the base html, css and js template#9
Merged
Merged
Conversation
Closed
3 tasks
📝 WalkthroughWalkthroughAdds a client-side visualization app and static UI to explore repository line-age: new HTML/CSS, a JavaScript Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Theseus as TheseusVisualizer
participant Manifest as Manifest (data/manifest.json)
participant RepoData as Repo Data (data/*.json)
participant SVG as SVG Renderer
participant Tooltip as Tooltip/Insights
User->>Theseus: Page load (DOMContentLoaded)
Theseus->>Manifest: fetch manifest.json
Manifest-->>Theseus: repo list
Theseus->>User: render repo selector
User->>Theseus: select repo
Theseus->>RepoData: fetch data/<repo_file>.json
RepoData-->>Theseus: snapshot dataset
Theseus->>Theseus: processData() (years, stacked geometry)
Theseus->>SVG: renderChart() (areas, gradients, axes, legend)
SVG-->>User: draw chart
User->>SVG: mouse move
SVG->>Theseus: pointer event -> find nearest snapshot
Theseus->>Tooltip: showTooltip(snapshot)
Tooltip-->>User: display totals + composition
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app.js`:
- Around line 217-225: The tooltip is being positioned with viewport coordinates
(e.clientX/e.clientY) but must use coordinates relative to the chart container;
in handleMouseMove compute container-relative coordinates using the svg bounding
rect (e.g., const rect = this.canvas.getBoundingClientRect()) and pass those to
showTooltip (for X: e.clientX - rect.left, or use the already-calculated mouseX
plus this.margin.left if showTooltip expects chart-content coordinates; for Y:
e.clientY - rect.top). Update the showTooltip calls in handleMouseMove (and the
other occurrences around the 229-233 region) to pass these container-relative
coordinates instead of raw clientX/clientY so the absolute-positioned tooltip
aligns with the chart.
- Around line 56-60: The code sets this.currentRepo before the fetch completes,
causing failed loads to leave loading state and block retries; change the flow
so you only assign this.currentRepo (and call updateActiveBtn) after the fetch
succeeds, and ensure showLoading(false) is called in both the fetch success and
catch/error paths; additionally, avoid the early-return "if (this.currentRepo
=== repoName) return" from preventing retries by either checking a separate
"loadingRepo" flag or clearing currentRepo on error so clicking the same repo
will retry (update references in the method containing currentRepo,
updateActiveBtn, showLoading).
- Around line 141-143: xScale and yScale can divide by zero for single-point or
all-zero datasets: guard (this.points.length - 1) and maxVal. Update xScale to
use a safe denominator (e.g., use Math.max(1, this.points.length - 1) or return
chartWidth/2 when this.points.length === 1) so x coordinate is valid, and update
yScale to handle maxVal === 0 by returning a baseline (e.g., chartHeight or
chartHeight/2) instead of dividing by zero; apply these changes where xScale and
yScale are defined to protect plotting for single-point or zero-valued datasets.
- Around line 269-274: The current calculation uses
Object.keys(first.composition)[0] and first.total_lines which yields an unstable
baseline and wrong denominator; instead determine the baseline year
deterministically by selecting the year key in first.composition with the
largest line count (e.g., iterate first.composition to find the year with max
value), set originalLinesInFirst = first.composition[originalYear], set
originalLinesInLast = last.composition[originalYear] || 0, guard against zero
before dividing, then compute replaced using those values and clamp as before
(update references to originalYear, originalLinesInFirst, originalLinesInLast,
replaced).
In `@data/manifest.json`:
- Around line 1-7: The manifest only lists "claude-code" but the data generation
in scripts/analyse_repository.py now produces additional datasets "react" and
"langchain"; update data/manifest.json to include entries for those datasets
(e.g., add objects for "react" with file "react_data.json" and "langchain" with
file "langchain_data.json") and provide concise descriptions so the UI selector
can surface them alongside "claude-code".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 796c5b3e-ac84-4264-a5d1-6cabbcbb0f27
📒 Files selected for processing (5)
app.jsdata/manifest.jsonindex.htmlscripts/analyse_repository.pystyle.css
Comment on lines
+269
to
+274
| const originalYear = Object.keys(first.composition)[0]; | ||
| const originalLinesInLast = last.composition[originalYear] || 0; | ||
| const originalLinesInFirst = first.total_lines; | ||
|
|
||
| const replaced = ((originalLinesInFirst - originalLinesInLast) / originalLinesInFirst) * 100; | ||
| document.getElementById('percent-replaced').textContent = `${Math.min(100, Math.max(0, replaced)).toFixed(1)}%`; |
There was a problem hiding this comment.
% replaced uses an unstable baseline and can report wrong values.
Object.keys(first.composition)[0] depends on key order, and first.total_lines is not the line count for that original year.
Suggested patch
-const originalYear = Object.keys(first.composition)[0];
-const originalLinesInLast = last.composition[originalYear] || 0;
-const originalLinesInFirst = first.total_lines;
+const compositionYears = Object.keys(first.composition).map(Number).sort((a, b) => a - b);
+if (compositionYears.length === 0) return;
+const originalYear = String(compositionYears[0]);
+const originalLinesInFirst = first.composition[originalYear] || 0;
+const originalLinesInLast = last.composition[originalYear] || 0;
+if (originalLinesInFirst === 0) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const originalYear = Object.keys(first.composition)[0]; | |
| const originalLinesInLast = last.composition[originalYear] || 0; | |
| const originalLinesInFirst = first.total_lines; | |
| const replaced = ((originalLinesInFirst - originalLinesInLast) / originalLinesInFirst) * 100; | |
| document.getElementById('percent-replaced').textContent = `${Math.min(100, Math.max(0, replaced)).toFixed(1)}%`; | |
| const compositionYears = Object.keys(first.composition).map(Number).sort((a, b) => a - b); | |
| if (compositionYears.length === 0) return; | |
| const originalYear = String(compositionYears[0]); | |
| const originalLinesInFirst = first.composition[originalYear] || 0; | |
| const originalLinesInLast = last.composition[originalYear] || 0; | |
| if (originalLinesInFirst === 0) return; | |
| const replaced = ((originalLinesInFirst - originalLinesInLast) / originalLinesInFirst) * 100; | |
| document.getElementById('percent-replaced').textContent = `${Math.min(100, Math.max(0, replaced)).toFixed(1)}%`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app.js` around lines 269 - 274, The current calculation uses
Object.keys(first.composition)[0] and first.total_lines which yields an unstable
baseline and wrong denominator; instead determine the baseline year
deterministically by selecting the year key in first.composition with the
largest line count (e.g., iterate first.composition to find the year with max
value), set originalLinesInFirst = first.composition[originalYear], set
originalLinesInLast = last.composition[originalYear] || 0, guard against zero
before dividing, then compute replaced using those values and clamp as before
(update references to originalYear, originalLinesInFirst, originalLinesInLast,
replaced).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/analyse_repository.py (1)
358-362: Nested concurrency can oversubscribe CPU/process resources.Repo-level parallelism (Line 358) plus per-repo blame pools (Line 161) can multiply active threads/subprocesses and throttle throughput on large repos. Consider capping top-level workers via env var and/or deriving from CPU count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/analyse_repository.py` around lines 358 - 362, Top-level ThreadPoolExecutor currently uses max_workers=len(TARGETS) which can oversubscribe when each repository spawns its own blame pools; change the executor creation in the code that builds the executor for TARGETS to bound max_workers by CPU or an environment variable (e.g., use min(len(TARGETS), int(os.getenv("MAX_TOP_LEVEL_WORKERS", os.cpu_count() or 1)))) so process_repository isn't started more concurrently than the machine can handle; keep the rest of the submission logic the same so futures = { executor.submit(process_repository, target, DATA_OUTPUT_DIR): target for target in TARGETS } still maps targets to futures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/analyse_repository.py`:
- Around line 320-326: The cleanup handler handle_remove_readonly is too
permissive: it blindly sets mode 0o777 and swallows all exceptions; instead,
change the chmod to only add write permission bits (use stat to read current
mode and OR with stat.S_IWUSR|stat.S_IWGRP|stat.S_IWOTH) so you don't
over-broadly open files, and replace the broad "except Exception: pass" with a
targeted PermissionError handler that either logs the failure or re-raises it
(do not silently ignore other exceptions). Apply the same change to the
duplicate handler later in the file.
- Around line 72-79: The loop in get_snapshots currently overwrites
snapshots[period] for each commit, resulting in the oldest commit being kept; to
fix, only record the first (newest) commit seen for a period instead of
overwriting—e.g., in the loop that parses log_output into commit_hash and
commit_date and computes period, change the assignment to set snapshots[period]
only if period not in snapshots (or use snapshots.setdefault(period,
commit_hash)) so that the first/newest commit per period is preserved.
- Around line 158-161: The BLAME_WORKERS env parsing can raise ValueError and
crash the run; wrap the int(os.environ.get("BLAME_WORKERS", ...)) logic used to
set max_workers in a small validation block: try to read BLAME_WORKERS, attempt
int() inside try/except ValueError and fall back to the computed default
((os.cpu_count() or 1) * 2 capped at 20) if parsing fails or if parsed value <=
0; finally clamp the value to a minimum of 1 and optionally a sensible max, then
pass that safe max_workers into ThreadPoolExecutor. Reference symbols:
BLAME_WORKERS, max_workers, ThreadPoolExecutor, and the default expression
(min(20, (os.cpu_count() or 1) * 2)).
---
Nitpick comments:
In `@scripts/analyse_repository.py`:
- Around line 358-362: Top-level ThreadPoolExecutor currently uses
max_workers=len(TARGETS) which can oversubscribe when each repository spawns its
own blame pools; change the executor creation in the code that builds the
executor for TARGETS to bound max_workers by CPU or an environment variable
(e.g., use min(len(TARGETS), int(os.getenv("MAX_TOP_LEVEL_WORKERS",
os.cpu_count() or 1)))) so process_repository isn't started more concurrently
than the machine can handle; keep the rest of the submission logic the same so
futures = { executor.submit(process_repository, target, DATA_OUTPUT_DIR): target
for target in TARGETS } still maps targets to futures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd7943cb-7fea-4861-bde2-5e3a0f597f49
📒 Files selected for processing (3)
data/langchain_data.jsondata/react_data.jsonscripts/analyse_repository.py
✅ Files skipped from review due to trivial changes (2)
- data/langchain_data.json
- data/react_data.json
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app.js (1)
91-117:⚠️ Potential issue | 🟡 MinorPrevious retry-blocking issue addressed; add guard for missing repo.
The fix correctly sets
currentRepoonly after successful fetch (line 105) and usesfinallyto always clear loading state. However,repoInfoon line 98 could beundefinedifrepoNameis not found in the manifest, causing a crash on line 99.Proposed fix
async loadRepo(repoName) { if (this.currentRepo === repoName && this.currentData) return; this.showLoading(true); this.hideError(); try { const repoInfo = this.manifest.find(r => r.name === repoName); + if (!repoInfo) throw new Error(`Unknown repository: ${repoName}`); this.repoDescription.textContent = repoInfo.description || '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app.js` around lines 91 - 117, In loadRepo, guard against a missing manifest entry: after retrieving repoInfo via this.manifest.find(...) check if repoInfo is undefined and, if so, call this.showError(`Repository "${repoName}" not found`) and return early so the code doesn't access repoInfo.description or repoInfo.file; keep the rest of the try/finally unchanged so loading state is still cleared in finally.
🧹 Nitpick comments (1)
app.js (1)
190-205: Previous single-point issue addressed; consider guarding empty datasets.The
xDenominatorfix (line 191) correctly handles single-point datasets. TheyScalefallback on line 204 handlesmaxVal === 0. However, ifthis.pointsis empty,Math.max(...[])returns-Infinity, causing downstream issues. Consider adding an early return.Proposed fix
const chartWidth = width - this.margin.left - this.margin.right; const chartHeight = height - this.margin.top - this.margin.bottom; + if (this.points.length === 0) return; + // Scales const xDenominator = Math.max(1, this.points.length - 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app.js` around lines 190 - 205, The code doesn’t guard against an empty dataset: Math.max(...this.points.map(...)) becomes -Infinity and breaks xScale/yScale; update the top of the scale setup (around xDenominator/xScale/maxVal) to check if this.points.length === 0 and return early or render an empty chart state, otherwise compute xDenominator, xScale, and maxVal as before; ensure yScale logic (yScaleMode, minLog/maxLog) only runs when maxVal is finite and > 0 to avoid division by Infinity/NaN.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app.js`:
- Around line 424-432: Compute originalLinesInFirst from
first.composition[this.years[0]] (store in originalLinesInFirst) before the
guard and change the defensive check to ensure originalLinesInFirst is nonzero
(or handle zero by setting percent to '--' or 0) instead of only checking
first.total_lines; update the block that calculates replaced to use
originalLinesInFirst and guard against division by zero so
document.getElementById('percent-replaced').textContent never receives Infinity
(references: this.years, originalYear, first, last, originalLinesInFirst,
'percent-replaced').
- Around line 366-378: The tooltip percent calculation can produce NaN when
point.total === 0; update each percent expression that uses (X / point.total) in
the tooltip (references: originalVal, refactoredVal and any other value using
point.total) to guard against division by zero by conditionally computing the
percent (e.g., if point.total > 0 then compute ((X / point.total) *
100).toFixed(1) else use "0.0") and render that string followed by "%" so the UI
shows a stable "0.0%" instead of "NaN%".
In `@scripts/analyse_repository.py`:
- Around line 372-375: The MAX_TOP_LEVEL_WORKERS environment parsing needs the
same defensive handling used for BLAME_WORKERS: replace the direct
int(os.getenv("MAX_TOP_LEVEL_WORKERS", os.cpu_count() or 1)) call used to
compute max_top_level_workers with the same pattern that reads the env var as a
string, attempts to int() it in a try/except, falls back to os.cpu_count() (or
1) on ValueError or missing value, and logs or warns about invalid input; update
the assignment of max_top_level_workers (which currently uses TARGETS and
int(...)) to use this validated parsed value.
---
Duplicate comments:
In `@app.js`:
- Around line 91-117: In loadRepo, guard against a missing manifest entry: after
retrieving repoInfo via this.manifest.find(...) check if repoInfo is undefined
and, if so, call this.showError(`Repository "${repoName}" not found`) and return
early so the code doesn't access repoInfo.description or repoInfo.file; keep the
rest of the try/finally unchanged so loading state is still cleared in finally.
---
Nitpick comments:
In `@app.js`:
- Around line 190-205: The code doesn’t guard against an empty dataset:
Math.max(...this.points.map(...)) becomes -Infinity and breaks xScale/yScale;
update the top of the scale setup (around xDenominator/xScale/maxVal) to check
if this.points.length === 0 and return early or render an empty chart state,
otherwise compute xDenominator, xScale, and maxVal as before; ensure yScale
logic (yScaleMode, minLog/maxLog) only runs when maxVal is finite and > 0 to
avoid division by Infinity/NaN.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 430a844b-02ad-4db3-a606-b92bac3e8343
📒 Files selected for processing (6)
.gitignoreapp.jsdata/manifest.jsonindex.htmlscripts/analyse_repository.pystyle.css
✅ Files skipped from review due to trivial changes (3)
- data/manifest.json
- index.html
- style.css
Comment on lines
+366
to
+378
| <div class="value-group"> | ||
| <strong>${originalVal.toLocaleString()}</strong> | ||
| <span class="percent-tag">${((originalVal / point.total) * 100).toFixed(1)}%</span> | ||
| </div> | ||
| </div> | ||
| <div class="tooltip-item"> | ||
| <div class="label-group"> | ||
| <span class="color-dot" style="background: #f0a33b"></span> | ||
| <span>Refactored</span> | ||
| </div> | ||
| <div class="value-group"> | ||
| <strong>${refactoredVal.toLocaleString()}</strong> | ||
| <span class="percent-tag">${((refactoredVal / point.total) * 100).toFixed(1)}%</span> |
There was a problem hiding this comment.
Tooltip percentages can show NaN% when point.total is zero.
Lines 368, 378, and 397 divide by point.total. If total is 0, this displays NaN%.
Proposed fix
- <span class="percent-tag">${((originalVal / point.total) * 100).toFixed(1)}%</span>
+ <span class="percent-tag">${point.total > 0 ? ((originalVal / point.total) * 100).toFixed(1) : 0}%</span>Apply similar guards to lines 378 and 397.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="value-group"> | |
| <strong>${originalVal.toLocaleString()}</strong> | |
| <span class="percent-tag">${((originalVal / point.total) * 100).toFixed(1)}%</span> | |
| </div> | |
| </div> | |
| <div class="tooltip-item"> | |
| <div class="label-group"> | |
| <span class="color-dot" style="background: #f0a33b"></span> | |
| <span>Refactored</span> | |
| </div> | |
| <div class="value-group"> | |
| <strong>${refactoredVal.toLocaleString()}</strong> | |
| <span class="percent-tag">${((refactoredVal / point.total) * 100).toFixed(1)}%</span> | |
| <div class="value-group"> | |
| <strong>${originalVal.toLocaleString()}</strong> | |
| <span class="percent-tag">${point.total > 0 ? ((originalVal / point.total) * 100).toFixed(1) : 0}%</span> | |
| </div> | |
| </div> | |
| <div class="tooltip-item"> | |
| <div class="label-group"> | |
| <span class="color-dot" style="background: `#f0a33b`"></span> | |
| <span>Refactored</span> | |
| </div> | |
| <div class="value-group"> | |
| <strong>${refactoredVal.toLocaleString()}</strong> | |
| <span class="percent-tag">${point.total > 0 ? ((refactoredVal / point.total) * 100).toFixed(1) : 0}%</span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app.js` around lines 366 - 378, The tooltip percent calculation can produce
NaN when point.total === 0; update each percent expression that uses (X /
point.total) in the tooltip (references: originalVal, refactoredVal and any
other value using point.total) to guard against division by zero by
conditionally computing the percent (e.g., if point.total > 0 then compute ((X /
point.total) * 100).toFixed(1) else use "0.0") and render that string followed
by "%" so the UI shows a stable "0.0%" instead of "NaN%".
Comment on lines
+424
to
+432
| let originalYear = this.years[0]; | ||
| if (!originalYear || first.total_lines === 0) { | ||
| document.getElementById('percent-replaced').textContent = '--'; | ||
| } else { | ||
| const originalLinesInFirst = first.composition[originalYear] || 0; | ||
| const originalLinesInLast = last.composition[originalYear] || 0; | ||
| const replaced = ((originalLinesInFirst - originalLinesInLast) / originalLinesInFirst) * 100; | ||
| document.getElementById('percent-replaced').textContent = `${Math.min(100, Math.max(0, replaced)).toFixed(1)}%`; | ||
| } |
There was a problem hiding this comment.
Previous baseline stability issue addressed; guard mismatch remains.
Using this.years[0] (sorted) fixes the unstable baseline. However, the guard on line 425 checks first.total_lines === 0, but line 430 divides by originalLinesInFirst. If the original year has 0 lines but total_lines > 0, division by zero produces Infinity.
Proposed fix
let originalYear = this.years[0];
- if (!originalYear || first.total_lines === 0) {
+ const originalLinesInFirst = first.composition[originalYear] || 0;
+ if (!originalYear || originalLinesInFirst === 0) {
document.getElementById('percent-replaced').textContent = '--';
} else {
- const originalLinesInFirst = first.composition[originalYear] || 0;
const originalLinesInLast = last.composition[originalYear] || 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let originalYear = this.years[0]; | |
| if (!originalYear || first.total_lines === 0) { | |
| document.getElementById('percent-replaced').textContent = '--'; | |
| } else { | |
| const originalLinesInFirst = first.composition[originalYear] || 0; | |
| const originalLinesInLast = last.composition[originalYear] || 0; | |
| const replaced = ((originalLinesInFirst - originalLinesInLast) / originalLinesInFirst) * 100; | |
| document.getElementById('percent-replaced').textContent = `${Math.min(100, Math.max(0, replaced)).toFixed(1)}%`; | |
| } | |
| let originalYear = this.years[0]; | |
| const originalLinesInFirst = first.composition[originalYear] || 0; | |
| if (!originalYear || originalLinesInFirst === 0) { | |
| document.getElementById('percent-replaced').textContent = '--'; | |
| } else { | |
| const originalLinesInLast = last.composition[originalYear] || 0; | |
| const replaced = ((originalLinesInFirst - originalLinesInLast) / originalLinesInFirst) * 100; | |
| document.getElementById('percent-replaced').textContent = `${Math.min(100, Math.max(0, replaced)).toFixed(1)}%`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app.js` around lines 424 - 432, Compute originalLinesInFirst from
first.composition[this.years[0]] (store in originalLinesInFirst) before the
guard and change the defensive check to ensure originalLinesInFirst is nonzero
(or handle zero by setting percent to '--' or 0) instead of only checking
first.total_lines; update the block that calculates replaced to use
originalLinesInFirst and guard against division by zero so
document.getElementById('percent-replaced').textContent never receives Infinity
(references: this.years, originalYear, first, last, originalLinesInFirst,
'percent-replaced').
Comment on lines
+372
to
375
| # Bound top-level workers by CPU count | ||
| max_top_level_workers = min( | ||
| len(TARGETS), int(os.getenv("MAX_TOP_LEVEL_WORKERS", os.cpu_count() or 1)) | ||
| ) |
There was a problem hiding this comment.
MAX_TOP_LEVEL_WORKERS parsing lacks the same validation applied to BLAME_WORKERS.
If MAX_TOP_LEVEL_WORKERS is set to a non-numeric value (e.g., "abc"), int() will raise ValueError and crash the script. Apply the same defensive parsing pattern used for BLAME_WORKERS (lines 161-172).
Proposed fix
- # Bound top-level workers by CPU count
- max_top_level_workers = min(
- len(TARGETS), int(os.getenv("MAX_TOP_LEVEL_WORKERS", os.cpu_count() or 1))
- )
+ # Bound top-level workers by CPU count
+ default_top_workers = os.cpu_count() or 1
+ raw_top_workers = os.getenv("MAX_TOP_LEVEL_WORKERS")
+ try:
+ parsed_top_workers = int(raw_top_workers) if raw_top_workers else default_top_workers
+ except ValueError:
+ logger.warning("Invalid MAX_TOP_LEVEL_WORKERS=%r, falling back to %d", raw_top_workers, default_top_workers)
+ parsed_top_workers = default_top_workers
+ max_top_level_workers = min(len(TARGETS), max(1, parsed_top_workers))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Bound top-level workers by CPU count | |
| max_top_level_workers = min( | |
| len(TARGETS), int(os.getenv("MAX_TOP_LEVEL_WORKERS", os.cpu_count() or 1)) | |
| ) | |
| # Bound top-level workers by CPU count | |
| default_top_workers = os.cpu_count() or 1 | |
| raw_top_workers = os.getenv("MAX_TOP_LEVEL_WORKERS") | |
| try: | |
| parsed_top_workers = int(raw_top_workers) if raw_top_workers else default_top_workers | |
| except ValueError: | |
| logger.warning("Invalid MAX_TOP_LEVEL_WORKERS=%r, falling back to %d", raw_top_workers, default_top_workers) | |
| parsed_top_workers = default_top_workers | |
| max_top_level_workers = min(len(TARGETS), max(1, parsed_top_workers)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/analyse_repository.py` around lines 372 - 375, The
MAX_TOP_LEVEL_WORKERS environment parsing needs the same defensive handling used
for BLAME_WORKERS: replace the direct int(os.getenv("MAX_TOP_LEVEL_WORKERS",
os.cpu_count() or 1)) call used to compute max_top_level_workers with the same
pattern that reads the env var as a string, attempts to int() it in a
try/except, falls back to os.cpu_count() (or 1) on ValueError or missing value,
and logs or warns about invalid input; update the assignment of
max_top_level_workers (which currently uses TARGETS and int(...)) to use this
validated parsed value.
This was referenced Apr 5, 2026
This was referenced May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores