fix(frontend): replace local notifications with global toast - #328
fix(frontend): replace local notifications with global toast#328prachishelke1312 wants to merge 2 commits into
Conversation
|
🎉 Thank you @prachishelke1312 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. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
left a comment
There was a problem hiding this comment.
PR #328 — Code Review: "SQLite Persistence for Scan Findings"
Verdict: ⛔ CHANGES REQUESTED
Critical Problem: Wrong PR, Wrong Branch
This is the most important finding of the review. The PR description is completely wrong.
| Attribute | PR Description Claims | Reality |
|---|---|---|
| Title | SQLite persistence for scan findings | Global toast notification fix |
| Branch | (implied SQLite) | fix-global-toast |
| Backend changes | SQLite integration, CRUD helpers | None (from this PR's author) |
| Frontend changes | "No frontend changes" | 100% of the actual changes |
| Linked issue | #38 | Unrelated |
The branch fix-global-toast contains exactly one original commit from this author:
464c954 fix(frontend): replace local notifications with global toast
The remaining ~3,100 line diff shown by git diff main...HEAD is purely merge noise — it's a snapshot of every other PR that was merged into main while this branch's merge base has drifted. The SQLite work (538c97f Merge pull request #246, 118fca0 Merge pull request #239) was authored by a completely different contributor (Trishanthsai) and was already merged into main weeks ago.
What the PR Actually Does
The single real commit (464c954) makes a small, clean frontend-only fix:
Changes in frontend/src/app/pages/dashboard.tsx
- Removes local
scanErrorstate (useState<string | null>(null)) - Replaces inline
<p className="text-sm text-destructive">error display withtoast.error(...)calls - Replaces post-scan navigation with
toast.success("Scan completed successfully!")
Changes in frontend/src/app/App.tsx
- Adds a global
<Toaster />component fromsonnerso toast messages render across all pages
Changes in frontend/src/app/components/ui/sonner.tsx
- Fixes TypeScript type: replaces
ToasterProps["theme"]with the inline literal"light" | "dark" | "system" - Uses
React.ComponentProps<typeof Sonner>instead of the removedToasterPropsexport
Changes in package.json / package-lock.json
- Pins
sonnerfrom exact"2.0.3"→"^2.0.3"(semver range)
Issues with the Actual Code Changes
1. sonner Version Unpinned (Minor)
-"sonner": "2.0.3",
+"sonner": "^2.0.3",Changing from a pinned to a range version is fine for flexibility but was not discussed. Not a blocker, but worth noting as an intentional choice.
2. No Newline at End of File
Both App.tsx and sonner.tsx lost their trailing newline:
-export { Toaster };
+export { Toaster };
\ No newline at end of file
This is a cosmetic issue but will cause eslint and prettier warnings. Should be fixed.
3. Missing Toast for Success on URL Import
The handleScanSuccess function now fires a toast.success after a ZIP scan, but there is no equivalent success toast after handleImportFromUrl succeeds — it just navigates silently. This is an inconsistency.
// ZIP scan (✅ has success toast)
toast.success("Scan completed successfully!");
navigate("/findings");
// URL import (❌ no success toast — navigates silently)
handleScanSuccess(scan); // <-- this calls navigate but not toast.success againWait — actually handleScanSuccess is called by both, so the toast.success is in handleScanSuccess and fires for both paths. This is fine. ✅
4. Toaster Placed Inside ThemeProvider But Relies on next-themes
sonner.tsx uses useTheme() from next-themes. This hook requires a ThemeProvider ancestor. <Toaster /> is correctly placed inside <ThemeProvider>, so this is fine. ✅
Mandatory Requests Before Approval
| # | Issue | Severity |
|---|---|---|
| 1 | Rewrite the PR description entirely — it describes SQLite work that is unrelated and already merged | 🔴 Blocker |
| 2 | Close or re-target the linked issue — #38 (SQLite persistence) is already resolved; this PR has nothing to do with it | 🔴 Blocker |
| 3 | Rebase onto current main — the branch diverged massively; PR must be rebased so the diff only shows the 15-line actual change |
🔴 Blocker |
| 4 | Fix trailing newline missing from App.tsx and sonner.tsx |
🟡 Minor |
Summary
The actual frontend fix (global toast over local error state) is clean and correct. The implementation is sound. However, the PR cannot be approved in its current state because:
- The description is entirely fabricated — it describes SQLite work done by a different contributor.
- The diff against
mainshows 3,000+ unrelated lines due to an unrebased branch, making review impossible. - The linked issue (#38) is already resolved and should not be associated here.
Once the description is corrected, the branch is rebased, and the linked issue is fixed, this PR should be a quick, clean approval.
|
@prachishelke1312 , Have you made the changes as i cannot see it! |
Linked issue
Closes #38
What this PR does
Adds a SQLite persistence layer for scan findings. Previously, findings were stored only in memory and were lost after server restarts. This PR introduces SQLite integration, a findings schema, CRUD helper functions, and persistent storage keyed by
job_id.Type of change
ML tier (if applicable)
Stack affected
Changes
Backend
job_id.Frontend
New dependencies
Database / schema changes
Testing
How did you test this?
job_id.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 SQLite schema, CRUD implementation, and integration with the scan workflow to ensure findings are persisted and retrieved correctly.
Screenshots (if UI changed)
N/A (Backend-only changes)