First draft for schema visualizer using infrahub-schema-graph #8582
First draft for schema visualizer using infrahub-schema-graph #8582
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe changes introduce a new Git submodule for the schema-visualizer package under 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
github-actions
Bot
added
group/frontend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
.yamllint.yml (1)
14-14: LGTM!The schema-visualizer's package-lock.json entry shows it declares react in both devDependencies and peerDependencies, which justifies this approach.
Consider using
import.meta.dirname(available in Vite/ESM) for more robust path resolution that doesn't depend on the current working directory:♻️ Optional: Use import.meta.dirname for robustness
resolve: { alias: { // Ensure all packages use the same React instance - react: path.resolve("./node_modules/react"), - "react-dom": path.resolve("./node_modules/react-dom"), + react: path.resolve(import.meta.dirname, "node_modules/react"), + "react-dom": path.resolve(import.meta.dirname, "node_modules/react-dom"), }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.yamllint.yml at line 14, Replace any current working directory–dependent path resolution (e.g., usages of process.cwd() or ad-hoc __dirname workarounds) with ESM-aware resolution using import.meta (or helper conversion via fileURLToPath from 'url' + path.dirname) so paths are computed from the module location rather than the process CWD; locate the path-resolving logic (calls to path.resolve, process.cwd, or any custom __dirname shim) and update it to use import.meta.url/import.meta.dirname to compute module-relative paths for the schema-visualizer package to make resolution robust in Vite/ESM..github/workflows/ci-docker-image.yml (1)
56-60: Standardize submodule checkout strategy across workflows.Two workflows (ci-docker-image.yml and update-sdk-submodule.yml) use
submodules: recursive, while the majority of workflows usesubmodules: true. This inconsistency should be resolved for maintainability. Currently both approaches are functionally equivalent since .gitmodules declares only top-level submodules with no nesting, but standardizing on a single approach across all workflows would improve clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-docker-image.yml around lines 56 - 60, The workflow uses actions/checkout@v6 with submodules: recursive which is inconsistent with most other workflows; update the Checkout step (actions/checkout@v6) to use submodules: true instead of submodules: recursive so the submodule checkout strategy is standardized across workflows.frontend/app/package.json (1)
11-11: Keepbuildside-effect free.Running
npm installinsidebuildmakesnpm run buildnon-deterministic and slower in CI/Docker. Prefer keepingbuildas just the compile step (vite build) and doing dependency setup insetup/CI bootstrap steps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/package.json` at line 11, The package.json "build" script currently runs npm install as a side-effect ("build" script name) which makes builds non-deterministic; remove the npm install invocation from the "build" script so "build" only runs the compile step (vite build), and move dependency installation into a separate script (e.g., "setup" or CI bootstrap) that runs npm install --prefix ../packages/schema-visualizer; update references to the "build" script so CI/docker uses the new "setup" script before invoking the "build" script.frontend/app/vite.config.ts (1)
2-3: Good approach to deduplicate React instances across packages.The schema-visualizer package declares
reactin bothdevDependenciesandpeerDependencies, which can cause npm to install a separate React copy during hoisting. The alias correctly forces all React imports to resolve to the app-levelnode_modules.One minor robustness consideration:
path.resolve("./node_modules/react")resolves relative to the current working directory rather than the config file's location. While this typically works since Vite runs from the project root, usingimport.meta.dirname(Node.js 20.11+) would be more explicit:♻️ Optional: More explicit path resolution
- react: path.resolve("./node_modules/react"), - "react-dom": path.resolve("./node_modules/react-dom"), + react: path.resolve(import.meta.dirname, "node_modules/react"), + "react-dom": path.resolve(import.meta.dirname, "node_modules/react-dom"),Also applies to: 49-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/vite.config.ts` around lines 2 - 3, The current alias uses path.resolve("./node_modules/react") which is cwd-dependent; update the resolution in vite.config.ts to compute the react path relative to the config file (e.g., use import.meta.dirname or resolve via new URL('./node_modules/react', import.meta.url)) instead of a plain "./node_modules" string so the alias always points at the app-level React; change the call sites that use path.resolve("./node_modules/react") to use import.meta.dirname (or new URL(..., import.meta.url)) when building the alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/AGENTS.md`:
- Around line 12-13: The README line for the setup step is misleading: update
the description for "npm run setup" to state it initializes git submodules and
installs the schema-visualizer (submodule) dependencies only, and clarify that
application dependencies are installed by the following "cd frontend/app && npm
install" command; reference the two commands "npm run setup" and "npm install"
in the updated sentence so readers understand which step does which.
In `@frontend/app/src/entities/schema/ui/schema-view-toggle.tsx`:
- Around line 8-19: Add a unit test file named schema-view-toggle.test.tsx next
to SchemaViewToggle that renders the SchemaViewToggle component and asserts both
buttons render and the active state is applied based on route matching; mock
React Router's useMatch to simulate matched and unmatched routes and verify
LinkToggleButton and LinkToggleButtonGroup behave correctly (e.g.,
SchemaViewToggle renders "List" and "Graph" buttons and when useMatch returns a
match for "/schema" the List button is active, and when it matches
"/schema-graph" the Graph button is active).
In `@frontend/app/src/shared/components/buttons/link-toggle-button.tsx`:
- Around line 5-12: The component's active-state matching must always receive a
string path: update LinkToggleButtonProps and the LinkToggleButton parameter
typing so that the inherited LinkProps 'to' is constrained to string (e.g.,
declare LinkToggleButtonProps extends Omit<LinkProps, "to"> & { to: string;
matchPath?: string }) and then use matchPath ?? to for the path passed into
useMatch; ensure all references to 'to' in LinkToggleButton and any callers are
compatible with the new string type so useMatch(path) no longer gets a fallback
empty string or an object.
In `@frontend/app/src/shared/components/layout/content.tsx`:
- Line 81: The title container div with className "flex flex-1 flex-col gap-0.5
overflow-hidden" is missing the Tailwind utility to allow truncation; add
"min-w-0" to that class list (i.e., update the div in content.tsx that renders
the title container) so long titles can shrink and preserve text truncation
behavior.
---
Nitpick comments:
In @.github/workflows/ci-docker-image.yml:
- Around line 56-60: The workflow uses actions/checkout@v6 with submodules:
recursive which is inconsistent with most other workflows; update the Checkout
step (actions/checkout@v6) to use submodules: true instead of submodules:
recursive so the submodule checkout strategy is standardized across workflows.
In @.yamllint.yml:
- Line 14: Replace any current working directory–dependent path resolution
(e.g., usages of process.cwd() or ad-hoc __dirname workarounds) with ESM-aware
resolution using import.meta (or helper conversion via fileURLToPath from 'url'
+ path.dirname) so paths are computed from the module location rather than the
process CWD; locate the path-resolving logic (calls to path.resolve,
process.cwd, or any custom __dirname shim) and update it to use
import.meta.url/import.meta.dirname to compute module-relative paths for the
schema-visualizer package to make resolution robust in Vite/ESM.
In `@frontend/app/package.json`:
- Line 11: The package.json "build" script currently runs npm install as a
side-effect ("build" script name) which makes builds non-deterministic; remove
the npm install invocation from the "build" script so "build" only runs the
compile step (vite build), and move dependency installation into a separate
script (e.g., "setup" or CI bootstrap) that runs npm install --prefix
../packages/schema-visualizer; update references to the "build" script so
CI/docker uses the new "setup" script before invoking the "build" script.
In `@frontend/app/vite.config.ts`:
- Around line 2-3: The current alias uses path.resolve("./node_modules/react")
which is cwd-dependent; update the resolution in vite.config.ts to compute the
react path relative to the config file (e.g., use import.meta.dirname or resolve
via new URL('./node_modules/react', import.meta.url)) instead of a plain
"./node_modules" string so the alias always points at the app-level React;
change the call sites that use path.resolve("./node_modules/react") to use
import.meta.dirname (or new URL(..., import.meta.url)) when building the alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa4693d1-0edd-44ba-8ab4-7b76a84ed15f
⛔ Files ignored due to path filters (1)
frontend/app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
.github/workflows/ci-docker-image.yml.github/workflows/ci.yml.gitmodules.yamllint.ymldevelopment/Dockerfilefrontend/app/AGENTS.mdfrontend/app/knip.config.tsfrontend/app/package.jsonfrontend/app/src/app/router.tsxfrontend/app/src/entities/schema/ui/schema-view-toggle.tsxfrontend/app/src/pages/schema-graph.tsxfrontend/app/src/pages/schema.tsxfrontend/app/src/shared/components/buttons/link-toggle-button.tsxfrontend/app/src/shared/components/layout/content.tsxfrontend/app/tailwind.config.jsfrontend/app/vite.config.tsfrontend/packages/schema-visualizer
| cd frontend/app && npm run setup # Init submodules + install all dependencies (run first) | ||
| cd frontend/app && npm install # Install app dependencies only (submodule must already be initialized) |
There was a problem hiding this comment.
setup description is currently inaccurate.
npm run setup initializes submodules and installs schema-visualizer deps, but app deps are installed by the next command. The “install all dependencies” wording on Line 12 is misleading.
Suggested doc tweak
-cd frontend/app && npm run setup # Init submodules + install all dependencies (run first)
+cd frontend/app && npm run setup # Init submodules + install schema-visualizer dependencies (run first)
cd frontend/app && npm install # Install app dependencies only (submodule must already be initialized)📝 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.
| cd frontend/app && npm run setup # Init submodules + install all dependencies (run first) | |
| cd frontend/app && npm install # Install app dependencies only (submodule must already be initialized) | |
| cd frontend/app && npm run setup # Init submodules + install schema-visualizer dependencies (run first) | |
| cd frontend/app && npm install # Install app dependencies only (submodule must already be initialized) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/AGENTS.md` around lines 12 - 13, The README line for the setup
step is misleading: update the description for "npm run setup" to state it
initializes git submodules and installs the schema-visualizer (submodule)
dependencies only, and clarify that application dependencies are installed by
the following "cd frontend/app && npm install" command; reference the two
commands "npm run setup" and "npm install" in the updated sentence so readers
understand which step does which.
| export const SchemaViewToggle = () => ( | ||
| <LinkToggleButtonGroup> | ||
| <LinkToggleButton to="/schema"> | ||
| <Icon icon="mdi:format-list-bulleted" /> | ||
| List | ||
| </LinkToggleButton> | ||
| <LinkToggleButton to="/schema-graph"> | ||
| <Icon icon="mdi:graph" /> | ||
| Graph | ||
| </LinkToggleButton> | ||
| </LinkToggleButtonGroup> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether tests already cover new schema toggle/route behavior.
rg -n --glob '*{test,spec}*.{ts,tsx}' 'SchemaViewToggle|LinkToggleButton|/schema-graph'Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Check the component file exists and find test files
find frontend/app/src/entities/schema/ui -type f -name '*.tsx' -o -name '*.ts' | head -20Repository: opsmill/infrahub
Length of output: 1073
🏁 Script executed:
# Search for test files in schema directory with various naming patterns
find frontend/app/src/entities/schema -type f \( -name '*.test.ts' -o -name '*.test.tsx' -o -name '*.spec.ts' -o -name '*.spec.tsx' \)Repository: opsmill/infrahub
Length of output: 1065
🏁 Script executed:
# Check the overall test structure in the frontend directory
find frontend/app/src -type d -name '__tests__' -o -type d -name 'tests' | head -10Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Look for schema-view-toggle test file with any pattern
find . -type f \( -name '*schema-view-toggle*test*' -o -name '*schema-view-toggle*spec*' \) 2>/dev/nullRepository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Find LinkToggleButton tests
find frontend/app -type f \( -name '*link-toggle-button*test*' -o -name '*link-toggle-button*spec*' \) 2>/dev/nullRepository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Search for LinkToggleButton definition and its tests
rg -l 'LinkToggleButton' --glob '*.tsx' --glob '*.ts' | head -20Repository: opsmill/infrahub
Length of output: 184
🏁 Script executed:
# Check if schema-view-toggle.tsx is a new file (check git status)
git log --oneline --follow frontend/app/src/entities/schema/ui/schema-view-toggle.tsx 2>/dev/null | head -5Repository: opsmill/infrahub
Length of output: 120
🏁 Script executed:
# Search for test files for LinkToggleButton
find frontend/app -type f -path '*/buttons/*' \( -name '*.test.tsx' -o -name '*.spec.tsx' \)Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Check the LinkToggleButton file to understand its structure
head -50 frontend/app/src/shared/components/buttons/link-toggle-button.tsxRepository: opsmill/infrahub
Length of output: 1225
🏁 Script executed:
# Check if there are any tests for link-toggle-button
rg -l 'LinkToggleButton' --glob '*.test.tsx' --glob '*.spec.tsx'Repository: opsmill/infrahub
Length of output: 42
Add tests for SchemaViewToggle component. Tests are required for this new UI component per the frontend coding guidelines. Place the test file as schema-view-toggle.test.tsx alongside the component and cover the toggle rendering and active-state behavior (using useMatch to determine which route is active).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/src/entities/schema/ui/schema-view-toggle.tsx` around lines 8 -
19, Add a unit test file named schema-view-toggle.test.tsx next to
SchemaViewToggle that renders the SchemaViewToggle component and asserts both
buttons render and the active state is applied based on route matching; mock
React Router's useMatch to simulate matched and unmatched routes and verify
LinkToggleButton and LinkToggleButtonGroup behave correctly (e.g.,
SchemaViewToggle renders "List" and "Graph" buttons and when useMatch returns a
match for "/schema" the List button is active, and when it matches
"/schema-graph" the Graph button is active).
| interface LinkToggleButtonProps extends LinkProps { | ||
| matchPath?: string; | ||
| } | ||
|
|
||
| export const LinkToggleButton = ({ className, to, matchPath, ...props }: LinkToggleButtonProps) => { | ||
| const path = matchPath ?? (typeof to === "string" ? to : ""); | ||
| const match = useMatch(path); | ||
| const isActive = !!match; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In react-router v7, is useMatch("") a valid pattern, and what matching behavior should be expected for an empty string pattern?
💡 Result:
Yes. In React Router v7, useMatch accepts any string as a path pattern (it’s passed straight to matchPath), so useMatch("") is syntactically valid. useMatch treats a string pattern as { path: pattern, caseSensitive: false, end: true } (i.e. an “exact” match). [1], [2]
Expected behavior for an empty-string pattern (""):
- It behaves like a zero-segment path (the same kind of value the router uses internally for “no additional path” in nested/index-route situations). The router code explicitly handles
route.path === ""as a meaningful case during route flattening. [3] - With
end: true,""should match only when the currentlocation.pathnameis effectively the root with no extra segments—i.e."/"in typical browser usage—souseMatch("")should return a match object at"/"andnullat"/anything". [1], [2]
Practical takeaway: useMatch("") is not a “match everything” pattern; it’s closer to “match the empty remainder / root only”. Use useMatch("/") if you mean the root path explicitly.
Sources
[1] React Router v7 matchPath docs (string patterns default to end: true): https://reactrouter.com/api/utils/matchPath
[2] React Router v7 matchPath implementation (string pattern -> { path, caseSensitive: false, end: true }): https://github.com/remix-run/react-router/blob/a3e4b8ed875611637357647fcf862c2bc61f4e11/packages/react-router/lib/router/utils.ts
[3] React Router v7 route flattening treats route.path === "" specially: https://github.com/remix-run/react-router/blob/a3e4b8ed875611637357647fcf862c2bc61f4e11/packages/react-router/lib/router/utils.ts
Constrain to to string to ensure reliable active-state matching.
Falling back to "" when to is object-shaped creates unreliable matching. In React Router v7, useMatch("") matches only the root path with end: true, not a general fallback pattern. Constrain to to string so the component always has a valid path for matching.
Suggested fix
interface LinkToggleButtonProps extends LinkProps {
+ to: string;
matchPath?: string;
}
export const LinkToggleButton = ({ className, to, matchPath, ...props }: LinkToggleButtonProps) => {
- const path = matchPath ?? (typeof to === "string" ? to : "");
+ const path = matchPath ?? to;📝 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.
| interface LinkToggleButtonProps extends LinkProps { | |
| matchPath?: string; | |
| } | |
| export const LinkToggleButton = ({ className, to, matchPath, ...props }: LinkToggleButtonProps) => { | |
| const path = matchPath ?? (typeof to === "string" ? to : ""); | |
| const match = useMatch(path); | |
| const isActive = !!match; | |
| interface LinkToggleButtonProps extends LinkProps { | |
| to: string; | |
| matchPath?: string; | |
| } | |
| export const LinkToggleButton = ({ className, to, matchPath, ...props }: LinkToggleButtonProps) => { | |
| const path = matchPath ?? to; | |
| const match = useMatch(path); | |
| const isActive = !!match; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/src/shared/components/buttons/link-toggle-button.tsx` around
lines 5 - 12, The component's active-state matching must always receive a string
path: update LinkToggleButtonProps and the LinkToggleButton parameter typing so
that the inherited LinkProps 'to' is constrained to string (e.g., declare
LinkToggleButtonProps extends Omit<LinkProps, "to"> & { to: string; matchPath?:
string }) and then use matchPath ?? to for the path passed into useMatch; ensure
all references to 'to' in LinkToggleButton and any callers are compatible with
the new string type so useMatch(path) no longer gets a fallback empty string or
an object.
| className={classNames("flex items-center border-gray-200 border-b p-5", className)} | ||
| {...props} | ||
| > | ||
| <div className="flex flex-1 flex-col gap-0.5 overflow-hidden"> |
There was a problem hiding this comment.
Add min-w-0 to preserve truncation behavior.
With flex-1, long titles may stop truncating correctly and can push the end control. Add min-w-0 to the title container.
Suggested fix
- <div className="flex flex-1 flex-col gap-0.5 overflow-hidden">
+ <div className="flex min-w-0 flex-1 flex-col gap-0.5 overflow-hidden">📝 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.
| <div className="flex flex-1 flex-col gap-0.5 overflow-hidden"> | |
| <div className="flex min-w-0 flex-1 flex-col gap-0.5 overflow-hidden"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/src/shared/components/layout/content.tsx` at line 81, The title
container div with className "flex flex-1 flex-col gap-0.5 overflow-hidden" is
missing the Tailwind utility to allow truncation; add "min-w-0" to that class
list (i.e., update the div in content.tsx that renders the title container) so
long titles can shrink and preserve text truncation behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/src/pages/schema/graph.tsx`:
- Around line 11-28: Add Vitest unit tests for the new schema pages to cover
route rendering, toggle behavior, and Jotai integration: create tests next to
the components (frontend/app/src/pages/schema/graph.test.tsx and list.test.tsx
or a combined test) that render the route-aware component hierarchy (rendering
the page layout that includes SchemaGraph, SchemaList and the layout component)
using Testing Library + Vitest, assert that navigating to "/schema" shows the
list view and "/schema/graph" shows the graph view, simulate clicking the toggle
button to switch between SchemaList and SchemaGraph, and provide mock atom state
for nodeSchemasAtom, genericSchemasAtom, profileSchemasAtom, templateSchemasAtom
by wrapping the render with a Jotai Provider/initial atom values so the
components receive predictable data; reference SchemaGraph, SchemaList,
layout.tsx, and the atom names in your tests for locating the targets.
In `@frontend/app/src/pages/schema/layout.tsx`:
- Around line 27-31: The schema badge currently computes badgeContent as
nodes.length + generics.length + profiles.length and omits template schemas;
update the badgeContent expression in layout.tsx to add templates.length (i.e.,
nodes.length + generics.length + profiles.length + templates.length) so the
badge matches the list/graph counts, and ensure the templates variable is in
scope where the badgeContent prop is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a8ec006-623c-420d-b305-533f2b0a6ed5
📒 Files selected for processing (4)
frontend/app/src/app/router.tsxfrontend/app/src/pages/schema/graph.tsxfrontend/app/src/pages/schema/layout.tsxfrontend/app/src/pages/schema/list.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/src/app/router.tsx
3 participants
Summary by CodeRabbit
New Features
Refactor
Chores