Refactor gr-checks-fix-preview to use gr-suggestion-diff-preview
To avoid duplication and having consistency gr-suggestion-diff-preview
is modified to not use comment but more general path, patchset
and optional commentId
Google-Bug-Id: b/360288262
Release-Notes: skip
Change-Id: Ia37fd467936880f52b210c262c8fe2d8ab53867f
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-fix-preview.ts b/polygerrit-ui/app/elements/checks/gr-checks-fix-preview.ts
index 1780839..1c8be76 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-fix-preview.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-fix-preview.ts
@@ -3,23 +3,12 @@
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import '../../embed/diff/gr-diff/gr-diff';
-import {css, html, LitElement, nothing, PropertyValues} from 'lit';
-import {customElement, property, state} from 'lit/decorators.js';
-import {getAppContext} from '../../services/app-context';
-import {EDIT, BasePatchSetNum, RepoName} from '../../types/common';
-import {anyLineTooLong} from '../../utils/diff-util';
-import {Timing} from '../../constants/reporting';
-import {
- DiffInfo,
- DiffLayer,
- DiffPreferencesInfo,
- DiffViewMode,
- RenderPreferences,
-} from '../../api/diff';
-import {GrSyntaxLayerWorker} from '../../embed/diff/gr-syntax-layer/gr-syntax-layer-worker';
+import '../shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview';
+import {GrSuggestionDiffPreview} from '../shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview';
+import {css, html, LitElement, nothing} from 'lit';
+import {customElement, query, property, state} from 'lit/decorators.js';
+import {BasePatchSetNum, RepoName} from '../../types/common';
import {resolve} from '../../models/dependency';
-import {highlightServiceToken} from '../../services/highlight/highlight-service';
import {
FixSuggestionInfo,
NumericChangeId,
@@ -27,28 +16,13 @@
} from '../../api/rest-api';
import {changeModelToken} from '../../models/change/change-model';
import {subscribe} from '../lit/subscription-controller';
-import {DiffPreview} from '../diff/gr-apply-fix-dialog/gr-apply-fix-dialog';
-import {userModelToken} from '../../models/user/user-model';
-import {navigationToken} from '../core/gr-navigation/gr-navigation';
import {fire} from '../../utils/event-util';
-import {createChangeUrl} from '../../models/views/change';
import {OpenFixPreviewEventDetail} from '../../types/events';
/**
- * This component renders a <gr-diff> and an "apply fix" button and can be used
- * when showing check results that have a fix for an easy preview and a quick
- * apply-fix experience.
- *
- * There is a certain overlap with similar components for comment fixes:
- * GrSuggestionDiffPreview also renders a <gr-diff> and fetches a diff preview,
- * it relies on a `comment` (and the comment model) to be available. It supports
- * both a `string` fix and `FixSuggestionInfo`. It also differs in logging and
- * event handling. And it misses the header component that we need for the
- * buttons.
- *
- * There is also `GrUserSuggestionsFix` which wraps `GrSuggestionDiffPreview`
- * and has the header that we also need. But it is very targeted to be used for
- * user suggestions and inside comments.
+ * There is a certain overlap with `GrUserSuggestionsFix` which wraps
+ * `GrSuggestionDiffPreview` and has the header that we also need.
+ * But it is very targeted to be used for user suggestions and inside comments.
*
* So there is certainly an opportunity for cleanup and unification, but at the
* time of component creation it did not feel wortwhile investing into this
@@ -56,6 +30,9 @@
*/
@customElement('gr-checks-fix-preview')
export class GrChecksFixPreview extends LitElement {
+ @query('gr-suggestion-diff-preview')
+ suggestionDiffPreview?: GrSuggestionDiffPreview;
+
@property({type: Object})
fixSuggestionInfo?: FixSuggestionInfo;
@@ -63,9 +40,6 @@
patchSet?: PatchSetNumber;
@state()
- layers: DiffLayer[] = [];
-
- @state()
repo?: RepoName;
@state()
@@ -74,37 +48,13 @@
@state()
latestPatchNum?: PatchSetNumber;
- @state()
- diff?: DiffPreview;
+ @state() previewLoaded = false;
@state()
applyingFix = false;
- @state()
- diffPrefs?: DiffPreferencesInfo;
-
- @state()
- renderPrefs: RenderPreferences = {
- disable_context_control_buttons: true,
- show_file_comment_button: false,
- hide_line_length_indicator: true,
- };
-
- private readonly reporting = getAppContext().reportingService;
-
- private readonly restApiService = getAppContext().restApiService;
-
private readonly getChangeModel = resolve(this, changeModelToken);
- private readonly getUserModel = resolve(this, userModelToken);
-
- private readonly getNavigation = resolve(this, navigationToken);
-
- private readonly syntaxLayer = new GrSyntaxLayerWorker(
- resolve(this, highlightServiceToken),
- () => getAppContext().reportingService
- );
-
constructor() {
super();
subscribe(
@@ -119,15 +69,6 @@
);
subscribe(
this,
- () => this.getUserModel().diffPreferences$,
- diffPreferences => {
- if (!diffPreferences) return;
- this.diffPrefs = diffPreferences;
- this.syntaxLayer.setEnabled(!!this.diffPrefs.syntax_highlighting);
- }
- );
- subscribe(
- this,
() => this.getChangeModel().repo$,
x => (this.repo = x)
);
@@ -150,11 +91,6 @@
.header .title {
flex: 1;
}
- .diff-container {
- border: 1px solid var(--border-color);
- border-top: none;
- border-bottom: none;
- }
.loading {
border: 1px solid var(--border-color);
padding: var(--spacing-xl);
@@ -163,12 +99,6 @@
];
}
- override willUpdate(changed: PropertyValues) {
- if (changed.has('fixSuggestionInfo')) {
- this.fetchDiffPreview().then(diff => (this.diff = diff));
- }
- }
-
override render() {
if (!this.fixSuggestionInfo) return nothing;
return html`${this.renderHeader()}${this.renderDiff()}`;
@@ -185,7 +115,7 @@
class="showFix"
secondary
flatten
- .disabled=${!this.diff}
+ .disabled=${!this.previewLoaded}
@click=${this.showFix}
>
Show fix side-by-side
@@ -207,48 +137,16 @@
}
private renderDiff() {
- if (!this.diff) {
- return html`<div class="loading">Loading fix preview ...</div>`;
- }
- const diff = this.diff.preview;
- if (!anyLineTooLong(diff)) {
- this.syntaxLayer.process(diff);
- }
return html`
- <div class="diff-container">
- <gr-diff
- .prefs=${this.getDiffPrefs()}
- .path=${this.diff.filepath}
- .diff=${diff}
- .layers=${this.layers}
- .renderPrefs=${this.renderPrefs}
- .viewMode=${DiffViewMode.UNIFIED}
- ></gr-diff>
- </div>
+ <gr-suggestion-diff-preview
+ .fixSuggestionInfo=${this.fixSuggestionInfo}
+ .patchSet=${this.patchSet}
+ .codeText=${'Loading fix preview ...'}
+ @preview-loaded=${() => (this.previewLoaded = true)}
+ ></gr-suggestion-diff-preview>
`;
}
- /**
- * Calls the REST API to convert the fix into a DiffInfo.
- */
- private async fetchDiffPreview(): Promise<DiffPreview | undefined> {
- if (!this.changeNum || !this.patchSet || !this.fixSuggestionInfo) return;
- const pathsToDiffs: {[path: string]: DiffInfo} | undefined =
- await this.restApiService.getFixPreview(
- this.changeNum,
- this.patchSet,
- this.fixSuggestionInfo.replacements
- );
-
- if (!pathsToDiffs) return;
- const diffs = Object.keys(pathsToDiffs).map(filepath => {
- const diff = pathsToDiffs[filepath];
- return {filepath, preview: diff};
- });
- // Showing diff for one file only.
- return diffs?.[0];
- }
-
private showFix() {
if (!this.patchSet || !this.fixSuggestionInfo) return;
const eventDetail: OpenFixPreviewEventDetail = {
@@ -268,61 +166,21 @@
if (!changeNum || !basePatchNum || !this.fixSuggestionInfo) return;
this.applyingFix = true;
- this.reporting.time(Timing.APPLY_FIX_LOAD);
- const res = await this.restApiService.applyFixSuggestion(
- changeNum,
- basePatchNum,
- this.fixSuggestionInfo.replacements,
- this.latestPatchNum
- );
- this.applyingFix = false;
- this.reporting.timeEnd(Timing.APPLY_FIX_LOAD, {
- method: '1-click',
- description: this.fixSuggestionInfo.description,
- });
- if (res?.ok) this.navigateToEditPatchset();
- }
-
- private navigateToEditPatchset() {
- const changeNum = this.changeNum;
- const repo = this.repo;
- const basePatchNum = this.patchSet;
- if (!changeNum || !repo || !basePatchNum) return;
-
- const url = createChangeUrl({
- changeNum,
- repo,
- patchNum: EDIT,
- basePatchNum,
- // We have to force reload, because the EDIT patchset is otherwise not yet known.
- forceReload: true,
- });
- this.getNavigation().setUrl(url);
- }
-
- /**
- * We have to override some diff prefs of the user, because for example in the context of showing
- * an inline diff for fixes we do not want to show context lines around the changes lines of code
- * as we would normally do for a diff.
- */
- private getDiffPrefs() {
- if (!this.diffPrefs) return undefined;
- return {
- ...this.diffPrefs,
- context: 0,
- line_length: Math.min(this.diffPrefs.line_length, 100),
- line_wrapping: true,
- };
+ try {
+ await this.suggestionDiffPreview?.applyFix();
+ } finally {
+ this.applyingFix = false;
+ }
}
private isApplyEditDisabled() {
if (this.patchSet === undefined) return true;
- return !this.diff;
+ return !this.previewLoaded;
}
private computeApplyFixTooltip() {
if (this.patchSet === undefined) return '';
- if (!this.diff) return 'Fix is still loading ...';
+ if (!this.previewLoaded) return 'Fix is still loading ...';
return '';
}
}
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-fix-preview_test.ts b/polygerrit-ui/app/elements/checks/gr-checks-fix-preview_test.ts
index 8a9997a..7dbac27 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-fix-preview_test.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-fix-preview_test.ts
@@ -6,9 +6,10 @@
import * as sinon from 'sinon';
import '../../test/common-test-setup';
import './gr-checks-results';
+import './gr-checks-fix-preview';
import {html} from 'lit';
import {fixture, assert} from '@open-wc/testing';
-import {createCheckFix, createDiff} from '../../test/test-data-generators';
+import {createCheckFix} from '../../test/test-data-generators';
import {GrChecksFixPreview} from './gr-checks-fix-preview';
import {rectifyFix} from '../../models/checks/checks-util';
import {
@@ -16,12 +17,10 @@
mockPromise,
queryAndAssert,
stubRestApi,
- waitUntil,
} from '../../test/test-utils';
import {NumericChangeId, PatchSetNumber, RepoName} from '../../api/rest-api';
import {FilePathToDiffInfoMap} from '../../types/common';
-import {testResolver} from '../../test/common-test-setup';
-import {navigationToken} from '../core/gr-navigation/gr-navigation';
+import {GrSuggestionDiffPreview} from '../shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview';
suite('gr-checks-fix-preview test', () => {
let element: GrChecksFixPreview;
@@ -45,11 +44,6 @@
await element.updateComplete;
});
- const loadDiff = async () => {
- promise.resolve({'foo.c': createDiff()});
- await waitUntil(() => !!element.diff);
- };
-
test('renders loading', async () => {
assert.shadowDom.equal(
element,
@@ -84,58 +78,14 @@
</gr-button>
</div>
</div>
- <div class="loading">Loading fix preview ...</div>
- `
- );
- });
-
- test('renders diff', async () => {
- await loadDiff();
- assert.shadowDom.equal(
- element,
- /* HTML */ `
- <div class="header">
- <div class="title">
- <span> Attached Fix </span>
- </div>
- <div>
- <gr-button
- class="showFix"
- aria-disabled="false"
- flatten=""
- role="button"
- secondary=""
- tabindex="0"
- >
- Show fix side-by-side
- </gr-button>
- <gr-button
- class="applyFix"
- aria-disabled="false"
- flatten=""
- primary=""
- role="button"
- tabindex="0"
- title=""
- >
- Apply fix
- </gr-button>
- </div>
- </div>
- <div class="diff-container">
- <gr-diff
- class="disable-context-control-buttons hide-line-length-indicator"
- style="--line-limit-marker: 100ch; --content-width: none; --diff-max-width: none; --font-size: 12px;"
- >
- </gr-diff>
- </div>
+ <gr-suggestion-diff-preview></gr-suggestion-diff-preview>
`
);
});
test('show-fix', async () => {
- await loadDiff();
-
+ element.previewLoaded = true;
+ await element.updateComplete;
const stub = sinon.stub();
element.addEventListener('open-fix-preview', stub);
@@ -152,9 +102,13 @@
});
test('apply-fix', async () => {
- await loadDiff();
-
- const setUrlSpy = sinon.stub(testResolver(navigationToken), 'setUrl');
+ element.previewLoaded = true;
+ await element.updateComplete;
+ const diffPreview = queryAndAssert<GrSuggestionDiffPreview>(
+ element,
+ 'gr-suggestion-diff-preview'
+ );
+ const applyFixSpy = sinon.spy(diffPreview, 'applyFix');
stubRestApi('applyFixSuggestion').returns(
Promise.resolve({ok: true} as Response)
);
@@ -163,10 +117,6 @@
assert.isFalse(button.hasAttribute('disabled'));
button.click();
- await waitUntil(() => setUrlSpy.called);
- assert.equal(
- setUrlSpy.lastCall.args[0],
- '/c/test-repo/+/123/5..edit?forceReload=true'
- );
+ assert.isTrue(applyFixSpy.called);
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index ea1c20d..cf5ea9d 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -1179,6 +1179,8 @@
id="suggestionDiffPreview"
.uuid=${this.generatedSuggestionId}
.fixSuggestionInfo=${this.generatedFixSuggestion}
+ .patchSet=${this.comment?.patch_set}
+ .commentId=${this.comment?.id}
></gr-suggestion-diff-preview>`;
} else {
return nothing;
diff --git a/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts b/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
index a70ca07..137c853 100644
--- a/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
+++ b/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
@@ -214,6 +214,8 @@
</div>
<gr-suggestion-diff-preview
.fixSuggestionInfo=${this.comment?.fix_suggestions?.[0]}
+ .patchSet=${this.comment?.patch_set}
+ .commentId=${this.comment?.id}
@preview-loaded=${() => (this.previewLoaded = true)}
></gr-suggestion-diff-preview>`;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
index 9d8b327..136979d 100644
--- a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
+++ b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
@@ -8,7 +8,6 @@
import {customElement, property, state} from 'lit/decorators.js';
import {getAppContext} from '../../../services/app-context';
import {
- Comment,
EDIT,
BasePatchSetNum,
PatchSetNumber,
@@ -30,7 +29,6 @@
import {subscribe} from '../../lit/subscription-controller';
import {DiffPreview} from '../../diff/gr-apply-fix-dialog/gr-apply-fix-dialog';
import {userModelToken} from '../../../models/user/user-model';
-import {commentModelToken} from '../gr-comment-model/gr-comment-model';
import {navigationToken} from '../../core/gr-navigation/gr-navigation';
import {fire} from '../../../utils/event-util';
import {Timing} from '../../../constants/reporting';
@@ -58,6 +56,8 @@
@property({type: Object})
fixSuggestionInfo?: FixSuggestionInfo;
+ // Used to determine if the preview has been loaded
+ // this is identical to previewLoadedFor !== undefined and can be removed
@property({type: Boolean, attribute: 'previewed', reflect: true})
previewed = false;
@@ -65,8 +65,12 @@
@property({type: String})
uuid?: string;
- @state()
- comment?: Comment;
+ @property({type: Number})
+ patchSet?: BasePatchSetNum;
+
+ // Optional. Used in logging.
+ @property({type: String})
+ commentId?: string;
@state()
layers: DiffLayer[] = [];
@@ -78,7 +82,7 @@
* fix suggestion info currently in gr-comment.
*/
@state()
- public previewLoadedFor?: string | FixSuggestionInfo;
+ public previewLoadedFor?: FixSuggestionInfo;
@state() repo?: RepoName;
@@ -110,8 +114,6 @@
private readonly getUserModel = resolve(this, userModelToken);
- private readonly getCommentModel = resolve(this, commentModelToken);
-
private readonly getNavigation = resolve(this, navigationToken);
private readonly syntaxLayer = new GrSyntaxLayerWorker(
@@ -128,14 +130,6 @@
);
subscribe(
this,
- () => this.getChangeModel().revisions$,
- revisions =>
- (this.hasEdit = Object.values(revisions).some(
- info => info._number === EDIT
- ))
- );
- subscribe(
- this,
() => this.getChangeModel().latestPatchNum$,
x => (this.latestPatchNum = x)
);
@@ -150,11 +144,6 @@
);
subscribe(
this,
- () => this.getCommentModel().comment$,
- comment => (this.comment = comment)
- );
- subscribe(
- this,
() => this.getChangeModel().repo$,
x => (this.repo = x)
);
@@ -198,11 +187,9 @@
if (
changed.has('fixSuggestionInfo') ||
changed.has('changeNum') ||
- changed.has('comment')
+ changed.has('patchSet')
) {
- if (this.previewLoadedFor !== this.fixSuggestionInfo) {
- this.fetchFixPreview();
- }
+ this.fetchFixPreview();
}
}
@@ -236,13 +223,12 @@
}
private async fetchFixPreview() {
- if (!this.changeNum || !this.comment?.patch_set || !this.fixSuggestionInfo)
- return;
+ if (!this.changeNum || !this.patchSet || !this.fixSuggestionInfo) return;
this.reporting.time(Timing.PREVIEW_FIX_LOAD);
const res = await this.restApiService.getFixPreview(
this.changeNum,
- this.comment?.patch_set,
+ this.patchSet,
this.fixSuggestionInfo.replacements
);
if (!res) return;
@@ -251,7 +237,7 @@
});
this.reporting.timeEnd(Timing.PREVIEW_FIX_LOAD, {
uuid: this.uuid,
- commentId: this.comment?.id ?? '',
+ commentId: this.commentId ?? '',
});
if (currentPreviews.length > 0) {
this.preview = currentPreviews[0];
@@ -276,7 +262,7 @@
public async applyFix() {
const changeNum = this.changeNum;
- const basePatchNum = this.comment?.patch_set as BasePatchSetNum;
+ const basePatchNum = this.patchSet;
const fixSuggestion = this.fixSuggestionInfo;
if (!changeNum || !basePatchNum || !fixSuggestion) return;
@@ -293,7 +279,7 @@
fileExtension: getFileExtension(
fixSuggestion?.replacements?.[0].path ?? ''
),
- commentId: this.comment?.id ?? '',
+ commentId: this.commentId ?? '',
});
if (res?.ok) {
this.getNavigation().setUrl(
@@ -305,7 +291,7 @@
forceReload: !this.hasEdit,
})
);
- fire(this, 'reload-diff', {path: this.comment?.path});
+ fire(this, 'reload-diff', {path: fixSuggestion.replacements[0].path});
fire(this, 'apply-user-suggestion', {});
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
index 39d937d..593d0a6 100644
--- a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
@@ -52,10 +52,9 @@
test('render diff', async () => {
stubFlags('isEnabled').returns(true);
+ element.previewLoadedFor = createFixSuggestionInfo();
element.codeText =
' private handleClick(e: MouseEvent) {\ne.stopPropagation();\ne.preventDefault();';
- element.previewLoadedFor =
- ' private handleClick(e: MouseEvent) {\ne.stopPropagation();\ne.preventDefault();';
element.preview = {
filepath:
'polygerrit-ui/app/elements/change/gr-change-summary/gr-summary-chip_test.ts',
diff --git a/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix.ts b/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix.ts
index 3c3d52e..42a2434 100644
--- a/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix.ts
+++ b/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix.ts
@@ -152,6 +152,8 @@
</div>
</div>
<gr-suggestion-diff-preview
+ .patchSet=${this.comment?.patch_set}
+ .commentId=${this.comment?.id}
.fixSuggestionInfo=${fixSuggestions[0]}
.codeText=${code}
@preview-loaded=${() => (this.previewLoaded = true)}
