-
Notifications
You must be signed in to change notification settings - Fork 1k
fix: improve project favicon detection for monorepos #1024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ponbac
wants to merge
2
commits into
pingdotgg:main
Choose a base branch
from
ponbac:favicon-recursive
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+741
−259
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import { assert, beforeEach, describe, it, vi } from "vitest"; | ||
|
|
||
| import type { ProcessRunOptions, ProcessRunResult } from "./processRunner"; | ||
|
|
||
| const { runProcessMock } = vi.hoisted(() => ({ | ||
| runProcessMock: | ||
| vi.fn< | ||
| ( | ||
| command: string, | ||
| args: readonly string[], | ||
| options?: ProcessRunOptions, | ||
| ) => Promise<ProcessRunResult> | ||
| >(), | ||
| })); | ||
|
|
||
| vi.mock("./processRunner", () => ({ | ||
| runProcess: runProcessMock, | ||
| })); | ||
|
|
||
| function processResult( | ||
| overrides: Partial<ProcessRunResult> & Pick<ProcessRunResult, "stdout" | "code">, | ||
| ): ProcessRunResult { | ||
| return { | ||
| stdout: overrides.stdout, | ||
| code: overrides.code, | ||
| stderr: overrides.stderr ?? "", | ||
| signal: overrides.signal ?? null, | ||
| timedOut: overrides.timedOut ?? false, | ||
| stdoutTruncated: overrides.stdoutTruncated ?? false, | ||
| stderrTruncated: overrides.stderrTruncated ?? false, | ||
| }; | ||
| } | ||
|
|
||
| describe("gitIgnore", () => { | ||
| beforeEach(() => { | ||
| runProcessMock.mockReset(); | ||
| vi.resetModules(); | ||
| }); | ||
|
|
||
| it("chunks large git check-ignore requests and filters ignored matches", async () => { | ||
| const ignoredPaths = Array.from( | ||
| { length: 320 }, | ||
| (_, index) => `ignored/${index.toString().padStart(4, "0")}/${"x".repeat(1024)}.ts`, | ||
| ); | ||
| const keptPaths = ["src/keep.ts", "docs/readme.md"]; | ||
| const relativePaths = [...ignoredPaths, ...keptPaths]; | ||
| let checkIgnoreCalls = 0; | ||
|
|
||
| runProcessMock.mockImplementation(async (_command, args, options) => { | ||
| if (args[0] === "check-ignore") { | ||
| checkIgnoreCalls += 1; | ||
| const chunkPaths = (options?.stdin ?? "").split("\0").filter((value) => value.length > 0); | ||
| const chunkIgnored = chunkPaths.filter((value) => value.startsWith("ignored/")); | ||
| return processResult({ | ||
| code: chunkIgnored.length > 0 ? 0 : 1, | ||
| stdout: chunkIgnored.length > 0 ? `${chunkIgnored.join("\0")}\0` : "", | ||
| }); | ||
| } | ||
|
|
||
| throw new Error(`Unexpected command: git ${args.join(" ")}`); | ||
| }); | ||
|
|
||
| const { filterGitIgnoredPaths } = await import("./gitIgnore"); | ||
| const result = await filterGitIgnoredPaths("/virtual/workspace", relativePaths); | ||
|
|
||
| assert.isAbove(checkIgnoreCalls, 1); | ||
| assert.deepEqual(result, keptPaths); | ||
| }); | ||
|
|
||
| it("fails open when git check-ignore cannot complete", async () => { | ||
| const relativePaths = ["src/keep.ts", "ignored.txt"]; | ||
|
|
||
| runProcessMock.mockRejectedValueOnce(new Error("spawn failed")); | ||
|
|
||
| const { filterGitIgnoredPaths } = await import("./gitIgnore"); | ||
| const result = await filterGitIgnoredPaths("/virtual/workspace", relativePaths); | ||
|
|
||
| assert.deepEqual(result, relativePaths); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| import { runProcess } from "./processRunner"; | ||
|
|
||
| const GIT_CHECK_IGNORE_MAX_STDIN_BYTES = 256 * 1024; | ||
|
|
||
| /** | ||
| * Shared git-ignore helpers for server-side workspace scans. | ||
| * | ||
| * Both callers use these helpers as an optimization and a consistency layer, not | ||
| * as a hard dependency. If git is unavailable, slow, or returns an unexpected | ||
| * result, we intentionally fail open so the UI keeps working and avoids hiding | ||
| * files unpredictably. | ||
| */ | ||
|
|
||
| function splitNullSeparatedPaths(input: string, truncated: boolean): string[] { | ||
| const parts = input.split("\0"); | ||
| if (truncated && parts[parts.length - 1]?.length) { | ||
| parts.pop(); | ||
| } | ||
| return parts.filter((value) => value.length > 0); | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether `cwd` is inside a git work tree. | ||
| * | ||
| * This is a cheap capability probe used to decide whether later git-aware | ||
| * filtering is worth attempting. | ||
| */ | ||
| export async function isInsideGitWorkTree(cwd: string): Promise<boolean> { | ||
| const insideWorkTree = await runProcess("git", ["rev-parse", "--is-inside-work-tree"], { | ||
| cwd, | ||
| allowNonZeroExit: true, | ||
| timeoutMs: 5_000, | ||
| maxBufferBytes: 4_096, | ||
| }).catch(() => null); | ||
|
|
||
| return Boolean( | ||
| insideWorkTree && insideWorkTree.code === 0 && insideWorkTree.stdout.trim() === "true", | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Filters repo-relative paths that match git ignore rules for `cwd`. | ||
| * | ||
| * We use `git check-ignore --no-index` so both tracked and untracked candidates | ||
| * respect the current ignore rules. Input is chunked to keep stdin bounded, and | ||
| * unexpected git failures return the original paths unchanged so callers fail | ||
| * open instead of dropping potentially valid files. | ||
| */ | ||
| export async function filterGitIgnoredPaths( | ||
| cwd: string, | ||
| relativePaths: readonly string[], | ||
| ): Promise<string[]> { | ||
| if (relativePaths.length === 0) { | ||
| return [...relativePaths]; | ||
| } | ||
|
|
||
| const ignoredPaths = new Set<string>(); | ||
| let chunk: string[] = []; | ||
| let chunkBytes = 0; | ||
|
|
||
| const flushChunk = async (): Promise<boolean> => { | ||
| if (chunk.length === 0) { | ||
| return true; | ||
| } | ||
|
|
||
| const checkIgnore = await runProcess("git", ["check-ignore", "--no-index", "-z", "--stdin"], { | ||
| cwd, | ||
| allowNonZeroExit: true, | ||
| timeoutMs: 20_000, | ||
| maxBufferBytes: 16 * 1024 * 1024, | ||
| outputMode: "truncate", | ||
| stdin: `${chunk.join("\0")}\0`, | ||
| }).catch(() => null); | ||
| chunk = []; | ||
| chunkBytes = 0; | ||
|
|
||
| if (!checkIgnore) { | ||
| return false; | ||
| } | ||
|
|
||
| // git-check-ignore exits with 1 when no paths match. | ||
| if (checkIgnore.code !== 0 && checkIgnore.code !== 1) { | ||
| return false; | ||
| } | ||
|
|
||
| const matchedIgnoredPaths = splitNullSeparatedPaths( | ||
| checkIgnore.stdout, | ||
| Boolean(checkIgnore.stdoutTruncated), | ||
| ); | ||
| for (const ignoredPath of matchedIgnoredPaths) { | ||
| ignoredPaths.add(ignoredPath); | ||
| } | ||
| return true; | ||
| }; | ||
|
|
||
| for (const relativePath of relativePaths) { | ||
| const relativePathBytes = Buffer.byteLength(relativePath) + 1; | ||
| if ( | ||
| chunk.length > 0 && | ||
| chunkBytes + relativePathBytes > GIT_CHECK_IGNORE_MAX_STDIN_BYTES && | ||
| !(await flushChunk()) | ||
| ) { | ||
| return [...relativePaths]; | ||
| } | ||
|
|
||
| chunk.push(relativePath); | ||
| chunkBytes += relativePathBytes; | ||
|
|
||
| if (chunkBytes >= GIT_CHECK_IGNORE_MAX_STDIN_BYTES && !(await flushChunk())) { | ||
| return [...relativePaths]; | ||
| } | ||
| } | ||
|
|
||
| if (!(await flushChunk())) { | ||
| return [...relativePaths]; | ||
| } | ||
|
|
||
| if (ignoredPaths.size === 0) { | ||
| return [...relativePaths]; | ||
| } | ||
|
|
||
| return relativePaths.filter((relativePath) => !ignoredPaths.has(relativePath)); | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated
splitNullSeparatedPathshelper across two filesLow Severity
splitNullSeparatedPathsis duplicated betweengitIgnore.tsandworkspaceEntries.tswith near-identical implementations. The PR explicitly set out to extract shared logic (likefilterGitIgnoredPathsandisInsideGitWorkTree) intogitIgnore.ts, but this helper was copied rather than shared. The two copies have a trivial difference (an unreachableparts.length === 0guard inworkspaceEntries.ts), and any future bug fix to one copy risks not being applied to the other.Additional Locations (1)
apps/server/src/workspaceEntries.ts#L188-L199