Conversation
|
The preview deployment for frontend-testownik-dev is ready. 🟢 Open Preview | Open Build Logs | Open Application Logs Last updated at: 2026-09-06 15:22:32 CET |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Antoni-Czaplicki
requested changes
Jun 18, 2026
Antoni-Czaplicki
left a comment
Antoni-Czaplicki
left a comment
Member
There was a problem hiding this comment.
Note
AI-generated review comment.
I reviewed the Markdown/LaTeX editor PR against current dev, including the editor integration, quiz display rendering, tests, build/typecheck/lint behavior, and mergeability.
Main blockers:
- The PR is currently not mergeable. GitHub reports it as
CONFLICTING; the conflict areas includepnpm-lock.yamlandsrc/components/ai/ai-explain-card.tsx. pnpm lintfails locally with 18@typescript-eslint/no-unsafe-*route typing errors after this PR's dependency/lockfile refresh.pnpm typecheckpasses afterpnpm next typegen, and the create-quiz page tests pass with a larger timeout, but lint needs to be clean before merge.- The OverType preview/value synchronization issue called out inline can make existing editor content render blank/stale.
Needed to merge: rebase/merge current dev and resolve conflicts, fix the lint regression or narrow the lockfile/tooling bump, fix the OverType controlled-value/initial-preview behavior, and add a targeted regression test for existing quiz text or parent-driven value updates in the editor.
olios123
reviewed
Jul 13, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/components/overtype-editor.tsx:230
styleIdincludes a leading#and is then assigned tostyle.id, producing an invalid HTML id value and breaking the selector logic (querySelector expects#id, but the element id should not include#).
const styleId = "#ot-math-styles";
if (document.querySelector(styleId) === null) {
const style = document.createElement("style");
style.id = styleId;
style.textContent = `
src/components/overtype-editor.tsx:292
- The cleanup removes the shared
<style>element fromdocument.head. Since multipleOverTypeEditorinstances can be mounted at once (question + multiple answers), unmounting any one instance can remove styles still needed by the others.
return () => {
observer.disconnect();
document.querySelector(styleId)?.remove();
};
.github/workflows/ci.yml:3
- The workflow no longer pins minimal token permissions. Without an explicit
permissions:block, the defaultGITHUB_TOKENpermissions depend on repository/org settings and may be broader than necessary.
name: CI
on:
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (8)
src/components/overtype-editor.tsx:229
styleIdincludes a leading#and is assigned tostyle.id, which makes the id invalid and breaks both the existence check and cleanup (querySelector("#...")won’t match an element whose id literally contains#).
const styleId = "#ot-math-styles";
if (document.querySelector(styleId) === null) {
const style = document.createElement("style");
style.id = styleId;
src/components/overtype-editor.tsx:292
- This effect injects a global
<style>tag. Removing it on unmount can break otherOverTypeEditorinstances that are still mounted (e.g., question + multiple answers). It’s safer to leave the style in place (or implement ref-counting) rather than always removing it.
return () => {
observer.disconnect();
document.querySelector(styleId)?.remove();
};
src/components/overtype-editor.tsx:251
processMathreturns from the whole function when it encounters a non-text child or a text child without$, which prevents processing any later siblings in the same node. This should skip that child and continue.
if (child.nodeType !== Node.TEXT_NODE) {
return;
}
const text = child.textContent ?? "";
if (!text.includes("$")) {
return;
}
.github/workflows/ci.yml:3
- The workflow no longer pins
permissions, so it will inherit the repository/org default token permissions (which may be broader than needed). Re-adding minimal permissions reduces the blast radius if a workflow is compromised.
name: CI
on:
package.json:60
is-url-superbis added as a dependency but is not imported/used anywhere in the repository. Keeping unused deps increases install size and audit surface; consider removing it (and updating the lockfile) unless it’s used in a follow-up commit.
"input-otp": "^1.4.2",
"is-url-superb": "^6.1.0",
"jose": "^6.1.3",
package.json:86
remark-stringifyis added to dependencies but there are no imports/usages in the repo. If it’s not needed for runtime, consider removing it (and updating the lockfile) to reduce bundle/install surface.
"remark-gfm": "^4.0.1",
"remark-math": "^6.0.0",
"remark-stringify": "^11.0.0",
"seedrandom": "^3.0.5",
package.json:121
remark-parseis added to devDependencies but there are no imports/usages in the repo. If it’s not needed, consider removing it (and updating the lockfile) to keep tooling deps minimal.
"msw": "^2.12.7",
"prettier": "^3.7.4",
"remark-parse": "^11.0.0",
"server-only": "^0.0.1",
src/hooks/use-overtype.ts:82
handleInputcreates a plain object and casts it toReact.ChangeEvent, so handlers that rely onpreventDefault,stopPropagation,nativeEvent, etc. can break at runtime. Since you already have the nativeEventfromaddEventListener, pass it through (casted) instead of fabricating a partial object.
onChangeRef.current?.({
target,
currentTarget: target,
} as React.ChangeEvent<HTMLTextAreaElement>);
};
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
PR zamyka #225, ale w zmianach nie widać implementacji pickera symboli matematycznych wymaganego w opisie issue.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+118
to
+138
| { | ||
| label: "Matematyka", | ||
| actions: [ | ||
| { | ||
| label: "Wzór w tekście", | ||
| icon: SigmaIcon, | ||
| apply: (ta) => { | ||
| markdownActions.applyCustomFormat(ta, { prefix: "$", suffix: "$" }); | ||
| }, | ||
| }, | ||
| { | ||
| label: "Blok wzoru", | ||
| icon: PiIcon, | ||
| apply: (ta) => { | ||
| markdownActions.applyCustomFormat(ta, { | ||
| prefix: "\n$$\n", | ||
| suffix: "\n$$\n", | ||
| }); | ||
| }, | ||
| }, | ||
| ], |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Wykryte problemy w useOverType (idempotencja highlightowania i niespójna synchronizacja opcji) oraz potencjalna niezgodność z zakresem zamykanego #225 wymagają korekt przed akceptacją.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
src/components/overtype-editor.tsx:122
- The PR description closes #225, but the referenced issue explicitly calls for a math symbol picker and a dedicated raw/source toggle (</>) for switching between live preview and Markdown/LaTeX source. The current toolbar only applies wrappers (e.g.
$…$ ,$$…$$ ) and the UI provides a preview popover, not a source/preview toggle or symbol picker.
src/hooks/use-overtype.ts:29 - highlightMath() can re-process text nodes that are already inside a previously inserted .ot-math span, which makes the function non-idempotent (a second onRender call can nest .ot-math spans). Excluding .ot-math from the walker filter keeps repeated renders stable.
src/hooks/use-overtype.ts:142 - useOverType() exposes theme/minHeight/maxHeight/autoResize as options, but the hook only synchronizes value and placeholder after mount. If these props change, the editor instance will keep the initial sizing/theme options, which is surprising for a hook that accepts controlled options.
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
PR oznacza CLOSES #225, ale w implementacji brakuje wymaganego w issue pickera symboli matematycznych (co najmniej doprecyzowania scope lub uzupełnienia funkcjonalności).
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
src/components/overtype-editor.tsx:137
- PR description closes #225, but Issue #225 explicitly calls for a math symbol picker (e.g. a “∑/fx” button opening a palette of common symbols and inserting LaTeX templates like \frac{}{}). The current implementation only adds actions to wrap selection with
$…$/$$…$$, so closing the issue looks premature unless the picker is intentionally out of scope.
{
label: "Matematyka",
actions: [
{
label: "Wzór w tekście",
icon: SigmaIcon,
apply: (ta) => {
markdownActions.applyCustomFormat(ta, { prefix: "$", suffix: "$" });
},
},
{
label: "Blok wzoru",
icon: PiIcon,
apply: (ta) => {
markdownActions.applyCustomFormat(ta, {
prefix: "\n$$\n",
suffix: "\n$$\n",
});
},
},
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
# Conflicts: # apps/web/src/__tests__/overtype-editor.spec.tsx # apps/web/src/components/overtype-editor.tsx # apps/web/src/hooks/use-overtype.ts # package.json
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Podsumowanie
Dodano edytor markdown wykorzystujący logikę istniejącego już edytora Overtype. Edytor jest dostosowany zarówno do PC jak i urządzeń mobilnych.
Poprawiono renderowanie pytań w quizie, ze względu na złe wyświetlanie markdownu w starej wersji.
Dodano
OverTypeEditorz menu formatowania i podglądemuseOverTypeinicjalizujący referencję do edytoraquestion-card.tsxZmiana w wyświetlaniu pytania
Wyświetlanie pytań obecnie usuwało formatowanie z pierwszej linii tekstu, przez co tekst nie wyświetlał się poprawnie
Po wprowadzeniu zmian przeniosłem tekst "Pytanie" wraz z numerem pytania w celu poprawnego formatowania do osobnego elemntu, rozdzielając treść od numeracji.
Edytor
Pytania i odpowiedzi domyślnie pokazują wyrenderowaną treść Markdown/LaTeX, tak jak wcześniej. Kliknięcie treści od razu otwiera edycję i pokazuje jej narzędzia, bez osobnego przycisku „Edytuj”.
Weryfikacja
CLOSES #225