Skip to content

feat: [ENG-2465] add web UI Configuration page + promote ByteRover provider onboarding#545

Merged
wzlng merged 8 commits intomainfrom
feat/ENG-2465
Apr 27, 2026
Merged

feat: [ENG-2465] add web UI Configuration page + promote ByteRover provider onboarding#545
wzlng merged 8 commits intomainfrom
feat/ENG-2465

Conversation

@wzlng
Copy link
Copy Markdown
Collaborator

@wzlng wzlng commented Apr 24, 2026

Configuration page

  • Git identity panel: get/set user.name & user.email via the daemon, one-click seed from the signed-in ByteRover account.
  • Remote panel: single origin, HTTPS-only validation, replace preview, Initialize VC CTA when git is uninitialized, "Find in ByteRover" link.
  • Shared SettingsSection + CalloutRow primitives; bg-card alignment with ConnectorsPanel; skeleton loading rows.

Provider onboarding

  • ByteRover pinned to top of the selector with an amber "Native" pill.
  • OAuth popup opens on row click (drops the redundant login-prompt confirmation); 800ms minimum visible delay before popup navigates.
  • ProviderFlowDialog mountable globally via provider-store so any surface can open it (task error CTA, etc.).

Error routing

  • toastVcError maps VC config gaps to a toast action that deep-links to /configuration#identity or #remotes.
  • Task ErrorSection shows "Configure provider" for provider-class errors; checks llmservice:error broadcast (task:error isn't always structured).
  • formatError overrides aligned with Configuration page copy.

Header

  • Responsive: Local badge hides <lg, HelpMenu/BranchDropdown/version collapse at narrower sizes, provider label truncates by breakpoint.
  • StatusDot primitive (tone + pulsing) — pulsing amber when unconfigured, green corner dot on the Plug when active.
  • brv vc init invalidates the remote query so panel auto-refreshes.

Other

  • ProjectDropdown: "Open local folder" opens OS file manager via new locations:reveal transport event (registry-guarded, no shell interp).
  • Global toast position = top-center; removed per-call overrides.
  • CSP: allow img-src https: for OAuth provider avatars.
  • BRV_GIT_REMOTE_BASE_URL surfaced via config:getEnvironment for the Remote URL input placeholder.

Tests

  • Pure util tests: detect-git-url-type, is-valid-email, validate-remote-url, is-provider-task-error, reveal-command.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 24, 2026

Claude finished @wzlng's task in 4m 34s —— View job


PR Review — feat: [ENG-2354] Configuration page + ByteRover provider onboarding

  • Gather context and understand the PR
  • Read key changed files
  • Review for correctness, security, and code quality
  • Post inline comments
  • Post final summary

Summary

This is a solid, well-scoped PR. The Configuration page design is clean with good UX patterns (skeleton loading, deep-link anchors, inline validation, "Use account" CTA). The security thinking on the reveal endpoint is particularly good. A few issues need addressing before merge.

Overall score: 4 / 5


Issues requiring attention

🔴 Bug — retry() in LoginPromptStep silently no-ops
The useEffect that calls start() has [popup, setLoggingIn] as its dep array. retry() resets didStartRef.current and triggers a state update, but neither dep changes, so the effect never re-fires. The UI shows the 'starting' spinner indefinitely with no actual login attempt. Fix: add a retryCount state and include it in the effect's deps. See inline comment on login-prompt-step.tsx:151.

🟠 Security — validateRemoteUrl accepts http:// URLs
detectGitUrlType maps both https:// and http:// to the 'https' bucket, so validateRemoteUrl lets plaintext HTTP remotes through. Since push/pull uses an HTTPS token, this could leak credentials in clear text. The fix is to split http:// into its own 'http' bucket and reject it explicitly. See inline comment on detect-git-url-type.ts:8.

🟡 Unhandled rejection — connectByteRover in onAuthenticated
The callback in provider-flow-dialog.tsx calls the async connectByteRover without a .catch. Errors are toasted internally, but the unhandled rejection still produces a console warning. See inline comment on provider-flow-dialog.tsx:429.


Smaller issues

  • DRY: hasCode type guard is copy-pasted between get-vc-config.ts and get-vc-remote.ts. Extract to a shared util.
  • Missing test: toast-vc-error.ts is the only new lib/ utility without a unit test. The CTA routing logic is worth pinning.
  • document.querySelector(hash): Works today, but getElementById(hash.slice(1)) is simpler and immune to CSS selector syntax edge cases.
  • ConnectorsPanel max-width duplication: The panel declares the same max-width constraints that ConfigurationPage already applies to its wrapper, creating a fragile sync requirement.
  • img-src https:: Acceptable for an open-ended provider list, but deserves a comment explaining why it's this broad.

Highlights

  • The registry-guard on locations:reveal before spawning the OS file manager is exactly right — the path comes from the client and must not be trusted blindly.
  • Promise.all([login(), sleep(MIN_VISIBLE_DELAY_MS)]) is an elegant way to enforce a minimum visible delay without a second setTimeout.
  • hadLlmServiceError fallback for provider errors that lose their structured code through the task error serializer is a thoughtful workaround for a real daemon limitation.
  • isSafeHttpUrl validation before window.open in the project dropdown is a good defence against tampered .git/config values.

Comment thread src/webui/features/vc/utils/detect-git-url-type.ts Outdated
Comment thread src/webui/features/vc/api/get-vc-config.ts Outdated
Comment thread src/webui/pages/configuration-page.tsx Outdated
Comment thread src/webui/features/connectors/components/connectors-panel.tsx Outdated
Comment thread src/webui/lib/toast-vc-error.ts
Comment thread src/server/infra/webui/webui-middleware.ts
Comment thread src/server/infra/transport/handlers/locations-handler.ts
wzlng added 2 commits April 25, 2026 06:25
…ovider onboarding

Configuration page
- Git identity panel: get/set user.name & user.email via the daemon,
  one-click seed from the signed-in ByteRover account.
- Remote panel: single `origin`, HTTPS-only validation, replace preview,
  Initialize VC CTA when git is uninitialized, "Find in ByteRover" link.
- Shared SettingsSection + CalloutRow primitives; `bg-card` alignment
  with ConnectorsPanel; skeleton loading rows.

Provider onboarding
- ByteRover pinned to top of the selector with an amber "Native" pill.
- OAuth popup opens on row click (drops the redundant login-prompt
  confirmation); 800ms minimum visible delay before popup navigates.
- ProviderFlowDialog mountable globally via provider-store so any
  surface can open it (task error CTA, etc.).

Error routing
- toastVcError maps VC config gaps to a toast action that deep-links to
  /configuration#identity or #remotes.
- Task ErrorSection shows "Configure provider" for provider-class
  errors; checks llmservice:error broadcast (task:error isn't always
  structured).
- formatError overrides aligned with Configuration page copy.

Header
- Responsive: Local badge hides <lg, HelpMenu/BranchDropdown/version
  collapse at narrower sizes, provider label truncates by breakpoint.
- StatusDot primitive (tone + pulsing) — pulsing amber when
  unconfigured, green corner dot on the Plug when active.
- `brv vc init` invalidates the remote query so panel auto-refreshes.

Other
- ProjectDropdown: "Open local folder" opens OS file manager via new
  locations:reveal transport event (registry-guarded, no shell interp).
- Global toast position = top-center; removed per-call overrides.
- CSP: allow img-src https: for OAuth provider avatars.
- BRV_GIT_REMOTE_BASE_URL surfaced via config:getEnvironment for the
  Remote URL input placeholder.

Tests
- Pure util tests: detect-git-url-type, is-valid-email, validate-remote-url,
  is-provider-task-error, reveal-command.
- Deep-link toast CTAs to /configuration#identity and #remotes so VC errors
  land on the exact panel the user needs to fix, not the page top.
- Split http:// detection from https:// in detectGitUrlType and surface a
  dedicated "Plain HTTP isn't supported" validator message.
- Extract the duplicated `hasCode` type guard from get-vc-config and
  get-vc-remote into src/webui/lib/transport-error.ts.
- Drop the duplicate sm/md/lg max-width on ConnectorsPanel — the parent
  wrapper in ConfigurationPage already applies the same constraints, so
  the self-constraint was a drift risk.
- Fix LoginPromptStep's retry(): the start effect's deps didn't include
  anything that changed on retry, so the UI got stuck on "Preparing
  sign-in…" forever. Added a retryCount trigger to the deps.
- Comment the deliberately broad `img-src https:` CSP directive: OAuth
  provider avatar CDNs can't be enumerated ahead of time.
- Unit-test toastVcError's CTA wiring (CONFIG_KEY_NOT_SET, NO_REMOTE,
  unknown-code fall-through) and the new http-vs-https url validation
  paths.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 24, 2026

Claude finished @wzlng's task in 2m 3s —— View job


Re-review — post-cleanup commit (1136381)

  • Gather context and understand the PR
  • Read files changed in the cleanup commit
  • Check each previously-flagged issue
  • Post updated review summary

The cleanup commit addressed all seven issues from the original review. Here's the per-issue accounting:


🔴 Bug — retry() silently no-ops → Fixed

login-prompt-step.tsx now has const [retryCount, setRetryCount] = useState(0) and the useEffect dep array is [popup, setLoggingIn, retryCount]. retry() increments retryCount, which forces the effect to re-run. The comment on line 101–103 accurately explains the intent. ✅


🟠 Security — validateRemoteUrl accepts http://Fixed

detect-git-url-type.ts now returns 'http' for http:// URLs (line 9), not 'https'. validate-remote-url.ts explicitly rejects 'http' with "Plain HTTP isn't supported — use an HTTPS URL." (line 15). Both test files (detect-git-url-type.test.ts and validate-remote-url.test.ts) have cases covering the http:// rejection path. ✅


🟡 Unhandled rejection — connectByteRover in onAuthenticatedNot changed

provider-flow-dialog.tsx was not touched in the cleanup commit; line ~422 still reads:

onAuthenticated={() => {
  connectByteRover(selectedProvider)
}}

The practical risk is low — connectByteRover catches all errors internally and toasts them — but the fire-and-forget call will still trigger a browser console warning when the callback fires. A one-line fix remains applicable: Fix this →


Nitpicks → all addressed

Issue Resolution
hasCode type guard duplicated Extracted to src/webui/lib/transport-error.ts; both get-vc-config.ts and get-vc-remote.ts now import from it ✅
Missing test for toast-vc-error.ts test/unit/webui/lib/toast-vc-error.test.ts added; covers CONFIG_KEY_NOT_SET CTA, NO_REMOTE CTA, and unknown-code plain toast ✅
document.querySelector(hash) selector risk Resolved by removing hash-based scroll entirely from configuration-page.tsx — the #identity/#remotes/#connectors anchor sections and the useEffect were dropped ✅
ConnectorsPanel max-width duplication Removed sm:max-w-lg md:max-w-xl lg:max-w-2xl from the panel; it now fills its parent with w-full
img-src https: needs a comment Comment added in webui-middleware.ts lines 38–42 explaining the open-ended provider CDN rationale ✅

One trade-off to note

Dropping the hash anchors simplifies the code but loses the scroll-to-section behaviour the PR description advertised. Toast CTAs (Set identity, Set remote) now navigate to /configuration without scrolling to the relevant panel. With three short panels on a single page this is probably fine, but worth a deliberate decision rather than a side-effect of the CSS selector cleanup.


Updated overall score: 4.5 / 5 — the one remaining issue (connectByteRover fire-and-forget) is low-severity. All structural and security concerns are resolved.

@bao-byterover bao-byterover changed the title feat: [ENG-2354] add web UI Configuration page + promote ByteRover provider onboarding feat: [ENG-2465] add web UI Configuration page + promote ByteRover provider onboarding Apr 25, 2026
Comment thread src/server/infra/transport/handlers/locations-handler.ts Outdated
- Replace the "Not set" callout (foreground title + dashed alert chrome) with
  a calmer account-led row: avatar + suggested identity, soft "from your
  ByteRover account — optional, used for commit attribution" sub-line, and
  ghost Edit / primary Apply actions. Fixes user feedback that the previous
  copy read as required-field-error.
- Drop the unused CalloutRow import from this file (kept for other panels).
- locations-handler: pass `windowsHide: true` to the reveal-folder spawn so
  the brief CMD flash is suppressed on Windows; minor format-only line-wrap
  on the REVEAL onRequest registration.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 25, 2026

Claude finished @wzlng's task in 1m 39s —— View job


Re-review — post-synchronize commit (d449491)

  • Gather context and understand new commits
  • Check windowsHide fix (blocking issue from bao-byterover)
  • Check connectByteRover fire-and-forget (remaining issue from prior review)
  • Review identity not-set row redesign
  • Post updated review summary

The new commit addresses the blocking windowsHide issue and ships the identity-row redesign. Here's the full accounting.


🔴 Blocking — windowsHide: true on spawnFixed

locations-handler.ts:126 now passes {detached: true, stdio: 'ignore', windowsHide: true} to spawn. The CMD flash on Windows is suppressed. ✅


🟡 Carry-over — connectByteRover fire-and-forget → Still not fixed

provider-flow-dialog.tsx:422 still reads:

onAuthenticated={() => {
  connectByteRover(selectedProvider)
}}

This remains a one-liner fix: Fix this →


Identity not-set row redesign — reviewed

The overall direction is correct — leading with the account avatar is a much calmer UX than the previous "Not set" alert box.

🟡 Minor — email shown twice when user.name is undefined

identity-panel.tsx:61:

const displayName = user.name ?? user.email

When the ByteRover account has no display name, displayName is the email address, and line 71 renders:

user@example.com <user@example.com>

Consider hiding the <email> suffix when displayName already is the email:

{displayName}
{user.name && (
  <span className="text-muted-foreground font-normal">&lt;{user.email}&gt;</span>
)}

Nitpick — initials() on an email address

initials('user@example.com') returns "US" (the first two characters) because @ and . are not split tokens. This is fine for the fallback avatar, but worth noting. Fix this →

Praise

The early-return split between !user (no account) and user (account found) is clean — two clearly distinct UX states with no tangled conditionals. The shrink-0 on the button group prevents it from collapsing on narrow panels. ✅


Summary

Issue Status
🔴 windowsHide on spawn ✅ Fixed in d449491
🟡 connectByteRover fire-and-forget ⏳ Still open (low severity)
🟡 Email shown twice when no display name New in d449491
Nitpick: initials() on email address New in d449491

Updated overall score: 4.5 / 5 — the blocking issue is resolved. Two low-severity items remain; neither is a merge blocker.

bao-byterover
bao-byterover previously approved these changes Apr 26, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Claude finished @wzlng's task in 2m 6s —— View job


Re-review — post-synchronize (merge from main, 3f4f6ae)

  • Gather context and understand new commits
  • Check remaining open issues from prior reviews
  • Check for new issues introduced by the merge
  • Post updated review summary

The synchronize event is a clean merge of main into the branch. The only files that changed are in server/infra/dream/ and server/infra/context-tree/ — the sortKeys frontmatter fix from PR #544. No feature code was touched; no conflicts were introduced.


Merge contents — nothing to flag

File Reason
summary-frontmatter.ts sortKeys: false to preserve curate's canonical field order
consolidate.ts Same sortKeys fix + mkdtemp flakiness fix
synthesize.ts Same sortKeys fix
Test files Updated assertions matching the new behavior

None of these interact with the Configuration page or provider-onboarding code. ✅


Carry-over issues (unchanged from d449491 review)

🟡 connectByteRover fire-and-forget — still open

provider-flow-dialog.tsx:422 still calls the async function without .catch():

onAuthenticated={() => {
  connectByteRover(selectedProvider)   // ← no .catch
}}

connectByteRover fully handles its own errors (lines 131–133 have a try/catch that toasts), so no user-visible impact — but the browser will log an unhandled-rejection console warning on error. One-liner fix: Fix this →

🟡 Email shown twice when user.name is undefined — still open

identity-panel.tsx:61–71:

const displayName = user.name ?? user.email   // email when no name
// ...
{displayName} <span>&lt;{user.email}&gt;</span>
// renders: "user@example.com <user@example.com>"

Fix: only render the <email> suffix when displayName !== user.email: Fix this →

Nitpick — initials() on an email address

src/webui/utils/initials.ts splits on [-_\s]+ only. initials('user@example.com') returns "US" (first two chars of the unsplit string) rather than something like "UE". Low impact for a fallback avatar, but the split regex could include @ and .: Fix this →


Summary

Issue Status
🟡 connectByteRover fire-and-forget ⏳ Still open (low severity)
🟡 Email shown twice when no display name ⏳ Still open
Nitpick: initials() on email address ⏳ Still open
Merge from main (3f4f6ae) ✅ Clean, no conflicts, unrelated files only

Overall score: 4.5 / 5 — merge is clean, no regressions. The two 🟡 items are not merge blockers, but fixing the double-email display would be a quick UX polish worth landing before the feature ships.

- Task retry: ErrorSection on the task detail now offers a "Try again"
  CTA alongside "Configure provider". A new `useComposerRetryStore` hands
  the failed task's content + type to whichever composer host is mounted
  (TaskListView in normal mode, TourHost in tour mode), which re-opens
  the composer prefilled. The task detail sheet closes on retry so the
  composer takes focus rather than stacking. Pure helpers
  (`composer-type-from-task`, the store) are unit-tested.

- Tour FSM rework for curate/query (steps 2/3): no more auto-mounted
  composer or auto-navigation — the user clicks through the highlighted
  Tasks tab → "New task" button so the IA itself is the lesson.
  - New `TourBackdrop`: page-wide dim + blur on every route during the
    composer steps. Click-blocking so a stray click on a blurred
    Configuration tab doesn't yank the user out of the tour.
  - New `TourPointer`: wraps a target with a primary-tinted glow and a
    hand-drawn curved arrow + label. Arrow is pinned to the target's
    center so the tip lands on the button regardless of the label's
    `align`; the curve mirrors itself based on which side the label sits
    on so the stick always flows from label to target.
  - Coachmark on the global Tasks tab (only when off /tasks). "New task"
    coachmark moves between the EmptyState and FilterBar buttons so we
    never highlight both at once.
  - TaskListView becomes the single composer host: reads tour state,
    prefills with the example + step label, sets `tourTaskId` on submit.
    `useTourWatchers` keeps only the provider→curate auto-advance.

- Failed tour task no longer surfaces "Continue": gated on
  `task.status === 'completed'` instead of `isTerminalStatus`. Banner
  copy branches per status. "Try again" broadened to all error codes so
  non-provider failures in tour mode aren't a dead end.

- VC remote delete: `RemotesPanel` gains a destructive ghost "Delete"
  button next to "Edit" with an `AlertDialog` confirm, reusing the
  daemon's `vc:remote { subcommand: 'remove' }` path landed via
  ENG-2466. `SetVcRemoteInput` tightened to a discriminated union so
  `remove` doesn't carry an unused url field.

- PrefillBadge moved bottom-left so it shares the textarea's reserved
  bottom band with the keyboard hint instead of overlapping the first
  line of the prefilled example.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Claude finished @wzlng's task in 4m 21s —— View job


Re-review — post-synchronize commit (0a99049)

  • Gather context and understand new commits
  • Check carry-over issues from prior reviews
  • Review new code (tour rework, task retry, vc remote delete)
  • Post updated review summary

The new commit adds tour FSM rework (steps 2/3), a TourPointer/TourBackdrop coachmark system, task-retry flow via ComposerRetryStore, and VC remote delete. Here's the full accounting.


Carry-over issues — status unchanged

🟡 connectByteRover fire-and-forget — still open

provider-flow-dialog.tsx:422 is still:

onAuthenticated={() => {
  connectByteRover(selectedProvider)   // ← no .catch
}}

This is now three review rounds without a fix. The practical risk remains low (connectByteRover catches internally), but the console warning will keep firing. Fix this →

🟡 Email shown twice when user.name is undefined — still open

identity-panel.tsx:71 still renders {displayName} <span>&lt;{user.email}&gt;</span> unconditionally when displayName equals user.email. Fix this →

Nitpick — initials() on email — still open

src/webui/utils/initials.ts:7 splits only on [-_\s]+, so 'user@example.com'"US". Fix this →


New code review

VC remote delete — clean

The discriminated-union approach in set-vc-remote.ts:

export type SetVcRemoteInput =
  | {subcommand: 'add' | 'set-url'; url: string}
  | {subcommand: 'remove'}

is a tidy improvement — TypeScript now rejects {subcommand: 'remove', url: '...'} at compile time. The server-side type IVcRemoteRequest keeps url?: string optional, so the cast in setVcRemote is safe.

The DeleteRemoteDialog.fire() wrapper correctly uses .catch(noop) — contrast with the still-open connectByteRover issue. ✅

UX detail: the dialog stays open on error (only closes on success), giving the user a chance to retry or cancel. Good decision.


Task retry — sound design, one missing dep

The ComposerRetryStore hand-off pattern (ErrorSection writes seed → TaskListView effect reads + clears) is clean. The consume() method atomically reads-and-clears in one store action, avoiding double-consume races. ✅

🟡 Minor — closeTask missing from useEffect deps in task-list-view.tsx:98

useEffect(() => {
  if (!retrySeed) return
  setComposer({...})
  closeTask()       // ← captured from closure, not in deps
  consumeRetry()
}, [retrySeed, consumeRetry])  // ← closeTask missing

closeTask is redefined each render (uses setSearchParams internally). Since retrySeed is in deps and the effect fires on change, the captured closeTask is always fresh at the moment it triggers, so this is safe in practice. But the exhaustive-deps rule would flag it, and a future refactor could break it. Wrapping closeTask in useCallback (its deps are just setSearchParams, which is stable) would make the deps correct. Fix this →


Tour FSM rework — correct, one observation

TourBackdrop + TourPointer + useTourWatchers are clean. The key design decisions hold up:

  • z-50 on TourPointer lifts the highlighted control above the z-40 backdrop → the coachmarked button stays clickable while the rest of the UI is dimmed and click-blocked. ✅
  • aria-hidden on both the backdrop and the arrow/label spans so the highlighted control itself remains accessible. ✅
  • snapshotIsProviderStep() named with a verb to signal it's a non-reactive store read — good discipline. ✅

Nitpick — TourPointer curveFrom direction for align="end"

const curveFrom: CurveFrom = align === 'end' ? 'left' : 'right'

For align="end", the label sits at right-0 (right edge of the target). A reader would expect the arrow stick to emerge from the right side of the SVG to connect toward the label — but 'left' starts the stick at x=6 (left side). The visual may look fine since the curve is small (6×12 Tailwind = 24×48px SVG), but the comment says "curves from the same side as the label", which doesn't match the code for end. If this is rendering correctly as designed, a clarifying comment or a rename (e.g. curveTowardLabel) would remove the ambiguity. Fix this →


Praise

  • bannerHint in tour-task-banner.tsx correctly branches on status === 'completed' (not isTerminalStatus) for the Continue CTA, so failed/cancelled tasks surface "Try again" without a confusing "Continue" alongside it. Clean UX logic.
  • The coachmark mutual-exclusivity (empty-state CTA vs filter-bar CTA via tasks.length > 0 guard in TaskListView:188) prevents the "two glowing buttons" problem cleanly.
  • Tour steps 2/3 now teach the user to navigate themselves (click tab → click "New task") rather than auto-navigating — this is the right pedagogical choice for onboarding.

Summary

Issue Status
🟡 connectByteRover fire-and-forget ⏳ Still open (3rd round)
🟡 Email shown twice when no display name ⏳ Still open
Nitpick: initials() on email address ⏳ Still open
🟡 closeTask missing from useEffect deps New in 0a99049
Nitpick: TourPointer curveFrom direction New in 0a99049

Overall score: 4.5 / 5 — the tour rework and retry flow are well-designed and the remote delete is clean. The three carry-over items (especially the double-email display) and the missing dep are worth addressing before the feature ships.

…s on query, badge alignment

- ComposerHeader: stop hiding the curate/query slider in tour mode. It now
  renders disabled with a Tooltip ("Locked during the tour — the next step
  covers the other mode."), so the user learns the affordance exists
  instead of meeting it cold after the tour ends. TypeSlider gained an
  optional `disabled` prop that applies `opacity-60` + `cursor-not-allowed`
  + `aria-disabled` and short-circuits hover styling.

- useTourWatchers: when the tour transitions into `query`, strip
  `?task=…` from the URL. Otherwise the just-finished curate task's
  detail sheet would still be open and would hide the FilterBar's
  "New task" button that the next coachmark points at. The setter
  returns `prev` unchanged when the param isn't present, so the effect
  doesn't trigger an unneeded navigation.

- TourStepBadge: add `inline-flex items-center leading-none` so the 10px
  label centers cleanly inside the 24px (h-6) pill instead of sitting on
  the default text baseline.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Re-review — post-synchronize commit (5e462c9)

  • Gather context and understand new commits
  • Check carry-over issues from prior reviews
  • Review new code (tour polish)
  • Post updated review summary

View job run

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Claude finished @wzlng's task in 2m 36s —— View job


Re-review — post-synchronize commit (5e462c9)

  • Gather context and understand new commits
  • Check carry-over issues from prior reviews
  • Review new code (tour polish)
  • Post updated review summary

The commit is small and focused: badge alignment, locked slider with tooltip, and task-detail auto-close on query step entry. All three changes are correct.


New code review

Badge alignment — clean

Adding inline-flex items-center leading-none to TourStepBadge is the right fix. The comment explaining leading-none (collapse line-height to glyph height for the 10px text inside the 24px pill) is exactly the kind of non-obvious justification worth keeping. ✅


useTourWatchers — task detail auto-close

The new useEffect is well-structured:

  • Functional update to setSearchParams is read-safe (no stale closure on prev)
  • The early-return if (!prev.has('task')) return prev avoids a no-op state update → no unnecessary re-render
  • {replace: true} skips polluting the browser history stack

✅ No issues.


Locked slider in tour

disabled on the <button> elements correctly prevents click events at the browser level. The Tooltip wrapping the slider div will receive onMouseEnter/onMouseLeave forwarded by TooltipTrigger render={...}, so hover-to-show works even though the inner buttons are disabled. ✅

Nitpick — aria-disabled on a non-interactive div

task-composer-header.tsx:62:

<div aria-disabled={disabled} ...>

aria-disabled has defined semantics on interactive roles (button, slider, etc.), but on a plain div it conveys nothing to assistive technologies. The child buttons already carry disabled natively — that's the correct and complete AT signal. The aria-disabled on the wrapper is harmless but needlessly misleading. Fix this →


Carry-over issues — all still open

🟡 connectByteRover fire-and-forget — 4th round

provider-flow-dialog.tsx:422 still reads:

onAuthenticated={() => {
  connectByteRover(selectedProvider)   // ← no .catch
}}

connectByteRover catches internally, so user impact is nil — but the browser console will log an unhandled-rejection warning on error. This has now been flagged in every review round since the original. Fix this →

🟡 Email shown twice when user.name is undefined

identity-panel.tsx:71:

{displayName} <span className="text-muted-foreground font-normal">&lt;{user.email}&gt;</span>

When user.name is undefined, displayName === user.email, so this renders user@example.com <user@example.com>. Fix this →

Nitpick — initials() on email address

src/webui/utils/initials.ts:7 splits on /[-_\s]+/ only. initials('user@example.com') returns "US" (the first two chars of the unsplit string). Fix this →

🟡 closeTask missing from useEffect deps — task-list-view.tsx:98

useEffect(() => {
  if (!retrySeed) return
  setComposer({...})
  closeTask()       // ← not in deps
  consumeRetry()
}, [retrySeed, consumeRetry])

closeTask is redefined each render but captured at effect-fire time (when retrySeed changes), so it's safe in practice. The exhaustive-deps lint rule would flag this though, and wrapping closeTask in useCallback([setSearchParams]) makes the deps array correct without any runtime cost. Fix this →

Nitpick — TourPointer curveFrom direction for align="end"

tour-pointer.tsx:89: const curveFrom: CurveFrom = align === 'end' ? 'left' : 'right' — the comment says the arrow curves from the same side as the label, but for align="end" (label at right-0) the code sets curveFrom='left'. If this renders correctly as designed, a clarifying comment would remove the ambiguity. Fix this →


Summary

Issue Status
🟡 connectByteRover fire-and-forget ⏳ Still open (4th round)
🟡 Email shown twice when no display name ⏳ Still open
Nitpick: initials() on email address ⏳ Still open
🟡 closeTask missing from useEffect deps ⏳ Still open
Nitpick: TourPointer curveFrom direction ⏳ Still open
Badge alignment (leading-none) ✅ Fixed in 5e462c9
Task detail auto-close on query step ✅ New in 5e462c9, clean
Slider visible-but-locked with tooltip ✅ New in 5e462c9, correct
Nitpick: aria-disabled on non-interactive div New in 5e462c9

Overall score: 4.5 / 5 — the tour polish commit is clean and purposeful. No new issues of substance were introduced. The five carry-over items (two 🟡 UX, two nitpicks, one longstanding fire-and-forget) remain the only things standing between this and a clean merge.

The previous strip-`?task=` effect on entering query re-fired whenever
`setSearchParams`'s reference changed (it's not stable across renders in
react-router-dom), so it stripped the URL again right after the user
submitted a query — closing the just-opened query detail sheet.

Gate the strip on `!tourTaskId` so it only runs at the curate→query
transition (when `advanceTour` has just cleared `tourTaskId`) and bails
once the user has submitted a query for this step. Also add `closeTask`
to the retry-seed effect's deps for completeness.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Claude finished @wzlng's task in 1m 46s —— View job


Re-review — post-synchronize commit (90e4913)

  • Gather context and understand new commits
  • Check carry-over issues from prior reviews
  • Review new code
  • Post updated review summary

The commit fixes the tour query-detail bug (task detail closing after a query is submitted during the tour) and makes two carry-over fixes. Here's the full accounting.


New code review

useTourWatchers — query detail fix

The root cause (per the commit message) was that setSearchParams is not reference-stable across renders in react-router-dom v6, so the strip-?task= effect re-fired whenever the component re-rendered — including after the user submitted a query in the tour. The fix gates the strip on !tourTaskId:

if (tourTaskId) return   // ← bail when a tour task is in flight

This is correct: advanceTour clears tourTaskId on the curate→query transition, triggering the strip exactly once. Once the user submits a query and tourTaskId is set again, the guard bails out and the detail sheet stays open. ✅

Note: setSearchParams is still in the dep array (which continues to re-trigger on reference changes), but the tourTaskId guard makes re-fires benign after a query is submitted. The approach works correctly.


Carry-over issues — status

closeTask missing from useEffect deps — Fixed

task-list-view.tsx:98 now reads [retrySeed, consumeRetry, closeTask]. ✅

TourPointer curveFrom direction — Fixed (comment clarified)

tour-pointer.tsx:85–93 now has a detailed comment explaining why curveFrom: 'left' is correct for align="end": the label is right-anchored but whitespace-nowrap makes it extend leftward, so curveFrom: 'left' keeps the stick flowing label → tip without doubling back. ✅


🟡 connectByteRover fire-and-forget — 5th round, still open

provider-flow-dialog.tsx was reformatted in this commit (import brace spacing) but the callback was not fixed. Line 438–440 still reads:

onAuthenticated={() => {
  connectByteRover(selectedProvider)   // ← no .catch
}}

connectByteRover catches internally (lines 127–133), so user impact is nil — but the browser will still emit an unhandled-rejection console warning on error. One-liner fix: Fix this →


🟡 Email shown twice when user.name is undefined — still open

identity-panel.tsx:61+71:

const displayName = user.name ?? user.email   // email when no name
// ...
{displayName} <span>&lt;{user.email}&gt;</span>
// renders: "user@example.com <user@example.com>"

Fix this →


Nitpick — initials() on an email address — still open

src/webui/utils/initials.ts:7 splits on /[-_\s]+/ only. initials('user@example.com') returns "US" (first two chars of the unsplit string). Fix this →


Nitpick — aria-disabled on non-interactive div — still open

task-composer-header.tsx:62:

<div aria-disabled={disabled} ...>

aria-disabled has defined semantics on interactive roles, but on a plain <div> it conveys nothing to assistive technologies. The child buttons already carry disabled natively — that's the correct and complete AT signal. Fix this →


Summary

Issue Status
🟡 connectByteRover fire-and-forget ⏳ Still open (5th round)
🟡 Email shown twice when no display name ⏳ Still open
Nitpick: initials() on email address ⏳ Still open
Nitpick: aria-disabled on non-interactive div ⏳ Still open
🟡 closeTask missing from useEffect deps ✅ Fixed in 90e4913
Nitpick: TourPointer curveFrom comment ✅ Clarified in 90e4913
Tour query-detail stays open after submit ✅ Fixed in 90e4913

Overall score: 4.5 / 5 — the tour detail fix is correct and well-reasoned. Two closed items. The one 🟡 that keeps appearing (connectByteRover fire-and-forget) and the double-email display are the only things worth addressing before the feature ships.

Copy link
Copy Markdown
Collaborator

@cuongdo-byterover cuongdo-byterover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Copy Markdown
Collaborator

@bao-byterover bao-byterover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wzlng wzlng merged commit 3c7f749 into main Apr 27, 2026
6 checks passed
@wzlng wzlng deleted the feat/ENG-2465 branch April 27, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants