feat: VER-496 add goal management controls - #33
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds hierarchical goal validation and persistence on the server. Adds typed goal API mutations and cache updates. Adds a validated goal editor and integrates goal creation, editing, ownership, metadata, loading, and error states into the goal tree. ChangesGoal management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GoalTree
participant GoalFormModal
participant useCreateGoal
participant goalsApi
participant goalsRouter
GoalTree->>GoalFormModal: open create or edit form
GoalFormModal->>useCreateGoal: submit validated goal data
useCreateGoal->>goalsApi: createGoal(companyId, data)
goalsApi->>goalsRouter: send goal request
goalsRouter-->>goalsApi: return persisted goal
goalsApi-->>useCreateGoal: return ApiResponse<Goal>
useCreateGoal-->>GoalFormModal: update cache and complete mutation
GoalFormModal-->>GoalTree: close editor and refresh goal tree
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoAdd goal create/edit controls with hierarchy validation and UI mutations
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
server/src/__tests__/goals.test.ts (1)
138-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the company-scoped update.
The router now constrains the PATCH write with both goal ID and company ID. No test covers a cross-company update. Add a case that patches a goal through another company's path and expects 404.
💚 Proposed test
it('rejects updates through another company', async () => { const goal = await request(app) .post(`/api/companies/${companyId}/goals`) .send({ title: 'Scoped goal' }) .expect(201); const otherCompany = await request(app) .post('/api/companies') .send({ name: 'Scope Corp' }) .expect(201); await request(app) .patch(`/api/companies/${otherCompany.body.data.id}/goals/${goal.body.data.id}`) .send({ title: 'Hijacked' }) .expect(404); });🤖 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/__tests__/goals.test.ts` around lines 138 - 142, Add a test alongside the existing PATCH goal tests that creates a goal for companyId, creates a separate company, then patches the original goal through the other company’s route and asserts a 404 response. Use the existing request setup and response data conventions in goals.test.ts.ui/src/lib/api.ts (1)
159-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
CreateGoalInputcannot expresstargetDateormetrics.The server
CreateGoalBodyacceptstargetDateandmetricsand persists them (server/src/routes/goals.ts, POST handler). The typed client contract omits both fields, so callers cannot set them without a cast. Consider adding them as optional fields to keep the client contract aligned with the route.♻️ Optional addition
export interface CreateGoalInput { title: string; description?: string; level: GoalLevel; status: GoalStatus; parentId: string | null; ownerAgentId: string | null; progress: number; + targetDate?: string | null; + metrics?: Record<string, unknown>; }🤖 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 159 - 171, Update CreateGoalInput to include optional targetDate and metrics fields matching the server CreateGoalBody contract, so callers can provide both values without casts. Keep UpdateGoalInput derived from CreateGoalInput so the new fields remain available for updates.ui/test/GoalFormModal.test.tsx (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe pending-state flags are never exercised.
mocks.createPendingandmocks.updatePendingare declared and reset, but no test sets either totrue. The pending behavior is untested:dismissible={!isPending}, the disabled fields, and the loading submit button. Add one test that setsmocks.createPending = trueand asserts the disabled controls.Also applies to: 86-87
🤖 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/GoalFormModal.test.tsx` around lines 10 - 12, Add a test in the GoalFormModal test suite that sets mocks.createPending to true before rendering and asserts the modal is non-dismissible, form fields are disabled, and the submit button shows its loading/disabled state; keep the existing reset behavior and cover the analogous pending-control behavior through the rendered UI.ui/src/components/goals/GoalFormModal.tsx (1)
153-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
z.flattenError(parsed.error)for Zod 4 error formatting.
ZodError.flatten()is deprecated in the resolved Zod 4.3.6 dependency. The schema is flat, so this preserves the currentfieldErrorsbehavior.🤖 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/goals/GoalFormModal.tsx` around lines 153 - 165, Update the validation error handling in GoalFormModal’s parsed failure branch to use Zod 4’s z.flattenError(parsed.error) instead of the deprecated parsed.error.flatten() call. Preserve the existing fieldErrors mapping and setErrors behavior for all listed fields.
🤖 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/goals.ts`:
- Around line 244-261: Wrap the goal update flow in a database transaction,
moving the existing-goal read inside it and locking the company’s goals with FOR
UPDATE before validation. Update validateGoalReferences and all related queries
to use the transaction executor, then perform the goals update within the same
transaction so reference validation and persistence are serialized; add
PostgreSQL coverage for the concurrent cycle race.
In `@ui/src/components/goals/GoalFormModal.tsx`:
- Around line 117-125: Clear parentId whenever the selected level change makes
it absent from parentOptions, so state stays aligned with the visible selection
and validation. Update the level-change handling near parentOptions and ensure
the Select value and handleSubmit use the cleared, selectable parent value
without altering valid parent selections.
---
Nitpick comments:
In `@server/src/__tests__/goals.test.ts`:
- Around line 138-142: Add a test alongside the existing PATCH goal tests that
creates a goal for companyId, creates a separate company, then patches the
original goal through the other company’s route and asserts a 404 response. Use
the existing request setup and response data conventions in goals.test.ts.
In `@ui/src/components/goals/GoalFormModal.tsx`:
- Around line 153-165: Update the validation error handling in GoalFormModal’s
parsed failure branch to use Zod 4’s z.flattenError(parsed.error) instead of the
deprecated parsed.error.flatten() call. Preserve the existing fieldErrors
mapping and setErrors behavior for all listed fields.
In `@ui/src/lib/api.ts`:
- Around line 159-171: Update CreateGoalInput to include optional targetDate and
metrics fields matching the server CreateGoalBody contract, so callers can
provide both values without casts. Keep UpdateGoalInput derived from
CreateGoalInput so the new fields remain available for updates.
In `@ui/test/GoalFormModal.test.tsx`:
- Around line 10-12: Add a test in the GoalFormModal test suite that sets
mocks.createPending to true before rendering and asserts the modal is
non-dismissible, form fields are disabled, and the submit button shows its
loading/disabled state; keep the existing reset behavior and cover the analogous
pending-control behavior through the rendered UI.
🪄 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: 876e0fce-5fca-4e56-bc2c-767e310fa9c7
📒 Files selected for processing (9)
server/src/__tests__/goals.test.tsserver/src/routes/goals.tsui/src/components/goals/GoalFormModal.tsxui/src/lib/api.tsui/src/lib/hooks.tsui/src/pages/GoalTree.tsxui/test/GoalFormModal.test.tsxui/test/GoalTree.test.tsxui/test/goal-hooks.test.tsx
| const relationChanged = body.parentId !== undefined || body.level !== undefined; | ||
| await validateGoalReferences({ | ||
| companyId, | ||
| goalId: id, | ||
| ownerAgentId: body.ownerAgentId, | ||
| parentId: relationChanged | ||
| ? body.parentId !== undefined ? body.parentId : existing.parentId | ||
| : undefined, | ||
| level: relationChanged ? body.level ?? existing.level : undefined, | ||
| }); | ||
|
|
||
| const progressChanged = | ||
| body.progress !== undefined && body.progress !== existing.progress; | ||
|
|
||
| const [updated] = await db.drizzle | ||
| .update(goals) | ||
| .set({ ...body, updatedAt: new Date() }) | ||
| .where(eq(goals.id, id)) | ||
| .where(and(eq(goals.id, id), eq(goals.companyId, companyId))) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="$(git ls-files | rg '(^|/)server/src/routes/goals\.ts$' | head -n1)"
printf '%s\n' "FILE=$file"
wc -l "$file"
printf '%s\n' '--- route structure ---'
ast-grep outline "$file" --lang typescript
printf '%s\n' '--- target and nearby code ---'
sed -n '180,330p' "$file"
printf '%s\n' '--- validation and tree references ---'
rg -n -C 4 'validateGoalReferences|/tree|nodeMap|parentId|for\(|transaction' "$file" server/src server/test test tests 2>/dev/null | head -n 500
printf '%s\n' '--- relevant tracked files ---'
git ls-files | rg -i '(goal|pglite|drizzle|migration|schema|test|package\.json)' | head -n 500Repository: VerticalLabs-ai/eidolon
Length of output: 42814
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all goal route definitions and imports ---'
file="$(git ls-files | rg '(^|/)server/src/routes/goals\.ts$' | head -n1)"
sed -n '1,180p' "$file"
printf '%s\n' '--- exact validation implementation ---'
rg -n -l 'function validateGoalReferences|validateGoalReferences' server/src server 2>/dev/null | while IFS= read -r f; do
echo "FILE=$f"
rg -n -C 25 'function validateGoalReferences|validateGoalReferences' "$f"
done
printf '%s\n' '--- database setup and transaction support ---'
rg -n -C 5 'PGlite|pglite|drizzle\(|drizzle-orm|forUpdate|FOR UPDATE|for\(' server package.json '**/package.json' 2>/dev/null | head -n 500
printf '%s\n' '--- goal tree implementation ---'
rg -n -C 25 'nodeMap|goals/tree|tree' server/src server 2>/dev/null | head -n 500Repository: VerticalLabs-ai/eidolon
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
find . -path '*/server/src/routes/goals.ts' -printRepository: VerticalLabs-ai/eidolon
Length of output: 190
🏁 Script executed:
rg -n -C 3 "validateGoalReferences|nodeMap|PGlite|pglite|transaction|for\('update'\)" .Repository: VerticalLabs-ai/eidolon
Length of output: 47285
🌐 Web query:
PGlite PostgreSQL FOR UPDATE row locking transaction concurrent transactions support
💡 Result:
PGlite is a WebAssembly-based distribution of PostgreSQL that operates as a single-process, single-user instance [1]. Because of its architecture, it behaves differently regarding row locking and transaction concurrency compared to a standard, multi-process PostgreSQL server [1]. Row Locking (SELECT FOR UPDATE) PGlite supports the standard PostgreSQL SQL syntax, including row-level locking commands like SELECT FOR UPDATE [2][3]. However, because PGlite operates as a single-process system [1], these locks do not function as a mechanism for handling concurrency between multiple independent client connections in the same way they would on a standard Postgres server. Within the context of a single PGlite instance, there is only one user/connection [1], so there are no concurrent transactions from other users to block. The commands will execute, but they will not provide the cross-connection protection that row-level locks typically offer in a multi-user environment [1][2]. Transaction Concurrency PGlite does not support concurrent transactions in the traditional sense because it is fundamentally a single-user, single-process engine [1]. 1. Single-Process Architecture: PGlite runs in a single-threaded WASM environment [1]. It does not fork processes for new connections [1]. 2. Transaction Handling: While PGlite supports transactions via its.transaction API (which ensures atomic operations for the single connection) [3], it cannot execute multiple transactions simultaneously. 3. Multi-connection management: Tools like pglite-socket can facilitate interactions that appear to manage multiple clients or connection pools (e.g., from Node.js) by queueing and scheduling protocol messages [4]. These tools help maintain isolation and prevent corruption when multiple clients attempt to use the same PGlite instance, but they do so by serializing the work rather than enabling true parallel transaction processing [4]. If your application requires true concurrent transaction support, a standard PostgreSQL server deployment is required, as PGlite's architecture is specifically optimized for single-process, WASM-based environments [1].
Citations:
- 1: https://github.com/electric-sql/pglite/
- 2: https://www.postgresql.org/docs/current/explicit-locking.html
- 3: https://pglite.dev/docs/api
- 4: Fix extended-query isolation for pooled postgres.js clients in pglite-socket electric-sql/pglite#977
Serialize goal-reference validation and the update in one transaction.
Concurrent production requests can both pass the cycle check and persist A.parentId = B and B.parentId = A. GET /goals/tree then returns neither goal as a root. Move the existing read into the transaction, lock the company’s goals with FOR UPDATE, and run every validation query through the same transaction executor. PGlite supports these operations, but its single-connection harness cannot test cross-connection locking; cover this race against PostgreSQL.
🤖 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/goals.ts` around lines 244 - 261, Wrap the goal update flow
in a database transaction, moving the existing-goal read inside it and locking
the company’s goals with FOR UPDATE before validation. Update
validateGoalReferences and all related queries to use the transaction executor,
then perform the goals update within the same transaction so reference
validation and persistence are serialized; add PostgreSQL coverage for the
concurrent cycle race.
| const parentOptions = [ | ||
| { value: "", label: "No parent (root goal)" }, | ||
| ...goals | ||
| .filter( | ||
| (item) => !unavailableParents.has(item.id) | ||
| && goalLevelRank[item.level] < goalLevelRank[level], | ||
| ) | ||
| .map((item) => ({ value: item.id, label: item.title })), | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale parentId remains after a level change, so the visible Select value and the validated value disagree.
parentOptions is recomputed from level, but parentId state is not. If the user raises the level, the previously selected parent is filtered out. The Select then has a value with no matching option, so the browser displays the first option, "No parent (root goal)". The state still holds the removed id. On submit, the code rejects with "Choose a parent above this goal's level.", which contradicts the displayed selection. The test at ui/test/GoalFormModal.test.tsx Line 215 shows the option is removed while submission still fails.
Clear parentId when it is no longer selectable.
🐛 Proposed fix
const parentOptions = [
{ value: "", label: "No parent (root goal)" },
...goals
.filter(
(item) => !unavailableParents.has(item.id)
&& goalLevelRank[item.level] < goalLevelRank[level],
)
.map((item) => ({ value: item.id, label: item.title })),
];
+ const selectableParentId = parentOptions.some((option) => option.value === parentId)
+ ? parentId
+ : "";Then use selectableParentId for the Select value and in handleSubmit, or reset the state in the level onChange:
- onChange={(event) => setLevel(event.target.value as GoalLevel)}
+ onChange={(event) => {
+ const nextLevel = event.target.value as GoalLevel;
+ setLevel(nextLevel);
+ const parent = goals.find((item) => item.id === parentId);
+ if (parent && goalLevelRank[parent.level] >= goalLevelRank[nextLevel]) {
+ setParentId("");
+ }
+ }}🤖 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/goals/GoalFormModal.tsx` around lines 117 - 125, Clear
parentId whenever the selected level change makes it absent from parentOptions,
so state stays aligned with the visible selection and validation. Update the
level-change handling near parentOptions and ensure the Select value and
handleSubmit use the cleared, selectable parent value without altering valid
parent selections.
Code Review by Qodo
Context used✅ Compliance rules (platform):
2 rules 1. Unneeded full goals scan
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| <div | ||
| className="h-2 overflow-hidden rounded-full bg-surface-overlay" | ||
| role="progressbar" | ||
| aria-label={`${goal.title} progress`} | ||
| aria-valuemin={0} | ||
| aria-valuemax={100} | ||
| aria-valuenow={goal.progress} | ||
| > | ||
| <div | ||
| className="h-full rounded-full bg-accent transition-all duration-500 ease-out" | ||
| style={{ width: `${goal.progress ?? 0}%` }} | ||
| style={{ width: `${goal.progress}%` }} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
1. Goaltree reimplements progressbar 📘 Rule violation ⚙ Maintainability
ui/src/pages/GoalTree.tsx renders a custom progress bar instead of using the existing design-system ProgressBar component, duplicating shared UI behavior and styling. This increases maintenance burden and risks inconsistent accessibility/visual behavior across the app.
Agent Prompt
## Issue description
The Goals tree uses an inline/custom progress bar implementation even though a shared design-system `ProgressBar` component already exists.
## Issue Context
This duplicates existing UI primitives and can cause inconsistent styling/accessibility behaviors across pages.
## Fix Focus Areas
- ui/src/pages/GoalTree.tsx[169-185]
- ui/src/components/ui/ProgressBar.tsx[26-69]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (parentId !== undefined || (goalId && level !== undefined)) { | ||
| const companyGoals = await db.drizzle | ||
| .select({ id: goals.id, parentId: goals.parentId, level: goals.level }) | ||
| .from(goals) | ||
| .where(eq(goals.companyId, companyId)); | ||
| const goalsById = new Map(companyGoals.map((goal) => [goal.id, goal])); | ||
|
|
There was a problem hiding this comment.
2. Unneeded full goals scan 🐞 Bug ➹ Performance
POST /goals always runs validateGoalReferences()'s full-company SELECT+Map build even for root goals, because CreateGoalBody defaults parentId to null and the guard only checks for undefined. This makes every goal creation O(number of company goals) in DB work and memory allocation, increasing latency/load as goal counts grow.
Agent Prompt
## Issue description
`validateGoalReferences()` loads every goal for the company whenever `parentId !== undefined`, but `CreateGoalBody` defaults `parentId` to `null`. This means root goal creation (no parent) still triggers a full company goals query + `Map` allocation that is not used for any validation.
## Issue Context
- Root creates pass `parentId: null` into `validateGoalReferences()`.
- The guard `parentId !== undefined` treats `null` as “present”, so it performs the company-wide scan.
## Fix Focus Areas
- server/src/routes/goals.ts[10-20]
- server/src/routes/goals.ts[47-66]
## Recommended fix
Update the guard so it only loads company goals when a *non-null* parent is being validated, or when `goalId && level !== undefined` is needed for child-level checks. For example:
```ts
if ((parentId !== undefined && parentId !== null) || (goalId && level !== undefined)) {
...
}
```
Optionally, also consider making `CreateGoalBody.parentId` `optional()` (no default) and normalize to `null` only at insert time, so omitted parent values don’t look “set” to validation logic.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo FixerNo findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page. |
1 participant
Summary
Verification
pnpm test:run server/src/__tests__/goals.test.ts(6 passed)pnpm --dir ui exec vitest run --config vitest.config.ts test/GoalFormModal.test.tsx test/GoalTree.test.tsx test/goal-hooks.test.tsx(13 passed)pnpm typecheckpnpm buildnode .gitnexus/run.cjs detect-changes --scope staged(medium, expected goal/API/UI flows)~/.agents/skills/autoreview/scripts/autoreview --mode local(clean, 0.98)CodeRabbit / Review Notes
Risk / Rollout Notes
Screenshots / UI Notes
Fixes VER-496
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Summary by CodeRabbit
New Features
Bug Fixes