Closes#85 item 5. journal_entries.planting_id was modelled, accepted by the API, and the JournalPanel already rendered a "planting" badge for such entries — but nothing in the UI ever created one. A rendered badge for a state the UI couldn't produce.
Give a plop the same "add note" affordance the bed inspector already has:
PlopInspector gains an onAddNote button — "📓 Add a note about this plant" — shown only to an editor (a viewer can't write notes anyway).
The editor store gains a journalPlantingId scope beside journalObjectId. The two are mutually exclusive — each setter clears the other — so the journal filter is never double-scoped.
JournalPanel filters by plantingId when that scope is set, shows a "Notes about one planting · Show all" banner, and its composer attaches new notes to the plop. Empty-state copy updated to match.
Two taps from a selected plant to typing, mirroring the bed flow (the plop stays selected, so switching back to its inspector still works). No backend change — the API already accepted plantingId.
Closes #85 item 5. `journal_entries.planting_id` was modelled, accepted by the API, and the `JournalPanel` already rendered a **"planting"** badge for such entries — but nothing in the UI ever created one. A rendered badge for a state the UI couldn't produce.
Give a plop the same "add note" affordance the bed inspector already has:
- **`PlopInspector`** gains an `onAddNote` button — *"📓 Add a note about this plant"* — shown only to an editor (a viewer can't write notes anyway).
- **The editor store** gains a `journalPlantingId` scope beside `journalObjectId`. The two are **mutually exclusive** — each setter clears the other — so the journal filter is never double-scoped.
- **`JournalPanel`** filters by `plantingId` when that scope is set, shows a *"Notes about one planting · Show all"* banner, and its composer attaches new notes to the plop. Empty-state copy updated to match.
Two taps from a selected plant to typing, mirroring the bed flow (the plop stays selected, so switching back to its inspector still works). **No backend change** — the API already accepted `plantingId`.
`tsc`, `vitest` (108), and `vite build` green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
journal_entries.planting_id was modelled, accepted by the API, and the
JournalPanel already rendered a "planting" badge for such entries — but
nothing in the UI ever created one. A badge for a state the UI couldn't
produce.
Give the plop the same "add note" affordance the bed inspector has:
- PlopInspector gains an onAddNote button ("📓 Add a note about this
plant"), shown only to an editor (a viewer can't write notes).
- The editor store gains a journalPlantingId scope beside journalObjectId.
The two are mutually exclusive — each setter clears the other — so the
journal filter is never double-scoped.
- JournalPanel filters by plantingId when that scope is set, shows a
"Notes about one planting · Show all" banner, and its composer attaches
new notes to the plop. Empty-state copy updated to match.
Two taps from a selected plant to typing, mirroring the bed flow. No
backend change — the API already accepted plantingId.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-23 01:33:54Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡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.
🪰 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>
🟡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>
🟡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>
Verdict: Minor issues · 8 findings (2 with multi-model agreement)
Finding
Where
Models
Lens
🟡
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'
web/src/editor/PlopInspector.tsx:179
3/5
maintainability
🟡
Removing a plop doesn't clear journalPlantingId, leaving the journal composer scoped to (and able to attach new notes to) a soft-removed planting
web/src/editor/PlopInspector.tsx:190
2/5
error-handling
6 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
Bed-scope dropdown shows 'Whole garden' and silently clears the planting scope when a planting scope is active
web/src/editor/JournalPanel.tsx:79
opencode/glm-5.2:cloud
correctness, maintainability
🟡
onScopePlantingChange is optional while onScopeChange is required — inconsistent API
web/src/editor/JournalPanel.tsx:54
kimi-k2.6:cloud
maintainability
🟡
Nested ternary for Composer scopeLabel duplicates priority logic from filter
web/src/editor/JournalPanel.tsx:154
kimi-k2.6:cloud
maintainability
🟡
Triple-nested ternary in empty-state message hurts readability
web/src/editor/JournalPanel.tsx:165
kimi-k2.6:cloud
maintainability
⚪
The 'plop scope wins over bed scope' precedence is re-derived as a nested ternary in three inline call sites instead of one shared helper
web/src/editor/JournalPanel.tsx:62
glm-5.2:cloud
maintainability
⚪
Mutual-exclusivity invariant is restated in three places (interface, setter, JournalPanel guard)
web/src/editor/store.ts:62
opencode/glm-5.2:cloud
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
This confirms canEdit is derived from server-provided garden role data (consistent with the rest of the file's usage), and the backend independently enforces roleEditor on CreateJournalEntry plus validates that plantingId actually belongs to the target garden (resolveJournalTarget in internal/service/journal.go:141-179) and the list query is always scoped by garden_id (internal/store/journal.go:82). So client-side gating of the "Add a note" button is convenience UI only, not a security boundary, and the boundary itself is sound and untouched by this diff. No backend code changed here, and the frontend changes are just state wiring (mutually-exclusive scope IDs, a filter param, static label text) — nothing that touches authn/authz, injection, deserialization, or secrets.
VERDICT: No material issues found
No security-relevant defects in this diff. The newly-exposed plantingId journal scope was already properly authorized server-side before this PR (create validates the planting belongs to the garden; list is always scoped by garden_id), so wiring it up in the UI doesn't introduce an IDOR or authz gap. All new code is client-side state/UI plumbing with no injection sinks, no secret handling, and no new trust boundary.
🎯 Correctness — No material issues found
That's fine. I've thoroughly traced the data flow front-to-back (store → JournalPanel → Composer → useCreateJournalEntry → API → service.resolveJournalTarget) and the mutual-exclusion/selection-effect interactions in GardenEditorPage. Everything checks out.
VERDICT: No material issues found
Traced the full path for correctness:
Store mutual exclusion (store.ts:126,129): setJournalObjectId/setJournalPlantingId each clear the other, and resetTransient clears both. Verified the one pre-existing call site that resets scope (selectMode('journal') at GardenEditorPage.tsx:258, calling setJournalObjectId(null)) transitively clears journalPlantingId too via the updated setter — no stale scope leaks through that path.
Selection-effect interaction: The effect at GardenEditorPage.tsx:224-234 that force-switches railTab to 'inspector' is keyed only off selectedId/selectedPlantingId, not railTab. Since onAddNote doesn't change the plop selection, calling setRailTab('journal') from it isn't immediately clobbered back to 'inspector' — the PR's claim about this holds.
Filter/composer wiring: JournalFilter/JournalInput in lib/journal.ts already declare plantingId?: number, matching what JournalPanel/Composer now send. objectId ?? undefined / plantingId ?? undefined correctly use nullish coalescing (no falsy-zero bug, though IDs won't be 0 anyway).
Backend already supports it: internal/service/journal.go's resolveJournalTarget fills in the parent object from the planting and validates it belongs to the garden; internal/api/journal.go already decodes plantingId on both read and write paths, confirmed by existing tests in journal_test.go. No backend changes were needed, matching the PR description.
Mutually-exclusive UI: filter construction in JournalPanel.tsx:62-70 correctly prioritizes plantingId over objectId with the same guard the store already enforces — belt-and-braces, not conflicting logic.
No logic bugs, no incorrect constants/thresholds in this diff (it introduces no numeric constants), no broken cross-file wiring.
🧹 Code cleanliness & maintainability — Minor issues
Both call sites pass onAddNote unconditionally (regardless of canEdit/readOnly). Confirmed: Inspector (bed) shows the button always when onAddNote is passed — including for readOnly viewers, rendering "📝 No notes" or a count as a view-only affordance (Inspector.tsx:145-153). PlopInspector gates the identical button on !readOnly (PlopInspector.tsx:179), hiding it entirely from viewers, despite the code/comments explicitly claiming parity with the bed inspector. The draft finding is accurate.
VERDICT: Minor issues
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 sites (GardenEditorPage.tsx:489-495 and :506-512) pass onAddNote unconditionally regardless of canEdit, so the difference is solely in each inspector's own render condition — not a missing prop. A viewer can browse a bed's notes count/empty-state but has no equivalent way to discover a plop has notes.
⚡ Performance — No material issues found
This is a small, well-contained UI addition (a button, a store scope, a filter branch). Nothing in the diff touches loops over data, query pagination, or introduces new fetches beyond the existing useJournal/useJournalCounts pattern, which is unchanged in shape (just one more optional filter key, consistent with how objectId/from/to already work). The filter object recreated per render was already the pattern before this PR (react-query structurally hashes query keys, so this isn't a regression). No new loops, no N+1 queries, no unbounded growth.
VERDICT: No material issues found
No performance-relevant regressions in this diff — it's a straightforward plumbing addition (new store field, one button, one filter branch) that reuses the existing journal query/pagination machinery unchanged.
🧯 Error handling & edge cases — Minor issues
All draft claims check out against the actual source. The single finding is confirmed: removing a plop closes the inspector via selectPlanting(null) but never clears journalPlantingId, leaving the journal composer scoped to (and able to attach new notes to) a soft-removed planting.
VERDICT: Minor issues
web/src/editor/PlopInspector.tsx:190-193 — "Remove plant" calls onClose() (→ selectPlanting(null), wired at GardenEditorPage.tsx:505) then soft-removes the plop via remove.mutate, but nothing clears journalPlantingId (store.ts:128-129). If a note was scoped to that plop via the new "📓 Add a note about this plant" button (PlopInspector.tsx:179-183, wired at GardenEditorPage.tsx:506-512), the journal stays filtered to the now-removed planting's id: the "Notes about one planting" banner and composer (JournalPanel.tsx:98-111,148-157) keep pointing at a plant that no longer exists in the plop list, and the composer will attach new notes to the removed plantingId with no warning. selectPlanting (store.ts:113) only sets selectedPlantingId/selectedId, never touching journalPlantingId; setJournalPlantingId is invoked only from the "Add a note" handler (GardenEditorPage.tsx:510) and the panel's own "Show all" button, so nothing resets it on removal. This mirrors a pre-existing identical gap on the bed side (Inspector.tsx:305-307 deletes an object via select(null) + del.mutate without touching journalObjectId either, and select() at store.ts:110 doesn't touch it), so it's a known class of gap rather than a regression unique to this PR — worth fixing but not blocking.
Verified the write path end-to-end from a security lens: PlopInspector.onAddNote only renders when !readOnly (i.e., editor only — viewers can't reach it, GardenEditorPage.tsx:503), the composer only renders when canEdit (JournalPanel{canEdit && (...)}), and the mutation useCreateJournalEntry posts the typed plantingId straight through to /gardens/{gid}/journal with no client-side authorization over the planting id beyond canEdit. That matches the existing bed (objectId) flow, which the PR description notes relies on the backend. No client-side IDOR mitigation was added for plantingId, but none exists for objectId either — this is parity, not a regression, and backend authz is out of scope for a UI-only change. No new injection/SSRF/deserialization surface (the plantingId is a number from the store, typed and passed as a JSON body field, never interpolated into a path or query). The store's mutual-exclusion guards (store.ts:125-128) correctly prevent a double-scoped filter being sent to the API.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change end-to-end through the correctness lens:
Mutual exclusion of scopes (store.ts:125-129): setJournalObjectId clears journalPlantingId and vice versa. JournalPanel.tsx:62-67 additionally guards so plantingId wins in the filter. No double-scoped filter is reachable.
Filter shape (JournalPanel.tsx:62-70, journal.ts:33-38): JournalFilter declares plantingId?: number, and useJournal spreads the filter into the API params (journal.ts:54). The planting-scoped query is well-formed.
Composer attachment (JournalPanel.tsx:150-156, 229): when scopePlantingId is set, objectId is forced to null and plantingId is passed; the mutation sends plantingId: plantingId ?? undefined against JournalInput (journal.ts:79-84), which accepts it. A bed-scoped note still attaches objectId. Correct.
"Show all" reset (JournalPanel.tsx:104 → setJournalPlantingId(null)): clears both scopes (store setter nulls journalObjectId too), so the panel returns to whole-garden. Correct.
Bed <select> interaction (JournalPanel.tsx:80-94): its onChange calls setJournalObjectId, which clears any planting scope — so picking a bed from the dropdown while a planting banner is shown correctly drops the planting scope. No stale state.
PlopInspector wiring (GardenEditorPage.tsx:506-512): selectedPlop.id is the EditorPlanting id, matching journal_entries.planting_id. The plop stays selected (selectPlanting untouched), so the inspector remains reachable after switching to the journal tab. Parity with the bed flow (Inspector.tsx:145, GardenEditorPage.tsx:489-494) holds.
Store reset (store.ts:160): journalPlantingId is cleared on reset alongside journalObjectId. No dangling scope.
No logic bugs, incorrect constants, or semantic mismatches found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/editor/PlopInspector.tsx:180 — the plop inspector's "Add note" button diverges from the bed inspector's affordance, dropping the note-count affordance the established pattern has.Inspector.tsx:145-153 (the bed inspector) renders a button that shows either 📝 {n} notes / 📝 No notes / 📝 Add note depending on noteCount and readOnly, styled w-full text-sm. The new plop button (PlopInspector.tsx:179-183) instead always renders the fixed label 📓 Add a note about this plant and uses a different emoji (📓 vs 📝) and different button styling (justify-start px-2 py-1 vs w-full). PlopInspector.tsx has no noteCount prop at all (grep confirms zero matches), so a plop with existing notes shows no count indicator on its inspector, and the icon/label/styling differ from the bed inspector for no functional reason. The PR description frames this as "parity with the bed flow," but the parity is only partial. Suggested fix: mirror the bed inspector — pass a noteCount, or at minimum reuse the 📝 glyph and the 📝 {n} notes / 📝 No notes / 📝 Add note label logic and the w-full text-sm styling so the two inspectors read as one family.
web/src/editor/JournalPanel.tsx:62-70 — the scopePlantingId != null ? { plantingId } : scopeObjectId != null ? { objectId } : {} chain has no shared helper for "pick the active scope." This is a minor readability nit, not a bug: the same scopePlantingId != null / scopeObjectId != null precedence is re-derived inline in three places in this file — the filter at lines 62-67, the ComposerobjectId/scopeLabel at lines 151-154, and the empty-state copy at lines 166-170. Each is a small nested ternary, and together they re-encode the "plop wins, else bed, else none" rule independently. A single activeScope helper (or const plantingScoped = scopePlantingId != null) would let each call site read the rule once instead of re-deriving it. Low churn, optional.
No dead/duplicated code beyond the above, no naming or structural problems in store.ts (the mutually-exclusive setters and the reset update are clean and follow the existing pattern), and the GardenEditorPage wiring matches the bed-inspector wiring.
⚡ Performance — No material issues found
No material issues found
Verified the journal data path (web/src/lib/journal.ts:46-59): useJournal is an infinite query keyed by the full filter object (now including plantingId), paginated at PAGE_SIZE = 30. Scoping to a plop produces a distinct, separately cached query — no full re-fetch of unbounded lists, no N+1. The mutation invalidates the garden's journal keys broadly (invalidateJournal), which is the same pre-existing strategy; adding plantingId doesn't widen that blast radius meaningfully.
The filter object is reconstructed every render (a new reference), but react-query hashes query keys structurally (journalKey returns a tuple containing the filter), so this does not cause refetch storms. This pattern pre-existed for scopeObjectId/from/to; the PR only adds one more conditional field.
objects.find((o) => o.id === scopeObjectId) runs each render but operates on the beds list (small, not the plantings list) and is unchanged by this PR.
No new allocations on a hot path, no loops added, no missing pagination.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
Stale journalPlantingId after the plop is removed — web/src/editor/PlopInspector.tsx:190-193 / GardenEditorPage.tsx:506-512. The "Add a note about this plant" button (onAddNote) sets journalPlantingId = selectedPlop.id and switches to the Journal tab, while leaving the plop selected. The user can switch back to the inspector and hit "Remove plant", which calls remove.mutate({ id: plop.id, ... }) + onClose() → selectPlanting(null). Neither path clears journalPlantingId: onClose only resets selectedPlantingId, and the store's remove/deletion flow does not touch the journal scope. resetTransient (which does clear journalPlantingId) only fires on garden change/leave, not on plop removal. Result: after the plant is deleted, the journal panel stays scoped to a dead planting — the "Notes about one planting · Show all" banner persists, useJournal keeps filtering on a non-existent plantingId (empty list → "Nothing written about this planting yet."), and the composer POSTs new notes with a plantingId pointing at a row that no longer exists. Suggested fix: clear the scope on remove (e.g. in the remove handler, when the removed id equals the scoped id, call setJournalPlantingId(null)), mirroring how setJournalObjectId clears journalPlantingId for mutual exclusivity. (Confidence: medium — confirmed via reading PlopInspector.tsx, GardenEditorPage.tsx, and store.ts; the only assumption is the backend treats a note keyed to a removed planting as orphaned rather than rejecting it, which doesn't affect the UI-side staleness.)
The PR is a straightforward UI-only change that wires up an already-existing backend capability. From a security lens, everything checks out:
Write authorization is preserved. The backend CreateJournalEntry requires roleEditor and validates the plantingId belongs to the garden via resolveJournalTarget (verified in internal/service/journal.go:102-180). The UI additionally gates the "Add note" button behind !readOnly (which is readOnly={!canEdit} in GardenEditorPage.tsx), so viewers cannot reach the composer even if they bypassed React rendering.
Mutual exclusivity of scopes is enforced in depth. The store setters clear each other (setJournalObjectId nullifies journalPlantingId and vice versa), the JournalPanel filter construction prioritizes plantingId over objectId, and the Composer receives at most one non-null target. The backend also defensively rejects mismatched objectId + plantingId pairs.
No new injection surfaces. The plantingId value flows from the user's own canvas selection (selectedPlop.id) through typed props to the API client; there is no user-supplied free text that could be injected into SQL, commands, or templates.
No authz gaps introduced. Scoping the journal list to a planting only requires garden viewer role, which is the existing permission model. The PR does not change the list endpoint.
🎯 Correctness — No material issues found
No material issues found
I traced the new data flows carefully and found no correctness issues.
Mutual exclusivity is enforced both in the store (setJournalObjectId clears journalPlantingId and vice‑versa) and defensively in the panel filter (planting scope wins if both were somehow present).
Composer payload correctly sends objectId: null and plantingId: <id> when scoped to a planting, so the backend receives the intended association.
Query invalidation in useCreateJournalEntry uses the prefix ['gardens', gardenId, 'journal'], which covers the filtered useJournal query keys — new planting notes appear in the scoped list as expected.
Conditional rendering (canEdit, readOnly, onAddNote) is wired correctly; the button is only shown to editors and the callback is only invoked when the plop inspector is in writable mode.
Empty-state copy and composer scopeLabel branch on scopePlantingId consistently.
Nothing in the diff introduces a logic bug or semantically incorrect behavior.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/JournalPanel.tsx:165 — Triple-nested ternary for the empty-state copy is hard to scan and will get worse if another scope is added. Extract to a helper variable or small function.
// Current:
{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.'}
Suggested fix: compute const emptyMessage = ... above the JSX, or add a getEmptyMessage(scopePlantingId, scopedObject) helper.
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.
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.
⚡ Performance — No material issues found
No material issues found
Verified the new scopePlantingId prop and filter construction in JournalPanel.tsx:62-70 — the inline object recreation is identical to the pre-existing pattern for scopeObjectId, and React Query's key serialization handles it correctly.
Verified the cache invalidation in journal.ts:89-92 — invalidateJournal broadly invalidates all journal queries for the garden, which was already the behavior before this PR; the comment explicitly notes this is intentional trade-off for a short list.
Verified the Zustand store setters in store.ts:125-128 — atomic updates prevent double-renders when scoping switches between bed and plop.
The onAddNote handler in GardenEditorPage.tsx:506-511 batches two store updates, which React will coalesce into a single render cycle.
🧯 Error handling & edge cases — No material issues found
No material issues found
Mutual exclusivity: Verified in store.ts:126-129 that setJournalObjectId and setJournalPlantingId correctly clear each other, and JournalPanel.tsx:62-70 guards the filter so only one scope ever wins. resetTransient at store.ts:160 also clears both.
Null/optional safety: scopePlantingId defaults to null at JournalPanel.tsx:40, and onScopePlantingChange is checked before rendering the "Show all" button at line 101. The Composer submit safely coerces both IDs with ?? undefined at JournalPanel.tsx:229.
Viewer access: PlopInspector.tsx:179 correctly gates the new button with !readOnly && onAddNote, so viewers never see an "add note" affordance they can't use. The Composer itself is already gated by canEdit at JournalPanel.tsx:148.
State transitions: Changing the bed dropdown while scoped to a planting correctly triggers setJournalObjectId, which clears the planting scope via the store. Switching gardens calls resetTransient, clearing both scopes.
The PR is a pure frontend wiring change; the backend authorization was already in place and is unchanged. I verified the security-relevant paths:
internal/service/journal.go:102-127 — CreateJournalEntry calls requireGardenRole(..., roleEditor) then resolveJournalTarget, which (lines 141-180) loads the planting via GetPlanting, loads its parent object, and rejects with ErrInvalidInput if the object's GardenID doesn't match the request's gardenID. So a forged plantingId from another garden is rejected; the new "Add a note about this plant" button can't be used to write into a garden the actor can't edit.
internal/service/journal.go:54-85 — ListJournal requires roleViewer on the garden before applying the PlantingID filter, so the new scopePlantingId filter can't be used to enumerate entries in a garden the viewer can't see.
web/src/lib/journal.ts:79-101 — JournalInput.plantingId is typed number and flows through api.post to the existing validated endpoint; no string interpolation into a URL or query, so no injection surface introduced.
The scopePlantingId/journalPlantingId values originate from selectedPlop.id in GardenEditorPage.tsx (a value already loaded under the garden's view authorization), not from any URL/param/unsanitized input.
No authn/authz gap, no injection, no secret handling, no untrusted-data sink introduced by this change.
🎯 Correctness — Minor issues
Verified. The bed-scope <select> at JournalPanel.tsx:80-95 keeps value={scopeObjectId ?? ''} regardless of planting scope, and the store (store.ts:126,129) keeps the two scopes mutually exclusive, so when a planting scope is active scopeObjectId is null and the dropdown reads "Whole garden". Selecting that option fires onScopeChange(null) → setJournalObjectId(null), which clears journalPlantingId (line 126), silently dropping the planting scope.
Verdict: Minor issues
web/src/editor/JournalPanel.tsx:80-95 — When a planting scope is active (scopePlantingId != null), the bed-scope <select> still renders with value={scopeObjectId ?? ''}, and scopeObjectId is guaranteed null at that point (the store's setJournalPlantingId clears journalObjectId at store.ts:129). The dropdown therefore displays "Whole garden" as the selected option while the panel is actually filtered to a single planting — the displayed selection misrepresents the real filter state. Worse, because the planting scope is not reflected in the dropdown's value, a user who opens the dropdown and picks the seemingly-already-selected "Whole garden" option triggers onScopeChange(null) → setJournalObjectId(null), which (per store.ts:126) clears journalPlantingId — silently dropping the planting scope they were viewing. Suggested fix: hide or disable the bed-scope <select> while scopePlantingId != null (the "Notes about one planting · Show all" banner already owns that state), or render a dedicated disabled option so the control reflects reality.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
JournalPanel.tsx:79-95 — the bed-scope <select> stays visible while a planting scope is active. When scopePlantingId != null, setJournalPlantingId clears journalObjectId (store.ts:129), so scopeObjectId becomes null and the dropdown renders value={scopeObjectId ?? ''} → '' = "Whole garden". Meanwhile the list below is filtered to one planting (JournalPanel.tsx:62-67, where scopePlantingId wins) and a "Notes about one planting" banner sits right under the dropdown (JournalPanel.tsx:98-111). Two contradictory scope indicators at once: the dropdown says "whole garden", the banner says "one planting". The filter logic hides the contradiction from the query, but a reader of the UI sees a stale "Whole garden" control beside an active planting filter. Suggested fix: hide or disable the bed <select> while scopePlantingId != null, mirroring how the banner only renders in that state. Confirmed by reading JournalPanel.tsx:79-111 and store.ts:124-129.
PlopInspector.tsx:179-183 vs Inspector.tsx:145-153 — affordance parity gap with the bed inspector's note button. The bed inspector's onAddNote button surfaces noteCount (📝 N notes / 📝 Add note / 📝 No notes, Inspector.tsx:145-153), so a bed with existing notes is discoverable from the inspector without opening the journal. The plop button is a static 📓 Add a note about this plant with no count (PlopInspector.tsx:179-183), so a plop that already has notes looks identical to one with none — the PR's stated goal is parity with the bed inspector's "add note" affordance, and this is half of that affordance. There's no per-planting counts endpoint (useJournalCounts in journal.ts:69-77 keys by object id only), so the data isn't there today, but the asymmetry is a cleanliness rough edge worth a follow-up. Also the emoji differs (📓 here vs 📝 in Inspector.tsx:148-151) for the same conceptual action — minor, but pick one so the two inspectors read as siblings.
store.ts:62-66 and JournalPanel.tsx:51-52,60-61 — the "mutually exclusive" invariant is restated three times. The interface comment (store.ts:62-64), the setter comment (store.ts:125), and the JournalPanel guard comment (JournalPanel.tsx:51-52 and 60-61) all repeat the same mutual-exclusivity explanation. The store is the actual source of truth; the JournalPanel guard (scopePlantingId wins over scopeObjectId) is defensive duplication that the store already guarantees. Not wrong, but the redundant guard comment ("the two are mutually exclusive in the store, but guard here too") could just reference the store invariant once rather than re-explaining it. Trivial.
⚡ Performance — No material issues found
VERDICT: No material issues found
I reviewed the change through the performance lens and verified the relevant code paths.
JournalPanel.tsx:62-70 — the filter object is rebuilt each render, but useJournal (web/src/lib/journal.ts:42-58) feeds it into a React Query queryKey that is hashed (deep-equal by content), so the new object identity does not cause refetches. Verified the query layer.
JournalPanel.tsx:71 — pagination is unchanged (PAGE_SIZE = 30, infinite query), no unbounded fetch introduced by the planting scope.
store.ts:122-127 — the two setters each perform a single set call clearing the sibling field; no extra work, no loop. reset correctly includes journalPlantingId.
GardenEditorPage.tsx:506-512 — onAddNote does two store writes (setJournalPlantingId + setRailTab); each is one Zustand set. No re-render storm (selectors are granular and unchanged).
No new queries, no N+1, no per-render allocations on a hot path, no backend round-trips added.
Nothing in the performance lane is materially wrong with this change.
🧯 Error handling & edge cases — No material issues found
Now let me check the service layer for validation around plantingId/objectId mutual exclusivity and planting existence checks.
Now let me check the resetTransient and other places that might need to clear journalPlantingId, and the bed-inspector flow for parity (does deselecting a bed clear its scope?).
Now let me check the soft-remove scenario for plantings: does GetPlanting return removed plantings? Could a journal entry be created against a soft-removed planting? Let me check the removed_at handling and whether that's an edge case the diff introduces.
Now I have a thorough picture. Let me focus on the error-handling & edge cases lens and verify the key concerns:
The bed inspector's onAddNote is gated by onAddNote presence AND respects readOnly for the button rendering — wait, actually line 145 shows {onAddNote && ( — it renders even for readOnly viewers (showing "📝 No notes"). The PlopInspector uses {!readOnly && onAddNote && ( — so it's hidden for viewers. Let me verify the GardenEditorPage passes onAddNote to ObjectInspector unconditionally vs conditionally for PlopInspector.
Let me check whether anything clears journalPlantingId when the selected plop is removed or when the plantings list no longer contains it.
Now I understand the architecture well. Let me trace the key edge case: the journal scope for a planting survives the plop's removal. Let me verify how useJournal handles a plantingId filter pointing at a non-existent/removed planting, and whether useCreateJournalEntry against a removed planting is rejected.
The key concern: soft-removed plantings. resolveJournalTarget calls GetPlanting which does NOT filter by removed_at IS NULL — so a journal entry CAN be created against a soft-removed planting. But that's a pre-existing backend behavior, not introduced by this diff. The diff only wires the UI to existing backend.
Let me focus on what the diff actually introduces and check for genuine edge cases in the new UI code. Let me verify the season view interaction — selectedPlop comes from plantings which is derived from full.data?.plantings which is the season-scoped set when seasonYear !== null. So in a past season, plantings contains historical plops (including removed ones). The "Add note" button is gated by canEdit which is false in past seasons, so it won't show. Good.
But there's a subtle issue: the onAddNote for PlopInspector uses selectedPlop.id directly. If the plop is soft-removed in the LIVE view... actually in live view, ListActivePlantingsForGarden filters removed_at IS NULL, so removed plops don't appear in plantings, so selectedPlop would be null and the inspector wouldn't render. So no issue there.
Let me check one more edge case: the resetTransient clears journalPlantingId on garden change, good. But does the JournalPanel's "Show all" button and the bed-scope <select> properly interact? When scopePlantingId != null, the bed <select> still shows value={scopeObjectId ?? ''}. Since the store keeps them mutually exclusive, scopeObjectId is null when scopePlantingId is set, so the select shows "Whole garden". That's slightly confusing UX but not an error-handling bug.
Let me check the date filter reset when switching scopes — not really an error handling issue.
Let me look at one potential real issue: the Composer's plantingId is passed but when scope is the whole garden (both null), plantingId is null and objectId is null — fine. When scopePlantingId is set, objectId becomes null and plantingId is set — correct.
Let me verify the useJournal query key behavior when switching between scopes rapidly — the filter object changes, creating new query keys, old ones get disposed. No leak concern.
Now let me check whether there's a stale journal scope issue: if you scope to a plop, then that plop gets deleted (hard delete via revision undo), journalPlantingId still holds the stale id. The journal query would return empty (no entries match), the composer would try to create an entry against a non-existent planting → backend returns ErrInvalidInput → handled by onError. So error is surfaced. Not a swallowed error.
Let me double-check there's no issue with the onScopePlantingChange being optional and the banner button.
Now let me check whether selectMode('journal') (the mobile mode bar journal tap) clears the plop scope. Line 258 only calls setJournalObjectId(null) which (per the store) ALSO clears journalPlantingId. Good — mutual exclusion holds.
But the desktop "Journal" button at line 585 calls setJournalObjectId(null) which clears planting scope too. Good.
Let me verify the test file to see if there are tests covering the new planting scope and edge cases.
No tests cover the new behavior, but that's a test-coverage concern, not strictly error-handling.
Let me verify one final edge case: what happens when selectedPlop becomes null mid-flow (e.g., the plop is removed via the "Remove plant" button while journal is scoped to it). The "Remove plant" button calls onClose() → selectPlanting(null), which clears selectedPlantingId but does NOT clear journalPlantingId. So the journal stays scoped to a now-removed planting id. Let me verify.
I have enough context. Now let me consider the error-handling & edge cases lens specifically. The diff is purely UI wiring to an existing backend. Let me assess the genuine edge cases:
Stale journalPlantingId after plop removal: If a user scopes the journal to a plop (via "Add a note about this plant"), then from the journal panel switches back to the inspector and clicks "Remove plant" — selectPlanting(null) fires (clearing selection) but journalPlantingId retains the now-removed planting's id. The journal stays scoped to a ghost. Entries written would target a soft-removed planting (backend accepts — GetPlanting doesn't filter removed_at), and the list filter would still match existing entries. This is a mild stale-state edge case but not a crash or swallowed error — the journal simply shows the removed plop's notes. Actually the backend rejects? Let me verify: resolveJournalTarget calls GetPlanting which returns the row even if removed_at is set. So a note CAN be written against a soft-removed planting. The FK is ON DELETE CASCADE so a hard-deleted planting would reject (FK violation → 500). This is pre-existing backend behavior though, not introduced by this diff.
This is a minor edge case but within my lens (stale state after removal). It's a real, if minor, issue: the journal scope survives the plop's removal with no cleanup, leaving a confusing "Notes about one planting" banner pinned to a plop the user just removed and can no longer reselect.
Let me confirm there's no cleanup mechanism I'm missing.
Confirmed: journalPlantingId is only cleared by: setting a bed scope, setting it to null explicitly (the "Show all" button), or resetTransient (garden switch). It is NOT cleared when the plop is removed or deselected. Compare with journalObjectId: the bed equivalent has the same property (not cleared on object deletion either), so this is consistent with existing behavior — not a regression introduced by this diff, but a parity-brought-along edge case.
This is genuinely minor and pre-existing in pattern. Let me assess severity. The bed equivalent has the same behavior (deleting a bed doesn't clear journalObjectId). So this is consistent parity, not a new defect. I'll note it as a trivial observation rather than a blocking finding, since it matches the established pattern and the PR explicitly aims for parity.
Let me check one more thing — whether the onAddNote callback in PlopInspector could fire when selectedPlop is stale (race). The callback closes over selectedPlop.id at render time; since selectedPlop is derived from selectedPlantingId and plantings, and the callback is recreated each render, it's always current. No stale closure issue.
Let me verify the Composer empty-body guard and the plantingId ?? undefined handling — already saw it. if (!text) return guards empty, and plantingId ?? undefined correctly omits when null. No issue.
Let me check the journal query when plantingId points to a planting in another garden (the "leak" the service guards against). The list query filters by garden_id AND planting_id, so cross-garden ids return empty — no leak. The create path is guarded by resolveJournalTarget. Good.
I've thoroughly verified. The diff is clean from an error-handling/edge-case perspective. The only observation is the stale-scope-after-removal parity issue, which is consistent with the existing bed-scope behavior and not a regression. Let me write up the final review.
VERDICT: No material issues found
The diff is purely frontend wiring over an already-validated backend. Through the error-handling/edge-case lens I checked the unhappy paths the change touches and found nothing material:
Mutual exclusion is enforced in both layers.setJournalObjectId and setJournalPlantingId each clear the other (store.ts:126,129), and JournalPanel re-guards with the scopePlantingId != null ? … : scopeObjectId != null ? … ternary (JournalPanel.tsx:62-67), so the filter is never double-scoped. resetTransient clears both on garden switch (store.ts:159-160), and the mobile selectMode('journal') + desktop Journal button both route through setJournalObjectId(null), which transitively clears the plop scope.
The composer correctly omits the id fields when null.objectId: objectId ?? undefined, plantingId: plantingId ?? undefined (JournalPanel.tsx:229) — a null scope produces a garden-level entry, not one with objectId: 0 or similar. Empty-body is still guarded by if (!text) return before the mutate.
The create error path is surfaced, not swallowed.onError: (err) => setError(errorMessage(err, "Couldn't save that note.")) (JournalPanel.tsx:235) renders the failure inline. I verified the backend (service/journal.go:141-179) rejects mismatched/cross-garden plantingId with ErrInvalidInput (→ 400), and a hard-deleted planting would FK-fail; both reach the UI as an error string.
Stale-scope-after-removal is consistent with the bed flow, not a regression. Removing the scoped plop (PlopInspector "Remove plant" → onClose() → selectPlanting(null)) does not clear journalPlantingId, so the "Notes about one planting" banner can linger on a plop the user can no longer reselect. I grepped every clearer of journalPlantingId (only the bed setter, the explicit "Show all", and resetTransient) and confirmed the bed equivalent has the identical property (journalObjectId is not cleared on object deletion either). This is parity-brought-along, not introduced here — and it self-heals the moment the user picks "Show all" or scopes to a bed. Calling it a finding would be re-reporting the existing pattern.
Past-season / read-only gating holds.canEdit is false when seasonYear !== null (GardenEditorPage.tsx:145-146), and the button is behind {!readOnly && onAddNote && …} (PlopInspector.tsx:179), so a viewer or past-season user can't reach the composer. The Composer itself is also gated by {canEdit && …} in JournalPanel.
I verified all of the above by reading JournalPanel.tsx, PlopInspector.tsx, store.ts, GardenEditorPage.tsx, lib/journal.ts, lib/objects.ts, internal/api/journal.go, internal/service/journal.go, and internal/store/journal.go.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
I reviewed the diff through the Security lens, tracing both the new frontend path and the existing backend it calls. Here's what I checked:
Authorization: The new onAddNote callback on PlopInspector is gated by !readOnly (GardenEditorPage.tsx:506), and readOnly is set from !canEdit. The backend CreateJournalEntry already requires roleEditor (internal/service/journal.go:103) and validates the planting belongs to the requesting garden via resolveJournalTarget (internal/service/journal.go:141-180), including a cross-check against the parent bed. No authz bypass introduced.
Injection: No new SQL or command construction. The frontend passes plantingId as a JSON field. The backend store layer uses ? placeholders (internal/store/journal.go:50-53) for both read and write paths.
Input validation: The listJournal handler already parses plantingId through optionalIDQuery, which rejects non-positive integers (internal/api/journal.go:55-58). The create path's resolveJournalTarget validates existence and garden membership before writing.
Secret leakage / unsafe deserialization / SSRF: Not introduced by this change.
🎯 Correctness — No material issues found
No material issues found
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The change is clean and follows existing patterns well:
The PlopInspector mirrors the bed Inspector's onAddNote callback pattern exactly — same optional signature, same caller-side wiring in GardenEditorPage.
The store's mutual-exclusion design (journalObjectId / journalPlantingId) is consistent with the existing selectedId / selectedPlantingId pattern, and the resetTransient clearance is complete.
JournalPanel's filter construction is defensive (plop scope wins, with a local guard even though the store enforces mutual exclusion), and the conditional UI states (banner, composer label, empty-state copy) are all accounted for.
The Composer internal prop extension (plantingId) is local and doesn't leak into the public JournalPanel interface beyond what's needed.
⚡ Performance — No material issues found
No material issues found
🧯 Error handling & edge cases — No material issues found
No material issues found.
After tracing every code path touched by this PR through the lens of error handling and edge cases:
Mutual exclusivity is enforced at the store level.setJournalObjectId always clears journalPlantingId, and vice versa. Call sites in selectMode, the desktop Journal button, and the Journal panel dropdown all correctly use these setters, so a double-scope is impossible.
The Composer only ever sends one scope. When scopePlantingId is active, objectId is explicitly passed as null and converted to undefined in the mutation payload, so the API receives only plantingId.
Null/empty collections are handled. The scopePlantingId prop defaults to null, the optional onScopePlantingChange is present at the sole call site, and the empty-state copy branches correctly.
Filter priority is defensive. The filter construction gives plantingId precedence over objectId as a guard, matching the store's mutual-exclusion invariant.
Reset and lifecycle paths clear both scopes.resetTransient includes journalPlantingId: null, and the garden-switch mount effect calls resetTransient.
Nothing in this diff introduces an ignored error, missing cleanup, unhandled nil, boundary condition, or stale-scope bug.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 8 findings (2 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | 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' | `web/src/editor/PlopInspector.tsx:179` | 3/5 | maintainability |
| 🟡 | Removing a plop doesn't clear journalPlantingId, leaving the journal composer scoped to (and able to attach new notes to) a soft-removed planting | `web/src/editor/PlopInspector.tsx:190` | 2/5 | error-handling |
<details><summary>6 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Bed-scope dropdown shows 'Whole garden' and silently clears the planting scope when a planting scope is active | `web/src/editor/JournalPanel.tsx:79` | opencode/glm-5.2:cloud | correctness, maintainability |
| 🟡 | onScopePlantingChange is optional while onScopeChange is required — inconsistent API | `web/src/editor/JournalPanel.tsx:54` | kimi-k2.6:cloud | maintainability |
| 🟡 | Nested ternary for Composer scopeLabel duplicates priority logic from filter | `web/src/editor/JournalPanel.tsx:154` | kimi-k2.6:cloud | maintainability |
| 🟡 | Triple-nested ternary in empty-state message hurts readability | `web/src/editor/JournalPanel.tsx:165` | kimi-k2.6:cloud | maintainability |
| ⚪ | The 'plop scope wins over bed scope' precedence is re-derived as a nested ternary in three inline call sites instead of one shared helper | `web/src/editor/JournalPanel.tsx:62` | glm-5.2:cloud | maintainability |
| ⚪ | Mutual-exclusivity invariant is restated in three places (interface, setter, JournalPanel guard) | `web/src/editor/store.ts:62` | opencode/glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
This confirms `canEdit` is derived from server-provided garden role data (consistent with the rest of the file's usage), and the backend independently enforces `roleEditor` on `CreateJournalEntry` plus validates that `plantingId` actually belongs to the target garden (`resolveJournalTarget` in `internal/service/journal.go:141-179`) and the list query is always scoped by `garden_id` (`internal/store/journal.go:82`). So client-side gating of the "Add a note" button is convenience UI only, not a security boundary, and the boundary itself is sound and untouched by this diff. No backend code changed here, and the frontend changes are just state wiring (mutually-exclusive scope IDs, a filter param, static label text) — nothing that touches authn/authz, injection, deserialization, or secrets.
VERDICT: No material issues found
No security-relevant defects in this diff. The newly-exposed `plantingId` journal scope was already properly authorized server-side before this PR (create validates the planting belongs to the garden; list is always scoped by `garden_id`), so wiring it up in the UI doesn't introduce an IDOR or authz gap. All new code is client-side state/UI plumbing with no injection sinks, no secret handling, and no new trust boundary.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
That's fine. I've thoroughly traced the data flow front-to-back (store → JournalPanel → Composer → useCreateJournalEntry → API → service.resolveJournalTarget) and the mutual-exclusion/selection-effect interactions in GardenEditorPage. Everything checks out.
## VERDICT: No material issues found
Traced the full path for correctness:
- **Store mutual exclusion** (`store.ts:126,129`): `setJournalObjectId`/`setJournalPlantingId` each clear the other, and `resetTransient` clears both. Verified the one pre-existing call site that resets scope (`selectMode('journal')` at `GardenEditorPage.tsx:258`, calling `setJournalObjectId(null)`) transitively clears `journalPlantingId` too via the updated setter — no stale scope leaks through that path.
- **Selection-effect interaction**: The effect at `GardenEditorPage.tsx:224-234` that force-switches `railTab` to `'inspector'` is keyed only off `selectedId`/`selectedPlantingId`, not `railTab`. Since `onAddNote` doesn't change the plop selection, calling `setRailTab('journal')` from it isn't immediately clobbered back to `'inspector'` — the PR's claim about this holds.
- **Filter/composer wiring**: `JournalFilter`/`JournalInput` in `lib/journal.ts` already declare `plantingId?: number`, matching what `JournalPanel`/`Composer` now send. `objectId ?? undefined` / `plantingId ?? undefined` correctly use nullish coalescing (no falsy-zero bug, though IDs won't be 0 anyway).
- **Backend already supports it**: `internal/service/journal.go`'s `resolveJournalTarget` fills in the parent object from the planting and validates it belongs to the garden; `internal/api/journal.go` already decodes `plantingId` on both read and write paths, confirmed by existing tests in `journal_test.go`. No backend changes were needed, matching the PR description.
- **Mutually-exclusive UI**: filter construction in `JournalPanel.tsx:62-70` correctly prioritizes `plantingId` over `objectId` with the same guard the store already enforces — belt-and-braces, not conflicting logic.
No logic bugs, no incorrect constants/thresholds in this diff (it introduces no numeric constants), no broken cross-file wiring.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both call sites pass `onAddNote` unconditionally (regardless of `canEdit`/`readOnly`). Confirmed: `Inspector` (bed) shows the button always when `onAddNote` is passed — including for readOnly viewers, rendering "📝 No notes" or a count as a view-only affordance (Inspector.tsx:145-153). `PlopInspector` gates the identical button on `!readOnly` (PlopInspector.tsx:179), hiding it entirely from viewers, despite the code/comments explicitly claiming parity with the bed inspector. The draft finding is accurate.
VERDICT: Minor issues
- `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 sites (`GardenEditorPage.tsx:489-495` and `:506-512`) pass `onAddNote` unconditionally regardless of `canEdit`, so the difference is solely in each inspector's own render condition — not a missing prop. A viewer can browse a bed's notes count/empty-state but has no equivalent way to discover a plop has notes.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
This is a small, well-contained UI addition (a button, a store scope, a filter branch). Nothing in the diff touches loops over data, query pagination, or introduces new fetches beyond the existing `useJournal`/`useJournalCounts` pattern, which is unchanged in shape (just one more optional filter key, consistent with how `objectId`/`from`/`to` already work). The `filter` object recreated per render was already the pattern before this PR (react-query structurally hashes query keys, so this isn't a regression). No new loops, no N+1 queries, no unbounded growth.
VERDICT: No material issues found
No performance-relevant regressions in this diff — it's a straightforward plumbing addition (new store field, one button, one filter branch) that reuses the existing journal query/pagination machinery unchanged.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All draft claims check out against the actual source. The single finding is confirmed: removing a plop closes the inspector via `selectPlanting(null)` but never clears `journalPlantingId`, leaving the journal composer scoped to (and able to attach new notes to) a soft-removed planting.
**VERDICT: Minor issues**
- `web/src/editor/PlopInspector.tsx:190-193` — "Remove plant" calls `onClose()` (→ `selectPlanting(null)`, wired at `GardenEditorPage.tsx:505`) then soft-removes the plop via `remove.mutate`, but nothing clears `journalPlantingId` (`store.ts:128-129`). If a note was scoped to that plop via the new "📓 Add a note about this plant" button (`PlopInspector.tsx:179-183`, wired at `GardenEditorPage.tsx:506-512`), the journal stays filtered to the now-removed planting's id: the "Notes about one planting" banner and composer (`JournalPanel.tsx:98-111,148-157`) keep pointing at a plant that no longer exists in the plop list, and the composer will attach *new* notes to the removed `plantingId` with no warning. `selectPlanting` (`store.ts:113`) only sets `selectedPlantingId`/`selectedId`, never touching `journalPlantingId`; `setJournalPlantingId` is invoked only from the "Add a note" handler (`GardenEditorPage.tsx:510`) and the panel's own "Show all" button, so nothing resets it on removal. This mirrors a pre-existing identical gap on the bed side (`Inspector.tsx:305-307` deletes an object via `select(null)` + `del.mutate` without touching `journalObjectId` either, and `select()` at `store.ts:110` doesn't touch it), so it's a known class of gap rather than a regression unique to this PR — worth fixing but not blocking.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
- Verified the write path end-to-end from a security lens: `PlopInspector.onAddNote` only renders when `!readOnly` (i.e., editor only — viewers can't reach it, `GardenEditorPage.tsx:503`), the composer only renders when `canEdit` (`JournalPanel` `{canEdit && (...)}`), and the mutation `useCreateJournalEntry` posts the typed `plantingId` straight through to `/gardens/{gid}/journal` with no client-side authorization over the planting id beyond `canEdit`. That matches the existing bed (`objectId`) flow, which the PR description notes relies on the backend. No client-side IDOR mitigation was added for `plantingId`, but none exists for `objectId` either — this is parity, not a regression, and backend authz is out of scope for a UI-only change. No new injection/SSRF/deserialization surface (the `plantingId` is a `number` from the store, typed and passed as a JSON body field, never interpolated into a path or query). The store's mutual-exclusion guards (`store.ts:125-128`) correctly prevent a double-scoped filter being sent to the API.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Verdict: No material issues found
I verified the change end-to-end through the correctness lens:
- **Mutual exclusion of scopes** (`store.ts:125-129`): `setJournalObjectId` clears `journalPlantingId` and vice versa. `JournalPanel.tsx:62-67` additionally guards so `plantingId` wins in the filter. No double-scoped filter is reachable.
- **Filter shape** (`JournalPanel.tsx:62-70`, `journal.ts:33-38`): `JournalFilter` declares `plantingId?: number`, and `useJournal` spreads the filter into the API params (`journal.ts:54`). The planting-scoped query is well-formed.
- **Composer attachment** (`JournalPanel.tsx:150-156, 229`): when `scopePlantingId` is set, `objectId` is forced to `null` and `plantingId` is passed; the mutation sends `plantingId: plantingId ?? undefined` against `JournalInput` (`journal.ts:79-84`), which accepts it. A bed-scoped note still attaches `objectId`. Correct.
- **"Show all" reset** (`JournalPanel.tsx:104` → `setJournalPlantingId(null)`): clears both scopes (store setter nulls `journalObjectId` too), so the panel returns to whole-garden. Correct.
- **Bed `<select>` interaction** (`JournalPanel.tsx:80-94`): its `onChange` calls `setJournalObjectId`, which clears any planting scope — so picking a bed from the dropdown while a planting banner is shown correctly drops the planting scope. No stale state.
- **PlopInspector wiring** (`GardenEditorPage.tsx:506-512`): `selectedPlop.id` is the `EditorPlanting` id, matching `journal_entries.planting_id`. The plop stays selected (`selectPlanting` untouched), so the inspector remains reachable after switching to the journal tab. Parity with the bed flow (`Inspector.tsx:145`, `GardenEditorPage.tsx:489-494`) holds.
- **Store reset** (`store.ts:160`): `journalPlantingId` is cleared on reset alongside `journalObjectId`. No dangling scope.
No logic bugs, incorrect constants, or semantic mismatches found in this lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
VERDICT: Minor issues
- **`web/src/editor/PlopInspector.tsx:180` — the plop inspector's "Add note" button diverges from the bed inspector's affordance, dropping the note-count affordance the established pattern has.** `Inspector.tsx:145-153` (the bed inspector) renders a button that shows either `📝 {n} notes` / `📝 No notes` / `📝 Add note` depending on `noteCount` and `readOnly`, styled `w-full text-sm`. The new plop button (`PlopInspector.tsx:179-183`) instead always renders the fixed label `📓 Add a note about this plant` and uses a different emoji (📓 vs 📝) and different button styling (`justify-start px-2 py-1` vs `w-full`). `PlopInspector.tsx` has no `noteCount` prop at all (grep confirms zero matches), so a plop with existing notes shows no count indicator on its inspector, and the icon/label/styling differ from the bed inspector for no functional reason. The PR description frames this as "parity with the bed flow," but the parity is only partial. Suggested fix: mirror the bed inspector — pass a `noteCount`, or at minimum reuse the `📝` glyph and the `📝 {n} notes` / `📝 No notes` / `📝 Add note` label logic and the `w-full text-sm` styling so the two inspectors read as one family.
- **`web/src/editor/JournalPanel.tsx:62-70` — the `scopePlantingId != null ? { plantingId } : scopeObjectId != null ? { objectId } : {}` chain has no shared helper for "pick the active scope."** This is a minor readability nit, not a bug: the same `scopePlantingId != null` / `scopeObjectId != null` precedence is re-derived inline in three places in this file — the `filter` at lines 62-67, the `Composer` `objectId`/`scopeLabel` at lines 151-154, and the empty-state copy at lines 166-170. Each is a small nested ternary, and together they re-encode the "plop wins, else bed, else none" rule independently. A single `activeScope` helper (or `const plantingScoped = scopePlantingId != null`) would let each call site read the rule once instead of re-deriving it. Low churn, optional.
No dead/duplicated code beyond the above, no naming or structural problems in `store.ts` (the mutually-exclusive setters and the `reset` update are clean and follow the existing pattern), and the `GardenEditorPage` wiring matches the bed-inspector wiring.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
- Verified the journal data path (`web/src/lib/journal.ts:46-59`): `useJournal` is an infinite query keyed by the full `filter` object (now including `plantingId`), paginated at `PAGE_SIZE = 30`. Scoping to a plop produces a distinct, separately cached query — no full re-fetch of unbounded lists, no N+1. The mutation invalidates the garden's journal keys broadly (`invalidateJournal`), which is the same pre-existing strategy; adding `plantingId` doesn't widen that blast radius meaningfully.
- The `filter` object is reconstructed every render (a new reference), but react-query hashes query keys structurally (`journalKey` returns a tuple containing the filter), so this does not cause refetch storms. This pattern pre-existed for `scopeObjectId`/`from`/`to`; the PR only adds one more conditional field.
- `objects.find((o) => o.id === scopeObjectId)` runs each render but operates on the beds list (small, not the plantings list) and is unchanged by this PR.
- No new allocations on a hot path, no loops added, no missing pagination.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
## VERDICT: Minor issues
- **Stale `journalPlantingId` after the plop is removed** — `web/src/editor/PlopInspector.tsx:190-193` / `GardenEditorPage.tsx:506-512`. The "Add a note about this plant" button (`onAddNote`) sets `journalPlantingId = selectedPlop.id` and switches to the Journal tab, while leaving the plop selected. The user can switch back to the inspector and hit "Remove plant", which calls `remove.mutate({ id: plop.id, ... })` + `onClose()` → `selectPlanting(null)`. Neither path clears `journalPlantingId`: `onClose` only resets `selectedPlantingId`, and the store's `remove`/deletion flow does not touch the journal scope. `resetTransient` (which does clear `journalPlantingId`) only fires on garden change/leave, not on plop removal. Result: after the plant is deleted, the journal panel stays scoped to a dead planting — the "Notes about one planting · Show all" banner persists, `useJournal` keeps filtering on a non-existent `plantingId` (empty list → "Nothing written about this planting yet."), and the composer POSTs new notes with a `plantingId` pointing at a row that no longer exists. Suggested fix: clear the scope on remove (e.g. in the remove handler, when the removed id equals the scoped id, call `setJournalPlantingId(null)`), mirroring how `setJournalObjectId` clears `journalPlantingId` for mutual exclusivity. (Confidence: medium — confirmed via reading `PlopInspector.tsx`, `GardenEditorPage.tsx`, and `store.ts`; the only assumption is the backend treats a note keyed to a removed planting as orphaned rather than rejecting it, which doesn't affect the UI-side staleness.)
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
The PR is a straightforward UI-only change that wires up an already-existing backend capability. From a security lens, everything checks out:
- **Write authorization is preserved.** The backend `CreateJournalEntry` requires `roleEditor` and validates the `plantingId` belongs to the garden via `resolveJournalTarget` (verified in `internal/service/journal.go:102-180`). The UI additionally gates the "Add note" button behind `!readOnly` (which is `readOnly={!canEdit}` in `GardenEditorPage.tsx`), so viewers cannot reach the composer even if they bypassed React rendering.
- **Mutual exclusivity of scopes is enforced in depth.** The store setters clear each other (`setJournalObjectId` nullifies `journalPlantingId` and vice versa), the `JournalPanel` filter construction prioritizes `plantingId` over `objectId`, and the Composer receives at most one non-null target. The backend also defensively rejects mismatched `objectId` + `plantingId` pairs.
- **No new injection surfaces.** The `plantingId` value flows from the user's own canvas selection (`selectedPlop.id`) through typed props to the API client; there is no user-supplied free text that could be injected into SQL, commands, or templates.
- **No authz gaps introduced.** Scoping the journal list to a planting only requires garden viewer role, which is the existing permission model. The PR does not change the list endpoint.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I traced the new data flows carefully and found no correctness issues.
- **Mutual exclusivity** is enforced both in the store (`setJournalObjectId` clears `journalPlantingId` and vice‑versa) and defensively in the panel filter (planting scope wins if both were somehow present).
- **Composer payload** correctly sends `objectId: null` and `plantingId: <id>` when scoped to a planting, so the backend receives the intended association.
- **Query invalidation** in `useCreateJournalEntry` uses the prefix `['gardens', gardenId, 'journal']`, which covers the filtered `useJournal` query keys — new planting notes appear in the scoped list as expected.
- **Conditional rendering** (`canEdit`, `readOnly`, `onAddNote`) is wired correctly; the button is only shown to editors and the callback is only invoked when the plop inspector is in writable mode.
- **Empty-state copy** and composer `scopeLabel` branch on `scopePlantingId` consistently.
Nothing in the diff introduces a logic bug or semantically incorrect behavior.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/JournalPanel.tsx:165`** — Triple-nested ternary for the empty-state copy is hard to scan and will get worse if another scope is added. Extract to a helper variable or small function.
```tsx
// Current:
{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.'}
```
Suggested fix: compute `const emptyMessage = ...` above the JSX, or add a `getEmptyMessage(scopePlantingId, scopedObject)` helper.
- **`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.
- **`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.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
* Verified the new `scopePlantingId` prop and `filter` construction in `JournalPanel.tsx:62-70` — the inline object recreation is identical to the pre-existing pattern for `scopeObjectId`, and React Query's key serialization handles it correctly.
* Verified the cache invalidation in `journal.ts:89-92` — `invalidateJournal` broadly invalidates all journal queries for the garden, which was already the behavior before this PR; the comment explicitly notes this is intentional trade-off for a short list.
* Verified the Zustand store setters in `store.ts:125-128` — atomic updates prevent double-renders when scoping switches between bed and plop.
* The `onAddNote` handler in `GardenEditorPage.tsx:506-511` batches two store updates, which React will coalesce into a single render cycle.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
- **Mutual exclusivity**: Verified in `store.ts:126-129` that `setJournalObjectId` and `setJournalPlantingId` correctly clear each other, and `JournalPanel.tsx:62-70` guards the filter so only one scope ever wins. `resetTransient` at `store.ts:160` also clears both.
- **Null/optional safety**: `scopePlantingId` defaults to `null` at `JournalPanel.tsx:40`, and `onScopePlantingChange` is checked before rendering the "Show all" button at line 101. The `Composer` submit safely coerces both IDs with `?? undefined` at `JournalPanel.tsx:229`.
- **Viewer access**: `PlopInspector.tsx:179` correctly gates the new button with `!readOnly && onAddNote`, so viewers never see an "add note" affordance they can't use. The `Composer` itself is already gated by `canEdit` at `JournalPanel.tsx:148`.
- **State transitions**: Changing the bed dropdown while scoped to a planting correctly triggers `setJournalObjectId`, which clears the planting scope via the store. Switching gardens calls `resetTransient`, clearing both scopes.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Verdict: No material issues found
The PR is a pure frontend wiring change; the backend authorization was already in place and is unchanged. I verified the security-relevant paths:
- **`internal/service/journal.go:102-127`** — `CreateJournalEntry` calls `requireGardenRole(..., roleEditor)` then `resolveJournalTarget`, which (lines 141-180) loads the planting via `GetPlanting`, loads its parent object, and rejects with `ErrInvalidInput` if the object's `GardenID` doesn't match the request's `gardenID`. So a forged `plantingId` from another garden is rejected; the new "Add a note about this plant" button can't be used to write into a garden the actor can't edit.
- **`internal/service/journal.go:54-85`** — `ListJournal` requires `roleViewer` on the garden before applying the `PlantingID` filter, so the new `scopePlantingId` filter can't be used to enumerate entries in a garden the viewer can't see.
- **`web/src/lib/journal.ts:79-101`** — `JournalInput.plantingId` is typed `number` and flows through `api.post` to the existing validated endpoint; no string interpolation into a URL or query, so no injection surface introduced.
- The `scopePlantingId`/`journalPlantingId` values originate from `selectedPlop.id` in `GardenEditorPage.tsx` (a value already loaded under the garden's view authorization), not from any URL/param/unsanitized input.
No authn/authz gap, no injection, no secret handling, no untrusted-data sink introduced by this change.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Verified. The bed-scope `<select>` at `JournalPanel.tsx:80-95` keeps `value={scopeObjectId ?? ''}` regardless of planting scope, and the store (`store.ts:126,129`) keeps the two scopes mutually exclusive, so when a planting scope is active `scopeObjectId` is `null` and the dropdown reads "Whole garden". Selecting that option fires `onScopeChange(null)` → `setJournalObjectId(null)`, which clears `journalPlantingId` (line 126), silently dropping the planting scope.
## Verdict: Minor issues
- `web/src/editor/JournalPanel.tsx:80-95` — When a planting scope is active (`scopePlantingId != null`), the bed-scope `<select>` still renders with `value={scopeObjectId ?? ''}`, and `scopeObjectId` is guaranteed `null` at that point (the store's `setJournalPlantingId` clears `journalObjectId` at `store.ts:129`). The dropdown therefore displays **"Whole garden"** as the selected option while the panel is actually filtered to a single planting — the displayed selection misrepresents the real filter state. Worse, because the planting scope is not reflected in the dropdown's value, a user who opens the dropdown and picks the seemingly-already-selected "Whole garden" option triggers `onScopeChange(null)` → `setJournalObjectId(null)`, which (per `store.ts:126`) clears `journalPlantingId` — silently dropping the planting scope they were viewing. Suggested fix: hide or disable the bed-scope `<select>` while `scopePlantingId != null` (the "Notes about one planting · Show all" banner already owns that state), or render a dedicated disabled option so the control reflects reality.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
VERDICT: Minor issues
- **`JournalPanel.tsx:79-95` — the bed-scope `<select>` stays visible while a planting scope is active.** When `scopePlantingId != null`, `setJournalPlantingId` clears `journalObjectId` (`store.ts:129`), so `scopeObjectId` becomes `null` and the dropdown renders `value={scopeObjectId ?? ''}` → `''` = "Whole garden". Meanwhile the list below is filtered to one planting (`JournalPanel.tsx:62-67`, where `scopePlantingId` wins) and a "Notes about one planting" banner sits right under the dropdown (`JournalPanel.tsx:98-111`). Two contradictory scope indicators at once: the dropdown says "whole garden", the banner says "one planting". The filter logic hides the contradiction from the query, but a reader of the UI sees a stale "Whole garden" control beside an active planting filter. Suggested fix: hide or disable the bed `<select>` while `scopePlantingId != null`, mirroring how the banner only renders in that state. Confirmed by reading `JournalPanel.tsx:79-111` and `store.ts:124-129`.
- **`PlopInspector.tsx:179-183` vs `Inspector.tsx:145-153` — affordance parity gap with the bed inspector's note button.** The bed inspector's `onAddNote` button surfaces `noteCount` (`📝 N notes` / `📝 Add note` / `📝 No notes`, `Inspector.tsx:145-153`), so a bed with existing notes is discoverable from the inspector without opening the journal. The plop button is a static `📓 Add a note about this plant` with no count (`PlopInspector.tsx:179-183`), so a plop that already has notes looks identical to one with none — the PR's stated goal is parity with the bed inspector's "add note" affordance, and this is half of that affordance. There's no per-planting counts endpoint (`useJournalCounts` in `journal.ts:69-77` keys by object id only), so the data isn't there today, but the asymmetry is a cleanliness rough edge worth a follow-up. Also the emoji differs (`📓` here vs `📝` in `Inspector.tsx:148-151`) for the same conceptual action — minor, but pick one so the two inspectors read as siblings.
- **`store.ts:62-66` and `JournalPanel.tsx:51-52,60-61` — the "mutually exclusive" invariant is restated three times.** The interface comment (`store.ts:62-64`), the setter comment (`store.ts:125`), and the JournalPanel guard comment (`JournalPanel.tsx:51-52` and `60-61`) all repeat the same mutual-exclusivity explanation. The store is the actual source of truth; the JournalPanel guard (`scopePlantingId` wins over `scopeObjectId`) is defensive duplication that the store already guarantees. Not wrong, but the redundant guard comment ("the two are mutually exclusive in the store, but guard here too") could just reference the store invariant once rather than re-explaining it. Trivial.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
I reviewed the change through the performance lens and verified the relevant code paths.
- `JournalPanel.tsx:62-70` — the `filter` object is rebuilt each render, but `useJournal` (`web/src/lib/journal.ts:42-58`) feeds it into a React Query `queryKey` that is hashed (deep-equal by content), so the new object identity does not cause refetches. Verified the query layer.
- `JournalPanel.tsx:71` — pagination is unchanged (`PAGE_SIZE = 30`, infinite query), no unbounded fetch introduced by the planting scope.
- `store.ts:122-127` — the two setters each perform a single `set` call clearing the sibling field; no extra work, no loop. `reset` correctly includes `journalPlantingId`.
- `GardenEditorPage.tsx:506-512` — `onAddNote` does two store writes (`setJournalPlantingId` + `setRailTab`); each is one Zustand `set`. No re-render storm (selectors are granular and unchanged).
- No new queries, no N+1, no per-render allocations on a hot path, no backend round-trips added.
Nothing in the performance lane is materially wrong with this change.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
Now let me check the service layer for validation around plantingId/objectId mutual exclusivity and planting existence checks.
Now let me check the `resetTransient` and other places that might need to clear `journalPlantingId`, and the bed-inspector flow for parity (does deselecting a bed clear its scope?).
Now let me check the soft-remove scenario for plantings: does `GetPlanting` return removed plantings? Could a journal entry be created against a soft-removed planting? Let me check the `removed_at` handling and whether that's an edge case the diff introduces.
Now I have a thorough picture. Let me focus on the error-handling & edge cases lens and verify the key concerns:
1. The bed inspector's `onAddNote` is gated by `onAddNote` presence AND respects `readOnly` for the button rendering — wait, actually line 145 shows `{onAddNote && (` — it renders even for readOnly viewers (showing "📝 No notes"). The PlopInspector uses `{!readOnly && onAddNote && (` — so it's hidden for viewers. Let me verify the GardenEditorPage passes `onAddNote` to ObjectInspector unconditionally vs conditionally for PlopInspector.
Let me check whether anything clears `journalPlantingId` when the selected plop is removed or when the plantings list no longer contains it.
Now I understand the architecture well. Let me trace the key edge case: the journal scope for a planting survives the plop's removal. Let me verify how `useJournal` handles a `plantingId` filter pointing at a non-existent/removed planting, and whether `useCreateJournalEntry` against a removed planting is rejected.
The key concern: **soft-removed plantings**. `resolveJournalTarget` calls `GetPlanting` which does NOT filter by `removed_at IS NULL` — so a journal entry CAN be created against a soft-removed planting. But that's a pre-existing backend behavior, not introduced by this diff. The diff only wires the UI to existing backend.
Let me focus on what the diff actually introduces and check for genuine edge cases in the new UI code. Let me verify the season view interaction — `selectedPlop` comes from `plantings` which is derived from `full.data?.plantings` which is the season-scoped set when `seasonYear !== null`. So in a past season, `plantings` contains historical plops (including removed ones). The "Add note" button is gated by `canEdit` which is false in past seasons, so it won't show. Good.
But there's a subtle issue: the `onAddNote` for PlopInspector uses `selectedPlop.id` directly. If the plop is soft-removed in the LIVE view... actually in live view, `ListActivePlantingsForGarden` filters `removed_at IS NULL`, so removed plops don't appear in `plantings`, so `selectedPlop` would be null and the inspector wouldn't render. So no issue there.
Let me check one more edge case: the `resetTransient` clears `journalPlantingId` on garden change, good. But does the JournalPanel's "Show all" button and the bed-scope `<select>` properly interact? When `scopePlantingId != null`, the bed `<select>` still shows `value={scopeObjectId ?? ''}`. Since the store keeps them mutually exclusive, `scopeObjectId` is null when `scopePlantingId` is set, so the select shows "Whole garden". That's slightly confusing UX but not an error-handling bug.
Let me check the date filter reset when switching scopes — not really an error handling issue.
Let me look at one potential real issue: the Composer's `plantingId` is passed but when scope is the whole garden (both null), `plantingId` is null and `objectId` is null — fine. When `scopePlantingId` is set, `objectId` becomes null and `plantingId` is set — correct.
Let me verify the `useJournal` query key behavior when switching between scopes rapidly — the filter object changes, creating new query keys, old ones get disposed. No leak concern.
Now let me check whether there's a stale journal scope issue: if you scope to a plop, then that plop gets deleted (hard delete via revision undo), `journalPlantingId` still holds the stale id. The journal query would return empty (no entries match), the composer would try to create an entry against a non-existent planting → backend returns `ErrInvalidInput` → handled by `onError`. So error is surfaced. Not a swallowed error.
Let me double-check there's no issue with the `onScopePlantingChange` being optional and the banner button.
Now let me check whether `selectMode('journal')` (the mobile mode bar journal tap) clears the plop scope. Line 258 only calls `setJournalObjectId(null)` which (per the store) ALSO clears `journalPlantingId`. Good — mutual exclusion holds.
But the desktop "Journal" button at line 585 calls `setJournalObjectId(null)` which clears planting scope too. Good.
Let me verify the test file to see if there are tests covering the new planting scope and edge cases.
No tests cover the new behavior, but that's a test-coverage concern, not strictly error-handling.
Let me verify one final edge case: what happens when `selectedPlop` becomes null mid-flow (e.g., the plop is removed via the "Remove plant" button while journal is scoped to it). The "Remove plant" button calls `onClose()` → `selectPlanting(null)`, which clears `selectedPlantingId` but does NOT clear `journalPlantingId`. So the journal stays scoped to a now-removed planting id. Let me verify.
I have enough context. Now let me consider the error-handling & edge cases lens specifically. The diff is purely UI wiring to an existing backend. Let me assess the genuine edge cases:
1. **Stale `journalPlantingId` after plop removal**: If a user scopes the journal to a plop (via "Add a note about this plant"), then from the journal panel switches back to the inspector and clicks "Remove plant" — `selectPlanting(null)` fires (clearing selection) but `journalPlantingId` retains the now-removed planting's id. The journal stays scoped to a ghost. Entries written would target a soft-removed planting (backend accepts — `GetPlanting` doesn't filter `removed_at`), and the list filter would still match existing entries. This is a mild stale-state edge case but not a crash or swallowed error — the journal simply shows the removed plop's notes. Actually the backend rejects? Let me verify: `resolveJournalTarget` calls `GetPlanting` which returns the row even if `removed_at` is set. So a note CAN be written against a soft-removed planting. The FK is `ON DELETE CASCADE` so a hard-deleted planting would reject (FK violation → 500). This is pre-existing backend behavior though, not introduced by this diff.
This is a minor edge case but within my lens (stale state after removal). It's a real, if minor, issue: the journal scope survives the plop's removal with no cleanup, leaving a confusing "Notes about one planting" banner pinned to a plop the user just removed and can no longer reselect.
Let me confirm there's no cleanup mechanism I'm missing.
Confirmed: `journalPlantingId` is only cleared by: setting a bed scope, setting it to null explicitly (the "Show all" button), or `resetTransient` (garden switch). It is NOT cleared when the plop is removed or deselected. Compare with `journalObjectId`: the bed equivalent has the same property (not cleared on object deletion either), so this is consistent with existing behavior — not a regression introduced by this diff, but a parity-brought-along edge case.
This is genuinely minor and pre-existing in pattern. Let me assess severity. The bed equivalent has the same behavior (deleting a bed doesn't clear `journalObjectId`). So this is consistent parity, not a new defect. I'll note it as a trivial observation rather than a blocking finding, since it matches the established pattern and the PR explicitly aims for parity.
Let me check one more thing — whether the `onAddNote` callback in PlopInspector could fire when `selectedPlop` is stale (race). The callback closes over `selectedPlop.id` at render time; since `selectedPlop` is derived from `selectedPlantingId` and `plantings`, and the callback is recreated each render, it's always current. No stale closure issue.
Let me verify the `Composer` empty-body guard and the `plantingId ?? undefined` handling — already saw it. `if (!text) return` guards empty, and `plantingId ?? undefined` correctly omits when null. No issue.
Let me check the journal query when `plantingId` points to a planting in another garden (the "leak" the service guards against). The list query filters by `garden_id` AND `planting_id`, so cross-garden ids return empty — no leak. The create path is guarded by `resolveJournalTarget`. Good.
I've thoroughly verified. The diff is clean from an error-handling/edge-case perspective. The only observation is the stale-scope-after-removal parity issue, which is consistent with the existing bed-scope behavior and not a regression. Let me write up the final review.
VERDICT: No material issues found
The diff is purely frontend wiring over an already-validated backend. Through the error-handling/edge-case lens I checked the unhappy paths the change touches and found nothing material:
- **Mutual exclusion is enforced in both layers.** `setJournalObjectId` and `setJournalPlantingId` each clear the other (`store.ts:126,129`), and `JournalPanel` re-guards with the `scopePlantingId != null ? … : scopeObjectId != null ? …` ternary (`JournalPanel.tsx:62-67`), so the filter is never double-scoped. `resetTransient` clears both on garden switch (`store.ts:159-160`), and the mobile `selectMode('journal')` + desktop Journal button both route through `setJournalObjectId(null)`, which transitively clears the plop scope.
- **The composer correctly omits the id fields when null.** `objectId: objectId ?? undefined, plantingId: plantingId ?? undefined` (`JournalPanel.tsx:229`) — a null scope produces a garden-level entry, not one with `objectId: 0` or similar. Empty-body is still guarded by `if (!text) return` before the mutate.
- **The create error path is surfaced, not swallowed.** `onError: (err) => setError(errorMessage(err, "Couldn't save that note."))` (`JournalPanel.tsx:235`) renders the failure inline. I verified the backend (`service/journal.go:141-179`) rejects mismatched/cross-garden `plantingId` with `ErrInvalidInput` (→ 400), and a hard-deleted planting would FK-fail; both reach the UI as an error string.
- **Stale-scope-after-removal is consistent with the bed flow, not a regression.** Removing the scoped plop (`PlopInspector` "Remove plant" → `onClose()` → `selectPlanting(null)`) does not clear `journalPlantingId`, so the "Notes about one planting" banner can linger on a plop the user can no longer reselect. I grepped every clearer of `journalPlantingId` (only the bed setter, the explicit "Show all", and `resetTransient`) and confirmed the bed equivalent has the identical property (`journalObjectId` is not cleared on object deletion either). This is parity-brought-along, not introduced here — and it self-heals the moment the user picks "Show all" or scopes to a bed. Calling it a finding would be re-reporting the existing pattern.
- **Past-season / read-only gating holds.** `canEdit` is false when `seasonYear !== null` (`GardenEditorPage.tsx:145-146`), and the button is behind `{!readOnly && onAddNote && …}` (`PlopInspector.tsx:179`), so a viewer or past-season user can't reach the composer. The `Composer` itself is also gated by `{canEdit && …}` in `JournalPanel`.
I verified all of the above by reading `JournalPanel.tsx`, `PlopInspector.tsx`, `store.ts`, `GardenEditorPage.tsx`, `lib/journal.ts`, `lib/objects.ts`, `internal/api/journal.go`, `internal/service/journal.go`, and `internal/store/journal.go`.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
I reviewed the diff through the **Security** lens, tracing both the new frontend path and the existing backend it calls. Here's what I checked:
- **Authorization**: The new `onAddNote` callback on `PlopInspector` is gated by `!readOnly` (`GardenEditorPage.tsx:506`), and `readOnly` is set from `!canEdit`. The backend `CreateJournalEntry` already requires `roleEditor` (`internal/service/journal.go:103`) and validates the planting belongs to the requesting garden via `resolveJournalTarget` (`internal/service/journal.go:141-180`), including a cross-check against the parent bed. No authz bypass introduced.
- **Injection**: No new SQL or command construction. The frontend passes `plantingId` as a JSON field. The backend store layer uses `?` placeholders (`internal/store/journal.go:50-53`) for both read and write paths.
- **Input validation**: The `listJournal` handler already parses `plantingId` through `optionalIDQuery`, which rejects non-positive integers (`internal/api/journal.go:55-58`). The create path's `resolveJournalTarget` validates existence and garden membership before writing.
- **Secret leakage / unsafe deserialization / SSRF**: Not introduced by this change.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found.
The change is clean and follows existing patterns well:
- The `PlopInspector` mirrors the bed `Inspector`'s `onAddNote` callback pattern exactly — same optional signature, same caller-side wiring in `GardenEditorPage`.
- The store's mutual-exclusion design (`journalObjectId` / `journalPlantingId`) is consistent with the existing `selectedId` / `selectedPlantingId` pattern, and the `resetTransient` clearance is complete.
- `JournalPanel`'s filter construction is defensive (plop scope wins, with a local guard even though the store enforces mutual exclusion), and the conditional UI states (banner, composer label, empty-state copy) are all accounted for.
- The `Composer` internal prop extension (`plantingId`) is local and doesn't leak into the public `JournalPanel` interface beyond what's needed.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found.
After tracing every code path touched by this PR through the lens of error handling and edge cases:
- **Mutual exclusivity is enforced at the store level.** `setJournalObjectId` always clears `journalPlantingId`, and vice versa. Call sites in `selectMode`, the desktop Journal button, and the Journal panel dropdown all correctly use these setters, so a double-scope is impossible.
- **The Composer only ever sends one scope.** When `scopePlantingId` is active, `objectId` is explicitly passed as `null` and converted to `undefined` in the mutation payload, so the API receives only `plantingId`.
- **Null/empty collections are handled.** The `scopePlantingId` prop defaults to `null`, the optional `onScopePlantingChange` is present at the sole call site, and the empty-state copy branches correctly.
- **Filter priority is defensive.** The filter construction gives `plantingId` precedence over `objectId` as a guard, matching the store's mutual-exclusion invariant.
- **Reset and lifecycle paths clear both scopes.** `resetTransient` includes `journalPlantingId: null`, and the garden-switch mount effect calls `resetTransient`.
Nothing in this diff introduces an ignored error, missing cleanup, unhandled nil, boundary condition, or stale-scope bug.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- The "add note" affordance no longer hides from viewers (it claimed
parity with the bed inspector but gated on !readOnly). A viewer now sees
"📓 Notes about this plant" and can open the plop's journal to read it;
the composer stays edit-gated, so they can't write. Real parity now.
- onScopePlantingChange is required, matching onScopeChange (its bed twin),
so a caller can't pass a plop scope with no way to clear it. Dropped the
now-dead guard on the "Show all" button.
- Pulled the plop-over-bed scope-label priority into one `scopeLabel`,
shared by the filter's sibling logic and the composer, so they can't
drift; trimmed the invariant comment that was restated a third time.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit cac26286b1 into main2026-07-23 01:42:02 +00:00
steve
deleted branch feat/plop-journal-notes2026-07-23 01:42:03 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #85 item 5.
journal_entries.planting_idwas modelled, accepted by the API, and theJournalPanelalready rendered a "planting" badge for such entries — but nothing in the UI ever created one. A rendered badge for a state the UI couldn't produce.Give a plop the same "add note" affordance the bed inspector already has:
PlopInspectorgains anonAddNotebutton — "📓 Add a note about this plant" — shown only to an editor (a viewer can't write notes anyway).journalPlantingIdscope besidejournalObjectId. The two are mutually exclusive — each setter clears the other — so the journal filter is never double-scoped.JournalPanelfilters byplantingIdwhen that scope is set, shows a "Notes about one planting · Show all" banner, and its composer attaches new notes to the plop. Empty-state copy updated to match.Two taps from a selected plant to typing, mirroring the bed flow (the plop stays selected, so switching back to its inspector still works). No backend change — the API already accepted
plantingId.tsc,vitest(108), andvite buildgreen.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
journal_entries.planting_id was modelled, accepted by the API, and the JournalPanel already rendered a "planting" badge for such entries — but nothing in the UI ever created one. A badge for a state the UI couldn't produce. Give the plop the same "add note" affordance the bed inspector has: - PlopInspector gains an onAddNote button ("📓 Add a note about this plant"), shown only to an editor (a viewer can't write notes). - The editor store gains a journalPlantingId scope beside journalObjectId. The two are mutually exclusive — each setter clears the other — so the journal filter is never double-scoped. - JournalPanel filters by plantingId when that scope is set, shows a "Notes about one planting · Show all" banner, and its composer attaches new notes to the plop. Empty-state copy updated to match. Two taps from a selected plant to typing, mirroring the bed flow. No backend change — the API already accepted plantingId. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-23 01:33:54Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -49,0 +51,4 @@/** Which single plop the panel is filtered to, if any (#85). Mutually* exclusive with scopeObjectId — the editor store enforces that. */scopePlantingId?: number | nullonScopePlantingChange?: (id: number | null) => void🟡 onScopePlantingChange is optional while onScopeChange is required — inconsistent API
maintainability · flagged by 1 model
web/src/editor/JournalPanel.tsx:54—onScopePlantingChangeis optional (?:) whileonScopeChangeis required, yet the component treatsscopePlantingIdas a first-class scope. The asymmetry means a caller can passscopePlantingIdwithout a way to clear it (the "Show all" button is gated ononScopePlantingChange). MakeonScopePlantingChangerequired to matchonScopeChange, or document why it must be optional.🪰 Gadfly · advisory
@@ -126,0 +151,4 @@objectId={scopePlantingId != null ? null : scopeObjectId}plantingId={scopePlantingId}scopeLabel={scopePlantingId != null ? 'this planting' : scopedObject ? objectDisplayName(scopedObject) : null🟡 Nested ternary for Composer scopeLabel duplicates priority logic from filter
maintainability · flagged by 1 model
web/src/editor/JournalPanel.tsx:154— Nested ternary forscopeLabelin theComposercall repeats the same scoping-priority logic already expressed infilter. A smallgetScopeLabel(scopePlantingId, scopedObject)helper would keep the logic in one place and avoid drift.🪰 Gadfly · advisory
@@ -172,6 +176,12 @@ export function PlopInspector({/></fieldset>{!readOnly && onAddNote && (🟡 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 bothPlopInspector.tsx:37-38and theGardenEditorPage.tsx:506-512call site explicitly claiming "parity with the bed inspector," the two diverge:Inspector.tsx:145renders its note button unconditionally ({onAddNote && (...)}), giving read-only viewers a view-only affordance (a note count, or "📝 No notes"), whilePlopInspector.tsx:179gates the equivalent button on!readOnly, hiding it entirely for viewers. Both call site…🪰 Gadfly · advisory
@@ -59,6 +59,12 @@ interface EditorState {journalObjectId: number | nullsetJournalObjectId: (id: number | null) => void// Which single plop the journal is filtered to, if any — the parallel of⚪ Mutual-exclusivity invariant is restated in three places (interface, setter, JournalPanel guard)
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 8 findings (2 with multi-model agreement)
web/src/editor/PlopInspector.tsx:179web/src/editor/PlopInspector.tsx:1906 single-model findings (lower confidence)
web/src/editor/JournalPanel.tsx:79web/src/editor/JournalPanel.tsx:54web/src/editor/JournalPanel.tsx:154web/src/editor/JournalPanel.tsx:165web/src/editor/JournalPanel.tsx:62web/src/editor/store.ts:62Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
This confirms
canEditis derived from server-provided garden role data (consistent with the rest of the file's usage), and the backend independently enforcesroleEditoronCreateJournalEntryplus validates thatplantingIdactually belongs to the target garden (resolveJournalTargetininternal/service/journal.go:141-179) and the list query is always scoped bygarden_id(internal/store/journal.go:82). So client-side gating of the "Add a note" button is convenience UI only, not a security boundary, and the boundary itself is sound and untouched by this diff. No backend code changed here, and the frontend changes are just state wiring (mutually-exclusive scope IDs, a filter param, static label text) — nothing that touches authn/authz, injection, deserialization, or secrets.VERDICT: No material issues found
No security-relevant defects in this diff. The newly-exposed
plantingIdjournal scope was already properly authorized server-side before this PR (create validates the planting belongs to the garden; list is always scoped bygarden_id), so wiring it up in the UI doesn't introduce an IDOR or authz gap. All new code is client-side state/UI plumbing with no injection sinks, no secret handling, and no new trust boundary.🎯 Correctness — No material issues found
That's fine. I've thoroughly traced the data flow front-to-back (store → JournalPanel → Composer → useCreateJournalEntry → API → service.resolveJournalTarget) and the mutual-exclusion/selection-effect interactions in GardenEditorPage. Everything checks out.
VERDICT: No material issues found
Traced the full path for correctness:
store.ts:126,129):setJournalObjectId/setJournalPlantingIdeach clear the other, andresetTransientclears both. Verified the one pre-existing call site that resets scope (selectMode('journal')atGardenEditorPage.tsx:258, callingsetJournalObjectId(null)) transitively clearsjournalPlantingIdtoo via the updated setter — no stale scope leaks through that path.GardenEditorPage.tsx:224-234that force-switchesrailTabto'inspector'is keyed only offselectedId/selectedPlantingId, notrailTab. SinceonAddNotedoesn't change the plop selection, callingsetRailTab('journal')from it isn't immediately clobbered back to'inspector'— the PR's claim about this holds.JournalFilter/JournalInputinlib/journal.tsalready declareplantingId?: number, matching whatJournalPanel/Composernow send.objectId ?? undefined/plantingId ?? undefinedcorrectly use nullish coalescing (no falsy-zero bug, though IDs won't be 0 anyway).internal/service/journal.go'sresolveJournalTargetfills in the parent object from the planting and validates it belongs to the garden;internal/api/journal.goalready decodesplantingIdon both read and write paths, confirmed by existing tests injournal_test.go. No backend changes were needed, matching the PR description.JournalPanel.tsx:62-70correctly prioritizesplantingIdoverobjectIdwith the same guard the store already enforces — belt-and-braces, not conflicting logic.No logic bugs, no incorrect constants/thresholds in this diff (it introduces no numeric constants), no broken cross-file wiring.
🧹 Code cleanliness & maintainability — Minor issues
Both call sites pass
onAddNoteunconditionally (regardless ofcanEdit/readOnly). Confirmed:Inspector(bed) shows the button always whenonAddNoteis passed — including for readOnly viewers, rendering "📝 No notes" or a count as a view-only affordance (Inspector.tsx:145-153).PlopInspectorgates the identical button on!readOnly(PlopInspector.tsx:179), hiding it entirely from viewers, despite the code/comments explicitly claiming parity with the bed inspector. The draft finding is accurate.VERDICT: Minor issues
web/src/editor/PlopInspector.tsx:179— Despite comments in bothPlopInspector.tsx:37-38and theGardenEditorPage.tsx:506-512call site explicitly claiming "parity with the bed inspector," the two diverge:Inspector.tsx:145renders its note button unconditionally ({onAddNote && (...)}), giving read-only viewers a view-only affordance (a note count, or "📝 No notes"), whilePlopInspector.tsx:179gates the equivalent button on!readOnly, hiding it entirely for viewers. Both call sites (GardenEditorPage.tsx:489-495and:506-512) passonAddNoteunconditionally regardless ofcanEdit, so the difference is solely in each inspector's own render condition — not a missing prop. A viewer can browse a bed's notes count/empty-state but has no equivalent way to discover a plop has notes.⚡ Performance — No material issues found
This is a small, well-contained UI addition (a button, a store scope, a filter branch). Nothing in the diff touches loops over data, query pagination, or introduces new fetches beyond the existing
useJournal/useJournalCountspattern, which is unchanged in shape (just one more optional filter key, consistent with howobjectId/from/toalready work). Thefilterobject recreated per render was already the pattern before this PR (react-query structurally hashes query keys, so this isn't a regression). No new loops, no N+1 queries, no unbounded growth.VERDICT: No material issues found
No performance-relevant regressions in this diff — it's a straightforward plumbing addition (new store field, one button, one filter branch) that reuses the existing journal query/pagination machinery unchanged.
🧯 Error handling & edge cases — Minor issues
All draft claims check out against the actual source. The single finding is confirmed: removing a plop closes the inspector via
selectPlanting(null)but never clearsjournalPlantingId, leaving the journal composer scoped to (and able to attach new notes to) a soft-removed planting.VERDICT: Minor issues
web/src/editor/PlopInspector.tsx:190-193— "Remove plant" callsonClose()(→selectPlanting(null), wired atGardenEditorPage.tsx:505) then soft-removes the plop viaremove.mutate, but nothing clearsjournalPlantingId(store.ts:128-129). If a note was scoped to that plop via the new "📓 Add a note about this plant" button (PlopInspector.tsx:179-183, wired atGardenEditorPage.tsx:506-512), the journal stays filtered to the now-removed planting's id: the "Notes about one planting" banner and composer (JournalPanel.tsx:98-111,148-157) keep pointing at a plant that no longer exists in the plop list, and the composer will attach new notes to the removedplantingIdwith no warning.selectPlanting(store.ts:113) only setsselectedPlantingId/selectedId, never touchingjournalPlantingId;setJournalPlantingIdis invoked only from the "Add a note" handler (GardenEditorPage.tsx:510) and the panel's own "Show all" button, so nothing resets it on removal. This mirrors a pre-existing identical gap on the bed side (Inspector.tsx:305-307deletes an object viaselect(null)+del.mutatewithout touchingjournalObjectIdeither, andselect()atstore.ts:110doesn't touch it), so it's a known class of gap rather than a regression unique to this PR — worth fixing but not blocking.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
PlopInspector.onAddNoteonly renders when!readOnly(i.e., editor only — viewers can't reach it,GardenEditorPage.tsx:503), the composer only renders whencanEdit(JournalPanel{canEdit && (...)}), and the mutationuseCreateJournalEntryposts the typedplantingIdstraight through to/gardens/{gid}/journalwith no client-side authorization over the planting id beyondcanEdit. That matches the existing bed (objectId) flow, which the PR description notes relies on the backend. No client-side IDOR mitigation was added forplantingId, but none exists forobjectIdeither — this is parity, not a regression, and backend authz is out of scope for a UI-only change. No new injection/SSRF/deserialization surface (theplantingIdis anumberfrom the store, typed and passed as a JSON body field, never interpolated into a path or query). The store's mutual-exclusion guards (store.ts:125-128) correctly prevent a double-scoped filter being sent to the API.🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change end-to-end through the correctness lens:
store.ts:125-129):setJournalObjectIdclearsjournalPlantingIdand vice versa.JournalPanel.tsx:62-67additionally guards soplantingIdwins in the filter. No double-scoped filter is reachable.JournalPanel.tsx:62-70,journal.ts:33-38):JournalFilterdeclaresplantingId?: number, anduseJournalspreads the filter into the API params (journal.ts:54). The planting-scoped query is well-formed.JournalPanel.tsx:150-156, 229): whenscopePlantingIdis set,objectIdis forced tonullandplantingIdis passed; the mutation sendsplantingId: plantingId ?? undefinedagainstJournalInput(journal.ts:79-84), which accepts it. A bed-scoped note still attachesobjectId. Correct.JournalPanel.tsx:104→setJournalPlantingId(null)): clears both scopes (store setter nullsjournalObjectIdtoo), so the panel returns to whole-garden. Correct.<select>interaction (JournalPanel.tsx:80-94): itsonChangecallssetJournalObjectId, which clears any planting scope — so picking a bed from the dropdown while a planting banner is shown correctly drops the planting scope. No stale state.GardenEditorPage.tsx:506-512):selectedPlop.idis theEditorPlantingid, matchingjournal_entries.planting_id. The plop stays selected (selectPlantinguntouched), so the inspector remains reachable after switching to the journal tab. Parity with the bed flow (Inspector.tsx:145,GardenEditorPage.tsx:489-494) holds.store.ts:160):journalPlantingIdis cleared on reset alongsidejournalObjectId. No dangling scope.No logic bugs, incorrect constants, or semantic mismatches found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/editor/PlopInspector.tsx:180— the plop inspector's "Add note" button diverges from the bed inspector's affordance, dropping the note-count affordance the established pattern has.Inspector.tsx:145-153(the bed inspector) renders a button that shows either📝 {n} notes/📝 No notes/📝 Add notedepending onnoteCountandreadOnly, styledw-full text-sm. The new plop button (PlopInspector.tsx:179-183) instead always renders the fixed label📓 Add a note about this plantand uses a different emoji (📓 vs 📝) and different button styling (justify-start px-2 py-1vsw-full).PlopInspector.tsxhas nonoteCountprop at all (grep confirms zero matches), so a plop with existing notes shows no count indicator on its inspector, and the icon/label/styling differ from the bed inspector for no functional reason. The PR description frames this as "parity with the bed flow," but the parity is only partial. Suggested fix: mirror the bed inspector — pass anoteCount, or at minimum reuse the📝glyph and the📝 {n} notes/📝 No notes/📝 Add notelabel logic and thew-full text-smstyling so the two inspectors read as one family.web/src/editor/JournalPanel.tsx:62-70— thescopePlantingId != null ? { plantingId } : scopeObjectId != null ? { objectId } : {}chain has no shared helper for "pick the active scope." This is a minor readability nit, not a bug: the samescopePlantingId != null/scopeObjectId != nullprecedence is re-derived inline in three places in this file — thefilterat lines 62-67, theComposerobjectId/scopeLabelat lines 151-154, and the empty-state copy at lines 166-170. Each is a small nested ternary, and together they re-encode the "plop wins, else bed, else none" rule independently. A singleactiveScopehelper (orconst plantingScoped = scopePlantingId != null) would let each call site read the rule once instead of re-deriving it. Low churn, optional.No dead/duplicated code beyond the above, no naming or structural problems in
store.ts(the mutually-exclusive setters and theresetupdate are clean and follow the existing pattern), and theGardenEditorPagewiring matches the bed-inspector wiring.⚡ Performance — No material issues found
No material issues found
web/src/lib/journal.ts:46-59):useJournalis an infinite query keyed by the fullfilterobject (now includingplantingId), paginated atPAGE_SIZE = 30. Scoping to a plop produces a distinct, separately cached query — no full re-fetch of unbounded lists, no N+1. The mutation invalidates the garden's journal keys broadly (invalidateJournal), which is the same pre-existing strategy; addingplantingIddoesn't widen that blast radius meaningfully.filterobject is reconstructed every render (a new reference), but react-query hashes query keys structurally (journalKeyreturns a tuple containing the filter), so this does not cause refetch storms. This pattern pre-existed forscopeObjectId/from/to; the PR only adds one more conditional field.objects.find((o) => o.id === scopeObjectId)runs each render but operates on the beds list (small, not the plantings list) and is unchanged by this PR.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
journalPlantingIdafter the plop is removed —web/src/editor/PlopInspector.tsx:190-193/GardenEditorPage.tsx:506-512. The "Add a note about this plant" button (onAddNote) setsjournalPlantingId = selectedPlop.idand switches to the Journal tab, while leaving the plop selected. The user can switch back to the inspector and hit "Remove plant", which callsremove.mutate({ id: plop.id, ... })+onClose()→selectPlanting(null). Neither path clearsjournalPlantingId:onCloseonly resetsselectedPlantingId, and the store'sremove/deletion flow does not touch the journal scope.resetTransient(which does clearjournalPlantingId) only fires on garden change/leave, not on plop removal. Result: after the plant is deleted, the journal panel stays scoped to a dead planting — the "Notes about one planting · Show all" banner persists,useJournalkeeps filtering on a non-existentplantingId(empty list → "Nothing written about this planting yet."), and the composer POSTs new notes with aplantingIdpointing at a row that no longer exists. Suggested fix: clear the scope on remove (e.g. in the remove handler, when the removed id equals the scoped id, callsetJournalPlantingId(null)), mirroring howsetJournalObjectIdclearsjournalPlantingIdfor mutual exclusivity. (Confidence: medium — confirmed via readingPlopInspector.tsx,GardenEditorPage.tsx, andstore.ts; the only assumption is the backend treats a note keyed to a removed planting as orphaned rather than rejecting it, which doesn't affect the UI-side staleness.)kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The PR is a straightforward UI-only change that wires up an already-existing backend capability. From a security lens, everything checks out:
CreateJournalEntryrequiresroleEditorand validates theplantingIdbelongs to the garden viaresolveJournalTarget(verified ininternal/service/journal.go:102-180). The UI additionally gates the "Add note" button behind!readOnly(which isreadOnly={!canEdit}inGardenEditorPage.tsx), so viewers cannot reach the composer even if they bypassed React rendering.setJournalObjectIdnullifiesjournalPlantingIdand vice versa), theJournalPanelfilter construction prioritizesplantingIdoverobjectId, and the Composer receives at most one non-null target. The backend also defensively rejects mismatchedobjectId+plantingIdpairs.plantingIdvalue flows from the user's own canvas selection (selectedPlop.id) through typed props to the API client; there is no user-supplied free text that could be injected into SQL, commands, or templates.🎯 Correctness — No material issues found
No material issues found
I traced the new data flows carefully and found no correctness issues.
setJournalObjectIdclearsjournalPlantingIdand vice‑versa) and defensively in the panel filter (planting scope wins if both were somehow present).objectId: nullandplantingId: <id>when scoped to a planting, so the backend receives the intended association.useCreateJournalEntryuses the prefix['gardens', gardenId, 'journal'], which covers the filtereduseJournalquery keys — new planting notes appear in the scoped list as expected.canEdit,readOnly,onAddNote) is wired correctly; the button is only shown to editors and the callback is only invoked when the plop inspector is in writable mode.scopeLabelbranch onscopePlantingIdconsistently.Nothing in the diff introduces a logic bug or semantically incorrect behavior.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/JournalPanel.tsx:165— Triple-nested ternary for the empty-state copy is hard to scan and will get worse if another scope is added. Extract to a helper variable or small function.Suggested fix: compute
const emptyMessage = ...above the JSX, or add agetEmptyMessage(scopePlantingId, scopedObject)helper.web/src/editor/JournalPanel.tsx:54—onScopePlantingChangeis optional (?:) whileonScopeChangeis required, yet the component treatsscopePlantingIdas a first-class scope. The asymmetry means a caller can passscopePlantingIdwithout a way to clear it (the "Show all" button is gated ononScopePlantingChange). MakeonScopePlantingChangerequired to matchonScopeChange, or document why it must be optional.web/src/editor/JournalPanel.tsx:154— Nested ternary forscopeLabelin theComposercall repeats the same scoping-priority logic already expressed infilter. A smallgetScopeLabel(scopePlantingId, scopedObject)helper would keep the logic in one place and avoid drift.⚡ Performance — No material issues found
No material issues found
scopePlantingIdprop andfilterconstruction inJournalPanel.tsx:62-70— the inline object recreation is identical to the pre-existing pattern forscopeObjectId, and React Query's key serialization handles it correctly.journal.ts:89-92—invalidateJournalbroadly invalidates all journal queries for the garden, which was already the behavior before this PR; the comment explicitly notes this is intentional trade-off for a short list.store.ts:125-128— atomic updates prevent double-renders when scoping switches between bed and plop.onAddNotehandler inGardenEditorPage.tsx:506-511batches two store updates, which React will coalesce into a single render cycle.🧯 Error handling & edge cases — No material issues found
No material issues found
store.ts:126-129thatsetJournalObjectIdandsetJournalPlantingIdcorrectly clear each other, andJournalPanel.tsx:62-70guards the filter so only one scope ever wins.resetTransientatstore.ts:160also clears both.scopePlantingIddefaults tonullatJournalPanel.tsx:40, andonScopePlantingChangeis checked before rendering the "Show all" button at line 101. TheComposersubmit safely coerces both IDs with?? undefinedatJournalPanel.tsx:229.PlopInspector.tsx:179correctly gates the new button with!readOnly && onAddNote, so viewers never see an "add note" affordance they can't use. TheComposeritself is already gated bycanEditatJournalPanel.tsx:148.setJournalObjectId, which clears the planting scope via the store. Switching gardens callsresetTransient, clearing both scopes.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
The PR is a pure frontend wiring change; the backend authorization was already in place and is unchanged. I verified the security-relevant paths:
internal/service/journal.go:102-127—CreateJournalEntrycallsrequireGardenRole(..., roleEditor)thenresolveJournalTarget, which (lines 141-180) loads the planting viaGetPlanting, loads its parent object, and rejects withErrInvalidInputif the object'sGardenIDdoesn't match the request'sgardenID. So a forgedplantingIdfrom another garden is rejected; the new "Add a note about this plant" button can't be used to write into a garden the actor can't edit.internal/service/journal.go:54-85—ListJournalrequiresroleVieweron the garden before applying thePlantingIDfilter, so the newscopePlantingIdfilter can't be used to enumerate entries in a garden the viewer can't see.web/src/lib/journal.ts:79-101—JournalInput.plantingIdis typednumberand flows throughapi.postto the existing validated endpoint; no string interpolation into a URL or query, so no injection surface introduced.scopePlantingId/journalPlantingIdvalues originate fromselectedPlop.idinGardenEditorPage.tsx(a value already loaded under the garden's view authorization), not from any URL/param/unsanitized input.No authn/authz gap, no injection, no secret handling, no untrusted-data sink introduced by this change.
🎯 Correctness — Minor issues
Verified. The bed-scope
<select>atJournalPanel.tsx:80-95keepsvalue={scopeObjectId ?? ''}regardless of planting scope, and the store (store.ts:126,129) keeps the two scopes mutually exclusive, so when a planting scope is activescopeObjectIdisnulland the dropdown reads "Whole garden". Selecting that option firesonScopeChange(null)→setJournalObjectId(null), which clearsjournalPlantingId(line 126), silently dropping the planting scope.Verdict: Minor issues
web/src/editor/JournalPanel.tsx:80-95— When a planting scope is active (scopePlantingId != null), the bed-scope<select>still renders withvalue={scopeObjectId ?? ''}, andscopeObjectIdis guaranteednullat that point (the store'ssetJournalPlantingIdclearsjournalObjectIdatstore.ts:129). The dropdown therefore displays "Whole garden" as the selected option while the panel is actually filtered to a single planting — the displayed selection misrepresents the real filter state. Worse, because the planting scope is not reflected in the dropdown's value, a user who opens the dropdown and picks the seemingly-already-selected "Whole garden" option triggersonScopeChange(null)→setJournalObjectId(null), which (perstore.ts:126) clearsjournalPlantingId— silently dropping the planting scope they were viewing. Suggested fix: hide or disable the bed-scope<select>whilescopePlantingId != null(the "Notes about one planting · Show all" banner already owns that state), or render a dedicated disabled option so the control reflects reality.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
JournalPanel.tsx:79-95— the bed-scope<select>stays visible while a planting scope is active. WhenscopePlantingId != null,setJournalPlantingIdclearsjournalObjectId(store.ts:129), soscopeObjectIdbecomesnulland the dropdown rendersvalue={scopeObjectId ?? ''}→''= "Whole garden". Meanwhile the list below is filtered to one planting (JournalPanel.tsx:62-67, wherescopePlantingIdwins) and a "Notes about one planting" banner sits right under the dropdown (JournalPanel.tsx:98-111). Two contradictory scope indicators at once: the dropdown says "whole garden", the banner says "one planting". The filter logic hides the contradiction from the query, but a reader of the UI sees a stale "Whole garden" control beside an active planting filter. Suggested fix: hide or disable the bed<select>whilescopePlantingId != null, mirroring how the banner only renders in that state. Confirmed by readingJournalPanel.tsx:79-111andstore.ts:124-129.PlopInspector.tsx:179-183vsInspector.tsx:145-153— affordance parity gap with the bed inspector's note button. The bed inspector'sonAddNotebutton surfacesnoteCount(📝 N notes/📝 Add note/📝 No notes,Inspector.tsx:145-153), so a bed with existing notes is discoverable from the inspector without opening the journal. The plop button is a static📓 Add a note about this plantwith no count (PlopInspector.tsx:179-183), so a plop that already has notes looks identical to one with none — the PR's stated goal is parity with the bed inspector's "add note" affordance, and this is half of that affordance. There's no per-planting counts endpoint (useJournalCountsinjournal.ts:69-77keys by object id only), so the data isn't there today, but the asymmetry is a cleanliness rough edge worth a follow-up. Also the emoji differs (📓here vs📝inInspector.tsx:148-151) for the same conceptual action — minor, but pick one so the two inspectors read as siblings.store.ts:62-66andJournalPanel.tsx:51-52,60-61— the "mutually exclusive" invariant is restated three times. The interface comment (store.ts:62-64), the setter comment (store.ts:125), and the JournalPanel guard comment (JournalPanel.tsx:51-52and60-61) all repeat the same mutual-exclusivity explanation. The store is the actual source of truth; the JournalPanel guard (scopePlantingIdwins overscopeObjectId) is defensive duplication that the store already guarantees. Not wrong, but the redundant guard comment ("the two are mutually exclusive in the store, but guard here too") could just reference the store invariant once rather than re-explaining it. Trivial.⚡ Performance — No material issues found
VERDICT: No material issues found
I reviewed the change through the performance lens and verified the relevant code paths.
JournalPanel.tsx:62-70— thefilterobject is rebuilt each render, butuseJournal(web/src/lib/journal.ts:42-58) feeds it into a React QueryqueryKeythat is hashed (deep-equal by content), so the new object identity does not cause refetches. Verified the query layer.JournalPanel.tsx:71— pagination is unchanged (PAGE_SIZE = 30, infinite query), no unbounded fetch introduced by the planting scope.store.ts:122-127— the two setters each perform a singlesetcall clearing the sibling field; no extra work, no loop.resetcorrectly includesjournalPlantingId.GardenEditorPage.tsx:506-512—onAddNotedoes two store writes (setJournalPlantingId+setRailTab); each is one Zustandset. No re-render storm (selectors are granular and unchanged).Nothing in the performance lane is materially wrong with this change.
🧯 Error handling & edge cases — No material issues found
Now let me check the service layer for validation around plantingId/objectId mutual exclusivity and planting existence checks.
Now let me check the
resetTransientand other places that might need to clearjournalPlantingId, and the bed-inspector flow for parity (does deselecting a bed clear its scope?).Now let me check the soft-remove scenario for plantings: does
GetPlantingreturn removed plantings? Could a journal entry be created against a soft-removed planting? Let me check theremoved_athandling and whether that's an edge case the diff introduces.Now I have a thorough picture. Let me focus on the error-handling & edge cases lens and verify the key concerns:
onAddNoteis gated byonAddNotepresence AND respectsreadOnlyfor the button rendering — wait, actually line 145 shows{onAddNote && (— it renders even for readOnly viewers (showing "📝 No notes"). The PlopInspector uses{!readOnly && onAddNote && (— so it's hidden for viewers. Let me verify the GardenEditorPage passesonAddNoteto ObjectInspector unconditionally vs conditionally for PlopInspector.Let me check whether anything clears
journalPlantingIdwhen the selected plop is removed or when the plantings list no longer contains it.Now I understand the architecture well. Let me trace the key edge case: the journal scope for a planting survives the plop's removal. Let me verify how
useJournalhandles aplantingIdfilter pointing at a non-existent/removed planting, and whetheruseCreateJournalEntryagainst a removed planting is rejected.The key concern: soft-removed plantings.
resolveJournalTargetcallsGetPlantingwhich does NOT filter byremoved_at IS NULL— so a journal entry CAN be created against a soft-removed planting. But that's a pre-existing backend behavior, not introduced by this diff. The diff only wires the UI to existing backend.Let me focus on what the diff actually introduces and check for genuine edge cases in the new UI code. Let me verify the season view interaction —
selectedPlopcomes fromplantingswhich is derived fromfull.data?.plantingswhich is the season-scoped set whenseasonYear !== null. So in a past season,plantingscontains historical plops (including removed ones). The "Add note" button is gated bycanEditwhich is false in past seasons, so it won't show. Good.But there's a subtle issue: the
onAddNotefor PlopInspector usesselectedPlop.iddirectly. If the plop is soft-removed in the LIVE view... actually in live view,ListActivePlantingsForGardenfiltersremoved_at IS NULL, so removed plops don't appear inplantings, soselectedPlopwould be null and the inspector wouldn't render. So no issue there.Let me check one more edge case: the
resetTransientclearsjournalPlantingIdon garden change, good. But does the JournalPanel's "Show all" button and the bed-scope<select>properly interact? WhenscopePlantingId != null, the bed<select>still showsvalue={scopeObjectId ?? ''}. Since the store keeps them mutually exclusive,scopeObjectIdis null whenscopePlantingIdis set, so the select shows "Whole garden". That's slightly confusing UX but not an error-handling bug.Let me check the date filter reset when switching scopes — not really an error handling issue.
Let me look at one potential real issue: the Composer's
plantingIdis passed but when scope is the whole garden (both null),plantingIdis null andobjectIdis null — fine. WhenscopePlantingIdis set,objectIdbecomes null andplantingIdis set — correct.Let me verify the
useJournalquery key behavior when switching between scopes rapidly — the filter object changes, creating new query keys, old ones get disposed. No leak concern.Now let me check whether there's a stale journal scope issue: if you scope to a plop, then that plop gets deleted (hard delete via revision undo),
journalPlantingIdstill holds the stale id. The journal query would return empty (no entries match), the composer would try to create an entry against a non-existent planting → backend returnsErrInvalidInput→ handled byonError. So error is surfaced. Not a swallowed error.Let me double-check there's no issue with the
onScopePlantingChangebeing optional and the banner button.Now let me check whether
selectMode('journal')(the mobile mode bar journal tap) clears the plop scope. Line 258 only callssetJournalObjectId(null)which (per the store) ALSO clearsjournalPlantingId. Good — mutual exclusion holds.But the desktop "Journal" button at line 585 calls
setJournalObjectId(null)which clears planting scope too. Good.Let me verify the test file to see if there are tests covering the new planting scope and edge cases.
No tests cover the new behavior, but that's a test-coverage concern, not strictly error-handling.
Let me verify one final edge case: what happens when
selectedPlopbecomes null mid-flow (e.g., the plop is removed via the "Remove plant" button while journal is scoped to it). The "Remove plant" button callsonClose()→selectPlanting(null), which clearsselectedPlantingIdbut does NOT clearjournalPlantingId. So the journal stays scoped to a now-removed planting id. Let me verify.I have enough context. Now let me consider the error-handling & edge cases lens specifically. The diff is purely UI wiring to an existing backend. Let me assess the genuine edge cases:
journalPlantingIdafter plop removal: If a user scopes the journal to a plop (via "Add a note about this plant"), then from the journal panel switches back to the inspector and clicks "Remove plant" —selectPlanting(null)fires (clearing selection) butjournalPlantingIdretains the now-removed planting's id. The journal stays scoped to a ghost. Entries written would target a soft-removed planting (backend accepts —GetPlantingdoesn't filterremoved_at), and the list filter would still match existing entries. This is a mild stale-state edge case but not a crash or swallowed error — the journal simply shows the removed plop's notes. Actually the backend rejects? Let me verify:resolveJournalTargetcallsGetPlantingwhich returns the row even ifremoved_atis set. So a note CAN be written against a soft-removed planting. The FK isON DELETE CASCADEso a hard-deleted planting would reject (FK violation → 500). This is pre-existing backend behavior though, not introduced by this diff.This is a minor edge case but within my lens (stale state after removal). It's a real, if minor, issue: the journal scope survives the plop's removal with no cleanup, leaving a confusing "Notes about one planting" banner pinned to a plop the user just removed and can no longer reselect.
Let me confirm there's no cleanup mechanism I'm missing.
Confirmed:
journalPlantingIdis only cleared by: setting a bed scope, setting it to null explicitly (the "Show all" button), orresetTransient(garden switch). It is NOT cleared when the plop is removed or deselected. Compare withjournalObjectId: the bed equivalent has the same property (not cleared on object deletion either), so this is consistent with existing behavior — not a regression introduced by this diff, but a parity-brought-along edge case.This is genuinely minor and pre-existing in pattern. Let me assess severity. The bed equivalent has the same behavior (deleting a bed doesn't clear
journalObjectId). So this is consistent parity, not a new defect. I'll note it as a trivial observation rather than a blocking finding, since it matches the established pattern and the PR explicitly aims for parity.Let me check one more thing — whether the
onAddNotecallback in PlopInspector could fire whenselectedPlopis stale (race). The callback closes overselectedPlop.idat render time; sinceselectedPlopis derived fromselectedPlantingIdandplantings, and the callback is recreated each render, it's always current. No stale closure issue.Let me verify the
Composerempty-body guard and theplantingId ?? undefinedhandling — already saw it.if (!text) returnguards empty, andplantingId ?? undefinedcorrectly omits when null. No issue.Let me check the journal query when
plantingIdpoints to a planting in another garden (the "leak" the service guards against). The list query filters bygarden_idANDplanting_id, so cross-garden ids return empty — no leak. The create path is guarded byresolveJournalTarget. Good.I've thoroughly verified. The diff is clean from an error-handling/edge-case perspective. The only observation is the stale-scope-after-removal parity issue, which is consistent with the existing bed-scope behavior and not a regression. Let me write up the final review.
VERDICT: No material issues found
The diff is purely frontend wiring over an already-validated backend. Through the error-handling/edge-case lens I checked the unhappy paths the change touches and found nothing material:
setJournalObjectIdandsetJournalPlantingIdeach clear the other (store.ts:126,129), andJournalPanelre-guards with thescopePlantingId != null ? … : scopeObjectId != null ? …ternary (JournalPanel.tsx:62-67), so the filter is never double-scoped.resetTransientclears both on garden switch (store.ts:159-160), and the mobileselectMode('journal')+ desktop Journal button both route throughsetJournalObjectId(null), which transitively clears the plop scope.objectId: objectId ?? undefined, plantingId: plantingId ?? undefined(JournalPanel.tsx:229) — a null scope produces a garden-level entry, not one withobjectId: 0or similar. Empty-body is still guarded byif (!text) returnbefore the mutate.onError: (err) => setError(errorMessage(err, "Couldn't save that note."))(JournalPanel.tsx:235) renders the failure inline. I verified the backend (service/journal.go:141-179) rejects mismatched/cross-gardenplantingIdwithErrInvalidInput(→ 400), and a hard-deleted planting would FK-fail; both reach the UI as an error string.PlopInspector"Remove plant" →onClose()→selectPlanting(null)) does not clearjournalPlantingId, so the "Notes about one planting" banner can linger on a plop the user can no longer reselect. I grepped every clearer ofjournalPlantingId(only the bed setter, the explicit "Show all", andresetTransient) and confirmed the bed equivalent has the identical property (journalObjectIdis not cleared on object deletion either). This is parity-brought-along, not introduced here — and it self-heals the moment the user picks "Show all" or scopes to a bed. Calling it a finding would be re-reporting the existing pattern.canEditis false whenseasonYear !== null(GardenEditorPage.tsx:145-146), and the button is behind{!readOnly && onAddNote && …}(PlopInspector.tsx:179), so a viewer or past-season user can't reach the composer. TheComposeritself is also gated by{canEdit && …}inJournalPanel.I verified all of the above by reading
JournalPanel.tsx,PlopInspector.tsx,store.ts,GardenEditorPage.tsx,lib/journal.ts,lib/objects.ts,internal/api/journal.go,internal/service/journal.go, andinternal/store/journal.go.opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
I reviewed the diff through the Security lens, tracing both the new frontend path and the existing backend it calls. Here's what I checked:
onAddNotecallback onPlopInspectoris gated by!readOnly(GardenEditorPage.tsx:506), andreadOnlyis set from!canEdit. The backendCreateJournalEntryalready requiresroleEditor(internal/service/journal.go:103) and validates the planting belongs to the requesting garden viaresolveJournalTarget(internal/service/journal.go:141-180), including a cross-check against the parent bed. No authz bypass introduced.plantingIdas a JSON field. The backend store layer uses?placeholders (internal/store/journal.go:50-53) for both read and write paths.listJournalhandler already parsesplantingIdthroughoptionalIDQuery, which rejects non-positive integers (internal/api/journal.go:55-58). The create path'sresolveJournalTargetvalidates existence and garden membership before writing.🎯 Correctness — No material issues found
No material issues found
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The change is clean and follows existing patterns well:
PlopInspectormirrors the bedInspector'sonAddNotecallback pattern exactly — same optional signature, same caller-side wiring inGardenEditorPage.journalObjectId/journalPlantingId) is consistent with the existingselectedId/selectedPlantingIdpattern, and theresetTransientclearance is complete.JournalPanel's filter construction is defensive (plop scope wins, with a local guard even though the store enforces mutual exclusion), and the conditional UI states (banner, composer label, empty-state copy) are all accounted for.Composerinternal prop extension (plantingId) is local and doesn't leak into the publicJournalPanelinterface beyond what's needed.⚡ Performance — No material issues found
No material issues found
🧯 Error handling & edge cases — No material issues found
No material issues found.
After tracing every code path touched by this PR through the lens of error handling and edge cases:
setJournalObjectIdalways clearsjournalPlantingId, and vice versa. Call sites inselectMode, the desktop Journal button, and the Journal panel dropdown all correctly use these setters, so a double-scope is impossible.scopePlantingIdis active,objectIdis explicitly passed asnulland converted toundefinedin the mutation payload, so the API receives onlyplantingId.scopePlantingIdprop defaults tonull, the optionalonScopePlantingChangeis present at the sole call site, and the empty-state copy branches correctly.plantingIdprecedence overobjectIdas a guard, matching the store's mutual-exclusion invariant.resetTransientincludesjournalPlantingId: null, and the garden-switch mount effect callsresetTransient.Nothing in this diff introduces an ignored error, missing cleanup, unhandled nil, boundary condition, or stale-scope bug.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.