feat(instances): add Hooks tab to Add Instance modal#140
Conversation
Reuse the existing HooksTab component (in its instance-less view/toggle/ reorder mode) so the Add Instance flow can show a preset's user hooks, their LD_PRELOAD order, and enabled/disabled status before any instance exists. - preset GET returns a `user_hooks` list alongside `enabled_hooks` - AddInstanceModal seeds the default preset's hooks into initial data - AddInstanceForm renders the Hooks tab, seeds order/status from the default (and any loaded) preset, folds hook edits into dirty/modified tracking, and persists hook order when saving a preset - create payload's enabled_hooks now reflects live tab state Bump version to 1.13.11.
There was a problem hiding this comment.
PR Review: Add Hooks Tab to Add Instance Modal
Summary
This PR adds a Hooks tab to the Add Instance modal, reusing the existing HooksTab component in instance-less mode. The backend gains a user_hooks field on the preset detail endpoint. Hook toggle/reorder state is tracked for dirty detection and persisted on preset save/overwrite.
Strengths
HooksTabreuse is clean (AddInstanceForm.jsx:995–1013): passinginstanceId={null}to conditionally hide upload/delete affordances avoids a parallel component.- Dirty state extended correctly (
AddInstanceForm.jsx:442–468,477–497): hooks changes feed into both the unsaved-changes guard and the preset-modified indicator. - Defensive backend code (
preset_api_routes.py:611–651):_read_preset_user_hookshandles missing directory,OSErroronstat, and non-.sofiles without raising. - Good test coverage: backend covers the with-directory and no-directory cases; the frontend test verifies toggle + submit end-to-end.
- Documentation updated in both
api_reference.mdandtechnical.md— theuser_hooksshape and its relationship toenabled_hooksis well-explained.
Issues
Critical (Must Fix)
None identified.
Important (Should Fix)
1. Hook descriptions will silently always be empty
preset_api_routes.py:631–635
descriptions = {
row.file_path: row.description or ''
for row in BinaryMetadata.query.filter_by(
context_type='preset', context_key=preset.name,
).all()
}Then at line 646:
'description': descriptions.get(name, ''),name is a bare filename (e.g. a.so). If BinaryMetadata.file_path stores a relative path (e.g. user-hooks/a.so) as it likely does when metadata is saved for preset hook files, the dict key never matches name and every description silently returns ''. This doesn't crash anything, but descriptions entered by users for preset hooks will never surface in the Add Instance Hooks tab.
Fix: Either normalise the key with os.path.basename(row.file_path) when building the dict, or look up descriptions.get(f'user-hooks/{name}', '') — whichever matches how file_path is actually stored.
2. enabled_hooks is now unconditionally sent on instance create
AddInstanceForm.jsx:872
// Before (removed):
if (loadedPresetEnabledHooksRef.current !== null) {
submitData.enabled_hooks = loadedPresetEnabledHooksRef.current;
}
// After:
submitData.enabled_hooks = enabledHookOrder;Previously enabled_hooks was omitted from the POST /api/instances payload unless the user had explicitly loaded a preset. Now it is always present (defaulting to [] or whatever the default preset's enabled_hooks is). Whether the backend treats absent enabled_hooks identically to enabled_hooks: [] for a new instance determines whether this is a regression. If the create path interprets [] as "clear all hooks" while absent means "inherit from copied preset", users who never touch the Hooks tab now get no hooks loaded even when the preset has them.
Fix: Confirm the backend create handler treats enabled_hooks: [] and absent enabled_hooks identically for new instances — or restore the conditional if they differ.
3. hooksChanged logic is duplicated across two useEffect calls
AddInstanceForm.jsx:442–444 and AddInstanceForm.jsx:477–480
Both effects recompute the same order-comparison inline. If the semantics need to change (e.g. case-insensitive filenames), it will need updating in two places.
Fix: Extract to a useMemo or a small helper:
const hooksChanged = useMemo(() => (
enabledHookOrder.length !== initialEnabledHookOrderRef.current.length ||
enabledHookOrder.some((f, i) => initialEnabledHookOrderRef.current[i] !== f)
), [enabledHookOrder]);Minor (Nice to Have)
4. Stale error message in AddInstanceModal.jsx:63
console.error('Failed to fetch default preset checked_plugins:', presetErr);The catch block now also covers user_hooks / enabled_hooks initialisation, but the error message still says "checked_plugins". Not harmful, but misleading when debugging hook-fetch failures.
5. Fragile setTimeout in the test
AddInstanceForm.test.jsx:547
await new Promise((resolve) => setTimeout(resolve, 350));A 350 ms arbitrary sleep is brittle under slow CI. Using waitFor or act to wait for the state update would be more reliable.
Assessment
Ready to merge? With fixes
Reasoning: The feature is well-structured and the surface area is small, but the description lookup bug (issue 1) means hook descriptions will never display in the Add Instance tab, and the behavioral change in enabled_hooks submission (issue 2) needs a quick backend confirmation to ensure no regression for users who don't use the hooks tab.
- extract duplicated hooks-changed comparison into a useMemo - clarify default-preset fetch error message (now covers hooks too) - replace arbitrary test sleep with waitFor on the reorder handle
Summary
Adds a Hooks tab to the Add Instance modal (it was only present on the Edit Instance Config modal). Reuses the existing
HooksTabcomponent in its instance-less mode — since no instance exists yet, hook files come from the preset instead of a live instance.When a preset is loaded (and for the default preset on open), the tab reflects that preset's configuration: it shows each user hook, its LD_PRELOAD order, and its enabled/disabled status. Toggle and drag-to-reorder are supported; upload/delete/rename are hidden (no instance to mutate).
Changes
ui/routes/preset_api_routes.py—GET /api/presets/<id>now returns auser_hookslist (the.sofiles in the preset'suser-hooks/) alongside theenabled_hooksit already returned.AddInstanceModal.jsx— captures the default preset'suser_hooks/enabled_hooksinto initial data (mirrors how default plugins are already captured).AddInstanceForm.jsx— new Hooks tab; seeds order/status from the default preset on open and from any loaded preset on import; toggle/reorder update theenabled_hookssent on create. Hook edits now feed the form's dirty-tracking and the "(modified)" preset indicator, and "Save Preset" from the add flow persists the hook order (previously dropped).Behavior notes
enabled_hooks; this replaces that one-way passthrough with real tab state, so existing preset behavior is preserved — loading a preset still applies its hooks, and now you can view/adjust them first.Tests
user_hooksfield (present + empty). Full preset suite passes (189).enabled_hooks. Full suite passes (223).api_reference.md,technical.md).