Skip to content

test(cli): pin behavior for malformed .svelte files in both passes - #116

Merged
oekazuma merged 1 commit into
mainfrom
advisor/002-malformed-svelte-characterization
Jul 5, 2026
Merged

oekazuma merged 1 commit into
mainfrom
advisor/002-malformed-svelte-characterization

Conversation

@oekazuma

@oekazuma oekazuma commented Jul 5, 2026 •

Copy link
Copy Markdown
Owner

Summary

Characterization tests only — no behavior change. svelte-vitals handles syntactically broken .svelte files asymmetrically across its two passes, and neither behavior was pinned by a test:

  • Component pass (collectComponentFacts): a parse/read failure is swallowed and the file contributes empty facts — analysis continues for the rest of the project.
  • Route pass (resolveRoute): no try/catch — one broken +page.svelte/+layout.svelte aborts the entire analysis with exit 2.

This PR pins both behaviors (including the asymmetry) as an intentional contract, so future refactors — notably the planned per-run parse cache — cannot change them unnoticed, and so the hand-maintained empty-facts fallback shape is caught by a strict toEqual when ComponentFacts gains a field.

Tests added (packages/cli/test/malformed-svelte.test.ts)

  1. A broken $lib component yields exact empty facts while the well-formed sibling route file is still parsed normally
  2. run() on a project with a broken component completes with exit 0/1 — never 2
  3. An unreadable file (readFile rejects) takes the same fallback, exercised via an in-memory Runtime
  4. A broken route file makes run() exit 2 with a svelte-vitals: error — commented as a known, currently-intentional asymmetry that must be updated deliberately, not silenced

Two new fixtures: malformed-component-project/, malformed-route-project/.

Note

The intentionally-invalid fixtures had to be added to .prettierignore (prettier-plugin-svelte cannot parse them), mirroring the existing eslint fixture exclusion.

Verification

  • pnpm typecheck / pnpm lint — clean
  • pnpm test — 719 tests pass across all packages (cli 288 incl. the 4 new)
  • No changeset (test-only change)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of malformed Svelte files so valid files can still be processed when nearby fixtures contain syntax errors or are unreadable.
    • Route checks now fail cleanly with a clear error when a page component cannot be parsed, instead of producing inconsistent results.
  • Tests

    • Added coverage for malformed component and route scenarios to verify parsing, file-read failures, and error reporting.

Adds characterization tests that lock in the current asymmetric handling of
syntactically invalid .svelte files: collectComponentFacts() swallows parse
failures and returns empty facts for the broken file only, while the route
path (resolveRoute/collectRoutes) has no try/catch and propagates the parse
error out to run()'s top-level catch, aborting the whole analysis with exit 2.

Also excludes the two new intentionally-invalid fixture projects from
Prettier's check (prettier-plugin-svelte can't format syntactically broken
Svelte source), mirroring the existing eslint ignore for test/fixtures.
@coderabbitai

coderabbitai Bot commented Jul 5, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds two fixture Svelte projects (component and route paths) containing intentionally malformed, unclosed template syntax, updates .prettierignore to exclude these fixtures from formatting, and introduces a Vitest suite validating that parsing failures are handled gracefully without crashing.

Changes

Malformed Svelte fixtures and parser-failure tests

Layer / File(s) Summary
Malformed fixture projects
.prettierignore, packages/cli/test/fixtures/malformed-component-project/*, packages/cli/test/fixtures/malformed-route-project/*
Adds two fixture projects with package.json metadata and Svelte files containing unclosed {#if} blocks; excludes these fixture directories from Prettier formatting.
Parser-failure test suite
packages/cli/test/malformed-svelte.test.ts
Adds tests verifying collectComponentFacts returns empty facts (rather than throwing) on parse errors or unreadable files while other files still parse correctly, and that run() exits with code 2 and logs an error when a route file is malformed.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

  • oekazuma/svelte-vitals#112: Overlaps with this PR's fixture/test coverage of parse and read failures, related to parseComponentFacts refactor behavior for handling malformed input.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the PR’s main change: characterization tests pinning malformed .svelte behavior in both analysis passes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch advisor/002-malformed-svelte-characterization

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
packages/cli/test/malformed-svelte.test.ts (1)

38-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: extract the shared "empty facts" fixture.

The expected empty-facts object is duplicated verbatim between the two tests. A small shared constant would reduce duplication and keep both assertions in sync if the shape in components.ts changes.

♻️ Proposed consolidation
+const EMPTY_FACTS = {
+  eachBlocks: [],
+  effects: [],
+  htmlTags: [],
+  javascriptUrls: [],
+  loc: 0,
+  propCount: 0,
+  imports: [],
+  namespaceImports: [],
+  constableStates: [],
+  suppressions: []
+};
+
 describe('collectComponentFacts: malformed .svelte files (component path)', () => {
   ...
-    expect(broken).toEqual({
-      file: 'src/lib/Broken.svelte',
-      eachBlocks: [],
-      ...
-    });
+    expect(broken).toEqual({ file: 'src/lib/Broken.svelte', ...EMPTY_FACTS });

Also applies to: 94-106

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cli/test/malformed-svelte.test.ts` around lines 38 - 50, The
empty-facts expectation is duplicated across the malformed Svelte tests, so
extract it into a shared fixture constant and reuse it in both assertions.
Update the tests around the broken/empty facts checks to reference the shared
object instead of inlining the same shape, keeping the fixture aligned with the
expected structure from components.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/cli/test/malformed-svelte.test.ts`:
- Around line 38-50: The empty-facts expectation is duplicated across the
malformed Svelte tests, so extract it into a shared fixture constant and reuse
it in both assertions. Update the tests around the broken/empty facts checks to
reference the shared object instead of inlining the same shape, keeping the
fixture aligned with the expected structure from components.ts.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 00d77c0c-0965-45c6-8522-b4d4bb1979c5

📥 Commits

Reviewing files that changed from the base of the PR and between e6747ec and f7f22c2.

📒 Files selected for processing (7)
  • .prettierignore
  • packages/cli/test/fixtures/malformed-component-project/package.json
  • packages/cli/test/fixtures/malformed-component-project/src/lib/Broken.svelte
  • packages/cli/test/fixtures/malformed-component-project/src/routes/+page.svelte
  • packages/cli/test/fixtures/malformed-route-project/package.json
  • packages/cli/test/fixtures/malformed-route-project/src/routes/+page.svelte
  • packages/cli/test/malformed-svelte.test.ts

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.

1 participant