Repository navigation
chore: hooks, CI, npm toolchain and a dead-code sweep - #13
Merged
Merged
Conversation
- build.yml pushed to GCP Artifact Registry on bh-main/bh-dev, branches this fork does not have, so it never ran; workflow_dispatch would have run and failed - the governance doc and script describe a CODEOWNERS gate whose owners file and workflow are already gone - desgin/ was a misspelled directory carried since the initial commit - .gitignore now covers keys, so the untracked oan/oan.pub keypair in the working tree cannot be committed by accident Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pre-commit gains knip and the test run. All four finish in a few seconds, so the hook stays usable; build stays out of it. commit-msg is new. It checks the subject against the convention rather than leaving it described in a file nobody opens. The issue number is encouraged but not required — requiring it blocks commits that have no tracker issue, and what people write then is [#0]. Merge, revert and fixup subjects are skipped, since their messages are generated. Three things had to be fixed to make knip a gate rather than noise: - it was never a declared dependency; it had been run through bunx, so `bun run knip` could not find it - src/routes.ts is loaded by vite.config.ts as a string path, invisible to static analysis, so knip called it and @tanstack/virtual-file-routes dead. It is now declared as an entry point - knip does not follow CSS imports, so tailwindcss, tw-animate-css and the typography plugin looked unused despite being imported by global.css The gate covers dead files and dependency drift. Unused exports are reported by `bunx knip` but not gated: src/components/ui is a vendored component kit kept as a whole API, and 35 of its exports would fail every run. Both hooks now set -e themselves. Husky's shim runs them with `sh -e`, so this changed nothing under git — but run directly the script executed every check and returned only the last one's status, which is how a failing lint could have looked like a pass. README says how to check whether hooks are active at all. core.hooksPath is local to a clone and is not committed, so anyone who installed dependencies before a hook existed has none and is not told. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three workflows, matching the shape decision-support-system and knowledge-provider already use, so a reader moving between OAN repos finds the same layout. - ci.yml: install (frozen), lint, typecheck, knip, test with coverage, build - security.yml: Trivy scan of bun.lock, gating on fixable HIGH/CRITICAL. Passes clean today - build-and-push.yml: the same checks, then an amd64 image, a report-only Trivy image scan, and a push to ghcr.io/openagrinet/experience-ui. PRs build but never publish Two checks the sibling repos have no equivalent for, both for things that have already cost this repo: - the route tree must be current. The Docker build cannot regenerate it, because oven/bun:1-alpine has no Node for the TanStack Router plugin, so a stale committed copy would ship a wrong route tree silently - a bundle budget: 6 MB for dist, 2 MB for any single file. This client shipped an 8.66 MB SVG to every visitor for its whole life upstream, and a budget is what would have caught it on the commit that added it. dist is 3.3 MB today, largest asset 832 KB Coverage is printed, not uploaded. No threshold: the suite is small, and a number invented now would either be trivially met or fail on the first honest commit. The image scan is report-only on purpose. A base-image CVE with no fix available should not block a deploy; security.yml is where an actionable finding stops things. Verified locally: both guards pass, and the Trivy gate reports zero vulnerabilities against bun.lock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches the layout decision-support-system and knowledge-provider already use, with CLAUDE.md a symlink to AGENTS.md, so someone moving between OAN repos finds the same two files. Both are adapted rather than copied. The cross-repo rules carry over — git workflow, commit format, PR shape, writing style. Everything Python is replaced: ruff becomes eslint and knip, the pytest tiers become the much smaller Vitest story, and the hexagonal layout becomes this repo's actual folder structure. The gotchas section is the part worth reading. Every entry in it has already cost time here: routes.ts being invisible to static analysis, the dynamic import in main.tsx that configuration ordering depends on, Vite refusing JS imports from public/, विस्तार meaning "area" so brand find-and-replace corrupts copy, and npm being unable to install this tree at all. Two sections are deliberately absent. There is no versioning section: package.json says 0.0.0, the settings drawer hardcodes 1.0.0, and a convention describing neither would be fiction. Formatting is documented as not enforced rather than quietly implied, since Prettier is installed but unconfigured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Split across ci.yml, security.yml and build-and-push.yml, all three fired
simultaneously on every push, and build-and-push.yml re-ran the lint,
typecheck, knip and test steps ci.yml had already run. That duplication
came from copying the sibling repos' layout without questioning it.
Now one workflow with a job graph:
check ──┬──→ build ──┬──→ image
└──→ security ┘
check is the fastest failure signal, so everything waits on it: a broken
type no longer costs a Docker build. build and security run alongside each
other because a vulnerable dependency and an oversized bundle are
independent failures and both are worth seeing. Nothing is published until
all of it passes.
This diverges from decision-support-system and knowledge-provider, which
keep three separate workflows and carry the same duplicate test job. The
job graph seemed worth more than matching a layout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pull request runs check, build and security. `bun run build` is what catches a build error there, and building a container image the run will never publish costs a couple of minutes on every push to every branch. The job-level condition replaces the two per-step guards on the login and push steps, which were the only thing stopping a PR from publishing. Gating the job says the same thing in one place, and makes it harder to add a publishing step later that forgets its own guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Nine commits covering how this repo is built, checked and documented. It grew past its original scope: a container build failure turned into a toolchain migration, and a review for unused code turned up a fair amount.
Toolchain: Bun → Node and npm
Bun was chosen because npm could not install this tree at all, crashing with
Cannot read properties of null (reading 'edgesOut').That was one dependency, not the tree. npm installs cleanly with vitest removed and crashes in
#loadPeerSetwith it present — vitest 4 declares a large optional-peer set npm's arborist cannot walk. Vitest 5 does not have the problem, so the fix was an upgrade rather than a workaround.What forced the question was the container build failing with
Cannot find package 'tsx:'.vite.config.tspassesvirtualRouteConfigas a path, so the router generator loads that TypeScript file throughtsx— a Node loader. The fix on Bun was to install Node beside it: two runtimes to work around one plugin. It also failed only intermittently, depending on whether the generator's cache re-ran, so the build worked on one machine and not the next.Nothing runs a JavaScript runtime in production — the artifact is static files behind nginx — so this is a build-time choice only.
Two things npm surfaced that Bun had absorbed silently:
@vitest/coverage-v8was pinned to4.0.17againstvitest@^4.1.11, a real conflict2.1.0-beta.1–4.1.10. Downgrading would have added a vulnerabilityNo
--legacy-peer-depsanywhere: it hides exactly this kind of conflict.CI: one workflow, chained jobs
Split across three files they all fired at once, and the image workflow re-ran every check the first had already run. That duplication came from copying the sibling repos' layout without questioning it; DSS and knowledge-provider both carry it.
A pull request stops after
build.npm run buildcatches build errors; the image is only built where it is published. The job-level condition replaces two per-step guards that were the only thing stopping a PR from publishing.Two checks the siblings have no equivalent for:
src/routes.tsdistand 2 MB per file. This client shipped an 8.66 MB SVG to every visitor for its whole life upstream.distis 3.3 MB todayCoverage is printed, not uploaded. No threshold — the suite is small and an invented number would be trivially met or fail on the first honest commit.
Hooks
pre-commitruns lint, typecheck, knip and tests.commit-msgenforces<type>: <summary> [#<issue-no>], with the issue number optional — requiring it blocks commits with no tracker issue, and what people write then is[#0].Three things had to be fixed before knip could gate rather than nag: it was never a declared dependency;
src/routes.tsis invisible to static analysis so knip called it and@tanstack/virtual-file-routesdead; and knip does not follow CSS imports, so three Tailwind packages looked unused.Both hooks now
set -ethemselves. Husky's shim runs them withsh -e, so nothing changes under git — but run directly, the script executed every check and returned only the last one's status. A failing lint could have looked like a pass.Dead code removed
A whole-graph sweep. Unused exports 35 → 20, unused types 7 → 0; the 20 remaining are all vendored shadcn kept as a complete API, which tree-shaking drops from the bundle anyway.
audio-utils.tswas dead — no callers, and removing it orphaned four private helpers and an interface in turn. 374 lines → 93. Playback survivesexport const ChatScreenQueries = {} as constwith a TODO, behind two barrels and a misspelledcomponets/directory. ThequeryClientstays; it is where the first thing needing caching plugs inOnRequestErrorwas a React component returning an empty<div>, used as aMutationCacheonError callback. Only the logging ever had an effectgetBrowserInfo,getFingerprintIdand@fingerprintjs/fingerprintjsConfigProvider's React context had no consumer — everything readsgetConfig()directlyCleanup
build.ymlpushed to GCP Artifact Registry on branches this fork does not have, so it never ran. The governance doc and script described aCODEOWNERSgate whose owners file and workflow are already gone.desgin/was a misspelled directory carried since the initial commit..gitignorenow covers keys, so the untrackedoan/oan.pubkeypair cannot be committed by accident.Documentation
AGENTS.mdwithCLAUDE.mdsymlinked, andCONVENTIONS.md— the same layout as the sibling repos, adapted rather than copied. The gotchas section is the part worth reading; every entry has already cost time here.No versioning section:
package.jsonsays0.0.0, the settings drawer hardcodes1.0.0, and documenting either would be fiction. Formatting is recorded as not enforced rather than implied, since Prettier is installed but unconfigured.Verified
Clean worktree at the head commit with
npm ci: lint, typecheck, knip, 23 tests, coverage, build, route-tree guard, bundle budget. The image builds--no-cacheonnode:22-alpineand serves — routes fall back, missing files 404, config isno-store, and the app renders under a strict CSP.Two still open
Both about future commits rather than tooling:
CONVENTIONS.mdsays nothing either way rather than deciding it silently🤖 Generated with Claude Code