fix(scripts): sequence-token race + comment/import cleanup (Codex ffec234 follow-up)
Codex review of ffec234 flagged:
1. **MEDIUM — loadScriptCount in-flight race on workspace switch.**
Previous implementation used `if (get().scriptCountLoading) return`
to dedupe. That meant switching workspaces WHILE a fetch was in
flight dropped the new fetch entirely; the stale response from the
previous workspace then overwrote state, leaving the dashboard
showing workspace A's total while the user is on workspace B.
Fix: drop the dedupe-via-flag, use a module-level
`_scriptCountSeq` counter. Every call increments, captures the
seq at start, and the response/finally block only mutates state
when `_scriptCountSeq === seq` — stale responses are silently
dropped. Rapid workspace switches each get their own fetch; only
the latest response wins.
2. **LOW — scriptCount scope comment was misleading.**
"范围与 list_scripts(parent_path=\"\") 对齐" is wrong: the
count includes all descendant depths, not just root-level.
Behaviour is correct for the dashboard's "全部脚本" intent but
the comment would mislead the next maintainer. Rewritten to
explicitly call out that the count is the UNION across all
parent_path depths, with the rationale for each design choice.
3. **LOW — local `from backend.scripts import _escape_like_pattern`
inside `resources.py` keyword-search block.**
The "avoid cycle" justification was false: scripts.py and
resources.py don't import each other at module level. Moved to
the top-of-file import block.
Skipped:
- get_workspace_tree `like_prefix` not run through the escape helper
(user_id is a 26-char Crockford ULID so no `_`/`%` can appear, but
the invariant is not documented at the call site). Pre-existing
pattern; out of scope for this round.
Verified: pytest 65 passed; pnpm typecheck clean.
This commit is contained in:
@@ -40,6 +40,7 @@ let _api: WorkspaceBoundApi | null = null;
|
||||
let _previewController: AbortController | null = null;
|
||||
let _previewRequest = 0;
|
||||
let _pythonEditorOpeningIds = new Set<string>();
|
||||
let _scriptCountSeq = 0;
|
||||
|
||||
export const bindScriptWorkspaceApi = (api: WorkspaceBoundApi | null) => {
|
||||
_api = api;
|
||||
@@ -397,17 +398,25 @@ export const useScriptWorkspaceStore = create<State>((set, get) => {
|
||||
|
||||
loadScriptCount: async () => {
|
||||
const api = requireApi();
|
||||
if (get().scriptCountLoading) return;
|
||||
// Always fire — don't dedupe via the loading flag. Rapid workspace
|
||||
// switches would otherwise drop the new fetch and leave the
|
||||
// dashboard showing the previous workspace's count. The sequence
|
||||
// counter below discards stale responses instead.
|
||||
const seq = ++_scriptCountSeq;
|
||||
set({ scriptCountLoading: true, scriptCount: null });
|
||||
try {
|
||||
const total = await api.countScripts();
|
||||
if (_scriptCountSeq !== seq) return; // a newer fetch superseded us
|
||||
set({ scriptCount: total });
|
||||
} catch {
|
||||
if (_scriptCountSeq !== seq) return;
|
||||
// Leave previous value in place; the dashboard already tolerates
|
||||
// a stale count by rendering `scriptCount ?? 0`. Don't toast —
|
||||
// the dashboard's other metrics are best-effort.
|
||||
} finally {
|
||||
set({ scriptCountLoading: false });
|
||||
if (_scriptCountSeq === seq) {
|
||||
set({ scriptCountLoading: false });
|
||||
}
|
||||
}
|
||||
},
|
||||
|
||||
|
||||
Reference in New Issue
Block a user