-
-
Notifications
You must be signed in to change notification settings - Fork 13
refactor: migrate action to TypeScript #128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Refactored all source and test files to TypeScript, updating file extensions and type annotations. Updated documentation and package configuration to reflect TypeScript usage, added TypeScript-related dev dependencies, and introduced tsconfig.json for compiler settings.
Added explicit null/undefined checks in several functions to prevent runtime errors when handling empty or missing values. Improved parsing of GITHUB_REPOSITORY environment variable to validate format and throw an error if owner or repo is missing.
Upgrades target and module to ES2024/ESNext, switches module resolution to 'bundler', and enables stricter type checking options. Removes unused and redundant options, and updates include/exclude paths for improved project structure.
Cleaned up extra whitespace after splitting GITHUB_REPOSITORY in main(). No functional changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request migrates the codebase from JavaScript to TypeScript, providing stronger type safety and improved developer experience. The migration includes converting the main source files to TypeScript, updating build and test tooling to support TypeScript, and modernizing the project configuration.
Key changes:
- Source code migrated from JavaScript with JSDoc annotations to native TypeScript with type definitions
- Test suite updated to use
tsxfor running TypeScript tests directly - New
tsconfig.jsonadded with strict type checking enabled - Documentation updated to reflect TypeScript usage
Reviewed Changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.ts |
Main source file converted from JS to TS with native type annotations |
src/types.ts |
New TypeScript types file centralizing all type definitions |
tsconfig.json |
New TypeScript configuration with strict mode enabled |
package.json |
Updated scripts to use tsx and added TypeScript dependencies |
package-lock.json |
Added TypeScript, tsx, and esbuild dependencies |
test/*.test.ts |
Test files updated to import from TypeScript source |
test/README.md |
Documentation updated for TypeScript testing approach |
.github/copilot-instructions.md |
Updated to reflect TypeScript instead of JavaScript |
dist/index.js |
Compiled output from TypeScript source |
Comments suppressed due to low confidence (1)
src/index.ts:785
- The type assertion
as ReviewEventon line 785 is unsafe. ThegetInput('event')returns a string, and after callingtoUpperCase(), there's no guarantee that the result is a validReviewEventvalue. If an invalid event type is passed, this could cause runtime errors. Consider adding validation to ensure the value is a validReviewEventbefore the type assertion, or define a helper function that validates and returns the correct type.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Replaces the 'any' type with the proper Octokit type in the RunConfig interface for improved type safety and clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
src/index.ts:180
- [nitpick] The explicit type annotation
c: AnyLineChangein the lambda function is redundant since TypeScript can infer the type from thegroupparameter which is already typed asAnyLineChange[].
Suggested fix:
const lastChange = group.findLast((c) => isDeletedLine(c) || isAddedLine(c))This is more idiomatic and cleaner in TypeScript.
src/index.ts:354
- The
deletedLinesvariable is being extracted twice fromfilterChangesByType(changes)- once at line 348 and again at line 354. This is redundant and inefficient. SincedeletedLinesis also used at line 378 in the "Mixed changes" section, it should be extracted once at the beginning of the function along withaddedLinesandunchangedLines(line 292-293).
Suggested fix: Add deletedLines to the destructuring at line 292-293:
const { addedLines, deletedLines, unchangedLines } = filterChangesByType(changes)Then remove the duplicate extractions at lines 348 and 354.
src/index.ts:391
- The
deletedLinesvariable is being extracted fromfilterChangesByType(groupChanges)here, but it's not used in this function. This call was likely added by mistake during the TypeScript migration. ThedeletedLinesis only needed ingenerateSuggestionBody(), not incalculateLinePosition().
Suggested fix: Remove this line as it serves no purpose and creates unnecessary overhead.
src/index.ts:552
- Using
as anyto bypass TypeScript's type checking defeats the purpose of the TypeScript migration. Theoctokit.pulls.getmethod should be properly typed or the type assertion should be more specific.
Consider using a more specific type assertion like:
typeof (octokit as { pulls?: { get?: unknown } }).pulls?.get !== 'function'Or better yet, check if the migration to TypeScript provides proper types for the Octokit instance that would make this check unnecessary.
src/index.ts:558
- Using
as anyto bypass TypeScript's type checking defeats the purpose of the TypeScript migration. Theoctokit.pulls.getcall should be properly typed.
Consider defining a proper interface for the Octokit pulls API or using the existing types from @octokit/types to avoid using any.
src/index.ts:780
- Using
anytype for the file parameter defeats the purpose of the TypeScript migration. The type should be properly defined based on the Octokit API response.
Consider using the proper type from @octokit/types:
.data.map((file: Endpoints['GET /repos/{owner}/{repo}/pulls/{pull_number}/files']['response']['data'][number]) => file.filename)Or define a more concise type alias in src/types.ts for this.
Improves type safety by introducing PullRequestFile type and using it in file mapping. Refactors generateSuggestionBody to avoid redundant filterChangesByType calls and clarifies handling of deleted and added lines. Updates fetchCanonicalDiff for stricter type checks and better handling of GitHub diff responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/index.ts:785
- The
eventvariable is cast toReviewEventafter callingtoUpperCase(), but there's no validation that the result is actually a validReviewEventvalue. If a user provides an invalid event value (e.g., 'invalid'), this could cause runtime errors when the value is passed to the GitHub API. Consider adding validation to ensure the value is one of the valid review event types ('APPROVE', 'REQUEST_CHANGES', 'COMMENT').
test/unit.test.ts:9 - [nitpick] The indentation of the import statement has changed to use 4 spaces per level instead of 2, which is inconsistent with the project's Prettier configuration that uses 2 spaces (as seen in
package.json). Consider running Prettier to ensure consistent formatting.
Adds validation to ensure the 'event' input is one of APPROVE, REQUEST_CHANGES, or COMMENT before proceeding. Updates tests to verify valid event types are accepted and throws an error for invalid types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/index.ts:788
- The
as anytype assertion bypasses TypeScript's type safety. Instead, use a proper type guard to check if the string is a valid event type. Consider:if (!validEvents.includes(eventInput as ReviewEvent))or create a type guard functionisValidEvent(value: string): value is ReviewEvent.
test/unit.test.ts:170 - Using
@ts-ignoresuppresses type checking. Consider using@ts-expect-errorinstead, which will error if the line is actually valid, helping catch unnecessary suppressions. Alternatively, define a proper mock type that satisfies the required interface.
Updated TypeScript comments in unit tests to use `@ts-expect-error` instead of `@ts-ignore` for mock Octokit interface. This improves clarity and ensures intentional error suppression is tracked.
Updated the type used for event validation to use ReadonlyArray<ReviewEvent> and cast eventInput as ReviewEvent for improved type safety.
This pull request migrates the codebase from JavaScript to TypeScript, updates documentation and testing conventions, and improves build and test tooling. The most important changes are the conversion of main source files and type annotations to TypeScript, updates to documentation to reflect the new language and file structure, and improvements to build and test scripts to use TypeScript-compatible tools.