fix: capture title from the json

[soundaryakp1999]

Pull Request

Description

Previously, the l t shortcut (announce plot title) was not capturing the title authored in the MAIDR JSON. For plots that omitted a top-level title, the announcement read "Title is MAIDR Plot" (the model's internal placeholder); for single-panel plots that authored their title at the layer level (e.g. examples/multiline_plot.html), l t never consulted the layer title and announced "No title available" — even though the d (description) command correctly showed the authored title.

Related Issues

Closes #613

Changes Made

After this change, l t reads the title strictly from the MAIDR JSON: it announces the authored title when one is provided (at either the figure or the layer level), and otherwise announces "No title available". The l t and d commands now agree on what counts as a real title.
All changes are in src/command/describe.ts:

• The title-announce command now ignores the model's placeholder defaults ("MAIDR Plot", "unavailable") and only announces titles authored in the JSON.
• For trace-level scope, the authored layer title is now preferred in both single- and multi-panel figures (labeled "Title" in single-panel, "Subplot title" in multi-panel), falling back to the authored figure title, then to "No title available".
• Verbose unavailable phrasing changed from "Title is not available" to "No title available". Terse output stays "unavailable" to match the convention used by the X/Y/Z/subtitle/caption announce commands.

No model-layer changes — the placeholder defaults still flow through the model for the UI and description paths that rely on them; only the title-announce command filters them out at announce time.

Screenshots (if applicable)

Checklist

• I have read the Contributor Guidelines.
• I have performed a self-review of my own code and ensured it follows the project's coding standards.
• I have tested the changes locally following ManualTestingProcess.md, and all tests related to this pull request pass.
• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation, if applicable.
• I have added appropriate unit tests, if applicable.

Additional Notes

[claude]

Code Review

Overview

This PR fixes a real UX bug: the l t (announce title) shortcut was announcing the model's internal placeholder strings ("MAIDR Plot", "unavailable") instead of the actual authored title from the MAIDR JSON, causing l t and d (describe) to disagree. The core logic change is correct and the fix is well-scoped to src/command/describe.ts.

---

What Works Well

• Minimal blast radius – all changes are in one file; the model layer is untouched.
• Clear comments – the JSDoc on isAuthoredTitle and the updated announceTraceTitle clearly explain the intent.
• Single-panel fix – the old code only looked at the trace-level title when isMultiPanel was true. The new code checks the trace title first regardless of panel count, which is the right precedence and matches DescriptionService.

---

Concerns

1. Cross-layer coupling (moderate)

// src/command/describe.ts
import { DEFAULT_SUBPLOT_TITLE } from '@model/abstract';
import { DEFAULT_FIGURE_TITLE } from '@model/plot';

The command layer now imports internal model constants to filter placeholder values. If either constant is renamed or its value changes, isAuthoredTitle silently breaks. Commands already receive Context (model layer), but importing private defaults is a tighter coupling than necessary.

Suggested alternative: move the authored-title predicate closer to where the defaults live. Export isAuthoredTitle from @model/abstract (it only depends on model constants), then import it in the command. Or expose it via Context:

// model/context.ts
public get authoredFigureTitle(): string | null {
  const t = this.figureTitle;
  return isAuthoredTitle(t) ? t : null;
}

Either approach keeps the model's internal defaults encapsulated in the model layer.

---

2. Fragile empty-state path in context.figureTitle

context.figureTitle returns the raw string 'unavailable' when the figure state is empty — not from any exported constant, but as a hard-coded literal. The new isAuthoredTitle catches it only because 'unavailable' === DEFAULT_SUBPLOT_TITLE. This is a coincidental match rather than a deliberate contract; if DEFAULT_SUBPLOT_TITLE changes the empty-state path leaks through.

// context.ts — current code
public get figureTitle(): string {
  const figureState = this.figure.state;
  if (!figureState.empty) {
    return figureState.title;
  }
  return 'unavailable';  // ← hard-coded literal, not the exported constant
}

Minor fix: change that literal to return DEFAULT_SUBPLOT_TITLE so the filter is guaranteed to match.

---

3. No tests added

The checklist item for unit tests is unchecked, and the test directory has no coverage for AnnounceTitleCommand. Given that this bug was a silent mismatch between two commands, tests would prevent regression. At minimum, three cases deserve coverage:

Scenario	Expected output
Figure has authored title in JSON	Announces that title
Figure title omitted; layer has authored title (single-panel)	Announces layer title as "Title"
Neither figure nor layer title authored	"No title available" (verbose) / "unavailable" (terse)

---

4. User-facing string change (minor)

The verbose unavailable message changes from "Title is not available" to "No title available". This is a breaking change for any screen-reader user or test fixture that matches the exact string. The PR description acknowledges this was intentional for consistency — worth an explicit confirmation from a reviewer before merging.

---

Summary

The fix is correct and the approach is sound. The main asks before merging:

1. Either encapsulate isAuthoredTitle in the model layer (or Context) or add a comment explicitly documenting why the command layer depends on these two model-layer constants.
2. Fix the hard-coded 'unavailable' literal in context.figureTitle to use the exported constant.
3. Add at least the three unit-test cases above to prevent regression.

[claude]

Code Review — PR #616: fix: capture title from the json

Overview

This PR fixes a real and observable UX bug: the l t (announce title) shortcut was announcing the model's internal placeholder defaults ("MAIDR Plot", "No title available") instead of the title authored in the MAIDR JSON, and was inconsistent with what the d (description) command showed. The fix is generally correct and well-motivated.

---

Positive Aspects

• Correct root-cause fix. The isAuthoredTitle() helper cleanly encapsulates the sentinel-value logic and keeps the command layer from reaching into model internals.
• Good test coverage for the new behavior — 6 targeted unit tests cover single-panel, multi-panel, terse mode, and fallback scenarios.
• No model-layer mutations — the placeholder defaults still flow through for the UI/description paths that rely on them; only the announce command filters them.
• Clear PR description explaining the exact scenarios that were broken.

---

Issues and Suggestions

1. Logic duplication between isAuthoredTitle() and DescriptionService (medium)

Context.isAuthoredTitle() and the inline logic in DescriptionService.getDescription() both independently check DEFAULT_FIGURE_TITLE / DEFAULT_SUBPLOT_TITLE:

// context.ts (new)
return title !== DEFAULT_FIGURE_TITLE && title !== DEFAULT_SUBPLOT_TITLE;

// description.ts (unchanged)
const hasFigureTitle = figureTitle && figureTitle !== DEFAULT_FIGURE_TITLE && figureTitle !== DEFAULT_SUBPLOT_TITLE;

DescriptionService doesn't use the new isAuthoredTitle() helper even though the logic is identical. If a new default constant is added in the future, it needs to be updated in two places. Consider having DescriptionService call this.context.isAuthoredTitle() to consolidate this logic.

Additionally, description.ts also checks truthiness of the string (figureTitle &&), while isAuthoredTitle() does not. Empty strings would pass isAuthoredTitle() — a minor inconsistency that's not a bug today but could become one.

2. Inconsistent "unavailable" phrasing in verbose mode (minor)

The PR changes the verbose unavailable message for title to "No title available", but the sibling commands use a different pattern:

Command	Verbose unavailable text
Title (this PR)	"No title available"
Subtitle (unchanged)	"Subtitle is not available"
Caption (unchanged)	"Caption is not available"

These three related commands now have inconsistent phrasing. Either align all three to the new pattern or keep the original for consistency.

3. Subtitle/Caption commands use raw string comparison (minor)

AnnounceSubtitleCommand and AnnounceCaptionCommand still compare directly against the string literal 'unavailable':

if (subtitle !== 'unavailable') { ... }

Whereas the title command now uses isAuthoredTitle(). If DEFAULT_SUBTITLE or DEFAULT_CAPTION were changed, these checks would silently break. Consider extending isAuthoredTitle() (or a sibling helper) to cover these too, or at least import the constants rather than using the raw string.

4. isAuthoredTitle() does not handle empty strings (minor)

A title authored as "" (empty string) is not a placeholder default, but isAuthoredTitle("") returns true. Announcing "Title is " would be confusing. The old code had no guard for this either, so it is not a regression, but worth documenting or guarding.

5. Magic-string collision risk (informational)

If a user authors a title with the exact value "MAIDR Plot" or "unavailable", isAuthoredTitle() will incorrectly treat it as a placeholder. This is an inherent limitation of the sentinel-value approach in the model layer. A comment in isAuthoredTitle() noting this known limitation would help future maintainers.

6. Missing test cases

• state.empty === true path in execute() (currently unreachable in tests)
• state.type === 'subplot' fallback branch (the // Fallback for unexpected state types comment)
• Multi-panel with no authored title at either level (falls through both isAuthoredTitle checks to announceUnavailable())
• The figureTitle getter when figureState.empty is true

---

Summary

The bug fix is correct and meaningfully improves accessibility. The main actionable items before merge are:

1. Have DescriptionService reuse isAuthoredTitle() to avoid diverging filter logic (medium priority).
2. Align or document the verbose-mode phrasing inconsistency with subtitle/caption (minor, but affects screen-reader UX consistency).
3. Add a test for the state.empty path and the subtitle/caption raw-string comparison gap.

Everything else is informational. Good work on the test file structure and the clean encapsulation of the sentinel-value check in Context.

[claude]

Code Review

Overview

This PR fixes a real usability bug: the l t shortcut was announcing placeholder defaults ("MAIDR Plot", "Title is MAIDR Plot") rather than the JSON-authored title, and single-panel plots with only a layer-level title were getting "No title available" even when a title existed. The root cause is correctly identified — the announce logic was not distinguishing model-internal placeholder strings from author-supplied values.

The fix is clean and the approach is sound overall.

---

What Works Well

• Correct encapsulation. Placing isAuthoredTitle/Subtitle/Caption on Context is the right call. Commands and services no longer need to import internal model constants (DEFAULT_FIGURE_TITLE, DEFAULT_SUBPLOT_TITLE, etc.) directly — the Context acts as the facade.
• DescriptionService cleanup (description.ts) is a nice bonus: the ad-hoc constant comparisons are replaced with the new context.isAuthoredTitle(), keeping behavior consistent between l t and d.
• Well-structured tests covering the main branches: empty state, figure scope, trace scope single/multi-panel, terse mode, and the "no authored title" fallbacks.
• Clear, accurate docstrings including the known-limitation caveat about sentinel string collision.

---

Issues & Suggestions

1. isAuthoredSubtitle / isAuthoredCaption have no tests (and AnnounceSubtitleCommand / AnnounceCaptionCommand are untested)

The PR modifies the unavailable phrasing in both AnnounceSubtitleCommand and AnnounceCaptionCommand ("Subtitle is not available" → "No subtitle available"; "Caption is not available" → "No caption available") and adds the isAuthoredSubtitle/isAuthoredCaption guards. Neither command has any test coverage in the existing test suite or this PR. This is the most significant gap in test coverage.

Consider adding a test/command/announce-subtitle.test.ts and announce-caption.test.ts (or a combined announce-metadata.test.ts) mirroring the pattern from announce-title.test.ts.

2. figureTitle fallback returns DEFAULT_SUBPLOT_TITLE instead of DEFAULT_FIGURE_TITLE

// src/model/context.ts
public get figureTitle(): string {
  const figureState = this.figure.state;
  if (!figureState.empty) {
    return figureState.title;   // This is DEFAULT_FIGURE_TITLE when unset
  }
  return DEFAULT_SUBPLOT_TITLE; // 'unavailable' — used when figure has no state
}

The empty-state fallback returning DEFAULT_SUBPLOT_TITLE ('unavailable') rather than DEFAULT_FIGURE_TITLE ('MAIDR Plot') is intentional (the figure has no state at all), and isAuthoredTitle correctly rejects both. This is fine, but the inconsistency could trip up a future reader — a brief comment explaining why DEFAULT_SUBPLOT_TITLE is used for the empty-state fallback would help.

3. isAuthoredTitle does not guard against null / undefined

PlotState.title is typed as string, so under normal model flow null/undefined shouldn't reach here. However, the method signature accepts string, and .trim() on a non-string would throw. Since the mock tests pass a string explicitly, this is low risk, but worth noting if PlotState typing is ever relaxed. A defensive typeof title === 'string' guard or an explicit type assertion in the JSDoc is future-proofing you may or may not want.

4. isAuthoredSubtitle and isAuthoredCaption are functionally identical

DEFAULT_SUBTITLE and DEFAULT_CAPTION are both 'unavailable'. The three isAuthored* methods are intentionally symmetric for extensibility (if defaults ever diverge), but it's worth a comment to explain this isn't an oversight. The current docstring mentions "same caveats apply as isAuthoredTitle", which is good — just consider adding one line like: "Currently identical to isAuthoredCaption; kept separate so the two can diverge independently."

5. Dual-import source for DEFAULT_* constants in context.ts

import { DEFAULT_SUBPLOT_TITLE } from '@model/abstract';
import { DEFAULT_CAPTION, DEFAULT_FIGURE_TITLE, DEFAULT_SUBTITLE } from './plot';

These come from two different source files. If there is a chance to consolidate the default exports into a single constants module (or re-export DEFAULT_SUBPLOT_TITLE from plot.ts), that would reduce the cognitive overhead. Not a blocker, but a minor housekeeping note.

6. Multi-panel fallback label

When a multi-panel figure has no layer title but does have a figure title, announceTraceTitle falls back to the figure title labeled "Title" (not "Figure title"):

// announceTraceTitle fallback path
this.announce(figureTitle, 'Title');

In multi-panel context this might confuse users who expect the fallback to be labeled "Figure title". The behavior is documented in the JSDoc, but it's worth considering whether "Figure title" would be more accurate here. This is a design question, not a bug.

---

Minor Nits

• test/command/announce-title.test.ts line 40: createMockContext accepts figureTitle?: string and defaults to 'unavailable', but Context.figureTitle on the real object defaults to DEFAULT_SUBPLOT_TITLE which is also 'unavailable'. Consistent, but explicitly defaulting to DEFAULT_SUBPLOT_TITLE (imported) would make the intent clearer.
• The test for "falls back to authored layer title in single-panel figures" (line 66–91) doesn't verify announceUnavailable was NOT called. Worth adding expect(audioService.playWarningToneIfEnabled).not.toHaveBeenCalled() to keep assertions tight.

---

Summary

The fix is correct, well-documented, and architecturally sound. The main gaps are:

1. No tests for AnnounceSubtitleCommand / AnnounceCaptionCommand (the changed commands).
2. No unit tests for isAuthoredSubtitle / isAuthoredCaption themselves.

Everything else is polish. Approve after addressing #1, or at minimum adding a follow-up issue for subtitle/caption test coverage.

[claude]

Code Review – PR #616: fix: capture title from the json

Overview

This PR fixes a real accessibility regression: the l t shortcut (announce plot title) was announcing the model's internal placeholder defaults ("MAIDR Plot", "unavailable") instead of the title authored in the MAIDR JSON, and was also ignoring layer-level titles on single-panel plots. The fix is surgical and well-reasoned.

---

What Works Well

• Targeted fix: Changes are contained to the command, context, and description layers — no model-level restructuring needed.
• Centralization: Moving "is this an authored value?" checks into Context is a good design call. description.ts was already importing the constants directly; now a single method owns that logic, and the cleanup in description.ts shows the benefit immediately.
• Test coverage: 3 new test files cover all the meaningful scenarios — figure-level vs trace-level, single- vs multi-panel, verbose vs terse, authored vs placeholder, empty state, and the fallback chain. That's thorough.
• Documentation: The JSDoc comments explain the why (sentinel collision caveat, independence rationale for the separate subtitle/caption predicates), which is exactly the right level of detail.
• Phrasing improvement: "No title available" over "Title is not available" is more natural and consistent with the sibling subtitle/caption messages.

---

Issues and Suggestions

1. Known sentinel-string collision is worth flagging more prominently (minor)

The documented limitation — a user who authors a title of literally "MAIDR Plot" or "unavailable" will silently get "No title available" instead — is acceptable for now, but worth a code comment at the sentinel constant definitions (in plot.ts / abstract.ts) rather than only in the Context JSDoc. Future contributors who add or rename sentinels won't necessarily read the isAuthoredTitle docs.

Suggestion: Add a short comment at DEFAULT_FIGURE_TITLE and DEFAULT_SUBPLOT_TITLE in their source files noting they serve double duty as "no value authored" sentinels and must not collide with realistic authored values.

2. The figureTitle empty-state branch returning DEFAULT_SUBPLOT_TITLE is confusing (minor)

// context.ts
return DEFAULT_SUBPLOT_TITLE; // was: return 'unavailable'

The comment explains the rationale, but returning a subplot-level constant from a figure-level getter is surprising to a reader who is not familiar with both constants being 'unavailable'. Since both sentinels are rejected by isAuthoredTitle, a cleaner alternative would be a dedicated TITLE_UNAVAILABLE (or similar) constant shared by both levels, making the intent self-evident without the comment. That said, this is cosmetic — the behavior is correct and documented.

3. Missing terse-mode test for figure-level scope (minor gap)

The test file covers terse mode for trace-level scope but not for figure-level. Adding the terse-mode figure-level path would close the matrix. Not a blocker but easy to add.

4. context-authored.test.ts instantiates a real Context (style note)

const context = new Context(createStubFigure());

Context's constructor imports hotkeys-js and wires navigation. Using a real Context works here because the predicates are pure, but it couples the test to the constructor's side-effects. If those side effects ever grow, these tests could break unexpectedly. The announce-command tests correctly mock Context — the same approach would make context-authored.test.ts more resilient. Worth considering for consistency.

5. Architecture observation (non-blocking)

The isAuthored* predicates live on Context (navigation manager). They don't interact with navigation state; they're pure string validators that happen to know the model's sentinel constants. Placing them on Context is a pragmatic choice that avoids leaking constants to callers, and it's consistent with how figureTitle/figureSubtitle/figureCaption already aggregate model data for command consumers. The trade-off is that Context gains a second responsibility. This is acceptable given the alternative (having commands import model constants directly), but worth being aware of if Context accumulates more such utilities over time.

---

Summary

Category	Assessment
Correctness	Fixes the reported bug correctly
Architecture (MVVC)	Layers respected; no boundary violations
Code quality	Clean, readable, well-commented
Test coverage	Comprehensive; one minor gap (figure-level terse mode)
Performance	No concerns (pure string comparisons)
Security	No concerns
Breaking changes	None — placeholder defaults still flow through model for other paths

The PR is in good shape. Items 1 and 3 above are the most actionable improvements — everything else is cosmetic or forward-looking.