feat: Optimize type count #1018
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new Rust crate for typegraph optimization, integrating it into the workspace and updating dependencies to use Changes
Sequence Diagram(s)Typegraph Optimization FlowsequenceDiagram
participant User
participant CLI as optimize/main.rs
participant Optimizer as tg_optimize::optimize
participant Kosaraju as Kosaraju
participant Dedup as DedupEngine
User->>CLI: Provide JSON typegraph input
CLI->>Optimizer: Call optimize(Arc<Typegraph>)
Optimizer->>Kosaraju: Find strongly connected components
Kosaraju-->>Optimizer: Return SCCs
Optimizer->>Dedup: Deduplicate types using SCCs
Dedup-->>Optimizer: Return optimized Typegraph
Optimizer-->>CLI: Return optimized Typegraph
CLI->>User: Output optimized JSON typegraph
Frontend Typegraph Explorer (Simplified)sequenceDiagram
participant Browser
participant OakServer as Oak Server
participant ReactApp as React App
participant API as /api endpoints
Browser->>OakServer: GET /
OakServer->>Browser: Serve index.html and assets
ReactApp->>API: GET /api/typegraphs
API-->>ReactApp: List of typegraph names
ReactApp->>API: GET /api/typegraphs/:tgName
API-->>ReactApp: Typegraph stats
ReactApp->>API: GET /api/typegraphs/:tgName/types/:typeIdx
API-->>ReactApp: Type node data
ReactApp->>Browser: Render UI with typegraph explorer
Type Deduplication (Rust Optimization Crate)sequenceDiagram
participant Typegraph
participant Kosaraju
participant DedupEngine
Typegraph->>Kosaraju: Provide type nodes for SCC analysis
Kosaraju-->>DedupEngine: SCCs and root/component mapping
DedupEngine->>DedupEngine: Hash and bucket types by structure
DedupEngine->>DedupEngine: Build conversion map for deduplication
DedupEngine->>Typegraph: Produce deduplicated typegraph with updated references
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
tools/tree-view-web.ts (1)
1-1:⚠️ Potential issueInvalid shebang prevents the script from running directly
ghjksneaked into the interpreter path, so the OS cannot locate the executable. Anyone invoking the file as a CLI will get “No such file or directory”.-#!/bin/env -S ghjk deno run -A +#!/usr/bin/env -S deno run -A
🧹 Nitpick comments (21)
tools/tree/.vite/deps/_metadata.json (1)
1-8: Consider adding this file to .gitignoreThis appears to be an auto-generated Vite dependency cache file. Auto-generated build artifacts are typically excluded from version control to avoid unnecessary churn and potential conflicts.
If this is an intentional addition for specific reasons, consider documenting why this auto-generated file needs to be in version control.
tools/tree/src/main.tsx (2)
9-13: Add error handling for missing root elementThe non-null assertion operator (
!) assumes the 'root' element will always exist. Consider adding a fallback or error handling for cases where the element might be missing.-createRoot(document.getElementById('root')!).render( +const rootElement = document.getElementById('root'); +if (!rootElement) { + console.error('Root element not found'); + throw new Error('Root element not found'); +} +createRoot(rootElement).render(
4-7: Consider using absolute importsFor better maintainability as the project grows, consider configuring and using absolute imports instead of relative ones.
-import './index.css' -import App from './App.tsx' +import 'src/index.css' +import App from 'src/App'This would require configuring path aliases in your tsconfig.json:
{ "compilerOptions": { "baseUrl": ".", "paths": { "src/*": ["src/*"] } } }tools/tree/index.html (1)
1-8: Enhance metadata and accessibility
Consider adding descriptive metadata—such as a<meta name="description">, theme color, and any necessary favicons or manifest links—and update the<title>to reflect the actual application (e.g., “Typegraph Tree View”) instead of the default “Vite + React + TS”.tools/tree/eslint.config.js (1)
7-15: Consider specifying the parser explicitly
To ensure full TypeScript support, add:languageOptions: { parser: '@typescript-eslint/parser', parserOptions: { project: ['./tsconfig.app.json', './tsconfig.node.json'] }, ecmaVersion: 2020, globals: globals.browser, },This enables type-aware rules and proper AST parsing.
tools/tree/README.md (2)
1-4: Add setup and installation steps
The README describes the template but omits prerequisites (e.g., Node.js version) and the commands to install dependencies (npm install/yarn install) and launch the dev server (npm run dev). Consider adding a “Getting Started” section with these instructions.
12-31: Clarify and fence code examples
The ESLint configuration snippets are useful, but ensure each code block is correctly fenced (opening and closing```) and include the filename or context as a comment header. This improves readability and copy-pasta accuracy.tools/tree/tsconfig.app.json (2)
3-8: Consider enabling source maps for development
Adding"sourceMap": true(and optionally"declarationMap": true) undercompilerOptionswill greatly aid debugging, especially when troubleshooting React component code in Vite.
1-25: ReviewtsBuildInfoFilelocation
Writing the build info tonode_modules/.tmprisks it being removed during installations. Consider relocating it to a dedicated cache folder (for example,./.cache/tsconfig.app.tsbuildinfo) to avoid pollutingnode_modules.tools/tree/src/components/Type.tsx (3)
8-8: Consider using a more maintainable import path.The deep relative import path makes the code brittle and harder to maintain.
-import type { TypeNode } from "../../../../src/typegate/src/typegraph/types"; +import type { TypeNode } from "@metatype/typegraph-types"; // Or another aliased import
35-50: Add accessibility attributes to the expand button.The button lacks proper accessibility attributes which may impact users with assistive technologies.
<button onClick={onClick} disabled={disabled} + aria-expanded={expanded} + aria-label={expanded ? "Collapse" : "Expand"} className={clsx( "w-4 h-4 flex items-center justify-center transition-colors outline-0 border-1 rounded-full flex-shrink-0", disabled ? "text-gray-600 border-gray-600 cursor-default" : "text-gray-400 hover:text-white cursor-pointer border-gray-400" )} >
145-147: Potential performance issue with recursive rendering.When expanding a large typegraph, rendering all child types at once could cause performance issues. Consider implementing virtualization or pagination for large lists.
Consider using a virtualized list library like
react-windoworreact-virtualizedto render only visible nodes:import { FixedSizeList } from 'react-window'; // Inside the component: {expanded && edges.length > 0 && ( <FixedSizeList height={Math.min(400, edges.length * 30)} width="100%" itemCount={edges.length} itemSize={30} > {({ index, style }) => { const [tag, typeIdx] = edges[index]; return ( <div style={style}> <Type key={`${typeIdx}-${tag}`} idx={typeIdx} tag={tag} /> </div> ); }} </FixedSizeList> )}tools/tree/src/App.tsx (3)
98-117: Unnecessary conditional check.The code checks for
typegraphDatainside the JSX even though it has already verified it exists earlier in the function.return ( <section className="flex items-center gap-4 px-4 py-1.5 bg-gray-900 text-sm"> - {typegraphData && ( - <> <div className="flex items-center gap-2"> <span className="text-gray-400">Type count:</span> <span className="text-white">{typegraphData.stats.types}</span> </div> <div className="flex items-center gap-2"> <span className="text-gray-400">Runtimes:</span> <span className="text-white">{typegraphData.stats.runtimes}</span> </div> <div className="flex items-center gap-2"> <span className="text-gray-400">Materializers:</span> <span className="text-white">{typegraphData.stats.materializers}</span> </div> <div className="flex items-center gap-2"> <span className="text-gray-400">Policies:</span> <span className="text-white">{typegraphData.stats.policies}</span> </div> - </> - )} </section> );
155-158: Consider adding retry logic for failed requests.The SWR configuration doesn't include retry logic, which could improve resilience against temporary network issues.
<SWRConfig value={{ fetcher, revalidateOnFocus: false, + onErrorRetry: (error, key, config, revalidate, { retryCount }) => { + // Only retry up to 3 times + if (retryCount >= 3) return; + // Retry after 3 seconds + setTimeout(() => revalidate({ retryCount }), 3000); + }, }}>
57-64: Enhance the Stats type with more robust typing.The Stats type definition is minimal and could benefit from more comprehensive typing.
-type Stats = { - stats: { - types: number; - runtimes: number; - materializers: number; - policies: number; - } -} +interface TypegraphStats { + types: number; + runtimes: number; + materializers: number; + policies: number; +} + +interface TypegraphResponse { + stats: TypegraphStats; + name: string; + version?: string; + created?: string; +} + +type Stats = TypegraphResponse;src/typegraph/core/src/typegraph.rs (1)
244-245: Ensure name preservation for critical types.The integration of the new optimizer replaces the previous commented-out
dedup_typesimplementation. As mentioned in the PR objectives, the new implementation no longer considers names during deduplication, which could cause some types to lose their user-defined names.While this change improves efficiency, it could impact metagen and introspection functionality where type names are important.
Consider if there are critical types where preserving names is essential, and ensure your consumers are aware of this breaking change. You might want to add a documentation note explaining this behavior change.
src/typegraph/optimize/src/main.rs (1)
1-16: Well-structured CLI for typegraph optimizationThis new entry point creates a simple but effective command-line interface for the typegraph optimizer. It reads a JSON typegraph from stdin, applies optimization, and outputs the result to stdout. The error handling is comprehensive using
color_eyre.However, consider adding some helpful output or usage information, especially for error cases. For instance, printing a usage message if stdin is empty or not valid JSON would improve the user experience.
fn main() -> color_eyre::eyre::Result<()> { + color_eyre::install()?; + let mut buffer = String::new(); - std::io::stdin().read_line(&mut buffer)?; + let bytes_read = std::io::stdin().read_line(&mut buffer)?; + if bytes_read == 0 { + eprintln!("Error: No input provided"); + eprintln!("Usage: echo '<typegraph-json>' | tg_optimize"); + std::process::exit(1); + } + let tg: Typegraph = serde_json::from_str(&buffer)?; let opt_tg = tg_optimize::optimize(Arc::new(tg)); println!("{}", serde_json::to_string(&opt_tg)?); Ok(())src/typegraph/optimize/src/lib.rs (2)
26-30: Method namesize()is misleading – consideras_usize()oridx()
size()returns the wrapped index cast tousize, but “size” usually refers to length.
Renaming improves readability and avoids mental friction when readingself.types[v.size()].-impl TypeIdx { - pub fn size(&self) -> usize { - self.0 as usize - } +impl TypeIdx { + #[inline] + pub const fn idx(self) -> usize { + self.0 as usize + } }
36-40:sizesmap is computed but never usedThe loop populates
sizes, yet the variable is not referenced afterwards.
Unless this is meant for future logging/telemetry, please remove it to silence the unused-variable warning (orlog::debug!the statistics).- let mut sizes = BTreeMap::new(); - for c in &k.components { - sizes.entry(c.1.len()).and_modify(|l| *l += 1).or_insert(1); - }src/typegraph/optimize/src/dedup.rs (1)
315-317: Bounds check missing inConversionMap::update
self.direct[*idx as usize]assumes that every index is within thedirectvector.
A corrupt or out-of-range index would panic. Consider an assertion or early-return guard to fail fast with a clear error message.src/typegraph/optimize/src/kosaraju.rs (1)
36-45: Deep recursion may overflow the stack on large graphs
visitperforms a recursive DFS. With thousands of types and deeply nested references, the call-stack can exceed the default 8 MiB limit, causing a runtime abort on some platforms.
An explicit stack (iterative DFS/BFS) would provide the same behaviour without this limitation and improves future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.locktools/tree/dist/assets/index-B7TWQuis.cssis excluded by!**/dist/**tools/tree/dist/assets/index-en07UP6E.jsis excluded by!**/dist/**tools/tree/dist/index.htmlis excluded by!**/dist/**tools/tree/dist/logo.svgis excluded by!**/dist/**,!**/*.svgtools/tree/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltools/tree/public/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (37)
.gitignore(2 hunks)Cargo.toml(3 hunks)src/typegraph/core/Cargo.toml(1 hunks)src/typegraph/core/src/t.rs(2 hunks)src/typegraph/core/src/typedef/float.rs(1 hunks)src/typegraph/core/src/typegraph.rs(1 hunks)src/typegraph/core/src/types/sdk/core.rs(2 hunks)src/typegraph/graph/Cargo.toml(1 hunks)src/typegraph/graph/src/types/float.rs(1 hunks)src/typegraph/optimize/Cargo.toml(1 hunks)src/typegraph/optimize/src/dedup.rs(1 hunks)src/typegraph/optimize/src/kosaraju.rs(1 hunks)src/typegraph/optimize/src/lib.rs(1 hunks)src/typegraph/optimize/src/main.rs(1 hunks)src/typegraph/schema/Cargo.toml(1 hunks)src/typegraph/schema/src/lib.rs(2 hunks)src/typegraph/schema/src/types.rs(5 hunks)src/typegraph/schema/src/validator/types.rs(1 hunks)src/typegraph/schema/src/validator/value.rs(1 hunks)tools/tree-view-web.ts(2 hunks)tools/tree/.gitignore(1 hunks)tools/tree/.vite/deps/_metadata.json(1 hunks)tools/tree/.vite/deps/package.json(1 hunks)tools/tree/README.md(1 hunks)tools/tree/eslint.config.js(1 hunks)tools/tree/index.html(1 hunks)tools/tree/package.json(1 hunks)tools/tree/src/App.css(1 hunks)tools/tree/src/App.tsx(1 hunks)tools/tree/src/components/Type.tsx(1 hunks)tools/tree/src/index.css(1 hunks)tools/tree/src/main.tsx(1 hunks)tools/tree/src/vite-env.d.ts(1 hunks)tools/tree/tsconfig.app.json(1 hunks)tools/tree/tsconfig.json(1 hunks)tools/tree/tsconfig.node.json(1 hunks)tools/tree/vite.config.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/typegraph/schema/src/validator/value.rs (1)
src/typegraph/core/src/t.rs (6)
min(122-125)min(171-174)min(426-429)max(128-131)max(177-180)max(432-435)
src/typegraph/optimize/src/main.rs (2)
src/typegraph/optimize/src/kosaraju.rs (2)
new(16-27)new(68-79)src/typegraph/optimize/src/lib.rs (1)
optimize(32-44)
src/typegraph/optimize/src/lib.rs (4)
src/typegate/src/types.ts (1)
TypeIdx(107-107)src/typegraph/optimize/src/dedup.rs (1)
from(26-37)src/typegraph/optimize/src/kosaraju.rs (2)
new(16-27)new(68-79)src/typegraph/schema/src/validator/types.rs (1)
new(13-15)
src/typegraph/optimize/src/kosaraju.rs (1)
src/typegate/src/types.ts (1)
TypeIdx(107-107)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (34)
tools/tree/.gitignore (1)
1-24: Standard ignore rules are correctly configured. The file excludes logs,node_modules, build outputs (dist-ssr), editor configurations, and local-specific files, which aligns with typical Vite/React project conventions.tools/tree/src/App.css (1)
1-49: CSS styles are well-structured and comprehensive. The flex layout, typography, logo transitions, and keyframe animations are defined correctly, with media queries respecting reduced-motion preferences.tools/tree/.vite/deps/package.json (1)
1-3:"type": "module"is correctly set. This designation is essential for ES module semantics in Vite's dependency optimizer..gitignore (2)
9-9: Unignoretools/tree/dist/for tracked builds. This ensures that the built frontend assets are committed where intended.
59-59: Ignore/vivavox/directory. Prevents external or unrelated files from being accidentally committed.tools/tree/src/vite-env.d.ts (1)
1-5: License header and Vite type reference are correct. The MPL-2.0 header and triple-slash directive support TypeScript compilation against Vite environment types.tools/tree/tsconfig.json (1)
1-7: LGTM: TypeScript project references setupThis is a clean, standard configuration for a TypeScript project using project references. The separation of app and node configs is a good practice for projects with different compilation targets.
tools/tree/vite.config.ts (2)
8-18: Consider security implications of host "0.0.0.0"Setting the host to "0.0.0.0" makes the development server accessible from any network interface. While this is useful for development in networked environments or containers, it could potentially expose your development server to other machines on the network.
For local development, consider using "localhost" instead, or ensure this configuration is only used in properly secured environments.
12-16:✅ Verification successful
Verify API server availability
The proxy configuration assumes a backend server running on port 9000. Ensure there's proper error handling in the frontend for cases when the API server is unavailable.
🏁 Script executed:
#!/bin/bash # Check if there's any error handling for API server unavailability in the codebase rg -A 3 -B 3 "catch|error.*api|api.*error" --type ts --type tsx tools/tree/src/Length of output: 109
To ensure we capture all TypeScript and TSX files, let’s broaden the Ripgrep search to use file globs instead of type flags:
🏁 Script executed:
#!/bin/bash # Search for any API-related error handling in all .ts and .tsx files rg -A 3 -B 3 "catch|error.*api|api.*error" -g '*.ts' -g '*.tsx' tools/tree/src/Length of output: 571
Let’s inspect the full
Headercomponent to see iferroris handled in the UI:
🏁 Script executed:
#!/bin/bash # Show the Header component (including error handling) in App.tsx rg -C 20 "function Header" tools/tree/src/App.tsxLength of output: 1436
Let’s confirm whether the
errorfromuseSWRis ever rendered or handled in the UI:
🏁 Script executed:
#!/bin/bash # Check all usages of `error` in App.tsx to see if it's displayed rg -n -C5 "error" tools/tree/src/App.tsxLength of output: 1708
API error handling is already implemented in the UI
Both components using the
/apiproxy surfacefetcherrors via SWR and display them:
- In
Header(around line 40):{error && <option className="text-red-400">Error loading typegraphs</option>}- In
Status(around lines 80–84):if (error) { return ( <div …> <span className="text-red-400">Error: {error.message}</span> </div> ); }Since the custom
fetcherthrows on network or non-OK responses and both components render an error state, no additional handling is needed here.tools/tree/index.html (1)
9-13: Verify script path for production
Ensure that thesrc="/src/main.tsx"reference aligns with your Vite deployment. In some configurations, a relative path (e.g.,./src/main.tsx) or embedding of%PUBLIC_URL%may be required post-build to correctly locate the module.tools/tree/eslint.config.js (2)
1-6: Validate the TypeScript ESLint import
The package'typescript-eslint'may not correspond to the official ESLint plugin or parser. Please confirm that this import resolves correctly innode_modules; you might need to switch to@typescript-eslint/eslint-plugin(for rules) and@typescript-eslint/parserunderlanguageOptions.parser.
20-26: Proper inclusion of React Hooks and Refresh rules
Spreadingreact-hooks.configs.recommended.rulesand configuring'react-refresh/only-export-components'is well aligned with HMR and hook linting. Nice setup for a Vite + React project.tools/tree/package.json (1)
21-33: Verify TypeScript ESLint package name
You list"typescript-eslint"indevDependencies, but the official ESLint tooling is scoped under@typescript-eslint. Confirm the correct package names (@typescript-eslint/parserand@typescript-eslint/eslint-plugin) to ensure ESLint loads your TypeScript rules.tools/tree/src/index.css (1)
1-47: LGTM! Well-structured CSS foundation.The CSS file correctly sets up styling with Tailwind integration, proper typography, color schemes for both light and dark modes, and responsive design considerations. The styling is clean and follows modern web standards.
tools/tree/tsconfig.node.json (1)
1-23: Strong TypeScript configuration with appropriate settings.The configuration is well-structured for a Node.js environment with modern TypeScript features. The strict type checking, ES2022/ES2023 targets, and bundler module resolution are appropriate for a Vite-based project.
src/typegraph/graph/Cargo.toml (1)
16-16: Good addition of ordered-float dependency.Adding the
ordered-floatworkspace dependency aligns with the broader changes across the codebase to improve floating-point safety by usingNotNan<f64>wrappers instead of rawf64values.src/typegraph/core/Cargo.toml (1)
8-8: New optimization dependency looks good.Adding the
tg_optimizeworkspace dependency enables the core package to invoke the new typegraph optimization functionality, which will help with type deduplication as mentioned in the PR objectives.src/typegraph/schema/Cargo.toml (1)
12-12: Good addition of ordered-float dependency.Adding the
ordered-floatworkspace dependency supports the use ofNotNanwrappers for floating-point values, improving type safety across the schema package.src/typegraph/optimize/Cargo.toml (1)
1-10: LGTM! Proper workspace configuration for the new optimization crate.The manifest correctly configures the new
tg_optimizecrate with workspace-aligned versioning and appropriate dependencies. This setup integrates well with the typegraph optimization feature mentioned in the PR objectives.src/typegraph/schema/src/lib.rs (2)
135-135: Added Hash trait to support type deduplication.The
Hashtrait derivation is correctly added to thePolicyIndicesByEffectstruct, enabling it to be used in hash-based collections for the typegraph optimization mentioned in the PR objectives.
144-144: Added Hash trait to support type deduplication.The
Hashtrait derivation is correctly added to thePolicyIndicesenum, consistent with the addition toPolicyIndicesByEffectand supporting the type deduplication feature.src/typegraph/schema/src/validator/value.rs (1)
82-82: Updated float validation to unwrap NotNan values.The validator now correctly unwraps
NotNan<f64>values before validation, maintaining compatibility with the type changes that replaced rawf64withNotNan<f64>for floating-point safety. This change ensures that validation logic continues to work with the updated type system.Also applies to: 85-85, 88-88, 91-91, 94-95
src/typegraph/core/src/t.rs (2)
13-13: Added NotNan import for float safety.The import of
ordered_float::NotNansupports the conversion of float values to non-NaN wrappers in the builder methods.
172-173:❓ Verification inconclusive
Added NotNan wrapping for float constraints.
The
FloatBuildermethods now wrap all float values withNotNan::new()to prevent NaN values in the type system, which aligns with the PR's type deduplication goals by ensuring consistent floating-point behavior.Note that using
.unwrap()will panic if a NaN value is passed. Consider if error handling would be more appropriate:Also applies to: 178-179, 184-185, 190-191
🏁 Script executed:
#!/bin/bash # Check if there are any existing error handling patterns for NotNan in the codebase # Look for patterns of NotNan error handling rg "NotNan::new\(.*\)(\.map|\.ok\(\)|\.or|\.unwrap_or|\.expect|\?)" -A 1 -B 1Length of output: 79
Review NotNan unwrap handling
The
FloatBuildermethods currently wrap float constraints withNotNan::new(..).unwrap(), which will panic if a NaN value is passed. Please review whether:
- Panicking on NaN inputs is acceptable (and document this behavior),
- Or the methods should be refactored to return a
Resultand propagate the error,- Or at minimum use
.expect("NaN values are not allowed")for clearer panic messages.Occurrences to review in
src/typegraph/core/src/t.rs:
- Lines 172–173
- Lines 178–179
- Lines 184–185
- Lines 190–191
src/typegraph/core/src/typedef/float.rs (2)
73-77: Simplified hashing implementation for TypeFloatThe code now directly hashes the
Option<f64>values rather than wrapping them inOrderedFloat. This aligns with the broader change to useNotNan<f64>wrappers for floating-point values across the codebase.
82-82: Direct value hashing for enumerationThe value is now directly hashed instead of using an
OrderedFloatwrapper, consistent with the changes above.Cargo.toml (3)
17-17: Added new optimization crate to workspace membersThe new
src/typegraph/optimizecrate has been added to the workspace members, which aligns with the PR objective of introducing type deduplication optimization.
41-41: Added internal dependency for the optimize crateThe
tg_optimizedependency is correctly added to the workspace dependencies, enabling other crates to use the optimization functionality.
136-136: Updated ordered-float dependency with necessary featuresThe
ordered-floatdependency has been updated to version 5.0.0 withstdandserdefeatures enabled. This supports theNotNan<f64>wrapper types used for floating-point constraints across the codebase.src/typegraph/schema/src/validator/types.rs (1)
177-177: Updated floating-point unwrapping for subtype validationThe code now correctly unwraps
NotNan<f64>values using.into_inner()before comparing them in the subtype validation. This adjustment is necessary due to the broader changes across the codebase to useNotNan<f64>wrappers for floating-point values.This ensures that numerical comparisons work correctly when validating that an integer type is a subtype of a float type.
Also applies to: 183-183, 189-189, 195-195, 201-201
src/typegraph/core/src/types/sdk/core.rs (1)
98-105: Consider documenting theNaN-safety guarantee introduced byNotNanReplacing raw
f64withNotNan<f64>is a solid safety improvement; the wrapper will rejectNaNvalues at deserialisation time.
Because this is now part of the public SDK surface, callers need to understand that providing, or programmatically producing,NaNwill yield a runtime error instead of silently accepting the value as before. A short note in the Rustdoc (and end-user docs) forTypeFloatwould avoid surprises.No code change required – this is a documentation / DX follow-up.
src/typegraph/graph/src/types/float.rs (1)
10-15: Hash / equality now rely onNotNan; verify downstream derivesThe struct fields switched to
Option<NotNan<f64>>, which automatically providesEq,Ord, andHash.
Double-check any place that previously calledto_bits()or wrapped values inOrderedFloatfor hashing/equality – those work-arounds should now be removed to avoid double-wrapping or divergent semantics (e.g.schema::types::FloatTypeData).If such legacy code still exists, compilation will succeed but logic may diverge (two identical numbers wrapped twice will compare unequal).
src/typegraph/optimize/src/dedup.rs (2)
208-217: Potential panic when parsingauth.auth_data["profiler"]
serde_json::from_value(v.clone()).unwrap()will panic if the value is not a valid unsigned integer (or if the key is missing).
As this field originates from user-supplied metadata, prefer graceful error handling (e.g.if let Ok(mut idx) = ...) or log a diagnostic instead of aborting the whole optimization pass.
124-132: 🛠️ Refactor suggestionHash is currently based on pointer addresses, breaking determinism
Collecting with
data.policies.iter().collect()stores references (&String,&u32) in theBTreeMap.
Because the hashed value now depends on pointer addresses, the same typegraph can hash differently between runs, defeating deterministic deduplication.- let policies: BTreeMap<_, _> = data.policies.iter().collect(); // TODO + // Clone into an owned, order-stable map to guarantee + // deterministic hashing across processes. + let policies: BTreeMap<_, _> = data + .policies + .iter() + .map(|(k, v)| (k.clone(), *v)) + .collect();Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/typegraph/optimize/src/lib.rs (1)
94-94:unreachable!()onAnytype may panic on legitimate graphsThe
TypeNode::Anyis a valid variant in the schema; hitting it during optimization would cause a hard panic. Unless you can guarantee it's impossible to encounter this variant, consider replacing with an empty neighbor list:- N::Any { .. } => unreachable!(), + N::Any { .. } => vec![],If it truly is unreachable, add a debug assertion with a comment explaining why.
🧹 Nitpick comments (5)
src/typegraph/optimize/src/lib.rs (5)
23-41: Add documentation for public structs and methodsThe
TypeIdxstruct and its methods are public but lack proper documentation. Consider adding documentation comments explaining:
- The purpose of this wrapper type
- What the
size()method does (returns the index as a usize)- Any invariants or assumptions that users should be aware of
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] +/// A wrapper around a `u32` that represents a type index in the typegraph. pub struct TypeIdx(u32); impl From<u32> for TypeIdx { fn from(value: u32) -> Self { Self(value) } } impl From<&u32> for TypeIdx { fn from(value: &u32) -> Self { Self(*value) } } impl TypeIdx { + /// Returns the index as a `usize`, suitable for indexing into arrays and vectors. pub fn size(&self) -> usize { self.0 as usize } }
43-55: Enhance theoptimizefunction documentation and consider error handlingThe main
optimizefunction would benefit from more comprehensive documentation including:
- Input/output specifications
- Performance characteristics
- Any edge cases or limitations
Additionally, consider handling potential errors rather than allowing panics during optimization.
+/// Optimizes a typegraph by deduplicating types when possible. +/// +/// # Arguments +/// +/// * `tg` - The input typegraph to optimize +/// +/// # Returns +/// +/// A new optimized typegraph with duplicated types merged +/// +/// # Performance +/// +/// This function performs Kosaraju's algorithm to find strongly connected components, +/// then uses hashing to deduplicate types, with a time complexity of O(V + E) where V +/// is the number of types and E is the number of references between types. pub fn optimize(tg: Arc<Typegraph>) -> Typegraph { let mut k = Kosaraju::new(tg); k.run(); let mut sizes = BTreeMap::new(); for c in &k.components { sizes.entry(c.1.len()).and_modify(|l| *l += 1).or_insert(1); } + // Consider logging or returning these statistics for analysis let mut dedup = crate::dedup::DedupEngine::from(k); dedup.hash_types(); dedup.get_converted_typegraph() }
47-51: Consider using the component size statistics or remove unused codeThe code collects statistics on component sizes but doesn't use them. Either:
- Use these statistics (e.g., for logging, telemetry, or diagnostics)
- Remove the unused code to improve clarity
- let mut sizes = BTreeMap::new(); - for c in &k.components { - sizes.entry(c.1.len()).and_modify(|l| *l += 1).or_insert(1); - } + // Optionally log component size statistics for performance analysis + if log::debug_enabled!() { + let mut sizes = BTreeMap::new(); + for c in &k.components { + sizes.entry(c.1.len()).and_modify(|l| *l += 1).or_insert(1); + } + log::debug!("SCC size distribution: {:?}", sizes); + }
57-60: Add documentation to theDirectedGraphtraitThe
DirectedGraphtrait would benefit from documentation explaining its purpose and usage within the optimization process.+/// Represents a directed graph with methods to traverse its structure. +/// +/// This trait is used by the Kosaraju algorithm to find strongly connected components +/// in the typegraph. trait DirectedGraph { + /// The type used to reference vertices in the graph type VertexRef; + + /// Returns the outgoing neighbors of a vertex + /// + /// # Arguments + /// + /// * `v` - A reference to the vertex + /// + /// # Returns + /// + /// A vector of references to neighboring vertices fn out_neighbours(&self, v: &Self::VertexRef) -> Vec<Self::VertexRef>; }
65-96: Consider optimizing memory allocations inout_neighboursThe
out_neighboursmethod creates a new vector for each node. For nodes with known output sizes, consider pre-allocating the vector with the correct capacity to reduce memory allocations.fn out_neighbours(&self, v: &TypeIdx) -> Vec<TypeIdx> { let type_node = &self.types[v.size()]; use TypeNode as N; match type_node { N::Boolean { .. } | N::Integer { .. } | N::Float { .. } | N::String { .. } | N::File { .. } => vec![], - N::Optional { data, .. } => vec![data.item.into()], + N::Optional { data, .. } => { + let mut res = Vec::with_capacity(1); + res.push(data.item.into()); + res + }, - N::List { data, .. } => vec![data.items.into()], + N::List { data, .. } => { + let mut res = Vec::with_capacity(1); + res.push(data.items.into()); + res + }, - N::Object { data, .. } => data.properties.values().map(|v| v.into()).collect(), + N::Object { data, .. } => { + let props = &data.properties; + let mut res = Vec::with_capacity(props.len()); + res.extend(props.values().map(|v| v.into())); + res + }, - N::Union { data, .. } => data.any_of.iter().map(|v| v.into()).collect(), + N::Union { data, .. } => { + let any_of = &data.any_of; + let mut res = Vec::with_capacity(any_of.len()); + res.extend(any_of.iter().map(|v| v.into())); + res + }, - N::Either { data, .. } => data.one_of.iter().map(|v| v.into()).collect(), + N::Either { data, .. } => { + let one_of = &data.one_of; + let mut res = Vec::with_capacity(one_of.len()); + res.extend(one_of.iter().map(|v| v.into())); + res + }, N::Function { data, .. } => { - let mut res = vec![data.input.into(), data.output.into()]; + let mut res = Vec::with_capacity(if data.parameter_transform.is_some() { 3 } else { 2 }); + res.push(data.input.into()); + res.push(data.output.into()); if let Some(pt) = &data.parameter_transform { res.push(pt.resolver_input.into()); } res } N::Any { .. } => unreachable!(), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegraph/optimize/src/lib.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: pre-commit
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tools/tree-view-web.ts (1)
81-91:⚠️ Potential issueAddress typegraph name collisions - requires updating from past review comments
The current implementation warns about duplicate typegraph names but still silently overwrites typegraphs with the same name. This issue becomes more critical with the recent PR change where "type deduplication no longer considers names," which increases the likelihood of name collisions.
The suggestion from a past review comment to store arrays of typegraphs for each name has not been implemented.
Apply this suggested fix:
-const byName: Map<string, TypeGraphDS> = new Map(); +const byName: Map<string, TypeGraphDS[]> = new Map(); for (const tg of tgs) { const name = tg.types[0].title; - if (byName.has(name)) { - console.log( - `[warn] Duplicate typegraph name '${name}'. The older one will be dropped`, - ); - } - byName.set(tg.types[0].title, tg); + const list = byName.get(name) ?? []; + if (list.length > 0) { + console.log( + `[info] Multiple typegraphs with name '${name}' found. All will be preserved.`, + ); + } + list.push(tg); + byName.set(name, list); }Then update the API routes at lines 119-162 to handle arrays of typegraphs instead of single instances.
🧹 Nitpick comments (1)
tools/tree-view-web.ts (1)
173-185: Enhance SPA fallback middleware with error handlingThe SPA fallback middleware doesn't handle potential errors when reading the index.html file, which could lead to unhandled exceptions.
app.use(async (ctx, next) => { const pathname = ctx.request.url.pathname; if ( pathname.startsWith("/api") || pathname == "/logo.svg" || pathname.startsWith("/assets") ) { await next(); } else { - ctx.response.body = await getDistFile("index.html"); - ctx.response.type = "html"; + try { + ctx.response.body = await getDistFile("index.html"); + ctx.response.type = "html"; + } catch (error) { + console.error(`[error] Failed to read index.html: ${error.message}`); + ctx.response.status = Status.InternalServerError; + ctx.response.body = "Internal Server Error"; + } } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tools/tree/dist/assets/index-BF0z4CXe.cssis excluded by!**/dist/**tools/tree/dist/assets/index-cH6Zi8mN.jsis excluded by!**/dist/**tools/tree/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (4)
src/typegraph/optimize/src/dedup.rs(1 hunks)src/typegraph/optimize/src/kosaraju.rs(1 hunks)tools/tree-view-web.ts(2 hunks)tools/tree/index.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/tree/index.html
- src/typegraph/optimize/src/dedup.rs
- src/typegraph/optimize/src/kosaraju.rs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (1)
tools/tree-view-web.ts (1)
95-107: LGTM! Binary files are now handled correctlyThe implementation now correctly uses
Uint8Arrayinstead of strings for the file system cache, andDeno.readFileinstead ofDeno.readTextFile, which addresses the previous issue with binary files.
There was a problem hiding this comment.
Nice work!
Worth to try probably is to have an utility that duplicates an existing typegraph N times, I think we need to check any method against some (or all) of our example typegraphs.
Then we can keep control of the how much the deduplication is improving or degrading throughout iterations... and more importantly its reduction trend to check wether or not we hit a wall.
# Conflicts: # Cargo.lock # deno.lock # tests/prisma-migrations/migration-failure-test-dev/main/20250430193822_generated/migration.sql # tests/prisma-migrations/migration-failure-test-dev/main/20250606234135_generated/migration.sql # tests/prisma-migrations/migration-failure-test-dev/main/20250618092719_generated/migration.sql # tools/tree-view-web.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tools/tree-view-web.ts (3)
32-43: Add error handling for JSON parsing failures.The JSON loading path lacks error handling for potential parsing failures, which could crash the application if an invalid JSON file is provided.
if (args.json) { for (const file of files) { - const raw = await Deno.readFile(file); - const str = new TextDecoder().decode(raw); - const parsed = JSON.parse(str); - if (Array.isArray(parsed)) { - tgs.push(...parsed); - } else { - tgs.push(parsed); - } + try { + const raw = await Deno.readFile(file); + const str = new TextDecoder().decode(raw); + const parsed = JSON.parse(str); + if (Array.isArray(parsed)) { + tgs.push(...parsed); + } else { + tgs.push(parsed); + } + } catch (error) { + console.error(`[error] Failed to parse JSON file '${file}': ${error.message}`); + continue; + } } }
81-91: Silent key collisions still occur in the byName map.The current implementation silently overwrites typegraphs with duplicate names, which is problematic after the type deduplication changes mentioned in the PR objectives.
-const byName: Map<string, TypeGraphDS> = new Map(); +const byName: Map<string, TypeGraphDS[]> = new Map(); for (const tg of tgs) { - const name = tg.types[0].title; - if (byName.has(name)) { - console.log( - `[warn] Duplicate typegraph name '${name}'. The older one will be dropped` - ); - } - byName.set(tg.types[0].title, tg); + const name = tg.types[0]?.title ?? "untitled"; + const existing = byName.get(name) ?? []; + existing.push(tg); + byName.set(name, existing); + if (existing.length > 1) { + console.log(`[warn] Multiple typegraphs named '${name}' (${existing.length} total)`); + } }You'll also need to update the API handlers to handle arrays of typegraphs.
112-121: Improve asset parameter validation.The current validation is basic and the error message has a typo ("assset"). Consider more comprehensive path validation.
router.get("/assets/:asset", async (ctx) => { const asset = ctx.params.asset; - if (asset.includes("..") || asset.includes("/")) { - ctx.response.body = `invalid assset name '${asset}'`; + // Validate asset path - only allow safe filenames + if (asset.includes("..") || asset.includes("/") || asset.includes("\\")) { + ctx.response.body = `invalid asset name '${asset}'`; ctx.response.status = Status.BadRequest; } else { - ctx.response.body = await getDistFile(`assets/${ctx.params.asset}`); - ctx.response.type = ctx.params.asset.split(".").slice(-1)[0]; + try { + ctx.response.body = await getDistFile(`assets/${asset}`); + ctx.response.type = asset.split(".").slice(-1)[0]; + } catch (error) { + ctx.response.status = Status.NotFound; + ctx.response.body = `asset not found: ${asset}`; + } } });
🧹 Nitpick comments (3)
.ghjk/lock.json (1)
2104-2141: New CI environment duplicates the existing one – consider extending instead of cloning
bciqmgoa4swy4…is almost byte-for-byte identical to the existing CI env (bciqgfkhptprs54…) with the sole addition of:{ "ty": "posix.envVar", "key": "MCLI_NO_UPGRADE_CHECK", "val": "1" }Maintaining two parallel definitions invites drift. A leaner option:
@@ "bciqgfkhptprs54tg3wl45acxctzc2lu4v2xlh3slk6w4m5g5ctysv4a": { "provides": [ @@ { "ty": "posix.envVar", "key": "GIT_CLIFF_CONFIG", "val": "tools/cliff.toml" }, + { "ty": "posix.envVar", "key": "MCLI_NO_UPGRADE_CHECK", "val": "1" }, @@ ] }, - "bciqmgoa4swy4qcnmwuku3qhhsg4mjh3jkw7k5by6s6ytp3hyrhkqr3i": { ... duplicate ... }Then revert
test-e2e.envKeyto the original ID.
This keeps the configuration single-sourced and avoids an ever-growing env registry.src/typegraph/core/src/t.rs (1)
186-186: Consider error handling for NaN valuesThe use of
NotNan::new(...).unwrap()will panic if NaN values are passed to these methods. Consider whether this is the intended behavior or if error handling should be implemented.If you want to handle NaN values gracefully, consider returning a
Resultinstead:- pub fn min(mut self, min: f64) -> Self { - self.data.min = Some(NotNan::new(min).unwrap()); + pub fn min(mut self, min: f64) -> Result<Self, ordered_float::FloatIsNan> { + self.data.min = Some(NotNan::new(min)?); + Ok(self)However, if panicking on NaN is the intended behavior for type safety, the current implementation is acceptable.
Also applies to: 192-192, 198-198, 204-204
tools/tree-view-web.ts (1)
152-165: Type index validation is improved but still has an issue.The validation logic is better now, but there's still a redundant access to
tg.types[+ctx.params.typeIdx]instead of using the already-validatedtypeNode.const typeNode = tg.types[typeIdx]; if (typeNode == null) { ctx.response.body = `type index out of bounds #${typeIdx}`; ctx.response.status = Status.NotFound; } else { - ctx.response.body = tg.types[+ctx.params.typeIdx]; + ctx.response.body = typeNode; ctx.response.type = "json"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.locktools/tree/dist/assets/index-B7TWQuis.cssis excluded by!**/dist/**tools/tree/dist/assets/index-en07UP6E.jsis excluded by!**/dist/**tools/tree/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (36)
.ghjk/lock.json(2 hunks).pre-commit-config.yaml(1 hunks)Cargo.toml(3 hunks)ghjk.ts(12 hunks)src/meta-cli/src/main.rs(1 hunks)src/typegraph/core/Cargo.toml(1 hunks)src/typegraph/core/src/global_store.rs(5 hunks)src/typegraph/core/src/lib.rs(2 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/additional_filters.rs(4 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/aggregate.rs(2 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/count.rs(1 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/filters.rs(7 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/group_by.rs(4 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/input_type.rs(1 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/order_by.rs(6 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/out_type.rs(1 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/query_input_type.rs(1 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/query_where_expr.rs(1 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/where_.rs(1 hunks)src/typegraph/core/src/runtimes/prisma/type_generation/with_nested_count.rs(1 hunks)src/typegraph/core/src/runtimes/substantial/type_utils.rs(2 hunks)src/typegraph/core/src/t.rs(3 hunks)src/typegraph/core/src/typegraph.rs(6 hunks)src/typegraph/core/src/types/type_ref.rs(6 hunks)src/typegraph/core/src/types/type_ref/xdef.rs(1 hunks)src/typegraph/core/src/utils/postprocess/naming.rs(4 hunks)src/typegraph/optimize/src/dedup.rs(1 hunks)src/typegraph/optimize/src/lib.rs(1 hunks)src/typegraph/optimize/src/main.rs(1 hunks)src/typegraph/schema/src/types.rs(6 hunks)src/typegraph/schema/src/validator/types.rs(1 hunks)tests/prisma-migrations/migration-failure-test-dev/main/20250618092719_generated/migration.sql(1 hunks)tools/tasks/lint.ts(2 hunks)tools/tasks/test.ts(1 hunks)tools/tree-view-web.ts(2 hunks)tools/verdaccio/config.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- tools/verdaccio/config.yaml
✅ Files skipped from review due to trivial changes (7)
- src/typegraph/core/src/runtimes/prisma/type_generation/aggregate.rs
- src/typegraph/core/src/runtimes/prisma/type_generation/input_type.rs
- src/typegraph/core/src/runtimes/prisma/type_generation/additional_filters.rs
- tests/prisma-migrations/migration-failure-test-dev/main/20250618092719_generated/migration.sql
- src/typegraph/core/src/runtimes/prisma/type_generation/filters.rs
- src/typegraph/core/src/types/type_ref/xdef.rs
- src/typegraph/core/src/runtimes/prisma/type_generation/order_by.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/typegraph/core/Cargo.toml
- src/typegraph/optimize/src/main.rs
- src/typegraph/schema/src/validator/types.rs
- Cargo.toml
- src/typegraph/schema/src/types.rs
- src/typegraph/core/src/typegraph.rs
- src/typegraph/core/src/global_store.rs
- src/typegraph/optimize/src/dedup.rs
- src/typegraph/optimize/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pre-commit
🔇 Additional comments (33)
ghjk.ts (1)
6-7: LGTM! Consistent formatting improvements.The trailing comma removals and empty comment line addition are consistent formatting changes that don't affect functionality. These changes improve code style consistency across the file.
Also applies to: 39-39, 45-45, 62-62, 74-74, 93-93, 112-112, 126-126, 148-148, 158-159, 168-168, 196-196, 212-212, 217-217
tools/tasks/test.ts (1)
11-13: Environment variable syntax looks correct.The addition of the
varsproperty withMCLI_NO_UPGRADE_CHECKset to "1" follows proper task definition syntax and will correctly set the environment variable during test execution..ghjk/lock.json (1)
1115-1119: I’ll broaden the search to the entire repo and also check whereenvsNamed.ciis defined to see if it still points at the old key.#!/bin/bash # 1. Search for any leftover references to the old envKey across the repo rg -n '"envKey": "bciqgfkhptprs54tg3wl45acxctzc2lu4v2xlh3slk6w4m5g5ctysv4a"' || true # 2. Find where envsNamed.ci is declared or referenced rg -n 'envsNamed\.ci' -C2 || truesrc/typegraph/core/src/runtimes/prisma/type_generation/out_type.rs (1)
76-76: LGTM: Consistent application of NameSource classification.The change from
build_namedtobuild_named_pcorrectly identifies this as a Prisma-generated type, aligning with the newNameSourceenum classification system.src/typegraph/core/src/runtimes/prisma/type_generation/count.rs (1)
17-17: LGTM: Proper classification of Prisma-generated count type.The change correctly uses
build_named_pto mark this count type as Prisma-generated, maintaining consistency with the new naming source classification.src/typegraph/core/src/runtimes/prisma/type_generation/with_nested_count.rs (1)
94-94: LGTM: Consistent NameSource classification for nested count types.The change appropriately uses
build_named_pto classify this complex nested count type as Prisma-generated, maintaining consistency with the refactoring effort.src/typegraph/core/src/runtimes/prisma/type_generation/query_input_type.rs (1)
72-72: LGTM: Proper classification of query input types.The change correctly applies
build_named_pto mark query input types as Prisma-generated, maintaining the existing conditional logic while aligning with the new classification system.src/typegraph/core/src/runtimes/prisma/type_generation/where_.rs (2)
112-112: LGTM: Consistent application of Prisma type classification.The change correctly uses
build_named_pfor WHERE clause types, maintaining the complex generation logic while properly marking them as Prisma-generated.
52-113: Verify completeness of the build_named_p refactor across all Prisma type generation files.Ensure that all Prisma runtime type generation files have been consistently updated to use
build_named_pinstead ofbuild_named.#!/bin/bash # Description: Verify that all Prisma type generation files use build_named_p consistently # Expected: All build_named calls in Prisma type generation should be replaced with build_named_p echo "=== Checking for remaining build_named calls in Prisma type generation files ===" rg "build_named\(" src/typegraph/core/src/runtimes/prisma/type_generation/ || echo "No remaining build_named calls found" echo -e "\n=== Verifying build_named_p usage in Prisma type generation files ===" rg "build_named_p\(" src/typegraph/core/src/runtimes/prisma/type_generation/ -A 1 -B 1 echo -e "\n=== Listing all Prisma type generation files for completeness check ===" fd -e rs . src/typegraph/core/src/runtimes/prisma/type_generation/src/typegraph/core/src/runtimes/prisma/type_generation/query_where_expr.rs (1)
58-58: LGTM: Consistent naming source refactoringThe replacement of
build_namedwithbuild_named_pcorrectly specifies thePrismaTypeGennaming source, aligning with the systematic naming source classification introduced across the codebase.src/typegraph/core/src/runtimes/prisma/type_generation/group_by.rs (1)
36-36: LGTM: Systematic naming source specificationAll replacements of
build_namedwithbuild_named_pcorrectly classify these types as Prisma-generated, maintaining consistent naming source tracking across the codebase.Also applies to: 69-69, 115-115, 167-167
src/typegraph/core/src/lib.rs (2)
7-7: LGTM: Required import for naming sourceThe import of
NameSourceis necessary for the updatedrename_typemethod implementation.
238-240: LGTM: Explicit user naming source specificationThe updated
rename_typemethod correctly specifiesNameSource::Userfor renamed types, ensuring proper classification in the new naming source system.src/typegraph/core/src/runtimes/substantial/type_utils.rs (1)
97-99: LGTM: Consistent Substantial runtime naming sourceAll replacements of
build_namedwithbuild_named_subscorrectly specify theSubstantialnaming source, maintaining consistency with the naming source classification system for Substantial runtime types.Also applies to: 104-106, 110-110, 131-132, 135-135, 139-139, 143-143
src/typegraph/core/src/t.rs (2)
10-10: LGTM: Required imports for enhanced type safetyThe imports for
NameSourceandNotNanare necessary for the naming source classification system and improved floating-point safety.Also applies to: 14-14
26-41: LGTM: Well-designed naming source methodsThe new
TypeBuildermethods provide clear, explicit naming source specification:
build_named_pfor Prisma typesbuild_named_subsfor Substantial typesbuild_named_sfor generic use with anyNameSourcebuild_namednow explicitly usesNameSource::UserThe method naming is intuitive and the implementation is consistent.
src/typegraph/core/src/utils/postprocess/naming.rs (6)
16-16: Import addition looks good.The import of
NameSourcefrom the typegraph module aligns with the enhanced type naming classification system.
25-25: Efficient conversion from HashMap keys to HashSet.The conversion from
HashMap<u32, NameSource>keys toHashSet<u32>is performed efficiently using iterator chaining. This maintains the same logical behavior while supporting the richer name source classification.
67-70: Consistent field renaming in VisitContext.The
VisitContextstruct correctly updates fromuser_namedtonamed_typesfield, maintaining consistency with the broader naming changes.
265-265: Consistent field reference update.The condition in
collect_ref_infocorrectly usescx.named_types.contains(&id)instead of the previouscx.user_named, maintaining the same logical behavior.
323-323: Consistent field reference update.The condition in
gen_namecorrectly usescx.named_types.contains(&id)instead of the previouscx.user_named, maintaining the same logical behavior.
18-20: Verify the lifetime parameter usage across the codebase.The
NamingProcessorstruct now correctly holds a reference toHashMap<u32, NameSource>with proper lifetime parameter'un. This change supports the enhanced name source classification.#!/bin/bash # Description: Verify that all instantiations of NamingProcessor provide the required lifetime parameter # Expected: All usages should compile correctly with the new signature rg -A 5 "NamingProcessor" --type rustsrc/typegraph/core/src/types/type_ref.rs (6)
10-10: Import addition looks good.The import of
NameSourcefrom the typegraph module enables the enhanced name source classification.
115-115: Consistent field addition in builder.The
NamedRefBuildercorrectly includes thename_sourcefield, maintaining consistency with theNamedTypeRefstructure.
245-245: Proper field propagation in builder.The
TypeRefBuilder::with_idmethod correctly propagates thename_sourcefield from builder to the finalNamedTypeRefinstance.
213-213: Breaking API change requires careful migration.The
TypeRef::namedmethod signature now requires aNameSourceparameter. This is a significant breaking change that affects all callers.Verify that all existing calls to
TypeRef::namedhave been updated to provide thename_sourceparameter:#!/bin/bash # Description: Find all calls to TypeRef::named to ensure they provide the new parameter # Expected: All calls should include the NameSource parameter rg -A 3 "TypeRef::named|\.named\(" --type rustAlso applies to: 218-218
378-378: Breaking change in Named trait requires migration attention.The
Namedtrait's method signature change is a breaking change that will affect all implementors and callers. The implementation correctly forwards the new parameter.Verify that all implementations and calls to the
Namedtrait are updated:#!/bin/bash # Description: Find all usages of Named trait to ensure compatibility with new signature # Expected: All usages should provide the NameSource parameter rg -A 2 -B 2 "\.named\(" --type rustAlso applies to: 386-387
82-82: ```shell
#!/bin/bashSearch for all usages of NamedTypeRef to ensure no other manual constructions are missing the new
name_sourcefieldrg "NamedTypeRef\s*{" -n
</details> <details> <summary>.pre-commit-config.yaml (1)</summary> `42-42`: **LGTM! Simplified command aligns with task definition.** This change delegates the full linting logic to the `lint-deno` task, which now properly handles exclusions and deno lint internally. This improves maintainability by keeping linting configuration centralized. </details> <details> <summary>tools/tasks/lint.ts (2)</summary> `42-42`: **Good exclusion of the new frontend project.** Adding "tree" to the exclude list prevents conflicts with the new React frontend project's own ESLint configuration. --- `50-50`: **Explicit ignore flags improve clarity.** The explicit `--ignore` flags make the exclusions more visible and align with the directory exclusions in file collection above. </details> <details> <summary>tools/tree-view-web.ts (2)</summary> `97-105`: **LGTM! Binary file handling is now correct.** The `getDistFile` function now properly handles binary files by using `Deno.readFile` and storing `Uint8Array` in the cache, resolving the previous UTF-8 issue. --- `169-199`: **Excellent Oak integration with proper middleware setup.** The server architecture is well-structured with appropriate middleware for logging, SPA fallback, and routing. The listen event handler provides good user feedback. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| let upgrade = std::env::var("MCLI_NO_UPGRADE_CHECK") | ||
| .map(|v| v == "1") | ||
| .unwrap_or(false); | ||
| if upgrade { | ||
| if let Err(err) = runner.block_on(upgrade_check()) { | ||
| warn!("cannot check for update: {err}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix semantic mismatch between environment variable name and behavior.
The logic is inverted from what the environment variable name suggests. MCLI_NO_UPGRADE_CHECK implies that setting it to "1" should disable upgrade checks, but the current implementation enables them when set to "1".
Consider one of these solutions:
Option 1: Rename the environment variable to match current behavior:
- let upgrade = std::env::var("MCLI_NO_UPGRADE_CHECK")
+ let upgrade = std::env::var("MCLI_UPGRADE_CHECK")
.map(|v| v == "1")
.unwrap_or(false);Option 2: Invert the logic to match the current variable name:
- let upgrade = std::env::var("MCLI_NO_UPGRADE_CHECK")
- .map(|v| v == "1")
- .unwrap_or(false);
- if upgrade {
+ let no_upgrade = std::env::var("MCLI_NO_UPGRADE_CHECK")
+ .map(|v| v == "1")
+ .unwrap_or(false);
+ if !no_upgrade {
if let Err(err) = runner.block_on(upgrade_check()) {
warn!("cannot check for update: {err}");
}
}If choosing Option 2, also update the corresponding variable in tools/tasks/test.ts to "0" or remove it entirely.
📝 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.
| let upgrade = std::env::var("MCLI_NO_UPGRADE_CHECK") | |
| .map(|v| v == "1") | |
| .unwrap_or(false); | |
| if upgrade { | |
| if let Err(err) = runner.block_on(upgrade_check()) { | |
| warn!("cannot check for update: {err}"); | |
| } | |
| } | |
| let upgrade = std::env::var("MCLI_UPGRADE_CHECK") | |
| .map(|v| v == "1") | |
| .unwrap_or(false); | |
| if upgrade { | |
| if let Err(err) = runner.block_on(upgrade_check()) { | |
| warn!("cannot check for update: {err}"); | |
| } | |
| } |
| let upgrade = std::env::var("MCLI_NO_UPGRADE_CHECK") | |
| .map(|v| v == "1") | |
| .unwrap_or(false); | |
| if upgrade { | |
| if let Err(err) = runner.block_on(upgrade_check()) { | |
| warn!("cannot check for update: {err}"); | |
| } | |
| } | |
| let no_upgrade = std::env::var("MCLI_NO_UPGRADE_CHECK") | |
| .map(|v| v == "1") | |
| .unwrap_or(false); | |
| if !no_upgrade { | |
| if let Err(err) = runner.block_on(upgrade_check()) { | |
| warn!("cannot check for update: {err}"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/meta-cli/src/main.rs around lines 114 to 121, the environment variable
MCLI_NO_UPGRADE_CHECK is used with inverted logic: setting it to "1" currently
enables upgrade checks, but the name implies it should disable them. To fix
this, invert the boolean logic so that when MCLI_NO_UPGRADE_CHECK is "1",
upgrade checks are skipped (i.e., do not run upgrade_check). Also, update the
related variable in tools/tasks/test.ts accordingly by setting it to "0" or
removing it if disabling the check is not desired.
Migration notes
N/A
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores