Skip to content

feat(viewer): add session compare page (#1104) - #1175

Open
chethanuk wants to merge 1 commit into
alibaba:mainfrom
chethanuk:feat/issue-1104-viewer-session-compare
Open

feat(viewer): add session compare page (#1104)#1175
chethanuk wants to merge 1 commit into
alibaba:mainfrom
chethanuk:feat/issue-1104-viewer-session-compare

Conversation

@chethanuk

Copy link
Copy Markdown
Contributor

Description

Closes #1104. ocr session compare had no equivalent in the web viewer. This adds GET /r/{repo}/compare?before=&after=, rendering the same four buckets the CLI prints, plus a compare link on each session-list row pointing at the next-older session. Findings render with the existing/suggested code panels session.html already uses, so a finding whose fix is a patch shows the patch and not only the prose around it.

The reviewed-path partition (manifest Completed + Reused) moves to an exported session.ReviewedPaths that the CLI now delegates to, so the two cannot drift. The viewer's own Summary.FilesReviewed is Coverage.Selected, the set a run intended to review, which would report files an interrupted run never reached as clean.

Route registration moves out of StartServer into newMux, so the literal compare segment beating the {sessionID} wildcard is covered by a test that dispatches through the real ServeMux instead of calling the handler directly. The repo guard there now rejects \ alongside /: ServeMux unescapes each path segment, and \ is a separator on Windows.

Entry points are plain links, never a form. The viewer's CSP sets form-action 'none', which blocks a GET submission with no server-side error.

Divergences and limitations

  • An empty bucket still renders, as New (0), where the CLI skips a zero-count section. On a web page a heading that has vanished is indistinguishable from a broken render, so the viewer keeps it. Stated in the viewer docs, which are updated in all five locales.
  • A pair of sessions from different working directories is refused with 400. /r/{repo} looks like it already partitions by repo, but encodeRepoPath maps both separators to -, so /home/a/b and /home/a-b land in one viewer directory. The CLI errors on that pair; so does the page.
  • session.Compare keys a finding on its current path, so a file renamed between the two runs reads as one resolved plus one new. That is pre-existing in internal/session/compare.go; the page shows what Compare returns and does not correct it.
  • The viewer loads comments through LoadSession, which does not replay resume semantics the way the CLI's session.LoadComments does: a later checkpoint for the same fingerprint doesn't supersede an earlier one, and a subsequent review_item_failed doesn't drop it. On a session with no resumes this is a no-op; on a resumed or partially-failed run the compare page can show a finding the CLI's own comparison would already exclude. Pre-existing in LoadSession, not introduced here, but this page is the first place the gap can produce a visibly wrong bucket.
  • The Korean heading anchor changes with the heading: {#three-pages} becomes {#four-pages}. No in-repo link uses it, but an external link to #three-pages will stop resolving.
  • unsafeSegment also tightens two routes that already existed, so /r/my%5Crepo now returns 400 where it used to 404 (internal/viewer/compare_test.go:435). That is a behavior change on old routes inside a feature PR. Splitting it out would mean two ordered PRs for about ten lines the new route needs regardless, so it is here — say the word and I will separate it.
  • compare.html declares lang="en". The three older viewer templates still say lang="zh-CN" while serving English; fixing those is a separate change.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

make test (-race -count=1, 23 packages, 0 failures), make coverage (91.2% against the 90% threshold), and make check all pass on the rebased branch.

  • TestNewMux_RouteDispatch sends nine paths through the real mux, including /r/{repo}/compare against the {sessionID} wildcard, so the segment precedence is asserted rather than assumed.
  • Removing the unsafeSegment guard on the query parameters flips the traversal cases (../s1, %2e%2e%2fs1, a%2fb, a\b) from 400 to 404, which is the escape reaching filepath.Join.
  • TestHandleCompare covers the four buckets, a self-compare where three of them come back empty, missing and unknown session ids, HTML escaping of finding text, and a cross-repo pair (400).
  • TestReviewedPaths in internal/session pins the partition both callers now share; a case with selected:[a.go,b.go], only b.go completed, is what catches a handler that reads Coverage.Selected instead.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

Closes #1104


Update (post-review): the commit was amended (39acc8e) to fix a LoadSession path-injection false-positive CodeQL flagged (internal/viewer/store.go): validation now happens in a safeSegment helper whose returned value is what's joined, rather than checking a boolean and then joining the original argument — this is what let CodeQL's dataflow see the sanitizer. Also added a SessionID tie-breaker to ListSessions's sort so the sessions.html "compare" link's row-i/row-i+1 pairing is deterministic when two sessions share a timestamp. No functional/behavioral change to the routes described above; see qa-report.md for full detail.

`ocr session compare` had no equivalent in the web viewer (alibaba#1104). Adds
GET /r/{repo}/compare?before=&after= rendering the same four buckets the
CLI prints, plus a compare link on each session-list row pointing at the
next-older session. Findings render with the existing/suggested code
panels session.html already uses, so a finding whose fix is a patch shows
the patch and not only the prose around it.

The reviewed-path partition (manifest Completed + Reused) moves to an
exported session.ReviewedPaths that the CLI now delegates to, so the two
cannot drift. The viewer's own Summary.FilesReviewed is Coverage.Selected
- the intended set - which would report files an interrupted run never
reached as clean.

Route registration moves out of StartServer into newMux, so the literal
"compare" segment beating the {sessionID} wildcard is covered by a test
that dispatches through the real ServeMux instead of calling the handler
directly. The repo guard there now rejects "\" alongside "/": ServeMux
unescapes each path segment, and "\" is a separator on Windows.

encodeRepoPath maps both path separators to "-", so two distinct working
directories ("/home/a/b" and "/home/a-b") can share one on-disk repo
directory, and a compare request could otherwise pair sessions from
different repos. handleCompare (internal/viewer/handler.go:185) rejects
that case by comparing the two sessions' Summary.CWD, the same check the
CLI does in cmd/opencodereview/session_cmd.go's runSessionCompare, tested
at internal/viewer/compare_test.go:272.

LoadSession (internal/viewer/store.go) now re-validates encodedRepo and
sessionID itself through a new safeSegment helper and joins the value
safeSegment returns rather than its own copy of the argument, instead of
only trusting that every caller had already checked them with
unsafeSegment. Both current callers (handleSession's route and
handleCompare's before/after loop) already rejected traversal before
reaching LoadSession, but a validate-then-use-the-original-argument shape
across a function boundary is not something CodeQL's path-injection query
can credit as a sanitizer, and it left LoadSession unsafe to call from
anywhere that skipped that pre-check.

ListSessions's sort by Timestamp alone was not a total order: two
sessions with an equal (or both-zero) timestamp could swap position
between calls, and sessions.html's "compare" link pairs row i with row
i+1 as chronologically adjacent. The sort now breaks ties on SessionID so
that pairing is deterministic.

One divergence from the CLI is stated in the viewer docs: an empty
bucket still renders as "New (0)" instead of being skipped. Entry points
are plain links, never a form: the viewer's CSP sets form-action 'none',
which blocks GET submissions with no server-side error.

compare.html declares lang="en". The three older viewer templates still
say lang="zh-CN" while serving English; fixing those is a separate change.

Inherited limitation: session.Compare keys a finding on its current path,
so a file renamed between the two runs reads as one resolved plus one
new. The page shows what Compare returns and does not correct it.

Signed-off-by: ChethanUK <chethanuk@outlook.com>
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 7 selected item(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sesson compare added to viewer

1 participant