perf(desktop): restore project context during startup - #6939
perf(desktop): restore project context during startup#6939thomaspblock wants to merge 5 commits into
Conversation
Restore the last validated project collection immediately and resolve the active channel with a scoped relay lookup so project context no longer waits on community-wide enumeration. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Move relay project fetching into a focused module so the startup optimization remains maintainable and passes the repository file-size gate. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Preserve project snapshot hydration and scoped home lookup alongside the extracted project deletion mutation flow from main. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
🔐 Codex Security Review
|
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 745ff6066c92372ea1ee6a5fe05862cdd9b81303..2a3ea94a957933c61b9d9547d9e76980ae34491b (exact head 2a3ea94a957933c61b9d9547d9e76980ae34491b)
Risk: high — startup hydration now feeds stale relay-derived project/repository state into a component with an automatic publication side effect, and adds a potentially large persistent cache.
Behavior/contracts traced: relay/identity-scoped snapshot persistence → React Query hydration → active-channel project resolution → ProjectChannelHome mount; project/repository authorization and healing publication; localStorage budgeting/recovery; stale-cache fallback; exact-head Desktop CI.
Blocking findings
-
[High] Snapshot-derived repository state can trigger an unauthorized stale heal.
seedProjectSnapshotinstalls staleProjectmodels directly into the live['projects']key (desktop/src/features/projects/projectSnapshot.ts:98-106).ChannelRouteScreenimmediately treats a matching snapshot entry as the project home and mountsProjectChannelHome(desktop/src/app/routes/ChannelRouteScreen.tsx:139-148,285-293), which unconditionally invokes repository healing (desktop/src/features/projects/ui/ProjectChannelHome.tsx:197). Healing derives missing attachments from the snapshot's repositories (desktop/src/features/projects/useHealProjectHomeRepositories.ts:33-55), while the attach mutation refetches only the project head and never revalidates the current repository announcement (desktop/src/features/projects/useAttachProjectRepository.ts:28-64). If a repository owner has since removed the project channel binding or revoked the project owner as maintainer without changing the project event, startup can republish the stale repository address and reverse that current authorization decision. The project timestamp guard does not fence this case because the project head is unchanged.Author action: do not permit healing/writes from snapshot-derived projects, or refetch and validate the repository's current channel binding plus owner/maintainer authorization before attach. Add deterministic coverage for stale repository authorization/binding → startup → no publication.
Verification owner: author (Desktop unit plus startup E2E); reviewer reruns at the new exact head.
-
[High] A stale non-matching snapshot disables the scoped lookup this PR introduces. Hydration uses
setQueryData(..., { updatedAt: 0 })(desktop/src/features/projects/projectSnapshot.ts:98-106), which makes React Query report a successful query while the authoritative enumeration is still fetching. The scoped home-channel lookup is enabled only when!projectsQuery.isSuccess(desktop/src/app/routes/ChannelRouteScreen.tsx:139-146). Therefore, when another client has added or changed the active project since this device's snapshot, the active channel is absent from the snapshot and the scoped lookup remains disabled; the route waits for the same community-wide enumeration the PR intends to avoid. A realQueryClient/QueryObserverreproduction at this head producedstatus=success,fetchStatus=fetching,enumeratedProjectHome=null, andscopedLookupEnabled=false.Author action: distinguish placeholder snapshot state from completed authoritative enumeration when gating the scoped query. Add a regression proving stale non-matching snapshot + pending enumeration runs the scoped lookup and can render the project home.
Verification owner: author for regression; reviewer for exact-head rerun.
-
[Medium] The new disposable snapshot bypasses cache budgeting and eviction.
persistProjectSnapshotwrites the complete project collection underbuzz-projects.v1:*(desktop/src/features/projects/projectSnapshot.ts:109-127), but that prefix is absent fromPURE_CACHE_KEY_PREFIXES(desktop/src/shared/lib/localStorageQuota.ts:10-27). It is therefore neither bounded by the 2 MiB disposable-cache budget nor evicted by startup/write quota recovery (desktop/src/shared/lib/localStorageQuota.ts:77-100,110-119,146-169). A quota-limited reproduction at this head left the project snapshot present while a durable community-state write failed. Large project/repository collections can crowd out preferences and read positions while the disposable cache survives.Author action: classify
buzz-projects.v1:as pure cache (or provide equivalent bounding/eviction), with oversized-single-entry, LRU/budget, and startup/write-recovery regressions proving durable state survives.Verification owner: author for Desktop unit coverage; reviewer for exact-head rerun.
Validation
- PASS: full
pnpm --dir desktop test— 5,709/5,709 in the product/adversarial lane. A separate full-suite run observed one untoucheduseDocumentVisibletimer failure; isolated rerun passed 5/5, so this is recorded as a reviewer-observed flake rather than a PR defect. - PASS:
pnpm --dir desktop typecheck. - PASS:
pnpm --dir desktop check(existing warnings/information; exit 0). - PASS: changed targeted tests — 15/15.
- PASS:
git diff --checkagainst the pinned base. - PASS: live exact-head GitHub checks: Desktop Core, macOS build, four Desktop smoke shards, two integration shards, aggregate Desktop, release candidate, and DCO.
- Freshness rechecked immediately before submission: live PR base/head remained the SHAs above; authenticated reviewer
jedwards27is not PR authorthomaspblock.
Manual/native evidence: not run. No native Desktop journey or numeric cold-start benchmark was captured, so actual startup latency/paint and stale→live transition remain unobserved. These are confidence gaps, not additional author actions; the three deterministic defects above independently require changes.
Residual risk: same-second replaceable project heads are guarded only by timestamp ordering, but the stale repository-authorization case does not depend on that edge. No directly changed keyboard/touch/accessibility control was identified.
Keep hydrated project snapshots out of publication decisions while preserving the scoped cold-start lookup, and subject project snapshots to disposable-cache budgeting. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖 Addressed the three blocking findings in
Snapshot data can still paint the project home immediately, but repository healing is now disabled until the project came from a completed relay enumeration or live scoped lookup. This is enforced at both
The route now distinguishes timestamp-zero snapshot hydration from authoritative relay data. A stale snapshot without the active project keeps the scoped
Validation: full Desktop tests, Desktop check/typecheck, file-size gate, focused 32-test startup/cache suite, and both new Playwright cold-start scenarios pass. Pre-push hooks also passed all Desktop lanes. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 745ff6066c92372ea1ee6a5fe05862cdd9b81303..b81c62090e0832fefffda849a31b69e364e51dcb (fresh re-review of exact head b81c62090e0832fefffda849a31b69e364e51dcb; prior reviewed head 2a3ea94a957933c61b9d9547d9e76980ae34491b)
Risk: high — the remediation still uses mutable cache metadata as authorization/provenance for an automatic relay publication side effect.
Behavior/contracts traced: persisted snapshot → React Query hydration/provenance → active-channel scoped lookup → project-home render → repository healing; all project-query mutation writers; pure-cache quota recovery; exact-head Desktop unit/E2E and CI gates.
Blocking finding
[High] Any local project cache mutation certifies the entire stale snapshot as authoritative. The remediation defines authority as dataUpdatedAt > 0 (desktop/src/features/projects/projectSnapshot.ts:25-30) after seeding snapshots at timestamp zero (desktop/src/features/projects/projectSnapshot.ts:123-131). React Query's timestamp is cache freshness metadata, not relay provenance: every setQueryData updates it. Existing mutation success paths write to the shared snapshot-backed ['projects'] collection, including create (desktop/src/features/projects/useCreateProject.ts:45-56), add channel (desktop/src/features/projects/useAddProjectChannel.ts:185-194), attach repository (desktop/src/features/projects/useAttachProjectRepository.ts:111-123), add repository (desktop/src/features/projects/useAddProjectRepository.ts:324-329), bind channel (desktop/src/features/projects/useBindProjectRepositoryChannel.ts:54-65), and delete (desktop/src/features/projects/projectDeletionMutation.ts:16-22).
Both independent exact-head reproductions held the exhaustive enumeration pending, seeded dataUpdatedAt=0, and performed an ordinary setQueryData update. The cache timestamp became current and isAuthoritativeProjectData changed from false to true (one recorded result: fetchStatus=fetching, dataUpdatedAt=1787872972113, isAuthoritativeProjectData=true).
ChannelRouteScreen then treats every retained snapshot row as trusted and enables healing (desktop/src/app/routes/ChannelRouteScreen.tsx:147-159,296-304; the project-detail route has the same authority input at desktop/src/app/routes/ProjectDetailScreen.tsx:694-702). Thus, while full enumeration is pending or offline, successfully creating/editing/deleting any project can re-enable healing for an unrelated stale snapshot project. A repository whose current buzz-channel binding or maintainer authorization changed can again be republished from stale state. The same provenance collapse suppresses the scoped lookup for a nonmatching stale project. The added helper/unit and cold-start E2E cases cover an untouched snapshot, not this mutation interleaving.
Author action: replace timestamp inference with explicit provenance that only completed relay enumeration can establish for the collection, while preserving per-project authority for scoped-live or newly written records without upgrading unrelated snapshot rows. Add deterministic snapshot → pending enumeration → unrelated local project mutation → stale project route regressions proving no heal publication and that scoped lookup remains enabled.
Verification owner: author for unit plus startup E2E regression; reviewer reruns the mutation proof at the new exact head.
Prior findings
- Scoped lookup: fixed for the passive-snapshot path and covered by the new E2E, but not closed under the local-mutation interleaving above.
- Quota: fixed by classifying
buzz-projects.v1:as pure cache; new oversized, LRU/budget, and startup recovery cases pass. - Stale healing: passive snapshots are fenced, but the mutable-timestamp provenance hole reopens the original publication risk.
Validation
- PASS: full
pnpm --dir desktop test— 5,714/5,714 in both independent lanes. - PASS:
pnpm --dir desktop typecheck. - PASS:
pnpm --dir desktop check(existing warnings/information only). - PASS: required E2E build plus
project-cold-startsmoke spec — 2/2 in both lanes. - PASS:
git diff --checkagainst pinned base. - PASS at submission poll: Unit Tests, both Desktop integration shards and aggregate integration, relay/backend integration, Security, Rust lint, server cross-compiles, release candidate, and DCO. Desktop Core, smoke shards, macOS build, and Windows Rust remained in progress; CI owns those gates and they are not an additional author defect.
- Freshness rechecked immediately before submission: live PR base/head remained the SHAs above; authenticated reviewer
jedwards27is not PR authorthomaspblock.
Manual/native evidence: not run. Native Tauri startup/transition and a numeric cold-start performance cohort remain unobserved reviewer-owned confidence gaps; they do not create additional author action.
Residual risk: the new tests still do not mutation-prove the provenance boundary. No directly changed keyboard/touch/accessibility control was identified.
Separate relay provenance from mutable React Query freshness so local project mutations cannot authorize stale sibling snapshots or suppress scoped startup lookup. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖 Addressed the follow-up provenance finding in
React Query timestamps are no longer used as provenance. Exhaustive-enumeration authority is tracked explicitly per query client, while each project carries non-serialized provenance distinguishing relay reads from local writes. Full and scoped relay results are relay-validated; mutation results mark only the returned project, and untouched snapshot sibling objects retain no authority. Repository-binding cache updates now preserve existing provenance without cloning or upgrading unrelated projects. The startup E2Es now perform a project-cache mutation while broad enumeration remains pending. They verify that the stale project cannot publish healing and that a non-matching snapshot still performs the scoped Validation: full Desktop tests, Desktop check/typecheck, file-size and whitespace gates, focused provenance tests, both mutation-interleaving Playwright scenarios, and all pre-push Desktop lanes pass. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 745ff6066c92372ea1ee6a5fe05862cdd9b81303..554ecaf0ebb6e01471abcbeedbf586d34ebb1130 (fresh re-review of exact head 554ecaf0ebb6e01471abcbeedbf586d34ebb1130; prior reviewed head b81c62090e0832fefffda849a31b69e364e51dcb)
Risk: high — startup authority metadata still controls whether automatic repository-healing publications may run, and the metadata is lost in a common successful-fetch transition.
Behavior/contracts traced: persisted snapshot → React Query hydration/reconciliation → collection and row provenance → scoped lookup → both project-home routes → repository healing; local project mutations; quota/recovery; exact-head unit/E2E and CI gates.
Blocking finding
[High] Successful relay validation is discarded when live project data is structurally equal to the hydrated snapshot. Fresh relay rows receive authority only through a non-enumerable Symbol (desktop/src/features/projects/projectSnapshot.ts:9,28-37; desktop/src/features/projects/projectFetch.ts:46-53). useProjectsQuery retains React Query's default structural sharing (desktop/src/features/projects/hooks.ts:613-626). During the common no-change startup, the relay result is JSON-equal to the persisted snapshot. React Query's replace-equal-deep reconciliation compares enumerable data, retains the old unmarked snapshot object, and discards the new marked object.
Both independent reviewers reproduced the production-shaped transition with the shipped QueryClient: seed an unmarked snapshot at timestamp zero, successfully fetch a deep-equal relay-marked clone, and complete collection validation. The cache retained the snapshot (sameAsSnapshot=true, sameAsLive=false) while row provenance remained absent; one reproduction recorded collectionAuthoritative=true with relayValidated=false.
Both routes derive allowRepositoryHealing from the row marker (desktop/src/app/routes/ChannelRouteScreen.tsx:298-304; desktop/src/features/projects/ui/ProjectDetailScreen.tsx:694-700). An unchanged, fully relay-validated project can therefore remain permanently ineligible to retry automatic repository healing after restart. The new successful-relay unit installs data directly into an empty cache (desktop/src/features/projects/projectSnapshot.test.mjs:110-127) and does not exercise snapshot → query-fetch reconciliation.
Author action: make row provenance survive React Query result reconciliation—for example, disable/customize structural sharing for the project queries or keep authority in state independent of object identity/non-enumerable fields. Add a real QueryClient/render regression: hydrate an equal snapshot → complete successful relay fetch → cached/rendered project is relay-validated and healing-enabled. Mutation-prove it against default symbol-blind structural sharing.
Verification owner: author for deterministic query/render regression; reviewer reruns the exact transition at the next head.
Prior findings
- Local-mutation escalation: causally fixed in the reviewed model. QueryClient-scoped collection state plus row provenance prevents unrelated
setQueryDatawrites from authorizing stale siblings; scoped lookup remains enabled and stale rows cannot heal. - Quota: prior pure-cache budgeting/oversized/LRU/startup recovery fixes remain intact in the reviewed delta.
- The current defect is fail-closed for stale publication, but it breaks the intended recovery behavior indefinitely on the ordinary equal-data startup path; that is author-actionable rather than a confidence gap.
Validation
- PASS: full
pnpm --dir desktop test— 5,714/5,714 in both independent lanes at exact clean head. - PASS: Desktop typecheck and check (existing warnings/information only) in the product lane.
- PASS: E2E TypeScript/Vite build in both lanes.
- PASS:
git diff --checkagainst pinned base. - Local Playwright runtime not executed: pinned Chromium headless-shell revision 1223 was absent. This is reviewer-tooling confidence only, not another author defect.
- PASS at final poll: Unit Tests, Rust lint, Security, macOS build, both Desktop integration shards and aggregate integration, backend/relay E2E, server cross-compiles, release candidate, and DCO. Desktop Core, four smoke shards, and Windows Rust remained in progress; CI owns these gates.
- Freshness rechecked immediately before submission: live base/head remained the SHAs above; authenticated reviewer
jedwards27is not PR authorthomaspblock.
Manual/native evidence: not run. Native Tauri startup transition and a numeric cold-start cohort remain reviewer/release-owned confidence gaps, not additional author actions.
Residual risk: provenance is currently coupled to object identity and an equality algorithm that cannot observe it. No directly changed keyboard/touch/accessibility control was identified.
Summary
Project channels previously appeared as ordinary channels until the community-wide project enumeration completed, delaying the contextual right rail most noticeably on larger relays.
#buzz-channelqueries instead of waiting for the complete project scan.Related issue
None found.
Testing
file-size-check,desktop-check,desktop-typecheck, anddesktop-testNo numeric startup benchmark was captured; this draft validates the cache and scoped-query behavior while leaving timing measurement for review.