feat(pages): add blog section with i18n support by lizhengfeng101 · Pull Request #355 · alibaba/open-code-review

lizhengfeng101 · 2026-07-10T14:48:55Z

Summary

Add a blog feature to the pages site:

BlogPage component — list view with tag filtering and search modal (⌘K), detail view with TOC sidebar and scroll-spy heading highlight
Blog content system — markdown-based posts with frontmatter metadata, i18n support for en/ja/zh, centralized blog registry in pages/src/content/blog/index.ts
Navbar integration — added Blog tab, improved active state detection to use startsWith matching for nested routes
MarkdownRenderer enhancement — custom image renderer to handle absolute paths with correct public path resolution
Webpack config — added copy-webpack-plugin to serve static assets from pages/public/ directory

Includes an initial blog post "Introducing Open Code Review Blog" in all three languages.

Review notes

Code review flagged a few items worth addressing in follow-up:

publicPath calculation in MarkdownRenderer may produce incorrect paths under multi-level routes
searchBlog slug source inconsistency with getAllBlogMetas
Deprecated navigator.platform usage
Heavy inline styles that could be moved to CSS

Add a blog feature to the pages site including: - BlogPage component with list/detail views, tag filtering, search, and TOC - Blog content system with markdown posts and i18n (en/ja/zh) - Navbar integration with blog tab and improved active state detection - MarkdownRenderer image path handling for relative/absolute paths - Webpack CopyPlugin for serving static blog assets

css521

lgtm

github-actions · 2026-07-10T14:56:03Z

🔍 OpenCodeReview found 10 issue(s) in this PR.

✅ Successfully posted inline: 10 comment(s)

github-actions · 2026-07-10T14:56:03Z

    const renderer = new Renderer();
+    renderer.image = function ({ href, title, text }: { href: string; title?: string | null; text: string }) {
+      const escapeAttr = (s: string) => s.replace(/&/g, '&amp;').replace(/"/g, '&quot;').replace(/</g, '&lt;').replace(/>/g, '&gt;');
+      const src = href.startsWith('/') ? `${publicPath}${href}` : href;


Potential null pointer exception: In the marked library, the href parameter passed to the image renderer can be null or undefined at runtime (e.g., for malformed markdown like ![alt]()). Although the TypeScript type annotation declares it as string, this does not enforce runtime safety. Calling href.startsWith('/') when href is null/undefined will throw a TypeError.

Consider adding a null guard before accessing href.

Suggestion:

Suggested change

const src = href.startsWith('/') ? `${publicPath}${href}` : href;

const safeHref = href ?? '';

const src = safeHref.startsWith('/') ? `${publicPath}${safeHref}` : safeHref;

github-actions · 2026-07-10T14:56:04Z

          <div style={{ display: 'flex', alignItems: 'center', gap: 4 }}>
            {navTabs.map((tab) => {
-              const isActive = currentPath === tab.path;
+              const isActive = tab.path === '/' ? currentPath === '/' : currentPath.startsWith(tab.path);


The startsWith check is fragile and could cause incorrect active state for tabs with overlapping prefixes. For example, if a future route like /docs-api or /benchmark-results is added, it would incorrectly highlight the /docs or /benchmark tab respectively.

A safer approach is to check that the current path either matches exactly or starts with the tab path followed by /:

Suggestion:

Suggested change

const isActive = tab.path === '/' ? currentPath === '/' : currentPath.startsWith(tab.path);

const isActive = currentPath === tab.path || currentPath.startsWith(tab.path + '/');

github-actions · 2026-07-10T14:56:04Z

+.blog-card:hover {
+  background: rgba(255, 255, 255, 0.05);
+  border-color: rgba(255, 255, 255, 0.15);
+}


The .blog-card has cursor: pointer indicating it's clickable, but no :focus or :focus-visible style is defined. In BlogPage.tsx, .blog-tag is rendered as a <button> element which is keyboard-focusable, but there's no visible focus indicator for it either. Consider adding focus styles for both interactive elements to support keyboard navigation.

For example:

.blog-tag:focus-visible { outline: 2px solid #2BDE5E; outline-offset: 2px; }

Note: .blog-card in BlogPage.tsx is a plain <div> with onClick — it won't receive keyboard focus at all. That component should use a <button> or add role="button" + tabIndex={0} + keyboard handlers.

github-actions · 2026-07-10T14:56:04Z

+export function getAllBlogMetas(language: string): BlogMeta[] {
+  const slugs = Object.keys(blogMap.en) as BlogSlug[];
+  return slugs
+    .map(slug => getBlogMeta(slug, language))
+    .sort((a, b) => b.date.localeCompare(a.date));
+}


Inconsistent slug sources across functions: getAllBlogMetas always derives slugs from blogMap.en, while searchBlog derives slugs from blogMap[language]. This means:

If a blog post exists only in a non-English language (e.g., zh), it will never appear in list views (getAllBlogMetas) but could appear in search results (searchBlog).

Conversely, if a post exists in English but not in another language, getAllBlogMetas will list it for all languages, silently falling back to English content via getRawContent.

Consider deriving slugs consistently — either always from the requested language's map, or by taking the union of all language maps — and documenting the fallback behavior.

github-actions · 2026-07-10T14:56:04Z

+function stripFrontmatter(md: string): string {
+  if (md.startsWith('---')) {
+    const end = md.indexOf('---', 3);
+    if (end !== -1) {
+      return md.slice(end + 3).trim();
+    }
+  }
+  return md;
+}


Duplicated frontmatter boundary detection: Both stripFrontmatter and parseFrontmatter independently locate the frontmatter delimiters using the same indexOf('---', 3) logic. Consider extracting a shared helper (e.g., extractFrontmatter(raw): { frontmatter: string; body: string } | null) to avoid duplication and ensure both functions stay consistent if the parsing logic changes.

github-actions · 2026-07-10T14:56:04Z

+  const isDetailView = !!(slugParam && validSlugs.has(slugParam as BlogSlug));
+  const activeSlug = isDetailView ? (slugParam as BlogSlug) : null;


Bug: No handling for invalid/missing blog slugs. When a user navigates to /blog/some-invalid-slug, isDetailView will be false (since the slug isn't in validSlugs), and the component silently falls through to the list view without any indication that the requested post was not found. This is confusing UX and masks routing issues.

Consider adding a 404/not-found state when slugParam is present but doesn't match a valid slug, or redirecting to /blog with an error message.

github-actions · 2026-07-10T14:56:04Z

+      const id = generateHeadingId(text);
+      headings.push({ id, text, level });


Bug: Duplicate heading IDs possible. The generateHeadingId function does not handle duplicate heading texts. If a blog post has two headings with the same text (e.g., two ## Overview sections), both will get the same ID. This causes:

The IntersectionObserver to track the wrong element.

TOC click-to-scroll to jump to the first matching element instead of the intended one.

Invalid HTML (duplicate id attributes).

Consider appending a counter suffix for duplicates (e.g., overview-1, overview-2). Note that both extractHeadings here and the MarkdownRenderer heading renderer must use the same deduplication strategy to keep IDs consistent.

github-actions · 2026-07-10T14:56:04Z

+            >
+              <img src={searchIcon} alt="" style={{ width: 16, height: 16, opacity: 0.6 }} />
+              <span>{t('blog.search.placeholder')}</span>
+              <span style={{ fontSize: 13, color: 'rgba(255,255,255,0.3)', fontFamily: '-apple-system, BlinkMacSystemFont, sans-serif', lineHeight: 1, marginLeft: 8 }}>{navigator.platform?.includes('Mac') ? '⌘K' : 'Ctrl+K'}</span>


Deprecated API: navigator.platform is deprecated. This property is unreliable on newer browsers/platforms (e.g., iPadOS reports as MacIntel, ChromeOS may return unexpected values). Consider using navigator.userAgentData?.platform (with fallback) or a simple userAgent check for more reliable platform detection.

github-actions · 2026-07-10T14:56:04Z

+            <button
+              onClick={() => navigate('/blog')}
+              style={{
+                background: 'none',
+                border: 'none',
+                cursor: 'pointer',
+                padding: 0,
+                marginBottom: 24,
+                fontSize: 14,
+                color: 'rgba(255,255,255,0.5)',
+                fontFamily,
+                transition: 'color 0.2s',
+              }}


Inline styles overuse: Many of these inline styles are static (e.g., the back button, title, metadata container, prev/next buttons, TOC container). They should be extracted to CSS classes in blog.css for better maintainability, reusability, and consistency with the existing pattern used for .blog-card, .blog-tag, etc. Only truly dynamic styles (like conditional padding based on isMobile or active/hover color states) should remain inline.

github-actions · 2026-07-10T14:56:04Z

+    const handleKeyDown = (e: KeyboardEvent) => {
+      if ((e.metaKey || e.ctrlKey) && e.key === 'k') {
+        const activeEl = document.activeElement;
+        if (activeEl && (activeEl.tagName === 'INPUT' || activeEl.tagName === 'TEXTAREA') && !searchOpen) return;


Potential issue: Cmd+K toggle inconsistency. The guard condition !searchOpen in the INPUT/TEXTAREA check means that when the search modal IS open and focus is on the search input, pressing Cmd+K won't toggle it closed (because the early return fires). Users who expect Cmd+K to act as a toggle will need to use Escape instead. Consider removing the !searchOpen condition so Cmd+K consistently toggles the modal regardless of focus state.

lizhengfeng101 requested review from MuoDoo, css521, wbgbg and xyJen July 10, 2026 14:50

css521 approved these changes Jul 10, 2026

View reviewed changes

lizhengfeng101 merged commit 4cc2fe8 into main Jul 10, 2026
7 checks passed

lizhengfeng101 deleted the feat/pages-blog branch July 10, 2026 14:55

github-actions Bot reviewed Jul 10, 2026

View reviewed changes

	const src = href.startsWith('/') ? `${publicPath}${href}` : href;
	const safeHref = href ?? '';
	const src = safeHref.startsWith('/') ? `${publicPath}${safeHref}` : safeHref;

	const isActive = tab.path === '/' ? currentPath === '/' : currentPath.startsWith(tab.path);
	const isActive = currentPath === tab.path \|\| currentPath.startsWith(tab.path + '/');

		const isDetailView = !!(slugParam && validSlugs.has(slugParam as BlogSlug));
		const activeSlug = isDetailView ? (slugParam as BlogSlug) : null;

		const id = generateHeadingId(text);
		headings.push({ id, text, level });

Uh oh!

Conversation

lizhengfeng101 commented Jul 10, 2026

Summary

Review notes

Uh oh!

css521 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jul 10, 2026 •

edited

Loading