feat(headless): expose logRecommendationOpen action #902
Conversation
samisayegh
requested review from
btaillon-coveo,
nbuissoncoveo,
olamothe and
ThibodeauJF
as code owners
|
Pull Request Report PR Title ✅ Title follows the conventional commit spec. Bundle Size
|
packages/headless/package.json
Outdated
| @@ -35,7 +35,7 @@ | |||
| "@reduxjs/toolkit": "^1.5.0", | |||
There was a problem hiding this comment.
Not sure what happened to the package-lock. If you try installing the latest analytics package, is the diff large?
I am using node 16.2.0, npm 7.13.0
There was a problem hiding this comment.
There's definitely something wonky with the package locks, I think they need to be updated more often
| const resultIndex = | ||
| state.recommendation?.recommendations.findIndex( | ||
| ({uniqueId}) => result.uniqueId === uniqueId | ||
| ) || 0; |
There was a problem hiding this comment.
Question on this ternary, similar to the one on line 120:
- If the
recommendationstate property does not exist, the index is0, and the document position is1(line 149). - If the
recommendationstate exists, but the result is not found in the array, the index is-1and document position is0.
I feel it's weird to send two different values for the same condition of "result not found".
However, I'm not sure on what the correct value for documentPosition should be, 0 or 1?
There was a problem hiding this comment.
-
Those documents not being found * should * not happen, since it's not really logical to be able to click on a document that does not exist. But.. 🤷
-
It's there as a fallback/artefact of the type system being unable to assure us that a given value will really be in the array, (which is normal, I guess 🤔 ) Another approach would be to just throw an error and not log anything. Which should happen upstream anyway, not in this function, which is pretty deep in the codebase.
-
For now I would suggest that
indexshould be 0, anddocumentPosition-1 when it's not found. Which most likely means it's going to be rejected by UA (I would assume...).
In any case, I guess just keep in mind that no matter what, if we are unable to find the given result unique ID in the list of results, there's not much we can do to make the UA payload "correct", it's most likely going to fail in one manner or the other. And it being 0, or -1, is a small detail versus the fact that there's some code upstream that is completely wrong when calling this function.
There was a problem hiding this comment.
Alright, I don't feel this is important enough to solve right now. It's definitely niche, but I still think it would be good to addressed, whether through a value, or some kind of feedback to the devs. I've created a jira to revisit.
| {recommendation.title} | ||
| </ResultLink> | ||
| </a> |
There was a problem hiding this comment.
These samples will eventually leverage a buildInteractiveRecommendation controller.
samisayegh
changed the title
| const resultIndex = | ||
| state.recommendation?.recommendations.findIndex( | ||
| ({uniqueId}) => result.uniqueId === uniqueId | ||
| ) || 0; |
There was a problem hiding this comment.
-
Those documents not being found * should * not happen, since it's not really logical to be able to click on a document that does not exist. But.. 🤷
-
It's there as a fallback/artefact of the type system being unable to assure us that a given value will really be in the array, (which is normal, I guess 🤔 ) Another approach would be to just throw an error and not log anything. Which should happen upstream anyway, not in this function, which is pretty deep in the codebase.
-
For now I would suggest that
indexshould be 0, anddocumentPosition-1 when it's not found. Which most likely means it's going to be rejected by UA (I would assume...).
In any case, I guess just keep in mind that no matter what, if we are unable to find the given result unique ID in the list of results, there's not much we can do to make the UA payload "correct", it's most likely going to fail in one manner or the other. And it being 0, or -1, is a small detail versus the fact that there's some code upstream that is completely wrong when calling this function.
| export function filterProtocol(uri: string) { | ||
| // Filters out dangerous URIs that can create XSS attacks such as `javascript:`. | ||
| const isAbsolute = /^(https?|ftp|file|mailto|tel):/i.test(uri); | ||
| const isRelative = /^\//.test(uri); |
There was a problem hiding this comment.
do we want to cover ./ ../
https://coveord.atlassian.net/browse/KIT-757