Skip to content

fix: update_page properties now update in-place via upsertBlockProperty - #49

Closed
nlsn wants to merge 1 commit into
ergut:mainfrom
nlsn:fix/update-page-properties
Closed

fix: update_page properties now update in-place via upsertBlockProperty#49
nlsn wants to merge 1 commit into
ergut:mainfrom
nlsn:fix/update-page-properties

Conversation

@nlsn

@nlsn nlsn commented May 18, 2026

Copy link
Copy Markdown
Contributor

Closes #48

Problem

update_page called with a properties dict and no content was routing
through _set_page_level_propertieslogseq.Editor.setPageProperties.
In practice, setPageProperties appended a new block to the page body with
properties serialized as text, rather than updating the existing page-level
property lines in place. The appended block was invisible to Logseq's property
system and the success message was misleading.

Secondary bug: tag values were serialized as Python list repr (['mcp-test'])
instead of Logseq syntax ([[mcp-test]]).

Fix

In src/mcp_logseq/logseq.py, update_page_with_blocks: replaced the
_set_page_level_properties call (and the pre-fetch via
_get_page_level_properties) with a single call to _update_page_properties.

That method calls logseq.Editor.upsertBlockProperty once per key on the
first block (the page-properties pre-block), updating values in place and
implicitly preserving untouched keys.

The tags serialization bug is also resolved — upsertBlockProperty receives a
proper Python list and Logseq renders it correctly as [[tag]] references.

Tests Added

  • test_update_page_properties_only_updates_in_place — regression test;
    asserts no setPageProperties/appendBlockInPage call, and exactly N
    upsertBlockProperty calls targeting the first block
  • Updated test_update_page_append_mode_merges_properties and
    test_update_page_replace_mode_replaces_properties to match new API call
    sequence and result shape

setPageProperties was appending properties as a new block body instead of
updating the existing page-level property lines, making them invisible to
Logseq's property system and rendering tag lists as Python repr strings.

Switch update_page_with_blocks to use _update_page_properties, which calls
upsertBlockProperty on the first block for each key. This updates properties
in-place, preserves unspecified keys implicitly, and fixes tag serialization.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nlsn

nlsn commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

Hi @ergut - before merging, I want to flag a potential secondary issue I haven't had time to investigate yet.

The fix routes tag values through upsertBlockProperty with a Python list, which Logseq renders as [[wikilink]]-style page references. This may be the correct behavior, but I want to verify it's consistent with what create_page produces for tags, and that it matches user expectations — plain tags and page references behave differently in Logseq (backlinks, page creation, etc.).

Could you hold off on merging until I've had a chance to test this? Thanks for your patience.

@ergut

ergut commented Jun 6, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR, the careful repro in #48, and for flagging the tags wikilink concern yourself and asking to hold off. Good instinct: my review reached the same area and found some blocking issues, so here is the full picture.

  1. DB mode regression (blocking): _update_page_properties uses upsertBlockProperty unconditionally. That is exactly the approach 5b99215 moved away from: block level properties don't register as page level on DB graphs (invisible to (page-property ...) queries and the page info panel). The PR has no db_mode guard.

  2. Replace mode no longer replaces: both modes collapse to the same per key upsert, so properties the caller didn't supply survive a mode: replace update. The updated test only asserts on the returned dict, not page state, so this doesn't show up at test level.

  3. Append mode merge semantics are lost: the previous code returned the full merged property set; the PR returns only the caller supplied keys, and the assertion that verified existing key preservation was removed.

  4. The repr claim doesn't hold: the tags:: ['mcp-test'] bug originates in parser.py (BlockNode.to_batch_format()), not in the property write path. Both setPageProperties and upsertBlockProperty receive values via JSON and serialize them correctly. That bug is fixed in fix: prevent Python repr in block property serialization #53. Your wikilink concern is real though, and it's a third reason tag semantics shouldn't be decided in the property write layer.

A version I'd be glad to merge: gate on graph type. File mode uses upsertBlockProperty on the first block, which is the native file mode representation of page properties, so it stays correct even after upstream fixes setPageProperties. DB mode keeps setPageProperties. Preserve the append and replace semantics, and add tests asserting actual page state for both modes. The db_mode flag currently lives in tools.py and would need plumbing into logseq.py. The upstream behavior itself is tracked in #51.

Happy to help if you want to take this, otherwise I can pick it up and credit your repro.

@ergut

ergut commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Thanks again for the careful repro in #48 and for flagging the wikilink concern. I went ahead and implemented the graph-type-aware version we discussed so this doesn't stall, and credited you as co-author on the commit.

It gates the property write on graph type: DB graphs keep setPageProperties, file graphs use upsertBlockProperty on the first block (the native file-graph representation, so it stays correct even after the upstream setPageProperties issue in #51 is fixed). Append and replace semantics are preserved in both modes, with tests asserting the actual API call sequence per mode and graph type.

Opened it as #62. Happy to fold in any further thoughts you have on the tag semantics.

@ergut ergut closed this Jun 14, 2026
ergut added a commit that referenced this pull request Jun 14, 2026
update_page wrote page properties via setPageProperties for every graph
type. On file graphs that appends a serialized property block to the page
body instead of updating the `key:: value` lines in the first block, so
the properties never register with Logseq's property system.

Branch the property write path on graph type:
  - DB graphs keep setPageProperties (page-entity level, the native
    representation that registers with (page-property ...) queries).
  - File graphs use upsertBlockProperty on the first block, the native
    file-graph representation, updating values in place.

Append vs replace semantics are preserved in both modes:
  - append upserts only the supplied keys (untouched keys survive),
  - replace removes first-block keys absent from the new set before
    upserting (new _replace_page_properties / _remove_block_property).

Plumb the db_mode flag from tools.py into the LogSeq client. Add tests
asserting the actual API call sequence for both modes on both graph types.

Builds on the repro and fix direction from #48 / #49.

Co-authored-by: Nelson Brown <813515+nlsn@users.noreply.github.com>
Co-Authored-By: Claude <noreply@anthropic.com>
ergut added a commit that referenced this pull request Jun 14, 2026
Closes #48. Supersedes #49 (credited @nlsn as co-author for the repro and direction).

## Problem

`update_page` wrote page properties via `setPageProperties` for every graph type. On file graphs that appends a serialized property block to the page body instead of updating the `key:: value` lines in the first block, so the properties never register with Logseq's property system. The success message was misleading.

This is the same area @nlsn surfaced in #48 / #49. #49 fixed it by switching to `upsertBlockProperty` unconditionally, but that regresses DB graphs: block-level properties don't register as page-level there (invisible to `(page-property ...)` queries and the page info panel).

## Fix

Branch the property write path on graph type:

* DB graphs keep `setPageProperties` (page-entity level, the native representation that registers with `(page-property ...)`).
* File graphs use `upsertBlockProperty` on the first block, the native file-graph representation, updating values in place. This stays correct even after the upstream `setPageProperties` behavior in #51 is addressed.

Append and replace semantics are preserved in both modes:

* append upserts only the supplied keys, so untouched keys survive (merge semantics),
* replace removes first-block keys absent from the new set before upserting, via new `_replace_page_properties` / `_remove_block_property`.

The `db_mode` flag is plumbed from `tools.py` into the `LogSeq` client.

## Tests

Rewrote the two property-update tests into four covering append and replace across both graph types, asserting the actual API call sequence (which method is called, on which block, with which keys) rather than only the returned dict. Full suite: 417 passing.

Co-authored-by: Nelson Brown <813515+nlsn@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update_page with properties-only appends properties as body block instead of updating page-level properties in place

2 participants