Let plop notes be written from the UI (#85 item 5) #123

Merged
steve merged 2 commits from feat/plop-journal-notes into main 2026-07-23 01:42:02 +00:00
2 changed files with 22 additions and 23 deletions
Showing only changes of commit f14875557b - Show all commits
+17 -20
View File
@@ -37,7 +37,7 @@ export function JournalPanel({
objects, objects,
scopeObjectId, scopeObjectId,
onScopeChange, onScopeChange,
scopePlantingId = null, scopePlantingId,
onScopePlantingChange, onScopePlantingChange,
}: { }: {
gardenId: number gardenId: number
@@ -48,17 +48,19 @@ export function JournalPanel({
/** Which bed the panel is filtered to, if any. */ /** Which bed the panel is filtered to, if any. */
scopeObjectId: number | null scopeObjectId: number | null
onScopeChange: (id: number | null) => void onScopeChange: (id: number | null) => void
/** Which single plop the panel is filtered to, if any (#85). Mutually /** Which single plop the panel is filtered to, if any (#85). The store keeps
* exclusive with scopeObjectId — the editor store enforces that. */ * this mutually exclusive with scopeObjectId. Required like its bed twin. */
scopePlantingId?: number | null scopePlantingId: number | null
onScopePlantingChange?: (id: number | null) => void onScopePlantingChange: (id: number | null) => void
Outdated
Review

🟡 onScopePlantingChange is optional while onScopeChange is required — inconsistent API

maintainability · flagged by 1 model

  • web/src/editor/JournalPanel.tsx:54onScopePlantingChange is optional (?:) while onScopeChange is required, yet the component treats scopePlantingId as a first-class scope. The asymmetry means a caller can pass scopePlantingId without a way to clear it (the "Show all" button is gated on onScopePlantingChange). Make onScopePlantingChange required to match onScopeChange, or document why it must be optional.

🪰 Gadfly · advisory

🟡 **onScopePlantingChange is optional while onScopeChange is required — inconsistent API** _maintainability · flagged by 1 model_ - **`web/src/editor/JournalPanel.tsx:54`** — `onScopePlantingChange` is optional (`?:`) while `onScopeChange` is required, yet the component treats `scopePlantingId` as a first-class scope. The asymmetry means a caller can pass `scopePlantingId` without a way to clear it (the "Show all" button is gated on `onScopePlantingChange`). Make `onScopePlantingChange` required to match `onScopeChange`, or document why it must be optional. <sub>🪰 Gadfly · advisory</sub>
}) { }) {
// Date-range narrowing (#85): the backend and JournalFilter already supported // Date-range narrowing (#85): the backend and JournalFilter already supported
// from/to; they just had no UI. Empty inputs don't filter. // from/to; they just had no UI. Empty inputs don't filter.
const [from, setFrom] = useState('') const [from, setFrom] = useState('')
const [to, setTo] = useState('') const [to, setTo] = useState('')
// A plop scope wins over a bed scope — the two are mutually exclusive in the const scopedObject = objects.find((o) => o.id === scopeObjectId) ?? null
// store, but guard here too so the filter is never double-scoped. // One source of scope priority — plop over bed — for both the filter and the
// composer's label, so they can't drift.
const scopeLabel = scopePlantingId != null ? 'this planting' : scopedObject ? objectDisplayName(scopedObject) : null
const filter = { const filter = {
...(scopePlantingId != null ...(scopePlantingId != null
? { plantingId: scopePlantingId } ? { plantingId: scopePlantingId }
@@ -70,7 +72,6 @@ export function JournalPanel({
} }
const journal = useJournal(gardenId, filter) const journal = useJournal(gardenId, filter)
const entries = journal.data?.pages.flatMap((p) => p.entries) ?? [] const entries = journal.data?.pages.flatMap((p) => p.entries) ?? []
const scopedObject = objects.find((o) => o.id === scopeObjectId) ?? null
return ( return (
<div className="flex flex-col gap-3"> <div className="flex flex-col gap-3">
@@ -98,15 +99,13 @@ export function JournalPanel({
{scopePlantingId != null && ( {scopePlantingId != null && (
<div className="flex items-center justify-between gap-2 rounded-md bg-accent/10 px-2 py-1 text-xs"> <div className="flex items-center justify-between gap-2 rounded-md bg-accent/10 px-2 py-1 text-xs">
<span className="text-accent-strong">Notes about one planting</span> <span className="text-accent-strong">Notes about one planting</span>
{onScopePlantingChange && ( <button
<button type="button"
type="button" onClick={() => onScopePlantingChange(null)}
onClick={() => onScopePlantingChange(null)} className="rounded px-1 text-muted outline-none hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40"
className="rounded px-1 text-muted outline-none hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40" >
> Show all
Show all </button>
</button>
)}
</div> </div>
)} )}
@@ -150,9 +149,7 @@ export function JournalPanel({
gardenId={gardenId} gardenId={gardenId}
objectId={scopePlantingId != null ? null : scopeObjectId} objectId={scopePlantingId != null ? null : scopeObjectId}
plantingId={scopePlantingId} plantingId={scopePlantingId}
scopeLabel={ scopeLabel={scopeLabel}
scopePlantingId != null ? 'this planting' : scopedObject ? objectDisplayName(scopedObject) : null
}
/> />
)} )}
Outdated
Review

🟡 Nested ternary for Composer scopeLabel duplicates priority logic from filter

maintainability · flagged by 1 model

  • web/src/editor/JournalPanel.tsx:154 — Nested ternary for scopeLabel in the Composer call repeats the same scoping-priority logic already expressed in filter. A small getScopeLabel(scopePlantingId, scopedObject) helper would keep the logic in one place and avoid drift.

🪰 Gadfly · advisory

🟡 **Nested ternary for Composer scopeLabel duplicates priority logic from filter** _maintainability · flagged by 1 model_ - **`web/src/editor/JournalPanel.tsx:154`** — Nested ternary for `scopeLabel` in the `Composer` call repeats the same scoping-priority logic already expressed in `filter`. A small `getScopeLabel(scopePlantingId, scopedObject)` helper would keep the logic in one place and avoid drift. <sub>🪰 Gadfly · advisory</sub>
+5 -3
View File
@@ -35,7 +35,9 @@ export function PlopInspector({
onChangePlant: () => void onChangePlant: () => void
onClose: () => void onClose: () => void
/** Scope the journal to this plop and open it — the plop parallel of the bed /** Scope the journal to this plop and open it — the plop parallel of the bed
* inspector's "add note" (#85). Absent for a viewer, who can't write notes. */ * inspector's "add note" (#85). Offered to viewers too (to READ the plop's
* notes, like the bed inspector does); the journal's composer is separately
* gated on edit rights, so a viewer just sees the entries. */
onAddNote?: () => void onAddNote?: () => void
readOnly?: boolean readOnly?: boolean
}) { }) {
@@ -176,9 +178,9 @@ export function PlopInspector({
/> />
</fieldset> </fieldset>
Outdated
Review

🟡 PlopInspector hides the note button from viewers entirely, unlike Inspector.tsx which shows a view-only affordance for the same feature despite the code claiming 'parity'

maintainability · flagged by 3 models

  • web/src/editor/PlopInspector.tsx:179 — Despite comments in both PlopInspector.tsx:37-38 and the GardenEditorPage.tsx:506-512 call site explicitly claiming "parity with the bed inspector," the two diverge: Inspector.tsx:145 renders its note button unconditionally ({onAddNote && (...)}), giving read-only viewers a view-only affordance (a note count, or "📝 No notes"), while PlopInspector.tsx:179 gates the equivalent button on !readOnly, hiding it entirely for viewers. Both call site…

🪰 Gadfly · advisory

🟡 **PlopInspector hides the note button from viewers entirely, unlike Inspector.tsx which shows a view-only affordance for the same feature despite the code claiming 'parity'** _maintainability · flagged by 3 models_ - `web/src/editor/PlopInspector.tsx:179` — Despite comments in both `PlopInspector.tsx:37-38` and the `GardenEditorPage.tsx:506-512` call site explicitly claiming "parity with the bed inspector," the two diverge: `Inspector.tsx:145` renders its note button unconditionally (`{onAddNote && (...)}`), giving read-only viewers a view-only affordance (a note count, or "📝 No notes"), while `PlopInspector.tsx:179` gates the equivalent button on `!readOnly`, hiding it entirely for viewers. Both call site… <sub>🪰 Gadfly · advisory</sub>
{!readOnly && onAddNote && ( {onAddNote && (
<Button variant="ghost" className="justify-start px-2 py-1 text-sm" onClick={onAddNote}> <Button variant="ghost" className="justify-start px-2 py-1 text-sm" onClick={onAddNote}>
📓 Add a note about this plant 📓 {readOnly ? 'Notes about this plant' : 'Add a note about this plant'}
</Button> </Button>
)} )}