Env vars page performance fix#3829
Conversation
The Environment Variables page presenter loaded the entire project
secret store via a prefix scan and decrypted every value on each
render — including secret values that are immediately masked in the
UI — then matched rows with nested O(N×M²) `.find()` lookups.
- Collect only the non-secret (environmentId, key) pairs and fetch
them with a targeted `key IN (...)` query; decrypt only those.
- Add `getSecretsByKeys` to the secret store and
`getVariableValuesForKeys` to the repository for this access path.
- Replace the nested `.find()` lookups with O(1) Map lookups keyed
by `${environmentId}:${key}`.
Cuts per-render decryption and server CPU for projects with many
variables and environments; secret values stay masked as before.
The page server-rendered every row (~13 KB of markup each), producing a tens-of-MB HTML document and mounting thousands of row components on hydration, which froze the browser for projects with many variables across many environments. - Server-render only the first 50 rows, hydrate those, then switch to @tanstack/react-virtual over the full dataset after mount via useLayoutEffect (server and first client render match — no hydration mismatch). - Virtualize with a spacer-row technique inside the existing <table> so column widths and the sticky header are preserved; extract a shared EnvironmentVariableTableRow used by both the SSR and virtual paths to avoid drift. - Seed useFuzzyFilter from the URL `search` param (controlled mode, matching the Tasks page) so filtering happens at SSR and deep links render the correct rows in the initial window. For ~11k rows the document drops from ~150 MB to ~5 MB with 50 SSR rows; the load freeze is gone.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
WalkthroughThis PR implements backend and frontend performance optimizations for the Environment Variables page. The changes add a bulk-secret retrieval API to the SecretStore, extend the EnvironmentVariablesRepository with a method to fetch variable values for specified keys only, introduce a shared 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/webapp/app/services/secrets/secretStore.server.ts (1)
144-174: ⚡ Quick winBatch parsing aborts entirely on a single schema-validation failure.
Invalid JSON is handled gracefully (log +
undefined), butschema.parseat Line 149 and Line 173 throws. Because#parseStoredSecretscalls this per secret, one malformed/legacy entry now causesgetSecretsByKeys/getSecretsto reject for the whole batch — which can break the entire Environment Variables SSR page rather than dropping one row. Consider making the batch path tolerant (skip + log) while keeping the single-secretgetSecretpath strict.Please confirm whether a single corrupt secret should fail the whole page load; if strict behavior is intended, ignore this.♻️ One option: tolerant batch wrapper
for (const secret of secrets) { - const value = await this.#parseStoredSecret(schema, secret); - if (value !== undefined) { - results.push({ key: secret.key, value }); - } + try { + const value = await this.#parseStoredSecret(schema, secret); + if (value !== undefined) { + results.push({ key: secret.key, value }); + } + } catch (error) { + logger.error(`Failed to parse secret ${secret.key}`, { error }); + } }apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx (1)
701-705: ⚡ Quick winAdd
keyprops to spacer rows.The spacer
<tr>elements should have explicitkeyprops to prevent React warnings during re-renders.🔧 Suggested fix
{topSpacerHeight > 0 && ( - <tr aria-hidden style={{ height: topSpacerHeight }}> + <tr key="top-spacer" aria-hidden style={{ height: topSpacerHeight }}> <td colSpan={columnCount} /> </tr> )} {virtualItems.map((virtualRow) => { const variable = groupedEnvironmentVariables[virtualRow.index]; if (!variable) { return null; } return ( <EnvironmentVariableTableRow key={`${variable.id}-${variable.environment.id}`} variable={variable} revealAll={revealAll} vercelIntegration={vercelIntegration} /> ); })} {bottomSpacerHeight > 0 && ( - <tr aria-hidden style={{ height: bottomSpacerHeight }}> + <tr key="bottom-spacer" aria-hidden style={{ height: bottomSpacerHeight }}> <td colSpan={columnCount} /> </tr> )}Also applies to: 721-725
apps/webapp/test/environmentVariablesRepository.test.ts (1)
129-136: 💤 Low valueReuse the
uniqueIdhelper instead of rawDate.now()for slug/externalRef.This inline creation duplicates the fixture's project-creation logic and relies on
Date.now()alone for uniqueness, which is weaker than theuniqueIdhelper (counter + timestamp) used elsewhere. Prefer the shared helper for consistency and collision resistance.♻️ Suggested change
- data: { - name: "Project B", - slug: `proj-b-${Date.now()}`, - organizationId: organization.id, - externalRef: `ext-b-${Date.now()}`, - }, + data: { + name: "Project B", + slug: uniqueId("proj-b"), + organizationId: organization.id, + externalRef: uniqueId("ext-b"), + },Add
uniqueIdto the fixtures import on line 24-28.apps/webapp/app/presenters/v3/EnvironmentVariablesPresenter.server.ts (1)
123-131: ⚡ Quick winResidual nested
.findlookups undercut the stated O(1) goal.
nonSecretItems(here) and the finalflatMap(Line 154) both iterateenvironmentVariables × sortedEnvironmentsand call.findovervalueson each pair. The PR aims to replace nested.find()with O(1) Map lookups; consider building aMap<environmentId, valueRecord>perenvironmentVariableonce and reusing it in both passes to keep the hot path linear.♻️ Sketch
- const nonSecretItems: Array<{ environmentId: string; key: string }> = []; - for (const environmentVariable of environmentVariables) { - for (const env of sortedEnvironments) { - const valueRecord = environmentVariable.values.find((v) => v.environmentId === env.id); - if (valueRecord && !valueRecord.isSecret) { - nonSecretItems.push({ environmentId: env.id, key: environmentVariable.key }); - } - } - } + const valuesByEnvForVariable = new Map( + environmentVariables.map((ev) => [ + ev.id, + new Map(ev.values.map((v) => [v.environmentId, v])), + ]) + ); + const nonSecretItems: Array<{ environmentId: string; key: string }> = []; + for (const environmentVariable of environmentVariables) { + const byEnv = valuesByEnvForVariable.get(environmentVariable.id)!; + for (const env of sortedEnvironments) { + const valueRecord = byEnv.get(env.id); + if (valueRecord && !valueRecord.isSecret) { + nonSecretItems.push({ environmentId: env.id, key: environmentVariable.key }); + } + } + }Then reuse
valuesByEnvForVariablein the finalflatMapinstead of.find.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 92f33967-95f9-4d5b-8b90-7f0d6902b836
📒 Files selected for processing (18)
.server-changes/environment-variables-page-performance.mdapps/webapp/app/components/primitives/Table.tsxapps/webapp/app/presenters/v3/CreateEnvironmentVariablesPresenter.server.tsapps/webapp/app/presenters/v3/EnvironmentVariablesPresenter.server.tsapps/webapp/app/presenters/v3/environmentVariablesEnvironments.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables.new/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsxapps/webapp/app/services/secrets/secretStore.server.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.tsapps/webapp/app/v3/environmentVariables/repository.tsapps/webapp/scripts/measure-create-environment-variables-loader.mtsapps/webapp/scripts/measure-environment-variables-html.mtsapps/webapp/scripts/measure-environment-variables-new-parent-loader.mtsapps/webapp/scripts/spike-environment-variables-table-dom.mtsapps/webapp/test/EnvironmentVariablesPresenter.test.tsapps/webapp/test/environmentVariablesEnvironments.test.tsapps/webapp/test/environmentVariablesRepository.test.tsapps/webapp/test/fixtures/environmentVariablesFixtures.ts
kathiekiwi
force-pushed
the
env-vars-page-performance-fix
branch
from
74b25d6 to
3142404
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Opening /environment-variables/new ran the full list presenter twice — once in the parent route loader and once in the child loader — fetching and decrypting every variable value just to show the create form, which only needs the list of environments. - Short-circuit the parent route loader on the /new path so it skips the list presenter entirely and renders only the create outlet. - Load just the environment list in the child route via a new CreateEnvironmentVariablesPresenter. - Extract the shared environment-loading logic into loadEnvironmentVariablesEnvironments, preserving the project access check and environment filtering for both presenters. Removes the heavy presenter work (full fetch + decrypt) when opening the create form.
kathiekiwi
force-pushed
the
env-vars-page-performance-fix
branch
from
3142404 to
8256c34
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| export default function Page() { | ||
| const loaderData = useTypedLoaderData<EnvironmentVariablesPageLoaderData>(); | ||
|
|
||
| if (loaderData.isCreateRoute) { | ||
| return ( | ||
| <PageContainer> | ||
| <NavBar> | ||
| <PageTitle title="Environment variables" /> | ||
| <PageAccessories> | ||
| <LinkButton | ||
| LeadingIcon={BookOpenIcon} | ||
| to={docsPath("v3/deploy-environment-variables")} | ||
| variant="docs/small" | ||
| > | ||
| Environment variables docs | ||
| </LinkButton> | ||
| </PageAccessories> | ||
| </NavBar> | ||
| <Outlet /> | ||
| </PageContainer> | ||
| ); | ||
| } | ||
|
|
||
| return <EnvironmentVariablesListPage loaderData={loaderData} />; | ||
| } |
There was a problem hiding this comment.
🚩 Outlet removal from list page is an intentional UX change
The old code rendered <Outlet /> inside the list page, so the /new dialog appeared as an overlay on top of the variable list. The new code conditionally renders either the list page (without <Outlet />) or a minimal NavBar + <Outlet /> shell for the create route. This means the user's scroll position and list state are lost when opening the create dialog, and the list is reloaded from scratch when navigating back. This is an intentional performance trade-off (avoiding the expensive list load just to show a create dialog), but reviewers should confirm this UX change is acceptable.
Was this helpful? React with 👍 or 👎 to provide feedback.
| project: { | ||
| slug: projectSlug, | ||
| }, | ||
| archivedAt: null, |
There was a problem hiding this comment.
Will this query be slow? There's an index on projectId we could use if we have the id to use instead.
There was a problem hiding this comment.
I just saw this is how we already do it, I wonder if this is part of why the page is slow on production where we have a ton of environments.
There was a problem hiding this comment.
We should switch to passing the project id into this function and use that here
3 participants
Summary
This PR improves performance across the Environment Variables page.
Changes
Targeted value loading
SSR windowing + virtualization
?search=DATABASE_URLLightweight 'Create' flow
Results
Large projects no longer render thousands of rows during SSR.
Example (~11k rendered rows):
Testing
Automated
Manual