feat: Align result and feed UI#933
Conversation
13faf80 to
887bd23
Compare
There was a problem hiding this comment.
Pull request overview
This PR aligns the UI styling and user experience between the main frontend “result” screen and the RSS/XSL feed-rendered HTML, while also modernizing the frontend toolchain (pnpm + ESLint + Stylelint) and improving feed “readiness” handling.
Changes:
- Unifies design primitives via
public/shared-ui.cssand updates RSS XSL (public/rss.xsl) to use the shared hero/actions styling plus richer per-item “quality” signals. - Adds a feed-readiness lifecycle to the frontend conversion flow (retries, explicit states, and a manual “retry readiness” action) and updates UI/tests accordingly.
- Migrates frontend workflows from npm to pnpm, adds ESLint/Stylelint configs, and updates CI/devcontainer/Docker build steps.
Reviewed changes
Copilot reviewed 46 out of 53 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| stylelint.config.mjs | Root Stylelint config extending the frontend ruleset. |
| spec/public/rss_xsl_spec.rb | Adds coverage for RSS XSL output structure and text sanitization behavior. |
| spec/html2rss/web/feeds/responder_spec.rb | Refactors expectations into helpers for readability. |
| spec/html2rss/web/app_spec.rb | Avoids static-route caching collisions by randomizing the path. |
| public/shared-ui.css | Expands shared design tokens + adds shared hero/button primitives. |
| public/rss.xsl | Redesigns feed HTML output to match shared UI and adds quality indicators/readiness stamp. |
| public/feed-reader-link.js | Adds client-side wiring for “Open in feed reader” link. |
| Makefile | Switches frontend tasks to pnpm; expands linting to include ESLint + Stylelint. |
| Gemfile.lock | Updates a few gem versions and removes ssrf_filter dependency entries. |
| Gemfile | Removes the unused ssrf_filter gem. |
| frontend/vitest.config.js | Normalizes timeout numeric formatting. |
| frontend/stylelint.config.mjs | Introduces frontend Stylelint config (standard + BEM-ish class regex). |
| frontend/src/utils/url.ts | Tweaks URL regexes for scheme/host-like detection. |
| frontend/src/styles/main.css | Updates layout/controls styling to use shared tokens and align with feed UI. |
| frontend/src/main.tsx | Uses document.querySelector('#app') for mount lookup. |
| frontend/src/hooks/useStrategies.ts | Migrates state shape to optional error and minor naming consistency. |
| frontend/src/hooks/useFeedConversion.ts | Adds readiness phases, retry loop/backoff, and manual retry entrypoint. |
| frontend/src/hooks/useAuth.ts | Uses globalThis and optional state fields; adjusts storage fallbacks. |
| frontend/src/hooks/useApiMetadata.ts | Uses optional metadata/error state fields and consistent setters. |
| frontend/src/hooks/useAccessToken.ts | Uses globalThis, optional token/error, and legacy-token migration behavior. |
| frontend/src/environment.d.ts | Adds Vite client type reference. |
| frontend/src/components/ResultDisplay.tsx | Updates result UI to show readiness state, actions, and preview expansion. |
| frontend/src/components/DominantField.tsx | Renames props interface for consistency. |
| frontend/src/components/Bookmarklet.tsx | Uses globalThis.location/globalThis.window checks. |
| frontend/src/components/AppPanels.tsx | Updates create panel copy/actions and adds OpenAPI link normalization. |
| frontend/src/components/App.tsx | Wires readiness retry through the app and adjusts messaging/heuristics. |
| frontend/src/api/contracts.ts | Adjusts optional/nullable fields in preview + retry types. |
| frontend/src/api/client.ts | Uses globalThis and makes bearer header helper accept optional token. |
| frontend/src/tests/useFeedConversion.test.ts | Updates tests for readiness phases/retry timing and optional state fields. |
| frontend/src/tests/useFeedConversion.contract.test.ts | Updates contract assertions for optional fields and readiness behavior. |
| frontend/src/tests/useAuth.test.ts | Updates for optional auth state fields and globalThis storage stubs. |
| frontend/src/tests/useAccessToken.test.ts | Updates for optional token/error and globalThis storage access. |
| frontend/src/tests/setup.ts | Improves test environment globals/storages and adds lint suppressions for null behavior. |
| frontend/src/tests/ResultDisplay.test.tsx | Updates result UI tests for readiness UX and preview expansion. |
| frontend/src/tests/App.test.tsx | Updates app tests for new copy, readiness wiring, and OpenAPI link normalization. |
| frontend/src/tests/App.contract.test.tsx | Updates contract test expectations for updated result copy and storage access. |
| frontend/playwright.config.ts | Switches webServer command to pnpm. |
| frontend/package.json | Pins pnpm, bumps deps, and adds ESLint/Stylelint + scripts. |
| frontend/eslint.config.js | Adds ESLint flat config (TS + unicorn + hooks). |
| frontend/.prettierignore | Replaces package-lock ignore with pnpm lockfile ignore. |
| frontend/.gitignore | Adds frontend-local ignores (node_modules/dist/.astro). |
| docs/README.md | Updates command docs for pnpm and clarifies make ready semantics. |
| docs/architecture.md | Clarifies routing/resolution boundaries for static vs token-backed feeds. |
| Dockerfile | Migrates frontend build stage from npm to pnpm/corepack. |
| bin/dev-with-frontend | Uses pnpm for frontend dev server. |
| bin/dev | Uses pnpm for frontend dev server and updates comments. |
| app/web/feeds/responder.rb | Refactors request resolution + result emission into helper methods. |
| .gitignore | Adds .pnpm-store and minor cleanup. |
| .github/workflows/ci.yml | Migrates frontend CI to pnpm; adds pnpm setup and Stylelint step. |
| .devcontainer/Dockerfile | Enables corepack/pnpm in devcontainer image. |
| .devcontainer/devcontainer.json | Adds ESLint extension + save-time fix actions and validation settings. |
| export interface CreatedFeedResult { | ||
| feed: FeedRecord; | ||
| preview: FeedPreviewState; | ||
| retry: FeedRetryState | null; | ||
| retry?: FeedRetryState; | ||
| } |
There was a problem hiding this comment.
CreatedFeedResult is now being treated as having readinessPhase (used in ResultDisplay/useFeedConversion/tests), but the interface here doesn't define that property, and there is no FeedReadinessPhase type exported from this module. This will cause TypeScript excess-property / missing-export errors. Add a FeedReadinessPhase union type and include readinessPhase: FeedReadinessPhase on CreatedFeedResult (and update any related types accordingly).
| import type { | ||
| CreatedFeedResult, | ||
| FeedPreviewItem, | ||
| FeedReadinessPhase, | ||
| FeedRecord, | ||
| } from '../api/contracts'; |
There was a problem hiding this comment.
This file imports FeedReadinessPhase from ../api/contracts, but frontend/src/api/contracts.ts does not currently export it. As-is, pnpm run typecheck should fail. Either export FeedReadinessPhase (and use it on CreatedFeedResult.readinessPhase) or remove this import and adjust the code to use an in-file type.
| const selectedStrategy = strategies.find((strategy) => strategy.id === feedFormData.strategy); | ||
| const urlInputRef = useRef<HTMLInputElement | null>(null); | ||
| const tokenInputRef = useRef<HTMLInputElement | null>(null); | ||
| const strategyOptionLabel = (strategy: Strategy) => { | ||
| if (strategy.id === 'faraday') return 'Default'; | ||
| if (strategy.id === 'browserless') return 'JavaScript pages (recommended)'; | ||
| return strategy.display_name; | ||
| }; | ||
| const urlInputReference = useRef<HTMLInputElement>(undefined as never); | ||
| const tokenInputReference = useRef<HTMLInputElement>(undefined as never); | ||
|
|
There was a problem hiding this comment.
useRef<HTMLInputElement>(undefined as never) forces the ref to be typed as always-present even though it's undefined until the element mounts, which defeats the purpose of the type and can mask real null/undefined issues. Prefer useRef<HTMLInputElement | null>(null) (or useRef<HTMLInputElement>() with an explicit union) and keep the existing runtime guard.
| document.addEventListener('DOMContentLoaded', function () { | ||
| var readerLink = document.querySelector('[data-feed-reader-link]'); | ||
| if (!readerLink) return; | ||
|
|
||
| readerLink.setAttribute('href', 'feed:' + window.location.href); | ||
| }); |
There was a problem hiding this comment.
This always overwrites the server-provided href value. If the XSL already emitted a canonical feed URL (e.g. from atom:link rel='self') this will replace it with feed: + window.location.href, which may differ (redirects, query params, alternate host) and can even double-prefix if the page was opened via a feed: URL. Consider only setting the href when it's missing or '#', and avoid double-prefixing when location.href already starts with feed:.
| font-size: var(--font-size-0); | ||
| line-height: var(--line-height-base); | ||
| -webkit-font-smoothing: antialiased; | ||
| -moz-osx-font-smoothing: grayscale; | ||
| text-rendering: optimizeLegibility; | ||
| text-rendering: optimizelegibility; | ||
| background: transparent; |
There was a problem hiding this comment.
text-rendering uses the value optimizelegibility here. While CSS keywords are generally case-insensitive in HTML, tooling (including some linters/validators) often expects the canonical optimizeLegibility spelling/casing for this property value. Consider switching back to optimizeLegibility to avoid false-positive lint errors.
| && corepack enable \ | ||
| && corepack prepare "pnpm@latest" --activate |
There was a problem hiding this comment.
The devcontainer forces pnpm@latest via corepack prepare, but frontend/package.json pins a specific pnpm version via packageManager. Activating latest can drift from the pinned version and cause lockfile / CI mismatches. Prefer preparing/activating the pinned version (or just corepack enable and let Corepack honor packageManager).
| && corepack enable \ | |
| && corepack prepare "pnpm@latest" --activate | |
| && corepack enable |
No description provided.