mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-01 07:23:14 +00:00
fix: reviewer reviews full diff; fix review UI scroll + dark-mode composer (#1575)
* fix: reviewer reviews full diff; fix review UI scroll + dark-mode composer - Drop the 200K-char PR diff truncation. The head/tail slice silently dropped whole files out of the middle; the reviewer's large context window reviews the complete diff. The grouping pass uses the full diff too. Other #1567 caps (fetch_url, Slack, pagination, message queue) are kept. - Sidebar file/group click now lands flush at the top under virtualization: re-assert alignment after the Virtualizer's mid-scroll height reconciliation settles, instead of trusting scrollIntoView's estimate-based target. - Review chat composer textarea no longer shows a lighter square in dark mode (override the Textarea's dark:bg-input default with dark:bg-transparent). * fix: anchored finding card sits flush in the diff gutter, no side-panel overlap The card was positioned next to the anchor and clamped to window.innerWidth, so when the anchor sat near the diff/side-panel boundary the 412px card spilled across into the side panel. Pin it flush to the diff column's right edge and clamp its width to the column so it never overlaps the side panel and shrinks to fit a narrow column. * fix: anchored finding card sits over the side panel, not the diff Flip the horizontal anchor: place the card flush against the diff column's right edge and extend it rightward over the side panel, instead of leftward over the diff. Width still caps at the preferred size and shrinks to fit a narrow panel. * fix: anchor finding card to diff content edge + small-screen fallback - Anchor to the centered diff content's right edge instead of the full-width scroller, removing the centering-whitespace gap so the card sits closer to the diff. - Below xl the side panel is hidden and the diff fills the viewport, leaving no gutter; positioning against the content edge would collapse the card to ~0px. Fall back to overlaying the diff flush-right when there's no usable gutter (addresses Open SWE review finding f_f6ea77527c). * fix: anchor finding card next to the annotation, close to the hunk Anchor the card's left to the finding's annotation (anchorRect.right) instead of the diff content's right edge, so it sits right by the hunk and overlaps the diff edge rather than parking out in the gutter. Extends right over the side panel, clamped to stay on-screen (also keeps it readable below xl with no side panel). * fix: finding card width shrinks to fit a small side panel Size the card to the room to the right of the annotation (capped at the preferred width) so it narrows as the side panel shrinks instead of overflowing. Below a readable minimum, hold that width and shift left over the diff.
This commit is contained in:
Johannes du Plessis
GitHub
parent
ecf0898f51
commit
75e594c861
4 changed files with 87 additions and 51 deletions
|
|
@ -47,7 +47,7 @@ from .middleware import (
|
|||
refresh_github_proxy_before_model,
|
||||
settle_review_check_on_exit,
|
||||
)
|
||||
from .reviewer_diff import compute_diff_line_set, fetch_pr_diff, fetch_pr_metadata, truncate_diff
|
||||
from .reviewer_diff import compute_diff_line_set, fetch_pr_diff, fetch_pr_metadata
|
||||
from .reviewer_findings import (
|
||||
list_findings as list_findings_async,
|
||||
)
|
||||
|
|
@ -871,14 +871,15 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel:
|
|||
and bool(github_api_token)
|
||||
)
|
||||
|
||||
async def _fetch_diff_context() -> tuple[str, str, dict[str, dict[str, set[int]]] | None]:
|
||||
"""Return (full_diff, truncated_diff, line_set).
|
||||
async def _fetch_diff_context() -> tuple[str, dict[str, dict[str, set[int]]] | None]:
|
||||
"""Return (diff, line_set).
|
||||
|
||||
The line set is computed from the full diff so findings on any changed
|
||||
line are accepted. Only the prompt-facing text is truncated.
|
||||
The reviewer model has a large context window and reviews the full diff;
|
||||
it is not truncated. The line set is computed from the same diff so
|
||||
findings on any changed line are accepted.
|
||||
"""
|
||||
if not can_fetch_pr or github_api_token is None or not isinstance(pr_number, int):
|
||||
return "", "", None
|
||||
return "", None
|
||||
fetched_diff = await fetch_pr_diff(
|
||||
owner=repo_owner,
|
||||
repo=repo_name,
|
||||
|
|
@ -886,8 +887,8 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel:
|
|||
token=github_api_token,
|
||||
)
|
||||
if fetched_diff is None:
|
||||
return "", "", None
|
||||
return fetched_diff, truncate_diff(fetched_diff), compute_diff_line_set(fetched_diff)
|
||||
return "", None
|
||||
return fetched_diff, compute_diff_line_set(fetched_diff)
|
||||
|
||||
async def _fetch_pr_overview() -> tuple[str, str]:
|
||||
if not can_fetch_pr or github_api_token is None or not isinstance(pr_number, int):
|
||||
|
|
@ -981,7 +982,7 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel:
|
|||
_fetch_org_guidelines(),
|
||||
fetch_api_standards_skill(),
|
||||
)
|
||||
_, pr_diff_text, pr_diff_line_set = diff_context
|
||||
pr_diff_text, pr_diff_line_set = diff_context
|
||||
pr_title, pr_body = pr_overview
|
||||
config["configurable"]["diff_text"] = pr_diff_text
|
||||
config["configurable"]["diff_line_set"] = pr_diff_line_set
|
||||
|
|
|
|||
|
|
@ -197,10 +197,6 @@ def is_range_in_diff(
|
|||
return all(line in side_lines for line in range(start_line, end_line + 1))
|
||||
|
||||
|
||||
PR_DIFF_MAX_CHARS = 200_000
|
||||
PR_DIFF_TRUNCATION_MARKER = "\n... [PR diff truncated: {kept}/{total} chars]\n"
|
||||
|
||||
|
||||
async def fetch_pr_diff(
|
||||
*,
|
||||
owner: str,
|
||||
|
|
@ -216,8 +212,8 @@ async def fetch_pr_diff(
|
|||
source for ``add_finding``'s in-diff anchor validation and for
|
||||
``publish_review``'s 422 retry filter.
|
||||
|
||||
The full diff is returned — callers must use ``truncate_diff`` if they
|
||||
need to cap the text before feeding it to the LLM.
|
||||
The full diff is returned uncapped: the reviewer model has a large context
|
||||
window and reviews the complete diff.
|
||||
"""
|
||||
import httpx
|
||||
|
||||
|
|
@ -240,19 +236,6 @@ async def fetch_pr_diff(
|
|||
return response.text
|
||||
|
||||
|
||||
def truncate_diff(diff_text: str) -> str:
|
||||
"""Truncate diff text to ``PR_DIFF_MAX_CHARS`` with a visible marker.
|
||||
|
||||
Keeps the first half from the beginning and the second half from the end
|
||||
so file headers and recent changes are both visible.
|
||||
"""
|
||||
if len(diff_text) <= PR_DIFF_MAX_CHARS:
|
||||
return diff_text
|
||||
half = PR_DIFF_MAX_CHARS // 2
|
||||
marker = PR_DIFF_TRUNCATION_MARKER.format(kept=PR_DIFF_MAX_CHARS, total=len(diff_text))
|
||||
return diff_text[:half] + marker + diff_text[-half:]
|
||||
|
||||
|
||||
async def fetch_pr_metadata(
|
||||
*,
|
||||
owner: str,
|
||||
|
|
|
|||
|
|
@ -579,7 +579,7 @@ function ChatBody({
|
|||
}}
|
||||
placeholder="Ask anything about this PR…"
|
||||
rows={1}
|
||||
className="max-h-40 min-h-7 flex-1 resize-none rounded-none border-0 bg-transparent px-0 py-1 shadow-none focus-visible:border-transparent focus-visible:ring-0"
|
||||
className="max-h-40 min-h-7 flex-1 resize-none rounded-none border-0 bg-transparent px-0 py-1 shadow-none focus-visible:border-transparent focus-visible:ring-0 dark:bg-transparent"
|
||||
/>
|
||||
<IconButton
|
||||
type="button"
|
||||
|
|
|
|||
|
|
@ -130,6 +130,48 @@ function buildSelectionAttachments(
|
|||
]
|
||||
}
|
||||
|
||||
// Scroll a file card / group flush to the top of the diff scroller. Under
|
||||
// virtualization, scrollIntoView computes its target against estimated row
|
||||
// heights; scrolling past unmeasured files reconciles their real heights
|
||||
// mid-animation and the Virtualizer re-pins its scroll anchor, which leaves the
|
||||
// target off the top. Once the smooth scroll settles, re-assert alignment (now
|
||||
// against measured heights) until the target sits at the top or the budget runs
|
||||
// out. Respects the element's scroll-margin-top.
|
||||
function scrollCardToTop(el: HTMLElement, scroller: HTMLElement | null): void {
|
||||
el.scrollIntoView({ block: "start", behavior: "smooth" })
|
||||
if (!scroller) return
|
||||
let frames = 0
|
||||
let lastTop = Number.NaN
|
||||
let stableFrames = 0
|
||||
let corrections = 0
|
||||
const align = () => {
|
||||
if (frames++ > 240) return
|
||||
const top = scroller.scrollTop
|
||||
if (top === lastTop) stableFrames++
|
||||
else {
|
||||
stableFrames = 0
|
||||
lastTop = top
|
||||
}
|
||||
// Wait for the smooth scroll + height reconciliation to settle.
|
||||
if (stableFrames < 3) {
|
||||
requestAnimationFrame(align)
|
||||
return
|
||||
}
|
||||
const marginTop = parseFloat(getComputedStyle(el).scrollMarginTop) || 0
|
||||
const delta =
|
||||
el.getBoundingClientRect().top -
|
||||
scroller.getBoundingClientRect().top -
|
||||
marginTop
|
||||
if (Math.abs(delta) > 1 && corrections++ < 5) {
|
||||
el.scrollIntoView({ block: "start", behavior: "smooth" })
|
||||
stableFrames = 0
|
||||
lastTop = Number.NaN
|
||||
requestAnimationFrame(align)
|
||||
}
|
||||
}
|
||||
requestAnimationFrame(align)
|
||||
}
|
||||
|
||||
interface ResolvedGroup {
|
||||
index: number
|
||||
title: string
|
||||
|
|
@ -325,6 +367,7 @@ function ReviewBodyInner({
|
|||
const [anchorEl, setAnchorEl] = useState<HTMLElement | null>(null)
|
||||
const [userSelection, setUserSelection] = useState<UserSelection | null>(null)
|
||||
const [diffScrollEl, setDiffScrollEl] = useState<HTMLDivElement | null>(null)
|
||||
const diffScrollElRef = useRef<HTMLDivElement | null>(null)
|
||||
const groupRefs = useRef<Record<number, HTMLDivElement | null>>({})
|
||||
const [diffStyle, setDiffStyleState] = useState<DiffStyle>(() =>
|
||||
readStoredDiffStyle()
|
||||
|
|
@ -516,19 +559,15 @@ function ReviewBodyInner({
|
|||
setSelectedFile(path)
|
||||
setExpandedFiles((prev) => ({ ...prev, [path]: true }))
|
||||
requestAnimationFrame(() => {
|
||||
fileRefs.current[path]?.scrollIntoView({
|
||||
block: "start",
|
||||
behavior: "smooth",
|
||||
})
|
||||
const el = fileRefs.current[path]
|
||||
if (el) scrollCardToTop(el, diffScrollElRef.current)
|
||||
})
|
||||
}, [])
|
||||
|
||||
const scrollToGroup = useCallback((index: number) => {
|
||||
requestAnimationFrame(() => {
|
||||
groupRefs.current[index]?.scrollIntoView({
|
||||
block: "start",
|
||||
behavior: "smooth",
|
||||
})
|
||||
const el = groupRefs.current[index]
|
||||
if (el) scrollCardToTop(el, diffScrollElRef.current)
|
||||
})
|
||||
}, [])
|
||||
|
||||
|
|
@ -544,7 +583,9 @@ function ReviewBodyInner({
|
|||
// anchored finding card can position against it.
|
||||
const scrollerProbe = useCallback((node: HTMLDivElement | null) => {
|
||||
const scroller = node?.parentElement?.parentElement
|
||||
setDiffScrollEl(scroller instanceof HTMLDivElement ? scroller : null)
|
||||
const el = scroller instanceof HTMLDivElement ? scroller : null
|
||||
diffScrollElRef.current = el
|
||||
setDiffScrollEl(el)
|
||||
}, [])
|
||||
|
||||
const registerSection = useCallback(
|
||||
|
|
@ -1264,15 +1305,20 @@ function FindingRailMarker({
|
|||
|
||||
const FINDING_CARD_WIDTH = 412
|
||||
const FINDING_CARD_GAP = 12
|
||||
// If the room beside the annotation is tighter than this, hold this width and
|
||||
// overlay the diff rather than shrinking into an unreadable sliver (e.g. a very
|
||||
// narrow side panel, or no panel at all below `xl`).
|
||||
const FINDING_CARD_MIN_WIDTH = 320
|
||||
|
||||
const FINDING_CARD_CLASS =
|
||||
"flex max-h-[70vh] w-[412px] flex-col overflow-hidden rounded-lg border border-border bg-background shadow-2xl"
|
||||
"flex max-h-[70vh] flex-col overflow-hidden rounded-lg border border-border bg-background shadow-2xl"
|
||||
|
||||
// Pinned to the right of the diff column (toward the side panel), vertically
|
||||
// aligned with the finding's annotation. The diff scroller is only as wide as
|
||||
// <main>, so the card is viewport-fixed (clamped to the window width) to sit in
|
||||
// the right gutter rather than overlapping the diff, and tracks the anchor as
|
||||
// the diff scrolls (rAF-throttled). Hidden while the anchor is out of view.
|
||||
// Anchored just to the right of the finding's annotation, close to the hunk, and
|
||||
// extending right over the side panel. Its width fits the room available to the
|
||||
// right (capped at the preferred size), so it narrows as the side panel shrinks;
|
||||
// when that room gets too tight to read it holds a minimum width and overlays
|
||||
// the diff (e.g. below `xl`, with no side panel). Tracks the anchor as the diff
|
||||
// scrolls (rAF-throttled); hidden while the anchor is out of view.
|
||||
function AnchoredFindingCard({
|
||||
detail,
|
||||
finding,
|
||||
|
|
@ -1307,13 +1353,19 @@ function AnchoredFindingCard({
|
|||
card.style.visibility = "hidden"
|
||||
return
|
||||
}
|
||||
const left = Math.max(
|
||||
FINDING_CARD_GAP,
|
||||
Math.min(
|
||||
anchorRect.right + FINDING_CARD_GAP,
|
||||
window.innerWidth - FINDING_CARD_WIDTH - FINDING_CARD_GAP
|
||||
)
|
||||
)
|
||||
// Sit just right of the finding's annotation (close to the hunk). Width
|
||||
// fits the room to its right so the card narrows as the side panel shrinks
|
||||
// instead of overflowing. When that room is too tight to read, hold a
|
||||
// minimum width and shift left over the diff.
|
||||
const rightBound = window.innerWidth - FINDING_CARD_GAP
|
||||
const minLeft = scrollerRect.left + FINDING_CARD_GAP
|
||||
let left = anchorRect.right + FINDING_CARD_GAP
|
||||
let width = Math.min(FINDING_CARD_WIDTH, rightBound - left)
|
||||
if (width < FINDING_CARD_MIN_WIDTH) {
|
||||
width = Math.min(FINDING_CARD_WIDTH, rightBound - minLeft)
|
||||
left = Math.max(minLeft, rightBound - width)
|
||||
}
|
||||
card.style.width = `${width}px`
|
||||
const top = Math.max(
|
||||
scrollerRect.top + FINDING_CARD_GAP,
|
||||
Math.min(
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue