perf: Reduce Full-Page Re-renders Triggered by Server-Sent Event (SSE Updates)#300
perf: Reduce Full-Page Re-renders Triggered by Server-Sent Event (SSE Updates)#300Aditya8369 wants to merge 1 commit into
Conversation
|
🎉 Thank you @Aditya8369 for submitting a Pull Request! We're excited to review your contribution. Before Review✅ Ensure all CI checks pass ⚡ Want faster reviews and contributor support? Join our Discord community: 🔗 https://discord.gg/FcXuyw2Rs Maintainers and mentors are active there and can help resolve blockers quickly. Happy Contributing! 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
left a comment
There was a problem hiding this comment.
PR #300 Review — SSE Re-render Optimization
Verdict: 🔴 Request Changes
1. Massive Scope Creep (Critical Blocker)
The PR description says:
"Refactored
useSingleScan.tsanduseOrganizationScan.tsto remove high-frequency SSE message listeners… They now only manage the static scan metadata."
"Frontend changes: None."
What the diff actually shows: 48 files changed, 3121 insertions, 1473 deletions.
The two hook files are genuinely new and correctly scoped. But bundled into this PR are:
| Category | Files Changed |
|---|---|
| New frontend components | ActiveOrgScanModal.tsx, ActiveSingleScanModal.tsx, ExportReportButton.tsx, OrganizationScanDialog.tsx, RecentScans.tsx, UrlImportDialog.tsx, app-sidebar.tsx, fix-confidence.tsx, split-text.tsx, useDragAndDrop.ts |
| Significant page rewrites | dashboard.tsx (−350 lines), fix.tsx, findings.tsx, root.tsx, landing.tsx |
| New/changed backend files | main.py (+776 lines), db.py, security.py, models.py, remediation/engine.py, reports/evidence_pack.py, utils/fs.py, ml/deduplicator.py |
| New backend tests | test_dependency_diff.py, test_fix_telemetry.py, test_safe_job_dir.py, test_security.py, test_org_scans.py |
| Docs & config | CONTRIBUTING.md, README.md, docs/ML_ARCHITECTURE.md, .github/pull_request_template.md, .github/workflows/pr-guardrails.yml |
These are changes from multiple other merged/open PRs (API auth from #295, org scans from #257, zip-slip fix from #297, etc.) rebased into a single branch. This PR is a monolith dump, not a focused refactor.
2. The Stated Optimization Is Incomplete / Misleading
The PR title claims SSE listeners were removed from the hooks. This is technically true for the two hook files themselves — but the SSE listeners were simply moved into two new modal components, not eliminated:
ActiveSingleScanModal.tsx — SSE still present (line 30)
const sse = new EventSource(`${API_BASE}/api/scans/${scanId}/stream`);ActiveOrgScanModal.tsx — SSE still present (line 29)
const sse = new EventSource(`${API_BASE}/api/scans/org/${orgJobId}/stream`);Moving SSE listeners from a hook to a modal component is an architectural refactor, not a "removal of high-frequency listeners." The full-page re-render problem the PR title promises to fix is partially addressed by isolating these listeners into dedicated modal components (so only the modal re-renders, not the entire Dashboard), but:
- The PR description is factually wrong — it claims the listeners were removed, not moved.
- The described optimization works only if the modal is not rendered inside the
Dashboardparent's render tree in a way that still cascades re-renders — this is not verified with any test.
3. Runtime Bug in ActiveSingleScanModal.tsx
Line 80: The JSX references scanState — a variable that does not exist in scope. The hook uses singleScanState (line 21), but the entire render block (lines 80–137) references scanState.*.
// Line 21 — actual variable:
const [singleScanState, setSingleScanState] = useState<any>(null);
// Lines 80–137 — UNDEFINED reference:
<Loader2 className={cn("...", scanState.status === "running" && "animate-spin")} />
// scanState.sast, scanState.dependency, scanState.secrets, scanState.status...This will crash at runtime with ReferenceError: scanState is not defined the moment a scan is initiated. This is a P0 bug.
Caution
The author claims "All files built perfectly with zero warnings or errors." This is only possible if the build did not include TypeScript strict checks, or if this file was excluded from the Vite build being referenced. A tsc --noEmit run would surface this immediately.
4. useOrganizationScan Has a State Leak
In useOrganizationScan.ts, orgScanLoading is set to true on scan start but never cleared on success — only on error. After handleScanOrg succeeds, orgScanLoading stays true forever until resetOrgScan() is called externally.
setOrgScanLoading(true);
try {
const data = await scanOrganization(trimmedUrl);
setActiveOrgJobId(data.org_job_id);
setExpectedRepoCount(data.repo_count);
// ❌ setOrgScanLoading(false) is missing here
if (onSuccessCallback) onSuccessCallback();
} catch (e: any) {
setOrgScanError(e?.message ?? "Organization batch scan failed");
setOrgScanLoading(false); // Only cleared on error
}Compare with useSingleScan.ts where the same pattern exists: setScanLoading is also never set to false on success. This means any button tied to orgScanLoading / scanLoading will appear stuck in a loading state after a successful dispatch.
5. ActiveOrgScanModal Stores SSE Connection in State (Anti-Pattern)
const [sseConnection, setSseConnection] = useState<EventSource | null>(null);Storing a mutable, non-serializable EventSource instance in React state is an anti-pattern. Every setSseConnection() call triggers a re-render. This should be a useRef. The existing cleanup in the useEffect return already handles closure correctly without needing state.
6. useRecentJobs Has a Stale Closure Issue
In useRecentJobs.ts:
const handleScanSuccess = useCallback((scan) => {
// ...
const currentJobs = getLocalRecentJobs(); // Reads fresh from localStorage ✓
const next = [...].slice(0, 10);
localStorage.setItem(RECENTS_KEY, JSON.stringify(next));
setRecentJobs(next); // ✓
}, [navigate]);This pattern is fine. However, onClearRecents and handleScanSuccess both mutate localStorage directly rather than going through the React state setter with a function form. If two components call these simultaneously (unlikely but possible), there's a race. Minor issue, not a blocker.
7. Missing Newline at End of Both Hook Files
Both useSingleScan.ts and useOrganizationScan.ts are missing a trailing newline (\ No newline at end of file in the diff). This is a minor style issue but will cause lint warnings in most CI setups.
Summary Table
| # | Issue | Severity |
|---|---|---|
| 1 | 48-file monolith diff contradicts PR description | 🔴 Critical — Split PR |
| 2 | scanState undefined in ActiveSingleScanModal JSX |
🔴 Runtime crash |
| 3 | orgScanLoading / scanLoading never cleared on success |
🟠 High — broken UX |
| 4 | SSE listeners moved, not removed — description is wrong | 🟠 High — misleading |
| 5 | EventSource stored in useState instead of useRef |
🟡 Medium — anti-pattern |
| 6 | Build claim suspect (tsc would catch scanState) |
🟡 Medium |
| 7 | Missing EOF newline in hook files | 🟢 Low |
Required Changes Before Approval
- Split this PR. Extract only the
useSingleScan.ts,useOrganizationScan.ts,ActiveSingleScanModal.tsx,ActiveOrgScanModal.tsx, and minimaldashboard.tsxwiring into PR #300. All other backend and unrelated frontend changes belong in separate PRs. - Fix the
scanStateundefined bug inActiveSingleScanModal.tsx— rename all usages tosingleScanState. - Add
setScanLoading(false)/setOrgScanLoading(false)in the success path of both hooks. - Correct the PR description — SSE listeners were moved into isolated components, not removed. This is still a valid optimization (prevents Dashboard parent re-renders), but it must be described accurately.
- Replace
useState<EventSource>withuseRef<EventSource>inActiveOrgScanModal.tsx. - Add trailing newlines to hook files.
Linked issue
Closes #248
What this PR does
successfully completed the optimization task to reduce full-page re-renders triggered by SSE updates
Type of change
ML tier (if applicable)
Stack affected
Changes
Backend
Frontend
New dependencies
Database / schema changes
Testing
How did you test this?
Checklist
console.erroror unhandled Python exceptions introducedrequirements.txt/package.jsonupdated if new dependencies added.pkl,.pt, etc.) are gitignored, not committedAnything reviewers should focus on
Please review the PR
Screenshots (if UI changed)
N/A (Backend-only changes)