ci: enforce that every workspace declares what it imports - #423
Merged
Conversation
apps/web imported @xenova/transformers and @mlc-ai/web-llm directly while declaring neither. They resolved only because shamefullyHoist flattens ai-core's optionalDependencies into the root node_modules -- the same latent breakage that #413 fixed for onnxruntime-web. Both now go through ai-core's lazy loaders (loadTransformers, loadWebLlm), and the module types derive from those loaders' return types instead of a direct `typeof import`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tauri-apps/plugin-dialog and @tauri-apps/plugin-notification were declared by apps/desktop and imported by apps/web, which resolved through hoisting alone. Declared as optionalDependencies, matching @tauri-apps/api: both are reached only through dynamic imports guarded by a Tauri-context check, so a plain web install must not fail without them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
predictYield() dynamically imported @tensorflow/tfjs once a grower had 10 or more recorded harvests. @tensorflow/tfjs is declared in no package.json in this repo and is not installed, so that import could only ever reject: the feature threw for exactly the users who had used the app the longest. Nothing caught it. The typecheck was blinded by an ambient `declare module '@tensorflow/tfjs'` in optional-deps.d.ts, the tests only exercise the zero- sample path, and the import is dynamic so the bundler never resolved it. The path is removed rather than repaired: making it work meant shipping a multi-megabyte runtime to train a three-layer network on a handful of samples. predictYield() is now heuristic throughout -- which is what every user was already getting, minus the exception. Recorded harvests still raise confidence. Removes: the model code, the ambient declaration (and its 11 no-explicit-any suppressions), the `usedTensorflowModel` field, the vite external, and the UI branch plus i18n strings that promised a local model, across all five locales. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shamefullyHoist flattens transitive dependencies into the root node_modules, so an undeclared import resolves anyway -- until the package that dragged it in drops it, and then it breaks at runtime with nothing in the diff to explain why. @sentry/browser reached that state in #413; this pass found five more. check:phantom-deps walks each workspace, extracts the external imports from its sources, and checks them against that workspace's own package.json rather than against whatever happens to sit in node_modules. Comments are stripped first, so a commented-out import or a JSDoc `@type {import('x')}` is not a dependency, and specifiers are validated so a regex that wandered across a line boundary reports nothing instead of a nonsense package name. It also flags runtime code importing a devDependency, which hoisting hides just as effectively. Runs in the quality job before the test suite: it is a sub-second static check and should fail early. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTensorFlow-based yield prediction was removed in favor of heuristic projections. Local ML modules now load through ChangesML prediction and UI
Centralized ML loading
Declared-dependency enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant PhantomDependencyChecker
participant WorkspaceSources
participant PackageManifests
CI->>PhantomDependencyChecker: run check:phantom-deps
PhantomDependencyChecker->>WorkspaceSources: collect source imports
PhantomDependencyChecker->>PackageManifests: read declared dependencies
PhantomDependencyChecker-->>CI: report phantom or misplaced imports
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Docker |  |
Jul 14, 2026 12:10p.m. | Review ↗ |
| JavaScript | |
Jul 14, 2026 12:10p.m. | Review ↗ |
| Python | |
Jul 14, 2026 12:10p.m. | Review ↗ |
| Rust | |
Jul 14, 2026 12:10p.m. | Review ↗ |
| Shell | |
Jul 14, 2026 12:10p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
#414 split the quality job into build/test/verify. The declared-dependency gate moves into `verify`, next to the service-dependency acyclic check -- both are dependency-graph gates, and verify is where the other static checks live. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/locales/es/plants.ts`:
- Around line 617-618: Update the mlForecast translation to a standalone
forecast label in all affected locale files: apps/web/locales/es/plants.ts lines
617-618 use “Pronostico”, apps/web/locales/fr/plants.ts lines 619-620 use
“Prevision”, and apps/web/locales/nl/plants.ts lines 603-604 use “Voorspelling”;
leave the confidence translations unchanged because the consumer appends them
separately.
In `@apps/web/services/yieldPredictionService.ts`:
- Around line 148-158: Update the sample-count logic in the yield prediction
flow around collectTrainingSamples so confidence and the explanation use the
number of actual harvested plants, not historical snapshots or the final plant
vector. Preserve the zero-harvest fallback, and add a regression test covering
one harvested plant with multiple history entries to ensure it is counted once
and reported as one recorded harvest.
- Around line 155-158: Update the prediction explanation branch in
yieldPredictionService to resolve both messages through the centralized getT
mechanism from apps/web/i18n.ts instead of returning inline English strings. Add
stable corresponding translation keys to the EN, DE, ES, FR, and NL locale
resources, preserving the sampleCount-specific wording and interpolation of the
recorded harvest count.
In `@scripts/check-phantom-deps.mjs`:
- Around line 129-133: Update the manifest-loading try/catch in the workspace
discovery flow to ignore only a genuinely missing package.json. Re-throw
malformed JSON and other I/O errors instead of treating them as non-workspaces,
and replace the empty catch behavior with explicit error discrimination while
preserving the existing packages.push path.
- Line 76: Update the import parsing regex and its consumers in the
dependency-checking flow to capture whether each import is type-only, rather
than discarding the type marker. In the runtime dependency validation around the
import handling at lines 163-165, skip checks for type-only imports while
preserving existing validation for runtime imports.
- Around line 87-99: Update stripComments and the importedPackages scanning flow
to remove or mask quoted strings and template literals, including their
contents, before applying IMPORT_PATTERNS. Preserve real import detection while
ensuring import-like text inside single-quoted, double-quoted, or backtick
literals is ignored; use a lexer/parser approach if needed for correct escaping
and multiline handling.
- Around line 66-68: Normalize import specifiers by removing the query portion
before applying BARE_SPECIFIER.test(), ensuring query-bearing imports reach
packageNameOf() and are checked like bare package imports. Preserve the existing
package-name extraction behavior for scoped and unscoped specifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ae73780-7a31-4951-992b-7a90d97b24f8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
.github/workflows/ci.ymlapps/web/components/views/plants/GrowStatsDashboard.tsxapps/web/locales/de/plants.tsapps/web/locales/de/settings.tsapps/web/locales/en/plants.tsapps/web/locales/en/settings.tsapps/web/locales/es/plants.tsapps/web/locales/es/settings.tsapps/web/locales/fr/plants.tsapps/web/locales/fr/settings.tsapps/web/locales/nl/plants.tsapps/web/locales/nl/settings.tsapps/web/package.jsonapps/web/services/local-ai/models/modelLoader.tsapps/web/services/local-ai/models/webLlmService.tsapps/web/services/local-ai/nlp/whisperService.tsapps/web/services/yieldPredictionService.test.tsapps/web/services/yieldPredictionService.tsapps/web/types/optional-deps.d.tsapps/web/vite.config.tsapps/web/workers/imageGeneration.worker.tsapps/web/workers/inference.worker.tspackage.jsonpackages/ai-core/src/domain/diagnosis.tsscripts/check-phantom-deps.mjs
💤 Files with no reviewable changes (3)
- apps/web/vite.config.ts
- packages/ai-core/src/domain/diagnosis.ts
- apps/web/components/views/plants/GrowStatsDashboard.tsx
qnbs
marked this pull request as ready for review
July 14, 2026 09:49
check-phantom-deps:
- Vite query suffixes (`import('pkg?worker')`) were rejected by the
bare-specifier check before packageNameOf() could strip them, so those imports
were never checked at all. Strip the query first. A fixture confirms
`undeclared-worker?worker` is now caught.
- Type-only imports were indistinguishable from value imports, so
`import type { X } from 'some-dev-dep'` in runtime code was reported as
reaching into a devDependency. Types are erased at compile time, so that is
legitimate. The two kinds are now tracked separately; a type-only import must
still be declared, but may be declared as a devDependency.
- stripComments() left string and template content, so `const s = "import('x')"`
could be counted as a dependency. The leading guard now also rejects a match
that begins right after a quote or backtick.
- A package.json that exists but fails to parse was swallowed by a bare catch,
which would silently disable the check for that workspace. It now exits 1 and
says which file. A missing package.json still just means "not a workspace".
check-file-budget:
- The changed-file path ignored SCAN_GLOBS entirely, so touching any long file
outside application source failed the gate. A locale dictionary is ~1000 lines
by nature and no amount of splitting changes that; the budget was never meant
to police it. The changed-file path now honours the same scope as the full
scan, and "diff touched nothing in scope" no longer escalates to a full scan.
yieldPredictionService:
- collectTrainingSamples() emits one sample per history *snapshot* of each
harvested plant, not one per harvest, so "recorded harvests" was wrong. Say
samples, which is what the UI already calls them.
- The explanation was a hardcoded English string on a user-facing field. Routed
through getT(), like chemotypeService, with keys in all five locales.
GrowStatsDashboard:
- confidence is a 0..1 fraction, and `confidence.toFixed(0)` rendered a flat
"0%" for every prediction the app has ever made. Scale by 100 first.
- `mlForecast` + `confidence` rendered as "Forecast confidence 45% confidence".
mlForecast is now the leading noun, so the sentence reads "Forecast 45%
confidence".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
DeepSource JS-0116: single-letter `t`. `t` is the i18next convention and reads fine, but the rule is enforced and a suppression would spend the ratchet on nothing. `translate` costs nothing either. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/components/views/plants/GrowStatsDashboard.tsx`:
- Around line 149-150: Update the comment immediately before the confidence
conversion in the GrowStatsDashboard rendering logic to accurately describe the
issue with rounding a 0–1 confidence fraction using toFixed(0), without claiming
all predictions rendered 0%. Leave the conversion on the following line
unchanged.
In `@apps/web/locales/es/plants.ts`:
- Around line 617-626: Update the new Spanish strings in the translation entries
around mlForecast, heuristicFallback, modelTraining, modelIdle,
explanationNoHistory, and explanationHeuristic to preserve all required
diacritics using ASCII-compatible Unicode escapes such as \u00f3 and \u00fa.
Keep the source file ASCII-only while ensuring the rendered text displays the
correctly accented Spanish words.
In `@scripts/check-file-budget.mjs`:
- Around line 103-105: Update the diff handling around gitDiffFiles so the raw
changed-file list is retained separately from the filtered budget-file list. Use
the raw diff’s non-empty status to choose between scanning the filtered changed
files and allTrackedInScanDirs(), ensuring out-of-scope-only changes do not
trigger a full scan.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e873cf34-984f-49ad-90f2-c75bd96aa683
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.github/workflows/ci.ymlapps/web/components/views/plants/GrowStatsDashboard.tsxapps/web/locales/de/plants.tsapps/web/locales/en/plants.tsapps/web/locales/es/plants.tsapps/web/locales/fr/plants.tsapps/web/locales/nl/plants.tsapps/web/services/yieldPredictionService.tspackage.jsonscripts/check-file-budget.mjsscripts/check-phantom-deps.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- package.json
- apps/web/locales/fr/plants.ts
- apps/web/locales/en/plants.ts
- apps/web/locales/de/plants.ts
- apps/web/services/yieldPredictionService.ts
- scripts/check-phantom-deps.mjs
check-file-budget: the fallback decision was still wrong. gitDiffFiles() already filters to ts/tsx/mjs and drops tests, so a branch touching only package.json, a workflow, docs or tests produced an empty list -- which then escalated to a full scan, the exact opposite of "out of scope". Split the raw diff (does this branch change anything?) from the budget-eligible subset (what can the budget apply to?). Locales: the new es/fr/de strings shipped without diacritics. These files encode accents as ASCII \uXXXX escapes -- ó appears 70 times in es alone, ä 58 times in de -- so "Pronostico" and "Schaetzung" were simply misspelt. Escaped properly. (The accent-free strings already in those files predate this branch and are left alone; correcting them belongs in its own pass.) GrowStatsDashboard: the comment overstated the bug. toFixed(0) on a 0..1 fraction rounds to a single digit, so predictions rendered as "0%" or "1%" -- not "0%" for every prediction. The fix is unchanged; the comment now says what actually happened. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DeepSource JS-R1005: GrowStatsDashboardComponent sits at a cyclomatic complexity of 8. The forecast copy carries three of those branches on its own -- prediction or not, loading or not, explanation or fallback -- and none of them are about the dashboard. Moved into YieldForecastCard, which owns them. No behaviour change; the parent just stops paying for branches that belong to one of its cards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
What
Adds
check:phantom-deps, a CI gate that fails when a workspace imports a package it does not declare — and fixes every violation it found.Why
pnpm-workspace.yamlsetsshamefullyHoist: true, which flattens transitive dependencies into the rootnode_modules. An undeclared import therefore still resolves — until the package that dragged it in drops it, or hoisting is turned off. Then it breaks at runtime, with nothing in the diff to explain why.This is not hypothetical. In #413,
@sentry/browserwas removed fromapps/webby a dependency bump while five source files still imported it directly. It kept working purely because@sentry/reacthappened to pull it in. CodeRabbit flagged the same class of risk onshamefullyHoistin that PR; this is the guard it asked for.Running it against
mainfound five more.What the guard found
@xenova/transformersapps/web(4 files)packages/ai-core(optional)ai-core'sloadTransformers()@mlc-ai/web-llmapps/webpackages/ai-core(optional)ai-core'sloadWebLlm()@tauri-apps/plugin-dialogapps/webapps/desktopoptionalDependencyofapps/web@tauri-apps/plugin-notificationapps/webapps/desktopoptionalDependencyofapps/web@tensorflow/tfjsapps/webThe TensorFlow finding
predictYield()dynamically imported@tensorflow/tfjsonce a grower had 10 or more recorded harvests. That package is declared in nopackage.jsonin this repo and is not installed, so the import could only ever reject.The feature threw for exactly the users who had used the app the longest.
Three layers of defence missed it:
declare module '@tensorflow/tfjs'inoptional-deps.d.ts,The path is removed rather than repaired: making it work meant shipping a multi-megabyte runtime to train a three-layer network on a handful of samples.
predictYield()is now heuristic throughout — which is what every user was already getting, minus the exception. Recorded harvests still raise confidence.This also deletes the ambient declaration and its 11
no-explicit-anysuppressions.How the guard works
For each workspace it extracts external imports from the sources and checks them against that workspace's own
package.json— never against whatever happens to sit innode_modules.Precision measures, since a noisy gate gets disabled:
@type {import('x')}is not counted as a dependency;exportsare ignored;devDependencies; runtime files may not — hoisting hides that just as effectively.It runs in the
qualityjob before the test suite: it is a sub-second static check and should fail early.Verification
pnpm run check:phantom-deps→ OK, zero findings, zero false positives.turbo run typecheck→ 5/5 green.yieldPredictionServicetests → green.🤖 Generated with Claude Code
Summary by CodeRabbit