Change request: fix diff comments

[pedrogaudencio]

Fixes /issues/151
Closes /issues/91

Notes: depends on /pull/147

---

[pedrogaudencio]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

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

Click to expand review

---

Review Summary

The changes look correct and implement defensive programming improvements. I found no bugs or security issues in the modified code. The changes are:

1. Type assertion safety fixes - Using the comma-ok idiom to prevent potential panics
2. UX improvement - Always showing the review button (with disabled state and counter) when on PR files page

---

1. 🔵 Type Assertion Fix in pull.go

• Addressed
• Dismissed

Location: routers/web/repo/pull.go:791

showOutdatedComments, _ := ctx.Data["ShowOutdatedComments"].(bool)

This is a good defensive fix. The middleware SetShowOutdatedComments is always applied before this handler, so the type assertion should always succeed. However, using the comma-ok idiom is still safer and prevents potential panics if there's ever a bug in routing.

---

2. 🔵 Type Assertion Fix in pull_review.go

• Addressed
• Dismissed

Location: routers/web/repo/pull_review.go:172-173

showOutdatedComments, _ := ctx.Data["ShowOutdatedComments"].(bool)
showOutdatedComments = origin == "timeline" || showOutdatedComments

This is semantically equivalent to the original code but safer. The fallback to false when the value isn't set is a safe default (don't show outdated comments if uncertain).

---

3. 🔵 Template Change in box.tmpl

• Addressed
• Dismissed

Location: templates/repo/diff/box.tmpl:41

{{if and .PageIsPullFiles (not $.Repository.IsArchived)}}
    {{template "repo/diff/review_submit" .}}
{{end}}

This removes the IsShowingAllCommits condition, so the review button is now always rendered when viewing PR files (if repo isn't archived). The review_submit template internally handles the logic:

• review_submit.tmpl:2-4 - Button is disabled when not showing all commits
• review_submit.tmpl:10 - Modal is only rendered when showing all commits

This is a reasonable UX improvement: users can always see their pending comment count and understand that they need to view the full diff to submit.

---

Note: Pre-existing Pattern Outside PR Scope

• Addressed
• Dismissed

Location: routers/web/repo/issue_view.go:535

if !ctx.Data["IsStopwatchRunning"].(bool) {

This is a similar unsafe type assertion pattern, though in this case the value is set on the immediately preceding line (534), so it's guaranteed to exist. This is outside the scope of this PR but could benefit from the same defensive pattern for consistency.

---

Conclusion

No issues found that require fixes. The changes are well-implemented defensive programming improvements that fix the referenced issues without introducing bugs or security vulnerabilities.

---

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

[pedrogaudencio]

1. Type Assertion Fix in pull.go: comma-ok idiom applied - safe fallback to false if middleware doesn't set the value
2. Type Assertion Fix in pull_review.go: comma-ok idiom applied - semantically equivalent but prevents potential panics
3. Template Change in box.tmpl: UX improvement to always show review button - review_submit template already handles disabled state internally
4. Pre-existing Pattern Outside PR Scope: value is set on the immediately preceding line

[taoeffect]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

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

Click to expand review

---

Code Review: Fix Diff Comments

Issue 1: 🟡 Safe Type Assertion Improvement - routers/web/repo/pull.go:791-792

• Addressed
• Dismissed

Context: The change from:

if err = diff.LoadComments(ctx, issue, ctx.Doer, ctx.Data["ShowOutdatedComments"].(bool)); err != nil {

to:

showOutdatedComments, _ := ctx.Data["ShowOutdatedComments"].(bool)
if err = diff.LoadComments(ctx, issue, ctx.Doer, showOutdatedComments); err != nil {

Assessment: This is a correct bug fix. The original code could panic if ctx.Data["ShowOutdatedComments"] is nil (not set) or contains a non-bool value. The safe comma-ok idiom prevents this by defaulting to false when the value is not present or not a bool.

While the middleware SetShowOutdatedComments should always set this value before this handler runs (as seen in web.go:1241, 1249, 1250), this defensive coding is appropriate for robustness against future code changes where someone might forget to add the middleware.

No changes needed - this is a good fix.

---

Issue 2: 🟡 Safe Type Assertion Improvement - routers/web/repo/pull_review.go:172-173

• Addressed
• Dismissed

Context: The change from:

showOutdatedComments := origin == "timeline" || ctx.Data["ShowOutdatedComments"].(bool)

to:

showOutdatedComments, _ := ctx.Data["ShowOutdatedComments"].(bool)
showOutdatedComments = origin == "timeline" || showOutdatedComments

Assessment: This is also a correct bug fix with the same reasoning as Issue 1. The logic is preserved:

Scenario	Before	After
origin == "timeline", value is true	true (short-circuit)	true
origin == "timeline", value is false	true (short-circuit)	true
origin == "timeline", value is nil	PANIC	true
origin != "timeline", value is true	true	true
origin != "timeline", value is false	false	false
origin != "timeline", value is nil	PANIC	false

The test at routers/web/repo/pull_review_test.go:70-74 confirms this behavior is expected:

run("timeline is not affected by ShowOutdatedComments=false", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
    ctx.Data["ShowOutdatedComments"] = false
    renderConversation(ctx, preparedComment, "timeline")
    assert.Contains(t, resp.Body.String(), `<div id="code-comments-`)
})

No changes needed - this is a good fix.

---

Summary

These are both valid bug fixes that prevent potential runtime panics from failed type assertions. The changes:

1. Are safe: Use the comma-ok idiom to prevent panics when ctx.Data["ShowOutdatedComments"] is unset or not a bool
2. Preserve behavior: The logical behavior is unchanged in all scenarios where the value is properly set
3. Improve robustness: Even though middleware should set these values, defensive coding protects against future code paths that might skip the middleware

No issues found that require changes. The PR is ready to merge.

---

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

[taoeffect]

Great work @pedrogaudencio !