Skip to content

[CI] Implement caching for playwright, npm, and vscode#42

Merged
Avni2000 merged 3 commits intomainfrom
cache
Feb 6, 2026
Merged

[CI] Implement caching for playwright, npm, and vscode#42
Avni2000 merged 3 commits intomainfrom
cache

Conversation

@Avni2000
Copy link
Owner

@Avni2000 Avni2000 commented Feb 6, 2026

Summary by CodeRabbit

  • Chores
    • Improved CI performance by adding caching for npm dependencies, browser binaries, and test resources.
    • Adjusted workflow trigger ordering to prioritize pull-request runs (updated trigger behavior).

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 6, 2026

Warning

Rate limit exceeded

@Avni2000 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 0 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

This PR updates the VSCode integration test GitHub Actions workflow: it changes the trigger ordering to run on pull_request and adds three caching steps (npm, Playwright browsers, VSCode test resources) placed before npm install.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow
/.github/workflows/vscode-integration-test.yml
Changed workflow triggers to run on pull_request (order swapped). Inserted three cache steps before npm install: cache ~/.npm, cache Playwright browsers ~/.cache/ms-playwright, and cache VSCode test resources .vscode-test, each keyed with OS and package-lock.json content hash.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Poem

🐇 I hopped through CI on dainty paws,

Packed npm crates and browser jaws,
Stored test bits in cozy nests,
Now pulls arrive with swifter quests,
A rabbit cheers for cached applause 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 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 main change in the pull request - implementing caching for playwright, npm, and vscode in the CI workflow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cache

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 and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/workflows/vscode-integration-test.yml:
- Line 3: The workflow currently triggers only on "on: [pull_request]" which
drops post-merge runs; update the "on" configuration to also trigger pushes to
the main branch (e.g., add a push trigger with branches: [main] alongside
pull_request) so integration tests run both on PRs and on pushes/merges to main;
modify the "on" YAML block to include push: { branches: [main] } in addition to
pull_request.
🧹 Nitpick comments (1)
.github/workflows/vscode-integration-test.yml (1)

25-29: Playwright browser cache looks correct; note that install-deps still runs every time.

The browser binary cache at ~/.cache/ms-playwright is the right path for Linux, and keying on package-lock.json is appropriate since the Playwright version (and thus browser revisions) is pinned there. Omitting restore-keys is the safer choice to avoid restoring incompatible browser builds.

Note that npx playwright install-deps (Line 44) installs system-level apt packages that aren't covered by this cache, so that step will still take time on every run. If you want to speed that up as well, you could conditionally skip it on a cache hit using the cache step's output:

Optional: skip browser install on cache hit
      - name: Cache Playwright browsers
+       id: playwright-cache
        uses: actions/cache@v4
        with:
          path: ~/.cache/ms-playwright
          key: ${{ runner.os }}-playwright-${{ hashFiles('**/package-lock.json') }}

      - name: Install Playwright deps and browsers
+       if: steps.playwright-cache.outputs.cache-hit != 'true'
        run: |
          npx playwright install-deps
          npx playwright install

However, be cautious with this — system deps (from install-deps) aren't cached, so on a browser cache hit you'd still need install-deps to run. A more granular split may be needed if you go this route.

Also applies to: 42-45

name: VSCode Integration Tests

on: [push, pull_request]
on: [pull_request]
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Confirm that dropping push trigger is intentional.

By narrowing to pull_request only, integration tests will no longer run on pushes to main (e.g., after a PR merge). If a PR is merged with a broken test that wasn't caught (e.g., due to a stale base branch), there's no post-merge safety net. Consider keeping push for main at minimum:

Suggested trigger configuration
-on: [pull_request]
+on:
+  pull_request:
+  push:
+    branches: [main]
🤖 Prompt for AI Agents
In @.github/workflows/vscode-integration-test.yml at line 3, The workflow
currently triggers only on "on: [pull_request]" which drops post-merge runs;
update the "on" configuration to also trigger pushes to the main branch (e.g.,
add a push trigger with branches: [main] alongside pull_request) so integration
tests run both on PRs and on pushes/merges to main; modify the "on" YAML block
to include push: { branches: [main] } in addition to pull_request.

✅ Addressed in commit 8631f43

Copy link
Owner Author

@Avni2000 Avni2000 Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I didn't want it to be such that every time I push to a branch with a pr open, it runs the same check twice. Any solutions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Avni2000 Yes! The solution is to restrict the push trigger to only run on specific branches (like main). This way you get the best of both worlds:

on:
  pull_request:
  push:
    branches: [main]

How this works:

  • When you push to a PR branch: Only the pull_request trigger fires → runs once
  • When you push/merge directly to main: Only the push trigger fires → runs once
  • No duplication!

This configuration:

  • Runs tests on all PRs (regardless of target branch)
  • Runs tests when code is merged to main (catching issues that slip through)
  • Avoids running twice for PR branch pushes

This is a common pattern in GitHub workflows and solves your exact concern while still maintaining post-merge coverage.


🧠 Learnings used
Learnt from: Avni2000
Repo: Avni2000/MergeNB PR: 0
File: :0-0
Timestamp: 2026-02-04T23:12:05.308Z
Learning: In src/tests/vscodeIntegration.test.ts: The integration test has verification gaps: it never directly checks the outputs array content (only execution_count), uses getCellSource() normalization which masks source format inconsistencies (string vs array), and derives expected cell values from data-cell attributes containing original cells rather than from resolver output.

- Reverts 'on push' removal b/c of coderabbit suggestion
@Avni2000 Avni2000 merged commit a31e2db into main Feb 6, 2026
2 checks passed
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