Skip to content

Conversation

@som-sm
Copy link
Collaborator

@som-sm som-sm commented Oct 21, 2025

WIP

@som-sm som-sm force-pushed the feat/add-lint-rule-to-add-playground-link branch 2 times, most recently from c14142e to 73e7a64 Compare October 25, 2025 14:22
Repository owner deleted a comment from claude bot Oct 25, 2025
Repository owner deleted a comment from claude bot Oct 25, 2025
Repository owner deleted a comment from claude bot Oct 25, 2025
Repository owner deleted a comment from claude bot Oct 25, 2025
@som-sm som-sm force-pushed the feat/add-lint-rule-to-add-playground-link branch from 73e7a64 to 879af08 Compare November 12, 2025 10:22
@som-sm som-sm force-pushed the feat/add-lint-rule-to-add-playground-link branch from 596aa61 to ab66b08 Compare November 12, 2025 17:51
Repository owner deleted a comment from claude bot Nov 12, 2025
@claude
Copy link

claude bot commented Nov 12, 2025

Review Summary

This PR adds an automated lint rule to generate TypeScript Playground links for JSDoc code examples. The implementation is well-structured, but there are several issues that need to be addressed before this can be merged.

Critical Issues

1. Incomplete Implementation - PR marked as WIP

The PR description states "WIP" (Work in Progress), indicating this is not ready for review or merge.

2. Missing Test Execution

Before merging, you MUST run npm test to verify:

  • All TypeScript compiler checks pass
  • All type tests (tsd) pass
  • Linter passes with the new rule applied

3. Massive File Changes (184 files)

The PR modifies 184 files across the codebase, adding playground links to existing types. This needs careful verification:

  • Ensure all generated links are correct
  • Verify the lint rule can be run as a fixer: npm run test:xo -- --fix
  • Consider reviewing a sample of the generated links manually

4. Potential Bug: Empty Code Handling

File: lint-rules/require-playground-link.js:14-18

const getCodeIndent = code => {
	const line = code.split('\n').filter(Boolean); // eslint-disable-line unicorn/prefer-array-find
	const firstLine = line[0];
	const leadingSpaces = firstLine.slice(0, firstLine.length - firstLine.trimStart().length);
	return leadingSpaces;
};

Issue: If code contains only empty lines, line[0] will be undefined, causing a runtime error.

Fix: Add a guard:

const getCodeIndent = code => {
	const line = code.split('\n').filter(Boolean);
	if (line.length === 0) return '';
	const firstLine = line[0];
	const leadingSpaces = firstLine.slice(0, firstLine.length - firstLine.trimStart().length);
	return leadingSpaces;
};

5. Regex Doesn't Handle Edge Cases

File: lint-rules/require-playground-link.js:4

The regex /(?<openingFence>\``(?:ts|typescript)?)(?[\s\S]*?)```/g` will match:

  • Plain ``` blocks (no language specified)
  • Should it? Consider if this is intentional or should only match ts/typescript blocks

Code Quality Issues

6. Inconsistent Link Detection

File: lint-rules/require-playground-link.js:86

const doesPlaygroundLinkExist = nextLine.includes('[Playground Link]');

This is fragile - it will match any line containing the substring [Playground Link], even in unrelated text. Consider using a more precise check:

const doesPlaygroundLinkExist = /^\s*\[Playground Link\]\(/.test(nextLine);

7. Magic Number: Offset Calculation

File: lint-rules/require-playground-link.js:81-83

const codeblockStart = previousNode.range[0] + match.index + 2;
const fixerRangeStart = previousNode.range[0] + nextLineIndex + 2;

The + 2 offsets are unclear. Add comments explaining these magic numbers (likely accounting for /* characters).

8. Test Coverage Gaps

The test file has good coverage, but consider adding tests for:

  • Code blocks with only whitespace
  • Nested code blocks (if supported)
  • Multiple consecutive playground links
  • Code blocks with unusual indentation (tabs vs spaces)

Positive Aspects

✅ Well-structured lint rule with clear separation of concerns
✅ Comprehensive test suite with good edge case coverage
✅ Auto-fixable rule (saves developer time)
✅ Properly integrated into xo.config.js
✅ Follows repository conventions (import paths, ESLint structure)

Recommendations

  1. Remove WIP status - Complete the implementation before requesting review
  2. Run full test suite - Ensure npm test passes
  3. Fix the bugs - Address items #4, #5, and #6 above
  4. Verify generated links - Spot-check playground links in modified files to ensure they work
  5. Update commit message - Follow convention: Add \require-playground-link` lint rule`
  6. Consider adding documentation - Update contributing guide to mention this auto-generated feature

Verdict

Request Changes - This PR shows good work, but needs bug fixes and testing before it can be merged. The core implementation is solid, but the edge cases need attention.

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