Add explicit Save button to detail panel edit mode#7
Add explicit Save button to detail panel edit mode#7JohnRDOrazio wants to merge 6 commits intomainfrom
Conversation
Adds a Save button next to Cancel in edit mode for ClassDetailPanel, PropertyDetailPanel, and IndividualDetailPanel. Reorders header buttons so Graph stays in a fixed position (after edit/save/cancel buttons). Closes #6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an explicit Save-and-exit workflow across Class, Property, and Individual detail panels: Save calls triggerSave, awaits flushToGit (now returning Promise), and exits edit mode on success while showing a saving state and disabling the Save button during flush. Changes
Sequence Diagram(s)mermaid User->>UI: Click "Save" Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/editor/ClassDetailPanel.tsx`:
- Around line 200-205: saveAndExitEditMode currently calls triggerSave(), awaits
flushToGit(), and then always calls setIsEditing(false) so the UI exits edit
mode even when save fails; modify flushToGit (and the analogous function in
useEntityAutoSave) to return a boolean success indicator (return true on
success, false on caught error), then update saveAndExitEditMode to await the
boolean result from flushToGit and only call setIsEditing(false) when it returns
true (keep triggerSave and existing error-toasting behavior intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6458fda6-ec64-4f65-bc71-57190f3edca2
📒 Files selected for processing (3)
components/editor/ClassDetailPanel.tsxcomponents/editor/IndividualDetailPanel.tsxcomponents/editor/PropertyDetailPanel.tsx
| // Explicit save: flush draft to git and exit edit mode | ||
| const saveAndExitEditMode = useCallback(async () => { | ||
| triggerSave(); | ||
| await flushToGit(); | ||
| setIsEditing(false); | ||
| }, [triggerSave, flushToGit]); |
There was a problem hiding this comment.
User exits edit mode even when save fails.
flushToGit() catches errors internally and doesn't re-throw (see lib/hooks/useAutoSave.ts:144-211). This means setIsEditing(false) always executes, even when the save operation fails. The user will see an error toast but find themselves back in read-only mode, which is confusing.
🛠️ Proposed fix: check saveStatus before exiting
One approach is to have flushToGit return a success indicator, then conditionally exit:
- // Explicit save: flush draft to git and exit edit mode
- const saveAndExitEditMode = useCallback(async () => {
- triggerSave();
- await flushToGit();
- setIsEditing(false);
- }, [triggerSave, flushToGit]);
+ // Explicit save: flush draft to git and exit edit mode only on success
+ const saveAndExitEditMode = useCallback(async () => {
+ triggerSave();
+ const success = await flushToGit();
+ if (success) {
+ setIsEditing(false);
+ }
+ }, [triggerSave, flushToGit]);This requires modifying flushToGit in useAutoSave to return true on success and false on error:
// In useAutoSave.ts flushToGit:
try {
// ... existing success logic
return true;
} catch (err) {
// ... existing error handling
return false;
}Apply the same pattern to useEntityAutoSave for the other panels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/editor/ClassDetailPanel.tsx` around lines 200 - 205,
saveAndExitEditMode currently calls triggerSave(), awaits flushToGit(), and then
always calls setIsEditing(false) so the UI exits edit mode even when save fails;
modify flushToGit (and the analogous function in useEntityAutoSave) to return a
boolean success indicator (return true on success, false on caught error), then
update saveAndExitEditMode to await the boolean result from flushToGit and only
call setIsEditing(false) when it returns true (keep triggerSave and existing
error-toasting behavior intact).
Make flushToGit return a boolean success indicator so saveAndExitEditMode only exits edit mode when the save actually succeeds. Previously, a failed save would still switch the user back to read-only mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/hooks/useAutoSave.ts (1)
144-213:⚠️ Potential issue | 🟠 MajorDon't flush a stale class draft after validation fails.
With the new
triggerSave(); await flushToGit()flow, clearing all labels can still save the previous draft:triggerSave()stops at validation, but this method only checks whether an older draft exists. If that flush succeeds, callers gettrueand leave edit mode even though the current form is invalid. Re-validateeditStateRef.currenthere and returnfalsebefore reading the draft.Suggested guard
const flushToGit = useCallback(async (): Promise<boolean> => { if (flushingRef.current) return false; const canFlush = canEdit || saveMode === "suggest"; const hasHandler = saveMode === "suggest" ? !!onSuggestSave : !!onUpdateClass; if (!classIri || !branch || !canFlush || !hasHandler) return false; + const state = editStateRef.current; + if (!state || state.labels.every((l) => !l.value.trim())) { + setValidationError("At least one label is required"); + return false; + } + setValidationError(null); const key = draftKey(projectId, branch, classIri); const rawDraft = getDraft(key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useAutoSave.ts` around lines 144 - 213, flushToGit can flush a stale draft because it only checks stored draft existence; before reading the draft inside flushToGit re-check the current in-memory edit state (editStateRef.current) for validity and return false if it fails validation. In practice, add a guard near the top of flushToGit (before calling draftKey/getDraft) that inspects editStateRef.current (same validation logic used by triggerSave — e.g., labels/comments/required fields trimmed/non-empty) and if invalid immediately set flushingRef.current=false (if set), leave save status unchanged or set appropriate error, and return false so stale drafts are not flushed. Ensure you reference editStateRef.current and flushToGit when making the change.components/editor/PropertyDetailPanel.tsx (1)
476-491:⚠️ Potential issue | 🟠 MajorPropertyDetailPanel still can't render the fixed Graph action.
This header now swaps
[Edit Item]for[Save] [Cancel], but it still has no Graph button or action slot. The required[Edit Item] [Graph]↔[Save] [Cancel] [Graph]layout from the issue cannot be produced in this panel yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/editor/PropertyDetailPanel.tsx` around lines 476 - 491, PropertyDetailPanel currently conditionally renders only the Edit/Save/Cancel controls (isEditing, enterEditMode, saveAndExitEditMode, cancelEditMode) so the Graph action is missing; to fix, render the Graph action/button in the same header container outside (or duplicated in both branches of) the isEditing conditional so it is always present — either create a dedicated renderGraphAction/GraphButton element and place it next to the existing buttons inside the div with className "flex shrink-0 items-center gap-1" or add the Graph JSX to both the editing and non-editing branches so the layout becomes "[Edit Item] [Graph]" ↔ "[Save] [Cancel] [Graph]" while preserving saveStatus and button handlers.lib/hooks/useEntityAutoSave.ts (1)
118-145:⚠️ Potential issue | 🟠 MajorRe-run validation inside
flushToGit().
triggerSave()aborts on validation errors but leaves any older draft in the store. BecauseflushToGit()only checks for draft existence, the new save-and-exit flow can still continue after a failed validation; incomponents/editor/PropertyDetailPanel.tsx,onFlushrebuilds the payload from live edit state, so an invalid edit can still be posted here. Returnfalsebefore flushing whenvalidateRef.current?.()reports an error.Suggested guard
const flushToGit = useCallback(async (): Promise<boolean> => { if (flushingRef.current) return false; if (!entityIri || !branch || !canEdit || !onFlushRef.current) return false; + const error = validateRef.current?.(); + if (error) { + setValidationError(error); + return false; + } + setValidationError(null); const key = draftKey(projectId, branch, entityIri); const draft = getDraft(key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useEntityAutoSave.ts` around lines 118 - 145, flushToGit currently proceeds if a draft exists even when the latest live edits fail validation; call validateRef.current?.() at the start of flushToGit (after the early guards and before reading the draft/onFlushRef) and if it reports failure (returns falsy or throws) abort by setting saving state to error (setSaveStatus("error")), setSaveError with the validation message (or a generic message), call onErrorRef.current?.(msg) and return false so we don't call onFlushRef.current with invalid data; reference flushToGit, validateRef, onFlushRef, setSaveStatus, setSaveError, onErrorRef and returning false on validation failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/editor/PropertyDetailPanel.tsx`:
- Around line 476-491: PropertyDetailPanel currently conditionally renders only
the Edit/Save/Cancel controls (isEditing, enterEditMode, saveAndExitEditMode,
cancelEditMode) so the Graph action is missing; to fix, render the Graph
action/button in the same header container outside (or duplicated in both
branches of) the isEditing conditional so it is always present — either create a
dedicated renderGraphAction/GraphButton element and place it next to the
existing buttons inside the div with className "flex shrink-0 items-center
gap-1" or add the Graph JSX to both the editing and non-editing branches so the
layout becomes "[Edit Item] [Graph]" ↔ "[Save] [Cancel] [Graph]" while
preserving saveStatus and button handlers.
In `@lib/hooks/useAutoSave.ts`:
- Around line 144-213: flushToGit can flush a stale draft because it only checks
stored draft existence; before reading the draft inside flushToGit re-check the
current in-memory edit state (editStateRef.current) for validity and return
false if it fails validation. In practice, add a guard near the top of
flushToGit (before calling draftKey/getDraft) that inspects editStateRef.current
(same validation logic used by triggerSave — e.g., labels/comments/required
fields trimmed/non-empty) and if invalid immediately set
flushingRef.current=false (if set), leave save status unchanged or set
appropriate error, and return false so stale drafts are not flushed. Ensure you
reference editStateRef.current and flushToGit when making the change.
In `@lib/hooks/useEntityAutoSave.ts`:
- Around line 118-145: flushToGit currently proceeds if a draft exists even when
the latest live edits fail validation; call validateRef.current?.() at the start
of flushToGit (after the early guards and before reading the draft/onFlushRef)
and if it reports failure (returns falsy or throws) abort by setting saving
state to error (setSaveStatus("error")), setSaveError with the validation
message (or a generic message), call onErrorRef.current?.(msg) and return false
so we don't call onFlushRef.current with invalid data; reference flushToGit,
validateRef, onFlushRef, setSaveStatus, setSaveError, onErrorRef and returning
false on validation failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cbd7bd2-2785-4c9f-8b85-585cb771de41
📒 Files selected for processing (5)
components/editor/ClassDetailPanel.tsxcomponents/editor/IndividualDetailPanel.tsxcomponents/editor/PropertyDetailPanel.tsxlib/hooks/useAutoSave.tslib/hooks/useEntityAutoSave.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- components/editor/IndividualDetailPanel.tsx
- components/editor/ClassDetailPanel.tsx
Prevents flushing a stale draft when triggerSave() aborted due to validation failure (e.g. all labels cleared). Re-checks the current in-memory edit state at the top of flushToGit and returns false if labels are invalid. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Calls validateRef.current() at the top of flushToGit to prevent flushing a stale draft when the current edit state is invalid (e.g. triggerSave aborted on validation). Returns false so the Save button keeps the user in edit mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@damienriehl my first impression when testing the latest changes, I like the "dirty" indicator that shows when you introduce a change to a class, but I couldn't figure out how to "save" the changes. I had to ask Claude to examine the code to explain to me how the "saving" took place, and only then I found out that there is an auto-save when you navigate away from a class. I figure that is a nice feature so that you don't accidentally lose changes, but it is a bit counter-intuitive to not have a save button somewhere, where you can simply trigger the save and see that the state is no longer "dirty". It feels more natural to me to have a save button. |
1 participant
Summary
[Edit Item] [Graph]in view mode becomes[Save] [Cancel] [Graph]in edit modetriggerSave()+flushToGit()then exits edit mode, giving users an explicit commit path instead of relying solely on auto-flush-on-navigateTest plan
Closes #6
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Chores