feat(viewer): host exact OpenWorlds surface - #125
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 (4)
📜 Recent review details🧰 Additional context used🧬 Code graph analysis (2)viewer/openworlds/screen-merchant.jsx (1)
viewer/openworlds/screen-map.jsx (1)
🔍 Remote MCPThe repository appears to be private and the public DeepWiki endpoint cannot access it. Let me try with alternative search approaches using web search to understand OpenWorlds as a broader context:The web search returned results about Pathfinder: Kingmaker modding, which is context for the game mechanics being referenced, but not directly about the OpenWorlds project. Based on the AI summaries provided, I now have sufficient concrete information to summarize the relevant review context. Let me compile a focused summary of the key review points from the PR context already provided: Summary of Key Review Context for OpenWorlds PRBased on the comprehensive AI-generated summaries provided, here are the critical architectural and implementation details relevant to reviewing this PR: Architecture & State ManagementThe OpenWorlds viewer implements a read-mostly fidelity surface with defined constraints: State Authority & Write Lane [::SOURCE.md::]
State Boundary Constraints [::SOURCE.md::]
Component Architecture & Global Exposure PatternThe PR establishes a consistent pattern across all screen and UI components: Global Window Assignments [::All screen and chrome files::]
This pattern allows server-injected HTML to dynamically wire JSX components loaded via Runtime Asset Vendoring & DependenciesBundled Local Assets [::vendor/THIRD_PARTY_NOTICES.md, google-fonts.css::]
Bundle Entry Flow [::index.html::]
Server Implementation & SecurityRoute Security [::viewer/server.py::]
Static Routes [::viewer/server.py::]
Data Flow & State InitializationApp State Structure [::app.jsx, data.js::]
Validation Coverage [::test_openworlds_static.py::]Tests verify:
Known Limitations & Follow-ups
🔇 Additional comments (4)
📝 WalkthroughWalkthroughAdds a complete Open Worlds tabletop viewer UI: 13 screen components covering campaigns, parties, combat, inventory, lore, and settings; shared infrastructure (toast/tooltip systems, camp UI, chrome navigation); comprehensive stylesheet with multiple color palettes; vendored runtime and font assets; server routes for asset serving with traversal protection; and tests validating static assets and security boundaries. ChangesOpenWorlds Viewer Implementation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
✨ Finishing Touches📝 Generate docstrings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 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 `@viewer/openworlds/app.jsx`:
- Around line 59-67: The code dereferences current.title and current.day without
ensuring current exists (current is derived from state.campaigns and
state.activeCampaign), which will crash if campaigns is empty; update the logic
around const current in viewer/openworlds/app.jsx to guard against missing
campaigns by computing a safe fallback (e.g., const current =
state.campaigns?.find(c => c.id === state.activeCampaign) ||
state.campaigns?.[0] || null) and then either conditionally render TitleBar
(render nothing or an empty-state when current is null) or pass safe default
values for SCREEN_TITLES[screen], campaign, and day; ensure references to
current.title and current.day are only used when current is non-null.
- Around line 23-25: The onKey handler’s early-return guard (in function onKey)
only excludes input and textarea so global hotkeys still trigger when focus is
on select or contenteditable elements; update the guard to also ignore select
and contenteditable elements by checking e.target.matches for "input, textarea,
select, [contenteditable]" (or equivalent) and return early to prevent hotkeys
firing while the user is interacting with form controls or editable regions.
In `@viewer/openworlds/camp-sidebar.jsx`:
- Around line 25-74: The warning condition uses ration (0 or 5) but compares it
to a hardcoded 6 so it never fires; introduce an inPack variable (replace the
hardcoded 6) and use it for both the RationStat "In pack" value and the warning
check. Update the JSX to compute const inPack = 6 (or derive from inventory
state if available), render <RationStat label="In pack" value={inPack} /> and
change the conditional to {ration > inPack && (...)} so the warning triggers
when required rations exceed what's actually in the pack.
In `@viewer/openworlds/chrome.jsx`:
- Around line 217-227: The buttons render without an explicit type (so default
to "submit" in a form) — add type="button" to the <button> elements and to the
BrassButton component to prevent accidental form submissions; specifically
update the icon-plate button instance shown and the other button instances at
the referenced locations, and ensure the BrassButton component's props/defaults
accept or set type="button" when none is provided so callers that don't pass a
type still render as a non-submitting button.
In `@viewer/openworlds/screen-bestiary.jsx`:
- Around line 11-15: The effect watching tab/filter currently only updates
selection when filtered has items but doesn't clear selection when filtered is
empty; update the React.useEffect (the one using filtered, selected,
setSelected) to also call setSelected(null) (or undefined) when filtered.length
=== 0 so stale right-panel content is cleared, and include filtered in the
dependency array so the effect runs when search results change; apply the same
change to the other useEffect referenced around lines 78-79 (the similar
filtered/selected logic) to ensure both places clear selection when no filtered
entries remain.
In `@viewer/openworlds/screen-character.jsx`:
- Around line 54-56: The "Rest & Prepare" button currently always calls
onNavigate("map", { openCamp: true }) so restOpen is never set and
RestPrepareModal is unreachable; change the BrassButton onClick handler to set
the restOpen state to true (e.g., call the setter for restOpen) instead of—or
conditionally before—navigating, so RestPrepareModal can open (ensure the
RestPrepareModal is rendered based on restOpen and that any existing navigation
still runs only after confirming/resting if intended). Reference: BrassButton
onClick, restOpen state, RestPrepareModal, and onNavigate.
- Around line 4-10: The component initializes active with state.party[0].id and
immediately dereferences hero which will throw if state.party is empty; change
the active initializer and hero lookup to guard for an empty party (e.g., set
active to null or undefined when state.party.length === 0 and compute hero only
when state.party exists), update any uses of active/hero (in render or handlers)
to handle a null hero (early return, loading placeholder, or safe optional
chaining), and ensure setActive is still invoked with a valid id when party
becomes available; update the symbols const [active, setActive], const hero =
state.party.find(...), and any JSX that reads hero.* accordingly.
In `@viewer/openworlds/screen-combat.jsx`:
- Around line 33-37: The endTurn handler currently increments round and resets
AP but leaves the previous action active, causing stale UI/state; update the
endTurn function to reset the active action by calling the component's state
updater (e.g., setActiveAction(null) or the default action value) so
activeAction is cleared when a turn ends, and apply the same fix to the other
turn-ending handler referenced in the diff (the block around lines 87-94) so
both paths clear activeAction consistently.
- Around line 20-31: The onMove handler is directly mutating module-level TOKENS
via selected.x/selected.y; instead, create an updated tokens array and update
React state (e.g., the tokens state setter used in this component) rather than
mutating TOKENS; in the onMove function (and the similar block around the other
occurrence), copy the target token, set its new x/y on the copy, produce a new
tokens array with that updated token (preserving immutability), call the
component's setTokens (or equivalent state updater) with the new array, and then
perform setLog/setAp/setActiveAction/toast as before so the global exported
TOKENS is not mutated.
In `@viewer/openworlds/screen-dialogue.jsx`:
- Around line 146-149: The code assumes state.party[0] always exists and crashes
when party is empty; update the rendering around Placeholder and the name div to
be null-safe by checking state.party and state.party[0] (or use optional
chaining) and providing fallbacks for state.party[0].short and
state.party[0].name (e.g., default image/label and a default display name like
"Unknown" or empty string), or conditionally render the whole block only when a
member exists; ensure you modify the JSX that uses Placeholder,
state.party[0].short, and state.party[0].name to use these checks/fallbacks.
In `@viewer/openworlds/screen-forge.jsx`:
- Around line 15-17: The code dereferences hero (result of state.party.find)
without checking for undefined, causing crashes when the crafter isn't found;
update all places that use hero.id or hero.name (including the hero constant,
skillBonus, and successChance calculations) to first guard that hero exists
(e.g., check if hero is truthy) and provide safe fallbacks—use optional chaining
or conditional expressions so CRAFTER_SKILL[hero.id] is only accessed when hero
is defined, default the skillBonus to a sensible value (e.g., 0 or the current 4
fallback) when hero is missing, and compute successChance using those safe
values (ensure selected checks remain). Ensure the same pattern is applied at
the other mentioned occurrences.
In `@viewer/openworlds/screen-inventory.jsx`:
- Around line 6-10: The code assumes state.party[0] exists which can throw;
change initialization and resolution to safely handle missing party data by
defaulting activeHero to null (or undefined) instead of state.party[0].id and
resolve hero using a safe lookup: check state.party is an array and find only if
present, e.g. compute hero only when state.party?.length and activeHero are set;
update any places that read hero.* (the hero variable and uses in inventory
panels) to conditionally render or early-return when hero is null, and ensure
setActiveHero is only called with valid ids from state.party to avoid
dereferencing undefined.
In `@viewer/openworlds/screen-journal.jsx`:
- Around line 4-7: state.quests may be empty causing the initializers and
subsequent usage to throw; change the initialization of activeQuest to safely
handle empty lists (e.g., use state.quests && state.quests.length ?
state.quests[0].id : null) and guard all uses of quest (the const quest =
state.quests.find((q) => q.id === activeQuest)) so you never read quest.* when
quest is undefined—add conditional rendering or fallbacks around the UI that
consumes quest and ensure setActiveQuest is only called with a valid id.
In `@viewer/openworlds/screen-launcher.jsx`:
- Around line 98-125: The rendering code assumes campaigns[0] exists and
dereferences c (from campaigns.find(...) || campaigns[0]) which crashes when
campaigns is empty; add an early guard at the top of the component (before
computing c or rendering Placeholder/Pill) that checks if campaigns is empty
(e.g., campaigns == null || campaigns.length === 0) and returns a safe fallback
UI or null. Update the logic around the selected lookup (where c is computed) to
only run when campaigns has items and ensure any downstream uses of c
(Placeholder, Pill, c.region, c.system, c.chapter, c.title, c.day, c.subtitle)
are behind that guard so they never access properties on undefined.
In `@viewer/openworlds/screen-map.jsx`:
- Around line 142-144: The Y coordinate is being over-scaled by the expression
top: `${loc.y / 6 * 100}%`; update the top calculation in screen-map.jsx to
compute Y the same way as X (i.e., use loc.y as a percentage) or normalize loc.y
consistently before converting to percent — for example replace the expression
with top: `${loc.y}%` (or, if loc.y is in raw map units, normalize via (loc.y /
mapMaxY) * 100) so the pin placement uses the same coordinate space as left:
`${loc.x}%`.
In `@viewer/openworlds/screen-merchant.jsx`:
- Around line 154-155: The cart checkout currently assumes all items share one
mode and treats mixed-mode carts as buys; update the checkout logic (the
function that processes cart checkout) to iterate the cart array and handle each
item's mode individually: use the item.mode property (items are added with
setCart([...cart, { ...it, price: shownPrice, mode: tab }])) and compute gp
change by reducing over cart — subtract item.price for mode === "buy" and add
item.price for mode === "sell" — and apply inventory changes per item
accordingly (add inventory on buy, remove on sell) before clearing the cart or
updating state.
- Line 280: The merchant stock item for id "m15" (name "Salt") contains the
duplicate property "price"; remove the redundant "price" key so the object
literal has a single price entry (keep the intended value, e.g., price: 12) in
the merchant/items array in screen-merchant.jsx to resolve the lint/CI error.
In `@viewer/openworlds/screen-seed.jsx`:
- Around line 158-167: The SeedToggle components (e.g., the instances with
labels "Item destruction" and "Anachronism") are rendered as interactive but
have no onChange handler or connection to state, so either connect them to the
existing seed state (read their value from seed.<key> and supply an onChange
that calls the setter, e.g., setSeed(prev => ({...prev, <key>: newValue}))) or
mark them explicitly non-interactive by adding a disabled or readOnly prop to
SeedToggle (and update its implementation to render a non-clickable appearance
when disabled) — apply the same fix for other SeedToggle usages mentioned
(around the later block) so all toggles are either state-driven or visibly
disabled.
In `@viewer/openworlds/screen-settings.jsx`:
- Around line 68-69: Several Toggle components in screen-settings.jsx (e.g., the
Toggle instances labeled "Duck music during GM narration" and "Crossfade between
scenes" and other Toggles around the file) are rendered with a value prop but no
onChange handler, making them appear interactive but do nothing; either bind
each Toggle to local state in the ScreenSettings component (create useState
hooks like [duckMusic, setDuckMusic] and pass value={duckMusic}
onChange={setDuckMusic}) or explicitly make them non-interactive by passing
disabled or readOnly and adjusting styles (e.g., value={true} disabled />).
Update every static Toggle instance mentioned in the review (including the
groups at the other ranges) to follow one of these two patterns so the UI is
consistent and accessible.
In `@viewer/openworlds/screen-table.jsx`:
- Around line 30-31: The log entries for dice rolls are showing "d20" regardless
of the actual die; update the LogEntry creation(s) that build the roll message
(the object with kind: "roll", who: hero.name and the text property) to
interpolate the actual sides variable instead of hardcoding d20 — use the
existing sides variable in the template (e.g., `d${sides}`) wherever the roll
text is composed (lines creating the roll log entries around the code that
references r, sides, hero.name) so d12/d8/d6 display correctly.
- Around line 94-97: The "Camp" BrassButton currently has no onClick handler, so
it appears actionable but does nothing; wire it to the existing navigation flow
by adding an onClick that calls the onNavigate prop with the "camp" route
(consistent with how "map" and "dialogue" are handled), i.e. update the
BrassButton for "Camp" to invoke onNavigate("camp") so clicking it triggers the
camp view; ensure you reference the BrassButton component and the onNavigate
prop when making this change.
In `@viewer/openworlds/styles.css`:
- Around line 121-122: Interactive controls (button, .btn, .icon-btn, .nav-item,
and anchor links) lack an explicit keyboard-visible focus state; add
:focus-visible rules that provide a clear, high-contrast outline or ring (and an
accessible fallback using :focus for browsers without focus-visible) so keyboard
users can see focus without changing mouse styles. Update the CSS where button,
.btn, .icon-btn, .nav-item, and a are defined to include matching :focus-visible
(and optional :focus) declarations that set outline/box-shadow, outline-offset,
and ensure border-radius/transparent backgrounds render the ring; keep existing
pointer/mouse styles unchanged. Apply the same focus-visible additions to the
other ranges mentioned (lines ~268-325 and ~553-633) so all interactive controls
share the accessible focus pattern.
- Line 60: Rename the keyframes pageIn, toastIn, and tooltipIn to kebab-case
(page-in, toast-in, tooltip-in) and update every reference (animation-name,
animation, or `@apply` usage) to the new names; also normalize CSS keyword casing
for font and property values: change "Georgia" to georgia in --f-body and any
font-family lists, change optimizeLegibility to optimizelegibility where used
(font-synthesis/antialiasing-related rules), and change currentColor to
currentcolor wherever referenced to satisfy the Stylelint rules; update the
identifiers in the stylesheet where these exact symbols appear (keyframes
pageIn/toastIn/tooltipIn and CSS variables like --f-body and occurrences of
optimizeLegibility/currentColor) so all usages remain consistent.
- Around line 728-756: The animations for pageIn, flicker, toastIn, and
tooltipIn (used by .screen, .candleglow, toast/tooltip elements) don't respect
prefers-reduced-motion; add a media query `@media` (prefers-reduced-motion:
reduce) that disables or reduces motion for these selectors by setting
animation: none (or animation-duration: 0s) and removing transforms/transitions
(e.g., override .screen, .candleglow, and whatever toast/tooltip classes rely on
tooltipIn/toastIn to have animation: none and reset transform/opacity) so the
keyframes (pageIn, flicker, toastIn, tooltipIn) are effectively bypassed for
users who prefer reduced motion.
In `@viewer/openworlds/toast.jsx`:
- Around line 83-92: The deferred adding of global listeners inside the
setTimeout can race with unmount and cause stale listeners to be attached;
capture the timeout id returned by setTimeout when scheduling the listener
attachment in the ContextMenu/Toast component and call clearTimeout(timeoutId)
in the cleanup function before attempting to remove listeners, and only add
listeners inside the timeout callback if the component is still mounted (or
check that the timeout wasn't cleared) so handlers like close and esc are never
registered after unmount.
🪄 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: 05787e9e-c230-400e-90ff-4f68a1fe6457
⛔ Files ignored due to path filters (15)
viewer/openworlds/vendor/babel-standalone-7.29.0.min.jsis excluded by!**/*.min.jsviewer/openworlds/vendor/fonts/8vIU7ww63mVu7gtR-kwKxNvkNOjw-gjgTYo.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/8vIU7ww63mVu7gtR-kwKxNvkNOjw-jHgTYo.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/8vIU7ww63mVu7gtR-kwKxNvkNOjw-tbnTYo.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/8vIU7ww63mVu7gtR-kwKxNvkNOjw-uTnTYo.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/Ktk1ALSLW8zDe0rthJysWrnLsAz3Fw.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/Ktk3ALSLW8zDe0rthJysWrnLsAzHFaOd.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/co3smX5slCNuHLi8bLeY9MK7whWMhyjYrGFEsdtdc62E6zd58jDOjw.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/co3smX5slCNuHLi8bLeY9MK7whWMhyjYrGFEsdtdc62E6zd5wDDOjw.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/co3umX5slCNuHLi8bLeY9MK7whWMhyjypVO7abI26QOD_hg9GnM.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/co3umX5slCNuHLi8bLeY9MK7whWMhyjypVO7abI26QOD_iE9GnM.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/co3umX5slCNuHLi8bLeY9MK7whWMhyjypVO7abI26QOD_s06GnM.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/co3umX5slCNuHLi8bLeY9MK7whWMhyjypVO7abI26QOD_v86GnM.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/tDbY2o-flEEny0FZhsfKu5WU4zr3E_BX0PnT8RD8-qxjPQ.ttfis excluded by!**/*.ttfviewer/openworlds/vendor/fonts/tDbY2o-flEEny0FZhsfKu5WU4zr3E_BX0PnT8RD8yKxjPQ.ttfis excluded by!**/*.ttf
📒 Files selected for processing (32)
THIRD_PARTY_NOTICES.mdviewer/openworlds/SOURCE.mdviewer/openworlds/app.jsxviewer/openworlds/camp-sidebar.jsxviewer/openworlds/chrome.jsxviewer/openworlds/data.jsviewer/openworlds/index.htmlviewer/openworlds/screen-acts.jsxviewer/openworlds/screen-bestiary.jsxviewer/openworlds/screen-character.jsxviewer/openworlds/screen-combat.jsxviewer/openworlds/screen-create.jsxviewer/openworlds/screen-dialogue.jsxviewer/openworlds/screen-forge.jsxviewer/openworlds/screen-inventory.jsxviewer/openworlds/screen-journal.jsxviewer/openworlds/screen-launcher.jsxviewer/openworlds/screen-map.jsxviewer/openworlds/screen-merchant.jsxviewer/openworlds/screen-relations.jsxviewer/openworlds/screen-seed.jsxviewer/openworlds/screen-settings.jsxviewer/openworlds/screen-table.jsxviewer/openworlds/styles.cssviewer/openworlds/toast.jsxviewer/openworlds/tooltip.jsxviewer/openworlds/vendor/THIRD_PARTY_NOTICES.mdviewer/openworlds/vendor/google-fonts.cssviewer/openworlds/vendor/react-18.3.1.development.jsviewer/openworlds/vendor/react-dom-18.3.1.development.jsviewer/server.pyviewer/tests/test_openworlds_static.py
📜 Review details
🧰 Additional context used
🪛 Biome (2.4.15)
viewer/openworlds/screen-merchant.jsx
[error] 280-280: This property is later overwritten by an object member with the same name.
(lint/suspicious/noDuplicateObjectKeys)
🪛 Ruff (0.15.14)
viewer/tests/test_openworlds_static.py
[warning] 16-16: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 21-21: Missing return type annotation for private function log_message
Add return type annotation: None
(ANN202)
[warning] 21-21: Unused method argument: fmt
(ARG002)
[warning] 21-21: Missing type annotation for *args
(ANN002)
[warning] 21-21: Unused method argument: args
(ARG002)
[warning] 49-49: Missing return type annotation for private function _get
(ANN202)
[error] 50-50: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[warning] 53-53: Missing return type annotation for private function _status
(ANN202)
🪛 Stylelint (17.12.0)
viewer/openworlds/vendor/google-fonts.css
[error] 2-2: Expected no quotes around "Cinzel" (font-family-name-quotes)
(font-family-name-quotes)
[error] 9-9: Expected no quotes around "Cinzel" (font-family-name-quotes)
(font-family-name-quotes)
[error] 16-16: Expected no quotes around "Cinzel" (font-family-name-quotes)
(font-family-name-quotes)
[error] 23-23: Expected no quotes around "Cinzel" (font-family-name-quotes)
(font-family-name-quotes)
viewer/openworlds/styles.css
[error] 731-731: Expected keyframe name "pageIn" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
[error] 748-748: Expected keyframe name "toastIn" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
[error] 753-753: Expected keyframe name "tooltipIn" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
[error] 60-60: Expected "Georgia" to be "georgia" (value-keyword-case)
(value-keyword-case)
[error] 117-117: Expected "optimizeLegibility" to be "optimizelegibility" (value-keyword-case)
(value-keyword-case)
[error] 457-457: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
[error] 457-457: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
[error] 655-655: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🔍 Remote MCP
The DeepWiki tools are unable to access the private repository. Let me search the web for information about OpenWorlds and related concepts that would help contextualize this PR.The web searches returned general information about open-world game development and Pathfinder campaign managers, but not about the specific ClawDnD/OpenWorlds project. Let me focus on getting more specific technical context about the implementation details relevant to the PR review.Based on my research, I now have sufficient technical context to provide a comprehensive summary of relevant facts for PR review. Let me compile the findings.
Summary of Relevant Context for PR Review
Browser-Based JSX Compilation & Performance Considerations
When Babel Standalone is loaded in a browser, it automatically compiles and executes all script tags with type "text/babel" or "text/jsx". This is the approach used in the OpenWorlds viewer. However, important context for review:
- This approach is fine for learning and creating simple demos, but it makes your website slow and isn't suitable for production
- Babel/standalone does not support running native modules in the browser; you either have to compile them to something like AMD/RequireJS, or you have to pre-bundle your files on the server
- The PR documentation explicitly notes that "Babel-in-browser warning as temporary; replacement with precompiled bundle planned in follow-ups," indicating the team is aware of this limitation
Path Traversal Security & Implementation Best Practices
The PR implements path traversal protection in viewer/server.py. The security approach aligns with industry standards:
- Using resolve() expands ".." and similar sequences: if the user inputs "../../secret.txt", resolve() turns it into an absolute path (e.g., /secret.txt), which allows us to compare absolute paths cleanly
- The is_relative_to() check is the most important validation: even if the path is resolved, we must verify it lives inside the designated "safe zone"; if the resolved path is outside, the check fails and the attack is blocked
- Constructing a path securely requires resolving it to an absolute form, then comparing it to a known safe base directory; simply checking for "../" is never enough
The PR's use of Python's Path.resolve() plus relative_to() follows this best practice pattern and explicitly prevents both decoded path-traversal (e.g., %2e%2e) and nested traversal attempts by returning 404.
Architecture: State Authority Model
The PR documentation indicates a clear read-only surface design:
/openworlds/config.jsonexposes browser-safe metadata withstate_authority: "engine"andwrite_lane: "/move"- The OpenWorlds bundle does not write to snapshot.json, play-state, inventory, quests, XP, world clocks, or companion state
data.jsis explicitly marked as non-canonical demo/prototype data pending future binding to real ClawDnD read models- This maintains downstream read-surface semantics while preserving engine authority
🔇 Additional comments (16)
viewer/server.py (5)
38-38: LGTM!Also applies to: 47-47, 52-62
1420-1435: LGTM!
1438-1443: LGTM!
1446-1462: LGTM!
1580-1587: LGTM!viewer/tests/test_openworlds_static.py (4)
1-22: LGTM!
25-59: LGTM!
61-97: LGTM!
100-101: LGTM!viewer/openworlds/SOURCE.md (1)
1-49: LGTM!THIRD_PARTY_NOTICES.md (1)
25-40: LGTM!viewer/openworlds/index.html (1)
1-42: LGTM!viewer/openworlds/data.js (1)
1-388: LGTM!viewer/openworlds/vendor/THIRD_PARTY_NOTICES.md (1)
42-44: ⚡ Quick winClarify Google Fonts licensing/provenance in
viewer/openworlds/vendor/THIRD_PARTY_NOTICES.md(lines 42-44)
- Lines 42-44 only say “open font licenses”; enumerate each bundled Google Fonts family with its exact license identifier and canonical upstream source (and per-file hashes/provenance at the same level as the React/Babel entries).
- For Cinzel, Cormorant Garamond, IM Fell English, and JetBrains Mono, use OFL and these upstream sources:
- Cinzel — OFL — https://github.com/NDISCOVER/Cinzel
- Cormorant Garamond — OFL — https://github.com/CatharsisFonts/Cormorant
- IM Fell English — OFL — https://github.com/librefonts/imfellenglish
- JetBrains Mono — OFL — https://github.com/JetBrains/JetBrainsMono
viewer/openworlds/vendor/google-fonts.css (1)
2-2: ⚡ Quick winFix Stylelint
font-family-name-quotesforCinzelinviewer/openworlds/vendor/google-fonts.css
- Replace
font-family: 'Cinzel';withfont-family: Cinzel;at lines 2, 9, 16, and 23 to satisfyfont-family-name-quoteswhen enforced.Proposed fix
- font-family: 'Cinzel'; + font-family: Cinzel; ... - font-family: 'Cinzel'; + font-family: Cinzel; ... - font-family: 'Cinzel'; + font-family: Cinzel; ... - font-family: 'Cinzel'; + font-family: Cinzel;viewer/openworlds/tooltip.jsx (1)
3-59: LGTM!Also applies to: 62-85, 88-98
| React.useEffect(() => { | ||
| if (filtered.length > 0 && !filtered.find((e) => e.id === selected?.id)) { | ||
| setSelected(filtered[0]); | ||
| } | ||
| }, [tab, filter]); |
There was a problem hiding this comment.
Clear selection when no filtered entries remain.
When search yields zero results, the right panel can still show stale content unrelated to the current filter.
Proposed fix
React.useEffect(() => {
- if (filtered.length > 0 && !filtered.find((e) => e.id === selected?.id)) {
- setSelected(filtered[0]);
- }
- }, [tab, filter]);
+ if (filtered.length === 0) {
+ setSelected(null);
+ return;
+ }
+ if (!filtered.find((e) => e.id === selected?.id)) {
+ setSelected(filtered[0]);
+ }
+ }, [tab, filter, selected, filtered]);Also applies to: 78-79
🤖 Prompt for 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.
In `@viewer/openworlds/screen-bestiary.jsx` around lines 11 - 15, The effect
watching tab/filter currently only updates selection when filtered has items but
doesn't clear selection when filtered is empty; update the React.useEffect (the
one using filtered, selected, setSelected) to also call setSelected(null) (or
undefined) when filtered.length === 0 so stale right-panel content is cleared,
and include filtered in the dependency array so the effect runs when search
results change; apply the same change to the other useEffect referenced around
lines 78-79 (the similar filtered/selected logic) to ensure both places clear
selection when no filtered entries remain.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
viewer/openworlds/screen-merchant.jsx (1)
229-233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a functional state update for coin settlement at Line 230.
The checkout uses captured
coins, which is brittle under rapid repeated interactions. Use a functional updater so GP settlement always applies to latest state.Suggested fix
- <BrassButton onClick={() => { - setCoins({ ...coins, gp: coins.gp + balanceDelta }); + <BrassButton onClick={() => { + setCoins((prev) => ({ ...prev, gp: prev.gp + balanceDelta })); setCart([]); }} style={{ width: "100%" }} disabled={cart.length === 0 || coins.gp + balanceDelta < 0}>🤖 Prompt for 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. In `@viewer/openworlds/screen-merchant.jsx` around lines 229 - 233, The onClick handler for BrassButton captures the stale coins object when calling setCoins; replace the direct captured update with a functional state updater so GP is applied to the latest state (use setCoins(prev => ({ ...prev, gp: prev.gp + balanceDelta })) ), keep setCart([]) as-is, and ensure you still reference balanceDelta and cart for the disabled logic.
🤖 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 `@viewer/openworlds/camp-sidebar.jsx`:
- Line 246: The TalkPanel is being rendered with hero={party.find((p) => p.id
=== talkPartner)} which can be undefined and causes TalkPanel to crash when it
reads hero.id; update the parent render to only mount <TalkPanel ...> if
talkPartner is truthy and party.find(...) returns a defined object (e.g.,
compute const hero = party.find(p => p.id === talkPartner) and use {hero &&
<TalkPanel hero={hero} onClose={() => onTalk(null)} />}). Also add a defensive
null-check inside the TalkPanel component (e.g., guard accesses to hero.id /
hero.* and return null or a fallback UI when hero is undefined) so TalkPanel
never dereferences an undefined hero.
In `@viewer/openworlds/screen-map.jsx`:
- Around line 4-6: The initial selected state (selected, setSelected created via
React.useState) is only set once and won't update when the locations array
changes; add a React.useEffect that watches the locations variable and calls
setSelected(locations.find(l => l.current) || locations[0] || null) so the
details pane resyncs whenever locations is replaced or populated. Ensure the
effect's dependency array contains locations and avoid changing behavior when
locations is identical (i.e., use locations directly).
In `@viewer/openworlds/screen-settings.jsx`:
- Around line 69-70: The Toggle component currently renders a plain button and
needs proper accessible toggle semantics: update Toggle to expose role="switch"
(or use a checkbox input) and set aria-checked based on its value prop, ensure
it accepts an accessible label (aria-label or aria-labelledby) and that its
onChange/onClick toggles state; also support Space/Enter keyboard activation (or
delegate to native input behavior) so usages like setAudio({...audio, duckMusic:
v}) and other places receive the correct boolean. Locate the Toggle component
and its props (value, onChange, label) and implement role/aria-checked, proper
label support, and keyboard activation handling there so all wired instances
announce and toggle on/off correctly to assistive tech.
---
Duplicate comments:
In `@viewer/openworlds/screen-merchant.jsx`:
- Around line 229-233: The onClick handler for BrassButton captures the stale
coins object when calling setCoins; replace the direct captured update with a
functional state updater so GP is applied to the latest state (use setCoins(prev
=> ({ ...prev, gp: prev.gp + balanceDelta })) ), keep setCart([]) as-is, and
ensure you still reference balanceDelta and cart for the disabled logic.
🪄 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: d2cc0080-27df-4be7-a03e-8b8f4a8644fd
📒 Files selected for processing (22)
viewer/openworlds/app.jsxviewer/openworlds/camp-sidebar.jsxviewer/openworlds/chrome.jsxviewer/openworlds/screen-bestiary.jsxviewer/openworlds/screen-character.jsxviewer/openworlds/screen-combat.jsxviewer/openworlds/screen-dialogue.jsxviewer/openworlds/screen-forge.jsxviewer/openworlds/screen-inventory.jsxviewer/openworlds/screen-journal.jsxviewer/openworlds/screen-launcher.jsxviewer/openworlds/screen-map.jsxviewer/openworlds/screen-merchant.jsxviewer/openworlds/screen-seed.jsxviewer/openworlds/screen-settings.jsxviewer/openworlds/screen-table.jsxviewer/openworlds/styles.cssviewer/openworlds/toast.jsxviewer/openworlds/tooltip.jsxviewer/openworlds/vendor/THIRD_PARTY_NOTICES.mdviewer/openworlds/vendor/google-fonts.cssviewer/tests/test_openworlds_static.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (8)
viewer/openworlds/screen-bestiary.jsx (1)
viewer/openworlds/app.jsx (1)
ScreenBestiary(167-167)
viewer/openworlds/screen-dialogue.jsx (1)
viewer/openworlds/chrome.jsx (1)
Placeholder(204-213)
viewer/tests/test_openworlds_static.py (1)
viewer/server.py (1)
_Handler(1465-1757)
viewer/openworlds/screen-character.jsx (1)
viewer/openworlds/chrome.jsx (3)
Panel(245-260)SectionTitle(190-198)BrassButton(236-243)
viewer/openworlds/chrome.jsx (2)
viewer/openworlds/screen-combat.jsx (1)
ScreenCombat(3-172)viewer/openworlds/screen-character.jsx (1)
ScreenCharacter(3-222)
viewer/openworlds/screen-launcher.jsx (1)
viewer/openworlds/chrome.jsx (1)
SectionTitle(190-198)
viewer/openworlds/screen-merchant.jsx (1)
viewer/openworlds/chrome.jsx (1)
BrassButton(236-243)
viewer/openworlds/screen-table.jsx (2)
viewer/openworlds/chrome.jsx (2)
Pill(200-202)BrassButton(236-243)viewer/openworlds/app.jsx (1)
ScreenTable(156-156)
🪛 LanguageTool
viewer/openworlds/vendor/THIRD_PARTY_NOTICES.md
[typographical] ~37-~37: To join two clauses or introduce examples, consider using an em dash.
Context: ...e-fonts.css- Font families: - Cinzel - OFL -https://github.com/NDISCOVER/Cinz...
(DASH_RULE)
[typographical] ~37-~37: To join two clauses or introduce examples, consider using an em dash.
Context: ...s.css- Font families: - Cinzel - OFL -https://github.com/NDISCOVER/Cinzel` ...
(DASH_RULE)
[typographical] ~38-~38: To join two clauses or introduce examples, consider using an em dash.
Context: ...NDISCOVER/Cinzel - Cormorant Garamond - OFL -https://github.com/CatharsisFonts...
(DASH_RULE)
[typographical] ~38-~38: To join two clauses or introduce examples, consider using an em dash.
Context: ...VER/Cinzel - Cormorant Garamond - OFL -https://github.com/CatharsisFonts/Cormo...
(DASH_RULE)
[typographical] ~39-~39: To join two clauses or introduce examples, consider using an em dash.
Context: ...rsisFonts/Cormorant - IM Fell English - OFL -https://github.com/librefonts/imf...
(DASH_RULE)
[typographical] ~39-~39: To join two clauses or introduce examples, consider using an em dash.
Context: ...nts/Cormorant - IM Fell English - OFL -https://github.com/librefonts/imfelleng...
(DASH_RULE)
[typographical] ~40-~40: To join two clauses or introduce examples, consider using an em dash.
Context: ...efonts/imfellenglish - JetBrains Mono - OFL -https://github.com/JetBrains/JetB...
(DASH_RULE)
[typographical] ~40-~40: To join two clauses or introduce examples, consider using an em dash.
Context: .../imfellenglish - JetBrains Mono - OFL -https://github.com/JetBrains/JetBrainsM...
(DASH_RULE)
🪛 Ruff (0.15.14)
viewer/tests/test_openworlds_static.py
[warning] 20-20: Unused method argument: fmt
(ARG002)
[warning] 20-20: Unused method argument: args
(ARG002)
🔇 Additional comments (21)
viewer/openworlds/vendor/THIRD_PARTY_NOTICES.md (1)
37-45: LGTM!viewer/openworlds/screen-bestiary.jsx (1)
12-19: LGTM!viewer/openworlds/screen-dialogue.jsx (1)
7-7: LGTM!Also applies to: 147-149
viewer/openworlds/app.jsx (1)
10-10: LGTM!Also applies to: 24-31, 67-71
viewer/openworlds/screen-seed.jsx (1)
11-12: LGTM!Also applies to: 163-170
viewer/tests/test_openworlds_static.py (1)
1-2: LGTM!Also applies to: 13-16, 20-20, 36-37, 47-57, 88-88
viewer/openworlds/screen-map.jsx (1)
109-111: LGTM!Also applies to: 138-145, 168-168, 232-232, 309-309
viewer/openworlds/screen-character.jsx (1)
4-24: LGTM!Also applies to: 32-32, 69-69
viewer/openworlds/vendor/google-fonts.css (1)
2-2: LGTM!Also applies to: 9-9, 16-16, 23-23
viewer/openworlds/toast.jsx (1)
48-48: LGTM!Also applies to: 83-83, 89-89, 107-107
viewer/openworlds/chrome.jsx (1)
218-218: LGTM!Also applies to: 236-236, 268-268, 281-281, 324-324
viewer/openworlds/tooltip.jsx (1)
50-50: LGTM!viewer/openworlds/screen-forge.jsx (1)
4-4: LGTM!Also applies to: 16-17, 21-21, 147-147, 161-161
viewer/openworlds/screen-launcher.jsx (1)
4-5: LGTM!Also applies to: 9-11, 100-108, 149-149, 168-169, 208-208
viewer/openworlds/screen-merchant.jsx (1)
11-19: LGTM!Also applies to: 220-220, 224-224, 226-226, 283-283
viewer/openworlds/screen-table.jsx (1)
4-7: LGTM!Also applies to: 9-9, 12-12, 32-32, 51-51, 98-98, 117-117, 144-144, 167-167, 312-312
viewer/openworlds/styles.css (1)
60-60: LGTM!Also applies to: 117-117, 124-128, 463-463, 661-661, 735-735, 737-737, 754-754, 759-759, 764-771
viewer/openworlds/camp-sidebar.jsx (1)
4-4: LGTM!Also applies to: 19-19, 27-27, 70-70, 72-72, 101-101, 113-113, 130-130, 168-168, 176-176, 194-194
viewer/openworlds/screen-journal.jsx (1)
4-12: LGTM!Also applies to: 38-38
viewer/openworlds/screen-combat.jsx (1)
4-4: LGTM!Also applies to: 21-21, 24-27, 37-37
viewer/openworlds/screen-inventory.jsx (1)
4-8: LGTM!Also applies to: 12-23, 142-142
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
This PR implements PR B from the OpenWorlds fidelity rollout: the viewer now hosts the exported OpenWorlds surface directly instead of repainting it in SwiftUI.
viewer/openworlds/bundle fromOpenWorlds.zip, normalized toindex.html.GET /openworlds/,GET /openworlds/<asset>, andGET /openworlds/config.jsonfromviewer/server.py.OpenWorlds.zip, andtweaks-panel.jsx.data.jsas non-canonical prototype/demo data until later PRs bind real ClawDnD read models.Refs #113, #114, #122, #82. Supersedes the SwiftUI repaint direction in draft PR #123 but does not close the epic issues.
Architecture Notes
The viewer remains a downstream read surface:
viewer/server.pyserves static OpenWorlds files same-origin with existing viewer APIs./openworlds/config.jsonexposes browser-safe metadata only:state_authority: "engine",write_lane: "/move", anddemo_data: true.snapshot.json,play-state,qa/state, inventory, quests, XP, world clocks, companion state, or private notes./dashboardand/monitorremain unchanged fallback/debug routes.The static asset resolver uses
Path.resolve()plusrelative_to()before serving files, so decoded traversal attempts such as/openworlds/%2e%2e/server.pyand nestedvendor/../../server.pystay outside the bundle and return 404.Source And Asset Contract
Included source files are documented in
viewer/openworlds/SOURCE.md:Open Worlds.html,styles.css,app.jsx,chrome.jsx,screen-*.jsx,camp-sidebar.jsx,toast.jsx,tooltip.jsx.data.js.tweaks-panel.jsx.openworlds/screenshots/*,uploads/*.png,uploads/dndforever/*, andOpenWorlds.zip.Runtime notices and hashes are in
viewer/openworlds/vendor/THIRD_PARTY_NOTICES.md; the top-levelTHIRD_PARTY_NOTICES.mdnow records the bundled browser runtime category.Validation
Local validation from
/Volumes/LEXAR/repos/ClawDnD-openworlds-surface:python3 -m py_compile viewer/server.pypython3 -m unittest viewer.tests.test_openworlds_static -qpython3 -m unittest discover -s viewer/tests -qpython3 scripts/license_check.pygit diff --checkVisual smoke evidence stored outside the repo under
/Volumes/LEXAR/Codex/openworlds-visual-smoke-2026-05-26/:candidate-1366x768.png,candidate-console-1440x900.png,candidate-1920x1080.pngreference-source-http-1366x768.png,reference-source-http-1440x900.png,reference-source-http-1920x1080.png.windowand.parchment, loaded no external requests, and produced no console errors.Diff check against the source-hosted reference was visually tight:
1366x768: ~0.257% changed pixels1440x900: ~0.234% changed pixels1920x1080: ~0.144% changed pixelsThe small diff is isolated to text rendering/anti-aliasing in the recap text area after local font/runtime hosting; the window frame, nav rail, parchment surface, typography families, spacing, and screen routing are preserved.
Follow-Ups
/openworlds/.Summary by CodeRabbit
New Features
Style
Documentation
Tests