Repository navigation
fix(forms): stop placeholders reading as values a blank field doesn't submit - #1887
Draft
dawsontoth wants to merge 1 commit into
Draft
dawsontoth wants to merge 1 commit into
dawsontoth wants to merge 1 commit into
Conversation
… submit
A placeholder that shows a real-looking value reads as already filled in, so
users either leave the field blank expecting that value or retype it by hand.
Each one now either is what a blank field submits, or reads as an example:
- OAuth Scope: a blank field now submits the default its placeholder shows.
Central manager stores the scope verbatim and it overrides the provider
preset, so a blank Scope signed in with an empty scope.
- Install Command (import + redeploy): the "npm install" placeholder said
what to type, but a blank field already gets Harper's automatic install,
and typing "npm install" opts out of it. The placeholder now says so.
- Okta domain, load balancer host name, HNSW index type and the API
explorer bearer token read as examples ("e.g. ...").
Also gives the OAuth provider card's icon-only edit button an accessible
name ("Edit provider").
Closes #1294
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves user experience and form validation by updating several input placeholders for clarity, adding accessibility labels, and ensuring that empty or whitespace-only OAuth scope inputs default to a standard fallback value. Additionally, a new test suite has been added to verify the OAuth scope behavior under various conditions. There are no review comments, and the changes look correct and well-tested.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
⊙ Problem
Some Fabric placeholders show a real-looking value that a blank field does not submit (#1294). Users read the field as already filled and leave it blank, or retype the grey text by hand. The worst case is OAuth Scope: a blank Scope saved an empty string, and that empty string replaces the provider's preset scope, so sign-in ran with no scope at all.
💡 Solution
I went through all ~70 placeholders in
srcand sorted each one with a single rule. A placeholder that shows a value must either be what a blank field submits, or read as an example. Instructions ("Search…", "Choose Role") and placeholders that already match what a blank field submits are left alone (see Left unchanged below).openid email profile, but a blank field sent''openid email profilecompany.okta.come.g. company.okta.comexample.your-company.come.g. cluster.your-company.comnpm installLeave blank to install dependencies automaticallynpm installis not the same thing: it opts out of the automatic install. Evidence belowHNSWe.g. HNSWeyJhbGciOi…e.g. eyJhbGciOi…Evidence for the behavioral claims (traced, not assumed):
''for a blank Scope. Central manager passes the stored value to@harperfast/oauthunchanged. That library'sbuildProviderConfigspreads it over the preset:config.ts:79-82@ v1.6.0, the version central manager pins. v2.9.0 still keeps an explicit''(config.ts:548-552). The authorize request then sendsscope: this.config.scope || ''(OAuthProvider.ts:76@ v1.6.0).Application.ts:4402). Otherwise it uses the package manager frompackage.jsondevEngines.packageManager(:4457), and falls back to npm with--omit=dev(:4540). A redeploy with a blank command writes noinstallblock (operations.js:793). Harper 5.0.0 and 4.7 use the same order: custom command, thendevEngines, then npm.CUSTOM_INDEXESholds onlyHNSW(customIndexes.ts:4), and a blank type returns early as a standard index (databases.ts:3482). Distance left blank is cosine (HierarchicalNavigableSmallWorld.ts:543-548), so itscosineplaceholder stays.Left unchanged
idand Databasedata, Import Data's Databasedata, the schema editor's Databasedata, table/endpoint name overrides and Distancecosine, and the new application's Name.Choose …selects,Your password,Leave blank to keep existing,Okta Client ID/Secret,Optional,Page.e.g. us-east-1,e.g. 100OFF,e.g. example.com, *.example.com,e.g. https, operations, wss,e.g. 280ms, medium distribution.you@company.comon the auth pages reads as a template. The secret editor's********deliberately signals a hidden existing value, next to Reveal. The cluster instance protocol select'shttps://placeholder never shows, becausesecureis always set. In the admin-only notification composer, the link sample lists two alternatives joined by "or", and a blank message is rejected when you submit.Skipped (would conflict)
https://example.com/data.csv→e.g. https://example.com/data.csv. I skipped it because open PR feat(status): list Harper jobs on a Jobs tab, and follow a CSV import's job in a toast #1880 addsImportDataModal.progress.test.tsx, which queriesgetByPlaceholderText('https://example.com/data.csv'). That test would break when the two merge. It is a one-line follow-up once feat(status): list Harper jobs on a Jobs tab, and follow a CSV import's job in a toast #1880 lands.🔧 Changes
src/features/organization/settings/index.tsx:DEFAULT_OAUTH_SCOPEconstant. It is used as the add form's default value and in both Scope placeholders (edit, add).e.g.(edit, add).aria-label="Edit provider", which the new test also uses to find it.e.g.:ClusterFQDN.tsxFieldRow.tsxSettingsPanel.tsx(dprint re-wrapped this line)RedeployApplicationModal.tsxandImportInstructions.tsx.✅ Verification
src/features/organization/settings/index.test.tsxrenders the real page against a mockedapiClient. It checks three things: that a blank Scope on add sends exactly the placeholder's value, that an edit of a provider with no stored scope sends the default, and that a typed scope is kept. Mutation-checked: with both fallbacks reverted, the first two tests fail (expected ' ' / '' to be 'openid email profile').stagequeries those placeholders, and none of the open PRs that touch these files changes or queries them (checked feat(deploy): follow a rolling deploy's restart job, and show canary certification #1868, Show a failed app import or redeploy once, and keep a failed import beside its form #1871, feat(deploy): stage a release now and activate it later (Harper 5.3+) #1876, fix(sidebars): resize every sidebar with one pointer hook that never sticks mid-drag #1805 and fix(auth): finish sign-out when a storage write throws #1813). feat(status): list Harper jobs on a Jobs tab, and follow a CSV import's job in a toast #1880 does query the CSV URL placeholder, which is why that row was skipped.npx vitest runon all touched directories, 19 files / 221 tests, exit 0.npx tsc -bexit 0,npx oxlintexit 0,npx dprint checkexit 0. The pre-commit hook ran the full suite (396 files, 3,666 passed / 11 skipped), oxlint, dprint and commitlint, all green.git fetch. The Harper domain adjudicator failed on an expired Claude OAuth session, so the outside findings were triaged by hand:@/features/organization/settings, and instead of a comment explaining an unnamed-button selector, the edit button has an accessible name. I also renamed the add test, since gemini pointed out it clears Scope to whitespace rather than leaving it untouched.transform. Both schemas are module-private with one consumer each. A transform would splituseForm's input and output types to protect a hypothetical second consumer.FormDescriptionwould also collide with open PR feat(deploy): stage a release now and activate it later (Harper 5.3+) #1876, which adds that same import to both files.DEFAULT_OAUTH_SCOPE. It states a constraint the code can't show: a blank scope is stored as-is and overrides the provider preset.Closes #1294
🤖 Generated with Claude Code
Related PRs: none found
Complexity: easy
Review-Coverage: authored=claude; ran=gemini,codex; blocked=cursor-grok(no-receipt),cursor-composer(no-receipt),domain(auth); declined=cursor-kimi,cursor-muse; rounds=1; full=1 @ 53c9df4
Review-Attention: study ~11m (critical: FieldRow.tsx; raised: degraded review) @ 53c9df4