feat(book): add code kiko page use gitbook #2633
Conversation
go-wyvern
changed the title
Summary of ChangesHello @go-wyvern, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust documentation system by integrating GitBook (HonKit) into the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to display GitBook-generated tutorials within the application. The implementation includes a build script for the books, a new Vue component to render them in an iframe, and the tutorial content itself. My review has identified a couple of areas for improvement:
- The
build-books.shscript relies on a global npm installation, which is not a robust practice. I've suggested moving to a project-local dependency usingnpx. - The
books.vuecomponent has a bug in its loading logic and a user experience issue with the loading indicator. I've provided comments to fix the bug and address the UX problem.
The rest of the changes, including the updates to the tutorial markdown files for better navigation and syntax highlighting, are well-executed. Addressing the feedback will make the implementation more robust and user-friendly.
| // Load success | ||
| if (iframeRef.value) { | ||
| iframeRef.value.src = htmlPath.value | ||
| } |
There was a problem hiding this comment.
This block of code is buggy. When loadBook is called from onMounted, the <iframe> has not been rendered yet because of v-if="loading", so iframeRef.value is null. Attempting to access iframeRef.value.src will throw an error (which is currently caught). This code is also redundant because the :src="htmlPath" binding on the <iframe> tag already handles setting the source once the iframe is rendered. This block should be removed.
| # Check if honkit is installed | ||
| if ! command -v honkit &> /dev/null; then | ||
| echo "HonKit not installed, installing..." | ||
| npm install -g honkit | ||
| fi |
There was a problem hiding this comment.
Installing npm packages globally from a build script (npm install -g honkit) is not a good practice. It can cause permission issues, conflicts with other globally installed packages, and makes the project less self-contained. It's better to list honkit as a devDependency in package.json and run it with npx. This block can be removed, and honkit can be invoked with npx directly.
|
|
||
| # Build GitBook | ||
| cd "$dir" | ||
| honkit build . "../../$OUTPUT_DIR" |
There was a problem hiding this comment.
To avoid relying on a globally installed package and to make the build more reliable, use npx to execute honkit. This will use the version specified in your project's devDependencies (which should be added to package.json). This change should be made along with removing the global installation check above.
| honkit build . "../../$OUTPUT_DIR" | |
| npx honkit build . "../../$OUTPUT_DIR" |
| } finally { | ||
| loading.value = false | ||
| } |
There was a problem hiding this comment.
Setting loading.value = false in the finally block provides a poor user experience. The loading spinner disappears after the quick HEAD request finishes, but before the actual <iframe> content (which can be large) has loaded. This leaves the user looking at a blank space instead of a loading indicator.
To fix this, the loading state should only be set to false inside handleIframeLoad and handleIframeError. The loadBook function should only be responsible for checking for the book's existence and setting the error state if it's not found, without changing the loading state on success.
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| error.value = err instanceof Error ? err.message : 'Failed to load book' | ||
| console.error('Failed to load book:', err) | ||
| } finally { | ||
| loading.value = false |
There was a problem hiding this comment.
Critical: Race condition in loading state
The finally block sets loading.value = false immediately after assigning the iframe src, but the iframe hasn't finished loading yet. This means the loading spinner disappears before content actually loads.
Fix: Remove loading.value = false from the finally block and rely solely on the @load and @error handlers:
| loading.value = false | |
| } catch (err) { | |
| error.value = err instanceof Error ? err.message : 'Failed to load book' | |
| console.error('Failed to load book:', err) | |
| loading.value = false // Only set false on error | |
| } | |
| // Remove finally block entirely |
| // Watch for route changes | ||
| watch( | ||
| () => route.params.pathMatch, |
There was a problem hiding this comment.
Critical: Missing request cancellation
When navigating between books rapidly, there's no cleanup of the previous loadBook() call. This can cause race conditions where stale responses overwrite newer ones.
Recommended fix: Add request ID tracking:
| () => route.params.pathMatch, | |
| let currentLoadId = 0 | |
| // Watch for route changes | |
| watch(() => route.params.pathMatch, () => { | |
| currentLoadId++ // Invalidate previous requests | |
| loadBook() | |
| }) |
Then check currentLoadId in loadBook() before updating state.
| const bookPath = computed(() => { | ||
| const pathMatch = route.params.pathMatch | ||
| if (Array.isArray(pathMatch)) { | ||
| return pathMatch.join('/') | ||
| } | ||
| return pathMatch || '' | ||
| }) |
There was a problem hiding this comment.
High Security Risk: Path traversal vulnerability
The wildcard route accepts arbitrary paths without validation. An attacker could access files outside the books directory:
/books/../../../etc/passwd/books/tutorial/../../config/secrets.json
Required fix: Add path validation and sanitization:
| const bookPath = computed(() => { | |
| const pathMatch = route.params.pathMatch | |
| if (Array.isArray(pathMatch)) { | |
| return pathMatch.join('/') | |
| } | |
| return pathMatch || '' | |
| }) | |
| const bookPath = computed(() => { | |
| const pathMatch = route.params.pathMatch | |
| let rawPath = Array.isArray(pathMatch) ? pathMatch.join('/') : pathMatch || '' | |
| // Remove path traversal attempts | |
| rawPath = rawPath.replace(/\.\./g, '').replace(/\/+/g, '/') | |
| rawPath = rawPath.replace(/^\/+/, '') | |
| // Whitelist allowed patterns | |
| if (!rawPath.match(/^tutorial\/[a-zA-Z0-9_-]+/)) { | |
| return '' // Invalid path | |
| } | |
| return rawPath | |
| }) |
|
|
||
| <iframe | ||
| v-else | ||
| ref="iframeRef" |
There was a problem hiding this comment.
Medium Security: Missing iframe sandbox
The iframe loads content without sandboxing restrictions. While the content is pre-built, defense-in-depth requires sandboxing.
Recommendation:
| ref="iframeRef" | |
| <iframe | |
| v-else | |
| ref="iframeRef" | |
| :src="htmlPath" | |
| class="book-iframe" | |
| sandbox="allow-scripts allow-same-origin" | |
| @load="handleIframeLoad" | |
| @error="handleIframeError" | |
| /> |
Test to ensure GitBook features still work with these restrictions.
| "postinstall": "./install-spx.sh && ./build-books.sh", | ||
| "predev": "./install-spx.sh && ./build-wasm.sh && ./build-books.sh", |
There was a problem hiding this comment.
Critical Performance: Build on every npm install
Running build-books.sh on every npm install adds 10-20 seconds to:
- CI/CD pipelines (every dependency install)
- Developer onboarding
- Container builds
Problems:
- No caching - rebuilds even if source unchanged
- Output dir is gitignored, so rebuilds on every checkout
- Runs on both
postinstallANDpredev
Recommendation: Either:
- Move to explicit build step:
npm run build:books - Add conditional logic to skip if outputs are up-to-date
- Run only in production builds, not development
| # Build GitBook | ||
| cd "$dir" | ||
| honkit build . "../../$OUTPUT_DIR" | ||
| cd - > /dev/null |
There was a problem hiding this comment.
Critical: Missing error handling
If honkit build fails, the script may continue due to insufficient error checking. This can lead to silent failures.
Recommended fix:
| # Build GitBook | |
| cd "$dir" | |
| honkit build . "../../$OUTPUT_DIR" | |
| cd - > /dev/null | |
| cd "$dir" || { echo "Failed to enter $dir"; exit 1; } | |
| if ! honkit build . "../../$OUTPUT_DIR"; then | |
| echo "Failed to build $dir" | |
| exit 1 | |
| fi | |
| cd - > /dev/null || exit 1 |
| # Check if honkit is installed | ||
| if ! command -v honkit &> /dev/null; then | ||
| echo "HonKit not installed, installing..." | ||
| npm install -g honkit |
There was a problem hiding this comment.
Performance: Global npm install on every build
Installing HonKit globally has issues:
- Fails in CI environments with permission restrictions
- No version pinning → inconsistent builds
- Slower than local installs
Better approach:
- Add to
package.json:"honkit": "^4.0.0" - Use:
npx honkit buildinstead ofhonkit build
This ensures version consistency and leverages npm's cache.
| redirects: | ||
| previous/page: new-folder/page.md |
There was a problem hiding this comment.
Documentation: Placeholder redirect should be removed
This redirect configuration references non-existent pages (previous/page and new-folder/page.md). It's clearly a placeholder example.
Recommendation: Remove or comment out:
| redirects: | |
| previous/page: new-folder/page.md | |
| # redirects: | |
| # old/path: new/path.md # Add actual redirects here if needed |
|
|
||
| echo "" | ||
| echo "GitBook build completed!" | ||
| echo "Access path: /books/$TUTORIAL_DIR/<directory-name>" |
There was a problem hiding this comment.
Documentation: Incorrect path information
This message is misleading - $TUTORIAL_DIR expands to "tutorial", making the path redundant.
Fix:
| echo "Access path: /books/$TUTORIAL_DIR/<directory-name>" | |
| echo "Built files: $OUTPUT_BASE/tutorial/<directory-name>" | |
| echo "Web access: /books/tutorial/<directory-name>" |
Code Review SummaryThis PR successfully adds GitBook documentation infrastructure with a clean Vue-based viewer. However, several critical issues need attention: Critical Issues:
High Priority:
Positive Notes:
Please address the security and critical issues before merging. |
There was a problem hiding this comment.
Pull request overview
This pull request adds GitBook (HonKit) integration to build and serve the CodeKiko tutorial documentation as an interactive web-based book. The implementation includes configuration files, a build script, and a Vue.js component to render the generated HTML within the application.
Key Changes:
- Converted code fences from
xgotogosyntax highlighting throughout all tutorial chapters - Added GitBook configuration files (book.json, .gitbook.yaml) and restructured summary.md for GitBook compatibility
- Implemented a books route handler with an iframe-based viewer component
- Created a build script to generate static HTML from markdown using HonKit
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorial/codeKiko/book.json | GitBook configuration specifying title, language, and structure |
| tutorial/codeKiko/.gitbook.yaml | GitBook YAML config with structure definitions and placeholder redirects |
| tutorial/codeKiko/README.md | Updated chapter links to remove ./ prefix for GitBook compatibility |
| tutorial/codeKiko/summary.md | Restructured table of contents with GitBook-compatible anchor links |
| tutorial/codeKiko/chapter*.md | Changed all code fences from xgo to go for syntax highlighting |
| spx-gui/src/router.ts | Added wildcard route /books/:pathMatch(.*)* for book viewer |
| spx-gui/src/pages/books/books.vue | New Vue component rendering books in iframe with loading/error states |
| spx-gui/build-books.sh | Shell script to build GitBooks using HonKit |
| spx-gui/package.json | Updated npm scripts to include book building in postinstall and predev |
| spx-gui/.gitignore | Added public/books to ignore generated GitBook output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| OUTPUT_DIR="$OUTPUT_BASE/$TUTORIAL_DIR/$DIR_NAME" | ||
|
|
||
| echo "Building: $dir -> $OUTPUT_DIR" | ||
|
|
||
| # Build GitBook | ||
| cd "$dir" | ||
| honkit build . "../../$OUTPUT_DIR" | ||
| cd - > /dev/null |
There was a problem hiding this comment.
The output path construction creates a redundant directory structure. The script builds to $OUTPUT_BASE/$TUTORIAL_DIR/$DIR_NAME which expands to spx-gui/public/books/tutorial/codeKiko. However, the router expects paths like /books/tutorial/codeKiko to work. This means the final URL will be /books/tutorial/codeKiko/index.html. Consider if this nested structure is intentional or if books should be built directly to spx-gui/public/books/codeKiko for cleaner URLs like /books/codeKiko.
|
|
||
|  | ||
| > 课程地址：https://x.qiniu.com/editor/curator/Coding-Course-1/sprites/Kiko/code | ||
There was a problem hiding this comment.
All code fences throughout the chapters have been changed from xgo to go. While this change enables syntax highlighting in GitBook (since xgo is not a recognized language), it may cause confusion as the code is actually XGo language, not Go. The syntax highlighting might not be accurate for XGo-specific features. Consider using a comment or note in the documentation to clarify that the code is XGo despite the go syntax highlighting, or investigate if GitBook/HonKit supports custom language definitions.
| > 注意：下面的示例代码使用 **XGo** 语言，这里使用 `go` 仅作为语法高亮标签。 |
|
|
||
| redirects: | ||
| previous/page: new-folder/page.md |
There was a problem hiding this comment.
The redirect configuration contains a placeholder that should be updated or removed. The current redirect from "previous/page" to "new-folder/page.md" appears to be example/template content that doesn't correspond to actual pages in this documentation.
| redirects: | |
| previous/page: new-folder/page.md |
| # Check if honkit is installed | ||
| if ! command -v honkit &> /dev/null; then | ||
| echo "HonKit not installed, installing..." | ||
| npm install -g honkit |
There was a problem hiding this comment.
The build script should validate that HonKit installed successfully before proceeding. If the npm install command fails (e.g., due to network issues or permissions), the script will continue and fail later with a less clear error message. Consider adding error checking after the installation command.
| npm install -g honkit | |
| npm install -g honkit | |
| if [ $? -ne 0 ]; then | |
| echo "Error: Failed to install HonKit via npm." >&2 | |
| exit 1 | |
| fi | |
| if ! command -v honkit &> /dev/null; then | |
| echo "Error: HonKit was installed but is still not available on PATH." >&2 | |
| exit 1 | |
| fi |
| try { | ||
| // Check if file exists | ||
| const response = await fetch(htmlPath.value, { method: 'HEAD' }) | ||
| if (!response.ok) { | ||
| throw new Error(`Book not found: ${bookPath.value}`) | ||
| } | ||
| // Load success | ||
| if (iframeRef.value) { | ||
| iframeRef.value.src = htmlPath.value | ||
| } | ||
| } catch (err) { | ||
| error.value = err instanceof Error ? err.message : 'Failed to load book' | ||
| console.error('Failed to load book:', err) | ||
| } finally { |
There was a problem hiding this comment.
Missing error handling for the iframe HEAD request. While there is a try-catch block, if the fetch itself fails due to network issues, CORS, or other reasons (not just 404), the error handling might not provide helpful information. Additionally, the HEAD request check may create a race condition where the file exists during the HEAD check but fails to load in the iframe.
| try { | |
| // Check if file exists | |
| const response = await fetch(htmlPath.value, { method: 'HEAD' }) | |
| if (!response.ok) { | |
| throw new Error(`Book not found: ${bookPath.value}`) | |
| } | |
| // Load success | |
| if (iframeRef.value) { | |
| iframeRef.value.src = htmlPath.value | |
| } | |
| } catch (err) { | |
| error.value = err instanceof Error ? err.message : 'Failed to load book' | |
| console.error('Failed to load book:', err) | |
| } finally { | |
| let response: Response | |
| try { | |
| // Check if file exists and is reachable | |
| response = await fetch(htmlPath.value, { method: 'HEAD' }) | |
| } catch (err) { | |
| // Network, CORS, or other low-level fetch error | |
| error.value = 'Network or permission error while loading book.' | |
| console.error('Failed to load book (HEAD request failed):', err) | |
| loading.value = false | |
| return | |
| } | |
| if (!response.ok) { | |
| if (response.status === 404) { | |
| error.value = `Book not found: ${bookPath.value}` | |
| } else { | |
| error.value = `Failed to load book (HTTP ${response.status}).` | |
| } | |
| console.error('Failed to load book (non-OK response):', response.status, response.statusText) | |
| loading.value = false | |
| return | |
| } | |
| // HEAD request succeeded, attempt to load into iframe | |
| const iframe = iframeRef.value | |
| if (iframe) { | |
| // Ensure loading state is updated based on iframe events | |
| iframe.onload = handleIframeLoad | |
| iframe.onerror = (event: Event | string) => { | |
| error.value = 'Failed to display book content.' | |
| console.error('Failed to load book into iframe:', event) | |
| loading.value = false | |
| } | |
| iframe.src = htmlPath.value | |
| } else { | |
| // No iframe available to load the content | |
| error.value = 'Unable to display book: viewer is not available.' |
| { | ||
| path: '/books/:pathMatch(.*)*', | ||
| component: () => import('@/pages/books/books.vue') | ||
| }, |
There was a problem hiding this comment.
Route ordering issue: This catch-all route for /books/:pathMatch(.*)* is placed before the final 404 catch-all route /:pathMatch(.*)*, which is correct. However, ensure this doesn't conflict with any existing routes. The wildcard pattern will match any path starting with /books/, which could potentially override more specific book-related routes if they're added later. Consider documenting this route precedence behavior.
| if (isHtmlFile) { | ||
| return `/books/${bookPath.value}` | ||
| } else { | ||
| // Directory path, load index.html |
There was a problem hiding this comment.
Path construction could result in double slashes. On line 24, when bookPath.value is empty (root books path), the expression evaluates to /books/${bookPath.value.endsWith('/') ? '' : '/'}index.html which becomes /books//index.html because empty string doesn't end with '/'. Consider handling the empty string case explicitly or using a path joining utility.
| // Directory path, load index.html | |
| // Directory path, load index.html | |
| if (!bookPath.value) { | |
| // Root books path | |
| return '/books/index.html' | |
| } |
| @@ -0,0 +1,46 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
The script is missing executable permissions. Shell scripts should have the execute bit set. You should run chmod +x spx-gui/build-books.sh to make this script executable.
| ref="iframeRef" | ||
| :src="htmlPath" | ||
| class="book-iframe" | ||
| frameborder="0" |
There was a problem hiding this comment.
The deprecated frameborder attribute is used on the iframe. This HTML attribute has been deprecated in favor of CSS. Use border: none; in the CSS instead (which is already present in .book-iframe), and remove the frameborder="0" attribute from the iframe element.
| frameborder="0" |
| # Check if honkit is installed | ||
| if ! command -v honkit &> /dev/null; then | ||
| echo "HonKit not installed, installing..." | ||
| npm install -g honkit |
There was a problem hiding this comment.
The line npm install -g honkit introduces an unpinned, globally-installed third-party build dependency, which is a classic supply-chain risk: if the honkit package or its transitive dependencies are compromised, an attacker could execute arbitrary code during your docs build and inject malicious content into the generated site or access CI secrets. Because this script auto-installs whatever the latest honkit is, there is no integrity control or version pinning. To mitigate this, pin honkit to a specific version and install it via your existing project dependency management (e.g., local devDependency + lockfile or npx with an explicit version) rather than a global, latest install inside the build script.
1 participant
No description provided.