Conversation
fb9f560 to
c647c36
Compare
c647c36 to
770a6a4
Compare
fb3c719 to
1121bd9
Compare
The kind filter in "Manage courses" and "Manage course series" was a pair of filter chips, which was the wrong control twice over. Chips read as independent conditions, so they sit apart from each other and nothing says the two belong together; and the modal header lays its slot content out side by side without spacing, so the chips touched the create button. Guided and Playground are two views of one list, exactly one of which is shown. That is what `UITabRadioGroup` is for, and the code editor's input helper already uses it for the same kind of choice: one trough, the selected option raised, both options the same width. A margin separates it from the create button. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… and retry safely A review round found two problems in how a new Playground Course joins its series; the five new tests describe both. The series was read when the create form opened and written back whole when it was submitted, minutes later once the starter package had uploaded. A series changed in the meantime, from another tab or device, lost whatever had been added to it: the late write put back the old course list plus the new course, and a course dropped that way can no longer be opened from the list. It also wrote back the title, thumbnail, description and order it had read, overwriting edits it had nothing to do with. `appendCourseToSeries` reads the series right before writing it and sends its course list alone, which the endpoint has always accepted, so `UpdateCourseSeriesParams` says so now. What is left is the gap between that read and that write; closing it takes a server-side append the Course APIs do not have. Creating a course and adding it to a series are two requests. When the second failed, the author was told that creating the course failed, which it had not, and clicking "Create" again uploaded the starter package and created a second course, leaving the first in no series. `PlaygroundCourseCreation` remembers the course once it exists, so a retry only repeats what is left, and the message says the course exists and that retrying will not create another. A retry after a response that never arrived finds the course already in the series and writes nothing. Checked against the local backend: a patch carrying only `courseIDs` is accepted and leaves the other fields as they were, and a second run returns the same course. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…series Preview showed one course and stopped there. An author writing a course in the middle of a series could not see what a learner sees next, which is where the end of a course is judged: whether the course that follows picks it up. Finishing a previewed course now offers "Learn next course", the button a learner already gets, and it does what it says: the preview loads the next course of the series and runs it, on to the end of the series, where the modal offers only the way out. The walk starts at the course being edited, from the author's unsaved work; the courses after it are shown as they were saved, since only the one being edited has a working copy. The banner names the course on screen, which it never had to while a preview was always the course being edited. Loading a course of the series can fail -- it was deleted, or the series the editor loaded has changed since -- so the preview pane shows the failure with a retry that retries that course rather than starting the walk over. Leaving and re-entering the preview starts again at the course being edited. Walked a three-course series locally: each course ran its own program, the banner followed, the last one offered no next course, and leaving returned to the editor. Deleting a course mid-walk showed its error in the pane, and retry re-requested that course. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Folding a group of the course explorer lasted until the page was reloaded, which happens often here: the editor is reloaded on its own URL, and an author working through a series opens one course after another. What can be folded is a resource group or the heading of unused records, and those rows are the same in every course, so what is folded is remembered for the tab rather than for one course: the next course opens with the same groups out of the way, and a reload finds the tree as it was left. Rows start open, so the usual case remembers nothing. The rows no longer each own whether they are open, since a row is rebuilt whenever the tree changes and has nowhere to remember it. The explorer holds the folded keys and a row asks. Checked in the browser: folding "Videos" survived a reload and carried into another course, and unfolding it brought its video back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Course Editor and the host of the embedded Project Editor had no tests of their own. What they do is route- and lifecycle-shaped, and every regression in them so far was found by hand. The editor's tests drive the preview, where the series walk lives: the course being edited runs from a snapshot rather than from the working copy, "Learn next course" loads the next course of the series and runs it, the end of the series leaves the preview, "keep looking" stays, and a course that cannot be loaded is reported in the pane instead of ending the preview. The editing pane is stood in for, down to the document that carries Monaco. The host's tests are about the route: it stays off the route while the course is open on another document, opens the configured path the first time the project is shown, prefers a path already in the URL, goes on showing the project its own route once another document is opened, drops what the project asks for meanwhile, and comes back to where the project was left. The editor state is a fake that records the view of the route it was handed, which is the host's whole contract with it. Writing those turned up a bug they now guard. `initialize` clears the remembered project route, and the watcher that records it only sees route *changes*, so opening the course straight on a project URL left nothing remembered: the project was then handed a foreign route the moment the author opened another document, deselecting what they had selected, and reopening it from the explorer came back to the default selection rather than to where they were. `startRouteSync` now records the route it just opened. Checked in the browser as well as in the tests. Also corrects `getCourseEditorRoute`'s docstring, which still said that nobody called it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
75d320c to
8976a9a
Compare
`TutorialProject.load` builds an instance, and with it an `SpxProject` that carries watchers, before reading the course's records. When that read fails -- content without an `index.json`, or a course saved in a shape this version cannot read -- the instance was dropped on the floor: the call threw before returning it, so no caller ever had a reference to release. Course loading and the playground both go through this, and the editor retries a failed load, so the leak repeated with every retry. It now releases the SPX project before rethrowing, the way `CourseEditor.vue#loadPreviewSnapshot` already did for the snapshot it builds. The test fails without the release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…on screen A preview is left and entered again freely -- "Back to editor", the browser's history -- and what a preview started can still be in flight when it is. Two things from a preview the author had left could reach the one on screen; a review round found both, and tests that fail before this change reproduce them. A failed load. Every load checked whether it was still current, but only once it had succeeded. A course of the series, or a snapshot, that failed after the author had gone back to the editor and in again was shown as the preview's error: it replaced a preview running fine and released its snapshot. The completion modal. It outlives the preview that opened it -- the Back button does not close it -- and its answer only checked that some preview of this course was on screen. Answered after the author stepped out and back in, "Learn next course" walked the new preview on to the next course, and "Back to course series" ended it. Both now check the preview they belong to. `isCurrentPreview` takes the generation the work started with; the loaders already asked it on success, and now it is asked on failure and when the modal is answered too. Checked in the browser as well: a completion modal left open across Back and Forward, then answered, has no effect on the preview entered since. The modal itself still stays up over the editor after Back; that is left as it is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reopening the project from the explorer came back to its first sprite, not to where the author had left it: from the stage, or from map mode, the selection was lost every time. A review round pointed at the mechanism, and the browser reproduced it three times out of three. The explorer's project node addresses the project's bare root, which the host replaces with the remembered route. But the host also showed the bare root to the editor state, as a route with an empty path, and the state reacts to that by selecting its default and navigating there, overtaking the replacement. The host no longer shows the state the bare root: it is transient, so the state keeps seeing the remembered route until the replacement lands. Clicking the project node while the project is already open lands on the bare root the same way, and used to reset the selection too; the replacement now runs for it as well, from the route watcher, with the function the reopening path uses and one replacement in flight at a time. The earlier host tests could not have caught this, since their stand-in state never acts on what it is shown. New tests mount the host with a real editor state, the way the Course Editor mounts it -- open exactly while the route is inside the project -- so they only move the route, as the explorer does. Each half of the change is needed by one of them. The bug predates the fix that made the remembered route survive a reload: the stage here is reached from inside the editor, which was always remembered. Also corrects two "Called by" lists that the earlier fix left stale. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For the Course Editor's activity bar, which names its five views by icon: the course, the learner's project, videos, pictures and the course program. The icons are named for what they show rather than for where they are used, since any part of the app may want a video or a picture icon. They are drawn in the stroke style some of the set already uses (24 by 24, two-pixel `currentColor` strokes, round caps), and are meant to stand in until the designers provide their own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lorer tree A course has five parts an author works on -- the course itself (its settings), the learner's project, the videos, the pictures and the course program -- and the explorer spent a 260-pixel column on a tree of them. Its depth, folding and one row per resource told the author little the five parts do not. Worked out with the designers, the left edge is now a narrow activity bar like VS Code's, one icon per part; what it opens has the rest of the width, which matters most for the embedded Project Editor. - The views are addressed by the path of what they edit, as the tree nodes were: `''`, the project root, `assets/videos`, `assets/images`, `main_course.gox` (`course-views.ts`). An address the editor produced before, such as a single video's, is shown by the view that takes it and the URL is brought in line; anything else shows the course. - Videos and pictures each get a page laid out as a grid of cards, drawn like the course management lists. A card previews its resource when clicked; its corner menu renames it (in the shared rename dialog, which for a video warns that the program refers to it by name) or deletes it after a confirmation. The page says what is being added, so the upload modal and its choice of type are gone: "Add videos..." and "Add pictures..." open the file picker for the formats that page takes. - A card shows a video's first frame from where the course stored it (`getStoredWebUrl`), so the browser fetches a few ranges instead of the whole video, which `File.url()` would download first. The app is cross-origin isolated, so such media has to be requested with CORS (`crossorigin="anonymous"`); without it the browser reports a format error. Pictures are plain images, not the pixel-art `UIImg`. - Records the course format gives no role to, and resource kinds other than videos and pictures, are no longer shown, and the editor no longer creates them: nothing can be uploaded as "something else". A course that carries them keeps them; saving writes them back unchanged. - Unsaved changes show as a dot on the view they belong to. This removes the explorer, its folding (only just made to survive a reload), the upload modal, the plain-file document, the resource-group and single-resource documents, and the tree projection; `upload.ts` keeps only what adding resources needs. Comments that named any of them now name what replaced them, including in the tutorial models. Checked in the browser on a course with a video: every view opens at its address; the video card shows its first frame; preview plays it; renaming and deleting from the menu work and mark the view unsaved; adding a picture through the page's button (with the file picker stood in for) adds a card; the old address of a single video lands on the videos page; the activity bar hides while previewing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The skill the author's Copilot starts with described the explorer: a tree on the left, an "Add..." button that asks what is being added, and unused files shown under their own heading. The editor now has an activity bar with five views and adds resources from the page of their kind, and it neither shows nor creates records the course format gives no role to. Left as it was, the skill would have walked authors through controls that are gone. The format reference keeps what still holds -- such records are saved with the course unchanged -- and drops the claim that the editor shows them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The activity bar's right edge came out nearly black. Its class named a `border-line` colour the theme does not have, so the border fell back to the current text colour: this app does not use Tailwind's preflight, which would otherwise give borders a default colour. It now uses `dividing-line-2`, the colour `UIDivider`, menu groups and the course management footers draw their dividers in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The preview of a video said how the course program plays it, as plain text. What the author does with that line is put it into the program, so it is now shown as code, highlighted the way the program editor highlights it, with a copy button beside it (the shared `CopyButton`, which confirms with a toast). The name is written as a string literal, so a name holding a quote or a backslash still copies as code that compiles. Checked in the browser: the preview shows `showVideo "step"` highlighted, and the button puts exactly that on the clipboard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cture grids Each page opened with a line on what the course program does with what is on it: an example `showVideo` call for the videos, and a note that no call uses pictures yet. The pages are plain grids now, the header followed by the cards; how to play a particular video is shown, ready to copy, in that video's preview. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
At the designers' suggestion the activity bar is now a card like the view beside it: the same `UICard` shell, so its corners and shadow follow the cards around it, square on the left where it meets the window's edge and rounded on the right. It sits in the main area's row, which has no padding on the left, so it stretches to exactly the height of whatever is open -- the settings card, a resource grid, or the Project Editor -- and keeps the row's usual gap from it. The divider on its right edge is gone with the column it used to separate. Checked in the browser on the course, videos and project views: the bar starts at the window's left edge, its top and bottom line up with the view beside it, and the gap between them is the row's 16 pixels. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Review: course editor (PR #3494)
A large, well-architected feature. The reactive model, session lifecycle, async supersession guards (previewGeneration counter), per-view dirty tracking, and the DerivedFile memoization for stable File identity are all sound and thoroughly tested. Security posture is good: resource names/kinds go through validatePathSegment (rejects ., .., /, NUL), extraFiles uses a Map to avoid prototype pollution, uploads enforce size limits, and no user content is rendered via v-html (the only v-html renders build-time static SVGs).
A few findings below, mostly minor. See inline comments.
Additional non-inline notes:
spx-gui/src/components/course/management/playground/series.ts—listPlaygroundSeriesfetches only the first page (seriesPageSize = 100) and silently returnsdata. If an author ever exceeds 100 series, courses in later series become unfindable viafindSeriesOfCourse/ unattachable via the edit modal, with no signal. Consider paginating to exhaustion or surfacing a truncation warning whendata.length >= seriesPageSize.spx-gui/src/components/editor/editor-state.ts—selectByRoutewas widened fromprivatetopublicsoSpxProjectEditorHostcan open the initial path. This widensEditorState's contract; any future caller can now invoke it outside thesyncWithRouterlifecycle. A narrower dedicated method (e.g.openInitialPath) delegating internally would keep the invariant enforced. (Minor / design preference.)
The course skill called the round callback `Copilot.onRoundFinish`, a method the framework does not have: it is `onRoundComplete`. The Course Editor preloads this skill for Copilot, so Copilot would have written course programs that fail to compile. SKILL.md's overview now uses the same words for the event. Every edit exported the whole course twice, once for the unsaved flag and once for the per-view marks, as `TutorialProject.exportFiles()` builds a new map on every call. The two now read one computed export, so an edit builds one; a test counts the exports and checks that both marks still follow the edit. The editor tests now unmount each editor they mount, so one test no longer hears another's `window` listeners. Two comments are set right as well: `selectByRoute` no longer says it was made public "on this branch", and `isClaimedPath`'s comment sits on `isClaimedPath` again instead of on the helper added between them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| export type AddCourseParams = | ||
| | Pick<GuidedCourse, 'kind' | 'title' | 'thumbnail' | 'content'> | ||
| | Pick<PlaygroundCourse, 'kind' | 'title' | 'thumbnail' | 'content'> | ||
| /** |
There was a problem hiding this comment.
这段注释没啥营养,这样的内容这个 PR 里好像还挺多的,可以整体过下,把这种没营养的注释删掉或者精简下
尤其是 comsumer / caller 的信息(我看下边还有很多),本身的维护会带来成本;另外一般来说应该是消费方去关心被消费方,而不是被消费方关心消费方,所以维护这份信息本身就不太合理。
另外还有一些组件的 props、emits 说明文档等,这种也建议贴着 props 本身的定义代码(这样对工具链分析和文档内容维护更友好),类似:
defineProps<{
/** a is for ... */
a: string
/** b is for ... */
b: string
}>()我晚点再往项目 agents.md 里补一下关于代码中文档维护的偏好说明吧
There was a problem hiding this comment.
看完再来补一嘴,是不你跟你的 CC 说了啥啊,它补注释的热情高得有点不太正常...
注释太多了本身也会影响阅读和理解代码的效率的,当然也会提高维护的成本,一般来说我们只对那些代码本身不能(或较难)体现的事情才进行注释说明,或者对某个模块的对外接口简述定位和使用的注意事项。
There was a problem hiding this comment.
我晚点再往项目 agents.md 里补一下关于代码中文档维护的偏好说明吧
#3537 @Ethanlita 帮忙 review 下?
| { | ||
| // `:inEditorPath*` is the Project Editor's in-editor path (same param name as `/editor/...` routes), so the | ||
| // learner-side `EditorState.syncWithRouter` and `CoursePlayground.vue` work unchanged inside the preview. | ||
| path: '/course-editor/:courseSeriesIdInput/:courseIdInput/preview/:inEditorPath*', |
There was a problem hiding this comment.
既然 /preview/* 跟 /edit/* 对应的都是 pages/course-editor/index.vue,那 /preview 和 /edit 是不是也可以作为 inEditorPath 的一部分由 course editor 自己去处理?
这样也没必要再定义 courseEditorRouteName 和 courseEditorPreviewRouteName 了
| const handleOpenInCourseEditor = useMessageHandle( | ||
| async (course: PlaygroundCourse) => { | ||
| const courseSeries = await m.withLoading( | ||
| findSeriesOfCourse(course.id), |
There was a problem hiding this comment.
Nit: 这个界面会列出用户所有的 course,但没有 course 对应的 series 信息(也没有把 course 按 series 进行组织)。
这样其实也挺不友好的,毕竟不同 series 下可能会存在标题相近的 course(比如两个 course 都叫“1. 开始挑战”之类,但属于不同的 series);所以我想可能处理成课程按课程系列组织会更好,在确定 course 信息的时候直接就能确定对应的 series,也就不需要类似这里的 findSeriesOfCourse 反查逻辑了。当然那样课程系列的管理和课程的管理可能也就自然地合到一个界面/操作流程下了。
当然这个问题不是这个 PR 引入的,我们可以后续单独 PR 再处理这个事情。
There was a problem hiding this comment.
我看现在 course editor 是自己实现了一套保存机制(还有专门的逻辑去可视化每个“view”的 dirty 状态)。
其实可以考虑复用 spx project editor 的自动保存机制的,那套机制已经被抽取到 src/components/editor/editing.ts、跟 spx project 无关了。这个可以作为后续优化项
| export const courseViews: CourseView[] = ['course', 'project', 'videos', 'images', 'program'] | ||
|
|
||
| /** The resource kind each resource view shows. */ | ||
| export const viewResourceKinds = { videos: videosKind, images: imagesKind } as const |
There was a problem hiding this comment.
如果我们确实要支持 image 的话,需要在 course 的规格里加一下 image 的定义
或者先干掉(反正之后再加应该也不麻烦)。我们上次好像讨论到过,现在应该还没有 API 能够消费 image resource?
| * Course Editor can show the same controls in its own navbar while the embedded learner project is open. | ||
| * | ||
| * Props: | ||
| * - `state` - The `EditorState` whose `history` is undone/redone; null while no project editor is active |
There was a problem hiding this comment.
用于 course editor 中的时候,这个 history button 操作的是 course project 的 history 还是内嵌的 spx project 的?
| } | ||
|
|
||
| private selectByRoute(path: PathSegments) { | ||
| /** |
There was a problem hiding this comment.
这个注释内容长得有点过分了..看实现代码都未必比看这个注释文档吃力
|
|
||
| /** | ||
| * The web URL a file is already stored at, or null for a file that exists only locally (not uploaded yet). Unlike | ||
| * `saveFileForWebUrl` it never uploads anything, so it suits showing a file without saving it; and handing the URL |
There was a problem hiding this comment.
如果是展示一个文件(界面上消费文件内容)的目的,应该用 useFileUrl(builder/spx-gui/src/utils/file.ts) 或 useRenderableImageUrl(builder/spx-gui/src/utils/img-rendering.ts)
| code: string | ||
|
|
||
| /** Memo generating the `main_course.gox` record from `code`; see `DerivedFile` for why identity matters. */ | ||
| private codeFile = new DerivedFile((code) => fromText(mainCourseFilePath, code)) |
There was a problem hiding this comment.
这个 DerivedFile 搞得有点复杂了,如果是希望 code 没变 file 就不变的话,应该用一个 vue computed 就好了:
class Course {
private codeFile = computed(() => fromText(mainCourseFilePath, this.code))
export(): Files {
return { [mainCourseFilePath]: this.codeFile.value }
}
}当然如果我们像上面提到的走自动保存,而不在界面上去分 view 展示 unsaved 状态的话,这个 computed 也没有必要了
There was a problem hiding this comment.
我让 AI 总结了下 tutorial model 这边的改动:
- 资源从视频扩展为通用资源包:旧版的 videos: Video[] 改成 resources: Resource[],统一管理视频、图片及其他资源类型。
这个改动的考虑是?
感觉这个长远来看没有好处啊,不同的 resource 配置信息、子目录、使用姿势等都很可能不同,可以参考 spx project 中的 backdrop(类似图片)和 sound(类似音频)
现在这样合成一个 Resource,但还是要分 kind 去处理它们不同的逻辑;另外冲突检查也比较麻烦,按理说可以支持 video 跟 image 同名的,统一为 resource 的概念后,名字的冲突检查还是得按 kind 分开?路径的检查也要单独做(本来只要确保同一类资源不同名就能确保最终路径不会冲突了)。
- 保留模型暂不理解的文件:新增 extraFiles,加载时把未被配置、项目、程序或资源包认领的文件保留下来,导出时原样写回。
我们现在应该不提供按文件操作的界面了吧?是的话这个能力似乎也可以干掉了
- 集中管理文件归属和冲突:加入资源命名、ID 去重、保留目录和文件路径冲突检查,避免不同部分导出到同一路径。
如前面提到过,如果不同类型的资源各自做好命名冲突的检查,那么文件路径是不会冲突的;所以应该不需要“集中管理”才对
- 支持编辑器的保存与预览:新增 setConfig、exportFiles 和 snapshot 等方法。生成的配置文件会复用 File 实例,方便编辑器判断哪些内容未保存;snapshot 会等待项目操作完成再导出。
snapshot vs exportFiles 我看跟 spx project 的 export vs exportFiles 关系类似,要不要处理为跟那边类似(export 会 respect mutex 而 exportFiles 不会);另外这边有个细节的问题是,mutex 应该由 course project 自己构造一个来用,而不应该复用内嵌的 spx project 的 mutex。
Course Editor for Playground Courses (#3420): the authoring surface that edits a Course's Tutorial project (embedded SPX project, course program, videos, settings, other files) and saves it through Course APIs. Course metadata (title, thumbnail) stays with course management.
Architecture
Per
docs/develop/tutorial-v2/index.md, the Course Editor composes the existing SPX Project Editor rather than modifying it: the course's embedded project is loaded as an ownerless in-memorySpxProject, so the existing ownership rule puts the editor in effect-free mode (edits stay in memory), and the Course Editor's own Save exports theTutorialProjectfile collection and persists it via Course APIs. Preview snapshots the working copy and runs the same Tutorial lifecycle as learning.