feat(types): align terrain and furniture pry handling with BN data#163
Merged
feat(types): align terrain and furniture pry handling with BN data#163
Conversation
Contributor
Summary by CodeRabbit
WalkthroughPrying mechanics restructured: pry-specific types extracted (PryDataCommon, TerrainPryData, FurniturePryData); ActivityDataCommon gains centralized Changes
Sequence Diagram(s)sequenceDiagram
participant Viewer as Viewer
participant TerrainComp as Terrain/Furniture Component
participant PryComp as TerFurnPry
participant DataCtx as CBNData
participant ThingLink as ThingLink
Viewer->>TerrainComp: render item (reads `item.pry`)
TerrainComp->>PryComp: pass act={item.pry}, resultType
PryComp->>DataCtx: lookup translations/icons/item metadata
PryComp->>ThingLink: render pry/break items and "Becomes" links
ThingLink-->>Viewer: clickable UI entries
PryComp-->>Viewer: pry details (difficulty, flags, items, result)
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
It took only 3 years to support, yay |
ushkinaz
force-pushed
the
feat/update-pry-property-handling
branch
from
93fa94c to
020692d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types/Furniture.test.ts`:
- Around line 48-54: The assertions in Furniture.test.ts are too
permissive—calls like getByText("7") and getByText("prying") can collide with
other UI text; update the test to scope those checks to the correct definition
term/value pair instead of global text queries. Locate the "Difficulty" <dt> (or
the "Requires"/"Becomes" labels used in the test) and query its sibling <dd>
(e.g., use within(getByText("Difficulty").nextElementSibling) or find the
corresponding <dd> via the <dt>/<dd> pairing) and assert the expected "7" and
"prying" appear only inside that scoped node; replace the global getByText("7")
and getByText("prying") calls with these scoped queries and keep the
queryByText("Duration") assertion as-is.
In `@src/types/TerFurnPry.svelte`:
- Around line 82-83: The "Noisy" line currently reduces PryDataCommon.noise to a
boolean via (pry.noise ?? 0) > 0; change it to show the actual numeric noise
value when present (e.g., render t("Noisy") with {(pry.noise ?? 0) > 0 ?
pry.noise : t("No")} so you keep the numeric volume instead of Yes/No), and
optionally surface pry.break_noise near the Breakable field (e.g., add
break_noise display alongside the Breakable dd) so both noise volume and
break_noise are visible.
- Around line 47-52: The function formatProbability currently disambiguates
percent vs fraction by magnitude; replace that with explicit semantic checks:
keep the early hide for prob == null or prob === 1, then use
Number.isInteger(prob) to detect integer-percent encoding and return `${prob}%`,
otherwise treat it as a 0..1 fraction and return formatPercent(prob); update the
function formatProbability to use these checks so encoding is unambiguous
(consider whether prob === 0 should also be treated as a hide candidate and
adjust the initial hide condition if intended).
- Around line 24-29: pryItems and breakItems are currently produced by filtering
with isItemEntry which drops non-{item} variants and loses
group/distribution/collection entries; instead, feed the original arrays through
the same flatten pipeline used elsewhere (e.g., data.flattenItemGroup({ subtype:
"collection", entries: ... })) so nested groups are resolved to concrete items.
Replace the untrack(() => (act.pry_items ?? []).filter(isItemEntry)) and
untrack(() => (act.break_items ?? []).filter(isItemEntry)) with untrack wrappers
that call data.flattenItemGroup({ subtype: "collection", entries: act.pry_items
?? [] }) and likewise for act.break_items; remove the isItemEntry type-narrowing
function or stop using it for these two variables so structural variants are
preserved.
- Around line 94-129: Extract the duplicated dt/dd/ul rendering into a Svelte
{`#snippet`} (e.g., ItemList) that accepts props for the label (string) and the
source array (items), and moves the shared logic that iterates over items,
computes amount via formatAmount(entry.count, true) ??
formatAmount(entry.charges), computes probability via
formatProbability(entry.prob), and renders ThingLink(id=entry.item, type="item",
showIcon=false) plus conditional amount/probability. Replace the two repeated
blocks that reference pryItems and breakItems with two calls to the new snippet,
passing the corresponding label (t("Pry Items", {...}) / t("Break Items",
{...})) and items (pryItems / breakItems) so the formatting logic is centralized
and not duplicated.
In `@src/types/Terrain.test.ts`:
- Around line 32-34: The test uses integer-percent values like prob: 25 which
violates the codebase convention where formatPercent()/formatProbability()
expect fractions (0–1); update the test entries (both the item at line with {
item: "2x4", prob: 25 } and the similar entry at line ~78) to use fractional
probabilities (e.g., prob: 0.25) so they match spawnLocations.ts normalization
and avoid relying on the defensive prob > 1 branch in formatProbability();
confirm pry_items/break_items test fixtures are all converted to 0–1 fractions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecf3e783-3952-4d5d-b63d-8d44f0efa621
📒 Files selected for processing (7)
src/types.tssrc/types/Furniture.sveltesrc/types/Furniture.test.tssrc/types/TerFurnActivity.sveltesrc/types/TerFurnPry.sveltesrc/types/Terrain.sveltesrc/types/Terrain.test.ts
…e in Terrain tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types/TerFurnPry.svelte`:
- Around line 67-86: The break items list is only rendered under the
pry.breakable guard which can silently hide break_items supplied via copy-from;
update the template to decouple breakItems rendering from pry.breakable (or add
an explicit assertion/log) so breakItems are shown or flagged even when
pry.breakable is false/undefined. Specifically, move the {`#if` breakItems.length}
block out of the {`#if` pry.breakable} block (or add a
console.warn/processLogger/error when breakItems.length && !pry.breakable) and
reference pry.breakable, breakItems (from break_items), and the ThingLink list
to locate and fix the logic.
- Around line 61-66: The Alarm and Noise rows currently always render ("No"/0)
when pry.alarm/pry.noise are missing; change the template to only render those
<dt>/<dd> pairs when the values are meaningful by wrapping the Alarm row in a
Svelte conditional {`#if` pry.alarm}...{/if} and the Noise row in {`#if` pry.noise
&& pry.noise > 0}...{/if} (keeping the existing t(...) usage), mirroring how
pry.breakable is gated so empty/zero values don't produce low-signal rows.
In `@src/types/Terrain.test.ts`:
- Around line 64-77: The test uses global getByText/queryByText calls (e.g.,
expecting "11" and matching content.includes("0–2")) which can collide with
other numeric/text nodes; instead mirror the Furniture.test.ts approach: locate
each <dt> label (e.g., the dt that contains "Difficulty", "Requires", "Pry
Items", "Break Items", "Alarm", etc.) and use within(dt.nextElementSibling) to
scope assertions against the paired <dd> so the checks for "11", "0–2", "2x4",
"nail", "open door", "manhole cover" are anchored to the correct field and avoid
accidental matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18c1a566-714b-4999-96d9-31c97fdb9a25
📒 Files selected for processing (5)
src/types/Furniture.test.tssrc/types/TerFurnActivity.sveltesrc/types/TerFurnPry.sveltesrc/types/Terrain.test.tssrc/types/ThingLink.svelte
Comment on lines
+64
to
+77
| expect(getByText("Requires")).toBeTruthy(); | ||
| expect(getByText("prying")).toBeTruthy(); | ||
| expect(getByText("Difficulty")).toBeTruthy(); | ||
| expect(getByText("11")).toBeTruthy(); | ||
| expect(getByText("Alarm")).toBeTruthy(); | ||
| expect(getByText("Breakable")).toBeTruthy(); | ||
| expect(getByText("open door")).toBeTruthy(); | ||
| expect(getByText("Pry Items")).toBeTruthy(); | ||
| expect(getByText("manhole cover")).toBeTruthy(); | ||
| expect(getByText("Break Items")).toBeTruthy(); | ||
| expect(getByText("2x4")).toBeTruthy(); | ||
| expect(getByText("nail")).toBeTruthy(); | ||
| expect(getByText((content) => content.includes("0–2"))).toBeTruthy(); | ||
| expect(queryByText("Duration")).toBeNull(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assertion surface permissive. Partial collision vector remains.
Unlike the parallel Furniture.test.ts pattern (scoped via within(<dt>.nextElementSibling)), these global getByText matchers do not anchor to their <dt> labels. "11" in particular risks collision with adjacent numeric fields (difficulty, noise, break_noise, pry_quality) and future UI additions. The content.includes("0–2") predicate on line 76 matches any ancestor containing the substring—including the wrapping <dd>/<ul>.
Recommend parity with the furniture test: locate each <dt> and assert within its paired <dd>. Non-blocking; current fixtures pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/Terrain.test.ts` around lines 64 - 77, The test uses global
getByText/queryByText calls (e.g., expecting "11" and matching
content.includes("0–2")) which can collide with other numeric/text nodes;
instead mirror the Furniture.test.ts approach: locate each <dt> label (e.g., the
dt that contains "Difficulty", "Requires", "Pry Items", "Break Items", "Alarm",
etc.) and use within(dt.nextElementSibling) to scope assertions against the
paired <dd> so the checks for "11", "0–2", "2x4", "nail", "open door", "manhole
cover" are anchored to the correct field and avoid accidental matches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This updates terrain and furniture pry handling to match the actual BN data model instead of the legacy
pryingabstraction that no longer exists upstream. It introduces dedicated pry types, routes pry rendering through a separateTerFurnPrycomponent, and keeps non-pry map activities (boltcut,hacksaw,oxytorch) on the simpler activity path.It also surfaces pry-specific item outcomes directly in the UI, including both items gained on a successful pry and items dropped when the target breaks.
Why
The old code still expected a
pryingproperty plus nestedprying_data, which was a leftover from older CDD-era assumptions. Real BN terrain and furniture data uses apryobject with different fields and different semantics.Keeping the dead abstraction in place made the UI and types lie about the underlying data and forced pry handling into a generic activity component that did not really fit it.
What Changed
Types
PryDataCommon,TerrainPryData, andFurniturePryDatapryingusage withpryresultintoActivityDataCommonfor the non-pry activity pathUI
src/types/TerFurnPry.sveltesrc/types/TerFurnActivity.sveltefocused onboltcut,hacksaw, andoxytorchitem.pryResulttoBecomesfor clearer map-object wordingPry ItemsBreak ItemsTests
What's Improved
Compromises / Trade-offs
TerFurnActivityTerFurnPryhas its own formatting helpers for amounts and probabilities because pry item lists are not shaped like flattened activity byproductsBehavior Changes
User-visible
pryinstead of the removedpryingpropertyBecomesinstead ofResultInternal / reviewer-relevant
pry_resultfields more directlyReviewer Notes / Potential Triggers
pryingpath rather than preserving compatibility with a schema that no longer exists in BN databreak_itemshandles raw percentage-style values from current pry dataTerFurnActivityandTerFurnPryis structural, not cosmetic: pry data has different fields and different UI needsCritical Path For Manual Testing
Becomes,Pry Items, andBreak Itemswhen presentVerification
pnpm vitest run --no-color src/types/Terrain.test.ts src/types/Furniture.test.tspnpm lintpnpm checkRefs
Original PR cataclysmbn/Cataclysm-BN#837