sync: port upstream websocket/compression and git perf work (#4291–#4985) - #191
Conversation
…ingdotgg#4622) (cherry picked from commit d60f6e9)
📝 WalkthroughWalkthroughThis PR adds VCS cache invalidation and Git snapshot caching. It also adds HTTP compression, MCP liveness tracking, activity payload projection, launcher-aware CLI output, editor discovery timeouts, POSIX HOME hydration, and scroll-driven branch pagination. ChangesVCS cache coordination
Server behavior and transport
CLI and platform behavior
Branch pagination
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
github-actions
Bot
added
the
vouch:trusted
github-actions
Bot
added
the
size:XXL
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (4)
apps/server/src/os-jank.ts (1)
78-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a focused test for the HOME lookup failure path.
The current tests cover
hydratePosixHomedirectly, but they do not verify thatfixPathcatches a failed HOME lookup, logs a warning, and continues to PATH hydration. Add and run a focused regression test for this startup behavior.As per coding guidelines, backend changes must include and run focused tests for the changed behavior.
🤖 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/server/src/os-jank.ts` around lines 78 - 84, Update the tests for fixPath to cover a failed HOME lookup: mock or force hydratePosixHome to fail, assert that logPathHydrationWarning is called, and verify fixPath continues into PATH hydration rather than terminating. Run this focused regression test.Source: Coding guidelines
apps/server/src/vcs/VcsDriverRegistry.ts (1)
118-121: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a short non-zero TTL for negative detections.
The change fixes stale negative detections.
Duration.zeroalso removes all throttling for paths that are not repositories. Everydetectcall then spawnsrev-parse --is-inside-work-tree, andresolvecallsdetecton many request paths.
GitVcsDriverCore.tsLine 61 solves the same problem withNON_REPOSITORY_PATHS_CACHE_TTL = Duration.seconds(1). A one-second TTL keeps detection of a newly created repository fast and bounds the subprocess rate.♻️ Proposed change to throttle negative detections
Add the constant next to
DETECTION_CACHE_TTL:+const NON_DETECTION_CACHE_TTL = Duration.seconds(1);Then apply the short TTL to null results:
timeToLive: Exit.match({ - onSuccess: (detected) => (detected === null ? Duration.zero : DETECTION_CACHE_TTL), + onSuccess: (detected) => + detected === null ? NON_DETECTION_CACHE_TTL : DETECTION_CACHE_TTL, onFailure: () => Duration.zero, }),The existing test asserts two work-tree checks across two sequential
detectcalls. A non-zero TTL would coalesce those into one check, so the test needs aTestClock.adjustbetween the calls.🤖 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/server/src/vcs/VcsDriverRegistry.ts` around lines 118 - 121, Use the existing one-second NON_REPOSITORY_PATHS_CACHE_TTL for null detections in the VcsDriverRegistry timeToLive logic instead of Duration.zero, while retaining DETECTION_CACHE_TTL for successful detections and zero TTL on failures. Update the sequential detect test to advance TestClock between calls so it still verifies two work-tree checks.apps/server/src/vcs/GitVcsDriverCore.ts (1)
1522-1588: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn parsed numstat entries instead of re-serializing to text.
The unborn-HEAD fallback merges staged and unstaged entries into a map, then formats the map back into tab-separated numstat text at Line 1558. Line 1649 parses that text again with
parseNumstatEntries.The round-trip is lossy for any path that contains a tab or a newline, because
parseNumstatEntriessplits on those characters. The fallback also duplicates the merge logic that Line 1650 already performs.Change the
Effect.allelement to produceArray<{ path; insertions; deletions }>for both branches. Parse thediff HEADoutput in the success branch, and return the merged array directly in the unborn branch. Then Line 1649 consumes the array without a second parse.🤖 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/server/src/vcs/GitVcsDriverCore.ts` around lines 1522 - 1588, The numstat flow currently re-serializes parsed entries into text, which loses paths containing tabs or newlines and duplicates merging. Update the `GitVcsDriver.statusDetails.numstat` `Effect.all` element so the successful `diff HEAD` branch returns `parseNumstatEntries(result.stdout)`, while the unborn-HEAD branch merges staged and unstaged parsed entries into and returns an array of `{ path, insertions, deletions }`; then update the consumer near `parseNumstatEntries` to use that array directly without reparsing.packages/client-runtime/src/state/vcs.ts (1)
145-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared persist-and-warn logic for cached refs.
Lines 145-155 build a
saveVcsRefseffect with a warning log.commitVcsRefsRefreshrepeats the same call and the same annotated warning at lines 86-96. A future change to the log message or the annotation set must be applied twice. Extract one helper and call it from both places.♻️ Proposed extraction
+const persistVcsRefs = ( + cache: EnvironmentCacheStore["Service"], + environmentId: EnvironmentId, + cwd: string, + refs: VcsListRefsResult, +) => + cache.saveVcsRefs(environmentId, cwd, refs).pipe( + Effect.catch((error) => + Effect.logWarning("Could not persist cached Git refs.").pipe( + Effect.annotateLogs({ + environmentId, + cwd, + ...safeErrorLogAttributes(error), + }), + ), + ), + );Then in
refresh:- const persist = cache.saveVcsRefs(environmentId, input.cwd, refs).pipe( - Effect.catch((error) => - Effect.logWarning("Could not persist cached Git refs.").pipe( - Effect.annotateLogs({ - environmentId, - cwd: input.cwd, - ...safeErrorLogAttributes(error), - }), - ), - ), - ); + const persist = persistVcsRefs(cache, environmentId, input.cwd, refs);And in
commitVcsRefsRefreshreplace lines 86-96 withyield* persistVcsRefs(cache, input.environmentId, input.cwd, input.refs);.Based on the coding guideline "extract shared logic instead of duplicating local implementations".
🤖 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 `@packages/client-runtime/src/state/vcs.ts` around lines 145 - 155, Extract the duplicated save-and-warning effect into a shared persistVcsRefs helper near the existing VCS state logic, preserving saveVcsRefs, the warning message, and safeErrorLogAttributes annotations. Update both refresh and commitVcsRefsRefresh to call this helper with the cache, environmentId, cwd, and refs instead of constructing their own persistence effects.Source: Coding guidelines
🤖 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/server/src/cli/connect.ts`:
- Around line 530-534: Add focused tests in invocation.test.ts that execute the
connect link/connect command paths with injected launcher arguments, asserting
the output includes the resolved serve command. Cover the background-service
path and assert it returns without displaying the next-step message, while
preserving existing pure formatting-helper tests.
In `@apps/server/src/mcp/McpHttpServer.ts`:
- Around line 77-85: Update the rejected-credential branch in the MCP request
handler around invocation to rate-limit or sample the warning emitted by
Effect.logWarning, while preserving a metric for every rejected credential. Keep
the unauthorized response unchanged and ensure the metric remains recorded even
when the warning is suppressed.
In `@apps/server/src/os-jank.test.ts`:
- Around line 14-29: Remove the process.env.HOME mutation and restoration
try/finally block from the “hydrates HOME independently of a blank process HOME”
test. Keep the test isolated by passing the existing { HOME: " " } object
directly to hydratePosixHome and preserve the assertion against
NodeOS.userInfo().homedir.
In `@apps/server/src/vcs/GitVcsDriverCore.ts`:
- Around line 2414-2439: Bound the retry loop in resolveListRefsSnapshot with a
finite attempt limit. Track the most recently fetched snapshot and, when the
limit is reached despite generation changes, return that snapshot instead of
starting another scan; preserve the existing generation-stability fast path and
refresh/non-refresh cache selection.
---
Nitpick comments:
In `@apps/server/src/os-jank.ts`:
- Around line 78-84: Update the tests for fixPath to cover a failed HOME lookup:
mock or force hydratePosixHome to fail, assert that logPathHydrationWarning is
called, and verify fixPath continues into PATH hydration rather than
terminating. Run this focused regression test.
In `@apps/server/src/vcs/GitVcsDriverCore.ts`:
- Around line 1522-1588: The numstat flow currently re-serializes parsed entries
into text, which loses paths containing tabs or newlines and duplicates merging.
Update the `GitVcsDriver.statusDetails.numstat` `Effect.all` element so the
successful `diff HEAD` branch returns `parseNumstatEntries(result.stdout)`,
while the unborn-HEAD branch merges staged and unstaged parsed entries into and
returns an array of `{ path, insertions, deletions }`; then update the consumer
near `parseNumstatEntries` to use that array directly without reparsing.
In `@apps/server/src/vcs/VcsDriverRegistry.ts`:
- Around line 118-121: Use the existing one-second
NON_REPOSITORY_PATHS_CACHE_TTL for null detections in the VcsDriverRegistry
timeToLive logic instead of Duration.zero, while retaining DETECTION_CACHE_TTL
for successful detections and zero TTL on failures. Update the sequential detect
test to advance TestClock between calls so it still verifies two work-tree
checks.
In `@packages/client-runtime/src/state/vcs.ts`:
- Around line 145-155: Extract the duplicated save-and-warning effect into a
shared persistVcsRefs helper near the existing VCS state logic, preserving
saveVcsRefs, the warning message, and safeErrorLogAttributes annotations. Update
both refresh and commitVcsRefsRefresh to call this helper with the cache,
environmentId, cwd, and refs instead of constructing their own persistence
effects.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 35effc4e-c1a3-48b6-a5cd-15a485793c5d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (56)
apps/mobile/src/connection/environment-cache-store.test.tsapps/mobile/src/connection/environment-cache-store.tsapps/mobile/src/persistence/mobile-database.tsapps/server/src/cli/connect.tsapps/server/src/cli/invocation.test.tsapps/server/src/cli/invocation.tsapps/server/src/git/GitManager.tsapps/server/src/http.test.tsapps/server/src/http.tsapps/server/src/httpCompression/HttpResponseCompression.test.tsapps/server/src/httpCompression/HttpResponseCompression.tsapps/server/src/mcp/McpHttpServer.test.tsapps/server/src/mcp/McpHttpServer.tsapps/server/src/mcp/McpInvocationContext.test.tsapps/server/src/mcp/McpInvocationContext.tsapps/server/src/mcp/McpSessionRegistry.test.tsapps/server/src/mcp/McpSessionRegistry.tsapps/server/src/mcp/PreviewAutomationBroker.test.tsapps/server/src/mcp/PreviewAutomationBroker.tsapps/server/src/orchestration/ActivityPayloadProjection.tsapps/server/src/orchestration/http.tsapps/server/src/os-jank.test.tsapps/server/src/os-jank.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/server.test.tsapps/server/src/server.tsapps/server/src/vcs/GitVcsDriverCore.test.tsapps/server/src/vcs/GitVcsDriverCore.tsapps/server/src/vcs/VcsDriverRegistry.test.tsapps/server/src/vcs/VcsDriverRegistry.tsapps/server/src/ws.tsapps/server/test/ActivityPayloadProjection.test.tsapps/web/src/components/BranchToolbarBranchSelector.tsxapps/web/src/connection/storage.tsapps/web/src/state/paginatedBranches.test.tsapps/web/src/state/paginatedBranches.tsapps/web/src/state/queries.tspackages/client-runtime/src/connection/registry.test.tspackages/client-runtime/src/platform/persistence.tspackages/client-runtime/src/state/runtime.tspackages/client-runtime/src/state/server.test.tspackages/client-runtime/src/state/shell-sync.test.tspackages/client-runtime/src/state/sourceControl.test.tspackages/client-runtime/src/state/sourceControl.tspackages/client-runtime/src/state/threads-sync.test.tspackages/client-runtime/src/state/vcs.test.tspackages/client-runtime/src/state/vcs.tspackages/client-runtime/src/state/vcsAction.test.tspackages/client-runtime/src/state/vcsAction.tspackages/client-runtime/src/state/vcsRefInvalidation.tspackages/contracts/src/git.tspackages/contracts/src/orchestration.tspackages/contracts/src/rpc.tspatches/@effect__platform-bun@4.0.0-beta.102.patchpatches/@effect__platform-node@4.0.0-beta.102.patchpnpm-workspace.yaml
💤 Files with no reviewable changes (6)
- apps/server/src/mcp/McpInvocationContext.test.ts
- apps/server/src/mcp/McpHttpServer.test.ts
- apps/server/src/mcp/McpInvocationContext.ts
- packages/contracts/src/orchestration.ts
- apps/server/src/mcp/PreviewAutomationBroker.test.ts
- packages/contracts/src/rpc.ts
| const serveCommand = yield* resolveCliCommand("serve"); | ||
| yield* Console.log( | ||
| flags.publishOnly | ||
| ? `✓ Authorized${connectedAs(linked.identity)}\n\nNext\n Start T3 to publish agent activity (no managed tunnel).` | ||
| : `✓ Authorized${connectedAs(linked.identity)}\n\nNext\n Start the server with \`t3 serve\` to make this machine reachable.`, | ||
| : `✓ Authorized${connectedAs(linked.identity)}\n\nNext\n Start the server with \`${serveCommand}\` to make this machine reachable.`, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add focused tests for the connection command output.
apps/server/src/cli/invocation.test.ts tests the pure formatting helpers only. It does not execute connect link or connect.
Add tests that inject launcher arguments and assert the displayed serve command. Also assert that the background-service path returns without the next-step message.
As per coding guidelines, “Backend changes must include and run focused tests for the changed behavior.”
Also applies to: 696-704
🤖 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/server/src/cli/connect.ts` around lines 530 - 534, Add focused tests in
invocation.test.ts that execute the connect link/connect command paths with
injected launcher arguments, asserting the output includes the resolved serve
command. Cover the background-service path and assert it returns without
displaying the next-step message, while preserving existing pure
formatting-helper tests.
Source: Coding guidelines
| if (!invocation) { | ||
| // Without this the only symptom of a dead credential is the agent | ||
| // quietly losing the whole `t3-code` toolkit for the rest of its | ||
| // session, with nothing on the server to explain why. | ||
| yield* Effect.logWarning("rejected MCP request with an unusable credential", { | ||
| reason: token.length === 0 ? "missing_bearer_token" : "unknown_or_expired_token", | ||
| }); | ||
| return unauthorized; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound warnings for rejected MCP credentials.
An unauthenticated caller can send unlimited invalid requests to /mcp. Each request now writes a warning. This can flood logs and consume logging capacity during an attack. Rate-limit or sample this warning, and retain a metric for rejected credentials.
🤖 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/server/src/mcp/McpHttpServer.ts` around lines 77 - 85, Update the
rejected-credential branch in the MCP request handler around invocation to
rate-limit or sample the warning emitted by Effect.logWarning, while preserving
a metric for every rejected credential. Keep the unauthorized response unchanged
and ensure the metric remains recorded even when the warning is suppressed.
| it("hydrates HOME independently of a blank process HOME", () => { | ||
| const originalHome = process.env.HOME; | ||
| const env: NodeJS.ProcessEnv = { HOME: " " }; | ||
|
|
||
| try { | ||
| process.env.HOME = " "; | ||
| hydratePosixHome(env); | ||
| } finally { | ||
| if (originalHome === undefined) { | ||
| delete process.env.HOME; | ||
| } else { | ||
| process.env.HOME = originalHome; | ||
| } | ||
| } | ||
|
|
||
| assert.equal(env.HOME, NodeOS.userInfo().homedir); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the unnecessary global environment mutation.
hydratePosixHome reads the supplied env, so changing process.env.HOME does not affect this test. Remove the mutation and restoration block. Keep the test isolated by passing { HOME: " " } directly.
As per coding guidelines, prioritize reliability and predictable behavior under load and failures.
🤖 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/server/src/os-jank.test.ts` around lines 14 - 29, Remove the
process.env.HOME mutation and restoration try/finally block from the “hydrates
HOME independently of a blank process HOME” test. Keep the test isolated by
passing the existing { HOME: " " } object directly to hydratePosixHome and
preserve the assertion against NodeOS.userInfo().homedir.
Source: Coding guidelines
| const resolveListRefsSnapshot = Effect.fn("resolveListRefsSnapshot")(function* ( | ||
| gitCommonDir: string, | ||
| refresh: boolean, | ||
| ) { | ||
| while (true) { | ||
| const generation = currentListRefsGeneration(gitCommonDir); | ||
| const currentEpoch = listRefsEpochByCommonDir.get(gitCommonDir); | ||
| const snapshot = | ||
| refresh || currentEpoch === undefined | ||
| ? // The refresh cache owns the complete snapshot read, rather than only the | ||
| // epoch bump. Slow repositories therefore remain singleflight for the | ||
| // entire Git scan even when more refresh requests arrive after the | ||
| // coalescing TTL would otherwise have elapsed. | ||
| yield* Cache.get( | ||
| listRefsRefreshSnapshotCache, | ||
| new GitRefsRefreshCacheKey({ gitCommonDir, generation }), | ||
| ) | ||
| : yield* Cache.get( | ||
| listRefsSnapshotCache, | ||
| new GitRefsSnapshotCacheKey({ gitCommonDir, epoch: currentEpoch }), | ||
| ); | ||
| currentPath = exists ? candidatePath : null; | ||
| } else if (line.startsWith("branch refs/heads/") && currentPath) { | ||
| worktreeMap.set(line.slice("branch refs/heads/".length), currentPath); | ||
| } else if (line === "") { | ||
| currentPath = null; | ||
| } | ||
| } | ||
| if (currentListRefsGeneration(gitCommonDir) === generation) { | ||
| return snapshot; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the snapshot retry loop.
resolveListRefsSnapshot retries without a limit. Each iteration reads the generation, fetches a snapshot, and restarts when invalidateListRefsSnapshot advanced the generation during the fetch.
A repository under a continuous stream of mutations advances the generation on every mutation. A ref scan on a large repository can take seconds. In that case listRefs restarts on every attempt and never returns. The request blocks until the RPC times out, and each attempt spawns another full for-each-ref, worktree list, and remote scan.
Add a retry limit. After the limit, return the last snapshot that was read. The result is then at most one generation stale, which is the same guarantee that a non-refresh read already provides.
🔒️ Proposed fix to bound the retries
const resolveListRefsSnapshot = Effect.fn("resolveListRefsSnapshot")(function* (
gitCommonDir: string,
refresh: boolean,
) {
- while (true) {
+ // A repository under continuous mutation advances the generation on every
+ // scan. Stop after a bounded number of attempts and accept a snapshot that
+ // is at most one generation stale, rather than never returning.
+ const maxAttempts = 3;
+ for (let attempt = 1; ; attempt += 1) {
const generation = currentListRefsGeneration(gitCommonDir);
const currentEpoch = listRefsEpochByCommonDir.get(gitCommonDir);
const snapshot =
refresh || currentEpoch === undefined
? // The refresh cache owns the complete snapshot read, rather than only the
// epoch bump. Slow repositories therefore remain singleflight for the
// entire Git scan even when more refresh requests arrive after the
// coalescing TTL would otherwise have elapsed.
yield* Cache.get(
listRefsRefreshSnapshotCache,
new GitRefsRefreshCacheKey({ gitCommonDir, generation }),
)
: yield* Cache.get(
listRefsSnapshotCache,
new GitRefsSnapshotCacheKey({ gitCommonDir, epoch: currentEpoch }),
);
- if (currentListRefsGeneration(gitCommonDir) === generation) {
+ if (currentListRefsGeneration(gitCommonDir) === generation || attempt >= maxAttempts) {
return snapshot;
}
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resolveListRefsSnapshot = Effect.fn("resolveListRefsSnapshot")(function* ( | |
| gitCommonDir: string, | |
| refresh: boolean, | |
| ) { | |
| while (true) { | |
| const generation = currentListRefsGeneration(gitCommonDir); | |
| const currentEpoch = listRefsEpochByCommonDir.get(gitCommonDir); | |
| const snapshot = | |
| refresh || currentEpoch === undefined | |
| ? // The refresh cache owns the complete snapshot read, rather than only the | |
| // epoch bump. Slow repositories therefore remain singleflight for the | |
| // entire Git scan even when more refresh requests arrive after the | |
| // coalescing TTL would otherwise have elapsed. | |
| yield* Cache.get( | |
| listRefsRefreshSnapshotCache, | |
| new GitRefsRefreshCacheKey({ gitCommonDir, generation }), | |
| ) | |
| : yield* Cache.get( | |
| listRefsSnapshotCache, | |
| new GitRefsSnapshotCacheKey({ gitCommonDir, epoch: currentEpoch }), | |
| ); | |
| currentPath = exists ? candidatePath : null; | |
| } else if (line.startsWith("branch refs/heads/") && currentPath) { | |
| worktreeMap.set(line.slice("branch refs/heads/".length), currentPath); | |
| } else if (line === "") { | |
| currentPath = null; | |
| } | |
| } | |
| if (currentListRefsGeneration(gitCommonDir) === generation) { | |
| return snapshot; | |
| } | |
| } | |
| }); | |
| const resolveListRefsSnapshot = Effect.fn("resolveListRefsSnapshot")(function* ( | |
| gitCommonDir: string, | |
| refresh: boolean, | |
| ) { | |
| // A repository under continuous mutation advances the generation on every | |
| // scan. Stop after a bounded number of attempts and accept a snapshot that | |
| // is at most one generation stale, rather than never returning. | |
| const maxAttempts = 3; | |
| for (let attempt = 1; ; attempt += 1) { | |
| const generation = currentListRefsGeneration(gitCommonDir); | |
| const currentEpoch = listRefsEpochByCommonDir.get(gitCommonDir); | |
| const snapshot = | |
| refresh || currentEpoch === undefined | |
| ? // The refresh cache owns the complete snapshot read, rather than only the | |
| // epoch bump. Slow repositories therefore remain singleflight for the | |
| // entire Git scan even when more refresh requests arrive after the | |
| // coalescing TTL would otherwise have elapsed. | |
| yield* Cache.get( | |
| listRefsRefreshSnapshotCache, | |
| new GitRefsRefreshCacheKey({ gitCommonDir, generation }), | |
| ) | |
| : yield* Cache.get( | |
| listRefsSnapshotCache, | |
| new GitRefsSnapshotCacheKey({ gitCommonDir, epoch: currentEpoch }), | |
| ); | |
| if (currentListRefsGeneration(gitCommonDir) === generation || attempt >= maxAttempts) { | |
| return snapshot; | |
| } | |
| } | |
| }); |
🤖 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/server/src/vcs/GitVcsDriverCore.ts` around lines 2414 - 2439, Bound the
retry loop in resolveListRefsSnapshot with a finite attempt limit. Track the
most recently fetched snapshot and, when the limit is reached despite generation
changes, return that snapshot instead of starting another scan; preserve the
existing generation-stability fast path and refresh/non-refresh cache selection.
…pingdotgg#4791) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 5fcdefd) (cherry picked from commit 1712447)
…e diff HEAD --numstat (pingdotgg#4843) (cherry picked from commit faea70a) (cherry picked from commit 9c33505)
pingdotgg#4897) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 6ab2f16) (cherry picked from commit aa2e1e1)
896ccca to
f7ab627
Compare
Review triageEvery finding on this PR was checked against Deliberately not fixing them here: patching upstream bugs inside a 101-commit port would diverge the fork in exactly the files this stack exists to realign, and would make the next sync harder. They are better raised upstream or tracked as fork-local follow-ups on their own branch, where they can be reviewed on their merits. Notably, the files where this PR did carry fork-specific conflict resolutions drew no findings. |
![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: f7ab627485
ℹ️ 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".
| : Stream.fromEffect( | ||
| refresh().pipe( |
There was a problem hiding this comment.
When Git refs change outside the VCS RPCs—most commonly when a coding provider runs git branch or git checkout directly—no client invalidation callback fires. Replacing the five-second tick with this one-shot effect means a mounted branch list never discovers those changes; even remounting can reuse the server's two-minute ref snapshot because the web/mobile queries do not set refresh: true. Retain periodic revalidation or issue a forced-refresh request so provider-created branches and checkouts become visible.
Useful? React with 👍 / 👎.
| + perMessageDeflate: { | ||
| + compress: "dedicated", | ||
| + decompress: "shared" | ||
| + }, |
There was a problem hiding this comment.
On the Bun server path, this only negotiates permessage-deflate; the Effect Bun writer still calls ws.sendText(chunk) and ws.sendBinary(chunk) without Bun's per-send compression flag. Consequently clients advertise and negotiate the extension, but server-to-client orchestration frames have RSV1 unset and remain uncompressed, so the main high-volume direction receives none of the intended throughput reduction. Pass the compression flag when writing negotiated WebSocket frames or use a platform implementation that does so.
AGENTS.md reference: AGENTS.md:L127-L127
Useful? React with 👍 / 👎.
8 participants
L2/7 of the 2026-07-30 upstream sync stack. Stacked on #190.
What lands
diff HEAD --numstat(perf(server): merge separate staged/unstaged numstat calls into single diff HEAD --numstat pingdotgg/t3code#4843), external diff disabled for review previews (fix(git): disable external diff for review diff previews pingdotgg/t3code#4854), ref-refresh resource storms fixed (Fix Git ref refresh resource storms pingdotgg/t3code#4727), repo detection after init (fix(server): detect repositories after initialization pingdotgg/t3code#4848), PR detection withoutHOME(fix(server): restore PR detection without HOME pingdotgg/t3code#4985)Fork deviations preserved
apps/server/src/ws.ts: upstream replaced the live path withstreamDomainEvents; the fork keepssubscribeDomainEvents+latestSequenceand its earlyliveBufferqueue. Upstream'sprojectActivityEventpruning was applied to the fork's live-buffer path as well, so all four sites (replay, live, catch-up, snapshot) prune — otherwise the fix would be silently defeated on the live path.isThisThreadDetailEventkeeps the fork'sthread.deleted/thread.archivedcases.BranchToolbarBranchSelector: kept the fork-localgetBranchTriggerLabel(upstream'sresolveBranchTriggerLabellives in aBranchToolbar.logic.tsextraction the fork never took) and the fork's pagination effect.Verification
vp run typecheckandvp checkpass; full server suite green (211 files).Summary by CodeRabbit