ui: drop blueprintjs 6#1073
Conversation
WalkthroughThis PR migrates many UI components from Blueprint.js to Mantine across packages/ui-default and some frontend parts, replaces Popover/Modal/Tabs/Inputs with Mantine equivalents, and wraps roots with MantineProvider. It removes and adds multiple CSS/stylus rules, introduces postcss-preset-mantine and bp6-compat.css, updates package dependencies (React/ReactDOM, Mantine, mantine-contextmenu), and refactors several components (history, reaction, notification, upload, problemconfig tree/forms/testcase management). Reducer logic for testcase moves and __cases deduplication was adjusted. No public API signatures were added or removed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ui-default/components/discussion/reaction.page.tsx (1)
56-66: Add keyboard support to emoji items.The emoji selection items are
<div>elements withonClickhandlers but lack keyboard accessibility. Users navigating with keyboard cannot select emoji reactions.Apply this diff to add keyboard support:
<div key={emoji} className={`medium-${12 / elesPerRow} small-${12 / elesPerRow} columns popover-reaction-item`} + role="button" + tabIndex={0} + aria-label={`React with ${emoji}`} onClick={() => handleEmojiClick(payload, emoji, ele).then(() => { updateOpen(false); updateTrigger(false); })} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleEmojiClick(payload, emoji, ele).then(() => { + updateOpen(false); + updateTrigger(false); + }); + } + }} >packages/ui-default/components/problemconfig/tree/AddTestcase.tsx (1)
21-54: Block saving until both files are validRight now each effect overwrites
validwith only its own lookup. If a user picks the output first,validbecomes truthy whileinputis still empty, so the Save button activates and we end up dispatching a testcase with no input file. Please derive validity from both sides at once so we never emit an incomplete pair. One way is to drop the per-effect assignments and add a combined check, e.g.:@@ - setValid(inputValid); if (inputValid && autoOutput) { const filename = input.substring(0, input.lastIndexOf('.')); let outputFile = ''; @@ }, [input]); @@ - setValid(outputValid); if (outputValid && autoInput) { const filename = output.substring(0, output.lastIndexOf('.')); if (testdata.find((i) => i.name === `${filename}.in`)) { @@ }, [output]); + + useEffect(() => { + const inputValid = testdata.some((i) => i.name === input); + const outputValid = testdata.some((i) => i.name === output); + setValid(inputValid && outputValid); + }, [input, output, testdata]);
🧹 Nitpick comments (2)
packages/ui-default/components/problemconfig/tree/Testcase.tsx (2)
25-33: Redundant condition in prefix check.The condition
c.input.startsWith(prefix)is always true sinceprefixis derived fromc.input.slice(0, i). Only thec.output.startsWith(prefix)check is necessary.Apply this diff to simplify the logic:
const minlength = Math.min(c.input.length, c.output.length); for (let i = minlength; i >= 0; i--) { const prefix = c.input.slice(0, i); - if (c.input.startsWith(prefix) && c.output.startsWith(prefix)) { + if (c.output.startsWith(prefix)) { display = `${prefix}(${c.input.substring(i)}/${c.output.substring(i)})`; break; }
67-74: Consider optimizing dependency array.Using
JSON.stringify(cases)in the dependency array works but is not optimal for performance. Consider using a stable reference oruseMemoto track changes more efficiently.Example optimization:
+ const casesKey = React.useMemo(() => cases.map(c => `${c.input}|${c.output}`).join(','), [cases]); const [collected, drag] = useDrag(() => ({ type: 'cases', collect: (monitor) => ({ isDragging: monitor.isDragging(), }), canDrag: props.selected, item: { cases, subtaskId }, - }), [JSON.stringify(cases), subtaskId]); + }), [casesKey, subtaskId]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
packages/components/frontend/autocomplete/autocomplete.scss(0 hunks)packages/ui-default/components/datepicker/datepicker.styl(0 hunks)packages/ui-default/components/discussion/history.page.tsx(3 hunks)packages/ui-default/components/discussion/reaction.page.tsx(3 hunks)packages/ui-default/components/notification/index.ts(2 hunks)packages/ui-default/components/notification/notification.page.styl(1 hunks)packages/ui-default/components/problem/tag.page.styl(1 hunks)packages/ui-default/components/problemconfig/ProblemConfigForm.tsx(4 hunks)packages/ui-default/components/problemconfig/ProblemConfigTree.tsx(4 hunks)packages/ui-default/components/problemconfig/ProblemType.tsx(2 hunks)packages/ui-default/components/problemconfig/index.tsx(2 hunks)packages/ui-default/components/problemconfig/reducer/config.ts(2 hunks)packages/ui-default/components/problemconfig/tree/AddTestcase.tsx(3 hunks)packages/ui-default/components/problemconfig/tree/SubtaskSettings.tsx(3 hunks)packages/ui-default/components/problemconfig/tree/Testcase.tsx(4 hunks)packages/ui-default/components/upload.tsx(3 hunks)packages/ui-default/package.json(2 hunks)packages/ui-default/pages/problem_config.page.styl(0 hunks)packages/ui-default/pages/problem_config.page.tsx(4 hunks)packages/ui-default/postcss.config.js(1 hunks)packages/ui-default/templates/contest_detail.html(1 hunks)packages/ui-default/templates/problem_detail.html(1 hunks)packages/ui-default/theme/bp6-compat.css(1 hunks)packages/ui-default/theme/dark.styl(0 hunks)packages/ui-default/theme/default.js(2 hunks)
💤 Files with no reviewable changes (4)
- packages/components/frontend/autocomplete/autocomplete.scss
- packages/ui-default/theme/dark.styl
- packages/ui-default/components/datepicker/datepicker.styl
- packages/ui-default/pages/problem_config.page.styl
🧰 Additional context used
🧬 Code graph analysis (12)
packages/ui-default/components/upload.tsx (2)
packages/ui-default/components/dialog/index.tsx (1)
Dialog(11-63)packages/ui-default/utils/base.ts (2)
tpl(39-58)i18n(14-17)
packages/ui-default/components/problemconfig/reducer/config.ts (1)
framework/utils/lib/common.ts (1)
sortFiles(216-238)
packages/ui-default/components/problemconfig/tree/SubtaskSettings.tsx (3)
packages/ui-default/utils/base.ts (1)
i18n(14-17)framework/utils/lib/common.ts (2)
parseTimeMS(164-170)parseMemoryMB(172-178)packages/ui-default/components/autocomplete/CustomSelectAutoComplete.tsx (1)
CustomSelectAutoComplete(8-23)
packages/ui-default/components/problemconfig/index.tsx (3)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/problemconfig/ProblemConfigForm.tsx (1)
ProblemConfigForm(89-103)packages/ui-default/components/problemconfig/ProblemConfigTree.tsx (1)
ProblemConfigTree(147-163)
packages/ui-default/pages/problem_config.page.tsx (1)
packages/ui-default/components/problemconfig/index.tsx (1)
ProblemConfig(15-57)
packages/ui-default/components/notification/index.ts (1)
packages/ui-default/utils/base.ts (1)
tpl(39-58)
packages/ui-default/components/problemconfig/ProblemConfigForm.tsx (1)
packages/ui-default/components/problemconfig/BasicForm.tsx (1)
FormItem(8-20)
packages/ui-default/components/problemconfig/ProblemConfigTree.tsx (5)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/dialog/index.tsx (1)
confirm(253-258)packages/ui-default/components/problemconfig/tree/SubtaskSettings.tsx (2)
SubtaskSettings(17-126)GlobalSettings(128-180)packages/ui-default/components/problemconfig/tree/SelectionManager.tsx (1)
SelectionManager(21-130)packages/ui-default/components/problemconfig/tree/AddTestcase.tsx (1)
AddTestcase(9-137)
packages/ui-default/components/problemconfig/tree/AddTestcase.tsx (2)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/autocomplete/FileSelectAutoComplete.tsx (1)
FileSelectAutoComplete(11-26)
packages/ui-default/components/discussion/history.page.tsx (2)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/misc/Page.ts (1)
AutoloadPage(39-46)
packages/ui-default/components/problemconfig/ProblemType.tsx (3)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/problemconfig/BasicForm.tsx (3)
FormItem(8-20)SingleFileSelect(59-92)ManagedSelect(43-57)packages/ui-default/components/monaco/schema/problemconfig.ts (1)
testlibCheckers(3-7)
packages/ui-default/components/problemconfig/tree/Testcase.tsx (1)
packages/ui-default/utils/base.ts (1)
i18n(14-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (27)
packages/ui-default/templates/contest_detail.html (1)
15-23: Verify CSS classes exist and styling migrates cleanly.The migration from Blueprint.js (
bp6-tag,bp6-large,bp6-minimal) to generic classes (problem__tag-item,interactive) removes all bp6-specific styling. Ensure the new CSS rules provide equivalent visual appearance and spacing.Key verifications needed:
- Confirm
.problem__tag-itemis defined with baseline styling (padding, margin, display, font-size)- Verify
.interactiveclass maintains link-like appearance (cursor, color, hover states)- Ensure icon placement and sizing work correctly with combined
icon icon-*classesCan you confirm that the CSS for
.problem__tag-itemand.interactiveclasses is properly defined and that the visual appearance matches the Blueprint.js styling?packages/ui-default/templates/problem_detail.html (2)
93-107: Verify.problem__tag-itemstyling provides consistent appearance across diverse contexts.The migration replaces multiple bp6 classes with
.problem__tag-itemacross different semantic contexts: IDs, types, durations, memory, links, and difficulty. Ensure the unified class provides appropriate styling without losing context-specific visual differentiation.Specific concerns:
- Line 93: ID tag vs other tags—ensure sufficient visual distinction if needed
- Lines 104–105:
<a>elements with.interactiveclass—verify link styling, underlines, and hover states- Line 107: "Uploaded By" with
.interactive—confirm interactive styling is consistent with link tags abovePlease verify that
.problem__tag-itemand.interactiveclasses provide adequate styling for both static (<span>) and interactive (<a>) elements without visual regressions or loss of interactivity indicators.
110-112: Confirm tag display logic works with simplified classes.Lines 110 and 112 handle conditional tag display (show/hide logic). The
nojs--hideandhasjs--hideclasses are preserved, but verify the combined.problem__tag-item.interactive.nojs--hide.icon.icon-tagselectors are correctly styled, particularly for the toggle button on line 110.Verify that CSS selectors for combined classes (
.problem__tag-item.interactive.nojs--hide.icon.icon-tag) are defined and that the tag toggle button renders correctly with proper styling and interactivity.packages/ui-default/components/problem/tag.page.styl (2)
18-36: Verify intentional property overrides.Several properties are re-declared with different values compared to earlier lines:
padding: Line 18 (0 10px !important) vs. line 34 (6px 8px)margin-bottom: Line 20 (rem(5px)) vs. line 46 (implicitly overridden by child spacing rules)line-height: Line 21 (30px !important) vs. line 30 (18px)Confirm these overrides are intentional for the Mantine migration, or consolidate the rules to avoid confusion.
49-57: Good accessibility and UX improvements.The focus outline styling (lines 49-52) and interactive variant hover state (lines 53-57) enhance accessibility and user experience. The outline color and offset look appropriate.
packages/ui-default/package.json (4)
23-25: Mantine package versions are consistent and well-chosen.All Mantine packages use the same version (^8.3.6), ensuring compatibility across the UI library ecosystem.
27-27: Verify the purpose of the @sentry/webpack-plugin version update.Line 27 shows @sentry/webpack-plugin updated to ^4.6.0 with a
~marker. This change appears separate from the BP6→Mantine migration. Please confirm whether this update is intentional and related to the migration, or if it should be a separate commit.
70-70: Clarify mantine-contextmenu placement.According to the AI summary,
mantine-contextmenuwas added to bothdevDependenciesanddependencies. However, the provided code shows it only indevDependencies(line 70). Since this is a UI context menu component, verify whether it should be a runtime dependency (dependencies) rather than or in addition to a dev dependency, and update the placement if needed.
13-150: Blueprint.js packages successfully removed.Verification confirms complete removal of all
@blueprintjs/*packages from bothdevDependenciesanddependencies, with no remaining Blueprint imports or references in the codebase.packages/ui-default/components/notification/notification.page.styl (1)
68-72: LGTM! Clean Mantine styling addition.The closing brace properly closes the
@media printblock, and the new hover styling for Mantine's notification close button provides appropriate visual feedback with a subtle background color change.packages/ui-default/components/notification/index.ts (3)
1-5: LGTM! Proper imports for Mantine migration.The imports correctly include all necessary Mantine dependencies (core, notifications) and React for creating elements. This aligns well with the Blueprint.js to Mantine migration.
7-35: LGTM! Well-structured Mantine theme configuration.The custom theme properly extends Mantine's Notification component with project-specific styling, using CSS variables for flexibility and maintaining consistent visual design across the notification system.
37-41: LGTM! Correct React rendering approach.The MantineProvider and Notifications are properly rendered using
tplwith thetrueflag, which ensures they're rendered withReactDOM.createRootrather than static markup. This is necessary for Mantine's context and state management to work correctly.packages/ui-default/components/discussion/reaction.page.tsx (2)
4-4: @mantine/core is properly declared—no action needed.Verification confirms that
@mantine/core@^8.3.6,@mantine/hooks@^8.3.6, and@mantine/notifications@^8.3.6are all properly declared inpackages/ui-default/package.json. The import statement is valid and the packages are available as dependencies.
69-73: Clarify the purpose of the emojiSuggest input.The
emojiSuggestinput (line 71) has noonChangehandler,valueattribute, or any emoji suggestion/filtering logic. It only manages focus state to keep the Popover open while focused. What is the intended functionality—is this a placeholder for future emoji search/filtering, or is the focus-state-only behavior intentional?packages/ui-default/theme/default.js (1)
7-9: LGTM! Clean migration to Mantine stylesheets.The CSS imports correctly reflect the migration from Blueprint.js to Mantine, with the addition of bp6-compat.css for backward compatibility with any remaining bp6-styled components.
Also applies to: 31-31
packages/ui-default/theme/bp6-compat.css (5)
1-9: LGTM! Well-structured progress bar container.The base styles for the progress bar container are clean and follow standard patterns.
11-20: LGTM! Smooth progress meter implementation.The meter styles include a nice striped gradient effect and smooth width transitions for progress updates.
26-28: LGTM! Clean no-stripes variant.The override correctly removes the striped background for solid progress bars.
30-36: LGTM! Appropriate dark theme adjustments.The dark theme colors provide good contrast and follow standard dark mode conventions.
38-52: Intent color styles are correctly defined but unused in the codebase.The four intent color variants (primary, success, warning, danger) are properly defined with appropriate semantic colors. However, verification shows these classes exist only in the compatibility CSS file with no active usage in any component code, indicating these are orphaned styles in a deprecated compatibility layer. No action is required for this code section.
packages/ui-default/components/problemconfig/tree/Testcase.tsx (1)
64-91: LGTM! Context menu integration is well implemented.The context menu for moving test cases is correctly implemented with proper target filtering and Redux action dispatching. The fallback for no available targets is a good UX touch.
packages/ui-default/pages/problem_config.page.tsx (2)
118-137: LGTM! Auto-config guard logic is correct.The
autoConfigTriggeredflag correctly prevents repeated auto-configuration runs. The logic ensures that auto-configuration happens once and then the guard prevents further executions.
146-150: LGTM! MantineProvider integration is correct.The MantineProvider wrapper is correctly applied to provide Mantine theme and context to the component tree. This follows the standard Mantine integration pattern.
packages/ui-default/components/problemconfig/index.tsx (1)
1-57: LGTM! Blueprint to Mantine migration is well executed.The migration successfully replaces Blueprint.js components with Mantine equivalents while maintaining the same functionality. The Tabs and Modal implementations follow Mantine best practices, and the ContextMenuProvider integration enables proper context menu support.
packages/ui-default/components/problemconfig/ProblemConfigTree.tsx (2)
56-97: LGTM! SubtaskNode migration to Mantine is well implemented.The refactoring successfully migrates from Blueprint to Mantine while maintaining functionality. The delete confirmation pattern with
confirm().then()is correct, and the inline styles are appropriate for the Mantine migration.
117-143: LGTM! SubtaskConfigTree successfully migrated to Mantine Tree.The refactored tree implementation uses Mantine's Tree component effectively with a custom
renderNodefunction. The memoizedrootNodesstructure and type-based rendering logic are clean and maintainable.
| <span | ||
| className="icon icon-emoji" | ||
| onMouseEnter={() => updateTrigger(true)} | ||
| onMouseLeave={() => { | ||
| setTimeout(() => updateTrigger(false), 300); | ||
| }} | ||
| /> | ||
| </Popover.Target> |
There was a problem hiding this comment.
Add cleanup for setTimeout and improve accessibility.
Two issues with the trigger element:
-
Memory leak risk: The
setTimeouton line 48 is not cleaned up if the component unmounts before the timeout fires. This can cause attempted state updates on unmounted components. -
Accessibility blocker: The
<span>element with mouse handlers is not keyboard-accessible. Users who rely on keyboard navigation cannot interact with the emoji reactions.
Apply this diff to address both issues:
- const [open, updateOpen] = React.useState(false);
- const [trigger, updateTrigger] = React.useState(false);
+ const [open, updateOpen] = React.useState(false);
+ const [trigger, updateTrigger] = React.useState(false);
+ const timeoutRef = React.useRef<number>();
+
+ React.useEffect(() => {
+ return () => {
+ if (timeoutRef.current) clearTimeout(timeoutRef.current);
+ };
+ }, []);
+
return (
<Popover opened={trigger || open || focus}>
<Popover.Target>
<span
className="icon icon-emoji"
+ role="button"
+ tabIndex={0}
+ aria-label="Add reaction"
onMouseEnter={() => updateTrigger(true)}
onMouseLeave={() => {
- setTimeout(() => updateTrigger(false), 300);
+ timeoutRef.current = window.setTimeout(() => updateTrigger(false), 300);
}}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ updateTrigger((prev) => !prev);
+ }
+ }}
/>
</Popover.Target>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/ui-default/components/discussion/reaction.page.tsx around lines 44
to 51, the span using setTimeout for mouseLeave must clear the timeout on
unmount and before scheduling another one to avoid state updates on unmounted
components; use a ref to store the timeout id and call clearTimeout(ref.current)
before setting a new timeout and in a useEffect cleanup. Also make the span
keyboard-accessible by adding tabIndex={0}, onFocus/onBlur handlers that mirror
onMouseEnter/onMouseLeave, and an onKeyDown handler to open the popover when
Enter or Space is pressed.
| @@ -1,4 +1,4 @@ | |||
| import { Classes } from '@blueprintjs/core'; | |||
| import { MantineProvider, Progress } from '@mantine/core'; | |||
There was a problem hiding this comment.
Critical: Module-level state causes race conditions with concurrent uploads.
The state variables (fileLabel, uploadLabel, fileProgress, uploadProgress, render) are declared at module scope, making them shared across all concurrent invocations of uploadFiles. If multiple upload dialogs are opened simultaneously, their progress updates will interfere with each other, causing incorrect UI state and progress display.
Move all state management into the ProgressDialog component and pass update callbacks as props or use a context/ref pattern:
-import { MantineProvider, Progress } from '@mantine/core';
+import { MantineProvider, Progress } from '@mantine/core';
import $ from 'jquery';
import React from 'react';
import { Dialog } from 'vj/components/dialog/index';
import Notification from 'vj/components/notification';
import {
delay, i18n, pjax, request, tpl,
} from 'vj/utils';
function onBeforeUnload(e) {
e.returnValue = '';
}
interface UploadOptions {
type?: string;
pjax?: boolean;
sidebar?: boolean;
singleFileUploadCallback?: (file: File) => any;
filenameCallback?: (file: File) => string;
}
+
+interface ProgressState {
+ fileLabel: string;
+ uploadLabel: string;
+ fileProgress: number;
+ uploadProgress: number;
+}
+
export default async function uploadFiles(endpoint = '', files: File[] | FileList = [], options: UploadOptions = {}) {
- let fileLabel = '';
- let uploadLabel = '';
- let fileProgress = 0;
- let uploadProgress = 0;
- let render = () => { };
+ const stateRef = React.createRef<{
+ updateProgress: (state: Partial<ProgressState>) => void;
+ }>();
- function ProgressDialog() {
- const [, setRender] = React.useState(0);
+ function ProgressDialog() {
+ const [state, setState] = React.useState<ProgressState>({
+ fileLabel: '',
+ uploadLabel: '',
+ fileProgress: 0,
+ uploadProgress: 0,
+ });
+
React.useEffect(() => {
- render = () => setRender((r) => r + 1);
+ if (stateRef.current) {
+ stateRef.current.updateProgress = (newState) => {
+ setState((prev) => ({ ...prev, ...newState }));
+ };
+ }
}, []);
+
return <MantineProvider>
<div
style={{
textAlign: 'center', marginBottom: '5px', color: 'gray', fontSize: 'small',
}}
- >{uploadLabel}</div>
- <Progress value={uploadProgress} />
+ >{state.uploadLabel}</div>
+ <Progress value={state.uploadProgress} />
<div
style={{
textAlign: 'center', margin: '5px 0', color: 'gray', fontSize: 'small',
}}
- >{fileLabel}</div>
- <Progress value={fileProgress} />
+ >{state.fileLabel}</div>
+ <Progress value={state.fileProgress} />
</MantineProvider>;
}Then update the xhr event handlers to use stateRef.current.updateProgress:
xhr.upload.addEventListener('loadstart', () => {
- uploadLabel = `[${+i + 1}/${files.length}] ${file.name} `;
- uploadProgress = Math.round((+i + 1) / files.length * 100);
- fileLabel = i18n('Uploading... ({0}%)', 0);
- fileProgress = 0;
- render();
+ stateRef.current?.updateProgress({
+ uploadLabel: `[${+i + 1}/${files.length}] ${file.name} `,
+ uploadProgress: Math.round((+i + 1) / files.length * 100),
+ fileLabel: i18n('Uploading... ({0}%)', 0),
+ fileProgress: 0,
+ });
});
xhr.upload.addEventListener('progress', (e) => {
if (e.lengthComputable) {
const percentComplete = Math.round((e.loaded / e.total) * 100);
- if (percentComplete === 100) fileLabel = i18n('Processing...');
- else fileLabel = i18n('Uploading... ({0}%)', percentComplete);
- fileProgress = percentComplete;
- render();
+ stateRef.current?.updateProgress({
+ fileLabel: percentComplete === 100 ? i18n('Processing...') : i18n('Uploading... ({0}%)', percentComplete),
+ fileProgress: percentComplete,
+ });
}
}, false);Also applies to: 22-26
🤖 Prompt for AI Agents
packages/ui-default/components/upload.tsx around lines 1 and 22-26: module-level
state variables (fileLabel, uploadLabel, fileProgress, uploadProgress, render)
are declared at module scope causing race conditions when multiple upload
dialogs run concurrently; move all state into the ProgressDialog component (use
useState/useRef inside the component), remove the module-level declarations at
lines 22-26, and expose update callbacks or a stateRef (e.g.,
stateRef.current.updateProgress) passed into the uploadFiles/XHR handlers so the
handlers update the correct dialog instance instead of shared module state.
| function ProgressDialog() { | ||
| const [, setRender] = React.useState(0); | ||
| React.useEffect(() => { | ||
| render = () => setRender((r) => r + 1); | ||
| }, []); | ||
| return <MantineProvider> | ||
| <div | ||
| className="file-label" | ||
| style={{ | ||
| textAlign: 'center', marginBottom: '5px', color: 'gray', fontSize: 'small', | ||
| }} | ||
| ></div> | ||
| <div className={`${Classes.PROGRESS_BAR} ${Classes.INTENT_PRIMARY} bp5-no-stripes`}> | ||
| <div className={`file-progress ${Classes.PROGRESS_METER}`} style={{ width: 0 }}></div> | ||
| </div> | ||
| >{uploadLabel}</div> | ||
| <Progress value={uploadProgress} /> | ||
| <div | ||
| className="upload-label" | ||
| style={{ | ||
| textAlign: 'center', margin: '5px 0', color: 'gray', fontSize: 'small', | ||
| }} | ||
| ></div> | ||
| <div className={`${Classes.PROGRESS_BAR} ${Classes.INTENT_PRIMARY} bp5-no-stripes`}> | ||
| <div className={`upload-progress ${Classes.PROGRESS_METER}`} style={{ width: 0 }}></div> | ||
| </div> | ||
| </>)), | ||
| >{fileLabel}</div> | ||
| <Progress value={fileProgress} /> | ||
| </MantineProvider>; | ||
| } |
There was a problem hiding this comment.
Race condition: render() may be called before component mounts.
The render function is assigned inside useEffect, but it's invoked in the loadstart and progress event handlers (lines 72, 80) which can fire immediately after request.postFile is called on line 64. If these events fire before the component mounts and useEffect runs, render() will be a no-op, and the initial progress updates will be lost.
Consider one of these approaches:
- Use the ref-based pattern suggested in the previous comment to ensure the update function is available when needed
- Add a check to ensure the component is mounted before calling render:
function ProgressDialog() {
const [, setRender] = React.useState(0);
+ const mountedRef = React.useRef(false);
React.useEffect(() => {
+ mountedRef.current = true;
- render = () => setRender((r) => r + 1);
+ render = () => {
+ if (mountedRef.current) {
+ setRender((r) => r + 1);
+ }
+ };
+ return () => { mountedRef.current = false; };
}, []);Note: The ref-based solution in the previous comment is preferred as it eliminates the race condition entirely.
📝 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.
| function ProgressDialog() { | |
| const [, setRender] = React.useState(0); | |
| React.useEffect(() => { | |
| render = () => setRender((r) => r + 1); | |
| }, []); | |
| return <MantineProvider> | |
| <div | |
| className="file-label" | |
| style={{ | |
| textAlign: 'center', marginBottom: '5px', color: 'gray', fontSize: 'small', | |
| }} | |
| ></div> | |
| <div className={`${Classes.PROGRESS_BAR} ${Classes.INTENT_PRIMARY} bp5-no-stripes`}> | |
| <div className={`file-progress ${Classes.PROGRESS_METER}`} style={{ width: 0 }}></div> | |
| </div> | |
| >{uploadLabel}</div> | |
| <Progress value={uploadProgress} /> | |
| <div | |
| className="upload-label" | |
| style={{ | |
| textAlign: 'center', margin: '5px 0', color: 'gray', fontSize: 'small', | |
| }} | |
| ></div> | |
| <div className={`${Classes.PROGRESS_BAR} ${Classes.INTENT_PRIMARY} bp5-no-stripes`}> | |
| <div className={`upload-progress ${Classes.PROGRESS_METER}`} style={{ width: 0 }}></div> | |
| </div> | |
| </>)), | |
| >{fileLabel}</div> | |
| <Progress value={fileProgress} /> | |
| </MantineProvider>; | |
| } | |
| function ProgressDialog() { | |
| const [, setRender] = React.useState(0); | |
| const mountedRef = React.useRef(false); | |
| React.useEffect(() => { | |
| mountedRef.current = true; | |
| render = () => { | |
| if (mountedRef.current) { | |
| setRender((r) => r + 1); | |
| } | |
| }; | |
| return () => { mountedRef.current = false; }; | |
| }, []); | |
| return <MantineProvider> | |
| <div | |
| style={{ | |
| textAlign: 'center', marginBottom: '5px', color: 'gray', fontSize: 'small', | |
| }} | |
| >{uploadLabel}</div> | |
| <Progress value={uploadProgress} /> | |
| <div | |
| style={{ | |
| textAlign: 'center', margin: '5px 0', color: 'gray', fontSize: 'small', | |
| }} | |
| >{fileLabel}</div> | |
| <Progress value={fileProgress} /> | |
| </MantineProvider>; | |
| } |
🤖 Prompt for AI Agents
In packages/ui-default/components/upload.tsx around lines 28-47, replace the
current render assignment via useEffect with a ref-based updater to avoid the
race: create a useRef for the render function, set renderRef.current = () =>
setRender(r => r + 1) synchronously (not only inside useEffect), and update the
event handlers elsewhere to call renderRef.current?.() instead of render(); this
ensures the updater exists immediately and eliminates the race where
loadstart/progress fire before the effect runs.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ui-default/components/problem/tag.page.styl (1)
18-33: CSS specificity conflict:!importantrules prevent new styles from applying.Lines 18 and 21 declare
paddingandline-heightwith!important, while lines 29 and 33 add conflicting declarations without!important. In CSS,!importantrules always win—this means the new styles on lines 29 and 33 will never apply.Given this is a migration away from Blueprint.js, the intent is likely to use the new values (line-height: 18px, padding: 6px 8px). To fix this, either remove
!importantfrom the earlier declarations or add!importantto the new ones.Apply this diff (removing
!importantfrom lines 18 and 21):.problem__tag-item - padding: 0 10px !important + padding: 0 10px margin-left: rem(3px) margin-bottom: rem(5px) - line-height: 30px !important + line-height: 30px display: inline-flexAlternatively, if you prefer to keep the
!importantflags, apply!importantto lines 29 and 33:- font-size: 14px - line-height: 18px + font-size: 14px !important + line-height: 18px !important - padding: 6px 8px + padding: 6px 8px !importantpackages/ui-default/components/problemconfig/tree/AddTestcase.tsx (1)
84-101: Add keyboard accessibility to clickable elements.These divs act as buttons but lack semantic HTML and keyboard support, preventing keyboard-only users from accessing "Auto detect" and "Add testcase" features.
Consider using Mantine's
Buttoncomponent withvariant="subtle"or add proper ARIA attributes:- <div - style={{ cursor: 'pointer', padding: '6px 0' }} - onClick={auto} - > - <Text><i className="icon icon-wrench" /> {i18n('Auto detect')}</Text> - </div> - <div - style={{ cursor: 'pointer', padding: '6px 0' }} + <Button + variant="subtle" + onClick={auto} + style={{ padding: '6px 0' }} + leftSection={<i className="icon icon-wrench" />} + > + {i18n('Auto detect')} + </Button> + <Button + variant="subtle" + style={{ padding: '6px 0' }} onClick={() => { setInput(''); setOutput(''); setAutoInput(true); setAutoOutput(true); setOpen(true); }} + leftSection={<i className="icon icon-add" />} > - <Text><i className="icon icon-add" /> {i18n('Add testcase')}</Text> - </div> + {i18n('Add testcase')} + </Button>
♻️ Duplicate comments (1)
packages/ui-default/components/discussion/reaction.page.tsx (1)
44-51: Add setTimeout cleanup and keyboard accessibility.The issues flagged in the previous review remain unaddressed:
Memory leak risk: The
setTimeouton line 48 is not cleaned up if the component unmounts before the timeout fires, potentially causing state updates on unmounted components.Accessibility blocker: The
<span>element with mouse handlers is not keyboard-accessible, preventing users who rely on keyboard navigation from interacting with emoji reactions.Apply this diff to address both issues:
const [focus, updateFocus] = React.useState(false); const [open, updateOpen] = React.useState(false); const [trigger, updateTrigger] = React.useState(false); + const timeoutRef = React.useRef<number>(); + + React.useEffect(() => { + return () => { + if (timeoutRef.current) clearTimeout(timeoutRef.current); + }; + }, []); + return ( <Popover opened={trigger || open || focus}> <Popover.Target> <span className="icon icon-emoji" + role="button" + tabIndex={0} + aria-label="Add reaction" onMouseEnter={() => updateTrigger(true)} onMouseLeave={() => { - setTimeout(() => updateTrigger(false), 300); + if (timeoutRef.current) clearTimeout(timeoutRef.current); + timeoutRef.current = window.setTimeout(() => updateTrigger(false), 300); }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + updateTrigger((prev) => !prev); + } + }} /> </Popover.Target>
🧹 Nitpick comments (3)
packages/ui-default/theme/bp6-compat.css (1)
11-24: Add animation for striped progress meter (or confirm if static stripes are intentional).The CSS includes a striped pattern via
linear-gradient(line 12) withbackground-size: 30px 30px(line 14), but lacks the@keyframesanimation to move the stripes and create the characteristic "crawling" effect typical of Blueprint progress bars.A previous review flagged this missing animation. If animated stripes are desired (recommended for better UX feedback), apply this diff:
+@keyframes linear-progress-bar-stripes { + from { + background-position: 0 0; + } + to { + background-position: 30px 0; + } +} + .bp6-progress-bar .bp6-progress-meter { background: linear-gradient(-45deg, rgba(255, 255, 255, 0.2) 25%, transparent 25%, transparent 50%, rgba(255, 255, 255, 0.2) 50%, rgba(255, 255, 255, 0.2) 75%, transparent 75%); background-color: rgba(95, 107, 124, 0.8); background-size: 30px 30px; border-radius: 40px; height: 100%; position: absolute; transition: width 200ms cubic-bezier(0.4, 1, 0.75, 0.9); width: 100%; + animation: linear-progress-bar-stripes 300ms linear infinite; } .bp6-progress-bar.bp6-no-stripes .bp6-progress-meter {Confirm whether animated stripes should be included to match Blueprint.js behavior or if the current static stripes are intentional for this migration.
packages/ui-default/components/problemconfig/tree/AddTestcase.tsx (1)
85-132: Consider extracting repeated inline styles.Several inline styles are duplicated across the component. While not critical, extracting these to CSS classes or Mantine's
sxprop would improve maintainability.For example, the repeated
cursor: 'pointer', padding: '6px 0'pattern could be extracted to a shared style.packages/ui-default/components/notification/index.ts (1)
95-133: Consider using CSS variables or theme colors for consistency.The static methods use hardcoded color values (
'#238551','#2d72d2','#fbb360','#cd4246') while the theme configuration (lines 11-35) uses CSS variables extensively. This creates inconsistency and prevents the theme from controlling these notification colors.Consider defining these colors in the theme or using CSS variables for better maintainability and theming consistency.
Verify the empty message field pattern is intentional.
All static methods pass an empty string for the
messageparameter and place the actual notification text in thetitlefield. In Mantine's design system,titleis typically a heading andmessageis the body text. If this is intentional for single-line notifications, it's fine, but please confirm this is the desired UX pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/ui-default/components/discussion/reaction.page.tsx(3 hunks)packages/ui-default/components/notification/index.ts(2 hunks)packages/ui-default/components/notification/notification.page.styl(1 hunks)packages/ui-default/components/problem/tag.page.styl(1 hunks)packages/ui-default/components/problemconfig/tree/AddTestcase.tsx(3 hunks)packages/ui-default/package.json(3 hunks)packages/ui-default/theme/bp6-compat.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui-default/package.json
- packages/ui-default/components/notification/notification.page.styl
🧰 Additional context used
🧬 Code graph analysis (3)
packages/ui-default/components/problemconfig/tree/AddTestcase.tsx (2)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/autocomplete/FileSelectAutoComplete.tsx (1)
FileSelectAutoComplete(11-26)
packages/ui-default/components/notification/index.ts (1)
packages/ui-default/utils/base.ts (1)
tpl(39-58)
packages/ui-default/components/discussion/reaction.page.tsx (1)
packages/ui-default/misc/Page.ts (1)
AutoloadPage(39-46)
🔇 Additional comments (7)
packages/ui-default/components/problemconfig/tree/AddTestcase.tsx (1)
102-135: LGTM! Modal migration looks good.The Mantine Modal correctly replaces the Blueprint.js Dialog with proper localization on the title (line 105). The overflow styles are necessary for the autocomplete dropdowns to display properly outside the modal bounds.
packages/ui-default/components/notification/index.ts (2)
1-5: LGTM!The imports are correctly updated for the Mantine migration, including the necessary React import for
createElementcalls in the static methods.
7-41: LGTM!The Mantine theme configuration is well-structured with proper use of CSS variables for styling consistency. The notification provider is correctly mounted to the document body using the
tplutility.packages/ui-default/components/discussion/reaction.page.tsx (4)
4-4: LGTM!The import migration from Blueprint.js to Mantine is correct and aligns with the PR objectives.
39-40: LGTM!The state management for controlling Popover visibility is well-structured and clear.
52-74: LGTM!The Popover.Dropdown structure correctly implements Mantine's pattern with proper mouse handlers and state management. The emoji selection logic cleanly closes the popover after the async operation completes.
82-84: LGTM!The rendering correctly wraps each Reaction component with MantineProvider, and the JSX syntax is valid (no trailing comma).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/ui-default/components/problemconfig/ProblemType.tsx (6)
19-24: Consider clarifying the category initialization logic.The useEffect early-returns when
categoryis truthy, butcategoryisn't in the dependency array. While this appears intentional (to initialize category only once based on the initial checker value), the implicit one-shot behavior could be confusing for maintainers.Consider adding a comment explaining the intended behavior or refactoring to make the one-time initialization more explicit.
43-71: Extract repeated condition into a helper for maintainability.The condition
(['strict', 'default'].includes(checkerType) || !checkerType)appears on both lines 48 and 62. Consider extracting this into a named helper function likeisDefaultCheckerType(checkerType)to improve maintainability and reduce duplication.Apply this pattern:
+const isDefaultCheckerType = (type) => ['strict', 'default'].includes(type) || !type; + export default function ProblemType() { const Type = useSelector((state: RootState) => state.config.type); const checkerType = useSelector((state: RootState) => state.config.checker_type); ... <SegmentedControl - value={(['strict', 'default'].includes(checkerType) || !checkerType) - ? 'default' : (checkerType !== 'testlib' ? 'other' : 'testlib')} + value={isDefaultCheckerType(checkerType) ? 'default' : (checkerType !== 'testlib' ? 'other' : 'testlib')} onChange={(value) => { ... }} /> </div> - {((['strict', 'default'].includes(checkerType) || !checkerType)) && ( + {isDefaultCheckerType(checkerType) && ( <Switch
73-102: Simplify the condition for testlib checker.Line 73's condition
(!['strict', 'default'].includes(checkerType) && checkerType && checkerType === 'testlib')is overly complex. Since checkingcheckerType === 'testlib'already implies it's truthy and not in['strict', 'default'], the condition can be simplified.Apply this diff:
-{(!['strict', 'default'].includes(checkerType) && checkerType && checkerType === 'testlib') && ( +{checkerType === 'testlib' && ( <div className="row">
104-111: Consider a helper function for checker type checks.Similar to the testlib section, this condition could benefit from a helper function like
isCustomCheckerType(checkerType)for improved readability. This would make the intent clearer: checking for custom checker interfaces like 'syzoj', 'hustoj', etc.Example:
const isCustomCheckerType = (type) => type && !['strict', 'default', 'testlib'].includes(type);Then use:
-{(!['strict', 'default'].includes(checkerType) && checkerType && checkerType !== 'testlib') && ( +{isCustomCheckerType(checkerType) && ( <div className="row">
124-131: Consider using controlled input for state consistency.Line 126 uses
defaultValueinstead ofvalue, making this an uncontrolled input. IfnumProcesseschanges in the Redux store (e.g., through external updates or reset actions), the input field won't reflect the updated value. Consider switching to a controlled input pattern.Apply this diff:
<input - defaultValue={numProcesses || 2} + value={numProcesses || 2} placeholder="2" onChange={(ev) => dispatch(({ type: 'CONFIG_FORM_UPDATE', key: 'num_processes', value: +ev.currentTarget.value }))} className="textbox" />Note: The extra parentheses in line 128's
dispatch(({...}))are redundant but harmless—you may optionally simplify todispatch({...}).
147-155: Consider using controlled input for filename field.Similar to the num_processes input, line 149 uses
defaultValueinstead ofvalue, creating an uncontrolled input. This can lead to UI/state inconsistencies if the Redux state is updated externally.Apply this diff:
<input - defaultValue={filename || '#.txt'} + value={filename || '#.txt'} placeholder="#.txt" disabled={subType !== 'multi'} onChange={(ev) => dispatch(({ type: 'CONFIG_FORM_UPDATE', key: 'filename', value: ev.currentTarget.value }))} className="textbox" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/common/subtask.ts(2 hunks)packages/ui-default/components/problemconfig/ProblemConfigTree.tsx(4 hunks)packages/ui-default/components/problemconfig/ProblemType.tsx(2 hunks)packages/ui-default/components/problemconfig/index.tsx(2 hunks)packages/ui-default/package.json(5 hunks)packages/ui-default/pages/problem_config.page.styl(1 hunks)packages/ui-default/pages/problem_main.page.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ui-default/pages/problem_main.page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui-default/components/problemconfig/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
packages/ui-default/components/problemconfig/ProblemType.tsx (3)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/problemconfig/BasicForm.tsx (3)
FormItem(8-20)SingleFileSelect(59-92)ManagedSelect(43-57)packages/ui-default/components/monaco/schema/problemconfig.ts (1)
testlibCheckers(3-7)
packages/ui-default/components/problemconfig/ProblemConfigTree.tsx (5)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/dialog/index.tsx (1)
confirm(253-258)packages/ui-default/components/problemconfig/tree/SubtaskSettings.tsx (2)
SubtaskSettings(17-126)GlobalSettings(128-180)packages/ui-default/components/problemconfig/tree/SelectionManager.tsx (1)
SelectionManager(21-130)packages/ui-default/components/problemconfig/tree/AddTestcase.tsx (1)
AddTestcase(9-137)
packages/common/subtask.ts (1)
framework/utils/lib/common.ts (1)
sortFiles(216-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (13)
packages/common/subtask.ts (3)
1-1: LGTM! Import addition supports natural sorting.The
sortFilesimport is correctly added to support the natural/numeric sorting of test cases on line 130.
121-123: Good addition: Prevents duplicate test cases.The duplicate check ensures that cases with identical input and output filenames aren't added multiple times to a subtask. This improves data integrity and aligns with the testcase deduplication mentioned in the PR summary.
130-130: Good addition: Ensures consistent test case ordering.Sorting cases by input filename using
sortFilesprovides predictable, natural ordering (e.g., "1.in", "2.in", "10.in" instead of lexicographic "1.in", "10.in", "2.in"). This improves consistency across the system.packages/ui-default/components/problemconfig/ProblemConfigTree.tsx (5)
3-3: LGTM! Clean migration to Mantine components.The import changes correctly replace Blueprint.js components with Mantine equivalents and add the confirmation dialog utility.
Also applies to: 9-9
48-76: LGTM! Safe deletion flow with user confirmation.The delete confirmation logic properly prevents accidental deletions by requiring explicit user confirmation before dispatching the
deleteSubtaskaction. ThestopPropagationcorrectly prevents the parent click handler from firing.
56-97: LGTM! Successful migration to Mantine layout.The SubtaskNode component has been cleanly migrated from Blueprint TreeNode to a Mantine-based layout using Text and Button components. The expand/collapse logic and drag-and-drop functionality remain intact.
104-144: LGTM! Clean Mantine Tree implementation.The
SubtaskConfigTreehas been successfully migrated to use Mantine's Tree component with proper memoization and custom rendering logic. TheautoConfigurecallback dependencies are correct, and therootNodesstructure properly defines the tree hierarchy.
155-158: LGTM! Simplified layout structure.The right pane has been appropriately simplified by replacing the Blueprint Tree structure with a plain div container, aligning with the Mantine migration.
packages/ui-default/components/problemconfig/ProblemType.tsx (3)
27-41: LGTM!The Problem Type selector UI is well-structured with proper i18n labels and clean state binding.
115-119: LGTM!Interactive type configuration is straightforward and properly implemented.
159-161: LGTM!Appropriate messaging for unsupported objective type configuration.
packages/ui-default/package.json (2)
23-26: Well-coordinated dependency migration to Mantine + React 19 ecosystem.The additions and upgrades align with the broader Blueprint.js → Mantine migration documented across the codebase. All Mantine packages (core, hooks, notifications) are pinned to the same version (8.3.6) for consistency, and React is upgraded to 19.2.0 with matching @types (19.2.2). PostCSS and build tooling (postcss-preset-mantine, mantine-contextmenu) are properly integrated.
Also applies to: 39-40, 70-70, 86-86, 92-92, 96-96, 123-123
27-27: Remove React 19 compatibility concern; webpack plugin is runtime-agnostic.The @sentry/webpack-plugin v4.6.0 upgrade is safe to merge. The @sentry/webpack-plugin itself is agnostic to the React runtime; compatibility with React 19 is determined by the Sentry React SDK (@sentry/react or @sentry/nextjs), not the webpack plugin. The minor version bump handles only release and source map management, which are unaffected by React versions. No React 19-specific verification is needed for this dependency.
Likely an incorrect or invalid review comment.
| data={rootNodes} | ||
| levelOffset={12} | ||
| renderNode={({ node }) => { | ||
| const n: any = node; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid using any type; define a proper union type.
The any type assertion defeats TypeScript's type safety. Since the node structure is known from rootNodes (lines 117-122), define a proper union type for better type safety.
Add a type definition before the component:
type TreeNodeType =
| { id: 'auto'; label: string; type: 'action' }
| { id: 'global'; label: string; type: 'global' }
| { id: string; label: string; type: 'subtask'; subtaskId: number }
| { id: 'add'; label: string; type: 'add' };Then use it:
- const n: any = node;
+ const n = node as TreeNodeType;🤖 Prompt for AI Agents
In packages/ui-default/components/problemconfig/ProblemConfigTree.tsx around
line 129, the code uses "const n: any = node;" which bypasses TypeScript safety;
replace this with a proper union type definition (e.g., a TreeNodeType union
that covers 'auto', 'global', 'subtask' with subtaskId, and 'add') declared
before the component, then change the assertion to use that type (const n:
TreeNodeType = node) and update any downstream property accesses to match the
union (n.type narrowings or type guards where needed) so the compiler enforces
correct fields instead of using any.
| .main | ||
| .row | ||
| max-width: 100vw !important; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid !important and prefer 100% over 100vw.
This rule has two issues:
- The
!importantflag makes the CSS harder to maintain and override. Consider removing it and ensuring proper specificity instead. - Using
100vwcan cause horizontal scrollbars when the vertical scrollbar is visible, as100vwincludes the scrollbar width while the viewport does not. Usemax-width: 100%instead.
Apply this diff:
.main
.row
- max-width: 100vw !important;
+ max-width: 100%;🤖 Prompt for AI Agents
In packages/ui-default/pages/problem_config.page.styl around lines 2 to 4, the
rule uses "max-width: 100vw !important;" which is problematic: remove the
"!important" and replace "100vw" with "100%" so it becomes "max-width: 100%"; if
specificity issues remain, increase selector specificity or adjust stylesheet
order rather than using !important to enforce the rule.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui-default/package.json (1)
123-123: Verify webpack-manifest-plugin ^6.0.1 exists in npm registry.A previous review flagged that
webpack-manifest-pluginversion 6.0.0 does not exist in the npm registry (latest was 5.0.1). The version has been changed to 6.0.1, but this still needs verification to ensure the package can be resolved during installation.webpack-manifest-plugin 6.0.1 npm package release
🧹 Nitpick comments (2)
packages/ui-default/components/discussion/history.page.tsx (2)
48-54: Display the actual error message.The error message captured in state (line 36) is never shown to users. Line 51 only displays a generic "Loading failed." message, providing no insight into what went wrong.
Apply this diff to show the actual error:
{(isLoading || error) && ( <li className="menu__item"> <a className="menu__link"> - {isLoading ? i18n('Loading...') : i18n('Loading failed.')} + {isLoading ? i18n('Loading...') : `${i18n('Loading failed.')} ${error}`} </a> </li> )}
42-42: Consider resetting the load state on close for fresh data.The
loadstate is never reset tofalse, so closing and reopening the popover won't refetch the data. If the history might change and users expect fresh data on each open, consider adding anonClosehandler.If refetching is desired, add this:
-return <Popover onOpen={() => setLoad(true)}> +return <Popover onOpen={() => setLoad(true)} onClose={() => setLoad(false)}>Otherwise, the current caching behavior may be intentional and acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui-default/components/discussion/history.page.tsx(3 hunks)packages/ui-default/package.json(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui-default/components/discussion/history.page.tsx (2)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/misc/Page.ts (1)
AutoloadPage(39-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
packages/ui-default/package.json (2)
92-92: Verify React 19.2.0 compatibility with the build and runtime.The upgrade from React ^18.x to ^19.2.0 is a major version change with known breaking changes (ref prop handling, removed APIs like
propTypes/defaultPropsfor function components, string refs removal, etc.). Ensure:
- All ref callbacks are properly refactored (React 19 disallows returning values from ref callbacks).
- TypeScript types are updated correctly (your
@types/reactand@types/react-domare pinned to^19.2.2, which is good).- All JSX transform is using the modern 2020+ format.
- Any test utilities or legacy APIs have been migrated appropriately.
Also applies to: 96-96
23-25: Verify Mantine package version compatibility.The Mantine packages are pinned to different versions:
@mantine/core,@mantine/hooks,@mantine/notifications:^8.3.6mantine-contextmenu:^8.2.0postcss-preset-mantine:^1.18.0Confirm these versions are compatible with each other and the project's build configuration. If
postcss-preset-mantineis used in a PostCSS config, verify it is properly wired up in your Webpack/PostCSS pipeline.Also applies to: 70-70, 86-86
| React.useEffect(() => { | ||
| if (!load) return; | ||
| setLoading(true); | ||
| request.get(`${payload}?all=1`).then(({ history }) => { | ||
| setData(history); | ||
| }).catch((e) => { | ||
| setError(e.message); | ||
| }).finally(() => { | ||
| setLoading(false); | ||
| }); | ||
| }, [load]); |
There was a problem hiding this comment.
Reset error state before fetching and add request cleanup.
The error state is never cleared before making a new request. If a previous fetch failed, the stale error message persists in state even while isLoading is true, causing the condition on line 48 to show both loading and error simultaneously.
Apply this diff to reset the error state and add proper cleanup:
React.useEffect(() => {
if (!load) return;
+ setError('');
setLoading(true);
- request.get(`${payload}?all=1`).then(({ history }) => {
+ const abortController = new AbortController();
+ request.get(`${payload}?all=1`, { signal: abortController.signal }).then(({ history }) => {
setData(history);
}).catch((e) => {
+ if (e.name === 'AbortError') return;
setError(e.message);
}).finally(() => {
setLoading(false);
});
+ return () => abortController.abort();
}, [load]);🤖 Prompt for AI Agents
In packages/ui-default/components/discussion/history.page.tsx around lines 30 to
40, the effect does not clear previous error before starting a new fetch and
lacks cleanup to avoid updating state after unmount or when a later fetch
supersedes an earlier one; before calling request.get, clear the error state
(setError(null) or ''), then perform the fetch and on success/error update state
as now, and implement cleanup by using an AbortController (pass its signal to
request.get if supported) or a local "isMounted/isCurrent" flag to ignore
results from stale requests and prevent setState in the finally/then/catch when
the component is unmounted or the request was aborted.
1 participant
Summary by CodeRabbit
UI/Design Updates
Improvements
Bug Fixes