Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the microCMS API client to use a unified endpoint pattern and adds new functionality for LLM.txt generation and draft preview improvements. The changes consolidate duplicate code across the 'general' and 'medical-articles' endpoints into a single set of functions that can handle both endpoints or merge results from both.
Changes:
- Refactored microCMS API functions to accept optional endpoint parameters, merging 'general' and 'medical-articles' results when no endpoint is specified
- Updated all page components to use the new unified API functions and simplified sidebar data fetching with
getSidebarData() - Added
/llm.txtroute for LLM-based content indexing and enhanced draft preview with required endpoint parameter - Updated robots.txt to disallow /draft/ and changed sitemap domain from med-dent-hub.vercel.app to www.ishatohaisha.com
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/microCMS/microcms.ts | Core refactoring: unified API functions (getArticles, getArticleById, etc.) that handle single or merged endpoints, new getSidebarData helper |
| src/components/ArticleSidebar.tsx | Simplified props to accept single SidebarData object instead of individual arrays |
| src/app/page.tsx | Updated to use new getFeaturedArticles signature with endpoint parameter |
| src/app/medical-articles/page.tsx | Updated to use unified getArticles with endpoint: 'medical-articles' |
| src/app/medical-articles/[id]/page.tsx | Updated to use getArticleById and getSidebarData, removed redundant data fetching |
| src/app/general/page.tsx | Updated to use unified getArticles with endpoint: 'general' |
| src/app/general/[id]/page.tsx | Updated to use getArticleById and getSidebarData, removed redundant data fetching |
| src/app/draft/[id]/page.tsx | Enhanced to require endpoint parameter in URL for proper draft preview |
| src/app/draft/layout.tsx | New layout with noindex meta for draft pages |
| src/app/llm.txt/route.ts | New API route generating markdown content for LLM indexing |
| public/robots.txt | Added /draft/ disallow rule and updated sitemap domain |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client | ||
| .get<ArticleResponse>({ | ||
| endpoint: 'general', | ||
| queries: { ids: ids.join(','), limit: ids.length }, | ||
| }) | ||
| .catch(() => ({ contents: [] as Article[] })), | ||
| client | ||
| .get<ArticleResponse>({ | ||
| endpoint: 'medical-articles', | ||
| queries: { ids: ids.join(','), limit: ids.length }, | ||
| }) | ||
| .catch(() => ({ contents: [] as Article[] })), |
There was a problem hiding this comment.
The error handling in getArticlesByIds silently catches all errors and returns an empty contents array. This can hide real issues like network failures or authentication problems. Consider logging errors or returning a more specific error state so that calling code can differentiate between "no articles found" and "request failed".
src/app/draft/[id]/page.tsx
Outdated
|
|
||
| export const dynamic = 'force-dynamic' | ||
|
|
||
| const VALID_ENDPOINTS: Endpoint[] = ['general', 'medical-articles'] |
There was a problem hiding this comment.
The VALID_ENDPOINTS constant is defined inside the component but should be defined outside or use the Endpoint type directly. Since Endpoint is already a union type ('general' | 'medical-articles'), the validation could be simplified or the constant should be moved to a module-level scope for better maintainability.
src/app/llm.txt/route.ts
Outdated
| ${categories.contents.map((c) => `- [${c.name}](/categories/${c.slug}): ${c.description || ''}`).join('\n')} | ||
|
|
||
| ## タグ | ||
| ${tags.contents.map((t) => `- [${t.name}](/tags/${t.slug})`).join('\n')} |
There was a problem hiding this comment.
The code assumes categories and tags have 'slug' and 'description' properties that don't exist in the type definitions. Based on the types in microcms.ts, Category and Tag only have id, name, createdAt, and updatedAt. This will cause runtime errors when accessing c.slug, c.description, and t.slug.
| ${categories.contents.map((c) => `- [${c.name}](/categories/${c.slug}): ${c.description || ''}`).join('\n')} | |
| ## タグ | |
| ${tags.contents.map((t) => `- [${t.name}](/tags/${t.slug})`).join('\n')} | |
| ${categories.contents | |
| .map((c) => { | |
| const categorySlug = (c as any).slug ?? c.id | |
| const categoryDescription = (c as any).description ?? '' | |
| return `- [${c.name}](/categories/${categorySlug}): ${categoryDescription}` | |
| }) | |
| .join('\n')} | |
| ## タグ | |
| ${tags.contents | |
| .map((t) => { | |
| const tagSlug = (t as any).slug ?? t.id | |
| return `- [${t.name}](/tags/${tagSlug})` | |
| }) | |
| .join('\n')} |
| return map | ||
| return { | ||
| contents: merged, | ||
| totalCount: generalRes.totalCount + medicalRes.totalCount, |
There was a problem hiding this comment.
When merging articles from both endpoints, the totalCount is calculated as the sum of both endpoints (generalRes.totalCount + medicalRes.totalCount), but the returned contents array is limited to rest.limit items. This creates an inconsistency where totalCount doesn't match the actual number of items that could be retrieved with pagination. Consider documenting this behavior or adjusting the totalCount to reflect the actual merged result size.
| totalCount: generalRes.totalCount + medicalRes.totalCount, | |
| // totalCount reflects the actual merged result size for this query | |
| totalCount: merged.length, |
| 人気記事 | ||
| </h3> |
There was a problem hiding this comment.
The label "人気記事" (Popular Articles) is misleading. The section now displays the latest articles (latestArticles) instead of popular articles. Either update the label to "最新記事" (Latest Articles) or implement proper popular article tracking.
| 最新記事 | ||
| </h3> | ||
| <ul className="space-y-4"> | ||
| {sortedByNewest.slice(0, 5).map(({ article, endpoint }) => ( | ||
| <li key={article.id}> | ||
| {latestArticles.slice(0, 5).map((item) => ( |
There was a problem hiding this comment.
The section labeled "最新記事" (Latest Articles) displays the same data source (latestArticles) as the "人気記事" section above. This creates duplicate content. Consider removing this section or fetching a different set of articles.
src/app/llm.txt/route.ts
Outdated
| export async function GET() { | ||
| const [generalArticles, medicalArticles, categories, tags] = | ||
| await Promise.all([ | ||
| client.getList({ endpoint: 'general', queries: { limit: 100 } }), | ||
| client.getList({ endpoint: 'medical-articles', queries: { limit: 100 } }), | ||
| client.getList({ endpoint: 'categories' }), | ||
| client.getList({ endpoint: 'tags' }), |
There was a problem hiding this comment.
The code uses client.getList() which is not defined in the exported client interface. Based on the microcms.ts file, the client uses client.get() method. This will cause a runtime error. Replace getList with get and ensure proper type casting.
| export async function GET() { | |
| const [generalArticles, medicalArticles, categories, tags] = | |
| await Promise.all([ | |
| client.getList({ endpoint: 'general', queries: { limit: 100 } }), | |
| client.getList({ endpoint: 'medical-articles', queries: { limit: 100 } }), | |
| client.getList({ endpoint: 'categories' }), | |
| client.getList({ endpoint: 'tags' }), | |
| type MicroCMSListResponse<T = any> = { | |
| contents: T[] | |
| } | |
| export async function GET() { | |
| const [generalArticles, medicalArticles, categories, tags] = | |
| await Promise.all([ | |
| client.get<MicroCMSListResponse>({ endpoint: 'general', queries: { limit: 100 } }), | |
| client.get<MicroCMSListResponse>({ endpoint: 'medical-articles', queries: { limit: 100 } }), | |
| client.get<MicroCMSListResponse>({ endpoint: 'categories' }), | |
| client.get<MicroCMSListResponse>({ endpoint: 'tags' }), |
src/app/llm.txt/route.ts
Outdated
| ${generalArticles.contents.map((a) => `- [${a.title}](/general/${a.id}): ${a.excerpt || ''}`).join('\n')} | ||
| ${medicalArticles.contents.map((a) => `- [${a.title}](/medical-articles/${a.id}): ${a.excerpt || ''}`).join('\n')} |
There was a problem hiding this comment.
The code assumes articles have an 'excerpt' property that doesn't exist in the Article type definition. The Article type only has id, title, content, publishedAt, etc., but no excerpt field. This will result in undefined values in the output.
| queries: { | ||
| depth: 2, // 関連記事も取得するためにdepthを指定 | ||
| filters: buildFilters(params), | ||
| orders: '-publishedAt', |
There was a problem hiding this comment.
The fetchFromEndpoint function hardcodes the ordering to '-publishedAt', which removes flexibility for callers who might want different sorting options. Consider adding an 'orders' parameter to ArticleListParams to allow customizable sorting.
| Sitemap: https://med-dent-hub.vercel.app/sitemap.xml | ||
| Disallow: /draft/ | ||
|
|
||
| Sitemap: https://www.ishatohaisha.com/sitemap.xml |
There was a problem hiding this comment.
The sitemap URL has changed from "https://med-dent-hub.vercel.app/sitemap.xml" to "https://www.ishatohaisha.com/sitemap.xml". This appears to be a domain migration. Ensure that the new domain is correct and properly configured, and verify that the sitemap is accessible at this URL.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .catch(() => ({ contents: [] as Article[] })), | ||
| client | ||
| .get<ArticleResponse>({ | ||
| endpoint: 'medical-articles', | ||
| queries: { ids: ids.join(','), limit: ids.length }, | ||
| }) | ||
| .catch(() => ({ contents: [] as Article[] })), |
There was a problem hiding this comment.
The error handling in getArticlesByIds silently catches all errors and returns empty arrays. This masks potential issues like network failures, authentication errors, or API configuration problems. Consider logging errors or propagating them so that callers can handle different failure scenarios appropriately. Silent failures make debugging difficult in production.
| .catch(() => ({ contents: [] as Article[] })), | |
| client | |
| .get<ArticleResponse>({ | |
| endpoint: 'medical-articles', | |
| queries: { ids: ids.join(','), limit: ids.length }, | |
| }) | |
| .catch(() => ({ contents: [] as Article[] })), | |
| .catch((error) => { | |
| console.error('Failed to fetch articles from "general":', error) | |
| return { contents: [] as Article[] } | |
| }), | |
| client | |
| .get<ArticleResponse>({ | |
| endpoint: 'medical-articles', | |
| queries: { ids: ids.join(','), limit: ids.length }, | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to fetch articles from "medical-articles":', error) | |
| return { contents: [] as Article[] } | |
| }), |
| const [generalRes, medicalRes] = await Promise.all([ | ||
| fetchFromEndpoint('general', rest), | ||
| fetchFromEndpoint('medical-articles', rest), | ||
| ]) | ||
|
|
||
| return data.contents | ||
| } | ||
| const merged = [ | ||
| ...withEndpoint(generalRes.contents, 'general'), | ||
| ...withEndpoint(medicalRes.contents, 'medical-articles'), | ||
| ] | ||
| .sort( | ||
| (a, b) => | ||
| new Date(b.publishedAt).getTime() - new Date(a.publishedAt).getTime() | ||
| ) | ||
| .slice(0, rest.limit ?? Infinity) |
There was a problem hiding this comment.
When fetching from both endpoints without specifying a limit, each endpoint's API call will use its default limit (likely 10 items each). After merging and sorting, the code slices to rest.limit ?? Infinity, but if no limit was specified in params, you'll get at most 20 items (10 from each endpoint) instead of all articles. This could lead to missing articles in the merged view. Consider either fetching with a very high limit when merging both endpoints, or documenting this limitation clearly.
| limit: generalRes.limit, | ||
| offset: generalRes.offset, |
There was a problem hiding this comment.
The returned limit and offset values come from only the general endpoint response, but the merged contents and totalCount represent data from both endpoints. This creates inconsistent metadata where limit and offset don't match the actual returned data structure. Callers expecting to use these values for pagination will get incorrect results. Consider either returning merged/adjusted values or documenting that these fields are not meaningful when merging both endpoints.
| limit: generalRes.limit, | |
| offset: generalRes.offset, | |
| // Reflect the actual returned result and requested pagination, not just the general endpoint | |
| limit: merged.length, | |
| offset: rest.offset ?? 0, |
| function stripHtml(html: string): string { | ||
| return html | ||
| .replace(/<[^>]+>/g, ' ') | ||
| .replace(/ /g, ' ') | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/\s+/g, ' ') | ||
| .trim() |
There was a problem hiding this comment.
The stripHtml function uses regex-based HTML stripping which can be incomplete and may miss malicious content or edge cases. While this endpoint is read-only and serves public content, consider using a proper HTML sanitization library like 'sanitize-html' or a DOM-based parser to ensure all HTML tags and entities are correctly handled, especially if article content could contain user-generated or external content.
| function stripHtml(html: string): string { | |
| return html | |
| .replace(/<[^>]+>/g, ' ') | |
| .replace(/ /g, ' ') | |
| .replace(/&/g, '&') | |
| .replace(/</g, '<') | |
| .replace(/>/g, '>') | |
| .replace(/"/g, '"') | |
| .replace(/\s+/g, ' ') | |
| .trim() | |
| import sanitizeHtml from 'sanitize-html' | |
| function stripHtml(html: string): string { | |
| const sanitized = sanitizeHtml(html, { | |
| allowedTags: [], | |
| allowedAttributes: {}, | |
| }) | |
| return sanitized.replace(/\s+/g, ' ').trim() |
| 人気記事 | ||
| </h3> | ||
| <ul> | ||
| {sidebarPopular.map(({ article, endpoint }, index) => ( | ||
| {latestArticles.map((item, index) => ( |
There was a problem hiding this comment.
The section is labeled "人気記事" (Popular Articles) but displays latestArticles data. This is misleading to users who expect to see the most popular articles but actually see the latest ones. Either update the heading to "最新記事" (Latest Articles) to match the data being displayed, or implement actual popularity tracking and display popular articles as the label suggests. Note: The old code had a comment indicating GTM would replace this later, but that context is now lost.
| type ArticleListParams = { | ||
| q?: string | ||
| limit?: number | ||
| offset?: number |
There was a problem hiding this comment.
The PR description is entirely empty (only contains template placeholders), making it impossible to understand what changes were made and why. This violates good PR practices and makes code review and future code archaeology difficult. Please fill out the description with actual change details, the motivation for the refactoring, and any relevant context about the new LLM endpoints.
| Sitemap: https://med-dent-hub.vercel.app/sitemap.xml | ||
| Disallow: /draft/ | ||
|
|
||
| Sitemap: https://www.ishatohaisha.com/sitemap.xml |
There was a problem hiding this comment.
The robots.txt correctly blocks the /draft/ directory from search engines, which aligns with the noindex metadata in the draft layout. However, the sitemap URL was changed from 'med-dent-hub.vercel.app' to 'www.ishatohaisha.com'. Ensure this domain change is intentional and that the sitemap actually exists at the new URL, otherwise search engines won't be able to discover your pages.
| Sitemap: https://www.ishatohaisha.com/sitemap.xml | |
| Sitemap: https://med-dent-hub.vercel.app/sitemap.xml |
| async function fetchFromEndpoint( | ||
| endpoint: Endpoint, | ||
| params?: ArticleListParams | ||
| ): Promise<ArticleResponse> { | ||
| return client.get<ArticleResponse>({ | ||
| endpoint, | ||
| queries: { | ||
| q: params?.q, | ||
| limit: params?.limit, | ||
| offset: params?.offset, | ||
| filters: filters.length > 0 ? filters.join('[and]') : undefined, | ||
| }, | ||
| }) | ||
| return data | ||
| } | ||
|
|
||
| export const getGeneralArticleById = async (id: string) => { | ||
| const data = await client.get<Article>({ | ||
| endpoint: 'general', | ||
| contentId: id, | ||
| queries: { | ||
| depth: 2, // 関連記事も取得するためにdepthを指定 | ||
| filters: buildFilters(params), | ||
| orders: '-publishedAt', |
There was a problem hiding this comment.
The fetchFromEndpoint function always applies orders: '-publishedAt' regardless of whether params specify ordering. When merging results from both endpoints (lines 117-130), the data is fetched with this hardcoded ordering from each endpoint, then merged and re-sorted. However, there's no way for callers to customize the sort order. Consider either accepting an orders parameter in ArticleListParams, or documenting that results are always sorted by publishedAt descending.
| function buildFilters(params?: ArticleListParams): string | undefined { | ||
| const filters: string[] = [] | ||
|
|
||
| if (params?.categoryId) { | ||
| filters.push(`category[equals]${params.categoryId}`) | ||
| } | ||
| // 複数タグ対応（tagIdsが優先） | ||
| if (params?.tagIds && params.tagIds.length > 0) { | ||
| // 複数のタグをAND条件でフィルタリング | ||
| params.tagIds.forEach((tagId) => { | ||
| filters.push(`tags[contains]${tagId}`) | ||
| }) | ||
| } else if (params?.tagId) { | ||
| // 後方互換性のため、単一タグもサポート | ||
| filters.push(`tags[contains]${params.tagId}`) | ||
| } | ||
| if (params?.isFeatured) { | ||
| filters.push('isFeatured[equals]true') | ||
| } | ||
|
|
||
| return filters.length > 0 ? filters.join('[and]') : undefined |
There was a problem hiding this comment.
The buildFilters function doesn't validate or sanitize the categoryId, tagId, or tagIds values before constructing the filter string. If these values contain special characters like brackets, they could break the microCMS query syntax or potentially lead to unexpected query behavior. Consider adding validation to ensure these IDs only contain expected characters (e.g., alphanumeric and hyphens), or properly escape/encode them before use.
|
|
||
| ## タグ | ||
|
|
||
| ${tags.map((t) => `- [${t.name}](${BASE_URL}/articles?tag=${t.id})`).join('\n')} | ||
|
|
There was a problem hiding this comment.
Article titles and descriptions are directly interpolated into the markdown output without escaping. If these fields contain special markdown characters (like brackets, backticks, or asterisks), they could break the markdown formatting or potentially be interpreted as markdown syntax. Consider escaping markdown special characters in titles and descriptions to ensure proper rendering and prevent unintended formatting issues.
変更内容
変更の種類
テスト
チェックリスト
スクリーンショット（UI変更がある場合）
関連Issue
Closes #
追加情報