Feat/architecture map - #25
Merged
Merged
Conversation
A real scan produced 22 boxes for a 71-file repository, several of them holding a single file. The node cap was a ceiling with no floor, so at 22 the collapse logic never ran at all. Worse than the count was the nesting: pages/api was drawn beside pages/api/admin and pages/api/admin/users as though the three were siblings, when two of them sit inside the first. A reader takes boxes at the same level to be peers, so that misrepresents the structure rather than merely cluttering it. Two rules now run before the cap. A module whose ancestor is also a module folds into it, which is what removes container-beside-contents. A module below three files folds into its parent, because a directory holding one file is not a unit of architecture. Both are about representation rather than size, so they apply whether or not the map is over its cap. Deepest-first over the same depth buckets as the cap logic, so the pass stays O(n log n). On a repository shaped like the one scanned, 21 boxes become 7, with every file still accounted for. The cost is bluntness: pages and pages/api now merge, losing a distinction that means something in a Next.js app. There is no version of the ancestor rule that keeps them apart, because that pairing is the container-beside-contents case. Drawing nesting as actual nesting would recover it, and is a larger change.
Pins the shape a real scan exposed: nested API route directories collapsing to a single box, a one-file directory joining its parent, and a top-level module being left alone because it has no parent to fold into. Two cases guard the invariants that folding could quietly break — findings and file counts surviving a merge, and an edge whose endpoints both fold into the same module disappearing rather than pointing at a box that is no longer drawn. Seven existing cases now pass minModuleFiles: 0. Their fixtures are one and two file directories that the real defaults fold away, and those tests are about edges and severity, not folding.
The diagram rendered at its natural pixel size, so a map wider than the card scrolled in both directions and arrived clipped on the right. A diagram you have to scroll around to assemble mentally is not doing its job. Now sized by viewBox at full width, capped at its natural width so a small map is never blown up past its own size. It only ever shrinks to fit.
The map is no longer gated, so the run input drops architectureMap and the scan builds it unconditionally. The engine change and the route change are one commit because the field was required: removing it from the input while the route still passes it, or the reverse, leaves a commit that does not typecheck. Every scan now pays the module labelling call. The extra file reads were never gated — they decide which files get scanned — so the added cost is one Gemini call per scan.
Drops the read-side gate. It existed so that removing an account from the allowlist also hid maps on scans that had already stored one; with no allowlist there is nothing left to revoke.
The architecture map was the only thing flags.ts gated, so with the map open to everyone the module, its tests, and the ARCHITECTURE_MAP_LOGINS env key all go with it. Nothing else imported isLoginAllowed. The three files are one commit because they form a single type contract: dropping the env key while the module still reads it, or deleting the module while its test still imports it, leaves a commit that does not typecheck. Worth resurrecting from history rather than rewriting if another feature needs login gating — the wildcard and empty-list handling were the fiddly parts.
The flag is gone, so the variable no longer does anything and should not look configurable.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![jargons-ai[bot]](https://avatars.githubusercontent.com/in/4369042?s=60&v=4){kind=small}
There was a problem hiding this comment.
Jargons review
Found 2 issues worth a look:
🟡 Medium — Potential for increased LLM costs
src/server/scan-engine/run-scan.ts:100
The architectureMap flag has been removed from runScan input, meaning the architecture map will now always be built. This could lead to increased LLM costs as the LLM call for architecture mapping will no longer be skipped for users without the flag.
Suggestion: Reintroduce a mechanism to conditionally build the architecture map based on user permissions or feature flags to control LLM spend. Consider moving the isArchitectureMapEnabled check back into runScan or an equivalent function.
🟡 Medium — Missing access control for architecture map display
src/server/scans.ts:166
The isArchitectureMapEnabled check has been removed from getCodebaseScan, which means the architecture map will now be displayed to all users regardless of their permissions. This removes the read-side gating for the feature.
Suggestion: Reintroduce an access control mechanism to gate the display of the architecture map based on user permissions. This ensures that the feature is only visible to authorized users.
🔧 Suggested fixes: open the fix PR →
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architecture map from a codebase scan
Adds a module dependency diagram to the scan detail page, and changes how a
scan picks which files to read.
Why it's two changes
The map needs to know what depends on what, and the scan had no dependency data
at all. Building that graph fixed a bigger problem:
collectFileswalked therepo tree in order and stopped at 20 files, so a scan only ever read whichever
files git happened to sort first — findings were a function of path position,
not risk. Now the import graph is built first and the 20-file budget goes to the
files with the highest fan-in, since a bug in a module twenty files depend on
matters more than one in a leaf.
How the map is built
List the tree, rank it, read the head of at most 200 files, resolve their
imports (JS/TS, Python, Go, Ruby), aggregate to directory modules, then one
Gemini call for labels.
Boxes are directories, arrows are resolved imports, colour is the most severe
finding in the module. Only the labels come from the model — a wrong label
is a caption you ignore, a wrong arrow is a false dependency diagram someone
pastes into a design doc.
Small and nested directories fold into their parents, so a 71-file repo draws
about 7 boxes instead of 22. Over the cap, modules collapse rather than vanish,
and the caption states how many files the arrows came from plus any modules left
off — the modules are complete, the arrows are a bounded sample.
Cost
One extra Gemini call and roughly 180 extra file reads per scan. The
installation token is now cached, which is what makes that affordable —
previously every file read minted a fresh one.
Notes
Verified against a real repository. The module-folding change landed after that
run and hasn't been seen live yet. If import resolution misses for a repo's
language or alias style you get boxes with no arrows and no error — the
scan.run completelog reportsimportEdges, which is how to spot it.