sync: absorb upstream v0.0.30 (27 commits) - #33
Conversation
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
…g#4705) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…4787) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: codex <codex@users.noreply.github.com>
…pingdotgg#4791) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
PaletteIcon joins the lucide->Phosphor shim (upstream's new appearance settings category imports it), and SidebarV2 re-imports FolderIcon for upstream's redesigned project-actions dialog — its previous import arrived with the upstream scope-menu code the fork replaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thermo-nuclear code quality review — not approved.
Fork conflict absorption for the named surfaces (AppSidebarLayout, SidebarChrome, AGENTS.md, PaletteIcon shim) is mostly clean. The approval bar fails on structural regressions this sync pulls in and on a permanent dual-path the fork absorb locks in.
Ordered findings
-
Structural regression —
GitVcsDriverCorelistRefs cache lattice (2581 → 2796LOC; test976 → 1445, crosses 1k). Dual epoch + generation Maps, dual Effect caches, and awhile (true)coherence spin is storm-correct and design-wrong. Extract a singleGitRefsSnapshotCache(get/invalidate, one monotonic version) instead of absorbing another lattice into a 2.7k driver. -
Missed code-judo / dual-path debt — environment identification. Fork header is always-on brand chrome; composer/auth still honor the new setting; Appearance copy claims the setting chooses how Dev/Nightly are identified. That is three policies for one concept. Collapse behind one surface-aware resolver (
sidebarHeader: always-artwork) and make the settings copy tell the truth for this fork. -
Spaghetti / god-file growth — Appearance panel appended to
SettingsPanels(1765 → 1839). New category belongs in its own module; the route is already a one-liner. -
Boundary / dual resolve — ChatMarkdown +
markdown-links. Prefetch map with runtime??fallback keeps policy split across a 1.6k component and the new module. Resolve once; delete the fallback path. -
1k-line rule —
GitVcsDriverCore.test.tscrossed 1000 (+469). Split by cache/invalidation concern before this file becomes the next dumping ground. -
Maintainability residue —
SidebarV2stays a fence nuclear zone (~2976 LOC, dozens offork:begins; unbalancedsidebar-v2-card-rowsfence still present). Sync preserved intent but did not reduce concept count. Prefercustom//overrides/ownership over more leaf fences on the next touch.
Credit
Prompt-stash provider-scope removal, dead artwork-gated trigger drop in AppSidebarLayout, and the single useSidebarV2Enabled door are real simplifications.
Approval bar
Blocked: unjustified god-file / 1k growth, absorb of a dual-cache lattice that should have been extracted, and a fork dual-path that makes the new identification setting partially lie. Fix or clearly justify those before this should merge on maintainability grounds.
Sent by Cursor Automation: Thermo-nuclear PR review
| const listRefsEpochByCommonDir = new Map<string, number>(); | ||
| let listRefsEpochSequence = 0; | ||
| const bumpListRefsEpoch = (gitCommonDir: string): number => { | ||
| const nextEpoch = ++listRefsEpochSequence; | ||
| listRefsEpochByCommonDir.delete(gitCommonDir); | ||
| listRefsEpochByCommonDir.set(gitCommonDir, nextEpoch); | ||
| if (listRefsEpochByCommonDir.size > LIST_REFS_SNAPSHOT_CACHE_CAPACITY) { | ||
| const oldestKey = listRefsEpochByCommonDir.keys().next().value; | ||
| if (oldestKey !== undefined) { | ||
| listRefsEpochByCommonDir.delete(oldestKey); | ||
| } | ||
| if (remoteNamesResult.exitCode !== 0 && remoteNamesResult.stderr.trim().length > 0) { | ||
| yield* Effect.logWarning( | ||
| `GitVcsDriver.listRefs: remote name lookup returned code ${remoteNamesResult.exitCode} for ${input.cwd}: ${remoteNamesResult.stderr.trim()}. Falling back to an empty remote name list.`, | ||
| ); | ||
| } | ||
| return nextEpoch; | ||
| }; | ||
| const listRefsGenerationByCommonDir = new Map<string, number>(); | ||
| let listRefsGenerationSequence = 0; | ||
| const setListRefsGeneration = (gitCommonDir: string, generation: number): number => { | ||
| listRefsGenerationByCommonDir.delete(gitCommonDir); | ||
| listRefsGenerationByCommonDir.set(gitCommonDir, generation); | ||
| if (listRefsGenerationByCommonDir.size > LIST_REFS_SNAPSHOT_CACHE_CAPACITY) { | ||
| const oldestKey = listRefsGenerationByCommonDir.keys().next().value; | ||
| if (oldestKey !== undefined) { | ||
| listRefsGenerationByCommonDir.delete(oldestKey); | ||
| } | ||
|
|
||
| const defaultBranch = | ||
| defaultRef.exitCode === 0 | ||
| ? defaultRef.stdout.trim().replace(/^refs\/remotes\/origin\//, "") | ||
| : null; | ||
|
|
||
| const worktreeMap = new Map<string, string>(); | ||
| if (worktreeList.exitCode === 0) { | ||
| let currentPath: string | null = null; | ||
| for (const line of worktreeList.stdout.split("\n")) { | ||
| if (line.startsWith("worktree ")) { | ||
| const candidatePath = line.slice("worktree ".length); | ||
| const exists = yield* fileSystem.stat(candidatePath).pipe( | ||
| Effect.map(() => true), | ||
| Effect.orElseSucceed(() => false), | ||
| } | ||
| return generation; | ||
| }; | ||
| const currentListRefsGeneration = (gitCommonDir: string): number => { | ||
| const current = listRefsGenerationByCommonDir.get(gitCommonDir); | ||
| return current === undefined | ||
| ? setListRefsGeneration(gitCommonDir, ++listRefsGenerationSequence) | ||
| : setListRefsGeneration(gitCommonDir, current); | ||
| }; | ||
| const bumpListRefsGeneration = (gitCommonDir: string): number => | ||
| setListRefsGeneration(gitCommonDir, ++listRefsGenerationSequence); | ||
| const listRefsSnapshotCache = yield* Cache.makeWith( | ||
| (cacheKey: GitRefsSnapshotCacheKey) => readGitRefsSnapshot(cacheKey.gitCommonDir), | ||
| { | ||
| capacity: LIST_REFS_SNAPSHOT_CACHE_CAPACITY, | ||
| timeToLive: (exit) => (Exit.isSuccess(exit) ? LIST_REFS_SNAPSHOT_CACHE_TTL : Duration.zero), | ||
| }, | ||
| ); | ||
| const listRefsRefreshSnapshotCache = yield* Cache.makeWith( | ||
| (cacheKey: GitRefsRefreshCacheKey) => | ||
| Effect.suspend(() => { | ||
| const epoch = bumpListRefsEpoch(cacheKey.gitCommonDir); | ||
| return Cache.get( | ||
| listRefsSnapshotCache, | ||
| new GitRefsSnapshotCacheKey({ gitCommonDir: cacheKey.gitCommonDir, epoch }), | ||
| ); | ||
| }), | ||
| { | ||
| capacity: LIST_REFS_SNAPSHOT_CACHE_CAPACITY, | ||
| timeToLive: (exit) => | ||
| Exit.isSuccess(exit) ? LIST_REFS_REFRESH_COALESCE_TTL : LIST_REFS_REFRESH_FAILURE_COOLDOWN, | ||
| }, | ||
| ); | ||
| 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.
Structural regression / missed code-judo. This absorb lands a second cache lattice (epoch Map + generation Map + snapshot cache + refresh cache + while (true) coherence spin) inside an already ~2.8k driver.
Storm-coalescing is the right goal. This shape is not. Epoch and generation are doing overlapping jobs, and the spin loop is the tell that the model is fighting itself.
Extract something like GitRefsSnapshotCache with get(commonDir, { refresh }) / invalidate(cwd) and one monotonic version. Delete either epoch or generation — not both. Same treatment as the repository-paths dual cache above it. Absorbing upstream without that extraction makes the driver harder to reason about every sync.
| {/* Always, not only on a non-prod build: in the fork this is brand | ||
| chrome rather than a channel cue. Upstream's other two art surfaces | ||
| (composer send button, auth screen) still gate on the variant and are | ||
| untouched, so they keep signalling Dev. */} | ||
| chrome rather than a channel cue, so it also ignores the environment | ||
| identification setting upstream added for its own header art — that | ||
| setting still governs upstream's other two art surfaces (composer | ||
| send button, auth screen), which are untouched and keep signalling | ||
| Dev. */} | ||
| <ForkSidebarHeaderBackdrop stageLabel={stageLabel} /> |
There was a problem hiding this comment.
Dual-path debt from the absorb. Fork header artwork is always-on brand chrome and intentionally ignores the new environment-identification setting, while composer/auth still honor it (ComposerPrimaryActions via useEnvironmentIdentificationMode). Appearance settings then describe the setting as choosing how Dev/Nightly are identified — which is false for this surface.
That is three policies for one concept, glued together with comments. Collapse behind one fork resolver, e.g. resolveEnvironmentIdentification({ surface }) with an explicit sidebarHeader: 'always-artwork' policy, and point header / composer / auth / settings copy at it. Otherwise every future upstream art change will re-open this conflict.
| } | ||
|
|
||
| export function GeneralSettingsPanel() { | ||
| export function AppearanceSettingsPanel() { |
There was a problem hiding this comment.
File-size / decomposition. AppearanceSettingsPanel is a new settings category appended into an already ~1.8k god file (1765 → 1839). The route (settings.appearance.tsx) is already a thin shell — the panel should live in its own module now, not after the next appearance feature forces a split under pressure.
Also: the Environment identification row copy below claims users "Choose how Dev and Nightly environments are identified," but the fork sidebar header ignores that setting. Either move this panel out and make the description surface-accurate, or the settings UI is lying.
| const fileLinkMeta = | ||
| inlineCodeFileLinkMetaByText.get(codeText.trim()) ?? | ||
| resolveInlineCodeFileLinkMeta(codeText, cwd); |
There was a problem hiding this comment.
Boundary leak / dual resolve. Prefetch map + runtime ?? resolveInlineCodeFileLinkMeta(...) means inline-code link policy still has two call sites — one "optimized" path and a silent fallback inside a 1.6k component — while markdown-links.ts was supposed to own the model.
Resolve once in markdown-links (or guarantee the map is complete for every span the renderer can see) and delete this fallback. If the map can miss, that is an invariant bug, not a reason to keep a second resolver in the leaf.
| return input.configuredByUser || input.enabled | ||
| ? input.enabled | ||
| : resolveSidebarV2Default(input.stageLabel); |
There was a problem hiding this comment.
Ghost state in the hot path. configuredByUser || enabled ? enabled : default preserves a legacy third state ("stored true without the companion flag") on every resolve. The migration intent is real; keeping it as permanent runtime branching is not.
Code-judo: one-shot normalize on settings load (enabled && !configuredByUser → set configuredByUser), then the runtime path is just configuredByUser ? enabled : default(stage). Delete the ghost branch so sidebar-v2 resolution stops carrying forever-debt for a schema that no longer needs it.
| const [viewStack, setViewStack] = useState<CommandPaletteView[]>([]); | ||
| const currentView = viewStack.at(-1) ?? null; | ||
| const [browseGeneration, setBrowseGeneration] = useState(0); | ||
| const browseNavigationRef = useRef<ReturnType<typeof createBrowseNavigationCoordinator> | null>( |
There was a problem hiding this comment.
God-component growth (2094 → 2192). The new shared filesystem browse coordinator is the right idea — initializing/owning it inside CommandPalette.tsx is not. Pull browse orchestration into something like useCommandPaletteBrowse so the palette stops accumulating another independent subsystem. Next sync will otherwise keep paying the 2k+ LOC tax here.
| listEmpty | ||
| ); | ||
|
|
||
| if (threadListV2Enabled) { |
There was a problem hiding this comment.
Dual tree about to cross 1k (983 → 998). if (threadListV2Enabled) swapping entire list implementations inside HomeScreen is the spaghetti form of a feature flag. Split HomeThreadListV1 / HomeThreadListV2 (or delete v1 now that v2 is default) before this file tips over 1000 with the next mobile polish pass.
Review: upstream v0.0.30 syncI reviewed this as a conflict-resolution PR, not as 27 upstream commits — the fork's actual authored surface here is the merge resolution in 13 files plus The resolutions are good. I checked each of the four documented ones against both parents and they hold up — I've listed the verification at the bottom. What I want to push on is what this sync didn't write down, plus one visual regression the merge structurally cannot see. 1. The environment-identification setting is now partly inert in the fork, and nothing records thatUpstream pingdotgg#4652 added Net effect on a fork Dev build — the only build where the row is even shown (
Two of the three options do nothing to the surface the setting is named after, and "Version pill" shows no pill at all — upstream only ever rendered it in The "Behavioral note" in the PR body is the right call, but a PR body doesn't survive the merge. Nothing else catches this either: upstream ships no render test on the pill, and Pick one and land it with the sync: honor 2. Zero manifest or guard changes in a sync that made four design decisions
But this sync decided: drop upstream's 3. Likely visual regression:
|
Honor upstream's environment-identification pill in the fork header: the brand art never gates on the setting, but 'Version pill' now renders upstream's badge (white-on-art) so every option does something visible; guarded, and the decision is recorded in the fork-sidebar-chrome intent. Restore the fork chrome rows' icon tint: sidebarMenuButtonVariants' new parent-level [&>svg] pair (muted at opacity-60) outweighed the icons' own /80 class; the fork buttons now re-assert it via twMerge. Move the fork-authored Change Scope section back ahead of upstream's first heading in AGENTS.md and fence it as fork-change-scope, with a position guard under fork-workflow-docs. Close the unterminated sidebar-v2-card-rows fence and add a repo-wide fence-balance assertion to the manifest guard. Record the sync's four resolution decisions (pill, fixedHeader decline, status-slot decline, stage-label hook swap) in manifest intents. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All feedback addressed in 755e8c7, verified live in a dev environment. Point by point: §1 — environment identification. Landed the first of your two options, with a twist that keeps the fork intent intact: the header art still never consults the setting (brand chrome), but "Version pill" now renders upstream's badge in the fork header, white-on-art since it always sits on the artwork. That makes all three options distinct and true: artwork gates the composer/auth surfaces, pill shows a badge here, none shows no explicit identification. Verified all three modes in the browser. The decision is recorded in the §2 — manifest record of the sync decisions. Landed with the sync rather than as a follow-up, since §1 forced the manifest open anyway. §3 — confirmed regression, fixed. Measured before the fix: search, new-thread, and new-project icons at computed §4 — Change Scope. Moved ahead of upstream's first §5 — fence closed and now guarded. §6 — explained and corrected in the PR body. 846 was real but meaningless: run from the repo root, Your question — header pinning: confirmed in a real browser. Two checks: the scroller's box is bounded by the flex chain in normal layout (clientHeight 572 with the group's bottom at 704 in an 800px viewport — |
|
Re the Cursor automation review: findings 2 and 6 overlap the human review and are addressed in 755e8c7 (the identification setting now behaves honestly in the fork — pill honored, decision recorded in the manifest; the unbalanced The remaining findings (1, 3, 4, 5 — |
NoahHendrickson
left a comment
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code review: upstream v0.0.30 sync
High-effort multi-agent review of the full diff (152 files): 8 finder angles → 27 candidates → dedup to 20 → independent verification of each. 14 confirmed, 3 plausible, 3 refuted. Top 10 below, ranked most-severe first. Verdict legend: CONFIRMED = demonstrated from the code; PLAUSIBLE = realistic but not fully demonstrated.
A note on scope: findings 1, 2, 6, 7, 9, and 10 are upstream-authored behavior the sync carries — the ask there is a deliberate keep/counter decision (and a manifest intent where countered), not necessarily a code change in this PR. Findings 3, 4, 5, and 8 are fork-side and directly actionable here.
1. Branch list can be minutes stale after out-of-band git — CONFIRMED (upstream)
apps/server/src/vcs/GitVcsDriverCore.ts:2393
listRefs now serves a 2-minute-cached ref snapshot (LIST_REFS_SNAPSHOT_CACHE_TTL, line 61) with a 10-minute-cached current-branch fallback (REPOSITORY_PATHS_CACHE_TTL, line 57). Invalidation only wraps driver-initiated mutations (withListRefsInvalidation, line 2743), no production client ever sends the new refresh: true escape hatch (grep: only vcs.test.ts), and this diff also removed the branch picker's on-open branchRefState.refresh(). Run git checkout -b feature-x in a terminal and the picker misses the new branch for up to 2 minutes; when the snapshot's worktree-path match misses, the current-branch checkmark is wrong for up to 10. Pre-merge, every open re-ran git and was always fresh.
2. v1 prompt stash deleted at startup without migration — CONFIRMED (upstream, data loss)
apps/web/src/promptStashStore.ts:274
Module init unconditionally runs removeItem(LEGACY_PROMPT_STASH_STORAGE_KEY) and readPersistedEntries (line 177) reads only the v2 key. The v1 key shipped on custom pre-sync, so a fork user who stashed prompts before upgrading loses them (and their image attachments) unrecoverably on first launch. Upstream documents this as deliberate (lines 8–13, localStorage quota rationale) — but the sync silently accepts the data loss for existing fork users. Worth a conscious decision, and a release-note line if kept.
3. Fork header divergence sits above its own fork:begin fence — CONFIRMED (fork-side)
apps/web/src/components/sidebar/SidebarChrome.tsx:54
The fork's central divergence in this header — the always-rendered ForkSidebarHeaderBackdrop (line 54), its rationale comment (48–53), the px-0 header padding rewrite (40–46), and the ~/custom import (line 13) — all sit above the fork:begin fork-sidebar-chrome fence, which opens at line 55. Fence-based sync audits can't see those lines; the string-grep guards backstop only the backdrop render, not the padding rewrite. The next conflict here can be resolved to upstream's side straight through the unfenced lines. Fix: open the fence before line 13/40 so the contested lines live inside the mechanism the sync workflow actually honors.
4. [&>svg] counter-override duplicated in two literals, no guard — CONFIRMED (fork-side)
apps/web/src/custom/SidebarV2ChromeRows.tsx:85, 109
The [&>svg]:text-sidebar-muted-foreground/80 [&>svg]:opacity-100 pair that displaces upstream's new icon dim is pasted into both TRAILING_BUTTON and the search-row className, coupled only by a "mirrors TRAILING_BUTTON's" comment. No guard covers it (grep of __fork_guards__ for opacity-100/&>svg: zero hits). Next upstream restyle of that slot — or a selector-shape change ([&_svg], data-slot CSS, ! modifier) that twMerge no longer displaces — double-fades one row's duotone icons with every test green: the exact regression this pair was added to fix. Fix: hoist into one shared const, and add a guard asserting the merged cn(sidebarMenuButtonVariants(...), TRAILING_BUTTON) outcome drops opacity-60.
5. Fence-balance guard scans all ~15.6k tracked files — CONFIRMED (fork-side)
apps/web/src/__fork_guards__/customizationsManifest.test.ts:109
The new balance test runs its own git ls-files + readFileSync walk over every tracked file (~195 MB including ~13k vendored .repos/ files; measured ~3.8s warm per run), duplicating the sibling test's walk at line 77 with already-diverged filters (extension whitelist vs. everything; .fork/ excluded in only one), and compiles fresh RegExps per file per kind. Only ~43 tracked files contain fence markers. Cheaper and convergent: one shared git grep -lE "fork:(begin|end)" candidate pass (~0.7s measured) feeding both tests, one precompiled /fork:(begin|end) ([a-z0-9-]+)/gu matchAll.
6. Thread error tooltip hardcodes "Error occurred" — CONFIRMED (upstream)
apps/web/src/components/SidebarV2.tsx:359
custom previously rendered {thread.session.lastError} in the tooltip (old line 352); upstream's restyle (dd5ea32) replaces it with the literal string Error occurred, and grep shows the real message now renders nowhere in the sidebar — only inside the opened chat view. Diagnosing a failed session from the sidebar is no longer possible. Candidate for a small fenced fork restore.
7. Mobile thread-list opt-out ignored until prefs load — CONFIRMED (upstream)
apps/mobile/src/features/threads/threadListV2.ts:37
resolveThreadListV2Enabled returns the default (true) whenever preferences aren't successfully loaded. A device with threadListV2Enabled: false persisted mounts the v2 list every launch and remounts to v1 a tick later (losing scroll/expanded-group state); if both the SQLite and secure-storage reads fail (mobile-preferences.ts:228), loaded stays false forever and the opt-out is ignored all session. The startup window is documented as a tradeoff (lines 28–31); the persistent-failure path isn't.
8. Comment + manifest wrongly claim the setting gates auth-screen art — CONFIRMED (fork-side, docs)
apps/web/src/components/sidebar/SidebarChrome.tsx:48
The new comment ("that setting still governs upstream's other two art surfaces (composer send button, auth screen)", and "none produces neither pill nor composer/auth art") and the matching .fork/customizations.yaml text are factually wrong about the auth half: AuthSurfaceShell.tsx:11 resolves stage art with enabled defaulting true and never reads environmentIdentificationMode — upstream never gated that surface either (the mode gates only AppSidebarLayout, ComposerPrimaryActions, SidebarChrome). The manifest is the fork's sync-resolution contract; wrong text invites a wrong conflict resolution or a bogus bug hunt next sync. Fix the comment + manifest wording (or actually gate the auth surface).
9. Stored sidebarV2Enabled: false flips to v2 on Dev/Nightly — PLAUSIBLE (upstream)
apps/web/src/hooks/useSettings.ts:251
The old schema recorded the Beta toggle as plain false with no configuredByUser companion, so a pre-sync explicit opt-out decodes as {enabled: false, configuredByUser: false} and resolveSidebarV2Enabled (branding.logic.ts:53–57) applies the stage default — Dev/Nightly users who turned v2 off come up with v2 on after upgrading, plus one documented v1→v2 remount after async hydration. Upstream knowingly accepted the ambiguity (settings.ts:124–128) as part of the beta rollout; flagging because pre-sync opt-outs are silently overridden.
10. acceptsGzip misses gzip ; q=1 — coding token not trimmed — CONFIRMED (upstream, minor)
apps/server/src/http.ts:54
entry.trim().toLowerCase().split(";") trims the entry but never the coding token after the split (parameters do get .trim() at line 56), so the RFC-9110-legal OWS-before-; form yields the key "gzip " and accepted.get("gzip") misses — that client silently gets uncompressed responses. One-word fix: trim the coding after the split.
Refuted during verification (for the record): header art ignoring the "None" identification mode is a documented, guard-tested fork decision, not a defect; the fork-change-scope fence-id/anchor shape follows the pre-existing fork-workflow convention with manifest + guard coverage; the declined "Settle" text label is already enforced by sidebarV2RowActionHitArea.test.ts. Seven further low-severity confirmed items (mostly fragile string-window assertions in the new guard tests) fell below the 10-finding cap.
🤖 Generated with Claude Code
Fence the whole fork-divergent header in SidebarChrome — the always-on backdrop, px-0 rewrite, hooks, and the ~/custom import sat above the fork:begin, outside what a sync conflict resolution honors. Hoist the [&>svg] icon-tint counter-override into one shared CHROME_ROW_ICON_TINT const and guard the merged outcome, so a base selector reshape that stops displacing upstream's dim turns CI red. Share one git-grep candidate pass across both manifest fence tests instead of two full-tree walks (test phase 3.6s -> 0.25s). Correct the comment and manifest claim that the identification setting gates the auth screen — upstream gates that art on the build channel alone; the setting's fork-visible effects are the composer button and the header pill. Restore the session's actual last error in the thread tooltip, which upstream's restyle flattened to a literal 'Error occurred'; new sidebar-v2-error-tooltip customization with manifest intent and guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…se note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All ten findings triaged; fork-side fixes landed in 458acfc (+ ac649cb for the release note), guards 178/178, typecheck and fork-lint clean. Fork-side, fixed: #3 — fence coverage. The #4 — icon tint hoisted and guarded. One exported #5 — guard perf. Both fence tests now share one module-level #8 — docs corrected. You're right: Upstream-carried, decided: #6 — countered. The tooltip renders #2 — kept, with a durable record. Upstream's migration-less v1 stash cleanup is deliberate on their side and not worth a permanent storage-layer divergence. The user-facing consequence is now written into #1, #7, #9, #10 — kept, no fork divergence. All four live in |
5 participants
Brings
customup toupstream/mainat694f8d1c6(v0.0.30) — 27 commits, including the sidebar-v2-by-default rollout, the queued-row rebuild, 33 web UI fixes, websocket/gzip perf work, and the agent-guidance docs overhaul.Conflict resolutions
AGENTS.md— upstream rewrote the whole document. Took upstream's new body wholesale and re-appended the two fork sections at the end: the fork-authored Change Scope guidance and the fenced Fork Workflow section (fork-workflow-docsguard requires the fences).CLAUDE.mdsymlink blob untouched.AppSidebarLayout.tsx— upstream's new artwork-gated trigger styling only applies while the sidebar is visible, a state the fork's floating control never renders in (fork-sidebar-chromemoved the toggle into the header). Dropped the dead lines and the now-unuseduseEnvironmentIdentificationModeimport.SidebarChrome.tsx— kept the fork header (always-on brand backdrop, inline toggle,APP_BASE_NAMEbrand) perfork-sidebar-chrome/fork-app-identityintent; the fork's local stage-label hook is replaced by upstream's equivalent newuseEnvironmentStageLabel. Took upstream's simplified footer (fork never touched it).SidebarV2.tsx— upstream redesigned its card row in the same regionsidebar-v2-card-rows/sidebar-v2-row-action-hit-areaown; fork design wins in the fenced spans, upstream's tooltip/dialog/ghost-row refinements outside the fences are absorbed. Upstream'sfixedHeadercontrol rows lose to the fork'sSidebarV2SearchRow/SidebarV2ProjectScopeRow(the fork list already self-scrolls, so header pinning is preserved).Follow-up fixes in the sync
PaletteIconadded to the lucide→Phosphor shim (upstream's new appearance settings category imports it — caught by the shim guard).FolderIconre-imported inSidebarV2.tsxfor upstream's redesigned project-actions dialog.Behavioral note
Upstream's new environment identification setting (artwork/pill) does not affect the fork's sidebar header — that art is always-on brand chrome per
fork-sidebar-chrome. The setting still governs upstream's other art surfaces (composer send button, auth screen).Verification
vp test run apps/web/src/__fork_guards__path-matches the stale.claude/worktrees/*review checkouts and counts their copies of the suite too. Run it fromapps/web(vp test run src/__fork_guards__) to reproduce the real number.apps/webtypecheck: clean.fork/lint-owned.mjs: 52 files, no warnings;.fork/detect-drift.mjsreport reviewed — every flagged customization's guard is green🤖 Generated with Claude Code