Skip to content

Conversation

@vivekyadav-3
Copy link

This PR addresses the GSoC 2025 redesign goals for the Search interface (#3054) by:

  1. Modernizing the UI: Replaced the legacy Bootstrap/raw HTML form with a clean, responsive Shadcn UI Card layout.
  2. Standardizing Notifications: Migrated from the legacy Alert component to the new useNotification hook (Sonner) for consistent user feedback.
  3. Improving UX: Added clear sections for "Required" vs "Optional" filters using an Accordion to reduce visual clutter.
  • Refactored src/app/search/SearchClient.jsx.
  • Removed legacy Alert usage.
  • Implemented Card, Accordion, and Button components from the ui library.
  1. Navigate to /search.
  2. Verify the new Card-based layout.
  3. Perform a search (e.g., leaving fields empty) to trigger the new error notification toast.

Copilot AI review requested due to automatic review settings December 26, 2025 06:34
Copy link

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 modernizes the Search interface as part of the GSoC 2025 redesign initiative by migrating from legacy Bootstrap UI to Shadcn UI components and replacing the old Alert component with a new standardized notification system using the Sonner toast library.

Key Changes:

  • Introduced a new useNotification hook wrapping Sonner for consistent toast notifications across the application
  • Refactored SearchClient to use modern Card and Accordion components with improved visual hierarchy
  • Applied the notification migration to both SearchClient and HomeClient for consistency

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/hooks/use-notification.js New custom hook providing standardized notification methods (success, error, info, warning) using Sonner toast library
src/app/search/SearchClient.jsx Complete UI redesign using Shadcn Card, Accordion, and form components; replaced Alert with useNotification hook
src/app/HomeClient/HomeClient.jsx Migrated from inline Alert component to useNotification hook for error/success messages
src/components/Widgets/Alert/index.jsx Added auto-dismiss functionality via useEffect timer (5-second timeout)
src/app/layout.jsx Added Toaster component to root layout to enable toast notifications globally
src/styles/global.css Added CSS custom properties for notification colors and code formatting improvements

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

value={searchData.license}
onChange={handleChange}
placeholder="e.g. ^AGPL$"
/>
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The helpful example text has been removed from the License field. The previous version included: "For example, '^AGPL$'." While a similar example appears in the placeholder, users might benefit from understanding that regex patterns are supported. Consider adding a hint or tooltip to clarify this functionality.

Suggested change
/>
/>
<p className="mt-1 text-xs text-muted-foreground">
Supports regular expressions, for example: <code>^AGPL$</code>.
</p>

Copilot uses AI. Check for mistakes.
name="copyright"
value={searchData.copyright}
onChange={handleChange}
placeholder="e.g. Copyright 2024"
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The helpful example has been removed from the Copyright field. The previous version included: "For example, 'Copyright 2014-2020 fossology'." This gave users a clearer understanding of the expected format. While the placeholder shows a basic example, the more detailed example with year ranges was more informative.

Suggested change
placeholder="e.g. Copyright 2024"
placeholder="For example, 'Copyright 2014-2020 fossology'"

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +29
useEffect(() => {
const timer = setTimeout(() => {
setShow(false);
}, 5000);

return () => clearTimeout(timer);
}, [setShow]);
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The Alert component modification adds auto-dismiss functionality, but the useEffect has a potential issue with the dependency array. If the setShow function reference changes between renders, it will restart the timer unnecessarily. While this might not cause functional issues in most cases, it's better practice to either omit setShow from the dependencies (with an ESLint disable comment explaining why) or wrap it with useCallback in the parent component.

Suggested change
useEffect(() => {
const timer = setTimeout(() => {
setShow(false);
}, 5000);
return () => clearTimeout(timer);
}, [setShow]);
// We intentionally omit `setShow` from the dependency array to avoid
// restarting the auto-dismiss timer if the parent passes a new function
// reference on re-renders.
// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => {
const timer = setTimeout(() => {
setShow(false);
}, 5000);
return () => clearTimeout(timer);
}, []);

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +44
return toast.success(message, {
...options,
style: {
background: 'var(--success-bg, #e6ffed)',
borderColor: 'var(--success-border, #b7eb8f)',
color: 'var(--success-text, #14532d)',
},
});
};

const error = (message, options = {}) => {
return toast.error(message, {
duration: 5000, // Addresses Issue #154 in the new hook
...options,
style: {
background: 'var(--error-bg, #fff1f0)',
borderColor: 'var(--error-border, #ffa39e)',
color: 'var(--error-text, #721c24)',
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The useNotification hook's style object is being merged after the options spread, which means user-provided styles in options.style will be overridden by the hook's default styles. This prevents customization of notification appearance. Consider using a deep merge or spreading the options after the default styles to allow users to override specific style properties if needed.

Suggested change
return toast.success(message, {
...options,
style: {
background: 'var(--success-bg, #e6ffed)',
borderColor: 'var(--success-border, #b7eb8f)',
color: 'var(--success-text, #14532d)',
},
});
};
const error = (message, options = {}) => {
return toast.error(message, {
duration: 5000, // Addresses Issue #154 in the new hook
...options,
style: {
background: 'var(--error-bg, #fff1f0)',
borderColor: 'var(--error-border, #ffa39e)',
color: 'var(--error-text, #721c24)',
const { style, ...restOptions } = options;
return toast.success(message, {
...restOptions,
style: {
background: 'var(--success-bg, #e6ffed)',
borderColor: 'var(--success-border, #b7eb8f)',
color: 'var(--success-text, #14532d)',
...(style || {}),
},
});
};
const error = (message, options = {}) => {
const { style, ...restOptions } = options;
return toast.error(message, {
duration: 5000, // Addresses Issue #154 in the new hook
...restOptions,
style: {
background: 'var(--error-bg, #fff1f0)',
borderColor: 'var(--error-border, #ffa39e)',
color: 'var(--error-text, #721c24)',
...(style || {}),

Copilot uses AI. Check for mistakes.
value={searchData.filename}
onChange={handleChange}
placeholder="Enter filename (e.g. %v3.war)"
/>
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The helpful user guidance about wildcard usage has been removed. The previous version included: "You can use '%' as a wild-card. For example: '%v3.war', or 'mypkg%.tar'." While the placeholder now shows an example, the explanation of the wildcard syntax is missing, which could confuse users unfamiliar with SQL-style wildcards.

Suggested change
/>
/>
<p className="mt-1 text-sm text-muted-foreground">
You can use '%' as a wildcard. For example: '%v3.war' or 'mypkg%.tar'.
</p>

Copilot uses AI. Check for mistakes.
<Accordion type="single" collapsible className="w-full">
<AccordionItem value="search-criteria">
<AccordionTrigger className="text-lg font-semibold">
Required Search Criteria
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The heading text "You must choose one or more search criteria (not case sensitive)" was changed to "Required Search Criteria", but important information about case-insensitivity was lost. Users should know that searches are not case-sensitive, as this affects how they formulate their queries.

Suggested change
Required Search Criteria
Required Search Criteria (searches are not case sensitive)

Copilot uses AI. Check for mistakes.
search(searchData)
.then((result) => {
setSearchResult(result.search);
success(`Found ${result.search.length} files matching your search.`);
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The success notification in SearchClient shows a notification even when zero results are found (e.g., "Found 0 files matching your search."). This might not be the best user experience - finding zero results could be considered a neutral outcome or possibly an informational message rather than a success. Consider showing this notification only when results are found, or using an info notification when no results are found.

Suggested change
success(`Found ${result.search.length} files matching your search.`);
if (Array.isArray(result.search) && result.search.length > 0) {
success(`Found ${result.search.length} files matching your search.`);
}

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +13
/*
SPDX-FileCopyrightText: 2025 Antigravity AI

SPDX-License-Identifier: GPL-2.0-only

This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License
version 2 as published by the Free Software Foundation.
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.
*/
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The GPL license header is incomplete. Like the SearchClient.jsx file, it's missing the standard GPL boilerplate text: "You should have received a copy of the GNU General Public License along with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA." This text should be included to comply with GPL license requirements.

Copilot uses AI. Check for mistakes.
<div className="flex items-start gap-2">
<RadioGroupItem value="containers" id="containers" className="w-4 h-4 mt-1" />
<Label htmlFor="containers" className="text-base text-[#101010]">
Containers only (rpms, tars, isos, etc), excluding directories.
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The label text was truncated when removing the description about filtering for license or copyright not being supported for containers. The original text stated: "Containers only (rpms, tars, isos, etc), excluding directories. The filtering for license or copyright is not supported in this case." This important limitation should be preserved to inform users that license and copyright filtering won't work when this option is selected.

Suggested change
Containers only (rpms, tars, isos, etc), excluding directories.
Containers only (rpms, tars, isos, etc), excluding directories. The filtering for license or copyright is not supported in this case.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants