Observatory Next.js rewrite #4989
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: debe6f88f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function proxy(request: NextRequest) { | ||
| const { pathname } = request.nextUrl | ||
|
|
||
| if (PUBLIC_PATHS.some((p) => pathname.startsWith(p))) { | ||
| return NextResponse.next() |
There was a problem hiding this comment.
This looks like the login redirect middleware, but Next only executes middleware from a middleware.ts entrypoint (or an explicit middleware export), and this file isn’t referenced elsewhere. As written, the redirect logic never runs, so unauthenticated requests won’t be sent to the auth server and will instead fall through to API calls without a token, leaving new/expired sessions stuck on the error screen instead of entering the /auth/callback flow.
Useful? React with 👍 / 👎.
| try { | ||
| const userInfo = await repo.whoami() | ||
| currentUser = userInfo.user_email | ||
| } catch (err: any) { |
There was a problem hiding this comment.
If repo.whoami() gets a 401/404, Repo.handleErrorResponse throws redirect()/notFound(), but this catch intercepts those exceptions and renders the “Unable to connect” screen instead. That means expired or invalid tokens won’t trigger a re-login (or a 404), so users get stuck on the error view rather than being redirected. Consider rethrowing Next’s redirect/notFound errors or narrowing this catch to genuine network failures.
Useful? React with 👍 / 👎.
| export const config: Config = { | ||
| apiBaseUrl: import.meta.env.VITE_API_URL || 'http://localhost:8000', | ||
| authToken: import.meta.env.VITE_AUTH_TOKEN, | ||
| authServerUrl: import.meta.env.VITE_AUTH_SERVER_URL || 'https://softmax.com/api', | ||
| apiBaseUrl: process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8000', | ||
| authToken: process.env.NEXT_PUBLIC_AUTH_TOKEN, // set in dev mode for convenience based on ~/.metta/config.yaml token |
There was a problem hiding this comment.
This config is imported by client components (via AppContext), and NEXT_PUBLIC_* values are inlined at next build time. The Docker build doesn’t set NEXT_PUBLIC_API_URL and .env.production was removed, so the client bundle will hardcode the fallback http://localhost:8000; in production, any client-side API calls (SQL query, smart plugs, tasks) will hit localhost even though Helm sets the env at runtime. You likely need a build-time env or runtime config injection for client code.
Useful? React with 👍 / 👎.
Merge activity
|
- RSC instead of loading states - cookie (not HttpOnly yet) instead of localStorage - more persistent urls - various UI cleanups; in particular, pagination for /policies, sub-pages under /tournament/beta, etc. I'm less happy with this PR than I expected to be, because LLMs have a pretty strong bias in favor of client-side components, so I expect it to drift back in the direction of custom API routes and `useEffect` loaders. I'm not even sure I should fight this - not worth it if we can't convince LLMs to use RSCs, but maybe we can; I plan to experiment with [next-devtools-mcp](https://nextjs.org/docs/app/guides/mcp) later. But at least it's a negative diff, and I do think having Next.js with the ability to set a cookie instead of local storage is a good thing for security.
I'm less happy with this PR than I expected to be, because LLMs have a pretty strong bias in favor of client-side components, so I expect it to drift back in the direction of custom API routes and
useEffectloaders. I'm not even sure I should fight this - not worth it if we can't convince LLMs to use RSCs, but maybe we can; I plan to experiment with next-devtools-mcp later.But at least it's a negative diff, and I do think having Next.js with the ability to set a cookie instead of local storage is a good thing for security.