Repository navigation
Replace native overflow-menu and composer-asset extensions with LUI standard components - #36
Conversation
Migrate three platform extension surfaces onto standard LUI nodes so interaction stays identical with less custom backend code: - Overflow menus: menu-trigger + dropdown-menu + menu-item replace the native-overflow-menu extension on both screens. iOS keeps the 44x44 stroked-circle more_horiz trigger with the button.connection accessibility id; Flutter keeps more_vert, item icons, and the button.overflow-menu id. Favorite label/icon stay signal-driven, and page-actions/settings visibility stays signal-gated. - Composer attachments: file-image replaces the composer-asset extension at 128x128 r12 with fill fitting and the document icon+title fallback for non-image assets. - Asset preview: a file-preview node mounted while asset_preview is set replaces the hand-wired QuickLook host; dismissing sends DismissAssetPreview through the model. Removes both extensions end to end (OCaml schemas, Swift files and registrations, Flutter implementations and fingerprints) and bumps the lui pin to 1db993d, picking up LUIPickedEvent, LUIScrollCompletedEvent, and LUIVisibleRangeEvent plumbing in the native bridge and dispatch. Also refreshes three stale tests: the drain trace assertions now cover the apply-patch call sites added in cb9374b, and the icons/search contract tests scan the OCaml views instead of the removed view.cljc.
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
👀 3 findings need your review
Devin reviewed all 3 findings on 7dd1abf and left them for you. Click a finding below to jump to its comment.
For your review (3)
- Relocated attachments lose iOS previews
- Missing assets falsely open successfully
- PDF attachments lose their Android tile
View all findings in Devin Review
| asset_preview = | ||
| Some | ||
| { | ||
| preview_title = row.row_title; | ||
| preview_path = path; | ||
| }; |
There was a problem hiding this comment.
🟡 Relocated attachments lose iOS previews
For an attachment with an obsolete absolute path, asset_preview presents that stale path instead of the relocated file. Quick Look cannot open the attachment even when it exists under Documents/Assets.
Learn more
The old iOS presenter called LocalAssetPath.resolve, which recovers attachments from Documents/Assets when their stored absolute path is stale. The new preview sends the stored path directly to the LUI file-preview node. LUI's file path resolver handles current Documents-relative paths but cannot infer the relocated file from an obsolete absolute path. The platform effect now returns success without checking the file, so it cannot correct that path.
Example: An attachment stored as /old/container/Documents/Assets/photo.jpg survives in /new/container/Documents/Assets/photo.jpg. Opening it previously resolved the new URL; the new preview receives the old URL and fails.
Recommended fix: Resolve the path using the existing iOS LocalAssetPath.resolve fallback before publishing the preview, or add equivalent title/type-based recovery to the file-preview path resolver. Keep the Android MethodChannel path working independently.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Partially outdated: LUIFilePath (merged in lui #98) already covers the main relocation case — an obsolete absolute path falls back to Documents/Assets/<basename> before giving up, so /old/container/.../photo.jpg resolves to the new container's copy. The real residual is narrower: LocalAssetPath.resolve(title:assetType:) also recovered a file by title + MIME extension when the stored path was empty or fully unresolvable. That fallback has no equivalent in the new path. Whether it's worth porting depends on how often title-recovery actually fires in practice — flagging for a human call.
| // Asset preview is presented by the file-preview node on iOS; the | ||
| // effect only drives the Android MethodChannel path, so resolve it | ||
| // as a no-op here. | ||
| presentAsset: { _ in true }, |
There was a problem hiding this comment.
🟡 Missing assets falsely open successfully
When an asset file is missing, OpenOutlinerAsset still sets asset_preview and the iOS effect reports success. The unavailable-file error disappears, leaving a broken Quick Look presentation.
Learn more
An outliner asset triggers OpenOutlinerAsset, which now sets preview state before the platform effect runs. The old iOS presenter checked for a real file through LocalAssetPath.resolve and reported failure if it could not find one. The replacement callback always reports success. As a result, an unavailable attachment never produces the intended failure response, and its preview remains mounted.
Example: A synced block points to Assets/deleted.pdf, which no longer exists. Tapping it previously failed with an unavailable-asset result; now the effect succeeds and Quick Look receives an invalid path.
Recommended fix: Preserve the iOS file-existence check and failure result when resolving the asset effect. Only present a file-preview node for a successfully resolved path; avoid changing Android's existing MethodChannel behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed behavioral change on the missing-file edge case: the old presentAsset returned false when LocalAssetPath.resolve found nothing, so no preview was presented; now asset_preview is set unconditionally and the effect no-ops to success, so Quick Look opens on a nonexistent file. Options to restore the old contract: (a) keep an iOS-side existence check by resolving the path before the preview node mounts (needs an effect round-trip or a model-side signal), or (b) let file-preview fail dismiss itself in lui. Not addressed in this PR — see thread for maintainer decision.
| List.mem extension | ||
| [ "png"; "jpg"; "jpeg"; "gif"; "webp"; "bmp"; "wbmp"; "heic"; "heif" | ||
| ; "avif"; "tif"; "tiff"; "pdf" ] |
There was a problem hiding this comment.
🟡 PDF attachments lose their Android tile
For a staged PDF, composer_asset_is_image selects file_image rather than the document tile. Flutter cannot decode PDFs as images, so the attachment loses its title and document icon.
Learn more
Composer attachments previously used a Flutter renderer that checks isAndroidImageAsset and renders non-image assets with a document icon and filename. That predicate excludes PDF. The new shared classifier includes PDF and routes it to a file-image node; Flutter's image codec does not decode PDF pages. iOS can thumbnail PDF pages through ImageIO, but that does not make a PDF a Flutter image.
Example: Staging Assets/report.pdf on Android previously showed a tile labeled report.pdf with a document icon. It now renders a file-image that fails to decode and does not show the filename.
Recommended fix: Make the classification platform-aware: preserve iOS PDF thumbnails, but send PDF attachments to the non-image composer tile on Flutter unless the new Flutter backend explicitly provides PDF thumbnail rendering.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed — real regression on Android. The old Flutter isAndroidImageAsset excluded pdf/tif/tiff, so staged PDFs rendered the document-icon + title tile; the shared classifier now routes them to file_image, which Flutter can't decode. Fix is small and mechanical: split the extension list by platform (Apple keeps pdf/tif/tiff since CGImageSource thumbnails them; Flutter/Android keeps the old set). Awaiting approval to apply.
… CI PAT - CI: remove LOGSEQ_GITHUB_PAT usage (repos are public); db-sync-server checkout uses the default token and credential rewrites drop x-access-token. - view_base: composer_asset_is_image takes the ui_context; the Flutter host keeps the pre-change extension set (no pdf/tif/tiff, which the platform image codec cannot decode) while other hosts keep thumbnail previews. - Asset preview (iOS): the present-asset effect resolves the file on the host (LocalAssetPath: stored path, basename relocation, title/MIME recovery) and reports asset-preview-resolved; the core only mounts the file-preview node for a successfully resolved path, so missing files no longer present a broken QuickLook.
The lui bump restyled the standard composer (content-hugging composer-surface, bounded 168pt field, interactive glass). Vendor the previous surface spec so the expanded capture keeps the old chrome.
The Swift package was pinned to backend revision 55166025 which predates the file-image/file-preview node kinds, so wire batches carrying them failed to decode (invalidBatch) and froze the renderer. Bumping to 1db993d4 brings the interactive liquid-glass buttons, toolbar hit-area and fused-capsule fixes, and restores the ring-and-dots overflow trigger and the collapsed Capture glass capsule. The new backend also emits scrollCompleted, visibleRange and picked events; forward them through LGChatRendererEvent, LGChatNativeCalling and the core FFI so the OCaml callbacks already registered for them are reached.
|
UI 观感审计 round 2（lui 1db993d4 build, iPhone 17 sim, 亮/暗双主题） 上轮回归复查：图片附件 tile 已修复（invalidBatch 消失，缩略图正常渲染并可发送内联显示）、More 触发器已修复（描边省略号与 sync dot 融合 capsule）。file-preview/search-nav 按约定未重测。 🔴 本轮新发现：
完整分级报告（19 项 finding）在会话内可查。 |
Round-2 UI audit fixes, Apple backend via lui@1561261: - graph-delete + page-delete dialogs flattened to text+button content with style_class "alert" -> real UIAlertController with cancel/destructive roles (was a custom full-screen sheet). - Settings sheet -> style_class "navigation-form" with a form-style content column: heading delimiters become grouped Form sections; toggles render system green via native form rows; appearance picker -> radio_group style_class "menu" (Picker.menu); destructive Sign Out list_item honors its foreground token. - New sign_out_dialog (pending_sign_out model state): RequestSignOut closes the settings sheet so the alert presents on a clean context; CancelSignOut re-opens it. Stops one-tap sign-out. - LGChatEffects routes export-graph-database + open-external-url to the platform effect handler (was Unsupported LG effect — dead button). - Bump lui pin to 1561261 (modal swap sequencing + list-item foreground + destructive token + iOS list commit stability). - Drive/app_test fixtures updated for the new identifiers.
Unresolved [[refs]], #[[tags]], and [label](page) no longer leak raw markup syntax into rendered output — they degrade to their inner text. Refs that fail to resolve previously surfaced as literal [[x]] in the outliner (audit finding).
|
Round-2 audit fix verification (lui 1561261, commits 0fd5e69 + c9e4cdd) — all 6 items verified on iPhone 17 sim, light + dark.
more evidence
Minor: composer accepts invisible draft text when the field isn't focused (posting looked-empty drafts); export UX could use feedback while the sheet is deferred. |
- Export Graph / node share used a .sheet(item:) bound to the root view, so while Settings (or any sheet) was presented the share UI queued silently behind it. Present UIActivityViewController directly on the topmost presented view controller instead — it stacks above Settings immediately, with popover anchors for iPad. - Remove the now-dead pageSharePayload plumbing and NodeShareSheet. - Bump lui to 33e08ba (form-surface theming + draft-state fix).
lui 772b776 applies themed row surfaces per row in navigation-form sheets (Form-level listRowBackground never reached Section rows, so Settings cards stayed system gray #2C2C2E) and prefers a 'card' token over 'background' for rows. Define card: white on light, #1C4556 on dark — a lighter teal that keeps contrast against the #19394D elevated sheet and the #002D38 page without going black or gray.
1 participant
Summary
Replaces three hand-written platform extension surfaces with standard LUI nodes, keeping UI and interaction identical while deleting the custom backends:
menu-trigger+dropdown-menu+menu-item(replacingnative-overflow-menu). iOS keeps the 44×44 stroked-circlemore_horiztrigger and thebutton.connectionaccessibility id; Flutter keepsmore_vert, item icons, andbutton.overflow-menu. Favorite label/icon stay signal-driven (text_signal,iconsignal); page-actions/settings visibility staysif_-gated.file-image ~image-fit:fill ~width:128 ~height:128 ~corner-radius:12(replacingcomposer-asset), with the same document icon + title fallback for non-image assets.file-previewnode mounted whilemodel.asset_preview = Some(replacing the hand-wired.quickLookPreviewhost); dismiss dispatchesDismissAssetPreviewthrough the model.Both extensions are removed end to end: OCaml schemas in
view.ml,LGChatOverflowMenuExtension.swift/LGChatComposerAssetExtension.swift+ registrations, and the Flutter implementations + fingerprints.Bumps the lui pin to
1db993d(merged #98:LUIFilePathDocuments-relative resolution + Flutterfile-image), which requires plumbing the three newLUIEventvariants (picked,scrollCompleted,visibleRange) throughnative_bridge.mlandlui_dispatch.dart.Also fixes three stale tests unrelated to the migration:
native_effect_drain_testtrace expectations updated for the apply-patch call sites added in cb9374b, andlogseq_chat_icons_test/search_layout_contract_testnow scan the OCaml views instead of the long-removedview.cljc.Verified:
dune build @shared/native/runtest(719 tests),swift buildfor iOS simulator,flutter analyze,flutter test(140 tests) — all green.Not in scope (UI can't be preserved with today's components): MediaPanel →
file-picker, delete alert →dialog, nav/search →navigation-stack.Link to Devin session: https://app.devin.ai/sessions/5a45c4dddaf3412494ccfb58998fe767
Open in Devin Desktop: https://app.devin.ai/desktop/session/5a45c4dddaf3412494ccfb58998fe767?variant=devin
Requested by: @tiensonqin