feat[ui]: Add pending changes view to raw config mode with lazy JSON …#48621
feat[ui]: Add pending changes view to raw config mode with lazy JSON …#48621JiajunBernoulli wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds a collapsible "pending changes" diff panel to the raw (JSON5) config editor, matching the UX already present in form mode. It introduces Key findings:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/ui/views/config.ts
Line: 989-993
Comment:
**`onRawChange` misused to force re-render**
The `@toggle` handler calls `props.onRawChange(props.raw)` with the **unchanged** raw string as a hack to trigger a re-render after `cvs.rawDiffRevealed` is mutated. This is fragile in two ways:
1. If `onRawChange` de-duplicates (i.e. bails out early when the new value equals the current value), the re-render never fires. `cvs.rawDiffRevealed` will be `true` in memory, but `rawDiff` (which is only computed when `cvs.rawDiffRevealed` is `true`) will still be `[]` because `renderConfig` wasn't re-invoked. The user will see the fallback "Changes detected (JSON diff not available)" message even though a proper diff could be produced.
2. Even when it does trigger a re-render, calling a "content changed" callback with an identical value is semantically incorrect and may have unintended side effects upstream (e.g. resetting debounce timers, dirtying change-tracking state, etc.).
A cleaner fix is to compute the diff eagerly regardless of `rawDiffRevealed`, or expose a dedicated `onRequestUpdate` / `onStateChange` prop for re-render requests that carry no content semantics:
```
@toggle=${(e: Event) => {
const details = e.target as HTMLDetailsElement;
cvs.rawDiffRevealed = details.open;
props.onRawChange(props.raw); // BUG: unchanged value — replace with a dedicated re-render trigger
}}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/ui/views/config.ts
Line: 650
Comment:
**Misleading field name: `rawDiffRevealed` vs `rawRevealed`**
In this file, the existing `rawRevealed` flag means "the user has opted-in to showing sensitive/redacted values in the raw editor." The new `rawDiffRevealed` field, however, tracks whether the diff `<details>` panel is expanded — an entirely different concept (panel visibility, not value revelation).
This naming collision makes it easy to confuse the two flags (e.g. `renderRawDiffValue` already references both — `rawRevealed` for redaction logic and `rawDiffRevealed` implicitly through the lazy-diff guard). Consider renaming to `rawDiffExpanded` or `rawDiffOpen` to make the semantics distinct at a glance.
```suggestion
rawDiffExpanded: boolean;
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: ab8ab44 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
JiajunBernoulli
force-pushed
the
ui/raw-json-diff-test
branch
from
ab8ab44 to
c873bf3
Compare
![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: c873bf3d3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@BunsDev Are you reviewing it? I have submitted this PR multiple times, but it has not yet received a review. To keep it up to date, I will periodically rebase it onto If possible, I would greatly appreciate your assistance in reviewing and merging this ready-to-merge PR: #46654 Thanks a lot |
|
@BunsDev Are you reviewing it? |
|
Please resolve conflicts and then ping me here again and I will work on it ASAP @JiajunBernoulli also resolve convos, where appropriate. Thanks so much for your patience and contribution! |
JiajunBernoulli
force-pushed
the
ui/raw-json-diff-test
branch
from
c873bf3 to
ff8f01b
Compare
JiajunBernoulli
force-pushed
the
ui/raw-json-diff-test
branch
from
ff8f01b to
0e77b3b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e77b3b78f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@BunsDev CI Failed because 110782a26a and c6d0baf562 should I fix them in this PR? |
|
Here is new test |
JiajunBernoulli
force-pushed
the
ui/raw-json-diff-test
branch
from
0e77b3b to
89a83ef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89a83efa37
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const rawDiff = | ||
| formMode === "raw" && cvs.rawDiffRevealed ? computeRawDiff(props.originalRaw, props.raw) : []; |
There was a problem hiding this comment.
rawDiff is computed whenever formMode === "raw" && cvs.rawDiffRevealed, even if props.raw === props.originalRaw and the raw diff panel is not rendered. Because rawDiffRevealed is sticky after users open the panel once, subsequent renders (including idle host updates after save/apply) still parse both full config strings and run computeDiff, which adds avoidable O(n) work on large configs and can hurt UI responsiveness. Guarding this computation with hasRawChanges (or resetting rawDiffRevealed when changes clear) avoids the unnecessary hot-path parse.
Useful? React with 👍 / 👎.
BunsDev
left a comment
BunsDev
left a comment
There was a problem hiding this comment.
Thanks for the patience here. The feature target is useful: raw config mode should have an actionable pending-changes view like form mode. This implementation needs another pass before it can land.
Blocking changes:
-
Please remove the
props.onRawChange(props.raw)render-trigger hack from the<details @toggle>handler. Calling a content-change callback with the unchanged raw value is semantically wrong and can break if the host deduplicates identical values. Use actual component state/update plumbing, compute the diff from existing render state, or add a dedicated non-content update callback. -
Guard raw diff computation with
hasRawChangesand the panel state, or reset the panel state when changes clear. Right nowrawDiffRevealedis sticky, so later raw-mode renders can keep parsing both full config strings even when the panel is not rendered or there are no pending raw changes. -
Please rename
rawDiffRevealedto something likerawDiffOpen/rawDiffExpanded.rawRevealedalready means sensitive-value reveal in this file, so the new name is easy to confuse with redaction behavior.
Also, the branch is currently dirty and check-additional is failing. Please rebase on current main, resolve the check failure, and keep this PR scoped to the raw pending-changes UI. The existing json5 dependency is acceptable if the final diff path still needs it.
|
This assigned pull request has been marked as stale after being open for 27 days. |
openclaw-barnacle
Bot
added
the
triage: blank-template
|
Thanks again for the contribution and patience here. I’m closing this PR as superseded by the maintainer-owned replacement #72583. Why: this PR was stale, still conflicting against current main, and still had unresolved requested changes after the latest maintainer review. Since there had not been an author follow-up after that review, I created #72583 to resolve the raw config pending-changes UI directly while preserving the useful feature target. For transparency, #72583 credits the original contributor in the changelog, references the original feature request #39831, and notes that it supersedes both this PR and the older duplicate #46654. |
Adds a raw config pending-changes diff panel in Control UI raw mode, with JSON5 parsing, sensitive-value redaction until explicit reveal, bounded diff work, and tests for redaction/reveal and stale reveal-state reset. Also aligns provider manifest contract coverage for google-vertex and Qwen aliases to unblock the rebased CI matrix. Supersedes stale PRs #48621 and #46654. PR #48621 had gone stale without maintainer follow-up, so this maintainer-authored PR carries the implementation forward transparently while preserving changelog credit for the original contributor and @BunsDev.
|
@BunsDev It was successfully merged today; however, I was unable to complete the necessary contributions within the timeframe. Given my involvement in key aspects of the implementation, I would be honored to be acknowledged as a co-author. Thank you for your consideration. |
3 participants
…diff comparison
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Not View
View
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No)Yes/No)Yes/No)Failure Recovery (if this breaks)
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.