🐛 fix: keep full review thread context in filtered timelines#42
🐛 fix: keep full review thread context in filtered timelines#42
Conversation
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness gap in the timeline --after/--before filtering for PRs: review events can now be retained when their review threads contain updates that fall within the selected time window (so thread context isn’t lost).
Changes:
- Extend timeline filtering to consider “related timestamps” (e.g., review thread comment timestamps) in addition to the event’s own timestamp.
- Adjust PR review thread grouping/rendering so a window-matched thread shows full context, with per-comment window markers.
- Add CLI test coverage to validate that an older review is kept when its thread receives an in-window reply.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/test_cli.py |
Adds a regression test ensuring filtered PR timelines keep full review thread context when a thread update matches the window. |
src/gh_llm/pager.py |
Updates filtered collection logic to match events via related timestamps and threads the window through timeline fetches. |
src/gh_llm/models.py |
Adds related_timestamps to TimelineEvent to support window matching beyond the primary event timestamp. |
src/gh_llm/github_api.py |
Propagates timeline_window into parsing/rendering, collects review-thread comment timestamps, and renders window markers/full-context thread headers. |
Comments suppressed due to low confidence (2)
src/gh_llm/pager.py:414
- For PR timelines, this condition disables the early-stop optimization when
--afteris set, so_collect_filtered_from_end()will walk all the way to the first timeline page. On large PRs this can turn an incremental read into an O(total_pages) fetch, which is a noticeable performance/latency regression.
Consider keeping an early termination strategy for PRs (e.g., precomputing the set of review IDs/threads with any comment timestamps in-window from reviewThreads, then fetching only those missing review nodes by ID, or otherwise bounding how far back the backward walk must go).
if (
meta.kind != "pr"
and current_page.items
and timeline_window.after is not None
and current_page.items[0].timestamp <= timeline_window.after
):
break
src/gh_llm/pager.py:505
- Similarly, for PR timelines this disables the forward early-stop when
--beforeis set, which can force fetching every page from the start even when the requested window is small. That undermines the intended windowed/incremental behavior and can be expensive for long PR timelines.
If correctness requires pulling in older review nodes whose threads were updated in-window, consider a targeted strategy (derive “relevant review IDs” from reviewThreads and fetch only those older reviews) instead of scanning the entire timeline connection.
if (
meta.kind != "pr"
and current_page.items
and timeline_window.before is not None
and current_page.items[-1].timestamp >= timeline_window.before
):
break
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d7068826
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _event_matches_window(event: TimelineEvent, timeline_window: TimelineWindow) -> bool: | ||
| if _matches_window(event.timestamp, timeline_window): | ||
| return True | ||
| return any(_matches_window(timestamp, timeline_window) for timestamp in event.related_timestamps) |
There was a problem hiding this comment.
Restrict related timestamp matching for
--before windows
This change makes _event_matches_window include a review when any related_timestamps entry is inside the window, but reviews now carry full thread history from github_api._get_review_threads_by_review; as a result, a review submitted after a --before cutoff can still be included because the same thread has an older comment timestamp. That breaks time-filter correctness by showing out-of-range events whenever a thread spans multiple reviews.
Useful? React with 👍 / 👎.
| if is_minimized and not show_minimized_details and not force_full_context: | ||
| minimized_hidden_count += 1 |
There was a problem hiding this comment.
Preserve minimized-comment filtering in full-context threads
The new and not force_full_context condition bypasses minimized-comment hiding whenever a thread matches the selected window, so filtered timelines (--after/--before) can display minimized comments even when show_minimized_details is false. This regresses the existing CLI contract where minimized comments stay collapsed unless the user explicitly opts in via expand-minimized behavior.
Useful? React with 👍 / 👎.
修复 #41 可能忽略 review thread 的更新