UN-3386 [FEAT] Add Prompt Studio HITL change indicator plugin slot#1930
Conversation
Wires up the host-side hooks for the prompt-change-indicator plugin (implementation lives in unstract-cloud): a dynamic-import slot in the prompt card Header for the indicator button, and a route at :orgName/review/readonly/:documentId for the read-only audit view. Both gates fall through gracefully when the plugin is absent (OSS). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughHeader and routing now optionally load a prompt-change plugin via cloud-only dynamic imports: Header conditionally renders a PromptChangeIndicator, and the route tree conditionally registers a read-only review page under ReviewLayout when the plugin is available. Changes
Sequence Diagram(s)sequenceDiagram
participant AppRouter as App Router
participant Importer as Dynamic Importer
participant Plugin as ReadOnlyReviewPlugin
participant ReviewLayout as ReviewLayout
AppRouter->>Importer: attempt import "prompt-change-indicator"
alt import succeeds
Importer->>Plugin: load module
Plugin-->>Importer: module exported
Importer-->>AppRouter: register route `review/readonly/:documentId`
AppRouter->>ReviewLayout: mount nested route when visited
ReviewLayout->>Plugin: render ReadOnlyReviewPage
else module missing
Importer-->>AppRouter: swallow missing-plugin error (no route registered)
else unexpected error
Importer-->>AppRouter: console.error and do not register route
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
| Filename | Overview |
|---|---|
| frontend/src/components/custom-tools/prompt-card/Header.jsx | Adds a dynamic-import plugin slot for PromptChangeIndicator, rendered between ExpandCardBtn and PromptRunBtnSps. Error handling in the catch block is silent (consistent with other plugins in this file but less thorough than the paired ReadOnlyReviewPage import). |
| frontend/src/routes/useMainAppRoutes.js | Adds ReadOnlyReviewPage dynamic import with proper MODULE_NOT_FOUND detection and console.error for unexpected failures, a console.warn guard when the plugin loads without ReviewLayout, and the route nested correctly inside the existing ReviewLayout/ManualReviewPage block. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
frontend/src/components/custom-tools/prompt-card/Header.jsx:44-52
**Silent catch swallows real plugin errors**
The `PromptChangeIndicator` import catches every error silently, including syntax errors and runtime failures inside the plugin. The companion `ReadOnlyReviewPage` import in `useMainAppRoutes.js` was improved (after a prior review round) to distinguish `MODULE_NOT_FOUND` (expected in OSS) from real failures and emit a `console.error` for the latter. If the cloud plugin ships with a broken `PromptChangeIndicator.jsx`, the button will simply never appear and there will be no console signal — making the failure invisible to the developer.
Reviews (4): Last reviewed commit: "Merge branch 'main' into UN-3386-prompt-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses review feedback: the readonly route nests inside ReviewLayout (manual-review plugin), so a deployment that ships prompt-change-indicator without manual-review would silently fail to register the route. Log a console.warn in that case to make the misconfiguration discoverable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bare catch in the prompt-change-indicator dynamic import was swallowing syntax/runtime errors in the plugin file alongside the expected "plugin missing in OSS" case. Detect the missing-module messages explicitly and console.error anything else so a broken cloud plugin no longer disables the readonly route silently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/routes/useMainAppRoutes.js (1)
115-136: Module-missing heuristic risks false positives and Header.jsx has inconsistent error handling.The
"Failed to fetch dynamically imported module"message is emitted by Vite for both genuinely missing modules and transitive dependency failures (e.g., chunk 404, network blips, sub-import typos). This means the current check silently swallows errors that should be surfaced — the exact failure mode the improved error handling was meant to prevent. Consider tightening the check to include the plugin path substring, or rely solely onerr?.code === "MODULE_NOT_FOUND".Additionally,
frontend/src/components/custom-tools/prompt-card/Header.jsx(lines 42–49) uses a barecatch {}for the sameprompt-change-indicatorplugin. This creates an inconsistency: a broken cloud build surfaces an error here but silently disappears in the header, contradicting the PR goal of failing loudly for broken plugins.🔧 Optional: tighten the missing-module check
const msg = err?.message || ""; const isModuleMissing = err?.code === "MODULE_NOT_FOUND" || - msg.includes("Failed to fetch dynamically imported module") || - msg.includes("Cannot find module"); + msg.includes("Cannot find module") || + (msg.includes("Failed to fetch dynamically imported module") && + msg.includes("prompt-change-indicator/ReadOnlyReviewPage"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/useMainAppRoutes.js` around lines 115 - 136, The dynamic-import error handling in useMainAppRoutes.js around the ReadOnlyReviewPage import is too permissive and can hide runtime or transitive dependency failures; tighten the missing-module heuristic by checking err?.code === "MODULE_NOT_FOUND" or by additionally verifying the error message contains the plugin path substring ("prompt-change-indicator/ReadOnlyReviewPage") before treating it as a harmless missing-module case, and log/throw all other errors; also make Header.jsx's bare catch (in the prompt-change-indicator import there) consistent by replacing the empty catch with the same tightened heuristic and error-logging behavior so unexpected plugin failures are surfaced uniformly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/routes/useMainAppRoutes.js`:
- Around line 115-136: The dynamic-import error handling in useMainAppRoutes.js
around the ReadOnlyReviewPage import is too permissive and can hide runtime or
transitive dependency failures; tighten the missing-module heuristic by checking
err?.code === "MODULE_NOT_FOUND" or by additionally verifying the error message
contains the plugin path substring
("prompt-change-indicator/ReadOnlyReviewPage") before treating it as a harmless
missing-module case, and log/throw all other errors; also make Header.jsx's bare
catch (in the prompt-change-indicator import there) consistent by replacing the
empty catch with the same tightened heuristic and error-logging behavior so
unexpected plugin failures are surfaced uniformly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f482928-5b1e-48a0-a0d5-a2d4b16c27b8
📒 Files selected for processing (1)
frontend/src/routes/useMainAppRoutes.js
vishnuszipstack
requested review from
Deepak-Kesavan,
athul-rs,
chandrasekharan-zipstack,
jaags-dev and
muhammad-ali-e
Resolve conflict in Header.jsx by keeping both plugin import blocks: the prompt-change-indicator plugin (this branch) and the lookup-studio plugins (main). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
4 participants
What
Headerfor a new cloud-onlyPromptChangeIndicatorplugin button.:orgName/review/readonly/:documentIdgated on a cloud-onlyReadOnlyReviewPageplugin.Why
How
frontend/src/components/custom-tools/prompt-card/Header.jsx: adds atry/catchdynamic import forplugins/prompt-change-indicator/PromptChangeIndicator, renders it (withpromptDetailsandtoolDetails) betweenExpandCardBtnandPromptRunBtnSpswhen present.frontend/src/routes/useMainAppRoutes.js: adds a paralleltry/catchdynamic import forplugins/prompt-change-indicator/ReadOnlyReviewPage, registersreview/readonly/:documentIdinside the existingReviewLayoutgroup when present.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
if (PluginName) { ... }and the imports are insidetry/catch. When the cloud plugin is absent (OSS), both blocks short-circuit and the UI is identical to before. The route addition lives inside the existingReviewLayout && ManualReviewPageblock so it can never override an existing route.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
HITLChangeLogrows.Dependencies Versions
Notes on Testing
npm start, open Prompt Studio — no indicator button, no console errors, no broken routes./<orgName>/review/readonly/anythingin OSS — should fall through (route only registers when plugin loads).frontend/src/plugins/prompt-change-indicator/: indicator renders next to each prompt header and the readonly route works.Screenshots
N/A
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code