Skip to content

feat(frontend+backend): add scan history diff view#366

Closed
KBarathraj wants to merge 5 commits into
utksh1:mainfrom
KBarathraj:feat/scan-history-diff-view
Closed

feat(frontend+backend): add scan history diff view#366
KBarathraj wants to merge 5 commits into
utksh1:mainfrom
KBarathraj:feat/scan-history-diff-view

Conversation

@KBarathraj

Copy link
Copy Markdown
Contributor

Description

Adds a Compare Scans feature. Users can select two historical scans of the same target and see findings categorized as new, fixed, unchanged, or severity changed. Helps track remediation progress over time.

Related Issues

Closes #336

issue name : Add Scan History Diff View for Comparing Historical Scan Results

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • ./testing/test_python.sh :- 9/9 passing (9 new tests for diff_service)
  • cd frontend && npm run test :- passing
  • cd frontend && npm run build :- zero TypeScript errors, zero new warnings
  • Manual: seeded two scans, verified 200/400/404/422 responses via Swagger, verified all four diff categories render correctly in UI

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

Screenshots of changes

Before:
Screenshot 2026-05-28 100604
After :
Screenshot 2026-05-28 100839

Comparing scans:
Screenshot 2026-05-28 094613

@utksh1 utksh1 added area:backend Backend API, database, or service work area:frontend Frontend React/UI work type:feature Feature work category bonus label type:testing Testing work category bonus label level:advanced 55 pts difficulty label for advanced contributor PRs labels May 28, 2026

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes before merge. The scan diff feature is promising, but there are correctness issues to clean up: the frontend type has a duplicated 'before' property in SeverityChangePair, the diff fingerprint assumes every finding has title/category/target and can crash on malformed parser output, and the UI/API path needs regression coverage for missing fields and same-scan/different-target cases. Please tighten those before review.

@utksh1

utksh1 commented May 28, 2026

Copy link
Copy Markdown
Owner

Thanks for following up. Clarifying the change request so it is actionable:

Why this is blocked:
Requesting changes before merge. The scan diff feature is promising, but there are correctness issues to clean up: the frontend type has a duplicated 'before' property in SeverityChangePair, the diff fingerprint assumes every finding has title/category/target and can crash on malformed parser output, and the UI/API path needs regression coverage for missing fields and same-scan/different-target cases. Please tighten those before review.

What to do next:

  • Fix the specific issues called out above.
  • Push the updated branch and make sure the relevant CI checks pass.
  • Reply here when ready for re-review.

@KBarathraj KBarathraj force-pushed the feat/scan-history-diff-view branch 2 times, most recently from 00c2551 to 173e762 Compare May 29, 2026 18:14
@KBarathraj

Copy link
Copy Markdown
Contributor Author

Hey bro @utksh1, all three points have been addressed and all 6 CI checks are now passing.

SeverityChangePair : conformed clean
fingerprint() and _parse_findings() hardened with .get() fallbacks and null-byte separator
Route level integration tests added for same scan 400, different target 400, missing scan 404, and valid 200

32 tests passing total. please check it out man

@utksh1

utksh1 commented May 30, 2026

Copy link
Copy Markdown
Owner

Re-reviewed after the latest push. Still blocked: please fix the duplicated before field in the frontend severity-change type, make diff fingerprinting tolerate missing finding fields without crashing, and add regression coverage for missing fields plus same-scan/different-target cases.

@KBarathraj

Copy link
Copy Markdown
Contributor Author

Hi @utksh1, I've now done a complete line by line go through of every file in the commit and cannot reproduce any of the three issues you mentioned. Could you please help me understand exactly what you're seeing?
Specifically:
Which file and line number shows the duplicate before property?
Which exact line in fingerprint() or compute_diff() do you think can crash?
Which test case do you feel is missing?

I ask this because I've verified directly from git:

SeverityChangePair has always had exactly before: Finding and after: Finding
fingerprint() uses .get() with fallbacks on all three fields.
34 tests now cover missing fields, None values, same-scan 400, different-target 400, asymmetric 404 cases

A screenshot or direct quote from the Files Changed tab would help me address your concern precisely. I genuinely want to fix issue that the commit has. I just can't find it without knowing where to look.

@utksh1

utksh1 commented May 31, 2026

Copy link
Copy Markdown
Owner

Re-reviewed after the latest push. Still blocked: please fix the duplicated frontend severity-change field, make diff fingerprinting tolerate missing title/category/target without crashing, and add regressions for missing fields plus same-scan/different-target cases.

@KBarathraj

Copy link
Copy Markdown
Contributor Author

As stated previously,Please be more specific

@KBarathraj KBarathraj left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completed requested changes

@KBarathraj

Copy link
Copy Markdown
Contributor Author

Hi @utksh1 — this is now the third review cycle with the identical three complaints word-for-word. I've checked the diff at every commit:

SeverityChangePair in api.ts has exactly before: Finding and after: Finding — no duplicate
fingerprint() uses .get("title") or "", .get("category") or "", .get("target") or "" — cannot crash
34 tests cover same-scan 400, asymmetric 404s, different-target 400, and valid-diff content

Could you paste the specific file + line number for each concern? I can't fix something I can't locate in the code.

KBarathraj added 2 commits June 9, 2026 21:00
- Add GET /api/v1/scans/diff endpoint with 404/400/422 validation
- Add diff_service.py with fingerprint and compute_diff logic
- Add Pydantic v2 schemas for diff response
- Add ScanComparePicker, ScanDiffView, DiffFindingCard components
- Add useScanDiff hook with AbortController for race condition safety
- Harden fingerprint and _parse_findings against malformed data
- Add 32 tests: unit and route-level coverage

Closes utksh1#336
- Move same-scan guard before DB fetches (400 for non-existent same ID)
- Fix remediation default None instead of empty string in schemas
- Wire AbortController signal end-to-end through getScanDiff()
- Add asymmetric missing-scan 404 tests (scan_a missing, scan_b missing)
- Strengthen valid-diff test to assert diff content not just shape

34 tests passing
@KBarathraj KBarathraj force-pushed the feat/scan-history-diff-view branch from b597b2e to cce7816 Compare June 9, 2026 15:44
@KBarathraj KBarathraj requested a review from utksh1 June 9, 2026 15:56
@KBarathraj

Copy link
Copy Markdown
Contributor Author

hey @utksh1 ,can you review this pr out

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the latest push. Still too large/risky to merge as-is: 14 files and full-stack scan history diff behavior need a cleaner rebase and focused verification. Please rebase on main, reduce unrelated churn, and make the PR easier to validate end to end.

@utksh1

utksh1 commented Jun 24, 2026

Copy link
Copy Markdown
Owner

Closing due to unresolved review feedback.

@utksh1 utksh1 closed this Jun 24, 2026
@utksh1 utksh1 added the gssoc:invalid Admin validation: invalid for GSSoC scoring label Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:frontend Frontend React/UI work gssoc:invalid Admin validation: invalid for GSSoC scoring level:advanced 55 pts difficulty label for advanced contributor PRs type:feature Feature work category bonus label type:testing Testing work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Scan History Diff View for Comparing Historical Scan Results

2 participants