Repository navigation
fix(identity): link Builderlab from Identity sync where its page holds the next step - #3082
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot review.
Reviewed head 01c105cd5ea380d3b206d6a484e009a17f1849ca against target/merge base 58116b826171d190a7376fc57b0509c32c507dde.
No must-fix findings in this change. The two status gates, live card-registration gate, existing Settings navigation/focus handoff and native consent-before-link-state ordering are consistent with the intended recovery path. The destination retains its own fresh identity check and destructive-switch confirmation.
Optional follow-up: the disclosed focus gap also affects this new link. In src/app/IdentitySettings.tsx:144-157, a status update or card removal can unmount the focused Builderlab button without moving focus to the surviving sentence. Consider a row-level fallback for both this link and the existing Sync button, with focused-removal regression coverage. This is an optional keyboard-usability follow-up, not a request to expand this PR.
Source-only review: no tests, builds or app workflows were run. Added tests were inspected, not executed. The PR description and both screenshots were inspected; the video was not. Native FI, real keyboard/screen-reader behavior, themes, widths and enlarged text remain unverified by this review. A single CI snapshot showed Rust and tool integration and CI required failing, with Windows native validation skipped; their causes were not investigated. This COMMENT is not approval or merge authorization.
…s the next step Identity sync names Builderlab when this key isn't linked to the account (waiting not_linked) or the account holds a different key (failed other_key_linked). The next step, Connect Buzz identity or Switch to this device's identity, is on Settings → Plugins → Builderlab, but the row had nothing to click. Those two sentences now render the first "Builderlab" as a SettingsSectionLink (#2122) to block.builderlab/login. The status text is unchanged; only the name becomes a link. Settings passes the opener only while the Builderlab card is registered, and reuses openSection, so focus moves into the opened section as it does from Decisions API. Without an opener the sentence stays plain. The linked states arise only after the user opted into automatic backup: nsec_backup.rs admits consent (NoConsent) before it reads the account's key (NotLinked, OtherKeyLinked). The link only navigates; connecting on Builderlab's page starts a backup the user already agreed to. Unlinked on purpose: - no_session: Builderlab signs in with the build's NIP-FI session, and that Organization sign-in is on this same Identity page. - no_consent: its next step is the row's own Sync with Builderlab, and Builderlab's page has nothing for it. - Retryable failures and signed_out clear on a new sign-in; the other failures have no fix on Builderlab's page. Self-review found and fixed: the Settings test's plugin-off check waited on the unsplit sentence, so a regression failed it for the wrong reason; it now waits on the row and asserts the button is absent. It also dropped a vacuous assertion and updated the help guide and identity doc. Known gap, shared with Sync: a status change while the link has focus drops focus to the page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com>
Review follow-up to the Identity sync → Builderlab link. The Settings test removed the Builderlab card while its own section was shown and then remounted Identity, so it proved the gate at mount only. It now also removes the card while Identity is open: the link goes in place and the same status region keeps the unchanged sentence, with no section switch. The mount-time check stays. A BackupRow that read its opener once (useRef) passed the old test and fails the new assertion. The help guide now says what each Builderlab action does, including that Switch to this device's identity permanently unlinks the account's current key, as the Builderlab page's own confirmation warns. Both docs name the card Settings → Plugins → Builderlab, the help guide's usual form; the identity doc had no other mention of the card. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com>
01c105c to
033f94a
Compare
kazemek21
left a comment
There was a problem hiding this comment.
Kyle reviewed this PR and the Nexus review-tracker notes and approved with no comments.
Posted by Codex (gpt-6.1-sol) on behalf of kylez.
…restricted-communities * origin/main: ci: give WebKit shards headroom and keep fixture servers' idle connections open (#3074) fix(messages): offer Retry as soon as a send fails (#3080) fix(agents): link Settings from key failures and quote the provider (#3081) fix(identity): link Builderlab from Identity sync where its page holds the next step (#3082) fix(startup): show a recoverable screen when Buzz fails to open or crashes (#3085) fix(attention): ask before deleting a watch (#3092) fix(agents): remove the owner-access read tools (#3123) fix(channels): keep the side pane mounted under the image viewer (#3111) fix(solo): ask before Escape or Close drops a new workspace's typed fields (#3114) # Conflicts: # docs/configuration.md # vite.config.ts
Why
Identity sync names Builderlab in two of its sentences: when this key isn't linked to the account (waiting
not_linked) and when the account already holds a different key (failedother_key_linked). The next step for both, Connect Buzz identity or Switch to this device's identity, is on Settings → Plugins → Builderlab, but the row had nothing to click. People had to find that page themselves.What
In those two sentences the first "Builderlab" becomes a
SettingsSectionLinktoblock.builderlab/login, the shared inline link from #2122. Settings passes the opener only while the Builderlab card is registered and reusesopenSection, so focus moves into the opened section as it does from Decisions API. Without an opener, or in any other state, the sentence stays plain text. The status text itself is unchanged; only the name becomes a link.Deliberately unlinked:
no_session: Builderlab signs in with the build's NIP-FI session, and that Organization sign-in is on this same Identity page.no_consent: its next step is the row's own Sync with Builderlab button; Builderlab's page has nothing for it.signed_outclear on a new sign-in. The remaining failures have no fix on Builderlab's page.The linked states arise only after the user opted into automatic backup:
nsec_backup.rsadmits consent (NoConsent) before it reads the account's key (NotLinked,OtherKeyLinked). The link only navigates. Connecting on Builderlab's page starts a backup the user already agreed to.How
BackupRowtakes an optionalopenBuilderlaband splits the existing feedback sentence around the first "Builderlab" for the two states.Settingsderives the opener fromopenSectionand the contributed card list. Two files of production change, about 40 lines, plus tests and the help guide and identity doc.Risk
Low. No native, protocol, or persistence changes. The shared link style makes the Identity sync row about 2 px taller in the linked states. Known gap, shared with the existing Sync button: a status change while the link has focus drops focus to the page.
Testing
pnpm typecheckand biome pass on01c105cd5.IdentitySettings.test.tsxandSettings.test.tsx: 124 tests pass. With main'sIdentitySettings.tsxandSettings.tsxswapped in, three fail: the two "links Builderlab from the … sentence to its Settings page" cases and "opens Builderlab from Identity sync only while its card is registered". The negative cases (no_session,no_consent, idle, running, complete, every other failure kind, and no opener) pass on both, as intended.BackupRowthat reads its opener once (useRef) passes the old test and fails this assertion.Settingscomponent in headless Chromium.Not verified: a native FI build, both themes, narrow widths, enlarged text, and keyboard navigation in the real app.
Before and after
The same pointer script runs on origin/main (left) and this branch (right): click "Builderlab" in the Identity sync sentence, go back to Identity, switch the backup status from waiting
not_linkedto failedother_key_linked, click "Builderlab" again.identity-sync-builderlab-navigation-before-after.mp4
Real: the
Settingscomponent, the side menu, the link, the section switch and the focus move. Faked: the backup status, and a fixture Builderlab card that renders the realLoginandHostedCommunitieson fake sign-in and account data, since the plugin registers its card only with a native session or the live development server. Recorded in headless Chromium with Playwright, not a native build.Bigger picture
Overlaps with #3069, which deletes
BackupRowand moves backup state into a key-card notice, and rewrites the same paragraphs indocs/buzz-help.mdanddocs/identity.md. Whichever lands second reconciles. If this lands first, #3069 can either carryopenBuilderlaband the card gate into the notice, splitting the period-lessnot_linkedandother_key_linkedstrings, or drop the prop and its tests on the grounds that its plugin-contributedBuilderlabIdentityRowalready links Builderlab from Identity. If #3069 lands first, this becomes a re-implementation in the notice, and the "only button in the row is the link" assertion has to allow for Retry backup.#2458 links the other direction: Builderlab's Hosted communities card opens Settings → Identity when the account's backed-up key differs from this device's. It is complementary.
🤖 Generated with Claude Code