From 96aaa7bb64a0d9bbf8694a5514e9ff5af889565b Mon Sep 17 00:00:00 2001 From: "tao.chen" <93983997+taochen-ct@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:42:38 +0800 Subject: [PATCH] fix(scripts): AuthContext binding + load() cache invalidation (Codex review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex (deepseek-v4-flash) review of feat(scripts) parent_path filter surfaced four problems: 1. Critical — AuthContext listScripts binding silently dropped parentPath - frontend/app/context/AuthContext.tsx:224 had: listScripts: () => rawApi.listScripts(workspaceId) so loadScripts("foo/bar") hit GET /api/v1/scripts with no query and always received root-level scripts. Typecheck passed because `() => ...` is assignable to `(parentPath?: string) => ...`. - Fix: forward the parentPath argument. 2. High — load() cache invalidation gap on toolbar refresh / create / delete - load() replaced `scripts` with root-only items but never invalidated `loadedScriptPaths` for subfolders, so previously-expanded folders rendered empty (cached no-op on re-expand) and open tabs pointing into subfolders were dropped by the validIds filter. - Fix: load() now re-fetches every path currently in loadedScriptPaths (and loadedChildPaths for directories), then dedups by id/path. On initial mount the cache is empty so this degrades to a single root fetch. 3. Low — test docstring overclaimed coverage - The "actual ORM roundtrip is covered by the existing integration tests" line is false (no other list_scripts test exists). - Fix: honest docstring noting the repo has no endpoint integration test layer; SQL assertions are brittle to SQLAlchemy/dialect formatting. 4. Low — backend docstring overclaimed index role - Claimed `idx_storage_workspace_relative_path` "avoided全表扫", but the index isn't declared in the ORM model, only exists via the baseline migration's upgrade path, and EXPLAIN doesn't drive through it (scripts-first plan via idx_scripts_workspace). - Fix: accurate description — MySQL drives via idx_scripts_workspace, storage_objects PK lookup applies LIKE per row. Notes where to optimize if 10万-scale perf becomes a real problem. Skipped findings (out of scope): - Medium LIKE escape for `_` / `%` (pre-existing in list_workspace_directories; not introduced here). - Low dashboard `scripts.length` count underreport (separate UI bug, pre-existing assumption that broke under the new semantics). Verified: pytest 50 passed, pnpm typecheck clean. --- backend/src/backend/scripts.py | 10 ++-- .../tests/test_list_scripts_parent_path.py | 8 +-- frontend/app/context/AuthContext.tsx | 2 +- .../platform/state/scriptWorkspaceStore.ts | 52 +++++++++++++++---- 4 files changed, 54 insertions(+), 18 deletions(-) diff --git a/backend/src/backend/scripts.py b/backend/src/backend/scripts.py index 187bf14..6ea82d0 100644 --- a/backend/src/backend/scripts.py +++ b/backend/src/backend/scripts.py @@ -1073,9 +1073,13 @@ async def delete_workspace_directory( # 的关键端点,避免 10 万级脚本一次性返回。 # # 实现走 ``storage_objects.relative_path`` materialized path(``LIKE prefix%`` -# + ``NOT LIKE prefix%/%`` 取直接子节点),配合现有 -# ``idx_storage_workspace_relative_path (workspace_id, relative_path)`` 索引 -# 避免全表扫。 +# + ``NOT LIKE prefix%/%`` 取直接子节点)。MySQL 优化器目前选 +# ``scripts`` 驱动 ``idx_scripts_workspace``、再 PK lookup 回 +# ``storage_objects`` 应用 LIKE 过滤 —— 对 ``scripts`` 端高度选择性 +# (workspace + status) 的工作区来说已经够好;真要压平万级脚本可考虑 +# ``STRAIGHT_JOIN`` 或给 ``storage_objects.relative_path`` 加 prefix +# 索引(基线迁移里有 ``idx_storage_workspace_relative_path`` 但 ORM +# 模型未声明,不在此修复范围)。 @router.get("/api/v1/scripts") async def list_scripts( parent_path: str = Query(default="", max_length=1024), diff --git a/backend/tests/test_list_scripts_parent_path.py b/backend/tests/test_list_scripts_parent_path.py index aa6fea8..77d7ac5 100644 --- a/backend/tests/test_list_scripts_parent_path.py +++ b/backend/tests/test_list_scripts_parent_path.py @@ -2,9 +2,11 @@ Verifies the WHERE clause built by the endpoint encodes the intended "direct children of parent_path" semantics: ``relative_path LIKE 'prefix/%'`` -and ``NOT LIKE 'prefix/%/%'``. The actual ORM roundtrip is covered by the -existing integration tests; here we only care that the filter contract is -intact, so an AsyncMock session is enough. +and ``NOT LIKE 'prefix/%/%'``. These are SQL-contract assertions (mock +session, capture compiled SQL); the repo has no integration test layer +for endpoints, so this is the only coverage. Brittle to SQLAlchemy/dialect +rendering changes — review the assertions together with the endpoint if +you upgrade SQLAlchemy. Mirrors the pattern in test_scripts.py (unit-level, no live DB). """ diff --git a/frontend/app/context/AuthContext.tsx b/frontend/app/context/AuthContext.tsx index 3b01b75..08611d9 100644 --- a/frontend/app/context/AuthContext.tsx +++ b/frontend/app/context/AuthContext.tsx @@ -221,7 +221,7 @@ export function useApi(): WorkspaceBoundApi { const workspaceId = currentWorkspace?.workspace_id ?? ""; return useMemo(() => ({ - listScripts: () => rawApi.listScripts(workspaceId), + listScripts: (parentPath) => rawApi.listScripts(workspaceId, parentPath), listResources: (opts) => rawApi.listResources(workspaceId, opts), createScript: (input) => rawApi.createScript(workspaceId, input), uploadScript: (file, parentPath, visibility) => diff --git a/frontend/app/features/platform/state/scriptWorkspaceStore.ts b/frontend/app/features/platform/state/scriptWorkspaceStore.ts index ea6a59e..fa530fe 100644 --- a/frontend/app/features/platform/state/scriptWorkspaceStore.ts +++ b/frontend/app/features/platform/state/scriptWorkspaceStore.ts @@ -263,25 +263,55 @@ export const useScriptWorkspaceStore = create((set, get) => { if (!silent) set({ loading: true }); set({ refreshing: silent }); try { - // Fetch root-level scripts (parent_path="") and root directories in - // parallel. Deeper folders are lazy-fetched on expand via loadScripts - // / loadChildren. - const [items, folderItems] = await Promise.all([ - api.listScripts(""), + // Re-fetch every path currently in the cache. On initial mount the + // cache is empty so this degrades to a single root fetch; on + // toolbar refresh / after createFolder / after deleteScript the + // previously-expanded folders are also re-fetched so the UI stays + // consistent (otherwise the cache would keep stale "loaded" + // markers while the corresponding scripts had been replaced by + // the root-only payload, leaving subfolders empty on re-expand). + const cachedScriptPaths = Array.from(get().loadedScriptPaths); + const cachedChildPaths = Array.from(get().loadedChildPaths).filter( + (p) => p !== "", + ); + const scriptFetches = + cachedScriptPaths.length > 0 + ? cachedScriptPaths.map((p) => + api.listScripts(p).catch(() => [] as ScriptItem[]), + ) + : [api.listScripts("")]; + const dirFetches = [ api.listWorkspaceDirectories(""), + ...cachedChildPaths.map((p) => + api.listWorkspaceDirectories(p).catch(() => [] as WorkspaceDirectory[]), + ), + ]; + const [scriptLists, dirLists] = await Promise.all([ + Promise.all(scriptFetches), + Promise.all(dirFetches), ]); + const freshScripts = scriptLists.flat(); + const freshDirs = dirLists.flat(); + // Dedup: later occurrences win so fresh per-path payloads override + // any duplicates coming through different fetch slots. + const dedupedScripts = Array.from( + new Map(freshScripts.map((s) => [s.script_id, s])).values(), + ); + const dedupedDirs = Array.from( + new Map(freshDirs.map((d) => [d.path, d])).values(), + ); + const nextLoadedScripts = new Set(cachedScriptPaths); + nextLoadedScripts.add(""); const nextLoadedChildren = new Set(get().loadedChildPaths); nextLoadedChildren.add(""); - const nextLoadedScripts = new Set(get().loadedScriptPaths); - nextLoadedScripts.add(""); set({ - scripts: items, - directories: folderItems, + scripts: dedupedScripts, + directories: dedupedDirs, apiOnline: true, - loadedChildPaths: nextLoadedChildren, loadedScriptPaths: nextLoadedScripts, + loadedChildPaths: nextLoadedChildren, }); - const validIds = new Set(items.map((item) => item.script_id)); + const validIds = new Set(dedupedScripts.map((item) => item.script_id)); const currentSelected = get().selectedId; if (!currentSelected || !validIds.has(currentSelected)) { _selectedId = null;