Conversation
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideReplaces JSON-based deep cloning with shallow, immutable updates for treatment plan goals/objectives in TreatmentPlanManager, improving performance and keeping React state updates structurally shared. Sequence diagram for addObjective immutable state updatesequenceDiagram
actor Therapist
participant TreatmentPlanManager
participant ReactStateNewPlanData as React_state_newPlanData
Therapist->>TreatmentPlanManager: click AddObjective(goalIndex)
activate TreatmentPlanManager
TreatmentPlanManager->>TreatmentPlanManager: addObjective(goalIndex, isEdit=false)
TreatmentPlanManager->>TreatmentPlanManager: create newObjective
TreatmentPlanManager->>TreatmentPlanManager: updatedGoals = [...newPlanData.goals]
TreatmentPlanManager->>TreatmentPlanManager: updatedGoals[goalIndex] = { ...goal, objectives: [...goal.objectives, newObjective] }
TreatmentPlanManager->>ReactStateNewPlanData: setNewPlanData(prev => ({ ...prev, goals: updatedGoals }))
deactivate TreatmentPlanManager
ReactStateNewPlanData-->>Therapist: UI re-renders with new objective
Class diagram for TreatmentPlanManager state and data modelsclassDiagram
class TreatmentPlanManager {
- error : string | null
- isCreateModalOpen : boolean
- newPlanData : FormNewPlanData
- planToDelete : TreatmentPlan | null
- isEditModalOpen : boolean
- editingPlanData : FormUpdatePlanData | null
+ addObjective(goalIndex number, isEdit boolean) void
+ updateObjectiveField(goalIndex number, objIndex number, field string, value any, isEdit boolean) void
+ removeObjective(goalIndex number, objIndex number, isEdit boolean) void
+ handleSubmitNewPlan() Promise~void~
+ openEditModal(plan TreatmentPlan) void
+ openCreateModal() void
}
class FormNewPlanData {
+ clientId : string
+ startDate : string
+ diagnosis : string
+ goals : ClientSideNewGoal[]
}
class FormUpdatePlanData {
+ id : string
+ clientId : string
+ startDate : string
+ diagnosis : string
+ goals : EditableGoal[]
}
class TreatmentPlan {
+ id : string
+ clientId : string
+ startDate : string
+ diagnosis : string
+ goals : EditableGoal[]
}
class ClientSideNewGoal {
+ id : string
+ description : string
+ objectives : ClientSideNewObjective[]
}
class EditableGoal {
+ id : string
+ description : string
+ objectives : EditableObjective[]
}
class ClientSideNewObjective {
+ description : string
+ targetDate : string
+ status : string
}
class EditableObjective {
+ id : string
+ description : string
+ targetDate : string
+ status : string
}
TreatmentPlanManager --> FormNewPlanData : uses
TreatmentPlanManager --> FormUpdatePlanData : uses
TreatmentPlanManager --> TreatmentPlan : edits
FormNewPlanData --> ClientSideNewGoal : has
FormUpdatePlanData --> EditableGoal : has
TreatmentPlan --> EditableGoal : has
ClientSideNewGoal --> ClientSideNewObjective : has
EditableGoal --> EditableObjective : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The logic for resetting/cloning
newPlanData(inuseStateinitialization,openCreateModal, and after successful create) is duplicated; consider extracting a small helper likecreateEmptyNewPlanData()so any future shape changes toinitialNewPlanDataonly need to be updated in one place. - When mapping
plan.goalsinopenEditModal, you now shallow-copy theobjectivesarray but keep each objective object by reference; if those objects are ever mutated elsewhere this could lead to subtle bugs, so you may want to also spread each objective (objectives: g.objectives ? g.objectives.map(o => ({ ...o })) : []) or at least document the assumption that objectives are treated as immutable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for resetting/cloning `newPlanData` (in `useState` initialization, `openCreateModal`, and after successful create) is duplicated; consider extracting a small helper like `createEmptyNewPlanData()` so any future shape changes to `initialNewPlanData` only need to be updated in one place.
- When mapping `plan.goals` in `openEditModal`, you now shallow-copy the `objectives` array but keep each objective object by reference; if those objects are ever mutated elsewhere this could lead to subtle bugs, so you may want to also spread each objective (`objectives: g.objectives ? g.objectives.map(o => ({ ...o })) : []`) or at least document the assumption that objectives are treated as immutable.
## Individual Comments
### Comment 1
<location path="src/components/therapy/TreatmentPlanManager.tsx" line_range="310-313" />
<code_context>
- JSON.stringify(editingPlanData.goals || []),
- ) as EditableGoal[]
+ const updatedGoals = [...(editingPlanData.goals || [])]
if (
updatedGoals[goalIndex] &&
+ updatedGoals[goalIndex].objectives &&
updatedGoals[goalIndex].objectives[objIndex]
) {
- ;(updatedGoals[goalIndex].objectives[objIndex])[
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guarding on `objectives` presence may hide inconsistent state without recovery
In `updateObjectiveField`, the new guard `updatedGoals[goalIndex].objectives && updatedGoals[goalIndex].objectives[objIndex]` means that if `objectives` is unexpectedly `undefined`, the function will silently no-op. If `objectives` should always exist in this code path, consider normalizing it to an empty array when constructing/updating goals (as when mapping from `plan.goals`) so this check only needs to validate the index and inconsistent state becomes visible instead of silently ignored.
Suggested implementation:
```typescript
isEdit = false,
) => {
if (isEdit && editingPlanData) {
const updatedGoals = (editingPlanData.goals || []).map((goal) => ({
...goal,
objectives: goal.objectives || [],
}))
if (updatedGoals[goalIndex]?.objectives[objIndex]) {
const updatedObjectives = [...updatedGoals[goalIndex].objectives]
```
To fully enforce the invariant that `objectives` is always an array and avoid future silent no-ops, you should also:
1. Normalize `objectives` to `[]` anywhere goals are constructed or updated (e.g., when mapping from `plan.goals` into `editingPlanData` or `newPlanData`).
2. Ensure any goal-creation helpers or initial state definitions set `objectives: []` instead of leaving it `undefined`.
This keeps `updateObjectiveField` simple (only index checks) and ensures inconsistent state is surfaced earlier in the data flow.
</issue_to_address>
### Comment 2
<location path="src/components/therapy/TreatmentPlanManager.tsx" line_range="524-527" />
<code_context>
? new Date(plan.startDate).toISOString().split('T')[0]
: '',
goals: plan.goals
- ? JSON.parse(
- JSON.stringify(
- plan.goals.map((g) => ({ ...g, objectives: g.objectives || [] })),
- ),
- )
- : [], // Deep copy goals, ensure objectives is array
+ ? plan.goals.map((g) => ({
+ ...g,
+ objectives: g.objectives ? [...g.objectives] : [],
+ }))
+ : [], // Shallow copy goals, ensure objectives is array
</code_context>
<issue_to_address>
**question (bug_risk):** `plan.goals` mapping now uses a shallow copy of objectives; ensure this matches expectations
This used to deep-clone each goal and objective; now only goals and their `objectives` arrays are copied, while the objective objects themselves are shared. That’s a behavior change: if any consumer mutates objective objects in place (outside the controlled update paths that spread them on edit), those mutations will now affect both `plan.goals` and `editingPlanData.goals`. Please confirm no code depends on the previous full detachment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if ( | ||
| updatedGoals[goalIndex] && | ||
| updatedGoals[goalIndex].objectives && | ||
| updatedGoals[goalIndex].objectives[objIndex] |
There was a problem hiding this comment.
suggestion (bug_risk): Guarding on objectives presence may hide inconsistent state without recovery
In updateObjectiveField, the new guard updatedGoals[goalIndex].objectives && updatedGoals[goalIndex].objectives[objIndex] means that if objectives is unexpectedly undefined, the function will silently no-op. If objectives should always exist in this code path, consider normalizing it to an empty array when constructing/updating goals (as when mapping from plan.goals) so this check only needs to validate the index and inconsistent state becomes visible instead of silently ignored.
Suggested implementation:
isEdit = false,
) => {
if (isEdit && editingPlanData) {
const updatedGoals = (editingPlanData.goals || []).map((goal) => ({
...goal,
objectives: goal.objectives || [],
}))
if (updatedGoals[goalIndex]?.objectives[objIndex]) {
const updatedObjectives = [...updatedGoals[goalIndex].objectives]To fully enforce the invariant that objectives is always an array and avoid future silent no-ops, you should also:
- Normalize
objectivesto[]anywhere goals are constructed or updated (e.g., when mapping fromplan.goalsintoeditingPlanDataornewPlanData). - Ensure any goal-creation helpers or initial state definitions set
objectives: []instead of leaving itundefined.
This keepsupdateObjectiveFieldsimple (only index checks) and ensures inconsistent state is surfaced earlier in the data flow.
| goals: plan.goals | ||
| ? JSON.parse( | ||
| JSON.stringify( | ||
| plan.goals.map((g) => ({ ...g, objectives: g.objectives || [] })), | ||
| ), | ||
| ) | ||
| : [], // Deep copy goals, ensure objectives is array | ||
| ? plan.goals.map((g) => ({ | ||
| ...g, | ||
| objectives: g.objectives ? [...g.objectives] : [], |
There was a problem hiding this comment.
question (bug_risk): plan.goals mapping now uses a shallow copy of objectives; ensure this matches expectations
This used to deep-clone each goal and objective; now only goals and their objectives arrays are copied, while the objective objects themselves are shared. That’s a behavior change: if any consumer mutates objective objects in place (outside the controlled update paths that spread them on edit), those mutations will now affect both plan.goals and editingPlanData.goals. Please confirm no code depends on the previous full detachment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Switching from JSON deep-cloning to shallow copies is a good direction for performance, but several handlers still derive updates from closed-over state while using functional setters, which can drop updates under batching/rapid interactions. Resetting from initialNewPlanData can also yield a stale startDate because it’s computed at module initialization time. Finally, openEditModal now shares objective object references with plan, which can leak mutations back into plans if any in-place edits occur.
Additional notes (2)
- Maintainability |
src/components/therapy/TreatmentPlanManager.tsx:262-262
These handlers still buildupdatedGoalsfrom the closed-overeditingPlanData/newPlanDatavalues, but then call a functional setter (setEditingPlanData((prev) => ...)/setNewPlanData((prev) => ...)). This mismatch can drop updates under rapid successive interactions because the copied base may be stale.
If you’re going to use functional updates, compute from prev inside the setter and avoid referencing editingPlanData/newPlanData directly in the handler.
- Maintainability |
src/components/therapy/TreatmentPlanManager.tsx:522-530
openEditModalnow only shallow-copies objectives arrays, but not the objective objects themselves. If any code path mutates an objective object in-place (even outside the objective handlers), it can accidentally mutate the underlyingplanobject (and thereforeplansstate) because those object references are still shared.
Summary of changes
What changed
- Replaced repeated
JSON.parse(JSON.stringify(...))deep-cloning with shallow copy + immutable updates for goals/objectives state transitions. - Updated objective handlers (
addObjective,handleObjectiveChange,removeObjective) to:- copy the
goalsarray via[...] - replace the edited goal via
{ ...goal, objectives: [...] } - avoid in-place mutation of nested arrays.
- copy the
- Updated create/reset flows (
openCreateModal, post-create reset) to reinitializenewPlanDatavia object/array spreads. - Updated
openEditModalto map goals and ensureobjectivesis an array, without JSON serialization.
| const [newPlanData, setNewPlanData] = useState<FormNewPlanData>({ | ||
| ...initialNewPlanData, | ||
| goals: [...initialNewPlanData.goals], | ||
| } as FormNewPlanData) |
There was a problem hiding this comment.
initialNewPlanData is instantiated once at module load time (it contains startDate: new Date()...). Resetting form state by spreading initialNewPlanData means the “default” startDate can become stale (e.g., leaving the tab open overnight, then opening the create modal).
Also, the repeated as FormNewPlanData assertions make it easier to accidentally drift from the real shape without TypeScript helping you.
Suggestion
Introduce a factory function and reuse it for initialization + resets so startDate is always fresh and you can avoid as:
const makeInitialNewPlanData = (): FormNewPlanData => ({
...initialNewPlanData,
startDate: new Date().toISOString().split('T')[0],
goals: [],
})
const [newPlanData, setNewPlanData] = useState<FormNewPlanData>(makeInitialNewPlanData)Then replace the reset call sites with setNewPlanData(makeInitialNewPlanData()).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
1 participant
💡 What: Replaced JSON.parse/stringify with shallow copies in state updates.
🎯 Why: JSON.parse/stringify is slow for large objects and blocks the main thread.
📊 Impact: Faster UI response when modifying goals and objectives.
🔬 Measurement: Profile the component with DevTools while adding/editing/removing objectives.
PR created automatically by Jules for task 8955394391944180226 started by @daggerstuff
Summary by Sourcery
Optimize treatment plan objective and goal state updates for better performance and immutability in the TreatmentPlanManager component.
Enhancements:
Summary by cubic
Optimized state updates in
TreatmentPlanManagerby replacing deep JSON cloning with shallow, immutable updates for goals and objectives. This reduces main-thread work and makes adding/editing/removing objectives feel faster.JSON.parse/stringifywith array/object spreads in add, update, and delete objective flows.initialNewPlanDataand plan goals.objectivesarrays exist.Written for commit 12242c4. Summary will update on new commits.