-
Notifications
You must be signed in to change notification settings - Fork 80
Fix site presentation issues #480
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
Conversation
✅ Deploy Preview for zero-to-nix ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughA new content-access layer was added at Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Component/Page
participant Helpers as content/collections
participant Astro as astro:content
rect #eef6ff
Note over Page,Helpers: New flow — centralized helpers
Page->>Helpers: import getStartPagesByOrderParam/getConceptPagesAlphabetical()
Helpers->>Astro: getCollection("start") / getCollection("concepts")
Astro-->>Helpers: raw collection data
Helpers->>Helpers: optional sort/filter (e.g., sort by data.order)
Helpers-->>Page: ordered/filtered pages
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
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: 3
🧹 Nitpick comments (2)
src/pages/llms-full.txt.ts (1)
1-4: Align helper name with behavior: sort concepts alphabetically at the sourceCentralization looks good. However, getConceptPagesAlphabetical() currently returns unsorted concepts; the name implies a title-ascending sort. Sort in the helper to ensure consistent ordering across pages and dropdowns.
Apply in src/content/collections.ts:
export const getConceptPagesAlphabetical = async () => { - return await getCollection("concepts"); + return (await getCollection("concepts")).sort((a, b) => + a.data.title.localeCompare(b.data.title), + ); };Also applies to: 23-24
src/pages/start/[slug].astro (1)
34-34: Avoid duplicate fetch for count; use value from getStaticPathsUse the total passed in props and fall back to a fetch only if needed.
-const numQuickStartPages = (await getStartPagesByOrderParam()).length; +const numQuickStartPages = + Astro.props.total ?? (await getStartPagesByOrderParam()).length;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/Navbar.astro(1 hunks)src/components/Pagination.astro(1 hunks)src/components/mdx/Languages.astro(1 hunks)src/content/collections.ts(1 hunks)src/pages/concepts/[slug].astro(2 hunks)src/pages/concepts/index.astro(1 hunks)src/pages/llms-full.txt.ts(2 hunks)src/pages/llms-small.txt.ts(2 hunks)src/pages/llms.txt.ts(2 hunks)src/pages/start/[slug].astro(2 hunks)src/pages/start/index.astro(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/pages/llms.txt.ts (1)
src/content/collections.ts (2)
getStartPagesByOrderParam(3-8)getConceptPagesAlphabetical(10-12)
src/pages/llms-small.txt.ts (1)
src/content/collections.ts (2)
getStartPagesByOrderParam(3-8)getConceptPagesAlphabetical(10-12)
src/pages/llms-full.txt.ts (1)
src/content/collections.ts (2)
getStartPagesByOrderParam(3-8)getConceptPagesAlphabetical(10-12)
🔇 Additional comments (9)
src/components/mdx/Languages.astro (1)
20-20: LGTM! This fixes the language selector issue.The change from a single-quoted string to a template literal ensures Alpine.js correctly evaluates the
x-bind:classexpression. This addresses the language selector issue mentioned in the PR objectives.src/content/collections.ts (1)
3-8: LGTM! The sorting logic is correct.The function correctly sorts start pages by the
orderfield in ascending order, addressing the page ordering issue mentioned in the PR objectives.src/components/Pagination.astro (1)
4-12: LGTM! The migration to the new helper is correct.The component now uses
getStartPagesByOrderParam()to fetch ordered start pages. The pagination logic for finding previous/next pages remains intact and correct.src/pages/llms-small.txt.ts (1)
1-23: LGTM! The migration to the new collection helpers is correct.The file correctly uses both
getStartPagesByOrderParam()andgetConceptPagesAlphabetical()to fetch the respective collections. The template rendering logic remains unchanged.src/pages/llms.txt.ts (1)
1-4: LGTM! The migration to the new collection helpers is correct.The file correctly uses both
getStartPagesByOrderParam()andgetConceptPagesAlphabetical()to fetch the respective collections. The template rendering logic remains unchanged.Also applies to: 43-44
src/pages/concepts/[slug].astro (1)
18-25: LGTM! The migration to the new helper is correct.The file correctly uses
getConceptPagesAlphabetical()for bothgetStaticPathsand related concepts resolution. The addition of thesatisfies GetStaticPathstype guard improves type safety.Also applies to: 34-34
src/components/Navbar.astro (1)
9-28: LGTM! The migration to the new collection helpers is correct.The navbar correctly uses
getStartPagesByOrderParam()for start pages andgetConceptPagesAlphabetical()for concept pages. The mapping logic to create dropdown items remains intact.src/pages/start/index.astro (2)
8-8: LGTM on switching to the centralized ordered helper
15-15: The review comment is incorrect; no action needed.The
startPagePath()function already handles the slug format correctly. It internally removes the leading "/" viaslug.substring(1)on line 12 of src/lib/utils.ts. ThegetStartPagesByOrderParam()function returns collection items with slugs that include a leading "/", andstartPagePath()is designed to accept this format. All call sites throughout the codebase follow the same pattern of passing the slug directly without additional sanitization, confirming this is the intended behavior.Likely an incorrect or invalid review comment.
This PR fixes two issues:
1. Broken language selector
(notice none of the languages appears selected)
The PR fixes this:
2. Quick start pages out of order
(this should begin with "1. Get Nix running on your system")
The PR fixes this: