fix(scripts): AuthContext binding + load() cache invalidation (Codex review)
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.
This commit is contained in:
@@ -1073,9 +1073,13 @@ async def delete_workspace_directory(
|
|||||||
# 的关键端点,避免 10 万级脚本一次性返回。
|
# 的关键端点,避免 10 万级脚本一次性返回。
|
||||||
#
|
#
|
||||||
# 实现走 ``storage_objects.relative_path`` materialized path(``LIKE prefix%``
|
# 实现走 ``storage_objects.relative_path`` materialized path(``LIKE prefix%``
|
||||||
# + ``NOT LIKE prefix%/%`` 取直接子节点),配合现有
|
# + ``NOT LIKE prefix%/%`` 取直接子节点)。MySQL 优化器目前选
|
||||||
# ``idx_storage_workspace_relative_path (workspace_id, relative_path)`` 索引
|
# ``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")
|
@router.get("/api/v1/scripts")
|
||||||
async def list_scripts(
|
async def list_scripts(
|
||||||
parent_path: str = Query(default="", max_length=1024),
|
parent_path: str = Query(default="", max_length=1024),
|
||||||
|
|||||||
@@ -2,9 +2,11 @@
|
|||||||
|
|
||||||
Verifies the WHERE clause built by the endpoint encodes the intended
|
Verifies the WHERE clause built by the endpoint encodes the intended
|
||||||
"direct children of parent_path" semantics: ``relative_path LIKE 'prefix/%'``
|
"direct children of parent_path" semantics: ``relative_path LIKE 'prefix/%'``
|
||||||
and ``NOT LIKE 'prefix/%/%'``. The actual ORM roundtrip is covered by the
|
and ``NOT LIKE 'prefix/%/%'``. These are SQL-contract assertions (mock
|
||||||
existing integration tests; here we only care that the filter contract is
|
session, capture compiled SQL); the repo has no integration test layer
|
||||||
intact, so an AsyncMock session is enough.
|
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).
|
Mirrors the pattern in test_scripts.py (unit-level, no live DB).
|
||||||
"""
|
"""
|
||||||
|
|||||||
@@ -221,7 +221,7 @@ export function useApi(): WorkspaceBoundApi {
|
|||||||
const workspaceId = currentWorkspace?.workspace_id ?? "";
|
const workspaceId = currentWorkspace?.workspace_id ?? "";
|
||||||
|
|
||||||
return useMemo<WorkspaceBoundApi>(() => ({
|
return useMemo<WorkspaceBoundApi>(() => ({
|
||||||
listScripts: () => rawApi.listScripts(workspaceId),
|
listScripts: (parentPath) => rawApi.listScripts(workspaceId, parentPath),
|
||||||
listResources: (opts) => rawApi.listResources(workspaceId, opts),
|
listResources: (opts) => rawApi.listResources(workspaceId, opts),
|
||||||
createScript: (input) => rawApi.createScript(workspaceId, input),
|
createScript: (input) => rawApi.createScript(workspaceId, input),
|
||||||
uploadScript: (file, parentPath, visibility) =>
|
uploadScript: (file, parentPath, visibility) =>
|
||||||
|
|||||||
@@ -263,25 +263,55 @@ export const useScriptWorkspaceStore = create<State>((set, get) => {
|
|||||||
if (!silent) set({ loading: true });
|
if (!silent) set({ loading: true });
|
||||||
set({ refreshing: silent });
|
set({ refreshing: silent });
|
||||||
try {
|
try {
|
||||||
// Fetch root-level scripts (parent_path="") and root directories in
|
// Re-fetch every path currently in the cache. On initial mount the
|
||||||
// parallel. Deeper folders are lazy-fetched on expand via loadScripts
|
// cache is empty so this degrades to a single root fetch; on
|
||||||
// / loadChildren.
|
// toolbar refresh / after createFolder / after deleteScript the
|
||||||
const [items, folderItems] = await Promise.all([
|
// previously-expanded folders are also re-fetched so the UI stays
|
||||||
api.listScripts(""),
|
// 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(""),
|
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);
|
const nextLoadedChildren = new Set(get().loadedChildPaths);
|
||||||
nextLoadedChildren.add("");
|
nextLoadedChildren.add("");
|
||||||
const nextLoadedScripts = new Set(get().loadedScriptPaths);
|
|
||||||
nextLoadedScripts.add("");
|
|
||||||
set({
|
set({
|
||||||
scripts: items,
|
scripts: dedupedScripts,
|
||||||
directories: folderItems,
|
directories: dedupedDirs,
|
||||||
apiOnline: true,
|
apiOnline: true,
|
||||||
loadedChildPaths: nextLoadedChildren,
|
|
||||||
loadedScriptPaths: nextLoadedScripts,
|
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;
|
const currentSelected = get().selectedId;
|
||||||
if (!currentSelected || !validIds.has(currentSelected)) {
|
if (!currentSelected || !validIds.has(currentSelected)) {
|
||||||
_selectedId = null;
|
_selectedId = null;
|
||||||
|
|||||||
Reference in New Issue
Block a user