fix: correct generateDynamicNodeCallbacks return type - #2633
Open
camielvs wants to merge 1 commit into
Open
Conversation
`generateDynamicNodeCallbacks` wraps each `NodeCallbacks` entry so the
node & task ids are bound in, which means the values it returns take one
fewer argument than the ones it received. It nonetheless declared its
return type as `NodeCallbacks` — the un-bound shape — so every returned
callback was typed as needing an ids argument it in fact supplies itself.
The mismatch was invisible only because the sole caller
(`createTaskNode`) casts its result through `as Node`.
Return `TaskNodeCallbacks` instead, which is the shape actually
produced. With the types lined up, the reflective `Object.fromEntries`
construction is no longer needed to satisfy the compiler, so build the
object explicitly: this drops the `ExcludeNodeAndTaskId` helper, an
`...args: any[]` spread, and all three casts, and it makes a missing or
misordered argument a compile error rather than something the casts
absorb.
Two side effects, both verified unreachable in production:
- the no-callbacks branch now returns `DEFAULT_TASK_NODE_CALLBACKS`
rather than `{} as NodeCallbacks`, so consumers that gate on callback
truthiness (`TaskDetails/Actions.tsx`) see no-ops instead of
`undefined`. `FlowCanvas` always passes `nodeCallbacks`, so the branch
is not taken.
- the ids object is created once and shared by all seven wrappers
instead of being rebuilt per call. Every `useNodeCallbacks`
implementation only reads `ids.taskId`/`ids.nodeId`; none mutates it.
`TaskNodeCallbacks` is exported for the annotation, and
`CallbackWithIds` is un-exported — it is used only within
`src/types/taskNode.ts`, and Knip fails on an export used solely in its
own file.
Adds unit coverage for the wrapper, which had none. Reverting the type
change alone makes the new test file fail to compile with 8 arity
errors.
Part of B3 of #2626.
🎩 PreviewA preview build has been created at: |
Open
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of B3 of #2626.
The bug
generateDynamicNodeCallbacksbinds the node & task ids into eachNodeCallbacksentry, so what it returns takes one fewer argument than what it received. It declared its return type asNodeCallbacksanyway — the un-bound shape — so every callback it handed back was typed as still needing anidsargument that the wrapper already supplies.Nothing caught this because the only caller,
createTaskNode, launders the result throughas Node.The fix
Return
TaskNodeCallbacks, the shape actually produced. Once the types line up, the reflectiveObject.fromEntriesconstruction is no longer needed to satisfy the compiler, so the object is built explicitly. That removes:ExcludeNodeAndTaskIdhelper type...args: any[]spreadascastsand turns a missing or misordered argument into a compile error instead of something the casts absorb.
Behaviour deltas
Two, both verified unreachable in production:
DEFAULT_TASK_NODE_CALLBACKSinstead of{} as NodeCallbacks, so consumers gating on callback truthiness (TaskDetails/Actions.tsx:64,74,90) now see no-ops rather thanundefinedFlowCanvas.tsx:394-404always passesnodeCallbacks; the branch is never takenidsobject is built once and shared by all seven wrappers rather than rebuilt per calluseNodeCallbacksimplementations only readids.taskId/ids.nodeId; none mutates itTaskNodeCallbacksis exported for the annotation.CallbackWithIdsis un-exported — it's used only insidesrc/types/taskNode.ts, and Knip fails CI on an export consumed solely within its own file.Tests
Adds
generateDynamicNodeCallbacks.test.ts(6 tests) — the wrapper had no coverage. It pins id derivation, id injection ahead of caller args,undefinedtrailing args, laziness, and the no-callbacks branch.Reverting only the type change makes the new test file fail to compile with 8 arity errors, so the test genuinely depends on the fix rather than merely accompanying it.
Verification
typecheck·lint·knip·prettierclean; full suite 192 files / 1972 tests passing.Scope is v1-only:
NodeCallbacksappears in 8 files, all under the v1 canvas. v2's single edge to this area is a type-onlyTaskNodeContextTypeimport.Left deliberately out of scope:
TaskNodeProvider.tsx:51declares a third, structurally different localTaskNodeCallbacks(addssetCollapsed, makes four callbacks optional). Collapsing it is a separate change.🤖 Generated with Claude Code