Learning Tooltip - #1221
Open
crutchcorn wants to merge 40 commits into
Open
Conversation
Member
Author
fennifith
marked this pull request as ready for review
May 9, 2025 22:58
…n after the button
Member
|
TODO:
|
# Conflicts: # __mocks__/setup.ts # content/fennifith/posts/example/index.md # src/components/dialog/dialog.tsx # src/pages/[...locale]/posts/[postid].astro # src/styles/markdown/base.scss # src/types/index.ts # src/utils/api.ts # src/utils/data.ts # src/utils/markdown/components/index.ts # src/utils/markdown/createEpubPlugins.ts # src/utils/markdown/createHtmlPlugins.ts # src/utils/markdown/getMarkdownHtml.ts # src/utils/markdown/types.ts # src/utils/smooth-scroll-for-anchors-to-current-page.ts # src/views/blog-post/blog-post.astro # src/views/search/search-page.tsx # src/views/search/search-page.ui.spec.tsx # src/views/search/search.ts
Contributor
📝 WalkthroughWalkthroughThis change adds Snitip data models, Markdown parsing and link transforms, responsive popover and dialog interfaces, client interaction handling, and search-result integration. It also updates shared styles, dialog behavior, and test setup. ChangesSnitip data and Markdown pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Markdown
participant VFile
participant SnitipLink
participant SnitipTemplate
participant Browser
Markdown->>VFile: Parse and store SnitipInfo
Markdown->>SnitipLink: Resolve pfp-snitip link
SnitipLink-->>Markdown: Render scoped trigger
Markdown->>SnitipTemplate: Append resolved template
SnitipTemplate-->>Browser: Render popover and dialog markup
Browser->>SnitipLink: Receive hover, focus, or click
SnitipLink->>Browser: Open responsive popover or dialog
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/views/base/scripts/snitip-script-impl.ts (3)
345-354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
popoverEl.popover = "auto"appears to have no effect.No code path in this file sets
popoverto"manual". The popover is created from the template with its authoredpopoverattribute and is never reassigned. Reassigning"auto"in the close transition is therefore a no-op.If a manual-mode path was removed, delete this line. If a manual mode is still planned, add a comment that names the path which sets it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/views/base/scripts/snitip-script-impl.ts` around lines 345 - 354, Remove the `popoverEl.popover = "auto"` assignment from the `toggle` event handler’s closed-state branch, since no manual-mode path exists in the current implementation. Keep the existing `handleSnitipClosed(snitipElements)` behavior unchanged.
252-259: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRegistered
resizelisteners accumulate, one per snitip dialog.
initializeDialogruns for every trigger with a distinct dialog. Each call registers a permanentresizelistener that writesdialogEl.dataset.scrolled. A page with many snitips therefore performs one DOM write per dialog on every resize event, including for dialogs that are closed.Register a single shared
resizehandler that updates only the open dialog, or add and remove the listener in the dialogcloseandopentransitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/views/base/scripts/snitip-script-impl.ts` around lines 252 - 259, Update initializeDialog’s resize handling so each snitip dialog does not permanently register its own window listener. Use one shared handler that updates only the currently open dialog, or attach and detach the existing handleDialogScroll listener during the dialog’s open and close transitions; preserve scroll-event behavior for the active dialog.
133-140: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the rects to avoid a forced layout on every
mousemove.
handleMouseMoveis registered ondocument. While a hover-opened snitip is visible, every pointer move callsisInsideSnitip, which callsgetBoundingClientRect()on bothtriggerElandpopoverEl. Each call forces a synchronous layout. On a long markdown page this produces measurable jank during pointer movement.The trigger and popover geometry only changes on scroll and resize. Both events are already tracked in
handleSnitipOpened. Measure once when the snitip opens, and refresh the cached rects insidepositionSnitip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/views/base/scripts/snitip-script-impl.ts` around lines 133 - 140, Update the snitip state and handleSnitipOpened flow to cache triggerEl and popoverEl bounding rectangles when the snitip opens, then refresh those cached rectangles inside positionSnitip on its existing scroll/resize updates. Change isInsideSnitip and handleMouseMove to use the cached geometry instead of calling getBoundingClientRect() for every pointer event.src/components/snitip/snitip-card.tsx (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCaller-supplied
classis dropped onSnitipCardGrid.
SnitipCardGridPropsextendsHTMLAttributes<HTMLUListElement>, soclassandroleare part of the accepted prop surface. The literalclass={style.list}follows{...extra}, so any caller value is overridden without warning. Merge the values instead.♻️ Proposed change to merge the caller class
export function SnitipCardGrid({ snitips, headingTag, + class: className, ...extra }: SnitipCardGridProps) { return ( - <ul {...extra} role="list" class={style.list}> + <ul {...extra} role="list" class={`${style.list} ${className ?? ""}`}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/snitip/snitip-card.tsx` at line 25, Update the SnitipCardGrid list element’s class handling where {...extra} is spread so the caller-provided class is merged with style.list instead of being overwritten. Preserve the existing role and other HTML attributes from extra, while ensuring the component’s list styling remains applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/dialog/dialog.tsx`:
- Around line 47-55: Update the fallback branch in the dialog event handler to
explicitly close dialogRef.current after a backdrop click, while still invoking
onClose() and preserving undefined as the light-dismiss return value. Limit the
change to the closedBy-unsupported path in the dialog component.
In `@src/components/snitip/snitip.tsx`:
- Around line 41-44: Sanitize SnitipInfo.content before assigning it to
dangerouslySetInnerHTML in the rendered description div. Update the
transformSnitip/content flow to remove executable elements, event-handler
attributes, and unsafe URL protocols while preserving safe HTML output.
In `@src/styles/markdown/base.scss`:
- Around line 37-39: Update the universal selector in the markdown base styles
so scroll-margin-top applies only to hash-targetable content, excluding
SnitipDialog and its .form scroll container. Narrow the selector or add an
explicit exclusion while preserving the existing page-anchor offset behavior.
In `@src/utils/data.ts`:
- Around line 128-134: Enforce the maximum of four links in both Snitip
ingestion paths: validate frontmatter.links before storing the global Snitip in
src/utils/data.ts lines 128-134, and validate the extracted trailing link list
before storing the inline Snitip in
src/utils/markdown/components/snitip/rehype-transform.ts lines 84-103. Reject
invalid content or truncate it with an author-visible error, ensuring no Snitip
is stored with more than four links.
In `@src/utils/markdown/components/snitip/rehype-transform.ts`:
- Around line 41-58: Update the heading validation in the Snitip transformation
to require headingIndex === 0, not merely a non-negative index. Log the existing
“Snitip must start with a heading!” error and return when any preceding child
exists, while preserving the current heading, image, and contents processing for
valid input.
In `@src/utils/markdown/snitip-link/SnitipLink.tsx`:
- Around line 33-44: Update the desktop trigger in SnitipLink so popover open
and close transitions are announced to assistive technology, using an accessible
state/status mechanism or focus-managed interaction tied to the existing
popoverId. Add coverage that verifies screen-reader announcements for both
opening and closing the Snitip popover.
In `@src/views/search/search-page.tsx`:
- Around line 236-242: Update the search page’s filter-query hook to expose its
refetch function, then invoke that function alongside the existing
search-results refetch in the Retry handler. Preserve the current error-state
behavior while ensuring retrying recovers from /searchFilters.json failures.
---
Nitpick comments:
In `@src/components/snitip/snitip-card.tsx`:
- Line 25: Update the SnitipCardGrid list element’s class handling where
{...extra} is spread so the caller-provided class is merged with style.list
instead of being overwritten. Preserve the existing role and other HTML
attributes from extra, while ensuring the component’s list styling remains
applied.
In `@src/views/base/scripts/snitip-script-impl.ts`:
- Around line 345-354: Remove the `popoverEl.popover = "auto"` assignment from
the `toggle` event handler’s closed-state branch, since no manual-mode path
exists in the current implementation. Keep the existing
`handleSnitipClosed(snitipElements)` behavior unchanged.
- Around line 252-259: Update initializeDialog’s resize handling so each snitip
dialog does not permanently register its own window listener. Use one shared
handler that updates only the currently open dialog, or attach and detach the
existing handleDialogScroll listener during the dialog’s open and close
transitions; preserve scroll-event behavior for the active dialog.
- Around line 133-140: Update the snitip state and handleSnitipOpened flow to
cache triggerEl and popoverEl bounding rectangles when the snitip opens, then
refresh those cached rectangles inside positionSnitip on its existing
scroll/resize updates. Change isInsideSnitip and handleMouseMove to use the
cached geometry instead of calling getBoundingClientRect() for every pointer
event.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57df08ce-d49a-4176-9498-81f9f5afbd85
⛔ Files ignored due to path filters (6)
content/data/snitips/git.mdis excluded by!content/**content/data/snitips/javascript.mdis excluded by!content/**content/data/snitips/ssg.mdis excluded by!content/**content/data/snitips/ssr.mdis excluded by!content/**content/data/tags.jsonis excluded by!content/**content/fennifith/posts/example/index.mdis excluded by!content/**
📒 Files selected for processing (35)
__mocks__/setup.tssrc/components/chip/chip.tsxsrc/components/dialog/dialog.tsxsrc/components/inline-popup/inline-popup.scsssrc/components/inline-popup/inline-popup.tsxsrc/components/snitip/snitip-card.module.scsssrc/components/snitip/snitip-card.tsxsrc/components/snitip/snitip-dialog.tsxsrc/components/snitip/snitip.module.scsssrc/components/snitip/snitip.tsxsrc/pages/searchFilters.json.tssrc/styles/markdown/base.scsssrc/types/SnitipInfo.tssrc/types/index.tssrc/utils/api.tssrc/utils/data.tssrc/utils/markdown/components/components.tssrc/utils/markdown/components/index.tssrc/utils/markdown/components/snitip/rehype-transform.tssrc/utils/markdown/components/snitip/snitip-template.astrosrc/utils/markdown/createEpubPlugins.tssrc/utils/markdown/createHtmlPlugins.tssrc/utils/markdown/getMarkdownVFile.tssrc/utils/markdown/snitip-link/SnitipLink.tsxsrc/utils/markdown/snitip-link/rehype-transform-epub.tssrc/utils/markdown/snitip-link/rehype-transform.tssrc/utils/markdown/types.tssrc/views/about/about.astrosrc/views/base/scripts/snitip-script-impl.tssrc/views/base/scripts/snitip-trigger.scsssrc/views/collections/framework-field-guide/segments/code-block.astrosrc/views/search/search-page.module.scsssrc/views/search/search-page.tsxsrc/views/search/search-page.ui.spec.tsxsrc/views/search/search.ts
Comment on lines
47
to
+55
| (e: Event) => { | ||
| if (e.target === dialogRef.current) onClose(); | ||
| // https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/closedBy | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore Missing DOM types | ||
| if (typeof dialogRef.current?.closedBy == "undefined") { | ||
| if (e.target === dialogRef.current) { | ||
| onClose(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Close the fallback dialog on backdrop click.
When closedBy is unsupported, this handler calls onClose() but leaves the DOM dialog open. SnitipDialog supplies open={false} and ignores onClose, so a backdrop click cannot dismiss its modal in those browsers. Close the dialog in this branch and preserve undefined as the light-dismiss return value.
Proposed fix
const dialogRef = useRef<HTMLDialogElement>(null);
+const lightDismissedRef = useRef(false);
// ...
if (typeof dialogRef.current?.closedBy == "undefined") {
if (e.target === dialogRef.current) {
- onClose();
+ lightDismissedRef.current = true;
+ dialogRef.current.close();
}
}
const handleClose = useCallback(() => {
- onClose(dialogRef.current?.returnValue);
+ const returnValue = lightDismissedRef.current
+ ? undefined
+ : dialogRef.current?.returnValue;
+ lightDismissedRef.current = false;
+ onClose(returnValue);
}, [onClose]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/dialog/dialog.tsx` around lines 47 - 55, Update the fallback
branch in the dialog event handler to explicitly close dialogRef.current after a
backdrop click, while still invoking onClose() and preserving undefined as the
light-dismiss return value. Limit the change to the closedBy-unsupported path in
the dialog component.
Comment on lines
+41
to
+44
| <div | ||
| class={style.description} | ||
| dangerouslySetInnerHTML={{ __html: snitip.content }} | ||
| /> |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/utils/markdown/components/snitip/rehype-transform.ts --items all
rg -n -C 4 \
-g '*.ts' -g '*.tsx' \
'rehype-sanitize|sanitize|SnitipInfo|snitip.*content|content.*snitip' \
src/utils/markdown src/components/snitipRepository: playfulprogramming/playfulprogramming
Length of output: 10122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- snitip producer ---'
cat -n src/utils/markdown/components/snitip/rehype-transform.ts | sed -n '1,155p'
printf '%s\n' '--- serializable-node helper and related transforms ---'
rg -n -C 5 \
'isSerializableHastNode|serializableContents|rehype-sanitize|sanitize|allowDangerousHtml|toHtml\(' \
src
printf '%s\n' '--- SnitipInfo sources and assignments ---'
rg -n -C 6 \
'interface SnitipInfo|type SnitipInfo|SnitipInfo\s*=|content:\s*' \
src typesRepository: playfulprogramming/playfulprogramming
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- browser Markdown pipeline ---'
cat -n src/utils/markdown/createHtmlPlugins.ts | sed -n '45,145p'
printf '%s\n' '--- global SnitipInfo producer ---'
cat -n src/utils/data.ts | sed -n '45,140p'
printf '%s\n' '--- SnitipInfo definitions ---'
cat -n src/types/SnitipInfo.ts
printf '%s\n' '--- all direct SnitipInfo content assignments ---'
rg -n --glob '*.ts' --glob '*.tsx' \
'content:\s*(toHtml|snitipHtml|[^,}]+)' src/utils src/components src/typesRepository: playfulprogramming/playfulprogramming
Length of output: 8939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
producer = Path("src/utils/markdown/components/snitip/rehype-transform.ts").read_text()
pipeline = Path("src/utils/markdown/createHtmlPlugins.ts").read_text()
renderer = Path("src/components/snitip/snitip.tsx").read_text()
checks = {
"producer serializes serializable contents": r"content:\s*toHtml\(serializableContents\)",
"raw HAST nodes are accepted": r'\["comment",\s*"element",\s*"raw",\s*"text"\]',
"raw HTML is enabled before rehypeRaw": r"\.use\(remarkToRehype,\s*\{\s*allowDangerousHtml:\s*true\s*\}\).*?\.use\(rehypeRaw",
"no sanitizer is configured in browser pipeline": r"rehype-sanitize|sanitize",
"renderer bypasses escaping": r"dangerouslySetInnerHTML=\{\{\s*__html:\s*snitip\.content\s*\}\}",
}
for label, pattern in checks.items():
source = producer if "producer" in label or "raw HAST" in label else pipeline if "pipeline" in label or "sanitizer" in label else renderer
flags = re.S if "raw HTML" in label else 0
print(f"{label}: {'YES' if re.search(pattern, source, flags) else 'NO'}")
PYRepository: playfulprogramming/playfulprogramming
Length of output: 379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- component validation and HTML mapping ---'
rg -n -C 8 \
'export .*rehypeValidateComponents|function rehypeValidateComponents|rehypeValidateComponents|rehypePlayfulElementMap|export .*rehypeRaw' \
src/utils/markdown
printf '%s\n' '--- sanitizer dependencies and configuration ---'
rg -n -i \
'rehype-sanitize|sanitize-html|dompurify|xss|sanitize' \
package.json package-lock.json pnpm-lock.yaml yarn.lock bun.lockb src 2>/dev/null || true
printf '%s\n' '--- exact pipeline assertions ---'
python3 - <<'PY'
from pathlib import Path
pipeline = Path("src/utils/markdown/createHtmlPlugins.ts").read_text()
producer = Path("src/utils/markdown/components/snitip/rehype-transform.ts").read_text()
renderer = Path("src/components/snitip/snitip.tsx").read_text()
def between(text, start, end):
return text[text.index(start):text.index(end)]
pipeline_chain = between(pipeline, ".use(remarkToRehype", ".use(rehypePluginComponents")
assert "allowDangerousHtml: true" in pipeline_chain
assert ".use(rehypeRaw" in pipeline_chain
assert "rehype-sanitize" not in pipeline_chain
assert '["comment", "element", "raw", "text"]' in producer
assert "content: toHtml(serializableContents)" in producer
assert "dangerouslySetInnerHTML" in renderer
print("raw HTML enabled: YES")
print("rehypeRaw enabled: YES")
print("rehype-sanitize configured: NO")
print("raw HAST nodes reach SnitipInfo.content: YES")
print("SnitipInfo.content reaches dangerouslySetInnerHTML: YES")
PYRepository: playfulprogramming/playfulprogramming
Length of output: 8661
Sanitize SnitipInfo.content before injection.
transformSnitip accepts raw nodes and serializes them with toHtml. The Markdown pipeline enables dangerous HTML, and no sanitizer removes executable elements, event-handler attributes, or unsafe URL protocols before snitip.content reaches dangerouslySetInnerHTML.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 42-42: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(react-unsafe-html-injection)
🪛 React Doctor (0.9.3)
[error] 43-43: dangerouslySetInnerHTML is an XSS hole that runs attacker-controlled HTML in your users' browsers.
Render trusted content as React children so attacker-controlled HTML cannot run in users' browsers.
(no-danger)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/snitip/snitip.tsx` around lines 41 - 44, Sanitize
SnitipInfo.content before assigning it to dangerouslySetInnerHTML in the
rendered description div. Update the transformSnitip/content flow to remove
executable elements, event-handler attributes, and unsafe URL protocols while
preserving safe HTML output.
Source: Linters/SAST tools
Comment on lines
+37
to
+39
| * { | ||
| scroll-margin-top: inherit; | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where the snitip dialog is rendered relative to .post-body
rg -n -C 6 'snitip-dialog' --glob '!**/node_modules/**'
# Find where .post-body wraps rendered markdown content
rg -n -C 5 'post-body' --glob '*.astro' --glob '*.tsx' --glob '!**/node_modules/**'
# Find declared scroll containers that may be nested inside markdown content
rg -n -C 3 'overflow(-x|-y)?\s*:\s*(auto|scroll)' --glob '*.scss' --glob '!**/node_modules/**'Repository: playfulprogramming/playfulprogramming
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(base\.scss|snitip|.*\.(astro|tsx|scss))$' | head -200
printf '%s\n' '--- snitip references ---'
rg -n -C 8 'snitip|Snitip' --glob '!**/node_modules/**' . || true
printf '%s\n' '--- post-body references ---'
rg -n -C 8 'post-body' --glob '*.astro' --glob '*.tsx' --glob '*.scss' --glob '!**/node_modules/**' . || true
printf '%s\n' '--- overflow declarations ---'
rg -n -C 4 'overflow(-x|-y)?\s*:\s*(auto|scroll)' --glob '*.scss' --glob '*.astro' --glob '*.tsx' --glob '!**/node_modules/**' . || trueRepository: playfulprogramming/playfulprogramming
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -e
for f in $(git ls-files | rg 'snitip-script-impl\.ts$|base\.scss$'); do
echo "--- $f ---"
wc -l "$f"
rg -n -C 12 'scrollIntoView|scrollTop|overflow|snitip|post-body|dialog|form' "$f" || true
doneRepository: playfulprogramming/playfulprogramming
Length of output: 12009
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- all tracked markup/style files ---'
git ls-files | rg '\.(astro|tsx|jsx|html|scss|css)$' | head -300
printf '%s\n' '--- all relevant class/id tokens ---'
rg -n -C 5 'post-body|snitip-dialog|snitip|overflow\s*:|overflow-(x|y)\s*:' \
$(git ls-files | rg '\.(astro|tsx|jsx|html|scss|css)$') || trueRepository: playfulprogramming/playfulprogramming
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -e
echo '--- source tree ---'
git ls-files | sed -n '1,240p'Repository: playfulprogramming/playfulprogramming
Length of output: 20288
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- locate relevant files ---'
git ls-files | rg 'snitip|base\.scss|post|markdown|dialog' || true
printf '%s\n' '--- relevant references ---'
rg -n -C 8 'snitip|post-body|scroll-margin-top|overflow(-x|-y)?\s*:' . \
--glob '!**/node_modules/**' \
--glob '!**/.git/**' || trueRepository: playfulprogramming/playfulprogramming
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -e
for f in \
"src/pages/[...locale]/posts/[postid].astro" \
"src/utils/markdown/components/content.astro" \
"src/utils/markdown/components/snitip/snitip-template.astro" \
"src/components/snitip/snitip-dialog.tsx" \
"src/components/snitip/snitip-card.module.scss" \
"src/components/snitip/snitip.module.scss" \
"src/styles/post-body.scss" \
"src/styles/markdown/base.scss" \
"src/views/base/scripts/snitip-script-impl.ts"
do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n -C 8 'post-body|Content|Snitip|dialog|form|overflow|scroll' "$f" || true
fi
doneRepository: playfulprogramming/playfulprogramming
Length of output: 16095
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- .post-body definitions and uses ---'
rg -n -C 10 '\.post-body|class(Name)?=.*post-body|post-body' src --glob '!**/node_modules/**' || true
printf '%s\n' '--- Markdown content component declarations and calls ---'
rg -n -C 10 'content\.astro|Markdown|markdown|Content' \
src/pages src/views src/layouts src/utils/markdown \
--glob '*.astro' --glob '*.tsx' --glob '*.ts' || true
printf '%s\n' '--- dialog component structure ---'
sed -n '1,220p' src/components/dialog/dialog.tsxRepository: playfulprogramming/playfulprogramming
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- .post-body definitions and uses ---'
rg -n -C 10 '\.post-body|class(Name)?=.*post-body|post-body' src \
--glob '!**/node_modules/**' || true
printf '%s\n' '--- Markdown content component declarations and calls ---'
rg -n -C 10 'content\.astro|Markdown|markdown|Content' \
src/pages src/views src/layouts src/utils/markdown \
--glob '*.astro' --glob '*.tsx' --glob '*.ts' || true
printf '%s\n' '--- dialog component structure ---'
sed -n '1,220p' src/components/dialog/dialog.tsxRepository: playfulprogramming/playfulprogramming
Length of output: 50394
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
blog = Path("src/views/blog-post/blog-post.astro").read_text()
template = Path("src/utils/markdown/components/snitip/snitip-template.astro").read_text()
script = Path("src/views/base/scripts/snitip-script-impl.ts").read_text()
styles = Path("src/components/snitip/snitip.module.scss").read_text()
markdown_css = Path("src/styles/markdown/base.scss").read_text()
transform = Path("src/utils/markdown/snitip-link/rehype-transform.ts").read_text()
checks = {
"Markdown content is inside .post-body":
'<section' in blog and 'class="post-body"' in blog
and '<Content content={postHtml.content} />' in blog,
"SnitipDialog is emitted outside the popover template":
'<template' in template
and '</template>' in template
and template.index("<SnitipDialog") > template.index("</template>"),
"Snitip templates are appended to the Markdown root":
'tree.children.push(' in transform
and 'createComponent("SnitipTemplate"' in transform,
"Only the popover is moved to document.body":
'document.body.append(popoverEl)' in script
and 'document.body.append(dialogEl)' not in script,
"The dialog form is a scroll container":
'.form {' in styles and 'overflow-y: auto' in styles,
"The universal descendant rule assigns the post offset":
'.post-body' in markdown_css
and 'scroll-margin-top: inherit' in markdown_css,
}
for name, result in checks.items():
print(f"{'PASS' if result else 'FAIL'}: {name}")
if not all(checks.values()):
raise SystemExit(1)
print("RESULT: the snitip dialog form remains a descendant scroll container of .post-body.")
PYRepository: playfulprogramming/playfulprogramming
Length of output: 572
Limit scroll-margin-top to hash-targetable elements. SnitipDialog remains inside .post-body, and its .form is an overflow-y: auto scroll container. The universal rule can apply the page offset to dialog scrolling. Exclude the snitip dialog or narrow the selector.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/styles/markdown/base.scss` around lines 37 - 39, Update the universal
selector in the markdown base styles so scroll-margin-top applies only to
hash-targetable content, excluding SnitipDialog and its .form scroll container.
Narrow the selector or add an explicit exclusion while preserving the existing
page-anchor offset behavior.
Comment on lines
+128
to
+134
| const snitip: SnitipInfo = { | ||
| ...(frontmatter as RawSnitipInfo), | ||
| id: snitipId, | ||
| tagsMeta, | ||
| content: snitipHtml, | ||
| }; | ||
| snitips.set(snitipId, snitip); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce the four-link Snitip limit in both ingestion paths.
The PR requirement permits up to four links. Global Snitips and inline Snitips currently accept any number of links. Reject invalid source content, or truncate it with an author-visible error.
src/utils/data.ts#L128-L134: validatefrontmatter.links.lengthbefore storing the global Snitip.src/utils/markdown/components/snitip/rehype-transform.ts#L84-L103: validate the extracted trailing link list before storing the inline Snitip.
📍 Affects 2 files
src/utils/data.ts#L128-L134(this comment)src/utils/markdown/components/snitip/rehype-transform.ts#L84-L103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/data.ts` around lines 128 - 134, Enforce the maximum of four links
in both Snitip ingestion paths: validate frontmatter.links before storing the
global Snitip in src/utils/data.ts lines 128-134, and validate the extracted
trailing link list before storing the inline Snitip in
src/utils/markdown/components/snitip/rehype-transform.ts lines 84-103. Reject
invalid content or truncate it with an author-visible error, ensuring no Snitip
is stored with more than four links.
Comment on lines
+41
to
+58
| const headingIndex = children.findIndex( | ||
| (node) => isElement(node) && isNodeHeading(node), | ||
| ); | ||
|
|
||
| if (headingIndex < 0) { | ||
| logError(vfile, node, "Snitip must start with a heading!"); | ||
| return; | ||
| } | ||
|
|
||
| const heading = children[headingIndex] as Element; | ||
| const imageEl = heading.children | ||
| .filter(isElement) | ||
| .find((node) => node.tagName === "picture") | ||
| ?.children?.filter(isElement) | ||
| ?.find((node) => node.tagName === "img"); | ||
|
|
||
| const title = toString(heading); | ||
| const contents = children.slice(headingIndex + 1); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require the heading to be the first child.
Line 41 accepts a heading after preceding content. Lines 57-58 then discard that preceding content without an error. Reject the Snitip unless headingIndex === 0.
Proposed fix
- if (headingIndex < 0) {
+ if (headingIndex !== 0) {
logError(vfile, node, "Snitip must start with a heading!");
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const headingIndex = children.findIndex( | |
| (node) => isElement(node) && isNodeHeading(node), | |
| ); | |
| if (headingIndex < 0) { | |
| logError(vfile, node, "Snitip must start with a heading!"); | |
| return; | |
| } | |
| const heading = children[headingIndex] as Element; | |
| const imageEl = heading.children | |
| .filter(isElement) | |
| .find((node) => node.tagName === "picture") | |
| ?.children?.filter(isElement) | |
| ?.find((node) => node.tagName === "img"); | |
| const title = toString(heading); | |
| const contents = children.slice(headingIndex + 1); | |
| const headingIndex = children.findIndex( | |
| (node) => isElement(node) && isNodeHeading(node), | |
| ); | |
| if (headingIndex !== 0) { | |
| logError(vfile, node, "Snitip must start with a heading!"); | |
| return; | |
| } | |
| const heading = children[headingIndex] as Element; | |
| const imageEl = heading.children | |
| .filter(isElement) | |
| .find((node) => node.tagName === "picture") | |
| ?.children?.filter(isElement) | |
| ?.find((node) => node.tagName === "img"); | |
| const title = toString(heading); | |
| const contents = children.slice(headingIndex + 1); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/markdown/components/snitip/rehype-transform.ts` around lines 41 -
58, Update the heading validation in the Snitip transformation to require
headingIndex === 0, not merely a non-negative index. Log the existing “Snitip
must start with a heading!” error and return when any preceding child exists,
while preserving the current heading, image, and contents processing for valid
input.
Comment on lines
+33
to
+44
| <button | ||
| type="button" | ||
| class="snitip-trigger__button" | ||
| popovertarget={popoverId} | ||
| popovertargetaction="show" | ||
| aria-label={`Open tooltip for "${props.snitip.title}"`} | ||
| > | ||
| <span class="snitip-trigger__popup inline-popup"> | ||
| <span class="inline-popup__content">Open tooltip</span> | ||
| </span> | ||
| {InfoIcon} | ||
| </button> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Announce Snitip popover state changes.
The desktop trigger must announce when the Snitip popover opens and closes. The linked review reports that screen readers currently miss both events. Add an accessible state or status mechanism, or use an interaction that manages focus and announcement. Add assistive-technology coverage for open and close behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/markdown/snitip-link/SnitipLink.tsx` around lines 33 - 44, Update
the desktop trigger in SnitipLink so popover open and close transitions are
announced to assistive technology, using an accessible state/status mechanism or
focus-managed interaction tied to the existing popoverId. Add coverage that
verifies screen-reader announcements for both opening and closing the Snitip
popover.
Comment on lines
+236
to
+242
| const isError = isErrorFilters || isErrorData; | ||
|
|
||
| useEffect(() => { | ||
| if (errorPeople) { | ||
| console.error("There was an error", { error: errorPeople }); | ||
| if (errorFilters) { | ||
| console.error("There was an error", { error: errorFilters }); | ||
| } | ||
| }, [errorPeople]); | ||
| }, [errorFilters]); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Retry the search-filter query.
If /searchFilters.json fails, isErrorFilters keeps isError true. The Retry button only calls the search-results refetch(). The page cannot recover from a filter-query failure.
Expose the filter query refetch function and call it from the Retry handler with the search-results refetch.
Proposed fix
const {
+ refetch: refetchFilters,
isLoading: isLoadingFilters,
// ...
} = useQuery({
// Retry button
- onClick={() => refetch()}
+ onClick={() => void Promise.all([refetch(), refetchFilters()])}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/views/search/search-page.tsx` around lines 236 - 242, Update the search
page’s filter-query hook to expose its refetch function, then invoke that
function alongside the existing search-results refetch in the Retry handler.
Preserve the current error-state behavior while ensuring retrying recovers from
/searchFilters.json failures.
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.
Preview: https://pr-1221-playful-programming-preview-playfulprogramming.fly.dev/posts/example/#Tooltips
TODO:
Closes #1160
Summary by CodeRabbit