Skip to content

sync dev to ui#2996

Merged
qingqing-ux merged 3 commits intouifrom
dev
Mar 29, 2026
Merged

sync dev to ui#2996
qingqing-ux merged 3 commits intouifrom
dev

Conversation

@qingqing-ux
Copy link
Copy Markdown
Collaborator

No description provided.

cn0809 and others added 3 commits March 25, 2026 16:42
* add separate ai generation entry

* show asset library suggestions in AssetGenModal

* details

* more details

* refactor: split AssetGenModal into dedicated SpriteGenModal and BackdropGenModal

* fix: add ongoing asset generation repeatedly when collapse

* revert removed TODO comment

* optimize tip

* fix: backdrop gen modal close flow
Agent-Logs-Url: https://github.com/goplus/builder/sessions/382560f4-afcd-4e75-b90a-009f81603c3e

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: nighca <1492263+nighca@users.noreply.github.com>
* Fix case-sensitivity issues around backend unique identifiers

* Fix route param names & username update issue
Copy link
Copy Markdown

@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 a comprehensive refactoring of identifier resolution and user authentication state management, establishing a clear distinction between unresolved and canonical identifiers. It also enhances the AI asset generation workflow by integrating library suggestions and refactors components to use a more robust signed-in state query. Feedback was provided to improve the username update process by streamlining the redirection to the sign-in page to avoid a flickering signed-out state.

Comment on lines +35 to +57
const handleUsernameUpdated = useMessageHandle(
async (newUsername: string) => {
await router.replace({
params: {
nameInput: newUsername
},
query: route.query,
hash: route.hash
})
message.success(
i18n.t({
en: 'Username updated successfully. Redirecting to the sign-in page...',
zh: '用户名更新成功。正在重定向到登录页面...'
})
)
await timeout(2000)
initiateSignIn()
},
{
en: 'Failed to redirect after username update',
zh: '用户名更新后重定向失败'
}
).fn
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation of handleUsernameUpdated first replaces the route and then, after a delay, initiates the sign-in process. This can create a confusing user experience, as the new user page might briefly render in a signed-out state before the redirect to the login page occurs.

A more seamless approach would be to avoid the intermediate router.replace call. Instead, you can construct the full URL for the new user page and pass it as the redirectUri to initiateSignIn. This ensures the user is redirected to the correct page after successfully signing in, without showing an intermediate page state.

const handleUsernameUpdated = useMessageHandle(
  async (newUsername: string) => {
    message.success(
      i18n.t({
        en: 'Username updated successfully. Redirecting to the sign-in page...',
        zh: '用户名更新成功。正在重定向到登录页面...'
      })
    )
    await timeout(2000)
    const newRoute = router.resolve({
      name: route.name!,
      params: { nameInput: newUsername },
      query: route.query,
      hash: route.hash
    })
    const redirectUri = new URL(newRoute.href, window.location.origin).href
    initiateSignIn(redirectUri)
  },
  {
    en: 'Failed to redirect after username update',
    zh: '用户名更新后重定向失败'
  }
).fn

}

return { keyword: _keyword, suggestions, isLoading, selected, toggle }
}
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.

The composable doesn't clean up the in-flight AbortController or cancel the pending debounced call on component unmount. If the component is destroyed while a search request is in-flight, the resolved callback will still mutate suggestions / isLoading on an unmounted component (and hold a reference in the module-level closure).

Consider adding an onUnmounted (or getCurrentScope-based cleanup) to abort and cancel:

import { onScopeDispose } from 'vue'

// at the end of useAssetSuggestions:
onScopeDispose(() => {
  doSearch.cancel()
  abortCtrl?.abort()
})

if (!signedInState.isSignedIn) return
if (usedCopilotUsersRef.value.includes(signedInState.user.username)) return
copilot.open()
})
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.

onMounted is now async and awaits untilLoaded. If the component unmounts before that promise settles, copilot.open() will still be called. There's no way to cancel untilLoaded here because no abort signal is passed.

Consider guarding with a mounted flag or passing an abort signal tied to onBeforeUnmount:

onMounted(async () => {
  const ac = new AbortController()
  onBeforeUnmount(() => ac.abort())
  try {
    const signedInState = await untilLoaded(signedInStateQuery, ac.signal)
    if (!signedInState.isSignedIn) return
    if (usedCopilotUsersRef.value.includes(signedInState.user.username)) return
    copilot.open()
  } catch {
    // aborted on unmount — ignore
  }
})

const signedInState = await untilLoaded(this.signedInStateQuery, signal)
const ownedBySignedInUser = signedInState.isSignedIn && signedInState.user.username === projectData.owner
return !ownedBySignedInUser
}
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.

The preferPublished closure defers the ownership check until after the cloud request starts resolving. If the user's sign-in state changes between loadProject being called and the preferPublished callback being invoked (e.g., token expires mid-load, or the user signs in during the async window), the check will use the new state rather than the state at the start of loading. This could flip the preferPublishedContent decision unexpectedly.

Capturing the state once at the top of loadProject and using it synchronously inside the closure would be safer:

async loadProject(ownerInput, projectNameInput, helpers, reporter, signal) {
  // Resolve auth state once, upfront, before starting the cloud load
  const signedInState = await untilLoaded(this.signedInStateQuery, signal)
  signal.throwIfAborted()

  const preferPublished = (projectData: ProjectData) => {
    const ownedBySignedInUser =
      signedInState.isSignedIn && signedInState.user.username === projectData.owner
    return !ownedBySignedInUser
  }
  ...
}

This also simplifies preferPublished back to a synchronous function, matching the original intent.

const saving = new Saving(this.project, this.cloudHelpers, this.localCacheHelper, this.isOnline, signal)
const signal = getCleanupSignal(onCleanup)
const saving = new Saving(this.project, this.cloudHelpers, this.localCacheHelper, this.isOnline, signal)

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.

The watcher now tracks both exportFiles() and this.mode, which means the auto-save logic re-runs whenever the signed-in state changes (since mode reads from signedInStateQuery). This is intentional for reactivity — when the user logs in and becomes owner, auto-save should activate. However, if mode changes without any dirty files, the guard if (mode === EditingMode.EffectFree || !this.dirty) correctly prevents a spurious save, so the behavior is safe. The test added for this exact case (should not trigger auto-save when mode changes without dirty files) confirms it. Just flagging in case reviewers want to leave a short comment here explaining why this.mode is a watcher source rather than a one-time check.

@xgopilot
Copy link
Copy Markdown
Contributor

xgopilot bot commented Mar 29, 2026

Solid set of changes. The shift from getSignedInUsername() to reactive useSignedInStateQuery() / useSignedInUser() is well-executed, and the new identifier-resolution guidelines in AGENTS.md are a good complement to the code. The new useAssetGen / useAssetSuggestions composables are clean and well-tested. Three items worth addressing: (1) useAssetSuggestions needs scope-disposal cleanup; (2) the onMounted async handler in CopilotUI should be guarded against post-unmount execution; (3) the preferPublished closure in editing.ts should capture auth state once at load-start rather than re-resolving mid-request. See inline comments for details.

@qingqing-ux qingqing-ux merged commit c71161a into ui Mar 29, 2026
10 of 11 checks passed
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.

4 participants