feat: Architecture Review action V1#1
Open
brovatten wants to merge 24 commits into
Open
Conversation
Architecture review · 3 components changed
Compared against codeboarding-action · run 26436608937 |
Replace the server-mediated docs-generation flow with a self-contained PR-diff-comment flow that runs analysis in the user's GH runner and posts a sticky comment with a screenshot of the architecture diagram — components colored green/yellow/red for added/modified/removed. - New action.yml composite: checks out engine + webview-ui, runs incremental analysis seeded from PR base SHA, diffs against the freshly-generated head analysis, renders PNG via Playwright + bundled webview UI, pushes to orphan branch, posts sticky comment. - scripts/compute_diff.py: vendored port of the wrapper's tree_diff/loader logic (pure stdlib, no pydantic). - scripts/render_diagram.mjs: Playwright + static server + postMessage injection (matches the webview's e2e wire format). - README: usage, limitations, TODOs for upstream refactors that would let us drop the vendored bits. - example-usage.yml: replaced old daily-cron flow with a PR self-test that exercises the local action via uses: ./
The vscode repo isn't accessible from arbitrary GH Actions runners. Bundle the prebuilt dist (~2MB) directly. README updated to drop the vscode_ref input; rebuilding the bundle is a manual step until the webview-ui ships as a public release asset (tracked in TODOs).
brovatten
force-pushed
the
feat/architecture-review-v1
branch
from
1f0228c to
cba8719
Compare
generate_analysis() re-clones the repo from URL — the fresh clone only has 'main' as a local head, so checkout_repo fails on PR feature branches. Switch to run_incremental which takes the already-checked-out target-repo path directly. Bonus: kills the redundant clone. Also: seed static_analysis.pkl from base SHA if present (avoids IncrementalCacheMissingError on the first incremental run), and fail loud-and-early when OPENROUTER_API_KEY is empty.
Pasting a key into GitHub's secret UI often picks up a trailing newline. That breaks OpenRouter's Bearer auth (header value gets the newline). Strip whitespace defensively so the secret-as-pasted UX just works.
OpenRouter keys are 73 chars (sk-or-v1- + 64 hex). User's key was 75 chars after whitespace strip, suggesting wrap-around quotes or a leftover env-line prefix. Strip both forms and surface a prefix-OK signal in the log so we can see at a glance whether the value looks like an OpenRouter key.
Tiny repos / legacy pkls produce no cluster baseline and incremental can't proceed. Catch the exception and re-run the full pipeline so we still get an analysis.json to diff against the base.
Old position evaluated steps.base.outputs.found before the seed step ran, so the condition was always false and Playwright was never installed. Render then failed with ERR_MODULE_NOT_FOUND.
…s it Previously: addInitScript-defined acquireVsCodeApi suppressed __BROWSER_DEV__, then postMessage'd analysis-loaded manually. Some load-order or stub-shape interaction broke React mount with 'Cannot read properties of undefined (reading getState)'. Now: let the index.html inline stub run normally (sets __BROWSER_DEV__); the browser-dev-mock fetches /sample-analysis.json which we serve as our real analysis. Same boot path as Vite dev mode — known-working.
CSS injection right before the screenshot hides:
- welcome overlay (the 'Analyze my codebase' modal that covered the graph)
- demo banner ('Viewing Demo')
- help-tour buttons (bottom-left)
- analysis/view controls (top-right)
- commit picker / timeline (top-left)
- branch-state / outdated banners
Locally-verified the resulting PNG is just the diff diagram on a dark
background — no chrome.
Compute the union bounding box of all .react-flow__node elements via getBoundingClientRect() in page.evaluate(), pad 80 CSS px, clamp to viewport, and pass as Playwright clip. Result is a tightly-framed diagram with no empty canvas. Locally-verified: 1470x703 CSS px for the 3-node test case (vs the prior 1600x1000 full viewport). Fallback paths preserved: empty bounds -> full .react-flow shot; no-nodes-rendered -> #root/body shot (debug diagnostic mode).
- 'Compared against `main`' instead of the short SHA - '1 component changed' vs '5 components changed' (no '(s)') - 'no architectural changes' when N=0
Run engine's health checks against the just-persisted static analysis (via StaticAnalysisCache + run_health_checks from core), count warning + critical findings, emit /tmp/health.json. The comment-build step appends 'N code-health warning(s) detected — see more at codeboarding.org' when any are found. Silent when count is zero.
New format:⚠️ **6 architecture warnings detected.** *(e.g., circular dependency in `auth_service`, oversized function in `X`)* [ View full trace & diagnostic logs ](https://codeboarding.org/pr/123) Health-check step now captures up to 2 sample findings (check name + entity short-name) and persists them alongside the counts in /tmp/health.json. Comment-build composes the multi-line footer using those examples and links to codeboarding.org/pr/<num>.
Previously: '.split('.')[-1]' yielded '<function>' for Python entities
whose qualified name ended in a placeholder (<function>, <lambda>,
<module>). Now: filter out angle-bracket segments, fall back to the
file basename, and prefer last-2 segments when the leaf is generic
(main, __init__, run). Also: walk all entities per group (not just
the first) so we collect 2 distinct examples even when 6 warnings
come from one check.
Bundled, minified third-party JS bundle — its analysis produces noisy 'high fan-out in `fOe`' style warnings on minified identifiers that no one can act on. Excluding it from analysis.
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.
Summary
Rewrites this action from a server-mediated docs generator into a self-contained PR-diff visualizer. On every PR, it posts a sticky comment with a screenshot of the architecture diagram, with components colored green/yellow/red (added/modified/removed).
Architecture:
.codeboarding/analysis.jsonfrom git history, runs incremental analysis on PR head (cheap — LLM only for components whose code changed), diffs the two.CodeBoarding-vscodewebview UI (bundled by cloning + building at action time).codeboarding-images, posts sticky PR comment viamarocchino/sticky-pull-request-comment.What's in the PR
action.yml— composite action with steps for checkout, deps, analysis, diff, render, upload, comment.scripts/compute_diff.py— vendored port ofCodeBoarding-wrapper'sdiff_component_trees(pure stdlib, no pydantic). README documents this as a TODO to move into core.scripts/render_diagram.mjs— Playwright script; matches the webview's e2epostMessagewire format.README.md— usage, limitations, TODOs..github/workflows/example-usage.yml— replaced old cron-based docs flow with a PR self-test usinguses: ./.Test plan
.github/workflows/example-usage.ymlon this PR..codeboarding/analysis.json, expect the "no baseline yet" path: action runs, computes nothing, posts the cold-start comment. That's the V1 success criterion for the self-test.Limitations called out in the README
Out of scope for V1 / blocked on upstream
These need changes outside
CodeBoarding-action(see README "TODOs" section):diff_component_treesfrom wrapper to core.analyze-this-checkoutentry point in core (avoid the re-clone)./renderendpoint (would remove orphan-branch + fork-PR limitations).