Wire assets/rereference web route and add assets/upload UX (#31)

[leogdion]

Summary

• Finishes the web-server wiring for assets/rereference left as a pending stub in fdcaef0 (issue Referencing Existing Assets (assets/rereference) #31). Adds a sibling POST /api/assets/upload route built on CloudKitService.uploadAssets so the demo can drive the full upload → rereference flow from the browser.
• Moves the asset UI out of the standalone Assets card and into the Notes panel: a Generate button stages a 96×96 PNG (rendered client-side via Canvas) that the save flow uploads as part of the record write; the rereference sub-section auto-fills the selected row's record name as the source.
• Existing notes with assets now preview their image via CloudKit's signed downloadURL.

Implementation notes

• WebRequests.Create gained an optional recordName so the browser can pass the upload receipt's record name through to records/create — this is how MistKit-mode binds the just-uploaded bytes to the new record.
• MistKit mode does the two-call dance (/api/assets/upload → /api/records/create|update); CloudKit JS mode passes the Blob inline to saveRecords and lets CK JS handle the upload.
• Server-side: thin WebBackend adapters forward to CloudKitService.uploadAssets and rereferenceAsset(fromRecord:field:toRecord:field:).

Test plan

• swift build clean in Examples/MistDemo
• swift test — 945 tests pass (added 4: forwards + 401 for each new endpoint)
• ./Scripts/lint.sh (both root and Examples/MistDemo/) — clean, no periphery warnings on the new code
• Manual web smoke test: swift run mistdemo web → sign in → set title → Generate → Create uploads + binds the PNG; selecting an existing note with an image renders its preview; Rereference copies the image to another note.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 260e44c0-9512-4696-a2ab-62ef10a1a14b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 31-assets-rereference

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 97.91667% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (v1.0.0-beta.2@cf3bb66). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...oudKitService/CloudKitError+ErrorDescription.swift	50.00%	1 Missing ⚠️
...erations/Operations.rereferenceAssets.Output.swift	85.71%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## v1.0.0-beta.2 #393 +/- ##
================================================
Coverage ? 70.96% ================================================
Files ? 168 Lines ? 3844 Branches ? 0 ================================================
Hits ? 2728 Misses ? 1116 Partials ? 0 

Flag	Coverage Δ
mistdemo-spm-macos	10.71% <1.04%> (?)
mistdemo-swift-6.2-jammy	10.72% <1.04%> (?)
mistdemo-swift-6.2-noble	10.72% <1.04%> (?)
mistdemo-swift-6.3-jammy	10.72% <1.04%> (?)
mistdemo-swift-6.3-noble	10.72% <1.04%> (?)
spm	71.68% <97.91%> (?)
swift-6.1-jammy	71.59% <97.91%> (?)
swift-6.1-noble	71.57% <97.91%> (?)
swift-6.2-jammy	71.62% <97.91%> (?)
swift-6.2-noble	71.59% <97.91%> (?)
swift-6.3-jammy	71.59% <97.91%> (?)
swift-6.3-noble	71.62% <97.91%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review

Critical

None found. The critical transport-separation invariant is fully upheld — webUploadAsset delegates through CloudKitService.uploadAssets → uploadAssetData → URLSession.shared, keeping the CDN upload on a separate connection pool from the CloudKit API transport.

---

Major

1. zoneID is silently dropped for lookupRecords / updateRecord inside rereferenceAsset

Sources/MistKit/CloudKitService/CloudKitService+AssetRereference.swift (around lines 128–145)

The zoneID parameter is correctly forwarded to rereferenceAssets but the subsequent lookupRecords and updateRecord calls don't accept a zone override, so they always target the default zone. For any record stored in a custom zone this will silently return NOT_FOUND on lookup and write to the wrong zone on update.

Suggested fix: Pass zoneID through to both operations, or — if the current API shape doesn't support it — add a clear // FIXME: #<issue> and return an error when zoneID is non-nil.

2. Implicit cross-file global dependency in JS: assetsSourceInput used in assets.js without a declaration

Resources/js/assets.js (line ~14) reads assetsSourceInput, which is declared in app.js (line ~50). This relies on <script> load order in index.html being preserved. If that order ever changes, a ReferenceError is thrown at click time. The existing globals (currentMode, currentDatabase) follow the same pattern, but assetsSourceInput is a new implicit dependency introduced by this PR and it's less obviously global by name.

Suggested fix: Either declare it with var assetsSourceInput = null; at the top of assets.js and set it from app.js, or hoist the shared state into a small state.js loaded first.

---

Minor

3. processRereferenceAssetsResponse covers fewer HTTP status cases than sibling processors

Sources/MistKit/CloudKitService/CloudKitResponseProcessor+Changes.swift (lines ~122–134)

The openapi.yaml for rereferenceAssets only declares 400 / 401, so the generated Output enum only exposes .badRequest, .unauthorized, and .undocumented. This means CloudKit's real 403 / 404 / 429 / 500 responses will all fall through as .undocumented. The .undocumented catch-all does return a CloudKitError, so it's not a failure, but it produces less actionable errors than other endpoints.

Suggested fix: Expand the openapi.yamlrereferenceAssets responses section to include at least 403, 404, and 429 (matching how other endpoints are documented) in a follow-up. Track as a GitHub issue.

4. Missing test coverage for rereferenceAsset failure paths

Tests/MistKitTests/CloudKitService/Rereference/ — success cases and compose helpers are covered. Missing:

• Per-item NOT_FOUND failure inside the lookup RecordResult (guard on line 132 fires).
• rereferenceAssets returning an empty assets array (guard on line 124 fires, throwing invalidResponse).
• Web endpoint receiving a malformed base64 data field.

5. AssetRereferenceResponse.assets not marked required in openapi.yaml

openapi.yaml (lines ~1646–1650)

The assets property is defined on AssetRereferenceResponse but the schema has no required list, making the generated type use an optional assets: [AssetValue]?. The code nil-coalesces with ?? [], which is correct, but marking it required would give a cleaner non-optional generated type and be more accurate (a successful response from CloudKit always includes this key).

---

Nit

6. CloudKitResponseProcessor+Changes.swift is growing beyond changes operations

The file now handles user discovery, asset upload, and rereference in addition to record/zone changes — seven processors total. Consider splitting asset and user processors into their own files in a follow-up to keep the naming honest.

7. Pre-existing XSS note (not introduced here)

auth.js (pre-existing) uses innerHTML with the raw web-auth token. CloudKit tokens are opaque strings unlikely to contain HTML, but textContent would be safer. Out of scope for this PR.

---

Summary

The transport safety architecture is correct and the import/ACL conventions are followed throughout all new files. The JS XSS surface for new code is clean (setStatus uses textContent; JSON output goes via textContent on a <pre>). PNGEncoder is a clean pure-Swift/Foundation implementation with no platform guard needed.

The main substantive concern is Major #1 — zoneID dropping on the lookupRecords/updateRecord leg of rereferenceAsset will silently break non-default-zone callers. That should be addressed before merging or explicitly documented with a blocking issue reference if it's an intended scope cut.

🤖 Generated with Claude Code

[claude]

Code Review — PR #393: Wire assets/rereference + assets/upload UX

This is a well-structured PR that completes a long-standing stub. The overall architecture is sound and follows established MistKit patterns. Below are my findings.

---

Overview

The PR implements two new CloudKit operations:

1. assets/rereference — reuses an uploaded asset descriptor across records (avoiding a redundant CDN upload)
2. assets/upload — new web route + browser UX (canvas PNG → base64 → server → CloudKit CDN)

The three-call composition in rereferenceAsset (rereference → lookup → modify) is unavoidable given CloudKit's API surface and is clearly structured.

---

Issues

Error specificity (moderate)

CloudKitService+AssetRereference.swift uses CloudKitError.invalidResponse for three distinct failure cases with no additional context:

guardlet asset = descriptors.first else{throwCloudKitError.invalidResponse // no context
}guardlet targetInfo =try lookups.first?.get()else{throwCloudKitError.invalidResponse // ambiguous — first nil, or .get() failed?
}guardlet recordType = targetInfo.recordType else{throwCloudKitError.invalidResponse // which field?
}

The middle guard is particularly tricky: lookups.first?.get() will silently produce nil (matching the guard) even when lookups is empty, making it impossible at the call site to distinguish "target record not found" from "lookup returned an empty list". Each guard should carry a descriptive reason — or use a new CloudKitError case — so callers can surface a useful message.

Missing error-path tests (moderate)

CloudKitServiceTests.Rereference.SuccessCases covers the happy path and a top-level HTTP 400. What's not covered:

• Source record exists but has no asset on the requested field → descriptors.first is nil
• Target record doesn't exist → lookups.first is nil
• The rereferenceAsset three-call convenience method itself (only the primitives are tested)

JavaScript memory duplication (minor)

generateNoteImage (app.js ~line 962) returns and stores three representations simultaneously:

return{ dataURL, base64, blob,byteLength: bytes.length};

dataURL and base64 are both large base64 strings — dataURL is strictly "data:image/png;base64," + base64, so one is redundant. For a ~10 KB test image this is harmless, but the duplication is unnecessary.

No PNG validation test (minor)

PNGData.generate produces a hand-rolled DEFLATE/zlib + CRC-32 + Adler-32 PNG. The implementation looks correct, but there's no unit test that decodes the output with a real PNG parser to verify the bytes are actually a valid image. A single test that attempts CGImageSource (macOS) or a pure-Swift PNG check would catch any future regressions in the encoder.

Client-side record-name validation (minor)

assetsSourceInput.value.trim() and assetsTargetInput.value.trim() only check for empty. CloudKit will reject malformed record names with a generic server error. An early client-side regex check (e.g. ^[A-Za-z0-9._-]+$) would give users immediate feedback.

---

Positives

• Auth enforcement: both new routes return 401 before touching the backend, and there are dedicated @Tests confirming this (e.g. assetsUploadRequiresAuth, assetsRereferenceRequiresAuth).
• Correct async/await usage: no fire-and-forget, no force-unwrapped continuations, async let not misused.
• Cryptographic correctness: the hand-rolled CRC-32 (polynomial 0xEDB8_8320) and Adler-32 (modulus 65_521) in PNGData.swift are standard-spec values.
• Capacity reservation: raw.reserveCapacity(height * (1 + width * 3)) avoids repeated reallocation in the PNG encoder.
• Self-rereference guard: if (source === target) in assets.js prevents a pointless API call with a clear user message.
• Import conventions: all new files follow internal import / public import with explicit access modifiers as required.
• MockBackend refactor: splitting MockBackend into extension files (+RecordOperations, +ServiceOperations) improves maintainability without changing behavior.

---

Nits

• WebRequests+Assets.swift: fieldName and targetFieldName accept any String. A doc comment noting CloudKit's field name rules (alphanumeric + underscore, no spaces, ≤255 chars) would help future callers.
• The pending-stub removal in WebServer+Pending.swift is clean — good housekeeping.

---

Test plan gap

The one unchecked box — manual web smoke test — is understandable for a draft PR, but the three-call round-trip in rereferenceAsset is the riskiest path (three sequential async failures → three distinct error surfaces). I'd recommend adding at least one unit test that exercises the full composition with a mock that fails at each of the three calls.

---

Overall: solid implementation that completes a long-standing stub. The main ask before merge is improving the error messages in CloudKitService+AssetRereference.swift and adding unit tests for the error paths in the composition method. The rest are minor polish items.

🤖 Generated with Claude Code

[claude]

Code Review — PR #393: Wire assets/rereference web route and add assets/upload UX

Overall: This is a solid feature implementation that completes the assets/rereference stub and adds a full upload → rereference UX flow. The architecture follows existing patterns well. A few items worth addressing before merge.

---

Findings

1. MockBackend — private(set) removed from all stored properties

File: Examples/MistDemo/Tests/MistDemoTests/Server/MockBackend.swift

All recorded-call properties changed from internal private(set) var to plain internal var. The PR comment explains why (extensions in separate files can't satisfy private(set)), but this means any caller inside the module can now write directly to e.g. lastQuery without going through an actor-serialized method, bypassing the intended record-then-read invariant.

Consider an alternative: keep private(set) and make the extension bodies func calls that live in the main file, delegating to private setters. Or document clearly that the actor serialization is the only safety net here and internal var is deliberate.

---

2. rereferenceAsset — lookupRecords call always made even when caller has the info

File: Sources/MistKit/CloudKitService/CloudKitService+AssetRereference.swift, line ~107

letlookups=tryawaitlookupRecords(
recordNames:[targetRecordName],
database: database
)guardlet targetInfo =try lookups.first?.get()else{throwCloudKitError.invalidResponse
}guardlet recordType = targetInfo.recordType else{throwCloudKitError.invalidResponse
}

The lookupRecords call is needed to get the target's recordType and recordChangeTag. However, if a caller already has both, there's no overload that avoids the extra round trip. Since the updateRecord path needs a recordChangeTag to avoid conflicts, this is necessary for correctness — but it's worth considering an overload:

publicfunc rereferenceAsset(
fromRecord sourceRecordName:String,
field assetField:String,
toRecord targetRecordName:String,
recordType:String,
recordChangeTag:String?,
field targetField:String?=nil,
zoneID:ZoneID?=nil,
database:Database)asyncthrows(CloudKitError)->RecordInfo

Not a blocker, but a useful escape hatch for performance-sensitive callers.

---

3. rereferenceAsset — lookups.first?.get() error swallowed by invalidResponse

File: Sources/MistKit/CloudKitService/CloudKitService+AssetRereference.swift, line ~115

guardlet targetInfo =try lookups.first?.get()else{throwCloudKitError.invalidResponse
}

lookups.first?.get() can throw a CloudKitError from a per-record failure (e.g. the target record doesn't exist). In that case try rethrows the error correctly — but if the array is empty (no records returned), the guard catches that via nil and throws the generic .invalidResponse rather than something caller-actionable. The doc comment says "throws CloudKitError.invalidResponse" but the more common path (bad record name → per-record error) actually throws a different case. Minor doc/error-message mismatch but could confuse debugging.

---

4. RereferenceAssetPhase.verify — OR logic over unstable downloadURL

File: Examples/MistDemo/Sources/MistDemoKit/Integration/Phases/RereferenceAssetPhase.swift, line ~50

letchecksumMatches= expected.fileChecksum !=nil && targetAsset.fileChecksum == expected.fileChecksum
letdownloadURLMatches= expected.downloadURL !=nil && targetAsset.downloadURL == expected.downloadURL
guard checksumMatches || downloadURLMatches else{...}

CloudKit signed CDN URLs (cvws.icloud-content.com) have expiry embedded in them and are not stable across calls. If fileChecksum is nil on the receipt but downloadURL happens to match (or an older URL is reused by coincidence), the verify would pass falsely.

Recommend requiring fileChecksum when available, and treating downloadURL only as a fallback diagnostic rather than a primary verification signal:

iflet expected = expected.fileChecksum {guard targetAsset.fileChecksum == expected else{throwIntegrationTestError.verificationFailed(...)}}elseiflet expectedURL = expected.downloadURL {guard targetAsset.downloadURL == expectedURL else{throwIntegrationTestError.verificationFailed(...)}}

---

5. RereferenceAssetCommand.execute() — success printed twice

File: Examples/MistDemo/Sources/MistDemoKit/Commands/RereferenceAssetCommand.swift, line ~97 and ~102

print("\n✅ Re-referenced asset onto \(record.recordName)")tryawaitoutputResult(record, format: config.output)
// ...
print("✅ Re-reference completed!")

Two success lines are printed. One of them can be removed.

---

6. processRereferenceAssetsResponse — over-documented per CLAUDE.md

File: Sources/MistKit/CloudKitService/CloudKitResponseProcessor+Changes.swift

/// Process rereferenceAssets response
/// - Parameter response: The response to process
/// - Returns: The extracted asset re-reference response data
/// - Throws: CloudKitError for various error conditions

Per project conventions: "Don't explain WHAT the code does, since well-named identifiers already do that." This is a what comment on a private function — remove it to match the pattern used in other processX functions in the same file.

---

7. assetsSourceInput — implicit cross-file JS global dependency

File: Examples/MistDemo/Sources/MistDemoKit/Resources/js/assets.js, line ~28

constsource=assetsSourceInput.value.trim();

assetsSourceInput is declared in app.js and relied upon in assets.js via shared global scope. This works because app.js is loaded first, but it's a hidden coupling. If scripts are ever modularized, this silently breaks. Consider accepting this as a demo constraint and adding a comment, or re-binding locally: const source = document.getElementById('assets-source')?.value.trim() ?? '';.

---

8. Missing error-path unit tests for rereferenceAsset compose

Files: Tests/MistKitTests/CloudKitService/Rereference/CloudKitServiceTests.Rereference+Compose.swift

The compose test covers the happy path only. Three additional cases would complete coverage for the guard chains:

• rereferenceAssets returns an empty assets array → .invalidResponse
• lookupRecords returns an empty array → .invalidResponse
• targetInfo.recordType is nil → .invalidResponse
• targetField defaults to assetField when nil (verify the field name written)

Not a blocker for this PR but worth a follow-up issue.

---

What works well

• Transport separation maintained: webUploadAsset correctly delegates to CloudKitService.uploadAssets which uses URLSession.shared separately from the API transport — the critical invariant from CLAUDE.md is preserved.
• Import modifiers: All new files use explicit internal import / public import — no bare imports.
• Test structure: New tests use @Test, #expect, #require, and async correctly. The makeService(responsesByOperation:) pattern with fallback-to-empty is clean.
• OpenAPI-first: The openapi.yaml change drives regeneration in MistKitOpenAPI/ — correct workflow.
• OperationInputPath extension: Operations.rereferenceAssets.Input.Path: OperationInputPath follows the exact pattern of sibling extensions.
• UX: The assetsSourceInput auto-fill on row click (loadNoteIntoForm) is a nice touch that makes the rereference flow discoverable.

[claude]

Code Review — PR #393: Wire assets/rereference + assets/upload UX

Overview

This PR completes the assets/rereference story end-to-end: OpenAPI spec, generated client, CloudKitService wrapper, MistDemo web routes, CLI command, integration phase, and a solid cross-platform PNG encoder. The architecture is clean and follows established project patterns well. A few items worth addressing before merge:

---

Potential Bugs / Issues

1. processRereferenceAssetsResponse is unnecessarily async (CloudKitResponseProcessor+Changes.swift)

The function only pattern-matches on the response enum — no await points — but is declared async throws(CloudKitError). This adds a suspend overhead on every call. The analogous processUploadAssetsResponse and others are async only when they collect a streaming body; this one doesn't. Consider making it a plain throws(CloudKitError) function.

// current
internalfunc processRereferenceAssetsResponse(
_ response:Operations.rereferenceAssets.Output)asyncthrows(CloudKitError)->Components.Schemas.AssetRereferenceResponse{

2. Stale type reference in PNGData doc comment (Integration/PNGData.swift:458–462)

/// Delegates the byte-level encoding to ``PNGEncoder``, which produces a

The type is PNGData, not PNGEncoder. The PNGEncoder name doesn't exist anywhere in the codebase — this is a stale draft name that slipped through.

3. OpenAPI $ref + sibling description on zoneID (openapi.yaml:3938–3940)

zoneID:
$ref: '#/components/schemas/ZoneID'description: Optional zone ID. Defaults to default zone if not specified.

In OpenAPI 3.0, a $ref replaces the containing object — any sibling properties (including description) are silently ignored by conformant parsers. This is the same pattern used elsewhere in the existing spec, so not a regression, but worth tracking as a known spec limitation.

4. Missing 401 coverage for the uploadedRecordName binding in the JS upload flow (app.js:1082–1094)

If /api/assets/upload returns a non-2xx error, postJSON throws, which is caught by the outer catch. But the uploadedRecordName capture could theoretically leak if the try at line 1084 throws after partial assignment. This is structurally fine in JS (the outer catch cleans up), but the variable is let-declared in the outer scope:

letuploadedRecordName=null;if(hasPendingImage&&currentMode==='mistkit'){constreceipt=awaitpostJSON('/api/assets/upload',{ … });uploadedRecordName=receipt.recordName;// only set on successfields.image=receipt.asset;}

Actually, this is fine as written — on throw the outer catch fires and uploadedRecordName stays null. No bug, but worth a comment that the dangling state is intentional.

---

Code Quality

5. MockBackend access control downgrade (MockBackend.swift:2069–2081)

All stored properties changed from internal private(set) var to internal var to support extension-based conformance. The comment explains why, which is good. However, this means tests can now write to lastQuery, lastCreate, etc. directly — not just read them. If an accidental write in a future test silently hides a call-count bug, it'll be hard to catch. Consider whether the extension pattern could be refactored to avoid the downgrade, e.g. by keeping the actor's stored properties writable only through a package-private record_*(…) method rather than direct assignment.

6. MistDemoConfig.recordType = "Note" placement (MistDemoConfig.swift:163)

Moving IntegrationTestData.recordType into MistDemoConfig consolidates the constant, but MistDemoConfig is the runtime configuration type — putting a schema-coupling constant ("Note") there mixes concerns. IntegrationTestData was the cleaner home since it was explicitly for tests. A middle ground: a dedicated IntegrationTestConstants (or even staying in the now-deleted IntegrationTestData enum) would keep schema details out of the production config struct.

7. Emoji print statements in RereferenceAssetCommand.execute() (RereferenceAssetCommand.swift:57–79)

Using print with emoji directly in CLI output is consistent with other commands (e.g., UploadAssetCommand), so no issue there. Just noting the pattern for awareness.

---

Security Considerations

8. No upload size limit on POST /api/assets/upload

The route decodes the entire request body into memory before forwarding to CloudKit:

letbody=tryawait request.decode(as:WebRequests.UploadAsset.self, context: context)

The base64-encoded data field has no maximum size check. For a local demo server this is acceptable, but worth documenting explicitly that this route is not hardened for public exposure. Hummingbird's maxRequestBodySize could be set if this becomes production-adjacent.

---

Positives

• Three-phase composition documented clearly — rereferenceAssets → lookupRecords → modifyRecords with the recordType-aware overload cleanly skipping the middle step.
• PNGData is excellent — a proper cross-platform PNG encoder with correct CRC-32, Adler-32, and uncompressed DEFLATE blocks, replacing the malformed padding approach. Produces images that actually decode.
• incompleteResponse error case is a clean addition; the doc comment and HandlingErrors.md update keep the error taxonomy consistent.
• Test organisation — splitting MockBackend across +RecordOperations / +ServiceOperations files is much more navigable than the monolith it replaced.
• Integration phase cleanup — RereferenceAssetPhase properly returns the new targetRecordName in CreatedRecordNames so cleanup removes it alongside the other test records. Easy to miss; good catch.
• Asset(from:) centralisation (Asset+Components.swift) — the inline duplicate in FieldValue+Components.swift is gone cleanly.

---

Test Coverage

• ✅ WebServerTests+Assets — 401 and forwarding coverage for both new routes.
• ✅ Rereference+SuccessCases — descriptor mapping and top-level BAD_REQUEST path.
• ✅ Rereference+Compose — happy path and overload-skips-lookup assertion.
• ✅ Rereference+ComposeErrors — three incompleteResponse branches and target-field defaulting.
• ⚠️ Missing: a test verifying that POST /api/assets/upload with recordName present correctly forwards it to webUploadAsset (the existing assetsUploadForwards test omits recordName — only the nil/absent case is covered).

---

Overall this is solid work with good test coverage. Items 1–3 are the most actionable before merge; items 5–6 are worth a follow-up issue if not addressed now.