Skip to content

Fix actions fetching logic and loading state, prevent duplicate toasts #31124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from
Closed

Fix actions fetching logic and loading state, prevent duplicate toasts #31124

Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3d55932
Fix actions fetching logic and loading state, prevent duplicate toasts
silverwind May 27, 2024
77e6bec
add body variable
silverwind May 27, 2024
60e424d
Update web_src/js/components/RepoActionView.vue
silverwind May 27, 2024
1709eaa
Merge branch 'main' into afetch
silverwind Jun 20, 2024
a5cccf4
Merge branch 'main' into afetch
silverwind Jun 25, 2024
a8695fe
Update web_src/js/modules/toast.js
silverwind Jun 25, 2024
f88e817
remove boolean trap
silverwind Jun 25, 2024
77c4aa2
fix lint
silverwind Jun 25, 2024
f654f12
invert option
silverwind Jun 25, 2024
abfbb59
default preventDuplicates to default, use single key, use it for htmx
silverwind Jun 25, 2024
a0c9cf9
restrict htmx preventDuplicates to only polling elements
silverwind Jun 25, 2024
fe6bbd4
Merge branch 'main' into afetch
GiteaBot Jun 26, 2024
181ffa0
revert toast-related changes
silverwind Jun 26, 2024
18b0285
Update web_src/js/components/RepoActionView.vue
silverwind Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 50 additions & 30 deletions web_src/js/components/RepoActionView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {toggleElem} from '../utils/dom.js';
import {formatDatetime} from '../utils/time.js';
import {renderAnsi} from '../render/ansi.js';
import {GET, POST, DELETE} from '../modules/fetch.js';
import {showErrorToast} from '../modules/toast.js';

const sfc = {
name: 'RepoActionView',
Expand Down Expand Up @@ -87,9 +88,11 @@ const sfc = {

async mounted() {
// load job data and then auto-reload periodically
// need to await first loadJob so this.currentJobStepsStates is initialized and can be used in hashChangeListener
await this.loadJob();
this.intervalID = setInterval(this.loadJob, 1000);
// need to await first loadData so this.currentJobStepsStates is initialized and can be used in hashChangeListener
await this.loadData();
this.intervalID = setInterval(() => {
this.loadData();
}, 1000);
document.body.addEventListener('click', this.closeDropdown);
this.hashChangeListener();
window.addEventListener('hashchange', this.hashChangeListener);
Expand Down Expand Up @@ -142,7 +145,10 @@ const sfc = {
toggleStepLogs(idx) {
this.currentJobStepsStates[idx].expanded = !this.currentJobStepsStates[idx].expanded;
if (this.currentJobStepsStates[idx].expanded) {
this.loadJob(); // try to load the data immediately instead of waiting for next timer interval
this.currentJobStepsStates[idx].loading = true;
// force-load the data, otherwise the state will end up incorrect if loadData
// is already running and the job step will never expand.
this.loadData({force: true});
}
},
// cancel a run
Expand Down Expand Up @@ -206,7 +212,7 @@ const sfc = {
async deleteArtifact(name) {
if (!window.confirm(this.locale.confirmDeleteArtifact.replace('%s', name))) return;
await DELETE(`${this.run.link}/artifacts/${name}`);
await this.loadJob();
await this.loadData();
},

async fetchJob() {
Expand All @@ -222,8 +228,8 @@ const sfc = {
return await resp.json();
},

async loadJob() {
if (this.loading) return;
async loadData({force = false} = {}) {
if (this.loading && !force) return;
Copy link

Choose a reason for hiding this comment

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

In the event that a force request is received, I think that there will be a race condition that may cause a similar problem to the one that you're attempting to solve, occasionally:

There's loadData(A) which was already running, and then loadData(B) which was running with the force flag. If loadData(B) returns from the server first, and then loadData(A)'s data returns from the server afterwards, I believe the log data will be reverted back to the data from the first request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that's why I blocked this PR from merging #31124 (comment)

try {
this.loading = true;

Expand All @@ -235,32 +241,46 @@ const sfc = {
]);
} catch (err) {
if (err instanceof TypeError) return; // avoid network error while unloading page
throw err;
showErrorToast(err.message);
// reset all step loading states, we can't easily tell which one failed at this point
for (let i = 0; i < this.currentJob.steps.length; i++) {
if (this.currentJobStepsStates[i].loading) {
this.currentJobStepsStates[i].loading = false;
}
}
}

this.artifacts = artifacts['artifacts'] || [];

// save the state to Vue data, then the UI will be updated
this.run = job.state.run;
this.currentJob = job.state.currentJob;
if (artifacts) {
this.artifacts = artifacts['artifacts'] || [];
}

// sync the currentJobStepsStates to store the job step states
for (let i = 0; i < this.currentJob.steps.length; i++) {
if (!this.currentJobStepsStates[i]) {
// initial states for job steps
this.currentJobStepsStates[i] = {cursor: null, expanded: false};
if (job) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and above: why it needs new two if here? They are from [job, artifacts] = await Promise.all, and the response shouldn't be null.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the await Promise.all fails, none of these 2 variables could have value.

So it should return early in the catch block above, but not use extra if blocks here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the root problem is that the state management in this loadData/loadJob is quite messy (because after the initial version, then people only add code to "patch" it).

As a bug fix, I think this PR could work. While if we'd like to make the code right, I guess we need to rewrite the code and re-design the state management logic. For example, maybe this.loading doesn't make sense now, we need to track every request's state separately.

(Just some new thoughts)

Copy link
Member Author

@silverwind silverwind Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The ifs are meant to perform a partial state update if one of the two requests fails (which would be caught and show a toast).

Copy link
Member Author

Choose a reason for hiding this comment

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

But I see you are right, Promise.all can not deliver partial results, it either resolves both or rejects with the first encountered error, so my interpretion of the return value was wrong and the ifs are not needed.

// save the state to Vue data, then the UI will be updated
this.run = job.state.run;
this.currentJob = job.state.currentJob;

// sync the currentJobStepsStates to store the job step states
for (let i = 0; i < this.currentJob.steps.length; i++) {
if (!this.currentJobStepsStates[i]) {
// initial states for job steps
this.currentJobStepsStates[i] = {cursor: null, expanded: false, loading: false};
}
}
for (const logs of job.logs.stepsLog) {
// save the cursor, it will be passed to backend next time
this.currentJobStepsStates[logs.step].cursor = logs.cursor;
// append logs to the UI
this.appendLogs(logs.step, logs.lines, logs.started);
// update loading state
if (this.currentJobStepsStates[logs.step].loading && logs.cursor) {
this.currentJobStepsStates[logs.step].loading = false;
}
}
}
// append logs to the UI
for (const logs of job.logs.stepsLog) {
// save the cursor, it will be passed to backend next time
this.currentJobStepsStates[logs.step].cursor = logs.cursor;
this.appendLogs(logs.step, logs.lines, logs.started);
}

if (this.run.done && this.intervalID) {
clearInterval(this.intervalID);
this.intervalID = null;
if (this.run.done && this.intervalID) {
clearInterval(this.intervalID);
this.intervalID = null;
}
}
} finally {
this.loading = false;
Expand Down Expand Up @@ -313,7 +333,7 @@ const sfc = {
this.currentJobStepsStates[step].expanded = true;
// need to await for load job if the step log is loaded for the first time
// so logline can be selected by querySelector
await this.loadJob();
await this.loadData();
}
const logLine = this.$refs.steps.querySelector(selectedLogStep);
if (!logLine) return;
Expand Down Expand Up @@ -479,7 +499,7 @@ export function initRepositoryActionView() {
<!-- If the job is done and the job step log is loaded for the first time, show the loading icon
currentJobStepsStates[i].cursor === null means the log is loaded for the first time
-->
<SvgIcon v-if="isDone(run.status) && currentJobStepsStates[i].expanded && currentJobStepsStates[i].cursor === null" name="octicon-sync" class="tw-mr-2 job-status-rotate"/>
<SvgIcon v-if="isDone(run.status) && currentJobStepsStates[i].expanded && currentJobStepsStates[i].loading" name="octicon-sync" class="tw-mr-2 job-status-rotate"/>
<SvgIcon v-else :name="currentJobStepsStates[i].expanded ? 'octicon-chevron-down': 'octicon-chevron-right'" :class="['tw-mr-2', !isExpandable(jobStep.status) && 'tw-invisible']"/>
<ActionRunStatus :status="jobStep.status" class="tw-mr-2"/>

Expand Down