feat(datagrid): auto-detect JSON and PHP serialized values in text cells#1447
Conversation
![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: 07671d8fad
ℹ️ 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".
|
|
||
| let phpFirstScalars: Set<Unicode.Scalar> = ["N", "b", "i", "d", "s", "S", "a", "O", "C", "o", "r", "R"] | ||
| if let first, phpFirstScalars.contains(first) { | ||
| if PhpSerializeParser.looksLikePhpSerialized(value) { return .phpSerialized } |
There was a problem hiding this comment.
For editable text that merely starts with a PHP-ish prefix but is not serialized data (for example s: status or i: todo), this returns .phpSerialized because looksLikePhpSerialized only checks the first token and separator. CellInteractionResolver then opens the read-only PHP viewer for the cell, so double-click no longer starts the normal editor for those plain-text values.
Useful? React with 👍 / 👎.
| guard let result = cursor.parseValue(depth: 0) else { return nil } | ||
| return result |
There was a problem hiding this comment.
When the input has a valid serialized prefix followed by additional data, such as i:1; trailing text, parse returns the prefix value and never checks that the cursor consumed the whole string. The PHP viewer will therefore present the cell as the integer 1, hiding the extra bytes from the tree view instead of treating the value as invalid/plain text.
Useful? React with 👍 / 👎.
| var entries: [PhpKeyValue] = [] | ||
| entries.reserveCapacity(count) |
There was a problem hiding this comment.
A malformed cell like a:1000000000:{} is under the 5 MB input cap but causes reserveCapacity(count) to try allocating space for a billion entries before the parser discovers the body is invalid; the object-property path has the same pattern. Opening or auto-rendering such a value can crash the app from memory pressure, so the count should be bounded by the remaining payload or a parser cap before reserving.
Useful? React with 👍 / 👎.
… content detection
…rd, avoid byte copies
|
Addressed review feedback in two follow-up commits:
All 52 tests in the 7 affected suites pass. SwiftLint strict is clean. Build is clean. Not addressed (your call):
|
1 participant
Summary
Cells holding JSON or PHP
serialize()values in text columns (TEXT/VARCHAR/LONGTEXT) now open in a structured viewer automatically, without requiring the column type to be JSON.CellInteractionResolver's editable branch sniffedlooksLikeJsonbut the read-only branch didn't.OutlineGroupwithPicker(.segmented)Tree/Raw toggle, mirroring the existing JSON viewer surface (popover, right-sidebar inspector, pop-out window).ValueDisplayFormatgains.jsonand.phpSerializedcases for text columns..rawopts out of detection entirely.Competitive context: TablePlus #1144 (PHP unserialize) has been open 7+ years with no implementation; Sequel Ace's PHP support is a deprecated Bundle, broken on macOS 12+. TablePlus, Postico, and DataGrip all gate JSON viewing on declared column type (TablePlus #3215, #3477). This PR ships the auto-detect path none of them have.
Architecture
Detection runs only on cell open, never during grid render (zero render cost). One shared
CellValueContentDetectorconsumed by bothCellInteractionResolver(grid) andFieldEditorResolver(right sidebar) so future formats can't reintroduce the asymmetry.looksLikeJson(existing) orPhpSerializeParser.looksLikePhpSerialized(new). 5 MB cap via(value as NSString).length.N b i d s S a O C r R; degradesoand unknowns to.unsupported; PHPO:decodes protected (\0*\0name) and private (\0ClassName\0name) mangling;C:is a single leaf (no recursion into Serializable payload);r:/R:rendered as→ #Nmarkers (no cycle traversal). 256 depth cap, 5000 node cap..task { ... }; raw mode is always available as the safety net.JSONViewerContentView/JSONViewerView/JSONViewerWindowController) is column-type-agnostic and reused as-is. ExistingJSONTreeParserpreserves source key order.Tests
PhpSerializeParserTests— every token, INF/NAN, multi-byte strings, declared-length mismatches, depth cap, reference markers, protected/private mangling decode.CellValueContentDetectorTests— positive/negative for both formats, 5 MB guard, ambiguous-prefix false positives.ValueDisplayFormatTests— rawValue, Codable round-trip,applicableFormats(for:)membership.FieldEditorResolverTests— JSON/PHP detection plus override precedence (.rawskips both,.json/.phpSerializedforce).CellInteractionResolverTests— extended with PHP detection cases; the previousreadOnlyJsonLikeTextWithoutTypeReturnsViewInlinetest (which documented the bug) is inverted to...ReturnsViewJsonsince that's now the correct behavior.All new and modified tests pass locally (
xcodebuild test). Full project build is clean.Manual test
A test fixture and walkthrough are in the development notes (10 sample rows covering JSON object/array, deeply nested, PHP scalars, nested arrays, objects with protected/private properties, references, and false-positive guards).
Notes for review
Localizable.xcstringscontains some pre-existing WIP entries (pagination strings, BigQuery dataset strings, AWS credential_process strings, SSH/cloudflared messages) that propagated from the working tree when the branch was created. The PHP-specific keys I added are:Invalid PHP Serialized Value,Maximum depth reached,PHP — %@,PHP Serialized,PHP Viewer,private (%@),protected,The value could not be parsed. Use raw mode to inspect it as text.,Unsupported token: %@,Value Too Large,Value too large to parse,This value is too large to parse. Use raw mode to inspect it as text.(12 keys). Feel free to split the Localizable changes into a separate commit before merging.PHP — %@) match the existingJSONViewerWindowController.swiftconvention (JSON — %@) and macOS HIG ("Document — App"). CLAUDE.md's anti-em-dash rule is generally for prose; if you want to refactor both window titles to a different separator, that's a small follow-up.CellInteractionMode.editPhpSerializedis defined but unused (the editable branch always returns.viewPhpSerializedfor PHP, since PHP write-back is out of scope). Easy to remove if you'd rather not carry an unused case.isReadOnly, which hides the field-action menu (NULL/Default/Empty/Function) on those fields. Reasonable since they can't be edited via the viewer, but if you want those actions reachable, easy to gate differently.Closes the request from https://x.com (public reply asking for auto JSON parse and PHP unserialize).