Fix/promptid selectors #48
Conversation
This fixes bugs related to the lack of a promptId in certain contexts. The issue was caused by faulty selectors that assumed promptIndex (not ID!) was being passed into the stage component as a prop. I removed this during refactoring as it would cause the stage to re-render when the prompt changed (so was incorrect). The new implementation gets the promptId from the session, via the promptIndex. This is implemented within the `getPromptIndex` session selector.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…tacollective/Fresco into fix/promptid-selectors
| export const getPromptId = createSelector( | ||
| getPrompts, | ||
| getPromptIndex, | ||
| (prompts, promptIndex) => prompts[promptIndex].id, | ||
| ) | ||
|
|
There was a problem hiding this comment.
New selector, which fetches the prompt ID via the promptIndex property in the session state.
| export const getNetworkNodesForPrompt = createSelector( | ||
| getNetworkNodesForType, | ||
| getPropPromptId, | ||
| getPromptId, |
There was a problem hiding this comment.
Replace the old selector with the new one - this selector was previously also failing without us realizing.
| @@ -133,7 +140,7 @@ export const makeNetworkNodesForPrompt = () => getNetworkNodesForPrompt; | |||
| */ | |||
|
|
|||
| export const getNetworkNodesForOtherPrompts = createSelector( | |||
| getNetworkNodesForType, getPropPromptId, | |||
| getNetworkNodesForType, getPromptId, | |||
There was a problem hiding this comment.
As above, update to use the session state selector rather than looking for prompt ID in props.
| const getIDs = createSelector( | ||
| propStageId, | ||
| getPromptId, | ||
| (stageId, promptId) => { | ||
| return { | ||
| stageId, | ||
| promptId, | ||
| }; | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
A "helper" selector that just gets the stage and prompt ID and returns them in an object.
This replaces the one I removed from the props selectors file, and puts it in a more appropriate location given that it now depends on state rather than props. I could also have put this in the interfaces selector file, and may do so if it turns out other interfaces use it.
This is in effect what fixes this issue.
| getIDs, | ||
| ({ type }, { stageId, promptId }) => { | ||
| return { | ||
| type, | ||
| stageId, | ||
| promptId, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
This is the actual "fix" (although the real fix is in the new implementation of the getIDs selector, so as not to use props).
I also changed the style a little to remove the spread operator, which is something I want us to do more broadly. Spread operators can make it difficult to debug problems because it isn't clear which data they contain.
| @@ -22,7 +22,6 @@ export const getAdditionalAttributes = (stage, prompt) => { | |||
| export const propStage = (_, props) => props?.stage ?? null; | |||
| export const propPrompt = (_, props) => props?.prompt ?? null; | |||
| export const propStageId = (_, props) => props?.stage?.id ?? null; | |||
| export const getPropPromptId = (_, props) => props?.prompt?.id ?? null; | |||
There was a problem hiding this comment.
I removed this entirely because (1) it is broken now that prompt isn't consistently passed to props, and (2) I wanted to force myself to find other instances where this broken selector was used and fix them.
| // Returns current stage and prompt ID | ||
| export const getIds = createSelector( | ||
| propStageId, getPropPromptId, | ||
| (stageId, promptId) => ({ stageId, promptId }), | ||
| ); |
There was a problem hiding this comment.
This selector was replaced above, with a new implementation that uses session state.
| it('makeGetIds()', () => { | ||
| const selected = Interface.makeGetIds(); | ||
| expect(selected(mockState, mockProps)).toEqual({ | ||
| stageId: mockStage.id, | ||
| promptId: mockPrompt.id, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Removed this test because I also deleted the underlying selector. See below.
| stageId, | ||
| promptId, | ||
| }; | ||
| }, | ||
| ) | ||
|
|
||
| export const makeGetPromptNodeModelData = () => getPromptModelData; |
There was a problem hiding this comment.
I didn't understand the purpose of this function because it's not being called in anywhere
This fixes bugs related to the lack of a promptId in certain contexts.
The issue was caused by faulty selectors that assumed promptIndex (note: not the same as prompt ID!) was being passed into the stage component as a prop. I removed this during refactoring as it would cause the stage to re-render when the prompt changed (so was incorrect).
The new implementation gets the promptId from the session, via the promptIndex. This is implemented within the
getPromptIndexsession selector.Ultimately, the issue was manifesting in the
getPromptNodeModelDataselector, which is used to determine the metadata that nodes should have when they are created or added to a given stage/prompt combination. However it was also causing several other selectors to return incorrect data.