Skip to content

feat(instances): add Hooks tab to Add Instance modal#140

Merged
dngrtech merged 3 commits into
mainfrom
feat/add-instance-hooks-tab
Jul 8, 2026
Merged

feat(instances): add Hooks tab to Add Instance modal#140
dngrtech merged 3 commits into
mainfrom
feat/add-instance-hooks-tab

Conversation

@dngrtech

@dngrtech dngrtech commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

Adds a Hooks tab to the Add Instance modal (it was only present on the Edit Instance Config modal). Reuses the existing HooksTab component 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.pyGET /api/presets/<id> now returns a user_hooks list (the .so files in the preset's user-hooks/) alongside the enabled_hooks it already returned.
  • AddInstanceModal.jsx — captures the default preset's user_hooks/enabled_hooks into 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 the enabled_hooks sent 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

  • The create payload already carried a preset's 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.
  • Fresh instances now inherit the default preset's enabled hooks (previously they got none), consistent with how default plugins are already pre-checked.

Tests

  • Backend: 2 new tests for the preset user_hooks field (present + empty). Full preset suite passes (189).
  • Frontend: new integration test asserting the Hooks tab reflects preset order/status and submits enabled_hooks. Full suite passes (223).
  • Lint clean.
  • Docs updated (api_reference.md, technical.md).

rage added 2 commits July 7, 2026 22:12
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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • HooksTab reuse is clean (AddInstanceForm.jsx:995–1013): passing instanceId={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_hooks handles missing directory, OSError on stat, and non-.so files 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.md and technical.md — the user_hooks shape and its relationship to enabled_hooks is 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
@dngrtech dngrtech merged commit 30bd92f into main Jul 8, 2026
@dngrtech dngrtech deleted the feat/add-instance-hooks-tab branch July 8, 2026 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant