Address code review feedback for docs-only guard

Improvements to .github/actions/ensure-master-docs-safety/action.yml:
- Remove redundant null check (already handled by step condition)
- Add comprehensive documentation for 7-day lookback window
- Optimize API pagination with early exit strategy
- Reduce per_page from 100 to 30 for efficiency
- Improve deduplication to use run_number instead of created_at
- Add clickable markdown links to failing workflows in error messages
- Document handling of 'skipped' and 'neutral' workflow conclusions

Workflow improvements:
- Add clear inline comments explaining conditional logic in gem-tests.yml,
  integration-tests.yml, and lint-js-and-ruby.yml
- Clarify "Skip only if: master push AND docs-only changes" pattern

Code quality:
- Remove duplicate dependencies in knip.ts (@babel/runtime, mini-css-extract-plugin)
- Improve comment explaining runtime-injected dependencies

All changes verified with RuboCop and Prettier checks passing.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>

Author: justin808

.github/actions/ensure-master-docs-safety/action.yml

@@ -18,36 +18,57 @@ runs:
       with:
         script: |
           const previousSha = process.env.PREVIOUS_SHA;
-          if (!previousSha) {
-            core.info('No previous SHA found, skipping safety check.');
-            return;
-          }
 
-          // Query workflow runs from the last 7 days to avoid excessive API calls
-          // For very old commits, we'll skip the check rather than scan the entire history
+          // Query workflow runs from the last 7 days to avoid excessive API calls.
+          // Why 7 days? This balances API efficiency with practical needs:
+          // - Most master commits trigger CI within hours, not days
+          // - Commits older than 7 days are likely stale; better to run full CI anyway
+          // - Reduces pagination load on high-velocity repos
+          // For commits outside this window, we skip the check and allow the docs-only skip.
           const createdAfter = new Date(Date.now() - 1000 * 60 * 60 * 24 * 7).toISOString();
-          const workflowRuns = await github.paginate(github.rest.actions.listWorkflowRunsForRepo, {
-            owner: context.repo.owner,
-            repo: context.repo.repo,
-            branch: 'master',
-            event: 'push',
-            per_page: 100,
-            created: `>${createdAfter}`
-          });
 
-          const relevantRuns = workflowRuns.filter((run) => run.head_sha === previousSha);
+          // Optimize pagination: use lower per_page and collect only what we need
+          const workflowRuns = [];
+          let foundAllRelevant = false;
+
+          for await (const response of github.paginate.iterator(
+            github.rest.actions.listWorkflowRunsForRepo,
+            {
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              branch: 'master',
+              event: 'push',
+              per_page: 30,
+              created: `>${createdAfter}`
+            }
+          )) {
+            const pageRuns = response.data;
+            const relevantInPage = pageRuns.filter((run) => run.head_sha === previousSha);
+
+            if (relevantInPage.length > 0) {
+              workflowRuns.push(...relevantInPage);
+            }
+
+            // Early exit: if we found relevant runs and now seeing different SHAs,
+            // we've likely collected all runs for the previous commit
+            if (workflowRuns.length > 0 && relevantInPage.length === 0) {
+              foundAllRelevant = true;
+              break;
+            }
+          }
 
-          if (relevantRuns.length === 0) {
-            core.info(`No workflow runs found for ${previousSha}. Nothing to enforce.`);
+          if (workflowRuns.length === 0) {
+            core.info(`No workflow runs found for ${previousSha} in the last 7 days. Allowing docs-only skip.`);
             return;
           }
 
-          // Deduplicate workflow runs by keeping only the latest run for each workflow_id
-          // This handles cases where workflows are re-run manually
+          // Deduplicate workflow runs by keeping only the latest run for each workflow_id.
+          // This handles cases where workflows are re-run manually.
+          // Use run_number as tiebreaker since created_at might be identical for rapid reruns.
           const latestByWorkflow = new Map();
-          for (const run of relevantRuns) {
+          for (const run of workflowRuns) {
             const existing = latestByWorkflow.get(run.workflow_id);
-            if (!existing || new Date(run.created_at) > new Date(existing.created_at)) {
+            if (!existing || run.run_number > existing.run_number) {
               latestByWorkflow.set(run.workflow_id, run);
             }
           }
@@ -61,7 +82,7 @@ runs:
 
           if (incompleteRuns.length > 0) {
             const details = incompleteRuns
-              .map((run) => `- ${run.name} (run #${run.run_number}) is still ${run.status}`)
+              .map((run) => `- [${run.name} #${run.run_number}](${run.html_url}) is still ${run.status}`)
               .join('\n');
             core.setFailed(
               [
@@ -74,9 +95,14 @@ runs:
             return;
           }
 
-          // Check for workflows that failed on the previous commit
-          // We treat cancelled runs as failures to be conservative, since they might indicate
-          // timeouts or other infrastructure issues that should be resolved before skipping CI
+          // Check for workflows that failed on the previous commit.
+          // We treat these conclusions as failures:
+          // - 'failure': Obvious failure case
+          // - 'timed_out': Infrastructure or performance issue that should be investigated
+          // - 'cancelled': Conservative - might indicate timeout or manual intervention needed
+          // - 'action_required': Requires manual intervention
+          // We treat 'skipped' and 'neutral' as non-blocking since they indicate
+          // intentional skips or informational-only workflows.
           const failingRuns = Array.from(latestByWorkflow.values()).filter((run) => {
             return ['failure', 'timed_out', 'cancelled', 'action_required'].includes(run.conclusion);
           });
@@ -87,7 +113,7 @@ runs:
           }
 
           const details = failingRuns
-            .map((run) => `- ${run.name} (run #${run.run_number}) concluded ${run.conclusion}`)
+            .map((run) => `- [${run.name} #${run.run_number}](${run.html_url}) concluded ${run.conclusion}`)
             .join('\n');
 
           core.setFailed(

.github/workflows/gem-tests.yml

@@ -86,7 +86,9 @@ jobs:
 
   rspec-package-tests:
     needs: [detect-changes, setup-gem-tests-matrix]
-    # Run on master OR when Ruby tests needed on PR
+    # Skip only if: master push AND docs-only changes
+    # Otherwise run if: on master OR Ruby tests needed
+    # This allows docs-only commits to skip heavy jobs while ensuring full CI on master for code changes
     if: |
       !(
         github.event_name == 'push' &&

.github/workflows/integration-tests.yml

@@ -86,7 +86,9 @@ jobs:
 
   build-dummy-app-webpack-test-bundles:
     needs: [detect-changes, setup-integration-matrix]
-    # Run on master, workflow_dispatch, OR when tests needed on PR
+    # Skip only if: master push AND docs-only changes
+    # Otherwise run if: on master OR workflow_dispatch OR dummy tests needed
+    # This allows docs-only commits to skip heavy jobs while ensuring full CI on master for code changes
     if: |
       !(
         github.event_name == 'push' &&

.github/workflows/lint-js-and-ruby.yml

@@ -66,6 +66,9 @@ jobs:
 
   build:
     needs: detect-changes
+    # Skip only if: master push AND docs-only changes
+    # Otherwise run if: on master OR lint needed
+    # This allows docs-only commits to skip linting while ensuring full CI on master for code changes
     if: |
       !(
         github.event_name == 'push' &&

knip.ts

@@ -148,14 +148,11 @@ const config: KnipConfig = {
         '@rescript/react',
         // The Babel plugin fails to detect it
         'babel-plugin-transform-react-remove-prop-types',
-        // Runtime helpers injected by our Babel config, but not statically analyzable
-        '@babel/runtime',
         // This one is weird. It's long-deprecated and shouldn't be necessary.
         // Probably need to update the Webpack config.
         'node-libs-browser',
         // The below dependencies are not detected by the Webpack plugin
         // due to the config issue.
-        'mini-css-extract-plugin',
         'expose-loader',
         'file-loader',
         'imports-loader',
@@ -165,7 +162,7 @@ const config: KnipConfig = {
         'url-loader',
         // Transitive dependency of shakapacker but listed as direct dependency
         'webpack-merge',
-        // Dependencies not detected in production mode
+        // Dependencies not detected in production mode (runtime injected or dynamic imports)
         '@babel/runtime',
         'mini-css-extract-plugin',
         'css-loader',