Conversation
There was a problem hiding this comment.
Code Review
This pull request adds functionality to edit and open skill files through the web interface. Key changes include new backend handlers for saving and opening files with security validations, and a comprehensive frontend update featuring a rich markdown editor, frontmatter reference guide, and detailed content statistics. Review feedback identified a compilation error on Windows due to a non-existent function call and recommended removing a global mutex lock during disk I/O to prevent performance bottlenecks.
| import "golang.org/x/sys/windows" | ||
|
|
||
| func replaceFile(tempPath, fullPath string) error { | ||
| return windows.Rename(tempPath, fullPath) |
There was a problem hiding this comment.
The function windows.Rename does not exist in the golang.org/x/sys/windows package, which will cause a compilation error on Windows. Additionally, Go's standard os.Rename already implements atomic replacement on Windows using the MoveFileEx syscall with the MOVEFILE_REPLACE_EXISTING flag. Unless you need specific flags not provided by the standard library, you should use os.Rename here as well.
| import "golang.org/x/sys/windows" | |
| func replaceFile(tempPath, fullPath string) error { | |
| return windows.Rename(tempPath, fullPath) | |
| import "os" | |
| func replaceFile(tempPath, fullPath string) error { | |
| return os.Rename(tempPath, fullPath) |
| s.mu.Lock() | ||
| filename, err := writeSkillFile(d.SourcePath, fp, []byte(*req.Content)) | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
Holding the global server mutex s.mu during disk I/O in writeSkillFile can lead to performance issues, as it blocks all other server operations (including read-only API requests) until the write completes. Since writeSkillFile operates on a specific file path and does not access or modify any shared fields of the Server struct, this lock is unnecessary. The operating system's file system operations are sufficient to handle concurrent access at this level.
| s.mu.Lock() | |
| filename, err := writeSkillFile(d.SourcePath, fp, []byte(*req.Content)) | |
| s.mu.Unlock() | |
| filename, err := writeSkillFile(d.SourcePath, fp, []byte(*req.Content)) |
6abb359 to
6b74430
Compare
|
@runkids finished this today! |
|
Hey @salmonumbrella — watched your walkthrough. Inline markdown editing is something I've been thinking about, so the direction is interesting and I appreciate you exploring it. A few notes: at 14K lines across 54 files, this is more than I can review in one pass. I also noticed some files that were renamed in v0.19 ( I won't be merging this directly, but I'll definitely use it as a reference when I pick up this feature. Thanks for the time you put into this! |
39e5275 to
e0f946a
Compare
|
Glad you like it! I hope this makes it into v0.20.0, I'd love to be able to edit skills this way. |
Summary
skillshare ui$ARGUMENTSin SKILL.mdediting-markdown-runkids.mp4
Test Plan