Fix "ref-issue" handling in markup #35739

wxiaoguang · 2025-10-24T06:27:33Z

This is a follow up for #35662, and also fix #31181, help #30275, fix #31161

wxiaoguang · 2025-10-24T06:28:08Z

The "loading" state is still useful.

When I use GitHub, when my network lags, there is no UI response (no loading) when I hover the issue links, it looks unclear to me.

Yakov5776 · 2025-10-24T20:54:12Z

The "loading" state is still useful.

When I use GitHub, when my network lags, there is no UI response (no loading) when I hover the issue links, it looks unclear to me.

GitHub doesn't highlight the issue if it doesn't exist though, so how does it look unclear?

As seen here:
Yakov5776/symlink-tests#1

wxiaoguang · 2025-10-25T01:07:40Z

The "loading" state is still useful.
When I use GitHub, when my network lags, there is no UI response (no loading) when I hover the issue links, it looks unclear to me.

GitHub doesn't highlight the issue if it doesn't exist though, so how does it look unclear?

I mean when my network lags, I didn't change the "issue not exist" case, it can be optimized separately.

silverwind · 2025-10-27T10:31:15Z

Can the fetch inside ContextPopup.vue be moved outside? I still prefer to use dumb "render only" components where possible, it was the original idea in #31181.

web_src/js/components/ContextPopup.vue

silverwind · 2025-10-27T10:34:51Z

web_src/js/components/ContextPopup.vue

+    <div v-else-if="issue" class="tw-flex tw-flex-col tw-gap-2">
+      <div class="tw-text-12">
+        <a :href="repoLink" class="muted">{{ issue.repository.full_name }}</a>
+        on {{ createdAt }}


on needs translation ideally.

Agree, ideally.

But I think it doesn't need to be in this PR's scope.

Handling i18n with datetime is also tricky, especially here it is done on frontend and mixes HTML elements.

Leave it to the future (and I don't see anyone complains)

I think we dealt with on before related to relative-time-element, but it's probably all backend rendering there.

Nope, relative-time-element has dropped the "on" feature, it is non-translatable.

wxiaoguang · 2025-10-27T10:45:30Z

Can the fetch inside ContextPopup.vue be moved outside? I still prefer to use dumb "render only" components where possible, it was the original idea in #31181.

I don't think so. If you move it out, then the "loading" indicator state will be more complex. I believe the "loading" indicator is useful. And since we haven't resolved the "non-existing issue" problem, the "not found" error is also handled there.

I think the current approach is good enough

silverwind · 2025-10-27T11:29:59Z

If you move it out, then the "loading" indicator state will be more complex

Could just pass a loading: boolean prop to the component, but I guess it's not a big issue.

wxiaoguang · 2025-10-27T11:48:28Z

If you move it out, then the "loading" indicator state will be more complex

Could just pass a loading: boolean prop to the component, but I guess it's not a big issue.

Then how could you switch the "loading" prop? Vue doesn't work that way.

silverwind · 2025-10-27T13:20:40Z

Doesn't vue re-render when props change? At least that's how it works in React.

wxiaoguang · 2025-10-27T13:25:41Z

Doesn't vue re-render when props change? At least that's how it works in React.

If they are all in the reactive context, yes.

But here, the component is mounted from JS by a non-reactive props object. I don't think React works in this case either.

wxiaoguang · 2025-10-27T13:28:47Z

silverwind · 2025-10-27T13:35:24Z

Ah yes if you create a whole vue app or react root, then those props won't be reactive. The auto-update behaviour only works when a component renders another component (which is the typical case in a SPA).

wxiaoguang · 2025-10-27T13:39:41Z

Any other questions?

I think this PR is good enough, it resolves many legacy problems, the code is more maintainable, nothing becomes worse.

Although there could be some improvements, but none of them is easy to implement, so the improvements can be left to the future until someone really needs them and can use some right approaches to implement them.

silverwind · 2025-10-27T13:40:04Z

web_src/js/modules/tippy.ts

+
+export function getAttachedTippyInstance(el: Element): Instance | null {
+  return el._tippy ?? null;
+}


Instead of this, can you adjust globals.d.ts to add undefined to the property:

interface Element { _tippy: import('tippy.js').Instance | undefined; }

And then just use el._tippy, typescript will then take care that the undefined case is handled.

It is very ugly.

I believe in we need to encapsulate the modules, but don't let caller use fragile code.

el._tippy is very fragile, it totally depends the undocumented 3rd party library behavior, should never be widely used.

If you don't want direct el._tippy access, that's fine, but then these 2 cases should also use this function:

web_src/js/features/common-page.ts:62: queryElems(elDropdown, '.menu > .item', (el) => el._tippy?.hide()); web_src/js/features/repo-issue-list.ts:189: el._tippy.destroy();

If you don't want direct el._tippy access, that's fine, but then these 2 cases should also use this function:

web_src/js/features/common-page.ts:62: queryElems(elDropdown, '.menu > .item', (el) => el._tippy?.hide()); web_src/js/features/repo-issue-list.ts:189: el._tippy.destroy();

There are far more, these usages can be refactored later, not in this PR's scope.

TBH, I find the typescript solution better, it does not necessitate such big refactors and is equally safe.

A variable starting with underscore already means it is for internal usage only, it is a widely accepted agreement across many different languages.

Using a variable with underscore prefix really doesn't seem good.

It's a practical solution in case of tippy, I agree in a ideal world, properties should not be added to Element at all.

We should replace tippy (which is deprecated) with https://github.com/floating-ui/floating-ui.

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 24, 2025

github-actions bot added modifies/templates This PR modifies the template files modifies/frontend labels Oct 24, 2025

wxiaoguang mentioned this pull request Oct 24, 2025

Make ContextPopup stateless, improve fetching logic #31181

Closed

fix

c2af838

wxiaoguang force-pushed the fix-refissue branch from 87ad04a to c2af838 Compare October 24, 2025 06:34

wxiaoguang mentioned this pull request Oct 24, 2025

Nonexistent issues get linked in commit messages and comments #30275

Open

wxiaoguang added the type/bug label Oct 24, 2025

lunny approved these changes Oct 24, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 24, 2025

lunny added this to the 1.26.0 milestone Oct 24, 2025

silverwind reviewed Oct 27, 2025

View reviewed changes

web_src/js/components/ContextPopup.vue Show resolved Hide resolved

silverwind reviewed Oct 27, 2025

View reviewed changes

silverwind approved these changes Oct 27, 2025

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 27, 2025

Merge branch 'main' into fix-refissue

ce2491f

wxiaoguang merged commit cddff73 into go-gitea:main Oct 27, 2025
26 checks passed

wxiaoguang deleted the fix-refissue branch October 27, 2025 14:47

Uh oh!

Uh oh!

Fix "ref-issue" handling in markup #35739

Fix "ref-issue" handling in markup #35739

Conversation

wxiaoguang commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wxiaoguang commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yakov5776 commented Oct 24, 2025

Uh oh!

wxiaoguang commented Oct 25, 2025

Uh oh!

silverwind commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

silverwind Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Oct 27, 2025

Uh oh!

wxiaoguang commented Oct 27, 2025

Uh oh!

silverwind commented Oct 27, 2025

Uh oh!

wxiaoguang commented Oct 27, 2025

Uh oh!

wxiaoguang commented Oct 27, 2025

Uh oh!

silverwind commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wxiaoguang commented Oct 27, 2025

Uh oh!

silverwind Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wxiaoguang commented Oct 24, 2025 •

edited

Loading

wxiaoguang commented Oct 24, 2025 •

edited

Loading

silverwind commented Oct 27, 2025 •

edited

Loading

silverwind Oct 27, 2025 •

edited

Loading

wxiaoguang commented Oct 27, 2025 •

edited

Loading

silverwind commented Oct 27, 2025 •

edited

Loading

silverwind Oct 27, 2025 •

edited

Loading

silverwind Oct 27, 2025 •

edited

Loading