Repository navigation
Prepare HiveLore preview deployment - #32
KareemN1936 wants to merge 1 commit into
Conversation
|
Vercel preview is ready: https://hivelore-hiden7025-kareemn1936s-projects.vercel.app Commit: e26b8d5 |
mcfarhat
left a comment
There was a problem hiding this comment.
Requesting changes — but not on the code, which is clean. The problem is the branch this is aimed at.
The change itself builds and passes. I cloned it, installed, and ran the full set at e6b3c5c: typecheck pass, lint pass, format:check pass, build pass for both web and API. Consolidating client.ts onto env.ts is the right call — main genuinely had two different variable names in two files, and you fixed that. The CI placeholders and the URL validation are both improvements.
Blocking
This targets main, which is 89 commits behind develop
Your branch points at main and forks from 17ef470 (29 July). Every previous PR of yours went to develop, which is where the work actually is.
The consequence isn't stylistic. On this branch nothing imports the modules you changed:
$ git grep -n "lib/env\|lib/api/client" -- 'apps/web/src/**' # excluding the two files themselves
(no results)
Both lib/env.ts and lib/api/client.ts are orphaned here. So the rename and the fallback removal land on code nothing calls.
On develop those modules are very much alive — app/page.tsx imports env, and six modules under lib/api/ import apiClient. And develop still carries eleven NEXT_PUBLIC_API_URL references:
.env.example .github/workflows/ci.yml
README.md (x2) apps/web/.env.example
apps/web/README.md (x3) apps/web/src/lib/env.ts (x3)
So if this merges, the application people actually run is unchanged, and the next develop → main merge reintroduces the old name on top of it.
Three things that follow from the stale base
develop's env.ts exports a field this version doesn't. Develop has { apiBaseUrl, googleAuthEnabled }; yours has { apiBaseUrl }. If that merge is ever resolved in main's favour, env.googleAuthEnabled disappears silently.
The green tick on this PR is weaker than it looks. main's CI runs format, prisma, lint, typecheck, build. develop's CI additionally runs db:migrate:check:fresh, the migration-upgrade check, and npm test. No tests ran here — the root package.json on this branch has no test script at all:
$ npm test
npm ERR! Missing script: "test"
Dependency versions now diverge from develop in 15 packages — Prisma 7.10 vs 7.8, Fastify 5.12 vs 5.4, Next 15.5.25 vs 15.4.2, zod 4.6.5 vs 4.0.5, and others. Bumping them only on main means a 1,800-line lockfile conflict at the next merge, and leaves develop without the Next.js fix you describe as critical — which is the branch that needs it.
One thing to carry across when you re-target
Removing the localhost fallback does not fail the build. I checked:
$ env -u NEXT_PUBLIC_API_BASE_URL npm run build
rc=0
It passes here only because nothing imports env on this branch. On develop, app/page.tsx imports it at module scope, so a missing variable throws at runtime — the deploy succeeds and the page breaks for users. Dropping the fallback is still the right decision, but it needs the Vercel variable set beforehand, and it's worth considering an explicit build-time assertion so a missing value fails the deploy instead of the page.
What I'd like
Rebase onto develop and re-target the PR there. Everything you've done is wanted — the rename, the validation, the CI placeholders, the bumps. It just needs to be on the branch where the code lives.
For what it's worth, this is an easy trap rather than carelessness: main is the repository's default branch, so anything branching from the default lands on a July snapshot without any signal that it's stale. That's a repo problem and I'll deal with it separately — main being the default while develop is the real trunk is what made this possible.
Summary
Local verification
Known follow-up