Skip to content

Fix "ref-issue" handling in markup #35739

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

Merged
merged 2 commits into from
Oct 27, 2025
Merged

Fix "ref-issue" handling in markup #35739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2af838
fix
wxiaoguang Oct 24, 2025
ce2491f
Merge branch 'main' into fix-refissue
wxiaoguang Oct 27, 2025
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
1 change: 0 additions & 1 deletion templates/base/head_script.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ If you introduce mistakes in it, Gitea JavaScript code wouldn't run correctly.
copy_success: {{ctx.Locale.Tr "copy_success"}},
copy_error: {{ctx.Locale.Tr "copy_error"}},
error_occurred: {{ctx.Locale.Tr "error.occurred"}},
network_error: {{ctx.Locale.Tr "error.network_error"}},
remove_label_str: {{ctx.Locale.Tr "remove_label_str"}},
modal_confirm: {{ctx.Locale.Tr "modal.confirm"}},
modal_cancel: {{ctx.Locale.Tr "modal.cancel"}},
Expand Down
62 changes: 26 additions & 36 deletions web_src/js/components/ContextPopup.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,62 +2,53 @@
import {SvgIcon} from '../svg.ts';
import {GET} from '../modules/fetch.ts';
import {getIssueColor, getIssueIcon} from '../features/issue.ts';
import {computed, onMounted, shallowRef, useTemplateRef} from 'vue';
import type {IssuePathInfo} from '../types.ts';
import {computed, onMounted, shallowRef} from 'vue';

const {appSubUrl, i18n} = window.config;
const props = defineProps<{
repoLink: string,
loadIssueInfoUrl: string,
}>();

const loading = shallowRef(false);
const issue = shallowRef(null);
const renderedLabels = shallowRef('');
const i18nErrorOccurred = i18n.error_occurred;
const i18nErrorMessage = shallowRef(null);
const errorMessage = shallowRef(null);

const createdAt = computed(() => new Date(issue.value.created_at).toLocaleDateString(undefined, {year: 'numeric', month: 'short', day: 'numeric'}));
const body = computed(() => {
const body = issue.value.body.replace(/\n+/g, ' ');
if (body.length > 85) {
return `${body.substring(0, 85)}…`;
}
return body;
const createdAt = computed(() => {
return new Date(issue.value.created_at).toLocaleDateString(undefined, {year: 'numeric', month: 'short', day: 'numeric'});
});

const root = useTemplateRef('root');

onMounted(() => {
root.value.addEventListener('ce-load-context-popup', (e: CustomEventInit<IssuePathInfo>) => {
if (!loading.value && issue.value === null) {
load(e.detail);
}
});
const body = computed(() => {
const body = issue.value.body.replace(/\n+/g, ' ');
return body.length > 85 ? `${body.substring(0, 85)}…` : body;
});

async function load(issuePathInfo: IssuePathInfo) {
onMounted(async () => {
loading.value = true;
i18nErrorMessage.value = null;

errorMessage.value = null;
try {
const response = await GET(`${appSubUrl}/${issuePathInfo.ownerName}/${issuePathInfo.repoName}/issues/${issuePathInfo.indexString}/info`); // backend: GetIssueInfo
const respJson = await response.json();
if (!response.ok) {
i18nErrorMessage.value = respJson.message ?? i18n.network_error;
const resp = await GET(props.loadIssueInfoUrl);
if (!resp.ok) {
errorMessage.value = resp.status ? resp.statusText : 'Unknown network error';
return;
}
const respJson = await resp.json();
issue.value = respJson.convertedIssue;
renderedLabels.value = respJson.renderedLabels;
} catch {
i18nErrorMessage.value = i18n.network_error;
} finally {
loading.value = false;
}
}
});
</script>

<template>
<div ref="root">
<div class="tw-p-4">
<div v-if="loading" class="tw-h-12 tw-w-12 is-loading"/>
<div v-if="!loading && issue !== null" class="tw-flex tw-flex-col tw-gap-2">
<div class="tw-text-12">{{ issue.repository.full_name }} on {{ createdAt }}</div>
<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 }}
Copy link
Member

@silverwind silverwind Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

on needs translation ideally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

</div>
<div class="flex-text-block">
<svg-icon :name="getIssueIcon(issue)" :class="['text', getIssueColor(issue)]"/>
<span class="issue-title tw-font-semibold tw-break-anywhere">
Expand All @@ -69,9 +60,8 @@ async function load(issuePathInfo: IssuePathInfo) {
<!-- eslint-disable-next-line vue/no-v-html -->
<div v-if="issue.labels.length" v-html="renderedLabels"/>
</div>
<div class="tw-flex tw-flex-col tw-gap-2" v-if="!loading && issue === null">
<div class="tw-text-12">{{ i18nErrorOccurred }}</div>
<div>{{ i18nErrorMessage }}</div>
<div v-else>
{{ errorMessage }}
</div>
</div>
</template>
43 changes: 0 additions & 43 deletions web_src/js/features/contextpopup.ts
View file
Open in desktop

This file was deleted.

4 changes: 1 addition & 3 deletions web_src/js/features/repo-diff.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import {invertFileFolding} from './file-fold.ts';
import {parseDom, sleep} from '../utils.ts';
import {registerGlobalSelectorFunc} from '../modules/observer.ts';

const {i18n} = window.config;

function initRepoDiffFileBox(el: HTMLElement) {
// switch between "rendered" and "source", for image and CSV files
queryElems(el, '.file-view-toggle', (btn) => btn.addEventListener('click', () => {
Expand Down Expand Up @@ -86,7 +84,7 @@ function initRepoDiffConversationForm() {
}
} catch (error) {
console.error('Error:', error);
showErrorToast(i18n.network_error);
showErrorToast(`Submit form failed: ${error}`);
} finally {
form?.classList.remove('is-loading');
}
Expand Down
2 changes: 0 additions & 2 deletions web_src/js/features/repo-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {html, htmlRaw} from '../utils/html.ts';
import {createCodeEditor} from './codeeditor.ts';
import {hideElem, queryElems, showElem, createElementFromHTML} from '../utils/dom.ts';
import {attachRefIssueContextPopup} from './contextpopup.ts';
import {POST} from '../modules/fetch.ts';
import {initDropzone} from './dropzone.ts';
import {confirmModal} from './comp/ConfirmModal.ts';
Expand Down Expand Up @@ -199,5 +198,4 @@ export function initRepoEditor() {
export function renderPreviewPanelContent(previewPanel: Element, htmlContent: string) {
// the content is from the server, so it is safe to use innerHTML
previewPanel.innerHTML = html`<div class="render-content markup">${htmlRaw(htmlContent)}</div>`;
attachRefIssueContextPopup(previewPanel.querySelectorAll('p .ref-issue'));
}
3 changes: 0 additions & 3 deletions web_src/js/features/repo-issue-edit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {getComboMarkdownEditor, initComboMarkdownEditor, ComboMarkdownEditor} fr
import {POST} from '../modules/fetch.ts';
import {showErrorToast} from '../modules/toast.ts';
import {hideElem, querySingleVisibleElem, showElem, type DOMEvent} from '../utils/dom.ts';
import {attachRefIssueContextPopup} from './contextpopup.ts';
import {triggerUploadStateChanged} from './comp/EditorUpload.ts';
import {convertHtmlToMarkdown} from '../markup/html2markdown.ts';
import {applyAreYouSure, reinitializeAreYouSure} from '../vendor/jquery.are-you-sure.ts';
Expand Down Expand Up @@ -62,8 +61,6 @@ async function tryOnEditContent(e: DOMEvent<MouseEvent>) {
renderContent = newRenderContent;

rawContent.textContent = comboMarkdownEditor.value();
const refIssues = renderContent.querySelectorAll<HTMLElement>('p .ref-issue');
attachRefIssueContextPopup(refIssues);

if (!commentContent.querySelector('.dropzone-attachments')) {
if (data.attachments !== '') {
Expand Down
2 changes: 0 additions & 2 deletions web_src/js/index-domready.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import '../../node_modules/easymde/dist/easymde.min.css'; // TODO: lazy load in
import {initHtmx} from './htmx.ts';
import {initDashboardRepoList} from './features/dashboard.ts';
import {initGlobalCopyToClipboardListener} from './features/clipboard.ts';
import {initContextPopups} from './features/contextpopup.ts';
import {initRepoGraphGit} from './features/repo-graph.ts';
import {initHeatmap} from './features/heatmap.ts';
import {initImageDiff} from './features/imagediff.ts';
Expand Down Expand Up @@ -97,7 +96,6 @@ const initPerformanceTracer = callInitFunctions([
initHeadNavbarContentToggle,
initFootLanguageMenu,

initContextPopups,
initHeatmap,
initImageDiff,
initMarkupAnchors,
Expand Down
2 changes: 2 additions & 0 deletions web_src/js/markup/content.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {initMarkupRenderAsciicast} from './asciicast.ts';
import {initMarkupTasklist} from './tasklist.ts';
import {registerGlobalSelectorFunc} from '../modules/observer.ts';
import {initMarkupRenderIframe} from './render-iframe.ts';
import {initMarkupRefIssue} from './refissue.ts';

// code that runs for all markup content
export function initMarkupContent(): void {
Expand All @@ -15,5 +16,6 @@ export function initMarkupContent(): void {
initMarkupCodeMath(el);
initMarkupRenderAsciicast(el);
initMarkupRenderIframe(el);
initMarkupRefIssue(el);
});
}
41 changes: 41 additions & 0 deletions web_src/js/markup/refissue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import {queryElems} from '../utils/dom.ts';
import {parseIssueHref} from '../utils.ts';
import {createApp} from 'vue';
import ContextPopup from '../components/ContextPopup.vue';
import {createTippy, getAttachedTippyInstance} from '../modules/tippy.ts';

export function initMarkupRefIssue(el: HTMLElement) {
queryElems(el, '.ref-issue', (el) => {
el.addEventListener('mouseenter', showMarkupRefIssuePopup);
el.addEventListener('focus', showMarkupRefIssuePopup);
});
}

export function showMarkupRefIssuePopup(e: MouseEvent | FocusEvent) {
const refIssue = e.currentTarget as HTMLElement;
if (getAttachedTippyInstance(refIssue)) return;
if (refIssue.classList.contains('ref-external-issue')) return;

const issuePathInfo = parseIssueHref(refIssue.getAttribute('href'));
if (!issuePathInfo.ownerName) return;

const el = document.createElement('div');
const tippy = createTippy(refIssue, {
theme: 'default',
content: el,
trigger: 'mouseenter focus',
placement: 'top-start',
interactive: true,
role: 'dialog',
interactiveBorder: 5,
// onHide() { return false }, // help to keep the popup and debug the layout
onShow: () => {
const view = createApp(ContextPopup, {
// backend: GetIssueInfo
loadIssueInfoUrl: `${window.config.appSubUrl}/${issuePathInfo.ownerName}/${issuePathInfo.repoName}/issues/${issuePathInfo.indexString}/info`,
});
view.mount(el);
},
});
tippy.show();
}
4 changes: 4 additions & 0 deletions web_src/js/modules/tippy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,3 +209,7 @@ export function showTemporaryTooltip(target: Element, content: Content): void {
},
});
}

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

@silverwind silverwind Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

image

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@silverwind silverwind Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.