Improve dev experience and CI for optimiser#4263
Merged
leslieyip02 merged 25 commits intonusmodifications:masterfrom Feb 21, 2026
Hidden character warning
The head ref may contain hidden characters: "chore/dev\u2013exp"
Merged
Improve dev experience and CI for optimiser#4263leslieyip02 merged 25 commits intonusmodifications:masterfrom
leslieyip02 merged 25 commits intonusmodifications:masterfrom
Conversation
- Added new global variable to assign the correct optimiser api
|
@thejus03 is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4263 +/- ##
==========================================
+ Coverage 54.52% 56.74% +2.22%
==========================================
Files 274 300 +26
Lines 6076 6957 +881
Branches 1455 1682 +227
==========================================
+ Hits 3313 3948 +635
- Misses 2763 3009 +246 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
thejus03
changed the title
…rithm and related functions
…ty and maintainability
thejus03
changed the title
leslieyip02
requested changes
Feb 18, 2026
.circleci/config.yml
Outdated
Comment on lines
142
to
146
| go run ./_test/server/main.go -port 8020 & | ||
| SERVER_PID=$! | ||
| sleep 5 | ||
| go test ./_test/. -v | ||
| kill $SERVER_PID |
Member
There was a problem hiding this comment.
- I'm not sure if
sleep 5is guaranteed to work. I think we can check if the port is open before running the tests by doing something like this:
for i in {1..10}; do
nc -z localhost 8020 && break
sleep 1
done
- Instead of
kill $SERVER_PID, maybe we can do
go run ./_test/server/main.go -port 8020 &
SERVER_PID=$!
trap "kill $SERVER_PID" EXIT
for i in {1..10}; do
nc -z localhost 8020 && break
sleep 1
done
go test ./_test/. -v
This ensures the server is always killed.
| // Algorithm: | ||
| // 1. Accumulate duration of consecutive classes (no gap between them) | ||
| // 2. When a gap is detected, check if accumulated time exceeds limit and penalize if so | ||
| // 3. After the loop, check the final consecutive block |
Member
There was a problem hiding this comment.
Nitpick but step 3 isn't accurate since the final check isn't "after the loop".
| t.Fatalf("Failed to marshal request: %v", err) | ||
| } | ||
|
|
||
| client := &http.Client{Timeout: 60 * time.Second} |
Member
There was a problem hiding this comment.
Should we reuse the client instead of recreating one for each request?
Comment on lines
228
to
229
| respBody, err := io.ReadAll(resp.Body) | ||
| resp.Body.Close() |
Member
There was a problem hiding this comment.
I think it could be better to do
defer resp.Body.Close()
respBody, err := io.ReadAll(resp.Body)
so that the body is closed even if io.ReadAll panics.
Member
|
@greptile |
Contributor
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| .circleci/config.yml | Adds Go executor and optimiser CI job with linting, formatting, and testing. Test server cleanup could be more robust with a trap. |
| website/api/optimiser/_solver/solver.go | Refactored scoreConsecutiveHoursofStudy with extracted helper function, improved import ordering, expanded comments, and formatting changes. Logic is correct. |
| website/api/optimiser/_test/api_test.go | New integration tests for the optimiser API covering single module, collision detection, and multi-module scenarios with validation helpers. |
| website/package.json | New OPTIMISER_API_URL env var and start:local script. Missing leading slash in start URL, and build script doesn't set the env var, which will break production builds. |
| website/src/apis/optimiser.ts | Changed from hardcoded API path to Webpack-injected global OPTIMISER_API_URL. Will break if the global is undefined in production or test environments. |
| website/src/types/global.d.ts | Added OPTIMISER_API_URL global type declaration as string (not `string |
| website/webpack/webpack.config.common.js | Added OPTIMISER_API_URL to DefinePlugin. When env var is unset, JSON.stringify(undefined) produces undefined, which will break at runtime. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["yarn start / start:local"] -->|"sets OPTIMISER_API_URL env var"| B["Webpack DefinePlugin"]
B -->|"JSON.stringify(env var)"| C["Global OPTIMISER_API_URL"]
C --> D["optimiser.ts: const api = OPTIMISER_API_URL"]
D --> E["axios.post(api, params)"]
F["yarn build (production)"] -->|"OPTIMISER_API_URL not set"| G["JSON.stringify(undefined)"]
G -->|"⚠️ undefined injected"| H["optimiser.ts: const api = undefined"]
H -->|"❌ Runtime error"| I["API call fails"]
J["yarn start:local"] -->|"starts Go test server"| K["localhost:8020/optimise"]
E -->|"local dev"| K
L["CI: optimiser job"] --> M["golangci-lint run"]
L --> N["golangci-lint fmt --diff"]
L --> O["go test ./_test/. -v"]
O -->|"requires"| P["go run _test/server/main.go"]
style F fill:#fff3cd
style G fill:#fff3cd
style H fill:#f8d7da
style I fill:#f8d7da
Last reviewed commit: 7a1ae79
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Additional Comments (2)
The new global Other globals like Prompt To Fix With AIThis is a comment left during a code review.
Path: website/jest.common.config.js
Line: 19-25
Comment:
**Missing `OPTIMISER_API_URL` in Jest globals**
The new global `OPTIMISER_API_URL` (added in `global.d.ts` and `webpack.config.common.js`) is not defined in Jest globals. Any test file that imports from `apis/optimiser.ts` will fail with a `ReferenceError: OPTIMISER_API_URL is not defined` because Jest doesn't use Webpack's `DefinePlugin`.
Other globals like `DATA_API_BASE_URL` are already defined here, so `OPTIMISER_API_URL` should be added for consistency:
```suggestion
globals: {
NUSMODS_ENV: 'test',
DATA_API_BASE_URL: '',
OPTIMISER_API_URL: '',
VERSION_STR: '',
DISPLAY_COMMIT_HASH: '',
DEBUG_SERVICE_WORKER: false,
},
```
How can I resolve this? If you propose a fix, please make it concise.
The new Prompt To Fix With AIThis is a comment left during a code review.
Path: website/src/.eslintrc.js
Line: 107-113
Comment:
**Missing `OPTIMISER_API_URL` in ESLint globals**
The new `OPTIMISER_API_URL` global needs to be added to the ESLint globals config, matching how the other Webpack `DefinePlugin` globals (`DATA_API_BASE_URL`, `VERSION_STR`, etc.) are declared. Without this, ESLint may report `OPTIMISER_API_URL` as an undefined variable in `optimiser.ts`.
```suggestion
globals: {
NUSMODS_ENV: 'readonly',
DATA_API_BASE_URL: 'readonly',
VERSION_STR: 'readonly',
DISPLAY_COMMIT_HASH: 'readonly',
DEBUG_SERVICE_WORKER: 'readonly',
OPTIMISER_API_URL: 'readonly',
},
```
How can I resolve this? If you propose a fix, please make it concise. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
2 participants
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.
Context
Implementation
OPTIMISER_API_URLhttp://localhost:8020/optimiseorapi/optimiser/optimiseyarn start:localfor running the localhost api endpoint on the websiteyarn startscript to include the real endpoint for productionUpdated on 21 Dec 2025
golangci-lintlibrary.