enhance(apps/frontend-pwa): add embedded mode for practice quiz - #4987
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 26381906 | Triggered | Generic Password | b5e1c7b | .factory/skills/agent-browser/templates/authenticated-session.sh | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
dosubot
Bot
added
the
size:XXL
📝 WalkthroughWalkthroughThis PR introduces comprehensive documentation for agent-browser and React best-practices skills, removes configuration entries for MCP servers and Claude settings, deletes in-file CLI instructions, updates a root-level AGENTS documentation file, and adds embedded mode support to practice quiz pages via optional props and conditional rendering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Pull request overview
This PR adds embedded mode support for practice quizzes to enable usage in e-learning environments without header, footer, and other decorations.
Changes:
- Added
embeddedquery parameter parsing (?embed=trueor?embed=1) in practice quiz pages - Conditionally hides Layout header, footer, and mobile menu when in embedded mode
- Adds custom completion handler that resets quiz instead of navigating away in embedded mode
- Added comprehensive documentation files (AGENTS.md, skill files, templates)
- Removed configuration files (.mcp.json, .claude/*)
Reviewed changes
Copilot reviewed 145 out of 145 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/frontend-pwa/src/pages/course/[courseId]/practiceQuizzes/[id].tsx | Adds embedded parameter parsing and passes it through the component tree |
| apps/frontend-pwa/src/components/practiceQuiz/PracticeQuiz.tsx | Adds onAllStacksCompletion prop with custom embedded mode handler |
| apps/frontend-pwa/src/components/Layout.tsx | Conditionally renders header, footer, and mobile menu based on embedded prop |
| AGENTS.md | Adds repository documentation for AI agents |
| .github/skills/* | Adds React best practices and web design guideline skill files |
| .factory/skills/* | Mirrors skill files from .github |
| .mcp.json, .claude/* | Removes configuration files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend-pwa/src/components/Layout.tsx (1)
53-56: Fix cross-origin iframe detection in two locations to prevent DOMException.Accessing
window.parent.locationthrows aDOMExceptionin cross-origin embeds, breaking the layout before render. The same unsafe pattern exists in bothLayout.tsx(lines 53–56) andHeader.tsx, and theembeddedprop does not prevent this becausepageInFrameis computed at the top level regardless.Use
window.self !== window.topwith a try/catch guard, which is safe to read cross-origin:🔧 Proposed fixes
Layout.tsx (lines 53–56):
- const pageInFrame = - global?.window && - global?.window?.location !== global?.window?.parent.location + const pageInFrame = (() => { + if (typeof window === 'undefined') return false + try { + return window.self !== window.top + } catch { + return true + } + })()Header.tsx (same pattern):
- const pageInFrame = - global?.window && - global?.window?.location !== global?.window?.parent.location + const pageInFrame = (() => { + if (typeof window === 'undefined') return false + try { + return window.self !== window.top + } catch { + return true + } + })()
🤖 Fix all issues with AI agents
In
@.factory/skills/vercel-react-best-practices/rules/advanced-event-handler-refs.md:
- Around line 40-55: Update the documentation snippet to note the minimum React
version required for useEffectEvent: add a short note near the alternative block
stating that useEffectEvent (used in the useWindowEvent example) requires React
19.2 or later so developers know it’s unavailable in earlier stable releases.
In @.github/skills/vercel-react-best-practices/rules/rendering-hoist-jsx.md:
- Around line 14-22: The example uses an undefined variable `loading` inside the
JSX of the Container component (Container and LoadingSkeleton are the relevant
symbols), so make `loading` explicit: either add a local state/constant (e.g.,
const loading = ... inside Container) or accept it as a prop (e.g., function
Container({ loading }) { ... }) and pass a boolean when rendering; update the
Container definition accordingly to remove the undefined reference.
- Around line 27-31: The example uses an undefined variable loading inside
Container, causing a reference error; define and initialize loading (e.g., a
boolean state or prop) or accept it as a prop on Container and use that value
when rendering loadingSkeleton; update the Container function signature or add a
useState/useEffect to provide a boolean loading variable and keep the hoisted
loadingSkeleton constant as-is so the conditional {loading && loadingSkeleton}
is valid.
- Around line 8-36: Update the "Hoist Static JSX Elements" guidance to call out
the practical limitations of the hoisted pattern (the hoisted element like
loadingSkeleton can only be in one place at a time, cannot accept props or be
customized, and can preserve state/refs unexpectedly across render locations)
and either restrict the recommendation to truly static, large elements (e.g.,
complex SVGs) or recommend using a memoized component (e.g., LoadingSkeleton
wrapped with React.memo) as the safer default; also clarify the React Compiler
note to state that when the React Compiler is enabled it will auto-hoist static
JSX so manual hoisting is typically unnecessary.
🟡 Minor comments (24)
.github/skills/vercel-react-best-practices/rules/client-localstorage-schema.md-72-72 (1)
72-72: Clarify private browsing behavior across browsers: The statement thatgetItem()andsetItem()throw in private browsing applies primarily to Safari—in Safari Private Browsing, writes commonly fail withQuotaExceededError. However, Firefox's private browsing does not throw on storage access; instead, Firefox keeps site data ephemeral (cleared when the private session ends). The try-catch wrapper remains good defensive practice, but the Safari-specific caveat should be emphasized rather than grouping Firefox with Safari. Quota exceeded and disabled storage contexts do still warrant error handling..github/skills/vercel-react-best-practices/rules/rendering-content-visibility.md-38-38 (1)
38-38: Soften the absolute performance claim.
Consider “can be significantly faster” or “often faster” instead of “10× faster” to avoid over-promising in general guidance..github/skills/vercel-react-best-practices/rules/js-cache-property-access.md-20-27 (1)
20-27: Add a note about mutability when caching.
Mention that caching is safe whenobj/obj.configis stable for the loop duration; otherwise it could read stale values..github/skills/vercel-react-best-practices/rules/advanced-use-latest.md-25-37 (1)
25-37: Add React version and linting requirements touseEffectEventexample.
The snippet should note thatuseEffectEventrequires React 19.2+ and eslint-plugin-react-hooks@>=6.1.1 for proper linting support. Without this context, readers on older React versions or with outdated linters may attempt to use the API unsuccessfully..factory/skills/vercel-react-best-practices/rules/rendering-activity.md-10-22 (1)
10-22: Add a React version requirement note for<Activity>.
The documentation should note that<Activity>is available in React 19.2+. The component is stable and documented in the official React API reference, so no stability caveats are needed—just clarify the version requirement to help developers understand when they can use it..factory/skills/vercel-react-best-practices/rules/server-serialization.md-14-23 (1)
14-23: Correct the 'use client' directive syntax in the example.Line 20 shows
('use client')as a standalone expression, which is invalid JavaScript syntax. The'use client'directive should be a plain string literal at the top of a module, not wrapped in parentheses or used as an expression.Additionally, the example appears to show both a Server Component and Client Component in the same code block, which might confuse readers since
'use client'applies to an entire module. Consider either:
- Clearly separating the files in the example with comments indicating different modules
- Showing just the component signatures without the
'use client'directive📝 Suggested clarification
async function Page() { const user = await fetchUser() // 50 fields return <Profile user={user} /> } -;('use client') +// In Profile.tsx: +'use client' + function Profile({ user }: { user: User }) { return <div>{user.name}</div> // uses 1 field }.github/skills/vercel-react-best-practices/rules/js-cache-storage.md-41-54 (1)
41-54: Cookie parsing is oversimplified and may fail with encoded values.The cookie parsing logic doesn't handle:
- URL-encoded cookie values
- Cookies containing
=in their value- Quoted cookie values
- Empty cookies
Consider using a more robust parser or documenting these limitations.
More robust cookie parsing
function getCookie(name: string) { if (!cookieCache) { cookieCache = Object.fromEntries( - document.cookie.split('; ').map((c) => c.split('=')) + document.cookie.split('; ').map((c) => { + const [key, ...rest] = c.split('=') + return [key, decodeURIComponent(rest.join('='))] + }) ) } return cookieCache[name] }.github/skills/vercel-react-best-practices/rules/advanced-init-once.md-1-42 (1)
1-42: Consolidate duplicate skill rule file.This file is identical to
.factory/skills/vercel-react-best-practices/rules/advanced-init-once.md. Remove the duplicate or establish a single source of truth to avoid maintenance drift. If both locations are intentionally maintained, set up a mechanism to keep them synchronized..github/skills/vercel-react-best-practices/rules/js-batch-dom-css.md-65-66 (1)
65-66: Use a heading instead of emphasis (Line 65).
Static analysis flags MD036 here; make this a real heading for consistency.✅ Suggested fix
-**Better: use CSS classes** +### Better: use CSS classes.github/skills/vercel-react-best-practices/rules/js-length-check-first.md-27-41 (1)
27-41: Specify runtime requirements or provide a fallback fortoSorted()(line 34).
toSorted()is ES2023 and supported only in Chrome/Edge 110+, Firefox 115+, Safari 16+, Node.js 20+, and Deno 1.31+. Older environments will encounter runtime errors. Either:
- Document the minimum runtime versions required, or
- Use a fallback:
arr.slice().sort()or a polyfill likecore-js/es/array/to-sortedor thearray.prototype.tosortedpackage..github/skills/vercel-react-best-practices/rules/advanced-event-handler-refs.md-40-55 (1)
40-55: Specify React 19.2+ requirement foruseEffectEvent(line 40).
This Hook became stable only in React 19.2 (October 2025); prior versions require the experimentalexperimental_useEffectEvent. Add a note that this pattern requires React 19.2 or later to prevent users on older versions from encountering compilation errors..factory/skills/vercel-react-best-practices/rules/rendering-content-visibility.md-10-38 (1)
10-38: Add browser support note to documentcontent-visibilitylimitations.This tip requires browser support documentation.
content-visibilityhas limited adoption: Chrome 85+, Firefox 125+ (disabled by default in earlier versions), and Safari 18.0+ only. Older Safari and Firefox versions lack support. Include a compatibility note or browser support table, and consider documenting fallback strategies for projects requiring broader browser coverage..factory/skills/vercel-react-best-practices/rules/advanced-use-latest.md-8-38 (1)
8-38: Add React version requirement.
useEffectEventbecame stable in React 19.2 (October 2025). Add a note specifying the minimum React version to avoid confusion for readers on older React releases:📌 Suggested doc tweak
-Access latest values in callbacks without adding them to dependency arrays. Prevents effect re-runs while avoiding stale closures. +Access latest values in callbacks without adding them to dependency arrays. Prevents effect re-runs while avoiding stale closures. + +*Requires React 19.2 or later. For older versions, use a useRef-based stable callback pattern instead.*.github/skills/vercel-react-best-practices/rules/server-auth-actions.md-8-12 (1)
8-12: Use a proper heading instead of emphasized text.
MD036 flags emphasis used as a heading. Prefer an actual heading level for consistency and tooling.✍️ Suggested fix
-**Impact: CRITICAL (prevents unauthorized access to server mutations)** +### Impact: CRITICAL (prevents unauthorized access to server mutations).factory/skills/vercel-react-best-practices/rules/server-auth-actions.md-8-12 (1)
8-12: Use a proper heading instead of emphasized text.
Consistent heading levels improve readability and linting.✍️ Suggested fix
-**Impact: CRITICAL (prevents unauthorized access to server mutations)** +### Impact: CRITICAL (prevents unauthorized access to server mutations).github/skills/vercel-react-best-practices/SKILL.md-124-127 (1)
124-127: Add language specifier to fenced code block.The code block lacks a language identifier, which triggers a markdownlint warning (MD040). Since this shows file paths, use
textorplaintext.Proposed fix
-``` +```text rules/async-parallel.md rules/bundle-barrel-imports.md ```.factory/skills/vercel-react-best-practices/rules/js-cache-storage.md-46-53 (1)
46-53: Cookie parsing doesn't handle values containing=characters.The
c.split('=')approach will incorrectly parse cookies where the value contains=(e.g.,token=abc=defbecomes['token', 'abc', 'def']). Consider using a limit parameter or splitting only on the first occurrence.Proposed fix
function getCookie(name: string) { if (!cookieCache) { cookieCache = Object.fromEntries( - document.cookie.split('; ').map((c) => c.split('=')) + document.cookie.split('; ').map((c) => { + const idx = c.indexOf('=') + return [c.slice(0, idx), c.slice(idx + 1)] + }) ) } return cookieCache[name] }.factory/skills/vercel-react-best-practices/rules/server-dedup-props.md-10-12 (1)
10-12: Use a heading instead of bold “Impact” (markdownlint MD036).✏️ Suggested fix
-**Impact: LOW (reduces network payload by avoiding duplicate serialization)** +### Impact: LOW (reduces network payload by avoiding duplicate serialization).factory/skills/vercel-react-best-practices/SKILL.md-124-127 (1)
124-127: Specify a language for the fenced code block (markdownlint MD040).✏️ Suggested fix
-``` +```text rules/async-parallel.md rules/bundle-barrel-imports.md</details> </blockquote></details> <details> <summary>.github/skills/vercel-react-best-practices/AGENTS.md-91-100 (1)</summary><blockquote> `91-100`: **Replace bold “Impact” labels with headings (markdownlint MD036).** There are many repeated bold “Impact” lines; switching them to headings will satisfy linting and improve structure. Example: <details> <summary>✏️ Example fix (apply consistently)</summary> ```diff -**Impact: CRITICAL** +#### Impact: CRITICAL.github/skills/agent-browser/templates/authenticated-session.sh-23-37 (1)
23-37: Browser session not closed when expired state is detected.When the saved state is loaded but found to be expired (Lines 35-37), the browser is left open before the script continues to discovery mode. The discovery mode then opens a new browser session.
🔧 Suggested fix
if [[ "$CURRENT_URL" != *"login"* ]] && [[ "$CURRENT_URL" != *"signin"* ]]; then echo "Session restored successfully!" agent-browser snapshot -i exit 0 fi echo "Session expired, performing fresh login..." + agent-browser close rm -f "$STATE_FILE" fi.factory/skills/agent-browser/references/snapshot-refs.md-11-23 (1)
11-23: Add language identifiers to fenced blocks (markdownlint MD040).Several fences are missing language tags; add
text,bash, orjsonas appropriate.🧹 Example fix (apply similarly to the other fences)
-``` +```text Full DOM/HTML sent → AI parses → Generates CSS selector → Executes action ~3000-5000 tokens per interaction```diff -``` +```text Page: Example Site - Home URL: https://example.com ...```diff -``` +```text `@e1` [tag type="value"] "text content" placeholder="hint" ...</details> Also applies to: 37-57, 136-159 </blockquote></details> <details> <summary>.factory/skills/agent-browser/references/proxy-support.md-3-3 (1)</summary><blockquote> `3-3`: **Hyphenate the compound adjective.** Minor grammar: “rate-limiting avoidance” reads better as a compound modifier. <details> <summary>✏️ Suggested edit</summary> ```diff -Configure proxy servers for browser automation, useful for geo-testing, rate limiting avoidance, and corporate environments. +Configure proxy servers for browser automation, useful for geo-testing, rate-limiting avoidance, and corporate environments..github/skills/agent-browser/references/snapshot-refs.md-11-24 (1)
11-24: Add languages to fenced code blocks (MD040).markdownlint flags these blocks for missing languages. Use
textfor the plain-text diagrams/output.🛠️ Suggested fix
-``` +```text Full DOM/HTML sent → AI parses → Generates CSS selector → Executes action ~3000-5000 tokens per interaction -``` +``` -``` +```text Compact snapshot → `@refs` assigned → Direct ref interaction ~200-400 tokens per interaction -``` +``` -``` +```text Page: Example Site - Home URL: https://example.com @@ `@e13` [footer] `@e14` [a] "Privacy Policy" -``` +``` -``` +```text `@e1` [tag type="value"] "text content" placeholder="hint" │ │ │ │ │ │ │ │ │ └─ Additional attributes │ │ │ └─ Visible text │ │ └─ Key attributes shown │ └─ HTML tag name └─ Unique ref ID -``` +``` -``` +```text `@e1` [button] "Submit" # Button with text `@e2` [input type="email"] # Email input `@e3` [input type="password"] # Password input `@e4` [a href="/page"] "Link Text" # Anchor link `@e5` [select] # Dropdown `@e6` [textarea] placeholder="Message" # Text area `@e7` [div class="modal"] # Container (when relevant) `@e8` [img alt="Logo"] # Image `@e9` [checkbox] checked # Checked checkbox `@e10` [radio] selected # Selected radio -``` +```Also applies to: 37-58, 136-159
🧹 Nitpick comments (28)
.github/skills/vercel-react-best-practices/rules/server-dedup-props.md (1)
10-10: Use a heading instead of bold for the section title.Line 10 triggers MD036; convert the emphasized line into a markdown heading to satisfy linting and improve structure.
♻️ Suggested change
-**Impact: LOW (reduces network payload by avoiding duplicate serialization)** +### Impact: LOW (reduces network payload by avoiding duplicate serialization).factory/skills/vercel-react-best-practices/rules/client-swr-dedup.md (1)
14-33: Make the examples self-contained and renderable.
Add a minimal return value (e.g.,return nullor a small JSX snippet) and definefetcheronce so readers can copy/paste without confusion.♻️ Suggested doc tweak
+const fetcher = (url: string) => fetch(url).then((r) => r.json()) function UserList() { const [users, setUsers] = useState([]) useEffect(() => { fetch('/api/users') .then((r) => r.json()) .then(setUsers) }, []) + return null }function UserList() { const { data: users } = useSWR('/api/users', fetcher) + return null }function StaticContent() { const { data } = useImmutableSWR('/api/config', fetcher) + return null }function UpdateButton() { const { trigger } = useSWRMutation('/api/user', updateUser) return <button onClick={() => trigger()}>Update</button> }Also applies to: 37-43, 47-53
.github/skills/vercel-react-best-practices/rules/client-swr-dedup.md (1)
14-33: Make the examples self-contained and renderable.
Add a minimal return value (e.g.,return nullor a small JSX snippet) and definefetcheronce so readers can copy/paste without confusion.♻️ Suggested doc tweak
+const fetcher = (url: string) => fetch(url).then((r) => r.json()) function UserList() { const [users, setUsers] = useState([]) useEffect(() => { fetch('/api/users') .then((r) => r.json()) .then(setUsers) }, []) + return null }function UserList() { const { data: users } = useSWR('/api/users', fetcher) + return null }function StaticContent() { const { data } = useImmutableSWR('/api/config', fetcher) + return null }function UpdateButton() { const { trigger } = useSWRMutation('/api/user', updateUser) return <button onClick={() => trigger()}>Update</button> }Also applies to: 37-43, 47-53
.factory/skills/vercel-react-best-practices/rules/client-event-listeners.md (2)
38-55: Stabilize callbacks to avoid re-registering on every render.Because
callbackis in the deps array, any render with a new callback identity will churn the Set; consider mentioninguseCallback/useEffectEvent(or a ref wrapper) in the docs to keep registrations stable.
57-65: Call out client-only usage when accessingwindow.This snippet should explicitly note that it must run in client components (e.g.,
"use client"in Next.js) to avoid SSR/runtime errors..github/skills/vercel-react-best-practices/rules/rerender-functional-setstate.md (1)
1-77: LGTM! Well-structured React best practices documentation.The documentation accurately covers the functional setState pattern with clear examples demonstrating both the problem (stale closures, unnecessary recreations) and the solution (functional updates with stable callbacks). The guidance on when to use functional updates versus direct updates is practical and aligns with React best practices.
💡 Optional: Consider mentioning useReducer for complex state logic
For completeness, you could add a brief note that for more complex state update logic involving multiple related state values,
useReducercan be a cleaner alternative to nested functional setState calls. However, this is entirely optional as the current focus on functional setState is clear and appropriate..factory/skills/vercel-react-best-practices/rules/rerender-derived-state.md (1)
14-29: Clarify custom hook sources or provide implementation references.The examples reference
useWindowWidthanduseMediaQueryhooks without imports or definitions. Developers following this guide may be confused about where these hooks come from.💡 Suggested clarification
Consider adding a note about the custom hook sources, for example:
**Incorrect (re-renders on every pixel change):** ```tsx +// Note: useWindowWidth is a custom hook that listens to window resize events function Sidebar() { const width = useWindowWidth() // updates continuouslyOr reference a library like
react-responsiveforuseMediaQuery:**Correct (re-renders only when boolean changes):** ```tsx +import { useMediaQuery } from 'react-responsive' +// or use a custom implementation + function Sidebar() { const isMobile = useMediaQuery('(max-width: 767px)').factory/skills/vercel-react-best-practices/rules/async-api-routes.md (1)
38-38: Provide context or link forbetter-allreference.Line 38 mentions
better-alland "Dependency-Based Parallelization" without providing context, import details, or a link to the referenced documentation.💡 Suggested improvement
Add a brief explanation or link:
-For operations with more complex dependency chains, use `better-all` to automatically maximize parallelism (see Dependency-Based Parallelization). +For operations with more complex dependency chains, use the [`better-all`](https://www.npmjs.com/package/better-all) library to automatically maximize parallelism (see [Dependency-Based Parallelization](./async-dependencies.md)).Or if the cross-reference is intentional without a link:
-For operations with more complex dependency chains, use `better-all` to automatically maximize parallelism (see Dependency-Based Parallelization). +For operations with more complex dependency chains, consider using the `better-all` npm package to automatically maximize parallelism. See the related "Dependency-Based Parallelization" guide for details..github/skills/vercel-react-best-practices/rules/rerender-move-effect-to-event.md (1)
1-45: Confirm duplication is intentional.
This rule appears duplicated under both.github/skills/...and.factory/skills/.... If that’s intentional, all good; if not, consider sourcing from a single canonical file to avoid divergence..factory/skills/vercel-react-best-practices/rules/js-set-map-lookups.md (1)
10-24: Optional nuance: mention Set overhead for tiny lists.
A short note that Sets shine for repeated checks or larger lists would help readers choose appropriately..factory/skills/vercel-react-best-practices/rules/advanced-init-once.md (1)
25-40: Consider recommending module-level initialization over the guard pattern.While the
didInitguard prevents duplicate initialization, the pattern still runs initialization inside a component effect. For true app-wide initialization, consider recommending module-level execution instead:// Run once at module load loadFromStorage() checkAuthToken() function Comp() { // No effect needed }This eliminates the component lifecycle dependency entirely and avoids potential race conditions if multiple instances mount simultaneously.
.github/skills/vercel-react-best-practices/rules/js-cache-storage.md (1)
56-70: Clarify thatstorageevent only fires for cross-tab changes.The
storageevent listener only fires when storage is modified by another tab/window, not the current tab. Same-tab changes should invalidate viasetLocalStorage()directly. This behavior should be documented to avoid confusion..github/skills/vercel-react-best-practices/rules/advanced-init-once.md (1)
25-40: Consider recommending module-level initialization over the guard pattern.While the
didInitguard prevents duplicate initialization, the pattern still runs initialization inside a component effect. For true app-wide initialization, consider recommending module-level execution instead:// Run once at module load loadFromStorage() checkAuthToken() function Comp() { // No effect needed }This eliminates the component lifecycle dependency entirely and avoids potential race conditions if multiple instances mount simultaneously.
.github/skills/vercel-react-best-practices/rules/async-parallel.md (1)
20-28: Clarify "1 round trip" terminology.The comment "parallel execution, 1 round trip" might be misleading. This still makes 3 separate requests, just concurrently rather than sequentially. Consider:
// Correct (parallel execution, concurrent requests): const [user, posts, comments] = await Promise.all([ fetchUser(), fetchPosts(), fetchComments(), ])This clarifies that requests run in parallel without implying they're combined into a single network call.
.factory/skills/vercel-react-best-practices/rules/rerender-functional-setstate.md (1)
59-62: Rephrase “reduces memory leaks” to avoid a misleading claim.Functional updates stabilize callbacks and dependencies but don’t directly prevent memory leaks. Consider softening this to “reduces dependency churn” or similar.
✏️ Suggested wording tweak
-3. **Fewer dependencies** - Simplifies dependency arrays and reduces memory leaks +3. **Fewer dependencies** - Simplifies dependency arrays and reduces dependency churn.factory/skills/vercel-react-best-practices/rules/js-min-max-loop.md (1)
82-82: Minor wording polish.Consider replacing “very large” with “large” (or “extremely large”) per the style hint.
✏️ Suggested wording tweak
-This works for small arrays, but can be slower or just throw an error for very large arrays due to spread operator limitations. +This works for small arrays, but can be slower or just throw an error for large arrays due to spread operator limitations..factory/skills/vercel-react-best-practices/rules/rendering-hydration-no-flicker.md (1)
45-70: Add CSP caveat for inline scripts.Inline scripts may be blocked under strict CSP. A short note about adding a nonce or CSP allowance would prevent confusion.
📌 Suggested doc tweak
-The inline script executes synchronously before showing the element, ensuring the DOM already has the correct value. No flickering, no hydration mismatch. +The inline script executes synchronously before showing the element, ensuring the DOM already has the correct value. No flickering, no hydration mismatch. +If your app enforces a strict CSP, add a nonce (or allow this inline script) so it can execute..github/skills/vercel-react-best-practices/rules/rerender-dependencies.md (1)
31-44: Consider adding context for the width variable.The derived state example is excellent, but it doesn't show where
widthcomes from (e.g.,useState,useWindowWidthhook, etc.). Adding a brief comment or preceding line would make the example more complete for readers.✨ Suggested enhancement
// Incorrect: runs on width=767, 766, 765... +const [width, setWidth] = useState(window.innerWidth) useEffect(() => { if (width < 768) { enableMobileMode() } }, [width]) // Correct: runs only on boolean transition +const [width, setWidth] = useState(window.innerWidth) const isMobile = width < 768 useEffect(() => { if (isMobile) { enableMobileMode() } }, [isMobile]).factory/skills/vercel-react-best-practices/rules/rerender-transitions.md (1)
12-23: The "incorrect" example isn't actually broken.The scroll handler without
startTransitionisn't technically incorrect—it won't block the UI in practice for simple scroll position tracking. The example might benefit from clarifying that transitions are most valuable when the state update triggers expensive re-renders downstream (e.g., filtering a large list based on scroll position).💡 Consider enhancing the example
Adding a comment or using a more compelling use case would strengthen the guidance:
// When scroll position triggers expensive computations: function ScrollTracker() { const [scrollY, setScrollY] = useState(0) useEffect(() => { const handler = () => { startTransition(() => setScrollY(window.scrollY)) } window.addEventListener('scroll', handler, { passive: true }) return () => window.removeEventListener('scroll', handler) }, []) // scrollY is used to filter/sort a large list const visibleItems = useMemo(() => items.filter(item => item.position < scrollY + 1000) , [scrollY, items]) }.factory/skills/vercel-react-best-practices/rules/js-batch-dom-css.md (1)
65-65: Use a proper heading instead of bold text.The markdown uses bold emphasis for "Better: use CSS classes" but it should be a proper heading for better document structure and accessibility.
📝 Suggested fix
-**Better: use CSS classes** +### Better: use CSS classesBased on static analysis hints.
.github/skills/vercel-react-best-practices/rules/rerender-defer-reads.md (1)
8-39: Consider mentioning the reactivity trade-off.The guidance is correct for avoiding unnecessary subscriptions, but it's worth noting that the direct
URLSearchParamsapproach won't trigger component re-renders if the URL changes elsewhere. This is the intended behavior for callback-only usage, but adding a brief note about this trade-off would make the guidance more complete.💡 Suggested enhancement
Add a note after the correct example:
**Correct (reads on demand, no subscription):** ```tsx function ShareButton({ chatId }: { chatId: string }) { const handleShare = () => { const params = new URLSearchParams(window.location.search) const ref = params.get('ref') shareChat(chatId, { ref }) } return <button onClick={handleShare}>Share</button> }Note: This pattern is appropriate when you only need the value inside callbacks and don't need the component to re-render when the URL changes. If you need reactivity to URL changes, use
useSearchParams().</details> </blockquote></details> <details> <summary>.factory/skills/vercel-react-best-practices/rules/rerender-memo-with-default-value.md (1)</summary><blockquote> `10-23`: **Consider adding examples for arrays and objects.** The description mentions arrays, functions, and objects (Line 10), but only the function case is demonstrated. Adding brief examples for array and object defaults would make the guidance more comprehensive. <details> <summary>📚 Suggested enhancement</summary> After the function example, add: ```markdown **Also applies to arrays and objects:** ```tsx // Incorrect const UserList = memo(function UserList({ items = [], // New array on every render config = {} // New object on every render }: { items?: User[] config?: Config }) { // ... }) // Correct const EMPTY_ARRAY: User[] = [] const DEFAULT_CONFIG: Config = {} const UserList = memo(function UserList({ items = EMPTY_ARRAY, config = DEFAULT_CONFIG }: { items?: User[] config?: Config }) { // ... })</details> </blockquote></details> <details> <summary>.factory/skills/vercel-react-best-practices/rules/async-suspense-boundaries.md (1)</summary><blockquote> `79-87`: **Add React version requirement note for the `use()` hook example.** The `use()` hook is a React 19 feature and should be documented as such in best-practices guidance that may span multiple React versions. Add a version note to clarify this requirement for developers using earlier versions. <details> <summary>📝 Suggested enhancement</summary> Add a note before the alternative pattern code block: ```markdown **Alternative (share promise across components, React 19+):** > **Note:** This pattern uses the `use()` hook, which requires React 19 or later.Or inline in the function comments:
function DataDisplay({ dataPromise }: { dataPromise: Promise<Data> }) { const data = use(dataPromise) // React 19: Unwraps the promise return <div>{data.content}</div> }.github/skills/web-design-guidelines/SKILL.md (1)
25-27: Add language identifier to fenced code block.The fenced code block containing the URL should specify a language identifier for proper markdown formatting.
📝 Proposed fix
-``` +```text https://raw.githubusercontent.com/vercel-labs/web-interface-guidelines/main/command.md</details> Based on static analysis hints. </blockquote></details> <details> <summary>.factory/skills/vercel-react-best-practices/AGENTS.md (1)</summary><blockquote> `704-709`: **Unusual semicolon-prefixed `'use client'` syntax.** The semicolon before `'use client'` (Lines 707, 799, 813) is unconventional and may confuse readers. This appears to be an artifact of how the code blocks were formatted. <details> <summary>📝 Suggested fix</summary> ```diff -;('use client') +// Client component +'use client'.github/skills/agent-browser/templates/authenticated-session.sh (1)
29-34: URL pattern check may produce false positives.The check
[[ "$CURRENT_URL" != *"login"* ]]could match URLs that coincidentally contain "login" in other contexts (e.g.,/user/loginhistory,/blog/login-security-tips). Consider using a more specific pattern or checking the path component..github/skills/agent-browser/references/proxy-support.md (1)
71-82:grep -P(Perl regex) may not work on all systems.The
-Pflag for Perl-compatible regular expressions is not available in BSD grep (default on macOS). Consider usingsedor a POSIX-compatible pattern for better portability.📝 More portable alternative
- region=$(echo "$proxy" | grep -oP '^\w+-\w+') + region=$(echo "$proxy" | sed -E 's|^http://([a-z]+-[a-z]+).*|\1|').factory/skills/agent-browser/templates/capture-workflow.sh (1)
47-49: Make “optional PDF” truly optional (or relabel).The comment says optional, but the command always runs. Consider gating it behind a flag or commenting it out by default.
💡 One way to gate the PDF step
-# Optional: Save as PDF -agent-browser pdf "$OUTPUT_DIR/page.pdf" -echo "PDF saved: $OUTPUT_DIR/page.pdf" +# Optional: Save as PDF (set SAVE_PDF=1 to enable) +if [[ "${SAVE_PDF:-0}" == "1" ]]; then + agent-browser pdf "$OUTPUT_DIR/page.pdf" + echo "PDF saved: $OUTPUT_DIR/page.pdf" +fi
| **Alternative: use `useEffectEvent` if you're on latest React:** | ||
|
|
||
| ```tsx | ||
| import { useEffectEvent } from 'react' | ||
|
|
||
| function useWindowEvent(event: string, handler: (e) => void) { | ||
| const onEvent = useEffectEvent(handler) | ||
|
|
||
| useEffect(() => { | ||
| window.addEventListener(event, onEvent) | ||
| return () => window.removeEventListener(event, onEvent) | ||
| }, [event]) | ||
| } | ||
| ``` | ||
|
|
||
| `useEffectEvent` provides a cleaner API for the same pattern: it creates a stable function reference that always calls the latest version of the handler. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
React useEffectEvent version introduced stable experimental
💡 Result:
- Experimental (not in any stable release):
useEffectEventoriginally shipped only behind the experimental channel asexperimental_useEffectEvent, usable viareact@experimental/react-dom@experimental. The official docs explicitly say it’s “not available in a stable version of React yet” on that experimental API page. [1] - Stable (first stable release):
useEffectEventbecame a stable React API in React 19.2, where it’s listed as a new React feature in the React 19.2 release post (dated October 1, 2025). [2]
Answer: Experimental: react@experimental only; Stable: React 19.2. [1][2]
Add minimum React version requirement for useEffectEvent (Line 40).
The useEffectEvent API requires React 19.2 or later and is not available in earlier stable versions. Update the documentation to note: "Note: useEffectEvent requires React 19.2 or later" so developers don't attempt to use this pattern in unsupported environments.
🤖 Prompt for AI Agents
In
@.factory/skills/vercel-react-best-practices/rules/advanced-event-handler-refs.md
around lines 40 - 55, Update the documentation snippet to note the minimum React
version required for useEffectEvent: add a short note near the alternative block
stating that useEffectEvent (used in the useWindowEvent example) requires React
19.2 or later so developers know it’s unavailable in earlier stable releases.
| ## Hoist Static JSX Elements | ||
|
|
||
| Extract static JSX outside components to avoid re-creation. | ||
|
|
||
| **Incorrect (recreates element every render):** | ||
|
|
||
| ```tsx | ||
| function LoadingSkeleton() { | ||
| return <div className="h-20 animate-pulse bg-gray-200" /> | ||
| } | ||
|
|
||
| function Container() { | ||
| return <div>{loading && <LoadingSkeleton />}</div> | ||
| } | ||
| ``` | ||
|
|
||
| **Correct (reuses same element):** | ||
|
|
||
| ```tsx | ||
| const loadingSkeleton = <div className="h-20 animate-pulse bg-gray-200" /> | ||
|
|
||
| function Container() { | ||
| return <div>{loading && loadingSkeleton}</div> | ||
| } | ||
| ``` | ||
|
|
||
| This is especially helpful for large and static SVG nodes, which can be expensive to recreate on every render. | ||
|
|
||
| **Note:** If your project has [React Compiler](https://react.dev/learn/react-compiler) enabled, the compiler automatically hoists static JSX elements and optimizes component re-renders, making manual hoisting unnecessary. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reconsider the pattern recommendation or document limitations.
The "correct" hoisted JSX pattern has significant limitations not mentioned in the documentation:
- The element can only be used in one location in the tree at a time
- Cannot accept props for customization
- May cause unexpected state/ref preservation across render locations
Combined with the note that React Compiler makes this optimization unnecessary, this pattern may cause more problems than it solves for most use cases. Consider either:
- Documenting these limitations clearly
- Restricting the recommendation to truly static, large elements (like complex SVGs)
- Recommending React.memo for the component approach instead
🤖 Prompt for AI Agents
In @.github/skills/vercel-react-best-practices/rules/rendering-hoist-jsx.md
around lines 8 - 36, Update the "Hoist Static JSX Elements" guidance to call out
the practical limitations of the hoisted pattern (the hoisted element like
loadingSkeleton can only be in one place at a time, cannot accept props or be
customized, and can preserve state/refs unexpectedly across render locations)
and either restrict the recommendation to truly static, large elements (e.g.,
complex SVGs) or recommend using a memoized component (e.g., LoadingSkeleton
wrapped with React.memo) as the safer default; also clarify the React Compiler
note to state that when the React Compiler is enabled it will auto-hoist static
JSX so manual hoisting is typically unnecessary.
| ```tsx | ||
| function LoadingSkeleton() { | ||
| return <div className="h-20 animate-pulse bg-gray-200" /> | ||
| } | ||
|
|
||
| function Container() { | ||
| return <div>{loading && <LoadingSkeleton />}</div> | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Fix undefined variable reference.
The example references an undefined variable loading on line 20, making this code non-functional.
🐛 Proposed fix
function LoadingSkeleton() {
return <div className="h-20 animate-pulse bg-gray-200" />
}
-function Container() {
+function Container({ loading }: { loading: boolean }) {
return <div>{loading && <LoadingSkeleton />}</div>
}🤖 Prompt for AI Agents
In @.github/skills/vercel-react-best-practices/rules/rendering-hoist-jsx.md
around lines 14 - 22, The example uses an undefined variable `loading` inside
the JSX of the Container component (Container and LoadingSkeleton are the
relevant symbols), so make `loading` explicit: either add a local state/constant
(e.g., const loading = ... inside Container) or accept it as a prop (e.g.,
function Container({ loading }) { ... }) and pass a boolean when rendering;
update the Container definition accordingly to remove the undefined reference.
| const loadingSkeleton = <div className="h-20 animate-pulse bg-gray-200" /> | ||
|
|
||
| function Container() { | ||
| return <div>{loading && loadingSkeleton}</div> | ||
| } |
There was a problem hiding this comment.
Fix undefined variable reference.
The example references an undefined variable loading on line 30, making this code non-functional.
🐛 Proposed fix
const loadingSkeleton = <div className="h-20 animate-pulse bg-gray-200" />
-function Container() {
+function Container({ loading }: { loading: boolean }) {
return <div>{loading && loadingSkeleton}</div>
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const loadingSkeleton = <div className="h-20 animate-pulse bg-gray-200" /> | |
| function Container() { | |
| return <div>{loading && loadingSkeleton}</div> | |
| } | |
| const loadingSkeleton = <div className="h-20 animate-pulse bg-gray-200" /> | |
| function Container({ loading }: { loading: boolean }) { | |
| return <div>{loading && loadingSkeleton}</div> | |
| } |
🤖 Prompt for AI Agents
In @.github/skills/vercel-react-best-practices/rules/rendering-hoist-jsx.md
around lines 27 - 31, The example uses an undefined variable loading inside
Container, causing a reference error; define and initialize loading (e.g., a
boolean state or prop) or accept it as a prop on Container and use that value
when rendering loadingSkeleton; update the Container function signature or add a
useState/useEffect to provide a boolean loading variable and keep the hoisted
loadingSkeleton constant as-is so the conditional {loading && loadingSkeleton}
is valid.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
embed-pq
|
| Run status |
|
| Run duration | 09m 24s |
| Commit |
|
| Committer | Roland Schläfli |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
2
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
756
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/Q-practice-quiz-workflow.cy.ts • 2 failed tests
For usage in our elearning, we want to embed PQs without the header/footer/other decorations. This PR also adds agent skills (skills.sh) and AGENTS.md for usage with droid.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.