Scheduled Merge: dev to prod - #1602
Open
github-actions[bot] wants to merge 51 commits into
Open
Conversation
This reverts commit 0c6a5a0.
This reverts commit 6e7700b.
This reverts commit 1b74a50.
… to optimize performance over
- vite.config.mjs: use classic JSX runtime so .js components qualify as Fast Refresh boundaries (plugin-react's automatic runtime only treats .jsx/.tsx as boundaries, so every edit was forcing a full page reload) - README: document the docker dev workflow, the VITE_ vs REACT_APP_ env var pitfall, and the DB_HOST/DB_PORT values needed for the docker network - restore .github/workflows/node.js.yml (deleted with no replacement), fixing two latent bugs: a matrix var name typo that broke node-version selection, and cd steps not persisting across separate run: blocks so builds silently ran in the repo root instead of client/server
…ckerterraform' into 1121-story-update-cd-to-fully-dockerterraform
deploy.yml and staging-deploy.yml export ENV_URL/STAGING_ENV_URL secrets before building the client, but a wrong-prefixed value (leftover REACT_APP_ from before the Vite migration) would silently leave VITE_SERVER_URL undefined and ship a client with no API URL. Now the build exits with a clear error instead.
Both Dockerfiles COPY . ./ after installing deps. client/.dockerignore didn't exclude .env, and server/.dockerignore didn't exist at all, so building locally (the now-documented dev workflow) baked DB passwords, OAuth secrets, and session keys into an image layer that outlives the container. Verified with real .env files present in the build context that neither image now contains .env or .git.
Nothing previously validated that client/Dockerfile, server/Dockerfile, or .devcontainer/docker-compose.yml still build on PRs that touch them. Runs docker compose build on the devcontainer stack; confirmed locally this needs no .env/secrets to succeed. Build-only by design — it won't catch runtime/env issues (e.g. the DB_HOST or VITE_SERVER_URL bugs found earlier this branch), only Dockerfile/dependency breakage.
- client/package-lock.json was missing the linux-x64-gnu optional rollup binary (npm/cli#4828 - lockfile only had darwin entries from being generated on macOS). This is what made the just-restored node.js.yml CI check fail immediately: `vite build` couldn't find @rollup/rollup-linux-x64-gnu on the Linux runner. Regenerated inside a node:20 (debian) container to match CI and confirmed `vite build` now succeeds there. - server/package-lock.json was stale relative to server/package.json, which already declares octokit and simple-git as dependencies the lockfile never picked up. `npm ci` would fail on this outright. Verified `npm i` + eslint both work cleanly against the regenerated lockfile.
Addresses github-code-quality bot findings: MainFooter no longer needs the class-component/redux imports (Component, connect, bindActionCreators, appActions, mainActions) or the unused editPage color helpers.
VersionService, lab15's Exercise/RepairService, and 4 spots in ImagineService were still reading process.env.REACT_APP_SERVER_URL, which Vite never defines in the browser bundle. Switched them to import.meta.env.VITE_SERVER_URL like the rest of the services.
CodeQL flagged the workflow for not declaring an explicit permissions block, leaving it on the default GITHUB_TOKEN scope. It only checks out code and builds images locally, so read-only contents access is all it needs.
…o-fully-dockerterraform
…ockerterraform Fully Update Development to Dev Containers
The 1121 Vite/Docker migration merge dropped the prod-client pm2 app from ecosystem.config.js and renamed client's npm "start" script away, breaking the SSH deploy script's pm2 restart on staging (ball.rit.edu). Restores prod-client, pointed at a new "start" script that runs Vite's dev server on port 3000 (matching the old CRA dev-server behavior this pm2 app relied on). Skips reintroducing configPath.js's Windows/npm path workaround, since staging and prod both run Ubuntu.
This reverts commit 99a4a24.
octokit v5 is ESM-only, but VersionService.js is a CommonJS module and was require()-ing it at the top level. app.js requires VersionService before starting the server, so this crashed the entire backend on boot (see prod-backend-error.log on staging). Loads Octokit via a lazy dynamic import() instead, the supported way to consume an ESM-only package from CommonJS.
Crashed prod-backend on boot: pg-connection-string's URL parser threw on the literal "undefined" port, since DB_PORT was newly read directly from env with no fallback (previously computed in code) and staging's environment was never updated to set it. Separately, the password var had been changed to read POSTGRES_PASSWORD, which the README (from this same migration) documents as the *Docker postgres image's own* init variable, distinct from the app-facing DB_PASS the connection string is supposed to use — reverted that back to DB_PASS. Adds a 5432 fallback for DB_PORT, matching the old hardcoded default for non-dev environments, so this doesn't require every deploy target to define it going forward.
app.js required '../server/services/VersionService', a path that only resolved by coincidence on bare-metal deploys (server/ sits one level under the repo root, so going up and back into a sibling server/ happens to land back in the same place). Inside the devcontainer, /app *is* the server directory directly, so there's no outer server/ to go back into and the require failed with MODULE_NOT_FOUND. Uses the plain relative path within the same directory instead.
server/eslint.config.mjs imports "@eslint/js" directly, but the lockfile only placed it nested under eslint's own node_modules (node_modules/eslint/node_modules/@eslint/js), never hoisted to the top level. Node's ESM resolution for a bare specifier from a sibling config file only checks the top level, so any fresh `npm install`/ `npm ci` in server/ left `eslint --fix` unable to resolve it, including through the lint-staged pre-commit hook. Adds @eslint/js as an explicit devDependency, which is what ESLint's own flat-config docs recommend for exactly this reason.
vite.config.mjs runs the classic JSX runtime (JSX compiles to
React.createElement calls), which requires React in scope wherever
JSX is used. 37 files across labs 12-15, the Imagine26 context, and
both the desktop and mobile footers either imported nothing from
react, only named hooks, or used the broken `import { React, ... }
from "react"` pattern (react has no named export called React, so it
silently resolved to undefined).
With no error boundary anywhere in the app, an uncaught render error
in any of these unmounts the entire React tree - mobileFooter.js and
versionComponent.js in particular render on every page, so this was a
site-wide crash for any user hitting them, not just a broken lab.
Every affected file now has a proper `import React` (default import),
alongside whatever named hooks/exports it already used.
@emotion/react and @emotion/styled are only reachable through a
lazily-loaded route (the DragIndicator icon pulled in by lab13's
ConfidenceRanking/DraggableCard). Vite only scans statically-reachable
imports at server startup, so it discovered these mid-session and
re-optimized them separately from the copy the browser already cached
from the initial scan, producing two module instances ("You are
loading @emotion/react when it is already loaded").
Adding them to optimizeDeps.include forces both into the initial
pre-bundle. Verified via the regenerated .vite/deps/_metadata.json,
which now includes both packages from the upfront scan.
The server returns an error sentinel string (e.g. "Error: Reading Not Found"/"Error: Reading Not Retrieved") instead of a reading object when labID doesn't resolve to a lab with content. Reading.js assumed data[0] was always a real object and crashed trying to read .piechart off of the resulting undefined (strings don't have a .reading property, so data[0].reading was undefined, and .piechart on that threw). Now checks data[0]?.reading up front and bails out with a logged error instead of crashing when the server didn't return real content.
…uiz routes
<Reading> was missing labID and isImagine (its sibling <About> routes
still had labID={lab}), so LabService.getLabReading was always called
with labID=undefined - the server then queried "WHERE id = 'undefined'",
which Postgres rejects, producing the "Error: Reading Not Retrieved"
sentinel that the previous commit stopped from crashing but didn't
explain.
Separately, renderLabs() was missing all 16 ExerciseLabN routes plus
Reinforcement and Quiz entirely (confirmed via eslint flagging all of
those imports and the quiz state vars as unused) - every lab's actual
exercise, reinforcement, and quiz page fell through to the <Error
default /> fallback instead of rendering. Restored all of them
matching the props from the last known-working version of this
branch. Verified live: /Lab1/Exercise, /Lab1/Reinforcement,
/Lab1/Quiz, and /Lab15/Exercise all render real content now.
Also dropped the now-unused ScrollWrapper import, which was blocking
the pre-commit lint gate.
The sweep that found the previous 37 files had two blind spots: it required a JSX tag's attributes to start on the same line as the tag (missed Lab15Context.js's multi-line <ExerciseStateContext.Provider>), and it only checked .js files (missed TicTacToeButton.jsx). A corrected, broader sweep across .js and .jsx found these plus UserPfp.js and RenderTextWithHighlight.js, which had no react import at all. Re-ran the sweep afterward across the whole tree - clean. Lab15Context.js was the actual reported crash (ExerciseStateProvider on /Lab15/Exercise); verified fixed live in the browser.
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.
Automated PR created by GitHub Actions on a semi-weekly schedule.