GitHub-21 Fix framework lesson navigation to preserve user-created files#28
GitHub-21 Fix framework lesson navigation to preserve user-created files#28meanmail merged 6 commits intohyperskill:mainfrom
Conversation
- Read all files from task directory instead of only template files when saving snapshots - Add logic to preserve user files when navigating between solved tasks in same project - Implement getAllFilesFromTaskDir() to capture user-created files not in template - Use runReadAction for safe document access
meanmail
left a comment
There was a problem hiding this comment.
Summary
The core fix — replacing getTaskStateFromFiles(templateFiles.keys, taskDir) with getAllFilesFromTaskDir(taskDir, currentTask) in step 1 — correctly addresses the root cause from issue #21: user-created files were invisible to the navigation logic because only template file keys were read from disk.
However, there are several issues that need to be addressed before merging.
Key Issues
1. Target snapshot (step 10) doesn't capture user files — user files lost on round-trip navigation
After forward navigation, user files are correctly preserved on disk by calculateFirstVisitChanges. But step 10 (line ~500) saves the target snapshot using targetTask.allFiles.keys, which only reads template files. This means:
- Navigate forward from task1 → task2: user files preserved on disk ✅
- Navigate back to task1: task2's snapshot was saved WITHOUT user files
- Navigate forward to task2 again: loads snapshot from storage → user files are gone
Fix: use getAllFilesFromTaskDir(taskDir, targetTask) in step 10 instead of getTaskStateFromFiles(userFileKeys, taskDir).
2. getTaskState() not updated — two tests likely fail
getTaskState() (line 244) still uses task.allFiles and getUserChangesFromFiles, which only reads template file keys from disk. User-created files won't appear in the result. The tests test getAllFilesFromTaskDir captures user-created files and test getTaskState includes user files both call getTaskState() and assert on user-created files — they should fail.
3. shouldPreserveUserFiles branch ordering and reachability
The shouldPreserveUserFiles branch is placed after needsMerge and first-visit in the when block. For the primary bug scenario (first forward navigation from solved task), the first-visit branch catches it. For repeat navigation where needsMerge is true, the merge branch catches it. shouldPreserveUserFiles is only reached in the narrow case of revisiting a task where current is an ancestor of target. While the branch IS needed for that case, it would be unnecessary if step 10 correctly saved user files to the target snapshot.
...gin/hs-core/src/org/hyperskill/academy/learning/framework/impl/FrameworkLessonManagerImpl.kt
Outdated
Show resolved
Hide resolved
...gin/hs-core/src/org/hyperskill/academy/learning/framework/impl/FrameworkLessonManagerImpl.kt
Outdated
Show resolved
Hide resolved
...c/org/hyperskill/academy/learning/actions/navigate/FrameworkLessonUserFilesNavigationTest.kt
Show resolved
Hide resolved
meanmail
left a comment
There was a problem hiding this comment.
All previous comments addressed. The fix is correct — reading ALL files from disk (via getAllFilesFromTaskDir) instead of only template keys naturally preserves user-created files through the existing propagation/merge logic. Two minor nits below.
| import org.hyperskill.academy.learning.courseFormat.CheckStatus | ||
| import org.hyperskill.academy.learning.courseFormat.FrameworkLesson | ||
| import org.hyperskill.academy.learning.courseFormat.TaskFile | ||
| import org.hyperskill.academy.learning.courseFormat.hyperskill.HyperskillCourse |
There was a problem hiding this comment.
Nit: Unused import — leftover from the removed shouldPreserveUserFiles logic.
| * | ||
| * The fix: Use getAllFilesFromTaskDir() to read ALL files from disk, including user-created ones. | ||
| * Additionally, when navigating forward from a solved task in the same project lesson, | ||
| * preserve user files by treating it like a first visit (add only new template files). |
There was a problem hiding this comment.
Nit: Stale javadoc — lines 25-26 describe the removed shouldPreserveUserFiles logic. The actual fix is simpler: just reading all files from disk in all relevant places. Suggest updating to:
* The fix: Use getAllFilesFromTaskDir() to read ALL files from disk, including user-created ones,
* in all places that read task state (navigation, snapshots, getTaskState API).
Uh oh!
There was an error while loading. Please reload this page.