Conversation
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Ivan Stukov <stukov.ivan.time@gmail.com>
✅ Deploy Preview for cozystack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @IvanStukov, 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 focuses on enhancing the user experience and utility of the resource planning documentation. By simplifying the content, providing clearer guidance, and introducing an interactive tool, the aim is to make it easier for users to accurately estimate system resource allocations for their clusters. 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
|
📝 WalkthroughWalkthroughRewrote resource-planning docs (v0 and v1) to replace fixed recommendations with a calculator-driven planning model and added a client-side Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@content/en/docs/v0/install/resource-planning.md`:
- Around line 24-28: The cluster node ranges in the resource-planning table
overlap at 10 nodes between the "**Medium**" and "**Large**" rows; update the
ranges to be mutually exclusive (for example change the "**Medium**" Nodes cell
from "6-10" to "6-9" or change the "**Large**" Nodes cell from "10+" to "11+"),
and ensure any dependent CPU/RAM guidance remains aligned with the adjusted
range in the "**Medium**" and "**Large**" rows.
In `@content/en/docs/v1/install/resource-planning.md`:
- Around line 22-26: The node-range in the resource planning table overlaps
because the "Medium" row currently lists "6-10" nodes while the "Large" row
lists "10+"; update the ranges to be mutually exclusive by changing either the
Medium range to "6–9" or the Large range to "11+" (or switch to "10+"/ "6–10"
and "11+" accordingly) and ensure both the "Medium" and "Large" rows in the
table are updated consistently so the boundary value 10 is not present in both
rows.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the resource planning documentation by refactoring it for clarity and conciseness. The changes include restructuring the content, clarifying recommendations, fixing an inconsistency in tenant categories, and adding an interactive calculator which is a great enhancement. I've added a couple of minor suggestions to further improve the readability of the resource table by merging redundant columns. Overall, this is an excellent update to the documentation.
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Ivan Stukov <stukov.ivan.time@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@layouts/shortcodes/system-resource-calculator.html`:
- Line 157: The inline comment next to the "large" mapping is incorrect: update
the comment above the mapping in the system-resource-calculator shortcode so it
matches getClusterSize's logic (which treats 11+ nodes as large). Locate the
"large:" mapping and change its comment from "// 10+ nodes" to "// 11+ nodes" so
it aligns with the getClusterSize function and the markdown tables.
- Around line 35-138: The stylesheet hardcodes light-theme colors making
.calculator-card, .result-item, .label-text, .label-value, .result-value and
range thumb look wrong in dark mode; replace literal colors with theme CSS
custom properties (e.g., --bg, --card-bg, --text, --muted, --accent, --success)
and use those variables in the rules, providing sensible fallbacks (e.g.,
var(--card-bg, `#f8f9fa`)); additionally add a prefers-color-scheme: dark media
query to override the variables (or set dark-specific variables) so the
calculator adapts when the site or system uses a dark theme.
- Around line 6-10: The shortcode uses hardcoded IDs (e.g., nodes-slider,
nodes-value, tenants-value, cpu-result) which will collide when the shortcode is
rendered multiple times; update the HTML to replace those id attributes with
class names and change the JS that references getElementById(...) to use
relative DOM queries scoped to the calculator root (e.g., find the calculator
container element and use root.querySelector('.nodes-slider') /
root.querySelector('.nodes-value') etc.), or append a unique suffix/token to
each ID when the shortcode is instantiated; ensure all references across the
file (the slider, value displays, and result elements mentioned in lines around
14-18, 22-30, 184-202) are updated to the new scoped selectors so each
calculator instance binds only to its own elements.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@layouts/shortcodes/system-resource-calculator.html`: - Around line 35-138: The stylesheet hardcodes light-theme colors making .calculator-card, .result-item, .label-text, .label-value, .result-value and range thumb look wrong in dark mode; replace literal colors with theme CSS custom properties (e.g., --bg, --card-bg, --text, --muted, --accent, --success) and use those variables in the rules, providing sensible fallbacks (e.g., var(--card-bg, `#f8f9fa`)); additionally add a prefers-color-scheme: dark media query to override the variables (or set dark-specific variables) so the calculator adapts when the site or system uses a dark theme. - Around line 6-10: The shortcode uses hardcoded IDs (e.g., nodes-slider, nodes-value, tenants-value, cpu-result) which will collide when the shortcode is rendered multiple times; update the HTML to replace those id attributes with class names and change the JS that references getElementById(...) to use relative DOM queries scoped to the calculator root (e.g., find the calculator container element and use root.querySelector('.nodes-slider') / root.querySelector('.nodes-value') etc.), or append a unique suffix/token to each ID when the shortcode is instantiated; ensure all references across the file (the slider, value displays, and result elements mentioned in lines around 14-18, 22-30, 184-202) are updated to the new scoped selectors so each calculator instance binds only to its own elements.layouts/shortcodes/system-resource-calculator.html (2)
35-138: Hardcoded light-theme colors may render poorly in dark mode.All colors (
#f8f9fa,white,#495057, etc.) are hardcoded. If the site supports a dark theme, this calculator will appear as a bright box with potentially unreadable text. Consider using CSS custom properties from the site's theme or adding aprefers-color-scheme: darkmedia query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@layouts/shortcodes/system-resource-calculator.html` around lines 35 - 138, The stylesheet hardcodes light-theme colors making .calculator-card, .result-item, .label-text, .label-value, .result-value and range thumb look wrong in dark mode; replace literal colors with theme CSS custom properties (e.g., --bg, --card-bg, --text, --muted, --accent, --success) and use those variables in the rules, providing sensible fallbacks (e.g., var(--card-bg, `#f8f9fa`)); additionally add a prefers-color-scheme: dark media query to override the variables (or set dark-specific variables) so the calculator adapts when the site or system uses a dark theme.
6-10: Hardcoded element IDs will break if this shortcode is used more than once on a page.All DOM queries use fixed IDs like
nodes-slider,tenants-value,cpu-result, etc. If the shortcode is ever included twice on the same page, the second instance will silently bind to the first instance's elements.Scope all IDs with a unique suffix (e.g., a counter or random token) or switch to relative DOM queries scoped to the calculator's root element.
Suggested approach using relative DOM queries
<script> (function() { + // Find the script's own calculator root + const root = document.currentScript.previousElementSibling.closest + ? document.currentScript.closest('.resource-calculator') + : document.querySelectorAll('.resource-calculator'); + // A more reliable approach: grab the last .resource-calculator in the DOM at parse time + const calculators = document.querySelectorAll('.resource-calculator'); + const root = calculators[calculators.length - 1]; + // ... function updateResults() { - const nodes = parseInt(document.getElementById('nodes-slider').value); - const tenants = parseInt(document.getElementById('tenants-slider').value); - document.getElementById('nodes-value').textContent = nodes; + const nodes = parseInt(root.querySelector('.nodes-slider').value); + const tenants = parseInt(root.querySelector('.tenants-slider').value); + root.querySelector('.nodes-value').textContent = nodes; // ... (similarly for all other getElementById calls) } - document.getElementById('nodes-slider').addEventListener('input', updateResults); - document.getElementById('tenants-slider').addEventListener('input', updateResults); + root.querySelector('.nodes-slider').addEventListener('input', updateResults); + root.querySelector('.tenants-slider').addEventListener('input', updateResults);(and convert the
idattributes in the HTML toclassattributes accordingly)Also applies to: 14-18, 22-30, 184-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@layouts/shortcodes/system-resource-calculator.html` around lines 6 - 10, The shortcode uses hardcoded IDs (e.g., nodes-slider, nodes-value, tenants-value, cpu-result) which will collide when the shortcode is rendered multiple times; update the HTML to replace those id attributes with class names and change the JS that references getElementById(...) to use relative DOM queries scoped to the calculator root (e.g., find the calculator container element and use root.querySelector('.nodes-slider') / root.querySelector('.nodes-value') etc.), or append a unique suffix/token to each ID when the shortcode is instantiated; ensure all references across the file (the slider, value displays, and result elements mentioned in lines around 14-18, 22-30, 184-202) are updated to the new scoped selectors so each calculator instance binds only to its own elements.
| '15-30': { cpu: 3, ram: 12 }, | ||
| '30+': { cpu: 4, ram: 18 } | ||
| }, | ||
| large: { // 10+ nodes |
There was a problem hiding this comment.
Comment says "10+" but the logic maps 11+ to large.
The getClusterSize function (line 167) returns 'medium' for nodes 6–10 and 'large' for 11+. The comment on this line should say // 11+ nodes to match the code and the markdown tables.
- large: { // 10+ nodes
+ large: { // 11+ nodes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| large: { // 10+ nodes | |
| large: { // 11+ nodes |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@layouts/shortcodes/system-resource-calculator.html` at line 157, The inline
comment next to the "large" mapping is incorrect: update the comment above the
mapping in the system-resource-calculator shortcode so it matches
getClusterSize's logic (which treats 11+ nodes as large). Locate the "large:"
mapping and change its comment from "// 10+ nodes" to "// 11+ nodes" so it
aligns with the getClusterSize function and the markdown tables.
2 participants
Summary
Summary by CodeRabbit