-
Notifications
You must be signed in to change notification settings - Fork 98
[search/browse] link repo name to file browser; link code image to external #340
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
[search/browse] link repo name to file browser; link code image to external #340
Conversation
WalkthroughThe update refactors the repository icon and name rendering logic within the path header component. The icon is now conditionally wrapped in an anchor tag if a repository link exists. The repository name is always styled as clickable and navigates to the repository root path on click, removing previous conditional link logic. Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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/web/src/lib/utils.ts (1)
178-179
: Consider consolidating redundant properties.The implementation correctly propagates
webUrl
across all code host types. However, bothrepoLink
andwebUrl
are now set to the same value, which creates redundancy.Since the component now uses
webUrl
for the icon link and constructs its own path for the repository name link, consider removing therepoLink
property in a future refactor to simplify the interface:- repoLink: webUrl,
Also applies to: 190-191, 202-203, 215-216, 227-228, 239-240, 251-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web/src/app/[domain]/components/pathHeader.tsx
(3 hunks)packages/web/src/lib/utils.ts
(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
packages/web/src/lib/utils.ts (1)
157-157
: LGTM: Well-designed type extension.The addition of the optional
webUrl
property maintains backward compatibility while enabling the new linking behavior described in the PR objectives.packages/web/src/app/[domain]/components/pathHeader.tsx (4)
20-20
: LGTM: Proper domain context integration.The addition of the
useDomain
hook enables dynamic path construction for the internal file browser navigation.Also applies to: 64-64
228-242
: Excellent implementation of conditional icon linking.The conditional rendering properly implements the PR objective of linking the repository icon to the external repository when available, with appropriate security attributes for external links.
248-248
: Verify the styling change is intentional.The condition changed from a conditional expression to always
true
. This might be intentional since repository names should always be clickable now, but please confirm this change is deliberate.
250-250
: Verify branch context handling in path construction.The hard-coded
HEAD/-/blob/
pattern might not preserve the user's current branch context. If a user is browsing a specific branch, clicking the repository name should ideally maintain that branch context.Consider whether the path should use the current branch instead of always defaulting to
HEAD
:- href={`/${domain}/browse/${repo.name}@HEAD/-/blob/`} + href={`/${domain}/browse/${repo.name}@${branchDisplayName || 'HEAD'}/-/blob/`}Also verify if the path type should always be
blob
or if it should betree
when browsing directories.
Also if you could add a changelog entry 🙏 |
5e29bf4
to
d6f83e1
Compare
Added! I think all good now if you want to give it a local test. |
d6f83e1
to
13f03c0
Compare
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.
LGTM 👍
Summary by CodeRabbit