Add Netlify static portfolio demo - #34
Closed
tomnguyen103 wants to merge 2 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a zero-dollar static portfolio demo mode: static Netlify export, browser-side fixture-backed ApiClient and search, optional SHA-256 passcode gate, conditional frontend routing/UI for demo mode, and runbook + Netlify config for deployment. ChangesNetlify Static Portfolio Demo
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
tomnguyen103
marked this pull request as ready for review
June 8, 2026 02:46
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
docs/portfolio-demo-netlify.md (3)
31-35: ⚡ Quick winDocument the behavior when the passcode hash is omitted.
The runbook describes the passcode gate as "optional" (line 11) but doesn't clearly state what happens when
NEXT_PUBLIC_DEMO_ACCESS_HASHis NOT set in Netlify. Based on the code context, omitting this variable disables the gate entirely and grants immediate access. Consider adding a note after line 35 to make this explicit.📝 Suggested addition
NEXT_PUBLIC_DEMO_ACCESS_HASH=<lowercase sha256 hash of your demo passcode>
+> Note: Omitting
NEXT_PUBLIC_DEMO_ACCESS_HASHdisables the passcode gate entirely and grants immediate access to all visitors. Only include this variable if you want casual access control for the static demo.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/portfolio-demo-netlify.mdaround lines 31 - 35, Add an explicit note to
the demo Netlify docs stating the behavior when NEXT_PUBLIC_DEMO_ACCESS_HASH is
not set: explain that omitting NEXT_PUBLIC_DEMO_ACCESS_HASH disables the
passcode gate and grants immediate access to all visitors, and place this note
directly after the environment-variable instruction (the block that lists
NEXT_PUBLIC_DEMO_ACCESS_HASH) so readers know the default behavior when the
variable is absent.</details> <!-- cr-comment:v1:be600b87b9482d7b9681d47c --> --- `73-78`: _⚡ Quick win_ **Clarify the purpose of the first build.** The verification steps run `npm run build` twice (line 75 without demo env vars, line 78 with them). If the first build is intentional (e.g., to verify the normal build still works), add a comment explaining why. Otherwise, remove line 75 to avoid confusion. <details> <summary>📝 Suggested clarification</summary> ```diff npm ci npm run lint -npm run build +# Verify normal build still works +npm run build $env:NEXT_PUBLIC_SECOND_BRAIN_DEMO_MODE="static" $env:NEXT_PUBLIC_AGENTIC_RAG_ENABLED="true" +# Build static export for Netlify npm run buildOr remove the first build if it's not needed:
npm ci npm run lint -npm run build $env:NEXT_PUBLIC_SECOND_BRAIN_DEMO_MODE="static" $env:NEXT_PUBLIC_AGENTIC_RAG_ENABLED="true" npm run build🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/portfolio-demo-netlify.md` around lines 73 - 78, The docs currently run "npm run build" twice: once before setting demo env vars and again after setting NEXT_PUBLIC_SECOND_BRAIN_DEMO_MODE and NEXT_PUBLIC_AGENTIC_RAG_ENABLED; either remove the first "npm run build" if it is redundant, or keep it but add a short comment above the first "npm run build" explaining its purpose (e.g., "verify standard build succeeds before enabling demo env vars") so readers understand why the build is performed twice; update the lines referencing the two "npm run build" commands and the demo env vars (NEXT_PUBLIC_SECOND_BRAIN_DEMO_MODE, NEXT_PUBLIC_AGENTIC_RAG_ENABLED) accordingly.
96-97: 💤 Low valueSpecify where to add the portfolio link.
Line 97 suggests adding "the Netlify URL to the portfolio" but doesn't clarify whether this means updating README.md, a personal portfolio site, or somewhere else. Consider being more specific about the intended location.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/portfolio-demo-netlify.md` around lines 96 - 97, The instruction "After Netlify deploys successfully, add the Netlify URL to the portfolio as a live demo link" is ambiguous; update the doc text that currently begins "After Netlify deploys successfully" to explicitly state where to add the link: tell the reader to add the Netlify live-demo URL to their personal portfolio site and to the project's README.md under a clearly labeled "Live Demo" or "Deployment" section (keeping the repository link immediately adjacent, e.g., "Live demo: <Netlify URL> (Repo: <repo URL>)"), and mention to verify the live demo link is reachable after deployment.netlify.toml (1)
15-19: 💤 Low valueConsider adding Content-Security-Policy header.
The current security headers provide good baseline protection. For enhanced security, consider adding a
Content-Security-Policyheader to restrict resource loading and mitigate XSS risks.🔒 Example CSP header for static demo
[headers.values] X-Frame-Options = "DENY" X-Content-Type-Options = "nosniff" Referrer-Policy = "strict-origin-when-cross-origin" Permissions-Policy = "camera=(), microphone=(), geolocation=(), payment=()" Content-Security-Policy = "default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; img-src 'self' data: https:; font-src 'self' data:; connect-src 'self'"Note: Adjust
'unsafe-inline'and'unsafe-eval'based on Next.js requirements for the static export. Test thoroughly to ensure the app functions correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netlify.toml` around lines 15 - 19, Add a Content-Security-Policy header under the existing [headers.values] block (where X-Frame-Options, X-Content-Type-Options, Referrer-Policy, and Permissions-Policy are defined) by adding a Content-Security-Policy entry that restricts resource origins (e.g., default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; img-src 'self' data: https:; font-src 'self' data:; connect-src 'self'), then adjust/remove 'unsafe-inline'/'unsafe-eval' as needed for your Next.js/static export requirements and thoroughly test the app to ensure no functionality breaks.frontend/lib/api/demo-client.ts (1)
328-344: ⚡ Quick winAdd SSR safety guard to
delay()function.The
delay()function useswindow.setTimeoutwithout checking ifwindowis defined. While the static export mode (output: "export") means this code likely won't run during build, adding a guard would make the code more robust and consistent with the SSR checks elsewhere in the file (e.g., line 242).🛡️ Proposed fix to add SSR safety
function delay(ms: number, signal?: AbortSignal): Promise<void> { + if (typeof window === "undefined") { + return Promise.resolve(); + } return new Promise((resolve, reject) => { if (signal?.aborted) { reject(new DOMException("Aborted", "AbortError")); return; } const timer = window.setTimeout(resolve, ms); signal?.addEventListener( "abort", () => { window.clearTimeout(timer); reject(new DOMException("Aborted", "AbortError")); }, { once: true }, ); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/lib/api/demo-client.ts` around lines 328 - 344, The delay function uses window.setTimeout/clearTimeout which breaks SSR; update delay(ms: number, signal?: AbortSignal) to use a runtime-safe timer by switching window.setTimeout and window.clearTimeout to a safe fallback (e.g., use globalThis.setTimeout/globalThis.clearTimeout or (typeof window !== "undefined" ? window.setTimeout : setTimeout) and similarly for clearTimeout) so timers still work in SSR, and keep the existing AbortSignal handling and DOMException usage unchanged.frontend/lib/api/client.ts (1)
40-41: ⚡ Quick winConsider extracting
ApiClienttype to avoid circular imports.The current structure creates a circular dependency:
client.tsimportsdemoApifromdemo-client.ts(line 40)demo-client.tsimportsApiClienttype fromclient.tsWhile this works because the import is type-only (
import type), the circular dependency can make the codebase harder to reason about and may cause issues with certain bundlers or refactoring tools.♻️ Recommended refactor to eliminate circular dependency
Create a new file
frontend/lib/api/client-types.ts:import type { AppStatusResponse, CaptureRequest, CaptureResponse, ChatRequest, ChatResponse, // ... other imports } from "./types"; export interface ChatStreamHandlers { onDelta: (delta: ChatStreamDelta) => void; onComplete: (complete: ChatStreamComplete) => void; signal?: AbortSignal; } export interface ApiClient { getHealth(): Promise<HealthResponse>; getStatus(): Promise<AppStatusResponse>; capture(req: CaptureRequest): Promise<CaptureResponse>; // ... other methods }Then update imports:
- In
demo-client.ts:import type { ApiClient, ChatStreamHandlers } from "./client-types"- In
client.ts:import type { ApiClient, ChatStreamHandlers } from "./client-types"This eliminates the circular dependency while maintaining full type safety.
Also applies to: 447-449
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/lib/api/client.ts` around lines 40 - 41, Extract the ApiClient and ChatStreamHandlers type definitions into a new module (e.g., frontend/lib/api/client-types.ts) and move any related type imports (e.g., ChatStreamDelta, ChatStreamComplete, HealthResponse, AppStatusResponse, CaptureRequest/Response, ChatRequest/Response, etc.) into that file; then update demo-client.ts and client.ts to import those types via `import type { ApiClient, ChatStreamHandlers } from "./client-types"` so demo-client.ts no longer imports types from client.ts, eliminating the circular dependency while preserving type safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/app/page.tsx`:
- Around line 4-24: The current page component always renders a static landing
card with a Link to "/chat" and ignores STATIC_DEMO_MODE; update the component
(page.tsx) to conditionally redirect users to "/chat" when STATIC_DEMO_MODE is
false (or keep the landing page when true) by checking the STATIC_DEMO_MODE flag
at render/server-side and performing a Next.js redirect (or documented
intentional behavior) instead of always rendering the landing UI — locate the
Link with href="/chat" and the page export to add the conditional redirect logic
tied to STATIC_DEMO_MODE.
In `@frontend/next.config.ts`:
- Around line 6-8: turbopack.root currently uses process.cwd() which can be
incorrect in monorepos; change turbopack.root to derive an absolute path
relative to the config file instead (use __dirname and path.resolve with the
project folder or '.'), so update the turbopack config entry (symbol:
turbopack.root in next.config.ts) to compute a stable absolute path rather than
relying on process.cwd().
---
Nitpick comments:
In `@docs/portfolio-demo-netlify.md`:
- Around line 31-35: Add an explicit note to the demo Netlify docs stating the
behavior when NEXT_PUBLIC_DEMO_ACCESS_HASH is not set: explain that omitting
NEXT_PUBLIC_DEMO_ACCESS_HASH disables the passcode gate and grants immediate
access to all visitors, and place this note directly after the
environment-variable instruction (the block that lists
NEXT_PUBLIC_DEMO_ACCESS_HASH) so readers know the default behavior when the
variable is absent.
- Around line 73-78: The docs currently run "npm run build" twice: once before
setting demo env vars and again after setting NEXT_PUBLIC_SECOND_BRAIN_DEMO_MODE
and NEXT_PUBLIC_AGENTIC_RAG_ENABLED; either remove the first "npm run build" if
it is redundant, or keep it but add a short comment above the first "npm run
build" explaining its purpose (e.g., "verify standard build succeeds before
enabling demo env vars") so readers understand why the build is performed twice;
update the lines referencing the two "npm run build" commands and the demo env
vars (NEXT_PUBLIC_SECOND_BRAIN_DEMO_MODE, NEXT_PUBLIC_AGENTIC_RAG_ENABLED)
accordingly.
- Around line 96-97: The instruction "After Netlify deploys successfully, add
the Netlify URL to the portfolio as a live demo link" is ambiguous; update the
doc text that currently begins "After Netlify deploys successfully" to
explicitly state where to add the link: tell the reader to add the Netlify
live-demo URL to their personal portfolio site and to the project's README.md
under a clearly labeled "Live Demo" or "Deployment" section (keeping the
repository link immediately adjacent, e.g., "Live demo: <Netlify URL> (Repo:
<repo URL>)"), and mention to verify the live demo link is reachable after
deployment.
In `@frontend/lib/api/client.ts`:
- Around line 40-41: Extract the ApiClient and ChatStreamHandlers type
definitions into a new module (e.g., frontend/lib/api/client-types.ts) and move
any related type imports (e.g., ChatStreamDelta, ChatStreamComplete,
HealthResponse, AppStatusResponse, CaptureRequest/Response,
ChatRequest/Response, etc.) into that file; then update demo-client.ts and
client.ts to import those types via `import type { ApiClient, ChatStreamHandlers
} from "./client-types"` so demo-client.ts no longer imports types from
client.ts, eliminating the circular dependency while preserving type safety.
In `@frontend/lib/api/demo-client.ts`:
- Around line 328-344: The delay function uses window.setTimeout/clearTimeout
which breaks SSR; update delay(ms: number, signal?: AbortSignal) to use a
runtime-safe timer by switching window.setTimeout and window.clearTimeout to a
safe fallback (e.g., use globalThis.setTimeout/globalThis.clearTimeout or
(typeof window !== "undefined" ? window.setTimeout : setTimeout) and similarly
for clearTimeout) so timers still work in SSR, and keep the existing AbortSignal
handling and DOMException usage unchanged.
In `@netlify.toml`:
- Around line 15-19: Add a Content-Security-Policy header under the existing
[headers.values] block (where X-Frame-Options, X-Content-Type-Options,
Referrer-Policy, and Permissions-Policy are defined) by adding a
Content-Security-Policy entry that restricts resource origins (e.g., default-src
'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self'
'unsafe-inline'; img-src 'self' data: https:; font-src 'self' data:; connect-src
'self'), then adjust/remove 'unsafe-inline'/'unsafe-eval' as needed for your
Next.js/static export requirements and thoroughly test the app to ensure no
functionality breaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb12df22-bb51-4571-aed7-f9aa3aec2d4c
📒 Files selected for processing (16)
README.mddocs/PROGRESS.mddocs/implementation-notes.mddocs/portfolio-demo-netlify.mdfrontend/.env.examplefrontend/app/page.tsxfrontend/components/ConversationSidebar.tsxfrontend/components/DemoAccessGate.tsxfrontend/components/Providers.tsxfrontend/lib/api/client.tsfrontend/lib/api/demo-client.tsfrontend/lib/demo/config.tsfrontend/lib/demo/public-demo-data.tsfrontend/lib/demo/static-search.tsfrontend/next.config.tsnetlify.toml
Owner
Author
|
Moved this Netlify static portfolio demo into a separate repository for safer deployment isolation: https://github.com/tomnguyen103/second-brain-portfolio-demo. Closing this PR so the demo changes do not merge into the main second-brain repo. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Verification
Draft only: do not request CodeRabbit yet.
Summary by CodeRabbit
New Features
Documentation
Chores