Skip to content

[Page] Update: Sync prototype editor UI components#3194

Open
qingqing-ux wants to merge 38 commits into
goplus:uifrom
qingqing-ux:prototype-frontend-sync-2
Open

[Page] Update: Sync prototype editor UI components#3194
qingqing-ux wants to merge 38 commits into
goplus:uifrom
qingqing-ux:prototype-frontend-sync-2

Conversation

@qingqing-ux
Copy link
Copy Markdown
Collaborator

@qingqing-ux qingqing-ux commented May 20, 2026

Issue

N/A

Background

Continue prototype editor synchronization on a stacked branch after the current #3190 work, keeping the prototype aligned with the real Builder frontend while preserving local offline behavior.

Changes

  • Split editor surfaces into local prototype components: navbar, preview panel, asset panel, and map editor panel
  • Added local UI primitives copied/adapted from the frontend style: modal, form modal, button group, and checkbox
  • Restored editor interactions including API reference drag/drop, sidebar resize, API hover docs, sprite transform controls, animation settings, rename flows, and sprite generator dropdowns
  • Updated prototype contract checks for the synced editor behaviors and local UI component boundaries

Scope

  • Prototype editor page
  • Prototype editor preview, asset, animation, sprite generator, publish, and map editor panels
  • Prototype UI components used by the editor

Design System Impact

  • Yes
  • No

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces significant updates to the editor's UI components, including the implementation of a new PreviewPanel, modal components (UIModal, UIFormModal), and checkbox/button group controls. It also refactors the sprite and asset management logic, introduces drag-and-drop functionality for code snippets, and updates CSS variables for consistent styling. My review highlights two areas for improvement: the use of 'any' for the 'ctx' prop in the new PreviewPanel, which should be typed for better maintainability, and a potential memory leak in the snippet sidebar resize logic due to uncleaned event listeners.

import transformerFlipArrowUrl from '@/assets/editor/custom-transformer/transformer-flip-arrow.png'

const props = defineProps<{
ctx: any
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using any for the ctx prop bypasses TypeScript's type checking. Since this context object is used extensively throughout the component, defining a proper interface or type would improve maintainability and catch potential errors during development.

Comment on lines +1050 to +1073
function startSnippetSidebarResize(event: MouseEvent) {
if (!(event.currentTarget instanceof HTMLElement)) return
const codeBody = event.currentTarget.closest('.code-body')
if (!(codeBody instanceof HTMLElement)) return
event.preventDefault()
snippetSidebarResizing.value = true
const initialClientX = event.clientX
const initialWidth = snippetSidebarWidth.value
const maxWidth = Math.max(minSnippetSidebarWidth, codeBody.clientWidth - 60 - minCodeEditorWidth)

function handleMouseMove(moveEvent: MouseEvent) {
const offset = moveEvent.clientX - initialClientX
snippetSidebarWidth.value = Math.min(Math.max(minSnippetSidebarWidth, initialWidth + offset), maxWidth)
}

function endResizing() {
snippetSidebarResizing.value = false
window.removeEventListener('mousemove', handleMouseMove)
window.removeEventListener('mouseup', endResizing)
}

window.addEventListener('mousemove', handleMouseMove)
window.addEventListener('mouseup', endResizing)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The startSnippetSidebarResize function adds mousemove and mouseup listeners to the window object. If the component is unmounted while a resize operation is in progress, these listeners will persist, potentially causing memory leaks or unexpected behavior. Consider adding a cleanup mechanism in onBeforeUnmount.

@fennoai
Copy link
Copy Markdown
Contributor

fennoai Bot commented May 20, 2026

Code Review: Prototype Editor UI Sync

Overall this is a solid prototype synchronization with good accessibility throughout (ARIA roles, keyboard nav, focus management). A few structural issues are worth addressing before the pattern spreads to more components.


Important: Split watch calls can leak the keydown listener — UIModal.vue lines 96–115

Two separate watch calls on props.visible — one adds the listener, the other removes it. If visible flips true → false → true within the same microtask before the removal watcher runs, addEventListener is called twice while removeEventListener is only called once, leaving a ghost listener.

Merge into a single watcher:

watch(
  () => props.visible,
  async (visible) => {
    if (visible) {
      document.addEventListener('keydown', handleKeydown)
      if (props.autoFocus) {
        await nextTick()
        const container = containerRef.value
        const focusTarget = container == null ? null : getFirstFocusableElement(container)
        ;(focusTarget ?? container)?.focus()
      }
    } else {
      document.removeEventListener('keydown', handleKeydown)
    }
  }
)

Important: withDefaults return value discarded — UIFormModal.vue lines 34–42

Every other component in this PR correctly writes const props = withDefaults(...). Without the assignment, any script-block logic that reads a prop will silently get undefined instead of the default:

const props = withDefaults(
  defineProps<{ title: string; visible?: boolean; ... }>(),
  { visible: false, ... }
)

Important: ctx: any removes all type safety — PreviewPanel.vue line 11

PreviewPanel has 30+ accesses to ctx.* covering sprite drag, resize, quick-config, publish, and runner state — with no compiler guard on any of them. Combined with the v-html bindings that render ctx.*Icon values (lines 42, 124, 149, 179, 191), TypeScript cannot flag a future path where an icon field is populated from an API response instead of a build-time raw SVG import.

Define and export a typed context interface from the editor page and use it as the prop type:

export type PreviewPanelContext = {
  runnerActive: boolean
  stageBackdrop: string
  stageCompanionSprites: SpriteCard[]
  selectedSprite: SpriteCard | null
  publishActionIcon: string
  // ...
}

Important (latent bug): Injected getter functions break reactive dependency tracking — UIButtonGroup.vue lines 45–50

provide(typeInjectionKey, () => props.type)
provide(variantInjectionKey, () => props.variant)

UIButtonGroupItem calls variant() and type() inside a computed. Plain function wrappers like these are not tracked by Vue's reactivity system — if the parent changes type or variant dynamically, items' rootClass computed will not invalidate and class bindings will go stale with no visible error.

Provide ComputedRef instead:

provide(typeInjectionKey, computed(() => props.type))
provide(variantInjectionKey, computed(() => props.variant))

And update the injection key types to InjectionKey<ComputedRef<UIButtonGroupType>>.


Minor (performance): getStageSpriteFrameStyle(sprite) plain function call in template — PreviewPanel.vue line 54

This allocates a new CSSProperties object for every companion sprite on every render. Since ctx is a single reactive() object, any unrelated property change (activeQuickConfig, publishStatusMessage, etc.) re-renders PreviewPanel and re-invokes this function for all sprites. The selected sprite already follows the correct pattern using computed(() => ...) in the editor page — companion sprites should do the same.


Minor: selectedCandidate fallback hides an empty-array bug — SpriteGeneratorModal.vue line 58

const selectedCandidate = computed(() => candidates[selectedIndex.value] ?? candidates[0])

?? only guards the left side. If candidates is ever empty (e.g., after wiring to a real API), both sides are undefined and template accesses like :src="selectedCandidate.image" throw at runtime with no type-system warning. The computed type should be GeneratedSpriteCandidate | undefined and template usages should guard with v-if="selectedCandidate".


Minor: Group path in handleChange missing guard — UICheckbox.vue around line 50

The standalone path checks if (nextChecked !== checked.value) before emitting. The group path calls checkboxGroupContext.updateValue(props.value, nextChecked) unconditionally. updateValue is idempotent so there is no functional bug, but the asymmetry is confusing. Apply the same guard to the group branch for consistency.


Minor: Inline style conflicts with scoped CSS size constraints — SpriteGeneratorModal.vue line 99

<UIModal style="width: 1076px; height: 800px" ...>

The inline width takes precedence over the CSS width rules, leaving only the max-width: calc(100vw - 48px) clamp as the effective responsive constraint. On viewports narrower than 1124px the modal overflows its scroll container. Move the base dimensions into the CSS class and remove the inline style.


Positive observations: Accessibility is well done throughout — role="dialog", aria-modal="true", focus-trap entry in UIModal, role="menu"/role="menuitemradio" on dropdown menus, and keyboard handlers on interactive div elements in PreviewPanel are all correct. UICheckboxGroup.updateValue is correctly immutable (spread/filter, emits only on actual change). onBeforeUnmount cleanup in UIModal correctly removes the keydown listener regardless of visibility state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant