[amnkarn]

Fixes #1693

Description

This PR refactors getDiff and related consumers to store and access the git diff directly on the action object (action.diff).This makes it easier for subsequent actions (like scanDiff) to retrieve the diff without extracting it from the step content.

Changes Made

• Backend: Updated getDiff to assign the diff string directly to action.diff and updated the Action interface.
• Processors: Updated scanDiff.ts to retrieve the diff from action.diff.
• UI: Updated PushActionView in types.ts to accept Step | string for the diff property.
• UI Services: Modified getPush in git-push.ts to handle the new string format.

Backward Compatibility

To ensure historical database records and existing UI components do not break, fallback logic has been added. If action.diff is not present (or is not a string), the system gracefully falls back to extracting the content from steps.find((s) => s.stepName === 'diff')?.content in both scanDiff.ts and git-push.ts.

Storage note

action.diff adds a third copy of the diff to each push document, alongside steps[].content and steps[].logs. Removing setContent is deferred to a follow-up under #1691.

[netlify]

✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name	Link
🔨 Latest commit	5e5c33a
🔍 Latest deploy log	https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6ad03e020d3c2de6c2f4a899
😎 Deploy Preview	https://deploy-preview-1701.git-proxy.preview.finos.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: amnkarn / name: Aman Karn (6a75417)
• ✅ login: amnkarn / name: Aman Kumar Karn (ed30c23)

[dcoric]

Thanks for picking this up @amnkarn, I verified the consumer coverage end-to-end (scanDiff, getPush, PushDetails, Cypress mocks) and the legacy fallbacks keep old DB records working, so the compat approach is right. Requesting changes on one compile-level issue plus a few design/test follow-ups:

• PushActionView type breaks tsc (inline, blocking)
• diff now persisted 3x per push (inline)
• two test gaps (inline)
• plugin access expectation (inline)

Also: the -40 lines in package-lock.json (removal of "extraneous": true entries) look like an unrelated npm-prune side effect from a local install. Could you revert them or move them to a separate PR so the diff stays clean?

[amnkarn]

Thanks for the thorough review @dcoric!

I have addressed all the feedback:

1. **`PushActionView` interface fix**: Updated `PushActionView`

to Omit<Action, ActionMethods | 'diff'> with diff?: string | Step;. npm run check-types passes clean now.
2. Strict equality: Updated typeof data.diff === 'string'
in git-push.ts.
3. Precedence test: Added unit test in
test/processors/scanDiff.test.ts ensuring action.diff takes
precedence over step content if both exist.
4. getPush test coverage: Added unit tests in
test/ui/git-push.test.ts covering top-level string diff and
empty string diff.
5. Cleaned package-lock.json: Reverted the unrelated
package-lock.json changes.
6. Storage/Audit log note & Plugin scope: Noted in the PR
description that step.setContent(diff) will be evaluated for
complete removal in a follow-up once full legacy DB record
migration/deprecation is done, and clarified that plugin benefit
materialization will occur under #1683 once post-diff plugin
positioning is in place.

[fabiovincenzi]

@amnkarn The lockfile changes still look present and also test/fixtures/test-package/package-lock.json. Maybe the revert didn't get committed?

This would also resolve the conflicts.

[fabiovincenzi]

@dcoric this should be ready now. Would you mind taking another look?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.00%. Comparing base (a1e7993) to head (5e5c33a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1701      +/-   ##
==========================================
+ Coverage   86.89%   87.00%   +0.11%     
==========================================
  Files         113      113              
  Lines        6461     6463       +2     
  Branches     1253     1253              
==========================================
+ Hits         5614     5623       +9     
+ Misses        542      536       -6     
+ Partials      305      304       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jescalada]

I feel like this PR is adding complexity (especially with types) when the original goal was to make the diff available, and safe for plugin use.

Can we refactor this so it fits the goal, while being as clean, safe and succinct as possible? AI is very eager to implement exactly what the issue requested without any regard for the maintainability of the code or the appropriateness of the solution 😕

[fabiovincenzi]

@jescalada I left the scanDiff log alone since #1795 already replaces it.

Since nothing reads the step content any more we might want to drop it here?

[fabiovincenzi]

Removed step.setContent @jescalada

[jescalada]

Might need another approval from @dcoric before merging

[dcoric]

LGTM