Change request: fix review comments form

[pedrogaudencio]

• fix template to use /pulls/ instead of /issues/ in change request urls
• add routes for change request info, attachments, content-history, comments, reactions, title, content, and all other change request update operations

Fixes #93

---

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

[Copilot]

Pull request overview

This pull request fixes URL paths for pull request operations to use /pulls/ instead of /issues/ and adds article-based routes for pull request operations. The changes enable proper routing for pull requests in article-based views (accessed via /article/{username}/{subjectname} URLs).

Changes:

• Moved addIssuesPullsUpdateRoutes function earlier in the file to make it reusable for both standard and article-based routes
• Added article-based pull request view routes (info, attachments, content-history) at line 1413
• Added article-based pull request update routes (comments, reactions, title, content, etc.) at lines 1416-1431
• Fixed template to dynamically use /pulls/ or /issues/ path based on whether the item is a pull request

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
routers/web/web.go	Refactored route definitions to share update routes between standard and article-based URLs; added new article-based routes for pull request operations
custom/templates/repo/issue/view_content.tmpl	Introduced dynamic path selection to use correct URL path (/pulls/ vs /issues/) for reactions, attachments, and comments

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[taoeffect]

/review

[github-actions]

Automated Review Notice

This PR does not appear to have an associated issue. Every PR must have an assigned issue linked in its description.

Please update the PR description to reference an issue (e.g., Fixes #123 or Closes #456), then comment /review to trigger the automated review.

[taoeffect]

/review

[github-actions]

Automated PR Review

Combined Code Review

🔴 Critical: Article-based URLs missing /article prefix in template

Location:

• custom/templates/repo/issue/view_content.tmpl:33-35, 41, 51-53, 69-70
• routers/web.go:1435-1436 (route definition reference)

Problem:
The template uses {{$.RepoLink}} to build URLs, but for article-based routes, RepoLink is set to /<username>/<subjectname> (without the /article prefix). The article-based routes are defined as /article/{username}/{subjectname}/..., so all generated URLs will be missing the /article prefix, causing broken links and failed API calls.

Example broken URLs:

• Comment form action becomes /<owner>/<subject>/pulls/1/comments instead of /article/<owner>/<subject>/pulls/1/comments
• Content update URL becomes /<owner>/<subject>/pulls/1/content instead of /article/<owner>/<subject>/pulls/1/content

Code evidence:

// routers/web.go:1435-1436
m.Group("/article/{username}/{subjectname}", func() {
    m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader)

The article routes have /article prefix, but template uses {{$.RepoLink}} which doesn't include it.

Solution:
Add a BasePath context variable that includes the /article prefix for article-based routes. Modify the template to prepend it:

1. In route handlers (not in provided files, but needed): Set BasePath in context:

// In context.RepoAssignmentByOwnerAndSubject or similar:
if isArticleContext {
    ctx.Set("BasePath", "/article")
}

2. In template (view_content.tmpl):

- <form class="ui form form-fetch-action" id="comment-form" action="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/comments" method="post">
+ <form class="ui form form-fetch-action" id="comment-form" action="{{if $.BasePath}}{{$.BasePath}}{{end}}{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/comments" method="post">

- data-update-url="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/content"
+ data-update-url="{{if $.BasePath}}{{$.BasePath}}{{end}}{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/content"

- data-attachment-url="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/attachments"
+ data-attachment-url="{{if $.BasePath}}{{$.BasePath}}{{end}}{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/attachments"

- data-view-attachment-url="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/view-attachments"
+ data-view-attachment-url="{{if $.BasePath}}{{$.BasePath}}{{end}}{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/view-attachments"

- {{template "repo/issue/view_content/reactions" dict "ActionURL" (printf "%s/%s/%d/reactions" $.RepoLink $issueTypePath .Issue.Index) "Reactions" $reactions}}
+ {{template "repo/issue/view_content/reactions" dict "ActionURL" (printf "%s/%s/%d/reactions" (printf "%s%s" (default "" $.BasePath) $.RepoLink) $issueTypePath .Issue.Index) "Reactions" $reactions}}

Alternatively, if you can modify the context middleware, set RepoLink to include /article prefix directly for article-based routes, which is cleaner.

---

🟡 Medium: Duplicate route registration for comment and issue operations

Location:

• routers/web.go:1433-1438 (duplicate comment routes in article-based group)
• routers/web.go:1426-1430 (duplicate issue-specific routes in article-based group)

Problem:
The route definitions for comment operations (update, delete, reactions) and pull-specific operations (request review, dismiss review, resolve conversation) are duplicated between the standard repository group and the article-based group. This causes duplicate route registration, which can lead to panics or unpredictable behavior in the router.

Code evidence:
The following blocks appear in both the standard repository group and the article-based group:

Comment routes (duplicate in article group at lines 1433-1438):

m.Group("/comments/{id}", func() {
    m.Post("", repo.UpdateCommentContent)
    m.Post("/delete", repo.DeleteComment)
    m.Post("/reactions/{action}", web.Bind(forms.ReactionForm{}), repo.ChangeCommentReaction)
}, reqRepoIssuesOrPullsReader)

Issue-specific routes (duplicate in article group at lines 1426-1430):

m.Group("/issues", func() {
    m.Post("/request_review", repo.UpdatePullReviewRequest)
    m.Post("/dismiss_review", reqRepoAdmin, web.Bind(forms.DismissReviewForm{}), repo.DismissReview)
    m.Post("/resolve_conversation", repo.SetShowOutdatedComments, repo.UpdateResolveConversation)
}, reqUnitPullsReader)

These blocks are already present in the standard repository group (around lines 1380-1407). The article-based group calls addIssuesPullsUpdateRoutes, which should already include the comment routes, making the explicit duplicate comment routes in the article group unnecessary. Similarly, the issue-specific routes should be part of addIssuesPullsUpdateRoutes or a common function.

Solution:
Remove the duplicate route blocks from the article-based group:

• Remove lines 1433-1438 (comment routes)
• Remove lines 1426-1430 (issue routes)

Additionally, consider refactoring common route groups into reusable functions to avoid future duplication. For example:

addCommentRoutes := func() {
    m.Group("/comments/{id}", func() {
        m.Post("", repo.UpdateCommentContent)
        m.Post("/delete", repo.DeleteComment)
        m.Post("/reactions/{action}", web.Bind(forms.ReactionForm{}), repo.ChangeCommentReaction)
    }, reqRepoIssuesOrPullsReader)
}

addPullSpecificRoutes := func() {
    m.Group("/issues", func() {
        m.Post("/request_review", repo.UpdatePullReviewRequest)
        m.Post("/dismiss_review", reqRepoAdmin, web.Bind(forms.DismissReviewForm{}), repo.DismissReview)
        m.Post("/resolve_conversation", repo.SetShowOutdatedComments, repo.UpdateResolveConversation)
    }, reqUnitPullsReader)
}

Then call these functions in both the standard and article-based groups as needed.

---

🟡 Medium: Route registration order may cause conflicts

Location:

• routers/web.go:1397-1444

Problem:
Article-based routes are registered after the generic /{username}/{reponame} group but before the repo code routes. The order matters because:

1. The generic group /{username}/{reponame} catches all /{username}/{reponame}/* paths
2. Article routes are /article/... which don't conflict
3. However, if there's any route pattern overlap or if /{username}/{reponame} group uses a wildcard that could match /article/... (unlikely but possible if the router is prefix-based and not exact), there could be conflicts.

Current order:

// 1. Standard repo routes (/{username}/{reponame})
m.Group("/{username}/{reponame}", ...) { ... }

// 2. Article routes (/article/...)
m.Group("/article/{username}/{subjectname}", ...) { ... }

// 3. Repo code routes
m.Group("/{username}/{reponame}", ...) { ... }

Recommendation:
Move article routes before the generic /{username}/{reponame} group to be safe, or ensure the router uses exact matching for the first segment. Since /article is a distinct first segment, this is likely safe, but ordering more specific routes first is a good practice.

Suggested reorder:

// Article routes first (more specific)
m.Group("/article/{username}/{subjectname}", ...) { ... }

// Then standard repo routes
m.Group("/{username}/{reponame}", ...) { ... }

---

⚪️ Low: Inconsistent route naming in comments

Location:

• routers/web.go:1410 (comment above addIssuesPullsUpdateRoutes)

Problem:
The comment says:

// for "/{username}/{reponame}/issues" or "/{username}/{reponame}/pulls"

But after moving the function, it's now also used for /article/{username}/{subjectname}/pulls. The comment is outdated.

Solution:
Update the comment to reflect all use cases:

// for "/{username}/{reponame}/issues", "/{username}/{reponame}/pulls",
// or "/article/{username}/{subjectname}/pulls"

---

⚪️ Low: Minor Improvement – Template Consistency

Location:

• custom/templates/repo/issue/view_content.tmpl:3

Problem:
The template uses:

{{$issueTypePath := "issues"}}{{if .Issue.IsPull}}{{$issueTypePath = "pulls"}}{{end}}

While functional, this could be simplified by defining a helper template function to return the correct path based on .Issue.IsPull. However, the current approach is clear and works correctly.

Suggestion:
If a helper function is available, use:

{{$issueTypePath := issueTypePath .Issue.IsPull}}

But this is optional and not a bug.

---

Checklist

• 🔴 Article-based URLs missing /article prefix in template
• 🟡 Duplicate route registration for comment and issue operations
• 🟡 Route registration order may cause conflicts
• ⚪️ Inconsistent route naming in comments
• ⚪️ Minor Improvement – Template Consistency

---

Review generated using stepfun/step-3.5-flash via OpenRouter. Comment /review to re-run.

[taoeffect]

/review

[github-actions]

Automated PR Review

Combined Code Review

🔴 web.go:1411 - Compilation Error: addIssuesPullsViewRoutes Undefined

• Addressed
• Dismissed

The code references a function/variable addIssuesPullsViewRoutes inside the new article route group, but this function is not defined anywhere in the provided diff.
In web.go, addIssuesPullsUpdateRoutes is defined (lines 1310-1338), but addIssuesPullsViewRoutes is missing. This will likely cause a compilation error ("undefined: addIssuesPullsViewRoutes").

File:
routers/web/web.go

Lines:

	m.Group("/article/{username}/{subjectname}/{type:pulls}", addIssuesPullsViewRoutes, optSignIn, context.RepoAssignmentByOwnerAndSubject, reqUnitPullsReader)

Improvement:
You need to define addIssuesPullsViewRoutes. Assuming this was intended to register the viewing routes (GET requests) similar to how addIssuesPullsUpdateRoutes registers modification routes (POST requests), you should define it similarly. If it was intended to be the same update routes, you used the wrong name. Assuming it's for viewing:

// Define this near addIssuesPullsUpdateRoutes (e.g., around line 1310)
addIssuesPullsViewRoutes := func() {
    m.Get("/{index}", repo.ViewIssue)
    // Add other relevant GET routes for pulls/issues here, e.g.:
    // m.Get("/{index}/files", repo.ViewPullFiles)
    // m.Get("/{index}/commits", repo.ViewPullCommits)
}

// ... (later in the file)

// Line 1411: Usage remains the same
m.Group("/article/{username}/{subjectname}/{type:pulls}", addIssuesPullsViewRoutes, optSignIn, context.RepoAssignmentByOwnerAndSubject, reqUnitPullsReader)

---

🔴 Template/Route Mismatch - Sidebar operations won't work for article-based pull requests

• Addressed
• Dismissed

Location: Router registration vs. sidebar templates

Problem: The sidebar templates use hardcoded /issues/ paths, but the article routes only register /{type:pulls} paths. This means sidebar operations like updating labels, milestones, assignees, and projects will fail with 404 errors for article-based pull requests.

Affected templates:

templates/repo/issue/sidebar/label_list.tmpl:4:

data-update-url="{{$pageMeta.RepoLink}}/issues/labels?issue_ids={{$pageMeta.Issue.ID}}"

templates/repo/issue/sidebar/milestone_list.tmpl:6:

data-update-url="{{$pageMeta.RepoLink}}/issues/milestone?issue_ids={{$pageMeta.Issue.ID}}"

templates/repo/issue/sidebar/assignee_list.tmpl:6:

data-update-url="{{$pageMeta.RepoLink}}/issues/assignee?issue_ids={{$pageMeta.Issue.ID}}"

templates/repo/issue/view_title.tmpl:35:

<button type="submit" class="ui small primary button" data-update-url="{{$.RepoLink}}/issues/{{.Issue.Index}}/title">

What happens: For article-based pull requests, $.RepoLink is /article/{owner}/{subject}, so these generate URLs like /article/{owner}/{subject}/issues/labels, but only /article/{owner}/{subject}/pulls/labels is registered.

Suggested fix: Add a parallel /issues route group for article routes, matching the pattern used for standard repos:

// In routers/web/web.go, after line 1417:
m.Group("/article/{username}/{subjectname}", func() {
    // Register BOTH /pulls and /issues routes (same as standard repos)
    // FIXME: templates incorrectly send "pulls" requests to "issues" endpoints
    m.Group("/{type:issues}", addIssuesPullsUpdateRoutes, context.RequireUnitReader(unit.TypeIssues, unit.TypePullRequests), context.RepoMustNotBeArchived())
    m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader, context.RepoMustNotBeArchived())
    // ... rest of routes
}, reqSignIn, context.RepoAssignmentByOwnerAndSubject)

---

🟡 web.go:1419-1429 - Code Duplication for Comment and Legacy Routes

• Addressed
• Dismissed

The code introduces a block of routes for handling comments (/comments/{id}) and legacy pull request actions (/issues/...), but this logic is duplicated from the standard repository section (lines 1369-1401).
Duplicating route definitions increases maintenance overhead and the risk of divergence. You extracted addIssuesPullsUpdateRoutes, which was good, but you left these adjacent routes duplicated.

Additionally, the current code registers some pull-specific /issues routes (for request_review, dismiss_review, resolve_conversation) but not others (labels, milestone, assignee, status, etc.). This creates an inconsistent API where some operations work via /issues paths while others don't.

File:
routers/web/web.go

Lines:

		m.Group("/comments/{id}", func() {
			m.Post("", repo.UpdateCommentContent)
			m.Post("/delete", repo.DeleteComment)
			m.Post("/reactions/{action}", web.Bind(forms.ReactionForm{}), repo.ChangeCommentReaction)
		}, reqRepoIssuesOrPullsReader)

		// Pull-specific routes that use /issues path in standard routes
		m.Group("/issues", func() {
			m.Post("/request_review", repo.UpdatePullReviewRequest)
			m.Post("/dismiss_review", reqRepoAdmin, web.Bind(forms.DismissReviewForm{}), repo.DismissReview)
			m.Post("/resolve_conversation", repo.SetShowOutdatedComments, repo.UpdateResolveConversation)
		}, reqUnitPullsReader)
	}, reqSignIn, context.RepoAssignmentByOwnerAndSubject, context.RepoMustNotBeArchived())

Improvement:
Extract these route definitions into a helper function (similar to addIssuesPullsUpdateRoutes) or include them in the existing helper if the path structure allows. Since /comments/{id} does not follow the /{type}/{index} pattern of the update routes, a separate helper is cleaner.

// Define this near line 1310
addIssueCommentRoutes := func() {
    m.Group("/comments/{id}", func() {
        m.Post("", repo.UpdateCommentContent)
        m.Post("/delete", repo.DeleteComment)
        m.Post("/reactions/{action}", web.Bind(forms.ReactionForm{}), repo.ChangeCommentReaction)
    }, reqRepoIssuesOrPullsReader)

    // Handling the legacy "pull requests under /issues path" quirk
    m.Group("/issues", func() {
        m.Post("/request_review", repo.UpdatePullReviewRequest)
        m.Post("/dismiss_review", reqRepoAdmin, web.Bind(forms.DismissReviewForm{}), repo.DismissReview)
        m.Post("/resolve_conversation", repo.SetShowOutdatedComments, repo.UpdateResolveConversation)
    }, reqUnitPullsReader)
}

Then update the usage in both locations:

Standard Repo (around line 1369):

		addIssueCommentRoutes()
	}, reqSignIn, context.RepoAssignment, context.RepoMustNotBeArchived())

Article Repo (around line 1419):

		addIssueCommentRoutes()
	}, reqSignIn, context.RepoAssignmentByOwnerAndSubject, context.RepoMustNotBeArchived())

---

🟡 view_content.tmpl:4-78 - Simplification: Use .Issue.Link instead of Manual Path Construction

• Addressed
• Dismissed

The template changes introduce a variable $issueTypePath to manually construct URLs (e.g., issues/1 vs pulls/1). This logic is fragile. The .Issue object in Gitea templates typically has a .Issue.Link property that already contains the correct path (including whether it is an issue or a pull request). Using manual string formatting with printf is unnecessary complexity.

File:
custom/templates/repo/issue/view_content.tmpl

Lines:

{{$issueTypePath := "issues"}}{{if .Issue.IsPull}}{{$issueTypePath = "pulls"}}{{end}}
...
{{template "repo/issue/view_content/add_reaction" dict "ActionURL" (printf "%s/%s/%d/reactions" $.RepoLink $issueTypePath .Issue.Index)}}
...
<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/content" ... ></div>
...
{{template "repo/issue/view_content/reactions" dict "ActionURL" (printf "%s/%s/%d/reactions" $.RepoLink $issueTypePath .Issue.Index) "Reactions" $reactions}}
...
<form class="ui form form-fetch-action" id="comment-form" action="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/comments" method="post">

Improvement:
Remove the variable and use .Issue.Link directly. This ensures consistency and handles the path logic automatically.

{{/* Remove line 4: {{$issueTypePath := "issues"}}... */}}

{{/* Update Line 34 (Reactions) */}}
{{template "repo/issue/view_content/add_reaction" dict "ActionURL" (printf "%s/reactions" .Issue.Link)}}

{{/* Update Line 52 (Edit Zone) */}}
<div class="edit-content-zone tw-hidden" data-update-url="{{.Issue.Link}}/content" data-content-version="{{.Issue.ContentVersion}}" data-context="{{.RepoLink}}" data-attachment-url="{{.Issue.Link}}/attachments" data-view-attachment-url="{{.Issue.Link}}/view-attachments"></div>

{{/* Update Line 60 (Reactions) */}}
{{template "repo/issue/view_content/reactions" dict "ActionURL" (printf "%s/reactions" .Issue.Link) "Reactions" $reactions}}

{{/* Update Line 78 (Form Action) */}}
<form class="ui form form-fetch-action" id="comment-form" action="{{.Issue.Link}}/comments" method="post">

---

🟡 web.go:1413 - Missing context.RepoMustNotBeArchived() on view routes

• Addressed
• Dismissed

Problem: The article view routes use optSignIn while update routes use reqSignIn. However, the view routes are missing the context.RepoMustNotBeArchived() middleware that update routes have. While this may be intentional (viewing should work even for archived repos), it creates an inconsistency in route structure.

// Article-based pull request view routes (info, attachments, content-history)
m.Group("/article/{username}/{subjectname}/{type:pulls}", addIssuesPullsViewRoutes, optSignIn, context.RepoAssignmentByOwnerAndSubject, reqUnitPullsReader)

vs.

// Article-based pull request update routes (comments, reactions, title, content, etc.)
m.Group("/article/{username}/{subjectname}", func() {
    // ...
}, reqSignIn, context.RepoAssignmentByOwnerAndSubject, context.RepoMustNotBeArchived())

Assessment: This is likely intentional (viewing PRs on archived repos should work), but the comment could be clearer. No code change needed if this is intentional.

---

⚪️ web.go:1320-1364 - Closure captures router variable - potential maintenance issue

• Addressed
• Dismissed

The addIssuesPullsUpdateRoutes function is defined as a closure that captures the m router variable from the outer scope. While this works, it's unusual and could lead to confusion during refactoring.

addIssuesPullsUpdateRoutes := func() {
    // Uses 'm' from outer scope
    m.Group("/{index}", func() {
        m.Post("/title", repo.UpdateIssueTitle)
        // ...
    })
}

Suggested improvement: Consider passing the router as a parameter:

addIssuesPullsUpdateRoutes := func(m *web.Router) {
    m.Group("/{index}", func() {
        m.Post("/title", repo.UpdateIssueTitle)
        // ...
    })
    // ...
}

// Usage:
m.Group("/{type:issues}", func() {
    addIssuesPullsUpdateRoutes(m)
}, /* middlewares */)

---

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

[pedrogaudencio]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Combined (opencode)
• Model: glm-5

Click to expand review

1. 🔴 Critical: Route Registration path mismatch for Attachments

• Addressed
• Dismissed

File: routers/web/web.go:1336-1337

Problem:
The refactored function addIssuesPullsUpdateRoutes registers the attachment routes (/attachments and /attachments/remove) at the root of its route group.
When this function is called at line 1344 (m.Group("/{type:issues}", addIssuesPullsUpdateRoutes...)), the routes are registered as /{username}/{reponame}/issues/attachments.
However, the template changes in custom/templates/repo/issue/view_content.tmpl:57 generate URLs that include the issue index: {{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/attachments.
This results in a request to /owner/repo/issues/1/attachments, but the router only matches /owner/repo/issues/attachments. The request will fail with a 404 Not Found, breaking the attachment upload functionality for existing issues and pull requests.

Suggestion:
Move the attachment routes inside the m.Group("/{index}", ...) block within addIssuesPullsUpdateRoutes. This aligns the route definition with the URL generated by the template and the likely handler expectations (which often require the issue index context).

// routers/web/web.go:1308-1338
addIssuesPullsUpdateRoutes := func() {
	// for "/{username}/{reponame}/issues", "/{username}/{reponame}/pulls",
	// or "/article/{username}/{subjectname}/pulls"
	m.Group("/{index}", func() {
		m.Post("/title", repo.UpdateIssueTitle)
		m.Post("/content", repo.UpdateIssueContent)
		m.Post("/deadline", repo.UpdateIssueDeadline)
		m.Post("/watch", repo.IssueWatch)
		m.Post("/ref", repo.UpdateIssueRef)
		m.Post("/pin", reqRepoAdmin, repo.IssuePinOrUnpin)
		m.Post("/viewed-files", repo.UpdateViewedFiles)
		m.Group("/dependency", func() {
			m.Post("/add", repo.AddDependency)
			m.Post("/delete", repo.RemoveDependency)
		})
		m.Combo("/comments").Post(repo.MustAllowUserComment, web.Bind(forms.CreateCommentForm{}), repo.NewComment)
		m.Group("/times", func() {
			m.Post("/add", web.Bind(forms.AddTimeManuallyForm{}), repo.AddTimeManually)
			m.Post("/{timeid}/delete", repo.DeleteTime)
			m.Group("/stopwatch", func() {
				m.Post("/start", repo.IssueStartStopwatch)
				m.Post("/stop", repo.IssueStopStopwatch)
				m.Post("/cancel", repo.CancelStopwatch)
			})
		})
		m.Post("/time_estimate", repo.UpdateIssueTimeEstimate)
		m.Post("/reactions/{action}", web.Bind(forms.ReactionForm{}), repo.ChangeIssueReaction)
		m.Post("/lock", reqRepoIssuesOrPullsWriter, web.Bind(forms.IssueLockForm{}), repo.LockIssue)
		m.Post("/unlock", reqRepoIssuesOrPullsWriter, repo.UnlockIssue)
		m.Post("/delete", reqRepoAdmin, repo.DeleteIssue)
		m.Post("/content-history/soft-delete", repo.SoftDeleteContentHistory)

		// START FIX: Move attachments inside the /{index} group
		m.Post("/attachments", repo.UploadIssueAttachment)
		m.Post("/attachments/remove", repo.DeleteAttachment)
		// END FIX
	})

	// These routes likely apply to the issue list or batch operations, kept outside /{index}
	m.Post("/labels", reqRepoIssuesOrPullsWriter, repo.UpdateIssueLabel)
	m.Post("/milestone", reqRepoIssuesOrPullsWriter, repo.UpdateIssueMilestone)
	m.Post("/projects", reqRepoIssuesOrPullsWriter, reqRepoProjectsReader, repo.UpdateIssueProject)
	m.Post("/assignee", reqRepoIssuesOrPullsWriter, repo.UpdateIssueAssignee)
	m.Post("/status", reqRepoIssuesOrPullsWriter, repo.UpdateIssueStatus)
	m.Post("/delete", reqRepoAdmin, repo.BatchDeleteIssues)
	m.Delete("/unpin/{index}", reqRepoAdmin, repo.IssueUnpin)
	m.Post("/move_pin", reqRepoAdmin, repo.IssuePinMove)
}

---

2. 🟡 Medium: Potential duplication of Route Definitions

• Addressed
• Dismissed

File: routers/web/web.go:1344-1345

Problem:
The addIssuesPullsUpdateRoutes function registers routes like m.Post("/labels", ...) and m.Post("/status", ...) at the root of the passed group.
In the existing code block starting at line 1312 (visible in context), there might already be routes registered for /labels, /milestone, etc., inside the m.Group("/issues", ...) block (lines 1320-1326 in the diff context).
If addIssuesPullsUpdateRoutes is mounted on /{type:issues} (Line 1344), and the standard m.Group("/issues", ...) (Line 1312) is also active and defines overlapping routes, you might encounter duplicate route registration panics or conflicts depending on the router implementation details.
Furthermore, line 1345 mounts addIssuesPullsUpdateRoutes on /{type:pulls}. Standard Gitea often routes Pull-specific actions through /issues endpoints (as noted in comments), so applying the full suite of issue update routes (like /times or /dependency which may not apply to PRs) to pulls might be unnecessary or noisy, though likely harmless if the handlers check type.

Suggestion:
Ensure that the block starting at line 1312 (m.Group("/issues", ...)) does not double-register the routes that are now inside addIssuesPullsUpdateRoutes. If the existing code (lines 1312-1342) is being kept, verify that addIssuesPullsUpdateRoutes isn't redundant for the standard /issues path. If this is a refactor to consolidate routes, ensure the old routes (lines 1320-1340 in context) are actually removed or that this function is used instead of them. If they run in parallel, verify conflicts.

---

3. 🟡 Medium: Outdated Comment in addIssuesPullsViewRoutes

• Addressed
• Dismissed

File: routers/web/web.go:1286-1299

Problem:
The comment inside addIssuesPullsViewRoutes says "for /{username}/{reponame}/issues" or "/{username}/{reponame}/pulls"" but this function is now also being used for /article/{username}/{subjectname}/{type:pulls} (line 1413). The comment should be updated to reflect the new usage.

Suggestion:
Update the comment to reflect all use cases:

addIssuesPullsViewRoutes := func() {
    // for "/{username}/{reponame}/issues", "/{username}/{reponame}/pulls",
    // or "/article/{username}/{subjectname}/pulls"
    m.Get("/posters", repo.IssuePullPosters)
    ...
}

---

4. 🟡 Medium: Inconsistent Middleware Application in Article Routes

• Addressed
• Dismissed

File: routers/web/web.go:1413-1431

Problem:
The article view routes (line 1413) and article update routes (lines 1416-1431) have different middleware ordering, and the view routes are missing context.RepoMustNotBeArchived() (though this is intentional for read operations).

More importantly, the article routes use reqUnitPullsReader directly, while the standard routes for issues use context.RequireUnitReader(unit.TypeIssues, unit.TypePullRequests). This inconsistency could cause confusion:

Standard routes:

m.Group("/{type:issues}", addIssuesPullsUpdateRoutes, context.RequireUnitReader(unit.TypeIssues, unit.TypePullRequests), context.RepoMustNotBeArchived())
m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader, context.RepoMustNotBeArchived())

Article routes:

m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader)

The article routes only support pulls (which is consistent with Forkana's design), but if anyone tries to add issues support later, they might not realize the middleware difference.

Suggestion:
Add a comment clarifying the design decision:

// Article-based pull request update routes (comments, reactions, title, content, etc.)
// Note: Only pulls are supported for articles; issues use standard repo routes
m.Group("/article/{username}/{subjectname}", func() {
    m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader)
    ...
}, reqSignIn, context.RepoAssignmentByOwnerAndSubject, context.RepoMustNotBeArchived())

---

5. 🟡 Medium: Potential Missing Route for Issue Context in Articles

• Addressed
• Dismissed

File: custom/templates/repo/issue/view_content.tmpl:3

Problem:
The template introduces $issueTypePath which can be either "issues" or "pulls", but article routes only define routes for /{type:pulls}, not /{type:issues}.

{{$issueTypePath := "issues"}}{{if .Issue.IsPull}}{{$issueTypePath = "pulls"}}{{end}}

If this template is ever used for an issue (not a pull) in an article context, the generated URLs like /article/{owner}/{subject}/issues/{index}/comments would fail because no such route exists.

However, this appears to be intentional based on Forkana's design (only "change requests"/pulls are supported for articles). The standard repo routes still support issues via /{username}/{reponame}/issues/{index}.

Suggestion:
Document this design decision in the template or router to prevent confusion:

{{$issueTypePath := "issues"}}{{if .Issue.IsPull}}{{$issueTypePath = "pulls"}}{{end}}{{/* Note: Issues in article context are not supported; only pulls have article routes */}}

---

6. ⚪️ Low: Unused data-view-attachment-url Attribute

• Addressed
• Dismissed

File: custom/templates/repo/issue/view_content.tmpl:57

Problem:
The template includes a data-view-attachment-url attribute that references a non-existent route. This is being carried over from the original template.

<div class="edit-content-zone tw-hidden" data-update-url="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/content" data-content-version="{{.Issue.ContentVersion}}" data-context="{{.RepoLink}}" data-attachment-url="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/attachments" data-view-attachment-url="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/view-attachments"></div>

The data-view-attachment-url attribute points to /view-attachments, but no such route exists in the router. The JavaScript code in web_src/js/features/repo-issue-edit.ts only uses data-update-url, data-context, data-content-version, and data-attachment-url - it never references data-view-attachment-url.

Suggestion:
Either:

1. Remove the unused attribute entirely, or
2. Implement the corresponding route if this functionality was intended

---

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

[pedrogaudencio]

1. 🔴 Critical: Route Registration path mismatch for Attachments

data-attachment-url is used for GET listing with addIssuesPullsViewRoutes where /attachments is inside /{index}. POST uploads use data-upload-url from AddUploadContext, and that targets the non /{index} route. So there’s no 404.

2. 🟡 Medium: Potential duplication of Route Definitions

There's no duplication. The /issues group only adds /new and /search. addIssuesPullsUpdateRoutes registers the update endpoints (/title, /content, /labels, etc) under /{type:issues} and /{type:pulls}, so these are different routes.

3. 🟡 Medium: Outdated Comment in addIssuesPullsViewRoutes

Correct, the comment in addIssuesPullsViewRoutes is outdated (it doesn’t mention the article route). But addIssuesPullsUpdateRoutes already has the updated comment. This is a small docs change with no impact, so I believe it's not worth changing.

4. 🟡 Medium: Inconsistent Middleware Application in Article Routes

This middleware difference is intentional: articles only support Change Requests, so using reqUnitPullsReader is correct. And RepoMustNotBeArchived() is already enforced in the outer group. Adding a comment could help, but the behaviour is correct.

5. 🟡 Medium: Potential Missing Route for Issue Context in Articles

This is intentional: article flows don't have issues (only Change Requets). In Forkana's context, the template sets $issueTypePath = "pulls" when Issue.IsPull is true - which is always the case for articles. So there’s no need to add an "issues" route here.

6. ⚪️ Low: Unused data-view-attachment-url Attribute

This attribute comes from upstream Gitea and I kept it in the custom template. It doesn't seem to be used in the template but I'm not sure if it would break something later on. I'm leaving it and if @pieer sees fit then it be removed

[taoeffect]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

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

Click to expand review

No review found in agentic output.

---

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

[taoeffect]

/crush_fast (Z.ai seems to be having problems... let me try that one more time)

---

AI review started.

[github-actions]

Advanced AI Review

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

Click to expand review

Code Review: Discussion Feature for Change Requests

🔴 Issue 1: Missing context.RepoMustNotBeArchived() on inner /{type:pulls} route group

• Addressed
• Dismissed

File: routers/web/web.go:1417

m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader)

The original repo routes have context.RepoMustNotBeArchived() applied at both the outer group level AND the inner {type:issues}/{type:pulls} level:

// Original pattern (lines 1377-1378)
m.Group("/{type:issues}", addIssuesPullsUpdateRoutes, context.RequireUnitReader(unit.TypeIssues, unit.TypePullRequests), context.RepoMustNotBeArchived())
m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader, context.RepoMustNotBeArchived())

The new article routes only apply the archived check at the outer group level. While this is functionally equivalent (the outer middleware runs first), it's inconsistent with the established pattern and makes the code harder to understand.

Recommendation: Add context.RepoMustNotBeArchived() to the inner group for consistency:

m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader, context.RepoMustNotBeArchived())

---

🟡 Issue 2: Potential route ordering issue with overlapping view and update route groups

• Addressed
• Dismissed

File: routers/web/web.go:1413, 1416-1417

The view routes and update routes are registered in separate route groups with the same path prefix:

// Line 1413 - View routes
m.Group("/article/{username}/{subjectname}/{type:pulls}", addIssuesPullsViewRoutes, optSignIn, context.RepoAssignmentByOwnerAndSubject, reqUnitPullsReader)

// Lines 1416-1417 - Update routes
m.Group("/article/{username}/{subjectname}", func() {
    m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader)
    ...
}, reqSignIn, context.RepoAssignmentByOwnerAndSubject, context.RepoMustNotBeArchived())

Both groups will register routes under /article/{username}/{subjectname}/{type:pulls}. While the specific endpoints (GET vs POST, different sub-paths) don't conflict, the middleware is applied differently:

1. View routes: optSignIn (optional authentication)
2. Update routes: reqSignIn (required authentication) on the outer group

This is intentional and correct - view routes should be accessible without authentication while update routes require it. However, having two separate route groups with overlapping paths can make the code harder to maintain and could lead to confusion.

Recommendation: Consider consolidating into a single route group structure for clarity:

m.Group("/article/{username}/{subjectname}", func() {
    // View routes - accessible without sign-in
    m.Group("/{type:pulls}", addIssuesPullsViewRoutes, reqUnitPullsReader)
    
    // Update routes - require sign-in
    m.Group("", func() {
        m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader, context.RepoMustNotBeArchived())
        m.Group("/comments/{id}", func() {
            m.Post("", repo.UpdateCommentContent)
            m.Post("/delete", repo.DeleteComment)
            m.Post("/reactions/{action}", web.Bind(forms.ReactionForm{}), repo.ChangeCommentReaction)
        }, reqRepoIssuesOrPullsReader)
        m.Group("/issues", func() {
            m.Post("/request_review", repo.UpdatePullReviewRequest)
            m.Post("/dismiss_review", reqRepoAdmin, web.Bind(forms.DismissReviewForm{}), repo.DismissReview)
            m.Post("/resolve_conversation", repo.SetShowOutdatedComments, repo.UpdateResolveConversation)
        }, reqUnitPullsReader)
    }, reqSignIn, context.RepoMustNotBeArchived())
}, optSignIn, context.RepoAssignmentByOwnerAndSubject)

---

⚪️ Issue 3: Comment URL format in view_content.tmpl uses /{issueTypePath}/{index}/comments but routes register at /{index}/comments

• Addressed
• Dismissed

File: custom/templates/repo/issue/view_content.tmpl:83

<form class="ui form form-fetch-action" id="comment-form" action="{{$.RepoLink}}/{{$issueTypePath}}/{{.Issue.Index}}/comments" method="post">

File: routers/web/web.go:1323, 1335

m.Group("/{index}", func() {
    ...
    m.Combo("/comments").Post(repo.MustAllowUserComment, web.Bind(forms.CreateCommentForm{}), repo.NewComment)

The template generates URLs like /article/owner/subject/pulls/123/comments, and the routes are registered under /{type:pulls}/{index}/comments. These should match correctly.

However, I note that the comments.tmpl template (at line 57, 71, 78) uses /comments/{id} paths for individual comment operations, not /{issueTypePath}/.... This is correct because comment operations don't go through the issue-specific routes.

This is working correctly but worth verifying during testing.

---

⚪️ Issue 4: Inconsistent use of hardcoded /issues/ path for pull-specific operations

• Addressed
• Dismissed

File: routers/web/web.go:1426-1430

// Pull-specific routes that use /issues path in standard routes
m.Group("/issues", func() {
    m.Post("/request_review", repo.UpdatePullReviewRequest)
    m.Post("/dismiss_review", reqRepoAdmin, web.Bind(forms.DismissReviewForm{}), repo.DismissReview)
    m.Post("/resolve_conversation", repo.SetShowOutdatedComments, repo.UpdateResolveConversation)
}, reqUnitPullsReader)

This mirrors the existing pattern in the original routes (lines 1402-1407) where pull-specific operations like request_review, dismiss_review, and resolve_conversation are under /issues path rather than /pulls. The FIXME comment in the original code acknowledges this is a known issue.

While the changes are consistent with existing behavior, this perpetuates the inconsistency. The template changes use $issueTypePath for most operations, but these pull-specific operations still use hardcoded /issues/.

Note: This is an existing codebase issue that the changes correctly mirror, not something new introduced by these changes. However, it's worth being aware of for future cleanup.

---

Summary

The changes are generally well-structured and follow the existing patterns in the codebase. The primary concern is consistency with the established middleware application pattern (Issue 1). Issue 2 is a code organization suggestion that could improve maintainability. Issues 3 and 4 are observations that should work correctly but are worth noting for testing.

The template changes correctly distinguish between issues and pulls paths using the $issueTypePath variable, which will generate correct URLs for both regular repo routes and article-based routes since RepoLink always returns the article-style path (/article/owner/subject).

---

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

[taoeffect]

Great work @pedrogaudencio!! 🎉