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 <noreply@anthropic.com>
This commit is contained in:
@@ -205,6 +205,8 @@ describe('seedVisibleVerticalSlice', () => {
|
|||||||
key: 'wild-road-dog',
|
key: 'wild-road-dog',
|
||||||
name: 'Verwilderter Straßenhund',
|
name: 'Verwilderter Straßenhund',
|
||||||
level: 1,
|
level: 1,
|
||||||
|
silverMin: 0,
|
||||||
|
silverMax: 0,
|
||||||
artworkPath: '/images/monsters/wild-road-dog.png',
|
artworkPath: '/images/monsters/wild-road-dog.png',
|
||||||
iconPath: '/images/combat/icons/wild-road-dog-128.png',
|
iconPath: '/images/combat/icons/wild-road-dog-128.png',
|
||||||
}),
|
}),
|
||||||
@@ -212,6 +214,8 @@ describe('seedVisibleVerticalSlice', () => {
|
|||||||
key: 'charred-looter',
|
key: 'charred-looter',
|
||||||
name: 'Verkohlter Plünderer',
|
name: 'Verkohlter Plünderer',
|
||||||
level: 2,
|
level: 2,
|
||||||
|
silverMin: 0,
|
||||||
|
silverMax: 0,
|
||||||
artworkPath: '/images/monsters/charred-looter.png',
|
artworkPath: '/images/monsters/charred-looter.png',
|
||||||
iconPath: '/images/combat/icons/charred-looter-128.png',
|
iconPath: '/images/combat/icons/charred-looter-128.png',
|
||||||
}),
|
}),
|
||||||
|
|||||||
@@ -3,7 +3,11 @@ import { DataSource, EntityManager } from 'typeorm';
|
|||||||
import { Character } from '../characters/entities/character.entity';
|
import { Character } from '../characters/entities/character.entity';
|
||||||
import { CharacterRenownMilestone } from './entities/character-renown-milestone.entity';
|
import { CharacterRenownMilestone } from './entities/character-renown-milestone.entity';
|
||||||
import { RenownMilestoneDefinition } from './entities/renown-milestone-definition.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 {
|
import {
|
||||||
characterNotFound,
|
characterNotFound,
|
||||||
renownMilestoneAlreadyCompleted,
|
renownMilestoneAlreadyCompleted,
|
||||||
@@ -65,9 +69,13 @@ export class RenownService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const previousRenown = character.renown;
|
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(
|
const newRenown = Math.min(
|
||||||
RENOWN_MAX,
|
RENOWN_MAX,
|
||||||
previousRenown + milestone.renownReward,
|
Math.max(RENOWN_MIN, previousRenown + milestone.renownReward),
|
||||||
);
|
);
|
||||||
const renownGranted = newRenown !== previousRenown;
|
const renownGranted = newRenown !== previousRenown;
|
||||||
|
|
||||||
|
|||||||
@@ -21,8 +21,11 @@ const REPUTATION_RANKS: ReadonlyArray<{
|
|||||||
];
|
];
|
||||||
|
|
||||||
export function resolveReputationRank(reputation: number): ReputationRankInfo {
|
export function resolveReputationRank(reputation: number): ReputationRankInfo {
|
||||||
const index = REPUTATION_RANKS.findIndex(
|
// Clamp to the lowest rank: findIndex returns -1 for a negative total, and
|
||||||
(rank) => reputation >= rank.threshold,
|
// 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 rank = REPUTATION_RANKS[index];
|
||||||
const nextRank = index > 0 ? REPUTATION_RANKS[index - 1] : null;
|
const nextRank = index > 0 ? REPUTATION_RANKS[index - 1] : null;
|
||||||
|
|||||||
@@ -61,7 +61,7 @@ export class ReputationService {
|
|||||||
throw characterNotFound();
|
throw characterNotFound();
|
||||||
}
|
}
|
||||||
|
|
||||||
const faction = await factions.findOneBy({ key: factionKey });
|
const faction = await factions.findOneBy({ key: factionKey, enabled: true });
|
||||||
if (!faction) {
|
if (!faction) {
|
||||||
throw reputationFactionNotFound();
|
throw reputationFactionNotFound();
|
||||||
}
|
}
|
||||||
@@ -114,7 +114,7 @@ export class ReputationService {
|
|||||||
const scope: RepositoryScope = this.dataSource;
|
const scope: RepositoryScope = this.dataSource;
|
||||||
const factions = await scope
|
const factions = await scope
|
||||||
.getRepository(ReputationFaction)
|
.getRepository(ReputationFaction)
|
||||||
.find({ where: { enabled: true } });
|
.find({ where: { enabled: true }, order: { key: 'ASC' } });
|
||||||
const reputations = await scope
|
const reputations = await scope
|
||||||
.getRepository(CharacterReputation)
|
.getRepository(CharacterReputation)
|
||||||
.find({ where: { characterId } });
|
.find({ where: { characterId } });
|
||||||
|
|||||||
@@ -137,7 +137,7 @@ function combat(overrides: Partial<Combat> = {}): Combat {
|
|||||||
|
|
||||||
function createState(overrides: Partial<State> = {}): State {
|
function createState(overrides: Partial<State> = {}): State {
|
||||||
return {
|
return {
|
||||||
characters: [{ id: CHARACTER_ID, silver: 3 } as Character],
|
characters: [{ id: CHARACTER_ID, silver: 3, renown: 4 } as Character],
|
||||||
monsters: [
|
monsters: [
|
||||||
{
|
{
|
||||||
id: ASH_RAT_ID,
|
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', () => {
|
describe('idempotency', () => {
|
||||||
it('grants once and returns the same persisted reward on a repeat call', async () => {
|
it('grants once and returns the same persisted reward on a repeat call', async () => {
|
||||||
const state = createState();
|
const state = createState();
|
||||||
|
|||||||
@@ -139,12 +139,14 @@
|
|||||||
<section class="rewards" data-combat-rewards aria-label="Belohnungen">
|
<section class="rewards" data-combat-rewards aria-label="Belohnungen">
|
||||||
<h3 class="rewards__title">Belohnungen</h3>
|
<h3 class="rewards__title">Belohnungen</h3>
|
||||||
|
|
||||||
|
@if (rewards.silver) {
|
||||||
<dl class="rewards__currencies">
|
<dl class="rewards__currencies">
|
||||||
<div class="rewards__currency" data-reward-silver>
|
<div class="rewards__currency" data-reward-silver>
|
||||||
<dt>Silber</dt>
|
<dt>Silber</dt>
|
||||||
<dd>+{{ rewards.silver }}</dd>
|
<dd>+{{ rewards.silver }}</dd>
|
||||||
</div>
|
</div>
|
||||||
</dl>
|
</dl>
|
||||||
|
}
|
||||||
|
|
||||||
@if (rewards.items.length) {
|
@if (rewards.items.length) {
|
||||||
<h3 class="rewards__title">Beute</h3>
|
<h3 class="rewards__title">Beute</h3>
|
||||||
|
|||||||
@@ -540,6 +540,20 @@ describe('CombatPageComponent', () => {
|
|||||||
expect(element.querySelector('[data-reward-experience]')).toBeNull();
|
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 () => {
|
it('renders a dropped item with its icon, name, and rarity', async () => {
|
||||||
const fixture = await setup({
|
const fixture = await setup({
|
||||||
...wonWithRewards,
|
...wonWithRewards,
|
||||||
|
|||||||
@@ -61,7 +61,7 @@ const equipment: EquipmentResponse = {
|
|||||||
const character: CharacterResponse = {
|
const character: CharacterResponse = {
|
||||||
id: 'character-1',
|
id: 'character-1',
|
||||||
name: 'Aric Duskwalker',
|
name: 'Aric Duskwalker',
|
||||||
renown: 0,
|
renown: 1,
|
||||||
silver: 0,
|
silver: 0,
|
||||||
currentHp: 100,
|
currentHp: 100,
|
||||||
maxHp: 100,
|
maxHp: 100,
|
||||||
|
|||||||
@@ -87,8 +87,10 @@ export class WorldStore implements OnDestroy {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Re-reads the character from the server, e.g. after a combat granted XP and
|
* Re-reads the character from the server, e.g. after a combat granted item
|
||||||
* silver. Never mutates the values locally: the server owns them (spec §35).
|
* 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
|
* A failed refresh leaves the last known character in place rather than
|
||||||
* blanking the HUD.
|
* blanking the HUD.
|
||||||
*/
|
*/
|
||||||
|
|||||||
Reference in New Issue
Block a user