Repository navigation
Make sidebar PR lookups event-driven - #2453
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds per-panel on-disk PR caching, a per-panel force-probe signal, monotonic timestamping, and prompt/poll-loop coordination to reduce redundant Changes
Sequence DiagramsequenceDiagram
participant Shell as Shell / Prompt Hook
participant Cache as On-Disk Cache
participant Signal as Force-Signal File
participant Poll as PR Poll Loop
participant GitHub as GitHub (gh)
Shell->>Cache: Check cached branch/result/timestamp
alt Cache valid & fresh
Cache-->>Shell: Return cached PR result
else Cache missing/stale or context changed
Shell->>Signal: Create per-panel force signal
Signal-->>Poll: Poll loop detects signal file
Poll->>Signal: Delete signal file
Poll->>GitHub: Run `gh pr view` / `gh pr list`
GitHub-->>Poll: PR result / "no pull requests"
Poll->>Cache: Write result + timestamp (or "none")
Poll-->>Shell: Update panel UI/state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47c50458ba
ℹ️ 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".
| elif (( _CMUX_PR_FORCE )); then | ||
| should_restart_pr_poll=1 | ||
| if [[ -n "$_CMUX_PR_POLL_PID" ]] && kill -0 "$_CMUX_PR_POLL_PID" 2>/dev/null; then | ||
| should_signal_pr_probe=1 | ||
| else | ||
| should_restart_pr_poll=1 |
There was a problem hiding this comment.
In bash, a cached no-PR result can now remain stale after explicit gh actions (for example gh pr create) because _cmux_prompt_command only triggers an immediate probe when _CMUX_PR_FORCE is set, and this path does not get set for bash command executions. Since the poll loop is no longer restarted on every preexec, the loop keeps re-emitting cached none until the cache TTL expires (default 300s), so the sidebar can stay incorrect for several minutes right after the user creates/updates a PR.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Resources/shell-integration/cmux-zsh-integration.zsh (1)
659-667: Minor: Inconsistent output method for TSV result.Line 663 uses
printffor the TSV result while lines 660-662 useprint -r --. This works but is inconsistent with the rest of the file.♻️ Use print -r consistently
if [[ -n "$prefix" ]]; then print -r -- "$branch" >| "$branch_file" print -r -- "$repo_path" >| "$repo_file" print -r -- "$now" >| "$timestamp_file" - printf '%s\t%s\t%s\t%s\n' "pr" "$number" "$state" "$url" >| "$result_file" + print -r -- "pr\t$number\t$state\t$url" >| "$result_file" /bin/rm -f -- "$no_pr_branch_file" >/dev/null 2>&1 || true fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-zsh-integration.zsh` around lines 659 - 667, Replace the printf call that writes to "$result_file" with the same print utility used elsewhere for consistency: use print -r -- to write a tab-separated line containing "pr", $number, $state and $url (preserving the tabs between fields) and keep the same output redirection (>| "$result_file"); update the block around the prefix check that currently sets _CMUX_PR_LAST_BRANCH and _CMUX_PR_NO_PR_BRANCH so it still removes "$no_pr_branch_file" and writes the branch/repo/timestamp files unchanged.Resources/shell-integration/cmux-bash-integration.bash (2)
495-519: Consider extracting duplicate cache-write logic.The cache-write blocks at lines 495-501, 508-514, and 539-545 are nearly identical. While this works correctly, extracting a helper like
_cmux_pr_write_cache "$branch" "$repo_path" "$result"would reduce duplication and make future maintenance easier.♻️ Example helper extraction
_cmux_pr_write_cache() { local branch="$1" repo_path="$2" result="$3" is_no_pr="${4:-0}" local prefix="" now="" prefix="$(_cmux_pr_cache_prefix 2>/dev/null || true)" [[ -n "$prefix" ]] || return 0 now="$(_cmux_now)" printf '%s\n' "$branch" >| "${prefix}.branch" printf '%s\n' "$repo_path" >| "${prefix}.repo" printf '%s\n' "$now" >| "${prefix}.timestamp" printf '%s\n' "$result" >| "${prefix}.result" if (( is_no_pr )); then printf '%s\n' "$branch" >| "${prefix}.no-pr-branch" else /bin/rm -f -- "${prefix}.no-pr-branch" >/dev/null 2>&1 || true fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-bash-integration.bash` around lines 495 - 519, There are three duplicated cache-write blocks writing branch/repo/timestamp/result/no-pr-branch; extract them into a helper like _cmux_pr_write_cache and call it from each place (replace the repeated printf/.../rm logic with a single call). The helper should accept parameters (branch, repo_path, result, is_no_pr) and internally obtain prefix via _cmux_pr_cache_prefix and timestamp via _cmux_now, write the four files ("${prefix}.branch", "${prefix}.repo", "${prefix}.timestamp", "${prefix}.result") and set or remove "${prefix}.no-pr-branch" based on is_no_pr; update call sites in the branches that use _cmux_pr_output_indicates_no_pull_request, the earlier cache-write block, and the later one before invoking _cmux_clear_pr_for_panel so behavior and variables (branch, repo_path, result) remain identical.
339-347: Cache and signal files use predictable /tmp paths.The cache files at
/tmp/cmux-pr-cache-${CMUX_PANEL_ID}.*and signal file at/tmp/cmux-pr-force-${CMUX_PANEL_ID}are world-readable/writable by default. While the UUID-based naming makes prediction unlikely, on shared multi-user systems this could theoretically allow another user to:
- Read cached branch/repo paths (minor information disclosure)
- Create spurious signal files to trigger extra
ghqueries (minor DoS)For a desktop terminal app this is likely acceptable, but consider using
$XDG_RUNTIME_DIR(if available) or creating files with restrictive permissions if this ever targets server environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-bash-integration.bash` around lines 339 - 347, The temp paths returned by _cmux_pr_cache_prefix and _cmux_pr_force_signal_path are world-accessible; change them to use a per-user runtime directory (prefer $XDG_RUNTIME_DIR) or a locked per-user dir under /tmp and ensure directory permissions are restrictive before returning file paths: inside _cmux_pr_cache_prefix and _cmux_pr_force_signal_path, choose dir="${XDG_RUNTIME_DIR:-/tmp/cmux-${UID}}" (or similar), create it with mkdir -p -m 0700 if it doesn't exist, and return files under that dir (and ensure any code that creates the actual cache/signal files uses umask 077 or creates files with mode 600/700) so the cache and signal files are not world-readable/writable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 495-519: There are three duplicated cache-write blocks writing
branch/repo/timestamp/result/no-pr-branch; extract them into a helper like
_cmux_pr_write_cache and call it from each place (replace the repeated
printf/.../rm logic with a single call). The helper should accept parameters
(branch, repo_path, result, is_no_pr) and internally obtain prefix via
_cmux_pr_cache_prefix and timestamp via _cmux_now, write the four files
("${prefix}.branch", "${prefix}.repo", "${prefix}.timestamp",
"${prefix}.result") and set or remove "${prefix}.no-pr-branch" based on
is_no_pr; update call sites in the branches that use
_cmux_pr_output_indicates_no_pull_request, the earlier cache-write block, and
the later one before invoking _cmux_clear_pr_for_panel so behavior and variables
(branch, repo_path, result) remain identical.
- Around line 339-347: The temp paths returned by _cmux_pr_cache_prefix and
_cmux_pr_force_signal_path are world-accessible; change them to use a per-user
runtime directory (prefer $XDG_RUNTIME_DIR) or a locked per-user dir under /tmp
and ensure directory permissions are restrictive before returning file paths:
inside _cmux_pr_cache_prefix and _cmux_pr_force_signal_path, choose
dir="${XDG_RUNTIME_DIR:-/tmp/cmux-${UID}}" (or similar), create it with mkdir -p
-m 0700 if it doesn't exist, and return files under that dir (and ensure any
code that creates the actual cache/signal files uses umask 077 or creates files
with mode 600/700) so the cache and signal files are not
world-readable/writable.
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 659-667: Replace the printf call that writes to "$result_file"
with the same print utility used elsewhere for consistency: use print -r -- to
write a tab-separated line containing "pr", $number, $state and $url (preserving
the tabs between fields) and keep the same output redirection (>|
"$result_file"); update the block around the prefix check that currently sets
_CMUX_PR_LAST_BRANCH and _CMUX_PR_NO_PR_BRANCH so it still removes
"$no_pr_branch_file" and writes the branch/repo/timestamp files unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01c346ea-1536-4203-999e-31bcec9cc92a
📒 Files selected for processing (3)
Resources/shell-integration/cmux-bash-integration.bashResources/shell-integration/cmux-zsh-integration.zshtests/test_issue_1138_sidebar_pr_polling.py
Greptile SummaryThis PR replaces the unconditional "stop poll loop on every preexec, restart on every precmd" pattern with a persistent background loop that uses per-panel file-based caches ( Key changes:
Issues found:
Confidence Score: 4/5Safe to merge after fixing the zsh EPOCHSECONDS fallback; everything else is minor polish or test hardening. The zsh ${EPOCHSECONDS:-0} fallback is a real correctness defect: when EPOCHSECONDS is unavailable, cache_age is always 0 so the TTL guard is permanently satisfied and stale PR state is never refreshed. All other findings are P2. Once the fallback is corrected to :-$SECONDS the PR is solid. Resources/shell-integration/cmux-zsh-integration.zsh — the EPOCHSECONDS fallback appears in at least two locations and needs to be updated to ${EPOCHSECONDS:-$SECONDS}. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[precmd / prompt_command] --> B{PR poll loop running?}
B -- No or PWD changed --> C[stop_pr_poll_loop\nkill bg process\nrm signal file\nclear cache files]
C --> D[start_pr_poll_loop\nnew background loop]
B -- Yes PR_FORCE=1 --> E[pr_request_probe\ntouch signal file]
B -- Yes no force --> F[loop continues]
D --> G[Poll Iteration]
E --> G
F --> G
G --> H{Signal file present?}
H -- Yes --> I[force_probe=1\nrm signal file]
H -- No --> J[force_probe=0]
I --> K[run_pr_probe_with_timeout]
J --> K
K --> L{Cache valid?\nsame branch+repo\nage less than TTL\nforce_probe=0}
L -- Hit --> M[emit cached result\nno gh call]
L -- Miss or forced --> N[gh pr view BRANCH]
N --> O{gh result}
O -- PR found --> P[write cache\nreport_pr to sidebar]
O -- No PR or empty --> Q[write cache none\nclear_pr sidebar]
O -- Transient error --> R[return 1\nkeep old badge\nno cache write]
M --> S[sleep interval\nchecking signal every 1s]
P --> S
Q --> S
R --> S
S --> G
Reviews (1): Last reviewed commit: "Make sidebar PR lookups event-driven" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| [[ -n "$CMUX_PANEL_ID" ]] || return 0 | ||
|
|
||
| local branch repo_slug="" gh_output="" gh_error="" err_file="" number state url status_opt="" gh_status | ||
| local now="${EPOCHSECONDS:-0}" |
There was a problem hiding this comment.
EPOCHSECONDS:-0 fallback causes cache to never expire
When EPOCHSECONDS is unavailable in zsh (or the zsh/datetime module is not loaded), now is set to 0. Since the cache file is then written with timestamp 0, every subsequent read computes cache_age = 0 - 0 = 0. The condition (( cache_age >= 0 && cache_age < cache_ttl )) is permanently true, so the cache never expires and stale PR state would be displayed indefinitely — no live refresh until a branch change forces a _cmux_pr_cache_clear.
The bash side correctly falls back to $SECONDS (which advances over time, making cache_age positive and eventually exceeding the TTL). The zsh side should do the same:
| local now="${EPOCHSECONDS:-0}" | |
| local now="${EPOCHSECONDS:-$SECONDS}" |
$SECONDS is a POSIX shell special variable available in zsh even without zsh/datetime, so it's a safe fallback that preserves expiry semantics.
| } | ||
|
|
||
| _cmux_pr_cache_clear() { | ||
| local prefix="" |
There was a problem hiding this comment.
EPOCHSECONDS:-0 fallback in zsh debug log silently drops timing
The zsh _cmux_pr_debug_log also uses local now="${EPOCHSECONDS:-0}" (zsh file, same pattern). While the debug log is non-functional code, it's worth noting that the :-0 fallback in the zsh integration appears consistently in three places: _cmux_pr_debug_log, _cmux_report_pr_for_path, and _cmux_run_pr_probe_with_timeout (which reads $EPOCHSECONDS directly). All three should use ${EPOCHSECONDS:-$SECONDS} to match the bash fallback strategy.
| "${prefix}.repo" \ | ||
| "${prefix}.result" \ | ||
| "${prefix}.timestamp" \ | ||
| "${prefix}.no-pr-branch" \ |
There was a problem hiding this comment.
/tmp/cmux-pr-debug.log is written to without any per-user isolation. On multi-user systems this is a TOCTOU symlink risk: a local attacker could create /tmp/cmux-pr-debug.log -> /home/victim/.bashrc before the log is opened, causing >> to append to an arbitrary file. The same issue exists in the zsh integration.
Consider using a user-isolated path when debug mode is enabled:
printf '%s\tbranch=%s\tevent=%s\n' "$now" "$branch" "$event" >> "/tmp/cmux-pr-debug-${UID}.log"This only matters when _CMUX_PR_DEBUG=1, so it isn't a default-path issue, but it's worth hardening before the debug flag gets documented.
| if [[ -n "$_CMUX_PR_POLL_PID" ]] && kill -0 "$_CMUX_PR_POLL_PID" 2>/dev/null; then | ||
| _cmux_pr_request_probe | ||
| else | ||
| _cmux_run_pr_probe_with_timeout "$watch_pwd" 1 || true | ||
| fi |
There was a problem hiding this comment.
_cmux_pr_cache_clear inside a subshell — in-memory state is lost
_cmux_start_git_head_watch runs its body in { } >/dev/null 2>&1 &!, so every call inside it (including _cmux_pr_cache_clear and _cmux_clear_pr_for_panel) executes in a forked subshell. The file-based cache IS correctly deleted by the rm -f in _cmux_pr_cache_clear, but the in-memory resets (_CMUX_PR_LAST_BRANCH="", _CMUX_PR_NO_PR_BRANCH="") are thrown away when the subshell exits. The parent shell retains whatever stale values these variables held before the branch change.
In practice this doesn't cause a visible bug today because the parent's _cmux_precmd loop also calls _cmux_pr_cache_clear on the next prompt (when pr_context_changed is detected), but the in-memory state can be transiently wrong between a background HEAD change and the next prompt. The same pattern is present in the bash integration's git-head watcher path.
| if scenario == "prompt_helper_idle": | ||
| if gh_count < 2: | ||
| return (1, f"{shell}/{scenario}: expected idle polling to survive prompt helpers, saw {gh_count}") | ||
| if gh_count != 1: | ||
| return (1, f"{shell}/{scenario}: expected cached idle branch to hit gh once, saw {gh_count}") | ||
| if _report_line(1138) not in send_lines: | ||
| return (1, f"{shell}/{scenario}: missing report_pr payload\n" + "\n".join(send_lines)) | ||
| return (0, f"{shell}/{scenario}: ok") |
There was a problem hiding this comment.
prompt_helper_idle expectation tightened from >= 2 to == 1 — flaky under slow CI
The new assertion gh_count != 1 requires exactly one gh call over a 3-second sleep with a 1-second poll interval. The first probe must complete, write its cache file, and return before the second poll cycle starts its check — otherwise the second poll sees an empty cache and makes another gh call, yielding gh_count == 2 and a test failure.
On a heavily loaded CI host the subshell probe may not finish within one interval, causing an intermittent failure. The older >= 2 guard was intentionally loose for this reason. Consider allowing gh_count <= 2 to tolerate one extra call on slow runners:
if gh_count < 1 or gh_count > 2:
return (1, f"{shell}/{scenario}: expected 1-2 cached idle gh probes, saw {gh_count}")![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Resources/shell-integration/cmux-bash-integration.bash">
<violation number="1" location="Resources/shell-integration/cmux-bash-integration.bash:48">
P2: Use wall-clock epoch seconds as the fallback in `_cmux_now`; `$SECONDS` makes persisted cache timestamps invalid across shell processes.</violation>
<violation number="2" location="Resources/shell-integration/cmux-bash-integration.bash:467">
P2: Do not return immediately when cached PR emission fails; fall back to a live probe so malformed cache entries don't block PR updates.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| _cmux_restore_scrollback_once | ||
|
|
||
| _cmux_now() { | ||
| printf '%s\n' "${EPOCHSECONDS:-$SECONDS}" |
There was a problem hiding this comment.
P2: Use wall-clock epoch seconds as the fallback in _cmux_now; $SECONDS makes persisted cache timestamps invalid across shell processes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-bash-integration.bash, line 48:
<comment>Use wall-clock epoch seconds as the fallback in `_cmux_now`; `$SECONDS` makes persisted cache timestamps invalid across shell processes.</comment>
<file context>
@@ -44,6 +44,10 @@ _cmux_restore_scrollback_once() {
_cmux_restore_scrollback_once
+_cmux_now() {
+ printf '%s\n' "${EPOCHSECONDS:-$SECONDS}"
+}
+
</file context>
| printf '%s\n' "${EPOCHSECONDS:-$SECONDS}" | |
| if [[ -n "${EPOCHSECONDS:-}" ]]; then | |
| printf '%s\n' "$EPOCHSECONDS" | |
| else | |
| /bin/date +%s | |
| fi |
There was a problem hiding this comment.
3 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Resources/shell-integration/cmux-bash-integration.bash">
<violation number="1" location="Resources/shell-integration/cmux-bash-integration.bash:425">
P2: Cached PR results are read but never used to short-circuit probes, so the loop still calls `gh pr view` every cycle instead of reusing cache.</violation>
</file>
<file name="tests/test_issue_1138_sidebar_pr_polling.py">
<violation number="1">
P2: The idle helper scenario now enforces repeated `gh` polling (`gh_count >= 2`), which conflicts with event-driven cached idle behavior and weakens/invalidates the intended regression coverage.</violation>
</file>
<file name="Resources/shell-integration/cmux-zsh-integration.zsh">
<violation number="1" location="Resources/shell-integration/cmux-zsh-integration.zsh:543">
P2: Cached PR results are not used: the "cache-refresh" path still always runs `gh pr view`, causing unnecessary polling and rate-limit risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| _CMUX_PR_LAST_BRANCH="$cache_branch" | ||
| _CMUX_PR_NO_PR_BRANCH="$cache_no_pr_branch" | ||
| if [[ "$cache_branch" == "$branch" && -n "$cache_result" ]]; then |
There was a problem hiding this comment.
P2: Cached PR results are read but never used to short-circuit probes, so the loop still calls gh pr view every cycle instead of reusing cache.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-bash-integration.bash, line 425:
<comment>Cached PR results are read but never used to short-circuit probes, so the loop still calls `gh pr view` every cycle instead of reusing cache.</comment>
<file context>
@@ -443,31 +416,17 @@ _cmux_report_pr_for_path() {
-
- if [[ "$cache_no_pr_branch" == "$branch" ]] || [[ "$cache_result" == "none" ]]; then
- cache_ttl="${_CMUX_PR_NO_PR_TTL:-300}"
+ if [[ "$cache_branch" == "$branch" && -n "$cache_result" ]]; then
+ _cmux_pr_debug_log "$branch" "cache-refresh"
else
</file context>
|
|
||
| _CMUX_PR_LAST_BRANCH="$cache_branch" | ||
| _CMUX_PR_NO_PR_BRANCH="$cache_no_pr_branch" | ||
| if [[ "$cache_branch" == "$branch" && -n "$cache_result" ]]; then |
There was a problem hiding this comment.
P2: Cached PR results are not used: the "cache-refresh" path still always runs gh pr view, causing unnecessary polling and rate-limit risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-zsh-integration.zsh, line 543:
<comment>Cached PR results are not used: the "cache-refresh" path still always runs `gh pr view`, causing unnecessary polling and rate-limit risk.</comment>
<file context>
@@ -562,31 +534,17 @@ _cmux_report_pr_for_path() {
-
- if [[ "$cache_no_pr_branch" == "$branch" ]] || [[ "$cache_result" == "none" ]]; then
- cache_ttl="${_CMUX_PR_NO_PR_TTL:-300}"
+ if [[ "$cache_branch" == "$branch" && -n "$cache_result" ]]; then
+ _cmux_pr_debug_log "$branch" "cache-refresh"
else
</file context>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Resources/shell-integration/cmux-zsh-integration.zsh (1)
541-563:⚠️ Potential issue | 🟠 MajorCache is written but never used to skip
ghcalls.The cache files are read at lines 536-539 and the branch match is logged at lines 543-547, but the function always proceeds to call
gh pr viewat line 558. The PR objectives state: "reuse recent results via TTLs/freshness windows" and mention_CMUX_PR_RESULT_TTL/_CMUX_PR_NO_PR_TTLdefaults of 300s, but these variables aren't defined and there's no early-return logic when the cache is fresh.Expected flow for cache hits:
- If
cache_branch == branchAND cache timestamp is within TTL ANDforce_probe == 0→ return cached result without callingghAdditionally,
force_probe(line 500) is passed through but never used, since there's no TTL check for it to bypass.🐛 Sketch of the missing cache-hit early return
+typeset -g _CMUX_PR_RESULT_TTL="${_CMUX_PR_RESULT_TTL:-300}" +typeset -g _CMUX_PR_NO_PR_TTL="${_CMUX_PR_NO_PR_TTL:-300}" + _cmux_report_pr_for_path() { local repo_path="$1" local force_probe="${2:-0}" # ... existing guards ... # After reading cache files (lines 536-539): + local cache_ts=0 + [[ -r "$timestamp_file" ]] && cache_ts="$(<"$timestamp_file")" + + if (( ! force_probe )) && [[ "$cache_branch" == "$branch" && -n "$cache_result" ]]; then + local age=$(( now - cache_ts )) + local ttl=$_CMUX_PR_RESULT_TTL + [[ "$cache_result" == "none" ]] && ttl=$_CMUX_PR_NO_PR_TTL + if (( age < ttl )); then + _cmux_pr_debug_log "$branch" "cache-hit" + # Replay cached result without calling gh + if [[ "$cache_result" == "none" ]]; then + return 0 # No PR, already cleared + else + # Parse and send cached PR + local IFS=$'\t' + read -r _ number state url <<< "$cache_result" + # ... send report_pr ... + fi + return 0 + fi + fi # ... existing gh pr view call ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-zsh-integration.zsh` around lines 541 - 563, The cache is read into variables like cache_branch/cache_result but never used to skip the gh call; add TTL checks using _CMUX_PR_RESULT_TTL and _CMUX_PR_NO_PR_TTL (default to 300 if unset), parse the cached timestamp (e.g. cache_ts) and if cache_branch == branch and (now - cache_ts) <= appropriate TTL and force_probe == 0 then log via _cmux_pr_debug_log and return the cached output instead of running gh pr view; ensure force_probe (passed into the function) bypasses the TTL so gh pr view runs when force_probe != 0, and keep existing error file/gh_output logic unchanged for cache-miss paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 541-563: The cache is read into variables like
cache_branch/cache_result but never used to skip the gh call; add TTL checks
using _CMUX_PR_RESULT_TTL and _CMUX_PR_NO_PR_TTL (default to 300 if unset),
parse the cached timestamp (e.g. cache_ts) and if cache_branch == branch and
(now - cache_ts) <= appropriate TTL and force_probe == 0 then log via
_cmux_pr_debug_log and return the cached output instead of running gh pr view;
ensure force_probe (passed into the function) bypasses the TTL so gh pr view
runs when force_probe != 0, and keep existing error file/gh_output logic
unchanged for cache-miss paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7abcaa2-15c3-4089-aa14-47d48b926350
📒 Files selected for processing (2)
Resources/shell-integration/cmux-bash-integration.bashResources/shell-integration/cmux-zsh-integration.zsh
|
Im not sure this is fixed @austinywang ? My gh graphql api rate limit is being saturated by cmux with 7 workspaces open, it uses ~2/second |
This branch was successfully deployed
3 participants
Summary
Validation
zsh -n Resources/shell-integration/cmux-zsh-integration.zshbash -n Resources/shell-integration/cmux-bash-integration.bashpython3 -m py_compile tests/test_issue_1138_sidebar_pr_polling.pyCloses #2414.
Summary by cubic
Make sidebar PR lookups event-driven with per-panel caching, a reprobe signal, and a persistent idle poll loop to keep badges fresh while reducing
ghcalls. Restores predictable idle refresh cadence and reduces UI churn, addressing #2414.New Features
zshGit HEAD watcher./tmp/cmux-pr-debug.log.Bug Fixes
ghfailures and timeouts without stopping the poller.zshtimer handling by falling back to$SECONDSwhenEPOCHSECONDSis unset; add_cmux_nowinbashfor consistent timing.Written for commit a00b46c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements