Repository navigation
Conversation
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
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.
Description of Change
Documents four places where the README doesn't match the actual behavior
of the repo.
npm run buildis required beforenpm test. The error-pagetests assert that styles are inlined, which needs the compiled
public/css/errors.cssproduced bynpm run build:inline. CI runsnpm ci→npm run build→npm test, but the README's Testssection only mentions
npm test. On a fresh clone that meanstest/functional/pages.test.jsfails with no explanation of why.The Node version note was out of date. It said "Node v8 or
higher."
package.jsonsetsengines.nodeto>=18.x, and the repoalso pins
v24.4.1in.nvmrc, which the README never mentioned.Now points at
.nvmrcinstead of a hardcoded number so it can'tdrift again.
Eleven environment variables read by the server were
undocumented. Added a Configuration table covering
PORT,TRUST_PROXY,PATH_PREFIX,DRIVE_TIMEOUT_SECONDS,LIST_UPDATE_DELAY,EDIT_CACHE_DELAY,ALLOW_INLINE_CODE,REDIRECT_URL,TEST_EMAIL,GOOGLE_APPLICATION_CREDENTIALS, andLOG_LEVEL, with defaults read from the source.Corrected the drive refresh interval. The App structure section
said the tree is repopulated "on an interval (currently 60s)."
server/list.jsusesLIST_UPDATE_DELAY || 15, and the commentdirectly above it says 15s. Updated to match and to name the
environment variable.
The new section was added to the doctoc TOC by hand. I didn't re-run
doctoc, because the current version also rewrites the "Using Docker"
entry (the badge image in that heading confuses it) and drops a blank
line, which would have added unrelated churn to this diff.
Related Issue
None. The
LOG_LEVELrow notes that the value is currently ignored andlinks #379, which already fixes it.
Motivation and Context
Reproduced on a clean clone at 0c63f81, Node v24.4.1 (matching
.nvmrc), macOS.Following the README exactly (
npm install --no-optional, thennpm test):After
npm run build:(The 1 skipped suite is the intentionally disabled
test/functional/playlists.test.js, unrelated to this change.)Checklist
npm run lintand updated code style accordinglynpm run testpassestests are updated and/or added to cover new code— documentation only, no code changes