fix: standardize modal spacing

[botshen]

Summary

• centralize WKModal shell/title/body/footer spacing so common dialogs no longer rely on Semi default modal margins
• add wkConfirm and migrate confirm dialogs to the shared modal button/footer layout
• clean up voice settings modal internals while leaving container spacing to WKModal
• update edge-to-edge modal overrides to target WKModal shell classes instead of Semi internals

Closes #185

Verification

• pnpm --filter @octo/base exec vitest run src/Components/NavRail/tests/VoiceSettingsPanel.test.tsx
• git diff --check
• pnpm build

[lml2468]

Summary

Standardizes all modal usage across 22 files: replaces Modal.confirm with a custom wkConfirm wrapper, removes .semi-modal-* CSS overrides and !important declarations in favor of .wk-modal-shell/.wk-modal-content class hooks. Adds a footerConfig prop to WKModal for declarative footer rendering. Extracts VoiceSettingsPanel inline styles to a CSS file. Refactors the Conversation delete confirmation from an inline JSX modal to imperative wkConfirm.

Findings

No blocking issues.

• P2 confirm.tsx:21 — closeAfter does .then(() => destroy()) on the onOk promise without a .catch(). If an onOk handler rejects (e.g. the Conversation delete handler does throw e on failure), this produces an unhandled promise rejection in the browser console and the modal stays open with no way to close except Cancel/Esc. Fix: add .catch(() => {}) or .catch(() => modalRef?.destroy()) depending on desired UX. Functional impact is minor since the user can still dismiss via Cancel.

• P2 Behavioral change in Conversation delete: the old code closed the modal before the async delete (setState then deleteMessages); the new code keeps the modal open during the async operation and closes only on success. This is actually better UX (prevents orphan operations) but is worth noting since it changes user-visible behavior.

Verification

• All Modal.confirm → wkConfirm replacements preserve callback signatures and i18n keys. icon: null removals are correct since wkConfirm passes icon: null by default.
• ThreadPanel setTimeout pattern preserved (avoids Popover close event dismissing the modal).
• CSS class renaming is consistent: .semi-modal-body → .wk-modal-shell, .semi-modal-content → .wk-modal-content. All !important overrides eliminated.
• footerConfig in Chat/index.tsx correctly replaces the inline footer JSX with equivalent declarative config.
• showDeleteConfirm state removed from Conversation — no orphan references (grep confirmed).
• Modal import removed from all files that switched to wkConfirm — no unused imports.
• Build CI passed.

Verdict

APPROVED — clean refactoring, good centralization of modal patterns, proper elimination of !important anti-pattern.

[Jerry-Xin]

Summary: The PR is in scope and the modal spacing direction is sound, but the new shared confirm wrapper has a blocking async-action regression.

🔴 Blocking

• 🔴 Critical — wkConfirm leaves async destructive actions repeatable and does not handle rejected onOk promises. In packages/dmworkbase/src/Components/WKModal/confirm.tsx:17, closeAfter waits for promise fulfillment before destroying the modal, but the OK button at confirm.tsx:53 stays enabled, so users can click repeatedly while the request is in flight. Several migrated callers perform destructive operations, for example message deletion in packages/dmworkbase/src/Components/Conversation/index.tsx:2033; that caller also rethrows on failure at Conversation/index.tsx:2045, which becomes an unhandled rejection because closeAfter only attaches a fulfillment handler. Add pending state/loading or a one-shot guard, disable buttons while pending, and handle rejection explicitly so the modal either stays open cleanly or closes according to the intended semantics.

💬 Non-blocking

• 🟡 Warning — WKConfirmProps exposes the broad Semi modal prop surface, but the custom footer ignores button-level confirm props such as okButtonProps, cancelButtonProps, hasCancel, or autofocus-related behavior. Either narrow the public type to the supported API or map the common props onto the custom WKButtons.

• 🟡 Warning — WKModal now renders custom footer content inside .wk-modal-shell instead of Semi’s footer region. Existing custom footer fragments without their own wrapper, such as packages/dmworkbase/src/Components/NavRail/NavSettingsPanel.tsx:228, may lose right alignment and spacing. Consider normalizing custom footers through .wk-modal-footer or documenting that custom footer callers must provide their own layout.

✅ Highlights

• ✅ The PR removes several direct Semi internal modal overrides and moves common spacing into WKModal-owned classes, which aligns better with the project’s styling rules.
• ✅ The voice settings modal cleanup moves inline styling into tokenized CSS and keeps container spacing owned by WKModal.

Verification note: git diff --check passed locally. I could not run the listed Vitest command because this workspace does not have vitest available through pnpm --filter @octo/base exec.

[lml2468]

Summary (re-review of b745c08)

Addresses all previous review findings from three reviewers.

Previous Findings Resolution

1. RESOLVED (P2, my review + Allen) — closeAfter missing .catch(): Replaced with runAction that has proper .catch(() => updatePending(null)). On rejection, pending state resets so buttons become clickable again for retry.

2. RESOLVED (P1, Jerry-Xin + Allen) — async onOk not disabling buttons: Added WKConfirmPending state. Both buttons get disabled={pending !== null} and loading={pending === action} during async operations. if (pending) return guards against double-click. modalRef.update() re-renders content with new button states.

3. RESOLVED (P2, Jerry-Xin) — WKConfirmProps exposing too many Semi props: Now explicitly omits cancelButtonProps, cancelLoading, confirmLoading, footer, hasCancel, okButtonProps — props not wired to the custom UI.

4. RESOLVED (P2, Allen) — custom footer rendering position: Custom footer JSX now wrapped in <div className="wk-modal-footer"> for consistent layout.

New Findings

None.

Verification

• runAction flow: guard → callback → if promise: set pending → .then(destroy) / .catch(reset) → buttons re-enable on failure. Correct.
• renderContent is a pure function of pending state, called via modalRef.update() for re-renders. Clean approach.
• Sync callbacks still destroy immediately (no unnecessary pending flash).
• Build CI passed.

Verdict

APPROVED — all blocking findings fixed cleanly.

[botshen]

Addressed the blocking async confirm regression in b745c08.

Changes made:

• wkConfirm now tracks a one-shot pending state for async OK/Cancel actions.
• Buttons are disabled while an async action is pending, with loading shown on the active button, so destructive actions cannot be repeated.
• Rejected promises are caught; the modal stays open and returns to an actionable state without an unhandled rejection.
• WKConfirmProps is narrowed to exclude unsupported Semi footer/button props (okButtonProps, cancelButtonProps, hasCancel, loading props, etc.) so the custom footer API is explicit.
• Custom WKModal footers are now wrapped in .wk-modal-footer, preserving right alignment/spacing for fragment footers such as NavSettingsPanel.

Verification rerun:

• pnpm --filter @octo/base exec vitest run src/Components/NavRail/__tests__/VoiceSettingsPanel.test.tsx
• git diff --check
• pnpm build

[Jerry-Xin]

The PR is in scope for octo-web, but the modal spacing standardization currently relies on a class prop that Semi does not apply.

🔴 Blocking

• 🔴 Critical: modalContentClass is not a supported Semi Modal prop in the installed @douyinfe/semi-ui@2.24.2 API, so wk-modal-content is never attached to the modal content element. This makes .wk-modal-content { padding: 0; } a no-op and breaks the core goal of removing Semi’s default content padding. It also means overrides such as .claw-info-modal .wk-modal-content will not apply, while the new .wk-modal-shell padding is added inside Semi’s existing spacing, causing doubled/incorrect modal spacing. This affects both normal modals and confirms at packages/dmworkbase/src/Components/WKModal/index.tsx:137, packages/dmworkbase/src/Components/WKModal/confirm.tsx:90, and the dependent CSS at packages/dmworkbase/src/Components/WKModal/index.css:19. Use the actual content class hook wired by this Semi version, or add a typed wrapper/cast for the supported runtime prop before relying on .wk-modal-content.

💬 Non-blocking

• 🟡 Warning: I could not run the targeted Vitest command locally because dependencies are not installed in this worktree: Command "vitest" not found.

✅ Highlights

• The PR correctly moves confirm call sites toward a shared wkConfirm layout and removes several direct Semi confirm imports.
• The VoiceSettingsPanel cleanup improves token usage and removes a large amount of inline styling.

[botshen]

Addressed Jerry-Xin's blocking modalContentClass concern in 89697f0.

What changed:

• Aligned @octo/base's declared @douyinfe/semi-ui dependency from ^2.24.2 to ^2.93.0, matching the existing lockfile-resolved version and the other workspace packages.
• Updated pnpm-lock.yaml so the importer specifier also reflects ^2.93.0.
• Verified the installed Semi source wires modalContentClass through to ModalContent (Modal.tsx passes it into contentClassName), so .wk-modal-content is now relying on a declared/supported API rather than a prop outside the package range.
• Removed a stale .wk-base-modal-forward .wk-modal-content width override because that modal already owns width through WKModal width={625}.
• Replaced the ClawInfo modal's hardcoded radius with var(--wk-r-lg) while keeping its edge-to-edge shell behavior.

Verification rerun:

• pnpm --filter @octo/base exec vitest run src/Components/NavRail/__tests__/VoiceSettingsPanel.test.tsx
• git diff --check
• pnpm build

Build still reports the existing eval/chunk/dynamic-import warnings, but exits successfully.

[github-actions]

Dependency Changes Detected

This PR modifies dependency files. Please review whether these changes are intentional.

Changed files:

• packages/dmworkbase/package.json
• pnpm-lock.yaml

Maintainer checklist:

• Confirm dependency changes are intentional
• Review package delta if lockfile changed

[lml2468]

Summary (re-review of 89697f0)

Bumps @douyinfe/semi-ui specifier from ^2.24.2 to ^2.93.0, hardcoded border-radius: 18px to var(--wk-r-lg), and removes dead CSS rule.

Previous Findings Resolution

CLARIFICATION on modalContentClass: The pnpm-lock.yaml shows version: 2.93.0 was the resolved version in BOTH the old and new commits — only the specifier changed. The actual installed Semi UI was always 2.93.0, which supports modalContentClass. The previous CHANGES_REQUESTED findings by Jerry-Xin and Allen confused the minimum specifier (^2.24.2) with the resolved version (2.93.0). That said, bumping the specifier to ^2.93.0 is the right thing — it makes the minimum requirement explicit and prevents accidental downgrade.

Changes in this commit

1. package.json: @douyinfe/semi-ui specifier ^2.24.2 → ^2.93.0 (resolved version unchanged at 2.93.0)
2. ClawInfoModal.css: border-radius: 18px → var(--wk-r-lg) (design token consistency)
3. WKBase/index.css: removed dead .wk-base-modal-forward .wk-modal-content { width: 625px; } rule

New Findings

None.

Verdict

APPROVED — specifier aligned with reality, minor CSS cleanup.

[Jerry-Xin]

The PR is in scope and the modal spacing changes are aligned with the octo-web frontend; I found no blocking issues.

💬 Non-blocking

• 🟡 Warning: confirm.tsx introduces a shared imperative confirm flow used by many destructive actions, but there are no direct tests for it. Consider adding focused coverage for async onOk, rejected promises, cancel behavior, and custom React content.
• 🔵 Suggestion: index.tsx and confirm.tsx render native buttons through custom controls; setting type="button" would avoid accidental form submission if modal content later contains a form.

✅ Highlights

• The PR removes direct Semi modal spacing overrides from migrated CSS and targets WKModal shell classes instead.
• wkConfirm centralizes confirm footer/button styling and preserves async loading behavior for most migrated flows.
• git diff --check passed.

Caveat: I could not rerun the targeted Vitest command locally because dependencies are not installed in this worktree (vitest was not found).

[yujiawei]

Code Review — PR #189 (octo-web)

Verdict: APPROVED ✅

A well-executed, focused refactor. It centralizes modal spacing into WKModal-owned class hooks (.wk-modal-shell / .wk-modal-content), eliminates the !important overrides on Semi internals, adds a shared wkConfirm wrapper with proper async-action guarding, and extracts VoiceSettingsPanel inline styles into a CSS file. All previously-raised blocking findings are resolved. The remaining items below are non-blocking suggestions.

1. Verification of the core change

• ✅ modalContentClass is now valid. This was the prior blocking concern. The package.json bump to ^2.93.0 (packages/dmworkbase/package.json:10) makes modalContentClass a typed ModalProps member, and it is applied at runtime to the content element (@douyinfe/semi-ui modal/Modal.js → contentClassName). It did not exist in 2.24.2. The lockfile resolves to a single semi-ui@2.93.0, and every other package in the monorepo was already on ^2.93.0, so this only aligns the declared dep with what was already installed — not a real runtime version jump.
• ✅ No double-padding. .wk-modal-content { padding: 0 } zeroes Semi's content padding; .wk-modal-shell { padding: var(--wk-sp-6) } provides the single source of spacing. Per-modal overrides (.claw-info-modal .wk-modal-shell, .wk-base-modal-* .wk-modal-shell, etc.) target the correct elements.
• ✅ wkConfirm async guarding is correct (WKModal/confirm.tsx): runAction guards re-entry (if (pending) return), sets pending, disables both buttons, and on rejection resets pending via .catch so the modal stays open and retryable. Sync callbacks destroy immediately.
• ✅ Conversation delete migration (Conversation/index.tsx) preserves the empty-selection guard and only runs editOn=false / unCheckAllMessages on the success path; showDeleteConfirm state fully removed with no orphan refs.
• ✅ Footer handling improved. resolveFooter now wraps custom footer JSX in .wk-modal-footer, which fixes the earlier concern about custom footers losing right-alignment.
• ✅ Chat footerConfig migration (Pages/Chat/index.tsx) is behavior-equivalent to the old inline footer.
• ✅ VoiceSettingsPanel inline-style → CSS-class extraction is faithful; all referenced classes exist in the new CSS file; the renamed test documents the intentional move of the title into a primary row.
• ✅ CI Build passed. (The only failing check, check-sprint, is unrelated to this code.)

2. Findings

No P0/P1 issues. The following are P2 / nit — none block merge:

• P2 — Footer auto-wrap affects callers outside this diff. resolveFooter now wraps any custom footer JSX in .wk-modal-footer (display:flex; justify-content:flex-end; align-items:center). MessageInput/VoiceFeedbackNotice.tsx:63 passes a flex-direction: column footer (border separator + full-width checkbox + button row). As a single flex item inside the new row wrapper it will shrink-wrap and right-align, so the separator/checkbox may no longer span full width. Worth a visual check on the voice-feedback notice and the NavSettingsPanel update modal. Consider only wrapping when the footer isn't already a layout container, or documenting that custom footers own their own layout.

• P2 — Latent onCancel double-path in wkConfirm (WKModal/confirm.tsx): onCancel is both invoked via runAction (custom Cancel button) and passed directly to Modal.confirm (line ~94, used by Esc/mask dismissal). The direct path has no pending-state protection, so a future caller passing an async onCancel while maskClosable/closeOnEsc are enabled would bypass the guard. No current caller passes onCancel, so this is latent — but worth routing both paths through runAction or narrowing the type.

• P2 — Imprecise cast (WKModal/confirm.tsx:33): modalRef?.update({ content: ... } as WKConfirmProps) casts a partial update to the full public props type. update takes a partial; prefer letting TS infer or casting to Partial<ModalReactProps>.

• nit — Dangling class (NavRail/VoiceSettingsPanel.tsx:224): className="wk-voice-settings-modal" has no matching rule in VoiceSettingsPanel.css. Harmless, but either define it or drop it.

• nit — Disabled-button styling shift (NavRail/VoiceSettingsPanel.tsx): the old Save button swapped to a distinct disabled background; the new WKButton disabled state is opacity: 0.4 + cursor: not-allowed. State is still communicated, just a cosmetic change — flagging in case the distinct disabled color was intended.

3. Note on a prior review comment

An earlier round flagged that the removed .wk-base-modal .semi-modal-body-wrapper { margin: 0 } overrides would leave Semi's default 24px wrapper margin uncontrolled. That does not apply to WKModal-based modals: WKModal always passes header={null}, and Semi's renderBody only emits .semi-modal-body-wrapper when there is no header (hasHeader = title != null || 'header' in props). With header present in props, Semi renders .semi-modal-body directly, so .semi-modal-body-wrapper is not in WKModal's DOM and removing those overrides is correct cleanup. (The .semi-modal-body-wrapper rules that remain elsewhere belong to components using raw Semi Modal, not WKModal.)

Summary

Clean centralization of modal patterns, correct elimination of the !important anti-pattern, and proper async-confirm guarding. The version bump correctly enables modalContentClass. Approving; please give the custom-footer callers a quick visual pass and consider the onCancel hardening before relying on it more broadly.