PT-4363: Internet & Connectivity settings UI refinements - #2695
Open
katherinejensen00 wants to merge 4 commits into
Open
PT-4363: Internet & Connectivity settings UI refinements#2695katherinejensen00 wants to merge 4 commits into
katherinejensen00 wants to merge 4 commits into
Conversation
katherinejensen00
requested review from
irahopkinson,
jolierabideau,
lyonsil,
rolfheij-sil and
tjcouch-sil
as code owners
August 19, 2026 18:42
katherinejensen00
force-pushed
the
pt-4363-internet-settings-ui-refinements
branch
from
August 23, 2026 00:25
ae84c98 to
85a1038
Compare
Option descriptions move from always-visible body copy into a hover/focus tooltip, signalled by a new info icon on each row; the wizard step gains the "Internet & connectivity" headline and a short subtitle; the developer server picker becomes radio buttons with Test on its own row; and the "Disabled options are planned for future updates" footer is removed, since the per-row "Coming soon" badge already says it. Two things surfaced while building it and are included: - The first-run gate is an opaque dialog at Z_INDEX_FIRST_RUN (700) while tooltips portal to document.body at Z_INDEX_TOOLTIP (550), so any tooltip in the wizard would have painted behind the gate. TooltipPortalContainerProvider portals them into the gate instead. - That workaround already existed for popovers, so both primitives now build from one shared createPortalContainerContext factory rather than two copies inside vendored shadcn files. Also adds a shared WizardStepHeading and converges four of the five wizard steps onto it; PT-4402 tracks the remaining SyncProgressStep headings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-URL: <session URL>
Its two headings — the in-progress and complete branches — were the last
hand-rolled step headings in the wizard, at tw:font-medium while the other
four steps had moved to WizardStepHeading's tw:font-semibold. Shipping one
outlier out of five is worse than either consistent state.
They are step headings despite sitting inside role="status": the two are
mutually exclusive branches of a single step title ("Syncing your projects."
-> "Sync complete"), they were already marked up as <h2>, and role="status"
governs screen-reader announcement rather than heading semantics.
Retires the deferral note in WizardStepHeading's TSDoc and the
WeightMismatchToday story, which existed only to make the mismatch visible.
Not visually verified — the app does not start in this worktree. See PT-4402
for the revert path if UX considers these a distinct class of heading.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-URL: <session URL>
dist/ conflicted again on this rebase — main rebuilt the bundles too, and the hash-named resizable chunks were renamed on both sides. Resolved by taking main's dist wholesale and regenerating it from the rebased source, rather than hand-merging minified output. Verified the rebuilt output carries both sides (TooltipPortalContainerProvider from this branch, the Empty exports from main) and that every entry point — index.js/.cjs and experimental.js/.cjs — references only chunks present on disk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-URL: <session URL>
katherinejensen00
force-pushed
the
pt-4363-internet-settings-ui-refinements
branch
from
August 23, 2026 01:35
85a1038 to
cc36191
Compare
…n gate Nothing demonstrated the case the portal fix exists for: a tooltip opened inside the opaque gate, which stacks at Z_INDEX_FIRST_RUN (700) above the Z_INDEX_TOOLTIP (550) layer that tooltips portal into. Without TooltipPortalContainerProvider such a tooltip paints behind the gate and is invisible, and no story made that visible. Uses FirstRunGate's documented stepComponents override to inject a body that renders the real InternetAccessOptionList. The real step cannot be used here: it waits on a PAPI data provider that has no backend in Storybook, so it would only ever show a spinner. Every other step keeps its production component by spreading DEFAULT_STEP_COMPONENTS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-URL: <session URL>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PT-4363: Internet & Connectivity settings UI refinements
Base:
origin/main· Files: 28 source + 17 generateddist/· Reviewed with: Claude Opus 5 · Date: 2026-08-19Summary
Refinements to the Internet & Connectivity settings, which appear in two places — the standalone web
view and the first-run wizard step. Both are driven by shared
platform-bible-reactcomponents, somost changes land once and apply to both.
always-visible description paragraphs are gone.
plus a short subtitle.
its own row alongside Production and Development.
soon" badge already says it, and dropping the line keeps the wizard's Next button in view.
Two things surfaced while building it, and are included:
Z_INDEX_FIRST_RUN(700); tooltips portal todocument.bodyatZ_INDEX_TOOLTIP(550). Anytooltip in the wizard would have painted behind the gate — silently defeating change 1 in half
the places the ticket asks for it. Fixed by portalling tooltips into the gate.
of two copies inside vendored shadcn files.
Also folded in: a shared
WizardStepHeading, with all five wizard steps converged onto it —the wizard previously rendered step headings at two different font weights.
Reviewing this efficiently
advanced/internet-access-option-list/internet-access-option-list.component.tsxandcomponents/portal-container.context.tsx.first-run-overlay.component.tsxlooks huge and isn't. ~150 changed lines, ~14 real — therest is indentation from wrapping the dialog's children in a provider. Use
git diff -w, or the"Hide whitespace" toggle in GitHub's diff view.
lib/platform-bible-react/dist/(17 files) — generated bynpm run build:pbr, committedbecause the package tracks its build output.
What changed, by file
Shared components —
lib/platform-bible-reactadvanced/internet-access-option-list/…component.tsxsr-onlycopy wired to each radio viaaria-describedby; drops the footer; instance-scoped idsadvanced/developer-section/…component.tsxToggleGroup→RadioGroup; Test gains its own row;isSurfaced()decides which values display as themselves vs collapse to Productionshadcn-ui/tooltip.tsxTooltipPortalContainerProvider— the z-index fixshadcn-ui/popover.tsxcomponents/portal-container.context.tsxcreatePortalContainerContext()— one definition, one context per primitivesrc/index.tsWizard —
src/renderer/components/first-runfirst-run-overlay.component.tsxsteps/internet-settings-step.component.tsxwizard-step-heading.component.tsx<h2>, used byWizardStepFormand directly by steps that own their layoutsteps/language.component.tsxWizardStepHeadingsteps/sync-progress.component.tsxWizardStepHeading, retiring the last hand-rolledfont-mediumstep headingLocalization —
extensions/src/paratext-registration/contributions/localizedStrings.json%internetSettings_subtitle_short%— the wizard step's subtitle%paratextRegistration_internetUse_footer%, with adeprecationInfonote and no replacement keyThe removal follows the immutability convention: a
deprecationInfoentry records why, rather thanediting a live string's meaning in place.
Tests & stories
New:
tooltip.test.tsx,popover.test.tsx(both portal targets, both verified falsifiable bysabotage),
tooltip.test-utils.ts,wizard-step-heading.component.stories.tsx, and abundled-extension Storybook story for
DeveloperSectionthat renders it with real localized strings.Updated: both component tests, the first-run overlay test (now guards the z-index wiring), the
language and internet-settings step tests, and both lib story files.
E2E —
e2e-tests/tests/isolatedinternet-settings.page.tsgrew a page object;internet-settings.spec.tswas refactored onto it.This spec was previously broken — see Needs a decision #3.
Needs a decision from you
Guidelines/Tooltipssays "a tooltip is a hint, notan explanation" and its Avoid example is 68 characters; these descriptions run 67–162
characters, up to two sentences. The author chose to keep the full descriptive copy (shortening
needs new strings) and add the info icon for discoverability. This wants explicit UX sign-off,
not silent acceptance.
Guidelines/Product namesis wrong. It states "'Send/Receive' is now called Sync". Per theauthor these are distinct operations — Sync follows up a Send/Receive by also handling
connected resources and model texts. As written, that page will keep prompting engineers to
rename Send/Receive → Sync across the 10 strings that legitimately use it. Route to its owner.
internet-settings.spec.tssits at the top level oftests/isolated/, whichrun-isolated.mjscannot select (it enumerates subdirectories), and CIruns only
test:e2e:smoke. That is how a call toFrameLocator.filter()— a method that doesnot exist — survived in it. This PR fixes that call and a case-sensitivity bug beside it, but
wiring the suite into a real run is a team decision.
Verification
npm run typechecknpm run lint(repo-wide, incl. lib workspace)npx typedocsrc/renderer/components/first-runplatform-bible-reactunit(jsdom)platform-bible-reactstorybook (chromium)Two gaps, stated plainly:
(
dev-packages/scripture-editorshas ~900 dirty entries, solink-dev-packagesaborts and takesnpm startwith it). The fix is reasoned, unit-tested at the mechanism level, andregression-guarded at the wiring level — but nobody has seen it. Please eyeball it on a working
machine.
source gave 8 failures / 1047 tests collected, then 26 / 685 — machine saturation, not code. The
giveaway:
badge.stories.tsx > Defaultis a static render with no play function and it failed onTest timed out in 15000ms, i.e. the runner never reached it. The deterministicunitprojectcovers every component this branch touches and is green. Let CI arbitrate the browser project.
Full review record — findings, API surface, interview notes
API Changes
TooltipPortalContainerProvider(shadcn-ui/tooltip.tsx, re-exported fromsrc/index.ts) — props{ container: HTMLElement | null; children: React.ReactNode }.PopoverPortalContainerProvider— implementation moved from afunctiondeclaration to aconstbound to the shared factory; emitted declaration changes from
export declare function …toexport declare const …. Props type and runtime behavior unchanged.src/components/portal-container.context.tsxexportingcreatePortalContainerContext— deliberately not exported fromsrc/index.tsor theexperimental entry point.
DEVELOPER_SECTION_STRING_KEYS— gained%paratextRegistration_label_serverType_option_Test%.Consumers pass the resolved map straight through, so no consumer change is required.
INTERNET_ACCESS_OPTION_LIST_STRING_KEYS— dropped the footer key. Still derived fromOPTION_ROWS, so it stays in sync automatically.DeveloperSectionProps/InternetAccessOptionListProps— no type changes; TSDoc wording only.WizardStepHeading— new, renderer-internal, not a published package API.lib/platform-bible-utils,lib/papi-dts/papi.d.ts, extension*.d.ts: no changes.No exports removed and no signatures narrowed.
Findings
23 findings across four analysis passes (API/correctness, style/patterns, coverage/compliance, UX).
Critical — all fixed in review
{@link createPortalContainerContext}could not resolve (the factoryis internal) and
typedoc.jsonsetsinvalidLink: true+treatWarningsAsErrors: true, sonpx typedocexited 4 — breakingnpm run buildfor the package and thepublish-docsCIjob on merge. (fixed: inlined a self-contained contract on both providers, which also resolved
the TSDoc finding below; typedoc now exits 0)
@typescript-eslint/no-shadow×2 —radioId/descriptionIdparameters namedvalueshadowed the
valueprop. (fixed: renamed tooptionValue)no-type-assertiononrow as HTMLElementin the hover story. (fixed:querySelector<HTMLElement>+ explicit throw, which also made that story falsifiable)All three were introduced by the author's own final edits and missed by earlier checking — see
Verification gaps below.
Important
Tooltip copy violates(Author kept the descriptive copyGuidelines/Tooltips.deliberately. Escalated to Alex — see Needs a decision #1.)
clicking through never saw one. (fixed:
Infoicon per row. Deliberately decorative —aria-hidden, no tab stop — since making it the sole trigger needs an accessible name, i.e. anew localized string, plus five extra tab stops. The whole row stays the trigger.)
PopoverPortalContainerProviderlost its inline contract.Code-Style-Guide.md#api-surface-tsdoc-requirementsrequires these to be self-contained.(fixed: contract inlined into both)
WizardStepHeadinghad no story — the only component infirst-run/without one.(fixed: three stories, incl.
WeightMismatchTodayshowing the remaining weight mismatch)pass-through and ignored the new
ref, so deleting either left all tests green. (fixed: thestub records its
container,DialogContentStubforwards its ref, and a new test asserts both.Verified falsifiable — removing the
reffails it; removing the wrapper fails it.)Bare provider aliases share a runtime name in React DevTools and use an anonymous props(Cosmetic/dev-tooling only; TSDoc now carries the contract. Left as-is.)type.
Minor
Fixed: developer-section row padding now matches the option rows · row-wide hover highlight now
matches the clickable area in both lists · raw
<label>vs shadcnLabelasymmetry documented(
Label'speer-disabled:is a sibling combinator and the option-list label is nested twolevels down, so
Labelgenuinely cannot work there) · unusedexport defaultremoved · Reactact()warning fixed · addedpopover.test.tsx(verified falsifiable) · the[data-slot="tooltip-content"]selector de-duplicated across the two vitest harnesses.Dismissed: "Send/Receive" → "Sync" — the guideline is wrong, not the strings (see Needs a
decision #2).
Open, left as reviewer judgment: coming-soon rows are
disabledand so unreachable by sightedkeyboard-only users (screen readers get the
sr-onlycopy; mouse users get hover) · placement ofportal-container.context.tsxincomponents/vs the existingsrc/context/.Template propagation
None. No changed file carries a
#region shared withmarker.Positive observations
ToggleGroup→RadioGroupswap matchesComponent-Selection-Quick-Reference.md.item is clicked, stranding a user on a hidden server value with no one-click route back to
Production — was caught, handled, documented at the call site, and covered by tests including a
negative case asserting a Test user is not silently re-routed.
data-testids survived the swap, and the e2e spec addstoHaveRole('radio')so they can't driftback onto a different control.
useId(), fixing real duplicate-id collisions on Storybook autodocs pages.tooltip.tsx/popover.tsxsits under a// CUSTOM:comment.deprecationInfonote rather than deletedsilently, matching the string-immutability convention.
stylesheet; Playwright counts
sr-onlyas visible) — the kind of note that stops a correctassertion being "simplified" into a wrong one later.
toBeVisible()on[data-slot="tooltip-content"]rather thangetByRole('tooltip'), which Radix puts on a visually-hidden copy — precisely the assertion thatcatches the bug this PR fixes.
Interview notes
Author decisions during review: keep the full descriptive tooltip copy and add an info icon
(knowing deviation from the guideline, not an oversight) · leave
proper-names.mdxalone despite itserror · do not rename the Send/Receive strings · track the remaining heading convergence as
PT-4402 · land everything as one PR.
The Send/Receive correction is worth highlighting. The analysis assumed Sync was simply the new
name for Send/Receive, following the written guideline. The author corrected it: they are distinct
operations. That invalidated the finding and redirected it at the guideline page instead. No areas
were deferred to AI or left unexplained; no unresolved items.
Verification gaps this review exposed
These explain how three build-breaking issues survived earlier "all green" checks:
eslint --cachemasked real errors. A repo-rooteslint .reported clean while thelib/platform-bible-reactworkspace — whichnpm run lintalso runs, with stricter rules — had 3errors. Clearing
.eslintcacheand running the workspace lint directly surfaced them.build:basicskips TypeDoc.npm run buildisbuild:basic && lint-fix && typedoc; only thefull script validates doc links, so the TypeDoc break was invisible to every
build:basicrun.Follow-up
SyncProgressStepheadings are mutually exclusive branches of a single step title ("Syncing yourprojects." → "Sync complete"), already marked up as
<h2>; therole="status"wrapper governsscreen-reader announcement, not heading semantics. Leaving one step at
font-mediumwhile theother four moved to
font-semiboldwould have shipped a lone outlier — worse than eitherconsistent state. All five steps now share
WizardStepHeading, and the deferral note plus theWeightMismatchTodaystory that documented the mismatch are removed.AI-assisted — session
This change is