⚡ perf: avoid timeline bootstrap for lightweight PR views#30
Conversation
There was a problem hiding this comment.
💡 Codex Review
gh-llm/src/gh_llm/commands/pr.py
Line 404 in 1c885d5
For a closed-but-unmerged PR, this branch now forces checks=[], but render_mergeability_section() still relies on the check list to add blockers for required failing or pending status checks (src/gh_llm/render.py:262-278). As a result, pr view --show mergeability can flip from “blocked” to “allowed” whenever the only blocker is a required check, which is misleading for PRs that can still be reopened and merged.
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR makes gh-llm pr view / gh-llm pr checks truly “lightweight” when timeline output isn’t requested, avoiding the heavier timeline GraphQL bootstrap and reducing failure surface for metadata/checks-only views.
Changes:
- Introduces
build_context_from_meta()to construct a renderable PR context from metadata without loading timeline pages. - Updates
pr viewto only bootstrap timeline pages when--showincludestimeline, and updatespr checksto render from metadata context directly. - Adjusts frontmatter rendering to omit
timeline_events/page_size/total_pagesunless timeline data was actually loaded, and adds regression tests for these lightweight paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_cli.py |
Adds regression tests ensuring meta/checks/mergeability views and pr checks don’t trigger timeline GraphQL queries. |
src/gh_llm/render.py |
Hides timeline pagination/count fields in frontmatter unless timeline_loaded is true. |
src/gh_llm/pager.py |
Adds build_context_from_meta() and reuses it from build_initial() to avoid duplicated context construction. |
src/gh_llm/models.py |
Adds timeline_loaded to TimelineContext and ensures it round-trips via to_dict/from_dict. |
src/gh_llm/commands/pr.py |
Skips timeline bootstrap in pr view unless requested; makes pr checks build context from metadata and avoid timeline work. |
Comments suppressed due to low confidence (1)
src/gh_llm/commands/pr.py:583
cmd_pr_checks(andpr view --show checks) currently treat any non-OPEN PR as having no checks: it skipsfetch_checksand passesis_open=Falseintorender_checks_section. Howeverrender_checks_section’s message for closed PRs explicitly tells users to rerunpr checks ... --all, which will still not show checks with the currentis_openlogic. Consider either (a) allowing--allto fetch and render checks for closed/merged PRs (e.g., treatis_openasmeta.state == 'OPEN' or args.all), or (b) updating the closed-PR message/behavior so--allisn’t suggested when it can’t change the output.
meta = _resolve_pr_meta(client=client, args=args)
context = build_context_from_meta(meta=meta, page_size=DEFAULT_PAGE_SIZE)
checks = client.fetch_checks(meta.ref) if meta.state == "OPEN" else []
for line in render_checks_section(
context=context,
checks=checks,
show_all=bool(args.all),
is_open=(meta.state == "OPEN"),
):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ShigureNyako
left a comment
There was a problem hiding this comment.
结论:APPROVE。
我本地复跑了 uv run ruff check、uv run pyright、uv run pytest -q,并实际验证了这几个命令:
uv run gh-llm pr view 30 --repo ShigureLab/gh-llm --show metauv run gh-llm pr checks --pr 30 --repo ShigureLab/gh-llm --alluv run gh-llm pr view 30 --repo ShigureLab/gh-llm --show mergeability
这版改动把轻量视图和 timeline bootstrap 解耦得比较干净:
build_context_from_meta()的抽取让meta/checks/mergeability这些只依赖 PR 元信息的路径不再强绑 timelinerender_frontmatter()只在 timeline 实际加载后再输出timeline_events/page_size/total_pages,语义更准确- 回归测试也覆盖了
--show meta/--show checks/--show mergeability/pr checks这几个关键轻量路径
我这里没有看到新的阻塞性问题,可以合。
Motivation
Closes #25.
gh-llm pr view --show metaand other lightweight--showcombinations were still bootstrapping the timeline before deciding what to render. That made metadata-only and checks-only views pay the heavier timeline GraphQL cost and widened the failure surface for commands that should stay lightweight.What changed
build_context_from_meta()helper so PR metadata can be rendered without timeline bootstrappr viewto load timeline pages only when--showactually includestimelinepr checksto render from metadata context directly instead of building timeline state firsttimeline_events/page_size/total_pagesfrom frontmatter when timeline was not loaded, so lightweight output does not pretend those values were fetched--show meta,--show checks,--show mergeability, andpr checksValidation
uv run pytestuv run ruff check .uv run pyright src/gh_llm testsuv run gh-llm pr view 77938 --repo PaddlePaddle/Paddle --show metauv run gh-llm pr view 77938 --repo PaddlePaddle/Paddle --show checksuv run gh-llm pr checks --pr 77938 --repo PaddlePaddle/Paddle@SigureMo Please review this PR when you have time. Thanks!