Conversation
|
/bonk review please |
1 similar comment
|
/bonk review please |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for documenting real migration learnings — this kind of field-tested knowledge is genuinely useful for agents and humans alike. A few observations below, mostly about keeping the guidance general enough to age well and reducing duplication across the three files.
| 1. Auth/session behavior can regress even when pages appear to load. | ||
| - In request-wide auth guards (`proxy.ts` / middleware equivalents), treat vinext static asset paths (`/assets/*`) as internal like `/_next/*`. Redirecting these to sign-in breaks anonymous/public rendering. | ||
| - When calling auth APIs server-side, do not rely only on `headers()`. Merge cookie-store cookies into the request headers so RSC/server-action paths consistently see session cookies. | ||
| - For Better Auth + SIWE, use shared DB storage (not in-memory) and ensure schema exists (especially `verification`). Prefer explicit schema bootstrap scripts. |
There was a problem hiding this comment.
The Better Auth + SIWE guidance here is very specific to one authentication stack. Agents reading this skill will apply these instructions broadly, even when the project doesn't use Better Auth or SIWE.
Consider generalizing to something like: "For any server-side auth library that uses DB-backed sessions, ensure the backing store is persistent (not in-memory) and that all required schema tables exist before the first request." The Better Auth + SIWE specifics could live in troubleshooting.md under the new Auth section (where they already appear at line 44-47).
This would reduce duplication and keep the SKILL.md learnings stack-agnostic.
| - For Better Auth + SIWE, use shared DB storage (not in-memory) and ensure schema exists (especially `verification`). Prefer explicit schema bootstrap scripts. | ||
| 2. Avoid server actions for high-frequency client mutations. | ||
| - If a UI action creates many records (for example AI parsing into many day/category entries), use one route handler (`/api/...`) batch request instead of looped server actions. | ||
| - Reason: server actions trigger RSC replay and layout re-execution; with auth-protected layouts this can surface `NEXT_REDIRECT` noise and UX churn. |
There was a problem hiding this comment.
This is good practical advice, but framing it as a vinext-specific issue is slightly misleading — looped server actions causing NEXT_REDIRECT noise is a general React/Next.js architecture pattern, not something unique to vinext. It may be worth a short note like "This applies to Next.js as well, but surfaces more visibly during migration when layouts are restructured."
| - Excluding broad component libraries can cause ESM/CJS runtime mismatches (for example `react/jsx-runtime` named export errors in dev). | ||
| - Prefer default optimization unless there is a proven incompatibility. | ||
| 4. Route segment `error.tsx` can expose shim/runtime gaps. | ||
| - If adding segment error boundaries triggers runtime import errors from vinext shims, remove the segment boundary and use safer fallback handling until upstream fix is available. |
There was a problem hiding this comment.
"remove the segment boundary and use safer fallback handling" is a bit vague for an agent to act on. What does "safer fallback handling" mean concretely? A try/catch in the component? A top-level error boundary? Removing the route entirely?
Even a one-liner like "remove the error.tsx file and rely on the nearest parent error boundary or the root error boundary" would make this actionable.
| If you use request-wide auth (`proxy.ts`), treat `/assets/*` as internal/static just like `/_next/*`. | ||
|
|
||
| ```ts | ||
| function isInternalRequest(request: NextRequest): boolean { |
There was a problem hiding this comment.
The function accepts NextRequest but doesn't import it. Since this is example code that an agent might copy verbatim into a project, adding the import would prevent a follow-up error:
| function isInternalRequest(request: NextRequest): boolean { | |
| function isInternalRequest(request: Request): boolean { | |
| const pathname = new URL(request.url).pathname; | |
| if (pathname.startsWith("/_next/")) return true; | |
| if (pathname.startsWith("/assets/")) return true; // vinext static assets | |
| if (request.headers.has("rsc")) return true; | |
| if (request.headers.has("next-action")) return true; | |
| return false; | |
| } |
Alternatively, keep NextRequest but add import { NextRequest } from "next/server"; above the function. Using plain Request + new URL() is arguably more portable since this function doesn't use any NextRequest-specific APIs (.nextUrl is convenient but not required here).
|
|
||
| ```ts | ||
| // client | ||
| await fetchJson("/api/dashboard/work-blocks/batch", { |
There was a problem hiding this comment.
fetchJson isn't a standard browser/Node API — it's presumably a project-specific helper. An agent copying this example won't have it. Using fetch directly would be more universally applicable:
| await fetchJson("/api/dashboard/work-blocks/batch", { | |
| await fetch("/api/dashboard/work-blocks/batch", { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ entries }), | |
| }); |
|
|
||
| Do not broadly add libraries to `optimizeDeps.exclude` unless you have a verified reason. | ||
| This can trigger ESM/CJS runtime mismatches in dev. | ||
|
|
There was a problem hiding this comment.
Nit: there's an extra blank line here before ## VinextOptions. Minor, but the rest of the file uses single blank lines between sections.
| | `vinext: command not found` | vinext not installed or not in PATH | Install vinext: `npm install vinext`, then run via `npx vinext` or package.json scripts | | ||
| | RSC environment crash on dev start | Native Node module (sharp, satori) loaded in RSC env | vinext auto-stubs these in production; in dev, ensure these are only imported in server code behind dynamic `import()` | | ||
| | `ASSETS binding not found` | wrangler.jsonc missing assets config | Add `"assets": { "not_found_handling": "none" }` to wrangler.jsonc | | ||
| | `NEXT_REDIRECT:/auth/sign-in` after client mutation | Mutation implemented as server action triggers RSC replay + auth-protected layout re-exec | Move high-volume mutations to route handlers (`/api/...`) and batch writes in one request | |
There was a problem hiding this comment.
The three new table rows are solid additions. One small thing: the error string NEXT_REDIRECT:/auth/sign-in is very specific to one auth setup. Consider broadening the error column to NEXT_REDIRECT during server action or similar, so agents match on the pattern rather than the exact path.
| - For Better Auth with SIWE: | ||
| - use persistent/shared DB storage | ||
| - ensure schema bootstrap includes Better Auth tables (especially `verification`) | ||
| - avoid running migration introspection on every request; run explicit bootstrap migrations instead |
There was a problem hiding this comment.
This duplicates the Better Auth + SIWE guidance that's also in SKILL.md lines 144-145. Having it in one place (here in troubleshooting, since it's stack-specific) and referencing it from SKILL.md would be cleaner. Duplication in agent skill files is particularly costly because it inflates context without adding new information.
|
Review posted. Here's a summary of the feedback: Overall: Good addition — real-world migration learnings are valuable for the skill system. The main themes in my review are:
|
|
I'll open new one |
2 participants
Extending skills with real project problems me and my agent solved. No change in code, just the agentic knowledge.