fix(dashboard): fix External IPs factory EnrichedTable rendering#2175
fix(dashboard): fix External IPs factory EnrichedTable rendering#2175
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 resolves an issue where the External IPs page in the dashboard was not displaying data, appearing with empty rows. The problem stemmed from misconfigured properties within the 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)
📝 WalkthroughWalkthroughUpdated the EnrichedTable configuration for the external-ips services tab: renamed the key "clusterNamePartOfUrl" to "cluster" and changed "pathToItems" from an array Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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:S
![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 addresses a rendering issue on the External IPs page by updating the properties of an EnrichedTable component, specifically replacing clusterNamePartOfUrl with cluster and modifying pathToItems to use a dot-path string. A security audit identified two existing vulnerabilities in the dashboard controller logic: a recursive function in customformsoverride.go lacks a depth limit, potentially leading to a stack overflow and Denial of Service (DoS), and factory.go constructs UI templates using unescaped property names from OpenAPI schemas, which could result in template injection or Cross-Site Scripting (XSS) in the dashboard frontend. Additionally, there's an inconsistency in the pathToItems format across the codebase, with several instances still using an old array format, which could impact maintainability and lead to similar issues elsewhere.
| "cluster": "{2}", | ||
| "baseprefix": "/openapi-ui", | ||
| "customizationId": "factory-details-v1.services", | ||
| "pathToItems": ".items", |
There was a problem hiding this comment.
While changing pathToItems to a dot-path string fixes the issue here, the PR description claims this is to match a convention that is not consistently applied throughout the codebase. I've found other EnrichedTable instances that still use the array format for pathToItems:
internal/controller/dashboard/static_refactored.go: line 1318 ([]any{"spec", "rules"}) and line 1474 ([]any{"items"})internal/controller/dashboard/factory.go: multiple instances (e.g., line 171, 196, 272, etc.)
If the array format is indeed incorrect, leaving these other instances unchanged could lead to similar bugs elsewhere. For consistency and to improve maintainability, I recommend refactoring all instances. Would it be possible to expand the scope of this PR to update them, or should a follow-up issue be created to track this technical debt?
![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/static_refactored.go (1)
1927-1936: Theclusterfield rename is correct; notepathToItemsformat is a minority pattern.The change from
clusterNamePartOfUrltoclusteraligns with the established pattern infactory.go(e.g., vpc-subnets-table, resource-quotas-table).The
pathToItemschange from[]any{"items"}to".items"fixes the rendering bug, but uses a format that is less prevalent in the codebase:
- Array format
[]any{...}: 9 instances (7 in factory.go, 2 in static_refactored.go) — 69%- String format
"...": 4 instances (all in static_refactored.go) — 31%While both formats are functionally supported, the array format is the established convention, especially in factory.go where it is used consistently. Future updates should consider aligning to the array format for consistency across the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/dashboard/static_refactored.go` around lines 1927 - 1936, The change correctly renames clusterNamePartOfUrl to cluster and fixes rendering by switching pathToItems from []any{"items"} to the string ".items", but repository convention prefers the array format; update the pathToItems value back to the array form (e.g., []any{"items"}) in the static_refactored.go entry where pathToItems is currently ".items" so it matches the established pattern used in factory.go and the other table definitions while keeping the cluster field rename as-is.
🤖 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/static_refactored.go`:
- Around line 1927-1936: The change correctly renames clusterNamePartOfUrl to
cluster and fixes rendering by switching pathToItems from []any{"items"} to the
string ".items", but repository convention prefers the array format; update the
pathToItems value back to the array form (e.g., []any{"items"}) in the
static_refactored.go entry where pathToItems is currently ".items" so it matches
the established pattern used in factory.go and the other table definitions while
keeping the cluster field rename as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c68afc65-53bf-46de-a0d8-e5df3c9a096f
📒 Files selected for processing (1)
internal/controller/dashboard/static_refactored.go
| "cluster": "{2}", | ||
| "baseprefix": "/openapi-ui", | ||
| "customizationId": "factory-details-v1.services", | ||
| "pathToItems": ".items", |
There was a problem hiding this comment.
We use slice style for pathToItems everywhere
The external-ips factory used incorrect EnrichedTable properties causing empty rows in the dashboard. Replace `clusterNamePartOfUrl` with `cluster` and change `pathToItems` from array to dot-path string format to match the convention used by all other working EnrichedTable instances. Signed-off-by: IvanHunters <[email protected]>
kvaps
force-pushed
the
fix/external-ips-enriched-table
branch
from
4d2dd0b to
49601b1
Compare
kvaps
added
backport
|
Successfully created backport PR for |
|
Successfully created backport PR for |
3 participants
Summary
external-ipsfactoryclusterNamePartOfUrlwithclusterand changepathToItemsfrom array format to dot-path string to match convention used by all other EnrichedTable instancesTest plan
Summary by CodeRabbit