Update theme formatting and add tests #1
Conversation
WalkthroughThe pull request updates the repository by modifying the Changes
Sequence Diagram(s)sequenceDiagram
participant JSON as "JSON Config"
participant Loader as "load_theme()"
participant Segment as "ThemeSegment"
participant Prompt as "get_theme_prompt()"
JSON ->> Loader: Parse theme JSON
Loader ->> Segment: Create segment, assign key from JSON
Segment -->> Loader: Return segment with key
Loader ->> Logger: Log segment key for debug
Prompt ->> Segment: Use stored key to generate placeholder
Prompt ->> Executor: Execute commands based on key reference
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/theme.c (2)
551-558: Consider logging segments lacking a key.Skipping segments that are disabled or have no key is correct. For diagnosability, you may want to log a warning or debug message if
segment->keyisNULLso that any missing or malformed segment definitions can be quickly identified.
566-639: Refactor repeated color code replacements.This block calls
str_replacemultiple times to substitute color placeholders. Extracting this repetitive logic into a helper function (e.g.,replace_color_placeholders()) could reduce code duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)include/nutshell/theme.h(1 hunks)src/utils/theme.c(3 hunks)tests/test_theme.c(3 hunks)themes/default.json(1 hunks)
🔇 Additional comments (7)
include/nutshell/theme.h (1)
15-17: Good addition of the key field to ThemeSegment structureThe addition of the
keyfield to store the segment name from JSON is a good improvement that supports the PR objective of enhancing theme formatting with segment keys for better identification..gitignore (1)
3-5: Appropriate updates to .gitignoreThese additions are appropriate for the project:
*.testensures test binaries aren't committed*.dSYM/excludes macOS debug symbol directoriesnutshellprevents the main executable from being committedThis follows best practices for C projects.
themes/default.json (1)
15-15: Enhanced prompt format with Git branch informationAdding the Git branch information to the left prompt format improves the shell experience by showing important contextual information to the user. The space between
{directory}and{git_branch}ensures proper formatting.tests/test_theme.c (2)
49-50: Good implementation of segment key in test themeThe addition of the key field for the directory segment aligns with the structure changes in
theme.h.
275-277: Enhanced debug output with segment keysAdding the segment key to the debug output improves the visibility of segment information during testing, making it easier to identify and troubleshoot issues related to specific segments.
src/utils/theme.c (2)
336-339: Ensure memory allocation checks for segment key.Storing the segment key with
strdupis correct and straightforward. However, consider adding a small defensive check for memory allocation failures (e.g.,if (!segment->key) { ... }) to maintain consistency and reliability throughout the codebase.
450-450: Properly frees the segment key.Freeing the
segment->keyfield ensures no memory leaks occur. This is consistent with the rest of the cleanup logic.
| // Use git_info as the segment key to match prompt format | ||
| theme->segments[1]->key = strdup("git_info"); // Set key to match prompt format | ||
| theme->segments[1]->format = strdup("{secondary}git:({branch}){dirty_flag}{reset}"); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistency between test and default theme segment keys
There's a naming inconsistency between the test theme and the default theme JSON. The test uses git_info as the segment key, but the default theme in themes/default.json uses git_branch.
🏁 Script executed:
#!/bin/bash
# Check for inconsistencies between test theme and default theme JSON
echo "Checking segment key naming in default.json:"
grep -A 5 "segments" themes/default.json | grep -A 10 "git"
echo "Checking left prompt format in default.json:"
grep -A 3 "left" themes/default.json | grep "format"
echo "Comparing with test theme usage:"
grep -n "git_info" tests/test_theme.cLength of output: 991
Action Required: Align Git Key Naming Consistency
The verification confirms that the test theme uses "git_info" (e.g., in the left prompt at line 30 and segment at lines 63–65 in tests/test_theme.c), while the default theme JSON (themes/default.json) still defines the key as "git_branch". Please update one of these to ensure consistency. For example:
- Option 1: Update
themes/default.jsonto use"git_info"for both the segment key and within the left prompt format. - Option 2: Revise the test theme in
tests/test_theme.cto reference"git_branch"instead of"git_info".
This change will resolve the naming conflict between the test and the default theme.
| // Create the placeholder from the segment key that was stored during load | ||
| char placeholder[256]; | ||
| snprintf(placeholder, sizeof(placeholder), "{%s}", segment->key); | ||
| THEME_DEBUG("Checking for placeholder: %s", placeholder); | ||
|
|
||
| // First check if this is a well-known segment | ||
| if (i == 0 && strstr(segment->format, "{directory}")) { | ||
| segment_key = strdup("directory"); | ||
| } else if (i == 1 && strstr(segment->format, "git:({branch})")) { | ||
| segment_key = strdup("git_info"); // Match the key used in the prompt format | ||
| } else { | ||
| // For other segments, try to infer from format but skip color codes | ||
| const char *colors[] = {"primary", "secondary", "success", "error", "warning", "info", "reset"}; | ||
| const char *format_str = segment->format; | ||
| const char *key_start = NULL; | ||
| const char *key_end = NULL; | ||
| // Only process if the placeholder exists in the prompt | ||
| if (strstr(left_prompt, placeholder)) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Risk of buffer overflow for long segment keys.
Using a fixed buffer (char placeholder[256]) and snprintf(placeholder, sizeof(placeholder), "{%s}", segment->key); risks truncation or buffer overflow if a malicious or accidentally large key is encountered. Consider either validating the maximum length of segment->key or dynamically allocating the buffer.
Please run the following script to see if there are any notes or constraints on the maximum key length in the codebase or related documentation:
🏁 Script executed:
#!/bin/bash
# Scours the codebase for references to 'max key length' or 'segment key length'
rg -i "max key length|segment key length|MAX_KEY_LEN|segment->key"Length of output: 486
Action Required: Mitigate Buffer Overflow Risk in src/utils/theme.c
The investigation did not reveal any constraints on the length of segment->key anywhere in the codebase. As a result, using a fixed-size buffer (char placeholder[256]) for constructing the placeholder poses a real risk of data truncation and potential buffer overflow if a very long key is encountered.
Recommended Actions:
- Validate Length: Enforce a maximum length on
segment->keybefore using it to format the placeholder. - Dynamic Allocation: Alternatively, dynamically allocate the buffer with the exact size needed to avoid hard-coded limits.
Implementing one of these measures will help eliminate the risk associated with processing unusually long segment keys.
Improve theme formatting by adding a segment key for better identification and updating the prompt format. Include tests to ensure the new functionality works as intended.
Summary by CodeRabbit
New Features
Chores