fix: assistant style compatibility #225
Conversation
close DOC-92
🦋 Changeset detectedLatest commit: 777e976 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughPatch release for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
commit: |
Signed-off-by: JounQin <admin@1stg.me>
There was a problem hiding this comment.
Pull request overview
This PR improves the AI assistant component's styling and robustness by updating CSS properties for better compatibility and adding proper cleanup to side effects.
- Updated z-index to use a CSS variable for better maintainability
- Simplified CSS functions and improved height calculation to account for fixed positioning
- Added cleanup function to prevent memory leaks when setting document title
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/doom/styles/ai-assistant.module.scss | Updated z-index to use CSS variable, simplified width calculation, and improved height calculation to account for spacing |
| packages/doom/src/global/SiteOverrides/index.tsx | Replaced requestAnimationFrame with setTimeout and added cleanup function to prevent memory leaks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/doom/src/global/SiteOverrides/index.tsx (1)
92-94: Consider aligning the scheduling approach or add cleanup.This still uses
requestAnimationFramewhile the document.title update (lines 71-77) was changed tosetTimeout. This creates an inconsistency in scheduling approach.Additionally, there's no cleanup function to cancel the pending RAF if the component unmounts or logoText changes before execution.
🔎 Suggested fix to align with setTimeout approach and add cleanup
- requestAnimationFrame(() => { + const timeoutId = setTimeout(() => { setNavBarSpans(logoText) - }) + }, 1) + + return () => { + clearTimeout(timeoutId) + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/two-rabbits-taste.md(1 hunks)packages/doom/src/global/SiteOverrides/index.tsx(1 hunks)packages/doom/styles/ai-assistant.module.scss(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx,json,yml,yaml,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier according to the project's Prettier configuration
Files:
packages/doom/src/global/SiteOverrides/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode for type safety
Files:
packages/doom/src/global/SiteOverrides/index.tsx
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow ESLint rules defined in eslint.config.js
Files:
packages/doom/src/global/SiteOverrides/index.tsx
🧠 Learnings (2)
📚 Learning: 2025-05-26T09:09:21.339Z
Learnt from: JounQin
Repo: alauda/doom PR: 30
File: src/runtime/components/K8sCrd.tsx:5-5
Timestamp: 2025-05-26T09:09:21.339Z
Learning: The alauda/doom project uses yarn v4 for dependency management, which handles package exports and module resolution correctly, so imports like `rspress/core/theme` work without any ESLint errors.
Applied to files:
.changeset/two-rabbits-taste.md
📚 Learning: 2025-06-09T03:10:41.010Z
Learnt from: JounQin
Repo: alauda/doom PR: 75
File: src/cli/load-config.ts:4-7
Timestamp: 2025-06-09T03:10:41.010Z
Learning: The alauda/doom project uses yarn v4 as the package manager, not npm. Always reference yarn commands when suggesting package management operations.
Applied to files:
.changeset/two-rabbits-taste.md
⏰ 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). (5)
- GitHub Check: Build and Lint with Node.js 22 on ubuntu-latest
- GitHub Check: Build and Lint with Node.js 24 on ubuntu-latest
- GitHub Check: Build and Lint with Node.js 20 on ubuntu-latest
- GitHub Check: Build and Lint with Node.js 22 on ubuntu-latest
- GitHub Check: Build and Lint with Node.js 20 on ubuntu-latest
🔇 Additional comments (3)
packages/doom/styles/ai-assistant.module.scss (2)
7-7: LGTM! Using CSS variable for z-index.Replacing the hard-coded z-index value with
var(--rp-z-index-nav)improves consistency and ensures the assistant panel maintains proper layering relative to the navigation component.
9-10: LGTM! Improved sizing calculations.The changes correctly:
- Remove the unnecessary
calc()wrapper from the width'smin()function- Adjust the height calculation to subtract 2rem from the viewport height before taking the minimum, which properly accounts for the container's positioning (1rem bottom + spacing for visual breathing room)
These changes improve both the correctness and clarity of the sizing logic.
packages/doom/src/global/SiteOverrides/index.tsx (1)
71-77: Clarify the timing strategy: why setTimeout(1) for title but requestAnimationFrame for navbar?Lines 71-73 use
setTimeout(1)to updatedocument.title, while line 92 usesrequestAnimationFrameforsetNavBarSpans. These have different timing guarantees—RAF synchronizes with the next paint, whilesetTimeout(1)defers with minimal delay. The cleanup is correct, but this inconsistency should either be intentional and documented, or unified.
close DOC-92
Summary by CodeRabbit
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.