feat(properties): edit task properties from list, task code cleanup

[sedson]

No description provided.

[github-actions]

Preview: https://seamus-edit-props-from-soup-vlj6gq.preview.macro.com/app (c3cf318)

[claude]

Code Review

Found 1 high-confidence issue:

SolidJS Hook Rule Violation

File: js/app/packages/core/component/Properties/hooks/usePropertyEntityDisplay.tsx
Lines: 72-104

This code violates SolidJS hook rules by calling hooks conditionally inside functions that are re-evaluated on each render.

Problems:

1. Lines 72-80 (previewManager): Calls useItemPreview() inside a function that gets re-evaluated whenever entityType() changes. Each re-evaluation creates new effects and subscriptions without cleaning up old ones.

2. Lines 84-89 (channelNameManager): Calls useChannelName() conditionally inside a re-evaluated function, creating new memos on each call.

3. Lines 101-104 (nameFn): Calls useDisplayName() inside a match expression, creating new hooks during reactive evaluations.

In SolidJS, hooks must be called at the top level during component/hook initialization, not inside functions that get re-evaluated. This causes memory leaks, multiple subscriptions, and unpredictable behavior.

Comparison with old code:

The old code properly called hooks at the top level:

const [preview] = useItemPreview({
  id: needsPreview() ? entityId() : '',
  type: needsPreview() ? getPreviewType() : undefined,
});

const channelName = useChannelName(
  entityType().toUpperCase() === 'CHANNEL' ? entityId() : '',
  'Unknown Channel'
);

Recommended fix:

Call all hooks unconditionally at the top level with reactive parameters:

// Call hooks at top level with reactive parameters
const [preview] = useItemPreview({
  id: () => isPreviewable(entityType()) ? entityId() : '',
  type: previewType,
});

const channelName = useChannelName(
  () => previewType() === 'channel' ? entityId() : '',
  'Channel'
);

const [userName] = useDisplayName(
  () => entityType() === 'USER' ? tryMacroId(entityId()) : undefined
);

// Then use these in reactive contexts
const name = createMemo(() => {
  return match(entityType())
    .with('USER', () => userName())
    .with('CHANNEL', () => channelName() ?? 'Channel')
    // ...
});

This ensures hooks are only instantiated once during initialization, following SolidJS best practices.

---

Review Summary:

• ✅ No CLAUDE.md violations found
• ❌ 1 high-confidence bug found (SolidJS hook violation)

[dev-rb]

Looks good overall, just a few nits

[synoet]

Very Nice! One small note for onRefresh.

[synoet]

We shouldn't need to manually trigger refreshes everywhere. The mutation should optimistically update and invalidate. And since everything is using the solid queries they should just reactively update.

[synoet]

Same note about onRefresh

[dev-rb]

This is very complex and seems unnecessary. I think the previous change with the memo was fine but just required small changes. I think Claude thinks this is React 🤦

[synoet]

Yeah, this is cope.

[sedson]

yeah fair enough. the memo is not enough to smartly dispose the computations inside the usePreview() hook though right? like the "old" usePreview will still just be disposed with the lifecycle of the calling component?