feat: Forge mode, universal model loader, responsive viewport, auto-texture pipeline#1
feat: Forge mode, universal model loader, responsive viewport, auto-texture pipeline#1MolochDaGod wants to merge 2 commits into
Conversation
The layout used a fixed 3-column grid (260px + 1fr + 300px) that required 560px minimum width before the 3D viewport got any space. On mobile, the canvas was crushed to zero and sidebars overflowed off-screen. Changes: - Tablet (≤900px): 2-column grid, right panel drops below viewport - Mobile (≤768px): single-column flex layout, panels hidden by default with toggle buttons (☰ Chars / 📊 Stats) in the header - Small phone (≤480px): further font/grid reductions - 3D viewport gets a 4:3 aspect ratio container on mobile - Bottom panel stacks vertically instead of overlaying - Toggle buttons hidden on desktop via CSS (zero desktop impact) Co-Authored-By: Oz <oz-agent@warp.dev>
Phase 1 — Rendering Pipeline Upgrade:
- SmartLoader now supports all model formats: FBX, GLTF/GLB (Draco+KTX2),
OBJ+MTL, Collada (DAE), STL, USDZ/USDC
- GLTFLoader wired with DRACOLoader (Google CDN decoders) + KTX2Loader
(Basis Universal transcoder) for compressed meshes and GPU textures
- Renderer upgraded: SRGBColorSpace, high-performance power preference,
KTX2 auto-init on boot
- HDR environment map loader (RGBELoader) ready for IBL
Phase 2 — Forge Panel (Model Inspector):
- New ForgePanel.js module with:
- Drag-and-drop model intake (drop any file onto 3D viewport)
- File input button for direct file selection
- Recursive hierarchy tree with collapsible nodes
- Auto-labels 20 appendage types (head/arm_L/hand_R/leg_L/foot_R/etc.)
using bone name heuristics with color-coded badges
- Per-node: type icon, vertex/face count, material info, texture channels
- Visibility toggle (eye icon) and camera focus (target icon) per node
- Export hierarchy as JSON with full stats
- Forge panel added to bottom toolbar between Animations and Admin
Phase 3 — Texture Auto-Attach / Find / Generate:
- New TextureResolver.js with 3-stage pipeline:
1. Discovery: scans sibling files by naming convention
({model}_diffuse, _normal, _roughness, _metallic, _ao, _emissive)
2. Auto-attach: maps discovered textures to correct PBR channels
3. Fallback generation based on poly count:
- Low-poly (<5000 verts) → MeshToonMaterial (cel-shaded, 3-step gradient)
- High-poly (≥5000 verts) → MeshStandardMaterial with name-based
roughness/metalness heuristics (10 material categories: metal, wood,
cloth, stone, glass, skin, bone, etc.)
- Vertex color sampling → base color; hash-color fallback
- Pipeline runs automatically on every model load
Co-Authored-By: Oz <oz-agent@warp.dev>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Reviewer's GuideImplements a responsive layout for the character creator UI, adds a new Forge mode model inspector with drag-and-drop support, extends SmartLoader into a universal model loader (8 formats, Draco/KTX2, HDR env), and introduces an automatic texture resolution pipeline used on all loaded models. Sequence diagram for Forge mode drag-and-drop model inspectionsequenceDiagram
actor User
participant Viewport as viewportDiv
participant ForgePanel
participant SmartLoader as loadModelFromFile
participant THREE_Scene as scene
User->>Viewport: drag&drop File
Viewport->>ForgePanel: initDropZone drop event
ForgePanel->>SmartLoader: loadModelFromFile(file)
SmartLoader-->>ForgePanel: { scene, animations, format }
ForgePanel->>ForgePanel: prepareModel(model)
ForgePanel->>THREE_Scene: add(model)
ForgePanel->>ForgePanel: buildTreeData(model)
ForgePanel->>ForgePanel: renderTree()
ForgePanel-->>User: status "Forge: file (format) — verts/faces/anims"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Hey - I've found 2 security issues, 2 other issues, and left some high level feedback:
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
container.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- In
loadModelFromFile, the blob URL passed toloadModelloses the original filename/extension whendetectFormatstrips the hash, so OBJ/FBX/etc. files will hit theUnsupported formatpath; consider either teachingdetectFormatto look atlocation.hashor passing the previously computedfmtintoloadModelinstead of re-inferring from the blob URL. - In
ForgePanel.renderTree, a new click handler is attached to#forgeTreeevery time the tree rerenders, which will stack duplicate listeners; you can either attach the listener once in the constructor/init or remove any existing listener before reattaching. - The drop overlay in
ForgePanel.initDropZoneuses CSS variables (e.g.var(--accent)) from inlinestyle.cssText, which will only work if those variables are defined globally; if this panel is ever reused elsewhere, consider scoping or fallbacks so the overlay remains legible without relying on external CSS.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `loadModelFromFile`, the blob URL passed to `loadModel` loses the original filename/extension when `detectFormat` strips the hash, so OBJ/FBX/etc. files will hit the `Unsupported format` path; consider either teaching `detectFormat` to look at `location.hash` or passing the previously computed `fmt` into `loadModel` instead of re-inferring from the blob URL.
- In `ForgePanel.renderTree`, a new click handler is attached to `#forgeTree` every time the tree rerenders, which will stack duplicate listeners; you can either attach the listener once in the constructor/init or remove any existing listener before reattaching.
- The drop overlay in `ForgePanel.initDropZone` uses CSS variables (e.g. `var(--accent)`) from inline `style.cssText`, which will only work if those variables are defined globally; if this panel is ever reused elsewhere, consider scoping or fallbacks so the overlay remains legible without relying on external CSS.
## Individual Comments
### Comment 1
<location path="src/modules/SmartLoader.js" line_range="66" />
<code_context>
- if (lower.endsWith('.fbx')) return 'fbx';
- if (lower.endsWith('.gltf')) return 'gltf';
- if (lower.endsWith('.glb')) return 'glb';
+ const clean = url.toLowerCase().split('?')[0].split('#')[0];
+ for (const [ext, fmt] of Object.entries(FORMAT_MAP)) {
+ if (clean.endsWith(ext)) return fmt;
</code_context>
<issue_to_address>
**issue (bug_risk):** Blob-based file loading never reaches the format branches because the fragment portion carrying the filename gets stripped in `detectFormat`.
In `loadModelFromFile` you construct `fakeUrl = url + '#/' + file.name` so the format can be inferred from `file.name`, but `detectFormat` now strips everything after `#`, so the extension is lost and `fmt` is always `'unknown'`, causing all dropped files to fail. Consider either preserving the fragment when detecting the extension (e.g., only stripping the query or parsing the last segment after `#`), or computing `fmt` once from `fakeUrl` (including the fragment) and passing it directly into `loadModel` instead of recomputing it from the cleaned URL.
</issue_to_address>
### Comment 2
<location path="src/modules/ForgePanel.js" line_range="236-240" />
<code_context>
+ const container = document.getElementById('forgeTree');
+ if (!container || !this.treeData) return;
+
+ const lines = treeToHTML(this.treeData);
+ container.innerHTML = lines.join('');
+
+ // Wire click events
+ container.addEventListener('click', (e) => {
+ const togBtn = e.target.closest('.forge-toggle');
+ if (togBtn) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Rebinding the click handler on every `renderTree()` call can lead to duplicate listeners and repeated actions.
Because `renderTree()` runs multiple times, this code keeps adding new `click` listeners to `#forgeTree` without removing the old ones, causing duplicated actions and a small memory leak. Prefer registering the `click` handler once (e.g. in the constructor or `initDropZone`) and letting `renderTree()` only update `innerHTML`, still using event delegation on the single, persistent listener.
</issue_to_address>
### Comment 3
<location path="src/modules/ForgePanel.js" line_range="237" />
<code_context>
container.innerHTML = lines.join('');
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="src/modules/ForgePanel.js" line_range="237" />
<code_context>
container.innerHTML = lines.join('');
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `container.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (lower.endsWith('.fbx')) return 'fbx'; | ||
| if (lower.endsWith('.gltf')) return 'gltf'; | ||
| if (lower.endsWith('.glb')) return 'glb'; | ||
| const clean = url.toLowerCase().split('?')[0].split('#')[0]; |
There was a problem hiding this comment.
issue (bug_risk): Blob-based file loading never reaches the format branches because the fragment portion carrying the filename gets stripped in detectFormat.
In loadModelFromFile you construct fakeUrl = url + '#/' + file.name so the format can be inferred from file.name, but detectFormat now strips everything after #, so the extension is lost and fmt is always 'unknown', causing all dropped files to fail. Consider either preserving the fragment when detecting the extension (e.g., only stripping the query or parsing the last segment after #), or computing fmt once from fakeUrl (including the fragment) and passing it directly into loadModel instead of recomputing it from the cleaned URL.
| const lines = treeToHTML(this.treeData); | ||
| container.innerHTML = lines.join(''); | ||
|
|
||
| // Wire click events | ||
| container.addEventListener('click', (e) => { |
There was a problem hiding this comment.
issue (bug_risk): Rebinding the click handler on every renderTree() call can lead to duplicate listeners and repeated actions.
Because renderTree() runs multiple times, this code keeps adding new click listeners to #forgeTree without removing the old ones, causing duplicated actions and a small memory leak. Prefer registering the click handler once (e.g. in the constructor or initDropZone) and letting renderTree() only update innerHTML, still using event delegation on the single, persistent listener.
| if (!container || !this.treeData) return; | ||
|
|
||
| const lines = treeToHTML(this.treeData); | ||
| container.innerHTML = lines.join(''); |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| if (!container || !this.treeData) return; | ||
|
|
||
| const lines = treeToHTML(this.treeData); | ||
| container.innerHTML = lines.join(''); |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a container.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
🤖 Augment PR SummarySummary: This PR expands the 3D character creator with an admin “Forge” inspector mode, a more universal model loader, an automatic texture attachment pipeline, and a responsive layout fix for mobile/tablet. Changes:
Technical Notes: The new pipeline runs texture resolution after model load (before adding to the scene) and Forge hooks into the viewport as a drop zone while rendering a flat, collapsible hierarchy tree in the side panel. 🤖 Was this summary useful? React with 👍 or 👎 |
|
|
||
| const url = URL.createObjectURL(file); | ||
| // For FBX, rename blob URL so the loader can detect the format | ||
| const fakeUrl = url + '#/' + file.name; |
There was a problem hiding this comment.
loadModelFromFile() appends a #/filename.ext fragment to the blob URL so detectFormat() can see the extension, but detectFormat() currently strips fragments (split('#')[0]), which makes the format resolve to unknown and will likely break drag-and-drop loads.
Severity: high
Other Locations
src/modules/SmartLoader.js:66
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| if (fmt === 'dae') { | ||
| return new Promise((resolve, reject) => { | ||
| colladaLoader.load(url, (collada) => { | ||
| resolve({ scene: collada.scene, animations: collada.scene.animations || [], format: 'dae' }); |
There was a problem hiding this comment.
| container.innerHTML = lines.join(''); | ||
|
|
||
| // Wire click events | ||
| container.addEventListener('click', (e) => { |
There was a problem hiding this comment.
| async function tryLoadTexture(url) { | ||
| return new Promise((resolve) => { | ||
| textureLoader.load(url, (tex) => { | ||
| tex.colorSpace = THREE.SRGBColorSpace; |
There was a problem hiding this comment.
| }); | ||
|
|
||
| for (const mesh of meshes) { | ||
| const mat = Array.isArray(mesh.material) ? mesh.material[0] : mesh.material; |
There was a problem hiding this comment.
| export async function loadModelFromFile(file) { | ||
| const ext = '.' + file.name.split('.').pop().toLowerCase(); | ||
| const fmt = FORMAT_MAP[ext]; | ||
| if (!fmt) throw new Error(`[SmartLoader] Unsupported file: ${file.name}`); | ||
|
|
||
| const url = URL.createObjectURL(file); | ||
| // For FBX, rename blob URL so the loader can detect the format | ||
| const fakeUrl = url + '#/' + file.name; | ||
| try { | ||
| return await loadModel(fakeUrl, undefined, {}); | ||
| } finally { |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
loadModelFromFile() appends "#/filename.ext" to the blob URL so detectFormat() can infer the format, but detectFormat() strips hash fragments before extension checks. As a result, File/Blob loads (used by Forge drag-and-drop and file input) always resolve as "unknown" and throw "[SmartLoader] Unsupported format" for supported model files.
Suggestion: Change loadModelFromFile() so it passes the already-known format/extension into loadModel (e.g., via an explicit fmt/loader branch) or adjust detectFormat() to consider the filename hint on blob URLs instead of stripping the hash, ensuring File/Blob loading works for supported formats.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/modules/SmartLoader.js
**Line:** 163:173
**Comment:**
*CRITICAL: loadModelFromFile() appends "#/filename.ext" to the blob URL so detectFormat() can infer the format, but detectFormat() strips hash fragments before extension checks. As a result, File/Blob loads (used by Forge drag-and-drop and file input) always resolve as "unknown" and throw "[SmartLoader] Unsupported format" for supported model files.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| renderTree() { | ||
| const container = document.getElementById('forgeTree'); | ||
| if (!container || !this.treeData) return; | ||
|
|
||
| const lines = treeToHTML(this.treeData); | ||
| container.innerHTML = lines.join(''); | ||
|
|
||
| // Wire click events | ||
| container.addEventListener('click', (e) => { | ||
| const togBtn = e.target.closest('.forge-toggle'); |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
renderTree() attaches a new 'click' event listener to the #forgeTree container every time it is called, without removing or reusing prior handlers. After loading multiple models, each click is handled multiple times, causing toggle/visibility/focus actions to fire repeatedly and behave inconsistently.
Suggestion: Register the #forgeTree click handler once (e.g., in the ForgePanel constructor or init method) and only update container.innerHTML on re-render, or explicitly remove the old listener before adding a new one.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/modules/ForgePanel.js
**Line:** 232:241
**Comment:**
*HIGH: renderTree() attaches a new 'click' event listener to the #forgeTree container every time it is called, without removing or reusing prior handlers. After loading multiple models, each click is handled multiple times, causing toggle/visibility/focus actions to fire repeatedly and behave inconsistently.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Pull request overview
This PR adds a new “Forge” model-inspector workflow, expands the existing Three.js loader into a universal multi-format loader (with Draco/KTX2/HDR support), and implements a responsive layout fix so the 3D viewport remains usable on smaller screens.
Changes:
- Adds Forge mode UI + drag-and-drop model inspection with hierarchy export.
- Expands
SmartLoaderto support OBJ/MTL, DAE, STL, USDZ/USDC, Draco+KTX2, plus HDR environment loading. - Introduces an auto-texture pipeline that discovers sibling textures and generates fallback materials, and updates layout/CSS for mobile/tablet.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
index.html |
Adds responsive breakpoints, mobile panel toggles, and Forge panel UI + styles. |
src/main.js |
Initializes KTX2 support, wires Forge panel, and runs the texture auto-pipeline on character load. |
src/modules/SmartLoader.js |
Implements multi-format universal loader, KTX2 init, blob/file loading, and HDR environment helper. |
src/modules/ForgePanel.js |
New Forge model inspector: hierarchy tree, node stats, focus/visibility controls, JSON export. |
src/modules/TextureResolver.js |
New texture discovery + procedural fallback material generation pipeline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // For FBX, rename blob URL so the loader can detect the format | ||
| const fakeUrl = url + '#/' + file.name; | ||
| try { | ||
| return await loadModel(fakeUrl, undefined, {}); |
| if (fmt === 'dae') { | ||
| return new Promise((resolve, reject) => { | ||
| colladaLoader.load(url, (collada) => { | ||
| resolve({ scene: collada.scene, animations: collada.scene.animations || [], format: 'dae' }); |
| if (fmt === 'obj') { | ||
| const mtlUrl = opts.mtlUrl || url.replace(/\.obj$/i, '.mtl'); | ||
| let materials = null; | ||
| try { | ||
| materials = await new Promise((resolve) => { | ||
| mtlLoader.setPath(dirOf(mtlUrl)); | ||
| mtlLoader.load(mtlUrl.split('/').pop(), (mtl) => { | ||
| mtl.preload(); resolve(mtl); | ||
| }, undefined, () => resolve(null)); | ||
| }); | ||
| } catch { /* no MTL */ } | ||
| if (materials) objLoader.setMaterials(materials); | ||
| return new Promise((resolve, reject) => { | ||
| objLoader.load(url, (obj) => { | ||
| resolve({ scene: obj, animations: [], format: 'obj' }); | ||
| }, onProgress, reject); | ||
| }); |
| async function tryLoadTexture(url) { | ||
| return new Promise((resolve) => { | ||
| textureLoader.load(url, (tex) => { | ||
| tex.colorSpace = THREE.SRGBColorSpace; | ||
| tex.flipY = false; | ||
| resolve(tex); | ||
| }, undefined, () => resolve(null)); |
| for (const [channel, patterns] of Object.entries(CHANNEL_PATTERNS)) { | ||
| if (mat[channel]) continue; // already has a texture | ||
|
|
||
| for (const ext of TEXTURE_EXTS) { | ||
| for (const pattern of patterns) { | ||
| // Build candidate: modelName_diffuse.png, modelName_normal.png, etc. | ||
| const suffix = pattern.source.replace(/[\\^$.|?*+()[\]{}]/g, '').replace(/^_/, ''); | ||
| const candidate = `${baseDir}${modelBase}_${suffix}${ext}`; | ||
| const tex = await tryLoadTexture(candidate); | ||
| if (tex) { | ||
| mat[channel] = tex; | ||
| mat.needsUpdate = true; | ||
| attached = true; | ||
| break; | ||
| } | ||
| } | ||
| if (mat[channel]) break; | ||
| } | ||
| } |
| flat.push(`<div class="forge-node${vis}" data-uuid="${node.uuid}" data-depth="${node.depth}" style="padding-left:${indent}px"> | ||
| ${toggle} ${typeIcon} <span class="forge-name">${node.name}</span> ${badge} ${geoInfo} ${matInfo} | ||
| <button class="forge-vis" data-uuid="${node.uuid}" title="Toggle visibility">👁</button> | ||
| <button class="forge-focus" data-uuid="${node.uuid}" title="Focus camera">🎯</button> | ||
| </div>`); |
| // Wire click events | ||
| container.addEventListener('click', (e) => { | ||
| const togBtn = e.target.closest('.forge-toggle'); | ||
| if (togBtn) { | ||
| const row = togBtn.closest('.forge-node'); | ||
| const depth = parseInt(row.dataset.depth); | ||
| let sibling = row.nextElementSibling; | ||
| const collapsing = togBtn.textContent === '▼'; | ||
| togBtn.textContent = collapsing ? '▶' : '▼'; | ||
| while (sibling && parseInt(sibling.dataset.depth) > depth) { | ||
| sibling.style.display = collapsing ? 'none' : ''; | ||
| sibling = sibling.nextElementSibling; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const visBtn = e.target.closest('.forge-vis'); | ||
| if (visBtn) { | ||
| const obj = this._objectMap.get(visBtn.dataset.uuid); | ||
| if (obj) { | ||
| obj.visible = !obj.visible; | ||
| visBtn.closest('.forge-node').classList.toggle('forge-hidden', !obj.visible); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const focusBtn = e.target.closest('.forge-focus'); | ||
| if (focusBtn) { | ||
| const obj = this._objectMap.get(focusBtn.dataset.uuid); | ||
| if (obj) this.focusOn(obj); | ||
| return; | ||
| } | ||
| }); |
| import { OrbitControls } from 'three/addons/controls/OrbitControls.js'; | ||
| import { SkeletonHelper } from 'three'; | ||
| import { loadModel, loadAnimationClips, prepareModel, fbxLoader } from './modules/SmartLoader.js'; | ||
| import { loadModel, loadModelFromFile, loadAnimationClips, prepareModel, fbxLoader, initKTX2, SUPPORTED_EXTENSIONS } from './modules/SmartLoader.js'; |
| */ | ||
|
|
||
| import * as THREE from 'three'; | ||
| import { loadModelFromFile, prepareModel, SUPPORTED_EXTENSIONS } from './SmartLoader.js'; |
| flat.push(`<div class="forge-node${vis}" data-uuid="${node.uuid}" data-depth="${node.depth}" style="padding-left:${indent}px"> | ||
| ${toggle} ${typeIcon} <span class="forge-name">${node.name}</span> ${badge} ${geoInfo} ${matInfo} | ||
| <button class="forge-vis" data-uuid="${node.uuid}" title="Toggle visibility">👁</button> | ||
| <button class="forge-focus" data-uuid="${node.uuid}" title="Focus camera">🎯</button> | ||
| </div>`); |
There was a problem hiding this comment.
Suggestion: Node names from model files are injected directly into HTML, so a crafted model name can execute script in the panel. Build DOM nodes with textContent (or escape content) instead of interpolating untrusted text into innerHTML. [security]
Severity Level: Critical 🚨
- ❌ Malicious model can execute arbitrary JS in Forge UI.
- ❌ Compromised admin session risks account or data theft.Steps of Reproduction ✅
1. In any environment where `initScene()` (`src/main.js:61-158`) runs, Forge is
initialized via `forgePanel = new ForgePanel(...)` at `src/main.js:147` and
`forgePanel.initDropZone(container)` at `src/main.js:148`, enabling drag-and-drop model
loading.
2. Prepare or obtain a 3D model whose node name is a malicious HTML string, e.g. set a
mesh or bone name to `<img src=x onerror="alert('xss')">` inside the model file
(GLTF/FBX/etc.), then drop this file onto the viewport so `ForgePanel.loadDroppedFile()`
at `src/modules/ForgePanel.js:203-223` runs.
3. `loadDroppedFile()` calls `buildTreeData(model)` (`src/modules/ForgePanel.js:82-107`),
which copies `obj.name` into `node.name` at `src/modules/ForgePanel.js:88` without
sanitization, then `renderTree()` at `src/modules/ForgePanel.js:232-272` generates HTML
using `treeToHTML()` (`src/modules/ForgePanel.js:109-136`).
4. In `treeToHTML()`, `flat.push(\`... <span class="forge-name">${node.name}</span>
...\`)` at `src/modules/ForgePanel.js:125-129` injects the untrusted `node.name` directly
into an HTML string; `renderTree()` then sets `container.innerHTML = lines.join('');` at
`src/modules/ForgePanel.js:237`, causing the browser to parse and execute any embedded
script or event handlers from the model-controlled name, achieving script execution in the
characters.grudge-studio.com origin.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/modules/ForgePanel.js
**Line:** 125:129
**Comment:**
*Security: Node names from model files are injected directly into HTML, so a crafted model name can execute script in the panel. Build DOM nodes with `textContent` (or escape content) instead of interpolating untrusted text into `innerHTML`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| container.innerHTML = lines.join(''); | ||
|
|
||
| // Wire click events | ||
| container.addEventListener('click', (e) => { |
There was a problem hiding this comment.
Suggestion: renderTree re-attaches a new click listener every time a model is loaded, so each subsequent load multiplies handlers and causes duplicate toggles/focus actions. Bind the listener once (or remove the previous listener) before adding a new one. [memory leak]
Severity Level: Major ⚠️
- ❌ Forge tree controls fire multiple times per user click.
- ⚠️ Visibility toggles may appear to do nothing or flicker.Steps of Reproduction ✅
1. Load the app so `initScene()` in `src/main.js:61-158` runs, which creates `forgePanel =
new ForgePanel(scene, camera, controls, updateStatus);` at `src/main.js:147` and calls
`forgePanel.initDropZone(container)` at `src/main.js:148`.
2. Drop a first model file onto the viewport element; the `drop` listener in
`ForgePanel.initDropZone()` (`src/modules/ForgePanel.js:187-200`) calls
`this.loadDroppedFile(file)` at `src/modules/ForgePanel.js:199`.
3. Inside `loadDroppedFile()` (`src/modules/ForgePanel.js:203-223`), after loading and
preparing the model, `this.renderTree()` is invoked at `src/modules/ForgePanel.js:223`,
which sets `container.innerHTML = lines.join('');` and attaches a new `'click'` handler
via `container.addEventListener('click', ...)` at `src/modules/ForgePanel.js:237-240`.
4. Drop a second model file; `loadDroppedFile()` runs again and calls `renderTree()`
again, which re-attaches another `'click'` listener to the same `#forgeTree` container
without removing the previous one. Subsequent clicks on `.forge-toggle`, `.forge-vis`, or
`.forge-focus` inside `#forgeTree` now execute their logic multiple times (once per
attachment), causing duplicate toggles/focus actions and growing handler count on every
model load.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/modules/ForgePanel.js
**Line:** 237:240
**Comment:**
*Memory Leak: `renderTree` re-attaches a new `click` listener every time a model is loaded, so each subsequent load multiplies handlers and causes duplicate toggles/focus actions. Bind the listener once (or remove the previous listener) before adding a new one.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const collapsing = togBtn.textContent === '▼'; | ||
| togBtn.textContent = collapsing ? '▶' : '▼'; | ||
| while (sibling && parseInt(sibling.dataset.depth) > depth) { | ||
| sibling.style.display = collapsing ? 'none' : ''; | ||
| sibling = sibling.nextElementSibling; |
There was a problem hiding this comment.
Suggestion: The expand/collapse state logic is inverted: rows start visible with a closed icon, and first click only changes icon instead of collapsing children. Initialize expanded state consistently (or invert the condition) so one click actually collapses. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Forge tree expand/collapse requires two clicks initially.
- ⚠️ Confusing mismatch between toggle icon and row visibility.Steps of Reproduction ✅
1. Load the app and initialize the Forge panel via `initScene()` in `src/main.js:61-158`,
then drop any model so `ForgePanel.loadDroppedFile()` at
`src/modules/ForgePanel.js:203-223` builds `this.treeData` and calls `this.renderTree()`
at `src/modules/ForgePanel.js:223`.
2. `renderTree()` (`src/modules/ForgePanel.js:232-272`) builds HTML via `treeToHTML()`
where each expandable node gets a toggle span with initial text `'▶'` (`treeToHTML()` at
`src/modules/ForgePanel.js:111-112`) while all child rows are rendered visible.
3. In the click handler at `src/modules/ForgePanel.js:239-251`, clicking a `.forge-toggle`
sets `const collapsing = togBtn.textContent === '▼';` at `line 246`, and then flips
`togBtn.textContent` between `'▶'` and `'▼'` at `line 247`, hiding or showing descendants
in the loop at `lines 248-250`.
4. Because the initial icon text is `'▶'` (children visible), the first click makes
`collapsing` false, only changing the icon to `'▼'` but leaving `sibling.style.display`
unchanged (`''`), so children remain visible; only on the second click (with text now
`'▼'`) does `collapsing` become true and the loop hides descendants, meaning users must
click twice to actually collapse a node.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/modules/ForgePanel.js
**Line:** 246:250
**Comment:**
*Incorrect Condition Logic: The expand/collapse state logic is inverted: rows start visible with a closed icon, and first click only changes icon instead of collapsing children. Initialize expanded state consistently (or invert the condition) so one click actually collapses.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (materials) objLoader.setMaterials(materials); | ||
| return new Promise((resolve, reject) => { | ||
| objLoader.load(url, (obj) => { | ||
| resolve({ scene: obj, animations: [], format: 'obj' }); | ||
| }, onProgress, reject); | ||
| }); |
There was a problem hiding this comment.
Suggestion: OBJLoader is a singleton and materials are only set when an MTL is found, so a previously loaded MTL can leak into later OBJ loads without MTL. Clear/reset loader materials when no MTL is available to avoid stale material assignment. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Later OBJ loads can inherit materials from earlier ones.
- ⚠️ Visual appearance of OBJ assets can be misleading or wrong.Steps of Reproduction ✅
1. The universal loader exposes `loadModel(url, onProgress, opts)` in
`src/modules/SmartLoader.js:88-156`, which is already used for character models in
`loadCharacterModel()` at `src/main.js:163-193` and is intended to support `.obj` via
`FORMAT_MAP` at `src/modules/SmartLoader.js:59-63`.
2. When `detectFormat(url)` returns `'obj'` (e.g. for a URL ending in `.obj`), the OBJ
branch in `loadModel()` at `src/modules/SmartLoader.js:107-124` runs and attempts to load
an MTL file, setting `materials` based on `mtlLoader.load()` at `lines 111-116`.
3. If an MTL is successfully loaded, `if (materials) objLoader.setMaterials(materials);`
at `src/modules/SmartLoader.js:118` assigns those materials to the singleton `objLoader`
instance defined at `src/modules/SmartLoader.js:23`, and `objLoader.load(url, ...)` at
`lines 119-122` uses them for rendering the OBJ.
4. On a subsequent `loadModel(otherUrl)` call for a different `.obj` that has no MTL or
uses a different MTL, the `materials` variable becomes `null` (or a different set), but
there is no `else` branch to clear previous materials; `objLoader` retains the prior
materials, so `objLoader.load(otherUrl, ...)` reuses stale materials, causing the later
OBJ to render with the wrong material set even though the loader API (and `FORMAT_MAP`)
imply independence between loads.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/modules/SmartLoader.js
**Line:** 118:123
**Comment:**
*Stale Reference: `OBJLoader` is a singleton and materials are only set when an MTL is found, so a previously loaded MTL can leak into later OBJ loads without MTL. Clear/reset loader materials when no MTL is available to avoid stale material assignment.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const fakeUrl = url + '#/' + file.name; | ||
| try { | ||
| return await loadModel(fakeUrl, undefined, {}); | ||
| } finally { |
There was a problem hiding this comment.
Suggestion: loadModelFromFile appends the filename in the URL fragment, but detectFormat strips fragments before checking extensions. That makes dropped files resolve to unknown format and fail to load; detect the format from file.name directly (or preserve the fragment in format detection). [api mismatch]
Severity Level: Critical 🚨
- ❌ Forge drag-and-drop models always fail to load.
- ⚠️ Forge file input also cannot preview dropped models.Steps of Reproduction ✅
1. Start the app so `initScene()` in `src/main.js:61-158` runs, initializing Forge via
`forgePanel = new ForgePanel(...)` at `src/main.js:147` and wiring drag-and-drop in
`ForgePanel.initDropZone()` (`src/modules/ForgePanel.js:174-201`).
2. Drop any supported file type (e.g. `character.fbx`) onto the viewport; the `drop`
handler at `src/modules/ForgePanel.js:194-200` calls `forgePanel.loadDroppedFile(file)`.
3. Inside `loadDroppedFile()` (`src/modules/ForgePanel.js:203-223`), the file is passed to
`loadModelFromFile(file)` from `SmartLoader` at `src/modules/ForgePanel.js:206`;
`loadModelFromFile()` (`src/modules/SmartLoader.js:163-176`) creates `url =
URL.createObjectURL(file)` and then builds `const fakeUrl = url + '#/' + file.name;` at
`line 170` before calling `return await loadModel(fakeUrl, undefined, {});` at `line 172`.
4. `loadModel()` (`src/modules/SmartLoader.js:88-155`) calls `detectFormat(url)` at `line
89`, but `detectFormat()` strips the fragment with `.split('#')[0]` at
`src/modules/SmartLoader.js:65-67`, so `clean` is a `blob:` URL with no extension,
returning `'unknown'` at `line 70`; `loadModel()` then falls through to `throw new
Error(\`[SmartLoader] Unsupported format: ${url}\`);` at `line 155`, causing
`loadDroppedFile()` to hit the `catch` block at `src/modules/ForgePanel.js:224-227` and
display a Forge error instead of loading the model.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/modules/SmartLoader.js
**Line:** 170:173
**Comment:**
*Api Mismatch: `loadModelFromFile` appends the filename in the URL fragment, but `detectFormat` strips fragments before checking extensions. That makes dropped files resolve to `unknown` format and fail to load; detect the format from `file.name` directly (or preserve the fragment in format detection).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| textureLoader.load(url, (tex) => { | ||
| tex.colorSpace = THREE.SRGBColorSpace; | ||
| tex.flipY = false; | ||
| resolve(tex); |
There was a problem hiding this comment.
Suggestion: All discovered textures are forced to sRGB, including normal/roughness/metalness/AO maps, which must remain linear. This produces incorrect lighting/shading; set color space per channel and only use sRGB for base-color/emissive maps. [logic error]
Severity Level: Major ⚠️
- ⚠️ Discovered PBR maps render with incorrect lighting response.
- ⚠️ Normal and roughness maps can look flat or over-contrasted.Steps of Reproduction ✅
1. Character models are loaded via `loadCharacterModel(raceConfig)`
(`src/main.js:163-229`), which, after `prepareModel(model);` at `src/main.js:185`, calls
`const texResult = await resolveTextures(model, raceConfig.model);` at
`src/main.js:187-189` to run the texture pipeline.
2. `resolveTextures()` in `src/modules/TextureResolver.js:263-295` traverses meshes and,
when `modelUrl` is provided, calls `discoverTextures(mesh, modelUrl);` at
`src/modules/TextureResolver.js:281-283` to attach sibling textures based on naming
conventions across all channels in `CHANNEL_PATTERNS` (`map`, `normalMap`, `roughnessMap`,
`metalnessMap`, `aoMap`, `emissiveMap`) defined at `src/modules/TextureResolver.js:23-43`.
3. `discoverTextures()` builds candidate URLs and calls `tryLoadTexture(candidate)` at
`src/modules/TextureResolver.js:233-235`, regardless of which channel is being filled;
`tryLoadTexture()` (`src/modules/TextureResolver.js:199-206`) uses
`textureLoader.load(url, (tex) => { tex.colorSpace = THREE.SRGBColorSpace; ... })`,
forcing **all** loaded textures into sRGB color space.
4. This means non-color maps such as `normalMap`, `roughnessMap`, `metalnessMap`, and
`aoMap` are incorrectly tagged as sRGB instead of linear, so when a model lacking embedded
textures relies on discovered `_normal`, `_roughness`, or `_metallic` PNGs, the shading
and lighting computed by three.js will be physically incorrect (normals distorted,
roughness/metalness curves wrong), degrading visual fidelity of resolved models.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/modules/TextureResolver.js
**Line:** 201:204
**Comment:**
*Logic Error: All discovered textures are forced to sRGB, including normal/roughness/metalness/AO maps, which must remain linear. This produces incorrect lighting/shading; set color space per channel and only use sRGB for base-color/emissive maps.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const mat = Array.isArray(mesh.material) ? mesh.material[0] : mesh.material; | ||
|
|
||
| // Skip meshes that already have a proper texture | ||
| if (hasTexture(mat)) continue; | ||
|
|
||
| // Phase 1: Try to discover textures from sibling files | ||
| if (modelUrl) { | ||
| const found = await discoverTextures(mesh, modelUrl); | ||
| if (found) { discovered++; continue; } | ||
| } | ||
|
|
||
| // Phase 2: Generate procedural material | ||
| if (useToon) { | ||
| mesh.material = createToonMaterial(mesh, mat); | ||
| } else { | ||
| mesh.material = createPBRMaterial(mesh, mat); |
There was a problem hiding this comment.
Suggestion: Multi-material meshes are reduced to the first material for checks and then can be overwritten with a single generated material, dropping submesh materials and textures. Iterate and process each material index instead of collapsing to one. [logic error]
Severity Level: Major ⚠️
- ⚠️ Multi-material meshes lose distinct submesh materials after resolve.
- ⚠️ Existing submesh textures can be silently replaced by a single toon/PBR.Steps of Reproduction ✅
1. Models are loaded via `loadCharacterModel()` in `src/main.js:163-229`, which calls
`resolveTextures(model, raceConfig.model);` at `src/main.js:187-189` after
`prepareModel(model);`, so every playable character model goes through the
`TextureResolver` pipeline.
2. Inside `resolveTextures()` (`src/modules/TextureResolver.js:263-295`), the code
collects all meshes (`isMesh` or `isSkinnedMesh`) and for each mesh does `const mat =
Array.isArray(mesh.material) ? mesh.material[0] : mesh.material;` at `line 275`,
effectively ignoring any additional materials for multi-material meshes.
3. For meshes where this first material `mat` has no diffuse texture (`hasTexture(mat)`
check at `src/modules/TextureResolver.js:277-278` is false), it optionally tries
`discoverTextures(mesh, modelUrl);` at `lines 281-283`, which also only inspects and
mutates `mat`, not the full `mesh.material` array, so additional submesh materials are
never considered.
4. If no textures are discovered, the fallback branch at
`src/modules/TextureResolver.js:286-291` sets `mesh.material = createToonMaterial(mesh,
mat);` or `mesh.material = createPBRMaterial(mesh, mat);`, replacing the entire
`mesh.material` array with a single material. On any multi-material mesh (multiple
submeshes using different slices of `mesh.geometry`), this collapses all submeshes to one
generated material, discarding their distinct materials and any existing textures, so
multi-material models lose their intended per-submesh appearance after `resolveTextures()`
runs.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/modules/TextureResolver.js
**Line:** 275:290
**Comment:**
*Logic Error: Multi-material meshes are reduced to the first material for checks and then can be overwritten with a single generated material, dropping submesh materials and textures. Iterate and process each material index instead of collapsing to one.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
CodeAnt AI finished reviewing your PR. |
User description
Summary
Two major features + a critical responsive fix for characters.grudge-studio.com.
1. Responsive Viewport Fix
The layout used a fixed 3-column CSS grid (
260px + 1fr + 300px) that required 560px minimum width. On mobile, the 3D canvas was crushed to zero.2. Forge Mode — Model Inspector + Hierarchy Tool
New admin tool for breaking down any 3D model:
3. Universal Model Loader
SmartLoader upgraded from 3 formats to 8:
loadModelFromFile()for drag-and-drop File/Blob intake4. Texture Auto-Pipeline
3-stage pipeline runs on every model load:
_diffuse,_normal,_roughness, etc.)Files Changed
index.html— Responsive CSS (3 breakpoints) + Forge panel UI + toggle buttonssrc/main.js— KTX2 init, ForgePanel/TextureResolver wiring, SRGB color spacesrc/modules/SmartLoader.js— 8-format universal loader + Draco/KTX2 + HDRsrc/modules/ForgePanel.js— New: model inspector + hierarchy + appendage labelingsrc/modules/TextureResolver.js— New: texture discovery + auto-attach + fallback genCo-Authored-By: Oz oz-agent@warp.dev
Summary by Sourcery
Add an in-browser Forge inspector mode with drag-and-drop model support, extend the 3D asset loader to handle multiple formats and HDR environments, make the character creator layout responsive across tablet and mobile, and introduce an automatic texture resolution pipeline for loaded models.
New Features:
Bug Fixes:
Enhancements:
CodeAnt-AI Description
Add Forge model inspection, broader model support, texture auto-fill, and mobile layout fixes
What Changed
Impact
✅ Easier model inspection✅ Fewer missing-texture imports✅ Usable 3D viewport on phones💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.