-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[WEB-4881] feat(utils): add URL display formatter #7770
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: preview
Are you sure you want to change the base?
Conversation
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a new exported helper formatURLForDisplay in packages/utils/src/url.ts to present URLs without protocol, using URL.host when valid and extractHostname as fallback; returns empty string for falsy input. Also expands JSDoc for extractTLD; no functional changes to other utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant URLUtils as url.ts
Caller->>URLUtils: formatURLForDisplay(url)
alt url is falsy
URLUtils-->>Caller: ""
else url is valid URL
URLUtils->>URL: new URL(url)
URLUtils-->>Caller: parsed.host
else url invalid (throws)
URLUtils->>URLUtils: extractHostname(url)
URLUtils-->>Caller: hostname
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
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.
Pull Request Overview
This PR adds a new utility function formatURLForDisplay
that creates a clean, user-friendly representation of URLs by stripping the protocol and trailing slashes.
- Adds
formatURLForDisplay
function that returns the host portion of valid URLs or falls back to hostname extraction for invalid URLs - Uses the existing
extractHostname
helper as a fallback when URL parsing fails - Includes comprehensive JSDoc documentation explaining the function's behavior and purpose
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
* Returns a readable representation of a URL by stripping the protocol | ||
* and any trailing slash. For valid URLs, only the host is returned. | ||
* Invalid URLs are sanitized by removing the protocol and trailing slash. |
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.
The documentation mentions 'stripping trailing slash' but the implementation doesn't actually handle trailing slashes. The new URL(url).host
property and extractHostname
function don't remove trailing slashes from the host portion. Consider updating the documentation to accurately reflect what the function does, or implement trailing slash removal if that's the intended behavior.
Copilot uses AI. Check for mistakes.
* Invalid URLs are sanitized by removing the protocol and trailing slash. | ||
* | ||
* @param url - The URL string to format | ||
* @returns The formatted domain for display |
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.
The return description says 'formatted domain' but the function returns the host (which includes port numbers if present). Consider changing this to 'The formatted host for display' to be more accurate, since new URL(url).host
includes both hostname and port.
* @returns The formatted domain for display | |
* @returns The formatted host for display |
Copilot uses AI. Check for mistakes.
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: 0
🧹 Nitpick comments (1)
packages/utils/src/url.ts (1)
100-116
: Confirm intent: function currently drops path/query; PR description says only strip protocol and trailing slashIf the goal is to keep the path (minus a trailing slash) and just remove the scheme, this implementation is too aggressive (it returns only the host). Suggest updating the function to preserve pathname for display, trim input, and safely handle non-URL strings (including credentials) in the fallback.
-export function formatURLForDisplay(url: string): string { - if (!url) return ""; - - try { - return new URL(url).host; - } catch (_error) { - return extractHostname(url); - } -} +export function formatURLForDisplay(url: string): string { + if (!url) return ""; + const input = url.trim(); + if (!input) return ""; + + try { + const u = new URL(input); + // Keep email address for mailto: links; otherwise show host plus pathname (no trailing slash). + if (u.protocol === MAILTO_PROTOCOL) return u.pathname; + const path = u.pathname === "/" ? "" : u.pathname.replace(/\/+$/, ""); + return u.host + path; + } catch { + // Fallback for strings without a valid URL object: remove protocol, strip credentials, and trailing slash. + let s = input.replace(PROTOCOL_REGEX, ""); + const at = s.indexOf("@"); + if (at !== -1) s = s.substring(at + 1); + return s.replace(/\/+$/, ""); + } +}Quick test cases to confirm expected behavior:
- "https://example.com" → "example.com"
- "https://example.com/" → "example.com"
- "https://example.com/path/" → "example.com/path"
- "example.com/path/" → "example.com/path"
- "https://user:[email protected]/a" → "example.com/a"
- "mailto:[email protected]" → "[email protected]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/utils/src/url.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
Summary
Testing
pnpm --filter=@plane/utils check:lint
pnpm --filter=@plane/utils check:types
(fails: Cannot find module '@plane/types')pnpm --filter=@plane/utils check:format
pnpm --filter=@plane/utils test
(no tests found)https://chatgpt.com/codex/tasks/task_e_68c1e070b158832a8f20c8484496c613
Summary by CodeRabbit