UN-3386 [FEAT] Add Prompt Studio HITL change indicator plugin slot#1930
Open
vishnuszipstack wants to merge 3 commits intomainfrom
Open
UN-3386 [FEAT] Add Prompt Studio HITL change indicator plugin slot#1930vishnuszipstack wants to merge 3 commits intomainfrom
vishnuszipstack wants to merge 3 commits intomainfrom
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>
Contributor
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 the 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 |
Contributor
|
| Filename | Overview |
|---|---|
| frontend/src/components/custom-tools/prompt-card/Header.jsx | Adds dynamic-import slot for PromptChangeIndicator plugin; bare catch is inconsistent with the routes file, and indicator is not gated on !isSimplePromptStudio. |
| frontend/src/routes/useMainAppRoutes.js | Registers ReadOnlyReviewPage route inside existing ReviewLayout group; well-handled with discriminated catch and explicit console.warn for missing-layout misconfiguration. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Module load] --> B{import PromptChangeIndicator}
B -- found --> C[PromptChangeIndicator set]
B -- missing/error --> D[stays undefined]
A --> E{import ReadOnlyReviewPage}
E -- found --> F[ReadOnlyReviewPage set]
E -- module missing --> G[stays undefined, silent]
E -- unexpected error --> H[console.error + stays undefined]
F --> I{ReviewLayout present?}
I -- No --> J[console.warn + route skipped]
I -- Yes --> K[Route registered: review/readonly/:documentId]
C --> L{Render Header}
L -- PromptChangeIndicator defined --> M[Render indicator button]
L -- undefined --> N[No indicator rendered]
style J fill:#ffe0b2
style H fill:#ffe0b2
Prompt To Fix All With AI
This is a comment left during a code review.
Path: frontend/src/components/custom-tools/prompt-card/Header.jsx
Line: 44-52
Comment:
**Bare catch silently swallows non-missing-module errors**
The `PromptRunBtnSps` import above uses the same bare-catch pattern, but the sibling `ReadOnlyReviewPage` import in `useMainAppRoutes.js` (introduced in this same PR) correctly discriminates between "module not found" and unexpected errors (syntax/runtime). If the cloud plugin ships with a JS syntax error, the indicator silently disappears with no console signal. Consider applying the same discriminated-catch there for consistency and debuggability:
```js
} catch (err) {
const msg = err?.message || "";
const isModuleMissing =
msg.includes("Failed to fetch dynamically imported module") ||
msg.includes("Cannot find module");
if (!isModuleMissing) {
console.error("[prompt-change-indicator] PromptChangeIndicator import failed unexpectedly", err);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/components/custom-tools/prompt-card/Header.jsx
Line: 407-412
Comment:
**`PromptChangeIndicator` renders in Simple Prompt Studio mode too**
The indicator is placed outside any `isSimplePromptStudio` gate, so it will render in both standard and Simple Prompt Studio modes. The existing run buttons are each carefully gated (`!isSimplePromptStudio` for regular buttons, `isSimplePromptStudio` for `PromptRunBtnSps`). If HITL review doesn't apply to SPS workflows, you'd want:
```suggestion
{PromptChangeIndicator && !isSimplePromptStudio && (
<PromptChangeIndicator
promptDetails={promptDetails}
toolDetails={details}
/>
)}
```
If showing it in both modes is intentional, a brief comment here would clarify the design choice.
```suggestion
{PromptChangeIndicator && !isSimplePromptStudio && (
<PromptChangeIndicator
promptDetails={promptDetails}
toolDetails={details}
/>
)}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "UN-3386 [FIX] Surface real plugin import..." | 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>
|
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
chandrasekharan-zipstack
approved these changes
Apr 27, 2026
jaseemjaskp
approved these changes
Apr 27, 2026
Deepak-Kesavan
approved these changes
Apr 27, 2026
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
4 participants
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
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