-
Notifications
You must be signed in to change notification settings - Fork 19
Link language debugging #611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughThis change introduces a comprehensive debug string infrastructure for the link synchronization system. It adds the ability to generate, emit, collect, store, and visualize debug strings—especially DOT graph representations—throughout the stack, from Rust and TypeScript backends to the UI. The update includes new APIs, observer hooks, GraphQL queries, runtime service extensions, and integration tests to ensure debug information is accessible and useful for diagnosis. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (DebugStrings)
participant RuntimeClient
participant RuntimeResolver
participant Rust RuntimeService
User->>UI (DebugStrings): Open Debug Strings modal
UI (DebugStrings)->>RuntimeClient: debugStrings(languageAddress)
RuntimeClient->>RuntimeResolver: GraphQL: runtimeDebugStrings(languageAddress)
RuntimeResolver->>Rust RuntimeService: get_debug_strings(languageAddress)
Rust RuntimeService-->>RuntimeResolver: [DebugStringEntry...]
RuntimeResolver-->>RuntimeClient: [DebugStringEntry...]
RuntimeClient-->>UI (DebugStrings): [DebugStringEntry...]
UI (DebugStrings)-->>User: Render debug strings (text/graph)
sequenceDiagram
participant LinkSyncAdapter
participant Workspace
participant RuntimeService
Note over LinkSyncAdapter: During link sync operations
LinkSyncAdapter->>Workspace: generate_debug_graph() / emit_debug_graph()
Workspace->>LinkSyncAdapter: Emit debug signal (type: "debug_string")
LinkSyncAdapter->>RuntimeService: addDebugString(languageAddress, debugString, operation)
RuntimeService-->>LinkSyncAdapter: Ack
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/pull.rs (2)
82-86
: Consider error handling for debug signal emission.The debug signal emission could potentially fail and should not affect the main operation flow. Consider using a non-blocking approach or at least logging any emission failures.
- emit_signal(serde_json::json!({ - "type": "debug_string", - "operation": "pull-info", - "debug_string": "No current revision, collecting from latest", - }))?; + if let Err(e) = emit_signal(serde_json::json!({ + "type": "debug_string", + "operation": "pull-info", + "debug_string": "No current revision, collecting from latest", + })) { + debug!("Failed to emit debug signal: {:?}", e); + }
163-167
: Ensure consistent debug signal format across all emissions.The debug signals use different string formatting approaches. Consider standardizing the format for consistency and easier parsing.
All debug signals should use a consistent format. Consider creating a helper function to standardize debug signal emission:
+fn emit_debug_signal(operation: &str, message: &str) -> SocialContextResult<()> { + emit_signal(serde_json::json!({ + "type": "debug_string", + "operation": operation, + "debug_string": message, + })) +}Then use it consistently:
- emit_signal(serde_json::json!({ - "type": "debug_string", - "operation": "pull-info", - "debug_string": format!("Fast-forwarding from {} to {}", current.hash, theirs), - }))?; + emit_debug_signal("pull-info", &format!("Fast-forwarding from {} to {}", current.hash, theirs))?;Also applies to: 185-189
bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/workspace.rs (1)
874-882
: Well-structured debug signal emission.The method properly structures the JSON signal with the required fields and handles the emit_signal result appropriately. Consider the performance impact for very large graphs, as DOT generation could be expensive.
For very large graphs, consider adding a size check or graph simplification to avoid performance issues:
pub fn emit_debug_graph(&self, operation: &str) -> SocialContextResult<()> { + // Consider limiting debug output for very large graphs + if self.graph.node_count() > 1000 { + emit_signal(serde_json::json!({ + "type": "debug_string", + "operation": operation, + "debug_string": format!("Large graph with {} nodes - debug output truncated", self.graph.node_count()), + }))?; + return Ok(()); + } + emit_signal(serde_json::json!({ "type": "debug_string", "operation": operation, "debug_string": self.generate_debug_graph(), }))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
bootstrap-languages/p-diff-sync/hc-dna/Cargo.lock
is excluded by!**/*.lock
deno.lock
is excluded by!**/*.lock
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
tests/js/languages/note-store/deno.lock
is excluded by!**/*.lock
tests/js/languages/test-language/deno.lock
is excluded by!**/*.lock
📒 Files selected for processing (23)
bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/Cargo.toml
(1 hunks)bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/pull.rs
(4 hunks)bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/workspace.rs
(1 hunks)bootstrap-languages/p-diff-sync/index.ts
(1 hunks)bootstrap-languages/p-diff-sync/linksAdapter.ts
(4 hunks)cli/mainnet_seed.json
(1 hunks)core/src/language/Language.ts
(2 hunks)core/src/perspectives/PerspectiveHandle.ts
(1 hunks)core/src/runtime/RuntimeClient.ts
(1 hunks)core/src/runtime/RuntimeResolver.ts
(2 hunks)executor/src/core/LanguageController.ts
(1 hunks)executor/src/runtime_service_extension.d.ts
(1 hunks)rust-executor/src/graphql/graphql_types.rs
(2 hunks)rust-executor/src/graphql/query_resolvers.rs
(1 hunks)rust-executor/src/mainnet_seed.json
(1 hunks)rust-executor/src/runtime_service/mod.rs
(3 hunks)rust-executor/src/runtime_service/runtime_service_extension.js
(2 hunks)rust-executor/src/runtime_service/runtime_service_extension.rs
(2 hunks)tests/js/tests/neighbourhood.ts
(1 hunks)ui/package.json
(1 hunks)ui/src/components/DebugStrings.tsx
(1 hunks)ui/src/components/Language.tsx
(3 hunks)ui/src/components/Perspectives.tsx
(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/components/DebugStrings.tsx
[error] 112-112: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (41)
rust-executor/src/mainnet_seed.json (1)
7-7
: Align knownLinkLanguages with updated language hash
The new hash matches the version used incli/mainnet_seed.json
, ensuring both runtime and CLI seeds are consistent.ui/package.json (1)
23-23
: Add graph visualization dependency
Introducing@viz-js/viz
at^3.14.0
correctly supports DOT rendering inDebugStrings.tsx
. Ensure your lockfile (package-lock.json
/pnpm-lock.yaml
/yarn.lock
) is updated to include this dependency.bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/Cargo.toml (1)
14-14
: Verify necessity ofserde_json
dependency
Confirm thatserde_json
is used in the crate’s code (e.g., inpull.rs
orworkspace.rs
) for serializing debug strings. If unused, remove it to keep dependencies minimal.cli/mainnet_seed.json (1)
7-7
: Sync CLI seed with runtime seed update
The knownLinkLanguages entry now matches the updated hash inrust-executor/src/mainnet_seed.json
, ensuring consistency across environments.core/src/perspectives/PerspectiveHandle.ts (1)
5-10
: Confirm enum string values align with GraphQL schema
The enum variants are now PascalCase string literals; verify that the GraphQL layer expects these exact names (camelCase in schema) or adjust accordingly in yourtype-graphql
config to avoid mismatches.bootstrap-languages/p-diff-sync/index.ts (1)
36-36
: LGTM! Clean integration of debug string signal handling.The conditional logic properly extends the existing pattern to handle debug string signals alongside the existing reference and diff signals.
executor/src/core/LanguageController.ts (1)
282-288
: ```shell
#!/bin/bashShow the full reloadLanguage method to confirm the absence of addDebugStringCallback
rg -A 60 "async reloadLanguage" --type ts executor/src/core/LanguageController.ts
</details> <details> <summary>core/src/runtime/RuntimeClient.ts (1)</summary> `204-217`: **Well-implemented GraphQL client method.** The `debugStrings` method follows the established patterns in this class with proper GraphQL query structure, error handling via `unwrapApolloResult`, and consistent parameter handling. </details> <details> <summary>ui/src/components/Perspectives.tsx (1)</summary> `299-320`: **Debug UI integration looks good once hooks issue is resolved.** The debug button is properly scoped to neighbourhoods only, and the conditional rendering of the DebugStrings component follows good practices. The integration will work well once the state management is moved to the component level. </details> <details> <summary>core/src/language/Language.ts (2)</summary> `151-153`: **LGTM! Clean type definitions.** The new `DebugStringObserver` type follows the same pattern as the existing observer types and provides clear parameter typing. --- `192-193`: **LGTM! Proper optional method design.** The optional `addDebugStringCallback` method is well-designed and maintains backward compatibility while extending the LinkSyncAdapter interface appropriately. </details> <details> <summary>rust-executor/src/graphql/graphql_types.rs (2)</summary> `460-460`: **Good addition for GraphQL naming consistency.** Adding `#[graphql(rename_all = "camelCase")]` to `PerspectiveState` improves consistency with GraphQL naming conventions. --- `1078-1087`: **Well-structured GraphQL type definition.** The `DebugStringEntry` struct is properly annotated with both GraphQL and Serde attributes, following established patterns in the codebase. The explicit field renaming ensures consistent camelCase naming in the GraphQL schema. </details> <details> <summary>ui/src/components/Language.tsx (1)</summary> `165-175`: **LGTM! Well-structured debug button UI.** The debug button implementation follows the existing UI patterns with proper icon usage and styling. </details> <details> <summary>rust-executor/src/graphql/query_resolvers.rs (1)</summary> `464-485`: **Well-implemented GraphQL query resolver.** The `runtime_debug_strings` resolver follows proper patterns: - Appropriate capability checking - Clean mapping from internal types to GraphQL types - RFC 3339 timestamp formatting is standard-compliant - Optional filtering by language address provides flexibility </details> <details> <summary>rust-executor/src/runtime_service/runtime_service_extension.js (2)</summary> `2-3`: **LGTM! Clean import additions.** The new debug string operations are properly imported and follow the existing pattern. --- `16-21`: **Well-structured method additions.** The new async methods follow the same pattern as existing runtime service methods, with proper parameter forwarding to the underlying ops. </details> <details> <summary>core/src/runtime/RuntimeResolver.ts (2)</summary> `141-154`: **Properly structured GraphQL type definition.** The `DebugStringEntry` type is well-defined with appropriate field decorators and covers all necessary properties for debug string entries. --- `257-266`: **Appropriate mock implementation for test resolver.** The hardcoded mock data is suitable for testing purposes and demonstrates the expected structure of debug string entries. The optional parameter handling is correct. </details> <details> <summary>rust-executor/src/runtime_service/runtime_service_extension.rs (4)</summary> `1-1`: **Good addition to imports.** The `DebugStringEntry` import is properly added to support the new operations. --- `33-41`: **Well-implemented fast op for adding debug strings.** The `add_debug_string` operation is correctly marked as fast since it's a simple storage operation. Parameter handling and error handling are appropriate. --- `43-49`: **Properly structured serde op for retrieving debug strings.** The `get_debug_strings` operation correctly uses serde for returning structured data and handles the optional language address parameter appropriately. --- `53-53`: **Correct extension registration.** The new operations are properly added to the extension's ops list. </details> <details> <summary>executor/src/runtime_service_extension.d.ts (2)</summary> `6-7`: **Accurate method signatures for debug string operations.** The new methods are properly typed with correct parameter types and return types that match the implementation. --- `10-15`: **Well-defined interface structure.** The `DebugStringEntry` interface correctly uses snake_case naming to match the Rust data structures and includes all necessary fields. </details> <details> <summary>bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/workspace.rs (1)</summary> `861-872`: **Clean implementation of debug graph generation.** The method efficiently reuses existing graph mapping logic and properly configures DOT output with edge labels omitted for cleaner visualization. </details> <details> <summary>bootstrap-languages/p-diff-sync/linksAdapter.ts (4)</summary> `2-3`: **LGTM! Proper import of debug-related types.** The import statements correctly include the new debug types (`DebugGraphObserver`, `DebugStringObserver`) needed for the debug string functionality. --- `19-19`: **LGTM! Consistent callback pattern implementation.** The `debugStringCallback` property and `addDebugStringCallback` method follow the same pattern as existing callback implementations (`linkCallback`, `syncStateChangeCallback`), maintaining consistency in the codebase. Also applies to: 254-257 --- `186-186`: **Good enhancement for error debugging.** Adding `e.stack` and `JSON.stringify(e)` to the error logging provides more comprehensive error information, which will be valuable for debugging gossip-related issues. --- `260-271`: **Well-implemented debug signal handling.** The debug signal handling logic is correctly implemented with: - Early return to prevent interference with existing signal processing - Proper null checking before calling `debugStringCallback` - Defensive programming with the conditional check for required fields before processing regular signals - Good logging for debugging purposes The implementation maintains backward compatibility while adding the new debug functionality. </details> <details> <summary>ui/src/components/DebugStrings.tsx (6)</summary> `5-21`: **LGTM! Well-structured component interfaces.** The TypeScript interfaces are comprehensive and properly typed. The component props include all necessary fields for functionality and callbacks. --- `27-51`: **Excellent data fetching implementation.** The `fetchDebugStrings` function properly handles: - Loading and error states - Async operation with try-catch - Sorting by timestamp (latest first) for better UX - The useEffect dependency array is correct and complete --- `57-115`: **Well-implemented DOT graph visualization.** The DOT graph detection and rendering logic is robust: - Simple but effective detection using `digraph` keyword - Proper async handling of the viz.js library - Good error handling with fallback display - Clean component structure with loading states --- `112-112`: **Verify security implications of SVG rendering.** While using `dangerouslySetInnerHTML` is necessary here to render the SVG generated by `@viz-js/viz`, it's worth verifying the security implications since debug strings could potentially contain malicious content. The risk is mitigated by: - Debug strings originate from the language system, not direct user input - The `@viz-js/viz` library should sanitize DOT input during SVG generation Consider validating that debug strings only come from trusted sources or implementing additional sanitization if there are concerns about the source of debug strings. --- `119-158`: **Excellent UI design and user experience.** The modal implementation provides: - Fullscreen layout appropriate for debugging information - Refresh functionality with loading states - Clear messaging when no debug strings are available - Good responsive design with scrollable content --- `167-247`: **Comprehensive and user-friendly debug entry display.** The debug string entry rendering provides: - Clear visual distinction between operations and timestamps - Intelligent conditional rendering for DOT graphs vs text - Collapsible raw syntax view for graphs - Excellent styling with dark theme for code readability - Proper text wrapping and scrolling for long content </details> <details> <summary>tests/js/tests/neighbourhood.ts (1)</summary> `225-331`: **Comprehensive integration test for debug string functionality.** This test provides excellent coverage of the debug string feature: **Test Flow:** - Creates neighbourhood between Alice and Bob - Performs bidirectional link operations - Validates debug string generation on both clients **Verification Points:** - Debug strings are generated during link synchronization - Expected operations (merge, pull, commit, pull-info) are present - Timestamps and language addresses are valid - Proper filtering by language address **Good Practices:** - Comprehensive logging for debugging test issues - Proper waiting/retry logic for async operations - Validation of both structure and content - Testing from both client perspectives The test effectively validates the end-to-end debug string infrastructure. </details> <details> <summary>rust-executor/src/runtime_service/mod.rs (4)</summary> `4-14`: **Well-designed DebugStringEntry struct.** The struct definition includes all necessary fields: - Language address for filtering - Debug string content - Operation type for categorization - UTC timestamp for ordering The serde derives and DateTime<Utc> usage are appropriate choices for serialization and time handling. --- `42-46`: **Excellent thread-safe storage implementation.** The global storage design is well-architected: - `VecDeque` provides efficient FIFO operations for capacity management - `Arc<Mutex<>>` ensures thread safety across the runtime - Reasonable capacity limit (100) prevents memory issues - `lazy_static` provides safe global initialization --- `189-217`: **Robust debug string management implementation.** Both methods are well-implemented: **`add_debug_string`:** - Automatic UTC timestamp generation - Thread-safe storage access - FIFO capacity management prevents memory growth **`get_debug_strings`:** - Optional language filtering for targeted debugging - Thread-safe read access - Efficient filtering with iterator chains The implementation correctly handles concurrent access and provides useful functionality for debug string management. --- `220-343`: **Exceptionally comprehensive unit test coverage.** The tests provide excellent coverage: **Test Cases:** - Basic add/get functionality - Language address filtering - Capacity limit enforcement (FIFO eviction) - Entry field validation and timestamp bounds **Good Testing Practices:** - Test mutex prevents race conditions - Clean setup/teardown with debug string clearing - Edge case testing (non-existent addresses, capacity overflow) - Timestamp validation within reasonable bounds The tests give high confidence in the implementation's correctness and robustness. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/src/components/DebugStrings.tsx (1)
54-83
: Consider using modal's built-in close events instead of MutationObserver.The MutationObserver approach works but adds complexity. Consider if the custom modal component provides built-in close event handlers that would be more straightforward to use.
If the j-modal component supports onClose or similar events, this would be cleaner:
- // Watch for modal close via its built-in close button - useEffect(() => { - const modalElement = modalRef.current; - if (!modalElement) return; - - const handleModalChange = () => { - // If modal internally set open to false, call our onClose - if (open && !modalElement.open) { - onClose(); - } - }; - - // Listen for changes to the open attribute - const observer = new MutationObserver((mutations) => { - mutations.forEach((mutation) => { - if (mutation.type === 'attributes' && mutation.attributeName === 'open') { - handleModalChange(); - } - }); - }); - - observer.observe(modalElement, { - attributes: true, - attributeFilter: ['open'] - }); - - return () => { - observer.disconnect(); - }; - }, [open, onClose]); + // If j-modal supports built-in close events, use those instead
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
turbo.json
(1 hunks)ui/src/components/DebugStrings.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- turbo.json
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/components/DebugStrings.tsx
[error] 144-144: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (9)
ui/src/components/DebugStrings.tsx (9)
1-3
: LGTM! Clean and focused imports.The imports are well-organized and include only what's needed. The @viz-js/viz dependency aligns with the DOT graph visualization requirements mentioned in the PR objectives.
5-16
: Well-defined TypeScript interfaces.The interfaces are properly typed and provide clear contracts for the component's data structures and props.
28-46
: Robust error handling in fetch function.The async fetch function properly handles loading states, error cases, and includes sorting logic for timestamps. The error handling catches both Error instances and generic failures.
48-52
: Efficient data fetching with proper dependencies.The useEffect correctly triggers data fetching only when the modal is open and dependencies change, implementing lazy loading as intended.
89-93
: Simple but effective DOT graph detection.The detection logic is straightforward and should work for typical DOT graphs. Consider adding more robust validation if needed in the future.
95-104
: Good error handling in DOT graph rendering.The async function properly handles rendering failures and logs errors appropriately.
192-270
: Well-structured rendering logic with good UX considerations.The rendering logic properly handles different states (loading, error, empty, content) and provides good visual hierarchy. The scrollable container with max-height prevents UI overflow issues.
253-265
: Excellent text overflow handling for debug strings.The pre-formatted text styling with
word-break: break-word
andwhite-space: pre-wrap
ensures long debug strings display properly without breaking the layout.
152-152
: Verify modal accessibility and keyboard navigation.Ensure the fullscreen modal provides proper focus management and keyboard navigation support for accessibility compliance.
#!/bin/bash # Check if the j-modal component has accessibility features rg -A 10 -B 5 "j-modal.*fullscreen" rg -A 15 "interface.*Modal"
overflow: "auto", | ||
maxHeight: "300px" | ||
}} | ||
dangerouslySetInnerHTML={{ __html: svgElement.outerHTML }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address the XSS security concern with dangerouslySetInnerHTML.
While the SVG content comes from a trusted source (@viz-js/viz library), using dangerouslySetInnerHTML still poses potential security risks. Consider using a safer approach for SVG rendering.
Consider these safer alternatives:
- return (
- <div
- style={{
- padding: "10px",
- backgroundColor: "#f9f9f9",
- borderRadius: "4px",
- overflow: "auto",
- maxHeight: "300px"
- }}
- dangerouslySetInnerHTML={{ __html: svgElement.outerHTML }}
- />
- );
+ return (
+ <div
+ style={{
+ padding: "10px",
+ backgroundColor: "#f9f9f9",
+ borderRadius: "4px",
+ overflow: "auto",
+ maxHeight: "300px"
+ }}
+ ref={(div) => {
+ if (div && svgElement) {
+ div.innerHTML = '';
+ div.appendChild(svgElement.cloneNode(true));
+ }
+ }}
+ />
+ );
This approach directly manipulates the DOM with the trusted SVG element rather than parsing HTML strings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
dangerouslySetInnerHTML={{ __html: svgElement.outerHTML }} | |
return ( | |
<div | |
style={{ | |
padding: "10px", | |
backgroundColor: "#f9f9f9", | |
borderRadius: "4px", | |
overflow: "auto", | |
maxHeight: "300px" | |
}} | |
ref={(div) => { | |
if (div && svgElement) { | |
div.innerHTML = ''; | |
div.appendChild(svgElement.cloneNode(true)); | |
} | |
}} | |
/> | |
); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 144-144: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In ui/src/components/DebugStrings.tsx at line 144, avoid using
dangerouslySetInnerHTML to render the SVG due to XSS risks. Instead of injecting
the SVG as an HTML string, directly append or render the trusted SVG DOM element
using React refs or standard DOM manipulation methods to safely insert the SVG
into the component without parsing HTML strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/js/package.json (1)
59-59
: Version bump to^10.9.2
looks safe but pinning minor versions avoids surprise upgrades
^10.9.2
will track all10.x
releases. While ts-node generally follows semver, occasional regressions do slip through. If deterministic CI builds are critical, consider pinning to10.9.2
or using an npm lockfile checksum to freeze the exact version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/js/package.json
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (1)
tests/js/package.json (1)
21-21
: Transpile-only loader removes type-checking – confirm you still run a fulltsc
pass elsewhereSwitching the script from
ts-node/esm
tots-node/esm/transpile-only
speeds up execution but bypasses TypeScript type checks at runtime. Please ensure:
- A separate
tsc --noEmit
(or equivalent) step still runs in CI to catch type errors inutils/publishTestLangs.ts
.- Contributors are aware that this script will now execute even with type errors, which could surface only later in tests.
If such a safety net is missing, consider adding a dedicated type-check script before publishing test languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Language Association Logic Error
The logic for associating languages with perspectives was altered by removing the else
clause that checked p.neighbourhood.meta.links
for language references (where predicate
is 'language' and target
is the language address). This prevents languages used via meta links from being correctly identified as associated with perspectives, leading to them being incorrectly displayed as unused in the UI.
ui/src/components/Language.tsx#L87-L105
ad4m/ui/src/components/Language.tsx
Lines 87 to 105 in da3dc3a
const langs = await client!.languages.all(); | |
const perspectives = await client!.perspective.all(); | |
const tempLangs = []; | |
for (const lang of langs) { | |
const found = perspectives.find((p) => { | |
if (p.neighbourhood) { | |
if (p.neighbourhood.linkLanguage === lang.address) { | |
return true; | |
} | |
} | |
return false; | |
}); | |
tempLangs.push({ language: lang, perspective: found }); | |
} |
Bug: Enum Case Change and Typo Bug
The PerspectiveState
enum's string values were changed from SCREAMING_SNAKE_CASE
(e.g., "PRIVATE") to PascalCase
(e.g., "Private"), which is a breaking change for any code relying on the old string values. Additionally, the NeighboudhoodCreationInitiated
enum value contains a typo ("Neighboudhood" instead of "Neighbourhood"), causing inconsistency with other enum values and the corresponding Rust enum.
core/src/perspectives/PerspectiveHandle.ts#L4-L11
ad4m/core/src/perspectives/PerspectiveHandle.ts
Lines 4 to 11 in da3dc3a
export enum PerspectiveState { | |
Private = "Private", | |
NeighboudhoodCreationInitiated = "NeighboudhoodCreationInitiated", | |
NeighbourhoodJoinInitiated = "NeighbourhoodJoinInitiated", | |
LinkLanguageFailedToInstall = "LinkLanguageFailedToInstall", | |
LinkLanguageInstalledButNotSynced = "LinkLanguageInstalledButNotSynced", | |
Synced = "Synced", | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Add Debug Strings Functionality for Language Development and Debugging
Overview
This PR adds comprehensive debug strings functionality to AD4M, enabling language developers and field testers to monitor and debug language operations in real-time. The implementation includes backend debug string generation through a new callback interface, storage and retrieval systems, and a complete UI for viewing and analyzing debug information.
Features Added
🔧 Backend Implementation
LinkSyncAdapter Interface Extension
addDebugStringCallback
toLinkSyncAdapter
interfaceRuntime Service Integration
runtimeDebugStrings
query with optional language address filteringRuntimeService
for debug string managementP-Diff-Sync Implementation
🎨 Frontend Implementation
Debug Strings Component
DebugStrings.tsx
for displaying debug information across the appUI Integration
Technical Implementation
Backend Changes
LinkSyncAdapter Interface
Runtime Service
P-Diff-Sync Integration
Frontend Changes
RuntimeClient Extension
User Experience
For Language Developers
For Field Testers
Implementation Details
Debug String Flow
debugCallback
with DOT graph and operation typePerformance Considerations
Dependencies Added
@viz-js/viz
: Lightweight DOT graph rendering library for Graphviz visualizationTesting
Breaking Changes
None. This is a purely additive feature:
LinkSyncAdapter
interfaceFuture Enhancements
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores