GitHub App integration + banner system, PR reconcile-on-focus, and CRR sync-host wedge fix#887
Conversation
Real-time PR status was silently failing for unfocused projects. Root causes and fixes: Settings + banners - Separate the two independent GitHub App axes (account user-token vs per-repo install) via a shared renderer helper (githubIntegrationStatus.ts). Settings now shows two honest blocks and drops the always-green permission chips. - New shared Banner primitive + central IntegrationBannerHost (severity-ordered, capped at 2 + collapse) with a durable, fingerprint-based dismiss contract that survives restart and auto-resurfaces on regression. Migrates the connection- health family (GitHub, missing-AI, mock-provider) and adds a per-repo "GitHub App not connected" banner distinct from the gh-CLI banner. Reconcile-on-focus safety net (the always-on brain has no PR poller) - prService.reconcileOnFocus: in-memory throttle (90s), single-flight, bounded merged-heal, 30-min closed-sweep to heal merged-but-unmapped PRs. Global cap-1 jittered limiter in main.ts so opening many projects can't stampede. - prService.syncLanePr + a manual per-badge ⟳ on the PR chip, with a debounced "syncing…" indicator driven by a new pr-reconcile event. Post-auth auto-heal kicks reconcileNow so badges light up after authorizing. Daemon reliability - Fix the cr-sqlite sync-host wedge: rebuildTableInTransaction now drops any leftover __ade_crr_repair_* staging table before CREATE, plus a startup orphan-sweep. Bound the root cause: dispatched/failed automation_ingress_events rows now respect a hard 10k per-project cap so the table can't bloat and abort the rebuild. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cleanups Correctness (load-bearing): - Expose reconcileOnFocus + syncLanePr through the daemon action allowlist (registry.ts). Without this both new PR actions were rejected in the runtime-backed production build (manual ⟳ a silent no-op, post-auth reconcileNow an unhandled rejection). - Route reconcile-on-focus to the daemon in production: scheduleReconcile now takes a runner that re-resolves the live context at fire time and calls the daemon pr.reconcileOnFocus action when the desktop context is dormant (prService null). Fixes both the dead-in-production hook and the stale-closure captured-prService concern. - Guard the fire-and-forget reconcileNow with .catch. Cleanups: - Delete dead statusView (~90 lines) + isGitHubAppRepoAccessPending alias; move its test to githubIntegrationStatus.test.ts. - De-duplicate ingress-prune SQL into pruneIngressEventRowsForProject (dbMaintenanceApi.ts), called from automationService + kvDb maintenance. - Move describeGithubCliBanner into the shared githubIntegrationStatus helper. - Drop dead ?? "global" dismiss-key fallback. - Fix stuck ⟳ spinner: split the pr-reconcile subscription into its own effect so a linkedPr change can't strand the idle-hide timer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- registry.test.ts: regression test pinning reconcileOnFocus + syncLanePr in ADE_ACTION_ALLOWLIST.pr (the daemon rejects un-allowlisted actions; without this the reconcile safety net + manual ⟳ are dead in the production runtime-backed build). - docs: pull-requests (reconcile-on-focus + manual sync freshness layers), onboarding-and-settings (two-axis GitHub App panel + githubIntegrationStatus), ARCHITECTURE (Banner primitive/host + CRR rebuild self-heal + ingress cap), sync-and-multi-device/crdt-model (orphan staging sweep + hard cap). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
📝 WalkthroughWalkthroughThe change adds throttled PR reconciliation and lane synchronization, centralizes ingress retention and SQLite repair cleanup, and replaces inline integration banners with shared GitHub status derivation, durable dismissal, and unified rendering. ChangesPR reconciliation
Retention and database repair safety
Integration banner consolidation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 9814-9822: Add force: true to both non-forced getGithubSnapshot
calls in apps/desktop/src/main/services/prs/prService.ts at lines 9814-9822 and
9856-9859: update the closed-sweep call in reconcileOnFocus and the unmapped-PR
fallback in syncLanePr. No other sites require changes. Ensure both paths
perform an awaited live GitHub fetch rather than returning projected local data.
- Around line 9759-9837: Update the Phase 3 closed-sweep call in
reconcileOnFocus to force a fresh GitHub snapshot when includeExternalClosed is
enabled, rather than allowing the normal snapshot TTL cache to satisfy it.
Preserve the existing cadence gate, timestamp update, and closedSwept result
behavior.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 1244-1263: Update the orphan enumeration in
sweepOrphanedRepairStagingTables to exclude __crsql_clock and __crsql_pks
sibling tables, so only actual repair staging parents are processed by the
ambiguousTables check and dropRepairStagingTable. Preserve shadow tables
belonging to ambiguous staging bases and ensure dropped counts do not include
those siblings; extend the existing sweepOrphanedRepairStagingTables test with
an ambiguous base and shadow sibling.
In `@apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx`:
- Around line 153-174: Update the repo action construction in the block.repo
branch so access_pending exposes only a Recheck action that invokes
loadAppStatus(true), matching GitHubAppInstallPanel’s repoActions behavior. Keep
the existing install/manage actions for other repository states.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dcfbb1f-974f-4e58-9816-265d094181b9
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/crdt-model.mdis excluded by!docs/**
📒 Files selected for processing (27)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/automations/automationService.test.tsapps/desktop/src/main/services/automations/automationService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/state/dbMaintenanceApi.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.aiStatus.test.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsxapps/desktop/src/renderer/components/github/GitHubAppInstallPanel.test.tsapps/desktop/src/renderer/components/github/GitHubAppInstallPanel.tsxapps/desktop/src/renderer/components/shared/Banner.tsxapps/desktop/src/renderer/lib/bannerDismiss.test.tsapps/desktop/src/renderer/lib/bannerDismiss.tsapps/desktop/src/renderer/lib/githubIntegrationStatus.test.tsapps/desktop/src/renderer/lib/githubIntegrationStatus.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/prs.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/components/github/GitHubAppInstallPanel.test.ts
CI:
- secret-scan: gitleaks generic-api-key false positive on the bannerDismiss
localStorage key name; added a scoped .gitleaksignore fingerprint.
Review (CodeRabbit Major/Minor, verified):
- reconcileOnFocus closed-sweep + syncLanePr unmapped fallback now pass
force:true to getGithubSnapshot({includeExternalClosed}); without it the
projected/cached snapshot is returned and the merged/closed backfill (the
#402 heal) never runs synchronously.
- kvDb orphan-sweep excludes __crsql_clock/__crsql_pks siblings from
enumeration so an ambiguous base's shadow tables aren't over-dropped; added
a regression test.
- IntegrationBannerHost: access_pending repo banner shows Recheck (re-fetch
status) instead of Install/Manage, matching the Settings panel.
Skipped (verified false positive):
- Greptile P1 "missing-provider banner suppressed": aiStatus===null with
aiStatusLoaded===true is the AI-status FETCH-ERROR state (setAiStatus(null)
in .catch); the aiStatus!==null guard intentionally suppresses the banner
then. Applying the suggestion would show a spurious "No AI provider" banner
on any transient status-fetch error.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
![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: fb405dac1b
ℹ️ 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".
| if (status.repo === null) return "no_repo"; | ||
| if (status.installed) return "connected"; | ||
| if (status.state === "not_installed") return "not_installed"; |
There was a problem hiding this comment.
When an app is installed but the relay is unconfigured, or GitHub reports webhookState === "deleted", this branch still derives connected; isGithubRealtimeHealthy then marks the integration healthy, Settings says “Real-time PR updates are on,” and the banner exposes no blocker. The previous presentation explicitly warned for both states, so installation alone must not imply real-time delivery.
AGENTS.md reference: AGENTS.md:L59-L60
Useful? React with 👍 / 👎.
| const entry = readMap()[trimmed]; | ||
| if (!entry) return false; | ||
| if (entry.fingerprint !== fingerprint) return false; | ||
| return nowMs - entry.dismissedAt < graceMs; |
There was a problem hiding this comment.
If a user dismisses an issue, fixes it, and the same issue returns within 14 days—for example, they dismiss “No AI provider,” configure one, then remove it—the stored entry still has the same fingerprint, so this check suppresses the newly regressed banner. Nothing clears entries while a model is absent from the active issue list, meaning the promised resurface-after-recovery behavior does not occur for any issue that returns to the same state.
AGENTS.md reference: AGENTS.md:L59-L60
Useful? React with 👍 / 👎.
| const disposers: Array<() => void> = []; | ||
| const refresh = () => { | ||
| void loadAppStatus(true); | ||
| }; | ||
| const offPrs = window.ade?.prs?.onEvent?.(refresh); | ||
| if (offPrs) disposers.push(offPrs); |
There was a problem hiding this comment.
For every PR event this callback calls loadAppStatus(true), bypassing the preload cache and issuing a relay /status?refresh=1 request. In particular, each new focus reconcile emits both pr-reconcile: running and pr-reconcile: idle, so a single focus produces two forced status checks in addition to the initial load; the sequence guard only discards stale UI results and does not cancel those requests. Refreshing only on the completion event or coalescing these callbacks would avoid the repeated network work.
Useful? React with 👍 / 👎.
…ing) - githubIntegrationStatus: "connected" now requires relayConfigured AND webhookState !== "deleted". New "webhook_off" state (installed but real-time delivery not wired) surfaces a banner + Settings warning instead of a false-healthy "real-time on". Rendered in the panel + banner host. - bannerDismiss: add clearBannerDismissal(key); the host clears a dismissal once its condition is healthy so a later regression to the same state resurfaces a fresh banner (the promised resurface-on-recovery, previously blocked by the matching-fingerprint suppression within the 14-day grace). - IntegrationBannerHost: stop force-refetching App status on every PR event — ignore pr-reconcile pings entirely and debounce the rest into one non-forced, cache-served read; onStatusChanged remains the immediate forced trigger. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38cf503fd9
ℹ️ 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".
| useEffect(() => { | ||
| setAppStatusLoaded(false); | ||
| void loadAppStatus(false); |
There was a problem hiding this comment.
When switching directly between projects, this effect resets appStatusLoaded only after React has already rendered with the new currentProjectRoot and the previous project's appInstall/appAuth. That paint can show the old repository's warning and actionable Install/Manage links in the new project, contrary to the stated project-switch isolation. Track the root associated with the loaded status and render it only when it matches the current root.
Useful? React with 👍 / 👎.
| const account = deriveGithubAccountAuthState(appAuth); | ||
| const repo = deriveGithubRepoConnectionState(appInstall); | ||
| if (account === "valid") clearDismissal(`github-app-account:${currentProjectRoot}`); |
There was a problem hiding this comment.
GitHub App account authorization is machine/account-wide, but both its dismissal and recovery clearing are scoped to currentProjectRoot. If the missing/expired warning is dismissed in project A, authorization becomes healthy while project B is active, and the same account condition later regresses, A's stale matching dismissal still suppresses the warning for up to 14 days. Use a global account dismissal key or clear all account-scoped entries when authorization becomes valid.
Useful? React with 👍 / 👎.
…ismiss key
- IntegrationBannerHost: bind a completed App-status read to the project it was
loaded for (loadedRoot); gate the App block + clear-on-recovery on
loadedRoot === currentProjectRoot so a direct project switch can't paint the
previous repo's warning/Install-Manage for a frame before the reload lands.
- The GitHub App account (user-token) auth is machine/account-wide, so its
dismiss + clear-on-recovery key is now global ("github-app-account") instead
of per-project — a per-project key left a dismiss in project A suppressing the
same account warning after a cross-project regression.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 408819c11d
ℹ️ 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".
| where project_id = ? | ||
| order by received_at desc, rowid desc | ||
| limit -1 offset ${INGRESS_EVENT_HARD_MAX_ROWS_PER_PROJECT} |
There was a problem hiding this comment.
When a project exceeds 10,000 ingress rows and newer terminal rows coexist with older received rows still being matched or dispatched, this unfiltered oldest-first delete can remove those active rows despite the preceding query allowing up to 2,000 of them. Subsequent updateIngressEvent calls then update nothing, the audit record disappears, and a redelivery is no longer deduplicated, potentially running the automation twice. Apply the hard cap preferentially to terminal statuses or otherwise reserve the active-row allowance.
Useful? React with 👍 / 👎.
| and (name like '__ade_crr_repair_%' or name like '__ade_fk_repair_%') | ||
| and name not like '%__crsql_clock' | ||
| and name not like '%__crsql_pks'`, |
There was a problem hiding this comment.
SQLite treats every _ in a LIKE pattern as a single-character wildcard, so this query is broader than the intended __ade_crr_repair_ and __ade_fk_repair_ prefixes. For example, a legitimate table named xxadeXcrrYrepairZfoo matches, fails the subsequent literal startsWith("__ade_crr_repair_") check, and is passed to dropRepairStagingTable, which deletes it during every database open. Use a literal prefix comparison or escape the wildcard characters before dropping anything.
Useful? React with 👍 / 👎.
| try { | ||
| await window.ade.prs.syncLanePr(laneId); |
There was a problem hiding this comment.
In the standalone web client, window.ade.prs comes from createPrsNamespace, whose PR namespace still jumps from getForLane to listAll without defining syncLanePr. Clicking this newly rendered sync button therefore calls undefined; the broad catch hides the failure and only rereads the cached mobile snapshot, so merged or unmapped PRs are never synchronized on that supported surface. Add the new method to the web-client adapter, along with its command routing.
AGENTS.md reference: AGENTS.md:L32-L35
Useful? React with 👍 / 👎.
…eep prefix, web PR sync - dbMaintenanceApi: the 10k hard cap now orders active (non-terminal) rows first so it only ever trims the oldest TERMINAL rows. An old 'received' row that's still being matched/dispatched is no longer sacrificed (which would lose its audit row and break redelivery dedup → double-run). + regression test. - kvDb orphan sweep: require a LITERAL prefix before dropping. SQLite treats `_` in LIKE as a wildcard, so the enumeration could match a legit table; a literal startsWith guard ensures we never drop a non-repair table. - Web PR sync parity: createPrsNamespace gains syncLanePr + reconcileNow, AND the host command registry (syncRemoteCommandService) registers prs.syncLanePr / prs.reconcileOnFocus (+ SyncRemoteCommandAction union) so the ⟳ button and post-auth reconcile actually reach the heal path on web/mobile, not a no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d03c16be55
ℹ️ 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".
| // projected (local github_pr_projections) snapshot when projections | ||
| // exist and only dispatches the live fetch in the background, so the | ||
| // merged/closed backfill never runs synchronously. This is the #402 heal. | ||
| await getGithubSnapshot({ force: true, includeExternalClosed: true }); |
There was a problem hiding this comment.
When a lane reuses a branch that has multiple historical PRs, this state: "all" snapshot sends every matching PR through backfillLanePrRowsFromGithubPulls. That function processes the newest-first response sequentially, while upsertRow(..., { allowRepoPrAdoption: true }) identifies the existing row by lane and branch, so each older PR overwrites the same row and the oldest fetched PR can replace the lane's current PR number, URL, and state. Because this sweep now runs automatically on focus, merely opening such a project can make an active PR appear merged or closed; select one authoritative PR per lane branch before invoking the backfill.
Useful? React with 👍 / 👎.
backfillLanePrRowsFromGithubPulls now selects ONE authoritative PR per lane before upserting (prefer open/draft, else newest PR number), instead of upserting every matching PR. A reused branch carries multiple historical PRs in a state:"all" snapshot; upserting all of them adopts the same lane row repeatedly (upsertRow identifies by lane+branch), so the last-processed — newest-first response ⇒ oldest — PR would win and flip an active PR to a stale merged/closed one. Since the closed sweep now runs on focus reconcile, that could mis-mark a PR just by opening a project. Single-PR-per-branch behavior is unchanged (verified by the existing closed-history snapshot suite). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
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)
apps/desktop/src/main/services/prs/prService.ts (1)
9830-9833: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport successful heals, not attempted heals.
refreshPrIdstolerates individual failures and returns only successful refreshes, buthealedis set to the attempted count. Use the returned array length so callers do not receive an inflated reconcile result.Proposed fix
- if (staleFirst.length > 0) { - await refreshPrIds(staleFirst.map((pr) => pr.id)); - healed = staleFirst.length; + if (staleFirst.length > 0) { + const refreshed = await refreshPrIds(staleFirst.map((pr) => pr.id)); + healed = refreshed.length;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 9830 - 9833, Update the stale-first healing branch around refreshPrIds to assign healed from the length of the array returned by refreshPrIds, rather than staleFirst.length. Preserve the existing ID mapping and await behavior so the reconcile result reports only successful refreshes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 9830-9833: Update the stale-first healing branch around
refreshPrIds to assign healed from the length of the array returned by
refreshPrIds, rather than staleFirst.length. Preserve the existing ID mapping
and await behavior so the reconcile result reports only successful refreshes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 42515ba6-9934-4680-a790-24750a9cfaf0
📒 Files selected for processing (9)
apps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/automations/automationService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/state/dbMaintenanceApi.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/renderer/components/app/IntegrationBannerHost.tsxapps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.tsapps/desktop/src/renderer/webclient/adapter/prs.tsapps/desktop/src/shared/types/sync.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/services/state/dbMaintenanceApi.ts
- apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx
- apps/desktop/src/main/services/state/kvDb.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 642b6821d2
ℹ️ 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".
| // Throttled per-project + globally capped, so this never blocks focus. | ||
| // The runner routes in-process vs. daemon and re-resolves the live | ||
| // context at fire time (production contexts are dormant here). | ||
| scheduleReconcile(buildReconcileRunner(normalizedRoot)); |
There was a problem hiding this comment.
When a production runtime-backed project remains open while its window loses focus, returning to that same window does not call bindWindowToProject, so this scheduling path never runs. The actual win.on("focus") handler at lines 6188–6202 only updates the foreground project; because these contexts have no local PR poller, a PR merged while ADE was unfocused remains stale despite the reconcile-on-focus feature until the user switches projects or manually syncs. Schedule the throttled runner from that focus handler as well.
Useful? React with 👍 / 👎.
Also schedule the throttled reconcile from win.on("focus") for the focused
project root. Returning to an already-open project (window regains OS focus)
doesn't re-run bindWindowToProject, so without this the reconcile-on-focus
safety net never fired for the most common "come back to ADE after being away"
case — a PR merged while unfocused stayed stale until a project switch or manual
sync. The per-project 90s throttle + single-flight + app-wide cap-1 limiter
collapse refocus storms into at most one reconcile.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7832314ab
ℹ️ 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".
| * Hard ceiling on TOTAL ingress rows per project regardless of status. Well | ||
| * above the 2,000 active-row cap so normal dedup/audit history survives, but | ||
| * bounds dispatched/failed rows so an always-on brain dispatching high webhook | ||
| * volume can't bloat automation_ingress_events within the 7-day window and | ||
| * wedge the cr-sqlite table rebuild. | ||
| */ | ||
| export const INGRESS_EVENT_HARD_MAX_ROWS_PER_PROJECT = 10_000; |
There was a problem hiding this comment.
When a project accumulates terminal ingress events, this new policy permits up to 10,000 total rows, but apps/desktop/src/main/services/storage/storageLedger.ts still declares maxRows: INGRESS_EVENT_MAX_ROWS_PER_PROJECT (2,000). That makes the ledger's persisted-data policy inaccurate and contradicts the nearby claim that the ledger and enforcement constants cannot drift; update the ledger to represent the 10,000-row total ceiling, or model both the active and total caps explicitly.
Useful? React with 👍 / 👎.
storageLedger's automation_ingress_events policy declared maxRows = 2,000 (the active-row cap), but the table's real total ceiling is now the 10,000 hard cap. Point maxRows at INGRESS_EVENT_HARD_MAX_ROWS_PER_PROJECT so the persisted-data size policy is accurate and the ledger/enforcement constants don't drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cb1635336
ℹ️ 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".
| account: GithubAccountAuthState, | ||
| repo: GithubRepoConnectionState, | ||
| ): GithubRealtimeBlock { | ||
| if (account === "missing" || account === "expired") return { kind: "account", account }; |
There was a problem hiding this comment.
For an ADE-account-signed-in user without github.appUserToken.v1, this always reports an account blocker even when the repository status is connected. githubService.getAppInstallationStatus accepts getAccountAccessToken, and automationIngressService uses that same account-token fallback for relay polling/subscription, so real-time updates can already work without the device-flow token. The banner and Settings therefore persistently claim that a healthy integration is unauthorized; expose the effective relay credential in the shared status or derive health from the actual service result.
AGENTS.md reference: AGENTS.md:L33-L34
Useful? React with 👍 / 👎.
| try { | ||
| const auth = (await api.getAppUserAuthStatus?.()) ?? null; | ||
| if (!isCurrent()) return; | ||
| setAppAuth(auth); | ||
| } catch { | ||
| if (!isCurrent()) return; | ||
| setAppAuth(null); |
There was a problem hiding this comment.
When the renderer uses createAdeWebAdapter, these calls do not return runtime GitHub App status: adapter/misc.ts returns the legacy { authenticated, user } object for getAppUserAuthStatus and only { installed: false, state: "unknown" } for installation status. This code nevertheless marks the read as loaded, interprets missing tokenStored as an unauthorized account, and shows every bound hosted-web project a false GitHub App warning. Route these methods through the sync host with the full shared DTOs, or explicitly treat the adapter result as unsupported and omit the banner.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| {!hideSidebar && !showWelcome && project?.rootPath ? ( | ||
| <IntegrationBannerHost |
There was a problem hiding this comment.
When a project is on /onboarding, this now mounts the integration host even though the removed GitHub banner explicitly excluded that route. The onboarding GitHub card already renders GitHubAppInstallPanel (components/onboarding/GitHubCard.tsx:89), so users missing App authorization or installation see duplicate GitHub setup warnings and CTAs above the dedicated setup card. Retain the prior !isOnboardingRoute guard for the GitHub integration host or suppress its GitHub models on that route.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — the host is already gated off onboarding. hideSidebar = isOnboardingRoute || shouldHoldProjectRouteForOnboarding (AppShell.tsx:1158) and the host mounts under !hideSidebar (AppShell.tsx:1311), so on /onboarding it doesn't render and there's no duplication with GitHubCard's panel. Not applying.
…ub banner
- deriveGithubRealtimeBlock: a connected repo proves a working relay credential
(the install/webhook check succeeded, possibly via the ADE account-token
fallback), so surface no blocker even when the device-flow user token is
absent — fixes the persistent false "not authorized" for account-token users.
- IntegrationBannerHost: the standalone web-client adapter returns App-status
stubs ({authenticated,user} / {installed:false,state:"unknown"}) that lack the
real DTO fields; require the real shape (appName/relayConfigured/configured)
before deriving the App block, so hosted-web projects no longer flash a false
"GitHub App not authorized" banner.
- (onboarding-route duplication was a false positive: hideSidebar already gates
the host off /onboarding — replied on-thread.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
| syncLanePr: async (laneId: string): Promise<PrSummary | null> => | ||
| callPrReadRuntimeActionOr("syncLanePr", { arg: laneId }, () => | ||
| ipcRenderer.invoke(IPC.prsSyncLanePr, { laneId }), | ||
| ), |
There was a problem hiding this comment.
The runtime-backed path sends { arg: laneId }, but the daemon handler for prs.syncLanePr parses payload.laneId via parseLaneIdArgs, so the manual ⟳ action is rejected before reaching prService.syncLanePr. In production this makes the new per-badge sync silently fall back to null instead of healing the lane PR.
Artifacts
Repro: focused TypeScript harness for prs.syncLanePr payload handling
- Evidence file captured while the check ran.
Repro: execution log showing arg payload rejection and laneId payload success
- The full command output behind this check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/preload/preload.ts
Line: 7848-7851
Comment:
**Runtime sync drops lane**
The runtime-backed path sends `{ arg: laneId }`, but the daemon handler for `prs.syncLanePr` parses `payload.laneId` via `parseLaneIdArgs`, so the manual ⟳ action is rejected before reaching `prService.syncLanePr`. In production this makes the new per-badge sync silently fall back to `null` instead of healing the lane PR.
How can I resolve this? If you propose a fix, please make it concise.
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Why
Real-time PR status was silently failing for projects that weren't focused (e.g. a lane in another project whose PR merged overnight showed no badge). Root cause was a chain: the ADE GitHub App wasn't installed on the repo / the App user-token was expired, so no webhooks flowed; the only fallback (the snapshot poller) is desktop-side and runs solely for the focused project; and the settings UI conflated account-token failure with per-repo install, hiding the real cause.
What
GitHub App integration + honest settings
githubIntegrationStatus.ts): account user-token (valid/expired/missing) vs per-repo App install (connected/not_installed/access_pending/no_repo).Consolidated banner system
Bannerprimitive + centralIntegrationBannerHost(severity-ordered, capped at 2 + collapse) with a durable, fingerprint-aware dismiss contract (survives restart, auto-resurfaces on regression).PR reconcile-on-focus safety net
prService.reconcileOnFocus(in-memory 90s throttle, single-flight, bounded merged-heal, 30-min closed-sweep for merged-but-unmapped PRs) +syncLanePr(manual per-badge ⟳) + post-auth auto-heal.pr.reconcileOnFocusaction (with fire-time live-context re-resolution).Daemon reliability
rebuildTableInTransactiondrops any leftover__ade_crr_repair_*staging table before CREATE, plus a startup orphan-sweep.automation_ingress_eventsnow has a hard 10k per-project cap so the table can't bloat and abort the rebuild.Testing
reconcileOnFocus/syncLanePrin the daemon action allowlist (the /quality Blocker).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR adds GitHub App integration status, centralized integration banners, PR catch-up reconciliation, and CRR repair hardening. The main changes are:
Confidence Score: 4/5
Mostly safe to merge after fixing one contained production-path issue.
The main reconcile, banner, and DB repair changes are scoped and covered by targeted tests. One payload mismatch breaks the new runtime-backed manual lane sync path, while the broader focus reconcile path remains intact.
apps/desktop/src/preload/preload.tsWhat T-Rex did
Important Files Changed
syncLanePrsends the lane id under the wrong payload key.Sequence Diagram
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "ship: iter 8 — Codex P2s: honor account-..." | Re-trigger Greptile