Skip to content

Conversation

@Binlogo
Copy link
Contributor

@Binlogo Binlogo commented Nov 10, 2025

What does this PR do?

It enables quick opening of the source file in VSCode using CMD+click with the Extension.

Preview

image

@max-sixty
Copy link
Collaborator

thanks for the suggestion!

I don't know this code well, so I asked Claude to take a look. Can we indeed simplify? If not, I trust you more than Claude; but would like to keep this code simple if possible

Details

⏺ Based on my analysis, here's my review of PR #817:

Summary

The PR uses the correct API pattern (DocumentLinkProvider + custom command) since VSCode's DocumentLink doesn't support line/column positions directly. However, the implementation is over-engineered for cases
that don't exist in insta's actual snapshot format.

Issues Found

  1. Handles Non-Existent Fields (vscode-insta/src/SnapshotDocumentLinkProvider.ts:84-96)

The parser handles line: and column: fields that don't exist in insta's metadata structure:

const lineMatch = text.match(/^(?:assertion_)?line:\s*(\d+)/); // "line:" doesn't exist
const columnMatch = text.match(/^column:\s*(\d+)/); // "column:" doesn't exist

Reality: Insta only has assertion_line in pending snapshots (see insta/src/snapshot.rs:232-276). No line: or column: fields exist.

  1. Quote Handling Complexity (SnapshotDocumentLinkProvider.ts:45-51)

The code handles quoted vs unquoted paths, but insta's YAML output doesn't quote source paths:

const hasQuotes = sourcePath.startsWith(""") && sourcePath.endsWith(""");
if (hasQuotes && sourcePath.length >= 2) {
sourcePath = sourcePath.slice(1, -1);
// ... adjust positions ...
}

Reality: All snapshot examples show unquoted paths: source: insta/tests/test_basic.rs

  1. Trailing Location Parsing (SnapshotDocumentLinkProvider.ts:54-66)

Handles source: path/to/file:10:5 format, but insta stores line numbers separately in assertion_line, not in the source path.

  1. Over-Complex Path Resolution (SnapshotDocumentLinkProvider.ts:116-139)

Tries multiple fallback paths (workspace folder, all workspace folders, document directory) with file existence checks. For a Rust project, snapshots are always relative to the workspace root.

Simpler Approach

function extractSourceLink(document: TextDocument): SourceLink | undefined {
// Parse YAML frontmatter between --- markers
const text = document.getText();
const frontmatterMatch = text.match(/^---\n([\s\S]*?)\n---/);
if (!frontmatterMatch) return undefined;

const frontmatter = frontmatterMatch[1];

// Extract source (required) and assertion_line (optional, only in .snap.new files)
const sourceMatch = frontmatter.match(/^source:\s*(.+)$/m);
const lineMatch = frontmatter.match(/^assertion_line:\s*(\d+)$/m);

if (!sourceMatch) return undefined;

const sourcePath = sourceMatch[1].trim();
const line = lineMatch ? parseInt(lineMatch[1], 10) : undefined;

// Calculate range for the clickable link
const sourceLineStart = text.indexOf(`source: ${sourcePath}`);
const lineNumber = text.substring(0, sourceLineStart).split('\n').length - 1;
const startChar = text.substring(sourceLineStart).indexOf(sourcePath);

return {
  sourcePath,
  line,
  range: new Range(
    new Position(lineNumber, startChar),
    new Position(lineNumber, startChar + sourcePath.length)
  )
};

}

async function resolveSourceUri(
document: TextDocument,
sourcePath: string
): Promise<Uri | undefined> {
// Handle absolute paths
if (path.isAbsolute(sourcePath)) {
return Uri.file(sourcePath);
}

// Resolve relative to workspace root (snapshots are always workspace-relative)
const workspace = workspace.getWorkspaceFolder(document.uri);
return workspace ? Uri.file(path.join(workspace.uri.fsPath, sourcePath)) : undefined;

}

What's Good

  • ✅ Correct use of command pattern (necessary due to VSCode API limitation)
  • ✅ Good UX with CMD+click to jump to source
  • ✅ Proper handling of both file and inline snapshots

Recommendations

Option 1: Simplify to match actual format

  • Remove line:, column: field handling
  • Remove quote handling
  • Remove trailing location parsing
  • Simplify path resolution to workspace-relative only

Option 2: Keep defensive code

  • If planning future insta features (like column support), document this
  • Add comments explaining why handling non-existent cases

The command pattern itself is standard and correct—the complexity is in the parser, not the VSCode integration pattern.

@Binlogo Binlogo force-pushed the feat/vscode-source-open branch from 0d7d395 to cbdb481 Compare November 18, 2025 11:25
@Binlogo
Copy link
Contributor Author

Binlogo commented Nov 18, 2025

You're right; the current implementation is a bit complicated. This is my first time contributing to a VSCode extension with Codex's help, and I thought quick open would be useful.

I've simplified the code. Please take a look.

@max-sixty max-sixty merged commit 783ebc2 into mitsuhiko:master Nov 19, 2025
15 checks passed
@max-sixty
Copy link
Collaborator

super, thank you @Binlogo !

@Binlogo Binlogo deleted the feat/vscode-source-open branch November 19, 2025 13:49
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