VER-500: complete operator project lifecycle management - #35
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds shared HTTP(S) repository URL validation, project detail APIs and hooks, a reusable create/edit modal, project editing and soft-archiving actions, cache synchronization, and server and UI tests. ChangesProject management workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectDetail
participant ProjectHooks
participant ProjectAPI
participant ProjectServer
ProjectDetail->>ProjectHooks: load, update, or archive project
ProjectHooks->>ProjectAPI: call project endpoint
ProjectAPI->>ProjectServer: GET, PATCH, or DELETE request
ProjectServer-->>ProjectAPI: project response
ProjectAPI-->>ProjectHooks: persisted project
ProjectHooks-->>ProjectDetail: update caches and render state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
PR Summary by QodoProject lifecycle: add project detail/edit/archive flows with shared form and cache sync
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
2 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
Qodo Fixer✅ Committed (1) · ☑ Fixed (1) Commits pushed directly to this PR — no separate fix PR opened. Process — 1 fixed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ui/src/lib/hooks.ts (1)
132-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared cache sync and drop the redundant invalidation.
The
onSuccessbodies ofuseUpdateProjectanduseArchiveProjectare identical. Extract one helper to keep them in sync.React Query matches query keys by prefix. The list key
["projects", companyId]is a prefix of the detail key["projects", companyId, project.id], so the firstinvalidateQueriescall already invalidates the detail query. The second call adds no effect. The same overlap makes every list invalidation refetch all cached project details. If you want the two scopes to be independent, add a discriminator segment to the detail key, for example["projects", companyId, "detail", projectId], and updateuseProjectandui/test/project-hooks.test.tsxaccordingly.♻️ Proposed refactor
+function syncProjectCaches(qc: ReturnType<typeof useQueryClient>, companyId: string, project: api.Project) { + qc.setQueryData(["projects", companyId, project.id], project); + qc.setQueryData<api.Project[]>(["projects", companyId], (current) => + current?.map((item) => (item.id === project.id ? project : item)), + ); + qc.invalidateQueries({ queryKey: ["projects", companyId] }); +} + export function useUpdateProject(companyId: string) { const qc = useQueryClient(); @@ - onSuccess: (project) => { - qc.setQueryData(["projects", companyId, project.id], project); - qc.setQueryData<api.Project[]>(["projects", companyId], (current) => - current?.map((item) => item.id === project.id ? project : item), - ); - qc.invalidateQueries({ queryKey: ["projects", companyId] }); - qc.invalidateQueries({ queryKey: ["projects", companyId, project.id] }); - }, + onSuccess: (project) => syncProjectCaches(qc, companyId, project), }); }Apply the same change to
useArchiveProject.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/lib/hooks.ts` around lines 132 - 157, Extract the identical project cache synchronization from useUpdateProject and useArchiveProject into a shared helper, then call it from both onSuccess handlers. Remove the redundant detail invalidateQueries call, since invalidating ["projects", companyId] already covers the detail key; preserve the existing query-key structure unless independently updating all related consumers.ui/test/CreateProjectModal.test.tsx (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared
isPendingmock inuseUpdateProject.
useCreateProjectreadsmocks.isPending, but this mock hardcodesfalse. A test that togglesmocks.isPendingthen exercises the create path only.♻️ Proposed change
useUpdateProject: () => ({ mutate: mocks.updateProject, reset: mocks.reset, - isPending: false, + isPending: mocks.isPending, }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/test/CreateProjectModal.test.tsx` around lines 19 - 23, Update the useUpdateProject mock to return the shared mocks.isPending value instead of hardcoding false, so tests that toggle pending state exercise both create and update paths consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/routes/projects.ts`:
- Around line 10-13: Guard the protocol validation in repoUrlSchema at
server/src/routes/projects.ts lines 10-13 with try/catch so malformed URLs
produce a 400 validation error. In
ui/src/components/projects/ProjectFormModal.tsx lines 27-35, replace the union
with one guarded refinement accepting empty strings and valid http(s) URLs;
extract and reuse the client-side predicate in isSafeRepoUrl at
ui/src/pages/ProjectDetail.tsx lines 30-36, and add a form test submitting
github.com/org/repo that asserts a field error.
In `@ui/src/components/projects/ProjectFormModal.tsx`:
- Around line 68-76: Update the initialization useEffect in ProjectFormModal so
it is not triggered by every project object identity change while the modal is
open. Key the effect on the project identifier (alongside open as needed), while
preserving the existing field initialization and error reset behavior when
opening or switching projects.
In `@ui/src/lib/api.ts`:
- Around line 269-293: Update getProjects to request ApiResponse<Project[]>
instead of Project[], matching the wrapped response returned by the project list
endpoint; leave getProject and the project mutation methods unchanged.
---
Nitpick comments:
In `@ui/src/lib/hooks.ts`:
- Around line 132-157: Extract the identical project cache synchronization from
useUpdateProject and useArchiveProject into a shared helper, then call it from
both onSuccess handlers. Remove the redundant detail invalidateQueries call,
since invalidating ["projects", companyId] already covers the detail key;
preserve the existing query-key structure unless independently updating all
related consumers.
In `@ui/test/CreateProjectModal.test.tsx`:
- Around line 19-23: Update the useUpdateProject mock to return the shared
mocks.isPending value instead of hardcoding false, so tests that toggle pending
state exercise both create and update paths consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e8ff81c5-7f6e-4c89-a23e-5329ce7e9cd5
📒 Files selected for processing (10)
server/src/__tests__/projects.test.tsserver/src/routes/projects.tsui/src/components/projects/CreateProjectModal.tsxui/src/components/projects/ProjectFormModal.tsxui/src/lib/api.tsui/src/lib/hooks.tsui/src/pages/ProjectDetail.tsxui/test/CreateProjectModal.test.tsxui/test/ProjectDetail.test.tsxui/test/project-hooks.test.tsx
Comment on lines
+10
to
+13
| const repoUrlSchema = z.string().url().refine( | ||
| (value) => ['http:', 'https:'].includes(new URL(value).protocol), | ||
| 'Repo URL must start with http(s)', | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unguarded new URL() inside Zod refinements throws on invalid repository URLs. Both new repoUrl schemas run new URL(value) in a .refine callback that Zod still executes after the preceding URL format check fails, and Zod does not catch the resulting TypeError.
server/src/routes/projects.ts#L10-L13: wrap the protocol check in try/catch so an invalidrepoUrlreturns a 400 validation error instead of an unhandled error.ui/src/components/projects/ProjectFormModal.tsx#L27-L35: replace the union with one guarded refinement that accepts""and http(s) URLs, so submit shows a field error instead of throwing.
Consider one shared predicate for the client side, reused by isSafeRepoUrl in ui/src/pages/ProjectDetail.tsx#L30-L36. Add a test that submits github.com/org/repo in the form to lock this behavior.
📍 Affects 2 files
server/src/routes/projects.ts#L10-L13(this comment)ui/src/components/projects/ProjectFormModal.tsx#L27-L35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/routes/projects.ts` around lines 10 - 13, Guard the protocol
validation in repoUrlSchema at server/src/routes/projects.ts lines 10-13 with
try/catch so malformed URLs produce a 400 validation error. In
ui/src/components/projects/ProjectFormModal.tsx lines 27-35, replace the union
with one guarded refinement accepting empty strings and valid http(s) URLs;
extract and reuse the client-side predicate in isSafeRepoUrl at
ui/src/pages/ProjectDetail.tsx lines 30-36, and add a form test submitting
github.com/org/repo that asserts a field error.
Comment on lines
+68
to
+76
| useEffect(() => { | ||
| if (!open) return; | ||
| setName(project?.name ?? ""); | ||
| setDescription(project?.description ?? ""); | ||
| setStatus(project?.status === "archived" ? "planning" : (project?.status ?? "planning")); | ||
| setRepoUrl(project?.repoUrl ?? ""); | ||
| setErrors({}); | ||
| setSubmitError(null); | ||
| }, [open, project]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not re-initialize the open form on every project object identity change.
ProjectDetail.tsx passes the project object from the useProject cache. A background refetch of that query produces a new object identity with the same content. This effect then runs while the modal is open and overwrites the field values that the operator is editing. Key the initialization on the project identifier instead.
🐛 Proposed fix
useEffect(() => {
if (!open) return;
setName(project?.name ?? "");
setDescription(project?.description ?? "");
setStatus(project?.status === "archived" ? "planning" : (project?.status ?? "planning"));
setRepoUrl(project?.repoUrl ?? "");
setErrors({});
setSubmitError(null);
- }, [open, project]);
+ // Initialize once per opened project; later cache updates must not discard operator input.
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [open, project?.id]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!open) return; | |
| setName(project?.name ?? ""); | |
| setDescription(project?.description ?? ""); | |
| setStatus(project?.status === "archived" ? "planning" : (project?.status ?? "planning")); | |
| setRepoUrl(project?.repoUrl ?? ""); | |
| setErrors({}); | |
| setSubmitError(null); | |
| }, [open, project]); | |
| useEffect(() => { | |
| if (!open) return; | |
| setName(project?.name ?? ""); | |
| setDescription(project?.description ?? ""); | |
| setStatus(project?.status === "archived" ? "planning" : (project?.status ?? "planning")); | |
| setRepoUrl(project?.repoUrl ?? ""); | |
| setErrors({}); | |
| setSubmitError(null); | |
| // Initialize once per opened project; later cache updates must not discard operator input. | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [open, project?.id]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/src/components/projects/ProjectFormModal.tsx` around lines 68 - 76, Update
the initialization useEffect in ProjectFormModal so it is not triggered by every
project object identity change while the modal is open. Key the effect on the
project identifier (alongside open as needed), while preserving the existing
field initialization and error reset behavior when opening or switching
projects.
Comment on lines
269
to
+293
| export const getProjects = (companyId: string) => | ||
| request<Project[]>(`/companies/${companyId}/projects`); | ||
|
|
||
| export const getProject = (companyId: string, projectId: string) => | ||
| request<ApiResponse<Project>>(`/companies/${companyId}/projects/${projectId}`); | ||
|
|
||
| export const createProject = (companyId: string, data: CreateProjectInput) => | ||
| request<ApiResponse<Project>>(`/companies/${companyId}/projects`, { | ||
| method: "POST", | ||
| body: JSON.stringify(data), | ||
| }); | ||
|
|
||
| export const updateProject = ( | ||
| companyId: string, | ||
| projectId: string, | ||
| data: UpdateProjectInput, | ||
| ) => request<ApiResponse<Project>>(`/companies/${companyId}/projects/${projectId}`, { | ||
| method: "PATCH", | ||
| body: JSON.stringify(data), | ||
| }); | ||
|
|
||
| export const archiveProject = (companyId: string, projectId: string) => | ||
| request<ApiResponse<Project>>(`/companies/${companyId}/projects/${projectId}`, { | ||
| method: "DELETE", | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the request helper, ApiResponse type, and unwrap helper.
rg -nP -C 8 '(async function request|const request\s*=|interface ApiResponse|function unwrap)' ui/src/libRepository: VerticalLabs-ai/eidolon
Length of output: 2371
🏁 Script executed:
#!/bin/bash
# Inspect all project API declarations and hook call sites.
rg -n -C 12 'getProjects|getProject|createProject|updateProject|archiveProject|unwrap<' ui/src/lib/api.ts ui/src/lib/hooks.tsRepository: VerticalLabs-ai/eidolon
Length of output: 50379
🏁 Script executed:
#!/bin/bash
# Compare neighboring API typings and inspect the server project response shapes.
printf '%s\n' '--- api.ts neighboring declarations ---'
sed -n '45,125p' ui/src/lib/api.ts
printf '%s\n' '--- server project routes and response wrappers ---'
rg -n -C 8 'projects|Project' --glob '*.py' --glob '*.ts' --glob '*.tsx' . | head -n 240Repository: VerticalLabs-ai/eidolon
Length of output: 20198
🏁 Script executed:
#!/bin/bash
# Locate and inspect only the project route implementation and its response statements.
project_route="$(fd -t f 'projects\.js$|projects\.ts$' server)"
printf 'route=%s\n' "$project_route"
if [ -n "$project_route" ]; then
rg -n -C 12 'res\.(json|send)|json\(|data:|router\.(get|post|patch|delete)' "$project_route"
fiRepository: VerticalLabs-ai/eidolon
Length of output: 5330
Use ApiResponse<Project[]> for getProjects.
request<T> returns Promise<T>. The project list endpoint returns { data: rows, meta: ... }, so request<Project[]> does not describe the response shape. The ApiResponse<Project> generics for getProject and the mutations are correct.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/src/lib/api.ts` around lines 269 - 293, Update getProjects to request
ApiResponse<Project[]> instead of Project[], matching the wrapped response
returned by the project list endpoint; leave getProject and the project mutation
methods unchanged.
Comment on lines
+10
to
+17
| const HttpUrl = z.string().url().refine((value) => { | ||
| try { | ||
| const protocol = new URL(value).protocol; | ||
| return protocol === 'http:' || protocol === 'https:'; | ||
| } catch { | ||
| return false; | ||
| } | ||
| }, 'Repository URL must start with http:// or https://'); |
Contributor
There was a problem hiding this comment.
1. Repo url allows credentials 🐞 Bug ⛨ Security
Server/client URL validation only enforces http(s) protocol, so credential-bearing URLs (e.g., https://token@github.com/org/repo) can be persisted. ProjectDetail then renders the full unredacted URL as link text and href, potentially exposing embedded secrets to anyone who can view the project.
Agent Prompt
### Issue description
Repository URLs currently validate only that the scheme is `http:` or `https:`. This still permits URLs containing userinfo credentials (username/password/token in the URL), which can then be stored and rendered verbatim in the UI.
### Issue Context
Both the server (`HttpUrl`) and client (`isHttpUrl` + Zod refine) share the same limitation. The detail page renders the stored URL directly as an external link.
### Fix Focus Areas
- server/src/routes/projects.ts[10-17]
- ui/src/lib/urls.ts[1-7]
- ui/src/components/projects/ProjectFormModal.tsx[28-33]
- ui/src/pages/ProjectDetail.tsx[81-120]
### Suggested fix
1. Update server `HttpUrl` refine to additionally require:
- `new URL(value).username === ""`
- `new URL(value).password === ""`
(Optionally also reject non-empty `hash`/suspicious query params if desired.)
2. Update `ui/src/lib/urls.ts:isHttpUrl` to apply the same username/password checks.
3. Consider hardening the display layer (`ProjectDetail`) by redacting userinfo (e.g., render `url.origin + url.pathname + url.search + url.hash` without credentials) or by suppressing the link when userinfo is present, as defense-in-depth.
4. Add/extend tests to cover credential-bearing URLs being rejected/redacted.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit c577575 |
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Contributor
|
Code review by qodo was updated up to the latest commit af4f1a7 |
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
1 participant
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
Fixes VER-500
Verification
CodeRabbit / Review Notes
Risk / Rollout Notes
Screenshots / UI Notes
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Summary by CodeRabbit