docs(plan): Phase 17 — Graph as entity-scoped tab + Source migration (full GSD planning bundle)#230
docs(plan): Phase 17 — Graph as entity-scoped tab + Source migration (full GSD planning bundle)#230damienriehl wants to merge 10 commits into
Conversation
…to right pane
Adds the design contract and four validation sketches for Phase 17 (v0.5.0):
move the entity graph and the source view from their current homes (peer-tab
in Developer + hidden icon in Standard for graph; left-pane Tree|Source mode
strip + entity-actions </> Source link for source) into a unified right-pane
tab strip — Detail | Graph in Standard, Detail | Graph | Source in Developer.
Design contract: .planning/notes/graph-as-entity-pane.md
· 11 decisions covering layout, click semantics, tab persistence,
Source-as-Developer-tab, modal⇄Maximize round-trip with Restore
terminology, and tree→source auto-jump behavior spec.
Sketches: .planning/sketches/{001..004}/index.html
· 001 tab-strip-language → winner D (icon + label, ABOVE entity header)
· 002 standard-view-layout → winner D (Compact density + 50/50 tabs)
· 003 developer-view-and-modal → applied + Source-as-third-tab restructure
· 004 open-full-source → winner C (Modal ⇄ Maximize round-trip with
"Restore" matching Windows OS convention)
Roadmap: appended Phase 17 entry.
Tracker: ontokit-web#229 (under v0.5.0 milestone #214).
No code changes — design and planning artifacts only. Implementation lands
on its own feature branch via /gsd-spec-phase → /gsd-discuss-phase →
/gsd-plan-phase → /gsd-execute-phase, gated on PR #88 (entity graph) merging
to dev first.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Phase 17 planning, specs, and interactive sketches to relocate the entity graph into a right-pane entity-scoped "Detail | Graph" tab strip (Developer mode adds a Source tab), defines modal ⇄ maximize round-trip for full source, updates roadmap/STATE, and records API/backend contract and test plans. ChangesPhase 17 Planning & Sketches
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.planning/sketches/004-open-full-source/README.md (1)
56-63: ⚡ Quick winAdd modal accessibility requirements to implementation watch-outs.
Please include explicit focus-management/ARIA expectations (focus trap, initial focus target, return focus on close,
role="dialog"+aria-modal) for both modal and takeover transitions so the round-trip doesn’t regress keyboard/screen-reader flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/sketches/004-open-full-source/README.md around lines 56 - 63, Add explicit accessibility/focus-management rules to the modal/takeover implementation: in the reused EntityGraphModal component (and the takeover variant created when Maximize is invoked) ensure role="dialog" and aria-modal are set, implement a focus trap while open, set a clear initial focus target inside the modal body (e.g., first interactive element passed via the body slot or a dedicated focusable container), and restore focus to the element that opened the modal when it closes or when Restore toggles back; wire this into the source-overlay state (cameFromModal boolean) so toggling between Maximize and Restore preserves focus and scroll position, and ensure the global Esc handler still dismisses the entire source overlay (not just shrink to modal) while Restore only changes size without moving return focus unexpectedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.planning/ROADMAP.md:
- Line 208: Update the Phase 17 dependency statement to reflect the actual PR
gate: replace or modify the line "Depends on: none" in .planning/ROADMAP.md so
it references PR `#88` (entity graph) or otherwise marks the entity graph as a
prerequisite; ensure the text explicitly matches the PR gate (e.g., "Depends on:
PR `#88` (entity graph)") so scheduling and parallelization tools/readers will not
treat Phase 17 as independent.
- Line 211: The Phase 17 success criteria in ROADMAP.md conflicts with the
design contract: update the text so Developer view does not reduce the
right-pane tabs to "Tree | Source" but instead adopts the canonical "Detail |
Graph | Source" on the right-side detail pane (and remove any reference to a
left-pane "Tree | Source" tab strip for Developer layout); ensure the doc
explicitly states the Developer layout moves "Source" into the right-pane tabs
and that left-pane tab strip is removed so the behavior matches the design note
and avoids implementation drift.
In @.planning/sketches/002-standard-view-layout/README.md:
- Line 27: The spec for the Compact tab style is ambiguous about vertical
padding (it lists 8px in the Compact definition but later expects ~11px); pick
one consistent value or explicitly scope both by updating the "Compact"
definition and any later mentions so they match (e.g., change the Compact bullet
to "Compact — text-xs tab labels, 24px entity pip, 11px tab vertical padding" or
add a note like "8px applies to collapsed tabs, 11px applies to expanded tabs"),
and ensure every reference to Compact or tab padding in this file (including the
later ~11px mention) uses the same chosen value or the scoped explanation.
In @.planning/sketches/003-developer-view-and-modal/README.md:
- Around line 54-55: Update the README reference so implementers are directed to
the correct module: replace the mention of turtleClassUpdater.ts with
turtleBlockParser.ts and note that the existing helper findBlock(...) in
turtleBlockParser.ts should be reused when implementing
getEntitySourceSnippet(iri, ttlSource); also mention that findBlock is the
function to call to locate the Turtle block for an IRI and that the "Open full
source" affordance should reuse the existing full-file Source view logic and
scroll to the found block.
In @.planning/sketches/004-open-full-source/index.html:
- Around line 503-506: Rename all uses of the rejected "minimize" terminology to
"restore": change the element ID takeover-minimize-btn to takeover-restore-btn,
the CSS class .minimize-btn to .restore-btn, and the function minimizeToModal()
to restoreToModal(); update any HTML attributes (id, class, onclick/title text
references), corresponding JS references (function definition, calls, event
listeners) and any CSS selectors that target .minimize-btn so they target
.restore-btn instead to keep names consistent.
In @.planning/sketches/MANIFEST.md:
- Line 5: The MANIFEST entry for Phase 17 incorrectly says "eight" decisions;
update the sentence in .planning/sketches/MANIFEST.md that reads 'Read
`.planning/notes/graph-as-entity-pane.md` for the eight design decisions' to say
"eleven" to match the actual document; verify the referenced note file
`.planning/notes/graph-as-entity-pane.md` (whose heading reads "The design
contract (11 decisions)") is the source of truth and keep the rest of the
sentence unchanged.
---
Nitpick comments:
In @.planning/sketches/004-open-full-source/README.md:
- Around line 56-63: Add explicit accessibility/focus-management rules to the
modal/takeover implementation: in the reused EntityGraphModal component (and the
takeover variant created when Maximize is invoked) ensure role="dialog" and
aria-modal are set, implement a focus trap while open, set a clear initial focus
target inside the modal body (e.g., first interactive element passed via the
body slot or a dedicated focusable container), and restore focus to the element
that opened the modal when it closes or when Restore toggles back; wire this
into the source-overlay state (cameFromModal boolean) so toggling between
Maximize and Restore preserves focus and scroll position, and ensure the global
Esc handler still dismisses the entire source overlay (not just shrink to modal)
while Restore only changes size without moving return focus unexpectedly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71a4d8fd-b2f8-4046-9d3c-13c523b9487a
📒 Files selected for processing (12)
.planning/ROADMAP.md.planning/notes/graph-as-entity-pane.md.planning/sketches/001-tab-strip-language/README.md.planning/sketches/001-tab-strip-language/index.html.planning/sketches/002-standard-view-layout/README.md.planning/sketches/002-standard-view-layout/index.html.planning/sketches/003-developer-view-and-modal/README.md.planning/sketches/003-developer-view-and-modal/index.html.planning/sketches/004-open-full-source/README.md.planning/sketches/004-open-full-source/index.html.planning/sketches/MANIFEST.md.planning/sketches/themes/default.css
| ### Phase 17: Graph as Entity-Scoped Tab in Detail Pane | ||
| **Milestone**: v0.5.0 | ||
| **Goal**: Move the entity graph from a peer-tab in Developer view (and a hidden icon button in Standard view) into a `Detail | Graph` tab strip at the top of the right-side detail pane, applied uniformly across both views and across all three entity types (class, property, individual). Property graphs render real domain/range/parent-property edges (an explicit improvement over WebProtege's empty-island treatment). | ||
| **Depends on**: none (touches Standard and Developer layouts independently of other Phase 17+ work) |
There was a problem hiding this comment.
Align Phase 17 dependency with the stated PR gate.
Depends on: none conflicts with this PR’s stated implementation gate on PR #88 (entity graph). This can cause incorrect scheduling/parallelization assumptions in planning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/ROADMAP.md at line 208, Update the Phase 17 dependency statement
to reflect the actual PR gate: replace or modify the line "Depends on: none" in
.planning/ROADMAP.md so it references PR `#88` (entity graph) or otherwise marks
the entity graph as a prerequisite; ensure the text explicitly matches the PR
gate (e.g., "Depends on: PR `#88` (entity graph)") so scheduling and
parallelization tools/readers will not treat Phase 17 as independent.
|
|
||
| ## Variants | ||
|
|
||
| - **A: Compact** — text-xs tab labels, 24px entity pip, 8px tab padding. Tabs collapsed to the left, content-sized. |
There was a problem hiding this comment.
Unify tab vertical padding spec (8px vs ~11px).
The file defines Compact as 8px but later asks to keep ~11px. Pick one value (or explicitly scope when each applies) so implementation and visual QA don’t diverge.
Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/sketches/002-standard-view-layout/README.md at line 27, The spec
for the Compact tab style is ambiguous about vertical padding (it lists 8px in
the Compact definition but later expects ~11px); pick one consistent value or
explicitly scope both by updating the "Compact" definition and any later
mentions so they match (e.g., change the Compact bullet to "Compact — text-xs
tab labels, 24px entity pip, 11px tab vertical padding" or add a note like "8px
applies to collapsed tabs, 11px applies to expanded tabs"), and ensure every
reference to Compact or tab padding in this file (including the later ~11px
mention) uses the same chosen value or the scoped explanation.
| - **Entity-scoped source rendering.** New: needs a function `getEntitySourceSnippet(iri, ttlSource)` that finds the Turtle block for an IRI. Already exists in `lib/ontology/turtleClassUpdater.ts` (`findBlock`) — reuse it for the Source tab. | ||
| - **"Open full source" affordance.** Clicking it should open the existing full-file Source view in a modal or a new pane, scrolled to the entity's line. This preserves the "I need the whole file" workflow without making it the default. |
There was a problem hiding this comment.
Fix findBlock source-file reference.
This points implementers to lib/ontology/turtleClassUpdater.ts, but current usage evidence is in lib/ontology/turtleBlockParser.ts (where findBlock(...) is used). Please correct the reference to prevent wrong dependency assumptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/sketches/003-developer-view-and-modal/README.md around lines 54 -
55, Update the README reference so implementers are directed to the correct
module: replace the mention of turtleClassUpdater.ts with turtleBlockParser.ts
and note that the existing helper findBlock(...) in turtleBlockParser.ts should
be reused when implementing getEntitySourceSnippet(iri, ttlSource); also mention
that findBlock is the function to call to locate the Turtle block for an IRI and
that the "Open full source" affordance should reuse the existing full-file
Source view logic and scroll to the found block.
| <button id="takeover-minimize-btn" class="minimize-btn" onclick="minimizeToModal()" title="Restore to modal view" style="display:none;"> | ||
| <svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" style="vertical-align:-2px;"><polyline points="4 14 10 14 10 20"/><polyline points="20 10 14 10 14 4"/><line x1="14" y1="10" x2="21" y2="3"/><line x1="3" y1="21" x2="10" y2="14"/></svg> | ||
| Restore | ||
| </button> |
There was a problem hiding this comment.
CSS class, element ID, and function name use the rejected "minimize" terminology.
Decision 10 explicitly argues for "Restore" and against "Minimize" (OS muscle-memory conflict). The button label and title are correct (Restore / "Restore to modal view"), but the CSS class (.minimize-btn), element ID (takeover-minimize-btn), and minimizeToModal() function all use the rejected term. Implementors following this sketch will likely carry this naming into production.
✏️ Proposed alignment
- <button id="takeover-minimize-btn" class="minimize-btn" onclick="minimizeToModal()" title="Restore to modal view" style="display:none;">
+ <button id="takeover-restore-btn" class="restore-btn" onclick="restoreToModal()" title="Restore to modal view" style="display:none;">And in the JS:
- const minBtn = document.getElementById('takeover-minimize-btn');
+ const minBtn = document.getElementById('takeover-restore-btn');- function minimizeToModal() {
+ function restoreToModal() {Also update the CSS selector:
- .modal-actions .maximize-btn,
- .modal-actions .minimize-btn {
+ .modal-actions .maximize-btn,
+ .modal-actions .restore-btn {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/sketches/004-open-full-source/index.html around lines 503 - 506,
Rename all uses of the rejected "minimize" terminology to "restore": change the
element ID takeover-minimize-btn to takeover-restore-btn, the CSS class
.minimize-btn to .restore-btn, and the function minimizeToModal() to
restoreToModal(); update any HTML attributes (id, class, onclick/title text
references), corresponding JS references (function definition, calls, event
listeners) and any CSS selectors that target .minimize-btn so they target
.restore-btn instead to keep names consistent.
|
|
||
| ## Design Direction | ||
|
|
||
| Phase 17 — move the entity graph from a peer-tab in Developer view (and a small icon button in Standard view) into a `Detail | Graph` tab strip at the top of the right-side detail pane. Visual language matches OntoKit's existing app: sky-blue primary (`--color-primary-*`), slate neutrals, OWL-entity accent colors (class=green, property=blue, individual=pink), light + dark via CSS data-attribute. Folio-Enrich tab strip is the closest reference; WebProtege's docked entity graph is the loose mental model. Read `.planning/notes/graph-as-entity-pane.md` for the eight design decisions that constrain these sketches. |
There was a problem hiding this comment.
Stale decision count — "eight" should be "eleven".
graph-as-entity-pane.md documents 11 decisions (its own heading reads "The design contract (11 decisions)"). Decisions 9–11 were added during the sketch session, but the MANIFEST reference still says "eight."
✏️ Proposed fix
-Phase 17 — move the entity graph … Read `.planning/notes/graph-as-entity-pane.md` for the eight design decisions that constrain these sketches.
+Phase 17 — move the entity graph … Read `.planning/notes/graph-as-entity-pane.md` for the eleven design decisions that constrain these sketches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/sketches/MANIFEST.md at line 5, The MANIFEST entry for Phase 17
incorrectly says "eight" decisions; update the sentence in
.planning/sketches/MANIFEST.md that reads 'Read
`.planning/notes/graph-as-entity-pane.md` for the eight design decisions' to say
"eleven" to match the actual document; verify the referenced note file
`.planning/notes/graph-as-entity-pane.md` (whose heading reads "The design
contract (11 decisions)") is the source of truth and keep the rest of the
sentence unchanged.
…Pane — 11 requirements
Locks WHAT/WHY for Phase 17 (v0.5.0). 11 falsifiable requirements derived
from the design contract (.planning/notes/graph-as-entity-pane.md) + 2
rounds of Socratic refinement that pinned the cross-repo scope and
verification floor.
Key locked decisions surfaced in spec round 1-2:
· Phase 17 spans both ontokit-web and ontokit-api (web restructure +
api endpoint extension for property/individual focus IRIs)
· Hard dependency on PR #88 (entity graph) merging to dev before
Phase 17 implementation branch is cut
· Verification floor: Vitest unit tests + AI-driven Chrome DevTools
MCP smoke pass walking the success criteria in real browser before
human UAT (no Playwright e2e in this phase)
· Property graphs render full content per Decision 5: domain + range
+ parent properties + see-also (improvement over WebProtege's
empty-island treatment)
Ambiguity: 0.19 (gate ≤ 0.20). All 4 dimensions cleared their minimums.
Tracker: ontokit-web#229 (under v0.5.0 milestone #214).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…coped-tab-in-detail-pane/17-CONTEXT.md .planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-DISCUSSION-LOG.md
…e palette Locks visual + interaction contract for Phase 17. Concrete Tailwind classes for the right-pane PaneTabStrip (mirrors EntityTabBar density so bottom borders align), EntityModal rename + Maximize/Restore semantics, Source tab toolbar layout, edge palette extension for domain/range/subPropertyOf/rdfType/sameAs/objectProperty, empty-state copy for annotation properties, and full a11y/copy contracts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…entity-scoped-tab-in-detail-pane/17-VALIDATION.md
…l-pane Three-plan structure honoring SPEC R1-R11 + CONTEXT D-01..D-16: - 17-01-PLAN.md (cross-repo, autonomous: false, wave 1): Extend ontokit-api /entity-graph endpoint for property + individual focus, add 6 new edge_kind values (domain/range/subPropertyOf/rdfType/sameAs/ objectProperty), preserve /classes/graph as deprecating shim, add 14 pytest cases covering R11 acceptance literal. Sequential merge gate per D-08: api -> dev BEFORE web cuts its branch. - 17-02-PLAN.md (autonomous: true, wave 2, depends_on=17-01): Web primitives layer. PaneTabStrip + SourceTabBody + FullSourceOverlay + useFullSourceOverlay hook + selectionStore activePaneTab field + useEffectiveTab derived fallback (D-15) + EntityGraphModal -> EntityModal rename + OntologyEdge palette extension to 10 edge_kind values. CSS takeover rule (data-overlay-takeover) added to globals.css. 8 tasks + ~60 unit tests, no layout integration yet. - 17-03-PLAN.md (autonomous: false due to UAT checkpoint, wave 3, depends_on=17-02): Layout integration. Wire PaneTabStrip into Standard (Detail|Graph) and Developer (Detail|Graph|Source). Remove old Graph icon button (R3), Source link (R4), and Developer Tree|Source|Graph mode strip (R2). Wire useGraphData to pass focusType through. AI-driven Chrome DevTools MCP smoke test report covering R1, R5, R6, R7, R8, R9 across Standard+Developer x light+dark = 24-scenario matrix. Human UAT checklist mapped 1:1 to R1-R11. All three plans validate clean (gsd-sdk verify.plan-structure: valid=true). ROADMAP.md updated: Requirements: R1..R11; Plans: 3 plans. Phase artifacts cited: 17-SPEC.md (locked R1-R11), 17-CONTEXT.md (16 locked decisions), 17-RESEARCH.md (file paths, line numbers, pitfalls), 17-PATTERNS.md (analog files + code excerpts), 17-VALIDATION.md (Nyquist requirement-to-test mapping), 17-UI-SPEC.md (Tailwind tokens, ARIA roles, copy contract, locked edge palette). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- B1: 17-02 frontmatter requirements expanded to [R1, R6, R7, R8, R9, R10, R11] (Task 6 PaneTabStrip is load-bearing for R1; Task 7 FullSourceOverlay auto-scroll implements R10). - B2: 17-02 Task 7 — added Test 15 (Monaco instance ref persists across Maximize⇄Restore round-trip) and Test 16 (editor-tree-pane and editor-detail-pane testids remain in DOM during maximize). Concrete greppable test-name strings added to acceptance. - B3: 17-02 Task 8 — reconciled SPEC R11 vs UI-SPEC empty-state copy collision. SPEC literal now ships as aria-label on container; UI-SPEC heading + body remain visible. Tests grep for all three strings. - B4: 17-02 Task 7 — removed TODO escape hatch for R10 auto-scroll. Now mandates TurtleEditor refactor to forwardRef + useImperativeHandle exposing revealLineInCenter (mirrors existing OntologySourceEditor pattern). Added Test 17 (focusIri change calls revealLineInCenter). - B5: 17-03 Task 1 — added positive-absence assertions (queryByText / queryByLabelText / queryByRole) for the Source link in PropertyDetailPanel.test.tsx, IndividualDetailPanel.test.tsx, and ClassDetailPanel.test.tsx. Replaces the vacuous grep-of-absent-string. All three plans pass frontmatter + structure validation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@JohnRDOrazio review request — this is the full GSD planning bundle for Phase 17 (entity graph + Source moved into a unified right-pane tab strip in both Standard and Developer views). Zero code, all design + spec + plans. The four sketches under |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-01-PLAN.md:
- Around line 7-12: The plan's metadata key files_modified is missing the
required test artifact tests/unit/test_graph_schema.py referenced later (lines
375–377); update the files_modified list to include
tests/unit/test_graph_schema.py alongside the existing entries
(ontokit/api/routes/projects.py, ontokit/services/ontology.py,
ontokit/schemas/graph.py, tests/unit/test_entity_graph.py) so the execution
tooling will include and run the schema test.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-02-PLAN.md:
- Line 263: The markdown uses indented code blocks (e.g., the example line
adding the PaneTab type export `export type PaneTab = "detail" | "graph" |
"source";`) which triggers MD046; replace each indented snippet with a fenced
code block using triple backticks and an appropriate language tag (```ts for
TypeScript examples) so the snippet is fenced and syntax-highlighted; apply this
change to the mentioned occurrences (lines cited in the review) to ensure
consistent fenced blocks throughout the document.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-03-PLAN.md:
- Around line 808-814: The table cell in the T-17-13 row currently contains the
unescaped union literal class"|"property"|"individual which is breaking markdown
table parsing and triggering MD056/MD055; edit the T-17-13 cell (the
`useGraphData focusType param threading` row) and either escape the pipe
characters as class"\|"\|property"\|"\|individual or wrap the entire union in
inline code/backticks (e.g., `class|property|individual`) so the pipes are not
treated as column separators.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-CONTEXT.md:
- Around line 69-72: Normalize the activePaneTab contract by removing null as an
allowed value and making activePaneTab strictly 'detail' | 'graph' | 'source' in
useSelectionStore and all phase docs; ensure useSelectionStore initializes to
'detail' and that any reset logic (referenced by D-14/D-15) explicitly sets
activePaneTab = 'detail', and update useEffectiveTab(editorMode) wording to
treat activePaneTab as always present (only deriving a fallback from 'source'
when editorMode === 'standard') so downstream plans/tests assume a non-null
default.
- Around line 51-52: The comment flags conflicting guidance: don't re-implement
type-agnostic block-finding in lib/ontology/turtleClassUpdater.ts; instead adapt
the existing turtleUtils.findBlock wrapper to support both property and
individual-subject patterns and call that from turtleClassUpdater.ts; update
turtleClassUpdater.ts to use turtleUtils.findBlock (not a new findBlock) and
ensure it consumes the Web Worker IRI index from lib/editor/indexWorker.ts for
start line numbers; also adjust any references at lines ~102-103 and ~125-126 to
match the unified wrapper approach so all callers share the same type-agnostic
contract.
In
@.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-PATTERNS.md:
- Around line 708-712: The fenced code block that currently starts with the
lines containing the Tailwind utility classes (e.g., "flex border-b
border-slate-200 dark:border-slate-700" and "flex-1 px-3 py-2 text-xs
font-medium transition-colors") is missing a language identifier; update the
opening ``` fence to include a language such as css or html (e.g., ```css) to
satisfy MD040 and enable proper syntax highlighting for the utility-class
snippet.
In
@.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-RESEARCH.md:
- Line 42: The MD040/MD058 warnings are caused by missing fenced-code language
identifiers and tables lacking surrounding blank lines; add explicit language
tags (e.g., ```md or ```text) to every fenced code block and ensure there is a
blank line immediately before and after each markdown table (for example the
table starting with "| Library | Version | Purpose | Why Standard |"); apply the
same fixes to the other table occurrences noted (lines referenced in your
comment) so all fenced blocks have languages and all tables are separated by
blank lines.
- Around line 246-247: The doc contains a contradiction about rollout behavior
between the new /entity-graph endpoint and the existing /classes/graph shim:
update the text to be consistent by either (A) removing or adjusting the claim
that out-of-order frontend deploys won’t break due to the /classes/graph shim
(since that shim does not cover /entity-graph), or (B) explicitly document that
the /classes/graph shim only protects the classes route and that moving to
/entity-graph will allow the web to break if the API isn’t deployed first;
reference and change the sentences that mention "/entity-graph" and
"/classes/graph" and clarify the rollout guidance near the paragraphs that also
reference DeveloperEditorLayout/sourceIriIndex to keep the earlier note about no
feature flag or try/catch fallback consistent with the chosen approach.
In @.planning/STATE.md:
- Around line 6-14: The frontmatter field stopped_at currently says "Phase 17"
but the active-position/progress block still reports Phase 16 and 70%—update the
active-position and progress fields to match the frontmatter: set
active-position.phase to Phase 17, update progress.completed_phases/total_phases
and completed_plans/total_plans (or at minimum progress.percent) so the percent
reflects the 9/11 -> 92% values shown in stopped_at, and ensure any
session-continuity fields mirror stopped_at to remove the conflicting sources of
truth (fields to edit: stopped_at, active-position, and progress).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 526c150c-6ace-467c-baef-164088151eec
📒 Files selected for processing (12)
.planning/ROADMAP.md.planning/STATE.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-01-PLAN.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-02-PLAN.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-03-PLAN.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-CONTEXT.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-DISCUSSION-LOG.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-PATTERNS.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-RESEARCH.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-SPEC.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-UI-SPEC.md.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-VALIDATION.md
✅ Files skipped from review due to trivial changes (3)
- .planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-UI-SPEC.md
- .planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-DISCUSSION-LOG.md
- .planning/ROADMAP.md
| files_modified: | ||
| - "ontokit-api: ontokit/api/routes/projects.py" | ||
| - "ontokit-api: ontokit/services/ontology.py" | ||
| - "ontokit-api: ontokit/schemas/graph.py" | ||
| - "ontokit-api: tests/unit/test_entity_graph.py" | ||
| autonomous: false |
There was a problem hiding this comment.
files_modified is missing a required test artifact
Lines 375–377 require tests/unit/test_graph_schema.py, but it is not declared in files_modified (Lines 7–12). If execution tooling enforces this list, the schema test can be skipped or flagged as out-of-scope.
Also applies to: 375-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-01-PLAN.md
around lines 7 - 12, The plan's metadata key files_modified is missing the
required test artifact tests/unit/test_graph_schema.py referenced later (lines
375–377); update the files_modified list to include
tests/unit/test_graph_schema.py alongside the existing entries
(ontokit/api/routes/projects.py, ontokit/services/ontology.py,
ontokit/schemas/graph.py, tests/unit/test_entity_graph.py) so the execution
tooling will include and run the schema test.
| <action> | ||
| **Sub-step A — Extend `lib/stores/selectionStore.ts`** per D-13/D-14/D-15 and RESEARCH Code Example 3: | ||
|
|
||
| 1. Add a `PaneTab` type export at the top: `export type PaneTab = "detail" | "graph" | "source";` |
There was a problem hiding this comment.
Convert indented code snippets to fenced code blocks
These sections use indented code blocks where markdownlint expects fenced style (MD046). Converting them to fenced blocks will keep docs lint clean and preserve syntax highlighting consistently.
Also applies to: 356-356, 416-416, 474-474, 602-602, 619-619, 771-771, 850-850, 873-873, 1120-1120
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 263-263: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-02-PLAN.md
at line 263, The markdown uses indented code blocks (e.g., the example line
adding the PaneTab type export `export type PaneTab = "detail" | "graph" |
"source";`) which triggers MD046; replace each indented snippet with a fenced
code block using triple backticks and an appropriate language tag (```ts for
TypeScript examples) so the snippet is fenced and syntax-highlighted; apply this
change to the mentioned occurrences (lines cited in the review) to ensure
consistent fenced blocks throughout the document.
| | T-17-13 | T (Tampering) | useGraphData focusType param threading | mitigate | TypeScript Literal "class"|"property"|"individual" enforces compile-time validation; default of "class" preserves behavior on missing values | | ||
| | T-17-14 | I (Information disclosure) | OntologyGraph rendered in Graph tab | accept | Read-only graph; no PII; same data already accessible via the existing /classes/graph route | | ||
| | T-17-15 | D (Denial of service) | Maximize takeover via CSS attribute toggle | accept | Single document attribute write per state transition; useEffect cleanup guarantees attribute removal on unmount; bounded transitions | | ||
| | T-17-16 | E (Elevation of privilege) | Tab persistence in selectionStore (non-persist) | accept | activePaneTab is session-only client state; no privileged action; D-14 explicitly chose non-persist to avoid stale-state attacks across reload | | ||
| | T-17-17 | T (Tampering) | EntityModal aria-labelledby titleId | mitigate | titleId is rendered as id attribute on a known element controlled by the layout; no user-supplied IDs reach this attribute | | ||
| | T-17-18 | I (Information disclosure) | Smoke test screenshots written to $HOME | mitigate | Screenshots are deleted by Task 3 Sub-step E (per CLAUDE.md global instructions); no persistent artifact in repo or home dir | | ||
| </threat_model> |
There was a problem hiding this comment.
Escape pipe characters in STRIDE table cell content
Line 808 includes class"|"property"|"individual inside a table cell. The unescaped pipes split columns and trigger MD056/MD055; escape them (\|) or wrap the union in backticks without raw pipes.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 808-808: Table column count
Expected: 5; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 814-814: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing leading pipe
(MD055, table-pipe-style)
[warning] 814-814: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 814-814: Table column count
Expected: 5; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-03-PLAN.md
around lines 808 - 814, The table cell in the T-17-13 row currently contains the
unescaped union literal class"|"property"|"individual which is breaking markdown
table parsing and triggering MD056/MD055; edit the T-17-13 cell (the
`useGraphData focusType param threading` row) and either escape the pipe
characters as class"\|"\|property"\|"\|individual or wrap the entire union in
inline code/backticks (e.g., `class|property|individual`) so the pipes are not
treated as column separators.
| - **D-01:** Snippet computed **frontend-only** from already-loaded full source. Generalize `findBlock` in `lib/ontology/turtleClassUpdater.ts` to handle property + individual subject patterns (currently class-only). Use the Web Worker IRI index from `lib/editor/indexWorker.ts` for the start line number. | ||
| - **D-02:** When the extractor cannot locate the entity (e.g., entity defined in an imported ontology, malformed source), render an **empty-state body**: `"Source not available for this entity — it may be defined in an imported ontology."` Toolbar still shows `↗ Open full source` so the user can verify in the full file. |
There was a problem hiding this comment.
Unify snippet-extraction contract (current text conflicts with locked phase guidance)
Line 51 and related references still direct a findBlock generalization in turtleClassUpdater.ts, but the phase pattern contract says turtleUtils.findBlock is already type-agnostic and only a wrapper should be added. Keeping both instructions risks divergent implementations.
Also applies to: 102-103, 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-CONTEXT.md
around lines 51 - 52, The comment flags conflicting guidance: don't re-implement
type-agnostic block-finding in lib/ontology/turtleClassUpdater.ts; instead adapt
the existing turtleUtils.findBlock wrapper to support both property and
individual-subject patterns and call that from turtleClassUpdater.ts; update
turtleClassUpdater.ts to use turtleUtils.findBlock (not a new findBlock) and
ensure it consumes the Web Worker IRI index from lib/editor/indexWorker.ts for
start line numbers; also adjust any references at lines ~102-103 and ~125-126 to
match the unified wrapper approach so all callers share the same type-agnostic
contract.
| - **D-13:** `activePaneTab: 'detail' | 'graph' | 'source' | null` is added to **`useSelectionStore`** (per SPEC Requirement 6 literal — "useSelectionStore gains an activePaneTab field"). Selection-related state stays colocated. | ||
| - **D-14:** **Session-only persistence** — Zustand non-persist (selectionStore is already non-persist). Page reload starts on Detail. Matches SPEC Requirement 8 literal: "first entity opened in a NEW SESSION lands on Detail." | ||
| - **D-15:** **Derived fallback at render** — components consume `useEffectiveTab(editorMode)` which returns `(activePaneTab === 'source' && editorMode === 'standard') ? 'detail' : activePaneTab`. The store value is **never mutated** by mode change. User returning to Developer view restores their Source tab preference. Most graceful fallback semantics. | ||
| - **D-16:** **No URL deep-linking** for the active tab — tab is session state, not URL state. URL stays focused on entity (`?classIri=...`, `?propertyIri=...`, `?individualIri=...`). Deep-linkable tabs deferred to v0.6+ backlog. |
There was a problem hiding this comment.
Normalize activePaneTab type contract across phase docs
Line 69 allows activePaneTab to be null, but D-14/D-15 and downstream plan/test contracts assume default "detail" and reset-to-"detail" semantics. This ambiguity can produce incompatible store interfaces between plans.
🧰 Tools
🪛 LanguageTool
[grammar] ~69-~69: Ensure spelling is correct
Context: ... field"). Selection-related state stays colocated. - D-14: **Session-only persistence...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-CONTEXT.md
around lines 69 - 72, Normalize the activePaneTab contract by removing null as
an allowed value and making activePaneTab strictly 'detail' | 'graph' | 'source'
in useSelectionStore and all phase docs; ensure useSelectionStore initializes to
'detail' and that any reset logic (referenced by D-14/D-15) explicitly sets
activePaneTab = 'detail', and update useEffectiveTab(editorMode) wording to
treat activePaneTab as always present (only deriving a fallback from 'source'
when editorMode === 'standard') so downstream plans/tests assume a non-null
default.
| ``` | ||
| flex border-b border-slate-200 dark:border-slate-700 | ||
| flex-1 px-3 py-2 text-xs font-medium transition-colors | ||
| ``` | ||
| Active: `border-b-2 border-primary-600 text-primary-600 dark:border-primary-400 dark:text-primary-400`. Phase 17 adds the accent-tint background `bg-primary-50 dark:bg-primary-900/20` to the existing pattern. |
There was a problem hiding this comment.
Add a language identifier to this fenced code block
Line 708 opens a fence without a language, which triggers MD040 and reduces readability/syntax highlighting.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 708-708: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-PATTERNS.md
around lines 708 - 712, The fenced code block that currently starts with the
lines containing the Tailwind utility classes (e.g., "flex border-b
border-slate-200 dark:border-slate-700" and "flex-1 px-3 py-2 text-xs
font-medium transition-colors") is missing a language identifier; update the
opening ``` fence to include a language such as css or html (e.g., ```css) to
satisfy MD040 and enable proper syntax highlighting for the utility-class
snippet.
| ## Standard Stack | ||
|
|
||
| ### Core (already in repo — versions verified from `package.json` 2026-05-02) | ||
| | Library | Version | Purpose | Why Standard | |
There was a problem hiding this comment.
Fix markdownlint warnings in fenced blocks/tables.
Static analysis flags MD040/MD058 here. Please add fenced-code language identifiers and blank lines around tables to keep docs lint-clean.
Also applies to: 58-58, 64-64, 99-99, 153-153, 313-313, 480-480, 758-758, 770-770
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 42-42: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-RESEARCH.md
at line 42, The MD040/MD058 warnings are caused by missing fenced-code language
identifiers and tables lacking surrounding blank lines; add explicit language
tags (e.g., ```md or ```text) to every fenced code block and ensure there is a
blank line immediately before and after each markdown table (for example the
table starting with "| Library | Version | Purpose | Why Standard |"); apply the
same fixes to the other table occurrences noted (lines referenced in your
comment) so all fenced blocks have languages and all tables are separated by
blank lines.
| - **Don't add a feature flag or try/catch fallback for the new `/entity-graph` endpoint.** D-08: sequential cross-repo merge order — api ships first to dev, then web cuts from dev. No graceful degradation; if API isn't deployed, web breaks (and that's fine — it's a coordinated release). | ||
| - **Don't re-parse the full Turtle source for line numbers.** The Web Worker IRI index already maps every subject IRI to `{line, col, len}`. Read it from `sourceIriIndex` (already a `Map<string, IriPosition>` plumbed through `DeveloperEditorLayout` — lines 91, 152, 366). |
There was a problem hiding this comment.
Resolve the rollout-contract contradiction for web/API ordering.
Line 246 says web can break if API is not deployed yet (and that is acceptable), but Line 735 says out-of-order frontend deploys won’t break because of the /classes/graph shim. Those statements conflict once web migrates to /entity-graph.
Proposed doc fix
- The one cross-repo coordination is the API endpoint URL (`/classes/graph` → `/entity-graph`). D-05 mitigates by keeping `/classes/graph` as a delegating shim during the deprecation window, so even an out-of-order frontend deploy doesn't break.
+ The one cross-repo coordination is the API endpoint URL (`/classes/graph` → `/entity-graph`).
+ The `/classes/graph` shim protects older clients only.
+ New web builds that call `/entity-graph` still require API-first deployment/merge sequencing (D-08).Also applies to: 735-736
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/17-RESEARCH.md
around lines 246 - 247, The doc contains a contradiction about rollout behavior
between the new /entity-graph endpoint and the existing /classes/graph shim:
update the text to be consistent by either (A) removing or adjusting the claim
that out-of-order frontend deploys won’t break due to the /classes/graph shim
(since that shim does not cover /entity-graph), or (B) explicitly document that
the /classes/graph shim only protects the classes route and that moving to
/entity-graph will allow the web to break if the API isn’t deployed first;
reference and change the sentences that mention "/entity-graph" and
"/classes/graph" and clarify the rollout guidance near the paragraphs that also
reference DeveloperEditorLayout/sourceIriIndex to keep the earlier note about no
feature flag or try/catch fallback consistent with the chosen approach.
| 11. **Property and individual graph backend**: The entity-graph backend supports class, property, AND individual focus IRIs. | ||
| - Current: `ontokit-api` PR #37 (`entity-graph-endpoint`) supports `focus_iri` typed as a class only. Property and individual graphs are not implemented. | ||
| - Target: The `/api/v1/projects/{id}/ontology/classes/graph` endpoint (or equivalents per type) returns BFS neighborhood for class, property, and individual focus IRIs. **For properties:** nodes include the property itself (focus), domain classes, range classes, parent properties (rdfs:subPropertyOf chain), and see-also targets; edges are labelled `domain`, `range`, `subPropertyOf`, `seeAlso`. **For individuals:** nodes include the individual itself, class assertions (rdf:type), individuals connected via object property values, see-also targets, and sameAs targets; edges labelled `rdf:type`, `<predicate>`, `seeAlso`, `sameAs`. **For annotation properties:** when no domain, range, parent, or see-also exists, the response is `{nodes: [focus_only], edges: []}` and the frontend renders an empty-state message ("No relationships to display for this annotation property"). | ||
| - Acceptance: Backend endpoint returns non-empty `nodes` and `edges` for at least one object property in the test ontology with domain + range; for at least one individual with rdf:type + ≥1 object property value; for at least one annotation property with see-also. Annotation property with no relationships returns the focus-only response and frontend shows the empty-state copy. | ||
|
|
There was a problem hiding this comment.
Lock Requirement 11 to one endpoint contract (/entity-graph).
Requirement 11 is currently permissive (/classes/graph or equivalents), which can lead to split implementations. Since this phase already defines unified focus_iri + focus_type, the spec should explicitly make /entity-graph authoritative and describe /classes/graph only as a temporary deprecated shim.
Proposed spec wording
- - Target: The `/api/v1/projects/{id}/ontology/classes/graph` endpoint (or equivalents per type) returns BFS neighborhood for class, property, and individual focus IRIs.
+ - Target: The authoritative endpoint is `/api/v1/projects/{id}/ontology/entity-graph?focus_iri=...&focus_type=class|property|individual`, returning BFS neighborhoods for class, property, and individual focus IRIs. `/classes/graph` remains a deprecated delegating shim during the transition window only.| stopped_at: Phase 17 plans approved | ||
| last_updated: "2026-05-02T18:49:51.167Z" | ||
| last_activity: 2026-04-08 | ||
| progress: | ||
| total_phases: 10 | ||
| total_phases: 11 | ||
| completed_phases: 9 | ||
| total_plans: 35 | ||
| total_plans: 38 | ||
| completed_plans: 35 | ||
| percent: 100 | ||
| percent: 92 |
There was a problem hiding this comment.
Resolve Phase-state drift between frontmatter/session continuity and current-position sections
Line 6 and Lines 212–214 now point to Phase 17, but the active-position block still reports Phase 16 and 70% progress. This leaves two conflicting sources of truth in the same state file and can break resume handoff behavior.
Also applies to: 212-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/STATE.md around lines 6 - 14, The frontmatter field stopped_at
currently says "Phase 17" but the active-position/progress block still reports
Phase 16 and 70%—update the active-position and progress fields to match the
frontmatter: set active-position.phase to Phase 17, update
progress.completed_phases/total_phases and completed_plans/total_plans (or at
minimum progress.percent) so the percent reflects the 9/11 -> 92% values shown
in stopped_at, and ensure any session-continuity fields mirror stopped_at to
remove the conflicting sources of truth (fields to edit: stopped_at,
active-position, and progress).
Sets up #229. Zero code changes —
.planning/artifacts only. Direct-commit todevwas blocked by branch protection ("4 of 4 required status checks are expected"); this PR is the path the repo policy enforces.What's in this PR
The full Get-Shit-Done planning chain for Phase 17 — design contract, sketches, spec, context, UI spec, validation strategy, and three execution plans. No implementation code yet — that lands in a separate feature branch after this merges, on top of PR #88 (entity graph) once it's in
dev.Design (4 throwaway HTML mockups + decision contract)
.planning/notes/graph-as-entity-pane.md.planning/sketches/001-tab-strip-language/.planning/sketches/002-standard-view-layout/.planning/sketches/003-developer-view-and-modal/.planning/sketches/004-open-full-source/Open any sketch's
index.htmldirectly in a browser; corner toolbar toggles dark mode.Phase artifacts (
.planning/phases/17-graph-as-entity-scoped-tab-in-detail-pane/)17-SPEC.md17-CONTEXT.md17-DISCUSSION-LOG.md17-PATTERNS.md17-RESEARCH.md17-UI-SPEC.md17-VALIDATION.md17-01-PLAN.md17-02-PLAN.md17-03-PLAN.md.planning/ROADMAP.mdupdated with the Phase 17 entry..planning/STATE.mdcarries session bookkeeping for each step.Cross-repo scope
Wave 1 lives in
CatholicOS/ontokit-api— the backend entity-graph endpoint (currently class-only) needs to handle property and individual focus IRIs with full domain/range/parent/see-also (properties) and rdf:type/object-property/sameAs (individuals). That work depends on this PR for the contract and spec; the api PR will be filed separately once this merges.Waves 2-3 live in
ontokit-web, on a new feature branch cut fromdevafter PR #88 (entity graph server-side BFS) merges. Hard dependency.Why merge planning before implementation
devso anyone implementing or reviewing canopen .planning/sketches/{NNN}/index.htmland see the target design.Tracker + issue
What's NOT in this PR
.tsx/.pysource changesImplementation lands separately:
ontokit-apibranch for Wave 1 (filed after this merges)ontokit-webfeature branch for Waves 2-3 (cut fromdevafter PR Port entity graph to server-side BFS endpoint #88 merges)🤖 Generated with Claude Code