Skip to content

Stop requiring the window on every window save - #107

Merged
feruzm merged 1 commit into
mainfrom
fix/curation-window-fields
Sep 22, 2026
Merged

feruzm merged 1 commit into
mainfrom
fix/curation-window-fields

Conversation

@feruzm

@feruzm feruzm commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

Pairs with ecency/esync-py's fix of the same name, which ships first.

open was mandatory on application-window, so a save about the message or the numbers had to carry it, and sending a stale one could undo another admin's close and reopen applications. Every field on that settings row is optional upstream now, and absent means leave it.

A present open must still be a boolean. A call carrying none of the four fields is refused here rather than forwarded as a no-op, which is what the backend does with it too.

Verification

  • 583 tests green; new cases pin that a message-only and a quorum-only body pass with exactly their own keys, and that an empty body is refused.
  • Two mutations: keeping open required, and allowing the empty body, each turn tests red.

Summary by CodeRabbit

  • Bug Fixes
    • Curation desk saves can now update message or quorum without including the window’s open setting.
    • Empty save requests are now rejected with a clearer “nothing to set” error.
    • Existing open validation remains in place when that setting is provided.

`open` was mandatory here, so a save about the message or the numbers had to
carry it, and sending a stale one could undo another admin's close and
reopen applications. Every field on that settings row is optional upstream
now, and absent means leave it.

A present `open` must still be a boolean. A call carrying none of the four
fields is refused rather than forwarded as a no-op, which is what the
backend does with it too.
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Allow partial application window settings updates

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Makes application-window updates patch-like, allowing saves that omit open.
• Validates supplied open values and rejects empty window updates.
• Adds regression coverage for partial forwarding and empty-payload rejection.
Diagram

graph TD
  A["Admin Save"] --> B["Window Validator"] --> C{"Open present?"}
  C -->|Yes| D{"Boolean?"} -->|No| R["Reject Request"]
  C -->|No| E{"Any setting?"}
  D -->|Yes| E -->|Yes| F["Forward Fields"]
  E -->|No| R
Loading
High-Level Assessment

The patch-style validation matches the upstream contract and is the safest approach: omitted fields remain unchanged, preventing stale open values from reversing another administrator's action. Retaining full-payload saves was dismissed because it preserves the concurrency race, while versioned full-object updates would require disproportionate cross-system changes.

Files changed (2) +27 / -3

Bug fix (1) +12 / -1
PrivateApi.CurationDesk.csPermit partial application-window settings saves +12/-1

Permit partial application-window settings saves

• Makes 'open' optional while preserving boolean validation whenever it is supplied. Rejects requests containing none of the four supported settings, preventing empty no-op calls from reaching the upstream service.

dotnet/EcencyApi/Handlers/PrivateApi.CurationDesk.cs

Tests (1) +15 / -2
CurationDeskPayloadTests.csCover partial and empty application-window payloads +15/-2

Cover partial and empty application-window payloads

• Updates invalid-payload expectations so malformed supplied 'open' values remain rejected while an empty payload returns 'nothing to set'. Adds regression cases proving message-only and quorum-only saves forward exactly their supplied setting.

dotnet/EcencyApi.Tests/CurationDeskPayloadTests.cs

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Parity docs omit the input change 📘 Rule violation ▣ Testability
Description
Validate now accepts application-window bodies without open and rejects an empty body, but
this PR adds no dedicated update to KNOWN_DIVERGENCES. Because the route remains covered only by
the generic curation-desk exclusion, parity reviewers cannot distinguish the new partial-update
contract from the existing route-level divergence.
Code

dotnet/EcencyApi/Handlers/PrivateApi.CurationDesk.cs[R1290-1291]

+            if (body.ContainsKey("open")
+                && body.Field("open")?.GetValueKind() is not (JsonValueKind.True or JsonValueKind.False))
Evidence
Compliance rule 2667942 requires every observable endpoint input change to include both matching
tests and a new or updated parity divergence entry. The handler changes whether `POST
/private-api/curation-desk/application-window accepts bodies without open, while driver.py`
still assigns that endpoint only the unchanged generic curation-desk divergence description.

Rule 2667942: Require tests and parity divergence docs for observable endpoint behavior changes
dotnet/EcencyApi/Handlers/PrivateApi.CurationDesk.cs[1286-1299]
dotnet/parity/driver.py[244-272]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The application-window endpoint now permits partial settings updates without `open` and rejects an empty body, but the parity divergence documentation was not updated to record this observable input-contract change.
## Fix Focus Areas
- dotnet/parity/driver.py[244-272]
## Recommended Fix
Add a dedicated `KNOWN_DIVERGENCES` reason for the application-window cases that explicitly identifies the endpoint, notes that the reference image has no matching route, and documents the current partial-update and empty-body behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread dotnet/EcencyApi/Handlers/PrivateApi.CurationDesk.cs
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f4d09845-19cc-4226-a777-96f36693f9dc

📥 Commits

Reviewing files that changed from the base of the PR and between d7ada7e and 0e4bbd6.

📒 Files selected for processing (2)
  • dotnet/EcencyApi.Tests/CurationDeskPayloadTests.cs
  • dotnet/EcencyApi/Handlers/PrivateApi.CurationDesk.cs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

ApplicationWindow saves now support message-only and quorum-only updates. The handler rejects empty bodies with "nothing to set". Tests cover partial forwarding and updated validation expectations.

Changes

ApplicationWindow save validation

Layer / File(s) Summary
Partial save validation
dotnet/EcencyApi/Handlers/PrivateApi.CurationDesk.cs, dotnet/EcencyApi.Tests/CurationDeskPayloadTests.cs
The handler checks open only when present and rejects empty updates with "nothing to set". Tests verify message-only and quorum-only forwarding, plus the updated empty-body response.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 0e4bb

Partial saves can update message or quorum without changing omitted fields, while empty updates remain rejected. No production-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: window saves no longer require the window state on every request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

A rabbit saves one field today
The window keeps its state away
Message hops, quorum follows
Empty paws bring clear errors
No closing change slips through
The curation desk stays true

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

@feruzm
feruzm merged commit b06d1ff into main Sep 22, 2026
4 checks passed
@feruzm
feruzm deleted the fix/curation-window-fields branch September 22, 2026 18:52
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.

1 participant