Skip to content

Add the ckDebugPlugin() Vite plugin to the manual server package#1407

Open
filipsobol wants to merge 6 commits into
masterfrom
ci/4536-add-ck-debug-plugin
Open

Add the ckDebugPlugin() Vite plugin to the manual server package#1407
filipsobol wants to merge 6 commits into
masterfrom
ci/4536-add-ck-debug-plugin

Conversation

@filipsobol

@filipsobol filipsobol commented Jun 26, 2026

Copy link
Copy Markdown
Member

🚀 Summary

  • Add the ckDebugPlugin() Vite plugin to the manual server package.
  • Auto attach inspector in Vite-based manual test server.

📌 Related issues


💡 Additional information

N/A

const SOURCE_FILE_REGEXP = /\.[cm]?[jt]sx?$/;

export function ckDebugPlugin(): Plugin {
const debugFlags = getDebugFlags( process.env.CK_DEBUG );

@filipsobol filipsobol Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe instead of relying on environment variables like CK_DEBUG=mention,focustracker,typing pnpm manual:vite, we allow passing an array to ckDebugPlugin itself like so:

ckDebugPlugin( [
  'mention',
  'focustracker',
  'typing'
] )

This way, developers don't have to remember the exact syntax, but only to pass args to the plugin?

This would be more in line with how we allow changing loaded preset definition by updating the following import path in vite.manual.ts:

import identityDefinitions from './scripts/presets/staging.json' with { type: 'json' };

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.

Editing vite.manual.mts is already how we choose an identity file and the manual test include list, so putting debug flags in the config fits how the Vite manual server works today.

On the other hand, debug flags are usually quick, temporary switches. Making people edit the config for every debug run is awkward and can leave behind messy local changes. Passing them straight from the command line works much better from DX point of view.

In my personal opinion, the best solution would be a wrapper around Vite that supports the custom logic we had for a long time and that developers are already used to. Not all of the old options need to be supported, just some of them: debug flags, identity file, include list.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The plugin will keep using CLI env vars for now. We will discuss wrapping Vite to allow custom CLI flags in the follow-ups.

@marburek marburek requested a review from psmyrek June 29, 2026 08:28

@psmyrek psmyrek left a comment

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.

  1. Current source has 32 debug-only require() lines (examples: here, here and here). When ckDebugPlugin() uncomments them, Vite leaves require() calls in browser ESM output and that will fail.

    CK_DEBUG=mention,engine pnpm manual:vite
    

    After opening http://localhost:8125/external/ckeditor5/packages/ckeditor5-mention/tests/manual/mention.html, error is thrown:

    Uncaught TypeError: (intermediate value)(...).default is undefined
        <anonymous> model.ts:46
    
  2. The ckDebugPlugin() has been defined in ckeditor5-dev-manual-server. What about supporting debug flags in automated tests?

  3. Related PR is missing in ckeditor5-commercial that will enable the new plugin in vite.manual.mts.

Comment thread packages/ckeditor5-dev-manual-server/src/debug-plugin/plugin.ts Outdated
const SOURCE_FILE_REGEXP = /\.[cm]?[jt]sx?$/;

export function ckDebugPlugin(): Plugin {
const debugFlags = getDebugFlags( process.env.CK_DEBUG );

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.

Editing vite.manual.mts is already how we choose an identity file and the manual test include list, so putting debug flags in the config fits how the Vite manual server works today.

On the other hand, debug flags are usually quick, temporary switches. Making people edit the config for every debug run is awkward and can leave behind messy local changes. Passing them straight from the command line works much better from DX point of view.

In my personal opinion, the best solution would be a wrapper around Vite that supports the custom logic we had for a long time and that developers are already used to. Not all of the old options need to be supported, just some of them: debug flags, identity file, include list.

When `CK_DEBUG` is not set, the base `CK_DEBUG` flag is now enabled by default, matching the behavior of the legacy webpack-based manual test server. Only `CK_DEBUG=false` disables debug mode entirely.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 65d8018. Configure here.

get() {
return editor;
}
} );

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing inspector attach on existing editor

Low Severity

autoAttachInspector() copies the current window.editor into its closure but only calls CKEditorInspector.attach from the property setter. When an editor instance is already on window.editor when the function runs, the getter keeps that instance but the inspector is never attached.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 65d8018. Configure here.

@filipsobol

Copy link
Copy Markdown
Member Author

It looks like the original issue that forced us to use require is no longer valid, so I replaced all such calls with static imports:

Some imports also tried to pull default exports that were no longer valid (replaced by named exports), so I updated them too.

@filipsobol filipsobol requested a review from psmyrek June 29, 2026 11:52
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.

2 participants