fix: get_niagara_info reports 0 emitters when called with systemPath#495
Conversation
get_niagara_info and validate_niagara_system run through handleEffectAuthoringAction, which injects ensureDefaultNiagaraAuthoringAssets() defaults into systemPath/assetPath when a field is unset. The native GetNiagaraInfo handler resolves its target as assetPath-first, so a caller passing only systemPath had its empty assetPath overwritten with the throwaway default system and the query reported emitterCount: 0 for a valid asset. Read-only query actions now bypass default-asset injection and only mirror the caller-supplied path across assetPath/systemPath before dispatching.
|
👋 Thanks for your first Pull Request! We love contributions. Please ensure you have signed off your commits and followed the contribution guidelines. |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesNiagara Read-Only Query Path Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/tools/handlers/effect/effect-niagara-actions.ts`:
- Around line 76-83: The readonly Niagara action branch in
effect-niagara-actions.ts is using a generic assetPath-first fallback that can
overwrite a valid systemPath for validate_niagara_system. Update the path
selection logic in the NIAGARA_READONLY_QUERY_ACTIONS handling to use
action-specific precedence, matching the validation rules used by the Niagara
authoring handlers, and only mirror the chosen field into the other one after
selecting the correct target path for the current action. Refer to the
NIAGARA_READONLY_QUERY_ACTIONS block and executeAutomationRequest call to keep
the query routed to the intended asset or system path.
🪄 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
Run ID: 1c22edfd-2316-48df-b607-a4d07aafdeda
📒 Files selected for processing (2)
CHANGELOG.mdsrc/tools/handlers/effect/effect-niagara-actions.ts
| if (NIAGARA_READONLY_QUERY_ACTIONS.has(action)) { | ||
| const queryPath = (mutableArgs.assetPath ?? mutableArgs.systemPath) as string | undefined; | ||
| if (queryPath) { | ||
| mutableArgs.assetPath = queryPath; | ||
| mutableArgs.systemPath = queryPath; | ||
| } | ||
| return executeAutomationRequest(tools, 'manage_niagara_authoring', mutableArgs) as Promise<Record<string, unknown>>; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use action-specific path precedence before mirroring fields.
Line 77 always prefers assetPath; for validate_niagara_system, that can overwrite a valid systemPath when both are provided and differ, causing validation/query against the wrong target.
Suggested fix
if (NIAGARA_READONLY_QUERY_ACTIONS.has(action)) {
- const queryPath = (mutableArgs.assetPath ?? mutableArgs.systemPath) as string | undefined;
+ const assetPath = typeof mutableArgs.assetPath === 'string' ? mutableArgs.assetPath : undefined;
+ const systemPath = typeof mutableArgs.systemPath === 'string' ? mutableArgs.systemPath : undefined;
+ const queryPath =
+ action === 'validate_niagara_system'
+ ? (systemPath ?? assetPath)
+ : (assetPath ?? systemPath);
if (queryPath) {
mutableArgs.assetPath = queryPath;
mutableArgs.systemPath = queryPath;
}
return executeAutomationRequest(tools, 'manage_niagara_authoring', mutableArgs) as Promise<Record<string, unknown>>;
}Based on learnings from src/tools/handlers/niagara/niagara-authoring-handlers.ts:298-306, these two actions validate different required fields (assetPath vs systemPath).
📝 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.
| if (NIAGARA_READONLY_QUERY_ACTIONS.has(action)) { | |
| const queryPath = (mutableArgs.assetPath ?? mutableArgs.systemPath) as string | undefined; | |
| if (queryPath) { | |
| mutableArgs.assetPath = queryPath; | |
| mutableArgs.systemPath = queryPath; | |
| } | |
| return executeAutomationRequest(tools, 'manage_niagara_authoring', mutableArgs) as Promise<Record<string, unknown>>; | |
| } | |
| if (NIAGARA_READONLY_QUERY_ACTIONS.has(action)) { | |
| const assetPath = typeof mutableArgs.assetPath === 'string' ? mutableArgs.assetPath : undefined; | |
| const systemPath = typeof mutableArgs.systemPath === 'string' ? mutableArgs.systemPath : undefined; | |
| const queryPath = | |
| action === 'validate_niagara_system' | |
| ? (systemPath ?? assetPath) | |
| : (assetPath ?? systemPath); | |
| if (queryPath) { | |
| mutableArgs.assetPath = queryPath; | |
| mutableArgs.systemPath = queryPath; | |
| } | |
| return executeAutomationRequest(tools, 'manage_niagara_authoring', mutableArgs) as Promise<Record<string, unknown>>; | |
| } |
🤖 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 `@src/tools/handlers/effect/effect-niagara-actions.ts` around lines 76 - 83,
The readonly Niagara action branch in effect-niagara-actions.ts is using a
generic assetPath-first fallback that can overwrite a valid systemPath for
validate_niagara_system. Update the path selection logic in the
NIAGARA_READONLY_QUERY_ACTIONS handling to use action-specific precedence,
matching the validation rules used by the Niagara authoring handlers, and only
mirror the chosen field into the other one after selecting the correct target
path for the current action. Refer to the NIAGARA_READONLY_QUERY_ACTIONS block
and executeAutomationRequest call to keep the query routed to the intended asset
or system path.
1 participant
Summary
get_niagara_info(andvalidate_niagara_system) can report an empty result for a valid Niagara System when the caller passes the asset viasystemPath. The query returnsemitterCount: 0with an emptyemittersarray even though the system has emitters.Root cause is in the TypeScript layer, not the native bridge.
Changes
handleEffectAuthoringActionforget_niagara_infoandvalidate_niagara_system.ensureDefaultNiagaraAuthoringAssets()default-asset injection; they only mirror the caller-supplied path acrossassetPath/systemPathbefore dispatching.[Unreleased].Root Cause
get_niagara_infoandvalidate_niagara_systemare listed inEFFECT_AUTHORING_ACTIONS, so they run throughhandleEffectAuthoringAction, which is designed for authoring actions (add_*_module, etc.). That handler fills in default assets when a path is unset:The native
GetNiagaraInfohandler resolves its target asAssetPath.IsEmpty() ? SystemPath : AssetPath— i.e.assetPathwins. So when a caller passes onlysystemPath, the emptyassetPathgets overwritten with the freshly-created, empty default system, and the native handler inspects that empty system instead of the requested one.Calling the same action with
assetPathalready worked, which confirms the issue is the default-asset override path rather than the C++ handler.Reproduction
manage_effectwith:{ "action": "get_niagara_info", "systemPath": "/Game/Path/To/YourSystem.YourSystem" }emitterCount: 0, emptyemitters,userParameterCount: 0.assetPathinstead ofsystemPath→ correct result. After fix: bothsystemPathandassetPathreturn the correct emitter list.Type of Change
Testing
npm run build/tsc --noEmitpassnpm run test:smokepassesPre-Merge Checklist