fix(api,editor): per-request content-access checker + parallel entry listing; navigator loading state#78
Merged
Conversation
…g; navigator loading state GET /:branch/entries took ~45s per editor load on a branch with ~31 collections. Two compounding causes: the handler iterated collections serially, and access control re-loaded permissions (and re-resolved the settings-branch root) once per entry — both constant per request. - authorization: add createContentAccessChecker, a per-request batch primitive that resolves branch access, the settings root, and the permission rules ONCE and returns a synchronous per-path checker. checkContentAccess now delegates to it (public API unchanged). Chosen over a process-global cache to avoid serving stale ACLs after a permissions edit and to mirror the prod Lambda model. - api/entries: build one checker per request, make filterWithAccessControl synchronous, and parallelize the per-collection loop with Promise.all (mirrors the shared listEntries). Pagination stays filter-then-slice. - api/resolve-references, api/reference-options: same anti-pattern removed (build the checker once before the loop). - editor: thread entries-loading into EntryNavigator (loading prop) and the preview/form empty state so "No content" / "Select an item" no longer show while content is still loading. - tests: createTestContentAccess helper; once-per-request and pagination-with-filter coverage; EntryNavigator loading-state tests. - docs: note the batch checker in ARCHITECTURE.md and CODEBASE_GUIDE.md.
…or surfacing Address branch-review finding 3 (test gap) for the two reference endpoints whose behavior this branch deliberately changed: - reference-options: assert createContentAccessChecker is built once per request even with multiple options (guards against reintroducing per-item permission loads). - resolve-references: assert a settings-root build failure now surfaces (rejects) instead of being swallowed into a 200 with an empty resolved map.
…search filters Branch-review follow-up. In filterWithAccessControl the synchronous edit-access check now runs after the read and search filters instead of before, so it is skipped for entries that are filtered out. Behavior-preserving. Also slim the authorization file table in CODEBASE_GUIDE.md: the long createContentAccessChecker description moves from a wide table cell into the existing "Batch primitive" prose note (no information lost).
…ng…" not a flash The empty editor pane and navigator keyed "Loading content…" off the shared entriesLoading busy flag, which starts false and only flips true inside a post-mount effect — so initial load briefly flashed "Select an item to start editing." first. Add a dedicated entriesInitializing flag to useEntryManager, seeded from the branch prop so it is already true on the first render (no flash) and cleared when the first per-branch load settles (so a genuinely empty branch falls back to the normal empty state instead of a stuck loader). Editor's empty-pane copy and the navigator loading prop key off this flag. Covered by useEntryManager tests. Also document the deliberate unbounded read-concurrency trade-off in content listing (BACKLOG #19 + an inline note in api/entries.ts) so it isn't reinvestigated.
… helper
git-manager.test.ts inspected bare repos with simpleGit({ baseDir }), which fails under safe.bareRepository=explicit (injected by sandboxed/CI git environments) — git refuses cwd-based discovery of bare repos. Add an openBareRepo() test helper that sets safe.bareRepository=all for its own invocations (safe: the path is one
the test created) and route the bare-repo inspection sites through it. Product code already handles hostile-env git via explicit --git-dir; no product change.
Files: test-utils/git-helpers.ts, test-utils/index.ts, git-manager.test.ts
Retype MockServicesOptions from a bag of `any` fields to
`Partial<Omit<CanopyServices, 'config'>> & { config?: Partial<CanopyConfig> }`, so
wrong-shape overrides are caught at compile time (vitest mocks stay assignable). The
intentionally-partial mock GitManager default keeps one narrow cast; make
refreshActiveBranch overridable for consistency.
Migrate the hand-rolled 14-field services objects in api/content.test.ts (4 sites)
and api/assets.test.ts (makeCtx) onto createMockApiContext + allowContentAccess, so
new service fields no longer need to be added to each test. Behavior preserved
(handlers read branchContext.flatSchema / config.contentRoot, never config.schema).
debshila
approved these changes
Jun 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two user-facing improvements to the editor's entry list, plus supporting test/infra cleanup.
1. Performance — per-request batch content-access checker
The entry-list endpoint re-loaded path permissions and re-resolved the settings-branch root once per entry (every
checkContentAccesscall rangetSettingsBranchRoot+ a permission load). On a branch with many collections this took tens of seconds.createContentAccessChecker(deps, context, branchRoot, user)(inauthorization/content.ts) does the request-constant work once — branch access, settings-root resolution, permission-rule load — and returns a synchronous(relativePath, level) => ContentAccessResultchecker.checkContentAccessnow delegates to it (signature/behavior unchanged).CanopyServices; adopted by the three looping endpoints:api/entries.ts,api/reference-options.ts,api/resolve-references.ts.entries.tsalso lists collections in parallel (Promise.all, matching the recursive path andcontent-listing.ts); array ordering and filter-before-slice pagination are preserved.2. UX — navigator / empty-pane loading state
EntryNavigatorgains aloading?prop; a newentriesInitializingflag inuseEntryManageris seeded from the branch prop (true on the first render) so the empty pane shows "Loading content…" instead of flashing "No content" / "Select an item…". It clears when the first per-branch load settles, so a genuinely empty branch falls back to the normal empty state (no stuck loader)..finallyis seq-guarded (branchLoadSeqRef) so a superseded load from a rapid branch switch can't clear the flag early.3. Test & infra cleanup
resolve-referencesnow surfaces a settings-workspace failure instead of swallowing it into a200 {}(previously indistinguishable from "no references"). Covered by a new test.openBareRepo()test helper setssafe.bareRepository=allsogit-manager.test.tscan inspect bare simulated-remotes in environments that injectsafe.bareRepository=explicit(sandboxed/CI git). Product code already handles this via explicit--git-dir.MockServicesOptionsretyped from a bag ofanytoPartial<Omit<CanopyServices, 'config'>>; the hand-rolled 14-field mock-service objects incontent.test.ts(4 sites) andassets.test.tsmigrated ontocreateMockApiContext.Follow-up (documented, not in this PR)
Promise.allFS fan-out (consistent across all listing sites). Intentional; a bounded-concurrency helper is deferred unless prod EFS throttling appears.Testing
checkContentAccessparity, theentriesInitializinglifecycle (including the superseded-load race), navigator loading states, and the resolve-references error-surfacing.Review
Reviewed across two independent sub-reviews; no Critical/High issues. The one behavioral note — the checker now builds eagerly, so an empty list/refs request does one extra (per-request-cached)
getSettingsBranchRoot— is a consciously-accepted trade-off.