Conversation
…tion for Azure IPAM
…that were not being accounted for properly
…ue to improper handling of missing vNETs and vHUBs
…DataGrid component
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 128 out of 188 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
ui/src/features/analysis/visualize/visualize.jsx:567
Searchis no longer wrapped inReact.forwardRef, but it still callsuseImperativeHandle(ref, ...)and the parent uses<Search ref={searchRef} />. In React,refis not passed as a normal prop, sorefhere will beundefinedandsearchRef.currentwill staynull(breakinggetValue()/hasValue()calls and potentially crashing effects that dereference it). ConvertSearchback toReact.forwardRef((props, ref) => ...)(or rename to an explicit prop likeinnerRefand update callers) so the imperative API works.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 129 out of 189 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
ui/src/features/analysis/visualize/visualize.jsx:553
Searchis no longer wrapped inReact.forwardRef, but it still tries to receiverefvia props and usesuseImperativeHandle(ref, ...). In React,refis not passed as a normal prop to function components, sosearchRefwill never be wired up and the imperative API (getValue,hasValue, etc.) will be broken (and can also cause warnings like “Function components cannot be given refs”). ConvertSearchback toReact.forwardRef(acceptingrefas the 2nd arg) and removereffrom the props destructuring.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…-react + react-compiler and addressed new errors identified by ESLint v10
…ugins for the latest recommended ones
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 129 out of 189 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
ui/src/features/analysis/visualize/visualize.jsx:553
Searchis no longer wrapped inReact.forwardRef, but it still relies on arefargument (useImperativeHandle(ref, ...)). In React,ref={searchRef}is not passed as a normal prop, sorefwill beundefinedhere andsearchRef.currentwill never get the imperative API (breaking calls likesearchRef.current.hasValue()/getValue()). ConvertSearchback toReact.forwardRef((props, ref) => ...)(or rename to an explicit prop likeinnerRefand pass it manually).
ui/src/features/analysis/peering/peering.jsx:654Searchis defined as a normal component that destructures{ ref, ... }, but it's used as<Search ref={searchRef} />and callsuseImperativeHandle(ref, ...). React does not passrefthrough props unless the component usesReact.forwardRef, sosearchRef.current.setValue/clearSearch/...will break at runtime. RestoreReact.forwardRefforSearch(or pass a differently named prop likeinnerRef).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Update package version from ^2.13.0 to ^3.0.0 - Disable rules-of-hooks and exhaustive-deps (covered by react-compiler) - Downgrade component-hook-factories to warn (duplicates no-nested-component-definitions) - Disable set-state-in-effect (widespread pattern, to address incrementally)
- Bump @eslint-react/eslint-plugin from ^3.0.0 to ^4.0.0 - Rename rsc/function-definition to rsc-function-definition (v4 prefix flattening) - Move key before spread props in Autocomplete renderOption callbacks (11 sites)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 130 out of 190 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The bare `exit` in the `finally` block was overriding `exit 1` from the `catch` block, causing deployment failures to report exit code 0. This prevented GitHub Actions from detecting failed deployments.
…ngine - Add pyproject.toml with ruff config (E4, E7, E9, F, I, W291, W293, W605) - Replace bare except clauses with except Exception across 8 files - Replace star imports (from app.models import *) with explicit imports in 8 files - Remove unused imports and variables in main.py, space.py, dependencies.py - Fix invalid escape sequences with raw strings in space.py and argquery.py - Fix None comparisons, not-in operators, trailing semicolons - Rename duplicate function names (read_index, get_admins, get_block_reservations, delete_block_reservations) - Sort and organize all imports via isort across 14 files - Remove trailing whitespace in globals.py and status.py - Consolidate UUID imports in admin.py
- Replace ADD with COPY for local file operations (DL3020) - Convert CMD/ENTRYPOINT to JSON notation (DL3025) - Add SHELL pipefail directive before piped RUN commands (DL4006) - Add --no-cache-dir to pip upgrade commands (DL3042) - Add explicit WORKDIR in engine/Dockerfile.func (DL3045) - Add .hadolint.yaml for centralized rule suppressions
- Add lint job gating deploy with ESLint, Vite build, Ruff, Bicep, and Hadolint checks - Gate deploy job on lint success to avoid wasting Azure resources on bad code - Use hadolint-action for all Dockerfile variants (deb, rhel, func, dev, lb)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 147 out of 207 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | where isnotnull(tags["ipam-res-id"]) | ||
| | extend prefixes = properties.addressSpace.addressPrefixes | ||
| | project id, prefixes, resv = tags["ipam-res-id"] |
There was a problem hiding this comment.
The reservation settlement workflow has multiple sources in this PR (docs/examples) standardizing on the X-IPAM-RES-ID tag key, but the Resource Graph query still filters/projects tags["ipam-res-id"]. This will prevent reservations from being detected/settled if the deployed VNets are tagged with the new key. Update the query (and any downstream handling that depends on resv) to use X-IPAM-RES-ID, or support both keys (e.g., prefer X-IPAM-RES-ID but fall back to ipam-res-id) for backward compatibility.
| | where isnotnull(tags["ipam-res-id"]) | |
| | extend prefixes = properties.addressSpace.addressPrefixes | |
| | project id, prefixes, resv = tags["ipam-res-id"] | |
| | extend resv = coalesce(tostring(tags["X-IPAM-RES-ID"]), tostring(tags["ipam-res-id"])) | |
| | where isnotnull(resv) | |
| | extend prefixes = properties.addressSpace.addressPrefixes | |
| | project id, prefixes, resv |
| } else { | ||
| newOptions.title.show = true; | ||
| newOptions.legend.selected = Object.fromEntries( | ||
| newOptions.series.map(s => [s.name, false]) |
There was a problem hiding this comment.
In the else branch (no selection), legend.selected is being set to false for every series. In ECharts, legend.selected[name] = false hides that series, so this will likely render an empty chart when no search selection is active. For the “no selection” state, you probably want either (a) all series selected (true) or (b) to remove/reset legend.selected entirely so ECharts defaults apply.
| newOptions.series.map(s => [s.name, false]) | |
| newOptions.series.map(s => [s.name, true]) |
| disabled={ showSubnets ? (!selectedSubscription || !selectedNetwork || !selectedMask) : (!selectedSpace || !selectedBlock || !selectedMask) } | ||
| variant="contained" | ||
| loading={sending} | ||
| onClick={onSubmit} |
There was a problem hiding this comment.
loading={sending} was carried over from LoadingButton, but MUI Button does not consistently support a loading prop (depending on MUI version/component APIs), which can result in no spinner and/or an invalid DOM attribute warning. If you still need loading behavior, switch back to LoadingButton (lab) or implement loading UX explicitly (disable + show CircularProgress via startIcon/endIcon).
| disabled={ showSubnets ? (!selectedSubscription || !selectedNetwork || !selectedMask) : (!selectedSpace || !selectedBlock || !selectedMask) } | |
| variant="contained" | |
| loading={sending} | |
| onClick={onSubmit} | |
| disabled={ | |
| sending || | |
| (showSubnets | |
| ? (!selectedSubscription || !selectedNetwork || !selectedMask) | |
| : (!selectedSpace || !selectedBlock || !selectedMask)) | |
| } | |
| variant="contained" | |
| onClick={onSubmit} | |
| startIcon={sending ? <CircularProgress size={16} /> : null} |
| <Button onClick={onSubmit} loading={sending} disabled={!changed}> | ||
| Apply | ||
| </LoadingButton> | ||
| </Button> |
There was a problem hiding this comment.
Same issue as elsewhere: loading={sending} is being passed to Button. If Button doesn’t support loading in your MUI version, this will silently drop the loading UI and may leak an invalid attribute to the DOM. Prefer a loading-capable component (e.g., LoadingButton) or explicitly render a spinner while sending is true.
|
|
||
| # Execute Startup Script | ||
| ENTRYPOINT ./init.sh ${PORT} | ||
| ENTRYPOINT ["/bin/sh", "-c", "./init.sh ${PORT}"] |
There was a problem hiding this comment.
Using sh -c without exec can interfere with proper signal forwarding (SIGTERM/SIGINT) and graceful shutdown inside containers because PID 1 remains the shell rather than your process tree. Consider using exec ./init.sh ${PORT} in the shell command (as you already do in the repo root Dockerfiles) so the init script becomes PID 1 and receives signals reliably.
| ENTRYPOINT ["/bin/sh", "-c", "./init.sh ${PORT}"] | |
| ENTRYPOINT ["/bin/sh", "-c", "exec ./init.sh ${PORT}"] |
| </Provider> | ||
| </MsalProvider> | ||
| </React.StrictMode> | ||
| ); |
There was a problem hiding this comment.
msalInstance.initialize() is awaited via .then(...), but there’s no .catch(...) handler. If initialization fails (misconfig, storage issue, blocked third-party cookies, etc.), the app will never render and the failure may surface only as an unhandled promise rejection. Add a catch path that logs the error and renders a minimal fallback (or triggers a controlled redirect), so failures are diagnosable and don’t leave a blank page.
| ); | |
| ); | |
| }).catch((error) => { | |
| // Log initialization failures so they are diagnosable. | |
| // Without this, users may see only a blank page. | |
| console.error("MSAL initialization failed:", error); | |
| // Do not attempt to render UI inside MSAL's hidden iframe. | |
| if (isInHiddenIframe) { | |
| return; | |
| } | |
| // Render a minimal fallback so the user is not left with a blank screen. | |
| root.render( | |
| <React.StrictMode> | |
| <div style={{ padding: "1rem", fontFamily: "sans-serif" }}> | |
| <h1>Authentication initialization failed</h1> | |
| <p> | |
| We were unable to start the authentication service. Please refresh the | |
| page or try again later. If the problem persists, contact support. | |
| </p> | |
| </div> | |
| </React.StrictMode> | |
| ); |
Azure IPAM v4.0.0
This is a major release that delivers significant framework upgrades, a data grid migration, authentication modernization, comprehensive documentation overhaul, revamped examples, and numerous bug fixes.
Major Framework & Dependency Upgrades
forwardRefwrappers (ref-as-prop), removal ofPropTypes, explicitnullforuseRef()calls, and migration ofLoadingButtontoButton@azure/msal-browser4.x → 5.x,@azure/msal-react3.x → 5.x): Removed obsolete config, consolidated event types, fixed silent token timeout recovery (timed_outerror code), and prevented iframe fallback timeout loopsag-grid-community/ag-grid-react35.x): Complete migration to AG Grid including centralizedDataGridcomponent, custom styling, column state persistence, unified data loading overlays, and custom cell renderers (drill-down, info, progress)vite7.x → 8.x,@vitejs/plugin-react5.x → 6.x): Migrated to Vite 8 which replaces Rollup with Rolldown and esbuild with Oxc for bundling, transforms, and minification. Removedvite-plugin-eslint2(redundant with editor-based linting and incompatible with Vite 8)@eslint-react/eslint-plugin,eslint-plugin-react-compiler), replacingeslint-plugin-reactandeslint-plugin-react-hooks. Addeddist/ignore, fixedno-useless-assignmentviolations, and removed unusedeslint-plugin-jestEngine & Backend
msal,azure-common,azure-keyvault-secrets, andsix; addedazure-mgmt-resource-subscriptionsto address Azure SDK module separationpyproject.tomlwith Ruff linter configuration (pycodestyle, pyflakes, isort). Resolved all lint violations across 18 engine files — bare except clauses, wildcard imports replaced with explicit names, unused imports/variables, invalid escape sequences, import sorting and groupingUI & UX Improvements
AuthHandlerfor MSAL error handling, centralized token acquisition viatokenServiceDraggablePapercomponent: Replacedreact-draggablepackage with a purpose-built componentDataGridandConfigureGridcomponents with shared filter utilities, consistent loading overlays, and AG Grid custom styling (brightnessfilter for row hover)Deployment, Build & Infrastructure
ADD→COPY, JSON notation forCMD/ENTRYPOINT,pipefailfor pipedRUNcommands). Added centralized.hadolint.yamlfor rule suppressionsGet-AzAccessTokenbreaking changes (fixes Breaking changes to Get-AzAccessToken #343); updated deploy & migrate scriptsDocumentation Overhaul
.markdownlint.jsonconfigurationExamples
examples/scripts/folder with new helper scripts and READMETesting
Bug Fixes
Get-AzAccessTokenbreaking changes not accounted for[major]