fix: Context updating, environment rate limiting, toggle styling #437
Conversation
cdelst
changed the title
| updateOverride(flagKey, newValue); | ||
| }} | ||
| /> | ||
| <div className="animated-switch-container"> |
There was a problem hiding this comment.
The styles broke for our launchpad component. I want to get this fix out before debugging that, so for now I created a new style that styles correctly.
We were having issues with the styling regarding rems and correctly spacing the switch anyway, so this improves overall looks too at the cost of custom css.
| @@ -0,0 +1,77 @@ | |||
|
|
|||
| .animated-switch-container { | |||
There was a problem hiding this comment.
Going to be honest, most of this was written with Cursor. It looks great to me, and functions perfectly, so not questioning it.
It can be removed when we figure out the Switch issue in launchpad.
There was a problem hiding this comment.
i have a couple of questions! also, i found an error when switching to a couple of specific environments. it might be outside the scope of this specific PR but wanted to raise what i found:
when trying to switch to production i got:
PATCH http://localhost:5173/api/dev/projects/default 500
{
"code": "internal_server_error",
"message": "unable to get source flags from LD SDK: LaunchDarkly client initialization failed"
}
i also experienced this when switching to catfood, but not other environments like my own local dev, back to staging, etc.
| if limit != nil { | ||
| request = request.Limit(*limit) | ||
| } |
There was a problem hiding this comment.
i think we still need this in some form? i don't see the limit from the frontend being respected
There was a problem hiding this comment.
oh huh, interesting. I'll debug this. Thanks for testing and calling it out!
There was a problem hiding this comment.
yea, turns out I forgot to add it back 😅
| useEffect(() => { | ||
| const filtered = environments?.filter( | ||
| (env) => | ||
| env.name.toLowerCase().includes(searchQuery.toLowerCase()) || | ||
| env.key.toLowerCase().includes(searchQuery.toLowerCase()), | ||
| ); | ||
| setFilteredEnvironments(filtered); | ||
| setFilteredEnvironments(filtered || []); |
There was a problem hiding this comment.
it's not clear to me if we still need to have a reference to filteredEnvironments in addition to just environments. it seems like the intention was to limit the number of envs we get upfront, so the filtering should now be happening on the backend rather than the frontend? please correct me if i've misunderstood 😅
There was a problem hiding this comment.
Ooh yea I think you're right! The backend is now doing the filtering :)
|
@vroske-ld I am making a ticket to investigate your unrelated issue. |
cdelst
deleted the
cdelst/FUN-231/rate-limits-not-respected-by-launch-devly-environment-fetches
branch
We now query environments by name, instead of all X at once up front.