fix(harmony): open publisher/opener dialogs from menu, toolbar and shortcut - #661
fix(harmony): open publisher/opener dialogs from menu, toolbar and shortcut#661dennisweil wants to merge 2 commits into
Conversation
…ortcut The ftrack Open/Publish menu, toolbar and shortcut never opened a dialog: the actions targeted dynamically-assigned launch_<name> callbacks that Harmony's ScriptManager could not resolve, and used a relative "./configure.js" action path Harmony does not resolve, so the click was a silent no-op and no run.dialog event ever reached the standalone process. - configure.js: define top-level launch_open/launch_publish functions (delegating to a generic ftrackLaunchTool that reads the persisted launcher JSON and sends the run-dialog event) and build the action string with the absolute path to configure.js. Remove the now-unused createLaunchers method. - TB_sceneOpened.js / TB_sceneCreated.js: scope __packageFolder__ to the hook function instead of leaking an un-scoped global that forced later configure.js evaluations down the package-load branch. - tcp_rpc.py: guard the run-dialog dispatch so an exception surfaces in the log instead of vanishing inside the Qt readyRead slot, and still reply to Harmony. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bump the version to 26.7.0rc2 and move the menu/dialog-launch fixes into a dedicated rc2 release-notes section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughFramework Harmony release candidate metadata is updated alongside changes to ftrack launcher resolution, reconnect-hook state scoping, and TCP RPC exception logging. ChangesFramework Harmony release
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HarmonyMenu
participant configure_js
participant LauncherJSON
participant StandaloneIntegration
participant TCPRPCClient
HarmonyMenu->>configure_js: invoke absolute launch action
configure_js->>LauncherJSON: read ftrack_launchers_json
configure_js->>StandaloneIntegration: send run-dialog event
StandaloneIntegration->>TCPRPCClient: deliver dialog event
TCPRPCClient->>TCPRPCClient: catch and log callback exception
TCPRPCClient-->>StandaloneIntegration: send empty reply
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage report
This PR does not seem to contain any modification to coverable code. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
projects/framework-harmony/source/ftrack_framework_harmony/utils/tcp_rpc.py (1)
394-411: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the failure-and-acknowledgment contract.
Test that a callback exception is logged and
send_reply(event, {})still executes. No automated test currently covers this Qt socket-slot boundary.🤖 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 `@projects/framework-harmony/source/ftrack_framework_harmony/utils/tcp_rpc.py` around lines 394 - 411, Add regression coverage for the guarded callback path in the TCP RPC readyRead handler: configure on_run_tool_callback to raise, invoke the event-handling flow, and assert logger.exception records the failure while send_reply receives the original event and an empty response. Ensure the test verifies acknowledgment still occurs after the callback exception.projects/framework-harmony/resource/bootstrap/js/TB_sceneCreated.js (1)
86-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment narrates a rejected prior implementation ("un-scoped global") rather than describing current behavior; the actual scoping claim is also slightly inaccurate.
Per path instructions, in-body rationale that contrasts the current approach against a previously-used alternative ("rather than assigning an un-scoped global") reads as residue of the diff history rather than a description of current state. Separately, since
ftrackReconnectHook()is invoked as a plain function call (TB_sceneCreated.jsline 55,TB_sceneOpened.jsline 53) in non-strict Qt Script (ECMAScript-based),thishere resolves to the global object — sothis.__packageFolder__ = packageRootis not meaningfully "scoped to the hook's function scope"; it's the same global property the old unscoped assignment created. The real fix is the explicitdeleteafterward, not thethis.prefix.As per path instructions, "Flag in-body residue of prior drafts" and rejection rationale "scattered through body text" rather than kept in a dedicated decisions doc.
🤖 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 `@projects/framework-harmony/resource/bootstrap/js/TB_sceneCreated.js` around lines 86 - 90, The comments around __packageFolder__ in ftrackReconnectHook should describe the current delete-after-use behavior without narrating the rejected unscoped-global implementation. Remove the historical comparison and inaccurate claim that assigning through this provides function scoping; retain only concise rationale that the property is deleted after the include.Source: Path instructions
projects/framework-harmony/resource/bootstrap/js/configure.js (2)
36-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStatic
launch_<name>functions only cover "open"/"publish"; other launcher names will silently fail.
ftrackLaunchToolreads an arbitrary launcher list fromftrack_launchers_json, but the toolbar/shortcut/menu items built from that same list inhandleIntegrationContextDataCallbackandftrackRebuildMenusgenerate an action string"launch_"+name+" in "+scriptPathfor every launcher entry, whereas onlylaunch_openandlaunch_publishare statically defined here. If the standalone process ever sends a launcher whosenameisn't"open"or"publish", clicking that menu/toolbar item would reference an undefined top-level function — a failure that happens beforeftrackLaunchTool's own try/catch can run, so nothing gets logged viaMessageLog.trace.This matches the intent of the current PR (Open/Publish only), but it's a latent trap for the next launcher added.
♻️ Suggested guard when building actions for unsupported launcher names
for (var idx = 0; idx < this.launchers.length; idx++) { var launcher = this.launchers[idx]; var name = launcher["name"]; var label = launcher["label"]; + if (typeof this[["launch_", name].join("")] !== "function") { + MessageLog.trace( + "[ftrack] WARNING: no launch_" + name + + " function defined in configure.js; skipping UI for it." + ); + continue; + }🤖 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 `@projects/framework-harmony/resource/bootstrap/js/configure.js` around lines 36 - 95, Guard the launcher action construction in handleIntegrationContextDataCallback and ftrackRebuildMenus so only names with defined launch_<name> functions are exposed, or otherwise provide a safe generic dispatch. Preserve the existing open and publish actions, and prevent unsupported launcher names from generating undefined top-level function references before ftrackLaunchTool can run.
362-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAbsolute-path/action construction is duplicated between here and
ftrackRebuildMenus.The
packagePath/scriptPath/actionconstruction at lines 362-369 is byte-for-byte repeated at lines 514-520 insideftrackRebuildMenus. Keeping two copies of the env-var/__packageFolder__fallback logic in sync is exactly the kind of duplication that produced the original relative-path bug this PR fixes — a future change to one copy (e.g. a new fallback order) could easily miss the other.♻️ Suggested extraction of a shared helper
+// Build the "launch_<name> in <absolute configure.js path>" action string +// used by menu items, toolbar buttons, and shortcuts. +function ftrackGetLaunchAction(name) { + var packagePath = System.getenv("FTRACK_HARMONY_PACKAGE_PATH"); + if (!packagePath) { + packagePath = __packageFolder__ || ""; + } + var scriptPath = packagePath + "/ftrack/configure.js"; + return "launch_" + name + " in " + scriptPath; +}Then both call sites reduce to
var action = ftrackGetLaunchAction(name);.Also applies to: 503-530
🤖 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 `@projects/framework-harmony/resource/bootstrap/js/configure.js` around lines 362 - 369, Extract the shared packagePath/scriptPath/action construction into a helper such as ftrackGetLaunchAction(name), preserving the FTRACK_HARMONY_PACKAGE_PATH then __packageFolder__ fallback order and absolute configure.js path. Replace the duplicated construction in the shown launch flow and ftrackRebuildMenus with calls to the helper, keeping each existing action invocation unchanged.
🤖 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 `@projects/framework-harmony/resource/bootstrap/js/TB_sceneCreated.js`:
- Around line 86-93: Ensure __packageFolder__ cleanup in ftrackReconnectHook is
guaranteed when include(packageRoot + "/ftrack/configure.js") throws by wrapping
the assignment and include call in a nested try/finally and deleting
this.__packageFolder__ in that finally block. Preserve the existing outer catch
behavior and successful include flow.
In `@projects/framework-harmony/resource/bootstrap/js/TB_sceneOpened.js`:
- Around line 84-91: Wrap the scoped __packageFolder__ assignment and include
call in TB_sceneOpened.js with try/finally so delete this.__packageFolder__
always executes when include(packageRoot + "/ftrack/configure.js") throws.
Preserve the existing include behavior and cleanup on successful execution.
In `@projects/framework-harmony/source/ftrack_framework_harmony/utils/tcp_rpc.py`:
- Around line 400-410: Update the exception handling around on_run_tool_callback
to avoid re-indexing optional event fields in the logger.exception message.
Reuse safely captured .get() values or log only the guaranteed tool name,
ensuring logging cannot raise another KeyError and the subsequent send_reply
path remains reachable.
---
Nitpick comments:
In `@projects/framework-harmony/resource/bootstrap/js/configure.js`:
- Around line 36-95: Guard the launcher action construction in
handleIntegrationContextDataCallback and ftrackRebuildMenus so only names with
defined launch_<name> functions are exposed, or otherwise provide a safe generic
dispatch. Preserve the existing open and publish actions, and prevent
unsupported launcher names from generating undefined top-level function
references before ftrackLaunchTool can run.
- Around line 362-369: Extract the shared packagePath/scriptPath/action
construction into a helper such as ftrackGetLaunchAction(name), preserving the
FTRACK_HARMONY_PACKAGE_PATH then __packageFolder__ fallback order and absolute
configure.js path. Replace the duplicated construction in the shown launch flow
and ftrackRebuildMenus with calls to the helper, keeping each existing action
invocation unchanged.
In `@projects/framework-harmony/resource/bootstrap/js/TB_sceneCreated.js`:
- Around line 86-90: The comments around __packageFolder__ in
ftrackReconnectHook should describe the current delete-after-use behavior
without narrating the rejected unscoped-global implementation. Remove the
historical comparison and inaccurate claim that assigning through this provides
function scoping; retain only concise rationale that the property is deleted
after the include.
In `@projects/framework-harmony/source/ftrack_framework_harmony/utils/tcp_rpc.py`:
- Around line 394-411: Add regression coverage for the guarded callback path in
the TCP RPC readyRead handler: configure on_run_tool_callback to raise, invoke
the event-handling flow, and assert logger.exception records the failure while
send_reply receives the original event and an empty response. Ensure the test
verifies acknowledgment still occurs after the callback exception.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 631ca13b-7add-46fa-925d-330711c83b12
📒 Files selected for processing (6)
projects/framework-harmony/pyproject.tomlprojects/framework-harmony/release_notes.mdprojects/framework-harmony/resource/bootstrap/js/TB_sceneCreated.jsprojects/framework-harmony/resource/bootstrap/js/TB_sceneOpened.jsprojects/framework-harmony/resource/bootstrap/js/configure.jsprojects/framework-harmony/source/ftrack_framework_harmony/utils/tcp_rpc.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ftrackhq/ftrack(manual)
| // Scope __packageFolder__ to the hook's function scope (via `this`) | ||
| // rather than assigning an un-scoped global: a leaked global would | ||
| // persist and force later configure.js evaluations (e.g. a menu | ||
| // action) down the package-load branch. The include still sees it | ||
| // (JS includes share the caller's scope); delete it right after. | ||
| this.__packageFolder__ = packageRoot; | ||
| include(packageRoot + "/ftrack/configure.js"); | ||
| delete this.__packageFolder__; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
delete this.__packageFolder__ is skipped if include() throws, defeating the leak-prevention goal.
The whole ftrackReconnectHook body is wrapped in a single try { ... } catch (err) { ... } (lines 76-111). If include(packageRoot + "/ftrack/configure.js") throws — e.g. a syntax error or missing dependency inside configure.js — control jumps straight to the outer catch, and the delete this.__packageFolder__; on the next line never runs. Per the comment's own rationale, a leaked __packageFolder__ "would persist and force later configure.js evaluations... down the package-load branch," which is precisely the failure mode this change is meant to avoid.
🔒 Guarantee cleanup with try/finally
- this.__packageFolder__ = packageRoot;
- include(packageRoot + "/ftrack/configure.js");
- delete this.__packageFolder__;
+ this.__packageFolder__ = packageRoot;
+ try {
+ include(packageRoot + "/ftrack/configure.js");
+ } finally {
+ delete this.__packageFolder__;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Scope __packageFolder__ to the hook's function scope (via `this`) | |
| // rather than assigning an un-scoped global: a leaked global would | |
| // persist and force later configure.js evaluations (e.g. a menu | |
| // action) down the package-load branch. The include still sees it | |
| // (JS includes share the caller's scope); delete it right after. | |
| this.__packageFolder__ = packageRoot; | |
| include(packageRoot + "/ftrack/configure.js"); | |
| delete this.__packageFolder__; | |
| // Scope __packageFolder__ to the hook's function scope (via `this`) | |
| // rather than assigning an un-scoped global: a leaked global would | |
| // persist and force later configure.js evaluations (e.g. a menu | |
| // action) down the package-load branch. The include still sees it | |
| // (JS includes share the caller's scope); delete it right after. | |
| this.__packageFolder__ = packageRoot; | |
| try { | |
| include(packageRoot + "/ftrack/configure.js"); | |
| } finally { | |
| delete this.__packageFolder__; | |
| } |
🤖 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 `@projects/framework-harmony/resource/bootstrap/js/TB_sceneCreated.js` around
lines 86 - 93, Ensure __packageFolder__ cleanup in ftrackReconnectHook is
guaranteed when include(packageRoot + "/ftrack/configure.js") throws by wrapping
the assignment and include call in a nested try/finally and deleting
this.__packageFolder__ in that finally block. Preserve the existing outer catch
behavior and successful include flow.
| // Scope __packageFolder__ to the hook's function scope (via `this`) | ||
| // rather than assigning an un-scoped global: a leaked global would | ||
| // persist and force later configure.js evaluations (e.g. a menu | ||
| // action) down the package-load branch. The include still sees it | ||
| // (JS includes share the caller's scope); delete it right after. | ||
| this.__packageFolder__ = packageRoot; | ||
| include(packageRoot + "/ftrack/configure.js"); | ||
| delete this.__packageFolder__; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Same missing try/finally as TB_sceneCreated.js.
Identical pattern: delete this.__packageFolder__; (line 91) is skipped if include(packageRoot + "/ftrack/configure.js") (line 90) throws, so the property can leak on failure. See the consolidated comment for the shared fix.
🤖 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 `@projects/framework-harmony/resource/bootstrap/js/TB_sceneOpened.js` around
lines 84 - 91, Wrap the scoped __packageFolder__ assignment and include call in
TB_sceneOpened.js with try/finally so delete this.__packageFolder__ always
executes when include(packageRoot + "/ftrack/configure.js") throws. Preserve the
existing include behavior and cleanup on successful execution.
| self.on_run_tool_callback( | ||
| event["data"]["name"], | ||
| event["data"]["dialog_name"], | ||
| event["data"]["options"], | ||
| ) | ||
| except Exception: | ||
| self.logger.exception( | ||
| f"on_run_tool_callback failed for tool " | ||
| f"{event['data']['name']} (dialog: " | ||
| f"{event['data']['dialog_name']})" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tcp_rpc.py excerpt ==\n'
sed -n '360,430p' projects/framework-harmony/source/ftrack_framework_harmony/utils/tcp_rpc.py
printf '\n== callback signature ==\n'
sed -n '120,170p' projects/framework-harmony/source/ftrack_framework_harmony/__init__.py
printf '\n== search for send_reply / on_run_tool_callback usages ==\n'
rg -n "send_reply|on_run_tool_callback|run_tool_callback" projects/framework-harmony/source/ftrack_framework_harmony -SRepository: ftrackhq/integrations
Length of output: 6027
Avoid re-indexing optional fields in the exception path. If dialog_name or options are omitted, the KeyError is caught, but the logger.exception f-string reads event["data"]["dialog_name"] again and can raise before send_reply, leaving the peer waiting. Use .get() locals or log only fields that are guaranteed to exist.
🤖 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 `@projects/framework-harmony/source/ftrack_framework_harmony/utils/tcp_rpc.py`
around lines 400 - 410, Update the exception handling around
on_run_tool_callback to avoid re-indexing optional event fields in the
logger.exception message. Reuse safely captured .get() values or log only the
guaranteed tool name, ensuring logging cannot raise another KeyError and the
subsequent send_reply path remains reachable.
1 participant
Resolves: no linked tracking ticket provided
This PR has been tested on :
Overview
Purpose: Fix the Toon Boom Harmony integration so clicking the ftrack Open/Publish menu, toolbar or shortcut actually opens the publisher/opener dialog. Previously the menu entries appeared and the RPC connected, but clicking was a silent no-op — no
run.dialogevent ever reached the standalone process.Scope:
projects/framework-harmonyonly — the Harmony-side JS (configure.js,TB_sceneOpened.js,TB_sceneCreated.js), the standalone RPC transport (utils/tcp_rpc.py), version bump to26.7.0rc2and release notes.Implementation Details
Approach: The menu/toolbar/shortcut actions now target top-level
launch_open/launch_publishfunction declarations inconfigure.js(which Harmony'sScriptManagercan resolve) that delegate to a genericftrackLaunchTool(name)reading the persisted launcher JSON and sending the run-dialog event; the action string uses the absolute path toconfigure.js. TheTB_scene*hooks no longer leak__packageFolder__as an un-scoped global, and the standalone run-dialog dispatch is wrapped so exceptions are logged rather than swallowed by the Qt slot.Reasoning: Two Harmony-specific facts were the root cause:
ScriptManagercannot resolve dynamically-assignedthis["launch_"+name]callbacks (only real top-level declarations), and it does not resolve a relative./configure.jsaction path. A leaked__packageFolder__global also forced laterconfigure.jsevaluations down the package-load branch. Logs proved therun.dialogevent never reached Python, so the break was entirely JS-side.Changes:
configure.js— new top-levelftrackLaunchTool/launch_open/launch_publish, absolute-path action strings, removal of the now-unusedcreateLaunchers;TB_sceneOpened.js/TB_sceneCreated.js— scope__packageFolder__to the hook anddeleteit after the include;tcp_rpc.py— guard +logger.exceptionaround the run-dialog callback, still replying to Harmony.Trade-offs: Adds diagnostic
MessageLog.traceoutput on the JS side (kept intentionally, since this script surface is fragile and otherwise silent). The__packageFolder__-based branch split inconfigure.jsis retained rather than reworked, to keep the change minimal.Testing
Tests Added: None. The existing launch/standalone harness covers the connection handshake, not the menu-click → dialog path; this fix was verified manually against a live Harmony.
Manual Testing: Launch Harmony via Connect (it boots into the
ftrack_bootstrapscene). Click File > Ftrack Open and Ftrack Publish, and the Ftrack toolbar buttons/shortcut — the opener/publisher window opens each time. Open another scene / File > New and confirm the menu still works after the reconnect. The Harmony MessageLog should showlaunch_open invoked/Sending event: {…run.dialog…}and the standalone logDecoding incoming event: {…run.dialog…}followed by the dialog opening.Testing Environment: Verified working on Windows (Harmony Premium 25.2) from the equivalent built plugin. macOS/Linux not yet re-verified; the fix is platform-agnostic (JS scoping / action-path resolution).
Notes for Reviewers
Focus Areas:
configure.jstop-level launch functions and the absolute-path action strings inhandleIntegrationContextDataCallbackandftrackRebuildMenus; the__packageFolder__scoping in the twoTB_scene*hooks.Dependencies: None. This is a JS + standalone-process change, so it requires a Connect-plugin rebuild/deploy (a Python-only rebuild does not refresh the deployed JS).
Known Issues: The
QFSFileEngine::open: No file name specifiedwarnings seen during debugging are unrelated (Harmony's own Qt resource/font lookups) and are not addressed here.Summary by CodeRabbit