feat: implement protocols section and fix backend DI issue#150
feat: implement protocols section and fix backend DI issue#150Rajkoli145 wants to merge 6 commits intoInferara:mainfrom
Conversation
|
hello @Rajkoli145 Thank you for the submission and small demo video that you included. Could you add some additional search filters similar to the reports page? For example having the protocols search by default would be helpful, it let's users type the protocol themselves or choose from the existing list. 
I would recommend the default search box using the "protocol" search filter same as the Vulnerabilities page If you can't see the filter by default, press the collapsing pyramid icon Also having the company & auditor search drop down list available would be useful. Thanks again and let me know if you have any questions or comments |
|
Thanks @SurfingBowser for the feedback! |
|
Hii @SurfingBowser Implemented advanced search filters for the Protocols page |
There was a problem hiding this comment.
Pull request overview
This PR implements a new Protocols page to improve navigation and discoverability of audited protocols. The feature addresses issue #149 by adding a centralized view of all protocols with their security metrics. Additionally, it includes a critical backend dependency injection fix that resolves a service lifetime conflict that would have prevented the application from starting.
Changes:
- Added a new
/protocolsroute with a responsive grid layout displaying protocol cards with audit metrics (reports, vulnerabilities, fixed issues, fix rate) - Implemented client-side search and multi-select filtering by protocol name, company, auditor, and description
- Changed processor services from Scoped to Singleton lifetime in DI container to fix compatibility with background service (processors use IDbContextFactory pattern making them safe as singletons)
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| UI/src/features/pages/regular/protocols/protocols.tsx | Main protocols page component with card-based grid layout, search, and filtering UI |
| UI/src/features/pages/regular/protocols/hooks/useProtocols.hook.ts | Custom hook managing protocols data fetching, filtering logic, and metrics calculation |
| UI/src/features/pages/regular/protocols/hooks/index.ts | Barrel export file for hooks |
| UI/src/features/pages/regular/main-window/main-window.tsx | Adds "Protocols" navigation item and route registration |
| UI/public/env.js | Formatting cleanup (trailing whitespace) |
| UI/package-lock.json | Peer dependency metadata updates for proper package resolution |
| Backend/SorobanSecurityPortalApi/Startup.cs | Changes processor registration from AddScoped to AddSingletons to fix DI lifetime mismatch |
| Backend/SorobanSecurityPortalApi/Services/ProcessingServices/BackgroundWorkingHostedService.cs | Refactors StartAsync to use fire-and-forget pattern with improved error handling |
Files not reviewed (1)
- UI/package-lock.json: Language not supported
Comments suppressed due to low confidence (4)
UI/src/features/pages/regular/protocols/protocols.tsx:298
- The IconButton lacks an accessible label (aria-label or title attribute). Screen reader users won't know what this button does. Add an aria-label such as "View protocol details in fullscreen" or a Tooltip component to provide context for assistive technologies and improve accessibility.
<IconButton
sx={{
borderRadius: '10px',
border: '1px solid',
borderColor: 'divider'
}}
onClick={() => navigate(`/protocol/${protocol.id}`)}
>
<FullscreenIcon />
</IconButton>
UI/src/features/pages/regular/protocols/hooks/useProtocols.hook.ts:97
- The getProtocolMetrics function is called for each protocol during rendering, and each call filters through all reports and vulnerabilities arrays. With many protocols, reports, and vulnerabilities, this could cause performance issues. Consider memoizing the results using useMemo to compute metrics once when the underlying data changes, or pre-compute all metrics in the hook and return a Map of protocol IDs to metrics. This would reduce the time complexity from O(n*m) where n is protocols and m is reports+vulnerabilities, to O(m) for pre-computation.
const getProtocolMetrics = (protocolId: number) => {
const protocolReports = reports.filter(r => r.protocolId === protocolId);
const protocolVulnerabilities = vulnerabilities.filter(v => v.protocolId === protocolId);
const totalVulnerabilities = protocolVulnerabilities.length;
const fixedVulnerabilities = protocolVulnerabilities.filter(v => v.category === VulnerabilityCategory.Valid).length;
const fixRate = totalVulnerabilities > 0 ? Math.round((fixedVulnerabilities / totalVulnerabilities) * 100) : 0;
return {
reportsCount: protocolReports.length,
vulnerabilitiesCount: totalVulnerabilities,
fixedCount: fixedVulnerabilities,
fixRate,
companyName: protocolReports.length > 0 ? protocolReports[0].companyName : 'N/A',
auditors: Array.from(new Set(protocolReports.map(r => r.auditorName))).filter(Boolean) as string[]
};
};
UI/src/features/pages/regular/protocols/hooks/useProtocols.hook.ts:86
- The logic for calculating fixed vulnerabilities only counts vulnerabilities with category VulnerabilityCategory.Valid (0), which represents "Valid (Fixed)". This excludes partially fixed vulnerabilities (VulnerabilityCategory.ValidPartiallyFixed). Consider whether the fix rate calculation should include all types of fixes or only fully fixed vulnerabilities. The codebase shows inconsistent patterns - company-details.hook.ts uses the same approach (only counting Valid), while protocol-details.hook.ts uses status field instead. For consistency and accuracy, consider whether partially fixed vulnerabilities should contribute to the fix count or be tracked separately.
const fixedVulnerabilities = protocolVulnerabilities.filter(v => v.category === VulnerabilityCategory.Valid).length;
Backend/SorobanSecurityPortalApi/Services/ProcessingServices/BackgroundWorkingHostedService.cs:52
- The StartAsync method has been changed from async Task to Task with a fire-and-forget pattern using Task.Run without awaiting. While this prevents blocking during service startup, it creates an unobserved task that could silently fail. The cancellation token is passed to Task.Run, but if an exception occurs in the background loop before the try-catch blocks, it won't be logged. Consider adding a top-level try-catch around the Task.Run lambda or using a continuation to handle unobserved exceptions.
_ = Task.Run(async () =>
{
while (!cancellationToken.IsCancellationRequested)
{
try
{
await Task.Run(AutoCompactLargeObjectHeap, cancellationToken);
await DoReportsFix();
await DoReportsEmbedding();
await DoVulnerabilitiesEmbedding();
await Task.Delay(TimeSpan.FromSeconds(10), cancellationToken);
}
catch (OperationCanceledException)
{
break;
}
catch (Exception ex)
{
Console.WriteLine($"Error in background worker: {ex.Message}");
await Task.Delay(TimeSpan.FromSeconds(10), cancellationToken);
}
}
}, cancellationToken);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0xGeorgii
left a comment
0xGeorgii
left a comment
There was a problem hiding this comment.
@Rajkoli145 thank you for contributing, please address comments
| services.ForInterfacesMatching("^I.*Processor$") | ||
| .OfAssemblies(Assembly.GetExecutingAssembly()) | ||
| .AddScoped(); | ||
| .AddSingletons(); |
| public Task StartAsync(CancellationToken cancellationToken) | ||
| { | ||
| while (!cancellationToken.IsCancellationRequested) | ||
| _ = Task.Run(async () => |
There was a problem hiding this comment.
_ = Task.Run(...) -- the Task is discarded. If an unhandled exception escapes, it's silently lost. Use BackgroundService instead it's the standard .NET pattern for this exact scenario. Inherit from BackgroundService and override ExecuteAsync. Use ILogger<T> with the full exception, not just ex.Message.
| const fetchData = async () => { | ||
| setLoading(true); | ||
| try { | ||
| const [protocolsData, reportsData, vulnerabilitiesData, companiesData, auditorsData] = await Promise.all([ |
There was a problem hiding this comment.
This is O(P * (R + V)) on every render. As the database grows, this will be extremely slow. Add a backend endpoint (e.g., /api/v1/protocols/with-metrics) that returns pre-computed metrics, consistent with
existing statistics endpoints for other entities.
| setCompaniesList(companiesData); | ||
| setAuditorsList(auditorsData); | ||
| } catch (error) { | ||
| console.error('Failed to fetch protocols data:', error); |
There was a problem hiding this comment.
The hook catches errors with console.error but has no error state. If API calls fail, users see "No protocols found" instead of an error message. Other pages expose an error field -- this should too.
| } | ||
| }}> | ||
| <Box sx={{ position: 'relative', pt: '56.25%', bgcolor: themeMode === 'light' ? '#fff' : '#222', borderRadius: '20px 20px 0 0', overflow: 'hidden' }}> | ||
| <CardMedia |
There was a problem hiding this comment.
The cards use raw CardMedia with no loading state, no error fallback, and no 404 handling. Follow the image loading UX pattern from reports.tsx.
| }, [searchText, protocolsList, reports, selectedProtocols, selectedCompanies, selectedAuditors]); | ||
|
|
||
| // Aggregate metrics for each protocol | ||
| const getProtocolMetrics = (protocolId: number) => { |
There was a problem hiding this comment.
This function is recreated on every render and called for each protocol card. Pre-compute a Map<number, Metrics> with useMemo instead.
|
|
||
| {showFilters && ( | ||
| <Box sx={{ mb: 4, display: 'flex', flexWrap: 'wrap', gap: 2, alignItems: 'center', p: 2, borderRadius: '12px', bgcolor: themeMode === 'light' ? '#f5f5f5' : '#222', border: '1px solid', borderColor: 'divider' }}> | ||
| <Autocomplete |
There was a problem hiding this comment.
MUI will emit console warnings about object comparison. Add: isOptionEqualToValue={(option, value) => option.id === value.id}
There was a problem hiding this comment.
Both "View Details" and the FullscreenIcon button navigate to the same URL. Remove one.
- Refactored background worker to standard .NET BackgroundService pattern - Corrected dependency injection for processors to AddScoped - Optimized Protocols page performance by moving metrics calculation to the backend - Polished Frontend UX: fixed Autocomplete warnings, added image loading states, and simplified navigation - Added robust error handling and API status feedback to useProtocols hook
|
Hi @0xGeorgii , all requested changes are implemented, including moving metrics calculation to the backend. Ready for a review |
|
hii @0xGeorgii Could you please review it when you get a chance? Thanks! |
Description
This PR adds the new “Protocols” section as requested.
It introduces a /protocols page to make browsing audited protocols and viewing their security metrics easier and more centralized.
Changes Made
Protocols Page
• Created a new /protocols route.
• Implemented a responsive grid layout.
• Displayed protocol cards with:
• Protocol name & logo
• Company name
• Total audit reports
• Vulnerabilities
• Fixed issues
• Fix rate %
Search & Filtering
• Added client-side search.
• Supports filtering by:
• Name
• Company
• Auditor
• Description
Navigation Integration
• Added “Protocols” to the main navigation.
• Registered routing in MainWindow.tsx.
Local Testing
To validate the feature properly, I temporarily seeded sample protocol, report, and vulnerability data to verify:
• Correct metric aggregation
• Working multi-field search
• Proper UI rendering with real data
The seed logic has been removed from this PR.
Verification
• Confirmed backend runs locally (Port 7878).
• Verified search filtering works as expected.
• Tested responsive layout across screen sizes
Demo Video:
Protocol-demo.mov
close #149