-
Notifications
You must be signed in to change notification settings - Fork 144
fix(web): Fix text direction on filter panel #474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a child-level LTR direction override via CSS for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
@brendan-kellam your pull request is missing a changelog! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/app/[domain]/search/components/filterPanel/entry.tsx (1)
55-55
: Correct fix: child-level LTR wrapper enables proper rendering for C++/C# under left-truncationWrapping the text in a child element is the right approach to pair with
.truncate-start > * { direction: ltr; ... }
and fixes the reversed-language issue without breaking left-side ellipsis.You can drop redundant classes because
truncate-start
already applieswhite-space: nowrap
,overflow: hidden
, andtext-overflow: ellipsis
.-<p className="overflow-hidden text-ellipsis whitespace-nowrap truncate-start"><span>{displayName}</span></p> +<p className="truncate-start"><span>{displayName}</span></p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/web/src/app/[domain]/search/components/codePreviewPanel/codePreview.tsx
(1 hunks)packages/web/src/app/[domain]/search/components/filterPanel/entry.tsx
(1 hunks)packages/web/src/app/globals.css
(1 hunks)packages/web/src/features/chat/components/chatThread/tools/shared.tsx
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit Inference Engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
packages/web/src/app/[domain]/search/components/filterPanel/entry.tsx
packages/web/src/app/[domain]/search/components/codePreviewPanel/codePreview.tsx
packages/web/src/features/chat/components/chatThread/tools/shared.tsx
packages/web/src/app/globals.css
🔇 Additional comments (2)
packages/web/src/features/chat/components/chatThread/tools/shared.tsx (1)
35-35
: LGTM: span wrapper aligns with the child-direction overrideThis pairs correctly with the new CSS rule to keep left-ellipsis behavior while rendering the path LTR inside the truncated container.
packages/web/src/app/[domain]/search/components/codePreviewPanel/codePreview.tsx (1)
224-224
: LGTM: child span ensures correct LTR rendering inside left-truncation containerKeeps left-side ellipsis behavior intact and fixes symbol ordering (e.g., ++, #) in the header path. Retaining the title attribute for hover is a nice touch.
.truncate-start > * { | ||
direction: ltr; | ||
unicode-bidi: embed; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Direction override looks good; consider unicode-bidi: isolate
and verify class usage across the app
The child-level override is the right lever to preserve left-ellipsis (outer RTL) while fixing symbol order (inner LTR). Two follow-ups to consider:
- Using
unicode-bidi: isolate
instead ofembed
typically reduces bidi interactions with neighboring content. It’s a safe tweak for paths/language labels. - Since this affects all
.truncate-start
usages globally, verify that every instance actually wraps text in a single child element (as done in this PR), otherwise the override won’t apply.
If you’d like to try the isolate
variant:
-.truncate-start > * {
- direction: ltr;
- unicode-bidi: embed;
-}
+.truncate-start > * {
+ direction: ltr;
+ unicode-bidi: isolate;
+}
To audit .truncate-start
usage and quickly spot places that may still render text directly (without a child element), run:
🏁 Script executed:
#!/bin/bash
# List all occurrences of `truncate-start` with nearby context
rg -n -C3 --type tsx --type ts --type jsx --type js 'truncate-start' | sed 's/^\(.*\)$/\1\n---/'
echo
echo "Heuristic: show lines where `truncate-start` appears and the next non-empty line doesn't open a child tag (may indicate direct text). Review manually."
rg -nPU --type tsx --type jsx '(?s)className="[^"]*truncate-start[^"]*".*?\n\s*([^<\s])' -C0
Length of output: 660
🏁 Script executed:
#!/bin/bash
# Retry searching for `truncate-start` using glob-based file matching
echo "Occurrences of 'truncate-start' with context:"
rg -n -C3 -g '*.{tsx,ts,jsx,js}' 'truncate-start' | sed 's/^\(.*\)$/\1\n---/'
echo
echo "Heuristic check: show each `.truncate-start` usage and two lines after the opening tag to spot direct text vs. child element:"
rg -n -A2 -g '*.{tsx,jsx}' 'className\s*=\s*"[^"]*truncate-start[^"]*"'
Length of output: 4869
Wrap direct .truncate-start
elements to ensure the override applies
Two instances apply the truncate-start
class directly to the text-bearing element, so the child‐selector override (.truncate-start > *
) won’t take effect. Please wrap their contents in a single child node:
• packages/web/src/app/[domain]/search/components/codePreviewPanel/codePreview.tsx (≈ line 213)
- <span className="block truncate-start text-sm font-mono cursor-pointer hover:underline" onClick={…}>
- {filePath}
- </span>
+ <span className="block truncate-start text-sm font-mono cursor-pointer hover:underline" onClick={…}>
+ <span>{filePath}</span>
+ </span>
• packages/web/src/features/chat/components/chatThread/tools/shared.tsx (≈ line 26)
- <Link className="text-sm truncate-start" href={…}>
- {path}
- </Link>
+ <Link className="text-sm truncate-start" href={…}>
+ <span>{path}</span>
+ </Link>
The existing entry.tsx filter panel already wraps its text correctly.
Optional: to further isolate bidi behavior, you can switch from embed
to isolate
in globals.css:
.truncate-start > * {
direction: ltr;
- unicode-bidi: embed;
+ unicode-bidi: isolate;
}
🤖 Prompt for AI Agents
In packages/web/src/app/globals.css around lines 314 to 318, the child-selector
override `.truncate-start > * { direction: ltr; unicode-bidi: embed; }` won’t
apply when `.truncate-start` is applied directly to a text-bearing element; wrap
the direct `.truncate-start` usages in a single child node so the selector
targets that child. Concretely: update
packages/web/src/app/[domain]/search/components/codePreviewPanel/codePreview.tsx
(≈ line 213) and
packages/web/src/features/chat/components/chatThread/tools/shared.tsx (≈ line
26) to wrap the existing text content with one element (e.g., a span/div) inside
the element that has `.truncate-start`. Optionally, in globals.css you can
change `unicode-bidi: embed` to `unicode-bidi: isolate` if you want stronger
bidi isolation.
Fixes #463
Summary by CodeRabbit