mirror of
https://github.com/stablyai/orca.git
synced 2026-10-26 00:03:55 +00:00
JinjingandOrca
b501a6faa0
Replace the setState-driven data flow with useSyncExternalStore so the drawer reads cached work-item details synchronously on first render. Warm reopens now paint the cached content immediately with zero blank flash. Adds a pub/sub layer (subscribeWorkItemDetailsCache / notifyWorkItemDetailsCache) to all cache-write paths so React is notified on every touch or invalidation. Includes design doc at docs/gh-work-item-drawer-cache-flash.md. Co-authored-by: Orca <help@stably.ai>
133 lines
4.9 KiB
Markdown
133 lines
4.9 KiB
Markdown
# GitHub Work Item Drawer: Fix Reopen Flash
|
|
|
|
## Symptom
|
|
|
|
After #1655, reopening a GH issue/PR drawer can still flash a loading/skeleton frame before cached content appears.
|
|
|
|
## Verified root cause
|
|
|
|
`GitHubItemDialog` currently paints from component state (`details`, `loading`, `error`) that is updated in an effect (`src/renderer/src/components/GitHubItemDialog.tsx:2480+`).
|
|
|
|
Current sequence on close/reopen of the same item:
|
|
|
|
1. Close path runs effect branch with no `workItem` and calls `setDetails(null)`.
|
|
2. Reopen renders once with `details === null`.
|
|
3. Effect reads `workItemDetailsCache` and then calls `setDetails(painted)`.
|
|
4. Second render shows cached content.
|
|
|
|
With sheet animation, that first empty paint is visible as a flash.
|
|
|
|
## Refactor direction
|
|
|
|
Move drawer paint state to the module cache and subscribe with `useSyncExternalStore`. The drawer should read the cache entry synchronously during render.
|
|
|
|
This is not optional if we want reopen to paint cached content on first render.
|
|
|
|
## Required cache-store changes
|
|
|
|
Add a store subscription layer around `workItemDetailsCache`:
|
|
|
|
- `subscribeWorkItemDetailsCache(listener)`
|
|
- `notifyWorkItemDetailsCache()`
|
|
|
|
Call `notifyWorkItemDetailsCache()` after every actual cache mutation:
|
|
|
|
- `touchWorkItemDetailsCache`
|
|
- `invalidateWorkItemDetailsCacheForKey`
|
|
- `invalidateWorkItemDetailsCacheByMatch`
|
|
|
|
Also increment `workItemDetailsCacheGeneration` on **all** invalidations, not only `invalidateWorkItemDetailsCacheByMatch`. Otherwise an in-flight fetch launched before exact-key invalidation can repopulate stale data.
|
|
|
|
## `useSyncExternalStore` contract details
|
|
|
|
Use:
|
|
|
|
```ts
|
|
const cachedEntry = useSyncExternalStore(
|
|
subscribeWorkItemDetailsCache,
|
|
() => (detailsCacheKey ? workItemDetailsCache.get(detailsCacheKey) : undefined)
|
|
)
|
|
```
|
|
|
|
Important constraints:
|
|
|
|
- `getSnapshot` must return a referentially stable value when store data has not changed.
|
|
- Returning the map entry object is valid **only if** writes replace entry identity (`delete+set` with a new entry object).
|
|
- Do not build fresh wrapper objects in `getSnapshot` (that would violate stability and can cause render loops).
|
|
|
|
## Component state changes
|
|
|
|
Remove component-local `details/loading/error` state for work-item details.
|
|
|
|
Derive view state from `cachedEntry`:
|
|
|
|
- `details = cachedEntry?.details ?? null`
|
|
- blocking `loading = !!cachedEntry?.pending && !cachedEntry?.details`
|
|
- blocking `error = cachedEntry?.error && !cachedEntry?.details ? cachedEntry.error : null`
|
|
|
|
Keep stale-on-error behavior by preserving `details` in cache while setting `error`.
|
|
|
|
## Effect responsibilities after refactor
|
|
|
|
The main effect should:
|
|
|
|
- handle item-change bookkeeping (`prevItemIdRef`, `optimisticCommentsRef`, `setTab`)
|
|
- decide freshness and in-flight dedupe
|
|
- launch fetch and write results/errors/pending back into cache
|
|
|
|
The effect should not call `setDetails`, `setLoading`, or `setError` for work-item details.
|
|
|
|
## Optimistic comment race and merge rules
|
|
|
|
`appendOptimisticComment` must write through cache only (no local `setDetails`).
|
|
|
|
Rules:
|
|
|
|
- push comment into `optimisticCommentsRef`
|
|
- patch cache entry comments (if present), mark stale (`fetchedAt: 0`), notify
|
|
- derive rendered comments from cached details plus optimistic refs
|
|
|
|
Dependency pitfall:
|
|
|
|
- do not depend on a freshly-created array from `optimisticCommentsRef.current` in `useMemo`
|
|
- memo should key off stable signals (cache entry identity); cache notifications will re-render after optimistic writes anyway
|
|
|
|
This avoids unnecessary recomputation and avoids accidental infinite rerender patterns from unstable deps.
|
|
|
|
## Invalidation while drawer is open
|
|
|
|
When `invalidateWorkItemDetailsCacheByMatch` deletes the current key while drawer is mounted:
|
|
|
|
- subscription notifies immediately
|
|
- render sees `cachedEntry === undefined`
|
|
- UI falls back to loading/empty state and effect triggers refetch
|
|
|
|
This is expected and preferred over showing stale state.
|
|
|
|
## Fetch race constraints (must keep)
|
|
|
|
Keep current protections:
|
|
|
|
- per-key in-flight dedupe via `entry.pending`
|
|
- generation guard to block stale writes after invalidation
|
|
- stale-on-null handling (`result === null` must not wipe existing valid cached details)
|
|
|
|
Add one missing consistency rule:
|
|
|
|
- if fetch resolves after invalidation and key was dropped, do not recreate entry unless generation still matches launch generation.
|
|
|
|
## Scope of deletion
|
|
|
|
Delete only the work-item-detail local state and call sites:
|
|
|
|
- `useState<GitHubWorkItemDetails | null>` (`details`)
|
|
- `useState<boolean>` (`loading`) for this data path
|
|
- `useState<string | null>` (`error`) for this data path
|
|
- corresponding `setDetails`/`setLoading`/`setError` calls in the details effect and optimistic comment append path
|
|
|
|
Do not touch unrelated loading/error state in other subviews.
|
|
|
|
## Expected result
|
|
|
|
Reopen of a cached item paints content on first render (no flash). Background refetch still runs when stale, and cross-window/local invalidations remain authoritative.
|