Update package scopes when username changes - #793
Conversation
Username changes previously only rewrote users.username, leaving saved
packages and community listings on the old @{username} scope. Cascade
rewrites package.json names and same-account kody:@ references, publishes
an automatic update commit per package before flipping the username, warns
on the account profile form, and republishes community listings that were
already on the latest package commit.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughUsername changes now rewrite saved package scopes and references, publish updated package commits, republish affected community listings, and expose update results and warnings through the account API and client. ChangesUsername Scope Rename
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AccountRoute
participant AccountProfileHandler
participant PackageUpdateService
participant Repository
participant CommunityListings
User->>AccountRoute: submit username change
AccountRoute->>AccountProfileHandler: POST profile update
AccountProfileHandler->>PackageUpdateService: update package scopes
PackageUpdateService->>Repository: publish rewritten package commits
AccountProfileHandler->>AccountProfileHandler: persist username
AccountProfileHandler->>CommunityListings: republish pinned listings
AccountProfileHandler-->>AccountRoute: return update counts and warnings
AccountRoute-->>User: show save result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
🔎 Preview deployed: https://kody-pr-793.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/worker/src/package-registry/username-change-packages.node.test.ts (1)
61-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for unpublished saved packages.
Every test here mocks
loadPackageSourceBySourceIdwithpublished_commit: 'commit-old'. Add a case wherepublished_commitisnullto cover the bootstrap-publish path insyncArtifactSourceSnapshot— this is exactly the scenario behind the file-loss issue flagged inusername-change-packages.ts, and would catch regressions once fixed.🤖 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 `@packages/worker/src/package-registry/username-change-packages.node.test.ts` around lines 61 - 125, Add a test alongside updatePackagesForUsernameChange that mocks loadPackageSourceBySourceId with source.published_commit set to null, then verifies the username-change flow preserves and rewrites the package files through syncArtifactSourceSnapshot during bootstrap publish. Assert the unpublished package is updated successfully and the snapshot call includes the expected renamed package.json and source identifiers.packages/worker/src/app/handlers/account-profile.node.test.ts (1)
300-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the post-package-update DB-failure compensation branch.
Tests cover the pre-check duplicate-username rejection and the package-update failure, but not the case where
db.updateitself fails with a unique-constraint violation after packages were already renamed (lines 112-137 of the handler). That path re-invokesupdatePackagesForUsernameChangewith swapped scopes to compensate — worth a dedicated test (e.g. mockdb.updateto throw a constraint error and assert the compensating call args).🤖 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 `@packages/worker/src/app/handlers/account-profile.node.test.ts` around lines 300 - 336, Add a dedicated test for the post-package-update database failure path in the account profile API handler: make the username package update succeed, mock the user DB update to throw a unique-constraint error, then assert the response and that updatePackagesForUsernameChange is called again with swapped old/new scopes to compensate. Keep assertions focused on the compensation arguments and failure behavior, using the existing test helpers and mocks.packages/worker/src/repo/source-sync.ts (1)
26-33: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
expectedPackageScope/commitMessagearen't honored on the bootstrap path.These two new fields are only forwarded into
session.publishSession(used whensource.published_commitalready exists). The "never published" bootstrap branch ignores both, so renaming an unpublished package's scope publishes without scope validation and with a genericBootstrap source repo …message instead of the rename-specific one. Given the sibling data-loss issue flagged inusername-change-packages.tsconfirms this branch is reachable during rename, consider threading both fields through tobootstrapSourcefor consistency.🤖 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 `@packages/worker/src/repo/source-sync.ts` around lines 26 - 33, The bootstrap path must honor both options. Update the caller and `bootstrapSource` flow to accept and forward `expectedPackageScope` and `commitMessage`, applying them to bootstrap publish validation and commit-message generation instead of the generic defaults, while preserving existing behavior when they are absent.
🤖 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.
Inline comments:
In `@packages/worker/src/app/handlers/account-profile.ts`:
- Around line 82-137: Reserve the requested username atomically before calling
updatePackagesForUsernameChange, using the existing database transaction/locking
mechanism so concurrent requests cannot pass the availability check
simultaneously. Only publish package rewrites after the reservation succeeds,
and finalize or release the reservation consistently when the username update
succeeds or fails; keep the package compensation path for failures after
publication.
In `@packages/worker/src/package-registry/username-change-packages.ts`:
- Around line 35-46: Update the package rename flow around changedFilesOnly and
syncArtifactSourceSnapshot to pass the complete rewritten file set, not only
rewrite.changedPaths. Preserve the rewritten contents for every file and ensure
both unpublished bootstrap and incremental sync paths receive the full tree
without dropping unchanged files.
- Around line 83-100: Make the refreshSavedPackageProjection call in
rewriteAndPublishPackageScope best-effort by catching and handling refresh
failures without rejecting the function after the publish succeeds. Ensure the
function still returns its normal result so the package is added to applied and
remains eligible for compensation, while preserving successful projection
refresh behavior.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/account-profile.node.test.ts`:
- Around line 300-336: Add a dedicated test for the post-package-update database
failure path in the account profile API handler: make the username package
update succeed, mock the user DB update to throw a unique-constraint error, then
assert the response and that updatePackagesForUsernameChange is called again
with swapped old/new scopes to compensate. Keep assertions focused on the
compensation arguments and failure behavior, using the existing test helpers and
mocks.
In `@packages/worker/src/package-registry/username-change-packages.node.test.ts`:
- Around line 61-125: Add a test alongside updatePackagesForUsernameChange that
mocks loadPackageSourceBySourceId with source.published_commit set to null, then
verifies the username-change flow preserves and rewrites the package files
through syncArtifactSourceSnapshot during bootstrap publish. Assert the
unpublished package is updated successfully and the snapshot call includes the
expected renamed package.json and source identifiers.
In `@packages/worker/src/repo/source-sync.ts`:
- Around line 26-33: The bootstrap path must honor both options. Update the
caller and `bootstrapSource` flow to accept and forward `expectedPackageScope`
and `commitMessage`, applying them to bootstrap publish validation and
commit-message generation instead of the generic defaults, while preserving
existing behavior when they are absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83c97597-1e19-4260-8088-17b2cdcdc00c
📒 Files selected for processing (11)
docs/use/community-packages.mddocs/use/packages.mdpackages/worker/client/routes/account.tsxpackages/worker/src/app/handlers/account-profile.node.test.tspackages/worker/src/app/handlers/account-profile.tspackages/worker/src/package-registry/username-change-packages.node.test.tspackages/worker/src/package-registry/username-change-packages.tspackages/worker/src/package-registry/username-scope-rewrite.node.test.tspackages/worker/src/package-registry/username-scope-rewrite.tspackages/worker/src/repo/repo-session-do.tspackages/worker/src/repo/source-sync.ts
Claim the username before publishing package rewrites so concurrent renames lose on the unique constraint, sync the full rewritten file tree to avoid bootstrap data loss, and keep projection refresh best-effort so published packages stay eligible for compensation. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 215c34c. Configure here.
| error: getErrorMessage(error), | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Stale listing name after refresh
Medium Severity
After a scope rewrite publish, refreshSavedPackageProjection failures are logged and ignored, but the account handler still auto-republishes community listings. publishCommunityListing sets listing name from the saved-package row, so a failed projection can leave the public listing on the old @{username}/… name even though the published source already uses the new scope.
Reviewed by Cursor Bugbot for commit 215c34c. Configure here.
| }), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Failed compensation leaves wrong scopes
High Severity
When a later package fails during the username cascade, compensation republish errors are only logged. The profile handler still rolls back users.username to the previous value. Packages that were already published under the new scope can remain there while the account username is old again, breaking scope consistency and later publishes.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 215c34c. Configure here.
| error: `Username was not changed because package updates failed: ${getErrorMessage(error)}`, | ||
| }, | ||
| 500, | ||
| ) |
There was a problem hiding this comment.
Rollback failure misreports username
High Severity
On package cascade failure the handler tries to restore the previous username, but if that rollback update fails it only logs the error and still responds that the username was not changed. The database can retain the new username while the client is told the rename was aborted.
Reviewed by Cursor Bugbot for commit 215c34c. Configure here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/app/handlers/account-profile.ts (1)
111-151: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRollback-failure path leaves the account permanently stuck with no way to retry.
If the package update throws and the compensating
db.updateback topreviousUsername(122-125) also fails, execution still falls through to the same response at 144-150:"Username was not changed because package updates failed"— but the username actually was changed and never reverted.Worse, this creates a stuck state with no self-heal: on the client's next request with the same desired username,
previousUsername = user.usernameis now read fresh from the DB (already the new value), so the fast path at Line 59 (username === previousUsername) short-circuits and returns success without ever re-attemptingupdatePackagesForUsernameChange. Packages/community listings remain permanently unrewritten under the old scope while the account shows the new username, and there's no code path left to retry the package sync.Consider tracking rollback success explicitly and, on rollback failure, returning a distinct response (e.g. a different status/message indicating manual follow-up or a "pending package sync" flag) so the fast path at Line 59 doesn't swallow future retries.
💡 Sketch of a safer fallback
} catch (error) { + let rollbackSucceeded = true try { await db.update(usersTable, user.userId, { username: previousUsername, updated_at: utcSqliteTimestamp(), }) } catch (rollbackError) { + rollbackSucceeded = false console.error( JSON.stringify({ message: 'username-change rollback failed after package error', userId: packageUserId, error: getErrorMessage(rollbackError), }), ) } void logAuditEvent({ category: 'account', action: 'update_username', result: 'failure', email: user.email, ip: requestIp, path: url.pathname, - reason: 'package_scope_update_failed', + reason: rollbackSucceeded + ? 'package_scope_update_failed' + : 'package_scope_update_failed_rollback_failed', }) return jsonResponse( { ok: false, - error: `Username was not changed because package updates failed: ${getErrorMessage(error)}`, + error: rollbackSucceeded + ? `Username was not changed because package updates failed: ${getErrorMessage(error)}` + : `Username was changed to @${username}, but package updates failed and could not be automatically undone: ${getErrorMessage(error)}. Please retry or contact support.`, }, 500, ) }🤖 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 `@packages/worker/src/app/handlers/account-profile.ts` around lines 111 - 151, Track whether the compensating update in the updatePackagesForUsernameChange error path succeeds. When rollback fails, return a distinct pending/manual-follow-up response and ensure the username-equals-previousUsername fast path does not report success or swallow a retry while package synchronization remains incomplete; preserve the existing failure response when rollback succeeds.
🤖 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.
Outside diff comments:
In `@packages/worker/src/app/handlers/account-profile.ts`:
- Around line 111-151: Track whether the compensating update in the
updatePackagesForUsernameChange error path succeeds. When rollback fails, return
a distinct pending/manual-follow-up response and ensure the
username-equals-previousUsername fast path does not report success or swallow a
retry while package synchronization remains incomplete; preserve the existing
failure response when rollback succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f605099-aca7-4b73-810d-c524fe7c83f4
📒 Files selected for processing (3)
packages/worker/src/app/handlers/account-profile.tspackages/worker/src/package-registry/username-change-packages.node.test.tspackages/worker/src/package-registry/username-change-packages.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/src/package-registry/username-change-packages.node.test.ts
- packages/worker/src/package-registry/username-change-packages.ts
3 participants
Summary
Username changes previously only updated
users.username. Saved packages kept the old@{username}/{kodyId}name, so later saves/publishes failed scope checks and community “by @…” stayed stale.This PR cascades username changes across packages:
package.json#nameand same-account@old/references (kody.dependencies, emits/subscriptions topic keys,kody:@imports)/accountthat package commits and third-party / dynamic invocations may be affectedTest plan
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@6ed468bc· Head:215c34caClassification: extends — username change now rewrites and republishes saved packages (and may republish community listings).
Primitives touched
saved-packagesapp-uirepo-sessionscommunity-listingsSystem map
Username save claims
users.usernamefirst, then rewrites package scopes and republishes eligible community listings.flowchart TD A["Account /profile.json<br/>POST username"] --> B["Claim users.username"] B --> C["updatePackagesForUsernameChange"] C --> D["Rewrite @old → @new<br/>full file tree"] D --> E["syncArtifactSourceSnapshot<br/>force publish + commit"] E --> F["refreshSavedPackageProjection<br/>best-effort"] F --> G{"Listing was on<br/>latest commit?"} G -->|yes| H["publishCommunityListing"] G -->|no| I["Leave listing pin"] C -.->|failure| J["Roll back username<br/>compensate packages"] style B fill:#f4c27a,stroke:#8a5a00,color:#000 style D fill:#f4c27a,stroke:#8a5a00,color:#000 style E fill:#f4c27a,stroke:#8a5a00,color:#000 style H fill:#9fd89f,stroke:#2f6b2f,color:#000 style A fill:#9fd89f,stroke:#2f6b2f,color:#000Legend: green = composes · amber = extended by this PR
Invariants
@{username}scope are not rewritten (called out in UI + docs).Docs
docs/use/packages.md— username scope cascadedocs/use/community-packages.md— auto-republish when listing was on latestSummary by CodeRabbit