Skip to content

Release: champion timeline analytics + API/ingestor hardening#552

Merged
ilyanfraimbault merged 48 commits into
masterfrom
develop
Jun 18, 2026
Merged

Release: champion timeline analytics + API/ingestor hardening#552
ilyanfraimbault merged 48 commits into
masterfrom
develop

Conversation

@ilyanfraimbault

Copy link
Copy Markdown
Owner

Release developmaster

⭐ Champion timeline analytics (new)

Exploits the Riot match timeline — data op.gg/u.gg surface poorly — across new live champion-page sections. No extra Riot calls; the timeline payload is already fetched.

🛡️ API / ingestor hardening

🌐 Web

📦 Dependencies

6 Dependabot bumps: Scalar.AspNetCore, Test.Sdk, nuxt group, vue-tsc, happy-dom, vitest.


⚠️ Post-deploy

  • Apply the 2 new migrations (match_participant_timeline_snapshots, match_participant_kill_positions) via the idempotent SQL-script process (runtime migrations are gated).
  • The new analytics tables populate as new matches are ingested; the scaling chart works immediately (no timeline needed). A historical backfill (reset TimelineIngested + re-ingest timelines) is optional.

Merge as a merge commit (not squash), per the release convention.

claude and others added 30 commits June 15, 2026 12:49
…234)

GetOrCreatePerkCatalogIdsAsync caught DbUpdateException on the unique-index
collision that occurs under concurrent ingestion and recovered by calling
ChangeTracker.Clear(), which silently discarded any pending changes the
caller had staged on the context — a lost-update anti-pattern.

Insert the missing catalog rows via a raw parameterized
INSERT ... SELECT FROM unnest(...) ON CONFLICT DO NOTHING. Concurrent
collisions are absorbed by Postgres without an exception, the change
tracker is never touched, and IDs are reloaded by key for both
pre-existing and freshly inserted rows.

https://claude.ai/code/session_01EUvXxQTniziU8TgeCWCrEp
When match ingestion fails for an account, the process reverts the claim
back to Queued. If that revert itself threw, the exception escaped the
per-account loop and aborted the rest of the batch, and the claim stayed
Processing until lease expiry with no signal.

Wrap the revert in its own try/catch: log the revert failure as the new
MatchRevertFailed ops event with the original ingestion exception as its
cause, then continue with the remaining accounts.

Closes #263
…iter

Per-batch GetMatchAsync calls ran sequentially, making them the throughput
bottleneck for 20 matches x N accounts. Replace the sequential foreach with
Parallel.ForEachAsync writing into per-index slots so results stay ordered
and concurrent writes never collide.

Concurrency is bounded by the new MatchIngestion:MaxMatchFetchConcurrency
option (default 4); the resilience handler enforces per-region rate limiting.

Closes #265
appsettings.json ships "Cors:Origins": [], so the previous policy built
AllowAnyHeader/AllowAnyMethod with no WithOrigins call — a no-op CORS
policy that silently rejects the frontend in production while working
locally (Development ships real origins).

Promote the section to a typed CorsOptions class and validate it on
start: empty Origins fails the boot in any non-Development environment,
and only logs a warning under Development. Builds the FrontendCors policy
from the bound options so there is a single source for the origin list.

The test web factory now injects a default Cors:Origins entry (it runs
under the non-Development "Testing" environment) and a new integration
test covers both the fail-loud boot and the configured-origin path.

Closes #209
RankSnapshotWriter.Write mutated account.LastRankSyncAtUtc (and Score) even
when it returned Unchanged, which read as a surprising side-effect on the
no-op path.

The timestamp bump is load-bearing rather than accidental:
LastRankSyncAtUtc is the freshness gate (RankSyncFreshness, 15m) that stops
AccountRefreshProcess from re-issuing the League-v4 by-puuid call and dedups
against DiscoveryProcess. A fresh reading that matches the latest rank is
still a successful sync, so the timestamp must advance on the Unchanged path;
guarding it behind the changed-state branch would silently regress that gate
and re-fetch stable-rank accounts every cycle.

Per the issue's second option, advertise the side-effect instead of hiding
it: rename Write -> Ingest and document on the interface that every reading
advances the account's sync bookkeeping while a snapshot row is appended only
when the rank actually changed.

Closes #267
PatchVersion only captured major/minor, silently discarding Riot's
hotfix build segment ("16.4.521"). Add an optional Build int captured
by TryParse (round-tripped by ToString, included in equality and
ordering, with a build-less patch sorting before its hotfixes).

Normalizer call sites that relied on ToString() collapsing to
"MAJOR.MINOR" now reconstruct major.minor explicitly so scope keys,
CDN URLs and LIKE prefixes keep their canonical form.

Closes #282

https://claude.ai/code/session_013xQsPVCjN63CmiJe5PYzSY
Add app.UseHsts() ahead of the HTTPS redirect for all non-Development
environments, matching the canonical ASP.NET Core middleware order behind
TLS. Development is skipped so a cached HSTS policy can't wedge local HTTP
debugging.

Closes #202

Co-authored-by: Claude <noreply@anthropic.com>
Vocabulary consistency follow-up from PR review: the rank-write test name
still referenced the old method name.
…508)

Replace the per-batch SaveChangesAsync loop that loaded every participant
row with a single set-based UPDATE. A new
IMatchParticipantRepository.BackfillRiotAccountIdAsync uses EF Core's
ExecuteUpdateAsync to fill the orphan RiotAccountId values for the tracked
puuid across all existing matches in one round trip, dropping the now
unused batchSize parameter from the backfiller.

Closes #266

https://claude.ai/code/session_01U9gtZbx9rJoLfv8yWBDmeA

Co-authored-by: Claude <noreply@anthropic.com>
Address non-blocking review feedback: populate the unnest parameter
arrays with one loop over missingKeys instead of four separate
Select().ToArray() enumerations.

https://claude.ai/code/session_01EUvXxQTniziU8TgeCWCrEp
Address non-blocking review feedback on #515: replace the five
duplicated $"{Major}.{Minor}" reconstructions with a dedicated
ToMajorMinor() helper, and add an explicit assertion documenting that
the default Build value equals null.

https://claude.ai/code/session_013xQsPVCjN63CmiJe5PYzSY
perf(ingestor): parallelize per-match Riot fetches in MatchSnapshotWriter
…vent

Address review on #510:
- RevertClaimAsync now rethrows OperationCanceledException before the
  generic catch, so a cooperative shutdown propagates instead of being
  logged as a revert failure and swallowed for every remaining account.
- Strengthen the unit test with a capturing logger that asserts the
  MatchRevertFailed ops event (and its AggregateException cause) is
  emitted, plus a test covering the cancellation-propagation path.
The non-Development guard threw under the integration-test "Testing"
environment, which injects ConnectionStrings:TrueMain via
ConfigureAppConfiguration after the builder reads it here. Narrow the
fail-fast to IsProduction(), matching the issue's production concern and
all deployments (compose*.yaml run as Production), so /readyz can never
report green without a registered Postgres readiness check in prod while
the Testing host stays green.
- Fix CS1734: drop the <paramref> tag from the test factory's class doc
  comment (a class has no parameters to reference).
- Fix IDE0005: remove the redundant TrueMain.TestKit using (it is a
  global using) and the now-unused System.Net using.
- Consolidate the CORS config to a single source: bind CorsOptions once
  and build the FrontendCors policy from the bound options via
  Configure<IOptions<CorsOptions>>, instead of reading configuration a
  second time eagerly for AddCors.
- Decouple the CORS integration test from /champions business logic by
  asserting the Access-Control-Allow-Origin header on a preflight request
  rather than on a 200 from the endpoint.
fix(ingestor): advertise RankSnapshotWriter sync side-effect (#267)
fix(data): replace ChangeTracker.Clear() with ON CONFLICT DO NOTHING in MatchParticipantRepository
feat(core): support hotfix build segment in PatchVersion
…-60bzsp

refactor(api): fail-fast when health check connection string is missing in non-Development
Address non-blocking review feedback:
- Rename the typed options class to FrontendCorsOptions so it no longer
  collides with ASP.NET Core's Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions,
  removing the full-qualification burden for readers.
- Document that the Origins.Length > 0 guard in the policy builder is a
  Development-only path: ValidateOnStart already guarantees a non-empty
  list everywhere else.
…oads (#509)

Stream Riot API JSON straight off the response with ResponseHeadersRead + JsonSerializer.DeserializeAsync instead of buffering full payloads. Drain unread error/404 bodies (CopyToAsync(Stream.Null)) so connections return to the pool, and add unit coverage for the by-puuid streaming path.

Closes #253.
)

Inject TimeProvider (TimeProvider.System by default) into the ingestor
processes and components flagged in #270, replacing direct DateTime.UtcNow
calls so time-dependent business logic can be frozen under test.

Adds a FixedTimeProvider test fake; freezes time in the MatchClaim, RiotMatchMapper
and DiscoveryCadence tests, and threads a TimeProvider through the remaining
process/component tests.

Closes #270.
develop added a readiness-health-check guard that fails fast on a missing
ConnectionStrings:TrueMain only under Production. The test host injects
the connection string after Program reads it at startup, so a Production
host trips that guard before reaching the CORS validation. Use the
Testing environment (still non-Development, so the CORS guard is
exercised) which is exempt from the Production-only connection-string
check, matching TrueMainWebApplicationFactory.
- Program.cs: introduce an AspNetCorsOptions using-alias instead of the
  fully-qualified Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions.
- Program.cs: collapse the Development CORS warning into a single log
  template literal (friendlier to logging analyzers).
- CorsStartupIntegrationTests: assert the preflight returns 204 No Content
  for clearer diagnostics, and make the empty-origins test async with
  await using for consistency with the other test.
…fmk01i

fix(api): fail loudly when Cors:Origins is empty outside Development
…ta-fdi4kr

fix(ingestor): properly surface revert failure in MatchIngestionProcess
…ticipant (#518)

Adds TotalDamageDealtToChampions and VisionScore (int, default 0) to MatchParticipant: Riot DTO fields, entity columns, EF config + additive migration with 0 backfill, snapshot/designer/compiled-model regenerated, mapper copy, and unit tests for the populated and zero-default cases.

Closes #159.
…ion string (#519)

Adds a regression test pinning the #210 Production fail-fast: booting the API
host in Production without ConnectionStrings:TrueMain must throw at startup
rather than silently dropping the Npgsql "ready" health check.
… add map zone classifier (#540)

* feat(ingestor): capture timeline participant frames + event positions

Parse the parts of the Riot match timeline we previously discarded:
per-frame participantFrames (position, gold, CS, jungle minions, damage to
champions) and per-event position + kill participants (killer/victim/assists).
Extract the mapping into a testable RiotTimelineMapper.

Foundation for timeline-derived analytics: per-interval leads (#525),
jungle pathing (#535) and roam index (#536). No extra Riot calls — same
timeline payload, we just parse more of it.

Part of #538.

* feat(core): add Summoner's Rift zone classifier for timeline positions

LolMap.Classify maps a timeline (x, y) to a coarse MapZone (lanes / river /
jungle / base) and IsBlueSide splits on the river anti-diagonal. Heuristic
geometry anchored on the verified map bounds and validated against real turret
coordinates. Enables roam detection (#536) and side-aware jungle analytics.

Camp-coordinate lookup is intentionally not included: no authoritative source
for exact camp positions, so it will be derived empirically from real jungler
position frames rather than hardcoding unverified numbers.

Part of #538.

* refactor(ingestor): address PR review on timeline foundation

- MatchParticipantFrameDto.X/Y are now nullable so an absent position is
  distinct from a real (0, 0) coordinate (pathing/heatmap consumers, #535).
- Document that magic/physical/true damage are deserialized but intentionally
  not propagated (YAGNI).
- Note the river-boundary tie-break in LolMap.IsBlueSide docstring.

Part of #538.

---------

Co-authored-by: ilyanfraimbault <ilyanfraimbault@gmail.com>
dependabot Bot and others added 7 commits June 18, 2026 16:20
---
updated-dependencies:
- dependency-name: Scalar.AspNetCore
  dependency-version: 2.16.3
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
---
updated-dependencies:
- dependency-name: Microsoft.NET.Test.Sdk
  dependency-version: 18.6.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: xunit-stack
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
)

Bumps the nuxt group with 2 updates in the /web directory: [@nuxt/ui](https://github.com/nuxt/ui) and [nuxt](https://github.com/nuxt/nuxt/tree/HEAD/packages/nuxt).


Updates `@nuxt/ui` from 4.8.0 to 4.8.2
- [Release notes](https://github.com/nuxt/ui/releases)
- [Changelog](https://github.com/nuxt/ui/blob/v4/CHANGELOG.md)
- [Commits](nuxt/ui@v4.8.0...v4.8.2)

Updates `nuxt` from 4.4.6 to 4.4.8
- [Release notes](https://github.com/nuxt/nuxt/releases)
- [Commits](https://github.com/nuxt/nuxt/commits/v4.4.8/packages/nuxt)

---
updated-dependencies:
- dependency-name: "@nuxt/ui"
  dependency-version: 4.8.2
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: nuxt
- dependency-name: nuxt
  dependency-version: 4.4.7
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: nuxt
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [vitest](https://github.com/vitest-dev/vitest/tree/HEAD/packages/vitest) from 4.1.7 to 4.1.8.
- [Release notes](https://github.com/vitest-dev/vitest/releases)
- [Changelog](https://github.com/vitest-dev/vitest/blob/main/docs/releases.md)
- [Commits](https://github.com/vitest-dev/vitest/commits/v4.1.8/packages/vitest)

---
updated-dependencies:
- dependency-name: vitest
  dependency-version: 4.1.8
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [happy-dom](https://github.com/capricorn86/happy-dom) from 20.9.0 to 20.10.2.
- [Release notes](https://github.com/capricorn86/happy-dom/releases)
- [Commits](capricorn86/happy-dom@v20.9.0...v20.10.2)

---
updated-dependencies:
- dependency-name: happy-dom
  dependency-version: 20.10.2
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [vue-tsc](https://github.com/vuejs/language-tools/tree/HEAD/packages/tsc) from 3.3.1 to 3.3.4.
- [Release notes](https://github.com/vuejs/language-tools/releases)
- [Changelog](https://github.com/vuejs/language-tools/blob/master/CHANGELOG.md)
- [Commits](https://github.com/vuejs/language-tools/commits/v3.3.4/packages/tsc)

---
updated-dependencies:
- dependency-name: vue-tsc
  dependency-version: 3.3.4
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…551)

Install @nuxtjs/seo and wire the core SEO surfaces so truemain.lol can be indexed by Google: site identity, a dynamic sitemap (champions + truemains), module-managed robots.txt, and canonical/OG/Twitter defaults. The sitemap source caches its fan-out at the origin and degrades gracefully on upstream failures.

Closes #550
Comment thread backend/Api/Services/Champions/ChampionScalingQueryService.cs
Comment thread backend/Ingestor/Processes/Components/MatchIngestion/TimelineIngestionService.cs Outdated
Comment thread backend/Ingestor/Processes/Components/MatchIngestion/MatchSnapshotWriter.cs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Synthèse

Grande PR de release bien construite : HSTS/CORS hardening correct, désérialisation JSON en streaming propre, TimeProvider injecté partout, migrations idempotentes, couverture de tests solide sur les nouveaux endpoints. Un point bloquant à corriger avant merge.


Point BLOQUANT

Index composite manquant sur match_participants (ChampionId, TeamPosition)

Les quatre nouveaux services de query (ChampionScalingQueryService, ChampionTimelineLeadsQueryService, ChampionItemTimingsQueryService, ChampionRoamQueryService) filtrent tous match_participants sur ChampionId + TeamPosition + RiotAccountId IS NOT NULL. Les index actuels (RiotAccountId, MatchId/ParticipantId unique, Puuid/MatchId) ne couvrent pas cette combinaison : chaque cold miss déclenche un sequential scan sur une table qui grossit indéfiniment. Le cache mémoire 60 s n'aide que sur les requêtes strictement répétées.

Correction attendue : ajouter dans MatchParticipantConfiguration.cs + migration :

entity.HasIndex(e => new { e.ChampionId, e.TeamPosition })
      .HasDatabaseName("IX_match_participants_champion_position");

Suggestions (non bloquantes)

  • TimelineIngestionService.cs:97 — Commentaire trompeur : « The delete commits before that SaveChanges » est factuellement incorrect (le delete participe à la transaction externe et n'est pas auto-committé). Voir commentaire inline.
  • MatchSnapshotWriter.cs:47 — Le tableau pré-alloué fetchedSlots[] utilisé avec Parallel.ForEachAsync est un footgun si un futur wrapper absorbe l'exception. Préférer ConcurrentBag ou construire la liste dans le lambda. Voir commentaire inline.
  • KillPositionBuilder.cs:44 — L'exclusion de VictimId est intentionnelle mais non documentée ; une ligne de commentaire éviterait un futur « correctif » involontaire. Voir commentaire inline.

ilyanfraimbault pushed a commit that referenced this pull request Jun 18, 2026
A plain CREATE INDEX takes a write lock on match_participants for the whole build;
on the 35 GB prod table with the ingestor live that stalls all inserts/updates for
tens of minutes. Switch to CREATE INDEX CONCURRENTLY (suppressTransaction, since it
can't run in a transaction) so the build doesn't block writes.

Part of #553 / addresses the release review (#552).
…tion) (#553)

* perf(data): add partial index on match_participants (ChampionId, TeamPosition)

The champion-page reads (builds, matchups, scaling, leads, item-timings, roam)
all filter the tracked-account rows by champion + lane. Without a covering index
that was a sequential scan of the 35 GB match_participants table on every request
(the kind of regression #129 fixed for /champions). Adds a partial index keyed on
(ChampionId, TeamPosition) filtered to RiotAccountId IS NOT NULL — only the tracked
rows — so those filters seek instead of scan.

Also fixes a stale comment in TimelineIngestionService (the delete/reinsert run in
MatchIngestionProcess's ambient transaction, so they commit atomically) and notes
why KillPositionBuilder skips the victim.

Addresses the release review (#552).

* fix(data): build the champion-position index CONCURRENTLY

A plain CREATE INDEX takes a write lock on match_participants for the whole build;
on the 35 GB prod table with the ingestor live that stalls all inserts/updates for
tens of minutes. Switch to CREATE INDEX CONCURRENTLY (suppressTransaction, since it
can't run in a transaction) so the build doesn't block writes.

Part of #553 / addresses the release review (#552).

---------

Co-authored-by: ilyanfraimbault <ilyanfraimbault@gmail.com>
Comment thread backend/Api/Services/Champions/ChampionTimelineLeadsQueryService.cs
Comment thread backend/Api/Services/Champions/ChampionRoamQueryService.cs
Comment thread backend/Api/Services/Champions/ChampionRoamQueryService.cs
Comment thread web/server/routes/__sitemap__/urls.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review — Release: champion timeline analytics + API/ingestor hardening

Aucun point BLOQUANT identifié. Le scope est large (135 fichiers, +23k/−9k) mais le code est bien structuré, les requêtes EF Core sont paramétrées correctement (pas d'injection SQL), la désérialisation streaming est propre, et le middleware HSTS/CORS suit l'ordre canonique ASP.NET Core.

Suggestions (non bloquantes)

  1. ChampionTimelineLeadsQueryService.cs:73 — Jointure opponent sans Take(1) : en ranked ça ne pose pas de problème (TeamPosition unique par équipe), mais en queue non-ranked un doublon côté adverse gonflerait Games et biaserait toutes les moyennes sans avertissement. Guard défensif recommandé.

  2. ChampionRoamQueryService.cs:62ToListAsync() sans cap charge toutes les positions de kill en mémoire avant la classification C#. Limité aux comptes trackés, donc raisonnable aujourd'hui ; ajouter un .Take(50_000) préventif avant que le dataset ne grandisse.

  3. ChampionRoamQueryService.cs:101UTILITYBotLane uniquement : la river est comptée « hors-lane » pour le support, ce qui surestime mécaniquement son OutOfLaneShare. Décision de design acceptable, mais mérite un commentaire.

  4. web/server/routes/__sitemap__/urls.ts:41 — Séparateur - entre gameName et tagLine ambigu si gameName contient un tiret. Valide en pratique (les tagLines Riot n'ont pas de tiret), mais l'hypothèse devrait être documentée.

ilyanfraimbault pushed a commit that referenced this pull request Jun 18, 2026
Two comment-only clarifications from the #552 review:
- ChampionTimelineLeadsQueryService: document that the ranked-queue population
  guarantees exactly one lane opponent per row, so no Take(1) is needed to keep
  Games honest.
- sitemap urls source: document why the '{gameName}-{tagLine}' slug separator
  is unambiguous (Riot tagLines never contain '-') and matches the app-wide
  link convention.

Refs #554
…ollow-ups (#555)

Review follow-ups from the #552 release:
- MatchSnapshotWriter: ConcurrentBag instead of a pre-sized slot array (removes the uninitialized-slot footgun).
- Document the ranked-queue one-opponent invariant in ChampionTimelineLeadsQueryService.
- Document the sitemap profile-slug separator convention.

Closes #554
Comment thread backend/Api/Services/Champions/ChampionRoamQueryService.cs
var type = timelineEvent.Type;
if (timelineEvent.KillerId is > 0 && type.Equals("CHAMPION_KILL", StringComparison.OrdinalIgnoreCase))
{
Record(killTimestamps, timelineEvent.KillerId.Value, timelineEvent.TimestampMs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NON BLOQUANT — Seul le KillerId est comptabilisé dans les snapshots Kills ; les assistants (AssistingParticipantIds) ne sont pas enregistrés ici. Conséquence : le KillsDiff dans le chart Timeline Leads reflète uniquement les kills directs, pas les kill-participations. Pour un support ou un jungler qui roam pour assister, KillsDiff sera typiquement 0 même s'ils ont participé à tous les kills. Si c'est intentionnel, une ligne de commentaire dans MatchParticipantTimelineSnapshot.Kills le précisant éviterait toute confusion à la lecture de la donnée.

{
var normalizedPatch = string.IsNullOrWhiteSpace(patch)
? null
: PatchVersion.TryParse(patch, out var parsed) ? parsed.ToMajorMinor() : null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NON BLOQUANT — Les 4 nouveaux services (ItemTimings, Roam, Scaling, TimelineLeads) re-normalisent tous le patch via PatchVersion.TryParse en interne, alors que le controller appelle déjà ChampionQueryParameterNormalizer.NormalizePatch avant de déléguer. La re-normalisation est défensive, mais elle a un effet de bord : un patch invalide passé directement au service (par exemple dans un test d'intégration qui bypass le controller) sera silencieusement converti en null (= "tous les patchs") plutôt que de lever une erreur. Si la séparation controller/service doit être maintenue, documenter que le patch attendu en entrée est déjà normalisé (ou nommer le paramètre normalizedPatch) clarifierait le contrat.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review

Périmètre : champion timeline analytics (leads, scaling, item timings, roam), durcissement API/ingestor (HSTS, CORS fail-fast, streaming JSON, parallel fetches), fondations SEO, 4 migrations.

Points examinés

  • Logique des calculs analytiques : leads gold/CS/kills/level/damage, scaling index, item timings, roam share
  • Zone classifier (LolMap) et mapping Riot→DTO (RiotTimelineMapper)
  • Changement streaming JSON (GetFromJsonStreamingAsync / EnsureSuccessDrainingAsync) — gestion null body confirmée par test
  • Sécurité CORS fail-fast + HSTS (non-Development only, ordre middleware correct)
  • Health-check fail-fast en Production
  • SQL raw de ChampionItemTimingsQueryService — paramétrage EF Core correct, pas d'injection
  • Index IX_match_participants_champion_position_tracked + IX_match_participant_kill_positions_MatchId_ParticipantId présents
  • Couverture de tests : unit (LolMap, KillPositionBuilder, TimelineSnapshotBuilder, RiotTimelineMapper) + intégration pour chacun des 4 endpoints analytics et pour CORS/health-check

Aucun point bloquant identifié.

Suggestions (non bloquantes)

Voir les 3 commentaires inline :

  1. ChampionRoamQueryService L.64 — sémantique de Games : compte les matchs avec au moins une kill-participation avant 15 min, pas les games totales jouées. À documenter dans le read model.
  2. TimelineSnapshotBuilder L.37Kills dans les snapshots = kills directs uniquement (pas d'assists). Le KillsDiff du chart Timeline Leads est donc nul pour les supports/assistants. À préciser dans le doc-comment de MatchParticipantTimelineSnapshot.Kills si c'est intentionnel.
  3. Services — re-normalisation patch — les 4 services re-parsent patch alors que le controller l'a déjà normalisé. Pas de bug, mais le contrat d'entrée gagnerait à être documenté (ou le paramètre renommé normalizedPatch).

@ilyanfraimbault ilyanfraimbault merged commit 73ad074 into master Jun 18, 2026
18 checks passed
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