[Demo] Game window design#3330
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a tutorial course mode to the SPX editor, adapting various UI components (such as the main editor layout, Copilot panel, preview controls, navbar, and code editor) to provide a streamlined, focused learning experience. It also adds comprehensive design engineering workflow documentation and Pencil design assets, while removing obsolete prototype files. The review feedback focuses on improving code robustness by recommending optional chaining and fallback values to prevent potential TypeErrors when accessing course properties or API reference overviews.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (course.entrypoint.includes('Coding-Course-3')) { | ||
| names.add('step') | ||
| names.add('turn') | ||
| } |
There was a problem hiding this comment.
Use optional chaining when accessing course.entrypoint to prevent a potential TypeError if entrypoint is undefined or null.
if (course.entrypoint?.includes('Coding-Course-3')) {
names.add('step')
names.add('turn')
}
| if (course.entrypoint.includes('Coding-Course-3')) { | ||
| return ['step distance', 'turn direction'] | ||
| } |
There was a problem hiding this comment.
Use optional chaining when accessing course.entrypoint to prevent a potential TypeError if entrypoint is undefined or null.
if (course.entrypoint?.includes('Coding-Course-3')) {
return ['step distance', 'turn direction']
}
| function getAPINameCandidates(item: DefinitionDocumentationItem) { | ||
| const definitionName = item.definition.name ?? '' | ||
| const methodName = definitionName.split('.').at(-1) ?? definitionName | ||
| return [definitionName, methodName, item.overview].map(normalizeAPIName).filter((name) => name !== '') |
There was a problem hiding this comment.
Provide a fallback empty string for item.overview to prevent normalizeAPIName from throwing a TypeError if overview is undefined or null.
return [definitionName, methodName, item.overview ?? ''].map(normalizeAPIName).filter((name) => name !== '')
| const names = normalizedAllowedNames.value | ||
| const overviews = normalizedAllowedOverviews.value | ||
| const filterText = normalizedFilterText.value | ||
| if (overviews.length > 0) return overviews.includes(normalizeAPIName(item.overview)) |
There was a problem hiding this comment.
Provide a fallback empty string for item.overview to prevent normalizeAPIName from throwing a TypeError if overview is undefined or null.
if (overviews.length > 0) return overviews.includes(normalizeAPIName(item.overview ?? ''))
| lastPanicOutput.value = null | ||
| if (props.tutorialMode) { | ||
| runnerState.value = 'initial' | ||
| editorCtx.state.runtime.setRunning({ mode: 'none' }) |
There was a problem hiding this comment.
Use optional chaining when accessing editorCtx.state.runtime to prevent a potential TypeError if runtime is undefined or null.
editorCtx.state.runtime?.setRunning({ mode: 'none' })
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Game window design (tutorial layout)
This is a demo PR ([Demo only DO NOT MERGE]). Almost all of the 129 changed files are design artifacts (ui/** .pen files, images, and Chinese docs) — those were not reviewed. Review focused on the 9 changed spx-gui/src source files that implement the tutorial "game window" layout.
No security findings. No high-severity performance regressions. The substantive findings are correctness/maintainability concerns in the API-reference filtering logic, which I've left inline.
Additional findings (no reliable single inline line)
- Duplicated decorative background CSS. The multi-stop
radial-gradient(...) #78c966block plus itsbackground-sizelist is copy-pasted verbatim betweenspx-gui/src/components/editor/ProjectEditor.vue(.tutorial-preview-pane) andspx-gui/src/components/editor/preview/EditorPreview.vue(.tutorial-preview .stage-viewer-container, .runner-host). Extract into a shared class / custom property so the tutorial look is defined once. - Duplicated
isTutorialCoursepredicate.const tutorial = useMaybeTutorial()+const isTutorialCourse = computed(() => tutorial?.currentCourse != null)is repeated across ~5 components (apps/xbuilder/pages/editor/index.vue,CopilotUI.vue,ProjectEditor.vue,EditorNavbar.vue,CodeEditorUI.vue). Consider auseIsTutorialCourse()composable intutorials/tutorial.tsso the definition lives in one place. EditorPreview.vue— resize watch writes on the non-tutorial path too.watch(windowSize, ...)inCopilotUI.vue:110now writestutorialPanelHeight.value(alocalStorageRef) on every window resize regardless of whether a tutorial course is active, causing needless localStorage writes. Consider gating the write behindisTutorialCourse.valueand/or only writing when the clamped value changed.
Note: the useMaybeTutorial() addition, named layout constants, and the final-commit simplification of the preview header back to a static UICardHeader are all clean.
| if (course == null) return null | ||
|
|
||
| const names = new Set<string>() | ||
| const rawCourse = course as Record<string, unknown> |
There was a problem hiding this comment.
This loop probes five speculative keys (apis, apiReferences, apiReferenceIds, allowedApis, allowedAPIReferenceIds) via a course as Record<string, unknown> cast, but the Course type (spx-gui/src/apis/course.ts) only has id, owner, title, thumbnail, entrypoint, references, prompt. None of these keys exist on Course or are returned by the backend, so every lookup is undefined and the loop is dead code — and the cast silently defeats type-checking that would otherwise flag it. Remove it, or if per-course allowed-API metadata is really needed, add it to the Course type and API layer explicitly (with a comment explaining intent).
| } | ||
| } | ||
|
|
||
| if (course.entrypoint.includes('Coding-Course-3')) { |
There was a problem hiding this comment.
Hardcoded course.entrypoint.includes('Coding-Course-3') matching (repeated at line 110) plus hardcoded api names (step, turn) and overviews (step distance, turn direction) couples generic layout code to one specific course by an entrypoint-URL substring. It's brittle (renaming the course silently breaks filtering), non-scalable (every new course needs a code change here), and undocumented. Prefer driving the allowed-API list from the course/backend data; at minimum add a comment marking this as temporary demo scaffolding with a tracking issue.
| if (overviews.length > 0) return overviews.includes(normalizeAPIName(item.overview)) | ||
| if (names.length === 0 && filterText === '') return true | ||
| return getAPINameCandidates(item).some((candidate) => { | ||
| return names.includes(candidate) || filterText.includes(candidate) |
There was a problem hiding this comment.
filterText.includes(candidate) does substring matching of a normalized API-name candidate against the concatenated normalized course text (id + title + entrypoint + prompt). Since normalizeAPIName strips all non-alphanumerics and lowercases, short candidates cause false positives — e.g. an api/method fragment like on, go, or set will match almost any prompt containing those letters (python, message, settings). That defeats the filter's purpose of restricting shown APIs. Use token/word matching or an explicit allow-list instead of free-text substring containment. Also consider making normalizedAllowedNames a Set for O(1) membership rather than Array.includes in the .some().
|
|
||
| const props = defineProps<{ | ||
| controller: APIReferenceController | ||
| variant?: 'sidebar' | 'strip' |
There was a problem hiding this comment.
The four new public props (variant, filterText, allowedNames, allowedOverviews) have no JSDoc, and their interaction in isAllowedInTutorial is non-obvious: if allowedOverviews is non-empty it short-circuits and wins outright, so the allowedNames values (step/turn) supplied alongside it from CodeEditorUI are effectively dead. Document the precedence between the three filter inputs (or simplify to one clear strategy), since this is a shared component being extended.
| return Math.min(getMaxTutorialPanelHeight(), Math.max(tutorialPanelMinHeight, height)) | ||
| } | ||
|
|
||
| // resize the panel to fit the window size |
There was a problem hiding this comment.
This comment (// resize the panel to fit the window size) now under-describes the block: the watch body was extended to also re-clamp tutorialPanelHeight on resize, not just call updatePanelClampedPosition(). Update the comment to reflect both behaviors.
qingqing-ux
added
the
design-pr
qingqing-ux
changed the title
There was a problem hiding this comment.
FennoAI Review — PR #3330 (tutorial / game-window design)
Reviewed the diff against ui across four dimensions (quality, performance, security, docs). The change is well-scoped — tutorial behavior is gated behind isTutorialCourse / variant === 'strip' so the default paths stay untouched, listeners are cleaned up, and the new tests are accurate. A few findings worth addressing, the first two being the most impactful. Inline comments cover the concrete diff-line issues; the items below have no single reliable line.
Maintainability / data modeling
- The tutorial API allow-list is driven by (a) reading fields that don't exist on the
Coursetype and (b) a hardcodedCoding-Course-3entrypoint substring. Both are inline-commented. The durable fix is to model the allowed API set as real, typedCoursedata (e.g.apiReferences) populated by the backend, and remove both theas Record<string, unknown>cast and the magic string. As-is, adding another tutorial requires a frontend code change + redeploy.
Security (defense-in-depth, sink outside this diff)
spx-gui/src/apps/xbuilder/pages/sign-in/callback.vuedoeswindow.location.replace(returnTo)wherereturnTois typedstringwith no same-origin validation. Current callers pass same-origin relative paths and the value comes fromsessionStorage(not the callback query string), so this is not currently exploitable — but since this PR expands the OAuth redirect surface, consider normalizing/validatingreturnToto a same-origin relative path at the sink. The new configurableredirectUriitself is build-time only and server-enforced, so it does not add open-redirect risk.
Duplication (minor, opportunistic)
- The
radial-gradient(...) #78c966"game window" backdrop block is duplicated verbatim inEditorPreview.vueandProjectEditor.vue; the Copilot<svg>icon is duplicated for the tutorial vs. normal trigger inCopilotUI.vue. Consider extracting a shared class/component so the two copies don't drift. - Three "exit tutorial" entry points (
TutorialNavbarExit.vue,TutorialStateIndicator.vue,TutorialCourseExitLink.ts) each wrapexitCurrentTutorialwith divergent error copy ("exit tutorial" vs "exit course"); only one adds the course/series guard. A shareduseExitTutorial()composable would keep them consistent.
Not blocking — submitted as COMMENT.
Findings without inline locations
spx-gui/src/apps/xbuilder/.env:12: Minor doc accuracy: "the app uses the current web origin" understates the fallback. When empty,initUserStatefalls back to${window.location.origin}/sign-in/callback(origin plus the fixed callback path), consistent with the full URL configured in.env.staging. Suggest wording like "...uses the current web origin with the/sign-in/callbackpath."
|
|
||
| const names = new Set<string>() | ||
| const rawCourse = course as Record<string, unknown> | ||
| for (const key of ['apis', 'apiReferences', 'apiReferenceIds', 'allowedApis', 'allowedAPIReferenceIds']) { |
There was a problem hiding this comment.
This loop reads keys apis/apiReferences/apiReferenceIds/allowedApis/allowedAPIReferenceIds off the course via course as Record<string, unknown>, but the Course type (spx-gui/src/apis/course.ts) only defines id, owner, title, thumbnail, entrypoint, references, prompt. None of these keys exist on the model and the backend never returns them, so this block is permanently dead — it always yields an empty set. The as Record<string, unknown> cast is what hides this from the type checker. Either extend Course with a real, typed field (populated by the backend) or drop this block; as written it silently does nothing and misleads future readers into thinking course-driven API allow-listing works.
| } | ||
| } | ||
|
|
||
| if (course.entrypoint.includes('Coding-Course-3')) { |
There was a problem hiding this comment.
Hardcoding behavior to entrypoint.includes('Coding-Course-3') is fragile: it's a substring match (so Coding-Course-30, query strings, etc. also match) and the allowed API names/overviews live in a frontend if rather than in course data — adding another course needs a code change + redeploy. This exact check is also duplicated at line ~110. Prefer sourcing the allow-list from the (typed) course model; at minimum extract the string to a named constant.
| const names = normalizedAllowedNames.value | ||
| const overviews = normalizedAllowedOverviews.value | ||
| const filterText = normalizedFilterText.value | ||
| if (overviews.length > 0) return overviews.includes(normalizeAPIName(item.overview)) |
There was a problem hiding this comment.
Two concerns here: (1) When allowedOverviews is non-empty this returns early, so allowedNames is entirely ignored. CodeEditorUI passes both for Coding-Course-3, meaning the step/turn names path is unreachable — the intended precedence (AND/OR) should be made explicit. (2) overviews.includes(normalizeAPIName(item.overview)) requires the full overview text, stripped to [a-z0-9], to exactly equal stepdistance/turndirection. Any wording change or localization of the overview silently drops the item, leaving an empty toolbox with no error. Match on a stable identifier (definition id/name) rather than display prose.
|
|
||
| export async function exitCurrentTutorial(tutorial: Tutorial, router: Router) { | ||
| const navigationFailure = await router.push('/tutorials') | ||
| if (navigationFailure) return false |
There was a problem hiding this comment.
router.push resolves to a truthy NavigationFailure for benign cases too (e.g. duplicated/redirected), not just user-cancelled navigation. Treating any truthy value as failure and skipping endCurrentCourse() means an in-app redirect away from /tutorials would leave currentCourse/currentSeries set — keeping the whole app in isTutorialCourse mode on the wrong page. Prefer isNavigationFailure(result, NavigationFailureType.aborted | .cancelled) to detect only real aborts. Separately, the /tutorials route string should come from the centralized router module (as elsewhere in apps/xbuilder/router.ts) rather than being hardcoded here.
| function handleMouseMove(target: monaco.editor.IMouseTarget) { | ||
| const ui = props.controller.ui | ||
| if (target.type !== ui.monaco.editor.MouseTargetType.CONTENT_TEXT || target.position == null) return | ||
| hoveredDiagnostic.value = getDiagnosticAtPosition(target.position) |
There was a problem hiding this comment.
handleMouseMove fires on every raw mousemove over the editor and runs a linear .find() scan over all diagnostics (with a containsPosition check each). It's unthrottled, unlike the sibling drag handler in CodeEditorUI.vue which uses throttle(..., 50). It's saved from being severe by hoveredDiagnostic being a shallowRef (same-reference assignment is a no-op, so downstream re-renders only run on actual target change), but the per-event scan itself still runs on every pixel. Consider throttling to match the drag handler and/or early-returning when target.position is unchanged.
1 participant
No description provided.