feat(file-assoc): OS file handlers and launch-with-file (#664) - #716
Merged
Conversation
Register QtMeshEditor as a 3D model handler on macOS, Windows, and Linux, and route double-click / argv opens through AppLaunchHandler with single-instance focus-and-load. Includes packaging, WinGet FileExtensions, docs, and verification script. Co-authored-by: Cursor <cursoragent@cursor.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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds cross-platform file-association support: Qt single-instance launch routing, macOS plist types, Linux .desktop/MIME/postinst, Windows installer and per-user registration helper, CI verification, WinGet/Homebrew wiring, and documentation/README updates. ChangesFile Association Support: Qt, Windows, macOS, and Linux Integration
Sequence Diagram(s)sequenceDiagram
participant User as OS (double-click)
participant NewProcess as New Process
participant main_cpp as main.cpp
participant AppLaunchHandler
participant QLocalServer
participant MainWindow
User->>NewProcess: open file path
NewProcess->>main_cpp: start with args
main_cpp->>AppLaunchHandler: collectGuiLaunchPaths(args)
AppLaunchHandler->>QLocalServer: tryForwardToRunningInstance(paths)
alt forwarded to running instance
QLocalServer->>AppLaunchHandler: deliver QStringList
AppLaunchHandler->>MainWindow: emit filesRequested(paths)
MainWindow->>MainWindow: openLaunchFiles(paths)
else start server and continue startup
AppLaunchHandler->>AppLaunchHandler: startSingleInstanceServer()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 128a5a827e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+230
to
+234
| const QStringList launchPaths = AppLaunchHandler::collectGuiLaunchPaths(a.arguments()); | ||
| AppLaunchHandler launchHandler; | ||
| if (!launchPaths.isEmpty() && launchHandler.tryForwardToRunningInstance(launchPaths)) | ||
| return 0; | ||
| launchHandler.startSingleInstanceServer(); |
There was a problem hiding this comment.
When the app is started normally and WelcomeDialog::shouldShow() is true, this creates the launch handler and starts the local server before any filesRequested receiver is connected. If the user double-clicks a model (or Finder sends a QFileOpenEvent) while that modal welcome dialog is open, the second instance successfully forwards the path and exits, but the signal is emitted with no MainWindow connected, so the requested file is silently dropped.
Useful? React with 👍 / 👎.
|
|
||
| [Registry] | ||
| ; Animatable / common interchange formats (Alternate handler) | ||
| Root: HKCU; Subkey: "Software\Classes\.fbx"; ValueType: string; ValueName: ""; ValueData: "QtMeshEditor.Model.fbx"; Tasks: fileassoc; Flags: uninsdeletevalue |
There was a problem hiding this comment.
For users who leave the installer’s “Open with handler” task selected, writing the default value under HKCU\Software\Classes\.fbx makes QtMeshEditor the per-user default ProgID for that extension rather than just adding it to Open With; the same pattern is repeated for the other extensions. This can unexpectedly steal existing file associations, and uninstalling with uninsdeletevalue can remove the user's previous default instead of restoring it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/update-winget.sh (1)
27-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when release download fails before hashing.
Line 27 should use HTTP-fail behavior; otherwise a 404/500 body is hashed and the script emits a valid-looking but wrong SHA256.
Suggested patch
-SHA256=$(curl -sL "${ZIP_URL}" | shasum -a 256 | cut -d' ' -f1 | tr 'a-f' 'A-F') +SHA256=$(curl -fsSL "${ZIP_URL}" | shasum -a 256 | cut -d' ' -f1 | tr 'a-f' 'A-F')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/update-winget.sh` around lines 27 - 31, The current SHA256 assignment pipes the download body (including 404/500 HTML) into shasum; change the download step to fail fast on HTTP errors by using curl's fail option (or by checking curl's exit status before computing SHA256) so that "${ZIP_URL}" returns non-zero on HTTP errors and the script exits early; update the SHA256 assignment and/or add an explicit curl check referencing the ZIP_URL and SHA256 variables so the script prints the existing error message and exits if the download failed.
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
501-510: ⚡ Quick winConsider verifying Inno Setup installation before invoking ISCC.
The step installs Inno Setup via Chocolatey but doesn't verify success before running the compiler. If the installation fails silently, line 510 would produce a cryptic "command not found" error.
🛡️ Add installation check
run: | choco install innosetup -y --no-progress + $isccPath = "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe" + if (-not (Test-Path $isccPath)) { + Write-Error "Inno Setup installation failed or ISCC.exe not found at $isccPath" + exit 1 + } $version = "${{ github.ref_name }}".TrimStart("v") $iss = Get-Content "${{github.workspace}}/packaging/windows/QtMeshEditor.iss" -Raw $iss = $iss.Replace("`@PROJECT_VERSION`@", $version) Set-Content -Path "${{github.workspace}}/packaging/windows/QtMeshEditor.build.iss" -Value $iss - & "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe" "${{github.workspace}}/packaging/windows/QtMeshEditor.build.iss" + & $isccPath "${{github.workspace}}/packaging/windows/QtMeshEditor.build.iss"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/deploy.yml around lines 501 - 510, The workflow step installs Inno Setup with "choco install innosetup" but never verifies the install before calling ISCC.exe; update the step to check the install result (e.g., inspect $LASTEXITCODE or test the ISCC.exe path at "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe") after running choco, log a clear error via Write-Error if the installer is missing or choco failed, and exit the script (non-zero) so the job fails fast instead of producing a cryptic "command not found" when calling ISCC.exe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verify-file-associations.sh`:
- Line 12: The verifier uses grep -q "$needle" "$file" which treats $needle as a
regex; change it to use fixed-string matching by calling grep with the
--fixed-strings (or -F) flag and include -- before operands to protect needles
that start with -, e.g. replace the grep invocation that references the
variables needle and file with a fixed-string, quiet grep (e.g. grep -F -q --
"$needle" "$file") so the check is literal and deterministic.
In `@src/AppLaunchHandler.cpp`:
- Around line 176-189: Add a Sentry breadcrumb in
AppLaunchHandler::handleIncomingPaths before emitting filesRequested: when
accepted is not empty call SentryReporter::addBreadcrumb("file.import",
<message>) and include meaningful context (e.g., number of files or the joined
accepted paths) then emit filesRequested(accepted); place the call immediately
before the emit so each user file-open action is tracked.
- Around line 191-202: AppLaunchHandler::eventFilter currently handles macOS
QFileOpenEvent but doesn't record a Sentry breadcrumb; before calling
handleIncomingPaths (and after extracting the absolute path via QFileInfo), add
a Sentry breadcrumb describing the FileOpen action (include event type
"FileOpen" and the file path) so file-open user actions are tracked; ensure the
breadcrumb is added only when path is non-empty and isImportableMeshPath(path)
is true, then proceed to call handleIncomingPaths and return true as before.
In `@website/src/DocsApp.jsx`:
- Around line 217-223: The "Supported extensions" paragraph (h3 with class
s.subsection and following p with class s.para containing multiple <Code>
entries) currently lists many formats but overstates Windows file-association
behavior; update the text to either (A) restrict the listed extensions to only
those registered for double-click on Windows, or (B) relabel the paragraph to
"App import support" and add a new, explicit per-platform subsection (e.g.,
"Windows file associations") that lists the exact extensions registered on
Windows. Adjust the copy under the h3 (and any related <Code> entries)
accordingly so UI truthfully reflects platform-specific registration scope.
---
Outside diff comments:
In `@scripts/update-winget.sh`:
- Around line 27-31: The current SHA256 assignment pipes the download body
(including 404/500 HTML) into shasum; change the download step to fail fast on
HTTP errors by using curl's fail option (or by checking curl's exit status
before computing SHA256) so that "${ZIP_URL}" returns non-zero on HTTP errors
and the script exits early; update the SHA256 assignment and/or add an explicit
curl check referencing the ZIP_URL and SHA256 variables so the script prints the
existing error message and exits if the download failed.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 501-510: The workflow step installs Inno Setup with "choco install
innosetup" but never verifies the install before calling ISCC.exe; update the
step to check the install result (e.g., inspect $LASTEXITCODE or test the
ISCC.exe path at "${env:ProgramFiles(x86)}\Inno Setup 6\ISCC.exe") after running
choco, log a clear error via Write-Error if the installer is missing or choco
failed, and exit the script (non-zero) so the job fails fast instead of
producing a cryptic "command not found" when calling ISCC.exe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1780a39f-3db4-4e26-844a-fd4171a989ac
📒 Files selected for processing (20)
.github/workflows/deploy.ymlREADME.mdpackaging/linux/DEBIAN-postinstpackaging/linux/qtmesheditor-mimetypes.xmlpackaging/linux/qtmesheditor.desktoppackaging/macos/homebrew-cask-postflight.rbpackaging/windows/QtMeshEditor.issscripts/register-windows-file-associations.ps1scripts/update-winget.shscripts/verify-file-associations.shsnap/gui/qtmesheditor.desktopsrc/AppLaunchHandler.cppsrc/AppLaunchHandler.hsrc/AppLaunchHandler_test.cppsrc/CMakeLists.txtsrc/Info.plist.insrc/main.cppsrc/mainwindow.cppsrc/mainwindow.hwebsite/src/DocsApp.jsx
👮 Files not reviewed due to content moderation or server errors (7)
- src/main.cpp
- src/mainwindow.cpp
- src/mainwindow.h
- src/Info.plist.in
- packaging/macos/homebrew-cask-postflight.rb
- packaging/windows/QtMeshEditor.iss
- scripts/register-windows-file-associations.ps1
Queue launch paths during the welcome dialog, register Windows handlers via OpenWithProgids instead of replacing defaults, add Sentry breadcrumbs, tighten docs to per-platform registration scope, and harden CI/scripts. Co-authored-by: Cursor <cursoragent@cursor.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 (1)
scripts/register-windows-file-associations.ps1 (1)
8-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault
-BinDirpoints to the wrong directory in the shipped portable layout.When this script is bundled under
bin/scripts, Line 8 resolves tobin/bin, so Line 13 fails unless users manually pass-BinDir. That breaks the default invocation path for portable usage.Suggested fix
-param( - [string]$BinDir = (Join-Path $PSScriptRoot "..\bin") -) +param( + [string]$BinDir +) + +if (-not $BinDir) { + $portableBin = Join-Path $PSScriptRoot ".." + $repoBin = Join-Path $PSScriptRoot "..\bin" + $BinDir = if (Test-Path (Join-Path $portableBin "QtMeshEditor.exe")) { $portableBin } else { $repoBin } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/register-windows-file-associations.ps1` around lines 8 - 15, The default $BinDir calculation produces "bin/bin" when the script sits in bin/scripts; update how $BinDir is derived so it points to the shipped portable layout's real bin directory: compute $BinDir from the script's parent directory (use Split-Path $PSScriptRoot -Parent and then Join-Path that parent with "bin") and add a simple fallback check that if the constructed $exe (Join-Path $BinDir "QtMeshEditor.exe") doesn't exist, also try the sibling bin under $PSScriptRoot itself before erroring; keep references to $BinDir, $PSScriptRoot, $exe and the Test-Path/Write-Error logic but adjust the initial $BinDir assignment and add the fallback probe.
🧹 Nitpick comments (1)
scripts/register-windows-file-associations.ps1 (1)
33-37: ⚡ Quick winAlign ProgID format with installer to keep one Windows association contract.
Line 33 generates
QtMeshEditor.Modelfbx, while the installer usesQtMeshEditor.Model.fbx. Keeping these different can create duplicate handlers between portable and installed flows.Suggested fix
- $progId = "QtMeshEditor.Model$($ext.Replace('.', ''))" + $progId = "QtMeshEditor.Model.$($ext.TrimStart('.'))"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/register-windows-file-associations.ps1` around lines 33 - 37, The ProgID construction uses $progId = "QtMeshEditor.Model$($ext.Replace('.', ''))" which removes the dot and produces "QtMeshEditor.Modelfbx"; change the assignment to preserve the dot so the ProgID matches the installer ("QtMeshEditor.Model.fbx") — e.g. build $progId from $ext directly or ensure a leading dot is inserted (use "QtMeshEditor.Model$ext" or "QtMeshEditor.Model.$($ext.TrimStart('.'))") so $progId aligns with the installer and prevents duplicate handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/register-windows-file-associations.ps1`:
- Around line 8-15: The default $BinDir calculation produces "bin/bin" when the
script sits in bin/scripts; update how $BinDir is derived so it points to the
shipped portable layout's real bin directory: compute $BinDir from the script's
parent directory (use Split-Path $PSScriptRoot -Parent and then Join-Path that
parent with "bin") and add a simple fallback check that if the constructed $exe
(Join-Path $BinDir "QtMeshEditor.exe") doesn't exist, also try the sibling bin
under $PSScriptRoot itself before erroring; keep references to $BinDir,
$PSScriptRoot, $exe and the Test-Path/Write-Error logic but adjust the initial
$BinDir assignment and add the fallback probe.
---
Nitpick comments:
In `@scripts/register-windows-file-associations.ps1`:
- Around line 33-37: The ProgID construction uses $progId =
"QtMeshEditor.Model$($ext.Replace('.', ''))" which removes the dot and produces
"QtMeshEditor.Modelfbx"; change the assignment to preserve the dot so the ProgID
matches the installer ("QtMeshEditor.Model.fbx") — e.g. build $progId from $ext
directly or ensure a leading dot is inserted (use "QtMeshEditor.Model$ext" or
"QtMeshEditor.Model.$($ext.TrimStart('.'))") so $progId aligns with the
installer and prevents duplicate handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eeb4e6f4-e547-4ffe-8754-25bab7c7beec
📒 Files selected for processing (8)
.github/workflows/deploy.ymlpackaging/windows/QtMeshEditor.issscripts/register-windows-file-associations.ps1scripts/update-winget.shscripts/verify-file-associations.shsrc/AppLaunchHandler.cppsrc/main.cppwebsite/src/DocsApp.jsx
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/update-winget.sh
- scripts/verify-file-associations.sh
- src/main.cpp
- website/src/DocsApp.jsx
- src/AppLaunchHandler.cpp
- packaging/windows/QtMeshEditor.iss
- .github/workflows/deploy.yml
Replace the heredoc in the Homebrew cask update step with echo lines so YAML parses correctly, and align the portable registration script with the Inno Setup ProgID format and bin layout. Co-authored-by: Cursor <cursoragent@cursor.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AppLaunchHandler: parses GUI launch paths fromargv, handles macOSQFileOpenEvent, single-instance viaQLocalServer/QLocalSocket, andMainWindow::openLaunchFiles()withapp.launch.file_openSentry breadcrumbs. CLI (qtmesh info …) unchanged.CFBundleDocumentTypes+UTExportedTypeDeclarationsfor.mesh,.rsd,.tmdand common interchange formats; Homebrew cask postflight runslsregister.HKCUhandlers; portable ZIP bundlesregister-windows-file-associations.ps1; WinGet manifest gainsFileExtensions..desktop+ XDG MIME XML in.debwithpostinst(update-mime-database/update-desktop-database); snap desktop updated withMimeType=+%F.scripts/verify-file-associations.shin CI; README + website docs.Closes #664 (child issues #665–#669).
Test plan
./build_local/bin/UnitTests --gtest_filter="AppLaunchHandler*"(7/7 pass)./scripts/verify-file-associations.sh.fbx→ app opens with modelregister-windows-file-associations.ps1→ Explorer Open Withxdg-open model.fbx→ QtMeshEditor in Open With listMade with Cursor
Summary by CodeRabbit
New Features
Packaging
Tests
Documentation