Modernization to Swift & SwuiftUI#46
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modernizes ClipMenu by migrating the codebase from legacy Objective-C to Swift, utilizing SwiftUI for the UI and SwiftData for persistence. The changes include a new event-driven clipboard pipeline, global hotkey support, and a JavaScript-based action system. Reviewers identified several improvement opportunities: securing the script engine against code injection by avoiding string interpolation, resolving concurrency violations when accessing the ModelContext, and replacing fragile retry logic in the AppDelegate. Additionally, suggestions were made to use type-safe enums instead of magic numbers and to implement a more robust hashing strategy for image data to prevent collisions.
| let wrapped = """ | ||
| function __wrapper(clipText, clip) { | ||
| try { \(script) } catch(e) { __scriptException = e.toString(); return; } | ||
| } | ||
| """ |
There was a problem hiding this comment.
Using string interpolation to inject the script source into a JavaScript function wrapper is risky. If the script content contains characters that break the JavaScript syntax (like a closing brace }), it will cause a syntax error or potentially allow for code injection. It is safer to pass the script as a variable to the JS context and use eval() or JSValue.call(withArguments:) to execute it.
| let wrapped = """ | |
| function __wrapper(clipText, clip) { | |
| try { \(script) } catch(e) { __scriptException = e.toString(); return; } | |
| } | |
| """ | |
| context.setObject(script, forKeyedSubscript: "__userScript" as NSString) | |
| let wrapped = """ | |
| function __wrapper(clipText, clip) { | |
| try { eval(__userScript) } catch(e) { __scriptException = e.toString(); return; } | |
| } | |
| """ |
| func rootActions() async -> [ActionNode] { | ||
| guard let context else { return [] } | ||
| let descriptor = FetchDescriptor<ActionNode>( | ||
| predicate: #Predicate<ActionNode> { $0.parent == nil }, | ||
| sortBy: [SortDescriptor(\ActionNode.sortIndex)] | ||
| ) | ||
| return (try? context.fetch(descriptor)) ?? [] | ||
| } |
There was a problem hiding this comment.
The ActionService actor is accessing the ModelContext (which is the mainContext from AppRuntime) directly on its own executor. Since mainContext is isolated to the MainActor, this is a concurrency violation. These fetches should be performed on the MainActor or, preferably, ActionService should use its own background ModelContext created from the ModelContainer.
func rootActions() async -> [ActionNode] {
guard let context else { return [] }
return await MainActor.run {
let descriptor = FetchDescriptor<ActionNode>(
predicate: #Predicate<ActionNode> { $0.parent == nil },
sortBy: [SortDescriptor(\ActionNode.sortIndex)]
)
return (try? context.fetch(descriptor)) ?? []
}
}| private func startDataServicesWhenReady(retryCount: Int) { | ||
| guard let modelContext = runtime.modelContainer?.mainContext else { | ||
| guard retryCount > 0 else { return } | ||
| Task { @MainActor in | ||
| try? await Task.sleep(nanoseconds: 100_000_000) | ||
| startDataServicesWhenReady(retryCount: retryCount - 1) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
The retry logic for waiting for the ModelContainer is fragile and likely unnecessary. Since runtime.modelContainer is initialized synchronously in ClipMenuApp.init(), it should be available by the time applicationDidFinishLaunching is called. If there is a concern about initialization timing, consider using a more robust observation mechanism or ensuring the container is ready before the AppDelegate methods are invoked.
| guard let files = entry.filenames, !files.isEmpty else { return } | ||
| let hfsPaths = files.compactMap { posixPath -> String? in | ||
| let url = URL(fileURLWithPath: posixPath) as CFURL | ||
| return CFURLCopyFileSystemPath(url, CFURLPathStyle(rawValue: 1)!) as String? |
There was a problem hiding this comment.
Avoid using magic numbers for CFURLPathStyle. Use the named enum case .cfurlhfsPathStyle for better readability and type safety. Also, the force unwrap ! should be avoided if possible.
| return CFURLCopyFileSystemPath(url, CFURLPathStyle(rawValue: 1)!) as String? | |
| return CFURLCopyFileSystemPath(url, .cfurlhfsPathStyle) as String? |
|
|
||
| if let imageData { | ||
| h ^= imageData.count |
There was a problem hiding this comment.
Using imageData.count as part of the contentHash is a very weak way to differentiate images and will lead to collisions for different images of the same byte size. Consider using a more robust hashing method, such as the hashValue of the Data object (if stability across restarts is not required) or a proper checksum/hash (like SHA256) for stable deduplication.
|
Awesome update! naotaka hasn't been active for many years on GitHub, so I wonder if there is some way to surface this if the PR is never merged? I also want to add in a search/filter function. I'll send you a PR if i get it to work. (I dont know how to code in swift, so perhaps this is something you could do easily) |
|
Oh you can probably and easily do it with Claude code, and I agree on the rebrand, that's what the license says we should do anyway if we continue working on it in my repo. |
|
I spent all week on this And 2 more features I've been trying to get working will come in a follow up PR: I've actually poured so much in to this - also because I've loved this app for so long and wondered what happened to Naotaka. How/Who publishes the new branded version? If you publish, I'd love to be included in the credits, and maybe the charity we run called United Visions. Or if I publish it, I have to credit you, so would love to know what you want it to say. I signed up as an Apple developer just to work on this. Sorry the PR includes taking out your team signing, and setting it up for generic dev building. I'm not really sure how this is supposed to be handled in xcode projects. Names, I was thinking @naotaka if you are around, or if any of Naotaka's friends are around, I'd love to hear your thoughts. and Juan, this is my first bigger open source project, so I hope I did the question about publishing and building in the right spirit. |
|
@juancavallotti @Frazer I just got a notification from apple that with the latest update Rosetta will be depreciated (causing the loss of support for running intel based apps on apple silicon) and thus ClipMenu will be depreciated. I also have been a loyal ClipMenu user for many years, and would like to revive this project. Where are you at with re-publishing it? Did you create a new repo? I would be happy to contribute in brining across apple silicon support so ClipMenu can survive. In regards to naming could call it "Jiffy", because it helps you navigate your clipboard in a jiffy. It has a bit more pizazz to it, and might help with the branding in getting broader support for the tool. |
|
@tommyrharper I do have a published version with the newer stack here https://github.com/juancavallotti/ClipMenu and @Frazer is working through a pull request with featues he added. |
|
Hi everyone — I find it really interesting that several long-time ClipMenu power users seem to be showing up here around the same time. It says a lot about how useful this little app still is after all these years. I’m currently rewriting the app from scratch, trying to stay very faithful to the original feel: native menu bar app, contextual menu, clipboard history, snippets, and keyboard-first workflow, but updated for current macOS. I know the original README asks derivatives not to use the “ClipMenu” name. That said, given that Naotaka Morimoto does not seem to have been active for many years, I wonder if “ClipMenu2” could be a reasonable and respectful name for a continuation/revival — clearly not the original app, but a successor inspired by it. In any case, I’m really happy to see people working on keeping this workflow alive. |
4 participants
Hello!
I have been using this app for many years and it finally broke with MacOSX 26, and I felt it was only fair to modernize it so I can continue using it but I also wanted to contribute it back to you as a thank you for all what you've helped me these last 14 years.