Implement two-phase project loading#683
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a two-phase project loading model: a lightweight project list (for the sidebar) loaded quickly (cached in Redis) plus on-demand fetching of full project details when navigating to a project, removing the previous blocking server-side project fetch from the authenticated layout.
Changes:
- Added
/api/projects(summaries) and/api/projects/[owner]/[repo](details) routes with auth checks, plus new GitHub data sources to back them. - Replaced the old single “projects context” with separate client contexts for project list and project details.
- Updated sidebar and project navigation flows to work with summaries first, then details, including new/updated Jest tests for the new data sources.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/features/sidebar/view/internal/sidebar/projects/ProjectListItem.tsx | Switches sidebar list items to use ProjectSummary and selection-by-owner/name. |
| src/features/sidebar/view/internal/sidebar/projects/ProjectListFallback.tsx | Uses ProjectListContext instead of the removed ProjectsContext. |
| src/features/sidebar/view/internal/sidebar/projects/ProjectList.tsx | Renders based on list loading state from ProjectListContext. |
| src/features/sidebar/view/internal/sidebar/projects/ProjectAvatar.tsx | Updates avatar prop type to ProjectSummary. |
| src/features/sidebar/view/internal/sidebar/projects/PopulatedProjectList.tsx | Updates list prop type to ProjectSummary[]. |
| src/features/sidebar/view/internal/sidebar/Header.tsx | Removes refresh UI tied to the old projects refresh mechanism. |
| src/features/projects/view/index.ts | Exports new list/details contexts and providers. |
| src/features/projects/view/ProjectsContextProvider.tsx | Removes the old monolithic projects provider. |
| src/features/projects/view/ProjectListContextProvider.tsx | Adds client provider for loading + refreshing project summaries via /api/projects. |
| src/features/projects/view/ProjectListContext.tsx | Adds ProjectListContext + useProjectList hook. |
| src/features/projects/view/ProjectDetailsContextProvider.tsx | Adds client provider for per-project details caching + de-duped fetches. |
| src/features/projects/view/ProjectDetailsContext.tsx | Adds ProjectDetailsContext + useProjectDetails hook. |
| src/features/projects/domain/index.ts | Updates domain exports for new list/details split. |
| src/features/projects/domain/ProjectSummary.ts | Introduces ProjectSummary model + Zod schema. |
| src/features/projects/domain/ProjectListRepository.ts | Implements Redis-backed user cache for project summaries. |
| src/features/projects/domain/IProjectRepository.ts | Removes old full-project repository interface. |
| src/features/projects/domain/IProjectListRepository.ts | Adds repository interface for project summaries. |
| src/features/projects/domain/IProjectListDataSource.ts | Adds data source interface for project summaries. |
| src/features/projects/domain/IProjectDetailsDataSource.ts | Adds data source interface for project details. |
| src/features/projects/domain/IProjectDataSource.ts | Removes old full-project data source interface. |
| src/features/projects/domain/IGitHubRepositoryDataSource.ts | Removes old repository-level GitHub abstraction. |
| src/features/projects/domain/FilteringGitHubRepositoryDataSource.ts | Removes old “filter hidden repos” wrapper in favor of list data source logic. |
| src/features/projects/domain/CachingProjectListDataSource.ts | Adds caching wrapper for project list fetches. |
| src/features/projects/domain/CachingProjectDataSource.ts | Removes old caching wrapper for full projects. |
| src/features/projects/data/useProjectSelection.ts | Updates selection/navigation to work without preloaded full project data. |
| src/features/projects/data/index.ts | Updates exports to new GitHub list/details data sources. |
| src/features/projects/data/GitHubRepositoryDataSource.ts | Removes old GitHub repository data source. |
| src/features/projects/data/GitHubProjectListDataSource.ts | Adds GitHub-backed list fetch producing ProjectSummary[]. |
| src/features/projects/data/GitHubProjectDetailsDataSource.ts | Adds GitHub-backed details fetch producing full Project. |
| src/features/projects/data/GitHubProjectDataSource.ts | Removes old “fetch everything” project data source. |
| src/composition.ts | Wires up new list/details data sources and repositories. |
| src/common/context/ProjectsContext.ts | Removes ProjectsContext value (keeps only SidebarTogglableContext). |
| src/app/api/refresh-projects/route.ts | Removes old refresh endpoint tied to full-project refresh. |
| src/app/api/projects/route.ts | Adds summaries endpoint with optional refresh=true. |
| src/app/api/projects/[owner]/[repo]/route.ts | Adds project-details endpoint for on-demand fetch. |
| src/app/(authed)/layout.tsx | Removes blocking server-side project fetch; installs list/details providers. |
| src/app/(authed)/(project-doc)/[...slug]/page.tsx | Fetches project details on navigation; uses new details context. |
| jest.config.js | Adds tsx to moduleFileExtensions. |
| test/projects/GitHubRepositoryDataSource.test.ts | Removes tests for deleted repo data source. |
| test/projects/GitHubProjectListDataSource.test.ts | Adds tests for the new list data source. |
| test/projects/GitHubProjectDetailsDataSource.test.ts | Adds tests for the new details data source. |
| test/projects/GitHubProjectDataSource.test.ts | Removes tests for deleted “fetch everything” data source. |
| test/projects/FilteringGitHubRepositoryDataSource.test.ts | Removes tests for deleted filtering wrapper. |
| test/projects/CachingProjectListDataSource.test.ts | Adds tests for new list caching wrapper. |
| test/projects/CachingProjectDataSource.test.ts | Removes tests for deleted full-project caching wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3f52bc6 to
ff6caac
Compare
e11e2ec to
dd29d56
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Splits project loading into two phases to eliminate the blocking server-side fetch that was gating the entire authenticated layout: Phase 1 loads a lightweight project list (name, owner, image) via a cached /api/projects route, populating the sidebar immediately. Phase 2 fetches full project details (versions, specs, PRs) on demand via /api/projects/[owner]/[repo] when navigating to a specific project. - Replace GitHubProjectDataSource/GitHubRepositoryDataSource with GitHubProjectListDataSource and GitHubProjectDetailsDataSource - Add ProjectSummary type for the lightweight list representation - Add CachingProjectListDataSource backed by Redis for the project list - Replace ProjectsContextProvider with ProjectListContextProvider and ProjectDetailsContextProvider, each fetching from the new API routes - Remove blocking server-side project fetch from the authed layout - Update useProjectSelection to resolve project from the details cache - Add auth checks to both new API routes - Fire background refresh after cached list loads so sidebar is instant - Fetch defaultBranchRef OID in list query for consistent image URLs - Add tests for GitHubProjectListDataSource, GitHubProjectDetailsDataSource, and CachingProjectListDataSource
dd29d56 to
239d88b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (defaultVersion) { | ||
| const defaultSpec = getDefaultSpecification(defaultVersion) | ||
| if (defaultSpec) { | ||
| router.replace(`/${currentProject.owner}/${currentProject.name}/${encodeURIComponent(defaultVersion.id)}/${encodeURIComponent(defaultSpec.id)}`) | ||
| return |
There was a problem hiding this comment.
defaultVersion.id can contain slashes (e.g. feature branches). Using encodeURIComponent(defaultVersion.id) will encode / as %2F, producing a URL that won’t match the app’s path semantics (ProjectNavigator encodes version IDs segment-by-segment). Consider reusing ProjectNavigator’s encoding logic here (e.g. call projectNavigator.navigate(...) / encodePath equivalent) so version IDs with / round-trip correctly.
| } | ||
|
|
||
| private makeImageURL(owner: string, repo: string, imagePath: string, oid?: string): string { | ||
| return `/api/blob/${owner}/${repo}/${encodeURIComponent(imagePath)}?ref=${oid ?? "HEAD"}` |
There was a problem hiding this comment.
encodeURIComponent(imagePath) will also encode / as %2F. Since the blob route uses a catch-all [...path] and expects path segments separated by /, this will break images stored in subdirectories (e.g. images/logo.png). Encode each path segment instead (split on /, encode segments, then join with /).
| return `/api/blob/${owner}/${repo}/${encodeURIComponent(imagePath)}?ref=${oid ?? "HEAD"}` | |
| const encodedImagePath = imagePath | |
| .split("/") | |
| .map(segment => encodeURIComponent(segment)) | |
| .join("/") | |
| return `/api/blob/${owner}/${repo}/${encodedImagePath}?ref=${oid ?? "HEAD"}` |
|
|
||
| let imageURL: string | undefined | ||
| if (config?.image) { | ||
| imageURL = `/api/blob/${data.owner}/${data.name}/${encodeURIComponent(config.image)}?ref=${data.defaultBranchRef.target.oid}` |
There was a problem hiding this comment.
encodeURIComponent(config.image) will encode / as %2F, which will break blob URLs for images stored in subdirectories given the blob route’s [...path] param handling. Encode per path segment (split on /, encode, re-join) to preserve directory separators.
| imageURL = `/api/blob/${data.owner}/${data.name}/${encodeURIComponent(config.image)}?ref=${data.defaultBranchRef.target.oid}` | |
| const encodedImagePath = config.image.split("/").map(segment => encodeURIComponent(segment)).join("/") | |
| imageURL = `/api/blob/${data.owner}/${data.name}/${encodedImagePath}?ref=${data.defaultBranchRef.target.oid}` |
| name: file.name, | ||
| url: `/api/blob/${owner}/${repoName}/${encodeURIComponent(file.name)}?ref=${ref.target.oid}`, | ||
| editURL: `https://github.com/${owner}/${repoName}/edit/${ref.name}/${encodeURIComponent(file.name)}`, | ||
| diffURL: isChanged ? `/api/diff/${owner}/${repoName}/${encodeURIComponent(file.name)}?baseRefOid=${pr!.baseRefOid}&to=${ref.target.oid}` : undefined, | ||
| diffBaseBranch: isChanged ? pr!.baseRefName : undefined, |
There was a problem hiding this comment.
Specification/blob/diff URL construction uses encodeURIComponent(file.name), which will encode / as %2F. If specs ever live in subdirectories (or if filenames include /), the blob/diff routes (which use catch-all segments) will receive a single segment and request the wrong GitHub path. Prefer encoding each path segment and joining with / (same approach used elsewhere for blob URLs).
| setCache(prev => { | ||
| const next = new Map(prev) | ||
| next.set(key, { project: prev.get(key)?.project ?? null, loading: true, error: null }) | ||
| return next |
There was a problem hiding this comment.
The cache semantics in getProject say null = fetched, not found and undefined = not yet fetched, but fetchProject writes { project: null, loading: true } for a brand-new key. That makes getProject return null while the request is still in flight, which can cause consumers to treat an in-flight fetch as “not found”. Consider allowing project to be undefined in the cache entry (or don’t insert an entry until completion) so in-flight and 404 states remain distinguishable.
| if (loading || (cachedProject === undefined && !error)) { | ||
| return <LoadingIndicator /> |
There was a problem hiding this comment.
If the URL only contains /owner (no repo segment), cachedProject stays undefined and error stays null, so this condition will render LoadingIndicator indefinitely because no fetch is triggered. Consider handling invalid/missing slug parts up-front (e.g., render <NotFound /> when !owner || !name).
Summary
/api/projects,/api/projects/[owner]/[repo])Removed
GitHubProjectDataSourceandGitHubRepositoryDataSourcereplaced byGitHubProjectListDataSourceandGitHubProjectDetailsDataSourceCachingProjectDataSource,FilteringGitHubRepositoryDataSource,IProjectDataSource,IProjectRepositoryremoved in favour of their list/details equivalentsProjectsContextProviderreplaced byProjectListContextProviderandProjectDetailsContextProviderProjectsContextremoved from common/api/refresh-projectsendpoint removed (refresh is now driven by the frontend via?refresh=true)