Skip to content

Apply the best-practices review: SQLite, paging, list summaries, health checks and build rules - #24

Merged
stphnwlsh merged 7 commits into
mainfrom
feature/best-practices
Oct 29, 2026
Merged

stphnwlsh merged 7 commits into
mainfrom
feature/best-practices

Conversation

@stphnwlsh

@stphnwlsh stphnwlsh commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

This applies the decisions from the best-practices review, plus the follow-ups from the second pass.

Shared layers, identical in all five repos:

  • Nullable reference types are on everywhere, and every warning is fixed.
  • The data now lives in SQLite with EF Core migrations, in place of the in-memory provider. Startup applies the migrations. It seeds sample data, from a fixed Bogus seed, only when Database:SeedSampleData is on, which it is in the local and Docker settings and nowhere else.
  • Each test gets its own in-memory SQLite database, so tests never touch a real file or each other's data.
  • The DbContext and repositories are scoped instead of singletons. With 200 parallel requests the old setup failed 169 of them, and now all 200 succeed.
  • Lists are paged by offset and limit, with an optional search query. The unbounded get-all queries and the separate search queries are gone. Search uses LIKE with escaping, so it translates to SQL.
  • List queries return MovieSummary and AuthorSummary, with a review count and average stars worked out in SQL, so a page no longer loads every review. The by-id queries still return the reviews. New GetReviewsForMovies and GetReviewsForAuthors queries fetch the reviews of many movies or authors in one call.
  • Updating a review changes only its stars.
  • Delete and update use ExecuteDeleteAsync and ExecuteUpdateAsync, and the CancellationToken flows from the presentation layer down to EF Core.
  • A database health check is available for readiness endpoints.
  • Build rules in Directory.Build.props: code style and the recommended analyzers are checked on every build and warnings fail it. Package versions live in Directory.Packages.props, and CI runs dotnet format --verify-no-changes.
  • The Seq image is pinned, and Dependabot now updates docker-compose images.
  • The Publish job only runs in this repository, so forks and repos created from the template don't try to publish.
  • Release notes are grouped by label through .github/release.yml: breaking changes, new features, bug fixes, documentation, build and pipeline, and dependencies.
  • The coverage reports that had been committed are removed, and *.db files and coverage reports are now ignored.

MCP server:

  • movies_search, authors_search and reviews_list take offset and limit, and return nextOffset (null on the last page) instead of a cursor.
  • movies_search and authors_search read the review count and average from the summaries. With includeReviews they fetch the reviews of the whole page in one query.
  • reviews_write with a reviewId changes only the stars. movieId and authorId are only needed to create a review.
  • The server instructions describe offset paging and stars-only updates.
  • Every tool, resource and prompt takes the CancellationToken the SDK binds, so a cancelled call stops.
  • /health/live and /health/ready answer without a token.
  • HSTS is sent outside local runs, and HTTPS redirection runs early in the pipeline.
  • The image runs as the non-root app user, keeps the database on a volume at /app/data, and has a HEALTHCHECK.

Related issues

None.

Type of change

  • Bug fix
  • New feature
  • Breaking change (the generated solution or template parameters change in a way existing users will notice)
  • Refactoring or clean-up
  • Documentation
  • Build, pipeline or dependency update

Shared layers

  • This only touches the Presentation layer of this template.
  • This touches the Application or Infrastructure layers, tests, pipeline or repository setup, so it needs to be carried across all five templates.

Checklist

  • dotnet build and dotnet test pass locally with no warnings.
  • I added or updated tests, and coverage stays at or above 95%.
  • I updated the README or docs where behaviour changed.
  • I checked the template still installs and creates a working solution (see CONTRIBUTING.md).

- Turn on nullable reference types and fix every warning
- Page the search and list tools with offset and limit, returning nextOffset instead of a cursor
- Pass the CancellationToken through every tool, resource and prompt
- Let reviews_write change only the stars of an existing review
- Store data in SQLite with EF Core migrations, seeded once at startup with a fixed seed
- Run tests against a throwaway in-memory SQLite database
- Register the DbContext and repositories as scoped, fixing concurrent request failures
- Only publish from stphnwlsh/CleanModelContextProtocol
- Remove committed coverage reports
@codecov

codecov Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.94366% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.51%. Comparing base (771fb83) to head (d2162bd).

Files with missing lines Patch % Lines
...etReviewsForAuthors/GetReviewsForAuthorsHandler.cs 85.71% 0 Missing and 1 partial ⚠️
.../GetReviewsForMovies/GetReviewsForMoviesHandler.cs 85.71% 0 Missing and 1 partial ⚠️
src/Presentation/Models/ResultMapper.cs 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
- Coverage   98.97%   98.51%   -0.47%     
==========================================
  Files          68       73       +5     
  Lines         686      741      +55     
  Branches       41       51      +10     
==========================================
+ Hits          679      730      +51     
- Partials        7       11       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…checks

- Check code style and the recommended analyzers on every build and treat warnings as errors
- Move package versions into Directory.Packages.props and check formatting in CI
- List movies and authors as summaries with a review count and average stars worked out in SQL
- Add queries that fetch the reviews of many movies or authors in one call
- Seed sample data only when Database:SeedSampleData is on
- Give each repository test its own in-memory database
- Add /health/live and /health/ready, and a HEALTHCHECK in the image
- Run the container as the non-root app user with the database on a volume
- Send HSTS outside local runs and redirect to HTTPS early in the pipeline
- Pin the Seq image and let Dependabot update it
@stphnwlsh stphnwlsh changed the title Apply the best-practices review Apply the best-practices review: SQLite, paging, list summaries, health checks and build rules Oct 29, 2026
@stphnwlsh stphnwlsh added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request breaking-change build labels Oct 29, 2026 — with Claude
@stphnwlsh
stphnwlsh merged commit 97a7f2c into main Oct 29, 2026
12 checks passed
@stphnwlsh
stphnwlsh deleted the feature/best-practices branch October 29, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change bug Something isn't working build documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant