Draft
Conversation
- keep auth token session-persisted and migrate legacy localStorage token - add abortable preview hydration to cancel stale in-flight polling - tighten app security header expectations in specs
gildesmarais
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Hard-cutover of the feed creation + status contract to structured payloads (no message parsing / no strategy-driven UX), with accompanying SPA route-flow simplification, storage cleanup, OpenAPI/client regen, and Sentry breadcrumb/boot improvements.
Changes:
- Backend:
POST /api/v1/feedsnow accepts{url}only, returns structured conversion metadata, and standardizes structured failure envelopes; addsGET /api/v1/feeds/:token/status. - Frontend: route-driven flow (
/create,/token,/result/:feedToken) consumes structured error/status metadata; removes strategy selection; adds draft/result snapshot persistence + deep-link recovery. - Infra/obs/docs: OpenAPI updated + client regenerated; Sentry breadcrumbs + release tagging; new
make ci-ready; updated docs.
Reviewed changes
Copilot reviewed 67 out of 70 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/shared_examples/api_error_contract_examples.rb | Updates shared API error contract assertions to structured envelope fields. |
| spec/support/api_contract_helpers.rb | Extends contract helpers for structured errors and conversion payloads; removes strategy checks. |
| spec/html2rss/web/xml_builder_spec.rb | Adds coverage for updated empty-feed warning copy in XML output. |
| spec/html2rss/web/sentry_logs_spec.rb | Adds breadcrumb expectations for structured logs even when Sentry logs are disabled. |
| spec/html2rss/web/json_feed_builder_spec.rb | Adds coverage for updated empty-feed warning copy in JSON Feed output. |
| spec/html2rss/web/feeds/rss_renderer_spec.rb | Adapts renderer contract to include error_kind. |
| spec/html2rss/web/feeds/responder_spec.rb | Adds 422 handling for empty extraction + observability expectations. |
| spec/html2rss/web/feeds/json_renderer_spec.rb | Adapts renderer contract to include error_kind. |
| spec/html2rss/web/feeds/cache_spec.rb | Updates cache specs for new render result contract shape. |
| spec/html2rss/web/feed_notice_text_spec.rb | Adds tests for new empty-feed guidance copy. |
| spec/html2rss/web/error_responder_spec.rb | Updates API error response expectation to structured envelope. |
| spec/html2rss/web/boot/sentry_spec.rb | Adds tests for Sentry boot configuration + scope tags. |
| spec/html2rss/web/app_spec.rb | Updates root/dev behavior, CSP expectations, and method handling expectations. |
| spec/html2rss/web/app_integration_spec.rb | Updates integration expectations for route behavior + structured errors + status endpoint. |
| spec/html2rss/web/api/v1/contract_spec.rb | Adds tests for class-based structured error classification. |
| spec/html2rss/web/api/v1_spec.rb | Adds OpenAPI example for status endpoint; updates create flow + error envelope assertions. |
| public/shared-ui.css | Moves base typography declarations to shared form-controls + body group. |
| public/rss.xsl | Expands feed-notice detection for updated warning titles/messages. |
| public/openapi.yaml | Updates create + error envelope + adds status endpoint schema. |
| Makefile | Adds RUBOCOP_FLAGS and a ci-ready target for CI-parity checks. |
| frontend/src/utils/persistentStorage.ts | Introduces safe Storage resolver with memory fallback. |
| frontend/src/utils/feedSessionStorage.ts | Adds draft + result snapshot persistence and validation/normalization. |
| frontend/src/styles/main.css | Adds footer nav layout + workspace layout refinements. |
| frontend/src/routes/appRoute.ts | Adds route parsing/building + navigation hook for SPA route flow. |
| frontend/src/hooks/useStrategies.ts | Removes now-unused strategies hook. |
| frontend/src/hooks/useAuth.ts | Removes legacy auth hook (superseded by access-token flow). |
| frontend/src/hooks/useAccessToken.ts | Hard-cuts to sessionStorage-first token persistence with in-memory fallback. |
| frontend/src/components/ResultDisplay.tsx | Renders readiness + preview status/warnings from structured metadata; adjusts actions. |
| frontend/src/components/Bookmarklet.tsx | Updates bookmarklet to target /create?url=.... |
| frontend/src/components/AppPanels.tsx | Removes strategy selector; adds workflow state + structured error rendering + retry CTA. |
| frontend/src/components/App.tsx | Implements route-driven flow, deep-link recovery via snapshots, structured error handling. |
| frontend/src/api/generated/types.gen.ts | Regenerated types for url-only create + structured errors + status endpoint. |
| frontend/src/api/generated/sdk.gen.ts | Regenerated SDK incl. getFeedStatus. |
| frontend/src/api/generated/index.ts | Regenerated exports incl. getFeedStatus. |
| frontend/src/api/contracts.ts | Replaces strategy-centric contract types with structured error/status metadata types. |
| frontend/src/tests/useFeedConversion.test.ts | Updates hook tests to structured create/status/error contract and new behavior. |
| frontend/src/tests/useFeedConversion.contract.test.ts | Updates MSW contract tests for url-only create + structured errors + status endpoint. |
| frontend/src/tests/useAuth.test.ts | Removes tests for deleted legacy hook. |
| frontend/src/tests/useAccessToken.test.ts | Updates tests to sessionStorage-only canonical behavior + memory fallback paths. |
| frontend/src/tests/ResultDisplay.test.tsx | Updates tests for workflow state + degraded/unavailable warnings + retry messaging. |
| frontend/src/tests/mocks/server.ts | Extends MSW server helpers for conversion/status + structured errors. |
| frontend/src/tests/feedSessionStorage.test.ts | Adds tests for draft + snapshot persistence and guard/normalization. |
| frontend/src/tests/App.test.tsx | Updates unit tests for route flow, removal of strategies UI, retry UX, and snapshot restore. |
| frontend/src/tests/App.contract.test.tsx | Updates contract tests for structured create/status + structured auth failures. |
| frontend/e2e/smoke.spec.ts | Updates Playwright smoke coverage for route flow and snapshot deep-link restore. |
| docs/README.md | Documents new dev routing defaults and make ci-ready; adds Sentry runbook. |
| app/web/telemetry/sentry_logs.rb | Adds breadcrumb recording with sanitization and structured breadcrumb mapping. |
| app/web/telemetry/app_logger.rb | Ensures Sentry breadcrumbs are recorded for Sentry-bound structured logs. |
| app/web/routes/feed_pages.rb | Adds SPA path gating + optional SPA serving based on environment. |
| app/web/routes/api_v1/feed_routes.rb | Adds /api/v1/feeds/:token/status route. |
| app/web/rendering/xml_builder.rb | Updates empty-feed warning item title copy. |
| app/web/rendering/json_feed_builder.rb | Updates empty-feed warning item title copy. |
| app/web/rendering/feed_notice_text.rb | Updates empty-feed copy to remove strategy-driven guidance. |
| app/web/rendering/development_landing_page.rb | Adds static API-origin landing page for development. |
| app/web/feeds/service.rb | Adds error_kind classification to render results. |
| app/web/feeds/responder.rb | Maps empty extraction to 422 + emits dedicated observability event. |
| app/web/feeds/contracts.rb | Extends RenderResult with error_kind. |
| app/web/errors/health_check_failed_error.rb | Adds typed error for health check failures. |
| app/web/errors/error_responder.rb | Switches API error rendering to Contract.failure_payload. |
| app/web/errors/auto_source_disabled_error.rb | Adds typed policy error for auto-source disabled. |
| app/web/error_classification.rb | Adds shared network/server classification helper. |
| app/web/boot/sentry.rb | Adds scope tagging (release/environment) during Sentry initialization. |
| app/web/api/v1/health.rb | Raises typed HealthCheckFailedError instead of generic internal error. |
| app/web/api/v1/feed_status.rb | Implements structured status endpoint and initial conversion snapshot. |
| app/web/api/v1/feed_metadata.rb | Removes strategy from create params contract. |
| app/web/api/v1/create_feed.rb | Hard-cuts create contract to url-only + conversion metadata in success payload. |
| app/web/api/v1/contract.rb | Adds structured failure payload builder + enums + warning helper. |
| app.rb | Adds dev-only root landing page on API origin; tightens CSP in non-dev; makes SPA serving env-dependent. |
| AGENTS.md | Adds make ci-ready verification rule for contract/frontend changes. |
Comment on lines
+317
to
+321
| "/feeds/{token}/status": | ||
| get: | ||
| description: Get feed status | ||
| operationId: getFeedStatus | ||
| responses: |
There was a problem hiding this comment.
/feeds/{token}/status is missing a parameters block for the required {token} path param (unlike /feeds/{token} above). This breaks generated clients (it currently produces path?: never and an SDK URL with a literal {token}). Add the path parameter definition (and consider documenting the same 401/403/500 error envelopes the other token routes expose), then re-generate the frontend client artifacts.
Comment on lines
35
to
40
| def expect_feed_payload(json) | ||
| feed = json.fetch('data').fetch('feed') | ||
| expect_feed_identifier_payload(feed) | ||
| expect_feed_source_payload(feed) | ||
| expect(feed).not_to have_key('strategy') | ||
| feed | ||
| end |
There was a problem hiding this comment.
expect_feed_payload no longer asserts required feed fields like url/name, and expect_feed_source_payload is now unused. This reduces contract coverage for create/status responses; consider calling expect_feed_source_payload(feed) from expect_feed_payload (or removing/relocating the unused helper if intentionally dropped).
Comment on lines
+1
to
+6
| import type { CreatedFeedResult, FeedPreviewItem, FeedStatusWarning } from '../api/contracts'; | ||
| import { getPersistentStorage } from './persistentStorage'; | ||
|
|
||
| const FEED_DRAFT_KEY = 'html2rss_feed_draft_state'; | ||
| const FEED_RESULT_KEY_PREFIX = 'html2rss_feed_result_snapshot'; | ||
|
|
There was a problem hiding this comment.
This module is named feedSessionStorage, but it delegates to getPersistentStorage(), which currently prefers localStorage over sessionStorage. Either switch to session-first storage (to match the name and the PR’s “session-first” intent) or rename the module/utilities to reflect local/persistent storage so future callers don’t assume session-only behavior.
Comment on lines
+8
to
+17
| clear: () => store.clear(), | ||
| getItem: (key: string) => store.get(key), | ||
| key: (index: number) => [...store.keys()][index], | ||
| removeItem: (key: string) => { | ||
| store.delete(key); | ||
| }, | ||
| setItem: (key: string, value: string) => { | ||
| store.set(key, value); | ||
| }, | ||
| } as Storage; |
There was a problem hiding this comment.
memoryStorage doesn’t fully match the Storage contract: getItem/key return undefined when missing, but the DOM Storage API returns null. Returning null here avoids surprises in callers that distinguish null vs undefined and lets you drop some string | undefined handling elsewhere.
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.
TODO:
Summary
This branch now includes both the SPA route-flow work and a hard-cutover API contract migration so frontend state is driven by structured payloads instead of message parsing.
1) API contract cutover (backend)
POST /api/v1/feedsnow accepts only{ "url": ... }for create flow usage.Api::V1::Contract.failure_payloadwith:kind(auth|input|network|server)coderetryablenext_actionretry_actionnext_strategyonly when explicitly applicableGET /api/v1/feeds/:token/statusreadiness_phasepreview_statuswarnings[]AutoSourceDisabledErrorHealthCheckFailedError2) Frontend flow simplification (no strategy UI/control path)
/create,/token,/result/:feedToken) with state recovery support.useFeedConversionnow consumes structured API metadata for auth/retry/readiness decisions.useStrategieshook usage from app path3) Token/storage cleanup
useAccessToken.4) OpenAPI + generated client sync
public/openapi.yamlto match create/status + error envelope contract.frontend/src/api/generated/index.tsfrontend/src/api/generated/sdk.gen.tsfrontend/src/api/generated/types.gen.ts5) Reliability / observability / infra hardening in branch
make lint-rubynow uses configurable RuboCop flags with cache disabled by default in this repo path to avoid cache-only exit-2 behavior.Notes
/api/v1/strategiesendpoint compatibility is preserved externally, but app flow no longer depends on strategy controls.