Fast Track: mock webapp dist in backend/coverage guardrail - #1767
Conversation
The backend/coverage guardrail runs pytest directly, whose conftest imports the full app. The app's webapp route raises at import time unless the built frontend dir (dist_lightly_studio_view_app) exists. The guardrails CI never builds the frontend, so backend PRs crashed the guardrail with an ImportError instead of producing a verdict. Mock the dist (empty dir + index.html) before invoking pytest, mirroring the Makefile's mock-webapp-dist target. The helper is idempotent and never overwrites an existing index.html, so local dev builds are preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Fast Track Guardrails workflow conditionally builds the ChangesBackend guardrail preparation
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![lightly-fast-track-bot[bot]](https://avatars.githubusercontent.com/in/4182859?s=60&v=4){kind=small}
✅ Fast Track: all required checks passed — auto-approved.
To run the guardrails locally, from Reflects |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42f7a64a0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@fast_track/src/guardrails/backend/coverage.ts`:
- Around line 147-153: Update ensureWebappDist() to create index.html atomically
with writeFileSync using the exclusive-create flag ('wx'), rather than checking
with existsSync before writing. Catch and ignore only EEXIST errors so
concurrent or pre-existing files remain untouched, while propagating other
filesystem errors.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e93c5c7-3019-4aa3-8cbd-ca2e30b2a574
📒 Files selected for processing (2)
fast_track/src/guardrails/backend/coverage.test.tsfast_track/src/guardrails/backend/coverage.ts
Superseded by a newer Fast Track approval.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lightly_studio/src/lightly_studio/dataset/embedding_manager.py`:
- Line 3: Remove the temporary validation comment marked “revert me” at the top
of the module, leaving the surrounding implementation unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bbe8831-767c-47ed-8790-ab51c9ee9568
📒 Files selected for processing (1)
lightly_studio/src/lightly_studio/dataset/embedding_manager.py
This reverts commit 0712b04.
Superseded by a newer Fast Track approval.
|
Validated the fix in real CI by temporarily adding a backend change (since this PR only touches
Successful run: https://github.com/lightly-ai/lightly-studio/actions/runs/30092838147 This is the same guardrail that previously threw on #1753 (#1753 (comment)). The temporary backend change has been reverted, so the PR diff contains only the |
- Create index.html with the exclusive-create flag ('wx') so a real build
written concurrently is never truncated (CodeRabbit).
- ensureWebappDist now returns a cleanup that removes only what it created;
runTests calls it in a finally so a local guardrail run in an unbuilt
checkout doesn't leave an empty dist that masks the app's "did you forget
to build the webapp?" error (Codex).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Superseded by a newer Fast Track approval.
Superseded by a newer Fast Track approval.
This reverts commit 45a5b3f.
Superseded by a newer Fast Track approval.
|
/review |
Per team discussion, build the real webapp in the guardrails CI for backend PRs rather than mocking the dist inside backend/coverage. The app's webapp route raises at import time unless dist_lightly_studio_view_app exists, so any backend guardrail that imports the app (backend/coverage runs pytest, whose conftest imports the full app) previously crashed. build-lightly_studio_view exports the schema/version, builds the Svelte app, and copies the dist into the backend package — the same real build test_and_release produces. This is a shared, reusable setup for any backend guardrail, so it lives in the workflow rather than in one guardrail's code. Reverts the TS mock (ensureWebappDist) added earlier on this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Superseded by a newer Fast Track approval.
This reverts commit 9c305d9.
Superseded by a newer Fast Track approval.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/fast_track_guardrails.yml:
- Around line 103-106: Update the “Build webapp for backend guardrails” workflow
step so frontend dependencies are installed before make
build-lightly_studio_view runs, including backend-only and backend-plus-frontend
executions. Move or reuse the existing npm ci setup with a condition covering
every backend build, while preserving the current frontend dependency behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d245633d-6ead-47dc-808f-42f9d4593c3d
📒 Files selected for processing (1)
.github/workflows/fast_track_guardrails.yml
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Superseded by a newer Fast Track approval.
2 participants
What
The
backend/coverageguardrail runspytestdirectly, whoseconftest.pyimports the full app. The app's webapp route (api/routes/webapp.py:18-19) raises aRuntimeErrorat import time unless the built frontend dirdist_lightly_studio_view_appexists.The Fast Track guardrails CI never builds the frontend, so on backend PRs the guardrail crashed with an
ImportErrorbefore collecting any tests — surfacing asGuardrail threw: ...instead of a real pass/fail verdict.Observed on PR #1753: #1753 (comment)
Fix
Add an idempotent
ensureWebappDist()helper, called at the start ofrunTests, that creates the dist dir + an emptyindex.htmlbefore invoking pytest — mirroring the Makefile'smock-webapp-disttarget already used for migration checks. It:index.html, so a real build on a dev machine is preserved,webapp.py's import-time guard remains a valid production check.The mock is a per-run throwaway: the dir is gitignored and not cached, so every CI run recreates and discards it; it doesn't leak into other jobs.
Testing
ensureWebappDist(creates dir + index when missing; does not overwrite an existing index).runTeststests updated for the addedexistsSynccall.npm run static-checksand fullnpm test(204 tests) pass.🤖 Generated with Claude Code
Summary by CodeRabbit