feat(profile): allow clearing free-text fields to empty - #1301
Conversation
Free-text profile fields (bio, job_title, address, city, postal_code, phone_number) now send the raw form value on save, so emptying a control sends '' and auth-service overwrites the stored value. Previously every field mapped `field || undefined`, dropping empties from the PATCH so a set value could never be cleared. Name and the constrained selects keep `|| undefined` and stay non-clearable. Change is client-only: the BFF is a pass-through, the validator accepts '', and the optimistic update already forwards empty strings. Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>
PR SummaryMedium Risk Overview Safety when metadata never loaded: If Flow C: Pending save in Tests: E2E S5–S7 cover clear-to- Reviewed by Cursor Bugbot for commit d341ce6. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProfile submission preserves empty free-text values when metadata loads successfully. It omits empty values when metadata is unavailable. Flow C restores mapped metadata and supports legacy envelopes. E2E tests verify both payload behaviors. ChangesProfile empty-value clearing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ProfileEditDrawerComponent
participant ProfileAPI
participant SessionStorage
participant ProfileLayoutComponent
ProfileEditDrawerComponent->>ProfileAPI: PATCH normalized user_metadata
ProfileEditDrawerComponent->>SessionStorage: Persist mapped userMetadata on redirect
SessionStorage->>ProfileLayoutComponent: Restore userMetadata
ProfileLayoutComponent->>ProfileLayoutComponent: Apply legacy envelope mapping when required
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Enables users to clear free-text profile fields by sending explicit empty strings.
Changes:
- Preserves empty values in direct and Flow C saves.
- Adds E2E coverage for clearing a bio.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
profile-edit-drawer.component.ts |
Sends raw free-text values. |
profile-layout.component.ts |
Preserves empty values during Flow C replay. |
profile-edit-drawer.spec.ts |
Tests clearing and optimistic UI updates. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Address PR #1301 review feedback from cursor[bot], copilot[bot]: - profile-edit-drawer.component.ts / profile-layout.component.ts: only send free-text fields as raw '' when the profile metadata loaded. On a NATS getUserInfo miss the GET returns 200 with profile: null and the controls seed empty, so an unguarded save would wipe unloaded bio, job_title, address, and phone data. Fall back to omitting empties when profile is null. (per cursor[bot]) - profile-layout.component.ts: narrow the applyOptimisticProfileUpdate comment so it no longer claims all empty fields map to undefined; only name/select fields and the failed-load case do, and cleared free-text fields send ''. (per copilot[bot]) Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>
Review Feedback AddressedCommit: 83bffb6 Changes Made
Threads Resolved2 of 2 unresolved threads addressed in this iteration. Note: the new guard's failed-load path isn't covered by the S5 E2E (it would require stubbing the profile GET to return |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts:252
- On the Flow C return, this check runs immediately from the constructor's
queryParamssubscription, before the cold-load profile GET populatescombinedProfile(the existing comment inapplyOptimisticProfileUpdate()documents that ordering). As a result,metadataLoadedis false and a saved''becomesundefined, so the replayed PATCH still cannot clear a free-text field whenever management-token authorization is required. Preserve the original loaded-state in the pending-save envelope or defer replay mapping until the profile load completes.
const metadataLoaded = this.combinedProfile?.profile != null;
const freeText = (value: string | null | undefined): string | undefined => (metadataLoaded ? (value ?? '') : value || undefined);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts (1)
208-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failed metadata-load branch.
The current E2E scenario covers clearing
bioafter a successful metadata load. It does not verify that empty values are omitted when the metadata lookup fails. Add a regression test for that branch and cover at least one additional clearable field.According to the PR objective, the existing E2E coverage is limited to
bio.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts` around lines 208 - 228, Add E2E coverage for the failed metadata-load path represented by metadataLoaded in the profile edit flow, verifying that empty bio is omitted rather than submitted when profile metadata is unavailable. Also clear and assert omission of at least one additional free-text field such as job_title, city, address, postal_code, phone_number, or bio, while preserving the existing successful-load bio coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts`:
- Around line 208-228: Add E2E coverage for the failed metadata-load path
represented by metadataLoaded in the profile edit flow, verifying that empty bio
is omitted rather than submitted when profile metadata is unavailable. Also
clear and assert omission of at least one additional free-text field such as
job_title, city, address, postal_code, phone_number, or bio, while preserving
the existing successful-load bio coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0116286e-3f13-4a53-9621-92798313cfa1
📒 Files selected for processing (2)
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.tsapps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts
Address Cursor Bugbot review on PR #1301 (LFXV2-2933): The management-token redirect (Flow C) re-derived the PATCH payload in ProfileLayoutComponent.handleProfileAuthReturn() from the persisted raw form, gating clear-to-empty on the live combinedProfile. On a full-page Flow C return that signal hasn't reloaded yet (still null), so legitimate cleared free-text fields were dropped instead of sent as ''. - profile-edit-drawer.component.ts: persist the already-mapped userMetadata payload (not the raw form) to sessionStorage at submit time, capturing the clear-to-empty decision while the profile is loaded. - profile-layout.component.ts: replay that payload verbatim on return, removing the duplicated mapping and the stale combinedProfile gate. Resolves 1 review thread. Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>
Review Feedback AddressedCommit: 9227aa6 Changes Made
Threads Resolved1 of 1 unresolved thread addressed in this iteration. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts:244
- Changing the pending-save envelope drops valid saves created by a pre-deploy tab. Flow C performs a full navigation, so a user can store the old
{ savedAt, form }shape, return after a deployment, and hit this new parser; the key has already been removed at line 234, then this returns without replaying the PATCH or showing an error. Accept the old shape for the existing 10-minute TTL and map it with the previous rules, or explicitly version and migrate the envelope.
const envelope = JSON.parse(savedState) as { savedAt?: unknown; userMetadata?: Partial<UserMetadata> };
if (typeof envelope?.savedAt !== 'number' || !envelope.userMetadata || Date.now() - envelope.savedAt > ProfileLayoutComponent.pendingSaveTtlMs) {
return;
}
userMetadata = envelope.userMetadata;
Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>
Comment cleanupCommit: 26457e7 Trimmed the clear-to-empty explanatory comments in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts:239
- This hard schema switch drops pending saves created by the previous client version, which stores
{ savedAt, form }. If a user starts Flow C before a deployment and returns after the new bundle is live, this branch removes the session entry first and then silently returns becauseuserMetadatais absent, so their profile edit is lost. Please accept the legacy envelope during the 10-minute migration window (mappingformwith the previous field rules) alongside the newuserMetadataenvelope.
const envelope = JSON.parse(savedState) as { savedAt?: unknown; userMetadata?: Partial<UserMetadata> };
if (typeof envelope?.savedAt !== 'number' || !envelope.userMetadata || Date.now() - envelope.savedAt > ProfileLayoutComponent.pendingSaveTtlMs) {
Review Feedback AddressedNo Change Needed
Threads Resolved1 of 1 unresolved thread addressed in this iteration. No code changes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts:260
- The new redirect payload contract is not exercised by S5: its PATCH always succeeds, so neither storing
userMetadatanor replayingbio: ''after Flow C is tested. A mismatch between the two envelope keys would silently lose the save. Add coverage where the first PATCH returnsmanagement_token_required, the pending state retains the empty string, and the callback replay sends it unchanged.
// Persist the mapped payload (not the raw form) so the submit-time clear-to-empty decision
// survives the redirect; the host replays it verbatim (stringify drops undefined keys).
sessionStorage.setItem(PENDING_PROFILE_SAVE_KEY, JSON.stringify({ savedAt: Date.now(), userMetadata }));
The Flow C replay parser only accepted the new { savedAt, userMetadata }
envelope. A pre-LFXV2-2933 bundle writes { savedAt, form } (raw form value),
so a save started just before a mid-Flow-C deploy would be silently dropped
on return. Accept the legacy shape during the pending-save TTL window and map
it with the prior omit-empties rules (per copilot-pull-request-reviewer).
Also add E2E S6: with profile metadata unloaded (GET returns no profile),
clearing bio and job_title omits both keys from the PATCH rather than wiping
stored data the form never received (per coderabbitai).
Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>
Review Feedback AddressedCommit: 4df86d5 Changes Made
Threads Resolved1 of 1 unresolved thread addressed in this iteration (the legacy-envelope parser thread). The earlier Cursor, CodeQL, and Copilot threads were already resolved. The legacy-envelope compat is intentionally temporary — removable once no pre-2933 bundle can still be serving the write path (past the pending-save TTL). It's commented as such. |
A failed profile GET left combinedProfile.profile null; the first save's optimistic merge fabricated a non-null profile, flipping the drawer's metadataLoaded true on reopen so a later save could send '' and wipe free-text fields the GET never loaded. On the null-profile path, refetch instead of merging so the load status stays truthful across save → reopen → save (per copilot-pull-request-reviewer). Add E2E S7: with the profile GET always returning no profile, a first save then reopen must still omit an empty bio rather than send ''. Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>
Review Feedback AddressedCommit: f5cdb47 Changes Made
Threads Resolved1 of 1 unresolved thread addressed in this iteration. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts:201
- This immediate refetch contradicts the component's eventual-consistency invariant (lines 79–81 and 269–271). When the initial metadata GET yields
profile: nullbut the PATCH succeeds, the refetch can still return the pre-save/null body, so the success toast is shown while the saved free-text value never appears in the panel or a reopened drawer. Preserve the optimistic update, but keep the original metadata-load status as separate state so reopening cannot enable clear-to-empty until a real metadata response has loaded.
// A null profile means the GET never loaded; merging would fabricate a non-null profile and flip
// the drawer's metadataLoaded true, letting a later save wipe unloaded fields. Refetch instead.
if (this.combinedProfile.profile == null) {
this.refreshProfile$.next();
return;
Review Feedback AddressedNo Change Needed
Threads Resolved1 of 1 unresolved thread addressed in this iteration. No code changes. |
|
Hey @fayazg — reviewed the current state of this PR (head Overall impression: This is a well-scoped fix for a real usability gap (free-text profile fields couldn't be cleared to empty because This PR has already been through multiple rounds of bot review (Copilot, cursor[bot], CodeQL) and the author has iterated commit-by-commit to address each one. I traced each of those through to the current code rather than re-litigating them:
I didn't find anything beyond what's already been raised and resolved. Server-side validation ( Issue count:
Final decision: ✅ Approved |
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Approved — see the summary comment for full reconciliation against prior review rounds. No blocking, minor, or open questions found in an independent pass; the metadataLoaded gate and Flow C persistence both hold up under tracing.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit d341ce6. Configure here.
| sessionStorage.setItem(PENDING_PROFILE_SAVE_KEY, JSON.stringify({ savedAt: Date.now(), form: this.profileForm.value })); | ||
| // Persist the mapped payload (not the raw form) so the submit-time clear-to-empty decision | ||
| // survives the redirect; the host replays it verbatim (stringify drops undefined keys). | ||
| sessionStorage.setItem(PENDING_PROFILE_SAVE_KEY, JSON.stringify({ savedAt: Date.now(), userMetadata })); |
There was a problem hiding this comment.
Flow C envelope rollout gap
Medium Severity
The pending-save format changed from { savedAt, form } to { savedAt, userMetadata }, and the host only accepts the reverse (legacy write → new read). A Flow C save started on the new bundle can still return onto an old bundle mid-rollout; that reader requires form, removes the session entry first, then bails — so the replay is silently dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d341ce6. Configure here.
Summary
bio,job_title,address,city,postal_code,phone_number) now send the raw form value on save, so emptying a control sends''and auth-service overwrites the stored value.field || undefined, so an emptied control wasomitted from the PATCH and a set value could never be cleared back to empty.
country,state_province,t_shirt_size,organization) keep|| undefinedand remain non-clearable, by product decision.onSubmit()and the Flow Cmanagement-token replay in
handleProfileAuthReturn().validateUserMetadataaccepts
'', and the optimistic update already forwards empty strings — no serveror optimistic-update change required.
PATCH carries
bio: ''and the panel drops the About Me block.