Use opaque WebDAV writeback source ids - #247
Conversation
|
Warning Review limit reached
More reviews will be available in 41 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMigrate WebDAV account identifiers from numeric primary keys to opaque string ChangesWebDAV opaque source ID
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Frontend
participant API
participant Service
participant DB
Browser->>Frontend: User selects WebDAV account
Frontend->>API: POST /api/webdav/writeback-intent { target_source_id }
API->>Service: determine_webdav_writeback_intent_from_db(organization_id, target_source_id)
Service->>DB: Query webdav_accounts where source_uid and organization_id matches and writeback_enabled = true
DB-->>Service: Return connected accounts with source_uid
Service->>Service: Select eligible account (or error)
Service-->>API: Return intent payload with source_id (opaque string)
API-->>Frontend: Respond with intent metadata
Frontend->>Browser: Render intent metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR governance metadata gate is not ready for
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/plans/2026-05-27-webdav-opaque-source-id.md (1)
27-28: 💤 Low valueConsider listing specific screenshot names for clarity.
For consistency with the other plan documents in this PR (
data-webdav-writeback-intent-ui.mdandself-sent-webdav-materialization-intent.md), consider listing the exact screenshot filenames that reviewers should inspect:
data-webdav-writeback-intent-desktop.pngdata-webdav-writeback-intent-mobile.pngdata-webdav-writeback-intent-mobile-scroll.pngself-sent-knowledge-webdav-intent-desktop.pngself-sent-knowledge-webdav-intent-mobile.pngself-sent-knowledge-webdav-intent-mobile-scroll.png🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-05-27-webdav-opaque-source-id.md` around lines 27 - 28, Update the "Browser evidence" section to list the exact screenshot filenames reviewers should inspect for consistency with the other plan docs: add the entries data-webdav-writeback-intent-desktop.png, data-webdav-writeback-intent-mobile.png, data-webdav-writeback-intent-mobile-scroll.png, self-sent-knowledge-webdav-intent-desktop.png, self-sent-knowledge-webdav-intent-mobile.png, and self-sent-knowledge-webdav-intent-mobile-scroll.png so the Data and Tasks E2E screenshots are explicitly named.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/services/webdav_service.py`:
- Around line 71-87: get_connected_accounts_from_db is ignoring organization_id
and hardcoding writeback_enabled=True; update the DB query to filter
WebdavAccount by organization_id when organization_id is not None and include
the persisted writeback eligibility column from WebdavAccount in the select
(instead of forcing True), then map that column into the returned dict as
"writeback_enabled"; apply the same fix to the other identical mapping in this
file that builds the account dict (the second occurrence that currently sets
writeback_enabled unconditionally).
---
Nitpick comments:
In `@docs/plans/2026-05-27-webdav-opaque-source-id.md`:
- Around line 27-28: Update the "Browser evidence" section to list the exact
screenshot filenames reviewers should inspect for consistency with the other
plan docs: add the entries data-webdav-writeback-intent-desktop.png,
data-webdav-writeback-intent-mobile.png,
data-webdav-writeback-intent-mobile-scroll.png,
self-sent-knowledge-webdav-intent-desktop.png,
self-sent-knowledge-webdav-intent-mobile.png, and
self-sent-knowledge-webdav-intent-mobile-scroll.png so the Data and Tasks E2E
screenshots are explicitly named.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a76e5211-05a2-4d94-9078-eda123d00577
📒 Files selected for processing (18)
AGENTS.mdREADME.mdbackend/api/webdav.pybackend/db/models.pybackend/scripts/bootstrap_db.pybackend/services/webdav_service.pybackend/tests/test_bootstrap_db.pybackend/tests/test_webdav_api.pydocs/operations/source-of-truth-and-writeback-sovereignty.mddocs/plans/2026-05-27-data-webdav-writeback-intent-ui.mddocs/plans/2026-05-27-self-sent-webdav-materialization-intent.mddocs/plans/2026-05-27-webdav-opaque-source-id.mdfrontend/src/app/data/page.test.tsxfrontend/src/app/tasks/page.test.tsxfrontend/src/components/DataLayout.tsxfrontend/src/components/TasksLayout.tsxfrontend/tests/e2e/dashboard-branding.spec.tsfrontend/tests/e2e/helpers.ts
|
Strix current-head evidence for
Following the documented temporary rollback policy, I will remove only the |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/webdav_service.py (1)
221-221: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winFail-closed: Default
writeback_enabledtoFalseinstead ofTrue.Line 221 defaults
writeback_enabledtoTruewhen the key is missing. For fail-closed behavior aligned with the guideline to enforce "persisted writeback eligibility," the default should beFalse. While the DB query (line 90) and mock data (line 35) always include this field, defensive coding should assume missing eligibility means "not writable."🛡️ Proposed fix
- writable_accounts = [ - account for account in accounts if account.get("writeback_enabled", True) - ] + writable_accounts = [ + account for account in accounts if account.get("writeback_enabled", False) + ]As per coding guidelines: "Calendar and WebDAV writeback source selection must resolve through opaque
source_uidvalues, signed-session organization scope, and persisted writeback eligibility, not sequential CalDAV or WebDAV account ids."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/webdav_service.py` at line 221, The list-comprehension filtering WebDAV accounts currently treats a missing "writeback_enabled" key as True; change that default to False so missing eligibility fails closed. Locate the filter that iterates over accounts (the comprehension using account.get("writeback_enabled", True)) and update it to account.get("writeback_enabled", False); ensure any related logic that expects persisted writeback eligibility still uses the "writeback_enabled" key and that tests/mocks are adjusted if they relied on the old default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/services/webdav_service.py`:
- Line 221: The list-comprehension filtering WebDAV accounts currently treats a
missing "writeback_enabled" key as True; change that default to False so missing
eligibility fails closed. Locate the filter that iterates over accounts (the
comprehension using account.get("writeback_enabled", True)) and update it to
account.get("writeback_enabled", False); ensure any related logic that expects
persisted writeback eligibility still uses the "writeback_enabled" key and that
tests/mocks are adjusted if they relied on the old default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c913a9ec-94b9-4a9a-8454-bf699b9186a7
📒 Files selected for processing (15)
AGENTS.mdREADME.mdbackend/api/webdav.pybackend/db/models.pybackend/scripts/bootstrap_db.pybackend/services/webdav_service.pybackend/tests/test_bootstrap_db.pybackend/tests/test_webdav_api.pydocs/operations/source-of-truth-and-writeback-sovereignty.mddocs/plans/2026-05-27-data-webdav-writeback-intent-ui.mddocs/plans/2026-05-27-self-sent-webdav-materialization-intent.mddocs/plans/2026-05-27-webdav-opaque-source-id.mdfrontend/src/app/data/page.test.tsxfrontend/src/components/DataLayout.tsxfrontend/tests/e2e/helpers.ts
✅ Files skipped from review due to trivial changes (5)
- docs/operations/source-of-truth-and-writeback-sovereignty.md
- frontend/src/app/data/page.test.tsx
- AGENTS.md
- docs/plans/2026-05-27-data-webdav-writeback-intent-ui.md
- docs/plans/2026-05-27-self-sent-webdav-materialization-intent.md
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/plans/2026-05-27-webdav-opaque-source-id.md
- README.md
- backend/tests/test_bootstrap_db.py
- backend/scripts/bootstrap_db.py
- backend/db/models.py
- frontend/src/components/DataLayout.tsx
- backend/api/webdav.py
- frontend/tests/e2e/helpers.ts
- backend/tests/test_webdav_api.py
|
Updated Strix evidence for current head
I will temporarily remove only the required |
|
Post-merge restore complete for PR #247: required contexts are back to |
1 participant
Summary
source_uidvalues instead of browser-visible account primary keystarget_account_idpayloads and wire frontend/E2E mocks totarget_source_idwebdav_accounts.source_uid, then document the WebDAV source-id roadmap and copied-review guardrailVerification
PYTHONDONTWRITEBYTECODE=1 DISABLE_BACKGROUND_WORKERS=1 python3 -m pytest backend/tests/test_webdav_api.py backend/tests/test_bootstrap_db.py -qDATABASE_URL=postgresql+asyncpg://test:test@localhost:15544/test_db PYTHONDONTWRITEBYTECODE=1 DISABLE_BACKGROUND_WORKERS=1 python3 -m pytest backend/tests/test_webdav_api.py -k 'real_postgres' -qnpm test -- --run src/app/data/page.test.tsx src/app/tasks/page.test.tsxnpm run typechecknpm run lintenv -u NO_COLOR -u FORCE_COLOR NEXT_TELEMETRY_DISABLED=1 POSTCSS_WORKERS=1 DISABLE_POSTCSS_WORKERS=true NEXT_STATIC_GENERATION_MAX_CONCURRENCY=1 npm run buildenv -u NO_COLOR -u FORCE_COLOR LIVE_BASE_URL=http://127.0.0.1:18139 npm run test:e2e -- --project=desktop --project=mobile -g "data WebDAV|self-sent knowledge|validates mobile hamburger composition"PYTHONDONTWRITEBYTECODE=1 DISABLE_BACKGROUND_WORKERS=1 python3 -m pytest backend/tests/test_webdav_api.py backend/tests/test_bootstrap_db.py backend/tests/test_release_governance.py -qbash scripts/ci/test_pr_governance_gate.shbash scripts/ci/test_strix_quick_gate.shStrix policy
models: readusage.Summary by CodeRabbit
Breaking Changes
target_source_id) and reject legacy numerictarget_account_idpayloads.Behavior
Documentation