Fix terrain and furniture deconstruction tool details#143
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughConstruction gained an optional Changes
Possibly related issues
Possibly related PRs
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 Tip You can disable poems in the walkthrough.Disable the |
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/Construction.svelte`:
- Around line 75-81: The code redundantly uses the null-coalescing fallback when
iterating required_skills even though the outer guard checks
construction.required_skills?.length; update the {`#each`} to iterate directly
over construction.required_skills (e.g., {`#each` construction.required_skills as
[id, level], i}) and remove the ?? [] so ItemLink and the surrounding logic use
the confirmed array instead of an unreachable fallback.
In `@src/types/Furniture.test.ts`:
- Around line 132-137: Update the test to also assert the negative case when
includeRequires is false: render the component (WithData) with
includeRequires={false} and use queryByText (instead of getByText) to assert
that the "Requires" heading and/or "Tools Required" entries (e.g., "hammering",
"chiseling", "prying", "screw driving") are absent by expecting queryByText(...)
toBeNull(); this verifies the includeRequires prop on the component is honored.
In `@src/types/Terrain.test.ts`:
- Around line 48-51: Add a negative assertion to the test to verify the
includeRequires={false} prop hides the "Requires" section: destructure
queryByText from the render result (alongside getByText), render the component
with includeRequires={false}, and assert
expect(queryByText("Requires")).toBeNull(); so the test confirms the absence of
the "Requires" section (reference render result destructuring and the
includeRequires prop in the test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 062140dd-f518-4acd-8555-76f42b8e2eea
📒 Files selected for processing (5)
src/types/Construction.sveltesrc/types/Furniture.sveltesrc/types/Furniture.test.tssrc/types/Terrain.sveltesrc/types/Terrain.test.ts
…ements logic - Removed redundant nullish coalescing operator in `required_skills` iteration. - Added tests for "hidden requirements" in Terrain and Furniture components.
Summary
check_deconstructconstructionsConstructionrendering while hiding redundant prerequisite links on these pagesnonewhen a construction has no required skills instead of leaving the field emptyTesting
pnpm vitest run --no-color src/types/Furniture.test.ts src/types/Terrain.test.ts src/types/Construction.test.ts