Conversation
Bundle Size ComparisonFull build detailsThe full bundle is larger than main by 46.45 kB. ❗ The index.js is larger than main by 46.45 kB. ❗ Run
|
macrouch
requested review from
eudoroolivares2016,
mandyparson,
stephenkilbourn and
trevorlang
August 12, 2026 20:59
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2079 +/- ##
==========================================
- Coverage 95.84% 95.84% -0.01%
==========================================
Files 756 761 +5
Lines 17776 17840 +64
Branches 4978 5026 +48
==========================================
+ Hits 17037 17098 +61
- Misses 687 690 +3
Partials 52 52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
macrouch
commented
Aug 13, 2026
stephenkilbourn
approved these changes
Aug 13, 2026
mandyparson
reviewed
Aug 14, 2026
| config="`jq '.application.nlpSearch = $newValue' --arg newValue $bamboo_NLP_SEARCH <<< $config`" | ||
| config="`jq '.application.numberOfGranules = $newValue' --arg newValue "$bamboo_NUMBER_OF_GRANULES" <<< $config`" | ||
| config="`jq '.application.placeLabelsStyleUrl = $newValue' --arg newValue $bamboo_PLACE_LABELS_STYLE_URL <<< $config`" | ||
| config="`jq '.application.growthbookEnabled = $newValue' --arg newValue $bamboo_GROWTHBOOK_ENABLED <<< $config`" |
Contributor
There was a problem hiding this comment.
--argjson, --arg evaluates everything to a string
Contributor
Author
There was a problem hiding this comment.
I like the idea of switching this, but I wouldn't only change this field, I'd want every boolean to behave the same way.
But that also means that environment variables used in a lambda would behave differently than those in React, because they are set within the dockerRun function in this file.
We should put a little thought into that separately
| event_type: eventType, | ||
| event_data: eventData, | ||
| session_id: gbSessionId, | ||
| user_id: gbUserId |
Contributor
There was a problem hiding this comment.
we'll probably tweak this quite a bit, but I think it's worth adding the window.location.pathname as well
Contributor
Author
There was a problem hiding this comment.
I'm just guessing on most of these, so I'm not going to change it for this PR. On the next ticket when we learn more that would be when we need to change these
|
|
||
| expect(localStorageSetItemSpy).toHaveBeenCalledTimes(2) | ||
| expect(localStorageSetItemSpy).toHaveBeenNthCalledWith(1, 'gbUserId', expect.any(String)) | ||
| expect(localStorageSetItemSpy).toHaveBeenNthCalledWith(2, 'gbSessionId', expect.any(String)) |
Contributor
There was a problem hiding this comment.
Can we make these more specific? Hard to be certain userId is actually the userId and not the sessionId
Contributor
Author
There was a problem hiding this comment.
I'm not sure how to be more specific. This is ensuring that we are setting gbUserId and gbSessionId. We don't need to test that crypto.randomUUID() is giving us a specific string
| }) | ||
| }) | ||
|
|
||
| test('renders the spinner then the children', async () => { |
Contributor
There was a problem hiding this comment.
this just tests the spinner
| useEffect(() => { | ||
| const gbUserId = window.localStorage.getItem('gbUserId') | ||
| if (!gbUserId) { | ||
| const newGbUserId = crypto.randomUUID() |
Contributor
There was a problem hiding this comment.
Thinking about the test, could make this user-${crypto.randomUUID} to help differentiate it from the sessionId
Contributor
Author
There was a problem hiding this comment.
We don't like writing code specifically for tests if we can avoid it. Adding a prefix to this value would only add meaningless characters to every row of data we end up querying downstream. We know its a user or session because they are labeled correctly in the places they are stored
| const growthbook = new GrowthBook({ | ||
| apiHost: growthbookApiHost, | ||
| clientKey: growthbookClientKey, | ||
| enableDevMode: true, |
Contributor
There was a problem hiding this comment.
base off of env instead
| }) | ||
| } | ||
| }) | ||
| growthbook.init({}) |
Contributor
There was a problem hiding this comment.
Can we check to see if growthbook has already been intialized first?
Contributor
Author
There was a problem hiding this comment.
There is annoying little documentation on this method, so I don't know if you can check.
But this component will only be rendered once. The only trigger it has to re-render is if the children prop changes, which can't happen
|
|
||
| import useEdscStore from '../../../zustand/useEdscStore' | ||
|
|
||
| const logEvent = async (eventKey: string, eventType: string, eventData: string) => { |
Contributor
Author
There was a problem hiding this comment.
My guess here is that we just need a simple data type to query downstream, so I went with string thinking that it would be which filter was changed. But in the next ticket when we know more it could change
mandyparson
approved these changes
Aug 14, 2026
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
3 participants
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.
Overview
What is the feature?
Adds GrowthBook SDK for A/B testing
What is the Solution?
Adds new Zustand slice to hold GrowthBook details
Creates a random user id and session id for users
Wraps the GrowthBook code around a deployment variable to turn on/off new code
Adds a new call to LoggerRequest for experiment event logging
Adds a new lambda to send experiment events to AWS Data Firehose
What areas of the application does this impact?
Adds new pieces
Testing
You can set up GrowthBook to run locally with a docker compose file. Create a user, create a feature flag (probably more things you have to create but I can't remember). Name the feature flag
nlpSearchand have it service a boolean with a default value of true or false, and you can see it enable/disable the nlpSearch on the home page.Add the
logEventutil to the code inchangeQueryto see it submit to the lambda to be logged.Checklist
npm audit fixand made note of any changes in this PR