fix(frontend): streamline web ux#916
Conversation
There was a problem hiding this comment.
Pull request overview
This PR streamlines the feed conversion UX by normalizing user-entered URLs, improving conversion retry behavior + preview loading UX, and making access-token persistence more robust across restricted browser storage contexts.
Changes:
- Add URL normalization (frontend + backend) so hostname-only inputs are coerced to
https://…before feed creation and bookmarklet auto-submit. - Update conversion flow to publish results immediately, hydrate preview async, and support automatic fallback retry (with UI messaging).
- Prefer persistent storage (localStorage) for tokens and add legacy session-token migration/cleanup.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/html2rss/web/api/v1_spec.rb | Adds API spec coverage for hostname-only URL normalization on feed creation. |
| public/shared-ui.css | Ensures new brand lockup link styling doesn’t inherit underlines. |
| frontend/src/utils/url.ts | Introduces shared frontend URL normalization + validation helpers. |
| frontend/src/styles/main.css | Adds spacing for new notice action area. |
| frontend/src/hooks/useFeedConversion.ts | Implements URL normalization, async preview hydration, and automatic fallback retry metadata. |
| frontend/src/hooks/useAuth.ts | Prefers localStorage over sessionStorage for auth persistence with fallback handling. |
| frontend/src/hooks/useAccessToken.ts | Prefers localStorage, migrates legacy session token, and attempts to clear old session copies. |
| frontend/src/components/ResultDisplay.tsx | Adds “Subscribe in reader” link, retry notice, and preview-loading UI state. |
| frontend/src/components/DominantField.tsx | Extends input props (inputMode, autoCapitalize, spellcheck) and fixes autoComplete casing. |
| frontend/src/components/Bookmarklet.tsx | Simplifies bookmarklet URL base calculation and uses location.assign. |
| frontend/src/components/AppPanels.tsx | Makes URL entry more permissive + adds retry CTA; updates token entry field attributes. |
| frontend/src/components/App.tsx | Normalizes URLs on submit/auto-submit, prefers faraday by default, and wires manual retry UX. |
| frontend/src/api/contracts.ts | Extends result contract with preview loading state + retry metadata. |
| frontend/src/tests/useFeedConversion.test.ts | Updates unit tests for async preview hydration, URL normalization, and auto-retry behavior. |
| frontend/src/tests/useFeedConversion.contract.test.ts | Extends contract tests for new preview loading field. |
| frontend/src/tests/useAuth.test.ts | Updates tests for localStorage-backed auth persistence. |
| frontend/src/tests/useAccessToken.test.ts | Adds coverage for localStorage persistence and legacy session migration/cleanup. |
| frontend/src/tests/ResultDisplay.test.tsx | Adds assertions for subscribe link, loading preview state, and retry notice. |
| frontend/src/tests/App.test.tsx | Updates tests for new default strategy, permissive URL field, auto-submit behavior, and manual retry CTA. |
| frontend/src/tests/App.contract.test.tsx | Updates contract expectations for faraday default + localStorage token usage. |
| app/web/security/url_validator.rb | Adds canonical_url wrapper and routes validation/allowlist checks through it. |
| app/web/api/v1/create_feed.rb | Normalizes hostname-only/// inputs before canonicalization + validation. |
| type="text" | ||
| class="input input--mono input--minimal" | ||
| aria-label="Access token" | ||
| placeholder="Paste access token" | ||
| autocomplete="off" | ||
| autoComplete="off" |
There was a problem hiding this comment.
The access token input was changed from type="password" to type="text", which exposes the token in cleartext (shoulder-surfing/screen recording) and may weaken user expectations around secret entry. Consider keeping this as a password field (or making visibility an explicit opt-in toggle) while retaining the autocomplete/manager-ignore attributes.
| const legacyToken = | ||
| token || typeof window === 'undefined' | ||
| ? '' | ||
| : (window.sessionStorage?.getItem(ACCESS_TOKEN_KEY)?.trim() ?? ''); | ||
|
|
There was a problem hiding this comment.
legacyToken is read directly from window.sessionStorage inside the effect. In restricted contexts where sessionStorage access throws (the case this PR mentions), this read will throw and put the hook into an error state, even though localStorage (or memoryStorage) would work fine. Consider wrapping the legacy-session read in its own try/catch (or gating it behind a successful sessionStorage probe) so restricted sessionStorage access is treated as “no legacy token” instead of a hard failure.
| const legacyToken = | |
| token || typeof window === 'undefined' | |
| ? '' | |
| : (window.sessionStorage?.getItem(ACCESS_TOKEN_KEY)?.trim() ?? ''); | |
| let legacyToken = ''; | |
| if (!token && typeof window !== 'undefined') { | |
| try { | |
| legacyToken = | |
| window.sessionStorage?.getItem(ACCESS_TOKEN_KEY)?.trim() ?? ''; | |
| } catch { | |
| // Treat restricted sessionStorage access as "no legacy token". | |
| legacyToken = ''; | |
| } | |
| } |
| return `Tried ${requestedStrategy} first, then ${fallbackStrategy}. ${secondMessage}`; | ||
| } | ||
|
|
||
| return `Tried ${requestedStrategy} first, then ${fallbackStrategy}. ${secondMessage}`; |
There was a problem hiding this comment.
buildRetryFailureMessage has an if (firstMessage === secondMessage) branch, but both branches currently return the exact same string (and firstMessage is otherwise unused). This is effectively dead logic and makes the intent unclear. Either remove the conditional/unused variable, or include the first failure’s message when it differs so the combined error is more informative.
| return `Tried ${requestedStrategy} first, then ${fallbackStrategy}. ${secondMessage}`; | |
| return `Tried ${requestedStrategy} first, then ${fallbackStrategy}. First attempt failed with: ${firstMessage}. Second attempt failed with: ${secondMessage}`; |
Summary
Validation