Broken link checker service#336
Conversation
|
👋 Thanks for opening this Pull Request! 🎉 We'll review it as soon as possible. Before we proceed, please double check:
Happy contributing! 🚀 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a React Query–backed scanning feature: new mutation hook, result normalization and error-translation utilities, expanded types, UI wiring for scan submission and results (loading/error/empty/single/multiple), styles, localization keys, and QueryClientProvider integration. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant LinksCard as ScanLinksCard
participant Page as ScannerPage
participant Mutation as useScanMutation
participant Service as LinkCheckerService
participant API as BrokenLinkCheckerAPI
participant ResultsCard as ScanResultsCard
User->>LinksCard: fill form & submit
LinksCard->>Page: onScan({scanType, url, multipleUrl})
Page->>Mutation: reset()
Page->>Mutation: mutate(variables)
Mutation->>Mutation: runScan (single or multiple)
Mutation->>Service: checkLink / checkLinks
Service->>API: HTTP request
API-->>Service: response (single or batch)
Service-->>Mutation: formatted result
Mutation-->>Page: update data / error / loading
Page->>ResultsCard: props { results, loading, error }
ResultsCard->>User: render loading / error / empty / single / multiple summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Scanner/components/ScanLinksCard.tsx`:
- Around line 74-84: getMultipleUrlsCall currently only parses multipleUrl and
ignores the repository `url` from RepositoryScanForm, causing valid
repository-only submissions to trigger ApiErrorTypes.ONE_URL_REQUIRED; update
getMultipleUrlsCall to include the `url` value when present (e.g., if `url` is
non-empty add it to the parsed `urls` array before validation), then run the
existing length checks (urls.length === 0 and > MAX_URLS_PER_REQUEST) and
finally return the call to linkCheckerService.checkLinks(urls); ensure you
reference the same symbols: multipleUrl, url, parseMultipleUrls,
getMultipleUrlsCall, MAX_URLS_PER_REQUEST, ApiErrorTypes, and
linkCheckerService.checkLinks.
In `@src/pages/Scanner/components/ScanResultsCard.tsx`:
- Around line 39-47: UrlResultRow currently only shows status via icons, so
update the component to expose the result status to assistive tech by adding an
accessible status label tied to the row (use the UrlResultRow props
url/isBroken). For example, add a screen-reader-only text node or an aria-label
on the container div (or set role="status") that includes the url and a short
status string like "working" or "broken" derived from isBroken, ensuring the
label is updated per row; keep the visual layout and existing style helpers
(scanPageStyle.statusIcon, scanPageStyle.resultDescription) unchanged.
In `@src/pages/Scanner/types/scan.ts`:
- Around line 38-41: The onError callback in the AsyncCallbacks interface
currently accepts raw string messages which forces UI components (e.g.,
ScanLinksCard, ScanResultsCard) to pass and render English text; change the
contract to use a stable error-code enum (e.g., ApiErrorTypes) or a typed
ErrorCode field instead of string, update AsyncCallbacks.onError signature to
onError: (error: ApiErrorTypes | null) => void (or a small ErrorPayload { code:
ApiErrorTypes; message?: string }), then update callers like ScanLinksCard to
pass the enum value and consumers like ScanResultsCard to map that enum to
localized display text via the i18n/localization layer rather than rendering the
raw message. Ensure ScanResult-related places that used onError (and the
AsyncCallbacks definition at lines ~54-57) follow the same change so all code
paths use error codes not raw strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b0c8057-89b3-421b-9e87-9e7ecd3dfc9a
📒 Files selected for processing (8)
public/locals/en.jsonsrc/pages/Scanner/Scanner.page.tsxsrc/pages/Scanner/components/ScanLinksCard.tsxsrc/pages/Scanner/components/ScanResultsCard.tsxsrc/pages/Scanner/components/styles.tssrc/pages/Scanner/scan.utils.tssrc/pages/Scanner/types/scan.tssrc/services/LinkChecker/types.ts
YusufErdemK
left a comment
There was a problem hiding this comment.
Clean integration overall — the callback-based state flow (onLoading / onError / onResults) keeps the components nicely decoupled.
One thing worth noting: onError takes a raw string | null, which means error messages from ScanLinksCard end up going straight into the UI. If you ever add i18n support for error states, you'll need to trace all the callers. Might be worth using ApiErrorTypes as the error type now and mapping to display text in ScanResultsCard — easier to change later.
Also noticed getMultipleUrlsCall doesn't seem to account for the url field from RepositoryScanForm, so a repo-only submission would hit ONE_URL_REQUIRED. Might be worth double-checking that path.
Tamir198
left a comment
There was a problem hiding this comment.
Left some comments as a initial review
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/Scanner/components/ScanResultsCard.tsx (1)
40-49:⚠️ Potential issue | 🟠 MajorAdd accessible status announcement to UrlResultRow.
The working/broken status is conveyed only through icons. Screen readers won't announce the outcome. This was flagged in a previous review.
♿ Suggested fix for accessibility
-const UrlResultRow = ({ url, isBroken }: UrlResultRowProps) => ( - <div style={scanPageStyle.urlRowContainer}> - {isBroken ? ( - <IconX style={scanPageStyle.statusIcon(isBroken)} /> - ) : ( - <IconCheck style={scanPageStyle.statusIcon(isBroken)} /> - )} +const UrlResultRow = ({ url, isBroken }: UrlResultRowProps) => ( + <div + style={scanPageStyle.urlRowContainer} + aria-label={`${url}: ${isBroken ? 'broken' : 'working'}`} + > + {isBroken ? ( + <IconX style={scanPageStyle.statusIcon(isBroken)} aria-hidden /> + ) : ( + <IconCheck style={scanPageStyle.statusIcon(isBroken)} aria-hidden /> + )} <span style={scanPageStyle.resultDescription}>{url}</span> </div> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Scanner/components/ScanResultsCard.tsx` around lines 40 - 49, UrlResultRow currently conveys status only via icons; add an accessible textual status that screen readers can announce by inserting a visually-hidden/aria-live text node alongside the icon that reflects isBroken (e.g., "Broken" vs "Working"), or set a role="status" and aria-atomic on the container so screen readers announce changes; update the UrlResultRow component (props: url, isBroken) to render that hidden status text next to the IconX/IconCheck and ensure scanPageStyle or a CSS sr-only class is used to visually hide the text but keep it accessible.
🧹 Nitpick comments (1)
src/pages/Scanner/components/ScanResultsCard.tsx (1)
65-72: Localize the error message.The
errorprop (typed asApiErrorTypes) is rendered directly as text. Map the enum to a translated string for proper i18n support.🌐 Suggested fix
if (error) { return ( <CardShell title={TITLE_KEY} contentStyle={scanPageStyle.resultsStack}> <IconX style={scanPageStyle.errorIcon} /> - <Typography style={scanPageStyle.errorText}>{error}</Typography> + <Typography style={scanPageStyle.errorText}> + {t(`scanner_page.scan_results_card.errors.${error}`)} + </Typography> </CardShell> ); }Then add corresponding keys to
public/locals/en.jsonunderscanner_page.scan_results_card.errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Scanner/components/ScanResultsCard.tsx` around lines 65 - 72, The ScanResultsCard currently renders the error prop (typed as ApiErrorTypes) directly; change it to map the ApiErrorTypes enum to a localized string before rendering—use the component's i18n hook (e.g., t) to look up keys under scanner_page.scan_results_card.errors (e.g., t(`scanner_page.scan_results_card.errors.${error}`) or a switch that maps each ApiErrorTypes case to a specific key) and render that translated string instead of raw error; also add the corresponding keys and human-friendly messages to public/locals/en.json under scanner_page.scan_results_card.errors so each enum value has a translation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Scanner/useScanMutation.ts`:
- Around line 13-14: The type guard hasError incorrectly narrows to `{ error:
string }`; update it to narrow to the correct type used by
UrlCheckResult/MultipleUrlsResponse by changing the predicate to `result is {
error: ApiErrorTypes }` (i.e., accept `error?: ApiErrorTypes | null` and return
`Boolean(result.error)`), and import or reference the ApiErrorTypes type if not
already imported so the throws at lines where `hasError` is used (around the
throw sites in useScanMutation, referenced by hasError and the throw calls on
lines ~23 and ~38) are type-safe.
---
Duplicate comments:
In `@src/pages/Scanner/components/ScanResultsCard.tsx`:
- Around line 40-49: UrlResultRow currently conveys status only via icons; add
an accessible textual status that screen readers can announce by inserting a
visually-hidden/aria-live text node alongside the icon that reflects isBroken
(e.g., "Broken" vs "Working"), or set a role="status" and aria-atomic on the
container so screen readers announce changes; update the UrlResultRow component
(props: url, isBroken) to render that hidden status text next to the
IconX/IconCheck and ensure scanPageStyle or a CSS sr-only class is used to
visually hide the text but keep it accessible.
---
Nitpick comments:
In `@src/pages/Scanner/components/ScanResultsCard.tsx`:
- Around line 65-72: The ScanResultsCard currently renders the error prop (typed
as ApiErrorTypes) directly; change it to map the ApiErrorTypes enum to a
localized string before rendering—use the component's i18n hook (e.g., t) to
look up keys under scanner_page.scan_results_card.errors (e.g.,
t(`scanner_page.scan_results_card.errors.${error}`) or a switch that maps each
ApiErrorTypes case to a specific key) and render that translated string instead
of raw error; also add the corresponding keys and human-friendly messages to
public/locals/en.json under scanner_page.scan_results_card.errors so each enum
value has a translation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bc3a3c9-5a01-4476-9a0e-199449d3cb8b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.jsonsrc/App.tsxsrc/pages/Scanner/Scanner.page.tsxsrc/pages/Scanner/components/ScanLinksCard.tsxsrc/pages/Scanner/components/ScanResultsCard.tsxsrc/pages/Scanner/components/styles.tssrc/pages/Scanner/types/scan.tssrc/pages/Scanner/useScanMutation.tssrc/pages/Scanner/utils/scan.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Scanner/components/styles.ts
- src/pages/Scanner/components/ScanLinksCard.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/Scanner/components/ScanResultsCard.tsx (1)
40-48:⚠️ Potential issue | 🟠 MajorExpose per-URL result status to assistive tech (still unresolved).
UrlResultRowcurrently communicates working/broken via icon only; add an accessible label per row and hide decorative icons from screen readers.♿ Minimal accessibility patch
const UrlResultRow = ({ url, isBroken }: UrlResultRowProps) => ( - <div style={scanPageStyle.urlRowContainer}> + <div + style={scanPageStyle.urlRowContainer} + aria-label={`${url}: ${isBroken ? 'broken' : 'working'}`} + > {isBroken ? ( - <IconX style={scanPageStyle.statusIcon(isBroken)} /> + <IconX style={scanPageStyle.statusIcon(isBroken)} aria-hidden /> ) : ( - <IconCheck style={scanPageStyle.statusIcon(isBroken)} /> + <IconCheck style={scanPageStyle.statusIcon(isBroken)} aria-hidden /> )} <span style={scanPageStyle.resultDescription}>{url}</span> </div> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Scanner/components/ScanResultsCard.tsx` around lines 40 - 48, UrlResultRow currently conveys status only via icons; update it to expose per-URL status to assistive tech by marking the decorative icons (IconX/IconCheck) as aria-hidden and adding an accessible label on the row container that includes the URL and its status (e.g., a descriptive aria-label or a visually-hidden span with text like "{url} — broken" or "{url} — working"). Locate UrlResultRow (props url, isBroken) and modify the JSX so IconX/IconCheck include aria-hidden={true} and the wrapper div (or scanPageStyle.resultDescription element) includes an aria-label or an extra screen-reader-only element that reports the status text derived from isBroken.
🧹 Nitpick comments (1)
src/pages/Scanner/components/ScanResultsCard.tsx (1)
9-9: Use a type-only import forScanResultsCardPropsto improve clarity and enable tree-shaking.
ScanResultsCardPropsis used only as a type annotation on line 51. Marking it as type-only clarifies intent and improves dead code elimination.♻️ Proposed fix
-import { ResolvedKind, ScanResultsCardProps } from '../types/scan'; +import { ResolvedKind, type ScanResultsCardProps } from '../types/scan';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Scanner/components/ScanResultsCard.tsx` at line 9, Change the mixed import so that ScanResultsCardProps is imported as a type-only import: keep ResolvedKind as the runtime/value import and import ScanResultsCardProps using an "import type" declaration (e.g., separate or combined import statements) in ScanResultsCard.tsx where ResolvedKind and ScanResultsCardProps are currently imported; this clarifies intent and enables tree-shaking for the type-only symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pages/Scanner/components/ScanResultsCard.tsx`:
- Around line 40-48: UrlResultRow currently conveys status only via icons;
update it to expose per-URL status to assistive tech by marking the decorative
icons (IconX/IconCheck) as aria-hidden and adding an accessible label on the row
container that includes the URL and its status (e.g., a descriptive aria-label
or a visually-hidden span with text like "{url} — broken" or "{url} — working").
Locate UrlResultRow (props url, isBroken) and modify the JSX so IconX/IconCheck
include aria-hidden={true} and the wrapper div (or
scanPageStyle.resultDescription element) includes an aria-label or an extra
screen-reader-only element that reports the status text derived from isBroken.
---
Nitpick comments:
In `@src/pages/Scanner/components/ScanResultsCard.tsx`:
- Line 9: Change the mixed import so that ScanResultsCardProps is imported as a
type-only import: keep ResolvedKind as the runtime/value import and import
ScanResultsCardProps using an "import type" declaration (e.g., separate or
combined import statements) in ScanResultsCard.tsx where ResolvedKind and
ScanResultsCardProps are currently imported; this clarifies intent and enables
tree-shaking for the type-only symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 480e8859-a21f-40cc-ba06-010a982b656a
📒 Files selected for processing (1)
src/pages/Scanner/components/ScanResultsCard.tsx
349b56c to
e15702e
Compare
…te, SingleResult, MultipleResults Deadlink-Hunter#320
Changes
Scanner.page.tsx- Added results, loading, and error state — wired toScanLinksCardandScanResultsCard.ScanLinksCard- Connected to LinkChecker API. Validates input, calls single/multi URL endpoints, and reports loading/error/results via callbacks.ScanResultsCard- Handles all render states: loading, error, empty, single result, and multiple results — with icons and summaries.scan.utils.ts(new) - Added helpers:resolveScanResults,sumResponseTimes, and type guards for single vs multi results.types/scan.ts- AddedScanSummary,ResolvedKind,AsyncCallbacks, and related types.styles.ts- Added styles for error state, URL rows, status icons, and results layout.Testing
Tested manually against local Broken Link Checker server:
Close #320
Summary by CodeRabbit
New Features
Bug Fixes