Repository navigation
feat(portal): add formatted and raw JSON views - #5657
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Portal show-text modal now supports formatted and raw JSON views. The directive validates JSON and duplicate keys before enabling formatting. The template adds localized view tabs. Tests cover view modes, fallback behavior, numeric handling, and bindings. ChangesPortal JSON view modes
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR adds formatted and raw views for eligible JSON values while preserving raw-only behavior for invalid, duplicate-key, and plain-text values; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
actor User
participant ShowTextModal as show-text-modal.html
participant Directive as show-text-modal-directive.js
User->>ShowTextModal: Select formatted or raw tab
ShowTextModal->>Directive: Call scope.setViewMode(viewMode)
Directive->>Directive: Update viewMode
Directive-->>ShowTextModal: Render selected value view
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
dosubot
Bot
added
the
size:M
![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
🧹 Nitpick comments (1)
apollo-portal/src/test/resources/static/scripts/test_showTextModal.js (1)
79-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover invalid and plain-text input.
Lines 79-83 only test duplicate-key JSON. Add cases for malformed JSON and non-JSON text. Assert
canFormat === falseandviewMode === 'raw'for both cases.Proposed test cases
+ const invalidJson = createScope(); + invalidJson.scope.text = '{"a":'; + invalidJson.refresh(); + assert.strictEqual(invalidJson.scope.canFormat, false); + assert.strictEqual(invalidJson.scope.viewMode, 'raw'); + + const plainTextValue = createScope(); + plainTextValue.scope.text = 'not JSON'; + plainTextValue.refresh(); + assert.strictEqual(plainTextValue.scope.canFormat, false); + assert.strictEqual(plainTextValue.scope.viewMode, 'raw');🤖 Prompt for 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. In `@apollo-portal/src/test/resources/static/scripts/test_showTextModal.js` around lines 79 - 83, Add test cases alongside the duplicate-key case in the plainText scope tests for malformed JSON and ordinary non-JSON text. For each input, call refresh() and assert canFormat is false and viewMode remains raw, reusing the existing createScope setup.
🤖 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
`@apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js`:
- Around line 89-92: Update the validation condition in the show-text modal flow
around isJsonText so all successfully parsed JSON values—including strings,
numbers, booleans, and null—can enter the formatted view. Preserve the
duplicate-key check for object JSON and keep assigning jsonObject, canFormat,
and viewMode as in the existing branch.
---
Nitpick comments:
In `@apollo-portal/src/test/resources/static/scripts/test_showTextModal.js`:
- Around line 79-83: Add test cases alongside the duplicate-key case in the
plainText scope tests for malformed JSON and ordinary non-JSON text. For each
input, call refresh() and assert canFormat is false and viewMode remains raw,
reusing the existing createScope setup.
🪄 Autofix
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 Plus
Run ID: 55bbad54-f29d-4c79-9b90-b2926031c2df
📒 Files selected for processing (6)
CHANGES.mdapollo-portal/src/main/resources/static/i18n/en.jsonapollo-portal/src/main/resources/static/i18n/zh-CN.jsonapollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.jsapollo-portal/src/main/resources/static/views/component/show-text-modal.htmlapollo-portal/src/test/resources/static/scripts/test_showTextModal.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apollo-portal/src/test/resources/static/scripts/test_showTextModal.js (1)
117-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify the value bindings in the template.
The checks only verify labels and view-mode conditions. They do not verify that raw mode binds
textor that formatted mode bindsjsonObject | json:4 | jsonBigIntFilter. A binding regression can pass this test.Proposed test addition
assert.ok(template.includes("viewMode == 'formatted'")); assert.ok(template.includes("viewMode == 'raw'")); + assert.ok(template.includes('ng-bind="text"')); + assert.ok(template.includes('ng-bind="jsonObject | json:4 | jsonBigIntFilter"'));🤖 Prompt for 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. In `@apollo-portal/src/test/resources/static/scripts/test_showTextModal.js` around lines 117 - 121, Extend the template assertions in the test around the existing Component.ShowText and viewMode checks to verify the raw-mode binding uses text and the formatted-mode binding uses jsonObject piped through json:4 and jsonBigIntFilter. Keep the current label and mode-condition assertions unchanged.
🤖 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.
Nitpick comments:
In `@apollo-portal/src/test/resources/static/scripts/test_showTextModal.js`:
- Around line 117-121: Extend the template assertions in the test around the
existing Component.ShowText and viewMode checks to verify the raw-mode binding
uses text and the formatted-mode binding uses jsonObject piped through json:4
and jsonBigIntFilter. Keep the current label and mode-condition assertions
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b6d4c5e-e5cd-4c5e-99c9-40cf3fdf1c06
📒 Files selected for processing (2)
apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.jsapollo-portal/src/test/resources/static/scripts/test_showTextModal.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js
|
Automated follow-up on the current head Note: this reply was generated and posted automatically by AI for initial triage; a maintainer will follow up if needed. |
|
Follow-up on the current head Note: this reply was generated and posted automatically by AI for initial triage; a maintainer will follow up if needed. |
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
The default formatted path can silently display a different value for signed integers outside JavaScript's safe range. On the current head, -9007199254740993 is rendered as -9007199254740992: parseBigInt only protects unsigned digit-only integers, while this PR now sends all valid JSON scalars through the formatted view by default. Before this change, top-level numbers remained raw.
Please fix this blocker before updating:
- Preserve signed unsafe integers through the formatting pipeline, or keep values that cannot be formatted losslessly in raw mode.
- Add a focused regression test that asserts the exact displayed/formatted value, rather than only
canFormat,viewMode, and the retained raw text.
|
Addressed the signed unsafe integer blocker in
Validated with all static JavaScript tests, |
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
Thanks—the signed unsafe-integer blocker is fixed on 60962bc3f. I verified the top-level, nested, array, and marker-like string cases. Two blockers remain before approval:
show-text-modal-directive.js:129-133still protects only number tokens matching^-?\d+$. Because this PR now defaults every valid JSON scalar to the formatted view, valid fractional and exponent forms can still lose precision:9007199254740993.0and9.007199254740993e15both display as9007199254740992. Before this PR, these top-level numeric scalars remained raw. Please preserve JSON numbers that cannot round-trip losslessly, or keep them raw-only, and add exact-output coverage for fractional/exponent cases.- Move the #5657 entry from the top of the Apollo 3.0.0 section to the bottom of that section. The branch is currently one commit behind master, so please place it after the latest entry when updating the branch.
60962bc to
7f461ba
Compare
|
Addressed both blockers from review
Local validation passed: all static JavaScript tests, |
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
Re-reviewed the current head 7f461ba7e. Both requested blockers are resolved: the formatter now preserves integer, fractional, and exponent-form JSON numbers through the full display pipeline, including marker-like string cases, and the #5657 CHANGES.md entry is now appended after #5656 at the bottom of the Apollo 3.0.0 section.
The focused tests and additional numeric boundary checks pass, all required CI checks are green, the branch is up to date with master, and no active unresolved review threads remain. I found no blocking issues.
Merge Queue Status
This pull request spent 16 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
2 participants
What's the purpose of this PR
Add a formatted/raw value switch to the Portal text viewer for JSON configuration values.
Valid JSON values without duplicate keys open in the formatted view and can be switched to the exact raw value returned by the interface. Invalid JSON, duplicate-key JSON, and plain text remain raw-only. Text diff behavior is unchanged.
Which issue(s) this PR fixes:
None.
Brief changelog
FormattedandRawtabs to the text viewer for format-safe JSON values.CHANGES.md.Validation
node apollo-portal/src/test/resources/static/scripts/test_hasDuplicateKeys.jsnode apollo-portal/src/test/resources/static/scripts/test_showTextModal.js./mvnw -Dmaven.gitcommitid.skip=true spotless:check./mvnw -Dmaven.gitcommitid.skip=true clean test./mvnw -Dmaven.gitcommitid.skip=true spotless:applyThe git commit ID plugin is skipped locally because its legacy version cannot resolve
HEADfrom a Git worktree.Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.mvn spotless:applyto format your code.CHANGESlog.Summary by CodeRabbit
New Features
Bug Fixes