Update fork design

[pieer]

This PR updates the pull request review UI, improves internationalization, and standardizes CSS practices across the fork design pages.

Changes
🎨 UI & Styling
Review modal: Replaced the tippy.js popup with a proper overlay modal for submitting pull request reviews
Renamed new_review.tmpl → review_submit.tmpl for clarity
Split diff headers: Added styled column headers ("Current article" / "Change requested") to the split diff view
CSS variables: Replaced ~60 hardcoded hex/rgb colors across 6 CSS files with semantic CSS variables (e.g., --color-text-primary, --color-border, --color-surface-muted)
Font weights: Replaced hardcoded font-weight values (500, 600, 700) with CSS variable equivalents (--font-weight-medium, --font-weight-semibold, --font-weight-bold)
🌐 Internationalization
Added 12 new locale keys for the review modal, split diff headers, and back navigation
Replaced all hardcoded English strings in review_submit.tmpl,
section_split.tmpl
, and
view_title.tmpl
🧹 Code Quality
Removed inline <script> from the review template — moved logic to
repo-issue.ts
using data attributes and class hooks
Removed all stylelint-disable comments (13 total) by converting values to CSS variables
Removed unused createTippy import from
repo-issue.ts
Fixed getElementById → querySelector per ESLint rules
Fixed javascript:history.back() → proper onclick handler in
view_title.tmpl
Added
switchToUserPreference()
no-op to
ToastCommentEditor
for API compatibility

---

[devin-ai-integration]

Devin Review found 2 potential issues.

View 7 additional findings in Devin Review.

[pieer]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5

Click to expand review

Based on my thorough review of these changes, here are the issues I've identified:

---

🔴 Issue 1: Missing data-fork-confirm-body2 attribute in template

• Addressed
• Dismissed

Location: custom/templates/shared/repo/article.tmpl:97-103

The JavaScript code expects a data-fork-confirm-body2 attribute:

// web_src/js/features/article-editor.ts:71
const body2 = forkArticleButton.getAttribute('data-fork-confirm-body2') || '';

But the template only provides data-fork-confirm-body:

<button type="button" id="fork-article-button" class="ui secondary button"
    data-fork-confirm-title="{{ctx.Locale.Tr "repo.fork_article_confirm_title"}}"
    data-fork-confirm-body="{{ctx.Locale.Tr "repo.fork_article_confirm_body"}}"
    data-fork-confirm-yes="{{ctx.Locale.Tr "repo.fork_article_confirm_yes"}}"
    data-fork-confirm-cancel="{{ctx.Locale.Tr "repo.fork_article_confirm_cancel"}}">

Impact: The second paragraph in the fork confirmation modal will always be empty.

Fix: Add the missing attribute to the template:

<button type="button" id="fork-article-button" class="ui secondary button"
    data-fork-confirm-title="{{ctx.Locale.Tr "repo.fork_article_confirm_title"}}"
    data-fork-confirm-body="{{ctx.Locale.Tr "repo.fork_article_confirm_body"}}"
    data-fork-confirm-body2="{{ctx.Locale.Tr "repo.fork_article_confirm_body2"}}"
    data-fork-confirm-yes="{{ctx.Locale.Tr "repo.fork_article_confirm_yes"}}"
    data-fork-confirm-cancel="{{ctx.Locale.Tr "repo.fork_article_confirm_cancel"}}">

---

🔴 Issue 2: Review title field is not persisted to backend

• Addressed
• Dismissed

Location: templates/repo/diff/review_submit.tmpl:29-32

The new review modal adds a title input field:

<div class="field">
    <label class="review-field-label">{{ctx.Locale.Tr "repo.diff.review.add_title"}}</label>
    <input type="text" name="title" class="review-title-input" placeholder="">
</div>

However, the backend SubmitReviewForm struct in services/forms/repo_form.go:580-585 only has:

type SubmitReviewForm struct {
    Content  string
    Type     string
    CommitID string
    Files    []string
}

Impact: The title field is silently ignored - users enter a title but it's never saved or used.

Fix options:

1. Add Title field to SubmitReviewForm and update the review submission logic to use it
2. Remove the title input from the modal if it's not intended to be stored

---

🔴 Issue 3: Large block of CSS commented out, potentially breaking non-PR pages

• Addressed
• Dismissed

Location: web_src/css/repo.css:30-84

A significant block of CSS has been commented out:

/*
.issue-content {
  display: flex;
  align-items: flex-start;
  gap: 16px;
}
...
.issue-content-right .ui.list {
  margin: 0.5em 0;
  max-width: 100%;
} */

However, the media query at line 96-100 still references these classes:

@media (max-width: 767.98px) {
  .issue-content-left,
  .issue-content-right {
    width: 100%;
  }
}

Impact: This will break the layout of issue pages (non-PR pages) since the base flexbox layout styles are removed but media queries remain.

Fix: Either:

1. Remove the entire block including media queries if the intent is to fully replace it in repo.custom.css
2. Restore the styles and only override what's needed for PRs using a more specific selector

---

🟡 Issue 4: Inline onclick handlers violate CSP best practices

• Addressed
• Dismissed

Locations:

• templates/repo/diff/box.tmpl:7
• templates/repo/issue/view_title.tmpl:14

<!-- box.tmpl:7 -->
<button class="info-dismiss" onclick="this.parentElement.style.display='none'; try{localStorage.setItem('diff_info_dismissed','1')}catch(e){}" aria-label="Dismiss">

<!-- view_title.tmpl:14 -->
<a href="#" onclick="history.back(); return false" class="pr-back-link">

Impact: Inline event handlers won't work if CSP restricts unsafe-inline in script-src. This creates inconsistent behavior and security concerns.

Fix: Move to event listeners in TypeScript:

// In repo-issue.ts or similar
document.querySelector('.info-dismiss')?.addEventListener('click', function() {
  this.parentElement.style.display = 'none';
  try { localStorage.setItem('diff_info_dismissed', '1'); } catch {}
});

document.querySelector('.pr-back-link')?.addEventListener('click', (e) => {
  e.preventDefault();
  history.back();
});

---

🟡 Issue 5: Missing Escape key handler for review modal (accessibility)

• Addressed
• Dismissed

Location: web_src/js/features/repo-issue.ts:322-340

The review modal implementation only handles click events but not keyboard navigation:

if (elReviewBtn && elReviewOverlay) {
  elReviewBtn.addEventListener('click', (e) => {
    e.preventDefault();
    e.stopPropagation();
    elReviewOverlay.style.display = 'flex';
  });
  elReviewClose?.addEventListener('click', (e) => {
    e.preventDefault();
    elReviewOverlay.style.display = 'none';
  });
  elReviewOverlay.addEventListener('click', (e) => {
    if (e.target === elReviewOverlay) {
      elReviewOverlay.style.display = 'none';
    }
  });
}

Impact: Users cannot close the modal with the Escape key, which is expected behavior and an accessibility concern.

Fix:

// Add Escape key handler
document.addEventListener('keydown', (e) => {
  if (e.key === 'Escape' && elReviewOverlay?.style.display === 'flex') {
    elReviewOverlay.style.display = 'none';
  }
});

---

🟡 Issue 6: Hardcoded English strings not internationalized

• Addressed
• Dismissed

Locations:

• templates/repo/diff/comment_form.tmpl:39,41,43
• templates/repo/diff/box.tmpl:6

<!-- comment_form.tmpl -->
<button name="single_review" value="true" type="submit" class="ui submit tiny basic button btn-add-single">Post immediately</button>
<button name="pending_review" type="submit" class="ui submit primary tiny button btn-add-comment">Save draft</button>
<button name="pending_review" type="submit" class="ui submit primary tiny button btn-start-review">Save draft</button>

<!-- box.tmpl -->
<span class="info-text">Click a paragraph to add a comment.</span>

Impact: Non-English users will see mixed language UI.

Fix: Add locale keys and use {{ctx.Locale.Tr "..."}}:

; custom/options/locale/locale_en-US.ini
[repo.diff.comment]
post_immediately = Post immediately
save_draft = Save draft
info_banner_text = Click a paragraph to add a comment.

---

🟡 Issue 7: Mobile redirect on every page load

• Addressed
• Dismissed

Location: templates/repo/diff/box.tmpl:51-59

<script>
    // Default to unified view on mobile if no explicit style param is set
    (function() {
        var params = new URLSearchParams(window.location.search);
        if (!params.has('style') && window.innerWidth <= 768) {
            params.set('style', 'unified');
            window.location.search = params.toString();
        }
    })();
</script>

Impact: This causes a page reload on every mobile visit when there's no style parameter. Combined with the changed default in middlewares.go (which defaults to split view), this creates inconsistent behavior - the server renders split, but JS redirects to unified.

Fix: Handle this consistently on the server side by passing the viewport width or using CSS media queries instead of JavaScript redirects. Alternatively, set a cookie/sessionStorage to prevent redirect loops.

---

⚪ Issue 8: formatBoldText function is fragile

• Addressed
• Dismissed

Location: web_src/js/features/article-editor.ts:7-10

// Convert markdown-style bold (**text**) to <strong> tags
function formatBoldText(text: string): string {
  // eslint-disable-next-line github/unescaped-html-literal
  return text.replace(/\*\*(.+?)\*\*/g, '<strong>$1</strong>');
}

Impact: This manual markdown parsing is fragile:

• Doesn't handle ** at word boundaries correctly
• Could produce unexpected results with nested or malformed markdown
• The regex (.+?) could match across unintended boundaries

Since the input comes from trusted locale strings, this isn't a security issue, but it's fragile code.

Fix: Consider using a proper markdown parser or simply allow the locale strings to contain HTML directly (since they're trusted) with appropriate documentation.

---

⚪ Issue 9: Dead code should be removed, not commented

• Addressed
• Dismissed

Location: web_src/css/repo.css:90-94

/* .issue-content-right .ui.list.labels-list {
  display: flex;
  gap: var(--gap-inline);
  flex-wrap: wrap;
} */

Impact: Clutters the codebase and can cause confusion about whether code is intentionally disabled or forgotten.

Fix: Remove commented code entirely. Use git history to recover if needed.

---

Summary

Priority	Count
🔴 High	3
🟡 Medium	4
⚪ Low	2

The most critical issues are:

1. Missing template attribute - The fork modal won't display correctly
2. Unused title field - Misleading UX where user input is silently discarded
3. Commented CSS - Likely breaks non-PR issue pages

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View 12 additional findings in Devin Review.

[pedrogaudencio]

I've merged the two stacked PRs (/pull/152 and /pull/154) with the latest changes from update-fork-design and everything seems to be working well now. 👍

Seems good from my end!