refactor: migrate to TypeScript with Vitest and modern tooling#311
Merged
refactor: migrate to TypeScript with Vitest and modern tooling#311
Conversation
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant modernization effort, transitioning the project from JavaScript to TypeScript and upgrading its testing infrastructure. These changes enhance code quality, maintainability, and reliability, while also improving compatibility with the latest Vercel CLI and GitHub Actions features. The refactor also introduces more robust error handling and security practices. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Contributor
|
Deploy preview for team-scope-test ready! ✅ Preview Built with commit c3d6592. |
Contributor
|
Deploy preview for express-basic-auth ready! ✅ Preview Built with commit c80f8f7. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the vercel-action by migrating it from JavaScript to TypeScript, replacing Jest with Vitest for testing, and updating build configurations. The changes involve converting existing JavaScript files to TypeScript, introducing new test files and configurations, and modularizing the action's logic. Feedback includes implementing exponential backoff in the retry function, improving the head_commit debug logging by using JSON.stringify, and adjusting the logging level for vercel inspect output from warning to info/debug to reduce noise and confusion.
There was a problem hiding this comment.
Pull request overview
Refactors the GitHub Action implementation from a single JS entrypoint into a modular TypeScript codebase, introduces Vitest-based tests, and updates tooling/build output for a Node 24 action runtime.
Changes:
- Migrates action logic into
src/TypeScript modules and rebuildsdist/artifacts. - Replaces Jest with Vitest and adds a comprehensive test suite.
- Updates tooling configuration (ESLint, TSConfig) and package metadata/scripts for TS + NCC bundling.
Reviewed changes
Copilot reviewed 19 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adds Vitest runner + coverage configuration. |
| tsconfig.json | Introduces strict TypeScript compiler configuration. |
| src/vercel.ts | Implements Vercel CLI deploy/inspect/alias logic with retry handling. |
| src/utils.ts | Adds shared utilities (args parsing, retry, comment helpers). |
| src/types.ts | Defines shared TypeScript types for config/context payloads. |
| src/index.ts | New main action entrypoint coordinating config, deploy, aliasing, comments. |
| src/github-comments.ts | Adds PR/commit comment creation/update logic via Octokit REST. |
| src/config.ts | Centralizes action input parsing and Vercel env setup. |
| src/tests/vercel.test.ts | Unit tests for deploy/inspect/alias behavior and retry paths. |
| src/tests/utils.test.ts | Unit tests for utility functions and comment builders. |
| src/tests/index.test.ts | Basic integration-style coverage of index module behavior. |
| src/tests/github-comments.test.ts | Tests for PR/commit comment creation/update flows. |
| src/tests/config.test.ts | Tests for input parsing, alias templates, and env export rules. |
| package.json | Updates scripts/deps for TS/Vitest; sets engines to Node 24. |
| now.js | Removes deprecated legacy helper script. |
| jest.config.js | Removes Jest configuration. |
| index.test.js | Removes old placeholder Jest test. |
| index.js | Removes legacy monolithic JS implementation. |
| eslint.config.mjs | Enables TypeScript linting and Vitest globals. |
| dist/vercel.d.ts.map | Adds generated type declaration sourcemap for Vercel module. |
| dist/vercel.d.ts | Adds generated type declarations for Vercel module. |
| dist/utils.d.ts.map | Adds generated type declaration sourcemap for utils module. |
| dist/utils.d.ts | Adds generated type declarations for utils module. |
| dist/types.d.ts.map | Adds generated type declaration sourcemap for types module. |
| dist/types.d.ts | Adds generated type declarations for types module. |
| dist/sourcemap-register.js | Adds runtime sourcemap support bundle. |
| dist/licenses.txt | Adds bundled third-party license output from NCC build. |
| dist/index.d.ts.map | Adds generated type declaration sourcemap for index module. |
| dist/index.d.ts | Adds generated type declarations stub for index module. |
| dist/github-comments.d.ts.map | Adds generated type declaration sourcemap for GitHub comments module. |
| dist/github-comments.d.ts | Adds generated type declarations for GitHub comments module. |
| dist/config.d.ts.map | Adds generated type declaration sourcemap for config module. |
| dist/config.d.ts | Adds generated type declarations for config module. |
| .please/memory/tasklist.json | Adds please-session task tracking artifact for the migration. |
| .please/memory/session-summary.md | Adds please-session summary artifact for the migration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/utils.ts">
<violation number="1" location="src/utils.ts:46">
P3: Docstring claims "exponential backoff" but the implementation uses a constant 3-second delay. Either update the comment to say "constant delay" or implement actual exponential backoff (e.g., `RETRY_DELAY_MS * 2 ** retryCount`).</violation>
</file>
<file name="src/__tests__/github-comments.test.ts">
<violation number="1" location="src/__tests__/github-comments.test.ts:109">
P2: This test doesn't set up a return value for `mockListCommentsForCommit` and implicitly relies on mock state leaking from a prior test. `vi.clearAllMocks()` clears call history but preserves `mockResolvedValue` implementations, so this works by accident when tests run in the default order. Add an explicit mock setup to make this test independent.</violation>
</file>
<file name="src/__tests__/vercel.test.ts">
<violation number="1" location="src/__tests__/vercel.test.ts:247">
P2: This assertion is a no-op. `addVercelMetadata` produces args like `githubCommitMessage="..."`, which start with the key name, not `"`. The filter `a.startsWith('"')` never matches, so `metaArgs` is always empty and the loop never runs. The test passes regardless of whether sanitization works.</violation>
</file>
<file name=".please/memory/session-summary.md">
<violation number="1" location=".please/memory/session-summary.md:18">
P2: Incorrect Node version in session summary. The project uses Node 24 (per `action.yml` and `package.json` engines), not Node 20.</violation>
</file>
<file name="src/__tests__/utils.test.ts">
<violation number="1" location="src/__tests__/utils.test.ts:123">
P2: The `retry` tests use real `setTimeout` delays (3 s per retry), adding ~9 s of wall-clock time and requiring inflated timeouts. Use `vi.useFakeTimers()` + `vi.advanceTimersByTimeAsync()` to make these instant and deterministic.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:53">
P2: `@types/node` is pinned to the 20.x range (`^20.0.0`) but the project targets Node 24 (`engines.node: "24.x"`, `action.yml: node24`). Bump to `^24.0.0` so the type definitions match the runtime.</violation>
</file>
<file name="src/github-comments.ts">
<violation number="1" location="src/github-comments.ts:32">
P2: No pagination on comment listing — on PRs/commits with >30 comments, `findPreviousComment` will miss the existing bot comment and create a duplicate. At minimum, add `per_page: 100` to both API calls. For full correctness, use `octokit.paginate()` to iterate all pages.</violation>
</file>
<file name="src/__tests__/index.test.ts">
<violation number="1" location="src/__tests__/index.test.ts:8">
P1: The `@actions/core` mock is missing `setSecret`, which `src/config.ts` calls during `getActionConfig()`. When `await import('../index')` executes, `run()` immediately throws a TypeError on `core.setSecret(...)` and the error is silently caught by the top-level `.catch()` handler — so the test passes but never exercises the deployment path it claims to test.</violation>
</file>
<file name="src/config.ts">
<violation number="1" location="src/config.ts:27">
P2: `payload.pull_request_target` is always `undefined` — GitHub's `pull_request_target` event uses the `pull_request` key in the payload, not `pull_request_target`. This fallback never activates and gives a false sense of coverage. The `pull_request_target` property in `PullRequestPayload` should be removed (also in `types.ts` and `index.ts` where the same pattern appears).</violation>
</file>
<file name="src/__tests__/config.test.ts">
<violation number="1" location="src/__tests__/config.test.ts:25">
P2: This `vi.mock('../package.json')` resolves to `src/package.json` (relative to the test file), but `config.ts` imports `../package.json` which resolves to the root `package.json`. The mock is never applied, so the fallback test at line 82 silently reads the real `package.json` instead of the mocked `'30.0.0'`. The test passes by accident because the real value is `^50.0.0`. If that version ever changes, the test will break in a confusing way.
Use a path relative to the module under test, or use the resolved module path.</violation>
</file>
<file name="src/vercel.ts">
<violation number="1" location="src/vercel.ts:71">
P2: Deploy's personal-account retry removes `VERCEL_ORG_ID`/`VERCEL_PROJECT_ID` from the environment but still passes `--scope` in the rebuilt args (via `buildDeployArgs`). `aliasDomainsToDeployment` handles the same `PERSONAL_ACCOUNT_SCOPE_ERROR` by stripping `--scope` from the retry args. If the scope error is caused by `--scope` (not only the env vars), this retry will fail again with the same error.</violation>
</file>
Architecture diagram
sequenceDiagram
participant R as GitHub Action Runner
participant M as src/index.ts (Main)
participant C as src/config.ts
participant V as src/vercel.ts
participant G as src/github-comments.ts
participant CLI as Vercel CLI (npx)
participant API as GitHub REST API
Note over R,API: NEW: Modular TypeScript Runtime Flow
R->>M: run()
M->>C: CHANGED: getActionConfig()
C->>R: core.getInput() (Token, IDs, Args)
C->>R: core.setSecret(vercelToken)
C-->>M: ActionConfig object
M->>C: setVercelEnv(config)
C->>R: NEW: export VERCEL_TELEMETRY_DISABLED=1
opt Org and Project IDs provided
C->>R: CHANGED: export VERCEL_ORG_ID & VERCEL_PROJECT_ID
end
M->>M: getDeploymentContext()
Note right of M: Extracts Git ref, sha, and message
M->>V: vercelDeploy(config, context)
V->>CLI: npx vercel [args]
alt NEW: Personal Account Scope Error
CLI-->>V: 403 Personal Account Error
V->>R: core.warning (Retrying without IDs)
V->>V: Delete VERCEL_ORG_ID/PROJECT_ID env
V->>CLI: NEW: npx vercel [retry without scope]
end
CLI-->>V: Stdout (Deployment URL)
V-->>M: deploymentUrl
M->>V: CHANGED: vercelInspect(deploymentUrl)
V->>CLI: npx vercel inspect
CLI-->>V: Stderr (Project Name)
V-->>M: deploymentName
opt config.aliasDomains is not empty
M->>V: NEW: aliasDomainsToDeployment()
loop For each domain
V->>V: retry(alias command)
V->>CLI: npx vercel alias [url] [domain]
end
end
M->>R: core.setOutput(preview-url, preview-name)
opt config.githubComment is enabled
M->>G: createCommentOnPullRequest / Commit
G->>API: CHANGED: findPreviousComment() (Octokit v6)
API-->>G: Existing comment ID or null
alt Comment exists
G->>API: updateComment()
else New comment
G->>API: createComment()
end
end
M-->>R: Complete (Success/Failed)
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
BREAKING CHANGE: @actions/github upgraded from v2 to v6 - Convert index.js to src/index.ts with strict TypeScript mode - Extract pure utility functions to src/utils.ts for testability - Add tsconfig.json with strict compiler options - Replace Jest with Vitest for faster, ESM-native testing - Add comprehensive unit tests for utility functions (43 tests) - Add integration tests for GitHub Action structure (12 tests) - Target: 80%+ code coverage - Migrate from deprecated `new github.GitHub(token)` to `github.getOctokit(token)` - Update all API calls from `octokit.repos.*` to `octokit.rest.repos.*` - Update all API calls from `octokit.issues.*` to `octokit.rest.issues.*` - Update all API calls from `octokit.git.*` to `octokit.rest.git.*` - Enable TypeScript support in @antfu/eslint-config - Replace Jest globals with Vitest globals (vi instead of jest) - Add lib/ to ignore patterns - Update ncc build to compile TypeScript directly - Add source maps and licenses to dist output - Add typecheck script for standalone type checking - Update all npm scripts for TypeScript workflow - index.js (migrated to src/index.ts) - index.test.js (migrated to src/__tests__/*.test.ts) - jest.config.js (replaced by vitest.config.ts) - now.js (unused legacy file) Closes #291
- Move PullRequestPayload and ReleasePayload interfaces to top of file - Use PullRequestPayload type instead of inline type assertion - Simplify parseArgs regex match logic (remove redundant fallback)
- Extract types to src/types.ts - Extract config and initialization to src/config.ts - Extract Vercel functions to src/vercel.ts - Extract GitHub comment functions to src/github-comments.ts - Refactor run() into smaller focused functions (< 50 LOC each) - Add try-catch around execSync for git log with descriptive error - Add error handling for GitHub API calls - Make alias failures explicit with warning messages - Extract magic numbers to named constants (RETRY_DELAY_MS, ALIAS_RETRY_COUNT) - Use buildCommentPrefix() consistently in both comment functions All files now under 300 LOC limit, all functions under 50 LOC limit.
- Extract and validate deployment URL from vercel CLI stdout - Wrap vercelInspect exec in try-catch to prevent action failure - Expand try-catch scope in comment functions to cover API lookups - Route stderr to core.warning for better error visibility
- Add vercel.test.ts: URL extraction, inspect regex, alias retry - Add config.test.ts: alias domain substitution, env export - Add github-comments.test.ts: comment create/update, error handling - Total tests: 55 → 100
- Add core.setSecret for vercel token to prevent log exposure - Disable telemetry with VERCEL_TELEMETRY_DISABLED env var - Require both org and project IDs together (v41+ compat) - Auto-retry deployment on personal account scope error - Sanitize commit message newlines and quotes in metadata - Add ignoreReturnCode for proper exit code handling - Update tests for new behavior (105 total)
Reduce parameter count from 6 to 2 by passing DeploymentContext object instead of individual ref, commit, sha, commitOrg, commitRepo arguments. Aligns with AGENTS.md parameter limit of 5.
Capture stderr in alias exec and retry without --scope when Vercel CLI rejects personal account scope, matching the deploy retry pattern. Add tests for alias scope retry, retry failure, and non-scope failures.
Source map files (.d.ts.map) contain absolute local paths that differ between local builds and CI runners, causing check-dist to fail. Disable declarationMap in tsconfig.json since these files are not needed for GitHub Action runtime execution.
amondnet
force-pushed
the
amondnet/determined-twister
branch
from
34dde56 to
ba5b830
Compare
- Implement exponential backoff in retry function (was constant delay) - Fix debug log for head_commit to use JSON.stringify - Change vercelInspect stderr logging from warning to info level - Fix retry after PERSONAL_ACCOUNT_SCOPE_ERROR to omit --scope - Bump @types/node from ^20.0.0 to ^24.0.0 to match Node 24 runtime - Add setSecret to @actions/core mock in index.test.ts - Fix mock path in config.test.ts from ../package.json to ../../package.json - Update test expectation to match mocked vercel version (30.0.0) - Remove pull_request_target from PullRequestPayload (always undefined) - Add per_page: 100 to comment listing API calls to reduce duplicate risk - Fix Node version in session-summary.md (20 → 24)
There was a problem hiding this comment.
0 issues found across 11 files (changes from recent commits).
Requires human review: Major refactor and migration to TypeScript with significant dependency updates (Vercel v50, @actions/github v6) require human verification of core logic and runtime compatibility.
Co-authored-by: amondnet <1964421+amondnet@users.noreply.github.com> Agent-Logs-Url: https://github.com/amondnet/vercel-action/sessions/16d171b4-3be8-43f8-a736-b5f3ff13deba
|
Copilot stopped work on behalf of
amondnet due to an error
March 26, 2026 08:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
vercelScopeoptional inActionConfigand use{ ...config, vercelScope: undefined }instead of the unsafe castundefined as unknown as string💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.