[wojciech-dron]

No description provided.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The new in-memory SQLite detection in PrepareDbAsync only checks SqliteConnection.DataSource for "" or ":memory:", which will not match shared-cache in-memory connection strings like Data Source=JobScraperTest;Mode=Memory;Cache=Shared. This may incorrectly call EnsureDbDirectoryExists for an in-memory DB and potentially create unexpected directories or fail on startup. Consider detecting Mode=Memory via SqliteConnectionStringBuilder instead of relying on DataSource. public static async Task PrepareDbAsync(this IServiceProvider services) { using var scope = services.CreateScope(); var dbContext = scope.ServiceProvider.GetRequiredService<JobsDbContext>(); if (dbContext.Database.GetDbConnection() is not SqliteConnection { DataSource: "" or ":memory:" }) EnsureDbDirectoryExists(dbContext); await dbContext.Database.MigrateAsync(); Data Integrity DbContext.Companies.Find(companyName) assumes companyName is the primary key and non-null. If Company is keyed by something else (e.g., Id) or if companyName is null, this can fail or silently behave incorrectly, leading to duplicate companies or runtime exceptions. Prefer querying by the actual key (or by Name via SingleOrDefault) and make companyName non-nullable if it’s required. var company = objectMother.DbContext.Companies.Find(companyName); company ??= objectMother.CreateCompany(name: companyName); entity.Company = company; Test Side Effect The test writes output.pdf into the project directory. This can pollute the working tree, cause flaky failures in parallel runs, or fail in read-only CI environments. Prefer writing to a temp directory (e.g., Path.GetTempPath() / ITestOutputHelper attachments) and/or ensure cleanup after the test. var outputPath = Path.Combine(projectDir, "Features", "Cv", "Pdf", "output.pdf"); await File.WriteAllBytesAsync(outputPath, resultBytes, CancellationToken); File.Exists(outputPath).ShouldBeTrue(); var header = Encoding.ASCII.GetString([.. resultBytes.Take(4)]); header.ShouldBe("%PDF"); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix in-memory SQLite detection The current in-memory detection is incorrect for SQLite connection strings using Mode=Memory;Cache=Shared because DataSource won’t be :memory:. This can cause EnsureDbDirectoryExists to run for an in-memory database and create unexpected files/directories. Parse the SQLite connection string and check SqliteOpenMode.Memory instead. JobScraper.Web/Modules/Persistence/Setup.cs [53-54] -if (dbContext.Database.GetDbConnection() is not SqliteConnection { DataSource: "" or ":memory:" }) +if (dbContext.Database.GetDbConnection() is SqliteConnection sqliteConn) +{ + var csb = new SqliteConnectionStringBuilder(sqliteConn.ConnectionString); + var isInMemory = csb.Mode == SqliteOpenMode.Memory || csb.DataSource is "" or ":memory:"; + if (!isInMemory) + EnsureDbDirectoryExists(dbContext); +} +else +{ EnsureDbDirectoryExists(dbContext); +} Suggestion importance[1-10]: 7 __ Why: The current check only treats DataSource == "" or ":memory:" as in-memory, but SQLite in-memory via Mode=Memory;Cache=Shared typically has a non-:memory: DataSource, so EnsureDbDirectoryExists(dbContext) may run unnecessarily. Parsing the connection string via SqliteConnectionStringBuilder makes the in-memory detection correct and avoids unintended filesystem side effects.	Medium
	Fix company lookup and nulls DbSet.Find(companyName) will only work if Company is keyed by Name; otherwise it can throw or always return null, causing duplicates and flaky tests. Also, companyName is nullable and can propagate into EF lookups and required fields. Normalize companyName to a non-null value and query by the Name column. JobScraper.IntegrationTests/Factories/JobOfferFactory.cs [8-46] public static JobOffer CreateJobOffer(this ObjectMother objectMother, string offerUrl = "https://example.com/job/1", string title = "Software Engineer", DataOrigin? origin = DataOrigin.Indeed, string? companyName = "Test Company", string? location = "Remote", DateTime? scrapedAt = null, List<string>? offerKeywords = null, string? description = "Job description", int? salaryMinMonth = 15000, int? salaryMaxMonth = 20000, string? salaryCurrency = "PLN", DateTime? publishedAt = null, DetailsScrapeStatus detailsScrapeStatus = DetailsScrapeStatus.ToScrape, string? htmlPath = null, string? screenShotPath = null) { + companyName ??= "Test Company"; + var entity = new JobOffer { OfferUrl = offerUrl, Title = title, Origin = origin, CompanyName = companyName, Location = location, ScrapedAt = scrapedAt ?? DateTime.UtcNow, OfferKeywords = offerKeywords ?? [], Description = description, SalaryMinMonth = salaryMinMonth, SalaryMaxMonth = salaryMaxMonth, SalaryCurrency = salaryCurrency, PublishedAt = publishedAt, DetailsScrapeStatus = detailsScrapeStatus, HtmlPath = htmlPath, ScreenShotPath = screenShotPath, }; - var company = objectMother.DbContext.Companies.Find(companyName); - company ??= objectMother.CreateCompany(name: companyName); + var company = objectMother.DbContext.Companies + .FirstOrDefault(c => c.Name == companyName) + ?? objectMother.CreateCompany(name: companyName); + entity.Company = company; Suggestion importance[1-10]: 7 __ Why: Using DbSet.Find(companyName) is only correct if companyName is the primary key; otherwise it will always return null (or error on null), potentially creating duplicate Company rows and making tests flaky. Normalizing companyName to non-null and querying by c.Name aligns the lookup with the intended semantics.	Medium
	Dispose shared SQLite connection AddTestPersistence opens a keep-alive SQLite connection, but it is currently not stored/disposed, which can leak connections and keep resources open across the test run. Keep the returned connection in WebApiTestFactory and dispose it when the factory is disposed. This prevents file locks/resource exhaustion in longer runs. JobScraper.IntegrationTests/Host/WebApiTestFactory.cs [14-54] internal sealed class WebApiTestFactory(string connectionString) : WebApplicationFactory<Program> { private readonly TestLoggerProvider testLoggerProvider = new(); + private Microsoft.Data.Sqlite.SqliteConnection? keepAliveConnection; public ITestOutputHelper? TestOutput { set => testLoggerProvider.TestOutput = value; } protected override void ConfigureWebHost(IWebHostBuilder builder) { builder.ConfigureLogging(loggingBuilder => { loggingBuilder.ClearProviders(); loggingBuilder.SetMinimumLevel(LogLevel.Error); loggingBuilder.AddFilter("Microsoft.EntityFrameworkCore", LogLevel.Information); // sql queries from ef in test output loggingBuilder.AddProvider(testLoggerProvider); }); builder.ConfigureAppConfiguration(config => { config.AddInMemoryCollection(new Dictionary<string, string?> { ["ConnectionStrings:DefaultConnection"] = connectionString, ["AppSettings:TickerQ:DisableJobs"] = "true", }); }); builder.ConfigureServices(s => { - s.AddTestPersistence(connectionString); + keepAliveConnection = s.AddTestPersistence(connectionString); s.MockAllHttpClients(); s.AddTimeProviderMock(); s.AddObjectMother(); s.AddAuthentication(TestAuthHandler.SchemeName) .AddScheme<AuthenticationSchemeOptions, TestAuthHandler>( TestAuthHandler.SchemeName, _ => { }); }); } + + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + if (disposing) + { + keepAliveConnection?.Dispose(); + keepAliveConnection = null; + } + } } Suggestion importance[1-10]: 6 __ Why: AddTestPersistence opens a long-lived SQLite connection to keep the in-memory DB alive, and not disposing it can leak resources across the test run. Holding that connection on WebApiTestFactory and disposing it in Dispose(bool) is a reasonable fix to prevent lingering connections/file locks.	Low