Conversation
- Updated base Node.js version from 22 to 24 in Dockerfile.node.base.dockerfile and related build scripts. - Changed Node.js version in pnpm-workspace.yaml files to 24.14.0 for compatibility. - Updated Rust toolchain version from 1.92 to 1.93 in rust-toolchain.toml and Cargo.toml files
github-actions
bot
added
dependencies
github-actions
bot
added
headless-lms
cms
example-exercise
main-frontend
tmc
root
deployments
scripts
labels
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated Node base images to Node 24, bumped pnpm/workspace and many JS/Rust dependencies, updated Rust toolchain to 1.93, refactored headless-lms chapter‑locking and RNG usage, expanded Gutenberg block attributes/guards, adjusted type import/generation scripts, and added/updated Playwright system tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Client"
participant Controller as "API Controller"
participant Model as "user_chapter_locking_statuses\n(Model)"
participant DB as "Database"
Client->>Controller: GET /courses/:id/locking-statuses
Controller->>Model: get_or_init_all_for_course(conn, user_id, course_id)
Model->>DB: query course locking_enabled
DB-->>Model: course_locking_enabled
alt locking enabled and no statuses
Model->>DB: insert NotUnlockedYet rows for chapters
DB-->>Model: inserted rows
Model->>DB: fetch statuses for user & course
else statuses exist
Model->>DB: fetch statuses for user & course
end
Model-->>Controller: Vec<UserChapterLockingStatus>
Controller-->>Client: 200 OK + statuses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
system-tests/src/tests/mark-as-spam.spec.ts (1)
76-77:⚠️ Potential issue | 🔴 CriticalMissing
awaiton click action causes race condition.The
.click()on line 76 is not awaited, so theexpecton line 77 may execute before the click completes, causing flaky test behavior.🐛 Proposed fix
- student2Page.getByLabel("Report Answer").getByRole("button", { name: "Submit" }).click() + await student2Page.getByLabel("Report Answer").getByRole("button", { name: "Submit" }).click()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/mark-as-spam.spec.ts` around lines 76 - 77, Add an await before the click action to avoid the race condition: await the call on student2Page.getByLabel("Report Answer").getByRole("button", { name: "Submit" }).click() so the subsequent await expect(student2Page.getByText("Success", { exact: true })).toBeVisible() runs only after the click completes; ensure the click promise is awaited (and optionally await any resulting navigation or network idle if applicable).system-tests/src/tests/draft.spec.ts (1)
86-103:⚠️ Potential issue | 🟠 MajorWrap mutation clicks with success-notification waits to avoid flakiness.
Line 86 (
Create) and Line 102 (Update) trigger backend mutations but proceed withoutwaitForSuccessNotification, so subsequent steps can race ahead.Suggested fix
- await page.getByRole("dialog").getByRole("button", { name: "Create" }).click() + await waitForSuccessNotification(page, async () => { + await page.getByRole("dialog").getByRole("button", { name: "Create" }).click() + }) @@ - await page.getByRole("dialog").getByRole("button", { name: "Update" }).click() - await page.getByRole("button", { name: "Update", exact: true }).waitFor({ state: "hidden" }) + await waitForSuccessNotification(page, async () => { + await page.getByRole("dialog").getByRole("button", { name: "Update" }).click() + })As per coding guidelines, "For Playwright system tests, make sure each test that triggers a backend mutation ... waits for a UI confirmation element ... Prefer using
waitForSuccessNotificationfor success toasts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/draft.spec.ts` around lines 86 - 103, The Create and Update button clicks in the test (the calls that invoke page.getByRole("dialog").getByRole("button", { name: "Create" }).click() and page.getByRole("dialog").getByRole("button", { name: "Update" }).click() / page.getByRole("button", { name: "Update", exact: true }).waitFor(...)) must be wrapped with waits for the backend mutation success toast: after triggering each mutation, call the shared helper waitForSuccessNotification (or equivalent) to wait for the success notification before continuing; update the flows around the "Create" click and the "Update" click to wait for waitForSuccessNotification() (or its Promise) so later assertions and interactions are not raced.system-tests/src/tests/reset-exercises.spec.ts (3)
88-95:⚠️ Potential issue | 🟡 MinorMissing wait after "Start peer review" mutation.
Clicking "Start peer review" triggers a backend mutation, but the test immediately navigates to a different URL without waiting for confirmation. This can cause flaky tests if the operation hasn't completed before navigation.
Add a wait for a UI confirmation element (e.g., a success notification or state change) after clicking the button before proceeding with the
goto.As per coding guidelines: "make sure each test that triggers a backend mutation waits for a UI confirmation element that proves the request completed successfully before proceeding."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/reset-exercises.spec.ts` around lines 88 - 95, After clicking the "Start peer review" button via student1Page.getByRole("button", { name: "Start peer review" }), wait for a UI confirmation (for example a visible success notification, the appearance/disappearance of a specific toast, or a change in the button/state such as the "Try again" or a peer-review status element) before calling student1Page.goto(...). Locate the click in the test and insert an explicit wait (e.g., waitFor on the confirmation element or waitForSelector for the toast/state change) to ensure the backend mutation completed before navigation.
112-114:⚠️ Potential issue | 🟡 MinorMissing wait after Submit mutation before navigation.
The Submit click on line 112 triggers a backend mutation, but the test proceeds to navigate to the next page without waiting for confirmation that the submission completed.
Add a wait for a confirmation element (similar to the pattern used elsewhere in this test with
waitFor({ state: "visible" })).Proposed fix
await student1Page.getByRole("button", { name: "Submit" }).click() + await student1Page.getByRole("button", { name: "Try again" }).waitFor({ state: "visible" }) await navigateToNextPageInMaterial(student1Page)As per coding guidelines: "make sure each test that triggers a backend mutation waits for a UI confirmation element that proves the request completed successfully before proceeding."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/reset-exercises.spec.ts` around lines 112 - 114, After calling student1Page.getByRole("button", { name: "Submit" }).click(), add a wait for the submission confirmation UI (use the same pattern as elsewhere by calling waitFor({ state: "visible" }) on the confirmation element) before calling navigateToNextPageInMaterial; locate the confirmation element the test uses elsewhere (e.g., the "submission saved" / success text or specific selector used with waitFor) and await it to ensure the backend mutation completed before navigation.
203-203:⚠️ Potential issue | 🟡 MinorTypo: "resest" → "resets".
Proposed fix
- // Admin resest exercises that have a peer review + // Admin resets exercises that have a peer review🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/reset-exercises.spec.ts` at line 203, Update the inline comment "Admin resest exercises that have a peer review" to correct the typo: replace "resest" with "resets" so the comment reads "Admin resets exercises that have a peer review"; locate this string in system-tests/src/tests/reset-exercises.spec.ts (the comment near the test describing admin behavior) and make the single-word correction.system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-column.spec.ts (1)
77-77:⚠️ Potential issue | 🟠 MajorFix iframe scoping: use consistent frame references instead of generic selectors.
Lines 77, 108, 136–137 use
page.frameLocator("iframe")which may target the wrong iframe when multiple exist. On page-3 (lines 77 and nearby), useframe. On page-4 (after line 96), lines 108, 118, 131, 136–137, and 146 must useframe2.Additionally, line 14's comment
// Go to http://project-331.local/is redundant—it restates what the code does; remove it.Proposed fixes
- await page.frameLocator("iframe").locator('button:has-text("This is second option")').click() + await frame.locator('button:has-text("This is second option")').click() - await page.frameLocator("iframe").locator('button:has-text("This is first option")').click() + await frame2.locator('button:has-text("This is first option")').click() - frame.getByText(`Only first option can be correct because it is the first one.`), + frame2.getByText(`Only first option can be correct because it is the first one.`), - waitForTheseToBeVisibleAndStable: [frame.locator(`text="This is second option"`)], + waitForTheseToBeVisibleAndStable: [frame2.locator(`text="This is second option"`)], - await page.frameLocator("iframe").locator('button:has-text("This is first option")').click() - await page.frameLocator("iframe").locator('button:has-text("This is second option")').click() + await frame2.locator('button:has-text("This is first option")').click() + await frame2.locator('button:has-text("This is second option")').click() - frame.getByText(`Second option can't be the right answer because it isn't the first one.`), + frame2.getByText(`Second option can't be the right answer because it isn't the first one.`), - // Go to http://project-331.local/ - await page.goto("http://project-331.local/organizations") + await page.goto("http://project-331.local/organizations")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-column.spec.ts` at line 77, Replace generic page.frameLocator("iframe") calls with the appropriate named frame variables to avoid targeting the wrong iframe: on page-3 use frame (e.g., replace usages like page.frameLocator("iframe").locator(...).click() with frame.locator(...).click()), and on page-4 replace those calls with frame2 (use frame2.locator(...).click() for lines noted in the review such as the selectors around "This is second option" and other button/locator interactions). Also remove the redundant comment "// Go to http://project-331.local/" at the top of the test file. Ensure you update every instance that currently uses page.frameLocator("iframe") to the correct frame variable (frame or frame2) so selectors consistently target the intended iframe.system-tests/src/tests/query-mutation-notifications/success-notification.spec.ts (1)
17-28:⚠️ Potential issue | 🟠 MajorEnsure toast mode is always restored, even on assertion failures.
Line 17 changes toast behavior globally, but Line 28 won’t run if Lines 20-27 throw. This can leak state and make later tests flaky. Wrap the mutation/assertion block in
try/finallysoshowToastsNormally(page)always executes.As per coding guidelines, "If someone write a tests that depends on a toast staying on the screen, make sure to use the `showNextToastsInfinitely`, and `showToastsNormally` functions."Proposed fix
await showNextToastsInfinitely(page) - await page.click(`button:text-is("Save") >> visible=true`) - - await expectScreenshotsToMatchSnapshots({ - screenshotTarget: page, - headless, - testInfo, - snapshotName: "success-notification-test", - waitForTheseToBeVisibleAndStable: [page.getByText("Success").first().first()], - dontWaitForSpinnersToDisappear: true, - }) - await showToastsNormally(page) + try { + await page.click(`button:text-is("Save") >> visible=true`) + + await expectScreenshotsToMatchSnapshots({ + screenshotTarget: page, + headless, + testInfo, + snapshotName: "success-notification-test", + waitForTheseToBeVisibleAndStable: [page.getByText("Success").first().first()], + dontWaitForSpinnersToDisappear: true, + }) + } finally { + await showToastsNormally(page) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/query-mutation-notifications/success-notification.spec.ts` around lines 17 - 28, The test currently calls showNextToastsInfinitely(page) then runs actions and assertions (page.click and expectScreenshotsToMatchSnapshots) but if those throw the final call showToastsNormally(page) is skipped; wrap the block from page.click(...) through expectScreenshotsToMatchSnapshots(...) in a try/finally so showToastsNormally(page) is always executed to restore toast behavior (refer to showNextToastsInfinitely, page.click, expectScreenshotsToMatchSnapshots, and showToastsNormally).services/quizzes/tests/api/utils/appRouterTestClient.ts (3)
40-41:⚠️ Potential issue | 🟠 MajorUse binary-safe response forwarding instead of
response.text().The current code converts all responses to text via
response.text(), which corrupts binary payloads. Since the handler accepts a generic Web APIResponsethat can contain any data type (not just JSON), forwarding binary data as text loses integrity. UsearrayBuffer()+Bufferfor proper byte-safe forwarding.Suggested fix
- const responseBody = await response.text() - res.end(responseBody) + const responseBody = Buffer.from(await response.arrayBuffer()) + res.end(responseBody)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/quizzes/tests/api/utils/appRouterTestClient.ts` around lines 40 - 41, Replace the text-based forwarding with binary-safe forwarding: call response.arrayBuffer() instead of response.text(), convert it to a Node Buffer with Buffer.from(await response.arrayBuffer()), and pass that Buffer to res.end(...) so binary payloads are preserved; make sure to use the existing Response object (response) and the same status/headers handling around where responseBody and res.end(...) are used (e.g., in appRouterTestClient.ts where responseBody is created and res.end is called).
18-22:⚠️ Potential issue | 🟠 MajorNormalize incoming Node headers to handle multi-value headers like
Set-Cookiecorrectly.Line 20 casts
req.headers as HeadersInitdirectly, but Node.js headers containstring | string[] | undefinedvalues. The Fetch API'sRequestInitsilently concatenates array values with commas (e.g.,["a", "b"]becomes"a,b"), which breaks multi-value headers likeSet-Cookiethat require separate entries. Additionally,undefinedvalues become the string"undefined"rather than being omitted.The suggested fix normalizes headers using the
Headersobject, which correctly handles arrays viaappend():Suggested fix
+ const headers = new Headers() + for (const [key, value] of Object.entries(req.headers)) { + if (value === undefined) continue + if (Array.isArray(value)) { + for (const v of value) headers.append(key, v) + } else { + headers.set(key, value) + } + } + const requestObj = new Request(url, { method: req.method, - headers: req.headers as HeadersInit, + headers, body, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/quizzes/tests/api/utils/appRouterTestClient.ts` around lines 18 - 22, The request creation currently casts req.headers to HeadersInit (the Request instantiation at requestObj) which concatenates arrays and turns undefined into "undefined"; instead, normalize Node headers by creating a new Headers() instance, iterate over Object.entries(req.headers) and for each key: if value is an array call headers.append for each element, if value is a string call headers.set, and if value is undefined skip it, then pass that Headers instance into the Request constructor (replace the direct cast in the Request(url, {...}) call).
34-37:⚠️ Potential issue | 🟠 MajorRemove unconditional JSON
Content-Typefallback.Lines 34-37 force
application/jsonwhenever the handler omitsContent-Type. This masks bugs in handler implementations and mislabels responses that are plain text, HTML, binary, or empty. The fallback should be removed; handlers should be responsible for setting appropriate Content-Type headers.Suggested fix
- // Ensure Content-Type is set for JSON responses - if (!res.getHeader("Content-Type")) { - res.setHeader("Content-Type", "application/json") - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/quizzes/tests/api/utils/appRouterTestClient.ts` around lines 34 - 37, Remove the unconditional fallback that sets Content-Type to application/json in the test client: delete the block that checks res.getHeader("Content-Type") and calls res.setHeader("Content-Type", "application/json") (the lines shown in appRouterTestClient.ts). Let handlers themselves set appropriate Content-Type values and update any tests that relied on the fallback to explicitly set expected headers where needed; keep the rest of the response handling in the test client unchanged.
🧹 Nitpick comments (17)
system-tests/src/tests/mark-as-spam.spec.ts (2)
44-56: Same pattern: submission without confirmation wait.Line 55 also submits without waiting for confirmation. If the submission hasn't completed before Student2's test step begins, it could affect the peer review availability.
Consider adding a confirmation wait or using
waitForSuccessNotificationfor consistency with the coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/mark-as-spam.spec.ts` around lines 44 - 56, The Student1 submission step (inside the test.step "Student1 submits answer") clicks the Submit button on student1Page but doesn't wait for completion; after calling student1Page.getByRole("button", { name: "Submit" }).click(), wait for the confirmation by either using the existing helper waitForSuccessNotification(student1Page) or an equivalent assertion (e.g., wait for the success toast or the post-submission state) to ensure the submission is finished before proceeding to Student2's steps.
69-71: Consider waiting for submission confirmation before proceeding.The submit button click triggers a backend mutation but the test immediately proceeds to click "Start peer review" without verifying the submission completed. This could cause flaky behavior if the submission hasn't finished processing.
As per coding guidelines: "make sure each test that triggers a backend mutation (e.g., clicking a save button) waits for a UI confirmation element that proves the request completed successfully before proceeding."
♻️ Suggested approach
await student2Page.getByRole("button", { name: "Submit" }).click() + // Wait for submission confirmation before starting peer review + await expect(student2Page.getByText(/submitted/i)).toBeVisible() await student2Page.getByRole("button", { name: "Start peer review" }).click()Alternatively, if there's a success toast, use
waitForSuccessNotification:await waitForSuccessNotification(student2Page, async () => { await student2Page.getByRole("button", { name: "Submit" }).click() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/mark-as-spam.spec.ts` around lines 69 - 71, The test clicks the Submit button then immediately clicks "Start peer review", risking flakiness because the backend mutation may not have completed; update the flow around student2Page.getByRole("button", { name: "Submit" }).click() to wait for a UI confirmation before proceeding—either wrap the click with the helper waitForSuccessNotification(student2Page, async () => { ... }) or explicitly await a success toast or confirmation element (e.g., await student2Page.locator(...).waitFor()) before calling student2Page.getByRole("button", { name: "Start peer review" }).click(); ensure the chosen confirmation is deterministic and related to the submission action.system-tests/src/tests/user-research-consent.spec.ts (1)
68-69: Consider replacingnetworkidlewith a more reliable wait.While the ESLint disable comment acknowledges the issue,
waitForLoadState("networkidle")can be flaky. Consider waiting for a specific UI element that indicates the page is ready instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/user-research-consent.spec.ts` around lines 68 - 69, Replace the flaky waitForLoadState("networkidle") with an explicit wait for a specific UI element that signals readiness: locate the element that shows the consent UI (e.g., the consent banner/form button) and change the call to await page.waitForSelector('<CONSENT_SELECTOR>') instead of page.waitForLoadState("networkidle"); update the test in user-research-consent.spec.ts where page.waitForLoadState is used so the test waits deterministically for that selector before interacting with the page.services/headless-lms/doc-macro/Cargo.toml (1)
19-19: Use local#[allow(clippy::expect_used)]instead of global allow for better intent clarity.The crate has only 4
expect()callsites, all with clear invariant-checking rationale in parsing/generation logic. Allowingexpect_usedglobally while denyingunwrap_usedcreates policy asymmetry; consider adding targeted#[allow(clippy::expect_used)]at each callsite instead to make the intent explicit and maintain consistent panic-prevention policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/doc-macro/Cargo.toml` at line 19, Remove the global expect allowance by deleting the expect_used = "allow" entry from Cargo.toml and instead add targeted #[allow(clippy::expect_used)] attributes on the specific functions/methods that call expect() (the four parsing/generation callsites) so each callsite is annotated (place the attribute on the enclosing fn or impl block and add a short comment documenting the invariant that justifies the expect). Make sure no other global clippy allow for expect remains and run cargo clippy to verify only those annotated locations suppress the lint.system-tests/src/tests/reset-exercises.spec.ts (1)
146-149: Prefer semantic selectors over index-based checkbox selection.These lines use fragile index-based selectors (
nth(3),nth(4), etc.) that will break if the checkbox order changes. Compare to the better pattern used later at lines 171-172:await adminPage.getByRole("row", { name: "Multiple task exercise Page" }).getByLabel("").check()Consider refactoring to use row names or other semantic identifiers to make the test more resilient to UI changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/reset-exercises.spec.ts` around lines 146 - 149, The test uses fragile index-based checkbox selection via adminPage.locator('input[type="checkbox"]').nth(3/4/5/6'); replace each nth(...) call with semantic selectors—e.g., find the specific row by its visible name using adminPage.getByRole("row", { name: "<row name>" }) and then call .getByLabel("<checkbox label>" or "")/ .locator('input[type="checkbox"]') on that row before .check(); update the four checkbox lines to target their rows or labels so the assertions use stable, descriptive selectors instead of numeric indexes.system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-column.spec.ts (1)
14-14: Drop this redundant navigation comment.Line 14 repeats the intent already obvious from Line 15.
As per coding guidelines, "Prefer comment quality over comment quantity. Do not add comments that simply restate what the code already says, narrate control flow, or explain obvious syntax."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-column.spec.ts` at line 14, Remove the redundant inline comment "// Go to http://project-331.local/" that merely repeats the next statement; simply delete that comment so the test reads directly with the navigation call (e.g., the cy.visit or equivalent on the following line) without the duplicate explanatory line.system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-row.spec.ts (2)
17-19: Remove the pseudo-parallelPromise.allwrapper.Line 17 executes sequentially because of the inner
await, soPromise.alladds noise without benefit.♻️ Proposed cleanup
- await Promise.all([ - await selectOrganization(page, "University of Helsinki, Department of Computer Science"), - ]) + await selectOrganization(page, "University of Helsinki, Department of Computer Science")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-row.spec.ts` around lines 17 - 19, Remove the unnecessary Promise.all wrapper around a single awaited call: replace the current await Promise.all([ await selectOrganization(page, "University of Helsinki, Department of Computer Science") ]) by directly awaiting the selectOrganization call (await selectOrganization(page, "University of Helsinki, Department of Computer Science")); if you intended true parallelism with multiple calls, instead pass raw promises to Promise.all (e.g., Promise.all([selectOrganization(...), otherCall(...)]) ) but do not use an inner await.
14-14: Drop this redundant navigation comment.Line 14 restates exactly what Line 15 already expresses and adds no extra rationale.
As per coding guidelines, "Prefer comment quality over comment quantity. Do not add comments that simply restate what the code already says, narrate control flow, or explain obvious syntax."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-row.spec.ts` at line 14, Remove the redundant navigation comment "// Go to http://project-331.local/" in the multiple-choice-course-material-widget-row.spec.ts test file (it duplicates the next line). Delete that comment line so the test only contains the actual navigation code, or replace it with a concise, non-redundant explanation only if additional context is necessary.services/headless-lms/server/src/test_helper.rs (1)
169-195: LGTM - Functional but verbose.The fully-qualified trait syntax works correctly for generating random strings in macros where import hygiene matters. The pattern is repeated three times (lines 169-173, 180-184, 191-195).
If desired, the verbosity could be reduced by importing the trait at the macro expansion site or using a helper function, but the current approach is functionally correct for test code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/test_helper.rs` around lines 169 - 195, The repeated fully-qualified calls to ::rand::distr::Alphanumeric and ::rand::distr::SampleString in the macro arms (the `@inner` patterns creating rs) are verbose; simplify by centralizing randomness: either add a small helper function (e.g., sample_random_alphanumeric(len) used by the macro) or import the trait/type at the macro expansion site so you can call Alphanumeric::sample_string(&Alphanumeric, &mut rand::rng(), 8) or SampleString::sample_string(...) with shorter names; update the macro arms that reference ::rand::distr::Alphanumeric, SampleString and rand::rng() to call the helper or the imported names to remove duplication.services/headless-lms/models/src/user_chapter_locking_statuses.rs (1)
268-291: Nested async function is unconventional but functional.Defining
get_statuses_for_user_and_courseas a nested async function insideget_or_init_all_for_courseworks but is unusual. Consider extracting it as a private module-level function for better readability and potential reuse.♻️ Optional: Extract as module-level function
+async fn get_statuses_for_user_and_course( + conn: &mut PgConnection, + user_id: Uuid, + course_id: Uuid, +) -> ModelResult<Vec<UserChapterLockingStatus>> { + let rows = sqlx::query_as!( + DatabaseRow, + r#" +SELECT id, created_at, updated_at, deleted_at, user_id, chapter_id, course_id, status::text as "status!" +FROM user_chapter_locking_statuses +WHERE user_id = $1 + AND course_id = $2 + AND deleted_at IS NULL + "#, + user_id, + course_id + ) + .fetch_all(&mut *conn) + .await?; + + rows.into_iter() + .map(|r| r.try_into()) + .collect::<ModelResult<Vec<_>>>() +} + pub async fn get_or_init_all_for_course( // ... remove the nested function definition }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs` around lines 268 - 291, Extract the nested async function get_statuses_for_user_and_course out of get_or_init_all_for_course into a private module-level async fn with the same signature (async fn get_statuses_for_user_and_course(conn: &mut PgConnection, user_id: Uuid, course_id: Uuid) -> ModelResult<Vec<UserChapterLockingStatus>>) and move its SQL query body unchanged; update the call site inside get_or_init_all_for_course to call the new module-level function and ensure required types (DatabaseRow, UserChapterLockingStatus, ModelResult, Uuid, PgConnection) are in scope (add use statements if needed) and remove the nested function definition.services/quizzes/tests/api/utils/appRouterTestClient.ts (1)
13-39: Trim redundant inline comments in the listener body.Several comments here restate obvious operations (status/header/body setting), which makes the helper noisier to scan. Keep only intent/rationale comments where behavior is non-obvious.
As per coding guidelines, "Prefer comment quality over comment quantity... Do not add comments that simply restate what the code already says..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/quizzes/tests/api/utils/appRouterTestClient.ts` around lines 13 - 39, Remove redundant, self‑evident inline comments in the listener body and keep only concise intent comments: delete comments that simply restate the code (e.g., "Convert Node.js IncomingMessage to Web API Request", "Create a proper Request object using the Web API", "Set response status", "Set response headers", "Send response body"), and retain a short explanatory comment where behavior is non‑obvious (for example keep one line explaining why you set a default Content-Type for JSON). Locate the block around req, getBody, new Request(...), handler(requestObj), response, and res in appRouterTestClient.ts and simplify to a single intent comment plus the Content-Type rationale.shared-module/packages/common/pnpm-workspace.yaml (1)
21-24: Deduplicate overlappingminimatchoverride ranges.Line 22 already covers Line 21, and Line 24 already covers Line 23. Removing redundant entries keeps the override set easier to audit.
♻️ Proposed simplification
overrides: glob@>=10.2.0 <10.5.0: '>=10.5.0' js-yaml@<3.14.2: '>=3.14.2' js-yaml@>=4.0.0 <4.1.1: '>=4.1.1' - minimatch@<3.1.3: '>=3.1.3' minimatch@<3.1.4: '>=3.1.4' - minimatch@>=9.0.0 <9.0.6: '>=9.0.6' minimatch@>=9.0.0 <9.0.7: '>=9.0.7'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared-module/packages/common/pnpm-workspace.yaml` around lines 21 - 24, Remove the redundant minimatch overrides: delete the entries "minimatch@<3.1.3: '>=3.1.3'" and "minimatch@>=9.0.0 <9.0.6: '>=9.0.6'" because they are subsumed by "minimatch@<3.1.4: '>=3.1.4'" and "minimatch@>=9.0.0 <9.0.7: '>=9.0.7'"; after removing them, ensure the pnpm-workspace.yaml remains valid YAML (correct indentation/quotes) and keep the remaining minimatch override entries intact.services/main-frontend/pnpm-workspace.yaml (1)
20-23: Remove overlappingminimatchoverrides to reduce config noise.Line 21 subsumes Line 20, and Line 23 subsumes Line 22. Keeping only the stricter ranges preserves behavior and avoids ambiguity during maintenance.
♻️ Proposed simplification
overrides: glob@>=10.2.0 <10.5.0: '>=10.5.0' js-yaml@>=4.0.0 <4.1.1: '>=4.1.1' - minimatch@<3.1.3: '>=3.1.3' minimatch@<3.1.4: '>=3.1.4' - minimatch@>=9.0.0 <9.0.6: '>=9.0.6' minimatch@>=9.0.0 <9.0.7: '>=9.0.7'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/pnpm-workspace.yaml` around lines 20 - 23, Remove the overlapping minimatch overrides and keep only the stricter selectors: delete the minimatch@<3.1.3: '>=3.1.3' entry and the minimatch@>=9.0.0 <9.0.6: '>=9.0.6' entry, leaving minimatch@<3.1.4: '>=3.1.4' and minimatch@>=9.0.0 <9.0.7: '>=9.0.7' so behavior is preserved and duplicate/ambiguous overrides are eliminated.bin/extract-gutenberg-types (1)
26-30: Optional: avoid double-formatting the same type files.These new conditional blocks are valid, but combined with the earlier formatter call they can format the same
.tsfiles twice per run. Consider formatting once at the end per target pair.Also applies to: 33-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/extract-gutenberg-types` around lines 26 - 30, The script currently calls run_command pnpm exec prettier twice for the same targets (services/main-frontend/types/GutenbergBlockAttributes.ts and GutenbergBlockAttributes.guard.ts) which can double-format; update the logic in the extract-gutenberg-types script to collect the target files (e.g., GutenbergBlockAttributes.ts and GutenbergBlockAttributes.guard.ts) into a unique list and invoke a single run_command prettier call per target pair (or run one final prettier invocation for all unique files) instead of calling prettier in both the earlier formatter call and again in the conditional blocks—modify the sections that call run_command pnpm exec prettier so they use the deduplicated list or a single post-processing prettier step.services/main-frontend/src/components/course-material/ContentRenderer/core/common/Image/ImageBlock.tsx (2)
108-108: Remove unnecessary fragment wrapper.The fragment around
renderImage()is redundant whendisableInteractivityis false.♻️ Simplify rendering
- {disableInteractivity ? renderImage() : <>{renderImage()}</>} + {renderImage()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/components/course-material/ContentRenderer/core/common/Image/ImageBlock.tsx` at line 108, The JSX contains a redundant fragment: in the conditional expression inside ImageBlock the false branch wraps renderImage() in an unnecessary <>...</>; replace the ternary expression so both branches render the component directly (use disableInteractivity ? renderImage() : renderImage() or simply renderImage() with conditional props) — update the usage around disableInteractivity and renderImage() to remove the fragment wrapper.
68-70: UseVisuallyHiddenfrom react-aria instead of CSS class.Per coding guidelines, screen-reader-only text should use
VisuallyHiddenfrom react-aria rather than custom CSS classes.♻️ Proposed refactor
+import { VisuallyHidden } from "react-aria" // ... {linkTarget && linkTarget.includes("_blank") && ( - <span className="screen-reader-only">{t("screen-reader-opens-in-new-tab")}</span> + <VisuallyHidden>{t("screen-reader-opens-in-new-tab")}</VisuallyHidden> )}As per coding guidelines: "For screen-reader-only text, use VisuallyHidden from react-aria instead of custom sr-only / visually-hidden CSS."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/components/course-material/ContentRenderer/core/common/Image/ImageBlock.tsx` around lines 68 - 70, Replace the custom screen-reader-only span in ImageBlock (the conditional that renders {linkTarget && linkTarget.includes("_blank") && <span className="screen-reader-only">{t("screen-reader-opens-in-new-tab")}</span>}) with react-aria's VisuallyHidden: import VisuallyHidden from '@react-aria/visually-hidden' (or the project-specific react-aria entry) at the top of the file and render <VisuallyHidden>{t("screen-reader-opens-in-new-tab")}</VisuallyHidden> in place of the span so the translation string from t(...) is preserved and accessibility follows the coding guideline.services/main-frontend/src/components/course-material/ContentRenderer/core/formatting/VerseBlock.tsx (1)
12-24: Consider validatingtextAlignvalues before interpolation.The
textAlignvalue is directly interpolated into CSS. While Gutenberg typically provides sanitized values, consider constraining to known valid values (left,center,right,justify) for defense-in-depth, consistent with howalignis validated in other block types.🛡️ Optional: Constrain textAlign to valid CSS values
interface ExtraAttributes { - textAlign?: string + textAlign?: "left" | "center" | "right" | "justify" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/components/course-material/ContentRenderer/core/formatting/VerseBlock.tsx` around lines 12 - 24, The code interpolates data.attributes.textAlign directly into the CSS inside VerseBlock (ExtraAttributes → data.attributes → textAlign); validate textAlign against a whitelist (e.g., "left","center","right","justify") before using it in the className interpolation and fall back to a safe default or omit the rule if invalid. Add a small guard in VerseBlock that normalizes/checks textAlign (e.g., const safeTextAlign = allowed.has(textAlign) ? textAlign : undefined) and then use safeTextAlign in the template instead of the raw value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pnpm-workspace.yaml`:
- Around line 15-17: The onlyBuiltDependencies entry currently has a single
malformed scalar "esbuild unrs-resolver"; split it into two distinct list items
so each package is its own entry under onlyBuiltDependencies (e.g., create
separate items for esbuild and unrs-resolver) so strictDepBuilds can correctly
exclude each package; update the onlyBuiltDependencies list to standard YAML
list syntax ensuring "esbuild" and "unrs-resolver" appear as separate entries.
In `@services/cms/pnpm-workspace.yaml`:
- Around line 7-10: The minimumReleaseAgeExclude array (key name
minimumReleaseAgeExclude) is missing "react-dom"; update the list that currently
contains "next", "react", and "@next/*" to also include "react-dom" so React and
ReactDOM are both excluded from release-age gating and remain
version-synchronized with other workspaces.
In `@services/headless-lms/utils/Cargo.toml`:
- Line 40: Replace the deprecated reqwest builder call .use_rustls_tls() with
.tls_backend_rustls() in the HTTP client construction (locate the
reqwest::Client::builder() usage in services/headless-lms/utils/src/http.rs),
and ensure the Cargo.toml has reqwest compiled with the rustls backend feature
so the new tls_backend_rustls() method is available; leave rand = "0.10.0" as-is
since no changes are required for its usage.
In `@shared-module/packages/common/package.json`:
- Line 77: Update the `@types/node` dependency in package.json to match the
runtime major (Node 24) so type definitions align with the pinned runtime;
locate the "@types/node" entry in shared-module/packages/common/package.json and
change its version spec (currently "^25.3.3") to a 24.x range (for example
"^24.0.0" or a more specific 24.y.z you prefer), then run install/typecheck to
verify no type mismatches.
---
Outside diff comments:
In `@services/quizzes/tests/api/utils/appRouterTestClient.ts`:
- Around line 40-41: Replace the text-based forwarding with binary-safe
forwarding: call response.arrayBuffer() instead of response.text(), convert it
to a Node Buffer with Buffer.from(await response.arrayBuffer()), and pass that
Buffer to res.end(...) so binary payloads are preserved; make sure to use the
existing Response object (response) and the same status/headers handling around
where responseBody and res.end(...) are used (e.g., in appRouterTestClient.ts
where responseBody is created and res.end is called).
- Around line 18-22: The request creation currently casts req.headers to
HeadersInit (the Request instantiation at requestObj) which concatenates arrays
and turns undefined into "undefined"; instead, normalize Node headers by
creating a new Headers() instance, iterate over Object.entries(req.headers) and
for each key: if value is an array call headers.append for each element, if
value is a string call headers.set, and if value is undefined skip it, then pass
that Headers instance into the Request constructor (replace the direct cast in
the Request(url, {...}) call).
- Around line 34-37: Remove the unconditional fallback that sets Content-Type to
application/json in the test client: delete the block that checks
res.getHeader("Content-Type") and calls res.setHeader("Content-Type",
"application/json") (the lines shown in appRouterTestClient.ts). Let handlers
themselves set appropriate Content-Type values and update any tests that relied
on the fallback to explicitly set expected headers where needed; keep the rest
of the response handling in the test client unchanged.
In `@system-tests/src/tests/draft.spec.ts`:
- Around line 86-103: The Create and Update button clicks in the test (the calls
that invoke page.getByRole("dialog").getByRole("button", { name: "Create"
}).click() and page.getByRole("dialog").getByRole("button", { name: "Update"
}).click() / page.getByRole("button", { name: "Update", exact: true
}).waitFor(...)) must be wrapped with waits for the backend mutation success
toast: after triggering each mutation, call the shared helper
waitForSuccessNotification (or equivalent) to wait for the success notification
before continuing; update the flows around the "Create" click and the "Update"
click to wait for waitForSuccessNotification() (or its Promise) so later
assertions and interactions are not raced.
In `@system-tests/src/tests/mark-as-spam.spec.ts`:
- Around line 76-77: Add an await before the click action to avoid the race
condition: await the call on student2Page.getByLabel("Report
Answer").getByRole("button", { name: "Submit" }).click() so the subsequent await
expect(student2Page.getByText("Success", { exact: true })).toBeVisible() runs
only after the click completes; ensure the click promise is awaited (and
optionally await any resulting navigation or network idle if applicable).
In
`@system-tests/src/tests/query-mutation-notifications/success-notification.spec.ts`:
- Around line 17-28: The test currently calls showNextToastsInfinitely(page)
then runs actions and assertions (page.click and
expectScreenshotsToMatchSnapshots) but if those throw the final call
showToastsNormally(page) is skipped; wrap the block from page.click(...) through
expectScreenshotsToMatchSnapshots(...) in a try/finally so
showToastsNormally(page) is always executed to restore toast behavior (refer to
showNextToastsInfinitely, page.click, expectScreenshotsToMatchSnapshots, and
showToastsNormally).
In
`@system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-column.spec.ts`:
- Line 77: Replace generic page.frameLocator("iframe") calls with the
appropriate named frame variables to avoid targeting the wrong iframe: on page-3
use frame (e.g., replace usages like
page.frameLocator("iframe").locator(...).click() with
frame.locator(...).click()), and on page-4 replace those calls with frame2 (use
frame2.locator(...).click() for lines noted in the review such as the selectors
around "This is second option" and other button/locator interactions). Also
remove the redundant comment "// Go to http://project-331.local/" at the top of
the test file. Ensure you update every instance that currently uses
page.frameLocator("iframe") to the correct frame variable (frame or frame2) so
selectors consistently target the intended iframe.
In `@system-tests/src/tests/reset-exercises.spec.ts`:
- Around line 88-95: After clicking the "Start peer review" button via
student1Page.getByRole("button", { name: "Start peer review" }), wait for a UI
confirmation (for example a visible success notification, the
appearance/disappearance of a specific toast, or a change in the button/state
such as the "Try again" or a peer-review status element) before calling
student1Page.goto(...). Locate the click in the test and insert an explicit wait
(e.g., waitFor on the confirmation element or waitForSelector for the
toast/state change) to ensure the backend mutation completed before navigation.
- Around line 112-114: After calling student1Page.getByRole("button", { name:
"Submit" }).click(), add a wait for the submission confirmation UI (use the same
pattern as elsewhere by calling waitFor({ state: "visible" }) on the
confirmation element) before calling navigateToNextPageInMaterial; locate the
confirmation element the test uses elsewhere (e.g., the "submission saved" /
success text or specific selector used with waitFor) and await it to ensure the
backend mutation completed before navigation.
- Line 203: Update the inline comment "Admin resest exercises that have a peer
review" to correct the typo: replace "resest" with "resets" so the comment reads
"Admin resets exercises that have a peer review"; locate this string in
system-tests/src/tests/reset-exercises.spec.ts (the comment near the test
describing admin behavior) and make the single-word correction.
---
Nitpick comments:
In `@bin/extract-gutenberg-types`:
- Around line 26-30: The script currently calls run_command pnpm exec prettier
twice for the same targets
(services/main-frontend/types/GutenbergBlockAttributes.ts and
GutenbergBlockAttributes.guard.ts) which can double-format; update the logic in
the extract-gutenberg-types script to collect the target files (e.g.,
GutenbergBlockAttributes.ts and GutenbergBlockAttributes.guard.ts) into a unique
list and invoke a single run_command prettier call per target pair (or run one
final prettier invocation for all unique files) instead of calling prettier in
both the earlier formatter call and again in the conditional blocks—modify the
sections that call run_command pnpm exec prettier so they use the deduplicated
list or a single post-processing prettier step.
In `@services/headless-lms/doc-macro/Cargo.toml`:
- Line 19: Remove the global expect allowance by deleting the expect_used =
"allow" entry from Cargo.toml and instead add targeted
#[allow(clippy::expect_used)] attributes on the specific functions/methods that
call expect() (the four parsing/generation callsites) so each callsite is
annotated (place the attribute on the enclosing fn or impl block and add a short
comment documenting the invariant that justifies the expect). Make sure no other
global clippy allow for expect remains and run cargo clippy to verify only those
annotated locations suppress the lint.
In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs`:
- Around line 268-291: Extract the nested async function
get_statuses_for_user_and_course out of get_or_init_all_for_course into a
private module-level async fn with the same signature (async fn
get_statuses_for_user_and_course(conn: &mut PgConnection, user_id: Uuid,
course_id: Uuid) -> ModelResult<Vec<UserChapterLockingStatus>>) and move its SQL
query body unchanged; update the call site inside get_or_init_all_for_course to
call the new module-level function and ensure required types (DatabaseRow,
UserChapterLockingStatus, ModelResult, Uuid, PgConnection) are in scope (add use
statements if needed) and remove the nested function definition.
In `@services/headless-lms/server/src/test_helper.rs`:
- Around line 169-195: The repeated fully-qualified calls to
::rand::distr::Alphanumeric and ::rand::distr::SampleString in the macro arms
(the `@inner` patterns creating rs) are verbose; simplify by centralizing
randomness: either add a small helper function (e.g.,
sample_random_alphanumeric(len) used by the macro) or import the trait/type at
the macro expansion site so you can call
Alphanumeric::sample_string(&Alphanumeric, &mut rand::rng(), 8) or
SampleString::sample_string(...) with shorter names; update the macro arms that
reference ::rand::distr::Alphanumeric, SampleString and rand::rng() to call the
helper or the imported names to remove duplication.
In `@services/main-frontend/pnpm-workspace.yaml`:
- Around line 20-23: Remove the overlapping minimatch overrides and keep only
the stricter selectors: delete the minimatch@<3.1.3: '>=3.1.3' entry and the
minimatch@>=9.0.0 <9.0.6: '>=9.0.6' entry, leaving minimatch@<3.1.4: '>=3.1.4'
and minimatch@>=9.0.0 <9.0.7: '>=9.0.7' so behavior is preserved and
duplicate/ambiguous overrides are eliminated.
In
`@services/main-frontend/src/components/course-material/ContentRenderer/core/common/Image/ImageBlock.tsx`:
- Line 108: The JSX contains a redundant fragment: in the conditional expression
inside ImageBlock the false branch wraps renderImage() in an unnecessary
<>...</>; replace the ternary expression so both branches render the component
directly (use disableInteractivity ? renderImage() : renderImage() or simply
renderImage() with conditional props) — update the usage around
disableInteractivity and renderImage() to remove the fragment wrapper.
- Around line 68-70: Replace the custom screen-reader-only span in ImageBlock
(the conditional that renders {linkTarget && linkTarget.includes("_blank") &&
<span
className="screen-reader-only">{t("screen-reader-opens-in-new-tab")}</span>})
with react-aria's VisuallyHidden: import VisuallyHidden from
'@react-aria/visually-hidden' (or the project-specific react-aria entry) at the
top of the file and render
<VisuallyHidden>{t("screen-reader-opens-in-new-tab")}</VisuallyHidden> in place
of the span so the translation string from t(...) is preserved and accessibility
follows the coding guideline.
In
`@services/main-frontend/src/components/course-material/ContentRenderer/core/formatting/VerseBlock.tsx`:
- Around line 12-24: The code interpolates data.attributes.textAlign directly
into the CSS inside VerseBlock (ExtraAttributes → data.attributes → textAlign);
validate textAlign against a whitelist (e.g., "left","center","right","justify")
before using it in the className interpolation and fall back to a safe default
or omit the rule if invalid. Add a small guard in VerseBlock that
normalizes/checks textAlign (e.g., const safeTextAlign = allowed.has(textAlign)
? textAlign : undefined) and then use safeTextAlign in the template instead of
the raw value.
In `@services/quizzes/tests/api/utils/appRouterTestClient.ts`:
- Around line 13-39: Remove redundant, self‑evident inline comments in the
listener body and keep only concise intent comments: delete comments that simply
restate the code (e.g., "Convert Node.js IncomingMessage to Web API Request",
"Create a proper Request object using the Web API", "Set response status", "Set
response headers", "Send response body"), and retain a short explanatory comment
where behavior is non‑obvious (for example keep one line explaining why you set
a default Content-Type for JSON). Locate the block around req, getBody, new
Request(...), handler(requestObj), response, and res in appRouterTestClient.ts
and simplify to a single intent comment plus the Content-Type rationale.
In `@shared-module/packages/common/pnpm-workspace.yaml`:
- Around line 21-24: Remove the redundant minimatch overrides: delete the
entries "minimatch@<3.1.3: '>=3.1.3'" and "minimatch@>=9.0.0 <9.0.6: '>=9.0.6'"
because they are subsumed by "minimatch@<3.1.4: '>=3.1.4'" and
"minimatch@>=9.0.0 <9.0.7: '>=9.0.7'"; after removing them, ensure the
pnpm-workspace.yaml remains valid YAML (correct indentation/quotes) and keep the
remaining minimatch override entries intact.
In `@system-tests/src/tests/mark-as-spam.spec.ts`:
- Around line 44-56: The Student1 submission step (inside the test.step
"Student1 submits answer") clicks the Submit button on student1Page but doesn't
wait for completion; after calling student1Page.getByRole("button", { name:
"Submit" }).click(), wait for the confirmation by either using the existing
helper waitForSuccessNotification(student1Page) or an equivalent assertion
(e.g., wait for the success toast or the post-submission state) to ensure the
submission is finished before proceeding to Student2's steps.
- Around line 69-71: The test clicks the Submit button then immediately clicks
"Start peer review", risking flakiness because the backend mutation may not have
completed; update the flow around student2Page.getByRole("button", { name:
"Submit" }).click() to wait for a UI confirmation before proceeding—either wrap
the click with the helper waitForSuccessNotification(student2Page, async () => {
... }) or explicitly await a success toast or confirmation element (e.g., await
student2Page.locator(...).waitFor()) before calling
student2Page.getByRole("button", { name: "Start peer review" }).click(); ensure
the chosen confirmation is deterministic and related to the submission action.
In
`@system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-column.spec.ts`:
- Line 14: Remove the redundant inline comment "// Go to
http://project-331.local/" that merely repeats the next statement; simply delete
that comment so the test reads directly with the navigation call (e.g., the
cy.visit or equivalent on the following line) without the duplicate explanatory
line.
In
`@system-tests/src/tests/quizzes/widget/multiple-choice-course-material-widget-row.spec.ts`:
- Around line 17-19: Remove the unnecessary Promise.all wrapper around a single
awaited call: replace the current await Promise.all([ await
selectOrganization(page, "University of Helsinki, Department of Computer
Science") ]) by directly awaiting the selectOrganization call (await
selectOrganization(page, "University of Helsinki, Department of Computer
Science")); if you intended true parallelism with multiple calls, instead pass
raw promises to Promise.all (e.g., Promise.all([selectOrganization(...),
otherCall(...)]) ) but do not use an inner await.
- Line 14: Remove the redundant navigation comment "// Go to
http://project-331.local/" in the
multiple-choice-course-material-widget-row.spec.ts test file (it duplicates the
next line). Delete that comment line so the test only contains the actual
navigation code, or replace it with a concise, non-redundant explanation only if
additional context is necessary.
In `@system-tests/src/tests/reset-exercises.spec.ts`:
- Around line 146-149: The test uses fragile index-based checkbox selection via
adminPage.locator('input[type="checkbox"]').nth(3/4/5/6'); replace each nth(...)
call with semantic selectors—e.g., find the specific row by its visible name
using adminPage.getByRole("row", { name: "<row name>" }) and then call
.getByLabel("<checkbox label>" or "")/ .locator('input[type="checkbox"]') on
that row before .check(); update the four checkbox lines to target their rows or
labels so the assertions use stable, descriptive selectors instead of numeric
indexes.
In `@system-tests/src/tests/user-research-consent.spec.ts`:
- Around line 68-69: Replace the flaky waitForLoadState("networkidle") with an
explicit wait for a specific UI element that signals readiness: locate the
element that shows the consent UI (e.g., the consent banner/form button) and
change the call to await page.waitForSelector('<CONSENT_SELECTOR>') instead of
page.waitForLoadState("networkidle"); update the test in
user-research-consent.spec.ts where page.waitForLoadState is used so the test
waits deterministically for that selector before interacting with the page.
| onlyBuiltDependencies: | ||
| - esbuild unrs-resolver | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "pnpm-workspace.yaml" -type f | head -5Repository: rage/secret-project-331
Length of output: 255
🏁 Script executed:
cat -n pnpm-workspace.yaml | sed -n '10,20p'Repository: rage/secret-project-331
Length of output: 325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import pathlib, re
for p in pathlib.Path('.').rglob('pnpm-workspace.yaml'):
lines = p.read_text(encoding='utf-8').splitlines()
in_block = False
for i, line in enumerate(lines, start=1):
if re.match(r'^\s*onlyBuiltDependencies:\s*$', line):
in_block = True
continue
if in_block:
if re.match(r'^\S', line):
in_block = False
continue
m = re.match(r'^\s*-\s*(.+?)\s*$', line)
if not m:
continue
item = m.group(1).strip("'\"")
if ' ' in item:
print(f"{p}:{i}: suspicious onlyBuiltDependencies item -> {item!r}")
PYRepository: rage/secret-project-331
Length of output: 155
Fix malformed onlyBuiltDependencies entry.
Line 16 combines two package names into one scalar (esbuild unrs-resolver). With strictDepBuilds enabled, this prevents the intended packages from being excluded from dependency builds.
Suggested patch
onlyBuiltDependencies:
- - esbuild unrs-resolver
+ - esbuild
+ - unrs-resolver📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onlyBuiltDependencies: | |
| - esbuild unrs-resolver | |
| onlyBuiltDependencies: | |
| - esbuild | |
| - unrs-resolver | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pnpm-workspace.yaml` around lines 15 - 17, The onlyBuiltDependencies entry
currently has a single malformed scalar "esbuild unrs-resolver"; split it into
two distinct list items so each package is its own entry under
onlyBuiltDependencies (e.g., create separate items for esbuild and
unrs-resolver) so strictDepBuilds can correctly exclude each package; update the
onlyBuiltDependencies list to standard YAML list syntax ensuring "esbuild" and
"unrs-resolver" appear as separate entries.
| minimumReleaseAgeExclude: | ||
| - next | ||
| - react | ||
| - '@next/*' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's check if the file exists and see its current content
echo "=== Checking services/cms/pnpm-workspace.yaml ==="
if [ -f services/cms/pnpm-workspace.yaml ]; then
echo "File found. Lines 7-10:"
sed -n '7,10p' services/cms/pnpm-workspace.yaml
echo ""
echo "Full minimumReleaseAgeExclude section:"
sed -n '/minimumReleaseAgeExclude:/,/^[^ -]/p' services/cms/pnpm-workspace.yaml | head -20
else
echo "File not found at services/cms/pnpm-workspace.yaml"
fi
echo ""
echo "=== Running verification script to find all pnpm-workspace.yaml files ==="
python - <<'PY'
import pathlib, re
def parse_min_release_exclude(text: str):
vals = []
in_block = False
for line in text.splitlines():
if re.match(r'^\s*minimumReleaseAgeExclude:\s*$', line):
in_block = True
continue
if in_block:
if re.match(r'^\S', line):
break
m = re.match(r'^\s*-\s*(.+?)\s*$', line)
if m:
vals.append(m.group(1).strip("'\""))
return vals
print("Checking all pnpm-workspace.yaml files:")
for p in sorted(pathlib.Path('.').rglob('pnpm-workspace.yaml')):
vals = parse_min_release_exclude(p.read_text(encoding='utf-8'))
if vals:
print(f"\n{p}:")
print(f" minimumReleaseAgeExclude: {vals}")
if 'react' in vals:
has_react = 'react' in vals
has_react_dom = 'react-dom' in vals
print(f" react={has_react}, react-dom={has_react_dom}")
if has_react and not has_react_dom:
print(f" ⚠️ react present but react-dom missing")
PYRepository: rage/secret-project-331
Length of output: 1434
Include react-dom in the minimumReleaseAgeExclude list.
The react package is excluded from release-age gating, but react-dom is missing. All other workspace files in the repository (services/example-exercise, services/main-frontend, services/quizzes, services/tmc, shared-module/packages/common, and the root workspace) include both packages in their exclusion lists to maintain version synchronization between React and ReactDOM.
Suggested patch
minimumReleaseAgeExclude:
- next
- react
+ - react-dom
- '@next/*'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| minimumReleaseAgeExclude: | |
| - next | |
| - react | |
| - '@next/*' | |
| minimumReleaseAgeExclude: | |
| - next | |
| - react | |
| - react-dom | |
| - '@next/*' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/cms/pnpm-workspace.yaml` around lines 7 - 10, The
minimumReleaseAgeExclude array (key name minimumReleaseAgeExclude) is missing
"react-dom"; update the list that currently contains "next", "react", and
"@next/*" to also include "react-dom" so React and ReactDOM are both excluded
from release-age gating and remain version-synchronized with other workspaces.
| bytes = "1.11.1" | ||
| # Random number generators and other randomness functionality. | ||
| rand = "0.9.2" | ||
| rand = "0.10.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check rand and reqwest declarations in Cargo.toml ==="
cat -n services/headless-lms/utils/Cargo.toml | grep -E "(rand|reqwest)" || echo "Not found in snippet"
echo ""
echo "=== Full Cargo.toml around lines 40 and 77 ==="
wc -l services/headless-lms/utils/Cargo.toml
head -85 services/headless-lms/utils/Cargo.toml | tail -50Repository: rage/secret-project-331
Length of output: 2047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find .query() and .form() usages in services/headless-lms ==="
rg -n --type rust '\.query\s*\(|\.form\s*\(' services/headless-lms -A 2 -B 2
echo ""
echo "=== Find use_rustls_tls usages ==="
rg -n --type rust 'use_rustls_tls' services/headless-lms -A 2 -B 2
echo ""
echo "=== Find rand call sites ==="
rg -n --type rust '(rand::thread_rng|thread_rng\(|rand::rng\(|gen_range\(|gen_bool\(|sample_iter\()' services/headless-lms -A 1 -B 1Repository: rage/secret-project-331
Length of output: 6858
🌐 Web query:
reqwest 0.13.2 feature flags query form methods
💡 Result:
In reqwest 0.13.2, the RequestBuilder helper methods .query(...) and .form(...) are feature-gated and not enabled by default:
.query(...)→ requires crate featurequery(pulls inserde+serde_urlencoded) [1], and the method is “Available on crate featurequeryonly.” [2].form(...)→ requires crate featureform(pulls inserde+serde_urlencoded) [1], and the method is “Available on crate featureformonly.” [2]
Enable them in Cargo.toml like:
[dependencies]
reqwest = { version = "0.13.2", features = ["query", "form"] }Reqwest 0.13 introduced this as a breaking-ish change: “query and form are now crate features, disabled by default.” [3]
Feature flag inventory (0.13.2): query and form are among the non-default feature flags listed on docs.rs. [4]
Sources: [1] [2] [3] [4]
🌐 Web query:
rand 0.10.0 breaking changes from 0.9
💡 Result:
Key breaking changes in rand 0.10.0 (released 2026-02-08) vs 0.9.x (from the 0.10.0 changelog): [1]
OsRngrenamed toSysRng, andOsError→SysError; featureos_rng→sys_rng. [1]- Trait naming shift:
rand::Rng→rand::RngExt, because upstreamrand_corerenamedRngCore→Rng. [1] - Sequence API renames (breaking):
IndexedRandom::choose_multiple→samplechoose_multiple_array→sample_arraychoose_multiple_weighted→sample_weightedSliceChooseIter→IndexedSamplesIteratorRandom::choose_multiple→samplechoose_multiple_fill→sample_fill[1]
- Removed APIs:
SeedableRng::{from_os_rng, try_from_os_rng}removed. [1]ReseedingRngremoved. [1]Cloneremoved forStdRngandReseedingRng(andReseedingRngis gone anyway). [1]
- Feature removals:
small_rngfeature removed (and “nightly” feature removed). [1] - Toolchain bump: now Edition 2024 with MSRV 1.85. [1]
- StdRng backend dependency change:
rand_chachadependency replaced bychacha20(may break direct ChaCha type usage, thoughStdRngoutput is intended to remain the same). [1]
Source: rand CHANGELOG.md for 0.10.0. [1]
🏁 Script executed:
#!/bin/bash
# Check the actual http.rs file to see context around use_rustls_tls
cat -n services/headless-lms/utils/src/http.rs | head -25Repository: rage/secret-project-331
Length of output: 842
🌐 Web query:
reqwest 0.13.2 use_rustls_tls method ClientBuilder
💡 Result:
In reqwest 0.13.2, ClientBuilder::use_rustls_tls() still exists, but it’s deprecated. Use ClientBuilder::tls_backend_rustls() instead. [1]
let client = reqwest::Client::builder()
.tls_backend_rustls() // preferred in 0.13.x
.build()?;Requirements / notes:
- The builder methods are only available when the
rustlsCargo feature is enabled. In 0.13.2,rustlsis enabled by default (unless you turned off default features). [2] - If you did disable default features, enable rustls explicitly:
reqwest = { version = "0.13.2", default-features = false, features = ["rustls"] }Background: in 0.13, reqwest made TLS changes (including “many TLS-related methods renamed” while keeping older names as soft-deprecated). [3]
Sources: ClientBuilder docs (shows use_rustls_tls deprecated → tls_backend_rustls) [1], reqwest 0.13.2 feature list (shows rustls default) [2], 0.13.0 changelog notes about TLS method renames [3].
Replace deprecated use_rustls_tls() with tls_backend_rustls() in reqwest 0.13.2.
In services/headless-lms/utils/src/http.rs line 15, .use_rustls_tls() is deprecated in reqwest 0.13.2. Use .tls_backend_rustls() instead:
reqwest::Client::builder()
.tls_backend_rustls()
.https_only(!http_test_mode)
.build()The rand 0.10.0 usage in the codebase (.sample_iter(), .sample_string(), .random_range(), .shuffle()) is compatible with 0.10.0 and requires no changes. The .query() method found in the codebase is from actix-web, not reqwest, so reqwest's feature-gating of .query() and .form() does not apply here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/headless-lms/utils/Cargo.toml` at line 40, Replace the deprecated
reqwest builder call .use_rustls_tls() with .tls_backend_rustls() in the HTTP
client construction (locate the reqwest::Client::builder() usage in
services/headless-lms/utils/src/http.rs), and ensure the Cargo.toml has reqwest
compiled with the rustls backend feature so the new tls_backend_rustls() method
is available; leave rand = "0.10.0" as-is since no changes are required for its
usage.
| "@types/react": "^19.2.0", | ||
| "@types/react-dom": "^19.2.0", | ||
| "@types/lodash": "^4.17.24", | ||
| "@types/node": "^25.3.3", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, pathlib, re
pkg_path = pathlib.Path("shared-module/packages/common/package.json")
root_ws = pathlib.Path("pnpm-workspace.yaml")
pkg = json.loads(pkg_path.read_text(encoding="utf-8"))
types_node = pkg.get("devDependencies", {}).get("@types/node", "")
m_types = re.match(r'^\^?(\d+)', types_node)
node_major = None
for line in root_ws.read_text(encoding="utf-8").splitlines():
m = re.match(r'^\s*nodeVersion:\s*([0-9]+)\.', line)
if m:
node_major = m.group(1)
break
print(f"@types/node: {types_node}")
print(f"workspace node major: {node_major}")
if m_types and node_major and m_types.group(1) != node_major:
print("MISMATCH: `@types/node` major does not match workspace Node major")
PYRepository: rage/secret-project-331
Length of output: 176
🏁 Script executed:
# Also check the actual files to see their current state
echo "=== pnpm-workspace.yaml ==="
cat pnpm-workspace.yaml
echo -e "\n=== package.json (lines around 77) ==="
sed -n '70,85p' shared-module/packages/common/package.jsonRepository: rage/secret-project-331
Length of output: 1425
Align @types/node major with the pinned runtime major.
The @types/node dependency at line 77 specifies version ^25.3.3 (major 25), but the workspace is pinned to Node 24.14.0. This major version mismatch can hide runtime incompatibilities during type-checking.
Suggested patch
- "@types/node": "^25.3.3",
+ "@types/node": "^24.14.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@types/node": "^25.3.3", | |
| "@types/node": "^24.14.0", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared-module/packages/common/package.json` at line 77, Update the
`@types/node` dependency in package.json to match the runtime major (Node 24) so
type definitions align with the pinned runtime; locate the "@types/node" entry
in shared-module/packages/common/package.json and change its version spec
(currently "^25.3.3") to a 24.x range (for example "^24.0.0" or a more specific
24.y.z you prefer), then run install/typecheck to verify no type mismatches.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
system-tests/src/tests/exams/exam-instructions.spec.ts (2)
40-44: Normalize the empty-block selector; avoid escaped duplicate variant.The escaped aria-label selector at Lines 41-43 is hard to read and maintain. Reuse one canonical selector string for both interactions.
Suggested change
+ const emptyBlockSelector = + `[aria-label="Empty block; start writing or type forward slash to choose a block"]` + await page - .locator(`[aria-label="Empty block; start writing or type forward slash to choose a block"]`) + .locator(emptyBlockSelector) .fill("These are the instructions") await page.getByText("These are the instructions").press("Enter") await page - .locator( - `[aria-label="Empty\\ block\\;\\ start\\ writing\\ or\\ type\\ forward\\ slash\\ to\\ choose\\ a\\ block"]`, - ) + .locator(emptyBlockSelector) .pressSequentially("/")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/exams/exam-instructions.spec.ts` around lines 40 - 44, The aria-label selector used in the locator call is duplicated with hard-to-read escaping; extract the canonical selector string into a single constant (e.g., const EMPTY_BLOCK_SELECTOR = `[aria-label="Empty block; start writing or type forward slash to choose a block"]`) and replace the inline duplicated escaped selector in the locator(...) call and any other uses (like the pressSequentially("/") interaction) with that constant (update the locator invocation that calls .locator(EMPTY_BLOCK_SELECTOR) and then .pressSequentially("/")). Ensure the constant uses the unescaped, human-readable text so both locator and subsequent interactions reuse the same selector.
39-39: Use keyboard Enter instead of text-locator Enter to avoid focus ambiguity.At Line 39,
getByText("These are the instructions").press("Enter")can become flaky if multiple matches appear or focus shifts in the editor. Prefer pressing Enter on the active editor focus.Suggested change
- await page.getByText("These are the instructions").press("Enter") + await page.keyboard.press("Enter")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/exams/exam-instructions.spec.ts` at line 39, The current call page.getByText("These are the instructions").press("Enter") is flaky due to focus ambiguity; instead ensure the editor has focus and use keyboard input: focus the editor locator (e.g., page.getByRole("textbox") or the specific editor locator) via its .focus() method, then invoke page.keyboard.press("Enter") to send the Enter keystroke; update the test to replace the getByText(...).press("Enter") usage with the two-step focus + keyboard.press flow to avoid ambiguous text-locator pressing.services/cms/package.json (1)
30-30: Make@next/bundle-analyzerversioning consistent withnext.Line 30 pins
@next/bundle-analyzerto16.1.6while line 65 allowsnextto float with^16.1.6. These should use the same versioning strategy to avoid unintended version drift.♻️ Proposed fix
- "@next/bundle-analyzer": "16.1.6", + "@next/bundle-analyzer": "^16.1.6",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/cms/package.json` at line 30, The `@next/bundle-analyzer` dependency is pinned to "16.1.6" while the "next" dependency uses a caret range; make their versioning strategy consistent by updating the "@next/bundle-analyzer" entry in package.json to use the same semver style as "next" (e.g., change "@next/bundle-analyzer": "16.1.6" to "@next/bundle-analyzer": "^16.1.6"), or alternatively pin both to exact versions—ensure you update the "@next/bundle-analyzer" line to match whichever approach is used for "next".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/cms/package.json`:
- Line 30: The `@next/bundle-analyzer` dependency is pinned to "16.1.6" while the
"next" dependency uses a caret range; make their versioning strategy consistent
by updating the "@next/bundle-analyzer" entry in package.json to use the same
semver style as "next" (e.g., change "@next/bundle-analyzer": "16.1.6" to
"@next/bundle-analyzer": "^16.1.6"), or alternatively pin both to exact
versions—ensure you update the "@next/bundle-analyzer" line to match whichever
approach is used for "next".
In `@system-tests/src/tests/exams/exam-instructions.spec.ts`:
- Around line 40-44: The aria-label selector used in the locator call is
duplicated with hard-to-read escaping; extract the canonical selector string
into a single constant (e.g., const EMPTY_BLOCK_SELECTOR = `[aria-label="Empty
block; start writing or type forward slash to choose a block"]`) and replace the
inline duplicated escaped selector in the locator(...) call and any other uses
(like the pressSequentially("/") interaction) with that constant (update the
locator invocation that calls .locator(EMPTY_BLOCK_SELECTOR) and then
.pressSequentially("/")). Ensure the constant uses the unescaped, human-readable
text so both locator and subsequent interactions reuse the same selector.
- Line 39: The current call page.getByText("These are the
instructions").press("Enter") is flaky due to focus ambiguity; instead ensure
the editor has focus and use keyboard input: focus the editor locator (e.g.,
page.getByRole("textbox") or the specific editor locator) via its .focus()
method, then invoke page.keyboard.press("Enter") to send the Enter keystroke;
update the test to replace the getByText(...).press("Enter") usage with the
two-step focus + keyboard.press flow to avoid ambiguous text-locator pressing.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
services/cms/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlservices/headless-lms/Cargo.lockis excluded by!**/*.locksystem-tests/src/__screenshots__/quizzes/widget/multiple-choice-clickable.spec.ts/widget-multiple-choice-clickable-answered-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/quizzes/widget/multiple-choice-row-widget.spec.ts/widget-multiple-choice-row-initial-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/sidebar-toggle.spec.ts/block-list-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/sidebar-toggle.spec.ts/block-menu-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/sidebar-toggle.spec.ts/block-properties-desktop-regular.pngis excluded by!**/*.png
📒 Files selected for processing (11)
services/cms/package.jsonservices/cms/src/components/editors/GutenbergEditor.tsxservices/headless-lms/Cargo.tomlservices/headless-lms/server/Cargo.tomlservices/headless-lms/server/src/programs/start_server.rssystem-tests/src/tests/cms/converting-blocks.spec.tssystem-tests/src/tests/course-export.spec.tssystem-tests/src/tests/exams/exam-instructions.spec.tssystem-tests/src/tests/exams/grade-exams-manually.spec.tssystem-tests/src/tests/partners-block.spec.tssystem-tests/src/tests/research-form.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- services/headless-lms/server/Cargo.toml
# Conflicts: # services/headless-lms/Cargo.lock
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
system-tests/src/tests/user-research-consent.spec.ts (1)
57-59:⚠️ Potential issue | 🟠 MajorAssert dialog absence explicitly after re-login.
At Line 58, checking the “Organizations” heading does not prove the consent dialog is gone; this can pass as a false positive. Add an explicit negative assertion for the dialog.
Suggested patch
await page.locator("id=login-button").click() await expect(page.getByRole("heading", { name: "Organizations" })).toBeVisible() + await expect(page.getByTestId("research-consent-dialog")).not.toBeVisible()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/user-research-consent.spec.ts` around lines 57 - 59, After re-login (after interacting with "id=login-button" and asserting the "Organizations" heading), add an explicit negative assertion that the user-research consent dialog is not present (instead of relying on the heading check). Locate the consent dialog using the same selector used elsewhere in the test (e.g., the dialog role or its unique text/label) and assert its absence with a negative expectation (for example using page.getByRole('dialog', { name: /consent/i }) or the project's consent dialog selector and .not.toBeVisible() or .toHaveCount(0)); update the test around the "id=login-button" / "Organizations" assertions to include this explicit negative check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@system-tests/src/tests/user-research-consent.spec.ts`:
- Around line 57-59: After re-login (after interacting with "id=login-button"
and asserting the "Organizations" heading), add an explicit negative assertion
that the user-research consent dialog is not present (instead of relying on the
heading check). Locate the consent dialog using the same selector used elsewhere
in the test (e.g., the dialog role or its unique text/label) and assert its
absence with a negative expectation (for example using page.getByRole('dialog',
{ name: /consent/i }) or the project's consent dialog selector and
.not.toBeVisible() or .toHaveCount(0)); update the test around the
"id=login-button" / "Organizations" assertions to include this explicit negative
check.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
services/headless-lms/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
services/headless-lms/chatbot/Cargo.tomlservices/headless-lms/models/src/courses.rsservices/headless-lms/server/src/controllers/course_material/chatbot.rssystem-tests/src/tests/exams/grade-exams-manually.spec.tssystem-tests/src/tests/user-research-consent.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- system-tests/src/tests/exams/grade-exams-manually.spec.ts
- services/headless-lms/chatbot/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
shared-module/packages/common/package.json (1)
78-78:⚠️ Potential issue | 🟠 Major
@types/nodemajor is still ahead of the pinned Node runtime major.At Line 78,
@types/nodeis^25.3.3while workspace Node is pinned to 24.x in this PR context. This can type-check APIs that are unavailable at runtime.#!/bin/bash set -euo pipefail echo "== Node version pins from workspace files ==" fd 'pnpm-workspace.yaml$' | while read -r f; do echo "-- $f" rg -n 'nodeVersion:' "$f" || true done echo echo "== `@types/node` in shared-module/packages/common/package.json ==" rg -n '"@types/node"\s*:' shared-module/packages/common/package.jsonSuggested patch
- "@types/node": "^25.3.3", + "@types/node": "^24.14.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared-module/packages/common/package.json` at line 78, The `@types/node` dependency in shared-module/packages/common/package.json is set to "^25.3.3" which is ahead of the workspace Node runtime (pinned to 24.x); update the "@types/node" version to a 24.x-compatible release (e.g., "^24.0.0") in that package.json, run your install/type-check (pnpm install && pnpm -w -F common run typecheck or equivalent) to verify no new type errors, and commit the change so the dev typings match the pinned Node runtime.
🧹 Nitpick comments (2)
services/headless-lms/models/src/user_chapter_locking_statuses.rs (1)
236-307: Document side effects inget_or_init_all_for_courseThis public function now does more than “get/init”: it inserts missing statuses and may unlock chapters. Please add a short doc comment describing those side effects and when they happen, so callers don’t treat it as a read-only fetch.
As per coding guidelines: "Require consistent, minimal function documentation... include a short, plain-language description of what the function is for... Comments should add meaning rather than paraphrasing the name or implementation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs` around lines 236 - 307, Add a short doc comment above the public function get_or_init_all_for_course describing its side effects: it will insert missing user_chapter_locking_statuses for the course when course.chapter_locking_enabled is true, and it may call crate::chapters::unlock_first_chapters_for_user to unlock chapters (triggered when course locking is enabled and all existing statuses are ChapterLockingStatus::NotUnlockedYet), so callers should not assume this is a read-only fetch; mention the conditions under which each side effect occurs.services/main-frontend/package.json (1)
60-63: Keepreactandreact-domlocked to the same exact version.At Lines 60-63,
reactis exact (19.2.4) whilereact-domis ranged (^19.2.4). This can drift to different patch/minor builds over time.Suggested patch
- "react-dom": "^19.2.4", + "react-dom": "19.2.4",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/package.json` around lines 60 - 63, The package.json currently pins "react" to an exact version and "react-dom" with a caret, which can allow drift; update the dependency declaration for "react-dom" (the react-dom entry in package.json) to use the exact same version string as "react" (change "^19.2.4" to "19.2.4") so both "react" and "react-dom" are locked to identical versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/cms/package.json`:
- Line 86: The package.json dependency "@types/node" is pinned to ^25.3.3 which
mismatches the workspace Node runtime (nodeVersion 24.14.0); update the
"@types/node" entry in services/cms package.json to use the matching major/minor
version (e.g., ^24.14.0) so the types align with the pinned runtime and avoid
type/runtime incompatibilities.
In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs`:
- Around line 557-642: The test
get_or_init_all_for_course_unlocks_first_chapter_when_all_not_unlocked_yet is
misleading because no exercises are created so multiple chapters could
auto-unlock; fix by making the setup match the intent: create at least one
exercise tied to chapter1 (e.g., insert an exercise for chapter1 via the same
crate::exercises insert helper) so only chapter1 should be unlocked, or
alternatively strengthen the assertions after calling get_or_init_all_for_course
to assert chapter1 is Unlocked and chapter2 remains NotUnlockedYet (use
ChapterLockingStatus variants and the chapter1/chapter2 ids and functions
get_or_init_all_for_course and ensure_not_unlocked_yet_status to locate the
logic).
In `@services/main-frontend/package.json`:
- Line 88: The dev dependency "@types/node" is set to ^25.3.3 but the workspace
Node runtime is pinned to 24.14.0; update the entry for "@types/node" in
package.json to use a 24.x caret range (e.g., "^24.0.0" or "^24.x") so the type
definitions match the pinned Node major and prevent invalid API type-checking
against Node 25.
In `@services/quizzes/package.json`:
- Line 47: The package.json contains an unsupported "export": "next export" npm
script (and next.config.js currently uses output: "standalone"); either remove
the "export" script and any tooling invoking it if you don't need static
exports, or change next.config.js output to "export" and replace the script with
a build-based flow (use "next build" and then serve the generated static output)
so you stop calling the removed CLI; update the package.json scripts accordingly
and ensure next.config.js's output setting (the output property) and any
deployment steps align (refer to the "export" script in package.json and the
output property in next.config.js).
---
Duplicate comments:
In `@shared-module/packages/common/package.json`:
- Line 78: The `@types/node` dependency in
shared-module/packages/common/package.json is set to "^25.3.3" which is ahead of
the workspace Node runtime (pinned to 24.x); update the "@types/node" version to
a 24.x-compatible release (e.g., "^24.0.0") in that package.json, run your
install/type-check (pnpm install && pnpm -w -F common run typecheck or
equivalent) to verify no new type errors, and commit the change so the dev
typings match the pinned Node runtime.
---
Nitpick comments:
In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs`:
- Around line 236-307: Add a short doc comment above the public function
get_or_init_all_for_course describing its side effects: it will insert missing
user_chapter_locking_statuses for the course when course.chapter_locking_enabled
is true, and it may call crate::chapters::unlock_first_chapters_for_user to
unlock chapters (triggered when course locking is enabled and all existing
statuses are ChapterLockingStatus::NotUnlockedYet), so callers should not assume
this is a read-only fetch; mention the conditions under which each side effect
occurs.
In `@services/main-frontend/package.json`:
- Around line 60-63: The package.json currently pins "react" to an exact version
and "react-dom" with a caret, which can allow drift; update the dependency
declaration for "react-dom" (the react-dom entry in package.json) to use the
exact same version string as "react" (change "^19.2.4" to "19.2.4") so both
"react" and "react-dom" are locked to identical versions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
services/cms/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlservices/example-exercise/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlservices/main-frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlservices/quizzes/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlservices/tmc/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlshared-module/packages/common/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
services/cms/package.jsonservices/example-exercise/package.jsonservices/headless-lms/models/src/user_chapter_locking_statuses.rsservices/main-frontend/package.jsonservices/quizzes/package.jsonservices/tmc/package.jsonshared-module/packages/common/package.jsonshared-module/packages/common/src/hooks/useConcurrencyThrottle.tsshared-module/packages/common/tests/setup-jest.js
🚧 Files skipped from review as they are similar to previous changes (1)
- services/example-exercise/package.json
| "@types/node": "^24.6.2", | ||
| "@types/jsdom": "^28.0.0", | ||
| "@types/lodash": "^4.17.24", | ||
| "@types/node": "^25.3.3", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Workspace configuration files =="
fd -e yaml -e yml | grep -E '(workspace|pnpm)' | head -20
echo
echo "== Content of pnpm-workspace.yaml if exists =="
if [ -f "pnpm-workspace.yaml" ]; then
cat pnpm-workspace.yaml
fi
echo
echo "== services/cms/package.json - `@types/node` line =="
rg -n '"@types/node"' services/cms/package.json -A 1 -B 1
echo
echo "== services/cms/package.json line 86 context =="
sed -n '84,88p' services/cms/package.jsonRepository: rage/secret-project-331
Length of output: 1923
Align @types/node major version with the pinned Node runtime.
Line 86 pins @types/node to ^25.3.3, but the workspace configuration (pnpm-workspace.yaml) specifies nodeVersion: 24.14.0. The types major version should match the runtime major version to prevent type incompatibilities and mask runtime issues. Change to ^24.14.0.
Suggested patch
- "@types/node": "^25.3.3",
+ "@types/node": "^24.14.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@types/node": "^25.3.3", | |
| "@types/node": "^24.14.0", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/cms/package.json` at line 86, The package.json dependency
"@types/node" is pinned to ^25.3.3 which mismatches the workspace Node runtime
(nodeVersion 24.14.0); update the "@types/node" entry in services/cms
package.json to use the matching major/minor version (e.g., ^24.14.0) so the
types align with the pinned runtime and avoid type/runtime incompatibilities.
| async fn get_or_init_all_for_course_unlocks_first_chapter_when_all_not_unlocked_yet() { | ||
| insert_data!(:tx, :user, :org, course: course, instance: _instance, :course_module); | ||
|
|
||
| let chapter1 = crate::chapters::insert( | ||
| tx.as_mut(), | ||
| PKeyPolicy::Generate, | ||
| &crate::chapters::NewChapter { | ||
| name: "Chapter 1".to_string(), | ||
| color: None, | ||
| course_id: course, | ||
| chapter_number: 1, | ||
| front_page_id: None, | ||
| opens_at: None, | ||
| deadline: None, | ||
| course_module_id: Some(course_module.id), | ||
| }, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // insert a second chapter to ensure only the first is auto-unlocked | ||
| let chapter2 = crate::chapters::insert( | ||
| tx.as_mut(), | ||
| PKeyPolicy::Generate, | ||
| &crate::chapters::NewChapter { | ||
| name: "Chapter 2".to_string(), | ||
| color: None, | ||
| course_id: course, | ||
| chapter_number: 2, | ||
| front_page_id: None, | ||
| opens_at: None, | ||
| deadline: None, | ||
| course_module_id: Some(course_module.id), | ||
| }, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // Enable chapter locking for the course | ||
| let existing_course = crate::courses::get_course(tx.as_mut(), course) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| crate::courses::update_course( | ||
| tx.as_mut(), | ||
| course, | ||
| crate::courses::CourseUpdate { | ||
| name: existing_course.name, | ||
| description: existing_course.description, | ||
| is_draft: existing_course.is_draft, | ||
| is_test_mode: existing_course.is_test_mode, | ||
| can_add_chatbot: existing_course.can_add_chatbot, | ||
| is_unlisted: existing_course.is_unlisted, | ||
| is_joinable_by_code_only: existing_course.is_joinable_by_code_only, | ||
| ask_marketing_consent: existing_course.ask_marketing_consent, | ||
| flagged_answers_threshold: existing_course.flagged_answers_threshold.unwrap_or(1), | ||
| flagged_answers_skip_manual_review_and_allow_retry: existing_course | ||
| .flagged_answers_skip_manual_review_and_allow_retry, | ||
| closed_at: existing_course.closed_at, | ||
| closed_additional_message: existing_course.closed_additional_message, | ||
| closed_course_successor_id: existing_course.closed_course_successor_id, | ||
| chapter_locking_enabled: true, | ||
| }, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // Ensure we start from a state where all chapters are not_unlocked_yet | ||
| let _ = ensure_not_unlocked_yet_status(tx.as_mut(), user, chapter1, course) | ||
| .await | ||
| .unwrap(); | ||
| let _ = ensure_not_unlocked_yet_status(tx.as_mut(), user, chapter2, course) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let statuses = get_or_init_all_for_course(tx.as_mut(), user, course) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!(!statuses.is_empty()); | ||
| assert!( | ||
| statuses | ||
| .iter() | ||
| .any(|s| s.chapter_id == chapter1 && s.status == ChapterLockingStatus::Unlocked) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test intent is misleading and under-asserted
The test name/comment says only the first chapter should auto-unlock, but this setup creates no exercises, so unlock behavior can include multiple initial chapters. The assertion also only checks chapter1, which doesn’t validate the stated intent.
✅ Suggested test adjustment
- async fn get_or_init_all_for_course_unlocks_first_chapter_when_all_not_unlocked_yet() {
+ async fn get_or_init_all_for_course_unlocks_initial_chapters_when_all_not_unlocked_yet() {
@@
- // insert a second chapter to ensure only the first is auto-unlocked
+ // insert a second chapter to verify initial auto-unlock behavior
@@
assert!(
statuses
.iter()
.any(|s| s.chapter_id == chapter1 && s.status == ChapterLockingStatus::Unlocked)
);
+ assert!(
+ statuses
+ .iter()
+ .any(|s| s.chapter_id == chapter2 && s.status == ChapterLockingStatus::Unlocked)
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn get_or_init_all_for_course_unlocks_first_chapter_when_all_not_unlocked_yet() { | |
| insert_data!(:tx, :user, :org, course: course, instance: _instance, :course_module); | |
| let chapter1 = crate::chapters::insert( | |
| tx.as_mut(), | |
| PKeyPolicy::Generate, | |
| &crate::chapters::NewChapter { | |
| name: "Chapter 1".to_string(), | |
| color: None, | |
| course_id: course, | |
| chapter_number: 1, | |
| front_page_id: None, | |
| opens_at: None, | |
| deadline: None, | |
| course_module_id: Some(course_module.id), | |
| }, | |
| ) | |
| .await | |
| .unwrap(); | |
| // insert a second chapter to ensure only the first is auto-unlocked | |
| let chapter2 = crate::chapters::insert( | |
| tx.as_mut(), | |
| PKeyPolicy::Generate, | |
| &crate::chapters::NewChapter { | |
| name: "Chapter 2".to_string(), | |
| color: None, | |
| course_id: course, | |
| chapter_number: 2, | |
| front_page_id: None, | |
| opens_at: None, | |
| deadline: None, | |
| course_module_id: Some(course_module.id), | |
| }, | |
| ) | |
| .await | |
| .unwrap(); | |
| // Enable chapter locking for the course | |
| let existing_course = crate::courses::get_course(tx.as_mut(), course) | |
| .await | |
| .unwrap(); | |
| crate::courses::update_course( | |
| tx.as_mut(), | |
| course, | |
| crate::courses::CourseUpdate { | |
| name: existing_course.name, | |
| description: existing_course.description, | |
| is_draft: existing_course.is_draft, | |
| is_test_mode: existing_course.is_test_mode, | |
| can_add_chatbot: existing_course.can_add_chatbot, | |
| is_unlisted: existing_course.is_unlisted, | |
| is_joinable_by_code_only: existing_course.is_joinable_by_code_only, | |
| ask_marketing_consent: existing_course.ask_marketing_consent, | |
| flagged_answers_threshold: existing_course.flagged_answers_threshold.unwrap_or(1), | |
| flagged_answers_skip_manual_review_and_allow_retry: existing_course | |
| .flagged_answers_skip_manual_review_and_allow_retry, | |
| closed_at: existing_course.closed_at, | |
| closed_additional_message: existing_course.closed_additional_message, | |
| closed_course_successor_id: existing_course.closed_course_successor_id, | |
| chapter_locking_enabled: true, | |
| }, | |
| ) | |
| .await | |
| .unwrap(); | |
| // Ensure we start from a state where all chapters are not_unlocked_yet | |
| let _ = ensure_not_unlocked_yet_status(tx.as_mut(), user, chapter1, course) | |
| .await | |
| .unwrap(); | |
| let _ = ensure_not_unlocked_yet_status(tx.as_mut(), user, chapter2, course) | |
| .await | |
| .unwrap(); | |
| let statuses = get_or_init_all_for_course(tx.as_mut(), user, course) | |
| .await | |
| .unwrap(); | |
| assert!(!statuses.is_empty()); | |
| assert!( | |
| statuses | |
| .iter() | |
| .any(|s| s.chapter_id == chapter1 && s.status == ChapterLockingStatus::Unlocked) | |
| ); | |
| } | |
| async fn get_or_init_all_for_course_unlocks_initial_chapters_when_all_not_unlocked_yet() { | |
| insert_data!(:tx, :user, :org, course: course, instance: _instance, :course_module); | |
| let chapter1 = crate::chapters::insert( | |
| tx.as_mut(), | |
| PKeyPolicy::Generate, | |
| &crate::chapters::NewChapter { | |
| name: "Chapter 1".to_string(), | |
| color: None, | |
| course_id: course, | |
| chapter_number: 1, | |
| front_page_id: None, | |
| opens_at: None, | |
| deadline: None, | |
| course_module_id: Some(course_module.id), | |
| }, | |
| ) | |
| .await | |
| .unwrap(); | |
| // insert a second chapter to verify initial auto-unlock behavior | |
| let chapter2 = crate::chapters::insert( | |
| tx.as_mut(), | |
| PKeyPolicy::Generate, | |
| &crate::chapters::NewChapter { | |
| name: "Chapter 2".to_string(), | |
| color: None, | |
| course_id: course, | |
| chapter_number: 2, | |
| front_page_id: None, | |
| opens_at: None, | |
| deadline: None, | |
| course_module_id: Some(course_module.id), | |
| }, | |
| ) | |
| .await | |
| .unwrap(); | |
| // Enable chapter locking for the course | |
| let existing_course = crate::courses::get_course(tx.as_mut(), course) | |
| .await | |
| .unwrap(); | |
| crate::courses::update_course( | |
| tx.as_mut(), | |
| course, | |
| crate::courses::CourseUpdate { | |
| name: existing_course.name, | |
| description: existing_course.description, | |
| is_draft: existing_course.is_draft, | |
| is_test_mode: existing_course.is_test_mode, | |
| can_add_chatbot: existing_course.can_add_chatbot, | |
| is_unlisted: existing_course.is_unlisted, | |
| is_joinable_by_code_only: existing_course.is_joinable_by_code_only, | |
| ask_marketing_consent: existing_course.ask_marketing_consent, | |
| flagged_answers_threshold: existing_course.flagged_answers_threshold.unwrap_or(1), | |
| flagged_answers_skip_manual_review_and_allow_retry: existing_course | |
| .flagged_answers_skip_manual_review_and_allow_retry, | |
| closed_at: existing_course.closed_at, | |
| closed_additional_message: existing_course.closed_additional_message, | |
| closed_course_successor_id: existing_course.closed_course_successor_id, | |
| chapter_locking_enabled: true, | |
| }, | |
| ) | |
| .await | |
| .unwrap(); | |
| // Ensure we start from a state where all chapters are not_unlocked_yet | |
| let _ = ensure_not_unlocked_yet_status(tx.as_mut(), user, chapter1, course) | |
| .await | |
| .unwrap(); | |
| let _ = ensure_not_unlocked_yet_status(tx.as_mut(), user, chapter2, course) | |
| .await | |
| .unwrap(); | |
| let statuses = get_or_init_all_for_course(tx.as_mut(), user, course) | |
| .await | |
| .unwrap(); | |
| assert!(!statuses.is_empty()); | |
| assert!( | |
| statuses | |
| .iter() | |
| .any(|s| s.chapter_id == chapter1 && s.status == ChapterLockingStatus::Unlocked) | |
| ); | |
| assert!( | |
| statuses | |
| .iter() | |
| .any(|s| s.chapter_id == chapter2 && s.status == ChapterLockingStatus::Unlocked) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs` around
lines 557 - 642, The test
get_or_init_all_for_course_unlocks_first_chapter_when_all_not_unlocked_yet is
misleading because no exercises are created so multiple chapters could
auto-unlock; fix by making the setup match the intent: create at least one
exercise tied to chapter1 (e.g., insert an exercise for chapter1 via the same
crate::exercises insert helper) so only chapter1 should be unlocked, or
alternatively strengthen the assertions after calling get_or_init_all_for_course
to assert chapter1 is Unlocked and chapter2 remains NotUnlockedYet (use
ChapterLockingStatus variants and the chapter1/chapter2 ids and functions
get_or_init_all_for_course and ensure_not_unlocked_yet_status to locate the
logic).
| "@types/react": "^19.2.0", | ||
| "@types/react-dom": "^19.2.0", | ||
| "@types/lodash": "^4.17.24", | ||
| "@types/node": "^25.3.3", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node version pins =="
fd 'pnpm-workspace.yaml$' | while read -r f; do
echo "-- $f"
rg -n 'nodeVersion:' "$f" || true
done
echo
echo "== `@types/node` in services/main-frontend/package.json =="
rg -n '"@types/node"\s*:' services/main-frontend/package.jsonRepository: rage/secret-project-331
Length of output: 807
@types/node major should match the pinned Node runtime major.
At line 88, @types/node is ^25.3.3 (Node 25.x) while the workspace config is pinned to Node 24.14.0; this can allow invalid runtime API usage during type-checking. Update @types/node to ^24.x or align the runtime version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/main-frontend/package.json` at line 88, The dev dependency
"@types/node" is set to ^25.3.3 but the workspace Node runtime is pinned to
24.14.0; update the entry for "@types/node" in package.json to use a 24.x caret
range (e.g., "^24.0.0" or "^24.x") so the type definitions match the pinned Node
major and prevent invalid API type-checking against Node 25.
| "katex": "^0.16.33", | ||
| "lodash": "^4.17.23", | ||
| "monaco-editor": "^0.55.1", | ||
| "next": "16.1.6", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current package settings
echo "=== Checking Next.js version in package.json ==="
rg -n '"next"\s*:\s*"16\.1\.6"' services/quizzes/package.json || echo "Version not found"
echo -e "\n=== Checking for 'next export' in scripts ==="
rg -n '"export"\s*:\s*"next export"' services/quizzes/package.json || echo "Export script not found"
echo -e "\n=== Checking for next.config files ==="
fd -HI '^next\.config\.(js|mjs|cjs|ts)$' services/quizzes 2>/dev/null || echo "No next.config found in services/quizzes"
# Check if output: "export" is configured anywhere in the services directory
echo -e "\n=== Searching for output export configuration ==="
if [ -d "services/quizzes" ]; then
find services/quizzes -maxdepth 2 -name "next.config.*" -exec cat {} + 2>/dev/null | head -50 || echo "No config files found"
fiRepository: rage/secret-project-331
Length of output: 1474
🌐 Web query:
Next.js 16.1.x static export CLI command documentation - is "next export" still supported or deprecated
💡 Result:
In Next.js 16.1.x, the next export CLI command is not supported (it was deprecated in v13.3.0 and removed in v14.0.0). Instead, static exports are done by setting output: 'export' in next.config.js and then running next build, which produces the static output (default out/). [1]
If you still have scripts like next build && next export, you should remove next export and rely on output: 'export' + next build. [1]
Sources
[1] Next.js Docs – “Static Exports” (Last updated Feb 27, 2026), incl. version history for next export deprecation/removal: (nextjs.org)
Citations:
The next export command is not supported in Next.js 16.1.6 and will fail—this needs to be fixed.
Next.js removed the next export CLI command in v14.0.0. The "export": "next export" script on line 8 is incompatible with v16.1.6. Additionally, next.config.js is configured with output: "standalone" (for server deployments), not output: "export" (for static exports).
Either remove the export script if static exports aren't needed, or change output: "standalone" to output: "export" in next.config.js and use next build to generate static output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/quizzes/package.json` at line 47, The package.json contains an
unsupported "export": "next export" npm script (and next.config.js currently
uses output: "standalone"); either remove the "export" script and any tooling
invoking it if you don't need static exports, or change next.config.js output to
"export" and replace the script with a build-based flow (use "next build" and
then serve the generated static output) so you stop calling the removed CLI;
update the package.json scripts accordingly and ensure next.config.js's output
setting (the output property) and any deployment steps align (refer to the
"export" script in package.json and the output property in next.config.js).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/headless-lms/models/src/user_chapter_locking_statuses.rs (2)
209-307: Add rustdoc for the new public chapter-locking APIs.
complete_and_lock_chapterandget_or_init_all_for_courseintroduce important side effects (state mutation + initialization/unlock behavior) but are undocumented. Add short docs clarifying intent and side effects.As per coding guidelines, “Require consistent, minimal function documentation… Public interfaces should have clearer and more comprehensive documentation.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs` around lines 209 - 307, The two public functions complete_and_lock_chapter and get_or_init_all_for_course lack rustdoc describing intent and side effects; add concise /// comments above each function that state what the function does, its parameters, the side effects (DB mutations, potential creation of rows and unlocking behavior), the returned value (UserChapterLockingStatus or Vec<UserChapterLockingStatus>), and error/ not-found behaviors (ModelError on failure); ensure docs mention when course chapter locking must be enabled and that get_or_init_all_for_course will insert missing rows and may call unlock_first_chapters_for_user, and keep each docblock short (one to three sentences) following project doc style.
245-260: Align ON CONFLICT target to use constraint name for consistency.The insert paths at lines 245–260 and nearby use
ON CONFLICT (user_id, chapter_id, deleted_at)while other mutations in the same file useON CONFLICT ON CONSTRAINT idx_user_chapter_locking_statuses_user_chapter_active. The constraint definition isUNIQUE NULLS NOT DISTINCT (user_id, chapter_id, deleted_at), so both forms are functionally equivalent—PostgreSQL correctly infers the same constraint either way. However, using the explicit constraint name form consistently throughout improves code clarity and reduces reliance on inference.Suggested changes
-ON CONFLICT (user_id, chapter_id, deleted_at) DO NOTHING +ON CONFLICT ON CONSTRAINT idx_user_chapter_locking_statuses_user_chapter_active DO NOTHINGApply the same change to all insert operations in this file (lines 245 and nearby).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs` around lines 245 - 260, Replace the positional ON CONFLICT target with the explicit constraint name to match other mutations: in the INSERT query that currently uses "ON CONFLICT (user_id, chapter_id, deleted_at) DO NOTHING" (the query that inserts into user_chapter_locking_statuses and selects from chapters) change it to "ON CONFLICT ON CONSTRAINT idx_user_chapter_locking_statuses_user_chapter_active DO NOTHING"; apply the same replacement to the other insert statements in this file so all inserts consistently reference idx_user_chapter_locking_statuses_user_chapter_active instead of the column list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/headless-lms/models/src/user_chapter_locking_statuses.rs`:
- Around line 209-307: The two public functions complete_and_lock_chapter and
get_or_init_all_for_course lack rustdoc describing intent and side effects; add
concise /// comments above each function that state what the function does, its
parameters, the side effects (DB mutations, potential creation of rows and
unlocking behavior), the returned value (UserChapterLockingStatus or
Vec<UserChapterLockingStatus>), and error/ not-found behaviors (ModelError on
failure); ensure docs mention when course chapter locking must be enabled and
that get_or_init_all_for_course will insert missing rows and may call
unlock_first_chapters_for_user, and keep each docblock short (one to three
sentences) following project doc style.
- Around line 245-260: Replace the positional ON CONFLICT target with the
explicit constraint name to match other mutations: in the INSERT query that
currently uses "ON CONFLICT (user_id, chapter_id, deleted_at) DO NOTHING" (the
query that inserts into user_chapter_locking_statuses and selects from chapters)
change it to "ON CONFLICT ON CONSTRAINT
idx_user_chapter_locking_statuses_user_chapter_active DO NOTHING"; apply the
same replacement to the other insert statements in this file so all inserts
consistently reference idx_user_chapter_locking_statuses_user_chapter_active
instead of the column list.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores