Conversation
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 addresses a critical bug in the dashboard controller where user-configured 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)
📝 WalkthroughWalkthroughRefactors the MarketplacePanel controller to construct the ArbitrarySpec inside the CreateOrUpdate callback, reading and preserving existing Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as Reconciler (controller)
participant API as Kubernetes API
participant Resource as MarketplacePanel resource
Reconciler->>API: Get existing MarketplacePanel
API-->>Reconciler: existing resource (may include Spec.Raw)
Reconciler->>Reconciler: decode Spec.Raw -> extract disabled/hidden (default false)
Reconciler->>Reconciler: build specMap including preserved values
Reconciler->>API: CreateOrUpdate MarketplacePanel with new ArbitrarySpec (only if JSON differs)
API-->>Reconciler: result (created/updated/no-op)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
IvanHunters
requested review from
androndo,
kvaps,
lexfrei,
lllamnyp and
sircthulhu
as code owners
dosubot
bot
added
size:M
![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
The pull request correctly addresses an issue where disabled and hidden states on MarketplacePanel resources were being overwritten during reconciliation. The change preserves these user-set values by reading them from the existing resource before constructing the new spec. The implementation is sound. I have one suggestion to improve observability by logging an error if parsing the existing spec fails, which would help in debugging cases where the state might be unexpectedly reset.
| if err := json.Unmarshal(mp.Spec.Raw, &existing); err == nil { | ||
| if v, ok := existing["disabled"].(bool); ok { | ||
| disabled = v | ||
| } | ||
| if v, ok := existing["hidden"].(bool); ok { | ||
| hidden = v | ||
| } | ||
| } |
There was a problem hiding this comment.
The error from json.Unmarshal is silently ignored. While falling back to default values is a reasonable strategy if the existing spec is malformed, logging the error would provide valuable information for debugging, especially if user-configured disabled or hidden states are unexpectedly reset.
if err := json.Unmarshal(mp.Spec.Raw, &existing); err != nil {
logger.Info("Failed to unmarshal existing MarketplacePanel spec, using default disabled/hidden values", "name", mp.Name, "error", err)
} else {
if v, ok := existing["disabled"].(bool); ok {
disabled = v
}
if v, ok := existing["hidden"].(bool); ok {
hidden = v
}
}![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/marketplacepanel.go (1)
78-118: Please lock this in with a reconciliation regression test.This behavior now depends on two subtle paths: preserving existing booleans from
Spec.Rawon update, and defaulting both tofalseon create. A focused controller test for both cases would help prevent a future refactor from reintroducing the overwrite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/dashboard/marketplacepanel.go` around lines 78 - 118, Add a reconciliation regression test that covers both create and update paths for preserving the "disabled" and "hidden" booleans: simulate creating a MarketplacePanel (use the same controller reconcile entrypoint used in tests) and assert the stored mp.Spec.JSON (ArbitrarySpec/JSON.Raw) contains "disabled": false and "hidden": false by default; then simulate an update where mp.Spec.Raw contains {"disabled": true, "hidden": true}, run the reconcile logic so the code path that reads mp.Spec.Raw and builds specMap (the logic that sets disabled/hidden and constructs newSpec/ArbitrarySpec and calls compareArbitrarySpecs) runs, and assert the reconciled resource preserves those true values rather than overwriting them. Ensure the test exercises the compareArbitrarySpecs branch that skips assignment when specs are equal and fails if the booleans are lost.
🤖 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/marketplacepanel.go`:
- Around line 78-118: Add a reconciliation regression test that covers both
create and update paths for preserving the "disabled" and "hidden" booleans:
simulate creating a MarketplacePanel (use the same controller reconcile
entrypoint used in tests) and assert the stored mp.Spec.JSON
(ArbitrarySpec/JSON.Raw) contains "disabled": false and "hidden": false by
default; then simulate an update where mp.Spec.Raw contains {"disabled": true,
"hidden": true}, run the reconcile logic so the code path that reads mp.Spec.Raw
and builds specMap (the logic that sets disabled/hidden and constructs
newSpec/ArbitrarySpec and calls compareArbitrarySpecs) runs, and assert the
reconciled resource preserves those true values rather than overwriting them.
Ensure the test exercises the compareArbitrarySpecs branch that skips assignment
when specs are equal and fails if the booleans are lost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a938bd3f-cb7f-45e2-adc0-c4839c92cf13
📒 Files selected for processing (1)
internal/controller/dashboard/marketplacepanel.go
…conciliation The controller was hardcoding disabled=false and hidden=false on every reconciliation, overwriting any user changes made through the dashboard UI. Move spec building inside the CreateOrUpdate mutate function to read and preserve current disabled/hidden values from the existing resource. Signed-off-by: IvanHunters <[email protected]>
kvaps
force-pushed
the
fix/marketplace-preserve-disabled-hidden
branch
from
eab121c to
e69efd8
Compare
kvaps
added
backport
|
Successfully created backport PR for |
|
Successfully created backport PR for |
3 participants
Summary
disabledandhiddenvalues during controller reconciliationdisabled=falseandhidden=falseon every reconcile loop, overwriting any changes made through the dashboard UITest plan
Summary by CodeRabbit