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
4 changed files with 75 additions and 9 deletions
+40 -8
View File
@@ -37,6 +37,8 @@ export function JournalPanel({
objects,
scopeObjectId,
onScopeChange,
scopePlantingId,
onScopePlantingChange,
}: {
gardenId: number
canEdit: boolean
@@ -46,19 +48,30 @@ export function JournalPanel({
/** Which bed the panel is filtered to, if any. */
scopeObjectId: number | null
onScopeChange: (id: number | null) => void
/** Which single plop the panel is filtered to, if any (#85). The store keeps
* this mutually exclusive with scopeObjectId. Required like its bed twin. */
scopePlantingId: number | null
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
// from/to; they just had no UI. Empty inputs don't filter.
const [from, setFrom] = useState('')
const [to, setTo] = useState('')
const scopedObject = objects.find((o) => o.id === scopeObjectId) ?? null
// 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 = {
...(scopeObjectId != null ? { objectId: scopeObjectId } : {}),
...(scopePlantingId != null
? { plantingId: scopePlantingId }
: scopeObjectId != null
? { objectId: scopeObjectId }
: {}),
...(from ? { from } : {}),
...(to ? { to } : {}),
}
const journal = useJournal(gardenId, filter)
const entries = journal.data?.pages.flatMap((p) => p.entries) ?? []
const scopedObject = objects.find((o) => o.id === scopeObjectId) ?? null
return (
<div className="flex flex-col gap-3">
@@ -83,6 +96,19 @@ export function JournalPanel({
)}
</div>
{scopePlantingId != null && (
<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>
<button
type="button"
onClick={() => onScopePlantingChange(null)}
className="rounded px-1 text-muted outline-none hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40"
>
Show all
</button>
</div>
)}
<div className="flex items-center gap-2 text-xs text-muted">
<label className="flex items-center gap-1">
<span>From</span>
@@ -121,8 +147,9 @@ export function JournalPanel({
{canEdit && (
<Composer
gardenId={gardenId}
objectId={scopeObjectId}
scopeLabel={scopedObject ? objectDisplayName(scopedObject) : null}
objectId={scopePlantingId != null ? null : scopeObjectId}
plantingId={scopePlantingId}
scopeLabel={scopeLabel}
/>
)}
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>
@@ -133,9 +160,11 @@ export function JournalPanel({
{journal.isSuccess && entries.length === 0 && (
<p className="text-sm text-muted">
{scopedObject
? `Nothing written about ${objectDisplayName(scopedObject)} yet.`
: 'Nothing written yet. This is where what happened goes — when something went in, what came up, what the frost got. Next year you get to read it back.'}
{scopePlantingId != null
? 'Nothing written about this planting yet.'
: scopedObject
? `Nothing written about ${objectDisplayName(scopedObject)} yet.`
: 'Nothing written yet. This is where what happened goes — when something went in, what came up, what the frost got. Next year you get to read it back.'}
</p>
)}
@@ -174,10 +203,13 @@ export function JournalPanel({
function Composer({
gardenId,
objectId,
plantingId = null,
scopeLabel,
}: {
gardenId: number
objectId: number | null
/** When set, the note attaches to this plop rather than a bed (#85). */
plantingId?: number | null
scopeLabel: string | null
}) {
const create = useCreateJournalEntry(gardenId)
@@ -191,7 +223,7 @@ function Composer({
if (!text) return
setError(null)
create.mutate(
{ body: text, observedAt, objectId: objectId ?? undefined },
{ body: text, observedAt, objectId: objectId ?? undefined, plantingId: plantingId ?? undefined },
{
onSuccess: () => {
setBody('')
+12
View File
@@ -25,6 +25,7 @@ export function PlopInspector({
unit,
onChangePlant,
onClose,
onAddNote,
readOnly = false,
}: {
plop: EditorPlanting
@@ -33,6 +34,11 @@ export function PlopInspector({
unit: UnitPref
onChangePlant: () => void
onClose: () => void
/** Scope the journal to this plop and open it — the plop parallel of the bed
* 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
readOnly?: boolean
}) {
const update = useUpdatePlanting(gardenId)
@@ -172,6 +178,12 @@ export function PlopInspector({
/>
</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>
{onAddNote && (
<Button variant="ghost" className="justify-start px-2 py-1 text-sm" onClick={onAddNote}>
📓 {readOnly ? 'Notes about this plant' : 'Add a note about this plant'}
</Button>
)}
{!readOnly && (
<Button
variant="ghost"
+12 -1
View File
@@ -59,6 +59,12 @@ interface EditorState {
journalObjectId: number | null
setJournalObjectId: (id: number | null) => void
// Which single plop the journal is filtered to, if any — the parallel of
Review

Mutual-exclusivity invariant is restated in three places (interface, setter, JournalPanel guard)

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **Mutual-exclusivity invariant is restated in three places (interface, setter, JournalPanel guard)** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
// journalObjectId for a planting (#85). The two scopes are mutually exclusive
// (the setters clear each other), so the journal filter is never ambiguous.
journalPlantingId: number | null
setJournalPlantingId: (id: number | null) => void
// The plant armed for placing plops (set after the PlantPicker choice); stays
// armed for repeat-placement until cleared (Escape / done). null = not placing.
armedPlant: Plant | null
@@ -116,7 +122,11 @@ export const useEditorStore = create<EditorState>((set) => ({
setSeasonYear: (year) => set({ seasonYear: year }),
journalObjectId: null,
setJournalObjectId: (id) => set({ journalObjectId: id }),
// Scoping to a bed clears any plop scope, so only one is ever active.
setJournalObjectId: (id) => set({ journalObjectId: id, journalPlantingId: null }),
journalPlantingId: null,
setJournalPlantingId: (id) => set({ journalPlantingId: id, journalObjectId: null }),
armedPlant: null,
armedLotId: null,
@@ -147,6 +157,7 @@ export const useEditorStore = create<EditorState>((set) => ({
railTab: null,
seasonYear: null,
journalObjectId: null,
journalPlantingId: null,
mode: DEFAULT_MODE,
}),
}))
+11
View File
@@ -85,6 +85,8 @@ export function GardenEditorPage() {
const setRailTab = useEditorStore((s) => s.setRailTab)
const journalObjectId = useEditorStore((s) => s.journalObjectId)
const setJournalObjectId = useEditorStore((s) => s.setJournalObjectId)
const journalPlantingId = useEditorStore((s) => s.journalPlantingId)
const setJournalPlantingId = useEditorStore((s) => s.setJournalPlantingId)
const journalCounts = useJournalCounts(gid)
const capabilities = useCapabilities()
const journalTotal = useMemo(
@@ -501,6 +503,13 @@ export function GardenEditorPage() {
readOnly={!canEdit}
onChangePlant={() => setPicker('change')}
onClose={() => selectPlanting(null)}
onAddNote={() => {
// Parity with the bed inspector: scope the journal to this plop and
// open it. The plop stays selected, so the selection effect keeps the
// inspector reachable when you switch back.
setJournalPlantingId(selectedPlop.id)
setRailTab('journal')
}}
/>
) : (
<p className="text-sm text-muted">Select a bed or a planting to edit it.</p>
@@ -521,6 +530,8 @@ export function GardenEditorPage() {
objects={objects}
scopeObjectId={journalObjectId}
onScopeChange={setJournalObjectId}
scopePlantingId={journalPlantingId}
onScopePlantingChange={setJournalPlantingId}
/>
),
},