fix(webapp): write-path and appearance-control fixes for the theme work - #4756
Conversation
The limiter covered one of four paths that write the same dashboardPreferences column: resources.preferences.sidemenu and .favorites accept unlimited authenticated writes and go through the locked read-modify-write, which is more expensive than the single narrow jsonb_set this capped. The protected write is one indexed update on the caller's own row. It was also user-visible in the wrong way: Radix Slider commits on every arrow keypress, so keyboard users hit the 20-per-minute cap partway across the contrast range. Debouncing the control is the right fix for that, and lands separately.
getDashboardPreferences feeds the full-blob writers in dashboardPreferences.server.ts: mutateDashboardPreferences parses the column, hands the result to a mutator, and persists the whole object back. zod strips unknown keys by default, so a deploy that does not know about a preference field erases it on the next write through that path - and updateCurrentProjectEnvironmentId sits on the project navigation hot path. Passthrough makes those writers preserve fields they were not compiled against. It cannot help already-running deploys, so the four appearance fields added alongside it stay exposed until this lands; they are behind hasThemeSwitcher in the meantime.
The five dashboardPreferences writers already no-op for an impersonating admin, but the three profile writers added alongside them did not: requireUserId returns the impersonated user's id, so a support session could permanently rewrite that user's name, email and marketing-email preference. Both gates now refuse up front and say so, instead of the preference writers silently no-opping while the page reports success.
The customize dialog builds its hidden map from the sections it can see, and the write replaced hiddenItems wholesale. The profile page has no org in scope, so it resolves sections from the most-recently-updated project's org: confirming the dialog there dropped every hidden id belonging to a section that org's feature flags exclude, un-hiding those items everywhere else. The payload now carries the item ids the dialog rendered and the write only replaces those. Submissions without the list stay authoritative, so the side menu's own path is unchanged until it sends one.
Supersedes the passthrough approach earlier in this branch. z.object().passthrough() puts an index signature on the inferred type, which Prisma's InputJsonValue and UserWithDashboardPreferences both reject, so it did not typecheck. preserveUnknownKeys does the same job at the one place that matters - the full-blob write inside mutateDashboardPreferences - and leaves DashboardPreferences exactly as strict as before. Scope matches passthrough: unknown keys, not new values of a declared key. Also supplies the knownItemIds parameter the previous commit destructured but never declared.
getEmailOwnership fans out one SSO status lookup per organization the user belongs to. It ran in the profile loader on every page view, purely to choose which body the edit-email dialog renders; the action re-derives it before writing either way. It now loads from a resource route when the dialog opens, so page views that never open it cost nothing, and the check that guards the write has one call site instead of two.
The ownership check only looked at the address the user already had. It now considers the current and the submitted address together, so an organization that manages either one governs the change. Validation moved ahead of the check so the submitted domain is parsed before it is used. emailDomainOf splits on the last @ rather than the first, and is exported so its behaviour is covered directly.
The appearance submenu offers System, Light and Dark; Black and White live on the profile page. With one of those two stored, every row read as unselected, so the menu asserted the user had no theme at all. The row that leads to them now carries the check instead.
The theme and system-theme selects stamp data-theme before the write lands. When the write fails the loader returns the value it always had, so useSystemThemeSync's effect deps are unchanged and React's vdom diff sees no change either - nothing rewrites the attribute, and the page keeps rendering a theme that was never stored while the select shows the stored one. The stored pair is now re-applied explicitly, as the side menu's switcher already did. The contrast slider is debounced for the same reason it needed to be: Radix commits on every arrow keypress, so a keyboard user crossing the range fired one write per step. The resnap effect now waits for the debounce slot to drain so it can't undo a drag mid-flight.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Observability mapAs of 19/100 over 446 measured of 464 entry points (base 19, no change) What this PR changed
FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
2 participants
Fixes found while reviewing #4547, stacked on that branch so they can be reviewed on their own and merged into it. One commit per fix.
Write-path correctness
Refuse account writes while impersonating. The five
dashboardPreferenceswriters already no-op for an impersonating admin, but the three profile writers added next to them did not, andrequireUserIdreturns the impersonated user's id. Both gates now refuse up front and say so, rather than the preference writers silently no-opping while the page reports success.Preserve unknown keys on a full-blob write.
mutateDashboardPreferencesparses the JSON column, hands the result to a mutator and persists the whole object back. zod strips keys it does not declare, so a deploy that predates a preference field drops it on the next write through that path — andupdateCurrentProjectEnvironmentIdsits on the navigation hot path.preserveUnknownKeysre-attaches them at the write. Note this cannot help deploys already running, so it makes this the last release able to strip rather than retroactively protecting the fields added in #4547.Scope hidden-sidebar writes to what was shown. The customize dialog builds its hidden map from the sections it can see and the write replaced
hiddenItemswholesale. The profile page has no org in scope, so it resolves sections from the most-recently-updated project's org: confirming there dropped hidden ids belonging to sections that org's flags exclude. The payload now carries the ids the dialog rendered and the write only replaces those. Submissions without the list stay authoritative.Consider both addresses when checking email ownership. The check only looked at the address the user already had; it now considers the current and submitted address together, so an org managing either one governs the change. Validation moved ahead of the check, and
emailDomainOfsplits on the last@.Interaction
Revert unsaved themes, debounce contrast saves. The theme and system-theme selects stamp
data-themebefore the write lands. When it fails, the loader returns the value it always had — souseSystemThemeSync's effect deps are unchanged and React's vdom diff sees no change either, and nothing rewrites the attribute. The page kept rendering a theme that was never stored while the select showed the stored one. The stored pair is now re-applied explicitly, as the side menu's switcher already did. The contrast slider is debounced because Radix commits on every arrow keypress, so a keyboard user crossing the range fired one write per step.Tick More options for themes outside the short list. The appearance submenu offers System, Light and Dark; Black and White live on the profile page. With one of those stored, every row read as unselected.
Subtraction
Drop the profile update rate limiter. It covered one of four paths that write the same column —
resources.preferences.sidemenuand.favoritestake unlimited authenticated writes and go through the locked read-modify-write, which is more expensive than the single narrowjsonb_setthis capped. It was also what made the contrast slider unusable by keyboard. If preference writes want limiting, it belongs in one place covering all of them.Resolve email ownership when the dialog opens. It fans out one SSO status lookup per organization the user belongs to and ran in the profile loader on every page view, purely to pick which body the dialog renders. The action re-derives it before writing either way, so the check that guards the write now has one call site instead of two.
Testing
typecheck --filter webappandlintclean. New unit tests forpreserveUnknownKeys,mergeHiddenItemsandemailDomainOf;themePreference,mergeHiddenItemsandssoManagedIdentitysuites pass locally (26 tests). The rest of the webapp suite needs testcontainers and is left to CI.No changeset or
.server-changesentry: everything here fixes code on the parent branch that has not shipped. The one exception worth a maintainer's call ismergeHiddenItems, which also touches the side menu's own customize path.