Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

UI fixes & improvments #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mhd-fettah wants to merge 69 commits into ChatGPTBox-dev:master
base: master
Choose a base branch
Loading
from mhd-fettah:master
Open

Conversation

Copy link
Contributor

@mhd-fettah mhd-fettah commented Mar 23, 2023

No description provided.

josStorer and others added 30 commits March 16, 2023 00:09
github-actions bot and others added 13 commits March 20, 2023 14:45 
* fix: don't break words
* fix: don't break words as much as possible, but when the width is not enough, will still break
---------
Co-authored-by: josc146 <josStorer@outlook.com>
Copy link
Member

thanks, i will make a test later

Copy link
Contributor Author

no thank youuuu bro for accepting contributions from us . I will keep helping there is many stuff in my mind to improve the plugin since I use it every day now .

Copy link
Member

I have conducted some tests and noticed that numerous styles and functions appear to be compromised. It might be beneficial for you to perform your own build tests to ensure everything is working as intended.

There are subtle differences between the icons in @primer/octicons-react and react-bootstrap-icons. In my opinion, the download button in @primer/octicons-react seems to be more visually appealing.

Please note that the icons in @primer/octicons-react do not support onClick. As a result, I utilized a span to achieve the desired functionality. It is essential not to remove this part, or the functionality will be lost.

The gpt-util-icon style is solely for changing the cursor to indicate that the element is clickable. This style is being used in many places, and adding extra styles to it has inadvertently caused disruptions in the appearance of other buttons.
mhd-fettah reacted with thumbs up emoji

Copy link
Contributor Author

I will try to build and check whats broken and fix it .
we need to standardize and improve the UI & UX
so the plugin can bypass others in term of approval for final user .

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances UI behavior and text rendering, expands configuration to support more models (including a custom API), and updates build assets and dependencies.

  • Extend configuration (defaultConfig, model keys, and getUserConfig) to cover GPT-4 variants and a custom API endpoint
  • Improve Markdown rendering by adding remark-breaks and setting dir="auto" for proper line breaks and text direction
  • Refactor UI components (FloatingToolbar, ConversationItem, ConversationCard, InputBox) with new CSS classes, pin/drag support, mobile selection handling, and responsive sizing
  • Add custom API streaming handler and adjust background scripts to route requests to the new endpoint
  • Update build pipeline (optional tiktoken stripping) and bump several dependencies

Reviewed Changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/config/index.mjs Added new model variants and custom API keys, reorganized config fields, and version compatibility
src/components/MarkdownRender/markdown.jsx Wrapped renderer in <div dir="auto"> and included remark-breaks for line-break support
src/components/FloatingToolbar/index.jsx Added mobile selection listener, responsive width, drag/dock/pin logic, and CSS class updates
src/components/ConversationCard/index.jsx Integrated window-size hook, pin/close controls, port passing, and updated icon components
build.mjs Enhanced webpack to optionally strip tiktoken usage and expanded asset inline rules
Comments suppressed due to low confidence (2)

src/background/apis/custom-api.mjs:1

  • The new generateAnswersWithCustomApi module adds significant logic paths that lack automated tests; consider adding unit or integration tests to cover successful streams, errors, and abort scenarios.
// custom api version

src/components/ConversationItem/index.jsx:17

  • You introduced .gpt-header-icons and .gpt-header-items classes to replace inline flex and gap styling; ensure corresponding CSS rules (e.g. display: flex; gap: 15px;) are present or add them to preserve the intended layout.
 <div className="gpt-header-icons">

/>
)}
<span
<Download
Copy link

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The Download icon is rendered without a size or margin styling, which may cause inconsistent spacing; consider adding size={16} and a className or inline style to match other toolbar icons.

Suggested change
<Download
<Download
size={16}
className="gpt-util-icon"

Copilot uses AI. Check for mistakes.

Comment on lines +269 to +275
await new Promise((r) => setTimeout(r, 5000))
await runWebpack(
true,
true,
generateWebpackCallback(() => finishOutput('-without-katex-and-tiktoken')),
)
await new Promise((r) => setTimeout(r, 5000))
Copy link

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Relying on fixed timeouts to sequence multiple webpack runs can lead to nondeterministic build behavior; consider chaining the runs using webpack's compiler hooks or Promises rather than setTimeout.

Suggested change
await new Promise((r) => setTimeout(r, 5000))
await runWebpack(
true,
true,
generateWebpackCallback(() => finishOutput('-without-katex-and-tiktoken')),
)
await new Promise((r) => setTimeout(r, 5000))
await runWebpack(
true,
true,
generateWebpackCallback(() => finishOutput('-without-katex-and-tiktoken')),
)

Copilot uses AI. Check for mistakes.

Comment on lines +124 to +125
for (const index in defaultConfig.selectionTools) {
const key = defaultConfig.selectionTools[index]
Copy link

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Using for...in over an array iterates numeric keys; consider for (const key of defaultConfig.selectionTools) for clearer intent and to avoid inherited properties.

Suggested change
for (const index in defaultConfig.selectionTools) {
const key = defaultConfig.selectionTools[index]
for (const [index, key] of defaultConfig.selectionTools.entries()) {

Copilot uses AI. Check for mistakes.

* @param {string} apiKey
* @param {string} modelName
*/
export async function generateAnswersWithCustomApi(port, question, session, apiKey, modelName) {
Copy link

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This function lacks JSDoc comments for its parameters and return behavior; adding documentation will help future maintainers understand the expected port usage, streaming lifecycle, and error cases.

Copilot uses AI. Check for mistakes.

Copy link
Member

/review
qodo-merge-pro[bot] reacted with eyes emoji

Copy link
Contributor

qodo-merge-pro bot commented Jun 2, 2025

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Duplicate Code

There's duplicate code for removing chatgptbox-container elements. The same unmounting and removal code appears twice (lines 42-45 and 51-54), which could lead to maintenance issues.

 
document.querySelectorAll('.chatgptbox-container').forEach((e) => {
 unmountComponentAtNode(e)
 e.remove()
})
let question
if (userConfig.inputQuery) question = await getInput([userConfig.inputQuery])
if (!question && siteConfig) question = await getInput(siteConfig.inputQuery)
document.querySelectorAll('.chatgptbox-container').forEach((e) => {
 unmountComponentAtNode(e)
 e.remove()
})
Error Handling

The stopListener is removed in onEnd but not in some error paths, which could lead to memory leaks if errors occur during the request.

 
 }
 let data
 try {
 data = JSON.parse(message)
 } catch (error) {
 console.debug('json error', error)
 return
 }
 if (data.conversation_id) session.conversationId = data.conversation_id
 if (data.message?.id) session.parentMessageId = data.message.id
 answer = data.message?.content?.parts?.[0]
 if (answer) {
 port.postMessage({ answer: answer, done: false, session: session })
 }
},
async onStart() {
 // sendModerations(accessToken, question, session.conversationId, session.messageId)
},
async onEnd() {
 port.onMessage.removeListener(stopListener)
},
async onError(resp) {
 if (resp instanceof Error) throw resp
 port.onMessage.removeListener(stopListener)
 if (resp.status === 403) {
Font Consistency

The Cairo font is applied inconsistently. It's set for .chatgptbox-container * (line 52) but also specifically for .markdown-body (line 125), which could cause styling inconsistencies.

 
.chatgptbox-container * {
 font-family: 'Cairo', sans-serif;
}
.gpt-inner {
 border-radius: 8px;
 border: 1px solid;
 overflow: hidden;
 border-color: var(--theme-border-color);
 background-color: var(--theme-color);
 margin: 0;
 hr {
 height: 1px;
 background-color: var(--theme-border-color);
 border: none;
 }
}
.markdown-body {
 padding: 5px 15px 10px;
 background-color: var(--theme-color);
 color: var(--font-color);
 resize: vertical;
 overflow-y: auto;
 ul,
 ol {
 padding-left: 1.5em;
 }
 ol {
 list-style: none;
 counter-reset: item;
 li {
 counter-increment: item;
 &::marker {
 content: counter(item) '. ';
 }
 }
 }
}
.icon-and-text {
 color: var(--font-color);
 display: flex;
 align-items: center;
 padding: 15px;
 gap: 6px;
}
.manual-btn {
 cursor: pointer;
}
.gpt-loading {
 color: var(--font-color);
 animation: chatgptbox-pulse 2s cubic-bezier(0.4, 0, 0.6, 1) infinite;
}
.code-copy-btn {
 color: inherit;
 position: absolute;
 right: 10px;
 top: 3px;
 cursor: pointer;
}
:is(.answer, .question, .error) {
 font-size: 15px;
 line-height: 1.6;
 border-radius: 8px;
 word-break: break-word;
 font-family: 'Cairo', sans-serif;

Copy link
Member

/improve
qodo-merge-pro[bot] reacted with eyes emoji

Copy link
Contributor

qodo-merge-pro bot commented Jun 2, 2025

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Code Suggestions ✨

CategorySuggestion Impact
Possible issue
Fix position calculation logic

The current implementation has a logic error. When window.innerWidth -
element.offsetWidth is negative, the inner Math.max(0, ...) will force it to 0,
making the element always align to the left edge when it's wider than the
viewport. Instead, use a simpler approach to constrain the position.

src/utils/set-element-position-in-viewport.mjs [2-3] 

-const retX = Math.min(Math.max(0, window.innerWidth - element.offsetWidth), Math.max(0, x))
-const retY = Math.min(Math.max(0, window.innerHeight - element.offsetHeight), Math.max(0, y))
+const retX = Math.max(0, Math.min(window.innerWidth - element.offsetWidth, x))
+const retY = Math.max(0, Math.min(window.innerHeight - element.offsetHeight, y))
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a logic issue where elements wider than the viewport would always be positioned at x=0 due to the nested Math.max(0, ...) calls. The proposed fix provides a cleaner approach to constrain positioning within viewport bounds.

Medium
General
Limit font-family scope

The font-family declaration is applied to all elements within the container,
which can override intended font styles for code blocks and other specialized
elements. This should be more targeted to apply only to text content elements.

src/content-script/styles.scss [51-53] 

-.chatgptbox-container * {
+.chatgptbox-container .markdown-body p,
+.chatgptbox-container .markdown-body li,
+.chatgptbox-container .markdown-body h1,
+.chatgptbox-container .markdown-body h2,
+.chatgptbox-container .markdown-body h3,
+.chatgptbox-container .markdown-body h4 {
 font-family: 'Cairo', sans-serif;
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: Good suggestion to avoid unintended font application to specialized elements like code blocks and icons by making the selector more specific rather than using universal selector.

Low
Add minimum height constraint

Using a percentage of window height for maxHeight without a minimum value could
make the content area too small on certain devices. Add a minimum height to
ensure usability on all screen sizes.

src/components/ConversationCard/index.jsx [235] 

 <div
 ref={bodyRef}
 className="markdown-body"
- style={{ maxHeight: windowSize[1] * 0.75 + 'px' }}
+ style={{ maxHeight: Math.max(300, windowSize[1] * 0.75) + 'px' }}
 >

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: Reasonable UX improvement to prevent content area from becoming too small on certain screen sizes, though not critical functionality.

Low
Add font fallbacks

Using a single font without fallbacks can cause text rendering issues if 'Cairo'
fails to load. Add system font fallbacks to ensure text remains readable in all
situations.

src/content-script/styles.scss [52] 

-font-family: 'Cairo', sans-serif;
+font-family: 'Cairo', system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, sans-serif;
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: Adding font fallbacks is good practice, but the suggestion doesn't specify which of the multiple font-family declarations it refers to, making it ambiguous.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /