Skip to content

Conversation

@kaspersv
Copy link
Contributor

@kaspersv kaspersv commented Oct 27, 2025

This PR moves the diff-range computations and associated tests into diff-informed-analysis-utils as preparation for a future PR where the diff-range computation will be performed during the init action instead of the analyze action. The PR is intended to be a pure refactoring, without any observable effects.

The two commits are derived from splitting the first commit from #3192.

Diff-informed analysis is guarded by the diff_informed_queries feature flag. The diff_informed_queries flag is already fully rolled out.

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

  • Advanced setup - Impacts users who have custom workflows.
  • Default setup - Impacts users who use default setup.
  • Code Scanning - Impacts Code Scanning (i.e. analysis-kinds: code-scanning).
  • Code Quality - Impacts Code Quality (i.e. analysis-kinds: code-quality).
  • GHES - Impacts GitHub Enterprise Server.

How did/will you validate this change?

  • Test repository - This change will be tested on a test repository before merging.
  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Feature flags - All new or changed code paths can be fully disabled with corresponding feature flags.
  • Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

  • Telemetry - I rely on existing telemetry or have made changes to the telemetry.
    • Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

@github-actions github-actions bot added the size/XL May be very hard to review label Oct 27, 2025
@kaspersv kaspersv changed the title Kaspersv/extract diff range computation Move diff-range computation into utils Oct 27, 2025
@kaspersv kaspersv requested a review from mbg October 27, 2025 11:44
@kaspersv kaspersv marked this pull request as ready for review October 27, 2025 11:44
@kaspersv kaspersv requested a review from a team as a code owner October 27, 2025 11:44
Copilot AI review requested due to automatic review settings October 27, 2025 11:44
Copy link
Contributor

Copilot AI left a 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 PR refactors diff-informed analysis functionality by moving diff-range computation logic and associated tests from analyze.ts to diff-informed-analysis-utils.ts. The changes prepare for future work where diff-range computation will occur during the init action instead of the analyze action.

  • Moved getPullRequestEditedDiffRanges, getFileDiffsWithBasehead, getDiffRanges functions and FileDiff interface from analyze.ts to diff-informed-analysis-utils.ts
  • Relocated corresponding test cases from analyze.test.ts to diff-informed-analysis-utils.test.ts
  • Updated imports and exports to reflect the new module structure

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/diff-informed-analysis-utils.ts Added diff-range computation functions and FileDiff interface previously in analyze.ts
src/diff-informed-analysis-utils.test.ts Added test cases for getDiffRanges function moved from analyze.test.ts
src/analyze.ts Removed diff-range computation functions, updated imports to use new exports from diff-informed-analysis-utils
src/analyze.test.ts Removed getDiffRanges test cases now in diff-informed-analysis-utils.test.ts
lib/analyze-action.js Generated JavaScript reflecting the TypeScript changes

Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

This change looks OK. I am wondering if it would be better to move this to somewhere like api-client.ts though, since there isn't really anything specific to diff-informed queries.

// uses forward slashes as the path separator, so on Windows we need to
// replace any backslashes with forward slashes.
const filename = path
.join(actionsUtil.getRequiredInput("checkout_path"), fileDiff.filename)
Copy link
Member

Choose a reason for hiding this comment

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

I note that this input is specific to the analyze and upload-sarif actions, so this will need some kind of change if you are planning to call it from the init action in a follow-up change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer. I will keep that in mind for the next changes.

@kaspersv
Copy link
Contributor Author

I am wondering if it would be better to move this to somewhere like api-client.ts though, since there isn't really anything specific to diff-informed queries.

I think an argument can be made both ways. It is somewhat specific to diff-informed analysis in that getDiffRanges (and consequently getPullRequestEditedDiffRanges) is designed to return the information needed specifically for diff-informed analysis rather than all the information returned by compareCommitsWithBasehead. In particular, diff-informed analysis currently works at a file-level granularity and therefore the functions don't return any column information even though it could be extracted.

I am happy to move the functions to api-client.ts if you prefer.

@mbg
Copy link
Member

mbg commented Oct 27, 2025

No strong feelings. I am happy that some parts are specific to diff-informed analysis and, while there might be parts we could extract for more general usage, we can always do that if/when we actually need them for something else. Feel free to merge this as-is.

@kaspersv kaspersv merged commit 8d77149 into main Oct 27, 2025
481 checks passed
@kaspersv kaspersv deleted the kaspersv/extract-diff-range-computation branch October 27, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL May be very hard to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants