Add page details panel#18
Conversation
Greptile SummaryThis PR adds a native page details panel and folds in several related app updates. The main changes are:
Confidence Score: 4/5This is close, but the workspace icon cookie handling should be fixed before merging.
docmostly/Features/Spaces/WorkspaceIconView.swift and docmostly/Networking/SpaceLogoURL.swift
|
| Filename | Overview |
|---|---|
| docmostly/Features/PageReader/PageReaderViewModel+Backlinks.swift | Adds page-scoped backlink loading state and guards stale backlink responses. |
| docmostly/Features/Spaces/WorkspaceIconView.swift | Adds a manual workspace icon fetch path that still attaches cookies to unrestricted absolute logo URLs. |
| docmostly/Networking/FavoriteIDListResponse.swift | Adds favorite-ID decoding for both paginated and flat response shapes. |
Reviews (3): Last reviewed commit: "fix: address page details review finding..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 48 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
![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: a1ab095010
ℹ️ 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".
| rowCount: rowCount, | ||
| columnCount: columnCount |
There was a problem hiding this comment.
This call now passes rowCount and columnCount into NativeEditorTableSelectionStroke, but that view only declares selection, rowIndex, and columnIndex, so any target that compiles NativeEditorTableGridView will fail at this initializer instead of building the app.
Useful? React with 👍 / 👎.
| if reset == false, backlinkPagesByDirection[direction]?.isEmpty == false { | ||
| return |
There was a problem hiding this comment.
When the first backlink request returns items and hasNextPage == true, tapping “Load more” calls loadBacklinks(reset: false), but this guard immediately returns because the direction already has a non-empty page array. That means the saved cursor is never used and users can never fetch page 2+ for backlink lists that already displayed results.
Useful? React with 👍 / 👎.
|
|
||
| static func stats(in document: NativeEditorDocument) -> Self { | ||
| let text = document.blocks | ||
| .map { String($0.text.characters).trimmingCharacters(in: .whitespacesAndNewlines) } |
There was a problem hiding this comment.
For pages that contain tables, this stats builder only reads block.text, but table blocks store their actual content in NativeEditorTableCell.plainText while block.text is just the preview string such as the row/column count. The Details panel will therefore undercount or count the table dimensions instead of the user's words, making the displayed word and character counts wrong for table-heavy pages.
Useful? React with 👍 / 👎.
| .foregroundStyle(.primary) | ||
| .font(cell.map(NativeEditorTableLayout.font) ?? .callout) | ||
| .foregroundStyle(cell?.isHeader == true ? .primary : NativeEditorTableLayout.bodyForeground) | ||
| .lineLimit(6) |
There was a problem hiding this comment.
When viewing a page that has a table cell with more than six lines, this new limit silently hides the rest of the cell's content in the reader/preview state, and read-only users have no way to reveal it. Tables previously rendered the full displayText, so long documentation content can now disappear from normal page reading.
Useful? React with 👍 / 👎.
| .padding() | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| } | ||
| .task(id: direction) { |
There was a problem hiding this comment.
If the user changes pages while an incoming/outgoing backlink list is still presented, such as from the split-view sidebar, the engagement snapshot for the new page clears the backlink arrays but this task is keyed only by direction, so it does not fetch links for the new pageID. The list can stay empty or stale until it is dismissed and reopened; include the page id in the task identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| let cookies = await appState.activeSessionCookies(for: url) | ||
| if cookies.isEmpty == false { | ||
| request.setValue(cookieHeader(from: cookies), forHTTPHeaderField: "Cookie") |
There was a problem hiding this comment.
SpaceLogoURL.url(...) still returns absolute http and https logo strings unchanged, and this path then builds a manual Cookie header for that returned URL. httpShouldHandleCookies = false only disables the system cookie jar; it does not stop activeSessionCookies(for:) from adding stored cookies that match the absolute URL's domain and path. If a workspace logo is set to a same-site arbitrary URL, or to a subdomain covered by a domain-scoped session cookie, the icon request can still send authenticated cookies outside the intended workspace attachment endpoint. Please only attach cookies after confirming the URL is the expected same-origin workspace-icon attachment URL, or reject absolute workspace icon URLs before fetching.
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docmostly/Features/PageReader/PageReaderViewModel+Backlinks.swift">
<violation number="1" location="docmostly/Features/PageReader/PageReaderViewModel+Backlinks.swift:35">
P2: A failed backlinks request from the previous page can still show an error on the newly opened page. The success path now guards `backlinkPageID == pageID`, but the `catch` path should apply the same stale-request check before assigning `backlinkErrorMessage`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| do { | ||
| let cursor = loadNextPage ? backlinkNextCursorByDirection[direction] ?? nil : nil | ||
| let response = try await appState.loadPageBacklinks(pageId: pageID, direction: direction, cursor: cursor) | ||
| guard backlinkPageID == pageID else { return } |
There was a problem hiding this comment.
P2: A failed backlinks request from the previous page can still show an error on the newly opened page. The success path now guards backlinkPageID == pageID, but the catch path should apply the same stale-request check before assigning backlinkErrorMessage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docmostly/Features/PageReader/PageReaderViewModel+Backlinks.swift, line 35:
<comment>A failed backlinks request from the previous page can still show an error on the newly opened page. The success path now guards `backlinkPageID == pageID`, but the `catch` path should apply the same stale-request check before assigning `backlinkErrorMessage`.</comment>
<file context>
@@ -5,21 +5,36 @@ extension PageReaderViewModel {
+ let cursor = loadNextPage ? backlinkNextCursorByDirection[direction] ?? nil : nil
let response = try await appState.loadPageBacklinks(pageId: pageID, direction: direction, cursor: cursor)
- if reset {
+ guard backlinkPageID == pageID else { return }
+
+ if reset || loadNextPage == false {
</file context>
1 participant
Summary
Verification
Localisation Contributors, opened page actions, selectedShow Details, verified metadata/stats/backlinks/labels loaded, and openedIncoming linksto the loaded empty state.mcp__xcodebuildmcp.test_simon iPhone 17: 603 passed, 0 failed.xcodebuild test -quiet -project docmostly.xcodeproj -scheme docmostly -destination platform=iOS Simulator,name=iPad Pro 13-inch (M5): result bundle Passed, 603 total, 0 failed.xcodebuild test -quiet -project docmostly.xcodeproj -scheme DocmostlyMac -destination platform=macOS: 9 passed.swiftlint lint --strict --no-cacheon modified Details files: 0 violations.git diff --cached --checkbefore commit andgit diff --checkafter commit: clean.Notes