Skip to content

fix: repair Notes folder desired keys#211

Merged
timlaing merged 2 commits intotimlaing:mainfrom
MrJarnould:codex/fix-notes-folders
Apr 3, 2026
Merged

fix: repair Notes folder desired keys#211
timlaing merged 2 commits intotimlaing:mainfrom
MrJarnould:codex/fix-notes-folders

Conversation

@MrJarnould
Copy link
Copy Markdown

Summary

This fixes a Notes service bug in NotesService.folders().

folders() referenced NotesDesiredKey.TITLE_MODIFICATION_DATE and NotesDesiredKey.HAS_SUBFOLDER, but those enum members do not exist. That caused folder listing to raise AttributeError before the query could complete.

This change fixes the crash by:

  • removing the invalid desired-key enum references from NotesService.folders()
  • requesting the supported encrypted title field and the optional raw HasSubfolder field directly
  • treating the subfolder flag as optional when Apple omits it
  • adding focused regression coverage for the desired-key set and missing subfolder metadata

Impact

  • folder queries no longer fail before reaching CloudKit
  • callers can list note folders even when the subfolder flag is absent from the payload
  • this is a service-layer fix and does not depend on the new Notes CLI branch

Validation

  • UV_CACHE_DIR=/tmp/uv-cache PATH=$HOME/.local/bin:$PATH UV_NO_EDITABLE=1 uv run --no-sync ruff check pyicloud/services/notes/service.py tests/test_notes.py
  • UV_CACHE_DIR=/tmp/uv-cache PATH=$HOME/.local/bin:$PATH UV_NO_EDITABLE=1 uv run --no-sync python -m pytest tests/test_notes.py -q
  • manual smoke test: notes folders stopped crashing when this fix was applied onto the CLI branch

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 31, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 809b6c0b-dd69-4abf-9307-07d99b8a02df

📥 Commits

Reviewing files that changed from the base of the PR and between faea578 and 5eb9802.

📒 Files selected for processing (1)
  • pyicloud/services/notes/service.py
✅ Files skipped from review due to trivial changes (1)
  • pyicloud/services/notes/service.py

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Folder subfolder indicator is now treated as optional — folders without that metadata yield an unknown/nullable state rather than a default false value.
  • Tests

    • Added tests for folder listing to verify the service requests the subfolder metadata when available and correctly handles records that omit it.

Walkthrough

The NotesService.folders() method now requests the HasSubfolder field by its string name and maps the returned value to a tri-state has_subfolders (True / False / None) instead of defaulting to False when the field is absent.

Changes

Cohort / File(s) Summary
Service Implementation
pyicloud/services/notes/service.py
Added module-level constant _HAS_SUBFOLDER_FIELD and changed folders() to request "HasSubfolder" by string. Updated extraction to use rec.fields.get_value(_HAS_SUBFOLDER_FIELD) and set has_subfolders to None when the field is missing; otherwise coerce to bool.
Test Coverage
tests/test_notes.py
Added two tests: one asserting raw.query is called with desired_keys=["TitleEncrypted","HasSubfolder"] and parsing when present (has_subfolders=True), and another verifying missing HasSubfolder yields has_subfolders=None while preserving other fields.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I nibble at fields both small and wide,
Found "HasSubfolder" by name, not guide,
True, False, or None — a trio to keep,
No more a default that woke from sleep,
Hooray for tidy folders, soft and neat! 🥕✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and concisely describes the main change: fixing invalid desired-key enum references in the NotesService.folders() method.
Description check ✅ Passed The description is directly related to the changeset, clearly explaining the bug, the fix, the impact, and validation performed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Pylint (4.0.5)
pyicloud/services/notes/service.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pyicloud/services/notes/service.py (1)

280-285: Use get_value() directly for HasSubfolder extraction.

Line 280-Line 284 can be simplified without changing behaviour; get_value() already returns None when the field is missing, which preserves your tri-state handling.

♻️ Proposed refactor
-                    has_sub_value = getattr(
-                        rec.fields.get_field(_HAS_SUBFOLDER_FIELD) or (),
-                        "value",
-                        None,
-                    )
+                    has_sub_value = rec.fields.get_value(_HAS_SUBFOLDER_FIELD)
                     has_sub = None if has_sub_value is None else bool(has_sub_value)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pyicloud/services/notes/service.py` around lines 280 - 285, Replace the
getattr-based extraction with a direct call to the fields helper: call
rec.fields.get_value(_HAS_SUBFOLDER_FIELD) to get the raw value (it already
returns None if the field is missing) and then set has_sub = None if that
returned value is None else bool(value); this removes the getattr(...) "value"
lookup and keeps the tri-state semantics for has_sub.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pyicloud/services/notes/service.py`:
- Around line 280-285: Replace the getattr-based extraction with a direct call
to the fields helper: call rec.fields.get_value(_HAS_SUBFOLDER_FIELD) to get the
raw value (it already returns None if the field is missing) and then set has_sub
= None if that returned value is None else bool(value); this removes the
getattr(...) "value" lookup and keeps the tri-state semantics for has_sub.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6be23549-661f-41e5-9cc2-ecccfd9b4425

📥 Commits

Reviewing files that changed from the base of the PR and between 404b0d7 and faea578.

📒 Files selected for processing (2)
  • pyicloud/services/notes/service.py
  • tests/test_notes.py

coderabbitai[bot]
coderabbitai bot previously approved these changes Mar 31, 2026
@MrJarnould MrJarnould marked this pull request as ready for review March 31, 2026 15:47
@timlaing timlaing self-assigned this Apr 3, 2026
@timlaing timlaing added the bug Something isn't working label Apr 3, 2026
@timlaing timlaing merged commit 172dd79 into timlaing:main Apr 3, 2026
12 of 13 checks passed
@timlaing
Copy link
Copy Markdown
Owner

timlaing commented Apr 3, 2026

LGTM: Thanks for your update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants