Skip to content

Conversation

@Oluwaseyi89
Copy link

@Oluwaseyi89 Oluwaseyi89 commented Jan 29, 2026

Description
This PR implements a robust, threaded comment system that can be attached to any entity (e.g., Vulnerabilities, Reports) using a polymorphic association. It includes support for Markdown rendering and automated @mention detection.

Key Changes
Controller: Added CommentController with routes for GET (threaded view), POST (creation), and DELETE (soft-delete).

Service Layer: Created CommentService to handle business logic, including:

Markdown Processing: Integration with Markdig to convert content to ContentHtml.

Mention Detection: Regex-based identification of @username mentions, populating the Mentions collection with start indices.

Threading: Recursive logic to build a parent-child hierarchy for replies.

Testing: Implemented xUnit tests in SorobanSecurityPortalApi.Tests/Services to verify:

Regex accuracy for multiple mentions.

Safe handling of nullable HTML content.

Successful dependency injection of IMapper and ICommentProcessor.

Technical Details
Base Route: api/v1/comment

Polymorphic Key: Uses EntityType (string) and EntityId (int) to associate comments without strict Foreign Keys to every table.

Dependencies: Requires AutoMapper registration and Markdig NuGet package.

How to Test
API: Run the project and navigate to http://localhost:7848/api/v1/comment/{EntityType}/{EntityId}.

Unit Tests: Run dotnet test to execute the new test suite in the Services folder.

Markdown: POST a comment containing bold text and verify the returned ContentHtml contains .

Checklist
[x] Route prefix matches appsettings.json (api/v1)

[x] AutoMapper profiles registered in Startup.cs

[x] Handled null-reference warnings in test expression trees

[x] Verified Authorize attributes are present on write operations

closes issue #57

@Oluwaseyi89
Copy link
Author

@0xGeorgii @SurfingBowser Please, review and give feedback.

@0xGeorgii 0xGeorgii requested a review from Copilot January 30, 2026 02:53
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a polymorphic comment system with threading, Markdown rendering, and @mention detection, alongside a badge system for user achievements. The changes include database migrations, service layers, controllers, and comprehensive refactoring of dependency injection to use service scopes in background services.

Changes:

  • Added a polymorphic comment system with threading support, Markdown rendering via Markdig, and automated @mention detection
  • Implemented a badge award system with predefined achievement badges and reputation-based criteria
  • Refactored background services to use IServiceScopeFactory for proper scoped service resolution

Reviewed changes

Copilot reviewed 31 out of 34 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
Backend/SorobanSecurityPortalApi/Startup.cs Refactored JWT configuration to use IOptions pattern and removed premature service provider builds
Backend/SorobanSecurityPortalApi/SorobanSecurityPortalApi.csproj Downgraded Swashbuckle packages and added Microsoft.OpenApi dependency
Backend/SorobanSecurityPortalApi/Services/ProcessingServices/InstanceSyncHostedService.cs Updated to use IServiceScopeFactory for proper scoped service resolution
Backend/SorobanSecurityPortalApi/Services/ProcessingServices/ICommentService.cs Defines the interface for comment operations
Backend/SorobanSecurityPortalApi/Services/ProcessingServices/CommentService.cs Implements comment business logic with Markdown processing and mention detection
Backend/SorobanSecurityPortalApi/Services/ProcessingServices/BadgeAwardService.cs Implements badge awarding logic based on reputation and criteria
Backend/SorobanSecurityPortalApi/Services/ProcessingServices/BackgroundWorkingHostedService.cs Refactored to use scoped services and improved error handling
Backend/SorobanSecurityPortalApi/Services/ControllersServices/ReportService.cs Added platform-specific attributes for PDF rendering methods
Backend/SorobanSecurityPortalApi/Models/ViewModels/CommentViewModels.cs Defines view models for comments and mentions
Backend/SorobanSecurityPortalApi/Models/ViewModels/BadgeViewModel.cs Defines view model for badges
Backend/SorobanSecurityPortalApi/Models/Mapping/CommentMappingProfile.cs AutoMapper profile for comment entity-to-viewmodel mapping
Backend/SorobanSecurityPortalApi/Models/Mapping/BadgeMappingProfile.cs AutoMapper profile for badge entity-to-viewmodel mapping
Backend/SorobanSecurityPortalApi/Models/DbModels/UserBadgeModel.cs Database model for user badge associations
Backend/SorobanSecurityPortalApi/Models/DbModels/MentionModel.cs Database model for @mentions in comments
Backend/SorobanSecurityPortalApi/Models/DbModels/CommentStatus.cs Enum defining comment status values
Backend/SorobanSecurityPortalApi/Models/DbModels/CommentModel.cs Database model for polymorphic comments with threading
Backend/SorobanSecurityPortalApi/Models/DbModels/BadgeDefinitionModel.cs Database model for badge definitions
Backend/SorobanSecurityPortalApi/Migrations/DbModelSnapshot.cs Updated snapshot with new comment and badge tables
Backend/SorobanSecurityPortalApi/Migrations/20260129144018_AddCommentSystem.cs Migration creating comment system tables and indexes
Backend/SorobanSecurityPortalApi/Migrations/20260129011841_UpdateUserBadgeForeignKeys.cs Migration updating user badge foreign key relationships
Backend/SorobanSecurityPortalApi/Migrations/20260128231425_AddBadgeSystem.cs Migration creating badge system tables with seeded data
Backend/SorobanSecurityPortalApi/Data/Processors/IBadgeProcessor.cs Interface for badge data access operations
Backend/SorobanSecurityPortalApi/Data/Processors/CommentProcessor.cs Data access layer for comment operations
Backend/SorobanSecurityPortalApi/Data/Processors/BadgeProcessor.cs Data access layer for badge operations
Backend/SorobanSecurityPortalApi/Controllers/CommentController.cs REST API endpoints for comment CRUD operations
Backend/SorobanSecurityPortalApi/Controllers/BadgeController.cs REST API endpoints for badge retrieval
Backend/SorobanSecurityPortalApi/Common/Extensions/Extensions.cs Updated SHA256 usage to non-deprecated method
Backend/SorobanSecurityPortalApi/Common/ExtendedConfig.cs Refactored to use IServiceScopeFactory
Backend/SorobanSecurityPortalApi/Common/Enums/BadgeCategory.cs Enum defining badge categories
Backend/SorobanSecurityPortalApi/Common/Data/Db.cs Added DbSets and model configurations for comments and badges
Backend/SorobanSecurityPortalApi.Tests/Services/ProcessingServices/CommentServiceTests.cs Unit tests for comment service functionality
Files not reviewed (3)
  • Backend/SorobanSecurityPortalApi/Migrations/20260128231425_AddBadgeSystem.Designer.cs: Language not supported
  • Backend/SorobanSecurityPortalApi/Migrations/20260129011841_UpdateUserBadgeForeignKeys.Designer.cs: Language not supported
  • Backend/SorobanSecurityPortalApi/Migrations/20260129144018_AddCommentSystem.Designer.cs: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

private List<MentionModel> ParseMentions(string content)
{
var mentions = new List<MentionModel>();
var regex = new Regex(@"@(\w+)");
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The regex pattern is instantiated on every call. Consider making it a static readonly field with RegexOptions.Compiled for better performance, especially if this method is called frequently.

Copilot uses AI. Check for mistakes.
}

public async Task<bool> DeleteComment(int commentId, int userId)
{
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The userId parameter is accepted but never used to verify ownership or permissions before deleting the comment. This could allow any authenticated user to delete any comment. Add authorization checks to ensure only the comment author or an admin can delete comments.

Suggested change
{
{
var comment = await _processor.GetCommentById(commentId);
if (comment == null)
{
return false;
}
if (comment.AuthorId != userId)
{
// Only the author (or an admin, if checked elsewhere) is allowed to delete the comment
return false;
}

Copilot uses AI. Check for mistakes.
return Unauthorized();

var success = await _commentService.DeleteComment(id, userId);
if (!success) return BadRequest("Could not delete comment or unauthorized.");
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The error message 'Could not delete comment or unauthorized.' is ambiguous and combines two different error scenarios. Consider returning different status codes and messages: 404 Not Found if the comment doesn't exist, 403 Forbidden if unauthorized, or 500 Internal Server Error for other failures.

Suggested change
if (!success) return BadRequest("Could not delete comment or unauthorized.");
if (!success) return BadRequest("Failed to delete comment.");

Copilot uses AI. Check for mistakes.
Comment on lines 19 to 20
[MaxLength(50)]
public string? EntityType { get; set; }
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The EntityType field is marked as nullable with [Required] attribute, which is contradictory. Either remove the nullable annotation or remove the [Required] attribute. Since this is a key field for polymorphic associations, it should be non-nullable.

Copilot uses AI. Check for mistakes.
public virtual ICollection<CommentModel> Replies { get; set; } = new List<CommentModel>();

[Required]
public string? Content { get; set; }
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The Content field is marked as nullable with [Required] attribute, which is contradictory. Comments should have content, so remove the nullable annotation to make this non-nullable.

Suggested change
public string? Content { get; set; }
public string Content { get; set; }

Copilot uses AI. Check for mistakes.
catch (Exception ex)
{
Console.WriteLine($"Error during report ({reportModel.Name} / {reportModel.Id}) embedding: {ex.Message}");
Console.WriteLine($"Error during report embedding: {ex.Message}");
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

Multiple instances of Console.WriteLine are used for error logging. Replace all occurrences with proper ILogger usage for consistent, structured logging with appropriate log levels.

Copilot uses AI. Check for mistakes.
catch (Exception ex)
{
Console.WriteLine($"Error during vulnerability ({vulnerability.Title} / {vulnerability.Id}) embedding: {ex.Message}");
Console.WriteLine($"Error during vulnerability embedding: {ex.Message}");
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

Multiple instances of Console.WriteLine are used for error logging. Replace all occurrences with proper ILogger usage for consistent, structured logging with appropriate log levels.

Copilot uses AI. Check for mistakes.
Comment on lines 122 to 127
using (var sp = services.BuildServiceProvider())
{
var ec = sp.GetRequiredService<ExtendedConfig>();
var logger = sp.GetRequiredService<ILogger<Startup>>();
services.AddHttpClients(ec, logger);
}
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

Building a service provider during service registration (ConfigureServices) creates a temporary container that gets disposed when leaving the using block. This is an anti-pattern that can lead to resource leaks and incorrect lifetimes. Consider restructuring to avoid building the service provider during configuration, or use IOptions pattern to configure services that depend on other services.

Suggested change
using (var sp = services.BuildServiceProvider())
{
var ec = sp.GetRequiredService<ExtendedConfig>();
var logger = sp.GetRequiredService<ILogger<Startup>>();
services.AddHttpClients(ec, logger);
}
var serviceProvider = services.BuildServiceProvider();
var ec = serviceProvider.GetRequiredService<ExtendedConfig>();
var logger = serviceProvider.GetRequiredService<ILogger<Startup>>();
services.AddHttpClients(ec, logger);

Copilot uses AI. Check for mistakes.
return Ok(result);
}

[HttpGet("test-definitions")]
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The endpoint 'test-definitions' contains 'test' in its name, suggesting this is a temporary testing endpoint. Either rename it to a production-appropriate name like 'definitions' or remove it if it's only for development/testing purposes.

Suggested change
[HttpGet("test-definitions")]
[HttpGet("definitions")]

Copilot uses AI. Check for mistakes.
.HasMaxLength(20);

builder.Entity<CommentModel>()
.HasQueryFilter(c => c.DeletedAt == null);
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The global query filter on DeletedAt may cause issues when you need to access soft-deleted comments for auditing or administrative purposes. Consider adding a method to temporarily disable this filter when needed, or implement separate methods for accessing all comments including deleted ones.

Copilot uses AI. Check for mistakes.
@@ -50,6 +51,9 @@ public async Task<ReportViewModel> Get(int reportId)
}

//TODO UI should send Protocol and Auditor as Ids, not names. Then need to update the mapping used at the line 55
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

}

//TODO UI should send Protocol and Auditor as Ids, not names. Then need to update the mapping used at the line 55
[SupportedOSPlatform("windows")]
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these?

Copy link
Author

Choose a reason for hiding this comment

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

I added these attributes to avoid platform specific exceptions that may arise. I actually say Warning at the terminal about exceptions that may be raised on certain platforms.

@Oluwaseyi89
Copy link
Author

@0xGeorgii @SurfingBowser please review my latest commits to this branch.

@Oluwaseyi89 Oluwaseyi89 requested a review from 0xGeorgii January 31, 2026 16:18
@Oluwaseyi89
Copy link
Author

@0xGeorgii @SurfingBowser Please I am awaiting your review.

}

builder.Entity<BadgeDefinitionModel>().HasData(
new BadgeDefinitionModel { Id = Guid.Parse("00000000-0000-0000-0000-000000000001"), Name = "First Comment", Description = "Posted first comment", Icon = "🎉", Category = BadgeCategory.Participation, Criteria = "first_comment" },
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a part of #132 why is it here

Copy link
Author

Choose a reason for hiding this comment

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

It's as a result of branch chaining. I worked on badge system issue first. But I ensured the comment system issue branch is being up to date with the badge system issue branch.

@Oluwaseyi89
Copy link
Author

Oluwaseyi89 commented Feb 5, 2026

@0xGeorgii any feedback?

@0xGeorgii
Copy link
Contributor

@Oluwaseyi89, please resolve merge conflicts and remove code taken from other branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants