fix(dashboard): exclude hidden MarketplacePanel resources from sidebar menu#2177
fix(dashboard): exclude hidden MarketplacePanel resources from sidebar menu#2177
Conversation
IvanHunters
requested review from
androndo,
kvaps,
lexfrei,
lllamnyp and
sircthulhu
as code owners
dosubot
bot
added
the
size:S
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where resources configured as hidden within MarketplacePanels were still being displayed in the dashboard's sidebar menu. The changes introduce logic to fetch MarketplacePanel configurations during sidebar generation, allowing the system to identify and exclude hidden resources. This ensures that the sidebar accurately reflects the visibility status of marketplace items, improving the overall user experience and consistency of the dashboard. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe sidebar construction logic in the dashboard controller now includes a pre-processing step that lists MarketplacePanel resources, builds a hiddenResources map from each panel's spec.raw hidden flags, and filters CRDs from sidebar assembly based on that map. Listing errors are non-fatal and simply disable hiding. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller
participant K8sAPI as Kubernetes API
participant CRDStore as CRD list
participant Sidebar as Sidebar Builder
Controller->>K8sAPI: List MarketplacePanel
alt list successful
K8sAPI-->>Controller: MarketplacePanel list
Controller->>Controller: Decode spec.raw, build hiddenResources map
Controller->>CRDStore: List CRDs
CRDStore-->>Controller: CRD defs
Controller->>Controller: Filter out CRDs in hiddenResources
Controller->>Sidebar: Build sidebar from remaining CRDs
Sidebar-->>Controller: Sidebar structure
else list failed
K8sAPI-->>Controller: Error
Controller->>CRDStore: List CRDs
CRDStore-->>Controller: CRD defs
Controller->>Sidebar: Build sidebar from all CRDs (no hiding)
Sidebar-->>Controller: Sidebar structure
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/dashboard/sidebar.go (1)
41-56: Consider logging errors instead of silently swallowing them.While treating these errors as non-fatal is reasonable for graceful degradation, silently ignoring them could mask underlying issues (e.g., RBAC misconfiguration preventing MarketplacePanel listing, or malformed spec JSON). Adding debug/warning-level logs would aid troubleshooting without changing the behavior.
♻️ Suggested improvement
+ logger := log.FromContext(ctx) + // 1b) Fetch all MarketplacePanels to determine which resources are hidden hiddenResources := map[string]bool{} var mpList dashv1alpha1.MarketplacePanelList - if err := m.List(ctx, &mpList, &client.ListOptions{}); err == nil { + if err := m.List(ctx, &mpList, &client.ListOptions{}); err != nil { + logger.V(1).Info("failed to list MarketplacePanels, proceeding without hidden filtering", "error", err) + } else { for i := range mpList.Items { mp := &mpList.Items[i] if mp.Spec.Raw != nil { var spec map[string]any if err := json.Unmarshal(mp.Spec.Raw, &spec); err == nil { if hidden, ok := spec["hidden"].(bool); ok && hidden { hiddenResources[mp.Name] = true } + } else { + logger.V(1).Info("failed to unmarshal MarketplacePanel spec", "name", mp.Name, "error", err) } } } }As per coding guidelines,
internal/**/*.go: Follow controller-runtime patterns and kubebuilder style for Go code in internal directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/dashboard/sidebar.go` around lines 41 - 56, The loop that builds hiddenResources currently swallows errors from m.List and json.Unmarshal; update it to log non-fatal failures at debug/warn level so issues are visible without changing control flow: when calling m.List(ctx, &mpList, &client.ListOptions{}) log the returned err (use the controller logger or processLogger) if non-nil and continue, and inside the loop log json.Unmarshal failures for mp.Spec.Raw as well as unexpected types when asserting spec["hidden"].(bool); reference the hiddenResources map, mpList/MarketplacePanelList, m.List, mp.Spec.Raw, json.Unmarshal, and mp.Name when emitting messages to give clear context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/dashboard/sidebar.go`:
- Around line 41-56: The loop that builds hiddenResources currently swallows
errors from m.List and json.Unmarshal; update it to log non-fatal failures at
debug/warn level so issues are visible without changing control flow: when
calling m.List(ctx, &mpList, &client.ListOptions{}) log the returned err (use
the controller logger or processLogger) if non-nil and continue, and inside the
loop log json.Unmarshal failures for mp.Spec.Raw as well as unexpected types
when asserting spec["hidden"].(bool); reference the hiddenResources map,
mpList/MarketplacePanelList, m.List, mp.Spec.Raw, json.Unmarshal, and mp.Name
when emitting messages to give clear context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1585be49-40f4-4d21-87b6-cccb660553fa
📒 Files selected for processing (1)
internal/controller/dashboard/sidebar.go
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements the logic to exclude hidden MarketplacePanel resources from the sidebar menu. The approach of fetching all panels and checking the hidden flag is sound. My review includes a suggestion to improve error handling to make the feature more robust, particularly around API calls and JSON parsing, which currently allow for silent failures.
| if err := m.List(ctx, &mpList, &client.ListOptions{}); err == nil { | ||
| for i := range mpList.Items { | ||
| mp := &mpList.Items[i] | ||
| if mp.Spec.Raw != nil { | ||
| var spec map[string]any | ||
| if err := json.Unmarshal(mp.Spec.Raw, &spec); err == nil { | ||
| if hidden, ok := spec["hidden"].(bool); ok && hidden { | ||
| hiddenResources[mp.Name] = true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block has a couple of error handling issues that could lead to incorrect behavior or make debugging difficult:
-
m.Listerror is ignored: The logic is wrapped in anif err == nilblock. If fetchingMarketplacePanelListfails (e.g., due to a transient API server error), the function proceeds as if no resources are hidden. This would cause hidden resources to incorrectly appear in the sidebar. The error should be returned to trigger a reconciliation retry. -
json.Unmarshalerror is ignored: A malformedMarketplacePanelspec will be silently skipped. This makes debugging difficult. This error should at least be logged to help administrators identify misconfigured resources.
The suggestion below refactors the block to handle these errors more robustly.
if err := m.List(ctx, &mpList, &client.ListOptions{}); err != nil {
return fmt.Errorf("failed to list MarketplacePanels to check for hidden resources: %w", err)
}
for i := range mpList.Items {
mp := &mpList.Items[i]
if mp.Spec.Raw == nil {
continue
}
var spec map[string]any
if err := json.Unmarshal(mp.Spec.Raw, &spec); err != nil {
// TODO: Log this error. A malformed panel shouldn't fail the whole reconciliation.
continue
}
if hidden, ok := spec["hidden"].(bool); ok && hidden {
hiddenResources[mp.Name] = true
}
}
…r menu The sidebar was generated independently from MarketplacePanels, always showing all resources regardless of their hidden state. Fetch MarketplacePanels during sidebar reconciliation and skip resources where hidden=true, so hiding a resource from the marketplace also removes it from the sidebar navigation. Signed-off-by: IvanHunters <[email protected]>
kvaps
force-pushed
the
fix/sidebar-respect-hidden-marketplace
branch
from
5d77824 to
318079b
Compare
kvaps
added
backport
|
Successfully created backport PR for |
|
Successfully created backport PR for |
3 participants
Summary
hiddenstatehidden=trueTest plan
hidden: trueon a MarketplacePanel (e.g. qdrant)hidden: falseand verify the resource reappears in the sidebarSummary by CodeRabbit