Skip to content

Conversation

@asafashirov
Copy link
Contributor

@asafashirov asafashirov commented Dec 15, 2025

Treat subdomains of pulumi.com as internal links so they don't open in new tabs. The external-links.ts script now checks for subdomain relationships instead of exact domain match, allowing links to app.pulumi.com to be recognized as internal when linking from pulumi.com or vice versa.

This was an unintended side effect of PR #15080 which added the "open external links in new tabs" feature.

Treat subdomains of pulumi.com as internal links so they don't open in new tabs. The external-links.ts script now checks for subdomain relationships instead of exact domain match, allowing app.pulumi.com to be recognized as internal when linking from pulumi.com.
@claude
Copy link
Contributor

claude bot commented Dec 15, 2025

Documentation Review

Summary

This PR fixes the external-links.ts script to treat subdomains of pulumi.com as internal links, preventing them from opening in new tabs. The changes are clean and focused.

Issues Found

1. Logic correctness concern - theme/src/ts/external-links.ts:30-31

The subdomain check logic may not work correctly for all cases. Consider these scenarios:

  • When on pulumi.com (currentDomain = pulumi.com):

    • Link to app.pulumi.com (linkDomain = app.pulumi.com): ✅ Works (app.pulumi.com ends with .pulumi.com)
    • Link to www.pulumi.com (linkDomain = pulumi.com after www removal): ✅ Works (exact match)
  • When on app.pulumi.com (currentDomain = app.pulumi.com):

    • Link to pulumi.com (linkDomain = pulumi.com): ✅ Works (app.pulumi.com ends with .pulumi.com)
    • Link to registry.pulumi.com (linkDomain = registry.pulumi.com): ✅ Works (both end with .pulumi.com)

However, the current logic has a potential edge case issue:

  • When on example.com (currentDomain = example.com):
    • Link to notexample.com (linkDomain = notexample.com): ✅ Correctly external

The third condition (currentDomain.endsWith('.' + linkDomain)) could theoretically match unintended domains if someone creates pulumi.com.evil.com, but this is unlikely in practice since you control the deployment domains.

Suggestion: Consider using a more explicit base domain check:

// Extract base domain (last two parts for most TLDs)
const getBaseDomain = (domain: string): string => {
    const parts = domain.split('.');
    return parts.length >= 2 ? parts.slice(-2).join('.') : domain;
};

const linkBaseDomain = getBaseDomain(linkDomain);
const currentBaseDomain = getBaseDomain(currentDomain);
const isInternal = linkBaseDomain === currentBaseDomain;

This would treat pulumi.com, app.pulumi.com, registry.pulumi.com, and www.pulumi.com all as internal.

2. Code style - theme/src/ts/external-links.ts:29-31

The multi-line boolean expression could be more readable with parentheses or consistent formatting, though the current approach is acceptable.

Positive Notes

  • The fix correctly addresses the issue described in the PR
  • Code changes are minimal and focused
  • The logic handles the bidirectional subdomain relationship well
  • Comment update clearly explains the new behavior

Need additional reviews or fixes? Mention @claude and I'll help out!

@pulumi-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants