Conversation
dosubot
bot
added
the
size:S
📝 WalkthroughWalkthroughThe changes modify dimension handling in the 3D viewer by introducing conditional initialization. Default target dimensions are reduced from 512 to 0, and the composable now only enables viewer mode and provides dimension retrieval when both width and height dimensions are explicitly available. Changes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)tests-ui/**/*.test.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
Files:
tests-ui/**/*.test.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (6)📚 Learning: 2025-12-17T01:01:03.609ZApplied to files:
📚 Learning: 2025-12-09T03:39:54.501ZApplied to files:
📚 Learning: 2025-12-13T11:03:11.264ZApplied to files:
📚 Learning: 2025-12-17T00:40:09.635ZApplied to files:
📚 Learning: 2025-12-10T03:09:13.807ZApplied to files:
📚 Learning: 2025-12-11T12:25:15.470ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 12/18/2025, 07:56:40 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results🕵🏻 No test results found ⏰ Completed at: 12/19/2025, 04:53:00 AM UTC 📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.21 MB (baseline 3.21 MB) • 🔴 +70 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 996 kB (baseline 996 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.54 kB (baseline 6.54 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 297 kB (baseline 297 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 184 kB (baseline 184 kB) • ⚪ 0 BReusable component library chunks
Status: 8 added / 8 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 1.86 kB (baseline 1.86 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 8.46 MB (baseline 8.46 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.44 MB (baseline 3.44 MB) • ⚪ 0 BBundles that do not match a named category
Status: 20 added / 20 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/extensions/core/load3d/Load3d.ts (2)
273-280: Guard against division by zero in aspect ratio calculation.If
getDimensionsCallbackreturns dimensions where width or height is 0 or negative, line 278 will compute an invalid aspect ratio (NaN or Infinity). While widgets likely have sensible defaults in practice, defensive validation would prevent edge cases.🔎 Apply this diff to add validation:
if (this.getDimensionsCallback) { const dims = this.getDimensionsCallback() - if (dims) { + if (dims && dims.width > 0 && dims.height > 0) { this.targetWidth = dims.width this.targetHeight = dims.height this.targetAspectRatio = dims.width / dims.height } }
622-629: Guard against division by zero in aspect ratio calculation (duplicate issue).Same issue as lines 273-280: if
getDimensionsCallbackreturns zero or negative dimensions, line 627 will compute an invalid aspect ratio.🔎 Apply this diff to add validation:
if (this.getDimensionsCallback) { const dims = this.getDimensionsCallback() - if (dims) { + if (dims && dims.width > 0 && dims.height > 0) { this.targetWidth = dims.width this.targetHeight = dims.height this.targetAspectRatio = dims.width / dims.height } }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/composables/useLoad3dViewer.ts(1 hunks)src/extensions/core/load3d/Load3d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
src/**/*.{vue,ts}: Avoid usingrefandwatchtogether - usecomputedinstead if possible
Do not addrefif a prop would suffice; do not addcomputedif the ref/prop directly would work; usewatchonly whencomputedwon't accomplish the goal
Leverage VueUse functions for performance-enhancing styles
Files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
src/**/*.ts: Extract complex type definitions that are inlined in multiple related places - name them for reuse
Minimize the surface area (exported values) of each module and composable
Favor pure functions, especially testable ones
Files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using @ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
src/**/*.{ts,tsx,vue}: ESLint rules: Vue + TS rules, no floating promises, unused imports disallowed, i18n raw text restrictions in templates
Use es-toolkit for utility functions
Avoid mutable state - prefer immutability and assignment at point of declaration
Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
src/**/*.{js,ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript exclusively - no new JavaScript files
Files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
src/**/*.{ts,tsx,vue,js}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx,vue,js}: Indent with 2 spaces, use single quotes, no trailing semicolons, line width 80 - see.prettierrc
Sort and group imports by plugin, runpnpm formatbefore committing
Files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use function expressions - use function declarations instead when possible
Files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
src/**/{services,composables}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/{services,composables}/**/*.{ts,tsx}: Useapi.apiURL()for backend endpoints instead of constructing URLs directly
Useapi.fileURL()for static file access instead of constructing URLs directly
Files:
src/composables/useLoad3dViewer.ts
src/**/{composables,components}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Clean up subscriptions in state management to prevent memory leaks
Files:
src/composables/useLoad3dViewer.ts
src/**/{components,composables}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Use vue-i18n for ALL user-facing strings by adding them to
src/locales/en/main.json
Files:
src/composables/useLoad3dViewer.ts
src/composables/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Composables must follow the naming pattern
useXyz.ts
Files:
src/composables/useLoad3dViewer.ts
🧠 Learnings (4)
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
📚 Learning: 2025-12-13T11:03:11.264Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Applied to files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
📚 Learning: 2025-12-17T00:40:09.635Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Applied to files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
📚 Learning: 2025-12-11T12:25:15.470Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Applied to files:
src/extensions/core/load3d/Load3d.tssrc/composables/useLoad3dViewer.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: test
- GitHub Check: collect
- GitHub Check: lint-and-format
🔇 Additional comments (2)
src/extensions/core/load3d/Load3d.ts (1)
46-47: LGTM! Default target dimensions now properly indicate "no target size".Changing the defaults from 512 to 0 correctly implements the PR objective that "preview3d" should not have a target size. This pairs well with the
shouldMaintainAspectRatio()check on line 249 that verifies both dimensions are greater than 0 before maintaining aspect ratio.src/composables/useLoad3dViewer.ts (1)
190-201: LGTM! Conditional viewer mode initialization correctly implements the PR objective.The logic properly enables viewer mode and dimension tracking only when both width and height widgets exist. This aligns perfectly with the PR objective that "preview3d" (which presumably lacks these widgets) should not have a target size.
The conditional initialization pattern is clear and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests-ui/tests/composables/useLoad3dViewer.test.ts (1)
157-182: Add test coverage for the case when width/height widgets are present.The test correctly validates the behavior when no widgets exist (
isViewerMode: false). However, the PR introduces conditional logic where viewer mode is enabled only when both width and height widgets are present. To fully test this behavioral change and prevent future regressions, add a test case that verifiesisViewerMode: trueandgetDimensionsis provided when the node has width/height widgets.🔎 Suggested test case to add:
it('should initialize viewer mode when width and height widgets are present', async () => { // Add width and height widgets to the mock node mockNode.widgets = [ { name: 'width', value: 512 }, { name: 'height', value: 512 } ] const viewer = useLoad3dViewer(mockNode) const containerRef = document.createElement('div') await viewer.initializeViewer(containerRef, mockSourceLoad3d) expect(Load3d).toHaveBeenCalledWith(containerRef, { width: 512, height: 512, getDimensions: expect.any(Function), isViewerMode: true }) })Based on coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions" and "Aim for behavioral coverage of critical and new features."
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests-ui/tests/composables/useLoad3dViewer.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests-ui/**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
tests-ui/**/*.test.{js,ts,jsx,tsx}: Write tests for new features
Follow existing test patterns in the codebase
Use existing test utilities rather than writing custom utilities
Mock external dependencies in tests
Always prefer vitest mock functions over writing verbose manual mocks
Files:
tests-ui/tests/composables/useLoad3dViewer.test.ts
tests-ui/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
tests-ui/**/*.test.ts: Write tests for all changes, especially bug fixes to catch future regressions
Unit/Component test files must use pattern**/*.test.ts
Do not write change detector tests - do not just assert that defaults are certain values
Do not write tests dependent on non-behavioral features like utility classes or styles
Be parsimonious in testing - do not write redundant tests
Do not write tests that just test the mocks - ensure tests fail when code behaves unexpectedly
For mocking, leverage Vitest's utilities where possible
Keep module mocks contained - do not use global mutable state within test files, usevi.hoisted()if necessary
For Component testing, use Vue Test Utils and follow advice about making components easy to test
Aim for behavioral coverage of critical and new features
Files:
tests-ui/tests/composables/useLoad3dViewer.test.ts
🧠 Learnings (6)
📚 Learning: 2025-12-17T01:01:03.609Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T01:01:03.609Z
Learning: Applies to tests-ui/**/*.test.ts : Do not write change detector tests - do not just assert that defaults are certain values
Applied to files:
tests-ui/tests/composables/useLoad3dViewer.test.ts
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
tests-ui/tests/composables/useLoad3dViewer.test.ts
📚 Learning: 2025-12-13T11:03:11.264Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Applied to files:
tests-ui/tests/composables/useLoad3dViewer.test.ts
📚 Learning: 2025-12-17T00:40:09.635Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Applied to files:
tests-ui/tests/composables/useLoad3dViewer.test.ts
📚 Learning: 2025-12-10T03:09:13.807Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7303
File: src/components/topbar/CurrentUserPopover.test.ts:199-205
Timestamp: 2025-12-10T03:09:13.807Z
Learning: In test files, prefer selecting or asserting on accessible properties (text content, aria-label, role, accessible name) over data-testid attributes. This ensures tests validate actual user-facing behavior and accessibility, reducing reliance on implementation details like test IDs.
Applied to files:
tests-ui/tests/composables/useLoad3dViewer.test.ts
📚 Learning: 2025-12-11T12:25:15.470Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Applied to files:
tests-ui/tests/composables/useLoad3dViewer.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: lint-and-format
- GitHub Check: test
- GitHub Check: collect
| width: width.value as number, | ||
| height: height.value as number |
There was a problem hiding this comment.
Question: Why do we need so many type assertions around these?
There was a problem hiding this comment.
width and height need number type, but width.value could be others
There was a problem hiding this comment.
Can we constrain the type upstream?
## Summary improve 3d scene size logic, for preview3d, it should not have target size ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7619-improve-logic-of-3d-scene-size-2cd6d73d36508101b9f5dcef9bbe1314) by [Unito](https://www.unito.io)
2 participants
Summary
improve 3d scene size logic, for preview3d, it should not have target size
┆Issue is synchronized with this Notion page by Unito