-
Notifications
You must be signed in to change notification settings - Fork 5
Fix internal page links rendering with red boxes (CORE-733) #2876
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
Open
OpenStaxClaude
wants to merge
3
commits into
main
Choose a base branch
from
fix/CORE-733-resolve-internal-page-links
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import cmsFetch from '~/helpers/cms-fetch'; | ||
|
|
||
| // Cache for resolved page URLs to avoid repeated API calls | ||
| const urlCache = new Map<string, string>(); | ||
|
|
||
| interface PageApiResponse { | ||
| html_url?: string; | ||
| meta?: { | ||
| html_url?: string; | ||
| }; | ||
| } | ||
|
|
||
| async function getResolvedUrl(pageId: string): Promise<string | undefined> { | ||
| const cachedUrl = urlCache.get(pageId); | ||
|
|
||
| if (cachedUrl) { | ||
| return cachedUrl; | ||
| } | ||
|
|
||
| const response = (await cmsFetch(`pages/${pageId}/`)) as PageApiResponse; | ||
| const resolvedUrl = response.html_url ?? response.meta?.html_url; | ||
|
|
||
| if (!resolvedUrl) { | ||
| console.warn(`Page ${pageId} has no html_url in API response`); | ||
| return undefined; | ||
| } | ||
|
|
||
| urlCache.set(pageId, resolvedUrl); | ||
| return resolvedUrl; | ||
| } | ||
|
|
||
| async function resolvePageLink(link: HTMLAnchorElement): Promise<void> { | ||
| const pageId = link.getAttribute('id'); | ||
|
|
||
| if (!pageId) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const resolvedUrl = await getResolvedUrl(pageId); | ||
|
|
||
| if (!resolvedUrl || link.getAttribute('href')) { | ||
| return; | ||
| } | ||
|
|
||
| link.setAttribute('href', resolvedUrl); | ||
| } catch (err) { | ||
| // Log error but don't break the page | ||
| console.error(`Failed to resolve page link for id ${pageId}:`, err); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Resolves internal page links that have linktype="page" and id attributes | ||
| * but are missing href attributes. Fetches the page metadata from CMS API | ||
| * and populates the href with the resolved URL. | ||
| * | ||
| * @param element - The container element to search for page links | ||
| */ | ||
| export default async function resolvePageLinks(element: HTMLElement | null | undefined): Promise<void> { | ||
| if (!element) { | ||
| return; | ||
| } | ||
|
|
||
| // Find all anchor tags with linktype="page" and an id attribute | ||
| const pageLinks = element.querySelectorAll<HTMLAnchorElement>('a[linktype="page"][id]'); | ||
|
|
||
| if (pageLinks.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| // Process all links in parallel | ||
| const promises = Array.from(pageLinks).map((link) => resolvePageLink(link)); | ||
|
|
||
| await Promise.all(promises); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| import React from 'react'; | ||
| import {render, waitFor} from '@testing-library/preact'; | ||
| import {describe, it, expect, jest, beforeEach} from '@jest/globals'; | ||
| import RawHTML from '~/components/jsx-helpers/raw-html'; | ||
| import * as cmsFetchModule from '~/helpers/cms-fetch'; | ||
| import {PortalContextProvider} from '~/contexts/portal'; | ||
| import MemoryRouter from '~/../../test/helpers/future-memory-router'; | ||
|
|
||
| // Mock the cmsFetch module | ||
| jest.mock('~/helpers/cms-fetch'); | ||
|
|
||
| const mockCmsFetch = cmsFetchModule.default as jest.MockedFunction<typeof cmsFetchModule.default>; | ||
|
|
||
| function Component({html}: {html: string}) { | ||
| return ( | ||
| <MemoryRouter initialEntries={['/']}> | ||
| <PortalContextProvider> | ||
| <RawHTML html={html} /> | ||
| </PortalContextProvider> | ||
| </MemoryRouter> | ||
| ); | ||
| } | ||
|
|
||
| describe('RawHTML with page links', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('renders normal HTML without page links', () => { | ||
| const html = '<p>Normal content with <a href="/test">regular link</a></p>'; | ||
| const {container} = render(<Component html={html} />); | ||
|
|
||
| expect(container.querySelector('a')?.getAttribute('href')).toBe('/test'); | ||
| expect(mockCmsFetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('resolves internal page links', async () => { | ||
| const html = '<p>Check out <a linktype="page" id="560">this page</a> for more info</p>'; | ||
|
|
||
| mockCmsFetch.mockResolvedValue({ | ||
| meta: { | ||
| html_url: 'https://openstax.org/resolved-page' | ||
| } | ||
| }); | ||
|
mwvolo marked this conversation as resolved.
|
||
|
|
||
| const {container} = render(<Component html={html} />); | ||
|
|
||
| // Wait for the async resolution to complete | ||
| await waitFor(() => { | ||
| const link = container.querySelector('a[linktype="page"]') as HTMLAnchorElement; | ||
|
|
||
| expect(link.getAttribute('href')).toBe('https://openstax.org/resolved-page'); | ||
| }); | ||
|
|
||
| expect(mockCmsFetch).toHaveBeenCalledWith('pages/560/'); | ||
| }); | ||
|
|
||
| it('resolves multiple page links', async () => { | ||
| const html = ` | ||
| <div> | ||
| <p><a linktype="page" id="100">First</a></p> | ||
| <p><a linktype="page" id="200">Second</a></p> | ||
| </div> | ||
| `; | ||
|
|
||
| mockCmsFetch | ||
| .mockResolvedValueOnce({meta: {html_url: 'https://openstax.org/page-100'}}) | ||
| .mockResolvedValueOnce({meta: {html_url: 'https://openstax.org/page-200'}}); | ||
|
|
||
| const {container} = render(<Component html={html} />); | ||
|
|
||
| await waitFor(() => { | ||
| const links = container.querySelectorAll('a[linktype="page"]'); | ||
|
|
||
| expect((links[0] as HTMLAnchorElement).getAttribute('href')).toBe('https://openstax.org/page-100'); | ||
| expect((links[1] as HTMLAnchorElement).getAttribute('href')).toBe('https://openstax.org/page-200'); | ||
| }); | ||
|
|
||
| expect(mockCmsFetch).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it('handles API errors gracefully', async () => { | ||
| const html = '<p><a linktype="page" id="999">broken link</a></p>'; | ||
|
|
||
| const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
|
||
| mockCmsFetch.mockRejectedValue(new Error('API error')); | ||
|
|
||
| const {container} = render(<Component html={html} />); | ||
|
|
||
| // Wait a bit for async operations to attempt | ||
| await waitFor(() => { | ||
| expect(consoleErrorSpy).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // Link should not have href attribute | ||
| const link = container.querySelector('a[linktype="page"]') as HTMLAnchorElement; | ||
|
|
||
| expect(link.hasAttribute('href')).toBe(false); | ||
|
|
||
| consoleErrorSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('re-resolves links when html prop changes', async () => { | ||
| const html1 = '<p><a linktype="page" id="990">First page</a></p>'; | ||
| const html2 = '<p><a linktype="page" id="991">Second page</a></p>'; | ||
|
|
||
| mockCmsFetch | ||
| .mockResolvedValueOnce({meta: {html_url: 'https://openstax.org/page-990'}}) | ||
| .mockResolvedValueOnce({meta: {html_url: 'https://openstax.org/page-991'}}); | ||
|
|
||
| const {container, rerender} = render(<Component html={html1} />); | ||
|
|
||
| await waitFor(() => { | ||
| const link = container.querySelector('a[linktype="page"]') as HTMLAnchorElement; | ||
|
|
||
| expect(link.getAttribute('href')).toBe('https://openstax.org/page-990'); | ||
| }); | ||
|
|
||
| // Update the HTML prop | ||
| rerender(<Component html={html2} />); | ||
|
|
||
| await waitFor(() => { | ||
| const link = container.querySelector('a[linktype="page"]') as HTMLAnchorElement; | ||
|
|
||
| expect(link.getAttribute('href')).toBe('https://openstax.org/page-991'); | ||
| }); | ||
|
|
||
| expect(mockCmsFetch).toHaveBeenCalledWith('pages/990/'); | ||
| expect(mockCmsFetch).toHaveBeenCalledWith('pages/991/'); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| import {describe, it, expect, jest, beforeEach} from '@jest/globals'; | ||
| import resolvePageLinks from '~/helpers/resolve-page-links'; | ||
| import * as cmsFetchModule from '~/helpers/cms-fetch'; | ||
|
|
||
| // Mock the cmsFetch module | ||
| jest.mock('~/helpers/cms-fetch'); | ||
|
|
||
| const mockCmsFetch = cmsFetchModule.default as jest.MockedFunction<typeof cmsFetchModule.default>; | ||
|
|
||
| describe('resolvePageLinks', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
mwvolo marked this conversation as resolved.
|
||
|
|
||
| it('returns early if element is null', async () => { | ||
| await resolvePageLinks(null); | ||
| expect(mockCmsFetch).not.toHaveBeenCalled(); | ||
|
mwvolo marked this conversation as resolved.
|
||
| }); | ||
| it('does nothing if there are no page links', async () => { | ||
| const element = document.createElement('div'); | ||
|
|
||
| element.innerHTML = '<p>Some text <a href="/normal-link">normal link</a></p>'; | ||
|
|
||
| await resolvePageLinks(element); | ||
|
|
||
| expect(mockCmsFetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('resolves a single page link', async () => { | ||
| const element = document.createElement('div'); | ||
|
|
||
| element.innerHTML = '<p>Check out <a linktype="page" id="560">this page</a> for more info</p>'; | ||
|
|
||
| mockCmsFetch.mockResolvedValue({ | ||
| meta: { | ||
| html_url: 'https://openstax.org/some-page' | ||
| } | ||
| }); | ||
|
mwvolo marked this conversation as resolved.
|
||
|
|
||
| await resolvePageLinks(element); | ||
|
|
||
| const link = element.querySelector('a[linktype="page"]') as HTMLAnchorElement; | ||
|
|
||
| expect(mockCmsFetch).toHaveBeenCalledWith('pages/560/'); | ||
| expect(link.getAttribute('href')).toBe('https://openstax.org/some-page'); | ||
| }); | ||
|
|
||
| it('resolves multiple page links', async () => { | ||
| const element = document.createElement('div'); | ||
|
|
||
| element.innerHTML = ` | ||
| <p>Check out <a linktype="page" id="660">this page</a> and <a linktype="page" id="661">that page</a></p> | ||
| `; | ||
|
|
||
| mockCmsFetch | ||
| .mockResolvedValueOnce({meta: {html_url: 'https://openstax.org/page-660'}}) | ||
| .mockResolvedValueOnce({meta: {html_url: 'https://openstax.org/page-661'}}); | ||
|
|
||
| await resolvePageLinks(element); | ||
|
|
||
| const links = element.querySelectorAll('a[linktype="page"]'); | ||
|
|
||
| expect(mockCmsFetch).toHaveBeenCalledWith('pages/660/'); | ||
| expect(mockCmsFetch).toHaveBeenCalledWith('pages/661/'); | ||
| expect((links[0] as HTMLAnchorElement).getAttribute('href')).toBe('https://openstax.org/page-660'); | ||
| expect((links[1] as HTMLAnchorElement).getAttribute('href')).toBe('https://openstax.org/page-661'); | ||
| }); | ||
|
|
||
| it('uses cache for repeated page IDs', async () => { | ||
| const element = document.createElement('div'); | ||
|
|
||
| element.innerHTML = ` | ||
| <p><a linktype="page" id="770">first link</a></p> | ||
| `; | ||
|
|
||
| mockCmsFetch.mockResolvedValue({ | ||
| meta: { | ||
| html_url: 'https://openstax.org/cached-page' | ||
| } | ||
| }); | ||
|
mwvolo marked this conversation as resolved.
|
||
|
|
||
| // First call | ||
| await resolvePageLinks(element); | ||
|
|
||
| const firstCallCount = mockCmsFetch.mock.calls.length; | ||
|
|
||
| expect(firstCallCount).toBeGreaterThanOrEqual(1); | ||
| expect((element.querySelector('a') as HTMLAnchorElement).getAttribute('href')) | ||
| .toBe('https://openstax.org/cached-page'); | ||
|
|
||
| // Second call with same page ID | ||
| const element2 = document.createElement('div'); | ||
|
|
||
| element2.innerHTML = '<p><a linktype="page" id="770">second link</a></p>'; | ||
|
|
||
| await resolvePageLinks(element2); | ||
|
|
||
| // Should still have the same call count (using cache) | ||
| expect(mockCmsFetch.mock.calls.length).toBe(firstCallCount); | ||
| expect((element2.querySelector('a') as HTMLAnchorElement).getAttribute('href')) | ||
| .toBe('https://openstax.org/cached-page'); | ||
| }); | ||
|
|
||
| it('handles API errors gracefully', async () => { | ||
| const element = document.createElement('div'); | ||
|
|
||
| element.innerHTML = '<p><a linktype="page" id="880">broken link</a></p>'; | ||
|
|
||
| const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
|
||
| mockCmsFetch.mockRejectedValueOnce(new Error('API error')); | ||
|
|
||
| await resolvePageLinks(element); | ||
|
|
||
| const link = element.querySelector('a[linktype="page"]') as HTMLAnchorElement; | ||
|
|
||
| // Link should not have href attribute set (or it might be empty) | ||
| const href = link.getAttribute('href'); | ||
|
|
||
| expect(href === null || href === '').toBe(true); | ||
| expect(consoleErrorSpy).toHaveBeenCalledWith( | ||
| 'Failed to resolve page link for id 880:', | ||
| expect.any(Error) | ||
| ); | ||
|
|
||
| consoleErrorSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('handles missing html_url in API response', async () => { | ||
| const element = document.createElement('div'); | ||
|
|
||
| element.innerHTML = '<p><a linktype="page" id="888">incomplete data</a></p>'; | ||
|
|
||
| const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); | ||
|
|
||
| mockCmsFetch.mockResolvedValue({ | ||
| meta: { | ||
| // Missing html_url | ||
| } | ||
| }); | ||
|
mwvolo marked this conversation as resolved.
|
||
|
|
||
| await resolvePageLinks(element); | ||
|
|
||
| const link = element.querySelector('a[linktype="page"]') as HTMLAnchorElement; | ||
|
|
||
| // Link should not have href set | ||
| expect(link.hasAttribute('href')).toBe(false); | ||
| expect(consoleWarnSpy).toHaveBeenCalledWith('Page 888 has no html_url in API response'); | ||
|
|
||
| consoleWarnSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('ignores links without id attribute', async () => { | ||
| const element = document.createElement('div'); | ||
|
|
||
| element.innerHTML = '<p><a linktype="page">no id link</a></p>'; | ||
|
|
||
| await resolvePageLinks(element); | ||
|
|
||
| expect(mockCmsFetch).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.