Staging to Production#31
Merged
willy1989cv merged 14 commits intomainfrom Apr 21, 2026
Merged
Conversation
Render WMS previews for geoserver connected resources
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds SLD/OGC preview detection and styling, accessibility and semantic-markup updates, language-propagation for localized content, Danish/Swedish i18n keys, ISR revalidation reduction, and small configuration/docs updates (.gitignore, accessibility docs). Changes
Sequence Diagram(s)sequenceDiagram
participant ResourcePreview
participant OgcServiceMapPreview
participant ExternalSLD as "External SLD URL"
participant WFSService as "WFS Service"
participant Leaflet as "Leaflet Map"
ResourcePreview->>OgcServiceMapPreview: render with styleUrl / type
OgcServiceMapPreview->>ExternalSLD: fetch SLD XML (if styleUrl)
ExternalSLD-->>OgcServiceMapPreview: SLD XML or error
OgcServiceMapPreview->>OgcServiceMapPreview: sanitize & parse SLD → useSldStyler
OgcServiceMapPreview->>WFSService: GetFeature (normalized params)
WFSService-->>OgcServiceMapPreview: GeoJSON features
OgcServiceMapPreview->>Leaflet: apply styleFn per feature and render legend
Leaflet-->>ResourcePreview: display styled layer / legend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/components/package/dataset/DatasetInfo.tsx (1)
43-84:⚠️ Potential issue | 🟠 Major
<dl>wrapper creates invalid HTML structure with currentListItemimplementation
ListItemwraps its<dt>and<dd>elements in a<div>. When multipleListItemcomponents are placed as children of<dl>, this creates invalid definition-list markup—<dl>must contain only<dt>and<dd>as direct children, not<div>wrappers.Suggested fixes
Either revert the container to
<div>:- <dl className="grid grid-cols-1 divide-y"> + <div className="grid grid-cols-1 divide-y">Or refactor
src/components/ui/list-item.tsxto remove the wrapper<div>and render<dt>and<dd>as direct children for proper semantic HTML.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/package/dataset/DatasetInfo.tsx` around lines 43 - 84, DatasetInfo is rendering a <dl> whose children are ListItem components that currently wrap their <dt>/<dd> in a <div>, producing invalid HTML; fix by either changing the wrapper in DatasetInfo.tsx from <dl> to a neutral <div> or (preferable) refactor the ListItem implementation in src/components/ui/list-item.tsx so it does not output a wrapper element but instead returns the <dt> and <dd> as direct children (e.g., via React.Fragment or returning an array), keeping the title prop mapped to <dt> and the children to <dd> so <dl> contains only <dt>/<dd> direct children.src/components/package/search/FacetOptions.tsx (1)
39-70:⚠️ Potential issue | 🟠 MajorDuplicate keyboard targets and incorrect ARIA role for a selection control.
With this change, both the
<label>(lines 46–59,tabIndex={0}+onKeyDown) and the sibling<span>(lines 60–70, nowrole="button"+tabIndex={0}+onKeyDown) are independently focusable and both toggle the same value. Keyboard users now hit two tab stops that do the exact same thing per facet row, which is noisy and confusing with screen readers.A few related issues in this block:
- Wrong role. This is a selection control, not a button. Exposing it as
role="button"hides the checked state from assistive tech. The real<input type="checkbox">on lines 39–45 isclassName="hidden"(i.e.display:none), so it is removed from the accessibility tree entirely — AT never sees any checkbox at all.- Space scrolls the page.
onKeyDownon the<span>toggles on Space/Enter but never callse.preventDefault(), so pressing Space (the idiomatic activation key forrole="button"/checkbox) scrolls the page.- Redundant focus surface. Only one element should be focusable per row.
Recommended shape: drop the custom focusable wrappers and make the native checkbox the (visually-hidden but accessible) focus target, using
peer-*styling for the faux box.♻️ Suggested direction
<div className="flex"> <input type="checkbox" id={`${name}-${value}`} checked={isActive} onChange={toggleSelection} - className="hidden" + className="peer sr-only" /> <label htmlFor={`${name}-${value}`} - tabIndex={0} - onKeyDown={(e) => - (e.key === " " || e.key === "Enter") && toggleSelection() - } className={cn( - "mt-1 h-4 max-w-4 min-w-4 flex items-center justify-center rounded border-2 cursor-pointer transition-colors", + "mt-1 h-4 max-w-4 min-w-4 flex items-center justify-center rounded border-2 cursor-pointer transition-colors peer-focus-visible:ring-2 peer-focus-visible:ring-theme-green", isActive ? "bg-theme-green border-theme-green" : "bg-white border-gray-200" )} > {isActive && <Check className="text-white" />} <span className="sr-only">{label}</span> </label> - <span - onClick={toggleSelection} - onKeyDown={(e) => - (e.key === " " || e.key === "Enter") && toggleSelection() - } - role="button" - tabIndex={0} - className="ml-3 text-[`#364153`] cursor-pointer" - > + <label htmlFor={`${name}-${value}`} className="ml-3 text-[`#364153`] cursor-pointer"> {label} {count !== undefined && `(${count})`} - </span> + </label> </div>If the custom
role="button"pattern must be kept, at minimum:
- Remove
tabIndex/keydown from the<label>so there is exactly one focus stop.- Use
role="checkbox"witharia-checked={isActive}instead ofrole="button".- Call
e.preventDefault()inside the handler when the key is Space.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/package/search/FacetOptions.tsx` around lines 39 - 70, The row currently exposes two independent keyboard targets (<label> with tabIndex/onKeyDown and the sibling <span> with role="button" tabIndex/onKeyDown) while the native <input type="checkbox"> is hidden via className="hidden", making the real checkbox inaccessible; fix by removing redundant focus surfaces and restoring a single accessible checkbox target: either make the native <input> accessible (remove display:none hiding, use visually-hidden but focusable styling / peer-* for the faux box) and remove tabIndex/onKeyDown from the <label> and the <span>, or if keeping the custom control pattern, keep only one focusable element (remove tabIndex from <label>), change role="button" to role="checkbox" on the interactive element, add aria-checked={isActive}, call e.preventDefault() when handling Space in toggleSelection, and ensure toggleSelection updates the real checkbox state (isActive).src/components/package/search/Facets.tsx (1)
17-64:⚠️ Potential issue | 🟠 MajorSelected-but-unreturned facets can become unclearable on the filter panel.
ensureSelectedItemscorrectly keeps a selected value visible in theFacetCardeven when the server stops returning it as a facet item. However, persrc/components/package/search/SearchLayout.tsx(lines 29-34, 62-66, 81-94), the entire filter panel (and the mobile filter button) is gated byshowFilters = hasResults || hasVisibleFacets, wherehasVisibleFacetsonly inspects server-returned facet items and does not consider selected values. So when a user lands on a query that yields zero results and zero server facets but with selected groups/formats/tags in the URL,Facetswould render selected chips — butSearchLayoutwon't renderFacetsat all, leaving the user with no UI to clear those filters.Consider extending
showFilters/hasVisibleFacetsinSearchLayoutto also account foroptions?.groups/resFormat/tagslengths, so the gating matches the new visibility logic here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/package/search/Facets.tsx` around lines 17 - 64, SearchLayout's showFilters gating only checks server-returned facets and omits selected URL filters, causing ensureSelectedItems (in Facets) to show selected chips that can't be cleared; update the hasVisibleFacets / showFilters logic in SearchLayout to also consider the selected options arrays (options?.groups, options?.resFormat, options?.tags) by treating any non-empty selected array as a visible facet, so the filter panel and mobile filter button render whenever selected filters exist and match Facets' visibility behavior.src/app/[locale]/themes/page.tsx (1)
39-41:⚠️ Potential issue | 🟡 MinorPotential
TypeErrorwhenqParamis an empty array.
Array.isArray(qParam) ? qParam[0].toLowerCase() : ...will throwCannot read properties of undefined (reading 'toLowerCase')ifqParamis an empty array, or if the first entry is undefined. Use optional chaining for symmetry with the non-array branch.Proposed fix
- const query = Array.isArray(qParam) - ? qParam[0].toLowerCase() - : qParam?.toLowerCase() || ""; + const query = (Array.isArray(qParam) ? qParam[0] : qParam)?.toLowerCase() || "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/themes/page.tsx around lines 39 - 41, The construction of query can throw if qParam is an empty array because qParam[0] may be undefined; update the Array.isArray(qParam) branch to safely access the first element with optional chaining and a fallback before calling toLowerCase (e.g., use (qParam[0] ?? "").toLowerCase() or qParam[0]?.toLowerCase()), so both branches mirror the null/undefined-safe behavior and always produce a string; change the expression that sets query (referencing qParam and query) accordingly.src/hooks/sld.ts (1)
9-27:⚠️ Potential issue | 🟡 MinorPending
setTimeoutis never cleared on unmount or dep change.
tryCreatereschedules itself viasetTimeout(tryCreate, 50)indefinitely untilL.SLDStylerappears. The cleanup only flipsmounted = false, which prevents a stalesetStylerbut does not cancel the in-flight timer. IfL.SLDStylernever loads (e.g., script failed), the polling continues for the lifetime of the page; on rapidsldXmlchanges, multiple poll chains stack up.Proposed fix
useEffect(() => { let mounted = true; + let timer: ReturnType<typeof setTimeout> | null = null; const normalizedSldXml = sanitizeSldXml(sldXml); const tryCreate = () => { if ("SLDStyler" in L) { const s = new L.SLDStyler(normalizedSldXml); if (mounted) setStyler(s); } else { - setTimeout(tryCreate, 50); + timer = setTimeout(tryCreate, 50); } }; tryCreate(); return () => { mounted = false; + if (timer) clearTimeout(timer); }; }, [sldXml]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/sld.ts` around lines 9 - 27, The effect's polling via tryCreate uses setTimeout but never clears the timer, causing timers to leak across unmounts or sldXml changes; update the useEffect (symbols: useEffect, tryCreate, setTimeout, mounted, setStyler, sanitizeSldXml, sldXml) to track the timeout id (e.g., let timerId) and clearTimeout(timerId) before scheduling a new setTimeout and inside the cleanup return (set mounted = false and clearTimeout(timerId)) so any in-flight timers are cancelled when the component unmounts or dependencies change.src/components/layout/PageHero.tsx (1)
119-149:⚠️ Potential issue | 🟡 Minor
lang={descriptionLang}scope includes Read more/less button.The wrapper
<div lang={descriptionLang}>also contains theRead more/Read lessbuttons whose label comes fromt("Common.readMore")/t("Common.readLess")(UI locale), not the description's language. WhendescriptionLangdiffers from the UI locale, screen readers may announce the button label with the wrong language. Consider applyinglangonly to the inner content wrapper around<MarkdownRender />.Proposed fix
- <div className="mt-4" lang={descriptionLang}> + <div className="mt-4"> <div ref={ref} + lang={descriptionLang} className={[ "md:text-xl text-gray-700", expanded ? "" : "line-clamp-5", ].join(" ")} > <MarkdownRender content={description} /> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/PageHero.tsx` around lines 119 - 149, The lang={descriptionLang} is applied to the outer wrapper that also contains the Read more/Read less <Button>s, causing screen readers to announce the buttons in the description language; move the lang only to the element that wraps the MarkdownRender content (e.g., apply descriptionLang to the inner div with ref or add a dedicated wrapper around <MarkdownRender />) and remove lang from the outer container so the <Button>s (and their t(...) labels) remain announced in the UI locale; keep the existing state/props (expanded, isClamped, setExpanded, MarkdownRender, Button, descriptionLang, t) unchanged otherwise.
🧹 Nitpick comments (7)
src/components/ui/page-search-input.tsx (1)
49-62: Staticid="search-input"risks duplicate IDs.If
PageSearchInputis ever rendered more than once on the same page (or alongside another element using the same id), the hardcodedid/htmlFor="search-input"will produce duplicate IDs and break the label association for assistive tech — defeating the purpose of thesr-onlylabel. Consider deriving a unique id (e.g., viauseId()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/page-search-input.tsx` around lines 49 - 62, The hardcoded id/htmlFor "search-input" in PageSearchInput (label and Input) can cause duplicate IDs; change the component to generate a unique id (e.g., with React's useId()) and use that id for both the label's htmlFor and the Input's id so the sr-only label stays correctly associated; update references in the component (PageSearchInput, handleSubmit form, and the Input props) to use the generated uniqueId.src/hooks/sld.ts (1)
33-54: Consider boundingsanitizeSldXmlpolling/work or memoizing.Minor:
sanitizeSldXmlruns DOM parsing/serialization on every effect run for everysldXmlchange. IfsldXmlis large or the component re-renders frequently, consider memoizing viauseMemokeyed onsldXml. Not a correctness issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/sld.ts` around lines 33 - 54, The sanitizeSldXml function performs DOM parsing/serialization on every change, which can be expensive; wrap calls to sanitizeSldXml in a memoized hook where it’s used (e.g., useMemo(() => sanitizeSldXml(sldXml), [sldXml])) so the heavy work only runs when sldXml changes, or alternatively implement a memoized wrapper around sanitizeSldXml keyed by sldXml; locate usages of sanitizeSldXml in components/effects and replace direct calls with the memoized result to reduce repeated parsing/serialization.src/components/package/search/SearchResultHeader.tsx (1)
14-22: Consider scoping the live region to just the announcement text.Wrapping a heading inside a
role="status"live region can cause some screen readers to treat the inner content as plain text, potentially weakening heading semantics. Consider keeping the<h2>outside the live region and using a sibling element withrole="status"for the announcement, or usearia-liveon a dedicated visually-hidden element.Also confirm a top-level
<h1>exists on the search page now that this was demoted fromh1toh2, otherwise the page will lack a primary heading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/package/search/SearchResultHeader.tsx` around lines 14 - 22, The live region currently wraps the heading in SearchResultHeader (the <h2> that renders t("datasetsFound", { count })), which can weaken heading semantics; move the role="status" / aria-live="polite" / aria-atomic="true" off the <h2> and into a sibling announcement element (preferably visually-hidden) that receives the dynamic text, or place a separate visually-hidden element with aria-live that announces the count while keeping the <h2> purely semantic. Also verify that the search page includes a top-level <h1> (if the heading was demoted from h1 to h2) so the document maintains a primary heading.public/leaflet/leaflet-sld.js (2)
313-317: Compile thelikeregex once per filter, not per feature.
getFilterPatternRegex(comp)is invoked insideisFilterMatch, which runs for every feature being styled. For large GeoJSON layers this rebuilds the sameRegExpthousands of times. Consider caching the compiled regex on the comparison object duringparseFilter(or memoizing oncomp).Suggested approach
- } else if (comp.operator == 'like') { + } else if (comp.operator == 'like') { if (typeof value === 'undefined' || value === null) { return false; } - return getFilterPatternRegex(comp).test(String(value)); + if (!comp._regex) { + comp._regex = getFilterPatternRegex(comp); + } + return comp._regex.test(String(value));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/leaflet/leaflet-sld.js` around lines 313 - 317, The 'like' operator currently recompiles its RegExp for every feature because getFilterPatternRegex(comp) is called inside isFilterMatch; change this by compiling the RegExp once during filter parsing and storing it on the comparison object (e.g., set comp._likeRegex or similar in parseFilter) or memoize getFilterPatternRegex to return a cached RegExp for the same comp, then update isFilterMatch to use the stored regex instead of re-invoking the compiler.
218-228:matchCaseattribute onPropertyIsLikeis not honored, andwildCard/singleChar/escapeCharare attached to all comparison operators.The OGC Filter spec allows
PropertyIsLike(and other operators) to carry amatchCaseattribute (defaulttrue). The parser ignores it entirely—getAttribute('matchCase')is never called—so SLDs that explicitly request case-insensitive matching will silently behave as case-sensitive. ThegetFilterPatternRegexfunction should readmatchCaseand use the'i'flag when constructing the regex.Also,
wildCard,singleChar, andescapeCharare only valid forPropertyIsLike, but they're being attached to every comparison operator (lines 221–227). Only the'like'operator uses them (line 317); the others ignore them entirely. Gate this onkey === 'ogc:PropertyIsLike'for clarity and correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/leaflet/leaflet-sld.js` around lines 218 - 228, The parser currently attaches wildCard/singleChar/escapeChar to every comparison and ignores PropertyIsLike's matchCase; update the comparisionElements.forEach block that builds filterJson.comparisions so it only reads and sets wildCard, singleChar and escapeChar when the current comparison key is 'ogc:PropertyIsLike' (or operator indicates LIKE), and add reading of the matchCase attribute (e.g., getAttribute('matchCase')) to include a boolean matchCase property on the comparision object; then update getFilterPatternRegex to use that matchCase flag to set the 'i' regex flag when matchCase is false so PropertyIsLike respects case-insensitivity.src/components/package/resource/ResourcePreview.tsx (1)
40-46: PrefertoLowerCase()for format comparisons.Line 42 uses
toLocaleLowerCase()while line 40 usestoLowerCase(). Since the comparison is against ASCII"sld", locale-aware casing can produce unexpected results (e.g., Turkish locale convertingItoı) and is inconsistent with the rest of the function.♻️ Proposed change
const sldUrl = dataset.resources?.find( - (r) => r.format?.toLocaleLowerCase() === "sld", + (r) => r.format?.toLowerCase() === "sld", )?.url;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/package/resource/ResourcePreview.tsx` around lines 40 - 46, The code mixes toLowerCase and toLocaleLowerCase when normalizing resource formats; replace the locale-aware call inside the dataset.resources?.find predicate (the toLocaleLowerCase on r.format) with toLowerCase so it matches the normalization used for resource.format and avoids locale-specific edge cases—update the sldUrl lookup (the find callback) to use r.format?.toLowerCase() and keep the rest of the logic (hasSld, isMapPreview, getOgcPreviewConfig) unchanged.src/components/package/resource/OgcServiceMapPreview.tsx (1)
736-740: Dead fallback onsldError.The surrounding guard already requires
sldErrorto be truthy, sosldError ?? t("Map.sld.errors.failedToLoadStyle")can never use the fallback. Either drop the??or render the fallback from a different condition (e.g., whensldErroris an empty string or unknown).♻️ Proposed change
- {type === "wfs" && sldError && ( - <div className="mb-2 text-sm text-amber-700"> - {sldError ?? t("Map.sld.errors.failedToLoadStyle")} - </div> - )} + {type === "wfs" && sldError && ( + <div className="mb-2 text-sm text-amber-700">{sldError}</div> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/package/resource/OgcServiceMapPreview.tsx` around lines 736 - 740, The JSX currently guards with `type === "wfs" && sldError` which prevents the nullish fallback from ever being used; change the condition to `type === "wfs" && sldError != null` (or `sldError !== undefined`) so an empty string still renders the container, then keep the inner expression `{sldError ?? t("Map.sld.errors.failedToLoadStyle")}` to allow the fallback when `sldError` is null/undefined; locate this in the OgcServiceMapPreview component where `type` and `sldError` are referenced.
🤖 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/app/`[locale]/[org]/[dataset]/[resource]/page.tsx:
- Line 174: The DownloadIcon in page.tsx is rendered with size={5} (making it
effectively invisible); update the prop to a sensible icon size (e.g.,
size={20}) for proper visibility and do the same for the EyeIcon in
DatasetResource.tsx which has the same issue; search for other lucide-react
icons in the repo that use size={5} and replace with a readable size (20 or the
24 default) to keep UI consistent.
In `@src/app/globals.css`:
- Around line 4-6: The active custom variant declaration "@custom-variant dark
(&:is(.dark *))" remains but no dark token overrides exist, which can silently
make future dark:* utilities appear to work while producing no visual change;
either remove the "@custom-variant dark (&:is(.dark *))" line (and the now-empty
".dark { … }" block) or keep the declaration but add a concise inline comment
above it stating dark-mode tokens are intentionally unset pending the follow-up
tracked in docs/accessibility-fixes.md so future authors aren’t misled.
In `@src/components/layout/Footer.tsx`:
- Line 17: Replace the invalid Tailwind class "grid-2" used in the Container's
className with the correct "grid-cols-2" so the base breakpoint renders two
columns; update the className on the Container element (the JSX node using
Container and className="grid grid-2 gap-10 lg:grid-cols-3 lg:gap-20") to use
"grid-cols-2" instead.
In `@src/components/package/dataset/DatasetResource.tsx`:
- Line 116: The EyeIcon used in DatasetResource.tsx is set with size={5}, which
renders an unreadably small 5×5px icon; update the EyeIcon usage (the Preview
button's icon) to a sensible pixel size (for example size={20}) or remove the
size prop to use the component's default so the preview icon is visible (locate
the EyeIcon element in the DatasetResource component and change its size prop
accordingly).
In `@src/components/package/resource/OgcServiceMapPreview.tsx`:
- Around line 410-426: The hook useSldStyler is being called with an empty
string and may instantiate new L.SLDStyler("") which can throw; update
useSldStyler to defensively handle empty/null input and wrap the L.SLDStyler
instantiation and any parsing in a try-catch, returning null (or a safe styler
with getStyleFunction = () => undefined) on error so that variables here
(sldXml, styler, styler.getStyleFunction) never cause a runtime crash; keep the
current call site (styler = useSldStyler(sldXml ?? "")) but change useSldStyler
implementation to check for falsy sldXml and catch/handle exceptions from new
L.SLDStyler(...) and parsing, logging or setting an error as appropriate.
In `@src/components/package/search/SearchLayout.tsx`:
- Around line 90-93: The Button in SearchLayout currently includes a redundant
visually-hidden span (<span className="sr-only">{t("Search.filters")}</span>)
while also rendering visible text {t("Search.filters")}, causing screen readers
to announce "Filters" twice; remove the sr-only span (or alternatively remove
the visible text and add aria-label to the Button) so the accessible name is
provided only once, keeping the Funnel icon aria-hidden and leaving the visible
{t("Search.filters")} text inside the Button component.
In `@src/components/ui/list-item.tsx`:
- Around line 11-18: ListItem currently renders dt/dd but is used inside plain
divs in ResourceDetails, breaking semantics; wrap the group of ListItem usages
in ResourceDetails with a <dl> (as DatasetInfo already does) so each ListItem's
dt/dd become valid children, or alternatively change ListItem to render its own
<dl> wrapper when used as a standalone block; update ResourceDetails to replace
the surrounding div(s) around the ListItem instances with a <dl> (or adjust
ListItem to emit <dl><dt>…</dt><dd>…</dd></dl>) and ensure no nested divs remain
between the dl and the dt/dd.
In `@src/components/ui/markdown.tsx`:
- Around line 14-15: The outer <div> in the MarkdownRender component is always
rendered (returning <div lang={lang}>) which breaks inline usage when textOnly
is true; update MarkdownRender to avoid a block <div> in textOnly mode by using
a <span> wrapper with style={{display: 'contents'}} (or no wrapper if lang is
falsy) when textOnly is true so the flattened text can be safely placed inside
inline elements; ensure any lang attribute is only applied when valid and that
the render paths for renderMarkdown/renderText (or whatever internal helpers
inside MarkdownRender) use the span/display:contents solution to prevent invalid
HTML nesting and hydration warnings.
In `@src/lib/ogc.ts`:
- Around line 70-91: The issue is asymmetric handling of case-variant query
keys: before any params.set(...) calls (including params.set("service", ...),
params.set("typename", ...), and params.set("layers", ...)), first remove all
case variants of the target keys using deleteParams so no existing
differently-cased keys (e.g., SERVICE, LAYERS, TypeName) remain; specifically,
call deleteParams(params, "service") before setting service, call
deleteParams(params, "layers", "typenames", "typeNames", "bbox", "typename",
"TypeName") (or equivalently ensure deleteParams is invoked for both
plural/singular and all casings) in the WFS branch before set("typename") and in
the WMS branch before set("layers"); continue to use getParam/version logic
unchanged but ensure deletion covers all casing variants so parseOgcResourceUrl
and downstream lowercasing cannot be trumped by leftover differently-cased keys.
In `@src/lib/sld.ts`:
- Around line 15-70: normalizeLikeLiteral currently breaks on the first
wildcard/singleChar so literals beginning with a wildcard (e.g. "*foo" or
"*foo*") produce an empty normalized value; update normalizeLikeLiteral to strip
leading and trailing wildcard markers (wildCard and singleChar) while honoring
the escapeChar rather than bailing on the first match, i.e. walk the literal
tracking escapes, remove any unescaped leading/trailing wildCard/singleChar
characters and return the trimmed inner text; keep buildComparisonLabel as-is
(it will then receive a proper normalizedValue) and ensure you still fall back
to literal only if normalization yields an empty string.
---
Outside diff comments:
In `@src/app/`[locale]/themes/page.tsx:
- Around line 39-41: The construction of query can throw if qParam is an empty
array because qParam[0] may be undefined; update the Array.isArray(qParam)
branch to safely access the first element with optional chaining and a fallback
before calling toLowerCase (e.g., use (qParam[0] ?? "").toLowerCase() or
qParam[0]?.toLowerCase()), so both branches mirror the null/undefined-safe
behavior and always produce a string; change the expression that sets query
(referencing qParam and query) accordingly.
In `@src/components/layout/PageHero.tsx`:
- Around line 119-149: The lang={descriptionLang} is applied to the outer
wrapper that also contains the Read more/Read less <Button>s, causing screen
readers to announce the buttons in the description language; move the lang only
to the element that wraps the MarkdownRender content (e.g., apply
descriptionLang to the inner div with ref or add a dedicated wrapper around
<MarkdownRender />) and remove lang from the outer container so the <Button>s
(and their t(...) labels) remain announced in the UI locale; keep the existing
state/props (expanded, isClamped, setExpanded, MarkdownRender, Button,
descriptionLang, t) unchanged otherwise.
In `@src/components/package/dataset/DatasetInfo.tsx`:
- Around line 43-84: DatasetInfo is rendering a <dl> whose children are ListItem
components that currently wrap their <dt>/<dd> in a <div>, producing invalid
HTML; fix by either changing the wrapper in DatasetInfo.tsx from <dl> to a
neutral <div> or (preferable) refactor the ListItem implementation in
src/components/ui/list-item.tsx so it does not output a wrapper element but
instead returns the <dt> and <dd> as direct children (e.g., via React.Fragment
or returning an array), keeping the title prop mapped to <dt> and the children
to <dd> so <dl> contains only <dt>/<dd> direct children.
In `@src/components/package/search/FacetOptions.tsx`:
- Around line 39-70: The row currently exposes two independent keyboard targets
(<label> with tabIndex/onKeyDown and the sibling <span> with role="button"
tabIndex/onKeyDown) while the native <input type="checkbox"> is hidden via
className="hidden", making the real checkbox inaccessible; fix by removing
redundant focus surfaces and restoring a single accessible checkbox target:
either make the native <input> accessible (remove display:none hiding, use
visually-hidden but focusable styling / peer-* for the faux box) and remove
tabIndex/onKeyDown from the <label> and the <span>, or if keeping the custom
control pattern, keep only one focusable element (remove tabIndex from <label>),
change role="button" to role="checkbox" on the interactive element, add
aria-checked={isActive}, call e.preventDefault() when handling Space in
toggleSelection, and ensure toggleSelection updates the real checkbox state
(isActive).
In `@src/components/package/search/Facets.tsx`:
- Around line 17-64: SearchLayout's showFilters gating only checks
server-returned facets and omits selected URL filters, causing
ensureSelectedItems (in Facets) to show selected chips that can't be cleared;
update the hasVisibleFacets / showFilters logic in SearchLayout to also consider
the selected options arrays (options?.groups, options?.resFormat, options?.tags)
by treating any non-empty selected array as a visible facet, so the filter panel
and mobile filter button render whenever selected filters exist and match
Facets' visibility behavior.
In `@src/hooks/sld.ts`:
- Around line 9-27: The effect's polling via tryCreate uses setTimeout but never
clears the timer, causing timers to leak across unmounts or sldXml changes;
update the useEffect (symbols: useEffect, tryCreate, setTimeout, mounted,
setStyler, sanitizeSldXml, sldXml) to track the timeout id (e.g., let timerId)
and clearTimeout(timerId) before scheduling a new setTimeout and inside the
cleanup return (set mounted = false and clearTimeout(timerId)) so any in-flight
timers are cancelled when the component unmounts or dependencies change.
---
Nitpick comments:
In `@public/leaflet/leaflet-sld.js`:
- Around line 313-317: The 'like' operator currently recompiles its RegExp for
every feature because getFilterPatternRegex(comp) is called inside
isFilterMatch; change this by compiling the RegExp once during filter parsing
and storing it on the comparison object (e.g., set comp._likeRegex or similar in
parseFilter) or memoize getFilterPatternRegex to return a cached RegExp for the
same comp, then update isFilterMatch to use the stored regex instead of
re-invoking the compiler.
- Around line 218-228: The parser currently attaches
wildCard/singleChar/escapeChar to every comparison and ignores PropertyIsLike's
matchCase; update the comparisionElements.forEach block that builds
filterJson.comparisions so it only reads and sets wildCard, singleChar and
escapeChar when the current comparison key is 'ogc:PropertyIsLike' (or operator
indicates LIKE), and add reading of the matchCase attribute (e.g.,
getAttribute('matchCase')) to include a boolean matchCase property on the
comparision object; then update getFilterPatternRegex to use that matchCase flag
to set the 'i' regex flag when matchCase is false so PropertyIsLike respects
case-insensitivity.
In `@src/components/package/resource/OgcServiceMapPreview.tsx`:
- Around line 736-740: The JSX currently guards with `type === "wfs" &&
sldError` which prevents the nullish fallback from ever being used; change the
condition to `type === "wfs" && sldError != null` (or `sldError !== undefined`)
so an empty string still renders the container, then keep the inner expression
`{sldError ?? t("Map.sld.errors.failedToLoadStyle")}` to allow the fallback when
`sldError` is null/undefined; locate this in the OgcServiceMapPreview component
where `type` and `sldError` are referenced.
In `@src/components/package/resource/ResourcePreview.tsx`:
- Around line 40-46: The code mixes toLowerCase and toLocaleLowerCase when
normalizing resource formats; replace the locale-aware call inside the
dataset.resources?.find predicate (the toLocaleLowerCase on r.format) with
toLowerCase so it matches the normalization used for resource.format and avoids
locale-specific edge cases—update the sldUrl lookup (the find callback) to use
r.format?.toLowerCase() and keep the rest of the logic (hasSld, isMapPreview,
getOgcPreviewConfig) unchanged.
In `@src/components/package/search/SearchResultHeader.tsx`:
- Around line 14-22: The live region currently wraps the heading in
SearchResultHeader (the <h2> that renders t("datasetsFound", { count })), which
can weaken heading semantics; move the role="status" / aria-live="polite" /
aria-atomic="true" off the <h2> and into a sibling announcement element
(preferably visually-hidden) that receives the dynamic text, or place a separate
visually-hidden element with aria-live that announces the count while keeping
the <h2> purely semantic. Also verify that the search page includes a top-level
<h1> (if the heading was demoted from h1 to h2) so the document maintains a
primary heading.
In `@src/components/ui/page-search-input.tsx`:
- Around line 49-62: The hardcoded id/htmlFor "search-input" in PageSearchInput
(label and Input) can cause duplicate IDs; change the component to generate a
unique id (e.g., with React's useId()) and use that id for both the label's
htmlFor and the Input's id so the sr-only label stays correctly associated;
update references in the component (PageSearchInput, handleSubmit form, and the
Input props) to use the generated uniqueId.
In `@src/hooks/sld.ts`:
- Around line 33-54: The sanitizeSldXml function performs DOM
parsing/serialization on every change, which can be expensive; wrap calls to
sanitizeSldXml in a memoized hook where it’s used (e.g., useMemo(() =>
sanitizeSldXml(sldXml), [sldXml])) so the heavy work only runs when sldXml
changes, or alternatively implement a memoized wrapper around sanitizeSldXml
keyed by sldXml; locate usages of sanitizeSldXml in components/effects and
replace direct calls with the memoized result to reduce repeated
parsing/serialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09a67aad-d93d-46a9-a9d3-97f2a9942ed9
📒 Files selected for processing (45)
.gitignoredocs/accessibility-fixes.mdmessages/da.jsonmessages/en.jsonmessages/sv.jsonpublic/leaflet/leaflet-sld.jssrc/app/[locale]/[org]/[dataset]/[resource]/page.tsxsrc/app/[locale]/[org]/[dataset]/page.tsxsrc/app/[locale]/page.tsxsrc/app/[locale]/themes/page.tsxsrc/app/globals.csssrc/components/groups/GroupCard.tsxsrc/components/layout/Footer.tsxsrc/components/layout/Header.tsxsrc/components/layout/LanguageSwitcher.tsxsrc/components/layout/Page.tsxsrc/components/layout/PageHero.tsxsrc/components/map/SldLegend.tsxsrc/components/package/dataset/DatasetInfo.tsxsrc/components/package/dataset/DatasetResource.tsxsrc/components/package/dataset/DatasetSimpleCard.tsxsrc/components/package/resource/OgcServiceMapPreview.tsxsrc/components/package/resource/ResourcePreview.tsxsrc/components/package/search/ActiveFilters.tsxsrc/components/package/search/FacetCard.tsxsrc/components/package/search/FacetOptions.tsxsrc/components/package/search/Facets.tsxsrc/components/package/search/FilterBadge.tsxsrc/components/package/search/NoDataFound.tsxsrc/components/package/search/SearchForm.tsxsrc/components/package/search/SearchLayout.tsxsrc/components/package/search/SearchResultHeader.tsxsrc/components/package/search/SearchResultItem.tsxsrc/components/ui/heading.tsxsrc/components/ui/list-item.tsxsrc/components/ui/markdown.tsxsrc/components/ui/page-search-input.tsxsrc/hooks/sld.tssrc/lib/ckan-translations.tssrc/lib/ogc.tssrc/lib/resource.tssrc/lib/sld.tssrc/schemas/ckan/dataset.interface.d.tssrc/types/leaflet-sld.d.tssrc/types/styles.d.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/package/dataset/DatasetResource.tsx (1)
29-33:⚠️ Potential issue | 🟡 MinorMove
divide-yfrom the wrapper<div>to the<ul>so dividers appear between resource items.The
divide-yutility applies borders using the CSS child combinator, which only targets direct children of the element it's applied to. With resources wrapped in<li>elements inside the<ul>, the current placement on the parent<div>won't create dividers between list items. Apply it to the<ul>instead.Proposed fix
- <div className="w-full divide-y"> + <div className="w-full"> {!resources?.length && ( <p className="text-gray-600 py-3">{t("Dataset.noResources")}</p> )} - <ul> + <ul className="divide-y">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/package/dataset/DatasetResource.tsx` around lines 29 - 33, The wrapper <div> currently has the "divide-y" utility which doesn't target the <li> children inside the <ul>; move the "divide-y" class from the outer <div> to the <ul> so the CSS child combinator applies to the list items and creates dividers between resource entries (update the className on the <div> and the <ul> in DatasetResource component accordingly).
🧹 Nitpick comments (1)
src/lib/sld.ts (1)
86-90:extractFieldsassumescomparison.propertyis always defined.
comparison.property.trim()will throw ifpropertyis everundefined/null. IfSldComparison.propertyis strictly typed asstringthis is safe, but defensive handling would prevent a crash if upstream SLD parsing ever yields a malformed comparison.♻️ Suggested hardening
- return [...new Set(filter.comparisions.map((comparison) => comparison.property.trim()).filter(Boolean))]; + return [ + ...new Set( + filter.comparisions + .map((comparison) => (comparison.property ?? "").trim()) + .filter(Boolean) + ), + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sld.ts` around lines 86 - 90, The function extractFields assumes SldComparison.property is always a string and calls comparison.property.trim(), which can throw if property is undefined/null; update extractFields to defensively filter out comparisons without a string property before trimming: first ensure filter and filter.comparisions exist, then .filter(comparison => typeof comparison.property === 'string' && comparison.property.trim() !== '') and finally .map(comparison => comparison.property.trim()) (then wrap with new Set as before) so only valid, trimmed property strings are returned.
🤖 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/lib/sld.ts`:
- Around line 46-76: The current buildComparisonLabel misclassifies wildcards
because startsWithWildcard, endsWithWildcard and hasSingleChar use
literal.includes/startsWith/endsWith without honoring escaping; update
buildComparisonLabel to scan the comparison.literal char-by-char using the
escape character (use comparison.escape or a sensible default) and only treat
wildCard/singleChar as wildcards if they are not preceded by an unescaped escape
char; keep using normalizeLikeLiteral(comparison) for normalizedValue but
replace the simple boolean checks (startsWithWildcard, endsWithWildcard,
hasSingleChar) with the escape-aware scanning logic so the subsequent branches
(contains/starts with/ends with/matches) behave consistently with
normalizeLikeLiteral.
---
Outside diff comments:
In `@src/components/package/dataset/DatasetResource.tsx`:
- Around line 29-33: The wrapper <div> currently has the "divide-y" utility
which doesn't target the <li> children inside the <ul>; move the "divide-y"
class from the outer <div> to the <ul> so the CSS child combinator applies to
the list items and creates dividers between resource entries (update the
className on the <div> and the <ul> in DatasetResource component accordingly).
---
Nitpick comments:
In `@src/lib/sld.ts`:
- Around line 86-90: The function extractFields assumes SldComparison.property
is always a string and calls comparison.property.trim(), which can throw if
property is undefined/null; update extractFields to defensively filter out
comparisons without a string property before trimming: first ensure filter and
filter.comparisions exist, then .filter(comparison => typeof comparison.property
=== 'string' && comparison.property.trim() !== '') and finally .map(comparison
=> comparison.property.trim()) (then wrap with new Set as before) so only valid,
trimmed property strings are returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82b92880-0fc4-4575-8e8f-fa450c653d27
📒 Files selected for processing (22)
README.mddocs/accessibility-fixes.mddocs/env-vars/README.mdsrc/app/[locale]/[org]/[dataset]/[resource]/page.tsxsrc/app/[locale]/[org]/[dataset]/page.tsxsrc/app/[locale]/about-us/page.tsxsrc/app/[locale]/accessibility-statement/page.tsxsrc/app/[locale]/data/page.tsxsrc/app/[locale]/page.tsxsrc/app/[locale]/themes/page.tsxsrc/app/globals.csssrc/components/layout/Footer.tsxsrc/components/package/dataset/DatasetResource.tsxsrc/components/package/resource/OgcServiceMapPreview.tsxsrc/components/package/resource/ResourceDetails.tsxsrc/components/package/search/SearchLayout.tsxsrc/components/ui/markdown.tsxsrc/hooks/sld.tssrc/lib/env.tssrc/lib/isr.tssrc/lib/ogc.tssrc/lib/sld.ts
✅ Files skipped from review due to trivial changes (10)
- src/components/package/resource/ResourceDetails.tsx
- README.md
- src/app/[locale]/accessibility-statement/page.tsx
- src/app/[locale]/about-us/page.tsx
- docs/env-vars/README.md
- src/app/[locale]/data/page.tsx
- src/lib/env.ts
- src/lib/isr.ts
- docs/accessibility-fixes.md
- src/lib/ogc.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/components/ui/markdown.tsx
- src/hooks/sld.ts
- src/app/[locale]/[org]/[dataset]/page.tsx
- src/app/[locale]/[org]/[dataset]/[resource]/page.tsx
- src/app/[locale]/page.tsx
- src/components/package/search/SearchLayout.tsx
- src/components/layout/Footer.tsx
- src/components/package/resource/OgcServiceMapPreview.tsx
Comment on lines
+46
to
+76
| function buildComparisonLabel(comparison: SldComparison): string { | ||
| if (comparison.operator !== "like") { | ||
| return `${comparison.property} ${comparison.operator} ${comparison.literal}`; | ||
| } | ||
|
|
||
| const literal = comparison.literal ?? ""; | ||
| const wildCard = comparison.wildCard || "*"; | ||
| const singleChar = comparison.singleChar || "?"; | ||
| const startsWithWildcard = literal.startsWith(wildCard); | ||
| const endsWithWildcard = literal.endsWith(wildCard); | ||
| const hasSingleChar = literal.includes(singleChar); | ||
| const normalizedValue = normalizeLikeLiteral(comparison) || literal; | ||
|
|
||
| if (hasSingleChar) { | ||
| return `${comparison.property} matches ${literal}`; | ||
| } | ||
|
|
||
| if (startsWithWildcard && endsWithWildcard) { | ||
| return `${comparison.property} contains ${normalizedValue}`; | ||
| } | ||
|
|
||
| if (endsWithWildcard) { | ||
| return `${comparison.property} starts with ${normalizedValue}`; | ||
| } | ||
|
|
||
| if (startsWithWildcard) { | ||
| return `${comparison.property} ends with ${normalizedValue}`; | ||
| } | ||
|
|
||
| return `${comparison.property} matches ${literal}`; | ||
| } |
There was a problem hiding this comment.
hasSingleChar ignores escaping, potentially misclassifying literals with escaped ?.
literal.includes(singleChar) reports a match even when the occurrence is escaped (e.g., literal !?foo with escape !, singleChar ?). That would force the matches ${literal} branch and also prevent the starts with / ends with / contains phrasings from being considered, even though semantically the ? is a literal character. Similarly, startsWithWildcard / endsWithWildcard don't check whether the leading/trailing wildcard is itself escaped.
Minor edge case — likely rare in practice since most SLDs don't escape wildcards — but worth a follow-up if you want the label logic to stay consistent with normalizeLikeLiteral's escape handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/sld.ts` around lines 46 - 76, The current buildComparisonLabel
misclassifies wildcards because startsWithWildcard, endsWithWildcard and
hasSingleChar use literal.includes/startsWith/endsWith without honoring
escaping; update buildComparisonLabel to scan the comparison.literal
char-by-char using the escape character (use comparison.escape or a sensible
default) and only treat wildCard/singleChar as wildcards if they are not
preceded by an unescaped escape char; keep using
normalizeLikeLiteral(comparison) for normalizedValue but replace the simple
boolean checks (startsWithWildcard, endsWithWildcard, hasSingleChar) with the
escape-aware scanning logic so the subsequent branches (contains/starts
with/ends with/matches) behave consistently with normalizeLikeLiteral.
mpolidori
approved these changes
Apr 21, 2026
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
2 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Accessibility
Documentation