[WIP] New format of manual tests#1411
Conversation
…`manualStaticAssetsPlugin()` Vite plugin.
…ntries with the `<ck-manual-header>` component replacing the shell-based page composition, a new `rawSvgPlugin()`, removal of `rawHtmlPlugin()`, and Vite 8.1 support in `refreshPlugin()`.
…sed `packageRootPath` field and the `vite-svg-loader` dependency.
…ndencies. It existed only for the one-off migration of manual test instruction files.
… to `tests/manual/` instead of extracting `<title>`, and accept package root globs in the `paths` option.
…y and the test-only `getManualStaticAssetFilePath()` export, share the `include` package filter between plugins, and use the filtered `load` hook in the SVG plugin.
…uarantees every match contains the `tests/manual` segment.
…ev. Vite never regenerates HTML entries in the in-memory bundle, so the plugin splices the freshly built `<head>` with the post-`</head>` markup read from disk.
|
|
||
| configResolved( config ) { | ||
| workspaceRoot = config.root; | ||
| }, |
There was a problem hiding this comment.
Static assets cache not invalidated
Medium Severity
After splitting static assets out of manualTestsPlugin(), configResolved updates workspaceRoot but never clears the cached asset map. The old combined plugin invalidated both page and static caches on every configResolved, so a repeated resolve or any earlier cached collect can keep serving stale paths or omit newly added fixtures.
Reviewed by Cursor Bugbot for commit 3c10ffa. Configure here.
There was a problem hiding this comment.
Not a real issue. No change made.
The cache can only go stale if something reads it before configResolved sets the correct workspaceRoot. But this plugin has no config() hook — its cache is only read in configureServer (request time) and generateBundle (build time), both of which run after configResolved. So it's always computed with the right root. The sibling manualTestsPlugin needs invalidate() only because its config() hook reads the pages cache before configResolved; that early read doesn't exist here.
| } | ||
|
|
||
| const normalizedIncludePackageNames = new Set( includePackageNames.map( normalizePackageName ) ); | ||
| return builtHtml.slice( 0, builtHeadEnd + HEAD_CLOSE_TAG.length ) + sourceHtml.slice( sourceHeadEnd + HEAD_CLOSE_TAG.length ); |
There was a problem hiding this comment.
Header chrome stale after body edit
Medium Severity
Under experimental.bundledDev, keepManualHtmlSourceFresh only replaces markup after </head> from disk while keeping the initial built <head>. Opt-in <ck-manual-header> scripts and meta are injected in transformIndexHtml based on the first build. Adding that element later in the saved HTML updates the body but leaves the head without the component script and meta, so chrome stays broken until a full server restart.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3c10ffa. Configure here.
There was a problem hiding this comment.
Real behavior, but not a fixable bug. Documented it instead (commit 3d53465).
Adding <ck-manual-header> to a page after the initial build genuinely leaves the head without the header <meta> + component script under bundledDev, so chrome stays broken until restart. A code fix isn't feasible — regenerating the head injection means re-running the exact build pipeline the dev engine refuses to run for HTML entries, which is why the splice workaround exists at all. The real defect was that the existing doc comment understated the limitation (it claimed body edits are always reflected and only head-confined edits need a restart). I corrected the comment to state that toggling <ck-manual-header> also requires a restart.
…r bundledDev. The bundled dev HTML source freshness splice only refreshes the post-</head> markup, so adding or removing <ck-manual-header> updates the body but leaves the injected header chrome (<meta> and component script) in <head> stale until a restart. Clarify the comment to cover this case alongside head-confined edits.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3d53465. Configure here.
| return typeof file.source == 'string' ? file.source : Buffer.from( file.source ).toString( 'utf8' ); | ||
| } | ||
| if ( builtSourceMtime == undefined ) { | ||
| builtSourceMtimes.set( filePath, sourceMtime ); |
There was a problem hiding this comment.
Freshness hook path not normalized
Medium Severity
In keepManualHtmlSourceFresh, disk reads use resolve( workspaceRoot, filePath ) and mtime baselines are keyed by the raw filePath, while page lookup normalizes via toPublicSpecifier( filePath ). When bundled dev memory keys use a leading slash (as before this refactor), path.resolve treats them as absolute, statSync fails, and the catch block serves stale HTML with no body refresh. Mixed slash/no-slash keys also reset the mtime baseline separately.
Reviewed by Cursor Bugbot for commit 3d53465. Configure here.
There was a problem hiding this comment.
Not a real issue. No change made.
The premise is that memory-file keys "use a leading slash (as before this refactor)." They don't — the keys are relative paths without a leading slash (packages/ckeditor5-foo/tests/manual/foo.manual.html), which the tests encode and the implementation was verified against for Vite 8.1.0. The two consumers need different formats and both are correct:
getManualPages().has( toPublicSpecifier( filePath ) )— the page map is keyed by public specifier (/packages/…), so the lookup normalizes.resolve( workspaceRoot, filePath )— needs the relative form. Passing thetoPublicSpecifierform here would make it absolute (/packages/…) and breakresolve.
So using the raw key for resolve isn't an oversight — it's required. The old shell-based implementation resolved the disk path the exact same way (resolve( workspaceRoot, filePath )), so the "before this refactor keys had a slash" premise is false. And Vite passes one consistent key format, so the mtime map keyed by that same value is consistent.
|
|
||
| return `${ pattern }.{html,js,md,ts}`; | ||
| // HTML entry outputs are always emitted as strings. | ||
| const freshHtml = composeFreshManualHtml( file.source as string, readFileSync( sourceFilePath, 'utf8' ) ); |
There was a problem hiding this comment.
Binary HTML breaks body refresh
Low Severity
The bundled-dev freshness path passes file.source straight into composeFreshManualHtml as a string. The memory-file type still allows Uint8Array, and the prior implementation decoded buffers before merging. If Vite supplies HTML as bytes, toLowerCase throws, the catch block returns the stale bundle, and post-</head> edits never appear until restart.
Reviewed by Cursor Bugbot for commit 3d53465. Configure here.
There was a problem hiding this comment.
Not a real issue. No change made.
HTML entry outputs are emitted as strings — asserted deliberately with the inline comment and as string. There was an earlier getFileSource helper decoding Uint8Array, but that helper served the shell-composition path that's since been removed; it was dropped on purpose because HTML entries are strings under bundledDev (verified against Vite 8.1.0). Non-page memory files (JS/binary assets, which can be Uint8Array) are already filtered out one check earlier (getManualPages().has(...)), so only string HTML reaches composeFreshManualHtml. And in the impossible-today bytes case, toLowerCase throwing is caught and the built output is served — graceful degradation, not breakage.


🚀 Summary
A brief summary of what this PR changes.
📌 Related issues
💡 Additional information
Optional: Notes on decisions, edge cases, or anything helpful for reviewers.