feat(openworlds): add attributed icon registry - #182
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (12)
Cache: Disabled due to Reviews > Disable Cache setting 📝 WalkthroughWalkthroughAdds a local OpenWorlds semantic icon registry (manifest, aliases, resolver, React component), CSS mask rendering, server MIME mapping, attribution docs with CI checks, and integrates registry icons into Chrome/UI screens with tests verifying local assets and MIME correctness. ChangesOpenWorlds Icon Registry System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 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. ✨ Finishing Touches📝 Generate docstrings
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/license_check.py`:
- Around line 80-87: Guard the file reads for attr_path.read_text and
notice_path.read_text to avoid OSError crashes: wrap the
attr_path.read_text(encoding="utf-8") assignment (used to set attr_text) in a
try/except that catches OSError (or Exception), appends a descriptive error to
errors (e.g., "Failed reading ATTRIBUTION.md: {err}"), and sets attr_text = ""
on failure; do the same for notice_path.read_text(encoding="utf-8") that sets
notice_text so a failed read yields notice_text = "" and an errors entry instead
of raising.
In `@viewer/openworlds/icon-registry.jsx`:
- Around line 106-112: The resolveOpenWorldsIconId function only checks the raw
key against OPENWORLDS_ICON_MANIFEST, so case-variant semantic IDs like
"COMBAT.ATTACK" fail; after computing normalized (const normalized =
raw.toLowerCase().replace(/_/g, "-")), also check
OPENWORLDS_ICON_MANIFEST[normalized] and return normalized if present, before
falling back to OPENWORLDS_ICON_ALIASES[normalized] or "". Update
resolveOpenWorldsIconId to perform that extra manifest lookup using the
normalized key (referencing resolveOpenWorldsIconId, OPENWORLDS_ICON_MANIFEST,
and OPENWORLDS_ICON_ALIASES).
In `@viewer/openworlds/screen-map.jsx`:
- Around line 362-365: screen-map.jsx renders window.OpenWorldsIcon
unconditionally (in the BrassButton icons for "atlas.travel" and
"quest.scroll"); guard these renders the same way other screens do by checking
the registry before rendering the icon (use optional chaining and the .has
method on window.OpenWorldsIcon, e.g.
window.OpenWorldsIcon?.has?.('atlas.travel') and
window.OpenWorldsIcon?.has?.('quest.scroll')), and only render
<window.OpenWorldsIcon ... /> when the check passes, otherwise render the button
text only (preserve existing props like size, id references to "atlas.travel"
and "quest.scroll" and behavior tied to onTravel/onMark, busyTravel, selected,
travelDisabled).
In `@viewer/openworlds/screen-table.jsx`:
- Around line 246-249: The dice buttons (call sites using requestRoll and
actionById) currently reference window.OpenWorldsIcon directly; wrap the icon
usage with the same guard used elsewhere (e.g.,
window.OpenWorldsIcon?.has?.('dice.d20') or window.OpenWorldsIcon?) and only
render the <OpenWorldsIcon> when available (otherwise render just the label like
"d20" or no icon) so the buttons (and the chronicle CTA) won't crash when the
icon registry is missing; update the elements that render window.OpenWorldsIcon
for dice.d20, dice.roll, etc., to check availability before rendering the icon.
In `@viewer/openworlds/styles.css`:
- Around line 128-129: CSS uses the camel-cased keyword "currentColor" for the
color and background properties, which violates stylelint's value-keyword-case
rule; update the "color" and "background" declarations that currently use
"currentColor" to the lowercase "currentcolor" so both properties conform to the
lint rule (search for the "color" and "background" declarations in the shown
diff to locate and change them).
🪄 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: afe79e98-a107-4f7a-8e59-8d6ca5a87b5e
⛔ Files ignored due to path filters (12)
viewer/openworlds/assets/icons/game-icons/delapouite/coins.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/delapouite/dice-twenty-faces-twenty.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/delapouite/health-potion.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/delapouite/rolling-dices.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/delapouite/tavern-sign.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/lorc/campfire.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/lorc/compass.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/lorc/locked-chest.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/lorc/open-book.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/lorc/sword-clash.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/lorc/tied-scroll.svgis excluded by!**/*.svgviewer/openworlds/assets/icons/game-icons/willdabeast/round-shield.svgis excluded by!**/*.svg
📒 Files selected for processing (12)
THIRD_PARTY_NOTICES.mdscripts/license_check.pyviewer/openworlds/assets/icons/ATTRIBUTION.mdviewer/openworlds/chrome.jsxviewer/openworlds/icon-registry.jsxviewer/openworlds/index.htmlviewer/openworlds/screen-combat.jsxviewer/openworlds/screen-map.jsxviewer/openworlds/screen-table.jsxviewer/openworlds/styles.cssviewer/server.pyviewer/tests/test_openworlds_static.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
viewer/openworlds/icon-registry.jsx (1)
viewer/openworlds/vendor/babel-standalone-7.29.0.min.js (1)
id(4-4)
viewer/openworlds/screen-map.jsx (1)
viewer/openworlds/chrome.jsx (1)
BrassButton(242-249)
🪛 Stylelint (17.12.0)
viewer/openworlds/styles.css
[error] 128-128: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
[error] 129-129: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🔇 Additional comments (8)
viewer/openworlds/index.html (1)
21-21: LGTM!viewer/server.py (1)
61-61: LGTM!THIRD_PARTY_NOTICES.md (1)
42-56: LGTM!viewer/openworlds/assets/icons/ATTRIBUTION.md (1)
1-31: LGTM!viewer/openworlds/chrome.jsx (1)
60-63: LGTM!Also applies to: 232-236
viewer/openworlds/screen-combat.jsx (1)
194-198: LGTM!Also applies to: 479-481, 498-498
viewer/openworlds/screen-table.jsx (1)
319-319: LGTM!Also applies to: 338-338, 417-419, 427-427, 498-500, 523-523
viewer/tests/test_openworlds_static.py (1)
5-5: LGTM!Also applies to: 109-135
Member
Author
|
Pushed
Focused validation from python3 -m unittest viewer.tests.test_openworlds_static -q
python3 -m py_compile viewer/server.py scripts/license_check.py
python3 scripts/license_check.py
node <vendored Babel transform smoke for touched JSX>
git diff --checkAll passed locally. CI is re-running on the new head. |
100yenadmin
marked this pull request as ready for review
May 26, 2026 15:25
100yenadmin
marked this pull request as draft
May 26, 2026 16:05
100yenadmin
force-pushed
the
openworlds/icon-registry
branch
from
May 26, 2026 22:15
c82ef1a to
77ff6dd
Compare
100yenadmin
marked this pull request as ready for review
May 26, 2026 22:15
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
Adds the first roadmap-squeeze PR lane from #174: an attributed local icon registry for OpenWorlds gameplay surfaces.
What changed:
viewer/openworlds/icon-registry.jsxwith stable semantic icon ids such ascombat.attack,dice.d20,atlas.travel,quest.scroll,camp.rest,inventory.potion,economy.coins, andparty.shield.game-icons/icons, not the full icon repo.viewer/openworlds/assets/icons/ATTRIBUTION.mdand top-levelTHIRD_PARTY_NOTICES.mdcoverage for Game Icons CC-BY-3.0 attribution.scripts/license_check.pyto fail when tracked Game Icons SVGs lack local attribution/notice coverage..svgassets from/openworlds/withimage/svg+xml.Source Research And Decision
Wave 1 research recommended a selective direct asset import only for
game-icons/icons. The upstream license text says icons are Creative Commons BY 3.0 unless a contributor is explicitly CC0, and asks derivative works to mention the icon authors.Rejected alternatives:
Architecture Boundary
This PR is presentation-only.
/moveintents where actions are backed.Files To Review First
viewer/openworlds/icon-registry.jsxviewer/openworlds/assets/icons/ATTRIBUTION.mdscripts/license_check.pyviewer/tests/test_openworlds_static.pyviewer/openworlds/screen-table.jsxviewer/openworlds/screen-combat.jsxviewer/openworlds/screen-map.jsxValidation
Run from
/Volumes/LEXAR/repos/ClawDnD-icon-registry:Observed:
viewer.tests.test_openworlds_static: 17 tests passed.py_compile: passed.license_check: passed after staging the icon assets.git diff --check --cached: passed.Rollback Plan
Revert this commit. The icon registry is additive and does not alter engine state, persisted snapshots, or viewer API contracts. The fallback UI continues to render text/glyphs if the registry is absent.
Refs #174, #113, #115, #116, #117, #119, #121, #133.
Summary by CodeRabbit
New Features
Documentation
Styles
Tests
Chores