feat(viewer): mark comments as fixed/solved/ignored and hide them - #1101
feat(viewer): mark comments as fixed/solved/ignored and hide them#1101nitishagar wants to merge 1 commit into
Conversation
|
🔍 OpenCodeReview found 7 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Marks exist only for sessions that exist. | ||
| if _, err := os.Stat(filepath.Join(root, repo, sessionID+".jsonl")); err != nil { | ||
| http.Error(w, "session not found", http.StatusNotFound) | ||
| return | ||
| } |
There was a problem hiding this comment.
For POST requests, the session existence check (os.Stat) runs before the Content-Type validation below (line 132). This creates an information oracle: an attacker can enumerate valid session IDs by sending POST requests with any Content-Type (e.g., text/plain) and distinguishing 404 (session not found) from 415 (wrong content type). Since the Content-Type check is explicitly documented as a CSRF defense, the existence check should be moved after it for POST requests so that unauthorized requests are rejected before touching the filesystem.
Consider restructuring so that for POST, the Content-Type check happens first, then the session existence check. DELETE does not need this reordering since it has no body-based CSRF vector.
| func writeMarksResponse(w http.ResponseWriter, markID, state string) { | ||
| if err := json.NewEncoder(w).Encode(map[string]any{ | ||
| "ok": true, | ||
| "mark_id": markID, | ||
| "state": state, | ||
| }); err != nil { | ||
| http.Error(w, fmt.Sprintf("encode response: %v", err), http.StatusInternalServerError) | ||
| } | ||
| } |
There was a problem hiding this comment.
If json.NewEncoder(w).Encode() fails after partially writing the response body, the HTTP headers (200 OK, Content-Type: application/json) have already been flushed. The subsequent http.Error call will attempt to write a 500 status and text/plain content type, but these will be silently ignored since headers were already sent. The client would receive a truncated JSON body with a 200 status code.
To fix this, either marshal to a buffer first and write in one shot, or simply log the error (the client will see a broken response regardless):
func writeMarksResponse(w http.ResponseWriter, markID, state string) {
resp := map[string]any{"ok": true, "mark_id": markID, "state": state}
data, err := json.Marshal(resp)
if err != nil {
http.Error(w, fmt.Sprintf("encode response: %v", err), http.StatusInternalServerError)
return
}
w.Write(data)
}| let activeCategory = 'all'; | ||
| // Hide-marked is presentation, not data: the default (on) matches the | ||
| // mark-and-hide workflow, and the choice persists per browser only. | ||
| let hideMarked = hideMarkedToggle ? (localStorage.getItem('ocr-viewer-hide-marked') || '1') === '1' : false; |
There was a problem hiding this comment.
localStorage safety: Both localStorage.getItem and localStorage.setItem can throw in certain browser environments (e.g., private browsing with quota exceeded, or storage disabled by policy). An unhandled exception here would prevent the entire filter/marks feature from initializing. Wrap localStorage access in try-catch to degrade gracefully.
Suggestion:
| let hideMarked = hideMarkedToggle ? (localStorage.getItem('ocr-viewer-hide-marked') || '1') === '1' : false; | |
| let hideMarked = false; | |
| if (hideMarkedToggle) { | |
| try { | |
| hideMarked = (localStorage.getItem('ocr-viewer-hide-marked') || '1') === '1'; | |
| } catch (_) { | |
| hideMarked = true; // default when storage is unavailable | |
| } | |
| } |
| card.dataset.mark = state; | ||
| card.setAttribute('data-mark', state); |
There was a problem hiding this comment.
Redundant DOM manipulation: Setting both card.dataset.mark and card.setAttribute('data-mark', state) is redundant — they are two interfaces to the same underlying attribute. Either one suffices. Since the rest of this file reads marks via card.dataset.mark, using only card.dataset.mark = state would be consistent and sufficient.
Additionally, the state value is interpolated directly into a CSS class name ('mark-' + state) without validation on the client side. While the server validates against validMarkStates, if the response were ever tampered with or a bug introduced server-side, an unexpected value could inject arbitrary CSS classes. Consider validating state against an allowlist before constructing the class name.
Suggestion:
| card.dataset.mark = state; | |
| card.setAttribute('data-mark', state); | |
| card.dataset.mark = state; |
| } else { | ||
| delete card.dataset.mark; | ||
| card.removeAttribute('data-mark'); | ||
| if (chip) { | ||
| chip.hidden = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale chip text/class on clear: When clearing a mark (state is falsy), the chip is hidden but its textContent and className are not reset. If the chip becomes visible again through any path other than applyMarkState with a truthy state (e.g., a future code change, or a DOM inspector toggle), it would display the stale previous mark text and color class. Reset them defensively:
Suggestion:
| } else { | |
| delete card.dataset.mark; | |
| card.removeAttribute('data-mark'); | |
| if (chip) { | |
| chip.hidden = true; | |
| } | |
| } | |
| } else { | |
| delete card.dataset.mark; | |
| card.removeAttribute('data-mark'); | |
| if (chip) { | |
| chip.textContent = ''; | |
| chip.className = 'comment-badge mark-chip'; | |
| chip.hidden = true; | |
| } | |
| } |
| .catch(function(err) { | ||
| console.error('[ocr viewer]', err); | ||
| }); |
There was a problem hiding this comment.
Async Error Handling: When a mark POST or clear-all DELETE request fails, the error is only logged to console.error with no user-facing feedback. Users will have no indication that their action failed to persist, and the UI may show stale state (e.g., a mark appears applied locally but was never saved). Consider adding a lightweight user-visible notification (e.g., a toast, or temporarily reverting the UI change) so failures are not silent.
| hideMarkedToggle.checked = hideMarked; | ||
| hideMarkedToggle.addEventListener('change', function() { | ||
| hideMarked = hideMarkedToggle.checked; | ||
| localStorage.setItem('ocr-viewer-hide-marked', hideMarked ? '1' : '0'); |
There was a problem hiding this comment.
Same localStorage safety issue: This setItem call can also throw if storage is full or disabled. Wrap in try-catch to avoid breaking the toggle handler.
Suggestion:
| localStorage.setItem('ocr-viewer-hide-marked', hideMarked ? '1' : '0'); | |
| try { localStorage.setItem('ocr-viewer-hide-marked', hideMarked ? '1' : '0'); } catch (_) {} |
5028957 to
b518b4e
Compare
wu21-web
left a comment
wu21-web
left a comment
There was a problem hiding this comment.
- Concurrent UI mutations can leave the page inconsistent with disk
- Documentation changes incomplete
- Too many code comments
- Strict JSON parsing accepts trailing JSON values
| // MarkID is the stable identity used to bind viewer marks (fixed / | ||
| // solved / ignored) to this comment. Session files are immutable, so the | ||
| // identity is reload-stable: record uuid + comment index for records that | ||
| // carry a uuid, a full-field hash plus an occurrence counter otherwise. | ||
| MarkID string `json:"-"` |
There was a problem hiding this comment.
Can you please clean up those code comments?
| // MarkID is the stable identity used to bind viewer marks (fixed / | |
| // solved / ignored) to this comment. Session files are immutable, so the | |
| // identity is reload-stable: record uuid + comment index for records that | |
| // carry a uuid, a full-field hash plus an occurrence counter otherwise. | |
| MarkID string `json:"-"` | |
| MarkID string `json:"-"` |
There was a problem hiding this comment.
The documentation changes are not complete. At the very least, add the details about this feature to the corresponding site documentation section. Currently deployed at https://open-codereview.ai/docs/viewer
| if err := dec.Decode(&req); err != nil { | ||
| http.Error(w, fmt.Sprintf("invalid request body: %v", err), http.StatusBadRequest) | ||
| return |
There was a problem hiding this comment.
EOF after the first object
| if err := dec.Decode(&req); err != nil { | |
| http.Error(w, fmt.Sprintf("invalid request body: %v", err), http.StatusBadRequest) | |
| return | |
| if err := dec.Decode(&req); err != nil { | |
| http.Error(w, fmt.Sprintf("invalid request body: %v", err), http.StatusBadRequest) | |
| return | |
| } | |
| if err := dec.Decode(&struct{}{}); err != io.EOF { | |
| http.Error(w, "request body must contain exactly one JSON object", http.StatusBadRequest) | |
| return | |
| } |
| // The server whitelists states; this mirrors the same set so a tampered | ||
| // or buggy response can never grow an arbitrary class name on the chip. | ||
| const knownMarkStates = { fixed: true, solved: true, ignored: true }; | ||
|
|
There was a problem hiding this comment.
| // The server whitelists states; this mirrors the same set so a tampered | |
| // or buggy response can never grow an arbitrary class name on the chip. | |
| const knownMarkStates = { fixed: true, solved: true, ignored: true }; | |
| // The server whitelists states; this mirrors the same set so a tampered | |
| // or buggy response can never grow an arbitrary class name on the chip. | |
| const knownMarkStates = { fixed: true, solved: true, ignored: true }; | |
| let marksMutationQueue = Promise.resolve(); | |
| function enqueueMarksMutation(operation) { | |
| const result = marksMutationQueue.then(operation); | |
| marksMutationQueue = result.catch(function() { | |
| }); | |
| return result; | |
| } |
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "mime" | ||
| "net/http" | ||
| "os" | ||
| "path/filepath" |
There was a problem hiding this comment.
| import ( | |
| "encoding/json" | |
| "fmt" | |
| "mime" | |
| "net/http" | |
| "os" | |
| "path/filepath" | |
| import ( | |
| "encoding/json" | |
| "fmt" | |
| "io" | |
| "mime" | |
| "net/http" | |
| "os" | |
| "path/filepath" |
Walking through a long comment list in the session viewer had no way
to track progress: every finding stayed on screen no matter what was
already addressed. This adds per-comment marks (fixed / solved /
ignored, mutually exclusive; replace-on-set, clear to undo) with a
Hide marked toggle (default on, persisted per browser) and a Clear
all marks control.
Marks are viewer state, not session data: they live in a per-session
sidecar file <sessionID>.marks.json next to the immutable session
jsonl (a different suffix, so session listings never see it), written
atomically via temp+rename with 0600 permissions. The session store
itself is never opened for write; deleting the sidecar restores the
unmarked view exactly.
Each comment is identified by a stable mark ID derived from its owning
record's uuid plus its index within that record (legacy uuid-less
records fall back to a full-field hash with an occurrence counter, so
exact duplicates stay distinct). Mutation goes through two new
endpoints, POST and DELETE /r/{repo}/{sessionID}/marks, with the same
path validation as the existing routes, a 64 KiB body cap, strict
single-object JSON decoding (trailing data after the object is a
400), and a required JSON content type; responses echo the
authoritative state so concurrent tabs converge on their next
interaction. A corrupt or unreadable marks file degrades to no marks
rather than breaking the page.
On the client, mark mutations run through a serial promise queue, so
rapid clicks — or a set racing Clear all — apply responses in order
and a stale echo can never resurrect a replaced or cleared mark.
Docs: the Session Viewer page on the docs site (en/ja/ko/ru/zh)
gains a Review comments section covering the cards, filters, marks,
the hide-marked toggle, and the marks sidecar in the on-disk layout.
Closes alibaba#825
b518b4e to
e806fe5
Compare
|
Thanks @nitishagar — the But I have to hold the line on the architecture, and I owe you a better explanation than the issue thread gave. Keep the feature, drop the server. Re-reading @iredmail's scenario — "i may leave and come back to continue" — the persistence he needs is "close the tab, come back tomorrow." Two smaller things for the reduced version:
The thread stalled with us asking for a screenshot and going quiet — not a fair signal to build 1200 lines against, and that's on us. This isn't a no. I want the feature and I'd like you to land it; if you're up for the reduced form I'll review promptly. If you'd rather not, say so and I'll take the Also please update |
|
Thanks for making it happen guys. :) |
4 participants
What
Implements #825: in the
ocr viewersession page, each review comment card gains Fixed / Solved / Ignored buttons (mutually exclusive; setting one replaces another, Clear undoes) and the toolbar gains a Hide marked checkbox (default on, persisted per browser) plus Clear all marks. This is the workflow the issue asks for: walk the findings, mark as you fix, hide what is done, toggle back anytime to see everything.The session data stays untouched — marks are viewer state stored outside the read-only session store, per the design framing in the issue thread.
How
uuid+ its index within that record. Session files are immutable, so the identity is reload-stable and collision-free; legacy uuid-less records fall back to a full-field hash with an occurrence counter (exact duplicates stay distinct).<sessionID>.marks.jsonbeside the jsonl (different suffix — invisible to the session listing). Written atomically (temp + rename, 0600); corrupt/missing file degrades to "no marks"; deleting it restores the pristine view. The viewer never opens a.jsonlfor write (asserted by a test).POST /r/{repo}/{sessionID}/marks({mark_id, state}, empty state clears; response echoes the authoritative state so divergent tabs converge) andDELETE…/marks(clear all). Same path validation as existing routes, 64 KiB body cap, strict JSON decoding, JSON content-type required, wrong verbs get 405 +Allow.session.jswires the buttons via fetch and applies the echoed state; hide-marked composes with the existing severity/category filters through the same group-visibility/count path (per-file counts and the empty state reflect hidden cards).Semantics chosen (answering the open question from the thread)
fixed/solved/ignoredare alternatives, replace-on-set.Screenshot-friendly summary of placement: the toggle and Clear-all sit on the existing filter bar above the comment groups; the mark buttons sit at the bottom of each comment card; a marked card shows a colored state chip next to its category/severity badges.
Tests
Store (identity stability/duplicates/unicode fallback), marks file (round trip, corrupt ⇒ empty, 0600, atomicity, failed write keeps previous state), API (set/clear/echo, 400/404/405+Allow cases, traversal, concurrent posts, write failure 500, content-type enforcement), rendering (marked chip +
data-mark, unmarked identical to today, zero-comment session, session jsonl byte-immutability). Full suite green under-race.Closes #825