-
Notifications
You must be signed in to change notification settings - Fork 0
Save new file structures #112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughA conditional save operation is added to the FileStructureWindow's initialization logic. If the file structure contains directories, the code now saves the file structure before proceeding with paging setup and entering edit mode. Changes
Sequence DiagramsequenceDiagram
participant Init as FileStructureWindow<br/>Initialization
participant Check as Directory Check
participant Service as FileStructureService
participant UI as UI State
Init->>Check: Has directories?
alt Directories non-null and non-empty
Check->>Service: Save(fileStructure)
Service-->>Check: Saved
end
Check->>UI: Setup Paging
UI->>UI: Enter Edit Mode
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Simplic.FileStructure.UI/FileStructureWindow.xaml.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Simplic.FileStructure.UI/FileStructureWindow.xaml.cs (4)
src/Simplic.FileStructure.Service/FileStructureDocumentPathService.cs (1)
Save(90-172)src/Simplic.FileStructure.UI/ViewModel/FileStructureViewModel.cs (1)
Save(308-312)src/Simplic.FileStructure.Service/FileStructureService.cs (1)
Save(88-91)src/Simplic.FileStructure.Data.DB/FileStructureRepository.cs (1)
Save(137-145)
| if (fileStructure.Directories != null && fileStructure.Directories.Any()) | ||
| { | ||
| fileStructureService.Save(fileStructure); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Saving during initialization without user action is problematic and lacks error handling.
Several concerns with this implementation:
-
Unintended data persistence: Saving automatically during initialization bypasses user confirmation and could persist unintended changes, temporary data, or in-memory modifications.
-
No error handling: Unlike the
OnSavemethod (lines 105-117), this save operation has no try-catch block. If the save fails, it could crash the window during initialization or silently fail. -
Unclear business logic: The condition
fileStructure.Directories != null && fileStructure.Directories.Any()is arbitrary. Why only save structures with directories? If the intent is to persist "new" file structures (per the PR title), a more explicit check (e.g., checking anIsNewflag orId == Guid.Empty) would be clearer. -
Workflow disruption: This breaks the typical "open → modify → save" pattern, making it difficult for users to understand when data is being persisted.
To better understand the intent and potential issues, please run the following verification:
Could you clarify:
- What scenario requires saving during initialization?
- Why not prompt the user or save after they make changes?
- Should this only apply to newly created file structures?
🏁 Script executed:
#!/bin/bash
# Description: Find all callers of FileStructureWindow.Initialize to understand when this save executes
# Search for Initialize method calls on FileStructureWindow
rg -nP --type=cs -C3 'FileStructureWindow.*\.Initialize\s*\('
# Also search for constructors or factory methods that might call Initialize
rg -nP --type=cs -C3 'new\s+FileStructureWindow\s*\('Length of output: 4717
Need to verify the full context around Initialize calls in ApplicationHelper, particularly whether Initialize is called after the fileStructure is created/loaded:
🏁 Script executed:
#!/bin/bash
# Get full method implementations from ApplicationHelper
# Extract NewFileStructureTemplate method (around line 33-50)
echo "=== NewFileStructureTemplate method ==="
sed -n '33,60p' src/Simplic.FileStructure.UI/Helper/ApplicationHelper.cs
echo ""
echo "=== DuplicateFileStructure method ==="
sed -n '77,105p' src/Simplic.FileStructure.UI/Helper/ApplicationHelper.cs
echo ""
echo "=== EditFileStructure method ==="
sed -n '114,145p' src/Simplic.FileStructure.UI/Helper/ApplicationHelper.cs
echo ""
echo "=== OpenFileStructureEditor method ==="
sed -n '153,175p' src/Simplic.FileStructure.UI/Helper/ApplicationHelper.csLength of output: 4744
🏁 Script executed:
#!/bin/bash
# Get the full Initialize and OnSave methods from FileStructureWindow.xaml.cs
echo "=== Full Initialize method (lines 83-99) ==="
sed -n '83,99p' src/Simplic.FileStructure.UI/FileStructureWindow.xaml.cs
echo ""
echo "=== OnSave method (lines 105-117) ==="
sed -n '105,120p' src/Simplic.FileStructure.UI/FileStructureWindow.xaml.csLength of output: 1185
Remove the unconditional save from Initialize; it should only occur when user explicitly saves.
The code currently saves during initialization without user action or error handling, which is incorrect:
-
Premature data persistence: The save at lines 92-95 persists data immediately when Initialize is called, before the window is even set to Edit mode (line 97). For
NewFileStructureTemplate, this saves a brand-new template with only a default placeholder directory—before any user editing. -
Missing error handling: Unlike
OnSave(lines 105-117) which has try-catch, this save has no error handling. If the save fails, it crashes during initialization. -
Breaks intended workflow: All callers (NewFileStructureTemplate, DuplicateFileStructure, EditFileStructure, OpenFileStructureEditor) set the window to Edit mode after Initialize and expect users to edit before saving. The auto-save defeats this.
-
Arbitrary condition: Checking
fileStructure.Directories != null && fileStructure.Directories.Any()is unclear. If the goal is to persist "new" structures (per PR title), use an explicit flag (e.g.,fileStructure.Id == Guid.Empty).
Remove lines 92-95. Data should persist only when user explicitly clicks Save (which calls OnSave).
🤖 Prompt for AI Agents
In src/Simplic.FileStructure.UI/FileStructureWindow.xaml.cs around lines 92-95,
remove the unconditional call to fileStructureService.Save(...) from Initialize:
delete the block that checks Directories and calls Save so initialization no
longer persists data; ensure no other code expects persistence here and rely on
the existing OnSave method (which has try/catch) for all user-driven saves; if
there is a legitimate need to persist newly-created templates automatically,
replace this ad-hoc check with an explicit condition (e.g., fileStructure.Id ==
Guid.Empty) and perform the save only with proper error handling and explicit
intent elsewhere.
Summary by CodeRabbit