From 23ed527eeac4608524380df3a4a81718934afb2b Mon Sep 17 00:00:00 2001 From: Bastian Wagner Date: Fri, 21 Aug 2026 18:50:30 +0200 Subject: [PATCH] fix: address the final whole-branch review's findings The branch review approved the slice with no blocking findings. These are the substantive non-blocking ones. N1, the one no per-task review could see: every seeded monster rolls silverMin/silverMax = 0 (R7), so the victory screen showed "Silber +0" after every fight in the shipped game. Three tasks were each correct in isolation -- the mechanism stays, the values are zero, the field still exists -- and the composite was wrong. Now conditional, with a test. This does not contradict R16: R16 deleted the XP block because the field ceased to exist, leaving nothing to hide. Silver still exists and can be non-zero, so a conditional is the right tool. N2: world.store.ts still said a combat "granted XP and silver". Same false-fact-in-a-comment defect fixed in b5bcd50, one file over. N4: resolveReputationRank threw a TypeError on negative input, since findIndex returns -1 and REPUTATION_RANKS[-1] is undefined. Unreachable today, but grantReputation is public and accepts any number. N5/N6: grantReputation resolved factions without the enabled filter the read path applies, so a disabled faction could accumulate invisible reputation -- "disabled" was not actually a kill switch. The dense read also had no ORDER BY, so the list could reorder between requests. N8: design 13 requires silver stay 0 for every seeded monster; only two of four were pinned. Re-adding silver to the others would have shipped silently. N9: spec 36's "a normal kill grants no Renown" had no test. It was structurally guaranteed but unasserted -- now locked down against a later slice wiring renown into combat. N10: an impossible renown: 0 fixture, and RENOWN_MIN exported but never used to clamp the floor. API 268/268, web 230/230, API build zero errors. Co-Authored-By: Claude Opus 5 --- .../database/seeds/vertical-slice.seed.spec.ts | 4 ++++ apps/api/src/renown/renown.service.ts | 12 ++++++++++-- apps/api/src/reputation/reputation-rank.ts | 7 +++++-- apps/api/src/reputation/reputation.service.ts | 4 ++-- .../src/rewards/combat-reward.service.spec.ts | 16 +++++++++++++++- .../combat-page/combat-page.component.html | 14 ++++++++------ .../combat-page/combat-page.component.spec.ts | 14 ++++++++++++++ .../inventory/inventory-page.component.spec.ts | 2 +- apps/web/src/app/features/world/world.store.ts | 6 ++++-- 9 files changed, 63 insertions(+), 16 deletions(-) diff --git a/apps/api/src/database/seeds/vertical-slice.seed.spec.ts b/apps/api/src/database/seeds/vertical-slice.seed.spec.ts index 84f9a6a..dd2a036 100644 --- a/apps/api/src/database/seeds/vertical-slice.seed.spec.ts +++ b/apps/api/src/database/seeds/vertical-slice.seed.spec.ts @@ -205,6 +205,8 @@ describe('seedVisibleVerticalSlice', () => { key: 'wild-road-dog', name: 'Verwilderter Straßenhund', level: 1, + silverMin: 0, + silverMax: 0, artworkPath: '/images/monsters/wild-road-dog.png', iconPath: '/images/combat/icons/wild-road-dog-128.png', }), @@ -212,6 +214,8 @@ describe('seedVisibleVerticalSlice', () => { key: 'charred-looter', name: 'Verkohlter Plünderer', level: 2, + silverMin: 0, + silverMax: 0, artworkPath: '/images/monsters/charred-looter.png', iconPath: '/images/combat/icons/charred-looter-128.png', }), diff --git a/apps/api/src/renown/renown.service.ts b/apps/api/src/renown/renown.service.ts index 9850087..0f420a0 100644 --- a/apps/api/src/renown/renown.service.ts +++ b/apps/api/src/renown/renown.service.ts @@ -3,7 +3,11 @@ import { DataSource, EntityManager } from 'typeorm'; import { Character } from '../characters/entities/character.entity'; import { CharacterRenownMilestone } from './entities/character-renown-milestone.entity'; import { RenownMilestoneDefinition } from './entities/renown-milestone-definition.entity'; -import { RENOWN_BASE_STATS, RENOWN_MAX } from './renown-base-stats'; +import { + RENOWN_BASE_STATS, + RENOWN_MAX, + RENOWN_MIN, +} from './renown-base-stats'; import { characterNotFound, renownMilestoneAlreadyCompleted, @@ -65,9 +69,13 @@ export class RenownService { } const previousRenown = character.renown; + // Clamp both ends. The ceiling is the power curve's last row; the floor + // matters because renown_reward carries no CHECK, so bad content data + // would otherwise surface as a raw Postgres constraint violation instead + // of a clamped value. const newRenown = Math.min( RENOWN_MAX, - previousRenown + milestone.renownReward, + Math.max(RENOWN_MIN, previousRenown + milestone.renownReward), ); const renownGranted = newRenown !== previousRenown; diff --git a/apps/api/src/reputation/reputation-rank.ts b/apps/api/src/reputation/reputation-rank.ts index f7fe130..a09c534 100644 --- a/apps/api/src/reputation/reputation-rank.ts +++ b/apps/api/src/reputation/reputation-rank.ts @@ -21,8 +21,11 @@ const REPUTATION_RANKS: ReadonlyArray<{ ]; export function resolveReputationRank(reputation: number): ReputationRankInfo { - const index = REPUTATION_RANKS.findIndex( - (rank) => reputation >= rank.threshold, + // Clamp to the lowest rank: findIndex returns -1 for a negative total, and + // REPUTATION_RANKS[-1] would throw rather than return a rank. + const index = Math.max( + 0, + REPUTATION_RANKS.findIndex((rank) => reputation >= rank.threshold), ); const rank = REPUTATION_RANKS[index]; const nextRank = index > 0 ? REPUTATION_RANKS[index - 1] : null; diff --git a/apps/api/src/reputation/reputation.service.ts b/apps/api/src/reputation/reputation.service.ts index 0b41b45..a1e3503 100644 --- a/apps/api/src/reputation/reputation.service.ts +++ b/apps/api/src/reputation/reputation.service.ts @@ -61,7 +61,7 @@ export class ReputationService { throw characterNotFound(); } - const faction = await factions.findOneBy({ key: factionKey }); + const faction = await factions.findOneBy({ key: factionKey, enabled: true }); if (!faction) { throw reputationFactionNotFound(); } @@ -114,7 +114,7 @@ export class ReputationService { const scope: RepositoryScope = this.dataSource; const factions = await scope .getRepository(ReputationFaction) - .find({ where: { enabled: true } }); + .find({ where: { enabled: true }, order: { key: 'ASC' } }); const reputations = await scope .getRepository(CharacterReputation) .find({ where: { characterId } }); diff --git a/apps/api/src/rewards/combat-reward.service.spec.ts b/apps/api/src/rewards/combat-reward.service.spec.ts index 7acce1e..220bbb7 100644 --- a/apps/api/src/rewards/combat-reward.service.spec.ts +++ b/apps/api/src/rewards/combat-reward.service.spec.ts @@ -137,7 +137,7 @@ function combat(overrides: Partial = {}): Combat { function createState(overrides: Partial = {}): State { return { - characters: [{ id: CHARACTER_ID, silver: 3 } as Character], + characters: [{ id: CHARACTER_ID, silver: 3, renown: 4 } as Character], monsters: [ { id: ASH_RAT_ID, @@ -348,6 +348,20 @@ describe('CombatRewardService', () => { }); }); + it('grants no renown for a normal monster kill', async () => { + const state = createState(); + const loot = fakeLoot({ itemDefinitionId: BANDIT_BLADE, quantity: 1 }); + const subject = service(state, loot, fixedRandom(0)); + + await subject.grantVictoryRewards(fakeManager(state), combat()); + + // Renown comes from milestones only (spec §36). Killing things must never + // move it -- that is the whole point of replacing XP with Renown, so this + // asserts the prohibition rather than trusting that nothing wired it up. + expect(state.characters[0].renown).toBe(4); + expect(state.characters[0].silver).toBe(7); + }); + describe('idempotency', () => { it('grants once and returns the same persisted reward on a repeat call', async () => { const state = createState(); diff --git a/apps/web/src/app/features/combat/combat-page/combat-page.component.html b/apps/web/src/app/features/combat/combat-page/combat-page.component.html index 29bd2c1..7453aff 100644 --- a/apps/web/src/app/features/combat/combat-page/combat-page.component.html +++ b/apps/web/src/app/features/combat/combat-page/combat-page.component.html @@ -139,12 +139,14 @@

Belohnungen

-
-
-
Silber
-
+{{ rewards.silver }}
-
-
+ @if (rewards.silver) { +
+
+
Silber
+
+{{ rewards.silver }}
+
+
+ } @if (rewards.items.length) {

Beute

diff --git a/apps/web/src/app/features/combat/combat-page/combat-page.component.spec.ts b/apps/web/src/app/features/combat/combat-page/combat-page.component.spec.ts index 8b92845..7edd426 100644 --- a/apps/web/src/app/features/combat/combat-page/combat-page.component.spec.ts +++ b/apps/web/src/app/features/combat/combat-page/combat-page.component.spec.ts @@ -540,6 +540,20 @@ describe('CombatPageComponent', () => { expect(element.querySelector('[data-reward-experience]')).toBeNull(); }); + it('hides the silver tile entirely when the kill granted none', async () => { + // Every monster seeded in this slice rolls silverMin/silverMax = 0 (design + // R7), so without this guard the victory screen advertises "Silber +0" + // after every single fight in the shipped content. + const fixture = await setup({ + ...wonWithRewards, + rewards: { silver: 0, items: [] }, + }); + const element = fixture.nativeElement as HTMLElement; + + expect(element.querySelector('[data-reward-silver]')).toBeNull(); + expect(element.querySelector('[data-combat-rewards]')).toBeTruthy(); + }); + it('renders a dropped item with its icon, name, and rarity', async () => { const fixture = await setup({ ...wonWithRewards, diff --git a/apps/web/src/app/features/inventory/inventory-page.component.spec.ts b/apps/web/src/app/features/inventory/inventory-page.component.spec.ts index 7963599..058db2a 100644 --- a/apps/web/src/app/features/inventory/inventory-page.component.spec.ts +++ b/apps/web/src/app/features/inventory/inventory-page.component.spec.ts @@ -61,7 +61,7 @@ const equipment: EquipmentResponse = { const character: CharacterResponse = { id: 'character-1', name: 'Aric Duskwalker', - renown: 0, + renown: 1, silver: 0, currentHp: 100, maxHp: 100, diff --git a/apps/web/src/app/features/world/world.store.ts b/apps/web/src/app/features/world/world.store.ts index 9f7ce4c..e40e775 100644 --- a/apps/web/src/app/features/world/world.store.ts +++ b/apps/web/src/app/features/world/world.store.ts @@ -87,8 +87,10 @@ export class WorldStore implements OnDestroy { } /** - * Re-reads the character from the server, e.g. after a combat granted XP and - * silver. Never mutates the values locally: the server owns them (spec §35). + * Re-reads the character from the server, e.g. after a combat granted item + * drops. Renown comes from milestones, and silver reaches the player through + * turn-ins -- a kill grants neither. + * Never mutates the values locally: the server owns them (spec §35). * A failed refresh leaves the last known character in place rather than * blanking the HUD. */