docs: keep comments short, plain and about the why

Trims the comments added with the profiling tests and the preprocessor fix,
and writes the rule down in CLAUDE.md so it holds next time.

Author: DriesH

CLAUDE.md

@@ -166,6 +166,11 @@ Run the whole thing: `npm run test:e2e`. One project: `npx playwright test --pro
 - Linting: oxlint, root config in `.oxlintrc.json`, per-package configs in `packages/<pkg>/.oxlintrc.json`
 - Doc comments on functions must be terse and follow JSDoc (`/** ... */` with `@param` / `@returns` where they add
   information). Write them only when they clarify non-obvious behavior; do not restate the signature.
+- Comments explain WHY, not what. The code already says what it does.
+- Keep comments as short as possible. One or two lines is usually enough. If you need a paragraph, the code probably
+  needs the work instead.
+- Write them in plain English, the way you would explain it to the developer sitting next to you. No academic or
+  research-paper tone, no long build-ups, no restating the obvious.
 - When there is an opportunity to create a shared utility for the code or the tests, YOU MUST DO SO.
 - Code duplication must be kept at A MINIMUM and should only be done when it makes sense in the context of the feature.
 

packages/svelte/src/preprocessor.ts

@@ -121,19 +121,13 @@ function escapeString(str: string): string {
 }
 
 /**
- * The regex Svelte's own preprocessor uses to find script tags, copied verbatim from
- * `svelte/src/compiler/preprocess/index.js`. The leading comment alternative is load-bearing: it
- * consumes `<!-- ... -->` first, so a commented-out or documented `<script>` matches as a comment
- * rather than as a tag. Matching Svelte here is the point, because Svelte decides which tags reach
- * the `script` hook and the markup hook has to predict that exactly.
+ * Copied from Svelte's own preprocessor, because Svelte decides which tags reach the script hook and
+ * we have to agree with it. The `<!-- -->` branch is what stops a commented-out `<script>` counting.
  */
 const REGEX_SCRIPT_OR_COMMENT =
     /<!--[^]*?-->|<script((?:\s+[^=>'"/\s]+=(?:"[^"]*"|'[^']*'|[^>\s]+)|\s+[^=>'"/\s]+)*\s*)(?:\/>|>([\S\s]*?)<\/script>)/g;
 
-/**
- * Types Svelte still treats as an instance script. A tag carrying anything else (`application/ld+json`,
- * `importmap`, `text/template`) holds data rather than component code.
- */
+/** Anything else (`application/ld+json`, `importmap`, ...) holds data, not component code. */
 const JAVASCRIPT_SCRIPT_TYPES = new Set([
     'text/javascript',
     'application/javascript',
@@ -147,7 +141,7 @@ const JAVASCRIPT_SCRIPT_TYPES = new Set([
  * i.e. a script that is not `<script module>` / `<script context="module">`.
  */
 function hasInstanceScript(content: string): boolean {
-    // matchAll clones the regex, so the shared `g` literal keeps no lastIndex between calls.
+    // matchAll clones the regex, so the shared /g/ literal can't leak lastIndex between calls.
     for (const match of content.matchAll(REGEX_SCRIPT_OR_COMMENT)) {
         if (match[0].startsWith('<!--')) {
             continue;
@@ -162,10 +156,9 @@ function hasInstanceScript(content: string): boolean {
 }
 
 /**
- * Svelte hands the `script` hook every script tag in the file, nested ones included, so a
- * `<script type="application/ld+json">` inside the markup arrives here looking like component code.
- * Prepending an import to one corrupts the data it holds. Skipping on an unrecognized type costs at
- * most a missing registration; injecting into one ships broken output.
+ * Svelte passes us every script tag, nested ones too, so a JSON-LD block turns up here looking like
+ * component code. Injecting into one would corrupt it, so when in doubt we skip: a missing
+ * registration is cheaper than broken output.
  */
 function isJavaScriptScript(attributes: Record<string, string | boolean>): boolean {
     const type = attributes.type;

packages/svelte/tests/fixtures/preprocessed/+layout.svelte

@@ -1,7 +1,4 @@
-<!--
-    Deliberately contains NO call to __flareProfileComponent. Everything under this directory is
-    instrumented by the real preprocessor, configured in vitest.config.mts.
--->
+<!-- No profiling code here on purpose. vitest.config.mts injects it. -->
 <script lang="ts">
     let { children }: { children?: () => unknown } = $props();
 </script>

packages/svelte/tests/fixtures/preprocessed/AddToCartButton.svelte

@@ -1,7 +1,2 @@
-<!--
-    No instance script at all. This is the markup-hook path: the preprocessor has to add the script
-    block itself, and must not then inject a second time when the script hook runs over that block.
-    Keep the words "script tag" out of this comment in their angle-bracket form; hasInstanceScript
-    matches on raw text and would treat the mention as a real instance script.
--->
+<!-- No script tag, so the markup hook has to add one. -->
 <button data-testid="add-to-cart">Add to cart</button>

packages/svelte/tests/fixtures/preprocessed/Harness.svelte

@@ -1,8 +1,5 @@
-<!--
-    Stands in for SvelteKit's generated root: it passes the page to the layout as a snippet, which is
-    the shape the whole nesting design depends on. Its own name matches no allowlist entry, so it
-    records no span and the layout parents straight to the active root.
--->
+<!-- Stands in for SvelteKit's root: hands the page to the layout as a snippet. Not in the allowlist,
+     so it records nothing itself. -->
 <script lang="ts">
     import Layout from './+layout.svelte';
     import Page from './product/[id]/+page.svelte';

packages/svelte/tests/preprocessedRuntime.test.ts

@@ -16,13 +16,9 @@ beforeEach(() => {
 });
 
 /**
- * Everything else in this suite tests one half of the feature: preprocessor.test.ts asserts on the
- * string the preprocessor emits, profileComponent.test.ts calls __flareProfileComponent by hand. That
- * leaves the join between them untested, which is where a regression would actually hide: emit a call
- * the runtime no longer honours and both halves stay green while every span disappears.
- *
- * The fixtures under tests/fixtures/preprocessed/ contain no profiling code. The preprocessor is
- * installed over them in vitest.config.mts, so these assertions run against injected code only.
+ * preprocessor.test.ts checks what we emit, profileComponent.test.ts calls the function directly.
+ * Neither notices if the emitted call stops working, so that gap gets its own test. The fixtures hold
+ * no profiling code; vitest.config.mts injects it.
  */
 describe('preprocessor output at runtime', () => {
     it('records the documented pageload tree for components carrying no profiling code', async () => {
@@ -31,15 +27,13 @@ describe('preprocessor output at runtime', () => {
 
         const byName = Object.fromEntries(fake.spans().map((span) => [span.name, span]));
 
-        // The names are the route-aware ones from the README table, produced end to end rather than
-        // asserted against resolveProfileName in isolation. Compared as a set: onMount is bottom-up,
-        // so the recording order is an implementation detail this assertion should not pin.
+        // A set, because onMount runs bottom-up and we don't want to pin the recording order.
         expect(new Set(Object.keys(byName))).toEqual(new Set(['+layout', 'product/[id]/+page', 'AddToCartButton']));
 
-        // Harness matches no allowlist entry, so the layout parents straight to the active root.
+        // Harness isn't in the allowlist, so the layout parents straight to the root.
         expect(byName['+layout']!.parent).toEqual({ traceId: 'T', parentSpanId: 'root' });
-        // The page reaches the layout only because snippet content inherits the context of the
-        // component that renders the snippet. This is the assertion that fails if that ever changes.
+        // Only works because snippet content picks up the context of whoever renders it. If Svelte
+        // ever changes that, this is the test that breaks.
         expect(byName['product/[id]/+page']!.parent).toEqual({
             traceId: 'T',
             parentSpanId: byName['+layout']!.spanId,
@@ -50,18 +44,16 @@ describe('preprocessor output at runtime', () => {
         });
     });
 
-    // AddToCartButton.svelte has no instance script, so the markup hook has to add a <script> block
-    // and the script hook then runs over that same block. Without the double-injection guard this
-    // component records two spans, or fails to compile on a duplicate declaration.
+    // The markup hook adds a script block, then the script hook sees that same block. Without the
+    // guard we'd inject twice.
     it('injects exactly once into a component with no instance script', async () => {
         render(Harness);
         await tick();
 
         expect(fake.spans().filter((span) => span.name === 'AddToCartButton')).toHaveLength(1);
     });
 
-    // Only allowlisted components are instrumented. Harness renders and is a real component in the
-    // tree, but matches nothing, so it must be invisible rather than recording an unnamed span.
+    // Harness is a real component in the tree but matches nothing, so it should stay invisible.
     it('leaves unmatched components untouched', async () => {
         render(Harness);
         await tick();

packages/svelte/tests/preprocessor.test.ts

@@ -358,13 +358,10 @@ describe('withFlareConfig — profileComponents', () => {
     });
 });
 
-// Both bugs below come from treating a raw text match as a structural fact. Svelte decides what counts
-// as a script tag with the regex mirrored in REGEX_SCRIPT_OR_COMMENT, and it hands the `script` hook
-// every tag in the file rather than just the instance one.
+// Both cases below used to break because we matched raw text instead of matching what Svelte matches.
 describe('flarePreprocessor — script tags that only look like component code', () => {
     test('a <script> mentioned inside an HTML comment does not count as an instance script', async () => {
-        // Svelte's regex consumes comments first, so this component reaches the script hook with no
-        // instance script at all. Reading the mention as real leaves it silently uninstrumented.
+        // Svelte skips comments, so this component has no instance script at all.
         const source = `<!-- replaced the old <script>console.log(1)</script> block -->\n<p>hi</p>`;
         const out = await preprocess(source, flarePreprocessor(), { filename: FAKE_FILE });
 
@@ -382,8 +379,7 @@ describe('flarePreprocessor — script tags that only look like component code',
         expect((out.code.match(/__flare_prof__\(/g) || []).length).toBe(1);
     });
 
-    // A JSON-LD block is data. Prepending an ESM import to it produces invalid structured data in the
-    // shipped page, which is worse than not instrumenting the component at all.
+    // Injecting an import here would ship broken JSON-LD to the browser.
     test('leaves a nested non-JavaScript script untouched', async () => {
         const source = [
             '<script lang="ts">',
@@ -398,7 +394,6 @@ describe('flarePreprocessor — script tags that only look like component code',
         ].join('\n');
         const out = await preprocess(source, flarePreprocessor(), { filename: FAKE_FILE });
 
-        // Injected once, into the instance script only.
         expect((out.code.match(/__flare_node__/g) || []).length).toBe(1);
         expect(out.code).toMatch(/<script type="application\/ld\+json">\s*\{"@type": "Product"\}/);
         expect(() => compile(out.code, { filename: FAKE_FILE })).not.toThrow();

packages/svelte/vitest.config.mts

@@ -9,16 +9,10 @@ import { flarePreprocessor } from './src/preprocessor.js';
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 
-// The REAL preprocessor, installed over the test compile so preprocessedRuntime.test.ts can assert on
-// what injected code actually does at runtime, not just on the string the preprocessor emits.
-//
-// Scoped by allowlist rather than by path: `componentTracking: false` plus a `profileComponents` list
-// that only tests/fixtures/preprocessed/* can satisfy means every other .svelte file in this package
-// is returned untouched, so the rest of the suite compiles exactly as before.
-//
-// `importSource` points at the module itself instead of '@flareapp/svelte' so the test does not
-// depend on dist being built. That the published entries re-export the symbol is covered separately
-// by webEntry, injectEntry and sveltekitContract.
+// Runs the real preprocessor so preprocessedRuntime.test.ts can check what the injected code does,
+// not just what the preprocessor prints. The allowlist is what keeps it scoped: nothing outside
+// tests/fixtures/preprocessed/ matches, so the rest of the suite compiles unchanged. importSource
+// points straight at the module so the test doesn't need a build.
 const preprocessedFixtures = flarePreprocessor({
     componentTracking: false,
     profileComponents: [/\+(page|layout)(@[^/]*)?$/, 'AddToCartButton'],