Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTag navigation now syncs with the URL: clicks push/replace Changes
Sequence DiagramsequenceDiagram
participant User
participant ListItem as Waves List Item
participant TagCtx as WavesTagFilterContext
participant Router
participant URL
User->>ListItem: click tag
ListItem->>ListItem: detect interactive target (includes img)
ListItem->>Router: check current pathname
alt Not on /waves
ListItem->>Router: push /waves?tag=encodeURIComponent(tag)
Router->>URL: update location (add tag)
else On /waves
ListItem->>TagCtx: setSelectedTag(tag)
TagCtx->>Router: replace URL (set/remove tag) with {scroll:false}
Router->>URL: sync query
TagCtx->>ListItem: context state updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (1)
apps/web/src/features/waves/styles/thread-render.scss (1)
146-156: Redundant rule: Lines 152-156 duplicate Lines 146-148.The selector
:first-child:nth-last-child(3)at line 146 already matches the first image only when exactly 3 images exist. The nested:has()block at lines 152-156 applies the identicalgrid-row: span 2to the same element under the same condition.Also, odd image counts beyond 3 (e.g., 5, 7) will have an orphaned single image on the last row with no special layout handling—this may or may not be intentional.
♻️ Suggested fix: remove duplicate rule
.markdown-image-container { border-radius: 0; margin: 0; &:first-child:nth-last-child(3) { grid-row: span 2; } } - - // Single image remaining (odd count: 3 images = 1 tall left + 2 right) - &:has(> .markdown-image-container:first-child:nth-last-child(3)) { - .markdown-image-container:first-child { - grid-row: span 2; - } - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/waves/styles/thread-render.scss` around lines 146 - 156, The rule inside the :has(> .markdown-image-container:first-child:nth-last-child(3)) block duplicates the standalone &:first-child:nth-last-child(3) selector and should be removed; locate the duplicated selector blocks referencing .markdown-image-container and delete the nested :has(...) rule so the first image gets grid-row: span 2 only once (keep the single &:first-child:nth-last-child(3) rule). If you intended special handling for other odd image counts (5, 7, etc.), add a separate, distinct selector/logic instead of duplicating the 3-image case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/waves/_context/waves-tag-filter-context.tsx`:
- Around line 21-24: The selectedTag state (initialized from initialTag) can
desync when the URL changes because useState only uses the initial value; add a
useEffect that reads searchParams.get("tag") (or initialTag) and calls
setSelectedTagState(newTag) only when it differs from current selectedTag to
keep state in sync with browser navigation; reference the existing symbols
useSearchParams, initialTag, selectedTag, setSelectedTagState and update the
effect dependencies to include searchParams (or initialTag) and selectedTag to
avoid infinite loops.
---
Nitpick comments:
In `@apps/web/src/features/waves/styles/thread-render.scss`:
- Around line 146-156: The rule inside the :has(>
.markdown-image-container:first-child:nth-last-child(3)) block duplicates the
standalone &:first-child:nth-last-child(3) selector and should be removed;
locate the duplicated selector blocks referencing .markdown-image-container and
delete the nested :has(...) rule so the first image gets grid-row: span 2 only
once (keep the single &:first-child:nth-last-child(3) rule). If you intended
special handling for other odd image counts (5, 7, etc.), add a separate,
distinct selector/logic instead of duplicating the 3-image case.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3817c3b9-2ffd-4e80-b57c-699dc8af68b8
📒 Files selected for processing (3)
apps/web/src/app/waves/_components/waves-list-item.tsxapps/web/src/app/waves/_context/waves-tag-filter-context.tsxapps/web/src/features/waves/styles/thread-render.scss
| const searchParams = useSearchParams(); | ||
| const router = useRouter(); | ||
| const initialTag = searchParams?.get("tag") ?? null; | ||
| const [selectedTag, setSelectedTagState] = useState<string | null>(initialTag); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are other patterns in the codebase that handle URL-state sync
rg -n "useSearchParams.*useState" --type=ts --type=tsx -C 3Repository: ecency/vision-next
Length of output: 89
🏁 Script executed:
# First, read the actual file to see the full context
cat -n apps/web/src/app/waves/_context/waves-tag-filter-context.tsx | head -40Repository: ecency/vision-next
Length of output: 1463
🏁 Script executed:
# Search for useSearchParams patterns with correct rg syntax (tsx is not a separate type)
rg -n "useSearchParams" apps/web/src --type ts -C 3 | head -100Repository: ecency/vision-next
Length of output: 9054
🏁 Script executed:
# Look for useEffect patterns that sync state with URL parameters
rg -n "useEffect.*searchParams\|useEffect.*useRouter" apps/web/src --type ts -C 2Repository: ecency/vision-next
Length of output: 44
🏁 Script executed:
# Read the rest of the file to see if there's a useEffect handling state sync
wc -l apps/web/src/app/waves/_context/waves-tag-filter-context.tsxRepository: ecency/vision-next
Length of output: 125
🏁 Script executed:
# Read lines after 40 to see the full component
tail -n +40 apps/web/src/app/waves/_context/waves-tag-filter-context.tsxRepository: ecency/vision-next
Length of output: 633
🏁 Script executed:
# Check if there are tests for this component
find apps/web -name "*waves*filter*" -o -name "*waves*context*" | grep -i testRepository: ecency/vision-next
Length of output: 44
🏁 Script executed:
# Check for tests related to waves tag filter
find apps/web -type f \( -name "*.spec.ts" -o -name "*.spec.tsx" -o -name "*.test.ts" -o -name "*.test.tsx" \) | xargs grep -l "WavesTagFilter\|waves-tag-filter" 2>/dev/nullRepository: ecency/vision-next
Length of output: 46
🏁 Script executed:
# Look for usages of WavesTagFilterProvider and useWavesTagFilter
rg "WavesTagFilter|useWavesTagFilter" apps/web/src --type tsRepository: ecency/vision-next
Length of output: 2661
🏁 Script executed:
# Verify the callback dependency on line 39 to ensure the proposed useEffect won't cause issues
sed -n '26,40p' apps/web/src/app/waves/_context/waves-tag-filter-context.tsx | cat -nRepository: ecency/vision-next
Length of output: 591
State may desync from URL on browser navigation.
useState only uses initialTag on the first render. If the user navigates via browser back/forward buttons, searchParams will update but selectedTag state won't, causing the displayed filter to diverge from the URL.
Consider syncing state when searchParams changes:
🛠️ Proposed fix using useEffect to sync
const searchParams = useSearchParams();
const router = useRouter();
const initialTag = searchParams?.get("tag") ?? null;
const [selectedTag, setSelectedTagState] = useState<string | null>(initialTag);
+
+ // Sync state when URL changes (e.g., browser back/forward)
+ useEffect(() => {
+ const urlTag = searchParams?.get("tag") ?? null;
+ if (urlTag !== selectedTag) {
+ setSelectedTagState(urlTag);
+ }
+ }, [searchParams, selectedTag]);📝 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.
| const searchParams = useSearchParams(); | |
| const router = useRouter(); | |
| const initialTag = searchParams?.get("tag") ?? null; | |
| const [selectedTag, setSelectedTagState] = useState<string | null>(initialTag); | |
| const searchParams = useSearchParams(); | |
| const router = useRouter(); | |
| const initialTag = searchParams?.get("tag") ?? null; | |
| const [selectedTag, setSelectedTagState] = useState<string | null>(initialTag); | |
| // Sync state when URL changes (e.g., browser back/forward) | |
| useEffect(() => { | |
| const urlTag = searchParams?.get("tag") ?? null; | |
| if (urlTag !== selectedTag) { | |
| setSelectedTagState(urlTag); | |
| } | |
| }, [searchParams, selectedTag]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/waves/_context/waves-tag-filter-context.tsx` around lines 21
- 24, The selectedTag state (initialized from initialTag) can desync when the
URL changes because useState only uses the initial value; add a useEffect that
reads searchParams.get("tag") (or initialTag) and calls
setSelectedTagState(newTag) only when it differs from current selectedTag to
keep state in sync with browser navigation; reference the existing symbols
useSearchParams, initialTag, selectedTag, setSelectedTagState and update the
effect dependencies to include searchParams (or initialTag) and selectedTag to
avoid infinite loops.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/features/waves/styles/thread-render.scss (1)
133-134: Note: CSS:has()browser compatibility.The
:has()pseudo-class is well-supported in modern browsers (Chrome 105+, Safari 15.4+, Firefox 121+), but older browsers will fall back to stacking images vertically rather than the grid layout. This is likely acceptable graceful degradation, but worth noting if the project has legacy browser requirements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/waves/styles/thread-render.scss` around lines 133 - 134, Browser support for the p:has(> .markdown-image-container + .markdown-image-container) selector is limited on older browsers, so wrap the grid rule in a feature query using `@supports`(selector(...)) to apply the grid only when :has() is supported and leave the current vertical stacking as the graceful fallback; update the SCSS around the p:has(...) rule to use `@supports`(selector("p:has(> .markdown-image-container + .markdown-image-container)")) { /* grid styles for p:has(...) */ } and add a short inline comment noting the intentional graceful degradation for legacy browsers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/features/waves/styles/thread-render.scss`:
- Around line 115-126: The container rule .markdown-image-container currently
sets img { height: 100% } with overflow: hidden which will clip any sibling
.markdown-img-caption added by imageZoomEnhancer.ts; update the styles so the
caption is not clipped in grid views—either hide captions by adding a selector
for the grid context that sets .markdown-image-container .markdown-img-caption {
display: none; } or change the container layout to a column flex and make img
use max-height: calc(100% - <caption-height>) (or max-height: 100% with flex: 1
on img) so captions remain visible; modify the .markdown-image-container and img
rules and add a rule for .markdown-img-caption accordingly.
---
Nitpick comments:
In `@apps/web/src/features/waves/styles/thread-render.scss`:
- Around line 133-134: Browser support for the p:has(> .markdown-image-container
+ .markdown-image-container) selector is limited on older browsers, so wrap the
grid rule in a feature query using `@supports`(selector(...)) to apply the grid
only when :has() is supported and leave the current vertical stacking as the
graceful fallback; update the SCSS around the p:has(...) rule to use
`@supports`(selector("p:has(> .markdown-image-container +
.markdown-image-container)")) { /* grid styles for p:has(...) */ } and add a
short inline comment noting the intentional graceful degradation for legacy
browsers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 437b8412-880f-401e-96e2-cf3b2dd1a36c
📒 Files selected for processing (2)
apps/web/src/app/waves/_context/waves-tag-filter-context.tsxapps/web/src/features/waves/styles/thread-render.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/waves/_context/waves-tag-filter-context.tsx
1 participant
Summary by CodeRabbit
New Features
Improvements