fix: align furniture construction rendering with BN data#138
Merged
Conversation
Owner
Author
|
@coderabbitai review |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
✅ Actions performedReview triggered.
|
Contributor
Summary by CodeRabbitRelease Notes
WalkthroughTypeScript type definitions now explicitly model furniture construction targets via dedicated Changes
Sequence DiagramsequenceDiagram
participant Component as UI Component
participant Helper as construction.ts<br/>(Helpers)
participant Data as Construction<br/>Data
participant Render as Rendered<br/>Output
Component->>Data: pass construction object
Component->>Helper: getConstructionPrerequisites(construction)
Helper->>Data: extract pre_terrain / pre_furniture
Data-->>Helper: raw field values
Helper->>Helper: map to ConstructionSurfaceTarget[]<br/>(explicit type + id)
Helper-->>Component: prerequisites array
Component->>Helper: getConstructionResults(construction)
Helper->>Data: extract post_terrain / post_furniture
Data-->>Helper: raw field values
Helper->>Helper: map to ConstructionSurfaceTarget[]<br/>(explicit type + id)
Helper-->>Component: results array
Component->>Component: iterate prerequisites
Component->>Render: render ItemLink per<br/>prerequisite.type/.id
Component->>Component: iterate results
Component->>Helper: isNullConstructionResult(result)?
Helper-->>Component: boolean (t_null / f_null check)
alt null result
Component->>Render: render "nothing" label
else valid result
Component->>Render: render ItemLink per<br/>result.type/.id
end
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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: 2
🤖 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/ToolQuality.svelte`:
- Around line 210-215: The UI currently outputs a raw "on" literal in the
prerequisites list (in the block that calls getConstructionPrerequisites and
renders ItemLink), which must be localized; replace the hardcoded "on" with a
localized string using t from `@transifex/native` (e.g., t('on')) or extract a
small shared renderer/helper to render the separator so the same localized
string is reused in ConstructionByproduct.svelte and ComponentOf.svelte; update
the occurrences around getConstructionPrerequisites/ItemLink and the matching
fragments in ConstructionByproduct and ComponentOf to use the t(...) call or the
shared component instead of the raw literal so all user-facing text is
localized.
In `@src/utils/construction.ts`:
- Around line 8-47: Add JSDoc comments for the public functions
getConstructionPrerequisites, getConstructionResults, and
isNullConstructionResult: for each function include a one-line description, a
`@param` describing the construction parameter shape (e.g., Pick<Construction,
"..."> and meaning of fields like pre_terrain/pre_furniture or
post_terrain/post_furniture), and a `@returns` describing the
ConstructionSurfaceTarget[] (or boolean for isNullConstructionResult) and what
the values represent (terrain vs furniture and null ids t_null/f_null). Keep the
comments brief and place them immediately above each function so IDEs and docs
can pick them up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b98890bd-4f20-45ce-a93b-dc9fa370e51e
📒 Files selected for processing (9)
src/types.tssrc/types/Construction.sveltesrc/types/Construction.test.tssrc/types/Furniture.sveltesrc/types/Furniture.test.tssrc/types/ToolQuality.sveltesrc/types/item/ComponentOf.sveltesrc/types/item/ConstructionByproduct.sveltesrc/utils/construction.ts
Comment on lines
+210
to
+215
| {@const prerequisites = getConstructionPrerequisites(f)} | ||
| {#if prerequisites.length} | ||
| on {#each prerequisites as prerequisite, i} | ||
| {#if i !== 0}, | ||
| {/if} | ||
| <ItemLink type={prerequisite.type} id={prerequisite.id} /> |
Contributor
There was a problem hiding this comment.
Localize the new prerequisite separator.
The raw on literal is now shipped in UI output, so translated builds will leak English here. The same fragment was copied into src/types/item/ConstructionByproduct.svelte (Lines 36-41) and src/types/item/ComponentOf.svelte (Lines 132-137 and 155-160); route it through t(...) or extract a shared renderer before this drifts further.
As per coding guidelines, "Use t from @transifex/native for all user-facing text".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/ToolQuality.svelte` around lines 210 - 215, The UI currently
outputs a raw "on" literal in the prerequisites list (in the block that calls
getConstructionPrerequisites and renders ItemLink), which must be localized;
replace the hardcoded "on" with a localized string using t from
`@transifex/native` (e.g., t('on')) or extract a small shared renderer/helper to
render the separator so the same localized string is reused in
ConstructionByproduct.svelte and ComponentOf.svelte; update the occurrences
around getConstructionPrerequisites/ItemLink and the matching fragments in
ConstructionByproduct and ComponentOf to use the t(...) call or the shared
component instead of the raw literal so all user-facing text is localized.
Comment on lines
+8
to
+47
| export function getConstructionPrerequisites( | ||
| construction: Pick<Construction, "pre_terrain" | "pre_furniture">, | ||
| ): ConstructionSurfaceTarget[] { | ||
| const prerequisites: ConstructionSurfaceTarget[] = []; | ||
|
|
||
| if (construction.pre_terrain) { | ||
| prerequisites.push({ id: construction.pre_terrain, type: "terrain" }); | ||
| } | ||
|
|
||
| if (construction.pre_furniture) { | ||
| prerequisites.push({ id: construction.pre_furniture, type: "furniture" }); | ||
| } | ||
|
|
||
| return prerequisites; | ||
| } | ||
|
|
||
| export function getConstructionResults( | ||
| construction: Pick<Construction, "post_terrain" | "post_furniture">, | ||
| ): ConstructionSurfaceTarget[] { | ||
| const results: ConstructionSurfaceTarget[] = []; | ||
|
|
||
| if (construction.post_terrain) { | ||
| results.push({ id: construction.post_terrain, type: "terrain" }); | ||
| } | ||
|
|
||
| if (construction.post_furniture) { | ||
| results.push({ id: construction.post_furniture, type: "furniture" }); | ||
| } | ||
|
|
||
| return results; | ||
| } | ||
|
|
||
| export function isNullConstructionResult( | ||
| result: ConstructionSurfaceTarget, | ||
| ): boolean { | ||
| return ( | ||
| (result.type === "terrain" && result.id === "t_null") || | ||
| (result.type === "furniture" && result.id === "f_null") | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Advisory: Consider JSDoc for public API surface.
Per coding guidelines, public functions warrant JSDoc documentation. Current signatures are self-documenting, but brief descriptions would enhance IDE telemetry and onboarding velocity for downstream consumers.
📝 Optional: Add JSDoc headers
+/**
+ * Extracts prerequisite surfaces (terrain/furniture) from a construction definition.
+ */
export function getConstructionPrerequisites(
construction: Pick<Construction, "pre_terrain" | "pre_furniture">,
): ConstructionSurfaceTarget[] {+/**
+ * Extracts result surfaces (terrain/furniture) from a construction definition.
+ */
export function getConstructionResults(
construction: Pick<Construction, "post_terrain" | "post_furniture">,
): ConstructionSurfaceTarget[] {+/**
+ * Returns true if the target represents a null/removal result (t_null or f_null).
+ */
export function isNullConstructionResult(
result: ConstructionSurfaceTarget,
): boolean {As per coding guidelines: "Use JSDoc comments for public functions and complex logic."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function getConstructionPrerequisites( | |
| construction: Pick<Construction, "pre_terrain" | "pre_furniture">, | |
| ): ConstructionSurfaceTarget[] { | |
| const prerequisites: ConstructionSurfaceTarget[] = []; | |
| if (construction.pre_terrain) { | |
| prerequisites.push({ id: construction.pre_terrain, type: "terrain" }); | |
| } | |
| if (construction.pre_furniture) { | |
| prerequisites.push({ id: construction.pre_furniture, type: "furniture" }); | |
| } | |
| return prerequisites; | |
| } | |
| export function getConstructionResults( | |
| construction: Pick<Construction, "post_terrain" | "post_furniture">, | |
| ): ConstructionSurfaceTarget[] { | |
| const results: ConstructionSurfaceTarget[] = []; | |
| if (construction.post_terrain) { | |
| results.push({ id: construction.post_terrain, type: "terrain" }); | |
| } | |
| if (construction.post_furniture) { | |
| results.push({ id: construction.post_furniture, type: "furniture" }); | |
| } | |
| return results; | |
| } | |
| export function isNullConstructionResult( | |
| result: ConstructionSurfaceTarget, | |
| ): boolean { | |
| return ( | |
| (result.type === "terrain" && result.id === "t_null") || | |
| (result.type === "furniture" && result.id === "f_null") | |
| ); | |
| } | |
| /** | |
| * Extracts prerequisite surfaces (terrain/furniture) from a construction definition. | |
| */ | |
| export function getConstructionPrerequisites( | |
| construction: Pick<Construction, "pre_terrain" | "pre_furniture">, | |
| ): ConstructionSurfaceTarget[] { | |
| const prerequisites: ConstructionSurfaceTarget[] = []; | |
| if (construction.pre_terrain) { | |
| prerequisites.push({ id: construction.pre_terrain, type: "terrain" }); | |
| } | |
| if (construction.pre_furniture) { | |
| prerequisites.push({ id: construction.pre_furniture, type: "furniture" }); | |
| } | |
| return prerequisites; | |
| } | |
| /** | |
| * Extracts result surfaces (terrain/furniture) from a construction definition. | |
| */ | |
| export function getConstructionResults( | |
| construction: Pick<Construction, "post_terrain" | "post_furniture">, | |
| ): ConstructionSurfaceTarget[] { | |
| const results: ConstructionSurfaceTarget[] = []; | |
| if (construction.post_terrain) { | |
| results.push({ id: construction.post_terrain, type: "terrain" }); | |
| } | |
| if (construction.post_furniture) { | |
| results.push({ id: construction.post_furniture, type: "furniture" }); | |
| } | |
| return results; | |
| } | |
| /** | |
| * Returns true if the target represents a null/removal result (t_null or f_null). | |
| */ | |
| export function isNullConstructionResult( | |
| result: ConstructionSurfaceTarget, | |
| ): boolean { | |
| return ( | |
| (result.type === "terrain" && result.id === "t_null") || | |
| (result.type === "furniture" && result.id === "f_null") | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/construction.ts` around lines 8 - 47, Add JSDoc comments for the
public functions getConstructionPrerequisites, getConstructionResults, and
isNullConstructionResult: for each function include a one-line description, a
`@param` describing the construction parameter shape (e.g., Pick<Construction,
"..."> and meaning of fields like pre_terrain/pre_furniture or
post_terrain/post_furniture), and a `@returns` describing the
ConstructionSurfaceTarget[] (or boolean for isNullConstructionResult) and what
the values represent (terrain vs furniture and null ids t_null/f_null). Keep the
comments brief and place them immediately above each function so IDEs and docs
can pick them up.
Show furniture constructions from post_furniture and remove legacy f_ prefix inference across construction views. Add focused regression tests and verify the furniture construction path in the browser. Refs #135
ushkinaz
force-pushed
the
codex/fix-135-furniture-construction
branch
from
1eab576 to
e388ca2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 49-55: The terrain test fixture object in Furniture.test.ts (the
terrain entry in the test data array) is missing required schema fields
`symbol`, `color`, and `move_cost`, which may break future strict validation;
update the terrain object in the test fixture to include these fields (e.g., add
`symbol`, `color`, and `move_cost`) so it conforms to the schema used by the
CBNData constructor/validation logic (refer to the terrain object in the test
data array and CBNData usage to locate where to update).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00d2b0e3-af7d-4e9e-8720-dcc6b8c5606e
📒 Files selected for processing (9)
src/types.tssrc/types/Construction.sveltesrc/types/Construction.test.tssrc/types/Furniture.sveltesrc/types/Furniture.test.tssrc/types/ToolQuality.sveltesrc/types/construction.tssrc/types/item/ComponentOf.sveltesrc/types/item/ConstructionByproduct.svelte
Comment on lines
+49
to
+55
| { | ||
| type: "terrain", | ||
| id: "t_pit", | ||
| name: "pit", | ||
| description: "A test pit.", | ||
| }, | ||
| ]); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test fixture incomplete but non-critical.
Terrain record at Lines 50-54 lacks required schema fields (symbol, color, move_cost). CBNData constructor may tolerate partial records for test purposes. If strict validation is later enforced, this fixture will require augmentation.
🔧 Optional: Complete terrain fixture
{
type: "terrain",
id: "t_pit",
name: "pit",
description: "A test pit.",
+ symbol: "0",
+ color: "brown",
+ move_cost: 10,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| type: "terrain", | |
| id: "t_pit", | |
| name: "pit", | |
| description: "A test pit.", | |
| }, | |
| ]); | |
| { | |
| type: "terrain", | |
| id: "t_pit", | |
| name: "pit", | |
| description: "A test pit.", | |
| symbol: "0", | |
| color: "brown", | |
| move_cost: 10, | |
| }, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/Furniture.test.ts` around lines 49 - 55, The terrain test fixture
object in Furniture.test.ts (the terrain entry in the test data array) is
missing required schema fields `symbol`, `color`, and `move_cost`, which may
break future strict validation; update the terrain object in the test fixture to
include these fields (e.g., add `symbol`, `color`, and `move_cost`) so it
conforms to the schema used by the CBNData constructor/validation logic (refer
to the terrain object in the test data array and CBNData usage to locate where
to update).
This was referenced Mar 22, 2026
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
post_furnitureinstead of terrain-only lookupsf_prefix inference from construction views and add focused regression testsTesting
pnpm lintpnpm checkpnpm vitest run src --exclude "src/all.*.test.ts" --exclude "src/__mod_tests__/**" --no-colorhttp://127.0.0.1:3000/stable/furniture/f_signCloses #135